test(mobile): certify the browser pane's golden families and render it in a page (OTA phase C, C6.5) by Jinwoo-H · Pull Request #21777 · stablyai/orca

Jinwoo-H · 2026-09-20T09:54:16Z

	Files	Added	Deleted	Net
Test	7	$\color{#1a7f37}{\Huge{\mathbf{+}}}$1105	$\color{#cf222e}{\Huge{\mathbf{−}}}$3	$\color{#1a7f37}{\Huge{\mathbf{+}}}$1102
Prod	5	$\color{#1a7f37}{\Huge{\mathbf{+}}}$275	$\color{#cf222e}{\Huge{\mathbf{−}}}$26	$\color{#1a7f37}{\Huge{\mathbf{+}}}$249

Summary

Phase C, C6.5: the browser pane's certification and render check (design-ota-c6.md §3, §6; rulings rulings-ota-c6.md 2, 3, 4). Commits on 96c1dd8b70 (C6.1 f5d2d6e757, C6.2 6f0fb3fe39 with the pin fix 4e9d5b577e, and C6.3+C6.4 all ancestors), inert: no route registered, no manifest change, nothing under mobile/app/ moves.

commit	what
`5cabdb4424`	the job step's runtime recorded in its comment where someone would change it (25–30 s wall for the step, 2.5 s of it the sweep encoding 111 noise JPEGs in Chromium, one per viewport the budget fits: adding rows is a runtime decision)
`c251b01fa9`	round 1 and pullfrog: the `mobile_web_app` CI job ran a hand list of ten `config/scripts` files, and it is the only job with `ORCA_MOBILE_WEB_APP_DEPS_REQUIRED=1`, so a census whose closure block skips without the flag was green in the sharded `test` job whether or not it ever ran there. The list is replaced by two quoted vitest filename filters (`config/scripts/mobile-web-app-` and `config/scripts/build-mobile-web-app-bundle.test.mjs`; vitest matches a positional as a substring against discovered files, a glob finds nothing and a regex breaks on backslashes), the same prefix `pr-code-change-scope.mjs` fires the job on. The job now runs 18 files / 205 cases against 10 files before; 111 of the 205 skip without the deps, which is what only this job runs. Cases CI had never run: this PR's render check (7) and sweep (4), source-control external links (9), source-control keyboard (6), C4.2's source-control text inputs (6 of 19), route manifest (2 of 17). The sweep is renamed `mobile-web-app-frame-budget-sweep.test.ts` and gated on the deps, because as named before it matched neither filter and, ungated, would have launched Chromium in the sharded job. CodeRabbit's nit taken twice: `postedFrame` in the ruling-2 pin and in the sweep threw nothing when a subscribe opened no stream, which read as zero posts, indistinguishable from a drop; both throw now
`c73b405f81`	round 1: the shell double reproduces all three arms of `canCarry` (message size, unacked frames, unacked bytes) and consumes the page's acks as the host does; its caps are read from `bridge-caps.ts` and `bridge-host-subscriptions.ts`, never retyped; one case pushes thirty ~200 KB frames (about 6 MB through the 4 MiB window, nothing over the message cap) and every frame posts with the ack seqs read back; file docstring corrected; dead array deleted
`71254ab3a9`	round 1: Zod's JIT probe (`new Function('')`) filed a `script-src` violation on every page load. `z.config({ jitless: true })` at the page entry is too late (measured: `$ZodObject` reads `allowsEval` at schema construction, and esbuild evaluates Zod's chunk before any of ours), so the flag is set through `globalThis.__zod_globalConfig` in the esbuild banner beside the process shim; the eval filter is gone and every render case asserts zero CSP violations, including one that walks load and first paint
`fa933248e4`	round 1: `WORST_CASE_JPEG_BYTES_PER_PIXEL = 0.545` came from one 2400×2160 frame and was not a worst case: at that value 90 of 143 viewports posted over the cap and 59 of the 111 the budget claims to fit were dropped outright by the real shell, 390×712 at scale 1.8 among them. A sweep of 143 viewports (widths 320–1400, heights 480–1600), each encoded by Chromium at the scale the real budget picks, measured 0.54470–0.55351 B/px across the 111 the budget fits; the constant is 0.56 (the maximum plus ~1.2%), with the sweep, range, margin and date in its docstring. It is a fixed point: 0.555, 0.56 and 0.565 all leave the same 31 viewports over the cap, every one at the scale floor of 1 where the module refuses to go blurrier and ruling 1's drop rule owns the outcome; the sweep asserts both halves. Ruling 2's older case now says it measures the expansion and the drop rule and cannot falsify the constant
`d96133774c`	`c6-browser-closure-families.ts`, the half pin: 4 families / 15 goldens (`browser.dialog` 3, `browser.keyboard` 3, `browser.pointer-click` 6, `browser.wheel` 3), 6 identical / 9 `result-absent-settlement`, every verdict measured per family with vitest `-t` over the full 787-golden corpus at this base, with C1's 103 in 22 families reproduced as the control (0 mismatches); C2's rule predicts 15 of 15. No composed `c6-page-closure.ts` (ruling 3: C7 composes it with its route). The derivation census did not wait for a route: `mobileWebAppRouteClosure` is now one case of `mobileWebAppModuleClosure`, so the pane's closure is read from the module; the pane alone reaches exactly the four families, and beside `app/h/_layout` it adds exactly those four with the layout reproducing C1's 22. The design's "23 local modules" is stale: 48 local alone, 34 beyond the layout, 30 under `src/browser` and four through the `.web` siblings C6.2 and C6.3 added; the pin carries the measured number. The docstring says `browser.screencast` has no golden, so the pin certifies the input path only
`e7cd24ef10`	ruling 2's pin against the real encoder: a frame at CDP's nine metadata fields with a real timestamp, encoded by C6.1's encoder and serialized by the real `BridgeHostSubscriptions`, costs 303 bytes beside the image against C6.3's 516 B bound, and the bound is reconstructed exactly rather than only held above (holding above alone would tolerate 213 bytes of shell-side growth); a generated noise frame at the budgeted mobile-mode scale posts at 655,147 B, 213 under the cap
`1a51d7b53f`	a number in the previous commit's message (654,857) came from arithmetic, not the harness; the measured figure (655,147) now lives in the file where it was measured
`030a0fd93f`	`config/scripts/mobile-web-app-browser-pane-render.test.mjs`, six cases through `bundleMobileWebApp({ appDir })` on a one-route scratch tree (ruling 4): the pane subscribes with `wantsBinary` and paints; a second frame flips the double buffer; an over-cap frame is dropped by the shell double and the next paints on the same subscription (ruling 1 end to end); the grant withheld gives "Update the Orca app to stream browser tabs here." and no `subscribe` at all; a tap issues one `browser.mouseClick` at (194, 356) against a 390×712 source; no request leaves the bundle's origin. Frames are noise JPEGs the page encodes: 390×698 is 201,924 base64 chars, the phone's mobile mode 780×1424 is 811,168 (124% of the cap), 2400×2160 is 3,761,580 (574%)
`ade4729386`	round 2: the sweep and the render check encoded their fixtures at a retyped 0.72 while the pane ships `BROWSER_FRAME_QUALITY`; both now read it (reproduced: at 90 every budgeted viewport posts over the cap, and the sweep reds), so the certification follows the shipped number. The three render cases without a CSP assertion gained one, so "every render case" is now seven of seven; the sweep pins the 32 of 143 viewports left at scale 1 (31 of them over the cap, as before); references to a renamed sweep file and a never-existing `page-zod-jitless.ts` corrected; the shim-count comment made count-agnostic. The double's unacked-frame and unacked-byte arms are exercised by no render case (the shell's own are pinned in `bridge-host.test.ts`); left as an open item. Also on the branch: `2a1c3dfc38`, the sweep's mobile imports made lazy behind the dependency guard, after CI's sharded job failed the file at load (vite reads `mobile/tsconfig.json`, which extends an Expo base the shard never installs)

Red first

Pin: dropping browser.wheel from the table reds both census cases naming it.
Envelope: dropping the metadata widening reds four cases in the new file and two in C6.3's suite, the sharpest being the real shell answering the budgeted frame with zero posts; adding a field to the shell's envelope reds the reconstruction (516 against 548) while C6.3's own suite stays green on all 14, which is why the exact reconstruction exists.
Render check, by reverting C6.2 behaviour: stubbing the decode probe reds the flip case on two identical digests; pointing the image write at the host instead of the surface child reds paint and flip. The first also corrected a comment claiming the paint case proved decode-then-flip; the flip case owns that claim.

Found, not fixed

The page bundles two copies of Zod (mobile's 4.4.3 and the repo root's 4.5.4, because src/shared/zod-salvage.ts resolves upward): 808 KB of duplicate source and two module instances. Aliasing zod in the builder fixes it (measured), but it changes which Zod the shipped page runs, so it is its own PR, not a CSP fix.

(The Zod JIT-probe CSP violation found in round 0 is fixed in 71254ab3a9.)

Verified

Lane gates on 030a0fd93f, sequential, exit codes from the command: mobile tsc, ratchet (737 files), whole-tree oxlint, oxfmt, root oxlint and oxfmt, full mobile suite (8348 passed, no Errors line), changed-code quality (9 files, 0 findings), react-doctor, merge-tree clean with every changed file under its cap, machine-path grep clean. The lane saw one config/scripts red (agent-status-hot-path-benchmark.test.ts, an alias it could not resolve) that reproduced for it on pristine main in its tree; the lead's tree runs that suite green.
Round 1 (on 030a0fd93f): changes requested; folds in fa933248e4, 71254ab3a9, c73b405f81 and c251b01fa9.
Round 2 runs on c251b01fa9; 5cabdb4424 above it is a four-line comment in the workflow file.
Lead reproduction: verify-gates.sh on 030a0fd93f, c73b405f81 and c251b01fa9 against 96c1dd8b70 (mobile 741/8348 each, config/scripts 2069, 2074 and 2073 with one red on the untouched owner-loss temp-file flake that passes alone, all else exit 0, machine paths 0; merge-tree clean); on c73b405f81, restoring 0.545 reds the sweep. Earlier: verify-gates.sh on 030a0fd93f against 96c1dd8b70 (mobile 741/8348, config/scripts 225 files / 2069 passed, all exit 0, machine paths 0; merge-tree clean); dropping browser.wheel from the pin reds the census cases.

Part of the mobile OTA plan (Phase C). Reference PR #13386, never merged.

The half pin for C6: 4 families, 15 goldens, every verdict the one C2's rule predicts. Measured per family with vitest `-t` over the full 787-golden corpus, with C1's 103 reproduced golden-for-golden as the control: 6 byte-identical, 9 result-absent-settlement. No composed `c6-page-closure.ts`: a composed table is pinned against a route and the browser is a pane, so C7's route is what composes this with C1's. The derivation census does not wait for that route. `mobileWebAppRoute- Closure` becomes one case of `mobileWebAppModuleClosure`, which takes any entries, so the pane's own closure can be read from the module. Two cases: the pane alone reaches exactly the pinned four, and the pane beside `app/h/_layout` adds exactly those four and no other, with the layout reproducing C1's 22 as the control for the difference. Closure at this base: 48 local modules alone, 34 beyond the layout, 30 under `src/browser` and four through the web siblings. The design said 23, all under `src/browser`; it was measured before C6.2 and C6.3 added those siblings, so the pin carries the re-measured number. `browser.screencast` has no golden at all, so this certifies the input path and says nothing about the frame path. Red first: with `browser.wheel` dropped from the table, both census cases fail naming the missing family; restored, the file's 10 cases pass and the parity suite reports "15 goldens in 4 families, 6 byte-identical". Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

Ruling 2's pin. `binaryEventEnvelopeBytes()` sizes the mobile view's device scale from a skeleton it builds itself, and until now its only check was another skeleton of the same shape in the same file: two copies of one assumption agreeing with each other. This measures the real thing. A frame with CDP's nine metadata fields and a real `Page.screencastFrame` timestamp, encoded by C6.1's `encodeBridgeScreencastFrame` and serialized by the real `BridgeHostSubscriptions`, posted through the host harness: 303 bytes besides the image, against a bound of 516. Held above is not enough on its own — 213 bytes of slack is room for the shell to grow the envelope by a field the page never hears about — so the bound is reconstructed exactly instead. Every byte of that slack is a number this frame prints narrower than a double can; adding those back gives 516 on the nose. The budget cases run a generated noise image at the budgeted scale, not a committed fixture: the worst case is the image JPEG compresses least, and a photograph sits a tenth of the way to it. 901,161 px at 0.545 bytes per pixel is 491,132 bytes, which the shell posts at 654,857 of the 655,360-byte cap. One envelope more and the shell drops it, which is ruling 1 read from the budget's side. Red first, two ways. Drop the metadata widening from the bound and three cases fail, the sharpest being the real shell answering the frame the page thought it could send with zero posts. Add a field to the shell's own envelope and the reconstruction fails at 516 against 548, where the existing suite stays green on all 14 — which is the drift this file exists for. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

The previous commit message says the shell posts 654,857 bytes for a frame at the budgeted area. That number was not measured; I wrote it from the budget arithmetic instead of reading it off the harness. The measured value is 655,147, which is 213 under the cap rather than 503. Nothing in the assertions changes — they compare against the cap and the bound, never against a literal — but the figure now lives in the file where it was measured rather than only in a message that has it wrong. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

The only place C6's whole frame path runs. Every other check reads one half: the shell suites drive the host with no page, the page suites drive the hooks with no shell, and the parity pin certifies the input path from a recording. Ruling 4: no route is added. `bundleMobileWebApp` already takes an `appDir`, so this builds a one-route tree of its own and nothing under `mobile/app` moves. The shell double grows a screencast lane to serve it: it accepts a subscribe, posts `event.binary`, and prices each frame the way `BridgeHostSubscriptions` does, so an over-cap frame is dropped where the page can watch the stream survive it. Six cases: the pane subscribes with `wantsBinary` and paints the frame it is handed; a second frame flips the double buffer; an over-cap frame is dropped and the next one paints on the same subscription; the grant withheld produces the update-the-app copy and no subscribe at all; a tap issues one `browser.mouseClick` at the centre of the source viewport; and no request leaves the bundle's own origin. Measured. The frame the pane asks this viewport for is 390x698, which as noise is 201,924 base64 characters. The phone's mobile-mode frame is 780x1424 and encodes to 811,168, which is 124% of the cap and the reason the area budget exists; the over-cap case uses 2400x2160 at 3,761,580, 574% of it. The tap maps to (194, 356) against a 390x712 source, one device pixel off centre because the rendered width is 382.33 CSS pixels for 390 source pixels. One finding, recorded rather than fixed because it is not the pane's. The page files a CSP `script-src` violation on every load, on any route: Zod 4 feature-detects its compiled path with `new Function('')`, the shell's `script-src 'self'` blocks it, Zod catches the throw and takes the interpreted path. The page is correct and the report is filed anyway. One case names it so a second `eval` is visible, and every other case asserts no violation beyond it. Red first, twice, both by reverting behaviour C6.2 landed. Stub out the decode probe in `whenBrowserFrameDisplayable` and the flip case fails on two identical frame digests. Point `updateBrowserImageSource` at the host element instead of the surface child and the paint and flip cases both fail. The first case's comment is corrected by the first of those: it claimed a visible layer proved the decode-then-flip, and the frame still paints with the probe gone, so the flip case is what proves it. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

coderabbitai · 2026-09-20T10:07:42Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: db9b70c6-d24b-4f0f-92f6-e0b5578ffc49

📥 Commits

Reviewing files that changed from the base of the PR and between 2a1c3df and ade4729.

📒 Files selected for processing (6)

config/scripts/build-mobile-web-app-bundle.test.mjs
config/scripts/mobile-web-app-browser-pane-render.test.mjs
config/scripts/mobile-web-app-frame-budget-sweep.test.ts
config/scripts/mobile-web-app-render-harness.mjs
mobile/src/browser/browser-screencast-budget-at-the-shell.test.ts
mobile/src/browser/browser-screencast-request.web.ts

🚧 Files skipped from review as they are similar to previous changes (2)

mobile/src/browser/browser-screencast-budget-at-the-shell.test.ts
mobile/src/browser/browser-screencast-request.web.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.

📝 Walkthrough

Walkthrough

The PR generalizes mobile web bundle closure construction and adds a Zod jitless banner. It updates the render harness to enforce bridge message and acknowledgement windows. New Chromium tests cover browser-pane painting, binary subscriptions, frame drops, acknowledgements, clicks, and CSP behavior. Additional tests measure screencast budgets across 143 viewports. C6 browser closure fixtures and parity checks are added. CI now discovers mobile web app tests by filename prefix.

Priority: ➖ Normal

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description provides extensive technical context, testing details, and scope information, but it does not follow the required template. It omits or does not clearly provide the ELI5, What Changed,…	Restructure the description using the repository template. Add the missing required sections and complete their checkboxes. Provide a valid linked issue in the `Fixes #...` section, attach before-and-after visual proof or state `N/A` with a…

✅ Passed checks (4 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 12 files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the primary changes: browser-pane golden-family certification and page rendering for OTA Phase C6.5.

Full details: Description check

Explanation

The description provides extensive technical context, testing details, and scope information, but it does not follow the required template. It omits or does not clearly provide the ELI5, What Changed, Why, Linked Issue with a Fixes reference, Visual Proof, Testing checklist, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist sections.

Resolution

Restructure the description using the repository template. Add the missing required sections and complete their checkboxes. Provide a valid linked issue in the Fixes #... section, attach before-and-after visual proof or state N/A with a reason, document manual and automated testing with supported platforms, complete the required checklist, and address security, cross-platform, SSH, mobile, compatibility, and performance considerations.

✨ Finishing Touches

📝 Generate docstrings

Commit to this branch
Create a new PR

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

🧹 Nitpick comments (3)

mobile/src/browser/browser-screencast-budget-at-the-shell.test.ts (1)
98-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Fail loudly when the subscribe opens no binary stream.

bridge.client.streams[0]?.emitBinary?.(...) swallows the case where the subscribe produced no stream or no binary emitter. posts is then 0 for a reason unrelated to the shell. The ceiling case at lines 166-172 asserts exactly posts === 0, so it would stay green without the shell ever seeing a frame.

json has the same shape of problem: when nothing was posted, bridge.posted.at(-1) returns the earlier subscribe post, so bytes and envelope describe an unrelated frame.
♻️ Proposed fix
   const before = bridge.posted.length
-  bridge.client.streams[0]?.emitBinary?.(screencastFrame(image))
-  const json = bridge.posted.at(-1) ?? ''
-  const posts = bridge.posted.length - before
+  const stream = bridge.client.streams[0]
+  if (stream?.emitBinary === undefined) {
+    throw new Error('the subscribe opened no binary stream')
+  }
+  stream.emitBinary(screencastFrame(image))
+  const posts = bridge.posted.length - before
+  const json = posts === 0 ? '' : (bridge.posted.at(-1) ?? '')
config/scripts/mobile-web-app-browser-pane-render.test.mjs (2)

45-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Read the frame cap from the bridge caps module.

FRAME_CAP_BYTES duplicates BRIDGE_MAX_MESSAGE_BYTES. If the authoritative cap changes, this test can continue to pass while exercising the wrong threshold. Read BRIDGE_MAX_MESSAGE_BYTES from mobile/src/mobile-web-shell/bridge/bridge-caps.ts with the existing source-parsing approach.

22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Declare playwright-core as a development dependency.

config/scripts/mobile-web-app-browser-pane-render.test.mjs imports an undeclared package. The current pnpm configuration hoists transitive dependencies, but an isolated install may not resolve this import. Add playwright-core to devDependencies.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 562a13b4-c098-4684-af73-0fe80474628a

📥 Commits

Reviewing files that changed from the base of the PR and between 96c1dd8 and 030a0fd.

📒 Files selected for processing (9)

config/scripts/build-mobile-web-app-bundle.mjs
config/scripts/mobile-web-app-browser-pane-render.test.mjs
config/scripts/mobile-web-app-page-closure-families.test.mjs
config/scripts/mobile-web-app-render-harness.mjs
mobile/src/browser/browser-screencast-budget-at-the-shell.test.ts
mobile/src/browser/browser-screencast-request.web.ts
mobile/src/test-support/bridged-parity/c6-browser-closure-families.test.ts
mobile/src/test-support/bridged-parity/c6-browser-closure-families.ts
mobile/src/test-support/rpc-recording/rpc-recording-through-bridge.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

pullfrog

Important

The new browser-pane render check is added but not wired into the one CI job that can actually run it, so it silently skips everywhere. Please add it to the mobile_web_app job's file list before merging.

Reviewed changes

build-mobile-web-app-bundle.mjs — mobileWebAppRouteClosure(route) is generalized to mobileWebAppModuleClosure(entryModules); the route closure is now one case of it. The per-entry .tsx? strip preserves the prior entryPoints behaviour exactly.
c6-browser-closure-families.ts (+ .test.ts) — authored 4-family / 15-golden pin table for the browser pane, with no composed c6-page-closure.ts (C7 composes it with its route). The self-consistency, class-total, per-family-identical and disjointness assertions are all present.
mobile-web-app-page-closure-families.test.mjs — reads the pane's closure from the module itself, asserts it reaches exactly the four pinned families, and that adding it beside app/h/_layout contributes exactly those four while the layout still reproduces C1's 22.
browser-screencast-budget-at-the-shell.test.ts — makes binaryEventEnvelopeBytes() evidence by measuring a real encoder+host frame and reconstructing the 516-byte bound to the byte from the 303-byte real envelope (I verified the widening arithmetic sums to 213).
mobile-web-app-render-harness.mjs — binary stream subscription support and an emitBinary that prices the JSON envelope against the frame cap. Its drop rule (> cap, seq not advanced on drop) matches BridgeHostSubscriptions.canCarry / deliverBinaryFrame.
mobile-web-app-browser-pane-render.test.mjs — six Playwright cases over a scratch one-route tree: CSP probe, binary subscribe + paint, double-buffer flip, over-cap drop with stream survival, withheld grant, and tap geometry.
browser-screencast-request.web.ts — docstring now points at the test that makes the bound evidence.

⚠️ The new render check never runs in CI

config/scripts/mobile-web-app-browser-pane-render.test.mjs is missing from the explicit vitest file list in .github/workflows/pr.yml:711-721. The root config includes config/scripts/**/*.test.mjs, so the sharded test job does collect it — but that job installs no mobile dependencies, so mobileWebAppDependenciesPresent() is false and describePane becomes describe.skip. The mobile_web_app job is the only place with mobile/node_modules and Chrome, and it enumerates every render check by name; the new one is not among them. Net effect: the check this PR is built around executes nowhere, which is exactly the failure mode that job's ORCA_MOBILE_WEB_APP_DEPS_REQUIRED guard exists to prevent. pr-workflow-parallelism.test.mjs only asserts the env var is set on the step, so it does not catch the omission.

Technical details

# The new render check is not in the mobile_web_app job's file list

## Affected sites
- `.github/workflows/pr.yml:711-721` — the `pnpm exec vitest run` step lists each `config/scripts/mobile-web-app-*-render.test.mjs` by hand; `config/scripts/mobile-web-app-browser-pane-render.test.mjs` is absent.
- `config/scripts/mobile-web-app-browser-pane-render.test.mjs:107` — `describePane = bundles ? describe : describe.skip`; with no `mobile/node_modules/react-native-web` the whole suite skips.
- `.github/workflows/unit-tests.yml:31-70` — the sharded `test` job installs only node/cloud deps, so the file runs there and skips.

## Required outcome
- The new render check executes against real `mobile/node_modules` and Chrome on the `mobile_web_app` job, so a regression in the pane's paint/subscribe path fails CI.

## Suggested approach (optional)
- Add `config/scripts/mobile-web-app-browser-pane-render.test.mjs \` to the list at `pr.yml:711-721`. The `mobile_web_app` path filter already matches it (`config/scripts/mobile-web-app-` is one of `MOBILE_WEB_APP_PREFIXES`), so only the file list needs the entry.

## Open questions for the human (optional)
- Should `pr-workflow-parallelism.test.mjs` assert that every `config/scripts/mobile-web-app-*-render.test.mjs` on disk appears in the job list, to stop this class of omission recurring?

^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

The reviewer is right, and it is worse than the report says. At 0.545 bytes per pixel, 90 of 143 viewports posted a frame over the cap, and 59 of the 111 the budget claims to fit were dropped outright by the real shell — 390x712 at scale 1.8 among them. The old number came from one 2400x2160 frame. A single large frame is the cheapest per pixel in the whole range, so a worst case measured there is not a worst case anywhere else. Swept 143 viewports, widths 320 to 1400 and heights 480 to 1600, each encoded by Chromium at the scale the real budget picks for it. Across the 111 the budget fits, the cost ranges 0.54470 to 0.55351 bytes per pixel. The constant is now 0.56: that maximum plus 0.00649, about 1.2%, for the encoder version it was not swept on. The docstring carries the sweep, the range, the margin and the date. 0.56 is a fixed point, not a guess. Raising the constant shrinks the budget, which lowers the scale, which moves the cost; 0.555, 0.56 and 0.565 all leave the same 31 viewports over the cap, and every one of those sits at the scale floor of 1, where the module already declines to go blurrier and C6 ruling 1's drop rule is the protection. The new test asserts both halves: nothing the budget fits goes over, and the largest viewport it cannot fit is dropped by the real shell. The sweep lives in `config/scripts` because it needs Chromium: the frames are CDP screencast frames, so Chromium's encoder is the oracle and a Node JPEG library would calibrate against the wrong bytes. It drives the real budget, the real scale function and the real `BridgeHostSubscriptions`, and runs in about 4 seconds. Ruling 2's block in `browser-screencast-budget-at-the-shell.test.ts` now says plainly what it measures. It feeds `noise(area * theConstant)`, a byte count the constant itself produced, so it can falsify the expansion and the drop rule but never the constant. It read as if it validated the worst case, and it did not. Red first: put 0.545 back and the sweep fails with 59 viewports, each naming its scale and reporting `null` — the real shell dropping the frame rather than posting it over the cap. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

The page filed a CSP `script-src` violation on every load: Zod decides whether it may compile by constructing `new Function('')` and reading the throw as "no JIT here", the shell's `script-src 'self'` is exactly that throw, and the browser reports it before Zod catches it. Zod's own source gates the probe on `jitless` for this case. `z.config({ jitless: true })` at the entry does not work, and the reviewer's suggestion of putting it there was measured losing the race. `$ZodObject` reads `allowsEval` when a schema is constructed, not when one is parsed, so the first module-scope `z.object(...)` in the bundle fires the probe — and esbuild evaluates the chunk holding zod and its callers before the chunk holding any module of ours that imports zod. A Function-constructor trap in the page put the call under `new ZodObject` ahead of the entry's first statement. `globalConfig` is `globalThis.__zod_globalConfig`, which zod adopts with `??=` rather than replacing, so the banner can set the flag before any module runs. That is where it now lives, beside the `process` shim and under the same `MOBILE_WEB_APP_SHIMS` contract, which asserts it is applied. Nothing is lost: the compiled path was never reachable in a page under this policy. The render check's `newCsp()` filter is gone. It dropped violations by `blockedURI === 'eval'`, which would have hidden a real one, and every case now asserts zero. The first case walks load and first paint, which is where the second of the two reports fired. The dead `violations` array is deleted. Red first: blank the banner constant and four of the six cases fail, each naming a `blockedUri: 'eval'` the filter used to swallow. Finding, not fixed here and reported instead: the page bundles two copies of zod, mobile's 4.4.3 and the repo root's 4.5.4, because `src/shared/zod-salvage.ts` resolves upward. That is 808 KB of duplicate source. Aliasing `zod` to one copy in the builder fixes it and was measured working, but it changes which zod shared code runs in the shipped page, which is a call to make on its own rather than inside a CSP fix. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

The double reproduced one arm of `BridgeHostSubscriptions.canCarry`, the message cap, and silently carried anything the other two would have refused: a window already holding its maximum frames, and a window whose bytes the frame would push past the limit. It also ignored the page's `ack` frames, so its window never reopened — which was invisible only because no case streamed far enough to close it. Both arms are in now, and the `ack` arm consumes the page's acks exactly as the host does. The three caps are read out of `bridge-caps.ts` and `bridge-host-subscriptions.ts` rather than retyped, the same way the harness already reads the protocol version and the CSP, so a double carrying a stale number is not possible. The render check's own `640 * 1024` is gone with them. One case for it: thirty frames of about 200 KB, roughly 6 MB through a 4 MiB window, nothing over the message cap, so a drop can only come from the window. Every frame posts, nothing is dropped, and the page's ack seqs are read back to show the window stayed open because the page acked rather than because the double was generous. The file docstring said the double answers no RPC. It serves a screencast stream now, so it says that instead, and says what it still is not: it decides no domain behaviour. The dead `violations` array is gone, folded with the CSP commit. Red first: make the `ack` arm inert, as it was before this commit, and the case fails with `Set{'posted','dropped'}` against `Set{'posted'}`. A first attempt at that mutation left the byte subtraction in place and stayed green, which is the mutation being wrong rather than the case. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

… stale The `mobile_web_app` job hand-listed ten files. The render check this chain added was not among them, so it would have skipped in CI — and it was not the first: three landed censuses were already unlisted, and their closure blocks only run with `ORCA_MOBILE_WEB_APP_DEPS_REQUIRED=1`, so they are green in the sharded `test` job whether or not they ever ran here. Nobody could see it. The list is now two vitest filename filters, `config/scripts/mobile-web- app-` and the one builder test outside that prefix. Quoted, because vitest matches a positional as a substring against the discovered files rather than expanding a glob: `mobile-web-app-*.test.mjs` finds nothing, and it fails by reporting no test files rather than by running fewer. Both forms were tried before this one was written. It runs 18 files and 205 cases, against 10 files before. With mobile dependencies absent, 111 of those 205 skip, which is the measure of what only this job runs. Per file, cases CI has never run: browser-pane-render 7 of 7 (this chain) source-control-external-links 9 of 9 source-control-keyboard 6 of 6 source-control-text-inputs 6 of 19 (C4.2) frame-budget-sweep 4 of 4 (this chain) route-manifest 2 of 17 Two more files the filter adds run fully in the sharded job already and change nothing here: `browser-pane-text-inputs` (C6.4 — it censuses a hand-written closure and never bundles, so unlike the report it was not skipping) and `external-link-seam`. The sweep is renamed into the family for the same reason. As `mobile-browser-frame-budget-sweep.test.ts` it matched neither the job's filter nor `pr-code-change-scope.mjs`'s `config/scripts/mobile-web-app-` prefix, so a change to it alone would not have run the job that runs it. It is also gated on the dependency check now: it needs no react-native-web, but it launches Chromium, and that flag is what tells the job with a browser from the one without. Unguarded it would have failed the sharded `test` job outright. That filter is a prefix match. `config/scripts/mobile-web-app-` and `mobile/src/` both fire this job, and `.github/workflows/pr.yml` is in GLOBAL_FORCE_PREFIXES, so this commit runs everything. Also: `postedFrame` in the ruling-2 pin and in the sweep both reached the binary lane through `?.`, so a subscribe that opened no stream read as zero posts — indistinguishable from a dropped frame, which is the verdict both files are about. They throw now. The render check's restated `640 * 1024` went with the window caps in c73b405; the cap is read from `bridge-caps.ts`. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

The filter that replaced the hand list runs 18 files where the list ran 10, so the step's cost is now a function of what anyone names into the family rather than of what a reviewer remembered to add. Measured on this machine: 25-30s wall for the whole step, of which the frame-budget sweep is 2.5s. The sweep is the one part whose cost is a choice. It encodes 111 noise JPEGs in Chromium, one per viewport the budget fits, so adding rows to that set is a decision about this job's runtime and the comment says so where someone would make it. Found, not fixed, and reported for its own PR rather than folded here: the page bundles two copies of zod, mobile's 4.4.3 and the repo root's 4.5.4, reached through `src/shared/zod-salvage.ts`, which resolves upward while `mobile/src/` resolves to mobile's. That is 808 KB of duplicate source and two module instances in the shipped page. Aliasing `zod` in `mobileWebAppBuildOptions` fixes it and was measured working during this chain; it is reverted and stays reverted, because it changes which zod shared code runs in the page and that is not a call to make inside a CI commit. The CSP fix in 71254ab does not depend on it: `globalConfig` lives on `globalThis`, so the banner covers both copies. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: d3d67f58-8e18-4064-b32c-aa015b9f0820

📥 Commits

Reviewing files that changed from the base of the PR and between 030a0fd and c73b405.

📒 Files selected for processing (6)

config/scripts/build-mobile-web-app-bundle.mjs
config/scripts/mobile-browser-frame-budget-sweep.test.ts
config/scripts/mobile-web-app-browser-pane-render.test.mjs
config/scripts/mobile-web-app-render-harness.mjs
mobile/src/browser/browser-screencast-budget-at-the-shell.test.ts
mobile/src/browser/browser-screencast-request.web.ts

🚧 Files skipped from review as they are similar to previous changes (1)

mobile/src/browser/browser-screencast-budget-at-the-shell.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.

pullfrog

Caution

The new JPEG-budget sweep test cannot run in CI and will fail the sharded test job: config/vitest.config.ts collects config/scripts/**/*.test.ts, the sharded job installs no mobile dependencies, and the file's static imports of mobile/src/** make vite/oxc read mobile/tsconfig.json, whose extends: expo/tsconfig.base.json cannot resolve. The one job that installs mobile deps and Chrome (mobile_web_app) never runs it because its step lists files explicitly.

Reviewed changes

Re-reviewed the three commits pushed since the prior pullfrog review (fa933248, 71254ab, c73b405), against the full PR diff.

Disabled Zod's JIT probe through the esbuild banner — ZOD_JITLESS_BANNER sets globalThis.__zod_globalConfig.jitless before any module runs; verified both bundled Zod copies (4.4.3 and 4.5.4) read that flag in allowsEval and adopt the global with ??=.
Swept the JPEG worst case across 143 viewports — new mobile-browser-frame-budget-sweep.test.ts encodes real Chromium noise JPEGs at the budgeted scale and holds WORST_CASE_JPEG_BYTES_PER_PIXEL (0.545 → 0.56) to them.
Gave the shell double the whole canCarry rule and the page's acks — the harness now tracks requests, subscribes, dropped frames and acks, and consumes ack exactly as BridgeHostSubscriptions.ack does (verified against the real implementation).
Made the render check read its caps from source and assert zero CSP violations — FRAME_CAP_BYTES is gone, the blockedURI === 'eval' filter is gone, and a 30-frame long-stream case exercises the unacked window.

^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

pullfrog

Caution

c251b01fa9 wires the mobile_web_app job to the whole family correctly, but the guard it adds to the sweep test does not stop the file from failing to load in the sharded test job. The mobile imports are static, and ES imports are evaluated before describeSweep or beforeAll can skip anything, so vitest still transforms mobile/src/** and reads mobile/tsconfig.json — reproduced on this branch with mobile deps absent.

Reviewed changes

Re-reviewed c251b01fa9, the only commit since the prior pullfrog review.

Replaced the hand-maintained mobile_web_app file list with two vitest filename filters — config/scripts/mobile-web-app- plus config/scripts/build-mobile-web-app-bundle.test.mjs, keyed to the same prefix pr-code-change-scope.mjs fires the job on; the job now runs 18 files.
Renamed the sweep to mobile-web-app-frame-budget-sweep.test.ts and gated it on mobileWebAppDependenciesPresent() — intended to keep it out of the sharded job; see the caveat above.
Made both postedFrame helpers throw when a subscribe opens no binary stream — CodeRabbit's nit, taken in the ruling-2 pin and the sweep.

^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

…ependency guard vite transforms every file under mobile/ against mobile/tsconfig.json, which extends expo/tsconfig.base.json; the sharded test job installs no mobile dependencies, so the sweep's static imports failed the file at load before describe.skip ran. Type-only imports stay static; the values load in beforeAll behind mobileWebAppDependenciesPresent(). Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

Round 2: the sweep and the render check encoded fixtures at a retyped 0.72; both now read BROWSER_FRAME_QUALITY (the sweep from the module, the render check through the harness reader), so a quality change fails the certification instead of leaving it green. Every render case now asserts zero CSP violations; the sweep pins the 32 viewports left at scale 1; three references to a renamed file and a file that never existed are corrected; a shim count comment is made count-agnostic. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

pullfrog

✅ No new issues found.

Reviewed changes

Re-reviewed 2a1c3dfc38, the only commit since the prior pullfrog review. It resolves the open critical finding from the last two reviews.

Load the sweep's mobile modules lazily — mobile-web-app-frame-budget-sweep.test.ts now pulls its mobile/src/** dependencies through a loadSweepModules() helper called from beforeAll after the mobileWebAppDependenciesPresent() guard, with a sweep() accessor for the tests. Only the erased import type { BrowserScreencastFrame } stays static. Reproduced the sharded-job path on this branch with mobile deps absent (pnpm exec vitest run --config config/vitest.config.ts config/scripts/mobile-web-app-frame-budget-sweep.test.ts): the file loads and all 4 cases skip with exit 0, where it previously died on TSConfckParseError: expo/tsconfig.base.json. In mobile_web_app, beforeAll loads the modules and the sweep runs for real.

Both prior inline threads (the sweep's sharded-job load failure and its missing registration) are resolved as addressed by this commit and c251b01fa9.

^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

pullfrog

✅ No new issues found.

Reviewed changes

Re-reviewed ade4729386, the commit pushed after the prior pullfrog review. It is value-preserving and tightens the certification.

Read the shipped JPEG quality instead of retyping it — the sweep and the render check now both encode their noise fixtures at BROWSER_FRAME_QUALITY (read from browser-screencast-request-parameters.ts, currently 72) rather than a copied 0.72; the new readBrowserFrameQuality() harness helper uses the same source-read pattern as the other read* helpers, and every encodeNoiseJpeg call is awaited.
Pin the scale-1 viewport count — the sweep's drop case now asserts tooLarge.length === 32 (the 143 fixed viewports minus the 111 the budget fits) instead of > 0, so a silent shift in the budget denominator is visible.
Assert zero CSP on the remaining three render cases — all seven now assert no script-src violation.
Fix stale references — the sweep filename in browser-screencast-request.web.ts and browser-screencast-budget-at-the-shell.test.ts, and the page-zod-jitless.ts comment in the render check.

The two write-path edits are behavior-preserving (0.72 → readBrowserFrameQuality() → 0.72), and the changed files lint clean. No new issues.

^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

… the measured maximum The margin above the measured 0.55351 is what an encoder drift is allowed to spend; pinning the measurement made a drift inside the margin fail a budget that still held. The number stays in the docstring as the sweep's record. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

pullfrog

ℹ️ No behavioral concerns — one minor comment-placement suggestion inline.

Reviewed changes

Re-reviewed b78d1b2454, the only commit since the prior pullfrog review (ade4729386). It is a one-file tightening of the frame-budget sweep.

Assert the sweep against the shipped constant, not a retyped measured maximum — the local MEASURED_WORST_BYTES_PER_PIXEL = 0.55351 is removed and worstBytesPerPixel is now held to sweep().WORST_CASE_JPEG_BYTES_PER_PIXEL (0.56). The over-cap assertion above it still checks the real functional promise, and the measured-worst assertion still independently catches a too-low constant, so the pair remains non-circular. The change is sound: an encoder drift inside the constant's stated margin should not fail a budget that still holds.

^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

pullfrog · 2026-09-20T11:39:11Z

+/**
+ * What the sweep measured on 2026-09-20 across the 111 viewports the budget fits, rounded up at
+ * the fifth decimal: 0.55351. The constant carries a stated margin above it, and that margin is
+ * what an encoder drift is allowed to spend; the assertion below is against the constant, not this
+ * number, so a drift inside the margin does not fail a budget that still holds.
+ */


This docstring now dangles above type Viewport, which is not what it describes — it reads as the type's docs but talks about the measured 0.55351 and an assertion ~170 lines below. Since 0.55351 already lives in WORST_CASE_JPEG_BYTES_PER_PIXEL's docstring (browser-screencast-request.web.ts:27), consider dropping the first sentence here and folding the "assertion is against the constant" rationale into the comment at line 241, or replacing this with an actual doc for Viewport.

…tion it explains Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

Jinwoo-H added 4 commits September 20, 2026 05:26

coderabbitai Bot reviewed Sep 20, 2026

View reviewed changes

pullfrog Bot reviewed Sep 20, 2026

View reviewed changes

Comment thread config/scripts/mobile-web-app-browser-pane-render.test.mjs Resolved

Jinwoo-H added 5 commits September 20, 2026 06:16

coderabbitai Bot reviewed Sep 20, 2026

View reviewed changes

Comment thread config/scripts/mobile-web-app-frame-budget-sweep.test.ts Resolved

pullfrog Bot reviewed Sep 20, 2026

View reviewed changes

Comment thread config/scripts/mobile-web-app-frame-budget-sweep.test.ts Resolved

pullfrog Bot reviewed Sep 20, 2026

View reviewed changes

Comment thread config/scripts/mobile-web-app-frame-budget-sweep.test.ts Resolved

Jinwoo-H added 2 commits September 20, 2026 07:08

pullfrog Bot reviewed Sep 20, 2026

View reviewed changes

test(mobile): move the sweep's measured-maximum note beside the asser…

d52dccc

…tion it explains Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb

Jinwoo-H merged commit e6aa90f into main Sep 20, 2026
29 of 31 checks passed

Jinwoo-H deleted the ota-c6-5-certification branch September 20, 2026 11:43

Conversation

Jinwoo-H commented Sep 20, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Red first

Found, not fixed

Verified

Uh oh!

coderabbitai Bot commented Sep 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

⚠️ The new render check never runs in CI

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

pullfrog Bot Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Jinwoo-H commented Sep 20, 2026 •

edited by github-actions Bot

Loading

coderabbitai Bot commented Sep 20, 2026 •

edited

Loading