From 4ff7306dfd70034d8175cb32c82604123d84ccd9 Mon Sep 17 00:00:00 2001 From: Adam Eivy Date: Sat, 15 Aug 2026 05:18:10 +0000 Subject: [PATCH 1/3] fix: finish the CollapsibleText children-mode review fixes (#4170) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the review round on #4265 landed after that PR was merged, so they ship separately here. (The third — the effect deps missing the mode flag — went in with 6605399.) - `hasChildren` counted `[]` and `''` as children, so a caller writing `{items.map(…)}` over an empty list got an empty capped box with its `text` fallback silently dropped — the opposite of what "children wins over text" is meant to protect against. - The collapsed container is a scroll container with no visible scrollbar, and children may hold links, buttons, or a horizontally scrollable 
. Tabbing to one below the cap scrolled the preview to
  reveal it with no scrollbar, no wheel target, and no code path to
  scroll back — while `aria-expanded="false"` claimed content that was
  fully in the tab order was hidden. Focus inside now expands, gated on
  real overflow so content that fits doesn't mint a no-op "Show less".

Also adds the children-mode regression test for the deps fix, which
6605399 shipped without one.
---
 .changelog/next/changed-issue-4170.md         |  2 +-
 client/src/components/ui/CollapsibleText.jsx  | 24 +++++++-
 .../components/ui/CollapsibleText.test.jsx    | 56 +++++++++++++++++++
 3 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/.changelog/next/changed-issue-4170.md b/.changelog/next/changed-issue-4170.md
index 110a96a860..d3c9f08ac4 100644
--- a/.changelog/next/changed-issue-4170.md
+++ b/.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
diff --git a/client/src/components/ui/CollapsibleText.jsx b/client/src/components/ui/CollapsibleText.jsx
index dff8e4079d..dcf076738c 100644
--- a/client/src/components/ui/CollapsibleText.jsx
+++ b/client/src/components/ui/CollapsibleText.jsx
@@ -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 `
` and the capped `
`, 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
@@ -84,7 +88,13 @@ 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 `
+  // {items.map(…)}` over an empty list must get the text
+  // fallback, not an empty capped box with its `text` silently dropped.
+  const hasChildren = Array.isArray(children)
+    ? children.length > 0
+    : children != null && children !== false && children !== '';
 
   useEffect(() => {
     if (expanded) return;
@@ -100,6 +110,7 @@ export default function CollapsibleText({
   }, [text, hasChildren, expanded]);
 
   const clamp = CLAMP_CLASS[lines] || CLAMP_CLASS[2];
+
   const renderContent = () => {
     if (hasChildren) {
       return (
@@ -108,6 +119,15 @@ 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
+          // 
. 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.
+          onFocus={() => { if (isOverflowing) setExpanded(true); }}
         >
           
{children}

         

diff --git a/client/src/components/ui/CollapsibleText.test.jsx b/client/src/components/ui/CollapsibleText.test.jsx
index 242287b5ab..237c8453c7 100644
--- a/client/src/components/ui/CollapsibleText.test.jsx
+++ b/client/src/components/ui/CollapsibleText.test.jsx
@@ -237,6 +237,62 @@ 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 
 to the capped 
,
+    // so an effect that doesn't re-run stays bound to the now-detached 
 —
+    // 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({null});
+    expect(screen.queryByRole('button')).not.toBeInTheDocument();
+
+    spy.mockReturnValue(500);
+    rerender(
tall rendered markdown
);
+    expect(screen.getByRole('button', { name: /Show more/ })).toBeInTheDocument();
+  });
+
+  it('treats an empty child list as no children so the text fallback still renders', () => {
+    // `{items.map(…)}` over an
+    // empty list must not yield an empty capped box with `text` silently dropped.
+    render({[]});
+    expect(screen.getByText('plain fallback')).toBeInTheDocument();
+    expect(document.getElementById('c9')).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(
+      
+        buried link
+      
+    );
+
+    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('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(
+      
+        visible link
+      
+    );
+
+    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

From 587c0aeab2bf73760fd1ee7b452c6e9fbef57cc8 Mon Sep 17 00:00:00 2001
From: Adam Eivy 
Date: Sat, 15 Aug 2026 05:46:59 +0000
Subject: [PATCH 2/3] address review (codex): count renderable children, not
 array length (#4170)

---
 client/src/components/ui/CollapsibleText.jsx  | 13 ++++++++----
 .../components/ui/CollapsibleText.test.jsx    | 21 +++++++++++++++++++
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/client/src/components/ui/CollapsibleText.jsx b/client/src/components/ui/CollapsibleText.jsx
index dcf076738c..0655cd6b65 100644
--- a/client/src/components/ui/CollapsibleText.jsx
+++ b/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
@@ -92,9 +92,14 @@ export default function CollapsibleText({
   // happen to be blank" — a caller doing `
   // {items.map(…)}` over an empty list must get the text
   // fallback, not an empty capped box with its `text` silently dropped.
-  const hasChildren = Array.isArray(children)
-    ? children.length > 0
-    : children != null && children !== false && children !== '';
+  // Array *length* is the wrong signal: `items.map(i => i.show ?  : 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;
diff --git a/client/src/components/ui/CollapsibleText.test.jsx b/client/src/components/ui/CollapsibleText.test.jsx
index 237c8453c7..b2fa107c79 100644
--- a/client/src/components/ui/CollapsibleText.test.jsx
+++ b/client/src/components/ui/CollapsibleText.test.jsx
@@ -260,6 +260,27 @@ describe('CollapsibleText children (max-height) variant', () => {
     expect(document.getElementById('c9')).toHaveClass('line-clamp-2');
   });
 
+  it('treats a list of non-rendering children as no children', () => {
+    // `items.map(i => i.show ?  : 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(
+      {[null, false, undefined, '  ']}
+    );
+    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(
+      {[null, 
real
]}
+    );
+    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,

From c5e2bb865e823a2eec6f19399fa050f6c3de39ee Mon Sep 17 00:00:00 2001
From: Adam Eivy 
Date: Sat, 15 Aug 2026 05:51:41 +0000
Subject: [PATCH 3/3] address review (codex): measure overflow live on focus
 instead of reading lagging state (#4170)

---
 client/src/components/ui/CollapsibleText.jsx  | 12 ++++++++--
 .../components/ui/CollapsibleText.test.jsx    | 23 +++++++++++++++++++
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/client/src/components/ui/CollapsibleText.jsx b/client/src/components/ui/CollapsibleText.jsx
index 0655cd6b65..84d54eed62 100644
--- a/client/src/components/ui/CollapsibleText.jsx
+++ b/client/src/components/ui/CollapsibleText.jsx
@@ -73,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,
@@ -105,7 +110,7 @@ export default function CollapsibleText({
     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);
@@ -132,7 +137,10 @@ export default function CollapsibleText({
           // 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.
-          onFocus={() => { if (isOverflowing) setExpanded(true); }}
+          // 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); }}
         >
           
{children}

         

diff --git a/client/src/components/ui/CollapsibleText.test.jsx b/client/src/components/ui/CollapsibleText.test.jsx
index b2fa107c79..89597ff742 100644
--- a/client/src/components/ui/CollapsibleText.test.jsx
+++ b/client/src/components/ui/CollapsibleText.test.jsx
@@ -300,6 +300,29 @@ describe('CollapsibleText children (max-height) variant', () => {
     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(
+      
+        buried link
+      
+    );
+    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.