Solid PR — the split into navigateClientData / navigateClientHtml with a shared orchestrator in navigateClient is clean, the round-trip tests between buildPagesDataPath and parseNextDataPathname are exactly the right thing to do, and the parseNextDataPathname bug fix for index/-prefixed paths is a good catch.

A few things I noticed:

query on __NEXT_DATA__ is missing search params after data-path navigation

In navigateClientData, the synthesised __NEXT_DATA__ sets:

query: { ...target.params },

This only includes dynamic route params. After navigating to /about?foo=bar, window.__NEXT_DATA__.query will be {} instead of { foo: "bar" }.

The user-facing useRouter().query happens to still work because getPathnameAndQuery() independently reads window.location.search and merges. But any library or user code that reads window.__NEXT_DATA__.query directly (which is a public API surface in Next.js) will see stale/incomplete data. Next.js' __NEXT_DATA__.query always contains both dynamic params and search params merged.

target.search is already available on the resolved target — just needs to be parsed and spread into query alongside the params. Something like:

const searchQuery: Record<string, string | string[]> = {};
const searchParams = new URLSearchParams(target.search);
for (const [key, value] of searchParams) {
  const existing = searchQuery[key];
  if (existing !== undefined) {
    searchQuery[key] = Array.isArray(existing) ? [...existing, value] : [existing, value];
  } else {
    searchQuery[key] = value;
  }
}
// ...
query: { ...searchQuery, ...target.params },

Dynamic params should win over search params (spread order), matching Next.js.

gSSP redirect returns HTTP 3xx, not soft-redirect — x-nextjs-redirect check is dead code

navigateClientData checks for x-nextjs-redirect header (the soft-redirect protocol), but the server side (pages-page-data.ts resolvePagesPageData) returns raw HTTP 307/308 with a Location header for gSSP/gSP redirects, even on /_next/data requests. The browser's fetch() transparently follows 3xx redirects, so:

1. The x-nextjs-redirect check at line ~673 never fires for server-originated redirects.
2. After the browser follows the 3xx, the response the client sees is whatever lives at the redirect target — likely an HTML page, not a JSON payload — which will fail the res.json() parse and trigger a hard reload. So the end result is correct (user lands on the right page), but the error path is noisy/indirect.

Next.js handles this by returning 200 with x-nextjs-redirect header on data requests instead of a raw 3xx. This is documented as the "soft redirect" protocol. Not a blocker for this PR since the current behavior is functionally correct (fails to JSON, hard-reloads), but worth tracking as a follow-up to make the server emit soft-redirect responses for data requests specifically.

__VINEXT_PAGE_PATTERNS__ ordering

The comment in pages-client-entry.ts says the patterns list is exposed "so the router can sort it once (longest match first) without re-keying the loader map." But Object.keys(pageLoaders) returns insertion order from the generated code, which is whatever order pageRoutes.map(...) produces. I don't see any sorting happening — neither in the entry generator nor in matchPagesPattern (which explicitly says "Patterns are tried in patterns order — callers should pre-sort").

Is the route scanner already returning routes in priority order? If so the comment is misleading (it's not about the router sorting, it's about the scanner having already sorted). If not, there's a latent priority bug for apps with overlapping patterns like /posts/[id] and /posts/[...rest].

Minor nits

The navigateClientData and navigateClientHtml functions both have their own if (!root) guard + fallback. Since both are only called from navigateClient which already checks typeof window !== "undefined", the root check could be hoisted into navigateClient before the try block — would remove 8 lines of duplication. Not a big deal though.

Overall this is well-structured and the test coverage is thorough. The __NEXT_DATA__.query search-params gap is the main thing I'd want addressed before merge.

Verified the fixes in 9029611:

__NEXT_DATA__.query missing search params — Fix is correct. mergeRouteParamsIntoQuery(parseQueryString(target.search), target.params) properly parses the search string into a multi-value-aware record, then spreads route params on top so they win on collision. The utility functions in utils/query.ts are clean and well-factored. The regression test covers the key case: /blog/my-post?slug=spoofed&q=hello&tag=a&tag=b correctly produces { slug: "my-post", q: "hello", tag: ["a", "b"] } — route param slug beats the spoofed search param, single-value and multi-value search params both work.

__VINEXT_PAGE_PATTERNS__ ordering — Confirmed: pagesRouter() calls routes.sort(compareRoutes) at lines 85 and 234 of routing/pages-router.ts, so insertion order in the generated pageLoaders is already sorted by specificity. The updated comment now accurately describes this ("which pagesRouter() has already sorted by specificity … via compareRoutes").

x-nextjs-redirect dead code and !root duplication — fine as follow-ups / leave-as-is respectively.

LGTM.

Clean, well-structured PR. The split into navigateClientData / navigateClientHtml with a shared orchestrator in navigateClient reads clearly, the round-trip test suite between buildPagesDataPath and parseNextDataPathname is exactly the right thing for a wire-format contract, and the index/-prefix parsing fix in pages-data-route.ts:83 is a good catch.

E2E regression guard (5529ccc)

The new Playwright test at tests/e2e/cloudflare-pages-router/navigation.spec.ts:61 is the right shape: sniff the network for /_next/data/<buildId>/ssr.json requests and assert zero document-type requests to /ssr. This catches the exact failure mode where the loader manifest silently regresses and navigation falls back to HTML — something every other existing test would miss since they only assert on URL change + rendered content.

One thing worth noting: the test waits for __VINEXT_HYDRATED_AT (line 64) which is set at the end of hydrate() in the client entry (line 145 of pages-client-entry.ts), after __VINEXT_PAGE_LOADERS__ is already assigned (line 90). So the hydration gate is correct — by the time the test proceeds, the loader manifest is guaranteed to be populated.

Items verified from prior review rounds

• Search params merge (9029611): mergeRouteParamsIntoQuery(parseQueryString(target.search), target.params) at router.ts:766 correctly parses multi-value search params via addQueryParam and spreads route params on top. The regression test for /blog/my-post?slug=spoofed&q=hello&tag=a&tag=b covers the key collision case.

• Pattern ordering comment updated in pages-client-entry.ts:87-89 now correctly attributes the sort to pagesRouter()'s compareRoutes at build time.

Minor observations (no action needed)

1. parseQueryString accepts a full URL, not just search string — parseQueryString(target.search) at router.ts:766 passes a string like ?foo=bar, and the function splits on ? to extract the query portion. This works but is slightly redundant since target.search already starts with ?. Not a bug — URLSearchParams("foo=bar") and URLSearchParams("?foo=bar".split("?")[1]) produce the same result — just noting the indirection.

2. Prefetch duplication between link.tsx and router.ts — link.tsx:259-281 and router.ts:1209-1231 both independently resolve the data target and build prefetch links. The code is structurally identical. Not a blocker (the link.tsx path runs during component render and the router.ts path runs from Router.prefetch()), but if the prefetch shape ever changes, both sites need updating.

LGTM. The test coverage is thorough across unit and E2E, the failure modes all funnel to hard-reload correctly, and the addressed review items check out.

Thorough review of the full PR after the latest rebase and the move of _next/data normalization into the generated server entry.

Architecture

The split into navigateClientData / navigateClientHtml with a shared orchestrator in navigateClient is well-structured. The resolvePagesDataNavigationTarget function cleanly encapsulates the "can we use the fast path?" decision, and returning null to fall through to HTML is the right escape hatch. The __normalizePagesDataRequest helper in the generated server entry correctly centralizes data URL normalization so user-written worker entries stay thin — verified that all three server contexts (prod-server, dev-server, Cloudflare worker/deploy.ts) handle _next/data requests correctly with no parity gap.

The round-trip test contract between buildPagesDataPath (client) and parseNextDataPathname (server) is exactly the right thing to do for a wire format, and it caught the index/-prefix bug in pages-data-route.ts:83.

The E2E regression guard (navigation.spec.ts) is well-designed — the sentinel + network sniff approach catches the exact failure mode where the loader manifest silently regresses and the client falls through to HTML without any visible breakage.

Issues found

1. gSSP/gSP redirect on data requests returns raw 3xx instead of JSON body (follow-up, not blocking)

resolvePagesPageData returns a raw HTTP 307/308 with Location header for gSSP/gSP redirects, even on _next/data requests. The browser's fetch() transparently follows the 3xx per the Fetch spec, so the client never sees the redirect — it gets whatever the redirect destination serves (likely HTML), the res.json() parse fails, and the client hard-reloads. The end result is correct but indirect and noisy.

Next.js handles this differently: on data requests, it returns HTTP 200 with { __N_REDIRECT: "/destination", __N_REDIRECT_STATUS: 307 } in the JSON body. The client router interprets those fields and performs a client-side navigation to the redirect target, avoiding a full page reload.

The x-nextjs-redirect check at router.ts:684 is only reachable for middleware redirects (which do emit the header). For gSSP/gSP redirects, it's dead code on the current server implementation.

Already called out as out-of-scope in the PR description and prior reviews. Noting for completeness — the server-side fix (emit 200 + JSON body for data-request redirects) is the real fix and belongs in a separate PR.

2. Locale transitions silently skip the data path (latent, not blocking)

resolvePagesDataNavigationTarget calls matchPagesPattern(pagePath, patterns) where pagePath still includes the locale prefix (e.g., /fr/about). Route patterns in __VINEXT_PAGE_PATTERNS__ are locale-unaware (/about), so the match always fails for non-default-locale URLs. resolvePagesDataNavigationTarget returns null and the code correctly falls through to the HTML path.

This means locale transitions (and all non-default-locale navigations) never benefit from the JSON data path — they always take the slower HTML round trip. Not a correctness bug (the fallback works), but a performance gap worth knowing about.

If this is ever fixed (stripping locale prefix before pattern matching), navigateClientData also needs to be updated: it currently inherits locale fields from ...prev (the previous __NEXT_DATA__), which would be stale after a locale transition. The data JSON response only contains { pageProps } with no locale metadata.

3. Prefetch duplication between link.tsx and router.ts (nit)

The prefetch logic in link.tsx:309-360 and router.ts:1234-1267 is structurally identical — both resolve the data target, inject <link rel="prefetch" as="fetch">, and invoke the loader thunk. If the prefetch shape changes (e.g., adding Accept header hints, or switching from <link> to fetch() with keepalive), both sites need updating. Not blocking, but worth extracting into a shared helper at some point.

Verified from prior rounds

• Search params merge (9029611): mergeRouteParamsIntoQuery(parseQueryString(target.search), target.params) correctly merges search params with route params winning on collision. Regression test covers the key case.
• Pattern ordering comment updated to reference build-time sort via compareRoutes.
• Server parity: __normalizePagesDataRequest in the generated entry handles the Cloudflare Workers path (deploy.ts) correctly. prod-server.ts does its own normalization before calling the entry. dev-server.ts (index.ts) does its own normalization before the SSR handler. No gaps.
• E2E guard (5529ccc): hydration gate via __VINEXT_HYDRATED_AT is correctly ordered after __VINEXT_PAGE_LOADERS__ assignment. Window sentinel approach is more robust than resource-type sniffing.

LGTM — the items above are tracked follow-ups or latent issues, not blockers. The core navigation/prefetch/normalization logic is correct and well-tested.

Reviewed the refactor in 0a2760a. All three items check out.

Prefetch deduplication

Clean extraction. resolvePagesDataNavigationTarget + prefetchPagesData in shims/internal/pages-data-target.ts is the right factoring — the module lives under shims/internal/ so neither link.tsx nor router.ts pulls in the other at init time (no circular imports, no SSR side effects from the Router singleton). Both the Link prefetch path (line 324 of link.tsx) and the Router prefetch/navigation path (lines 930 and 1193 of router.ts) now go through the same resolver, so there is exactly one decision site for "can we use the JSON fast path?".

The link.tsx diff is a clean 40-line deletion with 6 lines replacing it — the inline try/catch + URL parse + stripBasePath + matchPagesPattern + buildPagesDataHref + createElement sequence is gone, replaced by two function calls. Good.

Locale transitions

The locale-stripping logic at pages-data-target.ts:84-87 is correct:

const locale = getLocalePathPrefix(pagePath, window.__VINEXT_LOCALES__);
const pathForMatch = locale ? pagePath.slice(locale.length + 1) || "/" : pagePath;

• getLocalePathPrefix returns the matched locale string (e.g. "fr") or undefined. The + 1 accounts for the leading / in the /<locale> segment. The || "/" handles the edge case where the URL is just /<locale> (e.g. /fr) which should resolve to the root page.
• pagePath (with locale prefix intact) is passed through to buildPagesDataHref, so the data URL keeps the prefix (e.g. /_next/data/<id>/fr/about.json), which is what the server expects for locale-specific gSSP data. Correct.

The locale refresh in navigateClientData (router.ts:729-732) handles both directions:

const hasI18n = (window.__VINEXT_LOCALES__?.length ?? 0) > 0;
const nextLocale = hasI18n
  ? (target.locale ?? window.__VINEXT_DEFAULT_LOCALE__)
  : (prev as VinextNextData | undefined)?.locale;

• Locale transition (/en/about → /fr/about): target.locale is "fr", so nextLocale is "fr". The spread ...(nextLocale !== undefined ? { locale: nextLocale } : {}) overrides prev.locale.
• Back to default (/fr/about → /about): target.locale is undefined (no prefix on an unprefixed URL), so nextLocale falls back to window.__VINEXT_DEFAULT_LOCALE__ (e.g. "en"). Correct — without this, locale would stay stale at "fr".
• No i18n configured: hasI18n is false, so nextLocale inherits from prev.locale unchanged. Also correct — no locale to refresh.

Then applyVinextLocaleGlobals(window, nextData) writes the new locale to window.__VINEXT_LOCALE__, so useRouter().locale (which reads from that global via the Router.locale getter at line 1497) reflects the transition immediately.

Tests

Both regression tests are well-structured:

1. Locale-prefixed nav (strips the locale prefix): navigates to /fr/about, asserts the fetch URL is /_next/data/test-build/fr/about.json (locale prefix preserved in data URL), the matched page is /about (locale stripped for pattern match), and both __NEXT_DATA__.locale and window.__VINEXT_LOCALE__ flip to "fr".

2. Default-locale restore (restores the default locale): starts on /fr, navigates to /about with { locale: "en" }, asserts the data URL has no locale prefix (/_next/data/test-build/about.json), and locale globals reset to "en".

Both tests properly clean up globalThis.window and globalThis.fetch in finally blocks, and call vi.resetModules() to avoid cross-test pollution from the router module's singleton state.

LGTM.