diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 8eafb9d1..2531c90e 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -1449,26 +1449,23 @@ 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). + it("issues exactly one automatic read per selection within a mount", async () => { + // The app renders under StrictMode, which deliberately replays effects, so + // without a guard one selection fires two identical `resources/read` calls + // — 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). The scope is deliberately one MOUNT: a `ScreenStage` remount + // mints a fresh ref and reads again, which is correct, because the preview + // bytes are local state and died with the same unmount. 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. + // Mounted with the skill ALREADY selected — a restored `SkillsUiState`. // - // ⚠️ 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. + // ⚠️ This pins the CONTRACT (one automatic read per selection) rather than + // guarding it: this environment does not double-invoke mount effects, so + // the test passes with or without `autoReadKey`. Do not read a pass here as + // evidence the duplicate-read defect is fixed; that is only observable in a + // real dev-mode browser. The reappearance test below IS a guard. renderWithMantine( { await waitFor(() => expect(onReadSkillFile).toHaveBeenCalledTimes(2)); }); + it("re-reads when the selected entry leaves the list and comes back", async () => { + // A refresh in flight (or a disconnect) can empty `skills` while the + // selection persists. The render invalidates the preview, so the viewer is + // blank — and when the IDENTICAL entry returns its `manifestKey` matches + // what the guard still holds. Without clearing the guard on the way out, + // the read is skipped and the viewer stays permanently empty (#2263). + const onReadSkillFile = vi.fn(async () => ({ + text: "---\nname: data-analysis\n---\n\nreloaded-body\n", + })); + const selectedUi = { + ...EMPTY_SKILLS_UI, + selectedSkillUri: CLEAN_SKILL.uri, + }; + const { rerender } = renderWithMantine( + , + ); + await waitFor(() => expect(onReadSkillFile).toHaveBeenCalledTimes(1)); + await waitFor(() => + expect(screen.getByTestId("skill-resource-viewer")).toHaveTextContent( + "reloaded-body", + ), + ); + + rerender( + , + ); + rerender( + , + ); + + await waitFor(() => expect(onReadSkillFile).toHaveBeenCalledTimes(2)); + await waitFor(() => + expect(screen.getByTestId("skill-resource-viewer")).toHaveTextContent( + "reloaded-body", + ), + ); + }); + 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) => ({ diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index 626e0835..93a9ae7a 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -908,19 +908,33 @@ export function SkillsScreen({ // 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. + // A ref rather than state, because this must not itself cause a render. Its + // scope is ONE MOUNT, and that is the whole of what it can promise: React + // StrictMode deliberately replays effects (`main.tsx` renders under it), so + // without a guard a single selection fires two identical `resources/read` + // calls. 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. + // + // It deliberately does NOT suppress the read after a `ScreenStage` remount, + // and could not: a remount mints a fresh ref. That is the correct behavior + // rather than a gap, because `previewState` is local state too and dies with + // the same unmount — so on return the viewer is empty and the read is what + // refills it. Suppressing it would need the preview bytes hoisted above + // `ScreenStage`, which would trade a legitimate request for a cache. const autoReadKey = useRef(null); useEffect(() => { - if (selectedUri === undefined) return; + if (selectedUri === undefined) { + // The entry left `skills` — a refresh in flight, or a disconnect. + // `useValueChange` has already emptied the viewer, so if the IDENTICAL + // entry reappears its `manifestKey` matches what this ref still holds + // and the read would be skipped, stranding the viewer permanently + // blank. Clearing the guard is what lets that reappearance re-read. + autoReadKey.current = null; + return; + } if (autoReadKey.current === manifestKey) return; autoReadKey.current = manifestKey; showResource(selectedUri, manifestKey);