feat(ui): add "Sign in as a different account" action to sign-in second factor#9190
feat(ui): add "Sign in as a different account" action to sign-in second factor#9190alexcarpenter wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 35f1b84 The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe sign-in second-factor screen now displays a localized “Sign in as a different account” action. Activating it navigates back to the sign-in start, with integration coverage for the SMS MFA flow. ChangesDifferent Account Sign-In
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant VerificationCodeCard
participant SignInFactorTwoCodeForm
participant Router
User->>VerificationCodeCard: Click different account action
VerificationCodeCard->>SignInFactorTwoCodeForm: Invoke callback
SignInFactorTwoCodeForm->>Router: navigate('../')
Router-->>User: Show sign-in start
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
ApprovabilityVerdict: Approved This PR adds a simple navigation link to the 2FA screen allowing users to go back and sign in with a different account. The implementation is minimal ( You can customize Macroscope's approvability policy. Learn more. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/src/components/SignUp/__tests__/SignUpVerifyEmail.test.tsx`:
- Around line 112-126: Restore the existing edit-icon test for SignUpVerifyEmail
while retaining the new back-button navigation test. Ensure both tests verify
their respective controls invoke the expected navigation behavior, using the
existing SignUpVerifyEmail fixture and render setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 05844200-3625-445a-b6d9-54a11600e470
📒 Files selected for processing (5)
.changeset/two-step-back-button.mdpackages/ui/src/components/SignIn/SignInFactorTwoCodeForm.tsxpackages/ui/src/components/SignIn/__tests__/SignInFactorTwo.test.tsxpackages/ui/src/components/SignUp/SignUpVerificationCodeForm.tsxpackages/ui/src/components/SignUp/__tests__/SignUpVerifyEmail.test.tsx
| it('clicking on the back button navigates to the previous route', async () => { | ||
| const { wrapper, fixtures } = await createFixtures(f => { | ||
| f.withEmailAddress({ required: true }); | ||
| f.startSignUpWithEmailAddress({ emailAddress: 'test@clerk.com' }); | ||
| }); | ||
| fixtures.signUp.prepareEmailAddressVerification.mockRejectedValue(null); | ||
|
|
||
| const { userEvent } = render(<SignUpVerifyEmail />, { wrapper }); | ||
| await userEvent.click( | ||
| screen.getByRole('button', { | ||
| name: /back/i, | ||
| }), | ||
| ); | ||
| expect(fixtures.router.navigate).toHaveBeenCalledWith('../', { searchParams: new URLSearchParams() }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Preserve the existing edit-button test.
This change replaces coverage for the edit icon even though onIdentityPreviewEditClicked={goBack} remains part of the component behavior. Restore the edit-icon test and keep this new back-button test so both controls remain covered.
As per coding guidelines, unit tests are required for all new functionality and should verify component behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/components/SignUp/__tests__/SignUpVerifyEmail.test.tsx`
around lines 112 - 126, Restore the existing edit-icon test for
SignUpVerifyEmail while retaining the new back-button navigation test. Ensure
both tests verify their respective controls invoke the expected navigation
behavior, using the existing SignUpVerifyEmail fixture and render setup.
Source: Coding guidelines
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
API Changes Report
Summary
@clerk/sharedCurrent version: 4.25.5 Subpath
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration/tests/session-tasks-setup-mfa.test.ts`:
- Around line 211-254: Wrap the setup and sign-in flow after createBapiUser in a
try/finally block, keeping user.deleteIfExists() in the finally clause so
cleanup runs on both success and failure. Apply this to the test flow using
createTestUtils and createBapiUser, matching the cleanup pattern used by the
email-code test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: c32bf548-d19a-4c21-b3dd-c19e90223e9c
📒 Files selected for processing (4)
.changeset/testing-back-link-po.mdintegration/tests/email-code.test.tsintegration/tests/session-tasks-setup-mfa.test.tspackages/testing/src/playwright/unstable/page-objects/common.ts
| const u = createTestUtils({ app, page, context }); | ||
| const user = u.services.users.createFakeUser({ | ||
| fictionalEmail: true, | ||
| withPhoneNumber: true, | ||
| withPassword: true, | ||
| }); | ||
| await u.services.users.createBapiUser(user); | ||
|
|
||
| // Enroll SMS as a second factor using the user's existing phone number. | ||
| await u.po.signIn.goTo(); | ||
| await u.po.signIn.waitForMounted(); | ||
| await u.po.signIn.signInWithEmailAndInstantPassword({ email: user.email, password: user.password }); | ||
| await u.po.expect.toBeSignedIn(); | ||
|
|
||
| await u.page.goToRelative('/page-protected'); | ||
| await u.page.getByText(/set up two-step verification/i).waitFor({ state: 'visible' }); | ||
| await u.page.getByRole('button', { name: /sms code/i }).click(); | ||
| const formattedPhoneNumber = stringPhoneNumber(user.phoneNumber); | ||
| await u.page.getByRole('button', { name: formattedPhoneNumber }).click(); | ||
| await u.page.getByText(/save these backup codes/i).waitFor({ state: 'visible', timeout: 10000 }); | ||
| await u.po.signIn.continue(); | ||
| await u.page.waitForAppUrl('/page-protected'); | ||
| await u.po.expect.toBeSignedIn(); | ||
|
|
||
| // Sign out and back in so the sign-in flow now requires the second factor. | ||
| await u.page.signOut(); | ||
| await u.page.context().clearCookies(); | ||
|
|
||
| await u.po.signIn.goTo(); | ||
| await u.po.signIn.waitForMounted(); | ||
| await u.po.signIn.getIdentifierInput().fill(user.email); | ||
| await u.po.signIn.setInstantPassword(user.password); | ||
| await u.po.signIn.continue(); | ||
|
|
||
| // We are now on the two-step verification (second factor) step. | ||
| await u.page.getByText(/check your phone/i).waitFor({ state: 'visible' }); | ||
|
|
||
| await u.po.signIn.getBackLink().click(); | ||
|
|
||
| // Back returns to the identifier step. | ||
| await expect(u.po.signIn.getIdentifierInput()).toBeVisible(); | ||
|
|
||
| await user.deleteIfExists(); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guarantee fake-user cleanup on test failures.
A failure after createBapiUser() skips line 253 and leaves the BAPI user behind. Wrap the flow in try/finally, as the email-code test does.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@integration/tests/session-tasks-setup-mfa.test.ts` around lines 211 - 254,
Wrap the setup and sign-in flow after createBapiUser in a try/finally block,
keeping user.deleteIfExists() in the finally clause so cleanup runs on both
success and failure. Apply this to the test flow using createTestUtils and
createBapiUser, matching the cleanup pattern used by the email-code test.
Dismissing prior approval to re-evaluate 38b7883
38b7883 to
2c84e49
Compare
Dismissing prior approval to re-evaluate 2c84e49
alexcarpenter
changed the title
…nd factor Adds a footer action on the sign-in two-step verification (second factor) step that abandons the current sign-in attempt and returns to the sign-in start, so a user who reached 2FA with the wrong account (e.g. wrong social account) can sign in again instead of being stuck.
2c84e49 to
b90ea15
Compare
Dismissing prior approval to re-evaluate b90ea15
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/ui/src/elements/VerificationCodeCard.tsx (1)
45-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the conditional footer action.
Cover the absent-callback case and verify that the localized
signIn.differentAccountActionlink invokes the supplied callback. This protects the new user-facing rendering and wiring behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/elements/VerificationCodeCard.tsx` around lines 45 - 54, Update the tests for VerificationCodeCard to cover both footer states: assert that no action renders when onDifferentAccountClicked is absent, and assert that the localized signIn.differentAccountAction link renders and invokes the supplied callback when provided.packages/ui/src/components/SignIn/SignInFactorTwoCodeForm.tsx (1)
49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit return type to the new handler.
signInAsDifferentUsercurrently relies on inference. Annotate it explicitly, for example:Proposed fix
- const signInAsDifferentUser = () => navigate('../'); + const signInAsDifferentUser = (): void => { + void navigate('../'); + };As per coding guidelines, “Always define explicit return types for functions, especially public APIs.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/SignIn/SignInFactorTwoCodeForm.tsx` at line 49, Update the signInAsDifferentUser handler to declare an explicit void return type while preserving its existing navigate('../') behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/ui/src/components/SignIn/SignInFactorTwoCodeForm.tsx`:
- Line 49: Update the signInAsDifferentUser handler to declare an explicit void
return type while preserving its existing navigate('../') behavior.
In `@packages/ui/src/elements/VerificationCodeCard.tsx`:
- Around line 45-54: Update the tests for VerificationCodeCard to cover both
footer states: assert that no action renders when onDifferentAccountClicked is
absent, and assert that the localized signIn.differentAccountAction link renders
and invokes the supplied callback when provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 59b9554d-4d4d-4ee9-841b-867dca57b6eb
📒 Files selected for processing (6)
.changeset/sign-in-different-account.mdpackages/clerk-js/sandbox/template.htmlpackages/localizations/src/en-US.tspackages/shared/src/types/localization.tspackages/ui/src/components/SignIn/SignInFactorTwoCodeForm.tsxpackages/ui/src/elements/VerificationCodeCard.tsx
Dismissing prior approval to re-evaluate 35f1b84
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@integration/tests/session-tasks-setup-mfa.test.ts`:
- Around line 207-255: Extend the test around “Sign in as a different account”
to cover email-code and TOTP second-factor flows in addition to SMS, preferably
by parameterizing the factor setup and verification-step assertions. Preserve
the existing account-switching assertions while using each factor’s appropriate
enrollment and challenge UI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f5e5a85-5d0f-4044-a0e6-9928d186f636
📒 Files selected for processing (6)
.changeset/sign-in-different-account.mdintegration/tests/session-tasks-setup-mfa.test.tspackages/localizations/src/en-US.tspackages/shared/src/types/localization.tspackages/ui/src/components/SignIn/SignInFactorTwoCodeForm.tsxpackages/ui/src/elements/VerificationCodeCard.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/ui/src/components/SignIn/SignInFactorTwoCodeForm.tsx
- packages/shared/src/types/localization.ts
- packages/localizations/src/en-US.ts
- packages/ui/src/elements/VerificationCodeCard.tsx
- .changeset/sign-in-different-account.md
| test('can sign in as a different account from the two-step verification step', async ({ page, context }) => { | ||
| const u = createTestUtils({ app, page, context }); | ||
| const user = u.services.users.createFakeUser({ | ||
| fictionalEmail: true, | ||
| withPhoneNumber: true, | ||
| withPassword: true, | ||
| }); | ||
| await u.services.users.createBapiUser(user); | ||
|
|
||
| // Enroll SMS as a second factor using the user's existing phone number. | ||
| await u.po.signIn.goTo(); | ||
| await u.po.signIn.waitForMounted(); | ||
| await u.po.signIn.signInWithEmailAndInstantPassword({ email: user.email, password: user.password }); | ||
| await u.po.expect.toBeSignedIn(); | ||
|
|
||
| await u.page.goToRelative('/page-protected'); | ||
| await u.page.getByText(/set up two-step verification/i).waitFor({ state: 'visible' }); | ||
| await u.page.getByRole('button', { name: /sms code/i }).click(); | ||
| const formattedPhoneNumber = stringPhoneNumber(user.phoneNumber); | ||
| await u.page.getByRole('button', { name: formattedPhoneNumber }).click(); | ||
| await u.page.getByText(/save these backup codes/i).waitFor({ state: 'visible', timeout: 10000 }); | ||
| await u.po.signIn.continue(); | ||
| await u.page.waitForAppUrl('/page-protected'); | ||
| await u.po.expect.toBeSignedIn(); | ||
|
|
||
| // Sign out and back in so the sign-in flow now requires the second factor. | ||
| await u.page.signOut(); | ||
| await u.page.context().clearCookies(); | ||
|
|
||
| await u.po.signIn.goTo(); | ||
| await u.po.signIn.waitForMounted(); | ||
| await u.po.signIn.getIdentifierInput().fill(user.email); | ||
| await u.po.signIn.setInstantPassword(user.password); | ||
| await u.po.signIn.continue(); | ||
|
|
||
| // We are now on the two-step verification (SMS second factor) step, with no way to | ||
| // complete it if this is the wrong account. | ||
| await u.page.getByText(/check your phone/i).waitFor({ state: 'visible' }); | ||
|
|
||
| // The "Sign in as a different account" action abandons the attempt and returns to the start. | ||
| const differentAccount = u.page.getByRole('link', { name: /sign in as a different account/i }); | ||
| await expect(differentAccount).toBeVisible(); | ||
| await differentAccount.click(); | ||
|
|
||
| // Back on the sign-in start, where a different account can be used. | ||
| await expect(u.po.signIn.getIdentifierInput()).toBeVisible(); | ||
|
|
||
| await user.deleteIfExists(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Cover the email-code and TOTP variants.
This only exercises SMS, while the feature is introduced for TOTP, SMS, and email-code second-factor screens. Add equivalent coverage, or parameterize this scenario, for the remaining factor types. As per coding guidelines, “Include tests for all new features.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@integration/tests/session-tasks-setup-mfa.test.ts` around lines 207 - 255,
Extend the test around “Sign in as a different account” to cover email-code and
TOTP second-factor flows in addition to SMS, preferably by parameterizing the
factor setup and verification-step assertions. Preserve the existing
account-switching assertions while using each factor’s appropriate enrollment
and challenge UI.
Source: Coding guidelines
1 participant
Description
Customer feedback:
A user who authenticates on the sign-in two-step verification (second factor) screen with the wrong account — most commonly after signing in with the wrong social/OAuth account — is stuck. The only actions on that screen are Continue (needs a code they can't produce) and Use another method (which just cycles between other 2FA methods, keeping them inside the same locked attempt). Unlike the first-factor screen, there is no way to change who you're signing in as.
This adds a "Sign in as a different account" footer action to the second-factor step. It abandons the current sign-in attempt and returns the user to the sign-in start, where they can re-authenticate with the correct account.
Why not reuse the first-factor "edit identifier" pencil?
The pencil lives on the
IdentityPreviewand means "edit the identifier I typed". On the second factor there is no account identifier displayed (and for TOTP there is no identifier at all), so the pencil has nothing valid to attach to and the wrong semantics. A distinct footer action ("throw away this attempt and start over") is the correct affordance.Scope
SignInFactorTwoCodeForm, so it appears on the TOTP, SMS, and email-code second-factor screens (covers the reported TOTP-after-OAuth case).VerificationCodeCardfooter, gated behind a new opt-inonDifferentAccountClickedprop so it does not appear on the first factor.signIn.differentAccountAction("Sign in as a different account").Testing
integration/tests/session-tasks-setup-mfa.test.ts): enrolls SMS 2FA, signs out, signs back in to reach the second-factor step, clicks "Sign in as a different account", and asserts the user lands back on the sign-in identifier step.Note
Getting the user out of Clerk's stuck state is handled here. Fully solving the OAuth "wrong account" case may also want
prompt=select_accounton the social connection so Google re-shows its account chooser on the retry — that is a separate connection/server-side concern and is not part of this change.Checklist
Summary by CodeRabbit
New Features
Localization
Tests