fix: address PR review feedback for disabled MCP servers

- Centralize disabled state validation in connectToServer() method
- Add protection against race conditions in toggleServerDisabled()
- Remove redundant grey color indicator (keep only opacity)
- Remove unnecessary status checks in toggle logic
- Add comprehensive test coverage for edge cases
- Fix failing test by ensuring correct disabled state in config

Author: hannesrudolph

src/services/mcp/McpHub.ts

@@ -134,6 +134,7 @@ export class McpHub {
 	isConnecting: boolean = false
 	private refCount: number = 0 // Reference counter for active clients
 	private configChangeDebounceTimers: Map<string, NodeJS.Timeout> = new Map()
+	private toggleOperations: Map<string, Promise<void>> = new Map() // Track ongoing toggle operations
 
 	constructor(provider: ClineProvider) {
 		this.providerRef = new WeakRef(provider)
@@ -557,6 +558,12 @@ export class McpHub {
 		config: z.infer<typeof ServerConfigSchema>,
 		source: "global" | "project" = "global",
 	): Promise<void> {
+		// Check if server is disabled before attempting to connect
+		if (config.disabled) {
+			console.log(`Skipping connection to disabled server: ${name}`)
+			return
+		}
+
 		// Remove existing connection if it exists with the same source
 		await this.deleteConnection(name, source)
 
@@ -973,24 +980,19 @@ export class McpHub {
 			}
 
 			if (!currentConnection) {
-				// New server - only connect if not disabled
-				if (!validatedConfig.disabled) {
-					try {
-						this.setupFileWatcher(name, validatedConfig, source)
-						await this.connectToServer(name, validatedConfig, source)
-					} catch (error) {
-						this.showErrorMessage(`Failed to connect to new MCP server ${name}`, error)
-					}
+				// New server
+				try {
+					this.setupFileWatcher(name, validatedConfig, source)
+					await this.connectToServer(name, validatedConfig, source)
+				} catch (error) {
+					this.showErrorMessage(`Failed to connect to new MCP server ${name}`, error)
 				}
 			} else if (!deepEqual(JSON.parse(currentConnection.server.config), config)) {
 				// Existing server with changed config
 				try {
 					this.setupFileWatcher(name, validatedConfig, source)
 					await this.deleteConnection(name, source)
-					// Only reconnect if not disabled
-					if (!validatedConfig.disabled) {
-						await this.connectToServer(name, validatedConfig, source)
-					}
+					await this.connectToServer(name, validatedConfig, source)
 				} catch (error) {
 					this.showErrorMessage(`Failed to reconnect MCP server ${name}`, error)
 				}
@@ -1245,6 +1247,33 @@ export class McpHub {
 		serverName: string,
 		disabled: boolean,
 		source?: "global" | "project",
+	): Promise<void> {
+		// Check if there's already a toggle operation in progress for this server
+		const operationKey = `${serverName}-${source || "global"}`
+		const existingOperation = this.toggleOperations.get(operationKey)
+
+		if (existingOperation) {
+			console.log(`Toggle operation already in progress for ${operationKey}, waiting...`)
+			await existingOperation
+			return
+		}
+
+		// Create a new toggle operation
+		const toggleOperation = this._performToggleServerDisabled(serverName, disabled, source)
+		this.toggleOperations.set(operationKey, toggleOperation)
+
+		try {
+			await toggleOperation
+		} finally {
+			// Clean up the operation from the map
+			this.toggleOperations.delete(operationKey)
+		}
+	}
+
+	private async _performToggleServerDisabled(
+		serverName: string,
+		disabled: boolean,
+		source?: "global" | "project",
 	): Promise<void> {
 		try {
 			// Find the connection to determine if it's a global or project server
@@ -1263,11 +1292,13 @@ export class McpHub {
 					connection.server.disabled = disabled
 
 					// If disabling the server, disconnect it
-					if (disabled && connection.server.status === "connected") {
+					if (disabled) {
 						await this.deleteConnection(serverName, serverSource)
-					} else if (!disabled && connection.server.status !== "connected") {
-						// If enabling the server and it's not connected, connect it
+					} else if (!disabled) {
+						// If enabling the server, connect it
 						const config = JSON.parse(connection.server.config)
+						// Ensure the config has the updated disabled state
+						config.disabled = false
 						await this.connectToServer(serverName, config, serverSource)
 					}
 

src/services/mcp/__tests__/McpHub.spec.ts

@@ -766,6 +766,141 @@ describe("McpHub", () => {
 			// Verify the server was connected
 			expect(mockClient.connect).toHaveBeenCalled()
 		})
+
+		it("should handle toggling a server that's already in the target state", async () => {
+			const mockConfig = {
+				mcpServers: {
+					"test-server": {
+						type: "stdio",
+						command: "node",
+						args: ["test.js"],
+						disabled: true,
+					},
+				},
+			}
+
+			// Mock reading initial config
+			vi.mocked(fs.readFile).mockResolvedValueOnce(JSON.stringify(mockConfig))
+
+			// Set up mock connection for already disabled server
+			const mockConnection: McpConnection = {
+				server: {
+					name: "test-server",
+					config: JSON.stringify({
+						type: "stdio",
+						command: "node",
+						args: ["test.js"],
+						disabled: true,
+					}),
+					status: "disconnected",
+					disabled: true,
+					source: "global",
+				},
+				client: {} as any,
+				transport: {} as any,
+			}
+			mcpHub.connections = [mockConnection]
+
+			// Try to disable an already disabled server
+			await mcpHub.toggleServerDisabled("test-server", true)
+
+			// Verify the config was still updated (idempotent operation)
+			expect(fs.writeFile).toHaveBeenCalled()
+		})
+
+		it("should handle errors during toggle operations gracefully", async () => {
+			const mockConfig = {
+				mcpServers: {
+					"test-server": {
+						type: "stdio",
+						command: "node",
+						args: ["test.js"],
+						disabled: false,
+					},
+				},
+			}
+
+			// Mock reading initial config
+			vi.mocked(fs.readFile).mockResolvedValueOnce(JSON.stringify(mockConfig))
+
+			// Mock writeFile to throw an error
+			vi.mocked(fs.writeFile).mockRejectedValueOnce(new Error("Permission denied"))
+
+			// Set up mock connection
+			const mockConnection: McpConnection = {
+				server: {
+					name: "test-server",
+					config: JSON.stringify({
+						type: "stdio",
+						command: "node",
+						args: ["test.js"],
+						disabled: false,
+					}),
+					status: "connected",
+					disabled: false,
+					source: "global",
+				},
+				client: {} as any,
+				transport: {} as any,
+			}
+			mcpHub.connections = [mockConnection]
+
+			// Try to toggle and expect it to throw
+			await expect(mcpHub.toggleServerDisabled("test-server", true)).rejects.toThrow()
+		})
+
+		it("should handle concurrent toggle operations", async () => {
+			const mockConfig = {
+				mcpServers: {
+					"test-server": {
+						type: "stdio",
+						command: "node",
+						args: ["test.js"],
+						disabled: false,
+					},
+				},
+			}
+
+			// Mock reading initial config
+			vi.mocked(fs.readFile).mockResolvedValue(JSON.stringify(mockConfig))
+
+			// Mock writeFile with a delay to simulate slow operation
+			let writeCallCount = 0
+			vi.mocked(fs.writeFile).mockImplementation(async () => {
+				writeCallCount++
+				await new Promise((resolve) => setTimeout(resolve, 100))
+			})
+
+			// Set up mock connection
+			const mockConnection: McpConnection = {
+				server: {
+					name: "test-server",
+					config: JSON.stringify({
+						type: "stdio",
+						command: "node",
+						args: ["test.js"],
+						disabled: false,
+					}),
+					status: "connected",
+					disabled: false,
+					source: "global",
+				},
+				client: {} as any,
+				transport: {} as any,
+			}
+			mcpHub.connections = [mockConnection]
+
+			// Start multiple concurrent toggle operations
+			const promise1 = mcpHub.toggleServerDisabled("test-server", true)
+			const promise2 = mcpHub.toggleServerDisabled("test-server", true)
+			const promise3 = mcpHub.toggleServerDisabled("test-server", true)
+
+			// Wait for all to complete
+			await Promise.all([promise1, promise2, promise3])
+
+			// Only the first operation should have executed
+			expect(writeCallCount).toBe(1)
+		})
 	})
 
 	describe("callTool", () => {

webview-ui/src/components/mcp/McpView.tsx

@@ -218,11 +218,6 @@ const ServerRow = ({ server, alwaysAllowMcp }: { server: McpServer; alwaysAllowM
 	]
 
 	const getStatusColor = () => {
-		// Show grey/disabled color for disabled servers regardless of connection status
-		if (server.disabled) {
-			return "var(--vscode-disabledForeground)"
-		}
-
 		switch (server.status) {
 			case "connected":
 				return "var(--vscode-testing-iconPassed)"