Uh oh!
There was an error while loading. Please reload this page.
[WIP] feat(ui): Origin-aware open/close animations for popover menus and selects - #9113
[WIP] feat(ui): Origin-aware open/close animations for popover menus and selects#9113maxyinger wants to merge 8 commits into
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: ab7d076 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 |
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPopover surfaces now animate on open and close with motion-aware timing. Menus, selects, and dropdown consumers pass the new animation props through their popover wrappers, and related tests now wait for exit unmounting. ChangesPopover exit animation
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 1
🧹 Nitpick comments (2)
packages/ui/src/elements/Popover.tsx (2)
60-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded
200duplicates the theme token instead of referencing it.The comment claims
close"matches the theme's$slow(200ms) transition duration" but hardcodes the literal instead of referencing that token.Drawer.tsxuses a sharedtransitionDurationValues.drawerconstant for the same purpose, avoiding drift if the theme value changes.♻️ Proposed fix
- duration: { open: 0, close: animateExit && isMotionSafe ? 200 : 0 },+ duration: { open: 0, close: animateExit && isMotionSafe ? transitionDurationValues.slow : 0 },🤖 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/Popover.tsx` around lines 60 - 72, The Popover transition duration is hardcoded to 200 instead of using the shared theme-derived value, which can drift from the actual token. Update the `useTransitionStyles` config in `Popover` to reference the same shared transition-duration constant/pattern used by `Drawer.tsx` (for the close duration) so the timing stays aligned with the theme token.
76-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnsafe generic assertion on
isValidElement.
React.isValidElement<{ style?: React.CSSProperties }>(children)only confirmschildrenis a valid React element — it doesn't verify the element actually accepts astyleprop. If a non-DOM component (e.g. a component without a passthroughstyle) is ever passed aschildrenwithanimateExit, the cloned prop would be silently dropped/ignored by that component. Currently safe in practice since consumers only pass DOM-like elements, but worth a comment noting the assumption.🤖 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/Popover.tsx` around lines 76 - 81, The generic on React.isValidElement in Popover.tsx is only assuming the child accepts a style prop, not verifying it. Update the content/cloneElement branch in Popover to add a clear comment near the React.isValidElement/React.cloneElement logic explaining that animateExit is only intended for DOM-like children that forward style, and that non-DOM components may ignore the injected styles.
🤖 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/Popover.tsx`:
- Around line 105-112: The non-portal branch in Popover is dropping the
outsideElementsInert prop, so it never reaches FloatingFocusManager when portal
is false. Update the Popover rendering path that uses FloatingFocusManager to
pass outsideElementsInert through there as well, matching the portal-based
branch so the prop works consistently in both cases.
---
Nitpick comments:
In `@packages/ui/src/elements/Popover.tsx`:
- Around line 60-72: The Popover transition duration is hardcoded to 200 instead
of using the shared theme-derived value, which can drift from the actual token.
Update the `useTransitionStyles` config in `Popover` to reference the same
shared transition-duration constant/pattern used by `Drawer.tsx` (for the close
duration) so the timing stays aligned with the theme token.
- Around line 76-81: The generic on React.isValidElement in Popover.tsx is only
assuming the child accepts a style prop, not verifying it. Update the
content/cloneElement branch in Popover to add a clear comment near the
React.isValidElement/React.cloneElement logic explaining that animateExit is
only intended for DOM-like children that forward style, and that non-DOM
components may ignore the injected styles.
🪄 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: 9439f1f7-4c7a-4777-8c69-277d23109fda
📒 Files selected for processing (6)
.changeset/popover-enter-exit-animations.mdpackages/ui/src/components/OrganizationSwitcher/OrganizationSwitcherPopover.tsxpackages/ui/src/components/OrganizationSwitcher/index.tsxpackages/ui/src/components/UserButton/UserButtonPopover.tsxpackages/ui/src/components/UserButton/index.tsxpackages/ui/src/elements/Popover.tsx
Uh oh!
There was an error while loading. Please reload this page.
789c9ac to
896c8d7Compare896c8d7 to
3e29ac4CompareThere was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/ui/src/elements/Popover.tsx (1)
152-165: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
outsideElementsInertstill not forwarded on the non-portal path.This branch was touched again in this PR but still omits
outsideElementsInertonFloatingFocusManager, unlike the portal branch (line 140). This was already flagged in a previous review round and remains unresolved.🤖 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/Popover.tsx` around lines 152 - 165, The non-portal render path in Popover still drops outsideElementsInert on FloatingFocusManager, unlike the portal branch. Update the FloatingFocusManager usage in the Popover component’s shouldRender branch to forward outsideElementsInert from the same props/state source used in the portal path, keeping the two branches consistent.
🧹 Nitpick comments (1)
packages/ui/src/elements/Popover.tsx (1)
125-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent no-op animation for non-single-element children.
contentonly appliestransitionStyleswhenchildrenis a single valid element; for arrays/fragments/text it silently falls back tochildrenwith no animation and no warning. Currently all call sites pass a single element, so this is low risk today, but it's a fragile contract for future consumers.🤖 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/Popover.tsx` around lines 125 - 130, The animation logic in Popover’s content handling only works for a single valid React element, so non-element children silently skip `transitionStyles` and lose exit animation. Update the `content`/`React.cloneElement` path to either enforce or normalize a single animatable child in `Popover` (or document and guard the contract) so future callers don’t get a no-op when passing fragments, arrays, or text.
🤖 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.
Duplicate comments:
In `@packages/ui/src/elements/Popover.tsx`:
- Around line 152-165: The non-portal render path in Popover still drops
outsideElementsInert on FloatingFocusManager, unlike the portal branch. Update
the FloatingFocusManager usage in the Popover component’s shouldRender branch to
forward outsideElementsInert from the same props/state source used in the portal
path, keeping the two branches consistent.
---
Nitpick comments:
In `@packages/ui/src/elements/Popover.tsx`:
- Around line 125-130: The animation logic in Popover’s content handling only
works for a single valid React element, so non-element children silently skip
`transitionStyles` and lose exit animation. Update the
`content`/`React.cloneElement` path to either enforce or normalize a single
animatable child in `Popover` (or document and guard the contract) so future
callers don’t get a no-op when passing fragments, arrays, or text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d5e46c9-c7de-47f7-b21d-8b2787ff307f
📒 Files selected for processing (10)
.changeset/popover-enter-exit-animations.mdpackages/ui/src/components/OrganizationSwitcher/OrganizationSwitcherPopover.tsxpackages/ui/src/components/OrganizationSwitcher/index.tsxpackages/ui/src/components/UserButton/UserButtonPopover.tsxpackages/ui/src/components/UserButton/index.tsxpackages/ui/src/elements/Menu.tsxpackages/ui/src/elements/PhoneInput/index.tsxpackages/ui/src/elements/Popover.tsxpackages/ui/src/elements/Select.tsxpackages/ui/src/foundations/transitions.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/popover-enter-exit-animations.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/ui/src/components/OrganizationSwitcher/index.tsx
- packages/ui/src/components/UserButton/index.tsx
- packages/ui/src/components/UserButton/UserButtonPopover.tsx
- packages/ui/src/components/OrganizationSwitcher/OrganizationSwitcherPopover.tsx
53de28e to
f489835Compare
TODO
Description
Improves snappyness and adds origin awareness of org switcher, user button, menus & select popover anims.
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
initialScale(with component-specific defaults) and improved exit timing viaexitFast.fasterduration andswiftOuteasing.