Conversation
WalkthroughPreserve terminal bottom alignment by detecting whether the viewport is at the bottom before writes and scrolling to bottom after writes; defer scroll-to-bottom after resize/fitting; buffer incomplete escape-sequence prefixes at chunk boundaries and preserve them on flush, with updated tests for cross-chunk reassembly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(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/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/Terminal.tsx
**/*.{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/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/components/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
**/components/**/*.tsx: Create one folder per component with structure:ComponentName/ComponentName.tsx+index.tsfor barrel export
Co-locate tests next to the component file they test (e.g.,ComponentName.test.tsx)
Co-locate dependencies (utils, hooks, constants, config, stories) next to the file using them
Use nestedcomponents/subdirectory within a parent component only if a sub-component is used 2+ times within that parent; otherwise keep it in the parent'scomponents/folder
One component per file - avoid multi-component files
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 (fs, path, os, net, etc.) in renderer process code - browser environment only
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/desktop/src/renderer/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Call IPC methods using
window.ipcRenderer.invoke()with object parameters - TypeScript will infer the exact response type automatically
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
🔇 Additional comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (2)
173-174: LGTM!Scrolling to bottom after flushing pending events ensures the latest content is visible after initialization.
183-184: LGTM!Scrolling to bottom after applying scrollback ensures the user sees the most recent terminal output.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/terminal-escape-filter.ts (1)
120-142: Consider consolidating repetitive prefix checks.The logic is correct, but the four similar if-blocks could be simplified using an array of prefixes to reduce duplication and improve maintainability.
- // Pre-check: Buffer trailing prefix fragments at chunk boundaries - // These could be the start of query responses split across chunks - // Covers: ESC (any), ESC[ (CSI), ESC] (OSC), ESC P (DCS) - if (combined.endsWith(ESC)) { - this.buffer = ESC; - const toFilter = combined.slice(0, -1); - return toFilter.replace(COMBINED_PATTERN, ""); - } - if (combined.endsWith(`${ESC}[`)) { - this.buffer = `${ESC}[`; - const toFilter = combined.slice(0, -2); - return toFilter.replace(COMBINED_PATTERN, ""); - } - if (combined.endsWith(`${ESC}]`)) { - this.buffer = `${ESC}]`; - const toFilter = combined.slice(0, -2); - return toFilter.replace(COMBINED_PATTERN, ""); - } - if (combined.endsWith(`${ESC}P`)) { - this.buffer = `${ESC}P`; - const toFilter = combined.slice(0, -2); - return toFilter.replace(COMBINED_PATTERN, ""); - } + // Pre-check: Buffer trailing prefix fragments at chunk boundaries + // These could be the start of query responses split across chunks + // Covers: ESC (any), ESC[ (CSI), ESC] (OSC), ESC P (DCS) + const prefixFragments = [`${ESC}P`, `${ESC}[`, `${ESC}]`, ESC]; + for (const prefix of prefixFragments) { + if (combined.endsWith(prefix)) { + this.buffer = prefix; + const toFilter = combined.slice(0, -prefix.length); + return toFilter.replace(COMBINED_PATTERN, ""); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/main/lib/terminal-escape-filter.test.ts(3 hunks)apps/desktop/src/main/lib/terminal-escape-filter.ts(2 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-escape-filter.test.tsapps/desktop/src/main/lib/terminal-escape-filter.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.test.tsapps/desktop/src/main/lib/terminal-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-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-escape-filter.test.tsapps/desktop/src/main/lib/terminal-escape-filter.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-escape-filter.test.tsapps/desktop/src/main/lib/terminal-escape-filter.ts
🧠 Learnings (1)
📚 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-escape-filter.ts
🧬 Code graph analysis (1)
apps/desktop/src/main/lib/terminal-escape-filter.test.ts (1)
apps/desktop/src/main/lib/terminal-escape-filter.ts (2)
filter(115-165)TerminalEscapeFilter(108-271)
⏰ 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 (4)
apps/desktop/src/main/lib/terminal-escape-filter.ts (1)
256-261: LGTM!The flush logic correctly preserves standalone prefix fragments that never formed a query response. This ensures genuine ESC bytes aren't incorrectly filtered when the terminal session ends.
apps/desktop/src/main/lib/terminal-escape-filter.test.ts (3)
420-482: Comprehensive test coverage for chunk boundary buffering.The new tests thoroughly cover all prefix fragment cases (ESC, ESC[, ESC], ESC P) and their reassembly scenarios. The test naming and comments clearly explain the expected behavior.
541-576: LGTM!The flush behavior tests correctly verify that standalone prefix fragments (ESC, ESC[, ESC], ESC P) are preserved rather than filtered when the session ends. Each test is focused and independent.
595-606: LGTM!The updated test correctly reflects the new buffering behavior where trailing ESC is buffered and reassembled with the next chunk. The combined output remains correct.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.