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

feat: refactor the Skills detail pane into collapsible sections (#2263)

Browse source 
The pane was one scrolling column: SKILL.md sat below the manifest behind a
"View SKILL.md" button, the frontmatter pushed it further out of view, and
nothing could be put away.

It is now a flex column that does not scroll as a whole. Conformance,
Resources, Frontmatter and Skill Resource are four collapsible sections of one
`disclosure` accordion, and the file viewer claims the remaining height and
scrolls internally.

The viewer's flex-basis is 0, not auto, and that is load-bearing: `auto` makes
an item's basis its content height, and a rendered document is ~1,795px for the
data-analysis fixture. That basis joined the sum the container distributes,
over-constraining the column so the shrink factors crushed the siblings —
Conformance to a 2px panel, Resources to zero — with their contents spilling
over the headers below. Collapsing the viewer removed the giant basis, so the
symptom appeared on collapse-then-reopen. Open sections also carry a `mih`
floor, since the disclosure CSS sets `min-height: 0` on an active item.

Frontmatter now follows the displayed file rather than the selected skill.
Both halves come from one `splitSkillFile` call, so the section and the viewer
cannot disagree, and a file with no frontmatter renders no section at all.

Also: SKILL.md loads on selection (a `resources/read` is not a load under
SEP-2640), the manifest highlights on hover with each URI a button that swaps
the viewed file, both whole-skill actions moved to the pane header, and the
Conformance badge goes yellow for a warnings-only entry instead of green.

Flat CSS properties moved out of call sites into theme variants (ThemeText
gains four; a new ThemeTable takes the manifest font size).

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-05 20:55:19 -04:00
commit e67eb3408e
9 changed files with 1070 additions and 311 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -17,7 +17,10 @@ import {
import { EMPTY_SKILLS_UI } from "../screenUiState"; import { EMPTY_SKILLS_UI } from "../screenUiState";
const REF_TEXT = "# Column rules\n"; const REF_TEXT = "# Column rules\n";
const SELF_TEXT = "# data-analysis\n"; // A real SKILL.md carries frontmatter, and the screen now splits it out of the
// served bytes (#2263) — so the fixture has to have some, or the Frontmatter
// section it drives would never render here.
const SELF_TEXT = "---\nname: data-analysis\n---\n\n# data-analysis\n";
const NOTES_TEXT = "different\n"; const NOTES_TEXT = "different\n";
// Computed once at module load so each fixture's advertised digest really is // Computed once at module load so each fixture's advertised digest really is
// the digest of the bytes the fake read returns — a hard-coded constant would // the digest of the bytes the fake read returns — a hard-coded constant would
@ -138,6 +141,14 @@ function ControlledSkillsScreen(props: Partial<SkillsScreenProps> = {}) {
); );
} }
// Mantine puts a Badge's colour on the ROOT as CSS custom properties, while
// `getByText` matches the inner label span — so the colour has to be read from
// the enclosing root rather than from the matched node.
function badgeStyle(text: RegExp): string {
const root = screen.getByText(text).closest(".mantine-Badge-root");
return root?.getAttribute("style") ?? "";
}
describe("SkillsScreen", () => { describe("SkillsScreen", () => {
it("renders the empty state until a skill is selected", () => { it("renders the empty state until a skill is selected", () => {
renderWithMantine(<SkillsScreen {...baseProps} />); renderWithMantine(<SkillsScreen {...baseProps} />);
@ -199,6 +210,28 @@ describe("SkillsScreen", () => {
expect(screen.queryByTestId("skill-issues")).not.toBeInTheDocument(); expect(screen.queryByTestId("skill-issues")).not.toBeInTheDocument();
}); });
it("badges a warning-only entry yellow, not green", async () => {
// Green reads as "nothing to see", which would hide the only signal the
// section carries for an entry whose findings are all warnings (#2263).
const user = userEvent.setup();
renderWithMantine(<ControlledSkillsScreen />);
await user.click(screen.getByText("dynamic-report"));
// `dynamic-resources` is a warning, and the only finding on this fixture.
const style = badgeStyle(/0 error\(s\), 1 warning\(s\)/);
expect(style).toContain("yellow");
expect(style).not.toContain("green");
});
it("badges a clean entry green and a broken one red", async () => {
const user = userEvent.setup();
renderWithMantine(<ControlledSkillsScreen />);
await user.click(screen.getByText("data-analysis"));
expect(badgeStyle(/0 error\(s\), 0 warning\(s\)/)).toContain("green");
await user.click(screen.getByText("right-name"));
expect(badgeStyle(/1 error\(s\), 0 warning\(s\)/)).toContain("red");
});
it("shows the name/path mismatch as a distinct, named finding", async () => { it("shows the name/path mismatch as a distinct, named finding", async () => {
const user = userEvent.setup(); const user = userEvent.setup();
renderWithMantine(<ControlledSkillsScreen />); renderWithMantine(<ControlledSkillsScreen />);
@ -262,7 +295,10 @@ describe("SkillsScreen", () => {
await user.click(screen.getByRole("button", { name: /Verify all/ })); await user.click(screen.getByRole("button", { name: /Verify all/ }));
// One alert per file in the manifest — both reads failed. // One alert per file in the manifest — both reads failed.
expect(await screen.findAllByText("Could not read file")).toHaveLength(2); expect(await screen.findAllByText("Could not read file")).toHaveLength(2);
expect(screen.getAllByText("403")).toHaveLength(2); // Three, not two: the same rejecting read also serves the SKILL.md the
// viewer loads on selection (#2263), so the message appears once per
// manifest row plus once in the viewer.
expect(screen.getAllByText("403")).toHaveLength(3);
}); });
it("wraps a non-Error read rejection", async () => { it("wraps a non-Error read rejection", async () => {
@ -273,7 +309,9 @@ describe("SkillsScreen", () => {
); );
await user.click(screen.getByText("data-analysis")); await user.click(screen.getByText("data-analysis"));
await user.click(screen.getByRole("button", { name: /Verify all/ })); await user.click(screen.getByRole("button", { name: /Verify all/ }));
expect(await screen.findAllByText("plain string")).toHaveLength(2); // Three: one per manifest row, plus the viewer's own auto-loaded SKILL.md
// read, which the same mock rejects (#2263).
expect(await screen.findAllByText("plain string")).toHaveLength(3);
}); });
it("titles a size disagreement a size mismatch, not a digest one", async () => { it("titles a size disagreement a size mismatch, not a digest one", async () => {
@ -336,7 +374,7 @@ describe("SkillsScreen", () => {
it("renders a base64 SKILL.md preview instead of a blank one", async () => { it("renders a base64 SKILL.md preview instead of a blank one", async () => {
// `onReadSkillFile` supports blob content, and verification reads it // `onReadSkillFile` supports blob content, and verification reads it
// correctly; dropping it in the preview would paint an empty box for a // correctly; dropping it in the viewer would paint an empty box for a
// file the screen had just checked. // file the screen had just checked.
const user = userEvent.setup(); const user = userEvent.setup();
const onReadSkillFile = vi.fn().mockResolvedValue({ const onReadSkillFile = vi.fn().mockResolvedValue({
@ -347,9 +385,8 @@ describe("SkillsScreen", () => {
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />, <ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
); );
await user.click(screen.getByText("data-analysis")); await user.click(screen.getByText("data-analysis"));
await user.click(screen.getByRole("button", { name: /View SKILL.md/ })); const viewer = screen.getByTestId("skill-resource-viewer");
const preview = await screen.findByTestId("skill-md-preview"); await waitFor(() => expect(viewer).toHaveTextContent("from a blob"));
expect(preview).toHaveTextContent("from a blob");
}); });
it("keeps the newest verdict when two verifications of one row overlap", async () => { it("keeps the newest verdict when two verifications of one row overlap", async () => {
@ -382,18 +419,21 @@ describe("SkillsScreen", () => {
/>, />,
); );
await user.click(screen.getByText("data-analysis")); await user.click(screen.getByText("data-analysis"));
// Selecting the skill already issued the viewer's own SKILL.md read
// (#2263), so the two clicks below are the reads AFTER that one.
const base = resolvers.length;
const rowVerify = screen.getByRole("button", { const rowVerify = screen.getByRole("button", {
name: "Verify skill://data-analysis/SKILL.md", name: "Verify skill://data-analysis/SKILL.md",
}); });
await user.click(rowVerify); await user.click(rowVerify);
await user.click(rowVerify); await user.click(rowVerify);
expect(resolvers).toHaveLength(2); expect(resolvers).toHaveLength(base + 2);
// The SECOND read answers first with the matching bytes, then the first // The SECOND read answers first with the matching bytes, then the first
// read answers with bytes that would verify as a mismatch. // read answers with bytes that would verify as a mismatch.
resolvers[1]({ text: SELF_TEXT }); resolvers[base + 1]({ text: SELF_TEXT });
expect(await screen.findByText("verified")).toBeInTheDocument(); expect(await screen.findByText("verified")).toBeInTheDocument();
resolvers[0]({ text: "stale bytes\n" }); resolvers[base]({ text: "stale bytes\n" });
// Still the newer verdict. // Still the newer verdict.
expect(await screen.findByText("verified")).toBeInTheDocument(); expect(await screen.findByText("verified")).toBeInTheDocument();
expect(screen.queryByText("mismatch")).not.toBeInTheDocument(); expect(screen.queryByText("mismatch")).not.toBeInTheDocument();
@ -608,19 +648,20 @@ describe("SkillsScreen", () => {
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />, <ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
); );
await user.click(screen.getByText("data-analysis")); await user.click(screen.getByText("data-analysis"));
const view = screen.getByRole("button", { name: /View SKILL.md/ }); // Past the viewer's own read for the selection (#2263).
const base = resolvers.length;
const view = screen.getByRole("button", {
name: "skill://data-analysis/reference.md",
});
await user.click(view); await user.click(view);
await user.click(view); await user.click(view);
expect(resolvers).toHaveLength(2); expect(resolvers).toHaveLength(base + 2);
resolvers[1]({ text: "# newest\n" }); const viewer = screen.getByTestId("skill-resource-viewer");
expect(await screen.findByTestId("skill-md-preview")).toHaveTextContent( resolvers[base + 1]({ text: "# newest\n" });
"newest", await waitFor(() => expect(viewer).toHaveTextContent("newest"));
); resolvers[base]({ text: "# stale\n" });
resolvers[0]({ text: "# stale\n" }); expect(viewer).not.toHaveTextContent("stale");
expect(screen.getByTestId("skill-md-preview")).not.toHaveTextContent(
"stale",
);
}); });
it("keeps the newest skills/get result when two fetches overlap", async () => { it("keeps the newest skills/get result when two fetches overlap", async () => {
@ -804,18 +845,21 @@ describe("SkillsScreen", () => {
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />, <ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
); );
await user.click(screen.getByText("data-analysis")); await user.click(screen.getByText("data-analysis"));
const view = screen.getByRole("button", { name: /View SKILL.md/ }); // Past the viewer's own read for the selection (#2263).
const base = resolvers.length;
const view = screen.getByRole("button", {
name: "skill://data-analysis/reference.md",
});
await user.click(view); await user.click(view);
await user.click(view); await user.click(view);
expect(resolvers).toHaveLength(2); expect(resolvers).toHaveLength(base + 2);
const viewer = screen.getByTestId("skill-resource-viewer");
// The OLDER read answers first, while the newer one is still in flight. // The OLDER read answers first, while the newer one is still in flight.
resolvers[0]({ text: "# stale\n" }); resolvers[base]({ text: "# stale\n" });
expect(screen.queryByTestId("skill-md-preview")).not.toBeInTheDocument(); expect(viewer).not.toHaveTextContent("stale");
resolvers[1]({ text: "# newest\n" }); resolvers[base + 1]({ text: "# newest\n" });
expect(await screen.findByTestId("skill-md-preview")).toHaveTextContent( await waitFor(() => expect(viewer).toHaveTextContent("newest"));
"newest",
);
}); });
it("rejects an older skills/get even when it resolves FIRST", async () => { it("rejects an older skills/get even when it resolves FIRST", async () => {
@ -849,12 +893,111 @@ describe("SkillsScreen", () => {
).toBeInTheDocument(); ).toBeInTheDocument();
}); });
it("shows the SKILL.md preview on demand", async () => { it("shows the skill's own SKILL.md as soon as it is selected", async () => {
// No button to press (#2263): the viewer opens on the skill's own file, so
// selecting it is the whole interaction.
const user = userEvent.setup(); const user = userEvent.setup();
renderWithMantine(<ControlledSkillsScreen />); renderWithMantine(<ControlledSkillsScreen />);
await user.click(screen.getByText("data-analysis")); await user.click(screen.getByText("data-analysis"));
await user.click(screen.getByRole("button", { name: /View SKILL.md/ })); const viewer = screen.getByTestId("skill-resource-viewer");
expect(await screen.findByTestId("skill-md-preview")).toBeInTheDocument(); await waitFor(() => expect(viewer).toHaveTextContent("data-analysis"));
expect(
screen.queryByRole("button", { name: /View SKILL.md/ }),
).not.toBeInTheDocument();
});
it("heads the viewer with the displayed file, not the section's purpose", async () => {
// The heading is static so it does not change shape as the file changes;
// the file name sits beside it (#2263).
const user = userEvent.setup();
renderWithMantine(<ControlledSkillsScreen />);
await user.click(screen.getByText("data-analysis"));
// The heading lives on the section's control (it is the collapsible
// section's own header), so it is queried at screen level rather than
// inside the panel.
const control = within(
screen.getByRole("button", { name: /Skill Resource/ }),
);
expect(control.getByText("Skill Resource")).toBeInTheDocument();
expect(control.getByText("SKILL.md")).toBeInTheDocument();
});
it("swaps the displayed file when a manifest URI is clicked", async () => {
const user = userEvent.setup();
renderWithMantine(<ControlledSkillsScreen />);
await user.click(screen.getByText("data-analysis"));
const viewer = screen.getByTestId("skill-resource-viewer");
await waitFor(() => expect(viewer).toHaveTextContent("data-analysis"));
await user.click(
screen.getByRole("button", {
name: "skill://data-analysis/reference.md",
}),
);
await waitFor(() => expect(viewer).toHaveTextContent("Column rules"));
// The section header follows the file, and the previous contents are gone.
expect(
within(screen.getByRole("button", { name: /Skill Resource/ })).getByText(
"reference.md",
),
).toBeInTheDocument();
expect(viewer).not.toHaveTextContent("data-analysis");
});
it("shows the frontmatter of the file on display, and hides the section when it has none", async () => {
// Both halves come from one split (#2263), so the section can never show
// one file's frontmatter beside another file's body — and the viewer never
// repeats what the section is already showing.
const user = userEvent.setup();
const onReadSkillFile = vi.fn(async (uri: string) =>
uri.endsWith("reference.md")
? { text: "# Ref\n\nNo frontmatter here.\n" }
: { text: "---\nname: data-analysis\n---\n\n# The body\n" },
);
renderWithMantine(
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
);
await user.click(screen.getByText("data-analysis"));
const viewer = screen.getByTestId("skill-resource-viewer");
await waitFor(() => expect(viewer).toHaveTextContent("The body"));
// Shown once, in its own section — not again in the viewer.
expect(
screen.getByRole("button", { name: /Frontmatter/ }),
).toBeInTheDocument();
expect(viewer).not.toHaveTextContent("name: data-analysis");
// reference.md has no frontmatter, so the section goes away entirely
// rather than lingering with SKILL.md's fields.
await user.click(
screen.getByRole("button", {
name: "skill://data-analysis/reference.md",
}),
);
await waitFor(() =>
expect(viewer).toHaveTextContent("No frontmatter here"),
);
expect(
screen.queryByRole("button", { name: /Frontmatter/ }),
).not.toBeInTheDocument();
});
it("marks the row whose file the viewer is showing", async () => {
const user = userEvent.setup();
renderWithMantine(<ControlledSkillsScreen />);
await user.click(screen.getByText("data-analysis"));
const self = screen.getByRole("button", {
name: "skill://data-analysis/SKILL.md",
});
const other = screen.getByRole("button", {
name: "skill://data-analysis/reference.md",
});
// The skill's own file is what the viewer opens on, so its row is current.
expect(self).toHaveAttribute("aria-current", "true");
expect(other).not.toHaveAttribute("aria-current");
await user.click(other);
await waitFor(() => expect(other).toHaveAttribute("aria-current", "true"));
expect(self).not.toHaveAttribute("aria-current");
}); });
it("reports a failed SKILL.md read", async () => { it("reports a failed SKILL.md read", async () => {
@ -864,10 +1007,10 @@ describe("SkillsScreen", () => {
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />, <ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
); );
await user.click(screen.getByText("data-analysis")); await user.click(screen.getByText("data-analysis"));
await user.click(screen.getByRole("button", { name: /View SKILL.md/ }));
expect( expect(
await screen.findByText("Could not read SKILL.md"), await screen.findByText("Could not read this resource"),
).toBeInTheDocument(); ).toBeInTheDocument();
expect(screen.getByText("gone")).toBeInTheDocument();
}); });
it("wraps a non-Error SKILL.md rejection", async () => { it("wraps a non-Error SKILL.md rejection", async () => {
@ -877,10 +1020,36 @@ describe("SkillsScreen", () => {
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />, <ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
); );
await user.click(screen.getByText("data-analysis")); await user.click(screen.getByText("data-analysis"));
await user.click(screen.getByRole("button", { name: /View SKILL.md/ }));
expect(await screen.findByText("bare")).toBeInTheDocument(); expect(await screen.findByText("bare")).toBeInTheDocument();
}); });
it("keeps the three sections independently collapsible", async () => {
const user = userEvent.setup();
renderWithMantine(<ControlledSkillsScreen />);
await user.click(screen.getByText("data-analysis"));
// Conformance and Resources open by default; Frontmatter is closed.
expect(screen.getByRole("button", { name: /Conformance/ })).toHaveAttribute(
"aria-expanded",
"true",
);
expect(screen.getByRole("button", { name: /Frontmatter/ })).toHaveAttribute(
"aria-expanded",
"false",
);
// Collapsing one leaves the others alone — `multiple`, not a single-open
// accordion.
await user.click(screen.getByRole("button", { name: /Conformance/ }));
expect(screen.getByRole("button", { name: /Conformance/ })).toHaveAttribute(
"aria-expanded",
"false",
);
expect(screen.getByRole("button", { name: /Resources/ })).toHaveAttribute(
"aria-expanded",
"true",
);
});
it("drops verification results when the selection changes", async () => { it("drops verification results when the selection changes", async () => {
const user = userEvent.setup(); const user = userEvent.setup();
renderWithMantine(<ControlledSkillsScreen />); renderWithMantine(<ControlledSkillsScreen />);
@ -895,14 +1064,27 @@ describe("SkillsScreen", () => {
expect(screen.getAllByText("—")).toHaveLength(2); expect(screen.getAllByText("—")).toHaveLength(2);
}); });
it("drops the SKILL.md preview when the selection changes", async () => { it("re-points the viewer at the newly selected skill's own file", async () => {
const user = userEvent.setup(); const user = userEvent.setup();
renderWithMantine(<ControlledSkillsScreen />); const onReadSkillFile = vi.fn(async (uri: string) => ({
text: `contents of ${uri}\n`,
}));
renderWithMantine(
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
);
await user.click(screen.getByText("data-analysis")); await user.click(screen.getByText("data-analysis"));
await user.click(screen.getByRole("button", { name: /View SKILL.md/ })); const viewer = screen.getByTestId("skill-resource-viewer");
expect(await screen.findByTestId("skill-md-preview")).toBeInTheDocument(); await waitFor(() =>
expect(viewer).toHaveTextContent("contents of skill://data-analysis"),
);
// 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")); await user.click(screen.getByText("tampered"));
expect(screen.queryByTestId("skill-md-preview")).not.toBeInTheDocument(); await waitFor(() =>
expect(viewer).toHaveTextContent("contents of skill://tampered/SKILL.md"),
);
expect(viewer).not.toHaveTextContent("contents of skill://data-analysis");
}); });
it("renders an em dash for a manifest entry with no size or digest", async () => { it("renders an em dash for a manifest entry with no size or digest", async () => {
@ -1032,10 +1214,15 @@ describe("SkillsScreen", () => {
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />, <ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
); );
await user.click(screen.getByText("data-analysis")); await user.click(screen.getByText("data-analysis"));
await user.click(screen.getByRole("button", { name: /View SKILL.md/ })); // `release` now holds the resolver for the SECOND skill's auto-read; the
// first skill's is stranded, which is the point — resolving the older one
// must not publish into the newer selection.
const stale = release;
await user.click(screen.getByText("tampered")); await user.click(screen.getByText("tampered"));
release?.({ text: SELF_TEXT }); stale?.({ text: "# from the abandoned skill\n" });
expect(screen.queryByTestId("skill-md-preview")).not.toBeInTheDocument(); expect(screen.getByTestId("skill-resource-viewer")).not.toHaveTextContent(
"abandoned",
);
}); });
it("discards a failed SKILL.md read that resolves after the selection moved on", async () => { it("discards a failed SKILL.md read that resolves after the selection moved on", async () => {
@ -1051,9 +1238,10 @@ describe("SkillsScreen", () => {
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />, <ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
); );
await user.click(screen.getByText("data-analysis")); await user.click(screen.getByText("data-analysis"));
await user.click(screen.getByRole("button", { name: /View SKILL.md/ })); // The abandoned skill's own read, stranded by the selection change below.
const stale = fail;
await user.click(screen.getByText("tampered")); await user.click(screen.getByText("tampered"));
fail?.(new Error("too late")); stale?.(new Error("too late"));
expect(screen.queryByText("too late")).not.toBeInTheDocument(); expect(screen.queryByText("too late")).not.toBeInTheDocument();
}); });
}); });

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

@ -1,5 +1,6 @@
import { useCallback, useMemo, useRef, useState } from "react"; import { useCallback, useEffect, useMemo, useRef, useState } from "react";
import { import {
Accordion,
Alert, Alert,
Badge, Badge,
Button, Button,
@ -8,6 +9,7 @@ import {
Flex, Flex,
Group, Group,
NavLink, NavLink,
Paper,
ScrollArea, ScrollArea,
Stack, Stack,
Table, Table,
@ -15,6 +17,7 @@ import {
TextInput, TextInput,
} from "@mantine/core"; } from "@mantine/core";
import { MdRefresh, MdSearch, MdVerifiedUser } from "react-icons/md"; import { MdRefresh, MdSearch, MdVerifiedUser } from "react-icons/md";
import { RiArrowRightSLine } from "react-icons/ri";
import type { import type {
SkillEntry, SkillEntry,
SkillResource, SkillResource,
@ -36,6 +39,7 @@ import {
skillFileBytes, skillFileBytes,
type SkillFileContents, type SkillFileContents,
} from "../../../utils/skillFileBytes"; } from "../../../utils/skillFileBytes";
import { splitSkillFile } from "../../../utils/splitSkillFile";
/** /**
* How many skill files are read at once by "Verify all". A conforming manifest * How many skill files are read at once by "Verify all". A conforming manifest
@ -76,14 +80,21 @@ interface VerificationState {
} }
/** /**
* The SKILL.md preview, plus the manifest it belongs to (`null` as above) and * The resource on display in the viewer, plus the manifest it belongs to
* the click that produced it. The manifest key cannot order two reads issued * (`null` as above) and the request that produced it. The manifest key cannot
* for the SAME manifest, so without `attempt` a double click whose older read * order two reads issued for the SAME manifest, so without `attempt` a second
* finishes last would replace the newer preview. * click whose older read finishes last would replace the newer contents.
*
* `uri` is claimed at request time rather than on settle, so the heading names
* the file being fetched while it is still in flight — a viewer that keeps
* announcing the previous file until the bytes land is reporting the wrong
* thing for exactly as long as the read takes.
*/ */
interface PreviewState { interface PreviewState {
key: string | null; key: string | null;
attempt?: number; attempt?: number;
/** The resource this slot is showing (or fetching). */
uri?: string;
contents?: SkillFileContents; contents?: SkillFileContents;
message?: string; message?: string;
} }
@ -177,16 +188,31 @@ const DetailCard = Card.withProps({
h: "100%", h: "100%",
}); });
const DetailScroll = ScrollArea.withProps({ // The detail pane is a flex column that does NOT scroll as a whole (#2263).
// The disclosure accordion inside it owns the height: each open section scrolls
// within its own share, so nothing scrolls until a section's content overflows
// the space it was given.
const DetailColumn = Stack.withProps({
gap: "md",
flex: 1,
mih: 0,
});
// The transient `skills/get` verdict sits between the sections and the viewer,
// so it is the one part of the column that may need to scroll on its own.
const FetchResultScroll = ScrollArea.withProps({
type: "auto", type: "auto",
scrollbars: "y", scrollbars: "y",
offsetScrollbars: true, offsetScrollbars: true,
h: "100%", flex: "0 1 auto",
mih: 0,
}); });
// Every constant below carries LAYOUT only; the typographic treatment each one
// wants (weight, size, colour, monospace face) is a `ThemeText` variant, since
// flat CSS properties belong in the theme rather than at the call site.
const EmptyState = Text.withProps({ const EmptyState = Text.withProps({
c: "dimmed", variant: "emptyState",
ta: "center",
py: "xl", py: "xl",
}); });
@ -217,39 +243,55 @@ const RefreshButton = Button.withProps({
leftSection: <MdRefresh aria-hidden size={14} />, leftSection: <MdRefresh aria-hidden size={14} />,
}); });
// The shield is a claim about integrity, so it belongs only on the controls
// that actually check it — "Verify all" and the per-row "Verify". `skills/get`
// re-fetches an entry and compares it against the listing; that is a
// consistency check, not a digest verification, and the icon would overstate
// what the button does.
const VerifyButton = Button.withProps({ const VerifyButton = Button.withProps({
variant: "light", variant: "light",
size: "compact-sm", size: "compact-sm",
leftSection: <MdVerifiedUser aria-hidden size={14} />, leftSection: <MdVerifiedUser aria-hidden size={14} />,
}); });
const FetchButton = Button.withProps({
variant: "light",
size: "compact-sm",
});
// A `Text` renders a `<p>`, so a section heading must never *wrap* the count // A `Text` renders a `<p>`, so a section heading must never *wrap* the count
// badge beside it — a `<div>` inside a `<p>` is invalid HTML that React reports // badge beside it — a `<div>` inside a `<p>` is invalid HTML that React reports
// as a hydration error and the Storybook run fails on. Heading and badge sit // as a hydration error and the Storybook run fails on. Heading and badge sit
// side by side in an `InlineRow` instead. // side by side in an `InlineRow` instead.
const SectionHeading = Text.withProps({ const SectionHeading = Text.withProps({
fw: 600, variant: "sectionHeading",
size: "sm",
}); });
const MonoCaption = Text.withProps({ const MonoCaption = Text.withProps({
size: "xs", variant: "monoCaption",
c: "dimmed",
ff: "monospace",
});
const DetailStack = Stack.withProps({
gap: "md",
}); });
const IssueStack = Stack.withProps({ const IssueStack = Stack.withProps({
gap: "xs", gap: "xs",
}); });
// The frontmatter JSON has no surface of its own — the editor renders straight
// onto the panel background, so it reads as loose text rather than as a block.
// A bordered Paper gives it the same framed treatment the manifest table gets
// from `withTableBorder`.
const FramedContent = Paper.withProps({
withBorder: true,
radius: "sm",
p: "xs",
});
const ManifestTable = Table.withProps({ const ManifestTable = Table.withProps({
variant: "manifest",
striped: true, striped: true,
withTableBorder: true, withTableBorder: true,
fz: "xs", // Rows react to the pointer because each one is now clickable — its URI cell
// swaps the file in the viewer below (#2263).
highlightOnHover: true,
verticalSpacing: "xs", verticalSpacing: "xs",
}); });
@ -270,12 +312,87 @@ const RowVerifyButton = Button.withProps({
size: "compact-xs", size: "compact-xs",
}); });
// The URI cell is a control, not a label: clicking it puts that file in the
// viewer. Full width of its column with the URI left-aligned, so the column
// still reads as a column of URIs rather than a column of centred buttons —
// the whole cell is the target, which is what makes a long list of files
// comfortable to click through.
const ResourceUriButton = Button.withProps({
size: "compact-xs",
fullWidth: true,
// Mantine's own alignment prop, not a style override: it drives the button's
// `inner` justify-content, which is what keeps a long URI reading as the
// start of a line rather than a centred label.
justify: "flex-start",
});
const SkillTitle = Text.withProps({ const SkillTitle = Text.withProps({
fw: 600, variant: "skillTitle",
size: "lg", // A Mantine behaviour prop, not a style: it swaps the element for a
// single-line truncating one rather than setting a CSS property.
truncate: true, truncate: true,
}); });
/**
* Per-section flex for the disclosure accordion, the same shape
* `ResourceControls` uses (#1462): an open section shrinks in proportion to how
* much it holds, so a long one gives up space before a short one has to scroll,
* and `flex-grow: 0` means nothing expands until the content actually
* overflows. A closed section stays at its header height.
*/
function sectionFlex(open: boolean, count: number): string {
return open && count > 0 ? `0 ${count} auto` : "0 0 auto";
}
/**
* The file viewer's flex, which is deliberately NOT `sectionFlex`.
*
* The basis is **`0`, not `auto`**, and that is the whole point. `auto` makes a
* flex item's basis its content height, and this panel's content is a rendered
* document — 1,795px for the `data-analysis` fixture. That basis joins the sum
* the container distributes, so the column becomes wildly over-constrained and
* the shrink factors crush the *other* sections: measured on the fixture,
* Conformance collapsed to a 2px panel and Resources to zero height, their
* contents spilling over the headers below. Collapsing this section removed the
* giant basis and everything laid out correctly, which is exactly why the mess
* appeared on collapse-then-reopen.
*
* With a basis of `0` the viewer contributes nothing to that sum and simply
* takes the space the other sections leave, which is what "fills the remainder"
* has to mean here.
*/
function viewerFlex(open: boolean): string {
return open ? "1 1 0" : "0 0 auto";
}
/**
* A floor for an open section, so shrinking can never take one below its own
* header.
*
* The `disclosure` CSS sets `min-height: 0` on an active item — necessary for
* its panel to scroll rather than overflow, but it also permits the collapse to
* nothing described above. A Mantine `mih` prop is an inline style and so wins
* over that rule, bounding the shrink without touching the shared stylesheet.
* The value leaves the control plus a usable sliver of panel; a section pushed
* to it scrolls its own content.
*/
const OPEN_SECTION_MIN_HEIGHT = 96;
/**
* Colour for the Conformance count badge, which summarises a whole finding list
* rather than one finding.
*
* Green means the entry is clean, so it must not be shown for an entry that has
* warnings: a `dynamic-resources` or `size-limit-exceeded` finding is something
* the reader is meant to notice, and a green badge is read as "nothing to see"
* — the badge would be actively hiding the only signal the section carries.
* Yellow matches the warnings' own alerts (`issueColor`).
*/
function summaryColor(errorCount: number, warningCount: number): string {
if (errorCount > 0) return "red";
return warningCount > 0 ? "yellow" : "green";
}
/** Colour token for a finding's severity — errors read as failures. */ /** Colour token for a finding's severity — errors read as failures. */
function issueColor(issue: SkillIssue): string { function issueColor(issue: SkillIssue): string {
return issue.severity === "error" ? "red" : "yellow"; return issue.severity === "error" ? "red" : "yellow";
@ -316,6 +433,22 @@ function isStale(
return held.attempt !== undefined && held.attempt > attempt; return held.attempt !== undefined && held.attempt > attempt;
} }
/**
* The file name a resource URI ends in, for the viewer heading.
*
* Deliberately string surgery rather than `new URL(...)`: a skill URI's scheme
* is not constrained by SEP-2640 (`skill://` is only a SHOULD), so a
* domain-native scheme this app has never seen can reach here, and a heading
* that throws would break the one region of the pane that has to keep working
* for a non-conforming server. Falls back to the whole URI when there is no
* trailing segment to take.
*/
function resourceFileName(uri: string): string {
const withoutQuery = uri.split(/[?#]/)[0];
const last = withoutQuery.slice(withoutQuery.lastIndexOf("/") + 1);
return last || uri;
}
/** `sha256:abcd…wxyz`, so a long digest stays readable in a table cell. */ /** `sha256:abcd…wxyz`, so a long digest stays readable in a table cell. */
function shortDigest(digest: string | undefined): string { function shortDigest(digest: string | undefined): string {
if (!digest) return "—"; if (!digest) return "—";
@ -375,6 +508,16 @@ export function SkillsScreen({
const [batches, setBatches] = useState<ReadonlyMap<string, number>>( const [batches, setBatches] = useState<ReadonlyMap<string, number>>(
() => new Map(), () => new Map(),
); );
// Which of the four collapsible sections are open. A view preference, so it
// is deliberately NOT reset by the manifest-change invalidation below — a
// user who collapsed the frontmatter wants it collapsed on the next skill
// too. Conformance, Resources and the file viewer start open because they are
// what the screen exists to show; the frontmatter is reference material.
const [openSections, setOpenSections] = useState<string[]>([
"conformance",
"resources",
"resource",
]);
// Monotonic attempt token, shared by every on-demand action here: a manifest // Monotonic attempt token, shared by every on-demand action here: a manifest
// row's verification, the SKILL.md preview, and the `skills/get` fetch. One // row's verification, the SKILL.md preview, and the `skills/get` fetch. One
// counter rather than three because it only has to be *increasing*, and each // counter rather than three because it only has to be *increasing*, and each
@ -522,33 +665,68 @@ export function SkillsScreen({
); );
}, [manifest, manifestKey, verifyRow]); }, [manifest, manifestKey, verifyRow]);
const showSkillMd = useCallback(() => { /**
if (!selected) return; * Put one of the skill's files in the viewer. Driven both by the effect that
const key = manifestKey; * loads `SKILL.md` on selection and by the manifest's URI buttons.
*
* The manifest `key` is a parameter rather than a closure read, so the
* selection effect below can pass the manifest it is loading *for* instead of
* whichever one happened to be current when this callback was created.
*/
const showResource = useCallback(
(uri: string, key: string) => {
const attempt = (nextAttempt.current += 1); const attempt = (nextAttempt.current += 1);
// A click handler cannot await, and this chain terminates in its own // A click handler cannot await, and this chain terminates in its own
// `catch` that surfaces the message in the preview slot. Both arms go // `catch` that surfaces the message in the viewer. Both arms go through
// through `writePreview`, which drops a result whose manifest has been // `writePreview`, which drops a result whose manifest has been
// invalidated OR whose click has been superseded. // invalidated OR whose request has been superseded.
const writePreview = (next: Omit<PreviewState, "key" | "attempt">) => const writePreview = (next: Omit<PreviewState, "key" | "attempt">) =>
setPreviewState((prev) => setPreviewState((prev) =>
isStale(prev, key, attempt) ? prev : { key, attempt, ...next }, isStale(prev, key, attempt) ? prev : { key, attempt, ...next },
); );
// Claimed BEFORE the request goes out, the way `verifyRow` claims its row. // Claimed BEFORE the request goes out, the way `verifyRow` claims its
// Recording the attempt only on settle leaves a window where an older // row. Recording the attempt only on settle leaves a window where an
// request that happens to resolve first is still considered current, and // older request that happens to resolve first is still considered
// publishes its contents while a newer one is in flight. Clearing the // current, and publishes its contents while a newer one is in flight.
// previous result at the same time also means the pane doesn't keep // The `uri` rides along on every write so the heading names the file
// showing the old file while the new read is running. // being fetched while the read is still running, rather than continuing
writePreview({}); // to announce the previous one for as long as the read takes.
void onReadSkillFile(selected.uri) writePreview({ uri });
.then((contents) => writePreview({ contents })) void onReadSkillFile(uri)
.then((contents) => writePreview({ uri, contents }))
.catch((err: unknown) => { .catch((err: unknown) => {
writePreview({ writePreview({
uri,
message: err instanceof Error ? err.message : String(err), message: err instanceof Error ? err.message : String(err),
}); });
}); });
}, [manifestKey, onReadSkillFile, selected]); },
[onReadSkillFile],
);
// The skill's own SKILL.md is what the viewer opens on, so it is read when
// the selection changes rather than behind a button (#2263).
//
// An EFFECT, deliberately, and it is the legitimate kind: `useValueChange`
// above already dropped the previous skill's results during render, so there
// is no stale frame to fix here — this is the separate act of talking to an
// external system, which is the one thing an effect is still for. It could
// not live in the `useValueChange` callback anyway; that runs during render
// and must stay `setState`-only.
//
// SEP-2640 is explicit that a `resources/read` of a SKILL.md is NOT a load
// and confers no standing, so reading it on selection claims nothing on the
// user's behalf.
// A primitive, so the effect below depends on the URI rather than on the
// entry object: `selected` is re-derived whenever `skills` is replaced, and
// depending on it would re-read the file every time the list was refetched
// with identical content.
const selectedUri = selected?.uri;
useEffect(() => {
if (selectedUri === undefined) return;
showResource(selectedUri, manifestKey);
}, [manifestKey, selectedUri, showResource]);
const fetchEntry = useCallback(() => { const fetchEntry = useCallback(() => {
if (!selected) return; if (!selected) return;
@ -607,10 +785,29 @@ export function SkillsScreen({
: "differs"; : "differs";
const batchRunning = batches.has(manifestKey); const batchRunning = batches.has(manifestKey);
const preview = const previewCurrent = previewState.key === manifestKey;
previewState.key === manifestKey ? previewState.contents : undefined; const preview = previewCurrent ? previewState.contents : undefined;
const previewError = const previewError = previewCurrent ? previewState.message : undefined;
previewState.key === manifestKey ? previewState.message : undefined; // The file the viewer is showing (or fetching). Falls back to the skill's own
// URI so the heading is never blank on the very first frame, before the
// selection effect has claimed a slot.
const previewUri =
(previewCurrent ? previewState.uri : undefined) ?? selectedUri;
// The displayed file, split once into frontmatter and body. BOTH halves of
// the pane read from this single split, which is what keeps them honest: the
// Frontmatter section shows the frontmatter of the file the viewer is
// showing, and a file that has none renders no section at all rather than
// leaving the previous file's on screen.
//
// Only the text form can be split; a base64 `blob` is served through
// untouched.
const previewParts = useMemo(
() =>
typeof preview?.text === "string"
? splitSkillFile(preview.text)
: undefined,
[preview],
);
const errorCount = issues.filter((i) => i.severity === "error").length; const errorCount = issues.filter((i) => i.severity === "error").length;
const warningCount = issues.length - errorCount; const warningCount = issues.length - errorCount;
@ -690,24 +887,76 @@ export function SkillsScreen({
{!selected ? ( {!selected ? (
<EmptyState>Select a skill to view details</EmptyState> <EmptyState>Select a skill to view details</EmptyState>
) : ( ) : (
<DetailScroll> <DetailColumn data-testid="skill-detail">
<DetailStack data-testid="skill-detail"> <SectionControlsRow>
<Stack gap={4}> <Stack gap={4}>
<SkillTitle>{skillDisplayName(selected)}</SkillTitle> <SkillTitle>{skillDisplayName(selected)}</SkillTitle>
<MonoCaption>{selected.uri}</MonoCaption> <MonoCaption>{selected.uri}</MonoCaption>
</Stack> </Stack>
{/* Both actions act on the whole skill, so they live on the
pane's header rather than inside a section — and a button
inside an `Accordion.Control` would toggle that section on its
way to firing. */}
<InlineRow>
<FetchButton onClick={fetchEntry}>
Fetch with skills/get
</FetchButton>
<VerifyButton
onClick={verifyAll}
disabled={manifest.length === 0 || batchRunning}
loading={batchRunning}
>
Verify all
</VerifyButton>
</InlineRow>
</SectionControlsRow>
{selected.frontmatter.description && ( {selected.frontmatter.description && (
<Text size="sm">{selected.frontmatter.description}</Text> <Text size="sm">{selected.frontmatter.description}</Text>
)} )}
<Stack gap="xs"> {/* Inline, not a `.withProps()` subcomponent: `Accordion` is a
compound, `multiple`-discriminated generic, and baking props
into it loses the JSX call signature (see AGENTS.md).
`variant="disclosure"` is the app's existing full-height
sections mechanism (#1462, also used by `ResourceControls`): the
headers stay pinned and each open panel scrolls within its own
share of the space, which is exactly what keeps this pane from
scrolling as one column. */}
<Accordion
multiple
variant="disclosure"
chevron={<RiArrowRightSLine />}
flex={1}
mih={0}
// Mantine's panel height animation fights the flex sizing above;
// the chevron still rotates smoothly via App.css (#1462).
transitionDuration={0}
value={openSections}
onChange={setOpenSections}
>
<Accordion.Item
value="conformance"
flex={sectionFlex(
openSections.includes("conformance"),
Math.max(issues.length, 1),
)}
mih={
openSections.includes("conformance")
? OPEN_SECTION_MIN_HEIGHT
: undefined
}
>
<Accordion.Control>
<InlineRow> <InlineRow>
<SectionHeading>Conformance</SectionHeading> <SectionHeading>Conformance</SectionHeading>
<CountBadge color={errorCount > 0 ? "red" : "green"}> <CountBadge color={summaryColor(errorCount, warningCount)}>
{errorCount} error(s), {warningCount} warning(s) {errorCount} error(s), {warningCount} warning(s)
</CountBadge> </CountBadge>
</InlineRow> </InlineRow>
</Accordion.Control>
<Accordion.Panel>
{issues.length === 0 ? ( {issues.length === 0 ? (
<Alert color="green" title="Conforms"> <Alert color="green" title="Conforms">
No structural issues found in this entry. No structural issues found in this entry.
@ -716,12 +965,13 @@ export function SkillsScreen({
<IssueStack data-testid="skill-issues"> <IssueStack data-testid="skill-issues">
{issues.map((issue, index) => ( {issues.map((issue, index) => (
<Alert <Alert
// The index is load-bearing, not decoration: a manifest // The index is load-bearing, not decoration: a
// repeating one URI three times yields three // manifest repeating one URI three times yields
// `duplicate-resource` findings with identical code and // three `duplicate-resource` findings with
// URI, and a key built from those alone would make // identical code and URI, and a key built from
// React drop the extras — hiding findings in exactly // those alone would make React drop the extras —
// the malformed input this view exists to inspect. // hiding findings in exactly the malformed input
// this view exists to inspect.
key={`${index}:${issue.code}:${issue.resourceUri ?? ""}`} key={`${index}:${issue.code}:${issue.resourceUri ?? ""}`}
color={issueColor(issue)} color={issueColor(issue)}
title={issue.code} title={issue.code}
@ -736,10 +986,22 @@ export function SkillsScreen({
))} ))}
</IssueStack> </IssueStack>
)} )}
</Stack> </Accordion.Panel>
</Accordion.Item>
<Stack gap="xs"> <Accordion.Item
<SectionControlsRow> value="resources"
flex={sectionFlex(
openSections.includes("resources"),
Math.max(manifest.length, 1),
)}
mih={
openSections.includes("resources")
? OPEN_SECTION_MIN_HEIGHT
: undefined
}
>
<Accordion.Control>
<InlineRow> <InlineRow>
<SectionHeading>Resources</SectionHeading> <SectionHeading>Resources</SectionHeading>
<CountBadge> <CountBadge>
@ -747,28 +1009,15 @@ export function SkillsScreen({
bytes bytes
</CountBadge> </CountBadge>
</InlineRow> </InlineRow>
<InlineRow> </Accordion.Control>
<VerifyButton onClick={fetchEntry}> <Accordion.Panel>
Fetch with skills/get <Stack gap="xs">
</VerifyButton>
<VerifyButton onClick={showSkillMd}>
View SKILL.md
</VerifyButton>
<VerifyButton
onClick={verifyAll}
disabled={manifest.length === 0 || batchRunning}
loading={batchRunning}
>
Verify all
</VerifyButton>
</InlineRow>
</SectionControlsRow>
{selected.resources === DYNAMIC_RESOURCES ? ( {selected.resources === DYNAMIC_RESOURCES ? (
<Alert color="yellow" title="Dynamic resources"> <Alert color="yellow" title="Dynamic resources">
This skill declares{" "} This skill declares{" "}
<Code>resources: &quot;dynamic&quot;</Code> — its files are <Code>resources: &quot;dynamic&quot;</Code> — its files
generated, so no manifest is advertised and integrity cannot are generated, so no manifest is advertised and
be verified. integrity cannot be verified.
</Alert> </Alert>
) : ( ) : (
<ManifestTable data-testid="skill-manifest"> <ManifestTable data-testid="skill-manifest">
@ -789,31 +1038,58 @@ export function SkillsScreen({
: state?.status === "error" : state?.status === "error"
? "red" ? "red"
: "gray"; : "gray";
// Compared by identity for the same reason every
// other URI comparison here is: a server that
// canonicalizes an escape is naming the same
// file, and the row the user just clicked must
// not read as unselected because of a spelling.
const showing =
previewUri !== undefined &&
skillUriIdentity(resource.uri) ===
skillUriIdentity(previewUri);
return ( return (
// Index-keyed for the same reason the verdicts are: // Index-keyed for the same reason the verdicts
// a duplicated URI is a case this screen reports, so // are: a duplicated URI is a case this screen
// it must not also collide two rows into one. // reports, so it must not also collide two rows
// into one.
<Table.Tr key={index}> <Table.Tr key={index}>
<Table.Td>{resource.uri}</Table.Td> <Table.Td>
<ResourceUriButton
variant={showing ? "light" : "subtle"}
aria-current={showing ? "true" : undefined}
onClick={() =>
showResource(resource.uri, manifestKey)
}
>
{resource.uri}
</ResourceUriButton>
</Table.Td>
<Table.Td>{resource.size ?? "—"}</Table.Td> <Table.Td>{resource.size ?? "—"}</Table.Td>
<Table.Td>{shortDigest(resource.digest)}</Table.Td> <Table.Td>
{shortDigest(resource.digest)}
</Table.Td>
<Table.Td> <Table.Td>
<InlineRow> <InlineRow>
<CountBadge color={color}> <CountBadge color={color}>
{verificationLabel(state)} {verificationLabel(state)}
</CountBadge> </CountBadge>
<RowVerifyButton <RowVerifyButton
// Every row's button reads "Verify", so the // Every row's button reads "Verify", so
// visible text alone gives a screen-reader // the visible text alone gives a screen-
// user no way to tell which file each one // reader user no way to tell which file
// checks; the URI cell is in the same row but // each one checks; the URI cell is in
// is not programmatically associated with it. // the same row but is not
// programmatically associated with it.
aria-label={`Verify ${resource.uri}`} aria-label={`Verify ${resource.uri}`}
// A click handler cannot await, and // A click handler cannot await, and
// `verifyRow` owns its own failures — it // `verifyRow` owns its own failures — it
// records them as this row's state. // records them as this row's state.
onClick={() => onClick={() =>
void verifyRow(index, resource, manifestKey) void verifyRow(
index,
resource,
manifestKey,
)
} }
> >
Verify Verify
@ -831,9 +1107,10 @@ export function SkillsScreen({
if (state?.status === "done") { if (state?.status === "done") {
const result = state.verification; const result = state.verification;
if (result.status !== "mismatch") return null; if (result.status !== "mismatch") return null;
// A size disagreement is caught BEFORE hashing, so it has // A size disagreement is caught BEFORE hashing, so it
// no `actualDigest` — titling it "Digest mismatch" and // has no `actualDigest` — titling it "Digest
// rendering "actual undefined" would hide the real failure. // mismatch" and rendering "actual undefined" would
// hide the real failure.
const sizeFailure = result.actualDigest === undefined; const sizeFailure = result.actualDigest === undefined;
return ( return (
<Alert <Alert
@ -878,7 +1155,103 @@ export function SkillsScreen({
return null; return null;
})} })}
</Stack> </Stack>
</Accordion.Panel>
</Accordion.Item>
{/* Rendered ONLY when the file on display actually carries
frontmatter. A skill's manifest files generally do not, and a
section that lingered would be showing the previously selected
file's fields against the current file's name. */}
{previewParts?.frontmatter !== undefined && (
<Accordion.Item
value="frontmatter"
// Weight 1: frontmatter is a handful of lines whatever the
// file, so it never needs a share proportional to anything.
flex={sectionFlex(openSections.includes("frontmatter"), 1)}
mih={
openSections.includes("frontmatter")
? OPEN_SECTION_MIN_HEIGHT
: undefined
}
>
<Accordion.Control>
<SectionHeading>Frontmatter</SectionHeading>
</Accordion.Control>
<Accordion.Panel>
<FramedContent>
{/* The raw YAML the server served, not a re-serialised
object: this app carries no YAML parser, and for a
conformance tool the bytes on the wire are the more
useful answer anyway. */}
<ContentViewer
block={{
type: "text",
text: previewParts.frontmatter,
}}
mimeType="text/yaml"
copyable
/>
</FramedContent>
</Accordion.Panel>
</Accordion.Item>
)}
<Accordion.Item
value="resource"
// The one section that GROWS, and the one whose basis must be
// zero — see `viewerFlex` for what a content-sized basis did to
// its siblings.
flex={viewerFlex(openSections.includes("resource"))}
mih={
openSections.includes("resource")
? OPEN_SECTION_MIN_HEIGHT
: undefined
}
>
<Accordion.Control>
<SectionControlsRow>
<SectionHeading>Skill Resource</SectionHeading>
{previewUri !== undefined && (
<MonoCaption>{resourceFileName(previewUri)}</MonoCaption>
)}
</SectionControlsRow>
</Accordion.Control>
<Accordion.Panel data-testid="skill-resource-viewer">
{previewError !== undefined ? (
<Alert color="red" title="Could not read this resource">
{previewError}
</Alert>
) : (
preview && (
/* `contents`, not a text `block`: a server may serve a
skill file as a base64 `blob`, and the block form would
substitute an empty string and paint a blank viewer for
a file it had just read correctly. */
<ContentViewer
contents={
typeof preview.text === "string"
? {
uri: previewUri ?? selected.uri,
// The body half of the same split the
// Frontmatter section reads from.
text: previewParts?.body ?? preview.text,
mimeType: preview.mimeType ?? "text/markdown",
}
: {
uri: previewUri ?? selected.uri,
blob: preview.blob ?? "",
mimeType: preview.mimeType ?? "text/markdown",
}
}
copyable
/>
)
)}
</Accordion.Panel>
</Accordion.Item>
</Accordion>
<FetchResultScroll>
{fetched?.message !== undefined && ( {fetched?.message !== undefined && (
<Alert color="red" title="skills/get failed"> <Alert color="red" title="skills/get failed">
{fetched.message} {fetched.message}
@ -888,6 +1261,7 @@ export function SkillsScreen({
<Alert <Alert
data-testid="skills-get-result" data-testid="skills-get-result"
data-verdict={fetchedVerdict} data-verdict={fetchedVerdict}
mt="sm"
// Red only for a genuine violation — a non-conforming entry, // Red only for a genuine violation — a non-conforming entry,
// or one answering with a different URI, neither of which a // or one answering with a different URI, neither of which a
// fresh snapshot excuses. A conforming entry that merely // fresh snapshot excuses. A conforming entry that merely
@ -932,57 +1306,15 @@ export function SkillsScreen({
text: JSON.stringify(fetched.entry, null, 2), text: JSON.stringify(fetched.entry, null, 2),
}} }}
mimeType="application/json" mimeType="application/json"
jsonLabel="Fetched skill entry"
copyable copyable
/> />
)} )}
</Stack> </Stack>
</Alert> </Alert>
)} )}
</FetchResultScroll>
{previewError && ( </DetailColumn>
<Alert color="red" title="Could not read SKILL.md">
{previewError}
</Alert>
)}
{preview && (
<Stack gap="xs" data-testid="skill-md-preview">
<SectionHeading>SKILL.md</SectionHeading>
{/* `contents`, not a text `block`: a server may serve
SKILL.md as a base64 `blob`, and the block form would
substitute an empty string and paint a blank preview for
a file verification just read correctly. */}
<ContentViewer
contents={
typeof preview.text === "string"
? {
uri: selected.uri,
text: preview.text,
mimeType: preview.mimeType ?? "text/markdown",
}
: {
uri: selected.uri,
blob: preview.blob ?? "",
mimeType: preview.mimeType ?? "text/markdown",
}
}
copyable
/>
</Stack>
)}
<Stack gap="xs">
<SectionHeading>Frontmatter</SectionHeading>
<ContentViewer
block={{
type: "text",
text: JSON.stringify(selected.frontmatter, null, 2),
}}
mimeType="application/json"
copyable
/>
</Stack>
</DetailStack>
</DetailScroll>
)} )}
</DetailCard> </DetailCard>
</ScreenLayout> </ScreenLayout>

14
clients/web/src/theme/Table.ts Normal file
View file

@ -0,0 +1,14 @@
import { Table } from "@mantine/core";
export const ThemeTable = Table.extend({
// The Skills resource manifest (#2263). `striped`, `withTableBorder`,
// `highlightOnHover` and `verticalSpacing` are Mantine's own API and stay
// props at the call site; the font size is a flat CSS property, so it lives
// here rather than as an `fz` prop on the screen's `.withProps()` constant.
styles: (_theme, props) => {
if (props.variant === "manifest") {
return { table: { fontSize: "var(--mantine-font-size-xs)" } };
}
return {};
},
});

36
clients/web/src/theme/Text.ts
View file

@ -42,6 +42,42 @@ export const ThemeText = Text.extend({
// footer's `space-between` Group. Both are grey, single-line, and out of the // footer's `space-between` Group. Both are grey, single-line, and out of the
// text-selection flow. (Superseded the fixed bottom-corner badges of #1639 // text-selection flow. (Superseded the fixed bottom-corner badges of #1639
// now that the footer is a real, full-width AppShell row.) // now that the footer is a real, full-width AppShell row.)
// The four typographic treatments the Skills screen repeats (#2263). They
// live here rather than as `fw`/`size`/`c`/`ff` props on the screen's
// `.withProps()` constants because they are flat CSS properties, which
// AGENTS.md places in the theme; the constants keep only layout.
//
// `sectionHeading` labels a collapsible section; `skillTitle` names the
// selected skill; `monoCaption` is the dimmed monospace line used for URIs
// and digests; `emptyState` is the centred placeholder shown before a
// selection exists.
if (props.variant === "sectionHeading") {
return {
root: { fontWeight: 600, fontSize: "var(--mantine-font-size-sm)" },
};
}
if (props.variant === "skillTitle") {
return {
root: { fontWeight: 600, fontSize: "var(--mantine-font-size-lg)" },
};
}
if (props.variant === "monoCaption") {
return {
root: {
fontSize: "var(--mantine-font-size-xs)",
fontFamily: "var(--mantine-font-family-monospace)",
color: "var(--inspector-text-secondary)",
},
};
}
if (props.variant === "emptyState") {
return {
root: {
color: "var(--inspector-text-secondary)",
textAlign: "center",
},
};
}
if ( if (
props.variant === "versionBadge" || props.variant === "versionBadge" ||
props.variant === "copyrightBadge" props.variant === "copyrightBadge"

1
clients/web/src/theme/index.ts
View file

@ -16,6 +16,7 @@ export { ThemePaper } from "./Paper";
export { ThemeScrollArea, ThemeScrollAreaAutosize } from "./ScrollArea"; export { ThemeScrollArea, ThemeScrollAreaAutosize } from "./ScrollArea";
export { ThemeSelect } from "./Select"; export { ThemeSelect } from "./Select";
export { ThemeSwitch } from "./Switch"; export { ThemeSwitch } from "./Switch";
export { ThemeTable } from "./Table";
export { ThemeText } from "./Text"; export { ThemeText } from "./Text";
export { ThemeTextInput } from "./TextInput"; export { ThemeTextInput } from "./TextInput";
export { ThemeTitle } from "./Title"; export { ThemeTitle } from "./Title";

2
clients/web/src/theme/theme.ts
View file

@ -19,6 +19,7 @@ import {
ThemeScrollAreaAutosize, ThemeScrollAreaAutosize,
ThemeSelect, ThemeSelect,
ThemeSwitch, ThemeSwitch,
ThemeTable,
ThemeText, ThemeText,
ThemeTextInput, ThemeTextInput,
ThemeTitle, ThemeTitle,
@ -201,6 +202,7 @@ export const theme = createTheme({
ScrollAreaAutosize: ThemeScrollAreaAutosize, ScrollAreaAutosize: ThemeScrollAreaAutosize,
Select: ThemeSelect, Select: ThemeSelect,
Switch: ThemeSwitch, Switch: ThemeSwitch,
Table: ThemeTable,
Text: ThemeText, Text: ThemeText,
TextInput: ThemeTextInput, TextInput: ThemeTextInput,
Title: ThemeTitle, Title: ThemeTitle,

66
clients/web/src/utils/splitSkillFile.test.ts Normal file
View file

@ -0,0 +1,66 @@
import { describe, it, expect } from "vitest";
import { splitSkillFile } from "./splitSkillFile";
describe("splitSkillFile", () => {
it("separates a leading frontmatter fence from the body", () => {
expect(splitSkillFile("---\nname: a\n---\n\n# Title\n\nBody\n")).toEqual({
frontmatter: "name: a",
body: "# Title\n\nBody\n",
});
});
it("reports no frontmatter for a file that has none", () => {
expect(splitSkillFile("# Title\n\nBody\n")).toEqual({
body: "# Title\n\nBody\n",
});
});
it("leaves an unterminated opening fence alone rather than eating the file", () => {
// `---` with no closing fence is not frontmatter. Treating it as such would
// truncate the document to nothing, which is far worse than showing it.
const text = "---\nnot really frontmatter\n\n# Title\n";
expect(splitSkillFile(text)).toEqual({ body: text });
});
it("does not treat a horizontal rule mid-file as frontmatter", () => {
const text = "# Title\n\n---\n\nAfter the rule\n";
expect(splitSkillFile(text)).toEqual({ body: text });
});
it("handles CRLF line endings", () => {
expect(splitSkillFile("---\r\nname: a\r\n---\r\n\r\n# Title\r\n")).toEqual({
frontmatter: "name: a",
body: "# Title\r\n",
});
});
it("returns an empty body when the file is nothing but frontmatter", () => {
expect(splitSkillFile("---\nname: a\n---\n")).toEqual({
frontmatter: "name: a",
body: "",
});
});
it("handles a closing fence with no trailing newline", () => {
// The file ends ON the fence, so there is no newline after it to split at.
expect(splitSkillFile("---\nname: a\n---")).toEqual({
frontmatter: "name: a",
body: "",
});
});
it("keeps a body that follows the fence with no blank line", () => {
// The blank line between fence and body is a convention, not a rule —
// stripping unconditionally would eat the first line of a file without one.
expect(splitSkillFile("---\nname: a\n---\nBody\n")).toEqual({
frontmatter: "name: a",
body: "Body\n",
});
});
it("keeps every frontmatter line, not just the first", () => {
expect(
splitSkillFile("---\nname: a\ndescription: b\n---\n\nBody\n"),
).toEqual({ frontmatter: "name: a\ndescription: b", body: "Body\n" });
});
});

55
clients/web/src/utils/splitSkillFile.ts Normal file
View file

@ -0,0 +1,55 @@
/**
* Split a skill file into its YAML frontmatter and its body (#2263).
*
* A pure transform with no I/O and no subsystem of its own, so it lives in
* `utils/` rather than `lib/` — and in its own module rather than in
* `SkillsScreen.tsx`, because a component file that also exports a function
* defeats React Fast Refresh (`react-refresh/only-export-components`).
*/
export interface SkillFileParts {
/**
* The raw YAML between the fences, fences excluded — `undefined` when the
* file has no frontmatter at all. Raw rather than parsed: this app carries no
* YAML parser, and showing the bytes the server actually served is the more
* useful answer for a conformance tool anyway.
*/
frontmatter?: string;
/** Everything after the closing fence, or the whole file when there is none. */
body: string;
}
/**
* Separate a leading YAML frontmatter fence from the rest of a skill file.
*
* The Skills screen renders the two halves in different places — the
* frontmatter in its own collapsible section, the body in the file viewer — and
* deriving both from **one** split is what stops them disagreeing: the section
* can never show one file's frontmatter while the viewer shows another's, and a
* file with no frontmatter cannot leave a stale section on screen.
*
* It also matters for rendering: the markdown renderer has no frontmatter
* support, so an un-split `---\nname: …\n---` is read as a setext heading and
* painted as a title above the document's real one.
*
* Two deliberate conservatisms, because this must never eat content:
*
* - Only a fence at the very **start** of the file counts. A `---` anywhere
* else is a horizontal rule and is left in the body.
* - A file that opens with `---` but never closes the fence is **not**
* frontmatter; the whole file is returned as the body rather than being
* truncated to nothing.
*/
export function splitSkillFile(text: string): SkillFileParts {
if (!/^---[ \t]*\r?\n/.test(text)) return { body: text };
const rest = text.slice(text.indexOf("\n") + 1);
const close = rest.search(/^---[ \t]*\r?$/m);
if (close === -1) return { body: text };
const frontmatter = rest.slice(0, close).replace(/\r?\n$/, "");
const after = rest.slice(close);
const newline = after.indexOf("\n");
if (newline === -1) return { frontmatter, body: "" };
// Drop the blank line conventionally left between the fence and the body, so
// the document does not open with dead space.
return { frontmatter, body: after.slice(newline + 1).replace(/^\r?\n/, "") };
}

67
test-servers/src/skills.ts
View file

@ -126,9 +126,74 @@ const DATA_ANALYSIS_FM: Frontmatter = {
name: "data-analysis", name: "data-analysis",
description: "Analyze a CSV and summarize its columns", description: "Analyze a CSV and summarize its columns",
}; };
// Deliberately the long one. Every other fixture file here is two or three
// lines, which is not enough to exercise the detail pane's resource viewer:
// with a short file the viewer never scrolls, so a regression that let the
// whole pane scroll instead — the thing #2263 fixed — would look identical to
// the fix. One fixture has to be taller than the viewport for that difference
// to be visible at all.
const DATA_ANALYSIS_MD = skillMd( const DATA_ANALYSIS_MD = skillMd(
DATA_ANALYSIS_FM, DATA_ANALYSIS_FM,
"# Data analysis\n\nLoad the CSV, then follow `reference.md` for the column rules.", [
"# Data analysis",
"",
"Load the CSV, then follow `reference.md` for the column rules.",
"",
"## Reading the file",
"",
"Read the file as UTF-8 and sniff the delimiter from the header line rather",
"than assuming a comma: exports from spreadsheet tools frequently use a",
"semicolon, and a mis-sniffed delimiter yields a single column whose name is",
"the entire header, which then reads as a valid — if useless — result.",
"",
"If the first line is not a header, every column name becomes a data value",
"and the row count is off by one. Prefer an explicit `has_header` flag over a",
"heuristic when the caller can supply it.",
"",
"## Typing the columns",
"",
"Infer a column's type from a sample rather than from its first value. A",
"column of integers with one empty cell is still numeric; a column of numbers",
"with a single stray `N/A` is not, and coercing it silently turns a data",
"quality problem into a wrong answer.",
"",
"Treat these as missing: the empty string, `NA`, `N/A`, `null`, and `-`.",
"Anything else that fails to parse is a value, not a gap, and belongs in the",
"report as such.",
"",
"## Summarising",
"",
"Numeric columns get min, max, mean and a null count. Report the null count",
"even when it is zero — its absence is indistinguishable from a column that",
"was skipped, and a reader cannot tell which they are looking at.",
"",
"Text columns get a distinct-value count and the five most common values with",
"their frequencies. Cap the distinct count: a free-text column can have as",
"many distinct values as rows, and enumerating them is not a summary.",
"",
"Date columns get an earliest and a latest. Do not attempt a mean of dates.",
"",
"## Reporting",
"",
"Lead with the shape — rows, columns — then the per-column detail. A reader",
"scanning the top of the report should learn whether the file is what they",
"expected before they read anything else.",
"",
"State the delimiter and the encoding you used. When either was guessed, say",
"that it was guessed: a summary computed from a mis-parsed file is worse than",
"no summary, because it looks the same as a correct one.",
"",
"## Failure modes worth naming",
"",
"A ragged file — rows with differing column counts — is a parse failure, not",
"a row to drop quietly. Report the first offending line number.",
"",
"A file whose every column types as text usually means the delimiter was",
"wrong. Say so rather than reporting fifty text columns as a finding.",
"",
"An empty file is not an error, but a summary of it must say the file was",
"empty rather than returning zeroed statistics that read like real ones.",
].join("\n"),
); );
const DATA_ANALYSIS_REF = const DATA_ANALYSIS_REF =
"# Column rules\n\nNumeric columns get min/max/mean; text columns get a value count.\n"; "# Column rules\n\nNumeric columns get min/max/mean; text columns get a value count.\n";