refactor(router-core): simplify matchId #5807
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughRemoved the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (router / devtools / matcher)
participant Path as interpolatePath
participant Matcher as matchRoutesInternal
rect rgb(240,248,255)
Caller->>Path: interpolatePath(pathTemplate, params, options?)
Note right of Path `#dfefff`: Wildcards/splats are always\nsubstituted & URL-encoded (no leaveWildcards)
Path-->>Caller: interpolatedPath
end
rect rgb(245,255,240)
Caller->>Matcher: request match / build matchId
Matcher->>Path: interpolatePath(route.id, params)
Path-->>Matcher: interpolatedPath
Matcher-->>Caller: matchId = route.id + interpolatedPath + loaderDepsHash
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/{react-router,solid-router}/**📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-09-23T17:36:12.598ZApplied to files:
📚 Learning: 2025-10-08T08:11:47.088ZApplied to files:
📚 Learning: 2025-09-28T21:41:45.233ZApplied to files:
⏰ 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)
🔇 Additional comments (2)
Comment |
|
View your CI Pipeline Execution ↗ for commit b9fcdfa
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/nitro-v2-vite-plugin
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@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-router-ssr-query
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-static-server-functions
@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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
e2e/react-router/basic-file-based/tests/params.spec.ts (1)
2-2: Consider removing the unused import.The
useExperimentalNonNestedRoutesimport appears to be unused after removing the conditional logic based on the experimental flag. Cleaning up unused imports improves code clarity.Apply this diff to remove the unused import:
-import { useExperimentalNonNestedRoutes } from './utils/useExperimentalNonNestedRoutes'e2e/solid-router/basic-file-based/tests/params.spec.ts (1)
2-2: Consider removing the unused import.The
useExperimentalNonNestedRoutesimport appears to be unused after removing the conditional logic based on the experimental flag. Cleaning up unused imports improves code clarity.Apply this diff to remove the unused import:
-import { useExperimentalNonNestedRoutes } from './utils/useExperimentalNonNestedRoutes'packages/router-core/tests/callbacks.test.ts (1)
67-67: Consider standardizing navigate() paths for clarity.The navigate calls mix two forms:
- Full ID form:
navigate({ to: '/foo/foo' })(lines 67, 70, 89, 99)- Short path form:
navigate({ to: '/foo' })(lines 77, 92)While both forms appear to work, standardizing on one form would improve test clarity and reduce confusion about which format to use.
Consider applying this pattern consistently - using the short path form throughout:
-await router.navigate({ to: '/foo/foo' }) +await router.navigate({ to: '/foo' }) -await router.navigate({ to: '/bar/bar' }) +await router.navigate({ to: '/bar' }) -await router.navigate({ to: '/foo/foo', search: { foo: 'quux' } }) +await router.navigate({ to: '/foo', search: { foo: 'quux' } })Also applies to: 70-70, 77-77, 89-89, 92-92, 99-99
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e/react-router/basic-file-based/tests/non-nested-paths.spec.ts(2 hunks)e2e/react-router/basic-file-based/tests/params.spec.ts(1 hunks)e2e/react-start/basic/tests/navigation.spec.ts(1 hunks)e2e/solid-router/basic-file-based/tests/non-nested-paths.spec.ts(2 hunks)e2e/solid-router/basic-file-based/tests/params.spec.ts(1 hunks)packages/router-core/tests/callbacks.test.ts(3 hunks)packages/router-core/tests/load.test.ts(9 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
e2e/solid-router/basic-file-based/tests/non-nested-paths.spec.tse2e/react-router/basic-file-based/tests/non-nested-paths.spec.tse2e/react-start/basic/tests/navigation.spec.tse2e/solid-router/basic-file-based/tests/params.spec.tspackages/router-core/tests/load.test.tse2e/react-router/basic-file-based/tests/params.spec.tspackages/router-core/tests/callbacks.test.ts
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/solid-router/basic-file-based/tests/non-nested-paths.spec.tse2e/react-router/basic-file-based/tests/non-nested-paths.spec.tse2e/react-start/basic/tests/navigation.spec.tse2e/solid-router/basic-file-based/tests/params.spec.tse2e/react-router/basic-file-based/tests/params.spec.ts
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/tests/load.test.tspackages/router-core/tests/callbacks.test.ts
🧠 Learnings (8)
📓 Common learnings
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Learnt from: nlynzaad
Repo: TanStack/router PR: 5182
File: e2e/react-router/basic-file-based/src/routes/non-nested/named/$baz_.bar.tsx:3-5
Timestamp: 2025-09-22T00:56:49.237Z
Learning: In TanStack Router, underscores are intentionally stripped from route segments (e.g., `$baz_` becomes `baz` in generated types) but should be preserved in base path segments. This is the correct behavior as of the fix in PR #5182.
Learnt from: nlynzaad
Repo: TanStack/router PR: 5284
File: e2e/react-start/basic/server.js:50-0
Timestamp: 2025-09-28T21:41:45.233Z
Learning: In Express v5, catch-all routes must use named wildcards. Use `/*splat` to match everything except root path, or `/{*splat}` (with braces) to match including root path. The old `*` syntax is not allowed and will cause "Missing parameter name" errors. This breaking change requires explicit naming of wildcard parameters.
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
e2e/solid-router/basic-file-based/tests/non-nested-paths.spec.tse2e/react-router/basic-file-based/tests/non-nested-paths.spec.tse2e/solid-router/basic-file-based/tests/params.spec.tspackages/router-core/tests/load.test.tse2e/react-router/basic-file-based/tests/params.spec.tspackages/router-core/tests/callbacks.test.ts
📚 Learning: 2025-10-14T18:59:33.990Z
Learnt from: FatahChan
Repo: TanStack/router PR: 5475
File: e2e/react-start/basic-prerendering/src/routes/redirect/$target/via-beforeLoad.tsx:8-0
Timestamp: 2025-10-14T18:59:33.990Z
Learning: In TanStack Router e2e test files, when a route parameter is validated at the route level (e.g., using zod in validateSearch or param validation), switch statements on that parameter do not require a default case, as the validation ensures only expected values will reach the switch.
Applied to files:
e2e/solid-router/basic-file-based/tests/non-nested-paths.spec.tse2e/react-router/basic-file-based/tests/non-nested-paths.spec.tse2e/solid-router/basic-file-based/tests/params.spec.tspackages/router-core/tests/load.test.tse2e/react-router/basic-file-based/tests/params.spec.tspackages/router-core/tests/callbacks.test.ts
📚 Learning: 2025-10-09T12:59:02.129Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/src/styles/app.css:19-21
Timestamp: 2025-10-09T12:59:02.129Z
Learning: In e2e test directories (paths containing `e2e/`), accessibility concerns like outline suppression patterns are less critical since the code is for testing purposes, not production use.
Applied to files:
e2e/solid-router/basic-file-based/tests/non-nested-paths.spec.tse2e/react-router/basic-file-based/tests/non-nested-paths.spec.ts
📚 Learning: 2025-09-23T17:36:12.598Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to packages/{react-router,solid-router}/** : Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Applied to files:
e2e/react-router/basic-file-based/tests/non-nested-paths.spec.ts
📚 Learning: 2025-10-09T12:59:14.842Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/public/site.webmanifest:2-3
Timestamp: 2025-10-09T12:59:14.842Z
Learning: In e2e test fixtures (files under e2e directories), empty or placeholder values in configuration files like site.webmanifest are acceptable and should not be flagged unless the test specifically validates those fields.
Applied to files:
e2e/react-router/basic-file-based/tests/non-nested-paths.spec.ts
📚 Learning: 2025-09-23T17:36:12.598Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to packages/router-core/** : Keep framework-agnostic core router logic in packages/router-core/
Applied to files:
packages/router-core/tests/load.test.ts
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
packages/router-core/tests/load.test.ts
⏰ 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 (10)
e2e/react-start/basic/tests/navigation.spec.ts (1)
12-12: LGTM! Improved test reliability and consistency.Adding
await page.waitForURL('/')ensures the navigation is fully complete before proceeding with test interactions. This brings the first test in line with all other tests in the file, which already follow this pattern.e2e/react-router/basic-file-based/tests/params.spec.ts (1)
103-108: LGTM! Test simplification aligns with router core refactor.The unconditional expectations correctly reflect the simplified matchId construction and removal of experimental flag branches. This makes the test suite cleaner and more maintainable.
e2e/solid-router/basic-file-based/tests/params.spec.ts (1)
102-107: LGTM! Test simplification aligns with router core refactor.The unconditional expectations correctly reflect the simplified matchId construction and removal of experimental flag branches. This makes the test suite cleaner and more maintainable.
e2e/solid-router/basic-file-based/tests/non-nested-paths.spec.ts (2)
187-187: LGTM! Simplified non-nested path expectation.The unconditional expectation of
paramValue2removes experimental flag branching and aligns with the router core simplification in this PR.
346-348: LGTM! Consistent expectation for deeply nested paths.The unconditional expectation standardizes the deeply nested non-nested path behavior, removing conditional logic and making the test suite cleaner.
e2e/react-router/basic-file-based/tests/non-nested-paths.spec.ts (2)
187-187: LGTM! Simplified non-nested path expectation.The unconditional expectation of
paramValue2removes experimental flag branching and aligns with the router core simplification in this PR.
346-348: LGTM! Consistent expectation for deeply nested paths.The unconditional expectation standardizes the deeply nested non-nested path behavior, removing conditional logic and making the test suite cleaner.
packages/router-core/tests/load.test.ts (2)
54-54: LGTM! Route ID expectations correctly updated.All test assertions consistently updated from
id: '/foo'toid: '/foo/foo'to reflect the new matchId construction that concatenates route.id + interpolatedPath + loaderDepsHash.Also applies to: 61-61, 76-76, 90-90, 236-236, 243-243, 258-258, 271-271, 285-285
497-497: LGTM! getMatch calls correctly updated.The
router.getMatch()calls properly updated to use the new fully-qualified route ID format.Also applies to: 508-508
packages/router-core/tests/callbacks.test.ts (1)
51-51: LGTM! Test expectations correctly updated.All callback test assertions consistently updated to expect fully-qualified route IDs (
/foo/foo,/bar/bar) aligning with the new matchId construction.Also applies to: 58-58, 73-73, 80-80, 95-95, 102-102
nlynzaad
approved these changes
Nov 10, 2025
This was referenced Nov 11, 2025
Merged
roduyemi
pushed a commit
to roduyemi/oss-router
that referenced
this pull request
Nov 19, 2025
* refactor(router-core): simplify matchId * ci: apply automated fixes * update tests * matchId !== routeId * nitpicks * missed in solid router Match --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Nico Lynzaad <[email protected]>
This was referenced Nov 21, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As far as I can tell, the
matchIddon't need to make an expensive call tointerpolatePath, and instead we can simply concatenate the strings that cover all the values we need to index on:route.idfor exhaustive route disambiguationinterpolatedPath(already needed for something else) to include all the paramsloaderDepsHashfor explicit loader dependenciesThis removes 1 call to
interpolatePathper route in a "branch" (/foo/bar/baz=> 3 calls) for every time we callpreloadRoute(hover on any link),beforeLoad(navigate to any route),buildLocation(rendering any link).interpolatePathis not cheap (parse pathname, encode every param, build a string).Bonus change: with this change, we don't need the
leaveWildcardsoption oninterpolatePathanymore, which is a good step towards making this function more performant.Summary by CodeRabbit
Refactor
Tests