Conversation
WalkthroughAdds a thread-based terminal subsystem: server PTY adapter and TerminalManager, WebSocket RPC + terminalEvent channel, client XTerm.js terminal UI and shortcuts, expanded contracts/schemas for a multi-method/event terminal API, persistence changes for per-thread terminal state, and tests across server, client, and contracts. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Add interactive terminal with session API, WebSocket methods, event channel, PTY-backed
|
Greptile OverviewGreptile SummaryThis PR adds a full-featured integrated terminal panel to the web UI with resizable drawer, persistent height per thread, and robust server-side PTY management. Major changes:
The architecture follows the project's patterns: server manages PTY sessions, streams events through WebSocket push messages, and web UI renders terminal with client-side state persistence. Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as ThreadTerminalDrawer
participant API as WsNativeApi
participant WS as WebSocket Server
participant TM as TerminalManager
participant PTY as node-pty
UI->>API: terminal.open({threadId, cwd, cols, rows})
API->>WS: WS_METHODS.terminalOpen
WS->>TM: terminalManager.open(input)
TM->>TM: Validate cwd, flush persist queue
TM->>TM: Read history from .logs/terminals/
TM->>PTY: ptyAdapter.spawn({shell, cwd, cols, rows})
PTY-->>TM: ptyProcess with pid
TM->>TM: Subscribe to onData, onExit
TM-->>WS: TerminalSessionSnapshot
WS-->>API: snapshot
API-->>UI: snapshot
UI->>UI: Write history to xterm, focus terminal
loop Terminal I/O
UI->>API: terminal.write({threadId, data})
API->>WS: WS_METHODS.terminalWrite
WS->>TM: terminalManager.write(input)
TM->>PTY: ptyProcess.write(data)
PTY->>TM: onData callback (output)
TM->>TM: Append to history, cap lines, queue persist
TM->>WS: Emit "output" event
WS->>API: Push to WS_CHANNELS.terminalEvent
API->>UI: onEvent callback
UI->>UI: terminal.write(event.data)
end
UI->>API: terminal.resize({threadId, cols, rows})
API->>WS: WS_METHODS.terminalResize
WS->>TM: terminalManager.resize(input)
TM->>PTY: ptyProcess.resize(cols, rows)
PTY->>TM: onExit callback
TM->>TM: Set status="exited", cleanup handles
TM->>WS: Emit "exited" event
WS->>API: Push to WS_CHANNELS.terminalEvent
API->>UI: onEvent callback
UI->>UI: onThreadExited() - close drawer
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
juliusmarminge
changed the title
![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
🤖 Fix all issues with AI agents
In `@apps/server/src/terminalManager.ts`:
- Around line 348-359: The queuePersist method can leave an unhandled rejection
if fs.promises.writeFile fails; update queuePersist (referencing queuePersist,
task, previous, next, persistQueues and the writeFile call) to attach a
rejection handler that logs the error and prevents unhandled rejections — e.g.
wrap the writeFile call in a try/catch inside task or append .catch(...) to next
to handle and log errors (using the module's logger or console.error), while
preserving the existing finally cleanup that removes the entry from
persistQueues.
juliusmarminge
force-pushed
the
feat/terminal
branch
from
63d60fa to
c561a86
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/components/ChatView.tsx`:
- Around line 1210-1211: There's a duplicate BranchToolbar rendering — two
consecutive <BranchToolbar envMode={envMode} onEnvModeChange={setEnvMode}
envLocked={envLocked} /> lines; remove the redundant duplicate so only a single
BranchToolbar instance remains (keep the one that correctly passes envMode,
onEnvModeChange/setEnvMode and envLocked) to prevent the toolbar from rendering
twice.
🧹 Nitpick comments (6)
apps/web/src/components/ThreadTerminalDrawer.tsx (1)
201-215: Duplicate window resize listeners may cause redundant work.There are two
window.addEventListener("resize", ...)calls:
- Lines 201-215: Handles drawer height clamping and syncing
- Line 366: Handles terminal fitting and backend resize notification
Both fire on every window resize, leading to duplicate calls to
clampDrawerHeightandsyncHeight. Consider consolidating resize handling into a single listener or ensuring they don't conflict.♻️ Suggested consolidation
You could merge the resize logic into one handler:
useEffect(() => { const onWindowResize = () => { const clampedHeight = clampDrawerHeight(drawerHeightRef.current); if (clampedHeight !== drawerHeightRef.current) { setDrawerHeight(clampedHeight); } if (!resizeStateRef.current) { syncHeight(clampedHeight); } + // Also fit terminal + const activeTerminal = terminalRef.current; + const activeFitAddon = fitAddonRef.current; + if (activeTerminal && activeFitAddon) { + activeFitAddon.fit(); + void api.terminal + .resize({ + threadId, + cols: activeTerminal.cols, + rows: activeTerminal.rows, + }) + .catch(() => undefined); + } }; window.addEventListener("resize", onWindowResize); return () => { window.removeEventListener("resize", onWindowResize); }; - }, [syncHeight]); + }, [api, syncHeight, threadId]);Then remove the duplicate listener from the terminal setup effect (line 366).
Also applies to: 366-367
apps/web/src/persistenceSchema.ts (2)
72-80: V5 and V6 schemas appear identical.Both
persistedStateV5SchemaandpersistedStateV6Schemahave the same structure - only the version literal differs. If V5 was a development iteration that shipped, this is fine for backward compatibility. Otherwise, V5 could potentially be removed to simplify the schema union.
36-39: Consider aligning schema bounds with UI constraints.The schema allows
terminalHeightmin of 120, butThreadTerminalDrawerusesMIN_DRAWER_HEIGHT = 180. While the schema being more permissive is acceptable for storage flexibility, consider documenting this discrepancy or aligning the values to avoid confusion.apps/server/src/terminalManager.test.ts (1)
103-134: Consider wrapping test body in try/finally for cleanup.In tests like
spawns lazily and reuses running terminal per thread, if an assertion fails beforemanager.dispose(), the manager won't be disposed. While this may not cause issues in isolated tests, using try/finally would ensure consistent cleanup:it("spawns lazily and reuses running terminal per thread", async () => { const { manager, ptyAdapter } = makeManager(); try { const first = await manager.open(openInput()); // ... assertions ... } finally { manager.dispose(); } });This is a minor concern since vitest runs tests in isolation, but it's a good practice for resource-heavy tests.
packages/contracts/src/terminal.test.ts (1)
97-118: Good coverage of discriminated union event types.The
terminalEventSchematests validate both "output" and "exited" event variants. Consider adding tests for other event types ("started", "error", "cleared", "restarted") for complete coverage.apps/server/src/terminalManager.ts (1)
226-232: Consider flushing pending persists on dispose (optional).The
disposemethod clearspersistQueueswithout awaiting pending writes. This could lose in-flight history data on shutdown. If history preservation during graceful shutdown is important, consider adding an optional flush:async dispose(): Promise<void> { for (const session of this.sessions.values()) { this.stopProcess(session); } // Optionally flush pending persists await Promise.all([...this.persistQueues.values()].map(p => p.catch(() => {}))); this.sessions.clear(); this.persistQueues.clear(); }However, the current synchronous approach is acceptable for fast shutdown scenarios.
Co-authored-by: codex <codex@users.noreply.github.com>
| } | ||
| if (chunk.length <= remaining) { | ||
| return { | ||
| next: `${target}${chunk.toString()}`, |
There was a problem hiding this comment.
🟢 Low
src/git.ts:50 Using chunk.toString() directly can corrupt multi-byte UTF-8 characters that span chunk boundaries. Consider using Node's StringDecoder to handle partial characters correctly.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/git.ts around line 50:
Using `chunk.toString()` directly can corrupt multi-byte UTF-8 characters that span chunk boundaries. Consider using Node's `StringDecoder` to handle partial characters correctly.
| } catch (error) { | ||
| if (ptyProcess) { |
There was a problem hiding this comment.
🟢 Low
src/terminalManager.ts:299 Consider calling this.cleanupProcessHandles(session) in the catch block before killing the process to prevent a memory leak when an exception occurs after listeners are attached.
| } catch (error) { | |
| if (ptyProcess) { | |
| } catch (error) { | |
| this.cleanupProcessHandles(session); | |
| if (ptyProcess) { |
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/terminalManager.ts around lines 299-300:
Consider calling `this.cleanupProcessHandles(session)` in the `catch` block before killing the process to prevent a memory leak when an exception occurs after listeners are attached.
|
|
||
| private async readHistory(threadId: string): Promise<string> { | ||
| try { | ||
| const raw = await fs.promises.readFile(this.historyPath(threadId), "utf8"); |
There was a problem hiding this comment.
🟠 High
src/terminalManager.ts:439 Reading entire history files into memory could cause OOM if legacy files are very large. Consider using streaming reads or checking file size first and truncating oversized files before reading.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/terminalManager.ts around line 439:
Reading entire history files into memory could cause OOM if legacy files are very large. Consider using streaming reads or checking file size first and truncating oversized files before reading.
| }); | ||
| } | ||
|
|
||
| dispose(): void { |
There was a problem hiding this comment.
🟢 Low
src/terminalManager.ts:244 dispose() clears threadLocks without awaiting pending operations. If called while close() is suspended (e.g., in flushPersistQueue), a concurrent open() can acquire the lock and create a session whose history is then deleted when close() resumes. Consider awaiting all pending locks before clearing them.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/terminalManager.ts around line 244:
`dispose()` clears `threadLocks` without awaiting pending operations. If called while `close()` is suspended (e.g., in `flushPersistQueue`), a concurrent `open()` can acquire the lock and create a session whose history is then deleted when `close()` resumes. Consider awaiting all pending locks before clearing them.
| } | ||
|
|
||
| private async readHistory(threadId: string): Promise<string> { | ||
| try { |
There was a problem hiding this comment.
🟢 Low
src/terminalManager.ts:438 Consider moving the writeFile call outside the try block (or into its own try/catch). If readFile succeeds but writeFile fails with ENOENT, the current catch block swallows the error and falls through to legacy lookup, silently losing the already-loaded history.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/terminalManager.ts around line 438:
Consider moving the `writeFile` call outside the `try` block (or into its own try/catch). If `readFile` succeeds but `writeFile` fails with `ENOENT`, the current catch block swallows the error and falls through to legacy lookup, silently losing the already-loaded history.
| }); | ||
| child.on("close", (code, signal) => { | ||
| clearTimeout(timeout); | ||
| if (outputTruncated) { |
There was a problem hiding this comment.
🟢 Low
src/git.ts:105 When output is truncated, the function appends a warning to stderr but still returns code: 0. Callers like listGitBranches check only code and will silently parse incomplete data. Consider either returning a non-zero code when truncated, adding a truncated field to the result, or throwing an error.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/git.ts around line 105:
When output is truncated, the function appends a warning to `stderr` but still returns `code: 0`. Callers like `listGitBranches` check only `code` and will silently parse incomplete data. Consider either returning a non-zero code when truncated, adding a `truncated` field to the result, or throwing an error.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/components/ThreadTerminalDrawer.tsx`:
- Around line 263-282: The clear-shortcut handler currently sends a form-feed
via sendClearShortcut (which calls api.terminal.write with "\u000c") but should
call the server-side clear endpoint to reset persisted history; update
sendClearShortcut to call await api.terminal.clear({ threadId }) instead of
api.terminal.write, and preserve the existing try/catch behavior to surface
errors via writeSystemMessage(activeTerminal, ...). Keep the
terminal.attachCustomKeyEventHandler logic (isTerminalClearShortcut,
event.preventDefault/stopPropagation and return false) so the cleared event flow
you already handle remains intact.
🧹 Nitpick comments (2)
apps/web/src/components/ThreadTerminalDrawer.tsx (1)
110-382: Consider extracting terminal setup + resize logic into subcomponents/hooks.This component is doing several distinct jobs (resize handling, terminal lifecycle, theme observer, shortcut wiring). Splitting into focused hooks/subcomponents would make it easier to maintain and test.
Based on learnings: "Extract large React components into multiple subcomponents with granular functionality. Co-locate subcomponents in the same file as the main component. Avoid hoisting callbacks too high up the component tree; prefer colocating logic close to JSX."
apps/server/src/git.ts (1)
71-108: Clarify whethermaxOutputBytesis per-stream or total.Right now stdout and stderr can each reach
maxOutputBytes, so total capture can be ~2× the stated cap, while the truncation note reads like a single limit. If you want a hard overall cap, track a shared remaining budget across both streams or rename tomaxOutputBytesPerStream.
| const sendClearShortcut = async () => { | ||
| const activeTerminal = terminalRef.current; | ||
| if (!activeTerminal) return; | ||
| try { | ||
| await api.terminal.write({ threadId, data: "\u000c" }); | ||
| } catch (error) { | ||
| writeSystemMessage( | ||
| activeTerminal, | ||
| error instanceof Error ? error.message : "Failed to clear terminal", | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| terminal.attachCustomKeyEventHandler((event) => { | ||
| if (!isTerminalClearShortcut(event)) return true; | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| void sendClearShortcut(); | ||
| return false; | ||
| }); |
There was a problem hiding this comment.
Use api.terminal.clear for the clear shortcut to reset persisted history.
Right now the shortcut only sends \u000c, so the server-side history is retained and will replay on reopen; it also misses the “clear scrollback” semantics expected from Cmd+K. Calling the clear API aligns with the cleared event you already handle.
🔧 Suggested change
- await api.terminal.write({ threadId, data: "\u000c" });
+ await api.terminal.clear({ threadId });🤖 Prompt for AI Agents
In `@apps/web/src/components/ThreadTerminalDrawer.tsx` around lines 263 - 282, The
clear-shortcut handler currently sends a form-feed via sendClearShortcut (which
calls api.terminal.write with "\u000c") but should call the server-side clear
endpoint to reset persisted history; update sendClearShortcut to call await
api.terminal.clear({ threadId }) instead of api.terminal.write, and preserve the
existing try/catch behavior to surface errors via
writeSystemMessage(activeTerminal, ...). Keep the
terminal.attachCustomKeyEventHandler logic (isTerminalClearShortcut,
event.preventDefault/stopPropagation and return false) so the cleared event flow
you already handle remains intact.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/web/src/components/ChatView.tsx`:
- Around line 1212-1234: The terminal drawer is being opened with the project
root as cwd (cwd={activeProject.cwd}) which can run commands in the wrong
workspace for threads that have a worktree; change the cwd prop passed to
ThreadTerminalDrawer to use the thread's worktree when available (e.g.,
activeThread.worktree or similar) and fall back to activeProject.cwd otherwise,
updating the JSX at the ThreadTerminalDrawer usage so the cwd prop becomes the
conditional value.
- Around line 412-421: The keydown handler in the useEffect (the inline function
referenced by handler) should ignore repeated keydown events to avoid rapid
toggling; update the handler used with window.addEventListener to check
event.repeat (return early if true) before calling isTerminalToggleShortcut and
toggleTerminalVisibility, keeping the existing guards for activeThreadId and
event.preventDefault(); reference the useEffect, handler,
isTerminalToggleShortcut, toggleTerminalVisibility, and activeThreadId symbols
when making this change.
🧹 Nitpick comments (1)
apps/web/src/components/ChatView.tsx (1)
695-705: Add toggle semantics for a11y (optional).Consider exposing toggle state for assistive tech.
♿ Suggested tweak
<button type="button" className={`rounded-md px-2 py-1 text-[10px] transition-colors duration-150 ${ activeThread.terminalOpen ? "bg-accent text-foreground" : "text-muted-foreground/40 hover:text-muted-foreground/60" }`} + aria-pressed={activeThread.terminalOpen} onClick={toggleTerminalVisibility} >
| useEffect(() => { | ||
| const handler = (event: globalThis.KeyboardEvent) => { | ||
| if (!activeThreadId) return; | ||
| if (!isTerminalToggleShortcut(event)) return; | ||
| event.preventDefault(); | ||
| toggleTerminalVisibility(); | ||
| }; | ||
| window.addEventListener("keydown", handler); | ||
| return () => window.removeEventListener("keydown", handler); | ||
| }, [activeThreadId, toggleTerminalVisibility]); |
There was a problem hiding this comment.
Guard against key-repeat toggling.
Holding Cmd/Ctrl+J will emit repeat keydown events and can rapidly flip the terminal open/closed.
💡 Suggested fix
useEffect(() => {
const handler = (event: globalThis.KeyboardEvent) => {
+ if (event.repeat) return;
if (!activeThreadId) return;
if (!isTerminalToggleShortcut(event)) return;
event.preventDefault();
toggleTerminalVisibility();
};📝 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.
| useEffect(() => { | |
| const handler = (event: globalThis.KeyboardEvent) => { | |
| if (!activeThreadId) return; | |
| if (!isTerminalToggleShortcut(event)) return; | |
| event.preventDefault(); | |
| toggleTerminalVisibility(); | |
| }; | |
| window.addEventListener("keydown", handler); | |
| return () => window.removeEventListener("keydown", handler); | |
| }, [activeThreadId, toggleTerminalVisibility]); | |
| useEffect(() => { | |
| const handler = (event: globalThis.KeyboardEvent) => { | |
| if (event.repeat) return; | |
| if (!activeThreadId) return; | |
| if (!isTerminalToggleShortcut(event)) return; | |
| event.preventDefault(); | |
| toggleTerminalVisibility(); | |
| }; | |
| window.addEventListener("keydown", handler); | |
| return () => window.removeEventListener("keydown", handler); | |
| }, [activeThreadId, toggleTerminalVisibility]); |
🤖 Prompt for AI Agents
In `@apps/web/src/components/ChatView.tsx` around lines 412 - 421, The keydown
handler in the useEffect (the inline function referenced by handler) should
ignore repeated keydown events to avoid rapid toggling; update the handler used
with window.addEventListener to check event.repeat (return early if true) before
calling isTerminalToggleShortcut and toggleTerminalVisibility, keeping the
existing guards for activeThreadId and event.preventDefault(); reference the
useEffect, handler, isTerminalToggleShortcut, toggleTerminalVisibility, and
activeThreadId symbols when making this change.
| {activeThread.terminalOpen && api && activeProject && ( | ||
| <ThreadTerminalDrawer | ||
| key={activeThread.id} | ||
| api={api} | ||
| threadId={activeThread.id} | ||
| cwd={activeProject.cwd} | ||
| height={activeThread.terminalHeight} | ||
| focusRequestId={terminalFocusRequestId} | ||
| onHeightChange={(height) => | ||
| dispatch({ | ||
| type: "SET_THREAD_TERMINAL_HEIGHT", | ||
| threadId: activeThread.id, | ||
| height, | ||
| }) | ||
| } | ||
| onThreadExited={() => | ||
| dispatch({ | ||
| type: "SET_THREAD_TERMINAL_OPEN", | ||
| threadId: activeThread.id, | ||
| open: false, | ||
| }) | ||
| } | ||
| /> |
There was a problem hiding this comment.
Use the thread worktree as terminal cwd.
Line 1217 always uses the project root; for worktree threads this can send commands to the wrong workspace. Prefer the thread’s worktree when present.
🛠️ Proposed fix
- cwd={activeProject.cwd}
+ cwd={activeThread.worktreePath ?? activeProject.cwd}📝 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.
| {activeThread.terminalOpen && api && activeProject && ( | |
| <ThreadTerminalDrawer | |
| key={activeThread.id} | |
| api={api} | |
| threadId={activeThread.id} | |
| cwd={activeProject.cwd} | |
| height={activeThread.terminalHeight} | |
| focusRequestId={terminalFocusRequestId} | |
| onHeightChange={(height) => | |
| dispatch({ | |
| type: "SET_THREAD_TERMINAL_HEIGHT", | |
| threadId: activeThread.id, | |
| height, | |
| }) | |
| } | |
| onThreadExited={() => | |
| dispatch({ | |
| type: "SET_THREAD_TERMINAL_OPEN", | |
| threadId: activeThread.id, | |
| open: false, | |
| }) | |
| } | |
| /> | |
| {activeThread.terminalOpen && api && activeProject && ( | |
| <ThreadTerminalDrawer | |
| key={activeThread.id} | |
| api={api} | |
| threadId={activeThread.id} | |
| cwd={activeThread.worktreePath ?? activeProject.cwd} | |
| height={activeThread.terminalHeight} | |
| focusRequestId={terminalFocusRequestId} | |
| onHeightChange={(height) => | |
| dispatch({ | |
| type: "SET_THREAD_TERMINAL_HEIGHT", | |
| threadId: activeThread.id, | |
| height, | |
| }) | |
| } | |
| onThreadExited={() => | |
| dispatch({ | |
| type: "SET_THREAD_TERMINAL_OPEN", | |
| threadId: activeThread.id, | |
| open: false, | |
| }) | |
| } | |
| /> |
🤖 Prompt for AI Agents
In `@apps/web/src/components/ChatView.tsx` around lines 1212 - 1234, The terminal
drawer is being opened with the project root as cwd (cwd={activeProject.cwd})
which can run commands in the wrong workspace for threads that have a worktree;
change the cwd prop passed to ThreadTerminalDrawer to use the thread's worktree
when available (e.g., activeThread.worktree or similar) and fall back to
activeProject.cwd otherwise, updating the JSX at the ThreadTerminalDrawer usage
so the cwd prop becomes the conditional value.
| this.schedulePersist(threadId); | ||
| } | ||
|
|
||
| private async persistHistory(threadId: string, history: string): Promise<void> { |
There was a problem hiding this comment.
🟢 Low
src/terminalManager.ts:385 enqueuePersistWrite catches all errors internally, so persistHistory never rejects. If the disk write fails in clear(), in-memory history is wiped but old data remains on disk—reappearing after reload. Consider propagating write errors so callers can handle them.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file apps/server/src/terminalManager.ts around line 385:
`enqueuePersistWrite` catches all errors internally, so `persistHistory` never rejects. If the disk write fails in `clear()`, in-memory history is wiped but old data remains on disk—reappearing after reload. Consider propagating write errors so callers can handle them.
1 participant
Adds a terminal
Summary by CodeRabbit