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/branch-review-ledger.md b/docs/branch-review-ledger.md index 259bdd75f..dcfabb87f 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -902,6 +902,7 @@ Records before 2026-07-28 were written by hand and had drifted: 146 lines carrie | 2026-08-11 | claude/filter-popup-design-mockups-x6sbjv | 7b64f2559741a9f353adcf939745831e0daff7db | services filter sheet redesign mockups (3 directions, desktop+phone) | PR #1828 opened; design-scratch route only, no production behaviour change | verify:pr-local (1 pre-existing root-uid test failure, reproduced on origin/main 046feb3), build, check:rag:fixtures, check:bundle-budget both baselines within tolerance, 320px 0px overflow | | 2026-08-12 | claude/filter-popup-design-mockups-x6sbjv | 5e41d164e30e8f5a74b255fbeafd34123385dbb9 | services filter: round-two options study (stop-the-bleed / recommended / presets-evicted) | Pushed to PR #1828; merged babysit fixes to round-one facet semantics; design-scratch only | verify:pr-local (1 pre-existing root-uid failure only), build, check:rag:fixtures, bundle-budget mockups 286.8 KiB within 25% tolerance, counts re-verified vs snapshot, 320px 0px overflow | | 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 | | 2026-08-12 | claude/design-issues-triage-wnr7k9 | 2553b64b2342b0ef2ebef0afde152e76c1252496 | Ledger sweep round 2: open-PR cross-check plus clinical/answer-surface verification | 24 rows flagged IN FLIGHT against 7 open PRs (none had said so); 3 answer-surface rows archived (#166 #208 #216); #250 wave plan re-scoped; main-merge conflict resolved preserving both sides, 23 branch changes re-applied via the writer | verify:pr-local 10/10 green; check:outstanding-issues 121 open/180 archived, no ids deleted from base | | 2026-08-12 | claude/rag-canary-test-review-seprbt | bcf357a96fde74d39fc4726ffabb5079a744ef28 | eval-canary review: workflow, compare tooling, snapshot builder, alias tiering, rag-behaviour docs | PR #1843 opened; no retrieval behaviour change; snapshot refresh handed off as /issues #304 | verify:pr-local (green except env-only #296), eval:rag:offline 574/574, focused suites 40/40 | | 2026-08-12 | claude/design-issues-triage-wnr7k9 | 586012639565e4d3306b44361ebc5a3bdb3024ad | Land PR #1838 ledger sweep; close #147 mobile CLS by measurement | Merge resolved as union (main renumbered #302/#303 to #306/#307 — not lost, correcting an earlier claim); #306/#307 archived as already-delivered. #147 archived on two identical offline Lighthouse runs: mobile CLS 0.035/0.000/0.013/0.081/0.000, all under 0.1, cause fixed by PR #1616 not this session. #118 updated (browser drift 141-vs-151, wider than recorded); new #308 for desktop /documents/search CLS 0.119 | verify:pr-local 10/10 green; check:outstanding-issues 121 open/185 archived; verify:lighthouse x2 (gate ungraded on browser drift, measurements valid) | 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..bd47b194f 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();