Skip to content

Conversation

@Sheraff
Copy link
Contributor

@Sheraff Sheraff commented Nov 19, 2025

trimPathRight is used on every matchRoutes call, any perf is good to take

bench

TL;DR: same when there is a trailing slash, 2x faster when there isn't.

 ✓  @tanstack/router-core  tests/trim.bench.ts > trimPathRight (trailing slash) 5363ms
     name                           hz     min     max    mean     p75     p99    p995    p999     rme  samples
   · Old Implementation  18,094,606.91  0.0000  4.5986  0.0001  0.0001  0.0001  0.0001  0.0002  ±1.80%  9047304
   · New Implementation  17,929,428.10  0.0000  3.7125  0.0001  0.0001  0.0001  0.0001  0.0001  ±1.46%  8964715

 ✓  @tanstack/router-core  tests/trim.bench.ts > trimPathRight (no trailing slash) 8778ms
     name                           hz     min     max    mean     p75     p99    p995    p999     rme   samples
   · Old Implementation  22,495,982.20  0.0000  0.0367  0.0000  0.0000  0.0001  0.0001  0.0001  ±0.07%  11247992
   · New Implementation  47,478,310.39  0.0000  1.9014  0.0000  0.0000  0.0000  0.0000  0.0000  ±0.86%  23739157

 ✓  @tanstack/router-core  tests/trim.bench.ts > trimPathRight (root path) 11253ms
     name                           hz     min     max    mean     p75     p99    p995    p999     rme   samples
   · Old Implementation  48,767,386.88  0.0000  0.1359  0.0000  0.0000  0.0000  0.0000  0.0000  ±0.09%  24383695
   · New Implementation  48,798,276.83  0.0000  0.8486  0.0000  0.0000  0.0000  0.0000  0.0000  ±0.34%  24399139

 BENCH  Summary

   @tanstack/router-core  Old Implementation - tests/trim.bench.ts > trimPathRight (trailing slash)
    1.01x faster than New Implementation

   @tanstack/router-core  New Implementation - tests/trim.bench.ts > trimPathRight (no trailing slash)
    2.11x faster than Old Implementation

   @tanstack/router-core  New Implementation - tests/trim.bench.ts > trimPathRight (root path)
    1.00x faster than Old Implementation
describe('trimPathRight (trailing slash)', () => {
  bench('Old Implementation', () => {
    trimPathRightOld('/some/test/path/')
  })
  bench('New Implementation', () => {
    trimPathRightNew('/some/test/path/')
  })
})

describe('trimPathRight (no trailing slash)', () => {
  bench('Old Implementation', () => {
    trimPathRightOld('/some/test/path')
  })
  bench('New Implementation', () => {
    trimPathRightNew('/some/test/path')
  })
})

describe('trimPathRight (root path)', () => {
  bench('Old Implementation', () => {
    trimPathRightOld('/')
  })
  bench('New Implementation', () => {
    trimPathRightNew('/')
  })
})

Summary by CodeRabbit

  • Bug Fixes
    • Fixed path handling for edge cases with very short strings, ensuring single-character inputs are now correctly preserved.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 19, 2025

Walkthrough

Updated trimPathRight function in router-core to add a guard condition preventing trailing-slash removal on very short strings. The function now only trims when path length exceeds 1, preserving single-character inputs while maintaining its existing behavior for longer paths. Public API signature unchanged.

Changes

Cohort / File(s) Summary
Router path utility enhancement
packages/router-core/src/path.ts
Updated trimPathRight function to guard against trimming very short strings; added length > 1 check before performing trailing-slash cleanup, preserving single-character path inputs

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

  • Single function update with straightforward guard logic
  • Edge-case boundary condition fix with minimal scope

Suggested labels

package: router-core, performance

Suggested reviewers

  • schiller-manuel
  • nlynzaad

Poem

A single char, once stripped of its pride,
Now stays untouched with our guard applied,
One length's the charm, no trim shall it face,
A path preserved in its rightful place! 🐰✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: a performance refactor of trimPathRight that removes unnecessary trailing slash processing, which is validated by the included benchmarks showing significant improvements.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor-router-core-trim-path-right-performance

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 37eb416 and 88f3ed7.

📒 Files selected for processing (1)
  • packages/router-core/src/path.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/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/path.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/src/path.ts
📚 Learning: 2025-09-22T00:56:53.426Z
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.

Applied to files:

  • packages/router-core/src/path.ts
🧬 Code graph analysis (1)
packages/router-core/src/path.ts (1)
packages/router-core/src/route.ts (1)
  • path (1553-1555)
⏰ 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/path.ts (1)

38-41: Well-executed performance optimization with solid correctness guarantees.

The refactored trimPathRight replaces an unconditional regex replacement with a guarded short-circuit check. By verifying that the path both exceeds length 1 and ends with / before invoking the regex engine, the function avoids the overhead of regex matching on paths without trailing slashes—which is the hot path, given that trimPathRight is called on every matchRoutes invocation.

The benchmark data validates this: ~2.11x speedup for non-trailing-slash paths while remaining roughly equivalent for the other cases. Correctness is preserved across all edge cases (root /, single-char strings, empty string, multiple trailing slashes).


Comment @coderabbitai help to get the list of available commands and usage tips.

@nx-cloud
Copy link

nx-cloud bot commented Nov 19, 2025

View your CI Pipeline Execution ↗ for commit 88f3ed7

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ✅ Succeeded 7m 6s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 1m 26s View ↗

☁️ Nx Cloud last updated this comment at 2025-11-19 20:24:06 UTC

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 19, 2025

More templates

@tanstack/arktype-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/arktype-adapter@5909

@tanstack/directive-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/directive-functions-plugin@5909

@tanstack/eslint-plugin-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/eslint-plugin-router@5909

@tanstack/history

npm i https://pkg.pr.new/TanStack/router/@tanstack/history@5909

@tanstack/nitro-v2-vite-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/nitro-v2-vite-plugin@5909

@tanstack/react-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router@5909

@tanstack/react-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-devtools@5909

@tanstack/react-router-ssr-query

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-ssr-query@5909

@tanstack/react-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start@5909

@tanstack/react-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-client@5909

@tanstack/react-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-server@5909

@tanstack/router-cli

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-cli@5909

@tanstack/router-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-core@5909

@tanstack/router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools@5909

@tanstack/router-devtools-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools-core@5909

@tanstack/router-generator

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-generator@5909

@tanstack/router-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-plugin@5909

@tanstack/router-ssr-query-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-ssr-query-core@5909

@tanstack/router-utils

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-utils@5909

@tanstack/router-vite-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-vite-plugin@5909

@tanstack/server-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/server-functions-plugin@5909

@tanstack/solid-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router@5909

@tanstack/solid-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router-devtools@5909

@tanstack/solid-router-ssr-query

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router-ssr-query@5909

@tanstack/solid-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start@5909

@tanstack/solid-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-client@5909

@tanstack/solid-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-server@5909

@tanstack/start-client-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-client-core@5909

@tanstack/start-plugin-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-plugin-core@5909

@tanstack/start-server-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-server-core@5909

@tanstack/start-static-server-functions

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-static-server-functions@5909

@tanstack/start-storage-context

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-storage-context@5909

@tanstack/valibot-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/valibot-adapter@5909

@tanstack/virtual-file-routes

npm i https://pkg.pr.new/TanStack/router/@tanstack/virtual-file-routes@5909

@tanstack/zod-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/zod-adapter@5909

commit: 88f3ed7

@Sheraff Sheraff merged commit 98d15a1 into main Nov 19, 2025
6 checks passed
@Sheraff Sheraff deleted the refactor-router-core-trim-path-right-performance branch November 19, 2025 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants