diff --git a/clients/web/package-lock.json b/clients/web/package-lock.json index 73bde734a..02ec7f3ba 100644 --- a/clients/web/package-lock.json +++ b/clients/web/package-lock.json @@ -8,6 +8,9 @@ "name": "@modelcontextprotocol/inspector-web", "version": "0.0.0", "dependencies": { + "@dnd-kit/core": "^6.3.1", + "@dnd-kit/sortable": "^8.0.0", + "@dnd-kit/utilities": "^3.2.2", "@emotion/react": "^11.14.0", "@hono/node-server": "^1.19.14", "@mantine/core": "^8.3.17", @@ -350,6 +353,59 @@ "storybook": "^0.0.0-0 || ^10.1.0 || ^10.1.0-0 || ^10.2.0-0 || ^10.3.0-0" } }, + "node_modules/@dnd-kit/accessibility": { + "version": "3.1.1", + "resolved": "https://registry.npmjs.org/@dnd-kit/accessibility/-/accessibility-3.1.1.tgz", + "integrity": "sha512-2P+YgaXF+gRsIihwwY1gCsQSYnu9Zyj2py8kY5fFvUM1qm2WA2u639R6YNVfU4GWr+ZM5mqEsfHZZLoRONbemw==", + "license": "MIT", + "dependencies": { + "tslib": "^2.0.0" + }, + "peerDependencies": { + "react": ">=16.8.0" + } + }, + "node_modules/@dnd-kit/core": { + "version": "6.3.1", + "resolved": "https://registry.npmjs.org/@dnd-kit/core/-/core-6.3.1.tgz", + "integrity": "sha512-xkGBRQQab4RLwgXxoqETICr6S5JlogafbhNsidmrkVv2YRs5MLwpjoF2qpiGjQt8S9AoxtIV603s0GIUpY5eYQ==", + "license": "MIT", + "dependencies": { + "@dnd-kit/accessibility": "^3.1.1", + "@dnd-kit/utilities": "^3.2.2", + "tslib": "^2.0.0" + }, + "peerDependencies": { + "react": ">=16.8.0", + "react-dom": ">=16.8.0" + } + }, + "node_modules/@dnd-kit/sortable": { + "version": "8.0.0", + "resolved": "https://registry.npmjs.org/@dnd-kit/sortable/-/sortable-8.0.0.tgz", + "integrity": "sha512-U3jk5ebVXe1Lr7c2wU7SBZjcWdQP+j7peHJfCspnA81enlu88Mgd7CC8Q+pub9ubP7eKVETzJW+IBAhsqbSu/g==", + "license": "MIT", + "dependencies": { + "@dnd-kit/utilities": "^3.2.2", + "tslib": "^2.0.0" + }, + "peerDependencies": { + "@dnd-kit/core": "^6.1.0", + "react": ">=16.8.0" + } + }, + "node_modules/@dnd-kit/utilities": { + "version": "3.2.2", + "resolved": "https://registry.npmjs.org/@dnd-kit/utilities/-/utilities-3.2.2.tgz", + "integrity": "sha512-+MKAJEOfaBe5SmV6t34p80MMKhjvUz0vRrvVJbPT0WElzaOJ/1xs+D+KDv+tD/NE5ujfrChEcshd4fLn0wpiqg==", + "license": "MIT", + "dependencies": { + "tslib": "^2.0.0" + }, + "peerDependencies": { + "react": ">=16.8.0" + } + }, "node_modules/@emnapi/core": { "version": "1.9.0", "resolved": "https://registry.npmjs.org/@emnapi/core/-/core-1.9.0.tgz", diff --git a/clients/web/package.json b/clients/web/package.json index a2f51301f..fd13fb1bc 100644 --- a/clients/web/package.json +++ b/clients/web/package.json @@ -23,6 +23,9 @@ "test:integration:watch": "npm run test-servers:build && vitest --project=integration" }, "dependencies": { + "@dnd-kit/core": "^6.3.1", + "@dnd-kit/sortable": "^8.0.0", + "@dnd-kit/utilities": "^3.2.2", "@emotion/react": "^11.14.0", "@hono/node-server": "^1.19.14", "@mantine/core": "^8.3.17", diff --git a/clients/web/src/App.css b/clients/web/src/App.css index 916d65785..8028d0e77 100644 --- a/clients/web/src/App.css +++ b/clients/web/src/App.css @@ -194,3 +194,17 @@ max-width: 100%; height: auto; } + +/* + * Reorder grip on ServerCard. The grab/grabbing cursor is a draggability + * affordance that can't be expressed as a Mantine prop (and the active-press + * state is a pseudo-selector), so the whole grab-cursor unit lives here rather + * than split across the theme. + */ +.server-drag-handle { + cursor: grab; +} + +.server-drag-handle:active { + cursor: grabbing; +} diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 40bc883c0..2c2832127 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -433,6 +433,7 @@ function App() { updateServer, updateServerSettings, removeServer, + reorderServers, } = useServers({ baseUrl: typeof window !== "undefined" @@ -2149,6 +2150,20 @@ function App() { const target = servers.find((s) => s.id === id); if (target) setRemoveTarget(target); }} + onServerReorder={(orderedIds) => { + // reorderServers reverts the optimistic order via an internal + // refresh() and re-throws on failure (409 from a racing external + // edit, or a network error). Surface that to the user so the drag + // doesn't silently bounce back — matching the toast pattern every + // other mutation here uses. + reorderServers(orderedIds).catch((err: unknown) => { + notifications.show({ + title: "Failed to reorder servers", + message: err instanceof Error ? err.message : String(err), + color: "red", + }); + }); + }} serverSupportsTaskToolCalls={ !!capabilities?.tasks?.requests?.tools?.call } diff --git a/clients/web/src/components/groups/ServerCard/ServerCard.test.tsx b/clients/web/src/components/groups/ServerCard/ServerCard.test.tsx index 02cd05fd8..6ac745bf6 100644 --- a/clients/web/src/components/groups/ServerCard/ServerCard.test.tsx +++ b/clients/web/src/components/groups/ServerCard/ServerCard.test.tsx @@ -215,4 +215,36 @@ describe("ServerCard", () => { renderWithMantine(); expect(screen.queryByText("Connection refused")).not.toBeInTheDocument(); }); + + it("renders the dragHandle slot when provided", () => { + renderWithMantine( + grip} + />, + ); + expect(screen.getByRole("button", { name: "grip" })).toBeInTheDocument(); + }); + + it("renders no drag handle by default", () => { + renderWithMantine(); + expect( + screen.queryByRole("button", { name: "grip" }), + ).not.toBeInTheDocument(); + }); + + it("renders the dragHandle before the server name in the header", () => { + renderWithMantine( + grip} + />, + ); + const grip = screen.getByRole("button", { name: "grip" }); + const name = screen.getByText("My MCP Server"); + // DOM order: the grip precedes the name (left of it in the header row). + expect(grip.compareDocumentPosition(name)).toBe( + Node.DOCUMENT_POSITION_FOLLOWING, + ); + }); }); diff --git a/clients/web/src/components/groups/ServerCard/ServerCard.tsx b/clients/web/src/components/groups/ServerCard/ServerCard.tsx index 98d91db31..a52b68d57 100644 --- a/clients/web/src/components/groups/ServerCard/ServerCard.tsx +++ b/clients/web/src/components/groups/ServerCard/ServerCard.tsx @@ -1,3 +1,4 @@ +import type { ReactNode } from "react"; import { Badge, Button, Card, Group, Stack, Text } from "@mantine/core"; import type { MCPServerConfig, @@ -18,6 +19,14 @@ export interface ServerCardProps extends ServerEntry { onClone: (id: string) => void; onRemove: (id: string) => void; compact?: boolean; + /** + * Optional drag-handle affordance rendered at the start of the card header, + * before the server name. Supplied by the sortable wrapper + * (`SortableServerCard`); omitted when the card is rendered outside a reorder + * context, so the card stays a dumb display component with no knowledge of + * drag-and-drop. + */ + dragHandle?: ReactNode; } const HeaderLeft = Group.withProps({ @@ -103,6 +112,7 @@ export function ServerCard({ onClone, onRemove, compact = false, + dragHandle, }: ServerCardProps) { const isDimmed = activeServer !== undefined && activeServer !== id; const transport = getTransport(config); @@ -121,6 +131,7 @@ export function ServerCard({ + {dragHandle} {name} diff --git a/clients/web/src/components/groups/SortableServerCard/SortableServerCard.tsx b/clients/web/src/components/groups/SortableServerCard/SortableServerCard.tsx new file mode 100644 index 000000000..e2ee38b77 --- /dev/null +++ b/clients/web/src/components/groups/SortableServerCard/SortableServerCard.tsx @@ -0,0 +1,65 @@ +import { ActionIcon, Box } from "@mantine/core"; +import { RiDraggable } from "react-icons/ri"; +import { useSortable } from "@dnd-kit/sortable"; +import { CSS } from "@dnd-kit/utilities"; +import { ServerCard, type ServerCardProps } from "../ServerCard/ServerCard"; + +export type SortableServerCardProps = ServerCardProps; + +/** + * Sortable wrapper around the dumb `ServerCard`. Owns all drag-and-drop + * concerns (the `@dnd-kit` sortable node, the per-frame transform, and the + * grip activator) so `ServerCard` itself stays a pure display component that + * only renders the `dragHandle` slot it's handed. + * + * The grip is the sole drag activator (pointer + keyboard) — bound via + * `listeners`/`attributes` and `setActivatorNodeRef` — so the card's own + * buttons (Clone / Edit / Remove / Settings) keep working without starting a + * drag. It's passed into `ServerCard.dragHandle`, which renders it at the start + * of the header row, before the server name. + */ +export function SortableServerCard(props: SortableServerCardProps) { + const { + attributes, + listeners, + setNodeRef, + setActivatorNodeRef, + transform, + transition, + isDragging, + } = useSortable({ id: props.id }); + + const grip = ( + + + + ); + + return ( + + + + ); +} diff --git a/clients/web/src/components/screens/ServerListScreen/ServerListScreen.stories.tsx b/clients/web/src/components/screens/ServerListScreen/ServerListScreen.stories.tsx index e374a4dae..8173df7e7 100644 --- a/clients/web/src/components/screens/ServerListScreen/ServerListScreen.stories.tsx +++ b/clients/web/src/components/screens/ServerListScreen/ServerListScreen.stories.tsx @@ -1,5 +1,5 @@ import type { Meta, StoryObj } from "@storybook/react-vite"; -import { fn } from "storybook/test"; +import { expect, fn, userEvent, waitFor, within } from "storybook/test"; import type { ServerEntry } from "@inspector/core/mcp/types.js"; import { ServerListScreen } from "./ServerListScreen"; @@ -18,6 +18,7 @@ const meta: Meta = { onEdit: fn(), onClone: fn(), onRemove: fn(), + onReorder: fn(), compact: false, onToggleCompact: fn(), }, @@ -109,3 +110,42 @@ export const WithActiveServer: Story = { activeServer: connectedStdioServer.id, }, }; + +/** + * Accessible keyboard reorder: focus a card's grip, press Space to pick it up, + * an arrow key to move it, and Space again to drop. Runs in a real browser + * (via the storybook test runner) where layout rects exist for the `@dnd-kit` + * keyboard sensor — the path that's unreliable under happy-dom. At the default + * 1280px viewport the grid is three columns wide, so ArrowRight moves the + * first card one position to the right. + */ +export const KeyboardReorder: Story = { + args: { + servers: [connectedStdioServer, disconnectedStdioServer, failedHttpServer], + }, + play: async ({ canvasElement, args, step }) => { + const canvas = within(canvasElement); + const handle = await canvas.findByRole("button", { + name: "Reorder Local Dev Server", + }); + + await step("pick up the first card", async () => { + handle.focus(); + await userEvent.keyboard("[Space]"); + }); + await step("move it one position to the right", async () => { + await userEvent.keyboard("[ArrowRight]"); + }); + await step("drop it", async () => { + await userEvent.keyboard("[Space]"); + }); + + await waitFor(() => expect(args.onReorder).toHaveBeenCalled()); + // The first card swapped places with the second; the third is unmoved. + expect(args.onReorder).toHaveBeenCalledWith([ + disconnectedStdioServer.id, + connectedStdioServer.id, + failedHttpServer.id, + ]); + }, +}; diff --git a/clients/web/src/components/screens/ServerListScreen/ServerListScreen.test.tsx b/clients/web/src/components/screens/ServerListScreen/ServerListScreen.test.tsx index 3517ceafe..873a112d0 100644 --- a/clients/web/src/components/screens/ServerListScreen/ServerListScreen.test.tsx +++ b/clients/web/src/components/screens/ServerListScreen/ServerListScreen.test.tsx @@ -11,6 +11,12 @@ const servers: ServerEntry[] = [ config: { type: "stdio", command: "echo" }, connection: { status: "disconnected" }, }, + { + id: "beta", + name: "Beta", + config: { type: "stdio", command: "echo" }, + connection: { status: "disconnected" }, + }, ]; const baseProps = { @@ -25,14 +31,16 @@ const baseProps = { onEdit: vi.fn(), onClone: vi.fn(), onRemove: vi.fn(), + onReorder: vi.fn(), compact: false, onToggleCompact: vi.fn(), }; describe("ServerListScreen", () => { - it("renders the server card", () => { + it("renders the server cards", () => { renderWithMantine(); expect(screen.getByText("Alpha")).toBeInTheDocument(); + expect(screen.getByText("Beta")).toBeInTheDocument(); }); it("renders the empty state with no servers", () => { @@ -44,10 +52,43 @@ describe("ServerListScreen", () => { it("toggles compact mode when the list toggle is clicked", async () => { const user = userEvent.setup(); - renderWithMantine(); - const buttons = screen.getAllByRole("button"); - expect(buttons.length).toBeGreaterThan(0); - await user.click(buttons[0]); - expect(screen.getByText("Alpha")).toBeInTheDocument(); + const onToggleCompact = vi.fn(); + renderWithMantine( + , + ); + await user.click(screen.getByRole("button", { name: /collapse all/i })); + expect(onToggleCompact).toHaveBeenCalledTimes(1); + }); + + describe("reorder affordances", () => { + it("renders a labelled drag handle per card when onReorder is provided", () => { + renderWithMantine(); + expect( + screen.getByRole("button", { name: "Reorder Alpha" }), + ).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Reorder Beta" }), + ).toBeInTheDocument(); + }); + + it("marks each drag handle as a sortable activator (keyboard-operable)", () => { + renderWithMantine(); + const handle = screen.getByRole("button", { name: "Reorder Alpha" }); + // dnd-kit's useSortable spreads accessibility attributes onto the + // activator: a roledescription plus a non-negative tabindex so keyboard + // users can focus it and press Space to pick the card up. + expect(handle).toHaveAttribute("aria-roledescription", "sortable"); + expect(handle).toHaveAttribute("tabindex", "0"); + }); + + it("omits drag handles when onReorder is not provided", () => { + renderWithMantine( + , + ); + expect(screen.getByText("Alpha")).toBeInTheDocument(); + expect( + screen.queryByRole("button", { name: /^Reorder / }), + ).not.toBeInTheDocument(); + }); }); }); diff --git a/clients/web/src/components/screens/ServerListScreen/ServerListScreen.tsx b/clients/web/src/components/screens/ServerListScreen/ServerListScreen.tsx index 5c45004d9..393d12644 100644 --- a/clients/web/src/components/screens/ServerListScreen/ServerListScreen.tsx +++ b/clients/web/src/components/screens/ServerListScreen/ServerListScreen.tsx @@ -1,7 +1,25 @@ import { ScrollArea, SimpleGrid, Stack, Text } from "@mantine/core"; +import { + DndContext, + KeyboardSensor, + PointerSensor, + closestCenter, + useSensor, + useSensors, +} from "@dnd-kit/core"; +import { + SortableContext, + rectSortingStrategy, + sortableKeyboardCoordinates, +} from "@dnd-kit/sortable"; import type { ServerEntry } from "@inspector/core/mcp/types.js"; import { ServerCard } from "../../groups/ServerCard/ServerCard"; import { ServerListControls } from "../../groups/ServerListControls/ServerListControls"; +import { SortableServerCard } from "../../groups/SortableServerCard/SortableServerCard"; +import { + buildReorderAnnouncements, + makeServerDragEndHandler, +} from "./serverReorder"; export interface ServerListScreenProps { servers: ServerEntry[]; @@ -18,6 +36,11 @@ export interface ServerListScreenProps { onEdit: (id: string) => void; onClone: (id: string) => void; onRemove: (id: string) => void; + /** + * Persist a new server ordering. Receives the complete set of server ids in + * the desired order. Omit to render the list without reorder affordances. + */ + onReorder?: (orderedIds: string[]) => void; compact: boolean; onToggleCompact: () => void; } @@ -45,9 +68,63 @@ export function ServerListScreen({ onEdit, onClone, onRemove, + onReorder, compact, onToggleCompact, }: ServerListScreenProps) { + const sensors = useSensors( + useSensor(PointerSensor), + useSensor(KeyboardSensor, { + coordinateGetter: sortableKeyboardCoordinates, + }), + ); + + const ids = servers.map((s) => s.id); + + // `reorderable` only when a persistence callback is wired. Without it we + // render plain `ServerCard`s (no grip, no DndContext) so the screen stays + // usable as a pure display — the SortableServerCard's grip would otherwise + // be a dead affordance. + const reorderable = onReorder !== undefined; + + // Built only when reorderable — the drag-end handler and the fresh + // announcements object (four closures) are otherwise allocated every render + // for nothing. + const handleDragEnd = reorderable + ? makeServerDragEndHandler(servers, onReorder) + : undefined; + const announcements = reorderable + ? buildReorderAnnouncements(servers) + : undefined; + + const cardProps = (server: ServerEntry) => ({ + compact, + activeServer, + onToggleConnection, + onConnectionInfo, + onSettings, + onEdit, + onClone, + onRemove, + ...server, + }); + + const grid = ( + + {servers.map((server) => + reorderable ? ( + + ) : ( + + ), + )} + + ); + return ( No servers configured. Add a server to get started. - ) : ( - - {servers.map((server) => ( - - ))} - + + {grid} + + + ) : ( + grid )} diff --git a/clients/web/src/components/screens/ServerListScreen/reorderIds.test.ts b/clients/web/src/components/screens/ServerListScreen/reorderIds.test.ts new file mode 100644 index 000000000..5cd5ec23e --- /dev/null +++ b/clients/web/src/components/screens/ServerListScreen/reorderIds.test.ts @@ -0,0 +1,40 @@ +import { describe, it, expect } from "vitest"; +import { reorderIds } from "./reorderIds"; + +describe("reorderIds", () => { + const ids = ["a", "b", "c", "d"]; + + it("moves an item forward (drop onto a later id)", () => { + expect(reorderIds(ids, "a", "c")).toEqual(["b", "c", "a", "d"]); + }); + + it("moves an item backward (drop onto an earlier id)", () => { + expect(reorderIds(ids, "d", "b")).toEqual(["a", "d", "b", "c"]); + }); + + it("moves to the very front", () => { + expect(reorderIds(ids, "c", "a")).toEqual(["c", "a", "b", "d"]); + }); + + it("moves to the very end", () => { + expect(reorderIds(ids, "a", "d")).toEqual(["b", "c", "d", "a"]); + }); + + it("returns the same array reference when active === over", () => { + expect(reorderIds(ids, "b", "b")).toBe(ids); + }); + + it("returns the same array reference when active id is missing", () => { + expect(reorderIds(ids, "x", "b")).toBe(ids); + }); + + it("returns the same array reference when over id is missing", () => { + expect(reorderIds(ids, "a", "x")).toBe(ids); + }); + + it("preserves the full id set (no drops or duplicates)", () => { + const out = reorderIds(ids, "a", "d"); + expect([...out].sort()).toEqual([...ids].sort()); + expect(out.length).toBe(ids.length); + }); +}); diff --git a/clients/web/src/components/screens/ServerListScreen/reorderIds.ts b/clients/web/src/components/screens/ServerListScreen/reorderIds.ts new file mode 100644 index 000000000..f58b2c880 --- /dev/null +++ b/clients/web/src/components/screens/ServerListScreen/reorderIds.ts @@ -0,0 +1,22 @@ +import { arrayMove } from "@dnd-kit/sortable"; + +/** + * Pure reorder: move `activeId` to where `overId` sits in `ids`. Returns the + * input array unchanged (referential identity preserved) when either id is + * missing or they're identical, so a no-movement drop is a no-op. Extracted + * from `ServerListScreen` for direct unit testing — the `@dnd-kit` + * keyboard/pointer sensors don't produce measurable layout rects under + * happy-dom, so the full gesture is exercised in the Storybook play (real + * browser) while this keeps the ordering math verifiable in the unit suite. + */ +export function reorderIds( + ids: string[], + activeId: string, + overId: string, +): string[] { + if (activeId === overId) return ids; + const from = ids.indexOf(activeId); + const to = ids.indexOf(overId); + if (from === -1 || to === -1) return ids; + return arrayMove(ids, from, to); +} diff --git a/clients/web/src/components/screens/ServerListScreen/serverReorder.test.ts b/clients/web/src/components/screens/ServerListScreen/serverReorder.test.ts new file mode 100644 index 000000000..35526e1a0 --- /dev/null +++ b/clients/web/src/components/screens/ServerListScreen/serverReorder.test.ts @@ -0,0 +1,102 @@ +import { describe, it, expect, vi } from "vitest"; +import type { DragEndEvent } from "@dnd-kit/core"; +import type { ServerEntry } from "@inspector/core/mcp/types.js"; +import { + buildReorderAnnouncements, + makeServerDragEndHandler, +} from "./serverReorder"; + +const entry = (id: string, name: string): ServerEntry => ({ + id, + name, + config: { type: "stdio", command: "echo" }, + connection: { status: "disconnected" }, +}); + +const servers: ServerEntry[] = [ + entry("a", "Alpha"), + entry("b", "Beta"), + entry("c", "Gamma"), +]; + +// Minimal stand-ins for dnd-kit's active/over/event descriptors — the handlers +// only read `.id`, so we cast through `unknown` rather than constructing the +// full (sensor-populated) event shape. +const ref = (id: string) => ({ id }); +const endEvent = (activeId: string, overId: string | null): DragEndEvent => + ({ + active: ref(activeId), + over: overId === null ? null : ref(overId), + }) as unknown as DragEndEvent; + +describe("makeServerDragEndHandler", () => { + it("calls onReorder with the reordered ids when the card moved", () => { + const onReorder = vi.fn(); + makeServerDragEndHandler(servers, onReorder)(endEvent("a", "c")); + expect(onReorder).toHaveBeenCalledWith(["b", "c", "a"]); + }); + + it("does nothing when dropped on itself", () => { + const onReorder = vi.fn(); + makeServerDragEndHandler(servers, onReorder)(endEvent("a", "a")); + expect(onReorder).not.toHaveBeenCalled(); + }); + + it("does nothing when there is no drop target", () => { + const onReorder = vi.fn(); + makeServerDragEndHandler(servers, onReorder)(endEvent("a", null)); + expect(onReorder).not.toHaveBeenCalled(); + }); + + it("tolerates an absent onReorder callback (no throw)", () => { + expect(() => + makeServerDragEndHandler(servers, undefined)(endEvent("a", "b")), + ).not.toThrow(); + }); +}); + +describe("buildReorderAnnouncements", () => { + const a = buildReorderAnnouncements(servers); + + it("narrates pick-up with name and 1-based position", () => { + expect(a.onDragStart({ active: ref("b") } as never)).toBe( + "Picked up server Beta. It is in position 2 of 3.", + ); + }); + + it("narrates a move over a target", () => { + expect(a.onDragOver?.({ active: ref("a"), over: ref("c") } as never)).toBe( + "Server Alpha moved to position 3 of 3.", + ); + }); + + it("returns undefined for onDragOver when there is no target", () => { + expect( + a.onDragOver?.({ active: ref("a"), over: null } as never), + ).toBeUndefined(); + }); + + it("narrates a drop on a target", () => { + expect(a.onDragEnd?.({ active: ref("a"), over: ref("b") } as never)).toBe( + "Server Alpha dropped at position 2 of 3.", + ); + }); + + it("narrates a drop with no target", () => { + expect(a.onDragEnd?.({ active: ref("a"), over: null } as never)).toBe( + "Server Alpha dropped.", + ); + }); + + it("narrates a cancellation", () => { + expect(a.onDragCancel?.({ active: ref("c") } as never)).toBe( + "Reorder cancelled. Server Gamma returned to its original position.", + ); + }); + + it("falls back to the id when the server is unknown", () => { + expect(a.onDragStart({ active: ref("missing") } as never)).toBe( + "Picked up server missing. It is in position 0 of 3.", + ); + }); +}); diff --git a/clients/web/src/components/screens/ServerListScreen/serverReorder.ts b/clients/web/src/components/screens/ServerListScreen/serverReorder.ts new file mode 100644 index 000000000..e04a938a3 --- /dev/null +++ b/clients/web/src/components/screens/ServerListScreen/serverReorder.ts @@ -0,0 +1,74 @@ +import type { Announcements, DragEndEvent } from "@dnd-kit/core"; +import type { ServerEntry } from "@inspector/core/mcp/types.js"; +import { reorderIds } from "./reorderIds"; + +/** + * Reorder glue for `ServerListScreen`, extracted from the component so the + * announcement copy and the drag-end resolution are unit-testable directly. + * The `@dnd-kit` keyboard/pointer sensors don't yield measurable layout rects + * under happy-dom, so the live gesture can't be driven in the unit suite (it's + * covered by the Storybook play instead) — keeping this logic out of the + * component body is what lets it stay verified. + */ + +/** Display name for an id, falling back to the id when it isn't found. */ +function nameFor(servers: ServerEntry[], id: string): string { + return servers.find((s) => s.id === id)?.name ?? id; +} + +/** 1-based position of an id in the list, for "position 3 of 7" narration. */ +function positionFor(servers: ServerEntry[], id: string): number { + return servers.findIndex((s) => s.id === id) + 1; +} + +/** + * Live-region narration so keyboard / screen-reader users hear the pick-up, + * each move, and the drop. Fed to `DndContext`'s `accessibility.announcements`, + * which writes into dnd-kit's built-in `aria-live` region. + */ +export function buildReorderAnnouncements( + servers: ServerEntry[], +): Announcements { + const count = servers.length; + return { + onDragStart: ({ active }) => + `Picked up server ${nameFor(servers, String(active.id))}. It is in position ${positionFor( + servers, + String(active.id), + )} of ${count}.`, + onDragOver: ({ active, over }) => + over + ? `Server ${nameFor(servers, String(active.id))} moved to position ${positionFor( + servers, + String(over.id), + )} of ${count}.` + : undefined, + onDragEnd: ({ active, over }) => + over + ? `Server ${nameFor(servers, String(active.id))} dropped at position ${positionFor( + servers, + String(over.id), + )} of ${count}.` + : `Server ${nameFor(servers, String(active.id))} dropped.`, + onDragCancel: ({ active }) => + `Reorder cancelled. Server ${nameFor(servers, String(active.id))} returned to its original position.`, + }; +} + +/** + * Build the `DndContext.onDragEnd` handler bound to the current list and the + * persistence callback. Returns a no-op-on-no-movement handler: when the drop + * lands outside any target, on itself, or doesn't change the order, `onReorder` + * is not called. + */ +export function makeServerDragEndHandler( + servers: ServerEntry[], + onReorder: ((orderedIds: string[]) => void) | undefined, +): (event: DragEndEvent) => void { + return ({ active, over }: DragEndEvent): void => { + if (!over || active.id === over.id) return; + const ids = servers.map((s) => s.id); + const next = reorderIds(ids, String(active.id), String(over.id)); + if (next !== ids) onReorder?.(next); + }; +} diff --git a/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx b/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx index 228509b5e..c48f32bcf 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx @@ -356,6 +356,7 @@ const meta: Meta = { onServerEdit: fn(), onServerClone: fn(), onServerRemove: fn(), + onServerReorder: fn(), serverSupportsTaskToolCalls: false, onToolsUiChange: fn(), onCallTool: fn(), diff --git a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx index ae0d32650..350e0e8d2 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx @@ -88,6 +88,7 @@ function makeProps( onServerEdit: vi.fn(), onServerClone: vi.fn(), onServerRemove: vi.fn(), + onServerReorder: vi.fn(), serverSupportsTaskToolCalls: false, onToolsUiChange: vi.fn(), onCallTool: vi.fn(), diff --git a/clients/web/src/components/views/InspectorView/InspectorView.tsx b/clients/web/src/components/views/InspectorView/InspectorView.tsx index 38b213170..941992a19 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.tsx @@ -268,6 +268,8 @@ export interface InspectorViewProps { onServerEdit: (id: string) => void; onServerClone: (id: string) => void; onServerRemove: (id: string) => void; + /** Persist a new server ordering (drag-and-drop / keyboard reorder). */ + onServerReorder: (orderedIds: string[]) => void; // Per-primitive actions (route to `inspectorClient` methods / hook refresh). // Each `on{Screen}UiChange` persists that screen's lifted UI state (#1417). @@ -380,6 +382,7 @@ export function InspectorView({ onServerEdit, onServerClone, onServerRemove, + onServerReorder, serverSupportsTaskToolCalls, onToolsUiChange, onCallTool, @@ -549,6 +552,7 @@ export function InspectorView({ onEdit={onServerEdit} onClone={onServerClone} onRemove={onServerRemove} + onReorder={onServerReorder} compact={serversCompact} onToggleCompact={() => setServersCompact((c) => !c)} /> diff --git a/clients/web/src/test/core/react/useServers.test.tsx b/clients/web/src/test/core/react/useServers.test.tsx index 1288a303c..d1ffc7a6b 100644 --- a/clients/web/src/test/core/react/useServers.test.tsx +++ b/clients/web/src/test/core/react/useServers.test.tsx @@ -492,6 +492,142 @@ describe("useServers", () => { }); }); + it("reorderServers persists the new order to disk and updates the list", async () => { + writeFileSync( + h.configPath, + JSON.stringify({ + mcpServers: { + alpha: { type: "stdio", command: "a" }, + beta: { type: "stdio", command: "b" }, + gamma: { type: "stdio", command: "g" }, + }, + }), + ); + + const { result } = renderHook(() => + useServers({ baseUrl: "http://test.local", fetchFn: h.fetchFn }), + ); + await waitFor(() => + expect(result.current.servers.map((s) => s.id)).toEqual([ + "alpha", + "beta", + "gamma", + ]), + ); + + await act(async () => { + await result.current.reorderServers(["gamma", "alpha", "beta"]); + }); + + await waitFor(() => { + expect(result.current.servers.map((s) => s.id)).toEqual([ + "gamma", + "alpha", + "beta", + ]); + }); + // mcp.json map iteration order reflects the new order. + expect(Object.keys(readConfig(h.configPath).mcpServers)).toEqual([ + "gamma", + "alpha", + "beta", + ]); + }); + + it("reorderServers updates the list optimistically before the round-trip resolves", async () => { + writeFileSync( + h.configPath, + JSON.stringify({ + mcpServers: { + alpha: { type: "stdio", command: "a" }, + beta: { type: "stdio", command: "b" }, + }, + }), + ); + + let resolvePut: (() => void) | undefined; + const gatedFetch: typeof fetch = async (input, init) => { + const url = input instanceof Request ? input.url : String(input); + if (init?.method === "PUT" && url.endsWith("/api/servers/order")) { + // Hold the PUT response open so we can observe the optimistic state + // that must already be applied before the network settles. + await new Promise((r) => { + resolvePut = r; + }); + } + return h.fetchFn(input, init); + }; + + const { result } = renderHook(() => + useServers({ baseUrl: "http://test.local", fetchFn: gatedFetch }), + ); + await waitFor(() => + expect(result.current.servers.map((s) => s.id)).toEqual([ + "alpha", + "beta", + ]), + ); + + let pending: Promise; + act(() => { + pending = result.current.reorderServers(["beta", "alpha"]); + }); + + // Optimistic reorder is visible while the PUT is still in flight. + await waitFor(() => + expect(result.current.servers.map((s) => s.id)).toEqual([ + "beta", + "alpha", + ]), + ); + + await act(async () => { + resolvePut?.(); + await pending; + }); + }); + + it("reorderServers reverts to disk truth and throws when the set no longer matches (409)", async () => { + writeFileSync( + h.configPath, + JSON.stringify({ + mcpServers: { + alpha: { type: "stdio", command: "a" }, + beta: { type: "stdio", command: "b" }, + }, + }), + ); + + const { result } = renderHook(() => + useServers({ baseUrl: "http://test.local", fetchFn: h.fetchFn }), + ); + await waitFor(() => + expect(result.current.servers.map((s) => s.id)).toEqual([ + "alpha", + "beta", + ]), + ); + + // Incomplete set (missing `beta`) — the backend rejects with 409 and the + // hook re-fetches, snapping the list back to the on-disk order. + await expect( + act(async () => { + await result.current.reorderServers(["alpha"]); + }), + ).rejects.toThrow(/does not match/); + + await waitFor(() => { + expect(result.current.servers.map((s) => s.id)).toEqual([ + "alpha", + "beta", + ]); + }); + expect(Object.keys(readConfig(h.configPath).mcpServers)).toEqual([ + "alpha", + "beta", + ]); + }); + it("uses DEFAULT_SEED_CONFIG keys on the first load (seed-write contract)", async () => { const { result } = renderHook(() => useServers({ baseUrl: "http://test.local", fetchFn: h.fetchFn }), diff --git a/clients/web/src/test/integration/mcp/remote/servers-order.test.ts b/clients/web/src/test/integration/mcp/remote/servers-order.test.ts new file mode 100644 index 000000000..d95caa925 --- /dev/null +++ b/clients/web/src/test/integration/mcp/remote/servers-order.test.ts @@ -0,0 +1,177 @@ +/** + * Integration tests for PUT /api/servers/order — the reorder route added for + * #1369. Spins up createRemoteApp against a per-test tmp mcpConfigPath and + * exercises the route via real HTTP, asserting on the resulting on-disk + * iteration order and the conflict/validation rejections. + */ + +import { describe, it, expect, afterEach, beforeEach } from "vitest"; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { serve } from "@hono/node-server"; +import type { ServerType } from "@hono/node-server"; +import { createRemoteApp } from "@inspector/core/mcp/remote/node/server.js"; +import { InMemorySecretStore } from "@inspector/core/auth/node/secret-store.js"; +import type { MCPConfig } from "@inspector/core/mcp/types.js"; + +interface Harness { + baseUrl: string; + server: ServerType; + configPath: string; + tempDir: string; +} + +async function startServer( + configPath: string, +): Promise<{ baseUrl: string; server: ServerType }> { + const { app } = createRemoteApp({ + dangerouslyOmitAuth: true, + mcpConfigPath: configPath, + initialConfig: { defaultEnvironment: {} }, + secretStore: new InMemorySecretStore(), + }); + return new Promise((resolve, reject) => { + const server = serve( + { fetch: app.fetch, port: 0, hostname: "127.0.0.1" }, + (info) => { + const port = + info && typeof info === "object" && "port" in info + ? (info as { port: number }).port + : 0; + resolve({ baseUrl: `http://127.0.0.1:${port}`, server }); + }, + ); + server.on("error", reject); + }); +} + +async function setup(seed: MCPConfig): Promise { + const tempDir = mkdtempSync(join(tmpdir(), "inspector-servers-order-")); + const configPath = join(tempDir, "mcp.json"); + writeFileSync(configPath, JSON.stringify(seed, null, 2)); + const { baseUrl, server } = await startServer(configPath); + return { baseUrl, server, configPath, tempDir }; +} + +async function teardown(h: Harness): Promise { + await new Promise((resolve) => h.server.close(() => resolve())); + try { + rmSync(h.tempDir, { recursive: true }); + } catch { + /* ignore */ + } +} + +function readOrder(path: string): string[] { + const cfg = JSON.parse(readFileSync(path, "utf-8")) as MCPConfig; + return Object.keys(cfg.mcpServers); +} + +const SEED: MCPConfig = { + mcpServers: { + alpha: { type: "stdio", command: "a" }, + beta: { type: "stdio", command: "b" }, + gamma: { type: "stdio", command: "g" }, + }, +}; + +async function putOrder(baseUrl: string, body: unknown): Promise { + return fetch(`${baseUrl}/api/servers/order`, { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(body), + }); +} + +describe("PUT /api/servers/order", () => { + let h: Harness; + + beforeEach(async () => { + h = await setup(SEED); + }); + + afterEach(async () => { + await teardown(h); + }); + + it("rewrites mcp.json in the supplied order", async () => { + const res = await putOrder(h.baseUrl, { + order: ["gamma", "alpha", "beta"], + }); + expect(res.status).toBe(200); + expect(await res.json()).toEqual({ ok: true }); + expect(readOrder(h.configPath)).toEqual(["gamma", "alpha", "beta"]); + }); + + it("preserves each entry's config when reordering (values untouched)", async () => { + await putOrder(h.baseUrl, { order: ["beta", "gamma", "alpha"] }); + const cfg = JSON.parse(readFileSync(h.configPath, "utf-8")) as MCPConfig; + expect(cfg.mcpServers.alpha).toEqual({ type: "stdio", command: "a" }); + expect(cfg.mcpServers.beta).toEqual({ type: "stdio", command: "b" }); + expect(cfg.mcpServers.gamma).toEqual({ type: "stdio", command: "g" }); + }); + + it("rejects an order missing an on-disk id (409) and leaves the file unchanged", async () => { + const res = await putOrder(h.baseUrl, { order: ["alpha", "beta"] }); + expect(res.status).toBe(409); + expect(((await res.json()) as { error: string }).error).toMatch( + /does not match/, + ); + // Original order preserved. + expect(readOrder(h.configPath)).toEqual(["alpha", "beta", "gamma"]); + }); + + it("rejects an order containing an unknown id (409)", async () => { + const res = await putOrder(h.baseUrl, { + order: ["alpha", "beta", "ghost"], + }); + expect(res.status).toBe(409); + expect(readOrder(h.configPath)).toEqual(["alpha", "beta", "gamma"]); + }); + + it("rejects duplicate ids in the order (400)", async () => { + const res = await putOrder(h.baseUrl, { + order: ["alpha", "alpha", "beta"], + }); + expect(res.status).toBe(400); + expect(((await res.json()) as { error: string }).error).toMatch( + /duplicate/, + ); + expect(readOrder(h.configPath)).toEqual(["alpha", "beta", "gamma"]); + }); + + it("rejects a non-array order (400)", async () => { + const res = await putOrder(h.baseUrl, { order: "alpha,beta,gamma" }); + expect(res.status).toBe(400); + expect(((await res.json()) as { error: string }).error).toMatch( + /array of strings/, + ); + }); + + it("rejects an order array with a non-string element (400)", async () => { + const res = await putOrder(h.baseUrl, { order: ["alpha", 2, "gamma"] }); + expect(res.status).toBe(400); + }); + + it("rejects an invalid JSON body (400)", async () => { + const res = await fetch(`${h.baseUrl}/api/servers/order`, { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: "{ not json", + }); + expect(res.status).toBe(400); + }); + + it("is not captured by the :id route — 'order' is never treated as a server id", async () => { + // Regression guard for route registration order: a PUT to /order must hit + // the reorder handler, not PUT /api/servers/:id with id="order". + const res = await putOrder(h.baseUrl, { + order: ["gamma", "beta", "alpha"], + }); + expect(res.status).toBe(200); + expect(readOrder(h.configPath)).toEqual(["gamma", "beta", "alpha"]); + // No phantom "order" server was created. + expect(readOrder(h.configPath)).not.toContain("order"); + }); +}); diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 4a906a98d..70f0dd9f6 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -1514,6 +1514,67 @@ export function createRemoteApp( } }); + // Reorder the on-disk `mcpServers` map. Registered before + // `PUT /api/servers/:id` so the literal `/order` segment isn't captured + // as an `:id` param ("order" would otherwise be a valid store id). The + // request body is `{ order: string[] }` — the complete set of server ids + // in the desired iteration order. We reject (409) unless that set matches + // the on-disk set exactly, so a reorder racing an external add/remove + // can't silently drop or duplicate an entry. Reordering touches no secret + // values, so we just permute the existing stripped entries and reuse the + // same atomic-write + watcher-notify path as the other mutators. + app.put("/api/servers/order", async (c) => { + let body: { order?: unknown }; + try { + body = (await c.req.json()) as { order?: unknown }; + } catch { + return c.json({ error: "Invalid JSON body" }, 400); + } + if ( + !Array.isArray(body.order) || + !body.order.every((id): id is string => typeof id === "string") + ) { + return c.json({ error: "order must be an array of strings" }, 400); + } + const order = body.order; + if (new Set(order).size !== order.length) { + return c.json({ error: "order contains duplicate ids" }, 400); + } + + try { + return await withWriteLock(async () => { + const current = await readMcpConfig(); + const currentIds = Object.keys(current.mcpServers); + // Exact-set match: same size and every requested id present on disk. + // The duplicate check above plus equal sizes guarantees this is a + // permutation, never a partial reorder that would drop an entry. + const currentSet = new Set(currentIds); + const sameSet = + currentIds.length === order.length && + order.every((id) => currentSet.has(id)); + if (!sameSet) { + return c.json( + { + error: + "order does not match the current server set (it may have changed on disk)", + }, + 409, + ); + } + const next: MCPConfig = { mcpServers: {} }; + for (const id of order) { + // Non-null: `sameSet` proves every `id` is a key of the map. + next.mcpServers[id] = current.mcpServers[id]!; + } + await writeMcpAndTrackMtime(serializeStore(next)); + return c.json({ ok: true }); + }); + } catch (error) { + const msg = error instanceof Error ? error.message : String(error); + return c.json({ error: `Failed to reorder servers: ${msg}` }, 500); + } + }); + app.put("/api/servers/:id", async (c) => { const originalId = c.req.param("id"); if (!originalId || !validateStoreId(originalId)) { diff --git a/core/react/useServers.ts b/core/react/useServers.ts index a546581a6..29c04c501 100644 --- a/core/react/useServers.ts +++ b/core/react/useServers.ts @@ -44,6 +44,14 @@ export interface UseServersResult { settings: InspectorServerSettings, ) => Promise; removeServer: (id: string) => Promise; + /** + * Persist a new ordering for the server list. `orderedIds` must be the + * complete set of current server ids in the desired order. The local list + * is reordered optimistically so the grid reflows instantly; on backend + * failure (e.g. the on-disk set changed underneath us) we re-fetch to snap + * back to disk truth. Routes through `PUT /api/servers/order`. + */ + reorderServers: (orderedIds: string[]) => Promise; } function buildHeaders( @@ -252,6 +260,45 @@ export function useServers(opts: UseServersOptions): UseServersResult { [base, authToken, doFetch, refresh], ); + const reorderServers = useCallback( + async (orderedIds: string[]): Promise => { + // Optimistic reorder: rebuild the local list in the requested order so + // the grid reflows immediately, before the round-trip resolves. Built + // from a lookup off the previous state (inside the setter) so we never + // capture a stale snapshot, and any id not currently present is simply + // skipped rather than producing an `undefined` hole. + setServers((prev) => { + const byId = new Map(prev.map((s) => [s.id, s])); + const reordered = orderedIds + .map((id) => byId.get(id)) + .filter((s): s is ServerEntry => s !== undefined); + // Defensive: if the requested order dropped any entries (shouldn't + // happen — callers pass the full set), keep the strays at the end so + // nothing vanishes from the UI before the refresh reconciles. + if (reordered.length !== prev.length) { + const seen = new Set(orderedIds); + for (const s of prev) if (!seen.has(s.id)) reordered.push(s); + } + return reordered; + }); + try { + const res = await doFetch(`${base}/api/servers/order`, { + method: "PUT", + headers: buildHeaders(authToken, true), + body: JSON.stringify({ order: orderedIds }), + }); + if (!res.ok) { + throw new Error(await readErrorMessage(res)); + } + } catch (err) { + // Revert to disk truth — the optimistic order may not have landed. + await refresh(); + throw err instanceof Error ? err : new Error(String(err)); + } + }, + [base, authToken, doFetch, refresh], + ); + return { servers, loading, @@ -261,5 +308,6 @@ export function useServers(opts: UseServersOptions): UseServersResult { updateServer, updateServerSettings, removeServer, + reorderServers, }; }