feat: add ISR caching for App Router route handlers

[NathanDrake2406]

Summary

• Route handlers with export const revalidate = N now get server-side ISR caching (MISS → HIT → STALE with background regeneration), matching the existing page ISR behavior
• Previously only Cache-Control headers were emitted — the handler re-executed on every request with no server-side cache lookup or store
• Reuses existing CachedRouteValue type (kind: "APP_ROUTE") and KV serialization that were already defined but unused

Implementation

• __isrRouteKey helper with "route" suffix for cache key construction (no HTML/RSC split needed)
• Cache READ before handler execution — HIT returns cached response immediately (skips handler), STALE serves stale data and triggers background regeneration
• Cache WRITE on MISS via response.clone() + waitUntil — stores raw handler response before cookie/middleware transforms
• Background regen uses synthetic request + clean ALS context (empty headers/cookies) to prevent user-specific data from leaking into cached responses
• Guards: production-only, GET/HEAD only, skips dynamic handlers (headers()/cookies()), skips handlers that set own Cache-Control, filters Infinity from revalidateSeconds

Test Plan

• 3 code generation tests (__isrRouteKey, APP_ROUTE cache read/write in generated code)
• 6 production behavioral tests (MISS, HIT, POST bypass, dynamic bypass, handler Cache-Control bypass, STALE lifecycle)
• 1 dev-mode guard test (no X-Vinext-Cache header in dev)
• 262/262 app-router.test.ts pass (no regressions to page ISR)
• 28/28 app-routes.test.ts pass
• 78/78 isr-cache.test.ts + kv-cache-handler.test.ts pass
• Typecheck clean, lint clean, format clean

[pkg-pr-new]

Open in StackBlitz

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

commit: 86bf96f

[james-elicx]

/bigbonk review

[ask-bonk]

Review: Route Handler ISR Caching

The overall approach is sound — reusing the existing CachedRouteValue type (APP_ROUTE), __isrGet/__isrSet/__triggerBackgroundRegeneration helpers, and following the same MISS → HIT → STALE lifecycle as page ISR. The guards (production-only, GET/HEAD-only, dynamic API bypass, handler Cache-Control bypass, Infinity filter) are correct and well-tested.

I found one bug and a few issues worth discussing before merge.

Bug: Background regeneration missing setHeadersAccessPhase

The background regeneration closure calls consumeDynamicUsage() to check if the handler became dynamic during regen, but it never calls setHeadersAccessPhase("route-handler") first. The normal handler execution path at line 2098 does set this phase. If the headers shim behavior depends on this phase being set (e.g., for how headers() / cookies() calls are tracked), the regen could behave differently than the original request path. This should either be set before invoking the handler in regen, or there should be a comment explaining why it's unnecessary.

Minor issues

1. Closure over request.url and url.searchParams in background regen — The regeneration lambda closes over the original request object (to read .url) and url.searchParams from the outer scope. While the request.url usage is fine (it's a string extraction for creating the synthetic Request), closing over url.searchParams means the regen's navigation context will include the original request's query parameters. This matches the page ISR pattern, but worth noting that if query strings affect handler output, subsequent requests with different query strings will get the regenerated result from the first query string's regen.

2. Snapshot test size — The same ~80-line ISR block is duplicated 6 times in the snapshot file (once per snapshot variant), adding ~480 lines. This is inherent to how the snapshots work, but it does inflate the PR by 666 lines of snapshot changes. Not blocking, but something to be aware of for maintenance.

3. Test coverage gap: HEAD auto-impl with ISR — The tests cover GET MISS/HIT/STALE and POST bypass, but there's no test for HEAD requests hitting the ISR cache. The code has isAutoHead handling in both the read and write paths (stripping body on HIT/STALE). A test for method: "HEAD" on /api/static-data would verify this works correctly.

See inline comments for specifics.

[ask-bonk]

Bug: consumeDynamicUsage() here checks whether headers() / cookies() were called during the regen handler execution. But the normal handler path at line 2098 calls setHeadersAccessPhase("route-handler") before invoking handlerFn, and restores it in finally at line 2217. This regen path never sets the phase.

If the phase affects how markDynamicUsage() is triggered when headers() / cookies() are called, this could cause the regen to miss detecting dynamic usage — meaning a handler that calls headers() during regen would write stale user-specific data to the cache instead of skipping the write.

Should this be:

Suggested change

	const __regenDynamic = consumeDynamicUsage();
	setHeadersAccessPhase("route-handler");
	const __revalResponse = await __revalHandlerFn(__syntheticReq, { params: __revalParams });
	const __regenDynamic = consumeDynamicUsage();

(And restore the phase afterward, or in a try/finally.)

[NathanDrake2406]

markDynamicUsage() is unconditional — it sets _getState().dynamicUsageDetected = true regardless of the headers access phase (see shims/headers.ts:82). The phase only gates _areCookiesMutableInCurrentPhase() for cookies().set(), which is irrelevant in the regen context (no client to send cookies to). So consumeDynamicUsage() correctly detects dynamic usage here without setHeadersAccessPhase.

[ask-bonk]

Nit: The regen creates a clean headersContext (empty Headers + cookies) on line 2054, which is correct for preventing user data leakage. But setNavigationContext here includes url.searchParams from the original request's URL.

For route handlers, unlike pages, the pathname is the canonical cache key (no query-string variation). If a handler reads url.searchParams from the request object, it gets the synthetic request's params (which would be empty since new Request(request.url, { method: "GET" }) preserves the full URL including query). So this is actually consistent — the nav context and the synthetic request agree on query params.

Just flagging for documentation: the ISR cache key is pathname-only, so two requests to /api/data?a=1 and /api/data?a=2 share the same cache entry. This matches Next.js behavior for route handlers.

[NathanDrake2406]

Acknowledged. The cache key is pathname-only, matching Next.js ISR behavior for route handlers. A handler that varies by query params should read searchParams from the request arg (which triggers markDynamicUsage via headers()) — at which point ISR is skipped entirely.

[ask-bonk]

The double __cached.value.value access looks a bit fragile. __isrGet returns { value: result, isStale } where result is the CacheHandlerValue (which itself has a .value property with the cache data). So __cached.value is the CacheHandlerValue and __cached.value.value is the actual CachedRouteValue.

This works correctly but could be clearer. Consider destructuring or aliasing, e.g.:

const __entry = __cached.value.value;
if (__cached && !__cached.isStale && __entry && __entry.kind === "APP_ROUTE") {

Not blocking — the page ISR path uses the same __cached.value.value pattern, so at least it's consistent.

[NathanDrake2406]

Consistent with the page ISR pattern — keeping it the same for now to avoid unnecessary divergence.

[ask-bonk]

Good catch on cloning the response before consuming it for cache. One edge case: if the route handler returns a streaming response (e.g., a large JSON blob or binary payload that's produced as a ReadableStream), response.clone() works correctly but the arrayBuffer() call on the clone will buffer the entire body in memory.

For most API route handlers this is fine (small JSON payloads), but it's worth noting that large streaming route handlers with ISR caching could cause memory pressure. Not blocking for this PR — just a future consideration if handlers start returning large responses.

[NathanDrake2406]

Acknowledged as a future consideration. Route handlers typically return small JSON payloads so this is acceptable for now.

[ask-bonk]

Nice STALE lifecycle test — it covers the full MISS → wait → STALE → regen → HIT cycle. The setTimeout(1500) + setTimeout(500) approach is pragmatic for a revalidate=1 handler.

Consider adding a HEAD request test as well (auto-HEAD via GET handler). The implementation has specific isAutoHead branches in both read and write paths that strip the response body — would be good to verify that a HEAD request against a cached route returns the expected status + headers with no body.

[NathanDrake2406]

Added in 16adcae — auto-HEAD test verifies cached headers with empty body.

[ask-bonk]

The dedup key for __triggerBackgroundRegeneration is __routeKey (which includes the "route" suffix via __isrRouteKey). This is correct — it's distinct from the page ISR regen key (which uses cleanPathname). Two concurrent requests to a stale route handler will only trigger one regeneration.

[NathanDrake2406]

Correct — the "route" suffix ensures route handler regen and page regen never collide.

[ask-bonk]

Review submitted. Here's a summary of what I found:

One potential bug:

• The background regeneration closure calls consumeDynamicUsage() to detect if headers()/cookies() were called during regen, but it never calls setHeadersAccessPhase("route-handler") first. The normal handler execution path does set this phase at line 2098. If the phase affects how dynamic API usage is tracked, the regen could fail to detect dynamic usage and incorrectly cache user-specific data.

Minor issues:

1. The regen closure captures url.searchParams from the original request — the ISR cache key is pathname-only, so different query strings share one cache entry. This matches the page ISR pattern but could surprise users.
2. No test coverage for HEAD requests hitting the ISR cache (the code has explicit isAutoHead body-stripping logic in both read and write paths).
3. The __cached.value.value double-access pattern is functional but fragile-looking (consistent with page ISR though).

What looks good:

• Correct guard conditions: production-only, GET/HEAD-only, Infinity filter, dynamic API bypass, handler Cache-Control bypass
• Clean context isolation in background regen (fresh headers/cookies, synthetic request)
• Proper dedup via __triggerBackgroundRegeneration with route-specific key
• Good test coverage of the MISS → HIT → STALE lifecycle, POST bypass, dynamic bypass, and handler Cache-Control bypass
• Clean separation: cache write happens before cookie/middleware transforms

github run

[james-elicx]

/bigbonk re-review

[ask-bonk]

Review: Route Handler ISR Caching

Solid implementation overall. The approach of reusing the existing ISR infrastructure (__isrGet/__isrSet/__triggerBackgroundRegeneration) with a new "route" cache key suffix is clean, and the guard conditions are correct and well-tested.

I verified that the previous reviewer's concern about setHeadersAccessPhase in the background regen was correctly addressed by the author — markDynamicUsage() is indeed unconditional, and the page ISR background regen has the same omission, so the behavior is consistent.

I have one real concern (potential correctness issue) and a few minor observations.

Issue: Stored cache-control header in cached entry is leaked on regen write

On MISS, the Cache-Control header is set on the response (line 2113) before the cache write block (line 2116-2145). The cache write stores all headers except x-vinext-cache, meaning the cache-control: s-maxage=N, stale-while-revalidate header is persisted in the cache entry.

For HIT and STALE reads, this is harmless because Cache-Control is explicitly overwritten (lines 2041, 2085). However, in the background regen write path (line 2076), the regen response's headers are stored similarly — the regen handler returns a fresh response without Cache-Control set (since the handler itself doesn't set it), so the regen write stores clean headers. This is actually correct, but it means HIT entries from the MISS path include the cache-control header while entries from the regen path do not. This inconsistency is invisible to the user because both HIT and STALE always overwrite Cache-Control, but it makes the cache entries subtly different depending on how they were populated. Not blocking — just worth noting.

Minor observations

1. The test coverage is good for the core lifecycle. The auto-HEAD test at line 1767 was a nice addition.

2. The revalidate = 1 fixture is pragmatic for testing the STALE lifecycle, but the 1500ms sleep + 500ms regen wait makes that test sensitive to CI timing. If it becomes flaky, bumping to revalidate = 2 with proportionally larger sleeps would be more robust.

3. The MISS and cache-write guard conditions (lines 2107-2111 and 2119-2124) are identical minus the NODE_ENV === "production" check. This duplication is intentional — the first sets Cache-Control in both dev and prod, the second only writes to cache in prod. Clear and correct.

Looks good to merge after considering the inline comments.

[ask-bonk]

The synthetic request is constructed from request.url, which preserves the full URL including query string. This is correct — the handler receives the same URL it would have originally.

But: the url.searchParams captured in the navigation context (from the outer scope's url) and the synthetic request's query string are always identical, since request.url is the same value that was parsed into url earlier. This is fine but worth noting — if the regen were ever to strip query params for cache normalization, these two values would need to diverge.

[ask-bonk]

The cache write IIFE starts executing immediately (before waitUntil is called), so on runtimes where _getRequestExecutionContext() returns null, the write still runs — but with no guarantee it completes before the isolate terminates.

This matches the pattern used by page ISR and __triggerBackgroundRegeneration, so it's consistent. However, unlike __triggerBackgroundRegeneration (which stores the promise in the module-level __pendingRegenerations Map, keeping it reachable), this __routeWritePromise is a local variable with no external reference if waitUntil isn't called.

In practice this is fine — the NODE_ENV === "production" guard means this only runs in prod, and the standard worker entry always provides an execution context. Just flagging the asymmetry with the regen dedup pattern.

[ask-bonk]

Minor: the two if blocks (lines 2033 and 2047) have nearly identical guard conditions — __cached && <stale-check> && __cached.value.value && __cached.value.value.kind === "APP_ROUTE". The only difference is !__cached.isStale vs __cached.isStale.

These could be restructured as a single guard with an inner branch on isStale, which would eliminate the repeated null/kind checks:

Suggested change

	if (__cached && !__cached.isStale && __cached.value.value && __cached.value.value.kind === "APP_ROUTE") {
	if (__cached && __cached.value.value && __cached.value.value.kind === "APP_ROUTE") {
	if (!__cached.isStale) {

Not blocking — the current structure is clear and consistent with the page ISR pattern. But if this pattern appears a third time, it would be worth extracting a helper.

[ask-bonk]

The 1500ms wait for a revalidate=1 entry to become stale is pragmatic but leaves only 500ms of margin. On slow CI runners, the cache entry might still be considered fresh at this point if there's clock skew or the initial request took a while to process.

If this test becomes flaky, consider either:

• Bumping to revalidate=2 with 2500ms sleep
• Or adding a retry loop that polls until it sees STALE (with a timeout)

[ask-bonk]

Review submitted successfully. Here's a summary of my review:

Overall assessment: Solid implementation that correctly reuses the existing ISR infrastructure with a new "route" cache key suffix. The guard conditions (production-only, GET/HEAD-only, dynamic API bypass, handler Cache-Control bypass, Infinity filter) are correct and well-tested.

Key findings:

1. Confirmed the setHeadersAccessPhase non-issue — I verified that markDynamicUsage() is unconditional and the page ISR background regen has the same omission, so the author's response to the earlier review was correct.

2. Noted a minor inconsistency — cached entries from the MISS path include the cache-control header while entries from the regen path do not. This is invisible to users since HIT/STALE always overwrite Cache-Control, but makes cache entries subtly different depending on how they were populated.

3. Flagged the waitUntil asymmetry — the MISS cache write promise is a local variable with no external reference if waitUntil isn't called, unlike __triggerBackgroundRegeneration which stores promises in a module-level Map. Not a practical bug but an asymmetry worth noting.

4. Suggested a structural simplification — the two consecutive if blocks for HIT vs STALE share nearly identical guards and could be nested under a single null/kind check.

5. Flagged potential CI flakiness — the 1500ms sleep for revalidate=1 STALE test has thin margins on slow CI runners.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review: Route Handler ISR Caching

This is a well-structured implementation that correctly reuses the existing ISR infrastructure (__isrGet/__isrSet/__triggerBackgroundRegeneration) with a new "route" cache key suffix. The guard conditions, test coverage, and clean context isolation in background regen are all solid.

I found one real correctness issue (export const dynamic = "force-dynamic" is silently ignored for route handlers), one header filtering inconsistency between the MISS and regen write paths, and a few smaller observations.

Issue: export const dynamic = "force-dynamic" silently ignored for route handlers

This is a pre-existing gap that this PR inherits, but this PR makes it worse by adding ISR caching without checking for it. A route handler that exports both revalidate = 60 and dynamic = "force-dynamic" will now get ISR-cached, which contradicts force-dynamic semantics.

The page ISR guard at line 2314 includes !isForceDynamic, but the route handler ISR guard at line 2047 has no equivalent. handler.dynamic is available on the module namespace (since the module is imported as import * as mod_N) — handler.revalidate is already read this way at line 1992 — but handler.dynamic is never read.

The SSE fixture at tests/fixtures/app-basic/app/api/sse/route.ts exports dynamic = "force-dynamic" and works only because it also sets its own Cache-Control: no-cache header, which triggers the handlerSetCacheControl bypass. But that's masking the bug — a handler with force-dynamic + revalidate that doesn't set its own Cache-Control would be incorrectly cached.

Recommendation: Add const isForceDynamic = handler.dynamic === "force-dynamic" near line 1992 and include !isForceDynamic in the ISR read guard (line 2047) and ISR write guard (line 2142). This could be a follow-up PR if preferred, but it should be tracked.

Header filter inconsistency between MISS write and regen write

The MISS write path (line 2158-2159) strips both x-vinext-cache AND cache-control from stored headers. The regen write path (line 2095-2096) only strips x-vinext-cache. This is harmless in practice (the regen handler response won't have cache-control set since it's a fresh handler execution), but it means cache entries have subtly different stored headers depending on how they were populated. The regen path should also filter cache-control for consistency.

Everything else looks good

• The revalidateSeconds computation at line 1992 correctly filters <= 0 and Infinity before the ISR guards, making the !== null checks in the read/write guards sufficient.
• The "route" suffix for cache keys correctly separates route handler entries from page HTML/RSC entries in the same cache namespace.
• The dedup key using __routeKey (not cleanPathname) is correct — route handler regen and page regen use different dedup key spaces.
• The consumeDynamicUsage() check in the regen closure (line 2087) is a good addition that the page ISR regen doesn't have — it prevents caching user-specific data if the handler conditionally uses dynamic APIs during regen.
• Test coverage is solid: MISS/HIT/STALE lifecycle, POST bypass, dynamic bypass, handler Cache-Control bypass, auto-HEAD, and dev-mode guard.
• The setHeadersAccessPhase omission in the regen path is consistent with the page ISR pattern, and markDynamicUsage() is unconditional, so consumeDynamicUsage() works correctly without it.

[ask-bonk]

This guard is missing a !isForceDynamic check. The page ISR guard at line 2314 includes !isForceDynamic to respect export const dynamic = "force-dynamic". Route handler modules can also export dynamic, and it's accessible on the handler namespace object (handler.dynamic), but it's never read.

A route handler with both export const revalidate = 60 and export const dynamic = "force-dynamic" will be ISR-cached, contradicting force-dynamic semantics.

Suggested fix (near line 1992):

Suggested change

	revalidateSeconds !== null &&
	revalidateSeconds !== null &&
	handler.dynamic !== "force-dynamic" &&

And add the same check to the MISS write guard at line 2144.

[NathanDrake2406]

Fixed in d4c52b5. Added handler.dynamic \!== "force-dynamic" to both the cache read guard and the MISS write guard.

[ask-bonk]

Minor inconsistency: the MISS write path (line 2159) strips both x-vinext-cache and cache-control from stored headers, but this regen write path only strips x-vinext-cache. The regen handler response won't have cache-control set in practice, so this is harmless — but the filter should be consistent to avoid surprises if the handler behavior changes.

Suggested change

	if (k !== "x-vinext-cache") __freshHeaders[k] = v;
	if (k !== "x-vinext-cache" && k !== "cache-control") __freshHeaders[k] = v;

[NathanDrake2406]

Fixed in d4c52b5. Regen write now filters both x-vinext-cache and cache-control, consistent with the MISS write path.

[ask-bonk]

The 1500ms sleep for a revalidate=1 entry leaves only 500ms of margin. If this becomes flaky on slow CI, consider bumping to revalidate=2 in the fixture with a proportionally longer sleep, or adding a retry loop that polls until STALE is observed.

Not blocking — just flagging for maintenance.

[NathanDrake2406]

Acknowledged. Will bump if it becomes flaky.

[ask-bonk]

Review submitted. Here's a summary of my findings:

One correctness issue:

• export const dynamic = "force-dynamic" in route handlers is silently ignored. The page ISR guard checks !isForceDynamic but the new route handler ISR guard does not. handler.dynamic is available on the module namespace (the same mechanism that makes handler.revalidate work at line 1992), but it's never read. A route handler exporting both revalidate = 60 and dynamic = "force-dynamic" would be incorrectly ISR-cached. This is technically a pre-existing gap, but this PR makes it worse by adding server-side caching.

One minor inconsistency:

• The MISS write path strips both x-vinext-cache and cache-control from stored headers, but the regen write path only strips x-vinext-cache. Harmless in practice but should be consistent.

One CI observation:

• The 1500ms sleep for revalidate=1 STALE test has thin margins on slow runners.

What looks good:

• Clean reuse of existing ISR infrastructure with "route" key suffix
• Correct guard conditions (production-only, GET/HEAD-only, Infinity filter, dynamic API bypass, handler Cache-Control bypass)
• consumeDynamicUsage() in the regen closure is a good addition over the page ISR pattern
• Solid test coverage across the full MISS/HIT/STALE lifecycle plus bypass scenarios
• Clean context isolation in background regen with fresh headers/cookies

github run

[NathanDrake2406]

@james-elicx I'm curious, I'm still studying so just wanna ask, is code review at the job kinda like this too? Doing dozens of passes and hairsplitting like this? Or is this LLM behaviour?

[james-elicx]

I work in the web side of my company, and normally PRs are kept quite small so there's not much to comment on, but reviews primarily focus on logic / behaviour / ui functionality/design.

I've noticed the AI will sometimes throw out nitpicks or useless comments in these reviews though. I actually wrote a blog post a couple weeks ago about my view on blockers in case you're interested.

Most human reviews and human authoring / addressing don't have big back and fourths like this. I've only seen a few occasions at work where there's been something like this between humans. It is possible to avoid these kinds of processes with agents, but it normally comes down to the scope+size of the PR, and the level of involvement by the human.

The way that I use agents at work is primarily through Zed's agent panel in a much collaborative manner. Well, I say collaborative, but it's more that I very frequently interrupt the agent the second it starts doing something I don't necessarily agree with, rather than letting it fully generate things itself. That probably helps.

I have done some AI experiments in one of our backend services going full agentic and experimented with Copilot reviews in that and having less human involvement, and there was a lot of back-and-forth on PR reviews with the agents in there.

I would note that I also manually go through diffs in this repo before opening prs or merging as well :)

[james-elicx]

/bigbonk re-review

[ask-bonk]

Review: Route Handler ISR Caching

This is a well-executed implementation. The approach of reusing the existing ISR infrastructure (__isrGet/__isrSet/__triggerBackgroundRegeneration) with a new "route" cache key suffix is clean, and the MISS → HIT → STALE lifecycle follows the established page ISR pattern closely. The previous review rounds caught the force-dynamic gap and the header filter inconsistency — both are fixed in the latest commits.

I have one remaining concern about a closure capture in the background regen, and a few smaller observations. Nothing here is a hard blocker.

Closure captures request object in background regen

The background regen closure (line 2086) creates a synthetic request via new Request(request.url, { method: "GET" }), which means the closure keeps the entire original request object alive for the lifetime of the regen. On Cloudflare Workers where memory is tightly constrained per isolate, this pins the original request's body stream and headers in memory until regen completes. The closure only needs the URL string — capturing request.url into a local variable before the closure would let the original Request be GC'd sooner.

The page ISR regen (line 2374) doesn't have this issue because it doesn't construct a synthetic request — it calls buildPageElement directly with the pathname.

STALE test robustness

The 2000ms sleep + 5s polling loop (line 1746-1762) is much better than the original 1500ms fixed sleep mentioned in earlier reviews. The polling pattern is resilient. No concern here.

Minor observations

1. The revalidateSeconds computation at line 1992 filters Infinity before the guards, which is correct. But the page ISR guard at line 2317 redundantly re-checks revalidateSeconds !== Infinity — the route handler ISR guard doesn't because it trusts the null check alone. Both are correct; the inconsistency is cosmetic.

2. Test ordering dependency: the 7 route handler ISR tests at lines 1688-1782 share a single production server. Since they all hit /api/static-data, the cache state from one test leaks into the next (the STALE test explicitly accounts for this). This is fine for the current test set, but adding more tests to this describe block will require care about cache state.

3. The typeof handlerFn === "function" check in the ISR read guard (line 2052) is redundant with the if (typeof handlerFn === "function") check at line 2121 — if handlerFn isn't a function, we'd fall through to the 405 response at line 2245 anyway. It's defensive and not harmful, just unnecessary.

[ask-bonk]

This closure captures the entire request object just to read .url for constructing the synthetic request. On Workers where isolate memory is tight, this keeps the original request (including body stream and all headers) alive until background regen completes.

Capturing the URL string upfront would let the original Request be GC'd:

Suggested change

	const __syntheticReq = new Request(request.url, { method: "GET" });
	const __syntheticReq = new Request(request.url, { method: "GET" });

More precisely, a local const __revalUrl = request.url; before the regen closure, then new Request(__revalUrl, { method: "GET" }) inside it, would avoid the capture. Not blocking — on most deployments the original request is small and short-lived — but worth noting for memory-sensitive Workers deployments.

[NathanDrake2406]

Fixed — hoisted request.url into const __revalUrl before the closure so the original Request can be GC'd during background regen.

[ask-bonk]

Nice addition from the earlier review feedback. One edge case this doesn't cover: what happens when the cache entry was populated by a GET but HEAD is the first request (cold cache). Right now this test always warms the cache with GET first, so the HEAD test only verifies the HIT read path with isAutoHead body stripping — never the MISS write path where isAutoHead strips the body before storing. Not blocking for this PR.

[ask-bonk]

Review submitted successfully. Here's a summary of what I found:

Overall assessment: Solid implementation, no hard blockers. The PR correctly reuses the existing ISR infrastructure with a "route" cache key suffix, and the previous review rounds already caught and fixed the two real issues (force-dynamic bypass and header filter inconsistency).

One actionable suggestion:

• The background regen closure captures the entire request object just to read .url. On memory-constrained Workers, extracting request.url into a local variable before the closure would allow the original Request to be GC'd sooner.

Minor observations (non-blocking):

• No test coverage for HEAD-as-first-request (cold cache MISS write path with isAutoHead)
• The typeof handlerFn === "function" guard in the ISR read block is redundant with the later check
• Test ordering dependency between the 7 route handler ISR tests sharing cache state (the STALE test already handles this correctly)
• Cosmetic inconsistency between page ISR guard (redundantly re-checks Infinity) and route handler ISR guard (trusts the null check)

github run

[james-elicx]

/bigbonk re-review

[ask-bonk]

Review: Route Handler ISR Caching

Solid implementation. The approach of reusing existing ISR infrastructure (__isrGet/__isrSet/__triggerBackgroundRegeneration) with a new "route" cache key suffix is clean and well-tested. The guard conditions are correct, the MISS → HIT → STALE lifecycle is thoroughly tested, and the fixes from earlier review rounds (force-dynamic bypass, header filter consistency, request.url hoisting) all look good.

I have one correctness concern and two gaps worth tracking. Nothing is a hard blocker.

Missing test for the force-dynamic guard

The handler.dynamic !== "force-dynamic" check was added in d4c52b5 (good catch from the earlier review), but there's no test covering it. A route handler exporting both revalidate = N and dynamic = "force-dynamic" should never get ISR-cached. Without a test, a future refactor could remove the guard and nothing would catch it. Page-level force-dynamic has 4 tests (lines 910-959); route handlers should have at least one.

Background regen closure captures url.searchParams from outer scope

Line 2086 passes url.searchParams into setNavigationContext() inside the regen closure. Unlike __revalUrl (hoisted to a string), url.searchParams is a live reference to the URLSearchParams from the outer url object (line 1504). This means the regen closure pins the entire URL object in memory — the same class of issue the request.url hoisting fixed for the Request object. On Workers with tight memory constraints this is worth addressing.

More importantly, the regen closure doesn't need to set navigation context at all for route handlers — route handlers receive the Request object directly and read URL/search params from it, not from the navigation context. The page ISR regen sets nav context because React components access it via useSearchParams() etc., but route handlers don't use those hooks. If the nav context is truly needed for some edge case (e.g., a route handler importing next/navigation server-side), hoisting url.searchParams to a local string-based copy before the closure would be more correct.

Observation: test ordering dependency

The 7 route handler ISR tests share a single production server and all hit /api/static-data (revalidate=1). The tests are order-dependent — the HIT test relies on the MISS test having warmed the cache, and the STALE test re-warms explicitly. This works today but is fragile for future test additions. A comment at the top of the block noting the shared cache state dependency would help maintainability.

[ask-bonk]

This captures the outer url.searchParams (a live URLSearchParams from const url = new URL(request.url) at line 1504), pinning the entire URL object in memory for the lifetime of the regen — the same class of issue the request.url → __revalUrl hoisting fixed for the Request object.

More substantively: route handlers receive the Request directly and read URL/query params from it, not from the navigation context. The page ISR regen needs nav context because React components use useSearchParams(), but route handler code doesn't use those hooks. If nav context is still needed here (e.g., for next/navigation server-side imports in a route handler), consider hoisting the search params to a string copy:

Suggested change

	setNavigationContext({ pathname: cleanPathname, searchParams: url.searchParams, params: __revalParams });
	const __revalSearch = new URLSearchParams(url.searchParams);
	setNavigationContext({ pathname: cleanPathname, searchParams: __revalSearch, params: __revalParams });

This way the outer URL can be GC'd during background regen.

[NathanDrake2406]

Fixed — hoisted url.searchParams into const __revalSearchParams = new URLSearchParams(url.searchParams) before the closure, same pattern as the request.url fix.

[ask-bonk]

Missing test: handler.dynamic !== "force-dynamic" was added as a guard in d4c52b5 but has no test coverage. Consider adding something like:

it("route handler ISR: force-dynamic handler is not cached", async () => {
  // Fixture: /api/force-dynamic-revalidate exports revalidate=60 and dynamic="force-dynamic"
  const res1 = await fetch(`${baseUrl}/api/force-dynamic-revalidate`);
  const res2 = await fetch(`${baseUrl}/api/force-dynamic-revalidate`);
  expect(res1.headers.get("x-vinext-cache")).toBeNull();
  expect(res2.headers.get("x-vinext-cache")).toBeNull();
});

Page-level force-dynamic has 4 dedicated tests (lines 910-959); the route handler guard should have at least one to prevent regression.

[NathanDrake2406]

Added. New fixture at app/api/force-dynamic-revalidate/route.ts exports both revalidate=60 and dynamic="force-dynamic". Test verifies neither request gets an x-vinext-cache header.

[ask-bonk]

Nit: This typeof handlerFn === "function" check in the ISR read guard is redundant — if handlerFn is not a function, we'd skip the ISR write block too (line 2122 checks typeof handlerFn === "function" again) and fall through to the 405 response. Not harmful, just unnecessary. Consistent with the defensive style though, so not blocking.

[ask-bonk]

Nit: Worth adding a comment here noting that these tests are order-dependent — they share a single production server and the /api/static-data cache state persists across tests. The HIT test depends on MISS having run first. The STALE test handles this correctly by re-warming, but future test additions need to be aware of the shared cache state.

[NathanDrake2406]

Added order-dependency comment to the test block.

[ask-bonk]

Review submitted successfully. Here's a summary of what I found:

Overall assessment: Clean implementation that correctly reuses the existing ISR infrastructure. The guard conditions, MISS → HIT → STALE lifecycle, and fixes from earlier review rounds are all correct.

Key findings:

1. Missing test for force-dynamic guard — The handler.dynamic !== "force-dynamic" check was added during review but has no test coverage. Page-level force-dynamic has 4 tests; route handlers should have at least one to prevent regression.

2. Background regen closure captures url.searchParams — The regen closure at line 2086 captures a live URLSearchParams reference from the outer scope, pinning the URL object in memory. This is the same class of issue that was already fixed for request.url → __revalUrl. Route handlers don't actually need navigation context (they read from the Request object directly), but if it's kept, the search params should be hoisted to a copy before the closure.

3. Test ordering dependency (non-blocking) — The 7 route handler ISR tests share cache state through a single production server. A comment noting this would help future maintainability.

4. Redundant typeof handlerFn === "function" check (nit) — The guard at line 2052 is redundant with the later check at line 2122, but consistent with the defensive coding style.

github run

[james-elicx]

/bigbonk re-review

[ask-bonk]

Review: Route Handler ISR Caching

This is a well-structured implementation that correctly extends the existing ISR infrastructure to route handlers. The approach of reusing __isrGet/__isrSet/__triggerBackgroundRegeneration with a new "route" cache key suffix is clean, and the guard conditions have been refined across multiple review rounds to cover force-dynamic, header filter consistency, and closure capture hygiene.

What looks good

• Guard conditions are correct and complete: production-only, GET/HEAD-only, Infinity filter, force-dynamic bypass, dynamic API bypass, handler Cache-Control bypass. The force-dynamic guard was added during review with test coverage — good.
• Clean context isolation in background regen: fresh Headers/cookies Map, synthetic request from hoisted __revalUrl, copied URLSearchParams via new URLSearchParams(url.searchParams) — all preventing user-specific data from leaking into cached responses.
• consumeDynamicUsage() in the regen closure is an improvement over the page ISR regen pattern. If a handler conditionally calls headers() during regen, the write is correctly skipped.
• Cache write before cookie/middleware transforms is the right ordering — those are request-specific.
• Test coverage is solid: MISS, HIT, POST bypass, dynamic bypass, handler Cache-Control bypass, force-dynamic bypass, STALE lifecycle with polling, auto-HEAD, and dev-mode guard. The polling pattern for STALE (2s sleep + 5s deadline with 500ms intervals) is resilient.
• Snapshot changes are mechanical reflections of the source change across all template variants.

Minor observations (non-blocking)

1. The MISS path sets Cache-Control on the response (line 2139) before the cache write block clones it (line 2154). The clone picks up cache-control and x-vinext-cache, but the header filter at line 2163 strips both — so the stored entry is clean. Correct but worth a mental note if the ordering ever changes.

2. The typeof handlerFn === "function" check in the ISR read guard (line 2052) is redundant with the same check at line 2123. Harmless — consistent with defensive coding style.

3. The background regen still sets setNavigationContext() with pathname/searchParams/params. Route handlers receive the Request directly and don't use navigation hooks, so this is arguably unnecessary for route handlers specifically. But it's defensive and ensures any shared utility code that reads nav context won't blow up. Fine to keep.

Looks good to merge.

[ask-bonk]

Nit: ordering detail worth noting — Cache-Control is set on the live response here (line 2139), then X-Vinext-Cache is set at line 2153, then response.clone() at line 2154 captures both headers. The clone's header filter (line 2162-2163) strips both x-vinext-cache and cache-control, so the stored entry is clean.

This is correct, but the correctness depends on the filter being exhaustive. If a future change adds another ISR-specific header between lines 2139–2153, it would need to be added to the filter too. A brief comment at line 2154 noting this dependency would help:

Suggested change

	response.headers.set("cache-control", "s-maxage=" + revalidateSeconds + ", stale-while-revalidate");
	const __routeClone = response.clone(); // headers filtered below — update filter if new ISR headers are added above

[ask-bonk]

Good — this test was added to cover the handler.dynamic !== "force-dynamic" guard from d4c52b5. The test verifies that neither request gets x-vinext-cache, which correctly asserts that ISR is fully bypassed (no MISS, no HIT).

One thought: this test also implicitly validates that Cache-Control is not set to s-maxage=60, stale-while-revalidate (since force-dynamic should suppress the ISR header emission too). If you wanted to be thorough, asserting res1.headers.get("cache-control") is either null or not the ISR pattern would make the test more explicit. Not blocking.

[ask-bonk]

Review submitted. Here's a summary:

Approved — this is a solid implementation that correctly extends the existing ISR infrastructure to App Router route handlers.

Key strengths:

• Complete guard conditions: production-only, GET/HEAD-only, Infinity filter, force-dynamic bypass, dynamic API bypass, handler Cache-Control bypass
• Clean context isolation in background regen (fresh headers/cookies, hoisted URL string, copied URLSearchParams)
• consumeDynamicUsage() in regen is an improvement over the page ISR pattern
• Cache write correctly placed before cookie/middleware transforms
• Good test coverage across the full MISS/HIT/STALE lifecycle plus all bypass scenarios

Two minor inline comments (non-blocking):

1. A comment suggestion on the clone line noting that the header filter must stay in sync with any new ISR-specific headers added above it
2. The force-dynamic test could also assert that the Cache-Control header doesn't have the ISR pattern, for extra explicitness

github run