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
![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 an owner decision: the draft spec:AST-094 adds requirements no owner has approved yet, and the sheet release rule, the UIKit constants table and the motion math all rest on its FR1–FR3 and DEC-1. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so if they are an engineering owner), or names the requirements to change. Independently of that decision, please:
- Land #8052–#8054 first, so this diff is its own 410 lines instead of 4,490.
- Correct the release-rule evidence in
sheet.tsandKNOWN_MISSES: on the same 47 releases a quadratic projection reproduces 46 to the linear rule's 44, and both misses called noise or an under-read are fast downward flicks UIKit dismissed. - In AST-094, restate FR1, FR9 and FR13 as observable outcomes, and order it after #8003, which proposes the AST-090 it cites, or drop that citation.
[Automated review]
| * detent it settled on): a linear projection at 0.114 s reproduces 44; | ||
| * two others straddle the medium / dismissed boundary less than 1 pt | ||
| * from it, closer than any single value separates (reading noise), and | ||
| * the last one's final drag update under-reports its throw (1140 pt/s | ||
| * for a 3000 pt/s injection). A quadratic projection (v·|v|) is | ||
| * rejected by releases at 927, 991 and 997 pt/s, Oct 23 2026 |
There was a problem hiding this comment.
The 47 releases don't support these three claims. A quadratic projection offset + k·v·|v| with k from about 9.4e-5 to 1.2e-4 s²/pt reproduces the 927, 991 and 997 pt/s releases and 46 of 47 overall; the linear 0.114 s rule reproduces 44. The other far-under-read releases that recorded a spring (off-120.v3000, off-60.v1500, zeta.awayFromTarget.v1000) attached ζ 1 and landed where their low reading projects, so UIKit acted on the reading. All seven releases at 1072 pt/s or faster moving down from below the medium detent dismissed, including the two the rule keeps at medium. Could this describe the model as unsettled and add releases that separate the candidates (M-SH09) before calling it measured?
| index: -1, | ||
| offset: dismissedOffset, | ||
| projected: choice.projected, | ||
| spring: isFast ? springs.sheetDismissSwipe : springs.sheetDetent, |
There was a problem hiding this comment.
None of the 18 releases in the test that started below the medium detent, every dismissal among them, recorded a spring, so the slow-dismissal spring (springs.sheetDetent) is unmeasured. By the table's own rule it should be labelled a guess with its measurement.
| - **FR1 — One table of UIKit values.** Every spring, duration, deceleration | ||
| rate, rubber-band coefficient, gesture threshold and haptic moment a touch | ||
| presentation uses MUST come from one shared table of UIKit values. A | ||
| component MUST NOT carry a number of its own. |
There was a problem hiding this comment.
FR1 (one shared table), FR9 (shared recognizers, no component render per move) and FR13 (one shared resolver) prescribe internal mechanisms, which architecture:knowledge-contracts INV18 keeps out of product requirements. Could these state the observable outcome and move the mechanism to an architecture record?
| 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 |
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewComponent 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 |
|
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. |
|
Agreed: one implementation.
|
d62a71f to
2999afe
Compare
|
Pushed in 2999afe (head): the rule is a pure function ( |
github-actions
Bot
added
the
needs:design-review
There was a problem hiding this comment.
Approval waits on an owner decision: resolveSheetRelease rests on draft spec:AST-094 FR3 and DEC-1, which no owner has approved, and an engineering owner has asked that the sheet's release decision converge with the shared release rule (AST-057 FR5) rather than live in BottomSheet. An engineering owner records that decision by commenting /approve-spec <full head SHA> once this pull request carries the record text being approved (the author may do so as an engineering owner), or says where the release rule must live before it lands. Independently of that decision, please:
- Land the ten open predecessors first (or retarget onto #8076), so this diff is its own 560 lines instead of 29,663.
- Remove the
platformadaptation condition, which the closedwhenvocabulary ofspec:AST-012FR2 does not allow. - Drop the outdated AST-094 copy or carry #8316's text, since the carried FR4 forbids the theme motion roles this diff ships.
- Make the adaptive compact test read the theme's effective
md(AST-012 FR9), or correct the Changeset that says it does.
[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 sits outside the closed when vocabulary of spec:AST-012 FR2 ("Future environment fields require explicit additions to this closed vocabulary."), and nothing here amends FR2. Could this wait until an owner adds it there?
| export const COMPACT_TOUCH_QUERY = | ||
| DEFAULT_COMPACT_DEFINITION.coarsePointerQuery; |
There was a problem hiding this comment.
This builds the compact test once from the default width points, so a theme that overrides md still gets the sheet decided at 768 px. spec:AST-012 FR9 measures compact against the effective md, and the touch-presentation Changeset says "the theme's md point". Could the query come from the active theme's points (compactDefinition(points)), or the Changeset state the fixed 768 px?
| * threshold from 997 to 1028 pt/s, and the spring follows the branch on | ||
| * all 87 releases of Oct 24 2026, so the switch lies in (997, 1028] |
There was a problem hiding this comment.
In sheetRelease.test.ts only 26 of the 87 Oct 24 releases (55 of all 134) carry a recorded ζ; the rest are null. Could this read "on the 26 of 87 releases that recorded a spring"? Line 16's "for every release" overstates it the same way.
| - **FR4 — Physics are not themable in touch presentations.** A touch | ||
| presentation MUST read its motion from the table on every theme. Themes keep |
There was a problem hiding this comment.
This FR4 is from an outdated AST-094 text. The description names #8316, whose FR4 makes touch motion themeable (as does the closed #8052), and this diff ships defineTheme({motion}) roles, which this text forbids. Could this pull request drop the record and depend on #8316, or carry #8316's text exactly?
2999afe to
a5e2879
Compare
There was a problem hiding this comment.
Approval waits on an owner decision: resolveSheetRelease rests on draft spec:AST-094 FR1–FR3 and DEC-1, which no engineering owner has approved, and this head still carries an older AST-094 text than #8316. An engineering owner records that decision by commenting /approve-spec <full head SHA> once the pull request carries the record text being approved (the author may do so as an engineering owner). Independently of that decision, please:
- Land the sixteen open predecessors first, so this diff is its own 560 lines instead of 34,681.
- Drop the outdated AST-094 copy or carry #8316's text, since its FR17 and FR8 contradict the now-current
spec:AST-100FR2 and the reduced-motion design's 200 ms cross-fade. - Keep
sheetout of theuikitnamespace that@astryxdesign/core/motion/experimentalpublishes, sincespec:AST-100FR3 keeps a component's thresholds internal. - Make the adaptive compact test read the theme's effective
md, take the press-surface selection lock only for touch or a coarse pointer (or addspec:AST-017FR59's fine-pointer evidence), and settle the stablereducedMotion.stylexsubpath.
[Automated review]
| - **FR17 — UIKit's readings.** A recognizer's thresholds and readings MUST | ||
| follow UIKit's and come from the table. A pan begins at 10 pt of travel on | ||
| either axis, and its translation excludes that travel; its velocity is | ||
| UIKit's estimate, 0.2 × the newest event's instantaneous velocity + 0.8 × | ||
| the previous event's, never decayed through a rest. |
There was a problem hiding this comment.
FR17's "never decayed through a rest" contradicts the now-current spec:AST-100 FR2 ("It is 0 when the contact stayed still for longer than a fixed stillness interval before lifting") and its DEC-1, which rejects iOS's blend with no stillness reset; the stack's VelocityTracker already follows AST-100. Could this record defer to AST-100 here, or carry #8316's text?
| place, and any other move fades through. A navigation push or pop | ||
| cross-fades over 0.5 s on the ease-in-out curve; a sheet, a full-screen | ||
| modal or an alert fades on its own presentation spring and drops its travel | ||
| and scale; menus open and close on UIKit's Reduce Motion spring. Motion that |
There was a problem hiding this comment.
Under Reduce Motion, the now-current reduced-motion design (DR2) requires a touch transition to "change opacity only and ... finish in 200 ms or less"; a 0.5 s push or pop cross-fade and a fade on the presentation spring exceed it, and timing.reducedMotionCrossFadeMs is already 200. Could FR8 follow DR2?
| } from './gestures'; | ||
| export {navigation, registerNavigationTable} from './navigation'; | ||
| export {registerScrollTable, scroll} from './scroll'; | ||
| export {sheet} from './sheet'; |
There was a problem hiding this comment.
motion/experimental.ts publishes this barrel as uikit, so this export makes uikit.sheet.release.fastSpeed (BottomSheet's flick threshold) readable from @astryxdesign/core/motion/experimental. spec:AST-100 FR3 keeps a component's thresholds internal (spec:AST-002 FR3), and the [chore] Changeset calls the rule internal. Could sheetRelease.ts import ../motion/uikit/sheet directly instead?
| @@ -283,6 +301,9 @@ function onPointerDown(event: PointerEvent): void { | |||
| gesture: idlePressGesture(), | |||
| pointerId: event.pointerId, | |||
| timer: null, | |||
| // A held finger never starts text selection on the surface (UIKit's | |||
| // controls and cells never do), on any coarse pointer. | |||
| unlock: lockSelection([element], event.target), | |||
There was a problem hiding this comment.
This runs for every non-mouse pointer, so a pen on a screen whose primary pointer is fine also gets user-select: none on the pressed surface (a Button probe shows it on this head and not on main), while the Changeset says "on any coarse pointer". spec:AST-017 FR59 needs a stated reason and fine-pointer before-and-after evidence for that; could the lock be limited to touch or a coarse primary pointer?
| // Proposed; pending owner review (spec:AST-094 OQ4): the Reduce Motion | ||
| // media keys and fade constants, for a transition in another package. | ||
| './motion/reducedMotion.stylex': { | ||
| source: './src/motion/reducedMotion.stylex.ts', | ||
| types: './dist/motion/reducedMotion.stylex.d.ts', | ||
| default: './dist/motion/reducedMotion.stylex.js', |
There was a problem hiding this comment.
This adds a stable @astryxdesign/core/motion/reducedMotion.stylex subpath, but neither this head's AST-094 nor #8316 has an OQ4, and no Changeset announces the new import path (touch-experimental-subpaths.md says nothing else is added). Could it sit behind the experimental boundary until an owner records the decision?
a5e2879 to
cdef266
Compare
There was a problem hiding this comment.
Changes requested. The sheet release rule follows the current spec:AST-094 (FR2, FR3, FR36 and FR37's back-out cancel, with spec:AST-100 FR3 for its speed thresholds), and all 164 recorded releases replay as the tests claim. This diff still carries its stacked predecessors, 24,505 of its 25,284 authored lines, so please rebase onto main (the rule's six files need nothing from the stack and conflict only on one motion/README.md table row), or land the predecessors first through their own reviews, so this pull request is its own six files.
[Automated review]
The sheet's release rule on main alone: the rule (sheetRelease.ts), its UIKit constants (motion/uikit/sheet.ts), their tests and changeset. Its six files need nothing from the gesture line; the motion README's table gains the uikit/sheet.ts row.
c08527b to
a9b0d8b
Compare
github-actions
Bot
removed
the
needs:design-review
There was a problem hiding this comment.
Changes requested. The rule follows the current spec:AST-094 (FR2, FR3, FR36, FR37) and spec:AST-100 FR3, and its 164 recorded releases replay as the tests claim, but the tests replay every release with start at medium, so FR37 decides each one that bounds minProjectionSpeed, detentTolerance and the projection time's lower edge, and the suite stays green with the floor at 100 or 494 pt/s or the tolerance at 100 pt. Please add a second replay of the 164 releases with start at each release's own offset (the cancel then never applies, as the spring test already does), expecting UIKit's recorded outcome: the head passes it on all 164, and it fails for a floor of 243 or 251 pt/s, a projection time of 0.09 s or a tolerance of 8 pt.
[Automated review]
| dismissedOffset: GONE, | ||
| position: offset, | ||
| velocity, | ||
| start: MEDIUM, |
There was a problem hiding this comment.
Replaying with start: MEDIUM lets FR37 decide 72 of the 164 releases, including every one that bounds minProjectionSpeed (back.large224.v250.r1 and .r2), detentTolerance (d0.v1000) and the lower edge of projectionSeconds (d0.v-1400), so the suite stays green with the floor at 100 or 494 pt/s or the tolerance at 100 pt. Could a second loop replay the same releases with start: offset (the cancel then never applies, as the spring test does) and expect UIKit's own outcome? The head passes it on all 164, and it fails for a floor of 243 or 251 pt/s, a projection time of 0.09 s or a tolerance of 8 pt.
…olding the rule's edges Every replay started at the medium detent, so FR37's cancel decided each release moving back toward it, and the suite stayed green with the slowest projected speed at 243 or 251 pt/s, a 0.09 s projection or an 8 pt tolerance on a stop. A second replay starts each release where it is lifted, so the cancel never applies, and expects UIKit's own outcome and spring on all 164: it passes here and fails each of those four mutants, which the first replay alone does not.
meta#8068 is now one commit on main. Its five code files are byte for byte the old meta#8068 head's (c08527b), which this branch already carries with its own changes on top, so the three conflicts take this branch's side: sheetRelease.ts and its test (this branch's edits on the same rule) and the motion README, whose table here already has the uikit/sheet.ts row with the gesture line's rows.
There was a problem hiding this comment.
Changes requested. The rule follows the current spec:AST-094 (FR2, FR3, FR36, FR37) and spec:AST-100 FR3, and the new second replay holds each sheet.release constant to its measured window. One fix remains: the FR37 cancel sends a release to the stop nearest start, gone included (sheetRelease.ts line 161), so when start is where a finger caught the sheet past the midpoint to gone, a release moving back toward it dismisses the sheet (detents 62 and 404.32, gone 874: start 720, let go at 600 moving down at 60 pt/s, dismissed, where the same lift from a drag that began on medium settles on medium). Please resolve a cancel to a detent, or to a stop the caller passes, and add a test with such a start.
[Automated review]
| // the drag began cancels, whatever the travel so far, to the stop nearest | ||
| // that start. | ||
| if (velocity !== 0 && Math.sign(velocity) === Math.sign(start - position)) { | ||
| index = nearestIndex(positions, start); |
There was a problem hiding this comment.
This can turn a cancel into a dismissal. positions includes the dismissed offset, so when start is where a finger caught a settling sheet past the midpoint to gone, the stop nearest it is gone: resolveSheetRelease({detents: [62, 404.32], dismissedOffset: 874, position: 600, velocity: 60, start: 720}) returns kind: 'dismiss', while the same lift with start: 404.32 settles on medium. spec:AST-094 FR37 says a release moving back toward the start must cancel. Could the cancel resolve to a detent (nearestIndex(detents, start) keeps all 339 tests in the file green), or take the stop to return to from the caller, with an edge test for a start past the dismiss midpoint?
…tent, never to gone spec:AST-094 FR37: a release whose velocity points back toward the start cancels. The rule sent it to the stop nearest the start with gone among the stops, so a drag begun where a finger caught the sheet past the midpoint to gone dismissed it when let go moving back down (detents 62 and 404.32, gone 874: started at 720, let go at 600 moving down at 60 pt/s), where the same lift from a drag begun on medium settles on medium. A cancel commits nothing, so it now goes to the detent nearest the start. Test: that release at 60, 400 and 1500 pt/s settles on medium (it dismissed before), as the same lift from medium does.
There was a problem hiding this comment.
Changes requested. The release rule follows the current spec:AST-094 (FR2, FR3, FR36, FR37) and spec:AST-100 FR3, a cancel now returns to a detent, and the second replay holds each sheet.release constant to its measured window. One change remains: resolveSheetRelease re-implements motion/resolveSnapPoint.ts, which main now documents as the one release policy a component's rule calls (sheetRelease.ts lines 65–86 and 132–165), and the two copies already differ on where a cancel lands. Please build the rule on resolveSnapPoint with the sheet.release thresholds, keeping only the spring choice and the detent-only cancel (ideally as a resolveSnapPoint option, so every use's cancel commits nothing); with that change sheetRelease.test.ts still passes in full.
[Automated review]
| // Below the slowest projected speed the release is where it was let go. | ||
| const projectedVelocity = | ||
| Math.abs(velocity) < sheet.release.minProjectionSpeed ? 0 : velocity; | ||
| const choice = selectSnapPoint(positions, { | ||
| position, | ||
| velocity: projectedVelocity, | ||
| rate: PROJECTION_RATE, | ||
| }); | ||
| const isFast = Math.abs(velocity) >= sheet.release.fastSpeed; | ||
| let index = choice.index; | ||
| if (isFast) { | ||
| // A flick goes at least to the next position beyond where it is. | ||
| const tolerance = sheet.release.detentTolerance; | ||
| let next = -1; | ||
| if (velocity > 0) { | ||
| next = positions.findIndex(p => p > position + tolerance); | ||
| } else { | ||
| for (let i = positions.length - 1; i >= 0 && next === -1; i -= 1) { | ||
| if (positions[i] < position - tolerance) { | ||
| next = i; | ||
| } | ||
| } | ||
| } | ||
| if (next !== -1 && (velocity > 0 ? next > index : next < index)) { | ||
| index = next; | ||
| } | ||
| } |
There was a problem hiding this comment.
This block, with rateForProjection and nearestIndex above (lines 65–86), repeats motion/resolveSnapPoint.ts (its lines 59–75 and 127–162), whose header calls it the one "where does a release land" helper and says "a second one should not be added beside it". Could the rule call it instead and keep only the detent-only cancel after it, or move that cancel into resolveSnapPoint as an option so every use's cancel commits nothing?
const choice = resolveSnapPoint({
points: positions,
position,
velocity,
flickThreshold: sheet.release.fastSpeed,
tolerance: sheet.release.detentTolerance,
projection: {
seconds: sheet.release.projectionSeconds,
minSpeed: sheet.release.minProjectionSpeed,
},
});
const isFast = Math.abs(velocity) >= sheet.release.fastSpeed;
let index = choice.index;With this in place of lines 132–158, and rateForProjection removed, all 340 tests in sheetRelease.test.ts pass.
Review only this PR's change: vjeux/astryx@6aea9bd...9e33080
User need
A BottomSheet released by a finger should settle where an iOS sheet would. Today it projects like a scroll view (0.998), sending a moderate flick a detent too far. This adds the measured rule as a pure function.
Problem and solution fit
motion/uikit/sheet.ts: projection time (0.1 s), slowest projected speed (250 pt/s), flick speed (1008 pt/s), on-a-stop tolerance (1 pt).BottomSheet/sheetRelease.ts:resolveSheetRelease, where the sheet goes and on which spring; gone below the screen counts as a stop.offset + velocity × 0.1 s. No minimum travel (UIKit has none).Current specification
spec:AST-094FR1, FR2, FR36 (measured values with sources; one decision, at the release) and FR37 (a release moving back toward its start cancels).BottomSheet.spec.mdclaims no release physics.Public delta and caller burden
None: internal; nothing calls it yet (
[chore]).Design and alternatives
Evidence
The tests replay 164 releases of a
[.medium(), .large()]sheet read from UIKit (iOS 26.2 simulator, 0–4000 pt/s; 87 placed to separate candidate laws, 30 back-outs):A factual note: UIKit's own sheet cancels no back-out. Past the midpoint toward large, let go moving back at 60–244 pt/s, 7 of 7 settled on large; 90 % of the way to gone, moving back at 96–264 pt/s, 6 of 6 dismissed. From 250.6 pt/s its projection decides (not at 243.6).
A slow dismissal's spring is a stand-in (none recorded).
Scope
Testing
npx vitest run packages/core/src/BottomSheet/sheetRelease.test.ts packages/core/src/motion/): 894 passed, the FR37 tests included (each fails without the rule);startis required, so no caller can skip the cancel. Failing first: a release moving back toward a start where a finger caught the sheet near gone settles on the detent nearest that start (it dismissed).pnpm -F @astryxdesign/core typecheck, eslint, prettier: clean.pnpm check:syncandpnpm check:changesets: clean. RootCI=true pnpm lint: 0 errors..github/scripts/lib/component-size.js), main → this PR: BottomSheet's import: JS 34,772 → 34,772 B gzip; CSS 8,079 B gzip, unchanged.Stack: this PR is on main. #8105 the release wiring and the rest of the sheet stack build on it.