From 1c11056c11d0010dafb2029f56556175b201d67e Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Tue, 17 Mar 2026 15:27:39 -0700 Subject: [PATCH 1/2] fix: reduce listener leak from per-part show-checkmarks subscriptions (#301186) Move the show-checkmarks class toggle from individual content parts to the row container level in chatListRenderer. This replaces N per-part onDidChangeConfiguration subscriptions (one per tool invocation, terminal tool, and code block pill) with a single subscription per rendered chat element, significantly reducing listener accumulation on the configuration service emitter in sessions with many tool invocations. --- .../chatMarkdownContentPart.ts | 9 --------- .../chatTerminalToolProgressPart.ts | 10 +--------- .../chatToolProgressPart.ts | 9 --------- .../chat/browser/widget/chatListRenderer.ts | 19 +++++++++++-------- .../chat/browser/widget/media/chat.css | 2 +- 5 files changed, 13 insertions(+), 36 deletions(-) diff --git a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatMarkdownContentPart.ts b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatMarkdownContentPart.ts index e1cb498c7cd..8197b51a71d 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatMarkdownContentPart.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatMarkdownContentPart.ts @@ -526,15 +526,6 @@ export class CollapsedCodeBlock extends Disposable { this.element.appendChild(this.statusIndicatorContainer); this.element.appendChild(this.pillElement); - // Toggle show-checkmarks class for the accessibility setting - const updateCheckmarks = () => this.element.classList.toggle('show-checkmarks', !!this.configurationService.getValue(AccessibilityWorkbenchSettingId.ShowChatCheckmarks)); - updateCheckmarks(); - this._register(this.configurationService.onDidChangeConfiguration(e => { - if (e.affectsConfiguration(AccessibilityWorkbenchSettingId.ShowChatCheckmarks)) { - updateCheckmarks(); - } - })); - this.registerListeners(); } diff --git a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts index 8f70589d4d4..640f0847c16 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts @@ -45,7 +45,7 @@ import { URI } from '../../../../../../../base/common/uri.js'; import { stripIcons } from '../../../../../../../base/common/iconLabels.js'; import { IAccessibleViewService } from '../../../../../../../platform/accessibility/browser/accessibleView.js'; import { IContextKey, IContextKeyService } from '../../../../../../../platform/contextkey/common/contextkey.js'; -import { AccessibilityVerbositySettingId, AccessibilityWorkbenchSettingId } from '../../../../../accessibility/browser/accessibilityConfiguration.js'; +import { AccessibilityVerbositySettingId } from '../../../../../accessibility/browser/accessibilityConfiguration.js'; import { ChatContextKeys } from '../../../../common/actions/chatContextKeys.js'; import { EditorPool } from '../chatContentCodePools.js'; import { IKeybindingService } from '../../../../../../../platform/keybinding/common/keybinding.js'; @@ -422,14 +422,6 @@ export class ChatTerminalToolProgressPart extends BaseChatToolInvocationSubPart this.domNode = this._createCollapsibleWrapper(progressPart.domNode, displayCommand, toolInvocation, context); } else { this.domNode = progressPart.domNode; - // Toggle show-checkmarks class on the progress container for accessibility setting - const updateCheckmarks = () => this.domNode.classList.toggle('show-checkmarks', !!this._configurationService.getValue(AccessibilityWorkbenchSettingId.ShowChatCheckmarks)); - updateCheckmarks(); - this._register(this._configurationService.onDidChangeConfiguration(e => { - if (e.affectsConfiguration(AccessibilityWorkbenchSettingId.ShowChatCheckmarks)) { - updateCheckmarks(); - } - })); } this._renderImagePills(toolInvocation, context, elements.container); diff --git a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatToolProgressPart.ts b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatToolProgressPart.ts index f142f629438..15be78e1fe8 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatToolProgressPart.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatToolProgressPart.ts @@ -35,15 +35,6 @@ export class ChatToolProgressSubPart extends BaseChatToolInvocationSubPart { super(toolInvocation); this.domNode = this.createProgressPart(); - - // Toggle show-checkmarks class for the accessibility setting - const updateCheckmarks = () => this.domNode.classList.toggle('show-checkmarks', !!this.configurationService.getValue(AccessibilityWorkbenchSettingId.ShowChatCheckmarks)); - updateCheckmarks(); - this._register(this.configurationService.onDidChangeConfiguration(e => { - if (e.affectsConfiguration(AccessibilityWorkbenchSettingId.ShowChatCheckmarks)) { - updateCheckmarks(); - } - })); } private createProgressPart(): HTMLElement { diff --git a/src/vs/workbench/contrib/chat/browser/widget/chatListRenderer.ts b/src/vs/workbench/contrib/chat/browser/widget/chatListRenderer.ts index 3b69200aa20..8752ac7acbf 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/chatListRenderer.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/chatListRenderer.ts @@ -744,6 +744,17 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer templateData.rowContainer.classList.toggle('show-checkmarks', !!this.configService.getValue(AccessibilityWorkbenchSettingId.ShowChatCheckmarks)); + updateContainerCheckmarks(); + templateData.elementDisposables.add(this.configService.onDidChangeConfiguration(e => { + if (e.affectsConfiguration(AccessibilityWorkbenchSettingId.ShowChatCheckmarks)) { + updateContainerCheckmarks(); + } + })); + if (!this.rendererOptions.noHeader) { this.renderAvatar(element, templateData); } @@ -928,14 +939,6 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer templateData.detail.classList.toggle('show-checkmarks', !!this.configService.getValue(AccessibilityWorkbenchSettingId.ShowChatCheckmarks)); - updateCheckmarks(); - templateData.elementDisposables.add(this.configService.onDidChangeConfiguration(e => { - if (e.affectsConfiguration(AccessibilityWorkbenchSettingId.ShowChatCheckmarks)) { - updateCheckmarks(); - } - })); } } diff --git a/src/vs/workbench/contrib/chat/browser/widget/media/chat.css b/src/vs/workbench/contrib/chat/browser/widget/media/chat.css index 402c43396a3..4478e007d9d 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/media/chat.css +++ b/src/vs/workbench/contrib/chat/browser/widget/media/chat.css @@ -2900,7 +2900,7 @@ have to be updated for changes to the rules above, or to support more deeply nes display: none; } - .interactive-item-container .header.partially-disabled .detail.show-checkmarks { + .interactive-item-container.show-checkmarks .header.partially-disabled .detail { margin-left: 4px; .codicon-check { From e0e770689fe994f225bbfd9d91dd626cf7793a49 Mon Sep 17 00:00:00 2001 From: Bryan Chen Date: Tue, 17 Mar 2026 16:32:44 -0700 Subject: [PATCH 2/2] fix: update instructions for handling Copilot review comments in PRs --- .github/prompts/fix-error.prompt.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/prompts/fix-error.prompt.md b/.github/prompts/fix-error.prompt.md index fcd757363fa..23d6174f3fd 100644 --- a/.github/prompts/fix-error.prompt.md +++ b/.github/prompts/fix-error.prompt.md @@ -30,8 +30,8 @@ After the fix is validated (compilation clean, tests pass): - Steps a user can follow to manually validate the fix. - How the fix addresses the issue, with a brief note per changed file. 5. **Monitor the PR** for Copilot review comments. Wait 1-2 minutes after each push for Copilot to leave its review, then check for new comments. Evaluate each comment: - - If valid, apply the fix, amend the commit, and force-push. + - If valid, apply the fix in a new commit, push, and **resolve the comment thread**. - If not applicable, leave a reply explaining why. - After addressing comments, update the PR description if the changes affect the summary, code flow explanation, or per-file notes. -6. **Repeat monitoring** after each force-push: wait 1-2 minutes, check for new Copilot comments, and address them. Continue this loop until no new comments appear. +6. **Repeat monitoring** after each push: wait 1-2 minutes, check for new Copilot comments, and address them. Continue this loop until no new comments appear. 7. **Re-run tests** after addressing review comments to confirm nothing regressed.