feat(site): add CollapsibleSection component and shared utilities#23990
feat(site): add CollapsibleSection component and shared utilities#23990DanielleMaywood wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
community
DanielleMaywood
force-pushed
the
danielle/collapsible-section
branch
2 times, most recently
from
b4985b7 to
4f8626a
Compare
DanielleMaywood
removed
the
community
DanielleMaywood
force-pushed
the
danielle/collapsible-section
branch
from
4f8626a to
d26f486
Compare
DanielleMaywood
force-pushed
the
danielle/collapsible-section
branch
4 times, most recently
from
b6bee09 to
8f3b539
Compare
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Solid extraction. The three hand-rolled toggle blocks in ModelForm are cleaner as a shared component, and building on Radix's Collapsible primitive is the right call for accessibility. The AvatarData and SectionHeader min-w-0 fixes are correct flexbox prerequisites.
Severity count: 1 P1, 1 P2, 3 P3, 2 Notes. One finding from all 7 reviewers.
The P1 is the heading level regression: the PR description promises the inline variant uses <h3>, but the code renders <h2> unconditionally. Every screen reader user who opens the model form gets a flat heading hierarchy instead of the parent/child nesting the old code had. This will compound across PRs 2-4 when card sections (h2) nest inline sub-sections (also h2). The other structural concern is the action slot living inside Radix's <button> trigger, which will become a problem when downstream PRs pass interactive controls into that slot.
Four {" "} text nodes were introduced as formatter artifacts. They're visually harmless (CSS collapses whitespace-only anonymous boxes in grid/flex contexts) but should be removed.
"Oh, this test suite looks polished! Six stories with play functions covering open, closed, keyboard, action isolation, inline variant. Real assertions on aria-expanded, content presence, toggle cycles." -- Bisky, before finding the heading level bug that no story checks for.
🤖 This review was automatically generated with Coder Agents.
| > | ||
| <div className="min-w-0 flex-1"> | ||
| <div className="flex items-center gap-2"> | ||
| <h2 className={headingStyles({ variant })}>{title}</h2> |
There was a problem hiding this comment.
P1 The inline variant renders <h2> but should render <h3>. The PR description explicitly states the inline variant uses "h3 title."
The old ModelForm used <h3> for Cost Tracking, Provider Configuration, and Advanced. The page already has <h2> for the parent section heading. After this change, subsections are also <h2>, flattening the heading hierarchy for screen readers (WCAG 1.3.1).
This is a class-of-bug: every future variant="inline" usage inherits the wrong heading level. PRs 2-4 in the stack will nest inline sections inside card sections, creating sibling <h2> elements where <h2> > <h3> is expected.
Fix: make the heading element variant-dependent (h2 for card, h3 for inline). The headingStyles CVA already differentiates visually; only the tag needs to change. (Knov P1, Bisky P2, Gon P2, Ging P2, Nami P2, Mafuuu P2, Meruem P2)
🤖
There was a problem hiding this comment.
🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖
Fixed — card renders <h2>, inline renders <h3>. The headingStyles cva handles the visual differentiation, the tag handles the semantic level.
DanielleMaywood
force-pushed
the
danielle/collapsible-section
branch
3 times, most recently
from
a28536d to
fcfa599
Compare
DanielleMaywood
force-pushed
the
danielle/collapsible-section
branch
from
fcfa599 to
de846d8
Compare
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
All 7 round 1 findings are resolved in code. The compound component refactor is a clean improvement over the original monolithic prop-based API. The as prop on CollapsibleSectionTitle is a better design than variant-dependent heading level: it lets the consumer match DOM nesting context rather than coupling heading semantics to visual variant.
Round 2 severity count: 1 P2, 2 P3. Clean design, minor gaps.
The P2 is the controlled mode API shipping without any story coverage. Every story uses uncontrolled mode (defaultOpen). The controlled path was added to resolve round 1 finding #5, but deleting lines 107-119 of CollapsibleSection.tsx leaves all five stories green. Passing open without onOpenChange silently freezes the section with no error or feedback.
One new stray {" "} was introduced at the same position class as the four removed in round 1. The as="h3" on the InlineVariant story exercises the heading prop but never asserts it.
Convention scan: em-dash (U+2014) found in CollapsibleSection.tsx:159 comment // Title — renders the heading element.... Project convention prohibits em-dashes. Replace with -- or rephrase.
"The tests that exist are genuine gems. They prove real behavior through the accessible API surface. Not a fake in the bunch. But let me look at what's not here." -- Bisky, before finding the missing controlled-mode story.
🤖 This review was automatically generated with Coder Agents.
| render: () => ( | ||
| <CollapsibleSection variant="inline" defaultOpen={false}> | ||
| <CollapsibleSectionHeader> | ||
| <CollapsibleSectionTitle as="h3">Cost Tracking</CollapsibleSectionTitle> |
There was a problem hiding this comment.
P2 No story tests controlled mode (open/onOpenChange), the API surface added to resolve round 1 finding #comment-3045753898.
Every story uses uncontrolled mode (either defaultOpen or the implicit default). The controlled code path at CollapsibleSection.tsx lines 109-119 is untested. You could delete those lines and all five stories pass green.
PRs 2-4 in the stack will consume controlled mode (force-open on validation error, "expand all" toggle). A consumer passing open without onOpenChange gets a dead trigger that silently ignores clicks, and there's no story documenting or testing that edge.
Fix: add a Controlled story that manages open state externally, toggles via a parent button, and asserts the section responds. Also test the open-without-onOpenChange frozen state.
PS. The as="h3" on this story is exercised but never asserted. expect(canvas.getByRole("heading", { level: 3 })).toBeInTheDocument() would catch a regression of the round 1 heading-level fix. (Bisky P2, Nami P3, Mafuuu P3, Meruem P3, Razor P3, Ging P3)
🤖
| </CollapsibleSection> | ||
| </div> | ||
| <div className="mt-auto py-6"> | ||
| {" "} |
There was a problem hiding this comment.
P3 New stray {" "} text node before <hr>. Same class of bug as resolved #comment-3045753889. The base code at this position had no whitespace node. The four original instances were removed, but this one was introduced. (Bisky P3, Gon P3, Nami P3, Mafuuu P3, Meruem P3, Razor P3, Ging P3)
🤖
2 participants
PR 1/4 in a stack to unify agent settings pages.
What
Adds a reusable
CollapsibleSectioncomponent and applies it to the existing model form, replacing hand-rolled toggle logic.Why
The agent settings pages are being restructured into collapsible bordered cards for visual consistency. Rather than each page rolling its own expand/collapse logic (as
ModelForm.tsxalready did with 3 separateuseState+button+ chevron blocks), this centralizes the pattern into a single component with proper accessibility (aria-expanded,aria-controlsviauseId(), keyboard handling for Enter/Space).Changes
CollapsibleSection— two variants:variant="card"(default): bordered card withrounded-lg border,h2title,px-6 py-5padding,hrdivider. Used by settings pages (PRs 2–4).variant="inline": lightweightborder-tdivider,h3title, no outer padding. Designed for sub-sections inside forms where a full bordered card would be too heavy.Both variants share toggle logic,
aria-expanded/aria-controls, keyboard nav, action/badge slots withstopPropagation, and chevron animation.ModelForm.tsx— replaces the 3 hand-rolled collapsible sections (Cost Tracking, Provider Configuration, Advanced) with<CollapsibleSection variant="inline">. RemovesshowAdvanced,showPricing,showProviderConfigstate variables and theChevronDownIcon/ChevronRightIconimports they required.AvatarData— addsmin-w-0to both inner flex containers. Without this, long usernames/titles cannot truncate because flexbox children default tomin-width: auto, which prevents text from shrinking below its intrinsic width.SectionHeader— addsmin-w-0 flex-1to the text container for the same truncation reason.Stories — 6 stories with play functions covering card open/close, collapsed default, action slot isolation, keyboard toggle (Enter + Space), and the inline variant.
Stack