feat(vscode): xterm.js terminal tabs in agent manager#9268
feat(vscode): xterm.js terminal tabs in agent manager#9268marius-kilocode merged 2 commits intomainfrom
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ) { | ||
| return undefined | ||
| } | ||
| if (!this.isExperimentalTerminalsEnabled()) { |
There was a problem hiding this comment.
WARNING: Toggling the flag off can orphan already-open PTYs
This guard drops agentManager.terminal.close and agentManager.terminal.resize for terminals that were opened before the setting changed. The webview still keeps those tabs around, so closing one only removes the UI state; xtermManager.close() never runs and the backend shell keeps running until the whole panel is disposed. The flag check should only block new terminal creation, or explicitly tear down tracked PTYs when the feature is turned off.
There was a problem hiding this comment.
No longer applicable — the experimentalTerminalTabs feature flag was removed in a later commit on this PR; the guard this comment was about no longer exists. Terminal tabs ship unconditionally alongside the legacy integrated-terminal shortcuts now, so there's no flag to toggle off.
| this.entries.delete(terminalId) | ||
| try { | ||
| const client = this.deps.getClient() | ||
| await client.pty.remove({ directory: entry.cwd, ptyID: entry.ptyID }) |
There was a problem hiding this comment.
WARNING: Failed PTY deletes are silently treated as success
client.pty.remove() only throws when throwOnError is enabled. Here the returned result is ignored, so a 4xx/5xx response leaves the backend PTY alive while the entry is dropped locally and Terminal closed is logged. That can leak orphan shells for the lifetime of the CLI server. Please check the returned error field or call this endpoint with { throwOnError: true }.
There was a problem hiding this comment.
Good catch — fixed in 4007c1f252. Both close() and dispose() now destructure { error } from the SDK response (same pattern used in create() and resize()) and log a clear "PTY may linger until kilo serve exits" message when the delete fails. Transport-level exceptions continue to be caught separately.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Fix these issues in Kilo Cloud Files Reviewed (40 files)
Reviewed by gpt-5.4-20260305 · 1,063,553 tokens |
marius-kilocode
force-pushed
the
feat/agent-manager-terminal-tabs
branch
6 times, most recently
from
0346e95 to
c2137f7
Compare
marius-kilocode
changed the title
marius-kilocode
force-pushed
the
feat/agent-manager-terminal-tabs
branch
from
c2137f7 to
4007c1f
Compare
marius-kilocode
force-pushed
the
feat/agent-manager-terminal-tabs
branch
2 times, most recently
from
2bdb62f to
7094a31
Compare
| @@ -0,0 +1,211 @@ | |||
| /** | |||
| * Experimental terminal manager. | |||
There was a problem hiding this comment.
do you consider it experimental? if so, should we hide it behind an experimental setting?
There was a problem hiding this comment.
So there are probably some edge, cases but it also doesn't hurt to introduce it. We won't intefere with standard functionality for now.
| const closeTerminal = (terminalId: string) => { | ||
| const ids = deps.tabIds() | ||
| const idx = ids.indexOf(terminalId) | ||
| const nextId = ids[idx + 1] ?? ids[idx - 1] |
There was a problem hiding this comment.
is this a hacky way of checking idx == ids.length?
There was a problem hiding this comment.
Not a hacky check — it's ids[idx+1] ?? ids[idx-1], which handles three cases in one line:
- closed tab wasn't the last → focus the next tab
- closed tab was the last →
ids[idx+1]isundefined, fall back to the previous tab - closed tab was the only one → both sides are
undefined,nextIdisundefinedand we intentionally focus nothing
Explicit length check would need the same two branches. Added a comment in 387ed0001b so the intent reads faster.
There was a problem hiding this comment.
Rewritten in f210beb732 — explicit IIFE with named hasNext / hasPrev locals and early returns for each case:
const nextId = ((): string | undefined => {
if (idx < 0) return undefined
const hasNext = idx + 1 < ids.length
if (hasNext) return ids[idx + 1]
const hasPrev = idx > 0
if (hasPrev) return ids[idx - 1]
return undefined
})()No more ?? trick, reads top-to-bottom as 'next → previous → nothing'. Matches the style guide's 'use IIFE instead of let' preference.
| "@opencode-ai/ui": "workspace:*", | ||
| "@thisbeyond/solid-dnd": "0.7.5", | ||
| "@xterm/addon-fit": "0.10.0", | ||
| "@xterm/xterm": "5.5.0", |
There was a problem hiding this comment.
do we have an idea of the impact on the size of the extension this has?
There was a problem hiding this comment.
Measured on disk before bundling (@xterm/xterm 5.5 ships pre-minified):
@xterm/xtermruntime: 283 KB (lib/xterm.js)@xterm/addon-fit: 1.5 KBxterm.css: 5.4 KB- Total: ~290 KB uncompressed, ~82 KB gzipped added to
agent-manager.js/agent-manager.css.
The .js.map files (~1.1 MB) are not bundled into the webview output.
For context, the unminified dev agent-manager.js is ~15 MB and the production-minified bundle is a few MB, so ~290 KB is a small fraction.
There was a problem hiding this comment.
Update after bumping to @xterm/xterm@6.0.0 + @xterm/addon-fit@0.11.0 in e5a8232f26:
@xterm/xtermruntime: 477 KB (up from 283 KB on 5.5 — 6.0 integrates the VS Code base platform scrollbar and other features)@xterm/addon-fit: 1.5 KB (unchanged)xterm.css: 6.9 KB (up from 5.4 KB)- New total: ~485 KB uncompressed, ~135 KB gzipped added to
agent-manager.js/agent-manager.css.
Dev bundle: 15 MB → 17 MB; production-minified delta is the ~135 KB gzip figure above.
There was a problem hiding this comment.
Added three more addons in 95dfb6cd6b — all compatible with 6.x:
@xterm/addon-web-links0.12.0 → 3.0 KB (clickable URLs in output)@xterm/addon-clipboard0.2.0 → 6.2 KB (OSC 52 for tmux/nvim)@xterm/addon-unicode110.9.0 → 51 KB (fixes emoji / CJK cell width for powerlevel10k / starship prompts; ~48 KB of that is the Unicode 11 width tables themselves)
New combined total: ~545 KB uncompressed, ~155 KB gzipped added to agent-manager.js.
marius-kilocode
force-pushed
the
feat/agent-manager-terminal-tabs
branch
4 times, most recently
from
f210beb to
95dfb6c
Compare
| } | ||
| if (m.type === "agentManager.terminal.close") { | ||
| void this.manager.close(m.terminalId).then(() => { | ||
| this.deps.post({ type: "agentManager.terminal.closed", terminalId: m.terminalId }) |
There was a problem hiding this comment.
WARNING: Close success is reported even when PTY teardown fails
TerminalManager.close() logs and returns when client.pty.remove() fails, so this callback still posts agentManager.terminal.closed and the webview removes the tab. That leaves the backend PTY alive until kilo serve exits, which is the resource-leak path this feature is trying to avoid. Only emit the closed message after a confirmed delete, or send an error message instead.
marius-kilocode
force-pushed
the
feat/agent-manager-terminal-tabs
branch
from
95dfb6c to
2396737
Compare
Click the chevron next to the + tab button and pick 'New Terminal' (or hit Cmd+Shift+T / Ctrl+Shift+T) to spawn a real shell inside the selected worktree or Local directory. Each terminal runs via the Kilo CLI's PTY backend, streamed over a direct loopback WebSocket so raw bytes bypass postMessage. Tabs mirror the worktree split-button pattern, support mixed drag-reorder with session tabs, and survive worktree-context switches without losing xterm state (slots are opacity-toggled in a persistent absolute-positioned layer, never unmounted). The legacy Cmd+/ integrated-terminal shortcut and console icon are preserved so existing muscle memory keeps working.
marius-kilocode
force-pushed
the
feat/agent-manager-terminal-tabs
branch
from
2396737 to
561f862
Compare
…rminal-tabs # Conflicts: # packages/opencode/src/global/index.ts # packages/opencode/src/pty/index.ts
2 participants
Summary
+tab button and pick "New Terminal" (or pressCmd+Shift+T/Ctrl+Shift+T) to spawn a real xterm.js terminal in the selected worktree or Local directory. Each tab runs its own shell via the Kilo CLI's existing PTY backend, streamed over a direct loopback WebSocket.Cmd+/and the>_console button are unchanged. Users can pick whichever they prefer.Notes for reviewers
packages/kilo-vscode/and the narrow security fix inpackages/opencode/src/global/index.ts+packages/opencode/src/pty/index.ts(strip$KILO_SERVER_PASSWORDfrom spawned shell env; defensive.trim()on xdg paths).webview-ui/agent-manager/terminal/so the feature can be retired cleanly if we ever want to.eslint.config.mjsraisesAgentManagerApp.tsxmax-lines3100 → 3175 (~40 lines of signal bindings and a stacking-container wrapper that must live alongside existing selection state).display: nonedoesn't work for xterm subtrees) is documented interminal/render.tsx's JSDoc.Design docs and follow-up
Testing
Walked through 22 edge cases locally. Key invariants verified:
pwdreturns the worktree path when opened in a worktree context)TerminalRouter.dispose()