refactor(router-core): route matching only needs string parts for 'from', not full segments (WIP) #5054
Conversation
…om', not full segments
WalkthroughRefactors path parsing to use two caches—ParseRouteCache for route-segment parsing and ParsePathCache for string-based path splits. Updates path parsing, matching, resolving, and RouterCore to pass and manage both caches, replacing the prior single-cache approach and updating public signatures and exports. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant RouterCore
participant PathModule as path.ts
participant RouteCache as ParseRouteCache
participant PathCache as ParsePathCache
App->>RouterCore: navigate(to, base)
RouterCore->>PathModule: resolvePath({ to, base, parseRouteCache: RouteCache, parsePathCache: PathCache })
activate PathModule
PathModule->>RouteCache: get/lookup(route pattern → Segment[])
alt route cache miss
PathModule-->>RouteCache: set(route pattern → Segment[])
end
PathModule->>PathCache: get/lookup(path string → string[])
alt path cache miss
PathModule-->>PathCache: set(path string → string[])
end
PathModule-->>RouterCore: resolvedPath (joined from string[] and Segment[])
deactivate PathModule
RouterCore->>PathModule: matchPathname({ pathname, base, parseRouteCache: RouteCache, parsePathCache: PathCache })
activate PathModule
PathModule->>RouteCache: get(route pattern)
PathModule->>PathCache: get(split pathname)
PathModule-->>RouterCore: match result (params / matched route)
deactivate PathModule
RouterCore-->>App: routing outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 (
|
|
View your CI Pipeline Execution ↗ for commit 7816b7a
☁️ 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 (2)
packages/router-core/src/path.ts (1)
274-355: Consider extracting segment creation logic for better maintainability.While the implementation is correct, the
baseParsePathnamefunction has become quite long with repetitive segment creation patterns. Consider extracting helper functions for creating each segment type to reduce duplication and improve readability.For example:
+function createWildcardSegment(prefix?: string, suffix?: string): Segment { + return { + type: SEGMENT_TYPE_WILDCARD, + value: '$', + prefixSegment: prefix || undefined, + suffixSegment: suffix || undefined, + } +} + +function createOptionalParamSegment(paramName: string, prefix?: string, suffix?: string): Segment { + return { + type: SEGMENT_TYPE_OPTIONAL_PARAM, + value: paramName, + prefixSegment: prefix || undefined, + suffixSegment: suffix || undefined, + } +} function baseParsePathname(pathname: string): ReadonlyArray<Segment> { // ... existing code ... if (wildcardBracesMatch) { - return { - type: SEGMENT_TYPE_WILDCARD, - value: '$', - prefixSegment: prefix || undefined, - suffixSegment: suffix || undefined, - } + return createWildcardSegment(prefix, suffix) }packages/router-core/src/router.ts (1)
1370-1373: Consider making cache sizes configurable for different use cases.While 500 entries is a reasonable default, different applications might benefit from different cache sizes. The comment about potentially using a plain Map for
parseRouteCache(sized to the exact number of routes) is a good future optimization to consider.For a future enhancement, consider:
- Making cache sizes configurable through router options
- Using a plain Map for
parseRouteCachesince the number of routes is known and fixed- Monitoring cache hit rates in development mode to help tune sizes
📜 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/path.ts(21 hunks)packages/router-core/src/router.ts(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/router-core/src/router.ts (2)
packages/router-core/src/path.ts (2)
ParseRouteCache(225-225)ParsePathCache(224-224)packages/router-core/src/lru-cache.ts (1)
createLRUCache(6-68)
packages/router-core/src/path.ts (2)
packages/router-core/src/lru-cache.ts (1)
LRUCache(1-4)packages/router-core/src/utils.ts (1)
last(187-189)
⏰ 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 (9)
packages/router-core/src/path.ts (7)
105-106: LGTM! Clean cache separation for improved performance.The refactoring from a single
parseCacheto separateparseRouteCacheappropriately distinguishes between route pattern parsing (which needs full segment information) and simple path splitting (which only needs strings).
164-165: Verify segment-to-string conversion preserves path semantics.The change from string-based path resolution to segment-based resolution with
parsePathnameis correct. The final conversion back to strings viasegmentToStringproperly handles all segment types including parameterized segments.
208-222: Well-designed generic caching wrapper.The
cacheMethodutility provides a clean abstraction for creating cached versions of parsing functions. The pattern of returning empty arrays for undefined keys and proper cache management is well implemented.
237-255: Proper URL decoding with special handling for encoded percent signs.The
baseSplitPathnamefunction correctly handles the edge case of%25(encoded percent sign) to prevent double-decoding issues. The normalization of leading/trailing slashes is also appropriate.
567-604: Efficient string-based matching with proper cache utilization.The updated
matchByPathcorrectly usessplitPathnamefor the base path (getting string arrays) andparsePathnamefor the route pattern (getting Segment arrays). This separation is the core optimization that avoids unnecessary regex processing for concrete paths.
607-608: Type signature correctly reflects the new string-based approach.The change from
Segment[]toReadonlyArray<string>forbaseSegmentsproperly enforces the optimization at the type level.
636-641: String comparisons are consistently updated throughout matching logic.All the places where
baseSegmentvalues are accessed have been properly updated to work with strings instead of segment objects. The comparisons now directly use the string values, which is more efficient.Also applies to: 701-704, 755-759, 770-770, 803-803, 825-825, 842-842
packages/router-core/src/router.ts (2)
38-38: Clear documentation of the dual-cache system.The comments clearly explain the purpose of each cache, which will help future maintainers understand the performance optimization.
Also applies to: 1370-1373
1043-1043: Consistent propagation of both caches throughout the codebase.All call sites have been properly updated to pass both
parseRouteCacheandparsePathCachewhere needed. The separation is maintained consistently across interpolation, matching, and resolution operations.Also applies to: 1225-1227, 1486-1488, 1513-1515, 1606-1608, 2287-2289, 2623-2625, 2648-2650
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/router-core/src/router.ts (2)
1370-1374: Type the Map and reset on route-tree changes to avoid stale entries.
- Use typed Map generics (or cast) to align with
ParseRouteCacheand prevent implicitany.- Consider resetting the route-segment cache when the route tree changes to bound growth if routes are hot-swapped.
Apply:
- /** a cache of parsed routes for `parsePathname` (e.g. `/foo/$bar` => Array<Segment>) */ - private parseRouteCache: ParseRouteCache = new Map() + /** a cache of parsed routes for `parsePathname` (e.g. `/foo/$bar` => Array<Segment>) */ + private parseRouteCache: ParseRouteCache = + new Map<string, ReadonlyArray<Segment>>() as ParseRouteCacheAnd inside the
updatebranch where the route tree changes:if (this.options.routeTree !== this.routeTree) { this.routeTree = this.options.routeTree as TRouteTree + // Reset route-segment parse cache when routes change + this.parseRouteCache = + new Map<string, ReadonlyArray<Segment>>() as ParseRouteCache this.buildRouteTree() }
1036-1044: Destructure and forwardparsePathCacheinresolvePathWithBase
resolvePathWithBasecurrently only passesparseRouteCacheintoparsePathname. To enable raw-path caching, extend its signature to acceptparsePathCacheand invokeparsePathname(base, parsePathCache)/parsePathname(to, parsePathCache).
📜 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(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/router-core/src/router.ts (2)
packages/router-core/src/path.ts (2)
ParseRouteCache(225-225)ParsePathCache(224-224)packages/router-core/src/lru-cache.ts (1)
createLRUCache(6-68)
⏰ 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 (7)
packages/router-core/src/router.ts (7)
38-38: Type imports match new dual-cache design — LGTM.
1225-1227: Consistent interpolation now uses both caches — good.
All interpolatePath call sites pass route- and path-caches, keeping parity and avoiding redundant parsing.Also applies to: 1486-1488, 1513-1515
1387-1389: Plumbing caches into getMatchedRoutes — LGTM.
1606-1608: Mask matching uses caches — LGTM.
2287-2289: matchRoute correctly forwards caches to matchPathname — LGTM.
2623-2635: getMatchedRoutes signature extended with optional caches — LGTM.
Backward compatible; internal callers pass both caches.
2648-2650: Cache-aware matchPathname usage inside getMatchedRoutes — LGTM.
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 (1)
packages/router-core/src/path.ts (1)
798-807: Respect caseSensitive in optional-param lookaheadLookahead equality uses strict case-sensitive compare regardless of caseSensitive, causing incorrect skips on case-insensitive routers.
Apply:
- if ( - futureRouteSegment?.type === SEGMENT_TYPE_PATHNAME && - futureRouteSegment.value === baseSegment - ) { + if (futureRouteSegment?.type === SEGMENT_TYPE_PATHNAME) { + const fv = caseSensitive + ? futureRouteSegment.value + : futureRouteSegment.value.toLowerCase() + const bv = caseSensitive ? baseSegment : baseSegment.toLowerCase() + if (fv === bv) { // The current base segment matches a future pathname segment, // so we should skip this optional parameter shouldMatchOptional = false break - } + } + }
🧹 Nitpick comments (3)
packages/router-core/src/path.ts (3)
636-647: Simplify wildcard prefix/suffix checksUsing the in operator on optional fields is misleading (it’s always true for Segment). Check prefix/suffix values directly.
Apply:
- // Check if the base segment starts with prefix and ends with suffix - if ('prefixSegment' in routeSegment) { - if (!baseSegment.startsWith(prefix)) { - return false - } - } - if ('suffixSegment' in routeSegment) { - if (!baseSegments[baseSegments.length - 1]?.endsWith(suffix)) { - return false - } - } + // Check if the base segment starts/ends with the given prefix/suffix + if (prefix && !baseSegment.startsWith(prefix)) return false + if (suffix && !baseSegments[baseSegments.length - 1]?.endsWith(suffix)) + return false
116-125: Return canonical "{$param}" when no prefix/suffixsegmentToString for bare params currently returns "$param". For consistency with optional params and brace syntax, emit "{$param}" when there’s no prefix/suffix.
Apply:
if (type === SEGMENT_TYPE_PARAM) { const param = value.substring(1) if (prefixSegment && suffixSegment) { return `${prefixSegment}{$${param}}${suffixSegment}` } else if (prefixSegment) { return `${prefixSegment}{$${param}}` } else if (suffixSegment) { return `{$${param}}${suffixSegment}` } + return `{${value}}` }
366-368: Unused parsePathCache in interpolatePath optionsparsePathCache is accepted but unused. Remove to avoid API noise, or mark as deprecated for a staged removal.
Option A (remove now):
interface InterpolatePathOptions { path?: string params: Record<string, unknown> leaveWildcards?: boolean leaveParams?: boolean // Map of encoded chars to decoded chars (e.g. '%40' -> '@') that should remain decoded in path params decodeCharMap?: Map<string, string> - parseRouteCache?: ParseRouteCache - parsePathCache?: ParsePathCache + parseRouteCache?: ParseRouteCache } ... export function interpolatePath({ path, params, leaveWildcards, leaveParams, decodeCharMap, - parseRouteCache, + parseRouteCache, }: InterpolatePathOptions): InterPolatePathResult {Option B (deprecate):
- parsePathCache?: ParsePathCache + /** @deprecated Unused; will be removed in a future version. */ + parsePathCache?: ParsePathCacheAlso applies to: 381-384
📜 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/path.ts(21 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/router-core/src/path.ts (3)
packages/router-core/src/route.ts (1)
AnyPathParams(45-45)packages/router-core/src/lru-cache.ts (1)
LRUCache(1-4)packages/router-core/src/utils.ts (1)
last(187-189)
🔇 Additional comments (2)
packages/router-core/src/path.ts (2)
648-666: Double-decoding risk for wildcard splatdecodeURI(joinPaths(remainingBaseSegments)) can undo the earlier %25 guard in baseSplitPathname, turning "%25" back into "%". Confirm this is intended for params. If not, consider decoding per-segment with decodeURIComponent and rejoining, or avoid decoding here and decode at consumption points.
Would you like a targeted test matrix (inputs with %, %2F, multi-byte chars) to lock behavior?
224-228: Dual-cache abstraction looks goodcacheMethod generalization and the split between ParsePathCache (strings) and ParseRouteCache (Segments) provide the intended perf win and cleaner semantics.
| // check basepath first | ||
| if (basepath !== '/' && !from.startsWith(basepath)) { | ||
| return undefined | ||
| } | ||
| // Remove the base path from the pathname | ||
| from = removeBasepath(basepath, from, caseSensitive) | ||
| // Default to to $ (wildcard) | ||
| // Default to `to = '$'` (wildcard) | ||
| to = removeBasepath(basepath, `${to ?? '$'}`, caseSensitive) |
There was a problem hiding this comment.
Fix case-insensitive basepath check in matchByPath
Using startsWith ignores the caseSensitive flag and can reject valid matches (e.g., basepath "/App" vs pathname "/app/..."). Rely on removeBasepath for the check.
Apply:
- // check basepath first
- if (basepath !== '/' && !from.startsWith(basepath)) {
- return undefined
- }
- // Remove the base path from the pathname
- from = removeBasepath(basepath, from, caseSensitive)
+ // Remove the base path from the pathname (respects caseSensitive)
+ const strippedFrom = removeBasepath(basepath, from, caseSensitive)
+ if (basepath !== '/' && strippedFrom === from) {
+ return undefined
+ }
+ from = strippedFrom📝 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.
| // check basepath first | |
| if (basepath !== '/' && !from.startsWith(basepath)) { | |
| return undefined | |
| } | |
| // Remove the base path from the pathname | |
| from = removeBasepath(basepath, from, caseSensitive) | |
| // Default to to $ (wildcard) | |
| // Default to `to = '$'` (wildcard) | |
| to = removeBasepath(basepath, `${to ?? '$'}`, caseSensitive) | |
| // Remove the base path from the pathname (respects caseSensitive) | |
| const strippedFrom = removeBasepath(basepath, from, caseSensitive) | |
| if (basepath !== '/' && strippedFrom === from) { | |
| return undefined | |
| } | |
| from = strippedFrom | |
| // Default to `to = '$'` (wildcard) | |
| to = removeBasepath(basepath, `${to ?? '$'}`, caseSensitive) |
🤖 Prompt for AI Agents
In packages/router-core/src/path.ts around lines 572 to 579, the current early
basepath check uses from.startsWith(basepath) which ignores the caseSensitive
flag and can incorrectly reject matches; remove that startsWith check and
instead call removeBasepath(basepath, from, caseSensitive) and return undefined
if it returns undefined, then set from to the returned value; likewise set to
using removeBasepath(basepath, `${to ?? '$'}`, caseSensitive) and handle
undefined there as before. Ensure both calls use the same caseSensitive
parameter.
Sheraff
changed the title
Description
matchByPathtakes 2 (main) arguments:to=> a route with custom params syntax, for example/foo/$barfrom=> a fully formed path (without params), for example/foo/123Currently, we handle both the same way:
However, in practice, the
fromargument doesn't need the 3rd step, we only need its string parts to match it against thetoroute.This PR proposes that we only do steps 1 and 2 on
from, and keep 1, 2, 3 onto. This change requires splitting the LRU cache into 2 caches for those 2 variants.Performance
This is very slightly (but consistently) beneficial in terms of performance, because path matching is already very costly, and the parsing itself is already cached.
matchByPathimplementations side by side.buildLocation, where we have a singlefromthat we try and match w/ manytoroutes.Additional improvements
With this change, we now have 2 caches:
parseRouteCachecaches astring => Segment[]transformation (steps 1, 2, 3)parsePathCachecaches astring => string[]transformation (steps 1, 2)And while
parsePathCachecould have an arbitrary size at runtime depending on navigations, theparseRouteCachewill only contain at most 1 entry per route. This meansparseRouteCachedoesn't need to be an LRU cache, its size is guaranteed to be reasonable, and it can be a regularMap.Warning
This is not exactly true, this cache is also used for relative
toroutes like'../foo/$bar'. This means depending on usage, it could also grow quite large.Making this additional change widens the gap in the benchmark:
Summary by CodeRabbit
Breaking Changes
Improvements
Performance