From ede9658d6247b1fec4dd7cf90b2fe7896eb87b45 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 6 Sep 2026 03:00:31 -0400 Subject: [PATCH] fix: guard the automatic read, and reset panel scroll on skill change (#2263) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 11: zero inline comments, two findings in the suppressed block. Third round where the inline count alone would have ended the cycle. The automatic SKILL.md read fired twice. I noted this possibility early and dismissed it as harmless dev noise, which was wrong twice over: `ScreenStage` remounts this screen while the selection persists, so it is not dev-only, and this app is a protocol inspector — a phantom `resources/read` in the Protocol panel that the user's own client would never send is the tool misreporting the conversation, not merely a wasted round trip. An `autoReadKey` ref makes the replay a no-op. Panel scroll positions survived a skill change. `scrollTop` lives on the DOM node rather than in React state, so switching between two long manifests showed the new one part-way down with its first rows hidden, which reads as missing data. The accordion is keyed by `manifestKey` so every panel gets a fresh scroll container; `openSections` is controlled and survives. ⚠️ The test for the first fix is a SPECIFICATION, not a regression guard, and is labelled as such in the file. It was written, then checked by removing the guard — and it still passed. Three shapes were tried (a plain re-render, a StrictMode wrapper, StrictMode with the selection present at mount) and none reproduce the replay, because this environment does not double-invoke mount effects. The fix stands on the reported reasoning; the test pins the contract of one read per selection and must not be read as evidence the defect is fixed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB Signed-off-by: cliffhall --- .../SkillsScreen/SkillsScreen.test.tsx | 48 +++++++++++++++++-- .../screens/SkillsScreen/SkillsScreen.tsx | 23 +++++++++ 2 files changed, 68 insertions(+), 3 deletions(-) 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. */} }