fix(servers): make settings form re-render on every keystroke (#1361)

[cliffhall]

Closes #1361.

Problem

ServerSettingsModal is fully controlled by App.tsx. Pending edits were stashed in a useRef and a 300ms-debounced PUT was scheduled — but refs don't trigger re-renders, so the form's value prop stayed stale until the PUT round-tripped and useServers refetched. Result: clicking "Add Header" / "Add Metadata" showed no input row, and typing in OAuth fields had no character echo, until ~500ms after each interaction.

The bug landed in #1353 with the debounce pattern; #1356 made it user-visible by adding keychain writes to the per-keystroke PUT path.

Fix

Replace the ref with useState. Every change re-renders immediately. Keep the 300ms debounce on the PUT side. Initialize the draft from the server-list entry only when the modal opens to a new target id — not on every servers change — so background SSE-triggered refreshes don't clobber the user's in-progress edits. Flush pending edits synchronously on modal close so the final keystrokes always land.

Files

• clients/web/src/App.tsx — pendingSettingsRef → settingsDraft state; flushPendingSettings → sendSettingsUpdate + inline flush at close; settingsModalTarget removed (now derived inside the open-effect).

Test plan

• npm run validate — passes (format / lint / build / coverage / 1743 tests).
• Manual verification in the dev server:
  • Click "Add Header" — input row appears instantly.
  • Click "Add Metadata" — input row appears instantly.
  • Type 18 chars in OAuth Client ID — every character echoes immediately.
  • Type 22 chars in OAuth Client Secret — every character echoes immediately.
  • On modal close, clientId lands in ~/.mcp-inspector/mcp.json and clientSecret lands in the macOS Keychain under mcp-inspector:${serverId}:oauth-client-secret.

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

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

---

Claude PR Review

• Read the PR diff vs. origin/v2/main
• Read current App.tsx (lines 760-880 + wiring at 933/982)
• Analyze the state/effect/debounce logic for correctness
• Check flush-on-close behavior and edge cases
• Post review findings

---

Summary

The fix is correct and the PR description accurately describes the pre-existing bug (ref-based draft producing no re-renders). Swapping to useState is the right call, and the reasoning in the comments is worth keeping around — especially the note about deliberately omitting servers from the init effect deps.

The diff is net +31 lines, much of which is comments. I found no correctness regressions, just a few minor observations.

Correctness — looks good

• Timer cleanup paths all line up:
  • onSettingsChange clears before rescheduling (L844).
  • onSettingsModalClose clears + flushes synchronously (L859-865).
  • Unmount effect clears (L873-880).
  • Effect init-on-open doesn't need to touch the timer because close handler already did.
• Stale-closure risk in the debounce is contained: setTimeout captures id and next at call time, so if settingsModalTargetId later changes, the pending PUT still targets the original server with the user's typed payload. That's the desired behavior.
• onSettingsChange guards with if (!settingsModalTargetId) return; (L837) — protects against stray onChange events after the modal is closed (e.g., onBlur firing during teardown).
• Flush on close is synchronous relative to setSettingsModalTargetId(undefined), so the final edits can't be lost to the effect nulling the draft.

Minor observations

1. servers-not-loaded race at modal open (low)

clients/web/src/App.tsx:787-802 — the effect intentionally drops servers from its deps (well-justified: avoids clobbering mid-edit). But as a side effect, if the modal is ever opened before servers hydrates, target is undefined → draft initializes to the empty-shell default → never resyncs when servers arrives.

In the current UI this is unlikely (the "Settings" button only exists on a rendered server card, which requires servers to be populated), so probably not worth fixing. Worth a one-liner in the comment noting the assumption, in case a future caller triggers the modal from somewhere that doesn't depend on a rendered server row.

2. settingsModalValue useMemo is now redundant (nit)

clients/web/src/App.tsx:807-816 — with the effect seeding the draft on open, settingsDraft is only null when the modal is closed. Mantine renders the modal shell even when opened={false}, so the fallback still has to exist — but the useMemo buys nothing over a plain constant:

const EMPTY_SETTINGS: InspectorServerSettings = {
  headers: [],
  metadata: [],
  connectionTimeout: 0,
  requestTimeout: 0,
};
// ...
const settingsModalValue = settingsDraft ?? EMPTY_SETTINGS;

One fewer ref allocation and the identical default object is shared with the effect's fallback at L794-799.

Fix this →

3. No regression test (policy)

AGENTS.md requires per-file coverage ≥ 90 lines / ≥ 80 functions and tests for new/modified code. This PR changes non-trivial logic in App.tsx (debounce, init-on-open, flush-on-close) but adds no test. A small hook-level test that asserts:

• onSettingsChange updates the visible value synchronously (guard against the regression this PR fixes), and
• onSettingsModalClose flushes a pending edit when invoked mid-debounce,

would lock this behavior in. Happy to add one in a follow-up if you'd like.

Verified behavior in the PR description

The manual checks listed (add-header, add-metadata, OAuth client id/secret echo, final-keystroke persistence at close) line up with the code paths I traced — the immediate setState + 300ms debounce + synchronous flush-on-close chain delivers exactly that.

---

· branch fix/settings-form-laggy-typing

[cliffhall]

Thanks for the review. All three observations addressed in f4537909:

1. Regression test (the policy ask). Extracted the draft state + debounce + flush logic into core/react/useSettingsDraft.ts (~140 lines, with the design rationale documented in the file header) and added 13 unit tests in clients/web/src/test/core/react/useSettingsDraft.test.tsx. The tests pin:

• the regression Server settings form: typing is laggy; Add Header/Metadata shows no row until PUT round-trips #1361 was about: onChange updates draft synchronously
• targetId-keyed re-seeding when the modal switches servers
• the "background refresh must not clobber in-progress edits" contract (verified by swapping the resolveInitial closure mid-edit and asserting the draft stays put)
• debounce window respected, burst-collapse with the latest payload winning
• flush() fires the pending PUT synchronously and clears the timer
• onPersist rejection routes to onError
• a pending PUT for the previous target still resolves against that target after a switch (closure captures id at schedule time)
• onChange with no target is a no-op

App.tsx is now thinner — it just wires inputs to the hook plus a tiny flushSettingsDraft() on close.

2. settingsModalValue useMemo redundant. Hoisted to a module-scope EMPTY_SETTINGS const shared between the hook's init path (via resolveInitial) and the closed-modal fallback. Plain settingsDraft ?? EMPTY_SETTINGS.

3. servers-not-loaded race. Added a comment on the resolveInitial callback explaining that we read servers from this render's closure under the assumption that the entry point is the "Settings" button on a rendered card (so servers is non-empty by the time the hook is called). Notes the empty-shell fallback as the safety net for any future caller that opens the modal pre-hydration.

CI: 144 files, 1756 tests, all green.

[cliffhall]

@claude review

[claude]

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

---

Claude PR Review — follow-up on f4537909

• Fetch branch & diff vs origin/v2/main
• Read core/react/useSettingsDraft.ts
• Read clients/web/src/test/core/react/useSettingsDraft.test.tsx
• Read updated App.tsx wiring
• Verify contracts the previous review asked for are covered
• Post findings

---

Summary

All three items from the prior round are addressed cleanly. The extracted hook is the right shape, the module-scope EMPTY_SETTINGS eliminates the duplicate fallback, and the resolveInitial comment at App.tsx:790-796 now calls out the servers-not-loaded assumption. Net diff is +519/-76 but the bulk is the new test file (+326) and the documented hook (+141), not App.tsx churn. App.tsx itself is slimmer and easier to read — the debounce/flush machinery is gone from the component body and replaced with a 5-key hook call.

The 13 tests in useSettingsDraft.test.tsx directly pin every behavior the first review flagged — including the subtle "swap resolveInitial's closure mid-edit" case at L83-111, which is exactly the regression contract that keeps background SSE refreshes from clobbering user edits.

Correctness — looks good

• Callbacks read through refs, effect depends only on targetId → no accidental re-init on parent re-render. (useSettingsDraft.ts:81-86, 88-94) Well explained in the header comment too.
• Timer captures id and next at schedule time (useSettingsDraft.ts:122-126) — pending PUT for the prior target still lands correctly. Test at L297-325 locks this.
• flush is synchronous and clears timerRef before calling persist, so a post-flush timer-advance can't produce a duplicate PUT. Test at L250-253 pins this specifically — nice belt-and-suspenders.
• Unmount cleanup at L99-106 clears the timer. No spurious PUT after route change / HMR.
• onError wiring: the notifications.show in App.tsx at L810-816 is the right call — the original silent-fail path was especially painful for OAuth secrets since the keychain write happens on the PUT path.

Minor observations (all non-blocking)

1. Latent: a mid-debounce targetId switch (without flush) loses the pending edit on next onChange (very low)

useSettingsDraft.ts:121 — onChange unconditionally calls clearTimeout(timerRef.current). If a caller ever switches targetId mid-debounce without first calling flush(), and then the new target receives an onChange, the prior target's pending PUT is canceled. Today the only switch path is onSettingsModalClose (which calls flush first), so this is unreachable — but the test at L297-325 only covers the no-onChange-after-switch case.

Not worth fixing now; worth a one-line comment at L121 noting "callers must flush() before changing targetId to preserve pending edits" so a future refactor doesn't quietly regress it.

2. flush identity churns on every keystroke (nit)

useSettingsDraft.ts:131-138 — flush depends on [targetId, draft, persist], so its identity changes on every setDraft. That makes onSettingsModalClose (App.tsx:822-825) also re-allocate on every keystroke, which then makes the onClose prop identity on ServerSettingsModal change per keystroke. Harmless in practice (Mantine's Modal doesn't care), but the useCallback is doing no work.

If you wanted to stabilize it, read draft through a ref inside flush (same pattern as the callback refs) and keep the dep list to [persist]. Minor. Totally skip-able.

3. Unmount mid-edit loses the final <debounceMs window of typing (intentional, probably)

useSettingsDraft.ts:99-106 — the cleanup effect clears the timer but doesn't flush. So a route change / HMR during typing drops the last edits. This matches how the modal's close-handler flow works (the user actively dismisses, not just navigates away) and a flush-on-unmount could trigger a PUT against a component that's already unmounting, which is a worse footgun. Current behavior is fine — calling it out so the trade-off is explicit in case anyone wonders later.

Test coverage

Strong. The 13 cases collectively exercise every exported surface of the hook and each named contract in the JSDoc. If you wanted one more for completeness, a test for the unmount-cleanup path ("timer pending → unmount → advance timers → no persist") would round it out — but the behavior is simple enough that the current coverage is already above the bar.

---

LGTM. The response to the prior review is thorough and the extracted hook is a legitimate improvement beyond just satisfying the coverage gate — it makes the behavior directly testable and gives the contracts a place to live as documented types.
· branch fix/settings-form-laggy-typing

[cliffhall]

Thanks for the LGTM. Picked up all three minor observations in daa90f7b:

1. flush identity churn (nit). Read draft and targetId through refs so the returned flush identity is stable across keystrokes. The consumer's onSettingsModalClose (which closes over flush) no longer re-allocates per character, which means the modal's onClose prop identity also stays stable. New test flush identity is stable across keystrokes pins this — result.current.flush === flushAfterMount after a burst of onChange calls.

2. Latent mid-debounce target switch (very low). Added a comment at the onChange clearTimeout site noting that callers must flush() before changing targetId to preserve pending edits. The only switch path today (onSettingsModalClose) already does that — the comment is so a future refactor doesn't quietly regress it.

3. Unmount mid-edit (intentional). Added a test that documents and pins the trade-off — typing → unmount → advance timers past the debounce window → assert onPersist was never called. The test's own comment explains why (flush-on-unmount could fire a PUT against an unmounting component, which is a worse footgun than losing the final <debounceMs window of typing on route change / HMR).

Validate clean: 144 files, 1758 tests, all green.

[cliffhall]

@claude review

[claude]

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

---

Claude PR Review — follow-up on daa90f7b

• Fetch branch & diff vs origin/v2/main
• Re-read core/react/useSettingsDraft.ts for the stable-identity / comment changes
• Re-read useSettingsDraft.test.tsx for the two new tests
• Re-read App.tsx wiring
• Verify all three minor observations from the prior round are resolved
• Post findings

---

Summary

All three observations from the previous round are resolved cleanly. The flush identity is now stable, the mid-debounce switch contract is documented at the site that enforces it, and the unmount trade-off is pinned by an explicit test. Net diff for this iteration is small and focused: two refs added (draftRef, targetIdRef), a [persist]-only dep list on flush, a comment at the clearTimeout in onChange, and two new tests. No behavior change for the happy path, just guarantees locked in.

Each observation, verified

1. flush identity stable across keystrokes — fixed. useSettingsDraft.ts:142-155 reads draft and targetId through refs and declares flush with useCallback(..., [persist]). Since persist itself has [] deps (L114-118), flush's identity is truly stable for the lifetime of the hook. Test flush identity is stable across keystrokes (test file L358-377) asserts result.current.flush === flushAfterMount after three onChange calls — exact pin of the contract.

2. Mid-debounce target switch — documented. useSettingsDraft.ts:127-131 now carries the "callers must flush() before changing targetId" note directly at the clearTimeout in onChange. The comment correctly identifies the single consumer (onSettingsModalClose in App.tsx) that closes the window today. The existing test at L297-325 (pending PUT for the previous target id still goes to that target after a switch) exercises the closure-capture half of the contract — the comment is the guardrail for the other half.

3. Unmount mid-edit — pinned as intentional. Test at L329-352 types, unmounts, advances past the debounce window, and asserts onPersist was never called. The test's own comment captures the trade-off reasoning ("flush-on-unmount could fire a PUT against an unmounting component, which is a worse footgun"). Good documentation of an intentional design choice rather than an accidental one.

Small cross-checks

• flush guard if (!timerRef.current) return; (L147) is the right signal — after a debounce fires naturally, timerRef is nulled on L135, so a subsequent close produces a no-op flush rather than a duplicate PUT against already-persisted state.
• persist deps of [] (L118) is correct because it reads its callbacks through refs — no stale-closure risk even though the deps list is empty.
• The App.tsx comment at L790-796 still calls out the servers-not-loaded fallback assumption; wiring remains readable.
• Tests are framework-consistent with the rest of the repo (renderHook from @testing-library/react, fake timers, no real DOM needed).

Nothing blocking

This is ready to ship. The refactor + tests add real value beyond the coverage gate — the hook's contracts are now explicit both as code (JSDoc), tests, and inline rationale, which is the right shape for a piece of state machinery that a future contributor might otherwise try to "simplify" without realizing which behaviors are load-bearing.

LGTM.

--- · branch fix/settings-form-laggy-typing