feat(web): reorderable ServerCards — drag-and-drop + keyboard (#1369)

[cliffhall]

Closes #1369.

Users can now control the order of ServerCards on the Servers screen instead of hand-editing mcp.json. Each card has a grip handle (in the header, left of the name) that drives an @dnd-kit sortable; pointer and keyboard reorder both work, with live-region announcements for screen readers. The new order persists to mcp.json so it survives reloads and is visible to other Inspector incarnations.

Screen capture

drag-cards.mov

Changes

Backend (core/mcp/remote/node/server.ts)

• New PUT /api/servers/order accepting { order: string[] }. Rewrites mcp.json in the supplied order inside the write lock, reusing the atomic-write + watcher-notify path so peers see exactly one change.
• Registered before PUT /api/servers/:id so the literal /order segment isn't captured as an :id.
• Rejects with 409 when the supplied id set no longer matches disk (race against an external edit → no silent drop/dup), and 400 on duplicate ids or malformed bodies. Reordering touches no secret values.

Hook (core/react/useServers.ts)

• reorderServers(orderedIds): optimistic local reorder so the grid reflows instantly; re-fetches disk truth on failure.

UI

• SortableServerCard wrapper owns all dnd-kit concerns (sortable node, per-frame transform, grip activator) so ServerCard stays a dumb display component that just renders the dragHandle slot.
• ServerListScreen wraps the grid in DndContext + SortableContext (rect strategy) with pointer + keyboard sensors and announcements. Reorder math and a11y copy are extracted to reorderIds.ts / serverReorder.ts for direct unit testing. Renders plain cards (no grip) when no onReorder is wired.
• Wired onServerReorder through InspectorView → App.tsx.
• Grip styled like ListToggle (subtle/gray/size=md), grab cursor via .server-drag-handle.

Acceptance criteria

• Drag a card to a new position; grid reflows in real time.
• Tab to a card's grip, Space to pick up, arrows to move, Space to drop; SR announcements describe the operation.
• New order persists to mcp.json and survives reload.
• Concurrent external edit → API fails cleanly (409) without corrupting the file (integration test).
• Active-server highlighting, compact toggle, and Add/Import/Export unchanged.
• Tests: ServerListScreen units, a Storybook keyboard-reorder play, and backend integration tests incl. the conflicting-id-set rejection.

Testing notes

The @dnd-kit keyboard sensor needs measurable layout rects that happy-dom doesn't provide, so the full keyboard gesture is exercised in a real-browser Storybook play (test:storybook); the ordering math + announcement copy are unit-tested directly via the extracted pure modules.

npm run validate, npm run test:integration, and npm run test:storybook all pass.

Adds @dnd-kit/core, @dnd-kit/sortable, @dnd-kit/utilities.

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 39s —— View job

---

Code Review — reorderable ServerCards (#1447)

• Read backend (server.ts order endpoint)
• Read hook (useServers.ts)
• Read UI (SortableServerCard, ServerListScreen, pure helpers)
• Review tests
• Post review

Overall this is a clean, well-factored PR. The dumb-ServerCard / SortableServerCard-wrapper split is exactly the right boundary, the ordering math and a11y copy are pulled into pure modules and tested directly, and the backend reuses the existing write-lock + atomic-write + watcher path correctly. The keyboard-vs-happy-dom tradeoff (pure-module units + real-browser Storybook play) is the right call and is documented well. Verified the secret-handling concern myself — see below — and it's safe.

Findings

1. Reorder failure is an unhandled promise rejection (minor, actionable) — App.tsx:2154

onServerReorder={(orderedIds) => {
  void reorderServers(orderedIds);
}}

reorderServers re-throws after reverting (useServers.ts: throw err instanceof Error ? err : …), but the caller discards the promise with void and no .catch(). On the 409 (external edit changed the set) or a network failure, this surfaces as an unhandled rejection with no user-facing feedback — inconsistent with every other mutation in App.tsx, which calls notifications.show({...}) on error. The grid does snap back via the internal refresh(), so it's not data-corrupting, but the user sees their drag silently bounce with no explanation. Suggest catching and notifying:

onServerReorder={(orderedIds) => {
  reorderServers(orderedIds).catch((err: unknown) => {
    notifications.show({
      color: "red",
      title: "Couldn't reorder servers",
      message: err instanceof Error ? err.message : String(err),
    });
  });
}}

Fix this →

2. Reorder glue is computed unconditionally each render (nit) — ServerListScreen.tsx

const ids = servers.map((s) => s.id);
const handleDragEnd = makeServerDragEndHandler(servers, onReorder);
const announcements = buildReorderAnnouncements(servers);

These run (and announcements allocates a fresh object with four closures) on every render even when reorderable is false and they're never consumed. Cheap, but trivially guardable — only ids is needed for SortableContext, and handleDragEnd/announcements are only used inside the reorderable branch. Pure cleanup, not blocking.

3. onReorder?.() optional chaining is unreachable-defensive (nit) — serverReorder.ts. makeServerDragEndHandler guards onReorder with ?., but the handler is only wired into DndContext, which only renders when reorderable (i.e. onReorder !== undefined). The chaining is harmless dead-defense; fine to leave, just noting it's never the undefined path in practice.

Things I checked that are correct ✅

• No secret loss. readMcpConfig() returns the on-disk stripped shape (secrets live in the keychain, rehydrated only on GET via the separate rehydrateConfig). The reorder permutes those stripped entries and writes them back — identical read-stripped/write-stripped pattern to the POST handler. Keychain is untouched, so the "touches no secret values" comment is accurate.
• Route ordering. /api/servers/order is registered before /api/servers/:id, and there's an explicit regression test (servers-order.test.ts:166) guarding against order being captured as an :id.
• Status codes. Duplicate ids → 400, set-mismatch (missing or unknown id) → 409, malformed body → 400. The exact-set check correctly prevents silent drop/dup on a racing external add/remove, and the file is left unchanged on every rejection (asserted in tests).
• No top-level key loss. next: MCPConfig = { mcpServers: {} } mirrors the other mutators — mcp.json only holds mcpServers; defaultEnvironment is separate initialConfig.
• Optimistic update. Rebuilt inside the setServers updater (no stale snapshot) with a defensive stray-append, and reverts via refresh() on failure.
• Test coverage is strong: backend integration (8 cases incl. conflict/validation), reorderIds/serverReorder unit tests, useServers optimistic + revert, component-level handle rendering/labelling, and the real-browser keyboard play.

Verdict

No blockers. Recommend addressing #1 (silent reorder failure) before merge for UX consistency; #2/#3 are optional polish.
· branch v2/reorderable-server-cards

[cliffhall]

Thanks for the review! Addressed in 855701c.

#1 — Silent reorder failure (fixed). Agreed, this was the one real gap. onServerReorder now catches the re-thrown rejection and shows a red toast matching the pattern every other mutation in App.tsx uses, so a 409 (racing external edit) or network error gives the user feedback instead of a silent bounce-back:

reorderServers(orderedIds).catch((err: unknown) => {
  notifications.show({
    title: "Failed to reorder servers",
    message: err instanceof Error ? err.message : String(err),
    color: "red",
  });
});

#2 — Reorder glue computed unconditionally (fixed). handleDragEnd and announcements are now built only when reorderable, so the four-closure announcements object isn't allocated on renders where no onReorder is wired. ids stays unconditional since SortableContext needs it.

#3 — onReorder?.() defensive chaining (keeping as-is). Intentional: makeServerDragEndHandler is a standalone pure module with its own contract, and the undefined path is exercised directly by a unit test (serverReorder.test.ts — "tolerates an absent onReorder callback"). With #2 the component never calls it with undefined in practice, but I'd rather the helper stay independently safe and tested than couple its guard to the caller's current wiring.

validate, test:integration, and test:storybook all still pass.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 42s —— View job

---

Re-review — fixes in 855701c7

• Review fix for Progress notifications #1 (reorder failure toast in App.tsx)
• Review fix for Request logging #2 (conditional reorder glue in ServerListScreen.tsx)
• Re-scan full diff for new issues
• Post verdict

Both items from the prior review are addressed correctly. Verified against the committed code, not just the description.

#1 — Silent reorder failure → resolved. App.tsx:2154 now chains .catch((err: unknown) => notifications.show({ color: "red", … })) on the re-thrown rejection, so a 409 (racing external edit) or network failure surfaces a red toast instead of an unhandled rejection + silent bounce-back. The err instanceof Error ? err.message : String(err) narrowing matches the other mutation handlers, and the comment explaining the revert-and-rethrow contract is a nice touch.

#2 — Unconditional glue → resolved. handleDragEnd and announcements are now built only when reorderable, falling back to undefined otherwise. Confirmed both are consumed only inside the reorderable DndContext branch (ServerListScreen.tsx:146-155), so the undefined values are never reached at a call site — and DndContext's onDragEnd / accessibility.announcements are both optional props, so this is type-safe. ids correctly stays unconditional since SortableContext always needs it.

#3 — Defensive onReorder?.() → agreed, leave as-is. Keeping makeServerDragEndHandler independently safe and unit-tested is the right call for a standalone pure module; coupling its guard to the caller's current wiring would be the worse trade.

Verdict

✅ All review feedback addressed. No new issues introduced by 855701c7 — the change is small, well-contained, and type-safe. No blockers; good to merge once CI is green.

--- · branch v2/reorderable-server-cards