Remove chatSessionTracker #279690
Remove chatSessionTracker #279690
Conversation
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @bpaseroMatched files:
|
There was a problem hiding this comment.
Pull request overview
This PR removes the ChatSessionTracker class and refactors session change tracking to be centralized in ChatSessionsService. The tracker previously monitored editor groups and local chat sessions, emitting change events. This functionality is now handled by registering model progress listeners directly in the ChatSessionsService constructor via an autorun that watches chat models.
Key changes:
- Centralized model progress listener registration in
ChatSessionsServiceusing autorun - Removed
ChatSessionTrackerclass and its usage throughout the codebase - Simplified
LocalAgentsSessionsProviderby removing local event listening logic - Updated
SessionsDataSourceto no longer integrate hybrid sessions
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/browser/chatSessions/chatSessionTracker.ts | Deleted the entire ChatSessionTracker class |
| src/vs/workbench/contrib/chat/browser/chatSessions.contribution.ts | Added autorun to register model progress listeners; refactored registerModelProgressListener to accept models iterable instead of individual model |
| src/vs/workbench/contrib/chat/browser/chatSessions/view/chatSessionsView.ts | Removed ChatSessionTracker instantiation and usage |
| src/vs/workbench/contrib/chat/browser/chatSessions/view/sessionsViewPane.ts | Removed ChatSessionTracker parameter and LocalAgentsSessionsProvider-specific event listener |
| src/vs/workbench/contrib/chat/browser/chatSessions/view/sessionsTreeRenderer.ts | Removed ChatSessionTracker parameter and hybrid session integration logic |
| src/vs/workbench/contrib/chat/browser/agentSessions/localAgentSessionsProvider.ts | Removed local model listener registration logic |
| src/vs/workbench/contrib/chat/common/chatSessionsService.ts | Updated interface signature for registerModelProgressListener |
| src/vs/workbench/contrib/chat/test/common/mockChatSessionsService.ts | Updated mock implementation signature |
| src/vs/workbench/contrib/chat/test/browser/localAgentSessionsProvider.test.ts | Removed tests for the deleted event handling functionality |
src/vs/workbench/contrib/chat/browser/chatSessions/view/sessionsTreeRenderer.ts
Show resolved
Hide resolved
| readonly _onDidChangeChatSessionItems = this._register(new Emitter<void>()); | ||
| readonly onDidChangeChatSessionItems = this._onDidChangeChatSessionItems.event; |
There was a problem hiding this comment.
Bug: The _onDidChangeChatSessionItems event is never emitted. Previously, listeners were set up to fire this event when models changed, but that code was removed (lines that called registerListeners() and registerModelListeners()). Without emitting this event, the UI won't be notified when session items change, and the sessions view won't update.
The provider should listen to relevant changes and fire the event. Consider:
- Listening to
chatService.chatModelschanges and firing the event - Listening to
chatSessionsService.onDidChangeSessionItemsfor the local session type and forwarding the event
src/vs/workbench/contrib/chat/test/common/mockChatSessionsService.ts
Outdated
Show resolved
Hide resolved
|
Copilot seems to have good points, and CI is red... |
| const chatService = this._chatServiceLazy.value; | ||
| this._register(autorun(reader => { | ||
| const models = chatService.chatModels.read(reader); | ||
| this.registerModelProgressListener(models); |
There was a problem hiding this comment.
I don't have the big picture, but it seems like a step backwards to do this in this service. I think _chatServiceLazy is trying to avoid a recursive dependency? Actually I don't really see how it helps since you still have to get a IChatService instance in the constructor of this service. And then if the end result of this is just to inform the list that the local sessions have changed, why shouldn't the local sessions provider do that?
There was a problem hiding this comment.
I was debugging this because I wasn't sure how it worked, and I think it works because our services are created lazily, and it would fail if the IChatSessionsService was referenced in the IChatService constructor
There was a problem hiding this comment.
This is because we need to do the same for all providers and not just the local chat sessions one. We need to detect progress in the chat model response and indicate the UI update so that we can show the latest progress status.
Do you have other suggestions of where to put the progress listeners if we wanted to do it for across all providers?
There was a problem hiding this comment.
Oh I see. Maybe each provider should just be responsible for reporting on its sessions? Even if the underlying data source is the chat service, it can filter the available models to the ones that it is in charge of. Isn't it odd now that the local sessions provider has a onDidChangeChatSessionItems and doesn't fire it?
There was a problem hiding this comment.
I guess I don't quite understand how this works for non-local providers, what is the extension's responsibility and what is core's?
There was a problem hiding this comment.
Yeah I think the provider having onDidChangeChatSessionItems and not firing is strange, the other providers do fire them in other cases but the local chat session provider is very bare bones so it doesn't really need to update for anything beyond tracking progress.
The extensions used to have the full responsability for the rendering but we want to start migrating things off of that pattern to create some rendering standard, for now status, description and icons fall into that category.
Not sure what the best pattern is for cases like this where core owns part of the UI and the extension another part of it, I guess we need to restrict the API accordingly but how do we manage the updates based on the chat model progress in this case?
There was a problem hiding this comment.
I don't quite understand and don't have full context on the API, happy to dig into it with you when we're back. But in general, I think we need a well-defined extension API, and a well-defined internal API for the view to be built on.
src/vs/workbench/contrib/chat/browser/chatSessions.contribution.ts
Outdated
Show resolved
Hide resolved
| const chatService = this._chatServiceLazy.value; | ||
| this._register(autorun(reader => { | ||
| const models = chatService.chatModels.read(reader); | ||
| this.registerModelProgressListener(models); |
There was a problem hiding this comment.
I was debugging this because I wasn't sure how it worked, and I think it works because our services are created lazily, and it would fail if the IChatSessionsService was referenced in the IChatService constructor
Co-authored-by: Copilot <[email protected]>
| const chatService = this._chatServiceLazy.value; | ||
| this._register(autorun(reader => { | ||
| const models = chatService.chatModels.read(reader); | ||
| this.registerModelProgressListener(models); |
There was a problem hiding this comment.
Oh I see. Maybe each provider should just be responsible for reporting on its sessions? Even if the underlying data source is the chat service, it can filter the available models to the ones that it is in charge of. Isn't it odd now that the local sessions provider has a onDidChangeChatSessionItems and doesn't fire it?
| this._register(request.response.onDidChange(() => { | ||
| callback(); | ||
| })); | ||
| this._register(request.response.onDidChange(() => { |
There was a problem hiding this comment.
registerModelProgressListener runs every time the set of models changes, but then any disposables using this._register in here will be retained for the lifetime of the service (so forever), I believe that would cause us to run these listeners multiple times per change and leak memory when chatmodels are disposed.
No description provided.