fix(app-router): schedule cache component link prefetches - #2857
fix(app-router): schedule cache component link prefetches#2857james-elicx wants to merge 5 commits into
Conversation
|
/bigbonk review for issues |
commit: |
|
Run 31290819291 backlog mapping for
The run artifact recorded all four as failures (0 passed, 4 failed, 1 skipped). On this exact PR head, the targeted Next.js E2E passed twice with 4 passed, 1 skipped. |
|
![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): schedule cache component link prefetches
Reviewed the scheduler (link-segment-prefetch-scheduler.ts), the link.tsx integration, and the three test files. Ran the targeted suites and vp check on the changed source — all green (9 passed). No blocking correctness issues found; the design mirrors Next.js's Segment Cache scheduler faithfully and dev/prod parity is preserved via the build-time define. A few observations below, none blocking.
Verified correct
- Cache Components gate & tree-shaking.
CACHE_COMPONENTS_ENABLEDchecks both=== "true"(dev/test string env) and=== true(production booleandefinefromindex.ts:2361, which JSON-stringifiesnextConfig.cacheComponents ?? false). Consistent with the existingslot.tsx/app-page-route-wiring.tsxpattern, andlink-segment-prefetch-bundle.test.tsconfirms the scheduler and global listeners dead-strip when disabled. - No double-run on cancel+reschedule of a running task.
scheduleskips the phase reset whileisRunningandenqueueearly-returns for running tasks, so the in-flight promise'sfinishTaskPhaseremains the single driver. The re-entry test covers this. activeRequestsaccounting. Canceling a running task defers the decrement tofinishTaskPhase, avoiding a double-decrement — correct.- Batch ordering.
hasBandwidth's older-batch route-tree gate works because there is always ≥1 queued older-batch task while any remain unstarted; once older tasks advance to the segment phase they stop blocking newer route trees, which is acceptable.
Non-blocking observations
-
previousIntentTaskis only cleared when it finishes its segment phase while still visible (finishTaskPhase, lines 186–188, guarded by!isCanceled && instance.isVisible). If apreviousIntentTaskbecomes invisible without going throughcancel(), the reference lingers withisRunning === false. This is currently harmless because the reserved-bandwidth block inhasBandwidthadditionally requirespreviousIntentTask.isRunning === true, andsetVisibleLinkPrefetch/unmount both route invisibility throughcancel(). Worth a short comment noting the invariant so a future refactor of the visibility path doesn't reintroduce a stale-reference block. -
if (prefetchMode === "disabled") return;(link.tsx:1380) is likely redundant —shouldViewportPrefetchalready gates viacanLinkPrefetch(which accounts forisDangerousandprefetch === false, the only two waysresolveLinkPrefetchModereturns"disabled"). It is safe and also satisfies the narrowedfetchStrategytype, so keeping it is fine; just flagging it as belt-and-suspenders. -
forceSegmentCacheFetchis captured at route-tree schedule time and carried into the segment phase rather than re-evaluated againsthasRecentUserInteraction()when the segment phase starts. The scheduler test at line 230 asserts this carry-over is intended, so this is by design — noting it only for future reference against Next.js parity.
Known gap (already disclosed)
The PR body correctly notes the upstream unstable_dynamicOnHover duplicated-static-layout case still rejects; this patch fixes the scheduling failure without broadening into duplicated-layout cache identity. Agreed that is out of scope here.
LGTM pending CI.
|
Review posted to PR #2857. SummaryI reviewed the scheduler module, the No blocking correctness issues. The implementation faithfully mirrors Next.js's Segment Cache scheduler and preserves dev/prod parity through the build-time Key things I verified:
Three non-blocking observations flagged in the review:
The disclosed |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
CI follow-up on exact head
Post-fix validation:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review — fix(app-router): schedule cache component link prefetches
Independent pass over the scheduler (link-segment-prefetch-scheduler.ts), the link.tsx integration, and the three test files. Ran the targeted suites and vp check on the changed source — all green (link-segment-prefetch-scheduler + link-segment-prefetch-bundle: 9 passed; link + link-navigation + prefetch-cache: 276 passed; vp check: clean). No blocking correctness issues.
Verified correct
- Cache Components gate & tree-shaking.
CACHE_COMPONENTS_ENABLED(link.tsx:977) checks both=== "true"(dev/test string env) and=== true. The productiondefineatindex.ts:2361isJSON.stringify(nextConfig.cacheComponents ?? false), i.e. a real boolean, so the=== truebranch is load-bearing, not redundant.link-segment-prefetch-bundle.test.tsconfirms the scheduler and global listeners dead-strip when disabled. activeRequestsaccounting. A queued (never-started) task canceled viacancel()never touchedactiveRequests; a running task defers its decrement tofinishTaskPhase(line 191), so cancel + reschedule of an in-flight task cannot double-decrement.drainremoves a task from the queue before the canceled/invisible skip (lines 213–217), sofindNextTaskcan't loop on the same task.- Cancel + reschedule of a running route-tree task.
scheduleskips the phase reset whileisRunning(line 277) andenqueueearly-returns for running tasks (line 169), so the original in-flight promise'sfinishTaskPhasestays the single driver that advances to the segment phase. Covered by the re-entry test. - Batch ordering.
hasBandwidth's older-batch route-tree gate (lines 144–155) only blocksroute-treedefault tasks, and older tasks have smallersortIdso they'd otherwise lose thecompareTaskstiebreak — the gate is what lets the older batch drain first. Once older tasks advance tosegmentthey stop blocking newer route trees. No starvation since a blocked newer task always has ≥1 queued older task making forward progress. forceSegmentCacheFetchcarry-over. Captured at route-tree schedule time (line 261) and threaded through the segment phase; the scheduler test at line 230 asserts this carry-over and theprefetchUrlguard at link.tsx:668–671 consumes it to bypass an alias cache hit. Intentional.
Non-blocking observations
-
previousIntentTaskstale-reference invariant. It's only cleared on segment completion while visible (finishTaskPhaselines 193, 200–202). If apreviousIntentTaskever became invisible without going throughcancel(), the reference would linger. Harmless today because (a)hasBandwidth's reserved-bandwidth block additionally requirespreviousIntentTask.isRunning === true(line 135), and (b) every invisibility path —setVisibleLinkPrefetchand the effect-cleanup unmount inlink.tsx— routes throughcancel(), which clears it (line 296). Worth a one-line comment documenting the invariant so a future refactor of the visibility path doesn't reintroduce a stale block. -
cancel(mostRecentIntentTask)doesn't re-promotepreviousIntentTask. When the most-recent hover leaves,mostRecentIntentTaskis nulled but the demotedpreviousIntentTaskis not restored to intent. This is acceptable (the second-most-recent still keeps its reserved-bandwidth block while running, matching the "keep the earlier hover's response ahead" intent) — noting only for future parity comparison. -
if (prefetchMode === "disabled") return;(link.tsx:1383) is belt-and-suspenders.shouldViewportPrefetchalready gates throughcanLinkPrefetch(which accounts for the only two waysresolveLinkPrefetchModereturns"disabled":isDangerousandprefetch === false). It's safe and also narrows thefetchStrategytype, so keeping it is fine.
Known gap (already disclosed)
The PR body correctly scopes out the upstream unstable_dynamicOnHover duplicated-static-layout case; this patch fixes the scheduling failure without broadening into duplicated-layout cache identity. Agreed that's out of scope here.
LGTM pending CI.
|
Review posted to PR #2857. SummaryI independently reviewed the scheduler ( No blocking correctness issues. The scheduler faithfully mirrors Next.js's Segment Cache lifecycle and dev/prod parity is preserved via the build-time Key things I verified independently:
Three non-blocking observations flagged (matching the prior review's read): the |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review — fix(app-router): schedule cache component link prefetches
Independent pass over the new scheduler (link-segment-prefetch-scheduler.ts), the link.tsx integration, and the navigation/popstate rewiring (navigation.ts, app-browser-entry.ts, navigation-runtime.ts). Ran the targeted suites and they're green:
link-segment-prefetch-scheduler+link-segment-prefetch-bundle: 14 passedlink+link-navigation: 211 passedprefetch-cache: 71 passed
No blocking correctness issues. The scheduler mirrors Next.js's Segment Cache lifecycle, and the Cache-Components gate keeps the scheduler + global listeners fully dead-stripped when disabled.
Verified correct
activeRequestsaccounting is balanced. Each phase increments indrain(line 258) and decrements exactly once infinishTaskPhase(line 211) via.finally. A queued task canceled before it starts never touched the counter; a running task defers its decrement tofinishTaskPhase, so cancel + reschedule of an in-flight task cannot double-decrement or leak.- No double-run on hover of a running task. With an unchanged strategy,
schedulehitsisRunning(line 332) and the innerifis false, so norestartAfterRunningis set — the original in-flight promise'sfinishTaskPhasestays the single driver and simply carries the now-intentpriority into the segment phase. Matches the "increases priority on hover" Next.js case. - No
drainself-loop. Canceled/invisible tasks are removed from the queue before the skip (lines 247-251), sofindNextTaskcan't reselect them. - Batch-ordering gate can't deadlock/starve. The older-batch block only applies to
route-treedefaulttasks and requires a queued older task; once older tasks run or advance tosegmentthey stop blocking, and a blocked newer task always has a queued older task making progress. - Popstate cancellation parity is now cleaner. Moving the cancel from
navigation.tsinto theapp-browser-entry.tspopstate handler (after theisSameAppRoutePopstateTargetearly return, line 2464) means hash-only traversals no longer over-cancel prefetches — an improvement over the previous "over-cancel by destination" behavior the old comment documented. - The synchronous click-time
cancel(link.tsx:1608-1616) guards onhasAppNavigationRuntime(the local boolean shadow at line 1552, not the imported fn) and skips hash-only changes — correct, and complements the runtime-widecancelLinkPrefetchTaskshook for the split-chunk race it documents. - The dual
=== "true" || === truegate is load-bearing, not redundant: the productiondefinecompiles to a real boolean while the string branch covers dev/test, consistent with the existingslot.tsxpattern.
Non-blocking observations (agree with prior reviews)
previousIntentTaskstale-reference invariant. Cleared only on segment completion while visible (lines 231-233); harmless today becausehasBandwidth's reserved-bandwidth block also requirespreviousIntentTask.isRunning === trueand every invisibility path routes throughcancel(). A one-line comment on the invariant would protect it against a future visibility-path refactor.if (prefetchMode === "disabled") return;(link.tsx:1398) is belt-and-suspenders relative toshouldViewportPrefetch/canLinkPrefetch, but it's safe and also narrows thefetchStrategytype — fine to keep.forceSegmentCacheFetchcarry-over from route-tree schedule time into the segment phase is asserted intentional by the scheduler test.
Known gap (already disclosed)
The PR body correctly scopes out the upstream unstable_dynamicOnHover duplicated-static-layout case. Agreed that's out of scope here.
LGTM pending CI.
|
Review posted to PR #2857. SummaryI independently reviewed the new scheduler ( No blocking correctness issues. The scheduler faithfully mirrors Next.js's Segment Cache lifecycle, and Cache Components dead-strips the scheduler and global listeners when disabled. Key things I verified independently:
Three non-blocking observations (matching the prior reviews): the LGTM pending CI. |
…fetch-scheduling # Conflicts: # packages/vinext/src/shims/link.tsx
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review — fix(app-router): schedule cache component link prefetches
Independent pass over the new scheduler (link-segment-prefetch-scheduler.ts), the link.tsx two-phase prefetch integration, the prefetch-policy skipSegmentCacheScheduler opt-out, and the popstate/traversal cancellation rewiring (navigation.ts, app-browser-entry.ts, navigation-runtime.ts). Ran the targeted suites locally — all green:
link-segment-prefetch-scheduler+link-segment-prefetch-bundle: 14 passedlink-navigation: 85 passed
No blocking correctness issues. The scheduler mirrors Next.js's Segment Cache lifecycle, and the Cache-Components gate dead-strips the scheduler + global listeners when disabled.
Verified correct
- Two-phase completion is real, not fire-and-forget.
prefetchUrlnow returns the inner async promise, and it awaitsscheduledEntry?.pending(link.tsx:839-841) plus the learning-only fresh-entry pending (link.tsx:584-588) before resolving. So the scheduler'sfinishTaskPhasefires only after the route-tree / segment request actually settles — this is what makes the route-tree→segment handoff and the reserved-bandwidth ordering correct. activeRequestsis balanced. Each phase increments once indrain(line 258) and decrements exactly once infinishTaskPhase(line 211) via.finally. A queued task canceled before it starts never touched the counter; a running task defers its decrement tofinishTaskPhase, so cancel + reschedule of an in-flight task can't double-decrement or leak. TherestartAfterRunning+cancelinterleaving also can't leakunfinishedTasks(cancel deletes it;finishTaskPhaseline 235 re-deletes harmlessly).- No
drainself-loop. Canceled/invisible tasks are removed from the queue before the skip (lines 247-251), sofindNextTaskcan't reselect the same task. - Batch-ordering gate can't deadlock/starve. The older-batch block only applies to
route-treedefaulttasks and requires a queued older task; a running/segment-phase older task stops blocking, and a blocked newer task always has a queued older task making progress. previousIntentTaskstale-reference is actually safe today. Every invisibility path routes throughcancel()—setVisibleLinkPrefetch(instance, false)(link.tsx:1038) and the effect-cleanup unmount which callscancel(instance)(link.tsx:1434) before settingisVisible = false(link.tsx:1437) — andcancelclears bothmostRecentIntentTaskandpreviousIntentTask(scheduler lines 367-368). Combined withhasBandwidth's extrapreviousIntentTask.isRunning === truerequirement, no stale block can form. Agree with prior reviews that a one-line invariant comment would protect this against a future visibility-path refactor, but it's non-blocking.- Click-time synchronous cancel (link.tsx:1614-1621) correctly guards on the local
hasAppNavigationRuntimeboolean (link.tsx:1558, not the imported fn) and skips hash-only changes viaisHashOnlyBrowserUrlChange. The added navigation-epoch re-check after URL hashing (link.tsx:574) closes the click-wins-during-await gap; if the runtime-widecancelLinkPrefetchTasksbumps the epoch, an in-flight segmentprefetchUrlreturns early and the already-canceled task'sfinishTaskPhaseis a no-op. - Popstate parity improved. Moving the cancel out of
notifyAppPopstateNavigationStartand into the browser-entry popstate handler after theisSameAppRoutePopstateTargetearly return (app-browser-entry.ts:2464) eliminates the old hash-only over-cancellation the removed comment documented. skipSegmentCacheScheduleropt-out correctly keeps encoded-slash / fully-dynamic-root learning-only paths on the single-shell request path (usesSegmentCachePrefetchSchedulerreturns false), avoiding a duplicate segment request for those cache-identity-sensitive routes.- Dual
=== "true" || === truegate is load-bearing, not redundant — the productiondefinecompiles to a real boolean while the string branch covers dev/test, consistent withslot.tsx.
Non-blocking observations (agree with prior reviews)
previousIntentTaskinvariant comment (above) — worth adding.if (prefetchMode === "disabled") return;(link.tsx:1406) is belt-and-suspenders vsshouldViewportPrefetch/canLinkPrefetch, but it's safe and narrows thefetchStrategytype — fine to keep.forceSegmentCacheFetchcarry-over from route-tree schedule time into the segment phase is asserted intentional by the scheduler test.
Known gap (already disclosed)
The PR body correctly scopes out the upstream unstable_dynamicOnHover duplicated-static-layout case; this patch fixes the scheduling failure without broadening into duplicated-layout cache identity. Agreed that's out of scope.
LGTM pending CI.
|
Review posted to PR #2857. SummaryI independently reviewed the new scheduler ( No blocking correctness issues. The implementation faithfully mirrors Next.js's Segment Cache scheduler and preserves dev/prod parity via the build-time Key things I verified independently (beyond prior reviews):
Three non-blocking observations flagged (matching prior reviews): the LGTM pending CI. |
# Conflicts: # packages/vinext/src/shims/internal/app-route-prefetch-policy.ts # packages/vinext/src/shims/link.tsx # tests/link-navigation.test.ts
1 participant
Summary
unstable_dynamicOnHoverstrategiesNext.js parity
Targets
test/e2e/app-dir/segment-cache/prefetch-scheduling/prefetch-scheduling.test.ts.Targeted Next.js E2E passed twice: 4 passed, 1 skipped.
Validation
vp test run tests/link-segment-prefetch-bundle.test.ts tests/link-navigation.test.ts tests/link-segment-prefetch-scheduler.test.ts tests/prefetch-cache.test.ts tests/link.test.ts— 285 passedvp run checkKnown unrelated parity gap
The upstream
unstable_dynamicOnHovercase still rejects a duplicated static layout. Currentmainfails earlier because it does not fetch the dynamic content; this patch fixes the scheduling failure but does not broaden into duplicated-layout cache identity.