fix(app-router): reuse shared loading state for searched routes

[james-elicx]

Summary

• reuse safe empty-search prefetches as the optimistic fallback for searched App Router navigations
• prevent no-store, hydration-seeded, or missing cache metadata snapshots from leaking stale server-search payloads
• add focused cache/navigation coverage and an e2e port for the shared loading-state scenario

Validation

• vp test run tests/prefetch-cache.test.ts tests/link-navigation.test.ts
• PLAYWRIGHT_PROJECT=app-router-prefetch-searchparams pnpm run test:e2e -- tests/e2e/app-router-prefetch-searchparams/search-params-shared-loading-state.spec.ts --workers=1 --retries=0
• REPO="/Users/jamesanderson/.codex/worktrees/search-params-shared-loading-state-28478866791/vinext" NEXTJS_DIR="/Users/jamesanderson/Developer/vinext/.nextjs-ref" ./scripts/run-targeted-nextjs-e2e.sh test/e2e/app-dir/segment-cache/search-params/segment-cache-search-params-shared-loading-state.test.ts

[james-elicx]

/bigbonk review for issues

[pkg-pr-new]

Open in StackBlitz

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

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

commit: 4093d8d

[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

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: shared loading state for searched routes

Read through the diff and ran vp check plus the two touched Vitest files (tests/prefetch-cache.test.ts, tests/link-navigation.test.ts) — all green. The core idea (reuse a safe empty-search prefetch as the optimistic fallback for a searched navigation, while gating on cache-control/no-store/hydration metadata) is sound and matches the Next.js segment-cache behavior. A few issues worth addressing.

1. Shared empty-search entry is deleted on the sync consume path but preserved on the async path (behavioral inconsistency)

The whole point of a shared loading state is that multiple searched navigations to the same pathname can reuse one empty-search prefetch. But the three consumption paths disagree on whether the shared entry survives:

• consumePrefetchResponseForNavigation pending → fallback branch preserves it (packages/vinext/src/shims/navigation.ts:1062, { deleteEntry: false }). ✅
• consumePrefetchResponse (sync) deletes it (navigation.ts:976 → consumePrefetchCacheEntryMatch with default delete). ❌
• consumePrefetchResponseForNavigation non-pending fall-through also deletes it (navigation.ts:1070, default delete) when the initial match is itself the empty-search fallback. ❌

The new tests actually codify this divergence: the sync test asserts cache.has(prefetchedRscUrl)).toBe(false) (tests/prefetch-cache.test.ts ~line 348) while the async pending-fallback test asserts .toBe(true) (~line 654). So a searched nav that lands on the non-pending path consumes-and-evicts the shared shell, and the next searched nav to the same pathname gets nothing to fall back on.

Recommendation: when match.isEmptySearchFallback === true, never delete the source entry (pass { deleteEntry: false }) on all consume paths, and update the sync test to assert the entry is retained. If deletion on the sync path is genuinely intended, please document why the two navigation paths differ.

2. Duplicated isNoStoreCacheControl helper

isNoStoreCacheControl is copy-pasted with identical bodies in packages/vinext/src/shims/navigation.ts:430 and packages/vinext/src/server/app-browser-entry.ts:438. Per AGENTS.md ("Split state from heavy implementations" / keep runtime logic in typed modules), export it once from navigation.ts (or a small shared util) and import it in the browser entry so the no-store parsing rule can't drift between the two.

3. hasPrefetchCacheEntryForNavigation can evict the shared entry as a side effect of a searched probe

With allowEmptySearchFallback: true, if the only match is an expired empty-search fallback, hasPrefetchCacheEntryForNavigation deletes it (navigation.ts:584). The production caller passes notifyInvalidation: false (app-browser-entry.ts:1789) so no callbacks fire, and an expired entry being swept is defensible — but a boolean "do I have a candidate?" probe eagerly mutating shared cache state for a different URL than the entry's own is surprising. Consider not deleting on the fallback path here (leave expiry sweeping to evictPrefetchCacheIfNeeded), or add a comment noting the intentional eviction.

Minor / confirmed-OK

• The opposite handling of optimisticRouteShell in canLearnOptimisticRouteTemplateFromPrefetch (returns true) vs canUseEmptySearchPrefetchFallbackEntry (returns false) is correct — learning wants shells, payload-reuse must exclude them. A one-line comment on the fallback check would help future readers.
• URL normalization (_rsc stripping before comparing search === "") correctly avoids matching prefetches that carried real search params. Good.
• Rewriting the returned snapshot url to the searched target while keeping the empty-search buffer is the intended shared-shell behavior and matches Next.js.

Nothing here is a blocker for the mechanism itself, but issue #1 undercuts the "shared" in "shared loading state" and should be resolved (or explicitly justified) before merge.

[ask-bonk]

Review posted to PR #2456. Summary of findings:

Review posted to PR #2456

Ran vp check and the two touched Vitest files — all green. The mechanism is sound and matches Next.js segment-cache behavior. Flagged three issues:

1. Shared entry deletion inconsistency (main concern) — The three consume paths disagree on whether the shared empty-search prefetch survives a searched navigation:

• consumePrefetchResponseForNavigation pending→fallback preserves it ({ deleteEntry: false })
• Sync consumePrefetchResponse deletes it
• Async non-pending fall-through also deletes it

The new tests actually codify this divergence (sync asserts cache.has(...) is false, async pending-fallback asserts true). This means a searched nav landing on the non-pending path evicts the shared shell, so the next searched nav to the same pathname gets no fallback — undercutting the "shared" in "shared loading state."

2. Duplicated isNoStoreCacheControl — identical copies in navigation.ts:430 and app-browser-entry.ts:438; should be exported once to avoid drift (per AGENTS.md guidance).

3. Side-effecting probe — hasPrefetchCacheEntryForNavigation can evict an expired shared fallback for a different URL than the entry's own; defensible but surprising, worth a comment or leaving eviction to the sweeper.

Also confirmed several things are correct (opposite optimisticRouteShell handling is intentional, _rsc URL normalization is right, snapshot URL rewrite is the intended shared-shell behavior). Issue #1 is the one that should be resolved or explicitly justified before merge; the rest are cleanups.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: shared loading state for searched routes

Read through the full diff, ran vp check on the three touched source files (clean) and the three touched Vitest files (tests/prefetch-cache.test.ts, tests/link-navigation.test.ts, tests/app-page-response.test.ts — 125 passing). The mechanism is sound: reuse a safe empty-search prefetch as the optimistic fallback for a searched navigation, gated on cache-control so no-store / hydration-seeded / metadata-less snapshots can't leak stale server-search payloads. This matches the Next.js segment-cache behavior the PR ports.

Correction to the prior /bigbonk review

The earlier review's two headline findings do not match the code on this branch, so I want to set the record straight to avoid churn:

1. "Shared entry deleted on the sync path" — not accurate. consumePrefetchCacheEntryMatch guards deletion with match.isEmptySearchFallback !== true (packages/vinext/src/shims/navigation.ts:994), so a shared empty-search fallback is never deleted on any consume path (sync or async). The sync test confirms this: it asserts expect(cache.has(prefetchedRscUrl)).toBe(true) (tests/prefetch-cache.test.ts ~line 348), not false. The three paths are consistent; the "shared shell gets evicted on the next nav" concern doesn't apply.

2. "Duplicated isNoStoreCacheControl" — not present. The helper is defined once and exported from navigation.ts:430, and app-browser-entry.ts imports it (app-browser-entry.ts:36) and uses it at :444. There is no second copy.

Both were flagged as blockers; neither is real on this diff.

Actual items worth a look

1. RSC vs HTML default-policy asymmetry (parity — please confirm intentional). The RSC policy fall-through changed from return {} to return { cacheControl: STATIC_CACHE_CONTROL, cacheState: "STATIC" } (app-page-response.ts:176). The corresponding HTML policy fall-through still returns { shouldWriteToCache: false } with no Cache-Control (app-page-response.ts:265). So for the same fully-static App Router page, the RSC response now advertises Cache-Control: s-maxage=31536000, stale-while-revalidate + x-vinext-cache-state: STATIC while the HTML response advertises no Cache-Control. This is needed for the fallback (the snapshot must carry a non-no-store cache-control to pass canUseEmptySearchPrefetchFallbackEntry), but it's a broader change than the PR title implies and it introduces an RSC/HTML header divergence on the default path. Please confirm this asymmetry is deliberate and matches Next.js (Next sends the same Cache-Control on the RSC and HTML variants of a static page), or align the HTML default.

2. Side-effecting has probe (minor, matches prior review's #3). With allowEmptySearchFallback: true, if the only candidate is an expired empty-search fallback, hasPrefetchCacheEntryForNavigation deletes it (navigation.ts:584) as a side effect of a boolean probe keyed on a different URL than the entry's own. The production caller passes notifyInvalidation: false (app-browser-entry.ts:1786) so no callbacks fire, and sweeping an expired entry is defensible — but a one-line comment noting the intentional eviction (or leaving expiry sweeping to evictPrefetchCacheIfNeeded) would prevent future confusion.

Confirmed correct

• canUseEmptySearchPrefetchFallback requires target.search !== "", so empty-search navigations never trigger the fallback path — no self-match risk.
• The opposite optimisticRouteShell handling (canLearnOptimisticRouteTemplateFromPrefetch returns true, canUseEmptySearchPrefetchFallbackEntry returns false) is right: learning wants shells, payload reuse must exclude them. A one-line comment on the fallback exclusion would help readers.
• _rsc stripping before comparing search === "" correctly avoids matching prefetches that carried real params.
• The searched visible link still prefetches its own searched RSC URL even when a shared shell exists (tests/link-navigation.test.ts "prefetches a searched visible link when only the no-search RSC URL is cached") — the shared shell is loading-state only, real data still fetches. Good.
• target-page-server-search reads searchParams (dynamic → no-store), so it's correctly excluded from fallback reuse, which is the point of the no-store gate.

No blockers on the mechanism. Item #1 (RSC/HTML default-policy asymmetry) is the one to confirm before merge; #2 is a cleanup.

[ask-bonk]

Review posted to PR #2456.

Summary

Reviewed the diff, ran vp check (clean) and the three touched Vitest files (125 tests passing). The mechanism is sound.

Key finding: the prior /bigbonk review's two headline "blocker" claims are wrong on this diff.

1. "Shared entry deleted on sync path" — false. consumePrefetchCacheEntryMatch guards deletion with match.isEmptySearchFallback !== true (navigation.ts:994), so the shared fallback is never deleted on any path. The sync test asserts cache.has(...) is true, not false as the prior review claimed.
2. "Duplicated isNoStoreCacheControl" — false. It's exported once from navigation.ts:430 and imported into app-browser-entry.ts:36. No duplicate exists.

Genuine items I flagged:

1. RSC/HTML default-policy asymmetry (the real, in-scope concern) — the RSC policy fall-through now returns STATIC cache-control while the HTML fall-through still returns none, so a static page's RSC and HTML responses advertise different Cache-Control. Needed for the fallback gate, but a broader change than the title implies; asked the author to confirm it's intentional and matches Next.js.
2. Side-effecting has probe — an expired fallback can be evicted during a boolean probe; minor, worth a comment.

Also confirmed several things are correct (empty-search self-match is impossible, opposite optimisticRouteShell handling is intentional, searched links still prefetch their own URL, no-store gating correctly excludes dynamic server-search pages).

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: reuse shared loading state for searched routes

Read through the full diff, ran vp check on the three touched source files (clean) and the two touched Vitest files (tests/prefetch-cache.test.ts, tests/link-navigation.test.ts — 97 passing). The mechanism is sound: reuse a safe empty-search prefetch as the optimistic fallback for a searched navigation, gated on cache-control so no-store / hydration-seeded / metadata-less snapshots can't leak stale server-search payloads. Matches the Next.js segment-cache behavior the PR ports.

Corrections to the earlier /bigbonk reviews

Both prior reviews posted headline "blocker" findings that are not accurate on this diff. Setting the record straight to avoid churn:

1. "Shared entry deleted on the sync path" (first review) — false. consumePrefetchCacheEntryMatch guards deletion with match.isEmptySearchFallback !== true (navigation.ts:1002), so a shared empty-search fallback is never deleted on any consume path. The sync test asserts expect(cache.has(prefetchedRscUrl)).toBe(true) (tests/prefetch-cache.test.ts ~L343), not false. The three consume paths are consistent.

2. "Duplicated isNoStoreCacheControl" (first review) — false. Defined and exported once in navigation.ts:431, imported into app-browser-entry.ts:36. No second copy.

3. "RSC vs HTML default-policy asymmetry in app-page-response.ts" (second review) — this file is not in the diff (git diff --name-only confirms it's untouched). That finding described a change that doesn't exist on this branch. The actual cache-control seeding happens client-side in bootstrapHydration (app-browser-entry.ts:1505), choosing HYDRATION_STATIC_CACHE_CONTROL vs HYDRATION_NO_STORE_CACHE_CONTROL off initialRscBootstrap?.initialCacheKind. There is no server-side RSC/HTML header divergence introduced here.

Actual items worth a look (all minor)

1. Probe/consume settled-state mismatch. hasPrefetchCacheEntryForNavigation({ allowEmptySearchFallback: true }) (used at app-browser-entry.ts:1782) reaches the fallback loop in findPrefetchCacheEntryForNavigation (navigation.ts:517), which does not require the fallback entry to be settled (pending === undefined && outcome === "cache-seeded"), whereas the actual consume via findSettledEmptySearchPrefetchFallbackForNavigation (navigation.ts:548) does. So the boolean probe can return true for a still-pending empty-search fallback, driving a consumePrefetch decision that then falls through (via await entry.pending on the fallback, or the fallbackReuseDecision path). It's handled and not incorrect, but the probe is more permissive than the consumer — a comment noting this, or aligning the probe's fallback eligibility with the settled check, would prevent future confusion.

2. Side-effecting has probe (both prior reviews flagged this consistently — it is real). With allowEmptySearchFallback: true, an expired empty-search fallback keyed on a different URL than the target gets deleted (navigation.ts:585) as a side effect of a boolean probe. Production passes notifyInvalidation: false so no callbacks fire, and sweeping an expired entry is defensible, but a one-line comment (or leaving expiry sweeping to evictPrefetchCacheIfNeeded) would clarify the intentional mutation.

3. One-line comments requested by both prior reviews still apply — the opposite optimisticRouteShell handling (canLearnOptimisticRouteTemplateFromPrefetch returns true, canUseEmptySearchPrefetchFallbackEntry returns false) is correct (learning wants shells; payload reuse must exclude them) but non-obvious; a short comment on the fallback exclusion at navigation.ts:437 would help.

Confirmed correct

• canUseEmptySearchPrefetchFallback requires target.search !== "" and source.search === "" (navigation.ts:424-427), so empty-search navigations can never self-match the fallback path — no self-match risk.
• _rsc cache-busting param is stripped before comparing search === "" (normalizeRscCacheLookupUrl), so prefetches that carried real search params are correctly excluded from fallback matching.
• Rewriting the returned snapshot url to the searched target (responseUrl) while keeping the empty-search buffer is the intended shared-shell behavior; navResponseUrl is threaded correctly at app-browser-entry.ts:1907.
• no-store gating (isNoStoreCacheControl) correctly excludes dynamic server-search pages; allowEmptySearchFallbackWithoutCacheControl is only opted in for full auto-prefetches (link.tsx:602), not learning-only shells.
• Searched visible links still prefetch their own searched RSC URL even when a shared no-search shell is cached (tests/link-navigation.test.ts "prefetches a searched visible link when only the no-search RSC URL is cached") — the shared shell is loading-state only; real data still fetches.
• .d.ts ambient types kept in sync with the runtime CachedRscResponse / PrefetchCacheEntry / options additions.

No blockers. The mechanism is correct and well-tested. Items above are comments/cleanups, not correctness fixes.

[ask-bonk]

Review posted to PR #2456.

Summary

Reviewed the full diff, ran vp check (clean) and both touched Vitest files (97 passing). The mechanism is sound and matches the Next.js segment-cache behavior being ported.

Key finding: all three headline "blocker" claims from the two prior /bigbonk reviews are wrong on this diff.

1. First review's "shared entry deleted on sync path" — false. Deletion is guarded by match.isEmptySearchFallback !== true (navigation.ts:1002); the sync test asserts cache.has(...) is true.
2. First review's "duplicated isNoStoreCacheControl" — false. Exported once, imported once.
3. Second review's "RSC/HTML asymmetry in app-page-response.ts" — false. That file isn't even in the diff. The second review hallucinated a change to a file that was never touched. Actual cache-control seeding happens client-side in bootstrapHydration.

Genuine (all minor) items I flagged:

• Probe/consume settled-state mismatch: the has probe's fallback loop doesn't require the entry to be settled, while the consumer does — permissive but handled.
• Side-effecting has probe evicting an expired fallback for a different URL (the one real item both prior reviews correctly identified) — defensible, worth a comment.
• Requested one-line comments on the non-obvious opposite optimisticRouteShell handling.

No blockers. The feature is correct and well-tested; remaining items are cleanups. I also confirmed several correctness properties (no self-match risk, _rsc normalization, snapshot URL rewrite, no-store gating, searched links still prefetch their own URL).

github run

[james-elicx]

This covers the run 28478866791 / job 84413308650 failure for test/e2e/app-dir/segment-cache/search-params/segment-cache-search-params-shared-loading-state.test.ts (if there is no matching prefetch entry for a page with particular search params, optimistically reuse a prefetch entry with the same pathname and different search params). The PR body already records the exact upstream targeted run for that suite, so I am skipping a duplicate backlog fix for the shared-loading-state row.

[james-elicx]

Coordinator backlog mapping for GitHub Actions run 28478866791 / job 84413308650: this PR appears to own the remaining App Router search-param shared-loading-state failures, especially test/e2e/app-dir/segment-cache/search-params/segment-cache-search-params-shared-loading-state.test.ts and related segment-cache-search-params.test.ts rows.

Per instruction I am skipping a duplicate implementation because this PR is by james-elicx. Remaining work looks like getting the red E2E (app-router-prefetch-searchparams) gate green and attaching exact targeted wrapper proof.

[james-elicx]

Coordinator mapping for deploy-suite run 31290819291: this PR appears to fix the scoped failure in test/e2e/app-dir/segment-cache/search-params/segment-cache-search-params-shared-loading-state.test.ts (if there is no matching prefetch entry for a page with particular search params, optimistically reuse a prefetch entry with the same pathname and different search params). Per the backlog rules I am skipping a duplicate implementation because this PR is by james-elicx.