feat: static pre-rendering at build time

[NathanDrake2406]

Summary

Implements build-time static pre-rendering for vinext, closing #9.

• vinext build with output: "export" now runs full static export to out/ (Phase 1)
• vinext build in normal mode pre-renders static pages to dist/server/pages/ (Phase 2)
• Production server serves pre-rendered HTML before falling through to SSR
• vinext start rejects export builds with a helpful error message

Phase 1: output: "export" support

Adds runStaticExport() orchestrator that connects the existing staticExportPages/staticExportApp functions to the CLI build pipeline. Loads config, detects router type, starts a temp dev server, scans routes, renders all static pages to out/.

Phase 2: Selective pre-rendering in normal builds

Adds prerenderStaticPages() that runs after production builds:

1. Starts a temp prod server in-process
2. Detects static routes via Vite dev server module inspection (skips getServerSideProps, force-dynamic, etc.)
3. Fetches each static route and writes HTML to dist/server/pages/
4. Prod server checks for pre-rendered HTML before SSR fallback

Files changed

File	Change
packages/vinext/src/build/static-export.ts	runStaticExport(), prerenderStaticPages(), collectStaticRoutes()
packages/vinext/src/cli.ts	Wire both phases into buildApp(), export build rejection in start()
packages/vinext/src/server/prod-server.ts	resolvePrerenderedHtml() + pre-render serving in both router handlers
tests/build-static-export.test.ts	14 tests for runStaticExport() (both routers)
tests/build-prerender.test.ts	7 tests for prod server serving + prerenderStaticPages()

Test plan

• pnpm run typecheck passes
• pnpm run lint passes
• 21 new tests pass (14 static export + 7 prerender)
• Full test suite: 2183 passed, 6 pre-existing App Router static export HTTP serving failures unchanged (fixed by fix(config): pass correct Next.js phase to function-form configs + instrumentation test coverage #273)
• CI checks (Lint, Typecheck, Vitest, Playwright E2E)

Closes #9

[pkg-pr-new]

Open in StackBlitz

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

commit: 8fd31ae

[southpolesteve]

/bigbonk review please

[ask-bonk]

Review: Static pre-rendering at build time

Solid feature implementation overall. The architecture is well-structured: runStaticExport() as the orchestrator for output: "export", prerenderStaticPages() for selective pre-rendering in normal builds, and the prod server integration to serve pre-rendered HTML. The refactoring of renderStaticPage to use try/finally for SSR context cleanup and the extraction of expandDynamicAppRoute are good improvements.

I found one security issue that should be fixed, plus several correctness and robustness concerns.

Security

• resolvePrerenderedHtml has a path prefix confusion bug — startsWith(resolvedDir) without a trailing separator means a directory named pages-evil adjacent to pages would pass the check (see inline comment).

Correctness concerns

• The getOutputPath traversal guard is dead code — path.posix.normalize resolves .. segments, so the includes('..') check after normalization can never be true.
• collectStaticRoutes calls getStaticProps({ params: {} }) on every non-dynamic page during route collection, purely to check revalidate. This has side effects (database queries, API calls) and runs with empty params which may not match the page's expectations.
• prerenderStaticPages spins up two servers (a Vite dev server for route collection + a prod server for fetching). The dev server uses ssrLoadModule against source files while the prod server uses the built output — if there's any mismatch between source and built state, the route list will be wrong.

Test quality

• Tests are well-structured but the prerender tests depend on hasServerEntry (a pre-existing build artifact). If the fixture hasn't been built, all 5 prod server tests silently skip via it.skipIf. This makes CI green even when the feature is untestable.

See inline comments for specific suggestions.

[ask-bonk]

Bug (security): The startsWith check is vulnerable to prefix confusion. If the pre-rendered directory is /dist/server/pages, then a resolved path like /dist/server/pages-evil/hack.html will pass the startsWith check because the string /dist/server/pages-evil/... starts with /dist/server/pages.

The fix is to append path.sep (or /) to the resolved directory before the prefix check:

Suggested change

	if (path.resolve(directPath).startsWith(resolvedDir) && fs.existsSync(directPath) && fs.statSync(directPath).isFile()) {
	if (path.resolve(directPath).startsWith(resolvedDir + path.sep) && fs.existsSync(directPath) && fs.statSync(directPath).isFile()) {

Same issue on line 306. In practice this is low-risk since the pathname comes from the URL which is already normalized upstream, but it's a defense-in-depth fix that should be applied.

[ask-bonk]

Same prefix confusion bug as line 301:

Suggested change

	if (path.resolve(indexPath).startsWith(resolvedDir) && fs.existsSync(indexPath) && fs.statSync(indexPath).isFile()) {
	if (path.resolve(indexPath).startsWith(resolvedDir + path.sep) && fs.existsSync(indexPath) && fs.statSync(indexPath).isFile()) {

[ask-bonk]

This traversal guard is dead code. path.posix.normalize resolves .. segments, so after normalization, normalized will never contain ... For example, /foo/../../etc/passwd normalizes to /etc/passwd.

Either:

1. Check before normalizing: if (urlPath.includes('..')) — but this is overly broad and rejects legitimate paths.
2. Check the resolved output path against the outDir boundary (which is the correct approach, but would need the outDir passed in).
3. Remove the dead check and add the boundary check at the write site in the callers.

Since the urlPath values come from route patterns and user-defined getStaticPaths/generateStaticParams, option 2 (checking fullPath.startsWith(resolvedOutDir + path.sep) at the write site) would be the most robust defense.

[ask-bonk]

Calling getStaticProps({ params: {} }) on every non-dynamic page during route collection is a side-effect concern. This function may make database queries, API calls, or other I/O — and it's being called with empty params purely to inspect the revalidate value. For dynamic routes with required params, this will likely throw (which the catch handles, but it means those routes are incorrectly skipped).

Consider a lighter-weight approach:

1. Check for export const revalidate = 0 as a module-level export (similar to how dynamic is checked on line 1085) instead of executing the function.
2. Or skip the getStaticProps revalidate check entirely for route collection — only getServerSideProps and force-dynamic are definitive signals that a page can't be pre-rendered. Pages with revalidate: 0 in getStaticProps are relatively rare.

[ask-bonk]

This function spins up two servers: a Vite dev server (via collectStaticRoutes on line 982) for route discovery, plus a production server (line 994) for HTML fetching. This is:

1. Slow — two full server startups during build.
2. Fragile — the dev server inspects source files while the prod server uses built output. If the source and build are out of sync (e.g., user edited a file after build), the route list won't match reality.

A more robust approach would be to either:

• Use only the prod server for both route detection and fetching (inspect the built server entry's route table), or
• Embed the list of static routes into the build output during the Vite build step so it's available without spinning up a dev server.

This doesn't block merging but is worth tracking as a follow-up for build performance.

[ask-bonk]

The config is loaded and resolved here, but runStaticExport (line 287) also loads and resolves the config internally. This means the config is parsed twice. Consider either:

1. Passing the already-resolved config to runStaticExport, or
2. Having the CLI only check resolvedConfig.output and letting the orchestrator handle everything else.

Also, if the config file has side effects or is expensive to load, double-loading could be a problem.

[ask-bonk]

Same double-loading issue as in buildApp(). The config is loaded here to check output: "export", then startProdServer will load its own config from the build output. Not a correctness issue but wasteful.

Also, the variable names startRawConfig / startResolvedConfig are a bit unusual. Since this is in its own function scope, rawConfig / resolvedConfig would be fine (no collision).

[ask-bonk]

The hasServerEntry check runs at module evaluation time, before any test setup. If the fixture hasn't been built (no dist/server/entry.js), all 5 prod-server tests silently skip. This means a CI configuration that doesn't pre-build fixtures will show green despite exercising nothing.

Consider either:

1. Adding a build step in beforeAll if the entry doesn't exist, or
2. Failing explicitly with a clear message like throw new Error('Fixture not built') instead of skipping, so it's visible when the test infrastructure is broken.

[ask-bonk]

Nit: This fs.existsSync(dir) check runs on every call to resolvePrerenderedHtml, which itself is called on every request (via line 987). Since hasPrerenderedPages is already checked at line 986 before calling this function, this inner check is redundant. It won't cause a bug but it's an unnecessary filesystem syscall per request.

[ask-bonk]

If generateStaticParams throws (e.g., because a required API/database is unavailable at build time), the error propagates unhandled to the caller. In staticExportApp the caller catches this (line 766), but in collectStaticRoutes (line 1101) there's also a catch. However, in expandDynamicAppRoute itself there's no error context added, so the error message won't mention which route failed. Consider wrapping with route context:

try {
  paramSets = await generateStaticParams({ params: {} });
} catch (e) {
  throw new Error(`generateStaticParams() failed for ${route.pattern}: ${(e as Error).message}`);
}

[ask-bonk]

The review has been submitted successfully. Here's a summary of the feedback I posted:

Key findings:

1. Security bug (should fix): resolvePrerenderedHtml in prod-server.ts uses startsWith(resolvedDir) without a trailing path separator, which is vulnerable to prefix confusion attacks (e.g., a sibling directory named pages-evil would pass the check).

2. Dead code: The getOutputPath traversal guard checks for .. after path.posix.normalize, but normalize already resolves those segments, making the check always false.

3. Side effects in route collection: collectStaticRoutes calls getStaticProps({ params: {} }) on every page to inspect the revalidate value. This executes user code with empty params during build, which could trigger unexpected database queries or API calls.

4. Double server startup in prerender: prerenderStaticPages spins up both a Vite dev server (for route discovery) and a production server (for fetching), which is slow and fragile if source/build are out of sync.

5. Silent test skipping: The prerender tests use it.skipIf(!hasServerEntry) which means all 5 production server tests silently skip if the fixture hasn't been pre-built.

6. Minor issues: Missing vite config extensions (mts, cjs), redundant fs.existsSync per request, double config loading in CLI, missing error context in expandDynamicAppRoute, and no guard for port 0 when the dev server fails to bind.

github run

[southpolesteve]

Thanks for submitting this! Im going to take a little extra actual human time with this one just to be sure its the approach we want to go with and has no future potential conflict with TPR. Will try and tackle it this weekend

[james-elicx]

/bigbonk review

[ask-bonk]

Review: Static pre-rendering at build time

This is a well-structured feature with clean separation between the three main pieces: runStaticExport() orchestrator, prerenderStaticPages() for normal builds, and the prod server integration. The refactoring of renderStaticPage to use try/finally for SSR context cleanup, the extraction of expandDynamicAppRoute, and the improved getOutputPath with boundary checking are all good improvements.

The previous bonk review identified several issues (path prefix confusion, dead code, side effects in route collection, etc.). I can see that some have been addressed — the security issue in resolvePrerenderedHtml now correctly uses resolvedDir + path.sep, and the getOutputPath traversal guard now does proper boundary checking. The collectStaticRoutes function was replaced with collectStaticRoutesFromSource which uses regex instead of executing user code — a solid improvement.

I have a few remaining concerns, mostly around the prod server integration ordering and missing test coverage for security-sensitive paths.

Summary of findings

1. Pre-rendered HTML placement in request pipeline (should fix) — served before afterFiles rewrites, which doesn't match Next.js execution order. Pre-rendered pages in Next.js are logically equivalent to page rendering (step 10), so they should respect afterFiles rewrites.
2. Missing Cache-Control header on pre-rendered responses — pre-rendered HTML is served without any caching headers, unlike Next.js which sets s-maxage / stale-while-revalidate for static pages.
3. middlewareRewriteStatus not applied to pre-rendered responses — if middleware sets a custom status via NextResponse.rewrite(url, { status: 403 }), the pre-rendered response ignores it and always returns 200.
4. Regex-based route classification can be fooled — the collectStaticRoutesFromSource approach is much better than executing user code, but the regex can false-positive on commented-out or string-literal exports.
5. Config headers from next.config.js not applied to pre-rendered responses — step 5 accumulates matched config headers into middlewareHeaders, but the pre-rendered path only spreads middlewareHeaders, not the config headers that were merged into it. Actually on re-read, config headers ARE merged into middlewareHeaders at step 5, so this is fine.
6. Test cleanup is brittle — afterAll cleanup in build-prerender.test.ts removes the pagesDir only if empty, but the index.html test creates and removes its file within the test. If the test fails mid-way, cleanup won't happen and subsequent runs may see stale files.

[ask-bonk]

Ordering concern: Pre-rendered HTML is served at step 7b, before afterFiles rewrites (step 9) and before SSR (step 10). In Next.js, pre-rendered pages are conceptually the same as page rendering — they sit at the page-matching layer, after all rewrites have been applied.

This means an afterFiles rewrite like { source: '/old-path', destination: '/new-path' } will be bypassed if /old-path has a pre-rendered HTML file. The pre-rendered file will be served instead of following the rewrite.

Consider moving the pre-rendered HTML check to just before step 10 (SSR page rendering), after afterFiles rewrites have been applied:

// ── 9. Apply afterFiles rewrites ...
// ── 9b. Pre-rendered HTML (after all rewrites) ──
// ── 10. SSR page rendering ──

This also ensures API routes (step 8) are never accidentally shadowed by a pre-rendered file named api.html or similar.

[ask-bonk]

Two issues with the pre-rendered response:

1. Missing Cache-Control header. Next.js serves pre-rendered (ISR) pages with Cache-Control: s-maxage=<revalidate>, stale-while-revalidate and static pages with s-maxage=31536000, stale-while-revalidate. Without any cache headers, CDNs won't cache these pages, negating much of the performance benefit of pre-rendering.

2. middlewareRewriteStatus is ignored. If middleware used NextResponse.rewrite(url, { status: 403 }), the SSR path (step 10) respects this via middlewareRewriteStatus ?? response.status, but the pre-rendered path hardcodes 200. Consider:

Suggested change

	if (pagesPrerenderedFile) {
	const html = await fs.promises.readFile(pagesPrerenderedFile, "utf-8");
	const prerenderedHeaders: Record<string, string | string[]> = { ...middlewareHeaders };
	sendCompressed(
	req,
	res,
	html,
	"text/html; charset=utf-8",
	200,
	prerenderedHeaders,
	if (pagesPrerenderedFile) {
	const html = await fs.promises.readFile(pagesPrerenderedFile, "utf-8");
	const prerenderedHeaders: Record<string, string | string[]> = { ...middlewareHeaders };
	sendCompressed(
	req,
	res,
	html,
	"text/html; charset=utf-8",
	middlewareRewriteStatus ?? 200,
	prerenderedHeaders,
	compress,
	);
	return;
	}

[ask-bonk]

The regex approach is a big improvement over the previous collectStaticRoutes that executed getStaticProps() — no more side effects during route collection.

However, regex-based detection can produce false positives:

• A commented-out // export async function getServerSideProps will match
• A string literal "export function getServerSideProps" will match
• Re-exports like export { getServerSideProps } from './data' won't match

For pre-rendering, false positives (incorrectly skipping a static page) are much safer than false negatives (incorrectly pre-rendering a dynamic page), so this tradeoff is reasonable. But consider adding a comment documenting that this is intentionally conservative:

Suggested change

	// Patterns that indicate a page has server-side data fetching
	const gsspPattern =
	/export\s+(async\s+)?function\s+getServerSideProps|export\s+(const|let|var)\s+getServerSideProps/;
	const revalidateZeroPattern = /export\s+const\s+revalidate\s*=\s*0\b/;
	// Patterns that indicate a page has server-side data fetching.
	// These are intentionally broad (may match comments/strings) — false positives
	// only cause a static page to be skipped, which is safe. False negatives
	// (missing a GSSP page) would cause a broken pre-render.
	const gsspPattern =
	/export\s+(async\s+)?function\s+getServerSideProps|export\s+(const|let|var)\s+getServerSideProps/;
	const revalidateZeroPattern = /export\s+const\s+revalidate\s*=\s*0\b/;

[ask-bonk]

Minor: revalidateZeroPattern checks for export const revalidate = 0 but this is a Pages Router context. Pages Router pages use getStaticProps returning { revalidate: 0 }, not a module-level export const revalidate. The export const revalidate convention is an App Router thing.

Since prerenderStaticPages already bails out for App Router builds (line 977), this pattern will never actually match in practice. It's dead code — consider removing it to avoid confusion, or add a comment explaining it's a defensive check for hybrid projects.

[ask-bonk]

Config is loaded and resolved here, but runStaticExport also accepts and loads config internally (line 853-856 of static-export.ts). For the output: "export" path, the resolved config is passed through (config: resolvedConfig on line 319), so it's only loaded once — good.

But for the non-export path, prerenderStaticPages loads config again internally if needed. The config loaded here on lines 311-312 is only used for the if (resolvedConfig.output === "export") check, then discarded. This is fine for correctness but wasteful.

Consider restructuring to check for output: "export" more cheaply — e.g., reading just the raw config and checking the property before resolving.

[ask-bonk]

Good improvement over the previous it.skipIf approach — throwing an explicit error makes test infrastructure failures visible. This directly addresses the concern from the previous review about silent test skipping.

One concern: this beforeAll creates pagesDir and prerenderedFile, but if the server startup fails (line 42-46), those files are left behind because afterAll cleanup may not run if beforeAll throws. Consider wrapping the server startup in a try/catch that cleans up the test files on failure.

[ask-bonk]

This test creates index.html at runtime and fetches / to verify it's served. But there's a subtle issue: the server was started in beforeAll with hasPrerenderedPages determined at startup time (line 752 of prod-server.ts). Since we created pagesDir in beforeAll, hasPrerenderedPages is true.

But the resolvePrerenderedHtml function does per-request fs.existsSync + fs.statSync calls, so adding files after server startup works. This is correct but relies on the implementation detail that file existence is checked per-request, not cached at startup. A comment noting this would help future maintainers.

[ask-bonk]

Good — this addresses the port 0 guard that the previous review flagged. The check is in the right place.

[ask-bonk]

If startProdServer fails to bind (e.g., address already in use), the error will propagate but the message won't mention what went wrong. Consider a guard similar to the one in runStaticExport (line 887):

const addr = server.address() as import("node:net").AddressInfo;
if (!addr || addr.port === 0) {
  throw new Error("Production server failed to bind to a port for pre-rendering");
}

[ask-bonk]

Good — this wraps the error with route context, addressing the previous review's concern about error messages not mentioning which route failed.

[ask-bonk]

The review has been submitted successfully. Here's a summary of the feedback I posted:

Key findings:

1. Pre-rendered HTML placement in request pipeline (should fix): The pre-rendered HTML is served at step 7b, before afterFiles rewrites (step 9). In Next.js, pre-rendered pages are logically equivalent to page rendering, so they should be served after all rewrites have been applied. An afterFiles rewrite would be bypassed if the path has a pre-rendered HTML file. Recommended moving it to just before step 10 (SSR).

2. Missing Cache-Control and middlewareRewriteStatus: Pre-rendered responses have no caching headers (Next.js sets s-maxage/stale-while-revalidate for static pages), and the middleware rewrite status is hardcoded to 200 instead of respecting middlewareRewriteStatus.

3. Dead revalidateZeroPattern check: The export const revalidate = 0 regex is an App Router convention, but collectStaticRoutesFromSource only runs for Pages Router builds (App Router bails at line 977). This pattern will never match in practice.

4. Regex detection tradeoffs are acceptable but undocumented: The regex approach for collectStaticRoutesFromSource can false-positive on comments/strings but is intentionally conservative (false positives only skip pages). Suggested adding a comment documenting this design choice.

5. Double config loading in CLI: Both buildApp() and start() load and resolve the full config just to check output === "export". Suggested lighter-weight alternatives.

6. Positive notes: The resolvePrerenderedHtml security fix (using + path.sep), the getOutputPath boundary checking, the expandDynamicAppRoute error wrapping with route context, and the port 0 guard are all well done. The replacement of collectStaticRoutes (which executed user code) with collectStaticRoutesFromSource (regex-based) is a solid improvement.

github run

[james-elicx]

Self-review: concerns and suggested improvements

After a full read of the PR diff I found a number of issues across correctness, API design, CLI UX, and test quality. Organised by severity below.

---

Correctness / Behavioural Bugs

ISR pages get a year-long Cache-Control
Any Pages Router page with getStaticProps returning { revalidate: N } will be pre-rendered and served with s-maxage=31536000 — the ISR TTL is completely ignored. The pre-renderer has no way to emit the right header at serve time because the revalidate value isn't persisted alongside the HTML. Fix options: (a) write a .meta.json sidecar with the revalidate value at pre-render time and read it in resolvePrerenderedHtml, or (b) exclude ISR pages from pre-rendering by detecting revalidate in the source scan.

middlewareRewriteStatus forwarded to pre-rendered response
middlewareRewriteStatus ?? 200 in step 9b means a middleware NextResponse.rewrite(url, { status: 403 }) serves the pre-rendered HTML body with a 403 status. Pre-rendered files should always be served as 200 — status codes from middleware that reach step 9b indicate a rewrite, not a denial.

buildUrlFromParams emits literal "undefined" for missing params
If getStaticPaths/generateStaticParams returns an object missing a required key, String(undefined) is used and a file called undefined.html is written to disk. Should throw with a clear message instead.

__NEXT_DATA__.page uses :slug format instead of [slug]
The Next.js client-side router matches hydration by the page field in __NEXT_DATA__. With the colon-parameterised pattern vinext emits (e.g. /blog/:slug), pages using useRouter().query will get an empty object instead of the actual params. Should convert to bracket notation before writing __NEXT_DATA__.

collectStaticRoutesFromSource misses re-exported getServerSideProps

export { getServerSideProps } from './shared'

This passes the regex filter and the page gets pre-rendered, resulting in stale HTML missing server-side data. The comment acknowledges false positives are safe but does not address this class of false negative.

getStaticPaths called without i18n config
locales: [] and defaultLocale: "" are hardcoded. Projects with i18n configured in next.config.js will generate incomplete output since getStaticPaths branches on locales.

---

API / Design Issues

configOverride silently ignored when config is provided
runStaticExport has undocumented mutual exclusion between config and configOverride. When both are passed, configOverride is silently dropped. Should be documented or resolved by always merging configOverride on top.

appDir: root passed to vinext plugin in createTempViteServer
For a Pages Router project, this causes vinext to search for a non-existent app/ directory. The server creation should be router-agnostic (pass only the root, let vinext auto-detect).

Hybrid app/ + pages/ layout skips Pages Router entirely
When both directories exist, only the App Router is exported. Next.js explicitly supports the hybrid layout during migration — both routers should be exported.

appDir field in AppStaticExportOptions is dead code
Declared on the interface, passed at the call site, never used inside the function.

---

CLI / UX Issues

"Pre-rendering static pages…" always printed for App Router builds
prerenderStaticPages returns early for App Router but the log message prints unconditionally. Users building App Router apps see a misleading message on every build.

next.config.js loaded twice during build
The Vite build already loads the config internally; buildApp() reads it again. If generateBuildId is non-deterministic the buildId in __NEXT_DATA__ could differ from the one used in the build.

start() static-export heuristic is too coarse
hasOutDir && !hasDistDir false-positives on projects using "outDir": "out" in tsconfig.json, and silently allows vinext start on a partial/hybrid build that has both directories. Checking for dist/server/entry.js specifically would be more reliable.

---

Test Quality

No test for the path-traversal guard in getOutputPath
The traversal guard is a security-relevant addition with no test. A crafted getStaticPaths return value like ../../etc/passwd should throw, not write outside outDir.

describe blocks share the same dist/server/pages/ directory
The "falls back to SSR" test doesn't verify that about.html is absent. If a previous test run left that file on disk, the assertion passes trivially, masking the fallback logic entirely.

beforeAll throws instead of skipping when fixture isn't built
Vitest surfaces this as a hard failure rather than a pending skip, producing a confusing error message in CI.

trailingSlash is implicit in file-presence assertions
expect(result.files).toContain("about.html") silently breaks if the fixture config ever gains trailingSlash: true (the file becomes about/index.html).

Empty-generateStaticParams test is vacuous
The test title claims it verifies a warning is emitted for empty generateStaticParams, but the assertions only check existing array types — they would pass even if the warning code were deleted.

[james-elicx]

/bigbonk review

[ask-bonk]

Review: Static pre-rendering at build time

I've read the full diff and all prior review rounds. This is a well-structured feature — the architecture is clean (orchestrator → export/prerender → prod-server serving), and the iterative review process has addressed most critical issues. The shared regex builders (makeFnRe/makeVarRe), isLocallyDefinedExport for conservative ISR handling, hybrid app/+pages/ support with collision detection, path traversal guards, Content-Type checks, AbortController timeouts, and correct pre-rendered HTML placement at step 9b are all good.

My comments below focus on the remaining issues I found — two should-fix items and several nits. I don't see any remaining must-fix blockers.

Should fix (2)

1. prerenderStaticPages doesn't pass trailingSlash from config — the call to getOutputPath at line 1247 uses config.trailingSlash, which is correct. But the test suite in build-prerender.test.ts has zero coverage for trailingSlash: true in the pre-render path. The build-static-export.test.ts has a dedicated suite for trailingSlash: true which is great, but the pre-render flow through resolvePrerenderedHtml (which checks both file patterns) is only tested indirectly.

2. classifyPagesRoute swallows fs.readFileSync errors silently — at report.ts:176, if the file read fails, it returns { type: "unknown" } with no warning. When called from collectStaticRoutesFromSource, this means a permissions error or missing file is silently classified as unknown and the route is pre-rendered as a static page (since unknown is not filtered out in the collectStaticRoutesFromSource function). Actually — looking again, collectStaticRoutesFromSource wraps this in its own try/catch and adds the route to skipped on error (line 1330-1333). So this is fine for the pre-render path. But classifyPagesRoute returning unknown for an unreadable file could be confusing in the build report. Low priority.

Nits (4)

See inline comments.

[ask-bonk]

Nit: the comment here is excellent and forward-looking, but there's a subtle issue — routeClassifications.set(urlPath, { type: "static" }) overwrites the more detailed classification that was seeded from collectStaticRoutesFromSource (which might have included a revalidate value for getStaticProps pages classified as static). Since collectStaticRoutesFromSource already filters out ISR/SSR routes, any route that reaches this point is genuinely static, so { type: "static" } is correct — but the overwrite discards any revalidate field that was false/null/Infinity (all of which map to static). This doesn't affect behavior since printBuildReport doesn't use the revalidate value for static routes, but it's worth noting.

More importantly: if collectStaticRoutesFromSource ever fails to classify a route (returns unknown) but the prod server successfully renders it, this line upgrades it to static — which is the desired behavior. Good.

[ask-bonk]

The _-prefix filter uses path.basename(route.filePath, path.extname(route.filePath)) which strips the extension first. This means a file like _middleware.tsx → basename _middleware → starts with _ → skipped. But a file named _hidden.tsx in a subdirectory like pages/admin/_hidden.tsx would also be skipped, which is correct (Next.js ignores _-prefixed files in pages/).

However, note that pagesRouter() (the route scanner) may or may not include these files in the first place — if the scanner already filters _-prefixed files, this check is redundant. If not, it's necessary. Worth verifying that both staticExportPages (line 172) and collectStaticRoutesFromSource (here) agree on the filtering behavior.

[ask-bonk]

This test calls the real prerenderStaticPages against the pages-basic fixture. If the fixture isn't built, prerenderStaticPages returns early with a warning ("dist/ directory not found") and pageCount is 0 — so the assertion at line 198 (expect(result.pageCount).toBeGreaterThan(0)) will fail, which is the right behavior.

But this test is outside the describe.skipIf(!fixtureBuilt) block (which only covers the prod-server tests). So if the fixture isn't built, this test fails while the sentinel test at line 31 also fails — giving the developer two failure messages for the same root cause. Consider either:

1. Moving this into the skipIf block, or
2. Adding a guard: it.skipIf(!fixtureBuilt)(...) so it skips cleanly alongside the sentinel.

[ask-bonk]

Good choice using s-maxage=3600 as the conservative default. The comment explaining the rationale (lines 1196-1202) is thorough.

One thing to note for the future: stale-while-revalidate without a value means different things to different CDNs. Per RFC 5861, stale-while-revalidate requires a delta-seconds value. Cloudflare treats the bare directive as stale-while-revalidate=0 (which means "don't serve stale"), while some other CDNs treat it more liberally. Consider adding an explicit value:

Suggested change

	"Cache-Control": "s-maxage=3600, stale-while-revalidate",
	"Cache-Control": "s-maxage=3600, stale-while-revalidate=86400",

This ensures consistent behavior across CDN providers — serve stale for up to 24h while revalidating in the background.

[ask-bonk]

Nit: prerenderStaticPages is called unconditionally for all non-export builds, including App Router builds. It returns early for App Router (at static-export.ts:1180), but the dynamic import of prod-server.js at static-export.ts:1205 happens after the early return check, so there's no wasted import for App Router builds. This is fine.

The fallback to classifyRoutesFromSource at line 485 for App Router builds is also correct — it provides source-based route classifications for the build report even when pre-rendering is skipped. A brief comment here explaining why the fallback exists would help:

Suggested change

	const prerenderResult = await prerenderStaticPages({
	// Use runtime-confirmed classifications from pre-rendering when available.
	// For App Router builds (which skip pre-rendering entirely and return an
	// empty routeClassifications map), fall back to static source analysis so
	// the build report still shows accurate route types.
	const knownRoutes =

[ask-bonk]

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

Overall assessment: The PR is in good shape after 6+ rounds of review iteration. No remaining must-fix blockers.

Should fix (2):

1. Missing test coverage for trailingSlash: true in the pre-render path (build-prerender.test.ts) — the static export tests cover it, but resolvePrerenderedHtml's dual-pattern lookup is only tested indirectly.
2. classifyPagesRoute silently returns unknown on file read errors — low priority since collectStaticRoutesFromSource has its own try/catch.

Nits (4 inline comments):

1. routeClassifications.set(urlPath, { type: "static" }) overwrites the seeded classification — correct behavior but worth noting.
2. _-prefix filtering in collectStaticRoutesFromSource may be redundant with the route scanner — worth verifying.
3. prerenderStaticPages integration test at line 187 is outside the skipIf block, causing a double failure when fixture isn't built.
4. stale-while-revalidate without an explicit delta-seconds value — suggested stale-while-revalidate=86400 for cross-CDN consistency.
5. Brief comment suggestion for the classifyRoutesFromSource fallback in cli.ts.

github run

[james-elicx]

/bigbonk review

[ask-bonk]

Review: Static pre-rendering at build time

This is a substantial, well-architected feature. The three-layer design (orchestrator → export/prerender → prod-server serving) is clean, and the code quality is high. The security hardening (path traversal guards with + path.sep, Content-Type checks, AbortController timeouts), the classifyPagesRoute single-source-of-truth via makeFnRe/makeVarRe shared builders, isLocallyDefinedExport for conservative ISR handling, hybrid app/+pages/ support with collision detection, and the correct placement of pre-rendered HTML at step 9b (after all rewrites) are all well done.

After reading the full diff and all prior review rounds, the critical issues have been resolved. What follows are a couple of should-fix items I noticed, plus some nits.

Summary

Severity	Count
Should fix	2
Nit	4

[ask-bonk]

Should fix: res.text() is called (line 888, existing code) before the Content-Type check, buffering the full response body into memory before discovering it's not HTML.

For the common case (HTML) this doesn't matter. But for a misclassified route returning a large binary/JSON payload, the entire body is buffered then discarded. Consider swapping the order — check content-type first, then call res.text():

const contentType = res.headers.get("content-type") ?? "";
if (!contentType.includes("text/html")) {
  result.errors.push({ route: urlPath, error: `Expected text/html but got "${contentType}" — skipping` });
  await res.body?.cancel();
  continue;
}
const html = await res.text();

Same pattern applies in prerenderStaticPages at line 1239.

[ask-bonk]

Should fix: same issue as staticExportApp — res.text() is called before the Content-Type check.

Check content-type first, cancel the body early for non-HTML responses:

Suggested change

	const html = await res.text();
	const contentType = res.headers.get("content-type") ?? "";
	const contentType = res.headers.get("content-type") ?? "";
	if (!contentType.includes("text/html")) {
	// Non-HTML response (e.g. a JSON API route misclassified as static).
	// Writing it as .html would produce a corrupt file — skip instead.
	result.skipped.push(`${urlPath} (non-HTML content-type: ${contentType})`);
	await res.body?.cancel();
	continue;
	}
	const html = await res.text();

[ask-bonk]

Nit / design note for ISR follow-up: The s-maxage=3600 is a safe conservative default. For proper ISR support, consider writing a .meta.json sidecar alongside each .html during pre-rendering that records { "revalidate": N | false }. The prod server can then read it here and emit the correct s-maxage — letting truly static pages get aggressive year-long caching while ISR pages get their configured TTL.

Not blocking for this PR — the 1-hour default is correct given the known false-negative cases in ISR detection.

[ask-bonk]

Nit: The block comment is thorough, but readers scanning quickly will be confused by appDir: root appearing to pass the project root as the app directory. Consider an inline hint:

Suggested change

	plugins: [vinextPlugin({ appDir: root })],
	plugins: [vinextPlugin({ appDir: root /* project root, not app/ dir — see comment above */ })],

[ask-bonk]

Nit: no test for trailingSlash: true in the pre-render path. The static export tests (line 186+) cover trailingSlash: true well, and resolvePrerenderedHtml checks both file patterns. But build-prerender.test.ts has no trailingSlash: true coverage — the about/index.html lookup path in resolvePrerenderedHtml is only tested indirectly.

Consider adding one test in build-prerender.test.ts that creates about/index.html and verifies it's served at /about/ (after trailing-slash redirect) or /about.

[ask-bonk]

Nit: bare catch swallows the error message. If classifyPagesRoute throws due to e.g. a permissions error, the user sees (failed to read source: ...) which is good. But if classifyPagesRoute internally returns { type: "unknown" } for a read failure (which it does at report.ts:177), this outer catch only fires for unexpected errors. Consider logging the error type to help distinguish classification failures from filesystem errors:

Suggested change

	const msg = e instanceof Error ? e.message : String(e);
	} catch (e) {
	const msg = e instanceof Error ? e.message : String(e);
	skipped.push(`${route.pattern} (classification error: ${msg})`);

[ask-bonk]

The review has been submitted successfully. Here's a summary of the feedback I posted:

Review Summary

This is a well-architected feature that has gone through extensive review iteration. The architecture is clean — three-layer design with orchestrator, export/prerender, and prod-server serving. Most critical issues from prior rounds have been addressed.

Should fix (2):

1. res.text() called before Content-Type check in both staticExportApp and prerenderStaticPages — The full response body is buffered into memory before discovering it's not HTML. For misclassified routes returning large binary/JSON payloads, this is wasteful. The fix is to check content-type first and cancel the body early for non-HTML responses.

2. Same issue in prerenderStaticPages (line 1239-1240) — with a concrete suggestion block for the fix.

Nits (4):

3. Cache-Control design note — The s-maxage=3600 is a safe default, but flagged writing .meta.json sidecars as a design consideration for proper ISR support.

4. appDir: root inline hint — The block comment is thorough but an inline comment at the call site would help quick scanners.

5. Missing trailingSlash: true test in the pre-render path — the static export tests cover it, but resolvePrerenderedHtml's index.html lookup is only tested indirectly.

6. Bare catch in collectStaticRoutesFromSource — suggested using classification error: prefix instead of failed to read source: to distinguish error types.

github run

[NathanDrake2406]

@james-elicx let's finish this today

[james-elicx]

My thinking was try to try and ship this on Saturday so that there's time to do manual testing and reviews now that Bonk seems to be settling down. I'm out for most of the afternoon+evening today unfortunately...

[james-elicx]

/bigbonk review

[ask-bonk]

Review: Static pre-rendering at build time

I read the full diff and all prior review rounds. This is a well-architected feature — the three-layer design (orchestrator → export/prerender → prod-server serving) is clean, the security hardening is thorough, and the iterative review process has clearly resolved the major issues. The makeFnRe/makeVarRe shared regex builders, isLocallyDefinedExport for conservative ISR handling, hybrid app/+pages/ support, correct step 9b placement, s-maxage=3600 default, and path traversal guards are all good.

After this many review rounds, most critical issues are resolved. I have one should-fix (body buffering before Content-Type check), and a few nits. No must-fix blockers.

Should fix (1)

1. staticExportApp buffers the full response body before checking Content-Type — res.text() is called at line 899 before the content-type check at line 888-897 for the !res.ok case (which correctly cancels), but when res.ok is true, res.text() runs unconditionally and the content-type check happens before it at line 888. Actually, re-reading more carefully: the content-type check IS before res.text() at lines 888-897. So this is correct for staticExportApp. However, in prerenderStaticPages at line 1248, res.text() is called AFTER the content-type check at 1240-1247. Both are actually fine. Disregard — this was addressed in a prior round and the current code is correct.

Nits (3)

See inline comments.

[ask-bonk]

Nit: The inline comment /* project root, not app/ dir — see comment above */ is helpful but could be slightly misleading since it implies appDir accepts a project root by design. The block comment above (lines 59-73) is excellent at explaining the workaround. Consider whether removing appDir: root entirely and instead patching earlyBaseDir upstream (in the plugin) would be cleaner long-term — but for this PR the workaround is fine.

[ask-bonk]

Good comment about the key semantics. One small note: this set(urlPath, { type: "static" }) overwrites the richer classification that was seeded from collectStaticRoutesFromSource (which may have included a specific revalidate value for pages classified as static — e.g., revalidate: Infinity or revalidate: false, both of which map to static). Since the build report doesn't use the revalidate field for static routes, this is correct. But if ISR support evolves to persist .meta.json sidecars alongside pre-rendered HTML, this overwrite would discard that data — worth keeping in mind.

[ask-bonk]

Nit: for both this path and prerenderStaticPages (line 1248), the response body is only consumed (res.text()) after the content-type and status checks — good. But in the !res.ok case at line 884, you use await res.body?.cancel() (with await), while in prerenderStaticPages at line 1236 you use res.body?.cancel() (without await) with a comment explaining the AbortController interaction. The inconsistency is harmless but slightly confusing — consider making them consistent (the non-awaiting pattern with the comment is the more defensive choice).

[ask-bonk]

Good choice on the conservative s-maxage=3600 with the explicit stale-while-revalidate=86400 delta-seconds. The comment explaining the rationale (lines 1195-1205) is thorough. For ISR follow-up: persisting a .meta.json sidecar with the actual revalidate value during pre-rendering would let the prod server emit correct per-page s-maxage headers.

[ask-bonk]

Nice addition — this test covers the trailingSlash: true file layout in resolvePrerenderedHtml, which was flagged as missing test coverage in prior reviews. The about/index.html lookup path is now tested directly.

[ask-bonk]

Overall Assessment

After reading the full diff (1691 additions across 20 files) and all prior review rounds, this PR is in good shape. The architecture is clean, the security hardening is solid, and the iterative reviews have resolved all previously-identified critical issues.

What's done well:

• Three-layer architecture: orchestrator (runStaticExport) → export/prerender functions → prod-server integration
• makeFnRe/makeVarRe shared regex builders eliminate duplication drift risk between hasNamedExport and isLocallyDefinedExport
• isLocallyDefinedExport correctly treats re-exported getStaticProps as ISR (conservative — avoids year-long CDN pin)
• Hybrid app/ + pages/ support with collision detection and deterministic precedence
• Pre-rendered HTML correctly placed at step 9b (after all rewrites, before SSR)
• Conservative s-maxage=3600, stale-while-revalidate=86400 instead of year-long cache
• Path traversal guards with + path.sep, backslash rejection, and boundary checks
• Content-Type validation before writing HTML files
• AbortController timeouts on all fetches with proper clearTimeout in finally
• patternToNextFormat reuse (deduplication of the inline copy in pages-server-entry.ts)
• try/finally for SSR context cleanup in renderStaticPage
• buildUrlFromParams now throws on missing params instead of writing undefined.html
• Thorough test coverage: 21 new tests across static export, prerender, and path-traversal scenarios
• Good trailingSlash: true test in both build-static-export.test.ts and build-prerender.test.ts (the latter covers the about/index.html lookup path in resolvePrerenderedHtml)

No must-fix blockers remaining. The inline comments I posted are nits and observations for future consideration. This looks ready for final human review.

[ask-bonk]

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

Overall assessment: The PR is in good shape after 8+ rounds of review iteration. No must-fix blockers remaining.

Key positives I called out:

• Clean three-layer architecture (orchestrator → export/prerender → prod-server serving)
• Shared regex builders (makeFnRe/makeVarRe) eliminate duplication drift
• Conservative ISR handling via isLocallyDefinedExport
• Hybrid app/ + pages/ support with collision detection
• Pre-rendered HTML correctly placed at step 9b (after all rewrites)
• Conservative s-maxage=3600 cache header with explicit stale-while-revalidate=86400
• Thorough security hardening (path traversal guards, Content-Type checks, AbortController timeouts)
• Good test coverage including trailingSlash: true scenarios

Inline nits (5 comments):

1. appDir: root workaround is fine but worth considering upstream fix long-term
2. routeClassifications.set(urlPath, { type: "static" }) overwrites richer seeded classification — fine now but relevant if .meta.json sidecars are added for ISR
3. Minor inconsistency in res.body?.cancel() await patterns between staticExportApp and prerenderStaticPages
4. Design note: .meta.json sidecars for proper per-page s-maxage in ISR follow-up
5. Positive note on trailingSlash: true test coverage addition

github run

[NathanDrake2406]

@james-elicx Heads up — I ran a few local review passes and found a critical pre-existing bug on main that I've fixed in this PR:

sanitizeDestinationLocal is never imported in the Pages Router server entry (pages-server-entry.ts line 281). It's called at two sites (gSSP redirect at L804, gSP redirect at L917) but only parseCookies is imported from config-matchers.js. Any getServerSideProps or getStaticProps that returns { redirect: { destination: '...' } } crashes in production with ReferenceError: sanitizeDestinationLocal is not defined.

Fix: 6cbca62 — added sanitizeDestination as sanitizeDestinationLocal to the import.

This exists on main too, not introduced by this PR. Might be worth cherry-picking independently if this PR takes a while to land.

Also addressed the latest bonk nits (body cancel consistency, content-type check ordering) and fixed two other bugs found during review:

• expandDynamicAppRoute missing Array.isArray guard on the common (no parent params) path
• renderErrorPage calling resetSSRHead() but never collecting/injecting head tags

[james-elicx]

/bigbonk review

[ask-bonk]

Review: Static pre-rendering at build time

This is a well-designed, substantial feature. The architecture is clean — three-layer separation between the orchestrator (runStaticExport), the export/prerender functions, and the prod-server integration. The PR has clearly benefited from extensive iterative review, and most critical issues have been addressed. Code quality is high overall.

I focused on correctness, edge cases, and areas that prior reviews may not have fully resolved. I have a few actionable findings below.

What's done well

• sanitizeDestinationLocal import fix — this is a real pre-existing production bug on main (gSSP/gSP redirects crash with ReferenceError). Good catch, worth cherry-picking independently.
• patternToNextFormat deduplication — moving from an inline copy in the generated server entry to importing from the shared module is the right call.
• isLocallyDefinedExport via makeFnRe/makeVarRe — clean solution for the ISR false-negative on re-exported getStaticProps, with shared regex builders to prevent drift.
• Path traversal guards — resolvedDir + path.sep, backslash rejection, boundary checks — all thorough.
• Hybrid app/ + pages/ support with collision detection and deterministic precedence.
• try/finally for SSR context cleanup in renderStaticPage — fixes a class of state leaks.
• Head tag collection in renderErrorPage — previously missing, now consistent.
• Conservative s-maxage=3600, stale-while-revalidate=86400 — correct tradeoff given known ISR detection gaps.
• Content-Type validation, AbortController timeouts, buildUrlFromParams missing-param throw.
• Test quality — sentinel test for fixture-built guard, trailingSlash:true coverage, path-traversal tests, hybrid export tests, and the file-after-server-start test pattern are all well thought out.

[ask-bonk]

When parentParamSets.length > 0 and generateStaticParams returns a non-array value, it's silently skipped (the if (Array.isArray(childResults)) guard discards it). In the no-parent branch (line 767-773), a non-array return correctly throws with a descriptive error.

This means with parent params, a buggy generateStaticParams that returns e.g. null or an object silently produces zero URLs for that parent, and the user gets a downstream "empty array" warning with no indication their function returned invalid data.

Consider adding an else branch:

Suggested change

	const childResults = await generateStaticParams({ params: parentParams });
	const childResults = await generateStaticParams({ params: parentParams });
	if (Array.isArray(childResults)) {
	for (const childParams of childResults) {
	paramSets.push({ ...parentParams, ...childParams });
	}
	} else if (childResults != null) {
	throw new Error(
	`generateStaticParams() for ${route.pattern} must return an array, got ${typeof childResults}`,
	);
	}

[ask-bonk]

This set(urlPath, { type: "static" }) overwrites the richer classification seeded from collectStaticRoutesFromSource. For the current use case (only non-dynamic routes are pre-rendered, so urlPath === route.pattern), this is correct and the overwrite is intentional.

But the discarded classification may have had a revalidate field (e.g., { type: "static", revalidate: Infinity } or { type: "static", revalidate: false }). If .meta.json sidecars are added for ISR in the future, this overwrite would discard that data.

Not a bug today — just noting it for the ISR follow-up. Consider a comment like:

// Overwrites the source-analysis classification. Currently safe because
// source analysis already filtered ISR pages. If per-page revalidate
// metadata is needed later (.meta.json sidecars), preserve the original
// classification's revalidate field here.

[ask-bonk]

Minor: fs.existsSync + fs.statSync is two syscalls per check. Since this runs on every non-cached request, consider consolidating to a single try { return fs.statSync(directPath).isFile() ? directPath : null } catch { /* ENOENT */ } pattern to halve the syscall count. Not a correctness issue — just a production hot-path optimization.

(Same applies to the indexPath check at line 362.)

[ask-bonk]

Nit: the 14-line block comment explaining appDir: root is thorough and necessary for anyone maintaining this code. One thing that would help quick readers: the option name appDir is semantically misleading here since it's actually the project root, not the app/ directory. Consider filing an issue upstream to expose a baseDir option on the plugin (so this workaround can eventually be replaced), and add a brief // TODO: referencing it.

[ask-bonk]

This test is outside the describe.skipIf(!fixtureBuilt) block. If the fixture isn't built, prerenderStaticPages returns early with pageCount: 0, and the assertions at lines 226-227 (toBeGreaterThan(0)) will fail — which is the correct behavior (it won't silently pass).

But the developer now gets two failures for the same root cause: the sentinel test at line 31 and this assertion. Consider guarding this with it.skipIf(!fixtureBuilt) for consistency, so the sentinel is the single failure point when the fixture is missing:

Suggested change

	it.skipIf(!fixtureBuilt)("PrerenderResult type is returned", async () => {
	it.skipIf(!fixtureBuilt)("PrerenderResult type is returned", async () => {

(I see it already has it.skipIf — disregard this if so, the current code is correct.)

[ask-bonk]

Nit: loadNextConfig and resolveNextConfig are already imported at the top of this file (line 25-26) as static imports. This line uses them directly, which is correct. But in cli.ts (line 422-424), they're dynamically imported. The comment in cli.ts explains the motivation (lazy loading for vinext dev/vinext start paths), which makes sense for the CLI entry point.

But here in static-export.ts, the top-level static import means the module is eagerly loaded regardless. This is fine — just noting the intentional asymmetry for anyone comparing the two files.

[ask-bonk]

Good catch — this fixes a real production crash. sanitizeDestinationLocal is called at two sites (gSSP redirect and gSP redirect) but was never imported. Any getServerSideProps or getStaticProps returning { redirect: { destination: '...' } } would crash in production with a ReferenceError.

This exists on main too — worth cherry-picking as a standalone fix so it doesn't have to wait for this PR to land.

[ask-bonk]

The Content-Type check and res.body?.cancel() are in the right order here — status check first, then content-type, then res.text(). The body is only consumed when we know we'll use it. Good.

One edge case: a 200 response with Content-Type: text/html but an enormous body (e.g., a page that renders an unbounded list at build time). There's no size guard, so res.text() would buffer the entire body into memory. Not a pressing concern for most apps, but a maxBodySize check could prevent OOM during static export of pathological pages.

[ask-bonk]

This is the correct conservative choice. When getStaticProps is re-exported, we can't inspect the revalidate value, so treating it as ISR prevents a year-long CDN pin.

One thing to consider: isLocallyDefinedExport checks for function/variable declarations but not for default export assignment patterns like:

export const getStaticProps: GetStaticProps = async (ctx) => { ... }

This is handled by makeVarRe (matches export const getStaticProps =). But a pattern like:

const gsp = async () => { ... }
export { gsp as getStaticProps }

would be detected by hasNamedExport (the re-export {...} branch) but NOT by isLocallyDefinedExport — so it would be treated as a re-export even though the implementation is local. This is a false conservative (ISR instead of static), which is safe. Just documenting it here for awareness.

[ask-bonk]

The heuristic hasOutDir && !hasDistServer is reasonable and well-documented. One edge case worth noting: if a user runs vinext build (which creates dist/) and then later runs vinext build with output: "export" (which creates out/ but does NOT delete the old dist/), then hasDistServer would be true and hasOutDir would be true, so vinext start would proceed to start the production server — serving the stale build instead of rejecting.

Conversely, if runStaticExport ever gains a cleanup step that removes dist/ before writing to out/, this heuristic would work correctly. The comment at line 504-509 covers the false-positive case but not this stale-build scenario. Low priority but worth a note.

[ask-bonk]

The review has been submitted. Here's a summary of what I posted:

Overall assessment: The PR is in good shape after 8+ rounds of iterative review. The architecture is clean (three-layer design: orchestrator → export/prerender → prod-server serving), the security hardening is thorough, and the code quality is high. No must-fix blockers remaining.

10 inline comments covering:

1. expandDynamicAppRoute silent discard of non-array generateStaticParams returns (should fix) — When parent params exist, a buggy generateStaticParams returning null or an object is silently discarded. The no-parent path correctly throws. Suggested adding an else if branch for consistency.

2. routeClassifications.set(urlPath, { type: "static" }) overwrites richer classification (design note) — Not a bug today, but noted for the ISR .meta.json sidecar follow-up.

3. resolvePrerenderedHtml double syscalls (nit) — fs.existsSync + fs.statSync could be consolidated to a single try { fs.statSync().isFile() } on the production hot path.

4. appDir: root workaround (nit) — Suggested filing an upstream issue for a baseDir plugin option.

5. Test guard consistency — Noted the prerenderStaticPages test outside skipIf gives double failures when fixture is missing (but confirmed it already uses it.skipIf).

6. Dynamic import asymmetry between cli.ts and static-export.ts — documented as intentional.

7. sanitizeDestinationLocal import fix — Confirmed this is a real production crash on main, recommended cherry-picking independently.

8. Content-Type check ordering — Confirmed correct, noted missing max body size guard as a low-priority edge case.

9. isLocallyDefinedExport false conservative on export { gsp as getStaticProps } — Documented as safe (false ISR classification), just awareness.

10. start() heuristic stale-build scenario — Noted that a user switching from normal build to output: "export" without cleaning dist/ would bypass the guard.

github run

[james-elicx]

/bigbonk review!

[ask-bonk]

Review: Static pre-rendering at build time

This is a well-architected feature after 8+ rounds of iteration. The three-layer design (orchestrator → export/prerender → prod-server serving) is clean, the security hardening is thorough, and the code quality is high. The iterative reviews have resolved the critical issues — path traversal guards, Content-Type validation, conservative cache headers, ISR-safe classification via isLocallyDefinedExport, hybrid app/+pages/ support, and correct step 9b placement are all well done.

I focused on finding issues the prior reviews may have missed or under-weighted. I have one should-fix and several nits/observations.

Should fix (1)

1. expandDynamicAppRoute silently produces 0 URLs when generateStaticParams returns null with parent params — In the parent-params branch, when childResults is null, neither the Array.isArray branch nor the != null guard fires, so the iteration silently produces zero entries for that parent. The no-parent branch correctly throws for non-array returns. This asymmetry means a buggy generateStaticParams returning null silently drops all pages for that parent set. See inline comment.

Nits / observations (4)

2. resolveParentParams (line 726-731, unchanged code) has the same silent-discard pattern — If a parent's generateStaticParams returns a non-array value, nextParams stays empty and all downstream children get no params. Consider adding a warning in a follow-up.

3. hasPrerenderedPages is checked once at startup (line 817) but files are written after server start during prerenderStaticPages — This is actually correct: the pre-renderer's temp prod server doesn't need to serve pre-rendered pages to itself. But it's worth noting that if this startup-time check were ever moved later in the lifecycle, it could cause subtle issues.

4. sanitizeDestinationLocal import fix — Confirmed this fixes a real pre-existing production crash on main. Worth cherry-picking as a standalone fix.

5. isLocallyDefinedExport edge case — export { gsp as getStaticProps } (local rename re-export) will be detected by hasNamedExport but NOT by isLocallyDefinedExport — so it's treated as a re-export even though the implementation is local. This is a false conservative (ISR instead of static), which is safe.

Overall

The PR is in good shape. The architecture, security hardening, test coverage, and documentation quality are all solid. Ready for final human review after considering the inline comment.

[ask-bonk]

Should fix: asymmetric null handling between parent and no-parent branches.

When parentParamSets.length > 0 and generateStaticParams returns null, neither branch fires — Array.isArray(null) is false, and null != null is false. The iteration silently produces zero entries for that parent set. In contrast, the no-parent branch (line 771-776) correctly throws for non-array returns.

This means a buggy generateStaticParams returning null with parent params silently drops all pages for that parent, while the same bug without parent params throws a clear error.

The current behavior (silently skip null) is defensible if you consider null as "no entries for this parent". But then the no-parent branch should do the same for consistency. Right now they disagree — either both should throw on null, or both should silently skip. Consider adding a comment documenting the intentional asymmetry, or making the branches consistent.

[ask-bonk]

Good catch — this fixes a real production crash on main where sanitizeDestinationLocal was called but never imported. Any gSSP/gSP redirect would throw ReferenceError. Worth cherry-picking as a standalone fix independent of this PR so it doesn't have to wait for the full feature to land.

[ask-bonk]

Nit / forward-looking note: This set(urlPath, { type: "static" }) overwrites the richer classification seeded from collectStaticRoutesFromSource. Currently safe because source analysis already filtered ISR pages. But if .meta.json sidecars are added for per-page s-maxage in the ISR follow-up, this overwrite would discard the revalidate field. Consider preserving the original classification's revalidate when upgrading:

const existing = result.routeClassifications.get(urlPath);
result.routeClassifications.set(urlPath, { type: "static", revalidate: existing?.revalidate });

[ask-bonk]

Good addition — the early return for root "/" avoids two unnecessary path.join + path.resolve + fs.existsSync + fs.statSync calls on every root request. The comment explaining why indexPath is dead for "/" is thorough and prevents future confusion.

[ask-bonk]

This is the right conservative choice. One edge case to note for awareness: export { gsp as getStaticProps } (local rename re-export) will be detected by hasNamedExport (the {...} branch) but NOT by isLocallyDefinedExport — so it's treated as a re-export even though the implementation is in this file. This is a false conservative (ISR instead of static), which is safe — just documenting the known gap.

[ask-bonk]

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

Overall assessment: The PR is in good shape after 8+ rounds of review iteration. The architecture is clean, security hardening is thorough, and code quality is high. No must-fix blockers remaining.

Should fix (1):

1. expandDynamicAppRoute has asymmetric null handling — When generateStaticParams returns null with parent params, it's silently skipped (neither Array.isArray(null) nor null != null fires). But the no-parent branch correctly throws for non-array returns. This means the same bug in user code produces different outcomes depending on whether parent params exist.

Nits/observations (4):
2. resolveParentParams has the same silent-discard pattern for non-array returns (pre-existing code, not introduced by this PR)
3. hasPrerenderedPages startup-time check is correct but worth noting the lifecycle dependency
4. sanitizeDestinationLocal import fix addresses a real pre-existing production crash on main — recommended cherry-picking independently
5. isLocallyDefinedExport has a known false-conservative edge case with export { gsp as getStaticProps } (safe)

github run