From eceda36ddd8f0fb068b4b5b02fd54b24cdf55cb4 Mon Sep 17 00:00:00 2001 From: hannesrudolph Date: Sat, 9 Aug 2025 11:06:23 -0600 Subject: [PATCH 1/4] Revert "fix: prevent unnecessary MCP server refresh on settings save (#6772) (#6779)" This reverts commit 8d05bc179b23445629139fe1654ac008013337da. --- .../__tests__/webviewMessageHandler.spec.ts | 148 ------------------ src/core/webview/webviewMessageHandler.ts | 15 +- 2 files changed, 4 insertions(+), 159 deletions(-) diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index 8e61f3f0d97..9a1683e4641 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -35,7 +35,6 @@ const mockClineProvider = { getCurrentCline: vi.fn(), getTaskWithId: vi.fn(), initClineWithHistoryItem: vi.fn(), - getMcpHub: vi.fn(), } as unknown as ClineProvider import { t } from "../../../i18n" @@ -577,150 +576,3 @@ describe("webviewMessageHandler - message dialog preferences", () => { }) }) }) - -describe("webviewMessageHandler - mcpEnabled", () => { - let mockMcpHub: any - - beforeEach(() => { - vi.clearAllMocks() - - // Create a mock McpHub instance - mockMcpHub = { - handleMcpEnabledChange: vi.fn().mockResolvedValue(undefined), - } - - // Mock the getMcpHub method to return our mock McpHub - mockClineProvider.getMcpHub = vi.fn().mockReturnValue(mockMcpHub) - - // Reset the contextProxy getValue mock - vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(undefined) - }) - - it("should not refresh MCP servers when value does not change (true to true)", async () => { - // Setup: mcpEnabled is already true - vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(true) - - // Act: Send mcpEnabled message with same value - await webviewMessageHandler(mockClineProvider, { - type: "mcpEnabled", - bool: true, - }) - - // Assert: handleMcpEnabledChange should not be called - expect(mockMcpHub.handleMcpEnabledChange).not.toHaveBeenCalled() - expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("mcpEnabled", true) - expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() - }) - - it("should not refresh MCP servers when value does not change (false to false)", async () => { - // Setup: mcpEnabled is already false - vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(false) - - // Act: Send mcpEnabled message with same value - await webviewMessageHandler(mockClineProvider, { - type: "mcpEnabled", - bool: false, - }) - - // Assert: handleMcpEnabledChange should not be called - expect(mockMcpHub.handleMcpEnabledChange).not.toHaveBeenCalled() - expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("mcpEnabled", false) - expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() - }) - - it("should refresh MCP servers when value changes from true to false", async () => { - // Setup: mcpEnabled is true - vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(true) - - // Act: Send mcpEnabled message with false - await webviewMessageHandler(mockClineProvider, { - type: "mcpEnabled", - bool: false, - }) - - // Assert: handleMcpEnabledChange should be called - expect(mockMcpHub.handleMcpEnabledChange).toHaveBeenCalledWith(false) - expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("mcpEnabled", false) - expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() - }) - - it("should refresh MCP servers when value changes from false to true", async () => { - // Setup: mcpEnabled is false - vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(false) - - // Act: Send mcpEnabled message with true - await webviewMessageHandler(mockClineProvider, { - type: "mcpEnabled", - bool: true, - }) - - // Assert: handleMcpEnabledChange should be called - expect(mockMcpHub.handleMcpEnabledChange).toHaveBeenCalledWith(true) - expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("mcpEnabled", true) - expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() - }) - - it("should handle undefined values with defaults correctly", async () => { - // Setup: mcpEnabled is undefined (defaults to true) - vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(undefined) - - // Act: Send mcpEnabled message with undefined (defaults to true) - await webviewMessageHandler(mockClineProvider, { - type: "mcpEnabled", - bool: undefined, - }) - - // Assert: Should use default value (true) and not trigger refresh since both are true - expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("mcpEnabled", true) - expect(mockMcpHub.handleMcpEnabledChange).not.toHaveBeenCalled() - expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() - }) - - it("should handle when mcpEnabled changes from undefined to false", async () => { - // Setup: mcpEnabled is undefined (defaults to true) - vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(undefined) - - // Act: Send mcpEnabled message with false - await webviewMessageHandler(mockClineProvider, { - type: "mcpEnabled", - bool: false, - }) - - // Assert: Should trigger refresh since undefined defaults to true and we're changing to false - expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("mcpEnabled", false) - expect(mockMcpHub.handleMcpEnabledChange).toHaveBeenCalledWith(false) - expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() - }) - - it("should not call handleMcpEnabledChange when McpHub is not available", async () => { - // Setup: No McpHub instance available - mockClineProvider.getMcpHub = vi.fn().mockReturnValue(null) - vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(true) - - // Act: Send mcpEnabled message with false - await webviewMessageHandler(mockClineProvider, { - type: "mcpEnabled", - bool: false, - }) - - // Assert: State should be updated but handleMcpEnabledChange should not be called - expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("mcpEnabled", false) - expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() - // No error should be thrown - }) - - it("should always update state even when value doesn't change", async () => { - // Setup: mcpEnabled is true - vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(true) - - // Act: Send mcpEnabled message with same value - await webviewMessageHandler(mockClineProvider, { - type: "mcpEnabled", - bool: true, - }) - - // Assert: State should still be updated to ensure consistency - expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("mcpEnabled", true) - expect(mockClineProvider.postStateToWebview).toHaveBeenCalled() - }) -}) diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index e2c6d6a475b..f5dc6a467f1 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -900,19 +900,12 @@ export const webviewMessageHandler = async ( } case "mcpEnabled": const mcpEnabled = message.bool ?? true - const currentMcpEnabled = getGlobalState("mcpEnabled") ?? true - - // Always update the state to ensure consistency await updateGlobalState("mcpEnabled", mcpEnabled) - // Only refresh MCP connections if the value actually changed - // This prevents expensive MCP server refresh operations when saving unrelated settings - if (currentMcpEnabled !== mcpEnabled) { - // Delegate MCP enable/disable logic to McpHub - const mcpHubInstance = provider.getMcpHub() - if (mcpHubInstance) { - await mcpHubInstance.handleMcpEnabledChange(mcpEnabled) - } + // Delegate MCP enable/disable logic to McpHub + const mcpHubInstance = provider.getMcpHub() + if (mcpHubInstance) { + await mcpHubInstance.handleMcpEnabledChange(mcpEnabled) } await provider.postStateToWebview() From 949c90fcfa1d46432c3818b2709213c67613a776 Mon Sep 17 00:00:00 2001 From: hannesrudolph Date: Sat, 9 Aug 2025 11:26:04 -0600 Subject: [PATCH 2/4] fix(mcp): Revert changes causing startup issues and temporarily disable notifications - Reverted PR #6779 which prevented unnecessary MCP server refreshes but caused startup failures - Temporarily disabled MCP notification popups as a stopgap solution - Added TODO comments explaining the temporary nature of disabled notifications - This allows MCP servers to function properly while a more robust solution is developed --- src/services/mcp/McpHub.ts | 41 ++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index 646236b5d56..25e5a017e25 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -1212,7 +1212,10 @@ export class McpHub { public async refreshAllConnections(): Promise { if (this.isConnecting) { - vscode.window.showInformationMessage(t("mcp:info.already_refreshing")) + // TODO: This notification is temporarily disabled as a stopgap until we can separate + // MCP settings from regular settings, allowing them to be saved independently without + // triggering unnecessary MCP server refreshes + // Disabled notification: vscode.window.showInformationMessage(t("mcp:info.already_refreshing")) return } @@ -1234,7 +1237,10 @@ export class McpHub { } this.isConnecting = true - vscode.window.showInformationMessage(t("mcp:info.refreshing_all")) + // TODO: This notification is temporarily disabled as a stopgap until we can separate + // MCP settings from regular settings, allowing them to be saved independently without + // triggering unnecessary MCP server refreshes + // Disabled notification: vscode.window.showInformationMessage(t("mcp:info.refreshing_all")) try { const globalPath = await this.getMcpSettingsFilePath() @@ -1244,11 +1250,14 @@ export class McpHub { const globalConfig = JSON.parse(globalContent) globalServers = globalConfig.mcpServers || {} const globalServerNames = Object.keys(globalServers) - vscode.window.showInformationMessage( - t("mcp:info.global_servers_active", { - mcpServers: `${globalServerNames.join(", ") || "none"}`, - }), - ) + // TODO: This notification is temporarily disabled as a stopgap until we can separate + // MCP settings from regular settings, allowing them to be saved independently without + // triggering unnecessary MCP server refreshes + // Disabled notification: vscode.window.showInformationMessage( + // t("mcp:info.global_servers_active", { + // mcpServers: `${globalServerNames.join(", ") || "none"}`, + // }), + // ) } catch (error) { console.log("Error reading global MCP config:", error) } @@ -1261,11 +1270,14 @@ export class McpHub { const projectConfig = JSON.parse(projectContent) projectServers = projectConfig.mcpServers || {} const projectServerNames = Object.keys(projectServers) - vscode.window.showInformationMessage( - t("mcp:info.project_servers_active", { - mcpServers: `${projectServerNames.join(", ") || "none"}`, - }), - ) + // TODO: This notification is temporarily disabled as a stopgap until we can separate + // MCP settings from regular settings, allowing them to be saved independently without + // triggering unnecessary MCP server refreshes + // Disabled notification: vscode.window.showInformationMessage( + // t("mcp:info.project_servers_active", { + // mcpServers: `${projectServerNames.join(", ") || "none"}`, + // }), + // ) } catch (error) { console.log("Error reading project MCP config:", error) } @@ -1286,7 +1298,10 @@ export class McpHub { await this.notifyWebviewOfServerChanges() - vscode.window.showInformationMessage(t("mcp:info.all_refreshed")) + // TODO: This notification is temporarily disabled as a stopgap until we can separate + // MCP settings from regular settings, allowing them to be saved independently without + // triggering unnecessary MCP server refreshes + // Disabled notification: vscode.window.showInformationMessage(t("mcp:info.all_refreshed")) } catch (error) { this.showErrorMessage("Failed to refresh MCP servers", error) } finally { From 57a7bdadd5e8b35c0796863b615f4a491bdfffbb Mon Sep 17 00:00:00 2001 From: hannesrudolph Date: Sat, 9 Aug 2025 11:47:30 -0600 Subject: [PATCH 3/4] test(mcp): restore mcpEnabled toggle coverage to verify delegation to McpHub --- .../__tests__/webviewMessageHandler.spec.ts | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index 9a1683e4641..7ba7128bb95 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -576,3 +576,55 @@ describe("webviewMessageHandler - message dialog preferences", () => { }) }) }) + +describe("webviewMessageHandler - mcpEnabled", () => { + let mockMcpHub: any + + beforeEach(() => { + vi.clearAllMocks() + + // Create a mock McpHub instance + mockMcpHub = { + handleMcpEnabledChange: vi.fn().mockResolvedValue(undefined), + } + + // Ensure provider exposes getMcpHub and returns our mock + ;(mockClineProvider as any).getMcpHub = vi.fn().mockReturnValue(mockMcpHub) + }) + + it("delegates enable=true to McpHub and posts updated state", async () => { + await webviewMessageHandler(mockClineProvider, { + type: "mcpEnabled", + bool: true, + }) + + expect((mockClineProvider as any).getMcpHub).toHaveBeenCalledTimes(1) + expect(mockMcpHub.handleMcpEnabledChange).toHaveBeenCalledTimes(1) + expect(mockMcpHub.handleMcpEnabledChange).toHaveBeenCalledWith(true) + expect(mockClineProvider.postStateToWebview).toHaveBeenCalledTimes(1) + }) + + it("delegates enable=false to McpHub and posts updated state", async () => { + await webviewMessageHandler(mockClineProvider, { + type: "mcpEnabled", + bool: false, + }) + + expect((mockClineProvider as any).getMcpHub).toHaveBeenCalledTimes(1) + expect(mockMcpHub.handleMcpEnabledChange).toHaveBeenCalledTimes(1) + expect(mockMcpHub.handleMcpEnabledChange).toHaveBeenCalledWith(false) + expect(mockClineProvider.postStateToWebview).toHaveBeenCalledTimes(1) + }) + + it("handles missing McpHub instance gracefully and still posts state", async () => { + ;(mockClineProvider as any).getMcpHub = vi.fn().mockReturnValue(undefined) + + await webviewMessageHandler(mockClineProvider, { + type: "mcpEnabled", + bool: true, + }) + + expect((mockClineProvider as any).getMcpHub).toHaveBeenCalledTimes(1) + expect(mockClineProvider.postStateToWebview).toHaveBeenCalledTimes(1) + }) +}) From d0f587442b8071b7df1b1664f1f65adda2e13674 Mon Sep 17 00:00:00 2001 From: hannesrudolph Date: Sat, 9 Aug 2025 11:56:17 -0600 Subject: [PATCH 4/4] refactor(mcp): remove info notifications during refresh; rely on UI indicator --- src/services/mcp/McpHub.ts | 29 ----------------------------- 1 file changed, 29 deletions(-) diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index 25e5a017e25..271c6e1fb3f 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -1212,10 +1212,6 @@ export class McpHub { public async refreshAllConnections(): Promise { if (this.isConnecting) { - // TODO: This notification is temporarily disabled as a stopgap until we can separate - // MCP settings from regular settings, allowing them to be saved independently without - // triggering unnecessary MCP server refreshes - // Disabled notification: vscode.window.showInformationMessage(t("mcp:info.already_refreshing")) return } @@ -1237,10 +1233,6 @@ export class McpHub { } this.isConnecting = true - // TODO: This notification is temporarily disabled as a stopgap until we can separate - // MCP settings from regular settings, allowing them to be saved independently without - // triggering unnecessary MCP server refreshes - // Disabled notification: vscode.window.showInformationMessage(t("mcp:info.refreshing_all")) try { const globalPath = await this.getMcpSettingsFilePath() @@ -1250,14 +1242,6 @@ export class McpHub { const globalConfig = JSON.parse(globalContent) globalServers = globalConfig.mcpServers || {} const globalServerNames = Object.keys(globalServers) - // TODO: This notification is temporarily disabled as a stopgap until we can separate - // MCP settings from regular settings, allowing them to be saved independently without - // triggering unnecessary MCP server refreshes - // Disabled notification: vscode.window.showInformationMessage( - // t("mcp:info.global_servers_active", { - // mcpServers: `${globalServerNames.join(", ") || "none"}`, - // }), - // ) } catch (error) { console.log("Error reading global MCP config:", error) } @@ -1270,14 +1254,6 @@ export class McpHub { const projectConfig = JSON.parse(projectContent) projectServers = projectConfig.mcpServers || {} const projectServerNames = Object.keys(projectServers) - // TODO: This notification is temporarily disabled as a stopgap until we can separate - // MCP settings from regular settings, allowing them to be saved independently without - // triggering unnecessary MCP server refreshes - // Disabled notification: vscode.window.showInformationMessage( - // t("mcp:info.project_servers_active", { - // mcpServers: `${projectServerNames.join(", ") || "none"}`, - // }), - // ) } catch (error) { console.log("Error reading project MCP config:", error) } @@ -1297,11 +1273,6 @@ export class McpHub { await delay(100) await this.notifyWebviewOfServerChanges() - - // TODO: This notification is temporarily disabled as a stopgap until we can separate - // MCP settings from regular settings, allowing them to be saved independently without - // triggering unnecessary MCP server refreshes - // Disabled notification: vscode.window.showInformationMessage(t("mcp:info.all_refreshed")) } catch (error) { this.showErrorMessage("Failed to refresh MCP servers", error) } finally {