Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded@Kitenite has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 24 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 (2)
WalkthroughIntroduces a new stateful TerminalEscapeFilter that removes terminal query response sequences from PTY output, integrates it into the terminal manager to filter stored scrollback/history while preserving raw output to consumers, and adds extensive tests covering buffering, reassembly, and edge cases. Changes
Sequence DiagramsequenceDiagram
participant PTY as PTY
participant Filter as TerminalEscapeFilter
participant Manager as TerminalManager
participant History as Scrollback/History
participant Consumer as Terminal Consumer
PTY->>Filter: send data chunk (may contain escape/query responses)
Filter->>Filter: buffer/reassemble split sequences
alt sequence identified as query response
Filter-->>Manager: omit sequence (filtered)
else normal content or non-query CSI
Filter-->>Manager: pass-through content
end
Manager->>History: store filtered content
Manager->>Consumer: emit original/raw data
Note over Filter,Manager: On terminal exit
Filter->>Filter: flush() buffered data
Filter-->>Manager: flushed filtered output
Manager->>History: persist flushed data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/terminal-escape-filter.test.ts (1)
198-203: Consider splitting loop into individual tests.This test iterates OSC 13-19 with an assertion per iteration, resulting in 7 assertions in one test. Per coding guidelines, tests should have one assertion per test.
Consider splitting into individual test cases or using test.each (if available in bun:test):
- it("should filter OSC 13-19 (highlight colors)", () => { - for (let i = 13; i <= 19; i++) { - const osc = `${ESC}]${i};rgb:aaaa/bbbb/cccc${BEL}`; - expect(filterTerminalQueryResponses(osc)).toBe(""); - } - }); + it("should filter OSC 13 (highlight color)", () => { + const osc = `${ESC}]13;rgb:aaaa/bbbb/cccc${BEL}`; + expect(filterTerminalQueryResponses(osc)).toBe(""); + }); + + it("should filter OSC 14-19 (highlight colors)", () => { + // Representative test for remaining OSC highlight colors + const osc = `${ESC}]17;rgb:aaaa/bbbb/cccc${BEL}`; + expect(filterTerminalQueryResponses(osc)).toBe(""); + });
📜 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(1 hunks)apps/desktop/src/main/lib/terminal-escape-filter.ts(1 hunks)apps/desktop/src/main/lib/terminal-manager.test.ts(1 hunks)apps/desktop/src/main/lib/terminal-manager.ts(3 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.test.tsapps/desktop/src/main/lib/terminal-manager.tsapps/desktop/src/main/lib/terminal-escape-filter.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.test.tsapps/desktop/src/main/lib/terminal-manager.tsapps/desktop/src/main/lib/terminal-escape-filter.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-manager.test.tsapps/desktop/src/main/lib/terminal-escape-filter.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid usinganytype - use explicit types instead for type safety
Use camelCase for variable and function names following existing codebase patterns
Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
Follow existing patterns and match the codebase style when writing new code
Files:
apps/desktop/src/main/lib/terminal-manager.test.tsapps/desktop/src/main/lib/terminal-manager.tsapps/desktop/src/main/lib/terminal-escape-filter.tsapps/desktop/src/main/lib/terminal-escape-filter.test.ts
apps/desktop/src/main/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Node.js modules (fs, path, os, net, etc.) can be used in main process code only
Files:
apps/desktop/src/main/lib/terminal-manager.test.tsapps/desktop/src/main/lib/terminal-manager.tsapps/desktop/src/main/lib/terminal-escape-filter.tsapps/desktop/src/main/lib/terminal-escape-filter.test.ts
🧠 Learnings (5)
📚 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 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 independent
Applied to files:
apps/desktop/src/main/lib/terminal-manager.test.ts
📚 Learning: 2025-11-28T01:03:47.963Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T01:03:47.963Z
Learning: Applies to **/*.{ts,tsx} : Keep diffs minimal with targeted edits only - avoid unnecessary changes when making modifications
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 fast
Applied to files:
apps/desktop/src/main/lib/terminal-manager.test.ts
🧬 Code graph analysis (2)
apps/desktop/src/main/lib/terminal-manager.ts (1)
apps/desktop/src/main/lib/terminal-escape-filter.ts (1)
TerminalEscapeFilter(108-240)
apps/desktop/src/main/lib/terminal-escape-filter.test.ts (1)
apps/desktop/src/main/lib/terminal-escape-filter.ts (3)
filterTerminalQueryResponses(250-252)filter(115-141)TerminalEscapeFilter(108-240)
⏰ 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/main/lib/terminal-manager.test.ts (1)
511-534: LGTM!The test correctly verifies that raw terminal data (including escape sequences) passes through unchanged to the event handler, while the updated comment accurately describes the data flow where filtering occurs before scrollback/history storage. Based on learnings, the test is readable, has one assertion, and is independent.
apps/desktop/src/main/lib/terminal-manager.ts (4)
6-6: LGTM!Import follows the existing pattern of relative imports used elsewhere in this file.
22-22: LGTM!The
escapeFilterfield is properly typed and required for every terminal session, ensuring consistent filtering behavior.
150-161: LGTM!The implementation correctly:
- Creates a stateful filter per session to handle chunked escape sequences
- Applies filtering before storing to scrollback/history to avoid garbage on replay
- Emits raw data to xterm so it can properly process terminal query responses
The clear comments explain the design rationale well.
163-174: LGTM!Properly flushes any buffered data from the escape filter on terminal exit, ensuring no data loss from incomplete sequences that were being reassembled. The ordering is correct: flush remaining data before closing history.
apps/desktop/src/main/lib/terminal-escape-filter.ts (5)
1-101: Well-documented patterns for terminal query responses.The pattern definitions are comprehensive and well-documented with JSDoc comments explaining each sequence type. The regex construction is correct.
108-141: LGTM!The
filter()method correctly:
- Combines buffered data with new input for reassembly
- Uses conservative heuristics (30 char limit, pattern matching) to avoid over-buffering
- Only buffers sequences that look like query responses to minimize latency for normal output
148-189: LGTM!The
looksLikeQueryResponse()method implements conservative buffering heuristics that:
- Buffer potential query responses (CPR, DA, DECRPM, OSC color, DCS)
- Don't buffer generic CSI sequences like color codes
- Minimize output latency by only buffering when necessary
194-222: LGTM!The
isIncomplete()method correctly identifies incomplete terminal sequences:
- CSI sequences need a final byte (A-Z, a-z, ~)
- OSC/DCS sequences need a terminator (BEL or ST)
224-255: LGTM!The utility methods are correctly implemented:
flush()applies filtering to remaining buffer before returningreset()provides clean state- Deprecated function maintained for backward compatibility with clear deprecation notice
- Patterns exported for testing access
apps/desktop/src/main/lib/terminal-escape-filter.test.ts (3)
1-10: LGTM!Clean imports and test helper constants that mirror the implementation for building test sequences.
366-522: Comprehensive stateful filter tests.The
TerminalEscapeFiltertests thoroughly cover:
- Chunked sequence reassembly (DA1, CPR, OSC, standard mode reports)
- Conservative buffering behavior (ESC alone, ESC [ alone not buffered)
- Color code pass-through (ESC[32m style sequences)
- Flush and reset semantics
Tests are readable, independent (new filter per test), and follow good patterns. Based on learnings, the tests are fast and repeatable.
324-364: LGTM!Solid edge case coverage including:
- Incomplete sequences (proper preservation)
- Long strings (performance)
- Unicode and binary content (encoding safety)
- Multiple ESC characters
1 participant
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.