refactor(desktop): use serialize addon to serialize/restore for terminal tab switching#656
refactor(desktop): use serialize addon to serialize/restore for terminal tab switching#656
Conversation
…ab switching Replace disk-based scrollback persistence with xterm's SerializeAddon for clean terminal state restoration when switching tabs. This eliminates escape sequence artifacts that occurred when replaying raw terminal bytes. Key changes: - Add SerializeAddon to renderer, serialize state on detach, restore on reattach - Remove DataBatcher (direct PTY→IPC emission - xterm handles rendering efficiently) - Remove HistoryWriter and disk persistence (terminal-history.ts) - Remove terminal-escape-filter.ts (no longer needed) - Simplify session.ts (~120 lines vs ~200 before) - Update types: serializedState replaces scrollback+wasRecovered in SessionResult Architecture (Tabby-style): 1. On detach: renderer serializes parsed terminal content via SerializeAddon 2. Main process stores serializedState in memory on TerminalSession 3. On reattach: main returns serializedState, renderer writes to new xterm instance Benefits: - No escape sequence corruption on tab switch - Simpler codebase (~1800 lines deleted) - No disk I/O for terminal history - Direct data flow (PTY → IPC → xterm)
📝 WalkthroughWalkthroughReplaces filesystem-backed terminal history and batching with serialized in-memory state. Sessions now exchange Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Renderer as Renderer (Terminal.tsx)
participant Xterm as Xterm + SerializeAddon
participant Manager as TerminalManager
participant Session as TerminalSession
Note over Renderer,Xterm: Detach flow (capture & store)
User->>Renderer: request detach
Renderer->>Xterm: serialize() → serializedState
Xterm-->>Renderer: serializedState (string)
Renderer->>Manager: detach({ paneId, serializedState })
Manager->>Session: attach serializedState to session
Note over Manager,Renderer: Reattach flow (restore)
User->>Renderer: request reattach/createOrAttach
Renderer->>Manager: createOrAttach({ paneId, ... })
Manager->>Session: lookup session (has serializedState)
Session-->>Manager: { serializedState }
Manager-->>Renderer: { paneId, isNew:false, serializedState }
Renderer->>Xterm: apply(serializedState)
Xterm-->>Renderer: restored buffer & cursor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 0
🧹 Nitpick comments (2)
apps/desktop/src/main/lib/terminal/session.ts (2)
87-95: Potential unbounded scrollback growth.The
session.scrollbackaccumulates all PTY output for port detection hints but has no upper bound. For long-running sessions with verbose output, this could consume significant memory.Consider capping the scrollback buffer (e.g., keep only the last N characters or clear it periodically after port detection windows).
♻️ Suggested improvement
+const MAX_SCROLLBACK_SIZE = 100_000; // ~100KB for port detection + ptyProcess.onData((data) => { session.scrollback += data; + // Trim to prevent unbounded growth (port detection only needs recent output) + if (session.scrollback.length > MAX_SCROLLBACK_SIZE) { + session.scrollback = session.scrollback.slice(-MAX_SCROLLBACK_SIZE); + } // Check for hints that a port may have been opened (triggers immediate scan) portManager.checkOutputForHint(data, session.paneId); // Direct emission to renderer onData(paneId, data); });
119-136: Extract magic number for shell prompt delay.The 100ms delay (line 136) for shell prompt readiness is a magic number. Consider extracting it to a named constant for clarity.
♻️ Suggested improvement
+/** Small delay to ensure shell prompt is fully ready before sending commands */ +const SHELL_PROMPT_DELAY_MS = 100; + // Wait for first data (shell prompt ready), then send commands const dataHandler = session.pty.onData(() => { dataHandler.dispose(); // Only trigger once - setTimeout(() => { + setTimeout(() => { if (session.isAlive) { // ... } - }, 100); + }, SHELL_PROMPT_DELAY_MS); });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/lib/data-batcher.tsapps/desktop/src/main/lib/terminal-escape-filter.test.tsapps/desktop/src/main/lib/terminal-escape-filter.tsapps/desktop/src/main/lib/terminal-history.test.tsapps/desktop/src/main/lib/terminal-history.tsapps/desktop/src/main/lib/terminal/manager.test.tsapps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/main/lib/terminal/session.test.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/main/lib/terminal/types.tsapps/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/stores/tabs/utils/terminal-cleanup.ts
💤 Files with no reviewable changes (7)
- apps/desktop/src/main/lib/data-batcher.ts
- apps/desktop/src/main/lib/terminal/manager.test.ts
- apps/desktop/src/main/lib/terminal-escape-filter.test.ts
- apps/desktop/src/main/lib/terminal-escape-filter.ts
- apps/desktop/src/main/lib/terminal-history.ts
- apps/desktop/src/main/lib/terminal-history.test.ts
- apps/desktop/src/main/lib/terminal/session.test.ts
🧰 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/stores/tabs/utils/terminal-cleanup.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.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/lib/terminal/session.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/main/lib/terminal/manager.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/stores/tabs/utils/terminal-cleanup.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.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/lib/terminal/session.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/main/lib/terminal/manager.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/stores/tabs/utils/terminal-cleanup.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.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/stores/tabs/utils/terminal-cleanup.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.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/lib/terminal/session.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/main/lib/terminal/manager.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/stores/tabs/utils/terminal-cleanup.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.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/lib/terminal/session.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/main/lib/terminal/manager.ts
**/*.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/Terminal/Terminal.tsx
🧠 Learnings (3)
📚 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/renderer/stores/tabs/utils/terminal-cleanup.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/lib/terminal/session.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/lib/terminal/types.ts
🧬 Code graph analysis (3)
apps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.ts (1)
apps/desktop/src/renderer/lib/trpc-client.ts (1)
trpcClient(9-11)
apps/desktop/src/main/lib/terminal/session.ts (2)
apps/desktop/src/main/lib/terminal/types.ts (1)
TerminalSession(3-19)apps/desktop/src/main/lib/terminal/port-manager.ts (1)
portManager(281-281)
apps/desktop/src/main/lib/terminal/manager.ts (3)
apps/desktop/src/main/lib/terminal/types.ts (2)
CreateSessionParams(40-51)TerminalSession(3-19)apps/desktop/src/main/lib/terminal/index.ts (1)
CreateSessionParams(3-3)apps/desktop/src/main/lib/terminal/session.ts (1)
setupInitialCommands(104-138)
⏰ 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). (1)
- GitHub Check: Build
🔇 Additional comments (16)
apps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.ts (1)
3-10: LGTM!The simplification aligns well with the PR's removal of disk-based history. The mutation now only requires
paneId, error handling logs with context, and uses the standalone tRPC client appropriately for non-React contexts.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
6-6: LGTM!SerializeAddon integration follows the established pattern for other xterm addons. The addon is created, loaded, and exposed correctly. The lifecycle is consistent with other addons like
ClipboardAddonandUnicode11Addonwhich also rely onxterm.dispose()for cleanup.Also applies to: 109-109, 123-123, 133-133, 192-192
apps/desktop/src/main/lib/terminal/types.ts (2)
11-14: LGTM!Clear separation of concerns:
scrollbackfor port detection (raw PTY output),serializedStatefor renderer's parsed terminal content. The comments effectively document the purpose of each field.
34-38: LGTM!
SessionResultnow returnsserializedStateinstead ofscrollback/wasRecovered. The type is non-optional (always returns a string, empty for new sessions), which provides a consistent API contract.apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (3)
94-98: LGTM!The return type correctly exposes
serializedStatefor the renderer to restore terminal content on reattachment.
146-160: LGTM!The detach mutation now accepts
serializedStatefrom the renderer's SerializeAddon, enabling clean reattachment. Good documentation in the comment.
172-174: Synchronous mutation is correct.The removal of
async/awaitis appropriate sinceterminalManager.clearScrollback()is now synchronous (returnsvoid). The mutation handler can remain async-compatible since tRPC handles both.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (3)
474-488: LGTM! Serialization options are well-chosen.
excludeAltBuffer: true- Correctly excludes alternate screen buffer (vim, less, etc.)excludeModes: true- Avoids capturing terminal mode statesscrollback: 1000- Reasonable limit to prevent large serialized statesThe cleanup properly serializes before detaching and clears the ref.
212-217: Simplified stream handling looks correct.With Tabby-style serialize/restore, events no longer need queuing—the terminal subscription is enabled immediately after xterm is ready. The guard for
xtermRef.currentprevents writes before initialization.
310-316: Serialization approach is correct and follows xterm.js documentation.SerializeAddon.serialize() outputs VT/ANSI escape sequences that encode framebuffer content (text, colors, styles), and Terminal.write() is the documented method to restore state by replaying those sequences. The implementation at lines 310-316 matches the standard xterm.js pattern. No changes needed.
apps/desktop/src/main/lib/terminal/session.ts (1)
72-85: LGTM!Session initialization is clean.
scrollbackstarts empty andserializedStateis intentionally omitted (set later by renderer on detach).apps/desktop/src/main/lib/terminal/manager.ts (5)
33-37: LGTM!Correctly returns stored
serializedStatefor existing sessions, enabling clean reattachment. Falls back to empty string if no state is stored yet.
71-76: LGTM!Initial commands are properly set up only for new sessions, with conditional agent hooks waiting. This replaces the previous complex data handler setup.
224-238: LGTM!Detach correctly stores the
serializedStatefrom the renderer for later reattachment. The conditional store (only if truthy) prevents accidentally clearing state with an empty string.
240-255: LGTM!Clearing both
scrollbackandserializedStateensures a clean slate for the terminal buffer, consistent with the user's clear command intention.
208-222: LGTM!The simplified
killmethod removes thedeleteHistoryparameter, aligning with the removal of disk-based history persistence. The method correctly handles both alive and dead sessions.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @apps/desktop/src/main/lib/terminal/session.ts:
- Around line 122-124: Wrap the session.pty.write(initialCommandString) call in
a try/catch so any failure when the PTY has closed or errors is not swallowed;
inside the catch log the error with context (include session id or other
identifying info and the initialCommandString) using the existing logging
facility and avoid rethrowing unless caller expects it; keep the isAlive check
(session.isAlive) but still guard the write with try/catch to surface and record
failures.
- Around line 96-100: The function setupInitialCommands currently takes three
positional parameters (session, initialCommands, beforeInitialCommands);
refactor it to accept a single params object (e.g., { session, initialCommands,
beforeInitialCommands }) by updating the function signature, destructuring those
named properties inside the function, and preserving the existing types and
optionality. Update all call sites to pass an object with those named keys
instead of positional args, and ensure any exported type annotations or usages
(e.g., in TerminalSession contexts) are adjusted to the new param shape so
TypeScript signatures remain correct.
🧹 Nitpick comments (5)
apps/desktop/src/main/lib/terminal/port-manager.ts (2)
11-11: Remove unused constant.The
_HINT_SCAN_DELAY_MSconstant is declared but never used after the removal of hint-based scanning. The underscore prefix suggests it's intended to be internal/unused.🧹 Proposed cleanup
-// Delay before running an immediate scan triggered by output hint (in ms) -// This gives the server time to fully bind the port -const _HINT_SCAN_DELAY_MS = 500; - // Ports to ignore (common system ports that are usually not dev servers) const IGNORED_PORTS = new Set([22, 80, 443, 5432, 3306, 6379, 27017]);
25-25: Consider removing unusedpendingHintScansinfrastructure.The
pendingHintScansmap is no longer populated after removingcheckOutputForHint. It's only cleared inunregisterSessionbut never has entries added, making it effectively dead code.🧹 Proposed cleanup
class PortManager extends EventEmitter { private ports = new Map<string, DetectedPort>(); private sessions = new Map<string, RegisteredSession>(); private scanInterval: ReturnType<typeof setInterval> | null = null; - private pendingHintScans = new Map<string, ReturnType<typeof setTimeout>>(); private isScanning = false;unregisterSession(paneId: string): void { this.sessions.delete(paneId); this.removePortsForPane(paneId); - - // Cancel any pending hint scan for this pane - const pendingTimeout = this.pendingHintScans.get(paneId); - if (pendingTimeout) { - clearTimeout(pendingTimeout); - this.pendingHintScans.delete(paneId); - } }stopPeriodicScan(): void { if (this.scanInterval) { clearInterval(this.scanInterval); this.scanInterval = null; } - - for (const timeout of this.pendingHintScans.values()) { - clearTimeout(timeout); - } - this.pendingHintScans.clear(); }Also applies to: 47-52, 80-83
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
308-312: Consider validating serializedState before writing.The
applySerializedStatefunction writes serialized content directly to xterm without validation. IfserializedStatecontains unexpected control sequences or is corrupted, it could cause terminal rendering issues.🛡️ Add validation
const applySerializedState = (serializedState: string) => { if (serializedState) { - xterm.write(serializedState); + try { + xterm.write(serializedState); + } catch (error) { + console.warn(`[Terminal] Failed to apply serialized state for pane ${paneId}:`, error); + } } };apps/desktop/src/main/lib/terminal/session.ts (2)
105-106: Move initialCommandString construction after early return.The
initialCommandStringis constructed before checking ifinitialCommandsis empty, wasting computation when commands aren't needed.⚡ Optimize construction order
if (!initialCommands || initialCommands.length === 0) { return; } const initialCommandString = `${initialCommands.join(" && ")}\n`;
110-110: Extract magic number to named constant.The 100ms delay before running initial commands is a hardcoded magic number. It should be extracted to a named constant at the module top for clarity and maintainability.
📝 Extract to named constant
At the top of the file after
AGENT_HOOKS_TIMEOUT_MS:const DEFAULT_COLS = 80; const DEFAULT_ROWS = 24; /** Max time to wait for agent hooks before running initial commands */ const AGENT_HOOKS_TIMEOUT_MS = 2000; +/** Delay after first shell output before running initial commands */ +const INITIAL_COMMAND_DELAY_MS = 100;Then use it:
- setTimeout(() => { + setTimeout(() => { if (session.isAlive) { void (async () => { if (beforeInitialCommands) { const timeout = new Promise<void>((resolve) => setTimeout(resolve, AGENT_HOOKS_TIMEOUT_MS), ); await Promise.race([beforeInitialCommands, timeout]).catch( () => {}, ); } if (session.isAlive) { session.pty.write(initialCommandString); } })(); } - }, 100); + }, INITIAL_COMMAND_DELAY_MS);As per coding guidelines: Extract magic numbers and hardcoded values to named constants at module top.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/desktop/src/main/lib/terminal/manager.tsapps/desktop/src/main/lib/terminal/port-hints.tsapps/desktop/src/main/lib/terminal/port-manager.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/main/lib/terminal/port-hints.ts
🧰 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/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/main/lib/terminal/port-manager.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/main/lib/terminal/manager.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/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/main/lib/terminal/port-manager.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/main/lib/terminal/manager.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/Terminal/Terminal.tsxapps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.ts
**/*.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/Terminal/Terminal.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/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/main/lib/terminal/port-manager.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/main/lib/terminal/manager.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/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/main/lib/terminal/port-manager.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/main/lib/terminal/manager.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 Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.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/lib/terminal/types.ts
🧬 Code graph analysis (2)
apps/desktop/src/main/lib/terminal/session.ts (1)
apps/desktop/src/main/lib/terminal/types.ts (1)
TerminalSession(3-16)
apps/desktop/src/main/lib/terminal/manager.ts (2)
apps/desktop/src/main/lib/terminal/types.ts (2)
CreateSessionParams(37-48)TerminalSession(3-16)apps/desktop/src/main/lib/terminal/session.ts (1)
setupInitialCommands(96-129)
⏰ 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). (1)
- GitHub Check: Build
🔇 Additional comments (5)
apps/desktop/src/main/lib/terminal/types.ts (1)
11-11: Type inconsistency betweenTerminalSession.serializedStateandSessionResult.serializedState.
TerminalSession.serializedStateis optional (string?) whileSessionResult.serializedStateis required (string). All code paths returningSessionResultalready provideserializedState(using empty string fallback), so the implementation is sound. However, consider aligning the type definitions for better consistency: either makeSessionResult.serializedStateoptional to matchTerminalSession, or document whySessionResultalways requires a value.apps/desktop/src/main/lib/terminal/manager.ts (4)
6-6: LGTM - Clean import refactoring.The shift from legacy history-based functions to
createSessionandsetupInitialCommandsaligns with the serialized-state approach and simplifies the session lifecycle.
17-91: Session lifecycle correctly refactored to use serialized state.The migration from scrollback-based recovery to in-memory
serializedStateis implemented correctly:
- Existing sessions return stored
serializedState(line 35)- New sessions initialize with empty state (line 89)
setupInitialCommandsproperly replaces the old history-based initializationNote: Serialized terminal state is now held in memory per session. For very long-running sessions with extensive output, this could grow, but it's the intended tradeoff for eliminating disk I/O and escape-sequence artifacts.
237-250: Verify clearScrollback clears both state and display.The method now only clears the in-memory
serializedState(line 248) without sending a clear sequence to the terminal's pty. This means:
- State tracking is reset ✓
- Terminal display content is not cleared
If the intent is to also clear the visible scrollback in the terminal, the implementation should additionally send the appropriate control sequence (e.g.,
\x1b[2J\x1b[3J\x1b[H) viasession.pty.write().Confirm whether the renderer handles display clearing separately or if this method should do both.
222-235: No action needed —serializedStateis consistently provided in all detach calls from the renderer. The Terminal component always serializes terminal state usingserializeAddon.serialize()before calling detach, so the optional handling in the method is defensive but not indicative of an actual issue.Likely an incorrect or invalid review comment.
| export function setupInitialCommands( | ||
| session: TerminalSession, | ||
| initialCommands: string[] | undefined, | ||
| wasRecovered: boolean, | ||
| onHistoryReinit: () => Promise<void>, | ||
| beforeInitialCommands?: Promise<void>, | ||
| ): void { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use object parameter for function with 3 parameters.
setupInitialCommands has 3 parameters and should accept a single params object per coding guidelines.
♻️ Refactor to object parameter
/**
* Set up initial commands to run after shell prompt is ready.
* Commands are only sent for new sessions (not reattachments).
*/
-export function setupInitialCommands(
- session: TerminalSession,
- initialCommands: string[] | undefined,
- beforeInitialCommands?: Promise<void>,
-): void {
+export function setupInitialCommands(params: {
+ session: TerminalSession;
+ initialCommands: string[] | undefined;
+ beforeInitialCommands?: Promise<void>;
+}): void {
+ const { session, initialCommands, beforeInitialCommands } = params;
if (!initialCommands || initialCommands.length === 0) {
return;
}As per coding guidelines: Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility.
📝 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.
| export function setupInitialCommands( | |
| session: TerminalSession, | |
| initialCommands: string[] | undefined, | |
| wasRecovered: boolean, | |
| onHistoryReinit: () => Promise<void>, | |
| beforeInitialCommands?: Promise<void>, | |
| ): void { | |
| export function setupInitialCommands(params: { | |
| session: TerminalSession; | |
| initialCommands: string[] | undefined; | |
| beforeInitialCommands?: Promise<void>; | |
| }): void { | |
| const { session, initialCommands, beforeInitialCommands } = params; |
🤖 Prompt for AI Agents
In @apps/desktop/src/main/lib/terminal/session.ts around lines 96 - 100, The
function setupInitialCommands currently takes three positional parameters
(session, initialCommands, beforeInitialCommands); refactor it to accept a
single params object (e.g., { session, initialCommands, beforeInitialCommands })
by updating the function signature, destructuring those named properties inside
the function, and preserving the existing types and optionality. Update all call
sites to pass an object with those named keys instead of positional args, and
ensure any exported type annotations or usages (e.g., in TerminalSession
contexts) are adjusted to the new param shape so TypeScript signatures remain
correct.
Kitenite
changed the title
Kitenite
changed the title
Kitenite
changed the title
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
…or terminal tab switching (superset-sh#656)" This reverts commit ccf9cdb.
…nal tab switching (#656) * refactor(desktop): adopt Tabby-style serialize/restore for terminal tab switching Replace disk-based scrollback persistence with xterm's SerializeAddon for clean terminal state restoration when switching tabs. This eliminates escape sequence artifacts that occurred when replaying raw terminal bytes. Key changes: - Add SerializeAddon to renderer, serialize state on detach, restore on reattach - Remove DataBatcher (direct PTY→IPC emission - xterm handles rendering efficiently) - Remove HistoryWriter and disk persistence (terminal-history.ts) - Remove terminal-escape-filter.ts (no longer needed) - Simplify session.ts (~120 lines vs ~200 before) - Update types: serializedState replaces scrollback+wasRecovered in SessionResult Architecture (Tabby-style): 1. On detach: renderer serializes parsed terminal content via SerializeAddon 2. Main process stores serializedState in memory on TerminalSession 3. On reattach: main returns serializedState, renderer writes to new xterm instance Benefits: - No escape sequence corruption on tab switch - Simpler codebase (~1800 lines deleted) - No disk I/O for terminal history - Direct data flow (PTY → IPC → xterm) * clean up and remove port hints
1 participant
Summary
Architecture Change
Before: Raw scrollback bytes stored on disk → replayed on reattach → escape sequence corruption
After (Tabby-style):
SerializeAddonserializedStatein memory onTerminalSessionserializedState, renderer writes to new xterm instanceFiles Changed
Terminal.tsx,helpers.tstypes.ts(serializedState replaces scrollback+wasRecovered)session.ts,manager.ts,terminal.ts(tRPC router)terminal-history.ts,data-batcher.ts,terminal-escape-filter.ts, testsTesting
Summary by CodeRabbit
New Features
Changes
Removals
✏️ Tip: You can customize this high-level summary in your review settings.