Uh oh!
There was an error while loading. Please reload this page.
perf: use slice for proven ordered bounds - #7977
Conversation
📝 WalkthroughWalkthroughThis change replaces 24 safe ChangesString slicing optimization
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
View your CI Pipeline Execution ↗ for commit 95bacef
☁️ Nx Cloud last updated this comment at |
Bundle Size Benchmarks
The following scenarios have bundle-size changes compared with the baseline:
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
Merging this PR will regress 1 benchmark
|
| Mode | Benchmark | BASE | HEAD | Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem server error-paths unmatched (react) | 273.1 KB | 1,660.6 KB | -83.55% |
| ⚡ | Memory | mem server server-fn-churn (vue) | 3,927.3 KB | 263.2 KB | ×15 |
| ⚡ | Memory | mem server peak-large-page (react) | 2,064 KB | 953.1 KB | ×2.2 |
| ⚡ | Simulation | ssr request loop (solid) | 152.3 ms | 145.5 ms | +4.67% |
| ⚡ | Memory | mem server error-paths not-found (vue) | 333 KB | 319.7 KB | +4.19% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing codex/shorter-string-operations (95bacef) with main (4fa1df7)
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud has identified a possible root cause for your failed CI:
We classified this failure as an environment issue unrelated to the PR. The error originates in a stale e2e-utils dist artifact missing the toRuntimePath export — a build environment problem entirely outside the scope of the substring → slice optimization changes in @tanstack/router-core. No changes to this PR are needed to resolve it.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
RESULT-optimization-shorter-string-operations.md (1)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRetain reproducible measurement artifacts.
The report stores the full bundle matrix in
/tmp/native-slice-only-full.json. That path is transient and unavailable to later reviewers. Commit the measurement input or document a reproducible command, environment, and artifact location for the reported validation and benchmark results.Also applies to: 54-74
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@RESULT-optimization-shorter-string-operations.md` at line 17, Make the benchmark results reproducible by committing the measurement input artifact or documenting the exact command, environment, and durable artifact location used to generate the reported validation and benchmark results. Replace the transient /tmp/native-slice-only-full.json reference in the report and apply the same change to the related content covering lines 54–74.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/router-core/src/path.ts`:
- Around line 380-390: In the optional-parameter handling branch, update the
guard for missing or undefined valueRaw to use a braced if block while
preserving its continue behavior; leave the surrounding usedParams and
prefix/suffix logic unchanged.
---
Nitpick comments:
In `@RESULT-optimization-shorter-string-operations.md`:
- Line 17: Make the benchmark results reproducible by committing the measurement
input artifact or documenting the exact command, environment, and durable
artifact location used to generate the reported validation and benchmark
results. Replace the transient /tmp/native-slice-only-full.json reference in the
report and apply the same change to the related content covering lines 54–74.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d17e760b-2f63-4977-a555-131f1ad9187b
📒 Files selected for processing (7)
RESULT-optimization-shorter-string-operations.mdpackages/history/src/index.tspackages/router-core/src/new-process-route-tree.tspackages/router-core/src/path.tspackages/router-core/src/searchParams.tspackages/router-core/tests/path-string-operations.bench.tspackages/router-core/tests/path.test.ts
| if (kind === SEGMENT_TYPE_OPTIONAL_PARAM) { | ||
| const key = path.substring(segment[2], segment[3]) | ||
| const key = path.slice(segment[2], segment[3]) | ||
| const valueRaw = params[key] | ||
| // Check if optional parameter is missing or undefined | ||
| if (valueRaw == null) continue | ||
| usedParams[key] = valueRaw | ||
| const prefix = path.substring(start, segment[1]) | ||
| const suffix = path.substring(segment[4], end) | ||
| const prefix = path.slice(start, segment[1]) | ||
| const suffix = path.slice(segment[4], end) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add braces to the optional-parameter guard.
Line 385 uses a one-line if body. Add braces to meet the TypeScript control-statement rule.
Proposed fix
- if (valueRaw == null) continue+ if (valueRaw == null) {+ continue+ }As per coding guidelines, “Always use curly braces for if, else, loops, and similar control statements.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(kind===SEGMENT_TYPE_OPTIONAL_PARAM){ | |
| constkey=path.substring(segment[2],segment[3]) | |
| constkey=path.slice(segment[2],segment[3]) | |
| constvalueRaw=params[key] | |
| // Check if optional parameter is missing or undefined | |
| if(valueRaw==null)continue | |
| usedParams[key]=valueRaw | |
| constprefix=path.substring(start,segment[1]) | |
| constsuffix=path.substring(segment[4],end) | |
| constprefix=path.slice(start,segment[1]) | |
| constsuffix=path.slice(segment[4],end) | |
| if(kind===SEGMENT_TYPE_OPTIONAL_PARAM){ | |
| constkey=path.slice(segment[2],segment[3]) | |
| constvalueRaw=params[key] | |
| // Check if optional parameter is missing or undefined | |
| if(valueRaw==null){ | |
| continue | |
| } | |
| usedParams[key]=valueRaw | |
| constprefix=path.slice(start,segment[1]) | |
| constsuffix=path.slice(segment[4],end) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/router-core/src/path.ts` around lines 380 - 390, In the
optional-parameter handling branch, update the guard for missing or undefined
valueRaw to use a braced if block while preserving its continue behavior; leave
the surrounding usedParams and prefix/suffix logic unchanged.
Source: Coding guidelines
Summary
Replace 24 internal
substringcalls with shorterslicecalls only where both bounds are proven non-negative and ordered.The five extraction sites whose bounds can reverse remain
substring; focused tests cover overlapping named, optional, and wildcard affixes plus Unicode lowercase expansion. No public API or browser-support floor changes.Bundle impact
Measured from clean commit
ae8b0397cf0a6813ea380fae4903371fd5d83ae5against exactmainbase697ebb6ddbd433d052b6b4707938a5c595865d58:A nearby
indexOf→includesgroup was dropped after its isolated performance result was not confidently neutral.Performance and validation
sliceaverages 10.34% faster thansubstringacross four final runsFull attribution, matrix, performance results, and rejected variants are in
RESULT-optimization-shorter-string-operations.md.Summary by CodeRabbit
Performance
Bug Fixes
Documentation