feat(desktop): terminal persistence via daemon process#619
Conversation
📝 WalkthroughWalkthroughAdds terminal persistence and a daemon-backed terminal host: new IPC protocol and daemon, headless emulator, PTY subprocess, history persistence, workspace runtime abstraction, renderer/UX changes for cold-restore and persistent terminal tabs, TRPC endpoints and settings, and many tests and migrations. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (Terminal UI)
participant TRPC as TRPC Router
participant Registry as Workspace Runtime Registry
participant Manager as Active Terminal Manager
participant Daemon as Terminal Host Daemon
participant PTYSub as PTY Subprocess
participant History as History Persistence
Renderer->>TRPC: createOrAttach(paneId, ...)
TRPC->>Registry: getForWorkspaceId(workspaceId)
Registry->>Manager: resolve terminal runtime
Manager->>Daemon: createOrAttach (IPC) / or spawn in-process
alt daemon mode
Daemon->>PTYSub: spawn or attach
PTYSub-->>Daemon: Ready / Spawned / Data / Exit
Daemon->>History: write metadata/scrollback
Daemon-->>TRPC: response (snapshot, isColdRestore)
else in-process
Manager->>History: init/write
Manager-->>TRPC: response (scrollback, snapshot)
end
TRPC-->>Renderer: snapshot + metadata
Renderer->>Renderer: rehydrate terminal UI
loop runtime
PTYSub->>Daemon: output
Daemon-->>TRPC: emit data event
TRPC-->>Renderer: stream data event
end
sequenceDiagram
participant App as Electron App
participant Registry as Workspace Runtime Registry
participant LocalRT as LocalWorkspaceRuntime
participant InProc as In-Process Manager
participant DaemonMgr as DaemonTerminalManager
App->>Registry: getWorkspaceRuntimeRegistry()
Registry->>Registry: getDefault()
Registry->>LocalRT: instantiate LocalWorkspaceRuntime
LocalRT->>InProc: use TerminalManager (if daemon off)
LocalRT->>DaemonMgr: use DaemonTerminalManager (if daemon on)
LocalRT-->>App: return runtime with capabilities
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI Agents
In @apps/desktop/src/main/lib/terminal-host/headless-emulator.ts:
- Around line 93-97: The anonymous callback passed to terminal.onData is missing
an explicit parameter type; change the handler to declare the data parameter
type (e.g., update this.terminal.onData((data: string) => { ... })) and ensure
related members (pendingOutput array and onDataCallback) are typed accordingly
(e.g., pendingOutput: string[] and onDataCallback?: (data: string) => void) so
TypeScript no longer reports TS7006.
- Around line 10-11: The build is failing because @xterm/headless is imported
(Terminal in headless-emulator.ts) but not listed in apps/desktop/package.json;
add "@xterm/headless" with a matching version to the other @xterm packages in
the apps/desktop/package.json dependencies (or update workspace manifest if
using workspaces), run yarn/npm install to update lockfile, and ensure imports
like `Terminal` continue to resolve.
In @apps/desktop/src/main/lib/terminal/daemon-manager.ts:
- Around line 159-163: The session cleanup timer created with setTimeout (using
SESSION_CLEANUP_DELAY_MS) isn't tracked, so its callback may run after the
manager is disposed and mutate this.sessions; to fix, store the timeout ID when
scheduling the cleanup (e.g., a new map like sessionCleanupTimers keyed by
paneId) and replace the anonymous setTimeout with one that saves the ID; then
update cleanup() to iterate and clearTimeout for any pending timers and remove
their entries before disposing so sessions.delete(paneId) cannot run on a
disposed manager.
In @apps/desktop/src/main/terminal-host/daemon.test.ts:
- Around line 89-141: The startDaemon() helper can race between the
DAEMON_TIMEOUT timer and other resolve/reject paths; fix it by tracking the
timeoutId returned from setTimeout and a local settled flag, clearing the
timeout (clearTimeout(timeoutId)) and marking settled=true when resolving or
rejecting, and early-return from subsequent handlers if settled; also remove or
cleanup listeners on daemonProcess (stdout/stderr/error/exit) when settling to
avoid multiple callbacks. Ensure this logic is applied inside startDaemon(),
referencing the DAEMON_TIMEOUT, daemonProcess, output, and the resolve/reject
handlers so the timeout is cleared on success and duplicate settles are
prevented.
In @apps/desktop/src/main/terminal-host/index.ts:
- Around line 218-240: The createOrAttach handler is async but handleRequest
currently invokes it without awaiting, so post-await errors escape the
try/catch; update handleRequest to await the handler invocation (e.g., await
handler(socket, id, payload, clientState)) so thrown/rejected errors are caught,
and ensure any caller of handleRequest (the socket data event handler) is either
made async and awaits handleRequest or properly handles the returned promise to
propagate rejections into the existing try/catch; apply the same awaiting
pattern for other async handlers invoked by handleRequest.
In @apps/desktop/src/main/terminal-host/session-lifecycle.test.ts:
- Around line 91-139: The startDaemon() promise can call resolve/reject multiple
times because the DAEMON_TIMEOUT timer and process event listeners remain active
after settlement; update startDaemon to store the timeout ID,
clearTimeout(timeoutId) immediately after calling resolve() or reject(), and
remove or noop the process listeners (daemonProcess.stdout?.off/on or use once)
to prevent further callbacks; ensure the timeout is set to a variable (e.g.,
const timeoutId = setTimeout(...)) and is cleared inside the stdout 'Daemon
started' handler, the 'error' handler, and the 'exit' handler so the promise
cannot attempt to settle after it has already resolved via
startDaemon/daemonProcess/DAEMON_TIMEOUT.
- Around line 189-219: The sendRequest function creates a 5s timeout but never
clears it on successful response; modify sendRequest to store the timeout id
(from setTimeout) in a variable and call clearTimeout(timeoutId) immediately
before resolve(JSON.parse(line)) and also before rejecting on parse error, and
ensure the timeout handler removes the data listener (socket.off("data",
onData)) as it already does so when expiring.
In @apps/desktop/src/main/terminal-host/session.ts:
- Around line 611-644: The timeout cleanup in flushToSnapshotBoundary
incorrectly clears the entire snapshotBoundaryWaiters array; instead remove only
the specific waiter added for this call so concurrent flushToSnapshotBoundary
callers aren't affected. Change the boundaryPromise creation to capture the
resolve function (e.g. const resolveFn = resolve), push resolveFn into
snapshotBoundaryWaiters, and on timeout/filter cleanup remove only that
resolveFn from snapshotBoundaryWaiters (e.g. this.snapshotBoundaryWaiters =
this.snapshotBoundaryWaiters.filter(r => r !== resolveFn)); also update the
misleading comment to state we remove only our waiter; ensure this logic works
with processEmulatorWriteQueue, scheduleEmulatorWrite, and snapshotBoundaryIndex
as currently used.
🧹 Nitpick comments (26)
apps/desktop/src/renderer/stores/hotkeys/store.ts (1)
301-307: Refine the null check and resolve type inconsistency.The guard works but has two issues:
Imprecise check:
if (!state)uses truthiness, catching null, undefined, 0, false, and empty string. For an object type, prefer an explicit null check:if (state == null) { // catches both null and undefinedType mismatch: The callback is typed
(state: HotkeysState)but the guard implies the query can return null/undefined. Either:
- Update the type:
.then((state: HotkeysState | null | undefined) => {- Or fix the upstream query/storage to guarantee non-null returns
The defensive guard suggests data quality concerns in the storage layer that may warrant investigation.
🔎 Proposed refinement
trpcClient.uiState.hotkeys.get .query() - .then((state: HotkeysState) => { + .then((state: HotkeysState | null | undefined) => { // Guard against null/undefined state from storage - if (!state) { + if (state == null) { console.warn( "[hotkeys] Storage returned null/undefined state, skipping sync", ); return; }apps/desktop/src/main/lib/terminal/port-manager.ts (3)
333-336: Inconsistent handling of standalone\rcharacters.The newline index calculation treats
\ras a line terminator (viaMath.max), but the actual line splitting on line 371 uses/\r?\n/which only handles\r\nor\n, not standalone\r(classic Mac line endings).This could cause issues:
- If output contains
"foo\rbar\n", thelastNewlineIndexwould be 7 (the\n), which is correct.- But if output is
"foo\rbar"with no\n,lastNewlineIndexwould be 3 (the\r), buffering"bar"as incomplete, and"foo"would be processed as a complete line—which is likely fine.- However, if the pattern spans across the
\rboundary, it could be missed.Consider using a consistent regex for both operations, or simplifying to only track
\n:🔎 Suggested simplification
- const lastNewlineIndex = Math.max( - combined.lastIndexOf("\n"), - combined.lastIndexOf("\r"), - ); + const lastNewlineIndex = combined.lastIndexOf("\n");
358-368: Heuristic may miss some port patterns.The relevance check doesn't cover all keywords from
PORT_PATTERNS. Patterns like "bound to port", "binding to", "Serving HTTP on", and "Tomcat" might be missed if they don't co-occur with the sampled keywords.Consider aligning the heuristic with
PORT_PATTERNSfor completeness:🔎 Suggested enhancement
const looksRelevant = - /(?:localhost|127\.0\.0\.1|0\.0\.0\.0|https?:\/\/|listening|started|ready|running|\bon port\b)/i.test( + /(?:localhost|127\.0\.0\.1|0\.0\.0\.0|https?:\/\/|listening|started|ready|running|bound|binding|serving|\bon port\b)/i.test( sample, );
360-363: Extract magic numbers to named constants.The values
4096and2048for sampling thresholds should be extracted to named constants for clarity and consistency withMAX_LINE_BUFFER. Per coding guidelines, magic numbers should be extracted to module-level constants.🔎 Suggested refactor
// Max buffer size for incomplete lines (bytes) - prevents memory issues with pathological input const MAX_LINE_BUFFER = 4096; + +// Threshold for sampling output during relevance check (chars) +const RELEVANCE_SAMPLE_THRESHOLD = 4096; +const RELEVANCE_SAMPLE_SIZE = 2048;Then update the usage:
const sample = - completePart.length > 4096 - ? `${completePart.slice(0, 2048)}${completePart.slice(-2048)}` + completePart.length > RELEVANCE_SAMPLE_THRESHOLD + ? `${completePart.slice(0, RELEVANCE_SAMPLE_SIZE)}${completePart.slice(-RELEVANCE_SAMPLE_SIZE)}` : completePart;apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx (1)
16-28: Consider: Refactor conditionals to a lookup object.Per coding guidelines, lookup objects are preferred over repeated
if (type === ...)conditionals. This is optional since the current pattern is established across the file.🔎 Example refactor
const SETTINGS_COMPONENTS: Record<SettingsSection, React.ComponentType> = { account: AccountSettings, project: ProjectSettings, workspace: WorkspaceSettings, appearance: AppearanceSettings, ringtones: RingtonesSettings, keyboard: KeyboardShortcutsSettings, presets: PresetsSettings, behavior: BehaviorSettings, terminal: TerminalSettings, }; export function SettingsContent({ activeSection }: SettingsContentProps) { const Component = SETTINGS_COMPONENTS[activeSection]; return ( <div className="h-full overflow-y-auto flex justify-center"> {Component && <Component />} </div> ); }apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (2)
52-71: Consider scoping mounted terminals to current workspace.The current implementation renders all tabs across all workspaces, keeping even inactive workspace terminals mounted. While this prevents remount issues, it's more aggressive than necessary and compounds the "increased memory with many terminals" limitation noted in the PR.
Suggestion: Filter to only render tabs from the current workspace:
-{allTabs.map((tab) => { +{currentWorkspaceTabs.map((tab) => {This would:
- Still prevent TUI white screen issues when switching tabs within a workspace
- Reduce memory footprint by unmounting terminals from inactive workspaces
- Make the visible/hidden logic simpler (only need to check
tab.id === activeTabId)If keeping all workspaces mounted is intentional, please add a comment explaining why tabs from inactive workspaces must remain mounted.
10-11: Consider handling loading state explicitly to prevent potential flicker.The
terminalPersistencequery has no loading state check. During initial load,terminalPersistencewill beundefined, causing the component to render non-persistence mode (line 78-90), then potentially switch to persistence mode once loaded.If the setting loads quickly this may not be noticeable, but with slower loads it could cause a visual flicker or re-mount of terminals.
🔍 Optional: Add explicit loading check
const { data: terminalPersistence } = trpc.settings.getTerminalPersistence.useQuery(); +const { isLoading: isLoadingPersistence } = + trpc.settings.getTerminalPersistence.useQuery(); // ... -if (terminalPersistence) { +if (isLoadingPersistence) { + return null; // or loading skeleton +} + +if (terminalPersistence) {Alternatively, if the setting is expected to load synchronously from local SQLite, this may not be necessary.
Also applies to: 36-36
apps/desktop/src/main/terminal-host/pty-subprocess.ts (2)
281-291: Sensitive environment variables may be logged.The spawn debug logging includes
ZDOTDIR,SUPERSET_ORIG_ZDOTDIR, and a prefix ofPATH. While useful for debugging, this could inadvertently log sensitive environment variables in production. Consider gating this behindDEBUG_OUTPUT_BATCHINGor a separate debug flag.🔎 Proposed fix
- // Debug: Log spawn parameters - console.error("[pty-subprocess] Spawning PTY:", { - shell: msg.shell, - args: msg.args, - cwd: msg.cwd, - cols: msg.cols, - rows: msg.rows, - ZDOTDIR: msg.env.ZDOTDIR, - SUPERSET_ORIG_ZDOTDIR: msg.env.SUPERSET_ORIG_ZDOTDIR, - PATH_start: msg.env.PATH?.substring(0, 100), - }); + if (DEBUG_OUTPUT_BATCHING) { + console.error("[pty-subprocess] Spawning PTY:", { + shell: msg.shell, + args: msg.args, + cwd: msg.cwd, + cols: msg.cols, + rows: msg.rows, + }); + }
410-431: handleDispose skips graceful shutdown.Unlike
handleKill,handleDisposeimmediately sendsSIGKILLwithout first attemptingSIGTERM. This is acceptable for disposal semantics (immediate cleanup), but consider if a brief graceful window would be beneficial for processes that can save state.apps/desktop/src/main/terminal-host/daemon.test.ts (1)
45-222: Significant code duplication with session-lifecycle.test.ts.The
cleanup(),startDaemon(),stopDaemon(),connectToDaemon(), andsendRequest()functions are nearly identical between this file andsession-lifecycle.test.ts. Consider extracting these into a shared test utilities module.🔎 Proposed approach
Create a shared test utilities file:
// apps/desktop/src/main/terminal-host/__tests__/daemon-test-utils.ts export const SUPERSET_DIR_NAME = ".superset-dev"; export const SUPERSET_HOME_DIR = join(homedir(), SUPERSET_DIR_NAME); // ... other constants export function cleanup(): void { /* ... */ } export function startDaemon(): Promise<void> { /* ... */ } export function stopDaemon(daemonProcess: ChildProcess | null): Promise<void> { /* ... */ } export function connectToDaemon(): Promise<Socket> { /* ... */ } export function sendRequest(socket: Socket, request: IpcRequest): Promise<IpcResponse> { /* ... */ }Then import in both test files:
import { cleanup, startDaemon, stopDaemon, connectToDaemon, sendRequest } from "./__tests__/daemon-test-utils";apps/desktop/src/main/lib/terminal/manager.ts (1)
413-426: Write return value ignored in refreshPromptsForWorkspace.
writeQueue.write()returnsfalsewhen the queue is full, but this return value is ignored. While unlikely for a single newline to exceed the queue, the inconsistency with the strict error-throwing inwrite()at line 176 is worth noting.🔎 Proposed fix for consistency
refreshPromptsForWorkspace(workspaceId: string): void { for (const [paneId, session] of this.sessions.entries()) { if (session.workspaceId === workspaceId && session.isAlive) { try { - session.writeQueue.write("\n"); + if (!session.writeQueue.write("\n")) { + console.warn( + `[TerminalManager] Queue full when refreshing prompt for pane ${paneId}`, + ); + } } catch (error) { console.warn( `[TerminalManager] Failed to refresh prompt for pane ${paneId}:`, error, ); } } } }apps/desktop/src/main/lib/terminal/daemon-manager.ts (2)
263-287: Async operation inwriteToHistoryinitiated without awaiting or error boundary.Lines 273-280 call
initHistoryWriterwithout awaiting and only attach a.catch(() => {})which silently swallows errors. While this is fire-and-forget, consider at minimum logging the error for debuggability.🔎 Suggested improvement
this.initHistoryWriter( paneId, session.workspaceId, session.cwd, 80, 24, contentAfterClear || undefined, - ).catch(() => {}); + ).catch((error) => { + console.error( + `[DaemonTerminalManager] Failed to reinit history after clear for ${paneId}:`, + error, + ); + });
685-693: Hardcoded dimensions (80x24) when reinitializing history after clearScrollback.These magic numbers should ideally come from the current session dimensions rather than hardcoded defaults.
🔎 Suggested improvement
Consider storing current dimensions in SessionInfo or querying the daemon for current terminal size:
+ // Get current dimensions from daemon if available + // Fallback to standard defaults + const currentCols = 80; // TODO: track actual dimensions + const currentRows = 24; await this.initHistoryWriter( paneId, session.workspaceId, session.cwd, - 80, - 24, + currentCols, + currentRows, undefined, );apps/desktop/src/main/terminal-host/index.ts (4)
51-61: Duplicate SUPERSET_DIR_NAME/SUPERSET_HOME_DIR definitions.These constants are defined in both this file and
app-environment.ts. Consider importing from a shared location to avoid drift.Based on the relevant code snippets,
SUPERSET_HOME_DIRis already exported fromapps/desktop/src/main/lib/app-environment.ts. Consider importing it:-const SUPERSET_DIR_NAME = - process.env.NODE_ENV === "development" ? ".superset-dev" : ".superset"; -const SUPERSET_HOME_DIR = join(homedir(), SUPERSET_DIR_NAME); +import { SUPERSET_HOME_DIR } from "../lib/app-environment";Note: The daemon runs as a separate process, so verify the import path works in the bundled daemon script.
85-102: Auth token generation and validation are secure.
- 32 bytes (256 bits) of randomness is cryptographically strong
- Token file permissions (0o600) restrict access to owner only
- Timing-safe comparison should be used for token validation to prevent timing attacks
🔎 Use timing-safe comparison for token validation
+import { timingSafeEqual } from "node:crypto"; function validateToken(token: string): boolean { - return token === authToken; + if (token.length !== authToken.length) { + return false; + } + try { + return timingSafeEqual(Buffer.from(token), Buffer.from(authToken)); + } catch { + return false; + } }
353-376: Shutdown handler sends response before exit but uses fixed delay.The 100ms delay is arbitrary. If the response write is slow, the process may exit before the client receives it.
Consider using a callback on the socket write to ensure the response is flushed:
// Send success response before shutting down - sendSuccess(socket, id, { success: true }); + socket.write(`${JSON.stringify({ id, ok: true, payload: { success: true } })}\n`, () => { + // Kill sessions if requested + if (request.killSessions) { + terminalHost.killAll({ deleteHistory: false }); + } + stopServer().then(() => process.exit(0)); + }); - // Kill sessions if requested - if (request.killSessions) { - terminalHost.killAll({ deleteHistory: false }); - } - - // Schedule shutdown after a brief delay to allow response to be sent - setTimeout(() => { - stopServer().then(() => process.exit(0)); - }, 100);
445-471:isSocketLiveusesrequireinstead of import.Line 452 uses dynamic
require("node:net")whenSocketis already imported at the top of the file.🔎 Proposed fix
- const testSocket = new (require("node:net").Socket)(); + const { Socket: NetSocket } = await import("node:net"); + const testSocket = new NetSocket();Or since
Socketis already imported, use it directly (though you'd need theSocketclass, not the type):+import { createServer, type Server, Socket } from "node:net"; // ... - const testSocket = new (require("node:net").Socket)(); + const testSocket = new Socket();apps/desktop/src/main/lib/terminal/index.ts (1)
74-83: Console log on everygetActiveTerminalManagercall may be noisy.Line 78 logs every time this function is called, which could happen frequently. Consider moving the log to only fire when the cached value is first determined.
🔎 Reduce logging noise
export function getActiveTerminalManager(): | TerminalManager | DaemonTerminalManager { const daemonEnabled = isDaemonModeEnabled(); - console.log("[getActiveTerminalManager] Daemon mode enabled:", daemonEnabled); if (daemonEnabled) { return getDaemonTerminalManager(); } return terminalManager; }The mode is already logged in
isDaemonModeEnabled()when the cached value is set.apps/desktop/src/main/lib/terminal-host/client.ts (2)
63-79: Duplicate SUPERSET path constants across files.Same issue as in the daemon index.ts - these paths are defined in multiple places. As per coding guidelines, consider extracting to a shared constants module.
591-627: Daemon log file helps debugging but file descriptor may leak.Line 595 opens a file descriptor for logging but it's never explicitly closed. The
spawnwithdetached: trueshould handle this, but consider using a helper that manages the fd lifecycle.The fd is passed to the child and the child is unref'd, so the parent doesn't need to manage it. However, explicitly closing after spawn would be cleaner:
child.unref(); + + // Close log fd - child process has its own reference + if (logFd >= 0) { + try { + require("node:fs").closeSync(logFd); + } catch { + // Best effort + } + }apps/desktop/src/main/terminal-host/terminal-host.ts (1)
321-343: Recursive cleanup scheduling could run indefinitely.If clients never detach from a dead session,
scheduleSessionCleanupwill reschedule itself forever every 5 seconds. Consider adding a maximum retry count.🔎 Add cleanup retry limit
- private scheduleSessionCleanup(sessionId: string): void { + private scheduleSessionCleanup(sessionId: string, retryCount = 0): void { + const MAX_CLEANUP_RETRIES = 12; // 1 minute max + setTimeout(() => { const session = this.sessions.get(sessionId); if (!session || session.isAlive) { return; } if (session.clientCount === 0) { session.dispose(); this.sessions.delete(sessionId); + } else if (retryCount >= MAX_CLEANUP_RETRIES) { + console.warn( + `[TerminalHost] Force disposing session ${sessionId} after max retries (clients still attached: ${session.clientCount})`, + ); + session.dispose(); + this.sessions.delete(sessionId); } else { - this.scheduleSessionCleanup(sessionId); + this.scheduleSessionCleanup(sessionId, retryCount + 1); } }, 5000); }apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (2)
48-77: Consider extracting theCreateOrAttachResulttype to a shared location.This inline type duplicates structure that likely exists in the backend types (
apps/desktop/src/main/lib/terminal-host/types.ts). Consider importing from there or from a shared types file to maintain a single source of truth.
657-660: Verify!xtermRef.currentcondition doesn't cause silent failures.In
handleStreamData, when!xtermRef.currentor!isStreamReadyRef.current, events are queued. However, ifxtermRef.currentbecomes null permanently (e.g., after cleanup), events will accumulate inpendingEventsRefindefinitely without being processed or cleared.Consider adding a guard in the cleanup to clear pending events, or add a size limit to prevent unbounded growth.
🔎 Suggested addition to cleanup
return () => { isUnmounted = true; + // Clear pending events to prevent memory leak if component never remounts + pendingEventsRef.current = []; // ... rest of cleanupapps/desktop/src/main/terminal-host/session.ts (2)
161-252: Subprocess spawn logic has a potential issue with env filtering.The
processEnvis built by filteringprocess.env, but thenspawn()uses{ ...process.env, ELECTRON_RUN_AS_NODE: "1" }directly (line 195-198), ignoring the carefully filteredprocessEnv. The filtered env is only used inpendingSpawnwhich is sent to the subprocess later.This appears intentional (subprocess needs ELECTRON_RUN_AS_NODE to run as Node, while the PTY process spawned inside gets the filtered env), but the code flow is confusing. Consider adding a clarifying comment.
🔎 Suggested clarifying comment
+ // Note: We use process.env + ELECTRON_RUN_AS_NODE for the subprocess itself + // (it needs to run as Node.js). The filtered processEnv is passed via IPC + // to the PTY spawn inside the subprocess. this.subprocess = spawn(electronPath, [subprocessPath], { stdio: ["pipe", "pipe", "inherit"], // pipe stdin/stdout, inherit stderr env: { ...process.env, ELECTRON_RUN_AS_NODE: "1", }, });
808-848: Dispose method is thorough but has a minor issue.The
killTimer.unref()prevents the daemon from staying alive just for this timer, which is good. However, afterthis.subprocess = nullon line 825, thesubprocessreference captured in the closure (line 814) is still valid, so the SIGKILL fallback works correctly.One issue: if
sendDisposeToSubprocess()returns false (queue full), the subprocess might not receive the dispose command, but we still setthis.subprocess = null. The fallback SIGKILL after 1s should handle this, but consider logging a warning.🔎 Suggested warning
if (this.subprocess) { // Capture reference before nullifying - the timeout needs it const subprocess = this.subprocess; - this.sendDisposeToSubprocess(); + const disposeSent = this.sendDisposeToSubprocess(); + if (!disposeSent) { + console.warn(`[Session ${this.sessionId}] Failed to send dispose, will force kill`); + }apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
318-431: Paste handler with chunking and bracketed paste support is comprehensive.The implementation handles:
- Small pastes synchronously for performance
- Large pastes chunked with delays to prevent PTY overwhelm
- Bracketed paste mode wrapping per-chunk (correct for TUIs that defer repainting)
- Cancellation of in-flight pastes
- Newline normalization (
\r?\n→\r)One concern: the
CHUNK_DELAY_MS = 0in the direct write path (line 395) means chunks are processed viasetTimeout(..., 0)which still yields to the event loop but may be very fast. This might still overwhelm slow PTYs. Consider using a small non-zero delay like the fallback path's 5ms.🔎 Consider non-zero delay for direct write path
let offset = 0; const CHUNK_CHARS = 16_384; - const CHUNK_DELAY_MS = 0; + const CHUNK_DELAY_MS = 1; // Small delay to allow PTY to process
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
andreasasprou
left a comment
andreasasprou
left a comment
There was a problem hiding this comment.
Please search for all logs introduced in this pr and make sure they are appropriately retained
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@docs/CLOUD_WORKSPACE_PLAN.md`:
- Around line 642-690: The examples for CloudTerminal and the POST
/api/cloud-workspace/command lack validation, error handling, timeouts and
cleanup: validate workspaceId as a UUID before using it in CloudTerminal, build
the WebSocket URL with URLSearchParams, add ws.onerror and reconnection/close
handling and ensure term.dispose() runs on unmount; for the command POST use an
AbortController to enforce a timeout, wrap fetch in try/catch, check
response.ok, handle AbortError vs other errors, and document/implement
server-side command allowlisting/rate-limiting; reference CloudTerminal,
workspaceId, ws, AttachAddon, and the POST endpoint
(/api/cloud-workspace/command) when making these changes.
- Around line 692-739: The freestyleService wrapper needs production hardening:
wrap createVM, getSSHCredentials, and exec in try/catch (use logger.error on
failures), validate that tmux exists (run 'which tmux' and check exitCode) and
verify the tmux new-session result in createVM (freestyleService.createVM),
parameterize or namespace the tmux session name instead of a single "main" to
support multiple windows, implement caching and expiration for identities/tokens
(use a credentialCache keyed by vmId and ensure tokens are cleaned up or reused)
in getSSHCredentials (avoid creating identities every call), and add execution
guards in exec (freestyleService.exec) such as a Promise timeout, output size
limits, and proper error handling and logging; also consider adding simple rate
limiting or a request pool around freestyle.* API calls to prevent throttling.
- Around line 43-189: The architecture doc misses operational/reliability
details and has an outdated note about "terminal persistence" via tmux; add a
dedicated Operations & Reliability subsection covering failure modes (VM
unavailability, network failures, startup failures) and recovery behaviors,
cost/billing model (usage tracking, org limits, idle timeouts), data residency &
compliance (region choices, GDPR/CCPA handling), security posture (secret
management, multi-tenant threat model, SSH/access controls), capacity planning
(concurrent workspace limits, resource quotas, autoscaling strategy) and
degraded/local modes (how users can work offline or sync locally), and update
the terminal persistence decision to explicitly state whether tmux remains the
plan or the daemon-based solution from PR `#619` supersedes tmux, referencing the
"terminal persistence" line and PR `#619` so reviewers know which approach is
authoritative.
- Around line 620-640: The current connectCloudTerminal implementation passes a
connectionString containing the token as a process argument (visible to ps), so
update connectCloudTerminal (and trpc.cloudWorkspace.getSSHCredentials usage) to
return host, vmId and token separately, write the token to a temporary file with
restrictive permissions (0600), spawn ssh with the -i <tempKeyPath> flag and use
`${vmId}@${host}` instead of embedding the token in args, avoid logging the
token anywhere, and ensure the temp file is securely unlinked when the pty exits
(handle pty.onExit / process cleanup); also consider adding ssh options like
StrictHostKeyChecking=no and ensure tokens are short-lived or rotated by the
backend.
- Around line 398-430: The GET WebSocket handler lacks input validation,
authorization, error handling and operational controls; update the GET function
to validate workspaceId (e.g., ensure searchParams.get('workspaceId') is present
and isValidUUID), wrap freestyleService.getSSHCredentials and DB lookups
(db.query.cloudWorkspaces.findFirst) in try/catch, enforce authorization by
checking session.user.organizationId against workspace.organizationId after
auth(), return proper 400/401/404/500 responses, and add operational protections
(rate limiting/connection limits per user/workspace, input/output sanitization
for the SSH proxy, and graceful shutdown/cleanup for long-lived WebSocket
connections) while logging failures with logger.error including error and
workspaceId for observability.
- Around line 1-10: docs/CLOUD_WORKSPACE_PLAN.md is a large future-planning
document that does not belong in this terminal daemon fixes PR; move the entire
CLOUD_WORKSPACE_PLAN.md out of this branch and either (a) create a separate
documentation PR containing it, or (b) relocate the file into a dedicated
docs/planning/ or docs/rfcs/ directory and commit that separately, then update
this PR to remove the file so only the terminal kill-all reliability and UI
feedback changes remain.
- Around line 743-777: Replace the unscoped xterm addon packages listed under
"Dependencies to Add" (xterm-addon-fit, xterm-addon-attach) with their scoped
equivalents (`@xterm/addon-fit`, `@xterm/addon-attach`) and ensure you pin or verify
versions in package.json so they match the installed xterm version (xterm) per
xterm.js release notes; leave ws, ssh2, and freestyle-sandboxes unchanged and
add a brief note in the docs to verify addon/xterm version compatibility after
installation.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (1)
30-31: Handle the loading state forterminalPersistenceto prevent tab remounting.When
terminalPersistenceisundefined(during initial load), the component renders the non-persistence path (lines 165-188). Once the query resolves totrue, it switches to the persistence path (lines 87-162), causing terminal tabs to unmount and remount.Provide a stable default using
initialDataorplaceholderData:- const { data: terminalPersistence } = - trpc.settings.getTerminalPersistence.useQuery(); + const { data: terminalPersistence } = + trpc.settings.getTerminalPersistence.useQuery(undefined, { + initialData: false, // or import DEFAULT_TERMINAL_PERSISTENCE from shared/constants + });Alternatively, gate rendering on
isLoadingto prevent committing to either strategy until the setting is known.
🧹 Nitpick comments (15)
apps/desktop/src/main/lib/terminal/types.ts (1)
64-86: Consider extracting thesnapshottype to a named interface.The inline
snapshottype definition is quite large and deeply nested. Extracting it to a separate named interface (e.g.,DaemonSnapshotandDaemonSnapshotDebug) would improve readability and enable reuse if this structure appears elsewhere.♻️ Suggested refactor
// At module level, before SessionResult: export interface DaemonSnapshotDebug { xtermBufferType: string; hasAltScreenEntry: boolean; altBuffer?: { lines: number; nonEmptyLines: number; totalChars: number; cursorX: number; cursorY: number; sampleLines: string[]; }; normalBufferLines: number; } export interface DaemonSnapshot { snapshotAnsi: string; rehydrateSequences: string; cwd: string | null; modes: Record<string, boolean>; cols: number; rows: number; scrollbackLines: number; debug?: DaemonSnapshotDebug; } // Then in SessionResult: export interface SessionResult { // ... other fields ... snapshot?: DaemonSnapshot; }apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (1)
147-159: Consider extracting the sidebar to reduce duplication.The sidebar JSX (lines 147-159 and 174-186) is identical in both rendering branches. Extracting it to a shared fragment or restructuring the component to render the sidebar outside the conditional would reduce maintenance burden.
♻️ Optional refactor to extract sidebar
const sidebarElement = isSidebarOpen && ( <ResizablePanel width={sidebarWidth} onWidthChange={setSidebarWidth} isResizing={isResizing} onResizingChange={setIsResizing} minWidth={MIN_SIDEBAR_WIDTH} maxWidth={MAX_SIDEBAR_WIDTH} handleSide="left" > <Sidebar /> </ResizablePanel> ); // Then use {sidebarElement} in both branchesapps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsx (4)
27-40: Potential unnecessary re-computation ofsessionsSorted.The
sessionsvariable at line 29 creates a new array reference on every render whendaemonSessions?.sessionsis nullish (due to?? []). This causesuseMemoto re-run even when the underlying data hasn't changed.♻️ Suggested improvement
+const EMPTY_SESSIONS: typeof sessions = []; + function TerminalSettingsPage() { const utils = trpc.useUtils(); const { data: terminalPersistence, isLoading } = trpc.settings.getTerminalPersistence.useQuery(); const { data: daemonSessions } = trpc.terminal.listDaemonSessions.useQuery(); const daemonModeEnabled = daemonSessions?.daemonModeEnabled ?? false; - const sessions = daemonSessions?.sessions ?? []; + const sessions = daemonSessions?.sessions ?? EMPTY_SESSIONS;
125-127: Extract magic number to a named constant.The 300ms delay should be extracted to a descriptive constant at module level for clarity and maintainability.
♻️ Suggested fix
Add at top of file:
const DAEMON_CLEANUP_DELAY_MS = 300;Then update:
- setTimeout(() => { - utils.terminal.listDaemonSessions.invalidate(); - }, 300); + setTimeout(() => { + utils.terminal.listDaemonSessions.invalidate(); + }, DAEMON_CLEANUP_DELAY_MS);
137-141: Consider adding console logging for mutation errors.Per coding guidelines, errors should not be swallowed silently—at minimum log them with context. While the toast shows the error to users, adding a console log with the
[terminal/settings]prefix aids debugging.♻️ Suggested improvement
onError: (error) => { + console.error("[terminal/settings] Failed to clear terminal history:", error); toast.error("Failed to clear terminal history", { description: error.message, }); },Apply similar pattern to
killDaemonSession.onError(line 149-153).
217-223: Extract session count threshold to a named constant.The threshold of 20 sessions for showing the performance warning should be a named constant at module level.
♻️ Suggested fix
Add at top of file:
const SESSION_WARNING_THRESHOLD = 20;Then update:
- {sessions.length >= 20 && ( + {sessions.length >= SESSION_WARNING_THRESHOLD && (apps/desktop/docs/HANDOFF-terminal-cold-restore-bug.md (1)
18-25: Add language specifier to fenced code block.This code block and the one at line 67 (architecture diagram) are missing language identifiers. Use
textorlogfor log output andtextfor ASCII diagrams to satisfy markdown linting.Suggested fix
-``` +```log [DaemonTerminalManager] Received data from daemon: paneId=..., bytes=211, listeners=1 # Working!And for line 67:
-``` +```text Renderer (Terminal.tsx)apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (3)
142-153: Minor: Duplicate error logging when DEBUG_TERMINAL is enabled.When
DEBUG_TERMINALis true, errors are logged twice - once viaconsole.warn(line 144) and once viaconsole.error(line 151). Consider consolidating to a single log or removing the unconditionalconsole.errorsince the debug log already captures the error.Suggested consolidation
} catch (error) { - if (DEBUG_TERMINAL) { - console.warn("[Terminal Router] createOrAttach failed:", { - callId, - paneId, - durationMs: Date.now() - startedAt, - error: error instanceof Error ? error.message : String(error), - }); - } - console.error("[Terminal Router] createOrAttach ERROR:", error); + console.error("[Terminal Router] createOrAttach failed:", { + callId, + paneId, + durationMs: Date.now() - startedAt, + error, + }); throw error; }
288-307: Extract magic numbers to module-level constants.Per coding guidelines, extract magic numbers to named constants. The retry configuration values (10, 100) are used only here, but naming them improves readability.
Suggested refactor
At module top (around line 14):
const KILL_SESSION_MAX_RETRIES = 10; const KILL_SESSION_RETRY_DELAY_MS = 100;Then in the function:
- const MAX_RETRIES = 10; - const RETRY_DELAY_MS = 100; + const MAX_RETRIES = KILL_SESSION_MAX_RETRIES; + const RETRY_DELAY_MS = KILL_SESSION_RETRY_DELAY_MS;
335-339: Consider parallel session kills with error resilience.Sequential kills could be slow with many sessions, and a single failure would abort remaining kills. Using
Promise.allSettledwould be more resilient.Suggested improvement
- for (const session of toKill) { - await terminal.kill({ paneId: session.sessionId }); - } + const results = await Promise.allSettled( + toKill.map((session) => terminal.kill({ paneId: session.sessionId })), + ); + const killedCount = results.filter((r) => r.status === "fulfilled").length; - return { daemonModeEnabled: true, killedCount: toKill.length }; + return { daemonModeEnabled: true, killedCount };apps/desktop/src/main/lib/terminal/manager.ts (3)
112-167: Enhanced exit handler looks good, but consider extracting magic number.The diagnostic logging (shell, exitCode, signal, sessionDuration, cwd) is valuable for debugging. The
writeQueue.dispose()is correctly placed after marking the session as not alive.Per coding guidelines, consider extracting the cleanup delay to a named constant for clarity.
Suggested improvement
+const SESSION_CLEANUP_DELAY_MS = 5000; + // Near top of file with other constantsThen at line 163:
- const timeout = setTimeout(() => { - this.sessions.delete(paneId); - }, 5000); + const timeout = setTimeout(() => { + this.sessions.delete(paneId); + }, SESSION_CLEANUP_DELAY_MS);
262-272: Consider prefixing unused parameter with underscore for consistency.The
viewportYparameter is added for API compatibility with the daemon manager but is unused in non-daemon mode. For consistency withackColdRestore(line 188), consider either prefixing it with an underscore or destructuring and ignoring it explicitly.Suggested improvement
- detach(params: { paneId: string; viewportY?: number }): void { - const { paneId } = params; + detach(params: { paneId: string; viewportY?: number }): void { + const { paneId, viewportY: _viewportY } = params;Or alternatively, add a comment explaining the parameter is for API compatibility.
332-397: WriteQueue disposal is correctly placed; consider extracting timeout constants.The
writeQueue.dispose()calls are correctly positioned to prevent further writes before killing sessions. The escalation strategy (SIGTERM → SIGKILL → force cleanup) is sound.Per coding guidelines, consider extracting the timeout magic numbers to named constants for clarity and maintainability.
Suggested improvement
+const SIGTERM_TIMEOUT_MS = 2000; +const SIGKILL_TIMEOUT_MS = 500; + // Near top of file with other constantsdocs/CLOUD_WORKSPACE_PLAN.md (2)
501-600: Enhance implementation phases with rollback strategy and observability.The phased approach is logical, but several production concerns are missing:
Missing phases:
- Observability: No phase for metrics, logging, alerting, or dashboards to monitor VM health, costs, and usage.
- Cost tracking: Phase 5 adds command API but nowhere do you implement cost tracking/billing mentioned in open questions.
Rollback strategy:
- Line 517 (Phase 1.7): "Run migration" with
bun run db:push—no mention of backup, rollback plan, or migration validation.- Each phase should define rollback procedures if verification fails.
Feature flag strategy:
- No mention of feature flags to enable gradual rollout per organization or user group.
- Launching cloud workspaces globally could be risky without controlled rollout.
Phase dependencies:
- Phase 2 (Web Terminal) and Phase 3 (Desktop Integration) appear independent—clarify if they can run in parallel or have dependencies.
Consider adding:
- Phase 0: Observability foundation (metrics, logs, alerts)
- Phase 6: Monitoring dashboards and cost tracking UI
- Phase 7: Load testing and performance optimization
45-45: Add language specifiers to fenced code blocks.Multiple fenced code blocks are missing language specifiers, which affects syntax highlighting and rendering in some markdown viewers. Based on the static analysis hints, add specifiers to improve documentation quality:
- Line 45: ASCII diagram →
```text- Lines 104, 115, 125, 135, 156: User flow descriptions →
```textor```markdown- Line 436: File tree structure →
```textExample fix for line 45:
-``` +```text ┌─────────────────────────────────────┐Also applies to: 104-104, 115-115, 125-125, 135-135, 156-156, 436-436
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/desktop/docs/HANDOFF-terminal-cold-restore-bug.mdapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/index.tsapps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/components/GeneralSettings/GeneralSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/stores/tabs/useAgentHookListener.tsdocs/CLOUD_WORKSPACE_PLAN.md
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsx
🧰 Additional context used
📓 Path-based instructions (7)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/components/GeneralSettings/GeneralSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsxapps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/main/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/lib/trpc/routers/terminal/terminal.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/components/GeneralSettings/GeneralSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsxapps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/main/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/lib/trpc/routers/terminal/terminal.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Files:
apps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/components/GeneralSettings/GeneralSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
One component per file - do not create multi-component files
Files:
apps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/components/GeneralSettings/GeneralSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/components/GeneralSettings/GeneralSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsxapps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/main/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/lib/trpc/routers/terminal/terminal.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/components/GeneralSettings/GeneralSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsxapps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/main/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/lib/trpc/routers/terminal/terminal.ts
apps/desktop/src/main/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
Load environment variables from monorepo root .env in desktop app with override: true before any imports in src/main/index.ts and electron.vite.config.ts
Files:
apps/desktop/src/main/index.ts
🧠 Learnings (4)
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/lib/electron-router-dom.ts : Do not import Node.js modules like node:path or dotenv in electron-router-dom.ts and similar shared files - they run in both main and renderer processes
Applied to files:
apps/desktop/src/lib/trpc/routers/projects/projects.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/lib/trpc/routers/projects/projects.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from `trpc/server/observable` instead of async generators, as the library explicitly checks `isObservable(result)` and throws an error otherwise
Applied to files:
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
🧬 Code graph analysis (3)
apps/desktop/src/main/lib/terminal/types.ts (1)
apps/desktop/src/main/lib/terminal/pty-write-queue.ts (1)
PtyWriteQueue(29-159)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (2)
apps/desktop/src/main/lib/workspace-runtime/index.ts (1)
getWorkspaceRuntimeRegistry(18-18)apps/desktop/src/main/lib/workspace-runtime/registry.ts (1)
getWorkspaceRuntimeRegistry(75-80)
apps/desktop/src/main/index.ts (3)
apps/desktop/src/main/lib/workspace-runtime/index.ts (1)
getWorkspaceRuntimeRegistry(18-18)apps/desktop/src/main/lib/workspace-runtime/registry.ts (1)
getWorkspaceRuntimeRegistry(75-80)apps/desktop/src/main/lib/terminal/index.ts (2)
reconcileDaemonSessions(101-116)shutdownOrphanedDaemon(125-152)
🪛 LanguageTool
apps/desktop/docs/HANDOFF-terminal-cold-restore-bug.md
[style] ~123-~123: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... its own subscription management. 5. **Is React Query tearing down the subscripti...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~180-~180: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ns 5. React component unmounting - Maybe the Terminal component is being unmount...
(REP_MAYBE)
🪛 markdownlint-cli2 (0.18.1)
docs/CLOUD_WORKSPACE_PLAN.md
45-45: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
104-104: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
115-115: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
125-125: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
135-135: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
156-156: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
436-436: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
apps/desktop/docs/HANDOFF-terminal-cold-restore-bug.md
18-18: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
67-67: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Web
- GitHub Check: Deploy Admin
- GitHub Check: Deploy API
- GitHub Check: Build
- GitHub Check: Typecheck
🔇 Additional comments (35)
apps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/components/GeneralSettings/GeneralSettings.tsx (1)
8-8: LGTM!The new Terminal settings entry is cleanly integrated following the existing patterns. The import, route type extension, and section configuration are all consistent with the other settings entries.
Also applies to: 21-22, 64-68
apps/desktop/src/main/lib/terminal/types.ts (3)
1-5: LGTM!The imports are well-organized with proper
typequalifiers for type-only imports, following TypeScript best practices.
20-21: LGTM!The
writeQueuefield is properly typed and documented. Making it required ensures all terminal sessions have proper backpressure handling for large writes.
100-101: LGTM!The
skipColdRestoreflag is well-documented and serves a clear purpose in preventing re-triggering of cold restore detection when resuming after a cold restore.apps/desktop/src/lib/trpc/routers/projects/projects.ts (2)
16-16: LGTM!Import correctly uses the path alias and aligns with the workspace runtime registry pattern introduced in this PR.
781-785: Code is type-safe; no defensive handling needed.The registry returns a
WorkspaceRuntimewith a guaranteedterminalproperty. The type system ensuresgetForWorkspaceId()never returns an object without a terminal member—it's a required, non-null property defined in theWorkspaceRuntimeinterface. The pattern is correct as written.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (5)
17-26: Well-structured constant and helper function.Good extraction of
WARM_TERMINAL_TAB_LIMITas a named constant and clear JSDoc on the helper. ThehasTerminalPanefunction is pure and straightforward.
44-54: Good fallback logic for hydration edge case.The fallback chain (
activeTabIds[workspaceId]→ first workspace tab →null) prevents blank renders during store hydration.
66-79: MRU tracking implementation looks correct.The effect properly maintains bounded MRU ordering. Stale tab IDs from deleted tabs are filtered out during render (lines 92-94), so no cleanup effect is strictly necessary.
117-133: Visibility toggle approach is appropriate for xterm persistence.Using
visibility: hiddeninstead ofdisplay: nonecorrectly preserves xterm's measured dimensions, avoiding resize/redraw issues on tab switch. TheisTabVisibleprop enables components to optimize behavior when hidden.
164-172: Original behavior preserved correctly.The fallback path maintains the existing single-active-tab rendering when persistence is disabled, ensuring backward compatibility.
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsx (8)
1-16: LGTM!Imports are well-organized and follow the coding guidelines—using tRPC from the renderer lib path, alias imports, and no Node.js modules in the renderer process.
18-20: LGTM!Route definition follows the file-based routing convention correctly.
42-48: LGTM!Local state management is appropriate here—zustand would be overkill for these ephemeral dialog states.
156-159: LGTM!Simple timestamp formatting that handles edge cases appropriately for this settings UI context.
161-198: LGTM on the persistence toggle section.Good accessibility with proper label-switch association via
htmlFor/id. The disabled state correctly accounts for both loading and pending mutation states.
326-369: LGTM on kill-all confirmation dialog.Proper confirmation flow with clear messaging about the destructive action. Dialog closes before mutation executes to prevent double-click issues.
371-414: LGTM on clear history confirmation dialog.Clear explanation of what will be cleared and its implications. Good UX to inform users that running processes continue.
453-458: No issues found. The parameter naming is correct.The tRPC
killmutation schema confirms the parameter should be namedpaneId(notsessionId). The code correctly callskillDaemonSession.mutate({ paneId: sessionId }), wheresessionIdis a local variable holding the value that gets passed as thepaneIdparameter. This is an appropriate pattern—extracting the value before clearing state and passing it with the correct parameter name.apps/desktop/docs/HANDOFF-terminal-cold-restore-bug.md (1)
1-192: Excellent debugging documentation.This handoff document is well-structured with clear problem description, root cause analysis, architecture overview, and reproduction steps. It will be valuable for future debugging of similar issues.
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (5)
163-183: Good error handling for write failures.The error handling properly distinguishes between session-not-found (emits synthetic exit) and other errors (emits structured error event). This prevents toast floods when workspaces are deleted and terminals are orphaned.
370-384: Consistent null returns for not-found cases.Good change to return
nullconsistently instead of mixingundefinedandnull. This improves type predictability for callers.
460-467: Critical fix: Stream subscription stays open across exit events.This is the key fix documented in the handoff. By not calling
emit.complete()on exit, the subscription remains active for the stablepaneId, allowing data to flow after session recovery. The comment clearly explains the rationale.
481-495: Proper event listener cleanup.The cleanup function correctly removes all four event listeners that were registered, preventing memory leaks. Good symmetry between
on()andoff()calls.
31-40: This pattern is intentional and correct — no changes needed.The
terminalinstance is captured once at router creation time because bothgetWorkspaceRuntimeRegistry()andgetDefault()return cached singletons for the lifetime of the Electron process. The registry explicitly cacheslocalRuntimeand only initializes it once. A daemon restart would terminate the entire process (creating a new singleton), and mode toggles don't occur dynamically at runtime. This stable reference pattern is consistent throughout the codebase (main.ts, index.ts) and is the intended design.apps/desktop/src/main/index.ts (3)
18-22: LGTM!The new imports correctly align with the terminal persistence architecture, bringing in the daemon session management functions and the workspace-runtime registry for terminal lifecycle management.
177-180: LGTM!The parallel cleanup via
Promise.allis appropriate since terminal and analytics shutdown are independent operations. The workspace-runtime registry abstraction correctly decouples the cleanup path from the specific terminal backend implementation.
220-227: LGTM!The sequential calls are correctly ordered -
reconcileDaemonSessions()early-returns if daemon mode is disabled, whileshutdownOrphanedDaemon()early-returns if daemon mode is enabled. These are mutually exclusive cleanup paths that appropriately handle both persistence-enabled and persistence-disabled scenarios before the renderer restore runs.apps/desktop/src/main/lib/terminal/manager.ts (6)
170-182: LGTM!The write method correctly uses the write queue with proper error handling when the queue is full. Throwing an exception provides a clear signal to callers about backpressure, which is appropriate for preventing write backlogs.
184-190: LGTM!The
ackColdRestoremethod correctly serves as a no-op in non-daemon mode while maintaining interface compatibility with the daemon manager. The underscore prefix on_paneIdproperly indicates the unused parameter, and the JSDoc clearly explains the design rationale.
399-403: LGTM!The async signature maintains interface compatibility with the daemon manager while the synchronous implementation remains performant. This allows seamless switching between daemon and non-daemon modes without caller changes.
409-422: LGTM!The method correctly uses the write queue with proper error handling via try-catch. Since
writeQueue.write()can throw when the queue is full, the existing error handling prevents prompt refresh failures from affecting other terminals in the workspace.
424-437: LGTM!The
detachAllListenersmethod correctly includes the newterminalExitevent in the cleanup list, ensuring proper cleanup of the broadcast event listeners added at line 160.
439-473: LGTM!The cleanup method correctly disposes write queues for both alive and non-alive sessions before termination. The exit promise pattern with timeout ensures cleanup doesn't hang indefinitely.
docs/CLOUD_WORKSPACE_PLAN.md (1)
433-499: LGTM: File structure is well-organized.The proposed file structure follows good separation of concerns and aligns with the architecture. The modular approach with separate procedure files and clear client/server boundaries will aid maintainability.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| ## Architecture | ||
|
|
||
| ``` | ||
| ┌─────────────────────────────────────┐ | ||
| │ GitHub │ | ||
| │ (Persistent Code Storage) │ | ||
| └────────────────┬────────────────────┘ | ||
| │ | ||
| git clone | ||
| git push | ||
| │ | ||
| ▼ | ||
| ┌─────────────────────────────────────────────────────────────────────────┐ | ||
| │ CLOUD WORKSPACE │ | ||
| │ ┌───────────────────────────────────────────────────────────────────┐ │ | ||
| │ │ Freestyle VM │ │ | ||
| │ │ │ │ | ||
| │ │ /workspace tmux session │ │ | ||
| │ │ ├── .git ├── window 0: shell │ │ | ||
| │ │ ├── src/ └── window 1: dev server │ │ | ||
| │ │ └── ... │ │ | ||
| │ │ │ │ | ||
| │ │ SOURCE OF TRUTH for active development │ │ | ||
| │ └───────────────────────────────────────────────────────────────────┘ │ | ||
| │ │ │ | ||
| │ SSH Access │ | ||
| │ │ │ | ||
| └────────────────────────────────────┼─────────────────────────────────────┘ | ||
| │ | ||
| ┌────────────────────────┼────────────────────────┐ | ||
| │ │ │ | ||
| ┌────┴────┐ ┌────┴────┐ ┌────┴────┐ | ||
| │ Desktop │ │ Web │ │ Phone │ | ||
| │ │ │ │ │ │ | ||
| │ SSH term│ │ xterm.js│ │ Command │ | ||
| │ +local │ │ via WS │ │ API │ | ||
| │ sync? │ │ │ │ │ | ||
| └─────────┘ └─────────┘ └─────────┘ | ||
| ``` | ||
|
|
||
| ### Cloud is Source of Truth | ||
|
|
||
| - **Files live on cloud VM** - Edits happen directly on the VM | ||
| - **GitHub is persistent storage** - Code is pushed/pulled via standard git | ||
| - **Clients connect to cloud** - No local file copies required (optional for desktop) | ||
|
|
||
| ### Client Capabilities | ||
|
|
||
| | Client | Terminal | File Edit | Sync | | ||
| |--------|----------|-----------|------| | ||
| | **Desktop** | SSH (node-pty) | Cloud or Local+Sync | Optional | | ||
| | **Web** | xterm.js → WebSocket → SSH | Cloud (vim/nano) | N/A | | ||
| | **Phone** | View-only | None | N/A | | ||
| | **API** | Send commands | None | N/A | | ||
|
|
||
| --- | ||
|
|
||
| ## User Flows | ||
|
|
||
| ### Flow 1: Create Cloud Workspace from Desktop | ||
|
|
||
| ``` | ||
| 1. User clicks "New Cloud Workspace" in sidebar | ||
| 2. Selects repository and branch | ||
| 3. System creates Freestyle VM, clones from GitHub | ||
| 4. VM initializes (install deps, start tmux) | ||
| 5. Desktop connects via SSH, shows terminal | ||
| 6. User works directly on cloud | ||
| ``` | ||
|
|
||
| ### Flow 2: Access from Web | ||
|
|
||
| ``` | ||
| 1. User opens web app, sees cloud workspace in list | ||
| 2. Clicks workspace to open | ||
| 3. Web terminal (xterm.js) connects via WebSocket to API | ||
| 4. API proxies to SSH on Freestyle VM | ||
| 5. User has full terminal access in browser | ||
| ``` | ||
|
|
||
| ### Flow 3: Send Command from Phone | ||
|
|
||
| ``` | ||
| 1. User opens mobile app (or uses API/chat) | ||
| 2. Sees list of running cloud workspaces | ||
| 3. Sends command: "npm test" | ||
| 4. API executes on VM, returns output | ||
| 5. User sees test results | ||
| ``` | ||
|
|
||
| ### Flow 4: Handoff (Laptop → Phone → Web) | ||
|
|
||
| ``` | ||
| [Laptop] | ||
| 1. Working on cloud workspace via desktop app | ||
| 2. Close laptop, walk away | ||
| 3. Cloud workspace continues running (or pauses after timeout) | ||
|
|
||
| [Phone] | ||
| 4. Open app, see workspace status | ||
| 5. Send command: /run "git status" | ||
| 6. Chat with AI: "Fix the failing test" | ||
| 7. AI edits files on cloud, pushes to GitHub | ||
|
|
||
| [Web - later] | ||
| 8. Open web app on another computer | ||
| 9. Connect to same cloud workspace | ||
| 10. See all changes made via phone/AI | ||
| 11. Continue working | ||
| ``` | ||
|
|
||
| ### Flow 5: Desktop with Local Sync (for IDE users) | ||
|
|
||
| ``` | ||
| 1. Create cloud workspace | ||
| 2. Click "Sync to Local" in desktop app | ||
| 3. System creates local worktree, clones from GitHub | ||
| 4. Edit files in VS Code/Cursor locally | ||
| 5. Commit + push to GitHub | ||
| 6. Cloud VM auto-pulls (or manual trigger) | ||
| 7. Terminal on cloud sees updated files | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Key Decisions | ||
|
|
||
| | Decision | Choice | Rationale | | ||
| |----------|--------|-----------| | ||
| | **Cloud Provider** | Freestyle.dev | Sub-second VM startup, built-in Git, SSH support | | ||
| | **Source of Truth** | Cloud VM | Simplifies multi-device access, no sync conflicts | | ||
| | **Persistent Storage** | GitHub | Standard git workflow, PRs, code review | | ||
| | **Sync Mechanism** | Git push/pull | No new tools, familiar workflow | | ||
| | **Desktop Local Sync** | Optional | For IDE users who prefer local editing | | ||
| | **Multi-device Model** | Shared access | Multiple clients can connect simultaneously | | ||
| | **Terminal Persistence** | tmux | Sessions survive disconnections | | ||
| | **Web Terminal** | xterm.js + WebSocket | Standard, well-supported | | ||
|
|
||
| ### Rejected Alternatives | ||
|
|
||
| | Alternative | Why Rejected | | ||
| |-------------|--------------| | ||
| | **Mutagen file sync** | Added complexity, not needed with cloud-centric model | | ||
| | **CRDT collaboration** | Overkill for file-based editing, complex integration | | ||
| | **Freestyle Git** | GitHub already serves as source of truth | | ||
| | **Handoff model (one writer)** | Shared access is more flexible | | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add operational and reliability considerations to the architecture.
The architecture section focuses on the happy path but lacks discussion of:
- Failure modes: What happens when the Freestyle VM is unavailable, experiences network issues, or fails to start?
- Cost model: How will VM usage be tracked, billed, and limited per organization?
- Data residency & compliance: Where do VMs run? GDPR/CCPA implications for code and data?
- Security posture: How are secrets managed? What's the threat model for multi-tenant VMs?
- Capacity planning: Max concurrent workspaces per org, VM resource limits, scaling strategy?
- Degraded modes: Can users work locally if cloud is unavailable?
Additionally, Line 178 states tmux will provide "terminal persistence," but PR #619 implements a daemon-based solution instead—clarify if tmux is still planned or if the daemon approach supersedes it.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
45-45: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
104-104: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
115-115: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
125-125: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
135-135: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
156-156: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@docs/CLOUD_WORKSPACE_PLAN.md` around lines 43 - 189, The architecture doc
misses operational/reliability details and has an outdated note about "terminal
persistence" via tmux; add a dedicated Operations & Reliability subsection
covering failure modes (VM unavailability, network failures, startup failures)
and recovery behaviors, cost/billing model (usage tracking, org limits, idle
timeouts), data residency & compliance (region choices, GDPR/CCPA handling),
security posture (secret management, multi-tenant threat model, SSH/access
controls), capacity planning (concurrent workspace limits, resource quotas,
autoscaling strategy) and degraded/local modes (how users can work offline or
sync locally), and update the terminal persistence decision to explicitly state
whether tmux remains the plan or the daemon-based solution from PR `#619`
supersedes tmux, referencing the "terminal persistence" line and PR `#619` so
reviewers know which approach is authoritative.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (1)
33-34: **Handle the loading state forterminalPersistenceto prevent rendering strategy flip.**WhenterminalPersistenceisundefined(during initial query load), the component takes the non-persistence code path (lines 173-196), then switches to the persistence path (lines 95-170) once data arrives. InitialData works on cache level, while placeholderData works on observer level. This has a couple of implications: First of all, initialData is persisted to the cache.Since
terminalPersistenceis a user setting with a known default, useinitialDatato provide an immediate stable value and prevent the rendering strategy flip:🐛 Proposed fix
+import { DEFAULT_TERMINAL_PERSISTENCE } from "@superset/shared/constants"; // or wherever the default is defined const { data: terminalPersistence } = - electronTrpc.settings.getTerminalPersistence.useQuery(); + electronTrpc.settings.getTerminalPersistence.useQuery(undefined, { + initialData: DEFAULT_TERMINAL_PERSISTENCE, + });Alternatively, destructure
isLoadingand defer the conditional branch until data is available:-const { data: terminalPersistence } = +const { data: terminalPersistence, isLoading: isLoadingPersistence } = electronTrpc.settings.getTerminalPersistence.useQuery(); +// Early return while loading to avoid strategy flip +if (isLoadingPersistence) { + return null; // or a minimal loading skeleton +}apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (2)
352-405: Extract paste chunking constants to module level.Per coding guidelines, magic numbers should be extracted to named constants at module top. The paste chunking values are defined inline with different values for different code paths:
MAX_SYNC_PASTE_CHARS = 16_384(line 352)CHUNK_CHARS = 4096for xterm.paste path (line 357)CHUNK_CHARS = 16_384for direct write path (line 404, shadows the first)The shadowing and different chunk sizes are confusing. Consider extracting with descriptive names like
XTERM_PASTE_CHUNK_SIZEandDIRECT_WRITE_CHUNK_SIZE.
87-156:TerminalRenderer.kindbecomes stale after WebGL context loss.The
onContextLosscallback updates the localkindvariable, but the returnedTerminalRendererobject captureskindby value at return time. After context loss, consumers checkingrenderer.kindstill see"webgl"even though the actual renderer switched to canvas.While
clearTextureAtlassafely becomes a no-op (the capturedwebglAddonis nulled), the stalekindcauses incorrect renderer identification inTerminal.tsx(e.g., line 630).Consider returning a mutable object whose properties update in the
onContextLosshandler, as suggested in the previous review.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (2)
53-53: Use DOM-safe timeout typing instead ofNodeJS.Timeout.
pendingDetachesusesNodeJS.Timeout, which can cause type mismatches in DOM-typed renderer builds. UseReturnType<typeof setTimeout>for portability.Proposed fix
-const pendingDetaches = new Map<string, NodeJS.Timeout>(); +const pendingDetaches = new Map<string, ReturnType<typeof setTimeout>>();
1042-1047: Remove duplicate state initialization.Lines 1042-1044 and 1045-1047 are identical - the same three assignments appear twice consecutively:
isStreamReadyRef.current = falsedidFirstRenderRef.current = falsependingInitialStateRef.current = nullProposed fix
xtermRef.current = xterm; fitAddonRef.current = fitAddon; rendererRef.current = renderer; isExitedRef.current = false; setXtermInstance(xterm); isStreamReadyRef.current = false; didFirstRenderRef.current = false; pendingInitialStateRef.current = null; -isStreamReadyRef.current = false; -didFirstRenderRef.current = false; -pendingInitialStateRef.current = null;apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
36-44: Terminal runtime captured at router creation may become stale.The
terminalreference is captured once when the router is created. If the daemon restarts or terminal persistence mode is toggled at runtime, procedures may operate on a dead or outdated backend.Consider resolving the terminal runtime lazily inside each procedure, or ensure the registry/runtime handles reconnection transparently.
#!/bin/bash # Check how the workspace runtime registry handles runtime lifecycle ast-grep --pattern $'class $_ implements WorkspaceRuntimeRegistry { $$$ getDefault() { $$$ } $$$ }'
🧹 Nitpick comments (7)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (2)
22-29: Consider using object parameters for consistency.Per coding guidelines, functions with 2+ parameters should use a single params object. This is a minor suggestion for this simple helper.
♻️ Optional refactor
-function hasTerminalPane(tab: Tab, panes: Record<string, Pane>): boolean { +function hasTerminalPane({ tab, panes }: { tab: Tab; panes: Record<string, Pane> }): boolean { const paneIds = extractPaneIdsFromLayout(tab.layout); return paneIds.some((paneId) => panes[paneId]?.type === "terminal"); }Then update call sites to use
hasTerminalPane({ tab, panes }).
148-153: Simplify the fallback condition.The condition
!activeNonTerminalTab && !tabToRendercan be simplified to!tabToRendersinceactiveNonTerminalTabis derived fromtabToRenderand will always benullwhentabToRenderisnull.♻️ Optional simplification
- {/* Fallback: show empty view without unmounting terminal tabs */} - {!activeNonTerminalTab && !tabToRender && ( + {/* Fallback: show empty view when no active tab */} + {!tabToRender && ( <div className="absolute inset-0 overflow-hidden"> <EmptyTabView /> </div> )}apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (3)
406-472: Event handling logic is duplicated withhandleStreamData.The event processing logic in
flushPendingEvents(lines 416-471) is nearly identical tohandleStreamData(lines 894-952). Both implement the same special-case handling for "Session not found", "PTY not spawned",WRITE_QUEUE_FULL, etc.Consider extracting a shared
processTerminalEventhelper to reduce duplication and divergence risk.
586-593: Consider extracting the SIGWINCH delay to a named constant.The 100ms timeout at line 592 is a magic number used for ensuring SIGWINCH propagation. Per coding guidelines, consider extracting this to a named constant at module level for clarity and maintainability.
Suggested change
// At module level const SIGWINCH_PROPAGATION_DELAY_MS = 100; // In the code setTimeout(() => { // ... }, SIGWINCH_PROPAGATION_DELAY_MS);
1461-1493: Consider extracting StrictMode detach delay to a named constant.The 50ms timeout at line 1474 is a magic number used to handle React StrictMode's unmount/remount cycle. While the delay is well-documented in comments, extracting it to a constant improves maintainability:
const STRICT_MODE_DETACH_DELAY_MS = 50;The
setTimeout(0)forxterm.dispose()at line 1491-1493 is appropriate and well-documented.apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
313-314: Extract retry constants to module level.Per coding guidelines, magic numbers should be extracted to named constants at module top for clarity and reuse.
Suggested refactor
Add near the top of the file (after line 19):
const KILL_SESSIONS_MAX_RETRIES = 10; const KILL_SESSIONS_RETRY_DELAY_MS = 100;Then reference them in the procedure.
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsx (1)
125-131: Consider cleanup for delayed invalidation.The
setTimeoutinonSettledcould fire after unmount. While React Query handles stale invalidations gracefully, a cleanup pattern would be cleaner.Alternative approach
Use a ref to track mount state, or consider using React Query's built-in
refetchIntervalon the query itself with a short burst after mutations, avoiding manual setTimeout.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/desktop/electron.vite.config.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/index.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/components/GeneralSettings/GeneralSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalConnection.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/stores/hotkeys/store.tsapps/desktop/src/renderer/stores/tabs/store.tsapps/desktop/src/renderer/stores/tabs/useAgentHookListener.tsapps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.tsapps/desktop/src/shared/constants.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts
- apps/desktop/src/renderer/stores/hotkeys/store.ts
- apps/desktop/src/shared/constants.ts
- apps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.ts
- apps/desktop/electron.vite.config.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsx
- apps/desktop/src/renderer/stores/tabs/store.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalConnection.ts
🧰 Additional context used
📓 Path-based instructions (7)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/components/GeneralSettings/GeneralSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/main/index.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/components/GeneralSettings/GeneralSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/main/index.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/components/GeneralSettings/GeneralSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/main/index.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/components/GeneralSettings/GeneralSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/main/index.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/components/GeneralSettings/GeneralSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
One component per file - do not create multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxapps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/components/GeneralSettings/GeneralSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/desktop/src/main/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
Load environment variables from monorepo root .env in desktop app with override: true before any imports in src/main/index.ts and electron.vite.config.ts
Files:
apps/desktop/src/main/index.ts
🧠 Learnings (10)
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/lib/electron-router-dom.ts : Do not import Node.js modules like node:path or dotenv in electron-router-dom.ts and similar shared files - they run in both main and renderer processes
Applied to files:
apps/desktop/src/main/windows/main.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Applied to files:
apps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/main/index.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/lib/*.ts : Never import Node.js modules in shared code like electron-router-dom.ts - it runs in both main and renderer processes
Applied to files:
apps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables from monorepo root .env in desktop app with override: true before any imports in src/main/index.ts and electron.vite.config.ts
Applied to files:
apps/desktop/src/main/windows/main.tsapps/desktop/src/main/index.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/desktop/src/shared/ipc-channels.ts : Define IPC channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Applied to files:
apps/desktop/src/main/windows/main.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/lib/trpc/routers/terminal/terminal.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to **/*.{ts,tsx} : Extract magic numbers and hardcoded values to named constants at module top
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
📚 Learning: 2026-01-02T06:50:28.671Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.671Z
Learning: Applies to apps/api/**/*.{ts,tsx} : Extract business logic from tRPC procedures into utility functions when logic exceeds ~50 lines, is used by multiple procedures, or needs independent testing
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from `trpc/server/observable` instead of async generators, as the library explicitly checks `isObservable(result)` and throws an error otherwise
Applied to files:
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
🧬 Code graph analysis (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (4)
apps/desktop/src/renderer/stores/tabs/types.ts (2)
Tab(18-20)Pane(12-12)apps/desktop/src/renderer/stores/tabs/utils.ts (1)
extractPaneIdsFromLayout(97-108)apps/desktop/src/renderer/lib/electron-trpc.ts (1)
electronTrpc(9-11)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsx (1)
TabView(27-200)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalConnection.ts (1)
useTerminalConnection(20-69)apps/desktop/src/renderer/lib/terminal-kill-tracking.ts (2)
isTerminalKilledByUser(7-8)clearTerminalKilledByUser(10-12)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/utils.ts (2)
restoreScrollPosition(27-37)getScrollOffsetFromBottom(21-24)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/attach-scheduler.ts (1)
scheduleTerminalAttach(129-209)
apps/desktop/src/main/index.ts (3)
apps/desktop/src/main/lib/workspace-runtime/index.ts (1)
getWorkspaceRuntimeRegistry(18-18)apps/desktop/src/main/lib/workspace-runtime/registry.ts (1)
getWorkspaceRuntimeRegistry(75-80)apps/desktop/src/main/lib/terminal/index.ts (2)
reconcileDaemonSessions(101-116)shutdownOrphanedDaemon(125-152)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (6)
apps/desktop/src/main/lib/workspace-runtime/index.ts (1)
getWorkspaceRuntimeRegistry(18-18)apps/desktop/src/main/lib/workspace-runtime/registry.ts (1)
getWorkspaceRuntimeRegistry(75-80)apps/desktop/src/main/lib/terminal/manager.ts (1)
signal(231-244)apps/desktop/src/main/lib/terminal/daemon-manager.ts (1)
signal(847-865)apps/desktop/src/main/lib/terminal-host/client.ts (1)
signal(1273-1276)apps/desktop/src/main/terminal-host/terminal-host.ts (1)
signal(235-245)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Web
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy API
- GitHub Check: Build
🔇 Additional comments (20)
apps/desktop/src/main/index.ts (3)
20-24: LGTM!The new imports for daemon session management (
reconcileDaemonSessions,shutdownOrphanedDaemon) and workspace runtime registry align with the terminal persistence feature. The imports are from concrete module paths as recommended.
245-251: LGTM!The daemon session reconciliation and orphan cleanup are correctly positioned:
- After
initAppState()(settings/state available)- Before
ensureShellEnvVars()and window creation (as required by the comment)- Sequential execution ensures reconciliation completes before orphan shutdown
Both functions internally guard against errors and log warnings without throwing, so startup remains resilient even if daemon cleanup fails.
179-182: No changes needed—the property access chain is safe.The chained call
getWorkspaceRuntimeRegistry().getDefault().terminal.cleanup()is fully type-safe:
getWorkspaceRuntimeRegistry()returns a cached singletonWorkspaceRuntimeRegistry(never undefined).getDefault()lazy-initializes and always returns aWorkspaceRuntimeinstance (never undefined).terminalis a required non-optional property onWorkspaceRuntime(always defined inLocalWorkspaceRuntimeconstructor)The cleanup pattern is sound: the try/finally ensures
app.quit()executes even if cleanup fails.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (5)
20-20: LGTM!Good practice extracting
WARM_TERMINAL_TAB_LIMITas a named constant at module top per coding guidelines.
48-72: LGTM!The memoization chain for deriving
activeTabId,tabToRender, andactiveTabHasTerminalis well-structured with appropriate dependencies. The separation of concerns makes the logic easy to follow.
74-87: LGTM!The MRU tracking logic correctly maintains a bounded warm set of terminal tabs with proper guards and functional state updates to avoid stale closures.
125-140: LGTM!The visibility toggling logic correctly combines workspace and tab ID checks, ensuring terminals from inactive workspaces remain mounted but hidden. The
pointerEvents: "none"prevents accidental interaction with hidden terminals.
172-196: LGTM!The non-persistence fallback correctly preserves the original single-tab rendering behavior.
apps/desktop/src/main/windows/main.ts (3)
25-25: LGTM!The import correctly references the new workspace runtime registry abstraction, aligning with the PR's architecture for terminal persistence.
235-235: LGTM!The cleanup correctly uses the workspace runtime registry pattern, maintaining consistency with the event listener registration at lines 176-177.
173-187: LGTM!The terminal exit event forwarding is well-structured:
- Inline type annotation correctly matches the actual event shape emitted by terminal runtime
- Proper cleanup via
detachAllListeners()on window close (line 235)- Follows the existing event forwarding pattern established for
AGENT_LIFECYCLEevents- Events are correctly exposed to the renderer through tRPC subscription with the observable pattern
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
193-226: Well-designed async renderer loading with proper cleanup guards.The deferred GPU renderer loading via
requestAnimationFramewithisDisposedguard elegantly avoids the race condition described in the comments. The cleanup properly cancels pending RAF and disposes the current renderer via the ref pattern.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (2)
799-803: Good defensive handling of stale events during cold restore.Dropping queued events before starting a new shell prevents stale
exitevents from incorrectly marking the new session as exited. The comment clearly explains the rationale.
1554-1610: Well-structured overlay system for terminal states.The three overlay states (killed, connection error, cold restore) are cleanly separated with appropriate conditions preventing overlap. The UX flow is clear:
- Session killed → Restart Session
- Connection error → Retry Connection
- Cold restore → Start Shell (with preserved CWD display)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (3)
461-523: LGTM!The stream subscription correctly uses the
observablepattern required for trpc-electron, properly handles multiple event types, and includes clear documentation explaining whyemit.complete()is intentionally not called on exit to support session reuse.
184-203: LGTM!Good error handling that gracefully emits an exit event when the session is gone, preventing error floods when workspaces with terminals are deleted. The signal is correctly typed as a number.
76-89: LGTM!The
userKilledSessionstracking withallowKilledbypass provides a clean mechanism to prevent auto-reattach of intentionally killed sessions while allowing explicit recreation.apps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/components/GeneralSettings/GeneralSettings.tsx (1)
64-68: LGTM!The Terminal settings entry follows the established pattern and integrates cleanly with the existing navigation structure.
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsx (2)
164-201: LGTM!The Terminal persistence toggle with clear explanations about memory usage and restart requirements provides good UX. The optimistic update pattern with rollback on error is well-implemented.
329-375: LGTM!The Kill All confirmation dialog provides appropriate warnings about the destructive action, correctly marks all sessions as killed by user before triggering the mutation to prevent auto-reattach.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| @@ -139,6 +149,9 @@ export const settings = sqliteTable("settings", { | |||
| terminalLinkBehavior: text( | |||
| "terminal_link_behavior", | |||
| ).$type<TerminalLinkBehavior>(), | |||
| navigationStyle: text("navigation_style").$type<NavigationStyle>(), | |||
| groupTabsPosition: text("group_tabs_position").$type<GroupTabsPosition>(), | |||
| terminalPersistence: integer("terminal_persistence", { mode: "boolean" }), | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
remove old variables
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsx`:
- Around line 252-263: The Clear terminal history button is currently disabled
when there are no live sessions; remove the aliveSessions.length === 0 check so
the button is enabled whenever daemonModeEnabled is true (still respecting
clearTerminalHistory.isPending). Update the disabled expression used on the
Button rendered in page.tsx (the Button component with onClick={() =>
setConfirmClearHistoryOpen(true)}) to only consider !daemonModeEnabled and
clearTerminalHistory.isPending so users can clear disk-based scrollback even
with no live sessions.
- Around line 374-380: The click handler currently iterates over sessions and
marks every session as killed by user; change that loop to iterate over
aliveSessions instead so only sessions that will be terminated are marked via
markTerminalKilledByUser(session.sessionId). Keep the existing calls to
setConfirmKillAllOpen(false) and killAllDaemonSessions.mutate(), but replace the
for (const session of sessions) { ... } loop with for (const session of
aliveSessions) { ... } (or equivalent filtering of sessions to only alive ones)
to ensure only live terminals are flagged.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsx (1)
31-46: Unstable array reference causes unnecessary memo recalculations.The
?? []fallback creates a new array reference each render whendaemonSessions?.sessionsis undefined, defeating the memoization ofaliveSessionsandsessionsSorted.♻️ Proposed fix
+const EMPTY_SESSIONS: typeof daemonSessions.sessions = []; + function TerminalSettingsPage() { const utils = electronTrpc.useUtils(); // ... - const sessions = daemonSessions?.sessions ?? []; + const sessions = daemonSessions?.sessions ?? EMPTY_SESSIONS;Alternatively, fold the filtering into a single memo:
- const sessions = daemonSessions?.sessions ?? []; - const aliveSessions = useMemo( - () => sessions.filter((session) => session.isAlive), - [sessions], - ); - const sessionsSorted = useMemo(() => { - return [...aliveSessions].sort((a, b) => { + const sessionsSorted = useMemo(() => { + const alive = (daemonSessions?.sessions ?? []).filter((s) => s.isAlive); + return alive.sort((a, b) => { // ... }); - }, [aliveSessions]); + }, [daemonSessions?.sessions]);Note: This is a refinement of the prior review comment about the useMemo dependency issue.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsx (2)
128-134: Extract magic number and consider race condition.The 300ms delay is a hardcoded value that may not be sufficient if the daemon cleanup takes longer. Consider extracting to a named constant and documenting the assumption.
♻️ Proposed fix
+// Delay before refetching to allow daemon cleanup to complete +const DAEMON_CLEANUP_DELAY_MS = 300; + // ... inside mutation config onSettled: () => { // Always refetch to get actual state after mutation settles - // Small delay to allow daemon to finish cleanup setTimeout(() => { utils.terminal.listDaemonSessions.invalidate(); - }, 300); + }, DAEMON_CLEANUP_DELAY_MS); },
162-165: Consider moving pure utility outside the component.
formatTimestampis recreated each render. Since it has no dependencies on component state, extract it to module scope.♻️ Proposed fix
+const formatTimestamp = (value?: string) => { + if (!value) return "—"; + return value.replace("T", " ").replace(/\.\d+Z$/, "Z"); +}; + function TerminalSettingsPage() { // ... - const formatTimestamp = (value?: string) => { - if (!value) return "—"; - return value.replace("T", " ").replace(/\.\d+Z$/, "Z"); - };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsx
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Files:
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsx
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
One component per file - do not create multi-component files
Files:
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsx
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Deploy API
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Web
- GitHub Check: Build
🔇 Additional comments (2)
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsx (2)
1-21: LGTM!Imports and route definition follow project conventions. Proper use of aliases and no Node.js modules in renderer code.
433-482: LGTM!The per-session kill dialog handles null state defensively with proper early return. Dialog structure follows accessibility patterns.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| <Button | ||
| variant="secondary" | ||
| size="sm" | ||
| disabled={ | ||
| !daemonModeEnabled || | ||
| aliveSessions.length === 0 || | ||
| clearTerminalHistory.isPending | ||
| } | ||
| onClick={() => setConfirmClearHistoryOpen(true)} | ||
| > | ||
| Clear terminal history | ||
| </Button> |
There was a problem hiding this comment.
"Clear terminal history" should be available when daemon is enabled, regardless of live sessions.
Clearing history removes disk-based scrollback files used for cold restore. Users may want to clear old history even when no sessions are currently alive (e.g., after all sessions exited but before app restart).
🐛 Proposed fix
<Button
variant="secondary"
size="sm"
disabled={
!daemonModeEnabled ||
- aliveSessions.length === 0 ||
clearTerminalHistory.isPending
}
onClick={() => setConfirmClearHistoryOpen(true)}
>
Clear terminal history
</Button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Button | |
| variant="secondary" | |
| size="sm" | |
| disabled={ | |
| !daemonModeEnabled || | |
| aliveSessions.length === 0 || | |
| clearTerminalHistory.isPending | |
| } | |
| onClick={() => setConfirmClearHistoryOpen(true)} | |
| > | |
| Clear terminal history | |
| </Button> | |
| <Button | |
| variant="secondary" | |
| size="sm" | |
| disabled={ | |
| !daemonModeEnabled || | |
| clearTerminalHistory.isPending | |
| } | |
| onClick={() => setConfirmClearHistoryOpen(true)} | |
| > | |
| Clear terminal history | |
| </Button> |
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsx`
around lines 252 - 263, The Clear terminal history button is currently disabled
when there are no live sessions; remove the aliveSessions.length === 0 check so
the button is enabled whenever daemonModeEnabled is true (still respecting
clearTerminalHistory.isPending). Update the disabled expression used on the
Button rendered in page.tsx (the Button component with onClick={() =>
setConfirmClearHistoryOpen(true)}) to only consider !daemonModeEnabled and
clearTerminalHistory.isPending so users can clear disk-based scrollback even
with no live sessions.
| onClick={() => { | ||
| setConfirmKillAllOpen(false); | ||
| for (const session of sessions) { | ||
| markTerminalKilledByUser(session.sessionId); | ||
| } | ||
| killAllDaemonSessions.mutate(); | ||
| }} |
There was a problem hiding this comment.
Iterate over aliveSessions when marking sessions as killed.
The loop marks all sessions (including already-dead ones) as killed by user, but only alive sessions are actually terminated.
🐛 Proposed fix
onClick={() => {
setConfirmKillAllOpen(false);
- for (const session of sessions) {
+ for (const session of aliveSessions) {
markTerminalKilledByUser(session.sessionId);
}
killAllDaemonSessions.mutate();
}}🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsx`
around lines 374 - 380, The click handler currently iterates over sessions and
marks every session as killed by user; change that loop to iterate over
aliveSessions instead so only sessions that will be terminated are marked via
markTerminalKilledByUser(session.sessionId). Keep the existing calls to
setConfirmKillAllOpen(false) and killAllDaemonSessions.mutate(), but replace the
for (const session of sessions) { ... } loop with for (const session of
aliveSessions) { ... } (or equivalent filtering of sessions to only alive ones)
to ensure only live terminals are flagged.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
185-191: InconsistentSAFE_IDvalidation onpaneIdacross procedures.
createOrAttachvalidatespaneIdwithSAFE_ID(preventing path traversal), butwrite,resize,signal,kill,detach,clearScrollback,ackColdRestore,getSession, andstreamall accept plainz.string().If
paneIdis used to construct file paths (e.g., for history persistence in daemon mode), this inconsistency could expose path traversal vulnerabilities. ApplySAFE_IDconsistently to all paneId inputs.🔒 Example fix for write mutation
write: publicProcedure .input( z.object({ - paneId: z.string(), + paneId: SAFE_ID, data: z.string(), }), )
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/terminal/terminal.ts`:
- Around line 303-317: The code adds session IDs to userKilledSessions before
calling terminal.management.killAllSessions(), causing createOrAttach to treat
sessions as dead if killAll fails; change the logic so you only add IDs to
userKilledSessions after killAllSessions() completes successfully (or in a loop
that confirms each id was killed), or wrap the killAllSessions() call in
try/catch and on failure remove any previously added IDs from
userKilledSessions; reference terminal.management.killAllSessions(),
userKilledSessions, and createOrAttach to locate and update the flow so session
IDs are added only after confirmed kill (or rolled back on error).
♻️ Duplicate comments (1)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
43-52: Terminal reference captured once may become stale on daemon restart or mode toggle.The
terminalobject is resolved once at router creation. If the daemon restarts or terminal persistence mode is toggled, procedures will continue using the stale reference.Consider resolving the terminal runtime inside each procedure or introducing a getter helper.
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (2)
18-19: Unbounded growth ofuserKilledSessionsSet.This Set accumulates paneIds across the app lifetime and is never pruned. Over extended usage (especially with many workspace/terminal operations), this could grow unboundedly.
Consider periodic cleanup (e.g., removing entries when the corresponding workspace is deleted or on a TTL basis), or using a bounded LRU cache.
368-371: Sequential session kills could be parallelized.The loop awaits each
terminal.kill()sequentially. If multiple sessions need killing, this adds latency. Consider parallel execution.♻️ Proposed refactor
-for (const session of toKill) { - userKilledSessions.add(session.sessionId); - await terminal.kill({ paneId: session.sessionId }); -} +await Promise.all( + toKill.map((session) => { + userKilledSessions.add(session.sessionId); + return terminal.kill({ paneId: session.sessionId }); + }), +);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/lib/terminal-host/client.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/main/lib/terminal-host/client.ts
🧰 Additional context used
📓 Path-based instructions (4)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
🧠 Learnings (2)
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from `trpc/server/observable` instead of async generators, as the library explicitly checks `isObservable(result)` and throws an error otherwise
Applied to files:
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Admin
- GitHub Check: Deploy API
- GitHub Check: Deploy Web
- GitHub Check: Build
🔇 Additional comments (2)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (2)
54-183: Well-structured mutation with proper validation and error handling.Good use of
SAFE_IDvalidation, debug telemetry withcallIdtracking, and theallowKilledescape hatch for intentional session restarts.
469-531: LGTM!The observable pattern is correctly used for the tRPC subscription (per trpc-electron requirements). Event handlers are properly registered and cleaned up, and the comment explaining why
emit.complete()is avoided on exit is valuable documentation. Based on learnings about trpc-electron observable requirements.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // Get sessions before kill for accurate count | ||
| const before = await terminal.management.listSessions(); | ||
| const beforeIds = before.sessions.map((s) => s.sessionId); | ||
| for (const id of beforeIds) { | ||
| userKilledSessions.add(id); | ||
| } | ||
| console.log( | ||
| "[killAllDaemonSessions] Before kill:", | ||
| beforeIds.length, | ||
| "sessions", | ||
| beforeIds, | ||
| ); | ||
|
|
||
| // Request kill of all sessions | ||
| await terminal.management.killAllSessions(); |
There was a problem hiding this comment.
Sessions marked as killed before killAllSessions() completes—may block createOrAttach incorrectly if kill fails.
If terminal.management.killAllSessions() throws or partially fails, the sessions are already in userKilledSessions, causing createOrAttach to reject them even though they're still alive.
Consider adding sessions to userKilledSessions only after confirming the kill succeeded, or wrapping in a try/catch that removes the entries on failure.
🐛 Proposed fix
-const beforeIds = before.sessions.map((s) => s.sessionId);
-for (const id of beforeIds) {
- userKilledSessions.add(id);
-}
-console.log(
- "[killAllDaemonSessions] Before kill:",
- beforeIds.length,
- "sessions",
- beforeIds,
-);
-
-// Request kill of all sessions
-await terminal.management.killAllSessions();
+const beforeIds = before.sessions.map((s) => s.sessionId);
+console.log(
+ "[killAllDaemonSessions] Before kill:",
+ beforeIds.length,
+ "sessions",
+ beforeIds,
+);
+
+// Request kill of all sessions
+await terminal.management.killAllSessions();
+
+// Mark as killed only after successful kill request
+for (const id of beforeIds) {
+ userKilledSessions.add(id);
+}🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/terminal/terminal.ts` around lines 303 -
317, The code adds session IDs to userKilledSessions before calling
terminal.management.killAllSessions(), causing createOrAttach to treat sessions
as dead if killAll fails; change the logic so you only add IDs to
userKilledSessions after killAllSessions() completes successfully (or in a loop
that confirms each id was killed), or wrap the killAllSessions() call in
try/catch and on failure remove any previously added IDs from
userKilledSessions; reference terminal.management.killAllSessions(),
userKilledSessions, and createOrAttach to locate and update the flow so session
IDs are added only after confirmed kill (or rolled back on error).
3 participants
Links
Summary
This PR implements terminal session persistence via a background daemon process that survives app restarts, and hardens the feature for real-world usage (many accumulated terminal panes).
Happy path: https://www.loom.com/share/d84c42fdb4c24952ad112f9e6be4c82e?from_recorder=1&focus_title=1
User-visible behavior
visibility: hidden) to keep common switching smooth.working/permission→idle). No new background "terminal output" notifications/indicators are introduced in this PR.Recovery / management
Settings → Terminal → Manage sessions:
Dev-only:
Architecture
Runtime Abstraction
This abstraction enables future cloud workspace providers without spreading backend-specific branching throughout the codebase. Backend selection uses capability checks (
terminal.management !== null) rather thaninstanceofchecks.Key modules:
apps/desktop/src/main/lib/workspace-runtime/- Runtime abstraction layer (types, registry, local adapter)apps/desktop/src/main/lib/terminal/daemon-manager.ts- Daemon-backed terminal managerapps/desktop/src/main/lib/terminal/manager.ts- In-process terminal managerDaemon Mode
Daemon components:
apps/desktop/src/main/terminal-host/- Daemon process (runs asELECTRON_RUN_AS_NODE)apps/desktop/src/main/lib/terminal-host/client.ts- Client for Electron main → daemonKey DX/Performance Changes (Hardening)
WARM_TERMINAL_TAB_LIMIT = 8) inapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsxMAX_CONCURRENT_ATTACHES = 3) inapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/attach-scheduler.tscreateOrAttachlimiter (max 3) to prevent accidental attach fan-outlistSessions) +skipColdRestoresupport to avoid spawning during cold restoreterminal.streamdoes not complete onexitso stable pane subscriptions survive session loss/restart (fixes thelisteners=0cold-restore regression)exitfrom clearing the terminal UI)$HOME)0o600(defense in depth)exitlifecycle event even when no client is attached; main forwards via notifications subscription (terminal-exit)Cold Restore Regression Fix (listeners=0 / blank terminal after Start Shell)
Symptom
listeners=0ondata:${paneId}.Root cause
terminal.streamcompleted the observable onexit(emit.complete()).paneIdkey;@trpc/react-querydoes not auto-resubscribe after completion unless the input changes.paneIdacross restarts/cold restore → the pane becomes permanently detached from output.Fix
terminal.streamsubscription open acrossexit(treat exit as a state transition).exittriggering an unintended restart/clear).apps/desktop/docs/TERMINAL_HOST_EVENTS.mdand covered by regression testapps/desktop/src/lib/trpc/routers/terminal/terminal.stream.test.ts.Verification Matrix
Optional debug signals:
SUPERSET_TERMINAL_DEBUG=1localStorage.setItem('SUPERSET_TERMINAL_DEBUG', '1')Non-daemon (default / main parity)
exitshows restart prompt; any key restarts and output resumesDaemon mode (warm attach)
working/permission→idleDaemon mode (cold restore)
listeners=0stateFailure & recovery
isExitedstateBackpressure / correctness
yes | head -n 50000): UI stays responsive; no crash/OOMValidation
bun run typecheckbun run lintNODE_ENV=test bun testKnown Limitations / Follow-ups
pnpm testfinishes" or general background terminal output indicators (out of scope; evaluate in follow-up PR based on feedback).Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.