From e8bbcf115a17f608db448810c7d3140775cd58fe Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 6 Sep 2026 02:03:44 -0400 Subject: [PATCH] fix: bound every header string, and make scrollable panels keyboard-reachable (#2263) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 8: the skill URI was still unbounded in the pane's fixed header. It is the fourth instance of a pattern named in this PR's own round-7 reply — anything server-controlled and variable-height that sits beside the zero-basis accordion sizes the layout — and it was directly above the description clamped last round, which says something about naming a class not being the same as sweeping for it. So this audits the whole header rather than the reported element. All three server-controlled strings are now bounded: the name truncates (already did), the URI clamps to two lines, the description to three, and both clamps carry the full value on a `title`. `HostileHeader` asserts the class is closed — a long name, a 30-segment URI and a 400-word description, with the accordion still taking more than half the card, every section non-zero, and the pane not scrolling. That new story then caught an accessibility defect this refactor introduced and would otherwise have shipped: `scrollable-region-focusable`. Making each panel its own scroll container means a panel whose content holds no focusable element — the Conformance alerts, for instance — cannot be scrolled by keyboard at all. Every `Accordion.Panel` now carries `tabIndex={0}`. The geometric assertion lives in Storybook rather than the unit test, because a CSS bound is not observable through happy-dom's `getComputedStyle`; the unit test pins the contract that makes clamping safe instead — the full value stays reachable on a `title`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB Signed-off-by: cliffhall --- .../SkillsScreen/SkillsScreen.stories.tsx | 91 ++++++++++++++++--- .../SkillsScreen/SkillsScreen.test.tsx | 41 +++++++++ .../screens/SkillsScreen/SkillsScreen.tsx | 31 ++++++- 3 files changed, 146 insertions(+), 17 deletions(-) diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx index 89540860..fc86c031 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx @@ -185,18 +185,6 @@ const LONG_SKILL_MD = [ ), ].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. - */ // 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. @@ -271,6 +259,85 @@ export const LongManifest: Story = { }, }; +// 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); +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, + }, + ], +}; + +/** + * 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, + ); + }, +}; + +/** + * 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 () => ({ diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index b342701b..39e41237 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -362,6 +362,47 @@ describe("SkillsScreen", () => { ); }); + 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(); + 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. + expect(screen.getByTitle(hostile.uri)).toBeInTheDocument(); + 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). diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index dc2e22a3..420fc1c5 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -273,6 +273,16 @@ const MonoCaption = Text.withProps({ variant: "monoCaption", }); +// The selected skill's URI, clamped for the same reason as its description +// below — it is the OTHER server-controlled string in the pane's fixed header, +// and a URI with many breakable path segments wraps just as freely as prose. +// A skill URI is usually one line, so two is generous; the full value stays on +// the `title` the call site passes. +const SkillUriCaption = Text.withProps({ + variant: "monoCaption", + lineClamp: 2, +}); + // The skill's description, clamped. // // It sits in the pane's FIXED header, beside an accordion whose basis is zero, @@ -1120,7 +1130,9 @@ export function SkillsScreen({ {skillDisplayName(selected)} - {selected.uri} + + {selected.uri} + {/* Both actions act on the whole skill, so they live on the pane's header rather than inside a section — and a button @@ -1168,6 +1180,12 @@ export function SkillsScreen({ headers stay pinned and each open panel scrolls within its own share of the space, which is exactly what keeps this pane from scrolling as one column. */} + {/* Each `Accordion.Panel` is a scroll container under the + `skillSections` variant, so every one carries `tabIndex={0}`: + axe's `scrollable-region-focusable` requires a scrollable region + to be keyboard-reachable, and a panel whose content holds no + focusable element (the Conformance alerts, for instance) is + otherwise unscrollable without a pointer. */} - + {/* A dynamic skill's `dynamic-resources` finding is rendered here in full rather than as a bare code and @@ -1393,7 +1411,7 @@ export function SkillsScreen({ - + @@ -1520,7 +1538,7 @@ export function SkillsScreen({ Frontmatter - + {/* The raw YAML the server served, not a re-serialised object: this app carries no YAML parser, and for a @@ -1559,7 +1577,10 @@ export function SkillsScreen({ )} - + {previewError !== undefined ? ( {previewError}