From 156db63f1b60f09791e426b043ea90d427b789ab Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 07:05:13 +0000 Subject: [PATCH 1/2] test(viewer): replace a source-text grep with the behaviour it claimed to guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The document-wide canvas budget was asserted by checking that pdf-canvas-viewer.tsx *contained the strings* resolveLiveCanvasWindow, resolveRenderAheadPages, liveCanvasLimit and requestIdleCallback. Its own comment recorded why: the virtualization DOM test's fixture pages were too small for the budget to bind, so a viewer that dropped it would still window correctly there. The observation was right and the conclusion was wrong. Measured against the regression it named, that guard did not work. Replacing the budget call with a hardcoded 3 leaves every one of those identifiers in the file — the import, the variable — so the grep stayed green while the viewer retained three full-zoom canvases. It was not merely brittle; it was brittle and ineffective, which is the worse combination because it reads as coverage. The DOM harness can bind the budget, it just needed the arithmetic. At maximum zoom on a dpr-3 display an A4 page costs sqrt(2^24 / 8.0M) = 1.45 output scale, so one canvas is ~16.8M backing pixels against the 24M document budget and resolveLiveCanvasWindow returns 1. The new case asserts render-ahead is vetoed by memory rather than by the idle gate — hence asserting after flushIdle, not before — and it fails on exactly the substitution the grep survived. It sits next to the default-density case so the pair documents the curve rather than one point. Two smaller couplings fixed while there, both of the same kind: a test breaking on a change that is not a regression. RAIL_IMAGE_WINDOW is now exported and the rail test derives its counts from it. Six, and the twelve/eighteen/thirty-four/forty derived from it, were literals, so tuning the window from 6 to 8 — a product judgement about how much of a figure rail is worth mounting up front — turned five assertions red. Verified by actually tuning it to 8: all seven still pass. The keyboard test pinned exact aria-label phrasing (/F fits the width/, /R rotates/). The contract is that every binding is named, because a binding a screen-reader user is never told about is a binding they do not have; the sentence around the keys is copy. It now asserts the keys. No production behaviour changes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7 --- .../document-viewer/source-panels.tsx | 2 +- tests/client-performance-boundaries.test.ts | 31 +++++----- tests/document-rail-image-window.dom.test.tsx | 57 ++++++++++++------- tests/document-viewer-keyboard.dom.test.tsx | 12 ++-- ...nt-viewer-page-virtualization.dom.test.tsx | 29 ++++++++++ 5 files changed, 88 insertions(+), 43 deletions(-) diff --git a/src/components/document-viewer/source-panels.tsx b/src/components/document-viewer/source-panels.tsx index 9ba3516b73..a725fff38a 100644 --- a/src/components/document-viewer/source-panels.tsx +++ b/src/components/document-viewer/source-panels.tsx @@ -444,7 +444,7 @@ export function DocumentImage({ * Six covers the great majority of indexed documents outright, so most rails * never mount a sentinel at all. */ -const RAIL_IMAGE_WINDOW = 6; +export const RAIL_IMAGE_WINDOW = 6; /** * Lookahead for the rail's own signed-URL requests, against `SignedImage`'s diff --git a/tests/client-performance-boundaries.test.ts b/tests/client-performance-boundaries.test.ts index 2db48d935e..865a1b1ec7 100644 --- a/tests/client-performance-boundaries.test.ts +++ b/tests/client-performance-boundaries.test.ts @@ -85,23 +85,20 @@ 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"); - }); + // The document-wide canvas budget and the deferred render-ahead are covered by + // `tests/document-viewer-page-virtualization.dom.test.tsx`, behaviourally. + // + // They were briefly asserted here instead, by checking the viewer source + // contained `resolveLiveCanvasWindow` / `resolveRenderAheadPages` / + // `liveCanvasLimit`. That guard was measured against the regression it named + // and did not catch it: replacing the budget call with a hardcoded `3` leaves + // every one of those identifiers in the file, so the grep stayed green while + // the viewer retained three full-zoom canvases. The DOM test now drives the + // budget until it binds (a page at maximum zoom on a dpr-3 display costs the + // whole per-canvas ceiling) and fails on exactly that substitution. + // + // Worth generalising when adding a contract here: a source-text assertion can + // only prove a name is present, never that it is load-bearing. it("revalidates cached document download URLs on every viewer action", () => { const viewer = source("src/components/DocumentViewer.tsx"); diff --git a/tests/document-rail-image-window.dom.test.tsx b/tests/document-rail-image-window.dom.test.tsx index ca56adf8fe..16944dbaed 100644 --- a/tests/document-rail-image-window.dom.test.tsx +++ b/tests/document-rail-image-window.dom.test.tsx @@ -10,7 +10,7 @@ vi.mock("@/lib/supabase/client", () => { return { useAuthSession: () => ({ authorizationHeader, markSessionExpired, session: null }) }; }); -import { DocumentImageList } from "@/components/document-viewer/source-panels"; +import { DocumentImageList, RAIL_IMAGE_WINDOW } from "@/components/document-viewer/source-panels"; import type { ImageRow } from "@/components/document-viewer/types"; /** @@ -93,56 +93,69 @@ function reachSentinel() { const renderedRows = () => screen.queryAllByTestId("document-image").length; +/** + * A list long enough that the window always applies, expressed against the + * window rather than as a literal. + * + * Every count below is derived from `RAIL_IMAGE_WINDOW`. Tuning the window from + * 6 to 8 is a product judgement about how much of a figure rail is worth + * mounting up front — it is not a regression, and it should not turn five + * assertions red. + */ +const LONG_LIST = RAIL_IMAGE_WINDOW * 6 + 4; + describe("document rail figure windowing", () => { it("mounts only the first window of rows for a long figure list", () => { - render(); + render(); - expect(renderedRows()).toBe(6); + expect(renderedRows()).toBe(RAIL_IMAGE_WINDOW); 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(); + const shortList = RAIL_IMAGE_WINDOW - 2; + render(); - expect(renderedRows()).toBe(4); + expect(renderedRows()).toBe(shortList); expect(screen.queryByTestId("document-image-reveal")).toBeNull(); }); it("grows by one window each time the sentinel is reached", () => { - render(); + render(); act(() => reachSentinel()); - expect(renderedRows()).toBe(12); + expect(renderedRows()).toBe(RAIL_IMAGE_WINDOW * 2); act(() => reachSentinel()); - expect(renderedRows()).toBe(18); + expect(renderedRows()).toBe(RAIL_IMAGE_WINDOW * 3); }); 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(); + render(); - const reveal = screen.getByRole("button", { name: /show the remaining 34/i }); + const reveal = screen.getByRole("button", { name: /show the remaining \d+/i }); fireEvent.click(reveal); - expect(renderedRows()).toBe(40); + expect(renderedRows()).toBe(LONG_LIST); 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); + const { rerender } = render(); + fireEvent.click(screen.getByRole("button", { name: /show the remaining \d+/i })); + expect(renderedRows()).toBe(LONG_LIST); - rerender(); + const shrunk = RAIL_IMAGE_WINDOW - 3; + rerender(); - expect(renderedRows()).toBe(3); + expect(renderedRows()).toBe(shrunk); expect(screen.queryByTestId("document-image-reveal")).toBeNull(); }); @@ -155,14 +168,16 @@ describe("document rail figure windowing", () => { it("resets the window when remounted for another document's long list", () => { const { rerender } = render( - , + , ); - fireEvent.click(screen.getByRole("button", { name: /show the remaining 34/i })); - expect(renderedRows()).toBe(40); + fireEvent.click(screen.getByRole("button", { name: /show the remaining \d+/i })); + expect(renderedRows()).toBe(LONG_LIST); - rerender(); + rerender( + , + ); - expect(renderedRows()).toBe(6); + expect(renderedRows()).toBe(RAIL_IMAGE_WINDOW); expect(screen.getByTestId("document-image-reveal")).toBeTruthy(); }); }); diff --git a/tests/document-viewer-keyboard.dom.test.tsx b/tests/document-viewer-keyboard.dom.test.tsx index 6eeaed8649..4ffadd1c1d 100644 --- a/tests/document-viewer-keyboard.dom.test.tsx +++ b/tests/document-viewer-keyboard.dom.test.tsx @@ -216,9 +216,13 @@ describe("PDF reader keyboard bindings", () => { 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/); + + // A binding a screen-reader user is never told about is a binding they do + // not have, so the contract is that every key is named. The phrasing around + // the keys is copy and may be rewritten freely — assert the keys, not the + // sentence, or the next wording pass turns this red for no defect. + for (const key of ["Page Up", "Page Down", "Home", "End", "F", "R"]) { + expect(label, `the holder's accessible name does not mention ${key}`).toMatch(new RegExp(`\\b${key}\\b`)); + } }); }); diff --git a/tests/document-viewer-page-virtualization.dom.test.tsx b/tests/document-viewer-page-virtualization.dom.test.tsx index c6684b8e6e..d459b2c91d 100644 --- a/tests/document-viewer-page-virtualization.dom.test.tsx +++ b/tests/document-viewer-page-virtualization.dom.test.tsx @@ -1,6 +1,8 @@ import { act, cleanup, render, screen, waitFor } from "@testing-library/react"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { VIEWER_MAX_ZOOM } from "@/components/document-viewer/viewer-zoom"; + /** * Page virtualization behaviour for the PDF reader, in a real React tree. * @@ -257,6 +259,33 @@ describe("PDF reader page virtualization", () => { expect(getPageCalls).not.toContain(6); }); + it("collapses render-ahead to the reader's page alone when one canvas costs the whole budget", async () => { + // The other half of the curve the test above measures. + // + // `resolveCanvasRasterPlan` bounds ONE canvas against WebKit's ~2^24 ceiling + // and says nothing about how many exist, so the document-wide budget is what + // stops three individually-legal canvases exhausting device memory. At this + // page size, zoom and density a single canvas costs ~16.8M backing pixels + // against a 24M budget, so `resolveLiveCanvasWindow` returns 1 and + // render-ahead is vetoed by memory rather than by the idle gate — which is + // why this asserts AFTER `flushIdle()`, not before. + vi.stubGlobal("devicePixelRatio", 3); + + await renderViewer({ initialPage: 4, fitWidth: false, zoom: VIEWER_MAX_ZOOM }); + + // `renderZoom` is a debounced mirror of `zoom`; the raster runs at the + // default zoom until it settles. Reading before that measures a cheap canvas + // and proves nothing about the budget. + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 200)); + }); + await flushIdle(); + + await waitFor(() => expect(renderedPages()).toEqual([4])); + expect(getPageCalls).not.toContain(3); + expect(getPageCalls).not.toContain(5); + }); + 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; From 3fc1b4a4488490f46d6ed4eae6dcb03f1c9af389 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 07:18:39 +0000 Subject: [PATCH 2/2] docs(ledger): record the test-simplification review for PR #1777 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01N496muQbJVbJW8XvkCgKc7 --- docs/branch-review-ledger.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index af6742fb0c..371471355b 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -835,3 +835,4 @@ Records before 2026-07-28 were written by hand and had drifted: 146 lines carrie | 2026-08-09 | claude/document-viewer-phase-3-bj5k5v | 0436c3b495bee05777efc2ea1709230044e26b42 | document viewer Phase 3: page virtualization, rail windowing, signed-URL/decode priority, keyboard reading mode, first canvas browser gate | PR #1772 opened. Self-reviewed during authorship; two real races found and fixed (route effect overriding reader scroll position when pdf.js reports its page count; in-flight scroll gate armed a frame too late). #279 closed by tests/ui-document-canvas.spec.ts; #283 batch-route deferral recorded with the measurement that should decide it; #290 added for the OffscreenCanvas number; #291 added for a pre-existing root-only pr-handoff-stop failure; #252 updated with measured bundle headroom (+9.4% of 10%). Crop-to-page overlay out of scope by design. | verify:pr-local (1 pre-existing root-only failure: pr-handoff-stop, reproduced on origin/main worktree; 5839 passed), build OK, eval:rag:offline 36 golden cases 574 tests, check:bundle-budget within tolerance, check:playwright-pr-shards 23 specs, canvas gate skip-with-reason verified locally and fail-closed verified with CI=1. Browser gates unrunnable here (Chromium 141 vs pdfjs-dist 6 needing 151) - delegated. | | 2026-08-09 | claude/document-viewer-phase-3-bj5k5v | 2cd72f111414935d4028a19932992c4ab475dcea | document viewer Phase 3 review-and-fix | PR #1772 deep review+fix: merged origin/main (renumber OffscreenCanvas #290->#294, drop dup #291=#284); fixed key-repeat pageRef, maxObservedCanvasPixels budget, rail remount-via-key, reserved-slot shadow-inset for DS ratchet; dispositioned Bugbot/Codex/Copilot canvas+rotation as already fixed at 8397aeb; #252 tip-only wording; #294 aggregate budgets; CodeRabbit ledger nit deferred to this superseding row | verify:cheap: Test Files 545 passed (545), Tests 5856 passed \| 4 skipped (5860); verify:pr-local completed check:runtime check:installed-lock-parity format:changed sitemap:check docs:check-index docs:check-inventory docs:check-scripts docs:check-links check:branch-review-ledger check:outstanding-issues lint typecheck test build eval:rag:offline failed:(none); focused vitest 47 passed (keyboard+rail+budget+virtualization); design-system-contract legacy shadow aliases 220; merge-tree vs origin/main exit 0; Production UI delegated (pdfjs Map.getOrInsertComputed needs Chromium 151) | | 2026-08-09 | cursor/differentials-diagnosis-links-9f18 | f784e81bcc0b53ef76b3da07a8e81f96d9bf0c71 | pr-1768 unblock | merged origin/main onto ba590f9; merge-tree clean; DIRTY mergeability cleared; push tip follows amend with this ledger | merge-tree clean; threads resolved; auto-merge was armed | +| 2026-08-09 | claude/document-viewer-phase-3-bj5k5v | 156db63f1b60f09791e426b043ea90d427b789ab | post-#1772 test simplification: replace the viewer perf source-text grep with behavioural coverage; de-literalise rail window and keyboard label assertions | PR #1777 opened. Self-review of #1772's own tests against an excessive-strictness challenge. Finding: the client-performance-boundaries grep for resolveLiveCanvasWindow / resolveRenderAheadPages / liveCanvasLimit / requestIdleCallback was not merely brittle, it was INEFFECTIVE - replacing the budget call with a hardcoded 3 leaves every identifier in the file, so it stayed green while the viewer retained three full-zoom canvases (measured both ways). Replaced by a DOM case that binds the budget (VIEWER_MAX_ZOOM at dpr 3 gives ~16.8M backing px against the 24M budget, window collapses to 1) and fails on exactly that substitution. Also exported RAIL_IMAGE_WINDOW so the rail test derives its counts (verified by tuning 6->8: all 7 still pass), and relaxed the keyboard aria-label assertions from exact prose to the key names. Pre-existing greps for disableAutoFetch / canvas.width = 0 / pageToCleanup left alone deliberately - two are now redundant but they are another author's guard. | verify:pr-local (1 pre-existing root-only failure: pr-handoff-stop #291; 5872 passed), build OK 80s + client bundle secret check, eval:rag:offline 36 golden cases / 574 tests, lint + typecheck clean. Sabotage-verified in both directions. Browser gates unrunnable here (#279) - unchanged by this diff. |