Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 44 additions & 42 deletions docs/branch-review-ledger.md

Large diffs are not rendered by default.

11 changes: 7 additions & 4 deletions docs/outstanding-issues.md

Large diffs are not rendered by default.

43 changes: 25 additions & 18 deletions docs/plans/document-viewer-redesign-plan.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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.

Expand All @@ -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 (`#294`) |

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
Expand Down Expand Up @@ -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.

---

Expand Down
30 changes: 30 additions & 0 deletions docs/wiring-conventions.md
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,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
Expand Down
4 changes: 2 additions & 2 deletions playwright.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,15 @@ 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/;

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
Expand Down
6 changes: 5 additions & 1 deletion scripts/playwright-pr-shards.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand Down
4 changes: 4 additions & 0 deletions src/components/DocumentViewer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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}
/>
) : (
<NonPdfSourcePreview
Expand Down
17 changes: 16 additions & 1 deletion src/components/clinical-dashboard/signed-image.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand Down
104 changes: 104 additions & 0 deletions src/components/document-viewer/canvas-raster-budget.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
}
Loading
Loading