fix(desktop): filter escape sequences when saving terminal history#280
fix(desktop): filter escape sequences when saving terminal history#280
Conversation
Previously, escape sequences were only filtered when restoring terminal history, not when initially saving recovered scrollback. This could leave unfiltered data on disk. Now HistoryWriter.init() filters initial scrollback before persisting, ensuring clean storage. Filtering on restore is kept for legacy data. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds persistent serialized scrollback support from renderer to main via a new saveScrollback flow (trpc endpoint → TerminalManager → HistoryWriter), integrates xterm SerializeAddon to produce that data, and tightens TerminalEscapeFilter buffering/flush logic and tests to avoid leaking incomplete escape/query sequences across chunk boundaries. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant TRPC
participant TerminalManager
participant HistoryWriter
participant Filesystem
Renderer->>Renderer: SerializeAddon.serialize(excludeAlt=true)
Renderer->>TRPC: terminal.saveScrollback(tabId, serialized) [mutation]
TRPC->>TerminalManager: saveScrollback({tabId, serialized})
TerminalManager->>TerminalManager: set session.serializedScrollback
TerminalManager->>HistoryWriter: saveSnapshot(serialized)
HistoryWriter->>Filesystem: replace scrollback file (close, write, reopen append)
Filesystem-->>HistoryWriter: ack / errors
HistoryWriter-->>TerminalManager: result
TerminalManager-->>TRPC: completion
TRPC-->>Renderer: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
…back When ESC appears at the end of a data chunk, it must be buffered to check if the next chunk forms a query response. Previously, lone ESC passed through, causing sequences like "[2R" to leak into scrollback. Also discard incomplete query responses on flush to prevent garbage characters (e.g., "2R1R12;2$y") from appearing after TUI apps like lazygit. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Buffer ESC[ alone at chunk end to prevent ";1R" leaking through - Match CPR with omitted row/column (ESC[;1R, ESC[R) - Buffer ESC[; for CPR with omitted parameters Fixes garbage like ";1R2;1R" appearing after TUI apps. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/lib/terminal-manager.ts (1)
118-124: Potential duplicate filtering of recovered scrollback.The
recoveredScrollbackis filtered here before being passed tohistoryWriter.init(). However,HistoryWriter.init()(lines 61-64 in terminal-history.ts) also applies the same filtering to itsinitialScrollbackparameter.This results in the recovered scrollback being filtered twice:
- Here in
terminal-manager.ts(lines 121-123)- In
HistoryWriter.init()(lines 62-64)While double-filtering is safe (idempotent), it's redundant. Consider removing filtering from one location.
- // Filter escape sequences from recovered scrollback - // This handles legacy unfiltered data and ensures clean display - if (recoveredScrollback) { - const recoveryFilter = new TerminalEscapeFilter(); - recoveredScrollback = - recoveryFilter.filter(recoveredScrollback) + recoveryFilter.flush(); - } + // Note: HistoryWriter.init() filters escape sequences from initialScrollback + // to handle legacy unfiltered data and ensure clean storageAlternatively, keep filtering here for display purposes and remove it from
HistoryWriter.init()if storage doesn't need its own sanitization pass.
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/terminal-escape-filter.test.ts (1)
420-430: Multiple assertions in single test.This test has multiple
expectcalls testing different aspects of the filter's behavior. Per coding guidelines, tests should have one assert per test. However, since this tests stateful behavior where the second assertion depends on the first chunk's processing, the multi-assertion pattern is reasonable here.Consider splitting into two tests if strict adherence is required:
- Test that ESC alone is buffered (returns "text")
- Test that subsequent chunk completing a query response is filtered
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/main/lib/terminal-escape-filter.test.ts(2 hunks)apps/desktop/src/main/lib/terminal-escape-filter.ts(4 hunks)apps/desktop/src/main/lib/terminal-history.ts(2 hunks)apps/desktop/src/main/lib/terminal-manager.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/main/lib/terminal-manager.tsapps/desktop/src/main/lib/terminal-escape-filter.tsapps/desktop/src/main/lib/terminal-history.tsapps/desktop/src/main/lib/terminal-escape-filter.test.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/main/lib/terminal-manager.tsapps/desktop/src/main/lib/terminal-escape-filter.tsapps/desktop/src/main/lib/terminal-history.tsapps/desktop/src/main/lib/terminal-escape-filter.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Avoidanytype in TypeScript unless absolutely necessary
Files:
apps/desktop/src/main/lib/terminal-manager.tsapps/desktop/src/main/lib/terminal-escape-filter.tsapps/desktop/src/main/lib/terminal-history.tsapps/desktop/src/main/lib/terminal-escape-filter.test.ts
apps/desktop/src/{main,renderer,shared}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC system for communication between Electron main and renderer processes
Files:
apps/desktop/src/main/lib/terminal-manager.tsapps/desktop/src/main/lib/terminal-escape-filter.tsapps/desktop/src/main/lib/terminal-history.tsapps/desktop/src/main/lib/terminal-escape-filter.test.ts
apps/desktop/**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.test.{ts,tsx,js,jsx}: Tests should have one assert per test
Tests should be readable
Tests should be fast
Tests should be independent
Tests should be repeatable
Files:
apps/desktop/src/main/lib/terminal-escape-filter.test.ts
🧠 Learnings (2)
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be repeatable
Applied to files:
apps/desktop/src/main/lib/terminal-escape-filter.test.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be independent
Applied to files:
apps/desktop/src/main/lib/terminal-escape-filter.test.ts
🧬 Code graph analysis (2)
apps/desktop/src/main/lib/terminal-history.ts (1)
apps/desktop/src/main/lib/terminal-escape-filter.ts (2)
filter(117-143)TerminalEscapeFilter(110-259)
apps/desktop/src/main/lib/terminal-escape-filter.test.ts (1)
apps/desktop/src/main/lib/terminal-escape-filter.ts (2)
filter(117-143)TerminalEscapeFilter(110-259)
⏰ 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 (6)
apps/desktop/src/main/lib/terminal-escape-filter.test.ts (2)
486-509: LGTM on flush behavior tests.The updated flush behavior tests correctly verify that incomplete query responses are discarded rather than passed through. The test cases cover:
- Incomplete DA1 sequences being discarded
- Lone ESC being discarded on flush
This aligns with the PR objective to prevent garbage characters when restoring history.
512-545: Good regression test coverage for the chunk boundary bug.These TUI scenario tests directly address the bug where ESC at chunk boundaries caused orphaned sequence parts like "[2R" to leak through. The explicit comment "This was the bug" at line 534 documents the regression scenario well.
apps/desktop/src/main/lib/terminal-escape-filter.ts (4)
21-27: LGTM on CPR pattern update.The updated regex
\d*(?:;\d*)?Rcorrectly handles CPR variants:
ESC[24;1R- standard CPRESC[2R- row onlyESC[;1R- omitted rowESC[R- both omitted (edge case)The
\d*allows zero or more digits, and(?:;\d*)?makes the semicolon and column optional.
150-169: LGTM on enhanced buffering logic.The changes correctly handle chunk boundary cases:
- Line 152: Buffering lone ESC prevents orphaned sequence parts like "[2R" from leaking
- Line 164: Buffering
ESC[alone allows proper handling of sequences starting with;- Lines 168-169: Buffering
ESC[;handles CPR with omitted row parameterThis directly addresses the bug described in the PR.
240-251: LGTM on flush discard behavior.The updated
flush()correctly discards incomplete query responses rather than passing them through. This is the right approach because:
- Incomplete sequences at session end are likely query responses that didn't complete
- Passing them through would display garbage characters on restore
- The fallback
replace(COMBINED_PATTERN, "")handles any complete sequences in non-query-response buffers
202-204: LGTM on isIncomplete handling.The comment clarifies that lone ESC is treated as incomplete, which is consistent with
looksLikeQueryResponsenow buffering lone ESC. Thestr.length < 2check correctly identifies this case.
Replace raw PTY byte storage with xterm.js serialize addon output for terminal scrollback persistence. This properly solves the escape sequence garbage problem because: 1. The serialize addon captures the *rendered* terminal buffer state 2. Query responses (DA1, CPR, OSC colors) never appear in the visual buffer 3. TUI apps in alternate screen mode are excluded via excludeAltBuffer option Architecture change: - Renderer serializes terminal state on detach via SerializeAddon.serialize() - Calls new saveScrollback tRPC procedure to persist to main process - Main process no longer filters/accumulates data for disk - only for in-memory - HistoryWriter.saveSnapshot() replaces file with clean serialized data Legacy support: - TerminalEscapeFilter kept for migrating old unfiltered scrollback files - Old files are filtered on first load, then replaced with clean data on save 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When reattaching to a live terminal session, return the last serialized snapshot instead of raw accumulated scrollback. This prevents query response garbage from appearing on reattach. Added serializedScrollback field to TerminalSession to track the clean serialized state separately from the raw accumulated data. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
For live terminal reattach, combine the last serialized snapshot with filtered new data that arrived since serialization. This ensures: 1. No garbage from the serialized snapshot (already clean) 2. New data since serialization is filtered through TerminalEscapeFilter 3. No gap in terminal history between detach and reattach 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
287-317: Serialization flow is sound, but consider removing debug logs for production.The cleanup serialization logic is well-structured:
- Null-checks prevent errors if addon isn't loaded
excludeAltBuffer: truecorrectly avoids saving TUI app state- Fire-and-forget with error logging is reasonable for unmount context
However, the
console.logstatements at lines 295-300 and 313-316 appear to be debug logging. Consider removing or gating these behind a debug flag before merging.- if (serializeAddon && !isExitedRef.current) { - try { - const serialized = serializeAddon.serialize({ - excludeAltBuffer: true, // Don't save TUI app alternate screen - }); - console.log( - "[Terminal] Serialized scrollback:", - serialized?.length, - "chars, first 200:", - serialized?.slice(0, 200), - ); - if (serialized) { + if (serializeAddon && !isExitedRef.current) { + try { + const serialized = serializeAddon.serialize({ + excludeAltBuffer: true, // Don't save TUI app alternate screen + }); + if (serialized) { // Fire and forget - don't block unmount on save trpcClient.terminal.saveScrollback .mutate({ tabId: paneId, serialized }) .catch((err) => { console.error("[Terminal] Failed to save scrollback:", err); }); } } catch (err) { console.error("[Terminal] Failed to serialize terminal:", err); } - } else { - console.log( - "[Terminal] Skipping serialize:", - !serializeAddon ? "no addon" : "exited", - ); }apps/desktop/src/main/lib/terminal-manager.ts (1)
328-335: Consider removing debug logging for production.Similar to the renderer-side code, these
console.logstatements appear to be debug output. Consider removing or gating behind a debug flag.- console.log( - "[TerminalManager] saveScrollback:", - tabId, - "length:", - serialized.length, - "first 200:", - serialized.slice(0, 200), - ); session.serializedScrollback = serialized;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts(1 hunks)apps/desktop/src/main/lib/terminal-history.ts(2 hunks)apps/desktop/src/main/lib/terminal-manager.test.ts(3 hunks)apps/desktop/src/main/lib/terminal-manager.ts(5 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(4 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/main/lib/terminal-manager.test.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/lib/terminal-history.tsapps/desktop/src/main/lib/terminal-manager.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/main/lib/terminal-manager.test.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/lib/terminal-history.tsapps/desktop/src/main/lib/terminal-manager.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Avoidanytype in TypeScript unless absolutely necessary
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/main/lib/terminal-manager.test.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/lib/terminal-history.tsapps/desktop/src/main/lib/terminal-manager.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/desktop/src/{renderer,shared,preload}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process code or shared code in the desktop app
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/desktop/src/{main,renderer,shared}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use type-safe IPC system for communication between Electron main and renderer processes
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.tsapps/desktop/src/main/lib/terminal-manager.test.tsapps/desktop/src/main/lib/terminal-history.tsapps/desktop/src/main/lib/terminal-manager.ts
apps/desktop/**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.test.{ts,tsx,js,jsx}: Tests should have one assert per test
Tests should be readable
Tests should be fast
Tests should be independent
Tests should be repeatable
Files:
apps/desktop/src/main/lib/terminal-manager.test.ts
**/{components,pages}/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
**/{components,pages}/**/*.tsx: Use one folder per component with structureComponentName/ComponentName.tsxplusindex.tsfor barrel export
Create one component per file - do not use multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
🧠 Learnings (3)
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be repeatable
Applied to files:
apps/desktop/src/main/lib/terminal-manager.test.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be readable
Applied to files:
apps/desktop/src/main/lib/terminal-manager.test.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be independent
Applied to files:
apps/desktop/src/main/lib/terminal-manager.test.ts
🧬 Code graph analysis (3)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
apps/desktop/src/main/lib/terminal-manager.ts (1)
terminalManager(588-588)
apps/desktop/src/main/lib/terminal-manager.ts (1)
apps/desktop/src/main/lib/terminal-escape-filter.ts (2)
filter(117-143)TerminalEscapeFilter(110-259)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
createTerminalInstance(53-135)apps/desktop/src/renderer/lib/trpc-client.ts (1)
trpcClient(9-11)
⏰ 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 (6)
apps/desktop/src/main/lib/terminal-history.ts (1)
89-112: LGTM! Well-structured snapshot persistence method.The
saveSnapshotmethod correctly handles the stream lifecycle: closing the existing stream before replacing the file, then reopening in append mode. Error handling is appropriately defensive.One minor observation: if
fs.writeFileat line 104 throws after the stream is closed (line 100), the method will propagate the error without reopening the stream. Subsequentwrite()calls will silently fail sincestreamisnull. This seems acceptable since a failed snapshot save is likely a critical error, but consider whether you want to attempt stream recovery.apps/desktop/src/main/lib/terminal-manager.test.ts (2)
375-379: LGTM! Tests correctly simulate the new renderer-driven serialization flow.The test updates accurately reflect the new architecture where the renderer calls
saveScrollbackbefore detach/exit. This ensures the persistence flow is exercised end-to-end.
762-766: Multi-session persistence tests properly updated.The multi-session test now correctly simulates cumulative scrollback persistence across sessions, matching the expected renderer behavior of serializing the complete terminal state.
Also applies to: 799-803
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (1)
85-95: LGTM! SerializeAddon integrated following existing addon patterns.The addon is correctly instantiated before
xterm.open()and loaded after, consistent with other addons. Exposing it in the return object enables the serialization flow inTerminal.tsx.apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
149-162: LGTM! Clean tRPC endpoint following existing patterns.The
saveScrollbackmutation correctly:
- Uses zod for input validation
- Awaits the async
terminalManager.saveScrollbackcall- Includes a descriptive JSDoc comment
As per coding guidelines, this properly uses tRPC for Electron IPC communication.
apps/desktop/src/main/lib/terminal-manager.ts (1)
308-342: LGTM! saveScrollback method correctly integrates with HistoryWriter.The method properly:
- Guards against missing sessions
- Updates the in-memory serialized snapshot
- Persists to disk via
historyWriter.saveSnapshot
| // For live sessions, use the last serialized snapshot as the base. | ||
| // If raw scrollback has grown since then, filter and append the new data. | ||
| let scrollback = existing.serializedScrollback || ""; | ||
| const rawLen = existing.scrollback.length; | ||
| const serializedLen = existing.serializedScrollback.length; | ||
| console.log( | ||
| "[TerminalManager] reattach:", | ||
| tabId, | ||
| "serializedLen:", | ||
| serializedLen, | ||
| "rawLen:", | ||
| rawLen, | ||
| "delta:", | ||
| rawLen - serializedLen, | ||
| ); | ||
| if (rawLen > serializedLen) { | ||
| // There's new data since last serialization - filter it | ||
| const newData = existing.scrollback.slice(serializedLen); | ||
| console.log( | ||
| "[TerminalManager] filtering delta, first 200:", | ||
| newData.slice(0, 200), | ||
| ); | ||
| const filter = new TerminalEscapeFilter(); | ||
| scrollback += filter.filter(newData) + filter.flush(); | ||
| } | ||
| console.log( | ||
| "[TerminalManager] returning scrollback, first 200:", | ||
| scrollback.slice(0, 200), | ||
| ); | ||
| return { | ||
| isNew: false, | ||
| scrollback: existing.scrollback, | ||
| scrollback, | ||
| wasRecovered: existing.wasRecovered, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Potential logic issue with delta calculation based on string length.
The delta filtering logic at lines 91-99 assumes that serializedScrollback.length can be used as an offset into scrollback. However, these two strings may have different content:
scrollbackcontains raw PTY output (may include escape sequences)serializedScrollbackcontains filtered/serialized content (escape sequences removed)
If the serialized length differs from the raw content it represents, slicing at serializedLen could produce incorrect results (either missing data or including already-processed data).
Consider tracking the raw scrollback length at the time of serialization instead:
interface TerminalSession {
// ...
scrollback: string;
serializedScrollback: string;
+ /** Raw scrollback length when serializedScrollback was last saved */
+ scrollbackLengthAtLastSerialize: number;
// ...
}Then in saveScrollback:
session.serializedScrollback = serialized;
+session.scrollbackLengthAtLastSerialize = session.scrollback.length;And in reattach:
-if (rawLen > serializedLen) {
- const newData = existing.scrollback.slice(serializedLen);
+if (rawLen > existing.scrollbackLengthAtLastSerialize) {
+ const newData = existing.scrollback.slice(existing.scrollbackLengthAtLastSerialize);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/terminal-manager.ts around lines 76 to 110, the
code uses serializedScrollback.length as an index into the raw scrollback which
is incorrect because serialization/filtering can change length; update the
serialization flow to store the raw scrollback length at the time of
serialization (e.g. save a rawScrollbackLength alongside serializedScrollback in
saveScrollback), then in the reattach path use that stored rawScrollbackLength
as the slice offset when computing the delta (fall back to 0 if the stored raw
length is missing or larger than current raw length), and keep existing
filtering/flush logic unchanged.
1 participant
Summary
HistoryWriter.init()before persisting initial scrollback to diskTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.