stackbuild: detect Next.js and run its production build#980
Conversation
The Node stack installed dependencies and copied the app, but never ran a build step, so a Next.js app shipped without a compiled `.next/` directory and failed at runtime when served with `next start`. Follow the pattern the Ruby stack already uses for Rails: detect the framework during Init (a `next` dependency), run its build in the image (`next build`, via the app's build script) after the source is copied, and serve it with `next start` bound to $PORT. User env vars are injected during the build so build-time values like NEXT_PUBLIC_* are inlined, the same way the Ruby stack injects env for asset precompilation. Scoped to the default Next.js output; standalone and static-export modes are follow-ups. Plain Node apps are unaffected (no build step, unchanged web command). Part of MIR-1462
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughNode stacks detect Next.js through project dependencies and record the result during initialization. Next.js projects receive a package-manager-specific framework build with configured environment variables before the existing build flow. Runtime command generation uses package-manager-specific Next.js production start commands. New table-driven tests cover npm, Yarn, dependency locations, and a non-Next.js project. Comment |
![miren-code-agent[bot]](https://avatars.githubusercontent.com/u/174767976?s=60&v=4){kind=small}
There was a problem hiding this comment.
🍪 biscuit:
This is a draft, so I'm judging readiness to hand off to human review. The change is small, well-motivated, and mostly correct — but there's one concrete correctness gap and a couple of smaller things worth calling out before it moves forward.
What the PR does
Init() now checks whether next is in either dependencies or devDependencies (via the new hasDependency helper). If it is, frameworkBuildCommand() returns yarn build or npm run build as appropriate, and WebCommand() short-circuits to npx next start -p $PORT instead of falling through to the script/entry-point resolution logic.
The TestNodeNextjs table test covers: Next.js under npm, under yarn, Next.js under devDependencies, and a plain Express app. That's good coverage for the detection and command-string logic.
Concern 1 — next start always uses npx, even when yarn is the package manager
WebCommand() returns "npx next start -p $PORT" regardless of which package manager was detected. The frameworkBuildCommand() correctly uses yarn build for yarn projects, but the start command doesn't follow suit. For a yarn project the natural equivalent would be yarn next start -p $PORT (or at minimum yarn dlx), not npx. The TestNodeNextjs "next with yarn" case accepts npx next start -p $PORT as wantWeb, so the test is written to match the implementation rather than to challenge it. If this is an intentional choice (e.g. to avoid depending on whether next is installed in the project's node_modules/.bin), it deserves a comment; if not, it should be fixed.
Concern 2 — No integration test for the Next.js build step in GenerateLLB
TestNodeNextjs exercises hasNext, frameworkBuildCommand(), and WebCommand() but never calls GenerateLLB. The new code path that injects env vars and emits build.Run(llb.Shlex(buildCmd), ...) is untested at the LLB level. The existing TestNode does call buildLLB, but that fixture has no next dependency, so it never enters the new branch. An integration test for a minimal Next.js project (even a stub one, similar to the Rails/Ruby pattern) would catch issues like bad LLB ordering or environment injection mistakes.
Concern 3 — hasDependency is called in Init, but Init calls parsePackageJSON first only when invoked directly
This is fine as written — Init always calls parsePackageJSON before hasDependency, so s.dependencies and s.devDependencies are populated. The guard if s.dependencies[name] doesn't crash on a nil map (Go treats a nil map read as a zero-value hit). No bug here, just worth being aware that hasDependency is only safe after parsePackageJSON runs.
Minor
wantWeb in the "next listed under devDependencies" test case is "npx next start -p $PORT". That's fine if Concern 1 is intentional, but it means this case doesn't actually verify that devDependencies detection works end-to-end through WebCommand differently from dependencies — it would pass regardless.
Bottom line: the core detection and command-string logic is sound and I like the clean hasDependency helper. The npx vs yarn mismatch in WebCommand is a concrete correctness question that should be resolved before merging, and the missing LLB-level integration test means the build-step wiring is unverified. Either address these or consciously accept them.
🍪 full review note · comment /biscuit review to run biscuit again.
Inline comments
pkg/stackbuild/node.go:280
WebCommand() returns "npx next start -p $PORT" unconditionally, but frameworkBuildCommand() already distinguishes yarn vs npm. For a yarn project, yarn manages the local next binary; relying on npx (an npm tool) could fail or use a different binary resolution path. Either use yarn next start -p $PORT when s.packageManager == nodePkgYarn, or leave a comment explaining why npx is intentional here.
🤖 Prompt for AI Agents
In pkg/stackbuild/node.go at line 280, inside
WebCommand(), the Next.js start command is
hard-coded as 'npx next start -p $PORT' for all
package managers. frameworkBuildCommand() already
switches on s.packageManager. Apply the same logic
here: return 'yarn next start -p $PORT' when
s.packageManager == nodePkgYarn, and 'npx next
start -p $PORT' (or 'npm exec next start -p
$PORT') otherwise. Also update the corresponding
wantWeb value in the TestNodeNextjs 'next with
yarn' table entry in stackbuild_test.go to match.
pkg/stackbuild/stackbuild_test.go:400
TestNodeNextjs never calls GenerateLLB, so the new branch in GenerateLLB that emits the build step and injects env vars is untested at the LLB level. Consider adding a lightweight integration test (guarded by checkDocker(), like TestNode) that creates a minimal Next.js-shaped project (the package.json stub is already here), calls GenerateLLB, and verifies at minimum that the returned *llb.State marshals without error — and ideally runs buildLLB to confirm the step sequence is valid.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/stackbuild/node.go (1)
218-227: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a persistent cache mount for Next.js's own build cache.
The dependency-install steps use persistent cache mounts (
llb.AsPersistentCacheDir) for npm/yarn caches, but thenext buildstep has no equivalent mount for.next/cache, which Next.js uses for incremental build caching. Adding one could meaningfully speed up repeat builds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/stackbuild/node.go` around lines 218 - 227, Update the Next.js build step in the frameworkBuildCommand block to mount a persistent cache directory at /app/.next/cache using llb.AsPersistentCacheDir, while preserving the existing environment setup, build command, and custom name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/stackbuild/node.go`:
- Around line 218-227: Update the Next.js build step in the
frameworkBuildCommand block to mount a persistent cache directory at
/app/.next/cache using llb.AsPersistentCacheDir, while preserving the existing
environment setup, build command, and custom name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 364133ff-8419-4e51-88bc-5500f5700108
📒 Files selected for processing (2)
pkg/stackbuild/node.gopkg/stackbuild/stackbuild_test.go
biscuit review flagged that WebCommand returned `npx next start` even for yarn projects, while frameworkBuildCommand already picks yarn vs npm. Make the serve command follow the detected package manager too. Authored-by: Claude Code (autonomous agent)
|
🤖 Thanks for the review. Addressed Concern 1 in 04d2d86 — Leaving two items for human review rather than expanding this autonomous change:
|
|
/biscuit review |
There was a problem hiding this comment.
🍪 biscuit:
My first concern — WebCommand() returning npx next start even for yarn projects — has been fixed. Lines 282–283 of node.go now return "yarn next start -p $PORT" when the package manager is yarn, and the "next with yarn" test case at line 434 correctly expects that value. That's the right fix and the comment at lines 279–280 makes the intent explicit.
My second concern — no LLB-level test for the Next.js build step — is still open. TestNodeNextjs (lines 408–476) exercises detection, frameworkBuildCommand(), and WebCommand(), but it never calls GenerateLLB(). The new code that chains build.AddEnv(k, v) in a loop and then calls build.Run(llb.Shlex(buildCmd), ...) before applyOnBuild is entirely untested at the graph-construction level. A mistake there — wrong ordering, a stale state variable, a nil-map env injection — would not be caught by the current suite.
This is a draft, so I'm judging readiness to hand to human review. The detection logic and command-string layer are solid and now consistent for both package managers. The unverified LLB wiring is a concrete gap: either add a GenerateLLB call to TestNodeNextjs (even just require.NoError(t, err) after the call, with checkDocker() guard if you want to skip buildLLB) to confirm the graph marshals without error, or accept that risk consciously.
🍪 full review note · comment /biscuit review to run biscuit again.
Addresses biscuit's second review concern: TestNodeNextjs exercised detection and the command strings but never called GenerateLLB, leaving the Next.js build step (env injection + build.Run) unverified. Construct and marshal the graph so ordering/env-injection mistakes are caught without needing Docker. Authored-by: Claude Code (autonomous agent)
|
🤖 Addressed the second concern in e37d9d1: |
|
/biscuit review |
There was a problem hiding this comment.
🍪 biscuit: ✅ ready to merge — auto-review, non-blocking
My open concern from the previous review has been addressed. TestNodeNextjs (lines 476–479) now calls stack.GenerateLLB(dir, BuildOptions{EnvVars: map[string]string{"NEXT_PUBLIC_FOO": "bar"}}) and follows it with state.Marshal(context.Background()). That exercises the new AddEnv loop and the build.Run(llb.Shlex(buildCmd)) step in the LLB construction path, with a non-nil env map, without needing Docker. It's precisely what I asked for and it catches the class of bugs I was worried about.
The rest of the implementation remains solid: hasDependency correctly checks both dependencies and devDependencies, frameworkBuildCommand and WebCommand are consistent for both npm and yarn, and the test table covers detection in both dep maps plus the negative case. The LLB wiring (env injection before the build step, then applyOnBuild) mirrors the pattern the Ruby stack uses for asset precompilation.
This draft is ready to graduate to human review.
🍪 full review note · comment /biscuit review to run biscuit again.
|
🤖 Settled and ready for human review. Summary of the automated round-trip:
Still a draft — a human decides readiness and merge. This is where the bot's autonomous pass ends. |
1 participant
What
Teaches the Node stack to recognize a Next.js app and produce a working image for it.
Before this, the Node stack ran
npm installand copied the source but never ran a build step, so a Next.js app shipped with no compiled.next/and fell over at runtime undernext start.How
Follows the pattern the Ruby stack already uses for Rails — detect the framework, run its build in-image, serve it with the framework's command:
nextdependency inpackage.json(deps or devDeps) setshasNextduringInit, emitting aframework: nextjsevent — same shape asruby.godetecting Rails andpython.godetecting Django/FastAPI.next buildvia the app'sbuildscript) with user env vars injected, so build-time values likeNEXT_PUBLIC_*are inlined. This mirrors the Ruby stack'sassets:precompilestep (which likewise injectsopts.EnvVars).next start -p $PORT, mirroringrails server -p $PORT.Scoped to Next.js's default output.
output: 'standalone'and staticexportare follow-ups — same altitude as the Ruby stack not enumerating every Rack server. Plain Node apps are unaffected: no build step runs and the web command is unchanged.Verification
go test ./pkg/stackbuild/— newTestNodeNextjscovers detection (deps + devDeps), the npm/yarn build command, the web command, and that a plain Express app is untouched. The Docker-gatedTestNode/TestBunbuild tests also ran green, confirming the existing Node/Bun build paths still work. Package builds and vets clean.Not covered locally: a full end-to-end
next buildimage build (needs a real Next fixture through buildkit) — worth adding as a Docker-gated test in a follow-up.Part of MIR-1462