diff --git a/.changeset/fix-pending-otp-reprepare.md b/.changeset/fix-pending-otp-reprepare.md new file mode 100644 index 00000000000..d7967766ab9 --- /dev/null +++ b/.changeset/fix-pending-otp-reprepare.md @@ -0,0 +1,5 @@ +--- +'@clerk/ui': patch +--- + +Avoid sending duplicate verification codes when persisted email or phone code verifications are already pending. diff --git a/integration/tests/email-code.test.ts b/integration/tests/email-code.test.ts index 7b7cf26e7c7..272c55280c3 100644 --- a/integration/tests/email-code.test.ts +++ b/integration/tests/email-code.test.ts @@ -1,4 +1,4 @@ -import { test } from '@playwright/test'; +import { expect, test } from '@playwright/test'; import type { Application } from '../models/application'; import { appConfigs } from '../presets'; @@ -39,6 +39,40 @@ test.describe('sign up and sign in with email code @generic', () => { await u.po.expect.toBeSignedIn(); }); + test('does not re-send the email code when the verification step is reloaded', async ({ page, context }) => { + const u = createTestUtils({ app, page, context }); + const reloadUser = createTestUtils({ app }).services.users.createFakeUser({ fictionalEmail: true }); + + try { + await u.po.signUp.goTo(); + await u.po.signUp.signUpWithEmailAndPassword({ + email: reloadUser.email, + password: reloadUser.password, + }); + await u.po.signUp.waitForEmailVerificationScreen(); + + // Count any /prepare_verification calls that happen after we land on the OTP screen. + // Before the fix, reloading would re-mount SignUpEmailCodeCard and trigger a duplicate prepare. + let prepareCount = 0; + await page.context().route('**/prepare_verification*', async route => { + prepareCount += 1; + await route.continue(); + }); + + await page.reload(); + await u.po.signUp.waitForEmailVerificationScreen(); + // Give clerk-js time to rehydrate the persisted sign-up and (formerly) call prepare on mount. + await page.waitForTimeout(1000); + + expect(prepareCount).toBe(0); + + await u.po.signUp.enterTestOtpCode({ awaitPrepare: false }); + await u.po.expect.toBeSignedIn(); + } finally { + await reloadUser.deleteIfExists(); + } + }); + // TODO: remove test.skip('sign in has been moved to sign in flow test suite, this will be removed', async () => {}); }); diff --git a/packages/ui/src/components/SignIn/SignInFactorOneCodeForm.tsx b/packages/ui/src/components/SignIn/SignInFactorOneCodeForm.tsx index da2863fd3d9..e057f588c29 100644 --- a/packages/ui/src/components/SignIn/SignInFactorOneCodeForm.tsx +++ b/packages/ui/src/components/SignIn/SignInFactorOneCodeForm.tsx @@ -40,7 +40,16 @@ export const SignInFactorOneCodeForm = (props: SignInFactorOneCodeFormProps) => const supportEmail = useSupportEmail(); const clerk = useClerk(); + const factorChannel = 'channel' in props.factor ? props.factor.channel : undefined; + const normalizedFactorChannel = factorChannel === 'sms' ? undefined : factorChannel; + const normalizedVerificationChannel = + signIn.firstFactorVerification.channel === 'sms' ? undefined : signIn.firstFactorVerification.channel; + const hasPendingFactorVerification = + signIn.firstFactorVerification.status === 'unverified' && + signIn.firstFactorVerification.strategy === props.factor.strategy && + normalizedFactorChannel === normalizedVerificationChannel; const shouldAvoidPrepare = signIn.firstFactorVerification.status === 'verified' && props.factorAlreadyPrepared; + const shouldAvoidInitialPrepare = shouldAvoidPrepare || hasPendingFactorVerification; const cacheKey = useMemo(() => { const factor = props.factor; @@ -82,7 +91,7 @@ export const SignInFactorOneCodeForm = (props: SignInFactorOneCodeFormProps) => .catch(err => handleError(err, [], card.setError)); }; - useFetch(shouldAvoidPrepare ? undefined : () => signIn?.prepareFirstFactor(props.factor), cacheKey, { + useFetch(shouldAvoidInitialPrepare ? undefined : () => signIn?.prepareFirstFactor(props.factor), cacheKey, { staleTime: 100, onSuccess: () => props.onFactorPrepare(), onError: err => handleError(err, [], card.setError), diff --git a/packages/ui/src/components/SignIn/__tests__/SignInFactorOneCodeForm.test.tsx b/packages/ui/src/components/SignIn/__tests__/SignInFactorOneCodeForm.test.tsx index c5f99f1a824..6de4646afe4 100644 --- a/packages/ui/src/components/SignIn/__tests__/SignInFactorOneCodeForm.test.tsx +++ b/packages/ui/src/components/SignIn/__tests__/SignInFactorOneCodeForm.test.tsx @@ -102,11 +102,14 @@ describe('SignInFactorOneCodeForm', () => { ); }); - it('still calls prepare when factor is already prepared but verification not verified', async () => { - const { wrapper } = await createFixtures(f => { + it('skips automatic prepare when the same factor verification is pending', async () => { + const { wrapper, fixtures } = await createFixtures(f => { f.withPhoneNumber(); f.startSignInWithPhoneNumber({ supportPhoneCode: true }); }); + fixtures.signIn.firstFactorVerification.status = 'unverified'; + fixtures.signIn.firstFactorVerification.strategy = 'phone_code'; + fixtures.signIn.firstFactorVerification.channel = 'sms'; const props = { factor: { @@ -114,7 +117,7 @@ describe('SignInFactorOneCodeForm', () => { phoneNumberId: 'idn_123', safeIdentifier: '+1234567890', }, - factorAlreadyPrepared: true, + factorAlreadyPrepared: false, onFactorPrepare: vi.fn(), cardTitle: localizationKeys('signIn.phoneCode.title'), cardSubtitle: localizationKeys('signIn.phoneCode.subtitle'), @@ -124,16 +127,18 @@ describe('SignInFactorOneCodeForm', () => { renderWithProviders(, { wrapper }); - expect(vi.mocked(useFetch)).toHaveBeenCalledWith(expect.any(Function), expect.any(Object), expect.any(Object)); + expect(vi.mocked(useFetch)).toHaveBeenCalledWith(undefined, expect.any(Object), expect.any(Object)); }); }); describe('shouldAvoidPrepare Logic', () => { - it('still calls prepare when factor is already prepared but verification not verified', async () => { - const { wrapper } = await createFixtures(f => { + it('allows prepare when the same factor verification is expired', async () => { + const { wrapper, fixtures } = await createFixtures(f => { f.withPhoneNumber(); f.startSignInWithPhoneNumber({ supportPhoneCode: true }); }); + fixtures.signIn.firstFactorVerification.status = 'expired'; + fixtures.signIn.firstFactorVerification.strategy = 'phone_code'; const propsWithFactorPrepared = { ...defaultProps, @@ -142,11 +147,7 @@ describe('SignInFactorOneCodeForm', () => { renderWithProviders(, { wrapper }); - expect(vi.mocked(useFetch)).toHaveBeenCalledWith( - expect.any(Function), // fetcher should still be a function because shouldAvoidPrepare requires BOTH conditions - expect.any(Object), - expect.any(Object), - ); + expect(vi.mocked(useFetch)).toHaveBeenCalledWith(expect.any(Function), expect.any(Object), expect.any(Object)); }); it('allows prepare when factor is not already prepared', async () => { diff --git a/packages/ui/src/components/SignUp/SignUpEmailCodeCard.tsx b/packages/ui/src/components/SignUp/SignUpEmailCodeCard.tsx index 65f6a2e1994..81af3377566 100644 --- a/packages/ui/src/components/SignUp/SignUpEmailCodeCard.tsx +++ b/packages/ui/src/components/SignUp/SignUpEmailCodeCard.tsx @@ -11,7 +11,10 @@ export const SignUpEmailCodeCard = () => { const card = useCardState(); const emailVerificationStatus = signUp.verifications.emailAddress.status; + const hasPendingEmailCodeVerification = + emailVerificationStatus === 'unverified' && signUp.verifications.emailAddress.strategy === 'email_code'; const shouldAvoidPrepare = !signUp.status || emailVerificationStatus === 'verified'; + const shouldAvoidInitialPrepare = shouldAvoidPrepare || hasPendingEmailCodeVerification; const prepare = () => { if (shouldAvoidPrepare) { @@ -24,7 +27,7 @@ export const SignUpEmailCodeCard = () => { // TODO: Introduce a useMutation to handle mutating requests useFetch( - shouldAvoidPrepare ? undefined : () => signUp.prepareEmailAddressVerification({ strategy: 'email_code' }), + shouldAvoidInitialPrepare ? undefined : () => signUp.prepareEmailAddressVerification({ strategy: 'email_code' }), { name: 'prepare', strategy: 'email_code', diff --git a/packages/ui/src/components/SignUp/SignUpPhoneCodeCard.tsx b/packages/ui/src/components/SignUp/SignUpPhoneCodeCard.tsx index cd9b906ce93..5e618fa0a63 100644 --- a/packages/ui/src/components/SignUp/SignUpPhoneCodeCard.tsx +++ b/packages/ui/src/components/SignUp/SignUpPhoneCodeCard.tsx @@ -15,6 +15,8 @@ export const SignUpPhoneCodeCard = withCardStateProvider(() => { const channel = signUp.verifications.phoneNumber.channel; const phoneVerificationStatus = signUp.verifications.phoneNumber.status; + const hasPendingPhoneCodeVerification = + phoneVerificationStatus === 'unverified' && signUp.verifications.phoneNumber.strategy === 'phone_code'; const shouldAvoidPrepare = !signUp.status || phoneVerificationStatus === 'verified'; const isAlternativePhoneCodeProvider = !!channel && channel !== 'sms'; @@ -34,7 +36,7 @@ export const SignUpPhoneCodeCard = withCardStateProvider(() => { useFetch( // If an alternative phone code provider is used, we skip the prepare step // because the verification is already created on the Start screen - shouldAvoidPrepare || isAlternativePhoneCodeProvider + shouldAvoidPrepare || hasPendingPhoneCodeVerification || isAlternativePhoneCodeProvider ? undefined : () => signUp.preparePhoneNumberVerification({ strategy: 'phone_code', channel: undefined }), { diff --git a/packages/ui/src/components/SignUp/__tests__/SignUpVerifyEmail.test.tsx b/packages/ui/src/components/SignUp/__tests__/SignUpVerifyEmail.test.tsx index 11e45639e3a..1ea27d0eb77 100644 --- a/packages/ui/src/components/SignUp/__tests__/SignUpVerifyEmail.test.tsx +++ b/packages/ui/src/components/SignUp/__tests__/SignUpVerifyEmail.test.tsx @@ -57,6 +57,42 @@ describe('SignUpVerifyEmail', () => { ); }); + it('does not prepare a new email code when the persisted email verification is pending', async () => { + const { wrapper, fixtures } = await createFixtures(f => { + f.withEmailAddress({ required: true, verifications: ['email_code'] }); + f.startSignUpWithEmailAddress({ + emailAddress: 'pending-code@clerk.com', + supportEmailLink: false, + supportEmailCode: true, + }); + }); + fixtures.signUp.prepareEmailAddressVerification.mockResolvedValue(fixtures.signUp); + + const { findByText } = render(, { wrapper }); + await waitFor(async () => expect(await findByText(/Verify your email/i)).toBeInTheDocument()); + + expect(fixtures.signUp.prepareEmailAddressVerification).not.toHaveBeenCalled(); + }); + + it('prepares a new email code when the persisted email verification is expired', async () => { + const { wrapper, fixtures } = await createFixtures(f => { + f.withEmailAddress({ required: true, verifications: ['email_code'] }); + f.startSignUpWithEmailAddress({ + emailAddress: 'expired-code@clerk.com', + supportEmailLink: false, + supportEmailCode: true, + emailVerificationStatus: 'expired', + }); + }); + fixtures.signUp.prepareEmailAddressVerification.mockRejectedValue(null); + + render(, { wrapper }); + + await waitFor(() => + expect(fixtures.signUp.prepareEmailAddressVerification).toHaveBeenCalledWith({ strategy: 'email_code' }), + ); + }); + it('clicking on the edit icon navigates to the previous route', async () => { const { wrapper, fixtures } = await createFixtures(f => { f.withEmailAddress({ required: true });