fix (desktop): fixing escape sequences handling so its not pushed to input #388
Conversation
WalkthroughThe PR introduces FastEscapeFilter (a state machine–based escape sequence filter) and ScrollbackBuffer (a chunked string accumulator) to replace regex-based filtering and optimize scrollback storage. Both are integrated into session and manager flows, with comprehensive test coverage added for new and modified components. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 Deployment🔗 Preview Links
Preview updates automatically with new commits |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/terminal/session.ts (1)
157-188: LGTM: Well-designed fast path optimization with correct filtering logic.The data handler implementation is solid:
- Fast path for plain text (no ESC) provides excellent performance optimization
- Slow path correctly handles escape sequences, preserving clear sequences for visual rendering while removing them from history
- Filter reset on clear sequence is appropriate
The logic is complex but appears correct. Consider extracting the slow path (lines 167-188) into a separate helper function to improve maintainability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/desktop/src/main/lib/fast-escape-filter.test.ts(1 hunks)apps/desktop/src/main/lib/fast-escape-filter.ts(1 hunks)apps/desktop/src/main/lib/scrollback-buffer.ts(1 hunks)apps/desktop/src/main/lib/terminal-escape-filter.test.ts(2 hunks)apps/desktop/src/main/lib/terminal-escape-filter.ts(3 hunks)apps/desktop/src/main/lib/terminal/manager.test.ts(3 hunks)apps/desktop/src/main/lib/terminal/manager.ts(4 hunks)apps/desktop/src/main/lib/terminal/session.test.ts(7 hunks)apps/desktop/src/main/lib/terminal/session.ts(6 hunks)apps/desktop/src/main/lib/terminal/types.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
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-escape-filter.tsapps/desktop/src/main/lib/terminal/manager.test.tsapps/desktop/src/main/lib/scrollback-buffer.tsapps/desktop/src/main/lib/fast-escape-filter.test.tsapps/desktop/src/main/lib/terminal-escape-filter.test.tsapps/desktop/src/main/lib/fast-escape-filter.tsapps/desktop/src/main/lib/terminal/session.test.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/main/lib/terminal/manager.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-escape-filter.tsapps/desktop/src/main/lib/terminal/manager.test.tsapps/desktop/src/main/lib/scrollback-buffer.tsapps/desktop/src/main/lib/fast-escape-filter.test.tsapps/desktop/src/main/lib/terminal-escape-filter.test.tsapps/desktop/src/main/lib/fast-escape-filter.tsapps/desktop/src/main/lib/terminal/session.test.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/main/lib/terminal/manager.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting, running at root level for speed
Files:
apps/desktop/src/main/lib/terminal-escape-filter.tsapps/desktop/src/main/lib/terminal/manager.test.tsapps/desktop/src/main/lib/scrollback-buffer.tsapps/desktop/src/main/lib/fast-escape-filter.test.tsapps/desktop/src/main/lib/terminal-escape-filter.test.tsapps/desktop/src/main/lib/fast-escape-filter.tsapps/desktop/src/main/lib/terminal/session.test.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/main/lib/terminal/manager.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
apps/desktop/src/main/lib/terminal-escape-filter.tsapps/desktop/src/main/lib/terminal/manager.test.tsapps/desktop/src/main/lib/scrollback-buffer.tsapps/desktop/src/main/lib/fast-escape-filter.test.tsapps/desktop/src/main/lib/terminal-escape-filter.test.tsapps/desktop/src/main/lib/fast-escape-filter.tsapps/desktop/src/main/lib/terminal/session.test.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/main/lib/terminal/session.tsapps/desktop/src/main/lib/terminal/manager.ts
apps/desktop/src/main/lib/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Implement IPC handlers accepting object parameters (not positional parameters) in apps/desktop/src/main/lib/*.ts files
Files:
apps/desktop/src/main/lib/terminal-escape-filter.tsapps/desktop/src/main/lib/scrollback-buffer.tsapps/desktop/src/main/lib/fast-escape-filter.test.tsapps/desktop/src/main/lib/terminal-escape-filter.test.tsapps/desktop/src/main/lib/fast-escape-filter.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.tsapps/desktop/src/main/lib/fast-escape-filter.test.tsapps/desktop/src/main/lib/terminal-escape-filter.test.tsapps/desktop/src/main/lib/terminal/session.test.ts
🧠 Learnings (5)
📚 Learning: 2025-12-12T05:45:09.686Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T05:45:09.686Z
Learning: Applies to apps/desktop/src/main/lib/*.ts : Implement IPC handlers accepting object parameters (not positional parameters) in apps/desktop/src/main/lib/*.ts files
Applied to files:
apps/desktop/src/main/lib/terminal/manager.test.tsapps/desktop/src/main/lib/terminal/types.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.tsapps/desktop/src/main/lib/terminal-escape-filter.test.tsapps/desktop/src/main/lib/terminal/session.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 repeatable
Applied to files:
apps/desktop/src/main/lib/terminal/manager.test.tsapps/desktop/src/main/lib/terminal-escape-filter.test.tsapps/desktop/src/main/lib/terminal/session.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 fast
Applied to files:
apps/desktop/src/main/lib/fast-escape-filter.test.tsapps/desktop/src/main/lib/terminal/session.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.tsapps/desktop/src/main/lib/terminal/session.test.ts
🧬 Code graph analysis (4)
apps/desktop/src/main/lib/terminal-escape-filter.test.ts (1)
apps/desktop/src/main/lib/terminal-escape-filter.ts (3)
filterTerminalQueryResponses(267-269)filter(122-154)TerminalEscapeFilter(115-257)
apps/desktop/src/main/lib/terminal/session.test.ts (3)
apps/desktop/src/main/lib/scrollback-buffer.ts (1)
ScrollbackBuffer(11-69)apps/desktop/src/main/lib/terminal/session.ts (1)
recoverScrollback(21-45)apps/desktop/src/main/lib/terminal/types.ts (1)
TerminalSession(7-25)
apps/desktop/src/main/lib/terminal/types.ts (3)
apps/desktop/src/main/lib/scrollback-buffer.ts (1)
ScrollbackBuffer(11-69)apps/desktop/src/main/lib/terminal-history.ts (1)
HistoryWriter(32-107)apps/desktop/src/main/lib/fast-escape-filter.ts (1)
FastEscapeFilter(70-333)
apps/desktop/src/main/lib/terminal/session.ts (3)
apps/desktop/src/main/lib/scrollback-buffer.ts (1)
ScrollbackBuffer(11-69)apps/desktop/src/main/lib/fast-escape-filter.ts (1)
FastEscapeFilter(70-333)apps/desktop/src/main/lib/terminal-escape-filter.ts (2)
containsClearScrollbackSequence(282-284)extractContentAfterClear(294-308)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Admin
- GitHub Check: Deploy API
- GitHub Check: Deploy Web
- GitHub Check: Build
🔇 Additional comments (25)
apps/desktop/src/main/lib/terminal-escape-filter.ts (2)
127-131: LGTM! Effective fast-path optimization.The early-exit when no ESC character is present eliminates unnecessary regex work for plain text, delivering a measurable performance boost for common terminal output.
158-206: LGTM! Conservative buffering reduces latency.The updated buffering logic correctly balances correctness with performance by:
- Not buffering ESC alone or ESC[ alone (too common in normal output)
- Buffering only sequences that strongly indicate query responses (ESC[?, ESC[>, ESC[digit, OSC 1x, DCS patterns)
- Including length checks to prevent array out-of-bounds access
The digit buffering at lines 185-187 is intentional: color codes like ESC[32m will complete quickly and pass through since they don't match filter patterns.
apps/desktop/src/main/lib/fast-escape-filter.ts (4)
20-64: LGTM! Well-structured state machine design.The use of character codes for fast comparison and a comprehensive state enum covering all target escape sequence patterns (CSI, OSC, DCS variants) is appropriate for high-performance filtering.
77-90: LGTM! Efficient single-pass implementation.The filter method correctly implements O(n) processing with pre-allocated output, iterating through each character and delegating to the state machine.
92-299: LGTM! Comprehensive state machine logic.The
processCharmethod correctly handles all target escape sequence patterns:
- CPR: ESC[row;colR or ESC[rowR
- DA1/DA2/DA: ESC[?...c, ESC[>...c, ESC[digits...c
- Mode reports: ESC[...$y
- OSC color responses: ESC]1x;rgb:... with BEL or ST terminators
- DCS sequences: DA3 (ESC P ! |) and XTVERSION (ESC P > |)
- Unknown CSI: ESC[O
State transitions properly emit pending data when sequences don't match patterns, ensuring normal terminal output is preserved.
314-332: LGTM! Proper lifecycle management.The
flush()andreset()methods correctly handle session lifecycle by emitting buffered incomplete sequences and clearing state, respectively.apps/desktop/src/main/lib/terminal/manager.test.ts (1)
518-542: LGTM! Test expectations align with filtering behavior.The renamed test now correctly verifies that CPR (ESC[2;1R), DA1 (ESC[?1;0c), and OSC color responses are filtered from the data stream, with only plain text ("helloworld\n") passed to the handler. This aligns with the PR's objective to prevent escape sequences from appearing as garbage in the input.
apps/desktop/src/main/lib/terminal/session.test.ts (1)
4-141: LGTM! Tests correctly updated for ScrollbackBuffer.All test cases have been appropriately updated to:
- Use
ScrollbackBuffer.fromString()for test data initialization- Call
.toString()on scrollback results for string assertions- Update mock session structures to hold
ScrollbackBufferinstancesThe changes maintain test coverage while aligning with the new type signature.
apps/desktop/src/main/lib/terminal/types.ts (3)
3-4: LGTM! Imports updated for new implementations.The imports correctly reflect the migration to
FastEscapeFilter(state machine-based filtering) andScrollbackBuffer(chunked string accumulator).
15-20: LGTM! Type changes deliver performance improvements.Updating
scrollbackfromstringtoScrollbackBuffereliminates O(n²) string concatenation overhead, providing O(1) amortized appends. Similarly,FastEscapeFilterreplaces regex-based filtering with a single-pass state machine for better performance.
60-60: LGTM! Parameter type aligned with TerminalSession.The
existingScrollbackparameter type change toScrollbackBuffer | nullcorrectly reflects the refactored session structure.apps/desktop/src/main/lib/terminal-escape-filter.test.ts (2)
367-510: LGTM! Comprehensive TUI scenario coverage.The new test suite thoroughly validates filtering behavior for real-world TUI applications (Claude Code, Codex):
- CPR burst filtering during redraws
- Interleaved query responses with UI output
- Tab switch scenarios with accumulated responses
- Chunked data handling across state transitions
- Conservative buffering rules (ESC alone and ESC[ alone not buffered for performance, ESC[digit buffered for correctness)
These tests provide strong evidence that the filtering logic correctly handles the reported bug scenario.
567-580: LGTM! Test descriptions clarify performance rationale.The updated test descriptions at lines 567 and 575 explicitly mention performance concerns, making it clear why ESC alone and ESC[ alone should pass through immediately rather than being buffered.
apps/desktop/src/main/lib/fast-escape-filter.test.ts (1)
1-182: LGTM! Comprehensive test coverage for FastEscapeFilter.The test suite thoroughly validates the state machine implementation:
- All target patterns filtered: CPR, DA1/DA2/DA, Mode Reports, OSC color responses, DCS sequences, Unknown CSI
- Normal sequences preserved: colors, cursor movements, clear scrollback, reset
- Chunked sequences handled correctly across chunk boundaries
- Flush behavior verified for incomplete sequences
- Plain text passthrough validated including edge cases
The tests are readable, fast, and independent, following project guidelines.
apps/desktop/src/main/lib/scrollback-buffer.ts (1)
1-69: LGTM! Efficient chunked buffer design.The
ScrollbackBufferclass effectively addresses the O(n²) string concatenation problem:
- Array-based chunk storage provides O(1) amortized appends
- Lazy
toString()with caching avoids redundant joins- Memory compaction at lines 34-36 reduces overhead after the first
toString()callfromString()factory method enables efficient initializationlengthgetter provides O(1) length trackingThis is a significant performance improvement over repeated string concatenation, especially for large scrollback buffers.
apps/desktop/src/main/lib/terminal/manager.ts (5)
37-42: LGTM: ScrollbackBuffer integration for existing sessions.The conversion to string via
toString()correctly maintains the API contract while leveraging the new ScrollbackBuffer abstraction.
44-48: LGTM: Correct use of nullish coalescing operator.Using
??instead of||properly preserves empty-string scrollback values, which is important for accurate recovery logic.
78-83: LGTM: Consistent scrollback serialization.The
toString()conversion maintains consistency with the existing session path (line 39) and properly serializes the ScrollbackBuffer for the API response.
108-114: LGTM: Correct scrollback preservation in crash recovery.Passing
session.scrollbackdirectly is appropriate here since the session definitely exists in this crash recovery path. This preserves the scrollback history when falling back to the fallback shell.
235-249: LGTM: Proper use of ScrollbackBuffer API.Using
scrollback.clear()instead of assignment correctly leverages the ScrollbackBuffer abstraction and maintains proper encapsulation.apps/desktop/src/main/lib/terminal/session.ts (5)
15-16: LGTM: ESC constant improves readability and performance.Defining the ESC character as a constant enhances code clarity and enables efficient fast-path checks in the data handler.
21-45: LGTM: ScrollbackBuffer integration in recovery path.The updated
recoverScrollbackfunction correctly:
- Integrates ScrollbackBuffer throughout the recovery flow
- Sanitizes recovered history using FastEscapeFilter to remove protocol responses
- Properly flushes the filter to include any pending data
117-120: LGTM: Efficient scrollback persistence.Converting the scrollback to string only when non-empty and passing it to the history writer is both correct and efficient.
138-138: LGTM: FastEscapeFilter initialization.Replacing the previous filter with the new state machine-based FastEscapeFilter aligns with the PR's objective to improve escape sequence handling.
241-249: LGTM: Consistent ScrollbackBuffer usage in flush.Using
scrollback.append()for the remaining filtered data maintains consistency with the ScrollbackBuffer abstraction throughout the session lifecycle.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
Performance
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.