fix(pages): interpolate same-segment rewrite params - #2030
Conversation
commit: |
|
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: same-segment rewrite param interpolation
I focused this pass on correctness and dev/prod parity, and on whether the new code matches Next.js's documented interpolateAs / href-interpolation-failed behavior. The core interpolation logic in router.ts looks correct and the parity against Next.js is good (the throw fires before routeChangeStart, exactly as Next.js does in router.js:994-1004 → 1017; optional catch-all handling, encoding, and param-omission all line up with interpolate-as.js).
However, there is one blocking regression.
🔴 Blocking: 6 existing tests in tests/link.test.ts now fail
navigatePagesRouterLink now unconditionally writes _vinextInterpolateDynamicRoute into the router options object, even when it is false:
// packages/vinext/src/client/pages-router-link-navigation.ts
const routerOptions: PagesRouterLinkTransitionOptions = {
scroll,
locale,
_vinextInterpolateDynamicRoute: interpolateDynamicRoute, // ← always present
};
if (shallow !== undefined) routerOptions.shallow = shallow;Six pre-existing assertions (not touched by this PR) check the exact shape of the options object passed to router.push/router.replace, e.g. tests/link.test.ts:807:
expect(push).toHaveBeenCalledWith("/", undefined, { scroll: true, locale: false });These now receive { scroll, locale, _vinextInterpolateDynamicRoute: false } and fail. vp test run tests/link.test.ts reports 6 failed | 115 passed, so the Vitest required check will go red in CI:
passes locale=false through the Pages Router Link handoffpasses explicit locale through the Pages Router Link handoffforwards shallow=true through the Pages Router Link handoff (push)forwards shallow=true through the Pages Router Link handoff (replace)forwards shallow=false explicitly when the default is usedomits shallow from router options when the caller does not pass it
Fix: mirror the existing conditional-assignment pattern used for shallow so the flag is only added when truthy:
const routerOptions: PagesRouterLinkTransitionOptions = { scroll, locale };
if (shallow !== undefined) routerOptions.shallow = shallow;
if (interpolateDynamicRoute) routerOptions._vinextInterpolateDynamicRoute = true;This keeps the options object shape unchanged for the common (non-interpolating) path, so the existing assertions pass and performNavigation's options?._vinextInterpolateDynamicRoute === true check still behaves identically. (Alternatively, update the six assertions — but the conditional assignment is the lower-risk change and matches how shallow is already handled.)
Non-blocking notes
- The new
shims.test.tscases (required/optional catch-all, missing-param throw, hash-only no-fetch, cross-origin domain-locale) all pass and cover the important branches well. - The missing-param check (
value === undefined || "" || empty array) is slightly stricter than Next.js's!query[param], but only diverges on falsy-but-present values that don't occur for route params in practice. Fine to leave as-is.
Once the link.test.ts regression is resolved and Vitest is green, this looks good.
|
Review posted to PR #2030. SummaryI reviewed PR #2030 ( 🔴 Blocking: 6 pre-existing tests in
|
8136a21 to
4e49c9d
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: same-segment rewrite param interpolation
I re-reviewed PR #2030 against the latest head (4e49c9d), focusing on correctness, dev/prod parity, and whether the previously-reported blocking regression was resolved.
✅ Prior blocking issue is fixed
The earlier review flagged that navigatePagesRouterLink unconditionally wrote _vinextInterpolateDynamicRoute: false into the router options object, breaking 6 pre-existing tests/link.test.ts assertions. The follow-up commit (4e49c9d) resolves this by switching to the conditional-assignment pattern used for shallow:
// packages/vinext/src/client/pages-router-link-navigation.ts:68-73
const routerOptions: PagesRouterLinkTransitionOptions = { scroll, locale };
if (interpolateDynamicRoute) routerOptions._vinextInterpolateDynamicRoute = true;
if (shallow !== undefined) routerOptions.shallow = shallow;I verified locally:
vp test run tests/link.test.ts tests/shims.test.ts tests/link-navigation.test.ts→ 1293 passedvp checkon the three changed source files → no format/lint/type errors
What's correct
- Interpolation throw ordering —
interpolateCurrentDynamicRoutethrowsHrefInterpolationErrorsynchronously insideperformNavigationbeforerouteChangeStartfires, matching Next.js'shref-interpolation-failedordering. The newshims.test.tscase asserts the throw happens with nopushState. - Param-format handling —
routePatternPartsnormalizes Next.js bracket format to:param; the missing-param filter correctly includes required (:id) and required catch-all (:slug+, stripping+) while excluding optional catch-all (:slug*). Verified againstfillRoutePatternSegmentsso the two never disagree. - Encoding & param omission — params are
encodeURIComponent-ed and route params are deleted from the resulting search string, matching Next.jsinterpolate-as.js. - Locale / basePath / hash-only — visible-path locale is stripped before matching and re-applied to the target; hash-only
UrlObjectnavigation is excluded from interpolation (covered by the new push/replace hash-only tests that assert nofetch). - Cross-origin guard — for cross-domain-locale targets the function returns
resolvedunchanged (no path interpolation into a foreign origin), and the test asserts the rewritten path is preserved. This is a deliberate, documented choice.
One thing worth a second look (non-blocking)
The Link click handler was refactored from a single try/catch wrapping the whole navigation to navigatePagesRouterLinkWithFallback, whose fallback only runs if loadRouter() throws. The actual router.push/router.replace await (pages-router-link-navigation.ts:106) is no longer inside a fallback try/catch.
This is intentional and correct for the HrefInterpolationError path (Next.js throws there too, it does not hard-navigate). But it also means any other rejection from router.push/router.replace now propagates as an unhandled rejection out of the async onClick instead of triggering the old hard-navigation fallback. In practice performNavigation resolves to false rather than rejecting for normal failures, so this is unlikely to regress real navigations — but it is a behavioral narrowing of the fallback that's worth a conscious confirmation against Next.js semantics. Not blocking.
Verdict
The blocking regression is resolved, all targeted Vitest files are green, and the interpolation logic lines up with Next.js parity. LGTM.
|
Review posted to PR #2030. SummaryI re-reviewed PR #2030 ( Key finding: the prior blocking regression is fixedThe earlier I verified locally:
What's correct
One non-blocking noteThe Link handler was refactored so the fallback now only fires on Verdict: LGTM. No actionable blocking issues remain. |
4e49c9d to
59e988a
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: same-segment rewrite param interpolation
I reviewed PR #2030 against local head (59e988a / 68a1e79), focusing on correctness, Next.js parity, dev/prod parity, and edge cases beyond what prior /bigbonk passes covered.
Verification
vp test run tests/link.test.ts tests/shims.test.ts tests/link-navigation.test.ts→ 1295 passedvp checkon the three changed source files → no format/lint/type errorsvp run vinext#build→ build complete (thevirtual:vite-rsc/client-referenceswarning is pre-existing and unrelated)
The blocking regression from the earlier pass (unconditional _vinextInterpolateDynamicRoute: false write breaking 6 link.test.ts assertions) is resolved — pages-router-link-navigation.ts:72 now only writes the flag when truthy, mirroring shallow.
What I checked and confirmed correct
- No double-encoding.
querycomes fromparseQueryString(target.search), which decodes viaURLSearchParams, so the subsequentencodeURIComponentininterpolateCurrentDynamicRoutere-encodes exactly once. - Non-route query params are preserved. Only
extractRouteParamNames(routePattern)keys are deleted fromtarget.searchParams, so?id=1&foo=baron/x/[id]correctly yields/x/1?foo=bar, matching Next.jsinterpolate-as.js. - Cross-origin guard is correct. Relative
resolvedparses tohttp://vinext.localorigin and is not early-returned; the cross-domain-locale case returnsresolvedunchanged and hard-navigates (tested,win.location.assignwith the foreign origin, nopushState). - Catch-all param extraction. The
missingParamsfilter keeps required (:id) and required catch-all (:slug+, stripping+) while excluding optional catch-all (:slug*), and stays consistent withfillRoutePatternSegments' null-on-empty behavior. - Hash-only excluded.
inheritsCurrentPathdeliberately omits thehashbranch present inreplaceInheritedLocale, so hash-onlyUrlObjectnavigation skips interpolation (covered by the new push/replace no-fetch tests). - Interpolation throw ordering.
HrefInterpolationErroris thrown synchronously beforerouteChangeStartfires, matching Next.jshref-interpolation-failedordering (asserted with nopushState).
On the fallback narrowing (re-confirmed, non-blocking)
The Link handler moved from a try/catch wrapping the whole navigation to navigatePagesRouterLinkWithFallback, whose fallback only fires on loadRouter() failure. I traced every reject path of performNavigation:
- Genuine navigation failures (network/parse/import/render) are caught inside
runNavigateClient(router.ts:2144-2157), which handles the hard-nav fallback itself and resolvesperformNavigationtofalse— it does not reject. So those never relied on the Link-level catch. - The only rejecting paths are
HrefInterpolationError, the synchronous dangerous-scheme assert (already blocked upstream in Link viaisDangerous), and the SSR no-router throw.
So the narrowing is benign and the rethrow-not-fallback behavior is intentional and tested (link.test.ts: "rethrows missing-required-param interpolation errors instead of using Link fallback"). This matches Next.js, which rethrows non-cancelled errors from linkClicked.
Minor notes (non-blocking)
- For a non-catch-all
[id]receiving an array of length > 1,fillRoutePatternSegmentsreturnsnullandinterpolateCurrentDynamicRoutesilently returnsresolved(no interpolation) rather than throwinghref-interpolation-failed. This is a rare shape and unlikely to occur for real route params; fine to leave. interpolateDynamicRouteis wired only through<Link>(resolvedHref.startsWith("?")); a direct stringrouter.push("?id=1")won't set the flag and won't interpolate. UrlObject pushes do interpolate without the flag. This is consistent with how the feature is scoped, just worth a mental note.
Verdict
LGTM. The blocking regression is fixed, targeted Vitest files and the build are green, and the interpolation logic matches Next.js parity. No actionable blocking issues found.
|
Review posted to PR #2030. SummaryI reviewed PR #2030 ( Verdict: LGTM — no actionable blocking issues. Verification
The previously-flagged blocking regression (unconditional Additional edge cases I scrutinized (all correct)
On the fallback narrowing (non-blocking, re-confirmed)I traced every reject path of Two minor non-blocking notes were included in the review (array-of-length>1 on a non-catch-all |
1 participant
Summary
href-interpolation-failedbehavior for missing required parametersStack
This PR is stacked on #2028 (
codex/fix-pages-rewrites-navigation-20260615) because it relies on that PR's visible-URL and history handling.Next.js parity
Fixes the remaining three same-segment assertions in
test/e2e/use-router-with-rewrites/use-router-with-rewrites.test.ts.Validation
Cache Components, PPR, and resume behavior are out of scope.