Skip to content

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

Open
khughitt wants to merge 3 commits into
sveltejs:mainfrom
khughitt:fix-superseded-onnavigate-main
Open

khughitt wants to merge 3 commits into
sveltejs:mainfrom
khughitt:fix-superseded-onnavigate-main

Conversation

@khughitt

@khughitt khughitt commented Oct 1, 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. A newer navigation can finish before that abort runs (a shallow goto() doesn't wait for the older render), so a navigation that finishes first removes any other navigation's registrations before it runs afterNavigate callbacks. Only navigations it superseded can have any, since registering requires the navigation to still be current. 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 3.x 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 (version-2) 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 version-2 if that's wanted.

This replaces #17200, which targeted version-3 and was closed when that branch merged into main. The change is the same, applied to main. 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 main 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. A second case supersedes the held navigation with a shallow goto(), which finishes before the held render settles. Without removing other navigations' registrations before dispatch, the held navigation's function runs and shared runs twice.

On main, all five new tests fail without the change, and each passes 10/10 in dev and build with it (the original four tests also passed 20/20 each on version-3 before).


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, SPA mode, data-sveltekit-preload-code and INP 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 Oct 1, 2026 •

Copy link
Copy Markdown

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

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

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

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 Oct 1, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: a83ab3a

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 October 2, 2026 10:14

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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant