/bigbonk review for issues

Open in StackBlitz

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

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

commit: 7d6106c

Performance benchmarks

Compared 7d6106c against base cf9d601 using alternating same-runner rounds. Next.js was unchanged and skipped.

4 improved · 0 regressed · 2 within ±1.5%

View detailed results and traces

_{🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head}

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

/bigbonk review for issues

/bigbonk review

I reviewed the diff against the four request-handling sites that AGENTS.md says must stay in sync. The Node prod-server change itself is correct and well-targeted, but the same fix is missing from the two Cloudflare Workers entries — which is the primary deployment target.

Blocking: Workers entries still short-circuit missing assets before middleware

The PR description says it keeps "App Router and Pages Router production server behavior aligned," and the goal is for middleware to run on missing /_next/static/* requests. But that fix was only applied to server/prod-server.ts (the Node prod servers). The two Cloudflare Workers entries still return a plain-text 404 for any asset-shaped path before middleware runs:

• App Router (Workers): packages/vinext/src/server/app-router-entry.ts:104-106

  if (isNextStaticPath(url.pathname, __workerBasePath, __workerAssetPathPrefix)) {
    return notFoundStaticAssetResponse();
  }

  This executes before rscHandler(request) (line 132), so middleware never sees a missing /_next/static/* request on Workers.

• Pages Router (Workers): packages/vinext/src/deploy.ts:625-627

  if (isNextStaticPath(pathname, basePath, assetPathPrefix)) {
    return notFoundStaticAssetResponse();
  }

  Same problem — this runs before runPagesRequest (and therefore before middleware) on line 717.

Per AGENTS.md, Cloudflare Workers is the primary deployment target and dev/prod parity is required; the parity checklist explicitly names cloudflare/worker-entry.ts/deploy.ts alongside the prod servers. As written, the exact bug this PR fixes for Node still exists on Workers: a middleware that rewrites a missing /_next/static/chunks/... request will work in the Node prod server but not when deployed to Workers. That is a silent dev/prod divergence — precisely the class of bug AGENTS.md warns about ("Do not leave known bugs as 'follow-ups' — fix them in the same PR").

These two sites need the same treatment: drop the early notFoundStaticAssetResponse() for missing assets, let the request reach middleware/routing, and convert a resulting 404 back to the canonical plain-text static 404. (Valid assets are still served by the ASSETS binding before the worker runs, so the fast path is preserved automatically on Workers.)

Test coverage gap

The new assertions are good and correctly model the three-part contract, but they only exercise one of the four handlers — the Pages Router Node prod server (startProdServer with disableAppRouter: true, test at tests/asset-prefix.test.ts:691). There is no coverage for:

1. the App Router Node prod server path that this PR also modified (startAppRouterServer, prod-server.ts:1437), and
2. either Cloudflare Workers entry.

Because the only new test runs against the one handler that was fixed, CI stays green while the Workers regression ships unnoticed. At minimum, please add an App Router prod-server case mirroring this one; ideally add a Workers-entry case so the parity is actually enforced.

Things that look correct

• Node Pages Router (prod-server.ts:1674, :1863) and Node App Router (prod-server.ts:1336, :1437) changes are sound: the missingBuildAsset flag is only consulted on the static-miss path (successful assets early-return before middleware, preserving the no-middleware fast path), and cancelResponseBody(response) correctly drains the routed 404 body before emitting the canonical plain-text response.
• The behavioral premise matches Next.js — the cited middleware-general deploy-suite assertion (should be able to rewrite on _next/static/chunks/pages/ 404) confirms Next.js lets middleware rewrite a missing static chunk before its plain-text asset 404.
• Targeted test passes locally (tests/asset-prefix.test.ts -t "Pages Router with basePath alone serves assets").

Summary

The Node-side fix is correct, but shipping it without the matching app-router-entry.ts and deploy.ts changes makes the PR title ("run on missing build assets") true only off-Workers. Recommend updating both Workers entries (and adding coverage that actually exercises them) before merge.

Posted my review to PR #2242. Summary of findings:

Blocking issue: incomplete fix for the primary deployment target

The PR fixes the Node prod servers (prod-server.ts) so middleware runs on missing /_next/static/* requests, but the same short-circuit still exists in both Cloudflare Workers entries — which AGENTS.md designates as the primary deployment target and part of the required parity set:

• app-router-entry.ts:104-106 returns notFoundStaticAssetResponse() before rscHandler runs middleware.
• deploy.ts:625-627 returns notFoundStaticAssetResponse() before runPagesRequest runs middleware.

Result: a middleware rewrite of a missing static chunk works on the Node server but silently fails when deployed to Workers — exactly the dev/prod divergence AGENTS.md warns against, left as an in-scope "known bug."

Test gap

The new assertions only exercise the Pages Router Node prod server. There's no coverage for the App Router prod server (also modified here) or either Workers entry, so CI stays green while the Workers regression ships unnoticed.

Correct parts

• The Node-side missingBuildAsset flag logic, fast-path preservation, and cancelResponseBody draining are all sound.
• The behavioral premise matches Next.js (confirmed by the cited deploy-suite assertion).
• The targeted test passes locally.

Recommendation: apply the same change to both Workers entries and add coverage that actually exercises them before merge.

github run

/bigbonk review for issues

/bigbonk review

Re-reviewed e1882dd (unchanged since the prior review). I independently verified every claim against the four parity sites in AGENTS.md. The prior /bigbonk findings are accurate and still stand — the Node fix is correct, but the same fix is missing from both Cloudflare Workers entries (the primary deployment target).

Blocking: Workers entries still short-circuit missing assets before middleware

Both Workers entries return a plain-text 404 for any asset-shaped path before middleware runs, so the exact bug this PR fixes for Node still ships on Workers:

• App Router (Workers): packages/vinext/src/server/app-router-entry.ts:104-106

  if (isNextStaticPath(url.pathname, __workerBasePath, __workerAssetPathPrefix)) {
    return notFoundStaticAssetResponse();
  }

  This runs before rscHandler(request, ctx) at line 132, so middleware never sees a missing /_next/static/* request.

• Pages Router (Workers): packages/vinext/src/deploy.ts:625-627

  if (isNextStaticPath(pathname, basePath, assetPathPrefix)) {
    return notFoundStaticAssetResponse();
  }

  This runs before runPagesRequest(request, deps) at line 717 (and therefore before middleware).

A middleware that rewrites or responds to a missing /_next/static/chunks/... request will work in the Node prod server but silently fail on Workers — exactly the dev/prod divergence AGENTS.md warns against ("Do not leave known bugs as 'follow-ups' — fix them in the same PR"). The PR title ("run on missing build assets") and the description's "keep App Router and Pages Router production server behavior aligned" are only true off-Workers as written.

Both sites need the same treatment as the Node fix: drop the early notFoundStaticAssetResponse() for missing assets, let the request reach middleware/routing, then convert a resulting 404 back to the canonical plain-text static 404. Valid assets are served by the ASSETS binding before the worker runs, so the no-middleware fast path is preserved automatically.

Test coverage gap

The new assertions correctly model the three-part contract (existing assets 200 with no middleware header, middleware can handle a missing asset, unhandled miss is 404 text/plain "Not Found"), but they only exercise one of the four handlers — the Pages Router Node prod server (tests/asset-prefix.test.ts:691, startProdServer with disableAppRouter: true). There is no coverage for:

1. the App Router Node prod server path this PR also modified (prod-server.ts:1437), and
2. either Cloudflare Workers entry.

Because the only new test hits the one handler that was fixed, CI stays green while the Workers regression ships unnoticed. At minimum add an App Router prod-server case; ideally add a Workers-entry case so the parity is actually enforced.

Verified correct

• Node fix is sound. App Router (prod-server.ts:1325, :1437) sets missingBuildAsset only after a confirmed static miss; Pages Router (prod-server.ts:1674, :1860) computes it from pagesAssetLookup !== null but a successful serve early-returns first, so the net behavior matches. In both, the flag is consulted only on the static-miss path, the request now reaches rscHandler / runPagesRequest (and thus middleware), and cancelResponseBody(response) correctly drains the routed 404 body before emitting the canonical plain-text response.
• Behavioral premise matches Next.js — the cited middleware-general assertion (should be able to rewrite on _next/static/chunks/pages/ 404) confirms Next.js lets middleware rewrite a missing static chunk before its plain-text asset 404.

Recommendation

Apply the same fix to app-router-entry.ts and deploy.ts, and add coverage that exercises the App Router prod server and at least one Workers entry, before merge.

Posted my review to PR #2242. I independently verified every finding against the four parity sites in AGENTS.md (the PR is unchanged at e1882dd).

Summary

Blocking: incomplete fix for the primary deployment target (Workers)

The Node prod-server fix is correct, but the same short-circuit still exists in both Cloudflare Workers entries:

• app-router-entry.ts:104-106 returns notFoundStaticAssetResponse() before rscHandler (line 132) runs middleware.
• deploy.ts:625-627 returns notFoundStaticAssetResponse() before runPagesRequest (line 717) runs middleware.

Result: a middleware rewrite of a missing /_next/static/* request works on Node but silently fails when deployed to Workers — the dev/prod divergence AGENTS.md explicitly warns against leaving as a follow-up.

Test gap

The new assertions only exercise the Pages Router Node prod server (tests/asset-prefix.test.ts:691). No coverage for the App Router Node prod server (also modified) or either Workers entry, so CI stays green while the Workers regression ships.

Verified correct

• The Node-side missingBuildAsset flag logic, fast-path preservation, and cancelResponseBody draining are all sound in both App Router (prod-server.ts:1437) and Pages Router (prod-server.ts:1860).
• The behavioral premise matches Next.js (the cited middleware-general deploy-suite assertion).

Recommendation: Apply the fix to both Workers entries and add coverage that exercises them before merge.

github run

/bigbonk review for issues

Review posted to PR #2242.

Summary

I re-reviewed the PR at its current head (7d6106c), which includes a new follow-up commit that wasn't present when the earlier /bigbonk reviews ran. That commit ("fix(middleware): preserve missing asset rewrites on workers") resolves both prior blocking concerns:

Prior blocking issue — fixed ✅

• A new shared helper finalizeMissingStaticAssetResponse (worker-utils.ts:39-46) is now wired into both Cloudflare Workers entries (app-router-entry.ts and deploy.ts) plus the example worker. Missing /_next/static/* requests now reach middleware on Workers, then get converted back to the canonical plain-text 404 if unhandled — matching the Node fix.

Prior test-gap — closed ✅

• All four parity sites now have coverage: App Router Node prod (asset-prefix.test.ts:633), Pages Router Node prod (:697), App Router Workers (ssr.spec.ts), Pages Router Workers (middleware.spec.ts), plus a focused unit test for the helper.

Verified locally: deploy.test.ts (282 passed), both asset-prefix prod-server tests passed, vp check clean on all four source files.

Non-blocking observations (noted in the review): the two Node prod paths use different predicates for asset-shape detection (functionally equivalent), and a few early-return paths skip finalization but can't be reached by asset-shaped requests today.

Verdict: LGTM — the follow-up commit makes the PR title/description accurate on Workers as well as Node.

github run