feat: add integrated terminal panel#88
Conversation
realDuang
left a comment
There was a problem hiding this comment.
Hey @OverCart345, thanks for putting this integrated terminal is definitely a feature we've been wanting. The xterm.js + node-pty combo is the right choice, and the multi-tab / per-session state management looks well thought out.together
That said, there's one architectural call I'd like us to align on before iterating further, plus a few things that need fixing:
Architecture: Gateway vs IPC
The biggest thing this PR goes through Electron IPC directly, bypassing the Gateway WebSocket layer. I get that it's simpler and IPC is slightly lower latency, but in practice:here
- node-pty + xterm.js already supports WebSocket transport natively (this is exactly how VS Code Server / code-server / ttyd work). The PTY side stays the same, you just swap the transport.
- We already have a mature Gateway infra with auto-reconnect, auth, and RPC. Hooking into it is close to zero extra work.
- Going through Gateway means we get Web PTY support for free just a matter of exposing the same handlers. IPC locks us into Electron-only with no upgrade path.later
So the ask is: please rearchitect this to go through the Gateway (terminal.create, terminal.write, terminal.data as new request/notification types in ws-server.ts + engine-manager.ts). The TerminalPanel frontend can stay mostly as-is, just swap terminalAPI calls for gateway.terminalCreate() etc.
If we do this, the terminalAPI in electron-api.ts, the preload additions, and the IPC handlers all go the whole thing actually gets simpler.away
Other items
See inline comments for specifics, but the highlights:
- **
cwdis passed straight topty.spawn()with zero validation. Need at minimum a directory-exists check and path normalization.Security** - Process
destroyAllTerminals()is exported but never wired intoapp.on('will-quit'). PTY children will orphan on quit.leak - Hardcoded
ru_RU.UTF- should been_US.UTF-8or detect from system.8 - No
terminal-manager.tsneeds unit tests, especially the lifecycle/cleanup paths.tests - ** all xterm instances across all sessions live forever in DOM. Needs some kind of eviction for inactive sessions.Memory**
Happy to discuss the Gateway approach if you want to hop on a call or open a design issue first.
|
|
||
| const env: Record<string, string> = { ...process.env } as Record<string, string>; | ||
| if (!env.LANG) env.LANG = "ru_RU.UTF-8"; | ||
| if (!env.LC_CTYPE) env.LC_CTYPE = env.LANG; |
There was a problem hiding this comment.
This ru_RU.UTF-8 fallback looks like it leaked from your local env :) Should be en_US.UTF-8 as a safe default — or better, detect via app.getLocale() and map to a POSIX locale.
| if (!env.LC_CTYPE) env.LC_CTYPE = env.LANG; | ||
| if (!env.LC_ALL) env.LC_ALL = env.LANG; | ||
|
|
||
| const ptyProcess = pty.spawn(shell, [], { |
There was a problem hiding this comment.
The cwd param comes straight from the renderer and gets passed directly to pty.spawn() with no validation.
At minimum we need:
path.resolve()to normalizefs.existsSync()+statSync().isDirectory()check- Maybe restrict to known project directories (the session already knows its
directory)
This is especially important since CodeMux supports remote access — a compromised renderer could spawn shells in arbitrary directories.
| term.pty.kill(); | ||
| terminals.delete(id); | ||
| } | ||
| } |
There was a problem hiding this comment.
destroyAllTerminals() is exported but never actually called anywhere. It needs to be wired into app.on("will-quit") in electron/main/index.ts, otherwise all PTY child processes become orphans when the app quits.
Also — there's no error handling around pty.spawn(). If the shell doesn't exist or cwd is invalid, this will throw uncaught.
| return isStartupReady(); | ||
| }); | ||
|
|
||
| // =========================================================================== |
There was a problem hiding this comment.
Per the architecture discussion — the terminal service should go through the Gateway WebSocket layer (as new request/notification types like terminal.create, terminal.write, terminal.data) rather than IPC.
Reasoning:
node-pty+xterm.jsnatively supports WebSocket transport (same as VS Code Server / code-server)- We already have Gateway infra with auth, reconnect, RPC
- Going through Gateway gives us Web PTY support later for free
- The IPC handlers, preload additions, and
electron-api.tswrapper all become unnecessary
The TerminalPanel frontend stays mostly the same — just swap terminalAPI.* calls for gateway.terminal*() methods.
| const xterm = new XTerm({ | ||
| cursorBlink: true, | ||
| fontSize: 13, | ||
| fontFamily: |
There was a problem hiding this comment.
Each tab registers a global terminal:data listener that receives output from all terminals, then filters by ID. With N tabs open, every PTY write fans out to N handlers in the renderer.
Once this moves to Gateway, consider using per-terminal subscription channels (or at least do the ID dispatch on the server side before sending to the client).
| @@ -0,0 +1,348 @@ | |||
| import { createSignal, createEffect, onCleanup, For, Show } from "solid-js"; | |||
There was a problem hiding this comment.
A few hard-coded strings that need i18n treatment:
"New terminal"(button title)"Hide terminal panel"(button title)`Terminal ${n}`(tab label)
Should use t().terminal.* — you already added the locale key for togglePanel, just need a few more.
| stroke-width="2" | ||
| stroke-linecap="round" | ||
| stroke-linejoin="round" | ||
| class="flex-shrink-0 opacity-60" |
There was a problem hiding this comment.
All tabs from all sessions stay in allTabs() permanently — DOM nodes and xterm instances never get cleaned up. If someone uses 20+ sessions over a workday, that's 20+ xterm renderers alive in memory.
Consider some kind of LRU eviction for inactive sessions — e.g., dispose xterm instances after a session has been inactive for a while, and re-attach from the still-running PTY when the user switches back.
| return session?.title || ""; | ||
| }); | ||
|
|
||
| // Terminal panel state — per-session open/closed |
There was a problem hiding this comment.
Terminal state management (terminalOpenBySession, terminalHeight, toggleTerminal, closeTerminal, addTerminalTab) adds ~35 lines here. Chat.tsx is already 2300+ lines.
Would be cleaner to pull this into a dedicated src/stores/terminal.ts — same pattern as fileStore for the file explorer panel. Keeps Chat.tsx as a composition root rather than accumulating more state.
Summary
Adds an integrated terminal panel to the chat interface. The terminal opens below the chat/file explorer area, supports multiple tabs per session, and preserves PTY state when switching between sessions.
Changes
TerminalPanelcomponent — xterm.js with multi-tab UI, per-session stateterminal:create/write/resize/destroy)Test Plan
bun run test:unit)bun run test:e2e)npm run typecheck)Screenshots