Replace a source-text grep in the viewer perf contract with the behaviour it guarded

[BigSimmo]

Summary

A test-only follow-up to #1772. Reviewing its tests against "is any of this excessively strict?" found one block that was a source-text grep rather than a behavioural test — mine — and measuring it showed it was also ineffective, which is the worse half of the finding.

• Replaced the grep with the behaviour it claimed to guard. tests/client-performance-boundaries.test.ts asserted that pdf-canvas-viewer.tsx contained the strings resolveLiveCanvasWindow, resolveRenderAheadPages, liveCanvasLimit, requestIdleCallback. The document-wide canvas budget is now covered behaviourally in tests/document-viewer-page-virtualization.dom.test.tsx instead.
• Derived the rail test's counts from RAIL_IMAGE_WINDOW instead of hardcoding 6 (and the 12/18/34/40 that follow from it).
• Relaxed the keyboard test's aria-label assertions from exact phrasing to the key names.

No production behaviour changes. canvas-raster-budget.ts and pdf-canvas-viewer.tsx are untouched; the only source edit is adding export to one constant.

The grep did not catch the regression it was named after

This is the part worth reading. Its own comment recorded why I reached for it: the virtualization DOM test's fixture pages were too small for the memory budget to bind, so a viewer that dropped the budget would still window correctly there. The observation was right and the conclusion was wrong.

Measured, by replacing the budget call with a hardcoded 3:

	source-text grep	new DOM case
liveCanvasLimit = resolveLiveCanvasWindow({ perCanvasPixels })	9 passed	9 passed
liveCanvasLimit = 3 (budget deleted)	9 passed	1 failed — expected [3, 4, 5] to deeply equal [4]

The identifiers all survive that substitution — the import stays, the variable keeps its name — so the grep stayed green while the viewer retained three full-zoom canvases. It read as coverage and was not.

Making the budget bind

The DOM harness could always do this; it needed the arithmetic rather than a new fixture. At VIEWER_MAX_ZOOM on a devicePixelRatio 3 display, an A4 page gives affordable = sqrt(2^24 / 8.0M) = 1.45 output scale → ~16.8M backing pixels for one canvas against the 24M document budget → resolveLiveCanvasWindow returns 1.

The new case asserts render-ahead is vetoed after flushIdle(), which is what distinguishes "vetoed by the memory budget" from "not yet reached the idle gate". It sits next to the existing default-density case (which still yields three pages) so the pair documents the curve rather than a single point.

On the two smaller ones

Both are the same defect class: a test that goes red on a change that is not a regression.

RAIL_IMAGE_WINDOW was a literal 6 in the test, so tuning the window to 8 — a product judgement about how much of a figure rail is worth mounting up front — broke five assertions. Verified the fix by actually setting it to 8: all seven still pass, then reverted.

The keyboard test pinned /F fits the width/ and /R rotates/. The real contract is that every binding is named — a binding a screen-reader user is never told about is a binding they do not have — while the sentence around the keys is copy. It now asserts the keys.

One thing deliberately left alone

The same file still greps for disableAutoFetch: true, disableStream: true, canvas.width = 0 and pageToCleanup?.cleanup(). Those are pre-existing and not mine, and two of the four are now redundant — the DOM test asserts the getDocument options and the page cleanup behaviourally. Removing another author's guard is a separate decision, so this notes it rather than takes it.

Verification

• npm run verify:pr-local

- completed: check:runtime, check:installed-lock-parity, format:changed, lint, typecheck
  Test Files  1 failed | 545 passed (546)
       Tests  1 failed | 5872 passed | 4 skipped (5877)

npm run build              ✓ Compiled successfully in 80s
                           Client bundle secret surface check passed.
npm run eval:rag:offline   Offline RAG fixture and manifest validation passed (36 golden cases, 23 suites).
                           Test Files 23 passed (23) · Tests 574 passed (574)

The one failing test is pre-existing and environment-only. tests/pr-handoff-stop.test.ts > emits handoff context only when the marker file exists injects a write failure with chmod 0o555; this container runs as uid 0 and root ignores directory write bits. Reproduced on a clean origin/main worktree during #1772 and captured then as #291. CI runs non-root and stays green.

build and eval:rag:offline show as "not reached" because the gate stops at the first failure; both were run directly and are quoted above.

• npm run verify:ui — UI verification not run: not applicable. No production rendering changes; the only source edit adds export to a constant. The browser gate for this surface (tests/ui-document-canvas.spec.ts) is unchanged and still cannot run in this container — Chromium 141 vs pdfjs-dist@6's need for 151 (#279).
• npm run verify:release — not run; not a release handoff, and provider-backed.
• npm run eval:retrieval:quality / eval:rag — not applicable; no retrieval, ranking, or answer-generation behaviour changed.
• npm run check:production-readiness — not run; no clinical workflow, privacy, environment, Supabase, source-governance, or deployment behaviour changed.

Risk and rollout

• Risk: low — test-only, plus one added export. The change strictly increases what the suite catches: the new case fails on a regression the removed one passed. Verified in both directions (sabotage fails the new case; tuning the rail window breaks nothing).
• Rollback: revert the single commit; nothing depends on it.
• Provider or production effects: None.

Notes

• #1772 merged as 644dd089; its branch was deleted, so this restarts the same branch name from current main per AGENTS.md rather than stacking on merged history.
• Worth recording as a general rule for this repo: a source-text assertion can only prove a name is present, never that it is load-bearing. Where a behavioural test seems impossible, the usual cause is a fixture that does not reach the regime under test — as here, where the fixture pages were simply too small.

---

Generated by Claude Code

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 59 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e7e72dcf-b8db-44a5-bd49-650504a176bd

📥 Commits

Reviewing files that changed from the base of the PR and between 199b303 and 1e091b5.

📒 Files selected for processing (6)

• docs/branch-review-ledger.md
• src/components/document-viewer/source-panels.tsx
• tests/client-performance-boundaries.test.ts
• tests/document-rail-image-window.dom.test.tsx
• tests/document-viewer-keyboard.dom.test.tsx
• tests/document-viewer-page-virtualization.dom.test.tsx

---

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

[supabase]

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[BigSimmo]

@cursoragent Work the current open PR end-to-end. Confirm the PR number and GitHub head first from context. If more than one open PR could apply, stop and say which one you would use and why.

Fetch and start from the remote tip that matches that GitHub head. If the named branch ref is missing or stale, use the PR head ref. Preserve unrelated local WIP, including any local-only ledger commits; do not discard dirty work, and do not treat a local-ahead commit as the reviewed tip. Do not merge the PR, force-push, rebase, or arm auto-merge unless I explicitly ask. No provider-backed gates without separate approval. If you cannot push or resolve threads, diagnose and comment only; if inline replies fail, resolve when possible and put dispositions in the summary comment. If auto-merge is already armed, push only for a real blocker, and avoid pushes that would cancel in-flight required CI unless the push itself clears that blocker.

If the PR is already merged or closed: confirm the head and merge commit, note required-CI outcome, post one summary, and stop.

Goal: deep review plus Bugbot, fix actionable issues with the smallest correct changes, clear merge / required-CI / thread blockers, run strong local offline verification, push fixes, append the review ledger, and post one PR summary. Prefer thoroughness over speed. Regenerate large assets only when a fix requires it; then run the asset check and keep compatibility aliases byte-identical where the repo uses them.

Snapshot the GitHub head SHA: tip, base, behind/ahead, mergeable state, merge-tree versus origin/main (real conflict versus behind-but-clean), required checks on that tip including Production UI when selected, advisory separately, unresolved actionable threads. Missing checks while dirty are not green. If the tip moves mid-work, re-snapshot and continue from the new head.

Ledger-lookup against that GitHub head under the heavy review-and-fix scope for this PR. Already reviewed at this head with clean merge-tree, green required checks, and no new actionable threads → summarize, comment, stop unless I asked for a fresh superseding pass. Follow the repo review protocol.

Unblock once: real conflict → merge origin/main (prefer main’s shared queues; keep this PR’s notes); behind-but-clean → one sync, late if required CI is in flight; clean → leave. Labels like skip-branch-sync do not block a manual sync needed to clear a real blocker. No sync thrash. After any sync or push, re-snapshot tip, merge-tree, and required checks before declaring done. Dedupe the ledger if a merge touched it.

Review high-confidence delta risks only. Separate PR-introduced defects from pre-existing re-emitted debt. For generated assets, review contracts, aliases, cache, and manifests—not every generated line. If protected RAG or ranking surfaces are touched, say so before editing; fix PR-body policy text only when wrong or missing. Ignore bot noise. No nit spam or broad rewrites. Escalate verification by touched risk: clinical / RAG / privacy / migrations / auth → domain check plus production-readiness when warranted; UI / phone-chrome / routing / styling → ensure plus phone-chrome or UI gates when warranted; generated assets or docs inventory/links → asset or docs checks when warranted.

Fix P0/P1 always; clear scoped P2 when locally provable; else disposition and resolve. Required-check failures on this tip only; ignore advisory. Prefer reverting a bad autofix commit. After fixes: format and commit before push; smallest targeted proof; repo cheap gate; repo PR-local gate. No release, lighthouse, live eval, or live provider gates without approval.

Push only this PR’s fix commits. Append ledger for the final GitHub head under the heavy scope. Never push a ledger-only tip—include ledger in a real fix push, or leave a no-change ledger append local and unpushed.

One PR comment: tip, sync/merge-tree, fixed versus dispositioned, threads, required CI, decisive local gate lines, residual risks; merge left to me. Inline only for remaining human-needed P0/P1.

Stop when merge-tree is clean on the current GitHub tip, actionable threads are clear, required checks are green or clearly in progress, heavy gates for scope passed, summary posted, merge left to me.

[cursor]

You need to increase your spend limit or enable usage-based billing to run background agents. Go to Cursor

[Copilot]

Pull request overview

Replaces brittle source-text checks with behavioral coverage for document-viewer performance contracts. No high-confidence issues found.

Changes:

• Adds behavioral coverage for the document-wide canvas budget.
• Derives rail-window expectations from the production constant.
• Tests keyboard accessibility by key names rather than exact wording.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tests/document-viewer-page-virtualization.dom.test.tsx	Tests canvas-budget collapse at maximum raster cost.
tests/document-viewer-keyboard.dom.test.tsx	Decouples binding coverage from label phrasing.
tests/document-rail-image-window.dom.test.tsx	Derives fixture sizes and expectations from the window constant.
tests/client-performance-boundaries.test.ts	Removes the ineffective source-text assertion.
src/components/document-viewer/source-panels.tsx	Exports the rail-window constant for tests.
docs/branch-review-ledger.md	Records the PR handoff and verification.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.