Skip to content

fix(app-router): align navigation prefetch commits - #2104

Closed
NathanDrake2406 wants to merge 7 commits into
cloudflare:mainfrom
NathanDrake2406:nathan/navigation-upstream-parity
Closed

NathanDrake2406 wants to merge 7 commits into
cloudflare:mainfrom
NathanDrake2406:nathan/navigation-upstream-parity

Conversation

@NathanDrake2406

Copy link
Copy Markdown
Contributor

Summary

This ports the relevant behaviour from Next.js's test/e2e/app-dir/navigation/navigation.test.ts and fixes the App Router navigation lifecycle issues that surfaced in the deploy harness:

  • keep optimistic/loading-shell prefetch payloads out of final navigation consumption, even if a later Link prefetch attempts to promote the cache entry
  • omit destination metadata from loading-shell prefetch payloads so a prefetched loading boundary cannot update the document title before the real navigation content commits
  • render the final authoritative payload against the latest router state after the optimistic shell has mounted, avoiding stale-state no-ops
  • resolve same-path search+hash navigations when the visible tree is unchanged, so URL and hash-scroll side effects still complete
  • route App-to-Pages/document navigations through the App Router's MPA scheduler before falling back to a hard browser navigation

Upstream references

Test notes

The local Playwright port keeps the same observable contracts as the upstream file. The hash fixture uses the same link structure and RSC-request assertions; target scroll positions are computed from the rendered fixture because app-basic has a different root/template offset than Next.js's upstream fixture. The async metadata fixture preserves the 5s metadata delay and fallback/content/title assertions. The upstream deploy-suite run remains the exact file-scope parity check.

Verification

  • vp test run tests/app-browser-entry.test.ts -t "same-path search and hash|stays pending until NavigationCommitSignal|authoritative payload"
  • PLAYWRIGHT_PROJECT=app-router pnpm exec playwright test tests/e2e/app-router/nextjs-compat/navigation.spec.ts --grep "hash|navigating to a page with async metadata"
  • vp test run tests/app-page-route-wiring.test.ts tests/prefetch-cache.test.ts tests/app-browser-entry.test.ts tests/link-navigation.test.ts tests/link.test.ts tests/navigation-planner-prefetch-reuse.test.ts
  • vp check
  • vp env exec --node 24 ./scripts/run-nextjs-deploy-suite.sh /Users/nathan/Projects/vinext/.refs/nextjs-v16.2.6 --retries 0 -c 1 --debug test/e2e/app-dir/navigation/navigation.test.ts

@pkg-pr-new

pkg-pr-new Bot commented Jun 17, 2026 •

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2104
npm i https://pkg.pr.new/create-vinext-app@2104
npm i https://pkg.pr.new/@vinext/types@2104
npm i https://pkg.pr.new/vinext@2104

commit: 72d2614

@NathanDrake2406
NathanDrake2406 marked this pull request as ready for review June 17, 2026 09:28
# Conflicts:
#	packages/vinext/src/server/app-page-element-builder.ts
#	packages/vinext/src/shims/link.tsx
@github-actions

github-actions Bot commented Jun 23, 2026 •

Copy link
Copy Markdown
Contributor

Performance benchmarks

Compared 72d2614 against base 5b6d0c8 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 131.5 KB 131.9 KB ⚫ +0.3%
Client entry size (gzip) vinext 118.9 KB 119.3 KB ⚫ +0.4%
Dev server cold start vinext 2.19 s 2.17 s ⚫ -0.9%
Production build time vinext 2.30 s 2.29 s ⚫ -0.1%
RSC entry closure size (gzip) vinext 102.4 KB 102.9 KB ⚫ +0.5%
Server bundle size (gzip) vinext 176.1 KB 177.1 KB ⚫ +0.6%

View detailed results and traces

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

# Conflicts:
#	packages/vinext/src/server/app-browser-entry.ts
#	packages/vinext/src/server/app-browser-navigation-controller.ts
#	packages/vinext/src/server/app-browser-state.ts
@james-elicx

Copy link
Copy Markdown
Member

Mapping this PR to App/Pages functional failures from run 28478866791 / job 84413308650: test/e2e/app-dir/navigation/navigation.test.ts — app dir - navigation > navigating to a page with async metadata > shows a fallback when prefetch completed. This looks like it covers those backlog rows, so I'm not opening a duplicate fix.

# Conflicts:
#	packages/vinext/src/shims/navigation.ts
#	tests/prefetch-cache.test.ts
@james-elicx

Copy link
Copy Markdown
Member

This maps to three remaining functional rows from compatibility run 29871622126 / job 88775791401 in test/e2e/app-dir/navigation/navigation.test.ts: hash navigation issues an unnecessary query-param RSC request, hash-with-scroll-offset lands at the wrong offset, and relative ?query#hash navigation drops the query.

I re-ran the exact targeted Next.js v16.2.6 suite on the current-main campaign baseline (3d30a3ca3): 46/50 pass and those same three hash/query assertions still fail, alongside the async-metadata fallback assertion. Existing james-elicx PR #2446 owns the async-metadata row; this PR appears to own the hash/query lifecycle cluster. I am adding it to the revival backlog and will keep the scopes separated when refreshing.

@james-elicx

Copy link
Copy Markdown
Member

I refreshed the currently failing functional subset against main in a scoped same-repo replacement: #2669.

The exact test/e2e/app-dir/navigation/navigation.test.ts runner is now 49/50: all hash/query/scroll cases owned here pass, and the only remaining failure is the separately owned #2446 async-metadata case. Focused vinext tests are 298/298 and targeted checks are clean. #2669 avoids carrying the older branch’s unrelated navigation/cache changes.

…ream merge

Upstream now owns both behaviors this branch previously patched:

- createPendingNavigationCommit baselines on the navigation initiation
  state (cloudflare#2609), which is what lets an authoritative payload replace a
  stale cross-param optimistic layout. The payload-ready live re-read
  re-introduced exactly that staleness, so the currentStateTiming /
  getCurrentStateAfterElementsReady plumbing is removed.
- Loading-shell prefetch responses intentionally stream generated
  metadata so rewrites can reuse it without a second request (cloudflare#2455,
  cloudflare#2318). Stripping resolvedMetadata / streamingMetadata from shell
  renders broke that protocol; the element-builder now matches upstream.
  Keeping shell payloads out of navigation consumption remains handled
  client-side via isPrefetchCacheEntryConsumableForNavigation.

Tests asserting the old behaviors are removed or aligned with the
upstream contract (authoritative commits declare navigationCommitKind).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants