From 9d991d1d7d682e9476e53771f461ad80df14520c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 02:31:40 +0000 Subject: [PATCH 01/14] test(viewer): gate the PDF canvas raster in a real browser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The viewer's raster surface had no browser proof: unit tests cover the raster budget, DOM tests cover gestures, and a static contract covers the lazy boundary, but none of them can see whether a clinical source page actually paints. A blank canvas still reports correct dimensions, a correct aria-label, and a resolved render promise. tests/ui-document-canvas.spec.ts reads the raster back — ink pixels on page 1, the real page count in the one toolbar readout, and a page flip whose FNV pixel signature differs from page 1's. It also attaches an advisory page-flip cost measurement (long tasks + time to paint) as the input the OffscreenCanvas decision is conditioned on. pdfjs-dist@6 calls Map.prototype.getOrInsertComputed, which ships in Chromium 151 but not in the 141 build some sandboxed containers pre-bake and pin via PLAYWRIGHT_BROWSERS_PATH. The skip guard is therefore asymmetric: without CI it skips with a reason naming the browser version; with CI set a missing engine feature FAILS, because a gate that can skip itself green on the machine that gates the merge is worse than no gate. Both directions were verified locally. Spec collection is three hand-maintained lists that must agree, so all three are updated together and tests/playwright-project-isolation.test.ts gains a fail-closed assertion for this basename — "did not run" and "ran and skipped" are indistinguishable in a log otherwise. Closes #279 in docs/outstanding-issues.md. Its two refuted remedies (bump the pinned Playwright build, pin pdfjs-dist down) were not actioned; the recorded measurements were re-derived after install and match. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7 --- docs/outstanding-issues.md | 2 +- playwright.config.ts | 4 +- scripts/playwright-pr-shards.mjs | 6 +- tests/playwright-project-isolation.test.ts | 25 +- tests/ui-document-canvas.spec.ts | 266 +++++++++++++++++++++ 5 files changed, 298 insertions(+), 5 deletions(-) create mode 100644 tests/ui-document-canvas.spec.ts diff --git a/docs/outstanding-issues.md b/docs/outstanding-issues.md index 593ef710d6..e215a6b527 100644 --- a/docs/outstanding-issues.md +++ b/docs/outstanding-issues.md @@ -321,7 +321,6 @@ removed after current-main verification; it is not missing recommended work. | #275 | P2 | task | The shared filter trigger carries arbitrary spacing values inherited from DocumentFilterTrigger | **Outcome:** the phone filter trigger expresses its measurements as named tokens rather than bracketed values. **Detail:** `result-filter-control.tsx`'s `ResultFilterTrigger` uses `pr-[0.6875rem]`, `h-[1.0625rem]`/`min-w-[1.0625rem]` for the badge, and the raw breakpoint window `min-[414px]:max-[429px]` for the label. CodeRabbit flagged these against the design-token rule in PR #1706. Every one of them is copied *verbatim* from `DocumentFilterTrigger`, which shipped on main earlier and is the component this one was deliberately lifted from so the two cannot drift — so the finding is real but its scope is both call sites, not the new one. Changing only the copy would reintroduce exactly the drift the extraction removed, and each value carries a measured justification in its own comment (the asymmetric padding answers a stroked glyph against a filled pill; the breakpoint window is the one band that is single-line and short of width). **Next:** tokenise in `@theme` once, then update the trigger — there is now only one implementation, so it is a single edit. Confirm the badge and padding render identically at 393/402/414/430px before and after. **Stop:** do not tokenise the trigger without also retiring the values from the documents original, and do not treat this as licence to change the measurements themselves. | CodeRabbit review on PR #1706; DocumentFilterTrigger on main | 2026-08-07 | | #277 | P2 | issue | docs/design-system/HANDOVER-2026-08-07.md is cited as provenance by nine ledger rows but is measurably wrong | Outcome: no session scopes design-system work from a document whose figures have already been disproved. Evidence: rows #261, #262, #264, #265, #266, #267, #268, #269 and #270 all carry 'session 2026-08-07 — design-system HANDOVER-2026-08-07 Track A1 handoff (PR #1678)' as their Source, and docs/design-system/README.md links it as 'measured state, the ordered plan'. Measured wrong so far, all corrected into the rows themselves rather than the document: its '229 --shadow-tight aliases' is the seven-token legacyShadowAliases total mislabelled as one token (real figure 100 sites across 55 files, total 228); its adoption figure was 24 unadopted against a measured 23; its claim that visual baselines cannot be generated on Windows is half true and led to the wrong conclusion, since the ubuntu CI job already produces the ones that count; and #270's '22 call sites pair a tap token with a dead numeric height' does not survive re-measurement at all (zero same-variant pairs, 84 cross-variant responsive step-downs that are not dead). The document itself still asserts the originals. Next: cheapest fix is a superseded banner at the top naming the ledger rows as the current source of truth, plus the same in docs/design-system/README.md's link text — not a rewrite, because the corrections already live in the rows and duplicating them re-creates the drift. If the live path should leave docs/design-system/, move the file to docs/archive/ (or the design-system archive) and update inbound links per docs/README.md; do not delete it, because the nine Source citations, the PR/commit record, and the handover's verification/gotcha sections are provenance the ledger is meant to preserve. Stop: do not re-copy its figures into any new plan or handover, do not delete the evidence, and do not silently correct it in place, which would leave the nine Source citations pointing at a document that no longer says what those rows were derived from. | session 2026-08-08 — measured while closing #263 follow-ups across PRs #1719 and #1720 | 2026-08-08 | | #278 | P3 | issue | The document-viewer visual baseline bakes in viewport-pinned chrome that overlaps content | Measured 2026-08-08 while adopting the baselines (#118 / PR #1729). The document-viewer target clips #main-content, which is 1196x2903 against a 900px viewport, and contains viewport-pinned chrome: the sm:sticky sm:top-0 document header (DocumentViewer.tsx:1028) and the sm:fixed search composer (DocumentViewer.tsx:1511). Playwright stitches an oversized element clip, so both composite partway down the image and OVERLAP the content behind them — the cited-excerpt card and a source passage are partly covered in the committed golden. Position tracks total content height, so any content-height change above them moves the pinned chrome and inflates the diff well beyond what actually changed. NOT a product bug and NOT a #1705 regression: the pre-#1705 candidate from run 31249978408 shows the same overlap, so it is inherent to the target's design. The capture is deterministic, so the comparison still means something — five of six candidates were byte-identical by SHA-256 across two independent CI runs. Next: narrow that target's clip to a smaller locator, or add the pinned chrome to the target's mask array (the spec already supports mask, with a comment warning a mask is a hole in the gate). Stop: do not fix this by capturing fullPage — the spec bans it because ledger #093 leaves a hidden duplicate page root under CI load. | session 2026-08-08 — visual baseline adoption, #118 | 2026-08-08 | -| #279 | P2 | issue | pdf.js 6 cannot raster in this container's Chromium, so no browser gate covers the viewer canvas | **Outcome:** viewer canvas behaviour is provable by a gate rather than only by unit test and device. **CORRECTED 2026-08-09 (PR #1765) — the original remedies were wrong; do not action them.** Measured: pinned playwright@1.62.1 expects Chromium 151.0.7922.34 (playwright-core/browsers.json rev 1234); this container ships 141.0.7390.37 at /opt/pw-browsers/chromium-1194; CI runs HeadlessChrome/151.0.0.0 (recorded in lighthouse-budget.json:27). pdfjs-dist@6.2.108 calls Map.prototype.getOrInsertComputed (pdf.mjs:2454, 6889, 6896), which ships in Chromium 151 and not 141. So 'this[#methodPromises].getOrInsertComputed is not a function' is CONTAINER-ONLY: CI's browser already runs pdf.js 6 correctly. Do NOT bump the pinned Playwright build and do NOT pin pdfjs-dist down — the container's pre-installed browser is simply older than its own pinned Playwright expects, and PLAYWRIGHT_BROWSERS_PATH=/opt/pw-browsers pins it there. This environment also forbids running 'playwright install'. **Next:** write the viewer-canvas Playwright journey (non-blank canvas pixels, page count, page-flip changes the raster), let it skip-with-reason locally and prove in CI; this is Task 0 of docs/plans/document-viewer-phase3-handover.md. **Stop:** do not weaken a viewer assertion to make it pass in this container. | session 2026-08-09 Phase 3 handover, PR #1765; playwright-core/browsers.json rev 1234; lighthouse-budget.json:27 | 2026-08-08 | | #280 | P2 | task | Physical iPhone acceptance is owed for the viewer pinch gesture and the canvas pixel budget | **Outcome:** the two phone-only viewer fixes are confirmed on the device class they were written for. **Detail:** the viewer-optimisation branch revives pinch-to-zoom in fit mode (it was gated off in the default state, so a pinch reached neither the viewer nor the browser) and adds a canvas pixel budget so WebKit stops blanking the page above roughly 2.3x zoom on a dpr-3 display. Neither is verifiable in this container (see the Chromium/pdf.js row) and neither is a Chromium behaviour anyway — the canvas ceiling is a WebKit limit and the touch-action contention is a Safari gesture question. **Next:** on a real iPhone, in Safari and in the installed PWA: pinch a freshly opened document and confirm it zooms without first tapping a control; zoom to maximum and confirm the page stays painted rather than going blank; confirm a pinch that drifts vertically is not cancelled mid-gesture by the holder's 'touch-action: pan-y' (the mitigation if it is, is switching touch-action to none while two pointers are down — the gesture hook already tracks pointer count and exposes 'pinching'). Record the result against docs/phone-chrome-physical-acceptance.md. **Stop:** do not re-gate pinch on '!fitWidth' to resolve a gesture-contention finding — that restores the original defect. | session 2026-08-08 document-viewer optimisation; docs/design-system/COMPONENTS.md phone clause | 2026-08-08 | | #281 | P2 | rec | The phone document route renders two clinical-summary surfaces and neither is canonical | **Outcome:** one clinical summary on the document route, chosen deliberately. **Detail:** a phone reader gets the gradient 'High-yield clinical summary' card (DocumentClinicalSummary, built by buildDocumentClinicalSummaryModel) and, further down, the rail's '#source-summary' / 'high-yield-summary' disclosure (DocumentSectionSummary + FormattedHighYieldSummary + BadgeCluster). They render the same document.summary row two different ways. The rail is not hidden on phones — only its DocumentSectionIndexCard is lg:block — so both appear. Only the rail panel carries the section anchor, so the more prominent card is the unnavigable one. Note the two disagree about emptiness as well: the card now renders nothing when the model yields no usable text, while the rail panel still renders for its label badges, which is why 'hasStoredSummary' was deliberately left keyed to the stored row rather than to card content. **Next:** decide which rendering is canonical — this is a clinical-content judgement about how a summary should read, not a layout fix — then delete the other and give the survivor the 'source-summary' anchor. If the rail's badges are the part worth keeping, they can move without the second summary body. **Stop:** do not merge the two renderings mechanically; they format clinical text differently and the difference is the decision. | session 2026-08-08 document-viewer optimisation; document-rail-panels.tsx; document-clinical-summary.tsx | 2026-08-08 | | #282 | P3 | task | Probe the corpus for JBIG2/JPX before deciding whether pdf.js needs its decoder assets shipped | **Outcome:** a measured decision about pdf.js's cMap/standard-font/WASM assets rather than an assumption either way. **Detail:** getDocument is configured with url plus the on-demand fetch flags and nothing else, so 'wasmUrl', 'standardFontDataUrl', 'cMapUrl' and 'iccUrl' are all unset. pdfjs-dist ships those assets (wasm 1.5 MB, standard_fonts 804 KB, cmaps 1.7 MB) and nothing copies them into public/. With wasmUrl null, 'useWorkerFetch' resolves false and the WASM image decoders cannot load, so JBIG2 and JPEG2000 images fall back to the JS decoders or fail; those are exactly the encodings a scanned guideline uses, and this repo runs an OCR pipeline, which implies scanned sources exist. Non-embedded standard-14 fonts fall back to system fonts, which is a fidelity risk on a clinical document rather than a failure. **Next:** sample the real corpus for JBIG2/JPX-encoded images and for PDFs relying on the standard 14 before shipping ~2 MB of static assets; if the corpus does use them, copy into public/pdfjs, set the URLs, and add immutable cache headers in next.config.ts (public/ is not counted by check:bundle-budget, so there is no budget risk — the cost is bytes over the wire on first use). **Stop:** do not ship the assets on the assumption alone. | session 2026-08-08 document-viewer optimisation; node_modules/pdfjs-dist/types/src/display/api.d.ts | 2026-08-08 | @@ -490,5 +489,6 @@ Move resolved rows here with the resolution date and a one-line outcome. Keep th | #247 | task | Widen the one-line results bar to the six modes that pass a full-width phone select | RESOLVED 2026-08-07. All seven surfaces that shipped a full-width phone select now pass one compact badged trigger and opt into `mobileControlsPlacement="inline"`, so the one-line results bar is universal rather than a documents/therapy-compass exception. Converted: differentials, services, factsheets, prescribing, and the tools launcher (one select each) plus formulation and specifiers (two selects each, in the two-column grid this row named as the hard case). `MobileResultFilterControl` is deleted — no caller remains. The shared idiom lives in `src/components/clinical-dashboard/result-filter-control.tsx`: `ResultFilterTrigger` (lifted verbatim from `DocumentFilterTrigger`, so the control is the same component everywhere) and `ResultFilterSheet`, which renders one `role="radiogroup"` per dimension because these are genuinely one-of-N. Documents keeps its own panel — multi-select facet groups with counts, a find-a-filter field and collapse-by-default are not radios. Desktop is untouched: the ribbon renders `filterControls` from `sm` up and `mobileControls` below it, never both, so every mode keeps its chip row or tab strip on a wide screen. Verified in a real browser at 390px: the differentials band is geometrically identical to the documents band (89px at 390, 60px at 414 and 430). The "Stop" in this row is honoured — `mobileControlsPlacement` still defaults to `row`; nothing relies on that fallback now, and it stays so a mode that forgets the prop degrades to a second row rather than to an unreadable line. Follow-up captured separately: the `max-[413px]:flex-wrap` threshold was measured when Sort still occupied the phone line and is now stale — one line fits with zero overflow at 320/360/375/390/402px. | 2026-08-07 | | #263 | task | DS Track A4: close the 13 open COMPONENTS section 0.4 defect rows | Closed on branch claude/ds-a4-component-defects (commit e674f6e20). Eleven defects fixed across ten registered components: Button ref forwarding (+ a testId prop, because @types/react@19 gives components no data-${string} index signature); Progress indeterminate sweep onto the animate-shimmer theme token; StageList step index clamped to >=1 and announcement moved off the
    onto an sr-only role=status SIBLING (a child
  1. would make a five-stage job announce as 'list, 6 items'); StatusMark DocumentStatus declared in the component with the app row type asserted to conform, not the reverse; PageHeader title column floored at minmax(20ch,1fr) — the pre-existing wrap decided WHERE actions sat, not how wide, so the title still starved; Disclosure collapsed panel print:block (and the docstring's Ctrl-F claim was false); AccessibleTable dense header keeps its full string as title and the expander is now the registered Button; Tabs invalid value no longer empties the tab order (reachability only, no onChange fired to repair caller state); Pagination props clamped, row wraps at 320px, boundary focus handed to the current page, page announced via LiveAnnouncer; Links download type-omitted AND written after the spread, and LinkAction's hover nudge is a composited translate-x because gap is not in Tailwind's transition list so hover:gap-2 never eased; Checkbox/RadioGroup raw size-[1.125rem]/h-[2px] onto size-5/h-0.5 (size-4.5 retired by check:icon-scale). THREE ROWS RE-MEASURED AS STALE and were not work: Checkbox/RadioGroup unsanitised ids (optionId already sanitises) and no group hint/error (fieldset already carries hint/error/describedBy). Button is now genuinely product-adopted by AccessibleTable with a real v2 mount, so it left the reference-only snapshot in tests/design-system-adoption.test.ts. Ratchets fell and none rose: edge conflicts 28->27, legacy shadow aliases 229->228; baseline deliberately NOT lowered — that is A3/A5 work and needs a full debt-baseline regeneration. Evidence executed: check:design-system-contract exit 0 (all three sub-checks); tsc --noEmit exit 0; lint exit 0 at --max-warnings 0; prettier --check . clean; verify:ui 407 passed (14.1m) exit 0; 260+191 unit tests across component suites. npm run verify:pr-local aggregate NOT run — Vitest fork workers unreliable under box load; components run individually. Deliberately OUT of scope and still open: TextField/SearchField/Select (PR 7) and the ui-primitives.tsx module split (PR 12) were never in this task; Disclosure title truncation, StatusMark inline styles/raw geometry, Citation route/source modes, Links implicit new-tab policy, AccessibleTable content-role widths and the Button client boundary remain on their own rows. New finding worth a row: LinkAction accepts tone via BaseProps but never destructures it, so tone is silently ignored (#276). | 2026-08-07 | | #276 | issue | LinkAction accepts a tone prop it never reads | Refused rather than honoured: LinkActionProps now carries tone?: never, shipped in PR #1720 (5c0504a40). Omit alone was not enough — excess-property checking only fires on object literals, so a spread still type-checked clean and rendered the accent; verified with a focused tsc probe (Omit accepted the spread with no diagnostic, tone?: never rejected it with TS2345). A type-level contract test in tests/ui-v2-components.dom.test.tsx stops compiling if the prop widens back, plus a render assertion that the accent is what ships. | 2026-08-08 | +| #279 | issue | pdf.js 6 cannot raster in this container's Chromium, so no browser gate covers the viewer canvas | Resolved by tests/ui-document-canvas.spec.ts (Phase 3 Task 0, PR for claude/document-viewer-phase-3-bj5k5v): a Chromium viewer-canvas journey that reads the raster back — non-blank ink pixels on page 1, the real page count in the one toolbar readout, and a page flip whose FNV pixel signature differs from page 1's. Registered in all three hand-maintained lists (playwright.config.ts testMatch + productionSpecPattern, scripts/playwright-pr-shards.mjs productionSpecFilePattern + shard group 3) with a new fail-closed assertion in tests/playwright-project-isolation.test.ts so a future regex edit cannot silently drop it. The container skip is guarded asymmetrically: without CI it skips with a reason naming the browser version, with CI set it FAILS — verified both ways on 2026-08-09 (local run: 3 skipped; CI=1 run: 1 failed at the probe). Measurements re-derived after npm ci and they match the corrected row exactly: playwright-core/browsers.json chromium revision 1234 = 151.0.7922.34, container /opt/pw-browsers/chromium-1194 = 141.0.7390.37, pdfjs-dist 6.2.108 calling Map.prototype.getOrInsertComputed at pdf.mjs:2454/6889/6896. New datapoint: Node 24.13.0 also lacks getOrInsertComputed, so pdf.js 6 cannot be driven headlessly from this runtime either. Neither refuted remedy was actioned. | 2026-08-09 | diff --git a/playwright.config.ts b/playwright.config.ts index b6b921bffd..ae292f68b0 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -23,7 +23,7 @@ const chromiumExecutablePath = process.env.PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH; // `tests/playwright-project-isolation.test.ts` asserts every such file on disk is // matched here. const productionSpecPattern = - /.*(?:answer-progress-ui-smoke|ui-(smoke|stress|accessibility|tools|overlap|universal-search|specifiers|formulation|forms-section-nav|chrome-scroll|therapy-nav-scroll|mode-nav-density|phone-scroll(?:-[a-z0-9-]+)?|pwa|route-coverage|style-contract|visual-artifacts|hydration))\.spec\.ts/; + /.*(?:answer-progress-ui-smoke|ui-(smoke|stress|accessibility|document-canvas|tools|overlap|universal-search|specifiers|formulation|forms-section-nav|chrome-scroll|therapy-nav-scroll|mode-nav-density|phone-scroll(?:-[a-z0-9-]+)?|pwa|route-coverage|style-contract|visual-artifacts|hydration))\.spec\.ts/; const mockupSpecPattern = /.*ui-(document-top-navigation-mockup|therapy-navigation-mockup|tools|tools-collapse|tools-task-directory)\.spec\.ts/; const mockupTag = /@mockup/; @@ -31,7 +31,7 @@ const mockupTag = /@mockup/; export default defineConfig({ testDir: "./tests", testMatch: - /.*(?:answer-progress-ui-smoke|ui-(smoke|stress|accessibility|document-top-navigation-mockup|therapy-navigation-mockup|tools|tools-collapse|tools-task-directory|overlap|universal-search|specifiers|formulation|forms-section-nav|chrome-scroll|therapy-nav-scroll|mode-nav-density|phone-scroll(?:-[a-z0-9-]+)?|pwa|route-coverage|style-contract|visual-artifacts|hydration))\.spec\.ts/, + /.*(?:answer-progress-ui-smoke|ui-(smoke|stress|accessibility|document-canvas|document-top-navigation-mockup|therapy-navigation-mockup|tools|tools-collapse|tools-task-directory|overlap|universal-search|specifiers|formulation|forms-section-nav|chrome-scroll|therapy-nav-scroll|mode-nav-density|phone-scroll(?:-[a-z0-9-]+)?|pwa|route-coverage|style-contract|visual-artifacts|hydration))\.spec\.ts/, timeout: 60_000, retries: 0, // Fail the run if a stray `test.only` is committed: otherwise it silently diff --git a/scripts/playwright-pr-shards.mjs b/scripts/playwright-pr-shards.mjs index 885db4d4e8..9a320d1ea0 100644 --- a/scripts/playwright-pr-shards.mjs +++ b/scripts/playwright-pr-shards.mjs @@ -19,7 +19,7 @@ import { childProcessExitCode } from "./child-process-result.mjs"; /** Same matcher as playwright.config.ts `productionSpecPattern` (keep in sync). */ export const productionSpecFilePattern = - /^(?:answer-progress-ui-smoke|ui-(?:smoke|stress|accessibility|tools|overlap|universal-search|specifiers|formulation|forms-section-nav|chrome-scroll|therapy-nav-scroll|mode-nav-density|phone-scroll(?:-[a-z0-9-]+)?|pwa|route-coverage|style-contract|visual-artifacts|hydration))\.spec\.ts$/; + /^(?:answer-progress-ui-smoke|ui-(?:smoke|stress|accessibility|document-canvas|tools|overlap|universal-search|specifiers|formulation|forms-section-nav|chrome-scroll|therapy-nav-scroll|mode-nav-density|phone-scroll(?:-[a-z0-9-]+)?|pwa|route-coverage|style-contract|visual-artifacts|hydration))\.spec\.ts$/; /** * Explicit shard membership. Ordered to mix the measured slow phone-scroll @@ -49,6 +49,10 @@ export const prUiShardGroups = { 3: [ "tests/ui-tools.spec.ts", "tests/ui-accessibility.spec.ts", + // Small (three document-route navigations) and placed here rather than in + // the mega-spec group; it skips entirely on a browser that cannot raster + // pdf.js 6, so its cost is near zero outside the pinned Chromium build. + "tests/ui-document-canvas.spec.ts", "tests/ui-overlap.spec.ts", "tests/ui-universal-search.spec.ts", "tests/ui-stress.spec.ts", diff --git a/tests/playwright-project-isolation.test.ts b/tests/playwright-project-isolation.test.ts index a90334cd9c..31ae1c8a99 100644 --- a/tests/playwright-project-isolation.test.ts +++ b/tests/playwright-project-isolation.test.ts @@ -1,4 +1,4 @@ -import { readdirSync, readFileSync } from "node:fs"; +import { existsSync, readdirSync, readFileSync } from "node:fs"; import { resolve } from "node:path"; import { describe, expect, it } from "vitest"; @@ -66,6 +66,29 @@ describe("Playwright production-project isolation", () => { } }); + /** + * The viewer-canvas gate is the only browser proof that a clinical source page + * actually paints, and it is the single spec most likely to be dropped by a + * future edit to those two hand-maintained regexes: it skips on any browser + * without pdf.js 6's engine requirement, so "it did not run" and "it ran and + * skipped" look identical in a log. Assert its collection directly. + */ + it("collects the viewer-canvas gate into the required browser projects", () => { + const source = readFileSync(resolve(process.cwd(), "playwright.config.ts"), "utf8"); + const productionSpecPattern = configPattern(source, "productionSpecPattern"); + const testMatch = source.match(/testMatch:\s*(\/.*\/),/); + expect(testMatch, "playwright.config.ts: could not read the top-level testMatch regex").not.toBeNull(); + const testMatchPattern = new RegExp(testMatch![1].slice(1, -1)); + + const spec = "tests/ui-document-canvas.spec.ts"; + expect(existsSync(resolve(process.cwd(), spec)), `${spec} is missing`).toBe(true); + expect(testMatchPattern.test(spec), `${spec} is not collected by testMatch`).toBe(true); + expect( + productionSpecPattern.test(spec), + `${spec} is not collected by productionSpecPattern, so the viewer canvas has no browser gate at all`, + ).toBe(true); + }); + it("blocks service workers for mocked journeys but allows the dedicated PWA suite", () => { const config = readFileSync(resolve(process.cwd(), "playwright.config.ts"), "utf8"); const pwaSpec = readFileSync(resolve(process.cwd(), "tests/ui-pwa.spec.ts"), "utf8"); diff --git a/tests/ui-document-canvas.spec.ts b/tests/ui-document-canvas.spec.ts new file mode 100644 index 0000000000..dbbbb317eb --- /dev/null +++ b/tests/ui-document-canvas.spec.ts @@ -0,0 +1,266 @@ +import { expect, test, type Locator, type Page } from "playwright/test"; + +import { blockExternalRequests } from "./helpers/phone-scroll"; + +/** + * Browser gate for the PDF reader's raster surface. + * + * Everything else about the viewer is provable offline: the raster budget is a + * pure function with unit tests, the gestures and keyboard bindings have DOM + * tests, and the lazy boundary has a static contract. The one thing none of + * those can see is whether a clinical source page **actually paints** — a canvas + * that stays blank still has correct dimensions, a correct `aria-label`, and a + * resolved render promise. A reader looking at a blank page and a reader looking + * at the wrong page are both clinical failures, so they need a gate that reads + * pixels back. + * + * ## Why this skips in some containers, and why that is not a silent green + * + * `pdfjs-dist@6` calls `Map.prototype.getOrInsertComputed` (a 2026 TC39 + * addition) from its core rendering path. That method ships in Chromium 151 — + * which is what CI runs (`HeadlessChrome/151.0.0.0`, recorded in + * `lighthouse-budget.json`) and what this repository's pinned Playwright build + * carries (`playwright-core/browsers.json` revision 1234 → 151.0.7922.34). Some + * sandboxed containers pre-bake an OLDER browser and pin lookup to it with + * `PLAYWRIGHT_BROWSERS_PATH`, so pdf.js dies there with + * `getOrInsertComputed is not a function` before a single pixel is drawn. That + * is a property of the container, not of the product (ledger `#279`). + * + * The guard below therefore does two different things depending on where it + * runs, and the asymmetry is the whole point: + * + * - **Locally, without `CI`:** skip, with a reason naming the browser version. + * - **In CI:** a missing engine feature **fails**. A gate that can quietly skip + * itself on the machine that gates the merge is worse than no gate at all. + * + * Do not "fix" a local skip by bumping the pinned Playwright build, pinning + * `pdfjs-dist` down, or running `playwright install` — all three were measured + * and refuted on 2026-08-09. Run it on a host with the pinned browser instead: + * + * npm ci --include=dev && npx playwright install chromium + * npm run ensure + * npm run test:e2e -- tests/ui-document-canvas.spec.ts --project=chromium + */ + +// The 2-page demo document: page 1 is the monitoring protocol text, page 2 is +// the embedded-image evidence page. Two visibly different pages is what makes +// "a page flip changed the raster" a real assertion rather than a tautology. +const CANVAS_DOCUMENT = "/documents/22222222-2222-4222-8222-222222222222?page=1"; + +type CanvasReading = { + readonly width: number; + readonly height: number; + readonly sampled: number; + readonly inkPixels: number; + readonly signature: number; +}; + +/** + * Read a canvas back as pixels. + * + * `inkPixels` counts samples darker than a near-white threshold on any channel — + * a painted clinical page is mostly white paper with dark type, so ink is the + * signal that separates "rendered" from "blank" and from "cleared to white". + * `signature` is an order-sensitive FNV-style rolling hash over the same + * samples, which is what makes two different pages distinguishable without + * committing a pixel baseline (this is a behaviour gate, not a visual one). + * + * The sample grid is capped so a 4x-zoomed backing store does not turn a single + * assertion into a multi-second `getImageData` walk. + */ +async function readCanvas(canvas: Locator): Promise { + return canvas.evaluate((node) => { + if (!(node instanceof HTMLCanvasElement)) { + throw new Error("expected an HTMLCanvasElement"); + } + const context = node.getContext("2d", { willReadFrequently: true }); + if (!context) throw new Error("could not acquire a 2d context to read the raster back"); + + const width = node.width; + const height = node.height; + if (width === 0 || height === 0) { + return { width, height, sampled: 0, inkPixels: 0, signature: 0 }; + } + + const stepX = Math.max(1, Math.floor(width / 240)); + const stepY = Math.max(1, Math.floor(height / 240)); + const { data } = context.getImageData(0, 0, width, height); + + let inkPixels = 0; + let sampled = 0; + // FNV-1a over the sampled channel bytes. `>>> 0` after each step keeps it in + // uint32 so the value is stable and comparable across runs. + let signature = 0x811c9dc5; + for (let y = 0; y < height; y += stepY) { + for (let x = 0; x < width; x += stepX) { + const offset = (y * width + x) * 4; + const r = data[offset]; + const g = data[offset + 1]; + const b = data[offset + 2]; + const alpha = data[offset + 3]; + sampled += 1; + if (alpha > 16 && (r < 240 || g < 240 || b < 240)) inkPixels += 1; + for (const channel of [r, g, b, alpha]) { + signature = (signature ^ channel) >>> 0; + signature = Math.imul(signature, 0x01000193) >>> 0; + } + } + } + + return { width, height, sampled, inkPixels, signature }; + }); +} + +/** + * Fail in CI, skip with a reason anywhere else, when the browser cannot run + * pdf.js 6 at all. Never returns quietly on an unsupported engine. + */ +async function requirePdfRasterEngine(page: Page): Promise { + const supported = await page.evaluate( + () => typeof (Map.prototype as unknown as Record).getOrInsertComputed === "function", + ); + if (supported) return; + + const version = page.context().browser()?.version() ?? "unknown"; + const reason = + `This browser (${version}) has no Map.prototype.getOrInsertComputed, which pdfjs-dist@6 calls from its ` + + "render path, so no page can raster here. CI and the pinned Playwright build (browsers.json revision 1234, " + + "Chromium 151.0.7922.34) both have it. Run this spec against the pinned browser rather than weakening it — " + + "see the file header and ledger #279."; + + // In CI a missing engine feature is a real failure: the merge gate must never + // be able to skip itself green. + expect( + Boolean(process.env.CI), + `${reason} Refusing to skip because CI is set — this is the environment the gate exists to protect.`, + ).toBe(false); + + test.skip(true, reason); +} + +/** Open the document route and settle the reader's first page. */ +async function openCanvasDocument(page: Page): Promise { + await page.goto(CANVAS_DOCUMENT, { waitUntil: "domcontentloaded" }); + await expect(page.locator("#main-content").first()).toBeVisible({ timeout: 20_000 }); + await requirePdfRasterEngine(page); + + const holder = page.locator('[data-testid="pdf-canvas-scroll"]:visible').first(); + await expect(holder).toBeVisible({ timeout: 20_000 }); + return holder; +} + +/** Wait until a page's canvas exists and has actually been painted. */ +async function waitForPaintedPage(holder: Locator, pageNumber: number): Promise { + const canvas = holder.locator(`canvas[aria-label$="page ${pageNumber}"]`); + await expect(canvas).toBeVisible({ timeout: 20_000 }); + await expect + .poll(async () => (await readCanvas(canvas)).inkPixels, { + timeout: 20_000, + message: `page ${pageNumber} never painted any ink into its canvas`, + }) + .toBeGreaterThan(0); + return readCanvas(canvas); +} + +test.beforeEach(async ({ page, browserName }) => { + // Chromium-only, in the same spirit as the focused PWA suite: the engine + // feature pdf.js 6 needs is not what this gate exists to test, and a red + // weekly Firefox/WebKit matrix would say nothing about the viewer. + test.skip(browserName !== "chromium", "The viewer-canvas raster gate runs on the pinned Chromium build."); + await blockExternalRequests(page); +}); + +test.describe("document viewer canvas", () => { + test("rasters a clinical source page rather than leaving the canvas blank", async ({ page }) => { + const holder = await openCanvasDocument(page); + const firstPage = await waitForPaintedPage(holder, 1); + + expect(firstPage.width, "the canvas has no backing store, so nothing can have been painted").toBeGreaterThan(0); + expect(firstPage.height, "the canvas has no backing store, so nothing can have been painted").toBeGreaterThan(0); + // A rendered A4 text page covers far more than this; the floor only has to + // separate "painted" from "blank" and from "cleared to white". + expect( + firstPage.inkPixels, + `page 1 sampled ${firstPage.sampled} pixels and found ${firstPage.inkPixels} of ink — the reader is looking at a blank page`, + ).toBeGreaterThan(20); + }); + + test("reports the document's real page count in the one toolbar readout", async ({ page }) => { + await openCanvasDocument(page); + + // pdf.js is authoritative for the count; the indexed metadata is only a + // fallback. Both say 2 for this fixture, so a wrong number here means the + // reader is being told the document is a different length than it is. + await expect(page.getByText("/ 2").first()).toBeVisible({ timeout: 20_000 }); + await expect(page.getByLabel("Page number").first()).toHaveValue("1"); + }); + + test("a page flip paints a different page", async ({ page }) => { + const holder = await openCanvasDocument(page); + const firstPage = await waitForPaintedPage(holder, 1); + + // Capture long tasks across the flip. This is the measurement Phase 3 owes + // for the OffscreenCanvas decision (`docs/plans/document-viewer-phase3-handover.md` + // Task 5): the question is whether main-thread raster cost is large enough + // to justify moving it to a worker, and this is the only place in the repo + // where a real browser rasters a real page. Reported, never asserted on — + // a timing threshold in a blocking gate is a flake generator. + await page.evaluate(() => { + const store: number[] = []; + (window as unknown as Record).__viewerLongTasks = store; + try { + new PerformanceObserver((list) => { + for (const entry of list.getEntries()) store.push(entry.duration); + }).observe({ type: "longtask", buffered: false }); + } catch { + // Long-task observation is best-effort; its absence must not fail the gate. + } + }); + + const flipStartedAt = Date.now(); + await page.getByLabel("Next page").first().click(); + + const secondPage = await waitForPaintedPage(holder, 2); + const flipMs = Date.now() - flipStartedAt; + + expect( + secondPage.inkPixels, + `page 2 sampled ${secondPage.sampled} pixels and found ${secondPage.inkPixels} of ink — the flip landed on a blank page`, + ).toBeGreaterThan(20); + expect( + secondPage.signature, + "page 2 rasterised to the same pixels as page 1 — the flip changed the label but not the raster", + ).not.toBe(firstPage.signature); + + // The route must follow the reader, without remounting pdf.js. + await expect(page.getByLabel("Page number").first()).toHaveValue("2"); + expect(new URL(page.url()).searchParams.get("page")).toBe("2"); + + const longTasks = await page.evaluate( + () => ((window as unknown as Record).__viewerLongTasks as number[] | undefined) ?? [], + ); + const longTaskMs = longTasks.reduce((total, duration) => total + duration, 0); + await test.info().attach("page-flip-raster-cost.json", { + contentType: "application/json", + body: Buffer.from( + JSON.stringify( + { + note: "Task 5 input: main-thread cost of one page flip. Advisory measurement, never asserted on.", + flipToPaintedMs: flipMs, + longTaskCount: longTasks.length, + longTaskTotalMs: Math.round(longTaskMs), + longestTaskMs: Math.round(Math.max(0, ...longTasks)), + canvasBackingPixels: secondPage.width * secondPage.height, + }, + null, + 2, + ), + ), + }); + // Surfaced in the run log too, so the number is readable without downloading + // the artifact. + console.log( + `[viewer-canvas] page flip painted in ${flipMs}ms; ${longTasks.length} long task(s) totalling ${Math.round(longTaskMs)}ms`, + ); + }); +}); From e53904a43305188f0efc68f3dd0256fad782cf1c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 02:52:35 +0000 Subject: [PATCH 02/14] perf(viewer): virtualize the PDF reader into a windowed page column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reader rasterised exactly one page into one canvas, so every page flip on a long guideline was a cold pdf.js render. That is the remaining felt slowness in the document view. The viewer now renders a column of page slots and keeps a small window of them rastered. Each slot reserves its page's box whether or not a canvas is currently in it, so disposing a far page does not move the scroll position a reader navigates by, and pages outside the window drop their backing store instead of holding it until collection. Three constraints shaped this and are resolved explicitly rather than deferred: The raster budget is now document-wide. resolveCanvasRasterPlan bounds ONE canvas against WebKit's ~2^24 ceiling and says nothing about how many exist, so N individually-legal canvases could still exhaust device memory. resolveLiveCanvasWindow caps total retained raster instead. Its useful property is the curve, not the constant: a fit-width phone page never binds against it, while a page at maximum zoom costs the whole per-canvas ceiling and collapses the window to one — render-ahead disappears exactly where retaining neighbours would be most dangerous, with no special-casing of zoom. MAX_CANVAS_PIXELS is unchanged. Render-ahead is reconciled with disableAutoFetch rather than trading it away. Those flags exist because a reader looks at one page and pdf.js would otherwise pull a whole guideline over cellular; rendering neighbours pulls exactly those bytes back. Both flags stay, and the policy is bounded on three independent axes: one page either side, deferred to requestIdleCallback so a fast flip never pays for pages it passes, and switched off entirely under Save-Data or 2g. Three resident pages, never the document. Page sync stays one-way. Intent scrolls the column, scroll position derives the displayed page, and a derived page writes the route only when it did not come from a programmatic scroll. Two real races surfaced while testing this: the route effect re-runs when pdf.js reports its page count, which is always after the reader can have scrolled, so it now acts only when the route asks for somewhere the reader is not; and the in-flight gate is armed when intent is registered rather than a frame later when the scroll executes, since intersections landing in that gap read as reader input and cancel the jump. Multi-page documents get a bounded reading pane so the column is the thing that scrolls; single-page documents keep their existing geometry exactly. The fit scale now derives from the holder's content box rather than clientWidth minus a fixed 16px, which was 16px short at sm:p-4 — invisible with one canvas, a layout shift once slots reserve boxes from the same number. Preserved: the per-run pageToCleanup isolation (Sentry 15801413), canvas zeroing on dispose, the renderZoom debounce with its interim transform, and the isLikelyExpiredUrl recovery path — which now also fires for a neighbour's range 403 while refusing to blank the reader's good page over a failed prefetch. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7 --- .../document-viewer/canvas-raster-budget.ts | 104 +++ .../document-viewer/pdf-canvas-viewer.tsx | 707 +++++++++++++++--- tests/canvas-raster-budget.test.ts | 116 +++ tests/client-performance-boundaries.test.ts | 18 + ...nt-viewer-page-virtualization.dom.test.tsx | 350 +++++++++ 5 files changed, 1182 insertions(+), 113 deletions(-) create mode 100644 tests/document-viewer-page-virtualization.dom.test.tsx diff --git a/src/components/document-viewer/canvas-raster-budget.ts b/src/components/document-viewer/canvas-raster-budget.ts index 2d604a5ada..d425945f4c 100644 --- a/src/components/document-viewer/canvas-raster-budget.ts +++ b/src/components/document-viewer/canvas-raster-budget.ts @@ -23,6 +23,33 @@ /** WebKit's per-canvas ceiling, in device pixels. */ export const MAX_CANVAS_PIXELS = 16_777_216; +/** + * Document-wide ceiling for *retained* canvases, in device pixels. + * + * `MAX_CANVAS_PIXELS` bounds one canvas. Once the reader holds several pages + * live at once, N individually-legal canvases can still exhaust device memory — + * the per-canvas rule says nothing about how many of them exist. At four bytes + * per pixel this budget is roughly 96 MB of backing store, which is the point of + * it: the ceiling is on *total* retained raster, not on any single page. + * + * The interesting property is how it behaves against zoom rather than its exact + * value. A fit-width phone page costs about 1.3 megapixels, so the budget is + * never the binding constraint there and `MAX_RETAINED_CANVASES` is. A page at + * maximum zoom costs the full per-canvas ceiling, so the budget collapses the + * window to exactly one — render-ahead disappears precisely where retaining + * neighbours would be most dangerous, without anyone having to special-case zoom. + */ +export const MAX_LIVE_CANVAS_PIXELS = 24_000_000; + +/** + * Hard cap on retained canvases regardless of how cheap each one is. + * + * A tiny page at low zoom would otherwise let the budget retain dozens, which is + * memory spent on pages the reader is not looking at. This is the ceiling; the + * render-ahead policy below is deliberately far more conservative than it. + */ +export const MAX_RETAINED_CANVASES = 5; + /** Never raster finer than this multiple of CSS pixels, whatever the display reports. */ export const MAX_RENDER_SCALE = 2.5; @@ -75,3 +102,80 @@ export function resolveCanvasRasterPlan({ budgetLimited: affordable < preferred, }; } + +/** + * How many rendered canvases the document may retain at this raster size. + * + * `perCanvasPixels` is the backing-store area one page currently costs — take it + * from `resolveCanvasRasterPlan` for the page geometry and scale actually in + * use, not from a nominal page size, or the window will not tighten when the + * reader zooms in. + * + * Returns at least 1: the page the reader is looking at is never optional. + * An unmeasured or nonsensical size also returns 1, so a viewer that does not + * yet know its page geometry renders the current page alone rather than + * guessing a window it cannot afford. + */ +export function resolveLiveCanvasWindow({ + perCanvasPixels, + maxLivePixels = MAX_LIVE_CANVAS_PIXELS, + maxRetainedCanvases = MAX_RETAINED_CANVASES, +}: { + perCanvasPixels: number; + maxLivePixels?: number; + maxRetainedCanvases?: number; +}): number { + if (!Number.isFinite(perCanvasPixels) || perCanvasPixels <= 0) return 1; + const affordable = Math.floor(maxLivePixels / perCanvasPixels); + return Math.max(1, Math.min(maxRetainedCanvases, affordable)); +} + +/** + * Which pages should hold a rendered canvas right now, most important first. + * + * ## Why this is deliberately timid + * + * The viewer opens its document with `disableAutoFetch` + `disableStream` + * precisely so pdf.js stops pulling a whole guideline down in the background + * when the reader only ever looks at one page — the wrong default on a phone, on + * cellular. Rendering neighbours re-introduces exactly that fetch amplification, + * one page at a time, so the policy is bounded on three independent axes: + * + * 1. **±1 page only.** Resident pages top out at three, never the document. The + * next page leads the previous one because readers move forward. + * 2. **The memory budget can veto it.** `liveCanvasLimit` of 1 — which is what + * `resolveLiveCanvasWindow` returns at high zoom — returns the active page + * alone. + * 3. **A constrained connection vetoes it.** Save-Data and 2g/slow-2g are the + * exact conditions `disableAutoFetch` was chosen for, so on those the viewer + * fetches nothing it was not asked for. + * + * The caller is additionally expected to defer the neighbour render to idle, so + * a reader flipping quickly never pays for pages they have already left. + */ +export function resolveRenderAheadPages({ + activePage, + totalPages, + liveCanvasLimit, + renderAhead = true, + saveData = false, + effectiveType, +}: { + activePage: number; + totalPages: number; + liveCanvasLimit: number; + /** False until the caller's idle callback has fired. */ + renderAhead?: boolean; + saveData?: boolean; + effectiveType?: string; +}): number[] { + const lastPage = totalPages > 0 ? totalPages : activePage; + const current = Math.min(Math.max(Math.trunc(activePage) || 1, 1), Math.max(lastPage, 1)); + if (!renderAhead || liveCanvasLimit <= 1 || saveData) return [current]; + if (effectiveType === "2g" || effectiveType === "slow-2g") return [current]; + + const pages = [current]; + if (current + 1 <= lastPage) pages.push(current + 1); + if (current - 1 >= 1) pages.push(current - 1); + return pages.slice(0, liveCanvasLimit); +} diff --git a/src/components/document-viewer/pdf-canvas-viewer.tsx b/src/components/document-viewer/pdf-canvas-viewer.tsx index 5ffea2c93f..0c6aea48b0 100644 --- a/src/components/document-viewer/pdf-canvas-viewer.tsx +++ b/src/components/document-viewer/pdf-canvas-viewer.tsx @@ -6,6 +6,7 @@ import { useCallback, useEffect, useLayoutEffect, + useMemo, useRef, useState, } from "react"; @@ -13,7 +14,11 @@ import { ExternalLink, FileText, Loader2, RefreshCw } from "lucide-react"; import type { PDFDocumentLoadingTask, PDFDocumentProxy, PDFPageProxy, RenderTask } from "pdfjs-dist"; import { cn, floatingControl } from "@/components/ui-primitives"; -import { resolveCanvasRasterPlan } from "@/components/document-viewer/canvas-raster-budget"; +import { + resolveCanvasRasterPlan, + resolveLiveCanvasWindow, + resolveRenderAheadPages, +} from "@/components/document-viewer/canvas-raster-budget"; import { announce } from "@/components/ui/live-announcer"; import { useViewerGestures } from "@/components/document-viewer/use-viewer-gestures"; import { @@ -28,6 +33,9 @@ const secondaryButton = floatingControl; const MAX_FIT_SCALE = 2.8; +/** Page geometry at pdf.js scale 1, after rotation. */ +type PageGeometry = { width: number; height: number }; + // A signed URL that has passed its (10-min) TTL fails pdf.js with an auth/HTTP // error rather than a parse error. Detect those so the parent can re-issue a // fresh URL, without mistaking a genuinely corrupt PDF for an expiry. @@ -42,11 +50,271 @@ function isLikelyExpiredUrl(error: unknown): boolean { ); } +/** + * The layout scale one page is drawn at — fit-to-width, or the reader's zoom. + * + * Three things must agree on this number or the column jitters: the raster + * itself, the memory budget that decides how many pages may be retained, and the + * box each slot reserves for a page it has not rendered yet. A slot that + * reserves a different height than its page renders at moves every page below it + * the moment that page paints. + * + * `contentWidth` is the holder's content box — its padding already removed. The + * previous single-page viewer subtracted a fixed 16px from `clientWidth`, which + * is right at `p-2` and 16px short at `sm:p-4`, so the fit scale ran slightly + * wide and `max-width: 100%` clamped the canvas back. That was invisible with + * one canvas and an explicit height; with reserved boxes it is a layout shift. + */ +function resolveViewportScale({ + fitWidth, + contentWidth, + baseWidth, + renderZoom, +}: { + fitWidth: boolean; + contentWidth: number; + baseWidth: number; + renderZoom: number; +}): number { + const availableWidth = Math.max(220, contentWidth); + const requested = fitWidth + ? Math.min(MAX_FIT_SCALE, Math.max(VIEWER_MIN_ZOOM, availableWidth / Math.max(baseWidth, 1))) + : renderZoom; + return Math.min(VIEWER_MAX_ZOOM, Math.max(VIEWER_MIN_ZOOM, requested)); +} + +/** Save-Data / effective connection type, when the browser reports them. */ +function readConnectionHints(): { saveData: boolean; effectiveType?: string } { + if (typeof navigator === "undefined") return { saveData: false }; + const connection = (navigator as Navigator & { connection?: { saveData?: boolean; effectiveType?: string } }) + .connection; + return { saveData: Boolean(connection?.saveData), effectiveType: connection?.effectiveType }; +} + +/** + * One page of the reading column. + * + * The slot always occupies its page's box, whether or not a canvas is currently + * rendered into it, so the scroll geometry a reader navigates by does not change + * when a far page is disposed. Pages outside the render window keep their box + * and drop their backing store — that is the whole trade virtualization makes. + * + * Memoised because the owner re-renders on every scroll-derived page change: a + * slot whose inputs did not change must not re-rasterise just because the reader + * moved somewhere else in the document. + */ +const PdfPageSlot = memo(function PdfPageSlot({ + pdf, + pageNumber, + title, + render, + fitWidth, + renderZoom, + rotation, + contentWidth, + fallbackGeometry, + registerSlot, + onGeometry, + onRenderStateChange, + onRenderError, +}: { + pdf: PDFDocumentProxy | null; + pageNumber: number; + title: string; + /** True when this page is inside the current render window. */ + render: boolean; + fitWidth: boolean; + renderZoom: number; + rotation: number; + /** The holder's content-box width, padding already removed. */ + contentWidth: number; + /** The first measured page's geometry, reserving a box for pages not yet loaded. */ + fallbackGeometry: PageGeometry | null; + registerSlot: (pageNumber: number, element: HTMLDivElement | null) => void; + onGeometry: (pageNumber: number, geometry: PageGeometry) => void; + onRenderStateChange: (pageNumber: number, rendering: boolean) => void; + onRenderError: (pageNumber: number, error: Error) => void; +}) { + const slotRef = useRef(null); + const canvasRef = useRef(null); + const [geometry, setGeometry] = useState(null); + const [painted, setPainted] = useState(false); + + useEffect(() => { + registerSlot(pageNumber, slotRef.current); + return () => registerSlot(pageNumber, null); + }, [pageNumber, registerSlot]); + + useEffect(() => { + if (!render || !pdf) return () => undefined; + const activePdf = pdf; + let cancelled = false; + let renderTask: RenderTask | null = null; + // Local to this effect run so a rapid page change cannot clean up the next + // page via a shared ref (Sentry 15801413). + let pageToCleanup: PDFPageProxy | null = null; + + async function renderPage() { + onRenderStateChange(pageNumber, true); + try { + const pdfPage = await activePdf.getPage(pageNumber); + if (cancelled) { + // getPage resolved after we left this page — release it here so the + // next effect's page is never touched by this run's cleanup. + pdfPage.cleanup(); + return; + } + pageToCleanup = pdfPage; + const canvas = canvasRef.current; + if (!canvas) return; + // Rotation is applied in the viewport so width/height already reflect the + // 90°/270° swap — the fit calculation and canvas sizing follow for free. + const baseViewport = pdfPage.getViewport({ scale: 1, rotation }); + const measured = { width: baseViewport.width, height: baseViewport.height }; + setGeometry(measured); + onGeometry(pageNumber, measured); + + const viewportScale = resolveViewportScale({ + fitWidth, + contentWidth, + baseWidth: baseViewport.width, + renderZoom, + }); + // WebKit paints nothing at all above ~2^24 canvas pixels, and this page + // at full device density can ask for three times that. Give up raster + // density before layout size — a soft page reads, a blank one does not. + const { outputScale } = resolveCanvasRasterPlan({ + baseWidth: baseViewport.width, + baseHeight: baseViewport.height, + viewportScale, + devicePixelRatio: window.devicePixelRatio, + }); + const viewport = pdfPage.getViewport({ scale: viewportScale * outputScale, rotation }); + const context = canvas.getContext("2d"); + if (!context) { + onRenderError(pageNumber, new Error("Could not initialize the PDF canvas.")); + return; + } + context.setTransform(1, 0, 0, 1, 0, 0); + context.imageSmoothingEnabled = true; + canvas.width = Math.floor(viewport.width); + canvas.height = Math.floor(viewport.height); + canvas.style.width = `${Math.floor(baseViewport.width * viewportScale)}px`; + canvas.style.height = `${Math.floor(baseViewport.height * viewportScale)}px`; + canvas.style.maxWidth = fitWidth ? "100%" : "none"; + + renderTask = pdfPage.render({ + canvasContext: context, + canvas, + viewport, + }); + await renderTask.promise; + if (!cancelled) setPainted(true); + } catch (renderError) { + if (!cancelled && renderError instanceof Error && renderError.name !== "RenderingCancelledException") { + onRenderError(pageNumber, renderError); + } + } finally { + if (!cancelled) onRenderStateChange(pageNumber, false); + } + } + + renderPage(); + return () => { + cancelled = true; + onRenderStateChange(pageNumber, false); + renderTask?.cancel(); + // Release this run's page only. pdf.js declines while a render is still + // live, so cancel above cannot be cut short by cleanup. + pageToCleanup?.cleanup(); + }; + }, [ + contentWidth, + fitWidth, + onGeometry, + onRenderError, + onRenderStateChange, + pageNumber, + pdf, + render, + renderZoom, + rotation, + ]); + + // A canvas keeps its backing store until the element is collected, which on a + // phone is memory held for a page the reader has already scrolled past — and + // in a virtualized column, for every page they have ever visited. Zeroing the + // dimensions releases it when the page leaves the window, not at some later + // collection. + useEffect( + () => () => { + const canvas = canvasRef.current; + if (!canvas) return; + canvas.width = 0; + canvas.height = 0; + }, + [], + ); + + useEffect(() => { + if (!render) setPainted(false); + }, [render]); + + const reservedGeometry = geometry ?? fallbackGeometry; + const reservedScale = reservedGeometry + ? resolveViewportScale({ fitWidth, contentWidth, baseWidth: reservedGeometry.width, renderZoom }) + : 1; + + return ( +
    + {render ? ( + + ) : ( + + ); +}); + // Memoised: this is the heaviest subtree in the document view (it holds the // pdf.js document and re-rasters the canvas). With stable props from the parent // it skips re-render when unrelated parent state (search, composer, connectivity) // changes, so a keystroke elsewhere never re-rasterises the page. // +// Every piece of per-page state below — the render window, the scroll-derived +// page, the retained canvases — stays inside this component on purpose. Lifting +// any of it into the parent would defeat that boundary, because the parent +// re-renders for reasons that have nothing to do with the document. +// // This component renders source pixels and nothing else. Every viewing control — // page navigation, zoom, fit, rotation, viewing aid, fullscreen — belongs to // DocumentFrame, so the viewer has exactly one toolbar and one page readout. @@ -82,7 +350,6 @@ export const PdfCanvasViewer = memo(function PdfCanvasViewer({ onZoomChange: (zoom: number) => void; }) { const holderRef = useRef(null); - const canvasRef = useRef(null); const [pdf, setPdf] = useState(null); const [page, setPage] = useState(initialPage); const [totalPages, setTotalPages] = useState(0); @@ -115,11 +382,12 @@ export const PdfCanvasViewer = memo(function PdfCanvasViewer({ }, [onZoomChange], ); - const [holderWidth, setHolderWidth] = useState(0); + const [contentWidth, setContentWidth] = useState(0); const [loading, setLoading] = useState(true); - const [rendering, setRendering] = useState(false); + const [renderingPages, setRenderingPages] = useState>(() => new Set()); const [error, setError] = useState(null); const [loadAttempt, setLoadAttempt] = useState(0); + const [referenceGeometry, setReferenceGeometry] = useState(null); const onUrlExpiredRef = useRef(onUrlExpired); const onLoadSuccessRef = useRef(onLoadSuccess); @@ -168,6 +436,7 @@ export const PdfCanvasViewer = memo(function PdfCanvasViewer({ setError(null); setPdf(null); setTotalPages(0); + setReferenceGeometry(null); try { const pdfjs = await import("pdfjs-dist"); pdfjs.GlobalWorkerOptions.workerSrc = new URL( @@ -181,6 +450,12 @@ export const PdfCanvasViewer = memo(function PdfCanvasViewer({ // does not take effect without `disableStream`; the installed types say // so explicitly. // + // Virtualization renders ahead, which pulls bytes this flag exists to + // hold back — so the render-ahead policy is bounded to ±1 page, deferred + // to idle, and switched off entirely under Save-Data or a 2g connection + // (`resolveRenderAheadPages`). Resident pages top out at three; the + // document is still never fetched whole. + // // The trade is that later bytes are requested later, so a signed URL can // expire mid-read. That path already exists and recovers: a range failure // is an auth/HTTP error, `isLikelyExpiredUrl` catches it, and the parent @@ -213,28 +488,198 @@ export const PdfCanvasViewer = memo(function PdfCanvasViewer({ }; }, [loadAttempt, reportUrlExpired, url]); + // --------------------------------------------------------------------------- + // Page ↔ scroll ↔ route synchronisation + // + // Three things can move the reader: the route (deep link, toolbar, rail + // filmstrip), the keyboard, and their own scrolling. Flow is deliberately + // one-way to stop those fighting each other: intent scrolls the column, the + // column's scroll position derives the displayed page, and the derived page + // only writes back to the route when it did NOT come from a programmatic + // scroll. Without that gate a route-driven jump derives its own page, writes + // it back to the route, and re-enters this loop on every frame of the scroll. + // --------------------------------------------------------------------------- + const slotElementsRef = useRef(new Map()); + const intersectionRef = useRef(new Map()); + const observerRef = useRef(null); + const pendingScrollTargetRef = useRef(null); + const pendingScrollTimerRef = useRef(null); + const lastRoutePageRef = useRef(initialPage); + const pageRef = useRef(page); + useEffect(() => { + pageRef.current = page; + }, [page]); + + const registerSlot = useCallback((pageNumber: number, element: HTMLDivElement | null) => { + const previous = slotElementsRef.current.get(pageNumber); + if (previous && observerRef.current) observerRef.current.unobserve(previous); + if (element) { + slotElementsRef.current.set(pageNumber, element); + observerRef.current?.observe(element); + return; + } + slotElementsRef.current.delete(pageNumber); + intersectionRef.current.delete(pageNumber); + }, []); + + const clearPendingScroll = useCallback(() => { + pendingScrollTargetRef.current = null; + if (pendingScrollTimerRef.current !== null) { + window.clearTimeout(pendingScrollTimerRef.current); + pendingScrollTimerRef.current = null; + } + }, []); + + /** + * Declare that a programmatic move to `target` is in flight. + * + * Armed the moment the intent is known rather than when the scroll executes. + * The scroll itself has to wait a frame for the target slot to exist, and any + * intersection that lands in that gap would otherwise be read as the reader + * choosing a page — writing it to the route and cancelling the jump they + * actually asked for. + */ + const armPendingScroll = useCallback((target: number) => { + pendingScrollTargetRef.current = target; + if (pendingScrollTimerRef.current !== null) window.clearTimeout(pendingScrollTimerRef.current); + // The observer releases this the moment the scroll settles on the target; + // the timer is only the backstop for a scroll that never gets there. + pendingScrollTimerRef.current = window.setTimeout(() => { + pendingScrollTargetRef.current = null; + pendingScrollTimerRef.current = null; + }, 600); + }, []); + + /** Move the column to a page without letting the move write back to the route. */ + const scrollToPage = useCallback( + (target: number) => { + const holder = holderRef.current; + const slot = slotElementsRef.current.get(target); + // Scroll the holder itself rather than `scrollIntoView`, which would also + // scroll every ancestor and drag the whole document route with it. + const delta = holder && slot ? slot.getBoundingClientRect().top - holder.getBoundingClientRect().top : 0; + + // Nothing moved, so there is no echo to swallow — and holding the gate open + // anyway would deafen the viewer to the reader's own scrolling for as long + // as the timeout lasts, which is exactly the page-tracking bug it exists to + // prevent in the other direction. + if (!holder || !slot || Math.abs(delta) < 1) { + clearPendingScroll(); + return; + } + + armPendingScroll(target); + holder.scrollTop += delta; + }, + [armPendingScroll, clearPendingScroll], + ); + + useEffect(() => { + const holder = holderRef.current; + if (!holder || typeof IntersectionObserver === "undefined") return () => undefined; + + const observer = new IntersectionObserver( + (entries) => { + for (const entry of entries) { + const slotPage = Number(entry.target.getAttribute("data-page")); + if (!Number.isFinite(slotPage) || slotPage < 1) continue; + intersectionRef.current.set(slotPage, entry.intersectionRatio); + } + + let best = 0; + let bestRatio = -1; + for (const [slotPage, ratio] of intersectionRef.current) { + if (ratio > bestRatio || (ratio === bestRatio && best !== 0 && slotPage < best)) { + best = slotPage; + bestRatio = ratio; + } + } + if (best < 1 || bestRatio <= 0) return; + + // A programmatic jump is still settling: adopt its target and stay quiet + // until the reader's own scrolling takes over again. + if (pendingScrollTargetRef.current !== null) { + if (best === pendingScrollTargetRef.current) clearPendingScroll(); + return; + } + if (best === pageRef.current) return; + pageRef.current = best; + setPage(best); + // Only a reader-driven change writes the route. + if (best !== lastRoutePageRef.current) { + lastRoutePageRef.current = best; + onPageChangeRef.current?.(best); + } + }, + { root: holder, threshold: [0, 0.15, 0.35, 0.6, 0.85, 1] }, + ); + + observerRef.current = observer; + for (const element of slotElementsRef.current.values()) observer.observe(element); + return () => { + observer.disconnect(); + observerRef.current = null; + }; + }, [clearPendingScroll, totalPages]); + + useEffect(() => clearPendingScroll, [clearPendingScroll]); + + // Route-driven page changes (deep link, toolbar, rail filmstrip, keyboard). + // + // This effect re-runs whenever the page count lands, which is always AFTER the + // reader can have started scrolling. It must therefore act only when the route + // is asking for somewhere the reader is not — an unconditional "go to + // initialPage" would drag them back to page 1 every time pdf.js reported its + // page count, and again on every later correction to it. useEffect(() => { const nextPage = Math.max(1, initialPage || 1); const boundedPage = totalPages > 0 ? Math.min(nextPage, totalPages) : nextPage; - const frame = window.requestAnimationFrame(() => { - setPage((current) => (current === boundedPage ? current : boundedPage)); - // pdf.js is authoritative for page count. A deep link or stale indexed - // page_count can leave the frame toolbar on an out-of-range route value - // while the canvas shows the clamped page — reconcile the parent route. - if (totalPages > 0 && boundedPage !== nextPage) { - onPageChangeRef.current?.(boundedPage); - } - }); + lastRoutePageRef.current = boundedPage; + + // pdf.js is authoritative for page count. A deep link or stale indexed + // page_count can leave the frame toolbar on an out-of-range route value + // while the canvas shows the clamped page — reconcile the parent route. This + // is independent of where the reader is, so it happens either way. + if (totalPages > 0 && boundedPage !== nextPage) { + onPageChangeRef.current?.(boundedPage); + } + + if (boundedPage === pageRef.current) return () => undefined; + pageRef.current = boundedPage; + setPage(boundedPage); + armPendingScroll(boundedPage); + // Scroll on the next frame so the slot for the target page exists to measure. + // If it still does not (a deep link that lands before the first raster), the + // re-anchor effect below picks it up when the geometry arrives. + const frame = window.requestAnimationFrame(() => scrollToPage(boundedPage)); + return () => window.cancelAnimationFrame(frame); + }, [armPendingScroll, initialPage, scrollToPage, totalPages]); + + // Re-anchor the column on its current page whenever every slot's box changes + // size underneath the reader. + // + // Reserved boxes are computed from a scale, so a zoom step, a fit toggle, a + // rotation, a resize, or the arrival of the first measured geometry all resize + // every slot at once — and a scroll offset that meant "page 12" before means + // some arbitrary point in the document afterwards. Re-anchoring is what makes + // those actions feel like they operate on the page the reader is looking at. + // It cannot fight the reader's own scrolling because none of these inputs + // change while they scroll. + useEffect(() => { + if (!pdf || totalPages < 1) return () => undefined; + const frame = window.requestAnimationFrame(() => scrollToPage(pageRef.current)); return () => window.cancelAnimationFrame(frame); - }, [initialPage, totalPages]); + }, [contentWidth, fitWidth, pdf, referenceGeometry, renderZoom, rotation, scrollToPage, totalPages]); useEffect(() => { if (!holderRef.current) return; let timeout: number | undefined; const observer = new ResizeObserver((entries) => { + // The content box, so the fit scale and the reserved slot boxes are both + // computed from the width a page can actually occupy. const width = Math.round(entries[0]?.contentRect.width ?? 0); window.clearTimeout(timeout); - timeout = window.setTimeout(() => setHolderWidth(width), 120); + timeout = window.setTimeout(() => setContentWidth(width), 120); }); observer.observe(holderRef.current); @@ -245,113 +690,119 @@ export const PdfCanvasViewer = memo(function PdfCanvasViewer({ }, []); // Settle rapid zoom deltas into a single raster. The interim CSS transform on - // the canvas keeps the view visually correct during this window. + // the column keeps the view visually correct during this window. useEffect(() => { if (renderZoom === zoom) return () => undefined; const timeout = window.setTimeout(() => setRenderZoom(zoom), 140); return () => window.clearTimeout(timeout); }, [renderZoom, zoom]); - useEffect(() => { - if (!pdf || !canvasRef.current || !holderRef.current) return; - const activePdf = pdf; - let cancelled = false; - let renderTask: RenderTask | null = null; - // Local to this effect run so a rapid page change cannot clean up the next - // page via a shared ref (Sentry 15801413). - let pageToCleanup: PDFPageProxy | null = null; + // --------------------------------------------------------------------------- + // Render window + // --------------------------------------------------------------------------- + const perCanvasPixels = useMemo(() => { + if (!referenceGeometry) return 0; + const viewportScale = resolveViewportScale({ + fitWidth, + contentWidth, + baseWidth: referenceGeometry.width, + renderZoom, + }); + const plan = resolveCanvasRasterPlan({ + baseWidth: referenceGeometry.width, + baseHeight: referenceGeometry.height, + viewportScale, + devicePixelRatio: typeof window === "undefined" ? 1 : window.devicePixelRatio, + }); + return plan.width * plan.height; + }, [contentWidth, fitWidth, referenceGeometry, renderZoom]); - async function renderPage() { - setRendering(true); - try { - const pdfPage = await activePdf.getPage(page); - if (cancelled) { - // getPage resolved after we left this page — release it here so the - // next effect's page is never touched by this run's cleanup. - pdfPage.cleanup(); - return; - } - pageToCleanup = pdfPage; - if (!canvasRef.current || !holderRef.current) return; - // Rotation is applied in the viewport so width/height already reflect the - // 90°/270° swap — the fit calculation and canvas sizing follow for free. - const baseViewport = pdfPage.getViewport({ scale: 1, rotation }); - const availableWidth = Math.max(220, holderRef.current.clientWidth - 16); - const requestedScale = fitWidth - ? Math.min(MAX_FIT_SCALE, Math.max(VIEWER_MIN_ZOOM, availableWidth / baseViewport.width)) - : renderZoom; - const viewportScale = Math.min(VIEWER_MAX_ZOOM, Math.max(VIEWER_MIN_ZOOM, requestedScale)); - // WebKit paints nothing at all above ~2^24 canvas pixels, and this page - // at full device density can ask for three times that. Give up raster - // density before layout size — a soft page reads, a blank one does not. - const { outputScale } = resolveCanvasRasterPlan({ - baseWidth: baseViewport.width, - baseHeight: baseViewport.height, - viewportScale, - devicePixelRatio: window.devicePixelRatio, - }); - const viewport = pdfPage.getViewport({ scale: viewportScale * outputScale, rotation }); - const canvas = canvasRef.current; - const context = canvas.getContext("2d"); - if (!context) { - setError("Could not initialize the PDF canvas."); - return; - } - context.setTransform(1, 0, 0, 1, 0, 0); - context.imageSmoothingEnabled = true; - canvas.width = Math.floor(viewport.width); - canvas.height = Math.floor(viewport.height); - canvas.style.width = `${Math.floor(baseViewport.width * viewportScale)}px`; - canvas.style.height = `${Math.floor(baseViewport.height * viewportScale)}px`; - canvas.style.maxWidth = fitWidth ? "100%" : "none"; + const liveCanvasLimit = useMemo(() => resolveLiveCanvasWindow({ perCanvasPixels }), [perCanvasPixels]); - renderTask = pdfPage.render({ - canvasContext: context, - canvas, - viewport, - }); - await renderTask.promise; - } catch (renderError) { - if (!cancelled && renderError instanceof Error && renderError.name !== "RenderingCancelledException") { - if (isLikelyExpiredUrl(renderError)) reportUrlExpired(); - setError(renderError.message); - } - } finally { - if (!cancelled) setRendering(false); - } + // Neighbour pages wait for idle. A reader flipping quickly through a document + // never reaches idle, so they never pay for the pages they are passing. + const [renderAheadReady, setRenderAheadReady] = useState(false); + useEffect(() => { + setRenderAheadReady(false); + if (!pdf || totalPages <= 1 || liveCanvasLimit <= 1) return () => undefined; + + if (typeof window.requestIdleCallback === "function") { + const handle = window.requestIdleCallback(() => setRenderAheadReady(true), { timeout: 1500 }); + return () => window.cancelIdleCallback(handle); } + const timer = window.setTimeout(() => setRenderAheadReady(true), 400); + return () => window.clearTimeout(timer); + }, [fitWidth, liveCanvasLimit, page, pdf, renderZoom, rotation, totalPages]); - renderPage(); - return () => { - cancelled = true; - renderTask?.cancel(); - // Release this run's page only. pdf.js declines while a render is still - // live, so cancel above cannot be cut short by cleanup. - pageToCleanup?.cleanup(); - }; - }, [fitWidth, holderWidth, page, pdf, renderZoom, reportUrlExpired, rotation]); + const renderPages = useMemo(() => { + const { saveData, effectiveType } = readConnectionHints(); + return new Set( + resolveRenderAheadPages({ + activePage: page, + totalPages, + liveCanvasLimit, + renderAhead: renderAheadReady, + saveData, + effectiveType, + }), + ); + }, [liveCanvasLimit, page, renderAheadReady, totalPages]); - // A canvas keeps its backing store until the element is collected, which on a - // phone is memory held for a document the reader has already left. Zeroing the - // dimensions releases it at unmount instead. - useEffect( - () => () => { - const canvas = canvasRef.current; - if (!canvas) return; - canvas.width = 0; - canvas.height = 0; + // The first page that renders seeds the reserved box for every page that has + // not loaded yet — deliberately the first to render rather than page 1, so a + // deep link straight to page 50 still gets a sized column. Measuring every page + // up front would mean fetching every page, which is the cost virtualization + // exists to avoid; mixed-size documents correct each slot as it renders. + const handleGeometry = useCallback((_pageNumber: number, geometry: PageGeometry) => { + setReferenceGeometry((current) => current ?? geometry); + }, []); + + const handleRenderStateChange = useCallback((pageNumber: number, rendering: boolean) => { + setRenderingPages((current) => { + if (rendering === current.has(pageNumber)) return current; + const next = new Set(current); + if (rendering) next.add(pageNumber); + else next.delete(pageNumber); + return next; + }); + }, []); + + const handleRenderError = useCallback( + (pageNumber: number, renderError: Error) => { + // A range request for ANY page can 403 once the signed URL expires, so a + // neighbour's failure is just as much an expiry signal as the reader's own + // page — report it either way, or a background prefetch failure would sit + // there un-recovered until the reader flipped into it. + if (isLikelyExpiredUrl(renderError)) reportUrlExpired(); + // Only the page the reader is actually looking at may take over the frame. + // A neighbour rendered ahead is invisible; blanking a perfectly good page + // because a prefetch failed would be a worse outcome than the prefetch + // simply not arriving. + if (pageNumber === pageRef.current) setError(renderError.message); }, - [], + [reportUrlExpired], ); - const jumpToPage = useCallback( + const pagesReady = Boolean(pdf && totalPages > 0 && !loading); + const rendering = renderingPages.has(page); + const multiPage = pagesReady && totalPages > 1; + const pageNumbers = useMemo( + () => Array.from({ length: Math.max(totalPages, 0) }, (_, index) => index + 1), + [totalPages], + ); + + const goToPage = useCallback( (nextPage: number) => { const bounded = Math.min(Math.max(nextPage, 1), totalPages || nextPage); - if (bounded === page) return; + if (bounded === pageRef.current) return; + pageRef.current = bounded; setPage(bounded); - onPageChange?.(bounded); + lastRoutePageRef.current = bounded; + armPendingScroll(bounded); + scrollToPage(bounded); + onPageChangeRef.current?.(bounded); }, - [onPageChange, page, totalPages], + [armPendingScroll, scrollToPage, totalPages], ); const zoomBy = useCallback( @@ -362,7 +813,6 @@ export const PdfCanvasViewer = memo(function PdfCanvasViewer({ [setFitWidth, setZoom], ); - const pagesReady = Boolean(pdf && totalPages > 0 && !loading); // While a zoom step waits for its debounced raster, scale the last raster with // a CSS transform so the view tracks the target zoom instantly. It resets to 1 // the moment `renderZoom` catches up and the crisp raster paints. Fit mode is @@ -416,11 +866,11 @@ export const PdfCanvasViewer = memo(function PdfCanvasViewer({ switch (event.key) { case "ArrowLeft": event.preventDefault(); - jumpToPage(page - 1); + goToPage(page - 1); break; case "ArrowRight": event.preventDefault(); - jumpToPage(page + 1); + goToPage(page + 1); break; case "+": case "=": @@ -447,6 +897,7 @@ export const PdfCanvasViewer = memo(function PdfCanvasViewer({ >
    ) : ( - + > + {pageNumbers.map((pageNumber) => ( + + ))} +
    )} diff --git a/tests/canvas-raster-budget.test.ts b/tests/canvas-raster-budget.test.ts index 92780b4f0c..cb71749c31 100644 --- a/tests/canvas-raster-budget.test.ts +++ b/tests/canvas-raster-budget.test.ts @@ -2,8 +2,12 @@ import { describe, expect, it } from "vitest"; import { MAX_CANVAS_PIXELS, + MAX_LIVE_CANVAS_PIXELS, MAX_RENDER_SCALE, + MAX_RETAINED_CANVASES, resolveCanvasRasterPlan, + resolveLiveCanvasWindow, + resolveRenderAheadPages, } from "@/components/document-viewer/canvas-raster-budget"; import { VIEWER_MAX_ZOOM } from "@/components/document-viewer/viewer-zoom"; @@ -81,3 +85,115 @@ describe("canvas raster budget", () => { } }); }); + +describe("document-wide live canvas window", () => { + /** The pixels one page actually costs at a given layout scale and density. */ + const pageCost = (viewportScale: number, devicePixelRatio: number) => { + const plan = resolveCanvasRasterPlan({ ...A4, viewportScale, devicePixelRatio }); + return plan.width * plan.height; + }; + + it("never drops the page the reader is looking at, however expensive it is", () => { + // A page at maximum zoom costs the entire per-canvas ceiling on its own. + const window = resolveLiveCanvasWindow({ perCanvasPixels: MAX_CANVAS_PIXELS }); + + expect(window).toBe(1); + }); + + it("collapses to the current page alone at maximum zoom, so render-ahead cannot stack canvases", () => { + // This is the property the per-canvas budget cannot express: three + // individually legal canvases at this size are 200MB of backing store. + const perCanvasPixels = pageCost(VIEWER_MAX_ZOOM, IPHONE_DPR); + + expect(perCanvasPixels * 3).toBeGreaterThan(MAX_LIVE_CANVAS_PIXELS); + expect(resolveLiveCanvasWindow({ perCanvasPixels })).toBe(1); + }); + + it("affords render-ahead at the fit-width sizes a phone actually reads at", () => { + // A phone page in fit mode: ~390 CSS px wide, so scale ~0.65 on an A4 sheet. + const perCanvasPixels = pageCost(0.65, IPHONE_DPR); + + expect(perCanvasPixels * 3).toBeLessThan(MAX_LIVE_CANVAS_PIXELS); + expect(resolveLiveCanvasWindow({ perCanvasPixels })).toBeGreaterThanOrEqual(3); + }); + + it("caps retention even when each canvas is nearly free", () => { + expect(resolveLiveCanvasWindow({ perCanvasPixels: 1 })).toBe(MAX_RETAINED_CANVASES); + }); + + it("renders the current page alone while the page size is still unknown", () => { + // Before any page has rendered there is nothing to size a window from, and + // guessing one would commit memory the viewer has not measured. + for (const perCanvasPixels of [0, -1, Number.NaN, Number.POSITIVE_INFINITY]) { + expect(resolveLiveCanvasWindow({ perCanvasPixels })).toBe(1); + } + }); + + it("tightens monotonically as the reader zooms in", () => { + let previous = Number.POSITIVE_INFINITY; + for (let viewportScale = 0.5; viewportScale <= VIEWER_MAX_ZOOM; viewportScale += 0.25) { + const window = resolveLiveCanvasWindow({ perCanvasPixels: pageCost(viewportScale, IPHONE_DPR) }); + expect(window).toBeLessThanOrEqual(previous); + previous = window; + } + expect(previous).toBe(1); + }); +}); + +describe("render-ahead policy", () => { + const base = { activePage: 5, totalPages: 40, liveCanvasLimit: 3 }; + + it("reads one page ahead and one behind, forward first", () => { + expect(resolveRenderAheadPages(base)).toEqual([5, 6, 4]); + }); + + it("renders nothing beyond the current page until the caller reaches idle", () => { + // A reader flipping quickly never reaches idle, so they never pay to raster + // the pages they are passing through. + expect(resolveRenderAheadPages({ ...base, renderAhead: false })).toEqual([5]); + }); + + it("lets the memory budget veto render-ahead outright", () => { + expect(resolveRenderAheadPages({ ...base, liveCanvasLimit: 1 })).toEqual([5]); + }); + + it("obeys a smaller budget than the policy wants", () => { + expect(resolveRenderAheadPages({ ...base, liveCanvasLimit: 2 })).toEqual([5, 6]); + }); + + it("fetches nothing it was not asked for under Save-Data or a 2g connection", () => { + // These are the exact conditions `disableAutoFetch` was chosen for. Reading + // ahead there would re-introduce the fetch amplification that flag prevents. + expect(resolveRenderAheadPages({ ...base, saveData: true })).toEqual([5]); + expect(resolveRenderAheadPages({ ...base, effectiveType: "2g" })).toEqual([5]); + expect(resolveRenderAheadPages({ ...base, effectiveType: "slow-2g" })).toEqual([5]); + // A healthy connection is not vetoed. + expect(resolveRenderAheadPages({ ...base, effectiveType: "4g" })).toEqual([5, 6, 4]); + }); + + it("never reads past either end of the document", () => { + expect(resolveRenderAheadPages({ ...base, activePage: 1 })).toEqual([1, 2]); + expect(resolveRenderAheadPages({ ...base, activePage: 40 })).toEqual([40, 39]); + expect(resolveRenderAheadPages({ ...base, activePage: 1, totalPages: 1 })).toEqual([1]); + }); + + it("clamps an out-of-range or malformed active page onto the document", () => { + expect(resolveRenderAheadPages({ ...base, activePage: 999 })).toEqual([40, 39]); + expect(resolveRenderAheadPages({ ...base, activePage: 0 })).toEqual([1, 2]); + expect(resolveRenderAheadPages({ ...base, activePage: -3 })).toEqual([1, 2]); + }); + + it("keeps the reader's page first so the caller can prioritise it", () => { + for (const activePage of [1, 2, 20, 39, 40]) { + expect(resolveRenderAheadPages({ ...base, activePage })[0]).toBe(activePage); + } + }); + + it("never exceeds the live-canvas ceiling it was given", () => { + for (let liveCanvasLimit = 1; liveCanvasLimit <= MAX_RETAINED_CANVASES; liveCanvasLimit += 1) { + const pages = resolveRenderAheadPages({ ...base, liveCanvasLimit }); + expect(pages.length).toBeLessThanOrEqual(liveCanvasLimit); + expect(new Set(pages).size).toBe(pages.length); + } + }); +}); diff --git a/tests/client-performance-boundaries.test.ts b/tests/client-performance-boundaries.test.ts index 9ac01749b4..2db48d935e 100644 --- a/tests/client-performance-boundaries.test.ts +++ b/tests/client-performance-boundaries.test.ts @@ -85,6 +85,24 @@ describe("fixture-free client performance boundaries", () => { expect(pdfViewer).not.toContain("renderedPageRef.current?.cleanup()"); }); + it("bounds retained page canvases by a document-wide budget, not only the per-canvas one", () => { + const pdfViewer = source("src/components/document-viewer/pdf-canvas-viewer.tsx"); + + // `resolveCanvasRasterPlan` bounds ONE canvas against WebKit's ~2^24 ceiling. + // Once several pages are live at once, N individually legal canvases can still + // exhaust device memory, so the render window must come from the document-wide + // budget rather than a hardcoded page count. The DOM test cannot catch a + // regression here: its fixture canvases are small enough that the budget never + // binds, so a viewer that dropped it would still window correctly there. + expect(pdfViewer).toContain("resolveLiveCanvasWindow"); + expect(pdfViewer).toContain("resolveRenderAheadPages"); + expect(pdfViewer).toMatch(/liveCanvasLimit/); + + // Render-ahead must stay deferred. Rendering neighbours eagerly re-introduces + // exactly the background fetching `disableAutoFetch` above exists to prevent. + expect(pdfViewer).toContain("requestIdleCallback"); + }); + it("revalidates cached document download URLs on every viewer action", () => { const viewer = source("src/components/DocumentViewer.tsx"); diff --git a/tests/document-viewer-page-virtualization.dom.test.tsx b/tests/document-viewer-page-virtualization.dom.test.tsx new file mode 100644 index 0000000000..c6684b8e6e --- /dev/null +++ b/tests/document-viewer-page-virtualization.dom.test.tsx @@ -0,0 +1,350 @@ +import { act, cleanup, render, screen, waitFor } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +/** + * Page virtualization behaviour for the PDF reader, in a real React tree. + * + * The budget arithmetic has its own unit tests and the raster has a browser + * gate; what neither covers is the wiring between them — which slots exist, how + * many hold a canvas at once, when a disposed page releases its backing store, + * and whether a scroll-derived page change writes the route exactly once. Those + * are the parts that would silently regress into "renders every page" or + * "remounts pdf.js on every flip". + * + * jsdom has no 2D canvas, so `getContext` is stubbed to a recording double and + * pdf.js is mocked entirely. That is enough for the wiring; it is deliberately + * NOT enough to claim the page paints, which is what + * `tests/ui-document-canvas.spec.ts` exists for. + */ + +const PAGE_COUNT = 8; +const PAGE_WIDTH = 595; +const PAGE_HEIGHT = 842; + +const renderCalls: number[] = []; +const cleanupCalls: number[] = []; +let getPageCalls: number[] = []; +let getDocumentOptions: Record | null = null; + +vi.mock("pdfjs-dist", () => { + const makePage = (pageNumber: number) => ({ + getViewport: ({ scale = 1 }: { scale?: number; rotation?: number }) => ({ + width: PAGE_WIDTH * scale, + height: PAGE_HEIGHT * scale, + }), + render: () => { + renderCalls.push(pageNumber); + return { promise: Promise.resolve(), cancel: () => {} }; + }, + cleanup: () => { + cleanupCalls.push(pageNumber); + }, + }); + + return { + GlobalWorkerOptions: { workerSrc: "" }, + getDocument: (options: Record) => { + getDocumentOptions = options; + return { + promise: Promise.resolve({ + numPages: PAGE_COUNT, + getPage: (pageNumber: number) => { + getPageCalls.push(pageNumber); + return Promise.resolve(makePage(pageNumber)); + }, + }), + destroy: () => Promise.resolve(), + }; + }, + }; +}); + +type ObserverRecord = { + callback: IntersectionObserverCallback; + observed: Set; +}; + +let observers: ObserverRecord[] = []; +/** + * Idle callbacks are captured rather than run, so a test can assert what the + * viewer does BEFORE render-ahead fires. A timer-backed stub would race every + * `waitFor` and make "only the reader's page" unassertable. + */ +let idleCallbacks: (() => void)[] = []; + +async function flushIdle() { + const pending = idleCallbacks; + idleCallbacks = []; + await act(async () => { + for (const callback of pending) callback(); + await new Promise((resolve) => setTimeout(resolve, 20)); + }); +} + +/** Drive the slot observer as though the reader had scrolled to a page. */ +function scrollToPage(pageNumber: number) { + for (const observer of observers) { + const entries = [...observer.observed].map((target) => ({ + target, + intersectionRatio: Number(target.getAttribute("data-page")) === pageNumber ? 1 : 0, + isIntersecting: Number(target.getAttribute("data-page")) === pageNumber, + })) as unknown as IntersectionObserverEntry[]; + if (entries.length === 0) continue; + observer.callback(entries, {} as IntersectionObserver); + } +} + +beforeEach(() => { + renderCalls.length = 0; + cleanupCalls.length = 0; + getPageCalls = []; + getDocumentOptions = null; + observers = []; + idleCallbacks = []; + + vi.stubGlobal( + "IntersectionObserver", + class { + readonly observed = new Set(); + + constructor(private readonly callback: IntersectionObserverCallback) { + observers.push({ callback, observed: this.observed }); + } + + observe(target: Element) { + this.observed.add(target); + } + + unobserve(target: Element) { + this.observed.delete(target); + } + + disconnect() { + this.observed.clear(); + observers = observers.filter((record) => record.observed !== this.observed); + } + + takeRecords() { + return []; + } + }, + ); + + vi.stubGlobal( + "ResizeObserver", + class { + observe() {} + unobserve() {} + disconnect() {} + }, + ); + + // jsdom throws on getContext; the raster path only needs a recording double. + vi.spyOn(HTMLCanvasElement.prototype, "getContext").mockImplementation( + () => ({ setTransform: () => {}, imageSmoothingEnabled: false }) as unknown as CanvasRenderingContext2D, + ); + + // requestIdleCallback drives render-ahead; jsdom does not implement it, and + // leaving it undefined would silently exercise only the setTimeout fallback. + vi.stubGlobal("requestIdleCallback", (callback: () => void) => { + idleCallbacks.push(callback); + return idleCallbacks.length; + }); + vi.stubGlobal("cancelIdleCallback", () => {}); +}); + +afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +async function renderViewer(props: Record = {}) { + const { PdfCanvasViewer } = await import("@/components/document-viewer/pdf-canvas-viewer"); + const onPageChange = vi.fn(); + const viewer = (overrides: Record) => ( + + ); + + let rerenderViewer: (ui: React.ReactElement) => void = () => {}; + await act(async () => { + ({ rerender: rerenderViewer } = render(viewer({}))); + }); + await waitFor(() => expect(screen.getAllByTestId("pdf-page-slot").length).toBe(PAGE_COUNT)); + + /** Re-render as the route would: a changed `initialPage` on the same tree. */ + const navigateRouteTo = async (page: number) => { + await act(async () => { + rerenderViewer(viewer({ initialPage: page })); + await new Promise((resolve) => setTimeout(resolve, 50)); + }); + }; + + return { onPageChange, navigateRouteTo }; +} + +/** + * Give the holder and every slot a real vertical layout. + * + * jsdom reports every rect as zero, which makes the viewer's scroll-to-page a + * no-op — and a no-op scroll deliberately does NOT arm the pending gate, so + * without this the gate can never be exercised at all. + */ +function stubColumnLayout(slotHeight = 100) { + vi.spyOn(HTMLElement.prototype, "getBoundingClientRect").mockImplementation(function (this: HTMLElement) { + const slotPage = Number(this.getAttribute?.("data-page") ?? Number.NaN); + const top = Number.isFinite(slotPage) ? (slotPage - 1) * slotHeight : 0; + return { + top, + bottom: top + slotHeight, + left: 0, + right: 600, + width: 600, + height: slotHeight, + x: 0, + y: top, + toJSON: () => ({}), + } as DOMRect; + }); +} + +const renderedPages = () => + screen + .getAllByTestId("pdf-page-slot") + .filter((slot) => slot.getAttribute("data-rendered") === "true") + .map((slot) => Number(slot.getAttribute("data-page"))); + +describe("PDF reader page virtualization", () => { + it("reserves a slot for every page but rasters only the reader's page on arrival", async () => { + await renderViewer(); + + expect(screen.getAllByTestId("pdf-page-slot")).toHaveLength(PAGE_COUNT); + // Before idle, render-ahead has not fired: exactly one canvas exists, which + // is what keeps opening a 300-page guideline from costing 300 rasters. + expect(renderedPages()).toEqual([1]); + expect(screen.getByLabelText("Synthetic monitoring protocol page 1")).toBeTruthy(); + expect(screen.queryByLabelText("Synthetic monitoring protocol page 2")).toBeNull(); + }); + + it("keeps range-fetching on demand rather than pulling the whole document", async () => { + await renderViewer(); + + // Render-ahead pulls bytes these flags hold back, so it must not have + // quietly dropped them to make neighbour rendering easier. + expect(getDocumentOptions).toMatchObject({ disableAutoFetch: true, disableStream: true }); + }); + + it("reads exactly one page ahead and one behind, and only once idle", async () => { + await renderViewer({ initialPage: 4 }); + expect(renderedPages()).toEqual([4]); + + await flushIdle(); + + await waitFor(() => expect(renderedPages().sort((a, b) => a - b)).toEqual([3, 4, 5])); + // Never the whole document, and never a page two away. + expect(getPageCalls).not.toContain(1); + expect(getPageCalls).not.toContain(6); + }); + + it("releases the backing store of a page that leaves the render window", async () => { + await renderViewer(); + const canvas = screen.getByLabelText("Synthetic monitoring protocol page 1") as HTMLCanvasElement; + expect(canvas.tagName).toBe("CANVAS"); + + await act(async () => { + scrollToPage(6); + await new Promise((resolve) => setTimeout(resolve, 50)); + }); + + await waitFor(() => expect(renderedPages()).toContain(6)); + expect(renderedPages()).not.toContain(1); + // pdf.js page resources are released too, not just the canvas element. + expect(cleanupCalls).toContain(1); + }); + + it("writes the route once when the reader scrolls to a new page, and not at all when they scroll back to it", async () => { + const { onPageChange } = await renderViewer(); + + await act(async () => { + scrollToPage(3); + await new Promise((resolve) => setTimeout(resolve, 50)); + }); + await waitFor(() => expect(onPageChange).toHaveBeenCalledWith(3)); + const callsAfterFirstScroll = onPageChange.mock.calls.length; + + // Re-reporting the same page must not re-enter the route. This is the loop + // the pending-scroll gate exists to prevent: route -> scroll -> derive -> + // route, once per animation frame. + await act(async () => { + scrollToPage(3); + scrollToPage(3); + await new Promise((resolve) => setTimeout(resolve, 50)); + }); + expect(onPageChange.mock.calls.length).toBe(callsAfterFirstScroll); + }); + + it("does not write the route back when the route itself moved the reader", async () => { + const { onPageChange, navigateRouteTo } = await renderViewer(); + onPageChange.mockClear(); + + // A rail filmstrip jump arrives as a changed `initialPage`. The scroll it + // triggers derives its own page, and echoing that back to the route is the + // feedback loop that would re-enter this effect on every frame. + await navigateRouteTo(5); + await act(async () => { + scrollToPage(5); + await new Promise((resolve) => setTimeout(resolve, 50)); + }); + + expect(onPageChange).not.toHaveBeenCalledWith(5); + expect(renderedPages()).toContain(5); + }); + + it("ignores the pages a route-driven scroll passes over on its way to the target", async () => { + stubColumnLayout(); + const { onPageChange, navigateRouteTo } = await renderViewer(); + onPageChange.mockClear(); + + await navigateRouteTo(7); + // Mid-flight intersections while the column is still travelling must not be + // mistaken for the reader choosing those pages. + await act(async () => { + scrollToPage(3); + scrollToPage(5); + await new Promise((resolve) => setTimeout(resolve, 20)); + }); + expect(onPageChange).not.toHaveBeenCalled(); + + // Arriving at the target releases the gate... + await act(async () => { + scrollToPage(7); + await new Promise((resolve) => setTimeout(resolve, 20)); + }); + expect(onPageChange).not.toHaveBeenCalled(); + + // ...and the reader's own scrolling is heard again immediately afterwards. + await act(async () => { + scrollToPage(8); + await new Promise((resolve) => setTimeout(resolve, 20)); + }); + expect(onPageChange).toHaveBeenCalledWith(8); + }); + + it("reconciles a deep link past the end of the document onto its last page", async () => { + const { onPageChange } = await renderViewer({ initialPage: 99 }); + + await waitFor(() => expect(onPageChange).toHaveBeenCalledWith(PAGE_COUNT)); + expect(renderedPages()).toContain(PAGE_COUNT); + }); +}); From b406a3259a4faa28decbfe5979b4b8b7c21cb0c2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 02:56:49 +0000 Subject: [PATCH 03/14] perf(viewer): window the document rail's figure cards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rail mapped every clinical figure and every audit figure into a DocumentImage on mount. That row is not cheap: it parses table markdown, decides whether a structured AccessibleTable can render at all, computes quality warnings and evidence tags, and mounts a SignedImage frame. A guideline with ninety indexed tables paid all of it during hydration, before the reader had opened the section, and the audit list underneath paid it again. DocumentImageList renders a window of six and grows it as a sentinel comes into view, with an explicit control to reveal the rest. Short lists — the overwhelming majority of indexed documents — render whole and get no extra chrome at all. The window is derived during render rather than synchronised in an effect, so a list that shrinks underneath an expanded reader clamps immediately instead of pointing past the end of the array for a frame. The filmstrip is left whole on purpose: it is one button per figure with no image behind it, and it is the cheap way to reach any page. One correction to the Phase 3 brief, recorded in the test rather than assumed either way. The brief says collapsed audit rows "still mint signed URLs". SignedImage already defers its fetch behind an IntersectionObserver and a closed
    is display:none, so that claim is at least doubtful — but it is a claim about real browser layout, and jsdom does no layout, so nothing available here settles it. What is certain, and is what this commit removes, is the mounting cost, which applies whether the section is open or shut. The rail's observer also uses a 320px root margin rather than SignedImage's 640px, so the two do not both run far ahead of the viewport once the section does open. No virtualization dependency: rows have data-dependent heights, a windowed list needs no measurement to be correct, and check:bundle-budget totals every built chunk, so a library would land straight on it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7 --- .../document-viewer/document-rail-panels.tsx | 23 ++- .../document-viewer/source-panels.tsx | 90 ++++++++++ tests/document-rail-image-window.dom.test.tsx | 155 ++++++++++++++++++ 3 files changed, 261 insertions(+), 7 deletions(-) create mode 100644 tests/document-rail-image-window.dom.test.tsx diff --git a/src/components/document-viewer/document-rail-panels.tsx b/src/components/document-viewer/document-rail-panels.tsx index 8541bc6822..89eb7cf44a 100644 --- a/src/components/document-viewer/document-rail-panels.tsx +++ b/src/components/document-viewer/document-rail-panels.tsx @@ -8,7 +8,7 @@ import { DocumentSectionIndexCard } from "@/components/document-viewer/section-n import { documentIndexingSectionId } from "@/components/document-viewer/section-index"; import { ClinicalSummaryProfile, - DocumentImage, + DocumentImageList, DocumentSectionSummary, FormattedHighYieldSummary, TableReviewPanel, @@ -263,10 +263,16 @@ export function DocumentViewerRail({ /> ) : ( <> + {/* The filmstrip stays whole: it is one button per figure with no + image behind it, so it is the cheap way to reach any page. The + detailed cards below it are what get windowed. */} - {clinicalImages.map((image) => ( - - ))} + )} {!effectiveLoadingDocument && auditImages.length > 0 ? ( @@ -275,9 +281,12 @@ export function DocumentViewerRail({ Administrative/reference tables retained for audit ({auditImages.length})
    - {auditImages.map((image) => ( - - ))} +
    ) : null} diff --git a/src/components/document-viewer/source-panels.tsx b/src/components/document-viewer/source-panels.tsx index 712a7324b8..c8deaf7414 100644 --- a/src/components/document-viewer/source-panels.tsx +++ b/src/components/document-viewer/source-panels.tsx @@ -431,6 +431,96 @@ export function DocumentImage({ ); } +/** + * Rows revealed before the reader has scrolled anywhere near the end of them. + * Six covers the great majority of indexed documents outright, so most rails + * never mount a sentinel at all. + */ +const RAIL_IMAGE_WINDOW = 6; + +/** + * The figure rail, windowed. + * + * A `DocumentImage` is not a cheap row. Each one parses table markdown, decides + * whether a structured `AccessibleTable` can render at all, computes quality + * warnings and evidence tags, and mounts a `SignedImage` frame. A guideline with + * ninety indexed tables paid all of that on hydration, for every row, before the + * reader had opened the section — and the audit list underneath paid it again. + * + * Signed-URL fetches were already deferred behind `SignedImage`'s own + * IntersectionObserver, so this is not primarily about network. What it removes + * is the mount cost of rows nobody has looked at, and the `640px` observer + * root-margin fanning requests out well past a tall desktop viewport once the + * section does open. + * + * Deliberately no virtualization dependency: rows have data-dependent heights, a + * windowed list needs no measurement to be correct, and `check:bundle-budget` + * totals every built chunk — a library here would land straight on it. + */ +export function DocumentImageList({ + images, + activePage, + onSelectPage, + revealLabel, +}: { + images: ImageRow[]; + activePage?: number; + onSelectPage?: (page: number) => void; + /** Accessible label for the manual reveal control. */ + revealLabel: string; +}) { + const [requestedCount, setRequestedCount] = useState(RAIL_IMAGE_WINDOW); + const sentinelRef = useRef(null); + // Derived rather than synchronised: the list shrinking underneath the reader (a + // reindex, or navigating documents without remounting the rail) clamps here + // during render, so there is no effect that can leave the window pointing past + // the end of the array for a frame. + const visibleCount = Math.min(Math.max(requestedCount, RAIL_IMAGE_WINDOW), images.length); + const remaining = Math.max(images.length - visibleCount, 0); + + useEffect(() => { + if (remaining === 0) return () => undefined; + const sentinel = sentinelRef.current; + if (!sentinel || !("IntersectionObserver" in window)) return () => undefined; + + const observer = new IntersectionObserver( + (entries) => { + if (!entries.some((entry) => entry.isIntersecting)) return; + setRequestedCount((current) => Math.min(current + RAIL_IMAGE_WINDOW, images.length)); + }, + // Enough to land the next rows before they are reached, far less than the + // signed-image observer's own 640px so the two do not both run far ahead. + { rootMargin: "320px 0px" }, + ); + observer.observe(sentinel); + return () => observer.disconnect(); + }, [images.length, remaining]); + + return ( + <> + {images.slice(0, visibleCount).map((image) => ( + + ))} + {remaining > 0 ? ( + // The control is not only a no-IntersectionObserver fallback: it is the + // only way to reach the rest of the list without scrolling, which is what + // a keyboard or screen-reader user needs. +
    + +
    + ) : null} + + ); +} + export function TableReviewPanel({ tableFacts, canReview, diff --git a/tests/document-rail-image-window.dom.test.tsx b/tests/document-rail-image-window.dom.test.tsx new file mode 100644 index 0000000000..10486dd28c --- /dev/null +++ b/tests/document-rail-image-window.dom.test.tsx @@ -0,0 +1,155 @@ +import { act, cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +// Each row mounts a SignedImage, which reads the auth session. Stable references +// on purpose: useSignedImageUrl lists them in its effect deps, so a fresh object +// per render would re-run the fetch effect on every window growth. +vi.mock("@/lib/supabase/client", () => { + const authorizationHeader = {}; + const markSessionExpired = vi.fn(); + return { useAuthSession: () => ({ authorizationHeader, markSessionExpired, session: null }) }; +}); + +import { DocumentImageList } from "@/components/document-viewer/source-panels"; +import type { ImageRow } from "@/components/document-viewer/types"; + +/** + * Windowing for the document rail's figure cards. + * + * A `DocumentImage` is an expensive row — table-markdown parsing, structured + * table feasibility, quality warnings, evidence tags, and a `SignedImage` frame + * — and a long guideline mounted every one of them on hydration. + * + * ## What this test can and cannot establish about signed-URL fetches + * + * The Phase 3 brief says collapsed audit rows "still mint signed URLs". That is + * a claim about a real browser, and it cannot be settled here: `SignedImage` + * defers its fetch behind an `IntersectionObserver`, a closed `
    ` is + * `display: none`, and jsdom performs no layout, so nothing in this environment + * decides whether such an element ever intersects. What IS decidable, and is + * asserted below, is the row-mounting cost — which is real whether the section + * is open or shut, and is what the window removes. + */ + +const image = (index: number): ImageRow => + ({ + id: `image-${index}`, + page_number: index + 1, + caption: `Figure ${index + 1}`, + image_type: "table", + labels: [], + }) as unknown as ImageRow; + +const images = (count: number) => Array.from({ length: count }, (_, index) => image(index)); + +type SentinelObserver = { callback: IntersectionObserverCallback; targets: Element[] }; +let sentinelObservers: SentinelObserver[] = []; + +beforeEach(() => { + sentinelObservers = []; + vi.stubGlobal( + "IntersectionObserver", + class { + private readonly record: SentinelObserver; + + constructor(callback: IntersectionObserverCallback) { + this.record = { callback, targets: [] }; + sentinelObservers.push(this.record); + } + observe(target: Element) { + this.record.targets.push(target); + } + unobserve(target: Element) { + this.record.targets = this.record.targets.filter((element) => element !== target); + } + // Disconnected observers must actually stop firing, or a stale sentinel + // from the previous window would grow the list a second time per scroll. + disconnect() { + sentinelObservers = sentinelObservers.filter((record) => record !== this.record); + } + takeRecords() { + return []; + } + }, + ); +}); + +afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); +}); + +/** Fire every live sentinel observer as though it had scrolled into view. */ +function reachSentinel() { + for (const record of [...sentinelObservers]) { + if (record.targets.length === 0) continue; + const entries = record.targets.map((target) => ({ + target, + isIntersecting: true, + })) as unknown as IntersectionObserverEntry[]; + record.callback(entries, {} as IntersectionObserver); + } +} + +const renderedRows = () => screen.queryAllByTestId("document-image").length; + +describe("document rail figure windowing", () => { + it("mounts only the first window of rows for a long figure list", () => { + render(); + + expect(renderedRows()).toBe(6); + expect(screen.getByTestId("document-image-reveal")).toBeTruthy(); + }); + + it("mounts a short list whole, with no sentinel and no reveal control", () => { + // The overwhelming majority of indexed documents are this case, so the + // window must not add chrome to them. + render(); + + expect(renderedRows()).toBe(4); + expect(screen.queryByTestId("document-image-reveal")).toBeNull(); + }); + + it("grows by one window each time the sentinel is reached", () => { + render(); + + act(() => reachSentinel()); + expect(renderedRows()).toBe(12); + + act(() => reachSentinel()); + expect(renderedRows()).toBe(18); + }); + + it("reveals the whole list from its control, for readers who never scroll it", () => { + // Scroll-driven growth is unreachable by keyboard and by a screen reader + // walking the rail, so the control is the real affordance rather than a + // no-IntersectionObserver fallback. + render(); + + const reveal = screen.getByRole("button", { name: /show the remaining 34/i }); + fireEvent.click(reveal); + + expect(renderedRows()).toBe(40); + expect(screen.queryByTestId("document-image-reveal")).toBeNull(); + }); + + it("clamps the window when the list shrinks underneath an expanded reader", () => { + // Navigating to another document, or a reindex, can replace the array while + // the reader has already expanded past the new length. + const { rerender } = render(); + fireEvent.click(screen.getByRole("button", { name: /show the remaining 34/i })); + expect(renderedRows()).toBe(40); + + rerender(); + + expect(renderedRows()).toBe(3); + expect(screen.queryByTestId("document-image-reveal")).toBeNull(); + }); + + it("renders nothing at all for an empty list", () => { + render(); + + expect(renderedRows()).toBe(0); + expect(screen.queryByTestId("document-image-reveal")).toBeNull(); + }); +}); From b42f5e43e8eb13e618886074edb00d015ce80480 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 03:00:06 +0000 Subject: [PATCH 04/14] perf(viewer): give deferred rail figures a lower fetch and decode priority MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two levers, both about the same thing: a secondary figure rail should not contend with whatever the reader actually opened. SignedImage now sets fetchPriority explicitly — high when the caller marked the figure above-the-fold, low otherwise. next/image already emits decoding="async", which governs when a decode blocks; fetch priority governs whether the image competes for that budget at all, and it was the missing half of the pair. The document rail passes a 240px IntersectionObserver root margin instead of the shared 640px default. The wide default suits a surface whose images are the point of the page; the rail's are not, and at 640px it minted signed URLs for rows most of a viewport away, which land while the reader is looking at something else. There is no cross-surface request scheduler, so this margin differential is the ordering: surfaces on the wide default resolve first. The 100-id batch signed-URL route stays unwired, deliberately. Beyond keeping a privileged owner-scoped API route out of a component-only diff, the case for it has actually weakened: windowing the rail to six rows means a figure-heavy document no longer mounts N rows at once, which was the many-distinct-images scenario the batch was meant to serve. Recorded on #283 with the measurement that should decide it, rather than left as a standing assumption. use-signed-image-url.ts is untouched. Its identity-in-the-dedupe-key and cache-write-outside-the-shared-promise fixes were confirmed green before and after (tests/auth-signed-url-cache.dom.test.tsx, 6 passed). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7 --- docs/outstanding-issues.md | 2 +- .../clinical-dashboard/signed-image.tsx | 17 ++++++- .../document-viewer/source-panels.tsx | 14 ++++++ tests/signed-image.dom.test.tsx | 46 +++++++++++++++++++ 4 files changed, 77 insertions(+), 2 deletions(-) diff --git a/docs/outstanding-issues.md b/docs/outstanding-issues.md index e215a6b527..91e08407c2 100644 --- a/docs/outstanding-issues.md +++ b/docs/outstanding-issues.md @@ -324,7 +324,7 @@ removed after current-main verification; it is not missing recommended work. | #280 | P2 | task | Physical iPhone acceptance is owed for the viewer pinch gesture and the canvas pixel budget | **Outcome:** the two phone-only viewer fixes are confirmed on the device class they were written for. **Detail:** the viewer-optimisation branch revives pinch-to-zoom in fit mode (it was gated off in the default state, so a pinch reached neither the viewer nor the browser) and adds a canvas pixel budget so WebKit stops blanking the page above roughly 2.3x zoom on a dpr-3 display. Neither is verifiable in this container (see the Chromium/pdf.js row) and neither is a Chromium behaviour anyway — the canvas ceiling is a WebKit limit and the touch-action contention is a Safari gesture question. **Next:** on a real iPhone, in Safari and in the installed PWA: pinch a freshly opened document and confirm it zooms without first tapping a control; zoom to maximum and confirm the page stays painted rather than going blank; confirm a pinch that drifts vertically is not cancelled mid-gesture by the holder's 'touch-action: pan-y' (the mitigation if it is, is switching touch-action to none while two pointers are down — the gesture hook already tracks pointer count and exposes 'pinching'). Record the result against docs/phone-chrome-physical-acceptance.md. **Stop:** do not re-gate pinch on '!fitWidth' to resolve a gesture-contention finding — that restores the original defect. | session 2026-08-08 document-viewer optimisation; docs/design-system/COMPONENTS.md phone clause | 2026-08-08 | | #281 | P2 | rec | The phone document route renders two clinical-summary surfaces and neither is canonical | **Outcome:** one clinical summary on the document route, chosen deliberately. **Detail:** a phone reader gets the gradient 'High-yield clinical summary' card (DocumentClinicalSummary, built by buildDocumentClinicalSummaryModel) and, further down, the rail's '#source-summary' / 'high-yield-summary' disclosure (DocumentSectionSummary + FormattedHighYieldSummary + BadgeCluster). They render the same document.summary row two different ways. The rail is not hidden on phones — only its DocumentSectionIndexCard is lg:block — so both appear. Only the rail panel carries the section anchor, so the more prominent card is the unnavigable one. Note the two disagree about emptiness as well: the card now renders nothing when the model yields no usable text, while the rail panel still renders for its label badges, which is why 'hasStoredSummary' was deliberately left keyed to the stored row rather than to card content. **Next:** decide which rendering is canonical — this is a clinical-content judgement about how a summary should read, not a layout fix — then delete the other and give the survivor the 'source-summary' anchor. If the rail's badges are the part worth keeping, they can move without the second summary body. **Stop:** do not merge the two renderings mechanically; they format clinical text differently and the difference is the decision. | session 2026-08-08 document-viewer optimisation; document-rail-panels.tsx; document-clinical-summary.tsx | 2026-08-08 | | #282 | P3 | task | Probe the corpus for JBIG2/JPX before deciding whether pdf.js needs its decoder assets shipped | **Outcome:** a measured decision about pdf.js's cMap/standard-font/WASM assets rather than an assumption either way. **Detail:** getDocument is configured with url plus the on-demand fetch flags and nothing else, so 'wasmUrl', 'standardFontDataUrl', 'cMapUrl' and 'iccUrl' are all unset. pdfjs-dist ships those assets (wasm 1.5 MB, standard_fonts 804 KB, cmaps 1.7 MB) and nothing copies them into public/. With wasmUrl null, 'useWorkerFetch' resolves false and the WASM image decoders cannot load, so JBIG2 and JPEG2000 images fall back to the JS decoders or fail; those are exactly the encodings a scanned guideline uses, and this repo runs an OCR pipeline, which implies scanned sources exist. Non-embedded standard-14 fonts fall back to system fonts, which is a fidelity risk on a clinical document rather than a failure. **Next:** sample the real corpus for JBIG2/JPX-encoded images and for PDFs relying on the standard 14 before shipping ~2 MB of static assets; if the corpus does use them, copy into public/pdfjs, set the URLs, and add immutable cache headers in next.config.ts (public/ is not counted by check:bundle-budget, so there is no budget risk — the cost is bytes over the wire on first use). **Stop:** do not ship the assets on the assumption alone. | session 2026-08-08 document-viewer optimisation; node_modules/pdfjs-dist/types/src/display/api.d.ts | 2026-08-08 | -| #283 | P3 | rec | The 100-id batch signed-URL route still has no caller | **Outcome:** either the batch minter is used or it is retired, rather than sitting as an untested, unreachable privileged surface. **Detail:** src/app/api/images/signed-urls/route.ts POSTs up to 100 image ids and returns their signed URLs, with its own rate limit, owner scoping and committed-generation filter. Nothing in src/ calls it — only tests/private-access-routes.test.ts imports it. The viewer resolves images one at a time through use-signed-image-url.ts. The 2026-08-08 pass added in-flight deduplication there, which removes the duplicate-consumer case (a figure and its lightbox racing for the same asset) but not the many-distinct-images case: a page of N figures is still N round trips where one batch call would do. **Next:** decide deliberately — wire the rail/filmstrip to the batch route when a page mounts several distinct images at once, or delete the route and its tests. The cost of leaving it is a privileged endpoint no product code exercises. **Stop:** if wiring it, keep the per-image endpoint for the lightbox's retry path; do not make the batch the only way to mint a URL. | session 2026-08-08 document-viewer optimisation; src/app/api/images/signed-urls/route.ts | 2026-08-08 | +| #283 | P3 | rec | The 100-id batch signed-URL route still has no caller | **Outcome:** either the batch minter is used or it is retired, rather than sitting as an untested, unreachable privileged surface. **Detail:** src/app/api/images/signed-urls/route.ts POSTs up to 100 image ids and returns their signed URLs, with its own rate limit, owner scoping and committed-generation filter. Nothing in src/ calls it — only tests/private-access-routes.test.ts imports it. **DEFERRED AGAIN, DELIBERATELY, 2026-08-09 (document viewer Phase 3, Task 3).** The user chose deferral over wiring when asked. Two reasons beyond cost: (a) wiring it puts a privileged owner-scoped API route into a diff that is otherwise confined to src/components/document-viewer/**, and it matches clinicalRiskPatterns (/^src\/app\/api\//) so pr-policy hard-blocks the merge without a complete Clinical Governance Preflight; (b) Phase 3 Task 2 windowed the rail to six rows and tightened its IntersectionObserver root margin from 640px to 240px, so the many-distinct-images case the batch route was meant to serve is now materially smaller — a page of N figures no longer mounts N rows at once. The batching win should be re-measured against the windowed rail before it is wired at all, rather than assumed from the pre-window numbers. **Next:** decide deliberately — measure concurrent distinct-image requests on a figure-heavy document with the windowed rail, then either wire the batch route in its own PR or delete it and its tests. **Stop:** if wiring it, keep the per-image endpoint for the lightbox's retry path; do not make the batch the only way to mint a URL. | session 2026-08-08 document-viewer optimisation; src/app/api/images/signed-urls/route.ts | 2026-08-08 | | #284 | P3 | issue | tests/pr-handoff-stop.test.ts fails whenever the suite runs as root | **Outcome:** 'npm run test' is green in a root container, so a real failure is not hidden behind a known one. **Detail:** 'pr-handoff-stop hook > emits handoff context only when the marker file exists' expects markerExists('sess-readonly') to be false — it makes the marker directory read-only and asserts the hook could not write there. Root ignores the permission bits, so the write succeeds and the assertion fails. Reproduced on an unmodified bc33d41 checkout as well as on the viewer-optimisation branch, so it is environment-dependent, not a regression. Cost is that every full-suite run in a root container reports '1 failed', which trains readers to skim past the failure count. **Next:** skip the case when 'process.getuid?.() === 0' with an explicit reason, or drop privileges for that assertion. **Stop:** do not delete the coverage — the read-only case is the point of the test on a normal user account. | session 2026-08-08 full-suite runs; reproduced on bc33d41 | 2026-08-08 | | #285 | P2 | issue | Fresh remote/Cloud containers cannot run npm ci — shipped Node 24.13.0 is below the ^24.15.0 floor that main's jsdom@30.0.1 now requires | Observed 2026-08-08 in a Claude Code web container while syncing PR #1730. npm ci --include=dev aborts with EBADENGINE on jsdom@30.0.1 (needs Node ^22.22.2, ^24.15.0 or >=26); the container ships v24.13.0, so node_modules stays stale and the pre-push static guard then fails typecheck on the missing tailwind-merge added by #1678. Worked around by nvm install 24.19.0 plus a PATH prefix (nvm use alone does not stick — system node shadows it). Next action: raise the engines.node floor in package.json to >=24.15 so the mismatch fails loudly at the declared contract, and provision a compatible Node in the remote/Cloud setup path so a fresh container is not blocked at first install. | session 2026-08-08 (PR #1730) | 2026-08-08 | | #286 | P2 | task | PR 2 of the in-page nav series - convert the six pill-rail information pages onto InPageNavHeader | **Outcome:** the six routes still drawing the shell-owned pill rail use the documented default in-page navigation template, and the rail behind them is deleted. **Detail:** PR 1 landed as 2806d5e (#1740), extracting InPageNavHeader plus PageSection/toDocumentSections and usePageSectionWeights into src/components/in-page-nav/, and converting differential-detail-page.tsx as proof. Remaining routes: /services/[slug], /forms/[slug], /specifiers/[slug] (record and reference), /formulation/[slug], /dsm/diagnoses/[slug], /dsm/diagnoses/[slug]/differentials. Settled with the owner: replace the rail at every width, drop the InformationPageBreadcrumbs row but keep the large in-body h1, move each record's actions into the ellipsis sheet. **Next:** FIRST widen InPageNavHeader actions from the render prop (close) => ReactNode to ReactNode or ((close) => ReactNode) and close both sheets on pathname change - four of the seven components in scope are Server Components (specifier-record-page, specifier-reference-page, formulation-mechanism-page, dsm-diagnosis-page carry no use client) and React refuses to pass a function across that boundary, so all four fail to build against the current signature. Then add use-resolved-page-sections and use-in-page-section-nav, generalise useDocumentChromeMetrics for an --inpage-anchor-offset, add scroll-mt to every anchor (information-page sections carry none today), convert each page, then delete informationPageSectionDefinitions and the section kind in secondary-navigation.tsx. Two DSM routes declare anchors nothing renders - see #256. Full brief: artifact 77de9f5b-d195-409d-8156-2e4b41cd2f45. **Stop:** do not convert DocumentViewer in this PR (its chrome-metric property names are contract-pinned by tests/header-scroll-hide-contract.test.ts:110-113), and do not verify anchors by grepping for id= - assert against the rendered DOM per route. | session 2026-08-08; PR #1740 (2806d5e) | 2026-08-08 | diff --git a/src/components/clinical-dashboard/signed-image.tsx b/src/components/clinical-dashboard/signed-image.tsx index 47c7ded5af..a19920c37d 100644 --- a/src/components/clinical-dashboard/signed-image.tsx +++ b/src/components/clinical-dashboard/signed-image.tsx @@ -46,7 +46,15 @@ export const SignedImage = memo(function SignedImage({ failureLabel?: string; /** Label for the retry button in the failed state. */ retryLabel?: string; - /** IntersectionObserver root margin that gates the network request. */ + /** + * IntersectionObserver root margin that gates the network request. + * + * The default is a wide lookahead, which suits a surface whose images are the + * point of the page. A long secondary list should pass something tighter: the + * margin is what decides how many signed URLs are in flight at once, and with + * no cross-surface scheduler it is also what decides which surface's requests + * go first. + */ rootMargin?: string; /** When true, the loaded image is clickable and opens a fullscreen lightbox. */ zoomable?: boolean; @@ -173,6 +181,13 @@ export const SignedImage = memo(function SignedImage({ sizes="(max-width: 768px) 92vw, 320px" unoptimized priority={priority} + // Decode priority, not just fetch order. An above-the-fold evidence + // figure competes for the main thread with the page it is part of; + // a deferred rail crop does not need to, and saying so explicitly is + // what keeps a long figure rail from contending with whatever the + // reader is actually looking at. next/image already emits + // `decoding="async"`, so this is the missing half of that pair. + fetchPriority={priority ? "high" : "low"} onLoad={() => setLoaded(true)} onError={handleImageError} className={cn( diff --git a/src/components/document-viewer/source-panels.tsx b/src/components/document-viewer/source-panels.tsx index c8deaf7414..1c922c8042 100644 --- a/src/components/document-viewer/source-panels.tsx +++ b/src/components/document-viewer/source-panels.tsx @@ -303,6 +303,14 @@ export function DocumentImage({ failureLabel="Image preview failed." retryLabel="Retry" expandLabel={isWideCrop ? "Open full screen" : undefined} + // Tighter than the shared default. The rail is secondary evidence beside + // (desktop) or below (phone) the source the reader opened, and the wide + // default lookahead minted signed URLs for rows most of a viewport away + // — competing with the page's own above-the-fold work for no benefit, + // since those rows are not on screen when the requests land. There is no + // cross-surface request scheduler, so this differential IS the ordering: + // surfaces that keep the wide margin resolve first. + rootMargin={RAIL_IMAGE_ROOT_MARGIN} // No `min-h-*` here. `aspect-ratio` transfers size constraints across // axes, so a min-height of 10rem on a 3:1 table crop became a *minimum // width* of 480px and blew the phone card past the viewport. The frame @@ -438,6 +446,12 @@ export function DocumentImage({ */ const RAIL_IMAGE_WINDOW = 6; +/** + * Lookahead for the rail's own signed-URL requests, against `SignedImage`'s + * wider default. See the call site in `DocumentImage` for why they differ. + */ +const RAIL_IMAGE_ROOT_MARGIN = "240px 0px"; + /** * The figure rail, windowed. * diff --git a/tests/signed-image.dom.test.tsx b/tests/signed-image.dom.test.tsx index c6dfd3f7b1..2e91dfac10 100644 --- a/tests/signed-image.dom.test.tsx +++ b/tests/signed-image.dom.test.tsx @@ -115,6 +115,52 @@ describe("SignedImage failure/retry (jsdom)", () => { expect(fetchMock).toHaveBeenCalled(); const src = img.getAttribute("src") ?? ""; expect(src.endsWith("/demo/hero.png")).toBe(true); + // Above-the-fold evidence competes with the page it is part of, so it says + // so — the deferred case below says the opposite. + expect(img).toHaveAttribute("fetchpriority", "high"); + }); + + it("tells the browser a deferred figure may decode last", async () => { + // `decoding="async"` (next/image's default) governs when the decode blocks; + // fetch priority governs whether this image contends with the page's own + // above-the-fold work at all. A long secondary figure rail should not. + vi.stubGlobal( + "fetch", + vi.fn().mockResolvedValue({ ok: true, status: 200, json: async () => ({ url: "/demo/rail.png" }) }), + ); + + render(); + + const img = await screen.findByRole("img", { name: "Rail crop" }); + expect(img).toHaveAttribute("fetchpriority", "low"); + expect(img).toHaveAttribute("decoding", "async"); + }); + + it("defers on the root margin its caller asked for, not only the shared default", async () => { + // The rail passes a tighter margin than the default. With no cross-surface + // request scheduler, that differential is what decides which surface's + // signed URLs resolve first, so a dropped prop is a real regression rather + // than a cosmetic one. + const observed: IntersectionObserverInit[] = []; + vi.stubGlobal( + "IntersectionObserver", + class { + constructor(_callback: IntersectionObserverCallback, options: IntersectionObserverInit) { + observed.push(options); + } + observe() {} + unobserve() {} + disconnect() {} + takeRecords() { + return []; + } + }, + ); + vi.stubGlobal("fetch", vi.fn()); + + render(); + + expect(observed.at(0)?.rootMargin).toBe("240px 0px"); }); it("uses a provided source aspect ratio instead of forcing every document crop into 4:3", async () => { From 516ec8606e79657c1129139ab265dac7cadf4e85 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 03:02:20 +0000 Subject: [PATCH 05/14] feat(viewer): complete the PDF reader's keyboard reading mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The holder handled arrow keys, +/-, and 0. Phase 3 adds Page Up / Page Down, Home / End, F for fit-to-width, and R for rotate. Rotation needed a route back out. `rotation` arrives as a controlled prop with no callback, so the keyboard could reach every viewing control except that one. Rather than give the viewer its own rotation state — a second source of truth for a single toolbar button — R calls the same `handlePdfRotate` that DocumentFrame's rotate control already calls, threaded down as `onRotate`. When no handler is supplied, R stays inert rather than swallowed: the event is not preventDefault'ed, so it still reaches whatever else wants it. Modified keystrokes are now explicitly ignored. Ctrl/Cmd+0 is the browser's own zoom reset and Cmd+Left is history back on macOS; a reader that lost either to the viewer would be worse off than one with no bindings at all. The holder's aria-label names the bindings, so a screen-reader user hears them on focus instead of having to discover them. Contract documented in docs/wiring-conventions.md and covered by tests/document-viewer-keyboard.dom.test.tsx, including the two rules that are easy to regress silently: only keystrokes aimed at the holder itself are handled, and rotation goes through the frame's callback. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7 --- docs/wiring-conventions.md | 30 +++ src/components/DocumentViewer.tsx | 4 + .../document-viewer/pdf-canvas-viewer.tsx | 47 +++- tests/document-viewer-keyboard.dom.test.tsx | 214 ++++++++++++++++++ 4 files changed, 294 insertions(+), 1 deletion(-) create mode 100644 tests/document-viewer-keyboard.dom.test.tsx diff --git a/docs/wiring-conventions.md b/docs/wiring-conventions.md index 27d6d9218b..c7a7c0a487 100644 --- a/docs/wiring-conventions.md +++ b/docs/wiring-conventions.md @@ -71,6 +71,36 @@ and is exactly what the ESLint gate below now catches. 4. Document the route in `docs/codebase-index.md` (the product-pages table). 5. The reachability gate then enforces step 2 automatically (see below). +## Keyboard wiring: the PDF reader + +The document viewer's page holder (`src/components/document-viewer/pdf-canvas-viewer.tsx`, +`data-testid="pdf-canvas-scroll"`) is focusable and carries the reading-mode bindings. Every one of +them is a keyboard route to a control that also exists in the `DocumentFrame` toolbar — the keyboard +is a second way to reach the same actions, never a hidden feature with no visible equivalent. + +| Key | Action | +| ------------------- | -------------------- | +| Left / Right arrow | Previous / next page | +| Page Up / Page Down | Previous / next page | +| Home / End | First / last page | +| `+` / `=` and `-` | Zoom in / out | +| `0` or `F` | Fit to width | +| `R` | Rotate 90 degrees | + +Three rules hold this together and are covered by `tests/document-viewer-keyboard.dom.test.tsx`: + +- **Only keystrokes aimed at the holder itself are handled** (`event.target !== event.currentTarget` + returns early), so Enter or typing inside a child control — the retry button, the source links — is + never hijacked. +- **Modified keystrokes are never intercepted.** Ctrl/Cmd+`0` is the browser's own zoom reset and + Cmd+Left is history back on macOS; a viewer that ate those would be worse than one with no bindings. +- **Rotation is asked for, not owned.** `rotation` reaches the viewer as a controlled prop, so `R` + calls the `onRotate` callback that `DocumentFrame`'s rotate button already calls. Do not give the + viewer its own rotation state — that would be a second source of truth for one toolbar control. + +The holder's `aria-label` names the bindings, so a screen-reader user hears them on focus rather than +having to discover them. + ## Mockups are exempt Design-scratch mockups — `src/app/mockups/**` (404 in production), the `*-mockups/` component diff --git a/src/components/DocumentViewer.tsx b/src/components/DocumentViewer.tsx index 57c2989cc2..a0aca346d6 100644 --- a/src/components/DocumentViewer.tsx +++ b/src/components/DocumentViewer.tsx @@ -1493,6 +1493,10 @@ export function DocumentViewer({ fullscreen={pdfFullscreen} onFitWidthChange={handlePdfFitWidthChange} onZoomChange={handlePdfZoomChange} + // The same handler DocumentFrame's rotate control uses, so + // the keyboard reaches rotation without the viewer owning a + // second copy of that state. + onRotate={handlePdfRotate} /> ) : ( void; onZoomChange: (zoom: number) => void; + /** + * DocumentFrame's rotate action. `rotation` arrives as a controlled prop with + * no way back, so without this the keyboard could reach every viewing control + * except rotation. The viewer does not own rotation state — it asks the frame + * that does, which keeps one toolbar and one source of truth. + */ + onRotate?: () => void; }) { const holderRef = useRef(null); const [pdf, setPdf] = useState(null); @@ -857,21 +865,48 @@ export const PdfCanvasViewer = memo(function PdfCanvasViewer({ onPanBy: handlePanByDelta, }); + /** + * Reading-mode key bindings. Documented in `docs/wiring-conventions.md`. + * + * | Key | Action | + * | -------------------- | --------------------- | + * | Left / Right arrow | Previous / next page | + * | Page Up / Page Down | Previous / next page | + * | Home / End | First / last page | + * | `+` / `=` and `-` | Zoom in / out | + * | `0` or `f` | Fit to width | + * | `r` | Rotate 90 degrees | + * + * Modified keystrokes are left alone throughout: Ctrl/Cmd+`0` is the browser's + * own zoom reset, Cmd+Left is history back on macOS, and a viewer that ate + * those would be worse than one with no bindings at all. + */ function handleHolderKeyDown(event: ReactKeyboardEvent) { if (!pagesReady) return; // Only act on keystrokes aimed at the holder itself, so Enter/typing inside // child controls (retry button, source links) is never hijacked. if (event.target !== event.currentTarget) return; + if (event.ctrlKey || event.metaKey || event.altKey) return; switch (event.key) { case "ArrowLeft": + case "PageUp": event.preventDefault(); goToPage(page - 1); break; case "ArrowRight": + case "PageDown": event.preventDefault(); goToPage(page + 1); break; + case "Home": + event.preventDefault(); + goToPage(1); + break; + case "End": + event.preventDefault(); + goToPage(totalPages); + break; case "+": case "=": event.preventDefault(); @@ -882,9 +917,19 @@ export const PdfCanvasViewer = memo(function PdfCanvasViewer({ zoomBy(-VIEWER_ZOOM_STEP); break; case "0": + case "f": + case "F": event.preventDefault(); setFitWidth(true); break; + case "r": + case "R": + // Silently inert without the callback rather than swallowing the key: + // an unhandled `r` still reaches whatever else wants it. + if (!onRotate) break; + event.preventDefault(); + onRotate(); + break; default: break; } @@ -901,7 +946,7 @@ export const PdfCanvasViewer = memo(function PdfCanvasViewer({ ref={holderRef} tabIndex={0} role="group" - aria-label={`${title} — page view. Use arrow keys to change pages, plus and minus to zoom.`} + aria-label={`${title} — page view. Arrow keys or Page Up and Page Down change pages, Home and End jump to the first or last page, plus and minus zoom, F fits the width, R rotates.`} onKeyDown={handleHolderKeyDown} {...gestureHandlers} className={cn( diff --git a/tests/document-viewer-keyboard.dom.test.tsx b/tests/document-viewer-keyboard.dom.test.tsx new file mode 100644 index 0000000000..67333bc54f --- /dev/null +++ b/tests/document-viewer-keyboard.dom.test.tsx @@ -0,0 +1,214 @@ +import { act, cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +/** + * Reading-mode key bindings for the PDF viewer. + * + * Contract in `docs/wiring-conventions.md` § "Keyboard wiring: the PDF reader". + * Every binding is a second route to a control that also exists in the + * DocumentFrame toolbar, so these tests assert the callbacks the toolbar uses + * are the ones the keyboard reaches — not that the viewer grew private state. + */ + +const PAGE_COUNT = 12; + +vi.mock("pdfjs-dist", () => ({ + GlobalWorkerOptions: { workerSrc: "" }, + getDocument: () => ({ + promise: Promise.resolve({ + numPages: PAGE_COUNT, + getPage: () => + Promise.resolve({ + getViewport: ({ scale = 1 }: { scale?: number }) => ({ width: 595 * scale, height: 842 * scale }), + render: () => ({ promise: Promise.resolve(), cancel: () => {} }), + cleanup: () => {}, + }), + }), + destroy: () => Promise.resolve(), + }), +})); + +beforeEach(() => { + vi.stubGlobal( + "IntersectionObserver", + class { + observe() {} + unobserve() {} + disconnect() {} + takeRecords() { + return []; + } + }, + ); + vi.stubGlobal( + "ResizeObserver", + class { + observe() {} + unobserve() {} + disconnect() {} + }, + ); + vi.spyOn(HTMLCanvasElement.prototype, "getContext").mockImplementation( + () => ({ setTransform: () => {}, imageSmoothingEnabled: false }) as unknown as CanvasRenderingContext2D, + ); + vi.stubGlobal("requestIdleCallback", () => 1); + vi.stubGlobal("cancelIdleCallback", () => {}); +}); + +afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +async function renderReader(overrides: Record = {}) { + const { PdfCanvasViewer } = await import("@/components/document-viewer/pdf-canvas-viewer"); + const handlers = { + onPageChange: vi.fn(), + onFitWidthChange: vi.fn(), + onZoomChange: vi.fn(), + onRotate: vi.fn(), + }; + + await act(async () => { + render( + , + ); + }); + + const holder = await screen.findByTestId("pdf-canvas-scroll"); + await waitFor(() => expect(screen.getAllByTestId("pdf-page-slot").length).toBe(PAGE_COUNT)); + return { holder, handlers }; +} + +describe("PDF reader keyboard bindings", () => { + it("changes pages with both the arrow keys and Page Up / Page Down", async () => { + const { holder, handlers } = await renderReader(); + + fireEvent.keyDown(holder, { key: "PageDown" }); + expect(handlers.onPageChange).toHaveBeenLastCalledWith(6); + + fireEvent.keyDown(holder, { key: "PageUp" }); + expect(handlers.onPageChange).toHaveBeenLastCalledWith(5); + + fireEvent.keyDown(holder, { key: "ArrowRight" }); + expect(handlers.onPageChange).toHaveBeenLastCalledWith(6); + + fireEvent.keyDown(holder, { key: "ArrowLeft" }); + expect(handlers.onPageChange).toHaveBeenLastCalledWith(5); + }); + + it("jumps to the first and last page with Home and End", async () => { + const { holder, handlers } = await renderReader(); + + fireEvent.keyDown(holder, { key: "Home" }); + expect(handlers.onPageChange).toHaveBeenLastCalledWith(1); + + fireEvent.keyDown(holder, { key: "End" }); + expect(handlers.onPageChange).toHaveBeenLastCalledWith(PAGE_COUNT); + }); + + it("never runs past either end of the document", async () => { + const { holder, handlers } = await renderReader({ initialPage: 1 }); + + fireEvent.keyDown(holder, { key: "PageUp" }); + fireEvent.keyDown(holder, { key: "ArrowLeft" }); + expect(handlers.onPageChange).not.toHaveBeenCalled(); + + fireEvent.keyDown(holder, { key: "End" }); + fireEvent.keyDown(holder, { key: "PageDown" }); + expect(handlers.onPageChange).toHaveBeenLastCalledWith(PAGE_COUNT); + }); + + it("fits the width from both 0 and F", async () => { + const { holder, handlers } = await renderReader(); + + fireEvent.keyDown(holder, { key: "0" }); + expect(handlers.onFitWidthChange).toHaveBeenLastCalledWith(true); + + handlers.onFitWidthChange.mockClear(); + fireEvent.keyDown(holder, { key: "f" }); + expect(handlers.onFitWidthChange).toHaveBeenLastCalledWith(true); + + handlers.onFitWidthChange.mockClear(); + fireEvent.keyDown(holder, { key: "F" }); + expect(handlers.onFitWidthChange).toHaveBeenLastCalledWith(true); + }); + + it("rotates through the frame's own callback rather than private viewer state", async () => { + const { holder, handlers } = await renderReader(); + + fireEvent.keyDown(holder, { key: "r" }); + fireEvent.keyDown(holder, { key: "R" }); + + // Rotation stays owned by DocumentFrame — one toolbar, one source of truth. + expect(handlers.onRotate).toHaveBeenCalledTimes(2); + }); + + it("leaves R inert, not swallowed, when no rotate handler was supplied", async () => { + const { holder } = await renderReader({ onRotate: undefined }); + + const event = new KeyboardEvent("keydown", { key: "r", bubbles: true, cancelable: true }); + holder.dispatchEvent(event); + + expect(event.defaultPrevented).toBe(false); + }); + + it("zooms with plus and minus", async () => { + const { holder, handlers } = await renderReader(); + + fireEvent.keyDown(holder, { key: "=" }); + const zoomedIn = handlers.onZoomChange.mock.calls.at(-1)?.[0] as number; + expect(zoomedIn).toBeGreaterThan(1); + + fireEvent.keyDown(holder, { key: "-" }); + const zoomedOut = handlers.onZoomChange.mock.calls.at(-1)?.[0] as number; + expect(zoomedOut).toBeLessThan(zoomedIn); + }); + + it("never intercepts a modified keystroke", async () => { + const { holder, handlers } = await renderReader(); + + // Ctrl/Cmd+0 is the browser's own zoom reset and Cmd+Left is history back on + // macOS. A reader that lost either to the viewer is worse off than one with + // no bindings at all. + fireEvent.keyDown(holder, { key: "0", ctrlKey: true }); + fireEvent.keyDown(holder, { key: "0", metaKey: true }); + fireEvent.keyDown(holder, { key: "ArrowLeft", metaKey: true }); + fireEvent.keyDown(holder, { key: "r", ctrlKey: true }); + + expect(handlers.onFitWidthChange).not.toHaveBeenCalled(); + expect(handlers.onPageChange).not.toHaveBeenCalled(); + expect(handlers.onRotate).not.toHaveBeenCalled(); + }); + + it("ignores keystrokes aimed at a control inside the holder", async () => { + const { holder, handlers } = await renderReader(); + + // Typing in a child (the retry button, a source link) must not be hijacked. + const child = holder.querySelector("[data-testid='pdf-page-slot']"); + expect(child).toBeTruthy(); + fireEvent.keyDown(child!, { key: "PageDown", bubbles: true }); + + expect(handlers.onPageChange).not.toHaveBeenCalled(); + }); + + it("names its bindings in the holder's accessible label", async () => { + const { holder } = await renderReader(); + + const label = holder.getAttribute("aria-label") ?? ""; + expect(label).toMatch(/Page Up and Page Down/); + expect(label).toMatch(/Home and End/); + expect(label).toMatch(/F fits the width/); + expect(label).toMatch(/R rotates/); + }); +}); From 388e7d53a453aeb68acc3abc3da87874227f6236 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 03:03:32 +0000 Subject: [PATCH 06/14] docs(viewer): record the Phase 3 outcome and the OffscreenCanvas decision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3's table said what to build; it now says what landed, including the two items that deliberately did not. Crop -> page overlay stays out: bbox is already SELECTed in document-detail.ts but absent from DocumentDetailImage, so it is a contract change across src/lib/**document** rather than a viewer change, and it is called out as the one remaining Phase 3 capability with the shape of the work named. OffscreenCanvas is not implemented, which is the plan's own instruction rather than a shortcut — it conditions the work on "measured main-thread paint cost", and no such measurement existed. Two things changed that. Virtualization keeps the reader's page and a neighbour already rastered, so the cold-render-per-flip cost that motivated a worker raster is largely gone before any threading work starts; and the new canvas gate now attaches the number (flip-to-painted, long task count and duration, backing pixels) on every Production UI run. #290 records how to read it and what result would close the question either way. Nothing about this could be measured locally: pdfjs-dist@6 needs Map.prototype.getOrInsertComputed, which this container's Chromium 141 lacks and Node 24.13.0 lacks too, so no browser and no headless harness here can raster a page at all. Toolbar density is struck from the table — it shipped in Phase 2. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7 --- docs/outstanding-issues.md | 3 +- docs/plans/document-viewer-redesign-plan.md | 43 ++++++++++++--------- 2 files changed, 27 insertions(+), 19 deletions(-) diff --git a/docs/outstanding-issues.md b/docs/outstanding-issues.md index 91e08407c2..9a7bc1fe93 100644 --- a/docs/outstanding-issues.md +++ b/docs/outstanding-issues.md @@ -171,7 +171,7 @@ removed after current-main verification; it is not missing recommended work. | 116 | `#289` | A2 | High — auth/identity | Next auth module touch | 1–2 hours | Export a named helper (e.g. `authorizationIdentity(headers)`) from the auth module and use it at every property-access call site; consider a lint rule or branded type so `.Authorization` stops type-checking at all. **Stop:** do not change `authorizationHeadersForAccessToken` to emit uppercase — lowercase is the correct Fetch/Headers convention and callers that pass the object wholesale to `fetch` depend on it. | - + ## Open items > **Merged-main canary update (2026-07-23, run `30018289898`):** the new structured report correctly recorded evaluated tree `c24f2e8f2d30d0c59fc1eba025d3dcd63478137e`, run/attempt identity and `cross-region-runner` latency context. Golden retrieval remained 36/36 with document/content recall 1.0 and no failed cases. The 44-case answer gate had grounded-supported and unsupported-correct rates of 1.0, but failed because `neuroleptic-side-effect-escalation` again returned one citation where two are required (citation-failure rate 0.0227). `admission-discharge-comparison` again omitted the specific AKG admission document after `comparison_source_extractive_fallback`; `admission-discharge-coverage-paraphrase` was advisory-only at 24,870 ms. Answer cost was reported as `$0.234736`. Do not retry immediately: retain this as the first structured datapoint, compare it with the scheduled 2026-07-26 report, and keep retrieval/ranking unchanged. @@ -331,6 +331,7 @@ removed after current-main verification; it is not missing recommended work. | #287 | P2 | task | PR 3 of the in-page nav series - the three locally-owned routes each need a decision, not just a conversion | **Outcome:** every information page uses the documented in-page navigation template, or has a recorded reason not to. **Detail:** the last three routes each own a different bespoke pattern, and none is a mechanical port. (1) /medications/[slug] - SectionTabs at medication-record-page.tsx:183 SWAPS CONTENT rather than scrolling: sectionsByTab[activeTab] at :391 filters record.sections by type, so a different set mounts per tab. The InPageNavHeader track is scroll-spy over anchors that all exist at once, so adopting it means either driving tab state from the track (the track stops meaning where am I on the page) or flattening to one scrolling page - a real behaviour change to a clinical record, and a product call. (2) /differentials/presentations/[slug] - MobileTabs at differential-presentation-workflow-page.tsx plus the xl review sidebar; the old `differentialPresentationSections` shell set is gone and the route is locally owned (`page-secondary-navigation.tsx`). Remaining work is the product decision to adopt `InPageNavHeader` (or keep the tab/sidebar model with a recorded reason), not resurrecting deleted section targetIds. (3) /factsheets/[slug] - the On this page list at factsheet-detail-page.tsx:365-371 is li text with no link, button or handler, and the sections themselves carry no ids at all (:214, :267, :313, :447, :453, :471); the tail is data-driven via factsheet.sections.map keyed on section.heading (:538), so anchor ids must be generated deterministically from headings and that generator becomes the contract the section list depends on. Therapy Compass is deliberately excluded from the whole series - ModeNav is a different multi-route pattern. **Next:** decide the medications tab model first (owner decision, blocks planning); decide whether presentations keep MobileTabs/sidebar or adopt InPageNavHeader; choose the factsheets heading-to-id scheme. Then convert. **Stop:** do not port medications mechanically - swapping the tablist for a scroll track silently changes what a clinician sees on a medication record. | session 2026-08-08; follows #286 | 2026-08-08 | | #288 | P3 | rec | Decide whether DocumentViewer adopts the template it was extracted from, or the partial adoption is recorded as final | **Outcome:** the in-page navigation template has one deliberate owner story rather than an unexplained gap. **Detail:** PR 1 (2806d5e, #1740) extracted the header from DocumentViewer.tsx and differential-detail-page.tsx, which held it near-verbatim twice, into src/components/in-page-nav/InPageNavHeader. differential-detail-page was converted onto it; DocumentViewer was deliberately NOT, because its own useDocumentSectionSpy and useDocumentChromeMetrics wiring and its CSS custom-property names (--document-anchor-offset, --document-sticky-header-height, [data-document-sticky-header]) are pinned verbatim by tests/header-scroll-hide-contract.test.ts:110-113. Once #286 and #287 land, the template is adopted on every information page EXCEPT DocumentViewer. `docs/search-chrome-behaviour.md` (Default in-page navigation template) already records that DocumentViewer keeps its own header copy because it owns the page h1, uses edge-glass-header, and is pinned by visual baselines — so the gap is documented, not overlooked. #286 generalises chrome metrics for information pages only; it does not close DocumentViewer convergence. **Next:** owner decision only — convert DocumentViewer later (leaving pinned `--document-*` property names untouched per tests/header-scroll-hide-contract.test.ts:110-113), or explicitly mark the documented non-adoption as the final end state in this ledger when the series closes. **Stop:** do not rename or repoint the pinned document CSS custom properties to unify them with the information-page ones - the contract test pins those exact strings and the document route is the highest-traffic surface in the app. | session 2026-08-08; PR #1740 | 2026-08-08 | | #289 | P2 | issue | authorizationHeader is lowercase, and reading .Authorization off it fails silently | **Outcome:** nobody keys identity off `authorizationHeader` by property again without hitting a gate or a comment first. **Detail:** `authorizationHeadersForAccessToken` returns `{ authorization: 'Bearer …' }` — lowercase, per the Fetch/Headers convention (`src/lib/supabase/client.tsx:82`). The value is typed `Record`, so reading `.Authorization` type-checks, returns undefined, and degrades to whatever fallback the caller wrote. On PR #1741 the same mistake was made twice in one session and both were identity-scoping code: the in-flight signed-URL dedupe key in `use-signed-image-url.ts` collapsed every user onto `endpoint+''`, so an account switch with a request in flight could hand user B user A's signed URL; and `detailRequestSignature` in `DocumentViewer.tsx` omitted the token it documented as being present (weaker in practice — `authStatus`/`initialDetailIdentityStale` and the render-time identity reset still moved on a real switch — but the stated defence was not the shipped one). Review caught both before merge and each site now reads `headers.authorization ?? headers.Authorization ?? ''`. Merged main has no other property reads: every remaining caller passes the header object wholesale to `fetch`, where casing is irrelevant. **Next:** cheapest first — export a named helper (e.g. `authorizationIdentity(headers)`) from the auth module and use it at both sites so there is one definition, then consider a lint rule or a narrower type (a branded `AuthorizationHeader` with a lowercase-only key) so `.Authorization` stops type-checking at all. **Stop:** do not 'fix' this by emitting uppercase from `authorizationHeadersForAccessToken` — lowercase is the correct convention and callers pass the object to `fetch`. | PR #1741 review (Codex signed-URL/cache finding); src/lib/supabase/client.tsx:82; session 2026-08-08 | 2026-08-08 | +| #290 | P3 | rec | OffscreenCanvas for the PDF raster is unjustified until the page-flip cost is read from CI | **Outcome:** the worker-raster question is settled by a number rather than left as a standing 'optional' item in the redesign plan. **Detail:** docs/plans/document-viewer-redesign-plan.md conditions OffscreenCanvas on 'measured main-thread paint cost'. Phase 3 (Task 5) did not implement it, deliberately: virtualization now keeps the reader's page and one neighbour already rastered, so the cold-render-per-flip cost that motivated a worker raster is largely gone before any threading work starts, and moving pdf.js rendering off the main thread would put the canvas the clinical source is drawn into behind a transfer boundary — a real risk on the one surface where a blank page is a clinical failure. **No number exists yet and none could be produced locally:** pdfjs-dist@6 needs Map.prototype.getOrInsertComputed, which this container's Chromium 141 lacks and Node 24.13.0 also lacks, so neither a browser nor a headless harness here can raster a page (see #279). **Next:** read the measurement the gate already captures. tests/ui-document-canvas.spec.ts attaches page-flip-raster-cost.json (flipToPaintedMs, longTaskCount, longTaskTotalMs, longestTaskMs, canvasBackingPixels) and logs a '[viewer-canvas] page flip painted in Nms' line, on every Production UI run and on any host with the pinned Chromium 151 build: npm ci --include=dev && npx playwright install chromium && npm run ensure && npm run test:e2e -- tests/ui-document-canvas.spec.ts --project=chromium. If the longest task during a flip is comfortably under ~50ms, close this as not-worth-doing and strike the row from the plan's Phase 3 table. **Stop:** do not implement OffscreenCanvas on principle because the plan lists it — the plan conditions it on the measurement, and the measurement is now cheap to obtain. | session 2026-08-09 document viewer Phase 3, Task 5; docs/plans/document-viewer-phase3-handover.md | 2026-08-09 | ## Resolved / archive diff --git a/docs/plans/document-viewer-redesign-plan.md b/docs/plans/document-viewer-redesign-plan.md index 9d3a50e31c..800726c8a3 100644 --- a/docs/plans/document-viewer-redesign-plan.md +++ b/docs/plans/document-viewer-redesign-plan.md @@ -1,11 +1,11 @@ # Document viewer redesign — PDF + photo surfaces -**Status:** programme plan (Phases 0–1 landed; Phase 2 detailed plan separate) +**Status:** programme plan (Phases 0–3 landed; only Phase 3's crop overlay and the optional Phase 4 polish remain) **Branch seed:** `cursor/document-viewer-redesign-plan-1db8` **Flightplan evidence:** `.local/workflow-evidence/2026-08-06T17-27-05-553Z-flightplan.json` **Related ledger:** `#214` / `#219` resolved; `#215` residual (PWA/demo WebP) optional -**Landed:** Phase 0 [#1660](https://github.com/BigSimmo/Database/pull/1660), Phase 1 [#1665](https://github.com/BigSimmo/Database/pull/1665) -**Next detail:** [`document-viewer-phase2-unified-chrome.md`](./document-viewer-phase2-unified-chrome.md) +**Landed:** Phase 0 [#1660](https://github.com/BigSimmo/Database/pull/1660), Phase 1 [#1665](https://github.com/BigSimmo/Database/pull/1665), Phase 2 [#1741](https://github.com/BigSimmo/Database/pull/1741), Phase 3 (this branch) +**Phase details:** [`document-viewer-phase2-unified-chrome.md`](./document-viewer-phase2-unified-chrome.md) · [`document-viewer-phase3-handover.md`](./document-viewer-phase3-handover.md) This is the execution plan for a dramatic improvement of design, style, approach, functionality, and optimisation of the PDF reader and photo/figure viewers. It is @@ -134,7 +134,7 @@ Clarify `document-viewer-lazy` naming vs real dynamic boundaries. **Verify:** shell/DOM contracts, pdf-reader-lazy, client-performance-boundaries, `document-detail-performance`, `verify:phone-chrome` (dry-run then focused). -### Phase 2 — Unified viewing chrome (design + approach) — **NEXT** +### Phase 2 — Unified viewing chrome (design + approach) — **DONE** (#1741) **Goal:** dramatic visual/UX convergence without changing clinical content. @@ -155,19 +155,24 @@ Clarify `document-viewer-lazy` naming vs real dynamic boundaries. **Verify:** per sub-PR focused contracts → `verify:pr-local`; phone-chrome only when sheet/composer touched. Update `docs/search-chrome-behaviour.md` only if ownership semantics change (prefer not to). -### Phase 3 — Functionality + optimisation +### Phase 3 — Functionality + optimisation — **DONE except crop overlay** **Goal:** make the viewer feel native and stay fast on long documents. -| Capability | Notes | -| ---------------------------------------- | ------------------------------------------------------------------------------ | -| Multi-page virtualization | Render near pages; dispose far canvases; preserve page URL sync | -| Crop → page overlay | When crop geometry exists, optional highlight on the PDF page | -| Keyboard reading mode | Page Up/Down, `+`/`-`, `f` fit, `r` rotate — documented, tested | -| Rail virtualization | Long `#source-images` lists | -| Smarter signed-URL / decode priority | Above-fold evidence vs below-fold rail | -| Toolbar density | Compact phone / expanded desktop; print chrome hidden via existing print hooks | -| Optional OffscreenCanvas / worker raster | Only after measured main-thread paint cost | +Execution brief: [`document-viewer-phase3-handover.md`](./document-viewer-phase3-handover.md). + +| Capability | Status | +| ---------------------------------------- | ------------------------------------------------------------------------------------------------------ | +| Multi-page virtualization | **Done** — windowed page column, document-wide live-canvas budget, idle ±1 render-ahead | +| Crop → page overlay | **Out of scope** — needs `bbox` through `DocumentDetailImage`; a wider contract change than this phase | +| Keyboard reading mode | **Done** — Page Up/Down, Home/End, `f` fit, `r` rotate; `docs/wiring-conventions.md` | +| Rail virtualization | **Done** — `DocumentImageList` windows `#source-images` and the audit list | +| Smarter signed-URL / decode priority | **Done** — explicit `fetchPriority`, tighter rail root margin; batch route still deferred (`#283`) | +| Toolbar density | ~~Phase 3~~ — already shipped in Phase 2 (`document-frame.tsx`, `hidden sm:inline` + overflow menu) | +| Optional OffscreenCanvas / worker raster | **Not implemented, by design** — the measurement it is conditioned on is now captured in CI (`#290`) | + +Phase 3 also closed `#279`: `tests/ui-document-canvas.spec.ts` is the first browser gate over the +viewer's raster, reading pixels back rather than trusting canvas dimensions. **Verify:** performance contract tests, bundle budget for document chunk, phone-chrome, then `verify:ui` at handoff. Lighthouse / live INP needs explicit approval if provider-backed @@ -296,10 +301,12 @@ Aligned with repo design rules and COMPONENTS §6: 1. ~~Land this plan doc.~~ Done (#1659). 2. ~~Execute **Phase 0**.~~ Done (#1660). 3. ~~**Phase 1** extraction.~~ Done (#1665). -4. Execute **Phase 2** via the detailed sub-PR plan - [`document-viewer-phase2-unified-chrome.md`](./document-viewer-phase2-unified-chrome.md) — - start with **PR 2a** (DocumentFrame controls + demote duplicate PDF zoom/fit). -5. Do not invent a second phone composer or change hide-reserve semantics. +4. ~~Execute **Phase 2**~~ Done (#1741, with Phase 0–2 squashed as `42f87ca`). +5. ~~Execute **Phase 3**~~ Done, except crop → page overlay — see the Phase 3 table above. +6. Crop → page overlay is the one remaining Phase 3 capability. It needs `bbox` plumbed from + `src/lib/document-detail.ts` (already SELECTed) through `DocumentDetailImage` in + `src/lib/document-detail-contract.ts`; scope it as a contract change, not a viewer change. +7. Do not invent a second phone composer or change hide-reserve semantics. --- From f116e00b5fe06ddd14e23efa67ae02b142835efd Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 03:15:56 +0000 Subject: [PATCH 07/14] test(viewer): keep the canvas gate out of the phone-chrome consumer list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new spec imported blockExternalRequests from tests/helpers/phone-scroll, which silently enrolled it in scripts/verify-phone-chrome.mjs's consumer list — tests/verify-phone-chrome.test.ts asserts that list equals the set of specs importing that helper, and went red. Enrolling it would have been wrong anyway: this is a desktop raster gate and has nothing to do with phone chrome selection, so it keeps a local copy of the request block instead, with a comment naming the coupling so the next person does not re-import it. Also records #291: tests/pr-handoff-stop.test.ts fails for any session running as root, because it injects a write failure with chmod 0o555 and root ignores directory write bits. Confirmed pre-existing on a clean origin/main worktree with no local diff, so it is not from this branch — CI runs non-root and stays green, and only container sessions ever see it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7 --- docs/outstanding-issues.md | 3 ++- tests/ui-document-canvas.spec.ts | 22 ++++++++++++++++++++-- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/docs/outstanding-issues.md b/docs/outstanding-issues.md index 9a7bc1fe93..ec525e9c1f 100644 --- a/docs/outstanding-issues.md +++ b/docs/outstanding-issues.md @@ -171,7 +171,7 @@ removed after current-main verification; it is not missing recommended work. | 116 | `#289` | A2 | High — auth/identity | Next auth module touch | 1–2 hours | Export a named helper (e.g. `authorizationIdentity(headers)`) from the auth module and use it at every property-access call site; consider a lint rule or branded type so `.Authorization` stops type-checking at all. **Stop:** do not change `authorizationHeadersForAccessToken` to emit uppercase — lowercase is the correct Fetch/Headers convention and callers that pass the object wholesale to `fetch` depend on it. | - + ## Open items > **Merged-main canary update (2026-07-23, run `30018289898`):** the new structured report correctly recorded evaluated tree `c24f2e8f2d30d0c59fc1eba025d3dcd63478137e`, run/attempt identity and `cross-region-runner` latency context. Golden retrieval remained 36/36 with document/content recall 1.0 and no failed cases. The 44-case answer gate had grounded-supported and unsupported-correct rates of 1.0, but failed because `neuroleptic-side-effect-escalation` again returned one citation where two are required (citation-failure rate 0.0227). `admission-discharge-comparison` again omitted the specific AKG admission document after `comparison_source_extractive_fallback`; `admission-discharge-coverage-paraphrase` was advisory-only at 24,870 ms. Answer cost was reported as `$0.234736`. Do not retry immediately: retain this as the first structured datapoint, compare it with the scheduled 2026-07-26 report, and keep retrieval/ranking unchanged. @@ -332,6 +332,7 @@ removed after current-main verification; it is not missing recommended work. | #288 | P3 | rec | Decide whether DocumentViewer adopts the template it was extracted from, or the partial adoption is recorded as final | **Outcome:** the in-page navigation template has one deliberate owner story rather than an unexplained gap. **Detail:** PR 1 (2806d5e, #1740) extracted the header from DocumentViewer.tsx and differential-detail-page.tsx, which held it near-verbatim twice, into src/components/in-page-nav/InPageNavHeader. differential-detail-page was converted onto it; DocumentViewer was deliberately NOT, because its own useDocumentSectionSpy and useDocumentChromeMetrics wiring and its CSS custom-property names (--document-anchor-offset, --document-sticky-header-height, [data-document-sticky-header]) are pinned verbatim by tests/header-scroll-hide-contract.test.ts:110-113. Once #286 and #287 land, the template is adopted on every information page EXCEPT DocumentViewer. `docs/search-chrome-behaviour.md` (Default in-page navigation template) already records that DocumentViewer keeps its own header copy because it owns the page h1, uses edge-glass-header, and is pinned by visual baselines — so the gap is documented, not overlooked. #286 generalises chrome metrics for information pages only; it does not close DocumentViewer convergence. **Next:** owner decision only — convert DocumentViewer later (leaving pinned `--document-*` property names untouched per tests/header-scroll-hide-contract.test.ts:110-113), or explicitly mark the documented non-adoption as the final end state in this ledger when the series closes. **Stop:** do not rename or repoint the pinned document CSS custom properties to unify them with the information-page ones - the contract test pins those exact strings and the document route is the highest-traffic surface in the app. | session 2026-08-08; PR #1740 | 2026-08-08 | | #289 | P2 | issue | authorizationHeader is lowercase, and reading .Authorization off it fails silently | **Outcome:** nobody keys identity off `authorizationHeader` by property again without hitting a gate or a comment first. **Detail:** `authorizationHeadersForAccessToken` returns `{ authorization: 'Bearer …' }` — lowercase, per the Fetch/Headers convention (`src/lib/supabase/client.tsx:82`). The value is typed `Record`, so reading `.Authorization` type-checks, returns undefined, and degrades to whatever fallback the caller wrote. On PR #1741 the same mistake was made twice in one session and both were identity-scoping code: the in-flight signed-URL dedupe key in `use-signed-image-url.ts` collapsed every user onto `endpoint+''`, so an account switch with a request in flight could hand user B user A's signed URL; and `detailRequestSignature` in `DocumentViewer.tsx` omitted the token it documented as being present (weaker in practice — `authStatus`/`initialDetailIdentityStale` and the render-time identity reset still moved on a real switch — but the stated defence was not the shipped one). Review caught both before merge and each site now reads `headers.authorization ?? headers.Authorization ?? ''`. Merged main has no other property reads: every remaining caller passes the header object wholesale to `fetch`, where casing is irrelevant. **Next:** cheapest first — export a named helper (e.g. `authorizationIdentity(headers)`) from the auth module and use it at both sites so there is one definition, then consider a lint rule or a narrower type (a branded `AuthorizationHeader` with a lowercase-only key) so `.Authorization` stops type-checking at all. **Stop:** do not 'fix' this by emitting uppercase from `authorizationHeadersForAccessToken` — lowercase is the correct convention and callers pass the object to `fetch`. | PR #1741 review (Codex signed-URL/cache finding); src/lib/supabase/client.tsx:82; session 2026-08-08 | 2026-08-08 | | #290 | P3 | rec | OffscreenCanvas for the PDF raster is unjustified until the page-flip cost is read from CI | **Outcome:** the worker-raster question is settled by a number rather than left as a standing 'optional' item in the redesign plan. **Detail:** docs/plans/document-viewer-redesign-plan.md conditions OffscreenCanvas on 'measured main-thread paint cost'. Phase 3 (Task 5) did not implement it, deliberately: virtualization now keeps the reader's page and one neighbour already rastered, so the cold-render-per-flip cost that motivated a worker raster is largely gone before any threading work starts, and moving pdf.js rendering off the main thread would put the canvas the clinical source is drawn into behind a transfer boundary — a real risk on the one surface where a blank page is a clinical failure. **No number exists yet and none could be produced locally:** pdfjs-dist@6 needs Map.prototype.getOrInsertComputed, which this container's Chromium 141 lacks and Node 24.13.0 also lacks, so neither a browser nor a headless harness here can raster a page (see #279). **Next:** read the measurement the gate already captures. tests/ui-document-canvas.spec.ts attaches page-flip-raster-cost.json (flipToPaintedMs, longTaskCount, longTaskTotalMs, longestTaskMs, canvasBackingPixels) and logs a '[viewer-canvas] page flip painted in Nms' line, on every Production UI run and on any host with the pinned Chromium 151 build: npm ci --include=dev && npx playwright install chromium && npm run ensure && npm run test:e2e -- tests/ui-document-canvas.spec.ts --project=chromium. If the longest task during a flip is comfortably under ~50ms, close this as not-worth-doing and strike the row from the plan's Phase 3 table. **Stop:** do not implement OffscreenCanvas on principle because the plan lists it — the plan conditions it on the measurement, and the measurement is now cheap to obtain. | session 2026-08-09 document viewer Phase 3, Task 5; docs/plans/document-viewer-phase3-handover.md | 2026-08-09 | +| #291 | P2 | issue | tests/pr-handoff-stop.test.ts fails whenever the unit suite runs as root, which is every remote/container session | **Outcome:** npm run test (and therefore verify:cheap and verify:pr-local) is green in a root container, so a real regression is not hidden behind a known-red test that everyone learns to ignore. **Detail:** 'pr-handoff-stop hook > emits handoff context only when the marker file exists' sets the fake git dir to 0o555 (tests/pr-handoff-stop.test.ts:166) to force the marker write to fail, then asserts markerExists() is false. Root ignores directory write permission bits, so the write succeeds, the marker exists, and the assertion fails with 'expected true to be false'. Verified 2026-08-09 as PRE-EXISTING and unrelated to the change under test: it reproduces on a clean detached worktree at origin/main (883e725) with no local diff, and 'id -u' is 0 in this container — a direct probe confirmed a touch succeeds inside a 0555 directory here. CI runs as a non-root user, so the test passes there and this never surfaces on a PR; it only ever bites a container session, where it costs a full re-run of the unit suite to rule out. **Next:** make the failure-injection root-proof rather than permission-based — e.g. point the hook at a path whose parent is a regular file, or a non-existent nested path, so the write fails for every uid. Alternatively skip the case when process.getuid?.() === 0 with an explicit reason, but prefer the first: a skipped case in a root container is coverage nobody has. **Stop:** do not delete the assertion. Failing open with no additionalContext when the marker cannot be written is the behaviour that keeps the model from being told tools are denied when they are not. | session 2026-08-09 document viewer Phase 3; verify:pr-local run, reproduced on origin/main worktree | 2026-08-09 | ## Resolved / archive diff --git a/tests/ui-document-canvas.spec.ts b/tests/ui-document-canvas.spec.ts index dbbbb317eb..faefb5d382 100644 --- a/tests/ui-document-canvas.spec.ts +++ b/tests/ui-document-canvas.spec.ts @@ -1,7 +1,5 @@ import { expect, test, type Locator, type Page } from "playwright/test"; -import { blockExternalRequests } from "./helpers/phone-scroll"; - /** * Browser gate for the PDF reader's raster surface. * @@ -47,6 +45,26 @@ import { blockExternalRequests } from "./helpers/phone-scroll"; // "a page flip changed the raster" a real assertion rather than a tautology. const CANVAS_DOCUMENT = "/documents/22222222-2222-4222-8222-222222222222?page=1"; +/** + * Fail any request that leaves the local origin. + * + * Deliberately a local copy rather than the shared `helpers/phone-scroll` + * version: importing that helper enrols a spec in the phone-chrome consumer list + * (`scripts/verify-phone-chrome.mjs`, asserted by `tests/verify-phone-chrome.test.ts`), + * and this desktop raster gate has nothing to do with phone chrome selection. + */ +async function blockExternalRequests(page: Page) { + await page.route("**/*", async (route) => { + const url = new URL(route.request().url()); + const local = ["localhost", "127.0.0.1", "::1", "[::1]"].includes(url.hostname); + if ((url.protocol === "http:" || url.protocol === "https:") && !local) { + await route.abort("blockedbyclient"); + return; + } + await route.fallback(); + }); +} + type CanvasReading = { readonly width: number; readonly height: number; From 0436c3b495bee05777efc2ea1709230044e26b42 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 03:24:58 +0000 Subject: [PATCH 08/14] docs(issues): record the measured bundle-budget headroom on #252 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A clean production build for this branch reports 306 client chunks at 1538.4 KiB gzip against the 1406.4 KiB baseline captured 2026-08-04 — +9.4% inside a 10% tolerance, so roughly 8 KiB of gzip headroom remains. The drift is pre-existing rather than from this branch: Phase 3 adds no dependency and its code delta is small. But it means the next feature-sized PR of any kind trips check:bundle-budget whatever it touches, which turns #252's open question — whether counting mockup chunks makes that a real signal — from theoretical into the thing that decides how the next red build is read. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7 --- docs/outstanding-issues.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/outstanding-issues.md b/docs/outstanding-issues.md index ec525e9c1f..3b52753cca 100644 --- a/docs/outstanding-issues.md +++ b/docs/outstanding-issues.md @@ -297,7 +297,7 @@ removed after current-main verification; it is not missing recommended work. | #249 | P2 | rec | Extend issues/plan with an agent-safe wins classifier | **Outcome:** /issues and recommendation answers can filter agent-safe short wins (estimate ≤4h, Capability not Operator-only, no RAG/provider, not blocked on a human decision) without inventing a 35th skill. **Next:** add an optional filter/section to the issues skill (and plan/flightplan when useful) that lists those wins from the open table + queue; keep it as procedure on the existing skills unless the classifier is reused thrice — only then mint a thin wins alias. **Stop:** do not weaken acuity ordering; operator A1s still outrank engineering wins. | session 2026-08-05 fastest-wins plan | 2026-08-05 | | #250 | P2 | task | Execute the fastest-wins multi-wave plan (Wave 0–4) | **Outcome:** the 2026-08-04/05 fastest-wins plan is executed with parallel agents, correct gates, and no regression. **Acuity first:** the recommended queue remains acuity-ordered — A1 rows (#059/#053/#231/#207/#226) are not demoted by wave numbers. Operator-track items (#022/#183) retain their A2 priority and approval gates. **A1 track (parallel, not a wave):** run approved A1 work (#059/#053/#231/#207/#226) whenever capability/approvals allow — do not wait for Waves 0–3. **Engineering waves only:** 0 = ledger/process gates (#201 resolved 2026-08-06; continue with remaining #202 work); 1A = #147+#176 phone CLS; 1B = #149+#167+#204+#210 gate integrity; 1C = hygiene/docs #232–#236+#223+#157+#151/#154/#187; 2 = #117 then #118; 3 = #098 then #189 (defer #099 body); 4 = remaining non-A1 clinical/UI packaging once approvals exist (never a holding pen for A1 items). **Next:** continue Wave 0/#202 on a fresh branch off origin/main in parallel with any approved A1 work; use gates + verification-router per PR; respect #155 concurrency. **Stop:** no RAG behaviour without flag+canary; no provider gates without approval; do not mix operationalRisk with clinical/UI in one squash. | session 2026-08-05 fastest-wins plan | 2026-08-05 | | #251 | P3 | rec | Handoff checklist should pair gates skill with verification-router | **Outcome:** every PR handoff picks the smallest correct gate and pastes the decisive proof line, using verification-router when scope is unclear. **Next:** add one line to handoff/gates productivity defaults: after flightplan, run verification-router (or gates) before claiming green; never report exit 0 alone. **Stop:** do not stack verify:cheap + verify:ui + verify:release by default. | session 2026-08-05 fastest-wins plan | 2026-08-05 | -| #252 | P2 | issue | check:bundle-budget counts mockup chunks, contradicting #013's initial-bundle position | The budget's totalGzipBytes comes from measureChunkPaths(walkJsFiles(CHUNKS_DIR)) — EVERY built client chunk, including routes that 404 in production. The manifest-scoped initialDashboardChunks set is used only for the fixture-payload assertion, not the budget. So two repo positions disagree about mockups and nothing says so: #013 records that mockup chunks 'are not an initial production bundle' and must not be restructured without deploy-artifact evidence, while the gate charges them against a repo-wide ceiling. PR #1580 is the live cost — a mockups-only PR blocked on 'FAIL +10.1% vs baseline (tolerance 10%)' for chunks no user can load; it has sat red and unmerged since 2026-08-02. Docs now state the mechanism (AGENTS.md gate bullet, CLAUDE.md mockups bullet) so it stops being a surprise, but the metric decision is unmade. Next action: pick one and make the script say so — (a) exclude mockup-only chunks from totalGzipBytes so the number means production weight, which matches #013 but removes all back-pressure on mockup growth (59 routes on main today, 4 more in open PRs); or (b) keep counting them, rename the reported metric so it does not read as production bundle weight, and treat the tolerance as a deliberate hygiene ceiling. Option (b) additionally wants the mockup share reported separately, which is the measurement #013 asks for before any prune. Stop: do not raise the tolerance or run --update to clear #1580 — that discards the only back-pressure without deciding anything. Renumbered from this PR's original #249 → #252 because main claimed #249–#251 via PR #1624. | session 2026-08-05 open-PR review; PR #1580 Build log; scripts/check-bundle-budget.mjs; ledger #013 | 2026-08-05 | +| #252 | P2 | issue | check:bundle-budget counts mockup chunks, contradicting #013's initial-bundle position | The budget's totalGzipBytes comes from measureChunkPaths(walkJsFiles(CHUNKS_DIR)) — EVERY built client chunk, including routes that 404 in production. The manifest-scoped initialDashboardChunks set is used only for the fixture-payload assertion, not the budget. So two repo positions disagree about mockups and nothing says so: #013 records that mockup chunks 'are not an initial production bundle' and must not be restructured without deploy-artifact evidence, while the gate charges them against a repo-wide ceiling. PR #1580 is the live cost — a mockups-only PR blocked on 'FAIL +10.1% vs baseline (tolerance 10%)' for chunks no user can load; it has sat red and unmerged since 2026-08-02. Docs now state the mechanism (AGENTS.md gate bullet, CLAUDE.md mockups bullet) so it stops being a surprise, but the metric decision is unmade. Next action: pick one and make the script say so — (a) exclude mockup-only chunks from totalGzipBytes so the number means production weight, which matches #013 but removes all back-pressure on mockup growth (59 routes on main today, 4 more in open PRs); or (b) keep counting them, rename the reported metric so it does not read as production bundle weight, and treat the tolerance as a deliberate hygiene ceiling. Option (b) additionally wants the mockup share reported separately, which is the measurement #013 asks for before any prune. Stop: do not raise the tolerance or run --update to clear #1580 — that discards the only back-pressure without deciding anything. Renumbered from this PR's original #249 → #252 because main claimed #249–#251 via PR #1624. **Headroom measured 2026-08-09 (document viewer Phase 3):** a clean production build reports 306 client chunks, 1538.4 KiB gzip against the 1406.4 KiB baseline captured 2026-08-04 — **+9.4% inside a 10% tolerance**, i.e. roughly 8 KiB gzip of headroom before the gate turns red. That drift is pre-existing and not from this branch (the Phase 3 diff adds no dependency and its code delta is small), but it means the next feature-sized PR of any kind will trip the gate whatever it touches, and the mockup-counting question this row is about is what decides whether that would be a real signal. **Next action unchanged, now urgent rather than theoretical:** reconcile the counting scope against #013's initial-bundle position, then either refresh the baseline deliberately (npm run check:bundle-budget -- --update) or narrow what the gate totals. Do not refresh the baseline as an incidental step inside an unrelated PR. | session 2026-08-05 open-PR review; PR #1580 Build log; scripts/check-bundle-budget.mjs; ledger #013 | 2026-08-05 | | #253 | P3 | task | #1606 needs a hand-merge against merged PR #1615, not a rebase | SUPERSEDED IN PART 2026-08-07: the component both PRs rewrite no longer exists. `MobileResultFilterControl` — the native `` whose system-blue selected-value highlight #1606 set out to fix — was deleted when every mode moved to `ResultFilterTrigger` + `ResultFilterSheet` (#247). The blue-highlight defect is therefore gone by removal rather than by #1606's portal menu, and #1615's iOS 16px anti-zoom rule no longer applies to any phone results filter (it still governs other native selects; see search-chrome-behaviour rule 5). What is left of this row is a decision, not a merge: #1606 is still open and cannot be rebased onto a function that was deleted. Recommend closing it as superseded rather than hand-merging — but that is a human call, and this row stays open until someone makes it. On keyboard parity, correcting an earlier version of this row that claimed otherwise: `ResultFilterSheet` does NOT use native radio inputs. It is `role="radiogroup"` over buttons with a bespoke roving-tabIndex handler — one tab stop per group, Arrow/Home/End moving focus, selection following except onto a dead end — modelled on `SegmentedControl`. That is the same shape #1606 hand-rolled, so parity is plausible but not free; the DOM tests in `tests/search-results-header-band.dom.test.tsx` pin it, and a real-device pass is still worth doing before closing #1606. The Codex P2 about ArrowDown focusing a disabled placeholder is answered by construction: arrowing onto a dead end moves focus so its note is announced but never commits it, asserted by "puts a dead end on the arrow path without ever selecting it". | session 2026-08-05 open-PR review sweep; PR #1606 (open, dirty, verified live); Codex review thread on search-results-header-band.tsx:696 | 2026-08-05 | | #254 | P2 | issue | PR #1617's Codex Cloud acceptance evidence is pinned to a head the branch has moved past | PR #1617 is the surviving side of the Codex Cloud MCP dispute (PR #1613 was closed against it on 2026-08-05), and its whole case rests on one first-hand measurement: a fresh Codex Cloud task reporting 'no Railway tool exposed', 'no Supabase tool exposed', 'no GitHub tool exposed' — zero provider tools from repo-written MCP config — while the hosted OAuth path passed in ChatGPT. That evidence is recorded in docs/audit/cloud-connection-acceptance-2026-08-05.md against task head 76dfe85fa93787b3845d0bd460aa18ff753ca2ca, and the PR body states 'The final branch head is 7d485f88db391cc7e8e73c57ddbde61f532375fc'. The branch head is now bf7684a77ef5fa5d53a1e7891cb7f446d4741e4a, so both pins are stale and the acceptance no longer describes what would merge. This matters more than usual because the acceptance IS the argument: #1613 was closed on the strength of it, and its own merge gate is the kind of exact-head check that a moved head invalidates. Next action: before #1617 leaves draft, re-run the fresh Codex Cloud acceptance at the exact current head and update both the report and the body pin, or state explicitly which parts of the evidence are head-independent (the connector-page absence of Railway/Supabase is a product-surface fact and does not depend on the head; the zero-tool discovery result does). Stop: do not treat the 76dfe85f/7d485f88 evidence as covering the current head, and do not reopen #1613 on the basis that #1617's evidence is stale — staleness is not refutation. Renumbered from this PR's original #251 → #254 because main claimed #249–#251 via PR #1624. | session 2026-08-05 open-PR review sweep; PR #1617 body and docs/audit/cloud-connection-acceptance-2026-08-05.md | 2026-08-05 | | #255 | P2 | issue | Remote/Cloud containers cannot run any browser gate: Playwright lock drift plus a missing Chromium build | In the 2026-08-05 remote container, npm run verify:pr-local failed closed at its first precondition — 'check:installed-lock-parity ... playwright: installed 1.62.0 does not match locked 1.62.1' — and Chromium could not launch at all: 'Executable doesn't exist at /opt/pw-browsers/chromium_headless_shell-1234/...' while the image only ships chromium-1194 and chromium_headless_shell-1194. So verify:ui, verify:phone-chrome and verify:release are all unrunnable in a remote session regardless of the diff, and verify:pr-local cannot even reach its selected gates. This is not a one-off 'run npm ci': the installed Playwright disagrees with the lockfile, and installing the locked 1.62.1 would request a browser revision the image also does not carry, so the fix is an image/pin alignment rather than an install. Consequence worth writing down: PRs authored from remote sessions can only produce static and unit evidence locally and must delegate browser proof to CI's Production UI jobs, which is what #1620/#1621/#1622 did — #1621's Chromium evidence came entirely from CI (0 failed of 17, all three Production UI shards). Next action: align the container image's Playwright browser builds with the lockfile pin, or document the delegation explicitly in docs/testing.md so a remote session does not read a fail-closed parity error as a defect in its own change. Related but distinct from #149, which is about the parity checker only comparing seven top-level packages rather than about an actual blocking drift. Stop: do not work around it by setting PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD or pointing executablePath at the mismatched 1194 build to force a green browser run — a browser gate run against the wrong revision is not evidence. Renumbered from this PR's original #252 → #255 because main claimed #249–#251 via PR #1624. **Second reproduction 2026-08-07 (remote container, PR #1670):** identical parity failure, same versions — 'playwright: installed 1.62.0 does not match locked 1.62.1' — two days later on a different image instance, so this is a standing property of the remote image rather than one bad container. New detail: the diff was docs-only (two append-only ledger files), and parity still halted `verify:pr-local` at step 2 of a 10-step plan that contained no browser gate at all. So the blast radius is wider than browser proof: a remote session cannot complete the PR-local gate for ANY scope, and has to re-run the selected steps by hand to get evidence (done there: the remaining eight all passed). That strengthens the documentation half of the next action — docs/testing.md should say a remote session may see this on a diff that never touches a browser. | session 2026-08-05 open-PR review sweep; verify:pr-local and verify:phone-chrome attempts on PRs #1620/#1621 | 2026-08-05 | @@ -333,7 +333,7 @@ removed after current-main verification; it is not missing recommended work. | #291 | P2 | issue | Placeholder buttons pin `disabled` and `disabled`+`aria-disabled` as two contradictory shapes; decide which wins | **CORRECTS this row's original text, which was wrong on its central claim.** As first written (2026-08-09) this row asserted that pairing `disabled` with an `aria-describedby` sr-only reason means the reason "is never announced to keyboard or screen-reader users", and that `docs/wiring-conventions.md` and `eslint-rules/require-button-wiring.mjs" blessed the broken pairing so a repo-wide pass was required. **All three claims are refuted.** A natively `disabled` button remains in the accessibility tree with its accessible description intact — a screen reader reaching it by virtual cursor or swipe still announces the reason. That is asserted, not assumed: `tests/favourites-hub-unavailable-controls.dom.test.tsx:31-46` pins `toBeDisabled()`, `not.toHaveAttribute("aria-disabled")` AND `toHaveAccessibleDescription(...)` together on all three favourites-hub placeholders, under the title "keeps unavailable actions natively disabled and exposes their reasons" — i.e. native `disabled` is a deliberate, tested decision, not drift. `require-button-wiring` already accepts both `disabled` and `aria-disabled`, so no rule change was ever needed. What `disabled` does remove is the tab stop, so a keyboard-only user without a screen reader cannot reach the control to discover why it is unavailable — a product judgement, not a WCAG failure (a disabled control is exempt from focus-order requirements, and the `title` covers pointer users). **Fixed in this PR:** the one genuine defect was documentation — `docs/wiring-conventions.md` named `favourites-hub.tsx` as "the reference markup" while showing `aria-disabled="true"`, which is not what that file does. The block now shows native `disabled`, states why the description survives it, cites the pinning test, and says when `aria-disabled` + no-op is correct instead (a roving-tabindex group such as `ResultFilterSheet`). **What remains open, and is the only reason this row is still here:** `disabled` and `aria-disabled="true"` together is a third shape pinned two contradictory ways inside one test file — `tests/mobile-interaction-regressions.test.ts` asserts density placeholders in `differential-presentation-workflow-page.tsx` are native-only (`not.toContain("aria-disabled")`), and asserts `disabled aria-disabled="true"` together for the Add placeholders in `visual-evidence.tsx` and `evidence-panels.tsx`. The pairing is redundant and the two attributes disagree about focusability. **Next:** an owner decision on which shape wins, then align the two pinned assertions and the four sites carrying the pairing (`visual-evidence`, `evidence-panels`, `forms-search-results-page`, `calculators/search-page`). **Stop:** do not swap `disabled` for `aria-disabled` in `favourites-hub.tsx` — that was attempted on 2026-08-09 and reverted; it contradicts the deliberate tested decision above. Do not treat `account-setup-dialog.tsx` as part of this: its placeholder styles itself through `disabled:` Tailwind variants, so any change there needs `aria-disabled:` variant renames as well. | session 2026-08-09; docs/wiring-conventions.md; eslint-rules/require-button-wiring.mjs; favourites-hub.tsx:367,382,544 | 2026-08-09 | | #292 | P2 | rec | Two assistants built the same queued conversion twice because neither workflow checks the open-PR list before starting | **Outcome:** picking up a queued ledger item cannot silently duplicate work another session already has in flight. **Detail:** on 2026-08-09 two assistants took the same queued `/issues` item roughly four hours apart and independently built the same in-page-nav conversion — PR #1766 (merged) and PR #1767 (closed as duplicate). Neither had any way to see the other: the ledger row was the only shared state. Correcting an earlier version of this row after CodeRabbit's review on PR #1773: it is not true that the ledger "has no in-progress state" — some rows do carry a progress marker in their prose (`IN PROGRESS` appears on two, and `IMPLEMENTED in PR #1766` on another). The accurate gap is narrower and worse: there is no structured status field and no atomic claim, so a marker is written by whoever did the work, usually after the fact, and nothing requires or checks one — which means the ABSENCE of a marker carries no information at all. Both sessions read it, both correctly concluded it was open, both built it. The wasted effort is the smaller cost; the larger one is that the two implementations diverged in shape, which is what forced the separate `PageSection` ownership decision recorded in `docs/search-chrome-behaviour.md`. Distinct from `#156`/`#168`, which are about two branches colliding on an **id** while appending; this is two sessions colliding on the **work** a row describes, and a collision-free id scheme would leave it untouched. **Mitigation landed 2026-08-09 (same PR as this row):** the check is now written into the three places an assistant actually reads before starting queued work — `.claude/skills/newtask/SKILL.md` "Before you start" (which already performed an open-PR read for PR bundling, so this asks that same list a second question and costs no extra call), `.claude/skills/issues/SKILL.md` after the read-only flow, and the `/issues` section of `AGENTS.md` so Codex and Cursor get it too rather than Claude Code only. All three say to scan for the **route, component or surface**, not the ledger id, because a duplicate PR rarely quotes the id; all three degrade to a warning when GitHub is unreachable so an offline session can still start work. **Next:** leave open for one or two queued-item cycles to see whether prose is enough. If a second duplicate lands anyway, this becomes the same class as `#258` — a rule enforced for one tool by prose with no gate — and the answer is a check, not more wording. **Stop:** do not implement a claim marker written back into the row when a session starts an item; that reintroduces exactly the read-modify-write contention `#168` exists to remove. Do not make the open-PR read a hard blocker. | session 2026-08-09; PR #1766 (merged); PR #1767 (closed duplicate) | 2026-08-09 | | #293 | P2 | issue | Controls declare min-h-tap and compute min-height 0px; a rendered-interactive tap audit needs a deterministic surface first | Two findings, one robust and one that blocked the gate. FINDING 1 (robust, reproduced in ALL SIX runs): controls that carry min-h-tap compute min-height 0px and render far below the 48px floor. Six distinct shapes seen across runs - 'a.inline-flex min-h-tap items-center' 16px, 'a.inline-flex min-h-tap shrink-0' 16.5px, 'button.flex min-h-tap w-full' 26.6px, 'button.grid min-h-tap min-w-tap' 36px, 'button.inline-flex min-h-tap items-center' 16px, 'button.inline-flex min-h-tap min-w-[94px]' 36px. min-h-tap works in general (the existing declared-carrier audit still measures carriers at or above 48px), so these elements have the declaration overridden to 0 rather than the utility being absent; likely an unlayered component class in globals.css, which by design outranks Tailwind utilities here. This was invisible because the pre-existing audit in tests/ui-style-contract.spec.ts only measures elements whose COMPUTED min-height is already at or above the floor (declared < tapFloor - 0.5 continue), so a floor overridden downward is skipped rather than flagged - the same structural blind spot as the h-10 case #265 named. FINDING 2 (why gate 2 did NOT land 2026-08-09): a rendered-interactive enumeration on /services?q=CMHT&run=1 is NOT DETERMINISTIC. Six runs against one production build returned 6, 5, 4, 3, 3 and 9 distinct control shapes, largely disjoint - one run saw answer-suggestion chips and a sort band, another a settled services results list. waitForLoadState('networkidle') plus deduplication to distinct shapes (instance counts measure how many results the query returned, and gave 9 vs 39) did NOT fix it; two consecutive agreeing runs were coincidence, and the next run differed again. The enumeration was written, proven to find real defects, and then REVERTED rather than landed, because tests/ui-style-contract.spec.ts runs in the required Production UI job via productionSpecPattern and scripts/playwright-pr-shards.mjs, so an intermittent version of it would block every merge in the repo. Next, in order: (1) find a deterministic surface for the audit - a static route with no async search, or a fixed seeded state - before re-attempting the enumeration; (2) separately, find what zeroes min-height on the min-h-tap carriers and fix or write a stated exception. Stop: do not re-land the enumeration on a live-search route, do not quarantine a brand-new test to get it merged (quarantine is for keeping flaky tests we already trust, and repo policy needs three reproductions on one SHA via tests/flake-ledger.json), do not lower any production tap target, and never to min-h-11 (known ui-smoke sub-pixel flake; production uses min-h-12). | session 2026-08-09 — M2 gate 2 enumeration (#265) | 2026-08-09 | -| #294 | P3 | rec | OffscreenCanvas for the PDF raster is unjustified until the page-flip cost is read from CI | **Outcome:** the worker-raster question is settled by a number rather than left as a standing 'optional' item in the redesign plan. **Detail:** docs/plans/document-viewer-redesign-plan.md conditions OffscreenCanvas on 'measured main-thread paint cost'. Phase 3 (Task 5) did not implement it, deliberately: virtualization now keeps the reader's page and one neighbour already rastered, so the cold-render-per-flip cost that motivated a worker raster is largely gone before any threading work starts, and moving pdf.js rendering off the main thread would put the canvas the clinical source is drawn into behind a transfer boundary — a real risk on the one surface where a blank page is a clinical failure. **No number exists yet and none could be produced locally:** pdfjs-dist@6 needs Map.prototype.getOrInsertComputed, which this container's Chromium 141 lacks and Node 24.13.0 also lacks, so neither a browser nor a headless harness here can raster a page (see #279). **Next:** read the measurement the gate already captures. tests/ui-document-canvas.spec.ts attaches page-flip-raster-cost.json (flipToPaintedMs, longTaskCount, longTaskTotalMs, longestTaskMs, canvasBackingPixels) and logs a '[viewer-canvas] page flip painted in Nms' line, on every Production UI run and on any host with the pinned Chromium 151 build: npm ci --include=dev && npx playwright install chromium && npm run ensure && npm run test:e2e -- tests/ui-document-canvas.spec.ts --project=chromium. If the longest task during a flip is comfortably under ~50ms, close this as not-worth-doing and strike the row from the plan's Phase 3 table. **Stop:** do not implement OffscreenCanvas on principle because the plan lists it — the plan conditions it on the measurement, and the measurement is now cheap to obtain. | session 2026-08-09 document viewer Phase 3, Task 5; docs/plans/document-viewer-phase3-handover.md | 2026-08-09 | +| #294 | P3 | rec | OffscreenCanvas for the PDF raster is unjustified until the page-flip cost is read from CI | **Outcome:** the worker-raster question is settled by a number rather than left as a standing 'optional' item in the redesign plan. **Detail:** docs/plans/document-viewer-redesign-plan.md conditions OffscreenCanvas on 'measured main-thread paint cost'. Phase 3 (Task 5) did not implement it, deliberately: virtualization now keeps the reader's page and one neighbour already rastered, so the cold-render-per-flip cost that motivated a worker raster is largely gone before any threading work starts, and moving pdf.js rendering off the main thread would put the canvas the clinical source is drawn into behind a transfer boundary — a real risk on the one surface where a blank page is a clinical failure. **No number exists yet and none could be produced locally:** pdfjs-dist@6 needs Map.prototype.getOrInsertComputed, which this container's Chromium 141 lacks and Node 24.13.0 also lacks, so neither a browser nor a headless harness here can raster a page (see #279). **Next:** read the measurement the gate already captures. tests/ui-document-canvas.spec.ts attaches page-flip-raster-cost.json (flipToPaintedMs, longTaskCount, longTaskTotalMs, longestTaskMs, canvasBackingPixels) and logs a '[viewer-canvas] page flip painted in Nms' line, on every Production UI run and on any host with the pinned Chromium 151 build: npm ci --include=dev && npx playwright install chromium && npm run ensure && npm run test:e2e -- tests/ui-document-canvas.spec.ts --project=chromium. Close this as not-worth-doing and strike the row from the plan's Phase 3 table only when a Production UI (or equivalent Chromium 151) run records decisive log lines for all three: longestTaskMs comfortably under ~50ms, plus explicit flipToPaintedMs and longTaskTotalMs budgets agreed for that host class and met on the same run. Do not close on longestTaskMs alone. **Stop:** do not implement OffscreenCanvas on principle because the plan lists it — the plan conditions it on the measurement, and the measurement is now cheap to obtain. | session 2026-08-09 document viewer Phase 3, Task 5; docs/plans/document-viewer-phase3-handover.md | 2026-08-09 | ## Resolved / archive diff --git a/src/components/document-viewer/document-rail-panels.tsx b/src/components/document-viewer/document-rail-panels.tsx index 89eb7cf44a..519865f1a0 100644 --- a/src/components/document-viewer/document-rail-panels.tsx +++ b/src/components/document-viewer/document-rail-panels.tsx @@ -272,6 +272,7 @@ export function DocumentViewerRail({ activePage={activePage} onSelectPage={onSelectPage} revealLabel="Tables and diagrams" + collectionKey={`${document?.id ?? "none"}:clinical`} /> )} @@ -286,6 +287,7 @@ export function DocumentViewerRail({ activePage={activePage} onSelectPage={onSelectPage} revealLabel="Administrative and reference tables" + collectionKey={`${document?.id ?? "none"}:audit`} />
    diff --git a/src/components/document-viewer/pdf-canvas-viewer.tsx b/src/components/document-viewer/pdf-canvas-viewer.tsx index a478299055..eef2ba85f6 100644 --- a/src/components/document-viewer/pdf-canvas-viewer.tsx +++ b/src/components/document-viewer/pdf-canvas-viewer.tsx @@ -302,7 +302,7 @@ const PdfPageSlot = memo(function PdfPageSlot({ ) : (