Skip to content

refactor(router-core): loadMatches extra microtask #4967

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

Merged
merged 3 commits into from
Aug 16, 2025
Merged

refactor(router-core): loadMatches extra microtask #4967

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a61bc35
refactor(router-core): loadMatches extra microtask
Sheraff Aug 16, 2025
cb1f497
ci: apply automated fixes
autofix-ci[bot] Aug 16, 2025
f9e3daa
update store-updates test w/ lower number
Sheraff Aug 16, 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
39 changes: 14 additions & 25 deletions packages/router-core/src/router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2930,33 +2930,22 @@ export class RouterCore<
}

try {
await new Promise<void>((resolveAll, rejectAll) => {
;(async () => {
try {
// Execute all beforeLoads one by one
for (let i = 0; i < innerLoadContext.matches.length; i++) {
const beforeLoad = this.handleBeforeLoad(innerLoadContext, i)
if (isPromise(beforeLoad)) await beforeLoad
}

// Execute all loaders in parallel
const max =
innerLoadContext.firstBadMatchIndex ??
innerLoadContext.matches.length
for (let i = 0; i < max; i++) {
innerLoadContext.matchPromises.push(
this.loadRouteMatch(innerLoadContext, i),
)
}
// Execute all beforeLoads one by one
for (let i = 0; i < innerLoadContext.matches.length; i++) {
const beforeLoad = this.handleBeforeLoad(innerLoadContext, i)
if (isPromise(beforeLoad)) await beforeLoad
}

await Promise.all(innerLoadContext.matchPromises)
// Execute all loaders in parallel
const max =
innerLoadContext.firstBadMatchIndex ?? innerLoadContext.matches.length
for (let i = 0; i < max; i++) {
innerLoadContext.matchPromises.push(
this.loadRouteMatch(innerLoadContext, i),
)
}
await Promise.all(innerLoadContext.matchPromises)

resolveAll()
} catch (err) {
rejectAll(err)
}
})()
})
const readyPromise = this.triggerOnReady(innerLoadContext)
if (isPromise(readyPromise)) await readyPromise
} catch (err) {
Comment on lines +2939 to 2951
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Potential regression: statusCode may remain 200 when a loader errors (non-redirect/notFound)

Because runLoader catches loader errors and updates match.status = 'error' without throwing, Promise.all here will typically resolve. With this structure, load()’s catch block won’t run, so the statusCode update to 500 (based on any match with status === 'error') won’t occur. Previously, the outer wrapper likely caused a rejection to flow back, triggering that status update.

Recommend propagating a sentinel error after loaders complete when any match ended in error (and not in preload mode). This preserves the existing statusCode behavior in load().

Apply this diff inside the changed block:

       for (let i = 0; i < max; i++) {
         innerLoadContext.matchPromises.push(
           this.loadRouteMatch(innerLoadContext, i),
         )
       }
-      await Promise.all(innerLoadContext.matchPromises)
+      await Promise.all(innerLoadContext.matchPromises)
+
+      // If any match errored, propagate so the caller can set statusCode = 500
+      // (do not throw during preloads).
+      if (
+        !innerLoadContext.preload &&
+        innerLoadContext.matches.some((m) => m.status === 'error')
+      ) {
+        const readyPromise0 = this.triggerOnReady(innerLoadContext)
+        if (isPromise(readyPromise0)) await readyPromise0
+        throw new Error('__TSR_LOADER_ERROR__')
+      }
 
       const readyPromise = this.triggerOnReady(innerLoadContext)
       if (isPromise(readyPromise)) await readyPromise

Alternative (if you prefer not to throw): explicitly set statusCode after await this.loadMatches(...) in load(), based on any error matches. I can draft that patch if you want.

Run this script to confirm that statusCode=500 is only set from load()’s catch and not elsewhere:

🏁 Script executed:

#!/bin/bash
set -euo pipefail
# Where is statusCode assigned?
rg -nP -C3 '\bstatusCode\s*:' packages/router-core/src/router.ts

# Where do we throw/propagate generic loader errors from loadMatches?
rg -nP -C2 'loadMatches\s*=|Promise\.all|throw new Error|status === .error.' packages/router-core/src/router.ts

Length of output: 2597

🏁 Script executed:

#!/bin/bash
set -euo pipefail
# Show the load() method to inspect where loadMatches is called and statusCode logic
rg -nP -C20 'loadMatches' packages/router-core/src/router.ts

Length of output: 7788

🏁 Script executed:

#!/bin/bash
set -euo pipefail
# Show definition of handleRedirectAndNotFound to see statusCode logic
rg -nP -C20 'handleRedirectAndNotFound' packages/router-core/src/router.ts

Length of output: 10838

🏁 Script executed:

#!/bin/bash
set -euo pipefail
# Locate the start of load() and its catch block around statusCode updates
rg -nP "status:\s*'pending'" -C100 packages/router-core/src/router.ts

Length of output: 21738

🏁 Script executed:

#!/bin/bash
set -euo pipefail
# Extract the load() implementation and its surrounding catch for statusCode updates
sed -n '1870,2020p' packages/router-core/src/router.ts

Length of output: 4836

Ensure loader errors propagate to set statusCode = 500

Loader errors are currently swallowed by loadMatches – they mark match.status = 'error' but don’t throw, so load()’s catch never runs and statusCode stays 200. We need to re-throw after all loaders complete when any match errored (unless in preload) so the catch block can update statusCode.

Please apply this patch in packages/router-core/src/router.ts inside loadMatches (around lines 2939–2951):

       // Execute all loaders in parallel
       const max =
         innerLoadContext.firstBadMatchIndex ?? innerLoadContext.matches.length
       for (let i = 0; i < max; i++) {
         innerLoadContext.matchPromises.push(
           this.loadRouteMatch(innerLoadContext, i),
         )
       }
-      await Promise.all(innerLoadContext.matchPromises)
+      await Promise.all(innerLoadContext.matchPromises)
+
+      // Propagate a loader error so load()’s catch sets statusCode = 500
+      if (
+        !innerLoadContext.preload &&
+        innerLoadContext.matches.some((m) => m.status === 'error')
+      ) {
+        const ready = this.triggerOnReady(innerLoadContext)
+        if (isPromise(ready)) await ready
+        throw new Error('__TSR_LOADER_ERROR__')
+      }
 
       const readyPromise = this.triggerOnReady(innerLoadContext)
       if (isPromise(readyPromise)) await readyPromise

This will ensure the existing catch in load() runs and applies a 500 status when any loader fails.

📝 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
// Execute all loaders in parallel
const max =
innerLoadContext.firstBadMatchIndex ?? innerLoadContext.matches.length
for (let i = 0; i < max; i++) {
innerLoadContext.matchPromises.push(
this.loadRouteMatch(innerLoadContext, i),
)
}
await Promise.all(innerLoadContext.matchPromises)
resolveAll()
} catch (err) {
rejectAll(err)
}
})()
})
const readyPromise = this.triggerOnReady(innerLoadContext)
if (isPromise(readyPromise)) await readyPromise
} catch (err) {
// Execute all loaders in parallel
const max =
innerLoadContext.firstBadMatchIndex ?? innerLoadContext.matches.length
for (let i = 0; i < max; i++) {
innerLoadContext.matchPromises.push(
this.loadRouteMatch(innerLoadContext, i),
)
}
await Promise.all(innerLoadContext.matchPromises)
// Propagate a loader error so load()’s catch sets statusCode = 500
if (
!innerLoadContext.preload &&
innerLoadContext.matches.some((m) => m.status === 'error')
) {
const ready = this.triggerOnReady(innerLoadContext)
if (isPromise(ready)) await ready
throw new Error('__TSR_LOADER_ERROR__')
}
const readyPromise = this.triggerOnReady(innerLoadContext)
if (isPromise(readyPromise)) await readyPromise
} catch (err) {
🤖 Prompt for AI Agents 
In packages/router-core/src/router.ts around lines 2939–2951, loadMatches
currently runs all loader promises and sets match.status = 'error' on failures
but does not throw, so load()'s catch never runs; after awaiting
Promise.all(innerLoadContext.matchPromises) and before resolving readyPromise,
check innerLoadContext.matches (or matchPromises) for any match with status ===
'error' (and skip this check if innerLoadContext.isPreload or similar preload
flag), and if any errored, re-throw an Error (or re-throw the first captured
loader error) so the outer load() catch runs and statusCode can be set to 500.

Expand Down
Loading