feat: clear scrollback history on clear commands#306
Conversation
- Add clearScrollback method to TerminalManager that clears in-memory scrollback and reinitializes history file - Add clearScrollback tRPC endpoint for renderer to call - Detect clear scrollback escape sequences (ESC[3J, ESC c) in PTY output to automatically clear stored buffers when shell clear command is used - Update Terminal component to call clearScrollback on Cmd+K - Use explicit UTF-8 encoding for terminal history read/write 🤖 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.
|
|
Warning Rate limit exceeded@Kitenite has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 0 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)
WalkthroughAdds a terminal scrollback-clear feature end-to-end: detects clear-scrollback escape sequences, enforces UTF-8 for history IO, exposes a TRPC mutation, extends TerminalManager with clear/reinitialize logic, and wires the renderer UI to invoke the mutation. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Terminal UI
participant TRPC as TRPC Router
participant Manager as TerminalManager
participant Filter as EscapeFilter
participant History as HistoryWriter
User->>UI: Press clear shortcut (e.g., Cmd+K)
UI->>UI: Clear xterm buffer (local)
UI->>TRPC: clearScrollback({ tabId })
TRPC->>Manager: clearScrollback({ tabId })
rect rgb(200,220,255)
Note over Manager: Clear in-memory state & reset filter
Manager->>Manager: session.scrollback = []
Manager->>Filter: reset/reinitialize filter state
end
rect rgb(200,220,255)
Note over Manager,History: Reinitialize persistent history
Manager->>History: close writer (async)
Manager->>History: recreate & initialize writer (truncate)
end
TRPC-->>UI: Success
Note over Manager,Filter: On incoming PTY data containing ESC[3J or ESC c
Manager->>Filter: containsClearScrollbackSequence(data)?
alt detected
Manager->>Manager: clear session.scrollback
Manager->>History: reinitializeHistory(session) (async)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Kitenite
changed the title
![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
🧹 Nitpick comments (2)
apps/desktop/src/main/lib/terminal-history.ts (1)
116-123: Minor inconsistency in encoding string format.Line 118 uses
"utf8"while line 123 uses"utf-8". Both are functionally equivalent in Node.js, but for consistency with the rest of this file (which uses"utf8"), consider using the same format.const metaPath = getMetadataPath(this.workspaceId, this.tabId); - const metaContent = await fs.readFile(metaPath, "utf-8"); + const metaContent = await fs.readFile(metaPath, "utf8");apps/desktop/src/main/lib/terminal-manager.ts (1)
229-236: Consider adding minimal error logging for history reinitialization failures.The silent
.catch(() => {})suppresses all errors fromreinitializeHistory. While history errors are non-critical, a console warning could help diagnose issues if scrollback clearing stops working.// Reinitialize history file asynchronously (truncate to empty) - this.reinitializeHistory(session).catch(() => {}); + this.reinitializeHistory(session).catch((err) => { + console.warn(`Failed to reinitialize history for ${session.tabId}:`, err); + });
📜 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(1 hunks)apps/desktop/src/main/lib/terminal-escape-filter.ts(2 hunks)apps/desktop/src/main/lib/terminal-history.ts(3 hunks)apps/desktop/src/main/lib/terminal-manager.ts(3 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(2 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/main/lib/terminal-escape-filter.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/main/lib/terminal-history.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/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/main/lib/terminal-history.tsapps/desktop/src/main/lib/terminal-manager.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Maintain type safety and avoid using
anyunless absolutely necessary in TypeScript code
Files:
apps/desktop/src/main/lib/terminal-escape-filter.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/main/lib/terminal-history.tsapps/desktop/src/main/lib/terminal-manager.ts
apps/desktop/src/main/lib/terminal-*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use node-pty for terminal session management in the desktop app
Files:
apps/desktop/src/main/lib/terminal-escape-filter.tsapps/desktop/src/main/lib/terminal-history.tsapps/desktop/src/main/lib/terminal-manager.ts
apps/desktop/src/lib/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules like node:fs, node:path, node:os in src/lib/electron-router-dom.ts or similar shared code
Files:
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
**/components/**/[A-Z]*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use one folder per component with structure ComponentName/ComponentName.tsx + index.ts barrel export
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process or shared code - only in src/main/
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
🧠 Learnings (1)
📚 Learning: 2025-12-08T23:32:32.232Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T23:32:32.232Z
Learning: Applies to apps/desktop/src/main/lib/terminal-*.ts : Use node-pty for terminal session management in the desktop app
Applied to files:
apps/desktop/src/main/lib/terminal-escape-filter.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/main/lib/terminal-history.tsapps/desktop/src/main/lib/terminal-manager.ts
🧬 Code graph analysis (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
apps/website/src/trpc/server.tsx (1)
trpc(26-30)
apps/desktop/src/main/lib/terminal-manager.ts (2)
apps/desktop/src/main/lib/terminal-escape-filter.ts (2)
containsClearScrollbackSequence(272-274)TerminalEscapeFilter(115-247)apps/desktop/src/main/lib/terminal-history.ts (1)
HistoryWriter(30-105)
🪛 ast-grep (0.40.0)
apps/desktop/src/main/lib/terminal-escape-filter.ts
[warning] 17-17: 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|${ESC}c, "g")
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 (7)
apps/desktop/src/main/lib/terminal-escape-filter.ts (1)
17-18: Static analysis false positive - no ReDoS risk.The static analysis warning about ReDoS is a false positive.
ESCis a compile-time constant ("\x1b"), not user input, and the pattern has no nested quantifiers or catastrophic backtracking potential.apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
148-160: LGTM!The new
clearScrollbackmutation follows the established patterns in this router, with proper input validation and async handling. The documentation clearly describes its purpose.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (2)
71-82: LGTM!The integration follows the established ref-based mutation pattern used throughout this component. The fire-and-forget approach for
clearScrollbackis consistent with other non-critical mutations likewrite.
306-311: LGTM!The clear flow correctly prioritizes immediate UI feedback by clearing xterm first, then asynchronously clearing the persisted scrollback. This provides a responsive UX while maintaining backend consistency.
apps/desktop/src/main/lib/terminal-manager.ts (3)
6-9: LGTM!The import correctly brings in both the
containsClearScrollbackSequencefunction for detection andTerminalEscapeFilterclass for filtering.
372-393: LGTM!The
clearScrollbackmethod correctly clears in-memory state, resets the escape filter, and awaits history reinitialization. The await here (vs fire-and-forget in the data handler) is appropriate since this is an explicit user action where completion confirmation matters.
395-407: LGTM!The helper correctly sequences the close-create-init operations and properly handles the case where
historyWritermay not exist.
Add tests for clearScrollback functionality: - Clear in-memory scrollback - Reinitialize history file on clear - Handle non-existent session gracefully - Clear scrollback on shell ED3 sequence Remove comments that restate what the code already expresses. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Using test() with a global regex causes stateful lastIndex behavior leading to sporadic false negatives on consecutive calls. 🤖 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: 0
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/terminal-manager.test.ts (1)
764-841: Optional: split multi‑assert tests to better follow “one assert per test” guidelineIn this new block, several tests contain multiple
expectcalls (e.g., checking bothscrollbackandwasRecovered, or result +console.warncall, or two scrollback content checks). If you want to adhere more strictly to the “one assert per test” guidance, consider splitting these into smaller tests per behavior (e.g., one test forscrollbackcontents, another forwasRecovered, etc.). This is optional, but would bring the new tests closer to the documented style without changing coverage. As per coding guidelines, tests should ideally have one assert per test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/main/lib/terminal-manager.test.ts(1 hunks)apps/desktop/src/main/lib/terminal-manager.ts(3 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts(0 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/main/lib/terminal-manager.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
🧰 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.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.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
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Maintain type safety and avoid using
anyunless absolutely necessary in TypeScript code
Files:
apps/desktop/src/main/lib/terminal-manager.test.ts
apps/desktop/src/main/lib/terminal-*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use node-pty for terminal session management in the desktop app
Files:
apps/desktop/src/main/lib/terminal-manager.test.ts
🧠 Learnings (5)
📚 Learning: 2025-12-08T23:32:32.232Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T23:32:32.232Z
Learning: Applies to apps/desktop/src/main/lib/terminal-*.ts : Use node-pty for terminal session management in the desktop app
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-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 have one assert per test
Applied to files:
apps/desktop/src/main/lib/terminal-manager.test.ts
⏰ 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 (1)
apps/desktop/src/main/lib/terminal-manager.test.ts (1)
737-842: Clear scrollback behavior is well-covered and tests look deterministicThe new
clearScrollbacksuite exercises all the key scenarios (in‑memory scrollback reset, history reinit across process exit/cleanup, non‑existent sessions, and ED3 clear‑sequence handling) using the real history implementation and the existingmockPtywiring. Tab/workspace IDs are isolated and there are no added timing hacks, so these tests should remain repeatable and independent. Based on learnings, this aligns well with the “readable / independent / repeatable” test guidelines.
1 participant
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.