refactor(router-core): utils/decodePath minor performance improvements #5865
Conversation
WalkthroughRefactored the path decoding logic in the router utilities by simplifying DECODE_IGNORE_LIST from a Map to an array, extracting a new internal helper function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
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 1590a0a
☁️ 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: 0
🧹 Nitpick comments (2)
packages/router-core/src/utils.ts (2)
550-550: Remove unnecessary global flag in regex.The
g(global) flag is unnecessary when using.test(), which only checks for existence of a match. The global flag is for methods that match multiple times like.exec()or when used with.replace().Apply this diff:
- if (part === '' || !/%[0-9A-Fa-f]{2}/g.test(part)) return part + if (part === '' || !/%[0-9A-Fa-f]{2}/.test(part)) return part
553-555: Consider checking if normalization is needed first.The
replaceAllcall creates a new string even when no replacements are made. For inputs that are already uppercase, this is unnecessary overhead.Consider this optimization:
- const normalizedPart = part.replaceAll(/%[0-9a-f]{2}/g, (match) => - match.toUpperCase(), - ) + const needsNormalization = /%[0-9a-f]{2}/.test(part) + const normalizedPart = needsNormalization + ? part.replaceAll(/%[0-9a-f]{2}/g, (match) => match.toUpperCase()) + : partHowever, this adds an extra regex test, so benchmark to confirm it's actually faster for typical inputs.
📜 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
🧠 Learnings (2)
📓 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.
📚 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)
⏰ 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 (3)
492-495: Simplification looks good.The change from a Map to a simple array is a sensible optimization for a small, fixed list of two items. Array iteration is straightforward and efficient for this use case.
497-543: Well-structured recursive algorithm.The
splitAndDecodehelper implements a clean recursive approach that:
- Efficiently traverses the ignore list using index-based iteration (avoiding redundant checks)
- Preserves ignore characters while decoding everything else
- Handles edge cases gracefully (empty strings, consecutive ignore chars, boundary positions)
- Includes robust fallback for malformed encodings
The
.toUpperCase()call on line 507 ensures compatibility with custom ignore lists that may not be pre-normalized, which is good defensive programming.
535-541: Update build target to match runtime requirements or add transpilation support forreplaceAll.The
replaceAll()method was added in ES2021, but the project's roottsconfig.jsonspecifies"target": "ES2020". This creates a type/runtime mismatch: while TypeScript compiles successfully due to"lib": ["ES2022"]in the compiler options, the actual runtime code will fail in ES2020 environments wherereplaceAllis not available.
replaceAllis used in multiple files across the codebase:
packages/router-core/src/utils.ts(lines 535, 553)packages/router-generator/src/(multiple files)packages/router-core/src/path.ts(line 500)Either update the
targetintsconfig.jsonto"ES2021"or higher to accurately reflect runtime requirements, or implement transpilation/polyfill support to backportreplaceAllfor ES2020 environments if that compatibility is required.
TanStack#5865) * refactor(router-core): utils/decodePath minor performance improvements * ci: apply automated fixes --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Minor performance improvements of
decodePathinrouter-core/utils:splitAndDecodeis pure and should be declared outside ofdecodePathso it has a better chance of getting JIT optimizedDECODE_IGNORE_LISTis a static array, and should be initialized as an array (instead ofArray.from(new Map([...]).values()))partsToJoinis known in advance, so should be initialized w/ a length from the start to avoid memory layout changes when we.push()on every iterationSummary by CodeRabbit