fix(app-router): guard stale popstate scroll restoration

[NathanDrake2406]

Summary

• Fixes an App Router popstate race where stale traversal completions could restore scroll after a newer traversal had already won.
• Extracts popstate restoration coordination into a small helper so ownership and cleanup semantics are testable.
• Stabilizes the same-build RSC replay E2E by waiting for the internal navigation promise that seeds the visited-response cache.

Details

• app-browser-entry.ts now delegates App Router popstate handling to createPopstateRestoreHandler.
• The helper restores popstate scroll only when the captured navigation id is still current.
• Pending navigation cleanup now checks ownership before clearing __VINEXT_RSC_PENDING__.
• The build-id navigation E2E no longer treats visible text commit as proof that cache seeding has completed.

Next.js parity notes

Next.js routes App Router popstate transitions through the queued navigation action path, where newer traverses can replace/discard stale work:

• https://github.com/vercel/next.js/blob/canary/packages/next/src/client/components/app-router.tsx#L390-L399
• https://github.com/vercel/next.js/blob/canary/packages/next/src/client/components/app-router-instance.ts#L195-L207

Validation

• vp check packages/vinext/src/server/app-browser-entry.ts packages/vinext/src/server/app-browser-popstate.ts tests/app-browser-entry.test.ts tests/e2e/app-router/build-id-navigation.spec.ts
• vp test run tests/app-browser-entry.test.ts
• PLAYWRIGHT_PROJECT=app-router npx playwright test tests/e2e/app-router/build-id-navigation.spec.ts --project app-router --repeat-each=10
• PLAYWRIGHT_PROJECT=app-router npx playwright test --project app-router

Note: the local commit hook's full formatting check still trips on intentionally invalid upstream fixtures under .refs/astro and .refs/sveltekit; the touched-file vp check is clean.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@1253

commit: b7289c1

[james-elicx]

/bigbonk review

[ask-bonk]

Clean extraction and correct fix. The stale-popstate scroll race is a real issue (Next.js handles it by routing popstate through the queued action reducer, which discards stale work), and the nav-id guard is the right equivalent for vinext's architecture.

The helper is well-factored: deps are explicit and testable, the unit tests cover both the "stale scroll suppression" and "stale pending cleanup" cases, and the E2E stabilization correctly addresses the cache-seeding timing gap.

One minor ordering observation below, but it's not blocking.

[ask-bonk]

Nit: The correctness of this line depends on navigate?.() calling beginNavigation() synchronously before returning the promise, so that getActiveNavigationId() on the next line reads the id that this popstate allocated. That's true today (navigateRsc is async and beginNavigation() is the first synchronous statement), but it's a subtle ordering contract between two files.

Consider adding a brief comment here noting the invariant, e.g.:

Suggested change

	const popstateNavId = deps.getActiveNavigationId();
	// navigate() calls beginNavigation() synchronously before its first await,
	// so getActiveNavigationId() reads the id allocated by *this* popstate.
	const popstateNavId = deps.getActiveNavigationId();

This would help future readers understand why getActiveNavigationId is called after navigate rather than before.

[ask-bonk]

Good stabilization. The waitForFunction + evaluate(await ...) two-step handles the case where the marker hasn't been set yet (first navigation hasn't started) and then the case where the promise hasn't settled yet (navigation in progress). Clean.

[ask-bonk]

The getNavigate mock mutates activeNavigationId to simulate beginNavigation() being called synchronously inside navigate. This is the right approach — it preserves the same ordering contract as production code where navigateRsc increments the active nav id synchronously before returning the promise. Well done making this explicit in the test harness.

[ask-bonk]

Review submitted. Here's a summary of my findings:

Approved -- the fix is correct and well-tested.

What the PR does:

1. Extracts the popstate scroll-restoration handler into createPopstateRestoreHandler in a new app-browser-popstate.ts module, making it independently testable.
2. Fixes a race where rapid back/forward popstate events could cause a stale traversal's .finally() to restore scroll position after a newer traversal had already won. The fix captures the active navigation ID at popstate time and only restores scroll if that ID is still current when the navigation settles.
3. Stabilizes the build-id navigation E2E test by properly waiting for the RSC navigation promise to settle (which seeds the visited-response cache) rather than assuming h1 text visibility implies cache readiness.

Key observations:

• The nav-id guard correctly mirrors how Next.js discards stale popstate work through its queued action reducer.
• There's a subtle ordering invariant: getActiveNavigationId() must be called after navigate() because navigateRsc calls beginNavigation() synchronously before its first await. I suggested a comment to document this.
• The __VINEXT_RSC_PENDING__ ownership check (getPendingNavigation() === pendingNavigation) correctly allows stale navigations to clean up the pending slot when they still own it, while preventing them from clobbering a newer navigation's promise.
• Tests cover both the "only latest popstate restores scroll" and "stale popstate still cleans up pending" scenarios.

github run