fix(router-core): useBlocker navigation issues for 404 pages and external URLs #4917
Conversation
|
View your CI Pipeline Execution ↗ for commit 0a400e9
☁️ Nx Cloud last updated this comment at |
jiji-hoon96
changed the title
jiji-hoon96
changed the title
jiji-hoon96
force-pushed
the
feat/useBlocker-navigation
branch
from
4ddec20 to
ab676cb
Compare
WalkthroughThe PR implements support for navigation from 404 "Not Found" routes when blockers are active. The router's navigate function becomes async and checks blockers for external URLs. Both React and Solid router useBlocker handlers now return notFound location objects instead of throwing errors, and blockers automatically allow navigation away from 404 states. Changes
Sequence DiagramsequenceDiagram
actor User
participant Nav as Navigation Request
participant Router as Router.navigate
participant Blocker as Blocker Logic
participant Location as Location Handler
User->>Nav: Trigger navigation
Nav->>Router: navigate({ to, ignoreBlocker? })
Router->>Blocker: Check blockers (if not ignoreBlocker)
alt External URL & Blockers Active
Blocker->>Router: Get history & blocker functions
Blocker->>Blocker: Invoke blockerFn(current, next)
alt Blocker Approves
Blocker-->>Router: Return (blocked)
Router-->>Nav: Resolve Promise (short-circuit)
else Blocker Allows
Blocker-->>Router: Continue
end
end
alt Current is 404 & Next is not 404
Location->>Blocker: Auto-allow (return false to block=false)
Blocker-->>Router: Navigate allowed
end
Router->>Router: Proceed with navigation
Router-->>User: Navigation complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router-core/src/router.ts (1)
1920-1958:buildAndCommitLocationmust return thecommitPromiseto satisfy theNavigateFncontract.The
navigatemethod (line 1961) implementsNavigateFn, which requires returningPromise<void>. At line 2001, it returns the result ofbuildAndCommitLocation({...}), butbuildAndCommitLocationcreatescommitPromise(line 1950) without returning it, causing the function to returnundefinedinstead ofPromise<void>. This violates the type contract and breaks any code awaiting the navigation promise.Add a return statement:
return commitPromiseat the end ofbuildAndCommitLocation(after line 1957).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/react-router/eslint.config.ts(1 hunks)packages/react-router/src/useBlocker.tsx(1 hunks)packages/react-router/tests/useBlocker.test.tsx(2 hunks)packages/router-core/src/router.ts(2 hunks)packages/solid-router/src/useBlocker.tsx(2 hunks)packages/solid-router/tests/useBlocker.test.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
packages/solid-router/tests/useBlocker.test.tsxpackages/react-router/tests/useBlocker.test.tsx
📚 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/react-router/src/useBlocker.tsxpackages/solid-router/src/useBlocker.tsxpackages/router-core/src/router.ts
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
packages/react-router/eslint.config.ts
🧬 Code graph analysis (3)
packages/solid-router/tests/useBlocker.test.tsx (1)
packages/solid-router/src/useBlocker.tsx (1)
useBlocker(159-266)
packages/react-router/tests/useBlocker.test.tsx (1)
packages/react-router/src/useBlocker.tsx (1)
useBlocker(151-260)
packages/router-core/src/router.ts (1)
packages/router-core/src/RouterProvider.ts (1)
NavigateFn(25-33)
⏰ 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: autofix
- GitHub Check: Test
🔇 Additional comments (5)
packages/solid-router/src/useBlocker.tsx (2)
191-197: LGTM! Not-found location handling is well-structured.The change to return a not-found location object instead of throwing an error enables graceful handling of navigation from 404 pages. The object structure is consistent with found routes and includes all necessary fields.
203-209: LGTM! Guard logic correctly allows navigation away from 404 pages.The guard ensures users can navigate from 404 pages to valid routes without blocker interference, preventing users from being trapped on error pages. The logic correctly:
- Allows 404 → valid route navigation (returns false)
- Continues blocker checks for 404 → 404 navigation (falls through)
- Preserves blocking for all other navigation scenarios
packages/solid-router/tests/useBlocker.test.tsx (1)
497-645: LGTM! Comprehensive test coverage for 404 navigation and external URLs.The three new test cases thoroughly exercise:
- 404 → valid route: Confirms blocker doesn't trap users on error pages
- 404 → 404: Verifies blocker still applies, maintaining original pathname
- External URL + ignoreBlocker: Tests both with and without the flag
Test assertions properly verify both DOM state and pathname changes, ensuring the full navigation flow works correctly.
packages/react-router/src/useBlocker.tsx (1)
203-209: LGTM! Guard correctly allows navigation away from 404 pages.The logic mirrors the Solid Router implementation and correctly prevents users from being trapped on 404 pages when a blocker is active. Navigation from 404 to valid routes bypasses the blocker, while all other navigation scenarios (including 404 → 404) continue through normal blocker evaluation.
packages/react-router/tests/useBlocker.test.tsx (1)
445-595: LGTM! Comprehensive test coverage mirrors Solid Router tests.The three test cases thoroughly exercise the new 404 and external URL functionality:
- Lines 445-511: Confirms navigation from 404 to valid routes works despite active blocker, testing both home and /posts destinations
- Lines 513-566: Verifies 404 → 404 navigation is still blocked, maintaining original pathname
- Lines 568-595: Tests external URL handling with
ignoreBlocker: trueand without it, using createMemoryHistory to avoid actual external navigationAll tests properly use
await router.navigate()and verify both DOM state and pathname changes.
| '@eslint-react/dom/no-missing-button-type': 'off', | ||
| 'react-hooks/exhaustive-deps': 'error', | ||
| 'react-hooks/rules-of-hooks': 'error', | ||
| '@typescript-eslint/no-unnecessary-condition': 'off', |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the necessity of disabling this TypeScript rule.
Disabling @typescript-eslint/no-unnecessary-condition affects the entire React Router package and could mask legitimate type issues beyond the blocker changes. The new routeId === '__notFound__' guards might be triggering this rule, but consider:
- Whether the
AnyShouldBlockFnLocationtype could be refined to avoid the lint error (e.g., using a discriminated union withrouteId: '__notFound__' | string) - Using inline
// eslint-disable-next-linecomments at specific locations instead of a blanket disable
Generate a verification script to check if the type system can be improved:
🏁 Script executed:
#!/bin/bash
# Check all locations where the disabled rule would apply
rg -n "routeId.*===.*'__notFound__'" packages/react-router/src/
rg -n "routeId.*!==.*'__notFound__'" packages/react-router/src/Length of output: 175
Let me check if the file exists first and get the context:
🏁 Script executed:
#!/bin/bash
# Get context around the routeId comparisons
sed -n '195,215p' packages/react-router/src/useBlocker.tsxLength of output: 541
🏁 Script executed:
#!/bin/bash
# First verify the file and get a larger context window
if [ -f "packages/react-router/src/useBlocker.tsx" ]; then
sed -n '190,220p' packages/react-router/src/useBlocker.tsx
else
echo "File not found, searching for useBlocker..."
find . -name "useBlocker.tsx" -type f 2>/dev/null
fiLength of output: 790
🏁 Script executed:
#!/bin/bash
# Find the getLocation function definition with more context
rg -n -B5 -A15 "const getLocation\s*=" packages/react-router/src/useBlocker.tsxLength of output: 80
🏁 Script executed:
#!/bin/bash
# Search for getLocation function definition in useBlocker
rg -n "getLocation" packages/react-router/src/useBlocker.tsx | head -20Length of output: 163
🏁 Script executed:
# Also search for the function definition across the codebase
rg -n -A20 "function getLocation|const getLocation" packages/react-router/src/useBlocker.tsxLength of output: 935
🏁 Script executed:
#!/bin/bash
# Find the AnyShouldBlockFnLocation type definition
rg -n "type AnyShouldBlockFnLocation|interface AnyShouldBlockFnLocation" packages/react-router/src/Length of output: 117
🏁 Script executed:
# Also search for it in the entire package to be thorough
rg -n "AnyShouldBlockFnLocation" packages/react-router/src/useBlocker.tsx | head -5Length of output: 116
🏁 Script executed:
#!/bin/bash
# Find the ShouldBlockFnLocation type definition
rg -n "type ShouldBlockFnLocation|interface ShouldBlockFnLocation" packages/react-router/src/Length of output: 76
🏁 Script executed:
#!/bin/bash
# Get the full ShouldBlockFnLocation interface definition
sed -n '15,35p' packages/react-router/src/useBlocker.tsxLength of output: 542
🏁 Script executed:
#!/bin/bash
# Check if there are other disables of this rule in eslint config or elsewhere
rg -n "no-unnecessary-condition" packages/react-router/Length of output: 101
🏁 Script executed:
# Also check the eslint config file for context around the disable
cat packages/react-router/eslint.config.tsLength of output: 727
Replace package-wide ESLint disable with inline comment at the specific locations.
The disable is justified—routeId is typed as any (via AnyShouldBlockFnLocation = ShouldBlockFnLocation<any, any, any, any>), preventing TypeScript from narrowing the type on the '__notFound__' literal check. However, the condition is a legitimate runtime check and the rule should not be disabled for the entire package.
Instead of the package-wide disable at line 22, add inline comments at the two specific lines in packages/react-router/src/useBlocker.tsx:204-205:
if (
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
current.routeId === '__notFound__' &&
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
next.routeId !== '__notFound__'
) {Then remove the blanket disable from packages/react-router/eslint.config.ts:22.
🤖 Prompt for AI Agents
In packages/react-router/eslint.config.ts around line 22 and
packages/react-router/src/useBlocker.tsx lines ~204-205, remove the package-wide
'@typescript-eslint/no-unnecessary-condition' disable from eslint.config.ts and
instead add two inline eslint disable comments directly above the two routeId
comparisons in useBlocker.tsx: place "// eslint-disable-next-line
@typescript-eslint/no-unnecessary-condition" immediately before the
"current.routeId === '__notFound__'" check and again immediately before the
"next.routeId !== '__notFound__'" check so the rule remains enabled package-wide
but is locally suppressed for these legitimate runtime checks.
| * @link https://tanstack.com/router/latest/docs/framework/react/api/router/NavigateOptionsType | ||
| */ | ||
| navigate: NavigateFn = ({ to, reloadDocument, href, ...rest }) => { | ||
| navigate: NavigateFn = async ({ to, reloadDocument, href, ...rest }) => { |
There was a problem hiding this comment.
🧩 Analysis chain
Note: navigate method is now asynchronous.
This change enables awaiting blocker checks for external URLs. While this aligns with the NavigateFn type signature in packages/router-core/src/RouterProvider.ts (line 24-32) which returns Promise<void>, verify that all call sites properly handle the async nature.
🏁 Script executed:
#!/bin/bash
# Find all navigate calls to verify they're handling promises correctly
rg -A3 "router\.navigate\(" packages/ --type ts --type tsx -g '!*.test.*' -g '!node_modules'Length of output: 27
🏁 Script executed:
#!/bin/bash
# Fix the ripgrep command - remove unrecognized tsx type
# Search for navigate calls in TypeScript files
rg -A3 "\.navigate\s*\(" packages/ --type ts -g '!*.test.*' -g '!node_modules' -g '!dist'Length of output: 4209
🏁 Script executed:
#!/bin/bash
# Find the navigate method definition around line 1960
cat packages/router-core/src/router.ts | sed -n '1955,1970p'Length of output: 375
🏁 Script executed:
#!/bin/bash
# Find the NavigateFn type signature in RouterProvider.ts
rg -A8 "type NavigateFn" packages/ --type ts -g '!node_modules'Length of output: 741
Critical: Most navigate() call sites are not handling the async return.
The navigate method is now async and returns Promise<void>, but verification shows many call sites don't await or return the promise, causing navigation to start without waiting for completion. This breaks the expected synchronous completion behavior and will cause race conditions with blockers.
Locations requiring fixes:
packages/solid-router/src/link.tsx:router.navigate({...})needsawaitpackages/router-core/src/router.ts: Twothis.navigate(...)calls needawaitpackages/router-devtools-core/src/NavigateButton.tsx:router().navigate(...)needsawaitpackages/router-core/src/ssr/ssr-client.ts:router.navigate(...)needsawaitpackages/router-core/src/load-matches.ts: Two calls lackawait(one already correctly usesawait)packages/react-router/src/useNavigate.tsxandpackages/react-router/src/link.tsx: Should return the promisepackages/solid-router/src/useNavigate.tsx: Should return the promise
🤖 Prompt for AI Agents
packages/router-core/src/router.ts around line 1960: the navigate method is now
async and returns a Promise<void>, so update all call sites to either await the
call or return the promise to preserve synchronous completion semantics and
avoid race conditions; specifically, in packages/solid-router/src/link.tsx
change calls like router.navigate({...}) to await router.navigate({...}); in
packages/router-core/src/router.ts make the two internal calls to
this.navigate(...) be awaited; in
packages/router-devtools-core/src/NavigateButton.tsx await
router().navigate(...); in packages/router-core/src/ssr/ssr-client.ts await
router.navigate(...); in packages/router-core/src/load-matches.ts add await to
the two navigate calls that currently omit it (keeping the one that already
awaits); and in packages/react-router/src/useNavigate.tsx,
packages/react-router/src/link.tsx and packages/solid-router/src/useNavigate.tsx
make the wrappers return the Promise from router.navigate(...) so callers can
await the navigation.
|
Some unit tests are failing |
birkskyum
force-pushed
the
feat/useBlocker-navigation
branch
from
fc870e6 to
0a400e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/router-core/src/router.ts (2)
1977-1977: Previously flagged: Navigate method is now async.This change was already identified in a previous review. All call sites need to be updated to handle the async return value properly.
1991-2008: Previously flagged: Blocker check relies on internal history API.This implementation was already identified in a previous review as using an internal/undocumented
getBlockers()API, which poses a maintenance risk.
🧹 Nitpick comments (5)
packages/react-router/eslint.config.ts (1)
22-22: Prefer inline ESLint disables over package-wide suppression.Disabling
@typescript-eslint/no-unnecessary-conditionfor the entire package could mask legitimate type issues elsewhere. The rule is triggered byrouteId === '__notFound__'checks inuseBlocker.tsxbecauserouteIdis typed asany, but these are valid runtime guards.Instead, add inline
// eslint-disable-next-linecomments directly above the specificrouteIdcomparisons inpackages/react-router/src/useBlocker.tsx(around lines 204-205), then remove this package-wide disable.Apply this diff to remove the package-wide disable:
'react-hooks/exhaustive-deps': 'error', 'react-hooks/rules-of-hooks': 'error', - '@typescript-eslint/no-unnecessary-condition': 'off', },Then add inline suppressions at the specific check locations in
useBlocker.tsx:if ( // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition current.routeId === '__notFound__' && // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition next.routeId !== '__notFound__' ) {Based on past review comments.
packages/solid-router/tests/useBlocker.test.tsx (2)
564-616: Consider consistent type assertions for non-existent routes.Line 574 uses
as anyfor the non-existent route, but line 600 doesn't use type assertion for the same pattern. Consider applying consistent typing for similar cases.Apply this diff for consistency:
- await router.navigate({ to: '/non-existent' }) + await router.navigate({ to: '/non-existent' as any })Otherwise, the test logic is correct and properly validates that navigation between 404 pages can be blocked.
618-645: Test doesn't verify blocker interaction with external URLs.The test is named to suggest it validates
ignoreBlockerwith blockers, but no blockers are actually registered on the router. Currently, it only verifies that external URL navigation doesn't crash. To properly test the fix for issue #4844 (ignoreBlocker not respected), consider:
- Registering an active blocker that would block navigation
- Verifying that
ignoreBlocker: truebypasses the blocker for external URLs- Verifying that without
ignoreBlocker, the blocker is consultedExample enhancement:
test('navigate function should handle external URLs with ignoreBlocker', async () => { const rootRoute = createRootRoute() + + const mockBlocker = vi.fn().mockReturnValue(true) + const indexRoute = createRoute({ getParentRoute: () => rootRoute, path: '/', - component: () => <div>Home</div>, + component: () => { + useBlocker({ shouldBlockFn: mockBlocker }) + return <div>Home</div> + }, }) const router = createRouter({ routeTree: rootRoute.addChildren([indexRoute]), history: createMemoryHistory({ initialEntries: ['/'], }), }) + + render(() => <RouterProvider router={router} />) + + // Wait for router to be ready + await screen.findByText('Home') await expect( router.navigate({ to: 'https://example.com', ignoreBlocker: true, }), ).resolves.toBeUndefined() + + // With ignoreBlocker, the blocker should not be called for external URLs + // (actual verification depends on implementation details) await expect( router.navigate({ to: 'https://example.com', }), ).resolves.toBeUndefined() })packages/react-router/tests/useBlocker.test.tsx (2)
513-566: LGTM: Test correctly validates 404-to-404 navigation blocking.The test appropriately verifies that navigation between 404 pages is still blocked when a blocker is active, ensuring the special case only applies to 404-to-valid-route transitions.
Minor inconsistency: Line 523 uses
as anytype assertion while line 550 doesn't. Consider using consistent typing for non-existent routes.
445-595: Consider adding test for valid-to-404 navigation with blocker.While the new tests cover 404-to-valid and 404-to-404 scenarios, there's no test verifying that navigation from a valid route to a 404 page is still blocked when a blocker is active. This would help ensure the fix doesn't break existing functionality.
Would you like me to generate a test case for this scenario?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/react-router/eslint.config.ts(1 hunks)packages/react-router/src/useBlocker.tsx(1 hunks)packages/react-router/tests/useBlocker.test.tsx(2 hunks)packages/router-core/src/router.ts(2 hunks)packages/solid-router/src/useBlocker.tsx(2 hunks)packages/solid-router/tests/useBlocker.test.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-router/src/useBlocker.tsx
- packages/solid-router/src/useBlocker.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
packages/react-router/eslint.config.ts
📚 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/react-router/eslint.config.tspackages/router-core/src/router.ts
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
packages/react-router/tests/useBlocker.test.tsxpackages/router-core/src/router.tspackages/solid-router/tests/useBlocker.test.tsx
🧬 Code graph analysis (3)
packages/react-router/tests/useBlocker.test.tsx (1)
packages/react-router/src/useBlocker.tsx (1)
useBlocker(151-260)
packages/router-core/src/router.ts (1)
packages/router-core/src/RouterProvider.ts (1)
NavigateFn(25-33)
packages/solid-router/tests/useBlocker.test.tsx (1)
packages/solid-router/src/useBlocker.tsx (1)
useBlocker(159-266)
⏰ 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 (4)
packages/solid-router/tests/useBlocker.test.tsx (2)
10-10: LGTM!The import of
createMemoryHistoryis correctly added and used in the external URL test case.
497-562: LGTM! Good coverage of 404-to-valid-route navigation.This test correctly validates that blockers allow navigation from 404 pages to valid routes, addressing the fix for issue #4881. The
as anytype assertion at line 546 is appropriate for testing navigation to non-existent routes.packages/react-router/tests/useBlocker.test.tsx (2)
10-10: LGTM: Import addition is appropriate.The
createMemoryHistoryimport is used in the external URL test to avoid side effects with browser navigation.
445-511: LGTM: Test correctly validates 404-to-valid-route navigation.The test appropriately verifies that navigation from a 404 page to a valid route succeeds even when a blocker is active, which aligns with the implementation in
useBlocker.tsx(lines 202-205) that bypasses blockers whencurrent.routeId === '__notFound__'andnext.routeId !== '__notFound__'.
| test('navigate function should handle external URLs with ignoreBlocker', async () => { | ||
| const rootRoute = createRootRoute() | ||
| const indexRoute = createRoute({ | ||
| getParentRoute: () => rootRoute, | ||
| path: '/', | ||
| component: () => <div>Home</div>, | ||
| }) | ||
|
|
||
| const router = createRouter({ | ||
| routeTree: rootRoute.addChildren([indexRoute]), | ||
| history: createMemoryHistory({ | ||
| initialEntries: ['/'], | ||
| }), | ||
| }) | ||
|
|
||
| await expect( | ||
| router.navigate({ | ||
| to: 'https://example.com', | ||
| ignoreBlocker: true, | ||
| }), | ||
| ).resolves.toBeUndefined() | ||
|
|
||
| await expect( | ||
| router.navigate({ | ||
| to: 'https://example.com', | ||
| }), | ||
| ).resolves.toBeUndefined() | ||
| }) |
There was a problem hiding this comment.
Incomplete test coverage for external URL blocker behavior.
The test only verifies that router.navigate resolves without throwing, but doesn't actually verify that:
- A blocker would normally prevent external navigation
- The
ignoreBlockeroption bypasses the blocker
To properly test the fix for issue #4844, the test should register a blocker and verify it's respected or bypassed based on the ignoreBlocker flag.
Consider this approach:
test('navigate function should handle external URLs with ignoreBlocker', async () => {
+ const blockerFn = vi.fn().mockResolvedValue(true)
+
const rootRoute = createRootRoute()
const indexRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/',
- component: () => <div>Home</div>,
+ component: () => {
+ useBlocker({ shouldBlockFn: blockerFn })
+ return <div>Home</div>
+ },
})
const router = createRouter({
routeTree: rootRoute.addChildren([indexRoute]),
history: createMemoryHistory({
initialEntries: ['/'],
}),
})
+ render(<RouterProvider router={router} />)
+
+ // With ignoreBlocker, blocker should not be called for external URLs
+ blockerFn.mockClear()
await expect(
router.navigate({
to: 'https://example.com',
ignoreBlocker: true,
}),
).resolves.toBeUndefined()
+ expect(blockerFn).not.toHaveBeenCalled()
+ // Without ignoreBlocker, blocker should be called for external URLs
+ blockerFn.mockClear()
await expect(
router.navigate({
to: 'https://example.com',
}),
).resolves.toBeUndefined()
+ expect(blockerFn).toHaveBeenCalled()
})📝 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.
| test('navigate function should handle external URLs with ignoreBlocker', async () => { | |
| const rootRoute = createRootRoute() | |
| const indexRoute = createRoute({ | |
| getParentRoute: () => rootRoute, | |
| path: '/', | |
| component: () => <div>Home</div>, | |
| }) | |
| const router = createRouter({ | |
| routeTree: rootRoute.addChildren([indexRoute]), | |
| history: createMemoryHistory({ | |
| initialEntries: ['/'], | |
| }), | |
| }) | |
| await expect( | |
| router.navigate({ | |
| to: 'https://example.com', | |
| ignoreBlocker: true, | |
| }), | |
| ).resolves.toBeUndefined() | |
| await expect( | |
| router.navigate({ | |
| to: 'https://example.com', | |
| }), | |
| ).resolves.toBeUndefined() | |
| }) | |
| test('navigate function should handle external URLs with ignoreBlocker', async () => { | |
| const blockerFn = vi.fn().mockResolvedValue(true) | |
| const rootRoute = createRootRoute() | |
| const indexRoute = createRoute({ | |
| getParentRoute: () => rootRoute, | |
| path: '/', | |
| component: () => { | |
| useBlocker({ shouldBlockFn: blockerFn }) | |
| return <div>Home</div> | |
| }, | |
| }) | |
| const router = createRouter({ | |
| routeTree: rootRoute.addChildren([indexRoute]), | |
| history: createMemoryHistory({ | |
| initialEntries: ['/'], | |
| }), | |
| }) | |
| render(<RouterProvider router={router} />) | |
| // With ignoreBlocker, blocker should not be called for external URLs | |
| blockerFn.mockClear() | |
| await expect( | |
| router.navigate({ | |
| to: 'https://example.com', | |
| ignoreBlocker: true, | |
| }), | |
| ).resolves.toBeUndefined() | |
| expect(blockerFn).not.toHaveBeenCalled() | |
| // Without ignoreBlocker, blocker should be called for external URLs | |
| blockerFn.mockClear() | |
| await expect( | |
| router.navigate({ | |
| to: 'https://example.com', | |
| }), | |
| ).resolves.toBeUndefined() | |
| expect(blockerFn).toHaveBeenCalled() | |
| }) |
🤖 Prompt for AI Agents
In packages/react-router/tests/useBlocker.test.tsx around lines 568-595, the
test only asserts that router.navigate resolves but doesn't verify blocker
behavior; register a blocker (e.g., with router.block or the library's blocker
API) that prevents navigation and spy on it (jest.fn), then assert the blocker
is called and that navigate to the external URL without ignoreBlocker is blocked
(navigation side-effect not performed or history location unchanged and the
promise resolves/rejects according to existing behavior), and finally call
router.navigate with ignoreBlocker: true and assert the blocker is bypassed (spy
called but navigation proceeds / external navigation logic is invoked and
history unchanged as expected); update assertions accordingly to confirm both
blocked and bypassed cases.
| // Check blockers for external URLs unless ignoreBlocker is true | ||
| if (!rest.ignoreBlocker) { | ||
| // Cast to access internal getBlockers method | ||
| const historyWithBlockers = this.history as any | ||
| const blockers = historyWithBlockers.getBlockers?.() ?? [] | ||
| for (const blocker of blockers) { | ||
| if (blocker?.blockerFn) { | ||
| const shouldBlock = await blocker.blockerFn({ | ||
| currentLocation: this.latestLocation, | ||
| nextLocation: this.latestLocation, // External URLs don't have a next location in our router | ||
| action: 'PUSH', | ||
| }) | ||
| if (shouldBlock) { | ||
| return Promise.resolve() | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix the hardcoded action to respect the replace option.
The blocker check always passes action: 'PUSH' (line 2001), but when rest.replace is true, the action should be 'REPLACE' to accurately reflect the type of navigation being blocked. This inconsistency could cause blockers to make incorrect decisions.
Apply this diff to fix the issue:
- const shouldBlock = await blocker.blockerFn({
- currentLocation: this.latestLocation,
- nextLocation: this.latestLocation, // External URLs don't have a next location in our router
- action: 'PUSH',
- })
+ const shouldBlock = await blocker.blockerFn({
+ currentLocation: this.latestLocation,
+ nextLocation: this.latestLocation, // External URLs don't have a next location in our router
+ action: rest.replace ? 'REPLACE' : 'PUSH',
+ })📝 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.
| // Check blockers for external URLs unless ignoreBlocker is true | |
| if (!rest.ignoreBlocker) { | |
| // Cast to access internal getBlockers method | |
| const historyWithBlockers = this.history as any | |
| const blockers = historyWithBlockers.getBlockers?.() ?? [] | |
| for (const blocker of blockers) { | |
| if (blocker?.blockerFn) { | |
| const shouldBlock = await blocker.blockerFn({ | |
| currentLocation: this.latestLocation, | |
| nextLocation: this.latestLocation, // External URLs don't have a next location in our router | |
| action: 'PUSH', | |
| }) | |
| if (shouldBlock) { | |
| return Promise.resolve() | |
| } | |
| } | |
| } | |
| } | |
| // Check blockers for external URLs unless ignoreBlocker is true | |
| if (!rest.ignoreBlocker) { | |
| // Cast to access internal getBlockers method | |
| const historyWithBlockers = this.history as any | |
| const blockers = historyWithBlockers.getBlockers?.() ?? [] | |
| for (const blocker of blockers) { | |
| if (blocker?.blockerFn) { | |
| const shouldBlock = await blocker.blockerFn({ | |
| currentLocation: this.latestLocation, | |
| nextLocation: this.latestLocation, // External URLs don't have a next location in our router | |
| action: rest.replace ? 'REPLACE' : 'PUSH', | |
| }) | |
| if (shouldBlock) { | |
| return Promise.resolve() | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/router-core/src/router.ts around lines 1991 to 2008, the blocker
check currently passes a hardcoded action 'PUSH'; change this to derive the
action from the navigation options (use 'REPLACE' when rest.replace is true,
otherwise 'PUSH') and pass that computed action into blocker.blockerFn so
blockers receive the correct navigation type; keep the rest of the blocker logic
unchanged.
2 participants
Summary
Fixes two critical navigation issues with the useBlocker hook:
404 Page Navigation: Previously, when a user was on a 404 page with an active blocker, attempting to navigate to any other page would throw an error:
"Error: No route found for location /foo"External URL ignoreBlocker: The
ignoreBlockeroption was not respected for external URLs, causing blockers to still prevent navigation even when explicitly ignoredChanges
404 Page Navigation Fix
foundRouteisundefinedby returningrouteId: '__notFound__'External URL ignoreBlocker Fix
ignoreBlockeroption for external URLsignoreBlockeris falseTest Coverage
Technical Details
404 Page Navigation
Root Cause: The blocker was trying to resolve 404 paths as valid routes, causing errors when getMatchedRoutes returned
undefinedforfoundRoute.Solution: The getLocation function now:
routeId: '__notFound__'for unmatched routesExternal URL ignoreBlocker
Root Cause: External URL navigation bypassed the normal blocker flow and didn't check the
ignoreBlockeroption.Solution: The navigate function now:
ignoreBlocker: truetrue, proceeds if all allowBreaking Changes
None. All existing functionality is preserved and the API remains unchanged.
Fixes
Closes #4881
Closes #4844
beforeunloaddialog triggered by redirect to external site even after original blocker proceed occurs #4094 (I think it's a similar issue.)Summary by CodeRabbit
Bug Fixes
New Features
ignoreBlockerparameter.✏️ Tip: You can customize this high-level summary in your review settings.