fix(app-router): match current navigation identity - #2876
Conversation
|
/bigbonk review for issues |
commit: |
|
Performance benchmarksCompared 1 improved · 1 regressed · 4 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(app-router): match current navigation identity
Reviewed all 18 changed files, traced the runtime seams end-to-end, and re-ran the affected unit suites (navigation-planner-early-intent, shims, app-browser-entry, prefetch-cache — 1615+313 assertions passing) plus vp check on the five modified source files (format/lint/types clean).
Verdict: NO BLOCKING ISSUES — approve
The three fixes are correct, well-scoped, and match Next.js behavior. Details below.
1. Soft push — departed-snapshot demotion (app-browser-entry.ts, navigation.ts)
restoreHistoryStateSnapshot now advances clientNavigationCacheGeneration before clearVisitedResponseCache() + disableNavigationResponsePrefetchCacheReuse(). Verified this ordering is load-bearing: both async publication sites (app-browser-entry.ts:2326 and :2350) gate on navigationCacheGeneration !== clientNavigationCacheGeneration and bail, so an in-flight storeVisitedResponseSnapshot that captured the old generation cannot repopulate the departed response after the maps are cleared. The comment accurately documents this.
disableNavigationResponsePrefetchCacheReuse keys off prefetchKind === undefined, which correctly targets only visited-response snapshots seeded via seedPrefetchResponseSnapshot (navigation.ts:1133, no prefetchKind) while leaving explicit Link/router prefetches (prefetchKind defaults to "navigation"/"loading-shell"/"route-tree") consumable. Setting cacheForNavigation = false causes consumePrefetchResponse/findPrefetchCacheEntryForNavigation (navigation.ts:1586, :1644) to skip the entry, while the separate optimisticRouteTemplateSources set (app-browser-entry.ts:241) still exposes it as an optimistic template — consistent with the docstring. The link.tsx cacheForNavigation reads are on the prefetch policy object, not cache entries, so demotion does not interfere with shell prefetch logic.
2. Soft replace + basePath/hash/query identity (navigation-planner.ts, navigation.ts)
The new currentUrlSpace discriminator cleanly fixes the basePath double-strip: an app-relative snapshot pathname is never re-stripped, so an app route whose first segment equals basePath (fixture app/docs/foo under basePath /docs) keeps its real segment. The appRelativeSnapshot vs browser split is applied asymmetrically and correctly (current side respects the flag; target side is always browser-space).
The exact-identity branch uses raw current.search === next.search && current.hash === next.hash → samePageRefresh (flight, bypass cache), which correctly diverges from the parsed sameSearch used for hash/search classification. This preserves %20 vs + as distinct spellings for exact identity (covered by the "keeps %20 and + as distinct raw spellings" test), while equivalent encodings with an unchanged hash fall through to an ordinary flight — the intended semantics. Adding search to ClientNavigationRenderSnapshot and simplifying createSnapshotPathAndSearch to pathname + search is the right way to preserve raw spelling that ReadonlyURLSearchParams would canonicalize away.
Hash removal now classifies as sameDocumentScroll with hash: ""; verified scrollToHashTarget("") scrolls to top (hash-scroll.ts:14), matching browser same-document behavior for #foo → no-hash. Good parity fix.
3. Server template seed identity (app-browser-visible-commit.ts)
hasSameServerTemplateSeedIdentity compares resolveAppPageRouteStateKey over the template's own treePath segments (treePath.split("/").filter(Boolean) — same idiom as app-page-element-builder.ts:767) rather than child-remount identity. This preserves the server template's rendered seed across child navigation while still remounting child template state, and the lane gate (navigation/traverse/server-action, requiring reuseCurrentBfcacheIds) correctly excludes refresh/HMR so a changed owner param still installs fresh output. navigationSnapshot is non-optional on both state and action, so no null-safety gap.
Tests & tooling
- E2E specs are properly ported from
test/e2e/app-dir/app/index.test.tswith source-linked comments (follows AGENTS.md). Fixtures usecrypto.randomUUID()render IDs to assert fresh-vs-replayed output — a good signal. playwright.config.ts: gating the basepath-i18n server behindVINEXT_BASEPATH_E2E_BASE_URLplus theserver != null(loose) filter correctly tolerates the newundefined. Minor nit (non-blocking): when the env var is set,use.baseURLstill hardcodeshttp://localhost:4196; if the intent is to point at an external server the baseURL won't follow. Fine if the override is only meant to reuse a locally-started server on the same port.
All required checks (Check, Vitest, Playwright E2E) are green.
|
Review posted to PR #2876. SummaryI reviewed all 18 changed files, traced the runtime logic end-to-end, ran the affected unit suites (all passing), and ran The PR correctly fixes three App Router navigation-identity bugs:
Tests are properly ported from the corresponding Next.js e2e file with source links. I flagged one non-blocking nit: when All required CI checks are green. |
|
Independent targeted Next.js E2E verification on immutable head
The checkout remained clean and the local/remote PR head SHA matched after the run. |
|
Independent full-diff review of immutable head I reviewed all 18 changed files independently of Big Bonk, covering App Router soft push/replace identity, history restoration, navigation/prefetch cache ownership, basePath and raw query/hash handling, server-template seed preservation, and the ported browser/unit coverage. Correctness/parity checks:
Independent validation on this exact SHA:
The local and remote heads still match, the worktree is clean, all checks are green, and the PR is mergeable. |
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
Review: fix(app-router): match current navigation identityReviewed all 20 changed files, traced the three fixes end-to-end, and re-ran the affected unit suites ( Verdict: no blocking issuesThe three navigation-identity fixes are correct and well-scoped. Notes below, plus the one non-blocking nit already surfaced. 1. Soft push — history-restore demotion (
|
|
Review posted to PR #2876. SummaryI reviewed all 20 changed files, traced the three fixes end-to-end, and re-ran the affected unit suites (1631 assertions, all passing) plus The PR correctly fixes three App Router navigation-identity bugs:
I noted two non-regression items and one non-blocking nit:
|
1 participant
Summary
Fix the three non-cache
test/e2e/app-dir/app/index.test.tsfailures recorded by deploy-suite run 31439707085 / test report job 93624401572. The archived report recorded 100 passed, 3 failed, and 5 skipped for this suite.Failure mapping:
<Link /> > should soft push: a new push after back could replay the departed visited-response snapshot instead of fetching and committing the current server output. Departed snapshots are now demoted after history restoration.<Link /> > should soft replace: an exact-current Link could reuse the response that produced the page already on screen. Exact raw URL identity now refreshes page segments without adding history, including basePath, hash, and encoded-query cases.template component > should render the template that is a server component and rerender on navigation: server template output followed child remount identity, replacing the template seed during child navigation. Template seed identity is now derived from parameters owned by the template boundary while child template state still remounts.The navigation planner now makes browser-space versus already app-relative snapshot URLs explicit, preserves raw query spelling (
%20versus+), handles basePath prefix collisions, refreshes an unchanged hash, and treats a changed or removed hash as same-document navigation.Validation
v16.2.6(ee6e79b1792a4d401ddf2480f40a83549fe8e722):REPO="$(pwd)" NEXTJS_DIR="/Users/jamesanderson/Developer/vinext/.nextjs-ref" ./scripts/run-targeted-nextjs-e2e.sh test/e2e/app-dir/app/index.test.ts%20identity.git diff --checkpassed.This PR intentionally targets only the three non-cache failures above.