diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index 73740082f3..19b3218287 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -837,5 +837,6 @@ Records before 2026-07-28 were written by hand and had drifted: 146 lines carrie | 2026-08-09 | cursor/differentials-diagnosis-links-9f18 | 0e77e7bc0842bef4ffc045c6dbea4626152490d1 | differentials diagnosis term links | implemented exact+alias termLinks chips on diagnosis+presentation pages; vitest 58/58; verify:pr-local green | vitest differential-diagnosis-links+detail+section-nav+route; verify:pr-local; ensure spot-check | | 2026-08-09 | cursor/differentials-diagnosis-links-9f18 | 0daa9e2f9fc84e879fd661da94203568309234a6 | PR #1768 Autopilot+Bugbot review-and-fix | Merged origin/main (DIRTY was ledger+detail-page staleness; merge-tree clean). Fixed SEGMENT_SPLIT to spaced-slash only so Delirium / medical psychosis links while alcohol/benzo, DVT/PE, food/fluid stay intact. Dispositioned: Copilot termLinks ??{} + Fragment key already fixed; CodeRabbit clean-keys moot (visibleSectionItems already cleans); CodeRabbit bare-slash split rejected (clinical harm). No Bugbot findings. Threads cleared on push. Merge left to user. | vitest differential-diagnosis-links+detail+route 49/49; verify:cheap exit 0 (543 files, 5828 passed/4 skipped); verify:pr-local exit 0 (lint/typecheck/test/build/rag-fixtures); merge-tree clean vs origin/main; no provider gates | | 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. | | 2026-08-09 | claude/planning-build-intelligence-9ot0nm | 3df3cb3993f73cda4dbbc4ac7549f84b3c6ea7ed | Node 24.15 engine floor: engines.node, preinstall hook, check:runtime, session-start provisioning, codex-cloud assertion | Authored and handed off as PR #1771; closes #285; operationalRisk true, clinicalRisk/ragRanking false | test 5800 passed/1 pre-existing root-uid failure (pr-handoff-stop, confirmed on stashed clean tree); lint 0; typecheck 0; prettier --check . pass; check:runtime pass; check:codex-cloud pass; check:outstanding-issues pass; preinstall boundary proof 24.13/24.14.9 reject, 24.15/24.19 accept, 25.0.0 reject; contract test mutation-checked red | | 2026-08-09 | pull/1771 | 466ec4216272c31c5f754db213dbdc529583b167 | PR 1771 runtime floor enforcement | P2: Cloud and Desktop setup paths remain major-only; do not merge until range-aware | static review; check:runtime PASS; check:codex-cloud PASS; ledger PASS; outstanding issues PASS; focused Vitest blocked by active Playwright lease | 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;