refactor(web): consolidate the three GET /api/config hooks into one

[cliffhall]

Closes #1643

Summary

The web app made three independent GET /api/config fetches on load — useSandboxUrl, useServerListWritable, and useInspectorVersion each fetched the same static InitialConfigPayload and read one field from it. This consolidates them into a single useInitialConfig hook that fetches once and exposes { version, sandboxUrl, writable, loading }.

Changes

• Add core/react/useInitialConfig.ts — one fetch of /api/config, exposing all three fields with the same tolerant parsing/defaults the old hooks used (writable defaults true; version/sandboxUrl undefined when absent or non-usable-string; unmount-safe cancellation guards).
• Update clients/web/src/App.tsx to destructure all three fields from the single hook.
• Remove useSandboxUrl / useServerListWritable / useInspectorVersion and their tests (nothing else consumed them).
• Consolidate the fetch-branch matrix (present / absent / non-string / non-ok / network-throw / post-unmount guards / auth header / global-fetch fallback) into one useInitialConfig.test.tsx.
• Fix a stale comment in test/setup.ts.

Acceptance

• ✅ One GET /api/config fetch on load (asserted via toHaveBeenCalledTimes(1)).
• ✅ version / sandboxUrl / writable behave exactly as before, including legacy-backend tolerance.
• ✅ Per-file ≥90 coverage maintained; full npm run ci (validate → coverage → smoke → storybook) passes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

[cliffhall]

@claude review

[claude]

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

---

Review: consolidate the three GET /api/config hooks

• Gather context (read diff + new/deleted files)
• Verify no stale references to the removed hooks
• Review useInitialConfig.ts for correctness/behavior parity
• Review App.tsx wiring
• Review test coverage of the consolidated matrix
• Post review

Verdict: LGTM. This is a clean, behavior-preserving consolidation — 3 identical fetches → 1, with a net −439 lines. No blocking issues. Three minor test/comment nits below.

---

What I verified

Behavior parity is exact. I diffed the merged load() in core/react/useInitialConfig.ts:82-110 against all three deleted hooks from origin/v2/main. The union is faithful in every detail:

Aspect	Old (×3)	New	Match
base normalization (replace(/\/$/, ""))	✅	✅	✅
x-mcp-remote-auth: Bearer … only when token present	✅	✅	✅
Three isCancelled() guards (post-fetch, post-json, finally)	✅	✅	✅
!res.ok → leave defaults, still clear loading	✅	✅	✅
version/sandboxUrl: usable non-empty string else undefined	inline	usableString() helper	✅
writable: only explicit false flips it	✅	✅	✅
catch {} swallow → defaults	✅	✅	✅
useCallback deps [base, authToken, doFetch]	✅	✅	✅

No stale references. grep across *.ts/tsx/md/json for the three removed hook names returns only the two intentional historical mentions in the new hook's docblock (useInitialConfig.ts:22-24) and the test header. There's no core/react barrel to update, and no README/AGENTS section enumerates these hooks, so no doc changes are owed. clients/web/src/test/setup.ts:37 comment correctly updated.

"One fetch" claim holds. grep for api/config shows the hook is the only browser-side consumer; the nearby loadClientConfigRemote call in App.tsx:715 hits a different endpoint (/api/storage/client, core/client/remote.ts:29), so it isn't a fourth duplicate.

CI is green on the head commit (run 30134424637) — validate → coverage → smoke all passed, so the ≥90 per-file gate on the new core/react/useInitialConfig.ts is satisfied. (I could not run the suite in this checkout — clients/web/node_modules is absent on the action runner — so I'm relying on that run plus reading the branch matrix.)

Things I liked

• ConfigPayload stays a local minimal interface with unknown fields (useInitialConfig.ts:54-59) rather than importing InitialConfigPayload from core/mcp/remote/node/server.ts. That's the right call twice over: it keeps a Node module out of the browser import graph, and unknown is what makes the legacy-backend tolerance type-honest instead of a lie the compiler believes.
• usableString() (:62-64) dedupes what was two copies of the same inline ternary, and both of its && branches are exercised by the tests (non-string via the omitted-field case, empty-string via the "" case).
• Nice unadvertised win: all three values now land in a single state batch, so writable and sandboxUrl can no longer arrive in different renders — one less chance of CRUD controls or the Apps iframe flickering through an intermediate state.
• App.tsx:698-710 — the consolidated comment preserves all three of the original explanatory comments (why writable exists, why the browser can't read the version off disk). Good; those were the valuable part of the deleted code.

Minor nits (non-blocking)

1. clients/web/src/test/core/react/useInitialConfig.test.tsx:111-124 — this test doesn't test what its name says.

it("keeps writable true for any non-false value", async () => {
  const fetchFn = vi.fn().mockResolvedValue(jsonResponse({ writable: undefined }));

writable: undefined is indistinguishable from an absent key — and can never appear in real JSON — so this duplicates "applies each field's default when the payload omits it" (:88) and exercises no new branch. To actually cover "any non-false value", use values a nonconforming backend could really send: null, 0, "no". Each is falsy yet !== false, so each should stay writable — that's the assertion with teeth. Fix this →

2. useInitialConfig.test.tsx:207-209 — the post-unmount comment overstates what's proven.

// No assertion error / React warning means the guards held; every field
// stayed at its initial value.

React 18 removed the setState-after-unmount warning, and result.current is frozen at the last render once unmount() runs — so both post-unmount tests (:186, :216) would pass even if the isCancelled() guards were deleted. They're valuable as branch coverage for the three guards, but they can't catch a regression in them. There's no clean way to observe this in React 18 (a render counter wouldn't help either — setState on an unmounted fiber is a silent no-op), so I'd keep the tests and just soften the comment to say they exist for guard-branch coverage rather than implying they detect a violation. This wording is inherited verbatim from the deleted useSandboxUrl.test.tsx, so it's not a regression from this PR.

3. useInitialConfig.ts:50-51 — loading is exposed but unconsumed. App.tsx:703-707 drops it, same as the three old hooks did. Keeping it is reasonable (it's the natural shape, and it's now more useful than before since one flag covers the whole payload), but it's worth noting that the "not yet loaded" and "backend omitted it" cases are still indistinguishable to consumers of sandboxUrl/version alone — e.g. the Apps screen shows its unavailable state during the in-flight window. Pre-existing, out of scope here; flagging only in case loading is meant to be wired up in follow-up work.

One thing I checked and am satisfied with

A single request is also a single point of failure — a non-ok or thrown /api/config now defaults all three fields at once, including leaving writable: true (full CRUD shown) after e.g. a 401. That's identical to the old behavior, though: all three hooks hit the same URL with the same header, so they'd have failed together anyway. Not a regression introduced here.

---

· branch v2/fix-1643-consolidate-config-hooks

[cliffhall]

Thanks for the thorough review! Addressed the two actionable nits in 884bb7d:

Nit 1 — "keeps writable true" test was a dupe. Fixed. It now loops over null, 0, and "no" — real falsy-but-not-false values a nonconforming backend could actually send — each asserted to leave the list writable. The old writable: undefined was indistinguishable from an absent key and duplicated the omitted-field test, as you noted.

Nit 2 — overstated post-unmount comment. Softened. It now says the test exists to exercise the isCancelled() guard branches for coverage and explicitly notes it can't detect their removal (React 18 dropped the setState-after-unmount warning; result.current is frozen at the last render). You're right that this wording was inherited verbatim from the deleted useSandboxUrl.test.tsx.

Nit 3 — loading exposed but unconsumed. Leaving as-is intentionally. It's the natural hook shape, it's pre-existing behavior (all three old hooks did the same), and wiring it up to distinguish "in-flight" from "backend omitted it" is out of scope for a pure consolidation. Flagging for potential follow-up rather than expanding this PR.

@claude review

[claude]

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

---

Re-review: nit fixes in 884bb7de

• Read the follow-up commit diff
• Verify nit 1 fix (writable non-false matrix)
• Verify nit 2 fix (post-unmount comment)
• Re-check branch parity / stale references
• Post review

Verdict: LGTM — both actionable nits are fixed correctly, and I agree with deferring nit 3. One small leftover inconsistency the fix surfaced, plus one optional test-ergonomics suggestion. Neither blocks merge.

---

Nit 1 — fixed ✅

clients/web/src/test/core/react/useInitialConfig.test.tsx:112-129 now loops [null, 0, "no"], each asserted to leave writable === true. That's exactly the assertion with teeth: all three are falsy yet !== false, so an implementation that used !body.writable or body.writable ?? true instead of body.writable !== false (core/react/useInitialConfig.ts:101) would now fail on all three, where writable: undefined would have passed the ?? variant. The unmount() per iteration is the right hygiene — each render gets its own fetchFn, so no cross-iteration bleed, and the fresh result per iteration makes the waitFor unambiguous.

Branch parity holds: the !== false true leg is covered three times here and the false leg by the happy-path test at :28, so the fix doesn't trade coverage for realism.

Nit 2 — fixed ✅

useInitialConfig.test.tsx:215-218 now states the test exercises the guard branches for coverage and explicitly disclaims detection of their removal. Accurate and appropriately scoped.

One leftover the fix surfaced (worth a one-line follow-up)

core/react/useInitialConfig.ts:80-81 still carries the old claim the new test comment contradicts:

// `isCancelled` lets the effect drop a response that resolves after unmount,
// avoiding a setState on a dead component (React 18 warns, doesn't throw).

The test now says React 18 dropped the setState-after-unmount warning — which is the correct half. So the two comments in this PR disagree with each other, and the hook's is the wrong one (the warning was removed in React 18; it's a silent no-op). The guard is still worth keeping — it avoids a pointless state update and, more usefully here, prevents a stale in-flight response from overwriting fresh state after a baseUrl/authToken change re-runs the effect. That's the accurate rationale, and it's stronger than the warning one. Suggested wording:

// `isCancelled` lets the effect drop a response that resolves after unmount or
// after a re-run (baseUrl/authToken change), so a stale payload can't overwrite
// current state. React 18 no longer warns on setState-after-unmount — it's a
// silent no-op — so the guard is about correctness, not the warning.

Fix this →

Optional test ergonomics

useInitialConfig.test.tsx:116 — a bare for loop means a failure reports only "keeps writable true for any non-false value", with no indication of which value broke, and the first failure short-circuits the remaining two. it.each([null, 0, "no"])("keeps writable true for writable=%p", …) gives you three independently-reported cases for the same assertions. Purely a debuggability preference; the loop is correct as written.

Nit 3 — agreed, leave it

loading staying exposed-but-unconsumed is the right call for a pure consolidation. It's now more defensible than before (one flag covers the whole payload rather than three racing ones), and wiring it into the Apps screen's unavailable-state distinction is separable work.

Notes

• No stale references. grep over *.{ts,tsx,md,json} for the three removed hook names still returns only the two intentional historical mentions (useInitialConfig.ts:22-24, test header :5-6).
• Test-only double casts (as unknown as Response ×2, as unknown as typeof fetch) are carried over verbatim from the deleted useSandboxUrl.test.tsx — standard partial-mock fixtures, not a regression from this PR.
• Couldn't run the suite locally — clients/web/node_modules is absent on this runner. CI run 30135516193 on 884bb7de was still in progress when I checked; the prior commit's run was green, and this commit touches only one test file, so I'd expect the ≥90 gate to hold — worth a glance before merge.

---

· branch v2/fix-1643-consolidate-config-hooks

[cliffhall]

Both addressed in 54d4e41:

Leftover comment contradiction — fixed. core/react/useInitialConfig.ts:80-83 now states the real rationale: the isCancelled() guard drops a response that resolves after unmount or after an effect re-run (baseUrl/authToken change), so a stale payload can't overwrite current state — and notes React 18 no longer warns on setState-after-unmount (silent no-op). This matches the corrected test comment and, as you noted, is the stronger rationale.

it.each ergonomics — adopted. The writable-non-false test is now it.each([null, 0, "no"])(...), so each value reports as an independent case instead of a short-circuiting loop.

@claude review

[claude]

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

---

Re-review: 54d4e412

• Read the follow-up commit diff
• Verify the corrected isCancelled rationale against the actual effect wiring
• Verify the it.each conversion
• Post review

Verdict: the comment fix is correct and complete. The it.each conversion has one real defect — %p is not a vitest format specifier, so all three cases render with the same literal title, which is precisely the debuggability the change was meant to buy. Non-blocking, one-character-class fix.

---

Comment fix — correct ✅

core/react/useInitialConfig.ts:80-83 now states the real rationale, and I verified each clause against the wiring rather than taking it on faith:

• "after a re-run (baseUrl/authToken change)" — accurate. load's useCallback deps are [base, authToken, doFetch] (:111) and the effect's deps are [load] (:120), so a baseUrl/authToken change produces a new load → React runs the cleanup (cancelled = true, :118) before re-running the effect. The prior call's continuation therefore sees isCancelled() === true at all three checkpoints (:93, :95, :108) and can't clobber the new fetch's values. This is the stronger rationale, and it's the one that describes a real failure mode.
• "React 18 no longer warns … silent no-op" — correct, and it now agrees with the test comment at useInitialConfig.test.tsx:215-218. The contradiction is gone.

The class-level docblock (:27-29) still describes only the unmount half ("Unmount-safe: a response that resolves after unmount is dropped"). Not a contradiction — :26 already documents the re-fetch — but if you touch this file again, extending that sentence to "…after unmount or a re-fetch" would make all three comments say the same thing.

it.each conversion — cases run independently, but the titles don't distinguish them ⚠️

clients/web/src/test/core/react/useInitialConfig.test.tsx:115-116:

it.each([null, 0, "no"])(
  "keeps writable true for writable=%p (falsy but not false)",

%p is a Jest specifier, not a vitest one. Verified against the actual vitest 4 sources (the repo pins vitest: ^4.1.0; I unpacked 4.1.10, since clients/web/node_modules is absent on this runner):

• @vitest/utils → display.js:641: const formatRegExp = /%[sdjifoOc%]/g — the recognized set is s d j i f o O c %. No p.
• @vitest/runner → chunk-artifact.js:2169-2219 (formatTitle): it walks the template with that same formatRegExp; matched segments go through format(match, item), and every non-matching segment is emitted verbatim (after $key attribute substitution, which doesn't touch %p). Unlike a bare format() call, formatTitle never appends leftover items to the end.

Net effect: all three cases are titled keeps writable true for writable=%p (falsy but not false) — literal %p, three identical names. The good half of the fix still lands (three independent test entries, no short-circuit on first failure, as intended), but a failure still won't tell you which value broke, which was the point.

%s or %j is in the supported set and does the right thing here — %s → null / 0 / no; %j → null / 0 / "no" (%j additionally distinguishes the string case from the number, which is arguably worth it for this particular matrix):

it.each([null, 0, "no"])(
  "keeps writable true for writable=%j (falsy but not false)",

Fix this →

Dropping the per-iteration unmount() is fine

The loop version called unmount() each pass; the it.each version doesn't. That's correct here — clients/web/src/test/setup.ts:2-3 registers RTL's cleanup in an afterEach, so each case's hook is unmounted between cases. Every other test in this file relies on the same thing.

Everything else re-verified

• Branch coverage on body.writable !== false (useInitialConfig.ts:103) is unchanged by the conversion: the true leg × 3 here, the false leg at :28. The %p issue is cosmetic to the title only — the cases still execute and assert.
• Types are fine: it.each([null, 0, "no"]) infers string | number | null, and jsonResponse(body: unknown) accepts it — no cast needed, no any.
• No stale references to the three removed hooks outside the intentional historical mentions (useInitialConfig.ts:22-24, test header :5-6).
• I could not run the suite here (clients/web/node_modules absent on this runner). This commit is comment- and test-title-only over a previously-green tree, so the ≥90 gate should be unaffected — worth confirming CI is green on 54d4e412 before merge.

Nit 3 (loading exposed but unconsumed) — still agree it's out of scope for a pure consolidation.
· branch v2/fix-1643-consolidate-config-hooks

[cliffhall]

Both addressed in the latest commit:

%p → %j. You're right — %p is Jest-only; vitest's formatRegExp doesn't include it, so all three cases were titled literally %p. Switched to %j, and verified the rendered titles are now distinct: writable=null, writable=0, writable="no" (the %j quoting also separates the string case from the number, as you noted).

Class docblock consistency. Took the optional suggestion too — the class-level docblock now says a response is dropped after unmount or a re-fetch, so all three comments (docblock, inline guard, test) tell the same story.

@claude review

[claude]

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

---

Re-review: a94e186c

• Read the follow-up commit diff
• Verify %j renders distinct titles (empirically, in a real vitest 4.1.10 run)
• Verify the docblock edit against the effect wiring
• Re-check stale references / CI
• Post review

Verdict: LGTM — ship it. Both follow-ups are correct, and I verified the %j fix by running it rather than by reading the formatter. No remaining issues, blocking or otherwise.

---

%p → %j — verified empirically ✅

Rather than re-reason from formatRegExp, I installed vitest@4.1.10 standalone (clients/web/node_modules is absent on this runner) and ran the old and new titles side by side against the same [null, 0, "no"] matrix:

✓ keeps writable true for writable=null (falsy but not false)
✓ keeps writable true for writable=0 (falsy but not false)
✓ keeps writable true for writable="no" (falsy but not false)
✓ OLD keeps writable true for writable=%p (falsy but not false)
✓ OLD keeps writable true for writable=%p (falsy but not false)
✓ OLD keeps writable true for writable=%p (falsy but not false)

Three distinct titles now, where %p produced three identical ones. The reported "no" vs 0 distinction lands too — %j routes through JSON.stringify (@vitest/utils display.js:719-720), so the string case is quoted and can't be confused with the number.

I also confirmed the value still arrives at the handler unchanged. @vitest/runner's each computes arrayOnlyCases = cases.every(Array.isArray) (chunk-artifact.js:2018) — false for [null, 0, "no"] — so it calls handler(i) with the raw case value, not handler(...items). async (value) therefore receives null / 0 / "no" directly, and the items = [i] wrapping affects only the title. %j consumes exactly one arg and formatTitle emits no leftovers, so nothing is appended past (falsy but not false).

Docblock consistency — correct ✅

core/react/useInitialConfig.ts:26-28 now reads "A response that resolves after unmount or a re-fetch is dropped rather than overwriting current state." That's accurate on both halves, and I re-confirmed the re-fetch half against the wiring: load's deps are [base, authToken, doFetch] (:110), the effect's are [load] (:119), so a baseUrl/authToken change runs the cleanup (cancelled = true, :117) before the re-run — the prior call's continuation then bails at :92, :94, or :107.

All three comments now tell one story: docblock (:26-28), inline guard rationale (:79-82), and the test's coverage disclaimer (useInitialConfig.test.tsx:215-218). The remaining "so no setState runs on the dead component" at :192-194 is a description of what the guards do, not a claim about React warning behavior, so it's consistent — no further wording owed.

Everything else re-verified

• Coverage unaffected. The body.writable !== false branch (:102) keeps the true leg ×3 and the false leg from :28/:136/:169; this commit changed only a title string and a comment.
• Types clean. it.each([null, 0, "no"]) infers string | number | null, consumed by jsonResponse(body: unknown) — no cast, no any.
• No stale references. grep over *.{ts,tsx,md,json} for the three removed hook names returns only the two intentional historical mentions (useInitialConfig.ts:22-23, test header :5-6).
• CI. 884bb7de is green; the runs for 54d4e412 and a94e186c were still in progress when I checked (run 30136126515). Both of those commits are comment- and title-only over that green tree, so the ≥90 gate should hold — glance at the run before merge.

Nit 3 (loading exposed but unconsumed) — still the right call to defer.

Reminder for merge: per AGENTS.md, Closes #1643 won't auto-close on a v2/main PR — close #1643 manually and move its board card to Done (248a3910).

--- · branch v2/fix-1643-consolidate-config-hooks