refactor: don't reparse upon navigation #6398
Conversation
schiller-manuel
requested review from
Sheraff and
nlynzaad
and removed request for
nlynzaad
github-actions
bot
added
package: react-router
package: router-core
package: history
labels
📝 WalkthroughWalkthroughAdds async navigation results and navigate options propagation, introduces match-snapshot caching and session markers for fast-path back/forward navigations, updates Transitioner to optionally skip loads, and updates tests to await asynchronous navigation timing. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant History as RouterHistory
participant Router as Router Core
participant Matcher as Route Matcher
Client->>History: push(path, state, navigateOpts)
activate History
History->>History: tryNavigation() -> Promise<NavigationResult>
alt navigateOpts.skipTransitionerLoad true
History->>Router: commit location without triggering Transitioner load
else
History->>Router: build/commit location and trigger load()
end
activate Router
alt snapshot available and valid
Router->>Matcher: reuse MatchSnapshot (fast-path)
else
Router->>Matcher: perform full matchRoutes()
end
Matcher-->>Router: matchResult or reused snapshot
Router-->>History: commit location (with __TSR_matches/__TSR_sessionId)
deactivate Router
alt Navigation blocked by blocker
History-->>Client: Promise resolves { type: "BLOCKED" }
else
History-->>Client: Promise resolves { type: "SUCCESS" }
end
deactivate History
sequenceDiagram
participant Browser as Browser (popstate)
participant History as RouterHistory
participant Snapshot as MatchSnapshot Cache
participant Matcher as Route Matcher
Browser->>History: popstate (back/forward)
activate History
alt __TSR_matches present and session valid
History->>Snapshot: read __TSR_matches / _matchSnapshot
Snapshot-->>History: snapshot data
History->>Matcher: reuse snapshot (skip full matching)
else
History->>Matcher: perform full matchRoutes()
end
Matcher-->>History: matchResult
History-->>Browser: navigation complete
deactivate History
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
View your CI Pipeline Execution ↗ for commit 4b0a7fe
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/router-core/src/router.ts`:
- Around line 1295-1313: The snapshot fast-path sets parsedParams = undefined
which causes a TypeError later when code checks "key in parsedParams" if
skipRouteOnParseError is enabled; change the snapshot branch to initialize
parsedParams to an empty object (e.g., parsedParams = {}) or otherwise ensure
parsedParams is always an object when snapshotValid is true so subsequent checks
in the router (referenced by parsedParams, skipRouteOnParseError, snapshotValid,
matchedRoutes, routeParams) won't throw.
- Around line 2933-2974: buildMatchSnapshot fails to include a configured custom
notFoundRoute in the returned snapshot, so when notFoundRoute is provided you
should set snapshot.globalNotFoundRouteId to that route's id; update
buildMatchSnapshot to check if notFoundRoute is truthy and assign
snapshot.globalNotFoundRouteId = notFoundRoute.id (before the fallback logic
that scans matchedRoutes or assigns rootRouteId) so snapshots include the custom
404 route for later matching.
🧹 Nitpick comments (1)
packages/history/src/index.ts (1)
227-256: Consider returningNavigationResultfromgo,back, andforwardfor API consistency.These methods call
tryNavigationbut discard its result, whilepushandreplacenow returnPromise<NavigationResult>. This creates an asymmetry in the API. For browser history, this may be intentional since navigation completes viapopstateevents, but callers cannot programmatically detect if these navigations were blocked.
packages/router-core/src/router.ts
Outdated
| // Fast-path: use snapshot hint if valid (all route IDs must exist) | ||
| const snapshot = opts?.snapshot | ||
| const snapshotValid = | ||
| snapshot && | ||
| snapshot.routeIds.length > 0 && | ||
| snapshot.routeIds.every((id) => this.routesById[id]) | ||
|
|
||
| let matchedRoutes: ReadonlyArray<AnyRoute> | ||
| let routeParams: Record<string, string> | ||
| let globalNotFoundRouteId: string | undefined | ||
| let parsedParams: Record<string, unknown> | undefined | ||
|
|
||
| if (snapshotValid) { | ||
| // Rebuild matched routes from snapshot | ||
| matchedRoutes = snapshot.routeIds.map((id) => this.routesById[id]!) | ||
| routeParams = { ...snapshot.params } | ||
| globalNotFoundRouteId = snapshot.globalNotFoundRouteId | ||
| parsedParams = undefined // Not available from snapshot, will re-parse if needed | ||
| } else { |
There was a problem hiding this comment.
Guard against undefined parsedParams in snapshot fast-path.
When skipRouteOnParseError is enabled, later logic uses key in parsedParams. With the snapshot path, parsedParams is set to undefined, which can throw a TypeError. Use an empty object (or a guard) to avoid crashes.
🐛 Proposed fix
- parsedParams = undefined // Not available from snapshot, will re-parse if needed
+ // Not available from snapshot; keep empty to avoid undefined access later
+ parsedParams = {}📝 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.
| // Fast-path: use snapshot hint if valid (all route IDs must exist) | |
| const snapshot = opts?.snapshot | |
| const snapshotValid = | |
| snapshot && | |
| snapshot.routeIds.length > 0 && | |
| snapshot.routeIds.every((id) => this.routesById[id]) | |
| let matchedRoutes: ReadonlyArray<AnyRoute> | |
| let routeParams: Record<string, string> | |
| let globalNotFoundRouteId: string | undefined | |
| let parsedParams: Record<string, unknown> | undefined | |
| if (snapshotValid) { | |
| // Rebuild matched routes from snapshot | |
| matchedRoutes = snapshot.routeIds.map((id) => this.routesById[id]!) | |
| routeParams = { ...snapshot.params } | |
| globalNotFoundRouteId = snapshot.globalNotFoundRouteId | |
| parsedParams = undefined // Not available from snapshot, will re-parse if needed | |
| } else { | |
| // Fast-path: use snapshot hint if valid (all route IDs must exist) | |
| const snapshot = opts?.snapshot | |
| const snapshotValid = | |
| snapshot && | |
| snapshot.routeIds.length > 0 && | |
| snapshot.routeIds.every((id) => this.routesById[id]) | |
| let matchedRoutes: ReadonlyArray<AnyRoute> | |
| let routeParams: Record<string, string> | |
| let globalNotFoundRouteId: string | undefined | |
| let parsedParams: Record<string, unknown> | undefined | |
| if (snapshotValid) { | |
| // Rebuild matched routes from snapshot | |
| matchedRoutes = snapshot.routeIds.map((id) => this.routesById[id]!) | |
| routeParams = { ...snapshot.params } | |
| globalNotFoundRouteId = snapshot.globalNotFoundRouteId | |
| // Not available from snapshot; keep empty to avoid undefined access later | |
| parsedParams = {} | |
| } else { |
🤖 Prompt for AI Agents
In `@packages/router-core/src/router.ts` around lines 1295 - 1313, The snapshot
fast-path sets parsedParams = undefined which causes a TypeError later when code
checks "key in parsedParams" if skipRouteOnParseError is enabled; change the
snapshot branch to initialize parsedParams to an empty object (e.g.,
parsedParams = {}) or otherwise ensure parsedParams is always an object when
snapshotValid is true so subsequent checks in the router (referenced by
parsedParams, skipRouteOnParseError, snapshotValid, matchedRoutes, routeParams)
won't throw.
| const snapshotValid = | ||
| snapshot && | ||
| snapshot.routeIds.length > 0 && | ||
| snapshot.routeIds.every((id) => this.routesById[id]) |
There was a problem hiding this comment.
Why is this check necessary? (mainly snapshot.routeIds.every((id) => this.routesById[id]))
Can we not trust what is passed here?
There was a problem hiding this comment.
theoretically if you reload you could get a new bundle served. this check tries to detect if the route tree changed. however it's not sufficient probably as the routes might still exist but would match otherwise.
There was a problem hiding this comment.
this checks if all snapshot ids exist in the routes, is this sufficient in the case where there are new routes that hasn't been loaded in the snapshot yet? specifically, is this.routeIds.every((id) => snapshot.routesById[id]) not necessary?
schiller-manuel
force-pushed
the
avoid-reparse
branch
from
ffc5d6c to
4279c5c
Compare
| const rewrittenUrl = executeRewriteOutput(this.rewrite, url) | ||
|
|
||
| // Use encoded URL path for href (consistent with parseLocation) | ||
| const encodedHref = url.href.replace(url.origin, '') |
There was a problem hiding this comment.
can probably just build this using url.pathname + url.search + url.hash here
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router-core/src/router.ts (1)
1858-1891:href/urlinvariant breaks after output rewrite.
hrefis computed from the pre‑rewrite URL whileurlis stored as the rewritten (public) URL. This violates the ParsedLocation contract (hrefshould be derived fromurl) and can desyncpathname/hreffromurlwhen rewrites change origin or path.🐛 Proposed fix
return { publicHref: rewrittenUrl.pathname + rewrittenUrl.search + rewrittenUrl.hash, href: encodedHref, - url: rewrittenUrl, + url, pathname: nextPathname,
🧹 Nitpick comments (2)
packages/history/src/index.ts (2)
34-43: Consider aligning go/back/forward with async results (or document the asymmetry).
With push/replace now returningPromise<NavigationResult>, consumers may expect similar affordances for other navigation methods. If you intend to keep them sync, a brief doc note would prevent confusion.
148-155: ExposenavigateOptsonRouterHistory.notifyfor custom histories.
Sincenotifynow forwardsnavigateOptsto subscribers, consider widening the publicRouterHistory.notifysignature to(action, navigateOpts?)so custom history implementations can propagate options without casting.
|
I think this exact PR broke my apps navigation. Before @tanstack/react-router@1.153.2 it worked and from this version it does not. Details: First when navigating to the article page I do it like that: Then on my article page I have back button which uses When I click on it, router shows 404 page, when I reload page then it shows correct page without any URL changes. Should it be done differently from now on or something is off? |
|
my route params is always undefined after this PR changes import { createRootRouteWithContext, createRoute } from '@tanstack/react-router';
const rootRoute = createRootRouteWithContext()();
const layoutRoute = createRoute({
getParentRoute: () => rootRoute,
id: 'layout',
component: AppPageLayout,
});
const lobbyRoute = createRoute({
getParentRoute: () => layoutRoute,
path: '/',
}).lazy(() => import('./lobby/LobbyPage').then((m) => m.Route));
const itemsIndexRoute = createRoute({
getParentRoute: () => layoutRoute,
path: 'items',
}).lazy(() => import('./item/ItemsPage').then((m) => m.Route));
const itemRoute = createRoute({
getParentRoute: () => rootRoute,
path: 'items/view/$itemId',
loader: (props) => {
// props.params.itemId is undefined after @tanstack/react-router@1.153.2
},
}).lazy(() => import('./item/ItemPage').then((m) => m.Route));
export const routeTree = rootRoute.addChildren([
itemRoute,
layoutRoute.addChildren([itemsIndexRoute]),
lobbyRoute
]); |
|
Seeing a similar issue, our app with lots of search param usage now causes a "not found" every time search params change. Once you reload the page, it works. |
Ignore me, it does not work, went back to stable version |
Yep, version |
|
we reverted this and will revisit this perf optimization at a later point |
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.