Fix PTY terminal read and shell session ergonomics#649
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughThe PR adds PTY-scoped terminal read support through the CLI, IPC, and PTY service layers, updates shell session creation output and spawn behavior, and changes the chat terminal drawer to deduplicate concurrent or restored tabs by session and PTY identifiers. ChangesPTY session routing and terminal UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/desktop/src/main/services/pty/ptyService.ts`:
- Around line 3726-3730: `launchEnv` used by `ptyService` for `startupCommand`
sessions may be missing the full executable PATH because clean shells skip user
rc/profile setup. Review the shell launch flow around `resolveShellCandidates`
and the `startupCommand`/`directCommand` path so that the environment passed
into the spawned PTY already contains a complete PATH (including user-managed
tool shims like nvm/asdf) before execution, rather than relying on shell
initialization to populate it.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c6eb569-1643-44e6-aa68-bc8f8007d571
⛔ Files ignored due to path filters (1)
docs/features/computer-use/app-control.mdis excluded by!docs/**
📒 Files selected for processing (8)
apps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/renderer/components/chat/ChatTerminalDrawer.test.tsxapps/desktop/src/renderer/components/chat/ChatTerminalDrawer.tsxapps/desktop/src/shared/types/sessions.ts
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
arul28
changed the title
|
@codex review |
![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: f321aa624a
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
ade terminal read --pty <pty-id>.Bug Fixes
Greptile Summary
This PR adds PTY-id-based terminal read lookup (
--pty <pty-id>in the CLI andptyIdin the IPC contract), fixes duplicate terminal tabs from rapid "New terminal" clicks and from restore/reveal re-entry, and improves shell session ergonomics by emitting a suggestedade terminal readcommand aftershell start.resolveTerminalIdnow acceptsptyIddirectly and also falls back to a PTY lookup whenterminalIddoesn't match a known session, making--ptyand--terminal <ptyId>both work from the CLI.createTabFlightRefserialises concurrentcreateTabcalls and tab IDs are now derived fromcreated.sessionId, so backend-returned duplicates are caught both at flight-guard time and inside thesetTabsupdater.resolveShellCandidatesgains acleanflag (skips startup files for command-backed shells),withResolvedCliLaunchPathaugments PATH with interactive-shell and known-CLI directories, andisNodeModulesBinPathnow excludes the user's yarn global bin so it isn't demoted to last resort.Confidence Score: 4/5
Safe to merge after addressing the stale-flight race in ChatTerminalDrawer; all other changes are additive and well-tested.
The
createTabFlightRefis not reset whenchatSessionIdchanges, which means a terminal create that's in-flight for the old chat session can resolve after the user has switched to a new session, append the old session's tab into the new session's drawer, and prevent the new session from auto-creating its own terminal. Every other change — the ptyId IPC path, the CLI formatter, the shell-env helpers, and the deduplication logic itself — is solid and covered by tests.apps/desktop/src/renderer/components/chat/ChatTerminalDrawer.tsx — the chatSessionId-change cleanup effect should reset createTabFlightRef.current to null.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant User participant Drawer as ChatTerminalDrawer participant IPC as registerIpc participant PtyService participant SessionService User->>Drawer: click "New terminal" Drawer->>Drawer: createTabFlightRef set → guard active Drawer->>IPC: "window.ade.pty.create({ chatSessionId, toolType: "shell" })" IPC->>PtyService: create(args) PtyService-->>IPC: "{ sessionId, ptyId, pid }" IPC-->>Drawer: created Drawer->>Drawer: "tabId = chat-term-{sessionId}" Drawer->>Drawer: setTabs (dedup by sessionId/ptyId) Drawer->>Drawer: "createTabFlightRef = null" User->>Drawer: "ade terminal read --pty <pty-id>" Drawer->>IPC: "terminal.read({ ptyId })" IPC->>PtyService: "readTerminal({ ptyId })" PtyService->>PtyService: resolveTerminalId → ptys.get(ptyId)?.sessionId PtyService->>SessionService: get(sessionId) SessionService-->>PtyService: session PtyService-->>IPC: "{ terminalId, data }" IPC-->>Drawer: result%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant User participant Drawer as ChatTerminalDrawer participant IPC as registerIpc participant PtyService participant SessionService User->>Drawer: click "New terminal" Drawer->>Drawer: createTabFlightRef set → guard active Drawer->>IPC: "window.ade.pty.create({ chatSessionId, toolType: "shell" })" IPC->>PtyService: create(args) PtyService-->>IPC: "{ sessionId, ptyId, pid }" IPC-->>Drawer: created Drawer->>Drawer: "tabId = chat-term-{sessionId}" Drawer->>Drawer: setTabs (dedup by sessionId/ptyId) Drawer->>Drawer: "createTabFlightRef = null" User->>Drawer: "ade terminal read --pty <pty-id>" Drawer->>IPC: "terminal.read({ ptyId })" IPC->>PtyService: "readTerminal({ ptyId })" PtyService->>PtyService: resolveTerminalId → ptys.get(ptyId)?.sessionId PtyService->>SessionService: get(sessionId) SessionService-->>PtyService: session PtyService-->>IPC: "{ terminalId, data }" IPC-->>Drawer: resultComments Outside Diff (1)
apps/desktop/src/renderer/components/chat/ChatTerminalDrawer.tsx, line 170-179 (link)When
chatSessionIdchanges, this cleanup effect resets most refs and clearstabs, but does not resetcreateTabFlightRef.current. If awindow.ade.pty.create(...)call is in-flight for Session A when the user switches to Session B, the flight resolves after the switch, appending the Session-A terminal entry into Session B's now-emptytabsviasetTabs. Becausetabs.length > 0, the auto-create guard for Session B (if (!autoCreateOnOpen || revealActive || tabs.length > 0)) short-circuits and Session B never gets its own terminal — it just shows Session A's PTY. AddingcreateTabFlightRef.current = nullhere would cancel the in-progress guard and let the old flight'ssetTabscall be ignored (or at least let Session B detect stale state).Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "Preserve interactive PATH for clean PTY ..." | Re-trigger Greptile