feat(filters): adopt the lens contract across the four one-of-N modes - #1857
Conversation
Differentials, medication, applications and specifiers each carried a bespoke
desktop rail built from aria-pressed toggles while their phone sheet rendered
the same dimension as role=radiogroup. The two breakpoints disagreed about
whether the dimension was one-of-N; the sheet was right. Each mode now builds
ONE option array and hands it to both SegmentedControl and ResultFilterSheet,
so the counts cannot drift and the semantics cannot diverge.
Also fixed, because both are defects in exactly this surface:
- Specifiers' footerNote reported results.length + catalogueMatches.length
while its groups narrow only results — the sheet claimed to scope a total
its filters could not move. Now reports what it governs, matching the
`${list.length} showing` wording every other mode uses.
- ResultFilterSheet options with a count announced as "All8": the label and
hint spans concatenate and the accessible-name computation normalises the
inter-element whitespace away. Same fix SegmentedControl already carries,
which is what makes a shared option array announce identically on both
surfaces. PR #1848 deferred this to the rollout; this is the rollout.
SegmentedControl gains an optional group-level ariaControls so the launcher
keeps pointing at #launcher-results-panel — the old rail put aria-controls on
each of its six buttons, and converging on the primitive must not drop the
association.
Dead code removed with its last caller: SpecifierFamilyFilterChips. Its
phone-only short label could never render — the band hides filterControls
below sm whenever a page also supplies mobileControls, which specifiers does.
The scope segment is deliberately NOT included; filter-contract.md section 4
gates it on a catalogue meaningfully larger than the result set, which is
services, not these four.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
…aControls PR #1848 concluded no .design-sync regeneration was needed, and that was correct for that change: `hint` went inside SegmentedControlOption, which dtsPropsFor references BY NAME rather than expanding, so the pinned string was untouched. `ariaControls` is the other case — it sits on SegmentedControlProps itself, which the pin does expand, so the string genuinely changed and both check-design-sync-contract and design-sync-visual-exports went red. Regenerated with design-system:design-sync:update. The diff is one line carrying exactly the new prop; no other component's pin moved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 28 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 (17)
Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
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 #9924 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
Production UI shard 3 caught the one assertion this conversion broke: ui-specifiers.spec.ts:82 asserted role="group" on the family rail, which was correct for the aria-pressed chips it replaced and wrong for SegmentedControl. The rail is a lens, so radiogroup is the point of the change rather than an incidental side effect. Found by CI rather than by my own search: I grepped for the component name and the test ids, not for the aria-label string, so a spec that only ever referenced the label slipped through. Shards 1 and 2 passed, so this was the only one. Verified in a browser against the running app: the radiogroup resolves, the old group role returns zero, and the other four assertions in the same test still hold. 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
…e lag guard Two review findings on PR #1858, both real. **P1 (codex) — the Chromium journey still drove the old sheet.** tests/ui-formulation.spec.ts:122 opened the phone sheet and asserted the `Pattern` radiogroup plus `Domain` radio semantics with an "All domains" option. All three are gone: Pattern was the query-replacing group evicted to suggestions, and Domain is now a facet. Left as-is this fails Production UI exactly the way ui-specifiers.spec.ts did on PR #1857. Rewritten to exercise the new behaviour rather than merely pass: the suggestions row is present, Pattern resolves to zero, Domain is a role="group" with no radios and nine derived options, and two toggles accumulate to "2 filters active". **LOW (sentry) — counts disagreed with results during the deferred lag.** For the frame where the live query has text but `useDeferredValue` has not caught up, `results` deliberately reports nothing rather than scoring the whole catalogue, but the counts were still computed against `searchQuery` — which is the empty deferred value, i.e. the full catalogue. The sheet would show "0 showing" beside nine non-zero counts. That is precisely the filter-vs-own- predicate disagreement this contract exists to remove, so it is worth fixing even at LOW severity. Counts now report 0 while ranking is pending, and the dead-end marker is suppressed there: a zero during the lag means "not scored yet", not "nothing matches", and would otherwise flash all nine options inert. Verified in a browser at 390px: every assertion in the rewritten journey passes against the running app. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011btGFwWKYFWDs5McQkqz9J
Summary
Step two of the filter contract rollout (
docs/filter-contract.md, landed in PR #1847;SegmentedControl's hint slot landed in PR #1848). Four modes adopt thelenskind for real.The defect being fixed is a disagreement, not a style. Differentials, medication, applications and specifiers each carried a bespoke desktop rail built from
aria-pressedtoggles, while the phone sheet rendered the same dimension asrole="radiogroup". One of those says many-of-N and the other says one-of-N. The sheet was right — a result cannot be both a Presentation and a Diagnosis. Each mode now builds one option array and hands it to bothSegmentedControlandResultFilterSheet, so the counts cannot drift and the semantics cannot diverge. Differentials had literally been declaring its three counts twice, once ascountand once ashint.Specifiers'
footerNotecounted what its filters do not govern. It reportedresults.length + catalogueMatches.length, but both groups narrow onlyresults—catalogueMatcheskeys on the query alone. The number sat still while a filter visibly changed the list. Now reportsresults.length, matching the${list.length} showingwording every other mode already uses. This is the invariant in filter-contract.md §6.ResultFilterSheetannounced a counted option as "All8". The label and hint spans concatenate, and the accessible-name computation normalises the inter-element whitespace away, so a text-node separator cannot fix it — the same defect and samearia-labelfixSegmentedControlalready carries. PR feat(ui): give SegmentedControl an option hint slot #1848 deferred this "to the rollout"; this is the rollout, and leaving it would mean one shared option array announcing two different ways on the two surfaces it now feeds. Applies to both group kinds, since facet counts would read "Crisis12".SegmentedControlgains an optional group-levelariaControls. The launcher's old rail putaria-controls="launcher-results-panel"on each of its six buttons. Nothing asserts those per-button ids, so dropping them was safe — but losing an accessibility association to a refactor is not the same as it being correct. One group-level reference, because the radiogroup is the control and the radios are its options.SpecifierFamilyFilterChipsdeleted with its last caller. Its phone-only short label could never render:SearchResultsHeaderBandapplieshidden sm:blocktofilterControlswhenever a page also suppliesmobileControls(search-results-header-band.tsx:583), which specifiers does. SoSegmentedControl's singlelabelloses nothing.The scope segment is deliberately not here.
docs/filter-contract.md§4 gates it on a catalogue meaningfully larger than the result set; for these four the two segments would show near-identical numbers. It lands with services, where 219 items against a typical result set makes it the only escape from a filtered-to-zero state that does not discard the query.Net −75 lines of product code.
docs/outstanding-issues.md#170updated with what remains: formulation, services, factsheets, therapy-compass, documents.Verification
npm run verify:pr-local—completed: check:runtime, check:installed-lock-parity, format:changed, sitemap:check, docs:check-index, docs:check-inventory, docs:check-scripts, docs:check-links, check:branch-review-ledger, check:outstanding-issues, lint, typecheck, test.buildfailed on its first pass and that failure was/issues#210, not this diff: the browser proof below needsnpm run ensure, and the running dev server writes.next/dev/types/validator.ts, which the production typecheck then chokes on (error TS1002: Unterminated string literal). Re-run with the server stopped and.nextdeleted:rm -rf .next && npm run build && npm run check:bundle-budget— therm -rfis mandatory or the check reads stale output and reports a false pass;.next/BUILD_IDmtime confirmed against this build.npm run verify:uiwhen UI, routing, styling, browser behavior, reduced-motion, or forced-colors behavior changed — UI verification not run as the full Chromium gate; proved instead against the running dev server at 1440 / 800 / 390 / 320 px on every changed mode, becausecheck:playwright-browser-revisionreports the known PW-1.62.1-wants-chromium-1234 / only-1194-installed drift (/issues#255) and forcing a mismatched binary is explicitly disallowed. Measured 0 px horizontal overflow at every width, 48 px tap targets, roving tabindex:Result type:All (8),Presentations (1),Diagnoses (7)Best (50),Indication (45),Safety (49),Monitor (13),.medication-filter-strippreservedaria-controls="launcher-results-panel"on the group, absent on each radioaria-labelfix:All (8)/Presentations (1)/Diagnoses (7), footer8 showingFull offline unit suite —
Tests 6100 passed | 4 skipped (6105),Test Files 557 passed (558). The one non-passing file istests/design-sync-contract.test.ts, which timed out at 30 s rather than failing an assertion: it shells out tocheck-design-sync-contract.mjs, which takes 7.3 s standalone and exceeds the timeout under 558-file parallel load. It passes in isolation (7 passed), andnpm run check:design-system-contractpasses directly —design-sync contract checked: 54 components and 7 guidelines. Not registered intests/flake-ledger.json; flagged here rather than silently re-run.tests/ui-v2-components.dom.test.tsx— 83 passed (+2: the group carriesaria-controlsand the radios do not; a rail governing no region omits it entirely)tests/search-results-header-band.dom.test.tsx— 51 passed (+1: a counted option is namedlabel (hint)on both group kinds, neverAll8)tests/medication-prescribing-workspace.dom.test.tsx— 6 passed, helper movedgetByRole("button")→getByRole("radio")and fouraria-pressedassertions →aria-checkedtests/favourites-auth-gate.dom.test.tsx— 13 passed, same button→radio change for the launcher'sSavedcategorynpm run lint,npm run typecheck— cleannpx prettier --check .—All matched files use Prettier code style!Not run, with reasons:
eval:retrieval:quality/eval:rag/check:production-readiness—classifyPullRequestFilesreturnsclinicalRisk: false,operationalRisk: false,ragRanking: false,ui: true; no retrieval, ranking, ingestion or answer-path file is touched.verify:release— provider-backed, not authorised and not a release.Risk and rollout
aria-labelonly whenoption.hintis present, so the three modes that pass no hints render and announce exactly as before.29b540b(the adoption) and5cf8871(the regenerated.design-syncpin). Nothing outside these files consumesariaControls.Notes
A correction worth recording for the next adopter. PR #1848 concluded that no
.design-syncregeneration was needed, and that was right for that change:hintwent insideSegmentedControlOption, whichdtsPropsForreferences by name rather than expanding.ariaControlsis the other case — it sits onSegmentedControlPropsitself, which the pin does expand. Bothcheck-design-sync-contractanddesign-sync-visual-exportswent red until regenerated. So the rule is not "primitive changes never need regen", it is "changes to the expanded props type need regen". Regenerated diff is one line carrying exactly the new prop.docs/design-system/COMPONENTS.mdmovesSegmentedControlconsumers 3 → 7, one per adopting mode — written by the pre-commit hook, not by hand.Next: PR B (formulation — derive the domain list, convert to
facet, evict the query-replacing pattern presets), then services, factsheets, therapy-compass, documents last.Generated by Claude Code