fix: navigate() respects rewrites when reloadDocument=true #5389
Conversation
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
WalkthroughAdds a new e2e React Router project with basepath support (Vite + Playwright), generated file-based routes and tests, global test server setup/teardown, and adjusts RouterCore.navigate to use the router-built location's Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as App (Router UI)
participant Router as RouterCore
participant Loc as BuildLocation
participant Win as window.location
User->>App: Click nav with reloadDocument:true
App->>Router: navigate({ to, reloadDocument: true })
Router->>Loc: buildLocation(to, opts)
Note right of Loc: buildLocation includes basepath
Loc-->>Router: { url, href, ... }
alt reloadDocument && no explicit href
Router->>Win: assign( url ) %% changed from href to url
else
Router->>Win: assign( provided href or existing href )
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit b097f19
☁️ 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
changed the title
schiller-manuel
changed the title
![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
♻️ Duplicate comments (2)
e2e/react-router/basepath-file-based/tests/setup/global.setup.ts (1)
2-2: Verify import assertion syntax compatibility.Same as the teardown file: the
with { type: 'json' }syntax requires Node.js 17.1+ or 16.14+ and TypeScript 4.5+. Ensure runtime compatibility.e2e/react-router/basepath-file-based/playwright.config.ts (1)
6-6: Verify import assertion syntax compatibility.The
with { type: 'json' }syntax requires Node.js 17.1+ or 16.14+ and TypeScript 4.5+. This is consistent with other files in this e2e project.
🧹 Nitpick comments (1)
e2e/react-router/basepath-file-based/src/main.tsx (1)
22-22: Consider defensive check for root element.The non-null assertion assumes the element exists. While this is safe in a controlled e2e environment, a defensive check with an error message would improve robustness.
Apply this diff for safer handling:
-const rootElement = document.getElementById('app')! +const rootElement = document.getElementById('app') +if (!rootElement) { + throw new Error('Root element not found') +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
e2e/react-router/basepath-file-based/.gitignore(1 hunks)e2e/react-router/basepath-file-based/index.html(1 hunks)e2e/react-router/basepath-file-based/package.json(1 hunks)e2e/react-router/basepath-file-based/playwright.config.ts(1 hunks)e2e/react-router/basepath-file-based/src/main.tsx(1 hunks)e2e/react-router/basepath-file-based/src/routeTree.gen.ts(1 hunks)e2e/react-router/basepath-file-based/tests/reload-document.test.ts(1 hunks)e2e/react-router/basepath-file-based/tests/setup/global.setup.ts(1 hunks)e2e/react-router/basepath-file-based/tests/setup/global.teardown.ts(1 hunks)e2e/react-router/basepath-file-based/tsconfig.json(1 hunks)e2e/react-router/basepath-file-based/vite.config.js(1 hunks)packages/router-core/src/router.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
e2e/react-router/basepath-file-based/playwright.config.tse2e/react-router/basepath-file-based/src/main.tsxe2e/react-router/basepath-file-based/tests/reload-document.test.tse2e/react-router/basepath-file-based/src/routeTree.gen.tspackages/router-core/src/router.tse2e/react-router/basepath-file-based/tests/setup/global.setup.tse2e/react-router/basepath-file-based/tests/setup/global.teardown.ts
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/react-router/basepath-file-based/playwright.config.tse2e/react-router/basepath-file-based/src/main.tsxe2e/react-router/basepath-file-based/tests/reload-document.test.tse2e/react-router/basepath-file-based/src/routeTree.gen.tse2e/react-router/basepath-file-based/tests/setup/global.setup.tse2e/react-router/basepath-file-based/tsconfig.jsone2e/react-router/basepath-file-based/index.htmle2e/react-router/basepath-file-based/package.jsone2e/react-router/basepath-file-based/tests/setup/global.teardown.tse2e/react-router/basepath-file-based/vite.config.js
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/router.ts
**/package.json
📄 CodeRabbit inference engine (AGENTS.md)
Use workspace:* protocol for internal dependencies in package.json files
Files:
e2e/react-router/basepath-file-based/package.json
🧠 Learnings (2)
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
PR: TanStack/router#5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.420Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.
Applied to files:
e2e/react-router/basepath-file-based/src/routeTree.gen.ts
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
PR: TanStack/router#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/router.ts
🧬 Code graph analysis (6)
e2e/react-router/basepath-file-based/playwright.config.ts (1)
scripts/set-ts-version.js (1)
packageJson(33-33)
e2e/react-router/basepath-file-based/src/main.tsx (2)
packages/router-core/src/router.ts (1)
Register(97-102)e2e/react-router/js-only-file-based/src/main.jsx (2)
rootElement(15-15)root(18-18)
e2e/react-router/basepath-file-based/src/routeTree.gen.ts (1)
e2e/react-router/js-only-file-based/src/routeTree.gen.js (2)
IndexRoute(30-34)rootRouteChildren(90-94)
packages/router-core/src/router.ts (1)
packages/history/src/index.ts (1)
location(162-164)
e2e/react-router/basepath-file-based/tests/setup/global.setup.ts (4)
e2e/react-router/basic/tests/setup/global.setup.ts (1)
setup(4-6)e2e/react-router/basic-virtual-file-based/tests/setup/global.setup.ts (1)
setup(4-6)e2e/react-router/js-only-file-based/tests/setup/global.setup.ts (1)
setup(4-6)e2e/solid-router/basic-file-based/tests/setup/global.setup.ts (1)
setup(4-6)
e2e/react-router/basepath-file-based/tests/setup/global.teardown.ts (2)
scripts/set-ts-version.js (1)
packageJson(33-33)e2e/solid-router/basic-file-based/tests/setup/global.teardown.ts (1)
teardown(4-6)
⏰ 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 (10)
packages/router-core/src/router.ts (1)
1919-1924: Reload navigations now honor basepath rewritesUsing
location.urlhere ensures we send the rewritten, basepath-aware absolute URL towindow.location, so reloadDocument navigations keep the configured basepath. This directly addresses the regression from #5388.e2e/react-router/basepath-file-based/index.html (1)
1-11: LGTM!Standard HTML5 boilerplate for a Vite + React application. The structure is clean and appropriate for the e2e test setup.
e2e/react-router/basepath-file-based/tsconfig.json (1)
1-15: LGTM!The TypeScript configuration is appropriate for a Vite-based e2e test project. Strict mode is enabled as per coding guidelines, and the settings align with modern React development practices.
e2e/react-router/basepath-file-based/src/main.tsx (1)
7-13: LGTM!The router configuration with
basepath: '/app'correctly aligns with the test objectives to verify basepath handling withreloadDocument: true.e2e/react-router/basepath-file-based/tests/reload-document.test.ts (1)
3-18: LGTM!The test effectively validates the PR objective: ensuring that
navigate()respects the basepath whenreloadDocument=true. The test flow is clear and uses appropriate Playwright patterns for navigation and assertions.e2e/react-router/basepath-file-based/tests/setup/global.teardown.ts (2)
4-6: LGTM!The teardown function correctly stops the dummy server using the package name, following the standard pattern used in other e2e projects.
2-2: Import assertion syntax is compatible. Node.js v24.3.0 and TypeScript v5.9.0 satisfy the JSON import assertion requirements.e2e/react-router/basepath-file-based/tests/setup/global.setup.ts (1)
4-6: LGTM!The setup function correctly starts the dummy server, mirroring the pattern used across other e2e test projects in the repository.
e2e/react-router/basepath-file-based/playwright.config.ts (2)
8-9: Top-level await usage is appropriate.Top-level await for retrieving dynamic ports is a clean approach and is well-supported in modern Node.js versions (14.8+) with ES modules.
14-41: LGTM!The Playwright configuration is comprehensive and follows best practices:
- Single worker prevents concurrency issues in e2e tests
- Global setup/teardown hooks for server lifecycle management
- Reuses existing server outside CI for faster local development
- Chromium-only testing is appropriate for this e2e scenario
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
e2e/react-router/basepath-file-based/src/routes/index.tsx (1)
1-25: LGTM! Navigation logic correctly implements the test scenario.The route definition and navigation with
reloadDocument: trueproperly exercises the basepath behavior that this PR fixes.Optional enhancement: Consider adding
type="button"to the button element for better HTML semantics:<button + type="button" data-testid="to-about-btn" onClick={() =>e2e/react-router/basepath-file-based/src/routes/about.tsx (1)
1-20: LGTM! Completes the bidirectional navigation test.The about route correctly mirrors the home route pattern, enabling complete testing of basepath behavior with
reloadDocumentin both directions.Optional enhancement: Consider adding
type="button"to the button element for better HTML semantics:<button + type="button" onClick={() => navigate({ to: '/', reloadDocument: true })} data-testid="to-home-btn"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
e2e/react-router/basepath-file-based/src/routes/__root.tsx(1 hunks)e2e/react-router/basepath-file-based/src/routes/about.tsx(1 hunks)e2e/react-router/basepath-file-based/src/routes/index.tsx(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:
e2e/react-router/basepath-file-based/src/routes/__root.tsxe2e/react-router/basepath-file-based/src/routes/index.tsxe2e/react-router/basepath-file-based/src/routes/about.tsx
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/react-router/basepath-file-based/src/routes/__root.tsxe2e/react-router/basepath-file-based/src/routes/index.tsxe2e/react-router/basepath-file-based/src/routes/about.tsx
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/react-router/basepath-file-based/src/routes/__root.tsxe2e/react-router/basepath-file-based/src/routes/index.tsxe2e/react-router/basepath-file-based/src/routes/about.tsx
🧬 Code graph analysis (3)
e2e/react-router/basepath-file-based/src/routes/__root.tsx (2)
e2e/react-router/basepath-file-based/src/routes/about.tsx (1)
Route(3-5)e2e/react-router/basepath-file-based/src/routes/index.tsx (1)
Route(3-5)
e2e/react-router/basepath-file-based/src/routes/index.tsx (2)
e2e/react-router/basepath-file-based/src/routes/__root.tsx (1)
Route(3-3)e2e/react-router/basepath-file-based/src/routes/about.tsx (1)
Route(3-5)
e2e/react-router/basepath-file-based/src/routes/about.tsx (2)
e2e/react-router/basepath-file-based/src/routes/__root.tsx (1)
Route(3-3)e2e/react-router/basepath-file-based/src/routes/index.tsx (1)
Route(3-5)
🔇 Additional comments (1)
e2e/react-router/basepath-file-based/src/routes/__root.tsx (1)
1-3: LGTM! Clean root route implementation.The minimal root route correctly establishes the foundation for the file-based routing structure used in the e2e tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 #5388
Summary by CodeRabbit