modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 3

refactor(web): consolidate the three GET /api/config hooks into one (#1770)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Details
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Details
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
Details

Browse source 
* refactor(web): consolidate the three GET /api/config hooks into one (#1643)

useSandboxUrl, useServerListWritable, and useInspectorVersion each fetched
the same static /api/config payload separately and read one field from it —
three requests on load for one payload. Replace them with a single
useInitialConfig hook that fetches once and exposes { version, sandboxUrl,
writable, loading } with the same tolerant parsing/defaults (writable
defaults true; version/sandboxUrl undefined when absent; unmount-safe).

- Add core/react/useInitialConfig.ts; wire App.tsx to it.
- Remove the three former single-field hooks and their tests.
- Fold the fetch-branch matrix into one useInitialConfig test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* test(web): address review nits on useInitialConfig tests (#1643)

- "keeps writable true for any non-false value" now sends real falsy-but-
  not-false values a nonconforming backend could actually emit (null / 0 /
  "no") instead of `writable: undefined`, which was indistinguishable from an
  absent key and duplicated the omitted-field test.
- Soften the post-unmount comment to say the test exercises the isCancelled()
  guard branches for coverage rather than implying it can detect their removal
  (React 18 dropped the setState-after-unmount warning).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* docs(web): fix contradicting unmount-guard comment; it.each writable cases (#1643)

Second-review follow-ups:
- core/react/useInitialConfig.ts: correct the isCancelled() rationale — React
  18 no longer warns on setState-after-unmount; the guard exists to stop a
  stale payload (after unmount or a baseUrl/authToken re-run) overwriting
  current state. The old wording contradicted the corrected test comment.
- useInitialConfig.test.tsx: convert the writable-non-false loop to it.each so
  each value (null / 0 / "no") reports independently.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* test(web): use vitest %j specifier in it.each title; tidy hook docblock (#1643)

Third-review follow-ups (both title/comment-only):
- useInitialConfig.test.tsx: `%p` is a Jest specifier vitest doesn't support,
  so all three cases rendered the literal `%p`. Switch to `%j` — titles now
  render null / 0 / "no" distinctly (verified: `writable=null`, `writable=0`,
  `writable="no"`).
- useInitialConfig.ts: extend the class docblock to say a response is dropped
  after unmount OR a re-fetch, matching the corrected inline guard comment.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Cliff Hall 2026-07-24 22:12:19 -04:00 • committed by GitHub
commit 8aba3122c5
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 230 additions and 662 deletions
Download patch file Download diff file Expand all files Collapse all files

252
clients/web/src/test/core/react/useInitialConfig.test.tsx Normal file
View file

@ -0,0 +1,252 @@
/**
* Tests for useInitialConfig — runs in happy-dom under the unit project. Uses a
* controlled fake `fetch` so each branch (present / absent / non-string / HTTP
* error / network throw / post-unmount guards) and the auth header are asserted
* directly. This is the single hook that replaced useSandboxUrl /
* useServerListWritable / useInspectorVersion (#1643), so it covers each field's
* branch matrix in one place.
*/
import { describe, it, expect, vi } from "vitest";
import { renderHook, waitFor } from "@testing-library/react";
import { useInitialConfig } from "@inspector/core/react/useInitialConfig";
function jsonResponse(body: unknown, ok = true): Response {
return {
ok,
status: ok ? 200 : 401,
json: async () => body,
} as unknown as Response;
}
describe("useInitialConfig", () => {
it("starts loading, then resolves all three fields from one config payload", async () => {
const fetchFn = vi.fn().mockResolvedValue(
jsonResponse({
version: "2.0.0",
sandboxUrl: "http://localhost:6299/sandbox",
writable: false,
}),
);
const { result } = renderHook(() =>
useInitialConfig({ baseUrl: "http://test.local", fetchFn }),
);
// Initial (pre-fetch) state: version/sandboxUrl undefined, writable defaults
// true, loading true.
expect(result.current.loading).toBe(true);
expect(result.current.version).toBeUndefined();
expect(result.current.sandboxUrl).toBeUndefined();
expect(result.current.writable).toBe(true);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.version).toBe("2.0.0");
expect(result.current.sandboxUrl).toBe("http://localhost:6299/sandbox");
expect(result.current.writable).toBe(false);
// One static payload, one request.
expect(fetchFn).toHaveBeenCalledTimes(1);
});
it("sends the bearer auth header when a token is provided", async () => {
const fetchFn = vi.fn().mockResolvedValue(jsonResponse({}));
renderHook(() =>
useInitialConfig({
baseUrl: "http://test.local/",
authToken: "secret-token",
fetchFn,
}),
);
await waitFor(() => expect(fetchFn).toHaveBeenCalled());
const [url, init] = fetchFn.mock.calls[0];
// Trailing slash on baseUrl is normalized away.
expect(url).toBe("http://test.local/api/config");
expect(init.method).toBe("GET");
expect(init.headers["x-mcp-remote-auth"]).toBe("Bearer secret-token");
});
it("omits the auth header when no token is provided", async () => {
const fetchFn = vi.fn().mockResolvedValue(jsonResponse({}));
renderHook(() =>
useInitialConfig({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(fetchFn).toHaveBeenCalled());
const [, init] = fetchFn.mock.calls[0];
expect(init.headers["x-mcp-remote-auth"]).toBeUndefined();
});
it("applies each field's default when the payload omits it", async () => {
const fetchFn = vi
.fn()
.mockResolvedValue(jsonResponse({ defaultEnvironment: {} }));
const { result } = renderHook(() =>
useInitialConfig({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.version).toBeUndefined();
expect(result.current.sandboxUrl).toBeUndefined();
// Missing writable (legacy backend) stays writable.
expect(result.current.writable).toBe(true);
});
it("leaves version/sandboxUrl undefined when the fields are not usable strings", async () => {
const fetchFn = vi
.fn()
.mockResolvedValue(jsonResponse({ version: "", sandboxUrl: "" }));
const { result } = renderHook(() =>
useInitialConfig({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.version).toBeUndefined();
expect(result.current.sandboxUrl).toBeUndefined();
});
// Only an explicit `false` flips the list read-only. A nonconforming backend
// could send a falsy-but-not-false value (null / 0 / a string); each is
// `!== false`, so each must leave the list writable.
it.each([null, 0, "no"])(
"keeps writable true for writable=%j (falsy but not false)",
async (value) => {
const fetchFn = vi
.fn()
.mockResolvedValue(jsonResponse({ writable: value }));
const { result } = renderHook(() =>
useInitialConfig({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.writable).toBe(true);
},
);
it("applies defaults on a non-ok response", async () => {
const fetchFn = vi
.fn()
.mockResolvedValue(
jsonResponse(
{ version: "9.9.9", sandboxUrl: "http://x/sb", writable: false },
false,
),
);
const { result } = renderHook(() =>
useInitialConfig({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.version).toBeUndefined();
expect(result.current.sandboxUrl).toBeUndefined();
expect(result.current.writable).toBe(true);
});
it("applies defaults when the fetch throws", async () => {
const fetchFn = vi.fn().mockRejectedValue(new Error("network down"));
const { result } = renderHook(() =>
useInitialConfig({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.version).toBeUndefined();
expect(result.current.sandboxUrl).toBeUndefined();
expect(result.current.writable).toBe(true);
});
it("falls back to globalThis.fetch when no fetchFn is provided", async () => {
const globalFetch = vi.fn().mockResolvedValue(
jsonResponse({
version: "3.1.4",
sandboxUrl: "http://global/sb",
writable: false,
}),
);
const original = globalThis.fetch;
globalThis.fetch = globalFetch as unknown as typeof fetch;
try {
const { result } = renderHook(() =>
useInitialConfig({ baseUrl: "http://test.local" }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(globalFetch).toHaveBeenCalledWith(
"http://test.local/api/config",
expect.objectContaining({ method: "GET" }),
);
expect(result.current.version).toBe("3.1.4");
expect(result.current.sandboxUrl).toBe("http://global/sb");
expect(result.current.writable).toBe(false);
} finally {
globalThis.fetch = original;
}
});
it("drops a response that resolves after unmount (no state update)", async () => {
// Gate the fetch so it is still in flight when we unmount; the
// isCancelled() guards (after fetch, after json, and in finally) must all
// short-circuit so no setState runs on the dead component.
let resolveFetch: ((r: Response) => void) | undefined;
const fetchFn = vi.fn().mockReturnValue(
new Promise<Response>((r) => {
resolveFetch = r;
}),
);
const { result, unmount } = renderHook(() =>
useInitialConfig({ baseUrl: "http://test.local", fetchFn }),
);
expect(result.current.loading).toBe(true);
unmount();
// Resolve after unmount — the post-fetch isCancelled() guard returns early.
resolveFetch?.(
jsonResponse({ version: "2.0.0", sandboxUrl: "http://late/sb" }),
);
// Let the microtask queue drain so the continuation runs.
await Promise.resolve();
await Promise.resolve();
// This test exists to exercise the post-unmount `isCancelled()` guard
// branches; it can't detect their removal (React 18 dropped the
// setState-after-unmount warning, and `result.current` is frozen at the last
// render), so it only asserts the fields stayed at their initial values.
expect(result.current.version).toBeUndefined();
expect(result.current.sandboxUrl).toBeUndefined();
expect(result.current.writable).toBe(true);
});
it("drops a response whose json resolves after unmount", async () => {
// Fetch resolves before unmount but the json() body resolves after, so the
// second isCancelled() guard (post-json) is the one that short-circuits.
let resolveJson: ((v: unknown) => void) | undefined;
const res = {
ok: true,
status: 200,
json: () =>
new Promise((r) => {
resolveJson = r;
}),
} as unknown as Response;
const fetchFn = vi.fn().mockResolvedValue(res);
const { result, unmount } = renderHook(() =>
useInitialConfig({ baseUrl: "http://test.local", fetchFn }),
);
// Let the fetch resolve so we're parked awaiting json().
await waitFor(() => expect(fetchFn).toHaveBeenCalled());
await Promise.resolve();
unmount();
resolveJson?.({ version: "2.0.0", writable: false });
await Promise.resolve();
await Promise.resolve();
expect(result.current.version).toBeUndefined();
expect(result.current.writable).toBe(true);
});
});

181
clients/web/src/test/core/react/useInspectorVersion.test.tsx
View file

@ -1,181 +0,0 @@
/**
* Tests for useInspectorVersion — runs in happy-dom under the unit project. Uses
* a controlled fake `fetch` so each branch (present / absent / non-string / HTTP
* error / network throw / post-unmount guards) and the auth header are asserted
* directly. Mirrors useSandboxUrl's test (same `/api/config` fetch shape).
*/
import { describe, it, expect, vi } from "vitest";
import { renderHook, waitFor } from "@testing-library/react";
import { useInspectorVersion } from "@inspector/core/react/useInspectorVersion";
function jsonResponse(body: unknown, ok = true): Response {
return {
ok,
status: ok ? 200 : 401,
json: async () => body,
} as unknown as Response;
}
describe("useInspectorVersion", () => {
it("starts loading, then resolves the version from the config payload", async () => {
const fetchFn = vi
.fn()
.mockResolvedValue(jsonResponse({ version: "2.0.0" }));
const { result } = renderHook(() =>
useInspectorVersion({ baseUrl: "http://test.local", fetchFn }),
);
expect(result.current.loading).toBe(true);
expect(result.current.version).toBeUndefined();
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.version).toBe("2.0.0");
});
it("sends the bearer auth header when a token is provided", async () => {
const fetchFn = vi.fn().mockResolvedValue(jsonResponse({}));
renderHook(() =>
useInspectorVersion({
baseUrl: "http://test.local/",
authToken: "secret-token",
fetchFn,
}),
);
await waitFor(() => expect(fetchFn).toHaveBeenCalled());
const [url, init] = fetchFn.mock.calls[0];
// Trailing slash on baseUrl is normalized away.
expect(url).toBe("http://test.local/api/config");
expect(init.method).toBe("GET");
expect(init.headers["x-mcp-remote-auth"]).toBe("Bearer secret-token");
});
it("omits the auth header when no token is provided", async () => {
const fetchFn = vi.fn().mockResolvedValue(jsonResponse({}));
renderHook(() =>
useInspectorVersion({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(fetchFn).toHaveBeenCalled());
const [, init] = fetchFn.mock.calls[0];
expect(init.headers["x-mcp-remote-auth"]).toBeUndefined();
});
it("leaves version undefined when the payload omits it", async () => {
const fetchFn = vi
.fn()
.mockResolvedValue(jsonResponse({ defaultEnvironment: {} }));
const { result } = renderHook(() =>
useInspectorVersion({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.version).toBeUndefined();
});
it("leaves version undefined when the field is not a usable string", async () => {
const fetchFn = vi.fn().mockResolvedValue(jsonResponse({ version: "" }));
const { result } = renderHook(() =>
useInspectorVersion({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.version).toBeUndefined();
});
it("leaves version undefined on a non-ok response", async () => {
const fetchFn = vi
.fn()
.mockResolvedValue(jsonResponse({ version: "9.9.9" }, false));
const { result } = renderHook(() =>
useInspectorVersion({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.version).toBeUndefined();
});
it("leaves version undefined when the fetch throws", async () => {
const fetchFn = vi.fn().mockRejectedValue(new Error("network down"));
const { result } = renderHook(() =>
useInspectorVersion({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.version).toBeUndefined();
});
it("falls back to globalThis.fetch when no fetchFn is provided", async () => {
const globalFetch = vi
.fn()
.mockResolvedValue(jsonResponse({ version: "3.1.4" }));
const original = globalThis.fetch;
globalThis.fetch = globalFetch as unknown as typeof fetch;
try {
const { result } = renderHook(() =>
useInspectorVersion({ baseUrl: "http://test.local" }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(globalFetch).toHaveBeenCalledWith(
"http://test.local/api/config",
expect.objectContaining({ method: "GET" }),
);
expect(result.current.version).toBe("3.1.4");
} finally {
globalThis.fetch = original;
}
});
it("drops a response that resolves after unmount (no state update)", async () => {
let resolveFetch: ((r: Response) => void) | undefined;
const fetchFn = vi.fn().mockReturnValue(
new Promise<Response>((r) => {
resolveFetch = r;
}),
);
const { result, unmount } = renderHook(() =>
useInspectorVersion({ baseUrl: "http://test.local", fetchFn }),
);
expect(result.current.loading).toBe(true);
unmount();
resolveFetch?.(jsonResponse({ version: "2.0.0" }));
await Promise.resolve();
await Promise.resolve();
expect(result.current.version).toBeUndefined();
});
it("drops a response whose json resolves after unmount", async () => {
let resolveJson: ((v: unknown) => void) | undefined;
const res = {
ok: true,
status: 200,
json: () =>
new Promise((r) => {
resolveJson = r;
}),
} as unknown as Response;
const fetchFn = vi.fn().mockResolvedValue(res);
const { result, unmount } = renderHook(() =>
useInspectorVersion({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(fetchFn).toHaveBeenCalled());
await Promise.resolve();
unmount();
resolveJson?.({ version: "2.0.0" });
await Promise.resolve();
await Promise.resolve();
expect(result.current.version).toBeUndefined();
});
});

177
clients/web/src/test/core/react/useServerListWritable.test.tsx
View file

@ -1,177 +0,0 @@
/**
* Tests for useServerListWritable — runs in happy-dom under the unit project.
* Uses a controlled fake `fetch` so each branch (writable / read-only / absent
* field / HTTP error / network throw) and the auth header are asserted directly.
*/
import { describe, it, expect, vi } from "vitest";
import { renderHook, waitFor } from "@testing-library/react";
import { useServerListWritable } from "@inspector/core/react/useServerListWritable";
function jsonResponse(body: unknown, ok = true): Response {
return {
ok,
status: ok ? 200 : 401,
json: async () => body,
} as unknown as Response;
}
describe("useServerListWritable", () => {
it("defaults to writable while loading, then keeps true for a writable session", async () => {
const fetchFn = vi.fn().mockResolvedValue(jsonResponse({ writable: true }));
const { result } = renderHook(() =>
useServerListWritable({ baseUrl: "http://test.local", fetchFn }),
);
expect(result.current.loading).toBe(true);
expect(result.current.writable).toBe(true);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.writable).toBe(true);
});
it("resolves writable:false for a read-only session", async () => {
const fetchFn = vi
.fn()
.mockResolvedValue(jsonResponse({ writable: false }));
const { result } = renderHook(() =>
useServerListWritable({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.writable).toBe(false);
});
it("stays writable when the field is absent (legacy backend)", async () => {
const fetchFn = vi.fn().mockResolvedValue(jsonResponse({}));
const { result } = renderHook(() =>
useServerListWritable({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.writable).toBe(true);
});
it("sends the bearer auth header when a token is provided", async () => {
const fetchFn = vi
.fn()
.mockResolvedValue(jsonResponse({ writable: false }));
renderHook(() =>
useServerListWritable({
baseUrl: "http://test.local/",
authToken: "secret-token",
fetchFn,
}),
);
await waitFor(() => expect(fetchFn).toHaveBeenCalled());
const [url, init] = fetchFn.mock.calls[0] as [string, RequestInit];
// Trailing slash on baseUrl is trimmed.
expect(url).toBe("http://test.local/api/config");
expect((init.headers as Record<string, string>)["x-mcp-remote-auth"]).toBe(
"Bearer secret-token",
);
});
it("stays writable on an HTTP error", async () => {
const fetchFn = vi
.fn()
.mockResolvedValue(jsonResponse({ writable: false }, false));
const { result } = renderHook(() =>
useServerListWritable({ baseUrl: "http://test.local", fetchFn }),
);
// An !ok response is ignored (the body is never read), so the writable
// default survives even though loading still settles via `finally`.
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.writable).toBe(true);
});
it("stays writable when fetch throws", async () => {
const fetchFn = vi.fn().mockRejectedValue(new Error("network down"));
const { result } = renderHook(() =>
useServerListWritable({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.writable).toBe(true);
});
it("falls back to globalThis.fetch when no fetchFn is provided", async () => {
const globalFetch = vi
.fn()
.mockResolvedValue(jsonResponse({ writable: false }));
const original = globalThis.fetch;
globalThis.fetch = globalFetch as unknown as typeof fetch;
try {
const { result } = renderHook(() =>
useServerListWritable({ baseUrl: "http://test.local" }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(globalFetch).toHaveBeenCalledWith(
"http://test.local/api/config",
expect.objectContaining({ method: "GET" }),
);
expect(result.current.writable).toBe(false);
} finally {
globalThis.fetch = original;
}
});
it("drops a response that resolves after unmount (no state update)", async () => {
// Gate the fetch so it is still in flight at unmount; the isCancelled()
// guards (after fetch and in finally) short-circuit so no setState runs on
// the dead component and `loading` is never flipped.
let resolveFetch: ((r: Response) => void) | undefined;
const fetchFn = vi.fn().mockReturnValue(
new Promise<Response>((r) => {
resolveFetch = r;
}),
);
const { result, unmount } = renderHook(() =>
useServerListWritable({ baseUrl: "http://test.local", fetchFn }),
);
expect(result.current.loading).toBe(true);
unmount();
resolveFetch?.(jsonResponse({ writable: false }));
await Promise.resolve();
await Promise.resolve();
// Guards held: the post-unmount value stayed at the writable default.
expect(result.current.writable).toBe(true);
});
it("drops a response whose json resolves after unmount", async () => {
// Fetch resolves before unmount but json() resolves after, exercising the
// second isCancelled() guard (post-json).
let resolveJson: ((v: unknown) => void) | undefined;
const res = {
ok: true,
status: 200,
json: () =>
new Promise((r) => {
resolveJson = r;
}),
} as unknown as Response;
const fetchFn = vi.fn().mockResolvedValue(res);
const { result, unmount } = renderHook(() =>
useServerListWritable({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(fetchFn).toHaveBeenCalled());
await Promise.resolve();
unmount();
resolveJson?.({ writable: false });
await Promise.resolve();
await Promise.resolve();
expect(result.current.writable).toBe(true);
});
});

2
clients/web/src/test/setup.ts
View file

@ -34,7 +34,7 @@ Object.defineProperty(window, "localStorage", {
});
// Benign default `fetch`. Several components hit the backend on mount — e.g.
// the app reads `GET /api/config` via `useSandboxUrl` / `useServerListWritable`.
// the app reads `GET /api/config` via `useInitialConfig`.
// Under happy-dom (no server) those real requests 404 and log alarming
// `GET .../api/config 404 (Not Found)` lines that make a green run look broken.
// Returning an empty 200 keeps such *incidental* calls quiet. Tests that care