fix(cli): improve multi-platform command execution (Windows scaffold fix) - #4
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe exec helper was refactored to accept a command array plus optional cwd, with input validation and try/catch error handling; call sites in CLI phases were updated to invoke exec with argv-style arrays and explicit cwd where needed. A new test suite verifies stdout capture, cwd behavior, and non-zero exit codes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/cli/src/lib/__tests__/helpers.exec.test.ts (1)
16-42: Consider adding coverage for the two new guard/catch paths inexec.The new implementation added two code paths that the existing three tests don't exercise:
- Empty-command guard (returns
exitCode: 1, stderr: "No command provided").catchblock whenBun.spawnthrows (returnsexitCode: 127).♻️ Suggested additional test cases
+ it("returns exitCode 1 with a descriptive error for an empty command", async () => { + const result = await exec([]); + + expect(result.exitCode).toBe(1); + expect(result.stderr).toBe("No command provided"); + }); + + it("returns exitCode 127 when the executable does not exist", async () => { + const result = await exec(["__nonexistent_binary_xyz__"]); + + expect(result.exitCode).toBe(127); + expect(result.stderr.length).toBeGreaterThan(0); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/lib/__tests__/helpers.exec.test.ts` around lines 16 - 42, Add two tests for exec to cover the new guard/catch paths: one test should call exec with an empty command (e.g., [] or []) and assert it returns exitCode: 1 and stderr contains "No command provided"; another should simulate Bun.spawn throwing (mock/spy Bun.spawn to throw) and assert exec returns exitCode: 127 (and appropriate stderr) rather than throwing. Locate the exec helper referenced in the tests and add these assertions in packages/cli/src/lib/__tests__/helpers.exec.test.ts alongside the existing "exec" tests so both guard and catch branches are exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/lib/__tests__/helpers.exec.test.ts`:
- Around line 24-35: The cwd assertion can be flaky on macOS due to /tmp being a
symlink; update the test in the "respects cwd" case to compare canonical paths
by resolving the temp dir before asserting: call fs.realpathSync(cwd) (or
path.resolve + fs.realpathSync) and compare that resolved path to result.stdout
(trimmed if needed) instead of comparing to the original cwd; modify the test
that calls exec(...) and uses expect(result.stdout).toBe(cwd) to use the
resolved path from fs.realpathSync(cwd).
In `@packages/cli/src/lib/helpers.ts`:
- Around line 97-99: The current sequential reads await new
Response(proc.stdout).text() then await new Response(proc.stderr).text(), which
can deadlock if the child fills the OS pipe buffer; change to drain both streams
concurrently by starting both text() reads at once (e.g., Promise.all for the
two Response(...).text() promises) and await proc.exited separately or together
as appropriate so stdout and stderr are read in parallel; update references
around proc, proc.stdout, proc.stderr, stdout, stderr, and proc.exited in the
helpers.ts block to use Promise.all to avoid the pipe-fill deadlock.
---
Nitpick comments:
In `@packages/cli/src/lib/__tests__/helpers.exec.test.ts`:
- Around line 16-42: Add two tests for exec to cover the new guard/catch paths:
one test should call exec with an empty command (e.g., [] or []) and assert it
returns exitCode: 1 and stderr contains "No command provided"; another should
simulate Bun.spawn throwing (mock/spy Bun.spawn to throw) and assert exec
returns exitCode: 127 (and appropriate stderr) rather than throwing. Locate the
exec helper referenced in the tests and add these assertions in
packages/cli/src/lib/__tests__/helpers.exec.test.ts alongside the existing
"exec" tests so both guard and catch branches are exercised.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/cli/src/lib/__tests__/helpers.exec.test.tspackages/cli/src/lib/helpers.tspackages/cli/src/phases/database.tspackages/cli/src/phases/infra.tspackages/cli/src/phases/scaffold.ts
Replace string-concatenated paths with path.join in state.ts and theme-apply.ts for Windows compatibility. Fix exec cwd test to resolve symlinks (macOS /var -> /private/var). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/src/theme/theme-apply.ts (1)
81-91: Spinner left dangling ifwriteFileSyncthrows.If the
srcsubdirectory doesn't exist undertargetDir(or a permissions error occurs),writeFileSyncthrows befores.stop()is reached, leaving the terminal spinner running indefinitely.♻️ Proposed fix: wrap the write in try/finally
export function applyTheme(targetDir: string, config: ThemeConfig): void { const s = spinner(); s.start("Applying theme..."); - const css = generateFullAppCss(config); - writeFileSync(join(targetDir, "src", "app.css"), css, "utf-8"); - - s.stop( - `Theme applied: ${config.theme} (base: ${config.baseColor}, radius: ${config.radius}, font: ${config.font})` - ); + try { + const css = generateFullAppCss(config); + writeFileSync(join(targetDir, "src", "app.css"), css, "utf-8"); + s.stop( + `Theme applied: ${config.theme} (base: ${config.baseColor}, radius: ${config.radius}, font: ${config.font})` + ); + } catch (err) { + s.stop("Failed to apply theme"); + throw err; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/theme/theme-apply.ts` around lines 81 - 91, The spinner started in applyTheme (variable s) can be left running if writeFileSync throws; wrap the file write (generateFullAppCss(...) and writeFileSync(...)) in a try/finally so s.stop() (or s.fail(...) on error) is always called; specifically, move the css generation/write into a try block and place s.stop(...) in the finally block (optionally call s.fail with the caught error before rethrowing) to ensure the spinner is cleaned up even on writeFileSync or permission errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/cli/src/theme/theme-apply.ts`:
- Around line 81-91: The spinner started in applyTheme (variable s) can be left
running if writeFileSync throws; wrap the file write (generateFullAppCss(...)
and writeFileSync(...)) in a try/finally so s.stop() (or s.fail(...) on error)
is always called; specifically, move the css generation/write into a try block
and place s.stop(...) in the finally block (optionally call s.fail with the
caught error before rethrowing) to ensure the spinner is cleaned up even on
writeFileSync or permission errors.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/cli/src/lib/__tests__/helpers.exec.test.tspackages/cli/src/lib/state.tspackages/cli/src/theme/theme-apply.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/lib/tests/helpers.exec.test.ts
Sequential reads can deadlock when child process fills stderr pipe buffer (~64KB) before stdout is consumed. Use Promise.all to drain both streams in parallel. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes Windows scaffold/setup failures by removing shell-dependent command execution in the CLI and switching to explicit argv + cwd process execution.
What changed
exechelper to useBun.spawn(commandArgs, { cwd })instead ofsh -cscaffold:git init,bun installdatabase:bunx get-db --yes --env .env --key DATABASE_URL,bun --env-file=.env drizzle-kit push --forceinfra:docker compose up -d <service>rm -f) with Node API (rmSync(..., { force: true }))path.resolve(...)cwdWhy
The previous implementation relied on POSIX shell behavior (
sh -c,cd && ...,rm -f) which is brittle on Windows and causedcreate-start-kit-devto fail during project creation/install.Validation
bun test packages/cli/srccd packages/cli && bun run buildImpact
Summary by CodeRabbit
Improvements
Tests