feat(therapy): standardise every Therapy page on the shared mode nav, centred on desktop - #1642
Conversation
Therapy showed two different secondary menus depending on the route.
`/therapy-compass/search` rendered the shared `ModeNav` tab bar; every other
route rendered `TherapyCompassNav`, a seven-button strip that scrolled
sideways with a visible scrollbar, carried no active underline, and cut
labels off at the screen edge. That was a staged rollout the code itself
described as unfinished — one ternary in `workspace.tsx`.
Finish it, and centre the bar on tablet and desktop.
Navigation
- Seven destinations in one order: search, compare, recommend, pathways,
home, brief, sheets. Order is load-bearing — only the first two slots
survive the narrowest band, so Search and Compare (with its 0/4 fill) stay
on the bar at every width and the rest are reached through More.
- Item ids are `resolveRoute` screen names, typed against `TherapyScreen`, so
`activeId={b.screen}` needs no second pathname parser. Without it Home's
href — the mode base — would prefix-match every Therapy route and claim to
be the current page everywhere.
- Brief Intervention and Patient Sheets resolve real hrefs through new
`briefHref`/`sheetHref` bindings, fed by one helper that `goBrief`/
`goSheets` also push, so a link and its imperative twin cannot disagree.
The generated-asset fallback is no longer gated on the home route: a
`<Link>` needs a real URL for SSR, prefetch and middle-click. These stay
always-navigable by product decision, which keeps the existing fallback to
the first record carrying the artifact when nothing is selected.
- The pill strip, its eight `nav*` class bindings, `navStyle`, and its
`[data-therapy-scroll]` CSS are deleted. Home keeps no bar, as every mode
home does.
Overflow slot
- The More slot no longer borrows the folded page's label. With seven items
the bands leave three destinations permanently in the overflow, which fires
`moreCarriesActive` for the first time in production — and at the 22rem
band that slot's whole budget is ~107px against ~213px for "Brief
Intervention". It keeps the word "More" and its exact width; the active
rule and an `aria-label` of "More — <page>" carry the signal at no layout
cost, so the calibrated 22/33/42rem thresholds need no re-measurement.
Centring
- The bar splits into a full-bleed `.mode-nav-rail` that owns the paint and
the header's gutter, and an inner `.mode-nav` capped to the header's own
column that owns the container query. Measuring the rail would report a
width the slots do not have.
- The gutter reads `--header-edge-pad` minus a new `--mode-nav-ink-offset`,
so the first tab's ink lands on the header's content edge rather than the
viewport's, and the container width is unchanged below `lg` where the two
gutters already agree.
- Therapy page bodies move off bespoke 1240/1180px caps onto `pageContainer`,
so bar, header and body finally share one rail.
Two contract gates moved with the code rather than being dropped:
`check-design-system-contract` now reads `aria-current` from `mode-nav.tsx`,
and `ui-overlay-css-contract` counts `.mode-nav-rail` as a third legitimate
`--header-edge-pad` consumer.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018rXz5oSCvv3vzTeE547dhuScreenshotting the standardised bar caught a defect the tests did not: on Pathways at 390px, nothing in the bar said you were on Pathways. `moreCarriesActive` asked "does the active item have a band at all", which is only true for items that never get a slot. Every page whose slot exists but has not been reached at the current width — Recommend from 33rem, Pathways from 42rem — fell through it. With Therapy's seven destinations that was five of seven pages showing nothing active on a phone. The flag had also never run in production before this rollout, because four items always have a band. Neither layer can answer this alone: the component knows which band would reveal the item, and only the container query knows the band the container is actually in. So the component publishes `data-active-from` — the revealing band, or `none` — and CSS decides, using the same mechanism `data-band` and `data-until` already use for the slots themselves. - More carries the rule and an off-screen name until the container reaches the page's band; at and above it the page's own `aria-current` tab does. Exactly one is marked at any width: never zero, never both. - The name is composed into the accessible name rather than set as an `aria-label`, so `display: none` can take it out of the accessibility tree per band and it is never announced twice. "More" stays the name's prefix (WCAG 2.5.3). - `moreCarriesActive` is gone; the More slot renders one fixed word at one fixed width, so the density thresholds are still untouched. Both new rules are unlayered and exist to beat Tailwind utilities (`bg-transparent` on the rule, `text-muted` on the ink), so they are registered in the style-contract registry with browser-proved effect assertions rather than exempted — `mode-nav__rule` against `--clinical-accent` and `mode-nav__ink` against `--text-heading`, both on the one route where the page never gets a slot. `mode-nav__more` is exempt as a scoping ancestor. ui-mode-nav-density gains a sweep over all five reachable destinations at all three bands asserting exactly one painted rule and at most one announced name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rXz5oSCvv3vzTeE547dhu
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:48 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 (2)
📝 WalkthroughWalkthroughTherapy Compass now uses shared ChangesTherapy ModeNav foundation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
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.
…ations Two review findings from CodeRabbit, both about tests rather than product code. The Therapy portal contract was negative-only: it asserted the legacy PhoneHeaderCollapsePortal was absent and that the shared portal file contained the right internals, so a nav.tsx that had dropped navigation entirely would still have passed. It now asserts the chain that actually exists — Therapy renders <ModeNav, and ModeNav is what claims the header slot. Therapy never names the portal itself, so the positive half has to follow that hop; asserting ModeNavHeaderPortal directly against nav.tsx (as the review suggested) matches zero occurrences and fails. Both halves were mutation-checked: removing either turns the contract red. ui-mode-nav-density covered Brief Intervention but not Patient Sheets, though both are permanently folded and reach the More slot the same way. The overflow-slot width test and the marks-the-current-page sweep now drive both, so the second-longest label in the set is proved never to leak into the bar. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rXz5oSCvv3vzTeE547dhu
…ion-g04g2x' into claude/therapy-menu-standardization-g04g2x
Uh oh!
There was an error while loading. Please reload this page.
…rmulation and Differentials (#1647) * feat(navigation): put DSM on the shared mode bar, and state addon-slot ownership Starts the site-wide rollout of the header-integrated `ModeNav` that #1642 shipped for Therapy. DSM first: two destinations, so no More slot and no overflow band to calibrate. `RegistryModeNav` adapts the canonical `modeSecondaryNavigationRegistry` to `ModeNavItem` rather than each mode hand-writing a list, so the shell and the mode cannot drift onto different URLs for the same tab. Its icon map is exhaustive by a type derived from the registry literal: adding a routed entry without choosing an icon fails the typecheck instead of shipping a document icon nobody picked. Also closes an invariant that until now held by accident. The header addon slot takes ONE page-owned header, and what kept `documents` and `differentials` clear was that both had fewer than two destinations, so `ModeNav` rendered nothing. That protection expires the moment a claimant's mode gains one. `/differentials/diagnoses/<slug>` is the live case: it is locally-owned information navigation, so it skips the sections branch, and a submitted search makes `isModeSecondaryNavigationRoute` true ahead of every per-mode check — mounting a second header into a slot `DifferentialDetailPage` owns. `isHeaderAddonSlotOwnedRoute` now names the claimants, and the render-level test asserts one occupant rather than inferring it from the item count. Adoption is an explicit per-mode list, not a "has two routed entries" derivation, so a registry edit cannot silently move a mode onto a different navigation surface. The eight single-destination modes keep their existing `SecondaryNavigation` strip untouched — dropping it is a deletion decision about their lone entry, not part of this port. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S9mA7QhMw1hpZCVMzH7pZL * feat(navigation): move Specifiers and Formulation onto the shared mode bar Replaces the two page-owned strips (`SpecifierSubnav`, `FormulationSubnav`) with the header-integrated bar, and drops the shell exclusion that skipped `PageSecondaryNavigation` for both modes — that exclusion existed only to stop a duplicate row next to those strips, and with the strips gone it would have suppressed the modes' only navigation. Desktop labels shorten: "Build wording" / "Build formulation" / "Mechanism map" become the registry's "Build" / "Map". `ModeNavItem` has no `shortLabel` by design (a slot shows its real word or folds into More), and the registry is now the single source of the wording. Lengthening it instead was the alternative, but "Build formulation" plus "Mechanism map" do not fit the calibrated 42rem band, and ledger #113 is explicit that the thresholds are measured, not padding to be widened. Both mode homes go bar-free, as every mode home is: `/specifiers` and `/formulation` already surface builder, compare and map as tiles, so nothing is stranded by the removal. Their `[slug]` detail routes keep the information-page sections nav they share with every other record route, rather than the mode strip they used to carry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S9mA7QhMw1hpZCVMzH7pZL * feat(navigation): move Differentials onto the shared mode bar Completes the four multi-destination modes. Differentials is last because it is the only one with a route that portals its own header into the universal header's single addon slot, and that slot takes one occupant. The protection there was never stated. It used to be that `documents` and `differentials` — the only modes with a claimant — had fewer than MODE_NAV_MIN_ITEMS destinations, so `ModeNav` rendered nothing; Differentials now has three. What holds it today is a second coincidence: every claimant route is also `hasLocalInformationPageNavigation`, which `PageSecondaryNavigation` returns null on well before the mode branch. Two separately maintained lists that happen to agree. Rather than add a guard that cannot execute, `isHeaderAddonSlotOwnedRoute` names the claimants and the test asserts the agreement route for route, plus the render-level outcome. Breaking the cover fails three assertions; the earlier explicit guard failed none, which is what showed it was unreachable. When a future claimant falls outside the cover, that is the signal the mode branch needs a guard of its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S9mA7QhMw1hpZCVMzH7pZL * test(navigation): drive the mode-nav density spec over every adopting mode The spec's header promises that a mode whose labels are longer than the thresholds allow fails there rather than shipping clipped words. Every route it drove was `/therapy-compass/*`, so that promise covered exactly one mode. Four more have now adopted the bar; their labels are short enough to pass either way, which is why the gap survives until a mode with long ones arrives. Each mode is loaded at every band boundary and one pixel either side, with the expected slot count derived as `min(items, capacity)` rather than tabulated, so a mode with a different destination count needs no second table. Two cases are new behaviour, not just new coverage: Specifiers and Formulation carry four destinations, so at the 22rem band their last item folds and the More slot has to carry the current page's rule. That is invariant I5's container-query branch, and no route outside Therapy exercised it before. `tests/mode-nav-contract.test.ts` pins the spec's mode table against MODE_NAV_ADOPTED_MODES and the registry, so adopting a mode or adding a destination without extending this spec fails offline instead of leaving a mode unmeasured. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S9mA7QhMw1hpZCVMzH7pZL * fix(navigation): restore record-page section anchors for mode-nav adoption (#1647) Wire the declared specifier/formulation information-section IDs into the record pages so "On this page" actually renders after Subnav removal. Also pin MODE_NAV_ADOPTED_MODES to the ≥2 routed-entry criterion, and stop marking Find/Search current on unmatched in-mode routes. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * test(mode-nav): always cleanup addon-slot DOM node after each case Co-authored-by: BigSimmo <87357024+BigSimmo@users.noreply.github.com> * test(navigation): pin which modes adopt the bar, not just the criterion The adoption test asserted that every listed mode has two or more routed entries, and that modes with fewer stay off the list. Neither half notices a mode being removed: drop `formulation` and the remaining three still satisfy the criterion, while the negative check only inspects modes with fewer than two entries. A mode silently losing its only navigation is precisely the regression the explicit list exists to prevent, so pin the membership as well. Raised in review. Confirmed by mutation: removing `formulation` passed before this, and fails after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S9mA7QhMw1hpZCVMzH7pZL * fix(test): address the formulation section label by id, not by its words `Production UI` caught this; my local run of the spec predated the commit that caused it. Wiring up the declared section anchors made `/formulation/<slug>` render an "On this page" nav, and its link carries the same words as the record's own section label — so `getByText("What matters now", { exact: true })` now resolves to two elements and fails Playwright strict mode. The assertion wants the record's label, so address it by `#formulation-what-matters-now-label` rather than by text that two elements legitimately share. `ui-specifiers.spec.ts:93` uses the same text but is not affected: "What matters now" is not one of the specifier section labels, so no nav link carries it there. Confirmed by running both specs, 12 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S9mA7QhMw1hpZCVMzH7pZL * docs(ledger): record PR #1647 mode-nav review at HEAD * fix(navigation): match mode-nav active ids by path segment (#1647) Specifier/Formulation active-slot detection used pathname.includes for builder/compare/map, so a future slug containing those substrings would falsely claim aria-current. Align with the exact-segment checks already used by isModeSecondaryNavigationRoute. * fix(test): pin formulation On-this-page link and specifier label by id Production UI failed when getByText("What matters now") matched both the formulation record label and the newly-wired On this page nav link. The tip already addressed the label by id; assert the nav link through its own role scope so the dual presence stays intentional and cannot regress into a second strict-mode collision. Mirror the id-scoped locator on the specifier record path so that suite does not depend on the current coincidence that specifier section labels omit those words. * ci: retrigger required checks after Actions outage (#1647) Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Summary
Therapy showed a different secondary menu depending on which page you were on.
/therapy-compass/searchrendered the sharedModeNavtab bar — compact tabs, an active underline, and a More ⌄ control opening a "Therapy pages" sheet. Every other Therapy route renderedTherapyCompassNav, a seven-button strip that scrolled sideways with a visible scrollbar, carried no active underline, and cut labels off at the screen edge. That was not two designs: it was a staged rollout the code itself described as unfinished, gated on one ternary inworkspace.tsx. This finishes it and centres the bar on tablet and desktop.0/4fill) stay on the bar at every width, and everything after Pathways is reached through More. The pill strip, its eightnav*class bindings,navStyle, and its[data-therapy-scroll]CSS are deleted. The mode home keeps no bar, as every mode home does.resolveRoutescreen names typed against a new exportedTherapyScreen, soTherapyModeNavpassesactiveId={b.screen}with no second pathname parser. Without that, Home's href is the mode base and would prefix-match every Therapy route, claiming to be the current page everywhere.detailandreviewmatch no item, which is the correct "nothing active" for a record and a governance queue.ModeNavtakes an href and never an onClick, so the target resolution moved out of the imperative handlers into newbriefHref/sheetHrefbindings fed by one shared helper thatgoBrief/goSheetsalso push — a link and its imperative twin can no longer disagree about where they go. The generated-asset fallback is no longer gated on the home route, because a<Link>needs a real URL for SSR, prefetch and middle-click. These two stay always-navigable by product decision, which keeps the existing fallback to the first catalogue record carrying the artifact when nothing is selected; the availability-guarded buttons on the record page remain the safe route to a specific therapy.moreCarriesActivebranch for the first time in production. At the 22rem band that slot's entire budget is ~107px against ~213px for "Brief Intervention", so borrowing the label overflows the bar. It keeps the word "More" and its exact width instead. Screenshotting the result then caught a second defect the tests had not:moreCarriesActiveasked "does the active item have a band at all", which is only true for items that never get a slot — so Recommend (band 4) and Pathways (band 5) marked nothing on a phone, five of seven pages in total. Neither layer can answer that alone, because the component knows which band would reveal the item and only the container query knows the band in force. The component now publishesdata-active-fromand CSS decides, using the same mechanismdata-bandanddata-untilalready use for the slots. Exactly one of the page's own tab and the More slot is marked at any width — never zero, never both — and the page name is composed into More's accessible name rather than set as anaria-label, sodisplay: nonetakes it out of the accessibility tree at the widths where the real tab is showing..mode-nav-railthat owns the paint and the header's inline gutter, and an inner.mode-navcapped to the header's own column that owns the container query — measuring the rail would report a width the slots do not have (1920px read against a 1312px bar). The gutter reads--header-edge-padminus a new--mode-nav-ink-offset, so the first tab's ink lands on the header's content edge rather than the viewport's, and the container width is unchanged belowlgwhere the two gutters already agree. Therapy page bodies move off bespokemax-w-[1240px]/max-w-[1180px]caps onto the canonicalpageContainer, so bar, header and body finally share one rail.check-design-system-contract's "Therapy navigation needs aria-current" now readsmode-nav.tsx, where those semantics live after the move;ui-overlay-css-contractcounts.mode-nav-railas a third legitimate--header-edge-padconsumer. The two new unlayered rules exist specifically to beat Tailwind utilities (bg-transparenton the rule,text-mutedon the ink), sostyle-contract-registrydemanded a decision — they are registered inSTYLE_EFFECT_CONTRACTSwith browser-proved computed-style assertions against--clinical-accentand--text-heading, not waved through as exemptions.mode-nav__moreis exempt as a scoping ancestor that carries no visual property of its own.Verification
npm run verify:pr-local— run stage by stage, because the gate's own first stage cannot pass in this container (see below).check:runtimePASS;format:changed"All matched files use Prettier code style!";lintclean;typecheckclean;test510 files, 5392 passed, 4 skipped;check:rag:fixtures"Offline RAG fixture and manifest validation passed (36 golden cases, 23 suites)".npm run verify:uiwhen UI, routing, styling, browser behavior, reduced-motion, or forced-colors behavior changed — run as its underlyingtest:e2e:prChromium project: 353 passed, 2 failed, and both failures reproduce unchanged on the base commitd04ec9cwith the same assertions (ui-pwamanifest/icons/root-worker, andui-smoke's mobile PDF-canvas preview). Neither is on a Therapy route and neither touches the mode nav; both sit in the areas a mismatched Chromium build bites first. Focused evidence:ui-mode-nav-density.spec.ts15 passed, covering three folded-destination cases at 352/528/672px, a three-band × five-destination "marks the current page exactly once" sweep, and a desktop ink-alignment assertion at 1024/1440/1920px;ui-style-contract.spec.ts7 passed, including the two new computed-effect contracts;ui-therapy-nav-scroll.spec.ts1 passed; the phone-chrome changed-browser stage (ui-phone-scroll*+ui-therapy-nav-scroll) 57 passed; its contracts stage 116 passed and focused-browser stage 6 passed.npm run verify:releasebefore release or handoff confidence claimsVerification not run:
verify:releaseand every provider-backed gate — no OpenAI, Supabase, or CI surface is touched by this diff, and they require explicit authorization.UI verification not run as a single command:
verify:phone-chromeandverify:uiboth abort at theircheck:installed-lock-paritystage because this container has playwright 1.62.0 installed against a locked 1.62.1, and ships Chromium build 1194 where 1.62.1 expects 1234.package.jsonandpackage-lock.jsonare untouched by this change, so this is a pre-existing environment gap rather than a regression. Every stage those gates would have selected was run directly withPLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH=/opt/pw-browsers/chromium; the coverage is real but ran on Chromium 1194 rather than the pinned build, so CI is the authoritative run.Also run and clean:
check:design-system-contract(53 components, 55 roots; design-sync 53 components and 7 guidelines),check:type-scale,check:icon-scale.Visual confirmation captured at 390px, 1024px and 1440px on
/therapy-compass/pathwaysand the folded-destination brief route: the bar is identical to the Search page's on every route, "More" carries the underline when the current page is inside it, the sheet lists all seven pages with the current one marked, and on desktop the first tab's ink sits on the content column rather than the viewport edge.Risk and rollout
#113showed that borrowing width in this bar clips labels silently, with nothing overflowing and nothing failing. That is why the More slot's width is held constant by design rather than by retuned thresholds, and whyui-mode-nav-density.spec.tsnow drives a folded-destination route at all three band boundaries. The deliberate behaviour change worth a reviewer's attention is Brief Intervention and Patient Sheets remaining always-navigable: with nothing selected they open the first catalogue record carrying that artifact, which is a therapy the reader was not looking at. That was the existing strip's behaviour, it was retained on request, andtherapy-compass-artifact-navigation.dom.test.tsxnow pins the resolution order so it is explicit rather than incidental.git revertthe two commits (the standardisation, then the current-page marking fix on top of it); reverting only the second leaves a coherent bar that simply does not mark a folded page. No migration, no generated asset, no persisted state, and no stored user preference is involved;ModeNavis untouched by any other mode, so the blast radius is the Therapy routes plus the shared bar's own markup.RAG impact: no retrieval behaviour change — Therapy navigation, layout and page-container width only; no file under
src/lib/rag/**, clinical-search, retrieval-selection, ranking-config, answer-ranking, the eval harness, or the golden fixture is modified.classifyPullRequestFilesreportsragRanking: false,clinicalRisk: false,operationalRisk: false,ui: true.Notes
classifyPullRequestFilesreturnsclinicalRisk: falsefor this diff. It touches no ingestion, answer generation, search/ranking, source rendering, document access, privacy, production env, or clinical output path — the record-scoped destinations change how they are reached, not what they render or which sources back it.src/lib/mode-secondary-navigation.tsstates that the global/sidebar navigation owns mode homes so a secondary bar must never repeat a generic Home destination, andtests/mode-secondary-navigation.test.tsenforces that for all 13 modes' registry. That test covers the registry, notModeNav, so nothing goes red — and Home sits at position 5, so it only ever appears inside the More sheet and never occupies a tab. Flagged rather than hidden.modeSecondaryNavigationRegistry["therapy-compass"]is left as data and gains only a comment. It has been inert sincePageSecondaryNavigationbegan early-returning on/therapy-compass*, and its labels are pinned by a 13-mode contract test; reconciling it belongs with any widerModeNavadoption, not here.ModeNav's sole consumer; every other mode keepsSecondaryNavigation. Eight modes register a single destination and would render no bar at all underMODE_NAV_MIN_ITEMS, so the real adoption candidates aredsm,differentials,specifiersandformulation— and two of those include detail pages that already claim the phone header addon slot, whichtests/mode-nav-contract.test.tsguards as single-owner.Generated by Claude Code
Summary by CodeRabbit
New Features
Accessibility
Style