feat(headless): add package foundation with Dialog primitive#8474
Open
alexcarpenter wants to merge 3 commits intomainfrom
Open
feat(headless): add package foundation with Dialog primitive#8474alexcarpenter wants to merge 3 commits intomainfrom
alexcarpenter wants to merge 3 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: 26a1fec 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/dev-cli
@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/vue
commit: |
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a new internal ESM package Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/README.md`:
- Around line 9-18: The README lists per-primitive import paths (e.g.
`@clerk/headless/select`, `@clerk/headless/tooltip`, etc.) that are not actually
exported by the package; either update the README to only show the real
entrypoints (e.g. ./dialog and ./utils) or add matching exports to the package's
exports map so those import paths resolve; locate the package exports via the
package.json "exports" field and the README table rows (Accordion, Autocomplete,
Dialog, Menu, Popover, Select, Tabs, Tooltip) and remove or replace non-exported
entries or add corresponding "./<primitive>" exports to package.json before
merging.
In `@packages/headless/src/primitives/dialog/dialog.tsx`:
- Around line 239-246: DialogBackdrop renders FloatingOverlay regardless of
mounted which locks body scroll; change the return so FloatingOverlay is only
rendered when the backdrop is mounted/open: in the DialogBackdrop function
(symbols: DialogBackdrop, mounted, scoped, backdropElement, FloatingOverlay,
lockScroll) keep the early return for scoped, but for the non-scoped path return
mounted ? <FloatingOverlay
lockScroll={lockScroll}>{backdropElement}</FloatingOverlay> : null (or return
null/undefined) so FloatingOverlay is not mounted when the dialog is closed.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5f11690d-ec4b-498a-83ca-49824b481e40
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
packages/headless/README.mdpackages/headless/package.jsonpackages/headless/src/hooks/use-animations-finished.test.tspackages/headless/src/hooks/use-animations-finished.tspackages/headless/src/hooks/use-controllable-state.test.tspackages/headless/src/hooks/use-controllable-state.tspackages/headless/src/hooks/use-transition-status.test.tspackages/headless/src/hooks/use-transition-status.tspackages/headless/src/hooks/use-transition.test.tspackages/headless/src/hooks/use-transition.tspackages/headless/src/primitives/dialog/README.mdpackages/headless/src/primitives/dialog/dialog.test.tsxpackages/headless/src/primitives/dialog/dialog.tsxpackages/headless/src/primitives/dialog/index.tspackages/headless/src/test-utils/axe.tspackages/headless/src/utils/index.tspackages/headless/src/utils/render-element.test.tsxpackages/headless/src/utils/render-element.tsxpackages/headless/tsconfig.jsonpackages/headless/vite.config.tspackages/headless/vitest.config.tspackages/headless/vitest.setup.tspnpm-workspace.yaml
alexcarpenter
force-pushed
the
carp/headless-foundation
branch
from
3abe7b8 to
db66631
Compare
alexcarpenter
force-pushed
the
carp/headless-foundation
branch
from
db66631 to
7439cc3
Compare
alexcarpenter
force-pushed
the
carp/headless-foundation
branch
from
7439cc3 to
6e1dfb8
Compare
alexcarpenter
force-pushed
the
carp/headless-foundation
branch
from
6e1dfb8 to
2a5b8e2
Compare
This was referenced May 5, 2026
Member
Author
📋 PR Stack OverviewThis is a stacked PR series breaking the Review order
Key decisions to validate in this PR
Once the patterns here look good, the remaining PRs are mechanical — just "one more primitive using the same patterns." |
Introduces the @clerk/headless package — a zero-style React component library providing accessible headless UI primitives. This first PR establishes the core infrastructure and patterns: - Package scaffold: Vite build, Vitest browser tests (Chromium), TypeScript config - Core utils: renderElement (polymorphic rendering with render prop support), mergeProps (event handler chaining, style/className merging) - Hooks: useControllableState, useTransitionStatus, useAnimationsFinished, useTransition - First primitive: Dialog (modal with focus trapping, scroll lock, portal support) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
alexcarpenter
force-pushed
the
carp/headless-foundation
branch
from
2a5b8e2 to
16b5d39
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/hooks/use-transition-status.ts`:
- Around line 3-49: The hook useTransitionStatus currently has no explicit
return type; add a stable return type annotation (either an exported alias like
export type UseTransitionStatusReturn = { mounted: boolean; transitionStatus:
TransitionStatus; setMounted: React.Dispatch<React.SetStateAction<boolean>> }
and then annotate function signature as export function
useTransitionStatus(open: boolean): UseTransitionStatusReturn, or inline the
same shape) and ensure the React types are available (import React or import
type { Dispatch, SetStateAction } from 'react' and use
Dispatch<SetStateAction<boolean>> for setMounted). This makes the public API of
useTransitionStatus explicit and prevents accidental signature changes.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0cbb7e7f-ed41-4bc1-970f-86d2eee9f8b9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
packages/headless/README.mdpackages/headless/package.jsonpackages/headless/src/hooks/use-animations-finished.test.tspackages/headless/src/hooks/use-animations-finished.tspackages/headless/src/hooks/use-controllable-state.test.tspackages/headless/src/hooks/use-controllable-state.tspackages/headless/src/hooks/use-transition-status.test.tspackages/headless/src/hooks/use-transition-status.tspackages/headless/src/hooks/use-transition.test.tspackages/headless/src/hooks/use-transition.tspackages/headless/src/primitives/dialog/README.mdpackages/headless/src/primitives/dialog/dialog.test.tsxpackages/headless/src/primitives/dialog/dialog.tsxpackages/headless/src/primitives/dialog/index.tspackages/headless/src/test-utils/axe.tspackages/headless/src/utils/index.tspackages/headless/src/utils/render-element.test.tsxpackages/headless/src/utils/render-element.tsxpackages/headless/tsconfig.jsonpackages/headless/vite.config.tspackages/headless/vitest.config.tspackages/headless/vitest.setup.tspnpm-workspace.yaml
✅ Files skipped from review due to trivial changes (9)
- packages/headless/vite.config.ts
- packages/headless/vitest.config.ts
- packages/headless/src/utils/index.ts
- pnpm-workspace.yaml
- packages/headless/vitest.setup.ts
- packages/headless/src/primitives/dialog/index.ts
- packages/headless/src/hooks/use-animations-finished.ts
- packages/headless/src/primitives/dialog/README.md
- packages/headless/src/hooks/use-controllable-state.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/headless/package.json
- packages/headless/src/hooks/use-transition.ts
- packages/headless/src/hooks/use-transition-status.test.ts
- packages/headless/README.md
- packages/headless/src/utils/render-element.tsx
- packages/headless/tsconfig.json
- packages/headless/src/utils/render-element.test.tsx
- packages/headless/src/hooks/use-controllable-state.ts
- packages/headless/src/hooks/use-transition.test.ts
- packages/headless/src/primitives/dialog/dialog.tsx
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…losed FloatingOverlay applies overflow:hidden to the body, which locks scroll. Previously it was always rendered in the non-scoped path — now it's only mounted when the backdrop is visible. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/dialog/dialog.tsx`:
- Around line 258-286: DialogPopup currently always mounts FloatingFocusManager
and the popup DOM; gate rendering on the dialog's mounted state from
useDialogContext (e.g., check mounted) so that if mounted is false the component
returns null (or nothing) and does not render FloatingFocusManager or the popup
markup; keep the existing defaultProps/mergeProps logic and combinedRef, but
wrap the final return in an if (mounted) { ... } else return null to prevent
closed content and focus manager from mounting when Dialog.Portal is omitted.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: bf051868-fec4-46ae-9967-711f8129468b
📒 Files selected for processing (1)
packages/headless/src/primitives/dialog/dialog.tsx
Comment on lines
+258
to
+286
| function DialogPopup(props: DialogPopupProps) { | ||
| const { render, ...otherProps } = props; | ||
| const { popupRef, refs, getFloatingProps, floatingContext, modal, labelId, descriptionId, transitionProps } = | ||
| useDialogContext(); | ||
|
|
||
| const combinedRef = useMergeRefs([popupRef, refs.setFloating]); | ||
|
|
||
| const defaultProps = { | ||
| 'data-cl-slot': 'dialog-popup', | ||
| ref: combinedRef, | ||
| 'aria-labelledby': labelId, | ||
| 'aria-describedby': descriptionId, | ||
| ...(getFloatingProps() as React.ComponentPropsWithRef<'div'>), | ||
| ...transitionProps, | ||
| }; | ||
|
|
||
| return ( | ||
| <FloatingFocusManager | ||
| context={floatingContext} | ||
| modal={modal} | ||
| > | ||
| {renderElement({ | ||
| defaultTagName: 'div', | ||
| render, | ||
| props: mergeProps<'div'>(defaultProps, otherProps), | ||
| })} | ||
| </FloatingFocusManager> | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Dialog popup renders even when closed if Dialog.Portal is omitted.
DialogPopup is not gated by mounted, so it still mounts FloatingFocusManager and popup markup in closed state unless consumers always wrap it with Dialog.Portal. That can leak closed content and affect focus behavior.
🐛 Proposed fix
function DialogPopup(props: DialogPopupProps) {
const { render, ...otherProps } = props;
- const { popupRef, refs, getFloatingProps, floatingContext, modal, labelId, descriptionId, transitionProps } =
+ const { popupRef, refs, getFloatingProps, floatingContext, modal, labelId, descriptionId, transitionProps, mounted } =
useDialogContext();
+ if (!mounted) {
+ return null;
+ }
+
const combinedRef = useMergeRefs([popupRef, refs.setFloating]);
const defaultProps = {As per coding guidelines, "Implement proper focus management for keyboard navigation in React components" and "Restrict feedback to errors, security risks, or functionality-breaking problems."
📝 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.
Suggested change
| function DialogPopup(props: DialogPopupProps) { | |
| const { render, ...otherProps } = props; | |
| const { popupRef, refs, getFloatingProps, floatingContext, modal, labelId, descriptionId, transitionProps } = | |
| useDialogContext(); | |
| const combinedRef = useMergeRefs([popupRef, refs.setFloating]); | |
| const defaultProps = { | |
| 'data-cl-slot': 'dialog-popup', | |
| ref: combinedRef, | |
| 'aria-labelledby': labelId, | |
| 'aria-describedby': descriptionId, | |
| ...(getFloatingProps() as React.ComponentPropsWithRef<'div'>), | |
| ...transitionProps, | |
| }; | |
| return ( | |
| <FloatingFocusManager | |
| context={floatingContext} | |
| modal={modal} | |
| > | |
| {renderElement({ | |
| defaultTagName: 'div', | |
| render, | |
| props: mergeProps<'div'>(defaultProps, otherProps), | |
| })} | |
| </FloatingFocusManager> | |
| ); | |
| } | |
| function DialogPopup(props: DialogPopupProps) { | |
| const { render, ...otherProps } = props; | |
| const { popupRef, refs, getFloatingProps, floatingContext, modal, labelId, descriptionId, transitionProps, mounted } = | |
| useDialogContext(); | |
| if (!mounted) { | |
| return null; | |
| } | |
| const combinedRef = useMergeRefs([popupRef, refs.setFloating]); | |
| const defaultProps = { | |
| 'data-cl-slot': 'dialog-popup', | |
| ref: combinedRef, | |
| 'aria-labelledby': labelId, | |
| 'aria-describedby': descriptionId, | |
| ...(getFloatingProps() as React.ComponentPropsWithRef<'div'>), | |
| ...transitionProps, | |
| }; | |
| return ( | |
| <FloatingFocusManager | |
| context={floatingContext} | |
| modal={modal} | |
| > | |
| {renderElement({ | |
| defaultTagName: 'div', | |
| render, | |
| props: mergeProps<'div'>(defaultProps, otherProps), | |
| })} | |
| </FloatingFocusManager> | |
| ); | |
| } |
🤖 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/dialog/dialog.tsx` around lines 258 - 286,
DialogPopup currently always mounts FloatingFocusManager and the popup DOM; gate
rendering on the dialog's mounted state from useDialogContext (e.g., check
mounted) so that if mounted is false the component returns null (or nothing) and
does not render FloatingFocusManager or the popup markup; keep the existing
defaultProps/mergeProps logic and combinedRef, but wrap the final return in an
if (mounted) { ... } else return null to prevent closed content and focus
manager from mounting when Dialog.Portal is omitted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduces the
@clerk/headlesspackage — a zero-style React component library providing accessible headless UI primitives. This is the first in a series of stacked PRs.This PR establishes all core infrastructure and patterns:
renderElement(polymorphic rendering with render prop support,data-cl-*state attributes),mergeProps(event handler chaining, style/className merging)useControllableState,useTransitionStatus,useAnimationsFinished,useTransition— the full controlled/uncontrolled + enter/exit animation lifecycleDialog— modal with focus trapping, scroll lock, scoped portal support, compound component APIReview Stack
Test plan
pnpm buildsucceeds with clean typespnpm test— 72 tests pass (hooks, utils, Dialog)🤖 Generated with Claude Code