feat(ui): rebuild the Mosaic Dialog on StyleX - #9388
Conversation
🦋 Changeset detectedLatest commit: 9d59b7d 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds nested-dialog state and Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 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: 7
🧹 Nitpick comments (10)
packages/ui/src/mosaic/components/dialog/keyboard-inset.ts (1)
75-82: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the release function idempotent.
Each call to the returned function decrements
listeners. A second call on the same release drives the count below zero. The counter then never returns to0, and the listeners plus the--_cl-keyboard-insetproperty stay attached for the lifetime of the page.acquireBrowserChromeinbrowser-chrome.tsguards this case with areleasedflag; this module does not.♻️ Proposed guard
- return () => { - listeners--; - if (listeners === 0 && detach) { - detach(); - detach = null; - } - }; + let released = false; + return () => { + if (released) { + return; + } + released = true; + listeners--; + if (listeners === 0 && detach) { + detach(); + detach = null; + } + };🤖 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/mosaic/components/dialog/keyboard-inset.ts` around lines 75 - 82, Make the release function returned by the keyboard-inset acquisition flow idempotent by adding a per-release guard, similar to acquireBrowserChrome’s released flag. Only decrement listeners and detach the keyboard-inset listener/property when that release has not already been executed.packages/ui/src/mosaic/components/dialog/dialog.tsx (1)
205-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
renderinrestsilently replaces the styled Button.
CloseButtonsetsrenderbefore{...rest}. If a consumer passesrender, their element replaces theButtonwrapper, andstyles.closeButtonpluscloseInsets[size]are lost. The button then loses its absolute anchoring. Consider omittingrenderfromDialogCloseButtonProps, or documenting that the override must supply its own positioning.♻️ Proposed type change
-export interface DialogCloseButtonProps extends MosaicComponentProps<'button'> { +export interface DialogCloseButtonProps extends Omit<MosaicComponentProps<'button'>, 'render'> {🤖 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/mosaic/components/dialog/dialog.tsx` around lines 205 - 229, Prevent consumers from overriding the internal render used by CloseButton: omit render from DialogCloseButtonProps and exclude it from the rest props spread, preserving the styled Button with styles.closeButton and closeInsets[size].packages/ui/src/mosaic/components/dialog/dialog.styles.ts (1)
199-232: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim the duplicated and oversized comments in
sizes.panel.Two blocks state the same fact. Lines 214-223 explain that the panel fills the viewport content box with
stretch, and lines 224-228 repeat it. Reduce the block to a single terse note. The same applies across this file, where multi-paragraph rationale blocks dominate the style declarations.The coding guidelines require minimal comments: "Add comments only when critical to explain why a non-obvious change was made; never restate code behavior, and keep warranted comments to one terse line rather than a verbose multi-line block."
🤖 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/mosaic/components/dialog/dialog.styles.ts` around lines 199 - 232, Trim the duplicated rationale comments in sizes.panel, especially the repeated explanation around alignSelf: 'stretch', leaving one concise line only where the non-obvious layout decision requires justification. Apply the same minimal-comment standard to nearby oversized rationale blocks in this file without changing the style declarations.Source: Coding guidelines
packages/ui/src/mosaic/components/dialog/browser-chrome.ts (2)
160-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe comment describes a binary search; the code performs a linear scan.
makeEasingwalks the table withwhile (lo < SAMPLES && table[lo + 1] < x) lo++. That is a linear scan, not a binary search. Correct the comment or implement the search that it describes.🤖 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/mosaic/components/dialog/browser-chrome.ts` around lines 160 - 190, Update makeEasing so its lookup uses a binary search over the monotonic table rather than incrementing lo through entries linearly; preserve the existing interpolation and axis calculation behavior.
229-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
acquireBrowserChromedoc block is attached toresolveTint.Lines 229-237 document
acquireBrowserChromeand its@param backdrop. A second doc block forresolveTintfollows at lines 238-243, and thefunction resolveTintdeclaration follows that. TypeScript and editors therefore associate the first block with nothing, andacquireBrowserChromeat line 259 has no JSDoc. Move the first block directly aboveexport function acquireBrowserChrome.The coding guidelines require that "All public APIs must be documented with JSDoc".
🤖 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/mosaic/components/dialog/browser-chrome.ts` around lines 229 - 257, Move the first JSDoc block describing the refcounted backdrop behavior and its backdrop parameter from above resolveTint to directly above export function acquireBrowserChrome. Keep the separate resolveTint documentation attached to resolveTint, ensuring the public acquireBrowserChrome API retains its documentation.Source: Coding guidelines
packages/ui/src/mosaic/styles/index.ts (1)
17-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRe-export the headless dialog types that the exported prop types reference.
packages/ui/src/mosaic/components/dialog/index.tsalso exportsDialogFocusTarget,DialogHandle, andDialogOpenChangeDetails. This barrel omits them.DialogRootPropsandDialogTriggerPropscarry ahandle?: DialogHandle<Payload>member, andDialogPopupPropscarriesinitialFocus/finalFocusof typeDialogFocusTarget. A consumer of this entry point can therefore pass those props but cannot name their types.♻️ Proposed addition
export { Dialog } from '../components/dialog'; export type { DialogBackdropProps, DialogCloseButtonProps, DialogCloseProps, DialogDescriptionProps, + DialogFocusTarget, + DialogHandle, + DialogOpenChangeDetails, DialogPopupProps, DialogProps, DialogRootProps, DialogSize, DialogTitleProps, DialogTriggerProps, DialogViewportProps, } from '../components/dialog';🤖 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/mosaic/styles/index.ts` around lines 17 - 30, Update the dialog type re-exports in the styles barrel to include DialogFocusTarget, DialogHandle, and DialogOpenChangeDetails from the existing dialog component exports, alongside the current DialogRootProps, DialogTriggerProps, and DialogPopupProps types.packages/headless/src/primitives/dialog/dialog.test.tsx (1)
385-455: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a
Dialog.Triggerwith neither a root nor a handle.
dialog-trigger.tsxline 36 throws a documented error for this case. No test covers it. The test also exposes the hook-order problem flagged inpackages/headless/src/primitives/dialog/dialog-trigger.tsxlines 33-37, because React reports a hook-count error instead of the intended message once a store disappears between renders.💚 Proposed test
+ it('throws when the trigger has neither a root nor a handle', () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); + expect(() => render(<Dialog.Trigger>Orphan</Dialog.Trigger>)).toThrow( + /must be nested in a <Dialog.Root> or given a `handle`/, + ); + consoleError.mockRestore(); + });🤖 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.test.tsx` around lines 385 - 455, Add a test in the detached-trigger describe block that renders Dialog.Trigger without a Dialog.Root or handle and asserts the documented error from Dialog.Trigger. Include a rerender or unmount scenario where the associated store disappears to verify stable hook ordering and ensure the intended error is reported instead of a React hook-count error.Source: Coding guidelines
packages/headless/src/primitives/dialog/dialog-root.tsx (1)
121-143: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConfirm the stability assumptions in the
setRooteffect.The effect only re-runs when
storechanges.openFromTriggerandcloseFromTriggercapturerefs,floatingContext, andsetActiveTriggerIdfrom the render that attached the controller.setActiveTriggerIdcomes fromuseControllableState, whose setter identity depends onisControlled. If a consumer switchestriggerIdbetweenundefinedand a value after mount, the captured setter becomes stale and trigger attribution stops updating.applyOpenChangealready avoids this through thelatestref; consider routingsetActiveTriggerIdandsetActivePayloadthrough the same ref.♻️ Proposed change to route trigger state through the latest ref
- const latest = useRef({ applyOpenChange, activeTriggerId }); + const latest = useRef({ applyOpenChange, activeTriggerId, setActiveTriggerId, setActivePayload }); useLayoutEffect(() => { - latest.current = { applyOpenChange, activeTriggerId }; + latest.current = { applyOpenChange, activeTriggerId, setActiveTriggerId, setActivePayload }; }); useLayoutEffect(() => { return store.setRoot({ openFromTrigger: (id, event) => { const registration = store.getTrigger(id); - setActiveTriggerId(id); - setActivePayload(registration?.payload); + latest.current.setActiveTriggerId(id); + latest.current.setActivePayload(registration?.payload);🤖 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-root.tsx` around lines 121 - 143, Update the setRoot effect callbacks in the dialog root to access setActiveTriggerId and setActivePayload through the latest ref, matching the existing latest.current.applyOpenChange pattern. Ensure openFromTrigger always uses the current controllable-state setters when triggerId control changes, while preserving the existing trigger registration, reference assignment, pending details, and open-change behavior.packages/headless/src/primitives/dialog/dialog-handle.ts (1)
59-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
getRegistryVersionmember.
dialog-root.tsxre-resolves throughstore.subscribe, and no caller readsgetRegistryVersion. Remove the member, counter, and getter.🤖 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-handle.ts` around lines 59 - 60, Remove the unused getRegistryVersion() member from the dialog handle contract, along with the registry version counter and its getter implementation. Preserve the existing store.subscribe-based re-resolution in dialog-root.tsx and remove only the obsolete registry-version plumbing.packages/headless/src/primitives/dialog/use-dialog-origin.ts (1)
34-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCondense the two explanatory comment blocks.
The coding guidelines state: "keep warranted comments to one terse line rather than a verbose multi-line block". The measurement reasoning is worth recording, but six-line and four-line blocks exceed that. Reduce each to one line, or move the full rationale into the function JSDoc at Lines 8-19.
Also applies to: 50-53
🤖 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/use-dialog-origin.ts` around lines 34 - 39, Condense the explanatory comment blocks surrounding the measurement logic in use-dialog-origin, including the blocks near lines 34-39 and 50-53, to one terse line each. Preserve the essential rationale about scaled getBoundingClientRect values, unscaled offset dimensions, and transform-origin coordinates, without changing the implementation.Source: Coding guidelines
🤖 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 @.changeset/olive-doors-tell.md:
- Around line 2-3: Update the Dialog changes in the relevant UI and headless
compatibility implementations to retain deprecated support for the removed sx
and existing size APIs, preserving consumers on `@clerk/ui`@1 during this patch
release. If compatibility cannot be preserved, change the changeset entries for
`@clerk/headless` and `@clerk/ui` from patch to major and document the migration
path.
In `@packages/headless/src/primitives/dialog/dialog-root.tsx`:
- Around line 117-178: Add an SSR-safe layout-effect helper in the dialog
primitives and use it for every useLayoutEffect shown in DialogInner, including
the latest ref, root registration, reference resolution, payload lookup, state
publication, and cleanup effects. Preserve each effect’s dependencies, cleanup
behavior, and execution order while replacing the direct layout-effect usage
with the helper.
In `@packages/headless/src/primitives/dialog/README.md`:
- Around line 151-171: Qualify the dialog dismissal documentation to reflect
that Escape and outside-press dismissal depend on closedBy: in
packages/headless/src/primitives/dialog/README.md lines 151-171, state that
Escape closes only when closedBy is not 'none'; in
packages/swingset/src/stories/dialog.component.mdx lines 54-55, replace the
unconditional “always” wording with behavior conditional on the default
closedBy='any' value.
- Around line 105-108: Update the initialFocus and finalFocus callback
documentation for Dialog.Popup to remove refs from the callback return options.
Document callback results as boolean, void, HTMLElement, or null, while keeping
refs listed only as supported direct values.
In `@packages/headless/src/primitives/dialog/use-dialog-origin.ts`:
- Around line 25-54: Reset the popup’s ORIGIN_PROPERTY to the neutral center
value before calling getBoundingClientRect() in the useLayoutEffect, ensuring
reused popups are measured without the previous transform origin. Keep the
existing open/popup/trigger guards and origin calculation unchanged, then set
the computed origin afterward.
In `@packages/swingset/src/stories/dialog.component.mdx`:
- Around line 275-286: Add the missing stylex import to the panel example before
its usage in the Dialog content, ensuring the existing stylex.props calls
resolve when the snippet is copied.
In `@packages/ui/src/mosaic/components/dialog/dialog.test.tsx`:
- Around line 541-557: Complete the test around the stacked-dialog teardown flow
by sending a second Escape after the existing inner-dialog assertion, then use
waitFor to assert that themeColor() becomes null after the outer dialog closes
and deferred fade cleanup finishes.
---
Nitpick comments:
In `@packages/headless/src/primitives/dialog/dialog-handle.ts`:
- Around line 59-60: Remove the unused getRegistryVersion() member from the
dialog handle contract, along with the registry version counter and its getter
implementation. Preserve the existing store.subscribe-based re-resolution in
dialog-root.tsx and remove only the obsolete registry-version plumbing.
In `@packages/headless/src/primitives/dialog/dialog-root.tsx`:
- Around line 121-143: Update the setRoot effect callbacks in the dialog root to
access setActiveTriggerId and setActivePayload through the latest ref, matching
the existing latest.current.applyOpenChange pattern. Ensure openFromTrigger
always uses the current controllable-state setters when triggerId control
changes, while preserving the existing trigger registration, reference
assignment, pending details, and open-change behavior.
In `@packages/headless/src/primitives/dialog/dialog.test.tsx`:
- Around line 385-455: Add a test in the detached-trigger describe block that
renders Dialog.Trigger without a Dialog.Root or handle and asserts the
documented error from Dialog.Trigger. Include a rerender or unmount scenario
where the associated store disappears to verify stable hook ordering and ensure
the intended error is reported instead of a React hook-count error.
In `@packages/headless/src/primitives/dialog/use-dialog-origin.ts`:
- Around line 34-39: Condense the explanatory comment blocks surrounding the
measurement logic in use-dialog-origin, including the blocks near lines 34-39
and 50-53, to one terse line each. Preserve the essential rationale about scaled
getBoundingClientRect values, unscaled offset dimensions, and transform-origin
coordinates, without changing the implementation.
In `@packages/ui/src/mosaic/components/dialog/browser-chrome.ts`:
- Around line 160-190: Update makeEasing so its lookup uses a binary search over
the monotonic table rather than incrementing lo through entries linearly;
preserve the existing interpolation and axis calculation behavior.
- Around line 229-257: Move the first JSDoc block describing the refcounted
backdrop behavior and its backdrop parameter from above resolveTint to directly
above export function acquireBrowserChrome. Keep the separate resolveTint
documentation attached to resolveTint, ensuring the public acquireBrowserChrome
API retains its documentation.
In `@packages/ui/src/mosaic/components/dialog/dialog.styles.ts`:
- Around line 199-232: Trim the duplicated rationale comments in sizes.panel,
especially the repeated explanation around alignSelf: 'stretch', leaving one
concise line only where the non-obvious layout decision requires justification.
Apply the same minimal-comment standard to nearby oversized rationale blocks in
this file without changing the style declarations.
In `@packages/ui/src/mosaic/components/dialog/dialog.tsx`:
- Around line 205-229: Prevent consumers from overriding the internal render
used by CloseButton: omit render from DialogCloseButtonProps and exclude it from
the rest props spread, preserving the styled Button with styles.closeButton and
closeInsets[size].
In `@packages/ui/src/mosaic/components/dialog/keyboard-inset.ts`:
- Around line 75-82: Make the release function returned by the keyboard-inset
acquisition flow idempotent by adding a per-release guard, similar to
acquireBrowserChrome’s released flag. Only decrement listeners and detach the
keyboard-inset listener/property when that release has not already been
executed.
In `@packages/ui/src/mosaic/styles/index.ts`:
- Around line 17-30: Update the dialog type re-exports in the styles barrel to
include DialogFocusTarget, DialogHandle, and DialogOpenChangeDetails from the
existing dialog component exports, alongside the current DialogRootProps,
DialogTriggerProps, and DialogPopupProps types.
🪄 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: e06bfa40-bf96-4071-89f5-c32a12b34f03
📒 Files selected for processing (37)
.changeset/lucky-donuts-invite.md.changeset/olive-doors-tell.md.changeset/spicy-clocks-argue.mdpackages/headless/src/primitives/dialog/README.mdpackages/headless/src/primitives/dialog/dialog-backdrop.tsxpackages/headless/src/primitives/dialog/dialog-context.tspackages/headless/src/primitives/dialog/dialog-handle.tspackages/headless/src/primitives/dialog/dialog-popup.tsxpackages/headless/src/primitives/dialog/dialog-root.tsxpackages/headless/src/primitives/dialog/dialog-trigger.tsxpackages/headless/src/primitives/dialog/dialog-viewport.tsxpackages/headless/src/primitives/dialog/dialog.test.tsxpackages/headless/src/primitives/dialog/index.tspackages/headless/src/primitives/dialog/parts.tspackages/headless/src/primitives/dialog/use-dialog-origin.tspackages/headless/src/primitives/drawer/drawer-context.tspackages/headless/src/utils/interaction-modality.tspackages/swingset/src/stories/dialog.component.mdxpackages/swingset/src/stories/dialog.component.stories.tsxpackages/swingset/src/stories/dialog.mdxpackages/swingset/src/stories/dialog.stories.tsxpackages/ui/src/mosaic/block/destructive.tsxpackages/ui/src/mosaic/components/button/button.tsxpackages/ui/src/mosaic/components/dialog.tsxpackages/ui/src/mosaic/components/dialog/browser-chrome.tspackages/ui/src/mosaic/components/dialog/dialog.styles.tspackages/ui/src/mosaic/components/dialog/dialog.test.tsxpackages/ui/src/mosaic/components/dialog/dialog.tsxpackages/ui/src/mosaic/components/dialog/index.tspackages/ui/src/mosaic/components/dialog/keyboard-inset.tspackages/ui/src/mosaic/organization/organization-profile-domains-section-add-verify.view.tsxpackages/ui/src/mosaic/organization/organization-profile-domains-section-enrollment.view.tsxpackages/ui/src/mosaic/organization/organization-profile-domains-section-remove.view.tsxpackages/ui/src/mosaic/organization/organization-profile-profile-section.view.tsxpackages/ui/src/mosaic/primitives/dialog.tsxpackages/ui/src/mosaic/styles/index.tspackages/ui/src/mosaic/tokens.stylex.ts
🔗 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/cli(auto-detected)clerk/clerk-ios(auto-detected)clerk/clerk-android(auto-detected)
💤 Files with no reviewable changes (2)
- packages/ui/src/mosaic/primitives/dialog.tsx
- packages/ui/src/mosaic/components/dialog.tsx
| '@clerk/headless': patch | ||
| '@clerk/ui': patch |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve the removed Dialog APIs in this patch release.
This changeset states that consumers must replace sx and migrate existing size usage. That breaks consumers that remain on @clerk/ui@1 after a patch upgrade.
Keep deprecated compatibility paths for these APIs in a non-major release. Otherwise, publish this as a major release with a migration path.
As per coding guidelines, “Maintain backward compatibility in packages/clerk-js and packages/ui with SDK versions already in the wild.”
🤖 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 @.changeset/olive-doors-tell.md around lines 2 - 3, Update the Dialog changes
in the relevant UI and headless compatibility implementations to retain
deprecated support for the removed sx and existing size APIs, preserving
consumers on `@clerk/ui`@1 during this patch release. If compatibility cannot be
preserved, change the changeset entries for `@clerk/headless` and `@clerk/ui` from
patch to major and document the migration path.
Sources: Coding guidelines, Linked repositories
| useLayoutEffect(() => { | ||
| latest.current = { applyOpenChange, activeTriggerId }; | ||
| }); | ||
|
|
||
| useLayoutEffect(() => { | ||
| return store.setRoot({ | ||
| openFromTrigger: (id, event) => { | ||
| const registration = store.getTrigger(id); | ||
| setActiveTriggerId(id); | ||
| setActivePayload(registration?.payload); | ||
| if (registration) { | ||
| refs.setReference(registration.element); | ||
| } | ||
| pendingDetailsRef.current = { trigger: registration?.element ?? null, triggerId: id, event }; | ||
| floatingContext.onOpenChange(true, event, 'click'); | ||
| }, | ||
| closeFromTrigger: (id, event) => { | ||
| const registration = store.getTrigger(id); | ||
| pendingDetailsRef.current = { trigger: registration?.element ?? null, triggerId: id, event }; | ||
| floatingContext.onOpenChange(false, event, 'click'); | ||
| }, | ||
| setOpen: nextOpen => { | ||
| latest.current.applyOpenChange(nextOpen, { trigger: null, triggerId: null, event: undefined }); | ||
| }, | ||
| }); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps -- floatingContext.onOpenChange, setActiveTriggerId and refs are stable | ||
| }, [store]); | ||
|
|
||
| // The floating reference is the ACTIVE trigger — origin measurement, return focus, and | ||
| // outside-press exclusion all read `elements.domReference`. With no active trigger the first | ||
| // registered one stands in, preserving single-trigger behaviour for `defaultOpen` dialogs. | ||
| // | ||
| // Subscribed imperatively rather than through `useSyncExternalStore`: re-registration must not | ||
| // re-render this component, or a trigger whose `payload` is an inline object literal would | ||
| // re-register on every render of its own and the two would feed each other forever. | ||
| useLayoutEffect(() => { | ||
| const resolve = () => { | ||
| const active = activeTriggerId != null ? store.getTrigger(activeTriggerId) : undefined; | ||
| refs.setReference(active?.element ?? store.getFirstTrigger()?.element ?? null); | ||
| }; | ||
| resolve(); | ||
| return store.subscribe(resolve); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps -- refs is stable | ||
| }, [store, activeTriggerId]); | ||
|
|
||
| // For opens that arrive without a trigger activation — a controlled `open`/`triggerId` pair, | ||
| // `defaultOpen` — the payload is looked up from the registry once the dialog is open. Runs | ||
| // after the children's layout effects, so triggers rendered inside the root are registered by | ||
| // the time it reads, and the pre-paint re-render delivers their payload on the first frame. | ||
| useLayoutEffect(() => { | ||
| if (open) { | ||
| setActivePayload(activeTriggerId != null ? store.getTrigger(activeTriggerId)?.payload : undefined); | ||
| } | ||
| }, [store, open, activeTriggerId]); | ||
|
|
||
| // What detached triggers render their open state and ARIA wiring from. | ||
| useLayoutEffect(() => { | ||
| store.publishState({ open, triggerId: activeTriggerId, popupId: floatingContext.floatingId }); | ||
| }, [store, open, activeTriggerId, floatingContext.floatingId]); | ||
| useLayoutEffect(() => { | ||
| return () => store.publishState({ open: false, triggerId: null, popupId: undefined }); | ||
| }, [store]); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for an existing isomorphic layout effect helper and how other primitives handle SSR.
rg -nP -g 'packages/headless/**/*.{ts,tsx}' -C2 'useIsomorphicLayoutEffect|useIsoLayoutEffect|typeof (window|document) !== .undefined.'
# Check how many primitives call useLayoutEffect directly.
rg -nc -g 'packages/headless/src/**/*.tsx' 'useLayoutEffect'Repository: clerk/javascript
Length of output: 154
🏁 Script executed:
#!/bin/bash
set +e
printf '%s\n' '--- candidate files ---'
git ls-files 'packages/headless/**' | sed -n '1,160p'
printf '%s\n' '--- reviewed file imports and effects ---'
sed -n '1,230p' packages/headless/src/primitives/dialog/dialog-root.tsx
printf '%s\n' '--- all effect/helper references in headless ---'
rg -n -S -g 'packages/headless/**/*.{ts,tsx,js,jsx}' \
'use(Layout|Effect)|use(Isomorphic|Iso).*Effect|isomorphic|typeof[[:space:]]*\((window|document)\)|typeof[[:space:]]+(window|document)' \
|| true
printf '%s\n' '--- package metadata and SSR references ---'
rg -n -S -g 'packages/headless/**' -g 'packages/*/package.json' \
'SSR|server rendering|server-side|useLayoutEffect|react-dom/server|renderToString|renderToPipeableStream|react-server' \
|| trueRepository: clerk/javascript
Length of output: 18699
🏁 Script executed:
#!/bin/bash
set +e
printf '%s\n' '--- direct layout-effect usage ---'
rg -n -S 'useLayoutEffect|useIsomorphicLayoutEffect|useIsoLayoutEffect' packages/headless/src || true
printf '%s\n' '--- SSR test ---'
sed -n '1,240p' packages/headless/src/primitives/drawer/drawer.ssr.test.tsx
printf '%s\n' '--- package metadata ---'
cat packages/headless/package.json
printf '%s\n' '--- package README SSR references ---'
rg -n -i -C3 'ssr|server|render' packages/headless/README.md packages/headless/src packages/headless/package.json || true
printf '%s\n' '--- repository-wide effect helper definitions ---'
rg -n -S 'use(Isomorphic|Iso).*Effect|isomorphic.*effect|useLayoutEffect' packages | sed -n '1,240p'Repository: clerk/javascript
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set +e
printf '%s\n' '--- exact layout-effect references in source ---'
rg -n -S '\buseLayoutEffect\b|\buseIsomorphicLayoutEffect\b|\buseIsoLayoutEffect\b' packages/headless/src --glob '!**/*.test.*' || true
printf '%s\n' '--- exact layout-effect references in tests ---'
rg -n -S '\buseLayoutEffect\b|\buseIsomorphicLayoutEffect\b|\buseIsoLayoutEffect\b' packages/headless/src --glob '**/*.test.*' || true
printf '%s\n' '--- drawer SSR test ---'
cat -n packages/headless/src/primitives/drawer/drawer.ssr.test.tsx
printf '%s\n' '--- package metadata ---'
cat packages/headless/package.json
printf '%s\n' '--- SSR-related package files ---'
rg -n -i -S 'SSR|server rendering|server-side|react-dom/server|renderToString|renderToPipeableStream' packages/headless --glob '!**/*.test.*' || trueRepository: clerk/javascript
Length of output: 8072
🌐 Web query:
React useLayoutEffect server rendering warning official documentation
💡 Result:
When using useLayoutEffect in a server-side rendering (SSR) environment, React issues a warning because the hook is designed to perform DOM operations or measurements that rely on layout information which does not exist on the server [1][2][3]. Since useLayoutEffect only runs on the client, it cannot affect the initial HTML generated during server rendering, potentially leading to a "flash" or a mismatch between the server-rendered UI and the intended client-rendered UI [3][4][5]. According to official React documentation and guidance, there are several recommended approaches to resolve this warning [1][2]: 1. Replace with useEffect: If the logic does not strictly need to block the browser from repainting the screen (e.g., it is not measuring layout for immediate positioning), replace useLayoutEffect with useEffect [1][2]. This will suppress the warning because useEffect is intended to run after the paint [1][3]. 2. Defer rendering until client-side hydration: If the component's UI depends on layout measurements and would appear broken without them, delay its rendering until after the component has mounted on the client [1][2]. You can achieve this by using a state variable (e.g., isMounted) initialized to false, and setting it to true within a useEffect hook [1][2][6]. The component will then only render its layout-dependent content after hydration is complete [1]. 3. Use useSyncExternalStore: If you are synchronizing your component with an external data store rather than performing DOM measurements, consider using the useSyncExternalStore hook, which is explicitly designed to support server rendering [1][2]. Attempting to create a custom "isomorphic" layout effect hook by simply suppressing the warning does not make the hook work on the server; it only hides the diagnostic message while the underlying issue—that the effect cannot execute on the server—remains [5].
Citations:
- 1: https://react.dev/reference/react/useLayoutEffect
- 2: https://uk.react.dev/reference/react/useLayoutEffect
- 3: https://kevinyank.com/posts/useeffect-vs-uselayouteffect-and-ssr/
- 4: useLayoutEffect in ssr react/react#14927
- 5: https://handlewithcare.dev/blog/no_such_thing_isomorphic_layout_effect/
- 6: https://www.hendriklammers.com/notes/uselayouteffect-and-ssr
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from pathlib import Path
import re
root = Path("packages/headless/src")
source_files = [
p for p in root.rglob("*")
if p.suffix in {".ts", ".tsx"} and ".test." not in p.name
]
direct = []
for path in source_files:
text = path.read_text()
count = len(re.findall(r"\buseLayoutEffect\b", text))
if count:
direct.append((str(path), count))
print("direct_useLayoutEffect_files=", len(direct))
print("direct_useLayoutEffect_occurrences=", sum(count for _, count in direct))
for path, count in direct:
print(f"{path}: {count}")
print("isomorphic_helper_names_found=", any(
re.search(r"\buse(?:Isomorphic|Iso)LayoutEffect\b", p.read_text())
for p in source_files
))
print("ssr_test_files=", [
str(p) for p in root.rglob("*.ssr.test.*")
])
PYRepository: clerk/javascript
Length of output: 895
Add an SSR-safe layout-effect helper for DialogInner
@clerk/headless supports SSR but has no such helper. Add one and use it for these effects to prevent React’s server-rendering warning.
🤖 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-root.tsx` around lines 117 -
178, Add an SSR-safe layout-effect helper in the dialog primitives and use it
for every useLayoutEffect shown in DialogInner, including the latest ref, root
registration, reference resolution, payload lookup, state publication, and
cleanup effects. Preserve each effect’s dependencies, cleanup behavior, and
execution order while replacing the direct layout-effect usage with the helper.
Source: Coding guidelines
| `initialFocus` and `finalFocus` on `Dialog.Popup` control where focus moves on open and close. | ||
| Each accepts `true` (the default behaviour), `false` (do not move focus), a ref, or a function of | ||
| the interaction type behind the open/close (`'mouse' | 'touch' | 'pen' | 'keyboard' | ''`, empty | ||
| for programmatic) returning any of those: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline packages/headless/src/primitives/dialog --items all --match 'DialogFocusTarget|initialFocus|finalFocus'
rg -n -C 4 '\b(DialogFocusTarget|initialFocus|finalFocus)\b' packages/headless/src/primitives/dialog -g '*.ts' -g '*.tsx'Repository: clerk/javascript
Length of output: 22452
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- README focus documentation ---'
cat -n packages/headless/src/primitives/dialog/README.md | sed -n '95,115p;190,210p'
printf '%s\n' '--- exported focus type and implementation ---'
cat -n packages/headless/src/primitives/dialog/dialog-popup.tsx | sed -n '10,32p;60,86p;86,116p'
cat -n packages/headless/src/primitives/dialog/index.ts | sed -n '1,25p'
printf '%s\n' '--- focus-related README references ---'
rg -n -C 3 'RefObject|callback|function|initialFocus|finalFocus|focus target' packages/headless/src/primitives/dialog/README.mdRepository: clerk/javascript
Length of output: 10340
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
source = Path("packages/headless/src/primitives/dialog/dialog-popup.tsx").read_text()
readme = Path("packages/headless/src/primitives/dialog/README.md").read_text()
type_match = re.search(
r"export type DialogFocusTarget\s*=\s*(.*?);",
source,
re.S,
)
assert type_match, "DialogFocusTarget declaration not found"
type_text = " ".join(type_match.group(1).split())
callback_match = re.search(r"\(\(interactionType: InteractionType\) => ([^)]+)\)", type_text)
assert callback_match, "DialogFocusTarget callback return type not found"
callback_return = callback_match.group(1)
claim_match = re.search(
r"Each accepts .*?a ref, or a function .*?returning any of those:",
readme,
re.S,
)
assert claim_match, "README focus-target claim not found"
print("DialogFocusTarget:", type_text)
print("Callback return type:", callback_return)
print("Callback return includes RefObject:", "RefObject" in callback_return)
print("README says callback returns the ref option:", "returning any of those" in claim_match.group(0))
PYRepository: clerk/javascript
Length of output: 449
Correct the focus callback documentation.
DialogFocusTarget allows refs only as direct values. Callback results allow boolean, void, HTMLElement, or null. Remove refs from the callback description.
🤖 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/README.md` around lines 105 - 108,
Update the initialFocus and finalFocus callback documentation for Dialog.Popup
to remove refs from the callback return options. Document callback results as
boolean, void, HTMLElement, or null, while keeping refs listed only as supported
direct values.
Source: Coding guidelines
| useLayoutEffect(() => { | ||
| const popup = popupRef.current; | ||
| if (!open || !popup || !trigger) { | ||
| return; | ||
| } | ||
|
|
||
| const triggerRect = trigger.getBoundingClientRect(); | ||
| const popupRect = popup.getBoundingClientRect(); | ||
|
|
||
| // `getBoundingClientRect` reports the SCALED box, and the entering frame is already at | ||
| // `scale(0.98)`. Its CENTRE is not affected, though — the property is still unset at this | ||
| // point, so that scale is about `center` — and `offsetWidth`/`offsetHeight` are the | ||
| // unscaled layout dimensions. Together they recover the untransformed box, which is what | ||
| // `transform-origin`'s coordinates are relative to. Measuring the scaled edges instead | ||
| // would offset the origin by half the scale delta on each axis. | ||
| const centerX = popupRect.left + popupRect.width / 2; | ||
| const centerY = popupRect.top + popupRect.height / 2; | ||
| const layoutLeft = centerX - popup.offsetWidth / 2; | ||
| const layoutTop = centerY - popup.offsetHeight / 2; | ||
|
|
||
| const originX = triggerRect.left + triggerRect.width / 2 - layoutLeft; | ||
| const originY = triggerRect.top + triggerRect.height / 2 - layoutTop; | ||
|
|
||
| popup.style.setProperty(ORIGIN_PROPERTY, `${originX}px ${originY}px`); | ||
|
|
||
| // Runs in a layout effect, so this lands before paint on the frame that still carries | ||
| // `data-starting-style` — the frame pinned at `opacity: 0` with `transition: none`. Moving | ||
| // the origin repositions the scaled box, and that reflow is invisible for the same reason | ||
| // the popover's is: nothing is painted yet, and the transition arms a frame later. | ||
| }, [popupRef, trigger, open]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Determine whether the dialog popup element can survive a close/reopen cycle.
set -euo pipefail
fd . packages/headless/src/primitives/dialog --type f --extension ts --extension tsx --exec cat -n {}
# Look for keepMounted / persistence options that would retain the popup node.
rg -n -C4 'keepMounted|unmount|mounted' packages/headless/src/primitives/dialog packages/headless/src/utilsRepository: clerk/javascript
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- use-dialog-origin.ts ---'
cat -n packages/headless/src/primitives/dialog/use-dialog-origin.ts
printf '%s\n' '--- use-transition implementation and usages ---'
rg -n -C5 'export function useTransition|function useTransition|useTransition\(' packages/headless/src/hooks packages/headless/src/primitives/dialog
printf '%s\n' '--- persistence-related dialog APIs ---'
rg -n -C3 'keepMounted|unmount|mounted|presence|exit|transition' packages/headless/src packages/headless/test packages/headless 2>/dev/null | head -240Repository: clerk/javascript
Length of output: 34603
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- use-transition-status.ts ---'
cat -n packages/headless/src/hooks/use-transition-status.ts
printf '%s\n' '--- use-transition.ts ---'
cat -n packages/headless/src/hooks/use-transition.ts
printf '%s\n' '--- animation-finish helper ---'
cat -n packages/headless/src/hooks/use-animations-finished.ts
printf '%s\n' '--- rapid close/open transition tests ---'
sed -n '145,220p' packages/headless/src/hooks/use-transition.test.tsRepository: clerk/javascript
Length of output: 10946
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C5 -- '--cl-dialog-origin|data-starting-style|data-ending-style' packages | head -300Repository: clerk/javascript
Length of output: 29599
Reset --cl-dialog-origin before measuring a reused popup.
useTransition keeps the popup mounted during an exit animation and supports rapid close-to-open cancellation. The next layout effect can measure the same element with the previous --cl-dialog-origin. Clear the property or set it to center before reading getBoundingClientRect().
🤖 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/use-dialog-origin.ts` around lines 25
- 54, Reset the popup’s ORIGIN_PROPERTY to the neutral center value before
calling getBoundingClientRect() in the useLayoutEffect, ensuring reused popups
are measured without the previous transform origin. Keep the existing
open/popup/trigger guards and origin calculation unchanged, then set the
computed origin afterward.
Source: Coding guidelines
| ```tsx | ||
| import { scrollAreaRoot, scrollAreaViewport } from '@clerk/ui/mosaic/components/scroll-area'; | ||
|
|
||
| <Dialog size='panel' trigger={props => <Button {...props}>Open settings</Button>}> | ||
| <Dialog.CloseButton /> | ||
| <Dialog.Title render={<Heading size='lg' />}>Settings</Dialog.Title> | ||
|
|
||
| <div style={{ display: 'flex', flex: 1, gap: '1.5rem', minHeight: 0 }}> | ||
| <nav style={{ flex: 'none', width: '12rem' }}>…</nav> | ||
|
|
||
| <div {...stylex.props(scrollAreaRoot)} style={{ flex: 1, minWidth: 0 }}> | ||
| <div {...stylex.props(...scrollAreaViewport())}> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 "import \\* as stylex from '`@stylexjs/stylex`'|stylex\\.props" packages/ui packages/swingset \
-g '*.ts' -g '*.tsx' -g '*.mdx'Repository: clerk/javascript
Length of output: 46756
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '260,295p' packages/swingset/src/stories/dialog.component.mdx
printf '\n--- stylex references and imports in this story ---\n'
rg -n "stylex|scrollAreaRoot|scrollAreaViewport" packages/swingset/src/stories/dialog.component.mdxRepository: clerk/javascript
Length of output: 2119
Import stylex in the panel example.
The example calls stylex.props without defining stylex, so copied code fails with an unresolved identifier.
🤖 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/swingset/src/stories/dialog.component.mdx` around lines 275 - 286,
Add the missing stylex import to the panel example before its usage in the
Dialog content, ensuring the existing stylex.props calls resolve when the
snippet is copied.
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 7
🧹 Nitpick comments (10)
packages/ui/src/mosaic/components/dialog/keyboard-inset.ts (1)
75-82: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the release function idempotent.
Each call to the returned function decrements
listeners. A second call on the same release drives the count below zero. The counter then never returns to0, and the listeners plus the--_cl-keyboard-insetproperty stay attached for the lifetime of the page.acquireBrowserChromeinbrowser-chrome.tsguards this case with areleasedflag; this module does not.♻️ Proposed guard
- return () => { - listeners--; - if (listeners === 0 && detach) { - detach(); - detach = null; - } - }; + let released = false; + return () => { + if (released) { + return; + } + released = true; + listeners--; + if (listeners === 0 && detach) { + detach(); + detach = null; + } + };🤖 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/mosaic/components/dialog/keyboard-inset.ts` around lines 75 - 82, Make the release function returned by the keyboard-inset acquisition flow idempotent by adding a per-release guard, similar to acquireBrowserChrome’s released flag. Only decrement listeners and detach the keyboard-inset listener/property when that release has not already been executed.packages/ui/src/mosaic/components/dialog/dialog.tsx (1)
205-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
renderinrestsilently replaces the styled Button.
CloseButtonsetsrenderbefore{...rest}. If a consumer passesrender, their element replaces theButtonwrapper, andstyles.closeButtonpluscloseInsets[size]are lost. The button then loses its absolute anchoring. Consider omittingrenderfromDialogCloseButtonProps, or documenting that the override must supply its own positioning.♻️ Proposed type change
-export interface DialogCloseButtonProps extends MosaicComponentProps<'button'> { +export interface DialogCloseButtonProps extends Omit<MosaicComponentProps<'button'>, 'render'> {🤖 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/mosaic/components/dialog/dialog.tsx` around lines 205 - 229, Prevent consumers from overriding the internal render used by CloseButton: omit render from DialogCloseButtonProps and exclude it from the rest props spread, preserving the styled Button with styles.closeButton and closeInsets[size].packages/ui/src/mosaic/components/dialog/dialog.styles.ts (1)
199-232: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim the duplicated and oversized comments in
sizes.panel.Two blocks state the same fact. Lines 214-223 explain that the panel fills the viewport content box with
stretch, and lines 224-228 repeat it. Reduce the block to a single terse note. The same applies across this file, where multi-paragraph rationale blocks dominate the style declarations.The coding guidelines require minimal comments: "Add comments only when critical to explain why a non-obvious change was made; never restate code behavior, and keep warranted comments to one terse line rather than a verbose multi-line block."
🤖 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/mosaic/components/dialog/dialog.styles.ts` around lines 199 - 232, Trim the duplicated rationale comments in sizes.panel, especially the repeated explanation around alignSelf: 'stretch', leaving one concise line only where the non-obvious layout decision requires justification. Apply the same minimal-comment standard to nearby oversized rationale blocks in this file without changing the style declarations.Source: Coding guidelines
packages/ui/src/mosaic/components/dialog/browser-chrome.ts (2)
160-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe comment describes a binary search; the code performs a linear scan.
makeEasingwalks the table withwhile (lo < SAMPLES && table[lo + 1] < x) lo++. That is a linear scan, not a binary search. Correct the comment or implement the search that it describes.🤖 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/mosaic/components/dialog/browser-chrome.ts` around lines 160 - 190, Update makeEasing so its lookup uses a binary search over the monotonic table rather than incrementing lo through entries linearly; preserve the existing interpolation and axis calculation behavior.
229-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
acquireBrowserChromedoc block is attached toresolveTint.Lines 229-237 document
acquireBrowserChromeand its@param backdrop. A second doc block forresolveTintfollows at lines 238-243, and thefunction resolveTintdeclaration follows that. TypeScript and editors therefore associate the first block with nothing, andacquireBrowserChromeat line 259 has no JSDoc. Move the first block directly aboveexport function acquireBrowserChrome.The coding guidelines require that "All public APIs must be documented with JSDoc".
🤖 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/mosaic/components/dialog/browser-chrome.ts` around lines 229 - 257, Move the first JSDoc block describing the refcounted backdrop behavior and its backdrop parameter from above resolveTint to directly above export function acquireBrowserChrome. Keep the separate resolveTint documentation attached to resolveTint, ensuring the public acquireBrowserChrome API retains its documentation.Source: Coding guidelines
packages/ui/src/mosaic/styles/index.ts (1)
17-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRe-export the headless dialog types that the exported prop types reference.
packages/ui/src/mosaic/components/dialog/index.tsalso exportsDialogFocusTarget,DialogHandle, andDialogOpenChangeDetails. This barrel omits them.DialogRootPropsandDialogTriggerPropscarry ahandle?: DialogHandle<Payload>member, andDialogPopupPropscarriesinitialFocus/finalFocusof typeDialogFocusTarget. A consumer of this entry point can therefore pass those props but cannot name their types.♻️ Proposed addition
export { Dialog } from '../components/dialog'; export type { DialogBackdropProps, DialogCloseButtonProps, DialogCloseProps, DialogDescriptionProps, + DialogFocusTarget, + DialogHandle, + DialogOpenChangeDetails, DialogPopupProps, DialogProps, DialogRootProps, DialogSize, DialogTitleProps, DialogTriggerProps, DialogViewportProps, } from '../components/dialog';🤖 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/mosaic/styles/index.ts` around lines 17 - 30, Update the dialog type re-exports in the styles barrel to include DialogFocusTarget, DialogHandle, and DialogOpenChangeDetails from the existing dialog component exports, alongside the current DialogRootProps, DialogTriggerProps, and DialogPopupProps types.packages/headless/src/primitives/dialog/dialog.test.tsx (1)
385-455: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a
Dialog.Triggerwith neither a root nor a handle.
dialog-trigger.tsxline 36 throws a documented error for this case. No test covers it. The test also exposes the hook-order problem flagged inpackages/headless/src/primitives/dialog/dialog-trigger.tsxlines 33-37, because React reports a hook-count error instead of the intended message once a store disappears between renders.💚 Proposed test
+ it('throws when the trigger has neither a root nor a handle', () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); + expect(() => render(<Dialog.Trigger>Orphan</Dialog.Trigger>)).toThrow( + /must be nested in a <Dialog.Root> or given a `handle`/, + ); + consoleError.mockRestore(); + });🤖 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.test.tsx` around lines 385 - 455, Add a test in the detached-trigger describe block that renders Dialog.Trigger without a Dialog.Root or handle and asserts the documented error from Dialog.Trigger. Include a rerender or unmount scenario where the associated store disappears to verify stable hook ordering and ensure the intended error is reported instead of a React hook-count error.Source: Coding guidelines
packages/headless/src/primitives/dialog/dialog-root.tsx (1)
121-143: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConfirm the stability assumptions in the
setRooteffect.The effect only re-runs when
storechanges.openFromTriggerandcloseFromTriggercapturerefs,floatingContext, andsetActiveTriggerIdfrom the render that attached the controller.setActiveTriggerIdcomes fromuseControllableState, whose setter identity depends onisControlled. If a consumer switchestriggerIdbetweenundefinedand a value after mount, the captured setter becomes stale and trigger attribution stops updating.applyOpenChangealready avoids this through thelatestref; consider routingsetActiveTriggerIdandsetActivePayloadthrough the same ref.♻️ Proposed change to route trigger state through the latest ref
- const latest = useRef({ applyOpenChange, activeTriggerId }); + const latest = useRef({ applyOpenChange, activeTriggerId, setActiveTriggerId, setActivePayload }); useLayoutEffect(() => { - latest.current = { applyOpenChange, activeTriggerId }; + latest.current = { applyOpenChange, activeTriggerId, setActiveTriggerId, setActivePayload }; }); useLayoutEffect(() => { return store.setRoot({ openFromTrigger: (id, event) => { const registration = store.getTrigger(id); - setActiveTriggerId(id); - setActivePayload(registration?.payload); + latest.current.setActiveTriggerId(id); + latest.current.setActivePayload(registration?.payload);🤖 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-root.tsx` around lines 121 - 143, Update the setRoot effect callbacks in the dialog root to access setActiveTriggerId and setActivePayload through the latest ref, matching the existing latest.current.applyOpenChange pattern. Ensure openFromTrigger always uses the current controllable-state setters when triggerId control changes, while preserving the existing trigger registration, reference assignment, pending details, and open-change behavior.packages/headless/src/primitives/dialog/dialog-handle.ts (1)
59-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
getRegistryVersionmember.
dialog-root.tsxre-resolves throughstore.subscribe, and no caller readsgetRegistryVersion. Remove the member, counter, and getter.🤖 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-handle.ts` around lines 59 - 60, Remove the unused getRegistryVersion() member from the dialog handle contract, along with the registry version counter and its getter implementation. Preserve the existing store.subscribe-based re-resolution in dialog-root.tsx and remove only the obsolete registry-version plumbing.packages/headless/src/primitives/dialog/use-dialog-origin.ts (1)
34-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCondense the two explanatory comment blocks.
The coding guidelines state: "keep warranted comments to one terse line rather than a verbose multi-line block". The measurement reasoning is worth recording, but six-line and four-line blocks exceed that. Reduce each to one line, or move the full rationale into the function JSDoc at Lines 8-19.
Also applies to: 50-53
🤖 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/use-dialog-origin.ts` around lines 34 - 39, Condense the explanatory comment blocks surrounding the measurement logic in use-dialog-origin, including the blocks near lines 34-39 and 50-53, to one terse line each. Preserve the essential rationale about scaled getBoundingClientRect values, unscaled offset dimensions, and transform-origin coordinates, without changing the implementation.Source: Coding guidelines
🤖 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 @.changeset/olive-doors-tell.md:
- Around line 2-3: Update the Dialog changes in the relevant UI and headless
compatibility implementations to retain deprecated support for the removed sx
and existing size APIs, preserving consumers on `@clerk/ui`@1 during this patch
release. If compatibility cannot be preserved, change the changeset entries for
`@clerk/headless` and `@clerk/ui` from patch to major and document the migration
path.
In `@packages/headless/src/primitives/dialog/dialog-root.tsx`:
- Around line 117-178: Add an SSR-safe layout-effect helper in the dialog
primitives and use it for every useLayoutEffect shown in DialogInner, including
the latest ref, root registration, reference resolution, payload lookup, state
publication, and cleanup effects. Preserve each effect’s dependencies, cleanup
behavior, and execution order while replacing the direct layout-effect usage
with the helper.
In `@packages/headless/src/primitives/dialog/README.md`:
- Around line 151-171: Qualify the dialog dismissal documentation to reflect
that Escape and outside-press dismissal depend on closedBy: in
packages/headless/src/primitives/dialog/README.md lines 151-171, state that
Escape closes only when closedBy is not 'none'; in
packages/swingset/src/stories/dialog.component.mdx lines 54-55, replace the
unconditional “always” wording with behavior conditional on the default
closedBy='any' value.
- Around line 105-108: Update the initialFocus and finalFocus callback
documentation for Dialog.Popup to remove refs from the callback return options.
Document callback results as boolean, void, HTMLElement, or null, while keeping
refs listed only as supported direct values.
In `@packages/headless/src/primitives/dialog/use-dialog-origin.ts`:
- Around line 25-54: Reset the popup’s ORIGIN_PROPERTY to the neutral center
value before calling getBoundingClientRect() in the useLayoutEffect, ensuring
reused popups are measured without the previous transform origin. Keep the
existing open/popup/trigger guards and origin calculation unchanged, then set
the computed origin afterward.
In `@packages/swingset/src/stories/dialog.component.mdx`:
- Around line 275-286: Add the missing stylex import to the panel example before
its usage in the Dialog content, ensuring the existing stylex.props calls
resolve when the snippet is copied.
In `@packages/ui/src/mosaic/components/dialog/dialog.test.tsx`:
- Around line 541-557: Complete the test around the stacked-dialog teardown flow
by sending a second Escape after the existing inner-dialog assertion, then use
waitFor to assert that themeColor() becomes null after the outer dialog closes
and deferred fade cleanup finishes.
---
Nitpick comments:
In `@packages/headless/src/primitives/dialog/dialog-handle.ts`:
- Around line 59-60: Remove the unused getRegistryVersion() member from the
dialog handle contract, along with the registry version counter and its getter
implementation. Preserve the existing store.subscribe-based re-resolution in
dialog-root.tsx and remove only the obsolete registry-version plumbing.
In `@packages/headless/src/primitives/dialog/dialog-root.tsx`:
- Around line 121-143: Update the setRoot effect callbacks in the dialog root to
access setActiveTriggerId and setActivePayload through the latest ref, matching
the existing latest.current.applyOpenChange pattern. Ensure openFromTrigger
always uses the current controllable-state setters when triggerId control
changes, while preserving the existing trigger registration, reference
assignment, pending details, and open-change behavior.
In `@packages/headless/src/primitives/dialog/dialog.test.tsx`:
- Around line 385-455: Add a test in the detached-trigger describe block that
renders Dialog.Trigger without a Dialog.Root or handle and asserts the
documented error from Dialog.Trigger. Include a rerender or unmount scenario
where the associated store disappears to verify stable hook ordering and ensure
the intended error is reported instead of a React hook-count error.
In `@packages/headless/src/primitives/dialog/use-dialog-origin.ts`:
- Around line 34-39: Condense the explanatory comment blocks surrounding the
measurement logic in use-dialog-origin, including the blocks near lines 34-39
and 50-53, to one terse line each. Preserve the essential rationale about scaled
getBoundingClientRect values, unscaled offset dimensions, and transform-origin
coordinates, without changing the implementation.
In `@packages/ui/src/mosaic/components/dialog/browser-chrome.ts`:
- Around line 160-190: Update makeEasing so its lookup uses a binary search over
the monotonic table rather than incrementing lo through entries linearly;
preserve the existing interpolation and axis calculation behavior.
- Around line 229-257: Move the first JSDoc block describing the refcounted
backdrop behavior and its backdrop parameter from above resolveTint to directly
above export function acquireBrowserChrome. Keep the separate resolveTint
documentation attached to resolveTint, ensuring the public acquireBrowserChrome
API retains its documentation.
In `@packages/ui/src/mosaic/components/dialog/dialog.styles.ts`:
- Around line 199-232: Trim the duplicated rationale comments in sizes.panel,
especially the repeated explanation around alignSelf: 'stretch', leaving one
concise line only where the non-obvious layout decision requires justification.
Apply the same minimal-comment standard to nearby oversized rationale blocks in
this file without changing the style declarations.
In `@packages/ui/src/mosaic/components/dialog/dialog.tsx`:
- Around line 205-229: Prevent consumers from overriding the internal render
used by CloseButton: omit render from DialogCloseButtonProps and exclude it from
the rest props spread, preserving the styled Button with styles.closeButton and
closeInsets[size].
In `@packages/ui/src/mosaic/components/dialog/keyboard-inset.ts`:
- Around line 75-82: Make the release function returned by the keyboard-inset
acquisition flow idempotent by adding a per-release guard, similar to
acquireBrowserChrome’s released flag. Only decrement listeners and detach the
keyboard-inset listener/property when that release has not already been
executed.
In `@packages/ui/src/mosaic/styles/index.ts`:
- Around line 17-30: Update the dialog type re-exports in the styles barrel to
include DialogFocusTarget, DialogHandle, and DialogOpenChangeDetails from the
existing dialog component exports, alongside the current DialogRootProps,
DialogTriggerProps, and DialogPopupProps types.
🪄 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: e06bfa40-bf96-4071-89f5-c32a12b34f03
📒 Files selected for processing (37)
.changeset/lucky-donuts-invite.md.changeset/olive-doors-tell.md.changeset/spicy-clocks-argue.mdpackages/headless/src/primitives/dialog/README.mdpackages/headless/src/primitives/dialog/dialog-backdrop.tsxpackages/headless/src/primitives/dialog/dialog-context.tspackages/headless/src/primitives/dialog/dialog-handle.tspackages/headless/src/primitives/dialog/dialog-popup.tsxpackages/headless/src/primitives/dialog/dialog-root.tsxpackages/headless/src/primitives/dialog/dialog-trigger.tsxpackages/headless/src/primitives/dialog/dialog-viewport.tsxpackages/headless/src/primitives/dialog/dialog.test.tsxpackages/headless/src/primitives/dialog/index.tspackages/headless/src/primitives/dialog/parts.tspackages/headless/src/primitives/dialog/use-dialog-origin.tspackages/headless/src/primitives/drawer/drawer-context.tspackages/headless/src/utils/interaction-modality.tspackages/swingset/src/stories/dialog.component.mdxpackages/swingset/src/stories/dialog.component.stories.tsxpackages/swingset/src/stories/dialog.mdxpackages/swingset/src/stories/dialog.stories.tsxpackages/ui/src/mosaic/block/destructive.tsxpackages/ui/src/mosaic/components/button/button.tsxpackages/ui/src/mosaic/components/dialog.tsxpackages/ui/src/mosaic/components/dialog/browser-chrome.tspackages/ui/src/mosaic/components/dialog/dialog.styles.tspackages/ui/src/mosaic/components/dialog/dialog.test.tsxpackages/ui/src/mosaic/components/dialog/dialog.tsxpackages/ui/src/mosaic/components/dialog/index.tspackages/ui/src/mosaic/components/dialog/keyboard-inset.tspackages/ui/src/mosaic/organization/organization-profile-domains-section-add-verify.view.tsxpackages/ui/src/mosaic/organization/organization-profile-domains-section-enrollment.view.tsxpackages/ui/src/mosaic/organization/organization-profile-domains-section-remove.view.tsxpackages/ui/src/mosaic/organization/organization-profile-profile-section.view.tsxpackages/ui/src/mosaic/primitives/dialog.tsxpackages/ui/src/mosaic/styles/index.tspackages/ui/src/mosaic/tokens.stylex.ts
🔗 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/cli(auto-detected)clerk/clerk-ios(auto-detected)clerk/clerk-android(auto-detected)
💤 Files with no reviewable changes (2)
- packages/ui/src/mosaic/primitives/dialog.tsx
- packages/ui/src/mosaic/components/dialog.tsx
🛑 Comments failed to post (1)
packages/headless/src/primitives/dialog/README.md (1)
151-171: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document dismissal as conditional on
closedBy.
closedBy='none'disables both Escape and outside-press dismissal. The current documentation makes unconditional dismissal claims.
packages/headless/src/primitives/dialog/README.md#L151-L171: qualify the Keyboard section so Escape closes only whenclosedByis not'none'.packages/swingset/src/stories/dialog.component.mdx#L54-L55: replace “always” with behavior conditional on the defaultclosedBy='any'value.📍 Affects 2 files
packages/headless/src/primitives/dialog/README.md#L151-L171(this comment)packages/swingset/src/stories/dialog.component.mdx#L54-L55🤖 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/README.md` around lines 151 - 171, Qualify the dialog dismissal documentation to reflect that Escape and outside-press dismissal depend on closedBy: in packages/headless/src/primitives/dialog/README.md lines 151-171, state that Escape closes only when closedBy is not 'none'; in packages/swingset/src/stories/dialog.component.mdx lines 54-55, replace the unconditional “always” wording with behavior conditional on the default closedBy='any' value.
340bbc0 to
ce2b3bd
Compare
maxyinger
changed the title
a8e71b4 to
26bf249
Compare
b573a89 to
d0aef82
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
packages/ui/src/mosaic/tokens.stylex.ts (1)
316-323: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce the motion-token comment.
Replace this multi-line usage policy with one terse sentence. Move detailed Dialog-specific guidance to Dialog documentation if it is required. The current text can drift as token consumers change.
🤖 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/mosaic/tokens.stylex.ts` around lines 316 - 323, Reduce the comment documenting --cl-ease-enter to one concise sentence describing its purpose, removing consumer-specific usage policy and Dialog guidance. Keep the token definition unchanged and do not add replacement documentation here.Source: Coding guidelines
packages/ui/src/mosaic/components/dialog/dialog.test.tsx (2)
484-497: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the duplicated ref-forwarding test.
This test is identical to
forwards the ref to the popup elementat Lines 138-151. Both render the same compound tree and assert the same identity. Keep one.🤖 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/mosaic/components/dialog/dialog.test.tsx` around lines 484 - 497, Remove the duplicated test case “still forwards the popup ref alongside the observing one” from the Dialog tests, keeping the existing “forwards the ref to the popup element” coverage unchanged.
158-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused module-scope trigger and share one definition.
addEmailTriggerSharedat Lines 158-165 is never referenced. Bothdescribeblocks declare their own identicaladdEmailTrigger— Lines 168-175 and Lines 248-255. Keep one module-scope definition and use it in both blocks.♻️ Proposed change
-const addEmailTriggerShared = (props: MosaicComponentProps<'button'>) => ( +const addEmailTrigger = (props: MosaicComponentProps<'button'>) => ( <button type='button' {...props} > Add email </button> ); describe('nested Mosaic Dialogs', () => { - const addEmailTrigger = (props: MosaicComponentProps<'button'>) => ( - <button - type='button' - {...props} - > - Add email - </button> - ); - function Nested() {Delete the second local copy at Lines 248-255 as well.
🤖 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/mosaic/components/dialog/dialog.test.tsx` around lines 158 - 175, Remove the unused local addEmailTrigger declaration and retain a single module-scope trigger definition, renaming or reusing addEmailTriggerShared as needed. Update both nested Mosaic Dialogs test blocks to reference that shared definition.packages/ui/src/mosaic/components/dialog/dialog.tsx (1)
260-281: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd explicit return types to the exported function components.
Dialog,Root, andDialogContenthave inferred return types. The coding guidelines require explicit return types for functions, especially public APIs.Dialogis the package's public entry point for this component.♻️ Proposed change
-export function Dialog({ trigger, children, size, open, defaultOpen, onOpenChange, modal, closedBy }: DialogProps) { +export function Dialog({ + trigger, + children, + size, + open, + defaultOpen, + onOpenChange, + modal, + closedBy, +}: DialogProps): React.JSX.Element {Apply the same annotation to
Root(Line 60) andDialogContent(Line 242).As per coding guidelines, "Always define explicit return types for functions, especially public APIs".
🤖 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/mosaic/components/dialog/dialog.tsx` around lines 260 - 281, Annotate the exported function components Dialog, Root, and DialogContent with explicit return types, using the appropriate JSX/component return type already supported by the project. Keep their existing props and rendering behavior unchanged.Source: Coding guidelines
packages/swingset/src/stories/dialog.component.mdx (2)
19-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMark
childrenas required in the props table.
childrenis a required property ofDialogPropsinpackages/ui/src/mosaic/components/dialog/dialog.tsxat Line 237. The path instructions require required props to carry a(required)suffix and every table to show aDefaultcolumn value.♻️ Proposed change
- { name: 'children', type: 'ReactNode | ((ctx: { close: () => void }) => ReactNode)' }, + { name: 'children', type: 'ReactNode | ((ctx: { close: () => void }) => ReactNode) (required)' },As per path instructions, "Every props table must include a separate
Defaultcolumn; use—for no default, append(required)for required props."🤖 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/swingset/src/stories/dialog.component.mdx` around lines 19 - 30, Update the children entry in the DialogStories PropTable extra metadata to append “(required)” to its type, and provide the table’s required Default column value as “—” for props without defaults, while preserving existing defaults.Source: Path instructions
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDocument the hybrid Dialog archetype.
Dialogexposes both a flatDialogwrapper and compound parts such asDialog.RootandDialog.Popup. Its story also definesmeta.stylesfor the size playground. Therefore,Playground → Props → Usage → Parts → Styling → Examplesmatches neither Components archetype. Add a hybrid archetype topackages/swingset/CLAUDE.mdand define its required section order, or split the flat and compound API documentation.🤖 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/swingset/src/stories/dialog.component.mdx` at line 17, Add a hybrid Dialog documentation archetype to packages/swingset/CLAUDE.md, covering both the flat Dialog wrapper and compound parts such as Dialog.Root and Dialog.Popup, and define the required section order as Playground → Props → Usage → Parts → Styling → Examples. Keep the existing story documentation aligned with this hybrid structure rather than splitting the API unless necessary.Source: Path instructions
packages/swingset/src/stories/dialog.component.stories.tsx (2)
22-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCast the knobs through a local
knobsAsPropshelper.
DefaultacceptsRecord<string, unknown>, which is correct, but it casts inline withargs as { size?: DialogSize }. The path instructions require the cast to go through a localknobsAsPropshelper for simple, knob-driven CVA story functions.♻️ Proposed change
+const knobsAsProps = (args: Record<string, unknown>) => args as { size?: DialogSize }; + export function Default(args: Record<string, unknown>) { - const { size } = args as { size?: DialogSize }; + const { size } = knobsAsProps(args);As per path instructions, "Simple CVA story functions must accept
Record<string, unknown>and cast through a localknobsAsPropshelper before rendering the typed component."🤖 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/swingset/src/stories/dialog.component.stories.tsx` around lines 22 - 48, Update the Default story to introduce a local knobsAsProps helper that converts the Record<string, unknown> args to the typed { size?: DialogSize } props, then use that helper before rendering Dialog instead of the inline cast. Keep Default’s existing signature and rendering behavior unchanged.Source: Path instructions
22-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
knobsAsPropsinDefault.Replace the inline
args as { size?: DialogSize }cast with a localknobsAsPropshelper, as required for Components stories.🤖 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/swingset/src/stories/dialog.component.stories.tsx` around lines 22 - 35, Update the Default story’s argument handling to use a local knobsAsProps helper instead of the inline args as { size?: DialogSize } cast. Preserve the existing size mapping and story behavior while conforming to the Components story pattern.Source: Path instructions
packages/ui/src/mosaic/components/dialog/dialog.styles.ts (1)
153-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc blocks are attached to the wrong declarations in
dialog.styles.ts. Two comment blocks describe a constant other than the one they precede, so a reader inspecting a symbol sees the contract of a different symbol. The shared root cause is comment placement, not comment content — the text itself is accurate.
packages/ui/src/mosaic/components/dialog/dialog.styles.ts#L153-L187: move the first block (Lines 153-170, which documents theprompt/card/panelwidths) so it sits immediately aboveexport const sizesat Line 230, and leave only the inside/outside scroll block aboveviewportSizes.packages/ui/src/mosaic/components/dialog/dialog.styles.ts#L394-L419: split the block so the scale and radius paragraphs (Lines 394-404) sit aboveENTER_SCALEandpopupRadius, and only the exit-curve paragraphs (Lines 405-415) stay aboveSHEET_EXIT_EASE.🤖 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/mosaic/components/dialog/dialog.styles.ts` around lines 153 - 187, Correct comment placement in packages/ui/src/mosaic/components/dialog/dialog.styles.ts: at lines 153-187, move the prompt/card/panel width documentation immediately above export const sizes and leave only the inside/outside scroll documentation above viewportSizes. At lines 394-419, place the scale and radius paragraphs above ENTER_SCALE and popupRadius, while keeping only the exit-curve paragraphs above SHEET_EXIT_EASE; do not alter the comment 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.
Inline comments:
In @.changeset/olive-doors-tell.md:
- Around line 1-2: Restore the changeset entry in the empty
`.changeset/olive-doors-tell.md` file for the `@clerk/ui` Dialog API change.
Choose either a minor bump with deprecated compatibility shims for removed
props, or a major bump documenting migration from `sx` and popup-level `size`;
include the corresponding changelog details and valid Changesets frontmatter.
In `@packages/swingset/src/stories/dialog.component.mdx`:
- Around line 181-225: Update the Dialog.Root description in the Parts table to
remove the nonexistent handle prop, leaving only the supported state and sizing
properties. Update the data-size entry in the state attributes table so it
applies to both Dialog.Viewport and Dialog.Popup, reflecting the public styling
hook emitted by the implementation.
In `@packages/swingset/src/stories/dialog.component.stories.tsx`:
- Line 137: Update the Input in AddValueDialog at
packages/swingset/src/stories/dialog.component.stories.tsx:137-137 to use title
as its aria-label, and update the Input in CardSurface at
packages/swingset/src/stories/dialog.component.stories.tsx:364-364 to use “Email
address” as its aria-label; retain the existing placeholders.
---
Nitpick comments:
In `@packages/swingset/src/stories/dialog.component.mdx`:
- Around line 19-30: Update the children entry in the DialogStories PropTable
extra metadata to append “(required)” to its type, and provide the table’s
required Default column value as “—” for props without defaults, while
preserving existing defaults.
- Line 17: Add a hybrid Dialog documentation archetype to
packages/swingset/CLAUDE.md, covering both the flat Dialog wrapper and compound
parts such as Dialog.Root and Dialog.Popup, and define the required section
order as Playground → Props → Usage → Parts → Styling → Examples. Keep the
existing story documentation aligned with this hybrid structure rather than
splitting the API unless necessary.
In `@packages/swingset/src/stories/dialog.component.stories.tsx`:
- Around line 22-48: Update the Default story to introduce a local knobsAsProps
helper that converts the Record<string, unknown> args to the typed { size?:
DialogSize } props, then use that helper before rendering Dialog instead of the
inline cast. Keep Default’s existing signature and rendering behavior unchanged.
- Around line 22-35: Update the Default story’s argument handling to use a local
knobsAsProps helper instead of the inline args as { size?: DialogSize } cast.
Preserve the existing size mapping and story behavior while conforming to the
Components story pattern.
In `@packages/ui/src/mosaic/components/dialog/dialog.styles.ts`:
- Around line 153-187: Correct comment placement in
packages/ui/src/mosaic/components/dialog/dialog.styles.ts: at lines 153-187,
move the prompt/card/panel width documentation immediately above export const
sizes and leave only the inside/outside scroll documentation above
viewportSizes. At lines 394-419, place the scale and radius paragraphs above
ENTER_SCALE and popupRadius, while keeping only the exit-curve paragraphs above
SHEET_EXIT_EASE; do not alter the comment text.
In `@packages/ui/src/mosaic/components/dialog/dialog.test.tsx`:
- Around line 484-497: Remove the duplicated test case “still forwards the popup
ref alongside the observing one” from the Dialog tests, keeping the existing
“forwards the ref to the popup element” coverage unchanged.
- Around line 158-175: Remove the unused local addEmailTrigger declaration and
retain a single module-scope trigger definition, renaming or reusing
addEmailTriggerShared as needed. Update both nested Mosaic Dialogs test blocks
to reference that shared definition.
In `@packages/ui/src/mosaic/components/dialog/dialog.tsx`:
- Around line 260-281: Annotate the exported function components Dialog, Root,
and DialogContent with explicit return types, using the appropriate
JSX/component return type already supported by the project. Keep their existing
props and rendering behavior unchanged.
In `@packages/ui/src/mosaic/tokens.stylex.ts`:
- Around line 316-323: Reduce the comment documenting --cl-ease-enter to one
concise sentence describing its purpose, removing consumer-specific usage policy
and Dialog guidance. Keep the token definition unchanged and do not add
replacement documentation here.
🪄 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: 7d03f1c7-b1c8-4862-8017-3416840318ad
📒 Files selected for processing (21)
.changeset/olive-doors-tell.mdpackages/headless/src/primitives/dialog/README.mdpackages/headless/src/primitives/dialog/dialog-backdrop.tsxpackages/headless/src/primitives/dialog/dialog-context.tspackages/headless/src/primitives/dialog/dialog-popup.tsxpackages/headless/src/primitives/dialog/dialog-root.tsxpackages/headless/src/primitives/dialog/dialog-viewport.tsxpackages/headless/src/primitives/drawer/drawer-context.tspackages/swingset/src/stories/dialog.component.mdxpackages/swingset/src/stories/dialog.component.stories.tsxpackages/ui/src/mosaic/components/dialog.tsxpackages/ui/src/mosaic/components/dialog/dialog.styles.tspackages/ui/src/mosaic/components/dialog/dialog.test.tsxpackages/ui/src/mosaic/components/dialog/dialog.tsxpackages/ui/src/mosaic/components/dialog/index.tspackages/ui/src/mosaic/components/dialog/keyboard-inset.tspackages/ui/src/mosaic/components/popover/popover.tsxpackages/ui/src/mosaic/hooks/useAccessibleNameWarning.tspackages/ui/src/mosaic/primitives/dialog.tsxpackages/ui/src/mosaic/styles/index.tspackages/ui/src/mosaic/tokens.stylex.ts
🔗 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)
💤 Files with no reviewable changes (3)
- packages/headless/src/primitives/drawer/drawer-context.ts
- packages/ui/src/mosaic/components/dialog.tsx
- packages/ui/src/mosaic/primitives/dialog.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/headless/src/primitives/dialog/dialog-backdrop.tsx
- packages/headless/src/primitives/dialog/dialog-viewport.tsx
- packages/ui/src/mosaic/components/dialog/keyboard-inset.ts
- packages/ui/src/mosaic/styles/index.ts
| --- | ||
| --- |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Restore a changeset entry for the @clerk/ui Dialog API change.
This changeset is now empty, so the release carries no version bump and no changelog entry. packages/ui is published and pinned by external consumers — clerk/dashboard pins @clerk/ui at 1.7.0, and both clerk/dashboard and clerk/clerk load @clerk/ui@1 from the CDN.
This PR replaces the Mosaic Dialog modules, moves size from the popup to Dialog.Root, and drops sx in favour of .cl-dialog-* classes. Consumers need a changelog entry and an appropriate semver bump for that.
Choose one path:
- Keep deprecated compatibility shims for the removed Dialog props and publish a minor release with a changeset describing the new API.
- Publish a major release with a changeset that documents the migration from
sxand from popup-levelsize.
As per coding guidelines, "Maintain backward compatibility in packages/clerk-js and packages/ui with SDK versions already in the wild" and "Use Changesets for version management and changelogs".
🤖 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 @.changeset/olive-doors-tell.md around lines 1 - 2, Restore the changeset
entry in the empty `.changeset/olive-doors-tell.md` file for the `@clerk/ui`
Dialog API change. Choose either a minor bump with deprecated compatibility
shims for removed props, or a major bump documenting migration from `sx` and
popup-level `size`; include the corresponding changelog details and valid
Changesets frontmatter.
Sources: Coding guidelines, Linked repositories
| <Dialog.CloseButton /> | ||
| <Dialog.Title render={<Heading size='sm' />}>{title}</Dialog.Title> | ||
| <Dialog.Description render={<Text />}>{description}</Dialog.Description> | ||
| <Input placeholder={placeholder} /> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Two Input elements in the stories have no accessible name. Both rely on placeholder alone. A placeholder is not an accessible name, so screen-reader users hear an unnamed text field. These stories are embedded in dialog.component.mdx, so the pattern gets copied by consumers.
packages/swingset/src/stories/dialog.component.stories.tsx#L137-L137: addaria-label={title}to theInputinsideAddValueDialog.packages/swingset/src/stories/dialog.component.stories.tsx#L364-L364: addaria-label='Email address'to theInputinsideCardSurface.
As per coding guidelines, "Implement proper form labels in React components".
📍 Affects 1 file
packages/swingset/src/stories/dialog.component.stories.tsx#L137-L137(this comment)packages/swingset/src/stories/dialog.component.stories.tsx#L364-L364
🤖 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/swingset/src/stories/dialog.component.stories.tsx` at line 137,
Update the Input in AddValueDialog at
packages/swingset/src/stories/dialog.component.stories.tsx:137-137 to use title
as its aria-label, and update the Input in CardSurface at
packages/swingset/src/stories/dialog.component.stories.tsx:364-364 to use “Email
address” as its aria-label; retain the existing placeholders.
Source: Coding guidelines
While a dialog is open, tints the browser's own chrome so it reads as one
continuous surface rather than a dimmed page inside undimmed furniture. Two
surfaces move together: `<meta name="theme-color">` for the address bar and
toolbar, and `<body>`'s background for the canvas outside the layout viewport —
the overscroll gutter and the strip revealed as the address bar collapses,
neither of which a `position: fixed` scrim covers.
It ships no colour of its own. The target is derived — the backdrop's computed
background composited over whatever the page already had — so it stays correct
if a consumer retunes the scrim. The meta is prepended rather than mutated, so
it overrides the app's own (including framework-managed tags like Next's
`viewport.themeColor`) and removing it restores theirs with no bookkeeping.
Refcounted across stacked dialogs, reverted exactly on close, and inert wherever
`theme-color` is ignored. Opt out with `syncBrowserChrome={false}`.
Split out of #9388: `animate` writes `document.body.style.backgroundColor` on
every frame, which is a full-viewport repaint per frame during the entrance.
That wants profiling on real mobile hardware before it ships. If it measures
badly, the fix is to set the body colour once at the target rather than
animating it — it is only ever visible outside the layout viewport, so nobody
sees it mid-entrance.
Moves the dialog off the Emotion slot-recipe engine onto StyleX, leaving `tabs` as the last component on the old path, and reworks its sizing, motion and mobile behaviour on top of that. `size` becomes three named surfaces — `prompt`, `card`, `panel` — and moves to `Dialog.Root`, since the backdrop reads it too. The gap to the screen edge is a fixed inset at three breakpoints rather than a percentage, which is what makes the surround an even frame. A `panel` clips and carries no padding, so its scroll region is composed inside it from the ScrollArea atoms; that keeps the close button anchored and makes a sidebar a plain flex row. Below 48rem a `prompt` becomes a bottom sheet, and `Dialog.Viewport` measures the on-screen keyboard so the sheet rises above it while a card re-centres and a panel shrinks. The chrome of a mobile browser is tinted to match the scrim, derived from the backdrop rather than shipped as a colour, refcounted across stacked dialogs and reverting exactly. Adds `Dialog.CloseButton`, `data-nested` for stacked scrims, and `--cl-dialog-origin` so a dialog scales out of whatever opened it. Also fixes a transition that never ran: it was keyed to a `data-cl-starting-style` attribute the headless layer does not emit, so dialogs appeared with no animation at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Drop the trigger-origin open animation in favour of a plain centre scale, deleting the headless `useDialogOrigin` hook and `--cl-dialog-origin` with it. Motion. Add a `--cl-ease-enter` token — a decelerate curve that lands on target rather than carrying ~2% past it like `--cl-ease-default` — and take it for every dialog entrance. The backdrop fades a step faster than the popup, so the scrim answers the gesture and the surface arrives into an already-dimmed page. The mobile sheet now fades over the full length of its slide instead of holding opaque. Enter scale 0.98 -> 0.94. Fix a `prompt` that never scaled at all. Its transform was a single media-scoped rule with no resting declaration, leaving the transition with `transform: none` as its other endpoint; it now mirrors `card` exactly. Surface. Match the popup shadow to Menu's, which restores a visible hairline on dark surfaces. A `card` no longer paints itself: it takes its surface from a `Card` rendered AS the popup, so one element both paints and animates and the radius counter-scale keeps landing on the corners you can see. Scrolling. A dialog taller than the screen now scrolls, and how follows from its size rather than a prop. `panel` keeps a pinned viewport and scrolls inside; `prompt` and `card` grow with their content, so the dialog moves within the viewport and keeps its inset at both ends. `Dialog.Viewport` gains `data-size`. Spacing. `prompt` padding to 1rem; phone-band inset to 1.25rem block, 1rem inline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Draw a real border on the popup under `forced-colors: active`. The mode discards `box-shadow` and the scrim alike, so the dialog otherwise floats edgeless over the page with nothing to separate it. A border rather than an outline, since the popup clears its outline deliberately — `FloatingFocusManager` focuses it when it holds no tabbable content — and the two would collide. Wrap long unbroken strings on the popup, matching `Popover`. A dialog holds prose it did not author, and an email address or an API key would otherwise push past the size's width clamp. Warn in development when a dialog has no accessible name, extracting `Popover`'s check into a shared `useAccessibleNameWarning` and pointing both at it. The check now RESOLVES `aria-labelledby` rather than testing for its presence: `Dialog` emits the attribute unconditionally, so with no `Dialog.Title` it references an id that is not in the document, which a presence check waves through while naming the dialog no better than having no attribute at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rigin A `panel` fades in and out rather than appearing instantly. It still does not scale — the absolute travel of a scale is a proportion of the element's own size, so the 6% that reads as emergence on a card arrives as a zoom on a surface that is most of the viewport. The fade is load-bearing on the popup, not only decoration: the headless transition watches the POPUP's animations to decide when to unmount and takes the whole subtree at once, so with nothing running there the scrim was pulled before it could fade. That is what previously forced the backdrop to be instant too. Also removes documentation for `--cl-dialog-origin` and the trigger-origin open, which no longer exist, and corrects `Item.Title` to `Item.Label` in the dialog stories after that rename.
…utton` Wraps `Dialog.CloseButton` in a positioned `span` instead of threading StyleX atoms into `Button`, and drops the `xstyle` prop that existed only to make that work. `Button`'s touch target sets `position` inside a media query, which compiles to a class an outside caller cannot dedupe against — so the override had to happen inside the button's own `stylex.props` call. A wrapper owns the positioning outright and leaves `Button`'s public surface alone.
Removes the `theme-color` / body-background sync and its `syncBrowserChrome` opt-out from this PR, so the StyleX rebuild can land without waiting on it. It animates `document.body.style.backgroundColor` per frame, which is a full-viewport repaint on every frame of the entrance, and rewrites the `theme-color` meta just as often. That wants profiling on real mobile hardware, which the rest of this PR does not — so it moves to a branch of its own rather than holding this one open.
`Dialog.Root` does not take a `handle` — that arrives with the composition APIs in #9419, not here. `data-size` lands on the Viewport as well as the Popup: `themeProps` emits `data-<axis>` for every variant it is handed, and `size` is passed to both.
727d933 to
040a388
Compare
While a dialog is open, tints the browser's own chrome so it reads as one
continuous surface rather than a dimmed page inside undimmed furniture. Two
surfaces move together: `<meta name="theme-color">` for the address bar and
toolbar, and `<body>`'s background for the canvas outside the layout viewport —
the overscroll gutter and the strip revealed as the address bar collapses,
neither of which a `position: fixed` scrim covers.
It ships no colour of its own. The target is derived — the backdrop's computed
background composited over whatever the page already had — so it stays correct
if a consumer retunes the scrim. The meta is prepended rather than mutated, so
it overrides the app's own (including framework-managed tags like Next's
`viewport.themeColor`) and removing it restores theirs with no bookkeeping.
Refcounted across stacked dialogs, reverted exactly on close, and inert wherever
`theme-color` is ignored. Opt out with `syncBrowserChrome={false}`.
Split out of #9388: `animate` writes `document.body.style.backgroundColor` on
every frame, which is a full-viewport repaint per frame during the entrance.
That wants profiling on real mobile hardware before it ships. If it measures
badly, the fix is to set the body colour once at the target rather than
animating it — it is only ever visible outside the layout viewport, so nobody
sees it mid-entrance.
…split `addEmailTriggerShared` was only ever rendered by the browser-chrome tests, which moved to their own branch — `eslint --quiet` fails the package on the unused binding. It travels with those tests rather than staying behind here.
While a dialog is open, tints the browser's own chrome so it reads as one
continuous surface rather than a dimmed page inside undimmed furniture. Two
surfaces move together: `<meta name="theme-color">` for the address bar and
toolbar, and `<body>`'s background for the canvas outside the layout viewport —
the overscroll gutter and the strip revealed as the address bar collapses,
neither of which a `position: fixed` scrim covers.
It ships no colour of its own. The target is derived — the backdrop's computed
background composited over whatever the page already had — so it stays correct
if a consumer retunes the scrim. The meta is prepended rather than mutated, so
it overrides the app's own (including framework-managed tags like Next's
`viewport.themeColor`) and removing it restores theirs with no bookkeeping.
Refcounted across stacked dialogs, reverted exactly on close, and inert wherever
`theme-color` is ignored. Opt out with `syncBrowserChrome={false}`.
Split out of #9388: `animate` writes `document.body.style.backgroundColor` on
every frame, which is a full-viewport repaint per frame during the entrance.
That wants profiling on real mobile hardware before it ships. If it measures
badly, the fix is to set the body colour once at the target rather than
animating it — it is only ever visible outside the layout viewport, so nobody
sees it mid-entrance.
While a dialog is open, tints the browser's own chrome so it reads as one
continuous surface rather than a dimmed page inside undimmed furniture. Two
surfaces move together: `<meta name="theme-color">` for the address bar and
toolbar, and `<body>`'s background for the canvas outside the layout viewport —
the overscroll gutter and the strip revealed as the address bar collapses,
neither of which a `position: fixed` scrim covers.
It ships no colour of its own. The target is derived — the backdrop's computed
background composited over whatever the page already had — so it stays correct
if a consumer retunes the scrim. The meta is prepended rather than mutated, so
it overrides the app's own (including framework-managed tags like Next's
`viewport.themeColor`) and removing it restores theirs with no bookkeeping.
Refcounted across stacked dialogs, reverted exactly on close, and inert wherever
`theme-color` is ignored. Opt out with `syncBrowserChrome={false}`.
Split out of #9388: `animate` writes `document.body.style.backgroundColor` on
every frame, which is a full-viewport repaint per frame during the entrance.
That wants profiling on real mobile hardware before it ships. If it measures
badly, the fix is to set the body colour once at the target rather than
animating it — it is only ever visible outside the layout viewport, so nobody
sees it mid-entrance.
While a dialog is open, tints the browser's own chrome so it reads as one
continuous surface rather than a dimmed page inside undimmed furniture. Two
surfaces move together: `<meta name="theme-color">` for the address bar and
toolbar, and `<body>`'s background for the canvas outside the layout viewport —
the overscroll gutter and the strip revealed as the address bar collapses,
neither of which a `position: fixed` scrim covers.
It ships no colour of its own. The target is derived — the backdrop's computed
background composited over whatever the page already had — so it stays correct
if a consumer retunes the scrim. The meta is prepended rather than mutated, so
it overrides the app's own (including framework-managed tags like Next's
`viewport.themeColor`) and removing it restores theirs with no bookkeeping.
Refcounted across stacked dialogs, reverted exactly on close, and inert wherever
`theme-color` is ignored. Opt out with `syncBrowserChrome={false}`.
Split out of #9388: `animate` writes `document.body.style.backgroundColor` on
every frame, which is a full-viewport repaint per frame during the entrance.
That wants profiling on real mobile hardware before it ships. If it measures
badly, the fix is to set the body colour once at the target rather than
animating it — it is only ever visible outside the layout viewport, so nobody
sees it mid-entrance.
2 participants
Description
Rebuilds the Mosaic
Dialogon StyleX, leavingtabsas the last component on the Emotion slot-recipe path. Composition APIs are stacked on top in #9419. Thetheme-color/ browser-chrome tint is split out ontomax/dialog-browser-chrome, to PR once it has been profiled on real mobile hardware.@clerk/ui/styles.css; style via.cl-dialog-*slot classes or per-partclassName/style, replacingsx.md/lgbecomeprompt/card/panel, andsizemoves toDialog.Rootso the backdrop can read it too.cardsurface — contributes width and motion only; render the popup as the card (<Dialog.Popup render={<Card.Root />}>) for its background and padding.1.25rem→2remat48rem→3remat90rem) instead of a percentage, so the surround reads as an even frame.panelscrolls inside,promptandcardscroll outside and keep their inset at both ends.panelclips and carries no padding; build the scroll region inside it withscrollAreaRoot/scrollAreaViewport().48remapromptbecomes a bottom sheet;Dialog.Viewportmeasures the on-screen keyboard and pads for it.promptandcardalso scale from their own centre,paneldoes not. New--cl-ease-entertoken for entrances, and corner radius no longer distorts during the scale. Also fixes a transition that never ran (keyed to adata-cl-starting-styleattribute the headless layer does not emit).data-nestedpaints a lighter scrim so backdrops don't compound into an opaque wall.Dialog.CloseButton— corner dismiss affordance, positioned by a wrapper soButton's own surface stays untouched.Dialog.Closeis unchanged.forced-colors: active, long unbroken strings wrap, and a dev warning when a dialog has no accessible name (shared withPopover, now resolvingaria-labelledbyto a real element).Follow ups
AlertDialogpreset, and close-confirmation built on top of it.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change