From 78e3ae7bdeb115aa425de9481157fe0d5b123037 Mon Sep 17 00:00:00 2001
From: yuneng-jiang <yuneng.jiang@gmail.com>
Date: Mon, 22 Dec 2025 15:17:36 -0800
Subject: [PATCH] Fix MCP Select button submitting form

---
 .../MCPToolPermissions.test.tsx               | 133 +++++++++++++++---
 .../MCPToolPermissions.tsx                    |  13 +-
 2 files changed, 120 insertions(+), 26 deletions(-)

diff --git a/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.test.tsx b/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.test.tsx
index ec541d7a63..fdd69d064d 100644
--- a/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.test.tsx
+++ b/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.test.tsx
@@ -1,22 +1,12 @@
 import { describe, it, expect, vi, beforeEach } from "vitest";
-import { render, screen, waitFor } from "@testing-library/react";
+import { screen, waitFor } from "@testing-library/react";
 import userEvent from "@testing-library/user-event";
-import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
+import { renderWithProviders } from "../../../tests/test-utils";
 import MCPToolPermissions from "./MCPToolPermissions";
 import * as networking from "../networking";
 
 vi.mock("../networking");
 
-const createQueryClient = () =>
-  new QueryClient({
-    defaultOptions: {
-      queries: {
-        retry: false,
-        gcTime: 0,
-      },
-    },
-  });
-
 describe("MCPToolPermissions", () => {
   const mockAccessToken = "test-token";
   const mockServerId = "server-123";
@@ -53,16 +43,13 @@ describe("MCPToolPermissions", () => {
       error: false,
     });
 
-    const queryClient = createQueryClient();
-    render(
-      <QueryClientProvider client={queryClient}>
-        <MCPToolPermissions
-          accessToken={mockAccessToken}
-          selectedServers={[mockServerId]}
-          toolPermissions={{}}
-          onChange={mockOnChange}
-        />
-      </QueryClientProvider>,
+    renderWithProviders(
+      <MCPToolPermissions
+        accessToken={mockAccessToken}
+        selectedServers={[mockServerId]}
+        toolPermissions={{}}
+        onChange={mockOnChange}
+      />,
     );
 
     // Wait for server and tools to load
@@ -87,4 +74,106 @@ describe("MCPToolPermissions", () => {
     expect(networking.fetchMCPServers).toHaveBeenCalledWith(mockAccessToken);
     expect(networking.listMCPTools).toHaveBeenCalledWith(mockAccessToken, mockServerId);
   });
+
+  it("should select all tools when Select All button is clicked", async () => {
+    const mockOnChange = vi.fn();
+    const mockTools = [
+      { name: "read_wiki_structure", description: "Get documentation topics" },
+      { name: "read_wiki_contents", description: "View documentation" },
+      { name: "ask_question", description: "Ask questions" },
+    ];
+
+    // Mock fetchMCPServers to return server details
+    vi.mocked(networking.fetchMCPServers).mockResolvedValue([
+      {
+        server_id: mockServerId,
+        server_name: mockServerName,
+        alias: mockServerName,
+      },
+    ]);
+
+    // Mock listMCPTools to return tools for the server
+    vi.mocked(networking.listMCPTools).mockResolvedValue({
+      tools: mockTools,
+      error: false,
+    });
+
+    renderWithProviders(
+      <MCPToolPermissions
+        accessToken={mockAccessToken}
+        selectedServers={[mockServerId]}
+        toolPermissions={{}}
+        onChange={mockOnChange}
+      />,
+    );
+
+    // Wait for server and tools to load
+    await waitFor(() => {
+      expect(screen.getByText(mockServerName)).toBeInTheDocument();
+    });
+
+    await waitFor(() => {
+      expect(screen.getByText("read_wiki_structure")).toBeInTheDocument();
+    });
+
+    // Click the Select All button
+    const selectAllButton = screen.getByRole("button", { name: "Select All" });
+    await userEvent.click(selectAllButton);
+
+    // Verify onChange was called with all tools selected
+    expect(mockOnChange).toHaveBeenCalledWith({
+      [mockServerId]: ["read_wiki_structure", "read_wiki_contents", "ask_question"],
+    });
+  });
+
+  it("should deselect all tools when Deselect All button is clicked", async () => {
+    const mockOnChange = vi.fn();
+    const mockTools = [
+      { name: "read_wiki_structure", description: "Get documentation topics" },
+      { name: "read_wiki_contents", description: "View documentation" },
+      { name: "ask_question", description: "Ask questions" },
+    ];
+
+    // Mock fetchMCPServers to return server details
+    vi.mocked(networking.fetchMCPServers).mockResolvedValue([
+      {
+        server_id: mockServerId,
+        server_name: mockServerName,
+        alias: mockServerName,
+      },
+    ]);
+
+    // Mock listMCPTools to return tools for the server
+    vi.mocked(networking.listMCPTools).mockResolvedValue({
+      tools: mockTools,
+      error: false,
+    });
+
+    renderWithProviders(
+      <MCPToolPermissions
+        accessToken={mockAccessToken}
+        selectedServers={[mockServerId]}
+        toolPermissions={{ [mockServerId]: ["read_wiki_structure", "read_wiki_contents"] }}
+        onChange={mockOnChange}
+      />,
+    );
+
+    // Wait for server and tools to load
+    await waitFor(() => {
+      expect(screen.getByText(mockServerName)).toBeInTheDocument();
+    });
+
+    await waitFor(() => {
+      expect(screen.getByText("read_wiki_structure")).toBeInTheDocument();
+    });
+
+    // Click the Deselect All button
+    const deselectAllButton = screen.getByRole("button", { name: "Deselect All" });
+    await userEvent.click(deselectAllButton);
+
+    // Verify onChange was called with no tools selected
+    expect(mockOnChange).toHaveBeenCalledWith({
+      [mockServerId]: [],
+    });
+  });
 });
diff --git a/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.tsx b/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.tsx
index a567791e22..ec7e279781 100644
--- a/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.tsx
+++ b/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.tsx
@@ -80,17 +80,19 @@ const MCPToolPermissions: React.FC<MCPToolPermissionsProps> = ({
 
   const handleSelectAll = (serverId: string) => {
     const tools = serverTools[serverId] || [];
-    onChange({
+    const newPermissions = {
       ...toolPermissions,
       [serverId]: tools.map((t) => t.name),
-    });
+    };
+    onChange(newPermissions);
   };
 
   const handleDeselectAll = (serverId: string) => {
-    onChange({
+    const newPermissions = {
       ...toolPermissions,
       [serverId]: [],
-    });
+    };
+    onChange(newPermissions);
   };
 
   if (selectedServers.length === 0) {
@@ -116,6 +118,7 @@ const MCPToolPermissions: React.FC<MCPToolPermissionsProps> = ({
               </div>
               <div className="flex items-center gap-3">
                 <button
+                  type="button"
                   className="text-sm text-blue-600 hover:text-blue-700 font-medium"
                   onClick={() => handleSelectAll(server.server_id)}
                   disabled={disabled || isLoading}
@@ -123,6 +126,7 @@ const MCPToolPermissions: React.FC<MCPToolPermissionsProps> = ({
                   Select All
                 </button>
                 <button
+                  type="button"
                   className="text-sm text-blue-600 hover:text-blue-700 font-medium"
                   onClick={() => handleDeselectAll(server.server_id)}
                   disabled={disabled || isLoading}
@@ -130,6 +134,7 @@ const MCPToolPermissions: React.FC<MCPToolPermissionsProps> = ({
                   Deselect All
                 </button>
                 <button
+                  type="button"
                   className="text-gray-400 hover:text-gray-600"
                   onClick={() => {
                     // Handle remove server if needed