Subscribe to dispose session in local agents provider #281450
Conversation
|
@osortega just fyi, any changes going forward that are for November also need to be manually backported into the |
Yeah I'm debating whether we want to include this or not, this seems a bit of an edge case any thoughts? |
There was a problem hiding this comment.
Pull request overview
This PR changes the IChatService.onDidDisposeSession event signature from accepting a single URI to accepting an array of URI[], enabling batch disposal notifications. The change adds a subscription to the dispose session event in LocalAgentsSessionsProvider to properly update UI state when sessions are disposed.
- Changed event signature from
{ sessionResource: URI; reason: 'cleared' }to{ sessionResource: URI[]; reason: 'cleared' } - Updated all event subscribers to iterate over the array of session resources
- Added new event subscription in
LocalAgentsSessionsProviderto handle session disposal events
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/common/chatService.ts | Updated interface signature for onDidDisposeSession to use URI[] |
| src/vs/workbench/contrib/chat/common/chatServiceImpl.ts | Updated emitter type and event firing to wrap single URI in array; added event fire in removeHistoryEntry |
| src/vs/workbench/contrib/chat/browser/agentSessions/localAgentSessionsProvider.ts | Added new subscription to onDidDisposeSession to fire change events when sessions are disposed |
| src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingServiceImpl.ts | Updated event handler to iterate over array of session resources |
| src/vs/workbench/api/browser/mainThreadChatAgents2.ts | Updated event handler to iterate over array and release each session |
| src/vs/workbench/contrib/terminalContrib/chat/browser/terminalChatService.ts | Updated event handler to iterate over array and clean up for matching sessions |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts | Updated event handler to iterate over array and clean up session terminals |
| src/vs/workbench/contrib/chat/test/common/mockChatService.ts | Updated mock event signature to match new interface |
| src/vs/workbench/contrib/chat/test/common/chatService.test.ts | Updated test to iterate over array when checking disposed sessions |
| src/vs/workbench/contrib/chat/test/browser/localAgentSessionsProvider.test.ts | Updated mock service to use array signature and provide helper method |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/electron-browser/runInTerminalTool.test.ts | Updated test emitter type and all test event firings to use arrays |
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/chat/common/chatServiceImpl.ts:451
- The
clearAllHistoryEntriesmethod should fire theonDidDisposeSessionevent for all cleared sessions, similar to howremoveHistoryEntrydoes (line 446). This is needed so that subscribers likeLocalAgentsSessionsProvider(which now listens to this event at line 58-63 inlocalAgentSessionsProvider.ts) can update their state when history is cleared. Consider collecting all session resources and firing the event with them in an array.
async clearAllHistoryEntries(): Promise<void> {
await this._chatSessionStore.clearAllSessions();
}
| this._register(this._chatService.onDidDisposeSession(e => { | ||
| if (LocalChatSessionUri.parseLocalSessionId(e.sessionResource) === terminalToolSessionId) { | ||
| this._terminalInstancesByToolSessionId.delete(terminalToolSessionId); | ||
| this._toolSessionIdByTerminalInstance.delete(instance); | ||
| this._terminalInstanceListenersByToolSessionId.deleteAndDispose(terminalToolSessionId); | ||
| // Clean up session auto approval state | ||
| const sessionId = LocalChatSessionUri.parseLocalSessionId(e.sessionResource); | ||
| if (sessionId) { | ||
| this._sessionAutoApprovalEnabled.delete(sessionId); | ||
| for (const resource of e.sessionResource) { | ||
| if (LocalChatSessionUri.parseLocalSessionId(resource) === terminalToolSessionId) { | ||
| this._terminalInstancesByToolSessionId.delete(terminalToolSessionId); | ||
| this._toolSessionIdByTerminalInstance.delete(instance); | ||
| this._terminalInstanceListenersByToolSessionId.deleteAndDispose(terminalToolSessionId); | ||
| // Clean up session auto approval state | ||
| const sessionId = LocalChatSessionUri.parseLocalSessionId(resource); | ||
| if (sessionId) { | ||
| this._sessionAutoApprovalEnabled.delete(sessionId); | ||
| } | ||
| this._persistToStorage(); | ||
| this._updateHasToolTerminalContextKeys(); | ||
| } | ||
| this._persistToStorage(); | ||
| this._updateHasToolTerminalContextKeys(); | ||
| } | ||
| })); |
There was a problem hiding this comment.
This event handler registration should be moved outside of registerTerminalInstanceWithToolSession to avoid creating duplicate listeners. Each time this method is called, a new listener is registered on the service (via this._register), but they all remain active for the service's lifetime. This causes:
- Memory leak - handlers accumulate and are never disposed individually
- Duplicate cleanup - all handlers fire for every session disposal, causing redundant cleanup operations
The listener should be registered once in the constructor and check all relevant sessions/instances inside the handler. Alternatively, register the listener per-session and store it in _terminalInstanceListenersByToolSessionId so it can be properly disposed.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Fixes: #264521
Fixes: #266407