Replace mobile search filter rails with dropdowns#1189
Merged
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Contributor
📝 WalkthroughWalkthroughMobile search result filters now use reusable accessible select controls on narrow screens across clinical, search, and navigation pages. Desktop filter controls remain available at larger breakpoints, with responsive behavior and accessibility covered by updated tests. ChangesMobile result filter controls
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant MobileResultFilterControl
participant SearchPage
participant SearchResultsHeaderBand
User->>MobileResultFilterControl: select filter option
MobileResultFilterControl->>SearchPage: invoke onChange(value)
SearchPage->>SearchResultsHeaderBand: update filter state or route
SearchResultsHeaderBand->>User: render filtered mobile controls and results
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/ui-accessibility.spec.ts (1)
406-406: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope the tab absence assertion to differential results.
This currently fails if any unrelated visible tab is added elsewhere on the page. Query within
differentials-search-resultsinstead.Proposed change
- await expect(page.getByRole("tab")).toHaveCount(0); + await expect(page.getByTestId("differentials-search-results").getByRole("tab")).toHaveCount(0);🤖 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/ui-accessibility.spec.ts` at line 406, Update the tab absence assertion in the accessibility test to scope the getByRole("tab") query within the differentials-search-results container, so unrelated tabs elsewhere on the page do not affect the expected zero count.
🤖 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/services/services-navigator-page.tsx`:
- Around line 628-644: Update the mobileControls MobileResultFilterControl
options to always include the "current" reset entry, including when
activeQuickFilter is truthy, and route selecting "current" through the same
clear/reset logic used by the desktop Clear action instead of ignoring it in
onChange. Preserve the existing quick-filter options and labels for non-reset
values.
---
Nitpick comments:
In `@tests/ui-accessibility.spec.ts`:
- Line 406: Update the tab absence assertion in the accessibility test to scope
the getByRole("tab") query within the differentials-search-results container,
so unrelated tabs elsewhere on the page do not affect the expected zero count.
🪄 Autofix (Beta)
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 Plus
Run ID: fa019b13-0e2e-4d8e-830c-fbe2e91c7ea2
📒 Files selected for processing (18)
docs/branch-review-ledger.mdsrc/components/applications-launcher-page.tsxsrc/components/clinical-dashboard/differentials-home.tsxsrc/components/clinical-dashboard/document-search-results.tsxsrc/components/clinical-dashboard/medication-prescribing-workspace.tsxsrc/components/clinical-dashboard/search-results-header-band.tsxsrc/components/factsheets/factsheets-search-page.tsxsrc/components/formulation/formulation-home-page.tsxsrc/components/services/services-navigator-page.tsxsrc/components/specifiers/specifiers-home-page.tsxsrc/components/therapy-compass/screens/search-screen.tsxtests/search-results-header-band.dom.test.tsxtests/ui-accessibility.spec.tstests/ui-formulation.spec.tstests/ui-smoke.spec.tstests/ui-specifiers.spec.tstests/ui-stress.spec.tstests/ui-tools.spec.ts
Comment on lines
+628
to
+644
| mobileControls={ | ||
| <MobileResultFilterControl | ||
| label="Filter" | ||
| ariaLabel="Apply a quick service filter" | ||
| testId="service-quick-filter-select" | ||
| value={activeQuickFilter?.query ?? "current"} | ||
| options={[ | ||
| ...(activeQuickFilter | ||
| ? [] | ||
| : [{ value: "current", label: query.trim() ? "Current search" : "All services", disabled: true }]), | ||
| ...serviceQuickFilters.map((filter) => ({ value: filter.query, label: filter.label })), | ||
| ]} | ||
| onChange={(value) => { | ||
| if (value !== "current") applyServiceQuery(value); | ||
| }} | ||
| /> | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Mobile select loses the "Clear quick filters" affordance once a quick filter matches.
When activeQuickFilter is truthy, the "current" sentinel entry is entirely omitted from options, so there is no entry left that resets to "All services." Previously this whole row (including the desktop "Clear" button) was visible on mobile; now filterControls is hidden below sm (because mobileControls is set), and the replacement mobile select never offers the reset action the desktop Clear button provides. Every other mobile select in this PR (documents/differentials/medications/formulation/specifiers/therapy/factsheets/applications) keeps an unconditional "all"/reset entry — this is the only page missing it, and it's a real capability the mobile ribbon has to lose here, not a stylistic difference.
🐛 Proposed fix: keep an unconditional reset entry and route it to the same clear logic as the desktop button
<MobileResultFilterControl
label="Filter"
ariaLabel="Apply a quick service filter"
testId="service-quick-filter-select"
- value={activeQuickFilter?.query ?? "current"}
+ value={activeQuickFilter?.query ?? "current"}
options={[
- ...(activeQuickFilter
- ? []
- : [{ value: "current", label: query.trim() ? "Current search" : "All services", disabled: true }]),
+ activeQuickFilter
+ ? { value: "current", label: "All services" }
+ : { value: "current", label: query.trim() ? "Current search" : "All services", disabled: true },
...serviceQuickFilters.map((filter) => ({ value: filter.query, label: filter.label })),
]}
onChange={(value) => {
- if (value !== "current") applyServiceQuery(value);
+ if (value === "current") {
+ if (activeQuickFilter) setLocalQuery({ urlQuery, value: "" });
+ } else {
+ applyServiceQuery(value);
+ }
}}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| mobileControls={ | |
| <MobileResultFilterControl | |
| label="Filter" | |
| ariaLabel="Apply a quick service filter" | |
| testId="service-quick-filter-select" | |
| value={activeQuickFilter?.query ?? "current"} | |
| options={[ | |
| ...(activeQuickFilter | |
| ? [] | |
| : [{ value: "current", label: query.trim() ? "Current search" : "All services", disabled: true }]), | |
| ...serviceQuickFilters.map((filter) => ({ value: filter.query, label: filter.label })), | |
| ]} | |
| onChange={(value) => { | |
| if (value !== "current") applyServiceQuery(value); | |
| }} | |
| /> | |
| } | |
| mobileControls={ | |
| <MobileResultFilterControl | |
| label="Filter" | |
| ariaLabel="Apply a quick service filter" | |
| testId="service-quick-filter-select" | |
| value={activeQuickFilter?.query ?? "current"} | |
| options={[ | |
| activeQuickFilter | |
| ? { value: "current", label: "All services" } | |
| : { value: "current", label: query.trim() ? "Current search" : "All services", disabled: true }, | |
| ...serviceQuickFilters.map((filter) => ({ value: filter.query, label: filter.label })), | |
| ]} | |
| onChange={(value) => { | |
| if (value === "current") { | |
| if (activeQuickFilter) setLocalQuery({ urlQuery, value: "" }); | |
| } else { | |
| applyServiceQuery(value); | |
| } | |
| }} | |
| /> | |
| } |
🤖 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/services/services-navigator-page.tsx` around lines 628 - 644,
Update the mobileControls MobileResultFilterControl options to always include
the "current" reset entry, including when activeQuickFilter is truthy, and route
selecting "current" through the same clear/reset logic used by the desktop Clear
action instead of ignoring it in onChange. Preserve the existing quick-filter
options and labels for non-reset values.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fields=index, differentials abort/debounce, universal documents typeahead soft-timeout (750ms), shared(search-app)shell to avoid composer remount, and Answer rate-limit in-memory fallback outside production./services→/dsm) syncssearchModeduring render (no stale-mode paint) even when the query string is unchanged; extracted ClinicalDashboard lazy imports to stay under the maintainability budget.RAG impact: no retrieval behaviour change — typeahead documents domain timeout and shell URL sync only; ranking formulas and full
/api/searchretrieval path unchanged.Verification
npm run verify:pr-local— focused Vitest on touched sources (362) plus api-rate-limit / search-shell / universal / route / site-map suites green;docs:check-indexOKverify:uinot required for this pass; mode-home smoke vianpm run ensurereturned HTTP 200 for/,/services,/dsm,/documents/search,/therapy-compass,/?mode=prescribing, and/api/answer/streamreturned 200 after the rate-limit fallback fixeval:retrieval:latency/ soak / live OpenAI canary — approval-gated provider work; not needed for timeout-only typeahead changeRisk and rollout
GlobalSearchShelllayouts and prior timeout/fallback behaviourClinical Governance Preflight
[REDACTED]([REDACTED])Notes
fields=indexremains for identity-only consumers (cross-mode links).Summary by CodeRabbit
New Features
Accessibility
Tests