Uh oh!
There was an error while loading. Please reload this page.
fix(ui): close only the open Select on Escape inside a Drawer - #9176
Conversation
🦋 Changeset detectedLatest commit: 1531723 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for GitHub.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdates Floating UI nesting so Escape closes a Select inside a Drawer without dismissing the Drawer, with an integration test and changeset documenting the fix. ChangesDrawer Select Escape handling
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
@clerk/astro@clerk/backend@clerk/chrome-extension@clerk/clerk-js@clerk/electron@clerk/electron-passkeys@clerk/eslint-plugin@clerk/expo@clerk/expo-passkeys@clerk/express@clerk/fastify@clerk/hono@clerk/localizations@clerk/nextjs@clerk/nuxt@clerk/react@clerk/react-router@clerk/shared@clerk/tanstack-react-start@clerk/testing@clerk/ui@clerk/upgrade@clerk/vuecommit: |
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/elements/Drawer.tsx (1)
112-121: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the floating tree node ID in sync.
floatingProps.nodeIdcan override the generated ID passed touseFloating, butFloatingNodestill uses the original ID. That splits tree registration and can break nested dismiss/focus behavior. OmitnodeIdfromfloatingProps, or derive one effective ID and use it for both.🤖 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 `@packages/ui/src/elements/Drawer.tsx` around lines 112 - 121, Update the Drawer component’s useFloating/FloatingNode setup to use one effective node ID: prevent floatingProps.nodeId from overriding the generated nodeId, or derive the override-aware ID and pass that same value to both useFloating and FloatingNode. Preserve the existing nested dismiss and focus behavior.Source: Path instructions
🤖 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 `@packages/ui/src/elements/__tests__/Drawer.test.tsx`:
- Around line 61-64: Extend the Escape-key test around the nested Select and
Drawer behavior by triggering a second Escape after confirming the Select is
dismissed, then assert that onOpenChange is called with false. Keep the existing
assertion that the first Escape does not dismiss the Drawer.
- Around line 19-20: Replace the as any casts in the
ClerkInstanceContext.Provider and EnvironmentProvider fixtures within
Drawer.test.tsx with properly typed Clerk and environment mocks. Ensure both
provider values satisfy their respective context types while preserving the
existing test data and exposing future provider shape changes to TypeScript.
---
Outside diff comments:
In `@packages/ui/src/elements/Drawer.tsx`:
- Around line 112-121: Update the Drawer component’s useFloating/FloatingNode
setup to use one effective node ID: prevent floatingProps.nodeId from overriding
the generated nodeId, or derive the override-aware ID and pass that same value
to both useFloating and FloatingNode. Preserve the existing nested dismiss and
focus behavior.
🪄 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: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ce80d4d-4e47-4a06-97a4-a275a4c5e08f
📒 Files selected for processing (4)
.changeset/drawer-select-escape.mdpackages/ui/src/elements/Drawer.tsxpackages/ui/src/elements/Select.tsxpackages/ui/src/elements/__tests__/Drawer.test.tsx
| <ClerkInstanceContext.Provider value={{ value: { client: {}, user: {} } as any }}> | ||
| <EnvironmentProvider value={{ displayConfig: { applicationName: 'TestApp' } } as any}> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'\n== Drawer.test.tsx ==\n'
cat -n packages/ui/src/elements/__tests__/Drawer.test.tsx
printf'\n== Search ClerkInstanceContext / EnvironmentProvider definitions ==\n'
rg -n "ClerkInstanceContext|EnvironmentProvider|displayConfig|applicationName|value: \{\s*client" packages/ui/src packages -g '!**/dist/**' -g '!**/build/**'printf'\n== Type outlines for likely provider files ==\n'
fd -a "ClerkInstanceContext" packages/ui/src packages ||true
fd -a "EnvironmentProvider" packages/ui/src packages ||trueRepository: clerk/javascript
Length of output: 50373
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'\n== packages/ui/src/contexts/EnvironmentContext.tsx ==\n'
cat -n packages/ui/src/contexts/EnvironmentContext.tsx
printf'\n== packages/ui/src/test/create-fixtures.tsx (relevant slice) ==\n'
sed -n '1,220p' packages/ui/src/test/create-fixtures.tsx
printf'\n== Search for reusable clerk/environment test fixtures in packages/ui ==\n'
rg -n "create-fixtures|environmentMock|ClerkInstanceContext.Provider|EnvironmentProvider value=" packages/ui/src -g '!**/dist/**' -g '!**/build/**'Repository: clerk/javascript
Length of output: 20221
Use typed fixtures for these providers. Replace the as any casts with a typed Clerk/environment mock so ClerkInstanceContext and EnvironmentProvider shape changes surface here instead of being hidden by the test setup.
🤖 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 `@packages/ui/src/elements/__tests__/Drawer.test.tsx` around lines 19 - 20,
Replace the as any casts in the ClerkInstanceContext.Provider and
EnvironmentProvider fixtures within Drawer.test.tsx with properly typed Clerk
and environment mocks. Ensure both provider values satisfy their respective
context types while preserving the existing test data and exposing future
provider shape changes to TypeScript.
Source: Coding guidelines
Uh oh!
There was an error while loading. Please reload this page.
Description
Pressing
Escapewhile aSelectwas open inside aDrawer(for example the payment method picker in Checkout) dismissed the entire Drawer instead of just the Select. Two issues were at play: theSelectnever applied floating-ui'sgetFloatingProps()to its floating element, so itsEscapehandler only lived as a native document listener that the Drawer's syntheticstopPropagation()prevented from firing; andDrawer.Rootwas not aFloatingTreenode, so nested floating elements were not recognized as its children. TheSelectnow wires up its interaction props (handlingEscapeitself and stopping propagation) and theDrawerroots a floating tree, soEscapenow closes only the openSelectand a secondEscapecloses the Drawer. To test: open the Checkout drawer, open the payment methodSelect, pressEscape, and confirm only the Select closes.BEFORE
before.mov
AFTER
after.mov
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
Bug Fixes
Tests