Repository navigation
Conversation
vjeux
requested review from
cixzhang,
imdreamrunner and
josephfarina
as code owners
October 25, 2026 07:57
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
needs:design-review
github-actions
Bot
added
the
needs:spec-owner-review
github-actions
Bot
requested review from
ernestt,
kentonquatman and
rubyycheung
October 25, 2026 07:58
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on a spec decision: spec:AST-094, whose FR29 handoff and FR26–FR30 govern this change, is added here through the unmerged #8378 and is not on main, so an engineering owner needs to record the decision by commenting /approve-spec 8a2a297101a5dd8485f3cd33f08528a575b95903 on this pull request (the author may do so as an engineering owner) or land #8378 first. Independently of that decision, these changes are needed:
- In
resolveHandoff(packages/core/src/hooks/useMenuPress.ts, lines 788–792),movingis read from the previous read's surfaces, so one move during the open spring keeps mid-grow rects for the rest of the slide; decideisStaleaftermeasureHandoff, and add tests for that case and for the autoscroll re-read. - The diff against
maincarries the whole unmerged stack (about 42,600 authored lines, 351 of them this commit), so please keep this pull request in draft until #8195 lands and then rebase onto it.
[Automated review]
Comment on lines
+788
to
+792
| const moving = state.roots.some(root => isMoving(root.surface)); | ||
| if (state.isStale || moving) { | ||
| measureHandoff(state, x, y); | ||
| // Read while it moves: read once more when it rests. | ||
| state.isStale = moving; |
There was a problem hiding this comment.
moving is computed from state.roots before the read, and on the first call that list is still empty, so a first read taken mid-grow stores isStale = false. If the finger then rests until the open animation retires, every later move hit-tests the mid-grow rects (line 794) and isInMenu uses the mid-grow box (line 796), so the wrong row, or none, lights and acts. Deciding after the read fixes it:
measureHandoff(state, x, y);
state.isStale = state.roots.some(root => isMoving(root.surface));A case for it: one move while getAnimations() reports a running animation, then [], rows re-placed, one more move, expecting a second read.
Contributor
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsAlertDialog (@astryxdesign/core) · View in Storybook
ContextMenu (@astryxdesign/core) · View in Storybook
MoreMenu (@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/AlertDialog | +26.0KB |
+64B |
| core/BreadcrumbItem | +15.5KB |
0B |
| core/ContextMenu | +15.2KB |
0B |
| core/DropdownMenu | +29.7KB |
0B |
| core/DropdownMenuSubMenu | +15.1KB |
0B |
| core/Markdown | +15.2KB |
0B |
| core/MoreMenu | +29.7KB |
0B |
| core/Pagination | +14.7KB |
0B |
| core/PowerSearch | +14.8KB |
0B |
| core/Selector | +14.9KB |
0B |
| core/Table | +15.2KB |
0B |
| core/TableCell | +15.2KB |
0B |
| core/TableHeaderCell | +15.2KB |
0B |
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.3KB | 34.3KB | +43B |
Accessibility Audit
Status: No accessibility violations detected.
Generated by PR Enrichment workflow | Storybook | Sandbox | View full report
meta#8089 now carries meta#8078's restructured press model: the trigger half lives in menuTriggerPress.ts, a menu with no trigger mounts useMenuSurfacePress, and the root passes its touch model to its sub-menus on the internal surface context. meta#8110 carries the shared tracker on the current gesture line. This pull request's finger tracking moves onto that shape: the shared model attaches the selection recognizer under 'uikit', a sub-menu takes the root's touch model from the surface context (its own touchModes field goes), and the module record keeps main's table, which no longer lists the touch model. Two coarse cases from the base now drive the finger through the gesture system, as a real finger is: the AR5 pick, and the cancelled touch that clears its lit row (the hook's and a flyout's).
… main) into UIControl press tracking
…hing is attached From CodeBunny on meta#8073 (spec:AST-094 FR24), fixed once here: the (pointer: coarse) change listener was added when the host was created, on a fine pointer too, and never removed. It is now added with the first attachment and removed when the last one goes, so nothing touch-related remains after the last instance unmounts. The test fails on the previous head (a listener at creation, none removed).
- The detach's landing commit can promote the element again (a coast still moving on another axis), so attach demotes after it, as dispose() orders them: no will-change stays on an unmounted or replaced node. - A new element taking over an exit by role hands the outgoing element its stylesheet's opacity, as for a borrowed dip (the same test stop() uses).
Lab imports no new core-private path until Lab's module-sharing decision, so the hooks/isRtlElement entry of the internal-module resolver, its two Lab tsconfig paths and its resolver tests lose their only users and go. A Lab component that needs the check keeps a local helper.
…nd a finger's tap acts once Where the primary pointer is fine (spec:AST-094 FR19, FR24), the open menu attaches nothing to the gesture system and adds no touch listener, and a finger's tap acts at its release through the press model with the browser's click that follows swallowed, so the row acts once. Where it is coarse, the menu joins the gesture system while it shows. The case fails if the touch model's gate on the selection recognizer is removed.
meta#8195 now carries meta#8078's restructured press model and main's handoff cases (module FR11). Two of those cases hand a finger to a 'uikit' menu in the hook's harness, where this pull request resolves a handed-off finger's rows from their measured rects; like this pull request's own handoff cases, they inject the row under the finger, so they still check the machine's handoff distance (the rect tracker is the DropdownMenu suite's).
…t syncs, and the stale headers and changesets match the code - useGestureRecognizer and usePressTracker re-attach when the element behind the ref is replaced and detach when it goes, checked after every render at no render cost (architecture:react-component-runtime INV6); each Effect names the system it syncs (INV3). - A control's profile releases on the deselect clock the shared press styles play, so a painter's `fading` never outlasts the fade it shows; UIButton's 250 ms arrives with a control's own release layer. - PressTracker's and GestureHost's headers describe the cell's delay inside scroll views only, and no delegated consumers or scroll handoff. - One changeset for the press tracking, the one press writer and the hooks. Co-authored-by: Christopher Chedeau <vjeux@users.noreply.github.com>
src/utils backs the public @astryxdesign/core/utils entry, where a deliberately experimental API may carry the tag, so the guard reads only its package-private touchPresentation.ts there. A test pins that boundary, and the guard checks that every file it names exists.
…e, and a backward end delay is not progress waitForAnimation's fallback finish time (read before the finish event is delivered, right after finish()) now goes through the playback rate's speed, and for a run going backward is its startTime; it was a whole run late for a reverse()d run and 500 ms late at rate 2. minProgressMs no longer counts a backward run's end delay, which plays first and presents nothing. Self-tests for both; the finish case failed by 1,000 ms before (Chromium, 27/27 and 5x repeat pass).
…ing is attached) into UIControl press tracking The host has no delegated consumers at this layer, so the listener follows the registry alone.
# Conflicts: # internal/interaction-harness/README.md # internal/interaction-harness/package.json # internal/interaction-harness/src/harness.interaction.browser.spec.ts # internal/interaction-harness/src/story.ts
…ded) as a dependency
spec:AST-094 FR28: when the window loses focus the host cancels every touch, as UIKit does, and forgets the fingers. A browser that keeps such a touch alive past the blur then delivers its lift untracked, so the tap's click runs whatever is under it. The unit case and the browser proof read that the lift is refused its click, and that only that lift is: a later finger with the same identifier, lifted or not, keeps its own. Fails on this head (the lift clicks).
When the app loses focus the host still cancels every touch in flight, as UIKit does, and now remembers each finger it dropped until the browser ends it. A touchend for one is cancelled and arms the click swallower, so a browser that keeps the touch alive past the blur cannot click whatever is under the lift (spec:AST-094 FR28: nothing acts). A touchcancel, a new touch with the identifier, or a touch list without the finger forgets the record.
…ewest call's frame has passed spec:AST-094 FR30: a menu's edge autoscroll announces each write with ownScroll, every frame. The first call's two-frame timer cleared the exemption the newest call had just set, so the newest write's scroll read as the browser taking the finger and the press was lost. Fails on this head.
…west call's frame has passed ownScroll numbers each call and keeps the scroller's newest number; a call's two-frame timer clears the exemption only if no newer call has come since, so an edge autoscroll that announces a write every frame keeps its finger (spec:AST-094 FR30).
…the host listening spec:AST-094 FR24: nothing disabled registers anything. Detaching an element's enabled recognizer, or its consumer, while a disabled recognizer stays attached left the host's document listeners installed, because the clean-up re-synced them only when the element had nothing left. Fails on this head.
…he host listening The clean-up after a detach now re-syncs the host's listeners whenever the element keeps something attached, not only when it is gone, so a disabled recognizer left behind registers nothing (spec:AST-094 FR24).
…so it stays on a short compact screen placeOverTrigger capped only the popover surface, while the role="menu" element inside kept its own taller cap and the surface's overflow showed it: at 800 x 220 a menu ran 64 px past the viewport and its last rows never came on screen. The room now caps every capped box from the surface down to the menu (and the column it shares with a header or footer), each under its own cap, so the scrolling element is the one held to the room.
…reen) as a dependency
There was a problem hiding this comment.
Approval waits on a spec decision: this head still carries architecture:layer-runtime INV13 and its "Exit copy" section from the stacked #8186, which main does not state, so an engineering owner needs to record the decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or INV13 can land on main through #8186 first. Independently of that decision, these changes are needed:
- In
resolveHandoff(packages/core/src/hooks/useMenuPress.ts, lines 824–828),movingis still read from the previous read's roots, so one move during the open spring leaves the opening finger on mid-grow rects; decideisStaleaftermeasureHandoffand add a test for that case. - The opening finger's autoscroll re-read (lines 675–680) still has no test; a case where that finger rests in the bottom edge zone and the lit row follows the scrolled rows would cover
spec:AST-094FR30. - The diff against
mainstill carries the unmerged stack (34,526 authored lines, 347 of them this pull request's own), so please keep this pull request in draft until #8195 lands and then rebase onto it.
[Automated review]
… a caller's inline cap back
placeOverTrigger removed every inline max-block-size on the menu chain at
each open, so a caller's style={{maxBlockSize}} on a desktop menu was lost at
the first open. It now keeps each box's inline value from before it capped
it, restores that when the cover clears, and leaves an uncapped menu alone.
…e it FR21 (the menu grows out of its trigger and closes on a pan that starts outside it) comes from the menu-motion pull request below this one, so this pull request no longer removes it: its rows match that pull request's text again, and the record's fate is decided there.
…n, on the gesture host's fix (spec:AST-094 FR28) The FR28 blur case waited (test.fixme) for the gesture host to keep a finger a blur cancels until the browser ends it and to swallow its lift's click; meta#8309 1be6b6c does, so the case runs: nothing acts, the lit row clears, and the lift acts on nothing.
…, not the menu's bottom padding (spec:AST-094 FR30) The finger rested 4 px above the menu's bottom edge, inside its 4 px bottom padding: once the list had scrolled to its end no row was under it at the lift, so nothing was picked though the press survived the scroll (the highlight followed it onto Share, then Archive). It rests 12 px up now, still in the edge zone and over the last row.
There was a problem hiding this comment.
Approval waits on a spec decision: this head carries architecture:layer-runtime INV13 and its "Exit copy" section (from #8186) and component:DropdownMenu FR21 (from #8080), which main does not state, so an engineering owner needs to record the decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), or both can land on main through their own pull requests first. Independently of that decision, these changes are needed:
- In
resolveHandoff(packages/core/src/hooks/useMenuPress.ts, lines 828–832),movingis still read from the previous read's roots, so one move during the open spring leaves the opening finger on mid-grow rects; decideisStaleaftermeasureHandoffand add a test for that case. - The opening finger's autoscroll re-read (lines 679–684) still has no test; a case where that finger rests in the bottom edge zone and the lit row follows the scrolled rows would cover
spec:AST-094FR30. - The diff against
mainstill carries the unmerged stack (35,134 authored lines, 347 of them this pull request's own), so please keep this pull request in draft until #8195 lands and then rebase onto it.
[Automated review]
Contributor
|
Posted by Cindy's agent on her behalf: closed and reopened only to restart this PR's CI on the current workflow after tonight's queue-wide cancel; nothing in the PR changed. |
vjeux
marked this pull request as draft
October 29, 2026 22:36
8 tasks done
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 #8195. Review only this PR's change: vjeux/astryx@b30de7f...d5ec70f
Maintainer impact
Under DropdownMenu's touch modes, a finger that lands in the open menu resolves its rows through the shared action-group tracker (#8195, #8346). The finger that opened the menu from its trigger (
spec:AST-094FR29) did not. It hit-tested the page withelementFromPointon every move. Two hit-testing paths for one menu meant two answers near a row's edge, and a hit test on every move.Change
presentation="popover") and an injectedhitTestkeepelementFromPoint.Evidence
A press on "Move to project", a slide into the menu to its bottom edge, a rest there while it autoscrolls, the lift on "Website refresh" (it runs and the menu closes). Every row lights in turn on both.
Public delta
None. The changeset is internal.
Writing rubric results
No knowledge record changes.
Testing
pnpm vitest runhooks, DropdownMenu, ContextMenu, MoreMenu, Selector, MultiSelector, motion, gestures and utils at this head: 2874 passed. They include "the finger a trigger hands to the menu": across a slide, the rects are read once andelementFromPointis never called; they are read again while the surface moves; a row scrolled out of the menu's box is not under the finger. The first two fail on DropdownMenu: a finger in the open menu is tracked as UIMenu's list tracks it #8195's head.pnpm test:interactionDropdownMenuTouch* and DropdownMenuSelection at this head: Chromium 31/31, WebKit (Playwright image) 26 + 5 skipped. The DropdownMenu, MoreMenu, pinch-zoom and menu-press a11y specs (Chromium) 156/156.pnpm build,pnpm typecheckandCI=true pnpm lintwith no error; CLI isolation and golden reads 47/47;check:knowledge,check:changesets.Stack (merge in this order): #8076 and #8298 → #8078 → #8508 and #8186 → #8080; #8446 → #8089; #8083 → #8084 and #8346 → #8110 → #8195; then this PR.