Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-pending-otp-reprepare.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@clerk/ui': patch
---

Avoid sending duplicate verification codes when persisted email or phone code verifications are already pending.
36 changes: 35 additions & 1 deletion integration/tests/email-code.test.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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 () => {});
});
Expand Down
11 changes: 10 additions & 1 deletion packages/ui/src/components/SignIn/SignInFactorOneCodeForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,19 +102,22 @@ 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: {
strategy: 'phone_code' as const,
phoneNumberId: 'idn_123',
safeIdentifier: '+1234567890',
},
factorAlreadyPrepared: true,
factorAlreadyPrepared: false,
onFactorPrepare: vi.fn(),
cardTitle: localizationKeys('signIn.phoneCode.title'),
cardSubtitle: localizationKeys('signIn.phoneCode.subtitle'),
Expand All @@ -124,16 +127,18 @@ describe('SignInFactorOneCodeForm', () => {

renderWithProviders(<SignInFactorOneCodeForm {...props} />, { 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,
Expand All @@ -142,11 +147,7 @@ describe('SignInFactorOneCodeForm', () => {

renderWithProviders(<SignInFactorOneCodeForm {...propsWithFactorPrepared} />, { 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 () => {
Expand Down
5 changes: 4 additions & 1 deletion packages/ui/src/components/SignUp/SignUpEmailCodeCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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',
Expand Down
4 changes: 3 additions & 1 deletion packages/ui/src/components/SignUp/SignUpPhoneCodeCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -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 }),
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(<SignUpVerifyEmail />, { 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(<SignUpVerifyEmail />, { 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 });
Expand Down
Loading