Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds NDJSON-backed terminal history persistence, integrates HistoryReader/HistoryWriter into TerminalManager, makes createOrAttach async and return a wasRecovered flag, extends kill to accept deleteHistory, updates TRPC router and renderer to await/recover and queue events, and adds tests for history and recovery behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Renderer UI
participant Router as TRPC Router
participant Manager as TerminalManager
participant Reader as HistoryReader
participant Writer as HistoryWriter
participant FS as File System
rect `#eaf5ea`
Note over UI,Router: Client requests terminal attach/create
UI->>Router: createOrAttach(tabId, workspaceId, ...)
Router->>Manager: await createOrAttach(...)
end
rect `#f7f7d9`
Note over Manager,Reader: Attempt recovery from disk
Manager->>Reader: getLatestSession()
Reader->>FS: read NDJSON history & metadata
Reader-->>Manager: { scrollback, wasRecovered?, metadata? }
end
rect `#e7f0fb`
Note over Manager,Writer: Prepare writer and resume session
Manager->>Writer: new HistoryWriter(...).init()
Manager-->>Router: { isNew, scrollback, wasRecovered }
Router-->>UI: result
end
rect `#f8e6f1`
Note over Manager,Writer: Persist PTY events and exit
loop PTY data events
Manager->>Writer: writeData(data)
Writer->>FS: append NDJSON data event (base64)
end
Manager->>Writer: writeExit(exitCode)
Writer->>FS: append exit event & write metadata
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 2
🧹 Nitpick comments (5)
apps/desktop/src/main/lib/terminal-manager.test.ts (1)
339-417: History persistence test relies on fixed timeouts; consider a more deterministic hookThe multi-session history test waits for async work using hard-coded
setTimeoutcalls (10 ms and several 50 ms sleeps) after invoking exit callbacks and between sessions. On slower CI or under load, these timings may not be sufficient and could introduce flaky behavior if history writing occasionally takes longer.If possible, prefer a deterministic signal, for example:
- Expose a promise or callback from the history writer/manager that resolves when all pending writes are flushed, or
- Inject a fake/in-memory
HistoryWriterin tests that you can await directly instead of sleeping.That would make the persistence tests robust without depending on timing assumptions.
apps/desktop/src/main/lib/terminal-manager.ts (2)
37-48: AsynccreateOrAttach+ recovery flow is coherent; consider DRY-ing scrollback initializationThe async
createOrAttachflow (recover history, spawn PTY, init writer, hydrate session + return shape) is consistent and preserves the existing scrollback representation (single-element array containing concatenated data). The recovery handling andwasRecoveredflag are clear.You duplicate the recovery expression for both
session.scrollbackand the returnedscrollback. You could centralize this in a small helper (e.g.const initialScrollback = recovery.wasRecovered && recovery.scrollback ? [recovery.scrollback] : [];) to avoid divergence if the shape changes later.Also applies to: 68-71, 80-89, 98-104, 133-140
106-112: PTY data streaming to history ignores backpressureWriting every
onDatachunk directly tohistoryWriter.writeDataworks functionally, but there’s no consideration for stream backpressure (i.e.,Writable.writereturningfalse). For very chatty sessions this could lead to buffered writes growing without flow control.If you expect heavy output, consider either:
- Allowing
HistoryWriter.writeDatato be async and awaiting drain when.writereturnsfalse, or- Accepting potential truncation/skip-on-pressure semantics explicitly (e.g., by dropping chunks when
writefails) and documenting that behavior.You may want to confirm expected throughput characteristics for
node-ptyin your desktop environment and whether write backpressure to disk has shown up in profiling.apps/desktop/src/main/lib/terminal-history.ts (2)
52-107:HistoryWritermetadata byte length and backpressure handling could be improvedTwo minor points in the writer implementation:
byteLengthaccounting
this.byteLength += data.length;uses JavaScript string length (UTF‑16 code units) rather than actual byte length. Since the PTY data is later converted viaBuffer.from(data).toString("base64"), the more accurate accounting would beBuffer.byteLength(data)(with the appropriate encoding). This only affects metadata, not core behavior, but it may be misleading if someone interprets it as bytes-on-disk.Backpressure on
writeStream
- As in
terminal-manager.ts,this.writeStream.write(line);doesn’t check the return value or listen for"drain", so high-volume output can grow the internal buffer without any throttling. Depending on expected workloads, you may want to either:
- Respect backpressure (convert
writeDatainto an async method that awaits"drain"when.writereturnsfalse), or- Explicitly accept that this is best-effort logging and document that behavior.
Neither issue is blocking for correctness, but both are worth tightening if you plan to surface metadata or run long-lived, noisy sessions.
You might want to verify whether
SessionMetadata.byteLengthwill be surfaced in any UI or tooling; if so, usingBuffer.byteLengthwould provide more accurate numbers.Also applies to: 127-141
160-205: HistoryReader is robust; note full-file scan and differing scrollback capsThe reader implementation is solid, with good error handling (access checks, metadata read guarded, malformed lines skipped, decode failures logged but not thrown). A couple of behavioral notes:
decodeHistorystreams and parses the entire NDJSON file, capping the in-memory scrollback to 100k characters. This keeps memory bounded but still requires scanning very large history files fully. If you expect extremely long-lived sessions, you might eventually consider a strategy that seeks near the end of the file (e.g., last N bytes) rather than decoding all events.- The persisted history cap (100k chars) differs from the in-memory scrollback cap in
TerminalManager.addToScrollback(50k chars). That may be intentional (persist more history than you keep in memory), but if not, you might want to align them or at least document the discrepancy.Functionally, this is good enough to ship and should behave well under normal workloads.
Also applies to: 206-246
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/desktop/src/lib/trpc/routers/projects/projects.ts(1 hunks)apps/desktop/src/lib/trpc/routers/terminal/terminal.ts(3 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts(1 hunks)apps/desktop/src/main/lib/terminal-history.ts(1 hunks)apps/desktop/src/main/lib/terminal-manager.test.ts(18 hunks)apps/desktop/src/main/lib/terminal-manager.ts(10 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/desktop/src/main/lib/terminal-history.ts (1)
apps/desktop/src/main/lib/terminal-manager.ts (1)
signal(177-190)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
apps/desktop/src/main/lib/terminal-manager.ts (1)
terminalManager(313-313)
apps/desktop/src/main/lib/terminal-manager.ts (1)
apps/desktop/src/main/lib/terminal-history.ts (2)
HistoryWriter(52-154)HistoryReader(160-256)
🔇 Additional comments (7)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
6-7: Import reordering only; no behavioral impact
nanoidandzare already used below; this just normalizes import order with no runtime change, looks good.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
2-2: DB import consolidation looks goodUsing a single
dbimport from"main/lib/db"is a straightforward cleanup with no behavioral change.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
128-133: Recovered-session branch cleanly distinguishes history recoveryUsing
result.wasRecoveredto special-case recovered scrollback (with a dim header) while preserving the existing!result.isNewbehavior for live reattachment looks correct and keeps previous semantics intact.apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
4-5: Router correctly awaits TerminalManager and exposeswasRecoveredAwaiting
terminalManager.createOrAttachand plumbing throughwasRecoveredaligns this router with the updated manager/tests and keeps the response shape compatible with the Terminal UI consumer.Please confirm
TerminalManager.createOrAttachalways returns a booleanwasRecovered(neverundefined) so the tRPC-inferred types and clients remain consistent.Also applies to: 38-51
apps/desktop/src/main/lib/terminal-manager.ts (1)
4-17: History integration intoTerminalSessionlooks structurally soundImporting
HistoryReader/HistoryWriterand threadinghistoryWriter?: HistoryWriterintoTerminalSessionis a reasonable way to keep per-session history state co-located with PTY metadata; optional typing is appropriate for future extensibility and non-history use cases.apps/desktop/src/main/lib/terminal-history.ts (2)
6-46: History event/schema and path helpers are reasonable and clearThe event types, session metadata shape, and history directory layout under
~/.superset/terminal-history/<workspaceId>/<tabId>are straightforward and self-describing. This should be easy to introspect/debug if needed.
248-255:cleanup()behavior is straightforward and suitable for tab-level history deletionUsing
fs.rm(dir, { recursive: true, force: true })is appropriate for per-tab history cleanup, and the try/catch with logging avoids crashes on I/O errors (e.g., already-deleted dirs). This aligns well with theTerminalManager.killsemantics of permanently deleting history.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
apps/desktop/src/main/lib/terminal-manager.ts (2)
115-121: Race condition betweenonExitandkill()remains unaddressed.This is the same critical race condition identified in the previous review. When
kill()is called on a live session:
kill()callspty.kill()then immediately callshistoryWriter.finalize()- The PTY exit event can fire while
finalize()is in progressonExitcheckshistoryWriter.isOpen()which returns true (stream not null yet)onExitcallswriteExit()which attempts to write afterend()was called- This throws
ERR_STREAM_WRITE_AFTER_ENDThe solution suggested in the previous review remains valid: implement a centralized
finalizeHistory()helper with ahistoryFinalizedflag to ensure only one code path performs finalization. Alternatively, setsession.historyWriter = undefinedat the start of finalization to prevent concurrent access.+ private async finalizeHistory( + session: TerminalSession, + options: { exitCode?: number; signal?: number; cleanup: boolean } + ): Promise<void> { + if (!session.historyWriter) return; + const writer = session.historyWriter; + session.historyWriter = undefined; // Prevent concurrent finalization + + if (options.exitCode !== undefined || options.signal !== undefined) { + await writer.writeExit(options.exitCode, options.signal); + } else if (writer.isOpen()) { + await writer.finalize(); + } + + if (options.cleanup) { + const historyReader = new HistoryReader(session.workspaceId, session.tabId); + await historyReader.cleanup(); + } + }Then call it from both
onExit(withcleanup: false) andkill(withcleanup: deleteHistory).
208-217: Race condition: concurrent finalization inkill()andonExit.As noted in the previous review comment,
kill()can race with theonExithandler. Both paths checkisOpen()and attempt to finalize, butisOpen()has a timing window that allows both to proceed.Use the centralized
finalizeHistory()approach mentioned in the previous comment to ensure only one finalization path executes per session.apps/desktop/src/main/lib/terminal-manager.test.ts (5)
23-31: Tests still delete real user terminal history.This is the same major issue identified in the previous review. The
beforeEachhook usesjoin(homedir(), ".superset", "terminal-history")and callsfs.rm(..., {recursive: true, force: true})on it. Running these tests will delete a developer's actual terminal history.The solution from the previous review remains valid: make the history directory configurable (via environment variable or parameter) and point tests to a temporary directory:
+const TEST_HISTORY_DIR = join(os.tmpdir(), 'superset-test-history-' + Date.now()); + beforeEach(async () => { // Clean up test history files before each test - const historyDir = join(homedir(), ".superset", "terminal-history"); + const historyDir = TEST_HISTORY_DIR; try { await fs.rm(historyDir, { recursive: true, force: true });This requires making the history directory configurable in
terminal-history.tsas suggested in an earlier comment.
57-68:afterEachalso deletes real user history.Same issue as in
beforeEach: this cleanup will delete real user data.Use a test-specific temporary directory as suggested in the previous comment.
248-258: History directory verification uses real user path.This test verifies the history directory exists after kill without
deleteHistory. While the test logic is correct, it usesjoin(homedir(), ".superset", "terminal-history", ...)which references the real user directory.Once the history directory is made configurable (as suggested in earlier comments), update this test to use the test-specific directory:
- const historyDir = join( - homedir(), - ".superset", - "terminal-history", - "workspace-1", - "tab-1", - ); + const historyDir = join(TEST_HISTORY_DIR, "workspace-1", "tab-1");
273-287: History cleanup verification uses real user path.This test verifies history deletion works correctly, but references the real user history directory.
Update to use the test-specific directory once configurability is added.
422-428: Multi-session test also uses real user history path.The test correctly exercises multi-session persistence but constructs the path using
homedir().Update to use test-specific directory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts(4 hunks)apps/desktop/src/main/lib/terminal-history.test.ts(1 hunks)apps/desktop/src/main/lib/terminal-history.ts(1 hunks)apps/desktop/src/main/lib/terminal-manager.test.ts(17 hunks)apps/desktop/src/main/lib/terminal-manager.ts(10 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(2 hunks)apps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/main/lib/terminal-history.test.ts
🧰 Additional context used
🧬 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-manager.ts (1)
apps/desktop/src/main/lib/terminal-history.ts (2)
HistoryWriter(52-167)HistoryReader(173-292)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
apps/desktop/src/main/lib/terminal-manager.ts (1)
terminalManager(318-318)
🔇 Additional comments (5)
apps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.ts (1)
5-12: LGTM! Terminal history deletion properly integrated.The addition of
deleteHistory: truecorrectly integrates with the new history persistence system, ensuring terminal history is cleaned up when tabs are closed.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
128-139: LGTM! Recovery flow properly sequenced.The recovery handling correctly:
- Applies recovered scrollback before enabling the subscription
- Displays a user-visible recovery indicator
- Falls back to the previous behavior for non-recovered sessions
- Guards array access with length checks
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (2)
38-50: LGTM! Async operations and recovery flag properly integrated.The changes correctly:
- Await the async
createOrAttachoperation- Propagate the
wasRecoveredflag to the client- Maintain backward compatibility with existing fields
93-98: LGTM! History deletion flag properly wired through.The
deleteHistoryparameter is correctly added to the input schema and awaited in the mutation, enabling controlled history cleanup on terminal kill.apps/desktop/src/main/lib/terminal-manager.test.ts (1)
420-506: Excellent test coverage for multi-session persistence.The multi-session test effectively validates:
- History is written and finalized in session 1
- Session 2 recovers session 1's output (
wasRecoveredis true)- Session 3 recovers both sessions' output (appending behavior)
This provides strong evidence the persistence system works correctly across multiple sessions.
Once the history directory is made configurable and uses a test directory, this test will be production-ready.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apps/desktop/src/main/lib/terminal-manager.ts (1)
118-138: cleanup() can still race with onExit/history finalization; rely on an idempotent HistoryWriter or centralize finalization.
kill()no longer callshistoryWriter.finalize(), which removes the earlier direct race withonExit. However,cleanup()now finalizes writers for non‑alive sessions:// in cleanup() } else { // For sessions that already exited, ensure their writers are finalized if (session.historyWriter?.isOpen()) { await session.historyWriter.finalize(); } }While
onExitdoes:ptyProcess.onExit(async ({ exitCode, signal }) => { session.isAlive = false; if (session.historyWriter?.isOpen()) { await session.historyWriter.writeExit(exitCode, signal); } ... });With the current
HistoryWriter:
writeExit()itself callsfinalize(), andisOpen()returnstruebetweenend()and"finish".If
cleanup()runs whileonExitis mid‑writeExit()/finalize(), theelsebranch will also seeisOpen() === trueand callfinalize()a second time against a stream that has already hadend()invoked, which can produceERR_STREAM_WRITE_AFTER_END.Given the comment on
HistoryWriter:
- Make
HistoryWriter.finalize()idempotent and guarded by aclosing/finalizePromiseflag, and- Have
isOpen()returnfalseimmediately once finalization starts.Once that’s in place, this
cleanup()pattern is safe: concurrent calls fromonExitandcleanup()will all share the same finalization promise instead of double‑ending the stream.If you’d rather keep the writer simple, an alternative is to centralize finalization in a helper (as suggested in the earlier review) and ensure:
onExitandcleanup()both call that helper, and- the helper clears
session.historyWriterand/or ahistoryFinalizedflag on first entry so subsequent callers become no‑ops.Either approach removes the double‑finalization race while preserving the intended semantics of
kill,cleanup, and history deletion.Also applies to: 201-229, 260-293
apps/desktop/src/main/lib/terminal-history.ts (2)
34-45: Hardcoded history directory risks deleting real user data; make base dir configurable.
getHistoryDir()currently bakes injoin(homedir(), ".superset", "terminal-history", ...). The tests interminal-manager.test.tscallfs.rmon the same path, so running the suite will happily wipe a developer’s real~/.superset/terminal-historydirectory.Make the base directory configurable (env or injectable) and have tests point to a temp dir instead of the real home:
-import { homedir } from "node:os"; -import { join } from "node:path"; +import { homedir } from "node:os"; +import { join } from "node:path"; + +// Allow tests/consumers to override the history base directory +const HISTORY_BASE_DIR = + process.env.SUPERSET_TERMINAL_HISTORY_DIR ?? + join(homedir(), ".superset", "terminal-history"); // History directory structure export function getHistoryDir(workspaceId: string, tabId: string): string { - return join(homedir(), ".superset", "terminal-history", workspaceId, tabId); + return join(HISTORY_BASE_DIR, workspaceId, tabId); }Once this is in place, update
terminal-manager.test.tsto either:
- set
process.env.SUPERSET_TERMINAL_HISTORY_DIRto a temp directory inbeforeEachand clean that up, or- derive paths via
getHistoryDir()rather than recomputingjoin(homedir(), ...), so tests can never touch real user data.
52-165: HistoryWriter finalization is not race‑safe; concurrent finalize/writeExit can causeERR_STREAM_WRITE_AFTER_END.
writeExit()writes to the stream then callsfinalize().TerminalManager.cleanup()also callshistoryWriter.finalize()for non‑alive sessions. With no explicit finalization state:
finalize()can be entered twice for the same stream (e.g., fromwriteExit()and fromcleanup()), callingend()multiple times.isOpen()returnstrueafterend()is called and before"finish"fires, so callers may still invokewriteExit()/finalize()on an already‑ended stream.This is essentially the same race previously flagged (now between
onExitandcleanup) and can surface asERR_STREAM_WRITE_AFTER_ENDand unhandled rejections.Make finalization explicitly idempotent and close the
isOpen()window:export class HistoryWriter { private writeStream: ReturnType<typeof createWriteStream> | null = null; private byteLength = 0; private metadata: SessionMetadata; private filePath: string; private metaPath: string; + private closing = false; + private finalizePromise: Promise<void> | null = null; @@ async writeExit(exitCode?: number, signal?: number): Promise<void> { - if (!this.writeStream) { + if (!this.writeStream || this.closing) { console.warn("HistoryWriter not initialized"); return; } @@ - const line = `${JSON.stringify(event)}\n`; - this.writeStream.write(line); - - await this.finalize(exitCode); + const line = `${JSON.stringify(event)}\n`; + this.writeStream.write(line); + + await this.finalize(exitCode); } async finalize(exitCode?: number): Promise<void> { - if (this.writeStream) { - this.writeStream.end(); - await new Promise<void>((resolve) => { - this.writeStream?.on("finish", () => resolve()); - }); - this.writeStream = null; - } + // If a finalization is already in progress, just wait for it. + if (this.closing && this.finalizePromise) { + await this.finalizePromise; + return; + } + + if (!this.writeStream) { + return; + } + + this.closing = true; + + const stream = this.writeStream; + this.finalizePromise = new Promise<void>((resolve) => { + stream.on("finish", () => resolve()); + }); + + stream.end(); + await this.finalizePromise; + + this.writeStream = null; + this.closing = false; // Update final metadata this.metadata.endedAt = new Date().toISOString(); @@ } isOpen(): boolean { - return this.writeStream !== null; + // Treat the stream as "closed" as soon as finalization starts. + return this.writeStream !== null && !this.closing; } }With this in place,
TerminalManagercan safely callwriteExit()inonExitandfinalize()fromcleanup()without double‑ending the same stream or writing afterend().apps/desktop/src/main/lib/terminal-manager.test.ts (1)
2-4: Tests still delete the real~/.superset/terminal-historydirectory; point them at a temp dir via shared helpers.The tests are still building paths with:
const historyDir = join(homedir(), ".superset", "terminal-history", ...), andawait fs.rm(historyDir, { recursive: true, force: true })inbeforeEach,afterEach, and the multi‑session test.Given production code uses the same base path, running this suite will happily wipe real terminal history under a developer’s home directory.
With
terminal-history.tsupdated to support a configurable base directory (e.g.SUPERSET_TERMINAL_HISTORY_DIRand/or aHISTORY_BASE_DIRconstant), tests should never touch the real home path.A safe pattern would be:
- Use the same helper as production (so paths stay in sync):
-import { promises as fs } from "node:fs"; -import { homedir } from "node:os"; -import { join } from "node:path"; +import { promises as fs } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { HistoryReader, getHistoryDir } from "./terminal-history"; - import { HistoryReader } from "./terminal-history";
- Force a test‑only base directory via env for the duration of each test:
let manager: TerminalManager; @@ - beforeEach(async () => { - // Clean up test history files before each test - const historyDir = join(homedir(), ".superset", "terminal-history"); - try { - await fs.rm(historyDir, { recursive: true, force: true }); - } catch { - // Ignore errors if directory doesn't exist - } - - manager = new TerminalManager(); + const TEST_HISTORY_BASE_DIR = join( + tmpdir(), + "superset-terminal-history-test", + ); + + beforeEach(async () => { + process.env.SUPERSET_TERMINAL_HISTORY_DIR = TEST_HISTORY_BASE_DIR; + await fs.rm(TEST_HISTORY_BASE_DIR, { recursive: true, force: true }); + + manager = new TerminalManager(); @@ - afterEach(async () => { - await manager.cleanup(); - mock.restore(); - - // Clean up test history files - const historyDir = join(homedir(), ".superset", "terminal-history"); - try { - await fs.rm(historyDir, { recursive: true, force: true }); - } catch { - // Ignore errors if directory doesn't exist - } - }); + afterEach(async () => { + await manager.cleanup(); + mock.restore(); + + await fs.rm(TEST_HISTORY_BASE_DIR, { recursive: true, force: true }); + delete process.env.SUPERSET_TERMINAL_HISTORY_DIR; + });
- Use
getHistoryDir()wherever you assert on the history directory instead of rebuilding the path:- const historyDir = join( - homedir(), - ".superset", - "terminal-history", - "workspace-1", - "tab-1", - ); + const historyDir = getHistoryDir("workspace-1", "tab-1"); @@ - const historyDir = join( - homedir(), - ".superset", - "terminal-history", - "workspace-1", - "tab-delete-history", - ); + const historyDir = getHistoryDir("workspace-1", "tab-delete-history"); @@ - const historyDir = join( - homedir(), - ".superset", - "terminal-history", - "workspace-1", - "tab-cleanup", - ); + const historyDir = getHistoryDir("workspace-1", "tab-cleanup"); @@ - const historyDir = join( - homedir(), - ".superset", - "terminal-history", - "workspace-1", - "tab-multi", - ); + const historyDir = getHistoryDir("workspace-1", "tab-multi");With this, tests operate entirely in a temp directory controlled by the env override and cannot affect real user data, while still validating the same layout that production uses.
Also applies to: 24-32, 58-69, 236-271, 273-313, 315-355, 396-469, 520-613
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
39-51: Queued subscription events during restart are never flushed.On initial mount you correctly queue stream events until recovery is applied, then call
flushPendingEvents()in thecreateOrAttachonSuccesshandler. For restarts,restartTerminal()also queues events whilesubscriptionEnabledisfalse, but itsonSuccessonly callssetSubscriptionEnabled(true)— any data that arrived between the restart call andonSuccessstays inpendingEventsRefforever.This can drop the initial prompt/output after a restart.
const restartTerminal = () => { isExitedRef.current = false; setSubscriptionEnabled(false); xterm.clear(); createOrAttachRef.current( @@ }, { - onSuccess: () => { - setSubscriptionEnabled(true); - }, + onSuccess: () => { + // Allow live processing and replay anything queued while restarting + setSubscriptionEnabled(true); + flushPendingEvents(); + }, }, ); };Alternatively, you could drive flushing from a
useEffectthat runs wheneversubscriptionEnabledflips totrue, but wiring it into bothcreateOrAttachpaths keeps the intent explicit.Also applies to: 79-115, 133-146
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts(4 hunks)apps/desktop/src/main/lib/terminal-history.ts(1 hunks)apps/desktop/src/main/lib/terminal-manager.test.ts(15 hunks)apps/desktop/src/main/lib/terminal-manager.ts(10 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/desktop/src/main/lib/terminal-manager.test.ts (1)
apps/desktop/src/main/lib/terminal-history.ts (1)
HistoryReader(172-291)
apps/desktop/src/main/lib/terminal-manager.ts (1)
apps/desktop/src/main/lib/terminal-history.ts (2)
HistoryWriter(52-166)HistoryReader(172-291)
apps/desktop/src/main/lib/terminal-history.ts (1)
apps/desktop/src/main/lib/terminal-manager.ts (1)
signal(186-199)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/main/lib/terminal-history.test.ts (1)
1-4: Tests still operate under real~/.superset/terminal-historypathsThese tests call
getHistoryDir(which is homedir‑based) and thenfs.rm(historyDir, { recursive: true, force: true })inbeforeEach/afterEach, so they are creating and deleting history directories under the actual~/.superset/terminal-historytree. Even though you only touch specific workspace/tab IDs, this can still destroy or mutate real user history if IDs collide.Consider making the history base directory configurable (e.g., via an env var or injectable base path) and pointing tests at a temp directory, or mocking
getHistoryDir/getHistoryFilePath/getMetadataPathhere as you already do interminal-manager.test.ts, so tests never touch real user data.Also applies to: 19-34, 168-183, 311-326
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/terminal-manager.test.ts (1)
5-37: Test harness and coverage forTerminalManager+ history look solidMocking
terminal-historyandnode-ptyavoids touching real user data and gives good coverage across lifecycle (create/attach, resize, signal, kill with/withoutdeleteHistory, cleanup, events, multi-session history). You may want a follow‑up test forkill({ deleteHistory: true })when the session is already dead, once the implementation semantics for that case are clarified.Also applies to: 54-101, 262-359, 399-583
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts(4 hunks)apps/desktop/src/main/index.ts(1 hunks)apps/desktop/src/main/lib/terminal-history.test.ts(1 hunks)apps/desktop/src/main/lib/terminal-history.ts(1 hunks)apps/desktop/src/main/lib/terminal-manager.test.ts(15 hunks)apps/desktop/src/main/lib/terminal-manager.ts(10 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/main/lib/terminal-history.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
apps/desktop/src/main/index.ts (1)
apps/desktop/src/main/lib/terminal-manager.ts (1)
terminalManager(376-376)
apps/desktop/src/main/lib/terminal-manager.test.ts (1)
apps/desktop/src/main/lib/terminal-manager.ts (2)
TerminalManager(35-374)signal(181-194)
apps/desktop/src/main/lib/terminal-manager.ts (1)
apps/desktop/src/main/lib/terminal-history.ts (2)
HistoryWriter(45-179)HistoryReader(181-293)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
apps/desktop/src/main/lib/terminal-manager.ts (1)
terminalManager(376-376)
apps/desktop/src/main/lib/terminal-history.test.ts (1)
apps/desktop/src/main/lib/terminal-history.ts (6)
getHistoryDir(31-33)HistoryWriter(45-179)getMetadataPath(40-43)SessionMetadata(21-29)getHistoryFilePath(35-38)HistoryReader(181-293)
🔇 Additional comments (3)
apps/desktop/src/main/index.ts (1)
42-44: Awaiting terminal cleanup onbefore-quitlooks correctAwaiting
terminalManager.cleanup()here aligns with the new async cleanup behavior and ensures history finalization runs before shutdown.apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
4-5: Router wiring toTerminalManager’s async API and history flags is soundThe mutation now correctly awaits
createOrAttach, exposeswasRecovered, and forwardsdeleteHistoryintokillwhile awaiting it; this matches the updatedTerminalManagercontract.Also applies to: 38-51, 89-98
apps/desktop/src/main/lib/terminal-manager.ts (1)
4-20: History integration and centralized finalization logic look correctAsync
createOrAttachwithHistoryReader/HistoryWriter, recovery intoscrollback/wasRecovered, and the newfinalizeHistoryhelper give a coherent lifecycle and address the earlier race betweenkill()andonExiton the history writer.cleanup()’s exit‑promise pattern plus 2s timeout is also a reasonable guard for app shutdown.Also applies to: 40-45, 71-107, 109-133, 251-289, 347-373
| async kill(params: { tabId: string; deleteHistory?: boolean }): Promise<void> { | ||
| const { tabId, deleteHistory = false } = params; | ||
| const session = this.sessions.get(tabId); | ||
|
|
||
| if (!session) { | ||
| console.warn(`Cannot kill terminal ${tabId}: session not found`); | ||
| return; | ||
| } | ||
|
|
||
| if (deleteHistory) { | ||
| session.deleteHistoryOnExit = true; | ||
| } | ||
|
|
||
| if (session.isAlive) { | ||
| session.pty.kill(); | ||
| } else { | ||
| // If already dead, finalize and cleanup immediately since exit handler won't run | ||
| await this.finalizeHistory(session, { | ||
| exitCode: undefined, | ||
| signal: undefined, | ||
| cleanupDir: deleteHistory, | ||
| }); | ||
| this.sessions.delete(tabId); | ||
| } | ||
|
|
||
| this.sessions.delete(tabId); | ||
| } |
There was a problem hiding this comment.
kill({ deleteHistory: true }) does not clean history if called after exit
Currently, if the session has already exited and finalizeHistory has run (so historyFinalized is true and historyWriter is cleared), a later call to:
await manager.kill({ tabId, deleteHistory: true });will hit the !session.isAlive branch, call finalizeHistory with cleanupDir: true, and immediately return from finalizeHistory due to historyFinalized, so HistoryReader.cleanup() is never invoked. In other words, the “delete history” behavior only works when kill is called while session.isAlive.
If you intend deleteHistory to apply even when the process is already dead (but the session is still tracked), consider special‑casing this:
- In
kill, when!session.isAlive && deleteHistory, callHistoryReader.cleanup()directly ifhistoryFinalizedis already true, or - Extend
finalizeHistoryso that it can still perform directory cleanup whenparams.cleanupDiristrueeven if the writer portion has already been finalized.
That would make deleteHistory semantics consistent regardless of whether the user closes the terminal before or after the underlying PTY exits.
Also applies to: 347-373
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/terminal-manager.ts around lines 196-220 (and
similarly 347-373), the kill handler currently skips history directory cleanup
when the session has already exited because finalizeHistory returns early if
historyFinalized is true; update kill so that when !session.isAlive &&
deleteHistory you explicitly trigger cleanup: if session.historyFinalized is
true call session.historyReader?.cleanup() (or equivalent
HistoryReader.cleanup()) and then remove the session from this.sessions;
otherwise call finalizeHistory(session, { exitCode: undefined, signal:
undefined, cleanupDir: true }) as before; alternatively, you may change
finalizeHistory to always perform directory cleanup when cleanupDir is true even
if the writer is already finalized — implement one of these two fixes and apply
the same change to the other block at lines 347-373.
1 participant
Summary by CodeRabbit
New Features
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.