refactor(router-core): more stable url decoding #5663
Conversation
WalkthroughAdds a tolerant path-segment decoder ( Changes
Sequence Diagram(s)sequenceDiagram
participant Consumer as Router Code (path.ts)
participant Utils as decodePathSegment
participant Native as decodeURIComponent
participant Router as buildLocation (router.ts)
Consumer->>Utils: request decodePathSegment(part, decodeIgnore?, startIndex?)
rect #f0f8ff
Note over Utils: scan for %XX and ignore delimiters\nsplit on ignore chars and recurse
Utils-->>Utils: recurse on subparts, preserve delimiters
end
rect #f7f0ff
Utils->>Native: decodeURIComponent(subpart)
Native-->>Utils: decoded value or throw
Utils-->>Utils: on throw -> preserve undecodable slice
end
Utils-->>Consumer: decoded segment
Consumer->>Router: continue parse/build with decoded segments
Router->>Utils: decodePathSegment(interpolatedPath)
Utils-->>Router: decoded interpolated path
Router->>Router: finalize location and params (emit)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit ba624dd
☁️ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/router-core/src/utils.ts (3)
528-541: Consider edge cases in recursive splitting logic.The recursive decoding splits by ignored characters and processes subparts, but there are potential edge cases:
Adjacent delimiters: If the input contains adjacent ignored characters (e.g.,
%25%5C), splitting by%25produces an empty string between delimiters. Empty strings are handled at line 526, but verify this doesn't cause issues.Ordering dependency: The function processes
decodeIgnorearray sequentially withstartIndex. This means the order of characters inDECODE_IGNORE_LISTaffects which character is used to split first. Document this behavior or consider if order matters for correctness.Performance: Deeply nested encoded sequences could cause many recursive calls. For typical URL segments this should be fine, but consider if there's a risk with pathological inputs.
492-504: Document why specific characters are in the ignore list.
DECODE_IGNORE_LISTincludes various special characters (%, \, /, ;, :, @, &, =, +, $, ,) but the rationale isn't immediately clear. Based on the PR objectives,%,\, and/are explicitly mentioned, but the others appear to be reserved URI characters from RFC 3986.Consider adding a comment explaining:
- Why each character or category is included
- The relationship to RFC 3986 reserved characters
- How this aligns with the PR's goal to prevent incorrect decoding of backslashes
Example:
+// Characters that should remain encoded in path segments: +// - % : percent itself to avoid double-decoding +// - \ : backslash (issue #3962) +// - / : path separator +// - Other reserved URI characters per RFC 3986: ; : @ & = + $ , const DECODE_IGNORE_LIST = [ '%', '\\',
526-526: Consider using stricter RFC 3986-compliant hex pattern for clarity.The regex
/%../gat line 525 (and line 516) is technically non-compliant with RFC 3986 percent-encoding, which requires two hexadecimal digits. As confirmed, it matches invalid sequences like%ggand%...However, the current implementation is functionally safe: invalid matches are caught by
decodeURIComponent()in the fallback (line 516), which throws and returns malformed sequences unchanged. The comment "leaving the malformed tags in place" suggests this resilient behavior is intentional.Recommendation: Either use the stricter pattern
/%[0-9A-Fa-f]{2}/gto enforce RFC 3986 compliance and improve clarity, or add a comment explaining why the permissive pattern is used for resilience. This will make the design choice explicit to future maintainers.All existing tests use valid hex sequences and pass; there is no test coverage for invalid sequences like
%gg.packages/router-core/tests/utils.test.ts (1)
502-511: Consider using actual path segments in test cases.While the test logic is correct, the function is named
decodePathSegmentbut tests use full URLs with query parameters. Consider using actual path segment examples (e.g.,/path/to/%D1%88%D0%B5%D0%BB%D0%BB%D1%8B) for better alignment with the function's intended use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
e2e/react-router/basic-file-based/src/routes/params-ps/index.tsx(1 hunks)e2e/react-router/basic-file-based/tests/params.spec.ts(1 hunks)e2e/solid-router/basic-file-based/src/routes/params-ps/index.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/src/router.ts(2 hunks)packages/router-core/src/utils.ts(1 hunks)packages/router-core/tests/path.test.ts(1 hunks)packages/router-core/tests/utils.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-core/src/path.tse2e/solid-router/basic-file-based/src/routes/params-ps/index.tsxe2e/solid-router/basic-file-based/tests/params.spec.tse2e/react-router/basic-file-based/tests/params.spec.tspackages/router-core/src/router.tspackages/router-core/tests/path.test.tspackages/router-core/src/utils.tspackages/router-core/tests/utils.test.tse2e/react-router/basic-file-based/src/routes/params-ps/index.tsx
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/path.tspackages/router-core/src/router.tspackages/router-core/tests/path.test.tspackages/router-core/src/utils.tspackages/router-core/tests/utils.test.ts
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/solid-router/basic-file-based/src/routes/params-ps/index.tsxe2e/react-router/basic-file-based/src/routes/params-ps/index.tsx
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/solid-router/basic-file-based/src/routes/params-ps/index.tsxe2e/solid-router/basic-file-based/tests/params.spec.tse2e/react-router/basic-file-based/tests/params.spec.tse2e/react-router/basic-file-based/src/routes/params-ps/index.tsx
🧠 Learnings (1)
📚 Learning: 2025-09-28T21:41:45.233Z
Learnt from: nlynzaad
PR: TanStack/router#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.
Applied to files:
e2e/solid-router/basic-file-based/src/routes/params-ps/index.tsxe2e/react-router/basic-file-based/src/routes/params-ps/index.tsx
🧬 Code graph analysis (4)
packages/router-core/src/path.ts (1)
packages/router-core/src/utils.ts (1)
decodePathSegment(506-544)
packages/router-core/src/router.ts (2)
packages/router-core/src/utils.ts (1)
decodePathSegment(506-544)packages/router-core/src/path.ts (1)
interpolatePath(403-517)
packages/router-core/tests/path.test.ts (1)
packages/router-core/src/path.ts (1)
SEGMENT_TYPE_PATHNAME(6-6)
packages/router-core/tests/utils.test.ts (1)
packages/router-core/src/utils.ts (1)
decodePathSegment(506-544)
⏰ 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 (11)
e2e/react-router/basic-file-based/src/routes/params-ps/index.tsx (1)
52-60: Good test coverage for escaped characters in wildcard parameters.The new link tests escaped sequences including backslashes, forward slashes, percent signs, and an emoji. This aligns well with the PR's goal of handling backslashes correctly in path parameters.
Note: The string literal
'test[s\\/.\\/parameter%!🚀]'correctly escapes backslashes for JavaScript string syntax. The\\/sequence becomes a single backslash followed by a forward slash in the runtime string value.e2e/solid-router/basic-file-based/src/routes/params-ps/index.tsx (1)
52-60: Consistent test coverage across frameworks.The test case matches the React Router implementation, ensuring consistent behavior for escaped wildcard parameters across both frameworks.
e2e/react-router/basic-file-based/tests/params.spec.ts (1)
293-301: Same verification needed for React Router tests.The test case is identical to the Solid Router version. See the comment on
e2e/solid-router/basic-file-based/tests/params.spec.tslines 368-376 for verification of the expected pathname encoding.packages/router-core/tests/path.test.ts (1)
835-842: Test correctly validates the new decoding behavior.The test case verifies that:
%25(percent) is preserved as%25after one decode pass%5C(backslash) is preserved as%5C- Cyrillic characters
%D1%88%D0%B5and%D0%BB%D0%BB%D1%8Bcorrectly decode toшеandллыThis aligns with the PR objective to prevent incorrect decoding of backslashes and percent signs while allowing normal character decoding.
packages/router-core/src/path.ts (2)
1-1: Import change consolidates decoding logic.The addition of
decodePathSegmentto the imports from./utilsenables the centralized decoding behavior introduced in this PR.
361-361: All verification points confirmed—no issues found.The control flow is correct:
- Parameter segments (lines 338-346) return early with
SEGMENT_TYPE_PARAMand are not decoded here- Wildcard segments (lines 349-356) return early with
SEGMENT_TYPE_WILDCARDand are not decoded here- Only the final return (lines 360-363) applies
decodePathSegment(), exclusively forSEGMENT_TYPE_PATHNAMEsegments- Parameters are decoded separately during matching (confirmed at lines 719, 775 in
matchByPath)packages/router-core/src/router.ts (2)
5-5: Import enables decoding in buildLocation.The import of
decodePathSegmentsupports applying the new decoding logic to interpolated paths in thebuildLocationmethod.
1674-1685: Review concern about double-encoding/decoding appears unfounded after code verification.The encoding/decoding flow is intentional and coordinated, not problematic:
encodePathParam(path.ts:519) usesencodeURIComponentthen selectively reverses encoding based ondecodeCharMapduring interpolation- The result still contains partially-encoded characters (those not in
decodeCharMap)decodePathSegment(utils.ts:506) performs final fulldecodeURIComponenton the interpolated pathThis is a controlled pattern where
decodeCharMappreserves specific encoded characters through interpolation (e.g., keeping "/" encoded for certain parameter types), thendecodePathSegmentdecodes the final result. No double-encoding occurs.e2e/solid-router/basic-file-based/tests/params.spec.ts (1)
368-376: Test expectation is correct—no issues found.The selective encoding in the test pathname is intentional. The code uses
encodeURI()for wildcard/splat params (lines 438–446 in path.ts), which preserves URI structural characters like/,[,],., and!, while still encoding special characters like backslashes (\), percent signs (%), and Unicode. This is the correct encoding strategy for wildcard parameters to preserve path structure while safely encoding dangerous characters.packages/router-core/tests/utils.test.ts (2)
2-7: LGTM!The import statement correctly includes the new
decodePathSegmentfunction alongside existing utilities.
513-569: Test coverage for ignore list behavior looks solid.These tests effectively validate that the function correctly preserves specified encoded characters while decoding others. The multiple test scenarios cover different combinations well.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/router-core/src/utils.ts (2)
492-504: Previous optimization suggestion still applicable.The past review comment about using a
SetforDECODE_IGNORE_LISTremains valid and would improve lookup performance in line 531. Since this feedback was already provided, I'm flagging this as duplicate.
526-544: Previous comment request still applicable.The past review comment requesting explanatory comments for this algorithm remains valid. The recursive splitting logic with
startIndexis clever but not immediately obvious to future maintainers.Additionally, a minor observation: the null-check
char &&on line 531 is unnecessary since array access within the loop bounds is always defined. However, this is defensive programming and harmless.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-core/src/utils.ts(1 hunks)packages/router-core/tests/utils.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/router-core/tests/utils.test.ts
🧰 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-core/src/utils.ts
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/utils.ts
🧬 Code graph analysis (1)
packages/router-core/src/utils.ts (1)
packages/router-core/src/index.ts (1)
decode(109-109)
⏰ 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 (1)
packages/router-core/src/utils.ts (1)
511-524: Well-designed error-tolerant decoding.The fallback mechanism that decodes individual percent-encoded sequences is a robust approach for handling malformed input. This ensures the function doesn't fail on partially invalid encodings.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/router-core/src/utils.ts (1)
506-510: Add explicit return type annotation for type safety.The exported function lacks an explicit return type annotation, which reduces type safety in strict TypeScript mode and makes the public API contract less clear.
Apply this diff:
export function decodePathSegment( part: string, decodeIgnore: Array<string> = Array.from(DECODE_IGNORE_LIST.values()), startIndex = 0, -) { +): string {
🧹 Nitpick comments (2)
packages/router-core/src/utils.ts (2)
492-504: Precompute the default ignore list to avoid repeated allocations.The default parameter in
decodePathSegmentcallsArray.from(DECODE_IGNORE_LIST.values())every time the function is invoked without an explicitdecodeIgnoreargument, creating unnecessary array allocations.Apply this diff to precompute the array:
const DECODE_IGNORE_LIST = new Map([ ['%', '%25'], ['\\', '%5C'], ['/', '%2F'], [';', '%3B'], [':', '%3A'], ['@', '%40'], ['&', '%26'], ['=', '%3D'], ['+', '%2B'], ['$', '%24'], [',', '%2C'], ]) + +const DECODE_IGNORE_ARRAY = Array.from(DECODE_IGNORE_LIST.values())Then update the function signature:
export function decodePathSegment( part: string, - decodeIgnore: Array<string> = Array.from(DECODE_IGNORE_LIST.values()), + decodeIgnore: Array<string> = DECODE_IGNORE_ARRAY, startIndex = 0, ): string {
511-527: Add return type to internal helper for consistency.The internal
decodehelper function should have an explicit return type annotation for consistency with strict TypeScript practices.Apply this diff:
- function decode(part: string) { + function decode(part: string): string { try {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/utils.ts(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-core/src/utils.ts
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/utils.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: Test
- GitHub Check: Preview
🔇 Additional comments (1)
packages/router-core/src/utils.ts (1)
529-558: Well-structured recursive decoding algorithm.The implementation correctly handles the recursive decoding logic:
- Splits on ignore characters
- Recursively processes each segment with incremented index
- Rejoins with the original delimiter preserved
- Falls back to full decode when no more ignore characters are found
The comments effectively explain the algorithm's approach.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/react-router/tests/link.test.tsx (1)
6556-6569: Fix component naming convention.The function
pathRouteComponentshould follow React naming conventions and bePathRouteComponent(PascalCase). This aligns with the pattern used throughout this file (e.g.,IndexComponent,PostsComponent,PostsIndexComponent).Apply this diff:
- function pathRouteComponent() { + function PathRouteComponent() { const params = pathRoute.useParams() return ( <div>e2e/react-router/basic-file-based/tests/params.spec.ts (1)
293-319: Align emoji encoding with Solid Router tests for consistency.The React test uses a hardcoded
%F0%9F%9A%80for the emoji (line 295), while the Solid Router equivalent uses`${encodeURIComponent('🚀')}`(Solid line 370). Both produce the same result, but usingencodeURIComponentis more explicit and maintainable.Apply this diff to align with the Solid approach:
{ id: 'l-to-wildcard-escaped', - pathname: `/params-ps/wildcard/test[s%5C/.%5C/parameter%25!%F0%9F%9A%80]`, + pathname: `/params-ps/wildcard/test[s%5C/.%5C/parameter%25!${encodeURIComponent('🚀')}]`, params: { _splat: 'test[s\\/.\\/parameter%!🚀]', '*': 'test[s\\/.\\/parameter%!🚀]', }, destHeadingId: 'ParamsWildcardSplat', },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
e2e/react-router/basic-file-based/src/routeTree.gen.ts(13 hunks)e2e/react-router/basic-file-based/src/routes/params-ps/index.tsx(3 hunks)e2e/react-router/basic-file-based/src/routes/params-ps/wildcard/prefix@대{$}.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/params-ps/wildcard/{$}suffix@대.tsx(1 hunks)e2e/react-router/basic-file-based/tests/params.spec.ts(1 hunks)e2e/solid-router/basic-file-based/src/routeTree.gen.ts(13 hunks)e2e/solid-router/basic-file-based/src/routes/params-ps/index.tsx(3 hunks)e2e/solid-router/basic-file-based/src/routes/params-ps/wildcard/prefix@대{$}.tsx(1 hunks)e2e/solid-router/basic-file-based/src/routes/params-ps/wildcard/{$}suffix@대.tsx(1 hunks)e2e/solid-router/basic-file-based/tests/params.spec.ts(1 hunks)packages/react-router/tests/link.test.tsx(1 hunks)packages/router-core/src/utils.ts(1 hunks)packages/solid-router/tests/link.test.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/router-core/src/utils.ts
- e2e/react-router/basic-file-based/src/routes/params-ps/index.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/src/routes/params-ps/index.tsxe2e/solid-router/basic-file-based/src/routes/params-ps/wildcard/prefix@대{$}.tsxe2e/react-router/basic-file-based/src/routes/params-ps/wildcard/prefix@대{$}.tsxe2e/react-router/basic-file-based/src/routes/params-ps/wildcard/{$}suffix@대.tsxe2e/solid-router/basic-file-based/src/routes/params-ps/wildcard/{$}suffix@대.tsxpackages/solid-router/tests/link.test.tsxe2e/react-router/basic-file-based/tests/params.spec.tse2e/solid-router/basic-file-based/tests/params.spec.tspackages/react-router/tests/link.test.tsxe2e/react-router/basic-file-based/src/routeTree.gen.tse2e/solid-router/basic-file-based/src/routeTree.gen.ts
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/solid-router/basic-file-based/src/routes/params-ps/index.tsxe2e/solid-router/basic-file-based/src/routes/params-ps/wildcard/prefix@대{$}.tsxe2e/react-router/basic-file-based/src/routes/params-ps/wildcard/prefix@대{$}.tsxe2e/react-router/basic-file-based/src/routes/params-ps/wildcard/{$}suffix@대.tsxe2e/solid-router/basic-file-based/src/routes/params-ps/wildcard/{$}suffix@대.tsx
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/solid-router/basic-file-based/src/routes/params-ps/index.tsxe2e/solid-router/basic-file-based/src/routes/params-ps/wildcard/prefix@대{$}.tsxe2e/react-router/basic-file-based/src/routes/params-ps/wildcard/prefix@대{$}.tsxe2e/react-router/basic-file-based/src/routes/params-ps/wildcard/{$}suffix@대.tsxe2e/solid-router/basic-file-based/src/routes/params-ps/wildcard/{$}suffix@대.tsxe2e/react-router/basic-file-based/tests/params.spec.tse2e/solid-router/basic-file-based/tests/params.spec.tse2e/react-router/basic-file-based/src/routeTree.gen.tse2e/solid-router/basic-file-based/src/routeTree.gen.ts
packages/{react-router,solid-router}/**
📄 CodeRabbit inference engine (AGENTS.md)
Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Files:
packages/solid-router/tests/link.test.tsxpackages/react-router/tests/link.test.tsx
🧠 Learnings (1)
📚 Learning: 2025-09-28T21:41:45.233Z
Learnt from: nlynzaad
PR: TanStack/router#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.
Applied to files:
e2e/solid-router/basic-file-based/src/routes/params-ps/index.tsx
🧬 Code graph analysis (2)
packages/solid-router/tests/link.test.tsx (1)
packages/react-router/src/link.tsx (1)
Link(567-598)
packages/react-router/tests/link.test.tsx (1)
packages/react-router/src/link.tsx (1)
Link(567-598)
⏰ 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 (12)
packages/react-router/tests/link.test.tsx (1)
6483-6527: Excellent test coverage for encoded and unicode paths.The test cases comprehensively cover different path patterns (prefix, suffix, wildcard, path param) with encoded characters, unicode, and special characters. The approach of testing both direct navigation (via
history.replaceState) and Link component navigation ensures the decoding behavior works correctly in both scenarios.packages/solid-router/tests/link.test.tsx (1)
6480-6598: LGTM! Comprehensive test coverage for encoded and unicode paths.The test suite effectively validates the PR's objectives:
- Tests preservation of special characters (backslash, percent, slash) in encoded form
- Covers multiple route patterns (splat with prefix/suffix, plain splat, path params)
- Validates both direct browser navigation and link-based navigation
- Verifies that unicode characters (emoji 🚀, Korean 대) are properly percent-encoded in URLs while params are correctly decoded
The test flow is thorough: it validates the href attribute on links, then actual navigation behavior, ensuring consistency between link generation and route matching.
Note: Ensure the different encoding behavior between splat params (where
/can appear unescaped as\/) vs path params (where/must be%2F) is intentional per the decodePathSegment implementation.e2e/react-router/basic-file-based/src/routes/params-ps/wildcard/{$}suffix@대.tsx (1)
1-15: React suffix@대 route OK.Route path, useParams, and test output are correct and consistent with other wildcard routes.
e2e/react-router/basic-file-based/src/routes/params-ps/wildcard/prefix@대{$}.tsx (1)
1-15: React prefix@대{$} route OK.Matches conventions; parameters surfaced for tests via data-testid.
e2e/solid-router/basic-file-based/src/routes/params-ps/wildcard/prefix@대{$}.tsx (1)
1-15: Solid prefix@대{$} route OK.Correct use of Route.useParams() accessor (p()) and test hook.
e2e/solid-router/basic-file-based/src/routes/params-ps/wildcard/{$}suffix@대.tsx (1)
1-15: Solid {$}suffix@대 route OK.Accessor usage and test output are consistent; no issues.
e2e/solid-router/basic-file-based/src/routes/params-ps/index.tsx (3)
88-96: Code snippet verified and consistent.The code at lines 88-96 matches the provided snippet exactly. Test cases at
e2e/solid-router/basic-file-based/tests/params.spec.ts:387and corresponding react-router tests validate the expected URL encoding behavior. The suffix variant follows established patterns.
52-60: Test coverage confirmed for escaped wildcard link.The data-testid
l-to-wildcard-escapedis properly referenced in both solid-router and react-router test suites (params.spec.tslines 369 and 294 respectively), with matching URL-encoded pathname expectations that align with the source code_splatvalue.
70-78: Wildcard prefix variant verified and consistent.The source code at lines 70–78 correctly implements the
prefix@대{$}route prefix with properly escaped_splatparameter. Path and label match, and the test expectations confirm correct URL encoding of the prefix and parameter values across both test suites.e2e/solid-router/basic-file-based/src/routeTree.gen.ts (1)
58-62: Route tree wiring for both new wildcard paths is confirmed and consistent.Both
/params-ps/wildcard/prefix@대{$}and/params-ps/wildcard/{$}suffix@대are present and properly wired across all sections: imports (lines 58, 61), consts (lines 339–343, 356–361), FileRoutesByFullPath/To/Id types (lines 616, 696, 787), module augmentation (line 1482, 1457–1460), and RootRouteChildren (lines 2114–2115). The file is auto-generated and no manual edits are required.e2e/solid-router/basic-file-based/tests/params.spec.ts (1)
368-394: LGTM! Comprehensive test coverage for escaped characters.The three new test cases properly validate the preservation of backslashes and other special characters in path parameters. The expected params correctly show decoded values with backslashes intact (
test[s\\/.\\/parameter%!🚀]), while the pathnames remain URL-encoded, which aligns with the PR's objectives.e2e/react-router/basic-file-based/src/routeTree.gen.ts (1)
59-62: Generated route imports look correct.The auto-generated imports for the new Korean wildcard routes follow the established naming convention and are properly integrated into the route tree.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/react-router/tests/link.test.tsx (3)
6529-6531: Prefertest.each(testCases)overObject.values(testCases).
testCasesis already an array;Object.valuesis redundant and discards type info, reducing strictness.Apply this diff:
- test.each(Object.values(testCases))( + test.each(testCases)(
6484-6527: Strengthen typing fortestCases(strict TS).Give
testCasesa readonly, explicit shape to preserve inference and catch mistakes early, while accommodating heterogeneousparamskeys (_splat,*,id).Apply this diff:
- const testCases = [ + const testCases = [ { name: 'with prefix', path: '/foo/prefix대{$}', browsePath: '/foo/prefix대test[s%5C/.%5C/parameter%25!🚀]', expectedPath: '/foo/prefix%EB%8C%80test[s%5C/.%5C/parameter%25!%F0%9F%9A%80]', params: { _splat: 'test[s\\/.\\/parameter%!🚀]', '*': 'test[s\\/.\\/parameter%!🚀]', }, }, { name: 'with suffix', path: '/foo/{$}대suffix', browsePath: '/foo/test[s%5C/.%5C/parameter%25!🚀]대suffix', expectedPath: '/foo/test[s%5C/.%5C/parameter%25!%F0%9F%9A%80]%EB%8C%80suffix', params: { _splat: 'test[s\\/.\\/parameter%!🚀]', '*': 'test[s\\/.\\/parameter%!🚀]', }, }, { name: 'with wildcard', path: '/foo/$', browsePath: '/foo/test[s%5C/.%5C/parameter%25!🚀]', expectedPath: '/foo/test[s%5C/.%5C/parameter%25!%F0%9F%9A%80]', params: { _splat: 'test[s\\/.\\/parameter%!🚀]', '*': 'test[s\\/.\\/parameter%!🚀]', }, }, // '/' is left as is with splat params but encoded with normal params { name: 'with path param', path: `/foo/$id`, browsePath: '/foo/test[s%5C%2F.%5C%2Fparameter%25!🚀]', expectedPath: '/foo/test[s%5C%2F.%5C%2Fparameter%25!%F0%9F%9A%80]', params: { id: 'test[s\\/.\\/parameter%!🚀]', }, }, - ] + ] as const satisfies ReadonlyArray<{ + name: string + path: string + browsePath: string + expectedPath: string + // Allow either splat or id cases. '*' included for assertions. + params: Record<string, string> + }>
6556-6570: Avoid TDZ-style cross-reference between component and route for clarity.
PathRouteComponentreadspathRoute.useParams()whilepathRouteis declared later. It works (function body runs after assignment), but the order can confuse readers. Consider relying on function hoisting or using the genericuseParamsto decouple.Option A — move the function below (uses hoisting, no behavior change):
- function PathRouteComponent() { - const params = pathRoute.useParams() - return ( - <div> - <h1>Path Route</h1> - <p> - params:{' '} - <span data-testid="params-to-validate"> - {JSON.stringify(params)} - </span> - </p> - </div> - ) - } - const pathRoute = createRoute({ getParentRoute: () => rootRoute, path, component: PathRouteComponent, }) + + function PathRouteComponent() { + const params = pathRoute.useParams() + return ( + <div> + <h1>Path Route</h1> + <p> + params:{' '} + <span data-testid="params-to-validate"> + {JSON.stringify(params)} + </span> + </p> + </div> + ) + }Option B — decouple with generic hook (if you don’t need route‑typed params here):
- function PathRouteComponent() { - const params = pathRoute.useParams() + function PathRouteComponent() { + const params = useParams({ strict: false }) ... }Also applies to: 6571-6575
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/solid-router/basic-file-based/tests/params.spec.ts(1 hunks)packages/react-router/tests/link.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/solid-router/basic-file-based/tests/params.spec.ts
🧰 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/react-router/tests/link.test.tsx
packages/{react-router,solid-router}/**
📄 CodeRabbit inference engine (AGENTS.md)
Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Files:
packages/react-router/tests/link.test.tsx
🧬 Code graph analysis (1)
packages/react-router/tests/link.test.tsx (1)
packages/react-router/src/link.tsx (1)
Link(567-598)
🔇 Additional comments (1)
packages/react-router/tests/link.test.tsx (1)
6483-6599: Great coverage; behavior matches PR intent.Solid end-to-end verification for mixed encoded/Unicode segments, splat vs. param slash handling, and href generation. Nothing blocking.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-router/tests/link.test.tsx(1 hunks)packages/solid-router/tests/link.test.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/solid-router/tests/link.test.tsxpackages/react-router/tests/link.test.tsx
packages/{react-router,solid-router}/**
📄 CodeRabbit inference engine (AGENTS.md)
Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Files:
packages/solid-router/tests/link.test.tsxpackages/react-router/tests/link.test.tsx
🧠 Learnings (1)
📚 Learning: 2025-09-23T17:36:12.598Z
Learnt from: CR
PR: TanStack/router#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:
packages/solid-router/tests/link.test.tsx
🧬 Code graph analysis (1)
packages/react-router/tests/link.test.tsx (1)
packages/react-router/src/link.tsx (1)
Link(567-598)
⏰ 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 (3)
packages/solid-router/tests/link.test.tsx (1)
6575-6584: The review comment is incorrect. This is the established test pattern throughout the file.The
validate()function (lines 6531-6537) usesawait waitFor()to asynchronously poll and wait for the router state to update after the URL change, which is the appropriate mechanism for handling this scenario. This exact pattern—render the router, then callwindow.history.replaceState, then validate—is used consistently across 13+ tests in this file (at lines 5286, 5306, 5326, 5347, 5366, 5387, 5406, 5427, 5447, 5462, 5478, 5497, and 6582), indicating it's the established convention for this codebase. The browser history integration appears to support this pattern, and the tests are functioning as designed.Likely an incorrect or invalid review comment.
packages/react-router/tests/link.test.tsx (2)
6483-6599: Duplication concern is unsubstantiated.The verification script found only a single occurrence of the "encoded and unicode paths" describe block at line 6483—not multiple. The review comment's primary concern about duplicate test suites does not reflect the actual state of the code.
Likely an incorrect or invalid review comment.
6484-6527: The{$}syntax is valid and correct for splat routes with prefix/suffix.After searching the codebase,
{$}is the correct and intentional syntax for splat routes that include prefix or suffix text. This is evident from multiple route definitions inroute.test-d.tsx:
path: 'prefix{$}suffix'path: 'docs/$docId/before{$}after/detail{-$detailId}/file-{$myFile}.pdf'The syntax pattern is: braces are required when combining literal text with a parameter or splat. Standalone sплat uses bare
$(as in your "with wildcard" test case), but splat with surrounding text requires{$}. Optional variants use{-$}.The test cases at lines 6487 and 6498 are syntactically correct.
Likely an incorrect or invalid review comment.
SeanCassiere
changed the title
This PR addresses the issue filed in #3962 and follows on from #5534 and #3987. As mentioned in #3987 we should ideally find a better solution to deal with this, however these will bring changes to users and ideally, we should leave these for the next major, this solution attempts to deal with this problem without breaking current use cases.
when backslashes are used in a path param the decoded value
\is removed from the pathname. we should not be decoding%5Cto\when decoding the various path segments. We already do this for%25(percentage signs) and a more complete solution is needed for these special characters that should not be decoded.This PR adds a new utility function decodePathSegment that will decode the path segment ignoring a defined array of encoded values. As a default we are not decoding
\,%and/. In addition to these, the defined list of special characters that can be allowed to not be encoded has also been added here.While working on this PR it also came to fore that when navigating to a path that contains special characters these are shown in the URL in encoded form, however when refreshing the page or browsing to the path directly these are shown in decoded form. In addition to the above we are also now standardizing this to show the decoded form on navigation similar as to when browsing directly to the path.
This PR also adds tests for the new utility function as well updates the solid-router and react-router e2e tests to test the functionality.
Summary by CodeRabbit
Bug Fixes
New Features
Tests