Skip to content

Remove chatSessionTracker #279690

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+75 −430
Open

Remove chatSessionTracker #279690

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1b1806c
Remove chatSessionTracker
osortega Nov 27, 2025
0451d1f
Dispose fix
osortega Nov 27, 2025
a33e3fc
Type fix
osortega Nov 27, 2025
631470d
Apply suggestion from @Copilot
osortega Nov 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 1 addition & 64 deletions src/vs/workbench/contrib/chat/browser/agentSessions/localAgentSessionsProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,8 @@ import { coalesce } from '../../../../../base/common/arrays.js';
import { CancellationToken } from '../../../../../base/common/cancellation.js';
import { Codicon } from '../../../../../base/common/codicons.js';
import { Emitter } from '../../../../../base/common/event.js';
import { Disposable, DisposableMap, DisposableStore, IDisposable } from '../../../../../base/common/lifecycle.js';
import { Disposable } from '../../../../../base/common/lifecycle.js';
import { ResourceSet } from '../../../../../base/common/map.js';
import { autorun } from '../../../../../base/common/observable.js';
import { truncate } from '../../../../../base/common/strings.js';
import { IWorkbenchContribution } from '../../../../common/contributions.js';
import { ModifiedFileEntryState } from '../../common/chatEditingService.js';
Expand All @@ -23,78 +22,16 @@ export class LocalAgentsSessionsProvider extends Disposable implements IChatSess
static readonly ID = 'workbench.contrib.localAgentsSessionsProvider';

readonly chatSessionType = localChatSessionType;

private readonly _onDidChange = this._register(new Emitter<void>());
readonly onDidChange = this._onDidChange.event;

readonly _onDidChangeChatSessionItems = this._register(new Emitter<void>());
readonly onDidChangeChatSessionItems = this._onDidChangeChatSessionItems.event;
Comment on lines 25 to 26
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

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:

  1. Listening to chatService.chatModels changes and firing the event
  2. Listening to chatSessionsService.onDidChangeSessionItems for the local session type and forwarding the event

Copilot uses AI. Check for mistakes.

private readonly modelListeners = this._register(new DisposableMap<string>());

constructor(
@IChatService private readonly chatService: IChatService,
@IChatSessionsService private readonly chatSessionsService: IChatSessionsService,
) {
super();

this._register(this.chatSessionsService.registerChatSessionItemProvider(this));

this.registerListeners();
}

private registerListeners(): void {

// Listen for models being added or removed
this._register(autorun(reader => {
const models = this.chatService.chatModels.read(reader);
this.registerModelListeners(models);
}));

// Listen for global session items changes for our session type
this._register(this.chatSessionsService.onDidChangeSessionItems(sessionType => {
if (sessionType === this.chatSessionType) {
this._onDidChange.fire();
}
}));
}

private registerModelListeners(models: Iterable<IChatModel>): void {
const seenKeys = new Set<string>();

for (const model of models) {
const key = model.sessionResource.toString();
seenKeys.add(key);

if (!this.modelListeners.has(key)) {
this.modelListeners.set(key, this.registerSingleModelListeners(model));
}
}

// Clean up listeners for models that no longer exist
for (const key of this.modelListeners.keys()) {
if (!seenKeys.has(key)) {
this.modelListeners.deleteAndDispose(key);
}
}

this._onDidChange.fire();
}

private registerSingleModelListeners(model: IChatModel): IDisposable {
const store = new DisposableStore();

this.chatSessionsService.registerModelProgressListener(model, () => {
this._onDidChangeChatSessionItems.fire();
});

store.add(model.onDidChange(e => {
if (!e || e.kind === 'setCustomTitle') {
this._onDidChange.fire();
}
}));

return store;
}

private modelToStatus(model: IChatModel): ChatSessionStatus | undefined {
Expand Down
115 changes: 65 additions & 50 deletions src/vs/workbench/contrib/chat/browser/chatSessions.contribution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { Action2, IMenuService, MenuId, MenuItemAction, MenuRegistry, registerAc
import { ContextKeyExpr, IContextKeyService } from '../../../../platform/contextkey/common/contextkey.js';
import { IRelaxedExtensionDescription } from '../../../../platform/extensions/common/extensions.js';
import { InstantiationType, registerSingleton } from '../../../../platform/instantiation/common/extensions.js';
import { ServicesAccessor } from '../../../../platform/instantiation/common/instantiation.js';
import { IInstantiationService, ServicesAccessor } from '../../../../platform/instantiation/common/instantiation.js';
import { ILabelService } from '../../../../platform/label/common/label.js';
import { ILogService } from '../../../../platform/log/common/log.js';
import { isDark } from '../../../../platform/theme/common/theme.js';
Expand All @@ -39,8 +39,9 @@ import { IChatEditorOptions } from './chatEditor.js';
import { NEW_CHAT_SESSION_ACTION_ID } from './chatSessions/common.js';
import { IChatModel, IChatProgressResponseContent, IChatRequestModel } from '../common/chatModel.js';
import { IChatService, IChatToolInvocation } from '../common/chatService.js';
import { autorunSelfDisposable } from '../../../../base/common/observable.js';
import { autorun, autorunSelfDisposable } from '../../../../base/common/observable.js';
import { IChatRequestVariableEntry } from '../common/chatVariableEntries.js';
import { Lazy } from '../../../../base/common/lazy.js';

const extensionPoint = ExtensionsRegistry.registerExtensionPoint<IChatSessionsExtensionPoint[]>({
extensionPoint: 'chatSessions',
Expand Down Expand Up @@ -273,15 +274,18 @@ export class ChatSessionsService extends Disposable implements IChatSessionsServ
private readonly _editableSessions = new ResourceMap<IEditableData>();
private readonly _registeredRequestIds = new Set<string>();
private readonly _registeredModels = new Set<IChatModel>();

private readonly _chatServiceLazy = new Lazy(() =>
this._instantiationService.invokeFunction(a => a.get(IChatService))
);
constructor(
@ILogService private readonly _logService: ILogService,
@IChatAgentService private readonly _chatAgentService: IChatAgentService,
@IExtensionService private readonly _extensionService: IExtensionService,
@IContextKeyService private readonly _contextKeyService: IContextKeyService,
@IMenuService private readonly _menuService: IMenuService,
@IThemeService private readonly _themeService: IThemeService,
@ILabelService private readonly _labelService: ILabelService
@ILabelService private readonly _labelService: ILabelService,
@IInstantiationService private readonly _instantiationService: IInstantiationService,
) {
super();

Expand Down Expand Up @@ -318,6 +322,12 @@ export class ChatSessionsService extends Disposable implements IChatSessionsServ
stripPathStartingSeparator: true,
}
}));

const chatService = this._chatServiceLazy.value;
this._register(autorun(reader => {
const models = chatService.chatModels.read(reader);
this.registerModelProgressListener(models);
Copy link
Member

@roblourens roblourens Nov 27, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Member

@roblourens roblourens Nov 27, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@osortega osortega Nov 27, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Member

@roblourens roblourens Nov 27, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Member

@roblourens roblourens Nov 27, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@osortega osortega Nov 27, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Member

@roblourens roblourens Nov 27, 2025

Choose a reason for hiding this comment

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

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.

}));
}

public reportInProgress(chatSessionType: string, count: number): void {
Expand Down Expand Up @@ -794,58 +804,63 @@ export class ChatSessionsService extends Disposable implements IChatSessionsServ
};
}

public registerModelProgressListener(model: IChatModel, callback: () => void): void {
// Prevent duplicate registrations for the same model
if (this._registeredModels.has(model)) {
return;
}
this._registeredModels.add(model);

// Helper function to register listeners for a request
const registerRequestListeners = (request: IChatRequestModel) => {
if (!request.response || this._registeredRequestIds.has(request.id)) {
return;
public registerModelProgressListener(models: Iterable<IChatModel>): void {
const disposables = new DisposableStore();
for (const model of models) {
// Prevent duplicate registrations for the same model
if (this._registeredModels.has(model)) {
continue;
}
this._registeredModels.add(model);

// Get the session type from the model's sessionResource scheme
// Helper function to register listeners for a request
const registerRequestListeners = (request: IChatRequestModel) => {
const chatSessionType = model.sessionResource.scheme === Schemas.vscodeLocalChatSession ? 'local' : model.sessionResource.scheme;
if (!request.response || this._registeredRequestIds.has(request.id)) {
return;
}

this._registeredRequestIds.add(request.id);
this._registeredRequestIds.add(request.id);

this._register(request.response.onDidChange(() => {
callback();
}));
this._register(request.response.onDidChange(() => {
Copy link
Member

@roblourens roblourens Nov 27, 2025

Choose a reason for hiding this comment

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

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.

this.notifySessionItemsChanged(chatSessionType);
}));

// Track tool invocation state changes
const responseParts = request.response.response.value;
responseParts.forEach((part: IChatProgressResponseContent) => {
if (part.kind === 'toolInvocation') {
const toolInvocation = part as IChatToolInvocation;
// Use autorun to listen for state changes
this._register(autorunSelfDisposable(reader => {
const state = toolInvocation.state.read(reader);

// Also track progress changes when executing
if (state.type === IChatToolInvocation.StateKind.Executing) {
state.progress.read(reader);
}
// Track tool invocation state changes
const responseParts = request.response.response.value;
responseParts.forEach((part: IChatProgressResponseContent) => {
if (part.kind === 'toolInvocation') {
const toolInvocation = part as IChatToolInvocation;
// Use autorun to listen for state changes
disposables.add(autorunSelfDisposable(reader => {
const state = toolInvocation.state.read(reader);

callback();
}));
}
});
};
// Listen for response changes on all existing requests
const requests = model.getRequests();
requests.forEach(registerRequestListeners);

// Listen for new requests being added
this._register(model.onDidChange(() => {
const currentRequests = model.getRequests();
currentRequests.forEach(registerRequestListeners);
}));
// Also track progress changes when executing
if (state.type === IChatToolInvocation.StateKind.Executing) {
state.progress.read(reader);
}

// Clean up when model is disposed
this._register(model.onDidDispose(() => {
this._registeredModels.delete(model);
}));
this.notifySessionItemsChanged(chatSessionType);
}));
}
});
};
// Listen for response changes on all existing requests
const requests = model.getRequests();
requests.forEach(registerRequestListeners);

// Listen for new requests being added
disposables.add(model.onDidChange(() => {
const currentRequests = model.getRequests();
currentRequests.forEach(registerRequestListeners);
}));

// Clean up when model is disposed
disposables.add(model.onDidDispose(() => {
this._registeredModels.delete(model);
}));
}
}

public getSessionDescription(chatModel: IChatModel): string | undefined {
Expand Down
Loading
Loading