Review 5255

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

freddymeta · community · MERGED · view on GitHub

Verdict: approve — posted 2026-08-24, squash-merged the same night for the 0.5.0 cut: dfdd778050d. Four 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: 74048e29d1d — freddymeta's 505fe3e7ab5 plus two commits of mine, both 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.

The second thing we carried: a promise the compiler does not keep

Gate 3 approved and armed the merge. Before it landed, the pre-release debt register (retired 2026-08-24 into the internal release-block register) turned out to carry #5255 as a MUST with a second owed item — a round two drafted and held:

"either the banner-collapsible-content codemod covers the props-object sites it currently skips, or the changeset stops promising the compiler finds them… <Banner {...args} /> carrying the removed defaultIsExpanded compiles clean and silently loses its behaviour"

Disarmed the merge and verified it rather than trusting the register. It is true. A probe with both shapes, run through tsc:

src/Banner/__tsprobe.tsx(16,35): error TS2322: Type '{ children: Element;
  status: "info"; title: string; defaultIsExpanded: true; }' is not assignable
  to type 'IntrinsicAttributes & BannerProps'.

Line 16 is the direct call, <Banner status="info" title="T" defaultIsExpanded> — it errors, as the changeset promised. The spread, <Banner {...args} /> with an inferred args, produced no diagnostic at all: TypeScript does not excess-property-check a spread. Confirmed a second way (R16g), by a different method: Banner spreads ...rest onto its root <div> at Banner.tsx:454 and :565, so the orphaned prop reaches the DOM rather than being dropped at the component boundary.

So a team with function AppBanner(props) { return <Banner {...props} /> } upgrades to a green build and a banner that used to open now starts closed — no compiler error, no codemod hit, at best a React warning about an unknown DOM attribute if someone is watching the console. On a PR whose entire migration story is "nothing changes under anyone without a compiler error first", that is the one sentence that was not true.

Why this was not the stop-and-report condition. The codemod is not wrong — it is correct on all nine fixture cases and it declines exactly the case it cannot decide, because an inferred object does not say which component it feeds and ChatToolCalls has a prop of the same name. Nobody is stranded: JSX attributes transform, a props object in a typed position still fails to compile, and the remaining shape needs one honest sentence. What was wrong was the promise, and the register had already framed the remedy as a binary whose other arm the author had correctly refused. Making a false sentence true is repair, not a design call — it binds no future PR (R27 test 1).

74048e29d1d rewrites the changeset's migration paragraph to name the spread and tell people to grep, and it was disclosed on the PR in our own words rather than left as a silent commit on someone else's branch (R13d).

The sequencing lesson, which is the transferable part. Reading the debt register after approving was the wrong order. A finding drafted and held is a prior review that never got posted, so it belongs beside the R1e prior-review read, not after the verdict. The check found a real defect; it should not have found it with auto-merge already armed.

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. [FIXED BY US]
   the changeset promised a compile error the compiler does not give
   → a team spreading props upgrades to a green build and a banner that used
     to open now starts closed, silently
                            · .changeset/banner-collapsible-content.md
   confirmed by tsc and by reading `...rest`. Fixed in 74048e29d1d.

3. [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.

4. [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

5. [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.

6. [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.

Plus a follow-up comment disclosing the second push, 60 words:

One more thing I changed, since it's a claim rather than code: <Banner {...args} /> with defaultIsExpanded in the object compiles clean — TypeScript doesn't excess-property-check a spread — so the prop reaches the DOM and the banner quietly starts collapsed. The changeset promised the compiler catches those; it now says to grep. Your JSX-only boundary was the right call, the promise around it was just too strong.

[Reviewed by Robohands]

R2m self-check on that one: it reports a defect and it does not tell the author he is free. It says what was done and asks nothing — the honest shape when the fix is already carried, and the opposite of the comment that earned R2m on this very PR.

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.
3 → 4	Not a critic failure — I changed the PR again, so my own change got the same pass his did. The debt register's held round-two finding, verified with `tsc`, and the changeset corrected. Gate 4 also ran R3d on my own two fixes: the null guard and the changelog claim are different shapes, so finding 1 is not a patch; the same-shape probe for it pointed at `SideNav`, which is finding 6.

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 26m
  setup       2m   worktree + install (16.9s) + storybook dev  (warm main reused: yes)
  reading     6m   brief, critic (3,959 lines), diff, useCollapsible, SideNav,
                   prior review, the debt register
  measuring   5m   2 probe runs (7 cases each), red-then-green on the null fix,
                   codemod fixture, promotion simulation, the tsc spread probe
  writing     9m   gates 1-4, a critique between each
  merging     4m   approve, arm, disarm, second push, re-arm, sweep
  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

The second thing we carried: a promise the compiler does not keep

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!

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