feat(filters): services adopts the filter contract (PR C, rebased — supersedes #1866) - #1878
Conversation
Five real facets (catchments, age_groups, setting_flags, acuity_flags, housing_flags) plus substance_flags as a lens (an exact partition: all 219 services carry exactly one of general/aod), a URL round-trip alongside q/group, the scope segment, and the six query-replacing quick filters evicted to suggested searches below the band. Fixes catalogToServiceRecord never populating ServiceRecord.catalogPayload, which the rollout plan flagged as a spike to verify — it was undefined in demo mode, anonymous/public access, and for any owner who had never run ensureRegistrySeeded. Also fixes onClearAll wiping the search box (a docs/filter-contract.md section 6 violation) and a stale/dead-end zero-results empty state. Adds the >3-facet-groups density tier (find-a-filter + collapse-by-default) directly to the shared ResultFilterSheet, since services is the first mode dense enough to need it. Below the threshold every existing sheet renders unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K5c1ZWQUTwmhgXXdVd2wuK
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K5c1ZWQUTwmhgXXdVd2wuK
Resolves the expected conflict from PR B (#1858, formulation) merging while PR C was open: reconciles FilterFacetGroup -> ResultFilterFacetChips (PR B's rename, now exported with idPrefix) with PR C's options/disclosure additions for the >3-facet-groups density chrome, keeping both. Also reconciles the #170 outstanding-issues row to describe the true current state (PR A/B merged, PR C rebased here, PR D open as #1872) rather than taking either side wholesale. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K5c1ZWQUTwmhgXXdVd2wuK
|
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: 25 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 (11)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69dd04de16
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR completes the services mode’s adoption of the shared filter contract by introducing a typed facet/lens model (including URL round-tripping and scope segmentation) and extending the shared ResultFilterSheet to support “dense” facet surfaces (>3 facet groups) via find-a-filter + collapse-by-default chrome.
Changes:
- Add a new typed services facet system (
src/lib/service-facets.ts) with option derivation, union counting, and URL param helpers, plus test coverage. - Update services navigation to use facet/lens/scope URL state, evict query-replacing quick filters into suggestion chips, and wire the shared filter sheet enhancements.
- Enhance
ResultFilterSheetto support dense facet-group chrome and ascopeControlslot; update docs and design-system adoption inventory.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/service-facets.test.ts | Adds unit tests covering services facet/lens matching, counting, and URL round-trip behavior. |
| tests/search-results-header-band.dom.test.tsx | Adds DOM tests for the dense facet-group sheet behavior (needle filtering, collapse rules, scope slot). |
| src/lib/service-facets.ts | Introduces typed facet/lens logic for services including derive-from-data option lists and union counting. |
| src/lib/service-catalog-mapper.ts | Ensures ServiceRecord.catalogPayload is populated so typed tags are available consistently. |
| src/components/services/services-navigator-page.tsx | Wires facets/lens/scope into services browsing, updates empty-state behavior, and moves quick filters to suggestions. |
| src/components/clinical-dashboard/result-filter-control.tsx | Adds dense facet-group chrome and scope slot support to the shared filter sheet. |
| docs/outstanding-issues.md | Updates rollout tracking entry (#170) to reflect current state and references. |
| docs/filter-contract.md | Updates contract documentation for scope slot behavior and dense facet-group tier details. |
| docs/design-system/COMPONENTS.md | Regenerates component usage counts (SegmentedControl adoption). |
| docs/design-system/adoption-manifest.json | Regenerates adoption manifest to include services usage of SegmentedControl. |
| docs/branch-review-ledger.md | Updates ledger entries relevant to the superseded/rebased work. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…matched nothing rankedMatches is query-only and never reflects resultScope, so a search that matched zero services in the whole catalogue always showed the query-empty state — even after the reader explicitly widened to "All items", which exists precisely to bypass that. Gate the query-empty branch on resultScope === "results" so "All items" falls through to the facet-driven branches, which correctly ignore the query in that scope. Independently found and verified per docs/filter-contract.md section 4; not imported from the untrusted commits reported on PR #1866/branch claude/plan-implementation-3t2vl5.
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #10303 (cancelled). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
|
@codex Work PR #1878 from current head |
|
@copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. |
…romeResetKey changes Co-authored-by: BigSimmo <87357024+BigSimmo@users.noreply.github.com>
Head branch was pushed to by a user without write access
Both review threads addressed in commit
|
|
@copilot Fix the three still-unresolved review comments on current head |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
tests/search-results-header-band.dom.test.tsx (1)
833-897: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting the live match count.
The dense sheet renders a polite live region with
N filter(s) match "needle". No test pins that string, so the singular/plural branch and the count source can regress silently. Add one assertion in this test.💚 Optional: pin the announcement
await user.type(screen.getByPlaceholderText("Find a filter…"), "peel"); + // One matched label plus the surviving selected option. + expect(screen.getByText('2 filters match "peel"')).toBeInTheDocument();🤖 Prompt for 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. In `@tests/search-results-header-band.dom.test.tsx` around lines 833 - 897, Add an assertion in the first ResultFilterSheet test to verify the live-region announcement reports the expected matching filter count for the “peel” needle, covering the count source and singular/plural wording. Keep the existing option visibility assertions unchanged.src/components/clinical-dashboard/result-filter-control.tsx (1)
625-649: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueThe reset effect is correct; consider dropping the redundant mount run.
The
useEffectfixes the render-phasesetChromedefect. Thescopedfallback also prevents a stale-needle flash on the transitional render. One small cost remains: the effect runs on mount and writes a fresh object identical in meaning to the initializer, which schedules one extra render for every sheet instance. You can skip that by comparing the key before writing.♻️ Optional: skip the no-op reset on mount
useEffect(() => { - setChrome({ key: chromeResetKey, needle: "", expanded: new Set(), collapsed: new Set() }); + setChrome((current) => + current.key === chromeResetKey + ? current + : { key: chromeResetKey, needle: "", expanded: new Set(), collapsed: new Set() }, + ); }, [chromeResetKey]);🤖 Prompt for 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. In `@src/components/clinical-dashboard/result-filter-control.tsx` around lines 625 - 649, Update the reset effect in the component using chromeResetKey and setChrome to avoid writing state when chrome.key already matches chromeResetKey. Keep resetting needle, expanded, and collapsed for changed keys while skipping the redundant mount update.
🤖 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 `@docs/branch-review-ledger.md`:
- Line 930: Update the 2026-08-12 ledger entry to explicitly record that
Playwright UI verification was not run because of the browser revision mismatch.
Preserve the existing unit, typecheck, lint, and Prettier results while adding
this limitation to the verification summary.
- Around line 886-930: The branch-review ledger must remain append-only; restore
the replaced historical tail and preserve every existing row, then append the
new record at EOF using npm run ledger:append -- --supersede when this is a
correction or superseding entry. Do not rewrite or delete prior ledger records.
In `@docs/outstanding-issues.md`:
- Line 196: Update the Services references in the durable issue ledger through
the repository’s /issues workflow, replacing both PR C/#1866 references with PR
`#1878` and preserving the row’s existing objectives and status. Do not hand-edit
the row directly; ensure the workflow records `#1878` as superseding `#1866` without
restoring duplicate or superseded work.
In `@src/components/clinical-dashboard/result-filter-control.tsx`:
- Around line 783-796: Update the selectedCount calculation in the groups.map
facet branch to use group.selected.size, matching the count rendered by
ResultFilterFacetChips. Keep the existing auto-open condition and badge behavior
otherwise unchanged so stale or unmatched URL selections still open the group.
In `@src/components/services/services-navigator-page.tsx`:
- Around line 447-503: Memoize both filter-group constructions to avoid
recomputation on each render. Wrap the substanceGroup expression in useMemo with
dependencies [facetBaseMatches, facetSelection, searchableRecords], and wrap
facetGroups in useMemo with [facetBaseMatches, facetSelection, substanceLens,
searchableRecords], preserving the existing group contents and callbacks.
In `@src/lib/service-catalog-mapper.ts`:
- Around line 354-362: Update the catalog payload assignment in the
service-record mapping to store only the typed facet metadata as { tags:
service.tags }, rather than the full source service. Apply the same narrowing in
buildDefaultServiceRows when restoring CatalogService data, while leaving form
payload handling unchanged.
In `@src/lib/service-facets.ts`:
- Around line 124-151: Update serviceCatalogTags, matchesServiceFacets, and
matchesSubstanceLens to distinguish missing catalogPayload.tags from present tag
metadata. Treat records with no facet metadata as matching active facet
selections and non-"all" substance lenses, while preserving existing
AND-across-dimensions and exact-value matching for tagged records. Add focused
tests in service-facets.test.ts using the existing bare record fixture for both
an active facet and a non-"all" lens.
- Around line 237-260: Make the catchments round-trip delimiter-safe in
serviceFacetSelectionFromParams and writeServiceFacetSelectionToParams by
encoding each catchment value before joining and decoding each value after
splitting, so commas within a tags.catchments option remain part of that option.
Preserve the existing behavior for the other facet dimensions and continue
omitting empty selections.
In `@tests/service-facets.test.ts`:
- Around line 173-179: Extend the bare-record test around serviceCatalogTags and
matchesServiceFacets to create non-empty selections for at least one service
facet and one lens, then assert the missing catalogPayload.tags record is
handled without throwing and matches the expected active-selection behavior.
Keep the existing empty-selection assertions, and use the serviceFacetDimensions
and established selection helpers to target both cases.
---
Nitpick comments:
In `@src/components/clinical-dashboard/result-filter-control.tsx`:
- Around line 625-649: Update the reset effect in the component using
chromeResetKey and setChrome to avoid writing state when chrome.key already
matches chromeResetKey. Keep resetting needle, expanded, and collapsed for
changed keys while skipping the redundant mount update.
In `@tests/search-results-header-band.dom.test.tsx`:
- Around line 833-897: Add an assertion in the first ResultFilterSheet test to
verify the live-region announcement reports the expected matching filter count
for the “peel” needle, covering the count source and singular/plural wording.
Keep the existing option visibility assertions unchanged.
🪄 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: 30fe7d88-fbe1-4b14-af07-eb4823e51e42
📒 Files selected for processing (12)
docs/branch-review-ledger.mddocs/design-system/COMPONENTS.mddocs/design-system/adoption-manifest.jsondocs/filter-contract.mddocs/outstanding-issues.mdsrc/components/clinical-dashboard/result-filter-control.tsxsrc/components/services/services-navigator-page.tsxsrc/lib/service-catalog-mapper.tssrc/lib/service-facets.tstests/search-results-header-band.dom.test.tsxtests/service-facets.test.tstests/services-navigator-scope-empty-state.dom.test.tsx
Summary
Publication status
Testing
|
|
@claude Address every currently unresolved inline review thread on this PR against the current GitHub head. Verify each finding against current code and collapse duplicate reports. Required outcomes where still valid: update the services Playwright journey for suggestion-chip behavior and separately exercise clearing an active facet; prevent owner/legacy records without typed |
|
Hi @BigSimmo — thanks for the @claude mention. Open this in Claude Code on the web to continue here: https://claude.ai/code Context: Automated backlink from the Claude backlink workflow. Claude Code can pick up the mention and push a scoped fix to the branch. |
|
Fixed the exact current-head static CI failure in |
|
@claude Please fix the exact-head CI failure on Root cause is proven by CI run Required changes:
|
|
Hi @BigSimmo — thanks for the @claude mention. Open this in Claude Code on the web to continue here: https://claude.ai/code Context: Automated backlink from the Claude backlink workflow. Claude Code can pick up the mention and push a scoped fix to the branch. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
@copilot Fix the two exact-head CI failures on current head CI proof: run
|
…ces-v2' into codex/pr1878-babysit
PR #1878 has now landed the Services filter-contract rollout on main with the same capabilities plus subsequent CI and regression fixes. Resolve this competing branch by retaining the current main tree rather than overwriting it with the older implementation.
Flagged in the previous commit's PR body but not actually captured as a trackable item: the services filter-contract adoption (#1878, then #1882) was independently implemented twice by concurrent sessions, and whatever conflict resolution happened between them when both merged was never reviewed line-by-line. A spot check found registry-records.ts has no ServiceRecord.facets carrier and no corresponding test file on main, so #1878's approach is what survived rather than #1882's — likely equivalent, not verified. Queues a P2 add request via the outstanding-issues inbox so this doesn't only live as prose inside an archived row's outcome text.
Summary
This PR supersedes #1866 and intentionally does not build on its branch. #1866's branch (
claude/plan-implementation-3t2vl5) received 6 commits after I opened it that I did not make and cannot attribute with confidence — they add a self-triggering.github/workflows/*.ymlwithcontents: write, resolve the exact same merge conflicts I resolve below, patch three files (plausibly fixing one real bug — see "All items" note below), then delete the workflow file and push directly back to the branch, bypassing review. That workflow's trigger, self-deletion, and direct-push behavior are exactly what this repo's ownAGENTS.mdprohibits ("AutomaticGITHUB_TOKENbranch updates are prohibited"). I have not merged, incorporated, or acted on anything from those 6 commits, andclaude/plan-implementation-3t2vl5/ #1866 are left untouched for direct investigation (GitHub audit log, Actions run history) rather than anything I can safely resolve from here. This PR is a clean rebase of my own verified work onto currentmain, pushed to a new branch instead.Content-wise, this is identical to what #1866 described:
catalogToServiceRecordnever populatingServiceRecord.catalogPayload(undefined in demo mode/public access/unseeded owners — a real gap, not just the theoretical one the plan flagged).> 3 facet groupsdensity tier (find-a-filter + collapse-by-default) directly to the sharedResultFilterSheet— services is the first mode dense enough to need it, closing#309(formulation's facet group bypassed the density rule when PR feat(filters): make formulation a real facet surface #1858 merged).onClearAllwiping the search box (adocs/filter-contract.mdsection 6 violation), and a stale zero-results empty state.main(which picked up PR feat(filters): make formulation a real facet surface #1858/formulation while feat(filters): services adopts the filter contract (PR C) #1866 was open) — reconciles PR feat(filters): make formulation a real facet surface #1858'sFilterFacetGroup→ResultFilterFacetChipsrename with this PR'soptions/disclosureadditions to that same function, by hand. Verified both formulation's and services' facet rendering work correctly through the merged component (browser-checked, not just typechecked).What changed
Same file set as #1866, now current against
main:src/lib/service-catalog-mapper.ts—catalogToServiceRecordsetscatalogPayloadto the fullCatalogService.src/lib/service-facets.ts(new) — typed facet index: lens vs facet classification (measured), OR/AND matching, non-additive union counts, derive-from-data option lists, value labels, URL round-trip helpers.src/components/clinical-dashboard/result-filter-control.tsx—ResultFilterSheetgains the> 3 facet groupsdensity chrome and ascopeControlslot; merged with PR feat(filters): make formulation a real facet surface #1858'sResultFilterFacetChipsrename.src/components/services/services-navigator-page.tsx— facet/lens/scope state in the URL, the scope segment, quick filters evicted toAnswerSuggestionChips, the zero-results empty-state fix.docs/filter-contract.md,docs/outstanding-issues.md#170— updated to describe the actual current state of the rollout (PR A/B merged, this PR, PR D#1872open).tests/service-facets.test.ts(13 tests), 7 new DOM tests intests/search-results-header-band.dom.test.tsx.One thing I have not independently re-derived: the embedded script in the suspicious workflow also patches a real-looking bug in this same file — the "no services match" empty-state branch in
services-navigator-page.tsxchecksrankedMatches.length === 0(query-based) ahead of the scope-aware branch, so switching to "All items" when a query matches zero services would still show the empty state instead of the catalogue. I have not applied that patch from the suspicious commit. I'll verify and fix this myself as a follow-up if it reproduces — flagging it here rather than silently importing a third party's fix perAGENTS.md's "third-party fix claims stay unverified until checked."Verification
Identical commit content to what was already verified for #1866 (same SHA before the branch rename) — not re-run here since nothing changed:
npm run test— 6158 passed, 4 skipped, 0 failed (includes everythingmainpicked up from PR feat(filters): make formulation a real facet surface #1858 and others in the interim)npm run lint/npm run typecheck— cleannpm run verify:pr-local— all 15 steps green, run twice against the final rebased diffnpm run check:bundle-budgeton a freshly rebuilt.next— production 1307.9 KiB gzip vs 1279.1 KiB baseline (within 10% tolerance), mockups 294.5 KiB vs 267.5 KiB baseline (within 25% tolerance)ResultFilterFacetChips) and services' dense-chrome sheet (this PR's code, through the same shared component) both verified working in a real browser after the merge.npm run verify:ui— not run: Playwright chromium/chrome-headless-shell revision mismatch in this environment (1234 wanted, 1194 preinstalled), same as feat(filters): adopt the lens contract across the four one-of-N modes #1857/feat(filters): make formulation a real facet surface #1858/feat(filters): services adopts the filter contract (PR C) #1866/feat(filters): converge factsheets category filter onto SegmentedControl (PR D) #1872. Delegated to CI.Risk and rollout
result-filter-control.tsx(used by 6 other modes). No schema, Supabase, or provider changes.claude/plan-implementation-3t2vl5.Notes
classifyPullRequestFilesreturnsclinicalRisk: false,operationalRisk: false,ragRanking: falsefor this diff, so the Clinical Governance Preflight andRAG impact:sections are not required.claude/plan-implementation-3t2vl5directly — I have not closed it myself, since that needs your explicit call, not mine.docs/outstanding-issues.md#170in this PR already accounts for feat(filters): services adopts the filter contract (PR C) #1866 being effectively replaced by this PR.Generated by Claude Code
Summary by CodeRabbit