Fix clearExpiredCache/AgeTicker on removed routes #5201
Conversation
WalkthroughUpdates in router-core and devtools replace non-null assertions with optional access when resolving routes and checking loaders. Guard conditions now short-circuit when a route is missing or lacks a loader, affecting control flow by safely handling undefined routes during loader checks and AgeTicker rendering. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor C as Caller
participant R as Router
participant LR as looseRoutesById
C->>R: getHasLoader(routeId)
R->>LR: route = LR[routeId]
alt route missing or no loader
R-->>C: true (no loader required)
else loader present
R-->>C: false (loader exists)
end
sequenceDiagram
autonumber
participant UI as AgeTicker
participant RT as Router
participant LR as looseRoutesById
UI->>RT: resolve match.routeId
RT->>LR: route = LR[routeId]
alt route missing or no loader
UI-->>UI: render null
else loader present
UI-->>UI: render age ticker
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CC-Bell
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/router-core/src/router.ts (2)
2052-2056: Possible crash when calling hooks for exiting/staying/entering matches if a route was removed.Accessing
this.looseRoutesById[match.routeId]!.options[...]will throw when the route no longer exists (the exact scenario this PR targets). Use optional chaining to skip missing routes safely.Apply this diff:
- ).forEach(([matches, hook]) => { - matches.forEach((match) => { - this.looseRoutesById[match.routeId]!.options[hook]?.(match) - }) - }) + ).forEach(([matches, hook]) => { + matches.forEach((match) => { + this.looseRoutesById[match.routeId]?.options[hook]?.(match) + }) + })
1-1: Fix unsafe non-null assertions for route lookups (looseRoutesById / routesById)Multiple occurrences of non-null assertions remain — replace with guarded lookups or explicit handling to avoid runtime crashes.
- packages/router-core/src/router.ts:1434, 1592, 2054
- packages/router-core/src/ssr/ssr-client.ts:96, 104, 166
- packages/router-core/src/load-matches.ts:164, 488, 723
- packages/solid-router/src/HeadContent.tsx:103
- packages/solid-router/src/Scripts.tsx:19
- packages/react-router/src/HeadContent.tsx:105
- packages/react-router/src/Scripts.tsx:19
- (also: routesById non-null assertions) packages/solid-router/src/Match.tsx:216, 337; packages/react-router/src/Match.tsx:316
🧹 Nitpick comments (2)
packages/router-core/src/router.ts (2)
1434-1469: Optional: consider guarding route lookups consistently.Elsewhere we still assume the route exists via non-null assertions. While typically safe for current matches resolved from the active route tree, using optional chaining makes hot-reload/module-federation updates more resilient.
Example:
- const route = this.looseRoutesById[match.routeId]! + const route = this.looseRoutesById[match.routeId] + if (!route) return
1590-1594: Optional: safe access when mapping destination routes.These are derived from current matching, so the route should exist. Still, optional chaining here would align with defensive handling in dynamic trees.
- }).map((d) => this.looseRoutesById[d.routeId]!) + }).map((d) => this.looseRoutesById[d.routeId]).filter(Boolean) as Array<typeof this.flatRoutes[number]>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-core/src/router.ts(1 hunks)packages/router-devtools-core/src/AgeTicker.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-devtools-core/src/AgeTicker.tsxpackages/router-core/src/router.ts
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/router.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: TanStack/router#0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.589Z
Learning: Applies to packages/router-core/** : Keep framework-agnostic core router logic in packages/router-core/
Learnt from: CR
PR: TanStack/router#0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.589Z
Learning: Applies to packages/{router-devtools,*-router-devtools}/** : Keep router devtools packages in packages/router-devtools/ and packages/*-router-devtools/
🔇 Additional comments (3)
packages/router-core/src/router.ts (2)
2262-2267: Good fix: guard against missing routes in cache GC.Switching to a safe lookup and early-evicting when the route is missing or has no loader prevents crashes when routes are removed dynamically.
2238-2256: Behavior note: filter semantics remain the same.Returning
trueremoves entries. This continues to eagerly drop cached matches for routes without loaders, which is consistent with previous behavior.If intended, no action needed. If you want to retain non-loader routes in cache, invert the condition.
packages/router-devtools-core/src/AgeTicker.tsx (1)
45-49: LGTM once route narrowing is fixed.Using defaults from the router options is correct; rendering null when no loader keeps devtools quiet for non-data routes or removed routes.
| const route = router().looseRoutesById[match.routeId] | ||
|
|
||
| if (!route.options.loader) { | ||
| if (!route?.options.loader) { | ||
| return null |
There was a problem hiding this comment.
Type-safety: add explicit null check to narrow route.
if (!route?.options.loader) does not narrow route in TS strict mode, and route.options.* below will error. Guard route first.
Apply this diff:
- const route = router().looseRoutesById[match.routeId]
-
- if (!route?.options.loader) {
+ const route = router().looseRoutesById[match.routeId]
+ if (!route || !route.options.loader) {
return null
}📝 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 route = router().looseRoutesById[match.routeId] | |
| if (!route.options.loader) { | |
| if (!route?.options.loader) { | |
| return null | |
| const route = router().looseRoutesById[match.routeId] | |
| if (!route || !route.options.loader) { | |
| return null |
🤖 Prompt for AI Agents
In packages/router-devtools-core/src/AgeTicker.tsx around lines 38 to 41, the
current check uses optional chaining (if (!route?.options.loader)) which does
not narrow `route` under TS strict mode and will cause later accesses to
`route.options` to error; change the guard to first check for `route` explicitly
(for example: if (!route || !route.options.loader) return null) so TypeScript
narrows `route` and subsequent `route.options.*` access is safe.
| const filter = (d: MakeRouteMatch<TRouteTree>) => { | ||
| const route = this.looseRoutesById[d.routeId]! | ||
| const route = this.looseRoutesById[d.routeId] | ||
|
|
There was a problem hiding this comment.
should we not just return here if route is undefined?
I am working on a application where we update the
routeTreeas new parts of the application is loaded (Module Federation architecture). When a new route is registered, these functions error out since the previous route no longer exists.Summary by CodeRabbit