From 1395d533cb13eadc705e47f76aa9f39a7a11c058 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 07:13:45 +0000 Subject: [PATCH 1/2] Record DocumentViewer nav non-adoption and merge the duplicated anchor predicate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /issues #288 asked whether DocumentViewer should converge onto the shared InPageNavHeader template. Evaluated and declined on the merits; the decision and its blocking reasons are now recorded in docs/search-chrome-behaviour.md so it reads as closed rather than as a migration waiting for an owner. The premise that four modules are duplicated does not hold. Only the ~70-line header row and its sheet-state plumbing are duplicated: the scroll spy, segment track, section list, jump, and anchor measurement are already single implementations that both paths import. Adoption would instead require InPageNavHeader to grow escape hatches for its one non-conforming consumer — controlled sheet state (DocumentViewer observes it for chrome hold, opens it from a second trigger in the composer dock, and blurs the composer first), sheet content slots (density toggle, portal/headerLeading), and an opt-out of its own chrome metrics (different scope, extra property, headerHidden consumed by the desktop rail) — on a component seven routes already mount. It would also break the DocumentViewer literals pinned by header-scroll-hide-contract and document-section-nav-contract, which are the contract this change exists to preserve. Converged what genuinely was duplicated: the visible-element predicate existed twice, character for character, in use-section-spy.ts and use-page-section-weights.ts. It is now one exported resolveVisibleElement(ids), with resolveSectionElement(id) as the alias-aware wrapper over it. Semantics are unchanged. useResolvedPageSections keeps its third, deliberately different predicate; converging that one would change resolution behaviour on seven live routes with no test covering it. Adds tests/in-page-nav-document-viewer-convergence.dom.test.tsx: behavioural cover for the merged predicate (alias fallback, zero-rect skip, absent section) plus static guards that the in-page-nav path keeps importing the shared spy, track, list and metrics rather than forking them, so the divergence cannot widen from two header rows to two of everything. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Q4VxLcvHim6btP5Zu2hFAo --- docs/search-chrome-behaviour.md | 65 ++++++- .../document-viewer/use-section-spy.ts | 26 ++- .../in-page-nav/use-page-section-weights.ts | 26 +-- ...v-document-viewer-convergence.dom.test.tsx | 166 ++++++++++++++++++ 4 files changed, 250 insertions(+), 33 deletions(-) create mode 100644 tests/in-page-nav-document-viewer-convergence.dom.test.tsx diff --git a/docs/search-chrome-behaviour.md b/docs/search-chrome-behaviour.md index 01fbe3f131..d9dcb98fb6 100644 --- a/docs/search-chrome-behaviour.md +++ b/docs/search-chrome-behaviour.md @@ -86,10 +86,67 @@ owner: Touch one of those files for an unrelated reason and leave its section table where it is. Converting a route onto `InPageNavHeader` for the first time is the moment the rule applies. -`src/components/DocumentViewer.tsx` still carries its own copy of the header rather than the -shared one: it owns the page `

`, uses the `edge-glass-header` treatment, and is pinned by -visual baselines, so converging it is a separate change. It remains the visual reference, and -detailed DocumentViewer rules remain invariant 22 — but new work mounts `InPageNavHeader`. +### DocumentViewer keeps its own header — decided, not pending + +`src/components/DocumentViewer.tsx` renders its own header row rather than mounting +`InPageNavHeader`. This was evaluated as a convergence task (`/issues #288`) and **declined on +the merits**. It is not a migration waiting for an owner: do not re-open it without new facts +against the four reasons below. New work on any _other_ page still mounts `InPageNavHeader`, +and the viewer remains the visual reference the template was drawn from. + +**What is already shared, and what is not.** The duplication is the ~70-line header row and its +sheet-state plumbing — nothing else. The behaviour underneath is one implementation that both +paths import today: + +| Concern | Single implementation | +| -------------------------------------- | --------------------------------------------------------- | +| Segment track, section list, jump | `document-viewer/section-nav.tsx` | +| Scroll spy, visible-element resolution | `document-viewer/use-section-spy.ts` | +| Anchor-offset / collapse measurement | `sticky-chrome-metrics.ts` (both hooks are thin bindings) | + +So "fix a bug in one and the other keeps it" does not hold for the spy, the track, the list, the +jump, or the anchor measurement — a fix to any of those already reaches every route. The residual +risk is confined to header markup. + +**Why the row itself is not converged.** Each of these would require `InPageNavHeader` to grow an +escape hatch for its one non-conforming consumer, on a component seven routes already mount: + +1. **Sheet state is observed and externally driven.** `DocumentViewer.tsx` feeds + `mobileActionsOpen || sectionSheetOpen` to `useDocumentViewerChromeScroll` so both chrome + edges stay open under a sheet; a **second** actions trigger lives in the phone composer dock; + and `openSectionSheet` blurs the source-search input first, because the viewer owns a composer + whose focus pins hide-on-scroll. `InPageNavHeader` owns its sheet state privately and + deliberately — pathname-keyed, so the four Server Component adopters need no client state. + Adoption means inverting that to controlled props for one caller. +2. **Both sheets carry viewer-only content.** The section sheet mounts + `DocumentViewDensityToggle` (persisted, defaults condensed); the actions sheet passes + `portal`, `contentClassName` and `headerLeading`, and is gated on `readyDocument`. The shared + sheets take no content slot and pass none of those through. +3. **Chrome metrics differ in scope, property set and return value.** The viewer scopes to its + own `
`, publishes a third property (`--document-collapse-height`), and consumes + `headerHidden` for the desktop rail. `InPageNavHeader` calls `useInPageChromeMetrics()` + itself — document-scoped, two properties, result discarded — with no way to opt out or read it. +4. **It breaks the contract tests that exist to protect this chrome.** + `tests/header-scroll-hide-contract.test.ts` requires `` and + `data-document-sticky-header` in `DocumentViewer.tsx`, and + `tests/document-section-nav-contract.test.ts` requires `data-testid="document-section-trigger"` + there; all three move into the shared header on adoption. Keeping them green would mean + threading literal strings through props purely to satisfy source-text greps. + +**Anchor aliasing: both models stand, and they are not rivals.** The viewer keeps +`sectionAnchorAliases` inside `resolveSectionElement`; information pages keep +`PageSection.targetIds`. The viewer's sections are derived from the indexed payload at render +time by `buildDocumentSectionIndex`, so there is no declaration site on which to hang `targetIds`; +pages resolve their copies _before_ the spy runs, which is what keeps `useDocumentSectionSpy` +generic. Do not "unify" these by moving the alias map into the page model. + +**What was converged instead.** The visible-element predicate had drifted into two identical +copies — one in `use-section-spy.ts`, one in `use-page-section-weights.ts`. It is now the single +exported `resolveVisibleElement(ids)`, with `resolveSectionElement(id)` as the alias-aware +wrapper over it. `useResolvedPageSections` still carries a third, deliberately _different_ +predicate (`getClientRects` + computed `display` rather than rect size); converging that one +would change resolution behaviour on seven live routes and is not covered by any current test, +so it was left alone. **Adopted so far:** `/differentials/diagnoses/[slug]`, `/services/[slug]`, `/forms/[slug]`, `/specifiers/[slug]` (record and catalogue reference), `/formulation/[slug]`, diff --git a/src/components/document-viewer/use-section-spy.ts b/src/components/document-viewer/use-section-spy.ts index 92b17615c7..0617bc59eb 100644 --- a/src/components/document-viewer/use-section-spy.ts +++ b/src/components/document-viewer/use-section-spy.ts @@ -13,18 +13,24 @@ const sectionAnchorAliases: Record = { }; /** - * The section's element as currently rendered, or null when the section is on - * the page but not displayed at this breakpoint. + * The first of `ids` that is currently rendered, or null when none of them is + * displayed at this breakpoint. * * A `display: none` element still resolves by id and reports a zero rect, which * reads as "at the very top of the viewport" — enough to make a hidden phone * panel win the active-section race on desktop. Size is the only reliable test. + * + * Exported as the candidate-list primitive so a caller that already knows a + * section's breakpoint copies supplies them directly. `usePageSectionWeights` + * is that caller: page sections declare their copies as `PageSection.targetIds` + * rather than through the viewer's alias map below, and the two had drifted into + * separate copies of this same loop. */ -export function resolveSectionElement(id: string): HTMLElement | null { +export function resolveVisibleElement(ids: readonly string[]): HTMLElement | null { if (typeof window === "undefined") return null; - for (const candidate of [id, ...(sectionAnchorAliases[id] ?? [])]) { - const element = window.document.getElementById(candidate); + for (const id of ids) { + const element = window.document.getElementById(id); if (!element) continue; const rect = element.getBoundingClientRect(); if (rect.width > 0 || rect.height > 0) return element; @@ -33,6 +39,16 @@ export function resolveSectionElement(id: string): HTMLElement | null { return null; } +/** + * The document section's element as currently rendered, resolved through the + * viewer's alias map: the viewer builds its sections from the indexed payload + * (`buildDocumentSectionIndex`), so there is no declaration site on which to + * hang per-section `targetIds` the way an information page has. + */ +export function resolveSectionElement(id: string): HTMLElement | null { + return resolveVisibleElement([id, ...(sectionAnchorAliases[id] ?? [])]); +} + /** * Tracks which document section the reader is currently in. * diff --git a/src/components/in-page-nav/use-page-section-weights.ts b/src/components/in-page-nav/use-page-section-weights.ts index de24e0f7a5..210a76aa1e 100644 --- a/src/components/in-page-nav/use-page-section-weights.ts +++ b/src/components/in-page-nav/use-page-section-weights.ts @@ -2,6 +2,7 @@ import { useEffect, useState } from "react"; +import { resolveVisibleElement } from "@/components/document-viewer/use-section-spy"; import { sectionTargetIds, type PageSection } from "@/components/in-page-nav/page-section-index"; /** @@ -11,29 +12,6 @@ import { sectionTargetIds, type PageSection } from "@/components/in-page-nav/pag */ const shortestSegmentShareOfTallest = 0.12; -/** - * The section's element as currently rendered, or null when it is on the page - * but not displayed at this breakpoint. - * - * Mirrors `resolveSectionElement` in `use-section-spy.ts`, including why size is - * the test: a `display: none` element still resolves by id and reports a zero - * rect. It is separate rather than shared because that one carries the document - * viewer's hardcoded alias map, while page sections declare their own breakpoint - * copies through `targetIds`. - */ -function resolveElement(ids: readonly string[]): HTMLElement | null { - if (typeof window === "undefined") return null; - - for (const id of ids) { - const element = window.document.getElementById(id); - if (!element) continue; - const rect = element.getBoundingClientRect(); - if (rect.width > 0 || rect.height > 0) return element; - } - - return null; -} - /** * `id>target,target` per section, joined by `|`. Sections are rebuilt on most * renders, so the array identity is never a usable effect dependency — this is @@ -103,7 +81,7 @@ export function usePageSectionWeights(sections: readonly PageSection[]): Readonl const measure = () => { frame = 0; const heights = plan.map(({ id, targets }) => { - const element = resolveElement(targets.length ? targets : [id]); + const element = resolveVisibleElement(targets.length ? targets : [id]); // Sections mount after the effect (record data arriving) and swap copies // across breakpoints, so the observer's targets are picked up here rather // than once at setup. Otherwise a late section's own resizes — an image diff --git a/tests/in-page-nav-document-viewer-convergence.dom.test.tsx b/tests/in-page-nav-document-viewer-convergence.dom.test.tsx new file mode 100644 index 0000000000..602912e002 --- /dev/null +++ b/tests/in-page-nav-document-viewer-convergence.dom.test.tsx @@ -0,0 +1,166 @@ +import { readFileSync } from "node:fs"; +import { join } from "node:path"; + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { resolveSectionElement, resolveVisibleElement } from "@/components/document-viewer/use-section-spy"; + +/** + * Keeps the document viewer and the shared in-page navigation template from + * forking into two implementations of one contract (`/issues #288`). + * + * `DocumentViewer` deliberately does **not** mount `InPageNavHeader` — that was + * evaluated and declined, with the four blocking reasons recorded in + * `docs/search-chrome-behaviour.md` ("DocumentViewer keeps its own header"). + * What makes the split affordable is that only the header row is duplicated: + * the scroll spy, the segment track, the section list, the jump and the anchor + * measurement are single implementations that both paths import. This file + * pins that arrangement, so the divergence cannot quietly widen from "two header + * rows" to "two of everything" — and covers the one primitive the convergence + * pass actually merged. + */ + +/** + * Repo-root relative, not `import.meta.url`: the node suite's usual idiom, but + * `import.meta.url` is not populated for this file's jsdom project, and vitest + * runs every project from the repository root. + */ +const read = (relativePath: string) => readFileSync(join(process.cwd(), relativePath), "utf8"); + +const inPageNavHeaderSource = read("src/components/in-page-nav/in-page-nav-header.tsx"); +const inPageSectionNavSource = read("src/components/in-page-nav/use-in-page-section-nav.ts"); +const pageSectionWeightsSource = read("src/components/in-page-nav/use-page-section-weights.ts"); +const inPageChromeMetricsSource = read("src/components/in-page-nav/use-in-page-chrome-metrics.ts"); +const documentChromeMetricsSource = read("src/components/document-viewer/use-document-chrome-metrics.ts"); +const behaviourDocSource = read("docs/search-chrome-behaviour.md"); + +/** + * jsdom gives every element a zero rect, which is exactly the "present but not + * displayed" state the predicate has to reject — so a visible element has to be + * stubbed explicitly. + */ +function mountAnchor(id: string, { visible }: { visible: boolean }) { + const element = document.createElement("div"); + element.id = id; + document.body.append(element); + + if (visible) { + vi.spyOn(element, "getBoundingClientRect").mockReturnValue({ + x: 0, + y: 0, + top: 0, + left: 0, + right: 320, + bottom: 40, + width: 320, + height: 40, + toJSON: () => ({}), + }); + } + + return element; +} + +beforeEach(() => { + document.body.innerHTML = ""; +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe("resolveVisibleElement", () => { + it("returns the first candidate that is actually displayed", () => { + const first = mountAnchor("section-phone", { visible: true }); + mountAnchor("section-desktop", { visible: true }); + + expect(resolveVisibleElement(["section-phone", "section-desktop"])).toBe(first); + }); + + it("skips a candidate that resolves by id but reports a zero rect", () => { + // The whole reason size is the test: a `display: none` copy still resolves + // by id and reports top 0, which reads as "at the very top of the viewport" + // and would win the active-section race against the copy on screen. + mountAnchor("section-phone", { visible: false }); + const displayed = mountAnchor("section-desktop", { visible: true }); + + expect(resolveVisibleElement(["section-phone", "section-desktop"])).toBe(displayed); + }); + + it("returns null when no candidate is rendered at all", () => { + expect(resolveVisibleElement(["absent-a", "absent-b"])).toBeNull(); + }); +}); + +describe("resolveSectionElement", () => { + it("resolves a document section through its own anchor when that copy is displayed", () => { + const primary = mountAnchor("source-evidence", { visible: true }); + mountAnchor("source-evidence-rail", { visible: true }); + + expect(resolveSectionElement("source-evidence")).toBe(primary); + }); + + it("falls back to the viewer's aliased copy when the primary one is hidden", () => { + // Pinned evidence renders twice — a phone copy and a desktop rail copy — + // and only one is displayed at a time. The alias map is what lets the spy + // follow it across the breakpoint. + mountAnchor("source-evidence", { visible: false }); + const rail = mountAnchor("source-evidence-rail", { visible: true }); + + expect(resolveSectionElement("source-evidence")).toBe(rail); + }); + + it("reports nothing for a section with no rendered copy", () => { + mountAnchor("source-evidence", { visible: false }); + mountAnchor("source-evidence-rail", { visible: false }); + + expect(resolveSectionElement("source-evidence")).toBeNull(); + }); +}); + +describe("in-page navigation reuses the document-viewer substrate", () => { + it("renders the document viewer's own track and list rather than copies", () => { + expect(inPageNavHeaderSource).toContain('from "@/components/document-viewer/section-nav"'); + expect(inPageNavHeaderSource).toContain("DocumentSectionList"); + expect(inPageNavHeaderSource).toContain("DocumentSectionTrack"); + }); + + it("drives position and jumps from the document viewer's own spy", () => { + expect(inPageSectionNavSource).toContain( + 'import { jumpToDocumentSection } from "@/components/document-viewer/section-nav"', + ); + expect(inPageSectionNavSource).toContain( + 'import { useDocumentSectionSpy } from "@/components/document-viewer/use-section-spy"', + ); + }); + + it("resolves breakpoint copies through the one shared predicate", () => { + // This loop existed twice, character for character, until #288 merged it. + // Its signature is the rect test — the file has no other reason to measure + // one, so a `getBoundingClientRect` reappearing here is the fork growing + // back. (`getElementById` is not the tell: the hook legitimately resolves + // `#main-content` as its MutationObserver root.) + expect(pageSectionWeightsSource).toContain( + 'import { resolveVisibleElement } from "@/components/document-viewer/use-section-spy"', + ); + expect(pageSectionWeightsSource).not.toContain("getBoundingClientRect"); + }); + + it("keeps both chrome-metric hooks as bindings of one measurement", () => { + for (const source of [inPageChromeMetricsSource, documentChromeMetricsSource]) { + expect(source).toContain('from "@/components/sticky-chrome-metrics"'); + expect(source).toContain("useStickyChromeMetrics({"); + } + }); +}); + +describe("the DocumentViewer non-adoption decision", () => { + it("stays recorded as decided rather than pending", () => { + // An "adopt it later" note is what turns a closed decision back into a + // migration someone re-opens without the reasons in front of them. + // Prose is hard-wrapped, so assert on fragments that sit within one line. + expect(behaviourDocSource).toContain("### DocumentViewer keeps its own header — decided, not pending"); + expect(behaviourDocSource).toContain("do not re-open it without new facts"); + expect(behaviourDocSource).toContain('Do not "unify" these by moving the alias map into the page model.'); + }); +}); From 51bb5e180b5dad2b5def7b490210956ace7b1003 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 07:18:02 +0000 Subject: [PATCH 2/2] Record the #288 convergence handoff in the branch review ledger Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Q4VxLcvHim6btP5Zu2hFAo --- 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..5b9881b23c 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/documentviewer-nav-convergence-oddhjx | 1395d533cb13eadc705e47f76aa9f39a7a11c058 | DocumentViewer / in-page-nav convergence (#288): non-adoption decision recorded in docs/search-chrome-behaviour.md; merged duplicated visible-element predicate into resolveVisibleElement; new convergence guard test | Converged what was duplicated; DocumentReviewer header adoption declined on the merits with four blocking reasons recorded. No contract test edited. | verify:pr-local (546/547 files, 5883 tests pass; sole failure tests/pr-handoff-stop.test.ts reproduced on pristine origin/main), verify:phone-chrome (contracts 123 pass; focused Chromium 7 pass), contract set 12 files/151 tests pass, lint, typecheck, format |