feat(navigation): adopt the shared mode nav for Factsheets - #1674
Merged
Conversation
Factsheets is the one of the eight remaining modes with a genuine second destination: `/factsheets` is the browse home (category chips + featured grid) and `/factsheets/search` is a separate component with filters, a view toggle and result rows. It registered a single action-only entry, which rendered one <button> whose only effect was focusing a composer already visible on the same screen. Replace that with two routed entries — Topics and Search — and add the mode to MODE_NAV_ADOPTED_MODES. Two items fit the 22rem band with no overflow slot, the same shape as DSM. Details worth knowing: - Topics carries no `focus: true`, unlike the Search/Find entry of every other adopted mode. Those tabs are their mode's search affordance; Topics is a browse destination, and autofocusing would open the phone keyboard over the topics the user asked to see. - The Search tab carries q, category and run. It is the current tab on /factsheets/search, and dropping `run` flips hasSubmittedModeSearch, which re-places the composer for a click that changed nothing. - factsheets gets its own activeModeSecondaryNavigationId branch. The array-index-0 fallback would otherwise mark Topics current on every unmatched path. - The icon is BookOpenText, the Factsheets hero glyph, not LayoutGrid — the search page already uses LayoutGrid for its card/list view toggle. Adopting expires the incidental protection that kept the bar off /factsheets/[slug] (it had fewer than MODE_NAV_MIN_ITEMS destinations). What keeps that route clear now is only the hasLocalInformationPageNavigation early return, so it is pinned at render rather than left to the count. Verification: typecheck and lint clean; 5 focused unit files (95 tests) pass; full offline suite 518/519 files, with the single failure (tests/pr-handoff-stop.test.ts) confirmed pre-existing by re-running it with these changes stashed. ui-mode-nav-density: 55 passed, including 7 new factsheets rows showing both labels in full at every band >=352px and collapsing at 351px. Both new cross-file pins were mutation-checked: removing the density table row fails 2 tests in mode-nav-contract, and dropping the `run` carry fails the href test. 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. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 50 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 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
Comment |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01823Ctwj4vinGhGuRNyK7oE
BigSimmo
enabled auto-merge (squash)
August 7, 2026 10:45
2 tasks
BigSimmo
added a commit
that referenced
this pull request
Aug 7, 2026
* refactor(navigation): remove the vestigial one-button mode strip 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 * docs(ledger): record the vestigial mode-strip removal review (PR #1679) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01823Ctwj4vinGhGuRNyK7oE --------- Co-authored-by: Claude <noreply@anthropic.com>
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ModeNav. It is the one of the eight remaining modes with a genuine second destination:/factsheetsis the browse home (category chips + featured grid) and/factsheets/searchis a separate component with filters, a card/list toggle and result rows. It previously registered a single action-only entry, which rendered one<button>inside a<nav>landmark whose only effect was focusing a composer already visible on the same screen. That is replaced by two routed entries — Topics and Search — plusfactsheetsinMODE_NAV_ADOPTED_MODES. Two items fit the 22rem band with no overflow slot, the same shape as DSM.focus: true, unlike the Search/Find entry of every other adopted mode. Those tabs are their mode's search affordance, so focusing the composer on arrival is the point. Topics is a browse destination —focus=1reachesinputRef.current?.focus()and would open the phone keyboard over the topics the user just asked to see. This mode's search affordance is the Search tab.q,categoryandrun. It is the current tab on/factsheets/search, so its own link must not reset the filter being read.runmatters specifically:hasSubmittedModeSearch = requestedRun && requestedQuery.length > 0drivesuseCompactBottomSearch, so dropping it re-places the composer for a click that changed nothing.factsheetsgets its ownactiveModeSecondaryNavigationIdbranch. Theregistry[modeId][0]?.idfallback would otherwise mark Topics current on every unmatched path, including/factsheets/search.BookOpenText, the Factsheets hero glyph — notLayoutGrid, which the search page already uses for its card/list view toggle. One glyph must not mean two things on one screen./factsheets/[slug]. It was bar-free because the mode had fewer thanMODE_NAV_MIN_ITEMSdestinations. What keeps it clear now is only thehasLocalInformationPageNavigationearly return, so that is pinned at render rather than left to the count.No new routes, so no new-route checklist work: all three factsheets routes are already in
docs/site-map.mdand the home already links to/factsheets/search.Three separate places pin the adopted-mode list and all three move together —
tests/mode-secondary-navigation.test.ts,tests/mode-nav-addon-slot.dom.test.tsx, and theMODEStable intests/ui-mode-nav-density.spec.ts(cross-checked bymode-nav-contract). This is the guard that makes the PR #1645 silent-deletion failure impossible; it did not exist when that PR was written.RAG impact: no retrieval behaviour change — navigation and layout only.
Verification
npm run verify:pr-localVerification not run:
verify:pr-localaborts in this container 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 build 1194 is present and browser downloads are disabled, which would stop the density spec running at all. The gate's components were run directly instead:npm run lint— clean.npm run typecheck— clean. This is the gate that catches a missingiconByItemIdentry; the map is exhaustive by a type derived from the registry literal, with no fallback.npm run test—Test Files 1 failed | 518 passed (519),Tests 1 failed | 5491 passed | 4 skipped (5496). The single failure istests/pr-handoff-stop.test.ts:180, which exercises a shell hook untouched by this diff; confirmed pre-existing by re-running it with these changes stashed (1 failed | 10 passedon the clean tree).Focused set —
tests/mode-secondary-navigation.test.ts,mode-nav-contract,mode-nav-addon-slot.dom,page-secondary-navigation.dom,secondary-navigation.dom:Test Files 5 passed (5),Tests 95 passed (95).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):
node scripts/run-playwright.mjs tests/ui-mode-nav-density.spec.ts --project=chromium→55 passed (1.1m), including 7 newfactsheetsrows. Both labels render in full at 352/353/527/528/529/672px and the bar collapses to the single control at 351px, so nothing is clipped and nothing overflows.Both new cross-file pins were mutation-checked rather than assumed:
factsheetsrow from the densityMODEStable →Tests 2 failed | 22 passedinmode-nav-contract.test.ts.runcarry from the href case →Tests 1 failed | 30 passedinmode-secondary-navigation.test.ts.Risk and rollout
ModeNavitself is unmodified. The one behaviour worth naming: the bar renders on/factsheets/search, and on/factsheetsonly via thehasSubmittedSearchearly return — the clean browse home keeps showing itsModeHomeTemplatetiles instead, per the existing "a mode home renders no bar" rule. Since a factsheets search from the composer routes to/factsheets/search, Topics will rarely be the marked-current tab in practice; it functions as the return path to browse. This is precedented (DSM's Search tab points at/dsm?focus=1, also outside its route predicate) but is called out because a two-tab bar where one tab is usually the current one is new for this registry.topicsid, and removing it narrowsRoutedModeSecondaryNavigationIdback automatically.Clinical Governance Preflight
Not applicable, and verified rather than assumed against
scripts/pr-policy.mjs:src/lib/mode-secondary-navigation.tsdoes not match theclinicalRiskPatternssrc/lib/…clause (it requires one of auth/permission/privacy/security/rag/retriev/rank/search/answer/clinical/citation/source/document/upload/download in the path), and themode-nav/page-secondary-navigationcomponent paths do not match the presentation clause, which is explicitly written so that living under a clinically-named directory is not by itself clinical-risk. Nothing matchesragRankingPatterns. The diff touches navigation wiring only — no ingestion, answer generation, ranking, source rendering, document access, privacy or production config.Notes
This is the first of two PRs from the mode-navigation handover. The second removes the vestigial one-button strip from the seven modes that stay single-surface (
answer,documents,services,forms,favourites,prescribing,tools) and must land after this one — reversing the order would strand/factsheets/searchwith no navigation at all.Tools was assessed and deliberately not adopted: 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