Review 5255

#5255 — feat(core)!: Banner content is visible by default; collapse moves to a `collapsible` prop

freddymeta · community · view on GitHub

Verdict: approve — posted 2026-08-24, squash-merged for the 0.5.0 cut. Three gates. The only breaking PR open ahead of Monday's release, and breaking changes land in that release or wait a full cycle.

The stale block is the story. Cindy's CHANGES_REQUESTED of 2026-08-20 had been satisfied by freddymeta's push six hours later, on 2026-08-21, and the review state still said changes-requested three days on — which is what held review-required. Same trap that sat on #4743 for a month. A met ask that nobody converts back into a verdict is indistinguishable from an unmet one, and the cost is paid by whoever is waiting.

Head reviewed: 5ea46b3dae6 — freddymeta's 505fe3e7ab5 plus one commit of mine, below.

Prior review (R1e)

One exists: cixzhang CHANGES_REQUESTED 2026-08-20 —

"This is the better pattern but I think we should avoid breakage. Could we: keep an internal default for collapsible; allow users to pass collapsible={false} or collapsible={} in order to customize it?"

Not contradicted — confirmed satisfied, and re-verified rather than taken from the author's reply. He claimed byte-identical PNGs; I re-shot both builds myself and added the case his claim did not cover (see Breaking · Visual). Her 2026-08-22 comment added one defect, collapsible={null}; fixed on the branch, by us, before merge. No inline review comments exist on this PR.

Problem

A banner that says "3 fields need attention" does not say which three. Any children are hidden behind a chevron, so the person reading the error has to notice a small glyph and click it before seeing the thing they need to fix — on the case Banner is most often reached for. The builder cannot show that content: defaultIsExpanded only picks the starting state, the reader can collapse it again, and nothing lets the app hold it open or drive it.

Solution (2 decisions · ~50 runtime lines of 547)

  1. the whole collapse axis becomes one prop —
     collapsible?: boolean | CollapsibleConfig, with `false` the opt-out,
     replacing defaultIsExpanded                     → the stated problem
  2. collapse state leaves Banner's useState for the shared useCollapsible,
     which is what makes controlled mode exist       → the stated problem

Both trace to a problem written in the body; the codemod and its 12 tests, the 8 Banner tests, the stories, Banner.doc.mjs (en/zh/dense), the block template and the changeset are how those two decisions are evidenced, not further judgements. Under the >2 smell, and the R1j audit found a stated rationale for each decision and for the default.

Impact

End user, today: nothing. No screen changes when this ships — measured, not asserted.

End user once a builder opts in: the three failed fields are on screen instead of behind a chevron nobody clicks.

Builder: every call site passing defaultIsExpanded stops compiling on 0.5.0, with a codemod for the JSX ones.

What merging newly exposes. The system now has two spellings for one axis: Banner says collapsible, ChatToolCalls (core, ChatToolCalls.tsx:88) and ChatReasoning (lab, ChatReasoning.tsx:49) still say defaultIsExpanded / isExpanded / onExpandedChange. Before this merge Banner and ChatToolCalls agreed. Not new breakage and not the contributor's to fix — and the codemod needs a component guard precisely because those props share a name. It is the argument for finishing the migration rather than leaving one component converted.

API

- Banner.defaultIsExpanded?: boolean = false                 (public, core barrel, REMOVED)
+ Banner.collapsible?: boolean | CollapsibleConfig = true    (public, core barrel)

CollapsibleConfig is genuinely reachable: Collapsible/index.ts:23, re-exported by src/index.ts:43. No half-public type. @astryxdesign/cli gains one codemod, staged in transforms/next/.

Theme targets

None added, removed or repointed. themeProps('banner-content', {container, status}) untouched. With collapsible={false} that element is always mounted, so a theme already targeting it paints on screens where it previously appeared only after a click — the feature, not a break.

Ossification

A class, and the other member is LANDED: SideNav.collapsible?: boolean | SideNavCollapsibleConfig (SideNav.tsx:292), merged, so boolean-or-config disclosure is settled precedent rather than a concurrent guess. API Conventions § Boolean-or-Config Props is the written rule. Banner reuses the exported CollapsibleConfig rather than minting a third spelling.

Alternatives, priced — this is what makes it an approve rather than a question for Cindy:

shape	cost
`isCollapsible` beside `defaultIsExpanded`	two props on one axis, one silently voiding the other; three once controlled arrives
keep `defaultIsExpanded`, add `isCollapsible`	non-breaking, permanently two vocabularies on one component
`collapsible: boolean \| CollapsibleConfig`	one breaking rename, codemod-able — matches the landed sibling
`@deprecated` alias for `defaultIsExpanded`	no break now; two spellings on Banner's own axis until someone removes them

R3k sweep, run before writing the finding: SideNav.tsx:292 spells it collapsible (not isCollapsible) and takes boolean | <X>Config. The PR's spelling matches the sibling exactly. It also inherited the sibling's bug.

Breaking

API — yes, and it is the purpose. defaultIsExpanded removed. @astryxdesign/core is the published public package, so there are real consumers; the break is a type error at every call site, never a silent behaviour change. Changeset minor + [breaking], matching #4881 for the same release.

Visual — no. Chromium, two independently built Storybooks (main's static build on 6100, PR head under storybook dev on 6580):

case	before	after
children, no collapse prop	`511ce148`	`511ce148`	byte-identical
starts open	`d0a351c0`	`d0a351c0`	byte-identical
default, then click Expand	`167b6120`	`167b6120`	byte-identical
empty slots	`08ea397d`	`08ea397d`	byte-identical
narrow container	`3ee4788f`	`3ee4788f`	byte-identical
the opt-out story	`442d51ef`	`3a8d0414`	differs — the chevron is gone, which is the feature

Row 3 is ours, not his. His claim covered two static states; swapping a state machine shows up in the transition, and that is the row a reviewer owes on a PR whose whole defence is "the default is untouched".

R15d: nothing got bigger. The default path is byte-identical, so no outer box moved and no neighbour reflows.

Theme — no.

Performance & resources

Render is even — one useState in Banner becomes one inside useCollapsible. No effect added or removed, no listener, no observer, no getComputedStyle, no layout read.

One piece of new structure: Banner reads CollapsibleGroupContext through use(), one context subscription per instance. Who pays: nobody today — counted, not guessed: zero <CollapsibleGroup> call sites exist outside CollapsibleGroup.test.tsx, Banner passes no value so the group branch is unreachable for it, and the densest frame the repo ships is 8 Banners (AllFeatures). It is real structure the library did not have, it survives the feature, and accepting it is Cindy's call, not the loop's (R18g). It is also the cheapest possible version: the read is what buys controlled mode.

Codemod — run, not read

A breaking change whose codemod does not work is worse than no codemod, so the transform was executed against a fixture rather than reviewed on the page.

fixture	result	matches the changeset?
`defaultIsExpanded` (bare)	`collapsible={{defaultIsOpen: true}}`	yes
`defaultIsExpanded={true}`	`collapsible={{defaultIsOpen: true}}`	yes
`defaultIsExpanded={false}`	removed, no `collapsible` emitted	yes
`defaultIsExpanded={showDetail}`	`collapsible={{defaultIsOpen: showDetail}}`	yes
`<Banner>` with children, no prop	untouched	yes
`<ChatToolCalls defaultIsExpanded>` in the same rewritten file	untouched	yes
`Banner` from a local module	untouched	yes
`<Banner {...props}>`	declines to guess	yes
`args: {defaultIsExpanded: true}`	untouched	yes — declared scope boundary

The CLI cannot run it today: transforms/next/ is absent from registry.mjs by design, and the release promotes it. The promotion was simulated — next/ copied to v0.5.0/, registry line added — and the real command applied it ("Applying v0.5.0 codemods… ✓ src/App.tsx"), then reverted.

codemod-verify.mjs → 1 file, 0 mismatches, superset as claimed. Codemod tests 12/12, Banner 52/52, core typecheck clean, eslint clean.

Visual evidence

Frames on assets/pr-5255 (fork), all captured in real Chromium at 900×420 and every one opened with the read tool: the default banner before/after (indistinguishable), the same banner after clicking Expand before/after (indistinguishable), and the opt-out before/after — where the only pixel difference in the whole PR lives: the chevron leaves the header end and the content stays.

Repro (R16f-repro):

probe-kit/banner-collapsible-frames.cjs            SB_PORT=6580
probe-kit/banner-collapsible-frames.cjs BEFORE=1   (warm main, :6100)

It shoots seven cases per build and prints a SHA-256 prefix per frame plus the DOM read (buttons, aria-expanded, content regions), so the byte-identity claim is a diff of two runs rather than an eyeball.

Risk class

Not low-risk (R19): it removes a public prop and adds a public one. The other three conditions hold — no behaviour regression, no perf regression, nothing got bigger. Not a subsystem (R26): one prop on one component, on a hook that already ships.

The fix we carried

collapsible={null} crashed the whole banner. typeof null === 'object', so it was read as a config and .defaultIsOpen was taken off it:

TypeError: Cannot read properties of null (reading 'defaultIsOpen')
  at Banner packages/core/src/Banner/Banner.tsx:474:40

Red first: a test added at the parent commit fails with exactly that. Then one clause at Banner.tsx:466 — collapsible != null && typeof collapsible === 'object' — and 52/52 green. Pushed as 5ea46b3dae6 and said out loud in the review (R13d): a contributor should never find commits on their branch they did not make and cannot explain.

The semantic choice, recorded because it is a choice. null now behaves as omitted — collapsible, starting closed — not as false. So a value widened to | null cannot silently drop a banner's toggle. The other reading (null → false, which is what useCollapsible does with it internally) would let a nullish config quietly change what a banner does.

Why it was ours and not his: the comment that reported it also said "nothing needed from you", so he reasonably did not push. That comment is what earned R2m — a comment has one status and a bug decides it — and the fix belonged to whoever wrote the sentence that stopped him.

Judgement

approve. No [BLOCKS] finding. feat(core)! is honest against the diff (R1f): the !, the [breaking] changeset and the removed prop all agree.

1. [FIXED BY US — never blocking on him]
   collapsible={null} crashed the whole banner
   → a JS caller, or anyone whose value is typed `CollapsibleConfig | null`,
     renders a Banner and gets nothing, plus a thrown TypeError that takes
     the surrounding render with it                     · Banner.tsx:466
   confirmed two ways: a red test at the parent commit, and reading the line.

2. [BREAKING · API — recorded, deliberately not raised (R16d)]
   defaultIsExpanded is removed
   → every consumer passing it fails to compile on 0.5.0
   The PR's stated purpose; title, changeset and body all say so; codemod
   verified above; 0.5.0 is the release that takes breaking changes.

3. [not blocking] the codemod matches on the element NAME, so an aliased
   import is missed
   → someone who wrote `import {Banner as Alert}` runs it, sees "no changes",
     and finds out at tsc — fails safe, same class as the props-object
     boundary the PR already declares  · banner-collapsible-content.mjs:116

4. [not blocking] no deprecation window
   → upgrading to 0.5.0 gives compile errors, not warnings. The author offered
     a `@deprecated` alias and declining is right for a breaking release: the
     alias would put two spellings on the axis this PR exists to unify.

5. [not blocking, pre-existing — NOT his]
   SideNav carries the identical null crash on main
   → `SideNav collapsible={null}` throws on `.hasButton`, same mechanism
                                                        · SideNav.tsx:335-337
   The PR copied a correct sibling idiom that happened to carry a bug (R13,
   R3i). Ours to file, not his to fix.

R30 checked and it does not fire. The findings do not share an interaction decision: one is a nullish guard, one is a transform's matching rule, one is a release-policy call, one is another component's. There is no parent choice they are children of.

The review as posted

Thanks, this is the right shape. The default really is untouched — byte-identical frames closed and expanded. I pushed the collapsible={null} guard so it lands with the cut.

(three-row frames table: default before/after, expanded before/after, collapsible={false} before/after)

[Reviewed by Robohands]

27 words of prose against a 30-word cap. No Discord link — R12e leaves it out of an approve.

What changed between the gates

gate	what the critic caught
1 → 2	R15-gate: the comment claimed the default was untouched "closed and expanded" and embedded a frame only for closed — a visual claim with no frame, on the load-bearing sentence of the whole review · R16d: BREAKING · API was "yes" and never reached JUDGEMENT · R19: risk class implied, not stated · R22: alternatives not priced, so nothing said why this is an approve rather than a hold · one anchor repointed (`:120` → `:116`)
2 → 3	R2, and it failed the way the rule predicts: 35 words against a 30-word cap, and the draft argued for the overage — "this is the one I am carrying past the cap, deliberately". That sentence is the rationalisation R2 exists to refuse. Fixed by deleting, not compressing.

R2 has now failed on the first or second draft of every run the loop has done. The interesting part here is not the overage, it is that gate 2 noticed it and wrote a defence instead of a cut. A self-check that can talk itself out of a hard cap is not a hard cap.

Time

TIME  total 19m
  setup       2m   worktree + install (16.9s) + storybook dev  (warm main reused: yes)
  reading     5m   brief, critic (3,959 lines), diff, useCollapsible, SideNav, prior review
  measuring   4m   2 probe runs (7 cases each), red-then-green on the null fix,
                   codemod fixture, promotion simulation
  writing     7m   gate 1, critique, gate 2, critique, gate 3
  waste       1m   plain `pnpm install` before reading the measurement harness,
                   which says to seed with fast-install.py's clonefile (~6s here
                   on a warm store, ~70s cold); assets branch pushed twice
                   because R15-gate wanted the expanded BEFORE too; and gate 2
                   spent words defending a word count instead of deleting words,
                   which cost a whole pass.

Review 5255

#5255 — feat(core)!: Banner content is visible by default; collapse moves to a collapsible prop

Prior review (R1e)

Problem

Solution (2 decisions · ~50 runtime lines of 547)

Impact

API

Theme targets

Ossification

Breaking

Performance & resources

Codemod — run, not read

Visual evidence

Risk class

The fix we carried

Judgement

The review as posted

What changed between the gates

Time

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

#5255 — feat(core)!: Banner content is visible by default; collapse moves to a `collapsible` prop