Repository navigation
Conversation
vjeux
requested review from
cixzhang,
imdreamrunner and
josephfarina
as code owners
October 24, 2026 07:31
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
github-actions
Bot
requested review from
cvkxx,
ernestt,
kentonquatman and
rubyycheung
October 24, 2026 07:32
This was referenced Oct 24, 2026
![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 owner decision before it can be approved. The reorder values and model rest only on draft spec:AST-094 FR1–FR3, which this branch adds without owner approval, and draft design:ordered-collection-reordering DR3 asks for surrounding rows to stay put until commit, while offsetsFor slides them during the drag. An engineering owner can settle it by commenting /approve-spec <full head SHA> on this pull request to approve FR1–FR3 for these values (the author can do this as an engineering owner), or by naming the record that should govern instead. Whatever that decision, these still need changes:
- Land or retarget the stacked predecessors (#8045, #8062, #8106) so this pull request carries only the reorder commit; today it carries 18,141 authored lines.
proposedSlotproposes a new slot at pickup for any row more than twice the height of the row below it (a 100 pt row over 44 pt rows proposes slot 2 at rest), so count only the boundaries crossed since pickup and add a tall-row test.- The
lintjob runs out of memory on this head but passes on the base.
[Automated review]
| } | ||
| const top = index < dragged ? rows[index].top : rows[index].top - shift; | ||
| const boundary = top + rows[index].height + gap / 2; | ||
| if (boundary < center) { |
There was a problem hiding this comment.
proposedSlot counts every boundary above the centre, so a row more than twice the height of the row below it starts past that row's boundary. For rows [44, 100, 44, 44], row 1 at its resting centre (94) returns slot 2, and offsetsFor slides row 2 up 100 pt before the finger moves. Counting only the boundaries crossed since pickup (start from dragged and step each time the centre passes the next boundary in the direction it moved) keeps every measured 44 pt case. A tall-row case in reorderModel.test.ts would pin it.
4 tasks done
Contributor
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsItem (@astryxdesign/core) · View in Storybook
TreeList (@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/CheckboxList | +7.7KB |
+22B |
| core/CheckboxListItem | +3.8KB |
+611B |
| core/ContextMenu | +12.1KB |
+626B |
| core/List | +7.9KB |
+40B |
| core/ListItem | +3.8KB |
+611B |
| core/Markdown | +12.0KB |
+620B |
| core/PowerSearch | +11.8KB |
+355B |
| core/Table | +12.0KB |
+618B |
| core/TableCell | +12.0KB |
+620B |
| core/TableHeaderCell | +12.1KB |
+611B |
| core/TreeList | +11.0KB |
+384B |
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 | 35.4KB | 36.0KB | +648B |
Accessibility Audit
Status: 2 accessibility violation(s) found — 2 serious.
Radio group pattern - 1 issue(s)
- 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
- Rule:
color-contrast· Affects 3/11 stories · Learn more - WCAG: 1.4.3 (Level AA)
- Rule:
RadioList - 1 issue(s)
- 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
- Rule:
color-contrast· Affects 3/14 stories · Learn more - WCAG: 1.4.3 (Level AA)
- Rule:
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
Contributor
Author
|
On the predecessor stack: this pull request is stacked, as its first line says, and only its last commit (a0df534, 7 files, +606 −2) is its own. It rebases onto main as each predecessor lands, and the diff then shrinks to that commit. On |
Contributor
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
Contributor
Author
|
Agreed: one implementation.
This pull request's own commit (the reorder values and model) reads no pointer, writing direction or motion preference. The List reorder that uses it keys its motion on the resolver's touch presentation ( |
vjeux
force-pushed
the
uikit/c13-reorder-values
branch
from
October 25, 2026 01:16
a0df534 to
bf440cf
Compare
Contributor
Author
|
Pushed: bf440cf, on #8106 at 4d54dbc, which carries all three.
|
vjeux
force-pushed
the
uikit/c13-reorder-values
branch
from
October 25, 2026 01:46
bf440cf to
21aab26
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: the reorder values and model rest only on draft spec:AST-094 FR1–FR3, which this branch adds without owner approval, while draft design:ordered-collection-reordering DR3 asks for surrounding rows to stay put until commit and AST-094 FR4 says touch motion is not themable, which the new reorder.slide.spring role contradicts. An engineering owner can settle it by commenting /approve-spec <full head SHA> on this pull request to approve AST-094 for these values (the author can do this as an engineering owner), or by naming the record that should govern them instead; that approval reads the record against the ## Writing rubric results the description does not yet carry. Whatever that decision, these still need changes:
- Land or retarget the stacked predecessors so this pull request carries only the reorder commit; it now carries 39,053 authored lines.
- The carried
platformadaptation condition inthemeAdaptations.tscontradicts the closedwhenvocabulary ofspec:AST-012FR2. uikit.reordernow reaches@astryxdesign/core/motion/experimentalwithout@experimentalor an[experimental]changeset; drop the barrel re-export or declare it.
[Automated review]
| * is everything else. Pair `ios` with `pointer: 'coarse'` for a touch | ||
| * profile, such as touch motion tuned per platform. | ||
| */ | ||
| platform?: 'ios' | 'other'; |
There was a problem hiding this comment.
platform widens the adaptation when vocabulary that spec:AST-012 FR2 closes: "A when object may contain width, height, pointer, contrast, and motion". Drop it here, or have an engineering owner amend FR2 before it ships.
| } from './gestures'; | ||
| export {list} from './list'; | ||
| export {navigation} from './navigation'; | ||
| export {reorder} from './reorder'; |
There was a problem hiding this comment.
This re-export puts reorder on the experimental @astryxdesign/core/motion/experimental subpath as uikit.reorder (motion/experimental.ts:71), but reorder carries no @experimental and .changeset/reorder-values.md is a [chore] that says it is not exported (spec:AST-017 FR21–FR22). registerTables.ts and reorderModel.ts import ./reorder by path, so dropping this line keeps the table internal; otherwise tag it @experimental and make the changeset [experimental].
Comment on lines
+14
to
+20
| * UIKit's rules (iOS 26.2, measured): the proposed slot changes when the | ||
| * dragged row's centre crosses a row boundary of the layout WITHOUT the | ||
| * dragged row, with no hysteresis; the rows between the dragged row's place | ||
| * and its slot each move by the dragged row's height; the drop goes to the | ||
| * current slot, from rest, on a spring whose duration scales with the | ||
| * remaining distance; autoscroll moves a whole number of points per tick, | ||
| * more the deeper the dragged row's edge is past the scroll view's edge. |
There was a problem hiding this comment.
This still states the measured rule as a crossing of "the layout WITHOUT the dragged row". proposedSlot (lines 102–108) follows that only while the row below is at least as tall, or the row above at most as tall, as the dragged row; otherwise it clamps to the neighbour's near edge, and no measurement covers those cases (a 44 pt row under an 88 pt row now changes places after 66 pt, where this rule gives 22 pt). Stating the implemented rule here and marking the clamp cases unmeasured would keep the header honest.
Comment on lines
+171
to
+182
| '::after': { | ||
| content: { | ||
| default: null, | ||
| '[data-astryx-travelling]:not([aria-selected="true"],[aria-current="true"])': | ||
| '""', | ||
| }, | ||
| position: 'absolute', | ||
| inset: 0, | ||
| zIndex: -2, | ||
| borderRadius: 'inherit', | ||
| pointerEvents: 'none', | ||
| backgroundColor: colorVars['--color-background-surface'], |
There was a problem hiding this comment.
These ::after properties apply to every row, and the caller's xstyle is merged after them, so an ::after decoration passed through xstyle inherits position: absolute, inset: 0, z-index: -2 and the surface background unless it sets each one. .changeset/list-row-press-compositor.md tells callers to move row decoration to ::after. Scoping these properties to [data-astryx-travelling], or pointing the changeset at a child element for ListItem, avoids the trap.
vjeux
force-pushed
the
uikit/c13-reorder-values
branch
from
October 25, 2026 02:40
21aab26 to
96480fc
Compare
…lowance under every primary pointer The useMenuPress record's FR9 governs the trigger's held finger by spec:AST-094 FR29 under every primary pointer (FR43). The trigger now hands the long press FR29's 13 px drift allowance whatever the primary pointer, so it no longer reads (pointer: coarse) as its press starts, and the menus drop their coarsePointer import. ContextMenu keeps the long press's own allowable movement. The changeset says the same.
From review on meta#8106: component:List FR12's verification lists layout or paint per frame as a failure for an insert, a removal and a reorder, and the trace covered the insert only. It now runs for all three; each passes in Chromium.
…wever late a busy first frame started it The touch press controller removed `fading` on a clock started at the lift. A busy main thread delays the release animation's first frame, and with it its end, so the clock cut the release short mid-fade. The controller now checks at that clock for the surface's own finite release animation (on it, or its `::before` or `::after`) and, while one runs or still waits for its first frame, ends the release when it finishes or is cancelled; with none, the release ends at the clock. Timers become cancel functions, so a release that waits on an animation is cancelled the same way. The clock is the table's, capped at 200 ms under reduced motion, and the overlay's release animation is capped the same way (design:reduced-motion DR2: a touch transition finishes in 200 ms or less); the stylesheet tests read the capped durations. This reaches every surface the touch press controller drives (buttons, switches, checkboxes, cards, menu rows, list rows). Split out of meta#8077 on review; meta#8077 builds its compositor row release on it. Red first: with the previous controller, three of the seven new cases fail (a release whose first frame came late, one still waiting when the clock ran out, another painter's release); the other four guard what stays (a cancelled release ends, no animation ends at the clock, other animations are ignored, a new press owns its paint).
…e row press release change # Conflicts: # packages/core/src/__tests__/pressState.ts # packages/core/src/utils/interactionOverlay.stylex.ts # packages/core/src/utils/pressFeedback.test.ts # packages/core/src/utils/pressFeedback.ts # scripts/build-css.test.mjs
…d the changeset says what a caller's ::before does From review on meta#8077: - The headers of motion/uikit/list.ts, both ListRowPress browser specs and interactionOverlay.cell.test.ts, and the notes in build-css.test.mjs and __tests__/pressState.ts, pointed the row press layer at utils/interactionOverlay.stylex.ts; it lives in utils/rowPressLayer.ts (rowPressLayerStyles.cell), its clocks in rowPressLayer.stylex.ts (cellPressConsts). - Notes that described the release as ending on a timer now say the controller checks at the release clock and the release ends with its animation; the stylesheet tests no longer parse a theme motion role the release stopped playing. - The changeset said a caller's ::before decoration takes the layer's paint during a press. Under a finger any ::before property a caller sets wins over the layer's, so a decoration with its own size and background jumps to the row's corner and the row shows no pressed fill until the release ends; it now says so and keeps the advice to decorate with ::after or a child.
…ontroller change split out) into the touch row target change # Conflicts: # .changeset/list-row-press-compositor.md
…le uses From review on meta#8085: the exemption let a file that paints a press pass the guard whenever it rendered a marked component through `xstyle`. It came with ListItem's divider arms, which the revert of meta#8079 took out, so no file at this head reaches it, and in place it would wave through a file whose press styles never reach that component's root. The guard is again exactly as on the line below; it passes without the exemption.
…never needed From review on meta#8077: the build-css press-paint check filtered out a `--_press-paint: transparent` declaration, with a note, that the row layer never writes (only the overlay declares the press paint), and interactionOverlay.cell.test.ts defined a `hostRules` helper no case uses. Both go; the stylesheet check is as on the line below again.
…ers a row with an end control From review on meta#8085: - `isTouchClick` and its comment sat between `Item`'s docblock and `export function Item`, so the docblock attached to the helper and `Item` lost its description in editors and Storybook. The helper now sits above the docblock. - component:List FR10's verification names a row with an end control. A new List story puts a compact row whose end content is a switch between two interactive rows, and the browser spec checks that the interactive rows grow only to half the gap to the switch, that the browser hit-tests the switch at its center, and that a real tap toggles it with no tap reaching the rows beside it. With the hit area's neighbour clamp removed, the case fails (a band of 5.5 px where half the gap is 2 px).
…meta#8890 and meta#8360's current head) into the tree expansion change # Conflicts: # packages/core/src/motion/uikit/list.ts
…ast its divider From review on meta#8085, two ways a tap in an interactive row's band missed the row: - The band is the row's ::after, and the rows after it are positioned with no z-index, so a later row that takes no taps (a static row, such as the switch row in the "With end controls" story) painted and hit-tested over the band. The layer now has z-index: 1 on itself; its even-odd clip still leaves the row's own box to the row's content, and the row still makes no stacking context. - The layer is offset from the row's padding box, while the insets are measured from its border box, so below a divided row the band was the divider's width short. A host that draws a border now declares it (--_hit-border-{top,right,bottom,left}), and a divided ListItem declares its divider's, so the band starts at the divider's outer edge. Above everything at z-index auto, the band still takes no tap from a control near the list: the hit area's neighbour clamp measures every interactive element on the page, not only rows. The "With end controls" story gains a button in its header and one 4 px after the list, and a browser case hit-tests both; with the clamp removed it fails (a 6 px band where half the gap is 2 px). Red first: the browser spec now reads the band past the border box and hit-tests points inside Network's band. On the previous CSS, the end-control case read a 1 px band where half the gap is 2 px and a compact divided row's band read -1 px; with z-index removed, a point 0.5 px below Network hit the Wi-Fi row.
…nd the list bounds what rises From review on meta#8085 and meta#8106: - A ListItem with swipeActions is a stacking context at rest (Item isolates and translates its swipe root), so its touch band's z-index ordered only inside it and a following static row still took taps in the band. Under a finger, a swipeable interactive row now raises itself (z-index: 1 on the row; with a mouse nothing changes). The "With end controls" story's Network row is now swipeable, and the browser case checks that the row isolates and that taps in its band are its own. - The list's ul now isolates (isolation: isolate), so what rises inside it (a row's band, a swipeable row, and later a crossing row) orders above the rows after it but stays below a page-level sticky header at the same z-index (spec:AST-027 DEC-1, DEC-2). Red first: with Network swipeable, a point 0.5 px below it hit the Wi-Fi row; the unit cases for the list's isolation and the swipeable row's raise failed on the previous CSS.
From review on meta#8085: ListItem raised every interactive row with a swipeActions prop, while Item makes a row a stacking context (serves its swipe actions) only when a side has an entry, and not for a disabled row, a link root, a role that forbids interactive descendants, or a row whose press goes to a control inside it (interactiveRef). Rows with empty swipe sides or a delegate control were raised for nothing. Item's condition is now one function, servesSwipeActions, which Item and ListItem both use. Red first: in List.test.tsx, a row with empty swipe sides and a row with a delegate control both carried the raise on the change before this one.
From review on meta#8106: a swipeable interactive row's raise (meta#8085) and the travelling raise both set the row's zIndex, and StyleX keeps one per element. The swipe raise came later, so under a finger every swipeable row sat at 1, the moved row included, and the moved row (first in the document) slid beneath the rows it crossed; with a mouse the swipe raise's default dropped the travelling raise altogether. The swipe raise now carries the travelling raise above its own (2), with a mouse as under a finger. The swipe story gains a "Move the last row first" button, and a browser case freezes that reorder halfway and hit-tests where the moved row overlaps a row it crosses, under a finger and with a mouse. On the change before this one it hit Inbox; it now hits Archive.
…e rows it crosses From review on meta#8106: the travelling raise was z-index 1, the same as a swipeable row's raise under a finger, so a moved row without swipe actions (first in the document) slid beneath the swipeable rows it crossed. The travelling raise is now 2, above the swipe raise. The swipe story gains a row without swipe actions (Notes) and two move buttons, "Move Archive first" (a swipeable row) and "Move Notes first". A new browser case freezes Notes' move halfway and hit-tests where it overlaps a row it crosses, under a finger and with a mouse: on the change before this one it hit Inbox under a finger; it now hits Notes. The swipeable-row case moves Archive with its own button.
ListItem declares --_hit-border-bottom (the divider's width, 0 on the last row) for the touch hit layer, and List.doc.mjs did not list it, so derivedVarRegistry.test.ts failed: "List has undocumented CSS vars in source: --_hit-border-bottom". It is now in List.doc.mjs theming.vars[] as a private var, and in VARS_WITHOUT_DERIVED_MAPPING with its reason: it is the ::after layer's geometry, which follows --border-width, not a property a theme author sets (as with --_thumbnail-hit-inset and --_input-clear-hit-inset). Red first: "List: all source vars are in doc file" failed on the change before this one; it passes now (122 of 122).
…head Main's merges of meta#8045 and meta#8046 (the frame-evidence harness and the coarse press) conflict with the older foundation heads this branch carries, in the interaction harness files. meta#8106's current head carries that main and the foundation it now rests on, so taking it leaves this branch clean against main with nothing resolved by hand. A clean merge.
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: proposedSlot keeps UIKit's crossing rule, which departs from spec:AST-103 FR1 (a drag reports the slot whose centre is nearest the dragged item's centre) wherever neighbouring rows differ in height, so past the 88 pt row of the measured table it moves to slot 6 at a centre of 308 pt where FR1 does at 286 pt, and the measured 306.8 pt sample lands on different slots. An engineering owner settles it by updating FR1 so a list without positions() follows UIKit's rule and recording that by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or by keeping FR1, in which case proposedSlot and its replay test conform to it. Whatever is decided, the diff still carries #8106's current head, 35,537 authored lines against this pull request's own 510, so land #8106 first and this pull request shrinks to the seven reorder files.
[Automated review]
vjeux
marked this pull request as draft
October 29, 2026 22:31
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #8106, and carries its current head. Review only this PR's change: vjeux/astryx@781c381...26a8bca (only merges since: main, then #8106's current head).
Why
A reorderable list on a phone should feel like the iOS one people know: the same pickup delay, autoscroll speed and drop rules. Those numbers must come from measuring iOS, not guessing.
What led to it
This is part of the UIKit-parity program, which makes Astryx's components behave like their iOS counterparts on touch screens. The List reorder (#8139) needs its values and rules first. This PR adds them, unused until then:
motion/uikit/reorder.tsholds values measured on iOS 26.2. A still finger picks a row up after 150 ms, so a flick scrolls instead. Autoscroll moves 2 to 10 points every 20 ms, depending on how deep the finger is into the edge.List/reorderModel.tsholds the rules: where a dragged row lands, which rows it may pass (only pinned rows at the ends stop it), and keeping it inside the list.MEASUREMENTS.mdlists the three values that are still guesses.Options and tradeoffs
spec:AST-103FR1's slot whose centre is nearest the dragged centre. They agree for rows of one height and differ past a row of another (past the 88 pt row: 308 pt against 275 to 286 pt).Why this choice
iOS 26.2 shows no lift, so neither does this.
design:ordered-collection-reorderingchose still rows, so UIKit's measured slide stays in the file header only. The 8 pt inset ships as a guess, marked inMEASUREMENTS.md, rather than holding the reorder back.Owner decision: the model follows UIKit's measured landing rule, which the replay test pins. I propose that FR1's nearest-centre rule cover
positions()and attached containers, with a list withoutpositions()following UIKit's rule. The alternative is conforming the model to FR1 for lists too, which gives up iOS's measured landing points past rows of different heights.How to check it
Nothing is visible yet; #8139 uses these values. I ran
reorderModel.test.ts, and it passes: it replays a drag measured on iOS, a 44 pt row past an 88 pt row and back, and all 8 landing positions match. It also checks that a row at rest never moves and that a pinned row in the middle doesn't stop a drag; that one failed before its fix.Evidence
The model's tests.
Stack (merge in this order): the motion foundation and the list stack through #8106 → this → the List reorder.