fix: finish the CollapsibleText children-mode review fixes (#4170) - #4272
Merged
Conversation
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 `<CollapsibleText text={fallback}>{items.map(…)}</CollapsibleText>` 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 <pre>. 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.
Owner
Author
|
Review loop closed (codex, 3 rounds). Fixed in this PR:
Both regressions have a test verified to fail against the pre-fix component. Declined — empty React fragments ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #4265, which shipped the
children-based max-height variant forCollapsibleTextand migratedAgentCard'sTaskDescriptiononto it. That PR was merged while its review round was still running, so two of the three review findings didn't make it in. They ship here.(The third — the effect deps missing the mode flag, which left the observer bound to the detached element after a
<p>→ capped<div>swap — landed onmainin 6605399.)1.
hasChildrencounted an empty list as childrenchildren != null && children !== falseis true for[]and''. A caller writing<CollapsibleText text={fallback}>{items.map(…)}</CollapsibleText>over an empty list got an empty capped box with itstextfallback silently dropped — the exact opposite of what the "children wins over text" rule exists to protect against.2. Focus inside the clipped region had no way back out
The collapsed container is
overflow-hidden: a scroll container with no visible scrollbar. Children in this mode are precisely the content that holds focusable things —MarkdownOutputemits<a target="_blank">,<img>, and<pre className="overflow-x-auto">. Tabbing to a link below the cap made the browser scroll the capped box to reveal it, and from there the top of the description was gone with no scrollbar, no wheel target, and no code path resettingscrollTop. Meanwhile the toggle assertedaria-expanded="false"over content that was fully present in the accessibility tree and the tab order.Focus inside the capped region now expands it, which puts the target on screen and makes the
aria-expandedclaim honest. It's 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.Also
Adds the children-mode regression test for the deps fix — 6605399 shipped that fix without one, so nothing currently guards it.
The
text/ line-clamp path is untouched;TaskItem.jsx(4 call sites) andReview.jsx(2) pass no children and are unaffected.Test plan
client/src/components/ui/CollapsibleText.test.jsx— 3 new cases: children arriving after mount get measured (guards 6605399), an empty child list falls back totextand its line clamp, focus inside a clipped preview expands it, and focus inside a fitting preview does not.cd client && npx vitest run src/components/ui/CollapsibleText.test.jsx src/components/cos/tabs/AgentCard.test.jsx— 34 passed.cd client && npm test— 647 files, 7892 tests, all green on the branch this was cut from.cd client && npm run lint— clean (biome,--error-on-warnings).Refs #4170, #4265