modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix: bound every header string, and make scrollable panels keyboard-reachable (#2263)

Browse source 
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-06 02:03:44 -04:00
commit e8bbcf115a
3 changed files with 146 additions and 17 deletions
Download patch file Download diff file Expand all files Collapse all files

91
clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx
View file

@ -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 () => ({

41
clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx
View file

@ -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(<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.
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).

31
clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx
View file

@ -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({
<SectionControlsRow>
<Stack gap={4}>
<SkillTitle>{skillDisplayName(selected)}</SkillTitle>
<MonoCaption>{selected.uri}</MonoCaption>
<SkillUriCaption title={selected.uri}>
{selected.uri}
</SkillUriCaption>
</Stack>
{/* 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. */}
<Accordion
multiple
variant="skillSections"
@ -1205,7 +1223,7 @@ export function SkillsScreen({
)}
</InlineRow>
</Accordion.Control>
<Accordion.Panel>
<Accordion.Panel tabIndex={0}>
<ConformanceStack>
{/* 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({
</CountBadge>
</InlineRow>
</Accordion.Control>
<Accordion.Panel>
<Accordion.Panel tabIndex={0}>
<Stack gap="xs">
<ManifestTable data-testid="skill-manifest">
<Table.Thead>
@ -1520,7 +1538,7 @@ export function SkillsScreen({
<Accordion.Control>
<SectionHeading>Frontmatter</SectionHeading>
</Accordion.Control>
<Accordion.Panel>
<Accordion.Panel tabIndex={0}>
<FramedContent>
{/* 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({
)}
</SectionControlsRow>
</Accordion.Control>
<Accordion.Panel data-testid="skill-resource-viewer">
<Accordion.Panel
tabIndex={0}
data-testid="skill-resource-viewer"
>
{previewError !== undefined ? (
<Alert color="red" title="Could not read this resource">
{previewError}