feat(filters): add the facet group kind and the global filter contract - #1847
Conversation
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 26 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe filter system now supports exclusive lens groups and cumulative facet groups. Facet controls use independent pressed states, selection counts, and accessible zero-result handling. Documentation defines the shared contract, and DOM tests cover mixed rendering and keyboard behavior. ChangesFilter contract and UI behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ResultFilterSheet
participant FilterFacetGroup
participant onToggle
User->>ResultFilterSheet: Open filter sheet
ResultFilterSheet->>FilterFacetGroup: Render facet group
User->>FilterFacetGroup: Toggle an option
FilterFacetGroup->>onToggle: Pass selected value
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #9872 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
|
@copilot resolve the merge conflicts on this branch. |
…t-global # Conflicts: # docs/branch-review-ledger.md
Head branch was pushed to by a user without write access
Resolved and pushed in commit |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/clinical-dashboard/result-filter-control.tsx`:
- Line 459: Replace the arbitrary `rounded-[0.25rem]` utility in the result
filter control’s class list with the closest existing Tailwind radius token or
repository semantic radius class, preserving the component’s current visual
intent and avoiding new hardcoded design values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 892feea4-f292-4e1a-a399-7fb7bf3f60c3
📒 Files selected for processing (5)
AGENTS.mddocs/branch-review-ledger.mddocs/filter-contract.mdsrc/components/clinical-dashboard/result-filter-control.tsxtests/search-results-header-band.dom.test.tsx
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
|
Recording why this merges without a The check is a false positive, and
That comment describes this PR precisely: Substantively there is nothing to attest: the change is additive types plus one renderer that no call site reaches yet, and The other CodeRabbit finding — the arbitrary Generated by Claude Code |
…ft alias CI Static PR checks failure: --text-soft is a decoration-only compatibility alias that production must not consume, and check:design-system-contract ratchets its consumer count at zero (textSoftConsumers increased from 0 to 1). --text-muted is what ResultFilterSheet already uses for option.hint, so this also makes the desktop rail and the phone sheet render the same count the same way, which was the point of adding the slot. Same root cause as the radius fix on #1847: lifted from the mockup study, where the token gates do not apply. Also merges origin/main to clear the 26-commit staleness that failed PR mergeability. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
Prerequisite for the filter contract rollout (docs/filter-contract.md, PR #1847). The four one-of-N mode rails that are about to converge onto this primitive — differentials, medication, applications, specifiers — all carry a per-option count, and the primitive had nowhere to put one. Baking it into `label` would fold the number into the truncating span and lose the tabular alignment. Landing it separately because it depends on none of the contract work, and it makes each adoption a pure call-site change. Two things worth knowing: - The hint needs an explicit aria-label. Adjacent label and hint spans concatenate to "All62" in the computed accessible name, and a text node separator does not survive — the name computation normalises inter-element whitespace away. The button is therefore named "All (62)", the shape the differentials rail already used. Both behaviours are pinned by tests, including that a hintless option's name does not drift. - No .design-sync regeneration is needed, contrary to expectation: dtsPropsFor.SegmentedControl references SegmentedControlOption<string> by name rather than expanding it, so adding a field leaves the pinned props string unchanged. Verified — all 56 design-sync tests pass. Note for the adoption PRs: ResultFilterSheet renders option.hint the same adjacent-span way, so its options announce "Crisis12" today. Same fix, tracked with the rollout rather than smuggled in here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
docs/outstanding-issues.md #170 conflicted because both branches rewrote that row's Detail cell — the file deliberately carries no merge driver (union was tried and removed, #133) so overlapping edits fail loudly rather than concatenate. Neither side was a superset: main uniquely explained the closed one-of-N/many-of-N defect and the "unlike #1847" stop nuance, this branch uniquely carried the formulation facet work. Resolved by taking main's file and re-applying a combined row through npm run issues:update rather than hand-editing or taking one side wholesale. result-filter-control.tsx auto-merged cleanly and both changes survive: PR #1857's two aria-label fixes (the "All8" concatenation) and this branch's exported ResultFilterFacetChips plus the narrowed builder return type. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
Summary
Step one of the cross-mode filter rollout: the contract, with no rendered output changed.
kinddefaults tolens, which is what all seven existing call sites already are, so this commit touches none of them 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" discards 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.
ResultFilterGroupis now discriminated bykind.lens(default) — options partition the result set, one-of-N,role="radiogroup"with the existing roving tabindex. Differentials' All/Presentations/Diagnoses; medication's Best/Indication/Safety/Monitor.facet— independent constraints that accumulate, many-of-N, OR within / AND across,role="group"witharia-pressedand one tab stop per option. Formulation's domains; documents' tag groups.navigatekind, deliberately. Services' quick filters and factsheets' categories callrouter.pushand replace the query — choosing one discards the search and its results with no warning and no undo. A control labelled "Filter" must not do that; those move to the composer as suggested searches in a later PR.docs/filter-contract.mdcarries the rules, the scope-segment condition, the density thresholds and the rollout order.A defect the tests caught while being written
The selected-count badge initially sat inside the labelling
<h3>, so a facet group's accessible name became "Domain 1" — the selection count leaking into the dimension's name and changing on every toggle. The id is now on the label text alone, with the badge carrying its ownsr-only"N selected". The DOM test asserts the name isDomain.Two defects documented but not fixed here
resultsis narrowed by family/diagnosis;catalogueMatches(up to 24 of 585 DSM items) is not;footerNotereports the sum (specifiers-home-page.tsx:191-199). Fixed when specifiers adopts.SearchScopeFilters(src/lib/search-scope.ts) is a second, invisible filter surface — 20 keys applied server-side at retrieval, editable from no panel. Reconciling it is tracked separately and is not a prerequisite for adoption.Verification
npm run verify:pr-local— fully green, no failurestests/search-results-header-band.dom.test.tsx, 50 passed in that file: facets render as a pressed-toggle group and accumulate; every facet gets its own tab stop; a zero-yield facet stays focusable, explained and unselectable (aria-disabled, neverdisabled); lens and facet render side by side with their own semantics.git diff --stat origin/mainover all seven mode files is empty. The diff is the component, its tests and the contract doc.UI verification not run: no call site adopts the new kind yet, so there is no rendered change to verify.
verify:uibelongs on the first per-mode adoption PR.Risk and rollout
resultFilterGroup()now stampingkind: "lens"explicitly, which is the value the type already defaulted to.Rollout order per
docs/filter-contract.md: contract (this PR) → one PR per mode to adopt the right kind, derive its option list and retire its desktop rail → services and factsheets evict their query-replacing presets → documents last, porting its find-a-filter and collapse up into the shared component as the >20-option tier.Generated by Claude Code
Summary by CodeRabbit
New Features
Documentation