feat(clerk-js,types): Add support for modal SSO sign-ins #7026
Conversation
🦋 Changeset detectedLatest commit: 653d006 The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 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.
|
WalkthroughSign-in SSO flows were changed to support popup-based routing and enterprise SSO parameters: redirect URLs can be wrapped for popup callbacks, new popup/oidcPrompt/enterpriseConnectionId fields were added to types, new popup-auth utilities were introduced, and SignIn resource flow now performs conditional prepare_first_factor calls and reloads after popup completion. Changes
Sequence DiagramsequenceDiagram
participant User
participant App as App (Main Window)
participant Portal as Account Portal
participant Popup as Popup Window
participant OAuth as OAuth Provider
User->>App: Start SSO (may request popup)
App->>App: build routes (redirectCallbackUrl, redirectUrl)
alt popup requested
App->>App: wrapWithPopupRoutes()
App->>App: POST /sign_ins (create) with popup-wrapped routes
alt strategy == enterprise_sso
App->>App: POST /sign_ins/prepare_first_factor (include oidcPrompt, enterpriseConnectionId, routes)
end
App->>Popup: open popup to externalVerificationURL
App->>Popup: _futureAuthenticateWithPopup (listen for postMessage)
Popup->>OAuth: perform OAuth flow
OAuth->>Popup: redirect back to Portal /popup-callback
Popup-->>App: postMessage(session or return_url)
App->>App: validate origin, resolve Promise
App->>App: reload SignIn resource
else non-popup
App->>App: POST /sign_ins (create) with direct routes
App->>OAuth: navigate main window to externalVerificationURL
end
App->>User: complete sign-in
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Points to review closely:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (6)**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Files:
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Files:
packages/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Files:
packages/**/*.{ts,tsx,d.ts}📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Files:
**/*.{js,ts,tsx,jsx}📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Files:
🧬 Code graph analysis (1)packages/clerk-js/src/utils/authenticateWithPopup.ts (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Comment |
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
.changeset/ripe-banks-pay.md (1)
6-6: Expand the changeset note with migration guidance.Call out:
- Removed:
flowandstrategy: 'saml'inSignInFutureSSOParams.- New:
popup,oidcPrompt,enterpriseConnectionId.
Add short migration examples.packages/types/src/signInFuture.ts (2)
183-196: Model the popup target with a named interface to allow future enhancements (e.g., detect closed).Current type is minimal. Consider:
- Add an exported interface to avoid importing DOM
Window.- Include optional
closed?: booleanfor cleanup.Example:
+export interface PopupLike { + readonly location: { href: string }; + readonly closed?: boolean; +} - popup?: { location: { href: string } }; + popup?: PopupLike;
197-201: Constrain and documentoidcPrompt.Either:
- Keep
stringbut enumerate allowed OIDC values in JSDoc with examples for multi-valued inputs, or- Introduce a union type (with allowance for provider-specific extensions).
This improves dev ergonomics without blocking edge cases.
packages/clerk-js/src/core/resources/SignIn.ts (1)
927-935: Popup path ignoresreturn_urlby design?In the popup branch you wait for completion and reload the resource, but you do not navigate when
return_urlis sent (unlike_authenticateWithPopup, which navigates). If the intended modal UX is “stay on page and update state,” this is fine. Otherwise, consider consumingreturn_urlto navigate or provide a callback.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.changeset/ripe-banks-pay.md(1 hunks)packages/clerk-js/src/core/resources/SignIn.ts(2 hunks)packages/clerk-js/src/utils/authenticateWithPopup.ts(1 hunks)packages/types/src/signInFuture.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
.changeset/**
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Automated releases must use Changesets.
Files:
.changeset/ripe-banks-pay.md
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/types/src/signInFuture.tspackages/clerk-js/src/utils/authenticateWithPopup.tspackages/clerk-js/src/core/resources/SignIn.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/types/src/signInFuture.tspackages/clerk-js/src/utils/authenticateWithPopup.tspackages/clerk-js/src/core/resources/SignIn.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/types/src/signInFuture.tspackages/clerk-js/src/utils/authenticateWithPopup.tspackages/clerk-js/src/core/resources/SignIn.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/types/src/signInFuture.tspackages/clerk-js/src/utils/authenticateWithPopup.tspackages/clerk-js/src/core/resources/SignIn.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/types/src/signInFuture.tspackages/clerk-js/src/utils/authenticateWithPopup.tspackages/clerk-js/src/core/resources/SignIn.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/types/src/signInFuture.tspackages/clerk-js/src/utils/authenticateWithPopup.tspackages/clerk-js/src/core/resources/SignIn.ts
🧬 Code graph analysis (2)
packages/clerk-js/src/utils/authenticateWithPopup.ts (2)
packages/types/src/clerk.ts (1)
Clerk(165-896)packages/shared/src/buildAccountsBaseUrl.ts (1)
buildAccountsBaseUrl(4-15)
packages/clerk-js/src/core/resources/SignIn.ts (3)
packages/clerk-js/src/utils/runAsyncResourceTask.ts (1)
runAsyncResourceTask(8-31)packages/clerk-js/src/utils/authenticateWithPopup.ts (2)
wrapWithPopupRoutes(88-120)_futureAuthenticateWithPopup(122-151)packages/clerk-js/src/utils/windowNavigate.ts (1)
windowNavigate(19-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
packages/types/src/signInFuture.ts (1)
170-176: Remove incorrect claim about 'flow' deprecation; clarify 'saml' removal from SignInFutureSSOParams is intentional.The removal of
'saml'fromSignInFutureSSOParams.strategy(line 173) is intentional and aligns with the@deprecatedmarking onSamlStrategyinpackages/types/src/strategies.ts. Users should migrate to'enterprise_sso'for SSO flows. However, this change should be documented here or in clerk-docs with clear migration guidance, especially since'saml'remains supported inSignInFutureCreateParams(line 17).The claim about
'flow'deprecation is incorrect—SignInFuturePasskeyParams.flow(line 244+) has no deprecation marker and appears unchanged. Remove this from the concern.
| '@clerk/clerk-js': minor | ||
| '@clerk/types': minor | ||
| --- | ||
|
|
||
| [Experimental] Add support for modal SSO sign-ins to new APIs |
There was a problem hiding this comment.
🧩 Analysis chain
Semver: this includes breaking type changes; minor bump is insufficient for @clerk/types.
SignInFutureSSOParams removes flow and drops 'saml' from strategy. That breaks user code compiling against @clerk/types. Please bump @clerk/types as a major (and consider whether @clerk/clerk-js also needs a major if runtime behavior changes), or gate these under experimental types with a pre-release tag.
🏁 Script executed:
#!/bin/bash
# Locate and examine SignInFutureSSOParams type definition
rg -nC5 'SignInFutureSSOParams' --type=ts --type=tsxLength of output: 27
🏁 Script executed:
#!/bin/bash
# Search for flow property removals in sign-in params
rg -nC3 'flow.*saml' --type=ts --type=tsxLength of output: 27
🏁 Script executed:
#!/bin/bash
# Find type definitions in @clerk/types package
fd -e ts -e tsx '@clerk/types' | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for strategy type definition
rg -nC5 "strategy.*'saml'" --type=ts --type=tsxLength of output: 27
🏁 Script executed:
#!/bin/bash
# Search for SignInFutureSSOParams without type filter
rg -nC5 'SignInFutureSSOParams'Length of output: 3074
🏁 Script executed:
#!/bin/bash
# Find packages directory structure
fd -type d 'types' | head -10Length of output: 170
🏁 Script executed:
#!/bin/bash
# Look for .ts files in packages
fd -type f -name '*.ts' | grep -i 'sign\|param' | head -20Length of output: 170
🏁 Script executed:
#!/bin/bash
# Search for strategy union with saml
rg -nC3 "strategy" | grep -i samlLength of output: 9531
🏁 Script executed:
#!/bin/bash
# Find the changeset file to understand PR scope
cat .changeset/ripe-banks-pay.mdLength of output: 117
🏁 Script executed:
#!/bin/bash
# Get full SignInFutureSSOParams interface definition
sed -n '169,190p' packages/types/src/signInFuture.tsLength of output: 664
🏁 Script executed:
#!/bin/bash
# Get context around line 17 to identify the type
sed -n '1,30p' packages/types/src/signInFuture.tsLength of output: 1466
🏁 Script executed:
#!/bin/bash
# Show git diff for signInFuture.ts to see what changed
git diff HEAD~1 packages/types/src/signInFuture.ts | head -100Length of output: 1677
🏁 Script executed:
#!/bin/bash
# Check if experimental tag/flag is used in the file
rg -n 'experimental|@experimental' packages/types/src/signInFuture.tsLength of output: 22
Breaking type changes require major version bump for @clerk/types; current minor classification is incorrect.
The git diff confirms the breaking changes claimed: SignInFutureSSOParams removes the optional flow property and removes 'saml' from the strategy union. Both changes break user code at compile-time or runtime. Only enterpriseConnectionId is marked @experimental; the breaking removals are not gated. Per semver, these require either a major version bump or experimental/pre-release tagging on the breaking properties themselves.
| let routes = { redirectUrl: SignIn.clerk.buildUrlWithAuth(redirectCallbackUrl), actionCompleteRedirectUrl }; | ||
| if (popup) { | ||
| const wrappedRoutes = wrapWithPopupRoutes(SignIn.clerk, { | ||
| redirectCallbackUrl: routes.redirectUrl, | ||
| redirectUrl: actionCompleteRedirectUrl, | ||
| }); | ||
| routes.redirectUrl = wrappedRoutes.redirectCallbackUrl; | ||
| routes.actionCompleteRedirectUrl = wrappedRoutes.redirectUrl; | ||
| } |
There was a problem hiding this comment.
Ensure routes.redirectUrl is absolute before passing to wrapWithPopupRoutes.
buildUrlWithAuth(redirectCallbackUrl) may return a relative URL; wrapWithPopupRoutes uses new URL(...) which requires an absolute or a base. Normalize here to prevent runtime errors.
- let routes = { redirectUrl: SignIn.clerk.buildUrlWithAuth(redirectCallbackUrl), actionCompleteRedirectUrl };
+ let routes = {
+ redirectUrl: (() => {
+ const withAuth = SignIn.clerk.buildUrlWithAuth(redirectCallbackUrl);
+ try {
+ // Will succeed if absolute
+ new URL(withAuth);
+ return withAuth;
+ } catch {
+ return window.location.origin + withAuth;
+ }
+ })(),
+ actionCompleteRedirectUrl,
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let routes = { redirectUrl: SignIn.clerk.buildUrlWithAuth(redirectCallbackUrl), actionCompleteRedirectUrl }; | |
| if (popup) { | |
| const wrappedRoutes = wrapWithPopupRoutes(SignIn.clerk, { | |
| redirectCallbackUrl: routes.redirectUrl, | |
| redirectUrl: actionCompleteRedirectUrl, | |
| }); | |
| routes.redirectUrl = wrappedRoutes.redirectCallbackUrl; | |
| routes.actionCompleteRedirectUrl = wrappedRoutes.redirectUrl; | |
| } | |
| let routes = { | |
| redirectUrl: (() => { | |
| const withAuth = SignIn.clerk.buildUrlWithAuth(redirectCallbackUrl); | |
| try { | |
| // Will succeed if absolute | |
| new URL(withAuth); | |
| return withAuth; | |
| } catch { | |
| return window.location.origin + withAuth; | |
| } | |
| })(), | |
| actionCompleteRedirectUrl, | |
| }; | |
| if (popup) { | |
| const wrappedRoutes = wrapWithPopupRoutes(SignIn.clerk, { | |
| redirectCallbackUrl: routes.redirectUrl, | |
| redirectUrl: actionCompleteRedirectUrl, | |
| }); | |
| routes.redirectUrl = wrappedRoutes.redirectCallbackUrl; | |
| routes.actionCompleteRedirectUrl = wrappedRoutes.redirectUrl; | |
| } |
🤖 Prompt for AI Agents
In packages/clerk-js/src/core/resources/SignIn.ts around lines 898 to 906,
ensure routes.redirectUrl is an absolute URL before passing it to
wrapWithPopupRoutes by detecting if it is relative and resolving it against a
known base (e.g. SignIn.clerk.frontendApi or window.location.origin) using new
URL(relativeOrAbsolute, base). Replace routes.redirectUrl with the resolved
absolute URL (and then call wrapWithPopupRoutes as before), so
wrapWithPopupRoutes always receives an absolute URL and avoids new URL runtime
errors.
| export function wrapWithPopupRoutes( | ||
| client: Clerk, | ||
| { | ||
| redirectCallbackUrl, | ||
| redirectUrl, | ||
| }: { | ||
| /** | ||
| * The route to navigate to if a session was not created. | ||
| */ | ||
| redirectCallbackUrl: string; | ||
| /** | ||
| * The route to navigate to if a session was created. | ||
| */ | ||
| redirectUrl: string; | ||
| }, | ||
| ): { redirectCallbackUrl: string; redirectUrl: string } { | ||
| const accountPortalHost = buildAccountsBaseUrl(client.frontendApi); | ||
|
|
||
| // We set the force_redirect_url query parameter to ensure that the user is redirected to the correct page even | ||
| // in situations like a modal transfer flow. | ||
| const r = new URL(redirectCallbackUrl); | ||
| r.searchParams.set('sign_in_force_redirect_url', redirectUrl); | ||
| r.searchParams.set('sign_up_force_redirect_url', redirectUrl); | ||
| // All URLs are decorated with the dev browser token in development mode since we're moving between AP and the app. | ||
| const redirectUrlWithForceRedirectUrl = client.buildUrlWithAuth(r.toString()); | ||
|
|
||
| const popupRedirectUrlComplete = client.buildUrlWithAuth(`${accountPortalHost}/popup-callback`); | ||
| const popupRedirectUrl = client.buildUrlWithAuth( | ||
| `${accountPortalHost}/popup-callback?return_url=${encodeURIComponent(redirectUrlWithForceRedirectUrl)}`, | ||
| ); | ||
|
|
||
| return { redirectCallbackUrl: popupRedirectUrl, redirectUrl: popupRedirectUrlComplete }; | ||
| } |
There was a problem hiding this comment.
Handle relative redirectCallbackUrl when constructing new URL(...).
new URL(redirectCallbackUrl) throws on relative paths. Use a base to support common inputs like /sso-callback.
- const r = new URL(redirectCallbackUrl);
+ const r = new URL(redirectCallbackUrl, window.location.href);Also consider guarding for SSR by asserting inBrowser() at the call site if needed.
| export function _futureAuthenticateWithPopup( | ||
| client: Clerk, | ||
| params: { popup: { location: { href: string } }; externalVerificationRedirectURL: URL }, | ||
| ): Promise<void> { | ||
| return new Promise((resolve, reject) => { | ||
| if (!client.client || !params.popup) { | ||
| reject(); | ||
| } | ||
|
|
||
| const messageHandler = async (event: MessageEvent) => { | ||
| if (event.origin !== buildAccountsBaseUrl(client.frontendApi)) { | ||
| return; | ||
| } | ||
|
|
||
| // The OAuth flow was successful, and we received a message with either a session or a return URL. | ||
| if (event.data.session || event.data.return_url) { | ||
| window.removeEventListener('message', messageHandler); | ||
| resolve(); | ||
| } else { | ||
| reject(); | ||
| } | ||
| }; | ||
|
|
||
| // Listen for messages from the popup window. | ||
| window.addEventListener('message', messageHandler); | ||
|
|
||
| // Navigate the popup window to the external verification redirect URL, which kicks off the OAuth flow. | ||
| params.popup.location.href = params.externalVerificationRedirectURL.toString(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fix Promise rejection/control-flow and ensure listener cleanup.
reject()is called without returning; code continues to add listeners and navigate.- Listener isn’t removed on reject paths; potential leak.
- Provide an error message for callers.
Apply:
export function _futureAuthenticateWithPopup(
client: Clerk,
params: { popup: { location: { href: string } }; externalVerificationRedirectURL: URL },
): Promise<void> {
return new Promise((resolve, reject) => {
- if (!client.client || !params.popup) {
- reject();
- }
+ if (!client.client || !params.popup) {
+ return reject(new Error('Popup auth requires an initialized client and a popup window.'));
+ }
- const messageHandler = async (event: MessageEvent) => {
- if (event.origin !== buildAccountsBaseUrl(client.frontendApi)) {
+ const apOrigin = buildAccountsBaseUrl(client.frontendApi);
+ const cleanup = () => window.removeEventListener('message', messageHandler);
+ const messageHandler = (event: MessageEvent) => {
+ if (event.origin !== apOrigin) {
return;
}
// The OAuth flow was successful, and we received a message with either a session or a return URL.
if (event.data.session || event.data.return_url) {
- window.removeEventListener('message', messageHandler);
- resolve();
+ cleanup();
+ return resolve();
} else {
- reject();
+ cleanup();
+ return reject(new Error('Popup auth did not return a session or return_url.'));
}
};
// Listen for messages from the popup window.
window.addEventListener('message', messageHandler);
// Navigate the popup window to the external verification redirect URL, which kicks off the OAuth flow.
params.popup.location.href = params.externalVerificationRedirectURL.toString();
});
}Optional: add a timeout to reject if no message is received within N seconds.
🤖 Prompt for AI Agents
In packages/clerk-js/src/utils/authenticateWithPopup.ts around lines 122-151,
the Promise rejects without returning, the message listener isn’t removed on
reject paths, and no error is provided to callers; update the control flow so
that after the initial validation failure you call reject(new Error(...)) and
immediately return; inside the messageHandler, always removeEventListener before
calling resolve() or reject(new Error(...)); if you add an optional timeout,
store its id, clearTimeout upon resolve/reject, and on timeout remove the
listener and reject with a descriptive Error; ensure every early exit path
cleans up the listener and provides an Error message.
Description
This PR adds support for modals to the SSO sign-in method in the new APIs.
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Chores