feat(headless): add package foundation with Dialog primitive#8474
feat(headless): add package foundation with Dialog primitive#8474alexcarpenter wants to merge 9 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: c3d5f6b 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/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: |
|
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. 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}
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
📋 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." |
alexcarpenter
force-pushed
the
carp/headless-foundation
branch
from
2a5b8e2 to
16b5d39
Compare
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
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
Ephem
left a comment
Ephem
left a comment
There was a problem hiding this comment.
This is very neat! Just dropping off some partway review comments, mostly me asking questions to try to wrap my head around stuff. Will continue reviewing tomorrow.
I did skim the animation parts as well, they are looking great!
|
|
||
| ## Architecture | ||
|
|
||
| - **Compound components** via `Object.assign` — each primitive is a single export with dot-accessed parts (`Select.Trigger`, `Select.Popup`, etc.) |
There was a problem hiding this comment.
I think this approach entirely disables tree-shaking? Maybe not a huge deal but since we are setting this up from scratch and now is the best time to fix it if we want to, I wanted to raise it.
If we still want compound components, I think export * as Dialog from './dialog'; achieves mostly the same but would tree shake if we set sideEffects: false?
There was a problem hiding this comment.
I think to achieve full tree-shaking we'd need to move all subcomponents into their own files and export similar to base-ui is doing with parts exports. https://github.com/mui/base-ui/tree/master/packages/react/src/avatar
Since this is not a public library this seemed unnecessary. but happy to reconsider if you think we should. lmk what you think!
There was a problem hiding this comment.
At the time, I was thinking how a natural step would be: #8654
When we want to export subcomponents like that, maybe we do want the underlying primitives to treeshake as well?
I think to achieve full tree-shaking we'd need to move all subcomponents into their own files and export similar to base-ui is doing with parts exports
Yes, that's exactly what I was thinking. It's annoying though so there's a real tradeoff not being able to colocate.
|
|
||
| - **Compound components** via `Object.assign` — each primitive is a single export with dot-accessed parts (`Select.Trigger`, `Select.Popup`, etc.) | ||
| - **`renderElement`** — every part uses this instead of returning JSX directly, enabling consumer `render` prop overrides and automatic state-to-data-attribute mapping | ||
| - **`data-cl-*` attributes** — structural (`data-cl-slot`), state (`data-cl-open`, `data-cl-selected`, `data-cl-active`), and animation lifecycle (`data-cl-starting-style`, `data-cl-ending-style`) |
There was a problem hiding this comment.
I love the data-cl-* attributes. ❤️
Did you consider also namespacing these per primitive? Maybe that's not necessary?
There was a problem hiding this comment.
what do you mean per primitive?
There was a problem hiding this comment.
data-cl-select-open etc to allow for more narrow targeting when writing CSS?
There was a problem hiding this comment.
Trying to keep the list lean. My recommendation would be the compose the selectors together
[data-cl-slot="select-root"][data-cl-open] { ... }|
|
||
| Headless UI primitives for Clerk's component library. These are unstyled, accessible React components built on [Floating UI](https://floating-ui.com/) that handle positioning, keyboard navigation, focus management, and ARIA attributes. | ||
|
|
||
| This package is **internal** (`private: true`) and consumed by `@clerk/ui`. It exists as a separate package because `@clerk/ui` uses `@emotion/react` as its JSX source, which conflicts with the standard `react-jsx` transform these primitives require. |
There was a problem hiding this comment.
Is this intended to be private forever? I'm guessing yes and that anything we want public we export through ui right?
I'm trying to think through the public parts, the css data-cl-* attributes is an obvious one, the rest is probably only exposed through other components right?
Are there any cases for our public components where you think C1s will pass through render props, or any other decisions here that are going to shape public facing APIs down the line?
There was a problem hiding this comment.
so the data attributes are to be used for c1s to be able to tie into and style with. the custom rendering is internal only atm. Possibly in the future that could be a mechanism we expose for custom rendering.
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>
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>
Without Dialog.Portal wrapping it, DialogPopup rendered FloatingFocusManager and popup DOM unconditionally. Gate on mounted state to match DialogOverlay and DialogPortal behavior.
Split the monolithic dialog.tsx into individual files per subcomponent and use `export * as Dialog` namespace re-export instead of Object.assign. This enables bundlers to tree-shake unused Dialog parts at the module level. API change: <Dialog> becomes <Dialog.Root>
alexcarpenter
force-pushed
the
carp/headless-foundation
branch
from
54016b4 to
4430af5
Compare
2 participants
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