fix(app-router): align dynamic-on-hover prefetches by james-elicx · Pull Request #2464 · cloudflare/vinext

james-elicx · 2026-07-01T06:02:19Z

Summary

keep dynamic-on-hover upgrades behind an existing loading-shell prefetch
omit static layouts from hover-time dynamic segment payloads
add focused Link and App Router render coverage for shell-first dynamic hover prefetching

Next.js parity

Targets the App Router Segment Cache behavior covered by:

test/e2e/app-dir/segment-cache/dynamic-on-hover/dynamic-on-hover.test.ts

Validation

vp test run tests/link-navigation.test.ts -t "loading-shell prefetch|shell prefetch fails|dynamic-on-hover"
vp test run tests/app-page-render.test.ts tests/app-page-route-wiring.test.ts -t "dynamic-on-hover|loading-shell prefetches without|omits static layouts"
vp test run tests/link-navigation.test.ts tests/app-page-render.test.ts tests/app-page-route-wiring.test.ts
vp check on touched files
REPO="$(pwd)" NEXTJS_DIR="/Users/jamesanderson/Developer/vinext/.nextjs-ref" NEXT_TEST_CONCURRENCY=1 ./scripts/run-targeted-nextjs-e2e.sh test/e2e/app-dir/segment-cache/dynamic-on-hover/dynamic-on-hover.test.ts (1/1 passed)

Review

Independent review found one issue before PR: hover dynamic-after-shell could proceed after a failed/stale shell prefetch and cache a payload missing static layouts. This branch includes the follow-up guard and regression test.

james-elicx · 2026-07-01T06:02:41Z

/bigbonk review for issues

pkg-pr-new · 2026-07-01T06:03:39Z

Open in StackBlitz

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

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

commit: c04ae67

github-actions · 2026-07-01T06:03:46Z

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 · 2026-07-01T06:03:54Z

UnknownError: ProviderInitError

github run

ask-bonk · 2026-07-01T06:03:58Z

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

View workflow run · To retry, trigger Bonk again.

james-elicx · 2026-07-01T06:08:57Z

/bigbonk review for issues

james-elicx · 2026-07-01T06:11:14Z

/bigbonk review for issues

ask-bonk · 2026-07-01T06:13:55Z

`/bigbonk` review — PR #2464

Reviewed the diff end-to-end (shim scheduling, render-mode wiring, skip disposition, shell guard, and tests). Ran the targeted suites locally — all green:

vp test run tests/link-navigation.test.ts → 58 passed
vp test run tests/app-page-render.test.ts tests/app-page-route-wiring.test.ts → 95 passed
vp check on the 4 touched source files → clean

Overall this is a well-scoped, well-tested change. The follow-up shell guard from the second commit is correct and the Next.js parity story checks out. No blocking issues found. A few notes below, mostly for the record.

Correctness — verified good

Shell guard (link.tsx:593-600) is sound. On a failed/500 shell fetch, prefetchRscResponse deletes the shell entry, so shellCache.get(shellCacheKey) returns undefined → outcome !== "cache-seeded" → the guard throws → the dynamic fetchPromise rejects → deletePrefetchCacheEntry removes the dynamic cacheKey from both the cache and getPrefetchedUrls(), so a later hover can retry cleanly. The regression test (does not cache a dynamic-on-hover payload when its shell prefetch fails) locks this in.
Cross-client contamination is prevented. The new prefetch-dynamic-after-shell mode gets its own getRscRenderModeCacheVariant value and its own contribution to the cache-busting hash via VINEXT_RSC_RENDER_MODE_HEADER + Next-Router-Segment-Prefetch: /_tree. On top of that, when the skip disposition is enabled, shouldBypassRscCacheForSkipTransport forces no-store and skips the RSC cache capture entirely — so a layout-omitting payload can never be served to a client that didn't first fetch the shell.
Dev/prod parity holds. Render-mode parsing is centralized in app-rsc-request-normalization.ts (shared by the RSC entry across all environments), so the new mode flows through Pages/App dev, prod, and the Workers entry without per-server changes.

Notes / things worth confirming

createDynamicPrefetchAfterShellSkipDisposition reuses code: "SKIP_STATIC_LAYOUT_VERIFIED" but skips the cache-proof pipeline. Unlike createRenderLifecycleSkipDisposition, the new path marks every flag === "s" layout as skipped unconditionally — no crossCheckClientReuseManifestEntryWithCache, no rejectUnsafeStaticLayoutObservation. That's defensible here because the safety guarantee is different ("the client just fetched a shell containing these exact static layouts") rather than cache-proof-derived, and "s" classification already excludes dynamic-API/observation-dynamic layouts. But the shared SKIP_STATIC_LAYOUT_VERIFIED label now covers two different proof regimes. Consider a distinct code (or a comment on the function) documenting that this variant's "verification" is the shell-prefetch precondition, not the cache proof, so a future reader doesn't assume the rigorous checks ran.
Scheduling change is broader than the title suggests (intended + tested). Moving from "high-priority runs sync, everything else waits on requestIdleCallback" to "priority === "high" || hasAppNavigationRuntime() runs sync" means all App Router viewport prefetches now bypass idle scheduling (only the Pages Router <link rel=prefetch> fallback still defers). The new test starts App Router viewport prefetches before browser idle callbacks asserts this, and it matches Segment Cache behavior, so it looks deliberate — flagging only because it's a general App Router prefetch-timing change riding along with a dynamic-on-hover fix.
Shell-without-loading.tsx behavior is now "render the page (its own Suspense fallbacks)" instead of null. The app-page-route-wiring.tsx refactor (shouldRenderPrefetchLoadingShell = isPrefetchLoadingShell && routeLoadingComponent !== null) fixes a real bug: a shell prefetch of a route with no loading.tsx previously emitted an empty page slot. Emitting the page so its nearest <Suspense fallback> becomes the shell is the correct PPR/Segment-Cache semantics. Good catch; the renamed route-wiring test documents it well.

Nothing here is blocking. LGTM once you decide whether note (4)'s labeling deserves a comment.

ask-bonk · 2026-07-01T06:14:04Z

Review posted to PR #2464.

Summary

I reviewed PR #2464 (fix(app-router): align dynamic-on-hover prefetches) end-to-end and ran the targeted test suites locally — all green (58 + 95 tests, vp check clean on the 4 touched source files).

Verdict: no blocking issues. It's a well-scoped, well-tested change.

Key things I verified as correct:

Shell guard (link.tsx:593-600) — a failed shell fetch correctly rejects the dynamic prefetch and removes the key from both cache and getPrefetchedUrls(), so retries work. The regression test locks this in.
No cross-client contamination — the new prefetch-dynamic-after-shell mode gets a distinct cache variant + cache-busting hash contribution, and skip-transport forces no-store + skips cache capture, so layout-omitting payloads never reach a client that didn't fetch a shell.
Dev/prod parity — render-mode parsing is centralized, so the new mode flows through all servers.

Non-blocking notes raised:

createDynamicPrefetchAfterShellSkipDisposition reuses the SKIP_STATIC_LAYOUT_VERIFIED code but skips the rigorous cache-proof pipeline — safe here for a different reason (shell precondition), but worth a comment to avoid future confusion.
The scheduling change makes all App Router viewport prefetches bypass idle scheduling (intended + tested, just broader than the title implies).
The shell-without-loading.tsx fix (render page's own Suspense fallbacks instead of null) is a legitimate correctness improvement matching PPR/Segment Cache semantics.

github run

NathanDrake2406

Requesting changes on two structural issues: the Link prefetch mode now bypasses the existing route eligibility boundary, and the server skip disposition reuses a cache-proof contract for a different shell-delta invariant.

NathanDrake2406 · 2026-07-01T06:29:50Z

-            : mode === "full-after-shell"
-              ? { cacheForNavigation: true, prefetchShellFirst: true, shouldPrefetch: true }
-              : resolveFullAppRoutePrefetch();
+            : mode === "auto-shell"


This adds auto-shell as an unconditional prefetch plan, which bypasses the route-manifest eligibility check that normal auto gets through resolveAutoAppRoutePrefetch(). Since visible unstable_dynamicOnHover links are switched to this mode, any same-origin App-runtime link can now issue a shell RSC prefetch even when it has no __VINEXT_LINK_PREFETCH_ROUTES__ match. This is exactly the kind of mode-string branching that makes link.tsx harder to reason about. Can we push this back into a single typed prefetch-plan helper that always does route eligibility first, then chooses shell-only vs navigation-cache behavior for matched routes?

NathanDrake2406 · 2026-07-01T06:29:50Z

  return plan.skipDisposition;
 }

+function createDynamicPrefetchAfterShellSkipDisposition(input: {


This reuses ClientReuseManifestSkipDisposition and returns SKIP_STATIC_LAYOUT_VERIFIED directly from current layoutFlags, but this path is not the same invariant as the existing manifest/cache-proof verifier in skip-cache-proof.ts; it depends on the client having a fresh shell payload, not on server-side cache proof. Reusing the verified disposition muddies the boundary and makes later readers think the normal proof pipeline ran. Can we add an explicit shell-delta skip model/code, or move this behind a helper in the skip/cache-proof layer with a name that states the prerequisite?

…he-dynamic-on-hover-28478866791 # Conflicts: # packages/vinext/src/shims/link.tsx # tests/link-navigation.test.ts

…he-dynamic-on-hover-28478866791

…he-dynamic-on-hover-28478866791 # Conflicts: # tests/link-navigation.test.ts

…he-dynamic-on-hover-28478866791

james-elicx · 2026-07-01T13:25:17Z

Deploy-suite run 28478866791 / report job 84413308650 triage: this PR appears to cover the App Router hover-prefetch behavior row from test/e2e/app-dir/segment-cache/dynamic-on-hover/dynamic-on-hover.test.ts.

Exact row:

dynamic on hover > prefetches the dynamic data for a Link on hover

Since this PR is already James-owned, I am skipping duplicate implementation work for this row.

…866791 # Conflicts: # packages/vinext/src/plugins/rsc-reference-validation-normalizer.ts # packages/vinext/src/shims/link.tsx

github-actions · 2026-07-01T17:38:57Z

Performance benchmarks

Compared c04ae67 against base 239595e using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario	Framework	Baseline	Current	Change
Client bundle size (gzip)	vinext	125.6 KB	125.9 KB	⚫ +0.2%
Client entry size (gzip)	vinext	119.8 KB	119.9 KB	⚫ +0.1%
Dev server cold start	vinext	2.52 s	2.53 s	⚫ +0.5%
Production build time	vinext	3.05 s	3.03 s	⚫ -0.6%
RSC entry closure size (gzip)	vinext	96.8 KB	96.9 KB	⚫ +0.1%
Server bundle size (gzip)	vinext	163.2 KB	163.3 KB	⚫ +0.1%

View detailed results and traces

_{🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head}

james-elicx added 2 commits July 1, 2026 06:50

fix(app-router): align dynamic-on-hover prefetches

380af32

fix(link): require shell before dynamic-on-hover upgrade

76dfd04

james-elicx marked this pull request as ready for review July 1, 2026 06:17

NathanDrake2406 suggested changes Jul 1, 2026

View reviewed changes

james-elicx added 6 commits July 1, 2026 10:59

Merge remote-tracking branch 'origin/main' into codex/fix-segment-cac…

d985db6

…he-dynamic-on-hover-28478866791 # Conflicts: # packages/vinext/src/shims/link.tsx # tests/link-navigation.test.ts

Merge remote-tracking branch 'origin/main' into codex/fix-segment-cac…

f7e5aa6

…he-dynamic-on-hover-28478866791

Merge remote-tracking branch 'origin/main' into codex/fix-segment-cac…

e1a5865

…he-dynamic-on-hover-28478866791

Merge remote-tracking branch 'origin/main' into codex/fix-segment-cac…

7ec9a23

…he-dynamic-on-hover-28478866791 # Conflicts: # tests/link-navigation.test.ts

Merge remote-tracking branch 'origin/main' into codex/fix-segment-cac…

486b3a5

…he-dynamic-on-hover-28478866791

fix(app-router): normalize rsc reference validation ids

eafca0a

Merge origin/main into codex/fix-segment-cache-dynamic-on-hover-28478…

c04ae67

…866791 # Conflicts: # packages/vinext/src/plugins/rsc-reference-validation-normalizer.ts # packages/vinext/src/shims/link.tsx

Uh oh!

Conversation

james-elicx commented Jul 1, 2026

Summary

Next.js parity

Validation

Review

Uh oh!

james-elicx commented Jul 1, 2026

Uh oh!

pkg-pr-new Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 1, 2026

Uh oh!

ask-bonk Bot commented Jul 1, 2026

Uh oh!

ask-bonk Bot commented Jul 1, 2026

Uh oh!

james-elicx commented Jul 1, 2026

Uh oh!

james-elicx commented Jul 1, 2026

Uh oh!

ask-bonk Bot commented Jul 1, 2026

/bigbonk review — PR #2464

Correctness — verified good

Notes / things worth confirming

Uh oh!

ask-bonk Bot commented Jul 1, 2026

Summary

Uh oh!

NathanDrake2406 left a comment

Choose a reason for hiding this comment

Uh oh!

NathanDrake2406 Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

NathanDrake2406 Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

james-elicx commented Jul 1, 2026

Uh oh!

github-actions Bot commented Jul 1, 2026

Performance benchmarks

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pkg-pr-new Bot commented Jul 1, 2026 •

edited

Loading

`/bigbonk` review — PR #2464