Documents: present the filter panel as a sheet - #1542
Conversation
On a phone the in-flow panel pushed the results down as it opened, so the count it reports scrolled out of view exactly while you were tapping the controls that change it. `ui/sheet.tsx` already solves this: bottom sheet with a drag grip below `sm`, centred dialog from `sm` up, focus trap and Escape included. The panel body is unchanged. The header row becomes the sheet title plus `headerActions` (Clear all), and the count and "Show N documents" become the sheet footer, which keeps them pinned above the fold rather than at the end of a long facet list. The trigger is now a dialog opener: `aria-haspopup="dialog"` with `aria-expanded`, and no `aria-controls`, since the sheet owns its own id. No `returnFocusRef` either — `Sheet` falls back to the previously focused element, which is the correct behaviour with two trigger copies in the DOM. Mounted unconditionally rather than behind `filterPanelOpen`: `Sheet` returns null while closed and owns its own transition, so gating the mount would cut the dismiss animation off mid-flight. Verified: `npm run verify:cheap` exit 0, 450 files / 4712 tests passed. `verify:phone-chrome` — lock-parity, runtime and contract stages green; `changed-browser` 93 passed / 1 failed on the pre-existing document-viewer PDF canvas test, which fails identically with these changes stashed under Chromium 1194 against the pinned 1228; the `focused-browser` stage it aborted before was run directly, 7 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughThe document filter panel now uses the shared ChangesDocument filter sheet
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant FilterTrigger
participant DocumentFilterPanel
participant Sheet
FilterTrigger->>DocumentFilterPanel: Open filter panel
DocumentFilterPanel->>Sheet: Render controlled Sheet
Sheet-->>DocumentFilterPanel: Close on Escape or completion
DocumentFilterPanel-->>FilterTrigger: Update dialog attributes
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
🔒 Upgrade RequiredPrivate repository analysis requires Pro or Enterprise. Upgrade: https://ecc.tools/pricing?plan=pro ECC Tools keeps the core app open, and puts private repos, team features, and enterprise controls behind paid tiers. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
🔒 Upgrade RequiredPrivate repository analysis requires Pro or Enterprise. Upgrade: https://ecc.tools/pricing?plan=pro ECC Tools keeps the core app open, and puts private repos, team features, and enterprise controls behind paid tiers. |
🔒 Upgrade RequiredPrivate repository analysis requires Pro or Enterprise. Upgrade: https://ecc.tools/pricing?plan=pro ECC Tools keeps the core app open, and puts private repos, team features, and enterprise controls behind paid tiers. |
🔒 Upgrade RequiredPrivate repository analysis requires Pro or Enterprise. Upgrade: https://ecc.tools/pricing?plan=pro ECC Tools keeps the core app open, and puts private repos, team features, and enterprise controls behind paid tiers. |
🔒 Upgrade RequiredPrivate repository analysis requires Pro or Enterprise. Upgrade: https://ecc.tools/pricing?plan=pro ECC Tools keeps the core app open, and puts private repos, team features, and enterprise controls behind paid tiers. |
|
@cursoragent Unblock the current open PR. Confirm the PR number and GitHub head first from context. If more than one open PR could apply, stop and say which one you would use and why. Fetch and start from the remote tip that matches that GitHub head. If the named branch ref is missing or stale, use the PR head ref. Preserve unrelated local WIP; do not discard dirty work; do not treat a local-ahead commit as the reviewed tip. Do not merge the PR, force-push, rebase, or arm auto-merge. No provider-backed gates without approval. If you cannot push or resolve threads, diagnose and comment only; if inline replies fail, resolve when possible and put blocker dispositions in the summary comment. If auto-merge is already armed, push only for a real blocker and avoid cancelling in-flight required CI unless the push clears that blocker. If the PR is already merged or closed: confirm outcome, post one summary, and stop. Primary job: clear mergeability and required-CI blockers on this tip. Do a full product review only if needed to explain or fix a blocker. Stay light otherwise—no broad verification suites or Playwright unless needed to prove one concrete unblock fix. Snapshot the GitHub head: tip, behind/ahead, mergeable state, merge-tree versus origin/main (real conflict versus behind-but-clean), required checks on that tip including Production UI when selected, and unresolved threads that block merge or required CI. Missing checks while dirty are not green. Tip moved → re-snapshot. Ledger-lookup against that GitHub head under an unblock/fix scope for this PR. If already handled at this head for unblock work, merge-tree clean, required checks green, and no blocking threads: summarize, comment, stop. Unblock once: real conflict → merge origin/main with the smallest correct resolution (prefer main’s shared queues; keep this PR’s notes); behind-but-clean → one sync, late if required CI is in flight; clean → leave. skip-branch-sync and similar labels do not block a manual sync needed to clear a real blocker. No sync thrash. After any sync or push, re-snapshot tip, merge-tree, and required checks before declaring done. Dedupe the ledger if a merge touched it. Fix only what blocks required checks or mergeability on this tip. Ignore advisory red unless it reveals a real required failure. Prefer the smallest safe fix; if a bot/autofix commit caused the break, prefer revert. Prove with the smallest targeted check only. Format and commit before push. Push only blocker-fix commits. Append ledger for the final GitHub head under the unblock scope. Never push a ledger-only tip. One PR comment: tip, sync/merge-tree, what blocked and what you fixed or dispositioned, required CI status, residual risks; merge left to me. Stop when merge-tree is clean on the current GitHub tip, no blocking threads remain for merge/required CI, and required checks are green or clearly in progress. |
|
PR #1542 unblocked (the dirty one; #1546/#1547 are unrelated behind-but-mergeable). GitHub said Now:   |
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
🔒 Upgrade RequiredPrivate repository analysis requires Pro or Enterprise. Upgrade: https://ecc.tools/pricing?plan=pro ECC Tools keeps the core app open, and puts private repos, team features, and enterprise controls behind paid tiers. |
Unblock summary — PR #1542Confirmed: open PR for this request is #1542 (
Required CI (tip
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/clinical-dashboard/document-search-results.tsx`:
- Around line 240-250: Update the footer action button in the document search
results component to use the 48px desktop tap target by replacing its
sm:min-h-10 class with sm:min-h-12, while preserving the existing min-h-tap and
other styling.
- Around line 1282-1288: Ensure DocumentFilterPanel remains mounted long enough
to receive open={false} and complete the Sheet close transition when loading or
query changes make showFilterControl false, or explicitly invoke the existing
onDone handler to close filterPanelState before unmounting. Update the relevant
mounting/close logic near DocumentFilterPanel and add a regression test covering
an open panel followed by a loading transition or new query, verifying focus
returns to the opener.
In `@tests/document-filter-panel.dom.test.tsx`:
- Around line 169-180: Expand the dialog interaction test around
DocumentSearchResultsPanel to assert the opened trigger has aria-expanded="true"
and an aria-controls value matching the rendered dialog’s id, while preserving
the existing role and aria-haspopup checks. After closing via Escape, assert
aria-expanded="false" and that aria-controls is removed from the trigger.
🪄 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
Run ID: dc2cba3b-6bf1-46f2-bfdd-e81805c886f2
📒 Files selected for processing (3)
docs/branch-review-ledger.mdsrc/components/clinical-dashboard/document-search-results.tsxtests/document-filter-panel.dom.test.tsx
3 participants
Summary
ui/sheet.tsx: bottom sheet with a drag grip belowsm, centred dialog fromsmup, focus trap and Escape included.headerActions(Clear all), and the count and "Show N documents" become the sheet footer, which keeps them pinned rather than stranded at the end of a long facet list.aria-haspopup="dialog"plusaria-expanded, andaria-controlspointing at the dialog via theidpropSheetgained onmainin feat(design-system): v2 token layer + 26 components — reconciled, cascade-ported, forced-colours owned #1538. NoreturnFocusRef—Sheetfalls back to the previously focused element, which is the correct behaviour given two trigger copies (-phone/-wide) sit in the DOM and only one is ever displayed.Sheetreturnsnullwhile closed and owns its own transition, so gating the mount would cut the dismiss animation off mid-flight. The query-scopedfilterPanelStateadded onmainis kept as-is, so a new search still cannot leave the sheet covering a different result set.Verification
npm run verify:cheap— exit 0,Test Files 457 passed (457),Tests 4780 passed | 4 skipped (4784)tests/document-filter-panel.dom.test.tsx— 9 passed, including two new cases for this change: the panel resolves torole="dialog"(it overlays the results rather than displacing them) and Escape dismisses it.ui-smoke.spec.tsdocument search mode lists matching documents and result actions @critical— 1 passed, Chromium. The existing phone and wide assertions from Documents: one reachable filter panel, and a library control named after what it opens #1536 pass unchanged through the sheet.npm run verify:phone-chromeon the pre-rebase commit: lock-parity, runtime and contract stages green;changed-browser93 passed / 1 failed; thefocused-browserstage the gate aborted before was then run directly — 7 passed.changed-browserfailure isdocument viewer puts the PDF preview first with pinned evidence after it on mobile, atpdfScroller.locator("canvas"). It fails identically with these changes stashed, so it is pre-existing and environmental: this box has Chromium 1194 against the project's pinned 1228, run via thePLAYWRIGHT_CHROMIUM_EXECUTABLE_PATHhook the config already supports.npm run verify:pr-local— not run; the gates above are the evidence.eval:*,check:production-readiness,check:supabase-project— not run and not applicable. No retrieval, ranking, ingestion, or provider surface is touched.RAG impact: no retrieval behaviour change — this changes only how an already-built client-side filter panel is presented. No file under
src/lib/rag/**, clinical-search, retrieval-selection, ranking-config, answer-ranking, the eval harness, or the golden fixture is touched, and no retrieval RPC or comparator ordering is altered.Risk and rollout
Clinical Governance Preflight
classifyPullRequestFilesreturnsclinicalRisk: false,operationalRisk: false,ragRanking: false,ui: truefor this file set — the diff issrc/components/clinical-dashboard/document-search-results.tsxand its DOM test only, and does not touchsrc/lib/document-tags.ts(which is what tripped the classifier on #1536). No ingestion, answer generation, search/ranking, source rendering, document access, privacy, or production-environment path is involved.Notes
origin/mainand this commit cherry-picked on top rather than stacked on the merged history. The one conflict was the panel's open-flag state, resolved in favour ofmain's query-scoped version.Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests