Skip to content

fix(desktop): suppress terminal query responses using xterm.js parser…#196

Merged
Kitenite merged 1 commit intomainfrom
pearl-river-77
Dec 1, 2025
Merged

fix(desktop): suppress terminal query responses using xterm.js parser…#196
Kitenite merged 1 commit intomainfrom
pearl-river-77

Conversation

@Kitenite
Copy link
Copy Markdown
Collaborator

@Kitenite Kitenite commented Dec 1, 2025

… hooks

Replace hacky PTY-level escape sequence filtering with proper xterm.js parser hooks. Terminal query responses (DA1, DA2, CPR, OSC color responses) are now suppressed at the display layer using the official xterm.js API.

Changes:

  • Remove terminal-escape-filter.ts module and its tests
  • Add suppressQueryResponses.ts with xterm.js parser hooks
  • Update terminal-manager.ts to pass raw data through
  • Integrate parser hooks in terminal creation

The parser hooks intercept CSI sequences ending in 'c' (device attributes), 'R' (cursor position reports), '$y' (mode reports), and OSC 10-19 (color query responses) - returning true to suppress display.

🤖 Generated with Claude Code

Description

Related Issues

Type of Change

  • Bug fix
  • New feature
  • Documentation
  • Refactor
  • Other (please describe):

Testing

Screenshots (if applicable)

Additional Notes

Summary by CodeRabbit

  • Refactor
    • Improved terminal response handling by transitioning from post-processing filters to protocol-level suppression of device query responses.

✏️ Tip: You can customize this high-level summary in your review settings.

@vercel
Copy link
Copy Markdown

vercel Bot commented Dec 1, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
website Ready Ready Preview Comment Dec 1, 2025 1:28am

… hooks

Replace hacky PTY-level escape sequence filtering with proper xterm.js
parser hooks. Terminal query responses (DA1, DA2, CPR, OSC color responses)
are now suppressed at the display layer using the official xterm.js API.

Changes:
- Remove terminal-escape-filter.ts module and its tests
- Add suppressQueryResponses.ts with xterm.js parser hooks
- Update terminal-manager.ts to pass raw data through
- Integrate parser hooks in terminal creation

The parser hooks intercept CSI sequences ending in 'c' (device attributes),
'R' (cursor position reports), '$y' (mode reports), and OSC 10-19 (color
query responses) - returning true to suppress display.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Dec 1, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

This change replaces a regex-based terminal escape sequence filter module with xterm.js parser-hook-based suppression. The TerminalEscapeFilter class and its integration are removed, and a new suppressQueryResponses function uses xterm parser handlers to intercept protocol-level terminal responses (device attributes, cursor reports, mode reports, color queries) at the parser level.

Changes

Cohort / File(s) Summary
Removal of regex-based filter module
apps/desktop/src/main/lib/terminal-escape-filter.ts, apps/desktop/src/main/lib/terminal-escape-filter.test.ts
Deleted entire terminal escape filter module including TerminalEscapeFilter class, filterTerminalQueryResponses function, FILTER_PATTERNS constants, and all associated regex patterns. Removed complete test suite covering filter behavior, state buffering, and edge cases.
TerminalManager integration update
apps/desktop/src/main/lib/terminal-manager.ts, apps/desktop/src/main/lib/terminal-manager.test.ts
Removed TerminalEscapeFilter import and escapeFilter member from TerminalSession. Replaced filtered data handling with raw data passthrough; eliminated buffer flush logic on session exit. Simplified test to verify raw data passthrough instead of filtering behavior.
New xterm parser-hook suppression
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.ts
Added new module exporting suppressQueryResponses function that registers xterm parser hooks to suppress CSI responses (c, R), mode report finals (y), and OSC codes (10–19). Returns cleanup function to dispose all registered handlers.
Terminal component and helpers refactoring
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx, apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
Updated createTerminalInstance to invoke suppressQueryResponses and return cleanup function alongside xterm and fitAddon. Terminal component now invokes cleanup on unmount as part of teardown sequence.

Sequence Diagram(s)

sequenceDiagram
    participant Term as Terminal Component
    participant Helper as createTerminalInstance
    participant XTerm as xterm Instance
    participant Parser as xterm Parser
    participant Handlers as Query Response Handlers

    Term->>Helper: createTerminalInstance(container, cwd, theme)
    Helper->>XTerm: new Terminal()
    Helper->>XTerm: create FitAddon
    Helper->>XTerm: registerHandler(CSI 'c') - DA
    Helper->>XTerm: registerHandler(CSI 'R') - CPR
    Helper->>XTerm: registerHandler(CSI $ 'y') - Mode Report
    Helper->>XTerm: registerHandler(OSC 10-19) - Colors
    Helper->>Handlers: return cleanup function
    Helper-->>Term: {xterm, fitAddon, cleanup}
    
    Note over Parser: Terminal receives DA/CPR/OSC<br/>sequences from pty
    Parser->>Handlers: executeCSI 'c'
    Handlers->>Handlers: suppress (return TRUE)
    Parser->>Handlers: executeCSI 'R'
    Handlers->>Handlers: suppress (return TRUE)
    
    Term->>Term: Component unmounts
    Term->>Term: cleanup()
    Handlers->>Handlers: dispose all registered handlers
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Missing test coverage: suppressQueryResponses.ts lacks unit tests; verify handler logic and cleanup disposal work correctly across different sequence types.
  • Parser hook side effects: Confirm CSI and OSC handler registration doesn't interfere with other terminal features or leak memory if cleanup is delayed.
  • OSC code range validation: Verify OSC codes 10–19 cover all necessary color-related responses; check if extended or alternative codes need suppression.
  • Complete removal verification: Ensure all references to TerminalEscapeFilter and FILTER_PATTERNS are eliminated from codebase (check imports, type references, and documentation).

Possibly related PRs

Poem

🐰 The old regex guards fade away,
Parser hooks take care today,
Query whispers vanish in the stream,
A cleaner terminal—our dream!

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch pearl-river-77

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c486937 and fc1d0d7.

📒 Files selected for processing (7)
  • apps/desktop/src/main/lib/terminal-escape-filter.test.ts (0 hunks)
  • apps/desktop/src/main/lib/terminal-escape-filter.ts (0 hunks)
  • apps/desktop/src/main/lib/terminal-manager.test.ts (1 hunks)
  • apps/desktop/src/main/lib/terminal-manager.ts (1 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 (4 hunks)
  • apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.ts (1 hunks)

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant