feat(in-page-nav): convert the six information routes onto the shared header - #1766
Conversation
… header PR 2 of the in-page navigation series. PR 1 (#1740) extracted the DocumentViewer in-page navigation template into src/components/in-page-nav/ and converted the differentials detail page as proof. This converts the six routes still drawing the shell-owned "On this page" pill rail — services, forms, specifiers (record and catalogue reference), formulation, and the two DSM diagnosis routes — then deletes the rail behind them. Three things the conversion needed first: - InPageNavHeader's `actions` was a render prop. Four of the seven components in scope are Server Components, and React cannot pass a function across that boundary, so `actions` is widened to `ReactNode | ((close) => ReactNode)`. `onSelectSection` and `PageSection.icon` have the same problem, so those four pages mount the header through a small "use client" sibling module that owns the section table and the hooks and takes `actions` as a slot. - Both sheets now derive their open state from the current pathname, so navigating closes them. Server-passed action JSX is mostly <Link>s and has no way to call close(). - Information-page sections carried no scroll-mt at all. A shared `inPageAnchor` token consumes `--inpage-anchor-offset`, published from the live chrome height by useInPageChromeMetrics, which InPageNavHeader calls itself. The measurement behind both anchor offsets is now useStickyChromeMetrics in src/components/sticky-chrome-metrics.ts, shared with the document viewer. use-document-chrome-metrics.ts keeps its selector and custom-property literals verbatim, because header-scroll-hide-contract.test.ts pins that file's source. Two DSM routes declared section anchors that nothing rendered, so /dsm/diagnoses/<slug>/differentials drew one nav entry out of four declared and /dsm/diagnoses/<slug> drew three out of five. Both are wired in, asserted against rendered DOM rather than a source grep (/issues #256's stop rule) by the new tests/in-page-nav-route-sections.dom.test.tsx, which covers all seven components including present-and-absent cases for the conditional anchors. Rail removal: hasLocalInformationPageNavigation collapses to isInformationPage, because every information page now owns its navigation; the `section` kind and its scroll spy leave secondary-navigation.tsx. That leaves the component with no production constructor at all — RegistryModeNav renders ModeNav, not SecondaryNavigation — which is folded into /issues #271 rather than resolved here. Also corrects /issues #256: its Forms half was stale, all six form anchors render. Records /issues #285 for a pre-existing ui-smoke phone scroll-hide flake at base commit 9ab3b73, proven on pristine source in the same worktree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis change adds shared in-page navigation and sticky chrome metrics. It migrates information pages to route-owned headers and action sheets. It removes section support from shared secondary navigation and updates ownership logic, tests, and documentation. ChangesIn-page navigation migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant InformationPage
participant InPageNavHeader
participant useInPageSectionNav
participant DOM
InformationPage->>InPageNavHeader: render section metadata and actions
InPageNavHeader->>useInPageSectionNav: select section
useInPageSectionNav->>DOM: resolve visible target and update fragment
DOM-->>InformationPage: scroll to anchored section
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
There was a problem hiding this comment.
Pull request overview
Migrates six information-route families to the shared in-page navigation header, removing the shell-owned section rail while preserving route actions, anchors, and responsive behavior.
Changes:
- Adopts
InPageNavHeaderacross service, form, specifier, formulation, and DSM detail pages. - Adds shared section resolution and sticky-chrome measurement infrastructure.
- Updates navigation contracts, browser tests, and documentation.
No high-confidence blocking defect was identified through static inspection. Local or provider-backed checks were not independently rerun.
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/components/services/service-detail-page.tsx |
Moves service navigation and actions into the shared header. |
src/components/forms/form-detail-page.tsx |
Adds shared header, actions sheet, and responsive anchors. |
src/components/specifiers/specifier-record-page.tsx |
Converts curated specifier records. |
src/components/specifiers/specifier-reference-page.tsx |
Converts catalogue reference records. |
src/components/specifiers/specifier-nav-header.tsx |
Adds the client navigation adapter. |
src/components/specifiers/specifier-ui.tsx |
Allows anchor styling on safety notes. |
src/components/formulation/formulation-mechanism-page.tsx |
Converts formulation detail navigation. |
src/components/formulation/formulation-nav-header.tsx |
Adds the client navigation adapter. |
src/components/formulation/formulation-ui.tsx |
Allows anchor styling on safety notes. |
src/components/dsm/dsm-diagnosis-page.tsx |
Converts diagnosis navigation and adds missing anchors. |
src/components/dsm/dsm-diagnosis-nav-header.tsx |
Adds the diagnosis header adapter. |
src/components/dsm/dsm-differential-considerations-page.tsx |
Converts navigation and wires missing sections. |
src/components/dsm/dsm-page-header.tsx |
Supports suppressing duplicate breadcrumbs. |
src/components/in-page-nav/in-page-nav-header.tsx |
Supports server-rendered actions and route-aware sheets. |
src/components/in-page-nav/in-page-nav-classes.ts |
Centralizes anchor and action-row classes. |
src/components/in-page-nav/use-in-page-chrome-metrics.ts |
Binds shared chrome metrics for information pages. |
src/components/in-page-nav/use-in-page-section-nav.ts |
Composes section resolution, scrolling, and deep links. |
src/components/in-page-nav/use-resolved-page-sections.ts |
Resolves visible conditional and responsive anchors. |
src/components/sticky-chrome-metrics.ts |
Extracts shared sticky-header measurement. |
src/components/document-viewer/use-document-chrome-metrics.ts |
Rebinds DocumentViewer to shared measurement. |
src/components/differentials/differential-detail-page.tsx |
Reuses the shared action-row class. |
src/components/secondary-navigation.tsx |
Removes obsolete section-navigation behavior. |
src/components/page-secondary-navigation.tsx |
Removes the shell-owned information-page rail. |
src/components/mode-nav/header-addon-slot.ts |
Registers the new page-owned header routes. |
src/components/clinical-dashboard/global-search-shell.tsx |
Updates shell navigation integration. |
tests/in-page-nav-route-sections.dom.test.tsx |
Guards declared anchors and scroll margins. |
tests/in-page-nav-header.dom.test.tsx |
Covers server actions and route-change closure. |
tests/secondary-navigation.dom.test.tsx |
Removes obsolete section-item tests. |
tests/page-secondary-navigation.dom.test.tsx |
Verifies local navigation ownership. |
tests/mode-nav-addon-slot.dom.test.tsx |
Covers claimant routes and Windows paths. |
tests/ui-forms-section-nav.spec.ts |
Exercises section sheets and stable fragments. |
tests/ui-formulation.spec.ts |
Updates formulation action and section flows. |
tests/ui-specifiers.spec.ts |
Updates specifier action-sheet coverage. |
tests/ui-tools.spec.ts |
Updates service action workflows. |
tests/ui-smoke.spec.ts |
Verifies the DSM header back control. |
tests/audit-content-services-regressions.test.ts |
Updates form source-action contracts. |
docs/search-chrome-behaviour.md |
Documents the shared navigation contract. |
docs/codebase-index.md |
Updates navigation architecture references. |
docs/outstanding-issues.md |
Records section-nav and browser-flake status. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
tests/in-page-nav-header.dom.test.tsx (2)
11-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset
pathnamebetween tests.
pathnameis module state. The route-change test leaves it at"/specifiers/compare", so every test that runs after it inherits that value. Test outcomes then depend on file order.Add a reset so each test starts from a known value.
♻️ Proposed reset
/** Reassigned per case so a rerender can simulate a route change. */ let pathname: string | null = null; vi.mock("next/navigation", () => ({ usePathname: () => pathname })); + +beforeEach(() => { + pathname = null; +});Import
beforeEachfromvitestin the existing import statement.🤖 Prompt for 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. In `@tests/in-page-nav-header.dom.test.tsx` around lines 11 - 13, Import beforeEach from vitest and reset the module-level pathname variable to null before each test, ensuring the route-change test cannot leak "/specifiers/compare" into subsequent tests.
231-242: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
renderHeaderprops for the rerender.The rerender repeats every prop from
renderHeader. If a required prop changes later, this copy drifts and the test can pass for the wrong reason. Extract the element into a helper that bothrenderHeaderandview.rerenderuse.🤖 Prompt for 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. In `@tests/in-page-nav-header.dom.test.tsx` around lines 231 - 242, Refactor the test setup around renderHeader so the InPageNavHeader element and its props are created by a reusable helper, then use that same helper for both the initial render and view.rerender. Remove the duplicated prop list while preserving the rerender-specific activeId behavior.tests/audit-content-services-regressions.test.ts (1)
204-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a rendered-DOM assertion for the placeholder.
Lines 208-209 assert exact source text. A rename of
sourceHrefor a formatting change breaks these assertions without a behaviour change.tests/in-page-nav-route-sections.dom.test.tsxstates the opposite rule for this migration: assert against the rendered DOM, never a source grep.Render
FormDetailPagewith a form that has nosource.urland assert the disabled placeholder text. Keep the source-text checks only where no rendered signal exists.🤖 Prompt for 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. In `@tests/audit-content-services-regressions.test.ts` around lines 204 - 210, Replace the exact source-text assertions for the missing-source placeholder in the regression test with a rendered-DOM test of FormDetailPage using a form without source.url, asserting the disabled placeholder text appears. Retain only source-text checks for behavior that cannot be verified through rendering.tests/in-page-nav-route-sections.dom.test.tsx (2)
70-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the fixtures exist instead of skipping.
Two issues make failures hard to read or invisible:
- Lines 106, 118, and 127-132 use
!onfindresults. If a data change makes afindreturnundefined, the test fails with a property-accessTypeErrorand no route name.- Lines 178 and 188 wrap the conditional-anchor assertions in
if (...). If no fixture without key features or without enrichment exists, the test passes and asserts nothing.Add explicit
expect(fixture).toBeDefined()checks so a missing fixture reports itself.Also applies to: 173-193
🤖 Prompt for 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. In `@tests/in-page-nav-route-sections.dom.test.tsx` around lines 70 - 75, Add explicit toBeDefined assertions for specifierCatalogItem, dsmDiagnosisWithKeyFeatures, dsmDiagnosisWithDifferentials, and the fixtures used by the conditional-anchor checks around the referenced assertions. Remove the conditional guards that skip those assertions when fixtures are missing, while retaining non-null assertions only after the explicit checks so failures identify the missing fixture clearly.
195-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe coverage guard cannot detect a new converted route.
expect(routes).toHaveLength(7)compares the local fixture array against a literal in the same file. An eighth component that mountsInPageNavHeaderdoes not change either side, so this test still passes and its sections stay unguarded. That is the exact failure mode the comment claims to cover.Derive the expected set from an external source.
tests/mode-nav-addon-slot.dom.test.tsxalready scanssrc/components/**/*.tsxfor the addon-slot claim and builds a claimant list; assert that every claimant route family has aRouteCasehere.🤖 Prompt for 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. In `@tests/in-page-nav-route-sections.dom.test.tsx` around lines 195 - 199, Update the coverage guard in the test that defines routes so its expected route families are derived from the external component scan used by mode-nav-addon-slot.dom.test.tsx, rather than the local routes array or a hard-coded count. Compare those discovered InPageNavHeader claimants against the RouteCase entries and fail when any claimant lacks coverage.tests/ui-forms-section-nav.spec.ts (1)
44-54: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the scroll result.
Lines 44-54 assert the fragment and sheet closure only. If
useInPageSectionNavupdates history butjumpToDocumentSectionfails, this test passes. After the selection, assert that the resolved visible target is in the viewport below sticky chrome.🤖 Prompt for 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. In `@tests/ui-forms-section-nav.spec.ts` around lines 44 - 54, Extend the test around the form-section trigger and “Source / verification” selection to assert the resolved visible section target is within the viewport below the sticky chrome. Use the existing section-navigation test identifiers or target symbol, and retain the fragment and hidden-sheet assertions.src/components/sticky-chrome-metrics.ts (1)
77-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the 16px anchor gap.
Line 79 adds a bare
16to the measured clearance. That value is a design spacing decision, not a measurement. Give it a named constant so it is greppable and so the intent stays clear when the chrome changes.♻️ Proposed refactor
+/** Breathing room between the chrome edge and the top of a jumped-to section. */ +const anchorGapPx = 16; + export function useStickyChromeMetrics({- root.style.setProperty(anchorOffsetProperty, `${Math.round(anchorClearance) + 16}px`); + root.style.setProperty(anchorOffsetProperty, `${Math.round(anchorClearance) + anchorGapPx}px`);🤖 Prompt for 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. In `@src/components/sticky-chrome-metrics.ts` around lines 77 - 80, Define a named constant for the 16px anchor gap and use it in the anchor offset calculation within the sticky chrome measurement flow. Replace the bare literal in the `root.style.setProperty(anchorOffsetProperty, ...)` expression while preserving the existing rounding and clearance behavior.Source: Coding guidelines
src/components/specifiers/specifier-reference-page.tsx (1)
139-156: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAlign the comment with the prop value.
InPageNavHeaderchecksactionsfor truthiness, sonullcorrectly suppresses the ellipsis control. Update the comment to state thatactionsis passed asnullwhen enrichment is absent.🤖 Prompt for 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. In `@src/components/specifiers/specifier-reference-page.tsx` around lines 139 - 156, Update the comment above the InPageNavHeader actions prop to state that actions is passed as null when enrichment is absent, matching the existing conditional value and truthiness behavior.
🤖 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 `@src/components/mode-nav/header-addon-slot.ts`:
- Around line 46-52: Update isInformationPage to exclude the reserved "search"
slug for services, forms, specifiers, and formulation routes, matching
isSlugDetailRoute behavior. Export isSlugDetailRoute and reuse it from
isInformationPage instead of maintaining separate slug-detail logic.
---
Nitpick comments:
In `@src/components/specifiers/specifier-reference-page.tsx`:
- Around line 139-156: Update the comment above the InPageNavHeader actions prop
to state that actions is passed as null when enrichment is absent, matching the
existing conditional value and truthiness behavior.
In `@src/components/sticky-chrome-metrics.ts`:
- Around line 77-80: Define a named constant for the 16px anchor gap and use it
in the anchor offset calculation within the sticky chrome measurement flow.
Replace the bare literal in the `root.style.setProperty(anchorOffsetProperty,
...)` expression while preserving the existing rounding and clearance behavior.
In `@tests/audit-content-services-regressions.test.ts`:
- Around line 204-210: Replace the exact source-text assertions for the
missing-source placeholder in the regression test with a rendered-DOM test of
FormDetailPage using a form without source.url, asserting the disabled
placeholder text appears. Retain only source-text checks for behavior that
cannot be verified through rendering.
In `@tests/in-page-nav-header.dom.test.tsx`:
- Around line 11-13: Import beforeEach from vitest and reset the module-level
pathname variable to null before each test, ensuring the route-change test
cannot leak "/specifiers/compare" into subsequent tests.
- Around line 231-242: Refactor the test setup around renderHeader so the
InPageNavHeader element and its props are created by a reusable helper, then use
that same helper for both the initial render and view.rerender. Remove the
duplicated prop list while preserving the rerender-specific activeId behavior.
In `@tests/in-page-nav-route-sections.dom.test.tsx`:
- Around line 70-75: Add explicit toBeDefined assertions for
specifierCatalogItem, dsmDiagnosisWithKeyFeatures,
dsmDiagnosisWithDifferentials, and the fixtures used by the conditional-anchor
checks around the referenced assertions. Remove the conditional guards that skip
those assertions when fixtures are missing, while retaining non-null assertions
only after the explicit checks so failures identify the missing fixture clearly.
- Around line 195-199: Update the coverage guard in the test that defines routes
so its expected route families are derived from the external component scan used
by mode-nav-addon-slot.dom.test.tsx, rather than the local routes array or a
hard-coded count. Compare those discovered InPageNavHeader claimants against the
RouteCase entries and fail when any claimant lacks coverage.
In `@tests/ui-forms-section-nav.spec.ts`:
- Around line 44-54: Extend the test around the form-section trigger and “Source
/ verification” selection to assert the resolved visible section target is
within the viewport below the sticky chrome. Use the existing section-navigation
test identifiers or target symbol, and retain the fragment and hidden-sheet
assertions.
🪄 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: 5dee6060-0486-4136-bde2-2dccc267d732
📒 Files selected for processing (39)
docs/codebase-index.mddocs/outstanding-issues.mddocs/search-chrome-behaviour.mdsrc/components/clinical-dashboard/global-search-shell.tsxsrc/components/differentials/differential-detail-page.tsxsrc/components/document-viewer/use-document-chrome-metrics.tssrc/components/dsm/dsm-diagnosis-nav-header.tsxsrc/components/dsm/dsm-diagnosis-page.tsxsrc/components/dsm/dsm-differential-considerations-page.tsxsrc/components/dsm/dsm-page-header.tsxsrc/components/forms/form-detail-page.tsxsrc/components/formulation/formulation-mechanism-page.tsxsrc/components/formulation/formulation-nav-header.tsxsrc/components/formulation/formulation-ui.tsxsrc/components/in-page-nav/in-page-nav-classes.tssrc/components/in-page-nav/in-page-nav-header.tsxsrc/components/in-page-nav/use-in-page-chrome-metrics.tssrc/components/in-page-nav/use-in-page-section-nav.tssrc/components/in-page-nav/use-resolved-page-sections.tssrc/components/mode-nav/header-addon-slot.tssrc/components/page-secondary-navigation.tsxsrc/components/secondary-navigation.tsxsrc/components/services/service-detail-page.tsxsrc/components/specifiers/specifier-nav-header.tsxsrc/components/specifiers/specifier-record-page.tsxsrc/components/specifiers/specifier-reference-page.tsxsrc/components/specifiers/specifier-ui.tsxsrc/components/sticky-chrome-metrics.tstests/audit-content-services-regressions.test.tstests/in-page-nav-header.dom.test.tsxtests/in-page-nav-route-sections.dom.test.tsxtests/mode-nav-addon-slot.dom.test.tsxtests/page-secondary-navigation.dom.test.tsxtests/secondary-navigation.dom.test.tsxtests/ui-forms-section-nav.spec.tstests/ui-formulation.spec.tstests/ui-smoke.spec.tstests/ui-specifiers.spec.tstests/ui-tools.spec.ts
|
@cursoragent Work the current open PR end-to-end. Confirm the PR number and GitHub head first from context. If more than one open PR could apply, stop and say which one you would use and why. Fetch and start from the remote tip that matches that GitHub head. If the named branch ref is missing or stale, use the PR head ref. Preserve unrelated local WIP, including any local-only ledger commits; do not discard dirty work, and do not treat a local-ahead commit as the reviewed tip. Do not merge the PR, force-push, rebase, or arm auto-merge unless I explicitly ask. No provider-backed gates without separate approval. If you cannot push or resolve threads, diagnose and comment only; if inline replies fail, resolve when possible and put dispositions in the summary comment. If auto-merge is already armed, push only for a real blocker, and avoid pushes that would cancel in-flight required CI unless the push itself clears that blocker. If the PR is already merged or closed: confirm the head and merge commit, note required-CI outcome, post one summary, and stop. Goal: deep review plus Bugbot, fix actionable issues with the smallest correct changes, clear merge / required-CI / thread blockers, run strong local offline verification, push fixes, append the review ledger, and post one PR summary. Prefer thoroughness over speed. Regenerate large assets only when a fix requires it; then run the asset check and keep compatibility aliases byte-identical where the repo uses them. Snapshot the GitHub head SHA: tip, base, behind/ahead, mergeable state, merge-tree versus origin/main (real conflict versus behind-but-clean), required checks on that tip including Production UI when selected, advisory separately, unresolved actionable threads. Missing checks while dirty are not green. If the tip moves mid-work, re-snapshot and continue from the new head. Ledger-lookup against that GitHub head under the heavy review-and-fix scope for this PR. Already reviewed at this head with clean merge-tree, green required checks, and no new actionable threads → summarize, comment, stop unless I asked for a fresh superseding pass. Follow the repo review protocol. Unblock once: real conflict → merge origin/main (prefer main’s shared queues; keep this PR’s notes); behind-but-clean → one sync, late if required CI is in flight; clean → leave. Labels like skip-branch-sync do not block a manual sync needed to clear a real blocker. No sync thrash. After any sync or push, re-snapshot tip, merge-tree, and required checks before declaring done. Dedupe the ledger if a merge touched it. Review high-confidence delta risks only. Separate PR-introduced defects from pre-existing re-emitted debt. For generated assets, review contracts, aliases, cache, and manifests—not every generated line. If protected RAG or ranking surfaces are touched, say so before editing; fix PR-body policy text only when wrong or missing. Ignore bot noise. No nit spam or broad rewrites. Escalate verification by touched risk: clinical / RAG / privacy / migrations / auth → domain check plus production-readiness when warranted; UI / phone-chrome / routing / styling → ensure plus phone-chrome or UI gates when warranted; generated assets or docs inventory/links → asset or docs checks when warranted. Fix P0/P1 always; clear scoped P2 when locally provable; else disposition and resolve. Required-check failures on this tip only; ignore advisory. Prefer reverting a bad autofix commit. After fixes: format and commit before push; smallest targeted proof; repo cheap gate; repo PR-local gate. No release, lighthouse, live eval, or live provider gates without approval. Push only this PR’s fix commits. Append ledger for the final GitHub head under the heavy scope. Never push a ledger-only tip—include ledger in a real fix push, or leave a no-change ledger append local and unpushed. One PR comment: tip, sync/merge-tree, fixed versus dispositioned, threads, required CI, decisive local gate lines, residual risks; merge left to me. Inline only for remaining human-needed P0/P1. Stop when merge-tree is clean on the current GitHub tip, actionable threads are clear, required checks are green or clearly in progress, heavy gates for scope passed, summary posted, merge left to me. |
|
Taking a look!   |
…2-6d32f9 # Conflicts: # docs/codebase-index.md # docs/outstanding-issues.md # src/components/forms/form-detail-page.tsx # src/components/services/service-detail-page.tsx
…2-6d32f9 # Conflicts: # docs/outstanding-issues.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/services/service-detail-page.tsx (1)
608-622: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the unavailable Call action reachable.
The
disabledattribute removes the button from the tab order. Keyboard users and screen-reader users cannot focus it, so neither thetitletext nor theservice-call-unavailabledescription is announced. Usearia-disabledwith an inert handler to keep the row focusable and the reason discoverable.♿ Proposed fix
<button type="button" - disabled - title="Call — no contact number listed for this service" + aria-disabled="true" + onClick={(event) => event.preventDefault()} aria-describedby="service-call-unavailable" className={cn(inPageActionRowClass, "opacity-60")} >🤖 Prompt for 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. In `@src/components/services/service-detail-page.tsx` around lines 608 - 622, Update the unavailable Call button in the service-detail action rendering to remove the native disabled attribute, add aria-disabled, and provide an inert click/activation handler so it remains focusable while performing no action. Preserve the existing title and service-call-unavailable accessible description.docs/outstanding-issues.md (2)
168-171: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPlace the A2 item before lower-acuity entries.
The queue is described as priority-ordered, but
#289(A2 auth/identity) appears after#286and#287(A3) and#288(Optional). Move#289before those entries, or document that this block is an append-only snapshot.🤖 Prompt for 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. In `@docs/outstanding-issues.md` around lines 168 - 171, Reorder the outstanding-issues queue so the A2 item `#289` appears before the lower-acuity `#286`, `#287`, and `#288` entries, preserving the existing row content. If the list is intentionally append-only, explicitly document that this section is an append-only snapshot instead.
168-168: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSynchronize
#286with the PR status.The queue still says
#286starts “After owner go-ahead”, while the detailed row records implementation in PR#1766. After that PR merges, close the row and remove or update its queue entry. Otherwise, the ledger can recommend completed work again.As per coding guidelines, use
docs/outstanding-issues.mdas the single durable ledger and update it through the/issuesworkflow.Also applies to: 331-334
🤖 Prompt for 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. In `@docs/outstanding-issues.md` at line 168, Update the `#286` entries in docs/outstanding-issues.md through the /issues workflow to reflect PR `#1766`’s implementation status: after the PR merges, close the detailed row and remove or update the queue entry so it no longer recommends completed work. Apply the same synchronization to issues 331–334.Source: Coding guidelines
🧹 Nitpick comments (1)
src/components/services/service-detail-page.tsx (1)
702-707: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn the contact classification from
contactDisplayValueinstead of re-deriving it.Lines 703 and 80 apply the same web-contact rule. The rule now lives in two places. If one copy changes, the
Contact:prefix and the shortened value can disagree.♻️ Proposed refactor
-function contactDisplayValue(contact: ServiceContact | null | undefined) { +function contactDisplayValue(contact: ServiceContact | null | undefined): { label: string; isWeb: boolean } | undefined { const value = contact?.value?.trim(); if (!value) return undefined; if (contact?.kind === "web" || /^https?:\/\//i.test(value)) { try { const host = new URL(value).hostname.replace(/^www\./i, ""); - return host || "Website"; + return { label: host || "Website", isWeb: true }; } catch { - return "Website"; + return { label: "Website", isWeb: true }; } } - return value.includes("|") ? compactCatalogField(value, 160) : value; + return { label: value.includes("|") ? compactCatalogField(value, 160) : value, isWeb: false }; }- {contactLabel - ? primaryContact?.kind === "web" || /^https?:\/\//i.test(primaryContact?.value ?? "") - ? contactLabel - : `Contact: ${contactLabel}` - : "Contact not listed"} + {contactLabel + ? contactLabel.isWeb + ? contactLabel.label + : `Contact: ${contactLabel.label}` + : "Contact not listed"}🤖 Prompt for 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. In `@src/components/services/service-detail-page.tsx` around lines 702 - 707, Update the contact heading rendering near primaryContact to use the contact classification returned by contactDisplayValue instead of re-evaluating the web-contact condition inline. Reuse that classification to keep the Contact: prefix consistent with the shortened contact value, and remove the duplicated /^https?:\/\//i check from this rendering path.
🤖 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.
Outside diff comments:
In `@docs/outstanding-issues.md`:
- Around line 168-171: Reorder the outstanding-issues queue so the A2 item `#289`
appears before the lower-acuity `#286`, `#287`, and `#288` entries, preserving the
existing row content. If the list is intentionally append-only, explicitly
document that this section is an append-only snapshot instead.
- Line 168: Update the `#286` entries in docs/outstanding-issues.md through the
/issues workflow to reflect PR `#1766`’s implementation status: after the PR
merges, close the detailed row and remove or update the queue entry so it no
longer recommends completed work. Apply the same synchronization to issues
331–334.
In `@src/components/services/service-detail-page.tsx`:
- Around line 608-622: Update the unavailable Call button in the service-detail
action rendering to remove the native disabled attribute, add aria-disabled, and
provide an inert click/activation handler so it remains focusable while
performing no action. Preserve the existing title and service-call-unavailable
accessible description.
---
Nitpick comments:
In `@src/components/services/service-detail-page.tsx`:
- Around line 702-707: Update the contact heading rendering near primaryContact
to use the contact classification returned by contactDisplayValue instead of
re-evaluating the web-contact condition inline. Reuse that classification to
keep the Contact: prefix consistent with the shortened contact value, and remove
the duplicated /^https?:\/\//i check from this rendering path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 56bd33dd-4fb5-4828-809a-6859f98fa781
📒 Files selected for processing (12)
docs/branch-review-ledger.mddocs/codebase-index.mddocs/outstanding-issues.mdsrc/components/clinical-dashboard/global-search-shell.tsxsrc/components/differentials/differential-detail-page.tsxsrc/components/forms/form-detail-page.tsxsrc/components/services/service-detail-page.tsxsrc/components/specifiers/specifier-ui.tsxtests/ui-formulation.spec.tstests/ui-smoke.spec.tstests/ui-specifiers.spec.tstests/ui-tools.spec.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/components/differentials/differential-detail-page.tsx
- tests/ui-smoke.spec.ts
- src/components/clinical-dashboard/global-search-shell.tsx
- docs/codebase-index.md
- src/components/specifiers/specifier-ui.tsx
- tests/ui-formulation.spec.ts
- tests/ui-tools.spec.ts
- src/components/forms/form-detail-page.tsx
- tests/ui-specifiers.spec.ts
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #9427 (cancelled). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
…lug-detail test `InPageNavHeader` calls `usePathname()`, so every test that renders a converted page needs it on the `next/navigation` mock. `tests/service-detail-page.dom.test.tsx` mocked only `useRouter`, which failed three cases in CI's Unit coverage job with `No "usePathname" export is defined on the "next/navigation" mock`. It was the only such gap: the other eight test files mocking `next/navigation` without `usePathname` render no converted page. Review follow-ups: - `tests/in-page-nav-header.dom.test.tsx` resets its module-level `pathname` in `beforeEach`. The route-change case left it at `/specifiers/compare`, so outcomes depended on file order. - `header-addon-slot.ts` now calls the exported `isSlugDetail` from `information-pages.ts` instead of its own hand-copied equivalent. Every claimant route it names must also be `isInformationPage`, and two slug tests are how that agreement would silently diverge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r-status claim Two valid review findings on #1773, both verified against the repo before acting. Codex (P2, docs/outstanding-issues.md #290): the "Next" step added in 16b6119 told the reader to append each of the next two reproductions to tests/flake-ledger.json. Following it cannot produce a valid ledger state. docs/testing.md requires three same-SHA reproductions before an entry is added or retained, and scripts/flake-ledger.mjs:48 throws "exact title must include @quarantine" while tests/ui-smoke.spec.ts:4470 is untagged — so an entry written today fails validation outright, and appending per run treats the ledger as a per-run log. The step now says to record the two observations in the row itself, then after the third reproduction tag the test @quarantine and add one complete entry with owner, repro command, tracking ref, first/last-seen and expiry. CodeRabbit (docs/outstanding-issues.md #292): the claim that the ledger "has no in-progress state" is wrong — IN PROGRESS appears on two rows and IMPLEMENTED in PR #1766 on another. The accurate gap is narrower and worse: there is no structured status field and no atomic claim, so a marker is written by whoever did the work, usually after the fact, and nothing requires or checks one, which means the absence of a marker carries no information. Corrected in all four places that carried the overstatement, not only the flagged row — AGENTS.md, the issues and newtask skills, and #292 itself — since the same sentence had been copied into each. CI on the previous head was green; no check failures were outstanding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Ex4A4xx3tNdZUADV9VaXK
…ree items (#1773) * docs: pin PageSection ownership to the nav-header sibling, capture three items Record the in-page-nav decision in docs/search-chrome-behaviour.md under "Default in-page navigation template": a page's PageSection[] table is always owned and exported by a colocated "use client" nav-header sibling, never declared inline in the page and never in a separate per-route section-index module. The four Server Component pages need the sibling regardless, since neither onSelectSection nor a LucideIcon crosses the RSC boundary. Applying the same shape to Client Component pages gives one answer to where the section table lives and one import path for tests/in-page-nav-route-sections.dom.test.tsx. PR #1766 shipped both shapes; the closed PR #1767 proposed a third. States explicitly that existing pages are not being migrated — the rule binds new conversions only, and the grandfathered sites are listed so the inconsistency is not read as a bug. Ledger: - #291 (P2 issue) disabled placeholder buttons pair `disabled` with an aria-describedby sr-only reason; `disabled` removes the tab stop, so the reason is never announced. Needs one repo-wide pass across the sites, docs/wiring-conventions.md and eslint-rules/require-button-wiring.mjs. - #292 (P2 rec) two sessions built the same queued item four hours apart (PR #1766, closed PR #1767); check the open-PR list for the affected route before starting a queued item, not just the ledger row. - #290 extended with the quarantine arithmetic: tests/flake-ledger.json is empty, so two more same-SHA reproductions are needed before a quarantine entry is admissible. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Ex4A4xx3tNdZUADV9VaXK * docs: fix the placeholder-pattern reference, wire the duplicate-work check Implements the smallest correct fix for the two recommendations that had one, and corrects a recommendation that turned out to be wrong. #291 — corrected, not implemented as written. The row claimed that pairing `disabled` with an aria-describedby sr-only reason means the reason is never announced, and that the docs and eslint rule blessed the pairing so a repo-wide pass was needed. All three claims are refuted. A natively disabled button stays in the accessibility tree with its accessible description intact; tests/favourites-hub-unavailable-controls.dom.test.tsx pins toBeDisabled(), not.toHaveAttribute("aria-disabled") and toHaveAccessibleDescription() together under the title "keeps unavailable actions natively disabled and exposes their reasons" — a deliberate, tested decision. require-button-wiring already accepts both forms. An attempt to swap the attribute in favourites-hub and four other sites was reverted for contradicting that decision. The one real defect was documentation: docs/wiring-conventions.md named favourites-hub.tsx as "the reference markup" while showing aria-disabled="true", which is not what that file does. The canonical block now shows native `disabled`, explains why the description survives it, cites the pinning test, and says when aria-disabled + a no-op handler is correct instead. It also records that `disabled` + `aria-disabled` together is a third shape pinned two contradictory ways in one test file, which stays open as the residue of #291. #292 — mitigation implemented. The open-PR check before starting a queued item is now in newtask's "Before you start" (which already made that GitHub read for PR bundling, so it asks the same list a second question at no extra cost), in the issues skill after the read-only flow, and in the /issues section of AGENTS.md so Codex and Cursor get it too rather than Claude Code only. All three scan for the route/component/surface rather than the ledger id, and degrade to a warning when GitHub is unreachable. #290 — not implementable here: it needs two more same-SHA Playwright reproductions, and this container has no node_modules or browsers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Ex4A4xx3tNdZUADV9VaXK * docs: address PR review — fix the quarantine sequencing and the ledger-status claim Two valid review findings on #1773, both verified against the repo before acting. Codex (P2, docs/outstanding-issues.md #290): the "Next" step added in 16b6119 told the reader to append each of the next two reproductions to tests/flake-ledger.json. Following it cannot produce a valid ledger state. docs/testing.md requires three same-SHA reproductions before an entry is added or retained, and scripts/flake-ledger.mjs:48 throws "exact title must include @quarantine" while tests/ui-smoke.spec.ts:4470 is untagged — so an entry written today fails validation outright, and appending per run treats the ledger as a per-run log. The step now says to record the two observations in the row itself, then after the third reproduction tag the test @quarantine and add one complete entry with owner, repro command, tracking ref, first/last-seen and expiry. CodeRabbit (docs/outstanding-issues.md #292): the claim that the ledger "has no in-progress state" is wrong — IN PROGRESS appears on two rows and IMPLEMENTED in PR #1766 on another. The accurate gap is narrower and worse: there is no structured status field and no atomic claim, so a marker is written by whoever did the work, usually after the fact, and nothing requires or checks one, which means the absence of a marker carries no information. Corrected in all four places that carried the overstatement, not only the flagged row — AGENTS.md, the issues and newtask skills, and #292 itself — since the same sentence had been copied into each. CI on the previous head was green; no check failures were outstanding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Ex4A4xx3tNdZUADV9VaXK --------- Co-authored-by: Claude <noreply@anthropic.com>
…es (PR 3 of 3) Completes the in-page navigation series. PR 1 (#1740) extracted the template into src/components/in-page-nav/ and converted differentials; PR 2 (#1766) converted the six information routes and deleted the shell-owned pill rail. Three routes were left; this lands two of them and records why the third is not a candidate. Medications (/medications/[slug]) — converted, panel-swap The owner's call: keep the tab swap and drive it from the header's segment track, exactly as differentials/differential-detail-page.tsx does. SectionTabs (a 58-line roving-tabindex tablist) is deleted; activeTab lifts to MedicationRecordPage so the header above the shell can drive it, and the InformationPageBreadcrumbs row goes with it. The tab->section-type grouping moves into the new medication-nav-header.tsx sibling so the segment weights and the rendered panel cannot disagree about what a tab contains. The panel keeps a per-tab id but drops role="tabpanel"/aria-labelledby: the control is now a list of buttons, so claiming the role would name a tab that no longer exists. Factsheets (/factsheets/[slug]) — converted, anchor-scrolling Already mounted InPageNavHeader in its breadcrumb shape; this gives it a real section index. tocFor is deleted rather than ported: it returned display strings with no anchors behind them, painted into an inert <li> "On this page" list, and was wrong in both directions (it named "What is this medicine?" where the page renders "What is <title>?", and never listed the Sources, More-in-topic or Related sections every sheet renders). The replacement derives sections from what each of the five kinds actually renders, with medLite's headings coming from the record. Differentials presentations — recorded exception, not converted The premise did not survive reading the file. It was carried as "a SectionTabs page that swaps panels"; it swaps nothing — MobileTabs is four <Link>s to other routes with "Compare" hardcoded active, which is the multi-route ModeNav pattern the template already carves out. Its candidate sections are also rendered two or three times per breakpoint in different DOM parents, one nested inside another section's anchor, which PageSection.targetIds does not model. Reasoned exception recorded in docs/search-chrome-behaviour.md. Also: delete the orphaned SecondaryNavigation component (/issues #271) Test-only since PR 2 removed the section kind. Its two stated side-conditions turned out not to exist — nothing outside its own test imports it, and tests/mode-nav-contract.test.ts string-matches page-secondary-navigation.tsx, a different file. Its test fixture was literally the medication tab bar, so it lands with the conversion that retired it. Guards tests/in-page-nav-route-sections.dom.test.tsx grows from 7 routes to 12 (one factsheet case per kind) and gains a panel-swap suite for medications, so both halves of /issues #256's stop rule hold: declared ids are proven against rendered DOM, never a source grep. A new `absent` field asserts the therapy and procedure sheets genuinely do not render More-in-topic rather than skipping it. Both new routes are registered in isHeaderAddonSlotOwnedRoute and the claimant enumeration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GHmzgER6rv8Va9HbQZw87Q
Summary
PR 2 of the in-page navigation series. PR 1 (#1740) extracted the DocumentViewer in-page navigation template into
src/components/in-page-nav/and converted the differentials detail page as proof. This converts the six routes still drawing the shell-owned "On this page" pill rail and deletes the rail behind them.InPageNavHeader: services, forms, specifiers (record and catalogue reference), formulation,/dsm/diagnoses/[slug], and its/differentialschild. Each drops its breadcrumb row, keeps its in-body<h1>, and moves its record actions into the header's ellipsis sheet.actionsAPI before converting anything. It shipped as a render prop, and four of the seven components are Server Components, which cannot pass a function across the RSC boundary. It is nowReactNode | ((close: () => void) => ReactNode).onSelectSectionandPageSection.iconhave the same constraint, so those four pages mount the header through a small"use client"sibling module (specifier-nav-header.tsx,formulation-nav-header.tsx,dsm-diagnosis-nav-header.tsx) that owns the section table and the hooks and receivesactionsas a slot.<Link>s and has no way to callclose().useResolvedPageSectionsnarrows a declaration to the anchors actually rendered at this breakpoint (ported from the rail'sAvailableInformationPageNavigation, including itsMutationObserveron#main-contentand its signature guard);useInPageSectionNavcomposes that with the existing scroll spy andjumpToDocumentSection, and resolves a deep-linked fragment that is deliberately not an element id. Anchored sections carryinPageAnchor, which consumes--inpage-anchor-offsetpublished byuseInPageChromeMetrics— information-page sections previously carried noscroll-mtat all, so every jump would otherwise land underneath the new header.useStickyChromeMetrics(src/components/sticky-chrome-metrics.ts) is now the single implementation, withuse-document-chrome-metrics.tsas the document viewer's binding of it. That file keeps its selector and custom-property literals verbatim, becausetests/header-scroll-hide-contract.test.tspins its source text. Document viewer behaviour is unchanged./dsm/diagnoses/[slug]/differentialsdeclared four section targets and rendered one;/dsm/diagnoses/[slug]declared five and rendered three. This is the one place the PR changes more than chrome.hasLocalInformationPageNavigationcollapses toisInformationPage, since every information page now owns its navigation. Thesectionkind, its scroll spy, and its internal select event leavesecondary-navigation.tsx./issues #256. Its Forms half was stale — all six form anchors render. The live gaps were the two DSM routes, now fixed./issues #285, a pre-existingui-smokephone scroll-hide flake at base commit9ab3b73a, and updated/issues #271, sincesecondary-navigation.tsxnow has no production constructor at all (RegistryModeNavrendersModeNav, notSecondaryNavigation). Both are left for their own change rather than resolved here.New guard:
tests/in-page-nav-route-sections.dom.test.tsxrenders all seven components with real records and asserts every declared section id resolves to an element in the DOM, plus the shared scroll-margin token on each, plus present-and-absent cases for the conditional anchors. That is the guard/issues #256asked for, and it asserts rendered DOM rather than grepping forid=, which is that issue's explicit stop rule.Verification
npm run verify:pr-local— 527 of 530 files, 5708 tests passed. Three files failed, none caused by this change:pr-handoff-stop.test.ts(5 cases) fails identically on unmodifiedorigin/mainin a sibling worktree;codex-cloud-setup.test.tsandworker-observability.test.tspass in isolation on both this branch and unmodified main, and did not fail in this branch's earlierverify:cheaprun, so they are order/concurrency dependent under the full suite.npm run verify:cheap— 529 of 530 files, 5710 tests passed; only the pre-existingpr-handoff-stop.test.ts.npm run verify:ui— reached vianpm run verify:phone-chrome, which escalated to the full Chromium suite as expected for shared-chrome scope. Contracts stage 119 passed; focused-browser stage 13 passed; full Chromium run 398 passed with 13 failures, all of them specs still driving the controls this PR moved into the actions sheet. After updating those specs, all 13 pass. A later full-gate run was 184 passed / 3 failed, all three in the pre-existingui-smokephone scroll-hide flake family described below.npm run typecheckand whole-treeprettier --checkclean.actionsboundary holds.Not run:
npm run verify:release,eval:*,check:supabase-project,check:production-readiness— provider-backed, and this diff touches no retrieval, ingestion, answer-generation, auth, privacy, or environment path.On the remaining browser failures. They are pre-existing on
main, and I verified that rather than assuming it: I reverted all ofsrc/to base commit9ab3b73ain the same worktree and moved this branch's new untracked modules aside so the build was pristine, andanswer glass header overlays main and fully hides while scrolling on phonesstill failed 2 of 5 repeats. A worktree at the olderorigin/maine8adde1b9passed 5 of 5 twice on the same machine, so the regression window ise8adde1b9..9ab3b73a. Tracked as/issues #285; not quarantined, because repo policy wants three same-SHA reproductions intests/flake-ledger.jsonand the likely fix is the scroll-hide reporter's settle timing.Risk and rollout
Presentation-layer only. No API, schema, retrieval, ingestion, auth, or privacy surface is touched, and no clinical content changes — the DSM work adds
idattributes to sections that already rendered, and moves oneidfrom an<h2>to its wrapping<section>with the heading renamed to-titleandaria-labelledbyupdated.The user-visible changes are deliberate and worth naming for review:
focus=1. Forms'ActionPanelwas rendered twice (desktop and mobile) and is now one set of sheet rows, and its hero "Source" button is replaced by the sheet's "Open official source".DsmHeaderActionsis deleted, its last consumer having moved into the sheet.Rollback is a single revert of this commit; nothing here is stateful or migration-backed.
Incidental fix worth flagging:
tests/mode-nav-addon-slot.dom.test.tsx's claimant walk compared OS paths against forward-slash literals, so on Windows its shared-header exclusion never matched and the assertion could not pass. It now normalises the separator.Notes
Base is behind
origin/mainat the time of opening; the branch was cut from9ab3b73aandmainhas since advanced. CI merges with currentmain, so the PR checks test the merge result.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests