fix: finish the CollapsibleText children-mode review fixes (#4170)

.changelog/next/changed-issue-4170.md

@@ -1 +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
+- 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, and keyboard focus landing on a link inside a clipped preview now expands it rather than scrolling it out of reach

client/src/components/ui/CollapsibleText.jsx

@@ -1,4 +1,4 @@
-import { useState, useRef, useEffect } from 'react';
+import { Children, useState, useRef, useEffect } from 'react';
 import { ChevronDown, ChevronUp } from 'lucide-react';
 
 // Tailwind scans source for literal class names, so the clamp variants must
@@ -55,7 +55,11 @@ const CLAMP_CLASS = {
  * 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.
+ * every render, which would churn the observer on every parent re-render) in the
+ * effect deps. Switching *modes* still has to re-run it, though — the rendered
+ * element swaps between `<p>` and the capped `<div>`, so the effect's captured
+ * element would otherwise stay bound to the detached one and never measure.
+ * That's what `hasChildren` (a stable boolean) is doing in the 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
@@ -69,6 +73,11 @@ const CLAMP_CLASS = {
  *
  * `id` is required: it wires the toggle's `aria-controls` to the content it expands.
  */
+
+// The 1px slack absorbs sub-pixel line-height rounding, which otherwise reports
+// a phantom overflow on content that fits exactly.
+const overflows = el => el.scrollHeight > el.clientHeight + 1;
+
 export default function CollapsibleText({
   id,
   text,
@@ -84,13 +93,24 @@ export default function CollapsibleText({
   const [isOverflowing, setIsOverflowing] = useState(false);
   const ref = useRef(null);
   const innerRef = useRef(null);
-  const hasChildren = children != null && children !== false;
+  // An empty list or an empty string is *no* children, not "children that
+  // happen to be blank" — a caller doing `<CollapsibleText text={fallback}>
+  // {items.map(…)}</CollapsibleText>` over an empty list must get the text
+  // fallback, not an empty capped box with its `text` silently dropped.
+  // Array *length* is the wrong signal: `items.map(i => i.show ? <Row/> : null)`
+  // over an all-hidden list yields `[null]` — length 1, renders nothing.
+  // `Children.toArray` drops exactly those non-rendering placeholders (`null`,
+  // `undefined`, booleans) and flattens nested arrays; empty/whitespace strings
+  // survive it, so they're filtered here too.
+  const hasChildren = Children.toArray(children).some(
+    child => typeof child !== 'string' || child.trim() !== ''
+  );
 
   useEffect(() => {
     if (expanded) return;
     const el = ref.current;
     if (!el) return;
-    const measure = () => setIsOverflowing(el.scrollHeight > el.clientHeight + 1);
+    const measure = () => setIsOverflowing(overflows(el));
     measure();
     if (typeof ResizeObserver === 'undefined') return;
     const observer = new ResizeObserver(measure);
@@ -100,6 +120,7 @@ export default function CollapsibleText({
   }, [text, hasChildren, expanded]);
 
   const clamp = CLAMP_CLASS[lines] || CLAMP_CLASS[2];
+
   const renderContent = () => {
     if (hasChildren) {
       return (
@@ -108,6 +129,18 @@ export default function CollapsibleText({
           id={id}
           className={`break-words ${className} ${expanded ? '' : 'overflow-hidden'}`}
           style={expanded ? undefined : { maxHeight }}
+          // `overflow-hidden` is a scroll container with no visible scrollbar,
+          // and children may hold links, buttons or a horizontally-scrollable
+          // <pre>. Tabbing to one below the cap would scroll the preview to
+          // reveal it with no way to scroll back — and `aria-expanded="false"`
+          // would be a lie, since the content is fully in the tab order.
+          // Expanding on focus keeps the claim honest and the target on screen.
+          // Gated on real overflow: content that fits is never clipped, so
+          // expanding it would only mint a no-op "Show less" into the tab order.
+          // Measured live rather than read off `isOverflowing`: a descendant with
+          // `autoFocus` takes focus during commit, before the passive measure
+          // effect has run, so the state flag is still false on that first focus.
+          onFocus={() => { if (ref.current && overflows(ref.current)) setExpanded(true); }}
         >
           <div ref={innerRef}>{children}</div>
         </div>

client/src/components/ui/CollapsibleText.test.jsx

@@ -237,6 +237,106 @@ describe('CollapsibleText children (max-height) variant', () => {
     expect(observed).toContain(box.firstElementChild);
   });
 
+  it('measures children that arrive after mount', () => {
+    // Switching modes swaps the rendered element from <p> to the capped <div>,
+    // so an effect that doesn't re-run stays bound to the now-detached <p> —
+    // which has no layout and never gets a resize callback. The content would
+    // clip at maxHeight with no Show more affordance at all.
+    const spy = forceOverflow();
+    spy.mockReturnValue(0);
+    const { rerender } = render(<CollapsibleText id="c8">{null}</CollapsibleText>);
+    expect(screen.queryByRole('button')).not.toBeInTheDocument();
+
+    spy.mockReturnValue(500);
+    rerender(<CollapsibleText id="c8"><p>tall rendered markdown</p></CollapsibleText>);
+    expect(screen.getByRole('button', { name: /Show more/ })).toBeInTheDocument();
+  });
+
+  it('treats an empty child list as no children so the text fallback still renders', () => {
+    // `<CollapsibleText text={fallback}>{items.map(…)}</CollapsibleText>` over an
+    // empty list must not yield an empty capped box with `text` silently dropped.
+    render(<CollapsibleText id="c9" text="plain fallback">{[]}</CollapsibleText>);
+    expect(screen.getByText('plain fallback')).toBeInTheDocument();
+    expect(document.getElementById('c9')).toHaveClass('line-clamp-2');
+  });
+
+  it('treats a list of non-rendering children as no children', () => {
+    // `items.map(i => i.show ? <Row/> : null)` over an all-hidden list yields
+    // `[null]` — length 1, renders nothing. Counting array length would take the
+    // children path and drop `text` into an empty capped box.
+    render(
+      <CollapsibleText id="c9b" text="plain fallback">{[null, false, undefined, '  ']}</CollapsibleText>
+    );
+    expect(screen.getByText('plain fallback')).toBeInTheDocument();
+    expect(document.getElementById('c9b')).toHaveClass('line-clamp-2');
+  });
+
+  it('takes the children path when a list renders even one child', () => {
+    forceOverflow();
+    render(
+      <CollapsibleText id="c9c" text="plain fallback">{[null, <p key="a">real</p>]}</CollapsibleText>
+    );
+    expect(screen.getByText('real')).toBeInTheDocument();
+    expect(screen.queryByText('plain fallback')).not.toBeInTheDocument();
+    expect(document.getElementById('c9c')).not.toHaveClass('line-clamp-2');
+  });
+
+  it('expands when focus lands inside the capped region', () => {
+    // The cap is a scroll container with no visible scrollbar. Tabbing to a link
+    // below it would scroll the preview to reveal the target with no way back,
+    // while `aria-expanded="false"` claimed content that is fully in the tab
+    // order was hidden.
+    forceOverflow();
+    render(
+      <CollapsibleText id="c10">
+        <a href="/r">buried link</a>
+      </CollapsibleText>
+    );
+
+    expect(screen.getByRole('button')).toHaveAttribute('aria-expanded', 'false');
+    fireEvent.focus(screen.getByRole('link', { name: 'buried link' }));
+
+    expect(screen.getByRole('button', { name: /Show less/ })).toHaveAttribute('aria-expanded', 'true');
+    expect(document.getElementById('c10')).not.toHaveClass('overflow-hidden');
+  });
+
+  it('measures overflow live on focus rather than trusting the state flag', () => {
+    // The focus handler must not read `isOverflowing`, which a passive effect
+    // populates after commit. A descendant with `autoFocus` takes focus during
+    // the commit phase — before that effect has ever run — so the flag is still
+    // a stale `false` and the focused control is stranded inside the clipped,
+    // scrolled region. Simulated here by letting the element start out fitting
+    // and become overflowing with no re-measure in between.
+    const spy = forceOverflow();
+    spy.mockReturnValue(0);
+    render(
+      <CollapsibleText id="c12">
+        <a href="/r">buried link</a>
+      </CollapsibleText>
+    );
+    expect(screen.queryByRole('button')).not.toBeInTheDocument();
+
+    spy.mockReturnValue(500);
+    fireEvent.focus(screen.getByRole('link', { name: 'buried link' }));
+
+    expect(screen.getByRole('button', { name: /Show less/ })).toHaveAttribute('aria-expanded', 'true');
+    expect(document.getElementById('c12')).not.toHaveClass('overflow-hidden');
+  });
+
+  it('leaves fitting children alone when focus lands inside them', () => {
+    // Nothing is clipped, so there is nothing to reveal — expanding here would
+    // only mint a no-op "Show less" button into the tab order.
+    render(
+      <CollapsibleText id="c11">
+        <a href="/r">visible link</a>
+      </CollapsibleText>
+    );
+
+    fireEvent.focus(screen.getByRole('link', { name: 'visible link' }));
+    expect(screen.queryByRole('button')).not.toBeInTheDocument();
+    expect(document.getElementById('c11')).toHaveClass('overflow-hidden');
+  });
+
   it('keeps the toggle when a resize fires against the uncapped container', () => {
     // Same in-flight-callback race as the line-clamp path: expanding removes the
     // cap, and a resize notification already in flight would otherwise measure