feat(nextjs,react): Add HandleSSOCallback component #7678
Conversation
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
🦋 Changeset detectedLatest commit: 146a6c8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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.
|
Contributor
📝 WalkthroughWalkthroughAdds a new HandleSSOCallback React component and its props interface to the React package, exports the component from the Next.js package surface and the Chrome Extension re-exports, and bumps minor versions for the clerk chrome-extension, nextjs, and react packages. The component orchestrates SSO callback flows (sign-in, sign-up, transfer paths, session activation), accepts navigateToApp/navigateToSignIn/navigateToSignUp callbacks and an optional render prop, and uses a single-run effect to avoid re-execution in SSR environments. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Comment on lines
+172
to
+173
| {/* Because a sign-in transferred to a sign-up might require captcha verification, make sure to render the | ||
| captcha element. */} |
Member
There was a problem hiding this comment.
should we allow children for them to render a spinner here possibly?
Member
Author
There was a problem hiding this comment.
I went with the render prop for that, but I'll defer to what you suggest here since I know the thinking has evolved recently!
Member
There was a problem hiding this comment.
to date we've used children, so I might lean towards children for now. We've yet to introduce a render prop but possibly in the future.
Member
Author
There was a problem hiding this comment.
stepping back do we even need either? This renders a div and the captcha (possibly). wouldn't folks probably write code that looks like this?
<Card>
<CardBody>
<Spinner />
<HandleSSOCallback />
</CardBody>
</Card>
In this situation I don't think they'd want to put their Spinner as a child of HandleSSOCallback. If it's a render prop it would make more sense for it to be:
<HandleSSOCallback render={() => {
return (
<Card><CardBody><Spinner /></CardBody></Card>
)
}} />
![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
🤖 Fix all issues with AI agents
In `@packages/react/src/components/HandleSSOCallback.tsx`:
- Around line 70-176: Add unit tests for the HandleSSOCallback component that
cover the sign-in/sign-up transfer and completion branches by mocking the Clerk
hooks and flows (useClerk, useSignIn, useSignUp) and asserting
navigation/finalize/setActive behavior: test when signIn.status === 'complete'
that signIn.finalize is called and navigateToApp is invoked; test
signUp.isTransferable path where signIn.create({transfer:true}) leads to
finalize or navigateToSignIn as appropriate; test signIn.isTransferable path
where signUp.create({transfer:true}) leads to finalize or navigateToSignUp; test
the needs_first_factor branch (when supportedFirstFactors do not all have
strategy === 'enterprise_sso') and the needs_second_factor/needs_new_password
branches call navigateToSignIn; test existingSession handling calls
clerk.setActive with sessionId and navigates to app; also include a render test
that returns the captcha div when no render prop provided. Mock and spy on
signIn.create, signIn.finalize, signUp.create, signUp.finalize, clerk.setActive
and the navigate callbacks to verify correct calls.
- Around line 77-164: HandleSSOCallback has no tests — add unit tests for the
component to cover the various SSO flows handled in its useEffect: signIn.status
=== 'complete' path (calls signIn.finalize -> navigateToApp),
signUp.isTransferable -> signIn.create then finalize or navigateToSignIn,
signIn.isTransferable -> signUp.create then finalize or navigateToSignUp,
needs_first_factor/needs_second_factor/needs_new_password -> navigateToSignIn,
and existingSession/session activation path (clerk.setActive -> navigateToApp);
in tests mock the clerk, signIn, signUp objects and assert the appropriate
methods (finalize, create, setActive) and navigation helpers (navigateToApp,
navigateToSignIn, navigateToSignUp) are called, and include at least one test
ensuring hasRun prevents re-running effect. Optionally, to silence
exhaustive-deps warnings in tests or code, either include the callback props
(navigateToApp/navigateToSignIn/navigateToSignUp) in the useEffect dependency
array or explicitly document/ignore the lint warning — but focus tests on
useEffect behaviour for functions useEffect, hasRun, clerk, signIn, signUp.
Comment on lines
70
to
176
| export function HandleSSOCallback(props: HandleSSOCallbackProps): ReactNode { | ||
| const { navigateToApp, navigateToSignIn, navigateToSignUp, render } = props; | ||
| const clerk = useClerk(); | ||
| const { signIn } = useSignIn(); | ||
| const { signUp } = useSignUp(); | ||
| const hasRun = useRef(false); | ||
|
|
||
| useEffect(() => { | ||
| (async () => { | ||
| if (!clerk.loaded || hasRun.current) { | ||
| return; | ||
| } | ||
| // Prevent re-running this effect if the page is re-rendered during session activation (such as on Next.js). | ||
| hasRun.current = true; | ||
|
|
||
| // If this was a sign-in, and it's complete, there's nothing else to do. | ||
| // Note: We perform a cast | ||
| if ((signIn.status as string) === 'complete') { | ||
| await signIn.finalize({ | ||
| navigate: async (...params) => { | ||
| navigateToApp(...params); | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // If the sign-up used an existing account, transfer it to a sign-in. | ||
| if (signUp.isTransferable) { | ||
| await signIn.create({ transfer: true }); | ||
| if (signIn.status === 'complete') { | ||
| await signIn.finalize({ | ||
| navigate: async (...params) => { | ||
| navigateToApp(...params); | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
| // The sign-in requires additional verification, so we need to navigate to the sign-in page. | ||
| return navigateToSignIn(); | ||
| } | ||
|
|
||
| if ( | ||
| signIn.status === 'needs_first_factor' && | ||
| !signIn.supportedFirstFactors?.every(f => f.strategy === 'enterprise_sso') | ||
| ) { | ||
| // The sign-in requires the use of a configured first factor, so navigate to the sign-in page. | ||
| return navigateToSignIn(); | ||
| } | ||
|
|
||
| // If the sign-in used an external account not associated with an existing user, create a sign-up. | ||
| if (signIn.isTransferable) { | ||
| await signUp.create({ transfer: true }); | ||
| if (signUp.status === 'complete') { | ||
| await signUp.finalize({ | ||
| navigate: async (...params) => { | ||
| navigateToApp(...params); | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
| return navigateToSignUp(); | ||
| } | ||
|
|
||
| if (signUp.status === 'complete') { | ||
| await signUp.finalize({ | ||
| navigate: async (...params) => { | ||
| navigateToApp(...params); | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (signIn.status === 'needs_second_factor' || signIn.status === 'needs_new_password') { | ||
| // The sign-in requires a MFA token or a new password, so navigate to the sign-in page. | ||
| return navigateToSignIn(); | ||
| } | ||
|
|
||
| // The external account used to sign-in or sign-up was already associated with an existing user and active | ||
| // session on this client, so activate the session and navigate to the application. | ||
| if (signIn.existingSession || signUp.existingSession) { | ||
| const sessionId = signIn.existingSession?.sessionId || signUp.existingSession?.sessionId; | ||
| if (sessionId) { | ||
| // Because we're activating a session that's not the result of a sign-in or sign-up, we need to use the | ||
| // Clerk `setActive` API instead of the `finalize` API. | ||
| await clerk.setActive({ | ||
| session: sessionId, | ||
| navigate: async (...params) => { | ||
| return navigateToApp(...params); | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
| } | ||
| })(); | ||
| }, [clerk, signIn, signUp]); | ||
|
|
||
| if (render) { | ||
| return render(); | ||
| } | ||
|
|
||
| return ( | ||
| <div> | ||
| {/* Because a sign-in transferred to a sign-up might require captcha verification, make sure to render the | ||
| captcha element. */} | ||
| <div id='clerk-captcha'></div> | ||
| </div> | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find tests that cover HandleSSOCallback
rg -n "HandleSSOCallback" --type ts --type tsx --type js --type jsx -g "*test*" -g "*spec*"Repository: clerk/javascript
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Find tests that cover HandleSSOCallback
rg -n "HandleSSOCallback" | grep -E "(test|spec)\.(ts|tsx|js|jsx)"Repository: clerk/javascript
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for HandleSSOCallback references in the codebase
rg "HandleSSOCallback" --max-count 20Repository: clerk/javascript
Length of output: 636
🏁 Script executed:
#!/bin/bash
# Find test files in packages/react
find packages/react -type f \( -name "*.test.*" -o -name "*.spec.*" \) | head -20Repository: clerk/javascript
Length of output: 1322
🏁 Script executed:
#!/bin/bash
# Check for HandleSSOCallback test file
find packages/react -name "*HandleSSOCallback*test*" -o -name "*HandleSSOCallback*spec*"Repository: clerk/javascript
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for HandleSSOCallback in all test files
find packages/react -path "*__tests__*" -type f \( -name "*.test.*" -o -name "*.spec.*" \) -exec grep -l "HandleSSOCallback" {} \;Repository: clerk/javascript
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check if HandleSSOCallback.tsx was modified in this PR
git diff --name-only | grep -i handlessocallbackRepository: clerk/javascript
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check the overall git status and what files were modified
git diff --name-only | head -20Repository: clerk/javascript
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check recent git history for HandleSSOCallback
git log --oneline --all -n 50 | head -20Repository: clerk/javascript
Length of output: 1527
🏁 Script executed:
#!/bin/bash
# Check if there are any references to HandleSSOCallback in existing test files
rg "HandleSSOCallback" packages/react/src/components/__tests__/Repository: clerk/javascript
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check the HandleSSOCallback.tsx file to understand its context
head -20 packages/react/src/components/HandleSSOCallback.tsxRepository: clerk/javascript
Length of output: 899
🏁 Script executed:
#!/bin/bash
# Check the git history for HandleSSOCallback specifically
git log --all --oneline -- packages/react/src/components/HandleSSOCallback.tsx | head -10Repository: clerk/javascript
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Check what changed in the recent commit for HandleSSOCallback
git show 9cf07019 --stat | grep -A 10 HandleSSOCallbackRepository: clerk/javascript
Length of output: 741
🏁 Script executed:
#!/bin/bash
# Check if tests were added for HandleSSOCallback in the same commit
git show 9cf07019 | grep -A 5 -B 5 "HandleSSOCallback.*test"Repository: clerk/javascript
Length of output: 42
🏁 Script executed:
#!/bin/bash
# List all files added in commit 9cf07019
git show 9cf07019 --name-only | grep -E "(test|spec)"Repository: clerk/javascript
Length of output: 35882
🏁 Script executed:
#!/bin/bash
# Specifically search for HandleSSOCallback in packages/react test directory
rg "HandleSSOCallback" packages/react/src/components/__tests__/Repository: clerk/javascript
Length of output: 42
Add tests for HandleSSOCallback component.
No test coverage found for this component. Add tests covering the sign-in/sign-up transfer branches and completion paths as required by coding guidelines.
🤖 Prompt for AI Agents
In `@packages/react/src/components/HandleSSOCallback.tsx` around lines 70 - 176,
Add unit tests for the HandleSSOCallback component that cover the
sign-in/sign-up transfer and completion branches by mocking the Clerk hooks and
flows (useClerk, useSignIn, useSignUp) and asserting
navigation/finalize/setActive behavior: test when signIn.status === 'complete'
that signIn.finalize is called and navigateToApp is invoked; test
signUp.isTransferable path where signIn.create({transfer:true}) leads to
finalize or navigateToSignIn as appropriate; test signIn.isTransferable path
where signUp.create({transfer:true}) leads to finalize or navigateToSignUp; test
the needs_first_factor branch (when supportedFirstFactors do not all have
strategy === 'enterprise_sso') and the needs_second_factor/needs_new_password
branches call navigateToSignIn; test existingSession handling calls
clerk.setActive with sessionId and navigates to app; also include a render test
that returns the captcha div when no render prop provided. Mock and spy on
signIn.create, signIn.finalize, signUp.create, signUp.finalize, clerk.setActive
and the navigate callbacks to verify correct calls.
Comment on lines
+77
to
+164
| useEffect(() => { | ||
| (async () => { | ||
| if (!clerk.loaded || hasRun.current) { | ||
| return; | ||
| } | ||
| // Prevent re-running this effect if the page is re-rendered during session activation (such as on Next.js). | ||
| hasRun.current = true; | ||
|
|
||
| // If this was a sign-in, and it's complete, there's nothing else to do. | ||
| // Note: We perform a cast | ||
| if ((signIn.status as string) === 'complete') { | ||
| await signIn.finalize({ | ||
| navigate: async (...params) => { | ||
| navigateToApp(...params); | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // If the sign-up used an existing account, transfer it to a sign-in. | ||
| if (signUp.isTransferable) { | ||
| await signIn.create({ transfer: true }); | ||
| if (signIn.status === 'complete') { | ||
| await signIn.finalize({ | ||
| navigate: async (...params) => { | ||
| navigateToApp(...params); | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
| // The sign-in requires additional verification, so we need to navigate to the sign-in page. | ||
| return navigateToSignIn(); | ||
| } | ||
|
|
||
| if ( | ||
| signIn.status === 'needs_first_factor' && | ||
| !signIn.supportedFirstFactors?.every(f => f.strategy === 'enterprise_sso') | ||
| ) { | ||
| // The sign-in requires the use of a configured first factor, so navigate to the sign-in page. | ||
| return navigateToSignIn(); | ||
| } | ||
|
|
||
| // If the sign-in used an external account not associated with an existing user, create a sign-up. | ||
| if (signIn.isTransferable) { | ||
| await signUp.create({ transfer: true }); | ||
| if (signUp.status === 'complete') { | ||
| await signUp.finalize({ | ||
| navigate: async (...params) => { | ||
| navigateToApp(...params); | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
| return navigateToSignUp(); | ||
| } | ||
|
|
||
| if (signUp.status === 'complete') { | ||
| await signUp.finalize({ | ||
| navigate: async (...params) => { | ||
| navigateToApp(...params); | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (signIn.status === 'needs_second_factor' || signIn.status === 'needs_new_password') { | ||
| // The sign-in requires a MFA token or a new password, so navigate to the sign-in page. | ||
| return navigateToSignIn(); | ||
| } | ||
|
|
||
| // The external account used to sign-in or sign-up was already associated with an existing user and active | ||
| // session on this client, so activate the session and navigate to the application. | ||
| if (signIn.existingSession || signUp.existingSession) { | ||
| const sessionId = signIn.existingSession?.sessionId || signUp.existingSession?.sessionId; | ||
| if (sessionId) { | ||
| // Because we're activating a session that's not the result of a sign-in or sign-up, we need to use the | ||
| // Clerk `setActive` API instead of the `finalize` API. | ||
| await clerk.setActive({ | ||
| session: sessionId, | ||
| navigate: async (...params) => { | ||
| return navigateToApp(...params); | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
| } | ||
| })(); | ||
| }, [clerk, signIn, signUp]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full HandleSSOCallback.tsx file
cat -n packages/react/src/components/HandleSSOCallback.tsxRepository: clerk/javascript
Length of output: 7284
🏁 Script executed:
# Search for ESLint configuration files
find . -type f -name ".eslintrc*" -o -name "eslint.config.*" | head -20Repository: clerk/javascript
Length of output: 144
🏁 Script executed:
# Check if eslintConfig is in package.json
find . -type f -name "package.json" | xargs grep -l "eslintConfig" 2>/dev/null | head -5Repository: clerk/javascript
Length of output: 139
🏁 Script executed:
# Look for test files related to HandleSSOCallback
find . -type f \( -name "*HandleSSOCallback*test*" -o -name "*HandleSSOCallback*spec*" \) 2>/dev/nullRepository: clerk/javascript
Length of output: 42
🏁 Script executed:
# Check the main ESLint config for exhaustive-deps rule
cat -n eslint.config.mjs | head -100Repository: clerk/javascript
Length of output: 4287
🏁 Script executed:
# Search for any test files related to HandleSSOCallback
find . -type f \( -name "*.test.*" -o -name "*.spec.*" \) | xargs grep -l "HandleSSOCallback" 2>/dev/nullRepository: clerk/javascript
Length of output: 44
🏁 Script executed:
# Check if there's a specific eslint config for the react package
find packages/react -type f -name "eslint.config.*" -o -name ".eslintrc*"Repository: clerk/javascript
Length of output: 42
🏁 Script executed:
# Look for HandleSSOCallback in any test directory
find . -type f \( -name "*.test.ts*" -o -name "*.spec.ts*" \) | xargs grep -l "HandleSSO" 2>/dev/null | head -5Repository: clerk/javascript
Length of output: 42
🏁 Script executed:
# Read more of the ESLint config to find the react-hooks configuration
cat eslint.config.mjs | grep -A 20 "react-hooks\|exhaustive-deps"Repository: clerk/javascript
Length of output: 1543
🏁 Script executed:
# Check if this file is part of the PR changes (git status)
git diff --name-only HEAD~1 HEAD 2>/dev/null | grep -i "handleSSO\|eslint"Repository: clerk/javascript
Length of output: 42
🏁 Script executed:
# Look for any __tests__ or test directories in packages/react
find packages/react -type d \( -name "__tests__" -o -name "test" -o -name "tests" -o -name "__test__" \) 2>/dev/nullRepository: clerk/javascript
Length of output: 273
🏁 Script executed:
# Check if there are tests for HandleSSOCallback in the __tests__ directory
ls -la packages/react/src/components/__tests__/ | grep -i "handle\|sso\|callback"Repository: clerk/javascript
Length of output: 42
🏁 Script executed:
# List all files in the components __tests__ directory to see what tests exist
ls -1 packages/react/src/components/__tests__/Repository: clerk/javascript
Length of output: 398
🏁 Script executed:
# Search the entire packages/react for any reference to HandleSSOCallback
grep -r "HandleSSOCallback" packages/react --include="*.ts" --include="*.tsx" --include="*.test.ts" --include="*.test.tsx" 2>/dev/nullRepository: clerk/javascript
Length of output: 562
🏁 Script executed:
# Check what the react-hooks recommended config includes for exhaustive-deps
# We need to check if exhaustive-deps is in the recommended rules and at what level
python3 << 'EOF'
# The react-hooks recommended config typically includes exhaustive-deps
# Let's check the default severity for recommended rules in React Hooks ESLint plugin
# According to react-hooks/eslint-plugin-react-hooks, the recommended config includes:
# - rules-of-hooks: 'error'
# - exhaustive-deps: 'warn'
# So exhaustive-deps should be 'warn' by default in the recommended config
print("React Hooks recommended config typically includes:")
print(" rules-of-hooks: 'error'")
print(" exhaustive-deps: 'warn'")
print("\nSince the config spreads ...pluginReactHooks.configs.recommended.rules,")
print("exhaustive-deps should be active at 'warn' level for this project.")
EOFRepository: clerk/javascript
Length of output: 146
🏁 Script executed:
# Let's verify by checking if any files in the repo have exhaustive-deps configured
grep -r "exhaustive-deps" . --include="eslint*" --include="*.config.*" 2>/dev/null | head -10Repository: clerk/javascript
Length of output: 42
🏁 Script executed:
# Final confirmation: check if any PR changes reference tests being added for HandleSSOCallback
git log --oneline -20 2>/dev/null | head -5Repository: clerk/javascript
Length of output: 93
🏁 Script executed:
# Check the exact react-hooks version to confirm exhaustive-deps behavior
grep -A 2 "eslint-plugin-react-hooks" packages/react/package.json 2>/dev/null || grep -A 2 "eslint-plugin-react-hooks" package.json 2>/dev/null | head -10Repository: clerk/javascript
Length of output: 187
Add tests to cover the new HandleSSOCallback component.
The component is new with no test coverage. Per project guidelines, tests must be added to cover changes.
The useEffect dependency array triggers an exhaustive-deps warning (warn-level) for the missing callback props, but the hasRun guard prevents re-execution regardless. If desired, add them to dependencies to silence the warning, but the pattern is valid as-is.
🤖 Prompt for AI Agents
In `@packages/react/src/components/HandleSSOCallback.tsx` around lines 77 - 164,
HandleSSOCallback has no tests — add unit tests for the component to cover the
various SSO flows handled in its useEffect: signIn.status === 'complete' path
(calls signIn.finalize -> navigateToApp), signUp.isTransferable -> signIn.create
then finalize or navigateToSignIn, signIn.isTransferable -> signUp.create then
finalize or navigateToSignUp,
needs_first_factor/needs_second_factor/needs_new_password -> navigateToSignIn,
and existingSession/session activation path (clerk.setActive -> navigateToApp);
in tests mock the clerk, signIn, signUp objects and assert the appropriate
methods (finalize, create, setActive) and navigation helpers (navigateToApp,
navigateToSignIn, navigateToSignUp) are called, and include at least one test
ensuring hasRun prevents re-running effect. Optionally, to silence
exhaustive-deps warnings in tests or code, either include the callback props
(navigateToApp/navigateToSignIn/navigateToSignUp) in the useEffect dependency
array or explicitly document/ignore the lint warning — but focus tests on
useEffect behaviour for functions useEffect, hasRun, clerk, signIn, signUp.
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@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: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
This PR adds a new
HandleSSOCallbackcomponent which wraps our custom ui flow for handling transferrable sign-ins and sign-ups. This component packages the specific logic that we use to handle that flow in our own components; customers will still be able to implement their own specific custom flow, but this component is being made available to make that a little easier.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.