fix(search-band): drop the sort segments from the phone results bar - #1689
Conversation
The shared results band puts count, query, sort and Filter on one line. On
a phone the two sort segments ("Relevance" / "A-Z") take roughly half of
that line, so the query truncates to pay for a control that is set about
once a session and already defaults to the order a phone reader wants.
ResultSortControl is now sm-and-up. Only the affordance moves: `?sort=`
still carries an alpha order onto a phone from a link or a wider session,
and the results honour it. The display class lives in the component's own
base string because `cn` here is a plain join with no Tailwind conflict
resolution, so a caller's `hidden` would resolve by stylesheet order
rather than by intent.
Pages whose only utility is sort (forms) would have kept an empty
utilities group mounted on a phone -- and in `inline` placement below
414px that child is `w-full basis-full`, i.e. a blank second line. Guard
it with hasPhoneUtilities.
Tests move rather than disappear: ui-smoke asserts the group is hidden at
390px and exercises the A-Z/Relevance interaction at the 1440px viewport
the same test already resizes to, and the two ui-tools differentials
phone tests assert hidden while keeping the 44px tap floor on the page
filter. The 320-540px clip sweep measured the sort node by name, which
would have measured a display:none element and passed blindly at every
width; it now measures the last rendered control.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013mcqh46SDdiR5KxScihkFQ
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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: 13 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 (4)
📝 WalkthroughWalkthroughThe search results header now hides sort controls below ChangesResponsive search utilities
Lighthouse baselines
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant SearchResultsHeaderBand
participant ResultSortControl
Browser->>SearchResultsHeaderBand: Render search results at viewport width
SearchResultsHeaderBand->>ResultSortControl: Apply responsive visibility
Browser->>SearchResultsHeaderBand: Provide URL sort query
SearchResultsHeaderBand->>ResultSortControl: Preserve URL-driven sorting
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
The HeadlessChrome/151 baseline refresh landed with `0.060` / `0.080` trailing zeros, which Prettier normalises to `0.06` / `0.08`. That is the whole diff -- no measurement changes. `format:changed` runs Prettier over the PR's entire changed set, so the unformatted blob failed the required Static PR checks job on this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013mcqh46SDdiR5KxScihkFQ
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@lighthouse-budget.json`:
- Around line 57-69: Regenerate the mobile-documents-search and mobile-dsm
baseline records in lighthouse-budget.json from the complete Lighthouse report
set, preserving the generated metric ordering with fcpMs before lcpMs. Add a
focused validation check that rejects any baseline row where lcpMs is less than
fcpMs.
In `@tests/ui-smoke.spec.ts`:
- Around line 3558-3563: At tests/ui-smoke.spec.ts lines 3558-3563,
tests/ui-tools.spec.ts lines 1663-1665, and tests/ui-tools.spec.ts lines
1767-1769, replace the existing “Sort results” visibility assertions with
getByRole("group", { name: "Sort results", includeHidden: true }); assert the
group has count 1, then assert it is hidden.
🪄 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: 765cdf3b-8f30-42bc-839b-5bf7027cff37
📒 Files selected for processing (6)
docs/search-chrome-behaviour.mdlighthouse-budget.jsonsrc/components/clinical-dashboard/search-results-header-band.tsxtests/ui-accessibility.spec.tstests/ui-smoke.spec.tstests/ui-tools.spec.ts
… absent The three phone assertions claimed a guarantee they did not provide. `toBeHidden()` passes for a hidden node AND for a node that does not exist, and in ui-tools the plain `getByRole` locator filters hidden nodes out before the assertion sees them -- so both would have passed with ResultSortControl deleted from the component entirely, which is exactly the regression the comment says they catch. Resolve the group with `includeHidden: true`, assert `toHaveCount(1)`, then assert `toBeHidden()`. A deleted control now fails the count; a control returned to the phone line fails the visibility. Raised by CodeRabbit on #1689. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013mcqh46SDdiR5KxScihkFQ
Summary
sm. The band puts count, query, sort and Filter on one line; on a phone those two segments take roughly half of it, so the query truncates to pay for a control that is set about once a session and already defaults to the order a phone reader wants.ResultSortControlis nowhidden … sm:inline-flex, unchanged from 640px up.sm-and-up, not the ordering:?sort=alphastill carries an alpha order onto a phone from a link or a wider session, and the results honour it.className, becausecnin this repo is a plain string join with no Tailwind conflict resolution — a baseinline-flexplus a caller'shiddenwould have resolved by stylesheet order rather than by intent.hasPhoneUtilities: a page whose only utility is sort (forms) would otherwise keep the utilities group mounted on a phone as an empty flex child, and ininlineplacement below 414px that child isw-full basis-full— a blank second line under the count.ui-smokenow asserts the group is hidden at 390px and exercises the A–Z/Relevance interaction at the 1440px viewport the same test already resizes to; the twoui-toolsdifferentials phone tests assert hidden while keeping the 44px tap-height floor on the page filter.aria-label="Sort results". With sort hidden belowsmthat node has no box, so the sweep would have measured zero at every width in its own range and passed blindly; it now measures the last rendered child of the track.docs/search-chrome-behaviour.mdrule 3, which is the contract of record for this band.RAG impact: no retrieval behaviour change — presentation-only. No file under
src/lib/rag/**,result-sort.ts, ranking/selection, the eval harness or the golden fixture is touched;sortResultItemsand the default relevance order are unchanged, and the?sort=URL contract still applies at every width.Verification
npm run verify:pr-local—Test Files 1 failed | 519 passed (520)·Tests 1 failed | 5527 passed | 4 skipped (5532). The single failure istests/pr-handoff-stop.test.ts > emits handoff context only when the marker file exists, which is a session-hook environment artefact: it reproduces identically on the unmodified base commit (Test Files 1 failed (1)·Tests 1 failed | 10 passed (11)ate085154, with this diff checked out to nothing).npm run lint— clean,--max-warnings 0.npm run typecheck— clean.npm run test:focused -- --files src/components/clinical-dashboard/search-results-header-band.tsx—Test Files 9 passed (9)·Tests 117 passed (117).npm run verify:phone-chrome— lock-parity and runtime PASS; contractsTest Files 9 passed (9)·Tests 118 passed (118); changed-browser (tests/ui-smoke.spec.ts+tests/ui-tools.spec.ts, chromium)1 failed · 185 passed (5.8m). The one failure isdocument viewer puts the PDF preview first with pinned evidence after it on mobile, failing ongetByTestId('pdf-canvas-scroll').locator('canvas')never appearing — a PDF.js canvas that does not render in this container, unrelated to the results band. Baselined: the same test fails identically ate085154with this diff checked out to nothing. Every band journey passed, includingdocument search mode lists matching documents and result actions @criticaland both differentials narrow-viewport tests.UI verification not run:
npm run verify:uiwas not run as a separate gate.npm run verify:phone-chromeis this repo's selector for phone-chrome scope and it reported "Focused ownership and journey coverage is sufficient for this page-local or test-infrastructure scope. Full UI policy: auto (not selected)" — its changed-browser stage already ran the completeui-smoke+ui-toolschromium sets (186 tests), which is where every assertion about this band lives. Firefox/WebKit and physical Safari/installed-PWA acceptance remain uncovered, as always in a container.Risk and rollout
SearchResultsHeaderBand, and the change is a responsive display class plus one derived boolean — no state, data, route or ordering logic moves. The real user-facing cost is stated plainly rather than hidden: a phone can no longer switch to alphabetical from the bar. Relevance is the default so the common path is unaffected, and?sort=alphastill works, but if alphabetical needs to stay reachable on a phone the natural home is the existing phone filter sheet — deliberately left as a follow-up rather than smuggled into this diff.ResultSortControl.Notes
origin/main, and the band had been substantially rewritten in the interval (Results bar: one line, one filter, nothing unreachable #1615's one-line collapse). The first attempt was discarded and the change re-applied on currentmain.maincannotnpm cion Node 24.13.0:jsdom@30.0.1requires^22.22.2 || ^24.15.0 || >=26.0.0, whichengine-strictrejects. Node 24.15.0 was installed locally to get a lock-parity-clean install;package.jsonstill declares24.x, so this is an environment note, not a repo change.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation