feat(desktop): terminal in v2 pane#3108
Merged
Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds a terminal runtime registry and runtime/transport modules for pane-scoped persistent terminals with attach/detach/dispose; backend WebSocket route switches to paneId with session reattachment, buffered output and a dispose message; a global lifecycle hook delays disposal to avoid premature teardown. Changes
Sequence Diagram(s)sequenceDiagram
participant TP as TerminalPane
participant REG as terminalRuntimeRegistry
participant RT as TerminalRuntime
participant TR as TerminalTransport
participant WS as WebSocket
participant BE as Backend
TP->>REG: attach(paneId, container, wsUrl)
REG->>RT: getOrCreate/createRuntime(paneId)
REG->>TR: getOrCreate/createTransport()
RT->>RT: restore scrollback, measure cols/rows
REG->>RT: attachToContainer(container)
REG->>TR: sendResize(cols, rows)
alt transport not connected to wsUrl
REG->>TR: connect(transport, terminal, wsUrl)
TR->>WS: new WebSocket(wsUrl)
WS->>BE: upgrade -> session[paneId] getOrCreate
BE-->>TR: (optional) replay buffered output
end
BE->>WS: send {type:"data"|"replay"|"exit"|"error"}
WS-->>TR: onmessage -> TR writes to terminal
TP->>REG: detach(paneId)
REG->>RT: detachFromContainer() -- persist scrollback, remove DOM
note right of TR: transport may remain connected for reattach
sequenceDiagram
participant UGL as useGlobalTerminalLifecycle
participant DB as v2WorkspaceLocalState
participant REG as terminalRuntimeRegistry
UGL->>DB: subscribe live workspace state
DB-->>UGL: state updates
UGL->>UGL: compute current terminal paneIds
UGL->>UGL: schedule 500ms disposal for removed paneIds
UGL->>UGL: cancel timers for re-added paneIds
alt 500ms elapsed and paneId still absent
UGL->>REG: dispose(paneId)
REG->>REG: sendDispose, disposeRuntime/transport, remove entry
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts">
<violation number="1" location="apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts:68">
P2: Avoid empty catch blocks in persistence helpers; log a warning with `paneId` and error so storage failures are diagnosable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.ts:76">
P2: Narrow this effect dependency to the specific collection field it reads; depending on the whole `collections` object can trigger redundant lifecycle work.
(Based on your team's feedback about narrowing React effect dependencies to required fields.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx:30">
P2: `useSyncExternalStore` subscribes before the runtime exists, so the initial subscription is a no-op and connection state updates can be missed on first mount.</violation>
</file>
<file name="packages/host-service/src/terminal/terminal.ts">
<violation number="1" location="packages/host-service/src/terminal/terminal.ts:60">
P2: Use byte-length accounting for the replay buffer. `data.length` does not match byte size, so the 64KB cap can be exceeded with multibyte terminal output.</violation>
<violation number="2" location="packages/host-service/src/terminal/terminal.ts:86">
P2: Do not use an empty catch block when killing the PTY; log the error so disposal failures remain observable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx (1)
14-33: Subscription function identity changes on every render, causing re-subscriptions.
useSyncExternalStorecompares subscribe function identity. SincesubscribeToState(paneId)returns a new function on each render, React will unsubscribe and resubscribe every render cycle.♻️ Memoize the subscribe function
+import { useEffect, useRef, useSyncExternalStore, useCallback, useMemo } from "react"; -import { useEffect, useRef, useSyncExternalStore } from "react"; import { type ConnectionState, terminalRuntimeRegistry, } from "renderer/lib/terminal/terminal-runtime-registry"; import { useWorkspaceWsUrl } from "../../../../../providers/WorkspaceTrpcProvider/WorkspaceTrpcProvider"; interface TerminalPaneProps { paneId: string; workspaceId: string; } -function subscribeToState(paneId: string) { - return (callback: () => void) => - terminalRuntimeRegistry.onStateChange(paneId, callback); -} - -function getConnectionState(paneId: string): ConnectionState { - return terminalRuntimeRegistry.getConnectionState(paneId); -} - export function TerminalPane({ paneId, workspaceId }: TerminalPaneProps) { const containerRef = useRef<HTMLDivElement | null>(null); const websocketUrl = useWorkspaceWsUrl(`/terminal/${paneId}`, { workspaceId, }); + const subscribe = useCallback( + (callback: () => void) => + terminalRuntimeRegistry.onStateChange(paneId, callback), + [paneId], + ); + + const getSnapshot = useCallback( + () => terminalRuntimeRegistry.getConnectionState(paneId), + [paneId], + ); + - const connectionState = useSyncExternalStore( - subscribeToState(paneId), - () => getConnectionState(paneId), - ); + const connectionState = useSyncExternalStore(subscribe, getSnapshot);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx around lines 14 - 33, The subscribe function passed into useSyncExternalStore is recreated each render because subscribeToState(paneId) returns a new function; make the subscribe callback stable by creating a memoized subscribe function inside TerminalPane (e.g., with useCallback) that calls terminalRuntimeRegistry.onStateChange(paneId, callback) and depends only on paneId, then pass that memoized function into useSyncExternalStore along with the existing getConnectionState(paneId) selector so React will not unsubscribe/resubscribe on every render.apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts (1)
206-216: Consider debouncing resize events to reduce WebSocket message volume.
ResizeObservercan fire rapidly during continuous resizing (e.g., window drag). Each callback sends a WebSocket message, which could overwhelm the server or cause unnecessary PTY resize operations.♻️ Add debounce to resize handler
+function debounce<T extends (...args: unknown[]) => void>( + fn: T, + ms: number, +): (...args: Parameters<T>) => void { + let timeoutId: ReturnType<typeof setTimeout> | null = null; + return (...args: Parameters<T>) => { + if (timeoutId) clearTimeout(timeoutId); + timeoutId = setTimeout(() => fn(...args), ms); + }; +} + function setupResizeObserver(runtime: TerminalRuntime) { runtime.resizeObserver?.disconnect(); if (!runtime.container) return; - const observer = new ResizeObserver(() => { + const handleResize = debounce(() => { measureAndResize(runtime); sendResize(runtime); - }); + }, 100); + + const observer = new ResizeObserver(handleResize); observer.observe(runtime.container); runtime.resizeObserver = observer; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts` around lines 206 - 216, The ResizeObserver callback in setupResizeObserver calls measureAndResize and sendResize on every event; debounce the resize handling to avoid flooding the WebSocket by creating a debounced wrapper (e.g., debouncedResize) and calling that from the observer instead of sendResize directly—implement the debounce using a short delay (e.g., 100–200ms) and store the debounced function on the runtime (e.g., runtime.debouncedResize) so it can be cancelled/cleared when disconnecting; update setupResizeObserver to invoke measureAndResize immediately if you need instant layout updates and then schedule the debounced sendResize, or have the debounced wrapper call both measureAndResize and sendResize, and ensure you cancel the debounce when calling runtime.resizeObserver?.disconnect().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts`:
- Around line 330-337: onStateChange currently returns early when no runtime
exists, causing listeners registered during useSyncExternalStore initialization
to be lost; modify onStateChange to eagerly create (or retrieve-or-create) the
runtime for the given paneId before adding the listener so the callback is
registered even if attach() runs later — i.e., replace the early return that
checks this.runtimes.get(paneId) with logic to create or get the runtime (via
the existing runtime creation method you already use elsewhere, e.g.,
createRuntime or getOrCreateRuntime), then add listener to
runtime.stateListeners and return the unsubscribe that deletes it.
In `@packages/host-service/src/terminal/terminal.ts`:
- Around line 58-66: The bufferOutput function currently uses data.length and
never truncates the newest chunk, so MAX_BUFFER_BYTES can be exceeded; change to
measure bytes with Buffer.byteLength(data, "utf8"), and if that single incoming
chunk would push session.bufferBytes over MAX_BUFFER_BYTES, truncate the
incoming UTF‑8 string to fit (reduce the new chunk to MAX_BUFFER_BYTES -
session.bufferBytes bytes when encoded) before pushing into session.buffer; then
update session.bufferBytes using byte lengths and keep the existing loop that
shifts off oldest entries (now comparing byte counts) until session.bufferBytes
<= MAX_BUFFER_BYTES. Ensure you reference bufferOutput, session.buffer,
session.bufferBytes and MAX_BUFFER_BYTES when making these changes.
- Around line 115-126: Existing session reattachments overwrite existing.socket
but do not prevent old/displaced WebSocket handlers from acting on paneId and
affecting the new socket; update the logic in the reattach path (the block
handling sessions.get(paneId) where existing.socket is set and
replayBuffer/sendMessage are called) and in the handlers that respond to
onMessage/onClose/onError so that each handler verifies it is acting on the
currently attached socket before mutating session state. Concretely, assign a
unique token/id (or capture the ws instance) to the session (e.g.,
session.socketId or capture socketRef) when replacing existing.socket and then,
inside the message/close/error handlers (the onMessage/onClose/onError code
paths referenced across the file), check that the token/ws identity still
matches the session’s current socket before performing replayBuffer, dispose,
clearing session.socket, or other state changes; only proceed if they match,
otherwise reject the stale event.
---
Nitpick comments:
In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts`:
- Around line 206-216: The ResizeObserver callback in setupResizeObserver calls
measureAndResize and sendResize on every event; debounce the resize handling to
avoid flooding the WebSocket by creating a debounced wrapper (e.g.,
debouncedResize) and calling that from the observer instead of sendResize
directly—implement the debounce using a short delay (e.g., 100–200ms) and store
the debounced function on the runtime (e.g., runtime.debouncedResize) so it can
be cancelled/cleared when disconnecting; update setupResizeObserver to invoke
measureAndResize immediately if you need instant layout updates and then
schedule the debounced sendResize, or have the debounced wrapper call both
measureAndResize and sendResize, and ensure you cancel the debounce when calling
runtime.resizeObserver?.disconnect().
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx:
- Around line 14-33: The subscribe function passed into useSyncExternalStore is
recreated each render because subscribeToState(paneId) returns a new function;
make the subscribe callback stable by creating a memoized subscribe function
inside TerminalPane (e.g., with useCallback) that calls
terminalRuntimeRegistry.onStateChange(paneId, callback) and depends only on
paneId, then pass that memoized function into useSyncExternalStore along with
the existing getConnectionState(paneId) selector so React will not
unsubscribe/resubscribe on every render.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09a3bc3f-689a-47b8-8de2-1614908a952c
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/GlobalTerminalLifecycle.tsxapps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/index.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useGlobalTerminalLifecycle/index.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.tsapps/desktop/src/renderer/routes/_authenticated/layout.tsxpackage.jsonpackages/host-service/src/terminal/terminal.ts
Split terminal-runtime-registry.ts into three focused modules: - terminal-runtime.ts: XTerm instance, addons, DOM wrapper, buffer/dimension persistence, fit/resize - terminal-ws-transport.ts: WebSocket connection, message protocol, connection state - terminal-runtime-registry.ts: orchestrator mapping paneId to runtime + transport
Contributor
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts">
<violation number="1" location="apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts:62">
P2: Avoid empty `catch {}` when parsing websocket payloads; log the error object so malformed-message failures remain diagnosable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts">
<violation number="1" location="apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts:56">
P2: Do not use empty `catch {}` here; log a warning with context so storage/serialization failures are diagnosable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts (1)
81-84:⚠️ Potential issue | 🟠 MajorState subscription is dropped if
onStateChangeruns before firstattach().Returning a noop when the entry is missing can miss
useSyncExternalStoresubscriptions created before runtime initialization.💡 Suggested fix
onStateChange(paneId: string, listener: () => void): () => void { - const entry = this.entries.get(paneId); - if (!entry) return () => {}; + const entry = this.getOrCreate(paneId); entry.transport.stateListeners.add(listener); return () => { entry.transport.stateListeners.delete(listener); }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts` around lines 81 - 84, onStateChange currently returns a noop if this.entries lacks paneId, dropping subscriptions created before attach(); change it to queue the listener instead so early subscriptions are retained: add a pendingStateListeners: Map<string, Set<() => void>> (or create a minimal entry placeholder) and in onStateChange, if no entry exists, add the listener to pendingStateListeners[paneId] and return an unsubscribe that removes it from that set; update attach (or the code that creates entries) to flush pendingStateListeners into the real entry.transport.stateListeners when the entry is created. Ensure references to entries, attach, transport.stateListeners, and onStateChange are used so the queued listeners are migrated and unsubscribe works both before and after attach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts`:
- Around line 58-67: dispose currently removes the local entry even when the
transport is disconnected so sendDispose is a no-op and the backend PTY can be
orphaned; update dispose(paneId) to first check entry.transport connection state
(e.g., transport.connected or isConnected()) and if connected await
sendDispose(entry.transport) (ensure sendDispose returns/uses a Promise),
otherwise invoke a fallback server-dispose via the runtime (e.g., a new method
on the runtime like disposeRemoteSession or ensureRuntimeSessionDisposed) to
explicitly tell the backend to kill the PTY; only after the appropriate
remote-dispose path completes call disposeTransport(entry.transport),
disposeRuntime(entry.runtime) and finally this.entries.delete(paneId),
referencing dispose, entries, sendDispose, disposeTransport, disposeRuntime,
transport and runtime in your changes.
---
Duplicate comments:
In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts`:
- Around line 81-84: onStateChange currently returns a noop if this.entries
lacks paneId, dropping subscriptions created before attach(); change it to queue
the listener instead so early subscriptions are retained: add a
pendingStateListeners: Map<string, Set<() => void>> (or create a minimal entry
placeholder) and in onStateChange, if no entry exists, add the listener to
pendingStateListeners[paneId] and return an unsubscribe that removes it from
that set; update attach (or the code that creates entries) to flush
pendingStateListeners into the real entry.transport.stateListeners when the
entry is created. Ensure references to entries, attach,
transport.stateListeners, and onStateChange are used so the queued listeners are
migrated and unsubscribe works both before and after attach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e325005-d246-420d-860e-f67b04c5b50f
📒 Files selected for processing (3)
apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime.tsapps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts
saddlepaddle
reviewed
Apr 2, 2026
- Guard onClose/onError/onMessage with socket reference comparison to prevent displaced connections from corrupting active session state - Close old socket explicitly on reattach (code 4000) - Create registry entry eagerly in onStateChange so useSyncExternalStore listeners are registered before the attach effect runs
Move hook from routes/_authenticated/hooks/ into components/GlobalTerminalLifecycle/hooks/ since it's only used by that component.
detach() now only removes the DOM wrapper, resize observer, and focus — the WebSocket and xterm data flow stay alive so output written while the pane is hidden (tab switch, workspace switch) is not lost. attach() checks whether the transport is already open for the same URL and skips reconnection on simple re-shows, only reconnecting when the socket dropped or the endpoint changed. A full terminal.refresh() is added on re-attach to repaint rows that were written while the canvas was offscreen.
Contributor
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts">
<violation number="1" location="apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts:49">
P2: Treat an in-flight connection to the same URL as already connected to avoid reconnect churn on rapid reattach.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
connect() now early-returns when already open or connecting to the same URL, covering rapid tab-switch during an in-flight handshake. The registry no longer branches — it just calls connect() unconditionally.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts (1)
75-83:⚠️ Potential issue | 🔴 CriticalDon't drop the local entry before the remote session is definitely disposed.
sendDispose()is a no-op unless the socket is already open. If the pane disappears after a renderer restart or transient disconnect, this still tears down the local registry entry and can leave the backend PTY running. Please keep a fallback remote-dispose path before callingdisposeTransport()/entries.delete().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts` around lines 75 - 83, The dispose(paneId) implementation currently calls sendDispose(entry.transport) then immediately disposes the local transport/runtime and deletes the registry entry, which can leave the remote PTY running if the socket was not open; change dispose(paneId) so it first attempts a reliable remote-dispose fallback before tearing down local resources: check entry.transport/socket state and if sendDispose would be a no-op, invoke the backend remote-dispose API (or enqueue a remote-dispose retry) for entry.runtime/entry.id, only after confirming the remote session has been requested to stop (or fallback enqueued) proceed to call disposeTransport(entry.transport), disposeRuntime(entry.runtime) and this.entries.delete(paneId); keep references to entry.transport and entry.runtime while performing the remote-dispose path so the local entry is not dropped prematurely.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts (1)
1-16: Prefer therenderer/...alias for these internal imports.This file sits under
apps/desktop, and both local imports can use the existing tsconfig alias instead of relative segments.As per coding guidelines,
apps/desktop/**/*.{ts,tsx}: Use alias as defined intsconfig.jsonwhen possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts` around lines 1 - 16, This file uses relative imports for terminal internals; update them to use the tsconfig alias "renderer/..." instead of "./terminal-runtime" and "./terminal-ws-transport". Replace the first import that brings in TerminalRuntime, attachToContainer, createRuntime, detachFromContainer, disposeRuntime with an aliased import from "renderer/lib/terminal/terminal-runtime" and replace the second import that brings in ConnectionState, TerminalTransport, connect, createTransport, disposeTransport, sendDispose, sendResize with an aliased import from "renderer/lib/terminal/terminal-ws-transport" so the same exported symbols are imported via the renderer alias.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts`:
- Around line 165-167: The detachFromContainer currently calls persistBuffer
once but leaves the transport/xterm alive, so implement two changes: (1)
register a renderer-shutdown handler that calls persistBuffer(runtime.paneId,
runtime.serializeAddon) and persistDimensions(runtime.paneId, runtime.lastCols,
runtime.lastRows) to flush the latest scrollback when the renderer exits (use
the same runtime identifiers as in detachFromContainer), and (2) debounce or
throttle periodic snapshots taken while the pane is detached (where
serializeAddon is invoked) so you continue updating the persisted snapshot at a
controlled rate while hidden; update the code paths that trigger
serializeAddon/periodic snapshotting to check runtime.detached (or equivalent)
and use a short debounce/timer to avoid gaps in middle scrollback.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.ts`:
- Around line 17-23: The current loop iterates over tab.panes which can include
orphaned metadata; instead traverse the persisted layout tree to collect pane
IDs and then confirm those IDs are terminal panes via the live metadata. In
useGlobalTerminalLifecycle, iterate tab.layout (the persisted layout/pane tree)
to gather paneIds, then for each id check tab.panes[paneId]?.kind === "terminal"
before adding to ids so orphaned entries don't prevent
terminalRuntimeRegistry.dispose() from running.
---
Duplicate comments:
In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts`:
- Around line 75-83: The dispose(paneId) implementation currently calls
sendDispose(entry.transport) then immediately disposes the local
transport/runtime and deletes the registry entry, which can leave the remote PTY
running if the socket was not open; change dispose(paneId) so it first attempts
a reliable remote-dispose fallback before tearing down local resources: check
entry.transport/socket state and if sendDispose would be a no-op, invoke the
backend remote-dispose API (or enqueue a remote-dispose retry) for
entry.runtime/entry.id, only after confirming the remote session has been
requested to stop (or fallback enqueued) proceed to call
disposeTransport(entry.transport), disposeRuntime(entry.runtime) and
this.entries.delete(paneId); keep references to entry.transport and
entry.runtime while performing the remote-dispose path so the local entry is not
dropped prematurely.
---
Nitpick comments:
In `@apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.ts`:
- Around line 1-16: This file uses relative imports for terminal internals;
update them to use the tsconfig alias "renderer/..." instead of
"./terminal-runtime" and "./terminal-ws-transport". Replace the first import
that brings in TerminalRuntime, attachToContainer, createRuntime,
detachFromContainer, disposeRuntime with an aliased import from
"renderer/lib/terminal/terminal-runtime" and replace the second import that
brings in ConnectionState, TerminalTransport, connect, createTransport,
disposeTransport, sendDispose, sendResize with an aliased import from
"renderer/lib/terminal/terminal-ws-transport" so the same exported symbols are
imported via the renderer alias.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 900f37f2-ceb8-4378-8c41-668370e2145d
📒 Files selected for processing (7)
apps/desktop/src/renderer/lib/terminal/terminal-runtime-registry.tsapps/desktop/src/renderer/lib/terminal/terminal-runtime.tsapps/desktop/src/renderer/lib/terminal/terminal-ws-transport.tsapps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/GlobalTerminalLifecycle.tsxapps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/index.tsapps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.tspackages/host-service/src/terminal/terminal.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/GlobalTerminalLifecycle.tsx
- packages/host-service/src/terminal/terminal.ts
- apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts
Kitenite
changed the title
7 tasks
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 5, 2026
…t-sh#3108) upstream 1588d20 — G2 ghost merge復元 - terminal-runtime-registry.ts を3モジュールに分割 (runtime, ws-transport, registry) - GlobalTerminalLifecycle コンポーネント追加 - TerminalPane簡素化、host-service/terminal.ts更新 - package.json: trustedDependencies追加、fstreamパッチ維持
9 tasks
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 5, 2026
cherry-pick方式で内容を取り込み済みの14コミットをgit履歴上もマージ済みにする。 取り込み済み (cherry-pick / 手動移植): - be22b46 superset-sh#3125 — スキップ (下記参照) - 88bc7fb superset-sh#3127 — Revert DA1 ✓ - 92d0ff9 superset-sh#3054 — DA1 fix ✓ - c48450e superset-sh#3093 — file viewer pane fix ✓ - fffa8db superset-sh#3128 — version 1.4.7 ✓ - 589a7c7 superset-sh#3136 — fuzzy scorer (ハイブリッド方式) ✓ - ceb8c81 superset-sh#3150 — Electron 40.8.5 ✓ - 8922b94 superset-sh#3137 — terminalId分離 ✓ - c7508e5 superset-sh#3152 — GitHub無料化 ✓ - 2b91f11 superset-sh#3155 — v2 terminal theme ✓ - b8b11af superset-sh#3154 — TUI dimension fix ✓ - 7599ace superset-sh#3149 — v2 sidebar file tree (手動統合) ✓ - 4d7c612 superset-sh#3174 — DnD重複削除 ✓ - 864977d superset-sh#3157 — Host Service分離 ✓ 意図的にスキップ: - be22b46 superset-sh#3125 (GitHub polling簡素化) フォーク独自のGitHubSyncService (バックエンド集中ポーリング) と 設計が異なるため不採用。upstreamはフロントエンドhover駆動、フォークは バックエンドキャッシュウォーマー方式。詳細は githubQueryPolicy.ts と github-sync-service.ts のFORK NOTEを参照。 ゴースト・マージ復元 (revert 134cfd5 で消失した内容): - 538f306 superset-sh#3120 — Patch vuln ✓ - 1588d20 superset-sh#3108 — terminal lifecycle分離 ✓ - 59426f6 superset-sh#3122 — file tree + FilePane + Alert refactor (手動統合) ✓ - 10d9a5d superset-sh#3097 — tiptap line-height ✓ - 337a9ae superset-sh#3121 — Codex hooks削除 ✓
This was referenced Apr 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Terminal runtime registry (
terminal-runtime-registry.ts): App-level singleton keyed bypaneIdthat manages xterm instances, websocket connections, and DOM attach/detach. The xterm wrapper div is reparented between containers — surviving tab/workspace switches without losing scrollback. Serializes buffer to localStorage on detach via@xterm/addon-serializeso scrollback survives renderer restarts.Server session registry (
terminal.ts): Route changed from/terminal/:workspaceIdto/terminal/:paneId. PTY lifetime is now independent of socket lifetime — socket close = detach (buffer output up to 64KB), reconnection replays buffer, explicitdisposemessage kills PTY.workspaceIdis init metadata passed as a query param.TerminalPane as attach/detach wrapper: Reduced from 168 to ~50 lines. Calls
registry.attach()on mount,registry.detach()on unmount. No xterm/websocket creation logic.Global terminal lifecycle hook (
useGlobalTerminalLifecycle): Mounted once in authenticated layout. Watchesv2WorkspaceLocalStateacross all workspaces. Disposes terminals only when apaneIddisappears from every persisted workspace layout, with a 500ms re-check delay for cross-workspace moves.Lifecycle model
Test plan
Summary by cubic
Decouples the terminal runtime from the view lifecycle so terminals persist across tab/workspace switches and renderer restarts without losing scrollback. Adds a per-pane runtime+transport registry and a new
/terminal/:paneIdserver route with buffered replay and explicit disposal.New Features
terminal-runtime.ts(per-pane@xterm/xterm, wrapper reparenting, buffer/dimension persistence, full repaint on reattach),terminal-ws-transport.ts(idempotentconnect— no-op if already open or connecting to the same URL; resize/input/dispose; closes displaced socket with code 4000; guards handlers against displaced sockets; connection state observable),terminal-runtime-registry.ts(per-paneIdorchestrator; attach/detach only; always callsconnect()— idempotency handled in transport; keeps WS alive across detach; eager entry creation foronStateChange).TerminalPanenow takespaneId, attaches/detaches only, and usesuseSyncExternalStoreto show connection state; shows a small “Disconnected” bar when closed.paneId; PTY lifetime is independent of the socket; up to 64KB is buffered and replayed on reconnect; reconnect displaces the previous socket;onClose/onError/onMessageare guarded by socket reference;disposekills the PTY.paneIddisappears from all persisted workspace layouts (500ms grace for cross-workspace moves).Migration
/terminal/:paneIdand passworkspaceIdas a query param.TerminalPanereceivespaneIdandworkspaceId.Written for commit 3259e18. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes / UI