157 lines
7.4 KiB
Markdown
157 lines
7.4 KiB
Markdown
# Code Review
|
|
|
|
## Overall Impression
|
|
|
|
This is a well-scoped project with a clear mental model. The module boundaries are sensible, the dependency footprint is minimal, and the functional (no classes) approach fits the problem well. The codebase reads quickly, which is valuable.
|
|
|
|
That said, there are structural patterns that will become friction points as you add commands or change behavior. Most of the feedback here is about **extracting repeated patterns** and **reducing the surface area of cli.ts**.
|
|
|
|
---
|
|
|
|
## 1. cli.ts is carrying too much weight
|
|
|
|
At ~820 lines, `cli.ts` is the entry point, the command registry, and the implementation of every command handler. That's three jobs. Each command handler is 20-60 lines of real logic — session lookup, spinner management, error handling, VM orchestration — all inline.
|
|
|
|
Right now there are 12+ commands. Adding a 13th means appending more logic to a file that's already the longest in the project. There's no natural place to put "command-adjacent" helpers either, so things like `saveChanges`, `branchFromPrompt`, and `fallbackBranchName` just float in the file above the command they serve.
|
|
|
|
Consider a `commands/` directory where each command (or logical group) is its own file exporting an action function. `cli.ts` becomes just the Commander wiring — argument definitions, descriptions, and `.action(handler)` calls. This makes each command independently readable and testable.
|
|
|
|
---
|
|
|
|
## 2. Repeated session-lookup boilerplate
|
|
|
|
This exact pattern appears in **8 commands** (open, review, shell, save, diff, show, log, dir):
|
|
|
|
```typescript
|
|
const root = await git.repoRoot()
|
|
const session = await state.getSession(root, branch)
|
|
if (!session) {
|
|
console.error(`✖ No session found for branch "${branch}".`)
|
|
process.exit(1)
|
|
}
|
|
```
|
|
|
|
A helper like `requireSession(branch): Promise<{ root: string; session: Session }>` would cut four lines per command and give you a single place to improve the error message later (e.g., suggesting `sandlot list` to see available sessions).
|
|
|
|
---
|
|
|
|
## 3. Duplicated "pipe through temp file into container" pattern
|
|
|
|
Both `saveChanges` (line 44-51 of cli.ts) and the merge conflict resolver (line 447-455) follow the same flow:
|
|
|
|
1. Write content to a temp file under `~/.sandlot/`
|
|
2. `cat` it into `claude -p` inside the container
|
|
3. Capture output
|
|
4. Delete the temp file
|
|
|
|
This should be a single helper in `vm.ts`, something like:
|
|
|
|
```typescript
|
|
export async function claudePipe(input: string, prompt: string): Promise<string>
|
|
```
|
|
|
|
The current approach is fragile — each call site has to remember to clean up the temp file, choose a temp path that won't collide, and handle errors. Centralizing this also makes it easier to switch to stdin piping later.
|
|
|
|
---
|
|
|
|
## 4. API key parsing is duplicated
|
|
|
|
`branchFromPrompt()` in cli.ts (lines 87-92) and `create()` in vm.ts (lines 83-88) both independently read `~/.env` and parse `ANTHROPIC_API_KEY` with the same regex. If you ever change the env file location or key format, you need to find both.
|
|
|
|
Extract this to a shared utility — maybe in a `env.ts` or even just a function in `config.ts`, which is currently underutilized.
|
|
|
|
---
|
|
|
|
## 5. config.ts is dead code
|
|
|
|
`config.ts` defines a config schema and loader, but nothing imports it. `vm.ts` hardcodes `-m 4G` and `ubuntu:24.04`. This is confusing for someone reading the codebase — it looks like configuration is supported but it silently isn't.
|
|
|
|
Either wire it in or remove it. Dead code that looks intentional is worse than no code at all, because it misleads people about how the system works.
|
|
|
|
---
|
|
|
|
## 6. Pager logic is duplicated
|
|
|
|
The `diff` and `show` commands both have identical pager-fallback logic (lines 551-560 and 590-599):
|
|
|
|
```typescript
|
|
const lines = output.split("\n").length
|
|
const termHeight = process.stdout.rows || 24
|
|
if (lines > termHeight) {
|
|
const pager = Bun.spawn(["less", "-R"], { ... })
|
|
// ...
|
|
}
|
|
```
|
|
|
|
This is a natural utility function: `pipeToPagedOutput(content: string)`.
|
|
|
|
---
|
|
|
|
## 7. ANSI codes are scattered and ad-hoc
|
|
|
|
The `list` command defines its own color constants inline (lines 255-261). The spinner uses raw escape codes. `markdown.ts` uses them too. The error/success icons (`✖`, `✔`, `◆`) are hardcoded at each call site.
|
|
|
|
A small `fmt.ts` or extending `spinner.ts` into a more general terminal output module would centralize this. You don't need a full library — just a handful of named helpers like `fmt.dim()`, `fmt.green()`, `fmt.success()`, `fmt.error()`. This also makes it trivial to add `--no-color` support later.
|
|
|
|
---
|
|
|
|
## 8. `git.branchExists()` has a surprising side effect
|
|
|
|
This function is named like a pure query, but it calls `git fetch origin` on every invocation (line 29). That means every `sandlot new` makes a network round-trip, which could be slow or fail on spotty connections. The caller (`createWorktree`) has no indication this will happen.
|
|
|
|
At minimum, document this. Ideally, separate the fetch from the check, or accept a `fetch?: boolean` option.
|
|
|
|
---
|
|
|
|
## 9. `vm.create()` is doing too many things
|
|
|
|
This ~90-line function handles: container creation, apt package installation, host symlink setup, Bun installation, Claude Code installation, git config, API key provisioning, activity hook installation, and settings file creation.
|
|
|
|
Breaking this into named phases (even just private helper functions within `vm.ts`) would make it easier to debug provisioning failures and to modify individual steps. Something like:
|
|
|
|
```
|
|
createContainer() → installPackages() → installTooling() → configureEnvironment()
|
|
```
|
|
|
|
---
|
|
|
|
## 10. State file has no concurrency protection
|
|
|
|
`state.ts` does load → modify → save with no locking. If a user runs `sandlot new foo` and `sandlot close bar` at the same time, one write can clobber the other. For a CLI tool this is unlikely but not impossible — especially since `sandlot new` is long-running (it starts a container, launches Claude, and then saves on exit).
|
|
|
|
A simple approach: use an atomic write pattern (write to `.state.json.tmp`, then rename). That at least prevents partial writes. Full advisory locking is probably overkill.
|
|
|
|
---
|
|
|
|
## 11. Inconsistent error presentation
|
|
|
|
Comparing error output across commands:
|
|
|
|
- `new`: `✖ Branch name or prompt is required.`
|
|
- `show`: `No session found for branch "${branch}".` (no icon)
|
|
- `log`: `✖ git log failed` (lowercase, terse)
|
|
- `vm start`: `✖ ${(err as Error).message ?? err}` (raw error message)
|
|
|
|
A consistent `die(message)` or `fatal(message)` function that always formats the same way would clean this up.
|
|
|
|
---
|
|
|
|
## 12. The fish completions string will rot
|
|
|
|
The ~55-line hardcoded fish completions block (lines 746-801) needs to be manually updated whenever a command, option, or subcommand changes. It's easy to forget.
|
|
|
|
You could generate this from Commander's own command tree at runtime — Commander exposes the registered commands and options. This way completions are always in sync.
|
|
|
|
---
|
|
|
|
## Summary of Suggested Extractions
|
|
|
|
| New module | What moves there |
|
|
|---|---|
|
|
| `commands/*.ts` | Individual command handlers out of cli.ts |
|
|
| `fmt.ts` | ANSI helpers, `die()`, `success()`, pager |
|
|
| `env.ts` (or extend `config.ts`) | API key reading, env file parsing |
|
|
| `vm.ts` internal helpers | Break `create()` into phases; add `claudePipe()` |
|
|
|
|
The architecture is sound. These are refinements to make the codebase match the quality of the design. The core module split (`git` / `vm` / `state`) is right — it's the glue layer in `cli.ts` that needs the most attention.
|