Skip to content

Review 5428

Jump to bottom
Cindy Zhang edited this page Aug 26, 2026 · 5 revisions

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)

Clone this wiki locally