Skip to content

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

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

Open
thetaPC wants to merge 20 commits into
base: main
Choose a base branch
from
+818 −28
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
e023609
fix(vue-router): clear navigation info when a guard aborts navigation
thetaPC Aug 18, 2026
6d5c199
fix(vue-router): clear staged route params when a guard aborts naviga…
thetaPC Aug 18, 2026
7ae3b16
docs(vue-router): clarify which navigation failures clear staged state
thetaPC Aug 18, 2026
b6c6e73
fix(vue-router): clear staged state when a navigation is cancelled
thetaPC Aug 18, 2026
d4b89b7
test(vue-router): use data-pageid in routing specs
thetaPC Aug 18, 2026
4bbcc7a
test(vue-router): align routing spec setup with the rest of the file
thetaPC Aug 18, 2026
2dc6efd
test(vue-router): tidy the navigation guard specs
thetaPC Aug 18, 2026
7ff2c5e
fix(vue-router): only clear staged state from the failed navigation
thetaPC Aug 20, 2026
b1b2aca
Merge branch 'main' of github.com:ionic-team/ionic-framework into FW-…
thetaPC Aug 20, 2026
1b50d95
docs(vue-router): correct what stages route params and when entries r…
thetaPC Aug 21, 2026
101143e
test(vue-router): cover the back button path and fix a misleading spe…
thetaPC Aug 21, 2026
64a77a6
test(vue-router): read the router through inject instead of the Vue i…
thetaPC Aug 21, 2026
398cb36
test(vue-router): add exclamation
thetaPC Aug 21, 2026
06e0174
test(vue-router): hoist the shared page factory to module scope
thetaPC Aug 21, 2026
67125fe
fix(vue-router): record the target of staged route params
thetaPC Aug 25, 2026
6856c6f
test(vue-router): add definite assignment assertions to the racing specs
thetaPC Aug 25, 2026
6a6032f
docs(vue-router): use a doc block for the staged state comment
thetaPC Aug 25, 2026
f40d2d1
test(vue-router): link only the specs that reproduce the issue
thetaPC Aug 25, 2026
f469679
test(vue-router): assert the view stack and share the spec helpers
thetaPC Aug 25, 2026
150ecb2
fix(vue-router): stage route params through a single writer
thetaPC Aug 26, 2026
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
149 changes: 123 additions & 26 deletions packages/vue-router/src/router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export const createIonRouter = (
direction: undefined,
action: undefined,
delta: undefined,
to: undefined,
};

/**
Expand All @@ -46,7 +47,59 @@ export const createIonRouter = (
_: RouteLocationNormalized,
failure?: NavigationFailure | void
) => {
if (failure) return;
if (failure) {

@ShaneK ShaneK Aug 26, 2026

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.

An async guard that throws instead of returning false never gets here at all, so the original bug still reproduces. Internally triggerError hands back a rejected promise, so the .then that would call triggerAfterEach never runs and the .catch(noop) on the end eats it. No failure object, no afterEach.

You don't need a deliberate throw either, an await on a session check that rejects does it. On /profile, tap back, guard throws, then tap a link to /settings: the URL says /settings but Ionic reports /home with a pop, and the stack collapses down to just Home, so Settings never mounts and Profile gets destroyed. Same on main so nothing regressed, it's just not covered.

Different thing from FW-7699, that one's the guard-returning-a-location case. I think a router.onError doing the same two gated clears would close it, and onError doesn't swallow the error so it wouldn't change any navigation outcomes. Feels cheap enough to do here, but I'm fine with a card if you'd rather keep this PR tight.

/**
* State staged for a navigation that failed describes something that
* did not happen. handleHistoryChange normally consumes it, but it does
* not run when the navigation fails, so it has to be cleared here or
* the next navigation picks it up instead.
*
* A delta is only staged for a history navigation, and a stale one
* makes the next navigation look like traversal, which stops the
* incoming route from being added. Route params are staged by any of
* the navigation helpers, and a stale set carries a pop action into
* whatever runs next. Only handleNavigateBack stages the previous
* route's id alongside them.
*
* Only clear state that belongs to this navigation, and check the two
* slots separately. Another navigation can replace this one and stage
* its own state first, in which case clearing would strip that state
* from the navigation still running.
*
* Params staged without a target fall back to the delta's target.
* Those come from the helpers that hand off to history, so a delta is
* always recorded for them.
*
* This only covers navigations that fail. A guard that returns a
* location redirects rather than fails, so afterEach is never called
* for the original navigation and its staged state reaches the redirect
* target instead.
*/
const deltaIsForThisNavigation =
currentNavigationInfo.to === undefined ||
currentNavigationInfo.to === to.fullPath;

const paramsAreForThisNavigation =

@ShaneK ShaneK Aug 26, 2026

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 only half of this made it in. Stamping happens in setIncomingRouteParams, but changeTab and handleNavigateBack both set incomingRouteParams directly and just leave whatever stamp was already sitting there. And the stamp is only ever set while a handleNavigate is in flight, which is exactly the case this gate exists for, so the navigation that gets cancelled matches its own stale stamp and wipes the params belonging to the one that replaced it.

I hit this with a global async guard, the session-check kind. Tap a link, then tap the Tab 2 button before the first one finishes, and you get { routerAction: 'push', routerDirection: 'forward', tab: undefined } where main gives { routerAction: 'push', routerDirection: 'none', tab: 'tab2' }. So the tab slides in like a forward push instead of swapping, and since tab is gone it takes the isPushed branch and picks up pushedByRoute from the tab you just left, which means the back button over on tab 2 sends you into tab 1's stack.

It's wider than I expected, too. On a 400ms guard I tried gaps from 10ms all the way up to about 400ms and every one of them did it, main was right on all of them. Makes sense in hindsight, the first navigation starts its guard first so it always reports the cancellation before the second one finishes. It does need a guard though. With plain lazy routes the tab's chunk is already cached by the time changeTab can even take this branch, so the tab always wins and nothing breaks, which is probably why the specs don't catch it.

The handleNavigateBack path looks like the same shape, I didn't manage a clean repro of that one. Both of them know their target though, so changeTab could stamp its pathname and handleNavigateBack could just null the stamp before router.back() so it falls back to the delta the way your comment says. Honestly, making setIncomingRouteParams the only thing that writes the params would save us from this coming back. The incomingRouteParamsTo doc needs a tweak either way, it says the back and forward helpers leave it undefined where those two leave it stale.

@thetaPC thetaPC Aug 26, 2026

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.

150ecb2

incomingRouteParamsTo === undefined
? deltaIsForThisNavigation
: incomingRouteParamsTo === to.fullPath;

if (deltaIsForThisNavigation) {
currentNavigationInfo = {
direction: undefined,
action: undefined,
delta: undefined,
to: undefined,
};
}

if (paramsAreForThisNavigation) {
incomingRouteParams = undefined;

@ShaneK ShaneK Aug 25, 2026

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 gate here was my suggestion, and I missed something when I proposed it. Saving the target path works for the delta, but this line clears incomingRouteParams too, and nothing ever stamps a path onto that. Only history.listen writes currentNavigationInfo.to, and neither setIncomingRouteParams nor changeTab records one, so whenever neither navigation is a history traversal the first disjunct is true and the clear runs unconditionally.

That regresses against main. With a push in flight on a lazy route, a logout ionRouter.replace('/login') cancels it, and the cancelled push wipes the replace's staged params before it gets to use them. The route comes out routerDirection: 'none' with canGoBack() true, where main gives 'root' and false. Without 'root' there's no clearHistory(), so the back button on the login page still walks into the authenticated pages after logout. The tab path loses tab and routerAnimation the same way.

I think stamping the resolved path onto incomingRouteParams, the way history.listen already stamps currentNavigationInfo.to, is the fix that keeps the gate honest for both slots. I haven't tried that one. What I did try is skipping just the params clear on cancelled failures, which gets main's result back with all 20 tests still passing, since 29721's own repro aborts rather than cancels. That leaves an ionRouter.back() cancelled by a push still uncleared though, which is why I'd lean toward the stamp.

@thetaPC thetaPC Aug 25, 2026

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.

Went with the stamp. setIncomingRouteParams now takes an optional target and resolves it, and the gate is two independent checks instead of one answer for both slots. goBack and goForward leave it unset and fall back to the delta's target, which is fine because those are the ones that hand off to history.

One thing I'd like your read on. I kept the target in a separate incomingRouteParamsTo rather than adding a to field to incomingRouteParams. The reason is that the params get spread wholesale onto a RouteInfo at the incomingRouteParams?.id branch, so a field on them would land there too and reach anything reading route info. The cost is two variables to keep in sync, and I had to clear the stamp in all three places the params are cleared. Happy to move it onto the params if you'd rather have one object, since the leak is cosmetic rather than harmful.

67125fe

@ShaneK ShaneK Aug 26, 2026

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.

incomingRouteParamsTo = undefined;
}

return;
}

const { direction, action, delta } = currentNavigationInfo;

Expand All @@ -68,6 +121,7 @@ export const createIonRouter = (
direction: undefined,
action: undefined,
delta: undefined,
to: undefined,
};
}
);
Expand All @@ -89,6 +143,24 @@ export const createIonRouter = (
* Cleared once `handleHistoryChange` has consumed them.
*/
let incomingRouteParams: RouteParams | undefined;
/**
* The location the staged params were meant for. Kept beside the params
* rather than on them so it is never spread onto a RouteInfo. Left undefined
* by the helpers that hand off to history and so cannot know the target yet,
* which is `goBack`, `goForward` and `handleNavigateBack`. Those fall back to
* the delta's target, which history always records for them.
*/
let incomingRouteParamsTo: string | undefined;

/**
* The only place that stages route params, so the recorded location can
* never be left over from an earlier navigation. Pass the target when it is
* known, and omit it to fall back to the delta.
*/
const stageRouteParams = (params: RouteParams, to?: RouteLocationRaw) => {
incomingRouteParams = params;
incomingRouteParamsTo = to ? router.resolve(to).fullPath : undefined;
};

const historyChangeListeners: any[] = [];

Expand All @@ -101,7 +173,7 @@ export const createIonRouter = (
});
}

opts.history.listen((_: any, _x: any, info: any) => {
opts.history.listen((to: any, _x: any, info: any) => {
/**
* history.listen only fires on certain
* event such as when the user clicks the
Expand All @@ -123,6 +195,12 @@ export const createIonRouter = (
*/
action: info.type === "pop" && info.delta >= 1 ? "push" : info.type,
direction: info.direction === "" ? "forward" : info.direction,

/**
* Recorded so that a failed navigation can tell whether this
* information is its own before clearing it.
*/
to,
};
});

Expand All @@ -142,12 +220,12 @@ export const createIonRouter = (
if (routeInfo && routeInfo.pushedByRoute) {
const prevInfo = locationHistory.findLastLocation(routeInfo);
if (prevInfo) {
incomingRouteParams = {
stageRouteParams({
...prevInfo,
routerAction: "pop",
routerDirection: "back",
routerAnimation: routerAnimation || routeInfo.routerAnimation,
};
});
if (
routeInfo.lastPathname === routeInfo.pushedByRoute ||
/**
Expand Down Expand Up @@ -223,6 +301,7 @@ export const createIonRouter = (
* letting them leak into the next navigation.
*/
incomingRouteParams = undefined;
incomingRouteParamsTo = undefined;
}
}
} else if (defaultHref) {
Expand All @@ -240,7 +319,13 @@ export const createIonRouter = (
routerAnimation?: AnimationBuilder,
tab?: string
) => {
setIncomingRouteParams(routerAction, routerDirection, routerAnimation, tab);
setIncomingRouteParams(
routerAction,
routerDirection,
routerAnimation,
tab,
path
);

if (routerAction === "push") {
router.push(path);
Expand Down Expand Up @@ -586,6 +671,7 @@ export const createIonRouter = (
currentRouteInfo = routeInfo;
}
incomingRouteParams = undefined;
incomingRouteParamsTo = undefined;
historyChangeListeners.forEach((cb) => cb(currentRouteInfo));
};

Expand All @@ -601,7 +687,13 @@ export const createIonRouter = (
const navigate = (navigationOptions: ExternalNavigationOptions) => {
const { routerAnimation, routerDirection, routerLink } = navigationOptions;

setIncomingRouteParams("push", routerDirection, routerAnimation);
setIncomingRouteParams(
"push",
routerDirection,
routerAnimation,
undefined,
routerLink
);

router.push(routerLink);
};
Expand Down Expand Up @@ -665,13 +757,6 @@ export const createIonRouter = (
const hrefSearch = search ? `?${search}` : "";

if (routeInfo) {
incomingRouteParams = {
...incomingRouteParams,
routerAction: "push",
routerDirection: "none",
tab,
};

/**
* When going back to a tab
* you just left, it's possible
Expand All @@ -684,15 +769,23 @@ export const createIonRouter = (
* are honored when re-selecting the tab.
*/
const effectiveSearch = hrefSearch || routeInfo.search || "";
const push = {
const target = {
path: routeInfo.pathname === pathname ? routeInfo.pathname : pathname,
query: parseQuery(effectiveSearch),
...(hrefHash ? { hash: hrefHash } : {}),
};
if (routeInfo.pathname === pathname) {
router.push({ path: routeInfo.pathname, ...push });
} else {
router.push({ path: pathname, ...push });
}

stageRouteParams(
{
...incomingRouteParams,
routerAction: "push",
routerDirection: "none",
tab,
},
target
);

router.push(target);
} else {
handleNavigate(
pathname + hrefSearch + hrefHash,
Expand Down Expand Up @@ -790,14 +883,18 @@ export const createIonRouter = (
routerAction: RouteAction = "push",
routerDirection: RouteDirection = "forward",
routerAnimation?: AnimationBuilder,
tab?: string
tab?: string,
to?: RouteLocationRaw
) => {
incomingRouteParams = {
routerAction,
routerDirection,
routerAnimation,
tab,
};
stageRouteParams(
{
routerAction,
routerDirection,
routerAnimation,
tab,
},
to
);
};

const goBack = (routerAnimation?: AnimationBuilder) => {
Expand Down
6 changes: 6 additions & 0 deletions packages/vue-router/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,4 +91,10 @@ export interface NavigationInformation {
action?: RouteAction;
direction?: RouteDirection;
delta?: number;
/**
* The location the browser moved to when this information was staged. Used to
* tell whether the information belongs to a particular navigation, since a
* second history navigation can stage its own before the first one settles.
*/
to?: string;
}
Loading
Loading