diff --git a/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.tsx b/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.tsx index 5988ed0d..6204c516 100644 --- a/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.tsx +++ b/clients/web/src/components/groups/ResourcePreviewPanel/ResourcePreviewPanel.tsx @@ -20,6 +20,7 @@ import { ContentViewer } from "../../elements/ContentViewer/ContentViewer"; import { getMimeKind } from "../../elements/ContentViewer/contentViewerUtils"; import { CopyButton } from "../../elements/CopyButton/CopyButton"; import { SubscribeButton } from "../../elements/SubscribeButton/SubscribeButton"; +import { inferMimeFromUri } from "../../../utils/inferMimeFromUri"; export interface ResourcePreviewPanelProps { resource: Resource; @@ -155,35 +156,6 @@ const ContentStack = Stack.withProps({ gap: "md", }); -// Map a file extension to the MIME type that drives ContentViewer's per-MIME -// renderer dispatch. MCP servers commonly omit `mimeType` (or return a generic -// `text/plain` / `application/octet-stream`), so the URI suffix is the most -// reliable signal for engaging the markdown / PDF / CSV / XML / HTML / CSS -// renderers. Order doesn't matter — suffixes are unique. -const URI_SUFFIX_MIME: ReadonlyArray = [ - [".md", "text/markdown"], - [".markdown", "text/markdown"], - [".csv", "text/csv"], - [".json", "application/json"], - [".xml", "application/xml"], - [".html", "text/html"], - [".htm", "text/html"], - [".css", "text/css"], - [".pdf", "application/pdf"], -]; - -// Infer a MIME type from the URI's file extension when the server didn't supply -// one. Returns undefined for unrecognized suffixes so callers fall through to -// the octet-stream default. -function inferMimeFromUri(uri: string): string | undefined { - const path = uri.split("?")[0].split("#")[0]; - const lower = path.toLowerCase(); - for (const [suffix, mime] of URI_SUFFIX_MIME) { - if (lower.endsWith(suffix)) return mime; - } - return undefined; -} - function effectiveMime( itemMime: string | undefined, resource: Resource, diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx index df786ba1..6b7bd17d 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx @@ -127,6 +127,11 @@ export const ConformingSkill: Story = { play: async ({ canvasElement }) => { const canvas = within(canvasElement); await userEvent.click(canvas.getByText("data-analysis")); + // A clean entry opens with Conformance COLLAPSED (#2263) — its badge + // already carries the whole answer — so the verdict is behind one click. + const control = canvas.getByRole("button", { name: /Conformance/ }); + await expect(control).toHaveAttribute("aria-expanded", "false"); + await userEvent.click(control); await expect(canvas.getByText("Conforms")).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 93f111d8..c69575f8 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -202,14 +202,79 @@ describe("SkillsScreen", () => { expect(onRefreshList).toHaveBeenCalled(); }); - it("reports a conforming skill as conforming", async () => { + it("collapses Conformance for a clean entry, and still reports it on expand", async () => { + // A clean entry opens collapsed (#2263): the header badge already says + // "0 error(s), 0 warning(s)", so an expanded "Conforms" panel is only + // taking space the file viewer could use. The verdict is still there. const user = userEvent.setup(); renderWithMantine(); await user.click(screen.getByText("data-analysis")); + const control = screen.getByRole("button", { name: /Conformance/ }); + expect(control).toHaveAttribute("aria-expanded", "false"); + expect(screen.queryByText("Conforms")).not.toBeInTheDocument(); + + await user.click(control); expect(screen.getByText("Conforms")).toBeInTheDocument(); expect(screen.queryByTestId("skill-issues")).not.toBeInTheDocument(); }); + it("opens Conformance for an entry that has findings", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("right-name")); + expect(screen.getByRole("button", { name: /Conformance/ })).toHaveAttribute( + "aria-expanded", + "true", + ); + expect(screen.getByTestId("skill-issues")).toBeInTheDocument(); + }); + + it("re-opens Conformance when switching from a clean entry to a broken one", async () => { + // The section tracks the signal rather than latching: a user who lands on a + // clean skill and then picks a broken one must not have the findings hidden + // behind a click. + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("data-analysis")); + expect(screen.getByRole("button", { name: /Conformance/ })).toHaveAttribute( + "aria-expanded", + "false", + ); + await user.click(screen.getByText("right-name")); + expect(screen.getByRole("button", { name: /Conformance/ })).toHaveAttribute( + "aria-expanded", + "true", + ); + }); + + it("reports a digest mismatch in Conformance, with its own red badge", async () => { + // `tampered-notes` is structurally clean but serves bytes that do not match + // its manifest, so its Conformance section starts collapsed — pressing + // Verify has to open it, or the verdict lands where nobody can see it + // (#2263). The mismatch count is a separate badge because it is a RUNTIME + // result: folding it into "N error(s)" would make that number change + // meaning after a click. + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("tampered")); + expect(screen.getByRole("button", { name: /Conformance/ })).toHaveAttribute( + "aria-expanded", + "false", + ); + expect(screen.queryByText(/digest mismatch\(es\)/)).not.toBeInTheDocument(); + + await user.click(screen.getByRole("button", { name: /Verify all/ })); + expect(await screen.findByText("Digest mismatch")).toBeInTheDocument(); + + const conformance = screen.getByRole("button", { name: /Conformance/ }); + expect(conformance).toHaveAttribute("aria-expanded", "true"); + // The alert renders inside Conformance, not beside the manifest table. + expect(conformance.closest(".mantine-Accordion-item")).toContainElement( + screen.getByText("Digest mismatch"), + ); + expect(badgeStyle(/1 digest mismatch\(es\)/)).toContain("red"); + }); + 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). @@ -240,17 +305,48 @@ describe("SkillsScreen", () => { expect(within(issues).getByText("name-path-mismatch")).toBeInTheDocument(); }); - it("shows the dynamic warning and no manifest table", async () => { + it("states the dynamic case once, in Conformance, with no Resources section", async () => { + // A dynamic skill has no manifest, so an empty Resources section whose only + // content explains its own emptiness is redundant with the conformance + // finding — the fact is stated once, in prose, in Conformance (#2263). const user = userEvent.setup(); renderWithMantine(); await user.click(screen.getByText("dynamic-report")); - expect(screen.getByText("Dynamic resources")).toBeInTheDocument(); + + const conformance = screen.getByRole("button", { name: /Conformance/ }); + expect(conformance.closest(".mantine-Accordion-item")).toContainElement( + screen.getByText("Dynamic resources"), + ); + // The section, its header and its table are all gone — not merely empty. + expect( + screen.queryByRole("button", { name: /Resources/ }), + ).not.toBeInTheDocument(); expect(screen.queryByTestId("skill-manifest")).not.toBeInTheDocument(); + // And the terse finding is not repeated beside the prose banner. + expect(screen.queryByText("dynamic-resources")).not.toBeInTheDocument(); + // It still counts toward the warning total, because it is still a finding. + expect( + screen.getByText(/0 error\(s\), 1 warning\(s\)/), + ).toBeInTheDocument(); + // "Verify all" has nothing to verify, so it is disabled rather than a // button that silently does nothing. expect(screen.getByRole("button", { name: /Verify all/ })).toBeDisabled(); }); + it("expand-all stays satisfiable for a dynamic skill", async () => { + // The toggle compares against the sections that actually render; leaving + // `resources` in that list would make "expand all" unreachable here. + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("dynamic-report")); + await user.click(screen.getByRole("button", { name: "Collapse all" })); + await user.click(screen.getByRole("button", { name: "Expand all" })); + expect( + screen.getByRole("button", { name: "Collapse all" }), + ).toBeInTheDocument(); + }); + it("verifies a file whose bytes match its digest", async () => { const user = userEvent.setup(); renderWithMantine(); @@ -500,6 +596,14 @@ describe("SkillsScreen", () => { expect( await screen.findByText("skills/get matches skills/list"), ).toBeInTheDocument(); + // The verdict is a conformance statement, so it renders inside the + // Conformance section (#2263) — and that section auto-collapses for a clean + // entry, so the fetch has to open it or the answer would be invisible. + const conformance = screen.getByRole("button", { name: /Conformance/ }); + expect(conformance).toHaveAttribute("aria-expanded", "true"); + expect(conformance.closest(".mantine-Accordion-item")).toContainElement( + screen.getByTestId("skills-get-result"), + ); }); it("treats key and manifest order as immaterial when matching", async () => { @@ -1023,19 +1127,18 @@ describe("SkillsScreen", () => { expect(await screen.findByText("bare")).toBeInTheDocument(); }); - it("keeps the three sections independently collapsible", async () => { + it("keeps the 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", - ); + // Everything starts open; `right-name` has a finding, so Conformance is + // open here too rather than auto-collapsed. + await user.click(screen.getByText("right-name")); + for (const name of [/Conformance/, /Resources/, /Frontmatter/]) { + expect(screen.getByRole("button", { name })).toHaveAttribute( + "aria-expanded", + "true", + ); + } // Collapsing one leaves the others alone — `multiple`, not a single-open // accordion. @@ -1050,6 +1153,31 @@ describe("SkillsScreen", () => { ); }); + it("toggles every section at once from the header control", async () => { + const user = userEvent.setup(); + renderWithMantine(); + const ALL = [/Conformance/, /Resources/, /Frontmatter/, /Skill Resource/]; + await user.click(screen.getByText("right-name")); + // The shared `ListToggle` element, whose labels are "Expand all" / + // "Collapse all". Everything starts open, so it offers to collapse first. + await user.click(screen.getByRole("button", { name: "Collapse all" })); + for (const name of ALL) { + expect(screen.getByRole("button", { name })).toHaveAttribute( + "aria-expanded", + "false", + ); + } + + // And back the other way from the same control. + await user.click(screen.getByRole("button", { name: "Expand all" })); + for (const name of ALL) { + expect(screen.getByRole("button", { name })).toHaveAttribute( + "aria-expanded", + "true", + ); + } + }); + it("drops verification results when the selection changes", async () => { const user = userEvent.setup(); renderWithMantine(); diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index 545f9195..502649f3 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -10,13 +10,13 @@ import { Group, NavLink, Paper, - ScrollArea, Stack, Table, Text, TextInput, + VisuallyHidden, } from "@mantine/core"; -import { MdRefresh, MdSearch, MdVerifiedUser } from "react-icons/md"; +import { MdSearch, MdVerifiedUser } from "react-icons/md"; import { RiArrowRightSLine } from "react-icons/ri"; import type { SkillEntry, @@ -34,12 +34,18 @@ import { type SkillVerification, } from "@inspector/core/mcp/skills.js"; import { ContentViewer } from "../../elements/ContentViewer/ContentViewer"; +import { ListToggle } from "../../elements/ListToggle/ListToggle"; import { useValueChange } from "../../../hooks/useValueChange"; import { skillFileBytes, type SkillFileContents, } from "../../../utils/skillFileBytes"; import { splitSkillFile } from "../../../utils/splitSkillFile"; +import { + inferMimeFromUri, + isMarkdownMime, +} from "../../../utils/inferMimeFromUri"; +import { tryDecodeBase64ToUtf8 } from "../../elements/ContentViewer/contentViewerUtils"; /** * How many skill files are read at once by "Verify all". A conforming manifest @@ -198,16 +204,6 @@ const DetailColumn = Stack.withProps({ 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, - 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. @@ -237,26 +233,31 @@ const SearchInput = TextInput.withProps({ leftSection: , }); +// Bare, like the Refresh in `ListChangedIndicator` that Tools, Resources and +// Prompts use — this screen was the only one with a glyph on it. const RefreshButton = Button.withProps({ variant: "subtle", size: "compact-xs", - 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. +// Both header actions take the Refresh button's style from +// `ListChangedIndicator` — `sm` + `subtle` — so the pane's controls read as the +// same kind of control the rest of the app uses, and sit level with the +// `ListToggle` beside them. +// +// The shield stays on the controls that actually check integrity — "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, +// so the icon would overstate what that button does. const VerifyButton = Button.withProps({ - variant: "light", - size: "compact-sm", + size: "sm", + variant: "subtle", leftSection: , }); const FetchButton = Button.withProps({ - variant: "light", - size: "compact-sm", + size: "sm", + variant: "subtle", }); // A `Text` renders a `

`, so a section heading must never *wrap* the count @@ -275,6 +276,14 @@ const IssueStack = Stack.withProps({ gap: "xs", }); +// The Conformance panel stacks banners from three different sources — the +// static findings, the per-file verification verdicts, and the `skills/get` +// comparison — and they are only legible as separate statements if they are +// spaced apart. `sm` is the gap the monitoring sidebar's list uses. +const ConformanceStack = Stack.withProps({ + gap: "sm", +}); + // 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 @@ -312,6 +321,13 @@ const RowVerifyButton = Button.withProps({ size: "compact-xs", }); +// The action column: sized to its button and right-aligned, so the buttons form +// one straight edge instead of tracking the verdict badge's width. +const ActionCell = Table.Td.withProps({ + w: 1, + ta: "right", +}); + // 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 — @@ -334,15 +350,20 @@ const SkillTitle = Text.withProps({ }); /** - * 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. + * Per-section flex for the metadata sections: **content height, never shrink**. + * + * `ResourceControls` weights its shrink by item count (#1462) because its + * panels hold uniform lists that degrade gracefully when squeezed. These do + * not — a findings list, a manifest table with alerts under it, a frontmatter + * block — and squeezing them sliced content mid-line: a `Digest mismatch` + * alert cut in half by the section header below it. The panel was scrollable, + * but a macOS overlay scrollbar is invisible until hover, so it read as broken. + * + * Sized to content instead, with the overflow handled once at the accordion + * root (`skillSections`), the stack scrolls at a *section boundary* rather than + * through the middle of a finding. */ -function sectionFlex(open: boolean, count: number): string { - return open && count > 0 ? `0 ${count} auto` : "0 0 auto"; -} +const SECTION_FLEX = "0 0 auto"; /** * The file viewer's flex, which is deliberately NOT `sectionFlex`. @@ -508,14 +529,19 @@ 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. + // Which sections are open. A view preference, so it is deliberately NOT reset + // by the manifest-change invalidation below — a user who collapsed a section + // wants it collapsed on the next skill too. + // + // Everything starts open. The one thing that closes on its own is Conformance + // for an entry with no findings, and that is because its header badge already + // carries the whole answer; nothing else here can be summarised by its header, + // so opening collapsed would just hide content behind a click the user has no + // reason to expect. const [openSections, setOpenSections] = useState([ "conformance", "resources", + "frontmatter", "resource", ]); // Monotonic attempt token, shared by every on-demand action here: a manifest @@ -550,6 +576,11 @@ export function SkillsScreen({ [selected], ); + // A `resources: "dynamic"` skill advertises no manifest at all, so it has no + // Resources section to show — the fact is a conformance statement, and it is + // made once, there. + const isDynamic = selected?.resources === DYNAMIC_RESOURCES; + const manifest: SkillResource[] = useMemo( () => selected && selected.resources !== DYNAMIC_RESOURCES @@ -581,6 +612,24 @@ export function SkillsScreen({ setVerification({ key: next, files: {} }); setPreviewState({ key: next }); setFetchedEntry({ key: next }); + // Conformance tracks whether it has anything to say: an entry with no + // errors and no warnings opens collapsed, because "0 error(s), 0 + // warning(s)" on the header already carries the whole message and an + // expanded "Conforms" panel is just space the file viewer could use. An + // entry WITH findings opens expanded, so switching from a clean skill to a + // broken one does not hide the findings behind a click. + // + // Adjusted here, during render, rather than in an effect — the same reason + // the three invalidations above are: an effect would paint one frame with + // the previous skill's answer. + setOpenSections((prev) => { + const hasFindings = issues.length > 0; + const isOpen = prev.includes("conformance"); + if (hasFindings === isOpen) return prev; + return hasFindings + ? [...prev, "conformance"] + : prev.filter((section) => section !== "conformance"); + }); }); const fileStates = verification.key === manifestKey ? verification.files : {}; @@ -594,6 +643,13 @@ export function SkillsScreen({ */ const verifyRow = useCallback( async (index: number, resource: SkillResource, key: string) => { + // A mismatch is reported in the Conformance section, which auto-collapses + // for an entry with no *static* findings — and `tampered-notes` is + // exactly that: structurally clean, bytes wrong. Opening it here is what + // stops the verdict landing somewhere the user cannot see it. + setOpenSections((prev) => + prev.includes("conformance") ? prev : [...prev, "conformance"], + ); // Claimed synchronously, so two verifications of this row are ordered // before either read starts. const attempt = (nextAttempt.current += 1); @@ -629,6 +685,11 @@ export function SkillsScreen({ ); const verifyAll = useCallback(() => { + // Same reason as `verifyRow`: the verdicts render in Conformance, which may + // be collapsed for a structurally clean entry. + setOpenSections((prev) => + prev.includes("conformance") ? prev : [...prev, "conformance"], + ); // Bounded concurrency, not `Promise.all` over the whole manifest: a // conforming skill may declare 512 files, and firing 512 simultaneous // `resources/read` calls would bury the transport and the server for no @@ -732,6 +793,13 @@ export function SkillsScreen({ if (!selected) return; const key = manifestKey; const attempt = (nextAttempt.current += 1); + // The verdict renders inside the Conformance section, which auto-collapses + // for a clean entry — and a clean entry is exactly the common case for this + // button. Without this the answer would land in a collapsed section and the + // click would look like it did nothing. + setOpenSections((prev) => + prev.includes("conformance") ? prev : [...prev, "conformance"], + ); // Same shape as the SKILL.md read: a click handler cannot await, the chain // ends in its own `catch`, and both arms drop a result whose manifest has // been invalidated or whose click has been superseded. @@ -793,21 +861,74 @@ export function SkillsScreen({ // selection effect has claimed a slot. const previewUri = (previewCurrent ? previewState.uri : undefined) ?? selectedUri; + // One effective MIME for the displayed file, inferred the same way the + // Resources screen infers one: servers routinely omit `mimeType` or answer a + // generic `text/plain`, and the URI suffix is the better signal. + const previewMime = useMemo( + () => + preview?.mimeType ?? + (previewUri !== undefined ? inferMimeFromUri(previewUri) : undefined) ?? + // Skill files are markdown by construction under SEP-2640, so that is the + // right last resort here rather than octet-stream. + "text/markdown", + [preview, previewUri], + ); + // 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( - () => + // Gated on the MIME being **markdown**, not on which payload arm the server + // used. Keying on the arm got both halves of that wrong: a markdown file + // served as a base64 `blob` — which this screen supports — kept its + // frontmatter in the viewer and produced no Frontmatter section, while a + // textual multi-document YAML resource had its first document silently + // removed as "frontmatter". A blob is decoded here so a base64 SKILL.md + // splits exactly like a text one. + const previewParts = useMemo(() => { + if (!isMarkdownMime(previewMime)) return undefined; + const text = typeof preview?.text === "string" - ? splitSkillFile(preview.text) - : undefined, - [preview], + ? preview.text + : preview?.blob !== undefined + ? tryDecodeBase64ToUtf8(preview.blob) + : null; + return text === null || text === undefined + ? undefined + : splitSkillFile(text); + }, [preview, previewMime]); + + // Which sections this skill actually renders — Frontmatter only exists when + // the displayed file has any, so an "expand all" that named it unconditionally + // would leave the toggle stuck reading "Expand" on a file without one. + const sectionIds = useMemo( + () => [ + "conformance", + // A dynamic skill renders no Resources section, so naming it here would + // leave "expand all" permanently unsatisfied. + ...(isDynamic ? [] : ["resources"]), + ...(previewParts?.frontmatter !== undefined ? ["frontmatter"] : []), + "resource", + ], + [isDynamic, previewParts], ); + const allSectionsOpen = sectionIds.every((id) => openSections.includes(id)); + + // Files whose bytes disagree with what the manifest advertised. A *runtime* + // count, unlike the static findings beside it: it only exists once a + // verification has actually run, which is why it renders as its own badge + // rather than being folded into the error total — "2 error(s)" that changes + // meaning after you press Verify would be the worse of the two options. + // + // Covers a size disagreement as well as a digest one: `verifySkillResource` + // reports both as `mismatch`, the size check simply being the cheaper one + // that runs first. + const mismatchCount = Object.values(fileStates).filter( + (state) => + state.status === "done" && state.verification.status === "mismatch", + ).length; const errorCount = issues.filter((i) => i.severity === "error").length; const warningCount = issues.length - errorCount; @@ -908,6 +1029,17 @@ export function SkillsScreen({ > Verify all + {/* The app's shared expand/collapse-all control, the same one + `ResourceControls` puts on its disclosure accordion. + `compact` means "currently collapsed", so it is the negation + of everything being open. */} + + setOpenSections(allSectionsOpen ? [] : [...sectionIds]) + } + /> @@ -926,7 +1058,7 @@ export function SkillsScreen({ scrolling as one column. */} } flex={1} mih={0} @@ -938,10 +1070,7 @@ export function SkillsScreen({ > {errorCount} error(s), {warningCount} warning(s) + {/* Only once something has actually failed verification — + a permanent "0 digest mismatches" would read as a + verified result before anything had been checked. */} + {mismatchCount > 0 && ( + + {mismatchCount} digest mismatch(es) + + )} - {issues.length === 0 ? ( - - No structural issues found in this entry. - - ) : ( - - {issues.map((issue, index) => ( - - - {issue.message} - {issue.resourceUri && ( - {issue.resourceUri} - )} - - - ))} - - )} - - - - - - - Resources - - {manifest.length} file(s), {totalSkillBytes(manifest)}{" "} - bytes - - - - - - {selected.resources === DYNAMIC_RESOURCES ? ( + + {/* A dynamic skill's `dynamic-resources` finding is + rendered here in full rather than as a bare code and + message, and the Resources section is dropped entirely — + otherwise the same fact is stated twice, once as a + finding and once as an empty section's explanation. */} + {isDynamic && ( This skill declares{" "} resources: "dynamic" — its files are generated, so no manifest is advertised and integrity cannot be verified. + )} + {issues.length === 0 ? ( + + No structural issues found in this entry. + ) : ( - - - - 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 - - - - - ); - })} - - + + {issues + // The banner above already states this one, in prose. + .filter((issue) => issue.code !== "dynamic-resources") + .map((issue, index) => ( + + + {issue.message} + {issue.resourceUri && ( + {issue.resourceUri} + )} + + + ))} + )} {manifest.map((resource, index) => { const state = fileStates[index]; @@ -1154,10 +1192,210 @@ export function SkillsScreen({ } return null; })} - + {fetched?.message !== undefined && ( + + {fetched.message} + + )} + {fetched?.entry !== undefined && ( + + + + {fetchedVerdict === "invalid" + ? fetched.wrongUri + ? "This entry is for a different URI than the one requested, which is never a valid refresh of it." + : "The returned entry does not conform to SEP-2640, so this is not simply a skill that changed since it was listed." + : fetchedVerdict === "matches" + ? "The entry this server returns for this URI describes the same skill it listed (compared ignoring key and manifest order)." + : "The entry this server returns for this URI differs from the one it listed. `skills/get` is a fresh snapshot, so this is expected if the skill changed since the list was fetched — and a server inconsistency if it did not."} + + {fetchedVerdict !== "matches" && ( + + )} + {/* Under the entry, not above it: each finding names + a field, so it reads as an annotation on the JSON + the reader has just been shown rather than a + preamble to something not yet on screen. */} + {(fetched.issues ?? []) + .filter((issue) => issue.severity === "error") + .map((issue, index) => ( + + {issue.code}: {issue.message} + + ))} + + + )} + + {/* A dynamic skill advertises no manifest, so there is no manifest + to show — the Conformance banner above says so once. */} + {!isDynamic && ( + + + + Resources + + {manifest.length} file(s), {totalSkillBytes(manifest)}{" "} + bytes + + + + + + {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 + {/* The action gets its own column so the buttons + line up down the table. Sharing a cell with + the verdict badge staggered them, because the + badge's width tracks its label — "—", + "checking…", "verified" and "mismatch" are all + different sizes. + + The header is named for screen readers but not + shown: a visible label over a column of + buttons is noise, while an *empty* `th` is an + axe `empty-table-header` violation and leaves + the column unnamed in a table's header + navigation. */} + + Actions + + + + + {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 + + + + ); + })} + + + )} + + + + )} + {/* 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 @@ -1167,7 +1405,7 @@ export function SkillsScreen({ 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)} + flex={SECTION_FLEX} mih={ openSections.includes("frontmatter") ? OPEN_SECTION_MIN_HEIGHT @@ -1226,22 +1464,32 @@ export function SkillsScreen({ /* `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. */ + a file it had just read correctly. + + When the file split, the decoded BODY is handed over as + text regardless of which arm the server used — that is + what lets a base64 markdown file lose its frontmatter + to the section above like a text one. Anything that did + not split is passed through in its original arm. */ @@ -1250,70 +1498,6 @@ export function SkillsScreen({ - - - {fetched?.message !== undefined && ( - - {fetched.message} - - )} - {fetched?.entry !== undefined && ( - - - - {fetchedVerdict === "invalid" - ? fetched.wrongUri - ? "This entry is for a different URI than the one requested, which is never a valid refresh of it." - : "This entry breaks a requirement of its own, so the difference is not simply a newer snapshot." - : fetchedVerdict === "matches" - ? "The entry this server returns for this URI describes the same skill it listed (compared ignoring key and manifest order)." - : "The entry this server returns for this URI differs from the one it listed. `skills/get` is a fresh snapshot, so this is expected if the skill changed since the list was fetched — and a server inconsistency if it did not."} - - {(fetched.issues ?? []) - .filter((issue) => issue.severity === "error") - .map((issue, index) => ( - - {issue.code}: {issue.message} - - ))} - {fetchedVerdict !== "matches" && ( - - )} - - - )} - )} diff --git a/clients/web/src/theme/Accordion.ts b/clients/web/src/theme/Accordion.ts index adec35d1..d783dc08 100644 --- a/clients/web/src/theme/Accordion.ts +++ b/clients/web/src/theme/Accordion.ts @@ -14,7 +14,7 @@ export const ThemeAccordion = Accordion.extend({ // and a filled background when the section is open (`aria-expanded`). // Pair it with `chevron={}` and per-item `flex` weights. classNames: (_theme, props) => { - if (props.variant === "disclosure") + if (props.variant === "disclosure" || props.variant === "skillSections") return { root: "disclosure-sections", chevron: "disclosure-chevron", @@ -22,4 +22,27 @@ export const ThemeAccordion = Accordion.extend({ }; return {}; }, + // `skillSections` is `disclosure` plus a scrolling root (#2263). + // + // The Skills pane holds sections whose content is a rendered document or a + // findings list, not a uniform row list, so they are sized to their content + // and never shrink. That removes the mid-content clipping a shrinking panel + // produced — the panel really was scrollable, but macOS overlay scrollbars + // are invisible until hover, so a Resources table cut off mid-alert read as + // broken rather than scrollable. Overflow moves up to the root, so in the + // rare case the sections genuinely exceed the pane it is the *stack* that + // scrolls, at a section boundary, instead of a panel slicing its own content. + styles: (_theme, props) => { + if (props.variant === "skillSections") { + return { + root: { overflowY: "auto", minHeight: 0 }, + // Every section gets the same breathing room under its header that its + // banners get between each other — without it a panel's first item sits + // flush against the control and reads as part of the header rather than + // as the section's content. + content: { paddingTop: "var(--mantine-spacing-sm)" }, + }; + } + return {}; + }, }); diff --git a/clients/web/src/utils/inferMimeFromUri.test.ts b/clients/web/src/utils/inferMimeFromUri.test.ts new file mode 100644 index 00000000..01a48c58 --- /dev/null +++ b/clients/web/src/utils/inferMimeFromUri.test.ts @@ -0,0 +1,53 @@ +import { describe, it, expect } from "vitest"; +import { inferMimeFromUri, isMarkdownMime } from "./inferMimeFromUri"; + +describe("inferMimeFromUri", () => { + it("maps every suffix in the table", () => { + // The whole table, so a typo in an entry cannot pass unnoticed — this is + // the only thing that engages ContentViewer's per-MIME renderers when a + // server omits `mimeType`. + expect(inferMimeFromUri("skill://a/SKILL.md")).toBe("text/markdown"); + expect(inferMimeFromUri("skill://a/notes.markdown")).toBe("text/markdown"); + expect(inferMimeFromUri("file:///data.csv")).toBe("text/csv"); + expect(inferMimeFromUri("file:///a.json")).toBe("application/json"); + expect(inferMimeFromUri("file:///a.xml")).toBe("application/xml"); + expect(inferMimeFromUri("file:///a.html")).toBe("text/html"); + expect(inferMimeFromUri("file:///a.htm")).toBe("text/html"); + expect(inferMimeFromUri("file:///a.css")).toBe("text/css"); + expect(inferMimeFromUri("file:///a.pdf")).toBe("application/pdf"); + }); + + it("is case-insensitive about the suffix", () => { + expect(inferMimeFromUri("skill://a/SKILL.MD")).toBe("text/markdown"); + }); + + it("ignores a query string and a fragment", () => { + // A URI's suffix is a property of its path; `?v=2` must not defeat the + // match, and a fragment must not be mistaken for one. + expect(inferMimeFromUri("https://x/a.md?v=2")).toBe("text/markdown"); + expect(inferMimeFromUri("https://x/a.md#top")).toBe("text/markdown"); + expect(inferMimeFromUri("https://x/a.md?v=2#top")).toBe("text/markdown"); + }); + + it("returns undefined for an unrecognised suffix, so callers can default", () => { + expect(inferMimeFromUri("skill://a/notes.bin")).toBeUndefined(); + expect(inferMimeFromUri("skill://a/no-extension")).toBeUndefined(); + // A dot in the query must not be read as the path's extension. + expect(inferMimeFromUri("https://x/file?name=a.md")).toBeUndefined(); + }); +}); + +describe("isMarkdownMime", () => { + it("accepts both spellings of markdown", () => { + expect(isMarkdownMime("text/markdown")).toBe(true); + expect(isMarkdownMime("text/x-markdown")).toBe(true); + }); + + it("rejects everything else, including undefined", () => { + // The gate on frontmatter splitting: a YAML resource must not be split, or + // a multi-document file loses its first document. + expect(isMarkdownMime("text/yaml")).toBe(false); + expect(isMarkdownMime("application/json")).toBe(false); + expect(isMarkdownMime(undefined)).toBe(false); + }); +}); diff --git a/clients/web/src/utils/inferMimeFromUri.ts b/clients/web/src/utils/inferMimeFromUri.ts new file mode 100644 index 00000000..077d9c3b --- /dev/null +++ b/clients/web/src/utils/inferMimeFromUri.ts @@ -0,0 +1,44 @@ +/** + * Infer a resource's MIME type from its URI suffix. + * + * A pure lookup with no I/O, so it lives in `utils/`. Extracted from + * `ResourcePreviewPanel` in #2263 when the Skills screen needed the same + * inference: both panels decide which renderer to engage for a resource whose + * server omitted `mimeType`, and two copies of this table would drift. + */ + +// Map a file extension to the MIME type that drives ContentViewer's per-MIME +// renderer dispatch. MCP servers commonly omit `mimeType` (or return a generic +// `text/plain` / `application/octet-stream`), so the URI suffix is the most +// reliable signal for engaging the markdown / PDF / CSV / XML / HTML / CSS +// renderers. Order doesn't matter — suffixes are unique. +const URI_SUFFIX_MIME: ReadonlyArray = [ + [".md", "text/markdown"], + [".markdown", "text/markdown"], + [".csv", "text/csv"], + [".json", "application/json"], + [".xml", "application/xml"], + [".html", "text/html"], + [".htm", "text/html"], + [".css", "text/css"], + [".pdf", "application/pdf"], +]; + +/** + * The MIME type a URI's file extension implies, or `undefined` for an + * unrecognised suffix so callers can fall through to their own default. + */ +export function inferMimeFromUri(uri: string): string | undefined { + const path = uri.split("?")[0].split("#")[0]; + const lower = path.toLowerCase(); + for (const [suffix, mime] of URI_SUFFIX_MIME) { + if (lower.endsWith(suffix)) return mime; + } + return undefined; +} + +/** Whether an effective MIME type is Markdown, which is the only form that + * carries YAML frontmatter worth splitting off (#2263). */ +export function isMarkdownMime(mime: string | undefined): boolean { + return mime === "text/markdown" || mime === "text/x-markdown"; +}