refactor(cli): decouple scaffolding logic with a planner#998
Conversation
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR refactors the ArkEnv CLI initialization command using a three-phase architecture: a collector gathers project state and user input, a pure planner generates a declarative scaffolding plan, and an executor applies the plan via workspace and reporter abstractions. New type contracts and implementations decouple logic from side effects, enabling testability and dry-run capability. ChangesScaffolding Architecture Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
🤖 Hi @yamcodes, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
c86db84 to
144a45c
Compare
arkenv
@arkenv/bun-plugin
@arkenv/cli
@arkenv/fumadocs-ui
@arkenv/vite-plugin
commit: |
📦 Bundle Size Report✅ All size limits passed! |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
🤖 I'm sorry @yamcodes, but I was unable to process your request. Please see the logs for more details. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/cli/src/plan.ts (1)
67-73: ⚡ Quick winConsider unifying bootstrap return types.
The
bootstrapViteConfigandbootstrapBunConfigmethods return different shapes:
- Vite:
{ success, updated, error? }- Bun:
{ success, instructions?, error? }This asymmetry requires callers to handle framework-specific property names. Consider a unified shape such as:
{ success: boolean; updated?: boolean; instructions?: string; error?: string }This would simplify executor logic and make the API more predictable.
🤖 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 `@packages/cli/src/plan.ts` around lines 67 - 73, Unify the return shapes of bootstrapViteConfig and bootstrapBunConfig by introducing a single common return type (e.g. { success: boolean; updated?: boolean; instructions?: string; error?: string }) and update the method signatures for bootstrapViteConfig(path: string, importPath: string) and bootstrapBunConfig(path: string) to both return Promise of that type; adjust any callers to use the new optional fields (use updated when applicable for Vite and instructions when applicable for Bun) and update any related types or interfaces to reflect the unified shape.
🤖 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.
Inline comments:
In `@packages/cli/src/commands/init.ts`:
- Around line 115-119: The code currently computes typeFileName via a ternary
(typeFileName = options.framework === "vite" ? "vite-env.d.ts" : "bun-env.d.ts")
which incorrectly falls back to "bun-env.d.ts" for "node"; change the logic to
only assign and check a type file when options.framework is "vite" or "bun"
(e.g., if options.framework === "vite" set "vite-env.d.ts", else if === "bun"
set "bun-env.d.ts"), and only compute typeFilePath and push into existingFiles
when that typeFileName is defined; ensure references are to typeFileName,
typeFilePath, options.framework and existingFiles so downstream
CollectedState.existingFiles does not get stray bun-env.d.ts entries for node
projects.
In `@packages/cli/src/executor.test.ts`:
- Around line 1-35: Add a beforeEach that isolates test state by clearing Vitest
mocks and resetting SKIP_INSTALL and the executor instance: call
vi.clearAllMocks() (or vi.resetAllMocks()) to reset mock call history for
mockWorkspace and mockReporter, delete or set process.env.SKIP_INSTALL =
undefined to ensure no env leakage, and reinstantiate the Executor (create a new
executor = new Executor(mockWorkspace, mockReporter)) inside beforeEach so each
test gets a fresh instance.
In `@packages/cli/src/executor.ts`:
- Around line 109-120: When plan.bootstrap.framework === "bun" and
this.workspace.findBunConfig() returns no path, add a user-facing fallback
message so users know bootstrap was skipped and how to proceed manually; update
the branch handling in executor (the block around this.workspace.findBunConfig
and this.workspace.bootstrapBunConfig) to call this.reporter.info or
this.reporter.warn with a short guidance string (e.g., "No Bun config found —
create a bun.config.ts or run `bun init` to bootstrap manually") so setup
doesn’t appear complete when Bun was skipped.
- Around line 68-70: The executor currently ignores plan.tsConfig.path and
always calls this.workspace.updateTsConfigToStrict() without a target, breaking
non-default tsconfig handling; change the call in the block that checks if
(plan.tsConfig) to pass the planned path (plan.tsConfig.path) into
updateTsConfigToStrict (or its equivalent parameter) so updateTsConfigToStrict
receives the target tsconfig file to modify; update any related handling of
tsResult (e.g., tsResult.status checks) to remain the same after passing the
path.
In `@packages/cli/src/lib/workspace.ts`:
- Around line 14-17: The constructor currently types stdio as any which bypasses
type checking; change the stdio parameter type to StdioOptions from
node:child_process (import StdioOptions) and update the constructor signature
(constructor(private isQuiet: boolean, private stdio: StdioOptions) {}) so the
value passed into spawn() in the method that calls spawn() is correctly typed
and validated.
---
Nitpick comments:
In `@packages/cli/src/plan.ts`:
- Around line 67-73: Unify the return shapes of bootstrapViteConfig and
bootstrapBunConfig by introducing a single common return type (e.g. { success:
boolean; updated?: boolean; instructions?: string; error?: string }) and update
the method signatures for bootstrapViteConfig(path: string, importPath: string)
and bootstrapBunConfig(path: string) to both return Promise of that type; adjust
any callers to use the new optional fields (use updated when applicable for Vite
and instructions when applicable for Bun) and update any related types or
interfaces to reflect the unified shape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0de8622b-1d50-4c91-b96e-e592b18a368a
📒 Files selected for processing (11)
packages/cli/src/commands/init.test.tspackages/cli/src/commands/init.tspackages/cli/src/executor.test.tspackages/cli/src/executor.tspackages/cli/src/lib/reporter.tspackages/cli/src/lib/workspace.tspackages/cli/src/plan.tspackages/cli/src/planner.test.tspackages/cli/src/planner.tspackages/cli/src/scaffold.test.tspackages/cli/src/scaffold.ts
…ecouple-scaffolding-logic-with-a-planner-1
|
@gemini-cli /review |
|
🤖 Hi @yamcodes, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
This PR introduces a significant architectural improvement to the InitCommand by decoupling the scaffolding logic into Collector, Planner, and Executor phases. This makes the logic more declarative, easier to test, and improves maintainability by isolating side effects.
🔍 General Feedback
- Well-Structured: The separation of concerns into
Planner(pure logic) andExecutor(side effects) is excellent and follows best practices. - Improved Testability: The addition of comprehensive unit tests for the Planner and Executor confirms the robustness of the new design.
- Security Consideration: Note the suggestion regarding
spawnandshell: trueto avoid potential command injection vulnerabilities. - Clean Code: The refactoring removes a lot of complexity from the main command class and the legacy
scaffoldfunction.
|
|
||
| let stdout = ""; | ||
| let stderr = ""; | ||
| const MAX_BUFFER = 10_000; |
There was a problem hiding this comment.
🟢 10,000 characters for MAX_BUFFER is generally sufficient for error logs, but for extremely verbose installation failures, it might truncate useful context. Consider if this should be configurable or slightly larger.
| const MAX_BUFFER = 10_000; | |
| const MAX_BUFFER = 10_000; |
|
|
||
| if (file.label === "environment schema") { | ||
| // We'll report this at the end or as we go | ||
| } else if (file.label?.includes("types")) { |
There was a problem hiding this comment.
🟢 This block is empty and seems to be a placeholder. It could be removed if no action is needed for the "environment schema" label.
| } else if (file.label?.includes("types")) { | |
| if (file.label === "environment schema") { | |
| // No specific reporting needed here | |
| } else if (file.label?.includes("types")) { |
| return new Promise<void>((resolve, reject) => { | ||
| const child = spawn(command, [], { | ||
| stdio: this.isQuiet ? "pipe" : this.stdio, | ||
| shell: true, |
There was a problem hiding this comment.
🟡 The use of shell: true in spawn can be a security risk if any part of the command string is influenced by external input. While currently safe with internal inputs, it's generally better to avoid shell: true and pass arguments as an array to prevent command injection.
| shell: true, | |
| const child = spawn(command, [], { | |
| stdio: this.isQuiet ? "pipe" : this.stdio, | |
| }); |
Wait, if you remove shell: true, you must pass the command and args separately. Consider refactoring execute to accept args: string[].
…o 982-decouple-scaffolding-logic-with-a-planner-1
715c31b
into
979-epic-arkenv-cli-architectural-deepening
Fixes #982\n\nThis PR refactors the
InitCommandto follow a declarative "Planner" pattern, decoupling state collection, planning, and execution.\n\n### Changes\n- Extracted a Collector phase to gather environment state and user input.\n- Implemented a pure Planner function that produces aScaffoldingPlanobject.\n- Implemented an Executor that applies the plan usingWorkspaceandReporterabstractions.\n- Added comprehensive unit tests for the Planner and Executor.\n- Cleaned up the legacyscaffoldfunction.\n\n### Benefits\n- Pure logic in the Planner is now easily unit-testable.\n- Side effects are isolated in the Executor.\n- Improved separation of concerns and maintainability.Summary by CodeRabbit
Release Notes