Rename fix for disposed models #286470
Rename fix for disposed models #286470
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where attempting to set a custom title on a disposed chat model would fail. The fix ensures that if a model is not currently loaded in memory, it will be restored from storage, the title will be set, and then the temporary reference will be disposed.
Key Changes:
- Added
setCustomTitlemethod to theIChatModelinterface - Updated
MockChatModelto implement the new interface method - Modified
setChatSessionTitleinChatServiceImplto restore disposed models when needed
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/common/model/chatModel.ts | Adds setCustomTitle method to the IChatModel interface |
| src/vs/workbench/contrib/chat/test/common/model/mockChatModel.ts | Implements setCustomTitle in mock for testing |
| src/vs/workbench/contrib/chat/common/chatService/chatServiceImpl.ts | Updates setChatSessionTitle to handle disposed models by restoring them temporarily |
| let modelRef: IChatModelReference | undefined; | ||
| try { | ||
| modelRef = await this.getOrRestoreSession(sessionResource); | ||
| modelRef?.object.setCustomTitle(title); |
There was a problem hiding this comment.
The else branch properly handles the case where the model is not currently in memory by restoring it, setting the title, and disposing the reference. However, if getOrRestoreSession returns undefined (line 512 shows this can happen when sessionData is not found), the code silently continues without any logging or error indication. Consider adding a log statement to trace when the session cannot be restored, which would help with debugging issues related to setting titles on non-existent sessions.
| modelRef?.object.setCustomTitle(title); | |
| if (modelRef) { | |
| modelRef.object.setCustomTitle(title); | |
| } else { | |
| this.trace('setChatSessionTitle', `Could not restore session ${sessionResource.toString()} to set title`); | |
| } |
|
cc @roblourens |
| } else { | ||
| let modelRef: IChatModelReference | undefined; | ||
| try { | ||
| modelRef = await this.getOrRestoreSession(sessionResource); |
There was a problem hiding this comment.
TBH doing this seems a bit silly to me, just restore the chat model to update the title and dispose it, futher down then updating the store.
@roblourens when reviving a chat model, do we take the title from the store? If so maybe we just need to fire onDidChange event without the need of creating the modelRef
There was a problem hiding this comment.
I think the title is already in the index to avoid having to do this. I think the problem is just that when loading a model, we should take the title from the index and let it overwrite the one in the stored model.
There was a problem hiding this comment.
And I believe the block below this (// Update the title in the file storage) can be in the else from if (model)
Fixes: #283514