modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 5

fix(skills): clear the auto-read guard when the selection leaves the list

Browse source 
`autoReadKey` cached the manifest key of the entry it had read for. When the
selected entry left `skills` — a refresh in flight, a disconnect — the render
emptied the preview but the ref kept the key. An identical entry reappearing
therefore matched the cache, the automatic read was skipped, and the viewer
stayed blank with no recovery short of reselecting. Clearing the guard while
no entry is selected is what lets the reappearance re-read.

Also corrects the guard's stated scope. It cannot survive a `ScreenStage`
remount — a remount mints a fresh ref — so the round-11 comment claiming it
did was wrong. That read is correct behavior rather than a gap: `previewState`
is local state and dies with the same unmount, so on return the viewer is
empty and the read is what refills it. Suppressing it would mean hoisting the
preview bytes above `ScreenStage` and showing bytes fetched under a connection
that may no longer exist.

The reappearance test is a real regression guard: restoring the defect fails
it while the older one-read-per-selection test still passes, so it is both
detecting and non-redundant. That older test remains a specification, for the
reason it already states, and now points at this one.

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>
This commit is contained in:
cliffhall 2026-09-06 03:15:50 -04:00
commit 25255c3499
2 changed files with 86 additions and 27 deletions
Download patch file Download diff file Expand all files Collapse all files

79
clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx
View file

@ -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(
<StrictMode>
<ControlledSkillsScreen
@ -1484,6 +1481,54 @@ describe("SkillsScreen", () => {
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(
<ControlledSkillsScreen
onReadSkillFile={onReadSkillFile}
ui={selectedUi}
/>,
);
await waitFor(() => expect(onReadSkillFile).toHaveBeenCalledTimes(1));
await waitFor(() =>
expect(screen.getByTestId("skill-resource-viewer")).toHaveTextContent(
"reloaded-body",
),
);
rerender(
<ControlledSkillsScreen
onReadSkillFile={onReadSkillFile}
skills={[]}
ui={selectedUi}
/>,
);
rerender(
<ControlledSkillsScreen
onReadSkillFile={onReadSkillFile}
ui={selectedUi}
/>,
);
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) => ({

34
clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx
View file

@ -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<string | null>(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);