Skip to content

Conversation

@roblourens
Copy link
Member

Fixes #290328

Problem

The factory methods in CollapsibleListPool and TreePool were registering disposables (like ResourceLabels and tree container scopes) to the pool class via this._register() instead of associating them with the pool item. This meant they would only be disposed when the entire pool was disposed, not when individual items were cleared or released.

Solution

This copies the correct strategy used by CollapsibleChangesSummaryListPool:

  • Create wrapper types (ICollapsibleListWrapper, ITreePoolWrapper) that implement IDisposable
  • Use a local DisposableStore in the factory method to track the item's disposables
  • Return a wrapper object with its own dispose() method that cleans up the store
  • Update get() to unwrap and return the inner list/tree while properly releasing the wrapper

…ol class

Fixes #290328

The factory methods in CollapsibleListPool and TreePool were registering
disposables (like ResourceLabels and tree container scopes) to the pool
class via this._register() instead of associating them with the pool
item. This meant they would only be disposed when the entire pool was
disposed, not when individual items were cleared.

This copies the correct strategy used by CollapsibleChangesSummaryListPool:
- Create wrapper types (ICollapsibleListWrapper, ITreePoolWrapper)
- Use a local DisposableStore in the factory method
- Return a wrapper with its own dispose() method that cleans up the store
Copilot AI review requested due to automatic review settings January 26, 2026 19:07
@vs-code-engineering vs-code-engineering bot added this to the January 2026 milestone Jan 26, 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

This PR fixes incorrect disposable ownership in chat UI ResourcePool factory methods so per-item disposables are tied to the pooled item (and can be cleaned up on pool clear()), rather than being registered on the pool class itself.

Changes:

  • Introduces disposable wrapper types for pooled trees/lists and tracks per-item disposables in a local DisposableStore.
  • Updates TreePool and CollapsibleListPool to pool wrappers while returning the inner tree/list via get().
  • Ensures pool release() operates on wrappers rather than the inner object.

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/chatContentParts/chatTreeContentPart.ts Pools a disposable wrapper around the chat progress file tree to associate per-tree disposables with the pooled item.
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/chatReferencesContentPart.ts Pools a disposable wrapper around the collapsible references list to associate per-list disposables with the pooled item.

@roblourens roblourens enabled auto-merge (squash) January 26, 2026 19:32
@roblourens roblourens merged commit 3fc6237 into main Jan 26, 2026
28 checks passed
@roblourens roblourens deleted the roblou/fix-resource-pool-disposal-290328 branch January 26, 2026 19:35
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.

Bad use of ResourcePool?

2 participants