feat(headless): hold Popover contents while it closes - #9365
feat(headless): hold Popover contents while it closes#9365alexcarpenter wants to merge 3 commits into
Conversation
…element Escape now closes one level: the menu, leaving its parent popover or menu open. An outside press still dismisses the whole stack.
Nudges the popup along the alignment axis, the counterpart to sideOffset.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: bfe1013 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-google-signin
@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/vue
commit: |
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
📝 WalkthroughWalkthroughThe change updates nested menu Escape handling so dismissal proceeds one level at a time. It adds the public Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![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 `@packages/headless/src/primitives/menu/menu.test.tsx`:
- Around line 708-713: Strengthen the menu keyboard and popover tests by
asserting each intermediate open/closed state: in
packages/headless/src/primitives/menu/menu.test.tsx lines 708-713, verify Share
is open after ArrowRight and before Escape; lines 742-746, verify Actions is
open after the click and before Escape; and lines 753-757, verify Actions is
closed while Open popover remains open after the first Escape and before the
second.
In `@packages/headless/src/primitives/popover/popover-root.tsx`:
- Line 34: Add regression coverage for the public alignOffset property in the
popover tests: render an aligned popover with a non-zero value, verify that the
Floating UI offset middleware receives it, and assert that the resulting aligned
placement changes accordingly. Keep the existing popup selector updates separate
from this new behavior test.
In `@packages/headless/src/utils/freeze.test.tsx`:
- Around line 53-70: The test around Counter must exercise a genuine internal
state update rather than only changing the parent-provided count prop. Add
interactive state and trigger its setter while the Freeze component is frozen,
assert the update remains uncommitted, then rerender with frozen set to false
and verify the state update appears.
🪄 Autofix
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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: fa3922d0-8aed-4336-bbcf-c3bf7cbcc279
📒 Files selected for processing (12)
.changeset/headless-popover-menu.mdpackages/headless/src/primitives/menu/README.mdpackages/headless/src/primitives/menu/menu-root.tsxpackages/headless/src/primitives/menu/menu.test.tsxpackages/headless/src/primitives/popover/README.mdpackages/headless/src/primitives/popover/popover-popup.tsxpackages/headless/src/primitives/popover/popover-root.tsxpackages/headless/src/utils/freeze.test.tsxpackages/headless/src/utils/freeze.tsxpackages/headless/src/utils/index.tspackages/swingset/src/stories/popover.component.mdxpackages/ui/src/mosaic/components/popover/popover.test.tsx
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/clerk-ios(auto-detected)clerk/cli(auto-detected)clerk/clerk-android(auto-detected)
| await user.keyboard('{ArrowDown}'); | ||
| await user.keyboard('{ArrowRight}'); | ||
| await user.keyboard('{Escape}'); | ||
|
|
||
| expect(screen.getByText('Share')).toHaveAttribute('data-closed', ''); | ||
| expect(screen.getByText('Actions')).toHaveAttribute('data-open', ''); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert each required intermediate open state.
These tests can pass if the menu never opens. The second Popover test can also pass if the first Escape closes the Popover.
packages/headless/src/primitives/menu/menu.test.tsx#L708-L713: Assert thatShareis open afterArrowRightand before Escape.packages/headless/src/primitives/menu/menu.test.tsx#L742-L746: Assert thatActionsis open after the click and before Escape.packages/headless/src/primitives/menu/menu.test.tsx#L753-L757: After the first Escape, assert thatActionsis closed andOpen popoverremains open before the second Escape.
As per coding guidelines, unit tests must cover new functionality and edge cases.
📍 Affects 1 file
packages/headless/src/primitives/menu/menu.test.tsx#L708-L713(this comment)packages/headless/src/primitives/menu/menu.test.tsx#L742-L746packages/headless/src/primitives/menu/menu.test.tsx#L753-L757
🤖 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/headless/src/primitives/menu/menu.test.tsx` around lines 708 - 713,
Strengthen the menu keyboard and popover tests by asserting each intermediate
open/closed state: in packages/headless/src/primitives/menu/menu.test.tsx lines
708-713, verify Share is open after ArrowRight and before Escape; lines 742-746,
verify Actions is open after the click and before Escape; and lines 753-757,
verify Actions is closed while Open popover remains open after the first Escape
and before the second.
Source: Coding guidelines
| onOpenChange?: (open: boolean) => void; | ||
| placement?: Placement; | ||
| sideOffset?: number; | ||
| alignOffset?: number; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add coverage for alignOffset.
This new public property has no supplied regression test. Add a test that verifies a non-zero alignOffset reaches the Floating UI offset middleware and changes an aligned placement. The changed Mosaic test only updates popup selectors.
As per coding guidelines, “Unit tests are required for all new functionality.”
🤖 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/headless/src/primitives/popover/popover-root.tsx` at line 34, Add
regression coverage for the public alignOffset property in the popover tests:
render an aligned popover with a non-zero value, verify that the Floating UI
offset middleware receives it, and assert that the resulting aligned placement
changes accordingly. Keep the existing popup selector updates separate from this
new behavior test.
Source: Coding guidelines
| it('holds state updates raised from inside the frozen subtree', () => { | ||
| function Counter({ count }: { count: number }) { | ||
| return <span>count: {count}</span>; | ||
| } | ||
|
|
||
| const { rerender } = render( | ||
| <Freeze frozen={false}> | ||
| <Counter count={0} /> | ||
| </Freeze>, | ||
| ); | ||
|
|
||
| rerender( | ||
| <Freeze frozen> | ||
| <Counter count={1} /> | ||
| </Freeze>, | ||
| ); | ||
|
|
||
| expect(screen.getByText('count: 0')).toBeInTheDocument(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test an actual state update from the frozen subtree.
Counter has no state or state setter. This test only replaces the parent-provided count prop. Add an interactive state update while frozen, then verify that it stays uncommitted until frozen becomes false.
As per coding guidelines, “Unit tests are required for all new functionality” and tests must verify edge cases.
🤖 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/headless/src/utils/freeze.test.tsx` around lines 53 - 70, The test
around Counter must exercise a genuine internal state update rather than only
changing the parent-provided count prop. Add interactive state and trigger its
setter while the Freeze component is frozen, assert the update remains
uncommitted, then rerender with frozen set to false and verify the state update
appears.
Source: Coding guidelines
1 participant
Description
Three
@clerk/headlesschanges, lifted out of the Mosaic UserButton stack so they can land on their own. Each was written against the UserButton's popover but none of them are about the UserButton.Freezeholds a subtree's DOM at its last committed frame. A popover that closes because the thing it was showing changed would otherwise swap its contents on the way out: the exit animation plays over content that is already the next state.Popover.Popupnow freezes while it is closing, so what animates out is what you were looking at.It works by suspending a boundary on a promise that never settles. Renders keep happening, they just don't reach the DOM, and the pending one commits when
frozenflips back. Two details are load-bearing and are commented in place: React hides a suspended boundary's host children withdisplay: none !important, which is the opposite of what this is for, so an insertion effect putsdisplay: contentsback before paint; and hiding a boundary detaches its children's refs, so the node is captured in a ref callback rather than read back later.React.use()would express the suspend more plainly, but it needs React 19.2 and this package supports React 18.Popover.RoottakesalignOffset. Nudges the popup along the alignment axis, the waysideOffsetmoves it along the side axis. It is a preference likeplacementis:shiftstill claws the popup back when the nudge would push it out of view.Escape closes one menu level instead of the whole stack.
useDismisswas letting Escape bubble, so a menu inside a popover closed both on one keypress. It now closes the innermost open menu and leaves its parent up, whether that parent is another menu or the popover the menu sits in; pressing it again closes the next level. An outside press is deliberately the opposite and still dismisses everything at once.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change