Split the bundle budget from mockup scratch, restore local browser gates, and pin the factsheet heading census - #1779
Conversation
The hero <h1> and the portaled print sheet's <h1> are correct and mutually
exclusive by construction: on screen `.factsheet-print-sheet { display: none }`
removes the print subtree, and in print
`html.factsheets-printing body > *:not(.factsheet-print-portal)` removes the
shell that owns the hero. Neither state exposes two headings to the
accessibility tree, and the printed PDF is a separate document whose section
headings are already <h2>, so demoting its title would leave it with no
top-level heading.
The real gap was that the existing assertion was scoped to the page testid, so
the document-level invariant was asserted nowhere and a stray third <h1> would
not have been caught. Pin the census instead: exactly two, one per container,
both carrying the title, plus a non-empty <h2> outline in the print sheet.
jsdom applies no stylesheet, so a census is the right guard rather than a
visibility assertion.
FactsheetPrintSheet stays in factsheet-detail-page.tsx —
design-system-contract-utils.mjs scopes its raw-colour exemption to the literal
factsheet-print-sheet marker and fails closed if it moves.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YEFowCVUVrybKvKvReQ924… gates The remote/Cloud drift note said to delegate browser proof to CI and left the impression that local gates were unrecoverable. They are recoverable; the blocker was two separate image faults, and the second is why the obvious fix looks impossible. The baked node_modules is stale or incomplete — containers have shipped none at all, and earlier ones reported playwright 1.62.0 against a locked 1.62.1 with tailwind-merge absent entirely, which is an incomplete install rather than a version skew, so the lockfile pin was never wrong. And npm ci cannot repair it because jsdom@30.0.1 requires node ^22.22.2 || ^24.15.0 || >=26.0.0 while images have shipped v24.13.0, so the install dies on EBADENGINE under engine-strict. Installing Node 24.19.0 clears that, npm ci then exits 0 and parity reports all seven pinned packages, and `npx playwright install` supplies Chromium 1234 (images ship only 1194). Verified end to end this session, launch included. Keeps the existing Stop intact and makes it cheap to honour: install the matching revision rather than forcing a run against 1194. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEFowCVUVrybKvKvReQ924
One number could not honestly answer two questions. totalGzipBytes summed every built client chunk, including src/app/mockups/** design scratch that 404s in production, against a ceiling named as though it were production weight. #13 held that mockup chunks are not a production bundle; this gate charged them anyway, which is how PR #1580 blocked at +10.1% for chunks no user can load. #252 recorded the contradiction and left the metric undecided. Measured on a clean build of main at af85cbc, the blur had become the whole signal: 1546.5 KiB total was +9.96% of the 1406.4 KiB baseline — 576 bytes from failing Build — while production-only was 1279.1 KiB, 9.06% BELOW that same baseline. Every byte of the apparent regression was design scratch (267.5 KiB across 76 chunks over 66 mockup routes) and production had actually shrunk. latency-audit-2026-07-28 corroborates: 1,309,274 bytes then against 1,309,772 production-only now, flat to +0.04%, so the 2026-08-04 bump to 1,440,201 had absorbed mockup growth as production growth. Raising the ceiling again would have hidden that permanently, so this splits rather than ratchets. production (10%) covers every chunk a non-mockup route reaches plus chunks no manifest claims — framework, polyfills, runtime. mockups (25%) covers chunks reachable only from /mockups/**, as a runaway detector rather than a per-mockup gate; a ceiling tight enough to fire on the next mockup would just be --update'd reflexively. A chunk shared by both counts as production because it would be built either way. Attribution reads the per-route *_client-reference-manifest.js files under .next/server/app, since Next 16 webpack emits no app-build-manifest.json, and fails closed when that tree is missing or resolves no routes so the buckets can never silently collapse. Both fail paths proven against the real build. Also captures #296: pr-handoff-stop.test.ts fails in any root container because it chmods a fixture dir to force a write failure and root ignores permission bits — pre-existing, reproduced on clean af85cbc. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YEFowCVUVrybKvKvReQ924
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:26 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
Comment |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:f77e8db59f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Splits production and mockup bundle budgets, documents browser-gate recovery, and strengthens factsheet heading coverage.
Changes:
- Adds route-based bundle attribution and separate budgets.
- Documents local Playwright restoration.
- Adds a document-wide factsheet heading census.
A blocking issue remains: malformed manifests can bypass fail-closed attribution.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
scripts/check-bundle-budget.mjs | Implements split attribution and enforcement. |
bundle-budget.json | Defines production and mockup baselines. |
tests/bundle-budget.test.ts | Tests attribution and CLI behavior. |
tests/factsheet-detail-header.dom.test.tsx | Adds heading census coverage. |
docs/testing.md | Documents browser-gate recovery. |
docs/plans/document-viewer-phase3-handover.md | Updates budget guidance. |
docs/outstanding-issues.md | Records resolutions and follow-ups. |
AGENTS.md | Documents the two-budget contract. |
CLAUDE.md | Updates mockup budget guidance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:7a28a89854
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
BigSimmo
commented
Aug 9, 2026
@cursoragent Please can you review all the comments on this page and resolve all of them logically grounded in the current repository and the current branch for me. Check you agree with the suggestion also and ensure no regression with smallest required fix. |
You need to increase your spend limit or enable usage-based billing to run background agents. Go to Cursor |
Review feedback on PR #1779: treat unparseable route manifests as fatal attribution errors instead of counting them as resolved empty routes, and handle a zero mockup/production baseline without NaN percentage math. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Summary
Three unrelated follow-ups from PR #1769, kept as three separately revertible commits rather than one blended diff. The repo's own
classifyPullRequestFilesreturnsclinicalRisk: false,operationalRisk: false,ragRanking: false,ui: falsefor the combined file list, and none of the paths fall in the broader PR-risk list, so all three are independently low-risk under both bundling checks inAGENTS.md.fix(bundle-budget): split production weight from mockup scratch (#252, half of#013).totalGzipBytessummed every built client chunk — includingsrc/app/mockups/**design scratch that 404s in production — against a ceiling named as though it were production weight.#013held that mockup chunks are not a production bundle; the gate charged them anyway, which is how PR Add three Clinical KB guide wireframe mockups #1580 blocked at+10.1%for chunks no user can load. Measured on a clean build ofmainataf85cbc, the blur had become the entire signal: 1546.5 KiB total was +9.96% of the 1406.4 KiB baseline — 576 bytes from failingBuild— while production-only was 1279.1 KiB, 9.06% below that same baseline. Every byte of the apparent regression was design scratch (267.5 KiB across 76 chunks over 66 mockup routes, 17.29% of the total); production had actually shrunk.docs/audit/latency-audit-2026-07-28.mdcorroborates — 1,309,274 bytes then against 1,309,772 production-only now, flat to +0.04% — so the 2026-08-04 bump to 1,440,201 had absorbed mockup growth as production growth. This therefore splits rather than ratchets:production(10%) covers every chunk a non-mockup route reaches plus chunks no manifest claims (framework, polyfills, runtime);mockups(25%) covers chunks reachable only from/mockups/**, as a runaway detector rather than a per-mockup gate. A chunk shared by both counts as production, because it would be built either way. Attribution reads the per-route*_client-reference-manifest.jsfiles under.next/server/app(Next 16 webpack emits noapp-build-manifest.json) and fails closed when that tree is missing or resolves no routes, so the buckets can never silently collapse.docs(testing): record the verified recipe for restoring local browser gates (#255). The drift note implied local browser gates were unrecoverable. They are recoverable, and the reason the obvious fix looked impossible was a second, separate image fault: the bakednode_modulesis stale or incomplete (containers have shipped none at all; earlier ones reportedplaywright1.62.0 against a locked 1.62.1 withtailwind-mergeabsent entirely — an incomplete install, not a version skew, so the lockfile pin was never wrong), andnpm cicannot repair it becausejsdom@30.0.1requires^22.22.2 || ^24.15.0 || >=26.0.0while images have shipped v24.13.0, so it dies onEBADENGINEunderengine-strict. Installing Node 24.19.0 clears that;npm cithen exits 0 and parity reports all seven pinned packages;npx playwright installsupplies Chromium 1234 (images ship only 1194). Verified end to end, launch included. No dependency or lockfile change — the container fix is not committable, so this commit is documentation only.test(factsheets): census everyh1in the factsheet detail document (#295). Investigated the reported double-<h1>and concluded it is correct and should stay. The two headings are mutually exclusive by construction: on screen.factsheet-print-sheet { display: none }removes the print subtree, and in printhtml.factsheets-printing body > *:not(.factsheet-print-portal)removes the shell that owns the hero, so neither state exposes two headings to the accessibility tree. The printed PDF is a separate document whose section headings are already<h2>, so demoting its title would leave it with no top-level heading — a regression, not a fix. The real gap was that the existing assertion was scoped to the page testid, leaving the document-level invariant asserted nowhere; that is what this closes. No component moved:FactsheetPrintSheetstays infactsheet-detail-page.tsx, sincedesign-system-contract-utils.mjsscopes its raw-colour exemption to the literalfactsheet-print-sheetmarker and fails closed if it moves.Ledger:
#252and#295closed,#013and#255updated,#296captured (below).Verification
npm run verify:pr-localThe single failure is
tests/pr-handoff-stop.test.ts > emits handoff context only when the marker file exists, and it is not caused by this diff. It chmods a fixture git dir to0o555to force a marker write to fail; root ignores permission bits, so the write succeeds and the assertion flips. Reproduced on a clean checkout ofaf85cbcwith every working change stashed —Test Files 1 failed (1) / Tests 1 failed | 10 passed (11)— andid -ureturns0in this container. CI is unaffected because its runner is non-root. Captured as ledger#296with a portable fix rather than papered over here.check:rag:fixtureswas reached separately and passed:Offline RAG fixture and manifest validation passed (36 golden cases, 23 suites).Bundle budget, against a clean
rm -rf .next && npm run build(.next/BUILD_IDmtime07:31:53, build exit 0):Both new fail paths were proven to exit 1 against the real build by temporarily lowering each baseline — a gate that cannot fail is not a gate:
Focused:
npx vitest run tests/bundle-budget.test.ts tests/factsheet-detail-header.dom.test.tsx→Test Files 2 passed (2) / Tests 28 passed (28).npm run verify:ui— not required: no UI, routing, styling, reduced-motion, or forced-colors behaviour changed. The onlysrc/-adjacent change is a test file; no component or stylesheet was touched.npm run verify:release— not run; no release or handoff-confidence claim is being made, and it is provider-backed.npm run eval:retrieval:quality— not applicable: no retrieval, ranking, selection, chunking, or scoring behaviour changed.npm run eval:rag/eval:quality— not applicable: no answer generation, synthesis prompt, or post-processing changed.npm run check:production-readiness— not applicable: no clinical workflow, privacy, environment, Supabase, source governance, or deployment behaviour changed.npm run check:deployment-readiness— not applicable: no deployment startup, hosting, or rollout behaviour changed.No provider-backed gate was run, and none was needed.
Risk and rollout
mockupstolerance is deliberately loose (25%) because a scratch ceiling tight enough to fire on the next mockup would be--updated reflexively, which is worse than no gate. Second-order risk:productionis now enforced against a baseline 9.06% tighter than the number it replaced, so it is a stricter gate than before, not a laxer one. The other two commits are documentation and a test-only assertion.scripts/check-bundle-budget.mjs+bundle-budget.json(restoretotalGzipBytes: 1440201and drop theproduction/mockupskeys to return to the old single-total behaviour).Notes
claude/playwright-browser-drift-fix-1bczxf) and never to push to another without explicit permission. Those two constraints pull opposite ways; splitting into three PRs would require three branches. Resolved by keeping one branch and one PR but three separately revertible commits, each with its own bullet above — which is whatAGENTS.md§ "PR bundling" describes as bundling done correctly, and it also avoids three concurrently open PRs all editingdocs/outstanding-issues.md, a file with no merge driver where manual conflict resolution has previously dropped rows (#152,#168). Happy to split into three PRs if you would rather review them separately — say the word and grant the extra branches.#255is updated, not closed. Local browser gates are restorable, but the container is ephemeral, so the ~5 minutes and ~330 MB are paid per session. The durable fix is still an image shipping Node ≥ 24.15.0, a completenpm ci, and the locked Chromium revision.#013is updated, not closed. Only its mockup half is settled; the route-scoped catalogue-payload work (/specifiers,/formulation) is still measurement-gated on#017. The split also supplies the separate mockup-share reporting that row asked for before any prune — and it removed the false pressure to prune, it did not authorise pruning.AGENTS.mdgains a "Bundle budget" section carrying the two-budget contract and the stale-.nextmeasurement trap:npm run buildreuses a cached.next, and the check then reads stale output and reports byte-identical numbers, which can report a pass that is not real.docs/plans/document-viewer-phase3-handover.mdreferenced the old single 1,440,201-byte total and is updated to the two new baselines.Generated by Claude Code