Feat: Persist and synchronize terminals#234
Conversation
jamesmurdza
force-pushed
the
feat/sync-terminals
branch
from
84954a8 to
e9ab395
Compare
akhileshrangani4
left a comment
akhileshrangani4
left a comment
There was a problem hiding this comment.
Good direction overall — caching projects server-side and broadcasting terminal events is the right call. Per-terminal resize fix is correct, the RunButtonModal cleanup is way nicer, and deleting that 96-line terminals/index.tsx feels great.
Main blocker: the project cache leaks everything. Fix that before merging — without it the server accumulates zombie projects forever. Rest is good to ship and iterate on.
One thing I couldn't leave as an inline comment since it's outside the diff: web/lib/api/terminal.ts:34 eagerly adds the terminal to local state, then the server broadcasts terminalCreated which triggers TerminalContext.tsx:82-85 to add it again (the prev.some guard catches the dupe). But if createTerminal fails on the server, you've got a ghost terminal in client state with no PTY behind it and no cleanup path.
|
|
||
| // Initialize containers and managers | ||
| const connections = new ConnectionManager() | ||
| const projectCache = new Map<string, Project>() |
There was a problem hiding this comment.
projectCache only grows, never shrinks. The disconnect handler on line 170 removes the socket but never calls project.disconnect() or deletes from the cache. That disconnect() method exists at Project.ts:169 and does all the right cleanup (kill dev servers, close terminals, stop file watchers) — but nothing ever invokes it. Every project anyone ever opens stays alive with its container, PTYs, and watchers running until you restart the server.
Consider tracking connection count and cleaning up when it hits zero:
socket.on("disconnect", async () => {
connections.removeConnectionForProject(socket, data.projectId)
if (connections.connectionsForProject(data.projectId).length === 0) {
await project.disconnect()
projectCache.delete(data.projectId)
}
})This will absolutely bite you in prod.
| } | ||
| return project | ||
| })() | ||
| projectCreationPromises.set(projectId, promise) |
There was a problem hiding this comment.
Once the promise resolves it just sits here forever. Tack on a .finally(() => projectCreationPromises.delete(projectId)) and you're good.
| await project.initialize() | ||
| // Get or create project (serialized so concurrent connections share one project) | ||
| const project = await getOrCreateProject(data.projectId) | ||
| await project.fileManager?.startWatching(sendFileNotifications) |
There was a problem hiding this comment.
Every new socket calls startWatching with its own callback. If startWatching stacks watchers instead of replacing the callback, you'll get duplicate file notifications multiplied by connection count. Worth checking.
| newSocket.on("ready", () => { | ||
| setIsReady(true) | ||
| // Defer so TerminalContext's useEffect can register listeners first | ||
| setTimeout(() => newSocket.emit("getInitialState"), 0) |
There was a problem hiding this comment.
You're deferring getInitialState with a setTimeout(0) hoping TerminalContext's useEffect has registered its listeners by then. Works today but React doesn't guarantee that — especially in StrictMode or concurrent mode. Cleaner to have TerminalContext emit getInitialState itself after it sets up listeners.
| if (buf !== undefined) { | ||
| buf += responseString | ||
| if (buf.length > MAX_SCREEN_BUFFER_CHARS) { | ||
| buf = buf.slice(-MAX_SCREEN_BUFFER_CHARS) |
There was a problem hiding this comment.
This cut could land in the middle of an ANSI escape sequence like \x1B[38;2;255;. When that gets replayed via term.write(initialScreen) at terminal.tsx:134 the terminal parser will freak out — garbled colors, missing text, cursor weirdness. Slice at a newline boundary or scan past partial sequences.
| ) | ||
| } | ||
| }, [term]) | ||
| }, []) |
There was a problem hiding this comment.
This effect references socket, id, setTerm, theme, initialScreen from the closure but has [] deps. Linter would yell at this. At least add a suppression comment explaining the intent.
| disposableOnResize.dispose() | ||
| resizeObserver.disconnect() | ||
| } | ||
| }, [term, terminalContainerRef.current]) |
There was a problem hiding this comment.
Ref .current in a dependency array doesn't trigger re-renders so this dep does nothing. Just remove it to avoid confusion.
| } | ||
|
|
||
| // Handle stopping the preview server (kills dev server, closes preview terminal) | ||
| const handleStopPreview: SocketHandler = async () => { |
There was a problem hiding this comment.
This calls terminalManager.closeTerminal() directly and broadcasts terminalClosed itself. But handleCloseTerminal at line 156 also checks if it's the run terminal and clears preview. Same result, two different code paths. Easy to get out of sync on future changes — consider having stopPreview go through handleCloseTerminal instead.
| tabComponent: "terminal", | ||
| }) | ||
| if (!isSocketReady || hasAttemptedInitialCreateRef.current) return | ||
| const t = setTimeout(() => { |
There was a problem hiding this comment.
Magic 400ms delay to wait for dock refs. Fragile — too short on slow machines, unnecessary latency on fast ones. Key off actual readiness (both refs being set) instead of a hardcoded timeout.
| const ref = terminalRef.current | ||
| const dock = dockRef.current | ||
| const panelId = `terminal-${id}` | ||
| if (ref?.getPanel(panelId) || dock?.getPanel(panelId)) return |
There was a problem hiding this comment.
nit: this exact guard (ref?.getPanel(panelId) || dock?.getPanel(panelId)) is copy-pasted in right-header-actions, usePanelToggles, shortcuts, and here. Pull it into a helper.
3 participants
This PR syncs and persists terminal sessions per project. This only happens in server memory, so all sessions will be lost when the server restarts.
This is a necessary first step to have long running agents like Claude Code and OpenCode in the terminal.
Untitled.mov