fix how _selectVendorDefaultLanguageModel is set#287804
Conversation
There was a problem hiding this comment.
Pull request overview
This PR moves the setting of the _selectVendorDefaultLanguageModel static field from widget initialization to the run() method and adds proper cleanup. The change addresses when the listener is attached to track language model changes and ensures the listener is disposed when the session completes.
Changes:
- Moved
onDidChangeCurrentLanguageModellistener from widget initialization torun()method - Added
DisposableStorefor proper cleanup of the listener - Wrapped existing wait logic in try-finally block to ensure cleanup
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/inlineChat/browser/inlineChatController.ts:439
- This line has the same issue as Comment 1. For notebook contexts, it should check against the widget's actual location (which could be
ChatAgentLocation.Notebook), notsession.chatModel.initialLocation(which is alwaysChatAgentLocation.EditorInline).
if (!persistModelChoice && InlineChatController._selectVendorDefaultLanguageModel && model && !model.metadata.isDefaultForLocation[session.chatModel.initialLocation]) {
| store.add(this._zone.value.widget.chatWidget.input.onDidChangeCurrentLanguageModel(newModel => { | ||
| InlineChatController._selectVendorDefaultLanguageModel = Boolean(newModel.metadata.isDefaultForLocation[session.chatModel.initialLocation]); | ||
| })); |
There was a problem hiding this comment.
Using session.chatModel.initialLocation is incorrect for notebook contexts. The session is always created with ChatAgentLocation.EditorInline (see InlineChatSessionServiceImpl.createSession), but the widget's location can be ChatAgentLocation.Notebook for notebook editors. The old code correctly used location.location which could be either EditorInline or Notebook. This should either store the widget's location for use here, or access it differently to handle the notebook case correctly.
|
|
||
| const session = this._inlineChatSessionService.createSession(this._editor); | ||
|
|
||
| const store = new DisposableStore(); |
There was a problem hiding this comment.
The store will leak if an exception is thrown anywhere between lines 433-504 (before the try block starts). Move the store creation and the try block to encompass all code that could throw after line 432, or wrap all potentially throwing code in the try block.
|
@jrieken just fyi looks like Copilot has some thoughts |
No description provided.