fix(ui): Stop the sign-in start card flashing after a code is accepted - #9331
Open
dmoerner wants to merge 2 commits into
Open
fix(ui): Stop the sign-in start card flashing after a code is accepted#9331dmoerner wants to merge 2 commits into
dmoerner wants to merge 2 commits into
Conversation
After a verification code was accepted, the sign-in start card could reappear for a moment before the app rendered its signed-in state. `SignInFactorOne` has a guard that sends you back to the start of the flow when the sign-in has no status left, written for "user tried a social button, then came back". #6462 gave that guard an early-return while `setActive` is running, and in doing so made it re-run every time `setActive` starts or stops rather than only on mount. So it runs once more as `setActive` finishes -- and at that point a successful sign-in also has no status left, because completing it consumed the sign-in. The guard can't tell the two cases apart, so it sends a user who just signed in back to the start card. It only showed up sometimes because it needs the card to re-render both during `setActive` and again after it, racing whatever the app does once the session exists. The fix records that `setActive` took over, so a card that has handed off navigation never sends the user back. Behaviour for a genuinely abandoned sign-in is unchanged. `SignInFactorTwo` has the same guard and the same bug, so it gets the same fix. This PR was written by Claude based on a reproduction video that I was able to record. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 5879680 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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.
|
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-google-signin
@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: |
dmoerner
marked this pull request as ready for review
August 7, 2026 13:51
Contributor
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe sign-in factor effects now track when Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/SignIn/__tests__/SignInFactorOneSetActiveGuard.test.tsx`:
- Around line 18-27: Update mockSetActiveLifecycle to use the inferred fixture
type and Clerk’s SetActiveParams instead of any, accessing
__internal_setActiveInProgress directly. Set firstFactorVerification.status
directly, and replace any-cast completion responses with typed SignInResource
and SignUpResource completion responses.
In `@packages/ui/src/components/SignIn/SignInFactorTwo.tsx`:
- Around line 32-43: Add a lifecycle regression test for SignInFactorTwo
covering activation while clerk.__internal_setActiveInProgress is true, the
subsequent signIn.status = null transition, resetting setActiveTookOverRef, and
confirming navigation to ../ does not occur. Reuse the component’s existing test
setup and navigation mocks.
🪄 Autofix
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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 327e5405-8086-424d-9d37-9e23848d4cb9
📒 Files selected for processing (4)
.changeset/olive-donuts-wave.mdpackages/ui/src/components/SignIn/SignInFactorOne.tsxpackages/ui/src/components/SignIn/SignInFactorTwo.tsxpackages/ui/src/components/SignIn/__tests__/SignInFactorOneSetActiveGuard.test.tsx
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/clerk-ios(auto-detected)clerk/cli(auto-detected)clerk/clerk-android(auto-detected)
Comment on lines
+18
to
+27
| 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; |
Contributor
There was a problem hiding this comment.
📐 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 any with the existing Clerk types.
Use the inferred fixture type and SetActiveParams contract. Access __internal_setActiveInProgress directly. Update firstFactorVerification.status directly. Use typed SignInResource and SignUpResource completion responses instead of as any casts.
🤖 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/__tests__/SignInFactorOneSetActiveGuard.test.tsx`
around lines 18 - 27, Update mockSetActiveLifecycle to use the inferred fixture
type and Clerk’s SetActiveParams instead of any, accessing
__internal_setActiveInProgress directly. Set firstFactorVerification.status
directly, and replace any-cast completion responses with typed SignInResource
and SignUpResource completion responses.
Source: Coding guidelines
Comment on lines
+32
to
43
| const setActiveTookOverRef = React.useRef(false); | ||
|
|
||
| React.useEffect(() => { | ||
| if (clerk.__internal_setActiveInProgress) { | ||
| // setActive owns navigation from here on. It consumes the sign-in (status -> null), so the | ||
| // check below would fire as setActive winds down and redirect over a flow that succeeded. | ||
| setActiveTookOverRef.current = true; | ||
| return; | ||
| } | ||
|
|
||
| if (setActiveTookOverRef.current) { | ||
| return; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline packages/ui/src/components/SignIn/SignInFactorTwo.tsx --items all
rg -n -C 5 \
'SignInFactorTwo|__internal_setActiveInProgress|setActive|navigate\(' \
packages/ui/src/components/SignIn \
--glob '*.{test,spec}.{ts,tsx}' || trueRepository: clerk/javascript
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- SignInFactorTwo.tsx ---'
sed -n '1,155p' packages/ui/src/components/SignIn/SignInFactorTwo.tsx
printf '%s\n' '--- SignInFactorTwo tests ---'
sed -n '1,220p' packages/ui/src/components/SignIn/__tests__/SignInFactorTwo.test.tsx
printf '%s\n' '--- lifecycle-specific test names and assertions ---'
rg -n -i \
'setActive|__internal_setActiveInProgress|status\s*=\s*null|navigate.*\.\./|not\.toHaveBeenCalledWith.*\.\./' \
packages/ui/src/components/SignIn/__tests__/SignInFactorTwo*.{test,spec}.{ts,tsx} \
2>/dev/null || trueRepository: clerk/javascript
Length of output: 15356
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
component = Path("packages/ui/src/components/SignIn/SignInFactorTwo.tsx").read_text()
test_files = list(Path("packages/ui/src/components/SignIn/__tests__").glob("SignInFactorTwo*.test.tsx"))
tests = "\n".join(p.read_text() for p in test_files)
required = {
"imports SignInFactorTwo": bool(re.search(r"import\s+\{\s*SignInFactorTwo\s*\}", tests)),
"sets setActive progress": "__internal_setActiveInProgress" in tests,
"sets signIn status null": bool(re.search(r"signIn\.status\s*=\s*null", tests)),
"resets progress flag": bool(re.search(r"__internal_setActiveInProgress\s*=\s*false", tests)),
"asserts no ../ navigation": bool(re.search(r"not\.toHaveBeenCalledWith\(\s*['\"]\.\./['\"]", tests)),
}
print("component has takeover guard:", "__internal_setActiveInProgress" in component)
for name, present in required.items():
print(f"{name}: {present}")
PYRepository: clerk/javascript
Length of output: 342
Add a SignInFactorTwo lifecycle regression test.
SignInFactorTwo.test.tsx does not cover the setActive flag lifecycle. Test activation, signIn.status = null, flag reset, and no navigation to ../.
🤖 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/SignInFactorTwo.tsx` around lines 32 - 43,
Add a lifecycle regression test for SignInFactorTwo covering activation while
clerk.__internal_setActiveInProgress is true, the subsequent signIn.status =
null transition, resetting setActiveTookOverRef, and confirming navigation to
../ does not occur. Reuse the component’s existing test setup and navigation
mocks.
Source: Coding guidelines
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After a verification code was accepted, the sign-in start card could reappear for a moment before the app rendered its signed-in state.
SignInFactorOnehas a guard that sends you back to the start of the flow when the sign-in has no status left, written for "user tried a social button, then came back". #6462 gave that guard an early-return whilesetActiveis running, and in doing so made it re-run every timesetActivestarts or stops rather than only on mount. So it runs once more assetActivefinishes -- and at that point a successful sign-in also has no status left, because completing it consumed the sign-in. The guard can't tell the two cases apart, so it sends a user who just signed in back to the start card.It only showed up sometimes because it needs the card to re-render both during
setActiveand again after it, racing whatever the app does once the session exists.The fix records that
setActivetook over, so a card that has handed off navigation never sends the user back. Behaviour for a genuinely abandoned sign-in is unchanged.SignInFactorTwohas the same guard and the same bug, so it gets the same fix.This PR was written by Claude based on a reproduction video that I was able to record. Reproduction video, at round 6.85 seconds:
sign-in-bounce.mp4
Description
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change