From b47a44fc304e0655c9020a654c0a4abf8110dbec Mon Sep 17 00:00:00 2001 From: austincalvelage Date: Fri, 23 Oct 2026 15:37:44 -0600 Subject: [PATCH 1/3] fix(mosaic): keep action menus pending until dialog handoff --- packages/mosaic/docs/controllers.md | 12 +++++ .../action-menu/action-menu.test.tsx | 15 +++++- .../components/action-menu/action-menu.tsx | 49 +++++++++++++++++-- 3 files changed, 70 insertions(+), 6 deletions(-) diff --git a/packages/mosaic/docs/controllers.md b/packages/mosaic/docs/controllers.md index ca7d158c2c5..c6c663658dc 100644 --- a/packages/mosaic/docs/controllers.md +++ b/packages/mosaic/docs/controllers.md @@ -104,6 +104,18 @@ section. The dialog opens once `visible` is true, so it skips the loading card, dismissal only while the controller has a cancel callback. Return focus with the hook's `lastKey`, which outlives the run, unlike `pendingKey`. +For a direct action in `ActionMenu`, derive its `progress` from the existing +pending key and the matching reverification controller. Pass `idle` before +activation, `pending` while the request or verification preparation is running, +and `handoff` when the dialog becomes visible. The menu keeps its selected item +and trailing spinner visible while pending, disables its actions, and blocks +dismissal until handoff or settlement. Ordinary failures close the menu and use +the section's existing error. Actions without `progress` still close immediately. +Keep the selected action and its callback present through data refreshes that +change eligibility. Disable other row triggers while the section is pending, +but keep the selected row's trigger available as the dialog's final-focus target. +The menu owns popup presentation only; `usePendingAction` still owns the operation. + A single open boolean earns nothing. A two-state machine with one event is a boolean spelled long. `useUserProfileEditUsernameController` holds `isOpen` in `useState` and hands the async part to `useForm`: diff --git a/packages/mosaic/src/components/action-menu/action-menu.test.tsx b/packages/mosaic/src/components/action-menu/action-menu.test.tsx index 166a868ecc6..821f532f35e 100644 --- a/packages/mosaic/src/components/action-menu/action-menu.test.tsx +++ b/packages/mosaic/src/components/action-menu/action-menu.test.tsx @@ -1,4 +1,4 @@ -import { render, screen } from '@testing-library/react'; +import { render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { describe, expect, it, vi } from 'vitest'; @@ -35,7 +35,20 @@ describe('Mosaic ActionMenu', () => { await user.click(await screen.findByRole('menuitem', { name: 'Rename' })); expect(onClick).toHaveBeenCalledOnce(); + await waitFor(() => expect(screen.queryByRole('menu')).not.toBeInTheDocument()); }); + it('closes a progress action that settles before pending renders', async () => { + const onClick = vi.fn(); + const user = userEvent.setup(); + renderMenu([{ label: 'Make default', progress: 'idle', onClick }]); + + await user.click(screen.getByRole('button', { name: 'Manage passkey' })); + await user.click(await screen.findByRole('menuitem', { name: 'Make default' })); + + expect(onClick).toHaveBeenCalledOnce(); + await waitFor(() => expect(screen.queryByRole('menu')).not.toBeInTheDocument()); + }); + it('keeps an eligible action visible but inert until its handler exists', async () => { const onClick = vi.fn(); const user = userEvent.setup(); diff --git a/packages/mosaic/src/components/action-menu/action-menu.tsx b/packages/mosaic/src/components/action-menu/action-menu.tsx index bc866589cda..567f8eebbfb 100644 --- a/packages/mosaic/src/components/action-menu/action-menu.tsx +++ b/packages/mosaic/src/components/action-menu/action-menu.tsx @@ -1,8 +1,10 @@ -import type { Ref } from 'react'; +import { type Ref, useState } from 'react'; +import { useSpinDelay } from '../../hooks/use-spin-delay'; import type { IconName } from '../../icons/registry'; import { Icon } from '../icon'; import { Menu } from '../menu'; +import { Spinner } from '../spinner'; type ActionMenuActionAppearance = { label: string; @@ -11,7 +13,10 @@ type ActionMenuActionAppearance = { }; export type ActionMenuAction = ActionMenuActionAppearance & - ({ onClick: () => void; disabled?: boolean } | { onClick?: never; disabled: true }); + ( + | { onClick: () => void; disabled?: boolean; progress?: 'idle' | 'pending' | 'handoff' } + | { onClick?: never; disabled: true; progress?: never } + ); export interface ActionMenuProps { label: string; @@ -21,12 +26,31 @@ export interface ActionMenuProps { } export function ActionMenu({ label, actions, triggerRef, disabled }: ActionMenuProps) { + const [open, setOpen] = useState(false); + const [activated, setActivated] = useState(false); + const pending = actions.some(action => action.progress === 'pending'); + const handoff = actions.some(action => action.progress === 'handoff'); + const showSpinner = useSpinDelay(pending ? true : null); + + if (activated && !pending) { + setActivated(false); + setOpen(false); + } + if (actions.length === 0) { return null; } return ( - + { + if (!pending && !handoff) { + setOpen(nextOpen); + } + }} + > { + if (action.progress !== undefined) { + setActivated(true); + } + action.onClick?.(); + }} + closeOnClick={action.progress === undefined} + aria-busy={action.progress === 'pending' || undefined} + disabled={disabled || action.disabled || pending || handoff} > {action.icon ? ( @@ -47,6 +78,14 @@ export function ActionMenu({ label, actions, triggerRef, disabled }: ActionMenuP ) : null} {action.label} + {action.progress === 'pending' && showSpinner ? ( + + ) : null} ))} From d999bb1f185a80ab95462e40d3b800a3f4fa4b00 Mon Sep 17 00:00:00 2001 From: austincalvelage Date: Fri, 23 Oct 2026 15:37:47 -0600 Subject: [PATCH 2/3] fix(mosaic): prepare two-step setup before opening its dialog --- .changeset/steady-two-step-preparation.md | 2 + ...-profile-mfa-interactions.feature.test.tsx | 44 ++++- ...er-profile-mfa-management.feature.test.tsx | 158 ++++++++++------- .../user-profile-mfa.feature.test.tsx | 31 ++++ .../user-profile-add-mfa.view.tsx | 17 +- .../user-profile-mfa-row.view.tsx | 50 ++++-- ...ser-profile-mfa-section-leaf.controller.ts | 3 +- .../user-profile-mfa-section.controller.ts | 159 ++++++++++-------- .../user-profile-mfa-section.tsx | 1 + .../user-profile-mfa-section.view.tsx | 19 ++- .../user-profile-mfa-setup.view.tsx | 2 + 11 files changed, 332 insertions(+), 154 deletions(-) create mode 100644 .changeset/steady-two-step-preparation.md diff --git a/.changeset/steady-two-step-preparation.md b/.changeset/steady-two-step-preparation.md new file mode 100644 index 00000000000..a845151cc84 --- /dev/null +++ b/.changeset/steady-two-step-preparation.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa-interactions.feature.test.tsx b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa-interactions.feature.test.tsx index 52460d42121..6084a908eff 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa-interactions.feature.test.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa-interactions.feature.test.tsx @@ -45,8 +45,8 @@ describe('User profile MFA interaction boundaries', () => { await user.click(screen.getByRole('button', { name: 'Add verification method' })); await user.click(screen.getByRole('button', { name: /Authenticator app/ })); await waitFor(() => expect(screen.getByRole('alert')).toHaveTextContent('Please try again.')); - const retry = screen.getByRole('button', { name: 'Try again' }); - await waitFor(() => expect(retry).toHaveFocus()); + const retry = screen.getByRole('button', { name: /Authenticator app/ }); + expect(retry).toBeEnabled(); await user.click(retry); expect(await screen.findByRole('img', { name: /QR code/i })).toBeVisible(); const code = screen.getByRole('textbox', { name: 'Verification code' }); @@ -74,7 +74,7 @@ describe('User profile MFA interaction boundaries', () => { expect(fapi.mfa.totpCreations).toBe(1); }); - it('disables Back while authenticator preparation is pending', async () => { + it('disables method choices and prevents duplicate authenticator preparation', async () => { await renderMfa(); const held = holdRequests('post', '/v1/me/totp'); const user = userEvent.setup(); @@ -83,8 +83,8 @@ describe('User profile MFA interaction boundaries', () => { await waitFor(() => expect(held.requests).toHaveLength(1)); try { await waitFor(() => { - expect(screen.getByRole('status', { name: 'Preparing authenticator…' })).toBeVisible(); - expect(screen.getByRole('button', { name: 'Back' })).toBeDisabled(); + expect(screen.getByRole('button', { name: /Authenticator app/ })).toBeDisabled(); + expect(screen.getByRole('button', { name: /SMS verification/ })).toBeDisabled(); }); } finally { held.release(); @@ -188,9 +188,39 @@ describe('User profile MFA interaction boundaries', () => { await waitFor(() => expect(screen.getByRole('alert')).toHaveTextContent('Please try again.')); expect(screen.queryByRole('button', { name: 'Copy and close' })).toBeNull(); expect(screen.queryByRole('button', { name: 'Download' })).toBeNull(); - const retry = screen.getByRole('button', { name: 'Try again' }); - await waitFor(() => expect(retry).toHaveFocus()); + const retry = screen.getByRole('button', { name: /Backup codes/ }); + expect(retry).toBeEnabled(); + const held = holdRequests('post', '/v1/me/backup_codes/'); await user.click(retry); + await waitFor(() => expect(held.requests).toHaveLength(1)); + try { + expect(retry).toBeDisabled(); + expect(retry).toHaveAttribute('aria-busy', 'true'); + expect(screen.queryByRole('list', { name: 'Backup codes' })).toBeNull(); + await user.click(retry); + expect(held.requests).toHaveLength(1); + } finally { + held.release(); + } + expect(await screen.findByText('CODE0100')).toBeVisible(); + }); + + it('keeps the selected backup choice while the account refresh completes', async () => { + await renderMfa(fapiUser({ id: 'user_1', totp_enabled: true, two_factor_enabled: true })); + const user = userEvent.setup(); + await user.click(screen.getByRole('button', { name: 'Add verification method' })); + const held = holdRequests('get', '/v1/me'); + await user.click(screen.getByRole('button', { name: /Backup codes/ })); + await waitFor(() => expect(held.requests).toHaveLength(1)); + try { + expect(within(screen.getByRole('dialog')).getByRole('button', { name: /Backup codes/ })).toHaveAttribute( + 'aria-busy', + 'true', + ); + expect(screen.queryByRole('list', { name: 'Backup codes' })).toBeNull(); + } finally { + held.release(); + } expect(await screen.findByText('CODE0100')).toBeVisible(); }); 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 c90e2e9d762..9d0ec31edbf 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 @@ -170,6 +170,10 @@ describe('User profile MFA management', () => { await user.click(screen.getByRole('menuitem', { name: 'Set as default' })); await waitFor(() => expect(start.requests).toHaveLength(1)); expect(screen.queryByRole('dialog')).toBeNull(); + expect(screen.getByRole('menuitem', { name: 'Set as default' })).toHaveAttribute('aria-busy', 'true'); + await waitFor(() => + expect(within(screen.getByRole('menuitem', { name: 'Set as default' })).getByRole('progressbar')).toBeVisible(), + ); start.release(); expect(await screen.findByText('Enter your current password to continue')).toBeVisible(); @@ -354,7 +358,7 @@ describe('User profile MFA management', () => { expect(clerk.user?.id).toBe('user_2'); }); - it('opens the backup-code surface while regeneration is pending', async () => { + it('keeps regeneration pending in its menu until backup codes are ready', async () => { await renderMfa( fapiUser({ id: 'user_1', totp_enabled: true, backup_code_enabled: true, two_factor_enabled: true }), ); @@ -363,21 +367,47 @@ describe('User profile MFA management', () => { await openMenu(user, screen.getByRole('button', { name: 'Manage Backup codes' })); await user.click(screen.getByRole('menuitem', { name: 'Regenerate' })); await waitFor(() => expect(held.requests).toHaveLength(1)); - await waitFor(() => expect(screen.getByRole('status', { name: 'Generating backup codes' })).toBeVisible()); - for (const name of ['Print', 'Download']) { - expect(screen.getByRole('button', { name })).toBeVisible(); - expect(screen.getByRole('button', { name })).toBeDisabled(); + try { + expect(screen.queryByRole('dialog')).toBeNull(); + expect(screen.getByRole('button', { name: 'Add verification method' })).toBeDisabled(); + expect(screen.getByRole('button', { name: 'Manage Authenticator app' })).toBeDisabled(); + expect(screen.getByRole('menuitem', { name: 'Regenerate' })).toHaveAttribute('aria-busy', 'true'); + expect(screen.queryByRole('status', { name: 'Generating backup codes' })).toBeNull(); + } finally { + held.release(); } - expect(screen.getByRole('button', { name: 'Copy and close' })).toBeVisible(); - expect(screen.getByRole('button', { name: 'Copy and close' })).toHaveAttribute('aria-disabled', 'true'); - await waitFor(() => expect(screen.getByRole('button', { name: 'Close' })).toHaveFocus()); - held.release(); await waitFor(() => expect(screen.getByText('CODE0100')).toBeVisible()); for (const name of ['Print', 'Download', 'Copy and close']) { expect(screen.getByRole('button', { name })).toBeEnabled(); } }); + 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 }), + ); + const handlers = worker.listHandlers(); + const held = holdRequests('post', '/v1/me/backup_codes/'); + const user = userEvent.setup(); + const trigger = screen.getByRole('button', { name: 'Manage Backup codes' }); + await openMenu(user, trigger); + await user.click(screen.getByRole('menuitem', { name: 'Regenerate' })); + await waitFor(() => expect(held.requests).toHaveLength(1)); + held.fail('action_blocked', 'Unable to create backup codes.'); + await waitFor(() => + expect(screen.getByRole('alert')).toHaveTextContent( + "This action couldn't be completed. Please try again later or contact support if this persists.", + ), + ); + expect(screen.queryByRole('dialog')).toBeNull(); + await waitFor(() => expect(screen.queryByRole('menu')).toBeNull()); + worker.resetHandlers(...handlers); + await openMenu(user, trigger); + await user.click(screen.getByRole('menuitem', { name: 'Regenerate' })); + expect(await screen.findByText('CODE0100')).toBeVisible(); + expect(fapi.mfa.backupCodeCreations).toBe(1); + }); + it('uses fresh server codes for explicit regeneration', async () => { const fapi = await renderMfa( fapiUser({ id: 'user_1', totp_enabled: true, backup_code_enabled: true, two_factor_enabled: true }), @@ -443,6 +473,7 @@ describe('User profile MFA management', () => { await regenerate(user); await screen.findByText('Enter your current password to continue'); const dialog = screen.getByRole('dialog'); + const handlers = worker.listHandlers(); const replay = holdRequests('post', '/v1/me/backup_codes/'); await user.type(screen.getByLabelText('Password'), 'hunter2{Enter}'); await waitFor(() => expect(replay.requests).toHaveLength(1)); @@ -452,40 +483,56 @@ describe('User profile MFA management', () => { expect(dialog).toBeVisible(); replay.fail('action_blocked', 'Unable to create backup codes.'); - await waitFor(() => expect(within(screen.getByRole('dialog')).getByRole('alert')).toBeVisible()); + await waitFor(() => expect(screen.queryByRole('dialog')).toBeNull()); + expect(screen.getByRole('alert')).toHaveTextContent( + "This action couldn't be completed. Please try again later or contact support if this persists.", + ); expect(screen.queryByRole('list', { name: 'Backup codes' })).toBeNull(); expect(fapi.mfa.backupCodeCreations).toBe(0); + worker.resetHandlers(...handlers); + await regenerate(user); + await screen.findByRole('list', { name: 'Backup codes' }); + expect(fapi.mfa.backupCodeCreations).toBe(1); }); - it('cancels a loading verification when the backup-code dialog closes so regeneration can start again', async () => { + it('keeps the menu pending until verification preparation is ready', async () => { const fapi = await renderBackupCodesNeedingReverification(); const start = holdRequests('post', '/v1/client/sessions/:id/verify'); const user = userEvent.setup(); await regenerate(user); await waitFor(() => expect(start.requests).toHaveLength(1)); - await user.click(within(screen.getByRole('dialog')).getByRole('button', { name: 'Close' })); - await waitFor(() => expect(screen.queryByRole('dialog')).toBeNull()); - start.release(); - - await regenerate(user); + try { + expect(screen.queryByRole('dialog')).toBeNull(); + expect(screen.getByRole('menuitem', { name: 'Regenerate' })).toHaveAttribute('aria-busy', 'true'); + await user.keyboard('{Escape}'); + expect(screen.getByRole('menuitem', { name: 'Regenerate' })).toBeVisible(); + } finally { + start.release(); + } + await screen.findByText('Enter your current password to continue'); + await waitFor(() => expect(screen.getByRole('dialog')).toContainElement(document.activeElement)); + await user.type(screen.getByLabelText('Password'), 'hunter2{Enter}'); await screen.findByRole('list', { name: 'Backup codes' }); expect(fapi.mfa.backupCodeCreations).toBe(1); }); - it('keeps the backup-code dialog open until the first response so a verification can appear', async () => { + it('keeps the menu pending until the first response can request verification', async () => { const fapi = await renderBackupCodesNeedingReverification(); const first = holdRequests('post', '/v1/me/backup_codes/'); const user = userEvent.setup(); await regenerate(user); await waitFor(() => expect(first.requests).toHaveLength(1)); - const dialog = screen.getByRole('dialog'); - await user.keyboard('{Escape}'); - expect(dialog).toBeVisible(); - first.release(); + try { + expect(screen.queryByRole('dialog')).toBeNull(); + await user.keyboard('{Escape}'); + expect(screen.getByRole('menuitem', { name: 'Regenerate' })).toHaveAttribute('aria-busy', 'true'); + } finally { + first.release(); + } - expect(await within(dialog).findByText('Enter your current password to continue')).toBeVisible(); + expect(await screen.findByText('Enter your current password to continue')).toBeVisible(); await user.type(screen.getByLabelText('Password'), 'hunter2{Enter}'); await screen.findByRole('list', { name: 'Backup codes' }); expect(fapi.mfa.backupCodeCreations).toBe(1); @@ -658,41 +705,38 @@ describe('User profile MFA management', () => { }); it.each([ - { 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', - async ({ method, path, pendingPhone }) => { - 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, - }); - await renderMfa(fapiUser({ id: 'user_1', phone_numbers: [first, second], two_factor_enabled: true })); - const held = holdRequests(method, path); - const user = userEvent.setup(); - const selected = screen.getByRole('button', { name: 'Manage SMS verification +1 (555) 555-0202' }); - await openMenu(user, selected); - await user.click(screen.getByRole('menuitem', { name: 'Set as default' })); - 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(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 user.click(screen.getByRole('button', { name: 'Manage SMS verification +1 (555) 555-0101' })); - await waitFor(() => expect(screen.getByRole('menuitem', { name: 'Set as default' })).toBeVisible()); - }, - ); + { phase: 'mutation', method: 'post', path: '/v1/me/phone_numbers/phone_2' }, + { phase: 'refresh', method: 'get', path: '/v1/me' }, + ] as const)('keeps the selected default action pending during $phase', async ({ method, path }) => { + 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, + }); + await renderMfa(fapiUser({ id: 'user_1', phone_numbers: [first, second], two_factor_enabled: true })); + const held = holdRequests(method, path); + const user = userEvent.setup(); + const selected = screen.getByRole('button', { name: 'Manage SMS verification +1 (555) 555-0202' }); + await openMenu(user, selected); + await user.click(screen.getByRole('menuitem', { name: 'Set as default' })); + await waitFor(() => expect(held.requests).toHaveLength(1)); + try { + const action = screen.getByRole('menuitem', { name: 'Set as default' }); + expect(action).toHaveAttribute('aria-busy', 'true'); + expect(screen.getByRole('menuitem', { name: 'Remove method' })).toHaveAttribute('aria-disabled', 'true'); + expect(screen.getByRole('button', { name: 'Manage SMS verification +1 (555) 555-0101' })).toBeDisabled(); + await user.click(await screen.findByRole('menuitem', { name: 'Remove method' })); + 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 user.click(screen.getByRole('button', { name: 'Manage SMS verification +1 (555) 555-0101' })); + await waitFor(() => expect(screen.getByRole('menuitem', { name: 'Set as default' })).toBeVisible()); + }); it('clears backup-code eligibility when the final optional factor is removed', async () => { const fapi = await renderMfa( diff --git a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa.feature.test.tsx b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa.feature.test.tsx index b1d31c0c143..b190645e2ef 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa.feature.test.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/__tests__/user-profile-mfa.feature.test.tsx @@ -32,6 +32,28 @@ describe('User profile MFA', () => { expect(await screen.findByRole('img', { name: /QR code/i })).toBeVisible(); }); + it('keeps the authenticator choice pending until its setup is ready', async () => { + await renderMfa(); + const held = holdRequests('post', '/v1/me/totp'); + const user = userEvent.setup(); + await user.click(screen.getByRole('button', { name: 'Add verification method' })); + const authenticator = await screen.findByRole('button', { name: /Authenticator app/ }); + await user.click(authenticator); + await waitFor(() => expect(held.requests).toHaveLength(1)); + try { + expect(screen.getByRole('button', { name: /Authenticator app/ })).toBeVisible(); + expect(authenticator).toHaveAttribute('aria-busy', 'true'); + await user.click(authenticator); + await user.click(screen.getByRole('button', { name: /SMS verification/ })); + expect(held.requests).toHaveLength(1); + expect(screen.queryByText('Preparing authenticator…')).toBeNull(); + expect(screen.queryByRole('img', { name: /QR code/i })).toBeNull(); + } finally { + held.release(); + } + expect(await screen.findByRole('img', { name: /QR code/i })).toBeVisible(); + }); + it('shows enrolled methods with the default SMS phone first', async () => { const second = fapiPhoneNumber({ ...phone, @@ -221,7 +243,16 @@ describe('User profile MFA', () => { const user = userEvent.setup(); await user.click(screen.getByRole('button', { name: 'Add verification method' })); await user.click(screen.getByRole('button', { name: /SMS verification/ })); + const held = holdRequests('post', '/v1/me/phone_numbers/phone_1/prepare_verification'); await user.click(screen.getByRole('button', { name: 'Continue' })); + await waitFor(() => expect(held.requests).toHaveLength(1)); + try { + expect(screen.getByRole('combobox', { name: /Phone number/ })).toBeVisible(); + expect(screen.getByRole('button', { name: 'Continue' })).toHaveAttribute('aria-busy', 'true'); + expect(screen.queryByRole('textbox', { name: 'Verification code' })).toBeNull(); + } finally { + held.release(); + } expect(await screen.findByRole('textbox', { name: 'Verification code' })).toBeVisible(); expect(fapi.mfa.phonePreparations).toEqual(['phone_1']); expect(fapi.mfa.phoneUpdates).toEqual([]); diff --git a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-add-mfa.view.tsx b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-add-mfa.view.tsx index 96ea7a19752..8dc1ff5a02d 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-add-mfa.view.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-add-mfa.view.tsx @@ -2,10 +2,13 @@ import { Card } from '../../../components/card'; import { useFlowAutoFocus } from '../../../components/flow'; import { Icon, IconFrame } from '../../../components/icon'; import { Item } from '../../../components/item'; +import { Spinner } from '../../../components/spinner'; import { useMessages } from '../../../localization'; import type { UserProfileMfaAddableMethod } from './user-profile-mfa-section.view'; export interface UserProfileAddMfaViewProps { + pendingMethod?: 'authenticator' | 'backup-codes'; + errorMessage?: string; methods: readonly UserProfileMfaAddableMethod[]; onSelect: (type: UserProfileMfaAddableMethod) => void; } @@ -16,7 +19,7 @@ const icons = { 'backup-codes': 'numbers', } as const; -export function UserProfileAddMfaView({ methods, onSelect }: UserProfileAddMfaViewProps) { +export function UserProfileAddMfaView({ methods, onSelect, pendingMethod, errorMessage }: UserProfileAddMfaViewProps) { const m = useMessages('userProfileMfa'); const firstMethodRef = useFlowAutoFocus(); return ( @@ -26,6 +29,12 @@ export function UserProfileAddMfaView({ methods, onSelect }: UserProfileAddMfaVi {m.addDialog.description} + + {errorMessage} + {methods.map((type, index) => ( onSelect(type)} /> } @@ -48,9 +59,7 @@ export function UserProfileAddMfaView({ methods, onSelect }: UserProfileAddMfaVi {m.methods[type]} {m.addDialog.methods[type]} - - - + {pendingMethod === type ? : } ))} 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 290a2a398a1..05bf8bcba7b 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 @@ -13,37 +13,44 @@ function menuActions({ method, onRemove, onSetDefault, + defaultProgress, onRegenerateBackupCodes, + regenerateProgress, showDisabledMenu, m, }: { method: UserProfileMfaMethod; onRemove?: () => void; onSetDefault?: (id: string) => void; + defaultProgress?: ActionMenuAction['progress']; onRegenerateBackupCodes?: () => void; + regenerateProgress?: ActionMenuAction['progress']; showDisabledMenu?: boolean; m: MosaicMessages['userProfileMfa']; }): ActionMenuAction[] { const actions: ActionMenuAction[] = []; - if (method.type === 'sms' && method.canSetDefault && !method.isDefault && onSetDefault) { - actions.push({ label: m.setDefault, onClick: () => onSetDefault(method.id) }); + const hasDefaultAction = + (method.canSetDefault && !method.isDefault) || defaultProgress === 'pending' || defaultProgress === 'handoff'; + if (method.type === 'sms' && hasDefaultAction && onSetDefault) { + actions.push({ label: m.setDefault, onClick: () => onSetDefault(method.id), progress: defaultProgress ?? 'idle' }); } if (method.type === 'backup-codes') { if (onRegenerateBackupCodes) { - actions.push({ label: m.regenerate, onClick: onRegenerateBackupCodes }); + actions.push({ label: m.regenerate, onClick: onRegenerateBackupCodes, progress: regenerateProgress }); } } else if (onRemove && method.canRemove !== false) { actions.push({ label: m.remove, color: 'negative', onClick: onRemove }); } - if (showDisabledMenu && actions.length === 0) { - if (method.type === 'sms' && method.canSetDefault && !method.isDefault) { - actions.push({ label: m.setDefault, disabled: true }); - } - if (method.type === 'backup-codes') { - actions.push({ label: m.regenerate, disabled: true }); - } else if (method.canRemove !== false) { - actions.push({ label: m.remove, color: 'negative', disabled: true }); - } + if (!showDisabledMenu || actions.length > 0) { + return actions; + } + if (method.type === 'sms' && hasDefaultAction) { + actions.push({ label: m.setDefault, disabled: true }); + } + if (method.type === 'backup-codes') { + actions.push({ label: m.regenerate, disabled: true }); + } else if (method.canRemove !== false) { + actions.push({ label: m.remove, color: 'negative', disabled: true }); } return actions; } @@ -53,13 +60,18 @@ export function UserProfileMfaRowView({ triggerRef, onRemove, onSetDefault, + defaultProgress, onRegenerateBackupCodes, + regenerateProgress, showDisabledMenu, -}: Pick & { + disabled, +}: Pick & { method: UserProfileMfaMethod; triggerRef?: Ref; + disabled?: boolean; onRemove?: () => void; onSetDefault?: (id: string) => void; + defaultProgress?: ActionMenuAction['progress']; showDisabledMenu?: boolean; }) { const m = useMessages('userProfileMfa'); @@ -70,7 +82,16 @@ export function UserProfileMfaRowView({ method.type === 'sms' && description ? fill(m.manageSms, { label, phoneNumber: description }) : fill(m.manage, { label }); - const actions = menuActions({ method, onRemove, onSetDefault, onRegenerateBackupCodes, showDisabledMenu, m }); + const actions = menuActions({ + method, + onRemove, + onSetDefault, + defaultProgress, + onRegenerateBackupCodes, + regenerateProgress, + showDisabledMenu, + m, + }); return ( @@ -87,6 +108,7 @@ export function UserProfileMfaRowView({ diff --git a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section-leaf.controller.ts b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section-leaf.controller.ts index 73419056fa2..5917008a851 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section-leaf.controller.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section-leaf.controller.ts @@ -59,6 +59,7 @@ export function useUserProfileMfaSectionLeafController({ finalRemovalFocus: removalFocus.finalFocus, setDefaultFinalFocus: () => (defaultAction.lastKey ? removalFocus.trigger(defaultAction.lastKey) : null), defaultError: defaultAction.error, - setDefault: onSetDefault && !defaultAction.isPending ? setDefault : undefined, + pendingDefaultId: defaultAction.pendingKey, + 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 0c0f90bbc16..6fd122d643b 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 @@ -14,14 +14,13 @@ type ReadyModel = Extract; type AuthenticatorSetup = { secret: string; uri: string }; type MfaAction = 'prepareAuthenticator' | 'verifyAuthenticator' | 'sms' | 'resend' | 'generate' | 'copy'; type MfaFlow = - | { kind: 'closed' } - | { kind: 'select' } + | { kind: 'closed'; error?: ErrorDescription } + | { kind: 'select'; methods: readonly UserProfileMfaAddableMethod[]; error?: ErrorDescription } | { kind: 'authenticator'; - setup?: AuthenticatorSetup; + setup: AuthenticatorSetup; code: string; error?: ErrorDescription; - setupError?: ErrorDescription; } | { kind: 'sms'; @@ -42,22 +41,25 @@ interface MfaContext { dismissed: boolean; resendSeconds: number; run: () => Promise; - resolve: () => void; - reject: (error: unknown) => void; } type MfaEvent = - | { type: 'OPEN' } + | { type: 'OPEN'; methods: readonly UserProfileMfaAddableMethod[] } | { type: 'CLOSE' } - | { type: 'SELECT'; method: UserProfileMfaAddableMethod; phoneId: string; hasPhones: boolean } - | { type: 'BACK'; hasPhones: boolean } + | { + type: 'SELECT'; + method: Exclude; + phoneId: string; + hasPhones: boolean; + } + | { type: 'BACK'; hasPhones: boolean; methods: readonly UserProfileMfaAddableMethod[] } | { type: 'EDIT_PHONE_ID'; value: string } | { type: 'EDIT_PHONE_NUMBER'; value: string } | { type: 'EDIT_CODE'; value: string } | { type: 'PHONE_RESOLVED'; id: string; phoneNumber: string } | { type: 'PRINT_FAILED' } | { type: 'TICK' } - | { type: 'RUN'; key: MfaAction; run: () => Promise; resolve: () => void; reject: (error: unknown) => void }; + | { type: 'RUN'; key: MfaAction; run: () => Promise }; const { createMachine, assign, fromPromise } = setup(); const RESEND_COOLDOWN_SECONDS = 30; @@ -71,13 +73,11 @@ const mfaMachine = createMachine({ dismissed: false, resendSeconds: 0, run: () => Promise.resolve({ kind: 'closed' }), - resolve: () => undefined, - reject: () => undefined, }, states: { idle: { on: { - OPEN: { actions: assign(() => ({ flow: { kind: 'select' } })) }, + OPEN: { actions: assign((_, event) => ({ flow: { kind: 'select', methods: event.methods } })) }, CLOSE: { actions: assign(() => ({ flow: { kind: 'closed' }, savedSetup: undefined, resendSeconds: 0 })), }, @@ -94,9 +94,9 @@ const mfaMachine = createMachine({ phoneNumber: '', code: '', } - : event.method === 'backup-codes' - ? { kind: 'backup', codes: [] } - : { kind: 'authenticator', setup: context.savedSetup, code: '' }, + : context.savedSetup + ? { kind: 'authenticator', setup: context.savedSetup, code: '' } + : context.flow, })), }, BACK: { @@ -111,7 +111,7 @@ const mfaMachine = createMachine({ return { flow: { ...flow, step: 'select', error: undefined } }; } return { - flow: { kind: 'select' }, + flow: { kind: 'select', methods: event.methods }, savedSetup: flow.kind === 'authenticator' ? flow.setup : context.savedSetup, }; }), @@ -154,19 +154,20 @@ const mfaMachine = createMachine({ }, RUN: { target: 'busy', - actions: assign((_, event) => ({ + actions: assign((context, event) => ({ + flow: { ...context.flow, error: undefined }, pending: event.key, dismissed: false, run: event.run, - resolve: event.resolve, - reject: event.reject, })), }, }, }, busy: { on: { - OPEN: { actions: assign(() => ({ flow: { kind: 'select' }, dismissed: true })) }, + OPEN: { + actions: assign((_, event) => ({ flow: { kind: 'select', methods: event.methods }, dismissed: true })), + }, CLOSE: { actions: assign(() => ({ flow: { kind: 'closed' }, @@ -189,7 +190,6 @@ const mfaMachine = createMachine({ onDone: { target: 'idle', actions: [ - context => context.resolve(), assign>((context, event) => { if (context.dismissed && event.output.kind !== 'backup') { return { pending: undefined, dismissed: false }; @@ -216,7 +216,6 @@ const mfaMachine = createMachine({ onError: { target: 'idle', actions: [ - (context, event) => context.reject(event.error), assign((context, event) => { const flow = context.flow; if (context.dismissed) { @@ -225,18 +224,9 @@ const mfaMachine = createMachine({ if (isReverificationCancelledError(event.error)) { return { flow: { kind: 'closed' }, pending: undefined }; } - if (flow.kind !== 'authenticator' && flow.kind !== 'sms' && flow.kind !== 'backup') { - return { pending: undefined }; - } const error = toLocalizableError(event.error); return { - flow: - flow.kind === 'authenticator' - ? { - ...flow, - ...(context.pending === 'prepareAuthenticator' ? { setupError: error } : { error }), - } - : { ...flow, error }, + flow: { ...flow, error }, pending: undefined, }; }), @@ -248,7 +238,7 @@ const mfaMachine = createMachine({ }); type MfaSend = (event: MfaEvent) => void; -type RunMfaAction = (key: MfaAction, action: () => Promise) => Promise; +type RunMfaAction = (key: MfaAction, action: () => Promise) => void; function createAuthenticatorActions(model: ReadyModel, flow: MfaFlow, run: RunMfaAction) { const prepareAuthenticator = () => { @@ -256,21 +246,21 @@ function createAuthenticatorActions(model: ReadyModel, flow: MfaFlow, run: RunMf if (!createAuthenticator) { return; } - void run('prepareAuthenticator', async () => { + run('prepareAuthenticator', async () => { const setup = await createAuthenticator(); return { kind: 'authenticator', setup, code: '' }; - }).catch(() => undefined); + }); }; const verifyAuthenticator = (code: string) => { const verify = model.verifyAuthenticator; - if (!verify || flow.kind !== 'authenticator' || !flow.setup) { + if (!verify || flow.kind !== 'authenticator') { return; } - void run('verifyAuthenticator', async () => { + run('verifyAuthenticator', async () => { const result = await verify(code); return result.backupCodes.length ? { kind: 'backup', codes: result.backupCodes } : { kind: 'closed' }; - }).catch(() => undefined); + }); }; return { prepareAuthenticator, verifyAuthenticator }; @@ -284,7 +274,7 @@ function createSmsActions(model: ReadyModel, flow: MfaFlow, resendSeconds: numbe return; } const current = flow; - void run('sms', async () => { + run('sms', async () => { let phoneId = current.phoneId ?? current.selectedPhoneId; if (current.step === 'phone' && !phoneId) { if (!findOrCreatePhone) { @@ -301,7 +291,7 @@ function createSmsActions(model: ReadyModel, flow: MfaFlow, resendSeconds: numbe return result.status === 'complete' && result.backupCodes.length ? { kind: 'backup', codes: result.backupCodes } : { kind: 'closed' }; - }).catch(() => undefined); + }); }; const resendSms = () => { @@ -310,10 +300,10 @@ function createSmsActions(model: ReadyModel, flow: MfaFlow, resendSeconds: numbe return; } const current = flow; - void run('resend', async () => { + run('resend', async () => { await resend(current.phoneId ?? ''); return { ...current, error: undefined }; - }).catch(() => undefined); + }); }; return { submitSms, resendSms }; @@ -332,11 +322,10 @@ function createBackupCodeActions( if (!generate) { return; } - send({ type: 'SELECT', method: 'backup-codes', phoneId: '', hasPhones: false }); - void run('generate', async () => { + run('generate', async () => { const codes = await generate(); return { kind: 'backup', codes }; - }).catch(() => undefined); + }); }; const copy = () => { @@ -344,10 +333,10 @@ function createBackupCodeActions( return; } const codes = flow.codes; - void run('copy', async () => { + run('copy', async () => { await navigator.clipboard.writeText(codes.join('\n')); return { kind: 'closed' }; - }).catch(() => undefined); + }); }; const download = () => { @@ -418,17 +407,44 @@ function createSetupProps( const sms = flow.kind === 'sms' ? flow : undefined; const authenticator = flow.kind === 'authenticator' ? flow : undefined; const backup = flow.kind === 'backup' ? flow : undefined; + let step: UserProfileMfaSetupViewProps['step']; + switch (flow.kind) { + case 'closed': + step = 'select'; + break; + case 'backup': + step = 'backup-codes'; + break; + case 'select': + case 'authenticator': + case 'sms': + step = flow.kind; + } + if (context.pending === 'generate' && model.backupCodesReverification.visible) { + step = 'verify'; + } + let pendingMethod: UserProfileMfaSetupViewProps['pendingMethod']; + switch (context.pending) { + case 'prepareAuthenticator': + pendingMethod = 'authenticator'; + break; + case 'generate': + pendingMethod = 'backup-codes'; + break; + case undefined: + case 'sms': + case 'verifyAuthenticator': + case 'resend': + case 'copy': + pendingMethod = undefined; + break; + } return { - step: - flow.kind === 'closed' - ? 'select' - : flow.kind === 'backup' - ? context.pending === 'generate' && model.backupCodesReverification.visible - ? 'verify' - : 'backup-codes' - : flow.kind, + step, + pendingMethod, + errorMessage: flow.kind === 'select' && flow.error ? errorText(flow.error) : undefined, reverification: model.backupCodesReverification, - methods: model.addableMethods, + methods: flow.kind === 'select' ? flow.methods : model.addableMethods, onSelect: actions.select, onBack: actions.back, onCancel: actions.close, @@ -452,7 +468,6 @@ function createSetupProps( }, authenticator: { setup: authenticator?.setup, - setupErrorMessage: authenticator?.setupError ? errorText(authenticator.setupError) : undefined, onRetry: actions.prepareAuthenticator, code: authenticator?.code ?? '', onCodeChange: value => send({ type: 'EDIT_CODE', value }), @@ -478,16 +493,12 @@ export function useUserProfileMfaController(model: ReadyModel) { const [{ context, value }, send] = useMachine(mfaMachine); const isBusy = value === 'busy'; const run: RunMfaAction = (key, action) => { - if (isBusy) { - return Promise.resolve(); - } - return new Promise((resolve, reject) => { - send({ type: 'RUN', key, run: action, resolve, reject }); - }); + send({ type: 'RUN', key, run: action }); }; const { resendSeconds } = context; const flow = context.flow; - const dialogOpen = flow.kind !== 'closed'; + const dialogOpen = + flow.kind !== 'closed' || (context.pending === 'generate' && model.backupCodesReverification.visible); useEffect(() => { if (!dialogOpen || resendSeconds === 0) { return; @@ -506,18 +517,18 @@ export function useUserProfileMfaController(model: ReadyModel) { ); const close = () => send({ type: 'CLOSE' }); - const back = () => send({ type: 'BACK', hasPhones: model.phones.length > 0 }); + const back = () => send({ type: 'BACK', hasPhones: model.phones.length > 0, methods: model.addableMethods }); const select = (method: UserProfileMfaAddableMethod) => { if (isBusy) { return; } - send({ type: 'SELECT', method, phoneId: model.phones[0]?.id ?? '', hasPhones: model.phones.length > 0 }); if (method === 'authenticator' && !context.savedSetup) { prepareAuthenticator(); - } - if (method === 'backup-codes') { + } else if (method === 'backup-codes') { generateBackupCodes(); + } else { + send({ type: 'SELECT', method, phoneId: model.phones[0]?.id ?? '', hasPhones: model.phones.length > 0 }); } }; @@ -545,6 +556,13 @@ export function useUserProfileMfaController(model: ReadyModel) { methods: [...model.methods], addableMethods: model.addableMethods, onRegenerateBackupCodes: model.generateBackupCodes ? generateBackupCodes : undefined, + regenerateProgress: + flow.kind === 'backup' || (context.pending === 'generate' && model.backupCodesReverification.visible) + ? 'handoff' + : context.pending === 'generate' + ? 'pending' + : 'idle', + regenerateError: flow.kind === 'closed' && flow.error ? errorText(flow.error) : undefined, onRemove: model.remove, onSetDefault: model.setDefault, removeReverification: model.removeReverification, @@ -558,6 +576,7 @@ export function useUserProfileMfaController(model: ReadyModel) { return { dialogOpen, showAddTrigger, + addDisabled: isBusy, showAddControl: showAddTrigger || dialogOpen, reverifying, onDialogOpenChange: (open: boolean) => { @@ -567,7 +586,7 @@ export function useUserProfileMfaController(model: ReadyModel) { } reverification.onCancel(); } - send({ type: open ? 'OPEN' : 'CLOSE' }); + send(open ? { type: 'OPEN', methods: model.addableMethods } : { type: 'CLOSE' }); }, showCloseButton: !reverifying || Boolean(reverification.onCancel), sectionProps, diff --git a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.tsx b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.tsx index ac404b503a2..63a99e49e5d 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-mfa-section/user-profile-mfa-section.tsx @@ -41,6 +41,7 @@ function MfaEditor({ model }: { model: Extract void; + regenerateProgress?: ActionMenuAction['progress']; + regenerateError?: string; onRegenerateBackupCodes?: () => void; onRemove?: (id: string) => void | Promise; onSetDefault?: (id: string) => void | Promise; @@ -37,6 +40,8 @@ export function UserProfileMfaSectionView({ actionsEligible, onAdd, onRegenerateBackupCodes, + regenerateProgress, + regenerateError, onRemove, onSetDefault, removeReverification, @@ -83,11 +88,23 @@ export function UserProfileMfaSectionView({ triggerRef={controller.registerTrigger(method.id)} onRemove={controller.remove ? () => controller.openRemoval(method) : undefined} onSetDefault={controller.setDefault} + defaultProgress={ + controller.pendingDefaultId === method.id + ? setDefaultReverification?.visible + ? 'handoff' + : 'pending' + : 'idle' + } + disabled={ + (controller.pendingDefaultId !== undefined && controller.pendingDefaultId !== method.id) || + (regenerateProgress === 'pending' && method.type !== 'backup-codes') + } onRegenerateBackupCodes={onRegenerateBackupCodes} + regenerateProgress={regenerateProgress} /> ))} - {controller.defaultError} + {regenerateError ?? controller.defaultError} {controller.remove ? ( From fd2dc3f45dcc92a1a37c9a9b434b4d2f2d95381f Mon Sep 17 00:00:00 2001 From: austincalvelage Date: Fri, 23 Oct 2026 15:37:56 -0600 Subject: [PATCH 3/3] fix(mosaic): keep contact verification and passkey actions pending --- .changeset/email-link-prepared-signal.md | 6 + .changeset/ready-contact-dialogs.md | 2 + .../src/core/resources/EmailAddress.ts | 27 +- .../resources/__tests__/EmailAddress.test.ts | 64 +++++ .../user-profile-contact-list-row.view.tsx | 80 ++++-- .../user-profile-contact.types.ts | 5 +- .../user-profile-add-email.controller.test.ts | 144 +++++++++-- .../user-profile-add-email.dialog.test.tsx | 11 + .../user-profile-email-actions.test.tsx | 3 +- ...ser-profile-email-section.feature.test.tsx | 240 ++++++++++++++++-- .../user-profile-add-email.controller.ts | 133 +++++++--- .../user-profile-add-email.dialog.tsx | 3 + .../user-profile-add-email.messages.ts | 1 + .../user-profile-email-section.controller.ts | 7 +- .../user-profile-email-section.model.ts | 42 ++- .../user-profile-email-section.types.ts | 2 + .../user-profile-email-section.view.tsx | 7 + ...-profile-passkeys-section.feature.test.tsx | 33 ++- .../user-profile-passkeys-section.messages.ts | 1 + .../user-profile-passkeys-section.view.tsx | 12 +- .../user-profile-add-phone.controller.test.ts | 4 +- .../user-profile-add-phone.dialog.test.tsx | 3 +- ...ser-profile-phone-section.feature.test.tsx | 67 +++++ .../user-profile-add-phone.controller.ts | 55 +++- .../user-profile-add-phone.dialog.tsx | 4 +- .../user-profile-phone-section.controller.ts | 7 +- .../user-profile-phone-section.model.ts | 16 +- .../user-profile-phone-section.types.ts | 2 + .../user-profile-phone-section.view.tsx | 7 + .../user-profile/user-profile-phone.steps.tsx | 4 +- packages/shared/src/types/emailAddress.ts | 6 +- 31 files changed, 825 insertions(+), 173 deletions(-) create mode 100644 .changeset/email-link-prepared-signal.md create mode 100644 .changeset/ready-contact-dialogs.md diff --git a/.changeset/email-link-prepared-signal.md b/.changeset/email-link-prepared-signal.md new file mode 100644 index 00000000000..ad36ae4db6c --- /dev/null +++ b/.changeset/email-link-prepared-signal.md @@ -0,0 +1,6 @@ +--- +"@clerk/clerk-js": patch +"@clerk/shared": patch +--- + +Email address link flows now accept an optional `onPrepared` callback that runs after the verification email is prepared, while the returned promise still waits for verification. Cancelling during preparation prevents subsequent polling. diff --git a/.changeset/ready-contact-dialogs.md b/.changeset/ready-contact-dialogs.md new file mode 100644 index 00000000000..a845151cc84 --- /dev/null +++ b/.changeset/ready-contact-dialogs.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/packages/clerk-js/src/core/resources/EmailAddress.ts b/packages/clerk-js/src/core/resources/EmailAddress.ts index 1d4df3c6b73..1fe9067a27f 100644 --- a/packages/clerk-js/src/core/resources/EmailAddress.ts +++ b/packages/clerk-js/src/core/resources/EmailAddress.ts @@ -8,7 +8,7 @@ import type { EmailAddressResource, IdentificationLinkResource, PrepareEmailAddressVerificationParams, - StartEmailLinkFlowParams, + StartEmailAddressLinkFlowParams, StartEnterpriseSSOLinkFlowParams, VerificationResource, } from '@clerk/shared/types'; @@ -51,15 +51,28 @@ export class EmailAddress extends BaseResource implements EmailAddressResource { }); }; - createEmailLinkFlow = (): CreateEmailLinkFlowReturn => { + createEmailLinkFlow = (): CreateEmailLinkFlowReturn => { const { run, stop } = Poller(); - const startEmailLinkFlow = async ({ redirectUrl }: StartEmailLinkFlowParams): Promise => { + let operation = 0; + + const startEmailLinkFlow = async ({ + redirectUrl, + onPrepared, + }: StartEmailAddressLinkFlowParams): Promise => { + const currentOperation = ++operation; await this.prepareVerification({ strategy: 'email_link', redirectUrl: redirectUrl, }); return new Promise((resolve, reject) => { + if (currentOperation !== operation) { + return; + } + onPrepared?.(); + if (currentOperation !== operation) { + return; + } void run(() => { return this.reload() .then(res => { @@ -75,7 +88,13 @@ export class EmailAddress extends BaseResource implements EmailAddressResource { }); }); }; - return { startEmailLinkFlow, cancelEmailLinkFlow: stop }; + return { + startEmailLinkFlow, + cancelEmailLinkFlow: () => { + operation++; + stop(); + }, + }; }; createEnterpriseSSOLinkFlow = (): CreateEnterpriseSSOLinkFlowReturn< diff --git a/packages/clerk-js/src/core/resources/__tests__/EmailAddress.test.ts b/packages/clerk-js/src/core/resources/__tests__/EmailAddress.test.ts index 8043aba70d8..2111d14274c 100644 --- a/packages/clerk-js/src/core/resources/__tests__/EmailAddress.test.ts +++ b/packages/clerk-js/src/core/resources/__tests__/EmailAddress.test.ts @@ -121,3 +121,67 @@ describe('EmailAddress', () => { }); }); }); + +describe('email link readiness', () => { + afterEach(() => vi.restoreAllMocks()); + + it('signals successful preparation without completing the verification promise', async () => { + const email = new EmailAddress({ id: 'email_123' }, '/me/email_addresses'); + const held = createDeferredPromise(); + vi.spyOn(email, 'prepareVerification').mockImplementation(async () => { + await held.promise; + return email; + }); + const reload = vi.spyOn(email, 'reload').mockImplementation(() => new Promise(() => {})); + const prepared = vi.fn(); + const complete = vi.fn(); + const flow = email.createEmailLinkFlow(); + void flow.startEmailLinkFlow({ redirectUrl: '/verify', onPrepared: prepared }).then(complete); + expect(prepared).not.toHaveBeenCalled(); + held.resolve(); + await vi.waitFor(() => expect(prepared).toHaveBeenCalledOnce()); + expect(reload).toHaveBeenCalledOnce(); + expect(complete).not.toHaveBeenCalled(); + flow.cancelEmailLinkFlow(); + }); + + it.each(['during preparation', 'from readiness'] as const)( + 'does not start polling when cancelled %s', + async phase => { + const email = new EmailAddress({ id: 'email_123' }, '/me/email_addresses'); + const held = createDeferredPromise(); + vi.spyOn(email, 'prepareVerification').mockImplementation(async () => { + await held.promise; + return email; + }); + const reload = vi.spyOn(email, 'reload'); + const flow = email.createEmailLinkFlow(); + const prepared = vi.fn(() => flow.cancelEmailLinkFlow()); + void flow.startEmailLinkFlow({ redirectUrl: '/verify', onPrepared: prepared }); + if (phase === 'during preparation') { + flow.cancelEmailLinkFlow(); + } + held.resolve(); + await held.promise; + await Promise.resolve(); + await Promise.resolve(); + expect(prepared).toHaveBeenCalledTimes(phase === 'during preparation' ? 0 : 1); + expect(reload).not.toHaveBeenCalled(); + }, + ); + + it('rejects failed preparation without reporting readiness and allows retry', async () => { + const email = new EmailAddress({ id: 'email_123' }, '/me/email_addresses'); + vi.spyOn(email, 'prepareVerification').mockRejectedValueOnce(new Error('send failed')).mockResolvedValue(email); + email.verification.status = 'verified'; + vi.spyOn(email, 'reload').mockResolvedValue(email); + const prepared = vi.fn(); + const flow = email.createEmailLinkFlow(); + await expect(flow.startEmailLinkFlow({ redirectUrl: '/verify', onPrepared: prepared })).rejects.toThrow( + 'send failed', + ); + expect(prepared).not.toHaveBeenCalled(); + await expect(flow.startEmailLinkFlow({ redirectUrl: '/verify', onPrepared: prepared })).resolves.toBe(email); + expect(prepared).toHaveBeenCalledOnce(); + }); +}); 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 982f19eca3c..b9ddce5a7dd 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 @@ -26,6 +26,10 @@ export interface UserProfileContactListRowViewProps { onAdd?: () => void; onVerify?: (id: string) => void; onSetPrimary?: (id: string) => void; + pendingPrimaryId?: string; + pendingVerifyId?: string; + verificationReadyId?: string; + primaryChallengeVisible?: boolean; onRemove?: (item: UserProfileContact) => void; children?: ReactNode; } @@ -39,6 +43,10 @@ export function UserProfileContactListRowView({ onAdd, onVerify, onSetPrimary, + pendingPrimaryId, + pendingVerifyId, + verificationReadyId, + primaryChallengeVisible, onRemove, addAction, rowRef, @@ -94,6 +102,10 @@ export function UserProfileContactListRowView({ eligibility, onVerify, onSetPrimary, + pendingPrimaryId, + pendingVerifyId, + verificationReadyId, + primaryChallengeVisible, onRemove, }); @@ -111,6 +123,10 @@ export function UserProfileContactListRowView({ @@ -135,21 +151,43 @@ function toContactActions( eligibility, onVerify, onSetPrimary, + pendingPrimaryId, + pendingVerifyId, + verificationReadyId, + primaryChallengeVisible, onRemove, }: Pick< UserProfileContactListRowViewProps, - 'kind' | 'canManage' | 'eligibility' | 'onVerify' | 'onSetPrimary' | 'onRemove' + | 'kind' + | 'canManage' + | 'eligibility' + | 'onVerify' + | 'onSetPrimary' + | 'onRemove' + | 'verificationReadyId' + | 'pendingVerifyId' + | 'pendingPrimaryId' + | 'primaryChallengeVisible' >, ): ActionMenuAction[] { const actions: ActionMenuAction[] = []; const canManageItem = canManage?.(item.id) !== false; - const verificationAction = toContactVerificationAction(item, m, { - kind, - canManage, - eligibility, - onVerify, - onSetPrimary, - }); + const verificationAction: ActionMenuAction | undefined = + pendingPrimaryId === item.id && onSetPrimary + ? { + label: m.setPrimary, + onClick: () => onSetPrimary(item.id), + progress: primaryChallengeVisible ? 'handoff' : 'pending', + } + : toContactVerificationAction(item, m, { + kind, + canManage, + eligibility, + onVerify, + pendingVerifyId, + verificationReadyId, + onSetPrimary, + }); if (verificationAction) { actions.push(verificationAction); } @@ -173,16 +211,22 @@ function toContactVerificationAction( eligibility, onVerify, onSetPrimary, - }: Pick, + pendingVerifyId, + verificationReadyId, + }: Pick< + UserProfileContactListRowViewProps, + 'kind' | 'canManage' | 'eligibility' | 'onVerify' | 'onSetPrimary' | 'pendingVerifyId' | 'verificationReadyId' + >, ): ActionMenuAction | undefined { - const canManageItem = canManage?.(item.id) !== false; - if (!item.isVerified && (eligibility || (canManageItem && onVerify))) { - const label = item.isDefault ? m.completeVerification : m[kind].verify; - return canManageItem && onVerify ? { label, onClick: () => onVerify(item.id) } : { label, disabled: true }; - } else if (item.isVerified && !item.isDefault && (eligibility || (canManageItem && onSetPrimary))) { - return canManageItem && onSetPrimary - ? { label: m.setPrimary, onClick: () => onSetPrimary(item.id) } - : { label: m.setPrimary, disabled: true }; + if (item.isVerified && item.isDefault) { + return undefined; + } + const action = item.isVerified ? onSetPrimary : onVerify; + const enabledAction = canManage?.(item.id) !== false && action; + if (!eligibility && !enabledAction) { + return undefined; } - return undefined; + const label = item.isVerified ? m.setPrimary : item.isDefault ? m.completeVerification : m[kind].verify; + const progress = verificationReadyId === item.id ? 'handoff' : pendingVerifyId === item.id ? 'pending' : 'idle'; + return enabledAction ? { label, onClick: () => enabledAction(item.id), progress } : { label, disabled: true }; } diff --git a/packages/mosaic/src/features/user-profile/user-profile-contact.types.ts b/packages/mosaic/src/features/user-profile/user-profile-contact.types.ts index 1f178b0f450..3b9ed5e9a2a 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-contact.types.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-contact.types.ts @@ -10,12 +10,13 @@ export type UserProfileEmail = UserProfileContact; export type UserProfilePhone = UserProfileContact; export type UserProfileEmailVerification = - | { method: 'code'; sent: Promise } + | { method: 'code' } | { method: 'link'; verified: Promise; cancel: () => void } | { method: 'sso'; connect: () => Promise }; export interface UserProfileEmailVerifier { - start: () => UserProfileEmailVerification; + prepare: () => Promise; + cancel: () => void; verifyCode: (code: string) => Promise; } 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 81784f74643..1bfaffdf2c2 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 @@ -6,7 +6,7 @@ import { SaveError } from '../../../../utils/errors'; import type { UserProfileEmailVerification, UserProfileEmailVerifier } from '../../user-profile-contact.types'; import { useUserProfileAddEmailController } from '../user-profile-add-email.controller'; -const sentCode = (): UserProfileEmailVerification => ({ method: 'code', sent: Promise.resolve() }); +const sentCode = (): UserProfileEmailVerification => ({ method: 'code' }); function sentLink(verified: Promise, cancel = vi.fn()) { return (): UserProfileEmailVerification => ({ method: 'link', verified, cancel }); @@ -16,7 +16,11 @@ function verifier( start: () => UserProfileEmailVerification = sentCode, verifyCode: (code: string) => Promise = () => Promise.resolve(), ) { - return { start: vi.fn(start), verifyCode: vi.fn(verifyCode) } satisfies UserProfileEmailVerifier; + return { + prepare: vi.fn(async () => start()), + cancel: vi.fn(), + verifyCode: vi.fn(verifyCode), + } satisfies UserProfileEmailVerifier; } const created = () => Promise.resolve(verifier()); @@ -114,7 +118,7 @@ describe('useUserProfileAddEmailController', () => { }); expect(result.current.resendSeconds).toBe(30); act(() => result.current.onResend()); - expect(email.start).toHaveBeenCalledTimes(1); + expect(email.prepare).toHaveBeenCalledTimes(1); for (let second = 0; second < 30; second++) { await act(async () => vi.advanceTimersByTimeAsync(1000)); } @@ -127,7 +131,7 @@ describe('useUserProfileAddEmailController', () => { result.current.onOpenChange(false); await Promise.resolve(); }); - expect(email.start).toHaveBeenCalledTimes(2); + expect(email.prepare).toHaveBeenCalledTimes(2); expect(email.verifyCode).not.toHaveBeenCalled(); expect(result.current.open).toBe(true); expect(result.current.code).toBe(''); @@ -171,7 +175,7 @@ describe('useUserProfileAddEmailController', () => { expect(result.current.open).toBe(true); await waitFor(() => expect(result.current.step).toBe('verify')); expect(onCreate).toHaveBeenCalledExactlyOnceWith('new@example.com'); - await waitFor(() => expect(email.start).toHaveBeenCalledOnce()); + await waitFor(() => expect(email.prepare).toHaveBeenCalledOnce()); act(() => result.current.onSubmit('123456')); await waitFor(() => expect(result.current.open).toBe(false)); @@ -183,11 +187,11 @@ describe('useUserProfileAddEmailController', () => { const email = verifier(); const { result } = renderHook(() => useUserProfileAddEmailController({ onCreate })); - act(() => result.current.onVerifyEmail(other, email)); + await act(() => result.current.onVerifyEmail(other, email)); expect(result.current.open).toBe(true); expect(result.current.step).toBe('verify'); expect(result.current.emailAddress).toBe('other@example.com'); - await waitFor(() => expect(email.start).toHaveBeenCalledOnce()); + await waitFor(() => expect(email.prepare).toHaveBeenCalledOnce()); act(() => result.current.onSubmit('123456')); await waitFor(() => expect(result.current.open).toBe(false)); @@ -200,11 +204,11 @@ describe('useUserProfileAddEmailController', () => { const { result } = renderHook(() => useUserProfileAddEmailController({ onCreate: created })); expect(result.current.verifyingId).toBeUndefined(); - act(() => result.current.onVerifyEmail(other, email)); + await act(() => result.current.onVerifyEmail(other, email)); expect(result.current.verifyingId).toBe('email_2'); await waitFor(() => expect(result.current.step).toBe('verify')); - act(() => result.current.onVerifyEmail({ id: 'email_3', value: 'person@acme.co' }, email)); + await act(() => result.current.onVerifyEmail({ id: 'email_3', value: 'person@acme.co' }, email)); expect(result.current.verifyingId).toBe('email_2'); expect(result.current.emailAddress).toBe('other@example.com'); @@ -216,21 +220,113 @@ describe('useUserProfileAddEmailController', () => { expect(result.current.verifyingId).toBeUndefined(); }); - it('stays on the code step when sending fails, and lets the user resend at once', async () => { + it('keeps preparation failure at the source and retries without creating another email', async () => { const email = verifier(); - email.start.mockImplementationOnce(() => ({ - method: 'code', - sent: Promise.reject(saveError('Too many requests')), - })); - const { result } = renderHook(() => useUserProfileAddEmailController({ onCreate: created })); - - act(() => result.current.onVerifyEmail(other, email)); + email.prepare.mockRejectedValueOnce(saveError('Too many requests')); + const onCreate = vi.fn(async () => email); + const { result } = renderHook(() => useUserProfileAddEmailController({ onCreate })); + act(() => result.current.onOpenChange(true)); + act(() => result.current.onEmailAddressChange('new@example.com')); + await act(() => result.current.onSubmit()); await waitFor(() => expect(result.current.errorMessage).toBe('Too many requests')); - expect(result.current.step).toBe('verify'); + expect(result.current.step).toBe('email'); + expect(result.current.resendSeconds).toBe(0); + await act(() => result.current.onSubmit()); + await waitFor(() => expect(result.current.step).toBe('verify')); + expect(onCreate).toHaveBeenCalledOnce(); + expect(email.prepare).toHaveBeenCalledTimes(2); + }); + + it('creates a different email when the value changes after failed preparation', async () => { + const email = verifier(); + email.prepare.mockRejectedValueOnce(saveError('Try again')); + const onCreate = vi.fn(async () => email); + const { result } = renderHook(() => useUserProfileAddEmailController({ onCreate })); + act(() => result.current.onOpenChange(true)); + act(() => result.current.onEmailAddressChange('first@example.com')); + await act(() => result.current.onSubmit()); + await waitFor(() => expect(result.current.errorMessage).toBe('Try again')); + act(() => result.current.onEmailAddressChange('second@example.com')); + await act(() => result.current.onSubmit()); + await waitFor(() => expect(result.current.step).toBe('verify')); + expect(onCreate.mock.calls).toEqual([['first@example.com'], ['second@example.com']]); + }); + + it.each(['code', 'link'] as const)( + 'keeps the ready %s destination through failed resend and starts cooldown after success', + async method => { + vi.useFakeTimers(); + const email = verifier(method === 'link' ? sentLink(new Promise(() => {})) : sentCode); + const { result } = renderHook(() => useUserProfileAddEmailController({})); + await act(() => result.current.onVerifyEmail(other, email)); + for (let second = 0; second < (method === 'link' ? 60 : 30); second++) { + await act(async () => vi.advanceTimersByTimeAsync(1000)); + } + const held = createDeferredPromise(); + email.prepare.mockImplementationOnce(async () => { + await held.promise; + return sentCode(); + }); + act(() => result.current.onResend()); + expect(result.current.step).toBe(method === 'link' ? 'link' : 'verify'); + expect(result.current.isResending).toBe(true); + expect(result.current.resendSeconds).toBe(0); + await act(async () => { + held.reject(saveError('Send failed')); + await Promise.resolve(); + }); + expect(result.current.step).toBe(method === 'link' ? 'link' : 'verify'); + expect(result.current.errorMessage).toBe('Send failed'); + expect(result.current.resendSeconds).toBe(0); + await act(() => result.current.onResend()); + expect(result.current.resendSeconds).toBe(method === 'link' ? 60 : 30); + expect(result.current.errorMessage).toBeUndefined(); + }, + ); + + it('preserves a failed resend error after the previous link expired', async () => { + vi.useFakeTimers(); + const expired = createDeferredPromise(); + const email = verifier(sentLink(expired.promise.then(() => undefined))); + const { result } = renderHook(() => useUserProfileAddEmailController({})); + await act(() => result.current.onVerifyEmail(other, email)); + await act(async () => { + expired.reject(saveError('Link expired')); + await Promise.resolve(); + }); + expect(result.current.errorMessage).toBe('Link expired'); + for (let second = 0; second < 60; second++) { + await act(async () => vi.advanceTimersByTimeAsync(1000)); + } + email.prepare.mockRejectedValueOnce(saveError('Too many requests')); + await act(() => result.current.onResend()); + expect(result.current.step).toBe('link'); + expect(result.current.errorMessage).toBe('Too many requests'); expect(result.current.resendSeconds).toBe(0); + }); + it('cancels a held link resend without reopening the dialog when it settles', async () => { + vi.useFakeTimers(); + const email = verifier(sentLink(new Promise(() => {}))); + const { result } = renderHook(() => useUserProfileAddEmailController({})); + await act(() => result.current.onVerifyEmail(other, email)); + for (let second = 0; second < 60; second++) { + await act(async () => vi.advanceTimersByTimeAsync(1000)); + } + const held = createDeferredPromise(); + email.prepare.mockImplementationOnce(async () => { + await held.promise; + return sentLink(new Promise(() => {}))(); + }); act(() => result.current.onResend()); - await waitFor(() => expect(email.start).toHaveBeenCalledTimes(2)); + act(() => result.current.onOpenChange(false)); + expect(email.cancel).toHaveBeenCalled(); + expect(result.current.open).toBe(false); + await act(async () => { + held.resolve(); + await held.promise; + }); + expect(result.current.open).toBe(false); }); it('shows the error for the field that failed', async () => { @@ -248,7 +344,7 @@ describe('useUserProfileAddEmailController', () => { const verified = createDeferredPromise(); const { result } = renderHook(() => useUserProfileAddEmailController({})); - act(() => result.current.onVerifyEmail(other, verifier(sentLink(verified.promise.then(() => undefined))))); + await act(() => result.current.onVerifyEmail(other, verifier(sentLink(verified.promise.then(() => undefined))))); expect(result.current.step).toBe('link'); expect(result.current.resendSeconds).toBe(60); @@ -264,12 +360,12 @@ describe('useUserProfileAddEmailController', () => { const email = verifier(sentLink(new Promise(() => {}), cancel)); const { result, unmount } = renderHook(() => useUserProfileAddEmailController({})); - act(() => result.current.onVerifyEmail(other, email)); + await act(() => result.current.onVerifyEmail(other, email)); await waitFor(() => expect(result.current.step).toBe('link')); act(() => result.current.onOpenChange(false)); expect(cancel).toHaveBeenCalledOnce(); - act(() => result.current.onVerifyEmail(other, email)); + await act(() => result.current.onVerifyEmail(other, email)); await waitFor(() => expect(result.current.step).toBe('link')); unmount(); expect(cancel).toHaveBeenCalledTimes(2); @@ -279,7 +375,7 @@ describe('useUserProfileAddEmailController', () => { const email = verifier(sentLink(Promise.reject(saveError('Link expired')))); const { result } = renderHook(() => useUserProfileAddEmailController({})); - act(() => result.current.onVerifyEmail(other, email)); + await act(() => result.current.onVerifyEmail(other, email)); await waitFor(() => expect(result.current.errorMessage).toBe('Link expired')); expect(result.current.step).toBe('link'); expect(result.current.open).toBe(true); @@ -290,7 +386,7 @@ describe('useUserProfileAddEmailController', () => { const email = verifier(() => ({ method: 'sso', connect })); const { result } = renderHook(() => useUserProfileAddEmailController({})); - act(() => result.current.onVerifyEmail({ id: 'email_3', value: 'person@acme.co' }, email)); + await act(() => result.current.onVerifyEmail({ id: 'email_3', value: 'person@acme.co' }, email)); expect(result.current.step).toBe('sso'); expect(result.current.resendSeconds).toBe(0); diff --git a/packages/mosaic/src/features/user-profile/user-profile-email-section/__tests__/user-profile-add-email.dialog.test.tsx b/packages/mosaic/src/features/user-profile/user-profile-email-section/__tests__/user-profile-add-email.dialog.test.tsx index b2fd295b191..f30341333ce 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-email-section/__tests__/user-profile-add-email.dialog.test.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-email-section/__tests__/user-profile-add-email.dialog.test.tsx @@ -207,6 +207,17 @@ describe('UserProfileAddEmailDialog', () => { await user.click(screen.getByRole('button', { name: 'Didn’t receive a link? Resend' })); expect(props.onResend).toHaveBeenCalledOnce(); + rerender( + + + , + ); + expect(screen.getByRole('button', { name: 'Sending link…' })).toBeDisabled(); + await user.click(screen.getByRole('button', { name: 'Cancel' })); expect(props.onOpenChange).toHaveBeenCalledWith(false, expect.anything()); }); diff --git a/packages/mosaic/src/features/user-profile/user-profile-email-section/__tests__/user-profile-email-actions.test.tsx b/packages/mosaic/src/features/user-profile/user-profile-email-section/__tests__/user-profile-email-actions.test.tsx index 08d46a119e1..3ed2f7a73d4 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-email-section/__tests__/user-profile-email-actions.test.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-email-section/__tests__/user-profile-email-actions.test.tsx @@ -13,7 +13,8 @@ import type { ReadyEmailSectionModel } from '../user-profile-email-section.types import { UserProfileEmailSectionView } from '../user-profile-email-section.view'; const codeVerifier = { - start: () => ({ method: 'code', sent: Promise.resolve() }) as const, + prepare: async () => ({ method: 'code' }) as const, + cancel: () => {}, verifyCode: () => Promise.resolve(), }; 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 e24d5dad330..813757443b9 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 @@ -88,7 +88,7 @@ describe('the user profile email section', () => { const { clerk } = await renderWithClerk(); expect(retained).toBeDefined(); await act(() => clerk.setActive({ session: 'sess_2' })); - expect(() => retained?.start()).toThrow(); + await expect(retained?.prepare()).rejects.toThrow(); await expect(retained?.verifyCode('123456')).rejects.toThrow(); }); it('adds an address and verifies the code it was sent', async () => { @@ -105,22 +105,150 @@ describe('the user profile email section', () => { expect(emailRow()).not.toHaveTextContent('Unverified'); }); - it('holds the code field until the code has been sent', async () => { + it('keeps Continue pending until the code has been sent', async () => { const { actor } = await renderSection(signedInWithEmails([])); const prepare = holdRequests('post', '/v1/me/email_addresses/:id/prepare_verification'); + try { + await actor.click(screen.getByRole('button', { name: 'Add email' })); + await actor.type(screen.getByRole('textbox', { name: 'Email' }), 'new@example.com'); + await actor.click(screen.getByRole('button', { name: 'Continue' })); + await waitFor(() => expect(prepare.requests).toHaveLength(1)); + + expect(screen.getByRole('button', { name: 'Continue' })).toHaveAttribute('aria-busy', 'true'); + expect(screen.getByRole('textbox', { name: 'Email' })).toBeVisible(); + expect(screen.queryByRole('textbox', { name: 'Verification code' })).not.toBeInTheDocument(); + + prepare.release(); + await enterCode(actor, VERIFICATION_CODE); + + await waitFor(() => expect(screen.queryByRole('dialog')).not.toBeInTheDocument()); + expect(emailsListed()).toEqual(['new@example.com']); + } finally { + prepare.release(); + } + }); - await actor.click(screen.getByRole('button', { name: 'Add email' })); - await actor.type(screen.getByRole('textbox', { name: 'Email' }), 'new@example.com'); - await actor.click(screen.getByRole('button', { name: 'Continue' })); - await waitFor(() => expect(prepare.requests).toHaveLength(1)); - - expect(await screen.findByRole('textbox', { name: 'Verification code' })).toHaveAttribute('aria-disabled', 'true'); + it.each(['code', 'link'] as const)( + 'keeps existing email Verify pending until %s preparation succeeds', + async method => { + const pending = fapiEmailAddress({ id: 'idn_pending', email_address: 'pending@example.com' }); + const { actor } = await renderSection( + signedInWithEmails([PRIMARY, pending], method === 'link' ? verifiesByLink : fapiEnvironment()), + ); + 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)); + const action = screen.getByRole('menuitem', { name: 'Verify' }); + const add = screen.getByRole('button', { name: 'Add email' }); + expect(add).toBeDisabled(); + await actor.click(add); + expect(screen.queryByRole('dialog')).toBeNull(); + expect(action).toHaveAttribute('aria-busy', 'true'); + expect(screen.queryByRole('dialog')).toBeNull(); + prepare.release(); + await screen.findByRole('dialog'); + if (method === 'code') { + await waitFor(() => expect(screen.getByRole('textbox', { name: 'Verification code' })).toHaveFocus()); + } else { + await waitFor(() => + expect(screen.getByRole('dialog')).toHaveTextContent('Open the link we sent to pending@example.com'), + ); + } + } finally { + prepare.release(); + } + }, + ); + + it.each(['code', 'link'] as const)( + 'retries failed %s preparation from the entry form without another email', + async method => { + const { actor, fapi } = await renderSection( + signedInWithEmails([], method === 'link' ? verifiesByLink : fapiEnvironment()), + ); + const prepare = holdRequests('post', '/v1/me/email_addresses/:id/prepare_verification'); + try { + await actor.click(screen.getByRole('button', { name: 'Add email' })); + await actor.type(screen.getByRole('textbox', { name: 'Email' }), 'new@example.com'); + await actor.click(screen.getByRole('button', { name: 'Continue' })); + await waitFor(() => expect(prepare.requests).toHaveLength(1)); + expect(screen.queryByRole('textbox', { name: 'Verification code' })).toBeNull(); + prepare.fail('action_blocked'); + await waitFor(() => + expect(screen.getByRole('button', { name: 'Continue' })).not.toHaveAttribute('aria-busy', 'true'), + ); + expect(screen.getByRole('textbox', { name: 'Email' })).toHaveValue('new@example.com'); + const retry = holdRequests('post', '/v1/me/email_addresses/:id/prepare_verification'); + try { + await actor.click(screen.getByRole('button', { name: 'Continue' })); + await waitFor(() => expect(retry.requests).toHaveLength(1)); + expect(fapi.client.sessions[0]?.user.email_addresses).toHaveLength(1); + retry.release(); + if (method === 'code') { + expect(await screen.findByRole('textbox', { name: 'Verification code' })).toBeVisible(); + } else { + await waitFor(() => + expect(screen.getByRole('dialog')).toHaveTextContent('Open the link we sent to new@example.com'), + ); + } + } finally { + retry.release(); + } + } finally { + prepare.release(); + } + }, + ); - prepare.release(); - await enterCode(actor, VERIFICATION_CODE); + it('cancels link preparation when the active account changes', async () => { + const pending = fapiEmailAddress({ id: 'idn_pending', email_address: 'pending@example.com' }); + const alice = fapiUser({ id: 'user_1', email_addresses: [PRIMARY, pending] }); + const bob = fapiUser({ id: 'user_2', email_addresses: [fapiEmailAddress({ id: 'idn_bob' })] }); + const { actor, clerk } = await renderSection({ + environment: verifiesByLink, + client: fapiClient([fapiSession({ id: 'sess_1', user: alice }), fapiSession({ id: 'sess_2', user: bob })]), + }); + const email = clerk.user?.emailAddresses.find(value => value.id === pending.id); + if (!email) { + throw new Error('Missing email'); + } + const reload = vi.spyOn(email, 'reload'); + const preparing = vi.spyOn(email, 'prepareVerification'); + 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)); + await act(() => clerk.setActive({ session: 'sess_2' })); + prepare.release(); + await Promise.allSettled(preparing.mock.results.map(result => result.value)); + expect(reload).not.toHaveBeenCalled(); + expect(screen.queryByRole('dialog')).toBeNull(); + } finally { + prepare.release(); + } + }); - await waitFor(() => expect(screen.queryByRole('dialog')).not.toBeInTheDocument()); - expect(emailsListed()).toEqual(['new@example.com']); + it('does not begin link polling after unmount during preparation', async () => { + const pending = fapiEmailAddress({ id: 'idn_pending', email_address: 'pending@example.com' }); + const { actor, clerk, unmount } = await renderSection(signedInWithEmails([PRIMARY, pending], verifiesByLink)); + const email = clerk.user?.emailAddresses.find(value => value.id === pending.id); + if (!email) { + throw new Error('Missing email'); + } + const reload = vi.spyOn(email, 'reload'); + 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)); + unmount(); + prepare.release(); + await waitFor(() => expect(email.verification.strategy).toBe('email_link')); + expect(reload).not.toHaveBeenCalled(); + expect(screen.queryByRole('dialog')).toBeNull(); + } finally { + prepare.release(); + } }); it('verifies an address left unverified', async () => { @@ -351,21 +479,81 @@ describe('email reverification', () => { ); }); - it('waits for a verification factor before opening the dialog', async () => { - const second = fapiEmailAddress({ id: 'idn_second', email_address: 'bob@example.com', verification: VERIFIED }); - const { actor } = await renderSection({ - ...signedInWithEmails([PRIMARY, second]), - verification: { secrets: { password: 'hunter2' } }, - }); - demandReverificationOnce('/v1/me'); - const start = holdRequests('post', '/v1/client/sessions/:id/verify'); - - await manageEmail(actor, 'bob@example.com', 'Set as primary'); - await waitFor(() => expect(start.requests).toHaveLength(1)); - expect(screen.queryByRole('dialog')).toBeNull(); + it.each(['pointer', 'keyboard'])( + 'keeps the %s action pending in its menu until verification is ready', + async input => { + const second = fapiEmailAddress({ id: 'idn_second', email_address: 'bob@example.com', verification: VERIFIED }); + const { actor } = await renderSection({ + ...signedInWithEmails([PRIMARY, second]), + verification: { secrets: { password: 'hunter2' } }, + }); + demandReverificationOnce('/v1/me'); + const mutation = holdRequests('post', '/v1/me'); + const start = holdRequests('post', '/v1/client/sessions/:id/verify'); + const trigger = within(emailRow()).getByRole('button', { name: 'Manage bob@example.com' }); + + try { + await openMenu(actor, trigger); + if (input === 'keyboard') { + await actor.keyboard('{ArrowDown}{Enter}'); + } else { + await actor.click(screen.getByRole('menuitem', { name: 'Set as primary' })); + } + await waitFor(() => expect(mutation.requests).toHaveLength(1)); + const selected = screen.getByRole('menuitem', { name: 'Set as primary' }); + expect(selected).toHaveAttribute('aria-busy', 'true'); + expect(selected).toHaveFocus(); + expect(await within(selected).findByRole('progressbar')).toBeVisible(); + const remove = screen.getByRole('menuitem', { name: 'Remove email' }); + expect(remove).toHaveAttribute('aria-disabled', 'true'); + await actor.keyboard('{Enter}{Escape}'); + await actor.click(selected); + await actor.click(remove); + await actor.click(screen.getByRole('heading', { name: 'Email' })); + expect(screen.getByRole('menu')).toBeVisible(); + expect(screen.queryByRole('alertdialog')).toBeNull(); + expect(mutation.requests).toHaveLength(1); + + mutation.release(); + await waitFor(() => expect(start.requests).toHaveLength(1)); + expect(screen.getByRole('menu')).toBeVisible(); + expect(screen.queryByRole('dialog')).toBeNull(); + start.release(); + + await waitFor(() => expect(screen.queryByRole('menu')).toBeNull()); + await waitFor(() => expect(screen.getByLabelText('Password')).toHaveFocus()); + await actor.keyboard('{Escape}'); + await waitFor(() => expect(screen.queryByRole('dialog')).toBeNull()); + await waitFor(() => expect(trigger).toHaveFocus()); + } finally { + mutation.release(); + start.release(); + } + }, + ); - start.release(); - expect(await screen.findByText('Enter your current password to continue')).toBeVisible(); + it('closes the pending menu and shows the section error when the primary action fails', async () => { + const second = fapiEmailAddress({ id: 'idn_second', email_address: 'bob@example.com', verification: VERIFIED }); + const { actor } = await renderSection(signedInWithEmails([PRIMARY, second])); + const mutation = holdRequests('post', '/v1/me'); + const trigger = within(emailRow()).getByRole('button', { name: 'Manage bob@example.com' }); + + try { + await manageEmail(actor, 'bob@example.com', 'Set as primary'); + await waitFor(() => expect(mutation.requests).toHaveLength(1)); + expect(screen.getByRole('menu')).toBeVisible(); + mutation.fail('action_blocked', 'Unable to set primary address.'); + await waitFor(() => expect(screen.queryByRole('menu')).toBeNull()); + await waitFor(() => + expect(within(emailRow()).getByRole('alert')).toHaveTextContent("This action couldn't be completed."), + ); + expect(screen.queryByRole('dialog')).toBeNull(); + await waitFor(() => expect(trigger).toHaveFocus()); + await openMenu(actor, trigger); + expect(screen.getByRole('menuitem', { name: 'Set as primary' })).not.toHaveAttribute('aria-disabled'); + } finally { + mutation.release(); + } }); it('focuses Close when the dialog opens on a verification that could not start', 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 eebbc61da14..136b7e526ed 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 @@ -1,11 +1,11 @@ -import { useEffect, useMemo } from 'react'; +import { useEffect, useMemo, useRef } from 'react'; import type { DialogHandle } from '../../../components/dialog'; import { Dialog } from '../../../components/dialog'; import { useForm } from '../../../components/form'; import { useErrorText } from '../../../localization'; import { setup } from '../../../machine/setup'; -import type { ErrorInvokeEvent } from '../../../machine/types'; +import type { DoneInvokeEvent, ErrorInvokeEvent } from '../../../machine/types'; import { useMachine } from '../../../machine/use-machine'; import type { FormError } from '../../../utils/errors'; import { toFormError } from '../../../utils/errors'; @@ -27,7 +27,7 @@ export interface UserProfileAddEmailControllerOptions { export interface UserProfileAddEmailController extends UserProfileAddEmailDialogProps { handle: DialogHandle; verifyingId: string | undefined; - onVerifyEmail: (email: Pick, verifier: UserProfileEmailVerifier) => void; + onVerifyEmail: (email: Pick, verifier: UserProfileEmailVerifier) => Promise; } interface Context { @@ -41,7 +41,13 @@ interface Context { type Event = | { type: 'ADD' } - | { type: 'START'; id?: string; emailAddress: string; verifier: UserProfileEmailVerifier } + | { + type: 'READY'; + id?: string; + emailAddress: string; + verifier: UserProfileEmailVerifier; + verification: UserProfileEmailVerification; + } | { type: 'VERIFIED' } | { type: 'CANCEL' } | { type: 'CONNECT' } @@ -60,19 +66,24 @@ const LINK_RESEND_SECONDS = 60; const tick = { actions: assign(context => ({ resendSeconds: Math.max(0, context.resendSeconds - 1) })) }; const fail = assign((_, event) => ({ error: toFormError(event.error) })); const resend = { - target: 'starting', + target: 'resending', guard: (context: Context) => context.resendSeconds === 0, actions: assign(() => ({ error: undefined })), }; -const start = { - target: 'starting', - actions: assign>((_, event) => ({ +const ready = { + target: 'ready', + actions: assign>((_, event) => ({ verifyingId: event.id, emailAddress: event.emailAddress, verifier: event.verifier, - verification: undefined, + verification: event.verification, error: undefined, - resendSeconds: 0, + resendSeconds: + event.verification.method === 'link' + ? LINK_RESEND_SECONDS + : event.verification.method === 'code' + ? CODE_RESEND_SECONDS + : 0, })), }; @@ -100,36 +111,37 @@ const machine = createMachine({ resendSeconds: 0, })), }, - START: start, + READY: ready, }, }, email: { - on: { CANCEL: 'idle', START: start }, + on: { CANCEL: 'idle', READY: ready }, }, - starting: { - entry: assign(context => ({ - verification: context.verifier ? context.verifier.start() : undefined, - error: undefined, - })), - always: ({ context }) => { - const method = context.verification?.method; - if (method === 'code') { - return { target: 'sending' }; - } - if (method === 'sso') { - return { target: 'connecting' }; - } - return { target: 'waiting', context: { resendSeconds: method === 'link' ? LINK_RESEND_SECONDS : 0 } }; - }, + ready: { + always: ({ context }) => ({ + target: + context.verification?.method === 'code' + ? 'verify' + : context.verification?.method === 'sso' + ? 'connecting' + : 'waiting', + }), }, - 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 }, + resending: { + on: { CANCEL: { target: 'idle', guard: context => context.verification?.method === 'link' } }, + invoke: fromPromise(context => (context.verifier ? context.verifier.prepare() : missingDependency()), { + onDone: { + target: 'ready', + actions: assign>((_, event) => ({ + verification: event.output, + resendSeconds: event.output.method === 'link' ? LINK_RESEND_SECONDS : CODE_RESEND_SECONDS, + })), }, - ), + onError: [ + { target: 'linkFailed', guard: context => context.verification?.method === 'link', actions: fail }, + { target: 'verify', actions: fail }, + ], + }), }, waiting: { on: { CANCEL: 'idle', TICK: tick, RESEND: resend }, @@ -137,10 +149,13 @@ const machine = createMachine({ context => (context.verification?.method === 'link' ? context.verification.verified : missingDependency()), { onDone: 'idle', - onError: { actions: fail }, + onError: { target: 'linkFailed', actions: fail }, }, ), }, + linkFailed: { + on: { CANCEL: 'idle', TICK: tick, RESEND: resend }, + }, connecting: { on: { CANCEL: 'idle', CONNECT: 'redirecting' }, }, @@ -167,13 +182,36 @@ export function useUserProfileAddEmailController({ const errorText = useErrorText(); const handle = useMemo(() => Dialog.createHandle(), []); const [snapshot, send, actor] = useMachine(machine); + const activeVerifier = useRef(undefined); + const created = useRef<{ value: string; verifier: UserProfileEmailVerifier }>(undefined); + const mounted = useRef(true); + const generation = useRef(0); + useEffect(() => { + mounted.current = true; + return () => { + mounted.current = false; + activeVerifier.current?.cancel(); + }; + }, []); const { resendSeconds, error, verification } = snapshot.context; const emailForm = useForm({ initialValues: { emailAddress: initialEmailAddress }, canSubmit: values => !username || values.emailAddress !== username, onSubmit: async ({ emailAddress }) => { if (onCreate) { - send({ type: 'START', emailAddress, verifier: await onCreate(emailAddress) }); + if (created.current?.value !== emailAddress) { + const verifier = await onCreate(emailAddress); + if (!mounted.current) { + return; + } + created.current = { value: emailAddress, verifier }; + } + const { verifier } = created.current; + activeVerifier.current = verifier; + const verification = await verifier.prepare(); + if (mounted.current) { + send({ type: 'READY', emailAddress, verifier, verification }); + } } }, }); @@ -207,7 +245,7 @@ export function useUserProfileAddEmailController({ return { handle, resendSeconds, - isResending: state === 'sending', + isResending: state === 'resending', open, step, emailAddress: step === 'email' ? emailForm.values.emailAddress : snapshot.context.emailAddress, @@ -216,14 +254,17 @@ export function useUserProfileAddEmailController({ errorMessage: formError?.message ?? form.error ?? machineError, isPending: form.isSubmitting || state === 'redirecting', onOpenChange: open => { - if (form.isSubmitting) { + if (form.isSubmitting || (actor.getSnapshot().value === 'resending' && verification?.method !== 'link')) { return; } + generation.current++; if (open) { + created.current = undefined; emailForm.reset(); codeForm.reset(); send({ type: 'ADD' }); } else { + activeVerifier.current?.cancel(); send({ type: 'CANCEL' }); } }, @@ -248,9 +289,19 @@ export function useUserProfileAddEmailController({ }, onConnect: () => send({ type: 'CONNECT' }), verifyingId: snapshot.context.verifyingId, - onVerifyEmail: (email, verifier) => { - codeForm.reset(); - send({ type: 'START', id: email.id, emailAddress: email.value, verifier }); + onVerifyEmail: async (email, verifier) => { + if (actor.getSnapshot().value !== 'idle') { + return; + } + const started = generation.current; + activeVerifier.current = verifier; + const verification = await verifier.prepare(); + if (mounted.current && generation.current === started && actor.getSnapshot().value === 'idle') { + codeForm.reset(); + send({ type: 'READY', id: email.id, emailAddress: email.value, verifier, verification }); + } else { + verifier.cancel(); + } }, }; } diff --git a/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-add-email.dialog.tsx b/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-add-email.dialog.tsx index d7559e90f3e..5d1926c50ff 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-add-email.dialog.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-add-email.dialog.tsx @@ -83,6 +83,7 @@ export function UserProfileAddEmailDialog(props: UserProfileAddEmailDialogProps) @@ -276,6 +277,7 @@ function VerifyEmailStep(props: VerifyEmailStepProps) { } interface VerifyLinkStepProps { + isResending?: boolean; emailAddress: string; onResend: () => void; errorMessage?: string; @@ -302,6 +304,7 @@ function VerifyLinkStep(props: VerifyLinkStepProps) { ) : null} diff --git a/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-add-email.messages.ts b/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-add-email.messages.ts index 7cd6329823a..c5651a34723 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-add-email.messages.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-add-email.messages.ts @@ -22,6 +22,7 @@ export const userProfileAddEmailMessages = { description: 'Open the link we sent to {emailAddress}', cancel: 'Cancel', resend: 'Didn’t receive a link? Resend', + resending: 'Sending link…', resendCountdown: 'Didn’t receive a link? Resend ({seconds})', }, sso: { 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 b3f94ee4a8b..78597a2132c 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 @@ -21,6 +21,7 @@ export function useUserProfileEmailSectionController({ }: Omit): UserProfileEmailSectionViewProps { const verification = useUserProfileAddEmailController({ username, onCreate: onCreateEmail }); const setPrimary = usePendingAction(); + const verify = usePendingAction(); const rowRef = useRef(null); const removalFocus = useListRemovalFocus({ ids: emails.map(email => email.id), @@ -38,7 +39,9 @@ export function useUserProfileEmailSectionController({ canManageEmail, canAdd: Boolean(onCreateEmail), verification, - error: setPrimary.error, + error: verify.error ?? setPrimary.error, + pendingVerifyId: verify.pendingKey, + pendingPrimaryId: setPrimary.pendingKey, rowRef, registerTrigger: removalFocus.registerTrigger, remove: onRemoveEmail ? remove : undefined, @@ -49,7 +52,7 @@ export function useUserProfileEmailSectionController({ onVerify: id => { const email = emails.find(email => email.id === id); if (email) { - verification.onVerifyEmail(email, getEmailVerifier(id)); + void verify.run(id, () => verification.onVerifyEmail(email, getEmailVerifier(id))); } }, onSetPrimary: id => void setPrimary.run(id, () => onSetPrimaryEmail(id)), diff --git a/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-email-section.model.ts b/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-email-section.model.ts index 2dff9a6865f..33b6c8aaaba 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-email-section.model.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-email-section.model.ts @@ -1,5 +1,6 @@ import { buildURL } from '@clerk/shared/internal/clerk-js/url'; import type { EmailAddressResource, UserResource } from '@clerk/shared/types'; +import { createDeferredPromise } from '@clerk/shared/utils'; import type { MosaicRouter } from '../../../hooks/use-mosaic-router'; import { useMosaicRouter } from '../../../hooks/use-mosaic-router'; @@ -28,11 +29,12 @@ function verifyRedirectUrl(userProfileUrl: string): string { return buildURL({ base: userProfileUrl, hashPath: '/verify' }, { stringify: true }); } -function startEmailVerification( +async function prepareEmailVerification( currentEmail: () => EmailAddressResource, linkRedirectUrl: string | undefined, router: MosaicRouter, -): UserProfileEmailVerification { + setCancel: (cancel: () => void) => void, +): Promise { if (currentEmail().matchesSsoConnection) { return { method: 'sso', @@ -62,17 +64,28 @@ function startEmailVerification( }; } if (linkRedirectUrl === undefined) { - return { method: 'code', sent: save(() => currentEmail().prepareVerification({ strategy: 'email_code' })) }; + await save(() => currentEmail().prepareVerification({ strategy: 'email_code' })); + currentEmail(); + return { method: 'code' }; } const { startEmailLinkFlow, cancelEmailLinkFlow } = currentEmail().createEmailLinkFlow(); - return { - method: 'link', - verified: save(() => { - currentEmail(); - return startEmailLinkFlow({ redirectUrl: linkRedirectUrl }); + const prepared = createDeferredPromise(); + setCancel(cancelEmailLinkFlow); + const verified = save(() => + startEmailLinkFlow({ + redirectUrl: linkRedirectUrl, + onPrepared: () => { + currentEmail(); + prepared.resolve(); + }, }), - cancel: cancelEmailLinkFlow, - }; + ); + void verified.catch((error: unknown) => { + cancelEmailLinkFlow(); + prepared.reject(error); + }); + await prepared.promise; + return { method: 'link', verified, cancel: cancelEmailLinkFlow }; } function toEmailVerifier( @@ -80,8 +93,15 @@ function toEmailVerifier( linkRedirectUrl: string | undefined, router: MosaicRouter, ): UserProfileEmailVerifier { + let cancel = () => {}; return { - start: () => startEmailVerification(currentEmail, linkRedirectUrl, router), + prepare: () => { + cancel(); + return prepareEmailVerification(currentEmail, linkRedirectUrl, router, next => { + cancel = next; + }); + }, + cancel: () => cancel(), verifyCode: code => save(() => currentEmail().attemptVerification({ code }), ADD_EMAIL_FIELDS), }; } 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 bd6fe076182..60a587062aa 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 @@ -21,6 +21,8 @@ export interface UserProfileEmailSectionViewProps { verificationFinalFocus: DialogFocusTarget; setPrimaryReverification: ReverificationController; setPrimaryFinalFocus: DialogFocusTarget; + pendingPrimaryId?: string; + pendingVerifyId?: string; onVerify: (id: string) => void; onSetPrimary: (id: string) => void; } 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 1b0b2cf8de4..dc8eff65949 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 @@ -23,6 +23,8 @@ export function UserProfileEmailSectionView({ verificationFinalFocus, setPrimaryReverification, setPrimaryFinalFocus, + pendingPrimaryId, + pendingVerifyId, onVerify, onSetPrimary, }: UserProfileEmailSectionViewProps) { @@ -33,6 +35,7 @@ export function UserProfileEmailSectionView({ render={ + ) : null} diff --git a/packages/mosaic/src/features/user-profile/user-profile-phone-section/__tests__/user-profile-add-phone.controller.test.ts b/packages/mosaic/src/features/user-profile/user-profile-phone-section/__tests__/user-profile-add-phone.controller.test.ts index 3340d462079..672e1ac134f 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-phone-section/__tests__/user-profile-add-phone.controller.test.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-phone-section/__tests__/user-profile-add-phone.controller.test.ts @@ -43,12 +43,12 @@ describe('useUserProfileAddPhoneController', () => { const verifier = { sendCode, verifyCode: () => Promise.resolve() }; expect(result.current.verifyingId).toBeUndefined(); - act(() => result.current.onVerifyPhone({ id: 'phone_2', value: '+18015550101' }, verifier)); + await act(() => result.current.onVerifyPhone({ id: 'phone_2', value: '+18015550101' }, verifier)); expect(result.current.verifyingId).toBe('phone_2'); await act(async () => vi.advanceTimersByTimeAsync(0)); expect(result.current.step).toBe('verify'); - act(() => result.current.onVerifyPhone({ id: 'phone_3', value: '+18015550102' }, verifier)); + await act(() => result.current.onVerifyPhone({ id: 'phone_3', value: '+18015550102' }, verifier)); expect(result.current.verifyingId).toBe('phone_2'); expect(result.current.phoneNumber).toBe('+18015550101'); diff --git a/packages/mosaic/src/features/user-profile/user-profile-phone-section/__tests__/user-profile-add-phone.dialog.test.tsx b/packages/mosaic/src/features/user-profile/user-profile-phone-section/__tests__/user-profile-add-phone.dialog.test.tsx index 6b29b222601..a4000e606c1 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-phone-section/__tests__/user-profile-add-phone.dialog.test.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-phone-section/__tests__/user-profile-add-phone.dialog.test.tsx @@ -55,8 +55,7 @@ describe('UserProfileAddPhoneDialog', () => { const user = userEvent.setup(); const onSubmit = vi.fn(); render(); - await waitFor(() => expect(screen.getByRole('button', { name: 'Close', exact: true })).toHaveFocus()); - await user.click(screen.getByRole('textbox', { name: 'Verification code' })); + await waitFor(() => expect(screen.getByRole('textbox', { name: 'Verification code' })).toHaveFocus()); if (method === 'typing') { await user.keyboard('12345'); 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 ab7649eefc4..499fa3a3295 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 @@ -339,6 +339,10 @@ describe('phone reverification', () => { await manage(actor, WORK_LABEL, 'Set as primary'); await waitFor(() => expect(start.requests).toHaveLength(1)); expect(screen.queryByRole('dialog')).toBeNull(); + const selected = screen.getByRole('menuitem', { name: 'Set as primary' }); + expect(selected).toHaveAttribute('aria-busy', 'true'); + expect(await within(selected).findByRole('progressbar')).toBeVisible(); + expect(screen.getByRole('menuitem', { name: 'Remove phone number' })).toHaveAttribute('aria-disabled', 'true'); start.release(); expect(await screen.findByText('Enter your current password to continue')).toBeVisible(); @@ -363,3 +367,66 @@ describe('Removing a number', () => { expect(screen.queryByRole('alertdialog')).toBeNull(); }); }); + +it('keeps Send code pending until phone verification is ready', async () => { + const { actor } = await renderSection(); + const prepare = holdRequests('post', '/v1/me/phone_numbers/:id/prepare_verification'); + try { + await addPhone(actor, '8015550100'); + await waitFor(() => expect(prepare.requests).toHaveLength(1)); + expect(screen.getByRole('button', { name: 'Send code' })).toHaveAttribute('aria-busy', 'true'); + expect(screen.getByRole('textbox', { name: 'Phone' })).toBeVisible(); + expect(screen.queryByRole('textbox', { name: 'Verification code' })).not.toBeInTheDocument(); + prepare.release(); + await enterCode(actor, VERIFICATION_CODE); + await waitFor(() => expect(screen.queryByRole('dialog')).not.toBeInTheDocument()); + } finally { + prepare.release(); + } +}); + +it('keeps an existing phone Verify pending and focuses the prepared code field', async () => { + const pending = fapiPhoneNumber({ + id: 'idn_pending', + phone_number: '+18015550199', + verification: fapiVerification('phone_code', { status: 'unverified' }), + }); + 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('menuitem', { name: 'Verify phone number' })).toHaveAttribute('aria-busy', 'true'); + expect(screen.getByRole('button', { name: 'Add phone number' })).toBeDisabled(); + expect(screen.queryByRole('dialog')).toBeNull(); + prepare.release(); + await waitFor(() => expect(screen.getByRole('textbox', { name: 'Verification code' })).toHaveFocus()); + } finally { + prepare.release(); + } +}); + +it('retries phone preparation from the entry form without creating another number', async () => { + const { actor, fapi } = await renderSection(); + const prepare = holdRequests('post', '/v1/me/phone_numbers/:id/prepare_verification'); + try { + await addPhone(actor, '8015550100'); + await waitFor(() => expect(prepare.requests).toHaveLength(1)); + prepare.fail('action_blocked'); + await waitFor(() => + expect(screen.getByRole('button', { name: 'Send code' })).not.toHaveAttribute('aria-busy', 'true'), + ); + const retry = holdRequests('post', '/v1/me/phone_numbers/:id/prepare_verification'); + try { + await actor.click(screen.getByRole('button', { name: 'Send code' })); + await waitFor(() => expect(retry.requests).toHaveLength(1)); + expect(fapi.client.sessions[0]?.user.phone_numbers).toHaveLength(1); + retry.release(); + expect(await screen.findByRole('textbox', { name: 'Verification code' })).toBeVisible(); + } finally { + retry.release(); + } + } finally { + prepare.release(); + } +}); 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 8293a6463b1..866e1793257 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 @@ -1,4 +1,4 @@ -import { useEffect, useMemo } from 'react'; +import { useEffect, useMemo, useRef } from 'react'; import type { DialogHandle } from '../../../components/dialog'; import { Dialog } from '../../../components/dialog'; @@ -21,7 +21,7 @@ export interface UserProfileAddPhoneControllerOptions { export interface UserProfileAddPhoneController extends UserProfileAddPhoneDialogProps { handle: DialogHandle; verifyingId: string | undefined; - onVerifyPhone: (phone: Pick, verifier: UserProfilePhoneVerifier) => void; + onVerifyPhone: (phone: Pick, verifier: UserProfilePhoneVerifier) => Promise; } interface Context { @@ -34,7 +34,7 @@ interface Context { type Event = | { type: 'ADD' } - | { type: 'START'; id?: string; phoneNumber: string; verifier: UserProfilePhoneVerifier } + | { type: 'READY'; id?: string; phoneNumber: string; verifier: UserProfilePhoneVerifier } | { type: 'VERIFIED' } | { type: 'CANCEL' } | { type: 'RESEND' } @@ -50,14 +50,14 @@ const CODE_RESEND_SECONDS = 30; const tick = { actions: assign(context => ({ resendSeconds: Math.max(0, context.resendSeconds - 1) })) }; const fail = assign((_, event) => ({ error: toFormError(event.error) })); -const start = { - target: 'sending', - actions: assign>((_, event) => ({ +const ready = { + target: 'verify', + actions: assign>((_, event) => ({ verifyingId: event.id, phoneNumber: event.phoneNumber, verifier: event.verifier, error: undefined, - resendSeconds: 0, + resendSeconds: CODE_RESEND_SECONDS, })), }; @@ -78,11 +78,11 @@ const machine = createMachine({ target: 'phone', actions: assign(() => ({ verifyingId: undefined, verifier: undefined, error: undefined, resendSeconds: 0 })), }, - START: start, + READY: ready, }, }, phone: { - on: { CANCEL: 'idle', START: start }, + on: { CANCEL: 'idle', READY: ready }, }, sending: { invoke: fromPromise(context => (context.verifier ? context.verifier.sendCode() : missingDependency()), { @@ -111,12 +111,32 @@ export function useUserProfileAddPhoneController({ const errorText = useErrorText(); const handle = useMemo(() => Dialog.createHandle(), []); const [snapshot, send, actor] = useMachine(machine); + const created = useRef<{ value: string; verifier: UserProfilePhoneVerifier }>(undefined); + const mounted = useRef(true); + const generation = useRef(0); + useEffect(() => { + mounted.current = true; + return () => { + mounted.current = false; + }; + }, []); const { resendSeconds, error } = snapshot.context; const phoneForm = useForm({ initialValues: { phoneNumber: '' }, onSubmit: async ({ phoneNumber }) => { if (onCreate) { - send({ type: 'START', phoneNumber, verifier: await onCreate(phoneNumber) }); + if (created.current?.value !== phoneNumber) { + const verifier = await onCreate(phoneNumber); + if (!mounted.current) { + return; + } + created.current = { value: phoneNumber, verifier }; + } + const { verifier } = created.current; + await verifier.sendCode(); + if (mounted.current) { + send({ type: 'READY', phoneNumber, verifier }); + } } }, }); @@ -159,7 +179,9 @@ export function useUserProfileAddPhoneController({ if (form.isSubmitting) { return; } + generation.current++; if (open) { + created.current = undefined; phoneForm.reset(); codeForm.reset(); send({ type: 'ADD' }); @@ -187,9 +209,16 @@ export function useUserProfileAddPhoneController({ } }, verifyingId: snapshot.context.verifyingId, - onVerifyPhone: (phone, verifier) => { - codeForm.reset(); - send({ type: 'START', id: phone.id, phoneNumber: phone.value, verifier }); + onVerifyPhone: async (phone, verifier) => { + if (actor.getSnapshot().value !== 'idle') { + return; + } + const started = generation.current; + await verifier.sendCode(); + if (mounted.current && generation.current === started && actor.getSnapshot().value === 'idle') { + codeForm.reset(); + send({ type: 'READY', id: phone.id, phoneNumber: phone.value, verifier }); + } }, }; } diff --git a/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-add-phone.dialog.tsx b/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-add-phone.dialog.tsx index e745067d2cd..07d92f56b51 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-add-phone.dialog.tsx +++ b/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-add-phone.dialog.tsx @@ -28,6 +28,7 @@ export interface UserProfileAddPhoneDialogProps { export function UserProfileAddPhoneDialog(props: UserProfileAddPhoneDialogProps) { const phoneRef = useRef(null); + const codeRef = useRef(null); return ( ): UserProfilePhoneSectionViewProps { const verification = useUserProfileAddPhoneController({ onCreate: onCreatePhone }); const setPrimary = usePendingAction(); + const verify = usePendingAction(); const rowRef = useRef(null); const removalFocus = useListRemovalFocus({ ids: phones.map(phone => phone.id), @@ -36,7 +37,9 @@ export function useUserProfilePhoneSectionController({ defaultPhoneCountry, canAdd: Boolean(onCreatePhone), verification, - error: setPrimary.error, + error: verify.error ?? setPrimary.error, + pendingVerifyId: verify.pendingKey, + pendingPrimaryId: setPrimary.pendingKey, rowRef, registerTrigger: removalFocus.registerTrigger, remove: onRemovePhone ? remove : undefined, @@ -47,7 +50,7 @@ export function useUserProfilePhoneSectionController({ onVerify: id => { const phone = phones.find(phone => phone.id === id); if (phone) { - verification.onVerifyPhone(phone, getPhoneVerifier(id)); + void verify.run(id, () => verification.onVerifyPhone(phone, getPhoneVerifier(id))); } }, onSetPrimary: id => void setPrimary.run(id, () => onSetPrimaryPhone(id)), diff --git a/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-phone-section.model.ts b/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-phone-section.model.ts index b22f5892fc2..03f3c91313a 100644 --- a/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-phone-section.model.ts +++ b/packages/mosaic/src/features/user-profile/user-profile-phone-section/user-profile-phone-section.model.ts @@ -12,10 +12,14 @@ import type { UserProfilePhoneSectionModel } from './user-profile-phone-section. const ADD_PHONE_FIELDS: readonly UserProfileAddPhoneField[] = ['phoneNumber', 'code']; -function toPhoneVerifier(phone: PhoneNumberResource): UserProfilePhoneVerifier { +function toPhoneVerifier(currentPhone: () => PhoneNumberResource): UserProfilePhoneVerifier { return { - sendCode: () => save(() => phone.prepareVerification()), - verifyCode: code => save(() => phone.attemptVerification({ code }), ADD_PHONE_FIELDS), + sendCode: () => + save(async () => { + await currentPhone().prepareVerification(); + currentPhone(); + }), + verifyCode: code => save(() => currentPhone().attemptVerification({ code }), ADD_PHONE_FIELDS), }; } @@ -53,10 +57,12 @@ export function useUserProfilePhoneSectionModel(): UserProfilePhoneSectionModel ? async phoneNumber => { const request = currentUser().createPhoneNumber({ phoneNumber }); await save(() => request, ADD_PHONE_FIELDS); - return toPhoneVerifier(await request); + currentUser(); + const phone = await request; + return toPhoneVerifier(() => byId(currentUser().phoneNumbers, phone.id, 'phone number')); } : undefined, - getPhoneVerifier: id => toPhoneVerifier(byId(user.phoneNumbers, id, 'phone number')), + getPhoneVerifier: id => toPhoneVerifier(() => byId(currentUser().phoneNumbers, id, 'phone number')), onSetPrimaryPhone: id => saveAsUser(current => setPrimaryPhone(current, id)), onRemovePhone: access.canRemove ? id => saveAsUser(current => removePhone(current, id)) : undefined, removeReverification, 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 ba90fb5a1ba..ddd9ee03760 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 @@ -20,6 +20,8 @@ export interface UserProfilePhoneSectionViewProps { verificationFinalFocus: DialogFocusTarget; setPrimaryReverification: ReverificationController; setPrimaryFinalFocus: DialogFocusTarget; + pendingPrimaryId?: string; + pendingVerifyId?: string; onVerify: (id: string) => void; onSetPrimary: (id: string) => void; } 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 da7137a38a7..638fc184e02 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 @@ -22,6 +22,8 @@ export function UserProfilePhoneSectionView({ verificationFinalFocus, setPrimaryReverification, setPrimaryFinalFocus, + pendingPrimaryId, + pendingVerifyId, onVerify, onSetPrimary, }: UserProfilePhoneSectionViewProps) { @@ -32,6 +34,7 @@ export function UserProfilePhoneSectionView({ render={