Adds support for pnpm as a bundler #438
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdd lockfile-based JS runtime detection during project init; centralize JS package-manager detection and CI-aware install command construction in the bundler; restrict supported runtimes to "bunjs", "nodejs", and "uv"; add unit tests for package-manager detection and PNPM CI flags; trivial schema newline update. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as cmd/project.go
participant FS as Filesystem
participant Cfg as Project Config
U->>CLI: init project (with Provider.Runtime default)
CLI->>FS: inspect lockfiles (bun.lockb/bun.lock -> pnpm-lock.yaml -> package-lock.json -> yarn.lock)
alt Bun lockfile found and default supports JS
CLI->>Cfg: set Bundler.Runtime = "bunjs"
else pnpm lockfile found and default supports JS
CLI->>Cfg: set Bundler.Runtime = "nodejs" (pnpm)
else npm/yarn lockfile found and default supports JS
CLI->>Cfg: set Bundler.Runtime = "nodejs"
else None or non‑JS default
CLI->>Cfg: set Bundler.Runtime = defaultRuntime
end
sequenceDiagram
autonumber
participant Bundler as internal/bundler
participant FS as Filesystem
participant CI as CI/Env
participant PM as Package Manager
Bundler->>Bundler: bundleJavascript(runtime, projectDir)
Bundler->>FS: detectPackageManager(projectDir) (pnpm/bun/yarn/npm)
alt runtime == bunjs
Bundler->>Bundler: generateBunLockfile (if logger present)
Bundler->>CI: check CI
Bundler->>PM: run `bun install` with CI-aware args
else runtime == nodejs and pnpm lock
Bundler->>CI: check CI
Bundler->>PM: run `pnpm install` (CI vs non-CI args)
else runtime == nodejs and yarn lock
Bundler->>PM: run `yarn install --frozen-lockfile`
else runtime == nodejs (default)
Bundler->>PM: run `npm install` with production flags
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/dev/dev.go (1)
128-138: NODE_OPTIONS is appended without a key (env var not set).The env slice requires entries of the form KEY=VALUE. Appending just
--enable-source-mapswon't set NODE_OPTIONS.Apply this diff:
- projectServerCmd.Env = append(projectServerCmd.Env, nodeOptions) + projectServerCmd.Env = append(projectServerCmd.Env, fmt.Sprintf("NODE_OPTIONS=%s", nodeOptions))Optional: avoid duplicating the flag if it already exists and keep imports tidy. I can push a follow-up change to add
strings.Contains(nodeOptions, "--enable-source-maps")and importstringsif you want.
🧹 Nitpick comments (2)
cmd/project.go (2)
41-71: Prefer package.json “packageManager” and cover npm-shrinkwrap.json to make detection more robust.Two low-risk tweaks improve fidelity without changing the overall intent:
- Read package.json’s packageManager (e.g., "pnpm@9", "yarn@4", "npm@10", "bun@1") and honor it first.
- Treat npm-shrinkwrap.json as npm (same lock semantics as package-lock.json).
Apply this diff within detectPackageManager:
func detectPackageManager(dir string, defaultRuntime string) string { // Only auto-detect for JavaScript-based runtimes if defaultRuntime != "nodejs" && defaultRuntime != "bunjs" && defaultRuntime != "pnpm" { return defaultRuntime } + // Prefer explicit packageManager declared in package.json if present. + if b, err := os.ReadFile(filepath.Join(dir, "package.json")); err == nil { + var pkg struct{ PackageManager string `json:"packageManager"` } + if json.Unmarshal(b, &pkg) == nil && pkg.PackageManager != "" { + name := pkg.PackageManager + if i := strings.IndexRune(name, '@'); i > 0 { + name = name[:i] + } + switch name { + case "pnpm": + return "pnpm" + case "bun": + return "bunjs" + case "yarn", "npm": + return "nodejs" + } + } + } + // Check for pnpm lockfile first (most specific) if util.Exists(filepath.Join(dir, "pnpm-lock.yaml")) { return "pnpm" } // Check for bun lockfiles if util.Exists(filepath.Join(dir, "bun.lockb")) || util.Exists(filepath.Join(dir, "bun.lock")) { return "bunjs" } // Check for npm lockfile - if util.Exists(filepath.Join(dir, "package-lock.json")) { + if util.Exists(filepath.Join(dir, "package-lock.json")) || util.Exists(filepath.Join(dir, "npm-shrinkwrap.json")) { return "nodejs" } // Check for yarn lockfile (fallback to nodejs for yarn projects) if util.Exists(filepath.Join(dir, "yarn.lock")) { return "nodejs" } // No lockfile found, use the template default return defaultRuntime }And add the missing import used above:
// add to imports at the top of this file import "strings"
41-71: Add table-driven unit tests for detection to prevent regressions.Recommend covering: explicit packageManager, each lockfile, no lockfile, and non-JS default runtime.
Here’s a minimal test you can drop in cmd/project_test.go:
package cmd import ( "os" "path/filepath" "testing" ) func write(t *testing.T, dir, name, body string) { t.Helper() fn := filepath.Join(dir, name) if err := os.WriteFile(fn, []byte(body), 0600); err != nil { t.Fatalf("write %s: %v", fn, err) } } func TestDetectPackageManager(t *testing.T) { t.Run("non-js default passthrough", func(t *testing.T) { tmp := t.TempDir() if got := detectPackageManager(tmp, "deno"); got != "deno" { t.Fatalf("got %s, want deno", got) } }) t.Run("packageManager pnpm", func(t *testing.T) { tmp := t.TempDir() write(t, tmp, "package.json", `{"packageManager":"pnpm@9.0.0"}`) if got := detectPackageManager(tmp, "nodejs"); got != "pnpm" { t.Fatalf("got %s, want pnpm", got) } }) t.Run("pnpm lock", func(t *testing.T) { tmp := t.TempDir() write(t, tmp, "pnpm-lock.yaml", "") if got := detectPackageManager(tmp, "nodejs"); got != "pnpm" { t.Fatalf("got %s, want pnpm", got) } }) t.Run("bun lock", func(t *testing.T) { tmp := t.TempDir() write(t, tmp, "bun.lockb", "") if got := detectPackageManager(tmp, "pnpm"); got != "bunjs" { t.Fatalf("got %s, want bunjs", got) } }) t.Run("npm lock and shrinkwrap", func(t *testing.T) { tmp := t.TempDir() write(t, tmp, "npm-shrinkwrap.json", "{}") if got := detectPackageManager(tmp, "pnpm"); got != "nodejs" { t.Fatalf("got %s, want nodejs", got) } }) t.Run("yarn lock maps to nodejs", func(t *testing.T) { tmp := t.TempDir() write(t, tmp, "yarn.lock", "") if got := detectPackageManager(tmp, "bunjs"); got != "nodejs" { t.Fatalf("got %s, want nodejs", got) } }) t.Run("no hints falls back to default", func(t *testing.T) { tmp := t.TempDir() if got := detectPackageManager(tmp, "pnpm"); got != "pnpm" { t.Fatalf("got %s, want pnpm", got) } }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
agentuity.schema.json(1 hunks)cmd/project.go(2 hunks)internal/bundler/bundler.go(1 hunks)internal/dev/dev.go(1 hunks)internal/mcp/project.go(1 hunks)internal/project/project.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/dev/dev.go (1)
internal/project/project.go (2)
Project(145-154)Bundler(481-490)
cmd/project.go (1)
internal/util/io.go (1)
Exists(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
internal/mcp/project.go (1)
26-26: LGTM — provider description updated to include pnpm.Doc-only change looks good and aligns with added pnpm support.
cmd/project.go (2)
41-47: Sensible scope limiting of auto-detect to JS runtimes — LGTM.Early return for non-JS runtimes avoids surprising overrides and keeps behavior predictable for Python/Deno templates.
175-175: Good integration: runtime now reflects on-disk reality.Using detectPackageManager here ensures the stored runtime matches the scaffolded project. No further action needed.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/bundler/bundler.go (1)
146-171: Extract install command selection for testability; then call it hereThe install selection logic is embedded in bundleJavascript, making it hard to test without invoking esbuild, typecheck, etc. Extract a pure function and use it both here and in tests. This will also fix the current tests being tautological.
Apply this focused change here to call the extractor:
- var install *exec.Cmd - switch theproject.Bundler.Runtime { - case "nodejs": - if util.Exists(filepath.Join(dir, "yarn.lock")) { - install = exec.CommandContext(ctx.Context, "yarn", "install", "--frozen-lockfile") - } else { - install = exec.CommandContext(ctx.Context, "npm", "install", "--no-audit", "--no-fund", "--include=prod", "--ignore-scripts") - } - case "bunjs": - args := []string{"install", "--production", "--no-save", "--ignore-scripts"} - if ctx.CI { - args = append(args, "--verbose", "--no-cache") - } else { - args = append(args, "--no-progress", "--no-summary", "--silent") - } - install = exec.CommandContext(ctx.Context, "bun", args...) - case "pnpm": - args := []string{"install", "--prod", "--ignore-scripts"} - if ctx.CI { - args = append(args, "--reporter=append-only", "--frozen-lockfile") - } else { - args = append(args, "--silent") - } - install = exec.CommandContext(ctx.Context, "pnpm", args...) - default: - return fmt.Errorf("unsupported runtime: %s", theproject.Bundler.Runtime) - } + cmdName, args, err := jsInstallCommandSpec(ctx, dir, theproject.Bundler.Runtime) + if err != nil { + return err + } + install := exec.CommandContext(ctx.Context, cmdName, args...)And add this helper (outside this hunk; place near bundleJavascript):
// jsInstallCommandSpec returns the package manager binary and arguments for the given runtime/dir/CI. func jsInstallCommandSpec(ctx BundleContext, dir, runtime string) (string, []string, error) { switch runtime { case "nodejs": if util.Exists(filepath.Join(dir, "yarn.lock")) { return "yarn", []string{"install", "--frozen-lockfile"}, nil } if ctx.CI && util.Exists(filepath.Join(dir, "package-lock.json")) { return "npm", []string{"ci", "--omit=dev", "--ignore-scripts", "--no-audit", "--no-fund"}, nil } return "npm", []string{"install", "--omit=dev", "--ignore-scripts", "--no-audit", "--no-fund"}, nil case "bunjs": args := []string{"install", "--production", "--ignore-scripts"} // --no-save is unnecessary for install; omit to reduce noise. if ctx.CI { args = append(args, "--verbose", "--no-cache") } else { args = append(args, "--no-progress", "--no-summary", "--silent") } return "bun", args, nil case "pnpm": args := []string{"install", "--prod", "--ignore-scripts"} if ctx.CI { args = append(args, "--frozen-lockfile", "--reporter=append-only") } else { args = append(args, "--silent") } return "pnpm", args, nil default: return "", nil, fmt.Errorf("unsupported runtime: %s", runtime) } }I can push a commit wiring this extraction and updating the tests if you’d like.
🧹 Nitpick comments (3)
internal/bundler/bundler.go (3)
150-154: NodeJS: prefer reproducible npm installs in CI and use omit=dev instead of include=prodCurrent fallback always uses “npm install … --include=prod”. In CI, “npm ci” is reproducible and will fail on lock drift; outside CI, “npm install … --omit=dev” is more idiomatic than “--include=prod”.
Apply this change:
- } else { - install = exec.CommandContext(ctx.Context, "npm", "install", "--no-audit", "--no-fund", "--include=prod", "--ignore-scripts") - } + } else { + if ctx.CI && util.Exists(filepath.Join(dir, "package-lock.json")) { + install = exec.CommandContext(ctx.Context, "npm", "ci", "--omit=dev", "--ignore-scripts", "--no-audit", "--no-fund") + } else { + install = exec.CommandContext(ctx.Context, "npm", "install", "--omit=dev", "--ignore-scripts", "--no-audit", "--no-fund") + } + }Note: npm ci requires package-lock.json; the guard avoids failures when it’s absent.
150-152: Yarn flag compatibility across Yarn v1 vs Berry“--frozen-lockfile” is correct for Yarn v1; Yarn Berry prefers “--immutable” (and optionally “--immutable-cache”). Optional: detect Berry (e.g., .yarnrc.yml with yarnPath) and switch flags, or append both flags to be tolerant.
Would you like me to wire a lightweight detector and test for it?
156-163: Bun install flags: verify necessity of --no-save on bun install“--no-save” is primarily relevant for add; install doesn’t update manifests. Keeping it is harmless if bun tolerates it, but consider removing to reduce noise. Otherwise LGTM on CI vs non-CI toggles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
agentuity.schema.json(1 hunks)internal/bundler/bundler.go(2 hunks)internal/bundler/bundler_test.go(2 hunks)internal/project/project.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/project/project.go
- agentuity.schema.json
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bundler/bundler.go (1)
internal/util/io.go (1)
Exists(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
internal/bundler/bundler.go (1)
163-171: PNPM flags LGTM and align with previous review feedbackCI: append-only reporter and frozen-lockfile; non-CI: silent. This replaces the invalid “--reporter=silent” and adds reproducibility in CI.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/bundler/bundler.go (1)
181-190: pnpm flags LGTM; previous reporter/silent issues are resolved.Non-CI uses
--silent; CI switches to--reporter=append-onlyand--frozen-lockfile. This matches pnpm best practices and avoids interleaved output.
🧹 Nitpick comments (5)
internal/bundler/bundler.go (5)
144-161: Prefer npm --omit=dev (or npm ci) and recognize Yarn Berry immutability.
- The npm flag
--include=prodis brittle and not consistently supported across npm versions. Prefer--omit=dev(or--production) to exclude dev deps.- Yarn Berry (v2+) uses
--immutableinstead of--frozen-lockfile. You can cheaply detect Berry via.yarnrc.ymlor.yarn/releases.Apply this diff to harden installs (base spec remains CI-agnostic):
-func jsInstallCommandSpec(ctx context.Context, projectDir, runtime string) (string, []string, error) { +func jsInstallCommandSpec(ctx context.Context, projectDir, runtime string) (string, []string, error) { switch runtime { case "nodejs": - if util.Exists(filepath.Join(projectDir, "yarn.lock")) { - return "yarn", []string{"install", "--frozen-lockfile"}, nil - } else { - return "npm", []string{"install", "--no-audit", "--no-fund", "--include=prod", "--ignore-scripts"}, nil - } + // Prefer Yarn when a yarn.lock is present; detect Berry immutability + if util.Exists(filepath.Join(projectDir, ".yarnrc.yml")) || util.Exists(filepath.Join(projectDir, ".yarn", "releases")) { + return "yarn", []string{"install", "--immutable"}, nil + } + if util.Exists(filepath.Join(projectDir, "yarn.lock")) { + return "yarn", []string{"install", "--frozen-lockfile"}, nil + } + // npm path + return "npm", []string{"install", "--omit=dev", "--ignore-scripts", "--no-audit", "--no-fund"}, nil case "bunjs": return "bun", []string{"install", "--production", "--no-save", "--ignore-scripts", "--no-progress", "--no-summary", "--silent"}, nil case "pnpm": return "pnpm", []string{"install", "--prod", "--ignore-scripts", "--silent"}, nil default: return "", nil, fmt.Errorf("unsupported runtime: %s", runtime) } }If you want
npm cionly in CI to avoid cleaning local node_modules, see my CI suggestion in a later comment.
Would you like me to add tests for Yarn Berry detection and npm flag behavior?
144-146: Nit: Unused ctx parameter; rename to underscore for clarity.The
ctx context.Contextparameter isn’t used. Rename to_to signal intent and avoid confusion with the outer BundleContext.-func jsInstallCommandSpec(ctx context.Context, projectDir, runtime string) (string, []string, error) { +func jsInstallCommandSpec(_ context.Context, projectDir, runtime string) (string, []string, error) {
163-194: Avoid mutating a slice while ranging; current i-- is a no-op.In both loops you modify
argswhile iterating withrange. That’s safe here becauserangeiterates over a snapshot, but thei--has no effect and the code is harder to reason about. Prefer filtering into a new slice for clarity and correctness.if ctx.CI { if runtime == "bunjs" { - // Replace silent flags with verbose for CI - for i, arg := range args { - if arg == "--no-progress" || arg == "--no-summary" || arg == "--silent" { - args = append(args[:i], args[i+1:]...) - i-- - } - } + // Replace silent flags with verbose for CI + drop := map[string]struct{}{"--no-progress": {}, "--no-summary": {}, "--silent": {}} + filtered := make([]string, 0, len(args)) + for _, a := range args { + if _, rm := drop[a]; !rm { + filtered = append(filtered, a) + } + } + args = filtered args = append(args, "--verbose", "--no-cache") } else if runtime == "pnpm" { // Remove silent flag and add CI-specific flags - for i, arg := range args { - if arg == "--silent" { - args = append(args[:i], args[i+1:]...) - break - } - } + for i := 0; i < len(args); i++ { + if args[i] == "--silent" { + args = append(args[:i], args[i+1:]...) + break + } + } args = append(args, "--reporter=append-only", "--frozen-lockfile") } }
163-194: Make npm installs deterministic in CI (use npm ci when lockfile exists).For the nodejs runtime path where npm is selected, CI runs benefit from
npm cifor speed and reproducibility.func getJSInstallCommand(ctx BundleContext, projectDir, runtime string) (string, []string, error) { cmd, args, err := jsInstallCommandSpec(ctx.Context, projectDir, runtime) if err != nil { return "", nil, err } // Apply CI-specific modifications if ctx.CI { if runtime == "bunjs" { ... - } else if runtime == "pnpm" { + } else if runtime == "pnpm" { ... + } else if runtime == "nodejs" && cmd == "npm" { + // Prefer deterministic npm installs in CI + if util.Exists(filepath.Join(projectDir, "package-lock.json")) { + args = []string{"ci", "--omit=dev", "--ignore-scripts", "--no-audit", "--no-fund"} + } else { + // Fall back to install, still omit dev deps + args = []string{"install", "--omit=dev", "--ignore-scripts", "--no-audit", "--no-fund"} + } } }If you want, I can add tests mirroring the pnpm CI tests for the npm/lockfile cases.
196-204: Stream installer output to the user in long-running installs.
CombinedOutput()buffers everything; for large dependency trees the feedback comes late. Consider streaming toctx.Writer(and keeping a trace buffer if you still want post-mortem logs).- install := exec.CommandContext(ctx.Context, cmd, args...) + install := exec.CommandContext(ctx.Context, cmd, args...) + // Stream logs for better UX during long installs + install.Stdout = ctx.Writer + install.Stderr = ctx.WriterIf you adopt this, adjust the error handling below to use
install.Run()and omit theout-based messages (or capture to a bytes.Buffer via io.MultiWriter for trace logs).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/bundler/bundler.go(1 hunks)internal/bundler/bundler_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/bundler/bundler_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bundler/bundler.go (2)
internal/util/io.go (1)
Exists(14-19)internal/project/project.go (2)
Project(145-154)Bundler(481-490)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
agentuity.schema.json(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Build and Test (macos-latest)
🔇 Additional comments (1)
agentuity.schema.json (1)
224-224: No substantive change at EOF.Closing brace touched; nothing to review here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/project.go (1)
64-67: Ensure yarn.lock triggers “yarn install” downstream (not npm) [duplicate of earlier concern].Runtime remains “nodejs” for Yarn, so the bundler/dev paths must switch installers based on lockfiles.
#!/bin/bash set -euo pipefail # Look for installer branching in bundler/dev flows rg -nP -C3 'exec\.Command\(\s*"(npm|yarn|pnpm)"' internal/bundler internal/dev || true rg -nP -C2 'yarn\.lock|pnpm-lock\.yaml|package-lock\.json|bun\.lockb?' internal/bundler internal/dev || true # Ideally we see: # - yarn.lock -> yarn install --frozen-lockfile # - pnpm-lock.yaml -> pnpm install --frozen-lockfile (CI-appropriate flags) # - package-lock.json (or none) -> npm ci/install with safe flags
🧹 Nitpick comments (2)
cmd/project.go (2)
41-43: Name/doc mismatch: this returns a runtime, not a package manager (optional rename).Consider renaming and tightening the comment to avoid confusion.
Apply:
-// detectPackageManager auto-detects the package manager based on lockfiles in the directory -// If no lockfile is found, falls back to the default runtime from the template -func detectPackageManager(dir string, defaultRuntime string) string { +// resolveJSRuntimeFromLockfiles selects the JS runtime (nodejs|bunjs) by inspecting lockfiles in dir. +// It does not encode the package manager (npm|yarn|pnpm); downstream code should choose installer accordingly. +// If no relevant lockfile is found, it falls back to the template-provided runtime. +func resolveJSRuntimeFromLockfiles(dir string, defaultRuntime string) string {
175-175: Update call site if you adopt the helper rename.- Runtime: detectPackageManager(args.Dir, args.Provider.Runtime), + Runtime: resolveJSRuntimeFromLockfiles(args.Dir, args.Provider.Runtime),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cmd/project.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/project.go (1)
internal/util/io.go (1)
Exists(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
cmd/project.go (3)
41-72: Lockfile precedence and JS-only guard look good.bun > pnpm > npm > yarn ordering is sensible, and restricting detection to JS runtimes avoids accidental overrides.
170-180: Confirm and document all supported bundler runtimesThe code currently accepts more than just “nodejs” and “bunjs” for
Bundler.Runtime. Please verify and update as needed:
- Ensure the JSON-schema tags (if any) and any CLI or API documentation for
project.Bundler.Runtimelist the full set of valid values:
• bunjs
• nodejs
• pnpm
• deno- Confirm that
detectPackageManager(args.Dir, args.Provider.Runtime)only ever returns one of those four and never “yarn” (or other unsupported strings).- If the schema or docs are out of date, please update the struct tags and documentation comments in:
•internal/project/project.go(runtime validation)
• Any JSON-schema annotations in the project-definition package
44-48: JS Templates Must Declare “nodejs” or “bunjs” as RuntimeI wasn’t able to locate any template definition files in the repo to confirm their
runtimefields. Since the code only auto-detects JavaScript runtimes whendefaultRuntimeis exactly"nodejs"or"bunjs", please manually verify that all JS-based templates include one of these values (and none are left empty or set to a different string).• Verify that every template’s
rules.yaml(or equivalent) under each<templateDir>/<identifier>/definesruntime: "nodejs"orruntime: "bunjs".
• Ensure no JS template has an empty, missing, or non-(nodejs|bunjs)runtime value—otherwise detection will be skipped.
• If templates are sourced externally or embedded differently, confirm the runtime defaults are applied before loading.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
internal/bundler/bundler.go (1)
170-210: Optional: strengthen CI installs for npm/yarn.
- npm: prefer “ci” and enforce --omit=dev.
- yarn: ensure --immutable on CI (Yarn 2+/Berry).
Apply this diff:
if ctx.CI { if runtime == "bunjs" { ... - } else if cmd == "pnpm" { + } else if cmd == "pnpm" { ... + } else if cmd == "npm" { + if len(args) > 0 && args[0] == "install" { + args[0] = "ci" + } + seen := false + for _, a := range args { + if a == "--omit=dev" { + seen = true + break + } + } + if !seen { + args = append(args, "--omit=dev") + } + } else if cmd == "yarn" { + hasImmutable := false + for _, a := range args { + if a == "--immutable" || a == "--frozen-lockfile" { + hasImmutable = true + break + } + } + if !hasImmutable { + args = append(args, "--immutable") + } } }internal/project/project.go (1)
196-202: Nit: clarify error messages to avoid “pnpm as runtime” confusion.Consider hinting that the package manager is auto-detected from lockfiles; runtime is the JS engine.
Example:
- return fmt.Errorf("invalid bundler.runtime value: %s. only bunjs and nodejs are supported", p.Bundler.Runtime) + return fmt.Errorf("invalid bundler.runtime value: %s. only bunjs and nodejs are supported (runtime refers to the JS engine; package manager is auto-detected from lockfiles)", p.Bundler.Runtime)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/bundler/bundler.go(2 hunks)internal/project/project.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bundler/bundler.go (2)
internal/util/io.go (1)
Exists(14-19)internal/project/project.go (1)
Bundler(493-502)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/bundler/bundler.go (1)
212-238: LGTM: install execution flow and diagnostics.Refactor to getJSInstallCommand + detailed exit-code reporting looks good.
internal/project/project.go (1)
196-202: LGTM: runtime validation aligned (JS: bunjs/nodejs; Python: uv).Matches schema intent and keeps package manager separate from runtime.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/bundler/bundler.go (2)
188-197: Do not mutate the slice while ranging; quiet flags may be skipped.Removing items from
argsinside arangecan skip elements. Build a filtered copy.- if runtime == "bunjs" { - // Replace silent flags with verbose for CI - for i, arg := range args { - if arg == "--no-progress" || arg == "--no-summary" || arg == "--silent" { - args = append(args[:i], args[i+1:]...) - i-- - } - } - args = append(args, "--verbose", "--no-cache") + if runtime == "bunjs" { + // Drop quiet flags for CI using a filtered copy + filtered := make([]string, 0, len(args)) + for _, arg := range args { + if arg == "--no-progress" || arg == "--no-summary" || arg == "--silent" { + continue + } + filtered = append(filtered, arg) + } + args = filtered + args = append(args, "--verbose", "--no-cache")
197-206: Fix PNPM CI flags gating and argument handlingThe PNPM-specific CI flags block is still gated on
runtime == "pnpm", butruntimeonly ever reflects the JavaScript engine (e.g. “nodejs” or “bunjs”), so this branch never executes. Additionally, removing--silentvia in-place slicing can lead to index errors—use a filtered slice instead.Locations to update:
- internal/bundler/bundler.go, lines 197–206
Proposed fix:
--- a/internal/bundler/bundler.go +++ b/internal/bundler/bundler.go @@ -195,11 +195,16 @@ func buildArgs(runtime, cmd string, args []string) []string { - } else if runtime == "pnpm" { + } else if cmd == "pnpm" { // Remove silent flag and add CI-specific flags - for i, arg := range args { - if arg == "--silent" { - args = append(args[:i], args[i+1:]...) - break - } - } - args = append(args, "--reporter=append-only", "--frozen-lockfile") + filtered := make([]string, 0, len(args)) + for _, arg := range args { + if arg == "--silent" { + continue + } + filtered = append(filtered, arg) + } + args = filtered + args = append(args, "--reporter=append-only", "--frozen-lockfile") } return args }
🧹 Nitpick comments (1)
internal/bundler/bundler.go (1)
151-153: Yarn: install only production deps in non-Dev builds.Consider excluding devDeps to match npm/pnpm/bun behavior.
- return "yarn", []string{"install", "--frozen-lockfile"}, nil + return "yarn", []string{"install", "--production=true", "--frozen-lockfile"}, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cmd/project.go(2 hunks)internal/bundler/bundler.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/project.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bundler/bundler.go (2)
internal/util/io.go (1)
Exists(14-19)internal/project/project.go (2)
Project(152-161)Bundler(493-502)
🔇 Additional comments (1)
internal/bundler/bundler.go (1)
212-220: LGTM: centralizing install command generation improves maintainability.The switch to
getJSInstallCommandwith unified logging and error handling is a good refactor.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/bundler/bundler.go (4)
144-155: Lockfile-based PM detection is correct; nice inclusion of bun.lockb.Optional: consider detecting Yarn Berry (.yarnrc.yml with yarnPath/nodeLinker) to tailor flags accordingly.
157-175: Tiny UX win: make Yarn non-interactive by default.Safer in scripts and CI even outside applyCIModifications.
case "yarn": - return "yarn", []string{"install", "--frozen-lockfile"}, nil + return "yarn", []string{"install", "--frozen-lockfile", "--non-interactive"}, nil
188-219: Remove unused parameter ‘runtime’ from applyCIModifications.It isn’t used and adds noise; simplify signature and call site.
-func applyCIModifications(ctx BundleContext, cmd, runtime string, args []string) []string { +func applyCIModifications(ctx BundleContext, cmd string, args []string) []string {And update the caller:
-args = applyCIModifications(ctx, cmd, runtime, args) +args = applyCIModifications(ctx, cmd, args)
220-241: Prefer reproducible npm installs in CI: switch to ‘npm ci’ when package-lock.json exists.This aligns with npm’s guidance and reduces flakiness.
cmd, args, err := jsInstallCommandSpec(projectDir) if err != nil { return "", nil, err } -// Apply CI-specific modifications -args = applyCIModifications(ctx, cmd, runtime, args) +// Prefer reproducible installs for npm in CI when a lockfile is present +if ctx.CI && cmd == "npm" && util.Exists(filepath.Join(projectDir, "package-lock.json")) { + args = []string{"ci", "--omit=dev", "--ignore-scripts", "--no-audit", "--no-fund"} +} + +// Apply CI-specific modifications +args = applyCIModifications(ctx, cmd, args) return cmd, args, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
cmd/project.go(2 hunks)internal/bundler/bundler.go(2 hunks)internal/bundler/bundler_test.go(2 hunks)internal/dev/dev.go(1 hunks)internal/mcp/project.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/dev/dev.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/project.go
- internal/bundler/bundler_test.go
- internal/mcp/project.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/bundler/bundler.go (2)
internal/util/io.go (1)
Exists(14-19)internal/project/project.go (2)
Project(152-161)Bundler(493-502)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Test (macos-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
internal/bundler/bundler.go (1)
244-268: Install integration via getJSInstallCommand looks good.Good separation of concerns and error reporting on exit codes.
potofpie
dismissed
jhaynie’s stale review
I went through your comment with Gabe and it looks good now
- Added: Adds support for pnpm as a bundler (#438) - Fixed: bump go-common to get light mode color fixes Co-Authored-By: unknown <>
Probably needs more updates on the deployment side for fully work.
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests