add terminal output dropdown, reveal command on focus #273175
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds terminal output dropdown functionality and implements command reveal on focus. It integrates terminal commands as attachments in chat, allowing users to attach terminal command output to chat conversations and navigate directly to commands in the terminal.
Key Changes:
- Added support for terminal command attachments in chat with associated UI widgets and hover previews
- Implemented command reveal functionality that scrolls to and highlights commands when terminal instances are focused
- Extended terminal URI handling to support command IDs for direct navigation to specific commands
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
terminalCommand.ts |
Added unique ID and session tracking to terminal commands |
capabilities.ts |
Extended command interfaces with ID and chat session fields |
decorationAddon.ts |
Added "Attach To Chat" action for terminal commands |
xtermTerminal.ts |
Updated constructor to accept resource parameter for URI-based navigation |
terminalUri.ts |
Extended URI generation to include command IDs |
terminalService.ts |
Implemented openResource method to reveal commands in terminals |
chatVariableEntries.ts |
Added ITerminalVariableEntry type for terminal command attachments |
chatAttachmentWidgets.ts |
Implemented TerminalCommandAttachmentWidget for displaying terminal commands |
chatContext.ts |
Added TerminalContext class for terminal command context integration |
chatTerminalToolProgressPart.ts |
Added collapsible output preview with show/hide functionality |
| Test files | Updated test instantiations to match new constructor signatures |
Co-authored-by: Copilot <[email protected]>
...ch/contrib/chat/browser/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts
Outdated
Show resolved
Hide resolved
| if (!terminalInstance) { | ||
| return { text: '', truncated: false }; | ||
| } |
There was a problem hiding this comment.
_collectOutput is only called when terminalInstance is truthy I believe
| const output = await this._collectOutput(this._terminalInstance); | ||
| const content = this._renderOutput(output); | ||
| const theme = this._terminalInstance?.xterm?.getXtermTheme(); |
There was a problem hiding this comment.
xterm is used in collect output and in this function. There should be a single xtermReadyPromise call and a single check whether it's truthy.
| const content = this._renderOutput(output); | ||
| const theme = this._terminalInstance?.xterm?.getXtermTheme(); | ||
| if (theme) { | ||
| const inlineTerminal = content.querySelector('div'); |
There was a problem hiding this comment.
Should use the h helper, and keep track of elements we care about. We should very rarely query sub-elements like this. Doing this should also make the if (inlineTerminal) on the following line unnecessary.
| const text = await xterm.getCommandOutputAsHtml(command, CHAT_TERMINAL_OUTPUT_MAX_PREVIEW_LINES); | ||
| if (!text) { | ||
| return { text: '', truncated: false }; | ||
| } | ||
|
|
||
| return { text, truncated: false }; |
There was a problem hiding this comment.
This if (!text) seems redundant as getCommandOutputAsHtml returns a string and when it's '' the behavior would be the same if it fell through.
| this._terminalService.setActiveInstance(this._instance); | ||
| if (this._instance.target === TerminalLocation.Editor) { | ||
| this._terminalEditorService.openEditor(this._instance); | ||
| } else { | ||
| this._terminalGroupService.showPanel(true); | ||
| await this._terminalGroupService.showPanel(true); | ||
| } | ||
| this._terminalService.setActiveInstance(this._instance); |
There was a problem hiding this comment.
setActiveInstance is now run twice? This shouldn't be needed. What case were you trying to fix here, as this code is relatively fragile and caused a new insiders to go out when changed before.
| // Trim empty lines from the end | ||
| let emptyLinesFromEnd = 0; | ||
| for (let i = endLine; i >= startLine; i--) { | ||
| const line = this.raw.buffer.active.getLine(i); | ||
| if (line && line.translateToString(true).trim() === '') { | ||
| emptyLinesFromEnd++; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
| endLine = endLine - emptyLinesFromEnd; | ||
|
|
||
| // Trim empty lines from the start | ||
| let emptyLinesFromStart = 0; | ||
| for (let i = startLine; i <= endLine; i++) { | ||
| const line = this.raw.buffer.active.getLine(i); | ||
| if (line && line.translateToString(true).trim() === '') { | ||
| emptyLinesFromStart++; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
| startLine = startLine + emptyLinesFromStart; |
| const bufferLine = this.raw.buffer.active.getLine(startLine); | ||
| if (bufferLine) { | ||
| startCol = Math.min(startCol, bufferLine.length); | ||
| } |
There was a problem hiding this comment.
It's not clear to me if this does anything
| const range = { startLine, endLine, startCol }; | ||
| const result = this._serializeAddon.serializeAsHTML({ range }); | ||
| return result; |
There was a problem hiding this comment.
nit: Easier to read when it's all on the same line imo
return this._serializeAddon.serializeAsHTML({ startLine, endLine, startCol });
fixes #271387
fixes #271388
Also updates xterm
demo.mov