fix: bound the resource file name, and move its typography into the theme (#2263)
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R4441JdNLZerCVM7mDDSfB Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
e8bbcf115a
commit
1a2e375379
3 changed files with 75 additions and 5 deletions
|
|
@ -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,
|
||||
);
|
||||
},
|
||||
};
|
||||
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -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({
|
|||
}
|
||||
>
|
||||
<Accordion.Control>
|
||||
<SectionControlsRow>
|
||||
<ResourceHeaderRow>
|
||||
<SectionHeading>Skill Resource</SectionHeading>
|
||||
{previewUri !== undefined && (
|
||||
<MonoCaption>{resourceFileName(previewUri)}</MonoCaption>
|
||||
<ResourceNameCaption title={previewUri}>
|
||||
{resourceFileName(previewUri)}
|
||||
</ResourceNameCaption>
|
||||
)}
|
||||
</SectionControlsRow>
|
||||
</ResourceHeaderRow>
|
||||
</Accordion.Control>
|
||||
<Accordion.Panel
|
||||
tabIndex={0}
|
||||
|
|
|
|||
Loading…
Reference in a new issue