diff --git a/.changeset/email-link-prepared-signal.md b/.changeset/email-link-prepared-signal.md new file mode 100644 index 00000000000..a845151cc84 --- /dev/null +++ b/.changeset/email-link-prepared-signal.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.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 f17f65de695..d6e62082d40 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 @@ -9,7 +9,7 @@ import { useUserProfileAddEmailController } from '../user-profile-add-email.cont const idleReverification = { status: 'idle', visible: false } satisfies ReverificationController; -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 }); @@ -19,7 +19,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()); @@ -125,7 +129,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)); } @@ -138,7 +142,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(''); @@ -185,7 +189,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)); @@ -199,11 +203,11 @@ describe('useUserProfileAddEmailController', () => { useUserProfileAddEmailController({ reverification: idleReverification, 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)); @@ -218,11 +222,11 @@ describe('useUserProfileAddEmailController', () => { ); 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'); @@ -234,23 +238,117 @@ 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')), - })); + email.prepare.mockRejectedValueOnce(saveError('Too many requests')); + const onCreate = vi.fn(async () => email); const { result } = renderHook(() => - useUserProfileAddEmailController({ reverification: idleReverification, onCreate: created }), + useUserProfileAddEmailController({ reverification: idleReverification, onCreate }), ); - - act(() => result.current.onVerifyEmail(other, email)); + 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({ reverification: idleReverification, 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({ reverification: idleReverification })); + 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({ reverification: idleReverification })); + 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({ reverification: idleReverification })); + 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 () => { @@ -270,7 +368,7 @@ describe('useUserProfileAddEmailController', () => { const verified = createDeferredPromise(); const { result } = renderHook(() => useUserProfileAddEmailController({ reverification: idleReverification })); - 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); @@ -288,12 +386,12 @@ describe('useUserProfileAddEmailController', () => { useUserProfileAddEmailController({ reverification: idleReverification }), ); - 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); @@ -303,7 +401,7 @@ describe('useUserProfileAddEmailController', () => { const email = verifier(sentLink(Promise.reject(saveError('Link expired')))); const { result } = renderHook(() => useUserProfileAddEmailController({ reverification: idleReverification })); - 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); @@ -314,7 +412,7 @@ describe('useUserProfileAddEmailController', () => { const email = verifier(() => ({ method: 'sso', connect })); const { result } = renderHook(() => useUserProfileAddEmailController({ reverification: idleReverification })); - 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 edf4f8d4a05..d4c39e6c991 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 @@ -213,6 +213,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 610b4c8b0d4..15b138fe34b 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 5086ae72501..c0ec6c2ba9f 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 @@ -22,7 +22,9 @@ import { } from '../../../../__tests__/feature/fapi'; import { openMenu } from '../../../../__tests__/feature/menu'; import { renderWithClerk } from '../../../../__tests__/feature/render'; +import type { UserProfileEmailVerifier } from '../../user-profile-contact.types'; import { UserProfileEmailSection } from '../user-profile-email-section'; +import { useUserProfileEmailSectionModel } from '../user-profile-email-section.model'; const VERIFIED = fapiVerification('email_code', { status: 'verified' }); const PRIMARY = fapiEmailAddress({ id: 'idn_primary', email_address: 'alice@example.com', verification: VERIFIED }); @@ -68,6 +70,27 @@ async function enterCode(actor: Actor, code: string) { } describe('the user profile email section', () => { + it('refuses a retained verifier after the account changes', async () => { + const pending = fapiEmailAddress({ id: 'idn_pending', email_address: 'pending@example.com' }); + const alice = fapiUser({ id: 'user_1', email_addresses: [pending] }); + const bob = fapiUser({ id: 'user_2', email_addresses: [fapiEmailAddress({ id: 'idn_other' })] }); + serveFapi({ + client: fapiClient([fapiSession({ id: 'sess_1', user: alice }), fapiSession({ id: 'sess_2', user: bob })]), + }); + let retained: UserProfileEmailVerifier | undefined; + function CaptureVerifier() { + const model = useUserProfileEmailSectionModel(); + if (model.status === 'ready' && !retained) { + retained = model.getEmailVerifier('idn_pending'); + } + return null; + } + const { clerk } = await renderWithClerk(); + expect(retained).toBeDefined(); + await act(() => clerk.setActive({ session: 'sess_2' })); + 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 () => { const { actor } = await renderSection(signedInWithEmails([])); expect(emailRow()).toHaveTextContent('No email addresses added'); @@ -82,22 +105,149 @@ 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'); - - prepare.release(); - await enterCode(actor, VERIFICATION_CODE); + 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 add = screen.getByRole('button', { name: 'Add email' }); + expect(add).toBeDisabled(); + await actor.click(add); + expect(screen.queryByRole('menu')).toBeNull(); + expect(within(emailRow()).getByText('pending@example.com')).toBeVisible(); + 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(); + } + }, + ); + + 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 () => { @@ -325,21 +475,74 @@ describe('email reverification', () => { ); }); - it('waits for a verification factor before opening the dialog', async () => { + it.each(['pointer', 'keyboard'])( + 'keeps the profile visible after the %s primary action 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)); + expect(screen.queryByRole('menu')).toBeNull(); + expect(within(emailRow()).getByText('bob@example.com')).toBeVisible(); + expect(screen.queryByRole('alertdialog')).toBeNull(); + expect(screen.queryByRole('dialog')).toBeNull(); + expect(mutation.requests).toHaveLength(1); + + mutation.release(); + await waitFor(() => expect(start.requests).toHaveLength(1)); + expect(within(emailRow()).getByText('bob@example.com')).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(); + } + }, + ); + + it('shows the section error when the primary action fails after closing the menu', 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(); - - start.release(); - expect(await screen.findByText('Enter your current password to continue')).toBeVisible(); + 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.queryByRole('menu')).toBeNull(); + expect(within(emailRow()).getByText('bob@example.com')).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 5b48368eb39..bf75e67392b 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'; @@ -29,7 +29,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 { @@ -43,7 +43,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' } @@ -62,19 +68,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, })), }; @@ -102,36 +113,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 }, @@ -139,10 +151,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' }, }, @@ -172,13 +187,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 => values.emailAddress !== '' && 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 }); + } } }, }); @@ -214,7 +252,7 @@ export function useUserProfileAddEmailController({ return { handle, resendSeconds, - isResending: state === 'sending', + isResending: state === 'resending', open, step, reverification, @@ -231,11 +269,17 @@ export function useUserProfileAddEmailController({ } reverification.onCancel(); } + if (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' }); } }, @@ -260,9 +304,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 a54f6f89993..1bc33db1eba 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 @@ -97,6 +97,7 @@ export function UserProfileAddEmailDialog(props: UserProfileAddEmailDialogProps) @@ -296,6 +297,7 @@ function VerifyEmailStep(props: VerifyEmailStepProps) { } interface VerifyLinkStepProps { + isResending?: boolean; emailAddress: string; onResend: () => void; errorMessage?: string; @@ -322,6 +324,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 ac0bcf3bd32..b4171e14b71 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 @@ -23,6 +23,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 822bdeca9b9..74930511c61 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 @@ -24,6 +24,7 @@ export function useUserProfileEmailSectionController({ reverification: createReverification, }); const setPrimary = usePendingAction(); + const verify = usePendingAction(); const rowRef = useRef(null); const removalFocus = useListRemovalFocus({ ids: emails.map(email => email.id), @@ -39,7 +40,8 @@ export function useUserProfileEmailSectionController({ emails, canAdd: Boolean(onCreateEmail), verification, - error: setPrimary.error, + error: verify.error ?? setPrimary.error, + pendingVerifyId: verify.pendingKey, rowRef, registerTrigger: removalFocus.registerTrigger, remove: onRemoveEmail ? remove : undefined, @@ -50,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 d72a5ef20b6..e0bdbced9ad 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,12 +29,13 @@ function verifyRedirectUrl(userProfileUrl: string): string { return buildURL({ base: userProfileUrl, hashPath: '/verify' }, { stringify: true }); } -function startEmailVerification( - email: EmailAddressResource, +async function prepareEmailVerification( + currentEmail: () => EmailAddressResource, linkRedirectUrl: string | undefined, router: MosaicRouter, -): UserProfileEmailVerification { - if (email.matchesSsoConnection) { + setCancel: (cancel: () => void) => void, +): Promise { + if (currentEmail().matchesSsoConnection) { return { method: 'sso', /* @@ -48,7 +50,7 @@ function startEmailVerification( */ connect: () => save(async () => { - const prepared = await email.prepareVerification({ + const prepared = await currentEmail().prepareVerification({ strategy: 'enterprise_sso', redirectUrl: window.location.href, }); @@ -61,24 +63,45 @@ function startEmailVerification( }; } if (linkRedirectUrl === undefined) { - return { method: 'code', sent: save(() => email.prepareVerification({ strategy: 'email_code' })) }; + await save(() => currentEmail().prepareVerification({ strategy: 'email_code' })); + currentEmail(); + return { method: 'code' }; } - const { startEmailLinkFlow, cancelEmailLinkFlow } = email.createEmailLinkFlow(); - return { - method: 'link', - verified: save(() => startEmailLinkFlow({ redirectUrl: linkRedirectUrl })), - cancel: cancelEmailLinkFlow, - }; + const { startEmailLinkFlow, cancelEmailLinkFlow } = currentEmail().createEmailLinkFlow(); + const prepared = createDeferredPromise(); + setCancel(cancelEmailLinkFlow); + const verified = save(() => + startEmailLinkFlow({ + redirectUrl: linkRedirectUrl, + onPrepared: () => { + currentEmail(); + prepared.resolve(); + }, + }), + ); + void verified.catch((error: unknown) => { + cancelEmailLinkFlow(); + prepared.reject(error); + }); + await prepared.promise; + return { method: 'link', verified, cancel: cancelEmailLinkFlow }; } function toEmailVerifier( - email: EmailAddressResource, + currentEmail: () => EmailAddressResource, linkRedirectUrl: string | undefined, router: MosaicRouter, ): UserProfileEmailVerifier { + let cancel = () => {}; return { - start: () => startEmailVerification(email, linkRedirectUrl, router), - verifyCode: code => save(() => email.attemptVerification({ code }), ADD_EMAIL_FIELDS), + prepare: () => { + cancel(); + return prepareEmailVerification(currentEmail, linkRedirectUrl, router, next => { + cancel = next; + }); + }, + cancel: () => cancel(), + verifyCode: code => save(() => currentEmail().attemptVerification({ code }), ADD_EMAIL_FIELDS), }; } @@ -113,7 +136,8 @@ export function useUserProfileEmailSectionModel(): UserProfileEmailSectionModel const verifiesByLink = Boolean(attributes.email_address?.verifications.includes('email_link')); const linkRedirectUrl = verifiesByLink ? verifyRedirectUrl(environment.displayConfig.userProfileUrl) : undefined; - const verifierFor = (email: EmailAddressResource) => toEmailVerifier(email, linkRedirectUrl, router); + const verifierFor = (email: EmailAddressResource) => + toEmailVerifier(() => byId(currentUser().emailAddresses, email.id, 'email address'), linkRedirectUrl, router); return { status: 'ready', diff --git a/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-email-section.types.ts b/packages/mosaic/src/features/user-profile/user-profile-email-section/user-profile-email-section.types.ts index 1b4985b0644..eafa33577aa 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 @@ -18,6 +18,7 @@ export interface UserProfileEmailSectionViewProps { verificationFinalFocus: DialogFocusTarget; setPrimaryReverification: ReverificationController; setPrimaryFinalFocus: DialogFocusTarget; + 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 11c03774c45..e25f7de94c9 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 @@ -22,6 +22,7 @@ export function UserProfileEmailSectionView({ verificationFinalFocus, setPrimaryReverification, setPrimaryFinalFocus, + pendingVerifyId, onVerify, onSetPrimary, }: UserProfileEmailSectionViewProps) { @@ -32,6 +33,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 9cc88879a47..9a28ea458c6 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 1fd442dabb0..093d7dc4a65 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 @@ -498,6 +498,8 @@ describe('phone reverification', () => { await manage(actor, WORK_LABEL, 'Set as primary'); await waitFor(() => expect(start.requests).toHaveLength(1)); + expect(screen.queryByRole('menu')).toBeNull(); + expect(within(row()).getByText(WORK_LABEL)).toBeVisible(); expect(screen.queryByRole('dialog')).toBeNull(); start.release(); @@ -523,3 +525,67 @@ 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 the profile visible until phone verification is ready and focuses the 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)); + await waitFor(() => expect(screen.queryByRole('menu')).toBeNull()); + expect(within(row()).getByText(WORK_LABEL)).toBeVisible(); + 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 7e6bf1d0a0b..e34141d1bb4 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'; @@ -23,7 +23,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 { @@ -36,7 +36,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' } @@ -52,14 +52,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, })), }; @@ -80,11 +80,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()), { @@ -114,12 +114,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 }); + } } }, }); @@ -167,7 +187,9 @@ export function useUserProfileAddPhoneController({ } reverification.onCancel(); } + generation.current++; if (open) { + created.current = undefined; phoneForm.reset(); codeForm.reset(); send({ type: 'ADD' }); @@ -195,9 +217,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 dac98862d02..6a2991bc432 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 @@ -31,6 +31,7 @@ export interface UserProfileAddPhoneDialogProps { export function UserProfileAddPhoneDialog(props: UserProfileAddPhoneDialogProps) { const phoneRef = useRef(null); + const codeRef = useRef(null); return ( (null); const removalFocus = useListRemovalFocus({ ids: phones.map(phone => phone.id), @@ -40,7 +41,8 @@ export function useUserProfilePhoneSectionController({ defaultPhoneCountry, canAdd: Boolean(onCreatePhone), verification, - error: setPrimary.error, + error: verify.error ?? setPrimary.error, + pendingVerifyId: verify.pendingKey, rowRef, registerTrigger: removalFocus.registerTrigger, remove: onRemovePhone ? remove : undefined, @@ -51,7 +53,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 aec4af43bfb..98f39011b09 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), }; } @@ -56,10 +60,12 @@ export function useUserProfilePhoneSectionModel(): UserProfilePhoneSectionModel ? async phoneNumber => { const request = createPhone(currentUser, 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 b9a659e3f4c..e769ed57c8f 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,7 @@ export interface UserProfilePhoneSectionViewProps { verificationFinalFocus: DialogFocusTarget; setPrimaryReverification: ReverificationController; setPrimaryFinalFocus: DialogFocusTarget; + 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 cd13d1f4408..26d6269e575 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 @@ -23,6 +23,7 @@ export function UserProfilePhoneSectionView({ verificationFinalFocus, setPrimaryReverification, setPrimaryFinalFocus, + pendingVerifyId, onVerify, onSetPrimary, }: UserProfilePhoneSectionViewProps) { @@ -33,6 +34,7 @@ export function UserProfilePhoneSectionView({ render={