Record DocumentViewer nav non-adoption and merge the duplicated anchor predicate

[BigSimmo]

Summary

/issues #288 asked whether DocumentViewer should converge onto the shared InPageNavHeader template. This PR answers it: declined on the merits, recorded, plus the one piece of duplication that genuinely was worth merging.

• Records the non-adoption decision in docs/search-chrome-behaviour.md, replacing the previous "converging it is a separate change" note — which read as a migration waiting for an owner — with a closed decision, its four blocking reasons, and what would have to change to re-open it.
• Merges the duplicated visible-element predicate. The same getElementById + rect-size loop existed character for character in document-viewer/use-section-spy.ts and in-page-nav/use-page-section-weights.ts. It is now one exported resolveVisibleElement(ids), with resolveSectionElement(id) as the alias-aware wrapper over it. Semantics unchanged.
• 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.

Why not converge the header

The issue's premise — that DocumentViewer runs its own use-section-spy.ts, use-document-chrome-metrics.ts, section-index.ts and section-nav.tsx — does not hold. Three of those four are already the shared implementation, imported by in-page-nav today:

Concern	Single implementation	Imported by
Segment track, section list, jump	document-viewer/section-nav.tsx	in-page-nav-header.tsx, use-in-page-section-nav.ts
Scroll spy, element resolution	document-viewer/use-section-spy.ts	use-in-page-section-nav.ts, use-page-section-weights.ts
Anchor-offset / collapse measurement	sticky-chrome-metrics.ts	both chrome-metric hooks, as thin bindings

So "fix a bug in one and the other keeps it" already does not apply to the spy, the track, the list, the jump or the anchor measurement. The real duplication is the ~70-line header row and its sheet-state plumbing. Converging that would require InPageNavHeader to grow escape hatches for its one non-conforming consumer, on a component seven routes already mount:

1. Sheet state is observed and externally driven.DocumentViewer feeds mobileActionsOpen || sectionSheetOpen to useDocumentViewerChromeScroll; a second actions trigger lives in the phone composer dock; and openSectionSheet blurs the source-search input first. InPageNavHeader owns its sheet state privately and deliberately (pathname-keyed), so its four Server Component adopters need no client state.
2. Both sheets carry viewer-only content — DocumentViewDensityToggle in the section sheet, portal / contentClassName / headerLeading on the actions sheet. The shared sheets take no content slot.
3. Chrome metrics differ in scope (viewerRootRef vs document), property set (--document-collapse-height) and return value (headerHidden drives the desktop rail). InPageNavHeader calls its metrics hook itself, with no opt-out.
4. It breaks the contract tests the change exists to preserve.header-scroll-hide-contract requires <PhoneHeaderCollapsePortal> and data-document-sticky-header in DocumentViewer.tsx; document-section-nav-contract requires data-testid="document-section-trigger" there. All three move into the shared header on adoption, and keeping them green would mean threading literal strings through props purely to satisfy source-text greps.

The anchor-alias question is settled rather than merged: the viewer keeps sectionAnchorAliases (its sections are derived from the indexed payload at render time, so there is no declaration site for targetIds), pages keep PageSection.targetIds (resolved before the spy, which is what keeps the spy generic).

Verification

• npm run 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. Unit suite: Test Files 1 failed | 546 passed (547), Tests 1 failed | 5883 passed | 4 skipped (5888).
  • The single failure is tests/pr-handoff-stop.test.ts > emits handoff context only when the marker file exists, which is pre-existing and unrelated to this diff — it fails identically on a pristine origin/main worktree in the same container (Test Files 1 failed (1), Tests 1 failed | 10 passed (11)). This diff touches nothing under .claude/.
• npm run verify:phone-chrome — all four stages green. Contracts: Test Files 9 passed (9), Tests 123 passed (123). Focused browser journeys in real Chromium: 7 passed (24.5s), including both document detail header overlay and footer follow … cases. Its selector reported "Focused ownership and journey coverage is sufficient for this page-local or test-infrastructure scope" and did not escalate to full UI.
• Must-stay-green contract set, unchanged, re-run after formatting: Test Files 12 passed (12), Tests 151 passed (151) — covering header-scroll-hide-contract, document-section-nav-contract, document-section-nav.dom, in-page-nav-header.dom, in-page-nav-route-sections.dom, document-viewer-shell.dom, document-view-density.dom, differential-section-nav.dom, mode-nav-contract.
• Production build succeeded as part of the phone-chrome browser stage (full route manifest, then ▲ Next.js 16.2.12 … ✓ Ready).
• UI verification not run: the phone-chrome smart selector explicitly declined to escalate to full verify:ui for this scope, and no rendering, class, or chrome behaviour changed — the focused Chromium journeys above are the browser proof. verify:release, eval:*, check:supabase-project and test:live were not run — provider-backed and out of scope.

Environment note: this container ships Chromium 1194 while Playwright 1.62.1 pins 1234 (/issues #255). Rather than force a mismatched binary, the matching revision 1234 was installed, so the browser evidence above is genuine and not a soft-skip.

Risk and rollout

• Risk: Low. The only executable change is a pure extraction — resolveVisibleElement(ids) contains the identical loop, and resolveSectionElement(id) calls it with the same candidate list it built inline before. It is exercised by the document viewer and by all seven InPageNavHeader routes, so it is broadly covered by the suite above and now has direct behavioural tests of its own.
• Rollback: git revert the single commit. No data, schema, config, or provider state is touched.
• Provider or production effects: None.

Notes

Deliberately left for a follow-up rather than folded in here (not written to docs/outstanding-issues.md — another session owns that file this cycle):

• useResolvedPageSections carries a third visibility predicate that is genuinely different from the two merged here: it tests getClientRects().length > 0 && computedStyle.display !== "none" rather than rect size. Converging it would change resolution behaviour on seven live routes and no current test covers the difference, so it was left alone and documented instead.
• The premise correction is worth carrying back to /issues #288 when that file is next writable: the issue names four duplicated modules, but three are already shared.

---

Generated by Claude Code

Summary by CodeRabbit

• Improvements

  • Improved in-page navigation so visible anchors and section targets resolve more consistently across document views.
  • Preserved the Document Viewer’s dedicated header and navigation presentation while aligning shared navigation behavior.
  • Improved fallback handling for aliased document sections and responsive layouts.

• Documentation

  • Clarified navigation behavior, viewer-specific presentation, metrics, and supported section-link conventions.
  • Added coverage to verify consistent navigation behavior and documented presentation decisions.

[supabase]

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in:11 minutes

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 50356704-9a90-4561-b4be-f8c27aa0bb74

📥 Commits

Reviewing files that changed from the base of the PR and between 5d0f350 and 01d2218.

📒 Files selected for processing (1)

• docs/branch-review-ledger.md

📝 Walkthrough

Walkthrough

The PR extracts shared visible-element resolution for DocumentViewer and in-page navigation. It adds convergence tests and documents the decision to retain a separate DocumentViewer header while sharing navigation infrastructure.

Changes

DocumentViewer navigation convergence

Layer / File(s)	Summary
Shared visibility resolution src/components/document-viewer/use-section-spy.ts, src/components/in-page-nav/use-page-section-weights.ts	Adds exported resolveVisibleElement, preserves aliased section resolution, and uses the shared resolver for section measurement.
Convergence validation and decisions tests/in-page-nav-document-viewer-convergence.dom.test.tsx, docs/search-chrome-behaviour.md, docs/branch-review-ledger.md	Adds DOM convergence tests and records the separate DocumentViewer header decision, shared navigation primitives, and verification results.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• BigSimmo/Database#1766: Shares the in-page navigation convergence infrastructure used by this PR.
• BigSimmo/Database#1769: Modifies InPageNavHeader and related in-page navigation behavior.
• BigSimmo/Database#1311: Modifies DocumentViewer section-navigation and visibility behavior.

Suggested reviewers:cursoragent

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the decision to retain the DocumentViewer navigation and the extraction of the duplicated anchor predicate.
Description check	✅ Passed	The description covers the change, verification, risks, rollback, provider effects, and scope exclusions with specific evidence.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/documentviewer-nav-convergence-oddhjx

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/branch-review-ledger.md`:
- Line 840: Update the ledger entry’s component reference from DocumentReviewer
to DocumentViewer, leaving the documented decision and all other entry details
unchanged.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d9203690-2823-4175-af5b-925069a822dc

📥 Commits

Reviewing files that changed from the base of the PR and between af85cbc and 5d0f350.

📒 Files selected for processing (5)

• docs/branch-review-ledger.md
• docs/search-chrome-behaviour.md
• src/components/document-viewer/use-section-spy.ts
• src/components/in-page-nav/use-page-section-weights.ts
• tests/in-page-nav-document-viewer-convergence.dom.test.tsx

[Copilot]

Pull request overview

Documents the decision to retain DocumentViewer’s specialized header while consolidating its duplicated visibility predicate.

Changes:

• Extracts shared resolveVisibleElement logic without changing behavior.
• Adds behavioral and architectural regression tests.
• Records the header non-adoption rationale and review outcome.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/components/document-viewer/use-section-spy.ts	Exports the shared visibility resolver.
src/components/in-page-nav/use-page-section-weights.ts	Reuses the shared resolver.
tests/in-page-nav-document-viewer-convergence.dom.test.tsx	Covers resolution behavior and sharing contracts.
docs/search-chrome-behaviour.md	Documents the non-adoption decision.
docs/branch-review-ledger.md	Records the review and verification.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

CI triage

CI failed on this PR. Automated classification of the 2 failed job(s):

• Production UI (3) — needs investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.
• PR required — needs investigation: inspect the failing step and uploaded diagnostics; rerun only after classifying the cause.

Compared with main CI run #9477 (cancelled).

Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger.