fix(run-tab): surface PTY startup failures and tighten run UI - #222
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub. |
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe changes introduce startup-failure diagnostics for PTY-backed processes, a shell-exec fallback for failed direct command spawns, and terminal integration in the Run page. On the backend: processService captures working directory and errors to transcript files; ptyService attempts shell-fallback execution with POSIX quoting when direct spawns fail. On the frontend: ChatTerminalDrawer gains creation-request nonces and customizable empty states; CommandCard and RunPage add runtime terminal access; RunPage refactors shell-session handling to use the drawer with pending-launch tracking. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
🚥 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. Review rate limit: 0/1 reviews remaining, refill in 8 minutes and 46 seconds.Comment |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Write PTY startup errors into the run transcript so failed commands are visible - Improve CommandCard, RunPage, and ChatTerminalDrawer behavior for run-tab fixes - Add tests covering PTY failure paths and advanced drawer behavior Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dcaa110 to
a927190Compare…ner active Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
arul28
commented
May 1, 2026
@codex review |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/main/services/processes/processService.test.ts (1)
1071-1078: ⚡ Quick winAssert the working-directory line to fully lock the new failure contract.
This test already validates command + error text; add one assertion for the emitted cwd line so the new cwd propagation behavior stays protected.
🔍 Suggested assertion
expect(tail).toContain("failed to start"); expect(tail).toContain("./missing-script.sh dev"); + expect(tail).toContain(`[ADE] Working directory: ${tmpDir}`); expect(tail).toContain("spawn ./missing-script.sh ENOENT");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/processes/processService.test.ts` around lines 1071 - 1078, Add an assertion to the existing test block that verifies the working-directory line is present in the emitted log tail to lock the new failure contract; specifically, after calling service.getLogTail(...) (the tail variable) add an expectation that the tail contains the emitted cwd/working-directory line produced by the process failure (so the test alongside checking "failed to start", the command string, and the spawn ENOENT message also asserts the working-directory propagation string emitted by the code that logs the cwd).apps/desktop/src/renderer/components/chat/ChatTerminalDrawer.tsx (1)
358-370: 💤 Low valuePotential subscription churn on tab count changes.
The
onExiteffect re-runs on everytabs.lengthchange, which means unsubscribing and resubscribing with each tab add/remove. During this brief window, an exit event could be missed.Consider using a stable dependency (e.g., a boolean derived from
tabs.length > 0) or moving the guard inside the subscription callback:♻️ Suggested approach
useEffect(() => { - if (tabs.length === 0) return undefined; const ptyBridge = window.ade?.pty; if (!ptyBridge?.onExit) return undefined; const unsubscribe = ptyBridge.onExit((ev: PtyExitEvent) => { setTabs((prev) => prev.map((tab) => ( tab.ptyId === ev.ptyId ? { ...tab, exited: true } : tab ))); }); return unsubscribe; -}, [tabs.length]);+}, []);The
setTabscallback already handles the case whereprevis empty (it just returns an empty array), so guarding inside the effect isn't strictly necessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/chat/ChatTerminalDrawer.tsx` around lines 358 - 370, The effect currently re-subscribes on every change to tabs.length which risks missing events; update the useEffect in ChatTerminalDrawer.tsx to use a stable dependency (e.g., Boolean(tabs.length) instead of tabs.length) or remove tabs.length from the dependency array and perform the empty-tabs guard inside the subscription callback; specifically adjust the useEffect that references ptyBridge, ptyBridge.onExit, and setTabs (and the PtyExitEvent handler) so subscriptions are not torn down/recreated on each tab add/remove and the setTabs callback continues to handle empty prev state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/main/services/processes/processService.test.ts`:
- Around line 1071-1078: Add an assertion to the existing test block that
verifies the working-directory line is present in the emitted log tail to lock
the new failure contract; specifically, after calling service.getLogTail(...)
(the tail variable) add an expectation that the tail contains the emitted
cwd/working-directory line produced by the process failure (so the test
alongside checking "failed to start", the command string, and the spawn ENOENT
message also asserts the working-directory propagation string emitted by the
code that logs the cwd).
In `@apps/desktop/src/renderer/components/chat/ChatTerminalDrawer.tsx`:
- Around line 358-370: The effect currently re-subscribes on every change to
tabs.length which risks missing events; update the useEffect in
ChatTerminalDrawer.tsx to use a stable dependency (e.g., Boolean(tabs.length)
instead of tabs.length) or remove tabs.length from the dependency array and
perform the empty-tabs guard inside the subscription callback; specifically
adjust the useEffect that references ptyBridge, ptyBridge.onExit, and setTabs
(and the PtyExitEvent handler) so subscriptions are not torn down/recreated on
each tab add/remove and the setTabs callback continues to handle empty prev
state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a6b4d75f-be16-41e0-822a-0f05c4df0165
📒 Files selected for processing (8)
apps/desktop/src/main/services/processes/processService.test.tsapps/desktop/src/main/services/processes/processService.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/renderer/components/chat/ChatTerminalDrawer.tsxapps/desktop/src/renderer/components/run/CommandCard.tsxapps/desktop/src/renderer/components/run/RunPage.advancedDrawer.test.tsxapps/desktop/src/renderer/components/run/RunPage.tsx
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:4616257738
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR surfaces PTY startup failures into the run transcript (with diagnostic details like command, cwd, and error message), adds a shell-exec fallback for non-Windows systems when direct PTY spawn fails, and replaces the inline shell-session list in
RunPagewith the sharedChatTerminalDrawer. A new "Terminal" button onCommandCardlets users open a live terminal for any running process.disposeTabsOnUnmount=truedisposes process-backed PTYs: When a user clicks the "Terminal" button on aCommandCard, the run-command's PTY is added to the drawer's tab list. OnRunPageunmount the cleanup disposes every tab indiscriminately, which kills live run-command processes (e.g.npm run dev) that should persist in the background. Only interactive shell tabs created via "New shell" need cleanup.trackedflag silently changed: The oldhandleLaunchShellusedtracked: false;ChatTerminalDrawer.createTabalways usestracked: true. If this is intentional, a note in the code would help.Confidence Score: 3/5
Not safe to merge as-is — navigating away from RunPage with an open run-command terminal will kill the underlying process.
One P1 defect: disposeTabsOnUnmount=true disposes all drawer tabs on unmount, including PTYs that back live run-command processes. Navigating away from RunPage after clicking Terminal on a running command silently kills it. The rest of the changes are well-structured with good test coverage.
apps/desktop/src/renderer/components/chat/ChatTerminalDrawer.tsx — the disposeTabsOnUnmount cleanup effect and the createTab tracked value both need review.
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant RunPage participant ChatTerminalDrawer participant processService participant ptyService User->>RunPage: Click Run on CommandCard RunPage->>processService: start({ laneId, processId }) processService->>ptyService: create(...) alt PTY spawn succeeds ptyService-->>processService: { ptyId, sessionId } processService-->>RunPage: ProcessRuntime { ptyId, sessionId } RunPage->>ChatTerminalDrawer: revealRequest (ptyId, sessionId) ChatTerminalDrawer->>ChatTerminalDrawer: addTab(ptyId) — now tracked by drawer else PTY spawn fails ptyService->>ptyService: buildDirectCommandShellFallback() ptyService->>ptyService: spawn shell + exec cmd ptyService-->>processService: throws Error processService->>processService: handleStartFailure → write to transcript processService-->>RunPage: throws Error RunPage->>RunPage: setActionError end User->>RunPage: Navigate away (unmount) RunPage->>ChatTerminalDrawer: unmount (disposeTabsOnUnmount=true) ChatTerminalDrawer->>ptyService: dispose(all tabs) — kills process-backed PTYs tooComments Outside Diff (1)
apps/desktop/src/renderer/components/chat/ChatTerminalDrawer.tsx, line 359-364 (link)disposeTabsOnUnmountkills process-backed PTYs alongside interactive shellsWhen a user clicks the "Terminal" button on a
CommandCard,revealRuntimeTerminaladds a tab with the run command'sptyIdto this drawer. WithdisposeTabsOnUnmount=true, all tabs are disposed onRunPageunmount — including those attached to live run-command processes. Disposing a process-backed PTY signals the process manager that the PTY has exited, which marks the process ascrashed. A user who opensnpm run devin the terminal drawer and then navigates away would silently kill the process.Only interactive shell tabs (those created via "New shell") should be disposed on unmount; tabs revealed from existing run-command PTYs should be left alone. One approach: add a
source: "shell" | "process"field toTabEntryso the cleanup can skip process-backed tabs:Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "ship: checkpoint before merging main int..." | Re-trigger Greptile