Conversation
|
Caution Review failedThe pull request is closed. WalkthroughReplaces the Meow single-command CLI with a Commander-based multi-command CLI (env, workspace, agent, change), adds Ink-based command UIs and a Table component, introduces a DB singleton, normalizes import paths (remove .js), switches build/start to Bun, and updates dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Bun as Bun (runtime)
participant CLI as Commander (cli.tsx)
participant Cmd as Command Module
participant Ink as Ink Renderer
participant Orch as Orchestrator
participant DB as Lowdb (getDb)
User->>Bun: run CLI (bun src/cli.tsx ...)
Bun->>CLI: deliver argv
CLI->>Cmd: route subcommand (env|workspace|agent|change)
Cmd->>Ink: render component (e.g., EnvList)
Ink->>Orch: call <orchestrator>.list/get/create/...
Orch->>DB: read/write db.json via getDb()
DB-->>Orch: return result
Orch-->>Ink: return data/result
Ink-->>User: render table / message
Ink->>CLI: onComplete -> exit process
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
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: 7
🧹 Nitpick comments (7)
apps/cli/src/components/Table.tsx (1)
4-6: Consider improving type safety.The use of
Record<string, any>[]reduces type safety. Consider using a generic type parameter to maintain type information:-interface TableProps { - data: Record<string, any>[]; +interface TableProps<T extends Record<string, unknown>> { + data: T[]; }Then update the component signature:
-export default function Table({ data }: TableProps) { +export default function Table<T extends Record<string, unknown>>({ data }: TableProps<T>) {This preserves type information while allowing flexible data structures.
apps/cli/src/lib/db.ts (1)
17-21: Consider using async filesystem operations.Using synchronous
existsSyncandmkdirSyncblocks the event loop during CLI initialization. While the impact is minimal for a CLI tool, async operations would be more idiomatic in a Node.js context.Apply this diff to use async filesystem operations:
-import { mkdirSync, existsSync } from "node:fs"; +import { mkdir, access } from "node:fs/promises"; +import { constants } from "node:fs"; -export function getDb(): LowdbAdapter { +export async function getDb(): Promise<LowdbAdapter> { if (adapter) { return adapter; } // Create ~/.superset directory if it doesn't exist const supersetDir = join(homedir(), ".superset"); - if (!existsSync(supersetDir)) { - mkdirSync(supersetDir, { recursive: true }); - } + try { + await access(supersetDir, constants.F_OK); + } catch { + await mkdir(supersetDir, { recursive: true }); + } const dbPath = join(supersetDir, "db.json"); adapter = new LowdbAdapter(dbPath); return adapter; }Note: This would require updating all
getDb()call sites toawait getDb().apps/cli/src/cli.tsx (1)
93-99: Consider extracting type validation logic.The type validation pattern is repeated across WorkspaceType, ProcessType, and AgentType. Extracting this into a helper function would improve maintainability.
Add a helper function at the top of the file:
function validateEnum<T extends Record<string, string>>( value: string, enumObj: T, typeName: string ): value is T[keyof T] { if (!Object.values(enumObj).includes(value as T[keyof T])) { console.error( `Invalid ${typeName}: ${value}. Must be one of: ${Object.values(enumObj).join(", ")}` ); process.exit(1); } return true; }Then simplify the validation calls:
// Workspace validation validateEnum(type, WorkspaceType, "workspace type"); // Process validation validateEnum(type, ProcessType, "process type"); // Agent validation (conditional) if (options.agentType) { validateEnum(options.agentType, AgentType, "agent type"); agentType = options.agentType as AgentType; }Also applies to: 148-154, 157-166
apps/cli/src/commands/workspace.tsx (1)
13-68: Consider using proper types instead ofany.The state variables use
anytypes, reducing type safety. Consider importing and using the properWorkspacetype from your type definitions.Apply this pattern:
+import type { Workspace } from "../types/workspace"; export function WorkspaceList({ environmentId, onComplete }: WorkspaceListProps) { - const [workspaces, setWorkspaces] = React.useState<any[]>([]); + const [workspaces, setWorkspaces] = React.useState<Workspace[]>([]);Apply similar changes to
WorkspaceGetandWorkspaceCreatecomponents.apps/cli/src/commands/change.tsx (1)
27-31: Consider extracting date formatting logic.The date formatting pattern is duplicated. A helper function would improve maintainability.
Add a helper at the top of the file:
function formatChangeDate(change: any) { return { ...change, createdAt: new Date(change.createdAt).toLocaleString(), }; }Then use it in both components:
// In ChangeList const formatted = chgs.map(formatChangeDate); // In ChangeCreate const formatted = formatChangeDate(chg);Also applies to: 92-96
apps/cli/src/commands/env.tsx (2)
11-57: Replaceanytypes with properEnvironmenttype.All state variables use
anytypes. Using the properEnvironmenttype would improve type safety and align with TypeScript best practices.Apply this pattern across all four components:
+import type { Environment } from "../types/environment"; export function EnvList({ onComplete }: EnvListProps) { - const [environments, setEnvironments] = React.useState<any[]>([]); + const [environments, setEnvironments] = React.useState<Environment[]>([]); export function EnvGet({ id, onComplete }: EnvGetProps) { - const [environment, setEnvironment] = React.useState<any | null>(null); + const [environment, setEnvironment] = React.useState<Environment | null>(null); export function EnvCreate({ onComplete }: EnvCreateProps) { - const [environment, setEnvironment] = React.useState<any | null>(null); + const [environment, setEnvironment] = React.useState<Environment | null>(null);Also applies to: 64-101, 107-144, 151-195
1-195: Consider abstracting the common command component pattern.The env, workspace, agent, and change command files share nearly identical structure for list/get/create/delete operations. Consider creating a higher-order component or custom hook to reduce duplication.
Example approach with a custom hook:
// hooks/useCommand.ts function useCommand<T>( operation: () => Promise<T>, deps: any[] ) { const [data, setData] = React.useState<T | null>(null); const [error, setError] = React.useState<string | null>(null); const [loading, setLoading] = React.useState(true); React.useEffect(() => { const execute = async () => { try { const result = await operation(); setData(result); } catch (err) { setError(err instanceof Error ? err.message : "Unknown error"); } finally { setLoading(false); } }; execute(); }, deps); return { data, error, loading }; }This would eliminate ~70% of the duplicated code across command files while maintaining the same behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
apps/cli/package.json(3 hunks)apps/cli/src/cli.tsx(1 hunks)apps/cli/src/commands/agent.tsx(1 hunks)apps/cli/src/commands/change.tsx(1 hunks)apps/cli/src/commands/env.tsx(1 hunks)apps/cli/src/commands/index.ts(1 hunks)apps/cli/src/commands/workspace.tsx(1 hunks)apps/cli/src/components/Table.tsx(1 hunks)apps/cli/src/lib/db.ts(1 hunks)apps/cli/src/lib/orchestrators/__tests__/change-orchestrator.test.ts(1 hunks)apps/cli/src/lib/orchestrators/__tests__/environment-orchestrator.test.ts(1 hunks)apps/cli/src/lib/orchestrators/__tests__/process-orchestrator.test.ts(1 hunks)apps/cli/src/lib/orchestrators/__tests__/workspace-orchestrator.test.ts(1 hunks)apps/cli/src/lib/orchestrators/change-orchestrator.ts(1 hunks)apps/cli/src/lib/orchestrators/environment-orchestrator.ts(1 hunks)apps/cli/src/lib/orchestrators/index.ts(1 hunks)apps/cli/src/lib/orchestrators/process-orchestrator.ts(1 hunks)apps/cli/src/lib/orchestrators/workspace-orchestrator.ts(1 hunks)apps/cli/src/lib/storage/__tests__/lowdb-adapter.test.ts(1 hunks)apps/cli/src/lib/storage/adapter.ts(1 hunks)apps/cli/src/lib/storage/index.ts(1 hunks)apps/cli/src/lib/storage/lowdb-adapter.ts(1 hunks)apps/cli/src/lib/storage/types.ts(1 hunks)apps/cli/src/server.ts(1 hunks)package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
apps/cli/src/commands/agent.tsx (4)
apps/cli/src/commands/index.ts (6)
AgentList(9-9)AgentGet(10-10)AgentCreate(11-11)AgentStop(12-12)AgentStopAll(13-13)AgentDelete(14-14)apps/cli/src/lib/db.ts (1)
getDb(12-27)apps/cli/src/components/Table.tsx (1)
Table(8-28)apps/cli/src/lib/orchestrators/process-orchestrator.ts (1)
stopAll(99-119)
apps/cli/src/cli.tsx (5)
apps/cli/src/commands/env.tsx (4)
EnvList(11-57)EnvGet(64-101)EnvCreate(107-144)EnvDelete(151-195)apps/cli/src/commands/index.ts (17)
EnvList(1-1)EnvGet(1-1)EnvCreate(1-1)EnvDelete(1-1)WorkspaceList(3-3)WorkspaceGet(4-4)WorkspaceCreate(5-5)WorkspaceDelete(6-6)AgentList(9-9)AgentGet(10-10)AgentCreate(11-11)AgentStop(12-12)AgentStopAll(13-13)AgentDelete(14-14)ChangeList(16-16)ChangeCreate(16-16)ChangeDelete(16-16)apps/cli/src/commands/workspace.tsx (4)
WorkspaceList(13-68)WorkspaceGet(75-112)WorkspaceCreate(121-163)WorkspaceDelete(170-212)apps/cli/src/commands/agent.tsx (6)
AgentList(14-74)AgentGet(81-127)AgentCreate(136-188)AgentStop(195-236)AgentStopAll(242-278)AgentDelete(285-327)apps/cli/src/commands/change.tsx (3)
ChangeList(12-68)ChangeCreate(76-124)ChangeDelete(131-173)
apps/cli/src/commands/workspace.tsx (4)
apps/cli/src/commands/index.ts (4)
WorkspaceList(3-3)WorkspaceGet(4-4)WorkspaceCreate(5-5)WorkspaceDelete(6-6)apps/cli/src/lib/db.ts (1)
getDb(12-27)apps/cli/src/lib/orchestrators/workspace-orchestrator.ts (1)
WorkspaceOrchestrator(14-115)apps/cli/src/components/Table.tsx (1)
Table(8-28)
apps/cli/src/commands/change.tsx (2)
apps/cli/src/lib/db.ts (1)
getDb(12-27)apps/cli/src/components/Table.tsx (1)
Table(8-28)
apps/cli/src/commands/env.tsx (3)
apps/cli/src/lib/db.ts (1)
getDb(12-27)apps/cli/src/lib/orchestrators/environment-orchestrator.ts (1)
EnvironmentOrchestrator(12-110)apps/cli/src/components/Table.tsx (1)
Table(8-28)
🪛 GitHub Actions: CI
package.json
[warning] 1-1: Diagnostics indicate 182 errors, 185 warnings from lint checks; consider increasing max diagnostics or addressing root causes in codebase.
apps/cli/src/commands/agent.tsx
[warning] 15-15: lint/suspicious/noExplicitAny: Unexpected any. Consider providing a specific type for state.
apps/cli/src/cli.tsx
[warning] 4-4: lint/correctness/noUnusedImports: Unused import 'React'.
[warning] 29-33: Formatter would have printed changes (formatting suggestions) for code in this file.
apps/cli/src/commands/change.tsx
[warning] 13-13: lint/suspicious/noExplicitAny: Unexpected any. Consider providing a specific type for state.
apps/cli/package.json
[warning] 1-1: Diagnostics indicate 182 errors, 185 warnings from lint checks; consider increasing max diagnostics or addressing root causes in codebase.
🔇 Additional comments (23)
apps/cli/src/lib/storage/types.ts (1)
9-9: LGTM! Import path normalized to extensionless specifier.The removal of the
.jsextension aligns with the project-wide migration to extensionless TypeScript module resolution.apps/cli/package.json (3)
11-11: Bun-based bundling aligns with PR objectives.The switch to Bun bundling is consistent with the CLI modernization described in the PR summary.
25-28: New dependencies align with Commander-based CLI architecture.The addition of
commander,ink-select-input, andink-tablesupports the multi-command CLI structure described in the PR summary.
38-38: Verify intentional downgrade of bun-types.The
bun-typesversion was changed from^1.3.2to^1.3.1, which is a downgrade. This could be intentional for compatibility reasons, but please confirm this was not an accidental change.apps/cli/src/components/Table.tsx (1)
13-13: Non-null assertion is safe here.The use of
data[0]!is protected by the length check on line 9, making the non-null assertion safe.apps/cli/src/lib/orchestrators/process-orchestrator.ts (1)
9-11: LGTM! Import paths normalized to extensionless specifiers.The removal of
.jsextensions aligns with the project-wide migration to extensionless TypeScript module resolution.apps/cli/src/lib/storage/lowdb-adapter.ts (1)
5-11: LGTM! Import paths normalized to extensionless specifiers.The removal of
.jsextensions from local module imports aligns with the project-wide migration to extensionless TypeScript module resolution.apps/cli/src/lib/storage/adapter.ts (1)
1-1: LGTM! Import path normalized to extensionless specifier.The removal of the
.jsextension aligns with the project-wide migration to extensionless TypeScript module resolution.apps/cli/src/lib/orchestrators/__tests__/process-orchestrator.test.ts (1)
5-11: LGTM! Test imports normalized to extensionless specifiers.The removal of
.jsextensions aligns with the project-wide migration to extensionless TypeScript module resolution.apps/cli/src/lib/orchestrators/workspace-orchestrator.ts (1)
7-8: LGTM: Consistent import path normalization.The removal of
.jsextensions aligns with the project-wide migration to Bun-based module resolution.apps/cli/src/lib/orchestrators/__tests__/workspace-orchestrator.test.ts (1)
5-10: LGTM: Test imports updated consistently.Import paths normalized to extensionless specifiers, matching the broader module resolution changes across tests and orchestrators.
apps/cli/src/lib/storage/__tests__/lowdb-adapter.test.ts (1)
6-8: LGTM: Storage test imports normalized.Import paths updated to extensionless specifiers, consistent with the storage layer module resolution changes.
apps/cli/src/server.ts (1)
13-13: Runtime switched to Bun.The watch server now spawns the CLI using Bun instead of Node, aligning with the PR's migration to Bun-based bundling. Ensure that the build output in
dist/cli.jsis compatible with Bun's runtime and that Bun is available in all development environments.apps/cli/src/lib/orchestrators/environment-orchestrator.ts (1)
5-6: LGTM: Import paths normalized.Extensionless import specifiers applied consistently with other orchestrator modules.
apps/cli/src/lib/orchestrators/__tests__/change-orchestrator.test.ts (1)
5-9: LGTM: Test imports updated.Import paths normalized to extensionless specifiers, matching the pattern across all orchestrator tests.
apps/cli/src/lib/orchestrators/change-orchestrator.ts (1)
5-6: LGTM: Imports normalized.Extensionless import paths applied consistently with the change orchestrator's dependencies.
apps/cli/src/lib/storage/index.ts (1)
1-5: LGTM: Storage exports normalized.All re-export paths updated to extensionless specifiers, maintaining the same public API while aligning with module resolution changes.
apps/cli/src/lib/orchestrators/__tests__/environment-orchestrator.test.ts (1)
5-10: LGTM!The import path normalization to extensionless module specifiers is consistent with the broader PR changes and TypeScript/Bun conventions.
apps/cli/src/lib/orchestrators/index.ts (1)
1-4: LGTM!Export path normalization aligns with the extensionless module resolution pattern used throughout the PR.
apps/cli/src/commands/index.ts (1)
1-16: LGTM!The centralized command export structure provides a clean public API surface for the CLI module system.
apps/cli/src/commands/agent.tsx (3)
1-7: LGTM!The imports are well-organized and appropriate for the agent management CLI commands.
38-39: Consistent error handling looks good.The error handling pattern is consistent across all components, properly checking
instanceof Errorbefore accessing the message property, with a fallback to "Unknown error". This provides a good user experience for CLI error messages.Also applies to: 102-103, 163-164, 207-208, 254-255, 297-298
47-47: Callback dependency pattern is correct for one-time CLI execution.The review comment raises a valid dependency array principle, but it doesn't apply here. All parent components pass
onComplete={() => process.exit(0)}as inline functions (see cli.tsx lines 127, 135, 169-173, 183, 190, 198). These components execute once, callonComplete?.()in the finally block, which terminates the process. There's no subsequent re-rendering or effect re-runs. IncludingonCompletein the dependency array is the correct React pattern—it ensures the effect captures the dependency. This is idiomatic for CLI applications that run single operations and exit.Likely an incorrect or invalid review comment.
| import { render } from "ink"; | ||
| import meow from "meow"; | ||
| import { Command } from "commander"; | ||
| import React from "react"; |
There was a problem hiding this comment.
Remove unused React import.
The React import is not needed as the JSX transform handles it automatically.
Apply this diff:
-import React from "react";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import React from "react"; |
🧰 Tools
🪛 GitHub Actions: CI
[warning] 4-4: lint/correctness/noUnusedImports: Unused import 'React'.
🤖 Prompt for AI Agents
In apps/cli/src/cli.tsx at line 4, remove the unused import "import React from
\"react\";" since the project uses the automatic JSX transform; delete that
import line and ensure no other references to React remain in the file.
| const formatted = processes.map((p) => ({ | ||
| ...p, | ||
| createdAt: new Date(p.createdAt).toLocaleString(), | ||
| updatedAt: new Date(p.updatedAt).toLocaleString(), | ||
| endedAt: p.endedAt ? new Date(p.endedAt).toLocaleString() : "—", | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicate date formatting logic into a helper function.
The same date formatting logic is repeated across three components (lines 30-35, 94-99, 155-160), violating the DRY principle. Consider extracting this into a reusable helper function.
Create a new utility file apps/cli/src/lib/format-utils.ts:
export function formatProcessDates<T extends { createdAt: Date | string; updatedAt: Date | string; endedAt?: Date | string | null }>(
process: T
): T & { createdAt: string; updatedAt: string; endedAt: string } {
return {
...process,
createdAt: new Date(process.createdAt).toLocaleString(),
updatedAt: new Date(process.updatedAt).toLocaleString(),
endedAt: process.endedAt ? new Date(process.endedAt).toLocaleString() : "—",
};
}Then import and use it in each component:
+import { formatProcessDates } from "../lib/format-utils";
- const formatted = processes.map((p) => ({
- ...p,
- createdAt: new Date(p.createdAt).toLocaleString(),
- updatedAt: new Date(p.updatedAt).toLocaleString(),
- endedAt: p.endedAt ? new Date(p.endedAt).toLocaleString() : "—",
- }));
+ const formatted = processes.map(formatProcessDates);Also applies to: 94-99, 155-160
🤖 Prompt for AI Agents
In apps/cli/src/commands/agent.tsx around lines 30-35 (and similarly at 94-99
and 155-160), duplicate date-formatting logic is used to convert
createdAt/updatedAt/endedAt to locale strings; extract this into a reusable
helper by adding apps/cli/src/lib/format-utils.ts with a generic
formatProcessDates function that accepts a process object and returns the same
object with createdAt and updatedAt as toLocaleString() and endedAt as
toLocaleString() or "—", then import that function into this file (and the other
two locations) and replace the inline mapping with a call to formatProcessDates
for each process item.
| : "No agents/processes found"; | ||
| return ( | ||
| <Text dimColor> | ||
| {message}. Create one with: superset agent create <workspace-id> <type> |
There was a problem hiding this comment.
Use JSX syntax instead of HTML entity.
The HTML entity < should be replaced with the actual < character in JSX. React handles escaping automatically.
Apply this diff:
- {message}. Create one with: superset agent create <workspace-id> <type>
+ {message}. Create one with: superset agent create <workspace-id> <type>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {message}. Create one with: superset agent create <workspace-id> <type> | |
| {message}. Create one with: superset agent create <workspace-id> <type> |
🤖 Prompt for AI Agents
In apps/cli/src/commands/agent.tsx around line 63, the help/message text
currently uses HTML entities "<" and ">"; replace them with the literal
characters "<" and ">" so the JSX renders "<workspace-id>" and "<type>"
correctly (React will escape them automatically). Update the string to: Create
one with: superset agent create <workspace-id> <type> (using the literal < and >
characters in the JSX).
| export function ChangeList({ workspaceId, onComplete }: ChangeListProps) { | ||
| const [changes, setChanges] = React.useState<any[]>([]); | ||
| const [error, setError] = React.useState<string | null>(null); | ||
| const [loading, setLoading] = React.useState(true); |
There was a problem hiding this comment.
Replace any type with proper Change type.
The pipeline warning flags the use of any for state. Using the proper Change type would improve type safety.
Apply this diff:
+import type { Change } from "../types/change";
export function ChangeList({ workspaceId, onComplete }: ChangeListProps) {
- const [changes, setChanges] = React.useState<any[]>([]);
+ const [changes, setChanges] = React.useState<Change[]>([]);Also update ChangeCreate (line 77) similarly:
- const [change, setChange] = React.useState<any | null>(null);
+ const [change, setChange] = React.useState<Change | null>(null);Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: CI
[warning] 13-13: lint/suspicious/noExplicitAny: Unexpected any. Consider providing a specific type for state.
🤖 Prompt for AI Agents
In apps/cli/src/commands/change.tsx around lines 12 to 15, replace the useState
any[] with the proper Change[] type and change the error state to use the
correct error type; update the state declarations to use the imported or locally
defined Change type (e.g., React.useState<Change[]>([])) and
React.useState<string | null>(null) as needed, and apply the same replacement
for the ChangeCreate state at line 77 so no use of any remains in component
state.
| "dependencies": { | ||
| "react-devtools-core": "^7.0.1" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the necessity and placement of react-devtools-core.
Adding react-devtools-core as a production dependency at the root level is unusual. Typically, dev tools dependencies are:
- Placed in specific workspace packages that use them
- Added as devDependencies rather than dependencies
🏁 Script executed:
#!/bin/bash
# Search for usage of react-devtools-core in the codebase
rg -n "react-devtools-core" --type-add 'code:*.{ts,tsx,js,jsx}' --type code -C3Length of output: 0
I need to perform broader verification to understand whether react-devtools-core is actually used in the codebase.
---
🏁 Script executed:
```shell
#!/bin/bash
# Broader search without type restrictions
echo "=== Searching for react-devtools-core in all files ==="
rg -n "react-devtools-core" -C3
echo ""
echo "=== Searching for react-devtools substring ==="
rg -n "react-devtools" -C3
echo ""
echo "=== Checking package.json for workspace/workspaces config ==="
cat package.json | head -60
Length of output: 7394
I need to verify if any workspace packages use the ink package (which declares react-devtools-core as a peerDependency) or if react-devtools-core is needed at all.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for ink usage in workspace packages
echo "=== Searching for ink imports/usage ==="
rg -n "from.*ink|import.*ink|require.*ink" --type-add 'code:*.{ts,tsx,js,jsx}' --type code
echo ""
echo "=== Checking workspace package.json files for ink dependency ==="
find . -path "*/node_modules" -prune -o -path "*/.git" -prune -o -type f -name "package.json" -print | xargs grep -l "ink" 2>/dev/null
echo ""
echo "=== Checking if any package.json has react-devtools-core ==="
find . -path "*/node_modules" -prune -o -path "*/.git" -prune -o -type f -name "package.json" -print | xargs grep -l "react-devtools-core" 2>/dev/null
Length of output: 1764
Let me verify the workspace package.json files to confirm the final placement issue:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check workspace package.json files
echo "=== apps/cli/package.json ==="
cat apps/cli/package.json | grep -A 20 -B 5 "ink\|react-devtools"
echo ""
echo "=== apps/desktop/package.json ==="
cat apps/desktop/package.json | grep -A 10 -B 5 "ink\|react-devtools"
Length of output: 1437
Move react-devtools-core to apps/cli/package.json as a devDependency.
The dependency is currently placed at the root level in production dependencies to satisfy an optional peerDependency of ink (which apps/cli uses). However, this placement is incorrect:
react-devtools-coreshould reside inapps/cli/package.jsonwhereink@^6.5.0is actually declared- It should be a
devDependency(React debugging is a development tool) - The version mismatch should be resolved: root declares
^7.0.1whileinkexpects^6.1.2
Remove the entry from root package.json and add it to apps/cli/package.json devDependencies with a compatible version.
🤖 Prompt for AI Agents
In package.json around lines 42 to 44, the production dependency
"react-devtools-core": "^7.0.1" is misplaced and version-mismatched for the CLI:
remove this entry from the root dependencies and add "react-devtools-core" as a
devDependency in apps/cli/package.json (where ink@^6.5.0 is declared) using a
version compatible with ink (e.g. ^6.1.2); ensure you run install/update
lockfile after the change so the workspace lockfile remains consistent.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cli/src/lib/orchestrators/__tests__/environment-orchestrator.test.ts (1)
56-60: Async.rejectsassertions must be awaited in all testsThe test file has multiple assertions using
.rejectswithoutawait. According to bun:test documentation, these assertions must be awaited or they won't execute and tests can pass even if errors aren't thrown.Affected locations:
- Lines 56–60 ("get" test):
expect(orchestrator.get("non-existent")).rejects.toThrow(...)- Lines 94–96 ("update" test):
expect(orchestrator.update("non-existent", {})).rejects.toThrow()- Lines 137, 147, 159, 163–164 ("delete" tests): multiple
.rejects.toThrow()callsAdd
awaitbefore eachexpect()call:- expect(orchestrator.get("non-existent")).rejects.toThrow( + await expect(orchestrator.get("non-existent")).rejects.toThrow(
♻️ Duplicate comments (1)
apps/cli/src/cli.tsx (1)
3-25: Remove unusedReactimport to satisfy lint and keep imports minimal.
Reactis not referenced in this file (JSX is used only via Ink components and the modern JSX transform), so the import at Line 4 is unused and flagged by lint. You can safely drop it.-import React from "react";Please re-run your linter/CI after removing this to confirm there are no JSX runtime configuration issues.
🧹 Nitpick comments (1)
apps/cli/src/cli.tsx (1)
27-35: Consider deriving CLI version frompackage.jsonto avoid drift.Hard‑coding
"0.1.0"in.version()can get out of sync with the actual package version. Consider reading the version from your root/package metadata instead so the CLI stays consistent across releases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/cli/src/cli.tsx(1 hunks)apps/cli/src/commands/agent.tsx(1 hunks)apps/cli/src/commands/change.tsx(1 hunks)apps/cli/src/commands/env.tsx(1 hunks)apps/cli/src/commands/workspace.tsx(1 hunks)apps/cli/src/components/Table.tsx(1 hunks)apps/cli/src/lib/orchestrators/__tests__/environment-orchestrator.test.ts(2 hunks)apps/cli/src/lib/orchestrators/change-orchestrator.ts(2 hunks)apps/cli/src/lib/orchestrators/process-orchestrator.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/cli/src/commands/workspace.tsx
- apps/cli/src/lib/orchestrators/process-orchestrator.ts
- apps/cli/src/commands/change.tsx
- apps/cli/src/commands/agent.tsx
- apps/cli/src/commands/env.tsx
- apps/cli/src/components/Table.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/cli/src/cli.tsx (5)
apps/cli/test.js (2)
render(8-8)render(14-14)apps/cli/src/commands/env.tsx (4)
EnvList(11-61)EnvGet(68-105)EnvCreate(111-148)EnvDelete(155-200)apps/cli/src/commands/workspace.tsx (4)
WorkspaceList(13-72)WorkspaceGet(79-116)WorkspaceCreate(125-167)WorkspaceDelete(174-218)apps/cli/src/commands/agent.tsx (6)
AgentList(14-77)AgentGet(84-132)AgentCreate(141-204)AgentStop(211-252)AgentStopAll(258-296)AgentDelete(303-347)apps/cli/src/commands/change.tsx (3)
ChangeList(12-68)ChangeCreate(76-128)ChangeDelete(135-177)
🔇 Additional comments (5)
apps/cli/src/lib/orchestrators/__tests__/environment-orchestrator.test.ts (1)
5-10: Import path normalization to extensionless modules looks consistentThe updated imports without
.jsextensions are consistent with the rest of the codebase and should work well with modern TS/bundler resolution; no issues from this change alone.apps/cli/src/cli.tsx (2)
74-129: Workspace command wiring and type validation look solid.The workspace subcommands (
list,get,create,delete) are wired cleanly into the Ink components, and the runtime validation ofWorkspaceTypeincreatewith clear error messaging +process.exit(1)is a good pattern that matches the help text.
224-263: Change commands and entrypoint wiring look consistent and clear.The
changesubcommands (list,create,delete) follow the same pattern as the other groups and route cleanly into the Ink components, andprogram.parse()at the end ensures the CLI is invoked on execution. No issues from a wiring/structure perspective.apps/cli/src/lib/orchestrators/change-orchestrator.ts (2)
40-45: LGTM: Improved readability with explicit field destructuring.The reformatted destructuring makes each immutable field (id, workspaceId, createdAt) more visible and explicit. The logic remains unchanged—these fields are still correctly filtered out to prevent desynchronization during updates.
5-6: No issues found — the build configuration already supports extensionless imports.The TypeScript configuration chain is properly set up:
apps/cli/tsconfig.jsonextends@superset/typescript/react-library.json, which extendspackages/typescript-config/base.json. The base configuration includes"moduleResolution": "bundler"and references Bun TypeScript documentation, confirming this is the intended Bun setup. Extensionless imports are already present in the file and working correctly. The PR changes are consistent with the existing, properly configured build system.
| // Environment commands | ||
| const env = program.command("env").description("Manage environments"); | ||
|
|
||
| env | ||
| .command("list") | ||
| .description("List all environments") | ||
| .action(() => { | ||
| render(<EnvList onComplete={() => process.exit(0)} />); | ||
| }); | ||
|
|
||
| env | ||
| .command("get") | ||
| .description("Get environment by ID") | ||
| .argument("<id>", "Environment ID") | ||
| .action((id: string) => { | ||
| render(<EnvGet id={id} onComplete={() => process.exit(0)} />); | ||
| }); | ||
|
|
||
| env | ||
| .command("create") | ||
| .description("Create a new environment") | ||
| .action(() => { | ||
| render(<EnvCreate onComplete={() => process.exit(0)} />); | ||
| }); | ||
|
|
||
| env | ||
| .command("delete") | ||
| .description( | ||
| "Delete an environment (cascades to workspaces, processes, changes)", | ||
| ) | ||
| .argument("<id>", "Environment ID") | ||
| .action((id: string) => { | ||
| render(<EnvDelete id={id} onComplete={() => process.exit(0)} />); | ||
| }); |
There was a problem hiding this comment.
Exit code is always 0 even when commands encounter runtime errors.
All env commands render components with onComplete={() => process.exit(0)}, and the corresponding components (EnvList, EnvGet, etc. in apps/cli/src/commands/env.tsx) always call onComplete in a finally block, regardless of whether an error occurred. That means the CLI will typically exit with code 0 even if the orchestrator throws and the user sees an Error: message.
For automation/scripts, it’s usually preferable to return a non‑zero exit code when the command fails. Consider threading a success/error flag through onComplete (or similar) so the CLI can process.exit(1) on failures.
🤖 Prompt for AI Agents
In apps/cli/src/cli.tsx around lines 36 to 69, the env subcommands always call
onComplete with process.exit(0) which forces a zero exit code even when the
underlying component encountered an error; change the pattern so onComplete
accepts a boolean or numeric status (e.g., onComplete(success: boolean)) and
propagate success/failure from the Env* components (they should call
onComplete(false) on error and onComplete(true) on success), then update each
CLI command to call process.exit(0) on success and process.exit(1) on failure
based on that status.
| agent | ||
| .command("create") | ||
| .description("Create a new agent/process") | ||
| .argument("<workspaceId>", "Workspace ID") | ||
| .argument("<type>", `Process type (${Object.values(ProcessType).join(", ")})`) | ||
| .option( | ||
| "--agent-type <agentType>", | ||
| `Agent type (${Object.values(AgentType).join(", ")}) - required if type is 'agent'`, | ||
| ) | ||
| .action( | ||
| (workspaceId: string, type: string, options: { agentType?: string }) => { | ||
| // Validate process type | ||
| if (!Object.values(ProcessType).includes(type as ProcessType)) { | ||
| console.error( | ||
| `Invalid process type: ${type}. Must be one of: ${Object.values(ProcessType).join(", ")}`, | ||
| ); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| // Validate agent type if provided | ||
| let agentType: AgentType | undefined; | ||
| if (options.agentType) { | ||
| if ( | ||
| !Object.values(AgentType).includes(options.agentType as AgentType) | ||
| ) { | ||
| console.error( | ||
| `Invalid agent type: ${options.agentType}. Must be one of: ${Object.values(AgentType).join(", ")}`, | ||
| ); | ||
| process.exit(1); | ||
| } | ||
| agentType = options.agentType as AgentType; | ||
| } | ||
|
|
||
| render( | ||
| <AgentCreate | ||
| workspaceId={workspaceId} | ||
| type={type as ProcessType} | ||
| agentType={agentType} | ||
| onComplete={() => process.exit(0)} | ||
| />, | ||
| ); | ||
| }, | ||
| }, | ||
| ); | ||
| ); |
There was a problem hiding this comment.
Enforce --agent-type when creating an agent process.
The help text says --agent-type is “required if type is 'agent', but the implementation only validates agentTypewhen it is provided and otherwise proceeds withagentTypeasundefined. This can lead to confusing behavior or downstream errors in ProcessOrchestrator.create`.
You can enforce the requirement directly in this action handler, e.g.:
// Validate process type
if (!Object.values(ProcessType).includes(type as ProcessType)) {
console.error(
`Invalid process type: ${type}. Must be one of: ${Object.values(ProcessType).join(", ")}`,
);
process.exit(1);
}
+ // If creating an agent process, require an explicit agent type
+ if (type === ProcessType.AGENT && !options.agentType) {
+ console.error(
+ "Missing required option: --agent-type <agentType> when type is 'agent'.",
+ );
+ process.exit(1);
+ }
+
// Validate agent type if provided
let agentType: AgentType | undefined;(Adjust the ProcessType.AGENT member name to match your actual enum.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| agent | |
| .command("create") | |
| .description("Create a new agent/process") | |
| .argument("<workspaceId>", "Workspace ID") | |
| .argument("<type>", `Process type (${Object.values(ProcessType).join(", ")})`) | |
| .option( | |
| "--agent-type <agentType>", | |
| `Agent type (${Object.values(AgentType).join(", ")}) - required if type is 'agent'`, | |
| ) | |
| .action( | |
| (workspaceId: string, type: string, options: { agentType?: string }) => { | |
| // Validate process type | |
| if (!Object.values(ProcessType).includes(type as ProcessType)) { | |
| console.error( | |
| `Invalid process type: ${type}. Must be one of: ${Object.values(ProcessType).join(", ")}`, | |
| ); | |
| process.exit(1); | |
| } | |
| // Validate agent type if provided | |
| let agentType: AgentType | undefined; | |
| if (options.agentType) { | |
| if ( | |
| !Object.values(AgentType).includes(options.agentType as AgentType) | |
| ) { | |
| console.error( | |
| `Invalid agent type: ${options.agentType}. Must be one of: ${Object.values(AgentType).join(", ")}`, | |
| ); | |
| process.exit(1); | |
| } | |
| agentType = options.agentType as AgentType; | |
| } | |
| render( | |
| <AgentCreate | |
| workspaceId={workspaceId} | |
| type={type as ProcessType} | |
| agentType={agentType} | |
| onComplete={() => process.exit(0)} | |
| />, | |
| ); | |
| }, | |
| }, | |
| ); | |
| ); | |
| agent | |
| .command("create") | |
| .description("Create a new agent/process") | |
| .argument("<workspaceId>", "Workspace ID") | |
| .argument("<type>", `Process type (${Object.values(ProcessType).join(", ")})`) | |
| .option( | |
| "--agent-type <agentType>", | |
| `Agent type (${Object.values(AgentType).join(", ")}) - required if type is 'agent'`, | |
| ) | |
| .action( | |
| (workspaceId: string, type: string, options: { agentType?: string }) => { | |
| // Validate process type | |
| if (!Object.values(ProcessType).includes(type as ProcessType)) { | |
| console.error( | |
| `Invalid process type: ${type}. Must be one of: ${Object.values(ProcessType).join(", ")}`, | |
| ); | |
| process.exit(1); | |
| } | |
| // If creating an agent process, require an explicit agent type | |
| if (type === ProcessType.AGENT && !options.agentType) { | |
| console.error( | |
| "Missing required option: --agent-type <agentType> when type is 'agent'.", | |
| ); | |
| process.exit(1); | |
| } | |
| // Validate agent type if provided | |
| let agentType: AgentType | undefined; | |
| if (options.agentType) { | |
| if ( | |
| !Object.values(AgentType).includes(options.agentType as AgentType) | |
| ) { | |
| console.error( | |
| `Invalid agent type: ${options.agentType}. Must be one of: ${Object.values(AgentType).join(", ")}`, | |
| ); | |
| process.exit(1); | |
| } | |
| agentType = options.agentType as AgentType; | |
| } | |
| render( | |
| <AgentCreate | |
| workspaceId={workspaceId} | |
| type={type as ProcessType} | |
| agentType={agentType} | |
| onComplete={() => process.exit(0)} | |
| />, | |
| ); | |
| }, | |
| ); |
🤖 Prompt for AI Agents
In apps/cli/src/cli.tsx around lines 157 to 199, the action handler does not
enforce that --agent-type is provided when the selected process type is the
agent variant; update the handler to check if type === ProcessType.AGENT (or the
correct enum member) and if so, ensure options.agentType is present — if
missing, print a clear error listing valid AgentType values and exit(1); keep
the existing validation for provided agentType and then set agentType
accordingly before calling render.
1 participant
Summary by CodeRabbit
New Features
Chores