fix(web): stop the remaining consumers calling a landed write a failure

Copilot review r3. Three findings, all the same root: a
ServerListReloadError rejects after the write succeeded, so any consumer
still reading "rejection" as "the write failed" now says something false.

- `useImportClientConfig.applyEntry` gave a successfully imported server a
  red "Failed" badge. It now reports the intended status and puts the
  reload message in `detail`, which renders in red beneath it either way —
  so the failure stays visible without claiming the row wasn't created.
- The settings-draft toast said "Failed to save settings". It now branches
  its title; this toast is the only signal there, since the modal has
  usually closed by the time a flush rejects.
- An edit modal left open by a failed reload kept targeting the old id.
  When the row landed under the new one, `configModalTarget` went
  undefined and ServerConfigModal's own reset blanked the open form and
  cleared the error it was showing. An edit/clone modal whose target has
  left the list now closes — adjusted during render via `useValueChange`,
  so no frame paints the blanked form. That also covers the ordinary case
  of an external mcp.json edit removing the server being edited.

The reachability here is not remote: the write is what triggers the
backend's change broadcast, so the SSE-driven refresh that lands the
renamed row is the likely next event.

Also adds the regression test r3 asked for on the add path — the
highlight batch has to be marked from the catch for the same reason the
rename is. Both new App tests and the import one fail without their fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>

clients/web/src/App.test.tsx

@@ -223,8 +223,9 @@ const { updateServerSettingsSpy } = vi.hoisted(() => ({
 
 // Same idea for the edit-modal rename path (#1914): the test needs to make the
 // PUT's *reload* fail, which only a spy it controls can do.
-const { updateServerSpy } = vi.hoisted(() => ({
+const { updateServerSpy, addServerSpy } = vi.hoisted(() => ({
   updateServerSpy: vi.fn(() => Promise.resolve()),
+  addServerSpy: vi.fn(() => Promise.resolve()),
 }));
 // Stable spy for the tools list-changed acknowledgement, so a test can assert
 // the paginated Refresh clears the indicator (#1721).
@@ -240,7 +241,7 @@ vi.mock("@inspector/core/react/useServers.js", async (importOriginal) => ({
   >()),
   useServers: vi.fn(() => ({
     servers: [SERVER_A],
-    addServer: vi.fn(),
+    addServer: addServerSpy,
     updateServer: updateServerSpy,
     updateServerSettings: updateServerSettingsSpy,
     removeServer: vi.fn(),
@@ -425,6 +426,8 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({
     onRefreshTasks: () => void;
     onServerSettings: (id: string) => void;
     onServerEdit: (id: string) => void;
+    onServerAdd: () => void;
+    highlightedServerIds?: string[];
     onClearProtocol: () => void;
     onReplayProtocol: (id: string) => void;
     onTogglePinProtocol: (id: string) => void;
@@ -550,9 +553,14 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({
       </button>
       <button onClick={() => props.onSetLogLevel("debug")}>set-level</button>
       <button onClick={() => props.onServerSettings("A")}>open-settings</button>
-      {/* The real server grid (and its Edit button) lives inside this mocked
+      {/* The real server grid (and its Add / Edit controls) lives inside this
-          view, so the edit modal is only reachable through its callback. */}
+          mocked view, so the config modal is only reachable through these
+          callbacks — and the highlight batch only observable through this prop. */}
       <button onClick={() => props.onServerEdit("A")}>edit-server</button>
+      <button onClick={() => props.onServerAdd()}>add-server</button>
+      <span data-testid="highlighted-servers">
+        {(props.highlightedServerIds ?? []).join(",") || "none"}
+      </span>
       <span data-testid="pinned-history">
         {Array.from(props.pinnedProtocolIds ?? []).join(",")}
       </span>
@@ -2481,12 +2489,11 @@ describe("App paginated list pagination toggle (#1721)", () => {
   });
 });
 
-// `onConfigSubmit` moves `activeServerId` to the new id only after
+// A post-write reload failure rejects *after* the write landed, so every piece
-// `updateServer` *resolves*. Once a failed list reload rejects, the rename is
+// of state `onConfigSubmit` used to apply on the resolve path has to be applied
-// still on disk — so without the catch-side `followRename` the active id stays
+// on the reject path too — the rename target and the highlight batch both
-// on the old one, and the next successful refresh leaves it pointing at a row
+// describe a row that really is on disk (#1914 review r2 / r3).
-// that no longer exists (#1914 review r2).
+describe("App config submit with a failed list reload (#1914)", () => {
-describe("App edit rename with a failed list reload (#1914)", () => {
   // Earlier describes install their own `useServers` return value, which
   // outlives them — so this block installs its own and puts the previous
   // implementation back, rather than trusting whatever leaked in.
@@ -2497,7 +2504,7 @@ describe("App edit rename with a failed list reload (#1914)", () => {
     loading: false,
     error: undefined,
     refresh: vi.fn().mockResolvedValue(undefined),
-    addServer: vi.fn().mockResolvedValue(undefined),
+    addServer: addServerSpy,
     updateServer: updateServerSpy,
     updateServerSettings: updateServerSettingsSpy,
     removeServer: vi.fn().mockResolvedValue(undefined),
@@ -2512,6 +2519,7 @@ describe("App edit rename with a failed list reload (#1914)", () => {
       ? () => vi.mocked(useServers).mockImplementation(previous)
       : undefined;
     updateServerSpy.mockClear();
+    addServerSpy.mockClear();
     updateServerSettingsSpy.mockClear();
     vi.mocked(useServers).mockReturnValue(serversResult(["A"]));
   });
@@ -2530,7 +2538,7 @@ describe("App edit rename with a failed list reload (#1914)", () => {
         "The server was saved, but the server list could not be reloaded: disk full",
       ),
     );
-    renderWithMantine(<App />);
+    const { rerender } = renderWithMantine(<App />);
     await user.click(screen.getByText("connect"));
     await waitFor(() => expect(clientInstances).toHaveLength(1));
 
@@ -2552,12 +2560,19 @@ describe("App edit rename with a failed list reload (#1914)", () => {
       await screen.findByText(/could not be reloaded: disk full/),
     ).toBeInTheDocument();
 
-    // Now let the row land, as a later successful refresh would, and read the
+    // Now let the row land, as the SSE refresh the write itself triggers
-    // active id back off the toggle.
+    // would. The modal's target ("A") is gone, so it closes rather than
+    // blanking its own form — `initialId`/`initialConfig` would go undefined
+    // and ServerConfigModal's reset would wipe the open form and the error
+    // it is showing (#1914 r3).
     vi.mocked(useServers).mockReturnValue(serversResult(["A-renamed"]));
-    await user.click(screen.getByRole("button", { name: "Cancel" }));
+    rerender(<App />);
-    await user.click(screen.getByText("paginated-on"));
+    await waitFor(() =>
+      expect(screen.queryByLabelText(/Server ID/)).not.toBeInTheDocument(),
+    );
 
+    // With the modal gone, read the active id back off the toggle.
+    await user.click(screen.getByText("paginated-on"));
     await waitFor(() =>
       expect(updateServerSettingsSpy).toHaveBeenCalledWith(
         "A-renamed",
@@ -2565,6 +2580,38 @@ describe("App edit rename with a failed list reload (#1914)", () => {
       ),
     );
   });
+
+  it("keeps a successful add in the highlight batch when the reload failed (#1914)", async () => {
+    // `addServerHighlighted` marked the new id only after `addServer`
+    // resolved. The row is on disk either way, so on a reload failure it has
+    // to be marked from the catch — the next successful refresh is what
+    // renders it, and it would otherwise arrive unhighlighted.
+    const user = userEvent.setup();
+    addServerSpy.mockRejectedValueOnce(
+      new ServerListReloadError(
+        "The server was added, but the server list could not be reloaded: disk full",
+      ),
+    );
+    renderWithMantine(<App />);
+    expect(screen.getByTestId("highlighted-servers")).toHaveTextContent("none");
+
+    await user.click(screen.getByText("add-server"));
+    await user.type(await screen.findByLabelText(/Server ID/), "brand-new");
+    await user.type(screen.getByLabelText(/^Command/), "node");
+    await user.click(screen.getByRole("button", { name: "Add" }));
+
+    await waitFor(() =>
+      expect(addServerSpy).toHaveBeenCalledWith("brand-new", expect.anything()),
+    );
+    expect(
+      await screen.findByText(/could not be reloaded: disk full/),
+    ).toBeInTheDocument();
+    await waitFor(() =>
+      expect(screen.getByTestId("highlighted-servers")).toHaveTextContent(
+        "brand-new",
+      ),
+    );
+  });
 });
 
 // A background command runs through `runCommandInBackground`, which owns the

clients/web/src/App.tsx

@@ -113,6 +113,7 @@ import { usePagedTools } from "@inspector/core/react/usePagedTools.js";
 import { usePagedPrompts } from "@inspector/core/react/usePagedPrompts.js";
 import { usePagedResources } from "@inspector/core/react/usePagedResources.js";
 import { usePaginatedList } from "./hooks/usePaginatedList";
+import { useValueChange } from "./hooks/useValueChange";
 import type { ListPaginationControlsProps } from "./components/elements/ListPaginationControls/ListPaginationControls";
 import { useManagedResourceTemplates } from "@inspector/core/react/useManagedResourceTemplates.js";
 import { useManagedRequestorTasks } from "@inspector/core/react/useManagedRequestorTasks.js";
@@ -4140,6 +4141,30 @@ function App() {
     return servers.find((s) => s.id === configModal.targetId);
   }, [configModal, servers]);
 
+  // An edit/clone modal whose target has left the list can no longer render a
+  // coherent form: `initialId`/`initialConfig` go undefined, and
+  // ServerConfigModal's own `useValueChange` reset then blanks the open form
+  // *and* clears the error it is showing.
+  //
+  // Only reachable since #1914 made the modal stay open on a failed post-write
+  // reload: rename A → B, the PUT lands, the reload fails, and the modal sits
+  // there targeting A. The write itself is what triggers the backend's change
+  // broadcast, so the SSE-driven background refresh that lands B (and drops A)
+  // is the *likely* next event, not a remote one. Closing is right — the edit
+  // is on disk and the list now agrees — and it covers the ordinary case too,
+  // where an external mcp.json edit removes the server being edited.
+  //
+  // Adjusted during render rather than in an effect (see `useValueChange`), so
+  // no frame ever paints the blanked form.
+  useValueChange(
+    configModal?.mode === "add" || configModal?.targetId === undefined
+      ? true
+      : servers.some((s) => s.id === configModal.targetId),
+    (targetPresent) => {
+      if (!targetPresent) setConfigModal(null);
+    },
+  );
+
   const settingsModalTarget = useMemo(() => {
     if (!settingsModalTargetId) return undefined;
     return servers.find((s) => s.id === settingsModalTargetId);
@@ -4188,7 +4213,14 @@ function App() {
     // didn't (especially painful for the OAuth client secret).
     onError: (id, err) => {
       notifications.show({
-        title: `Failed to save settings for "${id}"`,
+        // A `ServerListReloadError` means the PUT landed and only reading the
+        // list back failed, so "Failed to save" would be false — and this
+        // toast is the user's only signal here, since the modal has usually
+        // closed by the time a flush rejects (#1914 r3).
+        title:
+          err instanceof ServerListReloadError
+            ? `Saved settings for "${id}", but the server list did not reload`
+            : `Failed to save settings for "${id}"`,
         message: err instanceof Error ? err.message : String(err),
         color: "red",
       });

clients/web/src/hooks/useImportClientConfig.test.tsx

@@ -6,6 +6,7 @@ import {
 } from "./useImportClientConfig";
 import type { ImportSourceResult } from "@inspector/core/mcp/import/index.js";
 import type { MCPConfig } from "@inspector/core/mcp/types.js";
+import { ServerListReloadError } from "@inspector/core/react/useServers.js";
 
 function config(...ids: string[]): MCPConfig {
   const mcpServers: MCPConfig["mcpServers"] = {};
@@ -207,6 +208,31 @@ describe("useImportClientConfig", () => {
     expect(outcome.detail).toBe("add failed");
   });
 
+  it("reports a reload failure as imported-with-a-warning, not failed (#1914)", async () => {
+    // A `ServerListReloadError` rejects *after* the write landed, so the entry
+    // really was added. A red "Failed" badge would contradict the error's own
+    // message and invite a retry that then collides with the row it claims
+    // wasn't created. The reason still shows, as `detail`.
+    const onAddServer = vi.fn(async () => {
+      throw new ServerListReloadError(
+        "The server was added, but the server list could not be reloaded: disk full",
+      );
+    });
+    const { result } = setup({
+      onFetchSource: vi.fn(async () => foundSource(config("alpha"))),
+      onAddServer,
+    });
+    await act(async () => {
+      await result.current.pickSource("claude");
+    });
+    await act(async () => {
+      await result.current.runImport();
+    });
+    const outcome = result.current.outcomes[0];
+    expect(outcome.status).toBe("added");
+    expect(outcome.detail).toMatch(/could not be reloaded: disk full/);
+  });
+
   it("resolves conflicts by overwrite, skip, and rename", async () => {
     const onAddServer = vi.fn(async () => {});
     const onUpdateServer = vi.fn(async () => {});

clients/web/src/hooks/useImportClientConfig.ts

@@ -8,6 +8,7 @@ import {
   type ImportSourceResult,
 } from "@inspector/core/mcp/import/index.js";
 import { validateStoreId } from "@inspector/core/storage/store-id.js";
+import { ServerListReloadError } from "@inspector/core/react/useServers.js";
 import type { MCPConfig, MCPServerConfig } from "@inspector/core/mcp/types.js";
 
 export type ImportPhase = "select" | "loading" | "review" | "summary";
@@ -198,11 +199,17 @@ export function useImportClientConfig({
       await write();
       return { id, status: outcome };
     } catch (err) {
-      return {
+      const detail = err instanceof Error ? err.message : String(err);
-        id,
+      // A `ServerListReloadError` means the write landed and only reading the
-        status: "failed",
+      // list back failed, so this entry really was imported. Reporting it as
-        detail: err instanceof Error ? err.message : String(err),
+      // `failed` would contradict the error's own message and invite a retry
-      };
+      // that then collides with the row it says wasn't created (#1914 r3).
+      // The status is the truth about the write; `detail` renders in red
+      // beneath it either way, so the reload failure is still visible.
+      if (err instanceof ServerListReloadError) {
+        return { id, status: outcome, detail };
+      }
+      return { id, status: "failed", detail };
     }
   }