Uh oh!
There was an error while loading. Please reload this page.
refactor(router-core): shouldExecuteBeforeLoad is always true - #4992
Merged
Conversation
Contributor
WalkthroughRefactors beforeLoad orchestration in load-matches.ts: consolidates server-check logic, renames and changes signature of a precondition function, replaces boolean gating with promise-based setup and execution, simplifies redirect/notFound handling during preload, and maintains SSR semantics while altering control flow to use the new pre-execution setup. Changes
Sequence Diagram(s)sequenceDiagram
participant Router
participant loadMatches
participant preBeforeLoadSetup
participant execute as execute(beforeLoad)
participant Redirect as handleRedirectAndNotFound
Router->>loadMatches: start load
loadMatches->>preBeforeLoadSetup: prepare (SSR checks, preload setup)
preBeforeLoadSetup-->>loadMatches: resolve (no boolean)
loadMatches->>execute: run beforeLoad
execute-->>loadMatches: result (status)
alt preload and redirected/notFound
loadMatches->>Redirect: handle redirect/notFound
end
loadMatches-->>Router: done
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 ( |
Contributor
View your CI Pipeline Execution ↗ for commit 0dcac36
☁️ 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-adaptercommit: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/router-core/src/load-matches.ts (2)
331-335: Optional: also wait for an ongoing loader when coordinating with previous workRight now you only await a previous beforeLoad. If a prior in-flight loader (without a beforeLoad) is the thing that sets redirected/notFound, the then() check may run too early to observe that status. If that’s not intentional, consider awaiting the loader as well:
- // Wait for the previous beforeLoad to resolve before we continue- return existingMatch._nonReactive.beforeLoadPromise- ? existingMatch._nonReactive.beforeLoadPromise.then(then)- : then()+ // Wait for prior beforeLoad (or, if present, an ongoing loader) before continuing+ const p =+ existingMatch._nonReactive.beforeLoadPromise ??+ existingMatch._nonReactive.loaderPromise+ return p ? p.then(then) : then()Please confirm the intended semantics: is it acceptable for a new beforeLoad to proceed without waiting for an in-flight loader from a previous cycle?
492-494: Nit: define execute before it’s referenced to reduce cognitive loadqueueExecution references execute; while the call site order currently ensures execute is defined before queueExecution runs, placing execute first avoids accidental TDZ bugs if this block is refactored later.
- const queueExecution = () => {- if (shouldSkipLoader(inner, matchId)) return- const result = preBeforeLoadSetup(inner, matchId, route)- return isPromise(result) ? result.then(execute) : execute()- }-- const execute = () => executeBeforeLoad(inner, matchId, index, route)+ const execute = () => executeBeforeLoad(inner, matchId, index, route)++ const queueExecution = () => {+ if (shouldSkipLoader(inner, matchId)) return+ const result = preBeforeLoadSetup(inner, matchId, route)+ return isPromise(result) ? result.then(execute) : execute()+ }Also applies to: 496-497
📜 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(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/router-core/src/load-matches.ts (2)
packages/router-core/src/index.ts (1)
AnyRoute(163-163)packages/router-core/src/utils.ts (1)
isPromise(467-475)
⏰ 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 (2)
packages/router-core/src/load-matches.ts (2)
150-152: Simplified server-side skip condition looks correctCombining the server and ssr=false checks into a single condition preserves behavior and is easier to read. The 'data-only' case remains unaffected.
304-309: Renaming and signature change improve intent claritySwitching from shouldExecuteBeforeLoad to preBeforeLoadSetup and returning void | Promise matches the new orchestration where execution is unconditional post-setup. Early-return when no in-flight load is also correct.
Comment on lines
322
to
329
| const match = inner.router.getMatch(matchId)! | ||
| if (match.status === 'error') { | ||
| result = true | ||
| } else if ( | ||
| if ( | ||
| match.preload && | ||
| (match.status === 'redirected' || match.status === 'notFound') | ||
| ) { | ||
| handleRedirectAndNotFound(inner, match, match.error) | ||
| } | ||
| return result | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against missing match after preload redirect to avoid runtime crash
In preload flows, a prior beforeLoad/loader can redirect and the match may no longer exist in router.state. Accessing match.preload/status via a non-null assertion can crash at runtime.
Apply a defensive check before dereferencing:
- const match = inner.router.getMatch(matchId)!- if (+ const match = inner.router.getMatch(matchId)+ if (!match) return+ if (
match.preload &&
(match.status === 'redirected' || match.status === 'notFound')
) {
handleRedirectAndNotFound(inner, match, match.error)
}I can add a regression test that preloads a route whose beforeLoad redirects, ensuring no crash here. Want me to open a follow-up?
📝 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
| constmatch=inner.router.getMatch(matchId)! | |
| if(match.status==='error'){ | |
| result=true | |
| }elseif( | |
| if( | |
| match.preload&& | |
| (match.status==='redirected'||match.status==='notFound') | |
| ){ | |
| handleRedirectAndNotFound(inner,match,match.error) | |
| } | |
| returnresult | |
| } | |
| constmatch=inner.router.getMatch(matchId) | |
| if(!match)return | |
| if( | |
| match.preload&& | |
| (match.status==='redirected'||match.status==='notFound') | |
| ){ | |
| handleRedirectAndNotFound(inner,match,match.error) | |
| } |
🤖 Prompt for AI Agents
In packages/router-core/src/load-matches.ts around lines 322 to 329, the code
dereferences match with a non-null assertion after getting it from
inner.router.getMatch(matchId), which can crash if a prior beforeLoad/loader
redirected and removed the match; add a defensive check for match existence and
only proceed to check match.preload and match.status if match is defined (e.g.,
if (!match) continue/return), otherwise skip calling handleRedirectAndNotFound;
update control flow so handleRedirectAndNotFound is only invoked when match !==
undefined.
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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fix#4971 (comment)
With the code now simplified by previous PRs, we noticed that the
shouldExecuteBeforeLoadguard is always true, so we can clean it up.In a follow up PR, we'll revise the conditions under which a
beforeLoadshould be called or skipped.Summary by CodeRabbit
Refactor
Bug Fixes