Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewNew ComponentsRefreshControl (@astryxdesign/lab) · 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. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
f696241 to
2adb6f3
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval of this pull request waits on an engineering owner's spec decision: the draft spec:AST-094 requirements (FR1–FR23, DEC-1, DEC-2), RefreshControl's UIKit-parity behaviour in Lab, and the two new experimental core subpaths (@astryxdesign/core/motion/experimental, @astryxdesign/core/gestures/experimental, which spec:AST-017 FR21 does not cover) have no current spec authority. An engineering owner records that decision by commenting /approve-spec 2adb6f3b1f5994902204dfdcb8727820dffea32c on this pull request, and the author may do so if they are an engineering owner. Whatever the decision, please land the stacked dependencies first so this diff carries only the refresh-control commits, move the adaptive-presentation breakpoint change (744/500 px instead of 768 px) into its own pull request, and fix the three refresh-control defects:
- A controlled
isRefreshingthat staysfalsenever ends a pulled refresh (useRefreshControl.ts:455-460,:724-731). - Every pull leaves an inline
translateon the content, soposition: fixedchildren stop being fixed (useRefreshControl.ts:276-278,:530). - The pulled list gets a blocking
touchmovelistener although the pull never claims a touch (GestureHost.ts:148-153).
[Automated review]
| const callHandler = useCallback(() => { | ||
| const token = ++refreshToken.current; | ||
| const result = latest.current.onRefresh?.(); | ||
| if (latest.current.isControlled) { |
There was a problem hiding this comment.
In controlled mode a triggered pull returns here and can end only when isRefreshing changes (the Effect at line 724). A parent that keeps false, or sets true then false in one handler, leaves the list refreshing behind the 60 px inset with aria-busy="true". Render controlled state from the prop, or end on the first render after onRefresh in which the prop is still false.
| } | ||
| }, []); | ||
| const demote = useCallback(() => { | ||
| contentElement.current?.style.removeProperty('will-change'); |
There was a problem hiding this comment.
Only will-change is removed at rest; the content keeps the translate: 0px written by animator.set({y: 0}), which makes it a containing block for position: fixed children for the rest of the session (a fixed bar inside the page story moves from 0 to −284 px after one pull). Remove the inline translate here as well.
| }; | ||
| }, [applyEdge, isPullEnabled, releaseEdge, resolveOwner]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
This Effect sets state with no comment naming what it synchronizes (architecture:react-component-runtime INV1/INV3), and a lock already held at mount first renders the pull enabled. useSyncExternalStore(subscribeScrollLock, isScrollLockActive, () => false) reads it during render.
| function claimsWhileMoving(recognizer: GestureRecognizer): boolean { | ||
| return ( | ||
| recognizer.acceptsTouchInput && | ||
| !(recognizer instanceof TapGestureRecognizer) | ||
| ); | ||
| } |
There was a problem hiding this comment.
This ignores claimsTouches, so RefreshPullRecognizer (which never claims) still gives the whole refreshable list a {passive: false} touchmove listener; the README's Known limits say scrolls that start on such an element wait for the current task in Mobile Safari. Requiring recognizer.claimsTouches here removes it.
2adb6f3 to
eaffb19
Compare
left a comment
There was a problem hiding this comment.
Approval of this pull request waits on an engineering owner's spec decision: the iOS pull-to-refresh direction for the Lab RefreshControl, the draft spec:AST-094 requirements (whose FR4 and DEC-1 keep touch motion off the theme while this branch makes it themable), and the two new experimental core subpaths, none of which has current spec authority. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Whatever the decision, these still need changes:
- Land the stacked dependencies first so this diff carries only the refresh-control commit; it is over 33,000 authored lines today.
- Move the theme motion roles and the
platformadaptation condition (which contradictspec:AST-012FR2 andarchitecture:theme-tokensINV1) and the 744/500 px adaptive breakpoint into their own pull requests. - Make a controlled
isRefreshingthat staysfalseend a pulled refresh, apply the pull'soverscroll-behavior-y: noneonly while the scroll owner overflows (spec:AST-025FR14), give non-claiming recognizers no blockingtouchmovelistener, and replace the two state-setting Effects inuseRefreshControl.ts. - Add the
## Writing rubric resultssection that spec approval reads.
[Automated review]
| const callHandler = useCallback(() => { | ||
| const token = ++refreshToken.current; | ||
| const result = latest.current.onRefresh?.(); | ||
| if (latest.current.isControlled) { |
There was a problem hiding this comment.
In controlled mode a triggered pull returns here, and only the Effect at line 750 can end it, which runs only when isRefreshing changes. A parent that keeps false, or sets true then false in one handler, leaves the list refreshing behind the 60 px inset with aria-busy="true". Render controlled state from the prop, or end on the first render after onRefresh in which the prop is still false.
| }; | ||
| }, [applyEdge, isPullEnabled, releaseEdge, resolveOwner]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
This Effect sets state with no comment naming what it synchronizes (architecture:react-component-runtime INV1/INV3), and a lock already held at mount first renders the pull enabled. useSyncExternalStore(subscribeScrollLock, isScrollLockActive, () => false) reads it during render.
| const near = | ||
| isPullEnabledRef.current && | ||
| scrollTopOf(current) < Math.max(heightOf(current), REFRESH_HEIGHT); |
There was a problem hiding this comment.
near never checks that the owner overflows, so a short list keeps overscroll-behavior-y: none on an area that does not scroll; spec:AST-025 FR14 allows a containment policy only on axes that currently overflow.
There was a problem hiding this comment.
Kept on purpose. This is the refresh pull's own containment, not ScrollableArea's, and it follows UIKit: a UIScrollView with a UIRefreshControl always bounces vertically (alwaysBounceVertical), so a list shorter than its scroll view still pulls and refreshes. That is one of this stack's interaction proofs ("a list shorter than the screen still pulls and refreshes").
Without the claim, a pull on a scroll owner that doesn't overflow chains to the page: the page rubber-bands under the pull, and on iOS Safari the page's own reload can fire. The claim is held only near the top, only while a control is mounted and the pull is enabled (iOS only), and it is dropped while a scroll lock holds the page.
If AST-025 FR14 should govern the refresh pull too, that needs an owner decision on the spec, and pulls on short lists would stop working.
| recognizer.acceptsTouchInput && | ||
| !(recognizer instanceof TapGestureRecognizer) | ||
| ); | ||
| } |
There was a problem hiding this comment.
This ignores claimsTouches, so RefreshPullRecognizer (which never claims) still gives the whole refreshable list a {passive: false} touchmove listener; the note at the top of this file says a scroll that begins on such an element waits for a busy main thread in iOS Safari. Requiring recognizer.claimsTouches here removes it.
| * 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 is a fifth condition field; spec:AST-012 FR2 keeps the when vocabulary closed to width, pointer, contrast and motion until that record adds one, so this needs a pull request that amends FR2.
| } | ||
| const variable = motionRoleVariable(id); | ||
| try { | ||
| tokens[variable] = formatMotionRoleValue(kind, value); |
There was a problem hiding this comment.
These --motion-* names come from table paths and are never declared in tokens.stylex.ts, which architecture:theme-tokens INV1 names as the origin of every core token name; the draft spec:AST-094 FR4 in this same pull request also says touch motion is not themable.
eaffb19 to
4e4c16b
Compare
left a comment
There was a problem hiding this comment.
Approval of this pull request waits on an engineering owner's spec decision: the iOS pull-to-refresh direction for the Lab RefreshControl, the draft spec:AST-094 requirements (whose FR4 and DEC-1 keep touch motion off the theme while this branch makes it themable), and the two new experimental core subpaths have no current spec authority. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Whatever the decision, these still need changes:
- Land the stacked dependencies first so this diff carries only the refresh-control commit; it is over 33,000 authored lines today.
- Make the adaptive compact test follow
spec:AST-012FR9 (width below the effectivemd, 768 px by default, or height below 500) instead of 744 px, and move the theme motion roles and theplatformcondition, which contradictspec:AST-012FR2 andarchitecture:theme-tokensINV1, into their own pull request. - Make a controlled
isRefreshingthat staysfalseend a pulled refresh, start the end slide fromend()so a refresh that begins and ends in one render still slides back, apply the pull'soverscroll-behavior-y: noneonly while the scroll owner overflows (spec:AST-025FR14), give non-claiming recognizers no blockingtouchmovelistener, and replace the two state-setting Effects inuseRefreshControl.ts. - Add the
## Writing rubric resultssection that spec approval reads.
[Automated review]
| if (latest.current.isControlled) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
In controlled mode a triggered pull returns here, and only the Effect at line 848 can end it, which runs only when isRefreshing changes. A parent that keeps false, or sets true then false in one handler, leaves the list refreshing behind the 60 px inset with aria-busy="true". End a triggered refresh on the first commit after onRefresh returns in which the prop is still false, or render controlled state from the prop.
| useEffect(() => { | ||
| if (isRefreshing || pendingEnd.current == null) { | ||
| return; | ||
| } | ||
| endFrame.current = requestAnimationFrame(() => { | ||
| endFrame.current = requestAnimationFrame(() => { | ||
| endFrame.current = null; | ||
| const play = pendingEnd.current; | ||
| pendingEnd.current = null; | ||
| play?.(); | ||
| }); | ||
| }); | ||
| }, [isRefreshing]); |
There was a problem hiding this comment.
The end slide now waits for this Effect, which runs only when isRefreshing changes. When the begin and the end land in one render (a refresh() from a timer or socket handler with an onRefresh that settles at once), it never runs: aria-busy clears but the content stays at translate: 0px 60px with the spinner spinning in the gap. Scheduling the two frames from end() itself avoids depending on a state change rendering.
| const near = | ||
| isPullEnabledRef.current && | ||
| scrollTopOf(current) < Math.max(heightOf(current), REFRESH_HEIGHT); |
There was a problem hiding this comment.
near never checks that the owner overflows, so an overflow: auto container whose content fits still gets overscroll-behavior-y: none; spec:AST-025 FR14 allows a containment policy only on axes that currently overflow.
| useEffect(() => { | ||
| setIsLocked(isScrollLockActive()); | ||
| return subscribeScrollLock(setIsLocked); | ||
| }, []); |
There was a problem hiding this comment.
This Effect sets state with no comment naming what it synchronizes (architecture:react-component-runtime INV1/INV3), and a lock already held at mount first renders the pull enabled. useSyncExternalStore(subscribeScrollLock, isScrollLockActive, () => false) reads it during render.
| function claimsWhileMoving(recognizer: GestureRecognizer): boolean { | ||
| return ( | ||
| recognizer.acceptsTouchInput && | ||
| !(recognizer instanceof TapGestureRecognizer) | ||
| ); | ||
| } |
There was a problem hiding this comment.
This ignores claimsTouches, so RefreshPullRecognizer (which never claims) still gives the whole refreshable list a {passive: false} touchmove listener; the note at the top of this file says a scroll that begins on such an element waits for a busy main thread in iOS Safari. Requiring recognizer.claimsTouches here removes it.
| export const COMPACT_QUERY = '(width < 744px), (height < 500px)'; | ||
|
|
||
| /** Both: presentations such as a sheet in place of a popover. */ | ||
| export const COMPACT_TOUCH_QUERY = | ||
| '(width < 744px) and (pointer: coarse), (height < 500px) and (pointer: coarse)'; |
There was a problem hiding this comment.
Current spec:AST-012 FR9 defines compact as width below the effective md point (768 px by default) or height below 500, exported once from the theme, and says no resolver keeps size numbers of its own. The 744 px edge sends a 744–767 px touch screen to the popover where FR9 promises the sheet.
| * 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 is a new condition field; spec:AST-012 FR2 keeps the when vocabulary closed to width, height, pointer, contrast and motion until that record adds one, so this needs a pull request that amends FR2.
| const variable = motionRoleVariable(id); | ||
| try { | ||
| tokens[variable] = formatMotionRoleValue(kind, value); |
There was a problem hiding this comment.
These --motion-* names come from table paths and are never declared in tokens.stylex.ts, which architecture:theme-tokens INV1 names as the origin of every core token name; the draft spec:AST-094 FR4 in this same pull request also says touch motion is not themable.
4e4c16b to
8bfbe7c
Compare
github-actions
Bot
added
the
needs:design-review
8bfbe7c to
435e3f7
Compare
…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.
left a comment
There was a problem hiding this comment.
Approval needs an engineering owner to provide or identify the governing spec for the Lab RefreshControl (the four choices the description proposes, including the UIKit-table motion it plays, which current spec:AST-094 leaves to the component's own record) and to settle three conflicts with current spec: the refresh starting with the finger still down (spec:AST-094 FR36/FR37), a pull that works under a coarse pointer only (spec:AST-094 FR19/FR43), and overscroll-behavior-y: none on a scroll container that does not overflow, which needs spec:AST-025 FR14 updated if it stays. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Whatever the decision, these still need changes:
- Land the stacked predecessors first: this pull request carries 35,029 authored lines, about 5,300 of them for RefreshControl itself.
- Make
scripts/touch-support-bundle.test.mjspass again and let the gesture host attach for every pointer (architecture:react-component-runtimeINV13,spec:AST-094FR43). - Remove the per-move scroll-offset read (
spec:AST-094FR9), keep the title's status element mounted (spec:AST-086FR3), route the animator throughinternal/touch(INV13), and stop the page jump when the refresh button runs on short content. - Add fine-pointer before-and-after evidence for the scroll-lock change (
spec:AST-017FR59) and real-device evidence for the iOS Safari and Android Chrome claims.
[Automated review]
| {...stylex.props(styles.spokes)}> | ||
| {spokes} | ||
| </span> | ||
| {title != null ? ( |
There was a problem hiding this comment.
The role="status" span mounts only once title is set, so a title that first appears after mount lands in a newly mounted live region already holding its text, which spec:AST-086 FR3 forbids. Render the status span always and leave it empty without a title.
| const scrolledBack = | ||
| scrollTopOf(owner.current) - geometry.scrollTop0 > 0.5; |
There was a problem hiding this comment.
This reads scrollTop (or window.scrollY) on every move after the pull starts; spec:AST-094 FR9 allows no layout read after the gesture begins. Flag the scroll-back from the owner's scroll event, which the controller already listens for (:490-505), instead of reading the offset per move.
| place(REFRESH_HEIGHT); | ||
| scrollOwnerBy(current, REFRESH_HEIGHT); |
There was a problem hiding this comment.
When the owner cannot scroll by the whole inset, the 60 px translate shows: in the Controlled story with three rows, pressing Refresh scrolled the page 32 px and dropped the list 28 px in one frame. Settle the inset in, or skip it, when the owner cannot absorb it, and do not scroll the page for a scroller that does not overflow.
| * | ||
| * ONLY WHERE THE PRIMARY POINTER IS COARSE | ||
| * | ||
| * - Recognizers attach, and the host registers listeners, only while the |
There was a problem hiding this comment.
Current architecture:react-component-runtime INV13 says the host "attaches for every pointer, with no touch-only query", and spec:AST-094 FR19 and FR43 require recognizers to attach for every pointer; this gate keeps the host coarse-only.
|
|
||
| import {useEffect} from 'react'; | ||
| import {claimRootScrollStyle, reportScrollLock} from './rootScrollStyles'; | ||
| import {holdPageScroll} from '../gestures/pageHold'; |
There was a problem hiding this comment.
Importing the page hold here brings gestures/pageHold.ts and gestures/hostRegistry.ts into Core's root and hooks entries, and scripts/touch-support-bundle.test.mjs fails its two entry tests at this head (they pass on the base).
…he page presented Two proofs failed on a loaded machine with no defect in the control: - the busy-main-thread settle counted the screencast's delivered frames against its cadence. It now hands the compositor's own frame record to busyWindowProof, whose verdict alone judges the settle; a settle paused before the block fails it. - the catch read the settle's position before the touch, which lands later under load. A capture listener now records where the settle stood as the finger landed; a catch that restarts from zero travel fails it.
left a comment
There was a problem hiding this comment.
Approval needs an engineering owner to provide or identify the governing spec for the Lab RefreshControl (the four choices the description proposes, including the UIKit-table motion it plays, which current spec:AST-094 leaves to the component's own record) and to settle three intended departures from current spec: the UIKit release (the refresh starts with the finger still down, and any later lift settles onto the inset from rest, whatever its velocity, against spec:AST-094 FR36–FR38), a pull that works under a coarse pointer only (spec:AST-094 FR19/FR43), and overscroll-behavior-y: none on a scroll container that does not overflow, which needs spec:AST-025 FR14 updated if it stays. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Whatever the decision, these still need changes:
- Land the stacked predecessors first: this pull request carries 35,051 authored lines, about 5,300 of them for RefreshControl itself.
- Make
scripts/touch-support-bundle.test.mjspass again and let the gesture host attach for every pointer (architecture:react-component-runtimeINV13,spec:AST-094FR43). - Remove the per-move scroll-offset reads (
spec:AST-094FR9), keep the title's status element mounted (spec:AST-086FR3), route the animator throughinternal/touch(INV13), stop the page jump when the refresh button runs on short content, attach the pull's recognizer only under a touch presentation, and move the@astryxdesign/builddefault change to its own pull request. - Add fine-pointer before-and-after evidence for the scroll-lock change (
spec:AST-017FR59) and real-device evidence for the iOS Safari and Android Chrome claims.
[Automated review]
…dy part The proof failed on a loaded machine with one long task: it judged long tasks over the whole trace, the pull's trigger and the release's render included, while Layout and Paint were judged only in the steady part. Under a 4x CPU throttle the release's last commit frame also landed past the fixed 50 ms start. The steady part now starts once the page has rendered three frames after the settle began, counted in frames, and long tasks are judged there too. It passes 4 of 4 under the throttle; a paint on every frame of the settle, or an 80 ms block inside it, still fails it.
left a comment
There was a problem hiding this comment.
Approval needs an engineering owner to provide or identify the governing spec for the Lab RefreshControl (the four choices the description proposes, including the UIKit-table motion it plays, which current spec:AST-094 leaves to the component's own record) and to settle three intended departures from current spec: the UIKit release (the refresh starts with the finger still down, and any later lift settles onto the inset from rest, whatever its velocity, against spec:AST-094 FR36–FR38), a pull that works under a coarse pointer only (spec:AST-094 FR19/FR43), and overscroll-behavior-y: none on a scroll container that does not overflow, which needs spec:AST-025 FR14 updated if it stays. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. The latest commit's narrower settle trace is sound, but whatever the decision, these still need changes:
- Land the stacked predecessors first: this pull request carries 35,091 authored lines, about 5,400 of them for RefreshControl itself.
- Make
scripts/touch-support-bundle.test.mjspass again and let the gesture host attach for every pointer (architecture:react-component-runtimeINV13,spec:AST-094FR43). - Remove the per-move scroll-offset reads (
spec:AST-094FR9), keep the title's status element mounted (spec:AST-086FR3), route the animator throughinternal/touch(INV13), stop the page jump when the refresh button runs on short content, attach the pull's recognizer only under a touch presentation, and move the@astryxdesign/builddefault change to its own pull request. - Add fine-pointer before-and-after evidence for the scroll-lock change (
spec:AST-017FR59) and real-device evidence for the iOS Safari and Android Chrome claims.
[Automated review]
commented
Nov 4, 2026
|
Superseded by #9865. |
Depends on #8171. Carries #8562 (Core's internal entries) as a dependency; if the entries land differently, this follows her decision. Review only this PR's change: vjeux/astryx@338dc33...16f8df7
Why
Astryx has no pull to refresh: on a phone, a pull at the top of a list reloads the whole page instead of refreshing the list.
What led to it
Part of the touch-presentations program (
spec:AST-094). #8086 measured UIRefreshControl, and #8170 and #8171 built the pull; this is the component that renders it.Options and tradeoffs
onRefresh(a promise keeps it refreshing) and a controlledisRefreshing.Why this choice
These keep the pull opt-in per list and leave everything else to the browser. It works under a finger, on iOS and Android alike (#8688 and #8697 add the mouse, pen, trackpad and wheel). The docs require a visible refresh button, since a pull has no keyboard or screen-reader equivalent. Under Reduce Motion a release and the end only fade, within 200 ms. These four choices are proposed for owner review. The new lab exports are canary only:
RefreshControlanduseRefreshControl.How to check it
On a phone or the iOS simulator:
Evidence
With #8251's spinner, iOS 26.2 simulator at 60 fps: pull · cancel · end · short list · page, no reload · push back · pull again · catch · Reduce Motion. Android 15 emulator: scroll area · page, no reload.
touch-support-bundlefails because this branch carries fix(useScrollLock): keep the page where it is while an overlay is open #8098's scroll lock; test(core): a touch component's import carries its gesture code, and nothing else does #8671's per-component test replaces it and passes here.Stack (merge in this order): #8360 gesture foundation (carries #8076) → #8086 refresh values → #8170 pull and state machine → #8171 useRefreshControl → this PR. It also needs #8098 (useScrollLock v2, on #8360).