Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/branch-review-ledger.md
Original file line number Diff line number Diff line change
Expand Up @@ -698,3 +698,4 @@ Records before 2026-07-28 were written by hand and had drifted: 146 lines carrie
| 2026-08-07 | cursor/grok-quick-wins-a2c0 (PR #1651) | 2563b4adf4f0c0580ca371acfee94f9ae1ca3d0f | prlanded | MERGED; squash tip empty vs branch tip 4655643770dba7ca4078f1586c0461fd91657bf9; Grok-safe quick wins (tokens, a11y, perf, gates) | content tree empty vs squash; no provider-backed checks run |
| 2026-08-07 | cursor/viewer-phase0-gesture-a11y-1db8 (PR #1660) | 097dfd245f798f8105eeb6c1cf4fc077f969496f | prlanded | MERGED; squash tip empty vs branch tip 810cfc9b4a1c476a0dcc995bffb00d7329a85686; Phase 0 gesture INP, preview a11y, image decode | content tree empty vs squash; no provider-backed checks run |
| 2026-08-07 | claude/pr-handoff-loop-prevention-54y5zr (PR #1670) | dfe2946110e0ff93bd4acc571ae79c26b79a7a85 | PR #1670 heavy review-and-fix | synced origin/main (behind-but-clean DIRTY cleared); fixed CodeRabbit checks-cell to name exact #1649 gates + incomplete verify:pr-local/ui + no provider checks; Bugbot none; no P0/P1; #258/#ledger delta accurate; merge-tree clean; threads cleared | verify:cheap 519 files/5493 passed; verify:pr-local docs scope (format+docs+ledger+outstanding-issues); check:branch-review-ledger; check:outstanding-issues; no provider gates |
| 2026-08-07 | claude/handover-review-nlhuln | 978623337c12dc1721fe5236eadbf9a5ad929f03 | mode nav remaining modes: factsheets adoption (PR #1674) | Adopted the shared ModeNav for factsheets (Topics + Search); replaced the action-only entry, added the activeId branch, q/category/run carry, BookOpenText icon; three pinned adopted-mode lists updated together; record-route protection pinned at render now the item-count protection has expired | lint clean; typecheck clean; test 518/519 files (pr-handoff-stop failure confirmed pre-existing via stashed re-run); focused 5 files 95 tests; ui-mode-nav-density 55 passed incl 7 new factsheets rows; two mutation checks confirmed red; format committed; verify:pr-local blocked at check:installed-lock-parity (playwright 1.62.0 vs 1.62.1) |
15 changes: 14 additions & 1 deletion src/components/mode-nav/registry-mode-nav.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
"use client";

import { GitCompareArrows, ListChecks, Network, Search, Stethoscope, type LucideIcon } from "lucide-react";
import {
BookOpenText,
GitCompareArrows,
ListChecks,
Network,
Search,
Stethoscope,
type LucideIcon,
} from "lucide-react";

import { ModeNav, type ModeNavItem } from "@/components/mode-nav/mode-nav";
import { appModeDefinition, type AppModeId } from "@/lib/app-modes";
Expand All @@ -23,6 +31,11 @@ const iconByItemId: Record<RoutedModeSecondaryNavigationId, LucideIcon> = {
compare: GitCompareArrows,
builder: ListChecks,
map: Network,
// The Factsheets hero glyph (`factsheets-home-page.tsx`), so the tab wears the
// same mark as the surface it points at. Not LayoutGrid: the search page uses
// that for its card/list view toggle, and one glyph must not mean two things
// on the same screen.
topics: BookOpenText,
};

/**
Expand Down
45 changes: 44 additions & 1 deletion src/lib/mode-secondary-navigation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,20 @@ export const modeSecondaryNavigationRegistry = {
{ id: "brief", label: "Brief Intervention", action: "therapy-brief" },
{ id: "sheets", label: "Patient Sheets", action: "therapy-sheets" },
],
factsheets: [{ id: "search", label: "Search", action: "search" }],
// Two genuinely distinct surfaces: `/factsheets` is the browse home (category
// chips + a featured grid) and `/factsheets/search` is a separate component
// with filters, a view toggle and result rows. `/factsheets/[slug]` is a
// record and never reaches here — `hasLocalInformationPageNavigation` returns
// null for it first.
// No `focus: true` on Topics, unlike the Search/Find entry of every mode
// above. Those tabs are the mode's search affordance, so focusing the composer
// on arrival is the point. Topics is a browse destination — autofocusing there
// would open the phone keyboard over the topics the user asked to see. The
// search affordance for this mode is the Search tab.
factsheets: [
{ id: "topics", label: "Topics", href: appModeHomeHref("factsheets") },
{ id: "search", label: "Search", href: "/factsheets/search" },
],
} as const satisfies Record<AppModeId, readonly ModeSecondaryNavigationEntry[]>;

type RegistryEntry = (typeof modeSecondaryNavigationRegistry)[AppModeId][number];
Expand Down Expand Up @@ -94,6 +107,7 @@ export const MODE_NAV_ADOPTED_MODES = [
"specifiers",
"formulation",
"differentials",
"factsheets",
] as const satisfies readonly AppModeId[];

export function modeUsesHeaderModeNav(modeId: AppModeId): boolean {
Expand Down Expand Up @@ -139,6 +153,14 @@ export function activeModeSecondaryNavigationId(modeId: AppModeId, pathname: str
if (pathname === `/${modeId}` || pathname.startsWith(`/${modeId}?`)) return "search";
return null;
}
if (modeId === "factsheets") {
if (pathname === "/factsheets/search" || pathname.startsWith("/factsheets/search?")) return "search";
if (pathname === "/factsheets" || pathname.startsWith("/factsheets?")) return "topics";
// `/factsheets/<slug>` is a record. It cannot reach `ModeNav` today —
// `hasLocalInformationPageNavigation` returns null for it first — but the
// array-index-0 fallback below would mark Topics current if it ever did.
return null;
}
return modeSecondaryNavigationRegistry[modeId][0]?.id ?? null;
}

Expand All @@ -165,6 +187,10 @@ export function isModeSecondaryNavigationRoute(params: {
pathname === "/formulation/builder" || pathname === "/formulation/compare" || pathname === "/formulation/map"
);
}
// Same shape as `dsm` above: list the routed destination that is not the mode
// home. The clean `/factsheets` home stays out so its `ModeHomeTemplate` tiles
// remain the single answer to "where can I go"; it reaches the bar through the
// `hasSubmittedSearch` early return, and Topics is marked current there.
if (modeId === "factsheets") return pathname === "/factsheets/search";
if (modeId === "therapy-compass") return pathname !== "/therapy-compass";
return false;
Expand Down Expand Up @@ -284,5 +310,22 @@ export function modeSecondaryNavigationHref(params: {
]);
}

if (modeId === "factsheets") {
// Search carries the live query and category filter so switching tabs does
// not silently discard them. Topics goes to the clean browse home: it reads
// neither param, so appending them would only produce a misleading URL.
if (itemId !== "search") return href;
const category = currentSearchParams.get("category");
return navigationHrefWithParams(href, [
...(query ? ([["q", query]] as const) : []),
...(category ? ([["category", category]] as const) : []),
// `run` travels with the query, as it does for dsm. Search is the current
// tab on /factsheets/search, and dropping `run` from its own link flips
// `hasSubmittedModeSearch` (global-search-shell.tsx:421) to false, which
// re-places the composer — a layout jump from clicking where you already are.
...(query && currentSearchParams.get("run") === "1" ? ([["run", "1"]] as const) : []),
]);
}

return href;
}
8 changes: 7 additions & 1 deletion tests/mode-nav-addon-slot.dom.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,13 @@ describe("header addon slot ownership", () => {
// `documents` owns the slot on every detail route and has one registered
// destination. Adopting it would be a deletion decision about that lone
// entry, not a port — deliberately out of this rollout's scope.
for (const modeId of ["documents", "answer", "prescribing", "tools", "factsheets"] as const) {
//
// `factsheets` left this list when it gained a real second destination:
// `/factsheets` (browse) and `/factsheets/search` are separate components,
// so it is a port rather than a deletion. The others still have one surface
// each, and ModeNav renders nothing below two items — adopting them would
// remove the control they have and put nothing back (PR #1645).
for (const modeId of ["documents", "answer", "prescribing", "tools"] as const) {
expect([...MODE_NAV_ADOPTED_MODES]).not.toContain(modeId);
expect(modeUsesHeaderModeNav(modeId)).toBe(false);
}
Expand Down
51 changes: 49 additions & 2 deletions tests/mode-secondary-navigation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ const expectedLabels: Record<AppModeId, string[]> = {
prescribing: ["Search"],
tools: ["Search"],
"therapy-compass": ["Search", "Recommend", "Compare", "Pathways", "Brief Intervention", "Patient Sheets"],
factsheets: ["Search"],
factsheets: ["Topics", "Search"],
};

const cleanLandingPath: Record<AppModeId, string> = {
Expand Down Expand Up @@ -134,6 +134,39 @@ describe("mode secondary navigation registry", () => {
currentSearchParams: new URLSearchParams("q=confusion&ids=delirium%2Cdementia"),
}),
).toBe("/differentials/presentations?q=confusion&ids=delirium%2Cdementia");

// Search is the CURRENT tab on /factsheets/search, so its own link must not
// reset what you are looking at. `run` is carried with the query because
// dropping it flips hasSubmittedModeSearch and re-places the composer.
expect(
modeSecondaryNavigationHref({
modeId: "factsheets",
itemId: "search",
href: "/factsheets/search",
currentSearchParams: new URLSearchParams("q=sertraline&category=Medicines&run=1"),
}),
).toBe("/factsheets/search?q=sertraline&category=Medicines&run=1");

// The browse home's category chips link with a category and no query.
expect(
modeSecondaryNavigationHref({
modeId: "factsheets",
itemId: "search",
href: "/factsheets/search",
currentSearchParams: new URLSearchParams("category=Medicines"),
}),
).toBe("/factsheets/search?category=Medicines");

// Topics is the mode home: it reads neither param, so carrying them there
// would only put dead query string into a URL people share.
expect(
modeSecondaryNavigationHref({
modeId: "factsheets",
itemId: "topics",
href: "/factsheets",
currentSearchParams: new URLSearchParams("q=sertraline&category=Medicines&run=1"),
}),
).toBe("/factsheets");
});

it("adopts only modes with two or more routed destinations (explicit list, not silent derivation)", () => {
Expand All @@ -142,7 +175,13 @@ describe("mode secondary navigation registry", () => {
// remaining mode still has two routed entries, while the negative check
// below only inspects modes with fewer than two. A mode silently losing the
// bar is the regression this list exists to make impossible.
expect([...MODE_NAV_ADOPTED_MODES].sort()).toEqual(["differentials", "dsm", "formulation", "specifiers"]);
expect([...MODE_NAV_ADOPTED_MODES].sort()).toEqual([
"differentials",
"dsm",
"factsheets",
"formulation",
"specifiers",
]);

for (const modeId of MODE_NAV_ADOPTED_MODES) {
expect(
Expand All @@ -165,6 +204,14 @@ describe("mode secondary navigation registry", () => {
expect(activeModeSecondaryNavigationId("dsm", "/dsm/diagnoses/major-depressive-disorder")).toBeNull();
expect(activeModeSecondaryNavigationId("specifiers", "/specifiers/builder")).toBe("builder");
expect(activeModeSecondaryNavigationId("specifiers", "/specifiers")).toBe("search");

// Factsheets records cannot reach ModeNav today (hasLocalInformationPageNavigation
// returns null for them first), but the registry fallback would mark the
// first entry — Topics — current on any unmatched path, so the mode needs
// its own branch rather than inheriting that default.
expect(activeModeSecondaryNavigationId("factsheets", "/factsheets/sertraline")).toBeNull();
expect(activeModeSecondaryNavigationId("factsheets", "/factsheets")).toBe("topics");
expect(activeModeSecondaryNavigationId("factsheets", "/factsheets/search")).toBe("search");
});

it("matches workflow destinations by path segment, not substring", () => {
Expand Down
46 changes: 46 additions & 0 deletions tests/page-secondary-navigation.dom.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,52 @@ describe("PageSecondaryNavigation", () => {
]);
});

it("gives factsheets both destinations and keeps the current tab's filter state", () => {
render(
<PageSecondaryNavigation
modeId="factsheets"
pathname="/factsheets/search"
hasSubmittedSearch={false}
searchParamString="q=sertraline&category=Medicines&run=1"
onSearch={vi.fn()}
/>,
);
const bar = screen.getByTestId("mode-nav");
expect(bar).toHaveAttribute("aria-label", "Factsheets pages");
expect([...bar.querySelectorAll("li a")].map((link) => link.textContent)).toEqual(["Topics", "Search"]);
expect(screen.getByRole("link", { name: "Search" })).toHaveAttribute("aria-current", "page");
// Search is the tab you are already on. Its own link must not reset the
// category filter you are reading, nor drop `run` — that flips
// hasSubmittedModeSearch and re-places the composer for a no-op click.
expect(screen.getByRole("link", { name: "Search" })).toHaveAttribute(
"href",
"/factsheets/search?q=sertraline&category=Medicines&run=1",
);
// Topics is the browse home: it reads neither param, and carries no
// focus=1 either — autofocusing the composer there would open the phone
// keyboard over the topics the user just asked to browse.
expect(screen.getByRole("link", { name: "Topics" })).toHaveAttribute("href", "/factsheets");
});

it("keeps the newly adopted factsheets bar off its record routes", () => {
// Until adoption, `/factsheets/<slug>` was bar-free incidentally: the mode
// had one destination and ModeNav renders nothing below two. That
// protection expired the moment factsheets got a second one. What keeps the
// record route clear now is only the hasLocalInformationPageNavigation
// early return, so pin it at render rather than trusting the count.
expect(hasLocalInformationPageNavigation("/factsheets/sertraline")).toBe(true);
render(
<PageSecondaryNavigation
modeId="factsheets"
pathname="/factsheets/sertraline"
hasSubmittedSearch
onSearch={vi.fn()}
/>,
);
expect(screen.queryByTestId("mode-nav")).toBeNull();
expect(screen.queryByTestId("secondary-navigation")).toBeNull();
});

it("replaces mode navigation with only the information sections present in the record", async () => {
render(
<div>
Expand Down
1 change: 1 addition & 0 deletions tests/ui-mode-nav-density.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ const MODES = [
{ modeId: "specifiers", route: "/specifiers/compare", items: 4 },
{ modeId: "formulation", route: "/formulation/compare", items: 4 },
{ modeId: "differentials", route: "/differentials/diagnoses", items: 3 },
{ modeId: "factsheets", route: "/factsheets/search", items: 2 },
];

/**
Expand Down
Loading