Skip to content

Commit 75b861c

Browse files
hannesrudolphhannesrudolph
authored
fix(mcp): Revert changes causing startup issues and remove refresh notifications (#6878)
* Revert "fix: prevent unnecessary MCP server refresh on settings save (#6772) (#6779)" This reverts commit 8d05bc1. * 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 * test(mcp): restore mcpEnabled toggle coverage to verify delegation to McpHub * refactor(mcp): remove info notifications during refresh; rely on UI indicator
1 parent ad0e33e commit 75b861c

File tree

3 files changed

+18
-135
lines changed

3 files changed

+18
-135
lines changed

‎src/core/webview/__tests__/webviewMessageHandler.spec.ts‎

Lines changed: 14 additions & 110 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@ const mockClineProvider = {
3535
getCurrentCline: vi.fn(),
3636
getTaskWithId: vi.fn(),
3737
initClineWithHistoryItem: vi.fn(),
38-
getMcpHub: vi.fn(),
3938
} as unknown as ClineProvider
4039

4140
import { t } from "../../../i18n"
@@ -589,138 +588,43 @@ describe("webviewMessageHandler - mcpEnabled", () => {
589588
handleMcpEnabledChange: vi.fn().mockResolvedValue(undefined),
590589
}
591590

592-
// Mock the getMcpHub method to return our mock McpHub
593-
mockClineProvider.getMcpHub = vi.fn().mockReturnValue(mockMcpHub)
594-
595-
// Reset the contextProxy getValue mock
596-
vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(undefined)
591+
// Ensure provider exposes getMcpHub and returns our mock
592+
;(mockClineProvider as any).getMcpHub = vi.fn().mockReturnValue(mockMcpHub)
597593
})
598594

599-
it("should not refresh MCP servers when value does not change (true to true)", async () => {
600-
// Setup: mcpEnabled is already true
601-
vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(true)
602-
603-
// Act: Send mcpEnabled message with same value
595+
it("delegates enable=true to McpHub and posts updated state", async () => {
604596
await webviewMessageHandler(mockClineProvider, {
605597
type: "mcpEnabled",
606598
bool: true,
607599
})
608600

609-
// Assert: handleMcpEnabledChange should not be called
610-
expect(mockMcpHub.handleMcpEnabledChange).not.toHaveBeenCalled()
611-
expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("mcpEnabled", true)
612-
expect(mockClineProvider.postStateToWebview).toHaveBeenCalled()
613-
})
614-
615-
it("should not refresh MCP servers when value does not change (false to false)", async () => {
616-
// Setup: mcpEnabled is already false
617-
vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(false)
618-
619-
// Act: Send mcpEnabled message with same value
620-
await webviewMessageHandler(mockClineProvider, {
621-
type: "mcpEnabled",
622-
bool: false,
623-
})
624-
625-
// Assert: handleMcpEnabledChange should not be called
626-
expect(mockMcpHub.handleMcpEnabledChange).not.toHaveBeenCalled()
627-
expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("mcpEnabled", false)
628-
expect(mockClineProvider.postStateToWebview).toHaveBeenCalled()
629-
})
630-
631-
it("should refresh MCP servers when value changes from true to false", async () => {
632-
// Setup: mcpEnabled is true
633-
vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(true)
634-
635-
// Act: Send mcpEnabled message with false
636-
await webviewMessageHandler(mockClineProvider, {
637-
type: "mcpEnabled",
638-
bool: false,
639-
})
640-
641-
// Assert: handleMcpEnabledChange should be called
642-
expect(mockMcpHub.handleMcpEnabledChange).toHaveBeenCalledWith(false)
643-
expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("mcpEnabled", false)
644-
expect(mockClineProvider.postStateToWebview).toHaveBeenCalled()
645-
})
646-
647-
it("should refresh MCP servers when value changes from false to true", async () => {
648-
// Setup: mcpEnabled is false
649-
vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(false)
650-
651-
// Act: Send mcpEnabled message with true
652-
await webviewMessageHandler(mockClineProvider, {
653-
type: "mcpEnabled",
654-
bool: true,
655-
})
656-
657-
// Assert: handleMcpEnabledChange should be called
601+
expect((mockClineProvider as any).getMcpHub).toHaveBeenCalledTimes(1)
602+
expect(mockMcpHub.handleMcpEnabledChange).toHaveBeenCalledTimes(1)
658603
expect(mockMcpHub.handleMcpEnabledChange).toHaveBeenCalledWith(true)
659-
expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("mcpEnabled", true)
660-
expect(mockClineProvider.postStateToWebview).toHaveBeenCalled()
661-
})
662-
663-
it("should handle undefined values with defaults correctly", async () => {
664-
// Setup: mcpEnabled is undefined (defaults to true)
665-
vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(undefined)
666-
667-
// Act: Send mcpEnabled message with undefined (defaults to true)
668-
await webviewMessageHandler(mockClineProvider, {
669-
type: "mcpEnabled",
670-
bool: undefined,
671-
})
672-
673-
// Assert: Should use default value (true) and not trigger refresh since both are true
674-
expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("mcpEnabled", true)
675-
expect(mockMcpHub.handleMcpEnabledChange).not.toHaveBeenCalled()
676-
expect(mockClineProvider.postStateToWebview).toHaveBeenCalled()
604+
expect(mockClineProvider.postStateToWebview).toHaveBeenCalledTimes(1)
677605
})
678606

679-
it("should handle when mcpEnabled changes from undefined to false", async () => {
680-
// Setup: mcpEnabled is undefined (defaults to true)
681-
vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(undefined)
682-
683-
// Act: Send mcpEnabled message with false
607+
it("delegates enable=false to McpHub and posts updated state", async () => {
684608
await webviewMessageHandler(mockClineProvider, {
685609
type: "mcpEnabled",
686610
bool: false,
687611
})
688612

689-
// Assert: Should trigger refresh since undefined defaults to true and we're changing to false
690-
expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("mcpEnabled", false)
613+
expect((mockClineProvider as any).getMcpHub).toHaveBeenCalledTimes(1)
614+
expect(mockMcpHub.handleMcpEnabledChange).toHaveBeenCalledTimes(1)
691615
expect(mockMcpHub.handleMcpEnabledChange).toHaveBeenCalledWith(false)
692-
expect(mockClineProvider.postStateToWebview).toHaveBeenCalled()
693-
})
694-
695-
it("should not call handleMcpEnabledChange when McpHub is not available", async () => {
696-
// Setup: No McpHub instance available
697-
mockClineProvider.getMcpHub = vi.fn().mockReturnValue(null)
698-
vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(true)
699-
700-
// Act: Send mcpEnabled message with false
701-
await webviewMessageHandler(mockClineProvider, {
702-
type: "mcpEnabled",
703-
bool: false,
704-
})
705-
706-
// Assert: State should be updated but handleMcpEnabledChange should not be called
707-
expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("mcpEnabled", false)
708-
expect(mockClineProvider.postStateToWebview).toHaveBeenCalled()
709-
// No error should be thrown
616+
expect(mockClineProvider.postStateToWebview).toHaveBeenCalledTimes(1)
710617
})
711618

712-
it("should always update state even when value doesn't change", async () => {
713-
// Setup: mcpEnabled is true
714-
vi.mocked(mockClineProvider.contextProxy.getValue).mockReturnValue(true)
619+
it("handles missing McpHub instance gracefully and still posts state", async () => {
620+
;(mockClineProvider as any).getMcpHub = vi.fn().mockReturnValue(undefined)
715621

716-
// Act: Send mcpEnabled message with same value
717622
await webviewMessageHandler(mockClineProvider, {
718623
type: "mcpEnabled",
719624
bool: true,
720625
})
721626

722-
// Assert: State should still be updated to ensure consistency
723-
expect(mockClineProvider.contextProxy.setValue).toHaveBeenCalledWith("mcpEnabled", true)
724-
expect(mockClineProvider.postStateToWebview).toHaveBeenCalled()
627+
expect((mockClineProvider as any).getMcpHub).toHaveBeenCalledTimes(1)
628+
expect(mockClineProvider.postStateToWebview).toHaveBeenCalledTimes(1)
725629
})
726630
})

‎src/core/webview/webviewMessageHandler.ts‎

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -900,19 +900,12 @@ export const webviewMessageHandler = async (
900900
}
901901
case "mcpEnabled":
902902
const mcpEnabled = message.bool ?? true
903-
const currentMcpEnabled = getGlobalState("mcpEnabled") ?? true
904-
905-
// Always update the state to ensure consistency
906903
await updateGlobalState("mcpEnabled", mcpEnabled)
907904

908-
// Only refresh MCP connections if the value actually changed
909-
// This prevents expensive MCP server refresh operations when saving unrelated settings
910-
if (currentMcpEnabled !== mcpEnabled) {
911-
// Delegate MCP enable/disable logic to McpHub
912-
const mcpHubInstance = provider.getMcpHub()
913-
if (mcpHubInstance) {
914-
await mcpHubInstance.handleMcpEnabledChange(mcpEnabled)
915-
}
905+
// Delegate MCP enable/disable logic to McpHub
906+
const mcpHubInstance = provider.getMcpHub()
907+
if (mcpHubInstance) {
908+
await mcpHubInstance.handleMcpEnabledChange(mcpEnabled)
916909
}
917910

918911
await provider.postStateToWebview()

‎src/services/mcp/McpHub.ts‎

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1212,7 +1212,6 @@ export class McpHub {
12121212

12131213
public async refreshAllConnections(): Promise<void> {
12141214
if (this.isConnecting) {
1215-
vscode.window.showInformationMessage(t("mcp:info.already_refreshing"))
12161215
return
12171216
}
12181217

@@ -1234,7 +1233,6 @@ export class McpHub {
12341233
}
12351234

12361235
this.isConnecting = true
1237-
vscode.window.showInformationMessage(t("mcp:info.refreshing_all"))
12381236

12391237
try {
12401238
const globalPath = await this.getMcpSettingsFilePath()
@@ -1244,11 +1242,6 @@ export class McpHub {
12441242
const globalConfig = JSON.parse(globalContent)
12451243
globalServers = globalConfig.mcpServers || {}
12461244
const globalServerNames = Object.keys(globalServers)
1247-
vscode.window.showInformationMessage(
1248-
t("mcp:info.global_servers_active", {
1249-
mcpServers: `${globalServerNames.join(", ") || "none"}`,
1250-
}),
1251-
)
12521245
} catch (error) {
12531246
console.log("Error reading global MCP config:", error)
12541247
}
@@ -1261,11 +1254,6 @@ export class McpHub {
12611254
const projectConfig = JSON.parse(projectContent)
12621255
projectServers = projectConfig.mcpServers || {}
12631256
const projectServerNames = Object.keys(projectServers)
1264-
vscode.window.showInformationMessage(
1265-
t("mcp:info.project_servers_active", {
1266-
mcpServers: `${projectServerNames.join(", ") || "none"}`,
1267-
}),
1268-
)
12691257
} catch (error) {
12701258
console.log("Error reading project MCP config:", error)
12711259
}
@@ -1285,8 +1273,6 @@ export class McpHub {
12851273
await delay(100)
12861274

12871275
await this.notifyWebviewOfServerChanges()
1288-
1289-
vscode.window.showInformationMessage(t("mcp:info.all_refreshed"))
12901276
} catch (error) {
12911277
this.showErrorMessage("Failed to refresh MCP servers", error)
12921278
} finally {

0 commit comments

Comments
 (0)