From e67eb3408edd62e2c4be024fbfe24f0bafa801f4 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 20:55:19 -0400 Subject: [PATCH] feat: refactor the Skills detail pane into collapsible sections (#2263) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB Signed-off-by: cliffhall --- .../SkillsScreen/SkillsScreen.test.tsx | 276 +++++- .../screens/SkillsScreen/SkillsScreen.tsx | 866 ++++++++++++------ clients/web/src/theme/Table.ts | 14 + clients/web/src/theme/Text.ts | 36 + clients/web/src/theme/index.ts | 1 + clients/web/src/theme/theme.ts | 2 + clients/web/src/utils/splitSkillFile.test.ts | 66 ++ clients/web/src/utils/splitSkillFile.ts | 55 ++ test-servers/src/skills.ts | 67 +- 9 files changed, 1071 insertions(+), 312 deletions(-) create mode 100644 clients/web/src/theme/Table.ts create mode 100644 clients/web/src/utils/splitSkillFile.test.ts create mode 100644 clients/web/src/utils/splitSkillFile.ts diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 88df0cbb..93f111d8 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -17,7 +17,10 @@ import { import { EMPTY_SKILLS_UI } from "../screenUiState"; 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"; // 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 @@ -138,6 +141,14 @@ function ControlledSkillsScreen(props: Partial = {}) { ); } +// 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", () => { it("renders the empty state until a skill is selected", () => { renderWithMantine(); @@ -199,6 +210,28 @@ describe("SkillsScreen", () => { 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(); + 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(); + 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 () => { const user = userEvent.setup(); renderWithMantine(); @@ -262,7 +295,10 @@ describe("SkillsScreen", () => { await user.click(screen.getByRole("button", { name: /Verify all/ })); // One alert per file in the manifest — both reads failed. 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 () => { @@ -273,7 +309,9 @@ describe("SkillsScreen", () => { ); await user.click(screen.getByText("data-analysis")); 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 () => { @@ -336,7 +374,7 @@ describe("SkillsScreen", () => { it("renders a base64 SKILL.md preview instead of a blank one", async () => { // `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. const user = userEvent.setup(); const onReadSkillFile = vi.fn().mockResolvedValue({ @@ -347,9 +385,8 @@ describe("SkillsScreen", () => { , ); await user.click(screen.getByText("data-analysis")); - await user.click(screen.getByRole("button", { name: /View SKILL.md/ })); - const preview = await screen.findByTestId("skill-md-preview"); - expect(preview).toHaveTextContent("from a blob"); + const viewer = screen.getByTestId("skill-resource-viewer"); + await waitFor(() => expect(viewer).toHaveTextContent("from a blob")); }); 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")); + // 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", { name: "Verify skill://data-analysis/SKILL.md", }); 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 // 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(); - resolvers[0]({ text: "stale bytes\n" }); + resolvers[base]({ text: "stale bytes\n" }); // Still the newer verdict. expect(await screen.findByText("verified")).toBeInTheDocument(); expect(screen.queryByText("mismatch")).not.toBeInTheDocument(); @@ -608,19 +648,20 @@ describe("SkillsScreen", () => { , ); 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); - expect(resolvers).toHaveLength(2); + expect(resolvers).toHaveLength(base + 2); - resolvers[1]({ text: "# newest\n" }); - expect(await screen.findByTestId("skill-md-preview")).toHaveTextContent( - "newest", - ); - resolvers[0]({ text: "# stale\n" }); - expect(screen.getByTestId("skill-md-preview")).not.toHaveTextContent( - "stale", - ); + const viewer = screen.getByTestId("skill-resource-viewer"); + resolvers[base + 1]({ text: "# newest\n" }); + await waitFor(() => expect(viewer).toHaveTextContent("newest")); + resolvers[base]({ text: "# stale\n" }); + expect(viewer).not.toHaveTextContent("stale"); }); it("keeps the newest skills/get result when two fetches overlap", async () => { @@ -804,18 +845,21 @@ describe("SkillsScreen", () => { , ); 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); - 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. - resolvers[0]({ text: "# stale\n" }); - expect(screen.queryByTestId("skill-md-preview")).not.toBeInTheDocument(); - resolvers[1]({ text: "# newest\n" }); - expect(await screen.findByTestId("skill-md-preview")).toHaveTextContent( - "newest", - ); + resolvers[base]({ text: "# stale\n" }); + expect(viewer).not.toHaveTextContent("stale"); + resolvers[base + 1]({ text: "# newest\n" }); + await waitFor(() => expect(viewer).toHaveTextContent("newest")); }); it("rejects an older skills/get even when it resolves FIRST", async () => { @@ -849,12 +893,111 @@ describe("SkillsScreen", () => { ).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(); renderWithMantine(); await user.click(screen.getByText("data-analysis")); - await user.click(screen.getByRole("button", { name: /View SKILL.md/ })); - expect(await screen.findByTestId("skill-md-preview")).toBeInTheDocument(); + const viewer = screen.getByTestId("skill-resource-viewer"); + 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(); + 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(); + 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( + , + ); + 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(); + 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 () => { @@ -864,10 +1007,10 @@ describe("SkillsScreen", () => { , ); await user.click(screen.getByText("data-analysis")); - await user.click(screen.getByRole("button", { name: /View SKILL.md/ })); expect( - await screen.findByText("Could not read SKILL.md"), + await screen.findByText("Could not read this resource"), ).toBeInTheDocument(); + expect(screen.getByText("gone")).toBeInTheDocument(); }); it("wraps a non-Error SKILL.md rejection", async () => { @@ -877,10 +1020,36 @@ describe("SkillsScreen", () => { , ); await user.click(screen.getByText("data-analysis")); - await user.click(screen.getByRole("button", { name: /View SKILL.md/ })); expect(await screen.findByText("bare")).toBeInTheDocument(); }); + it("keeps the three sections independently collapsible", async () => { + const user = userEvent.setup(); + renderWithMantine(); + 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 () => { const user = userEvent.setup(); renderWithMantine(); @@ -895,14 +1064,27 @@ describe("SkillsScreen", () => { 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(); - renderWithMantine(); + const onReadSkillFile = vi.fn(async (uri: string) => ({ + text: `contents of ${uri}\n`, + })); + renderWithMantine( + , + ); await user.click(screen.getByText("data-analysis")); - await user.click(screen.getByRole("button", { name: /View SKILL.md/ })); - expect(await screen.findByTestId("skill-md-preview")).toBeInTheDocument(); + const viewer = screen.getByTestId("skill-resource-viewer"); + 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")); - 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 () => { @@ -1032,10 +1214,15 @@ describe("SkillsScreen", () => { , ); 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")); - release?.({ text: SELF_TEXT }); - expect(screen.queryByTestId("skill-md-preview")).not.toBeInTheDocument(); + stale?.({ text: "# from the abandoned skill\n" }); + expect(screen.getByTestId("skill-resource-viewer")).not.toHaveTextContent( + "abandoned", + ); }); it("discards a failed SKILL.md read that resolves after the selection moved on", async () => { @@ -1051,9 +1238,10 @@ describe("SkillsScreen", () => { , ); 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")); - fail?.(new Error("too late")); + stale?.(new Error("too late")); expect(screen.queryByText("too late")).not.toBeInTheDocument(); }); }); diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index e4842135..545f9195 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -1,5 +1,6 @@ -import { useCallback, useMemo, useRef, useState } from "react"; +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { + Accordion, Alert, Badge, Button, @@ -8,6 +9,7 @@ import { Flex, Group, NavLink, + Paper, ScrollArea, Stack, Table, @@ -15,6 +17,7 @@ import { TextInput, } from "@mantine/core"; import { MdRefresh, MdSearch, MdVerifiedUser } from "react-icons/md"; +import { RiArrowRightSLine } from "react-icons/ri"; import type { SkillEntry, SkillResource, @@ -36,6 +39,7 @@ import { skillFileBytes, type SkillFileContents, } from "../../../utils/skillFileBytes"; +import { splitSkillFile } from "../../../utils/splitSkillFile"; /** * 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 click that produced it. The manifest key cannot order two reads issued - * for the SAME manifest, so without `attempt` a double click whose older read - * finishes last would replace the newer preview. + * The resource on display in the viewer, plus the manifest it belongs to + * (`null` as above) and the request that produced it. The manifest key cannot + * order two reads issued for the SAME manifest, so without `attempt` a second + * 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 { key: string | null; attempt?: number; + /** The resource this slot is showing (or fetching). */ + uri?: string; contents?: SkillFileContents; message?: string; } @@ -177,16 +188,31 @@ const DetailCard = Card.withProps({ 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", scrollbars: "y", 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({ - c: "dimmed", - ta: "center", + variant: "emptyState", py: "xl", }); @@ -217,39 +243,55 @@ const RefreshButton = Button.withProps({ leftSection: , }); +// 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({ variant: "light", size: "compact-sm", leftSection: , }); +const FetchButton = Button.withProps({ + variant: "light", + size: "compact-sm", +}); + // A `Text` renders a `

`, so a section heading must never *wrap* the count // badge beside it — a `

` inside a `

` is invalid HTML that React reports // as a hydration error and the Storybook run fails on. Heading and badge sit // side by side in an `InlineRow` instead. const SectionHeading = Text.withProps({ - fw: 600, - size: "sm", + variant: "sectionHeading", }); const MonoCaption = Text.withProps({ - size: "xs", - c: "dimmed", - ff: "monospace", -}); - -const DetailStack = Stack.withProps({ - gap: "md", + variant: "monoCaption", }); const IssueStack = Stack.withProps({ 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({ + variant: "manifest", striped: 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", }); @@ -270,12 +312,87 @@ const RowVerifyButton = Button.withProps({ 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({ - fw: 600, - size: "lg", + variant: "skillTitle", + // 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, }); +/** + * 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. */ function issueColor(issue: SkillIssue): string { return issue.severity === "error" ? "red" : "yellow"; @@ -316,6 +433,22 @@ function isStale( 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. */ function shortDigest(digest: string | undefined): string { if (!digest) return "—"; @@ -375,6 +508,16 @@ export function SkillsScreen({ const [batches, setBatches] = useState>( () => 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([ + "conformance", + "resources", + "resource", + ]); // 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 // counter rather than three because it only has to be *increasing*, and each @@ -522,33 +665,68 @@ export function SkillsScreen({ ); }, [manifest, manifestKey, verifyRow]); - const showSkillMd = useCallback(() => { - if (!selected) return; - const key = manifestKey; - const attempt = (nextAttempt.current += 1); - // A click handler cannot await, and this chain terminates in its own - // `catch` that surfaces the message in the preview slot. Both arms go - // through `writePreview`, which drops a result whose manifest has been - // invalidated OR whose click has been superseded. - const writePreview = (next: Omit) => - setPreviewState((prev) => - isStale(prev, key, attempt) ? prev : { key, attempt, ...next }, - ); - // Claimed BEFORE the request goes out, the way `verifyRow` claims its row. - // Recording the attempt only on settle leaves a window where an older - // request that happens to resolve first is still considered current, and - // publishes its contents while a newer one is in flight. Clearing the - // previous result at the same time also means the pane doesn't keep - // showing the old file while the new read is running. - writePreview({}); - void onReadSkillFile(selected.uri) - .then((contents) => writePreview({ contents })) - .catch((err: unknown) => { - writePreview({ - message: err instanceof Error ? err.message : String(err), + /** + * Put one of the skill's files in the viewer. Driven both by the effect that + * 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); + // A click handler cannot await, and this chain terminates in its own + // `catch` that surfaces the message in the viewer. Both arms go through + // `writePreview`, which drops a result whose manifest has been + // invalidated OR whose request has been superseded. + const writePreview = (next: Omit) => + setPreviewState((prev) => + isStale(prev, key, attempt) ? prev : { key, attempt, ...next }, + ); + // Claimed BEFORE the request goes out, the way `verifyRow` claims its + // row. Recording the attempt only on settle leaves a window where an + // older request that happens to resolve first is still considered + // current, and publishes its contents while a newer one is in flight. + // The `uri` rides along on every write so the heading names the file + // being fetched while the read is still running, rather than continuing + // to announce the previous one for as long as the read takes. + writePreview({ uri }); + void onReadSkillFile(uri) + .then((contents) => writePreview({ uri, contents })) + .catch((err: unknown) => { + writePreview({ + uri, + 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(() => { if (!selected) return; @@ -607,10 +785,29 @@ export function SkillsScreen({ : "differs"; const batchRunning = batches.has(manifestKey); - const preview = - previewState.key === manifestKey ? previewState.contents : undefined; - const previewError = - previewState.key === manifestKey ? previewState.message : undefined; + const previewCurrent = previewState.key === manifestKey; + const preview = previewCurrent ? previewState.contents : undefined; + const previewError = previewCurrent ? 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 warningCount = issues.length - errorCount; @@ -690,56 +887,121 @@ export function SkillsScreen({ {!selected ? ( Select a skill to view details ) : ( - - + + {skillDisplayName(selected)} {selected.uri} + {/* 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. */} + + + Fetch with skills/get + + + Verify all + + + - {selected.frontmatter.description && ( - {selected.frontmatter.description} - )} + {selected.frontmatter.description && ( + {selected.frontmatter.description} + )} - - - Conformance - 0 ? "red" : "green"}> - {errorCount} error(s), {warningCount} warning(s) - - - {issues.length === 0 ? ( - - No structural issues found in this entry. - - ) : ( - - {issues.map((issue, index) => ( - - - {issue.message} - {issue.resourceUri && ( - {issue.resourceUri} - )} - - - ))} - + {/* 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. */} + } + 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} + > + + mih={ + openSections.includes("conformance") + ? OPEN_SECTION_MIN_HEIGHT + : undefined + } + > + + + Conformance + + {errorCount} error(s), {warningCount} warning(s) + + + + + {issues.length === 0 ? ( + + No structural issues found in this entry. + + ) : ( + + {issues.map((issue, index) => ( + + + {issue.message} + {issue.resourceUri && ( + {issue.resourceUri} + )} + + + ))} + + )} + + - - + + Resources @@ -747,138 +1009,249 @@ export function SkillsScreen({ bytes - - - Fetch with skills/get - - - View SKILL.md - - - Verify all - - - - {selected.resources === DYNAMIC_RESOURCES ? ( - - This skill declares{" "} - resources: "dynamic" — its files are - generated, so no manifest is advertised and integrity cannot - be verified. - - ) : ( - - - - URI - Size - Digest - Verification - - - - {manifest.map((resource, index) => { - const state = fileStates[index]; - const color = - state?.status === "done" - ? verificationColor(state.verification.status) - : state?.status === "error" - ? "red" - : "gray"; - return ( - // Index-keyed for the same reason the verdicts are: - // a duplicated URI is a case this screen reports, so - // it must not also collide two rows into one. - - {resource.uri} - {resource.size ?? "—"} - {shortDigest(resource.digest)} - - - - {verificationLabel(state)} - - - void verifyRow(index, resource, manifestKey) - } - > - Verify - - - + + + + {selected.resources === DYNAMIC_RESOURCES ? ( + + This skill declares{" "} + resources: "dynamic" — its files + are generated, so no manifest is advertised and + integrity cannot be verified. + + ) : ( + + + + URI + Size + Digest + Verification + + + {manifest.map((resource, index) => { + const state = fileStates[index]; + const color = + state?.status === "done" + ? verificationColor(state.verification.status) + : state?.status === "error" + ? "red" + : "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 ( + // Index-keyed for the same reason the verdicts + // are: a duplicated URI is a case this screen + // reports, so it must not also collide two rows + // into one. + + + + showResource(resource.uri, manifestKey) + } + > + {resource.uri} + + + {resource.size ?? "—"} + + {shortDigest(resource.digest)} + + + + + {verificationLabel(state)} + + + void verifyRow( + index, + resource, + manifestKey, + ) + } + > + Verify + + + + + ); + })} + + + )} + {manifest.map((resource, index) => { + const state = fileStates[index]; + if (state?.status === "done") { + const result = state.verification; + if (result.status !== "mismatch") return null; + // A size disagreement is caught BEFORE hashing, so it + // has no `actualDigest` — titling it "Digest + // mismatch" and rendering "actual undefined" would + // hide the real failure. + const sizeFailure = result.actualDigest === undefined; + return ( + + + {resource.uri} + {sizeFailure ? ( + {result.reason} + ) : ( + <> + + expected {result.expectedDigest} + + + actual {result.actualDigest} + + + )} + + ); - })} - - - )} - {manifest.map((resource, index) => { - const state = fileStates[index]; - if (state?.status === "done") { - const result = state.verification; - if (result.status !== "mismatch") return null; - // A size disagreement is caught BEFORE hashing, so it has - // no `actualDigest` — titling it "Digest mismatch" and - // rendering "actual undefined" would hide the real failure. - const sizeFailure = result.actualDigest === undefined; - return ( - - - {resource.uri} - {sizeFailure ? ( - {result.reason} - ) : ( - <> - - expected {result.expectedDigest} - - - actual {result.actualDigest} - - - )} - - - ); - } - if (state?.status === "error") { - return ( - - - {resource.uri} - {state.message} - - - ); - } - return null; - })} - + } + if (state?.status === "error") { + return ( + + + {resource.uri} + {state.message} + + + ); + } + return null; + })} + + + + {/* 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 && ( + + + Frontmatter + + + + {/* 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. */} + + + + + )} + + + + + Skill Resource + {previewUri !== undefined && ( + {resourceFileName(previewUri)} + )} + + + + {previewError !== undefined ? ( + + {previewError} + + ) : ( + 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. */ + + ) + )} + + + + + {fetched?.message !== undefined && ( {fetched.message} @@ -888,6 +1261,7 @@ export function SkillsScreen({ )} )} - - {previewError && ( - - {previewError} - - )} - {preview && ( - - SKILL.md - {/* `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. */} - - - )} - - - Frontmatter - - - - + + )} diff --git a/clients/web/src/theme/Table.ts b/clients/web/src/theme/Table.ts new file mode 100644 index 00000000..d768a03b --- /dev/null +++ b/clients/web/src/theme/Table.ts @@ -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 {}; + }, +}); diff --git a/clients/web/src/theme/Text.ts b/clients/web/src/theme/Text.ts index 8ee4f53d..864574d3 100644 --- a/clients/web/src/theme/Text.ts +++ b/clients/web/src/theme/Text.ts @@ -42,6 +42,42 @@ export const ThemeText = Text.extend({ // 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 // 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 ( props.variant === "versionBadge" || props.variant === "copyrightBadge" diff --git a/clients/web/src/theme/index.ts b/clients/web/src/theme/index.ts index 313c2ac5..97ca8ebd 100644 --- a/clients/web/src/theme/index.ts +++ b/clients/web/src/theme/index.ts @@ -16,6 +16,7 @@ export { ThemePaper } from "./Paper"; export { ThemeScrollArea, ThemeScrollAreaAutosize } from "./ScrollArea"; export { ThemeSelect } from "./Select"; export { ThemeSwitch } from "./Switch"; +export { ThemeTable } from "./Table"; export { ThemeText } from "./Text"; export { ThemeTextInput } from "./TextInput"; export { ThemeTitle } from "./Title"; diff --git a/clients/web/src/theme/theme.ts b/clients/web/src/theme/theme.ts index c76fb316..8d074835 100644 --- a/clients/web/src/theme/theme.ts +++ b/clients/web/src/theme/theme.ts @@ -19,6 +19,7 @@ import { ThemeScrollAreaAutosize, ThemeSelect, ThemeSwitch, + ThemeTable, ThemeText, ThemeTextInput, ThemeTitle, @@ -201,6 +202,7 @@ export const theme = createTheme({ ScrollAreaAutosize: ThemeScrollAreaAutosize, Select: ThemeSelect, Switch: ThemeSwitch, + Table: ThemeTable, Text: ThemeText, TextInput: ThemeTextInput, Title: ThemeTitle, diff --git a/clients/web/src/utils/splitSkillFile.test.ts b/clients/web/src/utils/splitSkillFile.test.ts new file mode 100644 index 00000000..fdd7e37b --- /dev/null +++ b/clients/web/src/utils/splitSkillFile.test.ts @@ -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" }); + }); +}); diff --git a/clients/web/src/utils/splitSkillFile.ts b/clients/web/src/utils/splitSkillFile.ts new file mode 100644 index 00000000..b60be956 --- /dev/null +++ b/clients/web/src/utils/splitSkillFile.ts @@ -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/, "") }; +} diff --git a/test-servers/src/skills.ts b/test-servers/src/skills.ts index 18d2edca..05cae596 100644 --- a/test-servers/src/skills.ts +++ b/test-servers/src/skills.ts @@ -126,9 +126,74 @@ const DATA_ANALYSIS_FM: Frontmatter = { name: "data-analysis", 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( 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 = "# Column rules\n\nNumeric columns get min/max/mean; text columns get a value count.\n";