Skip to content

Commit 85a4b4c

Browse files
cliffhallclaude
andauthored
feat(servers): persist per-server settings to mcp.json (#1352) (#1353)
* feat(servers): persist per-server settings to mcp.json (#1352) - Add optional `settings` node alongside the transport config in each `~/.mcp-inspector/mcp.json` entry. Round-trips through `mcpConfigToServerEntries` / `serverEntriesToMcpConfig` and the `/api/servers` POST/PUT routes. - Drop `MCPServerConfig.headers` from `SseServerConfig` / `StreamableHttpServerConfig` and remove the headers textarea from `ServerConfigModal`. Custom headers are now entered only in `ServerSettingsForm` and reach the wire via `settings.headers`. - Wire `ServerSettingsModal` into `App.tsx`; saves dispatch through a new `useServers.updateServerSettings` mutator. `updateServer` carries the existing settings across config-only saves so they aren't dropped. - Apply settings on the *first* outbound request after a server loads — no need to open the settings form first: - `settings.headers` → SSE / streamable-http wire headers. - `settings.metadata` → `InspectorClientOptions.defaultMetadata`, merged into every outgoing `_meta` (call-time keys win). - `settings.requestTimeout` → `InspectorClientOptions.timeout`. - `settings.connectionTimeout` → Promise.race wrapper around connect(). - `settings.oauth*` → pre-seeded OAuth client credentials. - `RemoteConnectRequest` carries `settings` so the remote backend can source transport headers from the same source as the local path. - `oauthClientSecret` is persisted in `mcp.json` alongside stdio `env` values, both protected by the file's `0o600` permission. - Update `specification/v2_servers_file.md` with the new shape and the removal of the legacy headers field. Tests: round-trip in `serverList.test.ts`; persistence on POST/PUT in `servers-route.test.ts`; first-connect header injection in `transport.test.ts`; `defaultMetadata` merging in `inspectorClient.test.ts`; `useServers.updateServerSettings` + `updateServer` settings-preservation in `useServers.test.tsx`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): rebuild InspectorClient on every connect so settings refresh The previous onToggleConnection branch reused the existing InspectorClient on a same-server reconnect, which froze settings at the moment the client was first constructed. Editing headers, metadata, timeouts, or OAuth credentials in the settings form was therefore invisible until the user either switched servers or reloaded the page — surprising right after a visible save. Always call setupClientForServer on (re)connect so the latest target.settings flow through to the transport's headers / requestInit, to defaultMetadata, and to the OAuth manager. Also adds remoteClientTransport tests that pin the wire-level contract: settings round-trip through the /api/mcp/connect body when supplied, and the field is omitted when no settings are configured. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(history): stop wiping pendingRequestEntries inside the connect event State managers (ManagedToolsState, ManagedPromptsState, ManagedResourcesState, ManagedResourceTemplatesState, ManagedRequestorTasksState) handle the client's "connect" event by calling `void this.refresh()`. The synchronous portion of refresh runs through SDK Protocol.request → transport.send → MessageTrackingTransport.send → trackRequest, all before yielding to the network — so each list request gets tracked into MessageLogState's pendingRequestEntries inside the same connect event dispatch. MessageLogState's own "connect" listener was registered after those state managers (it's the last one constructed in App.tsx) and cleared pendingRequestEntries on every connect. Listener order meant the clear ran *after* the requests were tracked but before their responses arrived, leaving the history with unmatched "response" entries marked PENDING. The clear-on-disconnect (via statusChange → disconnected) already handles session-boundary cleanup, so clear-on-connect was purely defensive and now actively harmful. Drop the connect listener; keep the disconnect path. Tests: updated the unit test to assert the new behavior (connect does not clear); added an integration regression that wires a sibling connect listener firing listTools and asserts the response folds into the request entry instead of appearing as an orphan. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: drop accidentally tracked debugging screenshot pending-requests.png was a local debugging artifact attached to a conversation, not source. Remove it from the tree (the working copy is kept so the user still has it locally) and rely on the existing untracked behavior going forward. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(servers): add e2e check that settings.headers reach upstream MCP Stitches the full v2 pipeline together in one assertion: browser-style RemoteClientTransport → POST /api/mcp/connect (settings in body) → backend createTransportNode applies settings.headers to requestInit → SDK StreamableHTTPClientTransport → upstream MCP test server Captures the upstream HTTP request via FetchRequestLogState and asserts both `X-Tenant` and `X-Trace` are present on the outbound POST. Catches any regression that breaks the wire between the form and the upstream server in a single integration test, instead of the previous coverage which exercised each hop in isolation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(ServerSettingsModal): invert ListToggle compact prop so icon matches state ListToggle's `compact` prop represents the list's *current* compact state: true means "currently compact" and renders the expand icon to advertise the action (open up). The settings modal was passing `compact={allExpanded}` — exactly inverted. When all sections were open it showed the expand icon (suggesting "open more") and when collapsed it showed the collapse icon, opposite of every other ListToggle caller in the app. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * style(HistoryListPanel): match ServerListScreen Export button — rename + default variant Renames "Export JSON" to "Export" and switches from the subtle ToolbarButton to the same `Button variant="default"` used by ServerListControls so the two Export affordances look identical across screens. Also moves the button to the right of the ListToggle (mirroring the order on the server list screen) and disables it when there are no entries to export. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * style(HistoryListPanel): move Clear button to top toolbar, match Export style Moves the Clear affordance out of the "History (N)" section header and up to the panel toolbar alongside Export, where it now uses the same `Button variant="default"` styling. Order in the toolbar is now: ListToggle · Clear · Export. Clear is disabled when there are no unpinned entries to remove (pinned entries are intentionally preserved). ToolbarButton helper is unused after this change and dropped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: ignore local debugging screenshots at repo root Stops `git add -A` from re-staging the `*.png` files Claude Code drops at the repo root from attached conversation screenshots. The pattern only matches the top-level (`/*.png`) so legitimate image assets in subdirectories (e.g. `clients/web/public/`) still get tracked when explicitly added. Also untracks the two screenshots that slipped through previously (initialize-request.png, pending-requests.png). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * style(LogStreamPanel): match HistoryListPanel button styling Clear / Export / Copy All on the Logging screen were rendered with the subtle ToolbarButton helper while History's equivalents use `Button variant="default"`. Switch all three to the default variant so the toolbars look identical across screens and mirror the disabling behavior — each button is now disabled when `entries.length === 0`. ToolbarButton helper is unused after this change and dropped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): address PR review — debounce save, preserve-on-omit PUT, validate shape, lift connect timeout Round-up of fixes from #1353 (comment): 1. ServerSettingsModal saves were firing on every keystroke (one PUT + full mcp.json refresh per character). App.tsx now debounces by 300ms and flushes on modal close so a burst of edits coalesces into one PUT and the form value can't flicker back mid-typing. 2. `PUT /api/servers/:id` with an omitted `settings` field used to silently wipe the persisted settings node. The route now treats the three intents distinctly: - field omitted → preserve existing on-disk settings - explicit null → clear the settings node - settings object → validate + apply `useServers.updateServer` is simplified to drop the client-side carry-through hack, and the test that documented the broken behavior is flipped + augmented with explicit-clear and bad-shape cases. 3. `connectionTimeout` Promise.race is moved from App.tsx into `InspectorClient.connect()`, reading `this.serverSettings?.connectionTimeout`. TUI/CLI consumers now get the same behavior; on timeout the client internally disconnects so the next connect starts clean. 4. Custom Headers hint in `ServerSettingsForm` notes that `Authorization` is owned by the OAuth flow when OAuth is configured — avoids the sharp edge where the SDK's authProvider silently overwrites a user-set Authorization header. 5. New `validateSettings` on the server route rejects malformed shapes (`settings: []`, `settings.headers: "oops"`, non-numeric timeouts, etc.) with 400 instead of letting them persist and crash the UI. 7 (nit). Renamed `settings` → `savedSettings` inside `setupClientForServer` so the variable holding the persisted settings doesn't shadow the later locally-derived values. Follow-ups opened separately: #1356 — Move oauthClientSecret out of mcp.json into the OS keychain (review #6). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): address PR review pass 2 — connect-timeout test, unmount cleanup, tagged intent, lenient settings read Pass-2 review at #1353 (comment): (a) New integration test pins the new client-layer connect-timeout contract: a hanging stub transport with `connectionTimeout: 50` rejects with the descriptive error and lands the client in "error" status. Documents what TUI/CLI consumers should expect from the lifted Promise.race in InspectorClient.connect(). (b) useEffect cleanup clears `pendingSettingsTimerRef` on unmount so a route change or HMR mid-debounce can't fire one stale PUT against an unmounted component. (c) Refactor the PUT-route `settingsIntent` discriminator from `"preserve" | "clear" | { value }` to a tagged union with `kind`, and consume it with a `switch` for exhaustiveness — reads cleaner and makes adding a fourth intent painless. (d) `normalizeMcpServers` now runs `validateSettings` on the on-disk settings node and silently strips it if invalid, mirroring the existing lenient-on-read pattern from `normalizeServerType`. Closes the read/write asymmetry where a hand-edited malformed `settings` could survive on disk and get carried through preserve-on-PUT. New route test pins the strip-on-GET + no-propagate-on-preserve-PUT behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): pass-3 review — patch-style PUT, toast on close-flush fail Pass-3 review at #1353 (comment). (b) `PUT /api/servers/:id` now treats both `config` and `settings` as optional patches: - field omitted → preserve the on-disk value - explicit body value → apply - settings: null → clear `updateServerSettings` stops reading `existing.config` from the in-memory `servers` snapshot, which closes the closure-capture hazard where a refresh between debounce-schedule and flush could silently revert a separate edit. The route is the single source of truth for transport config now. New tests cover settings-only PUT, rejected non-object config, and the empty-body no-op patch case. The previous "rejects a missing config" test is rewritten to document the new no-op-preserve semantics. (c) Inline comment near the PUT rebuild loop documents that writing the full mcpServers map back is what gives the route its desirable self-healing property for malformed settings on *other* entries — deliberate side-effect, not an accident of normalization. (d) `flushPendingSettings` surfaces failures via @mantine/notifications instead of swallowing them. The modal already closed by the time the flush PUT resolves, so a silent fail would leave the user thinking their last edits saved (especially painful for the OAuth client secret). The notifications wiring was already mounted in main.tsx. Pass-3 (a) — settings form being effectively uncontrolled mid-edit — is acknowledged in the reply: working as intended today; the proper fix is internal modal state when file-watching (#1345) forces the issue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): pass-4 review — strip smuggled settings out of config Pass-4 review at #1353 (comment). Main finding: `normalizeServerType` spreads unknown keys, so a body that nested `settings` inside `config` would smuggle a settings field onto the stored entry without ever passing through `validateSettings`. Strip the settings key off the incoming config in `buildStoredEntry` so `validateSettings` remains the single write path for the settings node — the "one source of truth" invariant pass 2 set up holds again. Two new tests pin the strip on both routes: - POST with `config.settings = { bogus }` lands an entry with no settings node on disk. - PUT with `config.settings = { bogus }` + `settings: null` clears the real settings *and* doesn't re-attach the bogus payload via the spread. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): pass-5 review — absorb late connect rejection, tighten validateSettings, log smuggle warning Pass-5 review at #1353 (comment). Main finding: when the connect-time `Promise.race` lost to the timeout, the original `this.client.connect(this.transport)` promise was left pending. After `disconnect()` tore the transport down, real transports (SSE / streamable-http) would reject that promise later — with no handler attached, producing a Node `unhandledRejection` (and a vitest suite warning). Attach a no-op `.catch` right after creating the connect promise so Node sees the late rejection as handled. Nit (a): `validateSettings` was casting the raw object through to `InspectorServerSettings`, which let unknown keys ride along onto disk. Replaced the cast with an explicit pick-and-build over the known fields. Unknown stowaways now silently drop on the way through. New route test pins the behavior. Nit (b): `buildStoredEntry` now logs a `warn` via the route's pino logger when an incoming `config` carries a `settings` key. The strip is still defensive (it isn't a documented contract) but the warning lets a misconfigured client find out about the wasted field instead of debugging a silent drop. Nit (c) — the defensive `?? {}` fallback in `buildStoredEntry` — acknowledged in the review reply; left in place as belt-and-braces. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(servers): pass-6 polish — scope absorber, thread id, drop empty OAuth, guard `in` Pass-6 review at #1353 (comment). Reviewer signed off as ready; landing the four marginal cleanups since they're all small. (a) `connectPromise.catch(() => {})` moves inside the `if (connectTimeoutMs > 0)` branch. The await-only path propagates rejection cleanly through the outer try/catch — the absorber is only needed when the race leaves the original promise unattended. Code now reads as "absorber is tied to the race," which is the real invariant. (b) Thread `id` (POST id, PUT newId) through `buildStoredEntry` and include it in the smuggle warning's pino bindings. Two clients smuggling simultaneously are distinguishable in the log instead of indistinguishable. (c) `validateSettings` coerces empty-string OAuth fields to absent. The form emits `""` when the user clears an input, and an empty `oauthClientId` on disk would later be misread as "OAuth configured." New route test pins it. (d) `buildStoredEntry` adds a null/object guard before `"settings" in configObj`. The route layer pre-checks this on POST and PUT, but the helper's `unknown` parameter is a weak contract; the guard makes it safe to call from anywhere. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(spec): reflect settings persistence + patch-PUT contract in v2_servers_file The doc was authored when `mcp.json` only stored the server list. Bring it forward to match what shipped in #1352 / PR #1353: - Summary calls out that the file also stores per-server settings and links into the Per-server settings section. - On-disk format note explains that `settings` is an optional Inspector- specific extension that other MCP tools ignore. - New top paragraph on the Per-server settings section explains the basic-config / settings UI split rationale (simpler form for the common case, advanced settings tucked behind a second dialog). - UI bullet is rewritten to match the patch-style PUT semantics that emerged across the review cycle: settings-only and config-only PUTs preserve their counterpart inside the write lock, so neither modal can silently clobber the other half. The earlier "must re-send settings on PUT" hack is gone. - New bullets capture the save cadence (300ms debounce + flush-on-close + failure toast), the patch-PUT contract (omit-preserve / null-clear / rename), and the write-path / read-path validator gates. - Secret storage bullet links to #1356 (OS-keychain follow-up). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(spec): show a third entry with the optional settings node The On-disk format example demonstrated only basic stdio entries. Add a streamable-http entry with a populated `settings` node so readers can see the shape inline rather than having to scroll to the Per-server settings section. Inline comment in the block cross-links there for the full contract and notes that other MCP tools ignore the extension key. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 420da2e commit 85a4b4c

34 files changed

Lines changed: 1884 additions & 355 deletions

.gitignore

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,3 +7,7 @@ dist
77
test-servers/build
88
.env
99
/.playwright-mcp/
10+
11+
# Local debugging screenshots dropped at the repo root (e.g. attached to
12+
# Claude Code conversations). Should never be part of the source tree.
13+
/*.png

clients/web/server/web-server-config.ts

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -97,29 +97,25 @@ export function webServerConfigToInitialPayload(
9797
return {
9898
defaultTransport: "sse",
9999
defaultServerUrl: mc.url,
100-
defaultHeaders: mc.headers ?? undefined,
101100
defaultEnvironment,
102101
};
103102
}
104103
if (mc.type === "streamable-http") {
105104
return {
106105
defaultTransport: "streamable-http",
107106
defaultServerUrl: mc.url,
108-
defaultHeaders: mc.headers ?? undefined,
109107
defaultEnvironment,
110108
};
111109
}
112110
// Forward-compat fallback: if a future SDK ships a new transport type the
113111
// launcher hasn't taught us about yet, prefer streamable-http (the most
114-
// permissive shape with `url` + optional `headers`) over throwing. Worst
115-
// case the UI shows the URL the user supplied; throwing here would deny
116-
// the user a hint at startup. Add a real branch above once the type is
117-
// known.
118-
const c = mc as unknown as { url: string; headers?: Record<string, string> };
112+
// permissive shape with `url`) over throwing. Worst case the UI shows the
113+
// URL the user supplied; throwing here would deny the user a hint at
114+
// startup. Add a real branch above once the type is known.
115+
const c = mc as unknown as { url: string };
119116
return {
120117
defaultTransport: "streamable-http",
121118
defaultServerUrl: c.url,
122-
defaultHeaders: c.headers,
123119
defaultEnvironment,
124120
};
125121
}

clients/web/src/App.tsx

Lines changed: 155 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { useCallback, useEffect, useMemo, useRef, useState } from "react";
22
import { useComputedColorScheme, useMantineColorScheme } from "@mantine/core";
3+
import { notifications } from "@mantine/notifications";
34
import type {
45
InitializeResult,
56
LoggingLevel,
@@ -10,6 +11,7 @@ import type { AppBridge } from "@modelcontextprotocol/ext-apps/app-bridge";
1011
import { InspectorClient } from "@inspector/core/mcp/index.js";
1112
import type { JsonValue } from "@inspector/core/mcp/index.js";
1213
import type {
14+
InspectorServerSettings,
1315
MCPServerConfig,
1416
MessageEntry,
1517
ServerEntry,
@@ -45,6 +47,7 @@ import {
4547
ServerConfigModal,
4648
type ServerConfigModalMode,
4749
} from "./components/groups/ServerConfigModal/ServerConfigModal";
50+
import { ServerSettingsModal } from "./components/groups/ServerSettingsModal/ServerSettingsModal";
4851
import { ServerRemoveConfirmModal } from "./components/groups/ServerRemoveConfirmModal/ServerRemoveConfirmModal";
4952
import { downloadJsonFile } from "./lib/downloadFile";
5053
import { createWebEnvironment } from "./lib/environmentFactory";
@@ -135,7 +138,13 @@ function App() {
135138
// Server list — sourced from ~/.mcp-inspector/mcp.json via the backend's
136139
// `/api/servers` routes. First-launch seeds are written by the backend when
137140
// the file is absent, so this hook returns a non-empty list on first load.
138-
const { servers, addServer, updateServer, removeServer } = useServers({
141+
const {
142+
servers,
143+
addServer,
144+
updateServer,
145+
updateServerSettings,
146+
removeServer,
147+
} = useServers({
139148
baseUrl:
140149
typeof window !== "undefined"
141150
? window.location.origin
@@ -149,6 +158,9 @@ function App() {
149158
mode: ServerConfigModalMode;
150159
targetId?: string;
151160
} | null>(null);
161+
const [settingsModalTargetId, setSettingsModalTargetId] = useState<
162+
string | undefined
163+
>(undefined);
152164
const [removeTarget, setRemoveTarget] = useState<ServerEntry | null>(null);
153165

154166
// The active connection target. `null` between sessions; set as soon as
@@ -336,6 +348,35 @@ function App() {
336348
getAuthToken(),
337349
redirectUrlProvider,
338350
);
351+
// The settings node persisted in mcp.json for this server — distinct
352+
// from the InspectorClient options we're about to derive from it.
353+
const savedSettings = server.settings;
354+
// Flatten the persisted settings into the InspectorClient options shape.
355+
// Empty / zero values stay unset so the SDK defaults apply.
356+
const defaultMetadata = savedSettings?.metadata
357+
? Object.fromEntries(
358+
savedSettings.metadata
359+
.filter((m) => m.key.trim() !== "")
360+
.map((m) => [m.key, m.value]),
361+
)
362+
: undefined;
363+
const oauth =
364+
savedSettings &&
365+
(savedSettings.oauthClientId ||
366+
savedSettings.oauthClientSecret ||
367+
savedSettings.oauthScopes)
368+
? {
369+
...(savedSettings.oauthClientId && {
370+
clientId: savedSettings.oauthClientId,
371+
}),
372+
...(savedSettings.oauthClientSecret && {
373+
clientSecret: savedSettings.oauthClientSecret,
374+
}),
375+
...(savedSettings.oauthScopes && {
376+
scope: savedSettings.oauthScopes,
377+
}),
378+
}
379+
: undefined;
339380
const client = new InspectorClient(server.config, {
340381
environment,
341382
// The Tasks tab needs the receiver-task pipeline; the
@@ -344,6 +385,16 @@ function App() {
344385
// Sampling / elicitation are on by default; keep the parameterized
345386
// options off until the UI grows the surface to render them.
346387
elicit: { form: true, url: true },
388+
...(savedSettings &&
389+
savedSettings.requestTimeout > 0 && {
390+
timeout: savedSettings.requestTimeout,
391+
}),
392+
...(defaultMetadata &&
393+
Object.keys(defaultMetadata).length > 0 && {
394+
defaultMetadata,
395+
}),
396+
...(oauth && { oauth }),
397+
...(savedSettings && { serverSettings: savedSettings }),
347398
});
348399

349400
setInspectorClient(client);
@@ -396,16 +447,23 @@ function App() {
396447
const target = servers.find((s) => s.id === id);
397448
if (!target) return;
398449

399-
// Different server (or first connect): rebuild the client + managers.
400-
let client = inspectorClient;
401-
if (id !== activeServerId || client === null) {
402-
client = setupClientForServer(target);
450+
// Always rebuild the InspectorClient on a (re)connect so the latest
451+
// `target.settings` (headers, metadata, timeouts, OAuth credentials)
452+
// are picked up. Reusing the previous client object would freeze the
453+
// settings at the moment it was first constructed, which would be
454+
// surprising right after the user edited them in the settings modal.
455+
const client = setupClientForServer(target);
456+
if (id !== activeServerId) {
403457
setActiveServerId(id);
404458
}
405459

406460
setErrorMessage(undefined);
407461
connectStartRef.current = Date.now();
408462
try {
463+
// `settings.connectionTimeout` is consumed inside InspectorClient.connect
464+
// (Promise.race + transport teardown live there now), so this branch
465+
// stays unaware of the per-server timeout. TUI/CLI consumers get the
466+
// same behavior by reading from `serverSettings` on the client.
409467
await client.connect();
410468
} catch (err) {
411469
// Handshake-only. A mid-session transport failure transitions the
@@ -705,6 +763,91 @@ function App() {
705763
return servers.find((s) => s.id === configModal.targetId);
706764
}, [configModal, servers]);
707765

766+
const settingsModalTarget = useMemo(() => {
767+
if (!settingsModalTargetId) return undefined;
768+
return servers.find((s) => s.id === settingsModalTargetId);
769+
}, [settingsModalTargetId, servers]);
770+
771+
// Stable starting shape for entries that have no `settings` node yet — the
772+
// form needs concrete arrays / numbers to render.
773+
const settingsModalValue = useMemo<InspectorServerSettings>(() => {
774+
return (
775+
settingsModalTarget?.settings ?? {
776+
headers: [],
777+
metadata: [],
778+
connectionTimeout: 0,
779+
requestTimeout: 0,
780+
}
781+
);
782+
}, [settingsModalTarget]);
783+
784+
// The settings modal fires onSettingsChange on every keystroke. Persisting
785+
// each character would run a tight PUT-then-full-refresh loop against the
786+
// backend (and the in-memory `servers` state could flicker mid-typing as
787+
// each refresh resets the modal's prop). Debounce so a burst of edits
788+
// coalesces into a single PUT, and flush on modal close so nothing is lost.
789+
const pendingSettingsRef = useRef<{
790+
id: string;
791+
settings: InspectorServerSettings;
792+
} | null>(null);
793+
const pendingSettingsTimerRef = useRef<
794+
ReturnType<typeof setTimeout> | undefined
795+
>(undefined);
796+
797+
const flushPendingSettings = useCallback(() => {
798+
if (pendingSettingsTimerRef.current) {
799+
clearTimeout(pendingSettingsTimerRef.current);
800+
pendingSettingsTimerRef.current = undefined;
801+
}
802+
const pending = pendingSettingsRef.current;
803+
if (!pending) return;
804+
pendingSettingsRef.current = null;
805+
// Fire-and-forget — but surface failures via toast. The modal closes
806+
// immediately on user dismiss, so a silent fail-on-flush would leave
807+
// the user thinking their last edits saved when they didn't (especially
808+
// painful for the OAuth client secret).
809+
updateServerSettings(pending.id, pending.settings).catch((err) => {
810+
notifications.show({
811+
title: `Failed to save settings for "${pending.id}"`,
812+
message: err instanceof Error ? err.message : String(err),
813+
color: "red",
814+
});
815+
});
816+
}, [updateServerSettings]);
817+
818+
const onSettingsChange = useCallback(
819+
(next: InspectorServerSettings) => {
820+
if (!settingsModalTargetId) return;
821+
pendingSettingsRef.current = {
822+
id: settingsModalTargetId,
823+
settings: next,
824+
};
825+
if (pendingSettingsTimerRef.current) {
826+
clearTimeout(pendingSettingsTimerRef.current);
827+
}
828+
pendingSettingsTimerRef.current = setTimeout(flushPendingSettings, 300);
829+
},
830+
[settingsModalTargetId, flushPendingSettings],
831+
);
832+
833+
const onSettingsModalClose = useCallback(() => {
834+
flushPendingSettings();
835+
setSettingsModalTargetId(undefined);
836+
}, [flushPendingSettings]);
837+
838+
// Cancel any pending debounce on unmount (route change / HMR). Without
839+
// this a stale setTimeout could still fire one final `updateServerSettings`
840+
// against an unmounted component — harmless today (just an extra PUT of
841+
// the final payload) but the cleanest pattern is to clear the timer.
842+
useEffect(() => {
843+
return () => {
844+
if (pendingSettingsTimerRef.current) {
845+
clearTimeout(pendingSettingsTimerRef.current);
846+
pendingSettingsTimerRef.current = undefined;
847+
}
848+
};
849+
}, []);
850+
708851
// The Resources screen needs `isSubscribed` to flip the Subscribe button
709852
// label to "Unsubscribe". Derive it from the live subscriptions list rather
710853
// than threading it through every setReadResourceState site — that way the
@@ -756,7 +899,7 @@ function App() {
756899
onServerImportJson={todoNoop}
757900
onServerExport={onServerExport}
758901
onServerInfo={todoNoop}
759-
onServerSettings={todoNoop}
902+
onServerSettings={(id) => setSettingsModalTargetId(id)}
760903
onServerEdit={(id) => setConfigModal({ mode: "edit", targetId: id })}
761904
onServerClone={(id) => setConfigModal({ mode: "clone", targetId: id })}
762905
onServerRemove={(id) => {
@@ -805,6 +948,12 @@ function App() {
805948
onClose={() => setConfigModal(null)}
806949
onSubmit={onConfigSubmit}
807950
/>
951+
<ServerSettingsModal
952+
opened={settingsModalTargetId !== undefined}
953+
settings={settingsModalValue}
954+
onClose={onSettingsModalClose}
955+
onSettingsChange={onSettingsChange}
956+
/>
808957
<ServerRemoveConfirmModal
809958
opened={removeTarget !== null}
810959
target={removeTarget}

clients/web/src/components/groups/HistoryListPanel/HistoryListPanel.test.tsx

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -69,12 +69,10 @@ const baseProps = {
6969
};
7070

7171
describe("HistoryListPanel", () => {
72-
it("renders the title and Export JSON button", () => {
72+
it("renders the title and Export button", () => {
7373
renderWithMantine(<HistoryListPanel {...baseProps} entries={[]} />);
7474
expect(screen.getByText("Requests")).toBeInTheDocument();
75-
expect(
76-
screen.getByRole("button", { name: "Export JSON" }),
77-
).toBeInTheDocument();
75+
expect(screen.getByRole("button", { name: "Export" })).toBeInTheDocument();
7876
});
7977

8078
it("renders the empty state when there are no entries", () => {
@@ -134,7 +132,7 @@ describe("HistoryListPanel", () => {
134132
expect(screen.getByText("History (1)")).toBeInTheDocument();
135133
});
136134

137-
it("invokes onExport when Export JSON is clicked", async () => {
135+
it("invokes onExport when Export is clicked", async () => {
138136
const user = userEvent.setup();
139137
const onExport = vi.fn();
140138
renderWithMantine(
@@ -144,7 +142,7 @@ describe("HistoryListPanel", () => {
144142
onExport={onExport}
145143
/>,
146144
);
147-
await user.click(screen.getByRole("button", { name: "Export JSON" }));
145+
await user.click(screen.getByRole("button", { name: "Export" }));
148146
expect(onExport).toHaveBeenCalledTimes(1);
149147
});
150148

clients/web/src/components/groups/HistoryListPanel/HistoryListPanel.tsx

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,6 @@ export interface HistoryListPanelProps {
2424
onTogglePin: (id: string) => void;
2525
}
2626

27-
const ToolbarButton = Button.withProps({
28-
variant: "subtle",
29-
size: "sm",
30-
});
31-
3227
const PanelContainer = Paper.withProps({
3328
withBorder: true,
3429
p: "lg",
@@ -101,13 +96,22 @@ export function HistoryListPanel({
10196
<Group justify="space-between" mb="sm">
10297
<Title order={4}>Requests</Title>
10398
<Group gap="xs">
104-
<ToolbarButton onClick={onExport}>Export JSON</ToolbarButton>
10599
{hasResults && (
106100
<ListToggle
107101
compact={compact}
108102
onToggle={() => setCompact((c) => !c)}
109103
/>
110104
)}
105+
<Button
106+
variant="default"
107+
onClick={onClearAll}
108+
disabled={unpinnedEntries.length === 0}
109+
>
110+
Clear
111+
</Button>
112+
<Button variant="default" onClick={onExport} disabled={!hasResults}>
113+
Export
114+
</Button>
111115
</Group>
112116
</Group>
113117

@@ -136,12 +140,9 @@ export function HistoryListPanel({
136140

137141
{unpinnedEntries.length > 0 && (
138142
<>
139-
<Group justify="space-between">
140-
<Title order={5}>
141-
{formatHistoryTitle(unpinnedEntries.length)}
142-
</Title>
143-
<ToolbarButton onClick={onClearAll}>Clear</ToolbarButton>
144-
</Group>
143+
<Title order={5}>
144+
{formatHistoryTitle(unpinnedEntries.length)}
145+
</Title>
145146
{unpinnedEntries.map((entry) => (
146147
<HistoryEntry
147148
key={entry.id}

0 commit comments

Comments
 (0)