From 1a2e375379a1a8c48020b275647b30245fabb981 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 6 Sep 2026 02:23:43 -0400 Subject: [PATCH] fix: bound the resource file name, and move its typography into the theme (#2263) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 9, two findings. The Skill Resource control shows the displayed file's name, and that control is PINNED — it never scrolls — so a resource URI ending in a long breakable segment grew the header. Fifth instance of the same class. It took three attempts, and the mutation table is what exposed each one. `lineClamp: 1` stopped the text wrapping but the row then wrapped the caption onto its own line (+30.6px), because flexbox wraps before it shrinks. A `nowrap` row improved that to +21.7px but Mantine's Accordion label wrapper still would not let the caption shrink, so `miw: 0` had nothing to act on. A hard `maw: "50%"` gives the ellipsis a definite width, and the control now grows by 0px. Worth recording that the FIRST version of that guard passed against the defect: it asserted an absolute `height < 120px`, which an unclamped name satisfies because it only wraps to a few lines. Comparing the control against its own baseline — measured with a short name, then re-measured after selecting the hostile one — is what makes it detect anything at all. Second finding: `SkillDescription` carried `size: "sm"`, leaving typography at the call site directly beneath a comment of mine asserting that these constants carry layout only and that typography belongs in a `ThemeText` variant. Added a `skillDescription` variant; only the clamp stays here. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB Signed-off-by: cliffhall --- .../SkillsScreen/SkillsScreen.stories.tsx | 31 ++++++++++++++ .../SkillsScreen/SkillsScreen.test.tsx | 5 ++- .../screens/SkillsScreen/SkillsScreen.tsx | 41 +++++++++++++++++-- clients/web/src/theme/Text.ts | 3 ++ 4 files changed, 75 insertions(+), 5 deletions(-) diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx index fc86c031..2c480950 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx @@ -263,6 +263,10 @@ export const LongManifest: Story = { // 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: { @@ -275,6 +279,7 @@ const hostileHeaderSkill: SkillEntry = { digest: SELF_DIGEST, size: 8, }, + { uri: HOSTILE_FILE_URI, digest: SELF_DIGEST, size: 8 }, ], }; @@ -323,6 +328,32 @@ export const HostileHeader: Story = { 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, + ); }, }; diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 39e41237..7c5843da 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -392,7 +392,10 @@ describe("SkillsScreen", () => { // 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(); + // 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(); diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index 420fc1c5..cb72458d 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -293,10 +293,41 @@ const SkillUriCaption = Text.withProps({ // documents. Three lines is enough to read a real description; the full text // stays available through the native `title` tooltip the call site passes. const SkillDescription = Text.withProps({ - size: "sm", + variant: "skillDescription", lineClamp: 3, }); +// The Skill Resource control's row. `nowrap` is load-bearing: flexbox wraps +// BEFORE it shrinks, so in a wrapping row a long file name pushes itself onto a +// second line and grows the pinned control no matter how the caption is +// clamped. With nowrap the caption's `miw: 0` can take effect and it ellipsizes +// instead. +const ResourceHeaderRow = Group.withProps({ + justify: "space-between", + wrap: "nowrap", + gap: "sm", + w: "100%", +}); + +// The displayed file's name, in the Skill Resource section's CONTROL — which is +// pinned and therefore does not scroll, so a resource URI ending in a very long +// breakable segment would grow the header itself. One line, with the full name +// on the `title` the call site passes. +const ResourceNameCaption = Text.withProps({ + variant: "monoCaption", + // `truncate` alone is not enough: the caption sits in a wrapping header row, + // so a long name whose MIN-CONTENT width exceeds the space simply pushes + // itself onto a second line and grows the control anyway. `miw: 0` lets flex + // shrink it below that width, which is what actually engages the ellipsis. + truncate: "end", + // `miw: 0` alone was not enough — Mantine's Accordion label wrapper does not + // let the row shrink, so the caption kept its content width and grew the + // control anyway. A hard cap gives the ellipsis a definite width to work + // against, which is what actually bounds the pinned header. + miw: 0, + maw: "50%", +}); + const IssueStack = Stack.withProps({ gap: "xs", }); @@ -1570,12 +1601,14 @@ export function SkillsScreen({ } > - + Skill Resource {previewUri !== undefined && ( - {resourceFileName(previewUri)} + + {resourceFileName(previewUri)} + )} - +