Review 5428

Review 5428 — replacing the toast surface, and theming the chrome

#5428 · freddymeta · collaborator (same bucket as #5385) Head reviewed 1fc9089471c — unmoved across all three gates Verdict request-changes · one block, drafted not posted, with Cindy Prior review none — reviews and comments both empty

Problem

Stated by the PR, and real, with a named consumer. A product with its own notification design can only get it by hiding Astryx's card with CSS. A shared library calls useToast({type: 'error'}); the app's rule hides the dismiss on every toast including that one, and an error toast does not auto-hide (ToastViewport.tsx:514) — so the user is left with a notification about a failure they can never close. The rule also hits any Button in endContent, and Toast's trailing slot welds endContent and the close together.

Solution — 3 decisions · 298 runtime lines of 674

1. renderToast on ToastViewport and on LayerProvider's toast config replaces the whole visible surface for every toast; the auto-hide timer moves out of the card into useToastTimer so it survives (:538-551, useToastTimer.ts, LayerContext.ts:40).
2. Two theme targets — astryx-toast-viewport with the position variant, and astryx-toast-item (:507, :522).
3. The viewport is a landmark only while it holds a toast (:500).

Every decision traces to a problem written down in the body, in its own section, with its own reason. Nothing is piggybacked — which is R29's sharper test, and it passes, so the count does not carry a verdict. What the count buys is a cheap gift: 2 and 3 need nothing from 1, and 1 is the only contested one, so the split is offered as what lands today rather than demanded.

Not bigger than its idea in lines — the 376 non-runtime are 203 of tests (five of which fail against an unmodified ToastViewport), a story, docs in two locales and a changeset. Bigger in scope: 2 and 3 are not renderToast.

Impact

Builders who supply a renderer: they draw their own card and a library's toast gets it too, which is the win — and an error toast whose renderer omits a close can never be dismissed. Measured: no dismiss control, Escape does nothing, F6 lands outside the viewport because nothing inside it is focusable, still on screen after ten seconds.

Screen-reader users of any app with a renderer: a custom surface is a bare <div> (:136); Astryx's card carries role={isError ? 'alert' : 'status'}, aria-live and aria-atomic (Toast.tsx:146-148), which :255-260 says is kept deliberately "for browse-mode discoverability". The announcement itself survives — imperative in addToast through the singleton region, and tested. The PR says the dismiss is "the one a11y contract that stops being Astryx's". It is the second.

Every app on the next release, no opt-in: the toast viewport stops being a landmark while empty. Two-sided against warm main — main exposes role="region" "Notifications" with no toast shown, this head exposes none. An improvement, and a behaviour change to shipped Toast under a renderToast title.

Theme authors: nobody yet, and not in a production build even then. Both new targets are components: {} keys, and #5410 (open, unmerged) exists because "Every components: {…} override a theme set … was silently dropped in the built app while working in dev." To the PR's credit it says which path it measured — the layer order dist/astryx.css emits, by injecting a competing rule. That is the library's own stylesheet, not an app's Vite build, which is the path #5410 is about.

API

+ ToastViewport.renderToast?: ToastRenderFn        (public, core barrel)
+ LayerToastConfig.renderToast?: ToastRenderFn     (public, LayerContext.ts:40)
+ type ToastRenderProps {…}                        types.ts:61 (public, BOTH barrels)
+ type ToastRenderFn                               types.ts:82 (public, BOTH barrels)
~ ToastViewport role/aria-label now conditional on hasToasts  (changed meaning,
                                                   no signature change)
  useToastTimer                                    (internal, not exported — correct)

ToastRenderProps carries no role and no isExiting, so a custom surface can participate in neither the a11y contract nor the exit transition.

Theme targets

+ astryx-toast-viewport → the fixed stack container, + the position variant class
+ astryx-toast-item     → each toast's wrapper: the gap between stacked toasts
                          and the collapse transition

Verified on the elements in real Chromium, not just in the generator — the PR's tests assert generateThemeCSS output and a comment says the placement is "covered in a browser". It is now.

Ossification

renderToast — class holds with a merged member: Tokenizer.renderToken(item, onRemove) is this exact shape, content plus an injected callback. Core already declares seven distinct render props across 14 declarations (renderItem ×6, renderOption ×2, renderGroupHeader ×2, renderValue, renderToken, renderExpanded, renderCell). The two targets — one real consumer with two needs, which is thin; but the alternative to a named target here is not "no surface", it is the structural [data-toast-id] > div selector that breaks whenever the DOM does. astryx-toast-item points at the wrapper whose grid-rows collapse is the exit animation, which is more load-bearing than a target usually names.

Breaking

• API no signature break; the role/aria-label change is a changed meaning on existing surface with nobody opted in.
• Visual no — omit renderToast and output, timers and DOM are unchanged apart from two class names no shipped theme selects.
• Theme no existing target changes; two added.

Far side of the bound: hasToasts driven both ways, plus isAutoHide: false — the error default and the only state where "no dismiss" is permanent.

Performance — the #5353 sweep, run deliberately, and clean

• No imperative call became an effect. The announcement stays imperative in addToast (:254-262 says why). No announcedIdsRef-shaped suppression set.
• No effect sets state. useToastTimer's two effects drive setTimeout and refs only. No eslint-disable for set-state-in-effect in the diff.
• Dep chains resolved (R18d). Effect 1's [autoHideDuration, startTimer] reduces to [autoHideDuration, isAutoHide]; the comment at :107-108 names isAutoHide as what makes the callback stable and then lists triggers as "mount and a genuine duration change", omitting isAutoHide changing. The enumeration is incomplete, not absent — benign, and moved verbatim from main.
• Listeners moved, not added, and scoped (R18f). The window blur/focus pair is per auto-hiding toast, gated, torn down, one instance per toast because Toast and RenderedToast are two arms of a ternary. F6's document keydown is pre-existing and already gated on hasToasts.
• One identity churn. RenderedToast's toast prop is a fresh literal per viewport render (:541), so a consumer's surface re-renders whenever the viewport does. Render count not measured.
• Geometry. The wrapper <div> carrying the pause handlers measures 320×50 against the surface's 320×50 — checked because a full-width wrapper would make hovering beside a card pause its timer. It does not. Finding killed.

Judgement

1. [BLOCKS] a renderToast surface that omits a dismiss control leaves an error
   toast with no exit at all
   → no button, Escape does nothing, F6 lands outside the viewport because
     nothing inside it is focusable, and it does not auto-hide. Still there
     after ten seconds                             · ToastViewport.tsx:538-551
   confirmed two ways: driven in Chromium, and against the built-in-card control
   in the same story, which has the button and takes F6. This is the state the
   PR's own problem statement condemns in the CSS hack — R3d, the same shape one
   step sideways. The ask is the CHEAP remedy the author's own body names: two
   of his three arbitration agents asked for a dev-mode error rather than a
   paragraph. Escape in the transport stays its own PR, as he proposed.
2. [not blocking] the landmark change is a behaviour change to shipped Toast for
   every app, no opt-in, under a renderToast title      · :500
4. [not blocking] a custom surface silently loses the per-toast role/aria-live
   that Astryx's card carries, and the body states the opposite in as many
   words                        · Toast.tsx:146-148 vs ToastViewport.tsx:136

Finding 1 carries the verdict, alone. The render prop's shape is fine and the review says so. The split — the two theme targets, and the landmark fix, each on their own — is offered as what lands today.

Visual evidence

Frames on assets/pr-5428 of cixzhang/astryx: stuck__no-dismiss.png (an error toast under a renderer with no close) and control__builtin-card.png (the same toast on Astryx's card, × present, F6 lands on it). The landmark change and the dropped role are a11y-tree facts, measured rather than photographed.

The review as drafted (146 words)

Thanks — the arbitration write-up and the timer bug your own test caught are why this reads as settled work. The render prop is the right shape and I'd take it.

One thing to fix. With a renderer that omits a close, an error toast has no exit: Escape does nothing, F6 lands outside the viewport because nothing in it is focusable, and it's still there after ten seconds.

(frames embedded — your renderer vs Astryx's card, same toast)

The landmark fix is a behaviour change to shipped Toast under a renderToast title — send it on its own and it lands today.

Escape in the transport is the durable fix and I agree it's its own PR. Your own agents asked for a dev-mode error rather than a paragraph, and I'd take that here.

Inlines: :538 — Nothing makes the renderer draw a close. An error toast without one never leaves. · :136 — Astryx's card carries role="status"; a custom surface doesn't. Second a11y contract that moves, not the first. · :500 — This is a good fix and it isn't renderToast. Own PR?

The second question for the maintainer (held behind #5385's)

Does the guaranteed exit have to ship before renderToast is public, or is a dev-mode warning enough? The draft asks for the warning. If the answer is that a permanent undismissable error toast must be structurally impossible rather than warned about, the ask becomes holding renderToast for the Escape PR.

What the gates changed

• Gate 1 → 2. Two blocks where there is one: the decision count was promoted to a block the draft's own analysis had already cleared, and it contradicted its other finding (one said a shipped-Toast behaviour change wants its own thread, the other demanded one be added here). 165 words against the cap. Two anchors written rather than opened — types.ts:865 and useToastTimer.ts:1003 on 92- and 129-line files. An R18d sentence that contradicted its own arithmetic one line earlier. And two findings missed entirely, neither needing any measurement: the dropped role="status".

• Gate 2 → 3. STEP 7b — the closing line was the concession ("Escape is its own PR") rather than the ask, so the author reads the hazard as deferred; the two sentences are swapped. The dep-chain sentence had overcorrected into the mirror of gate 1's error. Finding 4 was stated weaker than its own evidence, which was a sentence sitting unquoted in the PR body.

• The pattern across both: the draft disagreeing with itself, which needs no new judgement to catch — only reading the draft back against itself.

• Gate 3 — clean. "VIOLATIONS: none. WOULD SHE HAVE POSTED IT: yes — as written." 22 anchor positions re-opened at the head, all correct; head unmoved. One hand-off round number ("ten render props") corrected to the enumeration above.

What Cindy changed before posting

(not yet posted — awaiting her read)

---

Re-review — 2026-08-25

Versions: Review Loop unversioned (pre-1.0) · Component Audit Rubric 1.10
Head: 6fb67677 · Verdict: request changes

The non-painting targets are gone, landmark behavior is split into #5494, and static omission falls back to a dismiss control. Two runtime issues remain: a nested custom layout can unmount its dismiss control without rerendering Toast, and the bookkeeping increases unchanged default-toast commits from one to three.

Review as posted:

Thanks — my earlier review asked for the non-painting targets to come out, the landmark change to split, and persistent toasts to keep an exit. The targets are gone, the landmark is now #5494, and omission now falls back correctly.

Two blockers remain.

DismissButton only reports presence when Toast commits. A nested custom layout can remove it from its own state without rerendering Toast; Chromium went from one close to zero on a persistent error toast, and F6 stayed outside. The same bookkeeping takes an unchanged default toast from one commit to three; an A/B guard returned it to one.

Could the slot own mount/unmount registration, stay inactive without renderContent, and get a focused child-only toggle test?

[Reviewed by Robohands]

---

Round 4 — re-review — 2026-08-27

PR

#5428 — feat(Toast): renderContent — custom toast layout, with a DismissButton that cannot be lost, by freddymeta

HEAD REVIEWED

cb18d4b72aed2d4a80207076bb4981db508bff6c

VERSIONS

LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13

LANE

LANE: full WHY: public API, visible behavior, effect/state machinery, and standing changes-requested reviews.

PRIOR REVIEW AND DELTA

The last GitHub review at 4f7c9e17026eff74db6f5051e565a19b95f67712 asked for exactly one dismiss control through server rendering and hydration, while preserving the nested-child fallback. The only author single-parent commit after that review is cb18d4b; 91c0a07 is a merge from main.

prior finding	current-head result
non-painting theme targets	fixed earlier; still absent
landmark behavior mixed into this feature	split earlier into #5494
omitted close traps persistent errors	fixed earlier; fallback remains
nested child can remove its close without restoring fallback	fixed; focused test passes
unchanged default Toast incurs extra commits	fixed; Profiler guard remains one commit
SSR emits two closes before hydration	fixed; server and hydrated DOM each contain exactly one
stale dismissButton example and ToastProps wording	fixed

PROBLEM

WHY 1: A product-specific notification layout otherwise needs CSS that hides Astryx controls globally. WHY 2: Unrelated persistent error toasts can lose their only dismiss control. WHY 3: A notification system must let users recover from failures while products customize layout. USER-FACING PROBLEM: A person receiving a persistent error toast under a product theme can be left with no way to close it. PROBLEM SEVERITY: broken task — the user cannot dismiss the persistent error.

VERDICT: clear — demonstrated consumer and browser reproduction.

SOLUTION

Each toast may opt into custom content while Astryx keeps the card, status semantics, timer, and close. The layout receives Astryx’s DismissButton; placing it moves the close, while omitting or later removing it restores a corner fallback. The latest commit keeps the fallback as the one server/hydration close, then swaps to the placed control before client paint.

SOLUTION (1 current-head decision; feature shape already accepted)

1. A hydration-safe client snapshot distinguishes server/first hydration render from ordinary client rendering, so SSR emits one fallback and client placement still claims the slot.

BURDEN: medium — internal registration state, one layout registration effect, one hydration-safe external-store read, and focused tests. BURDEN MATCH: proportionate — it prevents undismissable persistent toasts and duplicate SSR controls while staying inactive on ordinary toasts.

VERDICT: clear.

ARCHITECTURE

OWNER: Toast owns dismissal; the internal dismiss-slot module owns custom-layout placement registration. TIER 1: Astryx Button, MediaTheme, translation, and Toast status/live-region behavior. TIER 2: status semantics. SEAMS: built-in layout; custom layout with placed close; omitted close; nested child unmount; direct Toast SSR/hydration. BEHAVIOR UNIT: the internal dismiss-slot module, with focused lifecycle and hydration tests.

seam	current-head result
built-in Toast	unchanged; Profiler guard remains one commit
custom, placed	one translated Astryx dismiss button in Chromium
custom, omitted	one fallback; F6 reaches it; click dismisses
nested child unmount	focused test restores fallback
SSR/hydration	focused test counts exactly one before and after hydration

VERDICT: clear.

IMPACT

Only builders opting into renderContent change layout. Their users keep one translated dismiss control, alert/status semantics, and timer behavior; calls without renderContent remain ordinary Toasts.

VERDICT: clear.

API

showToast({
  body: 'Upload failed',
  type: 'error',
  renderContent: ({body, DismissButton}) => (
    <><strong>{body}</strong><DismissButton /></>
  ),
});

	change	public?	class	docs	verdict
+	ToastOptions.renderContent?: ToastContentRenderFn	yes	established render-function composition	yes	accepted in prior review
+	ToastContentRenderProps.DismissButton: ComponentType	yes	injected system-owned control	yes	accepted in prior review
+	ToastContentRenderFn	yes	render-function type	yes	accepted in prior review

OSSIFICATION: the first review said “The render prop is the right shape and we want it”; rubyycheung approved the design. This round does not reopen either settled decision.

VERDICT: clear.

THEMING

No current-delta target or token change. The previously disputed non-painting targets remain absent; the existing astryx-toast card and astryx-button close targets remain.

VERDICT: clear.

BREAKING

BEHAVIOR: no regression found; direct SSR/hydration improves from two pre-hydration closes to one. API: no existing call site changes; renderContent remains opt-in. VISUAL: no unintended change found; fallback and placed endpoints each show one close. THEME: no target or override semantics change in the current delta.

VERDICT: clear.

PERFORMANCE & RESOURCES

EFFECTS: the changed registration layout effect keeps the same dependency, registration lifetime, and cleanup; the new external-store read uses stable no-op subscription and constant snapshots. RENDER: the exact-head default-toast Profiler test remains one commit; custom registration remains opt-in and unchanged from the previously reviewed head. LISTENERS/OBSERVERS: no browser listener or observer added. LAYOUT: no layout reads or writes. BUNDLE: no dependency change.

VERDICT: clear.

VISUAL EVIDENCE

VISUAL CHECK: manual frames required WHY: the visual-acceptance status is red and the fallback/placed runtime endpoints require exact-head evidence; Stable visual regression alone does not cover the choice.

Fallback when the custom layout omits DismissButton	Placed Astryx DismissButton

sensor	fallback	placed
build	cb18d4b72aed2d4a80207076bb4981db508bff6c	same
story	core-toast--custom-content	same
theme / mode / direction	neutral / light / LTR	same
viewport / media	1000×500@1; forced colors off; reduced motion off; fine pointer	same
semantic state	alert, aria-live=assertive, one “Dismiss notification”	same
visible box	400×52, in viewport	400×60, in viewport
readiness	fonts loaded; no page or Storybook error	same

Both frames were captured from the local exact-head Storybook and inspected.

VERDICT: clear.

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

Exact-head Chromium: the omitted-close error toast exposes role=alert, aria-live=assertive, and one translated “Dismiss notification” button; F6 focuses that button and click dismisses. Placed custom content exposes the same single named control. Exact-head pr-a11y is green without a baseline change.

VERDICT: clear.

JUDGEMENT

slot	verdict
PROBLEM	clear
SOLUTION	clear
ARCHITECTURE	clear
IMPACT	clear
API	clear
THEMING	clear
BREAKING	clear
PERFORMANCE	clear
VISUAL	clear
A11Y & I18N	clear

GOAL: met — focused SSR/hydration evidence and exact-head Chromium each show one dismiss; nested fallback and default render-count guards pass. DISPOSITION: every prior finding is fixed; no negative finding remains. ADVICE: omitted — no defect remains. AUTHOR CAN PROCEED: yes — no further change requested. WORST OUTCOME: none found at current head. JUDGEMENT NEEDED: none — API and design were settled in prior reviews.

Verdict: approve.

REVIEW

Thanks — my last review asked for exactly one dismiss control through SSR and hydration. This head does that, preserves nested fallback, and fixes the stale docs. Looks good.

[Reviewed by Robohands]

INLINE

None.

EVIDENCE I DID NOT SPEND

• All 44 focused Toast tests passed locally.
• Exact-head CI, lint, build, Storybook build, pr-a11y, pr-rtl, stable visual regression, registry, and smoke jobs passed.
• 91c0a07 contains only the merge from main; cb18d4b is the author’s one single-parent response.

CRITIC GATES

Gate 1 failed the explicit prior-review reconciliation sentence. Gate 2 added it. The next gate was clean: correct approve verdict, 26-word body, no violations, ready as written.

TIME

• setup/rules: 3m
• install/build/server: 2m (generated output and warm dependency donor reused)
• browser/a11y: 2m
• focused tests: <1m
• code/history analysis: 2m
• critique + wiki publication: 3m
• CI wait: 0m
• total active: 12m
• waste: 1m — failed filename/tool lookups; no redundant build

WHAT I COULD NOT VERIFY

• WebKit and Firefox were unavailable; Chromium plus exact-head CI supplied browser coverage.

WHAT CHANGED BEFORE PUBLICATION

The first gate’s review body did not explicitly name the prior request. The final 26-word body does. No code finding or verdict changed.