fix(pages): preserve rewritten URLs during navigation

[james-elicx]

Summary

• resolve query/search/hash-only Pages Router navigation against the browser-visible rewritten URL
• preserve locale and basePath behavior for query-only push, replace, and Link navigation
• retain raw history asPath, including a trailing bare ?
• align UrlObject empty/query/search/hash formatting with Next.js
• add dynamic rewrite fixtures and integration coverage

Next.js parity

Fixes six assertions in test/e2e/use-router-with-rewrites/use-router-with-rewrites.test.ts covering root and cross-segment push/replace/Link navigation. Three same-segment rewrite cases remain separate follow-up scope.

Validation

• pinned Next.js v16.2.6 targeted assertions: 6/6 passed
• 120 Link tests and 1,119 shim tests passed
• 14 Pages navigation E2E tests, router events, and PR fix(router): restore pages scroll traversal state #1999 scroll restoration passed
• full vp check and vinext build passed
• repeated independent reviews; no actionable findings remain

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2028

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

commit: 292db56

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original
workers-cache	preview	production
web	preview	production

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: fix(pages): preserve rewritten URLs during navigation

I reviewed the diff, checked Next.js parity against the upstream formatUrl source, and ran the targeted suites (tests/shims.test.ts, tests/link.test.ts, vp check). Everything passes and the implementation closely mirrors Next.js. No actionable findings.

CodeQL finding (router.ts:499) — not a bug, this is deliberate Next.js parity

CodeQL flags search.replace("#", "%23") as incomplete string escaping because it replaces only the first #. This is intentional and correct — it is a byte-for-byte match of Next.js's own formatUrl:

// next/src/shared/lib/router/utils/format-url.ts
pathname = pathname.replace(/[?#]/g, encodeURIComponent)
search = search.replace('#', '%23')   // <-- single replace, first occurrence only

vinext aims to replicate Next.js semantics exactly, so diverging here (e.g. using replaceAll) would be a parity regression, not a fix. The new test cases ({ search: "q=a#b" } → ?q=a%23b) confirm the matching behavior. I'd recommend dismissing the CodeQL alert as "won't fix / intended behavior" with a link to the Next.js source, and optionally adding a short comment at the call site noting the parity rationale so the next reader doesn't try to "fix" it.

Verified behaviors

• resolveUrl precedence — search correctly wins over query (urlObj.search || (query && ...) in Next.js); the { query: { id: "ignored" }, search: "id=3" } → ?id=3 test confirms parity.
• Path inheritance — query/search/hash-only UrlObjects without pathname inherit the visible window.location.pathname, which is what makes rewritten URLs survive navigation. Hash-only navigation correctly preserves the current window.location.search.
• getCurrentHistoryAsPath — guarded by a pathname+search equality check against the browser URL and wrapped in try/catch; falls back cleanly to the prior resolvedPath + search + hash computation. Sound.
• Locale handling — replaceInheritedLocale only triggers for as === undefined + explicit options.locale + pathname-less UrlObject, and removeNavigationLocalePrefix strips the inherited prefix before re-applying, avoiding double-prefixing.
• Link Router resolution — window.next?.router vs dynamic import("next/router") fallback (guarded by "reload" in Router) preserves the previous behavior when the debug global is unavailable.

Test coverage

The parametrized tests/shims.test.ts cases (empty object, empty query, bare ?, encoded space, escaped #, query/search override, hash combinations, locale prefix) provide good coverage of the formatting matrix, and getCurrentHistoryAsPath's raw asPath retention is exercised. E2E coverage for root/cross-segment push/replace/Link looks appropriate.

LGTM.

[ask-bonk]

Review posted to PR #2028.

Summary

I reviewed the PR's source changes, verified Next.js parity against the upstream formatUrl source, and ran the targeted test suites (tests/shims.test.ts, tests/link.test.ts, vp check) — all pass.

Outcome: No actionable findings. LGTM.

Key points:

• CodeQL finding (router.ts:499) — The flagged search.replace("#", "%23") (single-occurrence replace) is not a bug. It is a byte-for-byte match of Next.js's own formatUrl in format-url.ts, which uses the exact same single .replace('#', '%23'). Diverging here would be a parity regression. I recommended dismissing the alert as intended behavior and optionally adding a parity comment at the call site.

• Verified correct: search-over-query precedence, visible-pathname inheritance for query/search/hash-only navigation, the guarded getCurrentHistoryAsPath fallback, locale replaceInheritedLocale (no double-prefixing), and the window.next?.router vs dynamic-import fallback.

• Test coverage is thorough — the parametrized formatting matrix and raw asPath retention are well exercised, alongside E2E push/replace/Link cases.

github run

[james-elicx]

/bigbonk review for issues

[james-elicx]

/bigbonk review for issues

[ask-bonk]

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

[ask-bonk]

Review: fix(pages): preserve rewritten URLs during navigation

Reviewed the diff against Next.js source (format-url.ts, resolve-href.js, router.js from the pinned next@16.2.7 in node_modules), traced the formatting/locale/asPath edge cases, and ran the targeted suites: tests/shims.test.ts (1120), tests/link.test.ts (120), tests/routing.test.ts (112), plus vp check on the three changed source files. All green. No actionable findings — LGTM.

Parity verification

• resolveUrl search/# formatting matches formatUrl byte-for-byte: ?-prefix happens before the single .replace('#', '%23'), so { search: "q=a#b#c" } → ?q=a%23b#c and { search: "#foo" } → ?%23foo both line up with Next.js. The CodeQL "incomplete string escaping" alert on router.ts:499 is a deliberate parity match, not a bug — format-url.js:54 uses the same single-occurrence replace('#', '%23'). Recommend dismissing as intended.
• search over query precedence matches Next's urlObj.search || query && '?'+query ({ query: {...}, search: "id=3" } → ?id=3).
• Query-only / dynamic-rewrite path inheritance is the right scope. Next's resolve-href.js uses router.asPath for ?-hrefs, switching to router.pathname only when the route is dynamic and asPath matches the pattern. The fixture (/rewrite-navigation/:id → /rewrite-navigation/[id]/destination) is exactly the rewritten/non-matching case, so inheriting window.location.pathname is correct. The same-segment-rewrite cases the PR scopes out correspond to the documented edge case in Next's own source comment.
• getCurrentHistoryAsPath is correctly guarded: it only overrides asPath when state.as resolves to the same pathname+search as the visible URL (try/catch falls back cleanly), which is what preserves a raw trailing bare ? that window.location.search collapses. navState.as = stripHash(resolved) keeps hash out of the state as, matching the { hash } test expectations.
• Locale handling does not double-prefix: the Pages string path resolves locale once via resolvePagesLinkNavigationHref/applyLocaleToHref, and the subsequent router.push(stringHref, …, { locale }) short-circuits in applyNavigationLocale because the prefix is already present (replaceExistingLocale only triggers for pathname-less UrlObjects). resolvePagesRouterQueryOnlyHref strips the locale from the window.location.pathname fallback before resolving, since router.asPath is locale-free — confirmed by the basePath+locale test.

Notes (non-blocking)

• In link.tsx, pagesNavigateHref is computed for any ?-href even when hasAppNavigationRuntime is true (it's only consumed on the Pages branch). Harmless, just a tiny bit of wasted work; not worth changing.

Tests

Coverage is strong: the parametrized formatting matrix (empty object, empty query, bare ?, encoded space, escaped #, search-over-query, hash combinations), bare-? asPath retention for both push/replace, the basePath+locale Link resolution, and E2E push/replace/Link/search+hash/bare-query cases. The tests/routing.test.ts adjustment (same-depth static-before-dynamic) is the correct accommodation for the new nested dynamic fixture.

[ask-bonk]

Review posted to PR #2028.

Summary

I reviewed the source changes against the pinned Next.js v16.2.7 source (format-url.ts, resolve-href.js, router.js), traced the formatting/locale/asPath edge cases, and ran the targeted suites — tests/shims.test.ts (1120), tests/link.test.ts (120), tests/routing.test.ts (112), and vp check on all three changed source files. Everything passes.

Outcome: No actionable findings. LGTM.

Key verifications:

• resolveUrl search/# formatting is a byte-for-byte match of Next's formatUrl — including the CodeQL-flagged single replace('#', '%23') at router.ts:499, which is deliberate parity (recommend dismissing the alert).
• search-over-query precedence and query-only dynamic-rewrite path inheritance match Next's resolve-href.js logic; the scoped-out same-segment cases align with Next's own documented edge case.
• getCurrentHistoryAsPath is correctly guarded with a pathname+search equality check and try/catch fallback, preserving the raw bare trailing ?.
• Locale handling does not double-prefix — confirmed across the string path, the replaceInheritedLocale UrlObject path, and the basePath+locale Link path.

One non-blocking note: pagesNavigateHref is computed even when the App Router runtime is active (where it's unused) — harmless.

Test coverage is thorough across the formatting matrix, asPath retention, locale/basePath resolution, and E2E navigation cases. The routing test adjustment correctly accommodates the new nested dynamic fixture.

github run

[james-elicx]

/bigbonk review for issues

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: fix(pages): preserve rewritten URLs during navigation

Reviewed all three changed source files against the pinned next@16.2.7 source (format-url.js, resolve-href.js), traced the formatting/locale/asPath edge cases by hand, and ran the targeted suites: tests/routing.test.ts + tests/route-sorting.test.ts (159), tests/shims.test.ts + tests/link.test.ts (1240), plus vp check on the three changed source files. All green. No actionable findings — LGTM.

Parity verification

• resolveUrl formatting matches formatUrl byte-for-byte. The ?-prefix is applied before the single .replace('#', '%23'), %23 is applied only to search (never to hash, which is appended afterward), and the bare ? survives (new URL("?", base) preserves the trailing delimiter — verified directly). The CodeQL "incomplete string escaping" alert at router.ts:499 is a deliberate parity match with format-url.js:54's single-occurrence replace('#', '%23'); diverging (e.g. replaceAll) would be a regression. Recommend dismissing as intended.
• search over query precedence matches Next's urlObj.search || query && '?'+query — hasSearch short-circuits the query branch, and search: "" correctly falls through to query (matching the falsy urlObj.search semantics).
• Query-only path inheritance is the correctly-scoped subset. Next's resolve-href.js uses router.asPath for ?-hrefs, switching to router.pathname only when the route is dynamic and asPath matches the pattern. The fixture (/rewrite-navigation/:id -> /rewrite-navigation/[id]/destination) is exactly the rewritten/non-matching case, so inheriting the visible pathname is correct. The same-segment cases scoped out of this PR map to Next's own documented edge-case comment (resolve-href.js:49-54).
• getCurrentHistoryAsPath is properly guarded: it only overrides asPath when state.as resolves to the same pathname+search as the visible URL (try/catch falls back cleanly), preserving a raw trailing bare ? that window.location.search collapses. navState.as = stripHash(resolved) keeps hash out of the state, and the hash is re-applied from window.location.hash — consistent with the { hash } tests.
• Locale handling does not double-prefix. The Pages string path resolves locale once (resolvePagesLinkNavigationHref -> applyLocaleToHref), and the subsequent router.push(stringHref, …, { locale }) short-circuits because replaceInheritedLocale requires typeof url !== "string". removeNavigationLocalePrefix strips the inherited prefix before re-applying for the pathname-less UrlObject path. locale: false correctly resolves to the default locale and drops the prefix (basePath + locale:false test confirms /docs/rewrite-navigation/0?id=2).

Notes (non-blocking)

• In link.tsx, pagesNavigateHref is computed for any ?-href even when hasAppNavigationRuntime is true (it's only consumed on the Pages branch via the hasAppNavigationRuntime ? navigateHref : pagesNavigateHref ternary). Harmless wasted work; not worth changing.
• String #hash-only Link hrefs on rewritten pages don't go through the new rewrite-aware resolver (only ?-prefixed strings do), whereas Next's resolve-href uses router.asPath for # hrefs too. This is pre-existing behavior and outside this PR's stated scope (query/search/hash-only UrlObjects).

Tests

Coverage is strong: the parametrized formatting matrix (empty object, empty query, bare ?, encoded space, escaped #, search-over-query, hash combinations), bare-? asPath retention for both push/replace, basePath+locale resolution including locale: false, and E2E push/replace/Link/search+hash/bare-query cases. The tests/routing.test.ts adjustment (same-depth static-before-dynamic) is the correct accommodation for the new nested dynamic fixture (/rewrite-navigation/[id]/destination), which legitimately breaks the prior global "all static before all dynamic" invariant.

[ask-bonk]

Review posted to PR #2028.

Summary

I reviewed all three changed source files (router.ts, link.tsx, pages-router-link-navigation.ts) against the pinned next@16.2.7 source, traced the edge cases by hand, and ran the targeted suites (tests/routing.test.ts, tests/route-sorting.test.ts, tests/shims.test.ts, tests/link.test.ts — 1399 tests) plus vp check. Everything passes. No actionable findings — LGTM.

Key verifications:

• resolveUrl is a byte-for-byte match of Next's formatUrl — ?-prefix before the single .replace('#', '%23'), %23 applied only to search (not hash), and the bare ? survives (verified new URL("?", base) preserves it directly).
• CodeQL finding (router.ts:499) — the single-occurrence replace("#", "%23") is deliberate parity with format-url.js:54, not a bug. Recommend dismissing as intended.
• Query-only path inheritance is the correctly-scoped subset of Next's resolve-href.js; the same-segment cases the PR scopes out map to Next's own documented edge case.
• getCurrentHistoryAsPath is properly guarded with pathname+search equality + try/catch, preserving the raw bare ?.
• Locale handling does not double-prefix across the string path, the replaceInheritedLocale UrlObject path, and locale: false.

Two non-blocking notes: pagesNavigateHref is computed even when the App Router runtime is active (harmless), and string #hash-only Link hrefs don't get the new rewrite-aware resolver (pre-existing, outside scope). Test coverage and the routing.test.ts adjustment for the new nested-dynamic fixture are both correct.

github run