Skip to content

test: wait for popstate state updates - #16954

Closed
svelte-triage-bot[bot] wants to merge 1 commit into
version-3from
flake/kit-pr-16878-await-popstate-state
Closed

test: wait for popstate state updates#16954
svelte-triage-bot[bot] wants to merge 1 commit into
version-3from
flake/kit-pr-16878-await-popstate-state

Conversation

@svelte-triage-bot

Copy link
Copy Markdown
Contributor

Fixes a pre-existing browser-test flake observed on #16878 in https://github.com/sveltejs/kit/actions/runs/33085846048.

The test previously continued once the browser URL and unchanged scroll/focus values matched. Those conditions can become true before SvelteKit's asynchronous popstate handler accepts the new history index. Under contention, the subsequent navigation could therefore reuse a stale index and restore the wrong scroll entry.

Render the effective search state for this test route and await it after each history traversal so the test only continues once $app/state reflects the popstate.

Verification:

  • server-resolution dev: 100 repeats, 4 workers
  • server-resolution build: 30 repeats, 4 workers
  • client-resolution dev: 30 repeats, 4 workers
  • adjacent shallow-routing test: passed
  • pnpm -F test-basics check
  • focused formatting check

@svelte-triage-bot svelte-triage-bot Bot mentioned this pull request Aug 27, 2026
5 tasks
@pkg-svelte-dev

Copy link
Copy Markdown

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

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

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

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 Aug 27, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: d3a37f8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@Nic-Polumeyv

Copy link
Copy Markdown
Contributor

@svelte-triage-bot. Seems like you're working around the issue. Review proper documentation on modern playwright to understand how to programatically fix this. Consider I might be absolutely incorrect, but this feels like it can have more correctness and "taste"

@svelte-triage-bot

Copy link
Copy Markdown
Contributor Author

Thanks for raising it — I reviewed the current Playwright guidance.

The existing change is already using a web-first locator assertion (await expect(search).toHaveText(...)), which retries until the asserted DOM state is true. That is preferable to sleeps or manual polling.

page.waitForURL() / expect(page).toHaveURL() cannot fix this particular race: the browser updates location as part of the history traversal before SvelteKit’s asynchronous popstate handling updates $app/state. Waiting for the URL therefore reaches the same premature point as the failing test. Playwright explicitly notes that modern applications have no framework-independent “loaded” state; tests need to wait for the application condition that makes the next interaction valid.

The added hidden value is a test-fixture observation of precisely that condition: $app/state has accepted the traversed history entry. It is not a production API or a timing workaround.

That said, the concern about taste is valid. A cleaner version could expose this through an already-purposeful test fixture output (rather than introducing a standalone hidden <span>), but it would still need to render or otherwise expose the reactive $app/state value. Replacing it with waitForURL, goBack({ waitUntil: ... }), or a Playwright navigation event would weaken—not improve—the synchronization.

Rich-Harris pushed a commit that referenced this pull request Aug 27, 2026
Replaces #16954.

The `popstate` handler in `client.js` awaits route resolution
(`get_navigation_intent`, and again inside `navigate` before `accept`)
before recording that the browser has moved. Under server-side route
resolution that is a network round-trip, during which
`current_history_index` still points at the entry we left. A popstate
arriving in that window is swallowed by the cancellation guard, and a
`goto` pushes an entry with a duplicate `historyIndex`, so later scroll
and snapshot restores read the wrong entry. This is the flake in
https://github.com/sveltejs/kit/actions/runs/33085846048.

Record the traversal synchronously: capture the source entry's scroll
and snapshots and move the indices before anything async, restore them
in `block()` before `history.go(-delta)`, and capture the navigation
token up front so a superseded popstate stops instead of taking over the
newer navigation's token. `navigate` skips its own capture for popped
navigations.

Repro: a 120 ms delay after the `get_navigation_intent` await in the
handler fails `Preserves scroll and focus across popstate...` 4/4 in
`test:server-side-route-resolution:dev` before this change and passes
4/4 after it.
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