fix chatSession swapping when in sidebar chat#281312
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where chat sessions were not properly swapping when opened in the sidebar chat view. The fix adds logic to handle the case when a chat session exists as a widget in the sidebar (as opposed to being open in an editor).
Key changes:
- Added
IChatWidgetServicedependency injection to handle sidebar chat widgets - Imported necessary types (
ChatViewPaneTarget,isIChatViewViewContext) for working with chat view contexts - Added fallback logic to handle session swapping in sidebar chat views after the existing editor handling
| const chatViewWidget = this._chatWidgetService.getWidgetBySessionResource(originalResource); | ||
| if (chatViewWidget && isIChatViewViewContext(chatViewWidget.viewContext)) { | ||
| await this._chatSessionsService.getOrCreateChatSession(modifiedResource, CancellationToken.None); | ||
| await this._chatWidgetService.openSession(modifiedResource, ChatViewPaneTarget, { preserveFocus: true }); |
There was a problem hiding this comment.
The fix correctly handles chat session swapping in the sidebar, but there's a potential issue: if getWidgetBySessionResource returns a widget but isIChatViewViewContext returns false (e.g., for a quick chat or inline chat), the function will silently do nothing instead of handling that case or logging it. Consider adding an else clause to handle non-view contexts or at least log when a widget exists but isn't in the expected context.
| await this._chatWidgetService.openSession(modifiedResource, ChatViewPaneTarget, { preserveFocus: true }); | |
| await this._chatWidgetService.openSession(modifiedResource, ChatViewPaneTarget, { preserveFocus: true }); | |
| } else if (chatViewWidget) { | |
| this._logService.trace(`$onDidCommitChatSessionItem: widget found for resource ${originalResource} but not in view context (context: ${chatViewWidget.viewContext}) - no action taken`); |
| const chatViewWidget = this._chatWidgetService.getWidgetBySessionResource(originalResource); | ||
| if (chatViewWidget && isIChatViewViewContext(chatViewWidget.viewContext)) { | ||
| await this._chatSessionsService.getOrCreateChatSession(modifiedResource, CancellationToken.None); | ||
| await this._chatWidgetService.openSession(modifiedResource, ChatViewPaneTarget, { preserveFocus: true }); | ||
| } |
There was a problem hiding this comment.
This new code path for handling sidebar chat session swapping lacks test coverage. Consider adding a test case in mainThreadChatSessions.test.ts that verifies the behavior when a chat view widget exists and $onDidCommitChatSessionItem is called, ensuring the session is properly opened in the sidebar.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@joshspicer I've opened a new pull request, #281314, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
|
||
| const chatViewWidget = this._chatWidgetService.getWidgetBySessionResource(originalResource); | ||
| if (chatViewWidget && isIChatViewViewContext(chatViewWidget.viewContext)) { | ||
| await this._chatSessionsService.getOrCreateChatSession(modifiedResource, CancellationToken.None); |
There was a problem hiding this comment.
Do you have to do this step? I believe you should only have to call openSession
fix #281270