Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
needs:spec-owner-review
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: the dismissal tap, the scroll close and counting a settling row as open extend spec:AST-057 FR7 through an amendment the PR body calls pending but that is not yet in the spec, and the tap conflicts with AR3, which keeps the row's primary activation unchanged. An engineering owner (the author can act as one) approves, amends or rejects those rules by writing them into AST-057, and the draft AST-094 and Item.spec.md requirements this compare also carries are approved by an engineering owner commenting /approve-spec followed by the full head SHA. Independently, please rebase onto main once #8097 and its predecessors land so this PR's diff is its own 313-line commit rather than 28,264 lines, and amend AST-057 FR2 and FR5 (or conform the release rule) in the predecessor that changes them.
- AST-094 FR7 and its related-records list cite
spec:AST-090, which does not exist in the repository.
[Automated review]
| new TapGestureRecognizer({ | ||
| allowedPointerTypes: ['touch'], | ||
| name: 'item-swipe-dismiss', | ||
| shouldReceiveTouch: (_tap, touch) => |
There was a problem hiding this comment.
While the row is open this recognizer takes every touch tap on the row's content and cancels its click, so the row's onClick or href cannot fire by touch until the row closes. spec:AST-057 AR3 says swipeActions MUST NOT change the primary element's activation, and FR7 does not list a tap on the row's own content among the closing events. The FR7 amendment the PR body mentions needs to land in AST-057, with an owner ruling on AR3, before this can be approved.
| if (behavior === 'commit') { | ||
| return 'closed'; | ||
| } | ||
| if (velocity >= swipeActions.releaseVelocity) { |
There was a problem hiding this comment.
spec:AST-057 FR5 requires a fling to fire the outermost entry, under both reveal and commit; this rule never fires on a flick short of the commit point. Either amend FR5 with owner approval or conform the release rule.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
There is no spec:AST-090 in the repository, at this head or on main, so this departure note points at nothing an owner can read. Cite the record by an id that resolves, or drop the note until that record lands.
75e368f to
1f1f261
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: closing an open swipe row on a tap on its own content (which then activates nothing), on a scroll of anything containing it, and while it is still settling open extends spec:AST-057 FR7 through an amendment the PR body calls pending, and the tap conflicts with AR3, which keeps the row's primary activation unchanged. An engineering owner approves, amends or rejects those three rules by writing the decision into AST-057 and commenting /approve-spec <full head SHA> on this pull request, which also covers the draft spec:AST-094 and the Item.spec.md requirements this compare carries; the author may do so as an engineering owner. Independently, please rebase onto main once #8097 and its predecessors land so this diff is its own 313-line commit rather than 28,292 lines, and amend AST-057 FR2 and FR5 (or conform the release rule) in the predecessor that changes them.
- AST-094 FR7 and its related-records list cite
spec:AST-090, which does not exist in the repository.
[Automated review]
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsItem (@astryxdesign/core) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included.
|
| Component | JavaScript | CSS |
|---|---|---|
| core/BreadcrumbItem | +31.2KB |
-46B |
| core/CheckboxListItem | +31.0KB |
-115B |
| core/ContextMenu | +31.3KB |
-88B |
| core/ContextMenuItem | +30.8KB |
-98B |
| core/DropdownMenu | +30.6KB |
-83B |
| core/DropdownMenuCheckboxItem | +30.7KB |
-93B |
| core/DropdownMenuItem | +30.6KB |
-98B |
| core/DropdownMenuRadioItem | +30.7KB |
-93B |
| core/DropdownMenuSubMenu | +30.6KB |
-89B |
| core/Item | +30.5KB |
-107B |
| core/ListItem | +31.0KB |
-115B |
| core/Markdown | +31.3KB |
-125B |
| core/MoreMenu | +30.6KB |
-83B |
| core/Pagination | +31.0KB |
-131B |
| core/PowerSearch | +31.3KB |
-67B |
| core/RadioListItem | +31.0KB |
-114B |
| core/Selector | +31.8KB |
-89B |
| core/SelectorOption | +30.7KB |
-104B |
| core/Table | +31.3KB |
-110B |
| core/TableCell | +31.3KB |
-146B |
| core/TableHeaderCell | +31.3KB |
-133B |
Bundle Size Summary
Gzip figures measure each package's entry file, not the code an import pulls in.
| Package | Entry (ESM) | Entry gzip before | Entry gzip after | Delta |
|---|---|---|---|---|
| @astryxdesign/core | 4.9KB | 1.2KB | 1.2KB | 0B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 34.6KB | 34.5KB | -104B |
Accessibility Audit
Status: No accessibility violations detected.
Visual Regression
55 of 504 shot(s) changed.
A change here is a question, not a failure: check whether the after is the
picture you intended. Record that review on the PR. Baseline maintenance is an
explicit dispatch of CI; this report never rewrites the baseline or adds a release gate.
| component | story | theme | mode | pixels |
|---|---|---|---|---|
| Selector | Indicator Space Bottom Sheet Narrow End RTL | neutral | light | 753,933 |
| Selector | Indicator Space Bottom Sheet Wide Start | neutral | light | 740,072 |
| RichTextEditor | Markdown parity: side by side | probe | dark | 558,840 |
| RichTextEditor | Markdown parity: side by side | probe | light | 540,756 |
| Chat | Full AI Chat | probe | light | 230,823 |
| Chat | Full AI Chat | probe | dark | 229,805 |
| AppShell | With Mobile Nav | probe | light | 217,010 |
| AppShell | With Mobile Nav | probe | dark | 216,960 |
| Collapsible | Dividers | probe | dark | 66,445 |
| Collapsible | Dividers | probe | light | 66,445 |
| AppShell | Playground | neutral | dark | 65,198 |
| AppShell | Playground | neutral | light | 63,788 |
| Chat | Default | neutral | dark | 36,304 |
| Chat | Default | neutral | light | 35,767 |
| Chat | Empty State | probe | light | 23,531 |
| Chat | Empty State | probe | dark | 22,622 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | dark | 15,378 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | light | 14,107 |
| Chat | With Attachments | probe | light | 13,406 |
| Chat | With Attachments | probe | dark | 11,232 |
and 35 more.
Generated by PR Enrichment workflow | Storybook | Sandbox | View full report
1f1f261 to
6573ea8
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: the new swipe-row rules (a tap on an open row closes it without activating it, and any scroll closes it) change spec:AST-057 FR7 and AR3, and the proposed Item.spec.md claims and the draft spec:AST-094 this branch carries have no approval for this head. An engineering owner records the decision by commenting /approve-spec 6573ea83ee6ed43887b9a4d10ef076537253b05c on this pull request, and the author may do so as an engineering owner; AST-057 itself still needs the amended text. Whatever the decision, these need changes:
- Rebase onto the landed predecessor pull requests so this one is the 402-line dismissal change alone; today it carries 28,381 authored lines.
- Close the row only for scrolls a finger drove, because moving focus to an entry that is partly out of view now closes the row and drops that entry (AST-057 AR1).
- Report the open state from the swipe handlers instead of the
useEffectinItemSwipeLayer.tsx(react-component-runtime INV2 and INV3).
[Automated review]
| const onScroll = (event: Event) => { | ||
| const root = rootRef.current; | ||
| const {target} = event; | ||
| if ( | ||
| root != null && | ||
| (target === document || | ||
| (target instanceof Element && target.contains(root))) | ||
| ) { | ||
| close(); | ||
| } |
There was a problem hiding this comment.
This closes the open row for every scroll of an ancestor, including the scroll the browser makes when focus moves to an entry that is partly out of view: the row closes, the panel turns inert and focus falls back to the row, so a keyboard or screen-reader user loses the entry (AST-057 AR1). Closing only for a scroll a finger drove, as utils/pressFeedback.ts already decides, or ignoring scrolls while focus is inside the row, keeps the entry reachable.
| useEffect(() => { | ||
| if (!isOpen) { | ||
| return undefined; | ||
| } | ||
| onOpenChange(true); | ||
| return () => { | ||
| onOpenChange(false); | ||
| }; | ||
| }, [isOpen, onOpenChange]); |
There was a problem hiding this comment.
This Effect calls onOpenChange in reaction to the swipe state and names no external system, which architecture:react-component-runtime INV2 and INV3 rule out; Item then renders once more to drop data-astryx-pressable. Reporting open and closed from the useSwipeAction transitions that change the state, or letting Item read isSwipeOpen directly, avoids the relay.
6573ea8 to
d021d98
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: on iOS a tap on an open swipe row now closes it without activating it, any scroll closes it, and a row still springing open counts as open, which extends spec:AST-057 FR7 and conflicts with AR3, and the Item.spec.md requirements and the draft spec:AST-094 this branch carries have no approval for this head. An engineering owner records the decision by commenting /approve-spec followed by the full head SHA on this pull request (the author may do so as an engineering owner), and AST-057 itself still needs the amended text. Whatever the decision, these need changes:
- Close the row only for scrolls a finger drove, or not while focus is inside it, because moving focus or the VoiceOver cursor to a partly hidden entry now scrolls, closes the row and drops that entry (AST-057 AR1).
- Report the open state from the swipe transitions instead of the Effect in
ItemSwipeLayer.tsx(react-component-runtime INV2 and INV3). - Rebase onto the landed predecessor pull requests so this one is the 464-line dismissal change alone; today it carries 34,764 authored lines.
[Automated review]
d021d98 to
899d805
Compare
|
Thanks for the review. Where each point stands on the new head:
|
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: on iOS a tap on an open swipe row now closes it without activating it, a scroll closes it while focus is outside the row, and a row still springing open counts as open, which extends spec:AST-057 FR7 and conflicts with AR3, and the Item.spec.md requirements and the draft spec:AST-094 this branch carries have no approval for this head. An engineering owner records the decision by commenting /approve-spec followed by the full head SHA on this pull request (the author may do so as an engineering owner), and AST-057 itself still needs the amended text. The focus guard on the scroll close and the transition report settle the earlier AR1 and INV2 points; whatever the decision, these still need changes:
- Let the entries work from the moment the row counts as open: the panel stays
inertuntil the iOS settle ends, about 3.5 s after the release, so a tap on Archive or Delete in that window closes the row and fires nothing. - Remove the unmount cleanup Effect in
useSwipeAction.tsthat reportsonOpenChange(false)and letItemclear its open flag during render (react-component-runtime INV3). - Rebase onto the landed predecessor pull requests so this one is the 517-line dismissal change alone; today it carries 35,007 authored lines, including an iOS
reducedMotion: 'keep'that contradicts AST-057 FR5's reduced-motion rule onmain.
[Automated review]
| @@ -247,76 +283,87 @@ export default function ItemSwipeLayer({ | |||
| const outer = entries[entries.length - 1]; | |||
| const isCommit = behavior === 'commit'; | |||
| const isLive = isSwipeResting && !isCommit; | |||
There was a problem hiding this comment.
On iOS the row now counts as open from the release (isSwipeOpen in useSwipeAction.ts), but this keeps the panel inert until the phase is exactly resting. An inert panel is not hit-testable, so a tap on Archive or Delete during UIKit's overdamped settle (about 3.5 s for a row released 72 px short of open, over 2 s from 5 px) lands on the row or the list behind it, and the dismissal tap or the outside-pointer close shuts the row without firing the entry. Making the panel live, and activateEntry accept, from the same isSwipeOpen would keep spec:AST-057 FR5's "every entry tappable" true under the new open-from-release model; a test that taps an entry before settled() would cover it.
| // Releases the open report when the row leaves while open. | ||
| useEffect( | ||
| () => () => { | ||
| if (settleTimerRef.current != null) { | ||
| clearTimeout(settleTimerRef.current); | ||
| const {idiom: current, onOpenChange: report} = latestRef.current; | ||
| if (current === 'ios' && isSwipeOpen(stateRef.current)) { | ||
| report?.(false); |
There was a problem hiding this comment.
This cleanup calls onOpenChange(false) from an Effect, and its comment names no external system: architecture:react-component-runtime INV3 asks every new Effect to name the one it synchronizes, and what this keeps in step is Item's own isSwipeOpen state. Item can clear that flag during render when it stops rendering the swipe layer (when hasSwipe turns false), which removes the need for this Effect.
| * animators on tracking and release, and the full swipe still performs, | ||
| * Oct 23 2026 | ||
| */ | ||
| reducedMotion: 'keep', |
There was a problem hiding this comment.
spec:AST-057 FR5 on main now says: "Under reduced motion the slide out, the spring back, and the hold-out follow design:reduced-motion: the row reaches its resting place with no travel or spring and cross-fades (DR2), because the swipe's own touch pointer started it." keep springs the iOS settle, slide-out and spring back under Reduce Motion, so this needs an owner-approved FR5 amendment (the draft AST-094 FR8 argues for one) or the cross-fade.
899d805 to
11a6649
Compare
github-actions
Bot
added
the
needs:design-review
… the inline style it reads - The motion registry, the host's engine gate and the README name the presented hit test as an engine workaround (spec:AST-013 FR6) with its review condition (re-measure on each iOS release; remove it once iOS WebKit hit-tests a compositor-moved element where it is drawn) and its regression fixture. - While a curve runs the animator holds the value it starts from inline, so presentation.ts and the README no longer say the inline style holds the end.
…core/internal/touch Core ships the internal entry @astryxdesign/core/internal/touch, which re-exports the gesture primitives for Astryx's own packages, and the Lab-import guard fails a Lab import of core outside core's exports (architecture:react-component-runtime INV13). The README and the barrel's header named internal workspace imports and said nothing here is exported; they now name that entry.
…ill needs the element, and the listeners that cancel The release note, the stream's header, the host's comment and the README said the stream is one passive listener per event type and that a document-capture stopPropagation no longer starves the recognizers. Such a stop never hid a listener on the same node, and some subscriptions cancel. They now say what changed: an app's window-capture stopPropagation, or a document-capture stopImmediatePropagation registered before core, no longer hides a touch; a recognizer that must claim still needs the touch to reach its element; the gesture host's touchend, click and contextmenu cancel while a claimer is attached and the page hold's touchmove while it holds; and the stream stays ahead of an app's window-capture listener only until a type's listener is re-added. The README's list of blocking touchmove listeners gains the claimed touch's target and the document while a claiming element moves.
…default it moves
The {stylexOptions} form of astryxStylex now passes StyleX
treeshakeCompensation: true unless the caller sets it. spec:AST-017 FR3
asks the release note to name the old and new default and the option
that restores the old one; it now does.
…er and for the two stops the input stream fixes - A still finger with sub-pixel jitter over a scroll view keeps its tap and scrolls nothing (spec:AST-094 FR18–FR22). - A tap is still recognized under an app's window-capture stopPropagation, and under a document-capture stopImmediatePropagation registered before core's: the recognizers hear touches on the window's capture phase. Chromium and WebKit pass.
…s once Where nothing that claims is attached, the host's touchend listener is passive and cannot cancel the engine's own click after a retargeted tap clicks what is drawn. The shared click guard swallows that late click whatever is attached; this case pins it, as asked on meta#8293.
… first lift event, its pointerup or its touchend, whichever the gesture host releases on
…esture host's subscriptions The gesture host subscribes no click: the one cancelling click subscriber is the click guard's, added at a window's first arm and kept. The release note, the stream's header and the gestures README listed click with the host's touchend and contextmenu; they now name the guard's own.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: this pull request intentionally changes spec:AST-057 (a touch tap on an open swipe row's content closes it without activating the row, a scroll closes it while focus is outside the row, and a row still springing open counts as open), which extends FR7 and conflicts with AR3, so AST-057 needs updating to match what is decided. An engineering owner records that decision by commenting /approve-spec followed by the full head SHA on this pull request, and the author may do so as an engineering owner. Whatever the decision, these still need changes:
- Limit the scroll close's focus check to the open panel: as written, a row whose own button holds focus, as it does in Chrome after one of its entries fires, stays open through a scroll with Delete exposed.
- Rebase onto
mainonce the stack under #8097 lands, so this pull request is the 516-line dismissal alone; today the compare carries 32,905 authored lines, including the AST-057 FR2 and FR5 departures and an unrelatedastryxStylexdefault change.
[Automated review]
| (target instanceof Element && target.contains(root))) && | ||
| // Focus inside the row means the scroll may be the browser bringing | ||
| // the focused entry into view (keyboard or VoiceOver): the row stays. | ||
| !root.contains(document.activeElement) |
There was a problem hiding this comment.
This keeps the row open whenever focus is anywhere in it, not only on an entry. The row itself moves focus to its own button when an entry fires or it closes with focus inside (focusRowContent), and Chromium leaves a tapped button focused, so a reopened row then stays open through a scroll with Archive and Delete exposed. Checking that focus is inside the open panel (document.activeElement.closest('[data-swipe-panel]') != null) keeps the entry case this comment names.
|
Superseded by #8690, which contains this change; its description lists where each test and changeset line went. |
Depends on #8097. Review only this PR's change: vjeux/astryx@7fb1ae9...536a023
Why
On an open swipe-action row (
Item,ListItem), tapping the content fired the row's primary action, often navigating away, and scrolling left the row open with Delete exposed. UIKit closes the row in both cases.What led to it
Part of a series bringing Astryx's touch interactions to parity with native iOS (UIKit). An open row only watched for pointers landing outside it.
Options and tradeoffs
Owner decision proposed: amend
spec:AST-057FR7 so a resting row also closes on a tap on its content (activating nothing) and on a scroll of anything holding it.Not covered: a tap on another row closes this one but still reaches that row; UIKit swallows it.
Why this choice
It matches UIKit rows recorded on iOS 26.2 (the tap closes after the lift, unhighlighted; a scroll closes as it starts) and stops an accidental tap from navigating.
How to check it
On a phone, open the Item story "Swipe Actions":
We ran these with real touch in Chromium and on the iOS 26.2 simulator and the Android 15 emulator; each behaved as listed (recordings below). The unit tests and the Chromium and WebKit browser tests pass.
Evidence
Item's import grows from 42.0 to 42.9 KB gzip; the story keeps main's 237 elements. Known red until #8671 merges: its touch-bundle check, which a gesture-using
Itemfails by design.Scope