diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index 67ae5be6d9..3f5fdc0ceb 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -761,6 +761,7 @@ Records before 2026-07-28 were written by hand and had drifted: 146 lines carrie | 2026-08-08 | claude/document-viewer-optimization-tu8tnj | 2359e158cb7bca5954e9c5ee84ca0766964ad901 | PR #1741 document-viewer phone/PWA review-and-fix | supersede: fixed Production UI phone Zoom/section-trigger; handlePdfLoadSuccess clamp; prior P1/P2 fixes retained; merge-tree clean | prior verify:cheap+pr-local green; ui-smoke selectors fixed for overflow Zoom + revealPhoneHeaderControl; no provider gates | | 2026-08-08 | claude/mode-routing-search-pages-jabe17 | 6d1099b479358caa05c92f236848117feb920d4e | shared-home mode-routed search navigation | no high-confidence P0-P2 PR-introduced defects; prior bug-hunt P1/P2s appear fixed on tip; residual: prescribing submit-from-shared-home URL omits run=1 (pre-existing path), seed effect untested behaviourally, no browser/UI proof this pass | vitest app-modes+search-route-ownership+audit-navigation+pwa-manifest 61 pass; static read of focus files vs origin/main; ledger:lookup NOT REVIEWED; no provider/UI | | 2026-08-08 | cursor/safety-plan-phone-safe-area-624a (PR #1711) | ad1b1f5db24ed68ee4c0d5963620e4562829884e | heavy review-and-fix PR #1711 | fixed CodeRabbit sm:py guard parity; late-synced #1720 behind-but-clean; no P0/P1; Bugbot none; threads cleared; merge-tree clean; required CI green on 78c14205 pre-sync | vitest safety-plan+standalone 18p; verify:cheap 523/5582; verify:pr-local format+lint+typecheck+test+build+rag-fixtures; Production UI critical+(1)(2)(3)+PR required SUCCESS on 78c14205; no provider gates | +| 2026-08-09 | claude/inpage-nav-pr-2-6d32f9 | 249526988ea3d65c54e69ee7ca05e514bff50ed8 | in-page-nav PR 2: convert six information routes onto InPageNavHeader; delete the shell-owned pill rail | Shipped as PR #1766. Seven components converted; actions API widened for Server Components; two DSM routes' missing anchors wired; rail and section kind removed; new per-route rendered-DOM section contract added. | verify:pr-local 527/530 files 5708 tests; verify:cheap 529/530 5710 tests; verify:phone-chrome escalated to full Chromium 398 passed then 13/13 updated specs pass; typecheck + prettier clean; Playwright production build compiled. Residual failures proven pre-existing on pristine base 9ab3b73a (issues #285, pr-handoff-stop env). | | 2026-08-08 | dependabot/npm_and_yarn/js-yaml-4.3.1 | 072b83f79a70037a04a8412844c041db43c9ce48 | PR #1668 unblock | synced main; merge-tree clean; required CI was green on prior tip e9516021; js-yaml 4.3.1 + nanoid 3.3.18 preserved; no unresolved threads; CI re-run after sync | pre-sync PR required pass; Production UI skipped (deps); post-sync pending | | 2026-08-08 | claude/planning-build-intelligence-9ot0nm | 1ebc84bb288b516bb322c09cde2889e981d302a4 | AGENTS.md reasoning-effort calibration section (docs-only) | Authored and handed off as PR #1730; docs-only, pr-policy classifier returns clinicalRisk/operationalRisk/ragRanking false | prettier --check . (repo-wide, pass); docs:check-links (1665 refs resolve, pass); pr-policy classifyPullRequestFiles(AGENTS.md) | | 2026-08-08 | claude/planning-build-intelligence-9ot0nm | 2b0ad7d41d841c13515f10de7c41e449470dfa78 | pr-1730 review-and-fix | Deep review + Bugbot: no P0/P1; fixed 2 scoped P2 clarity risks (version-bump under-planning; live-state vs provider boundary). Residual: OPENAI_*_REASONING_EFFORT vocab overlap. Merge-tree clean; required CI was green pre-push. | prettier --check AGENTS.md; docs:check-links (1667); verify:pr-local (docs route pass); verify:cheap (524 files / 5607 tests pass); pr-policy classify clinical/operational/rag false; Bugbot no P0-P2 | @@ -824,20 +825,20 @@ Records before 2026-07-28 were written by hand and had drifted: 146 lines carrie | 2026-08-07 | cursor/site-testing-speed-08c1 | 91bac89827ae2f4f0e59aeed7de6344fe8779a95 | PR #1686 Autopilot+Bugbot review-and-fix: conflicts, threads, Static PR checks, CI/testing selection | fixed: merged origin/main (outstanding-issues #167/#255 archive + #256 keep); removed unused pathToFileURL; added ui-forms-section-nav to PR UI shards (21 specs); no unresolved threads; Bugbot unavailable (usage limit). Local: eslint file max-warnings0, vitest 36/36 focused, shard --validate OK, check:outstanding-issues OK. verify:cheap/pr-local blocked by foreign worktree heavy lock (PID 26228). | eslint scripts/playwright-pr-shards.mjs --max-warnings 0; vitest 36 passed; playwright-pr-shards --validate 21; check:outstanding-issues; verify:cheap/pr-local lock-blocked | | 2026-08-08 | cursor/more-modes-popup-2f4b | 04e6a80653c3ccf103577f6c3886b162498621ed | sidebar more-modes sheet popup | pass | focused-pw tablet rail; test:focused ClinicalSidebar; favourites+therapy wiring; verify:pr-local stages+build+rag-fixtures | | 2026-08-08 | cursor/more-modes-popup-2f4b | bea4b0c09b74368cf6d63e944bac9c1eec6b0c93 | sidebar more-modes sheet popup | pass | focused-pw tablet rail; test:focused ClinicalSidebar; favourites+therapy wiring; verify:pr-local stages+build+rag-fixtures | -| 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 | claude/m2-ds-gates-blocking | 8ed66a0570c95c2cc8597364467e67966b04854d | M2 design-system gates: #264 + gate 4 of #265 | ready-to-merge; gate 2 enumeration deliberately reverted as non-deterministic (#289) | ds-contract PASS (colour-only 4, numerals 2, inversions 0); mutation-verified x4; lint 0; tsc 0 errors; icon+type scale PASS; focused vitest 126p/3 files; verify:cheap 5777p with 10 pre-existing failures proven identical on pristine origin-main; format:check clean | -| 2026-08-09 | claude/inpage-nav-pr-2-6d32f9 | 249526988ea3d65c54e69ee7ca05e514bff50ed8 | in-page-nav PR 2: convert six information routes onto InPageNavHeader; delete the shell-owned pill rail | Shipped as PR #1766. Seven components converted; actions API widened for Server Components; two DSM routes' missing anchors wired; rail and section kind removed; new per-route rendered-DOM section contract added. | verify:pr-local 527/530 files 5708 tests; verify:cheap 529/530 5710 tests; verify:phone-chrome escalated to full Chromium 398 passed then 13/13 updated specs pass; typecheck + prettier clean; Playwright production build compiled. Residual failures proven pre-existing on pristine base 9ab3b73a (issues #285, pr-handoff-stop env). | | 2026-08-09 | claude/document-viewer-optimization-tu8tnj | b8c94dff2345a7d50c7bbce0c9c344740e6b92b1 | docs: document-viewer Phase 3 handover brief (PR #1765) | Docs-only. Adds docs/plans/document-viewer-phase3-handover.md scoping Phase 3 to all capabilities except crop-to-page overlay (bbox absent from DocumentDetailImage; plumbing crosses src/lib/**document** and forces a governance preflight). Corrects ledger #279: measured playwright@1.62.1 expects Chromium 151.0.7922.34, container ships 141.0.7390.37, CI runs HeadlessChrome/151.0.0.0, and pdfjs-dist 6.2.108 needs Map.getOrInsertComputed which ships in 151 not 141 - so the raster failure is container-only and neither proposed remedy (bump Playwright / pin pdfjs down) is needed. Cited #286 for the authorizationHeader casing trap after initially writing #285. | verify:pr-local all ten gates completed, none failed; docs:check-links 1688 references resolve; line refs re-verified against main 8db1e53 | | 2026-08-09 | claude/document-viewer-optimization-tu8tnj | 5a0d6be02bc92fa2615d2141b338ec8f7c1143b1 | docs: document-viewer Phase 3 handover brief (PR #1765) | Supersedes the earlier row, whose 'all ten gates completed' wording could read as all executable checks having run. Correct scope: verify:pr-local ran the ten gates APPLICABLE to docs-only changes (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); the risk router SKIPPED lint, typecheck, the full unit suite, RAG fixture validation, and build as recognised low-risk documentation scope. Also records the merge resolution: duplicate #286 (main's in-page-nav series vs this branch's authorizationHeader row) resolved by renumbering the branch row to #289, next-id 290, after the auto-merge silently dropped that detail row rather than conflicting. Review findings addressed: governance preflight now required by behaviour per AGENTS.md:257 rather than inferred from pr-policy path classification; API-route scope contradiction resolved; signed-URL warning corrected to state both identity bugs are already fixed on main with regression coverage. | verify:pr-local ten docs-scope gates passed, none failed; check:outstanding-issues 287 rows unique ids next-id=290 no ids deleted; ledger:dedupe 771 unique rows; git merge-tree vs origin/main exit 0; viewer line refs re-verified against 50ef12e | +| 2026-08-09 | claude/breadcrumb-header-mockups-cei6lw | 8effa5abe77e9008fb12f6ff996a51aa6d406ab5 | mockups: breadcrumb header study (3 directions) + sitemap/README | self-reviewed; design-scratch only, no production surface changed | typecheck, eslint(changed), prettier --check, sitemap:check, vitest(site-map/mockup-boundary/env-mockups/docs-inventory/route-reachability) 23 passed | +| 2026-08-09 | claude/breadcrumb-header-mockups-cei6lw | ca5e4e7ae9b53af49b362ad11ca988fdd1c3a9d0 | breadcrumb header shipped: InPageNavHeader breadcrumb shape + factsheet detail adoption | self-reviewed; browser-verified at 390/700/834/1280; phone-chrome gate blocked by #255 playwright drift | typecheck, lint, test (5806 passed, 1 pre-existing unrelated fail), build, check:bundle-budget, check:design-system-contract, format:changed, sitemap:check, docs+ledger checks | +| 2026-08-09 | claude/m2-ds-gates-blocking | 8ed66a0570c95c2cc8597364467e67966b04854d | M2 design-system gates: #264 + gate 4 of #265 | ready-to-merge; gate 2 enumeration deliberately reverted as non-deterministic (#289) | ds-contract PASS (colour-only 4, numerals 2, inversions 0); mutation-verified x4; lint 0; tsc 0 errors; icon+type scale PASS; focused vitest 126p/3 files; verify:cheap 5777p with 10 pre-existing failures proven identical on pristine origin-main; format:check clean | | 2026-08-09 | claude/m2-ds-gates-blocking | d204c6f7c84a5e7de3f28061121fda68e7d28670 | M2 design-system gates: #264 + gate 4 of #265 | ready-to-merge; supersedes the 8ed66a05 row — the tap-floor defect renumbered #289 to #291 after main claimed #289/#290, and main was merged in | post-merge ds-contract PASS (colour-only 4, numerals 2, inversions 0) against main's new #1765/#1766 component code; outstanding-issues guard PASS 289 rows next-id=292; ledger guard PASS 774 rows; format clean | | 2026-08-09 | claude/m2-ds-gates-blocking | e8447b042088998d34af750df72a946b1f074b97 | M2 design-system gates: #264 + gate 4 of #265 | ready-to-merge; supersedes the d204c6f7 row — seven review findings fixed, copilot-swe-agent commits merged keeping the safer numeral classifier, tap-floor defect renumbered #291 to #293 after main claimed #291/#292 | ds-contract PASS (colour-only 4, numerals 2, inversions 0); 11 reviewer cases probe-verified; mutation-verified incl. opacity and arbitrary-filter forms; lint 0; tsc 0 errors; format clean; outstanding-issues guard PASS 291 rows next-id=294 no ids deleted; ledger guard PASS 775 rows | -| 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 | 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 | 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/disabled-button-accessibility-piclvr | 722abdb780c715c0a89df268ed48f6c741ffd569 | disabled-placeholder buttons -> aria-disabled + inert handler (25 sites, 13 components); controlDisabled/therapy recipe aria-disabled styling; require-button-wiring redundantDisabledPair gate; wiring-conventions contract rewrite (settles #291) | authored — PR #1778 opened | lint (uncached, exit 0); typecheck; test 5878 passed/1 pre-existing root-env failure in pr-handoff-stop; build; check:rag:fixtures 36 golden cases; prettier --check clean; verify:ui not run (no browser in container) | -| 2026-08-09 | claude/breadcrumb-header-mockups-cei6lw | 8effa5abe77e9008fb12f6ff996a51aa6d406ab5 | mockups: breadcrumb header study (3 directions) + sitemap/README | self-reviewed; design-scratch only, no production surface changed | typecheck, eslint(changed), prettier --check, sitemap:check, vitest(site-map/mockup-boundary/env-mockups/docs-inventory/route-reachability) 23 passed | -| 2026-08-09 | claude/breadcrumb-header-mockups-cei6lw | ca5e4e7ae9b53af49b362ad11ca988fdd1c3a9d0 | breadcrumb header shipped: InPageNavHeader breadcrumb shape + factsheet detail adoption | self-reviewed; browser-verified at 390/700/834/1280; phone-chrome gate blocked by #255 playwright drift | typecheck, lint, test (5806 passed, 1 pre-existing unrelated fail), build, check:bundle-budget, check:design-system-contract, format:changed, sitemap:check, docs+ledger checks | +| 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 | | 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 | | 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/disabled-button-accessibility-piclvr | 722abdb780c715c0a89df268ed48f6c741ffd569 | disabled-placeholder buttons -> aria-disabled + inert handler (25 sites, 13 components); controlDisabled/therapy recipe aria-disabled styling; require-button-wiring redundantDisabledPair gate; wiring-conventions contract rewrite (settles #291) | authored — PR #1778 opened | lint (uncached, exit 0); typecheck; test 5878 passed/1 pre-existing root-env failure in pr-handoff-stop; build; check:rag:fixtures 36 golden cases; prettier --check clean; verify:ui not run (no browser in container) | diff --git a/docs/search-chrome-behaviour.md b/docs/search-chrome-behaviour.md index 6e4867b594..1328a95cc6 100644 --- a/docs/search-chrome-behaviour.md +++ b/docs/search-chrome-behaviour.md @@ -87,10 +87,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.'); + }); +});