From c32309fb2de108768fa8704ee0696b29d383733c Mon Sep 17 00:00:00 2001 From: yassin Date: Sat, 19 Sep 2026 00:13:44 +0000 Subject: [PATCH] feat(ui): show MCP allowed clients as cards edited in a dialog Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .../_components/MCPNetworkSettings.test.tsx | 114 +++++++++--- .../_components/MCPNetworkSettings.tsx | 167 ++++++++++++------ 2 files changed, 201 insertions(+), 80 deletions(-) diff --git a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/MCPNetworkSettings.test.tsx b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/MCPNetworkSettings.test.tsx index d27c18c5ae3..4cc87f1455c 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/MCPNetworkSettings.test.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/MCPNetworkSettings.test.tsx @@ -1,4 +1,4 @@ -import { fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { fireEvent, render, screen, waitFor, within } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { describe, it, expect, vi, beforeEach } from "vitest"; import MCPNetworkSettings from "./MCPNetworkSettings"; @@ -26,12 +26,20 @@ const renderSettings = () => render(); const ANTIGRAVITY = { alias: "Antigravity CLI", value: "antigravity-cli" }; const CODEX = { alias: "Codex", value: "codex-mcp-client" }; +const clientCard = (alias: string) => screen.getByRole("button", { name: new RegExp(`^${alias}`) }); + +const fillClientDialog = async (alias: string, value: string) => { + const dialog = await screen.findByRole("dialog"); + fireEvent.change(within(dialog).getByRole("textbox", { name: "Alias" }), { target: { value: alias } }); + fireEvent.change(within(dialog).getByRole("textbox", { name: "Value" }), { target: { value } }); + return dialog; +}; + const addClient = async (alias: string, value: string) => { await userEvent.click(screen.getByRole("button", { name: "Add client" })); - const aliases = screen.getAllByRole("textbox", { name: /^Client \d+ alias$/ }); - const values = screen.getAllByRole("textbox", { name: /^Client \d+ value$/ }); - fireEvent.change(aliases[aliases.length - 1], { target: { value: alias } }); - fireEvent.change(values[values.length - 1], { target: { value } }); + const dialog = await fillClientDialog(alias, value); + await userEvent.click(within(dialog).getByRole("button", { name: "Add" })); + await waitFor(() => expect(screen.queryByRole("dialog")).not.toBeInTheDocument()); }; describe("MCPNetworkSettings", () => { @@ -139,7 +147,7 @@ describe("MCPNetworkSettings", () => { expect(updateConfigFieldSetting).not.toHaveBeenCalled(); }); - it("labels the section Allowed Clients and renders each stored client as an alias and value row", async () => { + it("labels the section Allowed Clients and renders each stored client as a card showing alias and value", async () => { vi.mocked(getGeneralSettingsCall).mockResolvedValue([ { field_name: "mcp_allowed_clients", field_value: [ANTIGRAVITY, CODEX] }, ]); @@ -148,10 +156,24 @@ describe("MCPNetworkSettings", () => { expect(await screen.findByText("Allowed Clients")).toBeVisible(); expect(screen.queryByText(/Allowed Client IDs/)).not.toBeInTheDocument(); - expect(screen.getByRole("textbox", { name: "Client 1 alias" })).toHaveValue("Antigravity CLI"); - expect(screen.getByRole("textbox", { name: "Client 1 value" })).toHaveValue("antigravity-cli"); - expect(screen.getByRole("textbox", { name: "Client 2 alias" })).toHaveValue("Codex"); - expect(screen.getByRole("textbox", { name: "Client 2 value" })).toHaveValue("codex-mcp-client"); + expect(screen.queryByText(/Allowed Client Applications/)).not.toBeInTheDocument(); + expect(clientCard("Antigravity CLI")).toHaveTextContent("antigravity-cli"); + expect(clientCard("Codex")).toHaveTextContent("codex-mcp-client"); + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + }); + + it("opens an edit dialog when a client card is clicked, prefilled with that client's alias and value", async () => { + vi.mocked(getGeneralSettingsCall).mockResolvedValue([ + { field_name: "mcp_allowed_clients", field_value: [ANTIGRAVITY, CODEX] }, + ]); + + renderSettings(); + await screen.findByText("Allowed Clients"); + await userEvent.click(clientCard("Codex")); + + const dialog = await screen.findByRole("dialog", { name: "Edit client" }); + expect(within(dialog).getByRole("textbox", { name: "Alias" })).toHaveValue("Codex"); + expect(within(dialog).getByRole("textbox", { name: "Value" })).toHaveValue("codex-mcp-client"); }); it("warns that a stored allowlist in the old plain-string shape denies every client and lets Save remove it", async () => { @@ -162,7 +184,7 @@ describe("MCPNetworkSettings", () => { renderSettings(); expect(await screen.findByText(/stored allowlist is not a list of alias and value pairs/)).toBeVisible(); - expect(screen.queryByRole("textbox", { name: "Client 1 value" })).not.toBeInTheDocument(); + expect(screen.queryByText("antigravity-cli")).not.toBeInTheDocument(); await userEvent.click(screen.getByRole("button", { name: /Save/ })); @@ -203,15 +225,22 @@ describe("MCPNetworkSettings", () => { expect(deleteConfigFieldSetting).not.toHaveBeenCalledWith("tok", "mcp_allowed_clients"); }); - it("edits a stored client's value in place and saves the new value", async () => { + it("edits a stored client's value through its dialog and saves the new value", async () => { vi.mocked(getGeneralSettingsCall).mockResolvedValue([ { field_name: "mcp_allowed_clients", field_value: [ANTIGRAVITY] }, ]); renderSettings(); - fireEvent.change(await screen.findByRole("textbox", { name: "Client 1 value" }), { - target: { value: "0oa1b2c3d4e5f6g7h8i9" }, + await screen.findByText("Allowed Clients"); + await userEvent.click(clientCard("Antigravity CLI")); + const dialog = await screen.findByRole("dialog"); + fireEvent.change(within(dialog).getByRole("textbox", { name: "Value" }), { + target: { value: " 0oa1b2c3d4e5f6g7h8i9 " }, }); + await userEvent.click(within(dialog).getByRole("button", { name: "Done" })); + + await waitFor(() => expect(screen.queryByRole("dialog")).not.toBeInTheDocument()); + expect(clientCard("Antigravity CLI")).toHaveTextContent("0oa1b2c3d4e5f6g7h8i9"); await userEvent.click(screen.getByRole("button", { name: /Save/ })); await waitFor(() => @@ -221,22 +250,37 @@ describe("MCPNetworkSettings", () => { ); }); - it("refuses to save a client that has an alias but no value, and reports why", async () => { + it("keeps a stored client untouched when its dialog is cancelled", async () => { + vi.mocked(getGeneralSettingsCall).mockResolvedValue([ + { field_name: "mcp_allowed_clients", field_value: [ANTIGRAVITY] }, + ]); + + renderSettings(); + await screen.findByText("Allowed Clients"); + await userEvent.click(clientCard("Antigravity CLI")); + const dialog = await screen.findByRole("dialog"); + fireEvent.change(within(dialog).getByRole("textbox", { name: "Value" }), { target: { value: "changed" } }); + await userEvent.click(within(dialog).getByRole("button", { name: "Cancel" })); + + await waitFor(() => expect(screen.queryByRole("dialog")).not.toBeInTheDocument()); + expect(clientCard("Antigravity CLI")).toHaveTextContent("antigravity-cli"); + await userEvent.click(screen.getByRole("button", { name: /Save/ })); + + await waitFor(() => expect(toast.success).toHaveBeenCalledWith("MCP network settings saved")); + expect(updateConfigFieldSetting).not.toHaveBeenCalled(); + }); + + it("will not add a client that has an alias but no value", async () => { renderSettings(); await screen.findByText("Allowed Clients"); - await addClient("Antigravity CLI", ""); - await userEvent.click(screen.getByRole("button", { name: /Save/ })); + await userEvent.click(screen.getByRole("button", { name: "Add client" })); + const dialog = await fillClientDialog("Antigravity CLI", " "); - await waitFor(() => - expect(toast.fromError).toHaveBeenCalledWith(new Error("Every allowed client needs both an alias and a value")), - ); - expect(updateConfigFieldSetting).not.toHaveBeenCalled(); - expect(deleteConfigFieldSetting).not.toHaveBeenCalled(); - expect(toast.success).not.toHaveBeenCalled(); + expect(within(dialog).getByRole("button", { name: "Add" })).toBeDisabled(); }); - it("drops rows left completely blank instead of saving or failing on them", async () => { + it("adds nothing when the add dialog is cancelled", async () => { vi.mocked(getGeneralSettingsCall).mockResolvedValue([ { field_name: "mcp_allowed_clients", field_value: [ANTIGRAVITY] }, ]); @@ -244,6 +288,11 @@ describe("MCPNetworkSettings", () => { renderSettings(); await screen.findByText("Allowed Clients"); await userEvent.click(screen.getByRole("button", { name: "Add client" })); + const dialog = await fillClientDialog("Codex", "codex-mcp-client"); + await userEvent.click(within(dialog).getByRole("button", { name: "Cancel" })); + + await waitFor(() => expect(screen.queryByRole("dialog")).not.toBeInTheDocument()); + expect(screen.queryByText("Codex")).not.toBeInTheDocument(); await userEvent.click(screen.getByRole("button", { name: /Save/ })); await waitFor(() => expect(toast.success).toHaveBeenCalledWith("MCP network settings saved")); @@ -260,13 +309,17 @@ describe("MCPNetworkSettings", () => { ]); renderSettings(); - await userEvent.click(await screen.findByRole("button", { name: "Remove client Claude Code" })); + await screen.findByText("Allowed Clients"); + await userEvent.click(clientCard("Claude Code")); + await userEvent.click(within(await screen.findByRole("dialog")).getByRole("button", { name: "Remove client" })); + + await waitFor(() => expect(screen.queryByRole("dialog")).not.toBeInTheDocument()); + expect(screen.queryByText("claude-code")).not.toBeInTheDocument(); await userEvent.click(screen.getByRole("button", { name: /Save/ })); await waitFor(() => expect(updateConfigFieldSetting).toHaveBeenCalledWith("tok", "mcp_allowed_clients", [ANTIGRAVITY, CODEX]), ); - expect(screen.queryByDisplayValue("claude-code")).not.toBeInTheDocument(); }); it("removes a client and clears the setting when the list becomes empty", async () => { @@ -275,9 +328,12 @@ describe("MCPNetworkSettings", () => { ]); renderSettings(); - await userEvent.click(await screen.findByRole("button", { name: "Remove client Claude Code" })); + await screen.findByText("Allowed Clients"); + await userEvent.click(clientCard("Claude Code")); + await userEvent.click(within(await screen.findByRole("dialog")).getByRole("button", { name: "Remove client" })); - expect(screen.queryByDisplayValue("claude-code")).not.toBeInTheDocument(); + await waitFor(() => expect(screen.queryByRole("dialog")).not.toBeInTheDocument()); + expect(screen.queryByText("claude-code")).not.toBeInTheDocument(); await userEvent.click(screen.getByRole("button", { name: /Save/ })); @@ -304,7 +360,7 @@ describe("MCPNetworkSettings", () => { renderSettings(); - await screen.findByText("Allowed Client Applications"); + await screen.findByText("Allowed Clients"); expect(screen.queryByText(/every client is denied/)).not.toBeInTheDocument(); }); diff --git a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/MCPNetworkSettings.tsx b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/MCPNetworkSettings.tsx index ae1fad36599..db45cfdedd1 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/MCPNetworkSettings.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/MCPNetworkSettings.tsx @@ -1,9 +1,18 @@ -import React, { useState, useEffect } from "react"; +import React, { useState, useEffect, useId } from "react"; import { Save, Plus, X } from "lucide-react"; import { Badge } from "@/components/ui/badge"; import { Button } from "@/components/ui/button"; import { Card } from "@/components/ui/card"; +import { + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, +} from "@/components/ui/dialog"; import { Input } from "@/components/ui/input"; +import { Label } from "@/components/ui/label"; import { UiLoadingSpinner } from "@/components/ui/ui-loading-spinner"; import { DeprecationBanner } from "@/components/DeprecationBanner"; import { toast } from "@/lib/toast"; @@ -36,6 +45,10 @@ interface AllowedClientRow extends AllowedClient { readonly key: string; } +interface ClientDraft extends AllowedClient { + readonly key: string | null; +} + const isAllowedClient = (entry: unknown): entry is AllowedClient => { if (typeof entry !== "object" || entry === null) return false; const { alias, value } = entry as Partial>; @@ -58,14 +71,10 @@ const parseStoredClients = (fieldValue: unknown): StoredAllowlist => { }; let nextRowKey = 0; -const newRow = (client: AllowedClient = { alias: "", value: "" }): AllowedClientRow => ({ - ...client, - key: `client-${nextRowKey++}`, -}); +const newRow = (client: AllowedClient): AllowedClientRow => ({ ...client, key: `client-${nextRowKey++}` }); const trimClient = ({ alias, value }: AllowedClient): AllowedClient => ({ alias: alias.trim(), value: value.trim() }); -const isBlank = ({ alias, value }: AllowedClient) => alias === "" && value === ""; const isIncomplete = ({ alias, value }: AllowedClient) => alias === "" || value === ""; const sameList = (a: string[], b: string[]) => a.length === b.length && a.every((value, i) => value === b[i]); @@ -90,6 +99,67 @@ const clientsUnchangedSinceLoad = (value: AllowedClient[], stored: StoredAllowli const headerUnchangedSinceLoad = (value: string, stored: string | null) => stored === null ? value === "" : value !== "" && value === stored; +interface AllowedClientDialogProps { + readonly draft: ClientDraft | null; + readonly onChange: (draft: ClientDraft) => void; + readonly onCommit: () => void; + readonly onRemove: () => void; + readonly onClose: () => void; +} + +const AllowedClientDialog: React.FC = ({ draft, onChange, onCommit, onRemove, onClose }) => { + const aliasId = useId(); + const valueId = useId(); + if (draft === null) return null; + return ( + !open && onClose()}> + + + {draft.key === null ? "Add client" : "Edit client"} + + The alias is the name shown in the dashboard and gateway logs. The value is the exact JWT claim or header + value that identifies the client, such as the OAuth client ID your identity provider issues. + + +
+
+ + onChange({ ...draft, alias: e.target.value })} + /> +
+
+ + onChange({ ...draft, value: e.target.value })} + /> +
+
+ + {draft.key !== null && ( + + )} + + + + + + ); +}; + const MCPNetworkSettings: React.FC = ({ accessToken }) => { const [loading, setLoading] = useState(true); const [saving, setSaving] = useState(false); @@ -101,6 +171,7 @@ const MCPNetworkSettings: React.FC = ({ accessToken }) const [storedClientIdHeader, setStoredClientIdHeader] = useState(null); const [currentIp, setCurrentIp] = useState(null); const [rangeDraft, setRangeDraft] = useState(""); + const [clientDraft, setClientDraft] = useState(null); useEffect(() => { loadSettings(); @@ -154,10 +225,7 @@ const MCPNetworkSettings: React.FC = ({ accessToken }) }; const persistAllowedClients = async (token: string) => { - const clients = allowedClients.map(trimClient).filter((client) => !isBlank(client)); - if (clients.some(isIncomplete)) { - throw new Error("Every allowed client needs both an alias and a value"); - } + const clients = allowedClients.map(({ alias, value }) => ({ alias, value })); if (clientsUnchangedSinceLoad(clients, storedClients)) return; if (clients.length > 0) { await updateConfigFieldSetting(token, "mcp_allowed_clients", clients); @@ -218,10 +286,22 @@ const MCPNetworkSettings: React.FC = ({ accessToken }) setRangeDraft(""); }; - const updateClient = (key: string, patch: Partial) => - setAllowedClients(allowedClients.map((row) => (row.key === key ? { ...row, ...patch } : row))); + const commitClientDraft = () => { + if (clientDraft === null) return; + const client = trimClient(clientDraft); + setAllowedClients( + clientDraft.key === null + ? [...allowedClients, newRow(client)] + : allowedClients.map((row) => (row.key === clientDraft.key ? { ...row, ...client } : row)), + ); + setClientDraft(null); + }; - const removeClient = (key: string) => setAllowedClients(allowedClients.filter((row) => row.key !== key)); + const removeDraftedClient = () => { + if (clientDraft === null) return; + setAllowedClients(allowedClients.filter((row) => row.key !== clientDraft.key)); + setClientDraft(null); + }; if (loading) { return ( @@ -307,7 +387,7 @@ const MCPNetworkSettings: React.FC = ({ accessToken })
-

Allowed Client Applications

+

Allowed Clients

Only the MCP client applications listed here can use the gateway. Leave empty to allow every client. A client that authenticates with a JWT is identified by the claim named in litellm_jwtauth.mcp_client_id_jwt_field in @@ -317,9 +397,6 @@ const MCPNetworkSettings: React.FC = ({ accessToken })

-
-

Allowed Clients

-
{storedAllowlistIsMalformed && (

The stored allowlist is not a list of alias and value pairs, so every client is denied. Add the clients you @@ -333,35 +410,17 @@ const MCPNetworkSettings: React.FC = ({ accessToken })

)} {allowedClients.length > 0 && ( -
-

Alias

-

Value

- - {allowedClients.map((row, index) => ( - - updateClient(row.key, { alias: e.target.value })} - /> - updateClient(row.key, { value: e.target.value })} - /> - - +
+ {allowedClients.map((row) => ( + ))}
)} @@ -369,16 +428,14 @@ const MCPNetworkSettings: React.FC = ({ accessToken }) type="button" variant="outline" size="sm" - onClick={() => setAllowedClients([...allowedClients, newRow()])} + onClick={() => setClientDraft({ key: null, alias: "", value: "" })} > Add client

- The alias is the name shown here and in gateway logs. The value is the exact JWT claim or header value that - identifies the client, such as the OAuth client ID your identity provider issues. Leave the list empty to - allow every client. Every MCP request from an unlisted client, or from one with no resolvable identity, gets a - 403. + Click a client to edit or remove it. Leave the list empty to allow every client. Every MCP request from an + unlisted client, or from one with no resolvable identity, gets a 403.

@@ -403,6 +460,14 @@ const MCPNetworkSettings: React.FC = ({ accessToken }) Save
+ + setClientDraft(null)} + />
); };