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

Merge pull request #2191 from modelcontextprotocol/v2/chore/2161-react-hooks-suppressions

Browse source 
chore(web): resolve the react-hooks findings App.tsx's decomposition unmasked (#2161)
This commit is contained in:
Cliff Hall 2026-08-29 22:49:05 -04:00 • committed by GitHub
commit 1184792e26
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 51 additions and 30 deletions
Download patch file Download diff file Expand all files Collapse all files

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,