mirror of
https://github.com/BerriAI/litellm.git
synced 2026-08-28 05:25:59 +00:00
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.
This commit is contained in:
Yuneng Jiang
parent
c9917cbf99
commit
caf305f732
5 changed files with 175 additions and 61 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<string, string[]>, 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(
|
||||
<MCPToolPermissions
|
||||
accessToken={mockAccessToken}
|
||||
selectedServers={[mockServerId]}
|
||||
toolPermissions={toolPermissions}
|
||||
onChange={onChange}
|
||||
/>,
|
||||
);
|
||||
};
|
||||
|
||||
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<Record<string, string[]>>({
|
||||
[mockServerId]: allCrudToolNames,
|
||||
});
|
||||
return (
|
||||
<>
|
||||
<MCPToolPermissions
|
||||
accessToken={mockAccessToken}
|
||||
selectedServers={[mockServerId]}
|
||||
toolPermissions={permissions}
|
||||
onChange={setPermissions}
|
||||
/>
|
||||
<output>{(permissions[mockServerId] ?? []).join(",")}</output>
|
||||
</>
|
||||
);
|
||||
};
|
||||
|
||||
vi.mocked(networking.fetchMCPServers).mockResolvedValue([
|
||||
{ server_id: mockServerId, server_name: mockServerName, alias: mockServerName },
|
||||
]);
|
||||
vi.mocked(networking.listMCPTools).mockResolvedValue({ tools: crudTools, error: false });
|
||||
renderWithProviders(<Harness />);
|
||||
|
||||
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();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<MCPToolPermissionsProps> = ({
|
|||
{/* Header */}
|
||||
<div className="flex items-center justify-between p-4 border-b bg-white rounded-t-lg">
|
||||
<div>
|
||||
<Text className="font-semibold text-gray-900">{serverName}</Text>
|
||||
{server.description && <Text className="text-sm text-gray-500">{server.description}</Text>}
|
||||
<p className="text-sm font-semibold text-gray-900">{serverName}</p>
|
||||
{server.description && <p className="text-sm text-gray-500">{server.description}</p>}
|
||||
</div>
|
||||
<div className="flex items-center gap-3">
|
||||
{!disabled && tools.length > 0 && (
|
||||
<Radio.Group
|
||||
<RadioGroup
|
||||
value={viewMode}
|
||||
onChange={(e) => 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"
|
||||
>
|
||||
<label className="flex items-center gap-2 text-sm">
|
||||
<RadioGroupItem value="crud" />
|
||||
Risk Groups
|
||||
</label>
|
||||
<label className="flex items-center gap-2 text-sm">
|
||||
<RadioGroupItem value="flat" />
|
||||
Flat List
|
||||
</label>
|
||||
</RadioGroup>
|
||||
)}
|
||||
{!disabled && (
|
||||
<>
|
||||
|
|
@ -166,16 +171,16 @@ const MCPToolPermissions: React.FC<MCPToolPermissionsProps> = ({
|
|||
{/* Loading */}
|
||||
{isLoading && (
|
||||
<div className="flex items-center justify-center py-8">
|
||||
<Spin size="large" />
|
||||
<Text className="ml-3 text-gray-500">Loading tools...</Text>
|
||||
<UiLoadingSpinner />
|
||||
<p className="ml-3 text-sm text-gray-500">Loading tools...</p>
|
||||
</div>
|
||||
)}
|
||||
|
||||
{/* Error */}
|
||||
{error && !isLoading && (
|
||||
<div className="p-4 bg-red-50 border border-red-200 rounded-lg text-center">
|
||||
<Text className="text-red-600 font-medium">Unable to load tools</Text>
|
||||
<Text className="text-sm text-red-500 mt-1">{error}</Text>
|
||||
<p className="text-sm text-red-600 font-medium">Unable to load tools</p>
|
||||
<p className="text-sm text-red-500 mt-1">{error}</p>
|
||||
</div>
|
||||
)}
|
||||
|
||||
|
|
@ -198,6 +203,7 @@ const MCPToolPermissions: React.FC<MCPToolPermissionsProps> = ({
|
|||
<div key={tool.name} className="flex items-start gap-2">
|
||||
<input
|
||||
type="checkbox"
|
||||
aria-label={tool.name}
|
||||
checked={isSelected}
|
||||
onChange={() => {
|
||||
if (disabled) return;
|
||||
|
|
@ -211,8 +217,8 @@ const MCPToolPermissions: React.FC<MCPToolPermissionsProps> = ({
|
|||
/>
|
||||
<div className="flex-1 min-w-0">
|
||||
<div className="flex items-center gap-2">
|
||||
<Text className="font-medium text-gray-900">{tool.name}</Text>
|
||||
<Text className="text-sm text-gray-500">- {tool.description || "No description"}</Text>
|
||||
<p className="text-sm font-medium text-gray-900">{tool.name}</p>
|
||||
<p className="text-sm text-gray-500">- {tool.description || "No description"}</p>
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
|
|
@ -224,7 +230,7 @@ const MCPToolPermissions: React.FC<MCPToolPermissionsProps> = ({
|
|||
{/* Empty State */}
|
||||
{!isLoading && !error && tools.length === 0 && (
|
||||
<div className="text-center py-6">
|
||||
<Text className="text-gray-500">No tools available</Text>
|
||||
<p className="text-sm text-gray-500">No tools available</p>
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
|
|
|
|||
|
|
@ -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<McpCrudPermissionPanelProps> = ({
|
|||
|
||||
{!readOnly && (
|
||||
<div className="flex items-center gap-2 ml-4">
|
||||
<Text className="text-xs text-gray-500">
|
||||
{fullyAllowed ? "All on" : partial ? "Partial" : "All off"}
|
||||
</Text>
|
||||
<p className="text-xs text-gray-500">{fullyAllowed ? "All on" : partial ? "Partial" : "All off"}</p>
|
||||
{/* Checkbox supports `indeterminate`; Switch does not. */}
|
||||
<Checkbox
|
||||
aria-label={`Allow all ${meta.label} tools`}
|
||||
checked={fullyAllowed}
|
||||
indeterminate={partial}
|
||||
onChange={(e) => toggleGroup(op, e.target.checked)}
|
||||
onCheckedChange={(checked) => toggleGroup(op, checked)}
|
||||
onClick={(e) => e.stopPropagation()}
|
||||
/>
|
||||
</div>
|
||||
|
|
@ -228,16 +226,18 @@ const McpCrudPermissionPanel: React.FC<McpCrudPermissionPanelProps> = ({
|
|||
} ${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. */}
|
||||
<Checkbox
|
||||
aria-label={tool.name}
|
||||
checked={allowed}
|
||||
onChange={() => toggleTool(tool.name)}
|
||||
disabled={readOnly}
|
||||
onClick={(e) => e.stopPropagation()}
|
||||
/>
|
||||
<div className="flex-1 min-w-0">
|
||||
<Text className="font-medium text-gray-900 text-sm">{tool.name}</Text>
|
||||
<p className="font-medium text-gray-900 text-sm">{tool.name}</p>
|
||||
{tool.description && (
|
||||
<Text className="text-xs text-gray-500 mt-0.5 leading-snug">{tool.description}</Text>
|
||||
<p className="text-xs text-gray-500 mt-0.5 leading-snug">{tool.description}</p>
|
||||
)}
|
||||
</div>
|
||||
<span
|
||||
|
|
|
|||
|
|
@ -1,7 +1,7 @@
|
|||
import React, { useState, useEffect } from "react";
|
||||
import { Text, Badge } from "@tremor/react";
|
||||
import { ServerIcon, ChevronDownIcon, ChevronRightIcon } from "@heroicons/react/outline";
|
||||
import { Tooltip } from "antd";
|
||||
import { Badge } from "@/components/ui/badge";
|
||||
import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip";
|
||||
import { fetchMCPServers, fetchMCPToolsets } from "../networking";
|
||||
import { MCPServer, MCPToolset } from "../mcp_tools/types";
|
||||
import { ALL_PROXY_MCP_SERVERS_SENTINEL, NO_MCP_SERVERS_SENTINEL } from "../mcp_tools/constants";
|
||||
|
|
@ -111,8 +111,8 @@ export function MCPServerPermissions({
|
|||
<div className="space-y-3">
|
||||
<div className="flex items-center gap-2">
|
||||
<ServerIcon className="h-4 w-4 text-blue-600" />
|
||||
<Text className="font-semibold text-gray-900">MCP Servers</Text>
|
||||
<Badge color={blocksAllMcpServers ? "red" : "blue"} size="xs">
|
||||
<p className="text-sm font-semibold text-gray-900">MCP Servers</p>
|
||||
<Badge variant={blocksAllMcpServers ? "destructive" : "default"}>
|
||||
{blocksAllMcpServers ? "Blocked" : grantsAllProxyMcpServers ? "All" : totalCount}
|
||||
</Badge>
|
||||
</div>
|
||||
|
|
@ -120,14 +120,14 @@ export function MCPServerPermissions({
|
|||
{blocksAllMcpServers ? (
|
||||
<div className="flex items-center gap-2 px-3 py-2 rounded-lg bg-red-50 border border-red-200">
|
||||
<ServerIcon className="h-4 w-4 text-red-400" />
|
||||
<Text className="text-red-700 text-sm">
|
||||
<p className="text-red-700 text-sm">
|
||||
No MCP servers — this key is blocked from all MCP servers, including its team's servers
|
||||
</Text>
|
||||
</p>
|
||||
</div>
|
||||
) : grantsAllProxyMcpServers ? (
|
||||
<div className="flex items-center gap-2 px-3 py-2 rounded-lg bg-blue-50 border border-blue-200">
|
||||
<ServerIcon className="h-4 w-4 text-blue-400" />
|
||||
<Text className="text-blue-700 text-sm">All Proxy MCP Servers</Text>
|
||||
<p className="text-blue-700 text-sm">All Proxy MCP Servers</p>
|
||||
</div>
|
||||
) : totalCount > 0 ? (
|
||||
<div className="max-h-[400px] overflow-y-auto space-y-2 pr-1">
|
||||
|
|
@ -146,13 +146,14 @@ export function MCPServerPermissions({
|
|||
>
|
||||
<div className="flex items-center gap-2 flex-1 min-w-0">
|
||||
{item.type === "server" ? (
|
||||
<Tooltip title={`Full ID: ${item.value}`} placement="top">
|
||||
<div className="inline-flex items-center gap-2 min-w-0">
|
||||
<Tooltip>
|
||||
<TooltipTrigger render={<div className="inline-flex items-center gap-2 min-w-0" />}>
|
||||
<span className="inline-block w-1.5 h-1.5 bg-blue-500 rounded-full shrink-0"></span>
|
||||
<span className="text-sm font-medium text-gray-900 truncate">
|
||||
{getMCPServerDisplayName(item.value)}
|
||||
</span>
|
||||
</div>
|
||||
</TooltipTrigger>
|
||||
<TooltipContent>{`Full ID: ${item.value}`}</TooltipContent>
|
||||
</Tooltip>
|
||||
) : (
|
||||
<div className="inline-flex items-center gap-2 min-w-0">
|
||||
|
|
@ -256,7 +257,7 @@ export function MCPServerPermissions({
|
|||
) : (
|
||||
<div className="flex items-center gap-2 px-3 py-2 rounded-lg bg-gray-50 border border-gray-200">
|
||||
<ServerIcon className="h-4 w-4 text-gray-400" />
|
||||
<Text className="text-gray-500 text-sm">No MCP servers, access groups, or toolsets configured</Text>
|
||||
<p className="text-gray-500 text-sm">No MCP servers, access groups, or toolsets configured</p>
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue