Skip to content

fix: serial head execution #6093

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

Open
schiller-manuel wants to merge 3 commits into
base: main
Choose a base branch
from
+48 −108
Open

fix: serial head execution #6093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8bb4816
fix: serial head execution
schiller-manuel Dec 13, 2025
f320bb0
fix types for TS 5.4
schiller-manuel Dec 17, 2025
921730e
remove testing for not supported throwing of `notFound()` in `head()`
schiller-manuel Dec 17, 2025
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
23 changes: 0 additions & 23 deletions e2e/react-start/basic/src/routes/not-found/via-head.tsx
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion e2e/react-start/basic/tests/not-found.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ test.describe('not-found', () => {
test.describe('throw notFound()', () => {
const navigationTestMatrix = combinate({
// TODO beforeLoad!
thrower: [/* 'beforeLoad',*/ 'head', 'loader'] as const,
thrower: [/* 'beforeLoad',*/ 'loader'] as const,
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 22, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, find the test file
if [ -f "e2e/react-start/basic/tests/not-found.spec.ts" ]; then
  echo "=== Test file found ===" 
  wc -l "e2e/react-start/basic/tests/not-found.spec.ts"
  echo ""
  echo "=== Lines 25-35 (around line 29) ==="
  sed -n '25,35p' "e2e/react-start/basic/tests/not-found.spec.ts"
  echo ""
  echo "=== Lines 55-70 (around line 61) ==="
  sed -n '55,70p' "e2e/react-start/basic/tests/not-found.spec.ts"
else
  echo "Test file not found at e2e/react-start/basic/tests/not-found.spec.ts"
  # Try to find it
  fd -t f "not-found.spec.ts" e2e/
fi

Repository: TanStack/router

Length of output: 1108

🏁 Script executed:

#!/bin/bash
# Search for via-head route files
echo "=== Searching for via-head route files ==="
fd -type f "via-head" e2e/
echo ""
echo "=== Also checking for any via-head references ==="
rg "via-head" e2e/ --type ts --type tsx -l

Repository: TanStack/router

Length of output: 368

🏁 Script executed:

#!/bin/bash
echo "=== Searching for not-found route files ==="
find e2e/react-start/basic/app/routes -name "*not-found*" -type f 2>/dev/null | head -20
echo ""
echo "=== Looking for via-* route files ==="
find e2e/react-start/basic/app/routes -name "via-*" -type f 2>/dev/null
echo ""
echo "=== List all routes in not-found directory ==="
find e2e/react-start/basic/app/routes/not-found -type f 2>/dev/null

Repository: TanStack/router

Length of output: 189

🏁 Script executed:

#!/bin/bash
# Check the actual test to see what routes it expects and what the tests do
echo "=== Full test file context (lines 15-77) ==="
sed -n '15,77p' "e2e/react-start/basic/tests/not-found.spec.ts"

Repository: TanStack/router

Length of output: 1983

🏁 Script executed:

#!/bin/bash
echo "=== Check if not-found directory exists ==="
ls -la e2e/react-start/basic/app/routes/ | grep -i "not-found" || echo "No not-found directory found"
echo ""
echo "=== List the entire routes directory structure ==="
find e2e/react-start/basic/app/routes -maxdepth 2 -type d | sort
echo ""
echo "=== Search for files containing 'via-' or 'notFound' in routes ==="
grep -r "via-" e2e/react-start/basic/app/routes --include="*.ts" --include="*.tsx" 2>/dev/null | head -10 || echo "No via- references found in routes"

Repository: TanStack/router

Length of output: 417

🏁 Script executed:

#!/bin/bash
echo "=== Check for any head function/method in test routes ==="
grep -r "head" e2e/react-start/basic/app/routes --include="*.ts" --include="*.tsx" -B2 -A2 2>/dev/null | head -40 || echo "No head references found"

Repository: TanStack/router

Length of output: 116

🏁 Script executed:

#!/bin/bash
echo "=== Find the actual e2e app directory structure ==="
find e2e/react-start/basic -maxdepth 3 -type d | head -20
echo ""
echo "=== List files in e2e/react-start/basic ==="
ls -la e2e/react-start/basic/

Repository: TanStack/router

Length of output: 1496

🏁 Script executed:

#!/bin/bash
echo "=== List all files in not-found directory ==="
find e2e/react-start/basic/src/routes/not-found -type f | sort
echo ""
echo "=== Content of not-found directory ==="
ls -la e2e/react-start/basic/src/routes/not-found/

Repository: TanStack/router

Length of output: 732

🏁 Script executed:

#!/bin/bash
echo "=== Check via-loader.tsx to understand the pattern ==="
cat e2e/react-start/basic/src/routes/not-found/via-loader.tsx
echo ""
echo "=== Check for any references to via-head in the entire codebase ==="
grep -r "via-head" e2e/react-start/basic --include="*.ts" --include="*.tsx" --include="*.js"

Repository: TanStack/router

Length of output: 2084

🏁 Script executed:

#!/bin/bash
echo "=== Check if via-head.tsx file actually exists ==="
if [ -f "e2e/react-start/basic/src/routes/not-found/via-head.tsx" ]; then
  echo "via-head.tsx EXISTS"
  cat e2e/react-start/basic/src/routes/not-found/via-head.tsx
else
  echo "via-head.tsx DOES NOT EXIST"
fi
echo ""
echo "=== Check the index.tsx to see the link structure ==="
cat e2e/react-start/basic/src/routes/not-found/index.tsx

Repository: TanStack/router

Length of output: 1193

Remove 'head' from directVisitTestMatrix to match navigationTestMatrix

The via-head.tsx route file does not exist in the not-found routes directory. The navigationTestMatrix (line 29) correctly excludes 'head' from the thrower options, but directVisitTestMatrix (line 61) still includes it. This will cause the direct visit test to fail when attempting to navigate to the non-existent /not-found/via-head route. Either restore the via-head.tsx route file or remove 'head' from directVisitTestMatrix to align with the navigation test configuration.

🤖 Prompt for AI Agents 
In e2e/react-start/basic/tests/not-found.spec.ts around lines 29 and 61, the
navigationTestMatrix correctly excludes 'head' but directVisitTestMatrix still
includes 'head', which points to a non-existent via-head.tsx route and will fail
direct-visit tests; fix by removing 'head' from the directVisitTestMatrix
thrower options (or alternatively restore the via-head.tsx route file if that
behavior is required) so both matrices match and no test attempts to visit
/not-found/via-head.

preload: [false, true] as const,
})

Expand Down
23 changes: 0 additions & 23 deletions e2e/solid-start/basic/src/routes/not-found/via-head.tsx
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion e2e/solid-start/basic/tests/not-found.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ test.describe('not-found', () => {
test.describe('throw notFound()', () => {
const navigationTestMatrix = combinate({
// TODO beforeLoad!
thrower: [/* 'beforeLoad',*/ 'head', 'loader'] as const,
thrower: [/* 'beforeLoad',*/ 'loader'] as const,
preload: [false, true] as const,
})

Expand Down
23 changes: 0 additions & 23 deletions e2e/vue-start/basic/src/routes/not-found/via-head.tsx
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion e2e/vue-start/basic/tests/not-found.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ test.describe('not-found', () => {
test.describe('throw notFound()', () => {
const navigationTestMatrix = combinate({
// TODO beforeLoad!
thrower: [/* 'beforeLoad',*/ 'head', 'loader'] as const,
thrower: [/* 'beforeLoad',*/ 'loader'] as const,
preload: [false, true] as const,
})

Expand Down
81 changes: 45 additions & 36 deletions packages/router-core/src/load-matches.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -683,8 +683,6 @@ const runLoader = async (
// so we need to wait for it to resolve before
// we can use the options
if (route._lazyPromise) await route._lazyPromise
const headResult = executeHead(inner, matchId, route)
const head = headResult ? await headResult : undefined
const pendingPromise = match._nonReactive.minPendingPromise
if (pendingPromise) await pendingPromise

Expand All @@ -697,7 +695,6 @@ const runLoader = async (
status: 'success',
isFetching: false,
updatedAt: Date.now(),
...head,
}))
} catch (e) {
let error = e
Expand All @@ -721,28 +718,17 @@ const runLoader = async (
onErrorError,
)
}
const headResult = executeHead(inner, matchId, route)
const head = headResult ? await headResult : undefined
inner.updateMatch(matchId, (prev) => ({
...prev,
error,
status: 'error',
isFetching: false,
...head,
}))
}
} catch (err) {
const match = inner.router.getMatch(matchId)
// in case of a redirecting match during preload, the match does not exist
if (match) {
const headResult = executeHead(inner, matchId, route)
if (headResult) {
const head = await headResult
inner.updateMatch(matchId, (prev) => ({
...prev,
...head,
}))
}
match._nonReactive.loaderPromise = undefined
}
handleRedirectAndNotFound(inner, match, err)
Expand All @@ -767,14 +753,6 @@ const loadRouteMatch = async (

if (shouldSkipLoader(inner, matchId)) {
if (inner.router.isServer) {
const headResult = executeHead(inner, matchId, route)
if (headResult) {
const head = await headResult
inner.updateMatch(matchId, (prev) => ({
...prev,
...head,
}))
}
return inner.router.getMatch(matchId)!
}
} else {
Expand Down Expand Up @@ -852,18 +830,6 @@ const loadRouteMatch = async (
})()
} else if (status !== 'success' || (loaderShouldRunAsync && inner.sync)) {
await runLoader(inner, matchId, index, route)
} else {
// if the loader did not run, still update head.
// reason: parent's beforeLoad may have changed the route context
// and only now do we know the route context (and that the loader would not run)
const headResult = executeHead(inner, matchId, route)
if (headResult) {
const head = await headResult
inner.updateMatch(matchId, (prev) => ({
...prev,
...head,
}))
}
}
}
}
Expand Down Expand Up @@ -931,7 +897,51 @@ export async function loadMatches(arg: {
for (let i = 0; i < max; i++) {
inner.matchPromises.push(loadRouteMatch(inner, i))
}
await Promise.all(inner.matchPromises)
// Use allSettled to ensure all loaders complete regardless of success/failure
const results = await Promise.allSettled(inner.matchPromises)

const failures = results
// TODO when we drop support for TS 5.4, we can use the built-in type guard for PromiseRejectedResult
.filter(
(result): result is PromiseRejectedResult =>
result.status === 'rejected',
)
.map((result) => result.reason)

// Find the first redirect error and throw it immediately (skip head execution)
for (const err of failures) {
if (isRedirect(err)) {
throw err
}
}

// serially execute head functions after all loaders have completed (successfully or not)
// Each head execution is wrapped in try-catch to ensure all heads run even if one fails
// TODO: should we break out of head execution on first failure?
for (const match of inner.matches) {
const { id: matchId, routeId } = match
const route = inner.router.looseRoutesById[routeId]!
try {
const headResult = executeHead(inner, matchId, route)
if (headResult) {
const head = await headResult
inner.updateMatch(matchId, (prev) => ({
...prev,
...head,
}))
}
} catch (err) {
// Log error but continue executing other head functions
console.error(`Error executing head for route ${routeId}:`, err)
}
}

// After head execution, check for notFound errors and throw the first one
for (const err of failures) {
if (isNotFound(err)) {
throw err
}
}
Comment on lines +939 to +944
Copy link
Contributor

@Sheraff Sheraff Dec 13, 2025

Choose a reason for hiding this comment

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

I find it a little curious that we call executeHead for children of a match that would have thrown a notFound

If we have /a/b/c and /b throws a notFound it its loader, should we really still execute the head of /c? It feels "leaky"


const readyPromise = triggerOnReady(inner)
if (isPromise(readyPromise)) await readyPromise
Expand All @@ -945,7 +955,6 @@ export async function loadMatches(arg: {
throw err
}
}

return inner.matches
}

Expand Down
Loading