From caf305f732cb2d48ce65d798c2ad0704dd934c33 Mon Sep 17 00:00:00 2001 From: Yuneng Jiang Date: Fri, 14 Aug 2026 11:08:09 -0700 Subject: [PATCH] refactor(ui): move MCP permission panels onto shadcn primitives Replaces antd Radio, Checkbox and Tooltip, plus Tremor Text and Badge, with the in-repo shadcn equivalents across the three MCP permission panels, and drops the no-restricted-imports suppressions they no longer need. Also removes the stale suppression on settings.test.tsx, which imports neither library. The tool rows keep their existing click-to-toggle behaviour: the row owns the toggle and the checkbox no longer carries its own change handler, since Base UI replays the click through a hidden input that reaches the row on its own. Adds payload-level tests for the risk-group view covering group clear, mixed-state re-arm, single-tool toggles from both the box and the row, and a controlled round trip proving each control re-renders from the permissions it emitted. --- ui/litellm-dashboard/eslint-suppressions.json | 14 -- .../MCPToolPermissions.test.tsx | 133 +++++++++++++++++- .../MCPToolPermissions.tsx | 48 ++++--- .../mcp_tools/McpCrudPermissionPanel.tsx | 18 +-- .../permissions/MCPServerPermissions.tsx | 23 +-- 5 files changed, 175 insertions(+), 61 deletions(-) diff --git a/ui/litellm-dashboard/eslint-suppressions.json b/ui/litellm-dashboard/eslint-suppressions.json index f73e3e6dda3..d583aea1114 100644 --- a/ui/litellm-dashboard/eslint-suppressions.json +++ b/ui/litellm-dashboard/eslint-suppressions.json @@ -2504,9 +2504,6 @@ "src/components/mcp_server_management/MCPToolPermissions.tsx": { "local/no-complex-jsx-arrow": { "count": 1 - }, - "no-restricted-imports": { - "count": 2 } }, "src/components/mcp_tools/ByokCredentialModal.tsx": { @@ -2525,9 +2522,6 @@ "src/components/mcp_tools/McpCrudPermissionPanel.tsx": { "no-nested-ternary": { "count": 3 - }, - "no-restricted-imports": { - "count": 2 } }, "src/components/mcp_tools/types.tsx": { @@ -2720,9 +2714,6 @@ "src/components/permissions/MCPServerPermissions.tsx": { "no-nested-ternary": { "count": 3 - }, - "no-restricted-imports": { - "count": 2 } }, "src/components/policies/PolicySelector.tsx": { @@ -2816,11 +2807,6 @@ "count": 1 } }, - "src/components/settings.test.tsx": { - "no-restricted-imports": { - "count": 1 - } - }, "src/components/settings.tsx": { "local/filename-pascal-case": { "count": 1 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 42ecdd6fd9b..4b5447dfd89 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,3 +1,4 @@ +import { useState } from "react"; import { describe, it, expect, vi, beforeEach } from "vitest"; import { screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; @@ -63,13 +64,14 @@ describe("MCPToolPermissions", () => { expect(screen.getByText("read_wiki_structure")).toBeInTheDocument(); }); - // Switch to Flat List view for predictable checkbox ordering - const flatListOption = screen.getByText("Flat List"); - await userEvent.click(flatListOption); + // Switch to Flat List view, and prove the view actually switched: the flat + // list is the only view that renders the description inline after a dash. + await userEvent.click(screen.getByText("Flat List")); + expect(screen.getByRole("radio", { name: "Flat List" })).toBeChecked(); + expect(await screen.findByText("- Get documentation topics")).toBeInTheDocument(); - // Click the first checkbox to deselect read_wiki_structure - const checkboxes = screen.getAllByRole("checkbox"); - await userEvent.click(checkboxes[0]); + // Deselect read_wiki_structure + await userEvent.click(screen.getByRole("checkbox", { name: "read_wiki_structure" })); // Verify onChange was called with read_wiki_structure removed expect(mockOnChange).toHaveBeenCalledWith({ @@ -184,4 +186,123 @@ describe("MCPToolPermissions", () => { [mockServerId]: [], }); }); + + describe("risk-group (CRUD) view", () => { + const crudTools = [ + { name: "list_documents", description: "List every document" }, + { name: "get_document", description: "Fetch one document" }, + { name: "delete_document", description: "Destroy a document" }, + ]; + const allCrudToolNames = crudTools.map((t) => t.name); + + const renderCrudView = (toolPermissions: Record, onChange: () => void) => { + vi.mocked(networking.fetchMCPServers).mockResolvedValue([ + { server_id: mockServerId, server_name: mockServerName, alias: mockServerName }, + ]); + vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: crudTools, error: false }); + + renderWithProviders( + , + ); + }; + + it("removes a whole risk group from the saved payload when its group toggle is cleared", async () => { + const mockOnChange = vi.fn(); + renderCrudView({ [mockServerId]: allCrudToolNames }, mockOnChange); + + const readGroupToggle = await screen.findByRole("checkbox", { name: "Allow all Read tools" }); + expect(readGroupToggle).toBeChecked(); + + await userEvent.click(readGroupToggle); + + // Both read-classified tools drop out; the delete-classified one survives. + expect(mockOnChange).toHaveBeenCalledWith({ [mockServerId]: ["delete_document"] }); + }); + + it("adds the rest of a partially-allowed risk group when its mixed toggle is clicked", async () => { + const mockOnChange = vi.fn(); + renderCrudView({ [mockServerId]: ["list_documents"] }, mockOnChange); + + const readGroupToggle = await screen.findByRole("checkbox", { name: "Allow all Read tools" }); + expect(readGroupToggle).toBePartiallyChecked(); + + await userEvent.click(readGroupToggle); + + expect(mockOnChange).toHaveBeenCalledWith({ [mockServerId]: ["list_documents", "get_document"] }); + }); + + it("toggles a single tool exactly once when its checkbox is clicked inside the clickable row", async () => { + const mockOnChange = vi.fn(); + renderCrudView({ [mockServerId]: allCrudToolNames }, mockOnChange); + + await userEvent.click(await screen.findByRole("checkbox", { name: "delete_document" })); + + // The surrounding row is itself clickable, so a click that bubbles would + // toggle twice and the permission would silently stay allowed. + expect(mockOnChange).toHaveBeenCalledTimes(1); + expect(mockOnChange).toHaveBeenCalledWith({ [mockServerId]: ["list_documents", "get_document"] }); + }); + + it("toggles a single tool when the row around its checkbox is clicked", async () => { + const mockOnChange = vi.fn(); + renderCrudView({ [mockServerId]: allCrudToolNames }, mockOnChange); + + await userEvent.click(await screen.findByText("Destroy a document")); + + expect(mockOnChange).toHaveBeenCalledTimes(1); + expect(mockOnChange).toHaveBeenCalledWith({ [mockServerId]: ["list_documents", "get_document"] }); + }); + + it("re-renders each checkbox from the permissions it emitted", async () => { + // Drives the panel from real parent state so the assertions cover the full + // round trip: click, emitted payload, then the state the panel renders back. + const Harness = () => { + const [permissions, setPermissions] = useState>({ + [mockServerId]: allCrudToolNames, + }); + return ( + <> + + {(permissions[mockServerId] ?? []).join(",")} + + ); + }; + + vi.mocked(networking.fetchMCPServers).mockResolvedValue([ + { server_id: mockServerId, server_name: mockServerName, alias: mockServerName }, + ]); + vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: crudTools, error: false }); + renderWithProviders(); + + const deleteTool = await screen.findByRole("checkbox", { name: "delete_document" }); + const readGroupToggle = screen.getByRole("checkbox", { name: "Allow all Read tools" }); + expect(deleteTool).toBeChecked(); + expect(readGroupToggle).toBeChecked(); + + await userEvent.click(deleteTool); + expect(deleteTool).not.toBeChecked(); + expect(screen.getByRole("status")).toHaveTextContent("list_documents,get_document"); + + // Clearing one tool of the Read group must leave that group's toggle mixed. + await userEvent.click(screen.getByRole("checkbox", { name: "get_document" })); + expect(readGroupToggle).toBePartiallyChecked(); + expect(screen.getByRole("status")).toHaveTextContent("list_documents"); + + // Re-arming the group restores both read tools and leaves delete blocked. + await userEvent.click(readGroupToggle); + expect(readGroupToggle).toBeChecked(); + expect(screen.getByRole("status")).toHaveTextContent("list_documents,get_document"); + expect(screen.getByRole("checkbox", { name: "delete_document" })).not.toBeChecked(); + }); + }); }); 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 e05352d434f..d99dc3077d5 100644 --- a/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.tsx +++ b/ui/litellm-dashboard/src/components/mcp_server_management/MCPToolPermissions.tsx @@ -1,8 +1,8 @@ import React, { useEffect, useRef, useState, useMemo } from "react"; import { listMCPTools } from "../networking"; import { MCPTool, MCPServer } from "../mcp_tools/types"; -import { Text } from "@tremor/react"; -import { Spin, Radio } from "antd"; +import { RadioGroup, RadioGroupItem } from "@/components/ui/radio-group"; +import { UiLoadingSpinner } from "@/components/ui/ui-loading-spinner"; import { useMCPServers } from "../../app/(dashboard)/hooks/mcpServers/useMCPServers"; import McpCrudPermissionPanel from "../mcp_tools/McpCrudPermissionPanel"; import { classifyToolOp } from "../../utils/mcpToolCrudClassification"; @@ -121,22 +121,27 @@ const MCPToolPermissions: React.FC = ({ {/* Header */}
- {serverName} - {server.description && {server.description}} +

{serverName}

+ {server.description &&

{server.description}

}
{!disabled && tools.length > 0 && ( - setViewModes((prev) => ({ ...prev, [server.server_id]: e.target.value }))} - size="small" - optionType="button" - buttonStyle="solid" - options={[ - { label: "Risk Groups", value: "crud" }, - { label: "Flat List", value: "flat" }, - ]} - /> + onValueChange={(next) => + setViewModes((prev) => ({ ...prev, [server.server_id]: next as "crud" | "flat" })) + } + className="flex w-auto items-center gap-4" + > + + + )} {!disabled && ( <> @@ -166,16 +171,16 @@ const MCPToolPermissions: React.FC = ({ {/* Loading */} {isLoading && (
- - Loading tools... + +

Loading tools...

)} {/* Error */} {error && !isLoading && (
- Unable to load tools - {error} +

Unable to load tools

+

{error}

)} @@ -198,6 +203,7 @@ const MCPToolPermissions: React.FC = ({
{ if (disabled) return; @@ -211,8 +217,8 @@ const MCPToolPermissions: React.FC = ({ />
- {tool.name} - - {tool.description || "No description"} +

{tool.name}

+

- {tool.description || "No description"}

@@ -224,7 +230,7 @@ const MCPToolPermissions: React.FC = ({ {/* Empty State */} {!isLoading && !error && tools.length === 0 && (
- No tools available +

No tools available

)}
diff --git a/ui/litellm-dashboard/src/components/mcp_tools/McpCrudPermissionPanel.tsx b/ui/litellm-dashboard/src/components/mcp_tools/McpCrudPermissionPanel.tsx index 9cbf7025eaf..1b17f000584 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/McpCrudPermissionPanel.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/McpCrudPermissionPanel.tsx @@ -10,8 +10,7 @@ */ import React, { useMemo, useState } from "react"; -import { Checkbox } from "antd"; -import { Text } from "@tremor/react"; +import { Checkbox } from "@/components/ui/checkbox"; import { ChevronDownIcon, ChevronRightIcon } from "lucide-react"; import { CrudOp, MCPToolEntry, CRUD_GROUP_META, groupToolsByCrud } from "../../utils/mcpToolCrudClassification"; @@ -187,14 +186,13 @@ const McpCrudPermissionPanel: React.FC = ({ {!readOnly && (
- - {fullyAllowed ? "All on" : partial ? "Partial" : "All off"} - +

{fullyAllowed ? "All on" : partial ? "Partial" : "All off"}

{/* Checkbox supports `indeterminate`; Switch does not. */} toggleGroup(op, e.target.checked)} + onCheckedChange={(checked) => toggleGroup(op, checked)} onClick={(e) => e.stopPropagation()} />
@@ -228,16 +226,18 @@ const McpCrudPermissionPanel: React.FC = ({ } ${allowed ? "" : "opacity-60"}`} onClick={() => toggleTool(tool.name)} > + {/* The row's onClick is the single toggle path. Giving this checkbox its + own change handler as well would toggle twice per click on the box. */} toggleTool(tool.name)} disabled={readOnly} onClick={(e) => e.stopPropagation()} />
- {tool.name} +

{tool.name}

{tool.description && ( - {tool.description} +

{tool.description}

)}
- MCP Servers - +

MCP Servers

+ {blocksAllMcpServers ? "Blocked" : grantsAllProxyMcpServers ? "All" : totalCount}
@@ -120,14 +120,14 @@ export function MCPServerPermissions({ {blocksAllMcpServers ? (
- +

No MCP servers — this key is blocked from all MCP servers, including its team's servers - +

) : grantsAllProxyMcpServers ? (
- All Proxy MCP Servers +

All Proxy MCP Servers

) : totalCount > 0 ? (
@@ -146,13 +146,14 @@ export function MCPServerPermissions({ >
{item.type === "server" ? ( - -
+ + }> {getMCPServerDisplayName(item.value)} -
+ + {`Full ID: ${item.value}`} ) : (
@@ -256,7 +257,7 @@ export function MCPServerPermissions({ ) : (
- No MCP servers, access groups, or toolsets configured +

No MCP servers, access groups, or toolsets configured

)}