From acd63e770bdedb3ddbc7e73e5d4b3f5da4903007 Mon Sep 17 00:00:00 2001 From: Harald Kirschner Date: Mon, 9 Mar 2026 02:42:55 -0700 Subject: [PATCH] Fix handoff widget visibility: derive from response mode, persist modeInfo (#298867) * Fix handoff widget visibility: derive from response mode, persist modeInfo, fix lifecycle * Fix handoff widget response mode logic * Make findModeByName search _customModeInstances directly like findModeById does, bypassing the getCustomModes() gate * Fix tests * Fix custom mode lookup to avoid builtin name collisions * address review: use findModeByName consistently, respect getCustomModes guard --- .../contrib/chat/browser/widget/chatWidget.ts | 35 ++++++++----- .../contrib/chat/common/model/chatModel.ts | 3 ++ .../common/model/chatSessionOperationLog.ts | 1 + .../ChatService_can_deserialize.0.snap | 1 + ...rvice_can_deserialize_with_response.0.snap | 1 + .../ChatService_can_serialize.1.snap | 2 + .../ChatService_sendRequest_fails.0.snap | 1 + .../chat/test/common/model/chatModel.test.ts | 50 ++++++++++++++++++- 8 files changed, 79 insertions(+), 15 deletions(-) diff --git a/src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts b/src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts index 266efc7b245..0cae54e9130 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/chatWidget.ts @@ -53,7 +53,7 @@ import { ChatContextKeys } from '../../common/actions/chatContextKeys.js'; import { applyingChatEditsFailedContextKey, decidedChatEditingResourceContextKey, hasAppliedChatEditsContextKey, hasUndecidedChatEditingResourceContextKey, IChatEditingService, IChatEditingSession, inChatEditingSessionContextKey, ModifiedFileEntryState } from '../../common/editing/chatEditingService.js'; import { IChatLayoutService } from '../../common/widget/chatLayoutService.js'; import { IChatModel, IChatModelInputState, IChatResponseModel } from '../../common/model/chatModel.js'; -import { ChatMode, getModeNameForTelemetry, IChatModeService } from '../../common/chatModes.js'; +import { ChatMode, getModeNameForTelemetry, IChatMode, IChatModeService } from '../../common/chatModes.js'; import { chatAgentLeader, ChatRequestAgentPart, ChatRequestDynamicVariablePart, ChatRequestSlashPromptPart, ChatRequestToolPart, ChatRequestToolSetPart, chatSubcommandLeader, formatChatQuestion, IParsedChatRequest } from '../../common/requestParser/chatParserTypes.js'; import { ChatRequestParser } from '../../common/requestParser/chatRequestParser.js'; import { getDynamicVariablesForWidget, getSelectedToolAndToolSetsForWidget } from '../attachments/chatVariables.js'; @@ -1182,27 +1182,34 @@ export class ChatWidget extends Disposable implements IChatWidget { const lastItem = items[items.length - 1]; const lastResponseComplete = lastItem && isResponseVM(lastItem) && lastItem.isComplete; - if (!lastResponseComplete) { + if (!lastResponseComplete || lastItem.isCanceled) { + this.chatSuggestNextWidget.hide(); return; } - // Get the currently selected mode directly from the observable - // Note: We use currentModeObs instead of currentModeKind because currentModeKind returns - // the ChatModeKind enum (e.g., 'agent'), which doesn't distinguish between custom modes. - // Custom modes all have kind='agent' but different IDs. - const currentMode = this.input.currentModeObs.get(); - const handoffs = currentMode?.handOffs?.get(); - // Only show if: mode has handoffs AND chat has content AND not quick chat - const shouldShow = currentMode && handoffs && handoffs.length > 0; + // Derive handoffs from the mode that generated the last response, not the current UI selection. + // This ensures handoffs reflect what the response agent offers, regardless of mode picker state. + // Fall back to the current mode picker for old sessions where modeInfo was not persisted. + const modeInfo = lastItem.model.request?.modeInfo; + let responseMode: IChatMode | undefined; + if (modeInfo?.modeInstructions?.name) { + responseMode = this.chatModeService.findModeByName(modeInfo.modeInstructions.name); + } else if (modeInfo?.modeId) { + responseMode = this.chatModeService.findModeById(modeInfo.modeId); + } else { + responseMode = this.input.currentModeObs.get(); + } - if (shouldShow) { + const handoffs = responseMode?.handOffs?.get(); + + if (responseMode && handoffs && handoffs.length > 0) { // Log telemetry only when widget transitions from hidden to visible const wasHidden = this.chatSuggestNextWidget.domNode.style.display === 'none'; - this.chatSuggestNextWidget.render(currentMode); + this.chatSuggestNextWidget.render(responseMode); if (wasHidden) { this.telemetryService.publicLog2('chat.handoffWidgetShown', { - agent: getModeNameForTelemetry(currentMode), + agent: getModeNameForTelemetry(responseMode), handoffCount: handoffs.length }); } @@ -2006,6 +2013,7 @@ export class ChatWidget extends Disposable implements IChatWidget { if (e.kind === 'addRequest') { this.inputPart.clearTodoListWidget(this.viewModel?.sessionResource, false); this._sessionIsEmptyContextKey.set(false); + this.chatSuggestNextWidget.hide(); } // Hide widget on request removal if (e.kind === 'removeRequest') { @@ -2036,6 +2044,7 @@ export class ChatWidget extends Disposable implements IChatWidget { this.listWidget.scrollToEnd(); } + this.renderChatSuggestNextWidget(); this.updateChatInputContext(); this.input.renderChatTodoListWidget(this.viewModel.sessionResource); } diff --git a/src/vs/workbench/contrib/chat/common/model/chatModel.ts b/src/vs/workbench/contrib/chat/common/model/chatModel.ts index b7d2f749317..9e9b63d7861 100644 --- a/src/vs/workbench/contrib/chat/common/model/chatModel.ts +++ b/src/vs/workbench/contrib/chat/common/model/chatModel.ts @@ -1449,6 +1449,7 @@ export interface ISerializableChatRequestData extends ISerializableChatResponseD confirmation?: string; editedFileEvents?: IChatAgentEditedFileEvent[]; modelId?: string; + modeInfo?: IChatRequestModeInfo; } export interface ISerializableMarkdownInfo { @@ -2315,6 +2316,7 @@ export class ChatModel extends Disposable implements IChatModel { confirmation: raw.confirmation, editedFileEvents: raw.editedFileEvents, modelId: raw.modelId, + modeInfo: raw.modeInfo, }); request.shouldBeRemovedOnSend = raw.isHidden ? { requestId: raw.requestId } : raw.shouldBeRemovedOnSend; // eslint-disable-next-line @typescript-eslint/no-explicit-any, local/code-no-any-casts @@ -2657,6 +2659,7 @@ export class ChatModel extends Disposable implements IChatModel { confirmation: r.confirmation, editedFileEvents: r.editedFileEvents, modelId: r.modelId, + modeInfo: r.modeInfo, ...r.response?.toJSON(), }; }), diff --git a/src/vs/workbench/contrib/chat/common/model/chatSessionOperationLog.ts b/src/vs/workbench/contrib/chat/common/model/chatSessionOperationLog.ts index 767cb087be6..c6da97a55d2 100644 --- a/src/vs/workbench/contrib/chat/common/model/chatSessionOperationLog.ts +++ b/src/vs/workbench/contrib/chat/common/model/chatSessionOperationLog.ts @@ -148,6 +148,7 @@ const requestSchema = Adapt.object m.response?.contentReferences, objectsEqual), codeCitations: Adapt.v(m => m.response?.codeCitations, objectsEqual), timeSpentWaiting: Adapt.v(m => m.response?.timestamp), // based on response timestamp + modeInfo: Adapt.v(m => m.modeInfo, objectsEqual), }, { sealed: (o) => o.modelState?.value === ResponseModelState.Cancelled || o.modelState?.value === ResponseModelState.Failed || o.modelState?.value === ResponseModelState.Complete, }); diff --git a/src/vs/workbench/contrib/chat/test/common/chatService/__snapshots__/ChatService_can_deserialize.0.snap b/src/vs/workbench/contrib/chat/test/common/chatService/__snapshots__/ChatService_can_deserialize.0.snap index 643e3727914..374c04da997 100644 --- a/src/vs/workbench/contrib/chat/test/common/chatService/__snapshots__/ChatService_can_deserialize.0.snap +++ b/src/vs/workbench/contrib/chat/test/common/chatService/__snapshots__/ChatService_can_deserialize.0.snap @@ -75,6 +75,7 @@ confirmation: undefined, editedFileEvents: undefined, modelId: undefined, + modeInfo: undefined, responseId: undefined, result: { metadata: { metadataKey: "value" } }, responseMarkdownInfo: undefined, diff --git a/src/vs/workbench/contrib/chat/test/common/chatService/__snapshots__/ChatService_can_deserialize_with_response.0.snap b/src/vs/workbench/contrib/chat/test/common/chatService/__snapshots__/ChatService_can_deserialize_with_response.0.snap index f0e423320dd..3119761173f 100644 --- a/src/vs/workbench/contrib/chat/test/common/chatService/__snapshots__/ChatService_can_deserialize_with_response.0.snap +++ b/src/vs/workbench/contrib/chat/test/common/chatService/__snapshots__/ChatService_can_deserialize_with_response.0.snap @@ -75,6 +75,7 @@ confirmation: undefined, editedFileEvents: undefined, modelId: undefined, + modeInfo: undefined, responseId: undefined, result: { errorDetails: { message: "No activated agent with id \"ChatProviderWithUsedContext\"" } }, responseMarkdownInfo: undefined, diff --git a/src/vs/workbench/contrib/chat/test/common/chatService/__snapshots__/ChatService_can_serialize.1.snap b/src/vs/workbench/contrib/chat/test/common/chatService/__snapshots__/ChatService_can_serialize.1.snap index 3e5c32aa740..a3d39df663c 100644 --- a/src/vs/workbench/contrib/chat/test/common/chatService/__snapshots__/ChatService_can_serialize.1.snap +++ b/src/vs/workbench/contrib/chat/test/common/chatService/__snapshots__/ChatService_can_serialize.1.snap @@ -77,6 +77,7 @@ confirmation: undefined, editedFileEvents: undefined, modelId: undefined, + modeInfo: undefined, responseId: undefined, result: { metadata: { metadataKey: "value" } }, responseMarkdownInfo: undefined, @@ -162,6 +163,7 @@ confirmation: undefined, editedFileEvents: undefined, modelId: undefined, + modeInfo: undefined, responseId: undefined, result: { }, responseMarkdownInfo: undefined, diff --git a/src/vs/workbench/contrib/chat/test/common/chatService/__snapshots__/ChatService_sendRequest_fails.0.snap b/src/vs/workbench/contrib/chat/test/common/chatService/__snapshots__/ChatService_sendRequest_fails.0.snap index 9a7073212a2..5fc5d409eb3 100644 --- a/src/vs/workbench/contrib/chat/test/common/chatService/__snapshots__/ChatService_sendRequest_fails.0.snap +++ b/src/vs/workbench/contrib/chat/test/common/chatService/__snapshots__/ChatService_sendRequest_fails.0.snap @@ -77,6 +77,7 @@ confirmation: undefined, editedFileEvents: undefined, modelId: undefined, + modeInfo: undefined, responseId: undefined, result: { errorDetails: { message: "No activated agent with id \"ChatProviderWithUsedContext\"" } }, responseMarkdownInfo: undefined, diff --git a/src/vs/workbench/contrib/chat/test/common/model/chatModel.test.ts b/src/vs/workbench/contrib/chat/test/common/model/chatModel.test.ts index c0800d4b46d..e89b504b224 100644 --- a/src/vs/workbench/contrib/chat/test/common/model/chatModel.test.ts +++ b/src/vs/workbench/contrib/chat/test/common/model/chatModel.test.ts @@ -25,10 +25,10 @@ import { TestExtensionService, TestStorageService } from '../../../../../test/co import { CellUri } from '../../../../notebook/common/notebookCommon.js'; import { IChatRequestImplicitVariableEntry, IChatRequestStringVariableEntry, IChatRequestFileEntry, StringChatContextValue } from '../../../common/attachments/chatVariableEntries.js'; import { ChatAgentService, IChatAgentService } from '../../../common/participants/chatAgents.js'; -import { ChatModel, ChatRequestModel, IExportableChatData, ISerializableChatData1, ISerializableChatData2, ISerializableChatData3, isExportableSessionData, isSerializableSessionData, normalizeSerializableChatData, Response } from '../../../common/model/chatModel.js'; +import { ChatModel, ChatRequestModel, IChatRequestModeInfo, IExportableChatData, ISerializableChatData1, ISerializableChatData2, ISerializableChatData3, isExportableSessionData, isSerializableSessionData, normalizeSerializableChatData, Response } from '../../../common/model/chatModel.js'; import { ChatRequestTextPart } from '../../../common/requestParser/chatParserTypes.js'; import { ChatRequestQueueKind, IChatService, IChatToolInvocation } from '../../../common/chatService/chatService.js'; -import { ChatAgentLocation } from '../../../common/constants.js'; +import { ChatAgentLocation, ChatModeKind } from '../../../common/constants.js'; import { MockChatService } from '../chatService/mockChatService.js'; suite('ChatModel', () => { @@ -274,6 +274,52 @@ suite('ChatModel', () => { // Should keep file attachments and implicit attachments with URI values assert.deepStrictEqual(serialized.attachments, [fileAttachment, implicitWithUri]); }); + + test('modeInfo roundtrips through serialization', async () => { + const modeInfo: IChatRequestModeInfo = { + kind: ChatModeKind.Agent, + isBuiltin: false, + modeId: 'custom', + modeInstructions: { + name: 'plan', + content: 'You are a planning agent', + toolReferences: [], + }, + applyCodeBlockSuggestionId: undefined, + }; + + const serializableData: ISerializableChatData3 = { + version: 3, + sessionId: 'test-modeinfo-session', + creationDate: Date.now(), + customTitle: undefined, + initialLocation: ChatAgentLocation.Chat, + responderUsername: 'bot', + requests: [{ + requestId: 'req1', + message: { text: 'plan something', parts: [] }, + variableData: { variables: [] }, + response: [{ value: 'Here is my plan', isTrusted: false }], + modelState: { value: 1 /* ResponseModelState.Complete */, completedAt: Date.now() }, + modeInfo, + }], + }; + + const model = testDisposables.add(instantiationService.createInstance( + ChatModel, + { value: serializableData, serializer: undefined! }, + { initialLocation: ChatAgentLocation.Chat, canUseTools: true } + )); + + const requests = model.getRequests(); + assert.strictEqual(requests.length, 1); + assert.deepStrictEqual(requests[0].modeInfo, modeInfo); + + // Verify roundtrip through toExport + const exported = model.toExport(); + assert.strictEqual(exported.requests.length, 1); + assert.deepStrictEqual(exported.requests[0].modeInfo, modeInfo); + }); }); suite('Response', () => {