diff --git a/.changeset/calm-mfa-actions.md b/.changeset/calm-mfa-actions.md new file mode 100644 index 00000000000..a845151cc84 --- /dev/null +++ b/.changeset/calm-mfa-actions.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/.changeset/contact-verify-pending.md b/.changeset/contact-verify-pending.md new file mode 100644 index 00000000000..a845151cc84 --- /dev/null +++ b/.changeset/contact-verify-pending.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/.changeset/friendly-identifier-lock.md b/.changeset/friendly-identifier-lock.md new file mode 100644 index 00000000000..a845151cc84 --- /dev/null +++ b/.changeset/friendly-identifier-lock.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/packages/mosaic/AGENTS.md b/packages/mosaic/AGENTS.md index 0ef6ef8a5da..f94ebb1b3b5 100644 --- a/packages/mosaic/AGENTS.md +++ b/packages/mosaic/AGENTS.md @@ -34,6 +34,7 @@ Most problems a feature hits already have a shared answer. Find the row for your | Problem | Use | | ------------------------------------------------------------------------ | ----------------------------------------------------------------------------------------------------------------- | | An async action from a button or row: pending, an error, one lock | `usePendingAction` (`src/hooks/`) | +| Direct actions and confirmation dialogs that must exclude each other | `useActionLock` injected into `usePendingAction` and `useConfirmController` (`src/hooks/`, `src/blocks/confirm/`) | | A form: values, validation, submit, field errors | `useForm` (`src/components/form/`), with `validateAsync` for a server-side check | | A `
` element, without the browser's validation popup | `Form` (`src/primitives/form/`) | | A model save whose Clerk errors should land on form fields | `save` (`src/utils/errors.ts`) in the model, or reject with `FormSubmitError` | diff --git a/packages/mosaic/docs/controllers.md b/packages/mosaic/docs/controllers.md index d2aca72f306..8207d6e496d 100644 --- a/packages/mosaic/docs/controllers.md +++ b/packages/mosaic/docs/controllers.md @@ -51,6 +51,23 @@ return { }; ``` +When a direct action and a confirmation must not run together, create one +`useActionLock` (`src/hooks/use-action-lock.ts`) and inject it into both owners: + +```ts +const actionLock = useActionLock(); +const primary = usePendingAction({ actionLock }); +const remove = useConfirmController({ + actionLock, + onConfirm: contact => removalFocus.remove(contact.id), +}); +``` + +Pass `actionLock.isPending` to the section controls' `disabled` props, including +`Confirmation.disabled`. The lock prevents overlapping requests even before React +renders the disabled controls, and stays held until the action settles. Each owner +keeps its existing error handling. `actionLock` is a hook option, not a per-run option. + `run` resolves `true` on success, so the caller can close a dialog or move focus. To hold pending through an OAuth redirect, await a delay inside the action (`user-profile-connected-accounts-section.controller.ts`). When the user diff --git a/packages/mosaic/src/blocks/confirm/confirm.controller.ts b/packages/mosaic/src/blocks/confirm/confirm.controller.ts index 6924f103b5b..faf04107a81 100644 --- a/packages/mosaic/src/blocks/confirm/confirm.controller.ts +++ b/packages/mosaic/src/blocks/confirm/confirm.controller.ts @@ -4,6 +4,7 @@ import { useMemo } from 'react'; import type { DialogHandle } from '../../components/dialog'; import { Dialog } from '../../components/dialog'; import type { ReverificationController } from '../../features/reverification'; +import type { ActionLock } from '../../hooks/use-action-lock'; import type { ErrorDescription } from '../../localization'; import { useErrorText } from '../../localization'; import { setup } from '../../machine/setup'; @@ -85,6 +86,7 @@ export interface ConfirmController { } export interface ConfirmControllerOptions { + actionLock?: ActionLock; /** The action the dialog confirms. Resolve to close; reject to keep the dialog open showing why. */ onConfirm: (payload: Payload) => Promise | void; /** The flow `onConfirm` runs under, when the action may need step-up verification. */ @@ -107,6 +109,7 @@ export function useConfirmController({ onConfirm, reverification, errorFallback, + actionLock, }: ConfirmControllerOptions): ConfirmController { const handle = useMemo(() => Dialog.createHandle(), []); const [snapshot, send] = useMachine(confirmMachine); @@ -139,12 +142,17 @@ export function useConfirmController({ if (snapshot.value !== 'confirming') { return; } - send({ - type: 'CONFIRM', - run: async () => { - await onConfirm(payload); - }, - }); + const run = async () => { + await onConfirm(payload); + }; + if (actionLock) { + const accepted = actionLock.tryRun(run); + if (accepted) { + send({ type: 'CONFIRM', run: () => accepted }); + } + return; + } + send({ type: 'CONFIRM', run }); }, }; } diff --git a/packages/mosaic/src/blocks/confirmation/__tests__/confirmation.feature.test.tsx b/packages/mosaic/src/blocks/confirmation/__tests__/confirmation.feature.test.tsx index 2de465ee6ff..b256db420ed 100644 --- a/packages/mosaic/src/blocks/confirmation/__tests__/confirmation.feature.test.tsx +++ b/packages/mosaic/src/blocks/confirmation/__tests__/confirmation.feature.test.tsx @@ -38,7 +38,11 @@ function Host({ async function renderAccount( options: ConfirmControllerOptions, - { localization, description }: { localization?: MosaicLocalization; description?: ReactNode } = {}, + { + localization, + description, + disabled, + }: { localization?: MosaicLocalization; description?: ReactNode; disabled?: boolean } = {}, ) { const ref: { current: ConfirmController | undefined } = { current: undefined }; serveFapi({ client: fapiClient([session]) }); @@ -53,6 +57,7 @@ async function renderAccount( title={TITLE} description={description ?? 'Google will be removed from this account.'} actionLabel='Remove' + disabled={disabled} /> ); }} @@ -70,6 +75,19 @@ async function renderAccount( const confirmButton = () => screen.getByRole('button', { name: 'Remove' }); describe('Confirmation', () => { + it('disables confirmation while leaving Cancel usable', async () => { + const onConfirm = vi.fn(() => Promise.resolve()); + const { user, open, closed } = await renderAccount({ onConfirm }, { disabled: true }); + await open(); + + expect(confirmButton()).toBeDisabled(); + await user.click(confirmButton()); + expect(onConfirm).not.toHaveBeenCalled(); + + await user.click(screen.getByRole('button', { name: 'Cancel' })); + await closed(); + }); + it('opens from its trigger, confirms, closes once the action lands, and hands focus back', async () => { const run = deferred(); const onConfirm = vi.fn(() => run.promise); @@ -82,6 +100,7 @@ describe('Confirmation', () => { await user.click(confirmButton()); expect(onConfirm).toHaveBeenCalledOnce(); expect(confirmButton()).toHaveAttribute('aria-busy', 'true'); + expect(confirmButton()).toHaveFocus(); run.resolve(); await closed(); diff --git a/packages/mosaic/src/blocks/confirmation/confirmation.tsx b/packages/mosaic/src/blocks/confirmation/confirmation.tsx index 02dd5b79491..0387ddec10d 100644 --- a/packages/mosaic/src/blocks/confirmation/confirmation.tsx +++ b/packages/mosaic/src/blocks/confirmation/confirmation.tsx @@ -10,6 +10,7 @@ export type ConfirmationColor = 'negative' | 'primary'; export interface ConfirmationProps extends ConfirmBlockProps { /** The weight the confirming button carries. An action that can be undone takes `primary` (default: `negative`) */ color?: ConfirmationColor; + disabled?: boolean; } /** @@ -36,7 +37,7 @@ export interface ConfirmationProps extends ConfirmBlockProps */ -export function Confirmation({ color = 'negative', ...props }: ConfirmationProps) { +export function Confirmation({ color = 'negative', disabled, ...props }: ConfirmationProps) { return ( {({ title, description, actionLabel, cancelLabel, isPending, errorMessage, confirm }) => ( @@ -67,6 +68,7 @@ export function Confirmation({ color = 'negative', ...props }: C fullWidth color={color} isPending={isPending} + disabled={disabled && !isPending} onClick={confirm} > {actionLabel} diff --git a/packages/mosaic/src/features/user-profile/__tests__/user-profile-contact.utils.test.ts b/packages/mosaic/src/features/user-profile/__tests__/user-profile-contact.utils.test.ts index 3c2604a96dc..b70ad875de4 100644 --- a/packages/mosaic/src/features/user-profile/__tests__/user-profile-contact.utils.test.ts +++ b/packages/mosaic/src/features/user-profile/__tests__/user-profile-contact.utils.test.ts @@ -9,26 +9,30 @@ function contact(id: string, status: Contact['verification']['status'], expireAt } describe('toContacts', () => { - it('orders primary first, then verified, then pending by expiry, then never started', () => { + it('orders verified, then pending by expiry, then never started regardless of primary', () => { const items = [ contact('idn_unstarted', null), contact('idn_pending_late', 'unverified', new Date(2026, 0, 2)), contact('idn_verified_b', 'verified'), - contact('idn_primary', 'verified'), + contact('idn_verified_z', 'verified'), contact('idn_pending_soon', 'unverified', new Date(2026, 0, 1)), contact('idn_verified_a', 'verified'), ]; - const contacts = toContacts(items, 'idn_primary', item => item.id); + const contacts = toContacts(items, 'idn_verified_z', item => item.id); + const changedPrimary = toContacts(items, 'idn_verified_b', item => item.id); expect(contacts.map(item => item.id)).toEqual([ - 'idn_primary', 'idn_verified_a', 'idn_verified_b', + 'idn_verified_z', 'idn_pending_soon', 'idn_pending_late', 'idn_unstarted', ]); + expect(changedPrimary.map(item => item.id)).toEqual(contacts.map(item => item.id)); + expect(changedPrimary.find(item => item.id === 'idn_verified_b')?.isDefault).toBe(true); + expect(changedPrimary.find(item => item.id === 'idn_verified_z')?.isDefault).toBe(false); }); it('marks which contact is the primary one and which are verified', () => { diff --git a/packages/mosaic/src/features/user-profile/user-profile-contact-list-row.view.tsx b/packages/mosaic/src/features/user-profile/user-profile-contact-list-row.view.tsx index d4ed46d1525..a55e508de6f 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-contact-list-row.view.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-contact-list-row.view.tsx @@ -22,6 +22,9 @@ export interface UserProfileContactListRowViewProps { items: UserProfileContact[]; onAdd?: () => void; onVerify?: (id: string) => void; + pendingVerifyId?: string; + pendingPrimaryId?: string; + isDisabled?: boolean; onSetPrimary?: (id: string) => void; onRemove?: (item: UserProfileContact) => void; children?: ReactNode; @@ -33,6 +36,9 @@ export function UserProfileContactListRowView({ items, onAdd, onVerify, + pendingVerifyId, + pendingPrimaryId, + isDisabled = false, onSetPrimary, onRemove, addAction, @@ -62,6 +68,7 @@ export function UserProfileContactListRowView({ size='sm' variant='outline' onClick={onAdd} + disabled={isDisabled} > onVerify(item.id), + status: pendingVerifyId === item.id ? 'pending' : 'idle', }); } else if (item.isVerified && !item.isDefault && onSetPrimary) { - actions.push({ label: m.setPrimary, onClick: () => onSetPrimary(item.id) }); + actions.push({ + label: m.setPrimary, + onClick: () => onSetPrimary(item.id), + status: pendingPrimaryId === item.id ? 'pending' : 'idle', + }); } if (onRemove) { @@ -116,6 +128,8 @@ export function UserProfileContactListRowView({ diff --git a/packages/mosaic/src/features/user-profile/user-profile-contact.utils.ts b/packages/mosaic/src/features/user-profile/user-profile-contact.utils.ts index 2c720a69a7e..3aff9703ed5 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-contact.utils.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-contact.utils.ts @@ -39,11 +39,10 @@ export function byId(items: T[], id: string, kind: str type Identification = { id: string; verification: Pick }; -export function sortByVerification(items: T[], primaryId: string | null): T[] { - const others = items.filter(item => item.id !== primaryId); - const verified = others.filter(item => item.verification.status === 'verified'); - const pending = others.filter(item => item.verification.status && item.verification.status !== 'verified'); - const unstarted = others.filter(item => !item.verification.status); +export function sortByVerification(items: T[]): T[] { + const verified = items.filter(item => item.verification.status === 'verified'); + const pending = items.filter(item => item.verification.status && item.verification.status !== 'verified'); + const unstarted = items.filter(item => !item.verification.status); verified.sort((a, b) => a.id.localeCompare(b.id)); pending.sort((a, b) => { @@ -52,7 +51,7 @@ export function sortByVerification(items: T[], primary return aExpireAt && bExpireAt ? aExpireAt.getTime() - bExpireAt.getTime() : 0; }); - return [...items.filter(item => item.id === primaryId), ...verified, ...pending, ...unstarted]; + return [...verified, ...pending, ...unstarted]; } export function toContacts( @@ -60,7 +59,7 @@ export function toContacts( primaryId: string | null, value: (item: T) => string, ): UserProfileContact[] { - return sortByVerification(items, primaryId).map(item => ({ + return sortByVerification(items).map(item => ({ id: item.id, value: value(item), isDefault: item.id === primaryId, diff --git a/packages/mosaic/src/features/user-profile/user-profile-email-section/__tests__/user-profile-add-email.controller.test.ts b/packages/mosaic/src/features/user-profile/user-profile-email-section/__tests__/user-profile-add-email.controller.test.ts index f17f65de695..1f38a4fa3d1 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-email-section/__tests__/user-profile-add-email.controller.test.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-email-section/__tests__/user-profile-add-email.controller.test.ts @@ -192,18 +192,27 @@ describe('useUserProfileAddEmailController', () => { expect(email.verifyCode).toHaveBeenCalledExactlyOnceWith('123456'); }); - it('verifies an existing email without creating one', async () => { + it('verifies an existing email without creating one, opening once its code is sent', async () => { const onCreate = vi.fn(created); - const email = verifier(); + const sent = createDeferredPromise(); + const email = verifier(() => ({ method: 'code', sent: sent.promise.then(() => undefined) })); const { result } = renderHook(() => useUserProfileAddEmailController({ reverification: idleReverification, onCreate }), ); act(() => result.current.onVerifyEmail(other, email)); + expect(email.start).toHaveBeenCalledOnce(); + expect(result.current.open).toBe(false); + expect(result.current.pendingVerifyId).toBe('email_2'); + + await act(async () => { + sent.resolve(); + await sent.promise; + }); expect(result.current.open).toBe(true); + expect(result.current.pendingVerifyId).toBeUndefined(); expect(result.current.step).toBe('verify'); expect(result.current.emailAddress).toBe('other@example.com'); - await waitFor(() => expect(email.start).toHaveBeenCalledOnce()); act(() => result.current.onSubmit('123456')); await waitFor(() => expect(result.current.open).toBe(false)); diff --git a/packages/mosaic/src/features/user-profile/user-profile-email-section/__tests__/user-profile-email-section.feature.test.tsx b/packages/mosaic/src/features/user-profile/user-profile-email-section/__tests__/user-profile-email-section.feature.test.tsx index 5086ae72501..1724a360763 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-email-section/__tests__/user-profile-email-section.feature.test.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-email-section/__tests__/user-profile-email-section.feature.test.tsx @@ -68,6 +68,36 @@ async function enterCode(actor: Actor, code: string) { } describe('the user profile email section', () => { + it('holds the menu and row order while setting an address as primary', async () => { + const other = fapiEmailAddress({ id: 'idn_other', email_address: 'bob@example.com', verification: VERIFIED }); + const { actor } = await renderSection(signedInWithEmails([PRIMARY, other])); + const initialOrder = emailsListed(); + const primary = holdRequests('post', '/v1/me'); + try { + await manageEmail(actor, 'bob@example.com', 'Set as primary'); + await waitFor(() => expect(primary.requests).toHaveLength(1)); + const menu = screen.getByRole('menu'); + const item = within(menu).getByRole('menuitem', { name: 'Set as primary' }); + expect(item).toHaveAttribute('aria-busy', 'true'); + const manage = screen.getByRole('button', { name: 'Manage bob@example.com' }); + expect(manage).toHaveAttribute('aria-disabled', 'true'); + expect(within(menu).getByRole('menuitem', { name: 'Remove email' })).toHaveAttribute('aria-disabled', 'true'); + expect(screen.queryByRole('alertdialog')).toBeNull(); + expect(screen.getByRole('button', { name: 'Add email' })).toBeDisabled(); + expect(await within(item).findByRole('progressbar', { name: 'Set as primary' })).toBeVisible(); + expect(emailsListed()).toEqual(initialOrder); + } finally { + primary.release(); + await waitFor(() => + expect(within(emailRow()).getByText('bob@example.com').parentElement).toHaveTextContent('Primary'), + ); + } + expect(screen.getByRole('button', { name: 'Manage bob@example.com' })).not.toHaveAttribute('aria-disabled', 'true'); + await waitFor(() => expect(screen.queryByRole('menu')).toBeNull()); + expect(emailsListed()).toEqual(initialOrder); + expect(screen.queryByRole('progressbar', { name: 'Set as primary' })).toBeNull(); + }); + it('adds an address and verifies the code it was sent', async () => { const { actor } = await renderSection(signedInWithEmails([])); expect(emailRow()).toHaveTextContent('No email addresses added'); @@ -114,6 +144,27 @@ describe('the user profile email section', () => { ); }); + it('holds the menu open with a spinner on Verify until the code is sent', async () => { + const pending = fapiEmailAddress({ id: 'idn_pending', email_address: 'pending@example.com' }); + const { actor } = await renderSection(signedInWithEmails([PRIMARY, pending])); + const prepare = holdRequests('post', '/v1/me/email_addresses/:id/prepare_verification'); + + try { + await manageEmail(actor, 'pending@example.com', 'Verify'); + await waitFor(() => expect(prepare.requests).toHaveLength(1)); + expect(screen.getByRole('menu')).toBeVisible(); + expect(screen.getByRole('menuitem', { name: 'Verify' })).toHaveAttribute('aria-busy', 'true'); + expect(await screen.findByRole('progressbar', { name: 'Verify' })).toBeVisible(); + expect(screen.queryByRole('dialog')).toBeNull(); + } finally { + prepare.release(); + } + + await enterCode(actor, VERIFICATION_CODE); + expect(screen.queryByRole('menu')).toBeNull(); + await waitFor(() => expect(screen.queryByRole('dialog')).not.toBeInTheDocument()); + }); + it('waits for the emailed link when the instance verifies by link, then closes once it is opened', async () => { const pending = fapiEmailAddress({ id: 'idn_pending', email_address: 'pending@example.com' }); const { actor, fapi } = await renderSection(signedInWithEmails([PRIMARY, pending], verifiesByLink)); @@ -293,6 +344,7 @@ describe('email reverification', () => { expect(within(emailRow()).queryByRole('alert')).toBeNull(); expect(within(emailRow()).getByRole('button', { name: 'Manage bob@example.com' })).toHaveFocus(); + await waitFor(() => expect(screen.queryByRole('menu')).toBeNull()); await manageEmail(actor, 'bob@example.com', 'Set as primary'); await waitFor(() => expect(fapi.client.sessions[0]?.user?.primary_email_address_id).toBe('idn_second')); }); @@ -406,7 +458,7 @@ describe('email reverification', () => { await screen.findByRole('dialog', { name: 'Verify your email' }); await enterCode(actor, VERIFICATION_CODE); await waitFor(() => expect(screen.queryByRole('dialog')).not.toBeInTheDocument()); - expect(emailsListed()).toEqual(['alice@example.com', 'new@example.com']); + expect(emailsListed()).toEqual(['new@example.com', 'alice@example.com']); }); it('closes quietly when confirmation is dismissed and starts clean on reopen', async () => { diff --git a/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-add-email.controller.ts b/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-add-email.controller.ts index 5b48368eb39..cf7ca679023 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-add-email.controller.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-add-email.controller.ts @@ -29,6 +29,7 @@ export interface UserProfileAddEmailControllerOptions { export interface UserProfileAddEmailController extends UserProfileAddEmailDialogProps { handle: DialogHandle; verifyingId: string | undefined; + pendingVerifyId: string | undefined; onVerifyEmail: (email: Pick, verifier: UserProfileEmailVerifier) => void; } @@ -39,6 +40,7 @@ interface Context { verification: UserProfileEmailVerification | undefined; error: FormError | undefined; resendSeconds: number; + held: boolean; } type Event = @@ -75,9 +77,20 @@ const start = { verification: undefined, error: undefined, resendSeconds: 0, + held: false, })), }; +const sendCode = { + invoke: fromPromise( + (context: Context) => (context.verification?.method === 'code' ? context.verification.sent : missingDependency()), + { + onDone: { target: 'verify', actions: assign(() => ({ resendSeconds: CODE_RESEND_SECONDS, held: false })) }, + onError: { target: 'verify', actions: [fail, assign(() => ({ held: false }))] }, + }, + ), +}; + const machine = createMachine({ id: 'addEmail', initial: 'idle', @@ -88,6 +101,7 @@ const machine = createMachine({ verification: undefined, error: undefined, resendSeconds: 0, + held: false, }, states: { idle: { @@ -102,7 +116,7 @@ const machine = createMachine({ resendSeconds: 0, })), }, - START: start, + START: { ...start, actions: [start.actions, assign(() => ({ held: true }))] }, }, }, email: { @@ -116,7 +130,7 @@ const machine = createMachine({ always: ({ context }) => { const method = context.verification?.method; if (method === 'code') { - return { target: 'sending' }; + return { target: context.held ? 'preparing' : 'sending' }; } if (method === 'sso') { return { target: 'connecting' }; @@ -124,15 +138,8 @@ const machine = createMachine({ return { target: 'waiting', context: { resendSeconds: method === 'link' ? LINK_RESEND_SECONDS : 0 } }; }, }, - sending: { - invoke: fromPromise( - context => (context.verification?.method === 'code' ? context.verification.sent : missingDependency()), - { - onDone: { target: 'verify', actions: assign(() => ({ resendSeconds: CODE_RESEND_SECONDS })) }, - onError: { target: 'verify', actions: fail }, - }, - ), - }, + preparing: sendCode, + sending: sendCode, waiting: { on: { CANCEL: 'idle', TICK: tick, RESEND: resend }, invoke: fromPromise( @@ -194,7 +201,7 @@ export function useUserProfileAddEmailController({ }, }); const state = snapshot.value; - const open = state !== 'idle'; + const open = state !== 'idle' && state !== 'preparing'; const waitingFor = state === 'waiting' && verification?.method === 'link' ? verification : undefined; useEffect(() => waitingFor?.cancel, [waitingFor]); useEffect(() => { @@ -260,6 +267,7 @@ export function useUserProfileAddEmailController({ }, onConnect: () => send({ type: 'CONNECT' }), verifyingId: snapshot.context.verifyingId, + pendingVerifyId: state === 'preparing' ? snapshot.context.verifyingId : undefined, onVerifyEmail: (email, verifier) => { codeForm.reset(); send({ type: 'START', id: email.id, emailAddress: email.value, verifier }); diff --git a/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-email-section.controller.ts b/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-email-section.controller.ts index 822bdeca9b9..6f0bd477d76 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-email-section.controller.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-email-section.controller.ts @@ -1,6 +1,7 @@ import { useRef } from 'react'; import { useConfirmController } from '../../../blocks/confirm'; +import { useActionLock } from '../../../hooks/use-action-lock'; import { useListRemovalFocus } from '../../../hooks/use-list-removal-focus'; import { usePendingAction } from '../../../hooks/use-pending-action'; import type { UserProfileEmail } from '../user-profile-contact.types'; @@ -23,7 +24,8 @@ export function useUserProfileEmailSectionController({ onCreate: onCreateEmail, reverification: createReverification, }); - const setPrimary = usePendingAction(); + const actionLock = useActionLock(); + const setPrimary = usePendingAction({ actionLock }); const rowRef = useRef(null); const removalFocus = useListRemovalFocus({ ids: emails.map(email => email.id), @@ -31,6 +33,7 @@ export function useUserProfileEmailSectionController({ fallback: () => rowRef.current?.querySelector('button:not([disabled])') ?? rowRef.current, }); const remove = useConfirmController({ + actionLock, onConfirm: email => removalFocus.remove(email.id), reverification: removeReverification, }); @@ -40,6 +43,8 @@ export function useUserProfileEmailSectionController({ canAdd: Boolean(onCreateEmail), verification, error: setPrimary.error, + isDisabled: actionLock.isPending, + pendingPrimaryId: setPrimary.pendingKey, rowRef, registerTrigger: removalFocus.registerTrigger, remove: onRemoveEmail ? remove : undefined, diff --git a/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-email-section.types.ts b/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-email-section.types.ts index 1b4985b0644..88b7e698fe5 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-email-section.types.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-email-section.types.ts @@ -9,6 +9,8 @@ import type { UserProfileAddEmailController } from './user-profile-add-email.con export interface UserProfileEmailSectionViewProps { emails: UserProfileEmail[]; canAdd: boolean; + isDisabled: boolean; + pendingPrimaryId: string | undefined; verification: UserProfileAddEmailController; error: string | undefined; rowRef: Ref; diff --git a/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-email-section.view.tsx b/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-email-section.view.tsx index 11c03774c45..6cee98a1799 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-email-section.view.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-email-section.view.tsx @@ -13,6 +13,8 @@ import type { UserProfileEmailSectionViewProps } from './user-profile-email-sect export function UserProfileEmailSectionView({ emails, canAdd, + isDisabled, + pendingPrimaryId, verification, error, rowRef, @@ -35,6 +37,7 @@ export function UserProfileEmailSectionView({ color='neutral' size='sm' variant='outline' + disabled={isDisabled} /> } > @@ -59,6 +62,9 @@ export function UserProfileEmailSectionView({ onRemove={remove ? email => remove.open(email) : undefined} onSetPrimary={onSetPrimary} onVerify={onVerify} + isDisabled={isDisabled} + pendingPrimaryId={pendingPrimaryId} + pendingVerifyId={verification.pendingVerifyId} > {error} @@ -73,6 +79,7 @@ export function UserProfileEmailSectionView({ {remove ? ( fill(email.isVerified ? m.email.removeDialog.verifiedDescription : m.email.removeDialog.description, { diff --git a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa-management.feature.test.tsx b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa-management.feature.test.tsx index f146797e6cf..48a0d213e65 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa-management.feature.test.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa-management.feature.test.tsx @@ -367,6 +367,41 @@ describe('User profile MFA management', () => { } }); + it('blocks backup-code regeneration while the default SMS method is changing', async () => { + const first = fapiPhoneNumber({ ...phone, reserved_for_second_factor: true, default_second_factor: true }); + const second = fapiPhoneNumber({ + ...phone, + id: 'phone_2', + phone_number: '+15555550202', + reserved_for_second_factor: true, + }); + const fapi = await renderMfa( + fapiUser({ id: 'user_1', phone_numbers: [first, second], backup_code_enabled: true, two_factor_enabled: true }), + ); + const held = holdRequests('post', '/v1/me/phone_numbers/phone_2'); + const user = userEvent.setup(); + try { + await openMenu(user, screen.getByRole('button', { name: 'Manage SMS verification +1 (555) 555-0202' })); + await user.click(screen.getByRole('menuitem', { name: 'Set as default' })); + await waitFor(() => expect(held.requests).toHaveLength(1)); + await waitFor(() => expect(screen.queryByRole('menu')).toBeNull()); + const manage = screen.getByRole('button', { name: 'Manage Backup codes' }); + expect(manage).toHaveAttribute('aria-disabled', 'true'); + await user.click(manage); + expect(screen.queryByRole('menu')).toBeNull(); + expect(fapi.mfa.backupCodeCreations).toBe(0); + expect(screen.getByRole('button', { name: 'Add verification method' })).toBeDisabled(); + } finally { + held.release(); + } + const manage = screen.getByRole('button', { name: 'Manage Backup codes' }); + await waitFor(() => expect(manage).not.toHaveAttribute('aria-disabled', 'true')); + await openMenu(user, manage); + await user.click(screen.getByRole('menuitem', { name: 'Regenerate' })); + await waitFor(() => expect(screen.getByText('CODE0100')).toBeVisible()); + expect(fapi.mfa.backupCodeCreations).toBe(1); + }); + it('returns failed regeneration to the section and retries from the menu', async () => { const fapi = await renderMfa( fapiUser({ id: 'user_1', totp_enabled: true, backup_code_enabled: true, two_factor_enabled: true }), @@ -865,6 +900,12 @@ describe('User profile MFA management', () => { expect(screen.getAllByText('SMS verification')[0]?.closest('li')).toHaveTextContent('+1 (555) 555-0202'), ); expect(screen.getAllByText('SMS verification')[0]?.closest('li')).toHaveTextContent('Default'); + await waitFor(() => + expect(screen.getByRole('button', { name: 'Manage SMS verification +1 (555) 555-0101' })).not.toHaveAttribute( + 'aria-disabled', + 'true', + ), + ); await user.click(screen.getByRole('button', { name: 'Manage SMS verification +1 (555) 555-0101' })); await waitFor(() => expect(screen.getByRole('menuitem', { name: 'Set as default' })).toBeVisible()); }); @@ -908,6 +949,12 @@ describe('User profile MFA management', () => { expect(fapi.mfa.phoneUpdates).toContainEqual({ id: 'phone_2', default: true, reserved: undefined }), ); expect(attempts).toBe(2); + await waitFor(() => + expect(screen.getByRole('button', { name: 'Manage SMS verification +1 (555) 555-0101' })).not.toHaveAttribute( + 'aria-disabled', + 'true', + ), + ); await user.click(screen.getByRole('button', { name: 'Manage SMS verification +1 (555) 555-0101' })); await waitFor(() => expect(screen.getByRole('menuitem', { name: 'Set as default' })).toBeVisible()); }); @@ -916,7 +963,7 @@ describe('User profile MFA management', () => { { phase: 'mutation', method: 'post', path: '/v1/me/phone_numbers/phone_2', pendingPhone: '+1 (555) 555-0202' }, { phase: 'refresh', method: 'get', path: '/v1/me', pendingPhone: '+1 (555) 555-0101' }, ] as const)( - 'keeps method menus available but hides default changes during $phase', + 'disables method menus while changing the default during $phase', async ({ method, path, pendingPhone }) => { const first = fapiPhoneNumber({ ...phone, reserved_for_second_factor: true, default_second_factor: true }); const second = fapiPhoneNumber({ @@ -934,16 +981,21 @@ describe('User profile MFA management', () => { await waitFor(() => expect(held.requests).toHaveLength(1)); try { const pendingAction = screen.getByRole('button', { name: `Manage SMS verification ${pendingPhone}` }); - await openMenu(user, pendingAction); - await waitFor(() => expect(screen.getByRole('menuitem', { name: 'Remove method' })).toBeVisible()); - expect(screen.queryByRole('menuitem', { name: 'Set as default' })).toBeNull(); - await user.click(await screen.findByRole('menuitem', { name: 'Remove method' })); + expect(pendingAction).toHaveAttribute('aria-disabled', 'true'); + await user.click(pendingAction); + expect(screen.queryByRole('menu')).toBeNull(); expect(screen.queryByRole('alertdialog')).toBeNull(); } finally { held.release(); } await waitFor(() => expect(screen.getByText('+1 (555) 555-0202').closest('li')).toHaveTextContent('Default')); await user.keyboard('{Escape}'); + await waitFor(() => + expect(screen.getByRole('button', { name: 'Manage SMS verification +1 (555) 555-0101' })).not.toHaveAttribute( + 'aria-disabled', + 'true', + ), + ); await user.click(screen.getByRole('button', { name: 'Manage SMS verification +1 (555) 555-0101' })); await waitFor(() => expect(screen.getByRole('menuitem', { name: 'Set as default' })).toBeVisible()); }, diff --git a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-row.view.tsx b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-row.view.tsx index a4496159858..0f656bf74e9 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-row.view.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-row.view.tsx @@ -11,12 +11,14 @@ import type { UserProfileMfaMethod, UserProfileMfaSectionViewProps } from './use export function UserProfileMfaRowView({ method, + isDisabled, triggerRef, onRemove, onSetDefault, regenerate, }: Pick & { method: UserProfileMfaMethod; + isDisabled?: boolean; triggerRef?: Ref; onRemove?: () => void; onSetDefault?: (id: string) => void; @@ -56,6 +58,8 @@ export function UserProfileMfaRowView({ {actions.length > 0 ? ( (null); const removalFocus = useListRemovalFocus({ ids: methods.map(method => method.id), @@ -23,11 +27,12 @@ export function useUserProfileMfaSectionLeafController({ sectionRef.current?.querySelector('button:not([disabled])') ?? sectionRef.current, }); const remove = useConfirmController({ + actionLock: lock, onConfirm: method => removalFocus.remove(method.id), reverification: removeReverification, }); const [pickerOpen, setPickerOpen] = useState(false); - const defaultAction = usePendingAction({ errorFallback: m.setDefaultError }); + const defaultAction = usePendingAction({ actionLock: lock, errorFallback: m.setDefaultError }); const setDefault = (id: string) => { const method = methods.find(method => method.id === id); @@ -39,6 +44,7 @@ export function useUserProfileMfaSectionLeafController({ return { sectionRef, + isDisabled: lock.isPending, pickerOpen, onPickerOpenChange: setPickerOpen, addMethod: onAdd @@ -49,7 +55,7 @@ export function useUserProfileMfaSectionLeafController({ : undefined, registerTrigger: removalFocus.registerTrigger, openRemoval: (method: UserProfileMfaMethod) => { - if (defaultAction.isPending) { + if (lock.isPending) { return; } defaultAction.reset(); @@ -59,6 +65,6 @@ export function useUserProfileMfaSectionLeafController({ finalRemovalFocus: removalFocus.finalFocus, setDefaultFinalFocus: () => (defaultAction.lastKey ? removalFocus.trigger(defaultAction.lastKey) : null), defaultError: defaultAction.error, - setDefault: onSetDefault && !defaultAction.isPending ? setDefault : undefined, + setDefault: onSetDefault ? setDefault : undefined, }; } diff --git a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.controller.ts b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.controller.ts index d6311ba4808..61ceddbfb0f 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.controller.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.controller.ts @@ -1,6 +1,7 @@ import { isReverificationCancelledError } from '@clerk/shared/error'; import { useEffect } from 'react'; +import { useActionLock } from '../../../hooks/use-action-lock'; import { type ErrorDescription, fill, type MosaicMessages, useErrorText, useMessages } from '../../../localization'; import { setup } from '../../../machine/setup'; import type { DoneInvokeEvent } from '../../../machine/types'; @@ -504,6 +505,7 @@ function createSetupProps( } export function useUserProfileMfaController(model: ReadyModel) { + const actionLock = useActionLock(); const errorText = useErrorText(); const backupCodeMessages = useMessages('userProfileBackupCodes'); const [{ context, value }, send] = useMachine(mfaMachine); @@ -512,9 +514,14 @@ export function useUserProfileMfaController(model: ReadyModel) { if (isBusy) { return Promise.resolve(); } - return new Promise((resolve, reject) => { - send({ type: 'RUN', key, methods: model.addableMethods, run: action, resolve, reject }); - }); + return ( + actionLock.tryRun( + () => + new Promise((resolve, reject) => { + send({ type: 'RUN', key, methods: model.addableMethods, run: action, resolve, reject }); + }), + ) ?? Promise.resolve() + ); }; const { resendSeconds } = context; const flow = context.flow; @@ -585,6 +592,7 @@ export function useUserProfileMfaController(model: ReadyModel) { ); const sectionProps: UserProfileMfaSectionViewProps = { + actionLock, methods: [...model.methods], addableMethods: model.addableMethods, regenerate: model.generateBackupCodes @@ -607,7 +615,7 @@ export function useUserProfileMfaController(model: ReadyModel) { dialogOpen, showAddTrigger, showAddControl: showAddTrigger || dialogOpen, - isPending: isBusy, + isPending: actionLock.isPending, reverifying, onDialogOpenChange: (open: boolean) => { if (!open && !canClose) { diff --git a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.view.tsx b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.view.tsx index a5048a26df8..318358b5023 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.view.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.view.tsx @@ -4,6 +4,7 @@ import type { ReactNode, Ref } from 'react'; import { Confirmation } from '../../../blocks/confirmation'; import { ReverificationDialog } from '../../../blocks/reverification'; import { Section } from '../../../components/section'; +import type { ActionLock } from '../../../hooks/use-action-lock'; import { fill, type MosaicMessages, useMessages } from '../../../localization'; import type { ReverificationController } from '../../reverification'; import { UserProfileSecurityList } from '../user-profile-security-list'; @@ -17,6 +18,7 @@ export type { UserProfileMfaAddableMethod, UserProfileMfaMethod } from './user-p export interface UserProfileMfaSectionViewProps { methods: UserProfileMfaMethod[]; + actionLock?: ActionLock; addableMethods?: readonly UserProfileMfaAddableMethod[]; addButtonRef?: Ref; addControl?: ReactNode; @@ -30,6 +32,7 @@ export interface UserProfileMfaSectionViewProps { export function UserProfileMfaSectionView({ methods, + actionLock, addableMethods, addButtonRef, addControl, @@ -43,6 +46,7 @@ export function UserProfileMfaSectionView({ const m = useMessages('userProfileMfa'); const controller = useUserProfileMfaSectionLeafController({ methods, + actionLock, onAdd, onRemove, onSetDefault, @@ -57,6 +61,7 @@ export function UserProfileMfaSectionView({ addControl ?? (controller.addMethod && addableMethods?.length ? ( controller.openRemoval(method) : undefined} onSetDefault={controller.setDefault} @@ -88,6 +94,7 @@ export function UserProfileMfaSectionView({ {controller.remove ? ( (method.type === 'sms' ? m.removeDialog.smsTitle : m.removeDialog.authenticatorTitle)} description={method => describeMethodRemoval(method, m)} actionLabel={m.removeDialog.confirm} diff --git a/packages/mosaic/src/features/user-profile/user-profile-phone-section/__tests__/user-profile-phone-section.feature.test.tsx b/packages/mosaic/src/features/user-profile/user-profile-phone-section/__tests__/user-profile-phone-section.feature.test.tsx index 1fd442dabb0..a0e56983b35 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-phone-section/__tests__/user-profile-phone-section.feature.test.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-phone-section/__tests__/user-profile-phone-section.feature.test.tsx @@ -92,7 +92,37 @@ async function enterCode(actor: Actor, code: string) { } describe('the user profile phone section', () => { - it('lists the primary number first and marks it', async () => { + it('holds the menu and row order while setting a number as primary', async () => { + const { actor } = await renderSection(signedIn([HOME, WORK])); + const initialOrder = listed(); + const primary = holdRequests('post', '/v1/me'); + try { + await manage(actor, WORK_LABEL, 'Set as primary'); + await waitFor(() => expect(primary.requests).toHaveLength(1)); + const menu = screen.getByRole('menu'); + const item = within(menu).getByRole('menuitem', { name: 'Set as primary' }); + expect(item).toHaveAttribute('aria-busy', 'true'); + const manageWork = screen.getByRole('button', { name: `Manage ${WORK_LABEL}` }); + expect(manageWork).toHaveAttribute('aria-disabled', 'true'); + expect(within(menu).getByRole('menuitem', { name: 'Remove phone number' })).toHaveAttribute( + 'aria-disabled', + 'true', + ); + expect(screen.queryByRole('alertdialog')).toBeNull(); + expect(screen.getByRole('button', { name: 'Add phone number' })).toBeDisabled(); + expect(await within(item).findByRole('progressbar', { name: 'Set as primary' })).toBeVisible(); + expect(listed()).toEqual(initialOrder); + } finally { + primary.release(); + await waitFor(() => expect(within(row()).getByText(WORK_LABEL).parentElement).toHaveTextContent('Primary')); + } + expect(screen.getByRole('button', { name: `Manage ${WORK_LABEL}` })).not.toHaveAttribute('aria-disabled', 'true'); + await waitFor(() => expect(screen.queryByRole('menu')).toBeNull()); + expect(listed()).toEqual(initialOrder); + expect(screen.queryByRole('progressbar', { name: 'Set as primary' })).toBeNull(); + }); + + it('lists verified numbers in order and marks the primary', async () => { await renderSection(signedIn([HOME, WORK])); expect(listed()).toEqual([HOME_LABEL, WORK_LABEL]); @@ -155,12 +185,34 @@ describe('the user profile phone section', () => { await waitFor(() => expect(within(row()).getByText(WORK_LABEL).parentElement).not.toHaveTextContent('Unverified')); }); + it('holds the menu open with a spinner on Verify until the code is sent', async () => { + const pending = fapiPhoneNumber({ id: 'idn_new', phone_number: '+18015550199' }); + const { actor } = await renderSection(signedIn([HOME, pending])); + const prepare = holdRequests('post', '/v1/me/phone_numbers/:id/prepare_verification'); + + try { + await manage(actor, WORK_LABEL, 'Verify phone number'); + await waitFor(() => expect(prepare.requests).toHaveLength(1)); + expect(screen.getByRole('menu')).toBeVisible(); + expect(screen.getByRole('menuitem', { name: 'Verify phone number' })).toHaveAttribute('aria-busy', 'true'); + expect(await screen.findByRole('progressbar', { name: 'Verify phone number' })).toBeVisible(); + expect(screen.queryByRole('dialog')).toBeNull(); + } finally { + prepare.release(); + } + + await enterCode(actor, VERIFICATION_CODE); + expect(screen.queryByRole('menu')).toBeNull(); + await waitFor(() => expect(screen.queryByRole('dialog')).not.toBeInTheDocument()); + }); + it('makes another verified number the primary one', async () => { const { actor } = await renderSection(signedIn([HOME, WORK])); await manage(actor, WORK_LABEL, 'Set as primary'); - await waitFor(() => expect(listed()).toEqual([WORK_LABEL, HOME_LABEL])); + await waitFor(() => expect(within(row()).getByText(WORK_LABEL).parentElement).toHaveTextContent('Primary')); + expect(listed()).toEqual([HOME_LABEL, WORK_LABEL]); expect(within(row()).getByText(WORK_LABEL).parentElement).toHaveTextContent('Primary'); }); @@ -459,6 +511,7 @@ describe('phone reverification', () => { expect(within(row()).queryByRole('alert')).toBeNull(); expect(within(row()).getByRole('button', { name: `Manage ${WORK_LABEL}` })).toHaveFocus(); + await waitFor(() => expect(screen.queryByRole('menu')).toBeNull()); await manage(actor, WORK_LABEL, 'Set as primary'); await waitFor(() => expect(fapi.client.sessions[0]?.user?.primary_phone_number_id).toBe('idn_work')); }); diff --git a/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-add-phone.controller.ts b/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-add-phone.controller.ts index 7e6bf1d0a0b..12690e02cd0 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-add-phone.controller.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-add-phone.controller.ts @@ -23,6 +23,7 @@ export interface UserProfileAddPhoneControllerOptions { export interface UserProfileAddPhoneController extends UserProfileAddPhoneDialogProps { handle: DialogHandle; verifyingId: string | undefined; + pendingVerifyId: string | undefined; onVerifyPhone: (phone: Pick, verifier: UserProfilePhoneVerifier) => void; } @@ -63,6 +64,13 @@ const start = { })), }; +const sendCode = { + invoke: fromPromise((context: Context) => (context.verifier ? context.verifier.sendCode() : missingDependency()), { + onDone: { target: 'verify', actions: assign(() => ({ resendSeconds: CODE_RESEND_SECONDS })) }, + onError: { target: 'verify', actions: fail }, + }), +}; + const machine = createMachine({ id: 'addPhone', initial: 'idle', @@ -80,18 +88,14 @@ const machine = createMachine({ target: 'phone', actions: assign(() => ({ verifyingId: undefined, verifier: undefined, error: undefined, resendSeconds: 0 })), }, - START: start, + START: { ...start, target: 'preparing' }, }, }, phone: { on: { CANCEL: 'idle', START: start }, }, - sending: { - invoke: fromPromise(context => (context.verifier ? context.verifier.sendCode() : missingDependency()), { - onDone: { target: 'verify', actions: assign(() => ({ resendSeconds: CODE_RESEND_SECONDS })) }, - onError: { target: 'verify', actions: fail }, - }), - }, + preparing: sendCode, + sending: sendCode, verify: { on: { CANCEL: 'idle', @@ -134,7 +138,7 @@ export function useUserProfileAddPhoneController({ }, }); const state = snapshot.value; - const open = state !== 'idle'; + const open = state !== 'idle' && state !== 'preparing'; useEffect(() => { if (!open || resendSeconds === 0) { return; @@ -195,6 +199,7 @@ export function useUserProfileAddPhoneController({ } }, verifyingId: snapshot.context.verifyingId, + pendingVerifyId: state === 'preparing' ? snapshot.context.verifyingId : undefined, onVerifyPhone: (phone, verifier) => { codeForm.reset(); send({ type: 'START', id: phone.id, phoneNumber: phone.value, verifier }); diff --git a/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-phone-section.controller.ts b/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-phone-section.controller.ts index 9419a425010..08cf24cbc8b 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-phone-section.controller.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-phone-section.controller.ts @@ -2,6 +2,7 @@ import { stringToFormattedPhoneString } from '@clerk/shared/phone'; import { useRef } from 'react'; import { useConfirmController } from '../../../blocks/confirm'; +import { useActionLock } from '../../../hooks/use-action-lock'; import { useListRemovalFocus } from '../../../hooks/use-list-removal-focus'; import { usePendingAction } from '../../../hooks/use-pending-action'; import type { UserProfilePhone } from '../user-profile-contact.types'; @@ -23,7 +24,8 @@ export function useUserProfilePhoneSectionController({ onCreate: onCreatePhone, reverification: createReverification, }); - const setPrimary = usePendingAction(); + const actionLock = useActionLock(); + const setPrimary = usePendingAction({ actionLock }); const rowRef = useRef(null); const removalFocus = useListRemovalFocus({ ids: phones.map(phone => phone.id), @@ -31,6 +33,7 @@ export function useUserProfilePhoneSectionController({ fallback: () => rowRef.current?.querySelector('button:not([disabled])') ?? rowRef.current, }); const remove = useConfirmController({ + actionLock, onConfirm: phone => removalFocus.remove(phone.id), reverification: removeReverification, }); @@ -41,6 +44,8 @@ export function useUserProfilePhoneSectionController({ canAdd: Boolean(onCreatePhone), verification, error: setPrimary.error, + isDisabled: actionLock.isPending, + pendingPrimaryId: setPrimary.pendingKey, rowRef, registerTrigger: removalFocus.registerTrigger, remove: onRemovePhone ? remove : undefined, diff --git a/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-phone-section.types.ts b/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-phone-section.types.ts index b9a659e3f4c..d4abc3d5a6b 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-phone-section.types.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-phone-section.types.ts @@ -11,6 +11,8 @@ export interface UserProfilePhoneSectionViewProps { phones: UserProfilePhone[]; defaultPhoneCountry?: CountryIso; canAdd: boolean; + isDisabled: boolean; + pendingPrimaryId: string | undefined; verification: UserProfileAddPhoneController; error: string | undefined; rowRef: Ref; diff --git a/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-phone-section.view.tsx b/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-phone-section.view.tsx index cd13d1f4408..3acdf8c8bc1 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-phone-section.view.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-phone-section.view.tsx @@ -14,6 +14,8 @@ export function UserProfilePhoneSectionView({ phones, defaultPhoneCountry, canAdd, + isDisabled, + pendingPrimaryId, verification, error, rowRef, @@ -36,6 +38,7 @@ export function UserProfilePhoneSectionView({ color='neutral' size='sm' variant='outline' + disabled={isDisabled} /> } > @@ -60,6 +63,9 @@ export function UserProfilePhoneSectionView({ onRemove={remove ? phone => remove.open(phone) : undefined} onSetPrimary={onSetPrimary} onVerify={onVerify} + isDisabled={isDisabled} + pendingPrimaryId={pendingPrimaryId} + pendingVerifyId={verification.pendingVerifyId} > {error} @@ -75,6 +81,7 @@ export function UserProfilePhoneSectionView({ {remove ? ( fill(phone.isVerified ? m.phone.removeDialog.verifiedDescription : m.phone.removeDialog.description, { diff --git a/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/__tests__/user-profile-web3-wallets-section.feature.test.tsx b/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/__tests__/user-profile-web3-wallets-section.feature.test.tsx index cb9fc8857d2..4f120f3ad34 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/__tests__/user-profile-web3-wallets-section.feature.test.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/__tests__/user-profile-web3-wallets-section.feature.test.tsx @@ -464,7 +464,7 @@ describe('Web3 wallets', () => { await waitFor(() => expect(screen.getByText('Primary')).toBeInTheDocument()); }); - it('sets a verified wallet as primary through Clerk', async () => { + it('holds the wallet menu while setting primary and updates the badge in place', async () => { const fapi = await renderWeb3({ web3_wallets: [ fapiWeb3Wallet({ id: 'wallet_1', web3_wallet: '0x1234567890abcdef' }), @@ -475,13 +475,32 @@ describe('Web3 wallets', () => { const user = userEvent.setup(); await waitFor(() => expect(screen.getAllByRole('button', { name: 'Manage MetaMask' })).toHaveLength(2)); + expect(screen.getAllByRole('button', { name: 'Manage MetaMask' })[1].closest('.cl-section-row')).toContainElement( + screen.getByTitle('0xabcdef1234567890'), + ); await openMenu(user, screen.getAllByRole('button', { name: 'Manage MetaMask' })[0]); expect(screen.queryByRole('menuitem', { name: 'Set as primary' })).toBeNull(); await user.keyboard('{Escape}'); await openMenu(user, screen.getAllByRole('button', { name: 'Manage MetaMask' })[1]); - await user.click(screen.getByRole('menuitem', { name: 'Set as primary' })); + const primary = holdRequests('post', '/v1/me'); + try { + await user.click(screen.getByRole('menuitem', { name: 'Set as primary' })); + await waitFor(() => expect(primary.requests).toHaveLength(1)); + const menu = screen.getByRole('menu'); + const item = within(menu).getByRole('menuitem', { name: 'Set as primary' }); + expect(item).toHaveAttribute('aria-busy', 'true'); + expect(within(menu).getByRole('menuitem', { name: 'Remove wallet' })).toHaveAttribute('aria-disabled', 'true'); + expect(await within(item).findByRole('progressbar', { name: 'Set as primary' })).toBeVisible(); + expect(screen.getByRole('button', { name: 'Connect Solana' })).toBeDisabled(); + } finally { + primary.release(); + } await waitFor(() => expect(fapi.client.sessions[0]?.user.primary_web3_wallet_id).toBe('wallet_2')); + await waitFor(() => expect(screen.queryByRole('menu')).toBeNull()); + expect(screen.getAllByRole('button', { name: 'Manage MetaMask' })[1].closest('.cl-section-row')).toContainElement( + screen.getByTitle('0xabcdef1234567890'), + ); const selectedRow = screen.getByTitle('0xabcdef1234567890').closest('.cl-section-row'); expect(selectedRow).not.toBeNull(); if (selectedRow) { @@ -604,6 +623,7 @@ describe('Web3 wallets', () => { expect(screen.queryByRole('alert')).toBeNull(); await waitFor(() => expect(secondManage()).toHaveFocus()); + await waitFor(() => expect(screen.queryByRole('menu')).toBeNull()); await openMenu(user, secondManage()); await user.click(screen.getByRole('menuitem', { name: 'Set as primary' })); await waitFor(() => expect(fapi.client.sessions[0]?.user.primary_web3_wallet_id).toBe('wallet_2')); @@ -735,14 +755,14 @@ describe('Web3 wallets', () => { primary_web3_wallet_id: 'wallet_2', }); const user = userEvent.setup(); - const first = screen.getAllByRole('button', { name: 'Manage MetaMask' })[1]; + const first = screen.getAllByRole('button', { name: 'Manage MetaMask' })[0]; first.focus(); await user.keyboard('{Enter}'); await user.click(await screen.findByRole('menuitem', { name: 'Remove wallet' })); await user.keyboard('{Escape}'); await waitFor(() => expect(screen.queryByRole('alertdialog')).toBeNull()); expect(fapi.client.sessions[0]?.user.web3_wallets).toHaveLength(2); - await openMenu(user, screen.getAllByRole('button', { name: 'Manage MetaMask' })[0]); + await openMenu(user, screen.getAllByRole('button', { name: 'Manage MetaMask' })[1]); await user.click(screen.getByRole('menuitem', { name: 'Remove wallet' })); expect(screen.getByRole('alertdialog')).toHaveTextContent('0xabcd...7890'); await user.click(screen.getByRole('button', { name: 'Remove' })); diff --git a/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/__tests__/user-profile-web3-wallets-section.model.test.ts b/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/__tests__/user-profile-web3-wallets-section.model.test.ts index 60bfde88c35..dc2f236ba7a 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/__tests__/user-profile-web3-wallets-section.model.test.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/__tests__/user-profile-web3-wallets-section.model.test.ts @@ -121,7 +121,7 @@ describe('Web3 wallet projection', () => { } }); - it('WEB3-04 orders primary, verified, then unverified wallets without mutating the input', () => { + it('WEB3-04 orders verified, then unverified wallets without moving the primary or mutating the input', () => { const wallets = Object.freeze([ { ...metamask, @@ -133,7 +133,7 @@ describe('Web3 wallet projection', () => { { ...metamask, id: 'primary', - verification: { ...metamask.verification, status: 'unverified' as const }, + verification: { ...metamask.verification, status: null }, }, { ...metamask, @@ -147,17 +147,28 @@ describe('Web3 wallet projection', () => { enabledStrategies: ['web3_metamask_signature'], allowCreation: false, }); + const changedPrimary = projectWeb3Wallets({ + wallets, + primaryId: 'unverified_later', + enabledStrategies: ['web3_metamask_signature'], + allowCreation: false, + }); expect(result.status).toBe('ready'); if (result.status === 'ready') { expect(result.wallets.map(wallet => wallet.id)).toEqual([ - 'primary', 'verified_a', 'verified_z', 'unverified_earlier', 'unverified_later', + 'primary', ]); expect(result.availableProviders).toEqual([]); } + if (result.status === 'ready' && changedPrimary.status === 'ready') { + expect(changedPrimary.wallets.map(wallet => wallet.id)).toEqual(result.wallets.map(wallet => wallet.id)); + expect(changedPrimary.wallets.find(wallet => wallet.id === 'unverified_later')?.isPrimary).toBe(true); + expect(changedPrimary.wallets.find(wallet => wallet.id === 'primary')?.isPrimary).toBe(false); + } expect(wallets.map(wallet => wallet.id)).toEqual([ 'unverified_later', 'verified_z', diff --git a/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallet-row.view.tsx b/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallet-row.view.tsx index e788761c532..553f747bf05 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallet-row.view.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallet-row.view.tsx @@ -18,6 +18,7 @@ export function UserProfileWeb3WalletRowView({ wallet, triggerRef, isPending = false, + isPrimaryPending = false, isDisabled = false, onConnect, onVerify, @@ -27,6 +28,7 @@ export function UserProfileWeb3WalletRowView({ wallet: UserProfileWeb3Wallet | UserProfileWeb3Provider; triggerRef?: Ref; isPending?: boolean; + isPrimaryPending?: boolean; isDisabled?: boolean; onConnect?: (id: string) => void; onVerify?: (id: string) => void; @@ -43,7 +45,11 @@ export function UserProfileWeb3WalletRowView({ actions.push({ label: m.verify, onClick: () => onVerify(wallet.id) }); } if (linkedWallet && !linkedWallet.isPrimary && linkedWallet.isVerified && onSetPrimary) { - actions.push({ label: m.setPrimary, onClick: () => onSetPrimary(wallet.id) }); + actions.push({ + label: m.setPrimary, + onClick: () => onSetPrimary(wallet.id), + status: isPrimaryPending ? 'pending' : 'idle', + }); } if (linkedWallet && onRemove && linkedWallet.canRemove !== false) { actions.push({ label: m.remove, color: 'negative', onClick: () => onRemove(linkedWallet) }); diff --git a/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.controller.ts b/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.controller.ts index 4fba28cb105..6daca8a1e94 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.controller.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.controller.ts @@ -2,6 +2,7 @@ import { useRef, useState } from 'react'; import { useConfirmController } from '../../../blocks/confirm'; import { usePanelTitle } from '../../../components/panel'; +import { useActionLock } from '../../../hooks/use-action-lock'; import { useListRemovalFocus } from '../../../hooks/use-list-removal-focus'; import { usePendingAction } from '../../../hooks/use-pending-action'; import { useMessages } from '../../../localization'; @@ -45,7 +46,8 @@ export function useUserProfileWeb3WalletsController({ | 'createReverification' >) { const messages = useMessages('userProfileWeb3Wallets'); - const action = usePendingAction({ errorFallback: messages.errors.generic }); + const actionLock = useActionLock(); + const action = usePendingAction({ actionLock, errorFallback: messages.errors.generic }); const [picker, setPicker] = useState({ open: false }); const [owner, setOwner] = useState(); const sectionRef = useRef(null); @@ -58,8 +60,8 @@ export function useUserProfileWeb3WalletsController({ sectionRef.current ?? panelTitle(), }); - // TODO: Share the action lock during removal while preserving errors for the confirmation dialog. const removeWallet = useConfirmController({ + actionLock, onConfirm: wallet => removalFocus.remove(wallet.id), reverification: removeReverification, }); @@ -170,6 +172,8 @@ export function useUserProfileWeb3WalletsController({ ? action.error : undefined, pendingId, + pendingPrimaryId: owner?.kind === 'primary' ? pendingId : undefined, + isDisabled: actionLock.isPending, sectionRef, registerTrigger: removalFocus.registerTrigger, remove: remove ? removeWallet : undefined, diff --git a/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.model.ts b/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.model.ts index 9da99dda1c5..965a41e7bfe 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.model.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.model.ts @@ -45,13 +45,11 @@ export function projectWeb3Wallets({ wallet.verification.strategy === 'admin' || WEB3_PROVIDERS.some(provider => provider.strategy === wallet.verification.strategy), ); - const primary = knownWallets.filter(wallet => wallet.id === primaryId); - const remaining = knownWallets.filter(wallet => wallet.id !== primaryId); - const verified = remaining.filter(wallet => wallet.verification.status === 'verified'); - const unverified = remaining.filter( + const verified = knownWallets.filter(wallet => wallet.verification.status === 'verified'); + const unverified = knownWallets.filter( wallet => wallet.verification.status !== null && wallet.verification.status !== 'verified', ); - const withoutStatus = remaining.filter(wallet => wallet.verification.status === null); + const withoutStatus = knownWallets.filter(wallet => wallet.verification.status === null); verified.sort((first, second) => first.id.localeCompare(second.id)); unverified.sort((first, second) => { @@ -60,7 +58,7 @@ export function projectWeb3Wallets({ return firstExpiry && secondExpiry ? firstExpiry.getTime() - secondExpiry.getTime() : 0; }); - const sorted = [...primary, ...verified, ...unverified, ...withoutStatus]; + const sorted = [...verified, ...unverified, ...withoutStatus]; const connectedStrategies = new Set(wallets.map(wallet => wallet.verification.strategy)); diff --git a/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.tsx b/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.tsx index 38f77348d3a..9a9eb2cab3d 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.tsx @@ -35,6 +35,8 @@ function Web3Wallets({ model }: { model: ReadyWeb3WalletsModel }) { sectionRef={controller.sectionRef} availableProviders={controller.availableProviders} pendingId={controller.pendingId} + pendingPrimaryId={controller.pendingPrimaryId} + isDisabled={controller.isDisabled} error={controller.error} onConnect={id => { void controller.onConnect(id); diff --git a/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.types.ts b/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.types.ts index a0ce8fec689..87c625e2391 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.types.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.types.ts @@ -37,6 +37,8 @@ export interface UserProfileWeb3WalletsSectionViewProps { sectionRef?: Ref; availableProviders?: UserProfileWeb3Provider[]; pendingId?: string; + pendingPrimaryId?: string; + isDisabled?: boolean; error?: string; onConnect?: (id: string) => void; onVerify?: (id: string) => void; diff --git a/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.view.tsx b/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.view.tsx index bd259076163..fdd044c14d8 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.view.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-web3-wallets-section/user-profile-web3-wallets-section.view.tsx @@ -11,6 +11,8 @@ export function UserProfileWeb3WalletsSectionView({ sectionRef, availableProviders = [], pendingId, + pendingPrimaryId, + isDisabled = false, error, onConnect, onVerify, @@ -25,7 +27,7 @@ export function UserProfileWeb3WalletsSectionView({ }: UserProfileWeb3WalletsSectionViewProps) { const m = useMessages('userProfileWeb3Wallets'); const hasRows = wallets.length > 0 || (availableProviders.length > 0 && Boolean(onConnect)) || Boolean(error); - const isBusy = pendingId !== undefined; + const isBusy = isDisabled || pendingId !== undefined; return ( <> @@ -47,6 +49,7 @@ export function UserProfileWeb3WalletsSectionView({ wallet={wallet} triggerRef={registerTrigger(wallet.id)} isPending={pendingId === wallet.id} + isPrimaryPending={pendingPrimaryId === wallet.id} isDisabled={isBusy} onVerify={onVerify} onSetPrimary={onSetPrimary} @@ -73,6 +76,7 @@ export function UserProfileWeb3WalletsSectionView({ {remove ? ( fill(wallet.isVerified ? m.removeDialog.verifiedDescription : m.removeDialog.description, { diff --git a/packages/mosaic/src/hooks/__tests__/use-action-lock.test.tsx b/packages/mosaic/src/hooks/__tests__/use-action-lock.test.tsx new file mode 100644 index 00000000000..4d589c1d23a --- /dev/null +++ b/packages/mosaic/src/hooks/__tests__/use-action-lock.test.tsx @@ -0,0 +1,139 @@ +import { act, renderHook } from '@testing-library/react'; +import { describe, expect, it, vi } from 'vitest'; + +import { deferred } from '../../__tests__/async'; +import { useConfirmController } from '../../blocks/confirm'; +import { SaveError } from '../../utils/errors'; +import { useActionLock } from '../use-action-lock'; +import { usePendingAction } from '../use-pending-action'; + +function useOwners(onConfirm: () => Promise) { + const actionLock = useActionLock(); + const primary = usePendingAction({ actionLock }); + const remove = useConfirmController({ + actionLock, + onConfirm, + reverification: { status: 'loading', visible: true, onCancel: vi.fn() }, + }); + return { actionLock, primary, remove }; +} + +describe('shared action admission', () => { + it('refuses removal in the same stack as primary without changing the dialog', async () => { + const pending = deferred(); + const onConfirm = vi.fn(() => Promise.resolve()); + const { result } = renderHook(() => useOwners(onConfirm)); + act(() => result.current.remove.onOpenChange(true)); + const owners = result.current; + + act(() => { + void owners.primary.run('primary', () => pending.promise); + owners.remove.confirm(); + }); + + expect(onConfirm).not.toHaveBeenCalled(); + expect(result.current.remove.isOpen).toBe(true); + expect(result.current.remove.isPending).toBe(false); + expect(result.current.actionLock.isPending).toBe(true); + await act(() => pending.resolve()); + expect(result.current.actionLock.isPending).toBe(false); + await act(() => result.current.remove.confirm()); + expect(onConfirm).toHaveBeenCalledOnce(); + expect(result.current.remove.isOpen).toBe(false); + }); + + it('refuses primary in the same stack as removal without changing primary metadata', async () => { + const pending = deferred(); + const primary = vi.fn(); + const { result } = renderHook(() => useOwners(() => pending.promise)); + act(() => result.current.remove.onOpenChange(true)); + const owners = result.current; + + await act(async () => { + owners.remove.confirm(); + expect(await owners.primary.run('primary', primary)).toBe(false); + }); + + expect(primary).not.toHaveBeenCalled(); + expect(result.current.primary.lastKey).toBeUndefined(); + expect(result.current.primary.error).toBeUndefined(); + expect(result.current.actionLock.isPending).toBe(true); + await act(() => pending.resolve()); + await act(async () => expect(await owners.primary.run('primary', primary)).toBe(true)); + expect(primary).toHaveBeenCalledOnce(); + }); + + it('keeps a rejected removal error when another owner makes confirmation busy', async () => { + const pending = deferred(); + const failure = new SaveError({ global: { code: 'action_blocked', message: 'Removal refused.' } }); + const onConfirm = vi.fn(() => Promise.reject(failure)); + const { result } = renderHook(() => useOwners(onConfirm)); + act(() => result.current.remove.onOpenChange(true)); + await act(() => result.current.remove.confirm()); + const error = result.current.remove.errorMessage; + expect(error).toBeDefined(); + expect(result.current.remove.isOpen).toBe(true); + expect(result.current.primary.error).toBeUndefined(); + + act(() => { + void result.current.primary.run('primary', () => pending.promise); + result.current.remove.confirm(); + }); + + expect(onConfirm).toHaveBeenCalledOnce(); + expect(result.current.remove.errorMessage).toBe(error); + expect(result.current.remove.isOpen).toBe(true); + expect(result.current.remove.isPending).toBe(false); + await act(() => pending.resolve()); + }); + + it('holds admission after cancelling observation until the underlying removal settles', async () => { + const pending = deferred(); + const primary = vi.fn(); + const { result } = renderHook(() => useOwners(() => pending.promise)); + act(() => result.current.remove.onOpenChange(true)); + act(() => result.current.remove.confirm()); + act(() => result.current.remove.onOpenChange(false)); + expect(result.current.remove.isOpen).toBe(false); + await act(async () => expect(await result.current.primary.run('primary', primary)).toBe(false)); + expect(primary).not.toHaveBeenCalled(); + expect(result.current.actionLock.isPending).toBe(true); + + await act(() => pending.reject(new Error('Late removal failure'))); + expect(result.current.actionLock.isPending).toBe(false); + expect(result.current.remove.errorMessage).toBeUndefined(); + await act(async () => expect(await result.current.primary.run('primary', primary)).toBe(true)); + }); +}); + +describe('useActionLock', () => { + it('returns the accepted result and refuses reentrant work synchronously', async () => { + const { result } = renderHook(() => useActionLock()); + const other = vi.fn(); + await act(async () => { + const accepted = result.current.tryRun(() => { + expect(result.current.tryRun(other)).toBeUndefined(); + return 42; + }); + expect(await accepted).toBe(42); + }); + expect(other).not.toHaveBeenCalled(); + expect(result.current.isPending).toBe(false); + }); + + it.each(['throw', 'reject'])('preserves the original %s and releases admission', async mode => { + const { result } = renderHook(() => useActionLock()); + const failure = new Error('Original failure'); + await act(async () => { + const accepted = result.current.tryRun(() => { + if (mode === 'throw') { + throw failure; + } + return Promise.reject(failure); + }); + await expect(accepted).rejects.toBe(failure); + }); + expect(result.current.isPending).toBe(false); + await act(async () => expect(await result.current.tryRun(() => 'retry')).toBe('retry')); + }); +}); diff --git a/packages/mosaic/src/hooks/use-action-lock.ts b/packages/mosaic/src/hooks/use-action-lock.ts new file mode 100644 index 00000000000..e112d93ad84 --- /dev/null +++ b/packages/mosaic/src/hooks/use-action-lock.ts @@ -0,0 +1,29 @@ +import { useRef, useState } from 'react'; + +export interface ActionLock { + isPending: boolean; + tryRun: (action: () => T | Promise) => Promise | undefined; +} + +export function useActionLock(): ActionLock { + const running = useRef(false); + const [isPending, setIsPending] = useState(false); + + const tryRun = (action: () => T | Promise): Promise | undefined => { + if (running.current) { + return undefined; + } + running.current = true; + setIsPending(true); + return (async () => { + try { + return await action(); + } finally { + running.current = false; + setIsPending(false); + } + })(); + }; + + return { isPending, tryRun }; +} diff --git a/packages/mosaic/src/hooks/use-list-removal-focus.ts b/packages/mosaic/src/hooks/use-list-removal-focus.ts index 73621b38222..38a4604ede1 100644 --- a/packages/mosaic/src/hooks/use-list-removal-focus.ts +++ b/packages/mosaic/src/hooks/use-list-removal-focus.ts @@ -1,3 +1,4 @@ +import type { RefCallback } from 'react'; import { useRef } from 'react'; export function useListRemovalFocus({ @@ -10,14 +11,23 @@ export function useListRemovalFocus({ fallback: () => HTMLElement | null; }) { const triggers = useRef(new Map()); + const triggerRefs = useRef(new Map>()); const removed = useRef<{ id: string; index: number } | undefined>(undefined); - const registerTrigger = (id: string) => (element: HTMLButtonElement | null) => { - if (element) { - triggers.current.set(id, element); - } else { - triggers.current.delete(id); + const registerTrigger = (id: string) => { + const existing = triggerRefs.current.get(id); + if (existing) { + return existing; } + const ref = (element: HTMLButtonElement | null) => { + if (element) { + triggers.current.set(id, element); + } else { + triggers.current.delete(id); + } + }; + triggerRefs.current.set(id, ref); + return ref; }; const remove = async (id: string) => { diff --git a/packages/mosaic/src/hooks/use-pending-action.ts b/packages/mosaic/src/hooks/use-pending-action.ts index bfeb88f3274..b898cdf274c 100644 --- a/packages/mosaic/src/hooks/use-pending-action.ts +++ b/packages/mosaic/src/hooks/use-pending-action.ts @@ -1,15 +1,21 @@ import { isReverificationCancelledError } from '@clerk/shared/error'; -import { useRef, useState } from 'react'; +import { useState } from 'react'; import type { ErrorDescription } from '../localization'; import { useErrorText } from '../localization'; import { toLocalizableError } from '../utils/errors'; +import type { ActionLock } from './use-action-lock'; +import { useActionLock } from './use-action-lock'; export interface PendingActionOptions { /** Copy shown when the action fails without an error Clerk can describe, such as a network or code fault (default: the generic error) */ errorFallback?: string; } +export interface PendingActionHookOptions extends PendingActionOptions { + actionLock?: ActionLock; +} + export interface PendingAction { run: (key: TKey, action: () => Promise | void, options?: PendingActionOptions) => Promise; pendingKey: TKey | undefined; @@ -32,33 +38,34 @@ interface PendingActionFailure { */ export function usePendingAction({ errorFallback, -}: PendingActionOptions = {}): PendingAction { + actionLock, +}: PendingActionHookOptions = {}): PendingAction { const errorText = useErrorText(); const [pendingKey, setPendingKey] = useState(); const [lastKey, setLastKey] = useState(); const [failure, setFailure] = useState>(); - const running = useRef(false); + const localLock = useActionLock(); + const lock = actionLock ?? localLock; const run = async (key: TKey, action: () => Promise | void, options: PendingActionOptions = {}) => { - if (running.current) { - return false; - } - running.current = true; - setPendingKey(key); - setLastKey(key); - setFailure(undefined); - try { - await action(); - return true; - } catch (cause) { - if (!isReverificationCancelledError(cause)) { - setFailure({ key, error: toLocalizableError(cause), errorFallback: options.errorFallback }); - } - return false; - } finally { - running.current = false; - setPendingKey(undefined); - } + return ( + lock.tryRun(async () => { + setPendingKey(key); + setLastKey(key); + setFailure(undefined); + try { + await action(); + return true; + } catch (cause) { + if (!isReverificationCancelledError(cause)) { + setFailure({ key, error: toLocalizableError(cause), errorFallback: options.errorFallback }); + } + return false; + } finally { + setPendingKey(undefined); + } + }) ?? false + ); }; return {