Skip to content

Use DisposableMap for session listener lifecycle in FocusSessionActionViewItem#293442

Draft
Copilot wants to merge 10 commits intomainfrom
copilot/sub-pr-293155
Draft

Use DisposableMap for session listener lifecycle in FocusSessionActionViewItem#293442
Copilot wants to merge 10 commits intomainfrom
copilot/sub-pr-293155

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Feb 6, 2026

Addresses review feedback from #293155 to use DisposableMap for tracking debug session listeners, preventing leaks when the component is disposed while sessions are still active.

Changes

Replaced array-based listener tracking with DisposableMap pattern:

  • Session listeners now tracked in DisposableMap<string, DisposableStore> registered with component lifecycle
  • Listeners properly disposed on session end and component disposal
  • Follows pattern from loadedScriptsView.ts:545-591

Simplified disposal logic:

  • Removed redundant onDidEndAdapter cleanup (sessions always fire onDidEndSession)
  • Added defensive cleanup in registerSessionListeners to handle potential duplicate calls

Merged main branch:

  • Merged latest changes from main branch
  • Resolved conflict by keeping the improved DisposableMap implementation over the old array-based pattern
// Before: listeners leak when component disposes with active sessions
const sessionListeners: IDisposable[] = [];
sessionListeners.push(session.onDidChangeName(() => this.update()));
sessionListeners.push(session.onDidEndAdapter(() => dispose(sessionListeners)));

// After: proper lifecycle management
const sessionListeners = this._register(new DisposableMap<string, DisposableStore>());
const store = new DisposableStore();
sessionListeners.set(session.getId(), store);
store.add(session.onDidChangeName(() => this.update()));
// Auto-disposed when component or session ends

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

jrieken and others added 4 commits February 5, 2026 17:27
Fixes memory leaks caused by using this._register() inside callbacks
that fire multiple times. Each call added entries to the disposable
store that were never cleaned up.

Fixes:
- problemCollectors: register inside onModelRemoved handler
- loadedScriptsView: session listeners without per-session cleanup
- debugActionViewItems: existing sessions not tracked for cleanup
- terminalTaskSystem: register inside terminate() method
- textAreaEditContextInput: register inside runAndSubscribe callback
- terminal.history.contribution: register inside onDidAddCapability
- terminalInstance: nested register in capability handler
- storageMainService: per-profile listeners accumulating
…sables

# Conflicts:
#	src/vs/workbench/contrib/tasks/browser/terminalTaskSystem.ts
Copilot AI and others added 5 commits February 6, 2026 15:08
Co-authored-by: jrieken <1794099+jrieken@users.noreply.github.com>
Co-authored-by: jrieken <1794099+jrieken@users.noreply.github.com>
Co-authored-by: jrieken <1794099+jrieken@users.noreply.github.com>
Co-authored-by: jrieken <1794099+jrieken@users.noreply.github.com>
Co-authored-by: jrieken <1794099+jrieken@users.noreply.github.com>
Copilot AI changed the title [WIP] Address feedback to use disposable map in leak fix implementation Use DisposableMap for session listener lifecycle in FocusSessionActionViewItem Feb 6, 2026
Copilot AI requested a review from jrieken February 6, 2026 15:12
Base automatically changed from joh/fix-leaking-disposables to main February 6, 2026 16:06
@jrieken
Copy link
Copy Markdown
Member

jrieken commented Feb 9, 2026

@copilot merge main and fix conflicts

Co-authored-by: jrieken <1794099+jrieken@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Feb 9, 2026

@copilot merge main and fix conflicts

Merged main in 4d70e4c. The conflict in debugActionViewItems.ts was resolved by keeping our DisposableMap implementation (our changes replaced the old array-based pattern that was present in main).

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.

2 participants