Make web terminals persistent and attachable - #1614
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
-
[Blocker] Active-terminal reset tears down the newly mounted xterm —
TerminalViewregisters the xterm ref andonDatahandler from its child passive effect, then the new parent effect clears that ref and disposes the handler on the sameactiveTerminalIdchange. Output is dropped and keyboard input is no longer forwarded. Evidenceweb/src/routes/sessions/terminal.tsx:431.
Suggested fix:useEffect(() => { connectOnceRef.current = false setExitInfo(null) return () => { terminalRef.current = null inputDisposableRef.current?.dispose() inputDisposableRef.current = null } }, [activeTerminalId])
-
[Major] Terminal ownership and inventory changes remain local to the acting socket — session lists use
socket.emit, while attach overwrites the priorsocketIdwithout notifying that client. Other open views retain stale tabs; a displaced owner remainsconnected, stops receiving output, and has writes silently rejected. Evidencehub/src/socket/handlers/terminal.ts:70, related ownership transfer athub/src/socket/handlers/terminal.ts:143.
Suggested fix:const room = `session:${sessionId}` socket.join(room) // also on terminal:list socket.nsp.to(room).emit('terminal:sessions', buildTerminalSessions(sessionId)) const previousSocketId = entry.socketId terminalRegistry.attach(entry.terminalId, entry.sessionId, socket.id) if (previousSocketId && previousSocketId !== socket.id) { socket.nsp.sockets.get(previousSocketId)?.emit('terminal:error', { terminalId: entry.terminalId, message: 'Terminal attached in another view.' }) }
Summary
Review mode: initial
- 1 Blocker, 1 Major. Do not merge until the terminal lifecycle and cross-view synchronization issues are fixed.
Testing
- Not run (automation; PR code was not executed).
- Add a test using the real
TerminalViewmount/switch lifecycle; the current route test mocks it completely. - Add two-socket hub tests for attach takeover and cross-view create/close/list synchronization.
HAPI Bot
|
|
||
| useEffect(() => { | ||
| connectOnceRef.current = false | ||
| terminalRef.current = null |
There was a problem hiding this comment.
[BLOCKER] The active-terminal effect tears down the newly mounted terminal
TerminalView invokes onMount from its own passive effect (web/src/components/Terminal/TerminalView.tsx:46-135). Passive mount effects run child-first, so after an activeTerminalId change the new child stores the xterm instance and registers terminal.onData, then this effect immediately nulls the ref and disposes that subscription. The later resize callback can still attach the PTY, but onOutput drops all bytes through the null ref and keyboard input is no longer forwarded.
Suggested fix:
useEffect(() => {
connectOnceRef.current = false
setExitInfo(null)
return () => {
terminalRef.current = null
inputDisposableRef.current?.dispose()
inputDisposableRef.current = null
}
}, [activeTerminalId])| if (!isAuthorizedSession(sessionId)) { | ||
| return | ||
| } | ||
| socket.emit('terminal:sessions', { |
There was a problem hiding this comment.
[MAJOR] Ownership and list updates are not propagated to other views
Every mutation calls this per-connection helper, so only the acting socket receives the new terminal list. In addition, attachExistingTerminal overwrites an existing socketId without notifying the displaced socket. That view remains in connected state, receives no more output, and its writes are silently rejected by resolveEntryForSocket; create/close changes in another view also leave its selector stale.
Suggested fix:
const room = `session:${sessionId}`
socket.join(room) // also join when handling terminal:list
socket.nsp.to(room).emit('terminal:sessions', buildTerminalSessions(sessionId))
const previousSocketId = entry.socketId
terminalRegistry.attach(entry.terminalId, entry.sessionId, socket.id)
if (previousSocketId && previousSocketId !== socket.id) {
socket.nsp.sockets.get(previousSocketId)?.emit('terminal:error', {
terminalId: entry.terminalId,
message: 'Terminal attached in another view.'
})
}
There was a problem hiding this comment.
Findings
-
[Major] Detached terminal removals do not refresh subscribed inventories — exit/error/CLI-disconnect and idle paths notify only current viewers. A detached background terminal has no viewer, so subscribed selectors keep a stale tab/count and can remain incorrectly maxed out until another action requests the list. Evidence
hub/src/socket/handlers/cli/terminalHandlers.ts:143, relatedhub/src/socket/handlers/cli/terminalHandlers.ts:165,hub/src/socket/handlers/cli/terminalHandlers.ts:172, andhub/src/socket/server.ts:99.
Suggested fix:terminalRegistry.remove(parsed.data.terminalId) terminalNamespace .to(`session:${entry.sessionId}`) .emit('terminal:exit', parsed.data) // Apply the same session-room notification to error, cleanup, and idle removal.
-
[Major] Closing or exiting the last pre-existing terminal creates a replacement — when initial inventory contains a terminal,
autoCreateSessionRefis never marked for the session. Once that terminal disappears, the empty-list branch creates another PTY; multiple open views can each do this. Evidenceweb/src/routes/sessions/terminal.tsx:345.
Suggested fix:if (nextTerminal) { autoCreateSessionRef.current = sessionId setActiveTerminalId(nextTerminal.terminalId) return }
Summary
Review mode: follow-up after new commits
- 2 Major findings. The prior lifecycle teardown and cross-view takeover findings were changed, but removal synchronization and one-time auto-create semantics still block the persistent-terminal workflow.
Testing
- Not run (automation; PR code was not executed).
- Add a hub test where a detached terminal exits, idles, or loses its CLI while another socket is subscribed to the session inventory.
- Add a web test that updates the mocked inventory to empty after closing/exiting a pre-existing sole terminal and asserts
createTerminalis not called.
HAPI Bot
| return | ||
| } | ||
| terminalSocket.emit('terminal:exit', parsed.data) | ||
| emitToTerminalViewers(terminalNamespace, entry, 'terminal:exit', parsed.data) |
There was a problem hiding this comment.
[MAJOR] Detached terminal removals leave every selector stale
This sends the removal event only to entry.viewerSocketIds. Detached terminals normally have zero viewers, while browser sockets remain inventory subscribers through terminal:list. If a detached background PTY exits, errors, idles, or its CLI disconnects, nobody receives an event and therefore nobody requests the updated terminal:sessions list. The stale tab/count remains visible and can keep “New” disabled at the configured max.
Use the existing session room for removal notifications; useTerminalSocket already requests the list on terminal:exit/terminal:error and ignores non-current terminal state changes:
terminalRegistry.remove(parsed.data.terminalId)
terminalNamespace
.to(`session:${entry.sessionId}`)
.emit('terminal:exit', parsed.data)
// Apply the same session-room notification to error, cleanup, and idle removal.| ? terminals.find((terminal) => terminal.terminalId === preferredTerminalId) | ||
| : null | ||
| const nextTerminal = preferred ?? terminals.at(-1) ?? null | ||
| if (nextTerminal) { |
There was a problem hiding this comment.
[MAJOR] A pre-existing last terminal is recreated after Close or exit
The ref is set only by the empty-inventory auto-create branch. When the page initially selects an existing server terminal here, the ref stays null. After that sole terminal is explicitly closed or exits, the refreshed empty inventory reaches line 350 and silently creates a replacement PTY; with multiple open views, each view can create one.
Mark initial inventory handling when selecting an existing terminal:
if (nextTerminal) {
autoCreateSessionRef.current = sessionId
setActiveTerminalId(nextTerminal.terminalId)
return
}
There was a problem hiding this comment.
Findings
- [Major] Deterministic auto-terminal IDs can kill a replacement after Close — the page reuses
term-${sessionId}-autoon every mount. The CLI kills a closed PTY asynchronously; its oldonExitcallback then callscleanup(terminalId), so if the page has already recreated that ID, the callback kills the new runtime and its stale exit removes the new hub registry entry. Evidenceweb/src/routes/sessions/terminal.tsx:367, relatedcli/src/terminal/TerminalManager.ts:202andhub/src/socket/handlers/cli/terminalHandlers.ts:148.
Suggested fix:If cross-view auto-create deduplication is required, use a separate idempotency key or server-side create-if-empty operation rather than reusing the PTY resource ID.const terminalId = `term-${sessionId}-${randomId()}`
Summary
Review mode: follow-up after new commits
- 1 Major finding. A closed auto-created terminal can race with immediate page remount/recreation and tear down the replacement.
Testing
- Not run (automation; PR code was not executed).
- Add a regression test that closes the auto-created terminal, recreates a terminal for the same session before the old process exit callback fires, and verifies the stale callback cannot remove or kill the replacement.
HAPI Bot
| // views simultaneously observe an empty session, both create calls | ||
| // converge on the same server-side PTY instead of racing two random | ||
| // terminal resources into existence. | ||
| const terminalId = `term-${sessionId}-auto` |
There was a problem hiding this comment.
[MAJOR] Reusing the auto ID can tear down its replacement
term-${sessionId}-auto is reused on every fresh page mount. Close removes the hub entry and asks the CLI to kill the PTY, but process exit is asynchronous: the old runtime’s onExit still calls cleanup(terminalId) (cli/src/terminal/TerminalManager.ts:202). If the user reloads/reopens the empty terminal page first, the replacement is created under the same ID; the stale callback then cleans up that new runtime, and its terminal:exit also matches and removes the new hub entry (hub/src/socket/handlers/cli/terminalHandlers.ts:148).
Keep PTY resource IDs unique across lifecycles; use a separate server-side idempotency/create-if-empty mechanism if simultaneous auto-create requests must converge:
const terminalId = `term-${sessionId}-${randomId()}`
There was a problem hiding this comment.
Findings
- [Major] Deterministic auto-terminal IDs can tear down a replacement after Close — every fresh empty-page mount reuses
term-${sessionId}-auto. The hub removes the registry entry before asking the CLI to close, whileTerminalManager.close()kills the old process asynchronously. If the page remounts and recreates the same ID before the oldonExitcallback runs, that callback executescleanup(terminalId)against the new runtime, kills it, and its staleterminal:exitremoves the new hub entry. Evidenceweb/src/routes/sessions/terminal.tsx:367, relatedhub/src/socket/handlers/terminal.ts:383andcli/src/terminal/TerminalManager.ts:202.
Suggested fix:If simultaneous empty-page creation must converge, use a separate idempotency key or a server-side create-if-empty operation; do not reuse the PTY resource ID across lifecycles.const terminalId = `term-${sessionId}-${randomId()}`
Summary
Review mode: follow-up after new commits
- 1 Major finding. The latest commit only adjusts terminal-page assertions; it does not resolve the deterministic-ID lifecycle race from the prior bot review.
Testing
- Not run (automation; PR code was not executed).
- Add a regression test that closes the auto-created terminal, recreates a terminal for the same session before the old process exit callback fires, and verifies the stale callback cannot remove or kill the replacement.
HAPI Bot
| // views simultaneously observe an empty session, both create calls | ||
| // converge on the same server-side PTY instead of racing two random | ||
| // terminal resources into existence. | ||
| const terminalId = `term-${sessionId}-auto` |
There was a problem hiding this comment.
[MAJOR] Reusing this PTY ID can kill its replacement
Every fresh empty-page mount reuses term-${sessionId}-auto. Close removes the hub entry first and asks the CLI to kill the PTY, but process exit is asynchronous. If this page remounts and recreates the ID before the old process exits, the old onExit callback calls cleanup(terminalId) (cli/src/terminal/TerminalManager.ts:202) against the new runtime, killing it; the stale exit also matches and removes the new hub registry entry (hub/src/socket/handlers/terminal.ts:383).
Keep resource IDs unique across lifecycles:
const terminalId = `term-${sessionId}-${randomId()}`If concurrent auto-create requests must converge, use a separate idempotency key or server-side create-if-empty operation.
There was a problem hiding this comment.
Findings
- [Minor] New terminal-management controls bypass localization — the selector, terminal labels, attach/close hints, New action, empty state, and loading state are hardcoded in English, so the Chinese locale now renders a mixed-language terminal page. Evidence
web/src/routes/sessions/terminal.tsx:711(related added strings at lines 715, 731, 735, 741, 756, 759, 816, 826, and 832).
Suggested fix:Add matching keys toconst label = t("terminal.sessionLabel", { index: index + 1 }) // Use t("terminal.sessions"), t("terminal.new"), // t("terminal.close", { label }), etc. for every new user-facing string.
web/src/lib/locales/en.tsandweb/src/lib/locales/zh-CN.ts.
Summary
Review mode: follow-up after new commits
- 1 Minor finding. The previous deterministic terminal-ID lifecycle issue is resolved in the latest head; the remaining issue is the untranslated UI introduced by this PR.
Testing
- Not run (automation; PR code was not executed).
- Add a terminal-page rendering assertion under the
zh-CNlocale covering selector, close/new actions, and the empty/loading states.
HAPI Bot
| <ConnectionIndicator status={status} /> | ||
| </div> | ||
| {session.active && terminalSupported ? ( | ||
| <div className="mx-auto flex w-full max-w-content items-center gap-2 border-b border-[var(--app-border)] px-3 py-2"> |
There was a problem hiding this comment.
[MINOR] Localize the new terminal-management UI
This page already routes terminal text through t(...), but the new selector and related controls hardcode English (Terminal sessions, Terminal N, Attached, Close, New terminal, empty/loading text, and the max-limit title). Under zh-CN, the feature therefore renders a mixed-language page.
const label = t("terminal.sessionLabel", { index: index + 1 })
<div role="tablist" aria-label={t("terminal.sessions")}>
...
<span>{t("terminal.new")}</span>
</div>Add corresponding keys in both web/src/lib/locales/en.ts and web/src/lib/locales/zh-CN.ts, and use them for the close/attached/empty/loading/max strings as well.
There was a problem hiding this comment.
**Findings
- [Major] Reconnect the terminal socket after an in-place session route change - the new session-change effect resets the active terminal and disconnects the socket, while the inventory refresh effect only depends on the active/support booleans. If TanStack Router preserves
TerminalPageand the target session is already cached and active, those values remaintrue; no effect establishes a socket or requeststerminal:listfor the new session, leaving the page on "Loading terminals..." indefinitely. Evidenceweb/src/routes/sessions/terminal.tsx:468(related refresh effect atweb/src/routes/sessions/terminal.tsx:324).
Suggested fix:disconnect() if (session?.active && terminalSupported) { refreshTerminals() } // Include session?.active, terminalSupported, and refreshTerminals in deps.
Summary
Review mode: follow-up after new commits
- 1 Major finding. The prior localization finding is addressed; the remaining regression is the cached active-session route transition.
Testing
- Not run (automation; PR code was not executed).
- GitHub
testandintegrationchecks passed at review time. - Add a route-param transition test with cached active sessions A -> B and assert that B receives a new
terminal:listrequest.
HAPI Bot
| }, [activeTerminalId]) | ||
|
|
||
| useEffect(() => { | ||
| if (previousSessionIdRef.current === sessionId) { |
There was a problem hiding this comment.
[MAJOR] Reconnect after an in-place session change
This branch clears the selected terminal and disconnects the only terminal socket. The refresh effect above does not depend on sessionId; when the destination session is already cached and both sessions are active/supported, its dependencies remain unchanged, so no new socket/list request is made and the page stays in the loading state.
disconnect()
if (session?.active && terminalSupported) {
refreshTerminals()
}
// Add session?.active, terminalSupported, and refreshTerminals to this effect dependencies.Please cover an A -> B param change where both session details are cached and active.
There was a problem hiding this comment.
Findings
- [Major] Do not mark a new terminal attached before its xterm exists — 'register()' immediately adds the creator socket to 'viewerSocketIds', but the auto-create flow deliberately waits for 'terminal:sessions' before selecting/mounting that terminal. Any prompt/startup bytes arriving in that interval are filtered because it is not the current terminal, and the later 'terminal:attach' skips buffered replay because the socket is already recorded as a viewer. Evidence 'hub/src/socket/terminalRegistry.ts:61' (related 'hub/src/socket/handlers/terminal.ts:168', 'web/src/hooks/useTerminalSocket.ts:195', and 'web/src/routes/sessions/terminal.tsx:372').
Suggested fix:// Extend terminal:create with attach?: boolean (default true for legacy clients). // New web clients send attach: false; terminal:attach becomes the first viewer. register(..., socketId: string | null, ...) { const viewerSocketIds = socketId ? new Set([socketId]) : new Set<string>() // Only add terminalsBySocket when socketId is non-null. } socket.emit('terminal:create', { sessionId, terminalId, cols, rows, attach: false })
Summary
Review mode: follow-up after new commits
- 1 Major finding. The prior cached-session route-transition issue is addressed; the current creation path can still permanently omit initial PTY output before the first explicit attachment.
Testing
- Not run (automation; PR code was not executed).
- GitHub 'test' and 'integration' checks passed at review time.
- Add a regression test that emits terminal output after 'terminal:create' but before the first 'terminal:attach', then verifies the output is replayed exactly once.
HAPI Bot
There was a problem hiding this comment.
Findings
- [Major] Detached creation bypasses the per-socket terminal cap —
countForSocket()is backed by the viewer index, but the new web path always creates withattach: false, so this call registers the resource withsocketId = null. The socket count therefore stays zero for every PTY it creates; an authenticated client can open up to the per-session limit across any number of active sessions without ever reachingmaxTerminalsPerSocket. Evidencehub/src/socket/handlers/terminal.ts:257(relatedhub/src/socket/terminalRegistry.ts:61,68,194andweb/src/hooks/useTerminalSocket.ts:285).
Suggested fix:terminalRegistry.register(terminalId, sessionId, { creatorSocketId: socket.id, viewerSocketId: attach ? socket.id : null, cliSocketId, }) // Count/remove creatorSocketId in a separate resource-owner index; // detach should update only viewerSocketIds.
Summary
Review mode: follow-up after new commits
- 1 Major finding. The prior startup-output replay issue is addressed, but detached creation now makes the socket-level PTY resource guard ineffective.
Testing
- Not run (automation; PR code was not executed).
- GitHub
testandintegrationchecks passed at review time. - Add a regression test that performs
attach: falsecreates in two sessions withmaxTerminalsPerSocket: 1and verifies the second create is rejected.
HAPI Bot
| } | ||
|
|
||
| const entry = terminalRegistry.register(terminalId, sessionId, socket.id, cliSocketId) | ||
| const entry = terminalRegistry.register(terminalId, sessionId, attach ? socket.id : null, cliSocketId) |
There was a problem hiding this comment.
[MAJOR] Detached creation bypasses the per-socket terminal cap
countForSocket() is backed by terminalsBySocket, which now tracks viewers only. Because the new web client always sends attach: false, this call passes null, the created PTY is never added to that index, and the socket count remains zero. A client can therefore spawn the per-session maximum in many active sessions without ever hitting maxTerminalsPerSocket.
Track resource ownership separately from viewer attachment:
terminalRegistry.register(terminalId, sessionId, {
creatorSocketId: socket.id,
viewerSocketId: attach ? socket.id : null,
cliSocketId,
})Use the creator index for countForSocket()/removal, while detach updates only the viewer index. Please cover two attach: false creates in different sessions with maxTerminalsPerSocket: 1.
There was a problem hiding this comment.
Findings
-
[Major] Reconnect still resets the durable terminal resource cap — the new accounting lives only in the current Socket.IO handler and is cleared on disconnect even though its PTYs remain registered. Reconnecting gets an empty set, so a client can create one detached terminal, reconnect, create another in a different active session, and repeat beyond
HAPI_TERMINAL_MAX_TERMINALS. Evidencehub/src/socket/handlers/terminal.ts:67,hub/src/socket/handlers/terminal.ts:254,hub/src/socket/handlers/terminal.ts:517.
Suggested fix:const ownerKey = `${namespace}:${socket.data.userId}` if (terminalRegistry.countForOwner(ownerKey) >= maxTerminalsPerSocket) return terminalRegistry.register(terminalId, sessionId, { ownerKey, viewerSocketId, cliSocketId }) // Keep owner accounting across viewer disconnects; release it in remove().
-
[Major] One pending create can block every other terminal —
connect()andresize()gate on the whole pending-ID set, while inventory only attaches when the selected ID itself is pending. If the user presses New and selects an existing tab before the create finishes, the unselected new ID remains pending, the existing tab is never attached, and every resize only requests inventory. Evidenceweb/src/hooks/useTerminalSocket.ts:202,web/src/hooks/useTerminalSocket.ts:291,web/src/hooks/useTerminalSocket.ts:376.
Suggested fix:if (terminalChanged) { for (const pendingId of pendingTerminalIdsRef.current) { if (pendingId !== options.terminalId) { pendingTerminalIdsRef.current.delete(pendingId) } } }
Summary
Review mode: follow-up after new commits
- 2 Major findings. The previous resource-cap issue remains bypassable across reconnects, and pending-create state can strand an unrelated selected terminal.
Testing
- Not run (automation; PR code was not executed).
- GitHub
integrationpassed;testwas still in progress at review time. - Add regressions for detached create → disconnect → new-socket create with max 1, and New → select an existing tab before
terminal:ready, verifying attach and resize continue.
HAPI Bot
| for (const sessionId of new Set(detached.map((entry) => entry.sessionId))) { | ||
| emitTerminalSessions(sessionId) | ||
| } | ||
| createdTerminalIds.clear() |
There was a problem hiding this comment.
[MAJOR] Reconnect still resets the durable resource cap
createdTerminalIds is connection-local, and this clears it while the terminals themselves intentionally remain in terminalRegistry. A client can therefore create one detached PTY, reconnect with a new socket, create another in a different active session, and repeat beyond maxTerminalsPerSocket; the previous cap finding is still reachable with one reconnect between creates.
Track resource ownership with the durable registry using the authenticated user/namespace, and release it only when the terminal is removed:
const ownerKey = `${namespace}:${socket.data.userId}`
if (terminalRegistry.countForOwner(ownerKey) >= maxTerminalsPerSocket) return
terminalRegistry.register(terminalId, sessionId, { ownerKey, viewerSocketId, cliSocketId })
// Do not clear owner accounting on viewer disconnect.Please cover create → disconnect → new socket → create in another session with maxTerminalsPerSocket: 1.
| // While any create is pending, React may not yet have committed the new | ||
| // terminal ID. Never reattach a stale tab from this path. Request a fresh | ||
| // projection; terminal:sessions will attach only the pending current ID. | ||
| if (pendingTerminalIdsRef.current.size > 0) { |
There was a problem hiding this comment.
[MAJOR] A pending create blocks attaching unrelated terminals
This checks whether any terminal is pending, not whether the selected terminal is pending. If the user presses New and then selects an existing tab before the new PTY reaches terminal:ready, the new ID stays in the set, this path refuses to attach the selected existing tab, and terminal:sessions also refuses because that selected ID is not pending. resize() has the same global guard, so the view remains stuck.
Drop pending markers that are no longer the selected creation when the option changes:
if (terminalChanged) {
for (const pendingId of pendingTerminalIdsRef.current) {
if (pendingId !== options.terminalId) {
pendingTerminalIdsRef.current.delete(pendingId)
}
}
}Add a hook test for New → select an existing terminal before terminal:ready, then verify that terminal emits terminal:attach and subsequent resizes.
There was a problem hiding this comment.
Findings
-
[Major] Reconnect still resets durable resource-cap accounting — terminals intentionally survive viewer disconnects, but their creator accounting is connection-local and cleared at
hub/src/socket/handlers/terminal.ts:517. A client can create a detached terminal, reconnect without attaching it, and create another terminal in a different active session; repeating this bypassesHAPI_TERMINAL_MAX_TERMINALS.
Suggested fix:const ownerKey = `${namespace}:${socket.data.userId}` if (terminalRegistry.countForOwner(ownerKey) >= maxTerminalsPerSocket) { emitTerminalError(terminalId, `Too many terminals open (max ${maxTerminalsPerSocket}).`) return } terminalRegistry.register(terminalId, sessionId, { ownerKey, viewerSocketId: attach ? socket.id : null, cliSocketId })
-
[Major] A pending create still blocks selecting an existing terminal — the guards at
web/src/hooks/useTerminalSocket.ts:291andweb/src/hooks/useTerminalSocket.ts:376test whether any ID is pending. After New creates B detached, selecting existing A leaves B pending; B's CLIterminal:readyhad no viewer, so the marker never clears, A never emitsterminal:attach, and resize remains blocked.
Suggested fix:if (terminalChanged) { for (const pendingId of pendingTerminalIdsRef.current) { if (pendingId !== options.terminalId) { pendingTerminalIdsRef.current.delete(pendingId) } } }
Summary
Review mode: follow-up after new commits
- 2 Major findings remain. Commit
5dd5943changes only the reconnect/create hook test; it verifies stale A is not attached while B is pending, but does not cover selecting A after starting B.
Testing
- Not run (automation; PR code was not executed).
- GitHub
integrationpassed;testwas still in progress at review time. - Add regressions for create → disconnect → new socket → create in another session with max 1, and New B → select existing A before B is ready, verifying A attaches and resizes.
HAPI Bot
| for (const sessionId of new Set(detached.map((entry) => entry.sessionId))) { | ||
| emitTerminalSessions(sessionId) | ||
| } | ||
| createdTerminalIds.clear() |
There was a problem hiding this comment.
[MAJOR] Reconnect still resets durable resource-cap accounting
The PTYs intentionally remain in terminalRegistry, but this clears the only creator accounting. A new authenticated socket therefore starts at zero and can create another detached terminal in a different active session; reconnecting repeatedly bypasses maxTerminalsPerSocket.
Keep ownership in the durable registry and release it only when the terminal is removed:
const ownerKey = `${namespace}:${socket.data.userId}`
if (terminalRegistry.countForOwner(ownerKey) >= maxTerminalsPerSocket) {
emitTerminalError(terminalId, `Too many terminals open (max ${maxTerminalsPerSocket}).`)
return
}
terminalRegistry.register(terminalId, sessionId, {
ownerKey,
viewerSocketId: attach ? socket.id : null,
cliSocketId
})Please cover create → disconnect → new socket → create in another session with the limit set to 1.
| // While any create is pending, React may not yet have committed the new | ||
| // terminal ID. Never reattach a stale tab from this path. Request a fresh | ||
| // projection; terminal:sessions will attach only the pending current ID. | ||
| if (pendingTerminalIdsRef.current.size > 0) { |
There was a problem hiding this comment.
[MAJOR] A pending create still blocks selecting an existing terminal
This checks the whole pending set, not the selected ID. After New starts detached B, selecting existing A leaves B pending. Because B had no viewer when the CLI emitted terminal:ready, that marker never clears; this path only requests inventory, terminal:sessions refuses to attach non-pending A, and the matching resize guard keeps A stuck.
Drop pending creates that are no longer selected when the option changes:
if (terminalChanged) {
for (const pendingId of pendingTerminalIdsRef.current) {
if (pendingId !== options.terminalId) {
pendingTerminalIdsRef.current.delete(pendingId)
}
}
}The new test covers suppressing stale A while B is selected, but not New B → select A before B is ready.
There was a problem hiding this comment.
Findings
-
[Major] Pending creation still replaces the selected terminal identity —
createTerminal(B)writes B intoterminalIdRefeven when the controlled selection remains A. Since theoptions.terminalIddependency is still A, its effect does not run again; the nextconnect()sees B as pending and emits onlyterminal:list. The current head's new regression test fails on this exact path atweb/src/hooks/useTerminalSocket.create.test.tsx:272. Evidence:web/src/hooks/useTerminalSocket.ts:299.
Suggested fix:const suppressReconnectAttachRef = useRef(false) // connect handler if ( terminalId && size && !suppressReconnectAttachRef.current && !pendingTerminalIdsRef.current.has(terminalId) ) { emitAttach(socket, terminalId, size) } // createTerminal: do not overwrite terminalIdRef/lastSizeRef suppressReconnectAttachRef.current = true const emit = () => { socket.emit('terminal:create', payload) suppressReconnectAttachRef.current = false }
-
[Major] An unselected pending terminal can put the active tab into an error state — terminal errors are broadcast session-wide, and this condition deliberately accepts any pending ID. If detached B fails or idles out after the user returned to connected A, B is removed from the pending set but its message becomes the hook's global error state, disabling A's input until Retry. Evidence:
web/src/hooks/useTerminalSocket.ts:233.
Suggested fix:pendingTerminalIdsRef.current.delete(payload.terminalId) requestTerminalList(socket) if (!isCurrentTerminal(payload.terminalId)) { return } setErrorState(payload.message)
Summary
Review mode: follow-up after new commits
- 2 Major findings remain on the latest head.
- The required
testcheck currently fails in the newly added pending-create regression.
Testing
- Not run locally (review automation; PR code was not executed).
- GitHub
integration: passed. - GitHub
test: failed — 1 failed / 2800 passed;web/src/hooks/useTerminalSocket.create.test.tsx:272. - Add a regression where B remains pending, A is active/connected, and
terminal:errorarrives for B; A must remain connected.
HAPI Bot
| // this prevents the generic connect handler from reattaching the stale | ||
| // previous tab. The new xterm must still report its own size afterward. | ||
| pendingTerminalIdsRef.current.add(terminalId) | ||
| terminalIdRef.current = terminalId |
There was a problem hiding this comment.
[MAJOR] Pending creation still replaces the selected terminal identity
This mutates the controlled selection ref from A to B. If B is created while the prop remains A, the options.terminalId effect does not rerun because its dependency did not change; the next connect() therefore sees pending B and emits only terminal:list. The current head's new test fails on this exact path at useTerminalSocket.create.test.tsx:272.
Keep selection derived from options.terminalId; track only the short reconnect-suppression window separately:
const suppressReconnectAttachRef = useRef(false)
// connect handler
if (
terminalId
&& size
&& !suppressReconnectAttachRef.current
&& !pendingTerminalIdsRef.current.has(terminalId)
) {
emitAttach(socket, terminalId, size)
}
// createTerminal: do not overwrite terminalIdRef/lastSizeRef
suppressReconnectAttachRef.current = true
const emit = () => {
socket.emit('terminal:create', payload)
suppressReconnectAttachRef.current = false
}| if (!isCurrentTerminal(payload.terminalId)) { | ||
| const pending = pendingTerminalIdsRef.current.delete(payload.terminalId) | ||
| requestTerminalList(socket) | ||
| if (!isCurrentTerminal(payload.terminalId) && !pending) { |
There was a problem hiding this comment.
[MAJOR] A background pending error disables the active terminal
CLI/idle errors are broadcast to every session subscriber. If detached B is still pending after the user returns to connected A, this condition accepts B's error solely because it was pending and writes it into the hook-wide state. A then shows B's message and its input is disabled until Retry.
Clear background bookkeeping without changing the selected terminal's state:
pendingTerminalIdsRef.current.delete(payload.terminalId)
requestTerminalList(socket)
if (!isCurrentTerminal(payload.terminalId)) {
return
}
setErrorState(payload.message)Please cover B pending -> A connected -> terminal:error for B, asserting A remains connected.
1 participant
Summary
terminal:list,terminal:attach,terminal:detach, and explicit close semantics+ Newand per-terminal Close controlsHAPI_TERMINAL_MAX_TERMINALSlimit to the web UI and disable creation at the limitterminal:createreconnect behavior backward compatible with older web clientsBehavior
The hub registry is now the source of truth for terminal resources. A browser view can attach to one terminal at a time; switching tabs or leaving the page detaches the view without terminating the PTY. Only an explicit Close (or existing server/CLI idle/exit cleanup) removes the terminal.
Validation
tsc --noEmit: passtsc --noEmit: pass