diff --git a/.changelog/next/changed-issue-4170.md b/.changelog/next/changed-issue-4170.md new file mode 100644 index 0000000000..110a96a860 --- /dev/null +++ b/.changelog/next/changed-issue-4170.md @@ -0,0 +1 @@ +- CollapsibleText gained a children-based max-height variant for content CSS line-clamp cannot clamp; the CoS agent card's task description now uses it (measured overflow + aria-expanded/aria-controls) instead of its own character-count heuristic diff --git a/client/src/components/cos/tabs/AgentCard.jsx b/client/src/components/cos/tabs/AgentCard.jsx index 00553d8f65..228e31f3ff 100644 --- a/client/src/components/cos/tabs/AgentCard.jsx +++ b/client/src/components/cos/tabs/AgentCard.jsx @@ -28,6 +28,7 @@ import * as api from '../../../services/api'; import OutputBlocks from '../OutputBlocks'; import MarkdownOutput from '../MarkdownOutput'; import Modal from '../../ui/Modal'; +import CollapsibleText from '../../ui/CollapsibleText'; import toast from '../../ui/Toast'; import { copyToClipboard } from '../../../lib/clipboard'; import { extractCosTaskType } from '../../../lib/cosTaskType'; @@ -56,30 +57,20 @@ function normalizeDescriptionToMarkdown(text) { .trim(); } -// Truncated, markdown-rendered task description with expand toggle -function TaskDescription({ text }) { - const [descExpanded, setDescExpanded] = useState(false); +// Markdown-rendered task description, height-clamped behind a Show more toggle. +// `line-clamp` can't clamp this — MarkdownOutput emits block children — so it +// uses CollapsibleText's max-height variant, which measures the real overflow +// instead of guessing from a character count. +function TaskDescription({ id, text }) { const md = useMemo(() => normalizeDescriptionToMarkdown(text), [text]); - const isLong = text?.length > 200; if (!text) return null; return (
-
+ - {!descExpanded && isLong && ( -
- )} -
- {isLong && ( - - )} +
); } @@ -626,7 +617,7 @@ export default function AgentCard({ agent, onPause, onKill, onDelete, onResume, )}
- + {/* JIRA ticket info */} {agent.metadata?.jiraTicketId && ( diff --git a/client/src/components/cos/tabs/AgentCard.test.jsx b/client/src/components/cos/tabs/AgentCard.test.jsx index e3f99b2452..1d4ca31900 100644 --- a/client/src/components/cos/tabs/AgentCard.test.jsx +++ b/client/src/components/cos/tabs/AgentCard.test.jsx @@ -262,3 +262,47 @@ describe('AgentCard kill confirmation (#4034)', () => { }); }); + +describe('AgentCard task description (#4170)', () => { + // jsdom reports 0 for both scrollHeight and clientHeight, so nothing measures + // as overflowing unless we force it. + const forceOverflow = () => + vi.spyOn(HTMLElement.prototype, 'scrollHeight', 'get').mockReturnValue(500); + + afterEach(() => vi.restoreAllMocks()); + + it('caps a long description behind an accessible Show more toggle', async () => { + const user = userEvent.setup(); + forceOverflow(); + + render( + + + + ); + + const box = document.getElementById(`agent-desc-${agent.id}`); + expect(box).toHaveClass('overflow-hidden'); + + // The bespoke implementation this replaced carried no aria wiring at all. + const toggle = screen.getByRole('button', { name: /Show more/ }); + expect(toggle).toHaveAttribute('aria-controls', `agent-desc-${agent.id}`); + expect(toggle).toHaveAttribute('aria-expanded', 'false'); + + await user.click(toggle); + expect(box).not.toHaveClass('overflow-hidden'); + expect(screen.getByRole('button', { name: /Show less/ })).toHaveAttribute('aria-expanded', 'true'); + }); + + it('offers no toggle for a description that fits', () => { + // The replaced implementation gated on `text.length > 200`, so a 201-char + // description that fit in the cap still rendered a no-op "Show more". + render( + + + + ); + + expect(screen.queryByRole('button', { name: /Show more/ })).not.toBeInTheDocument(); + }); +}); diff --git a/client/src/components/ui/CollapsibleText.jsx b/client/src/components/ui/CollapsibleText.jsx index 2219b1fb12..dff8e4079d 100644 --- a/client/src/components/ui/CollapsibleText.jsx +++ b/client/src/components/ui/CollapsibleText.jsx @@ -14,15 +14,26 @@ const CLAMP_CLASS = { }; /** - * Long text collapsed to a few lines with a Show more / Show less toggle. + * Long content collapsed to a short preview with a Show more / Show less toggle. * - * `lines` (default 2) picks the clamp depth; only the values in `CLAMP_CLASS` - * are supported, since Tailwind needs the literal class name in source. + * Two clamp strategies, picked by which content prop you pass: * - * `expandedContent` lets a caller swap in richer markup once the user opts in — - * e.g. a card that previews arbitrary markdown as flattened plain text (so the - * clamp works and foreign headings stay out of the page outline) but renders - * the real markdown on expand. When omitted, expanding just unclamps `text`. + * 1. **`text` (line-clamp)** — the default. `lines` (default 2) picks the clamp + * depth; only the values in `CLAMP_CLASS` are supported, since Tailwind needs + * the literal class name in source. + * 2. **`children` (max-height)** — for content CSS `line-clamp` cannot clamp: + * rendered markdown and anything else that emits *block* children, where + * `line-clamp` applies to the container's own inline content and silently + * does nothing. `maxHeight` (default `3.5rem`) caps the collapsed preview. + * Passing `children` wins over `text`. + * + * `expandedContent` lets a `text` caller swap in richer markup once the user + * opts in — e.g. a card that previews arbitrary markdown as flattened plain text + * (so the clamp works and foreign headings stay out of the page outline) but + * renders the real markdown on expand. When omitted, expanding just unclamps + * `text`. It and `expandedClassName` are `text`-mode only: a `children` caller + * already holds the rich markup, and expanding simply lifts the height cap off + * it — the children stay mounted throughout, so there is nothing to swap in. * * `forceToggle` shows the toggle even when the preview fits. It exists for the * `expandedContent` case: there, the toggle is the ONLY route to the rich @@ -32,11 +43,19 @@ const CLAMP_CLASS = { * the preview is lossy, not merely when it is truncated. * * The overflow measurement runs against the *clamped* element, so the toggle - * only appears when the text actually spills. It is recomputed on the collapsed - * path when the text changes, so an edit that shortens the text clears a stale - * toggle. A ResizeObserver re-measures on width changes (sidebar collapse, - * rotation, window resize) so text that wraps to a new line at a narrower width - * still surfaces the toggle instead of silently clamping with no affordance. + * only appears when the content actually spills. It is recomputed on the + * collapsed path when `text` changes, so an edit that shortens the text clears a + * stale toggle. A ResizeObserver re-measures on width changes (sidebar collapse, + * rotation, window resize) so content that wraps to a new line at a narrower + * width still surfaces the toggle instead of silently clamping with no + * affordance. + * + * In `children` mode the observer is attached to the *inner* wrapper as well as + * the clamped outer one. The outer element is height-capped, so growing children + * never change its box and a resize callback bound to it alone would never fire; + * the inner wrapper is uncapped, so its height tracks the real content and a + * changed child re-measures without needing `children` (a fresh element object + * every render) in the effect deps. * * Two separate guards keep the toggle from vanishing mid-expand (which would * strand the user in the expanded wall of text with no way back): the effect @@ -48,11 +67,13 @@ const CLAMP_CLASS = { * notification). Without the `|| expanded` term that in-flight callback can * measure the now-unclamped element, see no overflow, and drop the toggle. * - * `id` is required: it wires the toggle's `aria-controls` to the text it expands. + * `id` is required: it wires the toggle's `aria-controls` to the content it expands. */ export default function CollapsibleText({ id, text, + children = null, + maxHeight = '3.5rem', className = '', lines = 2, expandedContent = null, @@ -62,6 +83,8 @@ export default function CollapsibleText({ const [expanded, setExpanded] = useState(false); const [isOverflowing, setIsOverflowing] = useState(false); const ref = useRef(null); + const innerRef = useRef(null); + const hasChildren = children != null && children !== false; useEffect(() => { if (expanded) return; @@ -72,24 +95,41 @@ export default function CollapsibleText({ if (typeof ResizeObserver === 'undefined') return; const observer = new ResizeObserver(measure); observer.observe(el); + if (innerRef.current) observer.observe(innerRef.current); return () => observer.disconnect(); - }, [text, expanded]); + }, [text, hasChildren, expanded]); const clamp = CLAMP_CLASS[lines] || CLAMP_CLASS[2]; - - return ( - <> - {expanded && expandedContent ? ( -
{expandedContent}
- ) : ( -

{ + if (hasChildren) { + return ( +

- {text} -

- )} +
{children}
+
+ ); + } + if (expanded && expandedContent) { + return
{expandedContent}
; + } + return ( +

+ {text} +

+ ); + }; + + return ( + <> + {renderContent()} {(isOverflowing || expanded || forceToggle) && (