-
Notifications
You must be signed in to change notification settings - Fork 465
fix(ui): Stop the sign-in start card flashing after a code is accepted #9331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@clerk/ui': patch | ||
| --- | ||
|
|
||
| Fix the sign-in start card briefly flashing over `<SignIn />` after a verification code is accepted, before the app renders its signed-in state. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| import { ClerkAPIResponseError } from '@clerk/shared/error'; | ||
| import type { SignInResource } from '@clerk/shared/types'; | ||
| import { waitFor } from '@testing-library/react'; | ||
| import { describe, expect, it } from 'vitest'; | ||
|
|
||
| import { bindCreateFixtures } from '@/test/create-fixtures'; | ||
| import { render, screen } from '@/test/utils'; | ||
|
|
||
| import { SignInFactorOne } from '../SignInFactorOne'; | ||
|
|
||
| const { createFixtures } = bindCreateFixtures('SignIn'); | ||
|
|
||
| /** | ||
| * Mirrors the real `setActive` lifecycle: the flag goes up, the completed sign-in is consumed on | ||
| * the client (`status` -> `null`), the card re-renders while the flag is still up (clerk-js emits | ||
| * transitive state right before navigating), then the flag drops once navigation is done. | ||
| */ | ||
| const mockSetActiveLifecycle = (fixtures: any) => { | ||
| let release = () => {}; | ||
| const gate = new Promise<void>(resolve => (release = resolve)); | ||
|
|
||
| fixtures.clerk.setActive.mockImplementation(async (params: any) => { | ||
| fixtures.clerk.__internal_setActiveInProgress = true; | ||
| fixtures.signIn.status = null; | ||
| await gate; | ||
| await params.navigate?.({ session: { currentTask: null }, decorateUrl: (url: string) => url }); | ||
| fixtures.clerk.__internal_setActiveInProgress = false; | ||
|
Comment on lines
+18
to
+27
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd -t f -E node_modules '^create-fixtures\.(ts|tsx)$' . \
-x ast-grep outline {} --items all
rg -n -C 5 \
'bindCreateFixtures|setActive|__internal_setActiveInProgress|SignInResource' \
packages/ui/src/components/SignIn/__tests__/SignInFactorOneSetActiveGuard.test.tsx \
$(fd -t f -E node_modules '^create-fixtures\.(ts|tsx)$' .)Repository: clerk/javascript Length of output: 18615 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- test outline ---'
ast-grep outline packages/ui/src/components/SignIn/__tests__/SignInFactorOneSetActiveGuard.test.tsx --items all
printf '%s\n' '--- fixture outline ---'
ast-grep outline packages/ui/src/test/create-fixtures.tsx --items all
printf '%s\n' '--- test source ---'
cat -n packages/ui/src/components/SignIn/__tests__/SignInFactorOneSetActiveGuard.test.tsx
printf '%s\n' '--- fixture implementation ---'
cat -n packages/ui/src/test/create-fixtures.tsx | sed -n '1,220p'
printf '%s\n' '--- relevant declarations and internal fields ---'
rg -n -C 5 \
'setActive\s*[:(]|__internal_setActiveInProgress|firstFactorVerification|class SignIn|interface SignIn|type SetActive|SetActive' \
packages packages/shared 2>/dev/null | head -n 500Repository: clerk/javascript Length of output: 50374 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- fixture helper and mock types ---'
fd -t f -E node_modules 'mock-helpers\.(ts|tsx)$|fixture-helpers\.(ts|tsx)$' packages/ui/src packages/clerk-js/src \
-x sh -c 'echo "--- $1"; cat -n "$1"' sh {}
printf '%s\n' '--- Clerk setActive declarations and implementation ---'
rg -l 'setActive|__internal_setActiveInProgress' packages/clerk-js packages/shared/src \
-g '*.{ts,tsx}' | sort | while read -r file; do
echo "--- $file"
rg -n -C 8 'setActive|__internal_setActiveInProgress' "$file" | head -n 180
done
printf '%s\n' '--- SignInResource relevant fields ---'
sed -n '35,180p' packages/shared/src/types/signIn.ts
rg -n -C 6 'firstFactorVerification' packages/clerk-js packages/shared/src packages/ui/src -g '*.{ts,tsx}' | head -n 240Repository: clerk/javascript Length of output: 50372 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- LoadedClerk and setActive type locations ---'
rg -n 'interface LoadedClerk|type LoadedClerk|setActive\s*\??:|setActive\(' \
packages/shared/src/types packages/clerk-js/src -g '*.{ts,tsx}' | head -n 160
printf '%s\n' '--- exact setActive declarations ---'
rg -l 'setActive' packages/shared/src/types packages/clerk-js/src -g '*.{ts,tsx}' | while read -r file; do
matches=$(rg -n 'setActive' "$file" | head -n 12)
if [ -n "$matches" ]; then
echo "--- $file"
printf '%s\n' "$matches"
fi
done
printf '%s\n' '--- exact sign-in response and verification types ---'
rg -n -C 5 \
'AttemptFirstFactor|FirstFactorVerification|firstFactorVerification|first_factor_verification|createdSessionId|created_session_id' \
packages/shared/src/types/signIn.ts packages/shared/src/types -g '*.ts' | head -n 260
printf '%s\n' '--- internal set-active state references ---'
rg -n -C 6 '__internal_setActiveInProgress' packages -g '*.{ts,tsx}' || trueRepository: clerk/javascript Length of output: 50372 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- LoadedClerk internal field and setActive contract ---'
sed -n '900,930p;1208,1230p;1660,1730p' packages/shared/src/types/clerk.ts
printf '%s\n' '--- SignInResource and status types ---'
sed -n '1,95p' packages/shared/src/types/signIn.ts
rg -n 'export type SignInStatus|type SignInStatus|interface VerificationResource|status:' \
packages/shared/src/types/signIn.ts packages/shared/src/types/verification.ts packages/shared/src/types -g '*.ts' | head -n 120
printf '%s\n' '--- SignUpResource completion contract ---'
rg -n -C 12 'export interface SignUpResource|createdSessionId|status:' \
packages/shared/src/types/signUp.ts | head -n 180
printf '%s\n' '--- Deep mock and fixture return declarations ---'
sed -n '1,100p' packages/ui/src/test/mock-helpers.ts
sed -n '38,150p' packages/ui/src/test/create-fixtures.tsxRepository: clerk/javascript Length of output: 22855 Replace Use the inferred fixture type and 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| }); | ||
|
|
||
| return { finishSetActive: () => release() }; | ||
| }; | ||
|
|
||
| describe('SignIn setActive guard', () => { | ||
| it('does not bounce factor one back to the start card once setActive has completed', async () => { | ||
| const { wrapper, fixtures } = await createFixtures(f => { | ||
| f.withEmailAddress(); | ||
| f.withPreferredSignInStrategy({ strategy: 'otp' }); | ||
| f.startSignInWithEmailAddress({ supportEmailCode: true, supportPassword: false }); | ||
| }); | ||
|
|
||
| fixtures.signIn.prepareFirstFactor.mockReturnValueOnce(Promise.resolve({} as SignInResource)); | ||
| fixtures.signIn.attemptFirstFactor.mockResolvedValueOnce({ | ||
| status: 'complete', | ||
| createdSessionId: 'sess_123', | ||
| } as any); | ||
| const { finishSetActive } = mockSetActiveLifecycle(fixtures); | ||
|
|
||
| const { userEvent, rerender } = render(<SignInFactorOne />, { wrapper }); | ||
|
|
||
| await userEvent.type(screen.getByLabelText(/Enter verification code/i), '123456'); | ||
| await waitFor(() => expect(fixtures.clerk.setActive).toHaveBeenCalled(), { timeout: 3000 }); | ||
|
|
||
| rerender(<SignInFactorOne />); | ||
| finishSetActive(); | ||
| await waitFor(() => expect((fixtures.clerk as any).__internal_setActiveInProgress).toBe(false)); | ||
|
|
||
| // The host app keeps <SignIn> mounted until its own signed-in state propagates, so the card | ||
| // re-renders at least once more after setActive resolves. | ||
| rerender(<SignInFactorOne />); | ||
|
|
||
| await waitFor(() => expect(fixtures.clerk.setActive).toHaveBeenCalled()); | ||
| expect(fixtures.router.navigate).not.toHaveBeenCalledWith('../'); | ||
| }); | ||
|
|
||
| it('does not bounce back to the start card after a signUpIfMissing transfer completes', async () => { | ||
| const { wrapper, fixtures, props } = await createFixtures(f => { | ||
| f.withEmailAddress(); | ||
| f.withPreferredSignInStrategy({ strategy: 'otp' }); | ||
| f.withEnumerationProtection(); | ||
| f.startSignInWithEmailAddress({ supportEmailCode: true, supportPassword: false }); | ||
| }); | ||
| props.setProps({ withSignUp: true }); | ||
|
|
||
| fixtures.signIn.prepareFirstFactor.mockReturnValueOnce(Promise.resolve({} as SignInResource)); | ||
| fixtures.signIn.attemptFirstFactor.mockImplementationOnce(() => { | ||
| (fixtures.signIn as any).firstFactorVerification = { status: 'transferable' }; | ||
| return Promise.reject( | ||
| new ClerkAPIResponseError('Error', { | ||
| data: [{ code: 'sign_up_if_missing_transfer', long_message: '', message: '' }], | ||
| status: 404, | ||
| }), | ||
| ); | ||
| }); | ||
| // A sign-up with no additional requirements transfers straight to `complete`. | ||
| fixtures.signUp.create.mockResolvedValueOnce({ status: 'complete', createdSessionId: 'sess_123' } as any); | ||
| const { finishSetActive } = mockSetActiveLifecycle(fixtures); | ||
|
|
||
| const { userEvent, rerender } = render(<SignInFactorOne />, { wrapper }); | ||
|
|
||
| await userEvent.type(screen.getByLabelText(/Enter verification code/i), '123456'); | ||
| await waitFor(() => expect(fixtures.clerk.setActive).toHaveBeenCalled(), { timeout: 3000 }); | ||
|
|
||
| rerender(<SignInFactorOne />); | ||
| finishSetActive(); | ||
| await waitFor(() => expect((fixtures.clerk as any).__internal_setActiveInProgress).toBe(false)); | ||
|
|
||
| // The terminal redirect leaves the page, but the document stays alive while the browser | ||
| // fetches the next one, so the card can still re-render and bounce. | ||
| rerender(<SignInFactorOne />); | ||
|
|
||
| expect(fixtures.router.navigate).not.toHaveBeenCalledWith('../'); | ||
| }); | ||
|
|
||
| it('still bounces to the start card when the sign-in was abandoned without setActive', async () => { | ||
| const { wrapper, fixtures } = await createFixtures(f => { | ||
| f.withEmailAddress(); | ||
| f.withPreferredSignInStrategy({ strategy: 'otp' }); | ||
| f.startSignInWithEmailAddress({ supportEmailCode: true, supportPassword: false }); | ||
| }); | ||
|
|
||
| fixtures.signIn.prepareFirstFactor.mockReturnValueOnce(Promise.resolve({} as SignInResource)); | ||
| (fixtures.signIn as any).status = 'needs_identifier'; | ||
|
|
||
| render(<SignInFactorOne />, { wrapper }); | ||
|
|
||
| await waitFor(() => expect(fixtures.router.navigate).toHaveBeenCalledWith('../')); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: clerk/javascript
Length of output: 50373
🏁 Script executed:
Repository: clerk/javascript
Length of output: 15356
🏁 Script executed:
Repository: clerk/javascript
Length of output: 342
Add a
SignInFactorTwolifecycle regression test.SignInFactorTwo.test.tsxdoes not cover thesetActiveflag lifecycle. Test activation,signIn.status = null, flag reset, and no navigation to../.🤖 Prompt for AI Agents
Source: Coding guidelines