refactor(router-core): skip beforeLoad and related store updates if options is not defined #4928
Conversation
…ptions is not defined
|
View your CI Pipeline Execution ↗ for commit 8a7f954
☁️ 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: |
WalkthroughReworks router-core beforeLoad and pending timing/flow with new helpers, AbortController-based pending lifecycle, earlier parse/validate error surfacing, batched state updates, and minor timing changes; adds tests and uses the new notFound export and router options. All production API signatures unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Router
participant Match
participant BeforeLoad
participant Loader
participant Timer as PendingTimeout
App->>Router: navigate(location, cause)
Router->>Match: create AbortController, call pending()
Router->>Timer: setupPendingTimeout()
alt no beforeLoad
Router-->>App: batched pending state rendered
else beforeLoad present
Router->>BeforeLoad: invoke (sync or async) with beforeLoadFnContext
alt sync result
Router-->>Match: merge context, set __beforeLoadContext (batched)
else async result
BeforeLoad-->>Router: resolve or throw
Router-->>Match: merge context or handle BEFORE_LOAD error
end
end
Timer-->>Router: pendingMs elapsed -> triggerOnReady()
Router->>Loader: start loader (if applicable)
alt error/redirect/notFound
Router-->>App: handle error/redirect/notFound flow
else success
Router-->>App: render ready state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 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 (
|
![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/router.ts (4)
2321-2324: Make pending timeout resilient to async errors from onReady
triggerOnReadycan return a Promise. IfonReadyever throws/rejects, this can surface as an unhandled rejection in the timeout callback. Guard it defensively.Apply this diff:
- // Update the match and prematurely resolve the loadMatches promise so that - // the pending component can start rendering - this.triggerOnReady(innerLoadContext) + // Update the match and prematurely resolve the loadMatches promise so that + // the pending component can start rendering + const result = this.triggerOnReady(innerLoadContext) + if (isPromise(result)) void result.catch(() => {})
2416-2433: Return early after PARSE_PARAMS/VALIDATE_SEARCH errors to avoid unnecessary state churnWe already resolve promises and mark the match as errored in
handleSerialError. Continuing (setting pending timers, etc.) is unnecessary and slightly noisy. Early returns here keep the flow tighter and avoid extra updates.Apply this diff:
if (paramsError) { this.handleSerialError( innerLoadContext, index, paramsError, 'PARSE_PARAMS', ) + return } if (searchError) { this.handleSerialError( innerLoadContext, index, searchError, 'VALIDATE_SEARCH', ) + return }
2436-2443: Skip fetchCount bump and redundant state updates when no beforeLoad is definedThe bail-out path still invokes
pending()which incrementsfetchCountand assigns a newabortController, then immediately clears fetching in the same batch. Since there's no beforeLoad work, we can resolve the promise without touching the match state, further reducing updates and avoiding a misleading fetchCount bump.Apply this diff:
- // if there is no `beforeLoad` option, skip everything, batch update the store, return early + // if there is no `beforeLoad` option, skip everything, resolve, return early if (!route.options.beforeLoad) { - batch(() => { - pending() - resolve() - }) + resolve() return }
2462-2471: Guard against null (or non-object) beforeLoad returns when merging contextIf a route's
beforeLoadmistakenly returnsnull, the spread operator will throw. Treatundefined/nullas "no additional context".Apply this diff:
- innerLoadContext.updateMatch(matchId, (prev) => ({ - ...prev, - __beforeLoadContext: beforeLoadContext, - context: { - ...prev.context, - ...beforeLoadContext, - }, - })) + innerLoadContext.updateMatch(matchId, (prev) => { + const safeCtx = + beforeLoadContext && typeof beforeLoadContext === 'object' + ? beforeLoadContext + : {} + return { + ...prev, + __beforeLoadContext: safeCtx, + context: { + ...prev.context, + ...safeCtx, + }, + } + })packages/react-router/tests/store-updates-during-navigation.test.tsx (1)
134-148: Great addition: sync beforeLoad scenario coveredCovers a key path; expectation of 13 seems consistent with the new lifecycle.
Consider adding a couple of small follow-ups to lock in the bail-out and edge behaviors that this PR targets:
- A route with no beforeLoad: assert minimal updates (ensures the bail-out truly produces no extra store updates).
- A beforeLoad that returns undefined: behaves like “no context change”.
- A beforeLoad that returns null: if you keep the suggested guard, verify it doesn’t throw and produces no context merge.
I can draft these tests 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 (2)
packages/react-router/tests/store-updates-during-navigation.test.tsx(2 hunks)packages/router-core/src/router.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/router-core/src/router.ts (3)
packages/router-core/src/utils.ts (2)
createControlledPromise(398-422)isPromise(477-485)packages/router-core/src/redirect.ts (1)
isRedirect(95-97)packages/router-core/src/route.ts (1)
BeforeLoadContextOptions(966-977)
⏰ 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 (3)
packages/router-core/src/router.ts (1)
2724-2726: Loader error path honors minPending only when present — LGTMAwaiting
potentialPendingMinPromise(matchId)conditionally avoids unnecessary waits and matches the intent. Looks good.packages/react-router/tests/store-updates-during-navigation.test.tsx (2)
113-114: Updated expectation reflects fewer store updates — LGTMThe drop to 14 aligns with the optimized beforeLoad/pending flow.
131-133: Preload redirection update count decreased — LGTMThe new expectation at 6 matches the refined error/pending timing.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/router-core/src/router.ts (3)
2321-2324: Guard triggerOnReady to avoid unhandled rejections in setTimeout callbackRemoving the try/catch means any thrown error or rejected promise from
onReadywill become an unhandled error in the timeout callback. Wrap the call and catch async rejections to avoid noisy console errors in production and hard-to-reproduce bugs.- const pendingTimeout = setTimeout(() => { - // Update the match and prematurely resolve the loadMatches promise so that - // the pending component can start rendering - this.triggerOnReady(innerLoadContext) - }, pendingMs) + const pendingTimeout = setTimeout(() => { + // Update the match and prematurely resolve the loadMatches promise so that + // the pending component can start rendering + try { + const ready = this.triggerOnReady(innerLoadContext) + if (isPromise(ready)) { + ready.catch((err) => { + if (process.env.NODE_ENV !== 'production') { + console.error('[router] onReady failed during pending timeout', err) + } + }) + } + } catch (err) { + if (process.env.NODE_ENV !== 'production') { + console.error('[router] onReady threw during pending timeout', err) + } + } + }, pendingMs)
2403-2425: Reuse existing abortController instead of swapping in a new instanceCreating a new
AbortControllerhere replaces the one established when the match was created. That swaps cancellation tokens mid-flight and can be surprising for integrations holding a reference to the previous controller. Prefer reusing the existing controller when present.- const abortController = new AbortController() + const abortController = match.abortController ?? new AbortController() @@ - innerLoadContext.updateMatch(matchId, (prev) => ({ + innerLoadContext.updateMatch(matchId, (prev) => ({ ...prev, isFetching: 'beforeLoad', fetchCount: prev.fetchCount + 1, abortController, context, }))
2436-2444: Skip updateMatch entirely when no beforeLoad is defined to align with PR goal (fewer store updates)This early-bail branch still calls
pending()andresolve()which each executeupdateMatch(batched, but still a state write andfetchCountbump). The PR’s objective is to “skip beforeLoad and related store updates if options is not defined.” You can resolve the internal promise and return without any store mutations; the loader phase will take over settingisFetchingas needed.- // if there is no `beforeLoad` option, skip everything, batch update the store, return early - if (!route.options.beforeLoad) { - batch(() => { - pending() - resolve() - }) - return - } + // If there is no `beforeLoad` option, resolve and return early with no store writes + if (!route.options.beforeLoad) { + match._nonReactive.beforeLoadPromise?.resolve() + match._nonReactive.beforeLoadPromise = undefined + return + }Follow-up: This also avoids an extra
fetchCountincrement when nothing actually fetched.
📜 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(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/router-core/src/router.ts (3)
packages/router-core/src/utils.ts (2)
createControlledPromise(398-422)isPromise(477-485)packages/router-core/src/route.ts (1)
BeforeLoadContextOptions(966-977)packages/router-core/src/redirect.ts (1)
isRedirect(95-97)
⏰ 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/router.ts (2)
2381-2400: Early surfacing of params/search validation errors is solidHandling
paramsErrorandsearchErrorup-front with explicit router codes (PARSE_PARAMS,VALIDATE_SEARCH) tightens control flow and simplifies downstream error handling. This looks correct and beneficial.
2729-2731: Good: only await pendingMinPromise when presentThis reduces unnecessary delay on the error path and keeps timing more predictable.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/react-router/tests/store-updates-during-navigation.test.tsx (5)
84-86: Router options are appropriate; consider makingdefaultPreloadconfigurable per testSetting
defaultPreload: 'intent'globally works and is needed for the hover/focus scenario later. To reduce coupling across tests, consider acceptingdefaultPreloadas asetupoption so tests not exercising preloading can explicitly opt out. This can help reduce flakiness risk over time if event semantics evolve.
93-101: PreferfindByRoleover wrappinggetByRoleinwaitFor
findBy*queries encapsulate waiting and read cleaner. This is functionally equivalent and a small readability win.Apply:
- fireEvent.click(link) - const title = await waitFor( - () => screen.getByRole('heading', { name: /Title$/ }), // matches /posts and /other and not found - ) + fireEvent.click(link) + const title = await screen.findByRole('heading', { name: /Title$/ }) // matches /posts and /other and not found
137-141: Optional: add a correctness assertion alongside the update-count metricIn addition to the update-count assertion, consider asserting the side-effect (redirect) to catch regressions where the redirect is swallowed but updates still occur. Example:
- expect(updates).toBe(6) + expect(updates).toBe(6) + // Optional: ensure we actually redirected + expect(router.state.location.pathname).toBe('/other')Note: If
preloadRouteis designed to not navigate, skip this assertion.
159-169: Good guardrails for minimal path; consider hardening against flakinessThe range assertion acknowledges timing variability. If this ever becomes noisy in CI, two knobs can help:
- Disable preloading for this test (see suggestion to make
defaultPreloadconfigurable).- Use fake timers around the 100–300ms pending thresholds to fully control scheduling.
If you want, I can sketch a minimal fake-timers pattern compatible with testing-library + React 18.
186-209: Preload-on-intent scenario is well captured; minor test ergonomicsThe focus+click sequence is appropriate for
'intent'preloading, and the assertion targets the metric you care about. Two tiny nits:
- Prefer
findByRolefor readability, as above.- If you ever want to exercise hover intent specifically, you can also
fireEvent.mouseOver(link)(oruserEvent.hover) before click.Apply:
- const title = await waitFor(() => - screen.getByRole('heading', { name: /Title$/ }), - ) + const title = await screen.findByRole('heading', { name: /Title$/ })
📜 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(4 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). (1)
- GitHub Check: Test
🔇 Additional comments (4)
packages/react-router/tests/store-updates-during-navigation.test.tsx (4)
2-8: Imports update looks goodThe test now consistently uses testing-library’s render/screen/waitFor and cleanup. No issues.
122-122: Expectation update matches the PR’s objectiveThe reduced update count (17 → 14) reflects the early bail-out when
beforeLoadis undefined. Looks good.
143-157: Solid coverage for synchronousbeforeLoadpathThis specifically validates the optimization delta when
beforeLoadis sync. No issues.
171-184: Covers not-found raised inbeforeLoadThis ensures the early not-found surfacing path doesn’t inflate updates. Looks correct.
| import { | ||
| Link, | ||
| Outlet, | ||
| RouterProvider, | ||
| createRootRoute, | ||
| createRoute, | ||
| createRouter, | ||
| notFound, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure the notFound export is available from the public entry
If notFound is a new public API, verify it’s exported from the package entry and re-exported where this test imports it.
Run this to confirm the export and its surface:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Looking for notFound exports (direct or re-export):"
rg -nP -C2 '(export\s+function\s+notFound\b)|(export\s+const\s+notFound\b)|(export\s*{\s*notFound\s*})' --type ts --glob '!**/node_modules/**'
echo
echo "Verifying createRouter options include defaultNotFoundComponent & defaultPreload:"
rg -nP -C3 '\b(defaultNotFoundComponent|defaultPreload)\b' --type ts --glob '!**/node_modules/**'Length of output: 82102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Import section from the test file ==="
sed -n '1,50p' packages/react-router/tests/store-updates-during-navigation.test.tsx
echo
echo "=== Re-exports of notFound in react-router public entry (index.ts) ==="
rg -nP 'export\s+.*notFound' packages/react-router/src/index.ts || echo "No direct re-exports found in index.ts"Length of output: 1446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Files in packages/react-router/src ==="
ls -1 packages/react-router/src
echo
echo "=== Looking for index.* files ==="
find packages/react-router/src -maxdepth 1 -type f -name "index.*" || echo "No index.* files found"
echo
echo "=== Showing contents of any index.* file ==="
for f in packages/react-router/src/index.*; do
echo "---- $f ----"
sed -n '1,200p' "$f"
done 2>/dev/null || trueLength of output: 5057
Missing public export of notFound
The test in
packages/react-router/tests/store-updates-during-navigation.test.tsx
imports notFound, but it isn’t re-exported in the public entry at
packages/react-router/src/index.tsx. Add it to the export list:
• File: packages/react-router/src/index.tsx
-export {
- defer,
- TSR_DEFERRED_PROMISE,
- isMatch,
- joinPaths,
- cleanPath,
- trimPathLeft,
- trimPathRight,
- trimPath,
- resolvePath,
- parsePathname,
- interpolatePath,
- matchPathname,
- removeBasepath,
- matchByPath,
- encode,
- decode,
- rootRouteId,
- defaultSerializeError,
- defaultParseSearch,
- defaultStringifySearch,
- parseSearchWith,
- stringifySearchWith,
- escapeJSON, // SSR
- pick,
- functionalUpdate,
- replaceEqualDeep,
- isPlainObject,
- isPlainArray,
- deepEqual,
- shallow,
- createControlledPromise,
- retainSearchParams,
- stripSearchParams,
-} from '@tanstack/router-core'
+export {
+ defer,
+ TSR_DEFERRED_PROMISE,
+ isMatch,
+ joinPaths,
+ cleanPath,
+ trimPathLeft,
+ trimPathRight,
+ trimPath,
+ resolvePath,
+ parsePathname,
+ interpolatePath,
+ matchPathname,
+ removeBasepath,
+ matchByPath,
+ encode,
+ decode,
+ rootRouteId,
+ defaultSerializeError,
+ defaultParseSearch,
+ defaultStringifySearch,
+ parseSearchWith,
+ stringifySearchWith,
+ escapeJSON, // SSR
+ pick,
+ functionalUpdate,
+ replaceEqualDeep,
+ isPlainObject,
+ isPlainArray,
+ deepEqual,
+ shallow,
+ createControlledPromise,
+ retainSearchParams,
+ stripSearchParams,
+ notFound,
+} from '@tanstack/router-core'📝 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.
| notFound, | |
| export { | |
| defer, | |
| TSR_DEFERRED_PROMISE, | |
| isMatch, | |
| joinPaths, | |
| cleanPath, | |
| trimPathLeft, | |
| trimPathRight, | |
| trimPath, | |
| resolvePath, | |
| parsePathname, | |
| interpolatePath, | |
| matchPathname, | |
| removeBasepath, | |
| matchByPath, | |
| encode, | |
| decode, | |
| rootRouteId, | |
| defaultSerializeError, | |
| defaultParseSearch, | |
| defaultStringifySearch, | |
| parseSearchWith, | |
| stringifySearchWith, | |
| escapeJSON, // SSR | |
| pick, | |
| functionalUpdate, | |
| replaceEqualDeep, | |
| isPlainObject, | |
| isPlainArray, | |
| deepEqual, | |
| shallow, | |
| createControlledPromise, | |
| retainSearchParams, | |
| stripSearchParams, | |
| notFound, | |
| } from '@tanstack/router-core' |
🤖 Prompt for AI Agents
packages/react-router/src/index.tsx (export list): The test imports `notFound`
but it is not exported from the package entry; add a named export for `notFound`
to the public export list in packages/react-router/src/index.tsx so it can be
imported by tests and consumers. Ensure the exported identifier matches the
internal symbol (or re-export from its source module), update the export list
accordingly, and run the test suite to verify the import succeeds.
…tore (#4964) Following #4916, #4925, #4926, and #4928, this PR keeps reducing the number of store updates (`__store.setState` through `updateMatch`). We check the values we want to set in the store are *actually different* from what is already in the store before calling `updateMatch` --- In the `store-updates-during-navigation` test tracking the number of executions of a `useRouterState > select` method during a navigation, our main test case goes from **14 calls** without this PR, to **10 calls** with this PR. Most test cases show significant improvements as well. --- Should be a partial improvement of #4359 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Prevented loss of previously loaded data during navigation when a loader yields no value. * Stabilized preloading to avoid redundant updates and unnecessary state churn. * Improved loading-state cleanup after navigation, reducing flicker and inconsistent “fetching” indicators. * **Performance** * Reduced unnecessary state updates and re-renders during and after preloads/loads for smoother navigation. * **Tests** * Updated async navigation tests to reflect refined timing and data-return behavior and added a helper to simulate delayed async results. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Following #4916, #4925, and #4926, this PR keeps reducing the number of store updates (
__store.setStatethroughupdateMatch).Early bail out of route before load step if
route.options.beforeLoadis not definedThe
store-updates-during-navigationtest tracking the number of executions of auseRouterState > selectmethod during a navigation goes from 17 calls without this PR, to 14 calls with this PR (or even 13 calls ifbeforeLoadis synchronous).Should be a partial improvement of #4359
Summary by CodeRabbit
New Features
Refactor
Tests