Skip to content

fix(router-core): useBlocker navigation issues for 404 pages and external URLs #4917

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
birkskyum merged 4 commits into from
Nov 23, 2025
Merged

fix(router-core): useBlocker navigation issues for 404 pages and external URLs #4917

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
be18d4c
feat: allow navigation from 404 pages when blocker is active
jiji-hoon96 Aug 11, 2025
aa9c4c7
feat: add blocker support for external URL navigation with ignoreBloc…
jiji-hoon96 Aug 11, 2025
8b61d95
fix: update useBlocker behavior and eslint config for 404 page handling
jiji-hoon96 Aug 11, 2025
0a400e9
fix(router): expand useBlocker 404 navigation fix to solid-route
jiji-hoon96 Nov 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/react-router/eslint.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export default [
'@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',
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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:

  1. Whether the AnyShouldBlockFnLocation type could be refined to avoid the lint error (e.g., using a discriminated union with routeId: '__notFound__' | string)
  2. Using inline // eslint-disable-next-line comments 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.tsx

Length 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
fi

Length 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.tsx

Length of output: 80

🏁 Script executed:

#!/bin/bash
# Search for getLocation function definition in useBlocker
rg -n "getLocation" packages/react-router/src/useBlocker.tsx | head -20

Length 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.tsx

Length 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 -5

Length 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.tsx

Length 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.ts

Length 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.

},
},
]
18 changes: 16 additions & 2 deletions packages/react-router/src/useBlocker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,20 +179,34 @@ export function useBlocker(
const parsedLocation = router.parseLocation(location)
const matchedRoutes = router.getMatchedRoutes(parsedLocation.pathname)
if (matchedRoutes.foundRoute === undefined) {
throw new Error(`No route found for location ${location.href}`)
return {
routeId: '__notFound__',
fullPath: parsedLocation.pathname,
pathname: parsedLocation.pathname,
params: matchedRoutes.routeParams,
search: router.options.parseSearch(location.search),
}
}

return {
routeId: matchedRoutes.foundRoute.id,
fullPath: matchedRoutes.foundRoute.fullPath,
pathname: parsedLocation.pathname,
params: matchedRoutes.routeParams,
search: parsedLocation.search,
search: router.options.parseSearch(location.search),
}
}

const current = getLocation(blockerFnArgs.currentLocation)
const next = getLocation(blockerFnArgs.nextLocation)

if (
current.routeId === '__notFound__' &&
next.routeId !== '__notFound__'
) {
return false
}

const shouldBlock = await shouldBlockFn({
action: blockerFnArgs.action,
current,
Expand Down
153 changes: 153 additions & 0 deletions packages/react-router/tests/useBlocker.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { z } from 'zod'
import {
RouterProvider,
createBrowserHistory,
createMemoryHistory,
createRootRoute,
createRoute,
createRouter,
Expand Down Expand Up @@ -440,4 +441,156 @@ describe('useBlocker', () => {

expect(window.location.pathname).toBe('/invoices')
})

test('should allow navigation from 404 page when blocker is active', async () => {
const rootRoute = createRootRoute({
notFoundComponent: function NotFoundComponent() {
const navigate = useNavigate()

useBlocker({ shouldBlockFn: () => true })

return (
<>
<h1>Not Found</h1>
<button onClick={() => navigate({ to: '/' })}>Go Home</button>
<button onClick={() => navigate({ to: '/posts' })}>
Go to Posts
</button>
</>
)
},
})

const indexRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/',
component: () => {
return (
<>
<h1>Index</h1>
</>
)
},
})

const postsRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/posts',
component: () => {
return (
<>
<h1>Posts</h1>
</>
)
},
})

const router = createRouter({
routeTree: rootRoute.addChildren([indexRoute, postsRoute]),
history,
})

render(<RouterProvider router={router} />)

await router.navigate({ to: '/non-existent' as any })

expect(
await screen.findByRole('heading', { name: 'Not Found' }),
).toBeInTheDocument()

expect(window.location.pathname).toBe('/non-existent')

const homeButton = await screen.findByRole('button', { name: 'Go Home' })
fireEvent.click(homeButton)

expect(
await screen.findByRole('heading', { name: 'Index' }),
).toBeInTheDocument()

expect(window.location.pathname).toBe('/')
})

test('should handle blocker navigation from 404 to another 404', async () => {
const rootRoute = createRootRoute({
notFoundComponent: function NotFoundComponent() {
const navigate = useNavigate()

useBlocker({ shouldBlockFn: () => true })

return (
<>
<h1>Not Found</h1>
<button onClick={() => navigate({ to: '/another-404' as any })}>
Go to Another 404
</button>
</>
)
},
})

const indexRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/',
component: () => {
return (
<>
<h1>Index</h1>
</>
)
},
})

const router = createRouter({
routeTree: rootRoute.addChildren([indexRoute]),
history,
})

render(<RouterProvider router={router} />)

await router.navigate({ to: '/non-existent' })

expect(
await screen.findByRole('heading', { name: 'Not Found' }),
).toBeInTheDocument()

const anotherButton = await screen.findByRole('button', {
name: 'Go to Another 404',
})
fireEvent.click(anotherButton)

expect(
await screen.findByRole('heading', { name: 'Not Found' }),
).toBeInTheDocument()

expect(window.location.pathname).toBe('/non-existent')
})

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()
})
Comment on lines +568 to +595
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Incomplete test coverage for external URL blocker behavior.

The test only verifies that router.navigate resolves without throwing, but doesn't actually verify that:

  1. A blocker would normally prevent external navigation
  2. The ignoreBlocker option 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.

Suggested change
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.

})
22 changes: 21 additions & 1 deletion packages/router-core/src/router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1974,7 +1974,7 @@ export class RouterCore<
*
* @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 }) => {
if (!reloadDocument && href) {
try {
new URL(`${href}`)
Expand All @@ -1987,6 +1987,26 @@ export class RouterCore<
const location = this.buildLocation({ to, ...rest } as any)
href = location.url
}

// 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()
}
}
}
}
Comment on lines +1991 to +2008
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
// 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.


if (rest.replace) {
window.location.replace(href)
} else {
Expand Down
15 changes: 14 additions & 1 deletion packages/solid-router/src/useBlocker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,13 @@ export function useBlocker(
const parsedLocation = router.parseLocation(location)
const matchedRoutes = router.getMatchedRoutes(parsedLocation.pathname)
if (matchedRoutes.foundRoute === undefined) {
throw new Error(`No route found for location ${location.href}`)
return {
routeId: '__notFound__',
fullPath: parsedLocation.pathname,
pathname: parsedLocation.pathname,
params: matchedRoutes.routeParams,
search: parsedLocation.search,
}
}
return {
routeId: matchedRoutes.foundRoute.id,
Expand All @@ -202,6 +208,13 @@ export function useBlocker(
const current = getLocation(blockerFnArgs.currentLocation)
const next = getLocation(blockerFnArgs.nextLocation)

if (
current.routeId === '__notFound__' &&
next.routeId !== '__notFound__'
) {
return false
}

const shouldBlock = await props.shouldBlockFn({
action: blockerFnArgs.action,
current,
Expand Down
Loading
Loading