From 3fea7fd2fc4e719dcf1c2f2b62a931aeddc114a7 Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Mon, 18 Aug 2025 12:33:27 -0400 Subject: [PATCH] feat: improve MCP config UI with comprehensive add/edit/delete functionality (#10145) Co-authored-by: OpenHands --- .openhands/microagents/repo.md | 29 + .../mcp-server-form.validation.test.tsx | 110 + .../__tests__/mcp-server-list.test.tsx | 158 + .../mcp-settings/mcp-config-editor.tsx | 78 - .../settings/mcp-settings/mcp-json-editor.tsx | 139 - .../settings/mcp-settings/mcp-server-form.tsx | 376 + .../mcp-settings/mcp-server-list-item.tsx | 110 + .../settings/mcp-settings/mcp-server-list.tsx | 71 + .../src/hooks/mutation/use-add-mcp-server.ts | 67 + .../hooks/mutation/use-delete-mcp-server.ts | 37 + .../hooks/mutation/use-update-mcp-server.ts | 69 + frontend/src/i18n/declaration.ts | 29 + frontend/src/i18n/translation.json | 25428 ++++++++-------- frontend/src/routes/mcp-settings.tsx | 223 +- frontend/src/services/settings.ts | 1 + frontend/src/types/settings.ts | 7 + 16 files changed, 14174 insertions(+), 12758 deletions(-) create mode 100644 frontend/src/components/features/settings/mcp-settings/__tests__/mcp-server-form.validation.test.tsx create mode 100644 frontend/src/components/features/settings/mcp-settings/__tests__/mcp-server-list.test.tsx delete mode 100644 frontend/src/components/features/settings/mcp-settings/mcp-config-editor.tsx delete mode 100644 frontend/src/components/features/settings/mcp-settings/mcp-json-editor.tsx create mode 100644 frontend/src/components/features/settings/mcp-settings/mcp-server-form.tsx create mode 100644 frontend/src/components/features/settings/mcp-settings/mcp-server-list-item.tsx create mode 100644 frontend/src/components/features/settings/mcp-settings/mcp-server-list.tsx create mode 100644 frontend/src/hooks/mutation/use-add-mcp-server.ts create mode 100644 frontend/src/hooks/mutation/use-delete-mcp-server.ts create mode 100644 frontend/src/hooks/mutation/use-update-mcp-server.ts diff --git a/.openhands/microagents/repo.md b/.openhands/microagents/repo.md index 4e420a11f6..78cf1b1dd8 100644 --- a/.openhands/microagents/repo.md +++ b/.openhands/microagents/repo.md @@ -144,6 +144,35 @@ Your specialized knowledge and instructions here... - Add the setting to the `Settings` model in `openhands/storage/data_models/settings.py` - Update any relevant backend code to apply the setting (e.g., in session creation) +#### Settings UI Patterns: + +There are two main patterns for saving settings in the OpenHands frontend: + +**Pattern 1: Entity-based Resources (Immediate Save)** +- Used for: API Keys, Secrets, MCP Servers +- Behavior: Changes are saved immediately when user performs actions (add/edit/delete) +- Implementation: + - No "Save Changes" button + - No local state management or `isDirty` tracking + - Uses dedicated mutation hooks for each operation (e.g., `use-add-mcp-server.ts`, `use-delete-mcp-server.ts`) + - Each mutation triggers immediate API call with query invalidation for UI updates + - Example: MCP settings, API Keys & Secrets tabs +- Benefits: Simpler UX, no risk of losing changes, consistent with modern web app patterns + +**Pattern 2: Form-based Settings (Manual Save)** +- Used for: Application settings, LLM configuration +- Behavior: Changes are accumulated locally and saved when user clicks "Save Changes" +- Implementation: + - Has "Save Changes" button that becomes enabled when changes are detected + - Uses local state management with `isDirty` tracking + - Uses `useSaveSettings` hook to save all changes at once + - Example: LLM tab, Application tab +- Benefits: Allows bulk changes, explicit save action, can validate all fields before saving + +**When to use each pattern:** +- Use Pattern 1 (Immediate Save) for entity management where each item is independent +- Use Pattern 2 (Manual Save) for configuration forms where settings are interdependent or need validation + ### Adding New LLM Models To add a new LLM model to OpenHands, you need to update multiple files across both frontend and backend: diff --git a/frontend/src/components/features/settings/mcp-settings/__tests__/mcp-server-form.validation.test.tsx b/frontend/src/components/features/settings/mcp-settings/__tests__/mcp-server-form.validation.test.tsx new file mode 100644 index 0000000000..a2546ac15c --- /dev/null +++ b/frontend/src/components/features/settings/mcp-settings/__tests__/mcp-server-form.validation.test.tsx @@ -0,0 +1,110 @@ +import { render, screen, fireEvent } from "@testing-library/react"; +import { describe, it, expect, vi } from "vitest"; +import { MCPServerForm } from "../mcp-server-form"; + +// i18n mock +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ + t: (key: string) => key, + }), +})); + +describe("MCPServerForm validation", () => { + const noop = () => {}; + + it("rejects invalid env var lines and allows blank lines", () => { + const onSubmit = vi.fn(); + + render( + , + ); + + // Fill required fields + fireEvent.change(screen.getByTestId("name-input"), { + target: { value: "my-server" }, + }); + fireEvent.change(screen.getByTestId("command-input"), { + target: { value: "npx" }, + }); + + // Invalid env entries mixed with blank lines + fireEvent.change(screen.getByTestId("env-input"), { + target: { value: "invalid\n\nKEY=value\n=novalue\nKEY_ONLY=" }, + }); + + fireEvent.click(screen.getByTestId("submit-button")); + + // Should show invalid env format error + expect( + screen.getByText("SETTINGS$MCP_ERROR_ENV_INVALID_FORMAT"), + ).toBeInTheDocument(); + + // Fix env with valid lines and blank lines + fireEvent.change(screen.getByTestId("env-input"), { + target: { value: "KEY=value\n\nANOTHER=123" }, + }); + + fireEvent.click(screen.getByTestId("submit-button")); + + // No error; submit should be called + expect(onSubmit).toHaveBeenCalledTimes(1); + }); + + it("rejects duplicate URLs across sse/shttp types", () => { + const onSubmit = vi.fn(); + + const existingServers = [ + { id: "sse-1", type: "sse" as const, url: "https://api.example.com" }, + { id: "shttp-1", type: "shttp" as const, url: "https://x.example.com" }, + ]; + + const r1 = render( + , + ); + + fireEvent.change(screen.getAllByTestId("url-input")[0], { + target: { value: "https://api.example.com" }, + }); + + fireEvent.click(screen.getAllByTestId("submit-button")[0]); + expect( + screen.getByText("SETTINGS$MCP_ERROR_URL_DUPLICATE"), + ).toBeInTheDocument(); + + // Unmount first form, then check shttp duplicate + r1.unmount(); + + const r2 = render( + , + ); + + fireEvent.change(screen.getAllByTestId("url-input")[0], { + target: { value: "https://api.example.com" }, + }); + + fireEvent.click(screen.getAllByTestId("submit-button")[0]); + expect( + screen.getByText("SETTINGS$MCP_ERROR_URL_DUPLICATE"), + ).toBeInTheDocument(); + + r2.unmount(); + }); +}); diff --git a/frontend/src/components/features/settings/mcp-settings/__tests__/mcp-server-list.test.tsx b/frontend/src/components/features/settings/mcp-settings/__tests__/mcp-server-list.test.tsx new file mode 100644 index 0000000000..4e1c4fa986 --- /dev/null +++ b/frontend/src/components/features/settings/mcp-settings/__tests__/mcp-server-list.test.tsx @@ -0,0 +1,158 @@ +import { render, screen } from "@testing-library/react"; +import { describe, it, expect, vi } from "vitest"; +import { MCPServerList } from "../mcp-server-list"; + +// Mock react-i18next +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ + t: (key: string) => key, + }), +})); + +const mockServers = [ + { + id: "sse-0", + type: "sse" as const, + url: "https://very-long-url-that-could-cause-layout-overflow.example.com/api/v1/mcp/server/endpoint/with/many/path/segments", + }, + { + id: "stdio-0", + type: "stdio" as const, + name: "test-stdio-server", + command: "python", + args: ["-m", "test_server"], + }, +]; + +describe("MCPServerList", () => { + it("should render servers with proper layout structure", () => { + const mockOnEdit = vi.fn(); + const mockOnDelete = vi.fn(); + + render( + , + ); + + // Check that the table structure is rendered + const table = screen.getByRole("table"); + expect(table).toBeInTheDocument(); + expect(table).toHaveClass("w-full"); + + // Check that server items are rendered + const serverItems = screen.getAllByTestId("mcp-server-item"); + expect(serverItems).toHaveLength(2); + + // Check that action buttons are present for each server + const editButtons = screen.getAllByTestId("edit-mcp-server-button"); + const deleteButtons = screen.getAllByTestId("delete-mcp-server-button"); + expect(editButtons).toHaveLength(2); + expect(deleteButtons).toHaveLength(2); + }); + + it("should render empty state when no servers", () => { + const mockOnEdit = vi.fn(); + const mockOnDelete = vi.fn(); + + render( + , + ); + + expect(screen.getByText("SETTINGS$MCP_NO_SERVERS")).toBeInTheDocument(); + }); + + it("should handle long URLs without breaking layout", () => { + const longUrlServer = { + id: "sse-0", + type: "sse" as const, + url: "https://extremely-long-url-that-would-previously-cause-layout-overflow-and-push-action-buttons-out-of-view.example.com/api/v1/mcp/server/endpoint/with/many/path/segments/and/query/parameters?param1=value1¶m2=value2¶m3=value3", + }; + + const mockOnEdit = vi.fn(); + const mockOnDelete = vi.fn(); + + render( + , + ); + + // Check that action buttons are still present and accessible + const editButton = screen.getByTestId("edit-mcp-server-button"); + const deleteButton = screen.getByTestId("delete-mcp-server-button"); + + expect(editButton).toBeInTheDocument(); + expect(deleteButton).toBeInTheDocument(); + + // Check that the URL is properly displayed with title attribute for accessibility + const detailsCells = screen.getAllByTitle(longUrlServer.url); + expect(detailsCells).toHaveLength(2); // Name and Details columns both have the URL + + // Check that both name and details cells use truncation and have title for tooltip + const [nameCell, detailsCell] = detailsCells; + expect(nameCell).toHaveClass("truncate"); + expect(detailsCell).toHaveClass("truncate"); + }); + + it("should display command and arguments for STDIO servers", () => { + const stdioServer = { + id: "stdio-1", + type: "stdio" as const, + name: "test-server", + command: "python", + args: ["-m", "test_module", "--verbose"], + }; + + const mockOnEdit = vi.fn(); + const mockOnDelete = vi.fn(); + + render( + , + ); + + // Check that the server details show command + arguments + const expectedDetails = "python -m test_module --verbose"; + expect(screen.getByTitle(expectedDetails)).toBeInTheDocument(); + expect(screen.getByText(expectedDetails)).toBeInTheDocument(); + }); + + it("should fallback to server name for STDIO servers without command", () => { + const stdioServer = { + id: "stdio-2", + type: "stdio" as const, + name: "fallback-server", + }; + + const mockOnEdit = vi.fn(); + const mockOnDelete = vi.fn(); + + render( + , + ); + + // Check that the server details show the server name as fallback + // Both name and details columns will have the same value, so we expect 2 elements + const fallbackElements = screen.getAllByTitle("fallback-server"); + expect(fallbackElements).toHaveLength(2); + + const fallbackTextElements = screen.getAllByText("fallback-server"); + expect(fallbackTextElements).toHaveLength(2); + }); +}); diff --git a/frontend/src/components/features/settings/mcp-settings/mcp-config-editor.tsx b/frontend/src/components/features/settings/mcp-settings/mcp-config-editor.tsx deleted file mode 100644 index 37c47a22a5..0000000000 --- a/frontend/src/components/features/settings/mcp-settings/mcp-config-editor.tsx +++ /dev/null @@ -1,78 +0,0 @@ -import React, { useState } from "react"; -import { useTranslation } from "react-i18next"; -import { MCPConfig } from "#/types/settings"; -import { I18nKey } from "#/i18n/declaration"; -import { MCPSSEServers } from "./mcp-sse-servers"; -import { MCPStdioServers } from "./mcp-stdio-servers"; -import { MCPJsonEditor } from "./mcp-json-editor"; -import { BrandButton } from "../brand-button"; - -interface MCPConfigEditorProps { - mcpConfig?: MCPConfig; - onChange: (config: MCPConfig) => void; -} - -export function MCPConfigEditor({ mcpConfig, onChange }: MCPConfigEditorProps) { - const { t } = useTranslation(); - const [isEditing, setIsEditing] = useState(false); - const handleConfigChange = (newConfig: MCPConfig) => { - onChange(newConfig); - setIsEditing(false); - }; - - const config = mcpConfig || { sse_servers: [], stdio_servers: [] }; - - return ( -
-
-
- {t(I18nKey.SETTINGS$MCP_TITLE)} -
-

- {t(I18nKey.SETTINGS$MCP_DESCRIPTION)} -

-
- {!isEditing && ( -
-
- setIsEditing(true)} - > - {t(I18nKey.SETTINGS$MCP_EDIT_CONFIGURATION)} - -
-
- )} -
- {isEditing ? ( - setIsEditing(false)} - /> - ) : ( - <> -
-
- -
- -
- -
-
- - {config.sse_servers.length === 0 && - config.stdio_servers.length === 0 && ( -
- {t(I18nKey.SETTINGS$MCP_NO_SERVERS_CONFIGURED)} -
- )} - - )} -
-
- ); -} diff --git a/frontend/src/components/features/settings/mcp-settings/mcp-json-editor.tsx b/frontend/src/components/features/settings/mcp-settings/mcp-json-editor.tsx deleted file mode 100644 index dadaaa2e64..0000000000 --- a/frontend/src/components/features/settings/mcp-settings/mcp-json-editor.tsx +++ /dev/null @@ -1,139 +0,0 @@ -import React, { useState, useRef, useEffect } from "react"; -import { useTranslation, Trans } from "react-i18next"; -import { MCPConfig } from "#/types/settings"; -import { I18nKey } from "#/i18n/declaration"; -import { BrandButton } from "../brand-button"; -import { cn } from "#/utils/utils"; - -interface MCPJsonEditorProps { - mcpConfig?: MCPConfig; - onChange: (config: MCPConfig) => void; - onCancel: () => void; -} - -const MCP_DEFAULT_CONFIG: MCPConfig = { - sse_servers: [], - stdio_servers: [], -}; - -export function MCPJsonEditor({ - mcpConfig, - onChange, - onCancel, -}: MCPJsonEditorProps) { - const { t } = useTranslation(); - const [configText, setConfigText] = useState(() => - mcpConfig - ? JSON.stringify(mcpConfig, null, 2) - : JSON.stringify(MCP_DEFAULT_CONFIG, null, 2), - ); - - const [error, setError] = useState(null); - - const textareaRef = useRef(null); - - useEffect(() => { - textareaRef.current?.focus(); - }, []); - - const handleTextChange = (e: React.ChangeEvent) => { - setConfigText(e.target.value); - }; - - const handleSave = () => { - try { - const newConfig = JSON.parse(configText); - - // Validate the structure - if (!newConfig.sse_servers || !Array.isArray(newConfig.sse_servers)) { - throw new Error(t(I18nKey.SETTINGS$MCP_ERROR_SSE_ARRAY)); - } - - if (!newConfig.stdio_servers || !Array.isArray(newConfig.stdio_servers)) { - throw new Error(t(I18nKey.SETTINGS$MCP_ERROR_STDIO_ARRAY)); - } - - // Validate SSE servers - for (const server of newConfig.sse_servers) { - if ( - typeof server !== "string" && - (!server.url || typeof server.url !== "string") - ) { - throw new Error(t(I18nKey.SETTINGS$MCP_ERROR_SSE_URL)); - } - } - - // Validate stdio servers - for (const server of newConfig.stdio_servers) { - if (!server.name || !server.command) { - throw new Error(t(I18nKey.SETTINGS$MCP_ERROR_STDIO_PROPS)); - } - } - - onChange(newConfig); - setError(null); - } catch (e) { - setError( - e instanceof Error - ? e.message - : t(I18nKey.SETTINGS$MCP_ERROR_INVALID_JSON), - ); - } - }; - - return ( -
-

- - documentation - - ), - }} - /> -

-