fix(router): rewriteBasepath not working without trailing slash
#5244
Conversation
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.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 (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
|
thanks for the PR! |
thenglong
force-pushed
the
fix/rewrite-basepath-trailing-slash-not-working-without-trailing-slash
branch
from
ea4b76b to
ee96896
Compare
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.
|
the tests dont fail with the current implementation, so something is off here (most likely the tests...) |
I'll do a recheck on those test. |
|
@thenglong I also noticed that |
|
@omridevk please create a new github issue for this including a complete minimal reproducer |
|
@schiller-manuel |
Fixes: #5221
Summary by CodeRabbit
Bug Fixes
Tests