diff --git a/clients/web/src/test/core/react/useInitialConfig.test.tsx b/clients/web/src/test/core/react/useInitialConfig.test.tsx index 17a1de66..26ea8ae1 100644 --- a/clients/web/src/test/core/react/useInitialConfig.test.tsx +++ b/clients/web/src/test/core/react/useInitialConfig.test.tsx @@ -11,6 +11,18 @@ import { describe, it, expect, vi } from "vitest"; import { act, renderHook, waitFor } from "@testing-library/react"; import { useInitialConfig } from "@inspector/core/react/useInitialConfig"; +function payloadWithPlaintext(plaintext: boolean): Response { + return jsonResponse({ + secretStorage: { + kind: "file", + reason: "fallback", + durable: true, + plaintext, + path: "/tmp/secrets.json", + }, + }); +} + function jsonResponse(body: unknown, ok = true): Response { return { ok, @@ -506,6 +518,46 @@ describe("useInitialConfig", () => { expect(fetchFn).toHaveBeenCalledTimes(2); }); + it("commits only the newest load when refreshes overlap", async () => { + // Two debounced settings saves landing close together fire two + // refreshes, and nothing orders the responses. Before the request + // token, an earlier request resolving last put the footer back to the + // descriptor it had *before* the write — reverting a security + // statement to a stale value, which is the worst direction for this + // particular field to be wrong in. + const resolvers: Array<(v: Response) => void> = []; + const fetchFn = vi + .fn() + .mockImplementation( + () => new Promise((resolve) => resolvers.push(resolve)), + ); + + const { result } = renderHook(() => + useInitialConfig({ baseUrl: "http://test.local", fetchFn }), + ); + // Mount fetch is resolvers[0]; settle it so `loading` clears. + await act(async () => { + resolvers[0]?.(payloadWithPlaintext(true)); + }); + await waitFor(() => expect(result.current.loading).toBe(false)); + + // Two overlapping refreshes: the first reports the pre-write state, + // the second the post-write state. + act(() => result.current.refresh()); // resolvers[1] — older + act(() => result.current.refresh()); // resolvers[2] — newer + + // Resolve the NEWER one first, then the older one. + await act(async () => { + resolvers[2]?.(payloadWithPlaintext(false)); + }); + await act(async () => { + resolvers[1]?.(payloadWithPlaintext(true)); + }); + + // The late-arriving older response must not win. + expect(result.current.secretStorage?.plaintext).toBe(false); + }); + it("drops a refresh that resolves after unmount", async () => { const fetchFn = vi.fn().mockResolvedValue(jsonResponse({})); const { result, unmount } = renderHook(() => diff --git a/core/react/useInitialConfig.ts b/core/react/useInitialConfig.ts index cbc5ddef..5594bd88 100644 --- a/core/react/useInitialConfig.ts +++ b/core/react/useInitialConfig.ts @@ -67,8 +67,10 @@ export interface UseInitialConfigResult { * about a file that is no longer plaintext. The backend already re-derives * the descriptor per request; this is the client half of that. * - * Safe to call at any time: the GET is idempotent, and a response that - * arrives after unmount is dropped. + * Safe to call at any time and safe to overlap: the GET is idempotent, and + * loads are ordered by a request token, so only the most recent one may + * commit — a response that arrives after unmount, after a re-fetch, or out + * of order behind a newer refresh is dropped. */ refresh: () => void; } @@ -179,63 +181,65 @@ export function useInitialConfig( >(undefined); const [loading, setLoading] = useState(true); - // `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. - const load = useCallback( - async (isCancelled: () => boolean): Promise => { - const headers: Record = {}; - if (authToken) headers["x-mcp-remote-auth"] = `Bearer ${authToken}`; - try { - const res = await doFetch(`${base}/api/config`, { - method: "GET", - headers, - }); - if (isCancelled() || !res.ok) return; - const body = (await res.json()) as ConfigPayload; - if (isCancelled()) return; - // Tolerate missing/non-usable fields — a legacy backend that omits any - // of them leaves that value at its "unavailable" default rather than - // showing a bogus value. - setVersion(usableString(body.version)); - setSandboxUrl(usableString(body.sandboxUrl)); - // Only an explicit `false` makes the list read-only; a missing field - // (legacy backend) stays writable. - setWritable(body.writable !== false); - setSecretStorage(usableSecretStorage(body.secretStorage)); - } catch { - // Network error / aborted fetch: leave every field at its default - // (version/sandboxUrl undefined, writable true). - } finally { - if (!isCancelled()) setLoading(false); - } - }, - [base, authToken, doFetch], - ); + /** + * Monotonic request token. Every load claims the next value; a load may + * commit only while it still holds the current one. + * + * This replaces a per-caller "am I cancelled" flag, which was enough for + * the mount fetch and *not* enough once `refresh()` existed. Two overlapping + * refreshes — two debounced settings saves landing close together — are + * unordered, so the earlier request could resolve last and put the footer + * back to the descriptor it had before the write. Stale-response ordering + * matters more here than in most places precisely because the field it + * would revert is a security statement. + * + * Bumping on effect teardown covers the other two cases for free: unmount, + * and a `baseUrl`/`authToken` change that re-runs the effect. + */ + const generation = useRef(0); + + const load = useCallback(async (): Promise => { + const mine = ++generation.current; + const stale = () => generation.current !== mine; + const headers: Record = {}; + if (authToken) headers["x-mcp-remote-auth"] = `Bearer ${authToken}`; + try { + const res = await doFetch(`${base}/api/config`, { + method: "GET", + headers, + }); + if (stale() || !res.ok) return; + const body = (await res.json()) as ConfigPayload; + if (stale()) return; + // Tolerate missing/non-usable fields — a legacy backend that omits any + // of them leaves that value at its "unavailable" default rather than + // showing a bogus value. + setVersion(usableString(body.version)); + setSandboxUrl(usableString(body.sandboxUrl)); + // Only an explicit `false` makes the list read-only; a missing field + // (legacy backend) stays writable. + setWritable(body.writable !== false); + setSecretStorage(usableSecretStorage(body.secretStorage)); + } catch { + // Network error / aborted fetch: leave every field at its default + // (version/sandboxUrl undefined, writable true). + } finally { + if (!stale()) setLoading(false); + } + }, [base, authToken, doFetch]); useEffect(() => { - let cancelled = false; - void load(() => cancelled); + void load(); return () => { - cancelled = true; + // Invalidate whatever is in flight: this fires on unmount and on a + // `baseUrl`/`authToken` change, and in both cases a response for the + // old inputs must not commit. + generation.current++; }; }, [load]); - // A manual re-fetch shares `load`'s cancellation contract, but has no - // effect teardown to key off — so it tracks its own liveness through a ref - // that the unmount effect flips. Without that, a refresh fired just before - // unmount would still call `setState` on a dead component. - const alive = useRef(true); - useEffect(() => { - alive.current = true; - return () => { - alive.current = false; - }; - }, []); - const refresh = useCallback(() => { - void load(() => !alive.current); + void load(); }, [load]); return { version, sandboxUrl, writable, secretStorage, loading, refresh };