feat(filters): services adopts the filter contract (PR C) - #1866
feat(filters): services adopts the filter contract (PR C)#1866BigSimmo wants to merge 11 commits into
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
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:48 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 (12)
Comment |
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K5c1ZWQUTwmhgXXdVd2wuK
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:30619385bc
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
This PR is PR C of the filter-contract rollout, migrating services to the shared filter contract by introducing a typed facet/lens model with URL round-tripping, adding the scope segment support, and extending the shared ResultFilterSheet to handle “dense” facet surfaces (>3 facet groups).
Changes:
- Add a new typed services facet/lens module (
src/lib/service-facets.ts) and wire services filtering state into URL params (facets + lens + scope) inServicesNavigatorPage. - Fix a real data-plumbing gap by ensuring every
ServiceRecordproduced from the services catalog carriescatalogPayload. - Extend the shared
ResultFilterSheetwith dense-mode chrome (find-a-filter + collapse-by-default) and document the updated contract + rollout state.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/service-facets.test.ts | Adds unit coverage for services facet/lens behavior, option derivation, union counts, and URL round-trip. |
| tests/search-results-header-band.dom.test.tsx | Adds DOM tests for the new dense facet-group chrome and scope slot rendering in ResultFilterSheet. |
| src/lib/service-facets.ts | Introduces typed services facet dimensions, labels, matching/counting helpers, and URL param serialization. |
| src/lib/service-catalog-mapper.ts | Ensures mapped service records retain the full catalog payload (catalogPayload). |
| src/components/services/services-navigator-page.tsx | Adopts the shared filter contract for services: facets + lens + scope in URL; quick filters moved to suggestions; empty state updated. |
| src/components/clinical-dashboard/result-filter-control.tsx | Enhances ResultFilterSheet with dense-mode UI (find field, collapsible facet groups, scope slot). |
| docs/outstanding-issues.md | Updates rollout tracking row #170 to mark services complete and record remaining modes. |
| docs/filter-contract.md | Updates contract docs for scope slot and dense-mode behavior, and refreshes rollout section. |
| docs/design-system/COMPONENTS.md | Regenerated design-system inventory (SegmentedControl adoption count). |
| docs/design-system/adoption-manifest.json | Regenerated adoption manifest (services now consumes SegmentedControl). |
| docs/branch-review-ledger.md | Appends a ledger entry recording this PR’s review/verification summary. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
BigSimmo
commented
Aug 12, 2026
@codex Address all currently unresolved inline review comments on this PR against the current GitHub head. Make minimal repository-grounded fixes, add/update focused regression tests, run proportionate verification, and push the fixes to this PR branch. Do not merge, close, rebase, force-push, deploy, or alter auto-merge. Please report the exact commit SHA and test results. |
BigSimmo
commented
Aug 12, 2026
@claude Fix both currently unresolved review threads on this PR at the current GitHub head. (1) An unmatched service query followed by Result scope = All items must render faceted catalogue cards rather than the earlier zero-query empty state. Add a focused regression. (2) ResultFilterSheet must not call setChrome during render when chromeResetKey changes; use an effect or preferably a keyed inner state subtree so reset is deterministic without render-phase state mutation, with a regression. Make minimal changes, run focused tests/format/typecheck as proportionate, push only to this PR branch, and report exact SHA/tests. Do not merge, close, rebase, force-push, deploy, alter auto-merge, or add temporary write-capable workflows. |
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. |
…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.
Summary
Testing
|
BigSimmo
commented
Aug 12, 2026
Pull request was closed
BigSimmo
commented
Aug 12, 2026
Final closeout — PR #1866
The PR was not merged. Final merge is left to the user. |
…upersedes #1866) (#1878) * feat(filters): services adopts the filter contract (PR C) 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 * chore(ledger): record PR C review (services filter-contract adoption) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K5c1ZWQUTwmhgXXdVd2wuK * chore(ledger): record services filter-contract adoption (PR C rebase, superseding #1866) * fix(services): fall through to All items scope when the query itself 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. * chore(ledger): record the resultScope query-empty fallthrough fix on PR #1878 * fix(filter-sheet): use useEffect instead of setState-in-render for chromeResetKey changes Co-authored-by: BigSimmo <87357024+BigSimmo@users.noreply.github.com> * docs: point services ledger to superseding PR 1878 * fix(filters): keep selected facet groups discoverable * perf(services): retain only facet tags in registry payload * fix(services): preserve legacy records during facet filtering * test(services): cover legacy records under active facets * fix(services): preserve comma-bearing facet values * test(services): cover comma-bearing facet URLs * test(services): follow quick search and facet clearing journeys * test(services): remove obsolete filter clear journey * test(services): correct URL regex escaping * perf(services): memoize filter group construction * refactor(services): stabilize filter callbacks * fix(services): include stable filter handlers in memo dependencies * docs(ledger): preserve current rows and append PR 1878 records * docs(ledger): preserve the current main history before appending review records * docs(ledger): append new records without rewriting history * perf(services): keep seeded registry payloads facet-sized * fix(services): narrow legacy seeded payloads at response merge * Fix merged services test syntax * Repair merged services selection handler * Remove stale service filter memo dependency * fix: mark unavailable ledger head explicitly * Restore canonical ledgers after main sync * Avoid effect state reset in filter sheet * Align services journeys with filter sheet contract --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Summary
docs/filter-contract.md, tracked in#170): services adopts the shared filter contract — five real facets, one lens, a URL round-trip, the scope segment, and the six query-replacing quick filters evicted to suggested searches.catalogToServiceRecordnever populatedServiceRecord.catalogPayload, so it wasundefinedin demo mode, anonymous/public access, and for any owner who had never runensureRegistrySeeded— i.e. most real traffic. Now populated directly in the mapper.> 3 facet groupsdensity tier (find-a-filter + collapse-by-default) directly to the sharedResultFilterSheet, since services is the first mode to need it (5 facet groups). This is the gap the Codex reviewer flagged on PR feat(filters): make formulation a real facet surface #1858 (tracked there as#309, not yet on this branch since that PR is unmerged) — formulation's 9-option facet group bypasseddocs/filter-contract.mdsection 5's density rule. Below the threshold every existing sheet renders byte-for-byte as before (formulation has one facet group, never crosses it).onClearAllpreviously calledclearServiceQuery, wiping the search box on "Clear filters" — a direct section-6 violation (clearing filters and clearing a search are different intentions). The newonClearAllonly touches facet/lens/scope URL params.What changed
src/lib/service-catalog-mapper.ts—catalogToServiceRecordnow setscatalogPayloadto the fullCatalogService, matching the doc comment's stated intent (service-ranker.ts's "Full source-specific payload retained in the registry JSONB column"). No schema/migration — purely a client-side mapper fix.src/lib/service-facets.ts(new) — the typed facet index: which dimension is a lens vs a facet (measured againstloadServicesSnapshot()—substance_flagsis an exact partition, all 219 services carry exactly one ofgeneral/aod;housing_flagsis a near-partition, 4 services carry two, so it stays a facet), OR-within-dimension/AND-across-dimension matching, non-additive union counts (serviceFacetOptionCount), derive-from-data option lists, per-dimension value labels, and the URL round-trip helpers.src/components/clinical-dashboard/result-filter-control.tsx—ResultFilterSheetgains the> 3 facet groupsdensity chrome (find-a-filter field, per-group collapse-by-default, needle-filtered options that always keep a selected option reachable) and ascopeControlslot for the section-4 scope segment. No behavior change below the threshold.src/components/services/services-navigator-page.tsx— facet/lens/scope selection state lives in the URL alongsideq/group; the scope segment (These results N | All items N) renders when the catalogue (219) is meaningfully larger than the query/group-scoped result set; the six quick filters (Best fit, Crisis, Culturally safe, Phone referral, Free, WA) move toAnswerSuggestionChipsbelow the band as suggested searches instead of living inside the filter sheet; the zero-results empty state fix above.docs/filter-contract.md— documents the density chrome and scope slot, updates the Rollout section, and corrects section 5's table to match what actually shipped (a> 20-band "full-width row list" renderer does not exist yet — that's stilldocument-search-results.tsx's own implementation, ported up in PR F).docs/outstanding-issues.md#170— records services' completion.docs/design-system/{COMPONENTS.md,adoption-manifest.json}— regenerated (npm run design-system:adoption:update) for the newSegmentedControlconsumer (services' scope segment).tests/service-facets.test.ts(new, 13 tests — partition/near-partition measurements, OR/AND matching, non-additive union counts, dead-end-never-on-selected, derive-not-declare, URL round-trip, malformed-payload tolerance) and 7 new DOM tests intests/search-results-header-band.dom.test.tsxfor the density chrome (below/at/above threshold, collapse-by-default, explicit collapse, needle filtering + selected-survives-needle, empty-group-disappears, no-match message, scope slot rendering).Verification
npm run test:focused -- --files <touched paths>— 286 passednpm run test— 6125 passed, 4 skipped, 0 failednpm run lint/npm run typecheck— cleannpm run verify:pr-local— all 15 steps green (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, build, check:rag:fixtures), run against the final diff after formattingnpm run check:bundle-budgeton a freshly rebuilt.next— production 1302.4 KiB gzip vs 1279.1 KiB baseline (within 10% tolerance), mockups 285.1 KiB vs 267.5 KiB baseline (within 25% tolerance)npm run verify:ui— not run: Playwright 1.62.1 wants chromium/chrome-headless-shell revision 1234; this environment has 1194 preinstalled (/opt/pw-browsers). Preflight fails closed withPlaywright browser preflight failed before the production build. Missing executable(s): ... Do not treat a later "N failed" summary as a product regression when the browser binary is absent.Delegated to CI's Production UI job, matching PR feat(filters): adopt the lens contract across the four one-of-N modes #1857/feat(filters): make formulation a real facet surface #1858's own precedent for the same environment limitation.catalogPayloadfix): 0px horizontal overflow at 1440/800/390/320px, sheet open and closed; facet toggle → URL updates (?catchments=...) and footer count updates; substance lens toggle → URL (?substance=aod) and footer (71 showing, matching the measured crosstab); scope segment hidden while browsing all services, shown and correctly counted (These results 45 | All items 219) once narrowed by a query, switching to "All items" updates the URL (?scope=all); suggestion chips render the six evicted quick filters and apply as a new search; dense chrome (find-a-filter field, collapsed-by-default groups) present at every breakpoint; tap targets measured at 48px (themin-h-tapfloor); a genuinely contradictory facet combination reaches the fixed zero-results state, and "Clear filters" there actually recovers the list.npm run check:outstanding-issues— 306 rows (115 open, 191 archived), no ids deleted from baseRisk and rollout
result-filter-control.tsx) already used by 6 other modes. The density addition is gated behind> 3 facet groups, which isfalsefor every existing call site today (formulation has 1), so this PR changes zero rendered output anywhere except services. No schema, Supabase, or provider changes.Notes
classifyPullRequestFilesreturnsclinicalRisk: false,operationalRisk: false,ragRanking: falsefor this diff (matching PR feat(filters): adopt the lens contract across the four one-of-N modes #1857/feat(filters): make formulation a real facet surface #1858), so the Clinical Governance Preflight andRAG impact:sections are not required.docs/outstanding-issues.mdhas no merge driver by design. This PR's edit to row#170will need a hand-resolved conflict against PR feat(filters): make formulation a real facet surface #1858 (open, also edits#170and adds#309) whichever lands second — same situation feat(filters): adopt the lens contract across the four one-of-N modes #1857/feat(filters): make formulation a real facet surface #1858 already navigate forresult-filter-control.tsxitself.docs/filter-contract.md's own "What PR C delivers": a "No cost" facet (cost_fundinghas 86 distinct free-text values and needs normalising first) and retrofitting the scope segment to the four lens modes.Generated by Claude Code