chat: combine adjacent code blocks in MCP tool output#289272
Conversation
Combines adjacent text code blocks in ChatToolOutputContentSubPart to reduce the number of editor models and listeners created when tools return multiple separate output parts. When an MCP tool outputs many lines (e.g., 100 lines), they are now merged into a single code block instead of creating 100 separate blocks, which significantly improves responsiveness and prevents listener leak warnings. - Modified createOutputContents() to group consecutive code parts - Modified addCodeBlock() to accept an array of parts and combine their text content with newline separators - Reduces model/listener creation for high-volume MCP tool outputs Fixes #279624 (Commit message generated by Copilot)
There was a problem hiding this comment.
Pull request overview
This pull request aims to improve performance when MCP tools return large amounts of output by combining adjacent code blocks into a single editor instance, thereby reducing the number of text models and event listeners created.
Changes:
- Modified
createOutputContents()to collect consecutive code parts into arrays before rendering - Changed
addCodeBlock()to accept an array of code parts and combine their text content with newline separators - Combined text from multiple parts is set into the first part's text model
Comments suppressed due to low confidence (3)
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatToolOutputContentSubPart.ts:181
- When combining parts, only the firstPart's renderOptions are used. If subsequent parts have different render options (such as different maxHeightInLines, different editor settings, or different toolbar visibility), those options will be ignored. This could result in unexpected rendering behavior. Consider only combining parts that have identical or compatible render options.
renderOptions: firstPart.options,
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatToolOutputContentSubPart.ts:189
- When combining multiple code parts, only the firstPart's codeBlockInfo is tracked in this.codeblocks. This could cause issues if code block tracking or navigation features expect all code blocks to be registered. Consider whether all combined parts' codeBlockInfo should be tracked, or if the codeBlockInfo should be updated to reflect the combined nature of the block.
this.codeblocks.push(firstPart.codeBlockInfo);
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatToolOutputContentSubPart.ts:175
- When combining parts with different languageIds, only the firstPart's languageId is used. If adjacent code parts have different language IDs (e.g., one is JSON and another is plaintext), combining them into a single code block with only the first language ID will result in incorrect syntax highlighting for the subsequent parts. Consider either: 1) only combining parts with matching language IDs, or 2) using a generic/plaintext language ID for combined blocks with mixed languages.
languageId: firstPart.languageId,
| if (firstPart.title) { | ||
| const title = dom.$('div.chat-confirmation-widget-title'); | ||
| const renderedTitle = this._register(this._markdownRendererService.render(this.toMdString(part.title))); | ||
| const renderedTitle = this._register(this._markdownRendererService.render(this.toMdString(firstPart.title))); | ||
| title.appendChild(renderedTitle.element); | ||
| container.appendChild(title); | ||
| } |
There was a problem hiding this comment.
When combining parts, only the firstPart's title is displayed. If subsequent parts have different titles, they will be silently ignored. This could lead to loss of important contextual information. Consider either: 1) only combining parts when they have the same title (or all lack titles), or 2) displaying all unique titles in some way, or 3) documenting this behavior if it's intentional.
This issue also appears in the following locations of the same file:
- line 181
- line 189
- line 175
| // Combine text from all adjacent code parts | ||
| const combinedText = parts.map(p => p.textModel.getValue()).join('\n'); | ||
| firstPart.textModel.setValue(combinedText); |
There was a problem hiding this comment.
Mutating the firstPart.textModel creates a memory leak and potential side effects. The text models from parts[1] onwards are extracted via getValue() but the models themselves are never disposed. These models were created and registered in their original context (e.g., in chatInputOutputMarkdownProgressPart.ts or chatToolPostExecuteConfirmationPart.ts), and modifying the firstPart's model could have unintended side effects if that model is referenced elsewhere.
Instead of mutating the firstPart.textModel, create a new combined text model and dispose of the individual models that are no longer needed, or ensure the non-primary models are properly disposed.
Combines adjacent text code blocks in ChatToolOutputContentSubPart to
reduce the number of editor models and listeners created when tools
return multiple separate output parts. When an MCP tool outputs many
lines (e.g., 100 lines), they are now merged into a single code block
instead of creating 100 separate blocks, which significantly improves
responsiveness and prevents listener leak warnings.
their text content with newline separators
Fixes #279624
(Commit message generated by Copilot)