Rename fix for disposed models

[osortega]

Fixes: #283514

[Copilot]

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 setCustomTitle method to the IChatModel interface
• Updated MockChatModel to implement the new interface method
• Modified setChatSessionTitle in ChatServiceImpl to 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

[Copilot]

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.

Suggested change

	modelRef?.object.setCustomTitle(title);
	if (modelRef) {
	modelRef.object.setCustomTitle(title);
	} else {
	this.trace('setChatSessionTitle', `Could not restore session ${sessionResource.toString()} to set title`);
	}

[bpasero]

cc @roblourens

[osortega]

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

[roblourens]

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.

[roblourens]

And I believe the block below this (// Update the title in the file storage) can be in the else from if (model)

[bpasero]

Whats the state of this PR?

[vs-code-engineering]

📬 CODENOTIFY

The following users are being notified based on files changed in this PR:

@bpasero

Matched files:

• src/vs/workbench/contrib/chat/browser/agentSessions/localAgentSessionsProvider.ts

[osortega]

@bpasero @roblourens not sure what the right thing to do here is TBH.
The problem is that the chat model is not there anymore by the time we want to rename the chat, so we just update the store, and at that point we need to tell the UI to refresh the view.

The UI registers to the chat model though and the service shouldn't probably be telling the UI to refresh, instead it should tell the providers and that's why I added this new event to fire when the chat model changes.

What is really ugly though is:

1. We fire only when the model is not available, which makes it an inconsistent event on service
2. If I change the model so that it doesn't fire the onDidChange event and I just fire the new event for the service it might look better, but I'm not sure of all the callers of this method and on top of that there's another event in this same service named setTitle which I don't really understand why it's there and what it could impact.

[roblourens]

How do you find out when something else changes about the chat session data? It seems like there should be a generic onDidChange event coming from the IChatService but I'm trying to trace through and I don't see how that works for the local provider. It seems like it's only chatSessionsService.onDidChangeSessionItems but I don't see how that knows about local chat sessions.