Merge branch 'v2/main' into v2/fix/2219-toast-teardown-set
This commit is contained in:
Cliff Hall
GitHub
commit
6d7e67d63d
14 changed files with 2592 additions and 415 deletions
|
|
@ -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<readonly [string, string]> = [
|
||||
[".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,
|
||||
|
|
|
|||
|
|
@ -127,7 +127,12 @@ export const ConformingSkill: Story = {
|
|||
play: async ({ canvasElement }) => {
|
||||
const canvas = within(canvasElement);
|
||||
await userEvent.click(canvas.getByText("data-analysis"));
|
||||
await expect(canvas.getByText("Conforms")).toBeInTheDocument();
|
||||
// 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("No structural issues")).toBeInTheDocument();
|
||||
},
|
||||
};
|
||||
|
||||
|
|
@ -157,3 +162,275 @@ export const DigestMismatch: Story = {
|
|||
).toBeInTheDocument();
|
||||
},
|
||||
};
|
||||
|
||||
// A SKILL.md long enough to overflow the viewer. Every other fixture here is a
|
||||
// line or two, which is precisely why the layout regression this screen was
|
||||
// refactored for could not be caught in a story: with short content the viewer
|
||||
// never scrolls, so a pane that scrolls as one column looks identical to one
|
||||
// that does not (#2263).
|
||||
const LONG_SKILL_MD = [
|
||||
"---",
|
||||
"name: data-analysis",
|
||||
"description: Analyze a CSV and summarize its columns",
|
||||
"---",
|
||||
"",
|
||||
"# Data analysis",
|
||||
"",
|
||||
...Array.from(
|
||||
{ length: 40 },
|
||||
(_, i) =>
|
||||
`Paragraph ${i + 1}. Read the file as UTF-8 and sniff the delimiter from ` +
|
||||
"the header line rather than assuming a comma, because a mis-sniffed " +
|
||||
"delimiter yields a single column whose name is the entire header.\n",
|
||||
),
|
||||
].join("\n");
|
||||
|
||||
// A conforming manifest may declare up to 512 files. This is the sibling case
|
||||
// to a long document: the metadata section, not the viewer, is what holds the
|
||||
// overflowing content.
|
||||
const manyFilesSkill: SkillEntry = {
|
||||
uri: "skill://big-manifest/SKILL.md",
|
||||
frontmatter: {
|
||||
name: "big-manifest",
|
||||
description: "A conforming skill that declares a great many files",
|
||||
},
|
||||
resources: [
|
||||
selfEntry("big-manifest"),
|
||||
...Array.from({ length: 120 }, (_, i) => ({
|
||||
uri: `skill://big-manifest/file-${String(i).padStart(3, "0")}.md`,
|
||||
digest: REF_DIGEST,
|
||||
size: 15,
|
||||
})),
|
||||
],
|
||||
};
|
||||
|
||||
/**
|
||||
* The other half of the layout contract: a huge **manifest**, rather than a
|
||||
* huge document.
|
||||
*
|
||||
* A section that keeps its full intrinsic height pushes the file viewer off the
|
||||
* bottom of the pane, so reaching the file means scrolling past the manifest —
|
||||
* which is the "the file is behind the manifest" problem this screen exists to
|
||||
* end. The metadata sections must therefore shrink to their floor and scroll
|
||||
* internally, leaving the viewer on screen.
|
||||
*/
|
||||
export const LongManifest: Story = {
|
||||
args: {
|
||||
skills: [manyFilesSkill],
|
||||
onReadSkillFile: fn(async () => ({
|
||||
text: "---\nname: big-manifest\n---\n\n# Big manifest\n",
|
||||
mimeType: "text/markdown",
|
||||
})),
|
||||
},
|
||||
play: async ({ canvasElement }) => {
|
||||
const canvas = within(canvasElement);
|
||||
await userEvent.click(canvas.getByText("big-manifest"));
|
||||
const viewerControl = await canvas.findByRole("button", {
|
||||
name: /Skill Resource/,
|
||||
});
|
||||
|
||||
const detailCard = canvasElement.querySelectorAll(".mantine-Card-root")[1];
|
||||
if (!(detailCard instanceof HTMLElement)) {
|
||||
throw new Error("Detail card not found");
|
||||
}
|
||||
|
||||
// The viewer's header is ON SCREEN, not pushed below the manifest.
|
||||
const cardRect = detailCard.getBoundingClientRect();
|
||||
const viewerRect = viewerControl.getBoundingClientRect();
|
||||
await expect(viewerRect.bottom).toBeLessThanOrEqual(cardRect.bottom + 1);
|
||||
|
||||
// The manifest section gave up space rather than keeping its full height,
|
||||
// so its own panel is what scrolls.
|
||||
const resourcesControl = canvas.getByRole("button", { name: /Resources/ });
|
||||
const resourcesPanel = resourcesControl
|
||||
.closest(".mantine-Accordion-item")
|
||||
?.querySelector(".mantine-Accordion-panel");
|
||||
if (!(resourcesPanel instanceof HTMLElement)) {
|
||||
throw new Error("Resources panel not found");
|
||||
}
|
||||
await expect(resourcesPanel.scrollHeight).toBeGreaterThan(
|
||||
resourcesPanel.clientHeight,
|
||||
);
|
||||
|
||||
// And the pane still does not scroll as one column.
|
||||
await expect(detailCard.scrollHeight).toBeLessThanOrEqual(
|
||||
detailCard.clientHeight + 1,
|
||||
);
|
||||
},
|
||||
};
|
||||
|
||||
// A skill whose every server-controlled header string is hostile: a very long
|
||||
// name, a URI with many breakable segments, and a description at the upper end
|
||||
// of what SEP-2640 permits.
|
||||
const HOSTILE_NAME = "an-extremely-long-skill-name-".repeat(8);
|
||||
// A manifest entry whose FILENAME is hostile, not just its path — selecting it
|
||||
// puts that name in the Skill Resource control, which is pinned and does not
|
||||
// scroll.
|
||||
const HOSTILE_FILE_URI = `skill://${"very-long-path-segment/".repeat(30)}${"a-very-long-file-name-".repeat(10)}.md`;
|
||||
const hostileHeaderSkill: SkillEntry = {
|
||||
uri: `skill://${"very-long-path-segment/".repeat(30)}SKILL.md`,
|
||||
frontmatter: {
|
||||
name: HOSTILE_NAME,
|
||||
description: "word ".repeat(400).trim(),
|
||||
},
|
||||
resources: [
|
||||
{
|
||||
uri: `skill://${"very-long-path-segment/".repeat(30)}SKILL.md`,
|
||||
digest: SELF_DIGEST,
|
||||
size: 8,
|
||||
},
|
||||
{ uri: HOSTILE_FILE_URI, digest: SELF_DIGEST, size: 8 },
|
||||
],
|
||||
};
|
||||
|
||||
/**
|
||||
* The pane's fixed header cannot starve the accordion.
|
||||
*
|
||||
* The header is a sibling of an accordion whose flex-basis is `0`, so every
|
||||
* unbounded string in it is subtracted from the sections rather than resisted
|
||||
* by them. That has been the same defect three times in this PR — the viewer's
|
||||
* content-sized basis, the `skills/get` region, and the description — so this
|
||||
* asserts the whole class is closed rather than any one instance.
|
||||
*/
|
||||
export const HostileHeader: Story = {
|
||||
args: {
|
||||
skills: [hostileHeaderSkill],
|
||||
onReadSkillFile: fn(async () => ({
|
||||
text: "---\nname: x\n---\n\n# Body\n",
|
||||
mimeType: "text/markdown",
|
||||
})),
|
||||
},
|
||||
play: async ({ canvasElement }) => {
|
||||
const canvas = within(canvasElement);
|
||||
await userEvent.click(canvas.getAllByText(HOSTILE_NAME)[0]);
|
||||
await canvas.findByRole("button", { name: /Skill Resource/ });
|
||||
|
||||
const detailCard = canvasElement.querySelectorAll(".mantine-Card-root")[1];
|
||||
const accordion = canvasElement.querySelector(".disclosure-sections");
|
||||
if (
|
||||
!(detailCard instanceof HTMLElement) ||
|
||||
!(accordion instanceof HTMLElement)
|
||||
) {
|
||||
throw new Error("detail card or accordion not found");
|
||||
}
|
||||
|
||||
// The header takes a minority of the pane, leaving the sections the rest.
|
||||
const cardHeight = detailCard.getBoundingClientRect().height;
|
||||
const accordionHeight = accordion.getBoundingClientRect().height;
|
||||
await expect(accordionHeight).toBeGreaterThan(cardHeight * 0.5);
|
||||
|
||||
// Every section is still usable, and the pane still does not scroll.
|
||||
for (const item of accordion.querySelectorAll(
|
||||
":scope > .mantine-Accordion-item",
|
||||
)) {
|
||||
await expect(item.getBoundingClientRect().height).toBeGreaterThan(0);
|
||||
}
|
||||
await expect(detailCard.scrollHeight).toBeLessThanOrEqual(
|
||||
detailCard.clientHeight + 1,
|
||||
);
|
||||
|
||||
// Selecting the resource with the hostile FILENAME puts it in the Skill
|
||||
// Resource control, which is pinned and does not scroll — so it has to be
|
||||
// clamped too, or the header grows instead.
|
||||
// Measure the control while a SHORT name is displayed, then select the
|
||||
// hostile one: a clamped caption leaves the control the same height, an
|
||||
// unclamped one grows it. Comparing against its own baseline is what makes
|
||||
// this detect the defect — an absolute threshold does not, because even an
|
||||
// unclamped name only wraps to a few lines.
|
||||
const control = await canvas.findByRole("button", {
|
||||
name: /Skill Resource/,
|
||||
});
|
||||
const controlBefore = control.getBoundingClientRect().height;
|
||||
const accordionBefore = accordion.getBoundingClientRect().height;
|
||||
await userEvent.click(
|
||||
canvas.getByRole("button", { name: HOSTILE_FILE_URI }),
|
||||
);
|
||||
await expect(
|
||||
Math.abs(control.getBoundingClientRect().height - controlBefore),
|
||||
).toBeLessThanOrEqual(1);
|
||||
await expect(
|
||||
Math.abs(accordion.getBoundingClientRect().height - accordionBefore),
|
||||
).toBeLessThanOrEqual(1);
|
||||
await expect(detailCard.scrollHeight).toBeLessThanOrEqual(
|
||||
detailCard.clientHeight + 1,
|
||||
);
|
||||
},
|
||||
};
|
||||
|
||||
/**
|
||||
* The layout contract, asserted in a real browser.
|
||||
*
|
||||
* This is the regression the refactor exists to prevent, and it is only visible
|
||||
* with content that overflows: the file viewer must scroll **inside its own
|
||||
* panel** while its sibling sections keep usable height, rather than the whole
|
||||
* pane scrolling as one column.
|
||||
*
|
||||
* It also pins the collapse-then-reopen case, which is how the original bug
|
||||
* actually presented — the viewer's content-sized `flex-basis` crushed its
|
||||
* siblings, so collapsing it laid out correctly and reopening it broke again.
|
||||
*/
|
||||
export const LongSkillDocument: Story = {
|
||||
args: {
|
||||
onReadSkillFile: fn(async () => ({
|
||||
text: LONG_SKILL_MD,
|
||||
mimeType: "text/markdown",
|
||||
})),
|
||||
},
|
||||
play: async ({ canvasElement }) => {
|
||||
const canvas = within(canvasElement);
|
||||
await userEvent.click(canvas.getByText("data-analysis"));
|
||||
const viewerControl = await canvas.findByRole("button", {
|
||||
name: /Skill Resource/,
|
||||
});
|
||||
|
||||
const sections = () => [
|
||||
...canvasElement.querySelectorAll(
|
||||
".disclosure-sections > .mantine-Accordion-item",
|
||||
),
|
||||
];
|
||||
const geometry = () =>
|
||||
sections().map((s) => Math.round(s.getBoundingClientRect().height));
|
||||
|
||||
// Every section keeps a usable height: none is crushed to nothing by the
|
||||
// viewer's content, which is exactly what a content-sized basis did.
|
||||
const before = geometry();
|
||||
await expect(before.length).toBeGreaterThanOrEqual(3);
|
||||
for (const height of before) {
|
||||
await expect(height).toBeGreaterThan(0);
|
||||
}
|
||||
|
||||
// Sections tile in document order — none overlaps the header below it,
|
||||
// which is how the crushed layout showed up on screen.
|
||||
const rects = sections().map((s) => s.getBoundingClientRect());
|
||||
for (let i = 1; i < rects.length; i++) {
|
||||
await expect(Math.round(rects[i].top)).toBeGreaterThanOrEqual(
|
||||
Math.round(rects[i - 1].bottom) - 1,
|
||||
);
|
||||
}
|
||||
|
||||
// The viewer scrolls WITHIN its own panel rather than growing the pane.
|
||||
const viewerPanel = viewerControl
|
||||
.closest(".mantine-Accordion-item")
|
||||
?.querySelector(".mantine-Accordion-panel");
|
||||
if (!(viewerPanel instanceof HTMLElement)) {
|
||||
throw new Error("Skill Resource panel not found");
|
||||
}
|
||||
await expect(viewerPanel.scrollHeight).toBeGreaterThan(
|
||||
viewerPanel.clientHeight,
|
||||
);
|
||||
|
||||
// And the detail pane itself does not scroll as one column.
|
||||
const detailCard = canvasElement.querySelectorAll(".mantine-Card-root")[1];
|
||||
if (!(detailCard instanceof HTMLElement)) {
|
||||
throw new Error("Detail card not found");
|
||||
}
|
||||
await expect(detailCard.scrollHeight).toBeLessThanOrEqual(
|
||||
detailCard.clientHeight + 1,
|
||||
);
|
||||
|
||||
// Collapse then reopen restores the same geometry.
|
||||
await userEvent.click(viewerControl);
|
||||
await userEvent.click(viewerControl);
|
||||
await expect(geometry()).toEqual(before);
|
||||
},
|
||||
};
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import { useState } from "react";
|
||||
import { StrictMode, useState } from "react";
|
||||
import { describe, it, expect, vi } from "vitest";
|
||||
import userEvent from "@testing-library/user-event";
|
||||
import type { SkillEntry } from "@inspector/core/mcp/skillsSchemas";
|
||||
|
|
@ -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<SkillsScreenProps> = {}) {
|
|||
);
|
||||
}
|
||||
|
||||
// Mantine puts a Badge's colour on the ROOT as CSS custom properties, while
|
||||
// `getByText` matches the inner label span — so the colour has to be read from
|
||||
// the enclosing root rather than from the matched node.
|
||||
function badgeStyle(text: RegExp): string {
|
||||
const root = screen.getByText(text).closest(".mantine-Badge-root");
|
||||
return root?.getAttribute("style") ?? "";
|
||||
}
|
||||
|
||||
describe("SkillsScreen", () => {
|
||||
it("renders the empty state until a skill is selected", () => {
|
||||
renderWithMantine(<SkillsScreen {...baseProps} />);
|
||||
|
|
@ -191,14 +202,232 @@ 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(<ControlledSkillsScreen />);
|
||||
await user.click(screen.getByText("data-analysis"));
|
||||
expect(screen.getByText("Conforms")).toBeInTheDocument();
|
||||
const control = screen.getByRole("button", { name: /Conformance/ });
|
||||
expect(control).toHaveAttribute("aria-expanded", "false");
|
||||
expect(screen.queryByText("No structural issues")).not.toBeInTheDocument();
|
||||
|
||||
await user.click(control);
|
||||
expect(screen.getByText("No structural issues")).toBeInTheDocument();
|
||||
expect(screen.queryByTestId("skill-issues")).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("collapses Conformance for a clean skill selected BEFORE mount", () => {
|
||||
// `useValueChange` deliberately does not fire on the first render, so the
|
||||
// auto-collapse it drives cannot cover a screen that mounts with a skill
|
||||
// already chosen — a restored `SkillsUiState` does exactly that. The
|
||||
// initialiser has to apply the same rule, or the behaviour only starts
|
||||
// working after some later selection change (#2263).
|
||||
renderWithMantine(
|
||||
<SkillsScreen
|
||||
{...baseProps}
|
||||
ui={{ ...EMPTY_SKILLS_UI, selectedSkillUri: CLEAN_SKILL.uri }}
|
||||
/>,
|
||||
);
|
||||
expect(screen.getByRole("button", { name: /Conformance/ })).toHaveAttribute(
|
||||
"aria-expanded",
|
||||
"false",
|
||||
);
|
||||
});
|
||||
|
||||
it("opens Conformance for a skill WITH findings selected before mount", () => {
|
||||
renderWithMantine(
|
||||
<SkillsScreen
|
||||
{...baseProps}
|
||||
ui={{ ...EMPTY_SKILLS_UI, selectedSkillUri: MISMATCHED_SKILL.uri }}
|
||||
/>,
|
||||
);
|
||||
expect(screen.getByRole("button", { name: /Conformance/ })).toHaveAttribute(
|
||||
"aria-expanded",
|
||||
"true",
|
||||
);
|
||||
});
|
||||
|
||||
it("opens Conformance for an entry that has findings", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<ControlledSkillsScreen />);
|
||||
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(<ControlledSkillsScreen />);
|
||||
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(<ControlledSkillsScreen />);
|
||||
await user.click(screen.getByText("tampered"));
|
||||
expect(screen.getByRole("button", { name: /Conformance/ })).toHaveAttribute(
|
||||
"aria-expanded",
|
||||
"false",
|
||||
);
|
||||
expect(screen.queryByText(/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 mismatch\(es\)/)).toContain("red");
|
||||
});
|
||||
|
||||
it("a stale Verify all batch does not reopen Conformance on another skill", async () => {
|
||||
// `verifyRow` is called once per row by every "Verify all" worker as it
|
||||
// advances, so a batch begun on one skill keeps calling it after the user
|
||||
// has moved on. The keyed writes discard those results, but an open-state
|
||||
// update is not keyed to a manifest — so opening the section from inside
|
||||
// `verifyRow` let obsolete work mutate the current pane (#2263).
|
||||
const user = userEvent.setup();
|
||||
// Held open so the batch is still in flight when the selection changes.
|
||||
const releases: (() => void)[] = [];
|
||||
const onReadSkillFile = vi.fn(
|
||||
() =>
|
||||
new Promise<{ text: string }>((resolve) => {
|
||||
releases.push(() => resolve({ text: SELF_TEXT }));
|
||||
}),
|
||||
);
|
||||
// More rows than the concurrency cap, so workers keep pulling.
|
||||
const manyRows: SkillEntry = {
|
||||
...CLEAN_SKILL,
|
||||
uri: "skill://many/SKILL.md",
|
||||
frontmatter: { name: "many", description: "Many rows" },
|
||||
resources: Array.from({ length: 10 }, (_, i) => ({
|
||||
uri: i === 0 ? "skill://many/SKILL.md" : `skill://many/f${i}.md`,
|
||||
digest: SELF_DIGEST,
|
||||
size: textToBytes(SELF_TEXT).byteLength,
|
||||
})),
|
||||
};
|
||||
renderWithMantine(
|
||||
<ControlledSkillsScreen
|
||||
skills={[manyRows, CLEAN_SKILL]}
|
||||
onReadSkillFile={onReadSkillFile}
|
||||
/>,
|
||||
);
|
||||
await user.click(screen.getByText("many"));
|
||||
await user.click(screen.getByRole("button", { name: /Verify all/ }));
|
||||
|
||||
// Switch to a clean skill and collapse Conformance deliberately.
|
||||
await user.click(
|
||||
within(screen.getByTestId("skills-screen")).getAllByText(
|
||||
"data-analysis",
|
||||
)[0],
|
||||
);
|
||||
await waitFor(() =>
|
||||
expect(
|
||||
screen.getByRole("button", { name: /Conformance/ }),
|
||||
).toHaveAttribute("aria-expanded", "false"),
|
||||
);
|
||||
|
||||
// Let the abandoned batch's workers advance. They must not reopen it.
|
||||
for (const release of releases) release();
|
||||
await waitFor(() => expect(onReadSkillFile).toHaveBeenCalled());
|
||||
expect(screen.getByRole("button", { name: /Conformance/ })).toHaveAttribute(
|
||||
"aria-expanded",
|
||||
"false",
|
||||
);
|
||||
});
|
||||
|
||||
it("bounds every server-controlled string in the fixed header", async () => {
|
||||
// The header sits beside an accordion whose flex-basis is 0, so anything
|
||||
// unbounded here is subtracted from the sections rather than resisted by
|
||||
// them. This has been the same bug three times over (#2263) — the viewer's
|
||||
// content-sized basis, the `skills/get` region, the description — so this
|
||||
// asserts the *class* is closed rather than chasing one more instance.
|
||||
const user = userEvent.setup();
|
||||
const hostile: SkillEntry = {
|
||||
uri: `skill://${"very-long-segment/".repeat(40)}SKILL.md`,
|
||||
frontmatter: {
|
||||
name: "x".repeat(300),
|
||||
// SEP-2640 permits 1,024 characters here.
|
||||
description: "word ".repeat(400).trim(),
|
||||
},
|
||||
resources: [
|
||||
{
|
||||
uri: `skill://${"very-long-segment/".repeat(40)}SKILL.md`,
|
||||
digest: SELF_DIGEST,
|
||||
size: textToBytes(SELF_TEXT).byteLength,
|
||||
},
|
||||
],
|
||||
};
|
||||
renderWithMantine(<ControlledSkillsScreen skills={[hostile]} />);
|
||||
await user.click(screen.getAllByText("x".repeat(300))[0]);
|
||||
|
||||
// The *geometric* bound is a CSS concern and belongs in a real browser —
|
||||
// `HostileHeader` in the stories asserts the header cannot starve the
|
||||
// accordion. What is worth pinning here is the contract that makes
|
||||
// clamping safe: the full value stays reachable on a `title`, so nothing
|
||||
// is actually hidden from the user.
|
||||
// Two captions legitimately carry it: the header's URI and the Skill
|
||||
// Resource control's file name, which for the skill's own SKILL.md is the
|
||||
// same URI.
|
||||
expect(screen.getAllByTitle(hostile.uri).length).toBeGreaterThanOrEqual(1);
|
||||
expect(
|
||||
screen.getByTitle(hostile.frontmatter.description as string),
|
||||
).toBeInTheDocument();
|
||||
// And the header still shows all three, rather than dropping any.
|
||||
const detail = screen.getByTestId("skill-detail");
|
||||
expect(detail.textContent).toContain("xxxx");
|
||||
expect(detail.textContent).toContain("skill://very-long-segment");
|
||||
expect(detail.textContent).toContain("word word");
|
||||
});
|
||||
|
||||
it("badges a warning-only entry yellow, not green", async () => {
|
||||
// Green reads as "nothing to see", which would hide the only signal the
|
||||
// section carries for an entry whose findings are all warnings (#2263).
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<ControlledSkillsScreen />);
|
||||
await user.click(screen.getByText("dynamic-report"));
|
||||
// `dynamic-resources` is a warning, and the only finding on this fixture.
|
||||
const style = badgeStyle(/0 error\(s\), 1 warning\(s\)/);
|
||||
expect(style).toContain("yellow");
|
||||
expect(style).not.toContain("green");
|
||||
});
|
||||
|
||||
it("badges a clean entry green and a broken one red", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<ControlledSkillsScreen />);
|
||||
await user.click(screen.getByText("data-analysis"));
|
||||
expect(badgeStyle(/0 error\(s\), 0 warning\(s\)/)).toContain("green");
|
||||
|
||||
await user.click(screen.getByText("right-name"));
|
||||
expect(badgeStyle(/1 error\(s\), 0 warning\(s\)/)).toContain("red");
|
||||
});
|
||||
|
||||
it("shows the name/path mismatch as a distinct, named finding", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<ControlledSkillsScreen />);
|
||||
|
|
@ -207,17 +436,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(<ControlledSkillsScreen />);
|
||||
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(<ControlledSkillsScreen />);
|
||||
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(<ControlledSkillsScreen />);
|
||||
|
|
@ -262,7 +522,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 +536,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 +601,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 +612,8 @@ describe("SkillsScreen", () => {
|
|||
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
|
||||
);
|
||||
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 +646,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();
|
||||
|
|
@ -460,6 +727,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 () => {
|
||||
|
|
@ -608,19 +883,20 @@ describe("SkillsScreen", () => {
|
|||
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
|
||||
);
|
||||
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 +1080,21 @@ describe("SkillsScreen", () => {
|
|||
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
|
||||
);
|
||||
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 +1128,190 @@ 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(<ControlledSkillsScreen />);
|
||||
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(<ControlledSkillsScreen />);
|
||||
await user.click(screen.getByText("data-analysis"));
|
||||
// The heading lives on the section's control (it is the collapsible
|
||||
// section's own header), so it is queried at screen level rather than
|
||||
// inside the panel.
|
||||
const control = within(
|
||||
screen.getByRole("button", { name: /Skill Resource/ }),
|
||||
);
|
||||
expect(control.getByText("Skill Resource")).toBeInTheDocument();
|
||||
expect(control.getByText("SKILL.md")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("swaps the displayed file when a manifest URI is clicked", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<ControlledSkillsScreen />);
|
||||
await user.click(screen.getByText("data-analysis"));
|
||||
const viewer = screen.getByTestId("skill-resource-viewer");
|
||||
await waitFor(() => expect(viewer).toHaveTextContent("data-analysis"));
|
||||
|
||||
await user.click(
|
||||
screen.getByRole("button", {
|
||||
name: "skill://data-analysis/reference.md",
|
||||
}),
|
||||
);
|
||||
await waitFor(() => expect(viewer).toHaveTextContent("Column rules"));
|
||||
// The section header follows the file, and the previous contents are gone.
|
||||
expect(
|
||||
within(screen.getByRole("button", { name: /Skill Resource/ })).getByText(
|
||||
"reference.md",
|
||||
),
|
||||
).toBeInTheDocument();
|
||||
expect(viewer).not.toHaveTextContent("data-analysis");
|
||||
});
|
||||
|
||||
it("shows the frontmatter of the file on display, and hides the section when it has none", async () => {
|
||||
// Both halves come from one split (#2263), so the section can never show
|
||||
// one file's frontmatter beside another file's body — and the viewer never
|
||||
// repeats what the section is already showing.
|
||||
const user = userEvent.setup();
|
||||
const onReadSkillFile = vi.fn(async (uri: string) =>
|
||||
uri.endsWith("reference.md")
|
||||
? { text: "# Ref\n\nNo frontmatter here.\n" }
|
||||
: { text: "---\nname: data-analysis\n---\n\n# The body\n" },
|
||||
);
|
||||
renderWithMantine(
|
||||
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
|
||||
);
|
||||
await user.click(screen.getByText("data-analysis"));
|
||||
const viewer = screen.getByTestId("skill-resource-viewer");
|
||||
await waitFor(() => expect(viewer).toHaveTextContent("The body"));
|
||||
// Shown once, in its own section — not again in the viewer.
|
||||
expect(
|
||||
screen.getByRole("button", { name: /Frontmatter/ }),
|
||||
).toBeInTheDocument();
|
||||
expect(viewer).not.toHaveTextContent("name: data-analysis");
|
||||
|
||||
// reference.md has no frontmatter, so the section goes away entirely
|
||||
// rather than lingering with SKILL.md's fields.
|
||||
await user.click(
|
||||
screen.getByRole("button", {
|
||||
name: "skill://data-analysis/reference.md",
|
||||
}),
|
||||
);
|
||||
await waitFor(() =>
|
||||
expect(viewer).toHaveTextContent("No frontmatter here"),
|
||||
);
|
||||
expect(
|
||||
screen.queryByRole("button", { name: /Frontmatter/ }),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("lets a .md suffix outrank a generic declared MIME", async () => {
|
||||
// Servers routinely serve SKILL.md as `text/plain`. Letting that outrank
|
||||
// the suffix meant the file was not recognised as markdown, so its YAML
|
||||
// stayed in the viewer and the Frontmatter section disappeared — for a
|
||||
// perfectly valid skill.
|
||||
const user = userEvent.setup();
|
||||
const onReadSkillFile = vi.fn(async () => ({
|
||||
text: "---\nname: data-analysis\n---\n\n# The body\n",
|
||||
mimeType: "text/plain",
|
||||
}));
|
||||
renderWithMantine(
|
||||
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
|
||||
);
|
||||
await user.click(screen.getByText("data-analysis"));
|
||||
const viewer = screen.getByTestId("skill-resource-viewer");
|
||||
await waitFor(() => expect(viewer).toHaveTextContent("The body"));
|
||||
expect(
|
||||
screen.getByRole("button", { name: /Frontmatter/ }),
|
||||
).toBeInTheDocument();
|
||||
expect(viewer).not.toHaveTextContent("name: data-analysis");
|
||||
});
|
||||
|
||||
it("keeps a SPECIFIC declared MIME over the suffix", async () => {
|
||||
// The converse: a server that says `text/csv` for a `.md` URI knows its own
|
||||
// resource, so the declaration wins and nothing is split.
|
||||
const user = userEvent.setup();
|
||||
const onReadSkillFile = vi.fn(async () => ({
|
||||
text: "---\na,b\n---\n1,2\n",
|
||||
mimeType: "text/csv",
|
||||
}));
|
||||
renderWithMantine(
|
||||
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
|
||||
);
|
||||
await user.click(screen.getByText("data-analysis"));
|
||||
await waitFor(() =>
|
||||
expect(
|
||||
screen.queryByRole("button", { name: /Frontmatter/ }),
|
||||
).not.toBeInTheDocument(),
|
||||
);
|
||||
});
|
||||
|
||||
it("does not treat an untyped supporting resource as markdown", async () => {
|
||||
// SEP-2640 expects a manifest to carry supporting scripts, examples and
|
||||
// assets with types of their own. A markdown fallback is right for the
|
||||
// skill's OWN SKILL.md and wrong for the rest: an extensionless, untyped
|
||||
// blob would be decoded and rendered as markdown rather than as binary.
|
||||
const user = userEvent.setup();
|
||||
const asset: SkillEntry = {
|
||||
uri: "skill://assets/SKILL.md",
|
||||
frontmatter: { name: "assets", description: "Has a typeless blob" },
|
||||
resources: [
|
||||
{ uri: "skill://assets/SKILL.md", digest: SELF_DIGEST, size: 1 },
|
||||
// No suffix and no mimeType — nothing says what this is.
|
||||
{ uri: "skill://assets/payload", digest: SELF_DIGEST, size: 1 },
|
||||
],
|
||||
};
|
||||
const onReadSkillFile = vi.fn(async (uri: string) =>
|
||||
uri.endsWith("payload")
|
||||
? { blob: btoa("---\nnot: frontmatter\n---\n\nbinary-ish") }
|
||||
: { text: SELF_TEXT, mimeType: "text/markdown" },
|
||||
);
|
||||
renderWithMantine(
|
||||
<ControlledSkillsScreen
|
||||
skills={[asset]}
|
||||
onReadSkillFile={onReadSkillFile}
|
||||
/>,
|
||||
);
|
||||
await user.click(screen.getByText("assets"));
|
||||
await user.click(
|
||||
screen.getByRole("button", { name: "skill://assets/payload" }),
|
||||
);
|
||||
// Not split, so no Frontmatter section is invented for it...
|
||||
await waitFor(() =>
|
||||
expect(
|
||||
screen.queryByRole("button", { name: /Frontmatter/ }),
|
||||
).not.toBeInTheDocument(),
|
||||
);
|
||||
});
|
||||
|
||||
it("marks the row whose file the viewer is showing", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<ControlledSkillsScreen />);
|
||||
await user.click(screen.getByText("data-analysis"));
|
||||
const self = screen.getByRole("button", {
|
||||
name: "skill://data-analysis/SKILL.md",
|
||||
});
|
||||
const other = screen.getByRole("button", {
|
||||
name: "skill://data-analysis/reference.md",
|
||||
});
|
||||
// The skill's own file is what the viewer opens on, so its row is current.
|
||||
expect(self).toHaveAttribute("aria-current", "true");
|
||||
expect(other).not.toHaveAttribute("aria-current");
|
||||
|
||||
await user.click(other);
|
||||
await waitFor(() => expect(other).toHaveAttribute("aria-current", "true"));
|
||||
expect(self).not.toHaveAttribute("aria-current");
|
||||
});
|
||||
|
||||
it("reports a failed SKILL.md read", async () => {
|
||||
|
|
@ -864,10 +1321,10 @@ describe("SkillsScreen", () => {
|
|||
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
|
||||
);
|
||||
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 +1334,107 @@ describe("SkillsScreen", () => {
|
|||
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
|
||||
);
|
||||
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 sections independently collapsible", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<ControlledSkillsScreen />);
|
||||
// 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.
|
||||
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("expand-all covers sections that are not visible yet", async () => {
|
||||
// `sectionIds` holds only what renders at this instant, and "expand all"
|
||||
// used to write exactly that — so a section absent at the moment of the
|
||||
// click (Frontmatter, while the read is still in flight; Resources, on a
|
||||
// dynamic skill) was DROPPED from the open set, and arrived collapsed with
|
||||
// the control offering to expand all over again (#2263).
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<ControlledSkillsScreen />);
|
||||
// Start on the dynamic skill, which renders no Resources section at all.
|
||||
await user.click(screen.getByText("dynamic-report"));
|
||||
// Settle the auto-read before touching the toggle, so the click lands on a
|
||||
// known state rather than racing the section set.
|
||||
await waitFor(() =>
|
||||
expect(
|
||||
screen.getByRole("button", { name: /Collapse all|Expand all/ }),
|
||||
).toBeInTheDocument(),
|
||||
);
|
||||
const toggle = () =>
|
||||
screen.getByRole("button", { name: /Collapse all|Expand all/ });
|
||||
if (toggle().getAttribute("aria-label") === "Collapse all") {
|
||||
await user.click(toggle());
|
||||
}
|
||||
await user.click(screen.getByRole("button", { name: "Expand all" }));
|
||||
|
||||
// Switch to a static skill WITH findings, so the clean-entry collapse rule
|
||||
// does not overlap with what this test is about. Scoped to the sidebar:
|
||||
// with every section expanded, the skill's own name also appears in the
|
||||
// detail pane's frontmatter block.
|
||||
await user.click(
|
||||
within(screen.getByTestId("skills-screen")).getAllByText("right-name")[0],
|
||||
);
|
||||
await waitFor(() =>
|
||||
expect(screen.getByRole("button", { name: /Resources/ })).toHaveAttribute(
|
||||
"aria-expanded",
|
||||
"true",
|
||||
),
|
||||
);
|
||||
expect(screen.getByRole("button", { name: /Frontmatter/ })).toHaveAttribute(
|
||||
"aria-expanded",
|
||||
"true",
|
||||
);
|
||||
// And the control agrees that everything is open.
|
||||
expect(
|
||||
screen.getByRole("button", { name: "Collapse all" }),
|
||||
).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("toggles every section at once from the header control", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<ControlledSkillsScreen />);
|
||||
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(<ControlledSkillsScreen />);
|
||||
|
|
@ -895,14 +1449,114 @@ describe("SkillsScreen", () => {
|
|||
expect(screen.getAllByText("—")).toHaveLength(2);
|
||||
});
|
||||
|
||||
it("drops the SKILL.md preview when the selection changes", async () => {
|
||||
it("issues exactly one automatic read per selection within a mount", async () => {
|
||||
// The app renders under StrictMode, which deliberately replays effects, so
|
||||
// without a guard one selection fires two identical `resources/read` calls
|
||||
// — and in a protocol inspector a phantom request in the Protocol panel is
|
||||
// worse than a wasted round trip: the tool misreports the conversation
|
||||
// (#2263). The scope is deliberately one MOUNT: a `ScreenStage` remount
|
||||
// mints a fresh ref and reads again, which is correct, because the preview
|
||||
// bytes are local state and died with the same unmount.
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<ControlledSkillsScreen />);
|
||||
const onReadSkillFile = vi.fn(async () => ({ text: SELF_TEXT }));
|
||||
// Mounted with the skill ALREADY selected — a restored `SkillsUiState`.
|
||||
//
|
||||
// ⚠️ This pins the CONTRACT (one automatic read per selection) rather than
|
||||
// guarding it: this environment does not double-invoke mount effects, so
|
||||
// the test passes with or without `autoReadKey`. Do not read a pass here as
|
||||
// evidence the duplicate-read defect is fixed; that is only observable in a
|
||||
// real dev-mode browser. The reappearance test below IS a guard.
|
||||
renderWithMantine(
|
||||
<StrictMode>
|
||||
<ControlledSkillsScreen
|
||||
onReadSkillFile={onReadSkillFile}
|
||||
ui={{ ...EMPTY_SKILLS_UI, selectedSkillUri: CLEAN_SKILL.uri }}
|
||||
/>
|
||||
</StrictMode>,
|
||||
);
|
||||
await waitFor(() => expect(onReadSkillFile).toHaveBeenCalledTimes(1));
|
||||
|
||||
// A genuine selection change is a different manifest, so it reads once more.
|
||||
await user.click(screen.getAllByText("tampered")[0]);
|
||||
await waitFor(() => expect(onReadSkillFile).toHaveBeenCalledTimes(2));
|
||||
});
|
||||
|
||||
it("re-reads when the selected entry leaves the list and comes back", async () => {
|
||||
// A refresh in flight (or a disconnect) can empty `skills` while the
|
||||
// selection persists. The render invalidates the preview, so the viewer is
|
||||
// blank — and when the IDENTICAL entry returns its `manifestKey` matches
|
||||
// what the guard still holds. Without clearing the guard on the way out,
|
||||
// the read is skipped and the viewer stays permanently empty (#2263).
|
||||
const onReadSkillFile = vi.fn(async () => ({
|
||||
text: "---\nname: data-analysis\n---\n\nreloaded-body\n",
|
||||
}));
|
||||
const selectedUi = {
|
||||
...EMPTY_SKILLS_UI,
|
||||
selectedSkillUri: CLEAN_SKILL.uri,
|
||||
};
|
||||
const { rerender } = renderWithMantine(
|
||||
<ControlledSkillsScreen
|
||||
onReadSkillFile={onReadSkillFile}
|
||||
ui={selectedUi}
|
||||
/>,
|
||||
);
|
||||
await waitFor(() => expect(onReadSkillFile).toHaveBeenCalledTimes(1));
|
||||
await waitFor(() =>
|
||||
expect(screen.getByTestId("skill-resource-viewer")).toHaveTextContent(
|
||||
"reloaded-body",
|
||||
),
|
||||
);
|
||||
|
||||
rerender(
|
||||
<ControlledSkillsScreen
|
||||
onReadSkillFile={onReadSkillFile}
|
||||
skills={[]}
|
||||
ui={selectedUi}
|
||||
/>,
|
||||
);
|
||||
rerender(
|
||||
<ControlledSkillsScreen
|
||||
onReadSkillFile={onReadSkillFile}
|
||||
ui={selectedUi}
|
||||
/>,
|
||||
);
|
||||
|
||||
await waitFor(() => expect(onReadSkillFile).toHaveBeenCalledTimes(2));
|
||||
await waitFor(() =>
|
||||
expect(screen.getByTestId("skill-resource-viewer")).toHaveTextContent(
|
||||
"reloaded-body",
|
||||
),
|
||||
);
|
||||
});
|
||||
|
||||
it("re-points the viewer at the newly selected skill's own file", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onReadSkillFile = vi.fn(async (uri: string) => ({
|
||||
text: `contents of ${uri}\n`,
|
||||
}));
|
||||
renderWithMantine(
|
||||
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
|
||||
);
|
||||
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();
|
||||
// Re-queried, not reused: the accordion is keyed by the manifest so that a
|
||||
// skill change gives every panel a fresh scroll container (#2263), which
|
||||
// means the node captured above is detached and frozen on the old content.
|
||||
await waitFor(() =>
|
||||
expect(screen.getByTestId("skill-resource-viewer")).toHaveTextContent(
|
||||
"contents of skill://tampered/SKILL.md",
|
||||
),
|
||||
);
|
||||
expect(screen.getByTestId("skill-resource-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 +1686,15 @@ describe("SkillsScreen", () => {
|
|||
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
|
||||
);
|
||||
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 +1710,10 @@ describe("SkillsScreen", () => {
|
|||
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
|
||||
);
|
||||
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();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
File diff suppressed because it is too large
Load diff
|
|
@ -216,9 +216,10 @@ export interface ServerCommands {
|
|||
uri: string,
|
||||
) => Promise<Awaited<ReturnType<InspectorClient["readResource"]>>["result"]>;
|
||||
/**
|
||||
* Read one skill file's contents (SEP-2640), for digest verification. Returns
|
||||
* the single content block that answers the URI, narrowed to the two fields
|
||||
* the digest is taken over.
|
||||
* Read one skill file's contents (SEP-2640) — for display in the Skills
|
||||
* screen's viewer, and for digest verification. Returns the single content
|
||||
* block that answers the URI, narrowed to the two fields the digest is taken
|
||||
* over.
|
||||
*/
|
||||
onReadSkillFile: (uri: string) => Promise<SkillFileContents>;
|
||||
/** Re-fetch one skill entry through `skills/get` (SEP-2640). */
|
||||
|
|
@ -947,9 +948,13 @@ export function useServerCommands({
|
|||
runCommandInBackground(() => resourcesPagination.onLoadMore(), "ambient"),
|
||||
[resourcesPagination, runCommandInBackground],
|
||||
);
|
||||
// Skill files are fetched on demand, never pre-fetched: SEP-2640 is explicit
|
||||
// that a `resources/read` of a skill file is not a load and confers no
|
||||
// standing, so the Inspector reads only what the user asks it to verify.
|
||||
// Skill files are still never pre-fetched in bulk — nothing walks the
|
||||
// manifest reading everything. What IS read without being asked is the single
|
||||
// `SKILL.md` of whichever skill is selected, so the file is on screen (#2263);
|
||||
// SEP-2640 is explicit that a `resources/read` of a skill file is not a load
|
||||
// and confers no standing, so that claims nothing on the user's behalf.
|
||||
// Digest verification remains strictly on demand.
|
||||
//
|
||||
// Routed through `onReadResourceContents` so a skill read gets the same
|
||||
// auth-recovery retry every other read does.
|
||||
const onReadSkillFile = useCallback(
|
||||
|
|
|
|||
|
|
@ -14,7 +14,7 @@ export const ThemeAccordion = Accordion.extend({
|
|||
// and a filled background when the section is open (`aria-expanded`).
|
||||
// Pair it with `chevron={<RiArrowRightSLine />}` 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,34 @@ export const ThemeAccordion = Accordion.extend({
|
|||
};
|
||||
return {};
|
||||
},
|
||||
// `skillSections` is `disclosure` plus a scrolling root (#2263).
|
||||
//
|
||||
// The Skills pane's metadata sections use `flex: 0 1 auto` with a `mih`
|
||||
// floor: they give up space until they reach that floor and then scroll their
|
||||
// own panels, which is what leaves the file viewer its share no matter how
|
||||
// large a manifest or findings list gets.
|
||||
//
|
||||
// The scrolling root is the FALLBACK for when even the combined floors do not
|
||||
// fit — with several sections open in a short window there is no arrangement
|
||||
// that shows everything, and scrolling the stack at a section boundary beats
|
||||
// crushing a panel below its floor.
|
||||
//
|
||||
// ⚠️ The floor is load-bearing, not decoration. Without it a section can be
|
||||
// squeezed far below its content and slice it mid-line; with `flex-shrink: 0`
|
||||
// instead, a 512-row manifest keeps its full height and pushes the viewer off
|
||||
// the bottom of the pane. `SECTION_FLEX` in `SkillsScreen.tsx` records both
|
||||
// failures — this variant only works in combination with it.
|
||||
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 {};
|
||||
},
|
||||
});
|
||||
|
|
|
|||
14
clients/web/src/theme/Table.ts
Normal file
14
clients/web/src/theme/Table.ts
Normal file
|
|
@ -0,0 +1,14 @@
|
|||
import { Table } from "@mantine/core";
|
||||
|
||||
export const ThemeTable = Table.extend({
|
||||
// The Skills resource manifest (#2263). `striped`, `withTableBorder`,
|
||||
// `highlightOnHover` and `verticalSpacing` are Mantine's own API and stay
|
||||
// props at the call site; the font size is a flat CSS property, so it lives
|
||||
// here rather than as an `fz` prop on the screen's `.withProps()` constant.
|
||||
styles: (_theme, props) => {
|
||||
if (props.variant === "manifest") {
|
||||
return { table: { fontSize: "var(--mantine-font-size-xs)" } };
|
||||
}
|
||||
return {};
|
||||
},
|
||||
});
|
||||
|
|
@ -42,6 +42,45 @@ 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 === "skillDescription") {
|
||||
return { root: { fontSize: "var(--mantine-font-size-sm)" } };
|
||||
}
|
||||
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"
|
||||
|
|
|
|||
|
|
@ -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";
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
138
clients/web/src/utils/inferMimeFromUri.test.ts
Normal file
138
clients/web/src/utils/inferMimeFromUri.test.ts
Normal file
|
|
@ -0,0 +1,138 @@
|
|||
import { describe, it, expect } from "vitest";
|
||||
import {
|
||||
inferMimeFromUri,
|
||||
isGenericMime,
|
||||
isMarkdownMime,
|
||||
} from "./inferMimeFromUri";
|
||||
|
||||
describe("isGenericMime", () => {
|
||||
it("names the types a server sends when it does not really know", () => {
|
||||
expect(isGenericMime("text/plain")).toBe(true);
|
||||
expect(isGenericMime("application/octet-stream")).toBe(true);
|
||||
// Normalised like `isMarkdownMime`, so parameters and casing still count.
|
||||
expect(isGenericMime("text/plain; charset=utf-8")).toBe(true);
|
||||
expect(isGenericMime("TEXT/PLAIN")).toBe(true);
|
||||
});
|
||||
|
||||
it("does not treat a specific type as generic", () => {
|
||||
// A server that says `text/markdown` or `text/csv` knows its own resource,
|
||||
// and that declaration must outrank a URI suffix.
|
||||
expect(isGenericMime("text/markdown")).toBe(false);
|
||||
expect(isGenericMime("text/csv")).toBe(false);
|
||||
expect(isGenericMime("application/json")).toBe(false);
|
||||
expect(isGenericMime(undefined)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
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("matches a percent-encoded suffix", () => {
|
||||
// `reference%2Emd` names the same file as `reference.md`, and
|
||||
// `skillUriIdentity` already treats those spellings as equivalent — so
|
||||
// matching the raw string here disagreed with the rest of the app and left
|
||||
// an encoded `.md` with no MIME, no markdown renderer and no split.
|
||||
expect(inferMimeFromUri("skill://a/reference%2Emd")).toBe("text/markdown");
|
||||
expect(inferMimeFromUri("skill://a/report%2Epdf")).toBe("application/pdf");
|
||||
// A percent-encoded path segment separator is decoded too.
|
||||
expect(inferMimeFromUri("skill://a/docs%2Fnotes.md")).toBe("text/markdown");
|
||||
});
|
||||
|
||||
it("inspects the PATH, not the whole URI", () => {
|
||||
// An authority ending in a mapped suffix is not a filename:
|
||||
// `https://documentation.md` has a pathname of `/` and no extension, and
|
||||
// routing a root resource into the markdown renderer on that basis is
|
||||
// wrong.
|
||||
expect(inferMimeFromUri("https://documentation.md")).toBeUndefined();
|
||||
expect(inferMimeFromUri("https://documentation.md/")).toBeUndefined();
|
||||
// The same host WITH a real markdown path still resolves.
|
||||
expect(inferMimeFromUri("https://documentation.md/a/readme.md")).toBe(
|
||||
"text/markdown",
|
||||
);
|
||||
// Non-special schemes SEP-2640 allows parse the same way.
|
||||
expect(inferMimeFromUri("skill://data-analysis/SKILL.md")).toBe(
|
||||
"text/markdown",
|
||||
);
|
||||
});
|
||||
|
||||
it("decodes escape runs independently, so one bad octet cannot poison the path", () => {
|
||||
// `%FF` is a syntactically valid triplet that is not valid UTF-8, so a
|
||||
// single `decodeURIComponent` over the whole path throws and abandons the
|
||||
// rest — leaving the `%2E` in a perfectly acceptable URI undecoded.
|
||||
expect(inferMimeFromUri("skill://a/%FF/reference%2Emd")).toBe(
|
||||
"text/markdown",
|
||||
);
|
||||
expect(inferMimeFromUri("skill://a/%FF/report%2Epdf")).toBe(
|
||||
"application/pdf",
|
||||
);
|
||||
});
|
||||
|
||||
it("falls back to the raw path on a malformed escape", () => {
|
||||
// `decodeURIComponent` throws on `%zz` or a lone `%`, and a server can send
|
||||
// either. A MIME guess is the wrong place to raise.
|
||||
expect(inferMimeFromUri("skill://a/bad%zz.md")).toBe("text/markdown");
|
||||
expect(inferMimeFromUri("skill://a/100%.md")).toBe("text/markdown");
|
||||
expect(inferMimeFromUri("skill://a/bad%zz.bin")).toBeUndefined();
|
||||
});
|
||||
|
||||
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("accepts a MIME carrying parameters", () => {
|
||||
// `text/markdown; charset=utf-8` is a perfectly ordinary response, and
|
||||
// ContentViewer accepts it. Rejecting it here skipped the frontmatter
|
||||
// split, so the fence stayed in the document AND the Frontmatter section
|
||||
// vanished.
|
||||
expect(isMarkdownMime("text/markdown; charset=utf-8")).toBe(true);
|
||||
expect(isMarkdownMime("text/markdown;charset=UTF-8")).toBe(true);
|
||||
});
|
||||
|
||||
it("is case-insensitive", () => {
|
||||
expect(isMarkdownMime("TEXT/MARKDOWN")).toBe(true);
|
||||
expect(isMarkdownMime("Text/X-Markdown; charset=utf-8")).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);
|
||||
});
|
||||
});
|
||||
128
clients/web/src/utils/inferMimeFromUri.ts
Normal file
128
clients/web/src/utils/inferMimeFromUri.ts
Normal file
|
|
@ -0,0 +1,128 @@
|
|||
/**
|
||||
* 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<readonly [string, string]> = [
|
||||
[".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 lower = decodePercentTriplets(pathOf(uri)).toLowerCase();
|
||||
for (const [suffix, mime] of URI_SUFFIX_MIME) {
|
||||
if (lower.endsWith(suffix)) return mime;
|
||||
}
|
||||
return undefined;
|
||||
}
|
||||
|
||||
/**
|
||||
* The **path** component of a URI, not the whole string.
|
||||
*
|
||||
* Stripping only the query and fragment left the authority in, so a host that
|
||||
* happened to end in a mapped suffix was read as a filename:
|
||||
* `https://documentation.md` has a pathname of `/` and no extension at all, yet
|
||||
* matched `.md` and routed a root resource into the markdown renderer.
|
||||
*
|
||||
* `new URL` handles this for every hierarchical URI including the non-special
|
||||
* schemes SEP-2640 allows (`skill://a/SKILL.md` → `/SKILL.md`). It throws on
|
||||
* anything it cannot parse — a bare relative name like `notes.md`, say — so
|
||||
* that case keeps the old string handling.
|
||||
*/
|
||||
function pathOf(uri: string): string {
|
||||
try {
|
||||
return new URL(uri).pathname;
|
||||
} catch {
|
||||
return uri.split("?")[0].split("#")[0];
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Percent-decode a path for matching, one escape run at a time.
|
||||
*
|
||||
* A URI may percent-encode unreserved characters, so `reference%2Emd` names the
|
||||
* same file as `reference.md` — and `skillUriIdentity` in `core/mcp/skills.ts`
|
||||
* already treats those spellings as equivalent.
|
||||
*
|
||||
* Decoding the path in one `decodeURIComponent` call was not enough. That
|
||||
* throws on a malformed escape (`%zz`) *and* on a syntactically valid octet
|
||||
* that is not valid UTF-8 (`%FF`), and a single such octet anywhere in the path
|
||||
* then defeated decoding for the whole string — `skill://a/%FF/reference%2Emd`
|
||||
* is a perfectly acceptable URI whose `%2E` would never be seen.
|
||||
*
|
||||
* So each run of escapes is decoded independently, and a run that cannot be
|
||||
* decoded as UTF-8 falls back to decoding its **ASCII** octets individually.
|
||||
* That is enough for suffix matching, where every character that matters is
|
||||
* ASCII, and it leaves a byte it cannot interpret untouched rather than
|
||||
* guessing.
|
||||
*/
|
||||
function decodePercentTriplets(path: string): string {
|
||||
return path.replace(/(?:%[0-9A-Fa-f]{2})+/g, (run) => {
|
||||
try {
|
||||
return decodeURIComponent(run);
|
||||
} catch {
|
||||
return run.replace(/%([0-9A-Fa-f]{2})/g, (raw, hex: string) => {
|
||||
const code = Number.parseInt(hex, 16);
|
||||
return code < 0x80 ? String.fromCharCode(code) : raw;
|
||||
});
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Types a server sends when it does not really know, or did not bother.
|
||||
*
|
||||
* `ResourcePreviewPanel` already records the underlying observation — servers
|
||||
* "commonly omit `mimeType` (or return a generic `text/plain` /
|
||||
* `application/octet-stream`), so the URI suffix is the most reliable signal".
|
||||
* Naming them lets a caller act on that: a declared type this generic is weaker
|
||||
* evidence than a `.md` suffix, while a specific declared type still wins.
|
||||
*/
|
||||
const GENERIC_MIMES = new Set(["text/plain", "application/octet-stream"]);
|
||||
|
||||
/**
|
||||
* Whether a declared MIME is too generic to outrank a URI suffix.
|
||||
*
|
||||
* Normalised the same way `isMarkdownMime` is, so `text/plain; charset=utf-8`
|
||||
* counts as generic too.
|
||||
*/
|
||||
export function isGenericMime(mime: string | undefined): boolean {
|
||||
if (mime === undefined) return false;
|
||||
return GENERIC_MIMES.has(mime.split(";")[0].trim().toLowerCase());
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether an effective MIME type is Markdown — the only form that carries YAML
|
||||
* frontmatter worth splitting off (#2263).
|
||||
*
|
||||
* Normalised before comparing, because a server may answer
|
||||
* `text/markdown; charset=utf-8` or `TEXT/MARKDOWN`, and `ContentViewer`
|
||||
* accepts both. Comparing the raw string rejected them, which skipped the
|
||||
* split: the frontmatter stayed in the rendered document AND the Frontmatter
|
||||
* section vanished, for a response that was perfectly valid.
|
||||
*/
|
||||
export function isMarkdownMime(mime: string | undefined): boolean {
|
||||
if (mime === undefined) return false;
|
||||
const base = mime.split(";")[0].trim().toLowerCase();
|
||||
return base === "text/markdown" || base === "text/x-markdown";
|
||||
}
|
||||
66
clients/web/src/utils/splitSkillFile.test.ts
Normal file
66
clients/web/src/utils/splitSkillFile.test.ts
Normal file
|
|
@ -0,0 +1,66 @@
|
|||
import { describe, it, expect } from "vitest";
|
||||
import { splitSkillFile } from "./splitSkillFile";
|
||||
|
||||
describe("splitSkillFile", () => {
|
||||
it("separates a leading frontmatter fence from the body", () => {
|
||||
expect(splitSkillFile("---\nname: a\n---\n\n# Title\n\nBody\n")).toEqual({
|
||||
frontmatter: "name: a",
|
||||
body: "# Title\n\nBody\n",
|
||||
});
|
||||
});
|
||||
|
||||
it("reports no frontmatter for a file that has none", () => {
|
||||
expect(splitSkillFile("# Title\n\nBody\n")).toEqual({
|
||||
body: "# Title\n\nBody\n",
|
||||
});
|
||||
});
|
||||
|
||||
it("leaves an unterminated opening fence alone rather than eating the file", () => {
|
||||
// `---` with no closing fence is not frontmatter. Treating it as such would
|
||||
// truncate the document to nothing, which is far worse than showing it.
|
||||
const text = "---\nnot really frontmatter\n\n# Title\n";
|
||||
expect(splitSkillFile(text)).toEqual({ body: text });
|
||||
});
|
||||
|
||||
it("does not treat a horizontal rule mid-file as frontmatter", () => {
|
||||
const text = "# Title\n\n---\n\nAfter the rule\n";
|
||||
expect(splitSkillFile(text)).toEqual({ body: text });
|
||||
});
|
||||
|
||||
it("handles CRLF line endings", () => {
|
||||
expect(splitSkillFile("---\r\nname: a\r\n---\r\n\r\n# Title\r\n")).toEqual({
|
||||
frontmatter: "name: a",
|
||||
body: "# Title\r\n",
|
||||
});
|
||||
});
|
||||
|
||||
it("returns an empty body when the file is nothing but frontmatter", () => {
|
||||
expect(splitSkillFile("---\nname: a\n---\n")).toEqual({
|
||||
frontmatter: "name: a",
|
||||
body: "",
|
||||
});
|
||||
});
|
||||
|
||||
it("handles a closing fence with no trailing newline", () => {
|
||||
// The file ends ON the fence, so there is no newline after it to split at.
|
||||
expect(splitSkillFile("---\nname: a\n---")).toEqual({
|
||||
frontmatter: "name: a",
|
||||
body: "",
|
||||
});
|
||||
});
|
||||
|
||||
it("keeps a body that follows the fence with no blank line", () => {
|
||||
// The blank line between fence and body is a convention, not a rule —
|
||||
// stripping unconditionally would eat the first line of a file without one.
|
||||
expect(splitSkillFile("---\nname: a\n---\nBody\n")).toEqual({
|
||||
frontmatter: "name: a",
|
||||
body: "Body\n",
|
||||
});
|
||||
});
|
||||
|
||||
it("keeps every frontmatter line, not just the first", () => {
|
||||
expect(
|
||||
splitSkillFile("---\nname: a\ndescription: b\n---\n\nBody\n"),
|
||||
).toEqual({ frontmatter: "name: a\ndescription: b", body: "Body\n" });
|
||||
});
|
||||
});
|
||||
55
clients/web/src/utils/splitSkillFile.ts
Normal file
55
clients/web/src/utils/splitSkillFile.ts
Normal file
|
|
@ -0,0 +1,55 @@
|
|||
/**
|
||||
* Split a skill file into its YAML frontmatter and its body (#2263).
|
||||
*
|
||||
* A pure transform with no I/O and no subsystem of its own, so it lives in
|
||||
* `utils/` rather than `lib/` — and in its own module rather than in
|
||||
* `SkillsScreen.tsx`, because a component file that also exports a function
|
||||
* defeats React Fast Refresh (`react-refresh/only-export-components`).
|
||||
*/
|
||||
|
||||
export interface SkillFileParts {
|
||||
/**
|
||||
* The raw YAML between the fences, fences excluded — `undefined` when the
|
||||
* file has no frontmatter at all. Raw rather than parsed: this app carries no
|
||||
* YAML parser, and showing the bytes the server actually served is the more
|
||||
* useful answer for a conformance tool anyway.
|
||||
*/
|
||||
frontmatter?: string;
|
||||
/** Everything after the closing fence, or the whole file when there is none. */
|
||||
body: string;
|
||||
}
|
||||
|
||||
/**
|
||||
* Separate a leading YAML frontmatter fence from the rest of a skill file.
|
||||
*
|
||||
* The Skills screen renders the two halves in different places — the
|
||||
* frontmatter in its own collapsible section, the body in the file viewer — and
|
||||
* deriving both from **one** split is what stops them disagreeing: the section
|
||||
* can never show one file's frontmatter while the viewer shows another's, and a
|
||||
* file with no frontmatter cannot leave a stale section on screen.
|
||||
*
|
||||
* It also matters for rendering: the markdown renderer has no frontmatter
|
||||
* support, so an un-split `---\nname: …\n---` is read as a setext heading and
|
||||
* painted as a title above the document's real one.
|
||||
*
|
||||
* Two deliberate conservatisms, because this must never eat content:
|
||||
*
|
||||
* - Only a fence at the very **start** of the file counts. A `---` anywhere
|
||||
* else is a horizontal rule and is left in the body.
|
||||
* - A file that opens with `---` but never closes the fence is **not**
|
||||
* frontmatter; the whole file is returned as the body rather than being
|
||||
* truncated to nothing.
|
||||
*/
|
||||
export function splitSkillFile(text: string): SkillFileParts {
|
||||
if (!/^---[ \t]*\r?\n/.test(text)) return { body: text };
|
||||
const rest = text.slice(text.indexOf("\n") + 1);
|
||||
const close = rest.search(/^---[ \t]*\r?$/m);
|
||||
if (close === -1) return { body: text };
|
||||
const frontmatter = rest.slice(0, close).replace(/\r?\n$/, "");
|
||||
const after = rest.slice(close);
|
||||
const newline = after.indexOf("\n");
|
||||
if (newline === -1) return { frontmatter, body: "" };
|
||||
// Drop the blank line conventionally left between the fence and the body, so
|
||||
// the document does not open with dead space.
|
||||
return { frontmatter, body: after.slice(newline + 1).replace(/^\r?\n/, "") };
|
||||
}
|
||||
Loading…
Reference in a new issue