Skip to content

fix: don't commit a navigation superseded while onNavigate is pending - #17200

Open
khughitt wants to merge 8 commits into
sveltejs:version-3from
khughitt:fix-superseded-onnavigate-commit
Open

khughitt wants to merge 8 commits into
sveltejs:version-3from
khughitt:fix-superseded-onnavigate-commit

Conversation

@khughitt

@khughitt khughitt commented Sep 24, 2026 •

Copy link
Copy Markdown

navigate() checks navigation_token once its route has loaded, then awaits onNavigate callbacks and commits without checking again. When a callback returns a promise (the view transition recipe from the docs holds the navigation until the transition's update callback runs), a newer navigation can start during that wait, and the superseded one still sets current and applies its render tree when its callbacks settle. This adds the same token check after the onNavigate callbacks as the one after load.

With experimental.forkPreloads, navigate() has already taken the preload fork out of load_cache at that point, so nothing else can reach it. The new abort path therefore discards the fork itself, the same way discard_load_cache() does.

Functions returned by onNavigate must also not outlive an aborted navigation, or they run when the next navigation completes. run_on_navigate_callbacks() used to register them before the check. It now returns them, and the caller registers them only if the navigation is still current. finish_navigation() also receives them and removes them when it aborts, which covers a navigation superseded while its render settles. They stay registered before the commit, so they still run in the same order relative to other afterNavigate callbacks. Each registration wraps the returned functions in entries of its own. after_navigate_callbacks is a Set, so if two navigations' onNavigate calls returned the same function object, removing the aborted navigation's registration would otherwise remove the newer navigation's too.

A shallow goto() has the same gap in update_state(): superseded while onNavigate was pending, it still applied its page state and registered its functions. It now gets the same check. As with a full navigation aborted at that point, its history entry has already been pushed and stays; the check only stops the page update and the registration.

On version-3 the newer navigation still commits afterwards, so the result is transient: the superseded page mounts, renders and runs its effects, and its onNavigate return value is registered as an afterNavigate callback, just before the newest navigation replaces it.

On 2.x (2.70.3, main) the same gap leaves the wrong page in place. There, a navigation only sends the data_N props whose node data differs from current at load time. If a superseded navigation commits between a newer navigation's load and its commit, the newer navigation's props are diffed against a stale current: it commits without data_N for a page whose data didn't change relative to that current. The URL then shows the newest route while the page keeps the superseded route's data. We hit this with rapid link clicks and Back under view transitions. I'm happy to open a backport against main if that's wanted.

No existing issue covers this. #12809 involved a pending onNavigate too, but that was a different symptom.

Test

navigation-lifecycle/on-navigate-superseded/[id] holds every navigation in onNavigate. The test starts a navigation to b, then a newer one back to a, releases the superseded one first, and asserts that b never renders and that only the committed navigation's onNavigate return value runs. It fails on version-3 without the change (Received: "b"), fails without the registration change (the aborted navigation's function runs), and passes 20/20 in dev and build with both. A second case runs the same sequence with a shallow goto() as the superseded navigation. Without the change, the superseded navigation's state is applied (Received: "active").

In async, which enables forkPreloads, fork/superseded preloads a page that subscribes to a store counting its subscribers. The navigation to that page is held in onNavigate and superseded, and the test asserts that releasing it drops the count back to 0, meaning the fork was discarded. Without the discard it fails (Received: 1), and it passes 20/20 in dev and build with it.

Also in async, navigation-settle holds a page's render with a top-level await, and a newer navigation starts before it settles. The test asserts that only the newer navigation's onNavigate return value runs. Without removing them on that abort it fails (the held navigation's function runs), and it passes 20/20 in dev and build with it. The page also returns one shared function object from every navigation, and the test asserts that the newer navigation still runs it. Without the per-registration entries, the aborted navigation's cleanup removes it.


Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Ran pnpm format, pnpm lint, pnpm -F @sveltejs/kit test:unit, and pnpm check in packages/kit, basics and async (all clean), plus the basics, options and async Playwright suites on Chromium in dev and build. options, async and basics build pass. In basics dev, a few Load and SPA mode tests fail under the full suite's load, and they did so before this change too. They pass when rerun on their own. I didn't run the full pnpm test.

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

@pkg-svelte-dev

pkg-svelte-dev Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Install the latest version of @sveltejs/kit from da3cc3d:

pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/da3cc3dd97cccaa76aabec54cf66c65d964059e0

Open in pkg.svelte.dev: https://pkg.svelte.dev/repos/kit/pr/17200

Note

This PR is from a fork. A maintainer must approve approve each commit before it can be built and installed.

@changeset-bot

changeset-bot Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: da3cc3d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@khughitt
khughitt marked this pull request as ready for review September 24, 2026 14:44
// `load_cache` no longer holds this fork, so nothing else will discard it
void load_cache_fork?.then((f) => f?.discard());
nav.reject(new Error('navigation aborted'));
return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

run_on_navigate_callbacks registers functions returned from onNavigate before this check, and this return leaves them in after_navigate_callbacks. A function returned for an aborted navigation runs when the next navigation commits, although its page never rendered.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Confirmed, the aborted navigation's returned function ran when the next one completed. run_on_navigate_callbacks() now returns those functions instead of registering them, and navigate() registers them after the token check, so an aborted navigation never registers its function. The shallow-routing caller registers them immediately, as before. The basics test now also asserts that only the committed navigation's function runs. It failed without this change, and passes with it.

This branch has not been deployed

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants