Review: _next/data JSON endpoint implementation

This is a well-structured PR. The helper is cleanly separated, the dev/prod paths are symmetric, and the test coverage is solid. A few things to address:

Issues

1. getStaticProps data requests bypass ISR cache but don't emit JSON (pages-page-data.ts:367-384, pages-page-data.ts:387-451)

The ISR cache bypass for isDataReq is correct — you can't serve cached HTML as JSON. But the current logic only short-circuits to JSON in the generated entry's _renderPage for gSSP pages (lines 617-629 of pages-server-entry.ts). For getStaticProps pages, resolvePagesPageData falls through to kind: "render", and then _renderPage hits the isDataReq branch and emits JSON.

This works, but there's a subtle gap: when getStaticProps returns notFound: true, the current code calls buildPagesDataNotFoundResponse() (which returns application/json with {}), but when getStaticPaths with fallback: false rejects the path, it calls buildPagesNotFoundResponse() which returns text/html. For a data request against a fallback: false path that isn't listed, the client would get HTML instead of JSON.

Consider gating buildPagesNotFoundResponse() at pages-page-data.ts:304-307:

if (!isValidPath) {
  return {
    kind: "response",
    response: options.isDataReq ? buildPagesDataNotFoundResponse() : buildPagesNotFoundResponse(),
  };
}

2. Dev server: isDataReq for getStaticProps ISR cache HIT/STALE paths short-circuits to HTML (dev-server.ts:587-606, dev-server.ts:609-615)

In dev-server.ts, the ISR cache HIT and STALE code paths at lines 587-606 and 609-615 have !isDataReq guards that skip the cached HTML response for data requests (good). But the subsequent getStaticProps MISS path at line 762+ doesn't have a JSON short-circuit before the full render. The isDataReq JSON short-circuit at line 814 covers this since it runs after getStaticProps completes and pageProps is populated. This works correctly but it's worth verifying that the ISR cache-fill path (lines 1059-1082) is also correctly skipped for data requests — it appears to be, since isrRevalidateSeconds is set correctly.

Confirmed: this path is fine.

3. Dev server hardcodes "development" as fallback buildId (index.ts:2867)

const devBuildId = process.env.__VINEXT_BUILD_ID ?? "development";

This is fine for the common case, but the test uses this same pattern:

const DEV_BUILD_ID = process.env.__VINEXT_BUILD_ID ?? "development";

The generated server entry embeds buildId from nextConfig.buildId. If next.config.mjs sets generateBuildId, the dev server index.ts fallback "development" won't match the entry's actual buildId. This isn't a regression (the endpoint didn't exist before), but it's a parity gap with how prod handles it. The prod server reads pagesBuildId from the server entry's export const buildId — the dev server should similarly read the resolved buildId from the plugin's nextConfig.buildId rather than relying on the define.

4. Missing _next/data test for getStaticProps page (both dev and prod)

All integration tests use /ssr which has getServerSideProps. Next.js also serves _next/data responses for getStaticProps pages (the client router uses it for any page with a data-fetching method). Adding a test for e.g. /_next/data/<buildId>/blog/hello-world.json (which has getStaticProps + getStaticPaths) would strengthen confidence that the GSP path also emits JSON correctly.

5. Entry template: inline JSON envelope logic could be extracted (pages-server-entry.ts:611-629)

Per AGENTS.md guidance ("generated code should stay thin"), the JSON envelope construction at lines 611-629 duplicates some of what buildNextDataJsonResponse already does. The generated entry builds its own Response manually rather than calling __buildNextDataJsonResponse. This means:

• The pages-data-route.ts helper buildNextDataJsonResponse is only used in tests (never actually called in production code paths).
• If the envelope shape changes, two places need updating.

Consider using __buildNextDataJsonResponse (which is already imported in the entry) here instead, passing the gsspRes.headers as init.headers.

Nits

6. pages-data-route.ts:76 — The endsWith("/index") check is correct but could also match a page literally named "index" nested under another segment (e.g., /_next/data/<buildId>/blog/index.json → /blog instead of /blog/index). This matches Next.js behavior (denormalizePagePath), just noting it as intentional.

7. The dev test uses process.env.__VINEXT_BUILD_ID ?? "development" which works because the test runs in the same Node process as the dev server. But this is fragile — if the dev server starts using the resolved nextConfig.buildId (per point 3 above), the test would need updating. Consider extracting the dev buildId from the server entry module or having the fixture's next.config.mjs pin a known buildId.

What looks good

• Clean separation: pages-data-route.ts is well-documented, focused, and independently unit-tested.
• Dev/prod parity: Both paths normalize the URL before middleware, both pass isDataReq through, both handle stale buildId as JSON 404.
• Middleware visibility test is exactly the right assertion — proving middleware sees the page path, not the raw _next/data URL.
• ISR cache bypass is correct: data requests skip cached HTML and don't pollute the HTML cache with JSON.
• The prod server handles stale buildId early (before middleware), which is the right thing to do.

Re-review: All previous findings addressed

The fix commit (07bfd5d) addresses all five findings from the previous review:

1. Entry template delegation — _renderPage now calls __buildNextDataJsonResponse instead of constructing the JSON response inline. The envelope shape lives in one place.
2. getStaticPaths fallback:false JSON 404 — pages-page-data.ts:303-313 now gates on isDataReq, returning buildPagesDataNotFoundResponse() for data requests and buildPagesNotFoundResponse() for HTML.
3. Dev buildId resolution — index.ts:2872-2873 reads nextConfig?.buildId first, falling back to the env var and "development". Tests now use the fixture's generateBuildId value ("test-build-id").
4. Missing getStaticProps test coverage — Added for both dev and prod.
5. Dev server notFound guards — isDataReq checks added for gSSP.notFound (line 526), gSP.notFound (line 801), and getStaticPaths fallback:false (line 448) in dev-server.ts.

Two items to track as follow-ups (not blocking)

1. Dev server: stale buildId returns HTML, not JSON 404

When isNextDataPathname matches but parseNextDataPathname returns null (wrong buildId), the dev server falls through to the dot-extension check at index.ts:2889 which calls next() — Vite serves an HTML 404. The prod server (prod-server.ts:1570-1578) correctly returns a JSON 404 via buildNextDataNotFoundResponse(). This is a dev/prod parity gap. There's a prod test for stale buildId but no corresponding dev test. Low priority since stale buildIds in dev are uncommon (the client and server restart together), but worth aligning for consistency.

2. pages-page-data.ts: gSSP.notFound and gSP.notFound return JSON for all requests in prod

Lines 354-358 and 479-483 unconditionally call buildPagesDataNotFoundResponse() regardless of isDataReq. For HTML navigations through the prod entry where gSSP/gSP returns { notFound: true }, the response is application/json {} instead of an HTML 404 page. The dev server is fine — it handles notFound before calling resolvePagesPageData with its own isDataReq-gated logic. This is technically a pre-existing bug (before this PR it returned plain text "404" which was also wrong for HTML), but the change from text/plain to application/json makes it slightly more visible. The fix is straightforward: gate on options.isDataReq at both call sites, same as the getStaticPaths path at line 309.

What's good

• Clean helper (pages-data-route.ts) — well-documented, independently tested, no unnecessary coupling.
• Dev/prod symmetry in the normalization-before-middleware pipeline.
• ISR cache bypass is correct and well-reasoned.
• The middleware visibility test (x-mw-pathname) is the right assertion for proving the normalization order.
• Stale buildId handling in prod returns JSON 404 before middleware, which is correct.
• The entry stays thin — delegates to typed helpers as AGENTS.md prescribes.

Approving. The two items above are minor and can be addressed in a follow-up.

Re-review: Both follow-ups addressed — approving

Commit e245bde addresses both remaining items from the prior review:

1. Dev stale buildId now returns JSON 404 — index.ts:2884-2889 returns res.writeHead(404, {"Content-Type": "application/json"}) + res.end("{}") when parseNextDataPathname returns null, matching the prod path. Test added at line 1176.

2. gSSP.notFound and gSP.notFound now gate on isDataReq — pages-page-data.ts:354-360 and 481-486 use the ternary options.isDataReq ? buildPagesDataNotFoundResponse() : buildPagesNotFoundResponse(), so HTML navigations get the HTML 404 page and data requests get JSON. Dev server handles the same cases at dev-server.ts:526 (gSSP) and dev-server.ts:801 (gSP).

What I verified

• Dev/prod parity: Both paths normalize _next/data before middleware, both return JSON 404 for stale buildId, both pass isDataReq through the render pipeline. The generated entry delegates to __buildNextDataJsonResponse (line 627) keeping it thin per AGENTS.md.
• ISR cache bypass: All four ISR cache hit/stale guards (pages-page-data.ts:374-380, 394-399, dev-server.ts:598-603, 621-626) include !isDataReq, correctly falling through to fresh data fetching.
• getStaticPaths fallback:false: Gated on isDataReq at pages-page-data.ts:309 and dev-server.ts:448.
• Test coverage: 14 unit tests for the helper, 7 dev integration tests (gSSP, gSP, middleware visibility, unknown page, fallback:false, stale buildId), 5 prod integration tests. Good coverage.

One non-blocking observation for future work

gSSP/gSP redirect returns HTTP 3xx for data requests — In Next.js, when getServerSideProps or getStaticProps returns { redirect } during a _next/data request, the server returns a JSON body with __N_REDIRECT and __N_REDIRECT_STATUS keys instead of an HTTP redirect. This lets the client router handle the redirect client-side without the browser following it transparently (which would lose the JSON content-type on the redirect target). The current code at pages-page-data.ts:344-351 and dev-server.ts:510-523 returns a raw HTTP redirect for both HTML and data requests. This is a pre-existing gap (the redirect behavior existed before this PR), so not blocking — just noting it for parity tracking.