fix(app-router): schedule segment-cache prefetch phases#2465
fix(app-router): schedule segment-cache prefetch phases#2465james-elicx wants to merge 18 commits into
Conversation
|
/bigbonk review for issues |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: segment-cache prefetch scheduling
Reviewed the client-side scheduler in link.tsx and the two new tests. Overall the change is well-structured and the phase/priority model is coherent. Tests pass and vp check is clean. A few observations, one of which is a real (latent) correctness concern.
1. Scheduler is not idempotent for in-flight tasks (latent double-scheduling)
processLinkPrefetchQueue removes a task from the queue before running it (removeLinkPrefetchTask sets task.queued = false), but the task object is still referenced by instance.scheduledTask. If scheduleSegmentCacheLinkPrefetch is called again while that task is in-flight (e.g. hover fires during a pending route-tree fetch — exactly the scenario in the second new test), pushLinkPrefetchTask re-queues the same task object (since queued === false), and the queue can invoke prefetchUrl for it a second time concurrently. scheduledLinkPrefetchInProgress then gets incremented twice for a single logical task, and two finally handlers run.
Today this is masked by dedup inside prefetchUrl (if (prefetched.has(cacheKey)) awaits the existing pending and returns), so no duplicate network request is issued and the tests pass. But the scheduler correctness relies on that downstream dedup rather than being idempotent itself. This is fragile:
scheduledLinkPrefetchInProgresscan temporarily overcount, affecting bandwidth gating for unrelated tasks.- Any future change to the dedup behavior in
prefetchUrlwould surface a real double-fetch.
Suggestion: add an explicit "in-flight" guard on the task (e.g. a running flag set before prefetchUrl and cleared in finally) and skip pushLinkPrefetchTask/re-run while running, deferring the reschedule to the existing finally re-queue.
2. Hover re-schedule resets a completed task back to route-tree phase
In scheduleSegmentCacheLinkPrefetch, the reuse branch unconditionally sets task.phase = "route-tree" (line 858). After a link has fully completed both phases, instance.scheduledTask is never nulled and its phase stays "segment". A subsequent hover (scheduleSegmentCacheLinkPrefetch(instance, "high")) therefore resets the finished task to "route-tree" and re-queues it, re-running the route-tree phase before the segment phase.
As with (1), prefetchUrl dedup makes this a no-op on the wire, so it's not a functional bug — just a wasted scheduler cycle. But it diverges from the intent (a hover on an already-prefetched link should bump priority, not restart from the tree). Consider only resetting to "route-tree" when the prior task hadn't completed the tree phase yet. There's no test covering hover-after-full-completion, so this path is currently unverified.
3. /_page segment-prefetch value is a vinext invention (documented divergence — confirm intent)
Next.js uses /_tree for the route-tree request (matches this PR) but uses actual encoded segment paths for segment requests — there is no /_page key in Next.js. Since vinext keeps a unified route payload (per the comment at the fetch-promise gate) rather than true per-segment fetching, /_page is only used as a distinct cache-busting marker and the vinext server doesn't branch on it. That's an acceptable deliberate divergence, but per AGENTS.md it should be a documented decision. Recommend a short code comment noting /_page is a vinext-specific marker (not a Next.js segment key) so a future reader doesn't assume parity.
Minor / non-blocking
- The new
await entry?.pendingat the end of the app-prefetch path now applies to all app modes, not just the scheduler. All non-scheduler callers usevoid prefetchUrl(...), so the later resolution is harmless — noting for the record. hasLinkPrefetchBandwidthonly batch-gatesroute-treelow-priority tasks against queued earlier batches; an in-flight earlier-batch task (removed from queue) does not gate later batches. Likely fine, but worth a sanity check against the Next.js scheduling contract if strict inter-batch ordering matters.
Verification
tests/link-navigation.test.ts— 56 passedvp check packages/vinext/src/shims/link.tsx— clean (format, lint, types)
None of these are blocking; (1) and (2) are the ones worth addressing to make the scheduler self-consistent rather than relying on downstream dedup.
|
Review posted to PR #2465. Summary of findings: Review completeI reviewed the segment-cache prefetch scheduler in 1. Scheduler not idempotent for in-flight tasks (latent correctness issue) — When 2. Hover resets a completed task back to 3. Plus two minor non-blocking notes on the broadened None are blocking; (1) and (2) are worth addressing so the scheduler is self-consistent rather than depending on |
|
/bigbonk review for issues |
james-elicx
force-pushed
the
codex/fix-segment-cache-prefetch-scheduling-28478866791
branch
from
0333a84 to
09d0a64
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: segment-cache prefetch scheduling
Re-reviewed the scheduler in link.tsx and the two new tests. Tests pass (56/56) and the phase/priority model is coherent. I confirmed the two prior findings and found one additional behavioral regression that affects all App Router apps, not just cache-components apps. Details below.
1. Concurrent re-run of an in-flight task is a real double-fetch, not just an overcount
The prior review flagged this as "latent, masked by prefetchUrl dedup." After tracing the await ordering, the dedup does not reliably mask it, so this is a genuine duplicate network request under the exact scenario the second new test exercises (hover during a pending route-tree fetch).
Sequence:
processLinkPrefetchQueuedequeues a task (removeLinkPrefetchTask→queued = false), setsscheduledLinkPrefetchInProgress++, and callsprefetchUrl(route-tree phase, still in-flight).instance.scheduledTaskstill points at this task.- A hover fires →
scheduleSegmentCacheLinkPrefetch(instance, "high"). Sinceinstance.scheduledTask !== null, the reuse branch runstask.phase = "route-tree"andpushLinkPrefetchTask(task)— re-queuing the same task object becausequeuedisfalse. - The queue runs it a second time concurrently →
scheduledLinkPrefetchInProgress++again and a secondprefetchUrlcall.
Why dedup does not save us: inside runPrefetch, the guard prefetched.add(cacheKey) (link.tsx:548) is reached only after await Promise.all([...dynamic imports]) and await createRscRequestUrl(...) (link.tsx:525, which is async). Both concurrent invocations can pass if (prefetched.has(cacheKey)) (link.tsx:528) before either reaches line 548, so both issue a real fetch. The prefetched set is not a synchronous mutex across the await boundary.
Consequences:
- Duplicate route-tree network request for the same link.
scheduledLinkPrefetchInProgressis incremented twice and decremented twice; if the twofinallyhandlers interleave with a cancel/visibility change it can drift (including negative), loosening the bandwidth gate for unrelated tasks.
Fix: add an explicit in-flight guard on the task (e.g. a running flag set before prefetchUrl and cleared in the finally), and in both scheduleSegmentCacheLinkPrefetch and pushLinkPrefetchTask refuse to re-queue a running task — defer the reschedule to the existing finally re-queue. Relying on downstream dedup that sits behind two awaits is not safe.
2. App Router low-priority viewport prefetches lost requestIdleCallback deferral (regression for non-cache-components apps)
This is the item I want to flag most strongly because it changes behavior for every App Router app, not just cache-components apps.
On main, all low-priority prefetches (App and Pages) were deferred via requestIdleCallback (or a setTimeout(fn, 100) fallback) "to avoid blocking the main thread during initial page load" — that is still what the docstring at link.tsx:404-407 describes.
This PR changes the gate to:
if (priority === "high" || hasAppNavigationRuntime()) {
return runPrefetch();
}hasAppNavigationRuntime() is true for any App Router app. So a regular App Router app with cache components off now runs its low-priority viewport prefetches immediately instead of during idle time. The segment-cache scheduler (usesSegmentCachePrefetchScheduler) only kicks in when __NEXT_CACHE_COMPONENTS === "true"; for everyone else, setVisibleLinkPrefetch still calls void prefetchUrl(instance.href, instance.mode, "low", ...) directly (link.tsx:927), which now bypasses idle scheduling entirely.
If the intent is to only start immediately when the scheduler is orchestrating timing, this gate is too broad. Consider narrowing it so non-scheduler App Router low-priority prefetches keep the idle deferral (e.g. gate on the scheduler/priority === "high" rather than on hasAppNavigationRuntime()), or confirm and document that removing idle deferral for all App Router viewport prefetches is intentional and matches Next.js. This should be a deliberate, documented decision per AGENTS.md rather than a side effect of the scheduler change.
3. Hover resets a completed task back to route-tree phase (untested path)
scheduleSegmentCacheLinkPrefetch's reuse branch unconditionally sets task.phase = "route-tree" (link.tsx:858). After a link fully completes both phases, instance.scheduledTask is never nulled and its phase stays "segment". A later hover therefore restarts the finished task from the tree phase and re-queues it. Combined with finding #1's await-window, this is not merely a wasted cycle — it can also re-issue the route-tree fetch. Only reset to "route-tree" when the prior task had not yet completed the tree phase, and null instance.scheduledTask (or mark it done) on completion. No test covers hover-after-full-completion.
4. /_page is a vinext-specific marker — needs a code comment
Next.js uses /_tree for the route-tree request (this PR matches) but real encoded segment paths for segment requests; there is no /_page key in Next.js. Since vinext keeps a unified route payload, /_page (link.tsx:521) is only a cache-busting marker and the server doesn't branch on it. That's an acceptable deliberate divergence, but per AGENTS.md it should carry a short code comment noting it is a vinext-specific marker, not a Next.js segment key.
Minor / non-blocking
- Bot handling: bots are correctly skipped for network fetches (link.tsx:449, and
prefetchOnIntentat 1298), but the scheduler still enqueues tasks and incrementsscheduledLinkPrefetchInProgressfor bot user agents beforeprefetchUrlno-ops. Harmless, but the scheduler could short-circuit bot instances up front. hasLinkPrefetchBandwidthonly batch-gates low-priorityroute-treetasks against queued earlier batches; an in-flight earlier-batch task (already removed from the queue) does not gate later batches. Likely fine, but worth a sanity check against the Next.js scheduling contract if strict inter-batch ordering matters.
Verification
tests/link-navigation.test.ts— 56 passed- Reviewed against
maindiff to confirm the idle-scheduling change in #2 is a real behavioral delta.
Findings #1 and #2 are the ones I'd resolve before merge: #1 is a reproducible double-fetch under the PR's own second test scenario, and #2 changes main-thread scheduling for all App Router apps. #3 and #4 are cheap follow-ups worth doing in the same PR.
|
Review posted to PR #2465. Review summaryI re-reviewed the segment-cache prefetch scheduler ( 1. Concurrent re-run of an in-flight task is a real double-fetch (blocking) — The prior review called this "latent, masked by dedup," but the dedup does not reliably mask it. The 2. App Router lost 3. Hover resets a completed task to 4. Plus two minor non-blocking notes (bot enqueue waste, inter-batch gating). Findings #1 and #2 are worth resolving before merge. |
james-elicx
force-pushed
the
codex/fix-segment-cache-prefetch-scheduling-28478866791
branch
from
09d0a64 to
6b34a58
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: segment-cache prefetch scheduling (re-review)
Re-reviewed link.tsx and the four new tests. The prior findings all look addressed:
- #1 in-flight double-run —
pushLinkPrefetchTasknow guards!task.running && !task.completed(link.tsx:777) andscheduleSegmentCacheLinkPrefetchreturns early whentask.running(link.tsx:863). Therunningflag is set synchronously beforeprefetchUrland cleared infinally(link.tsx:909/922), so the same task can no longer be re-queued concurrently. - #2 idle-deferral regression — the gate is now scheduler-specific (
priority === "high" || mode === "route-tree" || mode === "segment", link.tsx:674) instead ofhasAppNavigationRuntime(), and there's a test asserting non-cache-components App viewport prefetches stay deferred until idle. - #3 completed-task reset —
scheduleSegmentCacheLinkPrefetchreturns early ontask.completed(link.tsx:869), with a "does not restart completed cacheComponents prefetches on hover" test. - #4
/_pagemarker — now carries a code comment (link.tsx:521-523).
Tests pass (40 passed / 18 skipped) and the targeted suite is green. One new correctness issue below.
1. canceled is never cleared when a link scrolls out then back in while its fetch is in flight (segment phase is dropped)
cancelScheduledSegmentCacheLinkPrefetch sets task.canceled = true (link.tsx:889) but does not stop an already-running phase. If the link returns to the viewport while that phase is still in flight, scheduleSegmentCacheLinkPrefetch hits the task.running early-return branch (link.tsx:863-868), which bumps priority and returns without resetting task.canceled. The canceled = false reset only happens in the not-running/not-completed branch (link.tsx:874), which this path skips.
Sequence:
- Link visible → route-tree phase starts →
running = true,canceled = false. - Link scrolls out →
setVisibleLinkPrefetch(instance, false)→cancelScheduledSegmentCacheLinkPrefetchsetscanceled = true,isVisible = false. Fetch still in flight. - Link scrolls back in while route-tree is still pending →
setVisibleLinkPrefetch(instance, true)setsisVisible = true→scheduleSegmentCacheLinkPrefetch(instance, "low")→task.runningis true → bumps priority, returns early,canceledstays true. - Route-tree fetch resolves →
finally(link.tsx:924) checks!currentTask.canceled— false — so it neither advances tosegmentnor re-queues. Task is left{ canceled: true, running: false, completed: false, queued: false }.
Result: the link is visible with a completed route-tree fetch but the segment phase never runs. The task is orphaned until an intent event (hover/touch) or a pingVisibleLinks call comes in — both take the not-running branch and reset canceled. Plain visibility flicker during the route-tree fetch permanently drops the segment prefetch for that link.
Fix: clear canceled when a still-running task is re-scheduled for a now-visible instance. In the task.running branch, add task.canceled = false (the running phase then re-queues itself in finally since !canceled && isVisible). Alternatively, in the finally re-queue check, treat "canceled but instance is visible again" as a reschedule rather than a drop. A test toggling visibility off→on while the route-tree fetch is pending (the stubIntersectionObserver harness already supports dispatchIntersectingEntry(anchor, false)) would cover this.
Minor / non-blocking
mostRecentIntentPrefetchTaskkeeps pointing at a task after it completes (only cleared on cancel, link.tsx:891-893). A later high-priority intent on a different link then sets the completed task'spriority = "low"viatrackIntentLinkPrefetchTask— harmless since the task is done, but the stale reference lingers until replaced or canceled. Consider nulling it when a task completes (link.tsx:928) for symmetry.
Verification
npx vitest run tests/link-navigation.test.ts -t "prefetch scheduling"— 40 passed / 18 skipped.
Finding #1 is the only one worth addressing before merge; it's a real (if narrow) dropped-prefetch under visibility flicker during an in-flight route-tree fetch. The prior review items are resolved.
|
Review posted to PR #2465. Review summaryI re-reviewed the segment-cache prefetch scheduler in The four prior findings are all resolved:
One new correctness issue (worth fixing before merge):
Plus one minor non-blocking nit: |
james-elicx
force-pushed
the
codex/fix-segment-cache-prefetch-scheduling-28478866791
branch
from
6b34a58 to
68ea4fe
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: segment-cache prefetch scheduling (re-review)
Re-reviewed link.tsx and the five new scheduler tests against the current tip (68ea4fe). vp check is clean and the full tests/link-navigation.test.ts suite passes (59/59). I traced the scheduler state machine carefully; all four prior findings are now resolved and correctly tested, and I did not find a new blocking issue. Details below.
Prior findings — all resolved
-
In-flight double-run — resolved.
pushLinkPrefetchTaskguards!task.queued && !task.running && !task.completed(link.tsx:777),processLinkPrefetchQueueskipstask.running(link.tsx:903), andscheduleSegmentCacheLinkPrefetchreturns early in thetask.runningbranch (link.tsx:863-869).runningis set synchronously beforeprefetchUrl(link.tsx:910) and cleared infinally(link.tsx:923), so a concurrent reschedule can no longer re-enqueue the same task.scheduledLinkPrefetchInProgressis incremented once (link.tsx:909) and decremented once in the always-runningfinally(link.tsx:924) — no counter drift or leak. -
Idle-deferral regression for non-cacheComponents apps — resolved. The gate is now scheduler-specific:
priority === "high" || mode === "route-tree" || mode === "segment"(link.tsx:674), replacing the too-broadhasAppNavigationRuntime(). The new test "keeps non-cacheComponents App viewport prefetches deferred until idle" asserts a regular App Router viewport prefetch goes throughrequestIdleCallbackand stayspriority: "low". -
Completed-task reset on hover — resolved.
scheduleSegmentCacheLinkPrefetchreturns early ontask.completed(link.tsx:870-872), covered by "does not restart completed cacheComponents prefetches on hover" (hover +pingVisibleLinks, asserts fetch count stays at 2).mostRecentIntentPrefetchTaskis also nulled on segment completion (link.tsx:930-932), closing the prior stale-reference nit. -
cancelednever cleared on visibility flicker during an in-flight fetch (last review's #1) — resolved. Thetask.runningbranch now resetstask.canceled = false(link.tsx:864), and the new test "continues cacheComponents segment prefetches when visibility returns during route-tree fetch" exercises exactly the off→on-during-pending sequence and asserts the segment phase still runs (2 fetches,/_pagemarker). I verified this fix also covers the same flicker during the segment phase. -
/_pagemarker — now carries the required explanatory code comment (link.tsx:521-523) per AGENTS.md.
Verified as correct during trace
- Phase advancement / re-queue: after a route-tree fetch resolves, the
finallysetsphase = "segment"and re-queues exactly once (link.tsx:925-927); thepushLinkPrefetchTaskguard prevents duplicates. - Intent priority handoff:
trackIntentLinkPrefetchTaskdemotes the previous most-recent intent task to"low"and the running task inherits the current priority for its next phase — matches "only the latest hover stays high priority." - Counter balance under cancel-while-running: cancel sets
canceled = truebut does not touch the counter; thefinallystill decrements. No drift.
Minor / non-blocking (unchanged from prior reviews)
- Bot tasks still enqueue in the scheduler. The bot guard exists on the intent path (link.tsx:1324) and inside
runPrefetch(link.tsx:449), so bots issue no network fetch — correct. But the viewport path (setVisibleLinkPrefetch→scheduleSegmentCacheLinkPrefetch) still creates a task and briefly incrementsscheduledLinkPrefetchInProgressbeforeprefetchUrlno-ops at link.tsx:449. Harmless (self-corrects on the nextfinally), but the scheduler could short-circuit bot instances up front to avoid the churn. hasLinkPrefetchBandwidthinter-batch gating only considers queued earlier-batch route-tree tasks (link.tsx:810-819); an already-running earlier-batch task does not gate later batches. Likely fine given the concurrency cap, but worth a sanity check against the Next.js scheduling contract if strict inter-batch ordering ever matters.
Verification
vp check packages/vinext/src/shims/link.tsx— clean (format, lint, types)tests/link-navigation.test.ts— 59 passed
No blocking issues. The scheduler is now self-consistent and no longer relies on downstream prefetchUrl dedup for correctness. The two minor notes are optional follow-ups.
|
Review posted to PR #2465. Review summaryRe-reviewed All four prior findings are resolved and correctly tested:
Plus the No blocking issues. The scheduler is now self-consistent and no longer relies on downstream |
|
This likely also covers the current run 28478866791/job 84413308650 failures in That suite is currently failing before vary-param validation: revealing the links starts no |
There was a problem hiding this comment.
Requesting changes on maintainability/structure. I agree the current behavior is much better than the earlier iterations and CI is green, but this implementation still lowers the architecture bar in a way we should fix before merging.
1. link.tsx is becoming the Segment Cache scheduler owner
packages/vinext/src/shims/link.tsx already owned a lot of public <Link> behavior, and this PR adds a full mutable scheduler directly into it: task lifecycle, batching, queue insertion/removal, bandwidth gates, phase transitions, cancellation, and intent-priority tracking (LinkPrefetchTask plus scheduleSegmentCacheLinkPrefetch / processLinkPrefetchQueue). The file grows from 1450 to 1718 lines.
This is not just a line-count concern. The new code turns a shim that should wire visibility/intent events into the canonical owner of a client-side scheduler. Next keeps this boundary cleaner: client/components/links.ts owns link visibility/intent wiring, while client/components/segment-cache/scheduler.ts owns the scheduler. Vinext does not need to copy that exact implementation, but we should keep the same ownership split.
Can we decompose this first? A good move is to extract a focused internal scheduler module, e.g. shims/internal/link-segment-prefetch-scheduler.ts, with a small API along these lines:
create/registera task for a visible linkcancela link taskreschedulewith low/high prioritypingvisible links through the existing runtime hook
Then link.tsx only creates the instance, handles observer/hover/unmount, and delegates. That makes the state machine testable without rendering a full <Link> and keeps future Segment Cache parity work out of the shim.
2. prefetchUrl now has a hidden awaitable scheduler contract
prefetchUrl now returns Promise<void> | undefined, and whether callers can rely on the promise depends on the priority/mode combination. The scheduler depends on "route-tree" and "segment" bypassing idle scheduling and awaiting the prefetch-cache entry, while normal viewport callers still treat prefetchUrl as fire-and-forget. That mixes two contracts in one helper.
This is a boundary smell. The public-ish link prefetch wrapper should stay fire-and-forget/idle-aware, while the scheduler should call an explicit promise-returning phase runner, for example:
prefetchUrl(...): existing idle/fire-and-forget behavior for Link callersrunAppPrefetchPhase({ href, phase, priority, pagesRouteHref }): immediate, awaitable scheduler phase contract
That would remove the mode-specific return-value trap and make the scheduler's dependency explicit instead of encoded through mode === "route-tree" || mode === "segment" inside prefetchUrl.
3. The task lifecycle is encoded as overlapping booleans
LinkPrefetchTask has canceled, completed, queued, and running, and the behavior depends on specific combinations across schedule/cancel/process/finally paths. The previous iterations already found bugs in these combinations: re-running in-flight tasks, completed task resets, and canceled-running visibility flicker. Those bugs are good evidence that the lifecycle is easy to get wrong.
If the scheduler is extracted, please also make this lifecycle more explicit there. A small status model or state-machine-style helper would be easier to reason about than open-coded boolean combinations in the Link shim. At minimum, the invariants should live inside the scheduler module so the rest of link.tsx cannot accidentally depend on or mutate them.
Suggested shape
Keep the behavior and the tests, but move the new scheduler out of link.tsx and give it an explicit promise-returning app-prefetch phase boundary. That should let this PR preserve the parity fix while avoiding another large, stateful subsystem being bolted into an already-large shim.
|
Targeted proof for run 28478866791 / job 84413308650: this update covers all 15 original failing rows in Command: 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/vary-params-base-dynamic/vary-params-base-dynamic.test.ts 2>&1 | tee /tmp/vinext-vary-params-base-dynamic-28478866791-final-proof.logResult: PASS Turbopack, 15 passed / 15 total, retry 0/0, exiting with code 0. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: segment-cache prefetch scheduling + concrete segment identity
Reviewed the full diff at tip (b78b5af), not just link.tsx. The PR has grown well past the original scheduler change — it now also adds concrete segment-prefetch cache identity, cacheLife stale propagation, a foreground stale-revalidate path, and an unrelated RSC reference-validation normalizer plugin. The scheduler state machine in link.tsx looks solid (all prior review findings are resolved and tests/link-navigation.test.ts passes 61/61). But there is a blocking problem: the PR does not pass vp check, so the CI Check job will fail.
1. BLOCKING — vp check fails with 3 type errors (CI Check job will be red)
npx vitest run passes because Vitest does not type-check, but vp check (which CI runs) reports 3 errors, all introduced by this PR:
a) packages/vinext/src/server/app-page-render.ts:367 — metadata.sourcePage is possibly null (TS18047)
const metadata = AppElementsWire.readMetadata(input.element);
if (metadata.sourcePage.includes("[")) { // sourcePage: string | null
return input.layoutFlags;
}readMetadata().sourcePage is typed string | null (app-elements-wire.ts:205, 862). This dereference has no null guard. Beyond the type error, the runtime behavior for a null sourcePage needs a deliberate decision: the guard exists to bail out of materialization when the source page is itself dynamic. If sourcePage is null you can't tell, so decide whether null should bail (safest — metadata.sourcePage?.includes("[") ?? true) or proceed with materialization (?? false), and encode that intent explicitly rather than crashing.
b) & c) tests/app-page-render.test.ts:1557 and :1601 — element object not assignable to Record<string, ReactNode> (TS2322)
Both new tests spread AppElementsWire.createMetadataEntries({ ..., sourcePage: "…/page" }) into the element object. Because createMetadataEntries includes the __sourcePage key (typed string | null) when sourcePage is passed, the resulting object literal is no longer assignable to Record<string, ReactNode>. The existing helper usage at line 235 doesn't hit this because it omits sourcePage. These need a cast (as Record<string, ReactNode>) or an adjusted element type in createRscOptions.
Please run vp check locally before pushing — the validation section in the PR description only lists vp check packages/vinext/src/shims/link.tsx tests/link-navigation.test.ts, which is why these three (in app-page-render.ts and app-page-render.test.ts) were missed.
2. Non-blocking — foreground stale-revalidate can render the page twice
In readAppPageCacheResponse (app-page-cache.ts:412-497), when shouldForegroundRevalidateStaleAppPage is true, renderFreshPageForCache() is awaited (line 414) and used to build the response. But if buildAppPageCachedResponse returns null (RSC request with no rscData, or HTML request where the fresh render produced empty HTML — e.g. a render that resolves to redirect/notFound), control falls through the if (response) block at 486 into the legacy scheduleBackgroundRegeneration path (line 506), which calls renderFreshPageForCache() again. That is a redundant second full render for the same request. It's not a correctness bug (the second render is background/deduped), but for stale === 0 routes it defeats the point of the foreground revalidate. Consider bailing to a normal fresh-render fall-through (or reusing the already-rendered artifact) when the foreground response comes back null, instead of re-entering the background regen path.
3. Non-blocking — scope creep: unrelated RSC reference-validation normalizer
packages/vinext/src/plugins/rsc-reference-validation-normalizer.ts (+74) and its wiring in index.ts are a dev-only fix for @vitejs/plugin-rsc client/server reference-key encoding mismatches. That's a legitimate change, but it is unrelated to segment-cache prefetch scheduling and has no dedicated test. Per the repo's small-PR guidance, this would be cleaner as its own PR with its own test; at minimum call it out in the PR description so it isn't lost in the "segment-cache" framing. (Same applies to the cacheLife stale plumbing across cache-handler.ts, cache-runtime.ts, isr-cache.ts, seed-cache.ts, prerender.ts, app-page-response.ts — it's coherent and tested, but it's a second feature riding along.)
Confirmed good
- Scheduler self-consistency — the
running/completed/queued/canceledguards inpushLinkPrefetchTask,processLinkPrefetchQueue, andscheduleSegmentCacheLinkPrefetchare internally consistent;scheduledLinkPrefetchInProgressis balanced (one increment, one always-firingfinallydecrement). The prior double-fetch, completed-reset, and visibility-flicker findings are all fixed and tested. - Idle-deferral gate (link.tsx:688-695) — I traced the
mainbaseline:mainran all App Router prefetches immediately (priority === "high" || hasAppNavigationRuntime()). The new gate only adds idle-deferral for the explicit__NEXT_CACHE_COMPONENTS === "false"case, so the earlier reviews' "idle regression" concern does not apply to the current code — this is a narrowing, not a regression. /_pagemarker — carries the required explanatory comment (link.tsx) noting it is a vinext-specific marker, not a Next.js segment key. Segment-prefetch fresh renders are intentionally never teed into the cache (app-page-render.ts:831), with a test to lock that in.- Segment cache key isolation (
appIsrRscKey) andmaterializeDynamicSegmentsInTextcatch-all/optional-catch-all regex both look correct and are covered by the ported vary-params tests.
Verification
vp check— FAILS (3 type errors above)tests/app-page-cache.test.ts,tests/isr-cache.test.ts,tests/link-navigation.test.ts— 161 passedtests/app-page-render.test.ts,tests/app-page-element-builder.test.ts,tests/app-page-route-wiring.test.ts,tests/seed-cache.test.ts— 169 passed
Finding #1 must be fixed before merge (it will block the Check job). #2 and #3 are worth addressing but not blocking.
|
Review posted to PR #2465. Review summaryI reviewed the full diff at tip ( 1. BLOCKING —
The PR's listed validation only ran 2. Non-blocking — foreground stale-revalidate can double-render. When 3. Non-blocking — scope creep. The RSC reference-validation normalizer plugin and the Confirmed good: scheduler counter balance, the Finding #1 must be fixed before merge. |
|
Latest-tip proof after the Command: 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/vary-params-base-dynamic/vary-params-base-dynamic.test.ts 2>&1 | tee /tmp/vinext-vary-params-base-dynamic-28478866791-final-proof-after-type-fix.logResult: PASS Turbopack, 15 passed / 15 total, retry 0/0, exiting with code 0. |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: segment-cache prefetch scheduling (re-review at tip 9744631)
Re-reviewed the full diff, not just link.tsx. The prior blocking issue is resolved and I found no new blocking issues. The scheduler state machine is self-consistent and well-tested; the stale/segment-key plumbing is coherent. Two carry-over non-blocking notes and one pending structural review remain.
Prior blocking issue — RESOLVED
The last review flagged 3 type errors failing vp check (app-page-render.ts:367 TS18047 + two TS2322 in app-page-render.test.ts). Commit 9744631 fixes them:
metadata.sourcePage?.includes("[")guards the null deref.- The two tests cast the element literal
as unknown as Record<string, ReactNode>.
I ran the full vp check — clean (2359 formatted, 1058 lint/type-checked, no errors). CI Check job should now be green.
Verified good
vp checkfull run — clean.- Test suites —
link-navigation.test.ts61/61;app-page-cache/app-page-render/isr-cache/seed-cache/app-page-element-builder/app-page-route-wiring269/269. - Scheduler self-consistency — the
queued/running/completed/canceledguards inpushLinkPrefetchTask(link.tsx:828),processLinkPrefetchQueue(link.tsx:976), andscheduleSegmentCacheLinkPrefetch(link.tsx:935-953) are internally consistent.scheduledLinkPrefetchInProgressis balanced: one increment (link.tsx:982) and one always-firingfinallydecrement (link.tsx:998). A running task is removed from the queue before it runs (link.tsx:975) and only re-queued afterrunning=false, so it cannot be double-picked. - All five prior scheduler findings (double-run, idle-deferral gate, completed-reset, visibility-flicker
canceledreset,/_pagemarker comment) are fixed and each has a dedicated test (link.tsx:1326–1594 in the test file). - Segment-key isolation is safe.
appIsrRscKeynow foldssegmentPrefetchPathinto the cache variant (isr-cache.ts:407-425), and segment-prefetch fresh renders are intentionally never teed into the cache (shouldCaptureRscForCacheMetadatagates on!segmentPrefetchPath, app-page-render.ts:831). Because segment-prefetch responses are never written, the finalizer's 4-argisrRscKeycall (finalizer.ts:153, no segment path) can never collide with the 5-arg read key (app-page-cache.ts:340). Consistent. cacheLifestaleplumbing is threaded end-to-end (prerender → manifest → seed →MemoryCacheHandler→isrSet→ response header) with backward-compatible optional args (setAppPageCacheEntryonly forwardsstaleSecondswhen defined). Covered by the newisr-cache,seed-cache, andapp-page-cachetests.- Segment-prefetch headers — both phases set
Next-Router-Prefetch: 1(link.tsx:518/521) and the/_pagemarker carries the required vinext-divergence comment (link.tsx:522-524) per AGENTS.md.
Non-blocking
1. null sourcePage now falls through to materialization — undertested deliberate decision. materializeConcreteLayoutFlags runs for every app render (app-page-render.ts:718), not just segment prefetches. With sourcePage?.includes("[") a null source page now proceeds to materialize dynamic layout-flag IDs. This is mostly safe because resolveAppPageConcreteRouteSegments keeps the symbolic segment when a param is missing (?? segment, app-page-segment-state.ts:142/148), so absent params can't be mis-baked. But the null-source-page + dynamic-layout-flags path has no test, and the choice of "materialize on null" vs "bail on null" (?? true) is a deliberate behavioral decision. Please add a focused test pinning the null-source-page behavior so a future refactor doesn't silently flip it.
2. Foreground stale-revalidate can render twice (carried over, still present). In readAppPageCacheResponse (app-page-cache.ts:412), when shouldForegroundRevalidateStaleAppPage is true, renderFreshPageForCache() is awaited (line 414). If buildAppPageCachedResponse returns null (RSC request with no rscData, or empty HTML from a redirect/notFound render), the if (response) at line 486 is skipped and control falls through to scheduleBackgroundRegeneration (line 506), which calls renderFreshPageForCache() a second time. Not a correctness bug (the second render is background/deduped) but it defeats the point of the foreground revalidate for stale === 0 routes. Consider bailing to a normal fresh-render fall-through (or reusing the artifact) when the foreground response is null.
3. Scope creep (carried over). The RSC reference-validation normalizer plugin (plugins/rsc-reference-validation-normalizer.ts, +74, dev-only, no dedicated test) and the cacheLife stale propagation are unrelated to "segment-cache prefetch scheduling." Both are coherent, but per the repo's small-PR guidance they'd be cleaner as separate PRs; at minimum call them out in the PR description. Minor robustness note on the normalizer: it locates the RSC plugin by the hard-coded name "rsc:minimal" (line 51); if that name changes upstream it silently no-ops (graceful, but a one-line comment or a warn-on-missing would help future debugging).
Outstanding structural review
@NathanDrake2406's "changes requested" review (maintainability — extract the scheduler out of link.tsx, give prefetchUrl an explicit promise-returning phase contract instead of the mode-dependent return-value trap, and model the task lifecycle explicitly rather than via overlapping booleans) is still unaddressed: link.tsx remains 1797 lines with the full scheduler inline, and prefetchUrl still returns Promise<void> | undefined with behavior keyed on mode === "route-tree" || "segment". That review is a merge blocker until resolved or explicitly waived by the maintainer.
Summary
No new blocking issues; the prior vp check blocker is fixed. Behavior looks correct and is well-tested. Recommend addressing note #1 (test the null-source-page decision) in this PR, and resolving the pending structural review before merge.
james-elicx
force-pushed
the
codex/fix-segment-cache-prefetch-scheduling-28478866791
branch
from
4c23523 to
f584994
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: segment-cache prefetch scheduling (re-review at tip f584994)
Re-reviewed the full diff at the current tip. The only change since the last reviewed head (4c23523) is commit f584994 ("preserve immediate app viewport prefetches"), which rewrites the idle-deferral gate in prefetchUrl. No blocking issues. vp type-check is clean (tsc --noEmit -p packages/vinext/tsconfig.json → exit 0) and the relevant suites pass (link-navigation 61/61, link-segment-prefetch-scheduler 5/5, app-page-cache/render/isr/seed 181/181).
The new gate (f584994) — verify the intended parity, add a comment
The gate is now:
if (
priority === "high" ||
mode === "route-tree" ||
mode === "segment" ||
(hasAppNavigationRuntime() && String(process.env.__NEXT_CACHE_COMPONENTS) !== "false")
) {
return runPrefetch();
}I traced the full history of this line to be sure what the net effect vs origin/main is, because earlier rounds went back and forth on whether the idle change is a regression:
main/0e63ab0:priority === "high" || hasAppNavigationRuntime()→ every App Router prefetch (any mode/priority) ran immediately; only Pages low-priority prefetches were idle-deferred.bb6a17c(mid-PR): droppedhasAppNavigationRuntime()entirely → all App Routerauto/low viewport prefetches started deferring to idle. This was the real behavioral delta earlier reviews called a "regression."f584994(current): re-addshasAppNavigationRuntime() && __NEXT_CACHE_COMPONENTS !== "false".
Net result at this tip, in a production build (where process.env.__NEXT_CACHE_COMPONENTS is DefinePlugin-inlined as a boolean per index.ts:1882-1884, so String(false) === "false"):
| App type | main |
this PR |
|---|---|---|
| cacheComponents on | immediate | immediate |
| cacheComponents off (default) | immediate | idle-deferred |
So a plain App Router app with cache components off — the common case — still has its low-priority viewport prefetches moved from immediate to requestIdleCallback, which is a behavior change from main. The new test at link-navigation.test.ts:1542 codifies this as intended (__NEXT_CACHE_COMPONENTS = "false" → deferred until idle). That may well be more Next.js-aligned (App Router does schedule prefetches through idle-time), but per AGENTS.md this should be a deliberate, documented decision rather than an artifact of iterating on the gate. Two asks:
- Confirm this against Next.js and state the intent — is deferring cacheComponents-off App Router viewport prefetches to idle the desired parity, or should the common case stay immediate as on
main? The.nextjs-refclone wasn't present in this environment so I couldn't diff their scheduler directly. - Add a short code comment on the gate explaining the
__NEXT_CACHE_COMPONENTS !== "false"clause and theString(...)wrap (which exists because the value is a build-time-inlined boolean, not a string). Without it, the mismatch between this gate's!== "false"andusesSegmentCachePrefetchScheduler's=== "true"(link.tsx:800) reads as an inconsistency to a future reader, even though both resolve correctly for the boolean define.
Verified good at this tip
- Extracted scheduler is self-consistent (
shims/internal/link-segment-prefetch-scheduler.ts). The singlestatusenum (idle/queued/running/running-canceled/completed) replaces the old overlapping booleans.inProgresshas exactly one increment (line 235) and one always-firingfinallydecrement (line 252); cancel-while-running flips torunning-canceledbut still hits thefinally— no counter leak. A running task can't be double-picked (schedulereturns early in the running branch;processQueuere-checksstatus !== "idle" || !isVisibleafter dequeue). The visibility-flicker recovery (arunning-canceledtask rescheduled while visible → back torunning→finallycontinues) is correct. - No stuck queue / infinite loop —
hasBandwidthgates oninProgress < limit;removeQueuedTaskruns before the skip guard;finallyre-arms viascheduleQueue(). - Concrete segment identity (
app-page-segment-state.ts) slice offsets are correct ([[...]]→5/-2,[...]→4/-1,[]→1/-1) and missing params keep the symbolic segment (?? segment), so absent params can't be mis-baked. - Segment-key isolation is safe — segment-prefetch renders are never captured/written (
!segmentPrefetchPathgate, app-page-render.ts), andappIsrRscKeyfolds the segment path into the read variant, so the finalizer's shorter write key can't collide with the read key. - Prior TS blocker stays fixed (
sourcePage?.includes("[")+ element-literal casts); full package type-check is clean. /_pagemarker carries the required vinext-divergence comment; both scheduler phases setNext-Router-Prefetch: 1.
Non-blocking (carried over, still valid)
- Foreground stale-revalidate can render twice. In
readAppPageCacheResponse(app-page-cache.ts:412), whenshouldForegroundRevalidateStaleAppPageis true,renderFreshPageForCache()is awaited (line 414); ifbuildAppPageCachedResponsereturns null (RSC with norscData, or empty HTML from a redirect/notFound render), theif (response)at line 486 is skipped and control falls through toscheduleBackgroundRegeneration(line 506) — a secondrenderFreshPageForCache(), then serves stale. Wasteful (and a minorstale: 0parity gap), not incorrect; mirrors legacymain. null-sourcePage→ materialization is an undertested deliberate decision.materializeConcreteLayoutFlagsusesmetadata.sourcePage?.includes("["); a null source page proceeds to materialize. Safe today (missing params keep symbolic segments) but "materialize on null" vs "bail on null" (?? true) has no pinning test. Recommend adding one.- Scope creep. The dev-only
rsc-reference-validation-normalizerplugin and thecacheLife stalepropagation are unrelated to "segment-cache prefetch scheduling." Both are now tested (rsc-reference-validation-normalizer.test.tswas added, +123), but per the repo's small-PR guidance they'd be cleaner as separate PRs — at minimum call them out in the description.
Summary
No new blocking issues; the extraction addresses @NathanDrake2406's structural review. The one item worth resolving in this PR is the f584994 gate: confirm (against Next.js) that deferring cacheComponents-off App Router viewport prefetches to idle is intentional, and add a code comment documenting the !== "false" boolean-define check per AGENTS.md.
|
Review posted to PR #2465. Review summaryRe-reviewed the full diff at tip Main finding — the new gate needs parity confirmation + a comment. I traced the full history of the gate line to settle the earlier "regression vs narrowing" debate:
Net effect in a prod build (where Verified good: extracted scheduler state machine is self-consistent (balanced Non-blocking carry-overs: foreground stale-revalidate double-render, undertested null- |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
james-elicx
force-pushed
the
codex/fix-segment-cache-prefetch-scheduling-28478866791
branch
from
bb27831 to
e172419
Compare
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: segment-cache prefetch scheduling (re-review at tip 20929ba)
Re-reviewed the full diff and the four new commits since the last reviewed head (f584994): 54055c7 (rewarm invalidated segment prefetch links), e1724197 (preserve app prefetch scheduling parity), 83bb70aa (restart dirty segment prefetch tasks), 20929ba (ping late app runtime links). No new blocking issues. vp check is clean on all changed files, the full package type-check passes (tsc --noEmit -p packages/vinext/tsconfig.json → exit 0), and every relevant suite is green (link-navigation 64/64, link-segment-prefetch-scheduler 7/7, app-page-render/cache/isr/seed 181/181).
The idle-deferral gate debate is now definitively closed — the PR matches main
I diffed origin/main directly. The f584994 gate that earlier rounds flagged (hasAppNavigationRuntime() && String(__NEXT_CACHE_COMPONENTS) !== "false") was reverted in e1724197. The gate at link.tsx:693-698 is now:
priority === "high" || mode === "route-tree" || mode === "segment" || hasAppNavigationRuntime()main was priority === "high" || hasAppNavigationRuntime(). The added mode === "route-tree" || "segment" clauses only ever apply to App Router scheduler phases (Pages Router never uses those modes), so the idle-deferral behavior is now functionally identical to main for every app type. The "idle regression" thread from prior rounds no longer applies and requires no code comment / Next.js parity confirmation — there is no delta to explain.
New running-dirty state + force re-warm — traced, correct
The new mechanism lets invalidatePrefetchCache() re-warm visible links (navigation.ts:679 → pingVisibleLinks({ force: true })). I traced the added state machine:
schedule(..., { force: true })on arunning/running-canceled/running-dirtytask (scheduler.ts:189-205) flips torunning-dirty, re-arms batch/phase/sortId, and returns. Thefinally(scheduler.ts:266-273) seesrunning-dirty+ visible → resetsphase = "route-tree"and re-enqueues, so the in-flight phase completes then the task restarts from the tree phase. Covered by the two new "restarts from route-tree after forced invalidation while route-tree/segment is running" tests.forceon acompletedtask (scheduler.ts:207) falls through to the reset+enqueue path — covered by "restarts a completed task only when forced."- Counter balance holds: a
running-dirtytask still hits the singlefinallydecrement; no double-increment because the force branch returns without re-entering the queue while running. cancelon arunning-dirtytask (scheduler.ts:230) →running-canceled, which makes bothshouldRestartandshouldContinuefalse infinally, so it drops cleanly. No leak.- Non-visible dirty task drops in
finally(both guards requireisVisible), correctly avoiding re-warm of off-screen links. - Non-segment-cache app links re-warm correctly too:
invalidatePrefetchCacheclears theprefetchedset before pinging, anddrainVisibleAppPrefetchQueueresetsqueuedViewportPrefetch, soprefetchUrlre-fetches on the next drain. Theforceflag is only needed for the scheduler path (which tracks task state independent of theprefetchedset) — a correct, deliberate split.
pingVisibleLinkPrefetches router-mode fix
The loop guard changed from instance.routerMode === "app" to resolveCurrentLinkPrefetchRouterMode(instance) === "app" (link.tsx:861), matching the guard already used in drainVisibleAppPrefetchQueue (link.tsx:767) and usesSegmentCachePrefetchScheduler (link.tsx:799). Consistent — resolves the live router mode rather than a stale snapshot.
Non-blocking (carried over, still valid)
- Foreground stale-revalidate can render twice.
readAppPageCacheResponse(app-page-cache.ts) awaitsrenderFreshPageForCache(); ifbuildAppPageCachedResponsereturns null (RSC with norscData, or empty HTML from a redirect/notFound render), control falls through toscheduleBackgroundRegeneration— a second render, then serves stale. Wasteful + minorstale: 0parity gap, not incorrect; mirrors legacymain. null-sourcePage→ materialization is an undertested deliberate decision.metadata.sourcePage?.includes("[")(app-page-render.ts:367) proceeds to materialize whensourcePageis null. Safe today (missing params keep symbolic segments), but "materialize on null" vs "bail on null" (?? true) still has no pinning test — recommend adding one in this PR.- Scope creep. The dev-only
rsc-reference-validation-normalizerplugin and thecacheLife stalepropagation remain unrelated to "segment-cache prefetch scheduling." Both are tested and coherent; per the repo's small-PR guidance they'd be cleaner as separate PRs — at minimum call them out in the description. - Minor stale reference: a
running-dirty(orrunning-canceled) task that drops because its instance went invisible does not nullmostRecentIntentTask(only cancel and completion do). Harmless — a stale intent reference is overwritten by the next high-priority intent — but noting for symmetry.
Summary
Behavior is correct and well-tested; the extracted scheduler cleanly absorbs the new re-warm/running-dirty logic. No new blocking issues at this tip, and the long-running idle-deferral concern is resolved by the revert in e1724197 (the gate now equals main). Recommend addressing note #2 (pin the null-source-page decision) in this PR. The structural review from @NathanDrake2406 is materially addressed (scheduler extracted, explicit runPhase contract, status-enum lifecycle).
|
Review posted to PR #2465. Review summaryRe-reviewed the full diff at the current tip Key findings:
Non-blocking (carried over): foreground stale-revalidate double-render, undertested null- The scheduler extraction materially addresses @NathanDrake2406's structural review. No blocking issues at this tip. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: segment-cache prefetch scheduling (re-review at tip 3fc7b5b)
Re-reviewed the diff since the last reviewed head (20929ba). The only new commit is 3fc7b5b ("preserve segment prefetch re-entry state"), which adds a running-canceled-dirty scheduler state and fixes a stale-router-mode short-circuit in setVisibleLinkPrefetch. No new blocking issues. vp check is clean on the four changed files, the full package type-check passes (tsc --noEmit -p packages/vinext/tsconfig.json → exit 0), and the suites pass (link-segment-prefetch-scheduler 8/8, link-navigation 66/66).
The new running-canceled-dirty state — traced, correct
The commit closes a real gap in the prior running-dirty mechanism. Before this, a force-invalidated in-flight task (running-dirty) that hit a visibility flap (cancel while invisible → back visible) lost its "restart from route-tree" intent, because cancel collapsed running-dirty → running-canceled and the subsequent re-entry only restored it to running (advance to segment), not running-dirty (restart from tree). I traced the full transition table for the four running states:
schedule(force)on any running state →running-canceled/running-canceled-dirtybecomerunning-canceled-dirty, elserunning-dirty(scheduler.ts:197-204). Re-arms batch/phase/sortId. Correct — a canceled task keeps its canceled bit until re-entry.- Non-force
schedule(visibility return) onrunning-canceled-dirty→running-dirty(scheduler.ts:205-206). Un-cancels while preserving the dirty restart intent — this is the exact fix. - Non-force
scheduleonrunning-canceled→running(scheduler.ts:207-209). Pre-existing visibility-flicker recovery, unchanged. cancelonrunning-dirty→running-canceled-dirty(scheduler.ts:238); onrunning→running-canceled. Correct.finally—shouldRestartnow includesrunning-canceled-dirty(scheduler.ts:273-276), so a re-entered dirty task restarts from route-tree; an invisible dirty/canceled-dirty task drops cleanly (both guards requireisVisible) and returns toidle, so a later visibility event re-warms via the reset+enqueue path. No stuck-task path and counter balance is preserved (singleinProgress++/ single always-firingfinallydecrement).
Covered by the new unit test "preserves a forced route-tree restart across visibility cancellation and re-entry" and the integration test "restarts in-flight cacheComponents prefetches after invalidation across a visibility flap."
setVisibleLinkPrefetch stale-router-mode fix — correct
setVisibleLinkPrefetch previously gated the viewportPrefetched short-circuit on the stale snapshot instance.routerMode === "pages" while using the live resolveCurrentLinkPrefetchRouterMode(instance) for the app branch (link.tsx:836-839). A Pages-visible link that later has the App runtime register would keep short-circuiting and never enter the segment-cache scheduler on re-entry. The fix resolves the live router mode once and uses it for both the short-circuit and the branch. Matches the new test "uses the cacheComponents scheduler when a Pages-visible Link re-enters after App runtime registers." Consistent with the sibling guards in pingVisibleLinkPrefetches (link.tsx:862) and drainVisibleAppPrefetchQueue.
Non-blocking (carried over, still valid)
- Foreground stale-revalidate can render twice. In
readAppPageCacheResponse(app-page-cache.ts), whenshouldForegroundRevalidateStaleAppPageis true,renderFreshPageForCache()is awaited; ifbuildAppPageCachedResponsereturns null (RSC with norscData, or empty HTML from a redirect/notFound render), control falls through toscheduleBackgroundRegeneration— a second render, then serves stale. Wasteful + minorstale: 0parity gap, not incorrect; mirrors legacymain. null-sourcePage→ materialization is an undertested deliberate decision.metadata.sourcePage?.includes("[")(app-page-render.ts:367) proceeds to materialize whensourcePageis null. Safe today (missing params keep symbolic segments), but "materialize on null" vs "bail on null" (?? true) still has no pinning test — recommend adding one in this PR.- Scope creep. The dev-only
rsc-reference-validation-normalizerplugin and thecacheLife stalepropagation remain unrelated to "segment-cache prefetch scheduling." Both are tested and coherent; per the repo's small-PR guidance they'd be cleaner as separate PRs — at minimum call them out in the description. - Minor stale reference (now slightly broader): a
running-dirty/running-canceled-dirtytask that drops infinallybecause its instance went invisible does not nullmostRecentIntentTask(onlycanceland segment completion do). Harmless — the stale reference is overwritten by the next high-priority intent — but noting for symmetry.
Summary
The 3fc7b5b state-machine addition is correct and closes a genuine dropped-prefetch gap under force-invalidation + visibility flap; both new behaviors are well-tested and the scheduler remains self-consistent (no stuck task, balanced counter). No new blocking issues at this tip. The structural review from @NathanDrake2406 remains materially addressed (scheduler extracted, explicit runPhase contract, status-enum lifecycle). Recommend addressing non-blocking note #2 (pin the null-source-page decision) in this PR.
|
Review posted to PR #2465. Review summaryI re-reviewed the diff since the last reviewed head ( The new
Both new behaviors are well-tested (one scheduler unit test + one integration test). The Non-blocking (carried over): foreground stale-revalidate double-render, undertested null- |
NathanDrake2406
left a comment
NathanDrake2406
left a comment
There was a problem hiding this comment.
Blocking on one issue in packages/vinext/src/build/prerender.ts.
writePrerenderIndex drops stale when it serializes rendered routes. prerenderApp now computes stale, and seed-cache reads route.stale, so prerendered App routes lose cache-life stale metadata after startup.
Please persist stale in the manifest and add a focused test.
…he-prefetch-scheduling-28478866791 # Conflicts: # packages/vinext/src/shims/link.tsx # tests/link-navigation.test.ts
2 participants
Summary
Next.js parity
Targets the Segment Cache scheduling behavior covered by:
test/e2e/app-dir/segment-cache/prefetch-scheduling/prefetch-scheduling.test.tsValidation
vp test run tests/link-navigation.test.ts -t "Link prefetch scheduling"vp check packages/vinext/src/shims/link.tsx tests/link-navigation.test.tsREPO="$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/prefetch-scheduling/prefetch-scheduling.test.ts(4 passed, 1 skipped)Review
Independent review found and fixed two issues before PR: missing
Next-Router-Prefetch: 1on segment-phase requests, and early segment-phase starts when a visible link was rescheduled while the route-tree response was still pending.