refactor(navigation): remove the vestigial one-button mode strip - #1679
Conversation
Seven modes registered a single action-only entry — `answer` an "Ask" button, and documents/services/forms/favourites/prescribing/tools a "Search" one. Each rendered one <button> inside its own <nav> landmark whose only effect was focusing a composer already visible on the same screen. For a screen-reader user navigating by landmark that is a navigation region announced for a no-op. Every one of these modes is genuinely single-surface — records, or one page — so there was nothing to adopt onto the shared header bar and nothing to replace the button with. It is deleted, not ported. Factsheets, the one of the eight with a real second destination, took the bar in #1674, which is why that had to land first. The one argument for keeping the strip does not hold: the shell renders it with sticky={false} and never passes placeInShell, so it sits in normal flow and scrolls off-screen at exactly the moment a phone composer scroll-hides. It was never the fast way back to anything. Details worth knowing: - The `registry[modeId][0]?.id` fallback in activeModeSecondaryNavigationId is deleted, and the compiler forces it: an emptied `as const` entry is `readonly []`, so `[0]` is a hard TS2493. Anyone emptying the arrays without this gets a typecheck failure they might "fix" with a cast, quietly restoring the wrong-slot-current bug that function's own doc comment warns about. - The `documents` clause in isModeSecondaryNavigationRoute goes with its entry. The `hasSubmittedSearch` early return STAYS — it is the only thing that puts the bar on a submitted-search mode home for all five adopted modes. - PageSecondaryNavigation loses modeItems, modeAriaLabel, onSearch and stickyTop (which already had zero callers). The empty-registry return sits BELOW the informationDefinitions branch on purpose: services, forms, documents and prescribing register nothing yet still claim section navs, and hoisting it would strip navigation from every /services/*, /forms/*, /medications/* and /documents/<id> record. That ordering is mutation-checked. - The `action` kind on SecondaryNavigation is kept, with a comment saying it now has no live consumer. It carries the tablist roving-focus behaviour and is covered directly by its own component tests, and check:knip runs without --include exports so it would not be caught by the dead-code gate. Deleting it is a clean separate change. Noted while verifying, not fixed here: /forms/form-1 draws no section nav at all. form-detail-page.tsx carries "form-decision-context-mobile" as a testId rather than an element id, and the other five declared targetIds are rendered nowhere, so AvailableInformationPageNavigation filters them all out and returns null. That is /issues #256, pre-existing and unaffected by this change — the section branch won on that route before and still does. Recorded in the new test's comment. Verification: lint and typecheck clean; focused set 5 files / 97 tests; full offline suite 518/519 files, the single failure (tests/pr-handoff-stop.test.ts) re-confirmed pre-existing on this base by a stashed re-run. ui-mode-nav-density + ui-accessibility: 71 passed — the latter carries the duplicated-landmark scan, which is where seven deleted <nav>s would have surfaced. The branch-order guard was mutation-checked: hoisting it above the section branch fails 2 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01823Ctwj4vinGhGuRNyK7oE
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe PR removes legacy secondary-navigation search actions and fallback rendering. Seven modes now use empty registries. Route resolution returns no active destination when no explicit route exists. Tests cover registry, rendering, and route behavior. ChangesSecondary navigation cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01823Ctwj4vinGhGuRNyK7oE
There was a problem hiding this comment.
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 703: Update the 2026-08-07 ledger entry for commit
4ff613c10fbf734b1e740a31611296c17c791ec7 by appending a correction via the
ledger command. Replace the insufficient “lint exit 0” evidence with the
decisive lint output line, while preserving the rest of the verification
details.
🪄 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: 48364f73-402e-4901-bf0f-39dc3d9a25a4
📒 Files selected for processing (8)
docs/branch-review-ledger.mdsrc/components/clinical-dashboard/global-search-shell.tsxsrc/components/page-secondary-navigation.tsxsrc/components/secondary-navigation.tsxsrc/lib/mode-secondary-navigation.tstests/mode-nav-addon-slot.dom.test.tsxtests/mode-secondary-navigation.test.tstests/page-secondary-navigation.dom.test.tsx
💤 Files with no reviewable changes (1)
- src/components/clinical-dashboard/global-search-shell.tsx
…n-kind decision #256 was 'suspected remaining' for form-overview and the differential-presentation-* set. Both are now confirmed dead, so two live routes draw no section nav at all: /forms/<slug> (one anchor is a testId rather than an element id, the other five are rendered nowhere) and /differentials/presentations/<slug> (all six absent, and the only dynamic section id in that family uses a different prefix on a different route, so nothing is hiding behind a sectionId prop). Also captures #261: whether to delete SecondaryNavigationActionItem, which lost its last live consumer in PR #1679 and was deliberately kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01823Ctwj4vinGhGuRNyK7oE
Summary
answer(an "Ask" button) anddocuments,services,forms,favourites,prescribing,tools(a "Search" one). Each rendered one<button>inside its own<nav>landmark whose only effect was focusing a composer already visible on the same screen. For a screen-reader user navigating by landmark, that is a navigation region announced for a no-op. All seven are genuinely single-surface — records, or one page — so there was nothing to adopt onto the shared header bar and nothing to replace the button with. Deleted, not ported. Factsheets, the one mode of the eight with a real second destination, took the bar in feat(navigation): adopt the shared mode nav for Factsheets #1674, which is why that had to land first.sticky={false}and never passesplaceInShell, so it sits in normal flow and scrolls off-screen at exactly the moment the composer hides.registry[modeId][0]?.idfallback inactiveModeSecondaryNavigationIdis deleted, and the compiler forces it. An emptiedas constentry isreadonly [], so[0]is a hard TS2493. Anyone emptying the arrays without this gets a typecheck failure they might "fix" with a cast — quietly restoring the mark-the-first-slot-current bug that function's own doc comment warns callers about. Worth knowing before reviewing the diff.documentsclause inisModeSecondaryNavigationRoutegoes with its entry. ThehasSubmittedSearchearly return stays — it is the only thing that puts the bar on a submitted-search mode home for all five adopted modes, and is now commented as such so it is not mistaken for more dead gating.PageSecondaryNavigationlosesmodeItems,modeAriaLabel,onSearchandstickyTop(the last already had zero callers anywhere). The empty-registry return sits below theinformationDefinitionsbranch deliberately:services,forms,documentsandprescribingregister no destinations yet still claim section navs, so hoisting it would silently strip navigation from every/services/*,/forms/*,/medications/*and/documents/<id>record. That ordering is the highest-risk edit here and is mutation-checked.actionkind onSecondaryNavigationis kept, with a comment recording that it now has no live consumer. It carries thetablistroving-focus behaviour and is covered directly by its own component tests, so it is component API with tests rather than orphaned code — andcheck:knipruns without--include exports, so it would not be caught by the dead-code gate either way. Deleting it is a clean separate change; the point is not to do half of each.Found while verifying, deliberately not fixed here:
/forms/form-1draws no section nav at all.form-detail-page.tsx:881carriesform-decision-context-mobileas atestId, not an elementid, and the other five declaredformSectionstargetIds are rendered nowhere — soAvailableInformationPageNavigationfilters every item out and returnsnull. That is/issues#256 (declared section sets whose targets nothing renders), confirmed live for forms. It is pre-existing and unaffected by this PR: the section branch already won on that route and still does. Recorded in the new test's comment so the next person finds it where they'd look.RAG impact: no retrieval behaviour change — navigation and layout only.
Verification
npm run verify:pr-localVerification not run:
verify:pr-localaborts atcheck:installed-lock-parity— installed@playwright/testis 1.62.0 against a locked 1.62.1. Deliberately not resolved withnpm ci: 1.62.1 expects Chromium build 1234 while only 1194 is on disk with downloads disabled, which would stop the browser specs running at all. Components run directly instead:npm run lint— exit 0, clean.npm run typecheck— clean. This is the gate that forces the TS2493 fallback deletion; it also surfaced exactly the 12 test call sites still passing the removedonSearchprop, and nothing else.npm run test—Test Files 1 failed | 518 passed (519),Tests 1 failed | 5493 passed | 4 skipped (5498). The single failure istests/pr-handoff-stop.test.ts, a shell-hook test untouched by this diff; re-confirmed pre-existing on this base by a stashed re-run (1 failed | 10 passedon the clean tree), not carried over from feat(navigation): adopt the shared mode nav for Factsheets #1674's finding.Focused set —
mode-secondary-navigation,mode-nav-contract,mode-nav-addon-slot.dom,page-secondary-navigation.dom,secondary-navigation.dom:Test Files 5 passed (5),Tests 97 passed (97).npm run format— run and committed.npm run verify:uiwhen UI, routing, styling, browser behavior, reduced-motion, or forced-colors behavior changedUI verification run directly (same lock-parity reason):
tests/ui-mode-nav-density.spec.ts+tests/ui-accessibility.spec.ts→71 passed (1.1m).ui-accessibilityis the one that matters beyond the usual set here — it carries the duplicated-landmark scan, which is where seven deleted<nav>s would have surfaced.Mutation check on the load-bearing edit: hoisting the empty-registry guard above the
informationDefinitionsbranch fails 2 tests inpage-secondary-navigation.dom.test.tsx, including the newformsbranch-order test. The guard placement is genuinely protected, not just commented.Risk and rollout
PageSecondaryNavigationas before, just with the legacy fallback branch gone.mode-secondary-navigation.tsandpage-secondary-navigation.tsxdepends on the removed machinery.Clinical Governance Preflight
Not applicable, verified against
scripts/pr-policy.mjsrather than assumed: none ofsrc/lib/mode-secondary-navigation.ts,src/components/page-secondary-navigation.tsx,src/components/secondary-navigation.tsxorsrc/components/clinical-dashboard/global-search-shell.tsxmatchesclinicalRiskPatterns— thesrc/lib/…clause requires one of auth/permission/privacy/security/rag/retriev/rank/search/answer/clinical/citation/source/document/upload/download in the path, and the presentation clause is explicitly written so that living underclinical-dashboard/is not by itself clinical-risk. Nothing matchesragRankingPatterns. Navigation wiring only.Notes
This completes the mode-navigation handover. All 13 modes now have navigation matching what they actually contain: five carry the shared header bar (
dsm,specifiers,formulation,differentials,factsheets),therapy-compassfeedsModeNavfrom its ownuseTherapyNavItems, and the remaining seven are single-surface with no secondary nav at all.Tools was assessed and deliberately left single-surface: its five
ToolCatalogAreavalues drive filter chips, labels and icon tones inside the launcher with no area routes, and most catalogue records link out of/toolsentirely — the launcher's own visible filter row already is that navigation.Generated by Claude Code
Summary by CodeRabbit