Review 5353

Review 5353 — Toast: responsive and interaction behaviour

#5353 · rubyycheung · internal · draft Head reviewed c0c6941c9b4b (+2777/−156, 9 files) Verdict comment — posted 2026-08-24 UTC, with three inlines. Comment rather than request-changes because it is a draft; the review says the safe-area half would be taken today.

Two passes. Pass one found every one of the changes Cindy blocks on and drew no conclusion from any of them. That contrast is why this page exists, and it is below.

Problem

As stated in the PR: a toast against a notched phone's raw viewport edge, plus padding and wrapping that break at narrow widths. That is the whole of the written problem — six runtime changes ship against it.

Solution

Safe-area gutters and a width constraint; padding, alignment and wrapping; CSS-native entry/exit motion — and then four more, each changing behaviour for every existing consumer:

decision	traces to a stated problem?
safe-area gutters + width constraint	yes — the notch
layout / padding / motion polish	yes, same
one toast at a time below 640px, the rest queued	no
isAutoHide false whenever endContent is set	no
swipe-to-dismiss, 'swipe' added to ToastDismissReason	no
the addToast announce moved into an effect	no

Six decisions, four of them piggybacked — the rule this produced is R29.

Impact

Everyone on the next release, no opt-in. Four of the six change what an existing addToast call does.

API

~ ToastDismissReason: + 'swipe'                 (public, core barrel)   · types.ts:15
~ Toast.isAutoHide default → false whenever endContent is renderable    · ToastViewport.tsx:520

Ossification

'swipe' widens a union the library hands the consumer, permanently. Pass one argued for it — "a genuine third member of an existing enumeration" — a rationale the PR itself never offered. Cindy: "a swipe is arguably also manual, so why the third classification — what does it gain".

Performance & resources

• Render — the announce moved from an imperative call to an effect that sets state: +1 render pass per visible-toast change, plus announcedIdsRef, an unbounded Set existing only to suppress the re-runs the effect created. · ToastViewport.tsx:395
• Listeners — a ResizeObserver and a window resize listener watching the same number; one is redundant.
• Size — +2777 lines. Cindy: "2777 new LOC is a smell for sure." ~480 of it is runtime, under ~2,300 of stories, tests and docs, and that ratio is what let four unargued behaviour changes arrive looking like housekeeping.

Judgement

comment — take the safe-area half; the other four need their reasons written down, and two need fixing wherever they land.

1. Four runtime changes with no problem statement anywhere in 2,777 lines
   → a reviewer cannot weigh a change whose reason is not written
     · ToastViewport.tsx:390, :520, :395 · Toast.tsx:100 · types.ts:15
2. At 375px a persistent toast never leaves the head of the queue
   → five fired, one shown at 20s; main shows three   · ToastViewport.tsx:390
3. `touch-action: none` kills page scroll under the toast
   → a finger that lands on the toast cannot scroll the page   · Toast.tsx

The review, as posted

Thanks — the safe-area and wrapping work is the right fix and I'd take that half today. Commenting rather than blocking only because it's a draft.

Six runtime changes ship here and only the safe-area one has a problem written down. These four each change behaviour for every existing consumer, and I couldn't find the sentence saying what was wrong before:

• one toast at a time below 640px, the rest queued (ToastViewport.tsx:390)
• isAutoHide now false whenever endContent is set (:520)
• swipe-to-dismiss, and 'swipe' added to ToastDismissReason (Toast.tsx:100, types.ts:15)
• the addToast announce moved into an effect (:395)

Two I'd want fixed wherever they land: at 375px a persistent toast never leaves the head of the queue — five fired, one shown at 20s, main shows three — and touch-action: none kills page scroll under the toast.

Could you write up the problem each of those four solves, and split out any that want their own discussion? Happy with several problems in one PR — I just can't weigh a change I can't see the reason for.

[Reviewed by Robohands]

Inlines:

ToastViewport.tsx:520 — Any toast with endContent is persistent now. Why was the old default wrong?

ToastViewport.tsx:395 — Hmm, was the addToast announce not enough? The comment you deleted said why it lived there.

Toast.tsx:100 — BottomSheet keeps this on the handle and does the handoff in a non-passive touchmove — useSheetGestures.ts:1186.

Pass one → pass two — what actually changed

Both passes read the same diff. The findings did not change; the conclusions did.

the change	pass one	pass two
isAutoHide default flips	listed accurately in API and IMPACT	"Why was the old default wrong?" — a finding
'swipe' joins the union	OSSIFICATION argued for it	"what does it gain" — a finding
announce becomes an effect	PERFORMANCE called it "the smell"	+1 render pass, priced, and the Set named — a finding
six decisions	named mid-review, then dropped	the closing ask
the closing line	a design question about swipe direction	write up the four problems, split what needs it

Three things this cost, each now a rule:

• The loop blocks on what it can measure breaking and merely describes what it cannot. All three of Cindy's objections were changes the PR made and never justified; pass one found all three and drew no conclusion → R1j.
• Worse than a miss: pass one built the author's case. On 'swipe' it reasoned its way to a justification the PR never offered. An unargued change with a reviewer-supplied rationale is strictly worse than an unargued change, because the reviewer has laundered it.
• The closing ask is the verdict. Pass one named the split mid-review and then closed on a design question — so the author would have answered the design question and the split would have evaporated.

And the correction Cindy made to the first draft of R29 itself: "I actually don't mind more than one problem per PR, but all problems and solutions should be recorded, not piggybacked." Plural problems are fine; unrecorded ones are not.

Effects and listeners got their own rules from the same review — R18e (an effect that sets state costs a render pass; price it) and R18f (a listener is fine when scoped to the moment it is needed).

Calibration

#5352, same author, same day: ~50 runtime lines, 2 decisions, merged within the day. This one: ~480 runtime lines, 6 decisions, round two. See Review-Loop-2026-08-23.

---

Round two — 2026-08-24, head b889b4c8852f (+1330/−74, 8 files, undrafted)

Verdict request-changes. Run for the 0.5.0 cut at 11:00 PT; recommendation was do not include in this cut. Read-only — nothing posted. Banked at astryx/release_blocks kt-o26i. Frames: assets/pr-5353.

Round one's asks are closed, and that was checked rather than believed

All four unrecorded changes are gone from this head and both defects moved to #5375 / #5376. types.ts is -5/+1 — a prettier reflow of ToastPosition, no union member touched. Every one of the six "Problem and rationale" bullets now traces to a change and back. R29's remedy worked: the author wrote the problems down and split what needed splitting, and the count going 6 → 6 while the findings went to zero is the point — plural problems are fine, unrecorded ones are not.

The two findings, and where they came from

1. Toast.position is public surface that positions nothing · Toast.tsx:159. Only picks the motion drift sign and the transform-origin corner; same name and type as ToastViewport.position (ToastViewport.tsx:143), which really does place things. Dropping it and letting the wrapper set --_toast-enter-y and --_toast-origin — custom properties inherit — reproduces the entry drift, the scale and the RTL origin exactly, at −50/+32 lines and no public API. Built in a scratch worktree, typechecked, measured, reverted.
2. The retune is tuned single-line-only · Toast.tsx:109. align-items: center unconditionally moves Undo and × off the first line on any wrapped toast (× centre 406→434 at 900px, 586→624 at 375px, against first-line mids of 414 and 594). Banner does the opposite deliberately — flex-start with headerCentered only when single-line (Banner.tsx:230, :250, :572) — and keeps the −4px end-area margin this drops.

paddingBlock 16→12 folded into (2) under R30 rather than standing as a third finding: on a single-line toast the 8px it frees is taken straight back by dropping that −4px margin, so the box is 52px before and after. The headline change of the changeset is invisible in the common case, and visible only on the multi-line toast whose controls the same decision displaces.

Toast.position was missed in round one, not added by the split

It is present at the previously reviewed head c0c6941c9b4b:200. Round one's API slot listed ToastDismissReason: + 'swipe' and the isAutoHide default and not this. A round that is looking for unrecorded behaviour changes can walk past a new public prop — the API slot has to be enumerated from the source, not from the diff's loudest lines.

The critic overturned the reviewer, and was right

The first draft came in at approve-with-nits — internal author, nothing broken, escalate the taste call. The independent critic (blind to the code and to the reviewer's reasoning) failed it on R1h (a measured, reachable visual change is a regression, and the bucket has no exception) and R1g/R1g-api (a prop a consumer can set to something untrue), and called the class "not arguable". It was right and the verdict changed.

R2 passed on the first draft — 23 words against a 30 cap, inlines 12/13/5. First run in the kit's history where length was not the failure. What failed instead was structure: twelve missing slots, and an unstructured "what I verified" list standing in for them. Worth watching whether that trade is a pattern — a draft that is short because it dropped the slots is not a short draft.

Three catches from the critic that changed the evidence rather than the wording:

• R14f — max(16px, env(…)) had been measured only at the default. Driving the gutter token to 0px on both builds returns 0px on both, because env() is 0 in Chromium. So the theme axis is unchanged, which is a stronger and more honest answer than "inert on every surface I can test".
• R2l — the --_toast-enter-y alternative was proposed, not built. Building it turned an opinion into a measurement.
• R14h, self-inflicted — the review nearly reported that this PR would break main's new apiContractDrift test, which #4315 added this morning. #5382 deleted that test the same day. A confident merge-hazard claim, killed by the check that was supposed to confirm it.

What nobody has verified, on either round

env(safe-area-inset-*) is 0 in Chromium including device emulation, so the safe-area path — the PR's own headline — is inert on every surface the loop can reach. The author cites an iOS simulator; the loop cannot. Said plainly rather than assumed, per the brief.