fix(app-router): skip prefetches for bots#2323
Conversation
|
/bigbonk review for issues |
commit: |
|
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: skip prefetches for bots
Scoped, well-tested change. The two production gates (<Link> viewport/intent prefetch in link.tsx:438 and router.prefetch() in navigation.ts:1852) are placed correctly, the unit + browser coverage is solid, and vp check + the two unit files pass locally. The E2E spec is a genuine regression guard: it asserts no RSC: 1 prefetch on hover for a Googlebot UA but does still navigate (exactly one RSC request) on click, matching Next.js. The request.headers().rsc === "1" assertion is valid — createRscRequestHeaders sets RSC: "1" (app-rsc-cache-busting.ts:281) and Playwright lowercases header names.
Issue: next/form App Router prefetch is not gated for bots
The PR's stated goal is to "skip App Router Link viewport/intent prefetches" and router.prefetch() for bots, gating all App Router RSC prefetch creation. But there is a third App Router RSC prefetch entry point that issues the same kind of request and is left ungated:
packages/vinext/src/shims/form.tsx:311-369 — the <Form> viewport IntersectionObserver prefetches the action route via the same unified createRscRequestUrl + prefetchRscResponse path used by <Link> and router.prefetch(). There is no isBotUserAgent check anywhere in form.tsx (confirmed: no isBot/userAgent reference in the file).
Consequence: a bot scrolling a page containing an App Router <Form action="/..."> still triggers an RSC prefetch for the action route — the exact behavior this PR suppresses for <Link>. This is an inconsistency within vinext's own prefetch model, since all three call sites share the same RSC prefetch mechanism.
Note on Next.js parity: in Next.js v16.2.6 the App Router <Form> prefetch goes through the segment-cache scheduler (mountFormInstance), a different subsystem than createPrefetchURL/isBot — and this PR intentionally avoids the segment-cache track. So this is not strictly a Next.js-parity miss for Form. But because vinext routes Form prefetch through the same RSC path the PR is gating, leaving it ungated produces divergent bot behavior across vinext's own Link/router/Form prefetches.
Suggested fix — add the same guard inside the form viewport observer callback, mirroring link.tsx:438:
// form.tsx, inside the IntersectionObserver callback (before createRscRequestUrl)
if (isBotUserAgent(window.navigator?.userAgent ?? "")) return;with import { isBotUserAgent } from "../utils/html-limited-bots.js";. A focused unit test (bot UA → form viewport intersection issues no fetch) would round out the coverage to match the new Link/router tests.
Minor notes (non-blocking)
- Bot-list scope vs Next.js
isBot.isBotUserAgentcombines the HTML-limited bot list + the headlessGooglebotregex. Next.js's clientcreatePrefetchURLusesisBot()fromshared/lib/router/utils/is-bot.ts. These are intended to be equivalent per the utility's doc comment; worth a quick confirmation that the combined list is a superset of (and doesn't diverge from) Next'sisBotfor the prefetch use case, since the same util is reused for the Pages fallback path. - Ordering in
router.prefetch.assertSafeNavigationUrl(href)runs before the bot check, whereas Next'screatePrefetchURLdoes the bot check first (then URL parse). The URL-parse-throws ordering relative to the bot check matches Next (bot check before parse), so this is fine;assertSafeNavigationUrlis vinext defense-in-depth and running it unconditionally is acceptable.
Overall the core change is correct and safe to ship; the next/form gap is the one item I'd want addressed (here or as an immediate follow-up) so bot suppression is consistent across all three App Router prefetch entry points.
|
Review posted to PR #2323. SummaryThe PR is a focused, well-tested change that gates App Router One issue worth addressing — found by tracing all App Router RSC prefetch entry points:
This isn't strictly a Next.js parity miss (Next's App Form prefetch goes through the segment-cache scheduler, which this PR intentionally avoids), but it's an inconsistency within vinext's own prefetch model since all three call sites share the same mechanism. Minor non-blocking notes on bot-list equivalence vs Next's |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: skip prefetches for bots
The earlier review's one substantive concern — the ungated next/form App Router prefetch — has been addressed in 7576a6f. All three App Router RSC prefetch entry points are now gated consistently, the change passes vp check and the three unit files (110 tests), and the E2E spec now covers Link and Form. Looks good to ship.
Verified
-
All three RSC prefetch entry points gated. Traced every
createRscRequestUrl+prefetchRscResponsecall site:link.tsx:438— viewport/intent prefetch (gated insidehasAppNavigationRuntime())navigation.ts:1852—router.prefetch()(gated afterisServer)form.tsx:328— viewport prefetch (gated inside the IntersectionObserver async IIFE) ✅ new in7576a6f
The remaining
createRscRequestUrlcall sites inapp-browser-entry.ts(1283,1605,2176) are navigation-commit and HMR-replace paths, not prefetch paths, so they're correctly left ungated — a bot that actually navigates must still receive real content. -
Navigation preserved for bots. The E2E spec (
prefetch-bot.browser.spec.ts:148-199) asserts noRSC: 1prefetch on hover/viewport for a Googlebot UA, but exactly one RSC request on Link click and one on Form submit. This matches Next.js: only prefetch creation is gated, not navigation. -
Pages Router prefetch intentionally preserved.
link-navigation.test.tsaddspreserves Pages Router viewport, explicit, and intent prefetches for a bot user agent, confirming the gate is scoped tohasAppNavigationRuntime()and Pages<link rel="prefetch">document prefetches still emit for bots — matching Next.js, which only gates App RoutercreatePrefetchURL. -
form.tsxobserver hygiene.observer.unobserve(node)runs outside the async IIFE, so even when the bot gate returns early the observer still detaches — no leaked observer.
Minor notes (non-blocking, all acceptable)
router.prefetchgate ordering.isBotUserAgentnow runs before the URL-parse-throws block (navigation.ts:1862-1866). This actually matches Next'screatePrefetchURL, which checksisBotbefore parsing — a bot passing an unparseable href silently returns rather than throwing, same as Next.assertSafeNavigationUrlstill runs unconditionally first as vinext defense-in-depth, which is fine.- Bot-list equivalence.
isBotUserAgentcombines the HTML-limited list + the headlessGooglebotregex and is documented to mirror Next'sis-bot.ts. The combined list is a superset of Next'sisBotfor this use case. window.navigator?.userAgent ?? ""— the optional chaining + empty-string fallback means a missing navigator/UA is treated as "not a bot" (prefetch proceeds), which is the safe default and matchesisBotUserAgent("") === false.
Core change is correct, consistent across all vinext App Router prefetch entry points, and safe to ship.
|
Review posted to PR #2323. SummaryThis PR gates all three App Router RSC prefetch entry points behind Verified:
Minor non-blocking notes: bot check ordering relative to URL-parse-throws actually matches Next's Conclusion: correct, consistent, and safe to ship — no blocking issues found. |
1 participant
Summary
router.prefetch()for bots through the same existing Next.js-compatible matcherParity target
Report
28143992598showedtest/e2e/app-dir/app-prefetch/prefetching.test.tsfailingshould not prefetch for a bot user agentbecause vinext issued an RSC request for/static-page.Next.js v16.2.6 gates all prefetch URL creation with
isBot(window.navigator.userAgent)inpackages/next/src/client/components/app-router-utils.ts.This track intentionally avoids:
Validation
vp check packages/vinext/src/shims/link.tsx packages/vinext/src/shims/navigation.ts tests/link-navigation.test.ts tests/prefetch-cache.test.ts tests/e2e/app-router/nextjs-compat/prefetch-bot.browser.spec.tsvp test run tests/link-navigation.test.ts tests/prefetch-cache.test.ts --maxWorkers=1PLAYWRIGHT_PROJECT=app-router-chrome-browser-specific npx playwright test tests/e2e/app-router/nextjs-compat/prefetch-bot.browser.spec.ts --workers=1 --retries=0NEXTJS_PREPARE=0 NEXT_TEST_CONCURRENCY=1 vp env exec --node 24 ./scripts/run-nextjs-deploy-suite.sh /Users/jamesanderson/Developer/vinext/.nextjs-ref --retries 0 -c 1 --debug test/e2e/app-dir/app-prefetch/prefetching.test.tsshould not prefetch for a bot user agentpasses