Repository navigation
feat(orcad): ship the mobile web page so phones paired to orcad get it - #29882
Conversation
orcad never carried out/mobile-web, so it did not advertise mobileWeb.bundle.v1 and phones paired directly to an orcad server fell back to the app's built-in native screens. The built page now ships in every orcad package at out/mobile-web, the path getBundledMobileWebBundleRoot already probes under the install root. Its manifest is a fixed artifact; its content-addressed assets join the hashed artifact list from that manifest, as one shared copy in the template's common files. build-orcad fails when the page is missing or stale, and CI lanes that build orcad now build the page first.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
The description is detailed and explains the user impact, implementation, rationale, compatibility, testing, and size impact. However, it omits the required Linked Issue and Visual Proof sections, and… | Add an actual linked issue under “Linked Issue” or document the maintainer exception. Add before-and-after visual proof, or write N/A with a reason if no visual proof applies. Complete the required checklist and state the AI disclosure st… |
| Docstring Coverage | |
Docstring coverage is 54.17% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 23 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the primary change: shipping the mobile web page in OrCAD packages for paired phones. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Description check
-
Explanation
The description is detailed and explains the user impact, implementation, rationale, compatibility, testing, and size impact. However, it omits the required Linked Issue and Visual Proof sections, and it does not include the repository checklist or explicit AI disclosure status.
Resolution
Add an actual linked issue under “Linked Issue” or document the maintainer exception. Add before-and-after visual proof, or write
N/Awith a reason if no visual proof applies. Complete the required checklist and state the AI disclosure status when applicable.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
7f37ba4c-3943-4a37-b873-ed1d6b07589c
📒 Files selected for processing: 29
.github/actions/build-mobile-web-bundle/action.yml.github/workflows/e2e.yml.github/workflows/node-server-tests.yml.github/workflows/pr.yml.github/workflows/ssh-hostile-hosts.yml.github/workflows/ssh-windows-hosts.yml.github/workflows/win-orcad-serve-switch-e2e.yml.github/workflows/wsl-orcad-e2e.ymlconfig/scripts/build-orcad-template.mjsconfig/scripts/build-orcad.mjsconfig/scripts/node-server-change-scope.test.mjsconfig/scripts/orcad-artifact-version.mjsconfig/scripts/orcad-artifact-version.test.mjsconfig/scripts/orcad-mobile-web-bundle.mjsconfig/scripts/orcad-mobile-web-bundle.test.mjsconfig/scripts/orcad-template-test-fixture.mjsconfig/scripts/orcad-terminal-smoke-change-scope.test.mjsconfig/scripts/release-cut-token-permissions.test.mjsconfig/scripts/verify-packaged-orcad-template.cjsconfig/scripts/verify-packaged-orcad-template.test.mjssrc/main/orcad/orcad-artifact-identity.tssrc/main/orcad/orcad-artifact-materializer.test.tssrc/main/orcad/orcad-artifact-materializer.tssrc/main/orcad/orcad-artifact-preflight.test.tssrc/main/orcad/orcad-runtime-entry-artifacts.test.tssrc/main/runtime/rpc/methods/mobile-web-bundle.test.tssrc/main/ssh/remote-install-model.tssrc/shared/orcad-artifacts.test.tssrc/shared/orcad-artifacts.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.
The guard already refuses unlisted files, so staging copies the bundle directory as native-chat-visuals is copied. The shipped template takes the page the qualification lanes tested, the e2e build artifact carries it, and the PR smoke builds it through install-mobile-dependencies behind its own scope step. Drops tests that duplicated the guard's own.
There was a problem hiding this comment.
🔇 Additional comments: 9
.github/workflows/e2e.yml: 1
137-139: Zizmor self-repository hint is a style warning only.The
./.github/actions/...reference matches how the repository references its other local actions. ThehashFilesguard also supports older refs. No change is needed..github/workflows/node-server-tests.yml: 2
334-334: LGTM!Also applies to: 464-464, 522-526
563-567: LGTM!.github/workflows/pr.yml: 1
415-442: LGTM!config/scripts/node-server-change-scope.test.mjs: 1
577-577: LGTM!config/scripts/orcad-mobile-web-bundle.test.mjs: 1
1-6: LGTM!Also applies to: 50-58
config/scripts/orcad-terminal-smoke-change-scope.test.mjs: 1
95-115: LGTM!config/scripts/pr-preflight-gates.test.mjs: 1
246-246: LGTM!config/scripts/node-server-native-compat-scope.test.mjs: 1
331-334: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.The assertion relies on fragile string slicing.
floor.if.slice(0, -3)assumes the expression ends with exactly}}. The.replace(...)call also assumes the exact text and whitespace of the inserted clause. If the workflow text is reformatted,replacesilently does nothing, and the test then fails with a confusing message. This is acceptable now, but it is brittle.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
ac3fd554-4c8a-4fdf-86db-5bad78e811ef
📒 Files selected for processing: 11
.github/workflows/e2e.yml.github/workflows/node-server-tests.yml.github/workflows/pr.ymlconfig/scripts/node-server-change-scope.test.mjsconfig/scripts/node-server-native-compat-scope.test.mjsconfig/scripts/orcad-mobile-web-bundle.mjsconfig/scripts/orcad-mobile-web-bundle.test.mjsconfig/scripts/orcad-terminal-smoke-change-scope.test.mjsconfig/scripts/pr-preflight-gates.test.mjssrc/main/orcad/orcad-artifact-materializer.tssrc/shared/orcad-artifacts.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.
What was missing
orcad packages never carried the mobile web page (
out/mobile-web). So an orcad server did not advertisemobileWeb.bundle.v1, answeredmobileWeb.bundle.manifestwithmobile_web_bundle_unavailable, and phones paired directly to it (orca serve --mobile-pairingon orcad, managed SSH servers, WSL, local serve) got the app's old built-in screens instead of the page. This blocks deleting that native fallback later.What changed
src/shared/orcad-artifacts.ts:out/mobile-web/manifest.jsonis a required orcad artifact. The page's content-addressed assets join the hashed artifact list from that manifest (orcadMobileWebAssetFilenames), soorcadArtifactFilenamesandorcadTemplateCommonFilenamesnow take that asset list. The page is a common template file (one copy for all targets).config/scripts/orcad-mobile-web-bundle.mjs:build-orcadcopies the built page tree afterassertMobileWebBundleBuilt(the same guard electron-builder's beforePack uses, which also refuses unlisted files). A missing or staleout/mobile-webfails the build.verify-packaged-orcad-template.cjspin the template's common inventory to the page manifest and checksum every asset. The materializer reads the asset names from the template manifest (no extra file read on the WSL trusted-record path) and verifies each against its digest. Install identity (readOrcadArtifactIdentity,computeOrcadFullVersion) hashes every asset.getBundledMobileWebBundleRootalready probes<appPath>/out/mobile-web.build-mobile-web-bundlecomposite action (which usesinstall-mobile-dependenciesand its lockfile drift guard). Innode-server-tests.ymla newmobile_web_bundlejob builds it once; the glibc 2.28 and Alpine container lanes and the release template job download that artifact, so the shipped template carries the page the lanes tested. Ine2e.ymlthe shared build job builds it intoe2e-build-out. The PR orcad smoke builds it only when its own scope step says the smoke runs. The Windows SSH hosts sparse checkout now includesmobile/.Why this design
It reuses the built page, the existing packaging guard, and the existing resolver path. The asset list comes from the page's own manifest rather than a fixed list, because asset names change every build, and every asset stays under the same per-file sha256 checks as the rest of the artifact set.
Size
The page is about 8.4 MB raw (130 assets). It adds that once to the desktop's orcad template (common files, not per target) and to each materialized slot and SSH/WSL upload. A local single-target template went to 35 MB with the page included.
Known follow-up, not in this PR: the desktop installer now carries the page twice (in the app and in the orcad template). In a release those two copies can also differ byte for byte, since esbuild chunk hashes depend on the build path, so deduping them would also give desktop and orcad hosts one buildId.
Mixed versions
No wire change: the capability and RPCs already exist and old phones already handle them from desktop hosts. Older orcad servers keep serving no page, and newer phones handle that as they do now. The SSH install dir is keyed by the content hash, which now includes the page, so an updated desktop installs a fresh dir instead of reusing an older one.
Testing
pnpm tc, changed-code quality gate, oxfmt.build-orcad-node.mjswithout/mobile-webmoved away fails with "no bundle manifest ... Run pnpm build:mobile-web".--mobile-pairing:mobileWeb.bundle.v1,mobileWeb.bundle.manifestreturns buildId62dbc11e...with 130 assets / 8366243 bytes. The phone shows "Downloading the latest app ... of 8.4 MB", then the page-rendered workspace list. Network diagnostics show "Web shell: downloading app 130 assets", "activating app", "page painted source=download build=62dbc11e".mobile_web_bundle_unavailable, no download screen, native workspace list, and diagnostics stop at "Authenticated" with no web shell lines.build-orcad-template.mjs --targets darwin-arm64puts 131out/mobile-web/*entries incommonSha256. A slot materialized from it withassembleOrcadArtifactinto a temp cache has the page, its identity matches its.version, and orcad started from that slot advertises the capability and serves the same buildId.