Make CLI launch spinners actually animate#62
Conversation
Two distinct event-loop blocking issues kept the CLI launch spinners frozen
on their first frame. Both fixed here:
1. runInstallWithSpinner used spawnSync, which blocks Node's event loop for
the entire install. Ora's frame redraw runs on a setInterval, so the
spinner only ever painted its first frame. Switched to async spawn +
Promise so the loop keeps ticking. runInstall and runInstallOrThrow are
now async; both call sites await them.
2. The sandbox-mount setup phase had no spinner at all — just a static
"• sandbox mount → ..." line followed by a multi-second silent pause
while createMount mirrored the project tree. Added a setup spinner that
covers createMount, git config, the in-mount install (paused so the
install spinner takes over the line), config-file writes, and autosync
start, finalizing with .succeed("Sandbox mount ready → ...") just before
the harness child spawns. Wired into the catch/finally so a setup error
doesn't leak a hung spinner. Moved createMount inside the try block so
its initial-mirror failures clean up the spinner correctly.
The setup spinner only animates because @relayfile/local-mount 0.7.0
made createMount async and yields with setImmediate between directory
entries (relayfile #104). Bumped from ^0.6.15 to ^0.7.0 and switched
the call site to `await createMount(...)`. The MountHandle interface is
unchanged, so the rest of the lifecycle (syncBack / startAutoSync /
cleanup) is unaffected.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBump ChangesAsync Install and Mount Setup with Spinners
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb0137f9dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let buffered = ''; | ||
| child.stdout?.setEncoding('utf8'); | ||
| child.stderr?.setEncoding('utf8'); | ||
| child.stdout?.on('data', (chunk: string) => { | ||
| buffered += chunk; | ||
| }); | ||
| child.stderr?.on('data', (chunk: string) => { | ||
| buffered += chunk; | ||
| }); |
There was a problem hiding this comment.
runInstallWithSpinner now appends all child stdout/stderr into a single in-memory string with no size limit. If an install command emits very large logs (for example verbose package-manager output or repeated error traces), this can cause unbounded memory growth and terminate the CLI before it reports the actual install failure. The previous sync path had an explicit buffer cap; this async path should keep a similar limit (or truncate/stream) to avoid regressions on noisy installs.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/cli.ts (1)
1288-1318:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle Ctrl-C during pre-launch mount setup.
Installing this
SIGINTlistener beforecreateMount()suppresses Node’s default Ctrl-C exit, but the handler no-ops untilisSyncingbecomestrue. During the new setup phase there is no child process yet to receive SIGINT, so Ctrl-C is ignored while the mount is being prepared. Either register the listener only after the harness child is spawned, or make the pre-launch path fail the setup spinner, tear down the session dir, and exit 130.🤖 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/cli.ts` around lines 1288 - 1318, The SIGINT handler (sigintHandler) is registered before createMount() but no-ops while isSyncing is false, causing Ctrl-C to be ignored during pre-launch mount setup; fix by either registering process.on('SIGINT', sigintHandler) only after the harness child is spawned (so the child receives SIGINT), or change sigintHandler to handle pre-launch teardown: if isSyncing is false, fail/stop the setup spinner (syncSpinner), remove the session dir (rmSync(sessionRoot, { recursive: true, force: true })), and exit(130); ensure shutdownController.abort() is still used when aborting during sync, and keep references to sigintHandler, syncSpinner, isSyncing, sessionRoot, shutdownController and createMount() to locate the change.
🤖 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.
Outside diff comments:
In `@packages/cli/src/cli.ts`:
- Around line 1288-1318: The SIGINT handler (sigintHandler) is registered before
createMount() but no-ops while isSyncing is false, causing Ctrl-C to be ignored
during pre-launch mount setup; fix by either registering process.on('SIGINT',
sigintHandler) only after the harness child is spawned (so the child receives
SIGINT), or change sigintHandler to handle pre-launch teardown: if isSyncing is
false, fail/stop the setup spinner (syncSpinner), remove the session dir
(rmSync(sessionRoot, { recursive: true, force: true })), and exit(130); ensure
shutdownController.abort() is still used when aborting during sync, and keep
references to sigintHandler, syncSpinner, isSyncing, sessionRoot,
shutdownController and createMount() to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d2f69c1f-44f2-4804-afd0-4520723d9cfb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/cli/package.jsonpackages/cli/src/cli.ts
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Registering any 'SIGINT' listener suppresses Node's default exit-on-SIGINT, so the previous no-op-when-not-syncing handler silently swallowed Ctrl-C during pre-launch mount setup. With sync createMount this was masked by the blocked event loop; now that relayfile 0.7's createMount yields, the swallow is observable — Ctrl-C looks like a hang until setup finishes. Add a `childSpawned` phase flag and split the handler into three branches: pre-launch (fail setup spinner, rm session dir, exit 130), child running (no-op so the child receives Ctrl-C via the TTY pgid), and syncing (existing abort logic, unchanged). `childSpawned` flips inside the spawn Promise executor right before `spawn()` so the window between setup-spinner-succeed and child-spawn still falls into the pre-launch teardown branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
The CLI launch spinners (install + sandbox-mount setup) were frozen on their first frame. This bundles three changes that fix it end-to-end:
runInstallWithSpinner: replacespawnSyncwith asyncspawn+ Promise so ora'ssetIntervalframe timer can fire while the install runs.runInstall/runInstallOrThrowbecomeasync; call sitesawait.createMount+ git config + optional in-mount install + config-file writes + autosync start). Final.succeed("Sandbox mount ready → ...")just before the harness child spawns. Wired into catch/finally so setup errors don't leak a hung spinner.@relayfile/local-mountfrom^0.6.15to^0.7.0andawait createMount(...). 0.7.0 madecreateMountasync and yields withsetImmediatebetween directory entries (relayfile#104) — without this, the setup spinner would still be frozen during the initial mirror.Why all three together
Each fix addresses a distinct event-loop block:
spawnSyncspawncreateMountwalkawaitShipping any subset would leave at least one spinner still frozen.
API impact
MountHandleinterface is unchanged in 0.7.0 —syncBack,startAutoSync,cleanupkeep the same shape. The only consumer-visible change iscreateMountreturningPromise<MountHandle>instead ofMountHandle, handled with oneawait.Test plan
pnpm -r buildcleanpnpm -r test(130 CLI tests + workspace tests, all green)agentworkforce agent <persona>against a non-trivial repo and confirm both spinners animate visibly during install and during the sandbox-mount setup phase.Ctrl-Cduring the existing sync-back phase still aborts cleanly (this PR doesn't change that path).spinner.failexactly once (no duplicate "Aborting" line).🤖 Generated with Claude Code