Merge branch 'v2/main' into v2/fix/2165-oauth-recovery-floating-promises

clients/web/src/hooks/useConnectionLifecycle.ts

@@ -245,18 +245,26 @@ export function useConnectionLifecycle({
   sessionReset,
   seedModernLogLevel,
 }: UseConnectionLifecycleOptions): ConnectionLifecycle {
-  const [connectedServerId, setConnectedServerId] = useState<
+  // Track the just-connected server so its card gets the green highlight +
-    string | undefined
+  // scroll-into-view (#1682). It is a pure function of the two values it reads,
-  >(undefined);
+  // 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
+  // ref carries the latest value instead, so client construction reads the
-  // whichever entry point (connect, deep link, OAuth callback) reached it.
+  // 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)
+  useLayoutEffect(() => {
-  sandboxUrlRef.current = sandboxUrl;
+    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`).

clients/web/src/hooks/useMcpApps.ts

@@ -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)
+  useLayoutEffect(() => {
-  listedResourcesRef.current = resources;
+    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);
+  // Held as lazily-initialized state rather than a ref seeded during render:
-  appElicitationControllerRef.current ??= new AppElicitationController();
+  // the instance is never reassigned, so `useState`'s initializer gives the
-  const appElicitationController = appElicitationControllerRef.current;
+  // 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,