refactor(router-core): loadMatches extra microtask #4967
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughRefactors loadMatches in Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Router
participant Matches
participant ReadySignal
App->>Router: navigate()
Router->>Router: loadMatches()
loop for each match
Router->>Matches: await beforeLoad(match) (sequential)
end
Router->>Matches: start loaders (parallel, collect promises)
Router->>Router: await Promise.all(loaderPromises)
Router->>ReadySignal: trigger ready()
Router->>ReadySignal: await readyPromise
Router-->>App: resolve navigation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
View your CI Pipeline Execution ↗ for commit f9e3daa
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-plugin
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-plugin
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-server-functions-client
@tanstack/start-server-functions-fetcher
@tanstack/start-server-functions-server
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
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: 1
🧹 Nitpick comments (1)
packages/router-core/src/router.ts (1)
2952-2960: Consider surfacing non-special errors for easier debuggingThe catch branch only rethrows Redirect/NotFound. Any other unexpected error reaching here is swallowed. While most errors are handled earlier, adding a dev-only console.error (or rethrow) would aid debugging anomalous cases without changing production behavior.
For example:
} catch (err) { if (isNotFound(err) && !innerLoadContext.preload) { const readyPromise = this.triggerOnReady(innerLoadContext) if (isPromise(readyPromise)) await readyPromise throw err } if (isRedirect(err)) { throw err } + if (process.env.NODE_ENV !== 'production') { + // Unexpected error escaping internal handlers + console.error(err) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/router-core/src/router.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/router-core/src/router.ts (1)
packages/router-core/src/utils.ts (1)
isPromise(477-485)
⏰ 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). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (1)
packages/router-core/src/router.ts (1)
2933-2937: Clearer sequential beforeLoad execution — LGTMThe explicit loop to run beforeLoad hooks sequentially is straightforward and preserves ordering guarantees.
Comment on lines
+2939
to
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) | ||
|
|
||
| resolveAll() | ||
| } catch (err) { | ||
| rejectAll(err) | ||
| } | ||
| })() | ||
| }) | ||
| const readyPromise = this.triggerOnReady(innerLoadContext) | ||
| if (isPromise(readyPromise)) await readyPromise | ||
| } catch (err) { |
There was a problem hiding this comment.
💡 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 readyPromiseAlternative (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.tsLength 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.tsLength 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.tsLength 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.tsLength 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.tsLength 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 readyPromiseThis 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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/react-router/tests/store-updates-during-navigation.test.tsx (1)
186-186: Add a short rationale to future-proof the magic number.A brief inline note can help avoid churn if scheduling behavior changes again.
- expect(updates).toBe(7) + // After removing the extra microtask in loadMatches, this case deterministically yields 7 updates. + expect(updates).toBe(7)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/react-router/tests/store-updates-during-navigation.test.tsx(1 hunks)
⏰ 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). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (1)
packages/react-router/tests/store-updates-during-navigation.test.tsx (1)
186-186: Adjusted expectation to 7 updates is consistent with the loadMatches microtask removal — LGTMThe reduced update count matches the simplified scheduling in router-core and keeps enforcing the “no extra updates” contract.
This was referenced Aug 16, 2025
Open
tannerlinsley
pushed a commit
that referenced
this pull request
Aug 26, 2025
The `loadMatches` has an unnecessary complicated async setup, probably
due to how complicated the function was before recent cleanups:
```ts
async function loadMatches() {
try {
await new Promise((resolve, reject) => {
;(async () => {
try {
// the logic
resolve()
} catch (err) {
reject(err)
}
})()
})
// after promise
} catch (err) {
// error handling
}
}
```
Aside from some scheduling differences due to unnecessary promises in
the above example, this can be simplified down to this:
```ts
async function loadMatches() {
try {
// the logic
// after promise
} catch (err) {
// error handling
}
}
```
This is what this PR does.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- New Features
- No user-facing changes.
- Refactor
- Simplified internal async flow for route loading, removing redundant
promise wrapping.
- Maintains existing behavior and public API; no action required from
users.
- More consistent error handling and slightly reduced overhead during
parallel loads.
- Improves maintainability and prepares the codebase for future
enhancements.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
loadMatcheshas an unnecessary complicated async setup, probably due to how complicated the function was before recent cleanups:Aside from some scheduling differences due to unnecessary promises in the above example, this can be simplified down to this:
This is what this PR does.
Summary by CodeRabbit
New Features
Refactor