From 0fabcd191f74808991f4400b72c1ff8b36bd495a Mon Sep 17 00:00:00 2001 From: Tyler James Leonhardt <2644648+TylerLeonhardt@users.noreply.github.com> Date: Wed, 19 Mar 2025 17:57:06 -0700 Subject: [PATCH 1/6] Remove the URL from the response of the tool (#244067) The url can be inferred from the index. --- .../electron-sandbox/tools/fetchPageTool.ts | 29 ++++++------------- 1 file changed, 9 insertions(+), 20 deletions(-) diff --git a/src/vs/workbench/contrib/chat/electron-sandbox/tools/fetchPageTool.ts b/src/vs/workbench/contrib/chat/electron-sandbox/tools/fetchPageTool.ts index b4969f303b3..f05445610f5 100644 --- a/src/vs/workbench/contrib/chat/electron-sandbox/tools/fetchPageTool.ts +++ b/src/vs/workbench/contrib/chat/electron-sandbox/tools/fetchPageTool.ts @@ -59,14 +59,14 @@ export class FetchWebPageTool implements IToolImpl { const contents = await this._readerModeService.extract(validUris); // Make an array that contains either the content or undefined for invalid URLs - const contentsWithUndefined = new Map(); + const contentsWithUndefined: (string | undefined)[] = []; let indexInContents = 0; - parsedUriResults.forEach((uri, url) => { + parsedUriResults.forEach((uri) => { if (uri) { - contentsWithUndefined.set(url, contents[indexInContents]); + contentsWithUndefined.push(contents[indexInContents]); indexInContents++; } else { - contentsWithUndefined.set(url, undefined); + contentsWithUndefined.push(undefined); } }); @@ -154,21 +154,10 @@ export class FetchWebPageTool implements IToolImpl { return results; } - private _getPromptPartsForResults(results: Map): IToolResultTextPart[] { - const arr = new Array(); - for (const [url, content] of results.entries()) { - if (content) { - arr.push({ - kind: 'text', - value: `\n\n` + content - }); - } else { - arr.push({ - kind: 'text', - value: `\n\n` + localize('fetchWebPage.invalidUrl', 'Invalid URL') - }); - } - } - return arr; + private _getPromptPartsForResults(results: (string | undefined)[]): IToolResultTextPart[] { + return results.map(value => ({ + kind: 'text', + value: value || localize('fetchWebPage.invalidUrl', 'Invalid URL') + })); } } From 4abd6cd1c20669bc0a0d71a836b03afa2f274c01 Mon Sep 17 00:00:00 2001 From: Rob Lourens Date: Wed, 19 Mar 2025 18:04:27 -0700 Subject: [PATCH 2/6] Fix undo/remove actions in unified view (#244068) --- .../chat/browser/actions/chatClearActions.ts | 13 ++++++++----- .../chat/browser/actions/chatTitleActions.ts | 4 ++-- .../chat/browser/chatEditing/chatEditingActions.ts | 8 ++++---- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/vs/workbench/contrib/chat/browser/actions/chatClearActions.ts b/src/vs/workbench/contrib/chat/browser/actions/chatClearActions.ts index 8bf6f719422..7e601ac31e2 100644 --- a/src/vs/workbench/contrib/chat/browser/actions/chatClearActions.ts +++ b/src/vs/workbench/contrib/chat/browser/actions/chatClearActions.ts @@ -91,7 +91,10 @@ export function registerNewChatActions() { }, menu: [{ id: MenuId.ChatContext, - group: 'z_clear' + group: 'z_clear', + when: ContextKeyExpr.and( + ChatContextKeys.location.isEqualTo(ChatAgentLocation.Panel), + ChatContextKeys.inUnifiedChat.negate()), }, { id: MenuId.ViewTitle, @@ -244,14 +247,14 @@ export function registerNewChatActions() { constructor() { super({ id: 'workbench.action.chat.undoEdit', - title: localize2('chat.undoEdit.label', "Undo Last Edit"), + title: localize2('chat.undoEdit.label', "Undo Last Request"), category: CHAT_CATEGORY, icon: Codicon.discard, precondition: ContextKeyExpr.and(ChatContextKeys.chatEditingCanUndo, ChatContextKeys.enabled, ChatContextKeys.editingParticipantRegistered), f1: true, menu: [{ id: MenuId.ViewTitle, - when: ChatContextKeyExprs.inEditingMode, + when: ChatContextKeyExprs.inEditsOrUnified, group: 'navigation', order: -3 }] @@ -267,14 +270,14 @@ export function registerNewChatActions() { constructor() { super({ id: 'workbench.action.chat.redoEdit', - title: localize2('chat.redoEdit.label', "Redo Last Edit"), + title: localize2('chat.redoEdit.label', "Redo Last Request"), category: CHAT_CATEGORY, icon: Codicon.redo, precondition: ContextKeyExpr.and(ChatContextKeys.chatEditingCanRedo, ChatContextKeys.enabled, ChatContextKeys.editingParticipantRegistered), f1: true, menu: [{ id: MenuId.ViewTitle, - when: ChatContextKeyExprs.inEditingMode, + when: ChatContextKeyExprs.inEditsOrUnified, group: 'navigation', order: -2 }] diff --git a/src/vs/workbench/contrib/chat/browser/actions/chatTitleActions.ts b/src/vs/workbench/contrib/chat/browser/actions/chatTitleActions.ts index f5734cdeb53..74601935fae 100644 --- a/src/vs/workbench/contrib/chat/browser/actions/chatTitleActions.ts +++ b/src/vs/workbench/contrib/chat/browser/actions/chatTitleActions.ts @@ -358,7 +358,7 @@ export function registerChatTitleActions() { f1: false, category: CHAT_CATEGORY, icon: Codicon.x, - precondition: ChatContextKeys.chatMode.isEqualTo(ChatMode.Ask), + precondition: ContextKeyExpr.and(ChatContextKeys.chatMode.isEqualTo(ChatMode.Ask), ChatContextKeyExprs.unifiedChatEnabled.negate()), keybinding: { primary: KeyCode.Delete, mac: { @@ -371,7 +371,7 @@ export function registerChatTitleActions() { id: MenuId.ChatMessageTitle, group: 'navigation', order: 2, - when: ContextKeyExpr.and(ChatContextKeys.chatMode.isEqualTo(ChatMode.Ask), ChatContextKeys.isRequest) + when: ContextKeyExpr.and(ChatContextKeys.chatMode.isEqualTo(ChatMode.Ask), ChatContextKeys.isRequest, ChatContextKeyExprs.unifiedChatEnabled.negate()) } }); } diff --git a/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingActions.ts b/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingActions.ts index e2c11a7e14a..81acd879059 100644 --- a/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingActions.ts +++ b/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingActions.ts @@ -439,7 +439,7 @@ registerAction2(class RemoveAction extends Action2 { constructor() { super({ id: 'workbench.action.chat.undoEdits', - title: localize2('chat.undoEdits.label', "Undo Edits"), + title: localize2('chat.undoEdits.label', "Undo Requests"), f1: false, category: CHAT_CATEGORY, icon: Codicon.x, @@ -448,7 +448,7 @@ registerAction2(class RemoveAction extends Action2 { mac: { primary: KeyMod.CtrlCmd | KeyCode.Backspace, }, - when: ContextKeyExpr.and(ChatContextKeys.chatMode.notEqualsTo(ChatMode.Ask), ChatContextKeys.inChatSession, EditorContextKeys.textInputFocus.negate()), + when: ContextKeyExpr.and(ChatContextKeys.inChatSession, EditorContextKeys.textInputFocus.negate()), weight: KeybindingWeight.WorkbenchContrib, }, menu: [ @@ -456,7 +456,7 @@ registerAction2(class RemoveAction extends Action2 { id: MenuId.ChatMessageTitle, group: 'navigation', order: 2, - when: ContextKeyExpr.and(ChatContextKeys.chatMode.notEqualsTo(ChatMode.Ask), ChatContextKeys.isRequest) + when: ChatContextKeys.isRequest } ] }); @@ -479,7 +479,7 @@ registerAction2(class RemoveAction extends Action2 { const chatEditingService = accessor.get(IChatEditingService); const chatService = accessor.get(IChatService); const chatModel = chatService.getSession(item.sessionId); - if (chatModel?.initialLocation !== ChatAgentLocation.EditingSession) { + if (!chatModel) { return; } From 968a6f93879ab1b1bc688318ea782d860ee450c1 Mon Sep 17 00:00:00 2001 From: Connor Peet Date: Wed, 19 Mar 2025 18:08:52 -0700 Subject: [PATCH 3/6] mcp: fix server lenses click issues (#244069) Slight refactor: moved the logger to be owned by the McpServer rather than the McpServerRequestHandler so it doesn't disappear when a server is restarting. Fixes #244058 --- .../contrib/mcp/browser/mcp.contribution.ts | 3 +- .../contrib/mcp/browser/mcpCommands.ts | 21 +++- .../mcp/browser/mcpLanguageFeatures.ts | 21 +++- .../contrib/mcp/common/mcpRegistry.ts | 3 +- .../contrib/mcp/common/mcpRegistryTypes.ts | 2 + .../workbench/contrib/mcp/common/mcpServer.ts | 17 ++- .../contrib/mcp/common/mcpServerConnection.ts | 21 +--- .../workbench/contrib/mcp/common/mcpTypes.ts | 5 - .../mcp/test/common/mcpRegistry.test.ts | 20 +++- .../test/common/mcpServerConnection.test.ts | 104 +++++------------- 10 files changed, 105 insertions(+), 112 deletions(-) diff --git a/src/vs/workbench/contrib/mcp/browser/mcp.contribution.ts b/src/vs/workbench/contrib/mcp/browser/mcp.contribution.ts index b40f76ae284..d76e4d4c500 100644 --- a/src/vs/workbench/contrib/mcp/browser/mcp.contribution.ts +++ b/src/vs/workbench/contrib/mcp/browser/mcp.contribution.ts @@ -21,7 +21,7 @@ import { McpRegistry } from '../common/mcpRegistry.js'; import { IMcpRegistry } from '../common/mcpRegistryTypes.js'; import { McpService } from '../common/mcpService.js'; import { IMcpService } from '../common/mcpTypes.js'; -import { AddConfigurationAction, EditStoredInput, InstallFromActivation, ListMcpServerCommand, MCPServerActionRendering, McpServerOptionsCommand, RemoveStoredInput, ResetMcpCachedTools, ResetMcpTrustCommand, ShowOutput, StartServer, StopServer } from './mcpCommands.js'; +import { AddConfigurationAction, EditStoredInput, InstallFromActivation, ListMcpServerCommand, MCPServerActionRendering, McpServerOptionsCommand, RemoveStoredInput, ResetMcpCachedTools, ResetMcpTrustCommand, RestartServer, ShowOutput, StartServer, StopServer } from './mcpCommands.js'; import { McpDiscovery } from './mcpDiscovery.js'; import { McpLanguageFeatures } from './mcpLanguageFeatures.js'; import { McpUrlHandler } from './mcpUrlHandler.js'; @@ -50,6 +50,7 @@ registerAction2(StartServer); registerAction2(StopServer); registerAction2(ShowOutput); registerAction2(InstallFromActivation); +registerAction2(RestartServer); registerWorkbenchContribution2('mcpActionRendering', MCPServerActionRendering, WorkbenchPhase.BlockRestore); diff --git a/src/vs/workbench/contrib/mcp/browser/mcpCommands.ts b/src/vs/workbench/contrib/mcp/browser/mcpCommands.ts index 22d980d6cb1..9bdf4acf4c0 100644 --- a/src/vs/workbench/contrib/mcp/browser/mcpCommands.ts +++ b/src/vs/workbench/contrib/mcp/browser/mcpCommands.ts @@ -471,6 +471,26 @@ export class ShowOutput extends Action2 { } } +export class RestartServer extends Action2 { + static readonly ID = 'workbench.mcp.restartServer'; + + constructor() { + super({ + id: RestartServer.ID, + title: localize2('mcp.command.restartServer', "Restart Server"), + category, + f1: false, + }); + } + + async run(accessor: ServicesAccessor, serverId: string) { + const s = accessor.get(IMcpService).servers.get().find(s => s.definition.id === serverId); + s?.showOutput(); + await s?.stop(); + await s?.start(); + } +} + export class StartServer extends Action2 { static readonly ID = 'workbench.mcp.startServer'; @@ -485,7 +505,6 @@ export class StartServer extends Action2 { async run(accessor: ServicesAccessor, serverId: string) { const s = accessor.get(IMcpService).servers.get().find(s => s.definition.id === serverId); - await s?.stop(); await s?.start(); } } diff --git a/src/vs/workbench/contrib/mcp/browser/mcpLanguageFeatures.ts b/src/vs/workbench/contrib/mcp/browser/mcpLanguageFeatures.ts index 46a923ed8ba..ace9d8906f2 100644 --- a/src/vs/workbench/contrib/mcp/browser/mcpLanguageFeatures.ts +++ b/src/vs/workbench/contrib/mcp/browser/mcpLanguageFeatures.ts @@ -19,7 +19,7 @@ import { ConfigurationResolverExpression, IResolvedValue } from '../../../servic import { IMcpConfigPathsService } from '../common/mcpConfigPathsService.js'; import { IMcpRegistry } from '../common/mcpRegistryTypes.js'; import { IMcpService, McpConnectionState } from '../common/mcpTypes.js'; -import { EditStoredInput, RemoveStoredInput, ShowOutput, StartServer, StopServer } from './mcpCommands.js'; +import { EditStoredInput, RemoveStoredInput, RestartServer, ShowOutput, StartServer, StopServer } from './mcpCommands.js'; export class McpLanguageFeatures extends Disposable implements IWorkbenchContribution { private readonly _cachedMcpSection = this._register(new MutableDisposable<{ model: ITextModel; node: Node } & IDisposable>()); @@ -109,7 +109,7 @@ export class McpLanguageFeatures extends Disposable implements IWorkbenchContrib }, { range, command: { - id: StartServer.ID, + id: RestartServer.ID, title: localize('mcp.restart', "Restart"), arguments: [server.definition.id], }, @@ -143,19 +143,19 @@ export class McpLanguageFeatures extends Disposable implements IWorkbenchContrib }, { range, command: { - id: StartServer.ID, + id: RestartServer.ID, title: localize('mcp.restart', "Restart"), arguments: [server.definition.id], }, }, { range, command: { - id: 'workbench.action.chat.attachTools', + id: '', title: localize('server.toolCount', '{0} tools', read(server.tools).length), }, }); break; - case McpConnectionState.Kind.Stopped: + case McpConnectionState.Kind.Stopped: { lenses.lenses.push({ range, command: { @@ -164,6 +164,17 @@ export class McpLanguageFeatures extends Disposable implements IWorkbenchContrib arguments: [server.definition.id], }, }); + const toolCount = read(server.tools).length; + if (toolCount) { + lenses.lenses.push({ + range, + command: { + id: '', + title: localize('server.toolCountCached', '{0} cached tools', toolCount), + } + }); + } + } } } diff --git a/src/vs/workbench/contrib/mcp/common/mcpRegistry.ts b/src/vs/workbench/contrib/mcp/common/mcpRegistry.ts index a5ca2d3e84e..9bb1072b95f 100644 --- a/src/vs/workbench/contrib/mcp/common/mcpRegistry.ts +++ b/src/vs/workbench/contrib/mcp/common/mcpRegistry.ts @@ -293,7 +293,7 @@ export class McpRegistry extends Disposable implements IMcpRegistry { return await this._configurationResolverService.resolveAsync(folder, expr); } - public async resolveConnection({ collectionRef, definitionRef, forceTrust }: IMcpResolveConnectionOptions): Promise { + public async resolveConnection({ collectionRef, definitionRef, forceTrust, logger }: IMcpResolveConnectionOptions): Promise { const collection = this._collections.get().find(c => c.id === collectionRef.id); const definition = collection?.serverDefinitions.get().find(s => s.id === definitionRef.id); if (!collection || !definition) { @@ -356,6 +356,7 @@ export class McpRegistry extends Disposable implements IMcpRegistry { definition, delegate, launch, + logger, ); } } diff --git a/src/vs/workbench/contrib/mcp/common/mcpRegistryTypes.ts b/src/vs/workbench/contrib/mcp/common/mcpRegistryTypes.ts index 6c0461beacd..1f106a81d67 100644 --- a/src/vs/workbench/contrib/mcp/common/mcpRegistryTypes.ts +++ b/src/vs/workbench/contrib/mcp/common/mcpRegistryTypes.ts @@ -8,6 +8,7 @@ import { IDisposable } from '../../../../base/common/lifecycle.js'; import { IObservable } from '../../../../base/common/observable.js'; import { ConfigurationTarget } from '../../../../platform/configuration/common/configuration.js'; import { createDecorator } from '../../../../platform/instantiation/common/instantiation.js'; +import { ILogger } from '../../../../platform/log/common/log.js'; import { StorageScope } from '../../../../platform/storage/common/storage.js'; import { IWorkspaceFolderData } from '../../../../platform/workspace/common/workspace.js'; import { IResolvedValue } from '../../../services/configurationResolver/common/configurationResolverExpression.js'; @@ -32,6 +33,7 @@ export interface IMcpHostDelegate { } export interface IMcpResolveConnectionOptions { + logger: ILogger; collectionRef: McpCollectionReference; definitionRef: McpDefinitionReference; /** If set, the user will be asked to trust the collection even if they untrusted it previously */ diff --git a/src/vs/workbench/contrib/mcp/common/mcpServer.ts b/src/vs/workbench/contrib/mcp/common/mcpServer.ts index 66203057607..3bbd1bd7f58 100644 --- a/src/vs/workbench/contrib/mcp/common/mcpServer.ts +++ b/src/vs/workbench/contrib/mcp/common/mcpServer.ts @@ -8,9 +8,11 @@ import { CancellationToken, CancellationTokenSource } from '../../../../base/com import { Disposable, DisposableStore, IDisposable, toDisposable } from '../../../../base/common/lifecycle.js'; import { LRUCache } from '../../../../base/common/map.js'; import { autorun, autorunWithStore, derived, disposableObservableValue, IObservable, ITransaction, observableFromEvent, ObservablePromise, observableValue, transaction } from '../../../../base/common/observable.js'; +import { ILogger, ILoggerService } from '../../../../platform/log/common/log.js'; import { IStorageService, StorageScope, StorageTarget } from '../../../../platform/storage/common/storage.js'; import { IWorkspaceContextService } from '../../../../platform/workspace/common/workspace.js'; import { IExtensionService } from '../../../services/extensions/common/extensions.js'; +import { IOutputService } from '../../../services/output/common/output.js'; import { mcpActivationEvent } from './mcpConfiguration.js'; import { IMcpRegistry } from './mcpRegistryTypes.js'; import { McpServerRequestHandler } from './mcpServerRequestHandler.js'; @@ -128,6 +130,9 @@ export class McpServer extends Disposable implements IMcpServer { return fromServerResult.error ? (this.toolsFromCache ? McpServerToolsState.Cached : McpServerToolsState.Unknown) : McpServerToolsState.Live; }); + private readonly _loggerId: string; + private readonly _logger: ILogger; + public get trusted() { return this._mcpRegistry.getTrust(this.collection); } @@ -140,9 +145,17 @@ export class McpServer extends Disposable implements IMcpServer { @IMcpRegistry private readonly _mcpRegistry: IMcpRegistry, @IWorkspaceContextService workspacesService: IWorkspaceContextService, @IExtensionService private readonly _extensionService: IExtensionService, + @ILoggerService private readonly _loggerService: ILoggerService, + @IOutputService private readonly _outputService: IOutputService, ) { super(); + this._loggerId = `mcpServer/${definition.id}`; + this._logger = this._register(_loggerService.createLogger(this._loggerId, { hidden: true, name: `MCP: ${definition.label}` })); + // If the logger is disposed but not deregistered, then the disposed instance + // is reused and no-ops. todo@sandy081 this seems like a bug. + this._register(toDisposable(() => _loggerService.deregisterLogger(this._loggerId))); + // 1. Reflect workspaces into the MCP roots const workspaces = observableFromEvent( this, @@ -191,7 +204,8 @@ export class McpServer extends Disposable implements IMcpServer { } public showOutput(): void { - this._connection.get()?.showOutput(); + this._loggerService.setVisibility(this._loggerId, true); + this._outputService.showChannel(this._loggerId); } public start(isFromInteraction?: boolean): Promise { @@ -217,6 +231,7 @@ export class McpServer extends Disposable implements IMcpServer { if (!connection) { connection = await this._mcpRegistry.resolveConnection({ + logger: this._logger, collectionRef: this.collection, definitionRef: this.definition, forceTrust: isFromInteraction, diff --git a/src/vs/workbench/contrib/mcp/common/mcpServerConnection.ts b/src/vs/workbench/contrib/mcp/common/mcpServerConnection.ts index 2a870ca87d3..7e99c9083ae 100644 --- a/src/vs/workbench/contrib/mcp/common/mcpServerConnection.ts +++ b/src/vs/workbench/contrib/mcp/common/mcpServerConnection.ts @@ -8,11 +8,10 @@ import { Disposable, DisposableStore, IReference, MutableDisposable, toDisposabl import { autorun, IObservable, observableValue } from '../../../../base/common/observable.js'; import { localize } from '../../../../nls.js'; import { IInstantiationService } from '../../../../platform/instantiation/common/instantiation.js'; -import { ILogger, ILoggerService } from '../../../../platform/log/common/log.js'; -import { IOutputService } from '../../../services/output/common/output.js'; +import { ILogger } from '../../../../platform/log/common/log.js'; import { IMcpHostDelegate, IMcpMessageTransport } from './mcpRegistryTypes.js'; import { McpServerRequestHandler } from './mcpServerRequestHandler.js'; -import { McpCollectionDefinition, IMcpServerConnection, McpServerDefinition, McpConnectionState, McpServerLaunch } from './mcpTypes.js'; +import { IMcpServerConnection, McpCollectionDefinition, McpConnectionState, McpServerDefinition, McpServerLaunch } from './mcpTypes.js'; export class McpServerConnection extends Disposable implements IMcpServerConnection { private readonly _launch = this._register(new MutableDisposable>()); @@ -22,8 +21,6 @@ export class McpServerConnection extends Disposable implements IMcpServerConnect public readonly state: IObservable = this._state; public readonly handler: IObservable = this._requestHandler; - private readonly _loggerId: string; - private readonly _logger: ILogger; private _launchId = 0; constructor( @@ -31,22 +28,10 @@ export class McpServerConnection extends Disposable implements IMcpServerConnect public readonly definition: McpServerDefinition, private readonly _delegate: IMcpHostDelegate, public readonly launchDefinition: McpServerLaunch, - @ILoggerService private readonly _loggerService: ILoggerService, - @IOutputService private readonly _outputService: IOutputService, + private readonly _logger: ILogger, @IInstantiationService private readonly _instantiationService: IInstantiationService, ) { super(); - this._loggerId = `mcpServer/${definition.id}`; - this._logger = this._register(_loggerService.createLogger(this._loggerId, { hidden: true, name: `MCP: ${definition.label}` })); - // If the logger is disposed but not deregistered, then the disposed instance - // is reused and no-ops. todo@sandy081 this seems like a bug. - this._register(toDisposable(() => _loggerService.deregisterLogger(this._loggerId))); - } - - /** @inheritdoc */ - public showOutput(): void { - this._loggerService.setVisibility(this._loggerId, true); - this._outputService.showChannel(this._loggerId); } /** @inheritdoc */ diff --git a/src/vs/workbench/contrib/mcp/common/mcpTypes.ts b/src/vs/workbench/contrib/mcp/common/mcpTypes.ts index 4d94f298ea5..67b1e2a5bcc 100644 --- a/src/vs/workbench/contrib/mcp/common/mcpTypes.ts +++ b/src/vs/workbench/contrib/mcp/common/mcpTypes.ts @@ -314,11 +314,6 @@ export interface IMcpServerConnection extends IDisposable { readonly state: IObservable; readonly handler: IObservable; - /** - * Shows the current server output. - */ - showOutput(): void; - /** * Starts the server if it's stopped. Returns a promise that resolves once * server exits a 'starting' state. diff --git a/src/vs/workbench/contrib/mcp/test/common/mcpRegistry.test.ts b/src/vs/workbench/contrib/mcp/test/common/mcpRegistry.test.ts index 5a33d37f585..bcbbc95a074 100644 --- a/src/vs/workbench/contrib/mcp/test/common/mcpRegistry.test.ts +++ b/src/vs/workbench/contrib/mcp/test/common/mcpRegistry.test.ts @@ -14,7 +14,7 @@ import { ConfigurationTarget } from '../../../../../platform/configuration/commo import { IDialogService } from '../../../../../platform/dialogs/common/dialogs.js'; import { ServiceCollection } from '../../../../../platform/instantiation/common/serviceCollection.js'; import { TestInstantiationService } from '../../../../../platform/instantiation/test/common/instantiationServiceMock.js'; -import { ILoggerService } from '../../../../../platform/log/common/log.js'; +import { ILogger, ILoggerService, NullLogger } from '../../../../../platform/log/common/log.js'; import { IProductService } from '../../../../../platform/product/common/productService.js'; import { ISecretStorageService } from '../../../../../platform/secrets/common/secrets.js'; import { TestSecretStorageService } from '../../../../../platform/secrets/test/common/testSecretStorageService.js'; @@ -120,6 +120,7 @@ suite('Workbench - MCP - Registry', () => { let testDialogService: TestDialogService; let testCollection: McpCollectionDefinition & { serverDefinitions: ISettableObservable }; let baseDefinition: McpServerDefinition; + let logger: ILogger; setup(() => { testConfigResolverService = new TestConfigurationResolverService(); @@ -136,6 +137,8 @@ suite('Workbench - MCP - Registry', () => { [IProductService, {}], ); + logger = new NullLogger(); + const instaService = store.add(new TestInstantiationService(services)); registry = store.add(instaService.createInstance(McpRegistry)); @@ -211,7 +214,7 @@ suite('Workbench - MCP - Registry', () => { testCollection.serverDefinitions.set([definition], undefined); store.add(registry.registerCollection(testCollection)); - const connection = await registry.resolveConnection({ collectionRef: testCollection, definitionRef: definition }) as McpServerConnection; + const connection = await registry.resolveConnection({ collectionRef: testCollection, definitionRef: definition, logger }) as McpServerConnection; assert.ok(connection); assert.strictEqual(connection.definition, definition); @@ -219,7 +222,7 @@ suite('Workbench - MCP - Registry', () => { assert.strictEqual((connection.launchDefinition as any).env.PATH, 'interactiveValue0'); connection.dispose(); - const connection2 = await registry.resolveConnection({ collectionRef: testCollection, definitionRef: definition }) as McpServerConnection; + const connection2 = await registry.resolveConnection({ collectionRef: testCollection, definitionRef: definition, logger }) as McpServerConnection; assert.ok(connection2); assert.strictEqual((connection2.launchDefinition as any).env.PATH, 'interactiveValue0'); @@ -227,7 +230,7 @@ suite('Workbench - MCP - Registry', () => { registry.clearSavedInputs(StorageScope.WORKSPACE); - const connection3 = await registry.resolveConnection({ collectionRef: testCollection, definitionRef: definition }) as McpServerConnection; + const connection3 = await registry.resolveConnection({ collectionRef: testCollection, definitionRef: definition, logger }) as McpServerConnection; assert.ok(connection3); assert.strictEqual((connection3.launchDefinition as any).env.PATH, 'interactiveValue4'); @@ -245,7 +248,7 @@ suite('Workbench - MCP - Registry', () => { store.add(registry.registerCollection(testCollection)); testCollection.serverDefinitions.set([definition], undefined); - const connection = await registry.resolveConnection({ collectionRef: testCollection, definitionRef: definition }); + const connection = await registry.resolveConnection({ collectionRef: testCollection, definitionRef: definition, logger }); assert.ok(connection); assert.strictEqual(testDialogService.promptSpy.called, false); @@ -265,6 +268,7 @@ suite('Workbench - MCP - Registry', () => { testDialogService.setPromptResult(true); const connection = await registry.resolveConnection({ + logger, collectionRef: untrustedCollection, definitionRef: definition }); @@ -275,6 +279,7 @@ suite('Workbench - MCP - Registry', () => { testDialogService.promptSpy.resetHistory(); const connection2 = await registry.resolveConnection({ + logger, collectionRef: untrustedCollection, definitionRef: definition }); @@ -297,6 +302,7 @@ suite('Workbench - MCP - Registry', () => { testDialogService.setPromptResult(false); const connection = await registry.resolveConnection({ + logger, collectionRef: untrustedCollection, definitionRef: definition }); @@ -306,6 +312,7 @@ suite('Workbench - MCP - Registry', () => { testDialogService.promptSpy.resetHistory(); const connection2 = await registry.resolveConnection({ + logger, collectionRef: untrustedCollection, definitionRef: definition }); @@ -327,6 +334,7 @@ suite('Workbench - MCP - Registry', () => { testDialogService.setPromptResult(false); const connection1 = await registry.resolveConnection({ + logger, collectionRef: untrustedCollection, definitionRef: definition }); @@ -337,6 +345,7 @@ suite('Workbench - MCP - Registry', () => { testDialogService.setPromptResult(true); const connection2 = await registry.resolveConnection({ + logger, collectionRef: untrustedCollection, definitionRef: definition, forceTrust: true @@ -348,6 +357,7 @@ suite('Workbench - MCP - Registry', () => { testDialogService.promptSpy.resetHistory(); const connection3 = await registry.resolveConnection({ + logger, collectionRef: untrustedCollection, definitionRef: definition }); diff --git a/src/vs/workbench/contrib/mcp/test/common/mcpServerConnection.test.ts b/src/vs/workbench/contrib/mcp/test/common/mcpServerConnection.test.ts index ab27e7e25a2..90304c2d0a1 100644 --- a/src/vs/workbench/contrib/mcp/test/common/mcpServerConnection.test.ts +++ b/src/vs/workbench/contrib/mcp/test/common/mcpServerConnection.test.ts @@ -12,7 +12,7 @@ import { URI } from '../../../../../base/common/uri.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; import { ServiceCollection } from '../../../../../platform/instantiation/common/serviceCollection.js'; import { TestInstantiationService } from '../../../../../platform/instantiation/test/common/instantiationServiceMock.js'; -import { ILogger, ILoggerService } from '../../../../../platform/log/common/log.js'; +import { ILogger, ILoggerService, NullLogger } from '../../../../../platform/log/common/log.js'; import { IProductService } from '../../../../../platform/product/common/productService.js'; import { IStorageService, StorageScope } from '../../../../../platform/storage/common/storage.js'; import { IOutputService } from '../../../../services/output/common/output.js'; @@ -127,7 +127,8 @@ suite('Workbench - MCP - ServerConnection', () => { collection, serverDefinition, delegate, - serverDefinition.launch + serverDefinition.launch, + new NullLogger(), ); store.add(connection); @@ -154,7 +155,8 @@ suite('Workbench - MCP - ServerConnection', () => { collection, serverDefinition, delegate, - serverDefinition.launch + serverDefinition.launch, + new NullLogger(), ); store.add(connection); @@ -172,7 +174,8 @@ suite('Workbench - MCP - ServerConnection', () => { collection, serverDefinition, delegate, - serverDefinition.launch + serverDefinition.launch, + new NullLogger(), ); store.add(connection); @@ -197,7 +200,8 @@ suite('Workbench - MCP - ServerConnection', () => { collection, serverDefinition, delegate, - serverDefinition.launch + serverDefinition.launch, + new NullLogger(), ); store.add(connection); @@ -220,7 +224,8 @@ suite('Workbench - MCP - ServerConnection', () => { collection, serverDefinition, delegate, - serverDefinition.launch + serverDefinition.launch, + new NullLogger(), ); store.add(connection); @@ -251,7 +256,8 @@ suite('Workbench - MCP - ServerConnection', () => { collection, serverDefinition, delegate, - serverDefinition.launch + serverDefinition.launch, + new NullLogger(), ); // Start the connection @@ -265,81 +271,24 @@ suite('Workbench - MCP - ServerConnection', () => { assert.strictEqual(connection.state.get().state, McpConnectionState.Kind.Stopped); }); - test('showOutput should call logger and output services', () => { - let channelShown = false; - const outputService = { - showChannel: (id: string) => { - assert.strictEqual(id, `mcpServer/${serverDefinition.id}`); - channelShown = true; - } - }; - - let loggerVisible = false; - const loggerService = new class extends TestLoggerService { - override setVisibility(id: string, visible: boolean): void { - assert.strictEqual(id, `mcpServer/${serverDefinition.id}`); - assert.strictEqual(visible, true); - loggerVisible = true; - } - }; - - // Override services - const services = new ServiceCollection( - [ILoggerService, store.add(loggerService)], - [IOutputService, upcast(outputService)], - [IStorageService, store.add(new TestStorageService())] - ); - - const localInstantiationService = store.add(new TestInstantiationService(services)); - - // Create server connection - const connection = localInstantiationService.createInstance( - McpServerConnection, - collection, - serverDefinition, - delegate, - serverDefinition.launch - ); - store.add(connection); - - // Show output - connection.showOutput(); - - assert.strictEqual(channelShown, true); - assert.strictEqual(loggerVisible, true); - }); - test('should log transport messages', async () => { // Track logged messages const loggedMessages: string[] = []; - const loggerService = new class extends TestLoggerService { - override createLogger(id: string) { - return { - info: (message: string) => { - loggedMessages.push(message); - }, - error: () => { }, - dispose: () => { } - } as Partial as ILogger; - } - }; - - // Override services - const services = new ServiceCollection( - [ILoggerService, store.add(loggerService)], - [IOutputService, upcast({ showChannel: () => { } })], - [IStorageService, store.add(new TestStorageService())] - ); - - const localInstantiationService = store.add(new TestInstantiationService(services)); // Create server connection - const connection = localInstantiationService.createInstance( + const connection = instantiationService.createInstance( McpServerConnection, collection, serverDefinition, delegate, - serverDefinition.launch + serverDefinition.launch, + { + info: (message: string) => { + loggedMessages.push(message); + }, + error: () => { }, + dispose: () => { } + } as Partial as ILogger, ); store.add(connection); @@ -355,6 +304,9 @@ suite('Workbench - MCP - ServerConnection', () => { // Check that the message was logged assert.ok(loggedMessages.some(msg => msg === 'Test log message')); + + connection.dispose(); + await timeout(10); }); test('should correctly handle transitions to and from error state', async () => { @@ -364,7 +316,8 @@ suite('Workbench - MCP - ServerConnection', () => { collection, serverDefinition, delegate, - serverDefinition.launch + serverDefinition.launch, + new NullLogger(), ); store.add(connection); @@ -401,7 +354,8 @@ suite('Workbench - MCP - ServerConnection', () => { collection, serverDefinition, delegate, - serverDefinition.launch + serverDefinition.launch, + new NullLogger(), ); store.add(connection); From 2b7ffdea69fd76e6e966043d53919d46332b234f Mon Sep 17 00:00:00 2001 From: Rob Lourens Date: Wed, 19 Mar 2025 18:49:50 -0700 Subject: [PATCH 4/6] Enable editing files that are open, even if they are outside the workspace (#244071) --- .../contrib/chat/common/tools/editFileTool.ts | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/vs/workbench/contrib/chat/common/tools/editFileTool.ts b/src/vs/workbench/contrib/chat/common/tools/editFileTool.ts index b6384cf0ad5..ca087fb21f8 100644 --- a/src/vs/workbench/contrib/chat/common/tools/editFileTool.ts +++ b/src/vs/workbench/contrib/chat/common/tools/editFileTool.ts @@ -7,11 +7,13 @@ import { CancellationToken } from '../../../../../base/common/cancellation.js'; import { MarkdownString } from '../../../../../base/common/htmlContent.js'; import { IDisposable } from '../../../../../base/common/lifecycle.js'; import { autorun } from '../../../../../base/common/observable.js'; +import { isEqual } from '../../../../../base/common/resources.js'; import { URI, UriComponents } from '../../../../../base/common/uri.js'; import { generateUuid } from '../../../../../base/common/uuid.js'; import { localize } from '../../../../../nls.js'; import { IWorkspaceContextService } from '../../../../../platform/workspace/common/workspace.js'; import { SaveReason } from '../../../../common/editor.js'; +import { GroupsOrder, IEditorGroupsService } from '../../../../services/editor/common/editorGroupsService.js'; import { ITextFileService } from '../../../../services/textfile/common/textfiles.js'; import { CellUri } from '../../../notebook/common/notebookCommon.js'; import { INotebookService } from '../../../notebook/common/notebookService.js'; @@ -79,6 +81,7 @@ export class EditTool implements IToolImpl { @ILanguageModelIgnoredFilesService private readonly ignoredFilesService: ILanguageModelIgnoredFilesService, @ITextFileService private readonly textFileService: ITextFileService, @INotebookService private readonly notebookService: INotebookService, + @IEditorGroupsService private readonly editorGroupsService: IEditorGroupsService, ) { } async invoke(invocation: IToolInvocation, countTokens: CountTokensCallback, token: CancellationToken): Promise { @@ -88,8 +91,18 @@ export class EditTool implements IToolImpl { const parameters = invocation.parameters as EditToolParams; const uri = URI.revive(parameters.file); // TODO@roblourens do revive in MainThreadLanguageModelTools + if (!this.workspaceContextService.isInsideWorkspace(uri)) { - throw new Error(`File ${uri.fsPath} can't be edited because it's not inside the current workspace`); + const groupsByLastActive = this.editorGroupsService.getGroups(GroupsOrder.MOST_RECENTLY_ACTIVE); + const uriIsOpenInSomeEditor = groupsByLastActive.some((group) => { + return group.editors.some((editor) => { + return isEqual(editor.resource, uri); + }); + }); + + if (!uriIsOpenInSomeEditor) { + throw new Error(`File ${uri.fsPath} can't be edited because it's not inside the current workspace`); + } } if (await this.ignoredFilesService.fileIsIgnored(uri, token)) { From 8dd2ad65d44bb52b8d7a55880588f1c82708254f Mon Sep 17 00:00:00 2001 From: Robo Date: Thu, 20 Mar 2025 12:10:07 +0900 Subject: [PATCH 5/6] refactor: setup errorTelemetry from electron-main (#244020) --- src/vs/code/electron-main/app.ts | 33 ++-------------- .../telemetry/electron-main/errorTelemetry.ts | 38 +++++++++++++++++++ src/vs/workbench/electron-sandbox/window.ts | 8 ---- 3 files changed, 42 insertions(+), 37 deletions(-) create mode 100644 src/vs/platform/telemetry/electron-main/errorTelemetry.ts diff --git a/src/vs/code/electron-main/app.ts b/src/vs/code/electron-main/app.ts index fbe457fd459..3fb0f3a0e78 100644 --- a/src/vs/code/electron-main/app.ts +++ b/src/vs/code/electron-main/app.ts @@ -9,7 +9,6 @@ import { validatedIpcMain } from '../../base/parts/ipc/electron-main/ipcMain.js' import { hostname, release } from 'os'; import { VSBuffer } from '../../base/common/buffer.js'; import { toErrorMessage } from '../../base/common/errorMessage.js'; -import { isSigPipeError, onUnexpectedError, setUnexpectedErrorHandler } from '../../base/common/errors.js'; import { Event } from '../../base/common/event.js'; import { parse } from '../../base/common/jsonc.js'; import { getPathLabel } from '../../base/common/labels.js'; @@ -122,6 +121,7 @@ import { INativeMcpDiscoveryHelperService, NativeMcpDiscoveryHelperChannelName } import { NativeMcpDiscoveryHelperService } from '../../platform/mcp/node/nativeMcpDiscoveryHelperService.js'; import { IWebContentExtractorService } from '../../platform/webContentExtractor/common/webContentExtractor.js'; import { NativeWebContentExtractorService } from '../../platform/webContentExtractor/electron-main/webContentExtractorService.js'; +import ErrorTelemetry from '../../platform/telemetry/electron-main/errorTelemetry.js'; /** * The main VS Code application. There will only ever be one instance, @@ -373,15 +373,6 @@ export class CodeApplication extends Disposable { private registerListeners(): void { - // We handle uncaught exceptions here to prevent electron from opening a dialog to the user - setUnexpectedErrorHandler(error => this.onUnexpectedError(error)); - process.on('uncaughtException', error => { - if (!isSigPipeError(error)) { - onUnexpectedError(error); - } - }); - process.on('unhandledRejection', (reason: unknown) => onUnexpectedError(reason)); - // Dispose on shutdown Event.once(this.lifecycleMainService.onWillShutdown)(() => this.dispose()); @@ -528,25 +519,6 @@ export class CodeApplication extends Disposable { //#endregion } - private onUnexpectedError(error: Error): void { - if (error) { - - // take only the message and stack property - const friendlyError = { - message: `[uncaught exception in main]: ${error.message}`, - stack: error.stack - }; - - // handle on client side - this.windowsMainService?.sendToFocused('vscode:reportError', JSON.stringify(friendlyError)); - } - - this.logService.error(`[uncaught exception in main]: ${error}`); - if (error.stack) { - this.logService.error(error.stack); - } - } - async startup(): Promise { this.logService.debug('Starting VS Code'); this.logService.debug(`from: ${this.environmentMainService.appRoot}`); @@ -603,6 +575,9 @@ export class CodeApplication extends Disposable { // Services const appInstantiationService = await this.initServices(machineId, sqmId, devDeviceId, sharedProcessReady); + // Error telemetry + appInstantiationService.invokeFunction(accessor => this._register(new ErrorTelemetry(accessor.get(ILogService), accessor.get(ITelemetryService)))); + // Auth Handler appInstantiationService.invokeFunction(accessor => accessor.get(IProxyAuthService)); diff --git a/src/vs/platform/telemetry/electron-main/errorTelemetry.ts b/src/vs/platform/telemetry/electron-main/errorTelemetry.ts new file mode 100644 index 00000000000..e093fda1474 --- /dev/null +++ b/src/vs/platform/telemetry/electron-main/errorTelemetry.ts @@ -0,0 +1,38 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { isSigPipeError, onUnexpectedError, setUnexpectedErrorHandler } from '../../../base/common/errors.js'; +import BaseErrorTelemetry from '../common/errorTelemetry.js'; +import { ITelemetryService } from '../common/telemetry.js'; +import { ILogService } from '../../../platform/log/common/log.js'; + +export default class ErrorTelemetry extends BaseErrorTelemetry { + constructor( + private readonly logService: ILogService, + @ITelemetryService telemetryService: ITelemetryService + ) { + super(telemetryService); + } + + protected override installErrorListeners(): void { + // We handle uncaught exceptions here to prevent electron from opening a dialog to the user + setUnexpectedErrorHandler(error => this.onUnexpectedError(error)); + + process.on('uncaughtException', error => { + if (!isSigPipeError(error)) { + onUnexpectedError(error); + } + }); + + process.on('unhandledRejection', (reason: unknown) => onUnexpectedError(reason)); + } + + private onUnexpectedError(error: Error): void { + this.logService.error(`[uncaught exception in main]: ${error}`); + if (error.stack) { + this.logService.error(error.stack); + } + } +} diff --git a/src/vs/workbench/electron-sandbox/window.ts b/src/vs/workbench/electron-sandbox/window.ts index 1c187b2948e..d306dbe5bdc 100644 --- a/src/vs/workbench/electron-sandbox/window.ts +++ b/src/vs/workbench/electron-sandbox/window.ts @@ -6,7 +6,6 @@ import './media/window.css'; import { localize } from '../../nls.js'; import { URI } from '../../base/common/uri.js'; -import { onUnexpectedError } from '../../base/common/errors.js'; import { equals } from '../../base/common/objects.js'; import { EventType, EventHelper, addDisposableListener, ModifierKeyEmitter, getActiveElement, hasWindow, getWindowById, getWindows, $ } from '../../base/browser/dom.js'; import { Action, Separator, WorkbenchActionExecutedClassification, WorkbenchActionExecutedEvent } from '../../base/common/actions.js'; @@ -187,13 +186,6 @@ export class NativeWindow extends BaseWindow { } }); - // Error reporting from main - ipcRenderer.on('vscode:reportError', (event: unknown, error: string) => { - if (error) { - onUnexpectedError(JSON.parse(error)); - } - }); - // Shared Process crash reported from main ipcRenderer.on('vscode:reportSharedProcessCrash', (event: unknown, error: string) => { this.notificationService.prompt( From cc72ba213206db6abe4e3582e2152e7ba8ae2d02 Mon Sep 17 00:00:00 2001 From: Aaron Munger <2019016+amunger@users.noreply.github.com> Date: Wed, 19 Mar 2025 21:28:29 -0700 Subject: [PATCH 6/6] split output and markup decoration (#244076) Co-authored-by: amunger <> --- .../notebook/browser/diff/notebookDiffEditor.ts | 4 ++-- .../contrib/notebook/browser/notebookBrowser.ts | 2 +- .../notebook/browser/notebookEditorWidget.ts | 16 ++++++++++------ .../browser/view/cellParts/cellDecorations.ts | 4 ++-- .../browser/view/renderers/backLayerWebView.ts | 12 +++++++++++- .../browser/view/renderers/webviewMessages.ts | 2 +- .../browser/view/renderers/webviewPreloads.ts | 10 ++++++++++ 7 files changed, 37 insertions(+), 13 deletions(-) diff --git a/src/vs/workbench/contrib/notebook/browser/diff/notebookDiffEditor.ts b/src/vs/workbench/contrib/notebook/browser/diff/notebookDiffEditor.ts index 639e2fcd80d..45f4aa68cfe 100644 --- a/src/vs/workbench/contrib/notebook/browser/diff/notebookDiffEditor.ts +++ b/src/vs/workbench/contrib/notebook/browser/diff/notebookDiffEditor.ts @@ -929,9 +929,9 @@ export class NotebookTextDiffEditor extends EditorPane implements INotebookTextD deltaCellOutputContainerClassNames(diffSide: DiffSide, cellId: string, added: string[], removed: string[]) { if (diffSide === DiffSide.Original) { - this._originalWebview?.deltaCellContainerClassNames(cellId, added, removed); + this._originalWebview?.deltaCellOutputContainerClassNames(cellId, added, removed); } else { - this._modifiedWebview?.deltaCellContainerClassNames(cellId, added, removed); + this._modifiedWebview?.deltaCellOutputContainerClassNames(cellId, added, removed); } } diff --git a/src/vs/workbench/contrib/notebook/browser/notebookBrowser.ts b/src/vs/workbench/contrib/notebook/browser/notebookBrowser.ts index b37aaca062a..e89235ff0f4 100644 --- a/src/vs/workbench/contrib/notebook/browser/notebookBrowser.ts +++ b/src/vs/workbench/contrib/notebook/browser/notebookBrowser.ts @@ -807,7 +807,7 @@ export interface INotebookEditorDelegate extends INotebookEditor { * Hide the inset in the webview layer without removing it */ hideInset(output: IDisplayOutputViewModel): void; - deltaCellContainerClassNames(cellId: string, added: string[], removed: string[]): void; + deltaCellContainerClassNames(cellId: string, added: string[], removed: string[], cellKind: CellKind): void; } export interface IActiveNotebookEditorDelegate extends INotebookEditorDelegate { diff --git a/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts b/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts index 9e0b6f8cf18..db11fe5dead 100644 --- a/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts +++ b/src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts @@ -1617,21 +1617,21 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD store.add(cell.onCellDecorationsChanged(e => { e.added.forEach(options => { if (options.className) { - this.deltaCellContainerClassNames(cell.id, [options.className], []); + this.deltaCellContainerClassNames(cell.id, [options.className], [], cell.cellKind); } if (options.outputClassName) { - this.deltaCellContainerClassNames(cell.id, [options.outputClassName], []); + this.deltaCellContainerClassNames(cell.id, [options.outputClassName], [], cell.cellKind); } }); e.removed.forEach(options => { if (options.className) { - this.deltaCellContainerClassNames(cell.id, [], [options.className]); + this.deltaCellContainerClassNames(cell.id, [], [options.className], cell.cellKind); } if (options.outputClassName) { - this.deltaCellContainerClassNames(cell.id, [], [options.outputClassName]); + this.deltaCellContainerClassNames(cell.id, [], [options.outputClassName], cell.cellKind); } }); })); @@ -2285,8 +2285,12 @@ export class NotebookEditorWidget extends Disposable implements INotebookEditorD return ret; } - deltaCellContainerClassNames(cellId: string, added: string[], removed: string[]) { - this._webview?.deltaCellContainerClassNames(cellId, added, removed); + deltaCellContainerClassNames(cellId: string, added: string[], removed: string[], cellkind: CellKind): void { + if (cellkind === CellKind.Markup) { + this._webview?.deltaMarkupPreviewClassNames(cellId, added, removed); + } else { + this._webview?.deltaCellOutputContainerClassNames(cellId, added, removed); + } } changeModelDecorations(callback: (changeAccessor: IModelDecorationsChangeAccessor) => T): T | null { diff --git a/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellDecorations.ts b/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellDecorations.ts index 9068b002c64..4ae7534705b 100644 --- a/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellDecorations.ts +++ b/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellDecorations.ts @@ -72,11 +72,11 @@ export class CellDecorations extends CellContentPart { this.currentCell.getCellDecorations().forEach(options => { if (options.className && this.currentCell) { this.rootContainer.classList.add(options.className); - this.notebookEditor.deltaCellContainerClassNames(this.currentCell.id, [options.className], []); + this.notebookEditor.deltaCellContainerClassNames(this.currentCell.id, [options.className], [], this.currentCell.cellKind); } if (options.outputClassName && this.currentCell) { - this.notebookEditor.deltaCellContainerClassNames(this.currentCell.id, [options.outputClassName], []); + this.notebookEditor.deltaCellContainerClassNames(this.currentCell.id, [options.outputClassName], [], this.currentCell.cellKind); } }); } diff --git a/src/vs/workbench/contrib/notebook/browser/view/renderers/backLayerWebView.ts b/src/vs/workbench/contrib/notebook/browser/view/renderers/backLayerWebView.ts index 28c9dbf7774..b6c10143b39 100644 --- a/src/vs/workbench/contrib/notebook/browser/view/renderers/backLayerWebView.ts +++ b/src/vs/workbench/contrib/notebook/browser/view/renderers/backLayerWebView.ts @@ -1855,14 +1855,24 @@ export class BackLayerWebView extends Themable { } - deltaCellContainerClassNames(cellId: string, added: string[], removed: string[]) { + deltaCellOutputContainerClassNames(cellId: string, added: string[], removed: string[]) { this._sendMessageToWebview({ type: 'decorations', cellId, addedClassNames: added, removedClassNames: removed }); + } + deltaMarkupPreviewClassNames(cellId: string, added: string[], removed: string[]) { + if (this.markupPreviewMapping.get(cellId)) { + this._sendMessageToWebview({ + type: 'markupDecorations', + cellId, + addedClassNames: added, + removedClassNames: removed + }); + } } updateOutputRenderers() { diff --git a/src/vs/workbench/contrib/notebook/browser/view/renderers/webviewMessages.ts b/src/vs/workbench/contrib/notebook/browser/view/renderers/webviewMessages.ts index 073737f5b79..26149b87ca1 100644 --- a/src/vs/workbench/contrib/notebook/browser/view/renderers/webviewMessages.ts +++ b/src/vs/workbench/contrib/notebook/browser/view/renderers/webviewMessages.ts @@ -305,7 +305,7 @@ export interface IUpdateRenderersMessage { } export interface IUpdateDecorationsMessage { - readonly type: 'decorations'; + readonly type: 'decorations' | 'markupDecorations'; readonly cellId: string; readonly addedClassNames: readonly string[]; readonly removedClassNames: readonly string[]; diff --git a/src/vs/workbench/contrib/notebook/browser/view/renderers/webviewPreloads.ts b/src/vs/workbench/contrib/notebook/browser/view/renderers/webviewPreloads.ts index 5b7077bf1d1..ac3d98fd4e3 100644 --- a/src/vs/workbench/contrib/notebook/browser/view/renderers/webviewPreloads.ts +++ b/src/vs/workbench/contrib/notebook/browser/view/renderers/webviewPreloads.ts @@ -1780,6 +1780,16 @@ async function webviewPreloads(ctx: PreloadContext) { outputContainer?.classList.remove(...event.data.removedClassNames); break; } + case 'markupDecorations': { + const markupCell = window.document.getElementById(event.data.cellId); + // The cell may not have been added yet if it is out of view. + // Decorations will be added when the cell is shown. + if (markupCell) { + markupCell?.classList.add(...event.data.addedClassNames); + markupCell?.classList.remove(...event.data.removedClassNames); + } + break; + } case 'customKernelMessage': onDidReceiveKernelMessage.fire(event.data.message); break;