Skip to content

fix(vue-router): clear navigation info when a guard aborts navigation - #31364

Open
thetaPC wants to merge 27 commits into
mainfrom
FW-6706
Open

fix(vue-router): clear navigation info when a guard aborts navigation#31364
thetaPC wants to merge 27 commits into
mainfrom
FW-6706

Conversation

@thetaPC

@thetaPC thetaPC commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Issue number: resolves #29721


What is the current behavior?

A navigation guard that cancels a back navigation leaves Ionic's staged navigation info behind. The next push reads that stale delta, gets mistaken for history traversal, and the incoming route is never added to the location history. The router outlet then destroys a page it should have kept.

What is the new behavior?

  • currentNavigationInfo is cleared before router.afterEach returns on a navigation failure.
  • The clear is skipped for cancelled failures, matching vue-router, which reverts the history entry for aborted and duplicated navigations but leaves it in place when a navigation is superseded.
  • Added a unit spec covering the reported steps.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Dev build: 9.0.1-dev.11788285010.17561004

Co-authored-by: zhiqiang.guo <zguoby@gmail.com>
@vercel

vercel Bot commented Aug 18, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
ionic-framework Ready Ready Preview Sep 1, 2026 5:41pm UTC

Request Review

@github-actions github-actions Bot added the package: vue @ionic/vue package label Aug 18, 2026
@thetaPC
thetaPC marked this pull request as ready for review August 18, 2026 20:34
@thetaPC
thetaPC requested a review from a team as a code owner August 18, 2026 20:34
@thetaPC
thetaPC requested a review from ShaneK August 18, 2026 20:34

@ShaneK ShaneK left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice find on the cancelled issue, matching vue-router's own revert gate is the right call. I think incomingRouteParams needs clearing alongside currentNavigationInfo though, otherwise the ion-back-button path still breaks. A couple of smaller notes on the test as well.

Comment thread packages/vue-router/src/router.ts Outdated
Comment thread packages/vue-router/src/router.ts Outdated
Comment thread packages/vue/test/base/tests/unit/routing.spec.ts Outdated
Comment thread packages/vue/test/base/tests/unit/routing.spec.ts Outdated
Comment thread packages/vue/test/base/tests/unit/routing.spec.ts
Comment thread packages/vue/test/base/tests/unit/routing.spec.ts Outdated
…tion

Co-authored-by: ShaneK <561207+ShaneK@users.noreply.github.com>

@ShaneK ShaneK left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Really nice work on this so far!

Just one thing I'd definitely like to be worked out before we can merge this, which is that dropping the carve-out regresses the opposite ordering, where another back replaces the cancelled one. The rest is mostly nits

Comment thread packages/vue-router/src/router.ts Outdated
Comment thread packages/vue-router/src/router.ts Outdated
Comment thread packages/vue/test/base/tests/unit/routing.spec.ts Outdated
Comment thread packages/vue/test/base/tests/unit/routing.spec.ts Outdated
Comment thread packages/vue/test/base/tests/unit/routing.spec.ts Outdated
Comment thread packages/vue/test/base/tests/unit/routing.spec.ts Outdated
thetaPC and others added 2 commits August 21, 2026 13:47

@ShaneK ShaneK left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Really nice work on this, and I think the stamp was the right call. Two things worry me though. The stamp only got wired into setIncomingRouteParams, so changeTab and handleNavigateBack leave a stale one behind and the gate ends up clearing the wrong navigation's params. And the branch the stamp exists for doesn't have a test. The rest is a couple of inputs the gate can't match, which I don't think need solving here, just naming. I answered your question about the separate variable up in that thread.

Comment thread packages/vue-router/src/router.ts Outdated
Comment thread packages/vue-router/src/router.ts
Comment thread packages/vue/test/base/tests/unit/routing.spec.ts
Comment thread packages/vue-router/src/router.ts Outdated
Comment thread packages/vue-router/src/router.ts Outdated
to: undefined,
};

incomingRouteParams = undefined;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nah, keep it as it is. Your reason holds, and I think there's a better version of it. Since handleNavigateBack re-stages a stored RouteInfo as params, a leaked to wouldn't just end up on a RouteInfo, it'd come back out of locationHistory later as a stale target and give you a confidently wrong match at the gate. That's worse than cosmetic.

The shape isn't really what's biting you though, and I left a comment on the gate about that. Folding to onto the object wouldn't fix changeTab either, since it spreads ...incomingRouteParams and would carry the old target forward just the same. What fixes it is having one place that writes the params, so make setIncomingRouteParams the only writer and give the other two a way through it. You'd still have two variables, but only one spot that can desync them, which is the guarantee the single object would've bought you.

@ShaneK ShaneK left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Really nice work on this so far! There are two more cancellation cases where the newer navigation still loses its params, plus a couple of test follow-ups.

Comment thread packages/vue-router/src/router.ts Outdated
const paramsAreForThisNavigation =
incomingRouteParamsTo === undefined
? deltaIsForThisNavigation
: incomingRouteParamsTo === to.fullPath ||

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The fullPath stamp was my suggestion, but I missed same-target navigations. A held push to /login, followed by replace/root to the same path, still comes out replace/none. Home stays mounted and canGoBack() stays true. Could we give each staged navigation its own identity?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

const prevInfo = locationHistory.findLastLocation(routeInfo);
if (prevInfo) {
incomingRouteParams = {
stageRouteParams({

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suggested leaving this target undefined to fall back to the delta, but I missed the non-linear handleNavigateBack() branch that uses router.replace() without one. If the older push reports cancellation first, the back comes out replace/none instead of pop/back. Could this branch stamp the replace target?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed the approach rather than stamping this branch: params are now matched on the navigation that owns them instead of where it was heading, so this branch stages unclaimed and its replace claims ownership in beforeEach like anything else. No target or delta needed here any more.

Couldn't get a spec onto this branch though. Nothing in the suite reaches it, and every sequence I tried came out with a negative positionDelta and took router.go or the linear back. Let me know if you know the state that gets there.

51e1b8c

router.push('/settings');
await waitForRouter();

expect(currentRoute(navManager)).toEqual({

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like this one got missed from the stack assertion change. The viewStack() check passes with Home and Profile hidden and Settings visible, would you mind adding it here too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

* which cost the tab its direction and its tab name.
*/
navManager.handleNavigate('/details', 'push', 'forward');
await new Promise((resolve) => setTimeout(resolve, 50));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could this use promise gates like the back-navigation test below? They cover the same ordering without the fixed 50ms window or 1.2s wait. Just a test cleanup though, no worries if you don't want to change it here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ShaneK ShaneK left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Really nice work getting this to where it is, the ownership model is looking great. Just two issues that might be worth fixing left and they're both my fault. The fullPath stamp I suggested only ever covered the params half, so the delta still has the same-target hole, and the onError I asked for silences vue-router's own logging for any app that hasn't added a handler. I built and tested a patch for each so the comments have a suggested fix if you'd like to take them. That's up to you, though, and if you think these issues are worth addressing. I'm going to approve this now either way, sorry it's taken so long ☠️

Comment on lines +183 to +185
const deltaIsForThisNavigation =
currentNavigationInfo.to === undefined ||
currentNavigationInfo.to === to.fullPath;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The fullPath stamp was my suggestion and I only moved half of it across. Params match on the navigation now, but the delta still goes off the path, so the same-target problem is still open on this side.

Going /home to /list to /detail, then a push('/list') held in a guard and a browser back to /list: the cancelled push reports first and its path matches what's staged, so the back loses its delta. Main gives pop/back with /detail popped, this branch gives push/forward and /detail stays mounted.

I built and tested this one, it fixes the race and keeps all 27 green. Our history listener runs before vue-router's own setupListeners, so the same beforeEach can claim the delta, and the to field comes back off NavigationInformation. It touches six spots so it isn't a suggestion block, sorry:

-    to: undefined,
+  };
+  let currentNavigationInfoOwner: RouteLocationNormalized | undefined;
+  let currentNavigationInfoUnclaimed = false;
+
+  const clearNavigationInfo = () => {
+    currentNavigationInfo = {
+      direction: undefined,
+      action: undefined,
+      delta: undefined,
+    };
+    currentNavigationInfoOwner = undefined;
+    currentNavigationInfoUnclaimed = false;
   };
   router.beforeEach((to: RouteLocationNormalized) => {
     if (incomingRouteParamsUnclaimed) { ... }
+
+    if (currentNavigationInfoUnclaimed) {
+      currentNavigationInfoOwner = to;
+      currentNavigationInfoUnclaimed = false;
+    }
   });
     const deltaIsForThisNavigation =
-      currentNavigationInfo.to === undefined ||
-      currentNavigationInfo.to === to.fullPath;
+      currentNavigationInfoOwner === undefined
+        ? currentNavigationInfoUnclaimed
+        : currentNavigationInfoOwner === to;
-  opts.history.listen((to: any, _x: any, info: any) => {
+  opts.history.listen((_to: any, _x: any, info: any) => {
       direction: info.direction === "" ? "forward" : info.direction,
-
-      to,
     };
+    currentNavigationInfoOwner = undefined;
+    currentNavigationInfoUnclaimed = true;
   });

Both currentNavigationInfo = { ... } resets become clearNavigationInfo(), and the to field comes out of NavigationInformation.

One honest caveat: an async component leave guard could let another navigation claim the delta before the popstate one does, the same shape as the params fallback. Narrower than the path compare it replaces, but not zero.

A regression test for it, written to match the racing tests already in the file. This fails on the branch as it is and passes with the change:

// Guards against clearing a delta that belongs to another navigation still in flight.
it('should keep the delta of a back navigation that a cancelled push shares a path with', async () => {
  // /home -> /list -> /detail, then a held push('/list') racing a back to /list.
  // Release the push first so it reports cancelled before the back settles.
  holding = true;
  router.push('/list');
  await waitUntil(() => holds.length === 1, 'the push to reach the guard');
  router.back();
  await waitUntil(() => holds.length === 2, 'the back to reach the guard');
  holds[0]();
  await pushCancelled;
  holds[1]();
  await waitForRouter();

  expect(currentRoute(navManager)).toEqual({
    pathname: '/list',
    routerAction: 'pop',
    routerDirection: 'back'
  });
  expect(viewStack(wrapper)).toEqual([
    { id: 'home', hidden: true },
    { id: 'list', hidden: false }
  ]);
});

Comment on lines +81 to +94
/**
* A guard that throws, including an await on a session check that rejects,
* never reaches afterEach. vue-router rejects the navigation promise
* instead, so there is no failure to inspect there and the staged state
* would survive. This does not handle the error, so navigation outcomes are
* unchanged.
*
* A guard that returns a location is still not covered, because that
* redirects rather than fails and afterEach is never called for the original
* navigation.
*/
router.onError((_error: unknown, to: RouteLocationNormalized) => {
discardStagedStateFor(to);
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* A guard that throws, including an await on a session check that rejects,
* never reaches afterEach. vue-router rejects the navigation promise
* instead, so there is no failure to inspect there and the staged state
* would survive. This does not handle the error, so navigation outcomes are
* unchanged.
*
* A guard that returns a location is still not covered, because that
* redirects rather than fails and afterEach is never called for the original
* navigation.
*/
router.onError((_error: unknown, to: RouteLocationNormalized) => {
discardStagedStateFor(to);
});
/**
* vue-router only logs an uncaught navigation error while no error handler
* is registered, so the handler below would silence it for every app that
* has not added one of its own. Track whether the app adds one so the log
* can be put back when it has not.
*/
let appErrorHandlers = 0;
const addErrorHandler = router.onError.bind(router);
router.onError = (handler) => {
appErrorHandlers++;
const removeHandler = addErrorHandler(handler);
return () => {
appErrorHandlers--;
removeHandler();
};
};
/**
* A guard that throws, including an await on a session check that rejects,
* never reaches afterEach. vue-router rejects the navigation promise
* instead, so there is no failure to inspect there and the staged state
* would survive. The error is re-thrown to the app the same way it was
* before, so navigation outcomes are unchanged.
*
* A guard that returns a location is still not covered, because that
* redirects rather than fails and afterEach is never called for the original
* navigation.
*/
addErrorHandler((error: unknown, to: RouteLocationNormalized) => {
discardStagedStateFor(to);
if (appErrorHandlers === 0) {
console.error(error);
}
});

Asking for onError was my idea and I missed that registering a handler changes vue-router's own behaviour. Internally it only logs while errorListeners is empty, so ours kills that branch for any app that hasn't added its own. With a guard that throws, plain vue-router logs an error and a dev warning, and so does main, but this branch logs nothing. The rejected promise is all that's left, and the popstate path swallows that itself, so a throwing guard during a browser back is silent.

The suggestion counts the app's handlers and puts the log back when there are none. I tested it: no handler gets one log, an app handler gets one call and no duplicate log, and unregistering restores the log. We already reassign install and isReady over in index.ts, so patching onError is in keeping. This only brings back console.error, not the dev-only diagnostic, since that one would mean copying vue-router's own message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: vue @ionic/vue package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: ion-router-outlet does not show correct page after vue-router navigation guard was used

2 participants