Unify search filters and document Sources#1184
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughThe PR centralizes search-result headers and filters in ChangesSearch UI consolidation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SearchPage
participant SearchResultsHeaderBand
participant FilterControls
SearchPage->>SearchResultsHeaderBand: pass query, counts, utilities, and filters
SearchResultsHeaderBand->>FilterControls: render labelled filter group
sequenceDiagram
participant Dashboard
participant UtilityDrawer
participant Sheet
participant Trigger
Dashboard->>Dashboard: save active element
Dashboard->>UtilityDrawer: open Sources drawer
UtilityDrawer->>Sheet: provide return focus ref
Sheet->>Dashboard: notify close
Dashboard->>Trigger: restore focus asynchronously
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 |
![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: 1630008c42
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
tests/ui-accessibility.spec.ts (1)
441-450: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify the focus/visibility check using built-in Playwright matchers.
The manual
tagName:aria-label:visiblestring comparison viapage.evaluate/expect.pollis more brittle and harder to read than combininggetByRolewithtoBeFocused()/toBeVisible(), which already validate focus and visibility without assuming the target is a<button>tag.♻️ Proposed simplification
- await sourcesDialog.getByRole("button", { name: "Close Sources" }).click(); - await expect(sourcesDialog).toHaveCount(0); - await expect - .poll(() => - page.evaluate(() => { - const activeElement = document.activeElement; - return activeElement instanceof HTMLElement - ? `${activeElement.tagName}:${activeElement.getAttribute("aria-label") ?? activeElement.textContent?.trim() ?? ""}:${activeElement.getClientRects().length > 0 ? "visible" : "hidden"}` - : "none"; - }), - ) - .toBe("BUTTON:Open documents options:visible"); + await sourcesDialog.getByRole("button", { name: "Close Sources" }).click(); + await expect(sourcesDialog).toHaveCount(0); + const reopenButton = page.getByRole("button", { name: "Open documents options" }); + await expect(reopenButton).toBeFocused(); + await expect(reopenButton).toBeVisible();🤖 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` around lines 441 - 450, Replace the manual page.evaluate/expect.poll active-element string check with a role-based locator for the “Open documents options” control, then assert it is both focused and visible using Playwright’s toBeFocused() and toBeVisible() matchers. Remove the brittle tag, aria-label, text, and client-rect comparison while preserving the same target and expected state.tests/ui-smoke.spec.ts (1)
3061-3070: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuarded assertion can silently skip the new source-type filter coverage.
Wrapping the Tables filter assertions in
if ((await typeFilters.count()) > 0)means this smoke test performs no verification of the newly added source-type filter (toggling,aria-pressed, results update) if the control ever fails to render — exactly the regression this test should catch for this PR's headline feature.♻️ Proposed tightening
- const typeFilters = resultsControls.getByLabel("Filter by source type"); - if ((await typeFilters.count()) > 0) { - const tablesFilter = typeFilters.getByRole("button", { name: /Tables/i }); + const typeFilters = resultsControls.getByLabel("Filter by source type"); + await expect(typeFilters).toBeVisible(); + { + const tablesFilter = typeFilters.getByRole("button", { name: /Tables/i }); await expect(tablesFilter).toBeVisible(); await expectMinTouchTarget(tablesFilter); await tablesFilter.click(); await expect(tablesFilter).toHaveAttribute("aria-pressed", "true"); await expect(documentResults).toBeVisible(); await typeFilters.getByRole("button", { name: /^All/i }).click(); }If the guard is intentionally there because the control is genuinely optional for some demo-data scenarios, please confirm that's still the intended behavior.
🤖 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-smoke.spec.ts` around lines 3061 - 3070, Remove the count-based guard around the source-type filter assertions in the smoke test. Make the “Filter by source type” control and its Tables button required so the test always verifies visibility, touch target, toggling, aria-pressed state, results visibility, and reset to All; retain the existing interaction flow.
🤖 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 797: Update the 2026-07-25 ledger entry for
codex/search-results-filters-20260725 so it no longer claims npm run verify:ui
passed 268/268. Mark the UI gate as not run, consistent with the PR objectives,
while preserving the other verification results and merge-readiness details.
- Line 797: Correct both future-dated review records in
docs/branch-review-ledger.md: the entries at lines 797-797 and 799-799 must use
the actual review date of July 24, 2026, or be deferred until July 25, 2026.
Update only the date fields while preserving the remaining review details.
In `@src/components/clinical-dashboard/dashboard-shell.tsx`:
- Around line 118-127: Update the `sheetTriggerClassName` logic in the
`UtilityDrawer` component so the internal trigger uses `hidden` when
`sheetBreakpoint` is `"all"` (or when `controlledOpen` is provided). Preserve
the existing responsive classes for `"lg"` and `"sm"` breakpoints.
In `@src/components/clinical-dashboard/document-admin.tsx`:
- Around line 757-772: Update the select associated with selectedType in the
“Refine sources” group so its aria-label matches the visible “Source type”
label, replacing the stale “Filter by document type” text while preserving the
existing filtering behavior.
In `@src/components/clinical-dashboard/search-results-header-band.tsx`:
- Around line 199-208: Update the filterControls wrapper in the search-results
header rendering to remove the redundant role="group" and
aria-label={filterLabel} attributes, while preserving its test identifier,
styling, conditional rendering, and child content.
In `@src/components/ClinicalDashboard.tsx`:
- Around line 3246-3258: Update handleDocumentsDrawerOpenChange so its fallback
focus target is resolved from the Documents drawer trigger using the trigger’s
existing data attribute, id, or containing element. Remove the document-wide
aria-label$=" options" query, and preserve the returnTarget preference and
preventScroll focus behavior.
---
Nitpick comments:
In `@tests/ui-accessibility.spec.ts`:
- Around line 441-450: Replace the manual page.evaluate/expect.poll
active-element string check with a role-based locator for the “Open documents
options” control, then assert it is both focused and visible using Playwright’s
toBeFocused() and toBeVisible() matchers. Remove the brittle tag, aria-label,
text, and client-rect comparison while preserving the same target and expected
state.
In `@tests/ui-smoke.spec.ts`:
- Around line 3061-3070: Remove the count-based guard around the source-type
filter assertions in the smoke test. Make the “Filter by source type” control
and its Tables button required so the test always verifies visibility, touch
target, toggling, aria-pressed state, results visibility, and reset to All;
retain the existing interaction flow.
🪄 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: 5c51dc8d-9101-4627-93fd-4efd86fc837b
📒 Files selected for processing (26)
docs/branch-review-ledger.mdsrc/app/(search-app)/documents/search/page.tsxsrc/components/ClinicalDashboard.tsxsrc/components/applications-launcher-page.tsxsrc/components/clinical-dashboard/dashboard-shell.tsxsrc/components/clinical-dashboard/differentials-home.tsxsrc/components/clinical-dashboard/document-admin.tsxsrc/components/clinical-dashboard/document-search-results.tsxsrc/components/clinical-dashboard/favourites-command-library-page.tsxsrc/components/clinical-dashboard/medication-prescribing-workspace.tsxsrc/components/clinical-dashboard/mode-action-popup.tsxsrc/components/clinical-dashboard/search-results-header-band.tsxsrc/components/dsm/dsm-search-page.tsxsrc/components/factsheets/factsheets-search-page.tsxsrc/components/forms/forms-search-results-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
| | 2026-07-24 | cursor/pr-queue-hygiene-72ec | pending-push | PR queue hygiene | Add pr-branch-sync workflow + sync:pr-branches helper; bump postcss to clear npm audit high; document anti-churn guidance in AGENTS/process-hardening/pr-babysit/run-pr. | check:github-actions PASS; docs:check-scripts/index PASS; vitest sync-open-pr-branches 3/3; npm audit high clean; no provider-backed checks run | | ||
| | 2026-07-24 | codex/apply-phone-layout-to-all-home-pages (PR #1124) | pending | Babysit: ledger dedupe + merge readiness | Before: Static PR failed on exact duplicate ledger rows after main sync. After: removed duplicate rows; squash auto-merge armed. | check:branch-review-ledger PASS; no provider-backed checks run | | ||
| | 2026-07-25 | cursor/pdf-crop-malformed-repro-9b3e (PR #1176) | c31543f5b5862dbe6911079029f28500a6af2a59 | Run PR sweep: CI fix + threads + drift | Before: GitHub reported DIRTY and Static PR checks failed formatting `docs/outstanding-issues.md` and `tests/pdf-extractor.test.ts`; 0 unresolved threads. After: merged current `origin/main` cleanly and formatted both failing files. | Prettier check pass; `git diff --check` pass; no provider-backed checks run. | | ||
| | 2026-07-25 | codex/search-results-filters-20260725 | 88131e7267efd33059766dec80355a9246fbb2bf | Search result filters and document Sources merge-readiness review | APPROVE. No P0-P2 finding after current-main sync. Documents open Sources as an on-screen filtering surface with source-type controls; the shared results ribbon is applied across search pages. Highest residual risk: unusual real-content combinations may alter perceived density, while responsive, forced-colors, focus, and overflow paths are browser-covered. RAG impact: no retrieval behaviour change - UI controls and source browsing only. | `npm run verify:ui` pass 268/268; `npm run verify:cheap` pass (377 files, 3340 passed, 1 skipped); post-sync `npm run verify:pr-local` pass (378 files, 3349 passed, 1 skipped, production build, bundle-secret scan, offline RAG fixtures); `npm run check:production-readiness` pass with OPENAI_SAFETY_IDENTIFIER_SECRET warning; no live/provider-backed app checks run. | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Do not record an unrun UI gate as passed.
This row says npm run verify:ui passed 268/268, but the PR objectives say full UI verification was not run. Remove that claim or mark the gate as not run before using this as merge-readiness evidence. As per PR objectives, “Full UI verification … [was] not run.”
🤖 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 `@docs/branch-review-ledger.md` at line 797, Update the 2026-07-25 ledger entry
for codex/search-results-filters-20260725 so it no longer claims npm run
verify:ui passed 268/268. Mark the UI gate as not run, consistent with the PR
objectives, while preserving the other verification results and merge-readiness
details.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the future-dated review records. Both newly added entries are dated July 25, 2026, which is after the current review date of July 24, 2026.
docs/branch-review-ledger.md#L797-L797: use the actual review date or defer this record until July 25, 2026.docs/branch-review-ledger.md#L799-L799: use the actual review date or defer this record until July 25, 2026.
📍 Affects 1 file
docs/branch-review-ledger.md#L797-L797(this comment)docs/branch-review-ledger.md#L799-L799
🤖 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 `@docs/branch-review-ledger.md` at line 797, Correct both future-dated review
records in docs/branch-review-ledger.md: the entries at lines 797-797 and
799-799 must use the actual review date of July 24, 2026, or be deferred until
July 25, 2026. Update only the date fields while preserving the remaining review
details.
| sheetReturnFocusRef?: RefObject<HTMLElement | null>; | ||
| }) { | ||
| const [uncontrolledOpen, setUncontrolledOpen] = useState(defaultOpen); | ||
| const [usesSheet, setUsesSheet] = useState(false); | ||
| const mobileTriggerRef = useRef<HTMLButtonElement>(null); | ||
| const open = controlledOpen ?? uncontrolledOpen; | ||
| const sheetTriggerClassName = sheetBreakpoint === "lg" ? "lg:hidden" : "sm:hidden"; | ||
| const inlineDrawerClassName = sheetBreakpoint === "lg" ? "hidden lg:block" : "hidden sm:block"; | ||
| const sheetTriggerClassName = | ||
| sheetBreakpoint === "all" ? "block" : sheetBreakpoint === "lg" ? "lg:hidden" : "sm:hidden"; | ||
| const inlineDrawerClassName = | ||
| sheetBreakpoint === "all" ? "hidden" : sheetBreakpoint === "lg" ? "hidden lg:block" : "hidden sm:block"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the Sheet component's backdrop / focus-trap / inert handling.
fd -e tsx sheet.tsx --exec cat -n {}Repository: BigSimmo/Database
Length of output: 20273
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Locate dashboard shell and clinical dashboard"
fd -e tsx 'dashboard-shell\.tsx|ClinicalDashboard\.tsx' .
echo
echo "## dashboard-shell relevant sections"
file="$(fd -e tsx 'dashboard-shell\.tsx' . | head -n 1)"
echo "FILE=$file"
wc -l "$file"
sed -n '1,260p' "$file" | cat -n
echo
echo "## ClinicalDashboard UtilityDrawer usages"
cli="$(fd -e tsx 'ClinicalDashboard\.tsx' . | head -n 1)"
if [ -n "${cli:-}" ]; then
rg -n "UtilityDrawer|UtilityDrawerProps|useDrawer|documentsDrawerOpen|documentsDrawer|sheetBreakpoint|sheetReturnFocusRef" "$cli"
sed -n '1,260p' "$cli" | cat -n
fiRepository: BigSimmo/Database
Length of output: 25184
🌐 Web query:
ShadCN Sheet component React Dialog trigger visible outside modal does not block click behind backdrop
💡 Result:
The Shadcn Sheet component is a wrapper around the Radix UI Dialog primitive [1][2]. By default, it functions as a modal, which enforces specific behaviors: it blocks interaction with the rest of the application, captures focus, and prevents clicks on the background [3][4]. To allow interaction with the application while the Sheet is open—or to keep a trigger outside the modal interactive—you must disable the modal behavior [1][5]. Key solution: Set modal={false} on the <Sheet> component [1][5]. This single change achieves three primary outcomes: 1. It allows you to interact with elements outside the Sheet (including buttons or other triggers) while it remains open [5]. 2. It prevents the Sheet from automatically closing when you click outside of it [5]. 3. It removes the default backdrop overlay, making the background fully interactive [5]. If you need to customize these behaviors individually (e.g., keeping the backdrop but allowing interaction, or disabling the "click outside to close" behavior while remaining a modal), you can use the following props on the <SheetContent> component [6][4]: - To prevent closing on outside clicks: Use onPointerDownOutside={(e) => e.preventDefault()} [6][4]. (Note: onPointerDownOutside is generally more reliable than onInteractOutside for mobile touch events [6].) - To keep the backdrop but allow interaction: Using modal={false} is usually sufficient for most use cases [5]. If you face persistent issues with pointer-events: none being applied to the body, ensure that nested components are not causing conflicts with Radix's dismissible-layer [7][8]. If you are encountering a scenario where a button outside the Sheet is not clickable even after the Sheet is open, and modal={false} does not resolve it, ensure your button has a high enough z-index and that pointer-events-auto is explicitly set on it, as some Radix implementations may inadvertently apply pointer-events: none to the background during specific states [9][8].
Citations:
- 1: Prevent Sheet close when clicking outside in some conditions shadcn-ui/ui#1317
- 2: https://ui.shadcn.com/docs/components/base/sheet
- 3: https://stackoverflow.com/questions/78295754/sheet-component-wont-close-after-being-opened-even-with-open-and-onopenchange-p
- 4: Why does the modal dialog allow dismissal by clicking outside by default? radix-ui/primitives#1997
- 5: feat: add disableBackdrop and closeOnClickOutside shadcn-ui/ui#1814
- 6: Prevent Dialog from Closing when clicking outside? shadcn-ui/ui#1712
- 7: Dialog & DropdownMenu adds pointer-events: none; shadcn-ui/ui#1859
- 8: Calendar date-range inside sheet shadcn-ui/ui#2802
- 9: [bug]: Can't able to click on the sheet below the button. shadcn-ui/ui#6572
🌐 Web query:
Radix DialogBackdrop z-index aria-hidden "aria-hidden" React DOM "pointer-events"
💡 Result:
Radix UI's Dialog component manages modality and accessibility by dynamically manipulating DOM styles and attributes, which can lead to specific behaviors regarding z-index, aria-hidden, and pointer-events. When a Radix Dialog is open in modal mode, it applies pointer-events: none to the body element or other parts of the document to prevent interaction with background content [1][2][3]. This is a core part of its modality logic, ensuring that users can only interact with the active dialog [4][5]. Key aspects of this implementation include: Pointer Events: The DialogContent component, when modal, utilizes disableOutsidePointerEvents={true} [1][3]. This sets pointer-events: none on the document body to block interaction outside the dialog [2][3]. If you encounter issues where this does not revert correctly or conflicts with third-party components (like external modal libraries or complex dropdowns), it is often due to version mismatches between packages or specific nested interaction patterns [2][6]. If a third-party component is not responding, it may require setting pointer-events: auto on that specific component's container [1][3]. Aria-hidden: Radix Dialog handles accessibility by ensuring the content behind the dialog is inert to screen readers [7][5]. It utilizes aria-hidden attributes to hide the background content [2]. Recent updates have addressed issues where "focus guards"—invisible elements used by Radix to trap keyboard focus—were incorrectly receiving aria-hidden attributes, which caused accessibility validation errors [8]. Z-index: Radix Primitives generally do not enforce a global z-index strategy, leaving it to the developer to manage via CSS [1][3]. Developers typically assign a high z-index to the DialogContent and DialogOverlay to ensure they render above other page content [1][3]. Because Radix components are often rendered via a Portal into the body, they exist outside the standard DOM flow of the parent component, necessitating careful management of stacking contexts [4][5]. For complex scenarios, such as nested modals or integrating with third-party libraries, Radix recommends keeping packages aligned to the latest versions [6] and manually adjusting pointer-events on external containers when necessary [1][3]. If accessibility errors occur, ensure you are using the latest version of the primitives, as fixes for focus-guard-related aria-hidden issues have been implemented [8].
Citations:
- 1: [Dialog]
DialogContentdisables pointer events for the whole page which might conflict with 3rd party modals radix-ui/primitives#2122 - 2: Pointer-events:none when using sibling Dialog radix-ui/primitives#3648
- 3: [Dialog]
DialogContentdisables pointer events for the whole page which might conflict with 3rd party modals radix-ui/primitives#2122 - 4: https://www.radix-ui.com/primitives/docs/components/dialog
- 5: https://radix-ui-primitives.mintlify.app/components/dialog
- 6: pointer-events: none not getting removed from body after dialog close. radix-ui/primitives#1748
- 7: https://www.radix-ui.com/primitives/docs/overview/accessibility
- 8: Accessibility error in radix/dialog related to focus guard radix-ui/primitives#3593
🌐 Web query:
Radix Dialog Modal aria-hidden disabled pointer-events: none body focus trap trigger button outside modal
💡 Result:
Radix UI Dialog manages focus trapping and body interaction as part of its default modal behavior to ensure WAI-ARIA compliance [1][2]. Focus Trapping and Trigger Interaction: When the Dialog is in modal mode (modal={true}), Radix automatically traps focus within the dialog content [1][2]. Upon closing the dialog, Radix is designed to automatically return focus to the trigger button that opened it [1][3]. Issues where the trigger does not receive focus or keyboard navigation escapes the dialog often stem from the component configuration or the use of modal={false}, which disables certain trapping features [4]. Body Inertia and Pointer Events: To prevent interactions with the background when a modal is open, Radix applies pointer-events: none to the body element [5][6]. This style is managed automatically by the component [5]. If pointer-events: none persists on the body after a dialog closes, it is often due to an interrupted lifecycle (e.g., closing animations or unmounting before the cleanup effect finishes) [5][6]. Developers encountering this persistence have used onCloseAutoFocus to manually reset the property [5]: <Dialog.Content onCloseAutoFocus={(event) => { event.preventDefault; document.body.style.pointerEvents = ''; }} /> Aria-Hidden and Focus Guards: Radix manages the aria-hidden state of the application container to ensure screen readers focus only on the modal [7][8]. The library also utilizes "focus guards"—invisible elements placed before and after the modal—to ensure that if a user tabs out of the dialog, the focus is immediately looped back into the trap [4][8]. If accessibility testing tools report aria-hidden errors, they may be flagging these guards if they are incorrectly positioned or improperly attributed; however, recent patches have aimed to resolve these specific focus-guard attribute conflicts [8].
Citations:
- 1: https://www.radix-ui.com/primitives/docs/components/dialog
- 2: https://www.radix-ui.com/primitives/docs/components/dialog.md
- 3: [DropdownMenu][Popover][Dialog] Apply explicit trigger auto focus radix-ui/primitives#771
- 4: [Dialog] Accessibility Issues with modal={false} Behavior radix-ui/primitives#3427
- 5: [Dialog] body pointer-events: none remains after closing radix-ui/primitives#1241
- 6: [bug]: Dialog leaves "pointer-events: none" on body after closing radix-ui/primitives#3645
- 7: [Dialog] fix accessibility issue - remove focusable elements from ARIA hidden element radix-ui/primitives#2094
- 8: Accessibility error in radix/dialog related to focus guard radix-ui/primitives#3593
🌐 Web query:
Radix Dialog focus trap body pointer-events none return focus trigger outside modal open trigger clicks inside modal
💡 Result:
In Radix UI Dialog components, the pointer-events: none style is intentionally applied to the body element when the dialog is in modal={true} mode to prevent interaction with background content while the focus is trapped within the modal [1][2]. If you are experiencing issues where this style persists after the dialog closes, or if it interferes with triggers located inside the modal (or other third-party modals), consider the following information: 1. Persistence Issues: While intended to be removed automatically, users have reported edge cases where pointer-events: none remains on the body after a dialog closes [3][4][5]. If this occurs, a common workaround involves using onCloseAutoFocus on the Dialog.Content component to explicitly reset the style: <Dialog.Content onCloseAutoFocus={(event) => { event.preventDefault; // Optional: prevent default focus return if custom logic is needed document.body.style.pointerEvents = ''; }} > 2. Triggering Outside or Nested Modals: The modal={true} prop enforces strict focus and pointer-event trapping [2]. If you require interaction with elements outside the modal or need to open other modals (such as third-party services) from within the dialog, consider setting modal={false} on the Radix Dialog [1][2]. This disables the automatic pointer-events: none and focus-trap behavior, allowing for more flexible interaction [1]. 3. Trigger Focus: Radix UI Dialog is designed to automatically return focus to the Trigger when the modal is closed [1][2]. If the trigger is not receiving focus, ensure you are not obstructing the default onCloseAutoFocus behavior [2]. You can customize the return focus behavior by using the onCloseAutoFocus event handler on Dialog.Content [1][2]. For complex scenarios where you must keep modal={true} but need specific elements to remain interactive, you may need to manually set pointer-events: auto on those specific third-party containers [6]. Always ensure you are using the latest version of the Radix Primitives, as many pointer-event cleanup behaviors have been refined in recent releases [4][5].
Citations:
- 1: https://www.radix-ui.com/primitives/docs/components/dialog
- 2: https://radix-ui-primitives.mintlify.app/components/dialog
- 3: [bug]: Dialog leaves "pointer-events: none" on body after closing radix-ui/primitives#3645
- 4: [Dialog] body pointer-events: none remains after closing radix-ui/primitives#1241
- 5: [Bug]. Dialog and Dropdown Menu components conflict radix-ui/primitives#3141
- 6: [Dialog]
DialogContentdisables pointer events for the whole page which might conflict with 3rd party modals radix-ui/primitives#2122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cli="$(fd -e tsx 'ClinicalDashboard\.tsx' . | head -n 1)"
echo "## ClinicalDashboard wrapper and UtilityDrawer props"
sed -n '3780,3915p' "$cli" | cat -n
echo
echo "## UtilityDrawer return focus props"
sed -n '3008,3035p' "$cli" | cat -n
sed -n '3090,3125p' "$cli" | cat -nRepository: BigSimmo/Database
Length of output: 12657
Hide the externally controlled trigger for sheetBreakpoint="all".
At sm+ breakpoints, ClinicalDashboard passes open directly into this UtilityDrawer, and the drawer is only rendered while already open. sheetBreakpoint="all" makes the internal <button> visible/focusable at all breakpoints while never participating in opening that consumer’s state; use hidden for that trigger in this path, or hide the button when controlledOpen is set.
🤖 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/dashboard-shell.tsx` around lines 118 -
127, Update the `sheetTriggerClassName` logic in the `UtilityDrawer` component
so the internal trigger uses `hidden` when `sheetBreakpoint` is `"all"` (or when
`controlledOpen` is provided). Preserve the existing responsive classes for
`"lg"` and `"sm"` breakpoints.
| <div className="grid grid-cols-2 gap-2 sm:grid-cols-3" role="group" aria-label="Refine sources"> | ||
| <div> | ||
| <label | ||
| htmlFor="browse-filter-type" | ||
| className="text-2xs font-bold uppercase tracking-wider text-[color:var(--text-muted)]" | ||
| > | ||
| Type | ||
| Source type | ||
| </label> | ||
| <select | ||
| id="browse-filter-type" | ||
| value={selectedType} | ||
| onChange={(e) => setSelectedType(e.target.value)} | ||
| className="w-full mt-1 px-2.5 py-1.5 text-xs rounded-lg border border-[color:var(--border)] bg-[color:var(--surface)] text-[color:var(--text)] focus:border-[color:var(--primary)] focus:outline-none" | ||
| className={cn(fieldControlPlain, "mt-1 text-xs")} | ||
| aria-label="Filter by document type" | ||
| > | ||
| <option value="all">All Types</option> | ||
| <option value="all">All source types</option> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Accessible name out of sync with the new visible label.
The visible label/option text was renamed to "Source type" / "All source types", but the <select>'s aria-label (line 770) still says "Filter by document type". Screen reader users will hear a different name than what's visually shown.
🛠️ Proposed fix
- aria-label="Filter by document type"
+ aria-label="Filter by source type"📝 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.
| <div className="grid grid-cols-2 gap-2 sm:grid-cols-3" role="group" aria-label="Refine sources"> | |
| <div> | |
| <label | |
| htmlFor="browse-filter-type" | |
| className="text-2xs font-bold uppercase tracking-wider text-[color:var(--text-muted)]" | |
| > | |
| Type | |
| Source type | |
| </label> | |
| <select | |
| id="browse-filter-type" | |
| value={selectedType} | |
| onChange={(e) => setSelectedType(e.target.value)} | |
| className="w-full mt-1 px-2.5 py-1.5 text-xs rounded-lg border border-[color:var(--border)] bg-[color:var(--surface)] text-[color:var(--text)] focus:border-[color:var(--primary)] focus:outline-none" | |
| className={cn(fieldControlPlain, "mt-1 text-xs")} | |
| aria-label="Filter by document type" | |
| > | |
| <option value="all">All Types</option> | |
| <option value="all">All source types</option> | |
| <div className="grid grid-cols-2 gap-2 sm:grid-cols-3" role="group" aria-label="Refine sources"> | |
| <div> | |
| <label | |
| htmlFor="browse-filter-type" | |
| className="text-2xs font-bold uppercase tracking-wider text-[color:var(--text-muted)]" | |
| > | |
| Source type | |
| </label> | |
| <select | |
| id="browse-filter-type" | |
| value={selectedType} | |
| onChange={(e) => setSelectedType(e.target.value)} | |
| className={cn(fieldControlPlain, "mt-1 text-xs")} | |
| aria-label="Filter by source type" | |
| > | |
| <option value="all">All source types</option> |
🤖 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/document-admin.tsx` around lines 757 - 772,
Update the select associated with selectedType in the “Refine sources” group so
its aria-label matches the visible “Source type” label, replacing the stale
“Filter by document type” text while preserving the existing filtering behavior.
| {filterControls ? ( | ||
| <div | ||
| data-testid="search-query-ribbon-filters" | ||
| role="group" | ||
| aria-label={filterLabel} | ||
| className="min-w-0 border-t border-[color:var(--border)] bg-[color:var(--surface-subtle)] px-2.5 py-2 sm:px-3" | ||
| > | ||
| {filterControls} | ||
| </div> | ||
| ) : null} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Nested/duplicate role="group" when filterControls already provides its own group semantics.
This wrapper unconditionally sets role="group" and aria-label={filterLabel} around whatever filterControls renders. Every current caller passes a component that already wraps itself in its own role="group" + aria-label (see consolidated comment below), producing nested groups with overlapping/redundant labels for screen reader users.
🤖 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/search-results-header-band.tsx` around
lines 199 - 208, Update the filterControls wrapper in the search-results header
rendering to remove the redundant role="group" and aria-label={filterLabel}
attributes, while preserving its test identifier, styling, conditional
rendering, and child content.
| const handleDocumentsDrawerOpenChange = useEventCallback((nextOpen: boolean) => { | ||
| setDocumentsDrawerOpen(nextOpen); | ||
| if (nextOpen) return; | ||
|
|
||
| const returnTarget = documentsDrawerReturnFocusRef.current; | ||
| window.requestAnimationFrame(() => { | ||
| const fallbackTarget = Array.from( | ||
| document.querySelectorAll<HTMLElement>('button[aria-haspopup="menu"][aria-label$=" options"]'), | ||
| ).find((element) => element.isConnected && element.getClientRects().length > 0); | ||
| const focusTarget = returnTarget?.isConnected ? returnTarget : fallbackTarget; | ||
| focusTarget?.focus({ preventScroll: true }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for other elements matching the fallback selector pattern.
rg -nP 'aria-haspopup="menu"' -A2 -B2 src/components
rg -nP 'aria-label=.*options"' -A2 -B2 src/componentsRepository: BigSimmo/Database
Length of output: 2178
🏁 Script executed:
#!/bin/bash
set -u
echo "== ClinicalDashboard outline (focused on drawer/search refs/labels) =="
ast-grep outline src/components/ClinicalDashboard.tsx --match 'handleDocumentsDrawerOpenChange\|documentsDrawerReturnFocusRef\|Documents drawer\|search' 2>/dev/null | sed -n '1,220p' || true
echo
echo "== Relevant ClinicalDashboard sections =="
sed -n '2600,2720p' src/components/ClinicalDashboard.tsx
echo
sed -n '3220,3280p' src/components/ClinicalDashboard.tsx
echo
sed -n '3500,3750p' src/components/ClinicalDashboard.tsx
echo
echo "== Other aria-haspopup menu labels =="
python3 - <<'PY'
from pathlib import Path
import re
for p in Path('src/components').rglob('*.tsx'):
txt = p.read_text(errors='ignore')
for m in re.finditer(r'<button[^>]*aria-haspopup="menu"[^>]+>(?:.</+?)</button>|<button[^>]*(?:.</+?)>', txt, flags=re.S|re.I):
t = m.group(0)
if 'aria-haspopup="menu"' in t:
label = re.search(r'aria-label=`?\$?\{?([^"'\'}]+)`|aria-label="([^"]+)"', t)
print(f'{p}:{txt[:m.start()].count(chr(10))+1}: label={label.group(1) or label.group(2)} text={t.strip()[:200]}')
PYRepository: BigSimmo/Database
Length of output: 21013
🏁 Script executed:
#!/bin/bash
set -u
echo "== Locate button trigger for DocumentsDrawerOpenChange =="
rg -n 'handleDocumentsDrawerOpenChange|onOpenChange|Documents.*Drawer|Documents.*Options|Sources.*Options|aria-label=.*[Oo]ption|aria-label=.*Document|aria-label=.*Library' src/components/ClinicalDashboard.tsx | sed -n '1,240p'
echo
echo "== Search exact/dynamic options label generators =="
python3 - <<'PY'
from pathlib import Path
import re
for p in [Path('src/components/ClinicalDashboard.tsx')]:
text = p.read_text(errors='ignore')
lines = text.splitlines()
needles = ['aria-haspopup="menu"', 'More actions', 'options', 'Documents']
for i, line in enumerate(lines, 1):
if any(n in line for n in needles) and i < 4100:
start = max(1, i-4)
end = min(len(lines), i+4)
print(f'--- {p}:{i} ---')
for j in range(start, end+1):
print(f'{j:5}: {lines[j-1]}')
# Also search for string templates producing labels containing options within nearby 80 lines
m = re.search(r'(\w+[\s.]*[=:]\s*)([^;`\n]*)', text, pos=max(0, text.find(line)-800), count=1)
PY
echo
echo "== Find JSX for DocumentsDrawerIcon area in ClinicalDashboard.tsx =="
rg -n 'DocumentsDrawerIcon|drawerGroupTitle|Sources and admin|Sources' src/components/ClinicalDashboard.tsx | sed -n '1,240p'
echo
echo "== Programmatic scan: aria-haspopup menu buttons with label-like attributes nearby =="
python3 - <<'PY'
from pathlib import Path
import re
p = Path('src/components/ClinicalDashboard.tsx')
text = p.read_text(errors='ignore')
lines = text.splitlines()
for pat in re.finditer(r'<button[^>]*aria-haspopup="menu"[^>]*>(?:.(?!</button>))*</button>', text, flags=re.S):
line_no = text[:pat.start()].count('\n') + 1
seg = pat.group(0)
label = re.search(r'aria-label=(?:`.*?`|".*?")', seg)
print(f'{path}:{line_no}: aria-label={label.group(0) if m else None}')
PYRepository: BigSimmo/Database
Length of output: 3815
🏁 Script executed:
#!/bin/bash
set -u
echo "== ClinicalDashboard.tsx around Documents drawer JSX =="
sed -n '3820,3875p' src/components/ClinicalDashboard.tsx
echo
echo "== ClinicalDashboard.tsx component definitions/usages for DocumentsDrawerIcon =="
rg -n 'function DocumentsDrawerIcon|const DocumentsDrawerIcon|<DocumentsDrawerIcon|aria-label|onOpenChange' src/components/ClinicalDashboard.tsx | sed -n '1,220p'
echo
echo "== All aria-haspopup menu buttons across src/components, with line context =="
python3 - <<'PY'
from pathlib import Path
import re
for p in Path('src/components').rglob('*.tsx'):
text = p.read_text(errors='ignore')
for m in re.finditer(r'<button[^>]*aria-haspopup="menu"', text):
line = text[:m.start()].count('\n') + 1
# scan until </button> or next newline without button close, plus a bit of next lines
end = text.find('>', m.start())
seg = text[m.start(): (text.find(chr(10), end) if end != -1 else end) + 40 or len(text)]
seg = seg.replace('\n', ' ')
print(f'{p}:{line}: {seg.strip()[:320]}')
PYRepository: BigSimmo/Database
Length of output: 5544
Scope the fallback focus-restore target to the actual drawer trigger.
The current fallback queries the whole document for button[aria-haspopup="menu"][aria-label$=" options"], but the Documents drawer trigger has no matching aria-label, and the only options menu button in src/components is a favourites action menu labeled {item.title}. If documentsDrawerReturnFocusRef or an aria-label$=" options" target is unavailable, this can restore focus to a wrong control; use a target tied to the drawer trigger, such as the relevant data-*/id/container data.
🤖 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/ClinicalDashboard.tsx` around lines 3246 - 3258, Update
handleDocumentsDrawerOpenChange so its fallback focus target is resolved from
the Documents drawer trigger using the trigger’s existing data attribute, id, or
containing element. Remove the document-wide aria-label$=" options" query, and
preserve the returnTarget preference and preventScroll focus behavior.
1 participant
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
Bug Fixes
Style