diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx index 6b7bd17d..b60e248e 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx @@ -162,3 +162,103 @@ 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"); + +/** + * 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); + }, +}; diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index c69575f8..124c74e1 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -261,7 +261,7 @@ describe("SkillsScreen", () => { "aria-expanded", "false", ); - expect(screen.queryByText(/digest mismatch\(es\)/)).not.toBeInTheDocument(); + expect(screen.queryByText(/mismatch\(es\)/)).not.toBeInTheDocument(); await user.click(screen.getByRole("button", { name: /Verify all/ })); expect(await screen.findByText("Digest mismatch")).toBeInTheDocument(); @@ -272,7 +272,7 @@ describe("SkillsScreen", () => { expect(conformance.closest(".mantine-Accordion-item")).toContainElement( screen.getByText("Digest mismatch"), ); - expect(badgeStyle(/1 digest mismatch\(es\)/)).toContain("red"); + expect(badgeStyle(/1 mismatch\(es\)/)).toContain("red"); }); it("badges a warning-only entry yellow, not green", async () => { @@ -1085,6 +1085,44 @@ describe("SkillsScreen", () => { ).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( + , + ); + 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(); diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index 502649f3..ff798be4 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -864,15 +864,27 @@ export function SkillsScreen({ // 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( - () => + // Whether the viewer is showing the skill's OWN SKILL.md. Compared by + // identity, like every other URI comparison here. + const showingSkillMd = + previewUri !== undefined && + selectedUri !== undefined && + skillUriIdentity(previewUri) === skillUriIdentity(selectedUri); + + const previewMime = useMemo(() => { + const stated = 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], - ); + (previewUri !== undefined ? inferMimeFromUri(previewUri) : undefined); + if (stated !== undefined) return stated; + // Markdown is the right last resort for a skill's OWN `SKILL.md` — SEP-2640 + // makes that file markdown by construction. It is the WRONG one for the + // rest of a manifest, which the SEP expects to carry supporting scripts, + // examples and assets with types of their own: an extensionless, untyped + // blob would be decoded and rendered as markdown rather than falling back + // to binary. `undefined` here hands the decision to `ContentViewer`, whose + // own default is octet-stream for a blob and a text heuristic for text. + return showingSkillMd ? "text/markdown" : undefined; + }, [preview, previewUri, showingSkillMd]); // 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 @@ -924,7 +936,10 @@ export function SkillsScreen({ // // 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. + // that runs first. The badge is therefore labelled "mismatch(es)" rather than + // "digest mismatch(es)" — counting a size failure under a digest label would + // misreport it, and dropping it from the count would hide a real failure. + // The alerts below still distinguish the two by title. const mismatchCount = Object.values(fileStates).filter( (state) => state.status === "done" && state.verification.status === "mismatch", @@ -1088,7 +1103,7 @@ export function SkillsScreen({ verified result before anything had been checked. */} {mismatchCount > 0 && ( - {mismatchCount} digest mismatch(es) + {mismatchCount} mismatch(es) )} diff --git a/clients/web/src/utils/inferMimeFromUri.test.ts b/clients/web/src/utils/inferMimeFromUri.test.ts index 01a48c58..d9ee86f8 100644 --- a/clients/web/src/utils/inferMimeFromUri.test.ts +++ b/clients/web/src/utils/inferMimeFromUri.test.ts @@ -43,6 +43,20 @@ describe("isMarkdownMime", () => { 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. diff --git a/clients/web/src/utils/inferMimeFromUri.ts b/clients/web/src/utils/inferMimeFromUri.ts index 077d9c3b..76920ce2 100644 --- a/clients/web/src/utils/inferMimeFromUri.ts +++ b/clients/web/src/utils/inferMimeFromUri.ts @@ -37,8 +37,18 @@ export function inferMimeFromUri(uri: string): string | undefined { return undefined; } -/** Whether an effective MIME type is Markdown, which is the only form that - * carries YAML frontmatter worth splitting off (#2263). */ +/** + * 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 { - return mime === "text/markdown" || mime === "text/x-markdown"; + if (mime === undefined) return false; + const base = mime.split(";")[0].trim().toLowerCase(); + return base === "text/markdown" || base === "text/x-markdown"; }