refactor(router-core): flatten loadRouteMatch, can run synchronously #5018
Conversation
WalkthroughRefactors router-core load flow to use an Awaitable-based pattern. Replaces several awaits with promise chaining, adds settleLoadRouteMatch() for centralized finalization, updates type annotations, and reorganizes preload/stale-while-revalidate handling. Server mode head handling now chains via then(). No exported/public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Router as Router
participant Load as loadMatches
participant Match as loadRouteMatch
participant Final as settleLoadRouteMatch
Caller->>Router: navigate()/preload()
Router->>Load: loadMatches(matches)
loop for each route match
Load->>Match: loadRouteMatch(match, ctx)
alt loader already running
Match-->>Load: loaderPromise.then(updatedMatch)
else new load
alt server mode (head)
Match-->>Load: headResult.then(updateMatch)
else client mode
Match-->>Load: Awaitable<AnyRouteMatch>
end
end
Load->>Final: settleLoadRouteMatch(match, state)
Final-->>Load: match (finalized)
end
Load-->>Router: Array<Awaitable<AnyRouteMatch>>
Router-->>Caller: resolution when each settles
note over Final,Load: Centralizes clearing timeouts, resolving promises,<br/>resetting dehydration, updating isFetching/invalid
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 fead952
☁️ 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 (5)
packages/router-core/src/route.ts (1)
1210-1242: DeprecateparentMatchPromise& Clarify Awaitable SemanticsThe name
parentMatchPromisestill implies a Promise-only API, but its type is nowAwaitable<…>—it may be a raw value or a Promise. To avoid confusing JS consumers (who might doctx.parentMatchPromise.then(…)and hit runtime errors when it’s not thenable), we should:
- Deprecate the old name and introduce a clearer alias.
- Update the JSDoc to state it can be either a value or a Promise, and show how to normalize it.
- Recommend using
awaitorPromise.resolve()against it.Locations to update:
• packages/router-core/src/route.ts:1237
- Add a deprecation notice on
parentMatchPromisein its JSDoc.- Document that it’s an
Awaitable<…>(value | Promise) and suggestPromise.resolve(ctx.parentMatchPromise)orawait ctx.parentMatchPromise.• packages/router-core/src/route.ts
- Introduce a new readonly alias property:
/** * The parent route match, as a value or Promise. * @category loader */ parentMatch: Awaitable<MakeRouteMatchFromRoute<TParentRoute>>;- Mark
parentMatchPromiseas deprecated, e.g.:/** * @deprecated Use `parentMatch` instead. * A value or Promise for the parent route match. Normalize via `await` or `Promise.resolve()`. **/ parentMatchPromise: TId extends RootRouteId ? never : Awaitable<MakeRouteMatchFromRoute<TParentRoute>>;These changes preserve backward compatibility while guiding users toward the clearer API.
packages/router-core/src/load-matches.ts (4)
548-576: Type-safety for parentMatchPromise: avoidas anyand omit on rootTo prevent accidental runtime presence of a
neverfield for root routes and to preserve legacy.then(...)consumers, consider always wrapping non-root parent withPromise.resolve(...)and omitting the property for root.Apply this diff:
-const getLoaderContext = ( +const getLoaderContext = ( inner: InnerLoadContext, matchId: string, index: number, route: AnyRoute, ): LoaderFnContext => { - const parentMatchPromise = inner.matchPromises[index - 1] as any + const hasParent = index > 0 && route.id !== rootRouteId + const parentMatchPromise = hasParent + ? Promise.resolve(inner.matchPromises[index - 1]!) + : undefined const { params, loaderDeps, abortController, context, cause } = inner.router.getMatch(matchId)! const preload = resolvePreload(inner, matchId) - return { + const baseCtx = { params, deps: loaderDeps, preload: !!preload, - parentMatchPromise, abortController, context, location: inner.location, navigate: (opts) => inner.router.navigate({ ...opts, _fromLocation: inner.location, }), cause: preload ? 'preload' : cause, route, - } + } as any + if (hasParent) baseCtx.parentMatchPromise = parentMatchPromise + return baseCtx }This keeps runtime shape consistent (no
parentMatchPromiseon root) and preserves backwards compatibility for non-TS users by always providing a Promise when present.
784-788: Preload disabled: ensurepreloadflag doesn’t stay trueIf
preloadis requested butroute.options.preload === false, we early return — good. However,preloadmay have just been set totrueabove; keeping it set could affect future logic (e.g., the “don’t swallow errors if preload” branch). Consider resetting it before returning.Apply this diff:
if (preload && route.options.preload === false) { - // Do nothing - return settleLoadRouteMatch() + // Do nothing, and make sure we don't carry a stale preload flag + if (match.preload) { + inner.updateMatch(matchId, (prev) => ({ ...prev, preload: false })) + } + return settleLoadRouteMatch() }
586-589: Typo in comment“We''ll” → “we’ll”.
Apply this diff:
- // a minimum duration, we''ll wait for it to resolve + // a minimum duration, we’ll wait for it to resolve
554-565: No internal.thencalls detected onparentMatchPromise– optional compatibility safeguardI ran a repo-wide search and found zero occurrences of calling
.then(...)(including optional-chained?.then) onparentMatchPromiseoutside of tests, where we useawaitexclusively. This means there’s no immediate breakage inside our codebase.However, because we’ve changed
parentMatchPromiseto be potentially a direct (synchronous) value, JS‐only consumers who did call.then(...)would encounter a runtime error. To smooth migration for downstream JS users, you can:
- Always wrap the return value in a promise at runtime (e.g.
Promise.resolve(parentMatchPromise)) while keeping the TypeScript signature asAwaitable<T>.- Or document the change and recommend users switch any
ctx.parentMatchPromise.then(…)calls to eitherawait ctx.parentMatchPromiseorPromise.resolve(ctx.parentMatchPromise).then(…).No existing code needs updating, but you may choose one of those approaches to maintain public-API consistency.
📜 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 (2)
packages/router-core/src/load-matches.ts(5 hunks)packages/router-core/src/route.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/router-core/src/route.ts (2)
packages/router-core/src/utils.ts (1)
Awaitable(4-4)packages/router-core/src/Matches.ts (1)
MakeRouteMatchFromRoute(207-215)
packages/router-core/src/load-matches.ts (2)
packages/router-core/src/index.ts (1)
Awaitable(316-316)packages/router-core/src/utils.ts (1)
isPromise(467-475)
🔇 Additional comments (12)
packages/router-core/src/route.ts (1)
1237-1242: Widened parentMatchPromise to Awaitable — good callUsing Awaitable here unblocks sync paths and aligns with the flattened loader flow.
packages/router-core/src/load-matches.ts (11)
7-7: Importing Awaitable keeps this file self-containedExplicitly importing Awaitable improves local readability and makes the intent clear.
38-39: matchPromises now holds Awaitable — aligns with mixed sync/async flowThis lets downstream contexts access either a settled match or a thenable seamlessly.
707-716: loadRouteMatch now returns Awaitable — flatter and easier to reason aboutDropping
asyncand returning an Awaitable clarifies sync vs async paths and avoids unnecessary microtasks.
717-729: Server skip path updates head via.thenwithout forcing anawaitNice touch. This prevents stalling the server pipeline when only head assets need updating.
736-750: While a loader is running, return stale data unless it was a preloadThis preserves SWR semantics and avoids swallowing preload errors. The follow-up
.thencorrectly re-throws viahandleRedirectAndNotFoundif needed.
753-772: Staleness/shouldReload computation reads clean;shouldReloadgets full contextPassing the LoaderFnContext into a functional
shouldReloadenables rich decisions (and now supports sync parent via Awaitable).
791-809: Background SWR path: Redirects handled; what about NotFound?In the async IIFE, the
catchhandles redirects (navigate) but ignores NotFound. That’s likely intentional (background revalidation shouldn’t tear down navigation), but please confirm. If unintentional, decide whether to surface NotFound to the router error boundary or keep current behavior.
831-855: Centralized settlement is solid; cleans pending timeout and dehydration, normalizes flagsThe
settleLoadRouteMatchhelper makes lifecycle transitions much clearer and reduces duplication.
889-896: Conditional Promise.all avoids needless microtask churnThe
hasPromisesguard is a nice micro-optimization. It preserves sync flows when everything settled synchronously.
578-646: Minor: head execution awaits consistently in runLoader; client/server parity preservedThe switch to capturing
headResultthen awaiting keeps behavior consistent and readable.
858-911: End-to-end flow remains robust with earlyonReadytriggering and error handlingThe try/catch around the entire sequence, plus final
return inner.matches, keeps external API stable.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/router-core/src/utils.ts (1)
187-194: Make return-promise detection robust for unions and thenablesCurrent check treats mixed sync/async return types as
booleanand ignoresPromiseLike. If the loader’s return is a union (e.g.,Promise<T> | T | void), downstream narrowing that relies ontrue | falsecan become imprecise. Also, treatingPromiseLikeas async is safer and aligns with runtimethen-duck-typing.Consider this refinement that:
- Returns
trueif all branches are promises,falseif none are,booleanif some are promises and some are not,- and treats
PromiseLikeas async.-export type LooseIsReturnPromise<T> = T extends ( - ...args: Array<any> -) => infer TReturn - ? TReturn extends Promise<any> - ? true - : false - : never +export type LooseIsReturnPromise<T> = T extends (...args: Array<any>) => infer R + ? [Extract<R, Promise<any> | PromiseLike<any>>] extends [never] + ? false + : [Exclude<R, Promise<any> | PromiseLike<any>>] extends [never] + ? true + : boolean + : neverpackages/router-core/src/route.ts (1)
425-430:asyncLoaderFnflag is reasonable, but be careful with mixed sync/async loadersThe
unknown → booleanandnever → booleanfallbacks are sensible. For loaders typed as a union (Promise<T> | T | void),LooseIsReturnPromise<TLoaderFn>evaluates toboolean. Downstream consumers must handle that uncertainty explicitly. See theparentMatchPromisecomment below to ensure the ambiguity doesn’t incorrectly narrow to the sync branch.If you keep the
booleanoutcome for mixed loaders, ensure all places that branch onasyncLoaderFnhave a third branch for the ambiguous case.docs/router/framework/react/api/router/RouteOptionsType.md (1)
149-150: Clarify root route special-case and mixed loader behaviorTwo nitpicks to reduce confusion:
- Mention that root routes have no parent match, so
parentMatchPromiseisnever.- Suggest treating
parentMatchPromiseasAwaitablein examples when the parent loader can be sync or async.- `parentMatchPromise` is a promise _if_ the parent route's `loader` returns a promise as well, otherwise it is the resolved parent match object. + - `parentMatchPromise` is a promise _if_ the parent route's `loader` returns a promise; otherwise it is the resolved parent match object. + - For the root route, there is no parent match: `parentMatchPromise` is `never`. + - If a parent loader can be either sync or async, treat `parentMatchPromise` as an `Awaitable` in your code, e.g. `const parent = await Promise.resolve(parentMatchPromise)`.
📜 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 (3)
docs/router/framework/react/api/router/RouteOptionsType.md(2 hunks)packages/router-core/src/route.ts(3 hunks)packages/router-core/src/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/router-core/src/route.ts (2)
packages/router-core/src/utils.ts (1)
LooseIsReturnPromise(187-193)packages/router-core/src/Matches.ts (1)
MakeRouteMatchFromRoute(207-215)
⏰ 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 (2)
packages/router-core/src/route.ts (1)
27-30: Import looks goodImporting
LooseIsReturnPromisehere is appropriate and scoped to type-level usage only. No runtime impact.docs/router/framework/react/api/router/RouteOptionsType.md (1)
134-137: Docs match the new behavior (Promise or resolved), goodUpdating
parentMatchPromiseto a union type reflects the new sync/async behavior accurately.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/router-core/src/load-matches.ts (3)
586-590: Typo in comment.Minor: “we''ll” → “we’ll”.
- // a minimum duration, we''ll wait for it to resolve + // a minimum duration, we’ll wait for it to resolve
719-733: Server skip path updates head; client skip path does not — intentional?On server skip you compute head via
executeHead(...).then(...). On client skip you immediatelyreturn settleLoadRouteMatch()with no head refresh. IfbeforeLoad/context changes could affect head client-side even when loader is skipped (e.g., hydration withdehydrated), consider mirroring the server behavior for parity whenroute.options.head|scripts|headersexist.If desired, conditionally compute head before
settleLoadRouteMatch()on client when a head provider exists.
548-576: Public API note: widenparentMatchPromisein docs and types.The PR description mentions widening
parentMatchPromisefromPromise<RouteMatch>toRouteMatch | Promise<RouteMatch>. Ensure docs andRouteOptionsTypereflect the (likely) optionality for the root case.I can open a follow-up patch to adjust the type and docs and to add tests covering root/child loader contexts if helpful.
📜 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(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/router-core/src/load-matches.ts (1)
packages/router-core/src/index.ts (4)
Awaitable(316-316)AnyRouteMatch(90-90)createControlledPromise(278-278)isRedirect(372-372)
⏰ 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 (12)
packages/router-core/src/load-matches.ts (12)
7-7: AdoptAwaitablefor internal async flows — good call.Using a first-class
Awaitabletype will letloadRouteMatchshort‑circuit synchronously without widening the public Promise surface unnecessarily.
38-38:matchPromises: Array<Awaitable<AnyRouteMatch>>aligns with mixed sync/async results.This preserves type-safety while still allowing
Promise.all(inner.matchPromises)to work with plain values. Nice.
707-716: FlattenedloadRouteMatchreturningAwaitable— solid structural refactor.Clear separation of fast-path sync return vs async work. This should trim microtasks in common cases.
808-827: Head-only refresh path is correct and batched.Good use of
batchto coalesce the head update with settle. Ensures minimal extra renders.
840-866: Centralized finalization viasettleLoadRouteMatchlooks robust.
- Properly cleans pending timeout and dehydration flags.
- Resolves
loaderPromise/loadPromiseonly when not running in background.- Idempotent isFetching/invalid updates, with
batchto minimize churn.Nice improvement in readability and correctness.
880-905: Parallel loader kickoff with mixed return types — works withPromise.all.Pushing
Awaitableresults and thenawait Promise.allmaintains correct execution order forparentMatchPromisewhile enabling sync short-circuit.
900-921: EarlyonReadytrigger sequencing remains intact.You still guarantee a single
onReadyresolution even when pending timers fire or errors short‑circuit — good.
523-546: Consider lazy head computation only when providers exist.
executeHeadunconditionally allocatesassetContextand runsPromise.allwhen any ofhead|scripts|headersexist — good. For completeness, you already short‑circuit when none exist. No change required.
141-156: Skip conditions are clear and side-effect free.Hydration and server
ssr === falsechecks are cheap and early — good for hot paths.
338-473:beforeLoadpipeline is well-ordered and resolves state consistently.Abort controller propagation, pending state, and context merge are handled carefully; serial error handling flows into
handleRedirectAndNotFoundas expected.
578-705:runLoaderhappy/error paths handle head, pending, and status transitions cohesively.All branches clear
isFetching, setstatus, and merge head consistently. No change required.
554-576: I've added a script to display the fullLoaderFnContextdefinition for review. Let's confirm the exact type ofparentMatchPromisebefore updating the suggestion.
| status === 'success' && (invalid || (shouldReload ?? age > staleAge)) | ||
| if (loaderShouldRunAsync && !inner.sync) { | ||
| loaderIsRunningAsync = true | ||
| ;(async () => { | ||
| try { | ||
| 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, | ||
| })) | ||
| const match = inner.router.getMatch(matchId)! | ||
| match._nonReactive.loaderPromise?.resolve() | ||
| match._nonReactive.loadPromise?.resolve() | ||
| match._nonReactive.loaderPromise = undefined | ||
| } catch (err) { | ||
| if (isRedirect(err)) { | ||
| await inner.router.navigate(err.options) | ||
| } | ||
| } | ||
| } | ||
| })() | ||
| return settleLoadRouteMatch() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid unhandled rejections during background redirects.
The async IIFE isn’t observed; awaiting inner.router.navigate(...) can leak unhandled rejections if navigation fails. Fire-and-forget instead.
- } catch (err) {
- if (isRedirect(err)) {
- await inner.router.navigate(err.options)
- }
- }
+ } catch (err) {
+ if (isRedirect(err)) {
+ // Detach to avoid unhandled rejections in the background task
+ void inner.router.navigate(err.options)
+ }
+ }📝 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.
| status === 'success' && (invalid || (shouldReload ?? age > staleAge)) | |
| if (loaderShouldRunAsync && !inner.sync) { | |
| loaderIsRunningAsync = true | |
| ;(async () => { | |
| try { | |
| 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, | |
| })) | |
| const match = inner.router.getMatch(matchId)! | |
| match._nonReactive.loaderPromise?.resolve() | |
| match._nonReactive.loadPromise?.resolve() | |
| match._nonReactive.loaderPromise = undefined | |
| } catch (err) { | |
| if (isRedirect(err)) { | |
| await inner.router.navigate(err.options) | |
| } | |
| } | |
| } | |
| })() | |
| return settleLoadRouteMatch() | |
| } | |
| status === 'success' && (invalid || (shouldReload ?? age > staleAge)) | |
| if (loaderShouldRunAsync && !inner.sync) { | |
| loaderIsRunningAsync = true | |
| ;(async () => { | |
| try { | |
| await runLoader(inner, matchId, index, route) | |
| const match = inner.router.getMatch(matchId)! | |
| match._nonReactive.loaderPromise?.resolve() | |
| match._nonReactive.loadPromise?.resolve() | |
| match._nonReactive.loaderPromise = undefined | |
| } catch (err) { | |
| if (isRedirect(err)) { | |
| // Detach to avoid unhandled rejections in the background task | |
| void inner.router.navigate(err.options) | |
| } | |
| } | |
| })() | |
| return settleLoadRouteMatch() | |
| } |
🤖 Prompt for AI Agents
In packages/router-core/src/load-matches.ts around lines 784 to 801, the async
IIFE currently does "await inner.router.navigate(err.options)" inside the catch
block which can leak unhandled rejections when navigation fails; instead call
navigate in a fire-and-forget manner and ensure any rejection is handled locally
(e.g. invoke inner.router.navigate(err.options).catch(() => {/* optional log
*/}) or drop the await and attach a .catch handler) so the background redirect
cannot create an unhandled rejection.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/router-core/src/load-matches.ts (2)
771-784: Preload opt‑out can still flipmatch.preloadto true (order-of-operations bug).
nextPreloadis computed and applied before theroute.options.preload === falseearly-return branch, so the match may be marked as preloaded even when the route opted out. ComputenextPreloadto respect the route-level opt‑out and remove the now-redundant branch.Apply:
- const nextPreload = - !!preload && !inner.router.state.matches.some((d) => d.id === matchId) + const nextPreload = + !!preload && + route.options.preload !== false && + !inner.router.state.matches.some((d) => d.id === matchId) @@ - if (preload && route.options.preload === false) { - // Do nothing - return settleLoadRouteMatch() - } + // route.options.preload === false is respected via nextPreload computation above
792-805: Avoid potential unhandled rejections when redirecting from the background task.Awaiting
navigateinside the fire-and-forget IIFE can leak an unhandled rejection if navigation fails. Detach and attach a no-op catch.Apply:
- } catch (err) { - if (isRedirect(err)) { - await inner.router.navigate(err.options) - } - } + } catch (err) { + if (isRedirect(err)) { + // Detach to avoid surfacing unhandled rejections from a background redirect + void inner.router.navigate(err.options).catch(() => {}) + } + }
🧹 Nitpick comments (2)
packages/router-core/src/load-matches.ts (2)
7-8: Adopted Awaitable correctly; tighten typing where it’s consumed to avoidany.Importing
Awaitableand wideningmatchPromisestoArray<Awaitable<AnyRouteMatch>>aligns with the new mixed sync/async behavior. To keep types precise through the loader context, avoid theas anycast at the consumption site and express the union explicitly.You can type
parentMatchPromiseingetLoaderContextlike this (no behavioral change):// in getLoaderContext(...) const parentMatchPromise: Awaitable<AnyRouteMatch> | undefined = inner.matchPromises[index - 1]Also applies to: 38-39
808-810: Prevent double-resolvingloaderPromisein the sync-run branch.
runLoaderalready resolvesmatch._nonReactive.loaderPromise. CallingsettleLoadRouteMatchwithloaderIsRunningAsync === falsewill resolve it again. It’s harmless ifcreateControlledPromiseis idempotent, but avoid the extra resolve for clarity.Apply:
- if (status !== 'success' || (loaderShouldRunAsync && inner.sync)) { - return runLoader(inner, matchId, index, route).then(settleLoadRouteMatch) - } + if (status !== 'success' || (loaderShouldRunAsync && inner.sync)) { + // Mark as async to avoid double-resolving the loaderPromise in settle + loaderIsRunningAsync = true + return runLoader(inner, matchId, index, route).then(settleLoadRouteMatch) + }
📜 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(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/router-core/src/load-matches.ts (1)
packages/router-core/src/index.ts (4)
Awaitable(316-316)AnyRouteMatch(90-90)createControlledPromise(278-278)isRedirect(372-372)
⏰ 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)
716-731: Server-side head update path looks correct.Returning the match synchronously when only head assets need updating on the server keeps the new Awaitable contract intact and avoids unnecessary async hops.
833-855: Centralized settle covers necessary cleanup and state normalization.Clearing
pendingTimeout, resettingdehydrated, normalizingisFetching, and guarding extra store writes only when needed are all on point.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/router-core/src/load-matches.ts (2)
773-786: Bug: route-levelpreload: falsecan still mark the match as preloaded.You compute and apply
nextPreloadbefore checkingroute.options.preload === false. This can flipmatch.preloadtotruedespite a route-level opt‑out.Apply one of the following minimal fixes. Option A (preferred): short-circuit before touching
preload:- const nextPreload = - !!preload && !inner.router.state.matches.some((d) => d.id === matchId) - prevMatch._nonReactive.loaderPromise = createControlledPromise<void>() - if (nextPreload !== prevMatch.preload) { - inner.updateMatch(matchId, (prev) => ({ - ...prev, - preload: nextPreload, - })) - } - - if (preload && route.options.preload === false) { - // Do nothing - return settleLoadRouteMatch() - } + if (preload && route.options.preload === false) { + // Respect route-level opt-out from preloading: don't mark the match as preloaded + return settleLoadRouteMatch() + } + + const nextPreload = + !!preload && !inner.router.state.matches.some((d) => d.id === matchId) + prevMatch._nonReactive.loaderPromise = createControlledPromise<void>() + if (nextPreload !== prevMatch.preload) { + inner.updateMatch(matchId, (prev) => ({ + ...prev, + preload: nextPreload, + })) + }Option B: incorporate the opt‑out into the computation:
- const nextPreload = - !!preload && !inner.router.state.matches.some((d) => d.id === matchId) + const nextPreload = + !!preload && + route.options.preload !== false && + !inner.router.state.matches.some((d) => d.id === matchId)
788-808: Avoid unhandled rejections when redirecting in the background.The async IIFE is detached; awaiting
inner.router.navigate(...)inside its catch can leak unhandled rejections if navigation fails.- if (isRedirect(err)) { - await inner.router.navigate(err.options) - } + if (isRedirect(err)) { + // Detach and guard against unhandled rejections + void inner.router.navigate(err.options).catch(() => {}) + }Also update the renamed flag if you adopt the previous nit (e.g.,
loaderRunningInBackground = true).
🧹 Nitpick comments (3)
packages/router-core/src/load-matches.ts (3)
709-717: Signature change toAwaitable<AnyRouteMatch>looks good; minor naming nit.Variable
loaderIsRunningAsynccommunicates intent, butloaderRunningInBackgroundwould be more precise (it's about background execution vs. foreground sync, not just “async”).- let loaderIsRunningAsync = false + let loaderRunningInBackground = falseYou’d also need to rename usages in
settleLoadRouteMatch.
752-772:stale-while-revalidatecomputation is coherent; consider lazy context creation.You compute
shouldReloadby callingshouldReloadOption(getLoaderContext(...)), which constructs a full loader context. IfshouldReloadis a boolean, the context work is wasted. Optional micro-optimization:- const shouldReload = - typeof shouldReloadOption === 'function' - ? shouldReloadOption(getLoaderContext(inner, matchId, index, route)) - : shouldReloadOption + const shouldReload = + typeof shouldReloadOption === 'function' + ? shouldReloadOption(getLoaderContext(inner, matchId, index, route)) + : shouldReloadOption(No behavior change; only highlight that the function case is the only place context is needed.)
832-858: Centralized finalization is solid; minor polish.
- Clearing pending timeout, resetting dehydration, and normalizing
isFetching/invalidis correct.- Consider naming the flag
loaderRunningInBackgroundfor clarity if you accept the earlier nit.
📜 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 (1)
packages/router-core/src/utils.ts (2)
Awaitable(4-4)createControlledPromise(388-412)
⏰ 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). (1)
- GitHub Check: Test
🔇 Additional comments (6)
packages/router-core/src/load-matches.ts (6)
7-7: Good: importingAwaitablesets the stage for sync-or-async flows.This aligns with the PR goal to enable synchronous execution paths where possible.
38-39: WideningmatchPromisestoArray<Awaitable<AnyRouteMatch>>is appropriate.This lets
loadRouteMatchreturn a value synchronously without forcing a promise. The laterPromise.all(inner.matchPromises)will handle immediate values correctly.
718-733: Server skip path: head application viathen(...)is sound.Applying head and returning the updated match without forcing a full settle is appropriate on the server. No issues spotted.
735-750: In-flight loader handling preserves UX while avoiding error swallowing for preloads.Returning the stale success match immediately (when not
syncand notpreload) and deferring to the in-flight promise is correct. The post-resolution error path properly funnels throughhandleRedirectAndNotFound.
810-812: Foreground path (syncor not-success) executes loader then settles — LGTM.Chaining
runLoader(...).then(settleLoadRouteMatch)ensures promises resolve and pending state clears in order.
814-830: Deferred head update when loader doesn’t run is correct and batched.Batching the head application with
settleLoadRouteMatch()minimizes updates. Looks good.
| const parentMatchPromise = Promise.resolve( | ||
| inner.matchPromises[index - 1] as any, | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don’t wrap parentMatchPromise in Promise.resolve — preserve the new union type.
Wrapping with Promise.resolve(...) erases the benefit of the new public type (RouteMatch | Promise<RouteMatch>) by always forcing a microtask hop. Pass the underlying Awaitable through so a loader can synchronously access an already-settled parent when available.
Apply this diff:
- const parentMatchPromise = Promise.resolve(
- inner.matchPromises[index - 1] as any,
- )
+ const parentMatchPromise =
+ inner.matchPromises[index - 1] as Awaitable<AnyRouteMatch>Run this to confirm the public type actually advertises the union and that no downstream code assumes Promise<...>:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the LoaderFnContext type to ensure the union is declared
rg -nC3 -P '(type|interface)\s+LoaderFnContext\b' packages | sed -n '1,200p' || true
rg -nC2 -P 'parentMatchPromise\s*:\s*.*\|' packages || true
# Detect call sites still assuming Promise<...>
rg -nC2 -P 'parentMatchPromise\s*:\s*Promise<' packages || true
rg -nC2 -P 'await\s+[^;\n]*parentMatchPromise\b' packages || trueLength of output: 8611
Preserve the Awaitable type for parentMatchPromise and update affected tests
To retain the new RouteMatch | Promise<RouteMatch> union and avoid unnecessary microtask hops, remove the Promise.resolve wrapper in load-matches.ts:
--- a/packages/router-core/src/load-matches.ts
+++ b/packages/router-core/src/load-matches.ts
@@ -554,7 +554,7 @@
- const parentMatchPromise = Promise.resolve(
- inner.matchPromises[index - 1] as any,
- )
+ const parentMatchPromise =
+ inner.matchPromises[index - 1] as Awaitable<AnyRouteMatch>Because this change surfaces a union instead of always a Promise, update the test declarations that currently assume Promise<…> for parentMatchPromise. For example:
• packages/solid-router/tests/route.test-d.tsx
– Lines 415, 461, 655, 680, 704, 807, 1166
• change
parentMatchPromise: Promise<MakeRouteMatchFromRoute<typeof rootRoute>>
to
parentMatchPromise: Promise<MakeRouteMatchFromRoute<typeof rootRoute>> | MakeRouteMatchFromRoute<typeof rootRoute>
• packages/solid-router/tests/loaders.test.tsx
– Line 138
• update the loader context signature to reflect Awaitable<…> for parentMatchPromise
• packages/react-router/tests/route.test-d.tsx
– Lines 411, 462, 684, 709, 733, 836, 1203
• apply the same union-type adjustment as above
• packages/react-router/tests/loaders.test.tsx
– Line 159
• ensure the loader context declaration uses Awaitable<…>
Making these test updates is required so that the public API’s union type is correctly represented in both runtime and type-level tests.
📝 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.
| const parentMatchPromise = Promise.resolve( | |
| inner.matchPromises[index - 1] as any, | |
| ) | |
| // Preserve the Awaitable<AnyRouteMatch> type instead of always wrapping in a Promise | |
| const parentMatchPromise = | |
| inner.matchPromises[index - 1] as Awaitable<AnyRouteMatch> |
🤖 Prompt for AI Agents
In packages/router-core/src/load-matches.ts around lines 554 to 556, remove the
Promise.resolve wrapper so parentMatchPromise preserves the Awaitable type
(i.e., allow RouteMatch | Promise<RouteMatch> rather than always wrapping in a
Promise), and then update the affected test declarations to reflect the
union/Awaitable type: change the listed parentMatchPromise types in
packages/solid-router/tests/route.test-d.tsx (lines 415, 461, 655, 680, 704,
807, 1166) and packages/react-router/tests/route.test-d.tsx (lines 411, 462,
684, 709, 733, 836, 1203) from Promise<…> to Promise<…> | …
(MakeRouteMatchFromRoute<typeof rootRoute>), and update the loader context
signatures in packages/solid-router/tests/loaders.test.tsx (line 138) and
packages/react-router/tests/loaders.test.tsx (line 159) to use Awaitable<…>;
ensure types compile after these changes.
This PR flattens the
loadRouteMatchinload-matches.ts. This makes it easier to understand what code runs in which conditions, and makes it possible to not return a promise at all in some situations.Summary by CodeRabbit