Skip to content

Conversation

@meganrogge
Copy link
Contributor

@meganrogge meganrogge commented Nov 21, 2025

fixes #276946

Copilot AI review requested due to automatic review settings November 21, 2025 00:46
@meganrogge meganrogge marked this pull request as draft November 21, 2025 00:47
@meganrogge meganrogge added this to the November 2025 milestone Nov 21, 2025
@meganrogge meganrogge self-assigned this Nov 21, 2025
@meganrogge meganrogge changed the title wip rendering xterm instead of html wip rendering xterm instead of html for the chat terminal output Nov 21, 2025
@meganrogge meganrogge changed the title wip rendering xterm instead of html for the chat terminal output render xterm instead of html for the chat terminal output Nov 21, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors terminal command output rendering to use xterm's VT sequences instead of HTML serialization. The change introduces a new DetachedTerminalCommandMirror class that creates a detached terminal instance to render command output using VT sequences, replacing the previous HTML-based approach.

Key changes:

  • Introduces DetachedTerminalCommandMirror class to handle VT-based terminal rendering
  • Adds getRangeAsVT() method to XtermTerminal for extracting VT sequences between markers
  • Removes fallback output parameter and HTML serialization logic from TerminalCommandArtifactCollector
  • Simplifies ChatTerminalToolOutputSection by removing HTML sanitization and static output rendering

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
chatTerminalCommandMirror.ts New class that creates detached terminal instances to render command output using VT sequences
xtermTerminal.ts Adds getRangeAsVT() method to extract VT sequences between markers using the serialize addon
terminal.ts Adds getRangeAsVT() interface method and makes raw property available on IDetachedXtermTerminal
terminalCommandArtifactCollector.ts Simplifies capture logic by removing HTML serialization and fallback output handling
runInTerminalTool.ts Updates calls to remove fallback output parameter
chatTerminalToolProgressPart.ts Major refactor to use VT rendering instead of HTML, removes sanitization logic
chatTerminalToolProgressPart.css Updates styles for terminal container and adds styling for empty state

Tyriar
Tyriar previously requested changes Nov 21, 2025
Tyriar
Tyriar previously requested changes Nov 21, 2025
@meganrogge meganrogge marked this pull request as ready for review November 22, 2025 02:09
@meganrogge meganrogge enabled auto-merge (squash) November 22, 2025 02:48
Tyriar
Tyriar previously requested changes Nov 22, 2025
* Mirrors a terminal command's output into a detached terminal instance.
* Used in the chat terminal tool progress part to show command output for example.
*/
export class DetachedTerminalCommandMirror extends Disposable implements IDetachedTerminalCommandMirror {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want some unit tests for this, they'll be a lot more important in the next PR though. Basically they should write stuff to XtermTerminal, pass in the command and verify they both serialize to the same text. For the streaming PR we want it to check as things are progressing.

meganrogge and others added 5 commits November 21, 2025 21:01
…irror.ts

Co-authored-by: Daniel Imms <2193314+Tyriar@users.noreply.github.com>
…ocationParts/chatTerminalToolProgressPart.ts

Co-authored-by: Daniel Imms <2193314+Tyriar@users.noreply.github.com>
Co-authored-by: Daniel Imms <2193314+Tyriar@users.noreply.github.com>
@meganrogge meganrogge merged commit 60706b4 into main Nov 22, 2025
28 checks passed
@meganrogge meganrogge deleted the anxious-cuckoo branch November 22, 2025 03:36
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.

Embed xterm.js instead of text content

3 participants