Review 5375
#5375 feat(Toast): add scroll-safe swipe dismissal by rubyycheung (bucket: internal teammate — DESIGNOWNERS)
06067bcba4a595e5ea2466f1bd47b16ae6959bc6 <- every claim below was verified at this commit
Touch and pen users have no direct-manipulation way to dismiss a transient toast, while the first gesture shape for this area (touch-action: none) made normal page scrolling unavailable whenever the gesture started over the toast. The public dismissal reason should also stay stable unless a consumer needs a separate value; this PR shows no consumer needing swipe separate from manual dismissal.
VERDICT: clear
When a finger or pen starts on the notice, the notice waits until the movement is clearly aimed toward the edge it came from. Movement in any other direction stays with the page or with the control the person touched; accepted movement pauses the timer, follows the finger, and closes only after enough distance or speed. The placement owner decides which edge counts, and the notice itself owns the visual feedback and close, so apps get the new gesture without a new setting or a new dismissal reason.
SOLUTION (5 decisions · ~334 runtime lines of 1346)
- touch + pen can swipe a toast toward its top/bottom placement edge [direct dismissal]
- the gesture is accepted only after dominant edge-directed travel; opposite, horizontal, and interactive-descendant starts stay native [scroll/control preservation]
- auto-hide pauses during an in-flight gesture and resumes on cancel [user is manipulating it]
- swipe still reports the existing
manualdismissal reason [no stated consumer need for a public union expansion] - placement stays owned by the viewport; the toast reads a private edge direction from that placement owner [avoid
Toast.positionpublic surface]
This is over the “2 decisions” smell, but these do not split cleanly: the gesture is not safe without scroll arbitration, timer pause, input-type choice, and the API decision. Each traces to a stated problem in the body.
VERDICT: clear
OWNER: Toast's stable card/transport owns gesture lifecycle, timer pause/resume, visual swipe feedback, and the dismiss request. ToastViewport owns placement and supplies the dismiss-edge direction.
TIER 1: Accessibility primitives (INTERACTIVE_SELECTORS, existing close Button and translated label), plus public DOM/event composition.
TIER 2: Status/feedback (info/error) and semantic close icon reuse existing Button/Icon contracts; no size cascade change.
SEAMS: Built-in content, custom renderContent, portal/top layer, and consumer interactive children.
BEHAVIOR UNIT: inline → useToastGesture — arbitration, thresholds, timing, listeners, cancellation, timer handoff, and transient swipe state form one independently testable state machine.
Driven seam table:
| Seam | Result |
|---|---|
| built-in Toast | PASS — upward edge swipe dismissed |
custom renderContent
|
PASS — custom layout still dismissed |
| portal/top layer | PASS — swipe dismissed while the manual popover was open |
| custom interactive child | PASS — dragging Undo did not dismiss; dragging the surface did |
The custom-content seam therefore does not require moving gesture ownership out of Toast: renderContent replaces the card's inner layout, not the stable card root. The architecture block is the implementation unit. Roughly 330 lines of gesture state, listeners, direction arbitration, thresholds, timing, and cancel/dismiss decisions are embedded in the rendering component and can only be exercised through the full viewport/card integration. Extract useToastGesture, pass the resolved direction as React data, and keep the hook invoked by Toast's stable root.
VERDICT: BLOCKS — the gesture protocol needs a named, focused test boundary; lifecycle ownership stays on Toast
Every app using Toast gets an additional touch/pen dismissal path. A touch user can swipe a toast away when the gesture is clearly toward the toast’s edge; sideways, opposite-direction, and control-origin gestures keep their existing behavior. Mouse users see no drag behavior change and still use the close control.
VERDICT: clear
No public prop, export, default, or union member is added. ToastDismissReason remains 'auto' | 'manual', ToastViewport.position remains the placement API, and ToastProps still has no position. The new --_toast-* vars are documented as private lifecycle values and listed in the derived-var exception test.
Changeset is present and valid: [feat] patch entry for @astryxdesign/core with @rubyycheung. pnpm check:changesets passed.
VERDICT: clear
No new theme target and no public var. Existing astryx-toast target stays on the painting element and still carries type; new private vars are explicitly not authorable theme surface. Styling remains tokenized: motion uses duration/ease tokens and transform/opacity, no raw color/spacing/radius/shadow in the changed runtime styling.
VERDICT: clear
API: no existing call site stops compiling or changes callback reason shape; swipe calls onHide('manual'). Visual: default rendered toast before a gesture is unchanged except the transform expression now includes defaulted swipe vars; before/after Chromium frame shows the same visible toast before the swipe and no toast after accepted swipe. Theme: existing target and type state still resolve; private vars are additive.
Behavior changed intentionally for touch/pen users only: accepted edge swipes dismiss. Mouse drag, close button, F6, endContent controls, opposite/horizontal touches, reduced motion, top/bottom placement direction, RTL start/end placement, and timer resume after cancellation were covered by tests or browser probes.
VERDICT: clear
Effects/listeners/layout reads:
- Existing effects kept: auto-hide timer, window blur/focus pause.
- New effect: 4 element touch listeners per visible non-exiting toast (
touchstart, non-passivetouchmove,touchend,touchcancel), removed on exit/unmount. Instrumented in Chromium: 4 adds when the toast appears, 4 removes after dismiss. - No React state/render pass is added for gesture movement; refs and inline private vars hold transient state.
- Gesture start reads layout/style once: 1
getBoundingClientRect()and 2getComputedStyle()reads. Per-move work after acceptance writes private CSS vars only; no per-move layout read. - No global listener, observer, dependency, or bundle-sized package addition.
This is new feature cost, not a measured degradation from an existing path. Worst realistic N is bounded by Toast’s visible max (default 5), so the listener count is bounded (20 touch listeners at default max, with 5 non-passive touchmove).
VERDICT: clear
- accepted top-edge swipe, before/after, real Chromium:
shots/swipe-before.png·shots/swipe-after.png - Visual difference: intentional — the toast disappears after an accepted dismissing gesture, matching the stated problem and dismissal behavior. No layout/CSS matrix is owed beyond the interaction states because this PR does not change resting layout, dimensions, colors, target names, or typography.
VERDICT: clear
Auto-covered/static: no new strings in runtime; existing close button still uses @astryx.toast.dismiss; pr-a11y did not appear in the low-count stacked checks, so I did not treat CI as a full accessibility run. Manual Chromium checks: close button keeps accessible name and dismisses; F6 moves focus into the toast onto the dismiss button; interactive descendants do not start swipe; reduced motion collapses transition duration to 0.00001s; RTL still dismisses toward the block edge. Unit tests cover touch cancellation, timer resume, top/bottom RTL, no mouse drag, endContent descendants, and manual reason.
No forced-colors-specific painted state was introduced by the diff; swipe feedback is transform/opacity and the visible close remains the existing Button.
VERDICT: clear
Verdict lines:
- PROBLEM clear
- SOLUTION clear
- ARCHITECTURE BLOCKS — complex gesture protocol has no named testable unit
- IMPACT clear
- API clear
- THEMING clear
- BREAKING clear
- PERFORMANCE clear
- VISUAL EVIDENCE clear
- A11Y & I18N clear
GOAL: met — real Chromium proves edge-directed touch/pen swipes dismiss while opposite/sideways gestures and interactive children retain their behavior.
DISPOSITION:
- inline gesture state machine → blocks now — extract
useToastGesture, invoked from the stable Toast root - placement direction recovered from private CSS → blocks with the same extraction — pass resolved direction into the hook as React data
- custom
renderContentseam → accepted (driven) — default and custom content both preserve swipe and interactive-child arbitration
WORST OUTCOME: “arbitration, thresholds, timer behavior, and cancel/dismiss decisions are only testable through the full viewport/card stack” → request changes under audit C22 / critic R35.
JUDGEMENT NEEDED: none — the capability and lifecycle owner are settled; this is an implementation/test boundary.
request changes
- [BLOCKS] The full swipe protocol is embedded in
Toast.tsx→ one rendering component owns structure plus ~330 lines of gesture state, listeners, thresholds, timing, and timer handoff; focused behavior tests require the whole viewport/card stack - [covered by the same ask] Toast reads placement direction back from private CSS
→
useToastGestureshould take the resolved edge direction as React data from the placement owner
Thanks, this is good, and I checked it against the custom renderContent branch: the card still owns the gesture, so custom layouts keep swipe dismissal.
One architecture thing I'd like before merge: pull the swipe behavior into a named useToastGesture-style unit, still invoked by the Toast card root. The hook would take the resolved edge direction, pause/resume hooks, dismiss callback, enabled/exiting state, and interactive-descendant policy; it would return the root binding plus transient swipe updates and cleanup. That keeps custom content safely inside the card while making direction arbitration, thresholds, timer pause, and cancel/dismiss decisions independently testable instead of buried in the render file.
Could you make that split while keeping the hook attached to the Toast card, not the custom content?
[Reviewed by Robohands and Cindy]
0
- Open #5428 adds
renderContent; its patch does not apply cleanly over this stacked head, so I inspected the seam but did not execute the combined branch. CurrentendContentcomposition was driven in Chromium. - PR checks listed only Meta CLA, Vercel, and Vercel Preview Comments because this is stacked on #5353; I ran focused local checks instead.
- Prior GitHub review already called the shape right and asked only rationale nits; this draft does not depend on that for the verdict.
- Added the architecture routing slot and required every composition seam to be driven rather than inferred from source.
- Distinguished behavior isolation from lifecycle ownership: extract the gesture state machine into a focused
useToastGesture-style hook, but keep it invoked by the stable Toast root. - Kept the review focused on that architecture request after real Chromium proved built-in content, custom
renderContent, the top layer, and interactive custom children all preserve swipe behavior.
#5375 feat(Toast): add scroll-safe swipe dismissal by rubyycheung (bucket: internal teammate — DESIGNOWNERS)
Prior review (R1e): cixzhang first called the swipe shape right and accepted the vertical/pen/changeset rationale, then asked for the gesture state machine to move into a named useToastGesture unit while staying attached to the Toast card root. Head b8c39d2895f satisfies that architecture ask. xinzi-bot then reviewed this exact head and correctly noted the still-mounted standalone Toast and stale stacked base; this presentation extends rather than contradicts both reviews.
STEP 0: trusted facebook-owned head. The eight changed paths are source, tests, Storybook, docs, and a changeset: no dependency, lockfile, package script, workflow, shell, or build-time network change.
Main movement: #5353 merged to main, but this PR still targets its old head branch. A merge-tree against current main conflicts in five touched files, so retarget/rebase remains required before merge.
b8c39d2895f35d9c5f3459cc11f7d0cf65697dbd <- every claim below was verified at this commit
LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.13
LANE: full
WHY: this is a behavior feature with a new Effect and element listeners, intended visual feedback, an unresolved prior changes-requested review, and a conflict with current main.
WHY 1: touch and pen users have no direct-manipulation dismissal gesture; the first attempt to add one disabled native scrolling over the whole Toast. WHY 2: someone trying to scroll from a notification could lose the page gesture, while someone intentionally dismissing it needs predictable feedback and direction. WHY 3: transient feedback must stay dismissible without trapping or stealing the surrounding page interaction.
USER-FACING PROBLEM: a touch or pen user should be able to move a Toast away without the notification taking over unrelated page scrolling or controls. PROBLEM SEVERITY: polish — the visible close button already completes dismissal; swipe improves directness rather than unlocking an otherwise blocked task.
VERDICT: clear
A Toast waits until a finger or pen is clearly moving toward the edge the Toast came from before taking over the gesture. Once accepted, it follows the contact, pauses auto-hide, and dismisses after enough distance or speed; all other movement stays native. Placement supplies the edge direction, while a named internal gesture hook owns arbitration, transient styling, listeners, and cleanup on the stable Toast card root.
SOLUTION (5 decisions · ~429 runtime lines of 1,427 changed lines)
- touch and pen can swipe a Toast toward its top/bottom placement edge — direct dismissal
- only dominant edge-directed movement is accepted; opposite, horizontal, and interactive-descendant starts stay native — preserve page/control interaction
- auto-hide pauses during the gesture and resumes on cancellation — preserve the notification while manipulated
- swipe reports the existing
manualreason — avoid public union expansion -
ToastViewportresolves direction anduseToastGestureowns the gesture unit on the Toast root — satisfy the prior architecture ask
BURDEN: high — a 361-line gesture hook, one Effect, four element listeners per visible Toast, transient inline vars, threshold/timer state, and a broad behavior matrix. BURDEN MATCH: proportionate, human-settled — cixzhang's prior review accepted the capability and required this extraction; the measured runtime cost is bounded and adds no gesture-time React commits.
VERDICT: clear
OWNER: ToastViewport owns placement; the Toast card root owns the gesture lifecycle; useToastGesture owns the gesture state machine.
TIER 1: existing close Button/translated label, INTERACTIVE_SELECTORS, public DOM event composition.
TIER 2: status/feedback semantics remain info | error; no new size or status protocol.
SEAMS: viewport-managed Toast, standalone public Toast, interactive descendants, top/bottom placement, RTL.
BEHAVIOR UNIT: useToastGesture — named hook for refs, thresholds, touch listeners, pen bindings, timer handoff, and cleanup; invoked by the stable card root.
| seam | driven result |
|---|---|
| viewport-managed, real touch | pass — edge swipes dismiss; 45° scrolls 211px without engagement; 55°/63° dismiss |
| top/bottom + RTL | pass — focused unit tests cover both block-edge directions under RTL |
| interactive descendants | pass — Button and role-based descendant starts do not begin swipe |
standalone public Toast
|
note — callback fires, but a no-op callback leaves the card translated 200px at 0.6 opacity |
The requested extraction is complete and direction now arrives as React data rather than a computed-style read. The remaining standalone reset is correctness inside the new unit, not an ownership or public-shape disagreement.
VERDICT: note — the architecture ask is addressed; standalone successful-dismiss cleanup is incomplete
Every rendered Toast gets the touch/pen gesture. Viewport-managed users can dismiss toward the configured edge while horizontal, opposite, and unresolved movement stays with the page; mouse users keep selection and the close control. A builder rendering public <Toast> directly must currently unmount synchronously from onDismiss, or the still-mounted notice remains shifted and faded after a successful gesture.
VERDICT: note — standalone callers can retain transient swipe styling
No public prop, export, default, or union member changes. ToastDismissReason stays 'auto' | 'manual'; ToastProps still has no placement prop. ToastSurface, ToastGestureDirection, and useToastGesture are source-internal and absent from package barrels.
OSSIFICATION: no public surface is added. The behavior meaning of existing Toast expands to touch/pen swipe, but that capability was explicitly accepted in cixzhang's prior review; private --_toast-* vars remain documented as private.
VERDICT: clear
No target, token, themeable var, or override contract is added or removed. astryx-toast remains on the painting element with type; the five swipe/motion vars are private lifecycle state, and the changed runtime styling uses transform/opacity plus existing duration/ease tokens.
VERDICT: clear
BEHAVIOR: intentionally changed for touch/pen; accepted edge movement now dismisses, while mouse, interactive descendants, opposite/horizontal touch, timer cancellation, close, F6, and manual dismissal retain their paths. Standalone no-op dismissal is the uncovered boundary. API: no — signatures, defaults, exports, and dismissal reason are unchanged. VISUAL: resting main and head frames are byte-identical; active swipe intentionally translates/fades/scales. A standalone successful swipe can persist at 200px / 0.600 opacity after the callback, which is not transitional after 500ms. THEME: no — existing target and override resolution are unchanged; added variables are private.
VERDICT: note — non-shape correctness finding on re-review; fix before merge under R1e
EFFECTS: one new useEffect [beginGesture, enabled, endGesture, moveGesture, resetGesture] in useToastGesture.ts:295 synchronizes four native touch listeners because touchmove must be conditionally non-passive; cleanup removes the same four.
| Effect + deps | external system | why render/handler cannot do it | measured render cost | lifetime + cleanup | focused test |
|---|---|---|---|---|---|
useEffect [beginGesture, enabled, endGesture, moveGesture, resetGesture] |
native touch dispatch | React's delegated touch path cannot opt one movement into non-passive cancellation after intent resolves | 0 React Profiler commits for a cancelled gesture at N=1/3/5 | 4/12/20 listeners added at N=1/3/5; 4/12/20 removed on unmount |
ToastViewport.test.tsx covers touch handoff/cancel; Chromium probes cover real dispatch |
RENDER: 0 additional React commits during a short accepted/cancelled gesture at N=1, 3, and 5.
LISTENERS/OBSERVERS: four element listeners per enabled Toast, bounded to 20 at the default five visible; all removed on exit/unmount. No global listener or observer.
LAYOUT: one getBoundingClientRect() at gesture start; accepted moves write transform/opacity vars and do no layout read. The extraction removes the two computed-style direction reads from the prior head.
BUNDLE: no dependency; one internal 361-line source unit.
Reproduction: SB_PORT=6375 OUT=review-artifacts/pr-5375/perf-measurement.json node probe-kit/toast-gesture-perf-5375.cjs; the exact temporary Storybook arm is banked at review-artifacts/pr-5375/perf-arm.diff.
VERDICT: clear — measured feature cost, no degradation in an existing outcome
VISUAL CHECK: manual frames required
WHY: this feature intentionally changes rendered transform/opacity during interaction, visual-acceptance is pending with no exact-head visual workflow run, and the standalone post-dismiss endpoint is not covered by a visual gate.
| Before — same head, resting | After — same head, callback returned, 500ms settled |
|---|---|
 |
 |
SENSOR RECEIPT: head b8c39d2895f35d9c5f3459cc11f7d0cf65697dbd; story core-toast--narrow-layout-reference; neutral/light/LTR; 900×500@1; forced colors false, reduced motion false, coarse pointer false, hover true; exactly one visible .astryx-toast; message present; role status; aria-live=polite; one named dismiss button; fonts loaded; zero running animations and page/Storybook errors. Before state has no swipe vars. After real Chromium touch input has --_toast-swipe-y: 200px, exit 120%, opacity 0.600, scale 0.980. The only state difference is the driven gesture.
Reproduction: ROOT=<review worktree> EXPECT_SHA=b8c39d2895f SB_PORT=6375 ARM=head OUT=review-artifacts/pr-5375/shots node probe-kit/toast-standalone-swipe-5375.cjs. Touch intent matrix: PORT=6375 node probe-kit/toast-swipe-5375.cjs. Pen path: SB_PORT=6375 OUT=review-artifacts/pr-5375/pen-measurement.json node probe-kit/toast-standalone-pen-5375.cjs.
Control: main resting frame at 8bb01b1f095fb3eaf8d12c40c17bdb302e96f442 is byte-identical to the head resting frame (SHA-256 e9f4c9ec…). The persistent post-callback displacement is unintentional; the body promises dismissal, and the standalone story's required callback deliberately leaves the component mounted.
VERDICT: note — proven visible standalone defect, non-shape new finding on R1e re-review
Static/auto-covered: no new user- or AT-facing runtime string; the close Button keeps @astryx.toast.dismiss; role/status, aria-live, and aria-atomic are unchanged; no locale or directional glyph change. Real Chromium: viewport-managed touch preserves 45°/opposite scrolling, accepted swipes dismiss, and the visible close remains present. Unit coverage exercises interactive descendants, timer resume, top/bottom RTL, mouse exclusion, manual reason, and touch listener cleanup.
pr-a11y and pr-rtl did not run on this stacked head, so they are not cited as green. No new baseline entry exists in the PR delta.
VERDICT: clear — changed interaction paths driven; no semantic/string contract change
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | clear |
| ARCHITECTURE | note — extraction addressed; standalone cleanup incomplete |
| IMPACT | note — direct Toast can retain swipe styling |
| API | clear |
| THEMING | clear |
| BREAKING | note — new non-shape re-review finding |
| PERFORMANCE | clear — measured Effect cost |
| VISUAL | note — persistent translated/faded standalone state |
| A11Y & I18N | clear |
GOAL: partly met — the viewport-managed path passes the seven-case real-touch matrix and the architecture request is satisfied; public standalone Toast does not return to rest when its dismissal callback leaves it mounted.
DISPOSITION: standalone reset at useToastGesture.ts:227 → accepted only as an R1e non-shape re-review note, already owned by xinzi-bot's current-head review and required before merge; retarget/rebase → same owned note; no other negative finding.
ADVICE: bounded direction — preserve the viewport exit animation, but prove a still-mounted standalone Toast clears every transient swipe var after the callback; the existing current-head review already states this acceptance test.
AUTHOR CAN PROCEED: yes — retarget/rebase onto main, resolve the five conflicts, reset the still-mounted standalone Toast after successful swipe, and add the regression test; no human-owned API/design decision remains.
WORST OUTCOME: “a builder rendering public <Toast> directly must currently unmount synchronously from onDismiss, or the still-mounted notice remains shifted and faded” → approve-with-note under R1e because this is a new correctness finding, not public API/theming/scope shape.
JUDGEMENT NEEDED: none — cixzhang already accepted the swipe capability and card-root ownership; the prior architecture ask is satisfied.
approve with notes
- [not re-blocking under R1e] successful standalone swipe does not clear transient styles when
onDismissleaves it mounted → a static/direct Toast remains 200px displaced and 40% faded after the callback ·useToastGesture.ts:227 - [mechanical, already surfaced] the PR still targets the merged stack base and conflicts with current
main→ the author cannot get normal CI or merge this head until retarget/rebase · PR base + merge-tree
Thanks — my earlier review asked for this extraction, and
useToastGesturenow provides the right boundary. xinzi-bot’s reset/rebase notes are valid; please fix both before merge.[Reviewed by Robohands]
0 — xinzi-bot already anchored the current-head standalone issue; repeating it adds no action.
- 126 targeted tests, core typecheck, docs typecheck, and core build pass at the exact head.
- Targeted ESLint has zero errors and one inherited
no-style-only-wrapperwarning on the wrapper already present in the clean #5353 base. - Resting main/head frames are byte-identical; the PR adds no resting visual delta.
TIME total 16m setup 3m warm main reused: yes; exact-head worktree + clone/reconcile install + Storybook dev on port 6375 reading 5m current rules, prior reviews, swipe-only delta, source, tests, conventions, current rubric measuring 5m 126 tests; build/typechecks/lint; 8 real-touch cases; pen path; 3 sensor captures; N=1/3/5 Effect scaling writing 3m round-2 presentation + two critic gates + wiki record waste 2m first state sensor selected Undo text; first swipe probe read the pre-underscore var; both were corrected and re-run
- Physical iOS touch; real Chromium CDP touch/pen dispatch verified the browser event contract instead.
- Full PR CI,
pr-a11y, andpr-rtl; the stacked target produced only one skipped workflow, Vercel, CLA, and a pending visual status.
No public PR action was taken: this was a read-only re-review. The presentation passed gate 2 after gate 1 was corrected to include exact reproduction commands for every measured claim.