Fix listener leak when rerendering title file widgets in chat content parts#314005
Fix listener leak when rerendering title file widgets in chat content parts#314005ishaq2321 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a listener/memory leak in chat content parts where title re-renders could leave previously-registered file link widgets (and their anchor-service registrations) undisposed, accumulating over time.
Changes:
- Track title file-link widgets in a dedicated
DisposableStoreandclear()it before re-rendering inChatCollapsibleInputOutputContentPart. - Apply the same tracking/clearing approach for subagent title detail rendering in
ChatSubagentContentPart. - Add a regression unit test that asserts previous title widget registrations are disposed when the title updates.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/test/browser/widget/chatContentParts/chatToolInputOutputContentPart.test.ts | Adds regression coverage asserting prior title file-widget registrations are disposed on title updates. |
| src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatToolInputOutputContentPart.ts | Introduces _titleFileWidgetStore and re-renders title widgets on title changes to prevent leaks. |
| src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatSubagentContentPart.ts | Tracks title-detail file widgets in _titleFileWidgetStore and clears it before re-rendering. |
| group.style.display = expanded.read(r) ? 'none' : ''; | ||
| })); |
| @@ -526,7 +528,7 @@ export class ChatSubagentContentPart extends ChatCollapsibleContentPart implemen | |||
| } else { | |||
| const result = this.chatContentMarkdownRenderer.render(new MarkdownString(toolCallText)); | |||
| result.element.classList.add('collapsible-title-content', 'chat-thinking-title-detail'); | |||
| renderFileWidgets(result.element, this.instantiationService, this.chatMarkdownAnchorService, this._store); | |||
| renderFileWidgets(result.element, this.instantiationService, this.chatMarkdownAnchorService, this._titleFileWidgetStore); | |||
| this._titleDetailRendered.value = result; | |||
… parts When a chat content part's title changes, renderFileWidgets() is called to render file link widgets. Previously, widgets registered during the previous render were not cleaned up before adding new ones, causing a listener leak. This change: - Introduces a dedicated _titleFileWidgetStore in ChatCollapsibleInputOutputContentPart that is cleared and repopulated when the title is updated - Applies the same fix to ChatSubagentContentPart and ChatThinkingContentPart which had the same issue - Adds a regression test to ensure widgets are properly disposed on title update Fixes: listener leak in chat tool input/output content parts Fixes: listener leak in chat subagent content parts Fixes: listener leak in chat thinking content parts
baf2bbb to
018cd57
Compare
|
@ishaq2321 Thank you so much for this PR and for identifying the problem! |
|
@dmitrivMS Thanks for the clean implementation and for carrying it across the finish line — glad the fix made it in! I've been exploring the VS Code codebase and would love to contribute more regularly. If there are open issues you think need attention, or areas where a fresh set of eyes would help, feel free to tag me or point me in the right direction. Happy to dig in. |
|
@ishaq2321 Honestly, there's been OOM issues when using chat/agents, so anything you can contribute to cleaning that up would be appreciated. |
|
@dmitrivMS OOM issues in chat/agents sounds like exactly the kind of thing I enjoy digging into — memory profiling, listener cleanup, widget lifecycle edge cases. Happy to take a look. If it's easier, feel free to assign me specific issues directly — that way I can focus without duplicating work others might already be on. I'm also familiar with the Microsoft TypeScript compiler repository, so if anything crosses over there, I can help on that side too. One thing that helps me contribute effectively: early feedback on PRs — even a quick "looks like the right direction" or "rethink this part" — so I can iterate fast rather than going quiet for a week and losing context. Happy to work however suits the team best. |
|
@ishaq2321 We don't really do that (assign issues), but here's the query: https://github.com/microsoft/vscode/issues?q=is%3Aissue%20state%3Aopen%20(label%3Aperf-startup%20OR%20label%3Aperf-bloat%20OR%20label%3Aperf-profile%20OR%20label%3Aperf)&page=2 |
Summary
When a chat content part's title changes and contains file links,
renderFileWidgets()is called to render file link widgets. Previously, widgets registered during the previous render were not cleaned up before adding new ones, causing a listener leak that accumulated over time.Changes
ChatCollapsibleInputOutputContentPart: Added
_titleFileWidgetStore(aDisposableStore) that is cleared and repopulated when the title setter is called, ensuring previous widgets are properly disposed.ChatSubagentContentPart: Applied the same fix - added
_titleFileWidgetStoreto track title widgets and clear them before re-rendering.Test: Added regression test that verifies the fix by counting
registeranddisposecalls on the markdown anchor service.Testing
ChatCollapsibleInputOutputContentPartwith a title containing a file link, updates the title, and verifies that the previous widget'sdispose()was called when the new title is set.Related Issues