Repository navigation
Conversation
WalkthroughAdds documentation describing escape sequence suppression in the terminal renderer, and expands the suppressQueryResponses handler to intercept and suppress a broader range of CSI and OSC control sequences, including device status reports, mode queries, and terminal capability requests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![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/docs/ESCAPE_SEQUENCE_HANDLING.md (1)
99-100: Consider using HTTPS for documentation links.The xterm.js documentation links use HTTP. While these may redirect to HTTPS, using HTTPS directly is better practice.
-- [xterm.js Parser Hooks Documentation](http://xtermjs.org/docs/guides/hooks/) -- [xterm.js Parser API](http://xtermjs.org/docs/api/terminal/interfaces/iparser/) +- [xterm.js Parser Hooks Documentation](https://xtermjs.org/docs/guides/hooks/) +- [xterm.js Parser API](https://xtermjs.org/docs/api/terminal/interfaces/iparser/)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.ts (1)
66-72: Minor documentation clarification for 'q' sequences.The comment mentions DECRQSS, but DECRQSS typically uses DCS (Device Control String) sequences, not CSI. CSI 'q' sequences are more commonly associated with DECLL (Load LEDs) or cursor style queries. The suppression itself is correct, but the comment could be more precise.
- // CSI sequences ending in 'q' - Terminal ID and Capability Queries - // DECRQSS: ESC[?q - Request Status String (query) + // CSI sequences ending in 'q' - LED and Terminal Queries + // DECLL: ESC[0q - Load LEDs
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/docs/ESCAPE_SEQUENCE_HANDLING.md(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.ts(5 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/suppressQueryResponses.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/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.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/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.ts
**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/components/**/*.{ts,tsx}: Structure project folders as one folder per component with PascalCase naming (ComponentName/ComponentName.tsx + index.ts barrel export)
Co-locate component dependencies (utils, hooks, constants, config, tests, stories) next to the file using them
Use one component per file (no multi-component files)
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Call IPC methods from renderer process using window.ipcRenderer.invoke with type-safe object parameters
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.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). (6)
- GitHub Check: Deploy Web
- GitHub Check: Deploy API
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Marketing
- GitHub Check: Build
🔇 Additional comments (5)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.ts (5)
3-18: Clear and informative documentation.The docstring effectively explains the purpose, context, and return value of this function. Good practice documenting the "why" behind the suppression logic.
27-45: Correct implementation of core CSI query suppressions.The handlers for Device Attributes (
c), Cursor Position Reports (R), and Mode Reports ($y) are properly configured with appropriate final characters and intermediates.
123-129: Clean loop-based OSC handler registration.Using a loop for OSC 10-19 is maintainable and clear. The comprehensive comments explaining why other OSC sequences (4, 52, 1337) are intentionally not suppressed demonstrate good design reasoning.
175-179: Proper disposal pattern.The cleanup function correctly disposes all registered handlers, preventing memory leaks when the terminal is torn down.
19-21: Well-structured implementation with proper typing.The implementation follows TypeScript best practices with proper type imports from
@xterm/xterm, uses the disposables pattern correctly, and organizes handlers logically with clear section markers.
| ### How Hyper Handles It | ||
|
|
||
| Hyper uses xterm.js (like we do) but **does not use parser hooks** to suppress escape sequences. They rely entirely on xterm.js's default behavior. This means Hyper trusts that xterm.js handles query responses correctly internally. | ||
|
|
||
| However, we've found that some query responses can still leak into the terminal output, which is why we implement explicit suppression using parser hooks. | ||
|
|
||
| ### Our Implementation | ||
|
|
||
| We use the same parser hook approach as Hyper. The `suppressQueryResponses` function registers handlers for various escape sequence types: |
There was a problem hiding this comment.
Contradictory statements about Hyper's approach.
Line 19 states Hyper "does not use parser hooks" to suppress escape sequences, but line 25 says "We use the same parser hook approach as Hyper." These statements contradict each other.
Suggested fix:
-We use the same parser hook approach as Hyper. The `suppressQueryResponses` function registers handlers for various escape sequence types:
+Unlike Hyper, we explicitly register parser hooks via the `suppressQueryResponses` function to handle various escape sequence types:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### How Hyper Handles It | |
| Hyper uses xterm.js (like we do) but **does not use parser hooks** to suppress escape sequences. They rely entirely on xterm.js's default behavior. This means Hyper trusts that xterm.js handles query responses correctly internally. | |
| However, we've found that some query responses can still leak into the terminal output, which is why we implement explicit suppression using parser hooks. | |
| ### Our Implementation | |
| We use the same parser hook approach as Hyper. The `suppressQueryResponses` function registers handlers for various escape sequence types: | |
| ### How Hyper Handles It | |
| Hyper uses xterm.js (like we do) but **does not use parser hooks** to suppress escape sequences. They rely entirely on xterm.js's default behavior. This means Hyper trusts that xterm.js handles query responses correctly internally. | |
| However, we've found that some query responses can still leak into the terminal output, which is why we implement explicit suppression using parser hooks. | |
| ### Our Implementation | |
| Unlike Hyper, we explicitly register parser hooks via the `suppressQueryResponses` function to handle various escape sequence types: |
🤖 Prompt for AI Agents
apps/desktop/docs/ESCAPE_SEQUENCE_HANDLING.md around lines 17 to 25: the two
sentences contradict each other — one claims Hyper does not use parser hooks
while the next claims we use the same parser hook approach as Hyper; update the
text so it consistently describes Hyper's actual behavior and then contrasts it
with our implementation (for example, state clearly whether Hyper relies on
xterm.js defaults or uses parser hooks, then say "By contrast, we implement
explicit suppression using parser hooks" or, if Hyper does use parser hooks,
reword to "Like Hyper, we use parser hooks" so both lines align).
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 Preview resources have been processed for cleanup |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.