refactor(router-core): simplify decodePath implementation, improve performance #5867
Conversation
WalkthroughRefactored path decoding in router-core from a recursive, ignore-list-driven flow to an iterative segment-parsing approach. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant decodePath
participant decodeSegment
note right of decodePath `#DDFFDD`: New iterative flow
Caller->>decodePath: decodePath(path, decodeIgnore?)
decodePath->>decodeSegment: extract next segment (cursor..match)
decodeSegment-->>decodePath: decodedSegment (or preserved)
decodePath->>decodeSegment: continue until end
decodePath-->>Caller: reconstructed decoded path
sequenceDiagram
autonumber
participant Caller
participant decodePath_old as decodePath (old)
participant splitAndDecode
note right of decodePath_old `#FFEECC`: Old recursive flow
Caller->>decodePath_old: decodePath(part, decodeIgnore)
decodePath_old->>splitAndDecode: splitAndDecode(parts...)
splitAndDecode-->>decodePath_old: decoded parts (recursive)
decodePath_old-->>Caller: decoded path
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 07dbfea
☁️ Nx Cloud last updated this comment at |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-core/src/utils.ts(2 hunks)packages/router-core/tests/utils.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: nlynzaad
Repo: TanStack/router PR: 5182
File: e2e/react-router/basic-file-based/tests/non-nested-paths.spec.ts:167-172
Timestamp: 2025-09-22T00:56:53.426Z
Learning: In TanStack Router, underscores are intentionally stripped from route segments during path parsing, but preserved in base path segments. This is the expected behavior implemented in PR #5182.
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: 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.
📚 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/src/utils.tspackages/router-core/tests/utils.test.ts
📚 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:
packages/router-core/tests/utils.test.ts
📚 Learning: 2025-09-22T00:56:49.237Z
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.
Applied to files:
packages/router-core/tests/utils.test.ts
🪛 ast-grep (0.39.9)
packages/router-core/src/utils.ts
[warning] 508-508: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${decodeIgnore.join('|')}, 'gi')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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/router-core/src/utils.ts (2)
492-505: LGTM! Robust fallback strategy.The two-tier error handling (try full decode, then fall back to individual sequences) elegantly handles malformed percent-encodings while preserving valid ones.
507-521: Iterative approach improves clarity and performance.The refactored implementation is cleaner and more efficient than a recursive approach. The cursor-based iteration correctly preserves ignored sequences while decoding the rest.
packages/router-core/tests/utils.test.ts (1)
608-614: Test correctly reflects case-preserving behavior.The updated expectation confirms that
decodePathnow preserves the original casing of ignored percent-encodings (e.g., both%2fand%2Fare preserved as-is). This is consistent with the case-insensitive regex matching in the implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/router-core/src/utils.ts (1)
507-519: Fix emptydecodeIgnorehandling and escape dynamic regex inputTwo issues in
decodePath’s new implementation:
- Empty
decodeIgnorebreaks decoding logicWhen
decodeIgnoreis an empty array,new RegExp(decodeIgnore.join('|'), 'gi')becomesnew RegExp('', 'gi'). With a zero-length global pattern,re.exec(path)repeatedly matches empty strings, so everypath.slice(cursor, match.index)is empty and the finalcursorends up past the end of the string. The function then returns an empty string for any non-emptypath, which is almost certainly unintended.Guard against this by treating an empty array like “no custom ignore,” e.g.:
-export function decodePath(path: string, decodeIgnore?: Array<string>): string { +export function decodePath(path: string, decodeIgnore?: Array<string>): string { if (!path) return path - const re = decodeIgnore - ? new RegExp(`${decodeIgnore.join('|')}`, 'gi') - : /%25|%5C/gi + const re = + decodeIgnore && decodeIgnore.length + ? new RegExp(decodeIgnore.join('|'), 'gi') + : /%25|%5C/gi
- Dynamic regex from
decodeIgnoreshould be escapedAs already noted in an earlier review, constructing the
RegExpdirectly fromdecodeIgnore.join('|')means any regex metacharacters in those strings change the pattern’s meaning and could open the door to surprising matches or ReDoS if values ever become user-controlled.A defensive fix is to escape each entry before joining:
- ? new RegExp(decodeIgnore.join('|'), 'gi') + ? new RegExp( + decodeIgnore + .map((s) => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')) + .join('|'), + 'gi', + )(or use
RegExp.escapewhen your TS/JS targets support it).This keeps the performance characteristics of the new implementation while hardening behavior for future callers.
🧹 Nitpick comments (1)
packages/router-core/src/utils.ts (1)
492-505: decodeSegment fallback looks robust; minor compatibility nit aroundreplaceAllThe two-step decoding strategy (
decodeURIfirst, then per-%XXbyte fallback) is a sensible way to preserve malformed sequences while still recovering what can be decoded. Only minor consideration:String.prototype.replaceAllis ES2021+, so if you still support older runtimes without transpilation/polyfills, you may want to switch to.replacewith a global regex for broader compatibility.For example:
- return segment.replaceAll(/%[0-9A-F]{2}/gi, (match) => { + return segment.replace(/%[0-9A-F]{2}/gi, (match) => { try { return decodeURI(match) } catch { return match } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/utils.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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: 5182
File: e2e/react-router/basic-file-based/tests/non-nested-paths.spec.ts:167-172
Timestamp: 2025-09-22T00:56:53.426Z
Learning: In TanStack Router, underscores are intentionally stripped from route segments during path parsing, but preserved in base path segments. This is the expected behavior implemented in PR #5182.
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.
📚 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/src/utils.ts
🧬 Code graph analysis (1)
packages/router-core/src/utils.ts (1)
packages/router-core/src/index.ts (1)
decodePath(281-281)
🪛 ast-grep (0.39.9)
packages/router-core/src/utils.ts
[warning] 509-509: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${decodeIgnore.join('|')}, 'gi')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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
…rformance (TanStack#5867) * refactor(router-core): simplify decodePath implementation, improve performance * simplify more
Using a stateful regex w/
.exec()we can decode a string chunk by chunk while avoiding strings in the ignore list. This is less code and more performant.bench: ~1.5x faster
Summary by CodeRabbit
Refactor
Tests