Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRemoves a portalTarget prop and menu portal styling from AsyncMultiSelect, changes ModelMultiselect defaultOptions handling, adds portal-aware outside-interaction handlers to SheetContent, adds version/changelog bumps across packages, and introduces explicit transport-version validation in npx/bin.js. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AsyncMultiSelect
participant MenuPortal as "Menu (portaled)"
participant SheetContent
User->>AsyncMultiSelect: focus / type
AsyncMultiSelect->>MenuPortal: render menu into portal (document.body)
Note right of MenuPortal: Menu is rendered outside component tree
User->>MenuPortal: pointerdown on menu item
MenuPortal->>AsyncMultiSelect: pointerdown event (bubbled)
AsyncMultiSelect->>AsyncMultiSelect: set isInteractingWithMenuRef = true
User->>MenuPortal: pointerup on menu item
MenuPortal->>AsyncMultiSelect: pointerup event
AsyncMultiSelect->>AsyncMultiSelect: schedule flag reset (short timeout)
User->>AsyncMultiSelect: blur event
alt isInteractingWithMenuRef == true
AsyncMultiSelect->>AsyncMultiSelect: ignore blur (keep menu open)
else
AsyncMultiSelect->>AsyncMultiSelect: handle blur (close menu)
end
User->>SheetContent: pointerdown outside (target = portaled element)
SheetContent->>SheetContent: isPortaledElement? → true
SheetContent-->>User: prevent default / skip outside-close
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (28)
Comment |
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
ui/components/ui/sheet.tsx (2)
81-97: Type casting may be fragile; consider explicit typing.The handlers cast events to
CustomEventto access.detail?.originalEvent?.target, which suggests Radix UI passes events in a specific wrapper format. However, the type annotationReact.PointerEvent | CustomEventdoesn't accurately reflect this structure, making the casts potentially unsafe.🔎 Consider explicit type for Radix event structure
Define an explicit type for the Radix event structure:
+type RadixOutsideEvent = CustomEvent<{ + originalEvent: PointerEvent | FocusEvent; +}>; + -const handlePointerDownOutside = (event: React.PointerEvent | CustomEvent) => { - const target = (event as CustomEvent).detail?.originalEvent?.target as HTMLElement; +const handlePointerDownOutside = (event: RadixOutsideEvent) => { + const target = event.detail.originalEvent.target as HTMLElement; if (isPortaledElement(target)) { event.preventDefault(); return; } - onPointerDownOutside?.(event as any); + onPointerDownOutside?.(event); }; -const handleInteractOutside = (event: React.FocusEvent | CustomEvent) => { - const target = (event as CustomEvent).detail?.originalEvent?.target as HTMLElement; +const handleInteractOutside = (event: RadixOutsideEvent) => { + const target = event.detail.originalEvent.target as HTMLElement; if (isPortaledElement(target)) { event.preventDefault(); return; } - onInteractOutside?.(event as any); + onInteractOutside?.(event); };
68-79: Broad class selectors may match unintended elements.The patterns
[class*="-menu"]and[class*="MenuPortal"]use substring matching, which could match unintended elements with similar class names (e.g.,submenu,context-menu,MenuPortalOther).Consider more specific selectors or data attributes:
const isPortaledElement = (target: HTMLElement | null): boolean => { if (!target) return false; - // Check for react-select menu portal elements return !!( - target.closest('[class*="-menu"]') || - target.closest('[class*="MenuPortal"]') || + target.closest('[class^="react-select"][class*="-menu"]') || + target.closest('[class^="react-select"][class*="MenuPortal"]') || target.closest('[role="listbox"]') || target.closest('[role="option"]') || target.closest('[data-radix-popper-content-wrapper]') ); };Or coordinate with
asyncMultiselect.tsxto use a shared data attribute for more reliable detection.ui/components/ui/asyncMultiselect.tsx (3)
233-257: Magic number timeout could cause race conditions.The 200ms delay before clearing
isInteractingWithMenuRefappears arbitrary and could lead to timing issues:
- If a user clicks the menu, then quickly blurs, the flag might still be
true, incorrectly preventing menu close- If menu interactions complete faster than 200ms, the flag might persist unnecessarily
Consider alternatives:
- Use React's
onMouseDown/onMouseUpon the select component itself instead of global listeners- Synchronously clear the flag in
pointerupwithout the timeout- If the timeout is necessary for a specific reason, document why 200ms was chosen
🔎 Example: Remove timeout for immediate cleanup
const handlePointerUp = () => { - setTimeout(() => { - isInteractingWithMenuRef.current = false; - }, 200); + isInteractingWithMenuRef.current = false; };
237-240: Portal detection selectors differ from sheet.tsx implementation.The menu element detection logic here uses slightly different selectors than the
isPortaledElementfunction insheet.tsx:
- This file: checks for
[class*="-menu"],[role="listbox"],[role="option"],[class*="MenuPortal"]- sheet.tsx: same checks but different order and includes
[data-radix-popper-content-wrapper]Extract the portal detection logic to a shared utility function to ensure consistency:
// utils/portalDetection.ts export function isReactSelectMenuElement(target: HTMLElement | null): boolean { if (!target) return false; return !!( target.closest('[class*="-menu"]') || target.closest('[class*="MenuPortal"]') || target.closest('[role="listbox"]') || target.closest('[role="option"]') ); }Then import and use in both files.
449-449: Replace magic z-index with a documented constant.The
zIndex: 9999value is intentionally high to ensure the dropdown menu displays above other portaled elements (including Radix UI components like the sheet overlay at z-50), but it's undocumented and lacks context. Consider extracting this to a named constant (e.g.,Z_INDEX_ASYNC_MULTISELECT_MENU) to clarify the intent and make it maintainable if the value needs adjustment in the future.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ui/components/ui/asyncMultiselect.tsxui/components/ui/modelMultiselect.tsxui/components/ui/sheet.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/components/ui/sheet.tsxui/components/ui/modelMultiselect.tsxui/components/ui/asyncMultiselect.tsx
🔇 Additional comments (3)
ui/components/ui/modelMultiselect.tsx (1)
139-139: LGTM! Proper handling of empty defaultOptions.When
defaultOptionsis an empty array, passingtrueto react-select triggers it to callloadOptionsimmediately, ensuring the dropdown loads options on first open rather than appearing empty.ui/components/ui/asyncMultiselect.tsx (2)
411-412: Good defaults for portal behavior.Setting
menuPortalTargettodocument.bodyandmenuPositionto"fixed"are appropriate defaults that prevent positioning issues when the select is used inside containers with overflow or transform properties.
418-425: Blur guard prevents premature menu closure.The logic correctly prevents the menu from closing when the user is actively interacting with it, addressing the core issue described in the PR objectives.
akshaydeo
force-pushed
the
01-02-fixes_model_selected._closes_1182
branch
from
4459c07 to
6d0b8d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ui/components/ui/asyncMultiselect.tsxui/components/ui/modelMultiselect.tsxui/components/ui/sheet.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/components/ui/modelMultiselect.tsx
- ui/components/ui/sheet.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/components/ui/asyncMultiselect.tsx
🔇 Additional comments (2)
ui/components/ui/asyncMultiselect.tsx (2)
411-412: LGTM! Portal defaults align with modal/sheet usage.Setting
document.bodyas the default portal target and"fixed"positioning are appropriate defaults for ensuring the menu displays correctly above other UI layers and within modals/sheets.
449-449: LGTM! High z-index ensures proper stacking.Setting the menu portal z-index to 9999 is a standard approach for ensuring portaled menus appear above other UI layers, resolving the z-index conflicts mentioned in the PR objectives.
akshaydeo
force-pushed
the
01-02-fixes_model_selected._closes_1182
branch
from
6d0b8d8 to
9fe2245
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ui/components/ui/asyncMultiselect.tsx (1)
232-257: Add a brief comment explaining the 200ms grace period.The pointer interaction tracking logic correctly prevents premature menu closure during active selection. However, the 200ms timeout on line 249 lacks documentation explaining why this specific duration was chosen.
Suggested inline comment
const handlePointerUp = () => { + // 200ms grace period allows the blur event to fire after pointerup + // before resetting the interaction flag, preventing race conditions setTimeout(() => { isInteractingWithMenuRef.current = false; }, 200); };
🧹 Nitpick comments (3)
ui/components/ui/asyncMultiselect.tsx (1)
449-449: Consider using a design-system z-index token instead of a magic number.
z-index: 9999is effective but arbitrary. If your project has a z-index scale (e.g., CSS variables or Tailwind config), consider using a named token like--z-dropdownor similar for maintainability. This prevents z-index conflicts as the codebase grows.ui/components/ui/sheet.tsx (2)
68-79: Consider extractingisPortaledElementto a shared utility.This portal detection logic is nearly identical to the one in
asyncMultiselect.tsx(lines 237-240). To avoid duplication and ensure consistent behavior, consider extracting this to a shared utility inutils.ts.Suggested refactor
// In utils.ts export const isPortaledElement = (target: HTMLElement | null): boolean => { if (!target) return false; return !!( target.closest('[class*="-menu"]') || target.closest('[class*="MenuPortal"]') || target.closest('[role="listbox"]') || target.closest('[role="option"]') || target.closest('[data-radix-popper-content-wrapper]') ); };Then import and use in both files.
81-97: Type assertions reduce type safety; consider proper typing.The
as anycasts on lines 87 and 96 bypass TypeScript's type checking. Radix'sonPointerDownOutsideandonInteractOutsideusePointerDownOutsideEventandFocusOutsideEventfrom@radix-ui/react-dismissable-layer.Suggested improvement
+import type { PointerDownOutsideEvent, FocusOutsideEvent } from "@radix-ui/react-dismissable-layer"; -const handlePointerDownOutside = (event: React.PointerEvent | CustomEvent) => { - const target = (event as CustomEvent).detail?.originalEvent?.target as HTMLElement; +const handlePointerDownOutside = (event: PointerDownOutsideEvent) => { + const target = event.detail.originalEvent.target as HTMLElement | null; if (isPortaledElement(target)) { event.preventDefault(); return; } - onPointerDownOutside?.(event as any); + onPointerDownOutside?.(event); }; -const handleInteractOutside = (event: React.FocusEvent | CustomEvent) => { - const target = (event as CustomEvent).detail?.originalEvent?.target as HTMLElement; +const handleInteractOutside = (event: FocusOutsideEvent | PointerDownOutsideEvent) => { + const target = event.detail.originalEvent.target as HTMLElement | null; if (isPortaledElement(target)) { event.preventDefault(); return; } - onInteractOutside?.(event as any); + onInteractOutside?.(event); };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
core/versionframework/changelog.mdframework/versionplugins/governance/changelog.mdplugins/governance/versionplugins/jsonparser/changelog.mdplugins/jsonparser/versionplugins/logging/changelog.mdplugins/logging/versionplugins/maxim/changelog.mdplugins/maxim/versionplugins/mocker/changelog.mdplugins/mocker/versionplugins/otel/changelog.mdplugins/otel/versionplugins/semanticcache/changelog.mdplugins/semanticcache/versionplugins/telemetry/changelog.mdplugins/telemetry/versiontransports/changelog.mdtransports/versionui/components/ui/asyncMultiselect.tsxui/components/ui/modelMultiselect.tsxui/components/ui/sheet.tsx
✅ Files skipped from review due to trivial changes (13)
- transports/changelog.md
- plugins/governance/changelog.md
- plugins/logging/version
- plugins/semanticcache/changelog.md
- plugins/governance/version
- plugins/otel/changelog.md
- plugins/otel/version
- core/version
- plugins/mocker/changelog.md
- plugins/telemetry/version
- plugins/jsonparser/changelog.md
- plugins/logging/changelog.md
- framework/version
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/components/ui/modelMultiselect.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
plugins/jsonparser/versionplugins/telemetry/changelog.mdplugins/mocker/versionframework/changelog.mdtransports/versionplugins/semanticcache/versionui/components/ui/asyncMultiselect.tsxplugins/maxim/versionplugins/maxim/changelog.mdui/components/ui/sheet.tsx
🔇 Additional comments (11)
plugins/semanticcache/version (1)
1-1: LGTM!Version bump to 1.3.57 is consistent with the broader release updates in this PR.
plugins/maxim/version (1)
1-1: LGTM!Version bump to 1.4.59 aligns with the core/framework upgrade cycle.
plugins/jsonparser/version (1)
1-1: LGTM!Version bump to 1.3.58 is consistent with the PR's dependency upgrades.
plugins/mocker/version (1)
1-1: LGTM!Version bump to 1.3.57 aligns with the broader release updates.
transports/version (1)
1-1: LGTM!Version bump to 1.3.58 is consistent with the core/framework upgrade cycle.
plugins/maxim/changelog.md (1)
1-2: LGTM!Changelog entries appropriately document the maxim-go and core/framework upgrades.
framework/changelog.md (1)
1-1: LGTM!Changelog entry appropriately documents the core upgrade to 1.2.46.
plugins/telemetry/changelog.md (1)
1-1: Verify changelog completeness relative to PR scope.The changelog entry documents dependency upgrades (core to 1.2.46, framework to 1.1.57), but the PR objectives indicate this PR primarily addresses bug fixes for the model selector UI (AsyncMultiSelect menu closing, z-index conflicts, Sheet portal interaction, and ModelMultiselect empty defaultOptions handling). Typically, these user-facing fixes would also appear in a changelog.
Clarify whether:
- This changelog entry is for a separate maintenance/dependency bump cycle
- The bug fixes should be documented in a different changelog or location
- Version files have been updated to match these dependency versions (core 1.2.46, framework 1.1.57)
ui/components/ui/asyncMultiselect.tsx (2)
411-412: Reasonable defaults for portal behavior.Defaulting
menuPortalTargettodocument.bodyandmenuPositionto"fixed"ensures consistent portal rendering behavior across different contexts (modals, sheets, etc.). This aligns with the fix for preventing menu closure issues.
418-425: Core fix for the menu closure issue looks correct.The blur guard using
isInteractingWithMenuRefprevents the menu from closing when the user is actively interacting with portal elements. Combined with the pointer event tracking in theuseEffect, this should resolve the reported issue where model selection would fail when items were already selected.ui/components/ui/sheet.tsx (1)
103-106: Integration with Radix looks correct.The handlers properly intercept outside interactions, prevent default when the target is a portaled element (like react-select menus), and delegate to external handlers otherwise. This complements the
asyncMultiselect.tsxchanges to ensure dropdowns inside sheets work correctly.
akshaydeo
force-pushed
the
01-02-fixes_model_selected._closes_1182
branch
from
9fe2245 to
e880737
Compare
akshaydeo
force-pushed
the
01-02-fixes_model_selected._closes_1182
branch
from
e880737 to
2126414
Compare
1 participant
Fix AsyncMultiSelect and Sheet component interaction issues
Summary
Fixed interaction issues between AsyncMultiSelect components and Sheet modals by improving event handling for dropdown menus and preventing premature menu closing.
Closes #1182
Closes #1209
Changes
Type of change
Affected areas
How to test
Breaking changes
Related issues
Fixes dropdown menu interaction issues in modals and sheets
Security considerations
No security implications.
Checklist
docs/contributing/README.mdand followed the guidelines