diff --git a/docs/filter-contract.md b/docs/filter-contract.md index a95c40668..427c67fcf 100644 --- a/docs/filter-contract.md +++ b/docs/filter-contract.md @@ -21,15 +21,6 @@ component decides the renderer. No call site picks chips, rows or a segmented co `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. -**`renderAs` is the one exception, and it is a migration seam rather than a layout choice.** A -counted lens should be a segmented bar (section 5), and the component could derive that on its own -from `kind` plus "every option carries a count" — but doing so would restyle all thirteen lens call -sites in a single change, each needing its own browser proof. So `renderAs: "segmented"` is opt-in -per group while modes move over one at a time. It is deliberately not a free choice: it names the -same renderer the rule would derive, the component still refuses it for a group carrying a dead end, -and the end state is that the flag disappears and derivation takes over. Do not add a second value -to it, and do not read it as licence for a call site to pick its own layout. - **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 @@ -125,8 +116,15 @@ cannot state either segment honestly. `data/differentials-snapshot.json` is 1.2 component deliberately never imports it — doing so to get an "all" count would put the whole snapshot in the bundle — while `useDifferentialSearch` only ever receives query-matched results. So the page can produce a constant `232`, but not "how many of the 232 survive the current urgency selection", -and section 3 requires both counts to come from the same predicate as the filter. `/api/differentials` -is no help either: its `total` is the _match_ count once `q` is present, not the catalogue's. +and section 3 requires both counts to come from the same predicate as the filter. + +`/api/differentials` used to compound this: its `total` measured the records it was returning, which +under a query are the ranked matches, so it reported the caller's own result count rather than the +catalogue. That is fixed — all four branches of the route report the catalogue size, pinned by +`tests/differentials-route.test.ts` — so the honest figure is now available. What is still missing is +the _scoped_ count: "how many of the 232 survive the current urgency selection" needs the catalogue +in memory, which is the megabyte this client must not import. The total alone cannot satisfy +section 3. Differentials **browse** (`differential-stream-workspace.tsx`) does get scope, because its server component hands it a model carrying matched and unmatched entries together, distinguished by @@ -167,20 +165,31 @@ still the right renderer for short bare labels. **The same argument applies to a counted `lens`, and the answer there is the segmented bar.** A lens is an exact partition, so it takes `SegmentedControl` rather than the two-column grid — which is what `ChoiceChip`'s own contract already says: _"Compact many-of-many selection. Use SegmentedControl for -one-of-many choices."_ Opt in per group with `renderAs: "segmented"` on `resultFilterGroup()`; the -default stays `"chips"` so the twelve lens call sites that predate this render unchanged, and each -can move over with its own browser proof. Differentials is the first adopter — its Show (3) and -Clinical urgency (4) groups were the ragged wrapping row this rule exists to stop. - -Two constraints on that renderer, both load-bearing: - -- **A group carrying a dead-end option stays on chips.** `SegmentedControl` marks a disabled option - with the native `disabled` attribute, which takes it out of the tab order. A dead end has to stay - focusable and explained — a reader who has just narrowed to nothing needs to reach the option that - did it — so `renderAs` is ignored for such a group rather than silently degrading it. -- **Counts must be unit-free.** `SegmentedControl` uses one field for both the visible count and the - accessible name, so it has no `hintLabel` equivalent (see the rule below). A lens whose counts - carry a unit keeps the chip renderer until that second field exists. +one-of-many choices."_ + +**It is derived, never declared.** A lens whose options all carry a count renders as a segmented bar +because of what it is, not because a call site asked. There is no renderer flag, and adding one would +break section 1 — a mode declares semantics, and picking a layout is the thing that rule exists to +stop. An earlier revision shipped `renderAs: "segmented"` as a migration seam so modes could move one +at a time; it is gone, and the option list is what decides. + +Two conditions bound it, both load-bearing: + +- **At most five options.** That is where the chip tier above ends. A segmented bar is one control + read left to right; past five it wraps into rows and stops reading as one, which is the ragged + shape this rule exists to remove. A longer lens keeps the chip row. + A dead end does **not** send the group back to chips, and an earlier revision that made it do so was + wrong: documents' Source locality marks an option dead the moment its count reaches zero, so a + state-dependent renderer made the control morph from a segmented bar into a chip row while the reader + was using it. The shape of the option list decides the renderer; nothing about the current selection + can change it. `SegmentedControl` carries the dead end itself, on a `deadEnd` field kept deliberately + separate from `disabled` — `disabled` means "not on offer" and leaves the arrow path, `deadEnd` means + "your own narrowing emptied this" and stays on it with `aria-disabled` and a stated reason, exactly as + section 3 requires. + +Counts may carry units. `SegmentedControl` takes the same `hint`/`hintLabel` split as an option (see +the rule below), so `"1 loaded source"` is announced while `1` is displayed. Before that split a +counted lens with a unit had to stay on chips — which is what kept documents' Source locality there. **`hint` is announced, `hintLabel` is displayed.** `hint` carries the unit (`"1 loaded source"`) and is what the option's accessible name is built from; `hintLabel` is the short visible form (`"1"`). diff --git a/src/app/api/differentials/route.ts b/src/app/api/differentials/route.ts index a1a7209af..a8b942ad4 100644 --- a/src/app/api/differentials/route.ts +++ b/src/app/api/differentials/route.ts @@ -76,7 +76,14 @@ function publicDifferentialPayload(kind: DifferentialRecordKind, q: string | und return { records, matches: ranked ? recordMatchesPayload(ranked) : undefined, - total: records.length, + // The catalogue size, not `records.length`. With `q` present `records` holds + // the ranked matches, so measuring it made `total` the match count — which is + // already in `records`/`matches` — while the other three branches here report + // the catalogue (`snapshot.presentations.length` above, `rows.length` on both + // owner paths). A caller asking "how many differentials are there" got the + // size of its own result set back, so the differentials filter could not + // state the catalogue figure and had to omit it. + total: differentialRecords.length, governance: { sourceStatus: governance.source_status, validationStatus: governance.validation_status }, }; } diff --git a/src/components/clinical-dashboard/differentials-home.tsx b/src/components/clinical-dashboard/differentials-home.tsx index 2913f6aca..448c2896c 100644 --- a/src/components/clinical-dashboard/differentials-home.tsx +++ b/src/components/clinical-dashboard/differentials-home.tsx @@ -1200,19 +1200,17 @@ function SearchResultsView({ panelId={filterPanelId} testId="differential-filter-panel" title="Filter differentials" - description="Narrow by result type, then by clinical urgency. Both narrow the same list together." + description="Narrow by result type, then by clinical urgency. Both narrow the same list." groups={[ - // Both dimensions are exact partitions carrying counts, which is the - // case `ChoiceChip` itself sends to `SegmentedControl`: a counted - // chip is wide enough that four of them wrap one per line and leave - // most of each row empty. + // Both dimensions are exact partitions carrying counts, so the sheet + // derives the segmented bar for them — see docs/filter-contract.md + // section 5. Nothing here selects a renderer. resultFilterGroup({ id: "result-type", label: "Show", value: kindFilter, options: kindFilterOptions, onChange: setKindFilter, - renderAs: "segmented", }), resultFilterGroup({ id: "urgency", @@ -1220,7 +1218,6 @@ function SearchResultsView({ value: urgencyFilter, options: urgencyFilterOptions, onChange: setUrgencyFilter, - renderAs: "segmented", }), ]} onClearAll={activeFilterCount > 0 ? clearAllFilters : undefined} diff --git a/src/components/clinical-dashboard/result-filter-control.tsx b/src/components/clinical-dashboard/result-filter-control.tsx index 4459053d0..da00faead 100644 --- a/src/components/clinical-dashboard/result-filter-control.tsx +++ b/src/components/clinical-dashboard/result-filter-control.tsx @@ -105,25 +105,6 @@ export type ResultFilterLensGroup = ResultFilterGroupBase & { than role alone. Omit for a sheet with no facet groups; the roving radiogroup already says "one active" on its own there. */ note?: string; - /** - * How the one-of-N options are drawn. Defaults to `"chips"`, so every lens - * call site that predates this renders unchanged. - * - * `"segmented"` is the shape `docs/filter-contract.md` section 5 argues for - * once options carry counts: a counted chip is wide enough that four of them - * wrap one per line and leave most of each row empty, which is exactly the - * ragged column the counted-row renderer was added to fix for facets. A lens - * is an exact partition, so it gets the segmented bar rather than that - * two-column grid — `ChoiceChip`'s own contract sends one-of-many choices to - * `SegmentedControl`. - * - * Ignored for a group carrying a dead-end option. `SegmentedControl` marks a - * disabled option with the native `disabled` attribute, which drops it out of - * the tab order; this group keeps dead ends focusable and explained on - * purpose (see `isDeadEnd` below), and losing that is a real regression, so - * such a group falls back to chips. - */ - renderAs?: "chips" | "segmented"; }; export type ResultFilterFacetGroup = ResultFilterGroupBase & { @@ -158,7 +139,6 @@ export function resultFilterGroup(group: { onChange: (value: Value) => void; note?: string; optionSections?: ReadonlyArray; - renderAs?: "chips" | "segmented"; }): ResultFilterGroup { return { kind: "lens", @@ -169,7 +149,6 @@ export function resultFilterGroup(group: { options: group.options, note: group.note, optionSections: group.optionSections, - renderAs: group.renderAs, // The one narrowing, isolated here rather than repeated at seven call sites. onChange: (value) => group.onChange(value as Value), }; @@ -347,11 +326,25 @@ function FilterRadioGroup({ group, panelId }: { group: ResultFilterLensGroup; pa // selectable one when the value matches no option (a stale URL param, or a // catalogue that dropped a category between renders). const tabStopValue = selectable.some((o) => o.value === group.value) ? group.value : selectable[0]?.value; - // See `renderAs` on ResultFilterLensGroup for why a dead end vetoes the - // segmented bar: SegmentedControl disables such an option natively, which - // takes it out of the tab order, and an unreachable dead end cannot explain - // itself to the reader whose selection created it. - const useSegmented = group.renderAs === "segmented" && selectable.length === group.options.length; + // Derived, never declared — see docs/filter-contract.md section 5. A lens + // whose options all carry a count is a segmented bar: a counted chip is wide + // enough that four of them wrap one per line and leave most of each row + // empty, and `ChoiceChip`'s own contract sends one-of-many choices to + // `SegmentedControl`. One condition bounds it. + // + // Five options, because that is where section 5's chip tier ends. A segmented + // bar is one control read left to right; past five it wraps into rows and + // stops reading as one, which is the ragged shape this rule exists to remove. + // + // Dead ends do NOT veto it. They used to, and that was wrong: documents' + // Source locality marks an option dead when its count reaches zero, so a + // state-dependent veto made the control morph from a segmented bar into a + // chip row mid-interaction. `SegmentedControl` now keeps a dead end focusable + // and explained itself, exactly as the chip renderer does, so the shape of the + // option list decides the renderer and nothing about the reader's current + // selection can change it. + const everyOptionCounted = group.options.length > 0 && group.options.every((option) => Boolean(option.hint)); + const useSegmented = everyOptionCounted && group.options.length <= 5; const moveTo = useCallback( (next: ResultFilterOption | undefined) => { @@ -412,14 +405,18 @@ function FilterRadioGroup({ group, panelId }: { group: ResultFilterLensGroup; pa options={group.options.map((option): SegmentedControlOption => ({ value: option.value, label: option.label, - // `hint` only. SegmentedControl uses one field for both the - // visible count and the accessible name, whereas an option - // splits them into `hint` (announced, carries the unit) and - // `hintLabel` (displayed). Passing `hintLabel` here would - // strip the unit from the announced name, so a lens whose - // counts carry one has to stay on chips until SegmentedControl - // grows the second field. + // Both halves of the count. `hint` carries the unit and builds + // the accessible name; `hintLabel` is the short visible form. + // Documents' Source locality needs exactly this — "1 loaded + // source" announced, "1" displayed — and before the control took + // the second field, a lens whose counts carry a unit had to stay + // on chips. hint: option.hint, + hintLabel: option.hintLabel, + // `deadEnd`, not `disabled`: an option the reader's own narrowing + // emptied stays on the arrow path and explains itself, where + // `disabled` would skip it entirely. + deadEnd: isDeadEnd(option), }))} // Not `equal`: that stretches every segment to the same width with // `whitespace-nowrap` and truncates. "All priorities" and diff --git a/src/components/ui/segmented-control.tsx b/src/components/ui/segmented-control.tsx index 381d0dcc7..0537208bf 100644 --- a/src/components/ui/segmented-control.tsx +++ b/src/components/ui/segmented-control.tsx @@ -1,7 +1,7 @@ "use client"; import type { LucideIcon } from "lucide-react"; -import { useCallback, useRef } from "react"; +import { useCallback, useId, useRef } from "react"; import { cn } from "@/components/ui-primitives"; @@ -9,7 +9,22 @@ export type SegmentedControlOption = { value: T; label: string; icon?: LucideIcon; + /** + * Genuinely unavailable: skipped by the arrow keys and natively `disabled`, + * the standard radiogroup treatment for an option that is not on offer. + */ disabled?: boolean; + /** + * Offered, but empty under the reader's current narrowing — a dead end rather + * than an unavailable option, and a different thing from `disabled`. + * + * It stays on the arrow path and takes `aria-disabled`, because + * `docs/filter-contract.md` section 3 requires the option a reader just + * emptied to remain focusable and to explain itself. Native `disabled` would + * drop it out of the tab order, hiding the explanation from the one person + * who needs it. Selection is withheld; focus is not. + */ + deadEnd?: boolean; /** * Trailing detail, almost always a count — "Presentations 41". * @@ -22,6 +37,15 @@ export type SegmentedControlOption = { * array and hand it to both the desktop rail and the phone sheet. */ hint?: string; + /** + * Short display form of `hint`, mirroring `ResultFilterOption.hintLabel`. + * + * The announced name keeps `hint`'s unit ("1 loaded source"); the visible + * column shows only this ("1"). Without the split, a lens whose counts carry + * a unit had to choose between an unreadable segment and a name that dropped + * the unit — so it stayed on chips instead. Omit to display `hint` verbatim. + */ + hintLabel?: string; }; type AccessibleName = { label: string; ariaLabelledBy?: never } | { label?: never; ariaLabelledBy: string }; @@ -55,43 +79,52 @@ export function SegmentedControl({ className, }: SegmentedControlProps) { const refs = useRef(new Map()); - const enabled = options.filter((option) => !option.disabled); - // Keep the controlled value honest: a disabled matching option stays the - // checked radio. Never silently remap to the first enabled option — that + // Scopes the dead-end explanation ids, so two rails on one page cannot collide. + const idPrefix = useId(); + // `disabled` leaves the arrow path entirely; `deadEnd` stays on it. See the + // two fields on SegmentedControlOption for why they are not the same thing. + const reachable = options.filter((option) => !option.disabled); + const selectable = options.filter((option) => !option.disabled && !option.deadEnd); + // Keep the controlled value honest: an unavailable matching option stays the + // checked radio. Never silently remap to the first selectable option — that // would show a selection the owner state does not hold. const valueMatchesOption = options.some((option) => option.value === value); - const valueIsEnabled = enabled.some((option) => option.value === value); + const valueIsSelectable = selectable.some((option) => option.value === value); const selectedValue = valueMatchesOption ? value : undefined; - const tabStopValue = valueIsEnabled ? value : enabled[0]?.value; + const tabStopValue = valueIsSelectable ? value : selectable[0]?.value; - const selectAndFocus = useCallback( + // Focus always moves; selection only follows for an option that can hold it. + // Arrowing onto a dead end is how its explanation gets announced, so it must + // not commit the option before it and must not leave focus behind either. + const moveTo = useCallback( (next: SegmentedControlOption | undefined) => { - if (!next || next.disabled) return; - onChange(next.value); + if (!next) return; refs.current.get(next.value)?.focus(); + if (next.deadEnd) return; + onChange(next.value); }, [onChange], ); const onKeyDown = useCallback( (event: React.KeyboardEvent) => { - if (!enabled.length) return; + if (!reachable.length) return; const currentValue = (event.target as HTMLElement).dataset.segmentValue as T | undefined; const current = Math.max( - enabled.findIndex((option) => option.value === currentValue), + reachable.findIndex((option) => option.value === currentValue), 0, ); let next: number | null = null; - if (event.key === "ArrowRight" || event.key === "ArrowDown") next = (current + 1) % enabled.length; + if (event.key === "ArrowRight" || event.key === "ArrowDown") next = (current + 1) % reachable.length; else if (event.key === "ArrowLeft" || event.key === "ArrowUp") - next = (current - 1 + enabled.length) % enabled.length; + next = (current - 1 + reachable.length) % reachable.length; else if (event.key === "Home") next = 0; - else if (event.key === "End") next = enabled.length - 1; + else if (event.key === "End") next = reachable.length - 1; if (next == null) return; event.preventDefault(); - selectAndFocus(enabled[next]); + moveTo(reachable[next]); }, - [enabled, selectAndFocus], + [reachable, moveTo], ); return ( @@ -111,6 +144,8 @@ export function SegmentedControl({ > {options.map((option) => { const checked = option.value === selectedValue; + const deadEnd = Boolean(option.deadEnd) && !checked; + const deadEndDescId = `${idPrefix}-${option.value.replace(/[^A-Za-z0-9_-]/g, "-")}-note`; const Icon = option.icon; return ( ); diff --git a/tests/differentials-route.test.ts b/tests/differentials-route.test.ts index 246a9678d..84665c00f 100644 --- a/tests/differentials-route.test.ts +++ b/tests/differentials-route.test.ts @@ -291,6 +291,24 @@ describe("differentials API routes", () => { expect(payload.records?.length).toBe(payload.matches?.length); }); + it("reports the catalogue size in `total`, not the size of the query's own result set", async () => { + const client = createSupabaseMock(); + mockRuntime(client, { demoMode: true }); + const { GET } = await import("../src/app/api/differentials/route"); + + const unfiltered = await GET(request("/api/differentials?kind=diagnosis&limit=10")); + const filtered = await GET(request("/api/differentials?kind=diagnosis&q=delirium&limit=10")); + const unfilteredPayload = (await unfiltered.json()) as { total?: number }; + const filteredPayload = (await filtered.json()) as { records?: unknown[]; total?: number }; + + // `total` answers "how many differentials are there", so a query must not + // move it. It used to measure the returned records, which under a query are + // the ranked matches — so a caller asking for the catalogue figure got its + // own result count back and could not state the real one. + expect(filteredPayload.total).toBe(unfilteredPayload.total); + expect(filteredPayload.total ?? 0).toBeGreaterThan(filteredPayload.records?.length ?? 0); + }); + it("returns scored presentation matches for a query", async () => { const client = createSupabaseMock(); mockRuntime(client, { demoMode: true }); diff --git a/tests/ui-v2-components.dom.test.tsx b/tests/ui-v2-components.dom.test.tsx index 63f1eab23..318cc0d18 100644 --- a/tests/ui-v2-components.dom.test.tsx +++ b/tests/ui-v2-components.dom.test.tsx @@ -502,6 +502,63 @@ describe("SegmentedControl", () => { expect(screen.getByRole("radio", { name: "Comprehensive" })).toHaveFocus(); }); + // `disabled` and `deadEnd` are different states and must not converge. + // `disabled` is "not on offer" and leaves the keyboard path; `deadEnd` is + // "your own narrowing emptied this", and docs/filter-contract.md section 3 + // requires it to stay reachable so it can say so. + it("keeps a dead end on the arrow path, withholding only selection", async () => { + function DeadEndHarness() { + const [value, setValue] = useState("all"); + return ( + + ); + } + render(); + + const dead = screen.getByRole("radio", { name: /^Local/ }); + expect(dead).toHaveAttribute("aria-disabled", "true"); + // Never the native attribute: that would take it out of the tab order. + expect(dead).not.toBeDisabled(); + expect(dead).toHaveAccessibleDescription("Not selectable from here."); + + // Arrowing onto it moves focus so the description is announced, but must + // not commit it — nor silently commit the option before it. + screen.getByRole("radio", { name: /^Any locality/ }).focus(); + await userEvent.keyboard("{ArrowRight}"); + expect(dead).toHaveFocus(); + expect(dead).toHaveAttribute("aria-checked", "false"); + expect(screen.getByRole("radio", { name: /^Any locality/ })).toHaveAttribute("aria-checked", "true"); + + // Clicking it is guarded too. + await userEvent.click(dead); + expect(screen.getByRole("radio", { name: /^Any locality/ })).toHaveAttribute("aria-checked", "true"); + }); + + // The count is split: the unit is announced, the short form is displayed. + it("announces the hint with its unit while displaying only hintLabel", () => { + render( + undefined} + options={[{ value: "all", label: "Any locality", hint: "3 loaded sources", hintLabel: "3" }]} + layout="fit" + />, + ); + const option = screen.getByRole("radio", { name: "Any locality (3 loaded sources)" }); + expect(option).toHaveTextContent(/^Any locality3$/); + }); + // The one-of-N rails this control replaces across the modes all carry a count. // Baking it into `label` would fold the number into the truncating span, so it // gets its own slot — and it must reach the accessible name, or a screen