fix(link): avoid reusing dynamic app route prefetches

[james-elicx]

Summary

• stop treating automatic dynamic App Router prefetches as authoritative navigation payloads
• keep shell warming for dynamic links so click navigations issue a fresh dynamic request and derive params from the target tree
• add parallel-route navigation coverage that blocks the request and verifies the dynamic request happens before asserting updated slot params

Tests

• vp test run tests/link.test.ts tests/link-navigation.test.ts
• PLAYWRIGHT_PROJECT=app-router pnpm run test:e2e -- tests/e2e/app-router/parallel-route-navigations.spec.ts
• upstream Next.js e2e: test/e2e/app-dir/parallel-route-navigations/parallel-route-navigations.test.ts
• scoped vp check

Review

• Independent review by Locke: NO FINDINGS

[james-elicx]

/bigbonk review for issues

[pkg-pr-new]

Open in StackBlitz

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

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

commit: 5e3ae29

[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

[ask-bonk]

Review: fix(link): avoid reusing dynamic app route prefetches

Reviewed the shim change and its interaction with the navigation/prefetch reuse paths. No blocking issues found. The change is correct and internally consistent.

What the change does

resolveMatchedAutoAppRoutePrefetch now sets cacheForNavigation: !hasLoadingShell && !route.isDynamic (previously !hasLoadingShell). Dynamic routes without a loading shell now warm an optimistic shell (optimisticRouteShell: true) instead of caching a full payload keyed by concrete URL.

Correctness verification

• Navigation reuse is correctly gated. navigation.ts:440 and navigation.ts:451 skip entries where cacheForNavigation === false, and navigation.ts:871 returns null for them. So a dynamic auto-prefetch can no longer be reused as the authoritative nav payload — the click issues a fresh dynamic request, deriving params from the target tree. This matches the PR intent.
• Shell entries are not wrongly promoted. promotePrefetchEntriesForNavigation (link.tsx:672) skips optimisticRouteShell === true entries, so the warmed dynamic shell won't be silently upgraded to cacheForNavigation: true on click.
• unstable_dynamicOnHover path is unaffected. That test uses /blog/hello where canPrefetchLoadingShell: true, so cacheForNavigation was already false before this change (!true == !true && !isDynamic). The test correctly required no update.
• Test fixtures line up. The updated tests use /clothing/1 and /products/1 (canPrefetchLoadingShell: false, isDynamic: true), which flip from true -> false exactly as expected.

E2E test

The added lock-based test (parallel-route-navigations.spec.ts) is a faithful port. The page.route lock is installed before goto but lock is undefined until after the initial navigation settles, so the initial load passes through untouched. expect(hadLocked).toBe(1) under expectStable correctly asserts exactly one fresh request is issued on click (and stays 1 for the stable window), proving prefetch data isn't reused. Good coverage.

Checks run

• vitest run tests/link.test.ts tests/link-navigation.test.ts — 175 passed
• vitest run tests/shims.test.ts — 1174 passed
• tsc --noEmit -p packages/vinext/tsconfig.json — clean

Minor observations (non-blocking)

• canAutoPrefetchFullAppRoute is exported but only consumed by tests; the behavior change there is test-observable only. Fine as-is.
• Parity: this is a client-side shim change, so the dev/prod server files (prod-server.ts, worker-entry.ts) aren't affected — the reuse gating lives entirely in the shared navigation.ts consumption path, which all environments share.

LGTM.

[ask-bonk]

Review posted to PR #2450. Summary of my findings:

Review Result: No blocking issues — LGTM

I reviewed the change (cacheForNavigation: !hasLoadingShell && !route.isDynamic) and traced its interaction with the full prefetch/navigation lifecycle.

Correctness confirmed:

• Dynamic auto-prefetches now get cacheForNavigation: false, and the navigation reuse path (navigation.ts:440,451,871) correctly excludes such entries — so clicks issue a fresh dynamic request instead of reusing stale-param data. This is exactly the fix's intent.
• Shell entries flagged optimisticRouteShell: true are skipped in promotePrefetchEntriesForNavigation (link.tsx:672), so they won't be silently upgraded on click.
• The unchanged unstable_dynamicOnHover test (/blog/hello) is correctly unaffected since that route already had canPrefetchLoadingShell: true.
• Updated test fixtures (/clothing/1, /products/1) flip true→false exactly as expected.

E2E test: The lock-based test is a faithful port; the page.route lock passes through the initial load and correctly asserts exactly one fresh request on click.

Verification I ran:

• tests/link.test.ts + tests/link-navigation.test.ts — 175 passed
• tests/shims.test.ts — 1174 passed
• tsc --noEmit on the vinext package — clean

Only non-blocking notes: canAutoPrefetchFullAppRoute is test-only, and dev/prod parity is fine since the gating lives in the shared navigation.ts path all environments use.

github run

[james-elicx]

Deploy-suite run 28478866791 / job 84413308650 backlog check: this PR appears to cover the test/e2e/app-dir/segment-cache/encoded-slash-params/encoded-slash-params.test.ts failures when stacked with #2440. I verified the exact targeted Next.js E2E path passes with #2440 plus this PR applied, including unencoded param > back navigation does not refetch the route tree and encoded slash in param > back navigation does not refetch the route tree. Skipping a duplicate implementation pass for this item.

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: fix(link): avoid reusing dynamic app route prefetches

I traced the core shim change and its interaction with the prefetch/navigation reuse paths, ran the unit tests, and audited the E2E wiring. The runtime logic is correct, but there is one CI-blocking test-configuration issue.

Finding (blocking): new E2E spec runs against the dev server, where it will fail

playwright.config.ts adds segment-cache-client-params.spec.ts to the app-router-client-cache project (which runs a production build via cli.js build && cli.js start), but it does not add it to the testIgnore list of the app-router project:

"app-router": {
  testDir: "./tests/e2e",
  testMatch: ["**/app-router/**/*.spec.ts", "**/og-image.spec.ts"],
  testIgnore: [appRouterBrowserSpecificTests, "**/app-router/nextjs-compat/client-cache.spec.ts"],
  ...
}

The new spec lives at tests/e2e/app-router/nextjs-compat/segment-cache-client-params.spec.ts, so it matches **/app-router/**/*.spec.ts and will also run in the app-router project against the dev server (npx vp dev, port 4174). Both projects run in CI (.github/workflows/ci.yml has app-router split 1/3–3/3 and app-router-client-cache).

This is exactly why the sibling client-cache.spec.ts was explicitly added to the app-router project's testIgnore (added in #2424) — those prefetch-cache-reuse tests depend on production-build automatic-prefetch semantics and have no per-test dev/prod skip. The new spec tests the same class of behavior: its final assertion expect(requestsFor(requests, targetPath)).toEqual([]) asserts the concrete dynamic full prefetch is reused with zero RSC requests on click — a prod-build guarantee. Under the dev server this assertion is very likely to fail/flake.

Suggested fix: add the new spec to the app-router project's testIgnore alongside client-cache.spec.ts:

testIgnore: [
  appRouterBrowserSpecificTests,
  "**/app-router/nextjs-compat/client-cache.spec.ts",
  "**/app-router/nextjs-compat/segment-cache-client-params.spec.ts",
],

Correctness of the shim change (LGTM)

• resolveMatchedAutoAppRoutePrefetch now uses cacheForNavigation: !hasLoadingShell && route.requiresDynamicNavigationRequest !== true. This correctly narrows commit 1's overly-broad !route.isDynamic gate: concrete dynamic URLs (/clothing/1, /products/1) regain full-prefetch reuse, while dynamic routes with parallel slots force a fresh click-time request so slot params derive from the target tree.
• requiresDynamicNavigationRequest = route.isDynamic && route.parallelSlots.length > 0 is projected in toLinkPrefetchRoute and correctly matches the parallel-route fixture ([teamID]/sub/other-folder under a @slot layout).
• promotePrefetchEntriesForNavigation (link.tsx:672) skips optimisticRouteShell === true entries, so the warmed dynamic shell is never silently promoted to cacheForNavigation: true.
• The lock-based parallel-route-navigations.spec.ts change is a faithful port; expect(hadLocked).toBe(1) correctly asserts exactly one fresh dynamic request on click.

Minor (non-blocking)

• parallelSlots is inherited (includes ancestor slots), so a leaf route under a parallel-slot layout is covered. However, isDynamic is computed only from the route's own path segments (convertSegmentsToRouteParts(segments) at app-route-graph.ts:1624), not from params inside its parallel slots. A static path whose slot has a dynamic catch-all (e.g. /dashboard + @feed/[...items]) would have requiresDynamicNavigationRequest === false and could still reuse a prefetch across slot params. This is a pre-existing gap (the same was true under !hasLoadingShell and !route.isDynamic), not a regression from this PR — just flagging for a possible follow-up.

Checks run

• vitest run tests/link.test.ts tests/link-navigation.test.ts tests/entry-templates.test.ts — 215 passed

[ask-bonk]

Review posted to PR #2450.

Summary

I reviewed the PR (2 commits) which fixes reuse of dynamic App Router prefetches for parallel-route navigations.

Blocking finding: new E2E spec will run against the dev server and likely fail in CI

playwright.config.ts adds segment-cache-client-params.spec.ts to the prod-build app-router-client-cache project, but does NOT add it to the testIgnore list of the dev-server app-router project. Since it lives under tests/e2e/app-router/nextjs-compat/ and matches **/app-router/**/*.spec.ts, it will run in both projects. Its final assertion expect(requestsFor(requests, targetPath)).toEqual([]) (zero RSC requests on click) is a production-build prefetch-reuse guarantee that won't hold under npx vp dev. This is exactly why the sibling client-cache.spec.ts was explicitly excluded from the app-router project in #2424. Fix: add the new spec to the same testIgnore array.

Correctness (LGTM)

• The narrowed gate !hasLoadingShell && route.requiresDynamicNavigationRequest !== true correctly restores full-prefetch for concrete dynamic URLs while forcing fresh navigation only for dynamic routes with parallel slots.
• promotePrefetchEntriesForNavigation correctly skips optimisticRouteShell entries so warmed shells aren't promoted.
• The lock-based parallel-route E2E test faithfully asserts exactly one fresh request on click.

Minor (non-blocking, pre-existing)

• isDynamic is computed only from the route's own path, not from params inside parallel slots — a static path with a dynamic catch-all slot could still reuse prefetches. Not a regression from this PR.

Verification: vitest run tests/link.test.ts tests/link-navigation.test.ts tests/entry-templates.test.ts — 215 passed.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: fix(link): avoid reusing dynamic app route prefetches

Re-reviewed after the two follow-up commits (fix(link): preserve static dynamic app prefetches, test(e2e): scope segment cache client params to prod). No blocking issues found — LGTM. The prior blocking finding has been resolved.

Prior blocking finding is fixed

The earlier review flagged that the new segment-cache-client-params.spec.ts would also run in the dev-server app-router project and fail its prod-only prefetch-reuse assertion. This is now fixed: playwright.config.ts:51 adds "**/app-router/nextjs-compat/segment-cache-client-params.spec.ts" to the app-router project's testIgnore, alongside the sibling client-cache.spec.ts. The spec only runs in the prod-build app-router-client-cache project (playwright.config.ts:58).

Correctness (verified)

• The narrowed gate cacheForNavigation: !hasLoadingShell && route.requiresDynamicNavigationRequest !== true (link.tsx:325) correctly restores full-prefetch reuse for concrete dynamic URLs (/clothing/1, /products/1) while forcing a fresh click-time request only for dynamic routes with parallel slots.
• requiresDynamicNavigationRequest = route.isDynamic && route.parallelSlots.length > 0 (app-browser-entry.ts:68) is projected only when true, keeping the manifest compact; entry-templates.test.ts asserts the exact serialized shape.
• The E2E header-tracking is consistent with the runtime: header X-Vinext-Rsc-Render-Mode (headers.ts:68) → lowercased x-vinext-rsc-render-mode in Playwright; value prefetch-loading-shell matches APP_RSC_RENDER_MODE_PREFETCH_LOADING_SHELL.
• The lock-based parallel-route-navigations.spec.ts change faithfully asserts exactly one fresh dynamic request on click via expect(hadLocked).toBe(1) inside expectStable.

Verification I ran

• vp test run tests/link.test.ts tests/link-navigation.test.ts tests/entry-templates.test.ts — 215 passed
• tsc --noEmit -p packages/vinext/tsconfig.json — clean

Non-blocking observations (not regressions from this PR)

• requiresDynamicNavigationRequest? was added to the VinextPagesLinkPrefetchRoute type (vinext-next-data.ts:34), but the Pages Router projection (toPagesLinkPrefetchRoute) never sets it and Pages Router has no parallel slots. It's harmless dead surface — consider dropping it from the Pages type, or leave as-is for type symmetry.
• toDocumentOnlyAppRoute doesn't set the field; correct, since document-only (API) routes never reuse RSC prefetches.
• Pre-existing gap (also flagged earlier): isDynamic is computed from the route's own path segments only (app-route-graph.ts:1624), not from params inside parallel slots. A static path with a dynamic catch-all slot (e.g. /dashboard + @feed/[...items]) has requiresDynamicNavigationRequest === false and could still reuse a prefetch across slot params. This was equally true under the old !route.isDynamic gate, so it's not a regression — possible follow-up.
• Pre-existing (not this PR): app-router-client-cache and app-router-prefetch-searchparams both bind port 4191, but they use the same fixture and CI runs one project per runner via PLAYWRIGHT_PROJECT, so they never collide.

[ask-bonk]

Review posted to PR #2450.

Summary

I re-reviewed the PR after the two follow-up commits and found no blocking issues — LGTM.

Prior blocking finding resolved: The earlier review flagged that segment-cache-client-params.spec.ts would run against the dev server and fail its prod-only assertion. This is now fixed — playwright.config.ts:51 adds it to the app-router project's testIgnore, so it only runs in the prod-build app-router-client-cache project.

Correctness verified:

• The narrowed gate !hasLoadingShell && route.requiresDynamicNavigationRequest !== true correctly restores full-prefetch reuse for concrete dynamic URLs while forcing a fresh request only for dynamic routes with parallel slots.
• requiresDynamicNavigationRequest = isDynamic && parallelSlots.length > 0, projected only when true.
• E2E header tracking matches the runtime (X-Vinext-Rsc-Render-Mode / prefetch-loading-shell).
• Lock-based parallel-route test faithfully asserts exactly one fresh request on click.

Verification I ran:

• vp test run tests/link.test.ts tests/link-navigation.test.ts tests/entry-templates.test.ts — 215 passed
• tsc --noEmit on the vinext package — clean

Non-blocking notes: requiresDynamicNavigationRequest? was added to the Pages type but is never set (harmless dead surface); the isDynamic-ignores-slot-params gap and the pre-existing 4191 port sharing are both not regressions from this PR.

github run

[james-elicx]

Backlog recheck for deploy-suite run 28478866791 / job 84413308650: this PR already covers the remaining non-cacheComponents/use-cache functional failures in test/e2e/app-dir/segment-cache/encoded-slash-params/encoded-slash-params.test.ts when stacked with #2440. Skipping a duplicate implementation/worktree for this item.

[james-elicx]

Backlog triage for deploy-suite run 28478866791 / job 84413308650: this PR already covers the non-cacheComponents/use-cache test/e2e/app-dir/parallel-route-navigations/parallel-route-navigations.test.ts failure (parallel-route-navigations > should render the right parameters on client navigations). Its fix forces a fresh dynamic navigation request for dynamic App routes with parallel slots, and the PR validation includes the exact upstream Next.js E2E target. Skipping a duplicate implementation/worktree for this item.

[james-elicx]

Backlog ownership note for deploy-suite run 28478866791 / job 84413308650:\n\n- Artifact row inspected: test-results-4/e2e/app-dir/parallel-route-navigations/parallel-route-navigations.test.ts.results.json.\n- Exact upstream failure: test/e2e/app-dir/parallel-route-navigations/parallel-route-navigations.test.ts, parallel-route-navigations > should render the right parameters on client navigations, line 108, expect(hadLocked).toBe(1) with Expected: 1 Received: 0.\n- Ownership mapping: this PR covers that failure by forcing a click-time dynamic request for dynamic App Router routes with parallel slots, so the lock count becomes 1 and slot params derive from the target tree instead of a reused prefetch payload.\n- Validation already done here: merged into origin/main as b0367f144; PR validation included the exact upstream Next.js E2E target, the ported vinext Playwright spec tests/e2e/app-router/parallel-route-navigations.spec.ts, vp test run tests/link.test.ts tests/link-navigation.test.ts, scoped vp check, and a final /bigbonk re-review with no blocking findings.\n\nNo duplicate backlog branch/PR needed for parallel-route-navigations.

[ask-bonk]

fatal: couldn't find remote ref codex/fix-parallel-route-navigation-params

github run

[ask-bonk]

@james-elicx Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.