Make useBlocker exception on unknown routes optional #4965
Conversation
WalkthroughAdds UseBlockerOpts.throwOnUnknownRoute?: boolean (runtime default true), makes blocker Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Router
participant useBlocker
participant Parser as parseLocation
participant Guard as throwOnUnknownRoute?
App->>Router: navigate(to)
Router->>useBlocker: run blocker check
useBlocker->>Parser: parse current location
Parser-->>useBlocker: current | undefined
useBlocker->>Parser: parse next location
Parser-->>useBlocker: next | undefined
useBlocker->>Guard: is throwOnUnknownRoute true?
alt throwOnUnknownRoute && (current or next is undefined)
useBlocker-->>Router: throw Error("No route found for location <href>")
Router-->>App: error
else
useBlocker-->>Router: call shouldBlockFn({ current?, next? })
Router-->>App: proceed or block navigation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (7)
packages/solid-router/tests/useBlocker.test.tsx (1)
498-543: Unknown-route test covers the new flag; minor consistency and assertion nitpicksThe test effectively exercises throwOnUnknownRoute: false and validates the Not Found path. Two small improvements:
- Use the same event helper throughout the file for consistency.
- Optionally assert the URL change to '/unknown' to tighten the expectation.
Apply this diff to use fireEvent and add an optional pathname assertion:
- const unknownLink = screen.getByTestId('unknown-link') - unknownLink.click() + const unknownLink = screen.getByTestId('unknown-link') + fireEvent.click(unknownLink) @@ - expect(notFoundComponent).toBeInTheDocument() + expect(notFoundComponent).toBeInTheDocument() + // Optional: also assert the URL changed to the unknown path + expect(window.location.pathname).toBe('/unknown')Question: do we still need await router.load() here? The rest of the tests don’t call it. If not required for this scenario, removing it could shave a bit of test time.
packages/solid-router/src/useBlocker.tsx (2)
45-47: Making current/next optional in public shapes is sensible; ensure docs calloutAllowing current and next to be undefined in BlockerResolver and ShouldBlockFnArgs matches the runtime behavior when navigating to unknown routes. This is a widening change and source-compatible, but it does introduce potential undefined at runtime for consumers who didn’t guard previously. Please add a brief note in the API docs/CHANGELOG so users know to guard or use optional chaining.
Also applies to: 61-63
207-219: Guarded throws on unknown current/next are correct; consider DRYing the duplicationBoth current and next unknown are handled with near-identical throw logic. Not a big deal, but a tiny helper would reduce duplication and keep messages consistent.
Example refactor within blockerFnComposed:
- const current = getLocation(blockerFnArgs.currentLocation) - if (!current && props.throwOnUnknownRoute) { - throw new Error( - `No route found for location ${blockerFnArgs.currentLocation.href}`, - ) - } + const current = getLocation(blockerFnArgs.currentLocation) + const ensureKnown = (loc: any, historyLoc: HistoryLocation, label: 'current' | 'next') => { + if (!loc && props.throwOnUnknownRoute) { + throw new Error(`No route found for ${label} location ${historyLoc.href}`) + } + } + ensureKnown(current, blockerFnArgs.currentLocation, 'current') const next = getLocation(blockerFnArgs.nextLocation) - if (!next && props.throwOnUnknownRoute) { - throw new Error( - `No route found for location ${blockerFnArgs.nextLocation.href}`, - ) - } + ensureKnown(next, blockerFnArgs.nextLocation, 'next')packages/solid-router/tests/useBlocker.test-d.tsx (1)
68-68: Typo: “corrent” → “correct” in test titleSmall spelling fix to improve readability in test descriptions.
-test('shouldBlockFn has corrent action', () => { +test('shouldBlockFn has correct action', () => {packages/react-router/src/useBlocker.tsx (2)
76-77: New option throwOnUnknownRoute with default true—LGTM; consider documenting inlineThe default preserves existing behavior. Consider adding a short JSDoc on the option for clarity in editors/IDE.
Proposed inline doc:
export type UseBlockerOpts< TRouter extends AnyRouter = RegisteredRouter, TWithResolver extends boolean = boolean, > = { shouldBlockFn: ShouldBlockFn<TRouter> enableBeforeUnload?: boolean | (() => boolean) disabled?: boolean withResolver?: TWithResolver - throwOnUnknownRoute?: boolean + /** + * When true (default), navigating to a location that does not match any route + * will throw an error from the blocker. Set to false to allow such navigation + * (useful during progressive adoption or when linking to non-router pages). + */ + throwOnUnknownRoute?: boolean }Also applies to: 161-162
199-203: Unknown-route error throws: minor messaging/DRY nitThrowing only when
throwOnUnknownRouteis true is correct. Two small nits:
- Error message could include
pathnameandsearchfor readability in tests/logs.- The two throw branches are duplicated; a tiny helper (e.g.,
assertKnownRoute) would reduce repetition.No functional change required.
Also applies to: 205-209
packages/react-router/tests/useBlocker.test.tsx (1)
451-497: Use RTL’s fireEvent.click for consistency– Replace the direct
.click()call withfireEvent.click
– Add a complementary test to verify that the defaultthrowOnUnknownRoute=truestill throws on navigation to an unknown pathSuggested diff:
- const unknownLink = screen.getByTestId('unknown-link') - unknownLink.click() + const unknownLink = screen.getByTestId('unknown-link') + fireEvent.click(unknownLink)Consider adding, for example:
test('throws when navigating to an unknown route by default', async () => { // setup routes without throwOnUnknownRoute override… expect(() => { fireEvent.click(screen.getByTestId('unknown-link')) }).toThrow(/No route found for location/) })[optional_refactors_recommended]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
packages/react-router/src/useBlocker.tsx(7 hunks)packages/react-router/tests/useBlocker.test-d.tsx(2 hunks)packages/react-router/tests/useBlocker.test.tsx(3 hunks)packages/solid-router/src/useBlocker.tsx(6 hunks)packages/solid-router/tests/useBlocker.test-d.tsx(2 hunks)packages/solid-router/tests/useBlocker.test.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/solid-router/tests/useBlocker.test.tsx (1)
packages/solid-router/src/useBlocker.tsx (1)
useBlocker(160-269)
packages/react-router/src/useBlocker.tsx (1)
packages/history/src/index.ts (1)
location(162-164)
🔇 Additional comments (13)
packages/solid-router/tests/useBlocker.test.tsx (2)
9-10: Public API import of Link and Outlet looks correctGood to see these exported through ../src and exercised in tests; this validates consumer-facing availability.
350-351: Optional chaining on next is appropriate with the new APIGuarding with next?.fullPath aligns with current/next potentially being undefined.
packages/solid-router/src/useBlocker.tsx (3)
77-78: New option throwOnUnknownRoute on UseBlockerOpts is well-placedThe option belongs here and lines up with the new behavior. No issues.
169-171: Default throwOnUnknownRoute: true preserves existing behaviorMerging defaults with Solid.mergeProps keeps prior semantics unless explicitly opted out. Good backward compatibility stance.
189-197: Returning undefined for getLocation on unknown routes is the right pivotDeferring the decision to throw to the flag instead of within getLocation simplifies reuse and testing.
packages/solid-router/tests/useBlocker.test-d.tsx (1)
109-111: Type expectations updated to reflect optional current/nextUsing .exclude(undefined) before narrowing to routeId types matches the widened ShouldBlockFnArgs. Looks good.
Also applies to: 118-120
packages/react-router/src/useBlocker.tsx (3)
44-46: Making current/next optional matches runtime semantics—LGTMAllowing
current/nextto beundefinedin both the resolver andshouldBlockFnargs aligns with the new unknown-route behavior and avoids unsafe assumptions. Type tests reflect this correctly.Also applies to: 59-62
178-181: getLocation returning undefined on unmatched routes—LGTMThis cleanly distinguishes known vs. unknown routes without throwing, deferring the decision to the
throwOnUnknownRouteguard and consumer code.Also applies to: 186-196
258-259: Including throwOnUnknownRoute in effect deps—LGTMEnsures the blocker is rebound if this flag toggles at runtime.
packages/react-router/tests/useBlocker.test-d.tsx (1)
112-113: Type narrowing with .exclude(undefined) is correctThese assertions reflect the updated API where
current/nextcan beundefined. Solid type-level coverage.Also applies to: 121-122
packages/react-router/tests/useBlocker.test.tsx (3)
4-9: Import reshuffle—LGTMNo functional impact; consistent with RTL usage patterns.
13-15: Public exports Link/Outlet use in tests—LGTMGood exercise of the public surface and aligns with the new unknown-route test setup.
369-371: Optional chaining on next.fullPath avoids undefined access—LGTMMatches the new
useBlockertyping wherenextcan beundefined.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/react-router/src/useBlocker.tsx (5)
44-46: Type surface widened: BlockerResolver.current/next can be undefinedThis is a breaking type change for consumers using the resolver (e.g., UI code reading resolver.next.fullPath now needs guards). If the intended runtime behavior with throwOnUnknownRoute=false is to “allow navigation to proceed,” consider short-circuiting before exposing a blocked resolver with undefined locations. That would let you keep the resolver’s blocked shape non-undefined and avoid the breaking type change.
If you choose that path, add an early return (see separate comment with diff) and you could revert the type widening here.
60-62: Widening ShouldBlockFnArgs to include undefined may break existing shouldBlockFn implementationsExisting user-provided shouldBlockFn that assume non-undefined current/next will now need null checks. If you want to preserve backward compatibility and the “proceed” semantics for unknown routes, consider early-returning false (don’t block) when either location is unknown and keep ShouldBlockFnArgs non-undefined. Otherwise, please document this new possibility prominently.
Can you confirm that docs/examples and migration notes are updated to reflect that current/next can now be undefined in shouldBlockFn?
199-204: Clarify error message context (say “current” and suggest the option)Differentiate between current vs next to aid debugging and hint at the new option. Example tweak:
- throw new Error( - `No route found for location ${blockerFnArgs.currentLocation.href}`, - ) + throw new Error( + `useBlocker: No route found for current location ${blockerFnArgs.currentLocation.href}. ` + + `Set throwOnUnknownRoute: false to allow navigation to proceed.` + )
205-209: Clarify error message for “next” location as wellMirror the “current” message for next:
- throw new Error( - `No route found for location ${blockerFnArgs.nextLocation.href}`, - ) + throw new Error( + `useBlocker: No route found for next location ${blockerFnArgs.nextLocation.href}. ` + + `Set throwOnUnknownRoute: false to allow navigation to proceed.` + )
210-216: Optional: Short-circuit instead of calling shouldBlockFn with undefinedIf the intent of throwOnUnknownRoute=false is to “let navigation proceed” for unknown routes, you can avoid invoking shouldBlockFn with undefined and prevent exposing a blocked resolver with undefined locations. This also reduces the type surface change pressure.
const next = getLocation(blockerFnArgs.nextLocation) if (!next && throwOnUnknownRoute) { throw new Error( `No route found for location ${blockerFnArgs.nextLocation.href}`, ) } + // Optional: allow unknown routes to proceed without invoking shouldBlockFn + // This preserves previous ShouldBlockFnArgs shape and avoids blocking with undefined locations. + if (!current || !next) { + return false + } + const shouldBlock = await shouldBlockFn({ action: blockerFnArgs.action, current, next, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/react-router/src/useBlocker.tsx(7 hunks)packages/solid-router/src/useBlocker.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/solid-router/src/useBlocker.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/react-router/src/useBlocker.tsx (1)
packages/history/src/index.ts (1)
location(162-164)
🔇 Additional comments (4)
packages/react-router/src/useBlocker.tsx (4)
76-77: New option throwOnUnknownRoute is clear and keeps default behavior intactOption looks good and the default true preserves existing behavior. Nice addition for progressive adoption scenarios.
161-162: Default assignment preserves current behaviorDestructuring with throwOnUnknownRoute = true maintains the existing runtime default for all call sites (including legacy ones). LGTM.
187-187: Returning undefined for unmatched routes aligns with the new optionThis is the correct pivot to enable the configurable behavior and lines up with the test intent described in the PR.
258-259: Including throwOnUnknownRoute in useEffect deps is correctEnsures the blocker re-registers when the option changes. LGTM.
| ): AnyShouldBlockFnLocation|undefined { | ||
| const parsedLocation = router.parseLocation(location) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm parseLocation signature change (1 arg vs 2 args)
This switches to router.parseLocation(location). If parseLocation previously accepted two params (e.g., parseLocation(undefined, location)), please verify the current signature across router-core so we don’t silently break on older versions or alternate adapters.
🏁 Script executed:
#!/bin/bash
# Verify parseLocation signature(s) across the repo
# 1) Look for parseLocation declarations (function or const)
rg -nP -C2 '(function|const)\s+parseLocation\s*\(' --type=ts --no-ignore-vcs
# 2) Inspect all call sites for argument count
rg -nP -C2 'parseLocation\s*\(' --type=ts --no-ignore-vcsLength of output: 2920
🏁 Script executed:
#!/bin/bash
echo "=== Router-core parseLocation definition context ==="
rg -nP "parseLocation" -C5 packages/router-core/src/router.ts
echo
echo "=== History parseLocation definition context ==="
rg -nP "function parseLocation" -C5 packages/history/src/index.tsLength of output: 1465
Fix parseLocation usage in useBlocker
The call
const parsedLocation = router.parseLocation(location)passes your location as the first parameter (previousLocation), leaving the actual target (locationToParse) undefined. In router-core, parseLocation is still defined as
parseLocation: ParseLocationFn<TRouteTree> = (
previousLocation,
locationToParse,
) => { … }so you must supply both arguments.
• packages/react-router/src/useBlocker.tsx (around line 181)
– Change
const parsedLocation = router.parseLocation(location)
– To
const parsedLocation = router.parseLocation(undefined, location)
🤖 Prompt for AI Agents
In packages/react-router/src/useBlocker.tsx around lines 180 to 181, the call to
router.parseLocation passes the target location as the first parameter
(previousLocation) leaving the second parameter undefined; change the call to
pass undefined as the previousLocation and the current location as the second
argument (i.e., call parseLocation(undefined, location)) so both parameters
expected by ParseLocationFn are provided.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/router/framework/react/api/router/useBlockerHook.md (1)
31-35: Clarify whencurrent/nextcan beundefinedrelative tothrowOnUnknownRoute.Great to see the types reflect the new behavior. Add a short note right after the type block to explain that with the default (
throwOnUnknownRoute: true), these will be defined, and they’re only possiblyundefinedwhen the option is set tofalse. This removes ambiguity for readers.Apply this diff:
type ShouldBlockFnArgs = { current: ShouldBlockFnLocation | undefined next: ShouldBlockFnLocation | undefined action: HistoryAction }
+Note: When
throwOnUnknownRouteistrue(default),currentandnextare guaranteed to be defined. SetthrowOnUnknownRoutetofalseto allow unknown routes, in which case either may beundefined.</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** **💡 Knowledge Base configuration:** - MCP integration is disabled by default for public repositories - Jira integration is disabled by default for public repositories - Linear integration is disabled by default for public repositories You can enable these sources in your CodeRabbit configuration. <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between c14c387cb12a55b3fb8835f393128ea29e3a4a60 and 2ac23a7e6f144c595d467143c0de668cf503a342. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `docs/router/framework/react/api/router/useBlockerHook.md` (6 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>docs/router/framework/react/api/router/useBlockerHook.md</summary> [grammar] ~58-~58: There might be a mistake here. Context: ...Route` option - Optional - defaults to `true` - Type: `boolean` - Specify if the hook sh... (QB_NEW_EN) --- [grammar] ~59-~59: There might be a mistake here. Context: ...- Optional - defaults to `true` - Type: `boolean` - Specify if the hook should throw an exce... (QB_NEW_EN) </details> </details> </details> <details> <summary>🔇 Additional comments (2)</summary><blockquote> <details> <summary>docs/router/framework/react/api/router/useBlockerHook.md (2)</summary> `129-129`: **LGTM: safe fallback for unknown destinations.** Using `next?.pathname ?? 'Unknown Path'` is concise and prevents runtime errors when the next location is unavailable. --- `204-207`: **LGTM: safe optional access in type-narrowing example.** Using `current?`/`next?` here is consistent with the new types and avoids crashes when unknown routes are allowed. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| ### `options.throwOnUnknownRoute` option | ||
|
|
||
| - Optional - defaults to `true` | ||
| - Type: `boolean` | ||
| - Specify if the hook should throw an exception when either the current or next locations is unkown | ||
|
|
There was a problem hiding this comment.
Fix typo and grammar in the throwOnUnknownRoute option description.
“locations is unkown” → “location cannot be resolved” (and singular agreement). This is user-facing; fix it.
-### `options.throwOnUnknownRoute` option
-
-- Optional - defaults to `true`
-- Type: `boolean`
-- Specify if the hook should throw an exception when either the current or next locations is unkown
+### `options.throwOnUnknownRoute` option
+
+- Optional - defaults to `true`
+- Type: `boolean`
+- Specify whether the hook should throw when either the current or next location cannot be resolved.📝 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.
| ### `options.throwOnUnknownRoute` option | |
| - Optional - defaults to `true` | |
| - Type: `boolean` | |
| - Specify if the hook should throw an exception when either the current or next locations is unkown | |
| ### `options.throwOnUnknownRoute` option | |
| - Optional - defaults to `true` | |
| - Type: `boolean` | |
| - Specify whether the hook should throw when either the current or next location cannot be resolved. |
🧰 Tools
🪛 LanguageTool
[grammar] ~58-~58: There might be a mistake here.
Context: ...Routeoption - Optional - defaults totrue- Type:boolean` - Specify if the hook sh...
(QB_NEW_EN)
[grammar] ~59-~59: There might be a mistake here.
Context: ...- Optional - defaults to true - Type: boolean - Specify if the hook should throw an exce...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In docs/router/framework/react/api/router/useBlockerHook.md around lines 56 to
61, the description for the `options.throwOnUnknownRoute` option contains a typo
and grammatical error; update the bullet text so it reads: "Specify if the hook
should throw an exception when either the current or next location cannot be
resolved" (use singular "location" and the phrase "cannot be resolved" instead
of "is unkown").
| shouldBlockFn: ({ next }) => { | ||
| return !next.pathname.includes('step/') | ||
| return next && !next.pathname.includes('step/') | ||
| }, |
There was a problem hiding this comment.
Return type bug: expression can return undefined, not boolean.
return next && ... yields undefined when next is undefined, violating the documented contract that shouldBlockFn returns a boolean. Use a ternary or double negation.
- return next && !next.pathname.includes('step/')
+ // If `next` is unknown, do not block
+ return next ? !next.pathname.includes('step/') : false📝 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.
| shouldBlockFn: ({ next }) => { | |
| return !next.pathname.includes('step/') | |
| return next && !next.pathname.includes('step/') | |
| }, | |
| shouldBlockFn: ({ next }) => { | |
| // If `next` is unknown, do not block | |
| return next ? !next.pathname.includes('step/') : false | |
| }, |
🤖 Prompt for AI Agents
In docs/router/framework/react/api/router/useBlockerHook.md around lines 146 to
148, the shouldBlockFn uses "return next && !next.pathname.includes('step/')"
which can return undefined when next is undefined; change it to return an
explicit boolean such as "next ? !next.pathname.includes('step/')" or use
double-negation "!!next && !next.pathname.includes('step/')" so the function
always returns a boolean.
| @@ -170,7 +176,7 @@ function MyComponent() { | |||
|
|
|||
| useBlocker({ | |||
| shouldBlockFn: ({ next }) => { | |||
| if (next.pathname.includes('step/')) { | |||
| if (next?.pathname.includes('step/')) { | |||
There was a problem hiding this comment.
Optional chaining is incomplete; potential runtime error.
next?.pathname.includes(...) will still call .includes on undefined when pathname is missing. Chain ?. on pathname as well.
- if (next?.pathname.includes('step/')) {
+ if (next?.pathname?.includes('step/')) {📝 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.
| if (next?.pathname.includes('step/')) { | |
| if (next?.pathname?.includes('step/')) { |
🤖 Prompt for AI Agents
In docs/router/framework/react/api/router/useBlockerHook.md around line 179, the
expression next?.pathname.includes('step/') can throw if pathname is undefined;
change it to use optional chaining on pathname as well
(next?.pathname?.includes('step/')) so .includes is only called when pathname
exists.
|
thanks for this PR. |
Adds a
throwOnUnknownRouteoption to theuseBlockerhook for both React and Solid.When set to
false, this option preventsuseBlockerfrom throwing an exception if the user navigates to a route that is not recognized by the router. Instead, the navigation is allowed to proceed. This is useful for applications that are progressively adopting the router and may have links that navigate from router-managed pages to non-router-managed pages.The default value is
trueto maintain the existing behavior.I'm not sure the option is really necessary. We could imagine to completely reconsider throwing an exception from this function (I don't see anyway how the user could catch it. Let me know if we already can).
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation