Skip to content

fix disposable leak + fix empty thinking container#302595

Merged
justschen merged 1 commit intomainfrom
justin/haxorus
Mar 17, 2026
Merged

fix disposable leak + fix empty thinking container#302595
justschen merged 1 commit intomainfrom
justin/haxorus

Conversation

@justschen
Copy link
Collaborator

fix #302066

Copilot AI review requested due to automatic review settings March 17, 2026 22:55
@justschen justschen enabled auto-merge (squash) March 17, 2026 22:55
@vs-code-engineering vs-code-engineering bot added this to the 1.113.0 milestone Mar 17, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a disposable leak in the chat tool-invocation rendering flow (notably when tool parts are created lazily inside a thinking container) and removes/disposes thinking containers that become empty after a tool is moved out (e.g. for confirmation), addressing #302066.

Changes:

  • Extend the lazy tool-part factory return shape to include a disposable so lazy-created tool parts can be lifecycle-managed.
  • Add ChatThinkingContentPart.isEffectivelyEmpty() to detect thinking containers with no meaningful content.
  • When a tool is moved out of thinking during confirmation transition, remove/dispose the thinking container if it becomes empty.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/vs/workbench/contrib/chat/browser/widget/chatListRenderer.ts Updates lazy tool-part factory return type and adds logic to remove/dispose empty thinking containers during confirmation transitions.
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatThinkingContentPart.ts Adds isEffectivelyEmpty() helper to determine when a thinking container has no displayable content.
Comments suppressed due to low confidence (1)

src/vs/workbench/contrib/chat/browser/widget/chatListRenderer.ts:2055

  • In the branch where the tool part wasn’t previously created, this code path creates a new ChatToolInvocationPart via createToolPart() and appends its domNode, but does not register the returned disposable anywhere. Since the renderer is currently holding a no-op placeholder part at this content index, this tool part may never be disposed, reintroducing the leaked-disposable issue. Suggestion: either (a) avoid creating/appending the tool part here and let the normal render/diff cycle create + track it, or (b) capture disposable and add it to an appropriate disposable store (and ensure future renders don’t create a duplicate part).
				templateData.value.appendChild(domNode);
			}

You can also share your feedback on Copilot code review. Take the survey.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LEAKED DISPOSABLE: createToolPart

3 participants