Skip to content

fix(tanstack-react-start): Fix navigation promise resolving too early #7463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Ephem wants to merge 1 commit into from
+16 −8
Closed

fix(tanstack-react-start): Fix navigation promise resolving too early #7463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/mighty-hands-crash.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@clerk/tanstack-react-start': patch
---

Fix navigation edge case where the new auth state would sometimes be applied before the navigation finished.
19 changes: 11 additions & 8 deletions packages/tanstack-react-start/src/client/useAwaitableNavigate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import type { NavigateOptions } from '@tanstack/react-router';
import { useLocation, useNavigate } from '@tanstack/react-router';
import React, { useTransition } from 'react';
import React from 'react';

type Resolve = (value?: unknown) => void;
type Resolve = () => void;

export const useAwaitableNavigate = () => {
const navigate = useNavigate();
Expand All @@ -12,18 +12,21 @@ export const useAwaitableNavigate = () => {
resolveFunctionsRef.current.forEach(resolve => resolve());
resolveFunctionsRef.current.splice(0, resolveFunctionsRef.current.length);
};
const [_, startTransition] = useTransition();

React.useEffect(() => {
resolveAll();
}, [location]);

return (options: NavigateOptions) => {
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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 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();
});
});
Comment on lines 20 to 30
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the full file to understand the context
cat -n packages/tanstack-react-start/src/client/useAwaitableNavigate.ts

Repository: 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 -100

Repository: 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=jsx

Repository: 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 -20

Repository: 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 -30

Repository: clerk/javascript

Length of output: 289

🏁 Script executed:

#!/bin/bash
# Check the exports test file
cat packages/tanstack-react-start/src/__tests__/exports.test.ts

Repository: 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=ts

Repository: clerk/javascript

Length of output: 579

🏁 Script executed:

#!/bin/bash
# Check package index files for exports
cat packages/tanstack-react-start/src/client/index.ts

Repository: 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.tsx

Repository: 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 -30

Repository: 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 -10

Repository: 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.ts

Repository: 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.ts

Repository: clerk/javascript

Length of output: 1454

Critical: Double resolution and memory leak in navigation promise logic.

The promise resolution logic has a critical flaw:

  1. Memory leak: When navigate() completes, res() is called immediately (lines 25, 28), but the resolve function remains in the resolveFunctionsRef.current array. These functions are only removed when location changes (via resolveAll() at line 13). Rapid navigation calls will accumulate resolved functions in the array indefinitely until a location change occurs.

  2. Double resolution: The same res() function is called both when navigate() completes (lines 25, 28) and when location changes (line 12 via resolveAll()). While JavaScript ignores subsequent resolve() calls on an already-resolved promise, this indicates confused logic about when resolution should occur.

  3. 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 the useEffect.
    • Option B: If resolution should happen when navigate() completes, clean up the resolve function immediately after calling res():
    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.

Copy link
Contributor Author

@Ephem Ephem Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@coderabbitai coderabbitai bot Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

@Ephem Ephem Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.. 🤔

Copy link
Member

@wobsoriano wobsoriano Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ff.

thanks for checking this one out, frederik!

};
};
Loading