fix: prevent infinite redirect loop in beforeEnter guards#1924
fix: prevent infinite redirect loop in beforeEnter guards#1924madhuri-perumalla wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe router's redirect mechanism is refactored from a dedicated ChangesRouter redirect and navigation refactor
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant Router
participant BeforeEnter
Test->>Router: push(path)
Router->>Router: _navigateTo(path)
Router->>BeforeEnter: beforeEnter(to)
BeforeEnter-->>Router: returns string, false, or undefined
alt string redirect
Router->>Router: increment _redirectDepth
alt depth exceeds _maxRedirectDepth
Router->>Test: emit error("Too many redirects")
else
Router->>Router: _navigateTo(redirectPath)
end
else false
Router->>Test: navigation blocked
else no redirect
Router->>Router: reset _redirectDepth
Router->>Test: emit navigate {match, screen}
end
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/router/src/redirect.test.ts (1)
1-119: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWire
maxRedirectDepththroughRouterOptions
RouterOptionsstill hardcodes the redirect limit at 10. AddmaxRedirectDepthto the options and read it in the constructor sonew Router({ maxRedirectDepth: N })actually changes redirect handling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router/src/redirect.test.ts` around lines 1 - 119, RouterOptions currently ignores a configurable redirect limit and still uses a fixed depth of 10. Update the Router constructor and RouterOptions types so maxRedirectDepth is accepted from new Router({ maxRedirectDepth: N }) and used by the redirect handling logic instead of the hardcoded default; check the Router class and its redirect depth tracking/loop detection path so the new option flows through end-to-end.packages/router/src/router.ts (1)
136-136: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDefer pending initial navigation until
addRoutes()finishes.Each
addRoute()calls_applyInitialPathIfPending(). In a batch, an initial route whose guard redirects to a route registered later in the sameaddRoutes()call will fail before the target exists and clear the pending path. Suspend initial-path application during the loop, then apply once after all routes are registered.Also applies to: 152-158, 226-233
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router/src/router.ts` at line 136, The pending initial navigation is being applied too early inside the addRoute()/addRoutes() flow, so a guard redirect can clear the pending path before later routes in the same batch are registered. Update Router.addRoutes() to suspend _applyInitialPathIfPending() while iterating through the routes, and only invoke it once after all addRoute() calls complete; keep the change localized around the addRoute(), addRoutes(), and _applyInitialPathIfPending() logic.
🧹 Nitpick comments (1)
packages/router/src/redirect.test.ts (1)
99-119: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReset test doesn't distinguish reset-vs-no-reset behavior.
With only 2 redirects per
push('/a')call andmaxRedirectDepthof 10, cumulative depth across both pushes (4 total) stays well under the threshold even if the reset were missing entirely. This test would still pass with a broken (non-resetting) implementation. Consider increasing the redirect chain length or repeat count so the total without reset would exceedmaxRedirectDepth, e.g. callpush('/a')at least 6 times (6×2=12 > 10) and assert no error across all calls.🧪 Suggested strengthening
router.push('/a'); expect(router.currentPath).toBe('/c'); expect(errorHandler).not.toHaveBeenCalled(); - // Navigate again should reset depth - router.push('/a'); - expect(router.currentPath).toBe('/c'); - expect(errorHandler).not.toHaveBeenCalled(); + // Navigate again multiple times; without a reset, cumulative depth + // would exceed maxRedirectDepth (10) and trigger an error. + for (let i = 0; i < 6; i++) { + router.push('/a'); + expect(router.currentPath).toBe('/c'); + } + expect(errorHandler).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router/src/redirect.test.ts` around lines 99 - 119, The redirect reset test in redirect.test.ts does not actually prove that redirect depth is reset between navigations because two redirects per Router.push('/a') stay below maxRedirectDepth even if the reset is broken. Strengthen the test around Router.push and the redirect chain (/a -> /b -> /c) by repeating the navigation enough times that cumulative depth would exceed maxRedirectDepth without a reset, then assert no errorHandler calls across all pushes and currentPath remains '/c'.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/router/src/router.ts`:
- Around line 100-118: The options normalization in router should remove
unqualified `any` usage and unexplained assertions. Update the `finalOptions`
typing in `router.ts` to use a shared typed alias for `lazy`, `beforeEnter`, and
`afterEnter` instead of `Promise<any>`, and replace the `meta as any` /
delete-casts with a typed approach or add explicit inline comments where an
assertion is unavoidable. Keep the fix localized to the normalization logic
around `finalOptions`, `childrenOrOptions`, and the `strippedMeta` handling.
- Line 7: `addRoute()` and `addRoutes()` in `router.ts` dropped support for the
existing `redirect` route shape even though `Route` still includes `redirect?:
RedirectTarget`. Restore backward compatibility by accepting `redirect` in the
route inputs, normalizing it into the equivalent `beforeEnter` redirect
behavior, and ensuring both `addRoute` and `addRoutes` continue to forward
existing call shapes without breaking callers. Use the `Route`, `RouteMatch`,
and route registration logic in `router.ts` to keep the API compatible while
preserving current behavior.
- Around line 183-185: The no-match branch in Router navigation leaves
_redirectDepth uncleared, so a failed redirected target can poison later
navigations and trigger “Too many redirects.” Update the no-match handling in
both _navigateTo() and replace() to reset _redirectDepth before returning, using
the existing Router class logic around the match check and redirect flow.
- Around line 291-304: The history replay logic in back() and forward() bypasses
route guards by setting _currentMatch and emitting events directly, so guarded
or redirecting routes can be entered from history without the same checks used
by push() and replace(). Update the route replay flow in router.ts to run the
same beforeEnter guard path before committing the match, and only update
_currentMatch, unmountAll(), and emit the back/forward event after the guard
allows the route; use the existing routing helpers in Router to keep behavior
consistent.
- Around line 188-194: In Router.push, avoid mutating navigation state before
beforeEnter completes: _forwardStack is being cleared too early, so cancelled or
redirected navigations still lose forward history. Move the _forwardStack reset
into the successful commit path in push() right before _history.push(path), and
keep the guard handling around match.route.beforeEnter and the redirect/error
path unchanged so only committed navigations clear forward history.
- Around line 46-51: The redirect cap in Router is currently hard-coded, so
callers cannot configure it through RouterOptions. Update the Router constructor
to assign _maxRedirectDepth from options.maxRedirectDepth ?? 10, and add
maxRedirectDepth to the RouterOptions type/interface if it is missing so the
setting is exposed consistently. Use Router and RouterOptions to locate the
wiring point.
---
Outside diff comments:
In `@packages/router/src/redirect.test.ts`:
- Around line 1-119: RouterOptions currently ignores a configurable redirect
limit and still uses a fixed depth of 10. Update the Router constructor and
RouterOptions types so maxRedirectDepth is accepted from new Router({
maxRedirectDepth: N }) and used by the redirect handling logic instead of the
hardcoded default; check the Router class and its redirect depth tracking/loop
detection path so the new option flows through end-to-end.
In `@packages/router/src/router.ts`:
- Line 136: The pending initial navigation is being applied too early inside the
addRoute()/addRoutes() flow, so a guard redirect can clear the pending path
before later routes in the same batch are registered. Update Router.addRoutes()
to suspend _applyInitialPathIfPending() while iterating through the routes, and
only invoke it once after all addRoute() calls complete; keep the change
localized around the addRoute(), addRoutes(), and _applyInitialPathIfPending()
logic.
---
Nitpick comments:
In `@packages/router/src/redirect.test.ts`:
- Around line 99-119: The redirect reset test in redirect.test.ts does not
actually prove that redirect depth is reset between navigations because two
redirects per Router.push('/a') stay below maxRedirectDepth even if the reset is
broken. Strengthen the test around Router.push and the redirect chain (/a -> /b
-> /c) by repeating the navigation enough times that cumulative depth would
exceed maxRedirectDepth without a reset, then assert no errorHandler calls
across all pushes and currentPath remains '/c'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 062cc92d-e604-407d-b48e-6eff399c1a35
📒 Files selected for processing (3)
packages/router/src/redirect.test.tspackages/router/src/router.test.tspackages/router/src/router.ts
💤 Files with no reviewable changes (1)
- packages/router/src/router.test.ts
| import { EventEmitter } from '@termuijs/core'; | ||
| import { createElement, ErrorBoundary, unmountAll, type VNode } from '@termuijs/jsx'; | ||
| import { type Route, type RouteMatch, type RouteParams, type RouteMeta, type QueryParams, type RedirectTarget, matchRoute, compilePattern } from './route.js'; | ||
| import { type Route, type RouteMatch, type RouteParams, type RouteMeta, matchRoute, compilePattern } from './route.js'; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep redirect route registrations backward compatible.
addRoute() / addRoutes() no longer accept or forward redirect, while Route still exposes redirect?: RedirectTarget. Existing call shapes should keep working by accepting redirect and normalizing it to a beforeEnter redirect. As per coding guidelines, packages/router/**/router.ts: “Keep addRoute and addRoutes backward compatible; existing call shapes must keep working”.
Also applies to: 63-80, 93-104, 141-157
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/router/src/router.ts` at line 7, `addRoute()` and `addRoutes()` in
`router.ts` dropped support for the existing `redirect` route shape even though
`Route` still includes `redirect?: RedirectTarget`. Restore backward
compatibility by accepting `redirect` in the route inputs, normalizing it into
the equivalent `beforeEnter` redirect behavior, and ensuring both `addRoute` and
`addRoutes` continue to forward existing call shapes without breaking callers.
Use the `Route`, `RouteMatch`, and route registration logic in `router.ts` to
keep the API compatible while preserving current behavior.
Source: Coding guidelines
| private _redirectDepth = 0; | ||
| private readonly _maxRedirectDepth = 10; | ||
| readonly events = new EventEmitter<RouterEvents>(); | ||
|
|
||
| constructor(options: RouterOptions = {}) { | ||
| this._maxHistory = options.maxHistory ?? 100; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wire maxRedirectDepth through RouterOptions.
The limit is hard-coded to 10, so callers cannot configure the redirect cap as intended. Assign it from options.maxRedirectDepth ?? 10 and add the option to RouterOptions if it is not already there.
Proposed fix
- private readonly _maxRedirectDepth = 10;
+ private readonly _maxRedirectDepth: number;
@@
constructor(options: RouterOptions = {}) {
this._maxHistory = options.maxHistory ?? 100;
+ this._maxRedirectDepth = options.maxRedirectDepth ?? 10;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/router/src/router.ts` around lines 46 - 51, The redirect cap in
Router is currently hard-coded, so callers cannot configure it through
RouterOptions. Update the Router constructor to assign _maxRedirectDepth from
options.maxRedirectDepth ?? 10, and add maxRedirectDepth to the RouterOptions
type/interface if it is missing so the setting is exposed consistently. Use
Router and RouterOptions to locate the wiring point.
| let finalOptions: { | ||
| lazy?: () => Promise<any>; | ||
| beforeEnter?: (to: string) => boolean | string; | ||
| afterEnter?: (to: string) => void; | ||
| redirect?: RedirectTarget; | ||
| } | undefined = undefined; | ||
| } | undefined = options; | ||
|
|
||
| if (Array.isArray(childrenOrOptions)) { | ||
| children = childrenOrOptions; | ||
| } else if (childrenOrOptions && typeof childrenOrOptions === 'object') { | ||
| finalOptions = childrenOrOptions as any; | ||
| } | ||
|
|
||
| if (typeof options === 'string' || typeof options === 'function') { | ||
| finalOptions = { ...finalOptions, redirect: options }; | ||
| } else if (options && typeof options === 'object') { | ||
| finalOptions = options; | ||
| finalOptions = childrenOrOptions; | ||
| } | ||
|
|
||
| let finalMeta = meta ?? {}; | ||
| if (options === undefined && meta && typeof meta === 'object' && ('lazy' in meta || 'beforeEnter' in meta || 'afterEnter' in meta || 'redirect' in meta)) { | ||
| if (options === undefined && meta && typeof meta === 'object' && ('lazy' in meta || 'beforeEnter' in meta || 'afterEnter' in meta)) { | ||
| finalOptions = meta as any; | ||
| const strippedMeta = { ...meta }; | ||
| delete (strippedMeta as any).lazy; | ||
| delete (strippedMeta as any).beforeEnter; | ||
| delete (strippedMeta as any).afterEnter; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid any and unexplained assertions in the changed options normalization.
The changed normalization path still uses Promise<any> / as any without inline justification. Prefer a shared typed options alias such as Pick<Route, 'lazy' | 'beforeEnter' | 'afterEnter'>, or add explicit inline comments where assertions are unavoidable. As per coding guidelines, **/*.{ts,tsx}: “No any without an inline comment explaining why. No type assertions without an inline comment explaining why.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/router/src/router.ts` around lines 100 - 118, The options
normalization in router should remove unqualified `any` usage and unexplained
assertions. Update the `finalOptions` typing in `router.ts` to use a shared
typed alias for `lazy`, `beforeEnter`, and `afterEnter` instead of
`Promise<any>`, and replace the `meta as any` / delete-casts with a typed
approach or add explicit inline comments where an assertion is unavoidable. Keep
the fix localized to the normalization logic around `finalOptions`,
`childrenOrOptions`, and the `strippedMeta` handling.
Source: Coding guidelines
| if (!match) { | ||
| if (this._notFound) { | ||
| if (options.clearForwardStack) { | ||
| this._forwardStack = []; | ||
| } | ||
|
|
||
| const { modifyHistory = 'push', direction = 'push' } = options; | ||
|
|
||
| if (modifyHistory === 'push') { | ||
| this._history.push(resolvedPath); | ||
|
|
||
| if (this._history.length > this._maxHistory) { | ||
| this._history = this._history.slice(-this._maxHistory); | ||
| } | ||
| } else if (modifyHistory === 'replace') { | ||
| if (this._history.length > 0) { | ||
| this._history[this._history.length - 1] = resolvedPath; | ||
| } else { | ||
| this._history.push(resolvedPath); | ||
| } | ||
| } | ||
|
|
||
| const notFoundMatch = this._createNotFoundMatch(resolvedPath); | ||
| this._currentMatch = notFoundMatch; | ||
| if (this.autoUnmount) unmountAll(); | ||
| const screen = this._wrapScreen(notFoundMatch); | ||
| const emitEvent = direction === 'back' ? 'back' : 'navigate'; | ||
| this.events.emit(emitEvent, { match: notFoundMatch, screen, direction }); | ||
| return; | ||
| } | ||
|
|
||
| this.events.emit('error', new Error(`No route found for path: ${resolvedPath}`)); | ||
| this.events.emit('error', new Error(`No route found for path: ${path}`)); | ||
| return; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reset redirect depth when a redirected target is missing.
If a guard redirects to an unmatched route, _redirectDepth remains non-zero because the no-match branches return without clearing it. The next unrelated redirect chain can then fail early with “Too many redirects”.
Proposed fix
if (!match) {
+ this._redirectDepth = 0;
this.events.emit('error', new Error(`No route found for path: ${path}`));
return;
}Apply the same reset in both _navigateTo() and replace().
Also applies to: 239-241
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/router/src/router.ts` around lines 183 - 185, The no-match branch in
Router navigation leaves _redirectDepth uncleared, so a failed redirected target
can poison later navigations and trigger “Too many redirects.” Update the
no-match handling in both _navigateTo() and replace() to reset _redirectDepth
before returning, using the existing Router class logic around the match check
and redirect flow.
| // A new push(path) clears the forward stack | ||
| this._forwardStack = []; | ||
| const guardResult = match.route.beforeEnter?.(path); | ||
|
|
||
| if (guardResult === false) { | ||
| this._redirectDepth = 0; | ||
| return; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clear forward history only after push() commits.
push() clears _forwardStack before beforeEnter runs, so a guard returning false or a redirect error mutates navigation state even though no navigation happened. Move the clear to the successful commit path before _history.push(path).
Proposed fix
- // A new push(path) clears the forward stack
- this._forwardStack = [];
const guardResult = match.route.beforeEnter?.(path);
@@
this._redirectDepth = 0;
+ // A committed push(path) clears the forward stack
+ this._forwardStack = [];
this._history.push(path);Also applies to: 208-210
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/router/src/router.ts` around lines 188 - 194, In Router.push, avoid
mutating navigation state before beforeEnter completes: _forwardStack is being
cleared too early, so cancelled or redirected navigations still lose forward
history. Move the _forwardStack reset into the successful commit path in push()
right before _history.push(path), and keep the guard handling around
match.route.beforeEnter and the redirect/error path unchanged so only committed
navigations clear forward history.
| const prevPath = this._history[this._history.length - 1]; | ||
| const match = prevPath ? matchRoute(prevPath, this._routes) : null; | ||
|
|
||
| this._currentMatch = match; | ||
| if (this.autoUnmount) unmountAll(); | ||
| const screen = this._wrapScreen(match); | ||
|
|
||
| this.events.emit('back', { match, screen, direction: 'back' }); | ||
| if (match) { | ||
| unmountAll(); | ||
|
|
||
| const screen = this._wrapScreen(match); | ||
|
|
||
| match.route.afterEnter?.(prevPath); | ||
| this.events.emit('back', { match, screen }); | ||
| } else { | ||
| this.events.emit('back', null); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Run beforeEnter when replaying history entries.
back() and forward() set _currentMatch and emit UI events without running guards, so a guarded or redirecting route can be entered through history even when push() / replace() would block or redirect it. Route history replay should share the guard path before committing the match.
Also applies to: 314-325
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/router/src/router.ts` around lines 291 - 304, The history replay
logic in back() and forward() bypasses route guards by setting _currentMatch and
emitting events directly, so guarded or redirecting routes can be entered from
history without the same checks used by push() and replace(). Update the route
replay flow in router.ts to run the same beforeEnter guard path before
committing the match, and only update _currentMatch, unmountAll(), and emit the
back/forward event after the guard allows the route; use the existing routing
helpers in Router to keep behavior consistent.
1 participant
Description
This PR fixes an issue where
beforeEnterroute guards could redirect between routes indefinitely, resulting in infinite navigation loops that could freeze the application or cause a stack overflow.The router now tracks redirect depth during a navigation and throws an error when the configured maximum redirect limit is exceeded, preventing runaway redirect chains while preserving valid redirect behavior.
Related Issue
Closes #1918
Which package(s)?
Type of Change
type:bug)type:testing)Checklist
bun vitest runbun run buildbun run typecheckCONTRIBUTING.md.type: short description.markDirty()(if applicable).anytypes without an inline comment explaining why.GSSoC 2026 Participation
Notes for the Reviewer
Changes
maxRedirectDepthoption toRouterOptions(default:10).push()andreplace()navigation flows.Tests
push().replace().This change is backward compatible for existing applications while preventing application hangs caused by misconfigured route guards.
Summary by CodeRabbit