[PR #423] [MERGED] fix: honor beforePopState when useRouter is mounted #561

New issue

Closed

opened 2026-05-06 13:08:46 +02:00 by BreizhHardware · 0 comments

BreizhHardware commented

2026-05-06 13:08:46 +02:00

Owner

📋 Pull Request Information

Original PR: https://github.com/cloudflare/vinext/pull/423
Author: @JaredStowell
Created: 3/10/2026
Status: ✅ Merged
Merged: 3/10/2026
Merged by: @james-elicx

Base: main ← Head: jstowell/fix-before-pop-state-bypass

📝 Commits (1)

4fd2b11 Update router popstate handling

📊 Changes

6 files changed (+144 additions, -67 deletions)

View changed files

📝 packages/vinext/src/shims/router.ts (+2 -13)
📝 tests/__snapshots__/entry-templates.test.ts.snap (+57 -54)
📝 tests/e2e/pages-router/before-pop-state.spec.ts (+46 -0)
➕ tests/fixtures/pages-basic/pages/before-pop-state-destination.tsx (+16 -0)
📝 tests/fixtures/pages-basic/pages/before-pop-state-test.tsx (+3 -0)
📝 tests/shims.test.ts (+20 -0)

📄 Description

Fix a Pages Router bug where router.beforePopState() could be bypassed whenever the currently mounted page called useRouter().

Previously, the next/router shim had two separate popstate listeners:

A module-level listener that checked _beforePopStateCb
A useRouter()-scoped listener that always called navigateClient()

That meant a blocked back/forward navigation could still proceed through the hook-local listener even if the module-level listener returned early.

This change removes the duplicate useRouter() popstate listener and keeps browser back/forward handling owned by the module-level listener only. useRouter() still updates via the existing vinext:navigate event after successful navigations.

Problem

The bug was in packages/vinext/src/shims/router.ts.

Before this change:

beforePopState(cb) stored the callback in _beforePopStateCb
The module-level popstate handler checked _beforePopStateCb and returned if it got false
But useRouter() also registered its own popstate listener
That hook-local listener always called navigateClient(window.location.pathname + window.location.search)

So when a page mounted useRouter(), a blocked popstate could still navigate anyway.

In practice, the existing e2e test did not catch this because it navigated from /before-pop-state-test to /about, and /about does not mount useRouter(). By the time page.goBack() fired, the extra hook-level listener was no longer present.

Fix

Make popstate handling single-owner.

Changes:

Remove the useRouter()-local popstate useEffect
Keep all browser back/forward navigation in the module-level popstate listener
Continue syncing useRouter() state from the existing vinext:navigate event

This matches Next.js more closely. In Next.js, the router owns a single onPopState handler and checks beforePopState() there; it does not install an additional hook-level popstate listener.

Tests

Added regression coverage for the real failing case.

Fixture changes

Added tests/fixtures/pages-basic/pages/before-pop-state-destination.tsx
- This page mounts useRouter() and exposes its current asPath
Updated tests/fixtures/pages-basic/pages/before-pop-state-test.tsx
- Added a link to the new destination page

E2E coverage

Expanded tests/e2e/pages-router/before-pop-state.spec.ts to cover:

blocking callback prevents route change on back navigation
blocking callback still prevents route change when destination mounts useRouter
allowing callback permits back navigation
allowing callback permits back navigation when destination mounts useRouter

This closes the coverage gap that allowed the bug to slip through.

Unit/snapshot coverage

Added a small shim-surface assertion in tests/shims.test.ts
Updated tests/__snapshots__/entry-templates.test.ts.snap for the new Pages Router fixture route

Verification

pnpm test tests/shims.test.ts -t "beforePopState on both"
pnpm test tests/entry-templates.test.ts -u
PLAYWRIGHT_PROJECT=pages-router pnpm run test:e2e tests/e2e/pages-router/before-pop-state.spec.ts
PLAYWRIGHT_PROJECT=cloudflare-pages-router pnpm run test:e2e tests/e2e/cloudflare-pages-router/interactive.spec.ts

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/cloudflare/vinext/pull/423 **Author:** [@JaredStowell](https://github.com/JaredStowell) **Created:** 3/10/2026 **Status:** ✅ Merged **Merged:** 3/10/2026 **Merged by:** [@james-elicx](https://github.com/james-elicx) **Base:** `main` ← **Head:** `jstowell/fix-before-pop-state-bypass` --- ### 📝 Commits (1) - [`4fd2b11`](https://github.com/cloudflare/vinext/commit/4fd2b11e1b79fb4aa56ed6393da2cee9f91755ac) Update router popstate handling ### 📊 Changes **6 files changed** (+144 additions, -67 deletions) <details> <summary>View changed files</summary> 📝 `packages/vinext/src/shims/router.ts` (+2 -13) 📝 `tests/__snapshots__/entry-templates.test.ts.snap` (+57 -54) 📝 `tests/e2e/pages-router/before-pop-state.spec.ts` (+46 -0) ➕ `tests/fixtures/pages-basic/pages/before-pop-state-destination.tsx` (+16 -0) 📝 `tests/fixtures/pages-basic/pages/before-pop-state-test.tsx` (+3 -0) 📝 `tests/shims.test.ts` (+20 -0) </details> ### 📄 Description Fix a Pages Router bug where `router.beforePopState()` could be bypassed whenever the currently mounted page called `useRouter()`. Previously, the `next/router` shim had two separate `popstate` listeners: - A module-level listener that checked `_beforePopStateCb` - A `useRouter()`-scoped listener that always called `navigateClient()` That meant a blocked back/forward navigation could still proceed through the hook-local listener even if the module-level listener returned early. This change removes the duplicate `useRouter()` `popstate` listener and keeps browser back/forward handling owned by the module-level listener only. `useRouter()` still updates via the existing `vinext:navigate` event after successful navigations. ## Problem The bug was in `packages/vinext/src/shims/router.ts`. Before this change: - `beforePopState(cb)` stored the callback in `_beforePopStateCb` - The module-level `popstate` handler checked `_beforePopStateCb` and returned if it got `false` - But `useRouter()` also registered its own `popstate` listener - That hook-local listener always called `navigateClient(window.location.pathname + window.location.search)` So when a page mounted `useRouter()`, a blocked `popstate` could still navigate anyway. In practice, the existing e2e test did not catch this because it navigated from `/before-pop-state-test` to `/about`, and `/about` does not mount `useRouter()`. By the time `page.goBack()` fired, the extra hook-level listener was no longer present. ## Fix Make `popstate` handling single-owner. Changes: - Remove the `useRouter()`-local `popstate` `useEffect` - Keep all browser back/forward navigation in the module-level `popstate` listener - Continue syncing `useRouter()` state from the existing `vinext:navigate` event This matches Next.js more closely. In Next.js, the router owns a single `onPopState` handler and checks `beforePopState()` there; it does not install an additional hook-level `popstate` listener. ## Tests Added regression coverage for the real failing case. ### Fixture changes - Added `tests/fixtures/pages-basic/pages/before-pop-state-destination.tsx` - This page mounts `useRouter()` and exposes its current `asPath` - Updated `tests/fixtures/pages-basic/pages/before-pop-state-test.tsx` - Added a link to the new destination page ### E2E coverage Expanded `tests/e2e/pages-router/before-pop-state.spec.ts` to cover: - blocking callback prevents route change on back navigation - blocking callback still prevents route change when destination mounts `useRouter` - allowing callback permits back navigation - allowing callback permits back navigation when destination mounts `useRouter` This closes the coverage gap that allowed the bug to slip through. ### Unit/snapshot coverage - Added a small shim-surface assertion in `tests/shims.test.ts` - Updated `tests/__snapshots__/entry-templates.test.ts.snap` for the new Pages Router fixture route ## Verification - `pnpm test tests/shims.test.ts -t "beforePopState on both"` - `pnpm test tests/entry-templates.test.ts -u` - `PLAYWRIGHT_PROJECT=pages-router pnpm run test:e2e tests/e2e/pages-router/before-pop-state.spec.ts` - `PLAYWRIGHT_PROJECT=cloudflare-pages-router pnpm run test:e2e tests/e2e/cloudflare-pages-router/interactive.spec.ts` --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>