Uh oh!
There was an error while loading. Please reload this page.
fix(js): keep native OAuth transport callbacks inside the component router - #9370
Conversation
🦋 Changeset detectedLatest commit: 888ed33 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.
|
@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/vuecommit: |
Uh oh!
There was an error while loading. Please reload this page.
API Changes Report
Summary
@clerk/sharedCurrent version: 4.27.1 Subpath |
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 (2)
🔗 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 change adds internal router navigation to OAuth callback parameters and passes it from sign-in and sign-up social buttons. Clerk JS callback handling uses this navigation before default Clerk navigation. Transport authentication now uses the registered transport URL for both redirect parameters. Tests cover callback routing and redirect replacement. A patch changeset documents the fixes. Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/clerk-js/src/utils/__tests__/authenticateWithTransport.test.ts (1)
55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the new test type-safe.
The new fixture uses
as anyat Lines 55, 61, and 69. These casts bypass the resource, Clerk, and authentication parameter contracts. Define narrow mock types or construct the required fields so TypeScript can detect API drift in this test.As per coding guidelines, avoid
anytypes without justification and preferunknownwith type guards when the type is uncertain.Also applies to: 61-61, 69-69
🤖 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/clerk-js/src/utils/__tests__/authenticateWithTransport.test.ts` at line 55, Remove the `as any` casts from the new fixtures in `authenticateWithTransport` tests and make the resource, Clerk, and authentication parameter mocks satisfy their declared contracts. Define narrow mock types or provide the required fields, using `unknown` with type guards only where necessary, so TypeScript catches API changes.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/clerk-js/src/utils/__tests__/authenticateWithTransport.test.ts`:
- Line 55: Remove the `as any` casts from the new fixtures in
`authenticateWithTransport` tests and make the resource, Clerk, and
authentication parameter mocks satisfy their declared contracts. Define narrow
mock types or provide the required fields, using `unknown` with type guards only
where necessary, so TypeScript catches API changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 685c0cde-0621-43aa-b705-732bfd37449d
📒 Files selected for processing (10)
.changeset/native-oauth-modal-navigation.mdpackages/clerk-js/src/core/__tests__/clerk.test.tspackages/clerk-js/src/core/clerk.tspackages/clerk-js/src/utils/__tests__/authenticateWithTransport.test.tspackages/clerk-js/src/utils/authenticateWithTransport.tspackages/shared/src/types/clerk.tspackages/ui/src/components/SignIn/SignInSocialButtons.tsxpackages/ui/src/components/SignIn/__tests__/SignInSocialButtons.test.tsxpackages/ui/src/components/SignUp/SignUpSocialButtons.tsxpackages/ui/src/components/SignUp/__tests__/SignUpSocialButtons.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/cli(auto-detected)clerk/clerk-ios(auto-detected)clerk/clerk-android(auto-detected)
| customNavigate && typeof customNavigate === 'function' | ||
| ? customNavigate(this.buildUrlWithAuth(to)) | ||
| resolvedNavigate && typeof resolvedNavigate === 'function' | ||
| ? resolvedNavigate(this.buildUrlWithAuth(to)) |
There was a problem hiding this comment.
__internal_navigate should receive the raw component-relative path here. Native transport reaches this method through _authenticateWithTransport → __internal_handleResourceCallback; with a development publishable key, buildUrlWithAuth("continue") produces a root-level absolute URL such as http://host/continue. The virtual router then sees /continue as outside /CLERK-ROUTER/VIRTUAL and delegates to window-level navigation, so test-instance Electron flows still break out of the modal. Could we keep the existing decorated behavior for customNavigate, but pass raw to to params.__internal_navigate? A regression test should exercise __internal_handleResourceCallback with developmentPublishableKey; the current test calls handleRedirectCallback with a production key, so it does not cover this path.
jeremy-clerk
left a comment
There was a problem hiding this comment.
LGTM 👍 but touches core, would like to see another set of eyes
Description
This PR fixes native OAuth transport flows (e.g.
@clerk/electron) breaking out of modal components. When a callback needed an intermediate step such as a sign-in to sign-up transfer, the continue step, or an MFA factor, clerk-js navigated the app window to an internal Clerk route like/CLERK-ROUTER/VIRTUAL/sign-up. In Electron apps this reloaded the renderer on a route the app does not have, and the leaked URL was later submitted as the completion redirect, which production instances reject with a "Redirect url mismatch" error (ref).The prebuilt UI now hands its own router to the transport callback via an internal param so those steps render inside the component, and
_authenticateWithTransportnow submits the registered transport callback URL as the completion redirect while the app's real destination is navigated client-side. Both changes are internal and optional, so older UI versions keep the existing behavior.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change