Skip to content

Hide more instances of Keep/Undo for non-local chat sessions #279267

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+21 −6
Draft

Hide more instances of Keep/Undo for non-local chat sessions #279267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,13 @@ registerAction2(class AcceptAction extends WorkingSetAction {
title: localize2('accept.file', 'Keep'),
icon: Codicon.check,
menu: [{
when: ContextKeyExpr.and(ContextKeyExpr.equals('resourceScheme', CHAT_EDITING_MULTI_DIFF_SOURCE_RESOLVER_SCHEME), ContextKeyExpr.notIn(chatEditingResourceContextKey.key, decidedChatEditingResourceContextKey.key)),
when: ContextKeyExpr.and(ContextKeyExpr.equals('resourceScheme', CHAT_EDITING_MULTI_DIFF_SOURCE_RESOLVER_SCHEME), ContextKeyExpr.notIn(chatEditingResourceContextKey.key, decidedChatEditingResourceContextKey.key), ChatContextKeys.lockedToCodingAgent.negate()),
id: MenuId.MultiDiffEditorFileToolbar,
order: 0,
group: 'navigation',
}, {
id: MenuId.ChatEditingWidgetModifiedFilesToolbar,
when: ContextKeyExpr.equals(chatEditingWidgetFileStateContextKey.key, ModifiedFileEntryState.Modified),
when: ContextKeyExpr.and(ContextKeyExpr.equals(chatEditingWidgetFileStateContextKey.key, ModifiedFileEntryState.Modified), ChatContextKeys.lockedToCodingAgent.negate()),
order: 0,
group: 'navigation'
}],
Expand All @@ -167,13 +167,13 @@ registerAction2(class DiscardAction extends WorkingSetAction {
title: localize2('discard.file', 'Undo'),
icon: Codicon.discard,
menu: [{
when: ContextKeyExpr.and(ContextKeyExpr.equals('resourceScheme', CHAT_EDITING_MULTI_DIFF_SOURCE_RESOLVER_SCHEME), ContextKeyExpr.notIn(chatEditingResourceContextKey.key, decidedChatEditingResourceContextKey.key)),
when: ContextKeyExpr.and(ContextKeyExpr.equals('resourceScheme', CHAT_EDITING_MULTI_DIFF_SOURCE_RESOLVER_SCHEME), ContextKeyExpr.notIn(chatEditingResourceContextKey.key, decidedChatEditingResourceContextKey.key), ChatContextKeys.lockedToCodingAgent.negate()),
id: MenuId.MultiDiffEditorFileToolbar,
order: 2,
group: 'navigation',
}, {
id: MenuId.ChatEditingWidgetModifiedFilesToolbar,
when: ContextKeyExpr.equals(chatEditingWidgetFileStateContextKey.key, ModifiedFileEntryState.Modified),
when: ContextKeyExpr.and(ContextKeyExpr.equals(chatEditingWidgetFileStateContextKey.key, ModifiedFileEntryState.Modified), ChatContextKeys.lockedToCodingAgent.negate()),
order: 1,
group: 'navigation'
}],
Expand Down
19 changes: 17 additions & 2 deletions src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingEditorOverlay.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import { ThemeIcon } from '../../../../../base/common/themables.js';
import * as arrays from '../../../../../base/common/arrays.js';
import { renderAsPlaintext } from '../../../../../base/browser/markdownRenderer.js';
import { IKeybindingService } from '../../../../../platform/keybinding/common/keybinding.js';
import { LocalChatSessionUri } from '../../common/chatUri.js';

class ChatEditorOverlayWidget extends Disposable {

Expand Down Expand Up @@ -379,6 +380,17 @@ class ChatEditingOverlayController {
return chatModel.requestInProgress.read(r);
});


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);
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
return LocalChatSessionUri.parseLocalSessionId(chatModel.sessionResource);
return !!LocalChatSessionUri.parseLocalSessionId(chatModel.sessionResource);

Copilot uses AI. Check for mistakes.
});
Comment on lines +384 to +392
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

this._store.add(autorun(r => {

const data = sessionAndEntry.read(r);
Expand All @@ -397,8 +409,11 @@ class ChatEditingOverlayController {
}

if (
entry?.state.read(r) === ModifiedFileEntryState.Modified // any entry changing
|| (!session.isGlobalEditingSession && isInProgress.read(r)) // inline chat request
isLocalChat.read(r) // display widget only for local chat sessions
&& (
entry?.state.read(r) === ModifiedFileEntryState.Modified // any entry changing
|| (!session.isGlobalEditingSession && isInProgress.read(r)) // inline chat request
)
) {
// any session with changes
const editorPane = group.activeEditorPane;
Expand Down
Loading