From a0add717c2521c7fdeba4da5b094377014383c3e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 11:01:11 +0000 Subject: [PATCH 1/3] feat(filters): add the facet group kind and the global filter contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step one of the cross-mode filter rollout: the contract, with no rendered output changed. `kind` defaults to "lens", which is what all seven existing call sites are, so none of them are touched by this commit and every mode renders exactly as before. An audit of all ten filter surfaces found the same dimension rendered three different ways, two modes whose "filter" discarded the search instead of narrowing it, and a mode whose footer counts items its filters do not govern. The fix is agreeing what a group MEANS before deciding how it looks, so ResultFilterGroup is now discriminated: - lens (default) — the options partition the result set, one-of-N, role="radiogroup" with the existing roving tabindex. - facet — independent constraints that accumulate, many-of-N, OR within and AND across, role="group" with aria-pressed and one tab stop per option. Arrow keys are deliberately not bound: arrowing replaces for a lens, but would silently accumulate constraints for a facet. There is no navigate kind, deliberately. Options that replace the query rather than narrowing the result set do not belong in a control called Filter; those move to the composer as suggested searches. Counts stay on option.hint and keep the documents predicate ("how many would I have if I ticked this as well"), which is safe only alongside the companion rule that option lists are derived from the data rather than declared — that is what stops a permanently empty option, such as formulation's Biological/Social/Cultural, ever reaching the component. docs/filter-contract.md carries the rules, the scope-segment condition, the density thresholds and the rollout order. The DOM tests caught a real defect while being written: the selected-count badge sat inside the labelling heading, so a facet group's accessible name became "Domain 1" and changed on every toggle. The id is now on the label text alone. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J --- AGENTS.md | 4 +- docs/filter-contract.md | 146 +++++++++++++++ .../result-filter-control.tsx | 177 +++++++++++++++++- tests/search-results-header-band.dom.test.tsx | 150 +++++++++++++++ 4 files changed, 470 insertions(+), 7 deletions(-) create mode 100644 docs/filter-contract.md diff --git a/AGENTS.md b/AGENTS.md index 88ec727f8..483df4b4c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,7 +2,9 @@ # This is NOT the Next.js you know -This version has breaking changes — APIs, conventions, and file structure may all differ from your training data. Read the relevant guide in `node_modules/next/dist/docs/` before writing any code. Heed deprecation notices. +This version has breaking changes — APIs, conventions, and file structure may all differ from your training data. Read the relevant guide in `node_modules/next/dist/docs/` (resolved from this file's directory; in monorepos the `next` package may not be visible from the repo root) before writing any code. Heed deprecation notices. + +This block is written and re-added by `next dev` — verify at `node_modules/next/dist/server/lib/generate-agent-files.js`. Removing it from a diff only re-creates the uncommitted change; committing it with your work keeps the tree clean. diff --git a/docs/filter-contract.md b/docs/filter-contract.md new file mode 100644 index 000000000..8bdbd51db --- /dev/null +++ b/docs/filter-contract.md @@ -0,0 +1,146 @@ +# The filter contract + +One filter surface, shared by every mode. This is the rule set; the component is +`src/components/clinical-dashboard/result-filter-control.tsx`. + +It exists because an audit of all ten filter surfaces in 2026-08 found the same dimension +rendered three different ways, two modes whose "filter" discarded the search instead of +narrowing it, and a mode whose footer counted items its filters did not govern. The fix is not a +restyle — it is agreeing on what a filter group _means_ before deciding how it looks. + +## 1. A mode declares semantics, never a layout + +`ResultFilterGroup` is discriminated by `kind`. The mode says what the dimension is; the +component decides the renderer. No call site picks chips, rows or a segmented control. + +| Kind | Meaning | Selection | Built with | +| ---------------- | --------------------------------------------------------------- | --------------------------------- | -------------------------- | +| `lens` (default) | The options **partition** the result set; exactly one is active | one-of-N | `resultFilterGroup()` | +| `facet` | Independent constraints that accumulate | many-of-N, OR within / AND across | `resultFilterFacetGroup()` | + +`kind` is optional and defaults to `lens`, because that is what all seven existing call sites +are. Adding the facet kind changed no rendered output. + +**Which is which is a question about the data, not the UI.** Differentials' +All / Presentations / Diagnoses is a lens: a result cannot be both. Formulation's twelve domains +are facets: a mechanism routinely carries four. Rendering facets as radios — which formulation +does today — tells the reader they cannot hold two domains at once, which is false. + +### There is no `navigate` kind, and that is the point + +Services' quick filters and factsheets' categories do not filter. They call `router.push` and +**replace the query**, so choosing one discards the search and its results with no warning and no +undo. A control labelled "Filter" must not do that. Query-replacing presets belong beside the +composer as suggested searches (`AnswerSuggestionChips`), not inside the filter sheet. + +Until a mode has real facets, it is better for its filter trigger to be absent than to open a +sheet that throws the query away. + +## 2. Accessibility follows from the kind + +| | `lens` | `facet` | +| ---------- | ------------------------------- | -------------- | +| Container | `role="radiogroup"` | `role="group"` | +| Option | `role="radio"` + `aria-checked` | `aria-pressed` | +| Tab stops | one per group, roving tabindex | one per option | +| Arrow keys | move **and select** | not bound | + +The single tab stop is correct for a lens precisely because arrowing _replaces_ the selection. +It would be wrong for a facet, where arrowing would silently accumulate constraints the reader +never asked for, and where every toggle must be individually reachable. + +Both breakpoints use the same contract. The per-mode desktop chip rails that use `aria-pressed` +for one-of-N dimensions are wrong and are replaced as each mode adopts. + +## 3. Counts, and the rule that makes them safe + +A count goes in `option.hint` and answers **"how many would I have if I ticked this as well?"** — +the same predicate as the filter. Under OR-within-group, adding an option to an already-selected +group _widens_, so a count derived by narrowing the current subset would disagree with what the +click actually does. `projectSmartTagFacetGroups` in `src/lib/document-tags.ts` is the reference +implementation. + +That contract has one failure mode: an option that matches nothing reports the unchanged total +rather than zero, so an empty option looks identical to a full one. The companion rule removes +the failure rather than patching it: + +> **Derive the option list from the data. Never declare it.** + +Formulation declares twelve domains; three of them — Biological, Social and Cultural — match +none of the twelve mechanisms, so the sheet offers three controls that can never return anything. +Documents derives its facets from the current match set, so a zero-member facet cannot exist. +Derive, and the union count is always safe. + +A zero **as a consequence of the current selection** is legitimate and must stay visible: dashed +border, muted pair, `aria-disabled`, click guarded, still focusable, with an `sr-only` reason. +Never `opacity` — it multiplies against an already-muted foreground and does not survive +forced-colors, where border-style is preserved. A reader who has narrowed to nothing needs to see +which choice did it. + +## 4. Scope is conditional, and most modes do not get it + +Where a mode has a catalogue meaningfully larger than the current result set, the sheet offers a +scope segment — `These results N | All items N`, counts on both — built from the shared +`SegmentedControl` (`src/components/ui/segmented-control.tsx`), which already has the roving +tabindex and radio semantics. + +It earns its place because it is the only escape from a filtered-to-zero state that does not +discard the query: the commit becomes "Show N in all items" instead of a dead end. + +**Render it only when the catalogue is meaningfully larger than the result set.** Otherwise the +two segments show the same number and the row is noise. + +| Gets scope | Does not | +| ----------------------------------------------------------------------- | --------------------------------------------------- | +| services (219), medication (328), differentials (232), specifiers (585) | factsheets (8), applications (13), formulation (12) | + +Documents is deliberately excluded: it already answers this with a `N of M documents shown` meter +and a "Browse all sources" action framed as _reach, not refinement_. That is a better fit for a +corpus of that size, and it stays. + +## 5. Density is a function of option count + +Facet groups only. Thresholds match the rule documents already uses (`dense = groups.length > 3`). + +| Options | Renderer | +| ------------------- | ----------------------------------------------------------------------- | +| ≤ 5 | chips, single row where they fit | +| 6 – 20 | dense list: full-width rows, right-aligned count column, group headings | +| > 20, or > 3 groups | dense list plus find-a-filter and collapse-by-default | + +Collapse rules, when they apply: groups start collapsed; a group holding a selection opens +itself; an explicit user collapse beats that; an active needle forces every matched group open +and owns openness. A selected option always survives the needle, so an active constraint can +never become unreachable. + +## 6. Invariants + +- **`footerNote` counts what the filters actually govern.** Specifiers currently reports + `results.length + catalogueMatches.length` while the groups narrow only `results` — the sheet + claims to scope a list it half controls. A mode must not report a total its filters cannot move. +- **`onClearAll` never touches the query.** Therapy-compass's clear wipes the search box; the + shared sheet's does not. Clearing filters and clearing a search are different intentions. +- **One trigger component.** `ResultFilterTrigger`. Therapy-compass re-implements it with a + different icon, a hardcoded test id and a different label-hiding breakpoint. +- **Tap targets are `min-h-tap` (48px) on phone.** Do not relax to 44px for generic WCAG + guidance; it reintroduces a known `ui-smoke` flake. + +## 7. Not yet reconciled + +`SearchScopeFilters` (`src/lib/search-scope.ts`) is a second filter surface: 20 keys applied +server-side at retrieval, not editable from any panel, visible only as removable chips in the +documents zero-results state. Anything here that claims to be "the" filter contract is currently +telling half the story. Reconciling the two is tracked separately and is not a prerequisite for +adoption. + +## Rollout + +Contract first, then one PR per mode: + +1. **Contract** — add the kinds, the facet renderer and the builders, changing no rendered output. +2. **Per mode** — adopt the right kind, derive the option list, add counts, and retire that mode's + desktop rail so the breakpoints stop disagreeing. +3. **Services and factsheets** — evict the query-replacing presets to the composer. +4. **Documents last** — port its needle and collapse up into the shared component as the + `> 20` tier, then converge. It is the largest surface and should move once the contract is + proven elsewhere. diff --git a/src/components/clinical-dashboard/result-filter-control.tsx b/src/components/clinical-dashboard/result-filter-control.tsx index 87ea22f78..c14423119 100644 --- a/src/components/clinical-dashboard/result-filter-control.tsx +++ b/src/components/clinical-dashboard/result-filter-control.tsx @@ -41,15 +41,52 @@ export type ResultFilterOption = { disabled?: boolean; }; -export type ResultFilterGroup = { +/** + * A dimension the sheet can render, discriminated by what it MEANS rather than + * by how it should look. + * + * `lens` — one-of-N. The options partition the result set and exactly one is + * active: differentials' All/Presentations/Diagnoses, medication's + * Best/Indication/Safety/Monitor. This is the shape every call site uses today, + * so it is the default and `kind` may be omitted. + * + * `facet` — many-of-N, OR within the group and AND across groups. Formulation's + * domains and the documents tag groups are facets; rendering them as radios (as + * formulation does today) claims a reader cannot hold two domains at once, + * which is false. + * + * There is deliberately no `navigate` kind. Options that replace the query + * rather than narrowing the result set do not belong in a control called + * "Filter" — see `docs/filter-contract.md`. + */ +export type ResultFilterGroupKind = "lens" | "facet"; + +type ResultFilterGroupBase = { /** Stable within one sheet; used for the group's own labelling ids. */ id: string; label: string; - value: string; options: ReadonlyArray>; +}; + +export type ResultFilterLensGroup = ResultFilterGroupBase & { + kind?: "lens"; + value: string; onChange: (value: string) => void; }; +export type ResultFilterFacetGroup = ResultFilterGroupBase & { + kind: "facet"; + /** Selected option values. Empty means the group imposes no constraint. */ + selected: ReadonlySet; + onToggle: (value: string) => void; +}; + +export type ResultFilterGroup = ResultFilterLensGroup | ResultFilterFacetGroup; + +export function isFacetGroup(group: ResultFilterGroup): group is ResultFilterFacetGroup { + return group.kind === "facet"; +} + /** * Builds a type-checked group for `ResultFilterSheet`. * @@ -68,6 +105,7 @@ export function resultFilterGroup(group: { onChange: (value: Value) => void; }): ResultFilterGroup { return { + kind: "lens", id: group.id, label: group.label, value: group.value, @@ -77,6 +115,38 @@ export function resultFilterGroup(group: { }; } +/** + * Builds a type-checked multi-select facet group. + * + * Same erasure argument as `resultFilterGroup`: `selected`, `options[].value` + * and `onToggle` are checked against one `Value` at the call site and widen to + * `string` once, and the sheet only ever passes back a value taken from this + * group's own options. + * + * Counts belong in `option.hint` and must be produced by the same predicate as + * the filter — "how many would I have if I ticked this as well" — because under + * OR-within-group, adding to an already-selected group *widens*. The companion + * rule is that the option list is derived from the data rather than declared, + * which is what stops a permanently empty option ever reaching this function. + * Both rules and the reasoning are in `docs/filter-contract.md`. + */ +export function resultFilterFacetGroup(group: { + id: string; + label: string; + selected: ReadonlySet; + options: ReadonlyArray>; + onToggle: (value: Value) => void; +}): ResultFilterGroup { + return { + kind: "facet", + id: group.id, + label: group.label, + selected: group.selected as ReadonlySet, + options: group.options, + onToggle: (value) => group.onToggle(value as Value), + }; +} + /** * Opens a filter panel and reports how many filters are active. * @@ -186,7 +256,7 @@ export function ResultFilterTrigger({ * the *selected* option. This path is therefore defensive, and is asserted in the * DOM tests so it cannot rot before the first mode needs it. */ -function FilterRadioGroup({ group, panelId }: { group: ResultFilterGroup; panelId: string }) { +function FilterRadioGroup({ group, panelId }: { group: ResultFilterLensGroup; panelId: string }) { const refs = useRef(new Map()); const groupLabelId = `${panelId}-${group.id}-label`; @@ -318,6 +388,97 @@ function FilterRadioGroup({ group, panelId }: { group: ResultFilterGroup; panelI ); } +/** + * A multi-select facet group. + * + * Deliberately NOT a radio group. `aria-pressed` toggles inside a + * `role="group"` are the honest reading of many-of-N: each control is + * independently on or off, and nothing claims the options are mutually + * exclusive. That also means no roving tabindex — a reader must be able to + * reach every toggle, and arrow-to-select would commit constraints they did not + * ask for. The single tab stop the lens groups use is correct there precisely + * because arrowing *replaces* rather than accumulates. + * + * A zero-count option is a dead end: still focusable and explained, never + * silently dropped, because a reader who has just narrowed to nothing needs to + * see which of their choices did it. Under the derived-option-list rule such an + * option can only appear as a consequence of the current selection, never as a + * permanent fixture of the catalogue. + */ +function FilterFacetGroup({ group, panelId }: { group: ResultFilterFacetGroup; panelId: string }) { + const groupLabelId = `${panelId}-${group.id}-label`; + + return ( +
+

+ {/* The id is on the label text alone, not the heading. With the badge + inside the labelled element the group's accessible name became + "Domain 1" — the selection count leaking into the dimension's name, + and changing it on every toggle. Caught by the DOM test. */} + {group.label} + {group.selected.size > 0 ? ( + + {group.selected.size} selected + {group.selected.size} + + ) : null} +

+
+ {group.options.map((option) => { + const selected = group.selected.has(option.value); + const deadEnd = Boolean(option.disabled) && !selected; + const deadEndDescId = `${panelId}-${group.id}-${option.value.replace(/[^A-Za-z0-9_-]/g, "-")}-note`; + return ( + + ); + })} +
+
+ ); +} + /** * A single-choice filter sheet: one radio group per dimension. * @@ -399,9 +560,13 @@ export function ResultFilterSheet({ } >
- {groups.map((group) => ( - - ))} + {groups.map((group) => + isFacetGroup(group) ? ( + + ) : ( + + ), + )}
); diff --git a/tests/search-results-header-band.dom.test.tsx b/tests/search-results-header-band.dom.test.tsx index c4663c2f0..58109eb92 100644 --- a/tests/search-results-header-band.dom.test.tsx +++ b/tests/search-results-header-band.dom.test.tsx @@ -12,6 +12,7 @@ import { import { ResultFilterSheet, ResultFilterTrigger, + resultFilterFacetGroup, resultFilterGroup, } from "@/components/clinical-dashboard/result-filter-control"; @@ -547,6 +548,155 @@ describe("SearchResultsHeaderBand", () => { }); }); +describe("ResultFilterSheet facet groups", () => { + // The whole point of the kind split: a facet must NOT claim mutual exclusion. + // A radio bank here would tell a reader they cannot hold two domains at once, + // which is false for every mechanism in the formulation catalogue. + it("renders many-of-N as a pressed-toggle group and accumulates selections", async () => { + const user = userEvent.setup(); + const onToggle = vi.fn(); + + render( + (["Cognition"]), + options: [ + { value: "Cognition", label: "Cognition", hint: "7" }, + { value: "Affect", label: "Affect", hint: "9" }, + ], + onToggle, + }), + ]} + />, + ); + + expect(screen.queryByRole("radiogroup")).not.toBeInTheDocument(); + const group = screen.getByRole("group", { name: "Domain" }); + expect(within(group).getByRole("button", { name: /Cognition/ })).toHaveAttribute("aria-pressed", "true"); + expect(within(group).getByRole("button", { name: /Affect/ })).toHaveAttribute("aria-pressed", "false"); + + await user.click(within(group).getByRole("button", { name: /Affect/ })); + expect(onToggle).toHaveBeenCalledWith("Affect"); + }); + + // Every toggle must be individually reachable. The lens groups' single tab + // stop is correct there because arrowing replaces; here it would strand + // options behind a key binding that must not commit anything. + it("gives every facet its own tab stop", async () => { + const user = userEvent.setup(); + + render( + (), + options: [ + { value: "Cognition", label: "Cognition" }, + { value: "Affect", label: "Affect" }, + ], + onToggle: vi.fn(), + }), + ]} + />, + ); + + const group = screen.getByRole("group", { name: "Domain" }); + const first = within(group).getByRole("button", { name: /Cognition/ }); + const second = within(group).getByRole("button", { name: /Affect/ }); + first.focus(); + await user.tab(); + expect(second).toHaveFocus(); + }); + + // A zero reached through the current selection stays reachable and explained. + // Dropping it would hide the choice that emptied the results. + it("keeps a zero-yield facet focusable, explained and unselectable", async () => { + const user = userEvent.setup(); + const onToggle = vi.fn(); + + render( + (), + options: [ + { value: "Cognition", label: "Cognition", hint: "7" }, + { value: "Cultural", label: "Cultural", hint: "0", disabled: true }, + ], + onToggle, + }), + ]} + />, + ); + + const group = screen.getByRole("group", { name: "Domain" }); + const dead = within(group).getByRole("button", { name: /Cultural/ }); + expect(dead).toHaveAttribute("aria-disabled", "true"); + expect(dead).not.toHaveAttribute("disabled"); + expect(dead).toHaveAccessibleDescription("No matches with your current filters."); + + await user.click(dead); + expect(onToggle).not.toHaveBeenCalled(); + }); + + // Mixed sheets are the real shape for formulation: one lens, one facet. + it("renders lens and facet dimensions side by side with their own semantics", () => { + render( + (["Cognition"]), + options: [{ value: "Cognition", label: "Cognition" }], + onToggle: vi.fn(), + }), + ]} + />, + ); + + expect(screen.getByRole("radiogroup", { name: "Show" })).toBeInTheDocument(); + expect(screen.getByRole("group", { name: "Domain" })).toBeInTheDocument(); + }); +}); + describe("ResultFilterSheet", () => { it("exposes each dimension as a radio group and reports the selection back typed", async () => { const user = userEvent.setup(); From f625df78eeaaf48d6bb0b1e6cd33ecd431728195 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 11:04:03 +0000 Subject: [PATCH 2/3] docs(review): record global filter contract handoff Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J --- docs/branch-review-ledger.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index 7fd691776..2ec1259d0 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -900,3 +900,4 @@ Records before 2026-07-28 were written by hand and had drifted: 146 lines carrie | 2026-08-12 | PR #1815 / claude/spacing-icon-design-review-rxwh28 | 9f266210f02081be54d407c70a85f52fed436128 | babysit | no remaining actionable findings; one pre-existing thread resolved as no-change (Dockerfile.worker follow-up needed) | required checks: Gitleaks PR policy PR required (all pass); targeted vitest passed: tests/document-frame-contract.test.ts + tests/in-page-nav-header.dom.test.tsx | | 2026-08-12 | 1815 | 27ce96e1755055ceee2eeae02d6efdf11259fcde | babysit | fixed | Unit coverage: targeted vitest passed: tests/shared-home-empty-state.dom.test.tsx (17 passed). PR required still blocked on pre-existing check failure at old remote head before sync. | | 2026-08-12 | codex/pr-workflow-safety-230-296 | bc0a491fdf4146775629f9b2b03e2a2cc61bd7cb | pr-1830 unblock | unblocked: merged origin/main (outstanding-issues conflict), PR body RAG impact + governance, resolved Copilot thread; merge-tree clean; required CI in progress | check:outstanding-issues pass; evaluatePullRequestPolicy ok; merge-tree clean; PR policy/mergeability/Change scope in progress | +| 2026-08-12 | claude/filter-contract-global | a0add717c2521c7fdeba4da5b094377014383c3e | global filter contract: lens/facet kinds + docs/filter-contract.md (no rendered change) | PR #1847 opened; additive only, zero call sites touched; fixed an accessible-name leak caught by the new DOM tests | verify:pr-local fully green (no failures), 4 new DOM tests, git diff over all 7 mode files empty | From 561701371c7242ba0015e8a9b5939b28ebb433d9 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 12:28:32 +0000 Subject: [PATCH 3/3] fix(filters): use the radius-xs token instead of an arbitrary radius MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit review, and it is right: --radius-xs is exactly 0.25rem, so rounded-xs is the token equivalent of the arbitrary value. rounded-xs is already used in production (ui/choice.tsx, therapy-compass/ui.tsx), and this was the only non-mockup file carrying the arbitrary form — the other hits are mockups, which are exempt from the token gate. Copied in from the mockup study, where it was legal; it is not here. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J --- src/components/clinical-dashboard/result-filter-control.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/clinical-dashboard/result-filter-control.tsx b/src/components/clinical-dashboard/result-filter-control.tsx index c14423119..bd47b194f 100644 --- a/src/components/clinical-dashboard/result-filter-control.tsx +++ b/src/components/clinical-dashboard/result-filter-control.tsx @@ -456,7 +456,7 @@ function FilterFacetGroup({ group, panelId }: { group: ResultFilterFacetGroup; p