Conversation
|
Warning Rate limit exceeded@Kitenite has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 20 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR refactors terminal session management from using runtime serialization (SerializeAddon) to a disk-backed history system. It introduces persistent scrollback storage, batched data flushing, scrollback recovery on reattachment, and clear-sequence detection. The system manages terminal output through DataBatcher, writes history via HistoryWriter, recovers previous sessions via HistoryReader, and detects ED3 clear-scrollback sequences to reset scrollback appropriately. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (Terminal.tsx)
participant Manager as TerminalManager
participant Session as Session
participant HistoryReader
participant HistoryWriter
participant DataBatcher
participant PTY as Node PTY
participant FS as File System
Renderer->>Manager: createOrAttach({ workspaceId, paneId, ... })
Manager->>Session: recoverScrollback(existingScrollback, workspaceId, paneId)
Session->>HistoryReader: read()
HistoryReader->>FS: load scrollback.bin & metadata.json
FS-->>HistoryReader: prior session data
HistoryReader-->>Session: { scrollback, metadata? }
Session-->>Manager: { scrollback, wasRecovered }
Manager->>Session: createSession({ existingScrollback, ... })
Session->>HistoryWriter: init(initialScrollback)
HistoryWriter->>FS: create directory, write initial scrollback
Session->>DataBatcher: new DataBatcher(onFlush)
Manager->>PTY: spawn shell
Manager->>Session: setupDataHandler(session, initialCommands, wasRecovered, ...)
PTY-->>Manager: data event
Manager->>DataBatcher: write(data)
DataBatcher->>DataBatcher: buffer, check size/time threshold
DataBatcher-->>HistoryWriter: flush callback with batched data
HistoryWriter->>FS: append to scrollback.bin
DataBatcher-->>Renderer: onData event with batched content
Renderer->>Renderer: detect ED3 clear sequence
Renderer->>Manager: clearScrollback({ paneId })
Manager->>Session: reinitializeHistory()
Session->>HistoryWriter: close() & new HistoryWriter init()
Session->>Renderer: scrollback cleared
PTY-->>Manager: exit event
Manager->>Session: flushSession() & closeSessionHistory(exitCode)
Session->>DataBatcher: dispose()
Session->>HistoryWriter: close(exitCode)
HistoryWriter->>FS: write metadata.json with exitCode/endedAt
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes The PR spans 17 files with significant logic changes across multiple layers (persistence, batching, event handling, state recovery), introduces new concepts (history recovery, DataBatcher, escape-sequence detection), and requires understanding interconnected flows between terminal manager, session, renderer components, and file I/O. Heterogeneous changes demand separate reasoning for each area rather than applying a single refactor pattern repeatedly. Possibly Related PRs
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (3 warnings)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
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/main/lib/terminal/manager.ts (1)
113-157: Don’t let history cleanup errors become unhandled or silently swallowed.
pty.onExit(async ...)should defensively catchcloseSessionHistoryfailures; and the.catch(() => {})call sites should log context.
As per coding guidelines, never swallow errors silently.Example direction
- closeSessionHistory(session).catch(() => {}); + closeSessionHistory(session).catch((error) => { + console.warn("[TerminalManager/closeSessionHistory] Failed during forced cleanup:", error); + });Also applies to: 317-380, 420-444
🤖 Fix all issues with AI agents
In @apps/desktop/src/main/lib/terminal-history.ts:
- Around line 16-22: The getHistoryDir function currently uses workspaceId and
paneId directly as path segments, allowing path traversal; sanitize these
identifiers before joining by either validating and rejecting inputs that
contain path separators or traversal tokens (e.g., '/', '\\', '..'), or
normalizing them to safe tokens (e.g., using basename-like stripping or
safe-encoding) and throw/return an error on invalid values; update getHistoryDir
to sanitize workspaceId and paneId (references: getHistoryDir, workspaceId,
paneId, SUPERSET_HOME_DIR) before calling join so no user-supplied segment can
escape the intended directory.
In @apps/desktop/src/main/lib/terminal/manager.test.ts:
- Around line 51-61: The kill mock currently auto-triggers the onExit handler
via setImmediate while tests also manually invoke mockPty.onExit, causing
double-exit flakiness; update the kill mock (the mock function for "kill" that
references mockPty.onExit and setImmediate) to guard against double-calling by
tracking an "exited" flag in the closure or on mockPty (e.g., exited boolean)
and only invoke onExit if not already exited, or remove the auto-trigger
entirely and rely on tests' manual onExit calls—apply the same guard pattern to
the other kill mocks mentioned (around lines corresponding to 272-283, 316-325,
432-447) so exit handlers are invoked exactly once.
In @apps/desktop/src/main/lib/terminal/port-manager.ts:
- Around line 68-71: The setTimeout callback currently calls
this.scanPane(paneId).catch(() => {}) which silently swallows errors; change it
to catch the error and log it with context instead. Update the pendingHintScans
timeout callback so that the promise rejection from scanPane(paneId) is handled
by catching the error and calling a logger (e.g., this.logger.error or
processLogger) with a descriptive message including paneId and
HINT_SCAN_DELAY_MS, and ensure pendingHintScans.delete(paneId) still runs; use
the existing pendingHintScans, scanPane, paneId, and HINT_SCAN_DELAY_MS symbols
to locate and modify the code.
- Around line 156-158: The loop over panePortMap is calling
getListeningPortsForPids(pids) but not awaiting it, so portInfos will be a
Promise instead of the ports array; change the call to await
getListeningPortsForPids(pids) and pass the resolved array into
updatePortsForPane(paneId, workspaceId, portInfos), and ensure the enclosing
function is declared async (or otherwise supports awaiting) so the await is
valid.
- Around line 121-123: The call to getListeningPortsForPids returns a Promise
but the code assigns it directly to portInfos and passes it to
updatePortsForPane; change the call to await getListeningPortsForPids(pids) so
portInfos is a PortInfo[] before calling this.updatePortsForPane(paneId,
workspaceId, portInfos), ensuring the async result is resolved (and mark the
surrounding function async if it isn't already).
In @apps/desktop/src/main/lib/terminal/session.ts:
- Around line 31-41: session.scrollback can grow without bound during live
sessions; add an in-memory cap using the same MAX_SCROLLBACK_CHARS logic so any
time you append to session.scrollback or history.scrollback you truncate to the
last MAX_SCROLLBACK_CHARS characters. Locate places that mutate
session.scrollback (and the similar mutation around lines 164-166) and after
concatenation or push, assign session.scrollback = session.scrollback.length >
MAX_SCROLLBACK_CHARS ? session.scrollback.slice(-MAX_SCROLLBACK_CHARS) :
session.scrollback (use the constant MAX_SCROLLBACK_CHARS defined in this file)
so runtime memory never exceeds the cap while keeping the existing recovery
truncation logic intact.
In
@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx:
- Around line 31-33: The component is currently subscribing to the entire panes
map via useTabsStore((s) => s.panes), causing rerenders on any pane mutation;
change the selector to only read the current pane and/or the specific field you
need: replace const panes = useTabsStore((s) => s.panes); const pane =
panes[paneId]; const paneInitialCommands = pane?.initialCommands; with direct
selectors such as const pane = useTabsStore(s => s.panes[paneId]); and/or const
paneInitialCommands = useTabsStore(s => s.panes[paneId]?.initialCommands); so
the component only re-renders when that specific pane or its initialCommands
changes.
🧹 Nitpick comments (9)
apps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.ts (1)
7-13: Implementation follows tRPC patterns correctly.The function properly uses the standalone tRPC client for non-React contexts and includes appropriate error handling for the cleanup operation.
Optional: Align console logging with prefixed pattern
The coding guidelines recommend using prefixed console logging with context pattern
[domain/operation] message. Consider:.catch((error) => { - console.warn(`Failed to kill terminal for pane ${paneId}:`, error); + console.warn(`[terminal/kill] Failed to kill terminal for pane ${paneId}:`, error); });Based on coding guidelines.
apps/desktop/src/main/lib/terminal/port-hints.ts (1)
31-31: Extract magic number to a named constant.The minimum length
10should be extracted to a named constant at module top for clarity and maintainability.Suggested change
+const MIN_DATA_LENGTH = 10; + /** * Check if terminal output contains hints that a port may have been opened. * This is a lightweight check - false positives are acceptable since we verify * with actual process scanning. */ export function containsPortHint(data: string): boolean { - if (data.length < 10) return false; + if (data.length < MIN_DATA_LENGTH) return false; return HINT_PATTERNS.some((pattern) => pattern.test(data)); }As per coding guidelines, extract magic numbers and hardcoded values to named constants at module top.
apps/desktop/src/main/lib/terminal-history.test.ts (2)
10-12: Naming inconsistency:testTabIdis used aspaneId.The variable
testTabIdis passed toHistoryWriterandHistoryReaderconstructors as thepaneIdparameter. Consider renaming totestPaneIdfor clarity.Suggested change
describe("HistoryWriter", () => { const testWorkspaceId = "test-workspace"; - const testTabId = "test-tab"; + const testPaneId = "test-pane"; let historyDir: string; beforeEach(async () => { - historyDir = getHistoryDir(testWorkspaceId, testTabId); + historyDir = getHistoryDir(testWorkspaceId, testPaneId);Apply similar changes throughout the file where
testTabIdis used.
255-261: Consider using type assertion instead of@ts-expect-error.The error type can be properly narrowed using a type guard or assertion for cleaner error handling.
Suggested change
// Verify directory is gone try { await fs.stat(historyDir); throw new Error("Directory should not exist"); } catch (error) { - // @ts-expect-error - expect(error.code).toBe("ENOENT"); + expect((error as NodeJS.ErrnoException).code).toBe("ENOENT"); }apps/desktop/src/main/lib/terminal-escape-filter.ts (1)
15-18: Static analysis false positive - regex is safe.The ast-grep ReDoS warning is a false positive. The regex is constructed from a module-level constant (
ESC), not user input, and the pattern\x1b\[3Jcontains no unbounded quantifiers or backtracking risks.However, to eliminate the warning and improve clarity, you could use a literal regex:
Optional: Use literal regex to avoid static analysis warning
-const CLEAR_SCROLLBACK_PATTERN = new RegExp(`${ESC}\\[3J`); +const CLEAR_SCROLLBACK_PATTERN = /\x1b\[3J/;apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
218-253: Event buffering logic is directionally good; consider stabilizing the stream handler identity.
handleStreamDatais recreated every render; iftrpc.terminal.stream.useSubscriptionis sensitive to handler identity, this can cause churn or missed buffering semantics. ConsideruseCallback+ refs forsubscriptionEnabled/updateCwdFromData.Also applies to: 324-350, 352-464, 538-540
apps/desktop/src/main/lib/data-batcher.ts (1)
14-83: HardenonFlushexecution (don’t let it take down the main process).
IfonFlushthrows (especially from the timer), it can surface as an unhandled exception. Wrapthis.onFlush(...)with try/catch + contextual log.Proposed fix
flush(): void { if (this.timeout !== null) { clearTimeout(this.timeout); this.timeout = null; } if (this.buffer.length > 0) { - this.onFlush(this.buffer); + try { + this.onFlush(this.buffer); + } catch (error) { + console.error("[DataBatcher/flush] onFlush failed:", error); + } this.buffer = ""; } }apps/desktop/src/main/lib/terminal-history.ts (1)
137-144: Use prefixed logging for consistency.
Align with the “[domain/operation] message” convention.
As per coding guidelines, use prefixed console logging with context.Proposed fix
} catch (error) { - console.error("Failed to cleanup history:", error); + console.error("[TerminalHistory/cleanup] Failed to cleanup history:", error); } }apps/desktop/src/main/lib/terminal/session.ts (1)
19-23: Prefer a params object for new APIs with 2+ arguments.
recoverScrollback(existingScrollback, workspaceId, paneId)/setupDataHandler(...)/closeSessionHistory(session, exitCode)are new surfaces—worth aligning now before more call sites land.
As per coding guidelines, functions with 2+ parameters should accept a single params object.Also applies to: 142-148, 196-217
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
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/port-hints.tsapps/desktop/src/main/lib/terminal/port-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 (1)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.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/lib/terminal-escape-filter.tsapps/desktop/src/main/lib/terminal/port-hints.tsapps/desktop/src/main/lib/terminal-escape-filter.test.tsapps/desktop/src/main/lib/terminal-history.tsapps/desktop/src/main/lib/terminal/port-manager.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/lib/data-batcher.tsapps/desktop/src/main/lib/terminal/session.test.tsapps/desktop/src/main/lib/terminal/manager.test.tsapps/desktop/src/main/lib/terminal-history.test.tsapps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/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/main/lib/terminal-escape-filter.tsapps/desktop/src/main/lib/terminal/port-hints.tsapps/desktop/src/main/lib/terminal-escape-filter.test.tsapps/desktop/src/main/lib/terminal-history.tsapps/desktop/src/main/lib/terminal/port-manager.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/lib/data-batcher.tsapps/desktop/src/main/lib/terminal/session.test.tsapps/desktop/src/main/lib/terminal/manager.test.tsapps/desktop/src/main/lib/terminal-history.test.tsapps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/main/lib/terminal/manager.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/lib/terminal-escape-filter.tsapps/desktop/src/main/lib/terminal/port-hints.tsapps/desktop/src/main/lib/terminal-escape-filter.test.tsapps/desktop/src/main/lib/terminal-history.tsapps/desktop/src/main/lib/terminal/port-manager.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/lib/data-batcher.tsapps/desktop/src/main/lib/terminal/session.test.tsapps/desktop/src/main/lib/terminal/manager.test.tsapps/desktop/src/main/lib/terminal-history.test.tsapps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/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/main/lib/terminal-escape-filter.tsapps/desktop/src/main/lib/terminal/port-hints.tsapps/desktop/src/main/lib/terminal-escape-filter.test.tsapps/desktop/src/main/lib/terminal-history.tsapps/desktop/src/main/lib/terminal/port-manager.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/lib/data-batcher.tsapps/desktop/src/main/lib/terminal/session.test.tsapps/desktop/src/main/lib/terminal/manager.test.tsapps/desktop/src/main/lib/terminal-history.test.tsapps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/main/lib/terminal/manager.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Co-locate tests with implementation files using .test.ts or .test.tsx suffix
Files:
apps/desktop/src/main/lib/terminal-escape-filter.test.tsapps/desktop/src/main/lib/terminal/session.test.tsapps/desktop/src/main/lib/terminal/manager.test.tsapps/desktop/src/main/lib/terminal-history.test.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/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/Terminal/Terminal.tsx
🧠 Learnings (1)
📚 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
🧬 Code graph analysis (10)
apps/desktop/src/main/lib/terminal-escape-filter.test.ts (1)
apps/desktop/src/main/lib/terminal-escape-filter.ts (2)
containsClearScrollbackSequence(29-31)extractContentAfterClear(38-46)
apps/desktop/src/main/lib/terminal/port-manager.ts (2)
apps/desktop/src/main/lib/terminal/port-hints.ts (1)
containsPortHint(30-34)apps/desktop/src/main/lib/terminal/port-scanner.ts (2)
getProcessTree(21-28)getListeningPortsForPids(34-49)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (2)
apps/desktop/src/main/lib/terminal/manager.ts (1)
terminalManager(454-454)apps/desktop/src/main/lib/terminal/index.ts (1)
terminalManager(1-1)
apps/desktop/src/main/lib/terminal/session.test.ts (3)
apps/desktop/src/main/lib/terminal/session.ts (2)
recoverScrollback(19-42)flushSession(235-237)apps/desktop/src/main/lib/terminal-history.ts (1)
getHistoryDir(16-22)apps/desktop/src/main/lib/terminal/types.ts (1)
TerminalSession(5-22)
apps/desktop/src/main/lib/terminal/manager.test.ts (1)
apps/desktop/src/main/lib/terminal/manager.ts (1)
TerminalManager(20-451)
apps/desktop/src/main/lib/terminal-history.test.ts (1)
apps/desktop/src/main/lib/terminal-history.ts (3)
getHistoryDir(16-22)HistoryWriter(32-107)HistoryReader(109-145)
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/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts (1)
TerminalStreamEvent(6-8)apps/desktop/src/renderer/stores/theme/store.ts (1)
useTerminalTheme(200-201)apps/desktop/src/renderer/stores/theme/index.ts (1)
useTerminalTheme(4-4)
apps/desktop/src/main/lib/terminal/types.ts (2)
apps/desktop/src/main/lib/terminal-history.ts (1)
HistoryWriter(32-107)apps/desktop/src/main/lib/data-batcher.ts (1)
DataBatcher(20-84)
apps/desktop/src/main/lib/terminal/manager.ts (2)
apps/desktop/src/main/lib/terminal/types.ts (2)
InternalCreateSessionParams(56-59)TerminalSession(5-22)apps/desktop/src/main/lib/terminal/session.ts (4)
setupDataHandler(142-194)reinitializeHistory(219-233)flushSession(235-237)closeSessionHistory(196-217)
🪛 ast-grep (0.40.4)
apps/desktop/src/main/lib/terminal-escape-filter.ts
[warning] 15-15: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${ESC}\\[3J)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ 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 (13)
apps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.ts (1)
8-9: Good:deleteHistoryparameter is explicitly handled in terminal cleanup operations.The single call site in
killTerminalForPanecorrectly and explicitly setsdeleteHistory: true. The underlyingterminalManager.kill()implementation has a safe default ofdeleteHistory = false(preserves history), ensuring history is only permanently deleted when explicitly requested.apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (3)
97-99: LGTM!The return shape change from
serializedStatetoscrollbackandwasRecoveredaligns well with the new persistent history model. This provides clearer semantics for session recovery.
137-146: LGTM!The addition of
deleteHistoryoptional flag to the kill procedure provides necessary control over history cleanup when terminating sessions.
161-173: LGTM!Converting
clearScrollbackto async and updating the comment to reflect clearing both in-memory and persistent history accurately documents the new behavior.apps/desktop/src/main/lib/terminal-history.test.ts (1)
31-180: Good test coverage for HistoryWriter.The tests comprehensively cover key scenarios: basic writes, metadata with exit codes, session recovery with scrollback preservation, ANSI escape code handling, high-frequency small writes, and binary-like data. This provides solid validation of the history persistence mechanism.
apps/desktop/src/main/lib/terminal/session.test.ts (2)
8-76: Good test coverage for recoverScrollback.The tests effectively cover the key scenarios: returning existing scrollback when provided, returning empty scrollback with no history, recovering from disk, and preferring memory over disk. The cleanup with try/finally ensures test isolation.
78-96: LGTM!The flushSession test validates the essential behavior of disposing the data batcher. The mock approach is appropriate for unit testing this specific functionality.
apps/desktop/src/main/lib/terminal-escape-filter.test.ts (2)
9-35: Excellent test coverage for containsClearScrollbackSequence.The tests thoroughly validate ED3 detection while correctly rejecting RIS sequences (used by TUI apps). Good coverage of mixed content and other escape sequences.
37-138: Comprehensive edge case coverage for extractContentAfterClear.The tests cover all critical scenarios: ED3-only, content before/after ED3, multiple ED3 sequences, RIS preservation, mixed sequences, unicode, newlines, ANSI colors, and near-miss sequences. This provides strong confidence in the extraction logic.
apps/desktop/src/main/lib/terminal-escape-filter.ts (1)
29-46: LGTM!Both functions are well-implemented with clear documentation. The decision to only detect ED3 (not RIS) is well-reasoned and documented, preventing false clears from TUI application repaints.
apps/desktop/src/main/lib/terminal/port-manager.ts (1)
56-74: Well-implemented debounce pattern for hint-triggered scans.The debounce logic correctly cancels pending scans before scheduling new ones, preventing unnecessary duplicate scans when terminal output contains multiple hints in quick succession.
apps/desktop/src/main/lib/terminal/types.ts (1)
2-3: Type surface looks consistent with the new history + batching lifecycle; ensure it stays main-process-only.
HistoryWriter/DataBatcherare main-only concepts; keepapps/desktop/src/main/lib/terminal/types.tsout of any renderer/shared import graph to avoid accidental bundling/IPC serialization issues.Also applies to: 13-18, 39-41, 57-58
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
158-162: CWD sync: verifyterminalCwdnull handling before pushing into the store.
Right now this can callupdatePaneCwd(paneId, null, ...)on initial mount. If the store/type expectsstring, this is a runtime/typing footgun.
| export function getHistoryDir(workspaceId: string, paneId: string): string { | ||
| const baseDir = | ||
| env.NODE_ENV === "test" | ||
| ? join(tmpdir(), "superset-test", ".superset") | ||
| : SUPERSET_HOME_DIR; | ||
| return join(baseDir, "terminal-history", workspaceId, paneId); | ||
| } |
There was a problem hiding this comment.
Sanitize workspaceId / paneId before using them as path segments.
If either identifier can contain /, .., etc., this becomes a path traversal vector writing under SUPERSET_HOME_DIR.
Proposed fix
export function getHistoryDir(workspaceId: string, paneId: string): string {
+ const safe = (segment: string) => segment.replaceAll(/[^a-zA-Z0-9._-]/g, "_");
const baseDir =
env.NODE_ENV === "test"
? join(tmpdir(), "superset-test", ".superset")
: SUPERSET_HOME_DIR;
- return join(baseDir, "terminal-history", workspaceId, paneId);
+ return join(baseDir, "terminal-history", safe(workspaceId), safe(paneId));
}🤖 Prompt for AI Agents
In @apps/desktop/src/main/lib/terminal-history.ts around lines 16 - 22, The
getHistoryDir function currently uses workspaceId and paneId directly as path
segments, allowing path traversal; sanitize these identifiers before joining by
either validating and rejecting inputs that contain path separators or traversal
tokens (e.g., '/', '\\', '..'), or normalizing them to safe tokens (e.g., using
basename-like stripping or safe-encoding) and throw/return an error on invalid
values; update getHistoryDir to sanitize workspaceId and paneId (references:
getHistoryDir, workspaceId, paneId, SUPERSET_HOME_DIR) before calling join so no
user-supplied segment can escape the intended directory.
| kill: mock(function (this: any, _signal?: string) { | ||
| // Automatically trigger onExit when kill is called to avoid timeouts in cleanup | ||
| const onExitCallback = | ||
| mockPty.onExit.mock.calls[mockPty.onExit.mock.calls.length - 1]?.[0]; | ||
| if (onExitCallback) { | ||
| // Use setImmediate to avoid blocking | ||
| setImmediate(async () => { | ||
| await onExitCallback({ exitCode: 0, signal: undefined }); | ||
| }); | ||
| } | ||
| }), |
There was a problem hiding this comment.
Test flakiness risk: kill auto-exits AND tests manually invoke onExit (double-exit).
Pick one mechanism; double-calling exit handlers can hide real regressions and intermittently fail depending on timing.
Also applies to: 272-283, 316-325, 432-447
🤖 Prompt for AI Agents
In @apps/desktop/src/main/lib/terminal/manager.test.ts around lines 51 - 61, The
kill mock currently auto-triggers the onExit handler via setImmediate while
tests also manually invoke mockPty.onExit, causing double-exit flakiness; update
the kill mock (the mock function for "kill" that references mockPty.onExit and
setImmediate) to guard against double-calling by tracking an "exited" flag in
the closure or on mockPty (e.g., exited boolean) and only invoke onExit if not
already exited, or remove the auto-trigger entirely and rely on tests' manual
onExit calls—apply the same guard pattern to the other kill mocks mentioned
(around lines corresponding to 272-283, 316-325, 432-447) so exit handlers are
invoked exactly once.
| const timeout = setTimeout(() => { | ||
| this.pendingHintScans.delete(paneId); | ||
| this.scanPane(paneId).catch(() => {}); | ||
| }, HINT_SCAN_DELAY_MS); |
There was a problem hiding this comment.
Avoid silently swallowing errors.
The .catch(() => {}) swallows errors without any logging. At minimum, log the error with context.
Suggested fix
const timeout = setTimeout(() => {
this.pendingHintScans.delete(paneId);
- this.scanPane(paneId).catch(() => {});
+ this.scanPane(paneId).catch((error) => {
+ console.error(`[PortManager] Hint scan failed for pane ${paneId}:`, error);
+ });
}, HINT_SCAN_DELAY_MS);As per coding guidelines, never swallow errors silently - at minimum log them with context.
🤖 Prompt for AI Agents
In @apps/desktop/src/main/lib/terminal/port-manager.ts around lines 68 - 71, The
setTimeout callback currently calls this.scanPane(paneId).catch(() => {}) which
silently swallows errors; change it to catch the error and log it with context
instead. Update the pendingHintScans timeout callback so that the promise
rejection from scanPane(paneId) is handled by catching the error and calling a
logger (e.g., this.logger.error or processLogger) with a descriptive message
including paneId and HINT_SCAN_DELAY_MS, and ensure
pendingHintScans.delete(paneId) still runs; use the existing pendingHintScans,
scanPane, paneId, and HINT_SCAN_DELAY_MS symbols to locate and modify the code.
| export async function recoverScrollback( | ||
| existingScrollback: string | null, | ||
| workspaceId: string, | ||
| paneId: string, | ||
| ): Promise<{ scrollback: string; wasRecovered: boolean }> { | ||
| if (existingScrollback) { | ||
| return { scrollback: existingScrollback, wasRecovered: true }; | ||
| } | ||
|
|
||
| const historyReader = new HistoryReader(workspaceId, paneId); | ||
| const history = await historyReader.read(); | ||
|
|
||
| if (history.scrollback) { | ||
| // Keep only a reasonable amount of scrollback history | ||
| const MAX_SCROLLBACK_CHARS = 500_000; | ||
| const scrollback = | ||
| history.scrollback.length > MAX_SCROLLBACK_CHARS | ||
| ? history.scrollback.slice(-MAX_SCROLLBACK_CHARS) | ||
| : history.scrollback; | ||
| return { scrollback, wasRecovered: true }; | ||
| } | ||
|
|
||
| return { scrollback: "", wasRecovered: false }; | ||
| } |
There was a problem hiding this comment.
Fix clear-scrollback history race (writes can land in the old history file).
On \x1b[3J, onHistoryReinit() runs async, but you immediately write dataToStore to session.historyWriter (old stream). This can make “cleared” content reappear after recovery.
One workable approach (buffer writes until reinit finishes)
export function setupDataHandler(
session: TerminalSession,
initialCommands: string[] | undefined,
wasRecovered: boolean,
onHistoryReinit: () => Promise<void>,
beforeInitialCommands?: Promise<void>,
): void {
+ let historyReinit: Promise<void> | null = null;
+ let pendingHistoryWrites: string[] = [];
+
const initialCommandString =
!wasRecovered && initialCommands && initialCommands.length > 0
? `${initialCommands.join(" && ")}\n`
: null;
let commandsSent = false;
session.pty.onData((data) => {
let dataToStore = data;
if (containsClearScrollbackSequence(data)) {
session.scrollback = "";
- onHistoryReinit().catch(() => {});
+ historyReinit = onHistoryReinit().catch((error) => {
+ console.warn("[TerminalSession/historyReinit] Failed to reinitialize history:", error);
+ });
dataToStore = extractContentAfterClear(data);
}
session.scrollback += dataToStore;
- session.historyWriter?.write(dataToStore);
+ if (historyReinit) {
+ pendingHistoryWrites.push(dataToStore);
+ void historyReinit.finally(() => {
+ // Flush buffered writes after history is reinitialized
+ const buffered = pendingHistoryWrites;
+ pendingHistoryWrites = [];
+ historyReinit = null;
+ for (const chunk of buffered) {
+ session.historyWriter?.write(chunk);
+ }
+ });
+ } else {
+ session.historyWriter?.write(dataToStore);
+ }Also applies to: 142-194
| if (history.scrollback) { | ||
| // Keep only a reasonable amount of scrollback history | ||
| const MAX_SCROLLBACK_CHARS = 500_000; | ||
| const scrollback = | ||
| history.scrollback.length > MAX_SCROLLBACK_CHARS | ||
| ? history.scrollback.slice(-MAX_SCROLLBACK_CHARS) | ||
| : history.scrollback; | ||
| return { scrollback, wasRecovered: true }; | ||
| } | ||
|
|
||
| return { scrollback: "", wasRecovered: false }; |
There was a problem hiding this comment.
Cap in-memory session.scrollback growth during live sessions.
Right now it can grow without bound; recovery truncation (500k chars) doesn’t prevent runtime memory bloat.
Also applies to: 164-166
🤖 Prompt for AI Agents
In @apps/desktop/src/main/lib/terminal/session.ts around lines 31 - 41,
session.scrollback can grow without bound during live sessions; add an in-memory
cap using the same MAX_SCROLLBACK_CHARS logic so any time you append to
session.scrollback or history.scrollback you truncate to the last
MAX_SCROLLBACK_CHARS characters. Locate places that mutate session.scrollback
(and the similar mutation around lines 164-166) and after concatenation or push,
assign session.scrollback = session.scrollback.length > MAX_SCROLLBACK_CHARS ?
session.scrollback.slice(-MAX_SCROLLBACK_CHARS) : session.scrollback (use the
constant MAX_SCROLLBACK_CHARS defined in this file) so runtime memory never
exceeds the cap while keeping the existing recovery truncation logic intact.
| if (containsClearScrollbackSequence(data)) { | ||
| session.scrollback = ""; | ||
| onHistoryReinit().catch(() => {}); | ||
| dataToStore = extractContentAfterClear(data); | ||
| } | ||
|
|
||
| session.scrollback += dataToStore; | ||
| session.historyWriter?.write(dataToStore); | ||
|
|
||
| // Check for hints that a port may have been opened (triggers immediate scan) | ||
| portManager.checkOutputForHint(dataToStore, session.paneId); | ||
|
|
||
| session.dataBatcher.write(data); | ||
|
|
||
| if (initialCommandString && !commandsSent) { | ||
| commandsSent = true; | ||
| 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); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Stop silently swallowing errors in core terminal lifecycle paths.
At minimum log with context so recovery/scrollback bugs can be diagnosed.
As per coding guidelines, never swallow errors silently.
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 141-190: The onHistoryReinit() call currently swallows errors with
an empty catch inside setupDataHandler's session.pty.onData handler; change that
catch to log the error with context instead of ignoring it. Specifically, in
setupDataHandler (inside the session.pty.onData callback) replace .catch(() =>
{}) on onHistoryReinit() with a catch that logs the caught error and useful
context (e.g., session identifier via session.id or similar) using the session
logger if available (session.logger?.error) or fallback to console.error so
reinit failures are observable. Ensure you preserve the async/await flow and do
not rethrow to avoid breaking the handler.
- Around line 215-229: The function can leave the session without a working
writer if close() or init() throws; change reinitializeHistory to first capture
the existing writer (oldWriter), construct a new HistoryWriter and await
newWriter.init(), and only after successful init replace session.historyWriter
with newWriter and then attempt to close oldWriter inside a try/catch so close
failures are logged but don’t remove the new writer; ensure that if
newWriter.init() fails you do not replace the existing session.historyWriter and
surface or log the init error for debugging.
In
@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx:
- Around line 401-403: The onError handlers in restartTerminal and
createOrAttach currently only call setSubscriptionEnabled(true) but do not flush
or clear the queued terminal events; update both onError callbacks to also flush
or clear pending events (e.g., invoke the existing function that drains the
terminal event queue such as flushPendingEvents() or clearPendingEvents(), or
call the method that resets pendingEvents state) immediately after re-enabling
subscriptions so queued events cannot be processed out-of-order or against stale
state.
🧹 Nitpick comments (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (2)
240-269: Consider consolidating duplicate exit handling logic.The exit handling logic (lines 250-268) is duplicated in
flushPendingEvents(lines 357-369). This violates DRY and risks divergence if one is updated without the other.♻️ Suggested refactor - extract exit handling
Extract a helper function to handle exit events consistently:
+const handleExitEvent = ( + xterm: XTerm, + exitCode: number, + paneId: string, + setSubscriptionEnabled: (enabled: boolean) => void, +) => { + isExitedRef.current = true; + setSubscriptionEnabled(false); + xterm.writeln(`\r\n\r\n[Process exited with code ${exitCode}]`); + xterm.writeln("[Press any key to restart]"); + + const currentPane = useTabsStore.getState().panes[paneId]; + if (currentPane?.status === "working" || currentPane?.status === "permission") { + useTabsStore.getState().setPaneStatus(paneId, "idle"); + } +};Then use it in both
handleStreamDataandflushPendingEvents.
272-275: Subscription is always enabled regardless ofsubscriptionEnabledstate.The subscription has
enabled: truehardcoded, meaning events always flow tohandleStreamData. The gating happens inside the handler viasubscriptionEnabledstate. This is intentional to queue events, but could be clarified with a comment explaining this design choice.📝 Suggested comment for clarity
trpc.terminal.stream.useSubscription(paneId, { onData: handleStreamData, - enabled: true, + // Always enabled - events are queued in handleStreamData until terminal is ready + enabled: true, });apps/desktop/src/main/lib/terminal/session.ts (2)
109-120: Consider error handling forhistoryWriter.init()failure.If
historyWriter.init()fails (e.g., disk full, permissions), the session creation continues without history persistence. While theHistoryWriter.write()method handles errors silently, an initialization failure might indicate a more serious issue that should be logged.Based on learnings, errors should not be swallowed silently:
📝 Suggested improvement
-await historyWriter.init(recoveredScrollback || undefined); +try { + await historyWriter.init(recoveredScrollback || undefined); +} catch (error) { + console.error("[terminal/session] Failed to initialize history writer:", error); + // Continue without history persistence +}
168-188: Nested async IIFE with timeout creates complexity.The initial command dispatch uses a
setTimeoutwrapping an async IIFE that races with another timeout. This layered async pattern is hard to follow and test. While functional, consider extracting to a named async function.♻️ Suggested refactor
const dispatchInitialCommands = async ( session: TerminalSession, commandString: string, beforeInitialCommands?: Promise<void>, ) => { 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(commandString); } }; // In setupDataHandler: if (initialCommandString && !commandsSent) { commandsSent = true; setTimeout(() => { if (session.isAlive) { dispatchInitialCommands(session, initialCommandString, beforeInitialCommands); } }, 100); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.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/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/main/lib/terminal/session.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/main/lib/terminal/session.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.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/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/main/lib/terminal/session.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/main/lib/terminal/session.ts
🧠 Learnings (1)
📚 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} : Never swallow errors silently - at minimum log them with context
Applied to files:
apps/desktop/src/main/lib/terminal/session.ts
🧬 Code graph analysis (1)
apps/desktop/src/main/lib/terminal/session.ts (4)
apps/desktop/src/main/lib/terminal-history.ts (2)
HistoryReader(109-145)HistoryWriter(32-107)apps/desktop/src/main/lib/data-batcher.ts (1)
DataBatcher(20-84)apps/desktop/src/main/lib/terminal/types.ts (1)
TerminalSession(5-22)apps/desktop/src/main/lib/terminal-escape-filter.ts (2)
containsClearScrollbackSequence(29-31)extractContentAfterClear(38-46)
⏰ 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 (9)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (5)
42-44: LGTM on pending events queue and subscription gating.The combination of
pendingEventsReffor queuing andsubscriptionEnabledstate for gating provides a clean mechanism to prevent data loss during terminal initialization. Using a ref for the queue avoids unnecessary re-renders.
346-372: Potential stale closure issue withxtermreference.The
flushPendingEventsfunction capturesxtermfrom the outer scope at definition time. If called afterxterm.dispose()but before the component fully unmounts, this could cause issues. However, sinceflushPendingEventsis only called duringonSuccesscallbacks (before cleanup), this should be safe in practice.
374-381: LGTM onapplyInitialStatehelper.Clean extraction that writes scrollback and updates CWD state. The function correctly applies recovered terminal history before enabling the subscription.
383-406: Race condition:setSubscriptionEnabled(false)beforecreateOrAttachRef.current.Setting
subscriptionEnabledtofalseat line 385 will cause any incoming stream events during thecreateOrAttachmutation to be queued, which is the intended behavior. However, there's a subtle issue: React state updates are asynchronous, so thefalsevalue may not be reflected immediately whenhandleStreamDatachecks it.Since
handleStreamDatareads from the state variable directly (not a ref), and React batches updates, events arriving immediately after line 385 might still see the oldtruevalue until the next render cycle.Consider using a ref alongside the state for synchronous gating:
+const subscriptionEnabledRef = useRef(false); const [subscriptionEnabled, setSubscriptionEnabled] = useState(false); +// Keep ref in sync with state +useEffect(() => { + subscriptionEnabledRef.current = subscriptionEnabled; +}, [subscriptionEnabled]); const handleStreamData = (event: TerminalStreamEvent) => { - if (!xtermRef.current || !subscriptionEnabled) { + if (!xtermRef.current || !subscriptionEnabledRef.current) { pendingEventsRef.current.push(event); return; }
560-562: LGTM on cleanup sequence.The cleanup correctly detaches the pane and disables subscription before disposing the terminal. This ensures the main process knows to preserve the PTY for potential reattachment while stopping event processing on the renderer side.
apps/desktop/src/main/lib/terminal/session.ts (4)
18-41: LGTM onrecoverScrollbackfunction.Good implementation with:
- Fallback chain: existing scrollback → history file → empty string
- Memory protection via
MAX_SCROLLBACK_CHARSconstant (500KB limit)- Proper slicing from the end to keep recent history
154-166: Verify clear sequence detection doesn't cause data loss.When a clear scrollback sequence is detected, the code resets
session.scrollbackand stores only the content after the clear. However,session.dataBatcher.write(data)at line 166 writes the full originaldata(notdataToStore). This is intentional—the client needs to see the clear sequence to clear its display—but the asymmetry between what's stored vs. batched could be confusing.A brief comment clarifying this intentional behavior would help future readers:
+// Write full data to batcher (client needs clear sequence for display) +// but store only post-clear content in scrollback/history session.dataBatcher.write(data);
192-213: LGTM oncloseSessionHistoryfunction.The function properly handles both cleanup scenarios:
- Delete history on exit: closes writer, then cleans up files
- Normal exit: closes writer with exit code for metadata
The
undefinedassignment after closing helps with garbage collection.
231-233: LGTM onflushSessionfunction.Simple and correct - delegates to
DataBatcher.dispose()which handles flushing remaining data and cleaning up the decoder.
| export function setupDataHandler( | ||
| session: TerminalSession, | ||
| initialCommands: string[] | undefined, | ||
| wasRecovered: boolean, | ||
| onHistoryReinit: () => Promise<void>, | ||
| beforeInitialCommands?: Promise<void>, | ||
| ): void { | ||
| if (!initialCommands || initialCommands.length === 0) { | ||
| const initialCommandString = | ||
| !wasRecovered && initialCommands && initialCommands.length > 0 | ||
| ? `${initialCommands.join(" && ")}\n` | ||
| : null; | ||
| let commandsSent = false; | ||
|
|
||
| session.pty.onData((data) => { | ||
| let dataToStore = data; | ||
|
|
||
| if (containsClearScrollbackSequence(data)) { | ||
| session.scrollback = ""; | ||
| onHistoryReinit().catch(() => {}); | ||
| dataToStore = extractContentAfterClear(data); | ||
| } | ||
|
|
||
| session.scrollback += dataToStore; | ||
| session.historyWriter?.write(dataToStore); | ||
|
|
||
| session.dataBatcher.write(data); | ||
|
|
||
| if (initialCommandString && !commandsSent) { | ||
| commandsSent = true; | ||
| 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); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Swallowed error in onHistoryReinit callback.
Line 159 swallows errors from onHistoryReinit() with an empty catch. While this prevents the data handler from crashing, it could hide issues with history reinitialization. Based on learnings, errors should at minimum be logged with context.
🐛 Suggested fix
-onHistoryReinit().catch(() => {});
+onHistoryReinit().catch((error) => {
+ console.error("[terminal/session] Failed to reinitialize history:", error);
+});📝 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 setupDataHandler( | |
| session: TerminalSession, | |
| initialCommands: string[] | undefined, | |
| wasRecovered: boolean, | |
| onHistoryReinit: () => Promise<void>, | |
| beforeInitialCommands?: Promise<void>, | |
| ): void { | |
| if (!initialCommands || initialCommands.length === 0) { | |
| const initialCommandString = | |
| !wasRecovered && initialCommands && initialCommands.length > 0 | |
| ? `${initialCommands.join(" && ")}\n` | |
| : null; | |
| let commandsSent = false; | |
| session.pty.onData((data) => { | |
| let dataToStore = data; | |
| if (containsClearScrollbackSequence(data)) { | |
| session.scrollback = ""; | |
| onHistoryReinit().catch(() => {}); | |
| dataToStore = extractContentAfterClear(data); | |
| } | |
| session.scrollback += dataToStore; | |
| session.historyWriter?.write(dataToStore); | |
| session.dataBatcher.write(data); | |
| if (initialCommandString && !commandsSent) { | |
| commandsSent = true; | |
| 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); | |
| } | |
| }); | |
| } | |
| export function setupDataHandler( | |
| session: TerminalSession, | |
| initialCommands: string[] | undefined, | |
| wasRecovered: boolean, | |
| onHistoryReinit: () => Promise<void>, | |
| beforeInitialCommands?: Promise<void>, | |
| ): void { | |
| const initialCommandString = | |
| !wasRecovered && initialCommands && initialCommands.length > 0 | |
| ? `${initialCommands.join(" && ")}\n` | |
| : null; | |
| let commandsSent = false; | |
| session.pty.onData((data) => { | |
| let dataToStore = data; | |
| if (containsClearScrollbackSequence(data)) { | |
| session.scrollback = ""; | |
| onHistoryReinit().catch((error) => { | |
| console.error("[terminal/session] Failed to reinitialize history:", error); | |
| }); | |
| dataToStore = extractContentAfterClear(data); | |
| } | |
| session.scrollback += dataToStore; | |
| session.historyWriter?.write(dataToStore); | |
| session.dataBatcher.write(data); | |
| if (initialCommandString && !commandsSent) { | |
| commandsSent = true; | |
| 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); | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
In @apps/desktop/src/main/lib/terminal/session.ts around lines 141 - 190, The
onHistoryReinit() call currently swallows errors with an empty catch inside
setupDataHandler's session.pty.onData handler; change that catch to log the
error with context instead of ignoring it. Specifically, in setupDataHandler
(inside the session.pty.onData callback) replace .catch(() => {}) on
onHistoryReinit() with a catch that logs the caught error and useful context
(e.g., session identifier via session.id or similar) using the session logger if
available (session.logger?.error) or fallback to console.error so reinit
failures are observable. Ensure you preserve the async/await flow and do not
rethrow to avoid breaking the handler.
| export async function reinitializeHistory( | ||
| session: TerminalSession, | ||
| ): Promise<void> { | ||
| if (session.historyWriter) { | ||
| await session.historyWriter.close(); | ||
| session.historyWriter = new HistoryWriter( | ||
| session.workspaceId, | ||
| session.paneId, | ||
| session.cwd, | ||
| session.cols, | ||
| session.rows, | ||
| ); | ||
| await session.historyWriter.init(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing error handling in reinitializeHistory.
If historyWriter.close() or historyWriter.init() throws, the function will propagate the error. This could leave the session in an inconsistent state where the old writer is closed but no new writer exists.
🐛 Suggested fix
export async function reinitializeHistory(
session: TerminalSession,
): Promise<void> {
if (session.historyWriter) {
- await session.historyWriter.close();
- session.historyWriter = new HistoryWriter(
- session.workspaceId,
- session.paneId,
- session.cwd,
- session.cols,
- session.rows,
- );
- await session.historyWriter.init();
+ try {
+ await session.historyWriter.close();
+ } catch (error) {
+ console.error("[terminal/session] Failed to close history writer:", error);
+ }
+ try {
+ session.historyWriter = new HistoryWriter(
+ session.workspaceId,
+ session.paneId,
+ session.cwd,
+ session.cols,
+ session.rows,
+ );
+ await session.historyWriter.init();
+ } catch (error) {
+ console.error("[terminal/session] Failed to reinitialize history writer:", error);
+ session.historyWriter = undefined;
+ }
}
}📝 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 async function reinitializeHistory( | |
| session: TerminalSession, | |
| ): Promise<void> { | |
| if (session.historyWriter) { | |
| await session.historyWriter.close(); | |
| session.historyWriter = new HistoryWriter( | |
| session.workspaceId, | |
| session.paneId, | |
| session.cwd, | |
| session.cols, | |
| session.rows, | |
| ); | |
| await session.historyWriter.init(); | |
| } | |
| } | |
| export async function reinitializeHistory( | |
| session: TerminalSession, | |
| ): Promise<void> { | |
| if (session.historyWriter) { | |
| try { | |
| await session.historyWriter.close(); | |
| } catch (error) { | |
| console.error("[terminal/session] Failed to close history writer:", error); | |
| } | |
| try { | |
| session.historyWriter = new HistoryWriter( | |
| session.workspaceId, | |
| session.paneId, | |
| session.cwd, | |
| session.cols, | |
| session.rows, | |
| ); | |
| await session.historyWriter.init(); | |
| } catch (error) { | |
| console.error("[terminal/session] Failed to reinitialize history writer:", error); | |
| session.historyWriter = undefined; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In @apps/desktop/src/main/lib/terminal/session.ts around lines 215 - 229, The
function can leave the session without a working writer if close() or init()
throws; change reinitializeHistory to first capture the existing writer
(oldWriter), construct a new HistoryWriter and await newWriter.init(), and only
after successful init replace session.historyWriter with newWriter and then
attempt to close oldWriter inside a try/catch so close failures are logged but
don’t remove the new writer; ensure that if newWriter.init() fails you do not
replace the existing session.historyWriter and surface or log the init error for
debugging.
| onError: () => { | ||
| setSubscriptionEnabled(true); | ||
| }, |
There was a problem hiding this comment.
Error callback enables subscription but doesn't flush pending events.
In both restartTerminal (lines 401-403) and initial createOrAttach (lines 482-484), the onError callback only enables the subscription without flushing pending events. If events were queued during the failed mutation, they will remain in the queue and could be processed out of order or with stale terminal state.
🐛 Suggested fix
Consider clearing the pending events on error, or flushing them:
onError: () => {
setSubscriptionEnabled(true);
+ // Clear pending events on error to avoid stale data
+ pendingEventsRef.current = [];
},Also applies to: 482-484
🤖 Prompt for AI Agents
In
@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
around lines 401 - 403, The onError handlers in restartTerminal and
createOrAttach currently only call setSubscriptionEnabled(true) but do not flush
or clear the queued terminal events; update both onError callbacks to also flush
or clear pending events (e.g., invoke the existing function that drains the
terminal event queue such as flushPendingEvents() or clearPendingEvents(), or
call the method that resets pendingEvents state) immediately after re-enabling
subscriptions so queued events cannot be processed out-of-order or against stale
state.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
* Revert terminal addon change * Remove port hints
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.