Remove ChatSessionService.notifySessionOptionsChange#303060
Remove ChatSessionService.notifySessionOptionsChange#303060mjbvz merged 1 commit intomicrosoft:mainfrom
ChatSessionService.notifySessionOptionsChange#303060Conversation
- Use single event for options change - We should not expose notify type methods on services because callers should not control event flows directly like this - Remove use of async event. Afaik this was not actually being used for anything
There was a problem hiding this comment.
Pull request overview
This PR removes ChatSessionService.notifySessionOptionsChange and consolidates session-option change propagation around a single onDidChangeSessionOptions event + a new updateSessionOptions(...) API, aiming to reduce event debt and make option-change firing conditional on real changes.
Changes:
- Replaced the async “notify extension then update” flow with synchronous
setSessionOption/updateSessionOptionsthat updates state and fires a single change event. - Changed
onDidChangeSessionOptionspayload fromURIto a structured{ sessionResource, updates }event. - Updated main thread forwarding to listen to
onDidChangeSessionOptionsinstead of a dedicated “notify extension” async event.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/test/common/mockChatSessionsService.ts | Updates mock to new option-change event shape and removes notify API usage. |
| src/vs/workbench/contrib/chat/common/chatSessionsService.ts | Adjusts IChatSessionsService contract: new event payload + adds updateSessionOptions. |
| src/vs/workbench/contrib/chat/browser/widget/input/chatInputPart.ts | Switches option updates from notifySessionOptionsChange to updateSessionOptions / setSessionOption. |
| src/vs/workbench/contrib/chat/browser/chatSessions/chatSessions.contribution.ts | Implements updateSessionOptions, removes async notify event/method, and updates event firing semantics. |
| src/vs/workbench/api/test/browser/mainThreadChatSessions.test.ts | Updates tests to use setSessionOption/updateSessionOptions. |
| src/vs/workbench/api/browser/mainThreadChatSessions.ts | Forwards option changes via onDidChangeSessionOptions and removes waitUntil usage. |
| src/vs/sessions/contrib/chat/browser/newSession.ts | Replaces notify calls with setSessionOption for Sessions window new-session option selection. |
Comments suppressed due to low confidence (2)
src/vs/workbench/contrib/chat/browser/chatSessions/chatSessions.contribution.ts:1148
- Even when only some entries in
updatesactually change, the event payload currently re-emits the fullupdatesarray. This means listeners/extensions may receive “changes” for options that were no-ops, undermining the goal of only firing on real changes. Consider building achangedUpdatesarray (only the entries that differed) and firing with that instead.
let didChange = false;
for (const { optionId, value } of updates) {
const existingValue = session.getOption(optionId);
if (existingValue !== value) {
session.setOption(optionId, value);
didChange = true;
}
}
if (didChange) {
this._onDidChangeSessionOptions.fire({ sessionResource, updates: updates });
}
src/vs/sessions/contrib/chat/browser/newSession.ts:270
- Same issue as above for remote new sessions: option selections are made before a session exists, but
setSessionOptiononly works onceChatSessionsServicehas created session state. WithoutnotifySessionOptionsChange, these updates can be dropped. Consider deferring/batching these updates until after the session is created or having the service cache pending options per resource.
setOption(optionId: string, value: IChatSessionProviderOptionItem | string): void {
if (typeof value !== 'string') {
this.selectedOptions.set(optionId, value);
}
this._onDidChange.fire('options');
this._onDidChange.fire('disabled');
this.chatSessionsService.setSessionOption(this.resource, optionId, value);
}
You can also share your feedback on Copilot code review. Take the survey.
src/vs/workbench/contrib/chat/browser/chatSessions/chatSessions.contribution.ts
Show resolved
Hide resolved
src/vs/workbench/contrib/chat/browser/chatSessions/chatSessions.contribution.ts
Show resolved
Hide resolved
| * MainThreadChatSessions subscribes to this to forward changes to the extension host. | ||
| * Uses IWaitUntil pattern to allow listeners to register async work. | ||
| */ | ||
| readonly onRequestNotifyExtension: Event<IChatSessionOptionsWillNotifyExtensionEvent>; |
There was a problem hiding this comment.
Services should generally not know about the main thread or extensions
Instead the main thread should use apis exposed by the service
| * Uses IWaitUntil pattern to allow listeners to register async work. | ||
| */ | ||
| readonly onRequestNotifyExtension: Event<IChatSessionOptionsWillNotifyExtensionEvent>; | ||
| notifySessionOptionsChange(sessionResource: URI, updates: ReadonlyArray<{ optionId: string; value: string | IChatSessionProviderOptionItem }>): Promise<void>; |
There was a problem hiding this comment.
Notify type methods are generally a code smell because they let callers control event flows
It's better to hide event firing inside of service. Calls to notify methods also tend to get added unnecessary to workaround issues, which results in way too many events getting fired
There was a problem hiding this comment.
notify is also a lying name here because this method actually changes state instead of just notifying
| * Fired when options for a chat session change. | ||
| */ | ||
| readonly onDidChangeSessionOptions: Event<URI>; | ||
| readonly onDidChangeSessionOptions: Event<IChatSessionOptionsChangeEvent>; |
There was a problem hiding this comment.
Consolidating because we have two different events for session option changes. These were fired at different times
| this.chatSessionsService.notifySessionOptionsChange( | ||
| this.resource, | ||
| [{ optionId, value }] | ||
| ).catch((err) => this.logService.error(`Failed to notify session option ${optionId} change:`, err)); |
There was a problem hiding this comment.
This error handling logic was copy and pasted around. If we need this, it should done in a single central place instead
| * Extends IWaitUntil to allow listeners to register async work that will be awaited. | ||
| */ | ||
| export interface IChatSessionOptionsWillNotifyExtensionEvent extends IWaitUntil { | ||
| export interface IChatSessionOptionsChangeEvent { |
There was a problem hiding this comment.
An async event was used which adds complexity but it doesn't actually seem to needed
AsyncEvent should be used very sparingly. They should not be treated as replacement for normal async function calls.
If you're just posting something to the ext host and don't care about the response, don't need to await it
| } | ||
| }; | ||
|
|
||
| (proxy.$provideChatSessionContent as sinon.SinonStub).resolves(sessionContent); |
There was a problem hiding this comment.
I didn't fix it here but these casts are actually really bad because they are basically like any casts. It means resolve can be passed anything
Instead there's a Sinon type helper that is strongly typed that we should use
I'm trying to fix bugs in the chat session service and improve the extension api but am running into a lot of debt. Added comments noting the problems around
notifySessionOptionsChangeand why I want to remove it