feat(document-viewer): rework the viewer for phone and PWA reading by BigSimmo · Pull Request #1741 · BigSimmo/Database

BigSimmo · 2026-08-08T14:42:34Z

Summary

Reworks the document viewer for phone and PWA reading. Each item is its own commit and is independently revertible while this PR is open.

Remove the CSP-blocked native PDF reader. The "Sharper zoom" mode swapped the pdf.js canvas for an iframe pointed at the Supabase signed URL. That can never render in production: buildContentSecurityPolicy sets default-src 'self' and declares no frame-src/child-src, so a cross-origin frame inherits 'self' and is refused. It only appeared to work in demo/dev, where the PDF is a same-origin /demo-documents path — which is also all the Playwright suite exercises, so no browser test could see the failure. iOS additionally ignores the #page= fragment the embed relied on. Removed along with the preference machinery behind it, whose getDefaultPdfViewerMode() returned false unconditionally, so the 820px breakpoint and the matchMedia listener watching it could never change the result. "Open PDF" — a top-level navigation, which CSP does not restrict — remains the escape hatch on every platform. A source contract now asserts no framed reader returns while the CSP has no cross-origin frame-src, since the demo-corpus E2E suite structurally cannot guard this.
One toolbar instead of three stacked control bars. The PDF panel stacked DocumentFrame's zoom band, the viewer-mode row, and PdfCanvasViewer's own page toolbar before the first pixel of the document. The page number was printed twice ("Page 1 of 1" and "of 1") and Maximize2 meant "fit width" in one bar and "fullscreen" in the other. DocumentFrame now owns every viewing control and PdfCanvasViewer renders source pixels only, so the duplication is gone structurally rather than by convention. Fullscreen moved to the frame so the chrome goes fullscreen with the document instead of leaving a bare canvas.
Revive pinch-to-zoom. The gesture hook was called with pinchZoom: pagesReady && !fitWidth, and fit-width is how every document opens, so a two-finger pinch reached neither the viewer (the hook declined it) nor the browser (the holder's touch-action: pan-y suppresses native pinch-zoom). There was no way to magnify a page by gesture without first finding a zoom button. docs/design-system/COMPONENTS.md lists "native pinch zoom preserved" as a blocking phone clause.
Stop iOS blanking the page at high zoom. WebKit refuses to back a canvas beyond ~2^24 device pixels — it keeps the layout box and paints nothing, with no error to catch. The raster used a flat min(2.5, dpr) output scale, and dpr is 3 on iPhone, so A4 at VIEWER_MAX_ZOOM asked for about 50 megapixels, three times the ceiling. resolveCanvasRasterPlan gives up raster density before layout size, so a page that cannot be drawn at full device density is softer rather than blank.
Put the source first on phones. The high-yield clinical summary rendered above the PDF, and for a document with nothing indexed it still drew a full gradient header, icon and heading around "A structured clinical summary has not been indexed for this document yet." buildDocumentSectionIndex has always described the opposite order, so this moves the DOM to the order the index already assumed. The card now renders nothing when it has nothing to say.
Stop refetching the detail window on every page flip. /api/documents/:id returns a nine-page window centred on the requested page, but activePage was an effect dependency, so flipping from page 3 to 4 refetched a window the client already held — an uncached round trip per tap.
Fetch PDF bytes on demand and release the raster. getDocument left disableAutoFetch at its default, so pdf.js kept pulling the rest of the file down even when the reader only looked at one page. Also releases the rendered page's decoded resources and the canvas backing store on teardown.
Cheap image wins. The whole-document image preview carried loading="lazy" although for an image-source document it is the document and sits above the fold (audit finding LCP-1/LL-1, half-fixed by Phase 0). useSignedImageUrl had no in-flight deduplication, so several views of the same image each minted their own signed URL on first paint.
Docs. ADOPTION.md described DocumentFrame as a shell-only surround with no controls toolbar — already stale after PR 2a, and now the opposite of what ships. Six follow-ups captured to docs/outstanding-issues.md.

Measured in Chromium against the demo corpus, phone viewport:

	before	after
Toolbar height, 320px and 390px	~190px across two bars	48px, one row
Page readouts in the viewer	2	1
PDF panel offset, summarised document (390×844)	651px	395px
PDF panel offset, unsummarised document	537px	395px
Empty summary cards, unsummarised document	1	0
Detail requests per in-window page flip	1	0
Client bundle, gzip	1500.0 KiB	1499.8 KiB

Verification

npm run verify:pr-local

Its stages were run individually because the aggregate halts on a pre-existing failure unrelated to this change (tests/pr-handoff-stop.test.ts, which asserts a read-only directory blocks a write — root ignores the permission bits; reproduced identically on an unmodified bc33d41 checkout, and captured as #283).

npm run check:runtime — PASS, Node 24.15.0 / npm 11.17.0
npm run check:installed-lock-parity — clean after npm ci --include=dev
npm run format:changed — "All matched files use Prettier code style!"
npm run lint — clean, --max-warnings 0
npm run typecheck — clean
npm run test — 5625 passed | 4 skipped, 1 failed (the pre-existing root-container failure above)
npm run build — passed, "Client bundle secret surface check passed."
npm run check:rag:fixtures — "Offline RAG fixture and manifest validation passed (36 golden cases, 23 suites)."
npm run check:bundle-budget — "299 files, 1499.8 KiB gzip … baseline 1406.4 KiB gzip; within tolerance." Base bc33d41 measures 1500.0 KiB on the same machine, so this change is bundle-neutral; the gap to the 2026-08-04 baseline is pre-existing drift and was not touched.

UI verification not run: npm run verify:ui cannot exercise this surface in this container. Its Chromium is 141.0.7390.37, which predates a Map builtin pdfjs-dist 6.2.108 calls, so the document route renders this[#methodPromises].getOrInsertComputed is not a function instead of a page — identically on bc33d41, so it is the environment and not a regression. Layout, toolbar geometry, DOM order, request counts and the absence of iframes were measured in that Chromium and are reported above; anything requiring a painted canvas was not. Captured as #278, with the physical iPhone acceptance owed for the pinch gesture and the canvas budget as #279.

npm run verify:release before release or handoff confidence claims

Not run — provider-backed, and not claimed.

npm run eval:retrieval:quality not applicable: no retrieval, ranking, selection, chunking or scoring behaviour changed.
npm run check:production-readiness not run — provider-backed. No clinical workflow, privacy, environment, Supabase or deployment behaviour changed; the source-governance surface touched is rendering only.

RAG impact: no retrieval behaviour change — this change touches the document viewer's chrome, phone layout, canvas raster budget, client-side detail-window caching and signed-image request deduplication. No file under src/lib/rag/**, clinical-search, retrieval-selection, ranking-config, answer-ranking, the eval harness or the golden fixture is modified, and no ranking comparator, score or ordering is read or written.

Risk and rollout

Risk: the viewer is the surface a clinician reads a guideline on. The largest change is the toolbar rewrite, which moves page/zoom/rotate/fullscreen ownership from PdfCanvasViewer to DocumentFrame; a mistake there degrades reading controls rather than document access or content. The canvas raster budget changes the resolution a page is drawn at, never which page or which document. The detail-window guard changes when a request is made, not what it returns or who may see it — every request-shaping input including the bearer token is in its signature, so a different document, chunk, retry or identity still refetches. disableAutoFetch makes a signed URL more likely to expire mid-read; the existing recovery path handles that and its budget resets on every successful open, which was checked rather than assumed.
Rollback: each bullet above is a separate commit and can be reverted alone while this PR is open. After a squash merge, revert the relevant hunks of the squash commit.
Provider or production effects: None. No provider-backed command was run. No Supabase, OpenAI, CI or deployment configuration was touched, and the CSP was deliberately left unchanged — the framed reader was removed rather than the policy widened.

Clinical Governance Preflight

Source-backed claims still require linked source verification before clinical use
No patient-identifiable document workflow was introduced or expanded without explicit governance approval
Supabase target remains Clinical KB Database (sjrfecxgysukkwxsowpy)
Service-role keys and private document access remain server-only
Demo/synthetic content remains clearly separated from real clinical sources
Source metadata, review status, and outdated/unknown-source behavior remain conservative
Deployment classification/TGA SaMD impact was checked when clinical decision-support behavior changed

Notes on the ones that carry real content here. Document access is unchanged: every signed URL is still minted by the same owner-scoped routes, the in-flight deduplication is keyed by bearer token so a request started under one identity can never be handed to another, and the detail-window guard's signature includes that token for the same reason. Source rendering stays pixel-faithful — no filter, invert, color-scheme or backdrop-filter on document pixels, still enforced by tests/document-frame-contract.test.ts. The canvas budget degrades resolution, never content: a page renders softer rather than blank, which is the conservative direction for a clinical document, and the previous behaviour was a silently blank page. Suppressing the empty clinical-summary card removes a shell around "not indexed yet"; it does not suppress an indexed summary, and the document's own section navigation still lists the summary section because that anchor targets the rail's profile panel, which renders its label badges regardless. No clinical decision-support behaviour changed, so the classification is unaffected.

Notes

Correcting an analysis error made during planning rather than leaving it in the history: I had claimed the section index could point at an empty summary section and changed hasStoredSummary to match the card. That was wrong — source-summary anchors the rail's document-profile panel, which renders whether or not a summary was indexed. hasStoredSummary is left exactly as it was.
Six follow-ups captured to docs/outstanding-issues.md as #278–#283: the container Chromium that cannot raster pdf.js 6, the physical iPhone acceptance owed, the two competing clinical-summary surfaces on the phone route, the pdf.js decoder-asset probe, the still-uncalled batch signed-URL route, and the root-container test failure.
Deliberately not done: wiring the viewer to the existing 100-id batch signed-URL route (a second data path for a below-fold cost, and the deduplication takes the first-paint duplication out without one), and shipping pdf.js's ~2 MB of cMap/font/WASM assets (unverified until the corpus is sampled for JBIG2/JPX).

🤖 Generated with Claude Code

https://claude.ai/code/session_01ER2xPFzPzoS8fAxkgHC8yo

Generated by Claude Code

The "Sharper zoom" mode swapped the pdf.js canvas for an iframe pointed at the Supabase signed URL. That can never render in production: buildContentSecurityPolicy sets `default-src 'self'` and declares no frame-src/child-src, so a cross-origin frame inherits 'self' and is refused. It only appeared to work in demo/dev, where the PDF is a same-origin /demo-documents path — which is also all the Playwright suite exercises, so no browser test could see the failure. iOS additionally ignores the #page= fragment the embed relied on for deep links. Remove the reader, its lazy wrapper, and the preference machinery behind it. getDefaultPdfViewerMode() returned false unconditionally, so the 820px breakpoint and the rAF + matchMedia listener that watched it could never change the result — dead work on every viewer mount. "Open PDF" stays as the escape hatch on every platform: a top-level navigation, which CSP does not restrict. Add a source contract asserting no framed reader returns while the CSP has no cross-origin frame-src, since the demo-corpus E2E suite structurally cannot guard this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ER2xPFzPzoS8fAxkgHC8yo

The PDF panel stacked three separate bars before the first pixel of the document: DocumentFrame's zoom band, the viewer-mode row, and PdfCanvasViewer's own page toolbar. The page number was printed twice ("Page 1 of 1" and "of 1") and Maximize2 meant "fit width" in one bar and "fullscreen" in the other. DocumentFrame now owns every viewing control — page navigation, zoom, fit, rotation, viewing aid and fullscreen — and PdfCanvasViewer renders source pixels and nothing else. That collapses the duplication structurally rather than by convention: there is one page readout because there is one component that can render one. Measured in Chromium at the document route, phone viewport: 48px of toolbar in a single row, no horizontal overflow, at both 320x720 and 390x760 (was ~190px across two bars). Six tap targets plus page navigation do not fit a 320px row, so zoom goes inline from 380px up and the narrowest phones reach it, along with fit/rotate/viewing aid/fullscreen, through one overflow menu — every item at the production min-h-tap target. Fullscreen moves to the frame so the chrome goes fullscreen with the document instead of leaving a bare canvas, keeping the native-then-in-app fallback for browsers that will not fullscreen a plain element (iOS Safari). frameOwnsZoomChrome is retired: with the browser-engine reader gone there is one reader, so the conditional split had no second case. The Sentry 15778840 guard it carried survives as the zoomRef composition path, which is now the only thing between a rapid pinch and dropped deltas. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ER2xPFzPzoS8fAxkgHC8yo

…page Two defects that only show up on a phone. Pinch did nothing in the viewer's default state. The gesture hook was called with `pinchZoom: pagesReady && !fitWidth`, and fit-width is how every document opens, so a two-finger pinch reached neither the viewer (the hook declined it) nor the browser (the holder's `touch-action: pan-y` suppresses native pinch-zoom). There was no way to magnify a page by gesture without first finding a zoom button. docs/design-system/COMPONENTS.md lists "native pinch zoom preserved" as a blocking phone clause. Pinch is now live in fit mode. The first delta drops fit, which switches the holder to `touch-action: none`, so the browser can only contend for the opening moment of a gesture and never the rest. Drag-to-pan stays gated on `!fitWidth` because fit mode needs the holder's native momentum scrolling. The canvas also blanked above roughly 2.3x zoom on iPhone. WebKit refuses to back a canvas beyond ~2^24 device pixels — it keeps the layout box and paints nothing, with no error to catch. The raster used a flat min(2.5, dpr) output scale, and dpr is 3 on iPhone, so A4 at VIEWER_MAX_ZOOM asked for about 50 megapixels, three times the ceiling. resolveCanvasRasterPlan gives up raster density before layout size, so a page that cannot be drawn at full device density is softer rather than blank, and never a different size than the reader asked for. Unit-tested across the whole supported zoom range at dpr 1-4, including an oversized sheet that cannot fit at CSS resolution at all. Not verifiable in this container: its bundled Chromium 141 predates the JS builtin pdf.js 6 needs, so the canvas fails to raster here regardless of these changes. The budget is covered by unit tests; the gesture needs a real phone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ER2xPFzPzoS8fAxkgHC8yo

The first phone viewport was spent before reaching any of the document. The high-yield clinical summary rendered inside DocumentOverviewLanding, above the PDF, and for a document with nothing indexed it still drew a full gradient header, icon and heading around "A structured clinical summary has not been indexed for this document yet." buildDocumentSectionIndex has always described the opposite order — "main column (overview -> PDF -> evidence -> text) then the aside rail" — so this moves the DOM to the order the index already assumed rather than inventing a new one. The summary card leaves the landing and becomes its own grid child; phone order is set with max-sm:order-* so the desktop two-column layout is untouched. Every grid child now carries an explicit phone order. An unordered grid item defaults to `order: 0` and would sort ahead of `order-1`, which would have thrown the rail to the top of the page. The card renders nothing when the model yields nothing, instead of a shell around its empty state. Measured in Chromium at 390x844 against the demo corpus: summarised document: PDF panel 651px -> 395px from the top unsummarised document: PDF panel 537px -> 395px, and the empty card is gone (1 summary card -> 0) Correcting an analysis error from the plan while I am here: I had claimed the section index could point at an empty summary section, and changed hasStoredSummary to match the card. That was wrong — `source-summary` anchors the rail's document-profile panel, which renders its label badges whether or not a summary was indexed. hasStoredSummary is left exactly as it was. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ER2xPFzPzoS8fAxkgHC8yo

…e flip `/api/documents/:id` returns a window of pages centred on the requested one — nine, by defaultPageWindow — but `activePage` was a dependency of the detail effect, so flipping from page 3 to 4 refetched a window the client already held. The route sets no Cache-Control and the service worker never caches a query-string request, so each of those was a real round trip: one per tap, on a phone, on cellular. The client now remembers the window each payload covered, along with the request identity it was loaded under, and skips the effect when the only thing that moved is the page and the new page is inside that window. Anything else — a different document, chunk, retry attempt or auth identity — changes the signature and refetches as before. A chunk route never records a window at all, because its window is centred on the selected chunk rather than on the page. The decision is a pure function in document-viewer-navigation.ts alongside the existing load-key helpers, so the in-window/out-of-window/changed-identity cases are unit-tested rather than only reachable through the effect. Measured in Chromium against the demo corpus, flipping a page from the toolbar: 1 detail request before, 0 after. The demo documents top out at two pages, so the out-of-window refetch is covered by the unit tests rather than the browser. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ER2xPFzPzoS8fAxkgHC8yo

pdfjs.getDocument was called with the url alone, leaving disableAutoFetch at its default. pdf.js then keeps fetching the rest of the file in the background even when the reader only ever looks at one page — the wrong default for a long guideline opened on a phone, on cellular. The two flags are a pair: disabling pre-fetch does nothing while streaming is on, which the installed pdfjs-dist types state outright, so both are set and a contract test keeps them together. The trade is that later bytes are requested later, so a signed URL is more likely to expire mid-read against its 10-minute TTL. That path already exists and recovers: a range failure surfaces as an auth/HTTP error, isLikelyExpiredUrl catches it, and the parent re-issues the URL. I checked the budget rather than assuming it — signedUrlRefreshCountRef resets in handlePdfLoadSuccess on every successful open, so the cap is two *consecutive* failures, not two per session, and a long reading session is not dead-ended. Also release resources on teardown: the rendered page's decoded resources (pdf.js declines while a render is live, so this cannot cut one short) and the canvas backing store, which a canvas otherwise holds until collection — real memory on a phone for a document the reader has already left. Not verifiable here: this container's Chromium 141 predates a JS builtin pdf.js 6 needs, so pdf.js never initialises and no range request is ever issued. Covered by source contract; needs a real device or a current browser to observe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ER2xPFzPzoS8fAxkgHC8yo

…cate signed URLs Three small fixes to the document surface. The whole-document image preview carried loading="lazy". For an image-source document that element *is* the document and sits above the fold, so lazy-loading put the largest element on the page behind the browser's own lazy threshold and delayed LCP — audit finding LCP-1/LL-1 from performance-image-cwv-audit-2026-08-02, which Phase 0 half-fixed by adding decoding="async" and leaving the lazy attribute. It now loads eagerly with fetchPriority="high". useSignedImageUrl had no in-flight deduplication: the module LRU only helps once a response has landed, so a page holding several views of the same image — a figure and its lightbox, a rail panel and a filmstrip — minted a separate signed URL per consumer on first paint. One shared request per endpoint now, keyed by bearer token as well so a request started under one identity is never handed to another, and dropped on retry so a retry genuinely refetches. The rail's sticky offset was a hand-copied `lg:top-[69px]` for a bar whose height is not actually fixed — it moves with type size. useDocumentChromeMetrics already measures that row, so it now publishes the value and the rail offsets by it, keeping the literal only as a first-paint fallback. Wiring the viewer to the existing 100-id batch signed-URL route (src/app/api/images/signed-urls, which still has no caller anywhere in src/) is deliberately left out: it is a second data path for a below-fold cost, and the dedupe above takes the first-paint duplication out without one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ER2xPFzPzoS8fAxkgHC8yo

…rship notes Six rows captured from the document-viewer optimisation pass: the container Chromium that cannot raster pdf.js 6 (so no browser gate covers the viewer canvas), the physical iPhone acceptance owed for the pinch fix and canvas budget, the two competing clinical-summary surfaces on the phone route, the pdf.js decoder-asset probe, the still-uncalled batch signed-URL route, and the root-container test failure that makes every full-suite run report '1 failed'. ADOPTION.md still described DocumentFrame as a shell-only surround with no controls toolbar and PDF chrome on PdfCanvasViewer — already stale after PR 2a flipped that contract, and now the opposite of what ships. The phase-2 plan's component diagram still listed the removed NativePdfEmbed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ER2xPFzPzoS8fAxkgHC8yo

coderabbitai · 2026-08-08T14:43:06Z

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: 2 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: 328c67cf-1e83-4221-ac2a-9a28b301ea1e

📥 Commits

Reviewing files that changed from the base of the PR and between d33a6cd and 8d01991.

⛔ Files ignored due to path filters (1)

tests/__screenshots__/linux/document-viewer.png is excluded by !**/*.png

📒 Files selected for processing (36)

docs/branch-review-ledger.md
docs/design-system/ADOPTION.md
docs/outstanding-issues.md
docs/plans/document-viewer-phase2-unified-chrome.md
lighthouse-budget.json
src/app/globals.css
src/components/DocumentViewer.tsx
src/components/clinical-dashboard/use-signed-image-url.ts
src/components/document-viewer-lazy.tsx
src/components/document-viewer/canvas-raster-budget.ts
src/components/document-viewer/document-clinical-summary.tsx
src/components/document-viewer/document-overview-landing.tsx
src/components/document-viewer/document-rail-panels.tsx
src/components/document-viewer/non-pdf-source-preview.tsx
src/components/document-viewer/pdf-canvas-viewer.tsx
src/components/document-viewer/pdf-readers-lazy.tsx
src/components/document-viewer/pdf-viewer-mode.ts
src/components/document-viewer/use-document-chrome-metrics.ts
src/components/document-viewer/use-pdf-viewer-preference.ts
src/components/ui/document-frame.tsx
src/lib/document-viewer-navigation.ts
tests/__screenshots__/linux/provenance.json
tests/auth-signed-url-cache.dom.test.tsx
tests/canvas-raster-budget.test.ts
tests/client-performance-boundaries.test.ts
tests/document-clinical-summary.dom.test.tsx
tests/document-detail-performance.test.ts
tests/document-frame-contract.test.ts
tests/document-frame.dom.test.tsx
tests/document-viewer-navigation-source.test.ts
tests/document-viewer-pdf-reader-lazy.test.ts
tests/document-viewer-shell.dom.test.tsx
tests/header-scroll-hide-contract.test.ts
tests/ui-hydration.spec.ts
tests/ui-smoke.spec.ts
tests/use-viewer-gestures.dom.test.tsx

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch claude/document-viewer-optimization-tu8tnj

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

supabase · 2026-08-08T14:43:57Z

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 ↗︎.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ER2xPFzPzoS8fAxkgHC8yo

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 98b799a372

ℹ️ 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".

BigSimmo · 2026-08-08T14:50:53Z

@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 · 2026-08-08T14:50:59Z

PR #1741 worked end-to-end. Tip is 94ec9f455944441d9a35b07e58fbd4c557a5141b.

Snapshot

Confirmed sole target: feat(document-viewer): rework the viewer for phone and PWA reading #1741 (claude/document-viewer-optimization-tu8tnj)
Started at GitHub head 0336538c (matched local)
Real conflict in docs/outstanding-issues.md → merged origin/main, kept #278, renumbered follow-ups #279–#284
Now: merge-tree clean, mergeable: MERGEABLE, behind-but-clean by 1 vs newer main (eda8fe87) — left unsynced because Production UI was in flight

Fixed & pushed

P1 pdf page cleanup race → effect-local pageToCleanup
P1 signed-URL cache identity leak → cache write only from active consumer + regression test
P2 out-of-range page toolbar → route notify + DocumentFrame bounds
Design-system w-11 / shadow-lux debt, TS narrow, boundary-contract update

Verification

verify:cheap: 524 files / 5629 tests passed
verify:pr-local: EXIT 0 (bundle + RAG fixtures green)
Threads resolved (inline replies 403 — dispositions in the PR summary comment)
Review ledger appended locally only (not pushed — no ledger-only tip)

Left for you

Merge when ready: feat(document-viewer): rework the viewer for phone and PWA reading #1741
Optional late sync after required CI settles if still behind
Residual: physical iPhone acceptance (#280), container pdf.js Chromium gap (#279)

Resolve docs/outstanding-issues.md by keeping main's #278 visual-baseline row and renumbering this PR's follow-ups to #279–#284 (next-id=285). Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>

Copilot

Pull request overview

Reworks the Document Viewer’s phone/PWA reading experience by removing a CSP-incompatible native PDF iframe path, consolidating PDF controls into a single DocumentFrame toolbar, restoring pinch-to-zoom in the default fit-width mode, and hardening PDF rendering/perf (canvas raster budget, on-demand byte fetching, and fewer detail refetches).

Changes:

Remove the framed/native PDF reader path and its preference machinery; keep a single pdf.js canvas reader behind the existing lazy split.
Consolidate viewing controls (page nav, zoom/fit, rotate, viewing aid, fullscreen) into DocumentFrame, and make PdfCanvasViewer render pixels only.
Improve phone experience and performance: pinch-to-zoom in fit mode, avoid iOS blank canvases via raster budgeting, skip in-window detail refetches, dedupe signed-image URL requests, and release raster resources on teardown.

Reviewed changes

Copilot reviewed 33 out of 33 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/use-viewer-gestures.dom.test.tsx	Adds jsdom coverage for pinch gating (enabled vs disabled).
tests/ui-smoke.spec.ts	Updates smoke coverage for single reader + new testids/overflow fullscreen path; asserts no iframes.
tests/ui-hydration.spec.ts	Removes persisted PDF mode hydration scenario; keeps a document deep-link scenario.
tests/header-scroll-hide-contract.test.ts	Updates contract to use measured collapse height CSS var (with fallback) instead of hardcoded offset.
tests/document-viewer-shell.dom.test.tsx	Removes NativePdfEmbed mock/export usage.
tests/document-viewer-pdf-reader-lazy.test.ts	Updates lazy-split assertions for single reader; adds CSP/no-iframe source contract.
tests/document-viewer-navigation-source.test.ts	Adds unit coverage for skipping detail requests within a loaded page window.
tests/document-frame.dom.test.tsx	Updates DOM tests for the single-toolbar model, phone overflow actions, and distinct fit/fullscreen.
tests/document-frame-contract.test.ts	Updates contract: DocumentFrame owns all controls; pinch default + raster budget + phone order assertions.
tests/document-detail-performance.test.ts	Updates keying assertion to keep PdfCanvasViewer keyed by documentId only.
tests/document-clinical-summary.dom.test.tsx	Asserts summary renders nothing when empty; keeps “unindexed” line when only priorities survive.
tests/client-performance-boundaries.test.ts	Adds guard for on-demand PDF fetching and raster teardown behavior.
tests/canvas-raster-budget.test.ts	New unit coverage for canvas pixel budgeting across zoom and DPRs.
src/lib/document-viewer-navigation.ts	Adds `LoadedDetailWindow` + `canSkipDetailRequest` helper.
src/components/ui/document-frame.tsx	Adds page nav, rotation, fullscreen, and phone overflow menu into the shared viewer toolbar.
src/components/DocumentViewer.tsx	Removes PDF mode preference; wires new toolbar state (page/rotate/fullscreen/pageCount), skip-detail-refetch, and phone DOM reorder.
src/components/document-viewer/use-pdf-viewer-preference.ts	Removes native/canvas preference hook (now single reader).
src/components/document-viewer/use-document-chrome-metrics.ts	Adds `--document-collapse-height` CSS var for sticky chrome offsets.
src/components/document-viewer/pdf-viewer-mode.ts	Removes legacy PDF mode storage/breakpoint helpers.
src/components/document-viewer/pdf-readers-lazy.tsx	Keeps only PdfCanvasViewer dynamic split; documents why no iframe/native reader exists.
src/components/document-viewer/pdf-canvas-viewer.tsx	Removes internal toolbars/chrome; adds on-demand PDF byte fetching, raster budget, pinch-in-fit, and teardown cleanup.
src/components/document-viewer/non-pdf-source-preview.tsx	Updates docs comment; makes image-source preview eager/high priority for LCP.
src/components/document-viewer/document-rail-panels.tsx	Adds `className` passthrough and uses measured collapse-height offset var.
src/components/document-viewer/document-overview-landing.tsx	Removes embedded clinical summary from the overview landing; updates header comment.
src/components/document-viewer/document-clinical-summary.tsx	Adds explicit “has content” helper and renders `null` when empty.
src/components/document-viewer/canvas-raster-budget.ts	New raster budgeting utility and constants.
src/components/document-viewer-lazy.tsx	Updates comment to reflect single dynamic PDF reader.
src/components/clinical-dashboard/use-signed-image-url.ts	Adds in-flight request dedup keyed by endpoint+identity; integrates with retry/markFailed.
src/app/globals.css	Removes `pdf-toolbar` print-hide selector; documents why it’s no longer needed.
docs/plans/document-viewer-phase2-unified-chrome.md	Updates plan diagram to reflect single-toolbar + pixels-only reader model.
docs/outstanding-issues.md	Captures new follow-ups (#278–#283) and bumps next-id marker.
docs/design-system/ADOPTION.md	Updates DocumentFrame adoption notes to reflect it now owns viewer chrome.
docs/branch-review-ledger.md	Appends a ledger row for this PR’s review/verification snapshot.

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

- Scope pdf.js page cleanup to the effect-local page proxy so a rapid page flip cannot cleanup() the next page via a shared ref. - Write the signed-URL LRU only from an active consumer so a late in-flight response after an account switch cannot repopulate the endpoint cache with the prior identity's URL. - Bound DocumentFrame page navigation against pageCount and notify the parent route when pdf.js clamps an out-of-range deep link. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>

Replace the page-input w-11 literal and the overflow menu's shadow-lux alias so check:design-system-contract no longer regresses the DocumentFrame path budget introduced by the unified toolbar. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>

After moving the LRU write behind the active-consumer guard, pass an explicit url string so SignedUrlPayload stays required-url typed. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>

The shared renderedPageRef cleanup path was the Sentry race; the contract now asserts the effect-scoped pageToCleanup release instead. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>

cursor · 2026-08-08T15:10:33Z

Review-and-fix summary (PR #1741)

Tip: 94ec9f455944441d9a35b07e58fbd4c557a5141b on claude/document-viewer-optimization-tu8tnj
Base: main (local merge-base clean). After this tip landed, origin/main advanced one commit (eda8fe87); git merge-tree --write-tree origin/main HEAD is still clean (behind-but-clean). No second sync performed — required Production UI shards were already in flight on this tip.

Sync / merge-tree

Real conflict on start: docs/outstanding-issues.md vs 07ada91b.
Merged origin/main, kept main’s #278 visual-baseline row, renumbered this PR’s follow-ups to #279–#284 (next-id=285).
Current: merge-tree clean, GitHub mergeable: MERGEABLE, mergeStateStatus: BEHIND (one commit).

Fixed (pushed)

Severity	Issue	Fix
P1 / Sentry	`renderedPageRef.cleanup()` race on rapid page flips	Effect-local `pageToCleanup`; late `getPage` cleaned when cancelled
P1 / Codex	Stale identity repopulating signed-URL LRU	Cache write only from active consumer; regression test
P2 / Codex	Toolbar `page=999` vs clamped canvas	Route notify on pdf.js clamp + DocumentFrame bounds Previous/Next/readout
Gate	`w-11` + overflow `shadow-lux` design-system debt	`w-10` + `shadow-inset`
Gate	TS narrow on cached payload; boundary contract still expected old cleanup	Narrowed url; updated `client-performance-boundaries`

Dispositioned threads

Inline replies returned 403 Resource not accessible by integration. Threads were resolved via GraphQL; dispositions recorded here:

Sentry pdf cleanup → fixed-head 94ec9f455944441d9a35b07e58fbd4c557a5141b
Codex signed-URL cache → fixed-head 94ec9f455944441d9a35b07e58fbd4c557a5141b
Codex page clamp → fixed-head 94ec9f455944441d9a35b07e58fbd4c557a5141b

Required CI (at tip; advisory ignored)

PR mergeability / PR policy / Static PR / Safety / Build / Unit coverage / Production UI critical: SUCCESS
Production UI (1)(2)(3): in progress when snapped
Visual baselines / Lighthouse: advisory only (Visual baselines FAILURE ignored)

Local gates (decisive lines)

npm run verify:cheap → Test Files 524 passed (524) / Tests 5629 passed | 4 skipped (5633) / Design-system contract passed (… legacy tap classes 0 …)
npm run verify:pr-local → Client bundle secret surface check passed. / Offline RAG fixture and manifest validation passed (36 golden cases, 23 suites). / EXIT 0

Residual risks

Physical iPhone acceptance for pinch + canvas budget still owed (#280 after renumber).
Container Chromium still cannot raster pdf.js 6 (#279).
Behind-but-clean vs newest main — one late sync after required CI settles if still behind.
Ledger row for this review left local/unpushed (no ledger-only tip).

Merge left to you. Auto-merge not armed.

github-actions · 2026-08-08T15:15:31Z

CI triage

CI failed on this PR. Automated classification of the 4 failed job(s):

Lighthouse budget (advisory) — needs investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.
Visual baselines (advisory) — needs investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.
Production UI (2) — needs investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.
PR required — needs investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.

Compared with main CI run #9157 (cancelled).

Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger.

BigSimmo · 2026-08-08T15:17:58Z