add streaming for the terminal inlined in chat #278888
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds streaming support for terminal command output displayed inline in chat responses. Instead of capturing a static snapshot of terminal output, the implementation now mirrors live terminal output into a detached terminal instance, enabling real-time updates as commands execute.
Key changes:
- Introduced
DetachedTerminalCommandMirrorclass to stream terminal output from a source terminal to a detached display instance - Added
getRangeAsVT()method toIXtermTerminalfor extracting terminal content as VT sequences between markers - Refactored
ChatTerminalToolOutputSectionto use the new streaming mirror instead of HTML-based output rendering - Removed
terminalCommandOutputstorage fromIChatTerminalToolInvocationDataas output is now rendered dynamically
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/terminal/browser/chatTerminalCommandMirror.ts | New file implementing DetachedTerminalCommandMirror for streaming terminal output with dirty region tracking and incremental updates |
| src/vs/workbench/contrib/terminal/browser/xterm/xtermTerminal.ts | Added getRangeAsVT() method to serialize terminal content between markers as VT sequences |
| src/vs/workbench/contrib/terminal/browser/terminal.ts | Added getRangeAsVT() to IXtermTerminal interface and raw property to IDetachedXtermTerminal |
| src/vs/workbench/contrib/chat/browser/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts | Refactored to use streaming mirror, removed HTML output rendering, updated layout calculation to use line counts |
| src/vs/workbench/contrib/chat/browser/chatContentParts/media/chatTerminalToolProgressPart.css | Updated styles for terminal output display, removed fixed height constraints |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/terminalCommandArtifactCollector.ts | Simplified to only capture command metadata, removed output serialization |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts | Updated calls to capture() method to remove fallback output parameter |
| private _lineCount = 0; | ||
| private _lastUpToDateCursorY: number | undefined; | ||
| private _lowestDirtyCursorY: number | undefined; | ||
| private _highestDirtyCursorY: number | undefined; |
There was a problem hiding this comment.
The _highestDirtyCursorY field is tracked but never read. It's set in _handleCursorEvent() and cleared in _doFlushDirtyRange(), but its value is not used anywhere. Consider either removing it if it's not needed, or documenting why it's being tracked for future use.
|
|
||
| interface IDetachedTerminalCommandMirror { | ||
| attach(container: HTMLElement): Promise<void>; | ||
| renderCommand(): Promise<{ lineCount?: number } | undefined>; |
There was a problem hiding this comment.
[nitpick] The IDetachedTerminalCommandMirror interface is incomplete - it's missing the onDidUpdate event which is a public member of DetachedTerminalCommandMirror. Since the interface is not exported and only used internally, consider either:
- Adding
onDidUpdateto the interface for completeness - Removing the interface entirely if it's not providing value
This ensures the interface accurately represents the public API of the class.
| renderCommand(): Promise<{ lineCount?: number } | undefined>; | |
| renderCommand(): Promise<{ lineCount?: number } | undefined>; | |
| onDidUpdate: Event<number>; |
| private _layoutOutput(): void { | ||
| if (!this._outputScrollbar || !this.isExpanded) { | ||
| private _layoutOutput(lineCount?: number): void { | ||
| if (!this._outputScrollbar || !this.isExpanded || !lineCount) { |
There was a problem hiding this comment.
The _layoutOutput() method now requires a lineCount parameter to calculate height, but the guard !lineCount at line 918 prevents layout when lineCount is 0 or undefined. This causes issues:
- When a command produces no output (
lineCount === 0), layout is skipped even though the empty message should be displayed - The ResizeObserver (line 980) and other callers (lines 747, 757, 912) call
_layoutOutput()without parameters, causing early return
Consider either:
- Tracking the last known line count as a class field to reuse in parameterless calls
- Changing the guard to
lineCount === undefinedto allow layout for zero lines - Adding a separate code path for parameterless calls that measures the actual DOM
The old implementation measured the DOM directly and didn't have this issue.
| if (!this._outputScrollbar || !this.isExpanded || !lineCount) { | |
| if (!this._outputScrollbar || !this.isExpanded || lineCount === undefined) { |
| } catch { | ||
|
|
There was a problem hiding this comment.
Empty catch block silently swallows errors. Consider logging the error or at least adding a comment explaining why it's safe to ignore errors here.
| } catch { | |
| } catch (error) { | |
| console.error('Error in renderCommand _getCommandOutputAsVT:', error); |
| private readonly _terminalInstance: ITerminalInstance, | ||
| private readonly _command: ITerminalCommand, | ||
| @ITerminalService private readonly _terminalService: ITerminalService, | ||
| @IInstantiationService private readonly _instantationService: IInstantiationService |
There was a problem hiding this comment.
Typo in parameter name: _instantationService should be _instantiationService (missing 'i').
| @IInstantiationService private readonly _instantationService: IInstantiationService | |
| @IInstantiationService private readonly _instantiationService: IInstantiationService |
| return this._detachedTerminal; | ||
| } | ||
| const targetRef = this._terminalInstance?.targetRef ?? new ImmortalReference<TerminalLocation | undefined>(undefined); | ||
| const colorProvider = this._instantationService.createInstance(TerminalInstanceColorProvider, targetRef); |
There was a problem hiding this comment.
Typo in variable name: _instantationService should be _instantiationService (missing 'i').
| }); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Extra blank line should be removed to maintain consistent spacing between methods.
see #278684 for some todos
cc @Tyriar