Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@1962

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

commit: 818fb43

/bigbonk review for correctness issues

Review posted successfully.

Summary

I reviewed PR #1962 (route prefetch reuse through planner) for correctness and found one behavioral regression:

Unconditional prefetch probe on the visited-response-hit path (app-browser-entry.ts:1787-1795)

hasPrefetchCandidate is now computed unconditionally, including when cachedRoute is non-null. In the prior code, the prefetch cache was only touched after the if (cachedRoute) { ... return/continue } branch — so on a visited-response cache hit, the prefetch cache was never probed or mutated for that navigation.

After this change, hasPrefetchCacheEntryForNavigation(..., { notifyInvalidation: false }) runs before the reuse decision. Since classifyNavigationReuse prefers reuseVisitedResponse, the prefetch is never consumed — but the probe has already silently evicted any stale/expired prefetch entry that the old code would have left in place, plus it adds wasted work on the documented cache-hit soft-nav fast path.

This contradicts the PR's stated "no intended user-visible behavior change" and "silent stale-prefetch eviction for navigation probes preserved" claims, and it isn't covered by the new tests (they exercise the planner in isolation, not the executor's unconditional probe). I posted a suggestion to gate the probe on cachedRoute === null.

The rest of the change (planner decision types, ordering, trace codes, and the notifyInvalidation option) is correct and well-tested.

github run

/bigbonk review for issues

Review posted successfully.

Summary

I reviewed PR #1962 (route prefetch reuse through planner) for correctness. The refactor is clean and well-tested. Key findings:

Resolved since the prior review: The earlier bonk concern (unconditional prefetch probe on the visited-response-hit path) was fixed by commit 92b083d, which adds classifyNavigationPrefetchProbe. On a visited-response hit, visitedResponse.status === "available", so the probe returns skip/visitedResponseAvailable and no longer touches the prefetch cache. The notifyInvalidation: false option correctly preserves silent stale-prefetch eviction, and in-flight (pending) prefetch reuse is intact.

One remaining divergence (posted inline at app-browser-entry.ts:1904): Slot-mismatched exact-key prefetch entries are no longer evicted on the cache-miss path. The old code ran consumePrefetchResponseForNavigation unconditionally for every non-refresh/non-bypass miss, which reaches the exact-key slot-mismatch eviction branch in consumePrefetchResponse (navigation.ts:962-968). The new code gates the consume on hasPrefetchCandidate, which is derived from findPrefetchCacheEntryForNavigation — a function that deliberately skips slot-incompatible exact entries. So that eviction branch is now unreachable, leaving slot-mismatched entries in the cache (eventually reclaimed by the FIFO/expiry sweep). It's not a hard correctness bug, but it's a real divergence from the "no intended user-visible behavior change" claim and isn't covered by the new tests. I suggested either preserving the eviction or documenting it as deliberate, plus a regression test.

Everything else — planner decision types, ordering, trace codes, and the notifyInvalidation option — is correct.

github run

/bigbonk review for issues

Review posted successfully.

Summary

I reviewed PR #1962 (route prefetch reuse through planner) for correctness against the current head (970440523, which is newer than what the two prior bonk reviews saw). The refactor is solid — clean planner extraction, faithful decision ordering, passing tests/lint/types.

Key findings:

1. The earlier bonk review's slot-mismatch-eviction concern was incorrect, and the author's refutation is right. I traced it: the old executor miss path used the async consumePrefetchResponseForNavigation, which bails at findPrefetchCacheEntryForNavigation returning null for slot-mismatched exact-only entries — it never reached the sync consumePrefetchResponse eviction branch. So that branch was unreachable on the executor path before this PR.

2. One real (but benign) new divergence: The author's final fix commit added evictIncompatibleExactEntry: true to the probe. This doesn't preserve prior behavior — it adds an eviction the old executor never performed, in the opposite direction from the earlier reviewer's claim. It's defensible (cleans an unreusable entry, condition matches the canonical sync path), but it contradicts both the PR's "no intended user-visible behavior change" claim and the author's own "No code change needed here" reply. I posted an inline note asking them to either drop the option (faithful preservation) or document it as deliberate, plus a minor asymmetry note about the hardcoded notifyInvalidation: false in the new eviction.

Everything else — planner types, ordering, trace codes, classifyNavigationPrefetchProbe gate, traverse handling, and the prefetch fallback replanning — is correct. This is a documentation/intent nit, not a blocker.

github run

/bigbonk review for issues

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

/bigbonk review for issues

Review posted successfully.

Summary

I reviewed PR #1962 at the current head (818fb43) for correctness. The refactor is clean and faithfully preserves prior executor behavior. Both prior bonk concerns are now resolved at this head:

1. Unconditional prefetch probe on the visited-hit path (1st review) — fixed by classifyNavigationPrefetchProbe in 92b083d. On a visited hit, the probe returns skip/visitedResponseAvailable and never touches the prefetch cache.

2. evictIncompatibleExactEntry adding a new eviction (3rd review) — that option (added in 970440523) was removed in the final commit 818fb43. I confirmed via git show that it's gone at HEAD; the only navigation.ts change now is the notifyInvalidation option, so the slot-mismatch path is byte-identical to main. The author's earlier refutation was correct.

I independently re-verified the behavioral parity points that matter:

• Expired non-pending entry: old and new both delete with notify=false (locked down by the new test).
• In-flight (pending) prefetch reuse: intact (probe returns true without awaiting, consume awaits).
• Bypass / slot-mismatch visited cache: synthetic missing + evict decisions reproduce old delete-on-read semantics.
• traverse vs navigate gating in classifyNavigationReuse matches the old inline branching.

vp check and the targeted tests pass locally.

I posted two non-blocking nits inline: (1) the planner .trace fields are computed but never consumed in the executor (dead allocation on the soft-nav path), and (2) routeManifest is now computed eagerly even on the visited-hit path — harmless since it's a trivial property read, but flagged so it isn't mistaken for a regression against the deferral comment. No correctness blockers.

github run