fix(tanstack-react-start): Parse URL query params for TanStack Router navigation #7741
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: d3ecfeb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
📝 WalkthroughWalkthroughThis pull request introduces a fix for TanStack Start navigation when URLs contain query parameters. It adds a changeset documenting that URLs with query strings (such as 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. No actionable comments were generated in the recent review. 🎉 Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
@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/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: |
wobsoriano
left a comment
wobsoriano
left a comment
There was a problem hiding this comment.
Thanks! This should fix #7362 right?
|
great work, would really like to see this merged soon so our team can stop patching the clerk dependency. |
|
no, this does not fix #7362. i applied the patch manually to the dist/ output and i am noticing routerPush is called with |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @.changeset/green-humans-yawn.md:
- Line 5: Update the changeset text so it correctly states that using
rootAuthLoader without installing clerkMiddleware() will now throw a runtime
error (instead of saying “will not throw”); edit the sentence mentioning
rootAuthLoader and clerkMiddleware() to read something like “Usage of
rootAuthLoader without the clerkMiddleware() installed will now throw a runtime
error” so it matches the migration steps and makes the breaking change explicit.
In @.changeset/sparkly-aliens-see.md:
- Around line 1-11: This changeset is incorrect for the PR: it documents a
breaking change to getToken() throwing ClerkOfflineError and bumps many
packages, while the PR fixes TanStack Router parsing; verify whether this
changeset (.changeset/sparkly-aliens-see.md) belongs to this PR and if not
replace it with a focused changeset that documents the parseUrlForNavigation()
utility and the navigation query-parameter parsing fixes (or split unrelated
getToken() changes into a separate PR/changeset); ensure the new changeset
mentions the exact symbol parseUrlForNavigation and the concrete user-visible
behavior change (what navigation parsing bug was fixed), and remove any
unrelated package major bumps or ClerkOfflineError/getToken references unless
they truly belong to this PR.
In `@integration/tests/tanstack-start/keyless.test.ts`:
- Around line 11-13: The test file is using the wrong app config and
description: replace the use of appConfigs.reactRouter.reactRouterNode.clone()
(assigned to commonSetup) with appConfigs.tanstack.reactStart.clone(), update
the test.describe string from 'Keyless mode `@react-router`' to 'Keyless mode
`@tanstack-react-start`' (or remove the file if duplicates are undesired), and
ensure any helper invocations in this file include framework: 'tanstack-start'
so the keyless test targets TanStack Start instead of React Router.
In `@packages/expo/src/google/index.ts`:
- Around line 1-5: This file is a re-export-only barrel for useSignInWithGoogle
and its types (StartGoogleAuthenticationFlowParams,
StartGoogleAuthenticationFlowReturnType); replace the barrel by moving these
exports into a dedicated non-barrel entry (or have consumers import directly
from the original module) so the index.ts no longer only re-exports. Concretely,
remove the re-export lines from this index, update consumers to import
useSignInWithGoogle and the two types from the original hook module (the module
that defines useSignInWithGoogle and its types), or create a named module (e.g.,
googleAuth.ts) that directly exports the implementation and types instead of
acting as an index barrel.
In `@packages/hono/src/clerkMiddleware.ts`:
- Around line 38-43: The destructuring currently uses options || {envDefaults}
causing env defaults to be ignored when options exists; change the merge to
start from environment defaults and then spread options so provided fields
override env values (e.g., build an envDefaults object from
clerkEnv.CLERK_SECRET_KEY, CLERK_PUBLISHABLE_KEY, CLERK_API_URL,
CLERK_API_VERSION and then do { ...envDefaults, ...(options || {}) } before
destructuring) so secretKey, publishableKey, apiUrl, and apiVersion correctly
fall back to environment values when not supplied in options.
In `@packages/shared/src/keyless/resolveKeysWithKeylessFallback.ts`:
- Around line 5-10: There are two identical KeylessResult type definitions;
remove the duplicate in service.ts and import the exported KeylessResult from
resolveKeysWithKeylessFallback.ts instead: delete the KeylessResult interface in
service.ts, add an import for KeylessResult at the top of service.ts, and update
any local references in functions/methods in service.ts (e.g., places using
KeylessResult, function signatures, or return types) to use the imported type so
the project relies on the single canonical definition in
resolveKeysWithKeylessFallback.ts.
🧹 Nitpick comments (2)
.changeset/improve-token-validation.md (1)
1-5: Consider expanding the changeset description for better clarity.The changeset description is very brief. Users reviewing the changelog would benefit from more context about what specific token validation was improved, what behavior changed, or what issues this prevents.
For example:
- Which token types are now validated more strictly?
- What was the previous behavior that's being improved?
- Does this fix a specific authentication failure mode?
A more detailed description helps downstream consumers understand the impact and decide whether to upgrade.
.changeset/pretty-queens-relate.md (1)
1-5: Changeset appears unrelated to PR objective.This changeset documents an MFA strategy selection UI improvement, while the PR title and summary focus on fixing TanStack Router query parameter parsing. Bundling unrelated changes can make review and git history less clear.
Consider keeping PRs focused on a single concern, or ensuring the PR description explicitly mentions all included changes.
| '@clerk/react-router': major | ||
| --- | ||
|
|
||
| Usage of `rootAuthLoader` without the `clerkMiddleware()` installed will not throw a runtime error. |
There was a problem hiding this comment.
Fix the breaking-change wording (contradicts migration steps).
Line 5 says the behavior “will not throw,” but the rest of the changeset indicates middleware is now required. This should state that it will now throw (or equivalent) to avoid misleading users.
🛠️ Suggested wording fix
-Usage of `rootAuthLoader` without the `clerkMiddleware()` installed will not throw a runtime error.
+Usage of `rootAuthLoader` without the `clerkMiddleware()` installed will now throw a runtime error.📝 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.
| Usage of `rootAuthLoader` without the `clerkMiddleware()` installed will not throw a runtime error. | |
| Usage of `rootAuthLoader` without the `clerkMiddleware()` installed will now throw a runtime error. |
🤖 Prompt for AI Agents
In @.changeset/green-humans-yawn.md at line 5, Update the changeset text so it
correctly states that using rootAuthLoader without installing clerkMiddleware()
will now throw a runtime error (instead of saying “will not throw”); edit the
sentence mentioning rootAuthLoader and clerkMiddleware() to read something like
“Usage of rootAuthLoader without the clerkMiddleware() installed will now throw
a runtime error” so it matches the migration steps and makes the breaking change
explicit.
| --- | ||
| '@clerk/tanstack-react-start': major | ||
| '@clerk/react-router': major | ||
| '@clerk/clerk-js': major | ||
| '@clerk/upgrade': major | ||
| '@clerk/nextjs': major | ||
| '@clerk/shared': major | ||
| '@clerk/react': major | ||
| '@clerk/nuxt': major | ||
| '@clerk/vue': major | ||
| --- |
There was a problem hiding this comment.
Critical: Changeset content does not match PR objectives.
This changeset documents a breaking change to getToken() throwing ClerkOfflineError, but the PR title and summary describe fixing TanStack Router navigation query parameter parsing. These are completely unrelated changes.
The package list (10 major bumps across the entire SDK suite) also suggests a broad getToken() behavioral change rather than a focused TanStack Router navigation fix.
Please verify:
- Is this the correct changeset for this PR?
- If not, replace it with a changeset documenting the
parseUrlForNavigation()utility and navigation fixes described in the PR summary. - If the PR contains multiple unrelated changes, they should be split into separate PRs.
Based on the PR comments, users are urgently waiting for the TanStack Router navigation fix—ensure the changeset accurately documents that change.
🤖 Prompt for AI Agents
In @.changeset/sparkly-aliens-see.md around lines 1 - 11, This changeset is
incorrect for the PR: it documents a breaking change to getToken() throwing
ClerkOfflineError and bumps many packages, while the PR fixes TanStack Router
parsing; verify whether this changeset (.changeset/sparkly-aliens-see.md)
belongs to this PR and if not replace it with a focused changeset that documents
the parseUrlForNavigation() utility and the navigation query-parameter parsing
fixes (or split unrelated getToken() changes into a separate PR/changeset);
ensure the new changeset mentions the exact symbol parseUrlForNavigation and the
concrete user-visible behavior change (what navigation parsing bug was fixed),
and remove any unrelated package major bumps or ClerkOfflineError/getToken
references unless they truly belong to this PR.
| const commonSetup = appConfigs.reactRouter.reactRouterNode.clone(); | ||
|
|
||
| const mockClaimedInstanceEnvironmentCall = async (page: Page) => { | ||
| await page.route('*/**/v1/environment*', async route => { | ||
| const response = await route.fetch(); | ||
| const json = await response.json(); | ||
| const newJson = { | ||
| ...json, | ||
| auth_config: { | ||
| ...json.auth_config, | ||
| claimed_at: Date.now(), | ||
| }, | ||
| }; | ||
| await route.fulfill({ response, json: newJson }); | ||
| }); | ||
| }; | ||
|
|
||
| test.describe('Keyless mode @tanstack-react-start', () => { | ||
| test.describe('Keyless mode @react-router', () => { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if there's already a keyless test in react-router directory
fd -t f 'keyless.test.ts' integration/tests/react-router/ 2>/dev/null || echo "No keyless.test.ts in react-router"
# Check all test files in tanstack-start directory
echo "=== Files in tanstack-start ==="
fd -t f '*.test.ts' integration/tests/tanstack-start/
# Check all directories under integration/tests
echo "=== Test directories ==="
fd -t d . integration/tests/ -max-depth 1Repository: clerk/javascript
Length of output: 700
🏁 Script executed:
# Use correct fd syntax
echo "=== Checking for keyless.test.ts files ==="
fd -t f 'keyless.test.ts' integration/tests/
echo "=== All files in tanstack-start directory ==="
find integration/tests/tanstack-start/ -type f 2>/dev/null || echo "Directory empty or not found"
echo "=== All files in react-router directory ==="
find integration/tests/react-router/ -type f -name '*.test.ts' 2>/dev/null | head -20
echo "=== Checking if tanstack-start directory exists ==="
ls -la integration/tests/tanstack-start/ 2>/dev/null || echo "tanstack-start directory does not exist"Repository: clerk/javascript
Length of output: 873
🏁 Script executed:
echo "=== Content of tanstack-start/keyless.test.ts ==="
head -50 integration/tests/tanstack-start/keyless.test.ts
echo ""
echo "=== Content of react-router/keyless.test.ts (first 50 lines) ==="
head -50 integration/tests/react-router/keyless.test.tsRepository: clerk/javascript
Length of output: 3365
🏁 Script executed:
echo "=== Searching for tanstack-start configuration in appConfigs ==="
rg -A 5 'tanstackStart\|tanstack-start' integration/tests/presets.ts | head -40
echo ""
echo "=== Checking tanstack-start/basic.test.ts ==="
head -20 integration/tests/tanstack-start/basic.test.tsRepository: clerk/javascript
Length of output: 845
🏁 Script executed:
echo "=== Finding presets file ==="
fd -t f 'presets' integration/tests/
echo ""
echo "=== Checking for tanstack configurations in found presets ==="
find integration -name '*presets*' -type f | head -5 | xargs grep -l 'tanstack' 2>/dev/null || echo "No tanstack in presets files"
echo ""
echo "=== Finding appConfigs structure ==="
rg 'export.*appConfigs' integration/tests/ -A 3 | head -30Repository: clerk/javascript
Length of output: 218
🏁 Script executed:
echo "=== Finding presets.ts file ==="
fd -t f 'presets.ts'
echo ""
echo "=== Showing relative path from test file to presets ==="
# Test file is at integration/tests/tanstack-start/keyless.test.ts
# It imports from '../../presets'
# So presets should be at integration/presets.ts or similar
fd -t f 'presets' integration/
echo ""
echo "=== Checking git status to see if presets was modified ==="
git -C integration log --oneline -5 2>/dev/null || echo "Not a git repo"
echo ""
echo "=== Looking at directory structure ==="
ls -la integration/Repository: clerk/javascript
Length of output: 1636
🏁 Script executed:
echo "=== Files in presets directory ==="
ls -la integration/presets/
echo ""
echo "=== Checking for tanstack configuration ==="
rg -l 'tanstack' integration/presets/
echo ""
echo "=== Checking presets index or main export ==="
cat integration/presets/index.ts 2>/dev/null || ls integration/presets/*.tsRepository: clerk/javascript
Length of output: 1940
🏁 Script executed:
echo "=== Content of tanstack.ts preset ==="
cat integration/presets/tanstack.ts
echo ""
echo "=== Content of react-router.ts preset ==="
cat integration/presets/react-router.tsRepository: clerk/javascript
Length of output: 1867
Duplicate test file with incorrect framework configuration.
The file integration/tests/tanstack-start/keyless.test.ts is an exact duplicate of integration/tests/react-router/keyless.test.ts and tests React Router instead of TanStack Start. This contradicts the file's location and the pattern seen in integration/tests/tanstack-start/basic.test.ts, which correctly uses appConfigs.tanstack.reactStart and tests @tanstack-react-start.
Update integration/tests/tanstack-start/keyless.test.ts to use appConfigs.tanstack.reactStart instead of appConfigs.reactRouter.reactRouterNode, change the test description to @tanstack-react-start, and pass framework: 'tanstack-start' to helper functions. Alternatively, remove the file if duplicate keyless tests across frameworks are not needed.
🤖 Prompt for AI Agents
In `@integration/tests/tanstack-start/keyless.test.ts` around lines 11 - 13, The
test file is using the wrong app config and description: replace the use of
appConfigs.reactRouter.reactRouterNode.clone() (assigned to commonSetup) with
appConfigs.tanstack.reactStart.clone(), update the test.describe string from
'Keyless mode `@react-router`' to 'Keyless mode `@tanstack-react-start`' (or remove
the file if duplicates are undesired), and ensure any helper invocations in this
file include framework: 'tanstack-start' so the keyless test targets TanStack
Start instead of React Router.
| export { useSignInWithGoogle } from '../hooks/useSignInWithGoogle'; | ||
| export type { | ||
| StartGoogleAuthenticationFlowParams, | ||
| StartGoogleAuthenticationFlowReturnType, | ||
| } from '../hooks/useSignInWithGoogle.types'; |
There was a problem hiding this comment.
Avoid barrel re-exports in index.ts to prevent circular-dependency risk.
This file is a barrel (index.ts) that only re-exports symbols, which is disallowed and can introduce circular dependencies. Please expose the entry point via a non-barrel module or adjust exports so this file is not a re-export-only barrel.
As per coding guidelines, "Avoid barrel files (index.ts re-exports) as they can cause circular dependencies".
🤖 Prompt for AI Agents
In `@packages/expo/src/google/index.ts` around lines 1 - 5, This file is a
re-export-only barrel for useSignInWithGoogle and its types
(StartGoogleAuthenticationFlowParams, StartGoogleAuthenticationFlowReturnType);
replace the barrel by moving these exports into a dedicated non-barrel entry (or
have consumers import directly from the original module) so the index.ts no
longer only re-exports. Concretely, remove the re-export lines from this index,
update consumers to import useSignInWithGoogle and the two types from the
original hook module (the module that defines useSignInWithGoogle and its
types), or create a named module (e.g., googleAuth.ts) that directly exports the
implementation and types instead of acting as an index barrel.
| const { secretKey, publishableKey, apiUrl, apiVersion, ...rest } = options || { | ||
| secretKey: clerkEnv.CLERK_SECRET_KEY || '', | ||
| publishableKey: clerkEnv.CLERK_PUBLISHABLE_KEY || '', | ||
| apiUrl: clerkEnv.CLERK_API_URL, | ||
| apiVersion: clerkEnv.CLERK_API_VERSION, | ||
| }; |
There was a problem hiding this comment.
Options fallback logic doesn't merge with environment variables.
When options is provided (even partially), the environment variable fallback is completely bypassed. If a user passes { apiUrl: 'custom' } without secretKey, the secretKey will be undefined instead of falling back to CLERK_SECRET_KEY.
Proposed fix to properly merge options with env fallbacks
- const { secretKey, publishableKey, apiUrl, apiVersion, ...rest } = options || {
- secretKey: clerkEnv.CLERK_SECRET_KEY || '',
- publishableKey: clerkEnv.CLERK_PUBLISHABLE_KEY || '',
- apiUrl: clerkEnv.CLERK_API_URL,
- apiVersion: clerkEnv.CLERK_API_VERSION,
- };
+ const {
+ secretKey = clerkEnv.CLERK_SECRET_KEY || '',
+ publishableKey = clerkEnv.CLERK_PUBLISHABLE_KEY || '',
+ apiUrl = clerkEnv.CLERK_API_URL,
+ apiVersion = clerkEnv.CLERK_API_VERSION,
+ ...rest
+ } = options || {};🤖 Prompt for AI Agents
In `@packages/hono/src/clerkMiddleware.ts` around lines 38 - 43, The destructuring
currently uses options || {envDefaults} causing env defaults to be ignored when
options exists; change the merge to start from environment defaults and then
spread options so provided fields override env values (e.g., build an
envDefaults object from clerkEnv.CLERK_SECRET_KEY, CLERK_PUBLISHABLE_KEY,
CLERK_API_URL, CLERK_API_VERSION and then do { ...envDefaults, ...(options ||
{}) } before destructuring) so secretKey, publishableKey, apiUrl, and apiVersion
correctly fall back to environment values when not supplied in options.
| export interface KeylessResult { | ||
| publishableKey: string | undefined; | ||
| secretKey: string | undefined; | ||
| claimUrl: string | undefined; | ||
| apiKeysUrl: string | undefined; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for KeylessResult definitions
rg -n "interface KeylessResult" packages/shared/src/keyless/Repository: clerk/javascript
Length of output: 231
🏁 Script executed:
#!/bin/bash
# Also check for type exports of KeylessResult
rg -n "export.*KeylessResult" packages/shared/src/keyless/Repository: clerk/javascript
Length of output: 342
🏁 Script executed:
#!/bin/bash
# Check the imports in resolveKeysWithKeylessFallback.ts
head -20 packages/shared/src/keyless/resolveKeysWithKeylessFallback.tsRepository: clerk/javascript
Length of output: 950
🏁 Script executed:
#!/bin/bash
# Read the KeylessResult definition from service.ts to compare
sed -n '82,88p' packages/shared/src/keyless/service.tsRepository: clerk/javascript
Length of output: 232
🏁 Script executed:
#!/bin/bash
# Check if there are any imports of KeylessResult in service.ts from resolveKeysWithKeylessFallback
head -30 packages/shared/src/keyless/service.ts | grep -E "import|KeylessResult"Repository: clerk/javascript
Length of output: 219
🏁 Script executed:
#!/bin/bash
# Check what service.ts exports
rg -n "^export" packages/shared/src/keyless/service.ts | head -20Repository: clerk/javascript
Length of output: 337
🏁 Script executed:
#!/bin/bash
# Check index.ts to see full export structure
cat packages/shared/src/keyless/index.tsRepository: clerk/javascript
Length of output: 788
🏁 Script executed:
#!/bin/bash
# Check if KeylessResult from service.ts is actually used within service.ts
rg -n "KeylessResult" packages/shared/src/keyless/service.tsRepository: clerk/javascript
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Check if KeylessResult is used in resolveKeysWithKeylessFallback.ts (where it's defined)
rg -n "KeylessResult" packages/shared/src/keyless/resolveKeysWithKeylessFallback.tsRepository: clerk/javascript
Length of output: 125
Remove duplicate KeylessResult definition from service.ts.
KeylessResult is defined identically in both packages/shared/src/keyless/service.ts (line 82) and packages/shared/src/keyless/resolveKeysWithKeylessFallback.ts (line 5). The public API exports only the one from resolveKeysWithKeylessFallback, creating a maintenance risk. Consolidate by removing the duplicate from service.ts and importing the type instead to prevent type drift.
🤖 Prompt for AI Agents
In `@packages/shared/src/keyless/resolveKeysWithKeylessFallback.ts` around lines 5
- 10, There are two identical KeylessResult type definitions; remove the
duplicate in service.ts and import the exported KeylessResult from
resolveKeysWithKeylessFallback.ts instead: delete the KeylessResult interface in
service.ts, add an import for KeylessResult at the top of service.ts, and update
any local references in functions/methods in service.ts (e.g., places using
KeylessResult, function signatures, or return types) to use the imported type so
the project relies on the single canonical definition in
resolveKeysWithKeylessFallback.ts.
jacekradko
left a comment
jacekradko
left a comment
There was a problem hiding this comment.
Do we have a bad merge here? 424 files changed
… navigation TanStack Router doesn't parse query strings from the `to` parameter, expecting them in a separate `search` option. This was causing "Not Found" errors when Clerk navigated with URLs containing query parameters (e.g., `/sign-in?redirect_url=...`). The fix parses the URL string and separates pathname, search params, and hash before passing to TanStack Router's navigate function.
nikosdouvlis
force-pushed
the
nikos/fix-tanstack-navigations
branch
from
2a25ef1 to
d3ecfeb
Compare
…t intermittent redirect on navigation When navigating away from a page with SignIn/SignUp, useLocation() returns the new pathname before the component unmounts. This race condition causes the base path to change, which breaks internal route matching and fires the catch-all RedirectToSignIn, bouncing the user back to sign-in. Stabilize the computed base path with useRef so it's set once at mount time and doesn't change during navigation transitions. This is the same pattern already used in @clerk/nextjs usePathnameWithoutCatchAll.
nikosdouvlis
force-pushed
the
nikos/fix-tanstack-navigations
branch
from
d3ecfeb to
b66635a
Compare
Yup, lazygit rewrote all SHA's during rebase and threw the delta off, fixed! |
5 participants
Summary
TanStack Router's
navigate()function doesn't parse query strings from thetoparameter. It expects:to: pathname only (e.g.,/sign-in)search: object with query parameters (e.g.,{ redirect_url: '...' })hash: fragment string without#Previously, Clerk's
routerPushandrouterReplacewere passing the full URL string (e.g.,/sign-in?redirect_url=...) directly toto, which TanStack Router tried to match as a literal route name, causing "Not Found" errors.This affected satellite domain authentication flows and any Clerk navigation that includes query parameters.
Changes
parseUrlForNavigation()utility that extracts pathname, search params, and hash from a URL stringrouterPushandrouterReplacein ClerkProvider to use the new parsing functionTest plan
parseUrlForNavigationcovering various URL formatsSummary by CodeRabbit
Bug Fixes