fix: guard the automatic read, and reset panel scroll on skill change (#2263)

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB
Signed-off-by: cliffhall <cliff@futurescale.com>

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(
+      <StrictMode>
+        <ControlledSkillsScreen
+          onReadSkillFile={onReadSkillFile}
+          ui={{ ...EMPTY_SKILLS_UI, selectedSkillUri: CLEAN_SKILL.uri }}
+        />
+      </StrictMode>,
+    );
+    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 () => {

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<string | null>(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. */}
             <Accordion
+              // Keyed by the manifest so a skill change gives every panel a
+              // FRESH scroll container. `scrollTop` lives on the DOM node, not
+              // in React state, so reusing these panels carried one skill's
+              // scroll position into the next — switching between two long
+              // manifests showed the new one part-way down with its first rows
+              // hidden, which reads as missing data rather than as scroll.
+              // `openSections` is controlled, so it survives the remount.
+              key={manifestKey}
               multiple
               variant="skillSections"
               chevron={<RiArrowRightSLine />}