Hide more instances of Keep/Undo for non-local chat sessions #279267
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to hide the Keep/Undo UI elements (overlay widget and menu actions) for non-local chat sessions. The PR introduces checks to ensure these editing controls are only displayed for local chat sessions, which aligns with the behavior where non-local sessions are typically locked to a coding agent with different editing workflows.
Key Changes
- Added
isLocalChatcheck in the overlay controller to conditionally display the overlay widget only for local chat sessions - Added
ChatContextKeys.lockedToCodingAgent.negate()condition to the Keep and Undo action menu definitions to hide them when the session is locked to a coding agent
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingEditorOverlay.ts |
Added import for LocalChatSessionUri and created isLocalChat derived observable to check if the current session is a local session, then added this check to the condition for showing the overlay widget |
src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingActions.ts |
Added ChatContextKeys.lockedToCodingAgent.negate() to the menu visibility conditions for both Accept (Keep) and Discard (Undo) actions to prevent them from showing in non-local chat sessions |
| const isLocalChat = derived(r => { | ||
|
|
||
| const session = sessionAndEntry.read(r)?.session; | ||
| if (!session) { | ||
| return false; | ||
| } | ||
| const chatModel = chatService.getSession(session.chatSessionResource)!; | ||
| return LocalChatSessionUri.parseLocalSessionId(chatModel.sessionResource); | ||
| }); |
There was a problem hiding this comment.
The approach used here differs from the one in chatEditingActions.ts. In this file, the code checks isLocalChat (whether the session is a local session), while in chatEditingActions.ts, it checks ChatContextKeys.lockedToCodingAgent.negate() (whether the session is NOT locked to coding agent).
These two conditions are not semantically equivalent. According to the code in chatEditor.ts (lines 209-225), a non-local session may or may not be locked to the coding agent depending on whether a contribution is found. This means:
- A local session:
isLocalChat = true,lockedToCodingAgent = false - A non-local session with contribution:
isLocalChat = false,lockedToCodingAgent = true - A non-local session without contribution:
isLocalChat = false,lockedToCodingAgent = false
For the third case, the overlay widget would be hidden (because isLocalChat is false), but the Keep/Undo actions would still be shown (because lockedToCodingAgent is false). This creates an inconsistency where actions are available but the widget is not shown.
Consider using the same condition in both places for consistency. If the intent is to hide UI for all non-local sessions, use isLocalChat check in both places. If the intent is to hide UI only when locked to coding agent, use lockedToCodingAgent.negate() in both places.
| return false; | ||
| } | ||
| const chatModel = chatService.getSession(session.chatSessionResource)!; | ||
| return LocalChatSessionUri.parseLocalSessionId(chatModel.sessionResource); |
There was a problem hiding this comment.
The return value here is of type string | undefined. The function parseLocalSessionId returns the session ID string if it's a local session, or undefined if it's not. While this works as a boolean expression (truthy string vs falsy undefined), it would be clearer and more semantically correct to convert it to an explicit boolean:
return !!LocalChatSessionUri.parseLocalSessionId(chatModel.sessionResource);or
return LocalChatSessionUri.parseLocalSessionId(chatModel.sessionResource) !== undefined;This makes the intent explicit and ensures the derived observable has a consistent boolean type rather than string | undefined.
| return LocalChatSessionUri.parseLocalSessionId(chatModel.sessionResource); | |
| return !!LocalChatSessionUri.parseLocalSessionId(chatModel.sessionResource); |
No description provided.