feat(navigation): standardise multi-page mode menus in the header - #1645
feat(navigation): standardise multi-page mode menus in the header#1645BigSimmo wants to merge 18 commits into
Conversation
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughMode navigation now uses the shared ChangesRegistry mode navigation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #8412 (cancelled). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:4dddce2a2c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ["specifiers", "/specifiers/builder", "Specifiers pages", "Build", "/specifiers/builder"], | ||
| ["formulation", "/formulation/map", "Formulation pages", "Map", "/formulation/map"], |
There was a problem hiding this comment.
These direct PageSecondaryNavigation renders can pass while both workflows remain broken in the application: src/components/clinical-dashboard/global-search-shell.tsx:1004 still refuses to mount this component for specifiers and formulation, while their workflow pages still render local navigation (for example src/components/specifiers/specifier-builder-page.tsx:88 and src/components/formulation/formulation-builder-page.tsx:286). Consequently, a follow-up can satisfy this table yet show no shared header nav—or, after only removing the shell exclusion, stack it with the old subnav. Add an integration/static contract covering the shell and real workflow-page owners so the intended single shared navigation is enforced.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Mounted
PageSecondaryNavigationfor Specifier and Formulation routes through the shared global shell, removing the mode-specific exclusion that prevented their header navigation from appearing. src/components/clinical-dashboard/global-search-shell.tsxL993-L1012 - Routed eligible multi-page workflows through the header-integrated
RegistryModeNav, while retaining local ownership for Therapy Compass and information/detail-page navigation. src/components/page-secondary-navigation.tsxL259-L272 - Suppressed redundant navigation on clean mode landing pages while continuing to enable it on actual Differentials, DSM, Specifier, and Formulation workflow routes. src/lib/mode-secondary-navigation.tsL88-L118
- Removed duplicate route-local Specifier and Formulation subnavs from their home, builder, comparison, and map workflow owners; the builder pages now retain only their contextual breadcrumbs beneath the shared navigation. src/components/specifiers/specifier-builder-page.tsxL83-L89src/components/formulation/formulation-builder-page.tsxL281-L287
- Added a regression contract that verifies the real global shell mounts the navigation and that all eight Specifier/Formulation workflow owners do not mount duplicate local subnavs. tests/page-secondary-navigation.dom.test.tsxL139-L164
- Committed the changes as
196eb4a721218a1335fc97b14523ad826d898bfc(fix(mode-nav): wire shared workflow navigation owners). ⚠️ A pull request could not be created because the requiredmake_prtool is unavailable in this environment and GitHub CLI has no authenticated GitHub session.
Testing
- ✅
npm run workflow:design-sweep -- --write-evidence - ✅
npm run ensure - ✅
curl -fsS http://127.0.0.1:3131/api/local-project-id - ✅
node --input-type=module <<'NODE' …— verified/specifiers/builderrendered exactly onemode-nav, noSpecifier toolsnavigation, and captured/tmp/specifiers-builder-mode-nav.png. - ✅
npm run format - ✅
npm run test -- --run tests/page-secondary-navigation.dom.test.tsx tests/mode-secondary-navigation.test.ts tests/mode-home-loading-contract.test.ts— 3 test files passed; 66 tests passed. - ✅
npm run typecheck:source - ✅
git diff --check ⚠️ gh auth status— GitHub CLI reported that no GitHub host is authenticated, preventing push/PR creation through the CLI.
Co-authored-by: BigSimmo <87357024+BigSimmo@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The updated contracts currently encode/require behavior not yet implemented and include test scaffolding issues (notably the next/link mock needing ref-forwarding) that could make the tests diverge from production behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR introduces test-first contracts for completing the rollout of the shared header-integrated ModeNav across the remaining multi-page modes (Differentials, DSM-5 Diagnosis, Specifiers, Formulation), while ensuring one-destination modes don’t gain redundant secondary menus.
Changes:
- Updates DOM contracts for
PageSecondaryNavigationto assert multi-page workflows render the sharedModeNav(and suppress legacySecondaryNavigationexcept for genuine “On this page” section navigation). - Tightens
isModeSecondaryNavigationRouteexpectations so submitted searches do not automatically imply secondary navigation for one-destination modes.
File summaries
| File | Description |
|---|---|
| tests/page-secondary-navigation.dom.test.tsx | Adds/adjusts DOM contracts for when ModeNav vs SecondaryNavigation should appear. |
| tests/mode-secondary-navigation.test.ts | Updates mode-secondary-navigation contracts around submitted-search behavior and one-destination modes. |
Review details
Suppressed comments (1)
tests/mode-secondary-navigation.test.ts:50
multiPageModescurrently includestherapy-compass, which would make this contract expect a secondary navigation/menu on the Therapy mode home (/therapy-compass) after a submitted search. That conflicts with Therapy’s established behavior (mode home keeps no bar; Therapy owns its nav viaModeNavon its routed pages). Consider excludingtherapy-compassfrom this specific "submitted search on clean landing" expectation to avoid encoding an unintended behavior change.
const multiPageModes = new Set<AppModeId>(["differentials", "dsm", "specifiers", "formulation", "therapy-compass"]);
describe("mode secondary navigation registry", () => {
it("covers all 13 modes with the approved destinations and no Home item", () => {
expect(Object.keys(modeSecondaryNavigationRegistry).sort()).toEqual([...appModeIds].sort());
expect(appModeIds).toHaveLength(13);
for (const modeId of appModeIds) {
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| import { render, screen, waitFor } from "@testing-library/react"; | ||
| import type { ReactNode } from "react"; | ||
| import { beforeEach, describe, expect, it, vi } from "vitest"; | ||
| vi.mock("next/navigation", () => ({ | ||
| usePathname: () => "/", | ||
| })); | ||
| vi.mock("next/link", () => ({ | ||
| default: ({ children, href, ...rest }: { children: ReactNode; href: string }) => ( | ||
| <a href={href} {...rest}> | ||
| {children} | ||
| </a> | ||
| ), | ||
| })); |
BigSimmo
commented
Aug 6, 2026
@copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. |
Addressed in 43a8318. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
BigSimmo
commented
Aug 6, 2026
@codex please apply the exact repository Prettier output to the four files currently failing
After formatting, run the focused format check and preserve all current navigation behaviour and tests. |
Summary
Testing
|
Prettier-format the navigation contracts and drop the onSearch/stickyTop props that the header-integrated mode nav never honours. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/formulation/formulation-ui.tsx`:
- Line 20: Update the Formulation navigation flow around FormulationSubnav and
its callers to accept and pass the current encoded route query string into
RegistryModeNav instead of relying on the empty default, preserving mechanism,
a, b, and template parameters when switching modes. Add a navigation test
covering a URL with selected mechanisms and a template.
In `@src/components/specifiers/specifier-ui.tsx`:
- Line 24: Pass the current encoded route query string through SpecifierSubnav
and all of its callers into RegistryModeNav instead of relying on the empty
default, preserving builder, compare, and map selection parameters when
navigating workflow pages. Add a navigation test covering these three selection
parameters.
🪄 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: 2f3b09e8-6586-421b-8b0c-6ae108be8615
📒 Files selected for processing (9)
src/components/clinical-dashboard/global-search-shell.tsxsrc/components/formulation/formulation-ui.tsxsrc/components/mode-nav/mode-nav-portal.tsxsrc/components/mode-nav/registry-mode-nav.tsxsrc/components/page-secondary-navigation.tsxsrc/components/specifiers/specifier-ui.tsxsrc/lib/mode-secondary-navigation.tstests/mode-secondary-navigation.test.tstests/page-secondary-navigation.dom.test.tsx
| ))} | ||
| </nav> | ||
| ); | ||
| return <RegistryModeNav modeId="formulation" activeId={active} />; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass the current query string to RegistryModeNav.
This call uses the adapter default of "". A Formulation workflow URL with mechanism, a, b, or template loses that state when a user selects another mode-navigation item.
Thread the encoded route query string through FormulationSubnav and its callers. Add a navigation test that starts with selected mechanisms and a template.
🤖 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/formulation/formulation-ui.tsx` at line 20, Update the
Formulation navigation flow around FormulationSubnav and its callers to accept
and pass the current encoded route query string into RegistryModeNav instead of
relying on the empty default, preserving mechanism, a, b, and template
parameters when switching modes. Add a navigation test covering a URL with
selected mechanisms and a template.
| ))} | ||
| </nav> | ||
| ); | ||
| return <RegistryModeNav modeId="specifiers" activeId={active} />; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass the current query string to RegistryModeNav.
This call uses the adapter default of "". A Specifiers workflow URL with selected specifiers loses those selections when the user changes workflow pages.
Thread the encoded route query string through SpecifierSubnav and its callers. Add a navigation test with builder, compare, and map selection parameters.
🤖 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-ui.tsx` at line 24, Pass the current
encoded route query string through SpecifierSubnav and all of its callers into
RegistryModeNav instead of relying on the empty default, preserving builder,
compare, and map selection parameters when navigating workflow pages. Add a
navigation test covering these three selection parameters.
| export function SpecifierSubnav({ active }: { active: "search" | "builder" | "compare" | "map" }) { | ||
| const items = [ | ||
| { id: "search" as const, label: "Find", shortLabel: "Find", href: "/specifiers" }, | ||
| { id: "builder" as const, label: "Build wording", shortLabel: "Build", href: "/specifiers/builder" }, | ||
| { id: "compare" as const, label: "Compare", shortLabel: "Compare", href: "/specifiers/compare" }, | ||
| { id: "map" as const, label: "Map", shortLabel: "Map", href: "/specifiers/map" }, | ||
| ]; | ||
| return ( | ||
| <nav | ||
| aria-label="Specifier tools" | ||
| className="polished-scroll flex max-w-full gap-1 overflow-x-auto rounded-lg border border-[color:var(--border)] bg-[color:var(--surface-raised)] p-1 shadow-[var(--shadow-inset)]" | ||
| > | ||
| {items.map((item) => ( | ||
| <Link | ||
| key={item.id} | ||
| href={item.href} | ||
| aria-label={item.label} | ||
| aria-current={active === item.id ? "page" : undefined} | ||
| className={cn( | ||
| "inline-flex min-h-tap shrink-0 items-center justify-center rounded-md px-3 text-xs font-bold transition focus-visible:outline focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-[color:var(--focus)] sm:text-sm", | ||
| active === item.id | ||
| ? "bg-[color:var(--clinical-accent)] text-[color:var(--clinical-accent-contrast)] shadow-[var(--shadow-tight)]" | ||
| : "text-[color:var(--text-muted)] hover:bg-[color:var(--surface)] hover:text-[color:var(--text)]", | ||
| )} | ||
| > | ||
| <span className="sm:hidden" aria-hidden> | ||
| {item.shortLabel} | ||
| </span> | ||
| <span className="hidden sm:inline" aria-hidden> | ||
| {item.label} | ||
| </span> | ||
| </Link> | ||
| ))} | ||
| </nav> | ||
| ); | ||
| return <RegistryModeNav modeId="specifiers" activeId={active} />; | ||
| } |
There was a problem hiding this comment.
🟡 Selections are dropped when moving between Specifiers or Formulation workflow pages
The Specifiers and Formulation page menus are built without the page's current query string (<RegistryModeNav modeId="specifiers" activeId={active} /> at src/components/specifiers/specifier-ui.tsx:24 and the matching call at src/components/formulation/formulation-ui.tsx:20), so every menu link points at a bare page and the work already in progress is not carried across.
Impact: A clinician who has picked specifiers or mechanisms and then taps Build, Compare or Map from the header menu arrives at an empty page and has to re-pick everything.
Why the state-carrying helper never runs for these two modes
RegistryModeNav defaults searchParamString to "" (src/components/mode-nav/registry-mode-nav.tsx:25) and feeds that into modeSecondaryNavigationHref via new URLSearchParams("") (src/components/mode-nav/registry-mode-nav.tsx:31-43). That helper exists precisely to translate specifier=/a=/b=/selected= and mechanism=/template= into each destination URL (src/lib/mode-secondary-navigation.ts:168-247), but with empty params every branch degrades to the bare registry href.
The shared shell path does pass the bridged query string (src/components/page-secondary-navigation.tsx:279), yet GlobalSearchShell deliberately skips PageSecondaryNavigation for searchMode === "specifiers" | "formulation" (src/components/clinical-dashboard/global-search-shell.tsx:1005), so SpecifierSubnav/FormulationSubnav are the only owners of the bar on those routes and the translation is dead code there. Passing the current search params (e.g. via useSearchParams().toString() in a client wrapper, or threading the page's params down) would restore the intended behaviour.
Prompt for agents
SpecifierSubnav (src/components/specifiers/specifier-ui.tsx) and FormulationSubnav (src/components/formulation/formulation-ui.tsx) now delegate to RegistryModeNav but never supply searchParamString, which defaults to an empty string in src/components/mode-nav/registry-mode-nav.tsx. As a result modeSecondaryNavigationHref (src/lib/mode-secondary-navigation.ts) receives empty URLSearchParams and cannot carry the specifier/mechanism/template/a/b selection state between Find, Build, Compare and Map. Since GlobalSearchShell skips PageSecondaryNavigation for the specifiers and formulation modes, these two Subnavs are the only owners of the mode bar on those routes, so the state-carrying logic never runs at all for them. Consider giving these Subnavs access to the current query string (for example a small client wrapper reading useSearchParams, or threading the page's searchParams through the page components that render them) and passing it to RegistryModeNav, keeping the search-chrome invariant about not introducing a nested Suspense boundary under the standalone shell body in mind.
Was this helpful? React with 👍 or 👎 to provide feedback.
Intent
Complete the rollout started by #1642. Use the shared header-integrated
ModeNavfor every remaining multi-page mode while keeping single-destination modes free of redundant secondary menus.Planned owners:
Preserve Therapy's merged implementation, existing detail-page/local navigation ownership, route semantics, query-state handling, and the old
SecondaryNavigationonly for genuine “On this page” section navigation.Current state
Test-first draft. The branch currently contains only failing contracts that define the intended behaviour. Production implementation follows after CI confirms the expected red signal.
Safety
UI/navigation only. No provider, retrieval, clinical-content, database, schema, auth, or deployment changes.
Summary by CodeRabbit