fix(router-core): parse non-nested path params correctly #5165
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
WalkthroughAdds support for non-nested trailing-underscore params ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App / Route defs
participant Router as Router Core
participant Parser as path.parse
participant Interp as path.interpolate
rect rgba(230,245,255,0.45)
note over App,Router: Register route containing `$param_`
App->>Router: register("/users/$id_")
Router->>Parser: parse("/users/$id_")
Parser-->>Router: segment { type: PARAM, name: "id" }
end
rect rgba(240,255,230,0.45)
note over App,Interp: Navigation with params
App->>Interp: interpolate("/users/$id_", { id: "123" })
Interp-->>App: "/users/123"
App->>Router: navigate("/users/123")
Router-->>App: matched route, params { id: "123" }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
nlynzaad
changed the title
|
View your CI Pipeline Execution ↗ for commit 112143d
☁️ 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: 3
🧹 Nitpick comments (2)
e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/route.tsx (1)
21-28: Two links render identical visible text; use distinct labels for a11y and clarity.Both Links show "/params-ps/non-nested/$foo/$bar" which makes screen-reader and debugging output ambiguous. Align with Solid example and reflect actual param values.
Apply this diff:
params={{ foo: 'foo2', bar: 'bar2' }} > - /params-ps/non-nested/$foo/$bar + /params-ps/non-nested/foo2/bar2 </Link>(Optional) Consider also updating the first link’s label (Lines 19-20) to "/params-ps/non-nested/foo/bar" for parity with the Solid route.
packages/router-core/tests/path.test.ts (1)
435-440: Nice coverage for interpolatePath with non‑nested syntax; add parse/match assertions too.Recommend adding parse and match tests to prevent regressions in the tokenization/matching path.
Example additions:
it('parsePathname should parse $id_ as a param named $id', () => { expect(parsePathname('/$id_')).toEqual([ { type: SEGMENT_TYPE_PATHNAME, value: '/' }, { type: SEGMENT_TYPE_PARAM, value: '$id' }, ]) }) it('matchPathname should match non-nested param', () => { expect(matchPathname('/', '/users/123', { to: '/users/$id_' })) .toStrictEqual({ id: '123' }) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/route.tsx(1 hunks)e2e/react-router/basic-file-based/tests/params.spec.ts(2 hunks)e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/route.tsx(1 hunks)e2e/solid-router/basic-file-based/tests/params.spec.ts(1 hunks)packages/router-core/src/path.ts(2 hunks)packages/router-core/tests/path.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/route.tsx (3)
e2e/react-router/basic-file-based/src/routes/params-ps/index.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/$bar.tsx (1)
Route(3-5)e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/route.tsx (1)
Route(3-3)
e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/route.tsx (3)
e2e/solid-router/basic-file-based/src/routes/params-ps/index.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/$bar.tsx (1)
Route(3-5)e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/$foo_/route.tsx (1)
Route(3-3)
⏰ 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)
e2e/solid-router/basic-file-based/src/routes/params-ps/non-nested/route.tsx (1)
19-27: LGTM: distinct, descriptive link labels.The labels match the concrete hrefs (foo/bar and foo2/bar2), improving a11y and test readability.
packages/router-core/src/path.ts (1)
323-332: Parsing branch looks correct; consider char‑set parity comment.Logic is fine and returns
$paramfor$param_. Once the regexes above are tightened, this branch inherits the stricter charset and remains exclusive from the regular param case.Please add a quick unit test to assert
parsePathname('/$id_')yields a singleSEGMENT_TYPE_PARAMwithvalue: '$id'to lock behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
e2e/react-router/basic-file-based/tests/params.spec.ts (3)
74-76: Minor: fold wait+assert into a single, auto‑waiting URL assertionSimplifies and reduces boilerplate.
- await page.waitForURL('/params-ps/non-nested/foo/bar') - const pagePathname = new URL(page.url()).pathname - expect(pagePathname).toBe('/params-ps/non-nested/foo/bar') + await expect(page).toHaveURL('/params-ps/non-nested/foo/bar')
94-97: Same here: compress navigation wait + assertionUse Playwright’s auto‑waiting matcher.
- await page.waitForURL('/params-ps/non-nested/foo2/bar2') - const pagePathname2 = new URL(page.url()).pathname - expect(pagePathname2).toBe('/params-ps/non-nested/foo2/bar2') + await expect(page).toHaveURL('/params-ps/non-nested/foo2/bar2')
99-107: Reduce flakiness: assert via locator matchers (auto‑wait) instead of manual innerText/JSON.parseLocators’ toHaveText retries until content settles; manual reads don’t. This avoids timing races after navigation.
- const foo2ParamsValue = page.getByTestId('foo-params-value') - const foo2ParamsText = await foo2ParamsValue.innerText() - const foo2ParamsObj = JSON.parse(foo2ParamsText) - expect(foo2ParamsObj).toEqual({ foo: 'foo2' }) + await expect(page.getByTestId('foo-params-value')).toHaveText( + JSON.stringify({ foo: 'foo2' }), + ) - const params2Value = page.getByTestId('foo-bar-params-value') - const params2Text = await params2Value.innerText() - const params2Obj = JSON.parse(params2Text) - expect(params2Obj).toEqual({ foo: 'foo2', bar: 'bar2' }) + await expect(page.getByTestId('foo-bar-params-value')).toHaveText( + JSON.stringify({ foo: 'foo2', bar: 'bar2' }), + )packages/router-core/tests/path.test.ts (3)
435-440: Add missing-param case for non-nested interpolationTo lock parity with regular named params, add a case where
$id_is missing and yieldsundefined.{ name: 'should interpolate the non-nested path', path: '/users/$id_', params: { id: '123' }, result: '/users/123', }, + { + name: 'should interpolate the non-nested path with missing params', + path: '/users/$id_', + params: {}, + result: '/users/undefined', + },
663-670: Add negative match to prove “non-nested” behaviorEnsure
/users/$id_does not match extra trailing segments.{ name: 'should match and return the non-nested named path params', input: '/users/123', matchingOptions: { to: '/users/$id_', }, expectedMatchedParams: { id: '123' }, }, + { + name: 'should not match when extra segment follows non-nested param', + input: '/users/123/extra', + matchingOptions: { + to: '/users/$id_', + }, + expectedMatchedParams: undefined, + },
910-918: Cover root-level non-nested parsingAdd a root variant to confirm
$bar_canonicalizes to$barat/.{ name: 'should handle non-nested named params', to: '/foo/$bar_', expected: [ { type: SEGMENT_TYPE_PATHNAME, value: '/' }, { type: SEGMENT_TYPE_PATHNAME, value: 'foo' }, { type: SEGMENT_TYPE_PARAM, value: '$bar' }, ], }, + { + name: 'should handle non-nested named params at the root', + to: '/$bar_', + expected: [ + { type: SEGMENT_TYPE_PATHNAME, value: '/' }, + { type: SEGMENT_TYPE_PARAM, value: '$bar' }, + ], + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/route.tsx(1 hunks)e2e/react-router/basic-file-based/tests/params.spec.ts(2 hunks)e2e/solid-router/basic-file-based/tests/params.spec.ts(2 hunks)packages/router-core/src/path.ts(2 hunks)packages/router-core/tests/path.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/router-core/src/path.ts
- e2e/react-router/basic-file-based/src/routes/params-ps/non-nested/route.tsx
- e2e/solid-router/basic-file-based/tests/params.spec.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/router-core/tests/path.test.ts (1)
packages/router-core/src/path.ts (2)
SEGMENT_TYPE_PATHNAME(6-6)SEGMENT_TYPE_PARAM(7-7)
⏰ 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 (1)
e2e/react-router/basic-file-based/tests/params.spec.ts (1)
90-93: LGTM: correct href assertion for the second linkGood coverage to ensure interpolation is right.
nlynzaad
added a commit
that referenced
this pull request
Sep 18, 2025
This PR continues on from #5165 and primarily adds additional tests to path resolution and matchByPath for non-nested paths in additiona to e2e test for both react and solid router to verify non-nesting functionality. One additional issue was picked up for handling of normal path segments. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - New Features - Added non-nested demo routes and navigation examples (including dynamic and edit paths) for React and Solid, with headings, links, and parameter displays. - Bug Fixes - Improved path parsing/decoding for non-nested segments to prevent unintended nesting and ensure correct parameter values. - Tests - Added broad unit tests for non-nested path parsing/matching/resolution (case-sensitive/insensitive, wildcards, optional/fuzzy). - Added e2e tests validating non-nested navigation and params; reduced e2e flakiness by awaiting network idle. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
nlynzaad
added a commit
that referenced
this pull request
Sep 18, 2025
nlynzaad
added a commit
that referenced
this pull request
Sep 18, 2025
This PR merges the latest updates to main from #5169 and #5165 into Alpha and brings with it an update to how non-nested paths are parsed. It also adds additional unit testing for paths and matchByPath as well as additional e2e tests for react and solid --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This was referenced Oct 19, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
When parsing path params to interpolate the path non-nested path params
/path/$param_was being parsed in the same manner as regular path params/path/$param.This PR adds consideration for the trailing underscore when parsing the path for non-nested path params as well as unit tests on path interpolation and e2e tests for useParams to ensure this is parsed correctly and updates correctly when params change.
This resolves #5164
Summary by CodeRabbit
New Features
Tests