fix(tanstack-react-start): Fix navigation promise resolving too early #7463
Conversation
🦋 Changeset detectedLatest commit: 8b3574c 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughA patch release fixes a navigation edge case in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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/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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 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 (2)
.changeset/mighty-hands-crash.md(1 hunks)packages/tanstack-react-start/src/client/useAwaitableNavigate.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
All code must pass ESLint checks with the project's configuration
Files:
packages/tanstack-react-start/src/client/useAwaitableNavigate.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/tanstack-react-start/src/client/useAwaitableNavigate.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/tanstack-react-start/src/client/useAwaitableNavigate.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Follow established naming conventions (PascalCase for components, camelCase for variables)
Files:
packages/tanstack-react-start/src/client/useAwaitableNavigate.ts
packages/**/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
packages/**/src/**/*.{ts,tsx,js,jsx}: Maintain comprehensive JSDoc comments for public APIs
Use tree-shaking friendly exports
Validate all inputs and sanitize outputs
All public APIs must be documented with JSDoc
Use dynamic imports for optional features
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
Implement proper logging with different levels
Files:
packages/tanstack-react-start/src/client/useAwaitableNavigate.ts
**/*.ts?(x)
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
Files:
packages/tanstack-react-start/src/client/useAwaitableNavigate.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{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
Implement type guards forunknowntypes using the patternfunction isType(value: unknown): value is Type
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details in classes
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Use mixins for shared behavior across unrelated classes in TypeScript
Use generic constraints with bounded type parameters like<T extends { id: string }>
Use utility types likeOmit,Partial, andPickfor data transformation instead of manual type construction
Use discriminated unions instead of boolean flags for state management and API responses
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation at the type level
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Document functions with JSDoc comments including @param, @returns, @throws, and @example tags
Create custom error classes that extend Error for specific error types
Use the Result pattern for error handling instead of throwing exceptions
Use optional chaining (?.) and nullish coalescing (??) operators for safe property access
Let TypeScript infer obvious types to reduce verbosity
Useconst assertionswithas constfor literal types
Usesatisfiesoperator for type checking without widening types
Declare readonly arrays and objects for immutable data structures
Use spread operator and array spread for immutable updates instead of mutations
Use lazy loading for large types...
Files:
packages/tanstack-react-start/src/client/useAwaitableNavigate.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Use ESLint with custom configurations tailored for different package types
Files:
packages/tanstack-react-start/src/client/useAwaitableNavigate.ts
**/*.{js,ts,jsx,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Use Prettier for code formatting across all packages
Files:
packages/tanstack-react-start/src/client/useAwaitableNavigate.ts
⏰ 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). (25)
- GitHub Check: Integration Tests (handshake:staging, chrome)
- GitHub Check: Integration Tests (quickstart, chrome, 15)
- GitHub Check: Integration Tests (billing, chrome, RQ)
- GitHub Check: Integration Tests (nextjs, chrome, 16, RQ)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 16)
- GitHub Check: Integration Tests (quickstart, chrome, 16)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (machine, chrome)
- GitHub Check: Integration Tests (machine, chrome, RQ)
- GitHub Check: Integration Tests (custom, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Integration Tests (handshake, chrome)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (sessions:staging, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
.changeset/mighty-hands-crash.md (1)
1-5: LGTM! Changeset is well-formed.The changeset format is correct, the patch version bump is appropriate for a bug fix, and the description clearly communicates the fix to users.
| resolveAll(); | ||
| }, [location]); | ||
|
|
||
| return (options: NavigateOptions) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type annotation.
Per coding guidelines, functions should have explicit return types, especially for public APIs. The returned function should explicitly declare its return type.
As per coding guidelines: "Always define explicit return types for functions, especially public APIs."
- return (options: NavigateOptions) => {
+ return (options: NavigateOptions): Promise<void> => {📝 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.
| return (options: NavigateOptions) => { | |
| return (options: NavigateOptions): Promise<void> => { |
🤖 Prompt for AI Agents
In packages/tanstack-react-start/src/client/useAwaitableNavigate.ts around line
20, the returned function currently lacks an explicit return type; update the
signature to explicitly annotate the return type (e.g., change "return (options:
NavigateOptions) => {" to "return (options: NavigateOptions) => Promise<void> {"
or the correct concrete return type used by the implementation) so the public
API has a clear, explicit return-type annotation.
| return (options: NavigateOptions) => { | ||
| return new Promise(res => { | ||
| startTransition(() => { | ||
| resolveFunctionsRef.current.push(res); | ||
| res(navigate(options)); | ||
| }); | ||
| return new Promise<void>(res => { | ||
| resolveFunctionsRef.current.push(res); | ||
| navigate(options) | ||
| .then(() => { | ||
| res(); | ||
| }) | ||
| .catch(() => { | ||
| res(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the full file to understand the context
cat -n packages/tanstack-react-start/src/client/useAwaitableNavigate.tsRepository: clerk/javascript
Length of output: 1146
🏁 Script executed:
#!/bin/bash
# Search for tests related to useAwaitableNavigate
find . -type f \( -name "*.test.*" -o -name "*.spec.*" \) -exec rg -l "useAwaitableNavigate" {} \;Repository: clerk/javascript
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for usage of useAwaitableNavigate in the codebase
rg -n "useAwaitableNavigate" --type=ts --type=tsx -B2 -A2 | head -100Repository: clerk/javascript
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Search for usage patterns with broader search
rg "useAwaitableNavigate" -A 5 -B 5 --type=ts --type=js --type=jsxRepository: clerk/javascript
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Look for any test files in the tanstack-react-start package
find packages/tanstack-react-start -type f \( -name "*.test.*" -o -name "*.spec.*" \) 2>/dev/null | head -20Repository: clerk/javascript
Length of output: 198
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or git history that explains the design
rg -B 10 "useAwaitableNavigate" packages/tanstack-react-start/src/client/useAwaitableNavigate.ts | head -30Repository: clerk/javascript
Length of output: 289
🏁 Script executed:
#!/bin/bash
# Check the exports test file
cat packages/tanstack-react-start/src/__tests__/exports.test.tsRepository: clerk/javascript
Length of output: 1115
🏁 Script executed:
#!/bin/bash
# Search for actual usage in source files (avoiding test files)
rg "useAwaitableNavigate" packages/tanstack-react-start/src --type-add='tsx:*.tsx' --type=tsRepository: clerk/javascript
Length of output: 579
🏁 Script executed:
#!/bin/bash
# Check package index files for exports
cat packages/tanstack-react-start/src/client/index.tsRepository: clerk/javascript
Length of output: 193
🏁 Script executed:
#!/bin/bash
# Read ClerkProvider to understand how useAwaitableNavigate is used
cat -n packages/tanstack-react-start/src/client/ClerkProvider.tsxRepository: clerk/javascript
Length of output: 2843
🏁 Script executed:
#!/bin/bash
# Verify if navigate() actually triggers location change or if they're separate
rg -A 20 "navigate\(" packages/tanstack-react-start/src/client/useAwaitableNavigate.ts | head -30Repository: clerk/javascript
Length of output: 201
🏁 Script executed:
#!/bin/bash
# Check if there are any issues or PRs mentioning this hook
git log --oneline --all packages/tanstack-react-start/src/client/useAwaitableNavigate.ts 2>/dev/null | head -10Repository: clerk/javascript
Length of output: 227
🏁 Script executed:
#!/bin/bash
# Check the details of the commit that supposedly fixed this issue
git show 8b3574c5 --stat packages/tanstack-react-start/src/client/useAwaitableNavigate.tsRepository: clerk/javascript
Length of output: 380
🏁 Script executed:
#!/bin/bash
# Get full diff of that commit for the file
git show 8b3574c5 packages/tanstack-react-start/src/client/useAwaitableNavigate.tsRepository: clerk/javascript
Length of output: 1454
Critical: Double resolution and memory leak in navigation promise logic.
The promise resolution logic has a critical flaw:
-
Memory leak: When
navigate()completes,res()is called immediately (lines 25, 28), but the resolve function remains in theresolveFunctionsRef.currentarray. These functions are only removed whenlocationchanges (viaresolveAll()at line 13). Rapid navigation calls will accumulate resolved functions in the array indefinitely until a location change occurs. -
Double resolution: The same
res()function is called both whennavigate()completes (lines 25, 28) and whenlocationchanges (line 12 viaresolveAll()). While JavaScript ignores subsequentresolve()calls on an already-resolved promise, this indicates confused logic about when resolution should occur. -
Unclear intent: The implementation resolves on both
navigate()completion and location change, which seems contradictory. Clarify the intended behavior:- Option A: If resolution should happen when location changes, remove the
.then()and.catch()handlers (lines 24-29) and rely solely on theuseEffect. - Option B: If resolution should happen when
navigate()completes, clean up the resolve function immediately after callingres():
- Option A: If resolution should happen when location changes, remove the
return new Promise<void>(res => {
+ const index = resolveFunctionsRef.current.push(res) - 1;
- resolveFunctionsRef.current.push(res);
navigate(options)
.then(() => {
res();
+ resolveFunctionsRef.current.splice(index, 1);
})
.catch(() => {
res();
+ resolveFunctionsRef.current.splice(index, 1);
});
});Note: Option B assumes sequential resolution, which may not hold with concurrent navigations.
🤖 Prompt for AI Agents
In packages/tanstack-react-start/src/client/useAwaitableNavigate.ts around lines
20-30, the current implementation pushes resolve functions into
resolveFunctionsRef.current then also calls res() in the navigate().then/.catch
handlers, causing both a memory leak (resolved functions remain in the array
until location changes) and double-resolution; fix by choosing one resolution
strategy—recommended Option A: remove the .then() and .catch() handlers entirely
so the promise resolves only via the existing resolveAll() in the
location-change effect (ensure resolveAll clears the array after invoking each
resolver); alternatively, if you prefer resolution when navigate() completes
(Option B), then after calling res() immediately remove that resolve function
from resolveFunctionsRef.current (and guard so each resolver is invoked only
once) to avoid accumulation and duplicate calls.
There was a problem hiding this comment.
@wobsoriano @octoper I think this comment makes a lot of sense and articulates something I've been struggling with as well. Feels like we should either rely on the useEffect approach, or awaiting the navigate one, but using both feels like it makes things hard to reason about. 🤔
Because of #7411 I'm leaning towards the useEffect solution that we are using in other packages too. This should guarantee the page has fully mounted before we emit the new navigation state, which seems safer. When navigate resolves is out of our control, determined by the framework, and could even change subtly in the future.
What do you think?
I'll verify if removing the resolving on navigate still fixes the tests in my branch.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Turns out that wont work well either, because in TanStack Start, location does not seem to change if navigating to the same url you are already on, so promise never resolves in that case.. 🤔
There was a problem hiding this comment.
ff.
thanks for checking this one out, frederik!
|
I think |
Description
While debugging why the TanStack Start tests were failing in my branch that refactors to
useSyncExternalStoreI found that theuseAwaitableNavigatewas resolving before the navigation finished. This combined with timing changes in my branch that should have been inconsequential, were breaking the tests.I am still not a 100% sure of the reason. This PR essentially turns this:
to this:
which fixes the tests, but since
awaitunwraps nested promises, the original code should have been working. This might be a timing thing where the extra.thenmakes things resolve slightly later. 🤔 I will try to get to the bottom of this.I also removed the
startTransitionthat wraps the whole thing, because I did not see the point of it. Using a transition or not for the navigation should be a framework level concern, not a Clerk one. I'd be happy for pushback on this decision, but from my (albeit somewhat limited) testing, things seem to behave the same way before and after.The reason removing
startTransitionmight be important (now or in the future) is that if the framework wants to do something synchronously before the transition starts, it currently can't since we are wrapping it.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.