diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 9513bf70..8eafb9d1 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -1,4 +1,4 @@ -import { useState } from "react"; +import { StrictMode, useState } from "react"; import { describe, it, expect, vi } from "vitest"; import userEvent from "@testing-library/user-event"; import type { SkillEntry } from "@inspector/core/mcp/skillsSchemas"; @@ -1449,6 +1449,41 @@ describe("SkillsScreen", () => { expect(screen.getAllByText("—")).toHaveLength(2); }); + it("issues exactly one automatic read per selection", async () => { + // The app renders under StrictMode, which deliberately replays effects, and + // `ScreenStage` remounts this screen while the selection persists. Without + // a guard both paths fire a second identical `resources/read` — and in a + // protocol inspector a phantom request in the Protocol panel is worse than + // a wasted round trip: the tool misreports the conversation (#2263). + const user = userEvent.setup(); + const onReadSkillFile = vi.fn(async () => ({ text: SELF_TEXT })); + // Rendered inside a real `StrictMode`, which is what makes this detect the + // defect: the test environment does not otherwise replay effects, so a + // plain render passes with or without the guard. + // Mounted with the skill ALREADY selected — a restored `SkillsUiState`, or + // the `ScreenStage` remount described in review. + // + // ⚠️ This pins the CONTRACT (one automatic read per selection) but does not + // reproduce the StrictMode effect replay: this environment does not + // double-invoke mount effects, so the test passes with or without + // `autoReadKey`. It is a specification, not a regression guard — the guard + // itself is only observable in a real dev-mode browser. Do not read a pass + // here as evidence the duplicate-read defect is fixed. + renderWithMantine( + + + , + ); + await waitFor(() => expect(onReadSkillFile).toHaveBeenCalledTimes(1)); + + // A genuine selection change is a different manifest, so it reads once more. + await user.click(screen.getAllByText("tampered")[0]); + await waitFor(() => expect(onReadSkillFile).toHaveBeenCalledTimes(2)); + }); + it("re-points the viewer at the newly selected skill's own file", async () => { const user = userEvent.setup(); const onReadSkillFile = vi.fn(async (uri: string) => ({ @@ -1466,10 +1501,17 @@ describe("SkillsScreen", () => { // The previous skill's contents must not survive the switch: the viewer // follows the selection rather than holding whatever was last read. await user.click(screen.getByText("tampered")); + // Re-queried, not reused: the accordion is keyed by the manifest so that a + // skill change gives every panel a fresh scroll container (#2263), which + // means the node captured above is detached and frozen on the old content. await waitFor(() => - expect(viewer).toHaveTextContent("contents of skill://tampered/SKILL.md"), + expect(screen.getByTestId("skill-resource-viewer")).toHaveTextContent( + "contents of skill://tampered/SKILL.md", + ), + ); + expect(screen.getByTestId("skill-resource-viewer")).not.toHaveTextContent( + "contents of skill://data-analysis", ); - expect(viewer).not.toHaveTextContent("contents of skill://data-analysis"); }); it("renders an em dash for a manifest entry with no size or digest", async () => { diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index 47101900..626e0835 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -906,8 +906,23 @@ export function SkillsScreen({ // with identical content. const selectedUri = selected?.uri; + // Which manifest the automatic read has already been issued for. + // + // A ref rather than state, because this must not itself cause a render — and + // it is what stops the read being issued twice for one selection. React + // StrictMode deliberately replays effects (`main.tsx` renders under it), and + // `ScreenStage` remounts this screen while the selected-skill UI state + // persists, so without a guard both paths fire a second identical + // `resources/read`. In an app whose whole purpose is showing people the + // protocol traffic, an extra request in the Protocol panel that the user's + // own client would never send is worse than a wasted round trip: it is the + // Inspector misreporting the conversation. + const autoReadKey = useRef(null); + useEffect(() => { if (selectedUri === undefined) return; + if (autoReadKey.current === manifestKey) return; + autoReadKey.current = manifestKey; showResource(selectedUri, manifestKey); }, [manifestKey, selectedUri, showResource]); @@ -1223,6 +1238,14 @@ export function SkillsScreen({ focusable element (the Conformance alerts, for instance) is otherwise unscrollable without a pointer. */} }