diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index f67efb8a1e..7f8733fac8 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -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) | diff --git a/src/components/mode-nav/registry-mode-nav.tsx b/src/components/mode-nav/registry-mode-nav.tsx index bd8f8f9978..f5f3bb9544 100644 --- a/src/components/mode-nav/registry-mode-nav.tsx +++ b/src/components/mode-nav/registry-mode-nav.tsx @@ -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"; @@ -23,6 +31,11 @@ const iconByItemId: Record = { 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, }; /** diff --git a/src/lib/mode-secondary-navigation.ts b/src/lib/mode-secondary-navigation.ts index f57391e7fc..f71ee39945 100644 --- a/src/lib/mode-secondary-navigation.ts +++ b/src/lib/mode-secondary-navigation.ts @@ -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; type RegistryEntry = (typeof modeSecondaryNavigationRegistry)[AppModeId][number]; @@ -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 { @@ -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/` 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; } @@ -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; @@ -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; } diff --git a/tests/mode-nav-addon-slot.dom.test.tsx b/tests/mode-nav-addon-slot.dom.test.tsx index 646b8600da..02bebe93d2 100644 --- a/tests/mode-nav-addon-slot.dom.test.tsx +++ b/tests/mode-nav-addon-slot.dom.test.tsx @@ -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); } diff --git a/tests/mode-secondary-navigation.test.ts b/tests/mode-secondary-navigation.test.ts index bca5432276..0407478d77 100644 --- a/tests/mode-secondary-navigation.test.ts +++ b/tests/mode-secondary-navigation.test.ts @@ -24,7 +24,7 @@ const expectedLabels: Record = { prescribing: ["Search"], tools: ["Search"], "therapy-compass": ["Search", "Recommend", "Compare", "Pathways", "Brief Intervention", "Patient Sheets"], - factsheets: ["Search"], + factsheets: ["Topics", "Search"], }; const cleanLandingPath: Record = { @@ -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)", () => { @@ -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( @@ -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", () => { diff --git a/tests/page-secondary-navigation.dom.test.tsx b/tests/page-secondary-navigation.dom.test.tsx index 8262f8a37d..0c17b75c2f 100644 --- a/tests/page-secondary-navigation.dom.test.tsx +++ b/tests/page-secondary-navigation.dom.test.tsx @@ -177,6 +177,52 @@ describe("PageSecondaryNavigation", () => { ]); }); + it("gives factsheets both destinations and keeps the current tab's filter state", () => { + render( + , + ); + 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/` 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( + , + ); + 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(
diff --git a/tests/ui-mode-nav-density.spec.ts b/tests/ui-mode-nav-density.spec.ts index a56c9db889..75331ab187 100644 --- a/tests/ui-mode-nav-density.spec.ts +++ b/tests/ui-mode-nav-density.spec.ts @@ -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 }, ]; /**