Skip to content

Commit 5009842

Browse files
atscottatscott
committed
fix(router): reset internal state on error handler redirect after state commit
When an error occurs during route activation (after `BeforeActivateRoutes` has already committed `targetRouterState`) and `withNavigationErrorHandler` returns a `RedirectCommand`, the transition emits a redirecting `NavigationCancel` instead of `NavigationError`. Previously, `StateManager` skipped resetting internal state on all redirecting cancellations because redirects from guards and resolvers happen prior to `BeforeActivateRoutes`. This left the half-activated `targetRouterState` (whose unactivated routes do not yet have `snapshot` assigned) as the current `routerState` for the subsequent redirect navigation. This change ensures `setBrowserUrl` runs before `commitTransition` in `HistoryStateManager` (matching `NavigationStateManager`) and resets internal state if a redirecting cancellation occurs after `targetRouterState` was already committed. Fixes #71126
1 parent 7385890 commit 5009842

3 files changed

Lines changed: 39 additions & 6 deletions

File tree

‎packages/router/src/statemanager/navigation_state_manager.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -320,8 +320,11 @@ export class NavigationStateManager extends StateManager {
320320
this.currentNavigation.rejectNavigateEvent?.();
321321
const clearedState = {}; // Marker to detect if a new navigation started during async ops.
322322
this.currentNavigation = clearedState;
323-
// Do not reset state if we're redirecting or navigation is superseded by a new one.
323+
// Do not reset browser history if we're redirecting or navigation is superseded by a new one.
324324
if (isRedirectingEvent(cause)) {
325+
if (this.routerState === transition.targetRouterState) {
326+
this.resetInternalState(transition.finalUrl, false);
327+
}
325328
return;
326329
}
327330
// Determine if the rollback should be a traversal to a specific previous entry

‎packages/router/src/statemanager/state_manager.ts‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -229,12 +229,16 @@ export class HistoryStateManager extends StateManager {
229229
}
230230
}
231231
} else if (e instanceof BeforeActivateRoutes) {
232-
this.commitTransition(currentTransition);
233232
if (this.urlUpdateStrategy === 'deferred' && !currentTransition.extras.skipLocationChange) {
234233
this.setBrowserUrl(this.createBrowserPath(currentTransition), currentTransition);
235234
}
236-
} else if (e instanceof NavigationCancel && !isRedirectingEvent(e)) {
237-
this.restoreHistory(currentTransition);
235+
this.commitTransition(currentTransition);
236+
} else if (e instanceof NavigationCancel) {
237+
if (!isRedirectingEvent(e)) {
238+
this.restoreHistory(currentTransition);
239+
} else if (this.routerState === currentTransition.targetRouterState) {
240+
this.resetInternalState(currentTransition);
241+
}
238242
} else if (e instanceof NavigationError) {
239243
this.restoreHistory(currentTransition, true);
240244
} else if (e instanceof NavigationEnd) {

‎packages/router/test/integration/navigation_errors.spec.ts‎

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,6 @@ export function navigationErrorsIntegrationSuite(browserAPI: 'history' | 'naviga
132132
{path: 'error', component: BlankCmp},
133133
],
134134
{
135-
resolveNavigationPromiseOnError: true,
136135
errorHandler: () => new RedirectCommand(inject(Router).parseUrl('/error')),
137136
},
138137
),
@@ -171,7 +170,6 @@ export function navigationErrorsIntegrationSuite(browserAPI: 'history' | 'naviga
171170
},
172171
{path: 'error', component: BlankCmp},
173172
],
174-
withRouterConfig({resolveNavigationPromiseOnError: true}),
175173
withNavigationErrorHandler(() => new RedirectCommand(inject(Router).parseUrl('/error'))),
176174
),
177175
],
@@ -219,6 +217,34 @@ export function navigationErrorsIntegrationSuite(browserAPI: 'history' | 'naviga
219217
const router = TestBed.inject(Router);
220218
});
221219

220+
it('can redirect from error handler when a component throws during activation alongside a secondary outlet', async () => {
221+
let errors = 0;
222+
TestBed.configureTestingModule({
223+
providers: [
224+
provideRouter(
225+
[
226+
{path: 'throwing', component: ThrowingCmp},
227+
{path: 'user/:name', outlet: 'aux', component: UserCmp},
228+
{path: 'error', component: SimpleCmp},
229+
],
230+
withNavigationErrorHandler(() => {
231+
errors++;
232+
return errors <= 3 ? new RedirectCommand(inject(Router).parseUrl('/error')) : undefined;
233+
}),
234+
),
235+
],
236+
});
237+
const router = TestBed.inject(Router);
238+
const fixture = await createRoot(router, RootCmp);
239+
240+
await router.navigateByUrl('/throwing(aux:user/victor)');
241+
await advance(fixture);
242+
243+
expect(errors).toBe(1);
244+
expect(router.url).toEqual('/error');
245+
expect(fixture.nativeElement).toHaveText('simple');
246+
});
247+
222248
// Errors should behave the same for both deferred and eager URL update strategies
223249
(['deferred', 'eager'] as const).forEach((urlUpdateStrategy) => {
224250
it(`should dispatch NavigationError after the url has been reset back (${urlUpdateStrategy})`, async () => {

0 commit comments

Comments
 (0)