feat(servers): hot-reload server list on external mcp.json edits (#1345)

[cliffhall]

Summary

• Backend watches ~/.mcp-inspector/mcp.json via chokidar and emits change events on a new GET /api/servers/events SSE channel.
• Browser subscribes via fetch() + ReadableStream from useServers (so the existing x-mcp-remote-auth: Bearer … header still flows — EventSource can't send custom headers) and re-fetches on each event.
• Self-fires from backend POST/PUT/DELETE writes are suppressed by capturing the post-write mtime and comparing on each watcher event; only external edits trigger a broadcast.
• Watcher is lazy: starts on the first SSE subscriber, stops when the last disconnects. createRemoteApp now returns a close() that prod callers chain into their shutdown paths so the watcher is released on exit.

Closes #1345.

Decisions (per the issue's "Risks" section)

• chokidar over bare fs.watch — editors save via temp-file + rename, which produces unlink + add sequences that bare fs.watch handles poorly on macOS/Linux. awaitWriteFinish coalesces those into a single event.
• New /api/servers/events channel, not piggyback — the existing /api/mcp/events is per-MCP-session (only open while a transport session is active). mcp.json changes are app-scoped; piggybacking would miss changes while the user is on the Servers screen with no active connection.
• mtime-based self-fire suppression — captures stat().mtimeMs right after every backend write inside the existing withWriteLock; the watcher handler stat()s and bails when the value matches.

Test plan

• Unit suite — npm run test (1243 tests)
• Integration suite — npm run test:integration (437 tests, including 4 new in servers-events.test.ts)
• Storybook suite — npm run test:storybook (306 tests)
• Coverage gate — npm run test:coverage per-file thresholds pass
• Dev server smoke test — npm run dev boots, banner prints, browser opens

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

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

---

PR Review — hot-reload server list on external mcp.json edits

• Read backend (core/mcp/remote/node/server.ts)
• Read hook (core/react/useServers.ts)
• Read integration test (servers-events.test.ts)
• Read unit test updates (useServers.test.tsx)
• Read wiring (server.ts, vite-hono-plugin.ts, vite.config.ts, configs)
• Analyze for correctness / races / perf / security
• Post findings

---

TL;DR

Solid, well-documented PR. Architecture is sound: chokidar + awaitWriteFinish for editor-save quirks, fetch-based SSE (so the existing x-mcp-remote-auth: Bearer … header still flows), lazy watcher started on first subscription and torn down on last disconnect, and a post-write mtime capture inside withWriteLock to suppress self-broadcasts. The createRemoteApp shutdown contract (close()) is threaded cleanly through both prod server and Vite plugin.

A few things worth considering before merge — none blocking.

---

Correctness / race concerns

1. Narrow race: external edit masked by backend write (self-fire suppression)

core/mcp/remote/node/server.ts:298-326

lastWrittenMtimeMs is a single shared value captured after every backend write. awaitWriteFinish coalesces events to a single change, so the suppression check compares the final mtime. If this sequence interleaves:

1. External editor saves → file mtime B
2. Backend POST starts → readMcpConfig() reads the file (sees B's contents), merges its mutation, writes → mtime C, lastWrittenMtimeMs = C
3. chokidar coalesces → one watcher event, handler stats, sees C, suppresses

The merged write on disk is correct (backend read the external edit), but other connected subscribers never learn — they have stale state until another external edit or refresh. Data isn't lost, but a peer tab sees a silent revert.

For a single-user local dev tool this is acceptable. If you want to tighten it: snapshot preMtime before the backend write and track a small window [preMtime, postMtime]; suppress only events whose mtime falls inside the window. Happy to leave as-is given the use case, but worth a comment acknowledging the race.

2. setLoading(true) on every SSE-triggered refresh causes UI flicker

core/react/useServers.ts:79-97 + :111-146

Every time the SSE reader sees \n\n, it calls refresh() which flips loading: true and clears error: undefined. Consumers that render a loading spinner or skeleton will flash on every external edit. Suggest a separate refreshing flag (or skip the setLoading(true) on background refreshes) so the list stays visible while the refetch is in flight.

// quick sketch
const refresh = useCallback(async (opts?: { background?: boolean }) => {
  if (!opts?.background) setLoading(true);
  setError(undefined);
  // ...
}, [...]);
// and in the SSE handler:
void refresh({ background: true });

3. Multiple rapid SSE frames fire multiple refreshes back-to-back

Same block — the while (frameEnd !== -1) loop calls void refresh() once per frame. If a watcher broadcast race ever emits 2+ change events in the same decode chunk, you get N concurrent fetches whose state-set order isn't guaranteed. Low-probability given awaitWriteFinish's 100ms stability threshold, but a trailing-edge debounce (e.g. coalesce within ~50ms) would be more robust and idempotent.

4. No SSE reconnect on transient failure

core/react/useServers.ts:139-143 — the catch swallows all errors (AbortError + network blips) with no backoff/retry. Comment calls this out deliberately, and for a local dev tool that's fine. But if a user hibernates their laptop or their network wobbles, the hook silently stops receiving updates and they won't know unless they hit refresh. Consider at minimum a one-liner setting an eventsConnected boolean so the UI can surface "file-watch disconnected" if someone cares later.

---

Minor / nits

5. handleWatcherEvent stats outside the write lock

server.ts:309-326 — the stat runs on the raw event, unrelated to withWriteLock. Not a bug (the lock only serializes read-modify-write on the mutating routes), but note that if the backend write + stat inside writeMcpAndTrackMtime is not itself atomic with the watcher's stat, an external writer racing the backend could swap the mtime between capture and compare. In practice the filesystem mtime precision + awaitWriteFinish stability threshold make this vanishingly unlikely; flagging for completeness.

6. Test coverage: PUT and DELETE self-fire suppression

servers-events.test.ts covers POST self-suppression but not PUT or DELETE. Since all three go through writeMcpAndTrackMtime, they're on the same code path, but an explicit test per verb would catch a future refactor that forgot one. Cheap to add:

it("does NOT emit for backend PUT /api/servers/:id", async () => { /* ... */ });
it("does NOT emit for backend DELETE /api/servers/:id", async () => { /* ... */ });

7. close() clears subscribers but doesn't close in-flight SSE streams

server.ts:1167-1170 — calling close() does serverEventSubscribers.clear() then stops the watcher, but each subscriber's streamSSE promise is still awaiting its own stream.onAbort. For in-process Vite, the HTTP server closing will abort the underlying sockets, so onAbort eventually fires. For the standalone serve() path in server.ts:118-129 the order is closeApi() → sandboxController.close() → httpServer.closeAllConnections() → httpServer.close() — the SSE promises resolve only once closeAllConnections() fires. That ordering works but ties close()'s "done" signal to an event that happens after close() resolves. Not harmful (nothing depends on awaiting those resolutions), but the docblock on close() could clarify: "resolves when the watcher is released; individual SSE stream promises resolve on subsequent socket close."

8. validateSettings referenced before declaration

server.ts:750-768 references validateSettings, which is declared at server.ts:819. Works today because the inner functions execute at request time, not at createRemoteApp call time — but the file reads more naturally with helpers above their callers. Not introduced by this PR (the settings PR added them in that order); mention only because this PR sits next to it and the file is getting large.

9. Banner not updated to mention the new endpoint

GET /api/servers/events is a new public-ish route. The top-of-file docblock (server.ts:1-5) was updated to include it; nice. No README or dev doc changes — probably fine since this is a transport detail, but if clients/web/README.md enumerates endpoints anywhere, worth a look.

---

Positive observations

• The choice of a separate /api/servers/events channel (rather than piggybacking on /api/mcp/events) is correctly justified in the PR body — session-scoped vs app-scoped is a real distinction.
• The fetch + ReadableStream approach (not EventSource) is the right call and the code comment explains exactly why.
• awaitWriteFinish: { stabilityThreshold: 100, pollInterval: 50 } handles the editor temp-file + rename case well; good defense.
• ensureWatcher / maybeStopWatcher lifecycle keeps the watcher completely inert when no client is subscribed, which is exactly what you want for tests that don't exercise SSE.
• The write queue + mtime capture both happen inside withWriteLock, so there's no lost-update against concurrent backend mutations.
• Vite onwarn suppression for chokidar mirrors the existing atomically pattern — consistent and minimal.
• optimizeDeps.exclude update keeps chokidar out of browser pre-bundling.

---

Verdict

LGTM with optional polish. The race in (1) and the loading-flicker in (2) are the only items I'd personally address before merge; the rest are nits / follow-up candidates.
· Branch: feat/1345-hot-reload-mcp-json

[cliffhall]

Thanks for the review. Addressed in d90f8e6; running through each finding:

1. External edit masked by backend write — addressed. writeMcpAndTrackMtime now stats the file before the write and, if the on-disk mtime doesn't match the value we last recorded ourselves, broadcasts after the write completes. The originating tab still gets its own refresh from the mutator round-trip; the broadcast is for peer subscribers that would otherwise miss the external edit because chokidar coalesced it with our write into a single watcher event. Took your "snapshot pre and detect divergence" approach rather than a literal [preMtime, postMtime] window — the equality-test variant is enough because withWriteLock serializes our own writes, so any pre-mtime that isn't lastWrittenMtimeMs is by definition external. New integration test emits a broadcast when an external edit slips in between two backend writes covers it.

2. setLoading(true) flicker on background refresh — addressed. Split into a public refresh() (toggles loading) and an internal refreshInternal(background: boolean) that the SSE handler calls with background: true. error still resets/sets either way so a real failure surfaces. Public API surface unchanged; no consumer was destructuring refresh to call manually so this is risk-free.

3. Rapid SSE frames → concurrent fetches — addressed (for the in-chunk case). The reader loop now counts frames in a decode chunk and fires a single refreshInternal(true) per chunk instead of N. Skipped cross-chunk debounce: awaitWriteFinish's 100ms stability threshold already serializes external edits at the source, and the rare back-to-back pair just costs one extra GET against an idempotent endpoint — not worth a setTimeout and the cleanup-on-unmount surface that comes with it. Reasoning is captured in the inline comment.

4. No SSE reconnect on transient failure — skipping. The existing inline comment already calls this out as a deliberate dev-tool choice. Surfacing an eventsConnected boolean would be net-new API surface with no current consumer, and we've been trimming speculative surface elsewhere in v2. If/when a UI surface wants to render a "watch disconnected" badge, that's the moment to expose it.

5. handleWatcherEvent stats outside the write lock — this was the same race as (1), addressed by the pre-stat-and-broadcast in writeMcpAndTrackMtime. The watcher handler itself still doesn't take the lock (it doesn't write), which is correct.

6. PUT/DELETE self-suppression coverage — added. Two new cases in servers-events.test.ts. Both routes go through writeMcpAndTrackMtime so they share a code path with the existing POST test, but now an explicit assert per verb catches a future refactor that forgot to wrap one.

7. close() docblock clarity — addressed. The docblock now states that close() resolves once subscribers are cleared and the watcher is closed, and that callers needing the SSE callbacks to have settled (the standalone server's shutdown path) should call httpServer.closeAllConnections() after.

8. validateSettings forward-reference — out of scope per your own note (introduced by the settings PR). Agree the file is getting large; happy to take a structural pass in a follow-up issue if you want.

9. Banner / endpoint docs — checked clients/web/README.md and it does not enumerate endpoints, so no doc change needed. The top-of-file docblock in core/mcp/remote/node/server.ts was already updated to include /api/servers/events.

All checks green:

• npm run validate — 142 files / 1684 tests
• npm run test:integration — 21 files / 440 tests (3 new in servers-events.test.ts)
• npm run test:storybook — 69 files / 306 tests
• coverage gate satisfied (useServers.ts 97% lines, server.ts 95%)

[cliffhall]

@claude review

[claude]

Claude encountered an error after 22s —— View job

---

I'll analyze this and get back to you.

[claude]

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

---

Follow-up review — d90f8e6 (response to prior findings)

• Read d90f8e6 diff and verify claims
• Walk each prior finding through the new code
• Look for new issues introduced by the fixes
• Post review

---

TL;DR

The fixes do what they say. All nine prior findings are either addressed correctly or have a reasoned skip. Verdict: approve; no blockers. Three small notes below — none worth holding the PR.

---

Verification of prior findings

#	Finding	Status	Notes
1	External-edit race masked by self-suppression	✅ Fixed	Pre-stat + divergence check in writeMcpAndTrackMtime (server.ts:304-344). Walked every interleaving (backend-then-external, external-then-backend, back-to-back externals); peers get exactly one broadcast in each case.
2	setLoading(true) flicker on SSE refresh	✅ Fixed	refreshInternal(background) split at useServers.ts:85-111. Public refresh() preserves the old behavior so existing consumers are unchanged.
3	Multiple SSE frames → concurrent fetches	✅ Fixed (within-chunk)	sawFrame coalesces per reader.read() chunk (useServers.ts:152-160). Cross-chunk debounce explicitly skipped, with reasoning inline. Reasonable call.
4	No SSE reconnect	⏭ Skip (deliberate)	Agree with the rationale — no current consumer for an eventsConnected flag.
5	Watcher stats outside the write lock	✅ Fixed	Subsumed by (1); watcher handler correctly stays lock-free because it never writes.
6	PUT/DELETE coverage	✅ Fixed	Two new per-verb tests in servers-events.test.ts:228-280.
7	close() docblock	✅ Fixed	server.ts:93-102 now specifies what close() awaits and what callers must do additionally.
8	validateSettings forward reference	⏭ Out of scope	Fine as follow-up.
9	Banner/endpoint docs	✅ Verified	clients/web/README.md checked, doesn't enumerate endpoints.

---

New notes on the fix commit

A. The pre-stat fix catches notifications, not lost updates

writeMcpAndTrackMtime detects that an external edit happened and broadcasts — good. But the backend still overwrites the external edit with its own merged state, because the route handler's readMcpConfig() ran before the external edit landed. Peers learn "something changed" and re-fetch, but what they receive is the backend's overwrite, not the external edit's content.

This matches the stated goal ("peer-tab visibility") and the new integration test only asserts notification. It is the right tradeoff for a single-user local dev tool. Worth an inline comment in writeMcpAndTrackMtime acknowledging that the broadcast is a notification-of-divergence, not a preservation-of-external-content, so a future reader doesn't assume the external edit survived.

B. PUT/DELETE test comment is slightly off

servers-events.test.ts:237-240:

// A GET first so the backend records `lastWrittenMtimeMs` from any
// seed-write path it owns and so writeMcpAndTrackMtime's pre-stat below
// doesn't trip the external-edit-detected branch.
await fetch(`${h.baseUrl}/api/servers`);

The pre-stat branch is guarded by if (lastWrittenMtimeMs !== null) (server.ts:319), so without any prior backend write it's still null and the whole branch is skipped. The GET may or may not cause a seed-write depending on whether readMcpConfig() decides to seed (the test pre-writes { mcpServers: { alpha: ... } }, which is already non-empty). Either way the test passes, but the comment's causal claim ("so writeMcpAndTrackMtime's pre-stat doesn't trip…") is not quite right. The POST test at :203-226 is equivalent and omits the GET entirely, which makes the asymmetry visible. Not a bug; a clarity nit.

C. Missing regression test for finding #2

The fix for the loading flicker (refreshInternal(background)) isn't covered by an explicit test asserting loading stays false during an SSE-triggered refresh. The existing auto-refreshes when the /api/servers/events SSE channel signals a change test at useServers.test.tsx:228-260 verifies the state updates but not the flicker behavior. A cheap add around that block:

// After the initial load completes, SSE-triggered refreshes must not
// re-toggle loading — consumers rendering a skeleton would flash.
await new Promise((r) => setTimeout(r, 200));
writeFileSync(h.configPath, /* external edit */);
// Sample `loading` while the refetch is in-flight:
expect(result.current.loading).toBe(false);
await waitFor(() => expect(result.current.servers.map((s) => s.id)).toEqual(["external"]));
expect(result.current.loading).toBe(false);

Would lock in the behavior so a future refactor that reaches for setLoading(true) in the SSE path gets caught. Fix this →

---

Verdict

Fixes land cleanly, race analysis holds up under interleaving walk-through, tests are a real improvement. LGTM — items A, B, C are polish, not blockers.
· Branch: feat/1345-hot-reload-mcp-json

[cliffhall]

Thanks for the second pass. All three items addressed in fd7faa4:

A. writeMcpAndTrackMtime — notification vs preservation — addressed. The inline comment now explicitly calls out the two orderings: depending on whether the external write landed before or after readMcpConfig(), the external content either round-trips inside our serialized payload (preserved) or gets read-around and overwritten (lost). Either way peers learn there's been a change and re-fetch the authoritative on-disk state. A read-modify-write retry loop would close the lost-update window but is out of scope for this PR and probably not worth it for a single-user local dev tool — noted that explicitly so it doesn't become a follow-up surprise.

B. PUT/DELETE test comment — addressed by removing the GET entirely. You're right that the pre-stat branch is short-circuited by lastWrittenMtimeMs !== null so the GET was a no-op; the comment claiming it primed anything was just wrong. Both per-verb tests now mirror the POST test's structure exactly, which makes the suppression contract easier to read.

C. Loading-flicker regression test — addressed. Used a render-history array rather than a single-point sample so the assertion is deterministic under React batching: the hook is wrapped in a tracker that pushes loading on every render, and after the SSE-driven refresh lands we assert every render value since the baseline is false. A future regression that reintroduces setLoading(true) in refreshInternal(background=true) would have at least one render with loading === true in that slice, and the every(v => v === false) would fail.

All checks green:

• npm run validate — 142 files / 1685 tests (was 1684; +1 from the regression test)
• npm run test:integration — 21 files / 440 tests; servers-events still 7
• npm run test:storybook — 69 files / 306 tests
• coverage gate: useServers.ts 97.18% lines / 96.25% statements

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 18s —— View job

---

Follow-up review — fd7faa4 (pass 3)

• Read the fd7faa4 diff
• Verify finding A (writeMcpAndTrackMtime notification-vs-preservation comment)
• Verify finding B (PUT/DELETE test comment fix)
• Verify finding C (loading-flicker regression test)
• Look for any new issues introduced
• Post review

---

TL;DR

All three items from pass 2 land cleanly. No new issues. LGTM — ship it.

---

Verification

A. writeMcpAndTrackMtime comment — notification vs preservation ✅

core/mcp/remote/node/server.ts:314-323 now names both interleaves explicitly: if the external write lands before readMcpConfig() the content round-trips inside the serialized payload, and if it lands after it gets read-around and overwritten by writeStoreFile. In both cases peers learn of divergence and re-fetch the authoritative on-disk state. The "out of scope for a single-user local dev tool" note is the right forward-looking framing so a future reader doesn't re-open this as a bug.

B. PUT/DELETE test comment ✅

servers-events.test.ts:234 and :261 — the misleading GET /api/servers prefix is gone. Both per-verb tests now mirror the POST test's structure exactly (:203-226), which makes the three-way symmetry easy to read at a glance. Walked the lastWrittenMtimeMs !== null guard: without a prior backend write the pre-stat branch is short-circuited regardless, so the removed GET was indeed a no-op.

C. Loading-flicker regression test ✅

useServers.test.tsx:262-303 — render-history approach is the right call. Single-point sampling would miss the bug under React batching (setLoading(true) + setServers() would batch to a single render, but r.loading captured inside the factory gets the post-batch value); tracking every render and asserting .every(v => v === false) over the post-baseline slice would catch that.

baselineLen = loadingHistory.length after waitFor(loading === false) correctly anchors the slice after the mount phase. The expect(postRefreshLoadings.length).toBeGreaterThan(0) sanity check guarantees the SSE cycle actually produced a render (via setServers(...)) so a silently-dead SSE reader couldn't vacuously satisfy the .every() assert. Nice defensive touch.

Walked the failure mode: if a future refactor reinstated setLoading(true) in refreshInternal(background=true), that setLoading call would show as loading: true in the render before setServers lands, and the .every(v => v === false) would fail. Confirmed.

---

New notes (none blocking)

No new issues spotted. Diff is small and surgical; nothing introduced that wasn't explicitly called out in the response comment.

One minor observation for future work (not actionable here): the SSE handler in useServers.ts:149-161 coalesces within a decode chunk via sawFrame, but the inline comment at :140-148 doesn't mention the cross-unmount edge case — if the hook unmounts mid-decode the void refreshInternal(true) fires without awaiting, which could trigger a setState-after-unmount warning in strict mode. In practice the subsequent controller.abort() on unmount causes the fetch inside refreshInternal to reject, landing in the catch branch — the setError path. Probably benign given setError bails out cleanly, but if strict-mode warnings show up later, this is the first place to look. Not worth a change now.

---

Verdict

Three passes in, race analysis held up, test gaps closed. Approve for merge.
· Branch: feat/1345-hot-reload-mcp-json