Skip to content

refactor: give CollapsibleText a children-based max-height variant and migrate AgentCard onto it (#4170) - #4265

Merged
atomantic merged 2 commits into
mainfrom
claim/issue-4170
Aug 15, 2026
Merged

refactor: give CollapsibleText a children-based max-height variant and migrate AgentCard onto it (#4170)#4265
atomantic merged 2 commits into
mainfrom
claim/issue-4170

Conversation

@atomantic

Copy link
Copy Markdown
Owner

Summary

client/src/components/cos/tabs/AgentCard.jsx's TaskDescription was a second "long text behind a Show more toggle" implementation sitting one directory over from the shared client/src/components/ui/CollapsibleText.jsx. It guessed at overflow from text.length > 200, clamped with a bespoke max-h-[3.5rem] + gradient fade, and its toggle carried no aria-expanded / aria-controls.

It was never consolidated because AgentCard renders markdown blocks via MarkdownOutput, and CSS line-clamp applies to a container's own inline content — against block children it silently does nothing.

So CollapsibleText now offers a second clamp strategy, picked by which content prop you pass:

  • text (unchanged) — line-clamp at lines depth.
  • children (new) — a maxHeight cap (default 3.5rem) for content line-clamp can't clamp. children wins over text.

Both strategies share the same measured-overflow logic, the same toggle, and the same aria-expanded / aria-controls wiring, so the disclosure affordance and its accessibility are now identical everywhere.

One wrinkle worth calling out for review: in children mode the ResizeObserver attaches to the uncapped inner wrapper as well as the capped container. The outer element is height-capped, so growing children never change its box — an observer bound to it alone would never fire for content that arrives or changes after mount. Observing the inner wrapper also means children (a fresh element object every render) doesn't have to go in the effect deps, which would churn the observer on every parent re-render — and AgentCard re-renders once a second while an agent is running.

Visible change: AgentCard's fade treatment goes away

Flagged in the issue and repeated here because it's a deliberate, user-visible regression in polish:

  • The gradient fade (bg-gradient-to-t from-port-card) over the bottom of a clamped description is gone. The shared primitive clips flat.
  • The toggle restyles to the shared look: a chevron icon + label, hover:text-port-accent/80 rather than hover:text-white.
  • The toggle now appears based on measured overflow rather than text.length > 200. This is a behavior fix, not just a visual one: a 300-character description that fit inside the cap previously rendered a no-op "Show more" that expanded to nothing, and a short-but-tall description (a few bullet list items) previously got clipped with no affordance at all. Both are covered by new tests.
  • Because measurement is now real, a description that fits at desktop width but wraps past the cap on a phone correctly gains a toggle (the shared ResizeObserver path).

Other files

  • client/src/components/ui/README.md — catalog row updated to name both strategies.

No changes to the three existing text-mode call sites (TaskItem.jsx ×4, Review.jsx ×2) — the line-clamp path, expandedContent, expandedClassName, lines, and forceToggle all behave exactly as before, and their existing tests are untouched and green.

Test plan

  • client/src/components/ui/CollapsibleText.test.jsx — 7 new cases for the children variant: caps and uncaps on toggle, no toggle when the children fit, aria-controls/aria-expanded wiring, className forwarding, children preferred over text, the observer covering the inner wrapper as well as the container, and the in-flight-resize race that must not drop the toggle mid-expand. All 12 pre-existing line-clamp cases untouched and passing.
  • client/src/components/cos/tabs/AgentCard.test.jsx — 2 new cases: a long description gets an aria-wired toggle that lifts the cap on click, and a 300-character description that fits gets no toggle (the case the old length > 200 heuristic got wrong). Both fail against the pre-change component.
  • cd client && npm test — 647 files, 7888 tests. The 2 failures are in src/pages/SongBookViewer.test.jsx (an act() warning in SongLinksEditor), untouched by this branch and passing when that file runs on its own — a known full-suite parallelism flake.
  • cd client && npm run lint — clean (2042 files, biome, --error-on-warnings).

Closes #4170

…d migrate AgentCard onto it (#4170)

AgentCard's TaskDescription was a second "long text with a Show more
toggle" implementation one directory over from the shared primitive. It
guessed at overflow from `text.length > 200`, capped with a bespoke
`max-h-[3.5rem]` + gradient fade, and its toggle carried no
`aria-expanded`/`aria-controls`.

CollapsibleText now supports a `children` clamp strategy — a `maxHeight`
cap for content CSS `line-clamp` cannot clamp, i.e. anything emitting
block children like rendered markdown. The ResizeObserver attaches to the
uncapped inner wrapper as well as the capped container, since the outer
box never changes size when its children grow.
@atomantic
atomantic merged commit 08e745b into main Aug 15, 2026
7 checks passed
@atomantic
atomantic deleted the claim/issue-4170 branch August 15, 2026 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Give CollapsibleText a children-based variant and migrate AgentCard onto it

1 participant