refactor(router-core): fewer getMatch calls #4971
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 load-matches.ts to streamline match access, per-match state, and loader orchestration; adjusts NotFound/error handling; updates onReady to a non-async callback; removes unused imports; and revises SSR gating and head data updates while retaining the public loadMatches export. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant loadMatches
participant Match
participant BeforeLoad
participant Loader
participant UpdateMatch
participant onReady
Caller->>loadMatches: invoke({ matches, preload?, onReady?, sync? })
loop for each Match
loadMatches->>Match: evaluate shouldExecuteBeforeLoad()
alt should execute
loadMatches->>BeforeLoad: run beforeLoad
loadMatches->>Match: setupPendingTimeout(match)
end
alt loaderPromise in-flight
loadMatches-->>Match: early return (reuse promise)
else SSR gated or no reload
loadMatches->>UpdateMatch: update head/flags (no loader run)
else should reload (SWR)
loadMatches->>Loader: run loader (async)
Loader-->>UpdateMatch: commit data/error
end
end
alt onReady provided
loadMatches->>onReady: call () => void
end
loadMatches-->>Caller: resolved matches
sequenceDiagram
participant loadMatches
participant Matches
participant NotFound
participant UpdateMatch
loadMatches->>Matches: process list
alt NotFound route encountered
loadMatches->>NotFound: resolve via matches.find(routeId===cursor.id)
Note over loadMatches,NotFound: Trigger readiness without awaiting in some cases
NotFound-->>UpdateMatch: mark error/not found
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 0ec2d48
☁️ 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: 0
🧹 Nitpick comments (6)
packages/router-core/src/load-matches.ts (6)
75-83: Guard against missing match when handling NotFound (fallback to root match).If
err.routeIdtargets a route that isn’t present ininner.matches(e.g., it's not part of the matched chain for this location), the invariant will throw. Consider falling back to the root route’s match to avoid crashing and still display the notFound component.- // Find the match for this route - const matchForRoute = inner.matches.find((m) => m.routeId === routeCursor.id) - - invariant(matchForRoute, 'Could not find match for route: ' + routeCursor.id) + // Find the match for this route (fallback to root route's match) + let matchForRoute = inner.matches.find((m) => m.routeId === routeCursor.id) + if (!matchForRoute) { + matchForRoute = inner.matches.find((m) => m.routeId === rootRouteId) + } + + invariant(matchForRoute, 'Could not find match for route: ' + routeCursor.id)
276-303: Passing the match into setupPendingTimeout is a solid cleanup.Nice reduction of lookups and clearer ownership of the
pendingTimeout. Consider extracting theshouldPendingcondition into a helper for readability and testability, but not required.
320-340: Simplify: then() always returns true; drop dead code and reduce re-reads.
resultis alwaystrue, so the conditional path doesn’t influence the return value. You can streamline to reduce noise and an extragetMatchcall.- const then = () => { - let result = true - const match = inner.router.getMatch(matchId)! - if (match.status === 'error') { - result = true - } else if ( - match.preload && - (match.status === 'redirected' || match.status === 'notFound') - ) { - handleRedirectAndNotFound(inner, match, match.error) - } - return result - } + const then = () => { + const match = inner.router.getMatch(matchId)! + if ( + match.preload && + (match.status === 'redirected' || match.status === 'notFound') + ) { + handleRedirectAndNotFound(inner, match, match.error) + } + return true + }
401-409: Avoid bumping fetchCount for routes without beforeLoad.Even though updates are batched, calling
pending()here increasesfetchCountand transiently setsisFetchingfor a code path withoutbeforeLoad. That can skew metrics/UI logic.If you adopt this, ensure
abortControlleris initialized elsewhere before a loader runs (sincepending()currently sets it).- if (!route.options.beforeLoad) { - batch(() => { - pending() - resolve() - }) - return - } + if (!route.options.beforeLoad) { + // No beforeLoad: avoid fetchCount bump and transient beforeLoad fetching + inner.updateMatch(matchId, (prev) => ({ + ...prev, + isFetching: false, + })) + return + }
760-829: SWR orchestration looks good; tiny clarity improvement for shouldReload.The SWR path is thoughtfully laid out. Minor readability nit: avoid nested ternary and redundant
getLoaderContextinvocation whenshouldReloadis a function.- const shouldReload = - typeof shouldReloadOption === 'function' - ? shouldReloadOption(getLoaderContext(inner, matchId, index, route)) - : shouldReloadOption + let shouldReload: boolean | undefined + if (typeof shouldReloadOption === 'function') { + const ctx = getLoaderContext(inner, matchId, index, route) + shouldReload = shouldReloadOption(ctx) + } else { + shouldReload = shouldReloadOption + }
600-606: Typo in comment: double apostrophe in "we''ll".Minor doc fix.
- // a minimum duration, we''ll wait for it to resolve + // a minimum duration, we'll wait for it to resolve
📜 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/load-matches.ts(14 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/router-core/src/load-matches.ts (1)
packages/router-core/src/index.ts (3)
AnyRouteMatch(90-90)createControlledPromise(279-279)isRedirect(373-373)
⏰ 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 (6)
packages/router-core/src/load-matches.ts (6)
242-259: LGTM: SSR context now reuses per-match state.Using
existingMatchto build the SSR context reduces re-fetches and keeps the flow coherent. Looks good.
507-513: LGTM: Clear execution gate for beforeLoad.The
executeclosure is concise and the condition aligns with the upstream decision.
581-582: AbortController wiring: verify initialization when beforeLoad is absent.
getLoaderContextpullsabortControllerfrom the match. With the optimization above (if applied), ensure it’s still set for loader consumers in routes withoutbeforeLoad. If not, consider initializing it in the loader path.
607-614: LGTM: SSR-gated chunk preloading respects per-match ssr.Good call to gate
loadRouteChunkwithmatch.ssron the server and allow on the client.
746-756: LGTM: Early-return when a non-preload in-flight load has fresh data.Returning the current successful match without blocking fits the stale-while-revalidate strategy and avoids unnecessary waiting when
syncis false.
630-631: minPendingPromise lifecycle verified as correct.
- Promises are created in both client (
packages/react-router/src/Match.tsx&packages/solid-router/src/Match.tsx) and SSR hydration (packages/router-core/src/ssr/ssr-client.ts) wheneverpendingMinMsis set.- Each promise is always resolved by its
setTimeouthandler and immediately cleared (minPendingPromise = undefined).- In
load-matches.ts, any existing promise is awaited before proceeding or error handling, ensuring no accidental long waits or dangling promises.No changes required.
dsonet
reviewed
Aug 18, 2025
Comment on lines
322
to
334
| const then = () => { | ||
| let shouldExecuteBeforeLoad = true | ||
| let result = true | ||
| const match = inner.router.getMatch(matchId)! | ||
| if (match.status === 'error') { | ||
| shouldExecuteBeforeLoad = true | ||
| result = true | ||
| } else if ( | ||
| match.preload && | ||
| (match.status === 'redirected' || match.status === 'notFound') | ||
| ) { | ||
| handleRedirectAndNotFound(inner, match, match.error) | ||
| } | ||
| return shouldExecuteBeforeLoad | ||
| return result | ||
| } |
There was a problem hiding this comment.
Damn you're right. The PR that made this change is this one though: #4961. I'll try and see if I can track down the original logic, but if you know what it's supposed to be already, I'm all ears!
There was a problem hiding this comment.
Actually it looks like it's been always true since this older PR https://github.com/TanStack/router/pull/4550/files (2 months ago). So I think we can at least say that it's not causing things to break. But maybe it is sub-optimal and we're executing too many beforeLoad calls. @schiller-manuel do you remember this a little?
Sheraff
added a commit
that referenced
this pull request
Aug 19, 2025
Fix #4971 (comment) With the code now simplified by previous PRs, we noticed that the `shouldExecuteBeforeLoad` guard is always true, so we can clean it up. In a follow up PR, we'll revise the conditions under which a `beforeLoad` should be called or skipped. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Streamlined pre-load checks and execution flow, reducing branching and improving consistency across client and server rendering. * Unified orchestration of the before-load phase to simplify control flow and improve maintainability. * **Bug Fixes** * More reliable handling of redirects and “not found” states during preloading, preventing unintended execution paths and improving navigation stability. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
tannerlinsley
pushed a commit
that referenced
this pull request
Aug 26, 2025
some miscellaneous minor optimizations in the `loadMatches` pipeline. - try and reduce the number of times we call `getMatch` - ~~`onReady` doesn't need to return a promise (because it's never used as such)~~ actually some things fail without the artificially added microtask here - don't create `beforeLoadPromise` if there is no `beforeLoad` option anyway <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - Bug Fixes - Improved NotFound and error handling to prevent unnecessary blocking and ensure readiness triggers correctly. - More reliable SSR and preload behavior across route transitions. - Refactor - Streamlined route loading with per-route stale-while-revalidate, reducing redundant work and improving navigation responsiveness. - Reduced internal lookups and clarified readiness semantics for more predictable loading states. - Public API - onReady callback is now synchronous (no Promise), aligning with updated readiness flow. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
tannerlinsley
pushed a commit
that referenced
this pull request
Aug 26, 2025
Fix #4971 (comment) With the code now simplified by previous PRs, we noticed that the `shouldExecuteBeforeLoad` guard is always true, so we can clean it up. In a follow up PR, we'll revise the conditions under which a `beforeLoad` should be called or skipped. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Streamlined pre-load checks and execution flow, reducing branching and improving consistency across client and server rendering. * Unified orchestration of the before-load phase to simplify control flow and improve maintainability. * **Bug Fixes** * More reliable handling of redirects and “not found” states during preloading, preventing unintended execution paths and improving navigation stability. <!-- 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.
some miscellaneous minor optimizations in the
loadMatchespipeline.getMatchonReadydoesn't need to return a promise (because it's never used as such)actually some things fail without the artificially added microtask here
beforeLoadPromiseif there is nobeforeLoadoption anywaySummary by CodeRabbit
Bug Fixes
Refactor
Public API