fix(app-router): start optimistic prefetches immediately#2440
Conversation
|
/bigbonk review for issues |
commit: |
|
There was a problem hiding this comment.
Review: start optimistic prefetches immediately
I reviewed the diff against main, traced the call sites, and ran the targeted tests. Verdict: the change is correct and well-scoped. Tests pass locally (tests/link-navigation.test.ts 55✓, tests/app-optimistic-routing.test.ts 12✓) and tsc is clean for link.tsx.
What the change does (and why it's right)
The old code gated all low-priority prefetches behind requestIdleCallback. Viewport-triggered App Router prefetches are dispatched with "low" priority (setVisibleLinkPrefetch → prefetchUrl(..., "low", ...) at packages/vinext/src/shims/link.tsx:712 and pingVisibleLinkPrefetches at :727), so they were being deferred to browser idle. That's the exact behavior the Next.js optimistic-routing e2e asserts against. Moving the App Router path to run synchronously is the correct fix.
The split is clean:
priority === "high"→ immediate (unchanged behavior; high always ran immediately).hasAppNavigationRuntime()→ immediate (new; covers low-priority viewport prefetches).- Pure Pages Router, low priority → still
requestIdleCallback/setTimeoutfallback.
Test quality
The new regression is genuinely meaningful, not a tautology. The default renderIsolatedLink helper stubs requestIdleCallback as synchronous (tests/link-navigation.test.ts:1187), so the pre-existing "prefetches visible links" test would pass with or without this fix. The new test correctly overrides requestIdleCallback with a non-invoking vi.fn(() => 1) and asserts requestIdleCallback is never called, which actually proves the bypass. Good.
Minor: stale doc comment (non-blocking)
The prefetchUrl JSDoc still claims idle scheduling is always used:
packages/vinext/src/shims/link.tsx:398
* Uses `requestIdleCallback` (or `setTimeout` fallback) to avoid blocking
* the main thread during initial page load.
After this change that's only true for pure-Pages low-priority prefetches. Worth a one-line update so the comment matches the new scheduling logic, e.g. note that App Router and high-priority prefetches dispatch immediately and only low-priority Pages prefetches defer to idle.
Things I checked and confirmed are fine
- Hybrid apps: when App runtime is present, the inner hybrid-ownership guard (
:477-480) returns early for Pages/document-owned URLs rather than falling through to the Pages document-prefetchelse if, so the "keep Pages document prefetches on idle" claim holds — those don't run through this path at all when App runtime exists. - High-priority Pages intent prefetch: still runs immediately, matching prior behavior; the existing intent-prefetch test (
:1751) remains consistent.
LGTM aside from the doc comment nit.
|
Review posted to PR #2440. SummaryI reviewed PR #2440 ( What I verified:
One non-blocking nit raised: the No correctness or parity issues found. |
3181bf0 to
96d2dec
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: start optimistic prefetches immediately
Independently re-reviewed against main: traced the call sites, checked dev/prod and hybrid-app parity, and ran the targeted tests. The change is correct and well-scoped. No correctness or parity issues.
Verified
-
Root cause matches the fix. Viewport prefetches are dispatched as
"low"priority (setVisibleLinkPrefetch→prefetchUrl(..., "low", ...)andpingVisibleLinkPrefetches), so the oldpriority === "high" ? immediate : requestIdleCallbackgate deferred all App Router viewport prefetches to browser idle. The new gateif (priority === "high" || hasAppNavigationRuntime())runs them immediately, which is what the Next.jsoptimistic-routinge2e asserts. -
Pages parity is preserved, including hybrid apps. The "keep low-priority Pages fallback document prefetches on idle" claim holds:
- Pure Pages app (
hasAppNavigationRuntime()false): low-priority prefetches still go throughrequestIdleCallback/setTimeoutfallback (link.tsx:654). Unchanged. - Hybrid app (App runtime present): a Pages/document-owned URL now runs immediately, enters the
hasAppNavigationRuntime()block, and returns early at the hybrid-owner guard (link.tsx:478-481) — it never reaches theelse ifdocument-prefetch branch. This was already true onmain(theelse if (HAS_PAGES_ROUTER ...)branch is unreachable whenever the App runtime exists), so this PR only changes timing on the App path, not which branch runs.
- Pure Pages app (
-
Test is a genuine regression guard, not a tautology. The default
renderIsolatedLinkhelper stubsrequestIdleCallbackto fire synchronously (link-navigation.test.ts:1187), so the pre-existing "prefetches visible links" test would pass with or without the fix. The new test overrides it with a non-invokingvi.fn(() => 1)and assertsrequestIdleCallbackis never called — that actually proves the bypass. -
Local runs:
tests/link-navigation.test.ts55✓,tests/app-optimistic-routing.test.ts12✓.tscclean forlink.tsx.
Non-blocking nit (already addressed)
The earlier review's point about the stale prefetchUrl JSDoc has been fixed in this revision (link.tsx:398-400 now correctly distinguishes App/high-priority immediate dispatch from low-priority Pages idle scheduling).
LGTM.
|
Review posted to PR #2440. SummaryI independently re-reviewed PR #2440 ( What I verified:
Note: The stale JSDoc nit raised in the earlier review has already been fixed in this revision ( Verdict: LGTM. |
|
This looks like it covers the run 28478866791 / job 84413308650 |
|
Deploy-suite run 28478866791 / job 84413308650 backlog check: this PR appears to cover the |
|
Deploy-suite run 28478866791 / job 84413308650 backlog check: this PR appears to cover the |
|
Verified #2440 also covers |
|
Backlog duplicate check for run 28478866791 / job 84413308650: this PR likely covers the remaining non-cacheComponents/use-cache |
…routing-prefetch # Conflicts: # packages/vinext/src/shims/link.tsx
This reverts commit 34a3765.
Summary
Next.js parity
Fixes the optimistic-routing failures from deploy-suite run 28478866791:
test/e2e/app-dir/optimistic-routing/optimistic-routing.test.tsValidation
vp check packages/vinext/src/shims/link.tsx tests/link-navigation.test.tsvp test run tests/link-navigation.test.ts tests/app-optimistic-routing.test.ts --maxWorkers=1— 67 passedNEXT_TEST_CONCURRENCY=1 REPO="$(pwd)" NEXTJS_DIR="/Users/jamesanderson/Developer/vinext/.nextjs-ref" ./scripts/run-targeted-nextjs-e2e.sh test/e2e/app-dir/optimistic-routing/optimistic-routing.test.ts— 9 passedNotes
The broader app-prefetch protocol cluster from the same run appears covered by #2318, so this PR stays focused on the optimistic-routing dispatch timing issue.