From 7cfb40113cf37ddc0b6e0842b22ca7dea7b48728 Mon Sep 17 00:00:00 2001 From: Max Yinger Date: Thu, 13 Aug 2026 09:33:18 -0600 Subject: [PATCH 1/2] feat(ui): dialog close confirmation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A dialog holding unsaved work should ask before discarding it. Three pieces, each with one job: `createConfirmHandle()` links a question to its answer, `useConfirmedClose` guards the close path, and `` is the dialog, rendered inside the one it guards so the two share a floating tree — escape ordering, the stacking styles and the refcounted scroll lock all read that tree, and a globally mounted confirmation would break every one of them. `show()` returns a promise resolving to the answer, so a confirmation reads as `if (await confirm.show({…}))` rather than as a pair of state variables and a callback. Calling it while one is already showing returns the IN-FLIGHT promise instead of opening a second: holding Escape against a guarded dialog would otherwise stack a confirmation per keypress. The veto is the absence of a commit. `useConfirmedClose` wraps the consumer's own `onOpenChange`, so it covers every close the dialog owns — Escape, outside press, `Dialog.CloseButton`, `Dialog.Close`, and the `close` the wrapper hands its children all funnel through it. A button wired to the consumer's own `setOpen(false)` never reaches the dialog and so bypasses the question; that is inherent, and both the hook's JSDoc and the docs page say so. Two ordering details that are load-bearing. The action settles `true` before closing, and `settle` is a no-op once a question is answered, so the close that follows cannot overwrite the answer with `false`. And the hook reads `when` and `onOpenChange` through a ref, so the callback identity is stable across the keystrokes of the very form whose dirtiness `when` reports on. Headless gains `handle.open(payload)` — the programmatic counterpart of a trigger's payload, which is how the confirmation's own text reaches it. The root holds it in a ref as well as in state: the registry lookup that runs once the dialog is open resolves a trigger-less open to `undefined`, and would otherwise blank the dialog a commit after it was filled. --- .changeset/dialog-close-confirmation.md | 2 + .../headless/src/primitives/dialog/README.md | 11 + .../src/primitives/dialog/dialog-handle.ts | 24 +- .../src/primitives/dialog/dialog-root.tsx | 20 +- .../src/primitives/dialog/dialog.test.tsx | 57 ++++ .../src/stories/alert-dialog.component.mdx | 60 ++++ .../alert-dialog.component.stories.tsx | 117 +++---- .../src/stories/dialog.component.stories.tsx | 5 +- .../components/alert-dialog/alert-dialog.tsx | 70 +++++ .../components/alert-dialog/confirm-handle.ts | 87 ++++++ .../components/alert-dialog/confirm.test.tsx | 291 ++++++++++++++++++ .../mosaic/components/alert-dialog/index.ts | 5 + .../alert-dialog/use-confirmed-close.ts | 76 +++++ packages/ui/src/mosaic/styles/index.ts | 6 +- 14 files changed, 762 insertions(+), 69 deletions(-) create mode 100644 .changeset/dialog-close-confirmation.md create mode 100644 packages/ui/src/mosaic/components/alert-dialog/confirm-handle.ts create mode 100644 packages/ui/src/mosaic/components/alert-dialog/confirm.test.tsx create mode 100644 packages/ui/src/mosaic/components/alert-dialog/use-confirmed-close.ts diff --git a/.changeset/dialog-close-confirmation.md b/.changeset/dialog-close-confirmation.md new file mode 100644 index 00000000000..a845151cc84 --- /dev/null +++ b/.changeset/dialog-close-confirmation.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/packages/headless/src/primitives/dialog/README.md b/packages/headless/src/primitives/dialog/README.md index 1ef84c34301..c566d5bba4a 100644 --- a/packages/headless/src/primitives/dialog/README.md +++ b/packages/headless/src/primitives/dialog/README.md @@ -78,6 +78,17 @@ const detail = Dialog.createHandle<{ name: string }>(); ``` +An open with no trigger behind it can supply the payload directly: `handle.open(payload)` is the +programmatic counterpart, for a dialog raised by something that happened rather than by an element +— a confirmation that has to say what it is asking. A trigger-driven open supersedes it, since a +trigger names its own payload. + +```tsx +const confirmation = Dialog.createHandle<{ question: string }>(); + +confirmation.open({ question: 'Discard changes?' }); +``` + In controlled mode, track which trigger is active with `triggerId` — `onOpenChange`'s second argument reports the trigger behind each change: diff --git a/packages/headless/src/primitives/dialog/dialog-handle.ts b/packages/headless/src/primitives/dialog/dialog-handle.ts index 2ce8e89e9aa..739cec4364f 100644 --- a/packages/headless/src/primitives/dialog/dialog-handle.ts +++ b/packages/headless/src/primitives/dialog/dialog-handle.ts @@ -20,10 +20,10 @@ export interface DialogTriggerRegistration { * requests made with no root attached are ignored, matching Base UI. * @internal */ -export interface DialogRootController { +export interface DialogRootController { openFromTrigger: (id: string, event: Event) => void; closeFromTrigger: (id: string, event: Event) => void; - setOpen: (open: boolean) => void; + setOpen: (open: boolean, payload?: Payload) => void; } /** The slice of root state a trigger renders from: its `data-open` / ARIA wiring. */ @@ -45,8 +45,16 @@ const CLOSED_STATE: DialogHandleState = { open: false, triggerId: null, popupId: * lets a `DialogHandle` flow into contexts typed `DialogHandle`. */ export interface DialogHandle { - /** Opens the attached root. Ignored while no root is mounted. */ - open(): void; + /** + * Opens the attached root. Ignored while no root is mounted. + * + * The optional `payload` is the programmatic counterpart of a trigger's: it reaches the root's + * children-as-function as `{ payload }`, so an imperative open can carry the content the dialog + * is about — what a confirmation is asking, which record is being deleted — without the caller + * holding a second piece of state alongside `open`. A trigger-driven open supersedes it, since + * a trigger names its own payload. + */ + open(payload?: Payload): void; /** Closes the attached root. Ignored while no root is mounted. */ close(): void; /** Whether the attached root is open. `false` while no root is mounted. */ @@ -58,7 +66,7 @@ export interface DialogHandle { /** @internal */ getFirstTrigger(): DialogTriggerRegistration | undefined; /** @internal */ - setRoot(controller: DialogRootController): () => void; + setRoot(controller: DialogRootController): () => void; /** @internal */ requestOpen(id: string, event: Event): void; /** @internal */ @@ -79,14 +87,14 @@ export interface DialogHandle { export function createDialogHandle(): DialogHandle { const triggers = new Map>(); const listeners = new Set<() => void>(); - let root: DialogRootController | null = null; + let root: DialogRootController | null = null; let state = CLOSED_STATE; const notify = () => listeners.forEach(listener => listener()); return { - open() { - root?.setOpen(true); + open(payload) { + root?.setOpen(true, payload); }, close() { root?.setOpen(false); diff --git a/packages/headless/src/primitives/dialog/dialog-root.tsx b/packages/headless/src/primitives/dialog/dialog-root.tsx index 66bb2fb0f72..7ae9f947531 100644 --- a/packages/headless/src/primitives/dialog/dialog-root.tsx +++ b/packages/headless/src/primitives/dialog/dialog-root.tsx @@ -98,6 +98,10 @@ function DialogInner(props: DialogProps & { isNested: boolean // consumed by the floating `onOpenChange` the request funnels into. const pendingDetailsRef = useRef(null); + // The payload of the most recent programmatic `handle.open(payload)`, kept so the registry + // lookup below has something to fall back to when no trigger is involved. + const directPayloadRef = useRef(undefined); + // Every open/close funnels through `floatingContext.onOpenChange` — trigger activations, // dismissals, and programmatic `setOpen` alike. floating-ui emits its `openchange` event // synchronously before invoking this callback, which is what lets listeners (`useReturnFocus`, @@ -119,6 +123,9 @@ function DialogInner(props: DialogProps & { isNested: boolean openFromTrigger: (id, event) => { const registration = store.getTrigger(id); setActiveTriggerId(id); + // A trigger names its own payload, so it supersedes anything a previous programmatic + // open supplied — otherwise the stale one would resurface through the effect below. + directPayloadRef.current = undefined; setActivePayload(registration?.getPayload()); if (registration) { refs.setReference(registration.element); @@ -131,7 +138,14 @@ function DialogInner(props: DialogProps & { isNested: boolean pendingDetailsRef.current = { trigger: registration?.element ?? null, triggerId: id, event }; floatingContext.onOpenChange(false, event, 'click'); }, - setOpen: nextOpen => floatingContext.onOpenChange(nextOpen), + setOpen: (nextOpen, payload) => { + // Held in a ref as well as in state because the payload effect below re-runs on `open` + // and would otherwise resolve a trigger-less open to `undefined`, wiping this a commit + // after it was set. + directPayloadRef.current = payload; + setActivePayload(payload); + floatingContext.onOpenChange(nextOpen); + }, }); // `floatingContext` is rebuilt on open/element changes; re-registering is an idempotent swap. }, [store, refs, floatingContext, setActiveTriggerId]); @@ -157,7 +171,9 @@ function DialogInner(props: DialogProps & { isNested: boolean // 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)?.getPayload() : undefined); + setActivePayload( + activeTriggerId != null ? store.getTrigger(activeTriggerId)?.getPayload() : directPayloadRef.current, + ); } }, [store, open, activeTriggerId]); diff --git a/packages/headless/src/primitives/dialog/dialog.test.tsx b/packages/headless/src/primitives/dialog/dialog.test.tsx index 5a65d30c165..58839f9f106 100644 --- a/packages/headless/src/primitives/dialog/dialog.test.tsx +++ b/packages/headless/src/primitives/dialog/dialog.test.tsx @@ -528,6 +528,63 @@ describe('Dialog', () => { expect(screen.getByRole('dialog', { name: 'payload-b' })).toBeInTheDocument(); }); + + // The programmatic counterpart of a trigger's payload, for an open that no element initiated — + // a confirmation raised by a close request, say, which has to say what it is asking. + describe('handle.open(payload)', () => { + function renderDetached() { + const handle = Dialog.createHandle(); + render( + + {({ payload }) => ( + <> + Open A + + + + {payload ?? 'no payload'} + + + + + )} + , + ); + return handle; + } + + it('delivers it to the children render function', () => { + const handle = renderDetached(); + + act(() => handle.open('from-handle')); + + expect(screen.getByRole('dialog', { name: 'from-handle' })).toBeInTheDocument(); + }); + + it('survives the registry lookup that runs once the dialog is open', async () => { + const handle = renderDetached(); + + act(() => handle.open('from-handle')); + // The lookup effect re-runs on `open`; without a fallback it would resolve to `undefined` + // a commit later and blank the dialog. + await act(async () => { + await Promise.resolve(); + }); + + expect(screen.getByRole('dialog', { name: 'from-handle' })).toBeInTheDocument(); + }); + + it('is superseded by a trigger, which names its own payload', async () => { + const user = userEvent.setup(); + const handle = renderDetached(); + + act(() => handle.open('from-handle')); + act(() => handle.close()); + await user.click(screen.getByRole('button', { name: 'Open A' })); + + expect(screen.getByRole('dialog', { name: 'no payload' })).toBeInTheDocument(); + }); + }); }); describe('initialFocus', () => { diff --git a/packages/swingset/src/stories/alert-dialog.component.mdx b/packages/swingset/src/stories/alert-dialog.component.mdx index b1cbfe2c4cb..3a25e811c76 100644 --- a/packages/swingset/src/stories/alert-dialog.component.mdx +++ b/packages/swingset/src/stories/alert-dialog.component.mdx @@ -79,6 +79,65 @@ the same reason: a corner X is a way out without answering. Every close request — Escape or `AlertDialog.Close` — routes through `onOpenChange`, so a controlled consumer can decline one by not committing the state. +## Confirming a close + +A dialog holding unsaved work should ask before discarding it. That is three pieces: a handle, a +hook that guards the close, and the confirmation itself. + +```tsx +import { AlertDialog, createConfirmHandle, useConfirmedClose } from '@clerk/ui/mosaic/components/alert-dialog'; + +const confirm = React.useMemo(() => createConfirmHandle(), []); + +const onOpenChange = useConfirmedClose({ + handle: confirm, + when: () => value !== '', + onOpenChange: setOpen, + confirm: { + title: 'Discard changes?', + description: 'You have not finished adding this address.', + actionLabel: 'Discard', + cancelLabel: 'Keep editing', + destructive: true, + }, +}); + + + {/* … */} + + +``` + +**Render `AlertDialog.Confirm` inside the dialog it guards** — anywhere in its children. That is +what puts the two in one floating tree, and escape ordering, the stacking styles and the refcounted +scroll lock all read that tree. A confirmation mounted app-globally would be a sibling of the dialog +rather than a child of it, and all three would break. + +**The guarded dialog must be controlled.** A veto is the absence of a commit, and an uncontrolled +dialog has already committed by the time `onOpenChange` runs. + +**What it covers is every close the dialog owns**: Escape, an outside press where `closedBy` allows +one, `Dialog.CloseButton`, `Dialog.Close`, and the `close` the `Dialog` wrapper hands its children. +A button wired to your own `setOpen(false)` never reaches the dialog, so it bypasses the question +silently — route those through `Dialog.Close`. + +`when()` is evaluated at each close request, so a close that no longer needs guarding (the form has +just been submitted, the field cleared) passes straight through. + +### Asking without a close + +`show()` is the same confirmation, awaited directly — for a decision that is not about closing: + +```tsx +if (await confirm.show({ title: 'Delete this key?', description: 'Applications using it stop working.' })) { + await deleteKey(); +} +``` + +It resolves `true` for the action and `false` for cancel or any dismissal. Calling it while a +confirmation is already showing returns the in-flight promise rather than opening a second one, so +repeated close requests ask once. + ## Parts | Part | Slot | Description | @@ -93,6 +152,7 @@ consumer can decline one by not committing the state. | `AlertDialog.Description` | — | Description; wired to the popup's `aria-describedby`. Required. | | `AlertDialog.Close` | — | Dismisses the alert; unstyled, accepts a `render` prop. | | `AlertDialog.Actions` | `alert-dialog-actions` | The response row. Cancel first. | +| `AlertDialog.Confirm` | `dialog-popup` | A whole confirmation rendered from a `show()` call. See below. | Every part except `Popup` and `Actions` is `Dialog`'s own component, not a wrapper around it — one implementation, so the two cannot drift. `Title` and `Description` are unstyled passthroughs from the diff --git a/packages/swingset/src/stories/alert-dialog.component.stories.tsx b/packages/swingset/src/stories/alert-dialog.component.stories.tsx index d50b499f0d6..dc246efa812 100644 --- a/packages/swingset/src/stories/alert-dialog.component.stories.tsx +++ b/packages/swingset/src/stories/alert-dialog.component.stories.tsx @@ -1,6 +1,6 @@ /** @jsxImportSource @emotion/react */ import type { RenderProps } from '@clerk/headless/utils'; -import { AlertDialog } from '@clerk/ui/mosaic/components/alert-dialog'; +import { AlertDialog, createConfirmHandle, useConfirmedClose } from '@clerk/ui/mosaic/components/alert-dialog'; import { Button } from '@clerk/ui/mosaic/components/button'; import { Dialog } from '@clerk/ui/mosaic/components/dialog'; import { Heading } from '@clerk/ui/mosaic/components/heading'; @@ -62,75 +62,80 @@ const addEmailTrigger = (props: RenderProps) => - + {({ close }) => ( + <> + + }>Add email address + }> + You will need to verify this address before it can be used. + + setValue(event.target.value)} + /> +
+ }>Cancel + {/* Adding is the one close that must NOT be questioned, so it clears the field the + guard reads before closing. */} + +
- {/* `finalFocus` puts the caret back in the field. Without it there is nowhere to return to — - this alert is raised by the veto rather than by a trigger — so keeping editing would - leave focus on the body, at the top of the page rather than where the work was. */} - - }>Discard changes? - }> - You have not finished adding this address. It will not be saved. - - - }>Keep editing - - - + + + )} ); } diff --git a/packages/swingset/src/stories/dialog.component.stories.tsx b/packages/swingset/src/stories/dialog.component.stories.tsx index 0456ea80903..1ae1e21e39f 100644 --- a/packages/swingset/src/stories/dialog.component.stories.tsx +++ b/packages/swingset/src/stories/dialog.component.stories.tsx @@ -110,8 +110,9 @@ const sectionHeader = { * * With `confirmDiscard`, closing it while the field holds anything opens a confirmation stacked on * top rather than closing: `panel -> prompt -> prompt`, and the veto is nothing more than a - * controlled `open` whose `onOpenChange` declines to commit. Hand-rolled here on purpose — it is - * what the `AlertDialog` and close-confirmation work is meant to replace. + * controlled `open` whose `onOpenChange` declines to commit. Hand-rolled here on purpose, to show + * that a veto needs no machinery; `AlertDialog`'s `useConfirmedClose` is the same thing packaged, + * and its page has the composed version. */ function AddValueDialog({ trigger, diff --git a/packages/ui/src/mosaic/components/alert-dialog/alert-dialog.tsx b/packages/ui/src/mosaic/components/alert-dialog/alert-dialog.tsx index 31ade3e65b9..e7b53c55357 100644 --- a/packages/ui/src/mosaic/components/alert-dialog/alert-dialog.tsx +++ b/packages/ui/src/mosaic/components/alert-dialog/alert-dialog.tsx @@ -1,3 +1,5 @@ +import type { DialogFocusTarget } from '@clerk/headless/dialog'; +import { useDialogContext } from '@clerk/headless/dialog'; import { useRender } from '@clerk/headless/utils'; import * as stylex from '@stylexjs/stylex'; import type { ReactNode } from 'react'; @@ -6,6 +8,7 @@ import React from 'react'; import { useAccessibleDescriptionWarning } from '../../hooks/useAccessibleDescriptionWarning'; import type { MosaicComponentProps } from '../../props'; import { mergeStyleProps, themeProps } from '../../props'; +import { Button } from '../button'; import type { DialogBackdropProps, DialogCloseProps, @@ -20,8 +23,11 @@ import { Dialog } from '../dialog'; // Deep import: the part-name context and the content resolver are how one Mosaic component wraps // another and are deliberately absent from `../dialog`'s public surface. import { DialogContent, DialogPartNameContext } from '../dialog/dialog'; +import { Heading } from '../heading'; import { reset } from '../reset.styles'; +import { Text } from '../text'; import { styles } from './alert-dialog.styles'; +import { type ConfirmHandle, createConfirmHandle } from './confirm-handle'; /** * An alert dialog is a `Dialog` with three decisions already made, so the props that would make @@ -203,6 +209,67 @@ export function AlertDialog({ ); } +export interface AlertDialogConfirmProps { + /** Shared with the `show()` call, or with `useConfirmedClose`, that raises this confirmation. */ + handle: ConfirmHandle; + /** + * Where focus goes when the confirmation closes. Worth passing: the confirmation has no trigger, + * so by default there is nothing for focus to return to. Point it at the field the question was + * about and declining puts the caret back in it. + */ + finalFocus?: DialogFocusTarget; +} + +/** + * The dialog half of {@link createConfirmHandle} — an alert dialog rendered from whatever the + * `show()` call asked, and closed by answering it. + * + * Render it INSIDE the dialog it guards (anywhere in its children; outside its `Portal` is fine). + * That is what puts the two in one floating tree, which is what escape ordering, the stacking + * styles and the refcounted scroll lock all read. + */ +function Confirm({ handle, finalFocus }: AlertDialogConfirmProps) { + return ( + { + // Every close that is not the action lands here — cancel, Escape, a programmatic close — + // and they all mean no. The action settles `true` BEFORE closing, and `settle` is a no-op + // once the question is answered, so this cannot overwrite it. + if (!open) { + handle.settle(false); + } + }} + > + {({ payload }) => + payload ? ( + + + + + }>{payload.title} + }>{payload.description} + + }>{payload.cancelLabel ?? 'Cancel'} + + + + + + ) : null + } + + ); +} + /** * Compound parts. The ones an alert dialog does not change are `Dialog`'s own — same components, * not wrappers around them, so there is one implementation of each and no way for the two to @@ -220,3 +287,6 @@ AlertDialog.Title = Dialog.Title; AlertDialog.Description = Dialog.Description; AlertDialog.Close = Dialog.Close; AlertDialog.Actions = Actions; +AlertDialog.Confirm = Confirm; +/** Creates the handle pairing an awaitable `show()` with an ``. */ +AlertDialog.createConfirmHandle = createConfirmHandle; diff --git a/packages/ui/src/mosaic/components/alert-dialog/confirm-handle.ts b/packages/ui/src/mosaic/components/alert-dialog/confirm-handle.ts new file mode 100644 index 00000000000..d3e09c1b937 --- /dev/null +++ b/packages/ui/src/mosaic/components/alert-dialog/confirm-handle.ts @@ -0,0 +1,87 @@ +import { Dialog as Primitive, type DialogHandle } from '@clerk/headless/dialog'; +import type { ReactNode } from 'react'; + +/** What a confirmation asks. Delivered to `` as the dialog's payload. */ +export interface ConfirmOptions { + title: ReactNode; + description: ReactNode; + /** Label on the confirming button. @default 'Confirm' */ + actionLabel?: ReactNode; + /** Label on the declining button. @default 'Cancel' */ + cancelLabel?: ReactNode; + /** Colours the action as destructive, for a confirmation that discards or deletes. */ + destructive?: boolean; +} + +/** + * Links a `show()` call to the `` that answers it. Create with + * {@link createConfirmHandle}; `show` is the whole public surface. + */ +export interface ConfirmHandle { + /** + * Opens the confirmation and resolves with the user's answer: `true` for the action, `false` + * for cancel or any dismissal. + * + * Calling it while a confirmation is already showing returns the IN-FLIGHT promise rather than + * opening a second one — repeated Escapes against a guarded dialog would otherwise stack + * confirmations, one per keypress. The options of the later call are ignored, since the + * question on screen is already the one being answered. + */ + show(options: ConfirmOptions): Promise; + /** The dialog handle `` mounts against. @internal */ + readonly dialog: DialogHandle; + /** + * Resolves the in-flight promise, if any. Idempotent per question: the second call for the same + * `show()` is a no-op, which is what lets the action settle `true` and then close through the + * ordinary path without the close settling `false` on top of it. @internal + */ + settle(confirmed: boolean): void; +} + +/** + * Creates a {@link ConfirmHandle}: an awaitable confirmation, in the shape of a promise rather + * than a pair of state variables and a callback. + * + * ```ts + * const confirm = createConfirmHandle(); + * if (await confirm.show({ title: 'Discard changes?', description: '…' })) { + * discard(); + * } + * ``` + * + * The dialog itself is still rendered as JSX — `` — and + * where it is rendered matters: it belongs inside the dialog it guards, so the two are in the same + * floating tree and escape ordering, the stacking styles and the refcounted scroll lock all apply. + * A confirmation mounted app-globally would be a sibling of the dialog rather than a child of it, + * and every one of those would break. + * + * Create one per guarded dialog, at module scope or in a `useMemo` — never inside the render body + * without one, since a new handle each render would orphan the promise a `show()` is waiting on. + */ +export function createConfirmHandle(): ConfirmHandle { + const dialog = Primitive.createHandle(); + let pending: { promise: Promise; resolve: (confirmed: boolean) => void } | null = null; + + return { + dialog, + show(options) { + if (pending) { + return pending.promise; + } + let resolve!: (confirmed: boolean) => void; + const promise = new Promise(res => { + resolve = res; + }); + pending = { promise, resolve }; + // The options ride along as the dialog's payload rather than being held in state out here, + // so what is on screen and what the promise resolves are the same object. + dialog.open(options); + return promise; + }, + settle(confirmed) { + const inFlight = pending; + pending = null; + inFlight?.resolve(confirmed); + }, + }; +} diff --git a/packages/ui/src/mosaic/components/alert-dialog/confirm.test.tsx b/packages/ui/src/mosaic/components/alert-dialog/confirm.test.tsx new file mode 100644 index 00000000000..00fbbe41262 --- /dev/null +++ b/packages/ui/src/mosaic/components/alert-dialog/confirm.test.tsx @@ -0,0 +1,291 @@ +import { act, cleanup, render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import React from 'react'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { Dialog } from '../dialog'; +import { AlertDialog } from './alert-dialog'; +import { createConfirmHandle } from './confirm-handle'; +import { useConfirmedClose } from './use-confirmed-close'; + +afterEach(() => cleanup()); + +const settle = () => + act(async () => { + await new Promise(resolve => setTimeout(resolve, 0)); + }); + +/** + * The target shape: a form prompt that will not close while its field holds anything, guarded by a + * confirmation stacked on it. + */ +function GuardedForm({ onClosed }: { onClosed?: () => void } = {}) { + const confirm = React.useMemo(() => createConfirmHandle(), []); + const [open, setOpen] = React.useState(true); + const [value, setValue] = React.useState(''); + const inputRef = React.useRef(null); + + const onOpenChange = useConfirmedClose({ + handle: confirm, + when: () => value.trim() !== '', + onOpenChange: next => { + setOpen(next); + if (!next) { + onClosed?.(); + } + }, + confirm: { + title: 'Discard changes?', + description: 'This address has not been saved.', + actionLabel: 'Discard', + cancelLabel: 'Keep editing', + destructive: true, + }, + }); + + return ( + + Add email address + setValue(event.target.value)} + /> + Cancel + + + ); +} + +describe('useConfirmedClose', () => { + it('closes without asking when the guard does not apply', async () => { + const user = userEvent.setup(); + render(); + + await user.keyboard('{Escape}'); + + expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); + expect(screen.queryByRole('alertdialog')).not.toBeInTheDocument(); + }); + + it('asks instead of closing once the guard applies, and keeps the dialog open behind it', async () => { + const user = userEvent.setup(); + render(); + + await user.type(screen.getByRole('textbox', { name: 'Email address' }), 'name@example.com'); + await user.keyboard('{Escape}'); + + expect(screen.getByRole('alertdialog', { name: 'Discard changes?' })).toBeInTheDocument(); + // `hidden: true` because it has to be: a modal confirmation marks everything outside it inert + // and `aria-hidden`, the guarded dialog included, so the default accessible-tree query would + // report it missing when it is merely covered. That it is still mounted is the assertion. + expect(screen.getByRole('dialog', { name: 'Add email address', hidden: true })).toBeInTheDocument(); + }); + + it('renders the labels the caller asked for', async () => { + const user = userEvent.setup(); + render(); + + await user.type(screen.getByRole('textbox', { name: 'Email address' }), 'a'); + await user.keyboard('{Escape}'); + + expect(screen.getByRole('button', { name: 'Keep editing' })).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Discard' })).toBeInTheDocument(); + }); + + it('commits the close when the action is taken', async () => { + const user = userEvent.setup(); + const onClosed = vi.fn(); + render(); + + await user.type(screen.getByRole('textbox', { name: 'Email address' }), 'a'); + await user.keyboard('{Escape}'); + await user.click(screen.getByRole('button', { name: 'Discard' })); + + await waitFor(() => expect(onClosed).toHaveBeenCalled()); + expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); + }); + + it('leaves the dialog open when the confirmation is cancelled', async () => { + const user = userEvent.setup(); + const onClosed = vi.fn(); + render(); + + await user.type(screen.getByRole('textbox', { name: 'Email address' }), 'a'); + await user.keyboard('{Escape}'); + await user.click(screen.getByRole('button', { name: 'Keep editing' })); + await settle(); + + expect(onClosed).not.toHaveBeenCalled(); + expect(screen.getByRole('dialog', { name: 'Add email address' })).toBeInTheDocument(); + expect(screen.queryByRole('alertdialog')).not.toBeInTheDocument(); + }); + + // A veto still runs floating-ui's synchronous `openchange` emit, so the focus machinery fires on + // a close that never happened. Focus must not be dropped on the body. + it('keeps focus inside the dialog after a vetoed Escape', async () => { + const user = userEvent.setup(); + render(); + + await user.type(screen.getByRole('textbox', { name: 'Email address' }), 'a'); + await user.keyboard('{Escape}'); + await settle(); + + const alert = screen.getByRole('alertdialog'); + expect(alert.contains(document.activeElement)).toBe(true); + }); + + it('returns focus to the field when the confirmation is cancelled', async () => { + const user = userEvent.setup(); + render(); + + await user.type(screen.getByRole('textbox', { name: 'Email address' }), 'a'); + await user.keyboard('{Escape}'); + await user.click(screen.getByRole('button', { name: 'Keep editing' })); + + await waitFor(() => expect(screen.getByRole('textbox', { name: 'Email address' })).toHaveFocus()); + }); + + // One question per answer: hammering Escape used to be able to stack a confirmation per keypress. + it('opens one confirmation for repeated close requests', async () => { + const user = userEvent.setup(); + render(); + + await user.type(screen.getByRole('textbox', { name: 'Email address' }), 'a'); + await user.keyboard('{Escape}'); + await user.keyboard('{Escape}'); + await user.keyboard('{Escape}'); + await settle(); + + expect(screen.getAllByRole('alertdialog')).toHaveLength(1); + }); + + it('covers Dialog.Close as well as Escape', async () => { + const user = userEvent.setup(); + render(); + + await user.type(screen.getByRole('textbox', { name: 'Email address' }), 'a'); + await user.click(screen.getByRole('button', { name: 'Cancel' })); + + expect(screen.getByRole('alertdialog', { name: 'Discard changes?' })).toBeInTheDocument(); + expect(screen.getByRole('dialog', { name: 'Add email address', hidden: true })).toBeInTheDocument(); + }); + + it('can be asked again after an answer', async () => { + const user = userEvent.setup(); + render(); + const field = screen.getByRole('textbox', { name: 'Email address' }); + + await user.type(field, 'a'); + await user.keyboard('{Escape}'); + await user.click(screen.getByRole('button', { name: 'Keep editing' })); + await settle(); + await user.keyboard('{Escape}'); + + expect(screen.getByRole('alertdialog', { name: 'Discard changes?' })).toBeInTheDocument(); + }); +}); + +describe('createConfirmHandle', () => { + it('resolves true for the action and false for a cancel', async () => { + const user = userEvent.setup(); + const handle = createConfirmHandle(); + const answers: boolean[] = []; + + function Harness() { + return ( + + Host + + + + ); + } + render(); + + await user.click(screen.getByRole('button', { name: 'Ask' })); + await user.click(screen.getByRole('button', { name: 'Confirm' })); + await waitFor(() => expect(answers).toEqual([true])); + + await user.click(screen.getByRole('button', { name: 'Ask' })); + await user.click(screen.getByRole('button', { name: 'Cancel' })); + await waitFor(() => expect(answers).toEqual([true, false])); + }); + + it('resolves false when dismissed with Escape', async () => { + const user = userEvent.setup(); + const handle = createConfirmHandle(); + const answers: boolean[] = []; + + render( + + Host + + + , + ); + + await user.click(screen.getByRole('button', { name: 'Ask' })); + await user.keyboard('{Escape}'); + + await waitFor(() => expect(answers).toEqual([false])); + }); + + it('returns the in-flight promise rather than opening a second confirmation', async () => { + const handle = createConfirmHandle(); + render( + + Host + + , + ); + + let first!: Promise; + let second!: Promise; + await act(async () => { + first = handle.show({ title: 'First', description: 'One' }); + second = handle.show({ title: 'Second', description: 'Two' }); + }); + + expect(second).toBe(first); + expect(screen.getByRole('alertdialog', { name: 'First' })).toBeInTheDocument(); + }); + + it('stacks over the dialog it guards rather than replacing it', async () => { + const user = userEvent.setup(); + render(); + + await user.type(screen.getByRole('textbox', { name: 'Email address' }), 'a'); + await user.keyboard('{Escape}'); + await settle(); + + const popups = document.querySelectorAll('.cl-dialog-popup'); + expect(popups).toHaveLength(2); + expect(popups[0]).toHaveAttribute('data-stack-base', ''); + expect(popups[1]).toHaveAttribute('data-stacked', ''); + }); +}); diff --git a/packages/ui/src/mosaic/components/alert-dialog/index.ts b/packages/ui/src/mosaic/components/alert-dialog/index.ts index 4c16d774290..8acec07aa7e 100644 --- a/packages/ui/src/mosaic/components/alert-dialog/index.ts +++ b/packages/ui/src/mosaic/components/alert-dialog/index.ts @@ -1,8 +1,13 @@ export { AlertDialog } from './alert-dialog'; +export { createConfirmHandle } from './confirm-handle'; +export type { ConfirmHandle, ConfirmOptions } from './confirm-handle'; +export { useConfirmedClose } from './use-confirmed-close'; +export type { UseConfirmedCloseOptions } from './use-confirmed-close'; export type { AlertDialogActionsProps, AlertDialogBackdropProps, AlertDialogCloseProps, + AlertDialogConfirmProps, AlertDialogDescriptionProps, AlertDialogPopupProps, AlertDialogProps, diff --git a/packages/ui/src/mosaic/components/alert-dialog/use-confirmed-close.ts b/packages/ui/src/mosaic/components/alert-dialog/use-confirmed-close.ts new file mode 100644 index 00000000000..f29f718e595 --- /dev/null +++ b/packages/ui/src/mosaic/components/alert-dialog/use-confirmed-close.ts @@ -0,0 +1,76 @@ +import type { DialogOpenChangeDetails } from '@clerk/headless/dialog'; +import React from 'react'; + +import type { ConfirmHandle, ConfirmOptions } from './confirm-handle'; + +export interface UseConfirmedCloseOptions { + /** The handle shared with the `` rendered inside the guarded dialog. */ + handle: ConfirmHandle; + /** + * Whether closing needs confirming, evaluated at the moment of each close request — typically + * "is this form dirty". Return `false` and the close commits immediately, with no interruption. + */ + when: () => boolean; + /** Commits the open state. The guarded dialog's own `onOpenChange`, usually a `setOpen`. */ + onOpenChange: (open: boolean, details: DialogOpenChangeDetails) => void; + /** What to ask when `when()` returns true. */ + confirm: ConfirmOptions; +} + +/** A close the user never asked for: the one this hook commits after the action is confirmed. */ +const PROGRAMMATIC_DETAILS: DialogOpenChangeDetails = { trigger: null, triggerId: null, event: undefined }; + +/** + * Guards a dialog's close behind a confirmation, returning the `onOpenChange` to hand it. + * + * ```tsx + * const onOpenChange = useConfirmedClose({ + * handle: confirm, + * when: () => value !== '', + * onOpenChange: setOpen, + * confirm: { title: 'Discard changes?', description: '…', actionLabel: 'Discard' }, + * }); + * + * + * … + * + * + * ``` + * + * **The dialog must be controlled.** A veto is the absence of a commit, and an uncontrolled dialog + * has already committed internally by the time `onOpenChange` runs — there would be nothing left to + * decline. + * + * **What it covers is every close the dialog itself owns**: Escape, an outside press where + * `closedBy` allows one, `Dialog.CloseButton`, `Dialog.Close`, and the `close` the `Dialog` wrapper + * hands its children. All of them funnel through `onOpenChange`, so one branch here answers them + * all. What it cannot cover is a button wired to your own `setOpen(false)` — that never reaches the + * dialog, so it bypasses the question silently. Route those through `Dialog.Close` instead. + */ +export function useConfirmedClose({ handle, when, onOpenChange, confirm }: UseConfirmedCloseOptions) { + // Read through a ref so the returned callback is stable across renders: it is handed to a + // dialog that would otherwise see a new `onOpenChange` identity on every keystroke of the very + // form whose dirtiness `when` is reporting on. + const latest = React.useRef({ when, onOpenChange, confirm }); + React.useLayoutEffect(() => { + latest.current = { when, onOpenChange, confirm }; + }); + + return React.useCallback( + (open: boolean, details: DialogOpenChangeDetails) => { + if (open || !latest.current.when()) { + latest.current.onOpenChange(open, details); + return; + } + // The veto: return without committing, so the dialog stays open behind the question. The + // close is re-issued from here only if the answer is yes — and it is a fresh close rather + // than the original, whose event belongs to an interaction that has long since finished. + void handle.show(latest.current.confirm).then(confirmed => { + if (confirmed) { + latest.current.onOpenChange(false, PROGRAMMATIC_DETAILS); + } + }); + }, + [handle], + ); +} diff --git a/packages/ui/src/mosaic/styles/index.ts b/packages/ui/src/mosaic/styles/index.ts index 8a8d035db03..f4f0ba9c26a 100644 --- a/packages/ui/src/mosaic/styles/index.ts +++ b/packages/ui/src/mosaic/styles/index.ts @@ -6,11 +6,15 @@ export type { MosaicComponentProps, MosaicElementProps } from '../props'; -export { AlertDialog } from '../components/alert-dialog'; +export { AlertDialog, createConfirmHandle, useConfirmedClose } from '../components/alert-dialog'; export type { + ConfirmHandle, + ConfirmOptions, + UseConfirmedCloseOptions, AlertDialogActionsProps, AlertDialogBackdropProps, AlertDialogCloseProps, + AlertDialogConfirmProps, AlertDialogDescriptionProps, AlertDialogPopupProps, AlertDialogProps, From 5a6a3886f9e20e7f075a38d1d9586dbadde69401 Mon Sep 17 00:00:00 2001 From: Max Yinger Date: Thu, 13 Aug 2026 13:42:38 -0600 Subject: [PATCH 2/2] fix(ui): address review feedback on #9439 Let an explicit `handle.open(payload)` win over the stale `activeTriggerId` lookup, keep the payload published through the exit transition so a `handle.close()` no longer blanks children-as-function content, and settle an in-flight question `false` when `` unmounts rather than poisoning the handle. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/primitives/dialog/dialog-root.tsx | 23 ++++++++-- .../src/primitives/dialog/dialog.test.tsx | 46 +++++++++++++++++++ .../src/stories/alert-dialog.component.mdx | 5 ++ .../components/alert-dialog/alert-dialog.tsx | 7 ++- .../components/alert-dialog/confirm-handle.ts | 6 +++ .../components/alert-dialog/confirm.test.tsx | 30 ++++++++++++ 6 files changed, 113 insertions(+), 4 deletions(-) diff --git a/packages/headless/src/primitives/dialog/dialog-root.tsx b/packages/headless/src/primitives/dialog/dialog-root.tsx index 7ae9f947531..c3b1d077097 100644 --- a/packages/headless/src/primitives/dialog/dialog-root.tsx +++ b/packages/headless/src/primitives/dialog/dialog-root.tsx @@ -141,9 +141,16 @@ function DialogInner(props: DialogProps & { isNested: boolean setOpen: (nextOpen, payload) => { // Held in a ref as well as in state because the payload effect below re-runs on `open` // and would otherwise resolve a trigger-less open to `undefined`, wiping this a commit - // after it was set. + // after it was set. Cleared on close as well, so the next payload-less open does not + // resurface this one. directPayloadRef.current = payload; - setActivePayload(payload); + // Published only on the way IN. A close carries no payload, and writing it would blank + // the children-as-function while the popup is still mounted for its exit transition — + // rendering the dialog empty as it leaves, or throwing in a consumer that dereferences + // the payload. The next open sets it afresh. + if (nextOpen) { + setActivePayload(payload); + } floatingContext.onOpenChange(nextOpen); }, }); @@ -169,10 +176,20 @@ function DialogInner(props: DialogProps & { isNested: boolean // `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. + // + // An explicit programmatic payload wins over the registry lookup: `activeTriggerId` is never + // reset on close, so once any trigger has opened the dialog the lookup would otherwise overwrite + // every later `handle.open(payload)` with that trigger's payload. The mirror already holds — + // `openFromTrigger` clears `directPayloadRef`, so a trigger wins the other way. useLayoutEffect(() => { if (open) { + const direct = directPayloadRef.current; setActivePayload( - activeTriggerId != null ? store.getTrigger(activeTriggerId)?.getPayload() : directPayloadRef.current, + direct !== undefined + ? direct + : activeTriggerId != null + ? store.getTrigger(activeTriggerId)?.getPayload() + : undefined, ); } }, [store, open, activeTriggerId]); diff --git a/packages/headless/src/primitives/dialog/dialog.test.tsx b/packages/headless/src/primitives/dialog/dialog.test.tsx index 58839f9f106..81475c1bb57 100644 --- a/packages/headless/src/primitives/dialog/dialog.test.tsx +++ b/packages/headless/src/primitives/dialog/dialog.test.tsx @@ -539,6 +539,12 @@ describe('Dialog', () => { {({ payload }) => ( <> Open A + + Open B + @@ -584,6 +590,46 @@ describe('Dialog', () => { expect(screen.getByRole('dialog', { name: 'no payload' })).toBeInTheDocument(); }); + + it('supersedes the trigger that opened the dialog last', async () => { + const user = userEvent.setup(); + const handle = renderDetached(); + + // `activeTriggerId` is never reset on close, so without an explicit precedence the + // registry lookup would hand this open the previous trigger's payload back. + await user.click(screen.getByRole('button', { name: 'Open B' })); + act(() => handle.close()); + act(() => handle.open('from-handle')); + await act(async () => { + await Promise.resolve(); + }); + + expect(screen.getByRole('dialog', { name: 'from-handle' })).toBeInTheDocument(); + }); + + it('survives a `handle.close()` for the length of the exit transition', () => { + // Keep an animation pending so the popup stays mounted after the close. + const original = (Element.prototype as { getAnimations?: unknown }).getAnimations; + (Element.prototype as { getAnimations?: unknown }).getAnimations = () => [ + { finished: new Promise(() => {}) }, + ]; + try { + const handle = renderDetached(); + + act(() => handle.open('from-handle')); + act(() => handle.close()); + + // `close()` carries no payload; blanking it here would render the dialog empty on its + // way out, or throw in a children function that dereferences it. + expect(screen.getByRole('dialog', { name: 'from-handle' })).toBeInTheDocument(); + } finally { + if (original) { + (Element.prototype as { getAnimations?: unknown }).getAnimations = original; + } else { + delete (Element.prototype as { getAnimations?: unknown }).getAnimations; + } + } + }); }); }); diff --git a/packages/swingset/src/stories/alert-dialog.component.mdx b/packages/swingset/src/stories/alert-dialog.component.mdx index 3a25e811c76..03500a4ed12 100644 --- a/packages/swingset/src/stories/alert-dialog.component.mdx +++ b/packages/swingset/src/stories/alert-dialog.component.mdx @@ -138,6 +138,11 @@ It resolves `true` for the action and `false` for cancel or any dismissal. Calli confirmation is already showing returns the in-flight promise rather than opening a second one, so repeated close requests ask once. +**`AlertDialog.Confirm` must be mounted when `show()` is called** — it is the thing that opens, and +a `show()` with nothing mounted to answer it never resolves. Since the confirmation lives inside the +dialog it guards, that means asking from inside that dialog, while it is open. A confirmation that +unmounts with a question in flight answers `false` rather than leaving the `await` hanging. + ## Parts | Part | Slot | Description | diff --git a/packages/ui/src/mosaic/components/alert-dialog/alert-dialog.tsx b/packages/ui/src/mosaic/components/alert-dialog/alert-dialog.tsx index e7b53c55357..ad4ae4889f4 100644 --- a/packages/ui/src/mosaic/components/alert-dialog/alert-dialog.tsx +++ b/packages/ui/src/mosaic/components/alert-dialog/alert-dialog.tsx @@ -1,5 +1,4 @@ import type { DialogFocusTarget } from '@clerk/headless/dialog'; -import { useDialogContext } from '@clerk/headless/dialog'; import { useRender } from '@clerk/headless/utils'; import * as stylex from '@stylexjs/stylex'; import type { ReactNode } from 'react'; @@ -229,6 +228,12 @@ export interface AlertDialogConfirmProps { * styles and the refcounted scroll lock all read. */ function Confirm({ handle, finalFocus }: AlertDialogConfirmProps) { + // A question can only be answered while the thing that asks it is on screen. Going away with one + // in flight would leave the promise unresolved forever, and `show()` short-circuits on an + // in-flight question — so the handle would never open a confirmation again, and a guarded dialog + // whose closes route through one could no longer be closed at all. + React.useEffect(() => () => handle.settle(false), [handle]); + return ( ` must be MOUNTED when this is called. It is what opens, and a + * `dialog.open()` with no root attached is a no-op — the promise would never settle. Since the + * confirmation belongs inside the dialog it guards, that means asking only from inside that + * dialog while it is open. A confirmation that unmounts with a question in flight answers + * `false` rather than hanging. */ show(options: ConfirmOptions): Promise; /** The dialog handle `` mounts against. @internal */ diff --git a/packages/ui/src/mosaic/components/alert-dialog/confirm.test.tsx b/packages/ui/src/mosaic/components/alert-dialog/confirm.test.tsx index 00fbbe41262..4e242878a7d 100644 --- a/packages/ui/src/mosaic/components/alert-dialog/confirm.test.tsx +++ b/packages/ui/src/mosaic/components/alert-dialog/confirm.test.tsx @@ -275,6 +275,36 @@ describe('createConfirmHandle', () => { expect(screen.getByRole('alertdialog', { name: 'First' })).toBeInTheDocument(); }); + // An unanswered question poisons the handle: `show()` short-circuits on the in-flight promise, + // so the confirmation would never open again and a guarded dialog could no longer be closed. + it('answers false when the confirmation unmounts with a question in flight', async () => { + const handle = createConfirmHandle(); + const answers: boolean[] = []; + + function Harness({ mounted }: { mounted: boolean }) { + return ( + + Host + {mounted ? : null} + + ); + } + const { rerender } = render(); + + await act(async () => { + void handle.show({ title: 'Sure?', description: 'No going back.' }).then(a => answers.push(a)); + }); + rerender(); + + await waitFor(() => expect(answers).toEqual([false])); + // And the handle is usable again rather than stuck on the dead promise. + rerender(); + await act(async () => { + void handle.show({ title: 'Again?', description: 'Still no going back.' }).then(a => answers.push(a)); + }); + expect(screen.getByRole('alertdialog', { name: 'Again?' })).toBeInTheDocument(); + }); + it('stacks over the dialog it guards rather than replacing it', async () => { const user = userEvent.setup(); render();