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

Merge branch 'v2/main' into v2/chore/2163-agent-skills

Browse source 
Three conflicts, all in the files this PR refactored. The code from #1955 and
#2161 (core/react's useSyncExternalStore conversion) merged cleanly.

- `.github/copilot-instructions.md` — modify/delete. This PR deletes it, and
  the four lines v2/main added there are a distillation of AGENTS.md rules that
  arrive in AGENTS.md anyway. Kept deleted.
- `AGENTS.md` — the incoming change expanded the `core/react/` entry in the
  project tree by eleven lines; this PR compacted that tree to one line per
  entry on purpose. Took the compact tree and carried the substance onto its
  one line. The other incoming hunk — the store-subscription rules under React
  instructions — merged cleanly, which is the split working: it is a RULE, so
  it lands in the section that stayed.
- `README.md` — same shape, one tree line. This README no longer enumerates
  `core/`'s subdirectories at all, so the substance went where that content now
  lives: `docs/architecture.md` and the `project-structure` skill's core table.

Nothing from v2/main was dropped; every incoming statement is now in exactly one
place rather than the three it used to occupy.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-29 23:06:01 -04:00
commit 67abdf96b4
4 changed files with 1126 additions and 803 deletions
Download patch file Download diff file Expand all files Collapse all files

75
clients/web/src/App.test.tsx
View file

@ -44,12 +44,21 @@ const { messageLogClear } = vi.hoisted(() => ({ messageLogClear: vi.fn() }));
// trigger the handlers). See #1368.
// --- Fake InspectorClient ---------------------------------------------------
// Extends EventTarget so the App's `addEventListener("disconnect", …)` wiring
// is real; the test fires `dispatchEvent(new Event("disconnect"))` to simulate
// any of the three disconnect paths (toggle, header button, transport failure).
// Extends the real TypedEventTarget so the App's `addEventListener("disconnect",
// …)` wiring is real; the test fires `dispatchEvent(new Event("disconnect"))` to
// simulate any of the three disconnect paths (toggle, header button, transport
// failure).
//
// The real base rather than a bare EventTarget because the hooks this fake is
// handed to read state through `useStoreSnapshot`, which caches its snapshot
// against `getEventRevision` — a method the base owns and a bare EventTarget
// does not have (#1955). Inheriting it also means the counter is advanced by
// the `dispatchEvent` calls below, exactly as it would be in production.
vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => {
const actual =
await importOriginal<typeof import("@inspector/core/mcp/index.js")>();
const { TypedEventTarget } =
await import("@inspector/core/mcp/typedEventTarget.js");
// Each armed value makes one `connect()` reject, in FIFO order, so a test can
// exercise the handshake-failure path — or a two-connect sequence such as the
// auth-recovery retry, where the first call rejects with the recovery error
@ -68,7 +77,9 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => {
// control-flow decision from its resolving `false` (#2108): a throw is
// surfaced as a failed attempt rather than falling through to the redirect.
let nextChallengeCheckRejection: unknown = null;
class FakeInspectorClient extends EventTarget {
class FakeInspectorClient extends TypedEventTarget<
import("@inspector/core/mcp/index.js").InspectorClientEventMap
> {
connect = vi.fn(() => {
if (connectRejections.length > 0) {
return Promise.reject(connectRejections.shift());
@ -695,6 +706,50 @@ const clientInstances = (
McpIndex as unknown as { __clientInstances: EventTarget[] }
).__clientInstances;
/**
* Mirror how the real client enqueues a server-initiated request: put it on the
* queue, then announce the change.
*
* Both halves matter now that `usePendingClientRequests` reads the queue back
* off the client (`useStoreSnapshot`, #1955) rather than taking the event's
* `detail`. The store is the source of truth and the event is only the signal
* that it moved, so dispatching alone would announce a change that isn't there
* — and the modal under test would never open.
*/
function enqueuePendingRequests(
client: EventTarget,
queue: "getPendingSamples" | "getPendingElicitations",
event: "pendingSamplesChange" | "pendingElicitationsChange",
entries: readonly unknown[],
): void {
const withQueue = client as EventTarget &
Record<typeof queue, ReturnType<typeof vi.fn>>;
withQueue[queue].mockReturnValue(entries);
client.dispatchEvent(new CustomEvent(event, { detail: entries }));
}
const enqueuePendingSamples = (
client: EventTarget,
samples: readonly unknown[],
): void =>
enqueuePendingRequests(
client,
"getPendingSamples",
"pendingSamplesChange",
samples,
);
const enqueuePendingElicitations = (
client: EventTarget,
elicitations: readonly unknown[],
): void =>
enqueuePendingRequests(
client,
"getPendingElicitations",
"pendingElicitationsChange",
elicitations,
);
// The mock factory adds four test-only arming hooks to the module namespace.
// Intersecting with `typeof McpIndex` keeps the real module's shape checked and
// narrows this to a single cast — `as unknown as` would discard the former and
@ -1417,9 +1472,7 @@ describe("App pending server-initiated request modal", () => {
reject: vi.fn(),
};
act(() => {
clientInstances[0].dispatchEvent(
new CustomEvent("pendingSamplesChange", { detail: [sample] }),
);
enqueuePendingSamples(clientInstances[0], [sample]);
});
await waitFor(() =>
@ -1433,9 +1486,7 @@ describe("App pending server-initiated request modal", () => {
// The client clearing its queue (empty event) closes the modal.
act(() => {
clientInstances[0].dispatchEvent(
new CustomEvent("pendingSamplesChange", { detail: [] }),
);
enqueuePendingSamples(clientInstances[0], []);
});
await waitFor(() =>
expect(screen.queryByText("Sampling Request")).not.toBeInTheDocument(),
@ -1581,9 +1632,7 @@ describe("App task wiring", () => {
respond,
};
act(() => {
clientInstances[0].dispatchEvent(
new CustomEvent("pendingElicitationsChange", { detail: [elicitation] }),
);
enqueuePendingElicitations(clientInstances[0], [elicitation]);
});
await waitFor(() =>

42
clients/web/src/hooks/useConnectionLifecycle.ts
View file

@ -245,18 +245,26 @@ export function useConnectionLifecycle({
sessionReset,
seedModernLogLevel,
}: UseConnectionLifecycleOptions): ConnectionLifecycle {
const [connectedServerId, setConnectedServerId] = useState<
string | undefined
>(undefined);
// Track the just-connected server so its card gets the green highlight +
// scroll-into-view (#1682). It is a pure function of the two values it reads,
// so it is derived rather than mirrored into state by an effect: a state copy
// could only ever lag the status it copies by one committed render, which is
// a frame of the wrong highlight for no gain.
const connectedServerId =
connectionStatus === "connected" ? activeServerId : undefined;
// `setupClientForServer` is synchronous and memoized, so a caller that
// awaited the config would still resume with the `sandboxUrl` captured by the
// render it STARTED in — undefined, on the very load this matters for. The
// ref is written every render, so client construction reads the current value
// whichever entry point (connect, deep link, OAuth callback) reached it.
// ref carries the latest value instead, so client construction reads the
// current one whichever entry point (connect, deep link, OAuth callback)
// reached it. Published from a layout effect rather than written during
// render: every reader is an event handler or a passive effect, both of which
// run after the commit this writes in, so the value they see is unchanged.
const sandboxUrlRef = useRef<string | undefined>(undefined);
// eslint-disable-next-line react-hooks/refs -- pre-existing latest-ref pattern, unmasked when this component dropped below the React Compiler's bail-out (#2161)
sandboxUrlRef.current = sandboxUrl;
useLayoutEffect(() => {
sandboxUrlRef.current = sandboxUrl;
}, [sandboxUrl]);
const {
clearResultPanels,
@ -270,18 +278,6 @@ export function useConnectionLifecycle({
const deepLinkEnsureRef = useRef(false);
const deepLinkUpdateRef = useRef(false);
const deepLinkConnectRef = useRef(false);
// Track the just-connected server so its card gets the green highlight +
// scroll-into-view (#1682). Unlike `failedServerId` (which must survive the
// `disconnect` event a failed connect fires), "connected" is a stable status,
// so a status-driven effect can both set and clear it: set on connect, clear
// whenever the session isn't connected (disconnect, a new attempt's
// "connecting", or an error).
useEffect(() => {
// eslint-disable-next-line react-hooks/set-state-in-effect -- pre-existing status-driven effect, unmasked when this component dropped below the React Compiler's bail-out (#2161)
setConnectedServerId(
connectionStatus === "connected" ? activeServerId : undefined,
);
}, [connectionStatus, activeServerId]);
// Disconnect the previous InspectorClient when it's replaced (server
// switch) or when App unmounts (HMR, tests). Without this the prior
@ -843,7 +839,13 @@ export function useConnectionLifecycle({
const alreadyConnected =
activeServerId === deepLink.serverId && connectionStatus === "connected";
if (!alreadyConnected) {
// eslint-disable-next-line react-hooks/set-state-in-effect -- pre-existing status-driven effect, unmasked when this component dropped below the React Compiler's bail-out (#2161)
// `onToggleConnection` is a connect, not a state sync: it opens a
// transport and drives the whole handshake, so the state it sets along
// the way is a consequence of that I/O rather than something derivable
// from props. An effect is the right tool here, and the one-shot
// `deepLinkConnectRef` guard above is what keeps it from re-firing; the
// rule sees only that an effect called something that reaches setState.
// eslint-disable-next-line react-hooks/set-state-in-effect -- justified directly above
void onToggleConnection(deepLink.serverId).catch((err) => {
// The toast fires from inside `onToggleConnection` for the common
// cases; this catch covers the rest (surfaced on `data-error-message`).

39
clients/web/src/hooks/useMcpApps.ts
View file

@ -1,4 +1,11 @@
import { useCallback, useMemo, useRef, useSyncExternalStore } from "react";
import {
useCallback,
useLayoutEffect,
useMemo,
useRef,
useState,
useSyncExternalStore,
} from "react";
import type { RefObject } from "react";
import { notifications } from "@mantine/notifications";
import type { ElicitResult, Resource } from "@modelcontextprotocol/client";
@ -88,9 +95,14 @@ export function useMcpApps({
// hints (`connect-src 'none'`), exactly as before this wiring existed.
// Walking the whole list to close that would issue the very requests the user
// opted out of by turning pagination on, so the setting wins.
//
// Published from a layout effect rather than written during render: the only
// reader is the async `sandboxready` handler below, which runs long after the
// commit this writes in, so it sees the same listing either way.
const listedResourcesRef = useRef<Resource[]>([]);
// eslint-disable-next-line react-hooks/refs -- pre-existing latest-ref pattern, unmasked when this component dropped below the React Compiler's bail-out (#2161)
listedResourcesRef.current = resources;
useLayoutEffect(() => {
listedResourcesRef.current = resources;
}, [resources]);
// Best-effort by construction: this reads the list as it stands when the app
// opens, and does not distinguish "no entry for this URI" from "the list
// hasn't arrived yet". Both yield no hints, which is the same outcome as
@ -117,9 +129,15 @@ export function useMcpApps({
[configBaseUrl],
);
// `react-hooks/refs` cannot see through `createAppBridgeFactory`, so it warns
// that a ref-reading callback handed to it (`getListedResourceMeta`) *may* be
// called during render. It is not: the factory only stores its deps, and the
// sole call site is inside the async `sandboxready` handler, which runs once
// an app frame has mounted and its sandbox has handshaked. Reading the
// listing lazily there is the whole point — see `listedResourcesRef` above.
const sandboxBridgeFactory = useMemo(
() =>
// eslint-disable-next-line react-hooks/refs -- pre-existing latest-ref pattern, unmasked when this component dropped below the React Compiler's bail-out (#2161)
// eslint-disable-next-line react-hooks/refs -- justified directly above
createAppBridgeFactory({
publishAppDocument: publishDocument,
getClient: () => inspectorClient?.getAppRendererClient() ?? null,
@ -150,9 +168,12 @@ export function useMcpApps({
// handed to every InspectorClient at construction — its `render` is what opts
// this client into advertising the nested MCP Apps `elicitation` capability,
// which is why only the web client (the one with a sandbox) claims it.
const appElicitationControllerRef = useRef<AppElicitationController>(null);
appElicitationControllerRef.current ??= new AppElicitationController();
const appElicitationController = appElicitationControllerRef.current;
// Held as lazily-initialized state rather than a ref seeded during render:
// the instance is never reassigned, so `useState`'s initializer gives the
// same create-once semantics without reading a ref while rendering.
const [appElicitationController] = useState(
() => new AppElicitationController(),
);
// The window onto the controller for the CURRENT client. Closing it when the
// client is replaced both rejects that connection's queued requests and
// refuses any it enqueues during its own (asynchronous) teardown — a late
@ -160,9 +181,7 @@ export function useMcpApps({
// client, i.e. read and answered through a different server.
const appElicitationSessionRef = useRef<AppElicitationSession>(null);
const appElicitations = useSyncExternalStore(
// eslint-disable-next-line react-hooks/refs -- pre-existing latest-ref pattern, unmasked when this component dropped below the React Compiler's bail-out (#2161)
appElicitationController.subscribe,
// eslint-disable-next-line react-hooks/refs -- pre-existing latest-ref pattern, unmasked when this component dropped below the React Compiler's bail-out (#2161)
appElicitationController.getEntries,
);
@ -171,7 +190,7 @@ export function useMcpApps({
// an elicitation, so telling those apps otherwise would be a false claim.
const elicitationBridgeFactory = useMemo(
() =>
// eslint-disable-next-line react-hooks/refs -- pre-existing latest-ref pattern, unmasked when this component dropped below the React Compiler's bail-out (#2161)
// eslint-disable-next-line react-hooks/refs -- same dep, same reason as `sandboxBridgeFactory` above
createAppBridgeFactory({
advertiseElicitation: true,
publishAppDocument: publishDocument,

287
clients/web/src/test/core/react/useStoreSnapshot.test.tsx Normal file
View file

@ -0,0 +1,287 @@
import { describe, it, expect, beforeEach, vi } from "vitest";
import { act, renderHook } from "@testing-library/react";
import { TypedEventTarget } from "@inspector/core/mcp/typedEventTarget";
import { useStoreSnapshot } from "@inspector/core/react/useStoreSnapshot";
interface Item {
label: string;
seen?: boolean;
}
interface FakeStoreEventMap {
itemsChange: Item[];
otherChange: number;
}
/**
* A minimal stand-in for the real state stores, reproducing the two properties
* that make them awkward to read from `useSyncExternalStore`: the getter hands
* back a fresh defensive copy on every call, and the list is mutated in place
* (both by appending and, in `markSeen`, by mutating an entry the list already
* holds — the shape `MessageLogState` uses when it folds a response into its
* request entry).
*/
class FakeStore extends TypedEventTarget<FakeStoreEventMap> {
private items: Item[] = [];
getItems(): Item[] {
return [...this.items];
}
add(label: string): void {
this.items.push({ label });
this.dispatchTypedEvent("itemsChange", this.getItems());
}
/** Mutate an existing entry in place, then announce it. */
markSeen(label: string): void {
const entry = this.items.find((i) => i.label === label);
if (entry) entry.seen = true;
this.dispatchTypedEvent("itemsChange", this.getItems());
}
/** A dispatch on a different event, to prove per-event isolation. */
bumpOther(): void {
this.dispatchTypedEvent("otherChange", 1);
}
}
const NO_ITEMS: Item[] = [];
const readItems = (store: FakeStore): Item[] => store.getItems();
/**
* The shared subscription primitive behind every list/log hook in
* `core/react` (#1955). Its job is to read a store during render — closing
* both the stale-frame-on-store-swap and the missed-update windows the old
* `useState` + `useEffect` shape carried — while still handing
* `useSyncExternalStore` a referentially stable snapshot.
*/
describe("useStoreSnapshot", () => {
let store: FakeStore;
beforeEach(() => {
store = new FakeStore();
});
it("reports the fallback when there is no store", () => {
const { result } = renderHook(() =>
useStoreSnapshot(null, "itemsChange", readItems, NO_ITEMS),
);
expect(result.current).toBe(NO_ITEMS);
});
it("reads the store's current value on the first render", () => {
store.add("a");
const { result } = renderHook(() =>
useStoreSnapshot(store, "itemsChange", readItems, NO_ITEMS),
);
expect(result.current).toEqual([{ label: "a" }]);
});
it("updates when the store dispatches its event", () => {
const { result } = renderHook(() =>
useStoreSnapshot(store, "itemsChange", readItems, NO_ITEMS),
);
expect(result.current).toEqual([]);
act(() => store.add("a"));
expect(result.current).toEqual([{ label: "a" }]);
});
it("returns the identical value across renders with no dispatch", () => {
store.add("a");
const { result, rerender } = renderHook(() =>
useStoreSnapshot(store, "itemsChange", readItems, NO_ITEMS),
);
const first = result.current;
rerender();
rerender();
// Identity, not equality: a fresh array per read is what makes
// `useSyncExternalStore` loop, and what defeats every downstream memo.
expect(result.current).toBe(first);
});
it("reflects a store swap in the same render, with no stale frame", () => {
const next = new FakeStore();
store.add("from-first");
next.add("from-second");
// Every render's value, captured during render rather than after commit —
// the whole point is that no committed frame ever carries the previous
// store's data.
const rendered: Item[][] = [];
const { rerender } = renderHook(
({ s }: { s: FakeStore }) => {
const value = useStoreSnapshot(s, "itemsChange", readItems, NO_ITEMS);
rendered.push(value);
return value;
},
{ initialProps: { s: store } },
);
expect(rendered.at(-1)).toEqual([{ label: "from-first" }]);
const rendersBeforeSwap = rendered.length;
rerender({ s: next });
// Not one render after the swap carried the old store's list.
expect(rendered.slice(rendersBeforeSwap)).not.toContainEqual([
{ label: "from-first" },
]);
expect(rendered.at(-1)).toEqual([{ label: "from-second" }]);
});
it("picks up a change dispatched between the render and the subscribe", () => {
// The second defect the conversion fixes: the old shape took its snapshot
// during render and attached its listener in an effect, so anything the
// store did in between was lost. Reproduce that window exactly, by having
// the store change on its way into `addEventListener`.
const racing = new FakeStore();
const realAdd = racing.addEventListener.bind(racing);
racing.addEventListener = ((
type: "itemsChange",
listener: (event: Event) => void,
) => {
racing.add("slipped-in");
realAdd(type, listener);
}) as FakeStore["addEventListener"];
const { result } = renderHook(() =>
useStoreSnapshot(racing, "itemsChange", readItems, NO_ITEMS),
);
expect(result.current).toEqual([{ label: "slipped-in" }]);
});
it("re-reads when a dispatch mutated an entry the list already held", () => {
// The `MessageLogState` fold: same entries, same length, same references —
// only a field inside one of them changed. Caching against the dispatch
// count rather than the contents is what keeps this visible.
store.add("a");
const { result } = renderHook(() =>
useStoreSnapshot(store, "itemsChange", readItems, NO_ITEMS),
);
const before = result.current;
act(() => store.markSeen("a"));
expect(result.current).not.toBe(before);
expect(result.current).toEqual([{ label: "a", seen: true }]);
});
it("ignores a dispatch of a different event on the same store", () => {
store.add("a");
const { result, rerender } = renderHook(() =>
useStoreSnapshot(store, "itemsChange", readItems, NO_ITEMS),
);
const before = result.current;
act(() => store.bumpOther());
rerender();
expect(result.current).toBe(before);
});
it("falls back when the store is taken away", () => {
store.add("a");
const { result, rerender } = renderHook(
({ s }: { s: FakeStore | null }) =>
useStoreSnapshot(s, "itemsChange", readItems, NO_ITEMS),
{ initialProps: { s: store as FakeStore | null } },
);
expect(result.current).toEqual([{ label: "a" }]);
rerender({ s: null });
expect(result.current).toBe(NO_ITEMS);
});
it("re-reads when the reader changes under an unchanged store", () => {
// The cache key covers every input the value was derived from, not just
// the store and its revision — otherwise a new reader is answered with the
// previous reader's output.
store.add("a");
const readLabels = (s: FakeStore): string[] =>
s.getItems().map((i) => i.label);
const readCount = (s: FakeStore): string[] => [String(s.getItems().length)];
const { result, rerender } = renderHook(
({ read }: { read: (s: FakeStore) => string[] }) =>
useStoreSnapshot(store, "itemsChange", read, []),
{ initialProps: { read: readLabels } },
);
expect(result.current).toEqual(["a"]);
rerender({ read: readCount });
expect(result.current).toEqual(["1"]);
});
it("re-reads when the fallback changes while the store is absent", () => {
// With no store the revision is pinned at 0, so the revision alone can
// never notice this one.
const first: Item[] = [{ label: "first-fallback" }];
const second: Item[] = [{ label: "second-fallback" }];
const { result, rerender } = renderHook(
({ absent }: { absent: Item[] }) =>
useStoreSnapshot(null, "itemsChange", readItems, absent),
{ initialProps: { absent: first } },
);
expect(result.current).toBe(first);
rerender({ absent: second });
expect(result.current).toBe(second);
});
it("re-reads when the watched event changes", () => {
store.add("a");
// Widen the prop up front rather than at each `rerender` — `initialProps`
// is what fixes the type, so a bare literal there pins it to that one
// event and the swap below stops compiling.
const initialProps: { event: keyof FakeStoreEventMap } = {
event: "itemsChange",
};
const { result, rerender } = renderHook(
({ event }: { event: keyof FakeStoreEventMap }) =>
useStoreSnapshot(store, event, readItems, NO_ITEMS),
{ initialProps },
);
const first = result.current;
// Same store and same reader, so only the event distinguishes the two —
// and each event carries its own revision, so the cached entry must not
// be reused across them.
rerender({ event: "otherChange" });
expect(result.current).not.toBe(first);
expect(result.current).toEqual([{ label: "a" }]);
});
it("stops listening on unmount", () => {
// Assert on the listener, not on the rendered value. Watching
// `result.current` stay put after unmount proves nothing: React drops a
// store notification for an unmounted component either way, so that test
// passes with the listener still attached — which is the leak it was
// supposed to catch.
const addSpy = vi.spyOn(store, "addEventListener");
const removeSpy = vi.spyOn(store, "removeEventListener");
const { unmount } = renderHook(() =>
useStoreSnapshot(store, "itemsChange", readItems, NO_ITEMS),
);
expect(addSpy).toHaveBeenCalledWith("itemsChange", expect.any(Function));
const subscribed = addSpy.mock.calls[0]?.[1];
unmount();
// The exact function that was attached is the one detached — a cleanup
// that removed some other listener, or none, fails here.
expect(removeSpy).toHaveBeenCalledWith("itemsChange", subscribed);
});
it("unmounts cleanly with no store to unsubscribe from", () => {
const { unmount } = renderHook(() =>
useStoreSnapshot(null, "itemsChange", readItems, NO_ITEMS),
);
expect(() => unmount()).not.toThrow();
});
});