Repository navigation
Conversation
The keyboard layout guide reads only the keyboard's duration, so it is its own export, keyboardNotification.durationMs, the way press is; timing composes it from a binding under its table path, keyboardNotificationMs, so a bundle that reads only the keyboard's duration drops the rest of the table (architecture:component-style-authoring INV6). The table test checks a value bound from an exported group where the group declares it. A test bundles that one read with esbuild and finds no other timing row; both new tests fail on the previous code.
|
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
…export" That export moves to a pull request of its own, meta#8484, so this pull request carries only the animator; timing.ts goes back to the table's keyboardNotificationMs row.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. 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 |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Recommending approval: keyboardNotification gives the keyboard duration its own export while timing.keyboardNotificationMs keeps its path, value and type, and the new bundle test shows that a module reading the export carries no other timing row, as architecture:component-style-authoring INV6 asks. The measured source moved with the value, and the table test still enforces it. Nothing more is needed here; the size saving applies once the keyboard layout guide in #8176 imports the new export.
[Automated review]
meta#8484 puts the keyboard notifications' duration, `keyboardNotification`, in its own export on main, with the text this branch adds, a bundle test for that one read and the table test's rule for bound values. This branch now depends on it for that export. One conflict, in `timing.ts`: this branch's side, which carries the Reduce Motion fade group from meta#8076 beside the same `keyboardNotification` block; the file is unchanged by the merge.
meta#8176's head merges meta#8484 (the keyboard notifications' duration export on main) and meta#8076's 07aa9c0 (the maintainer's chain on main, touch motion internal, the `isRtlElement` resolver), moves the sub-pixel overhang fix in from the Chrome-on-Android branch, and drops its theme-role wording. No conflicts; the CLI goldens regenerate unchanged.
meta#8176's head merges meta#8484 and meta#8076's 07aa9c0 (touch motion internal: no theme sets a motion role), takes this branch's sub-pixel overhang fix (`overlapOf`), and says the keyboard's motion is no motion role. Conflicts, resolved: - useChatKeyboard.ts, keyboardModel.ts and the keyboard README keep this branch's two curves (UIKit's spring, Gboard's slides) in meta#8176's wording: values and no motion role, with no theme to name. - keyboardModel.test.ts keeps the Android table and the bézier, and reads the internal role registry (`motionRoleIn`). `overlapOf` and its two BottomSheet tests now come from meta#8176.
meta#8076's head carries meta#8055's revert of the keyboard notifications' own export (via meta#8258 and the maintainer's meta#8067), so on that line `timing.ts` keeps only the `keyboardNotificationMs` row, and the press convergence (gestures README, the press feedback). Conflicts, resolved: - `timing.ts`: git applied meta#8076's revert over this branch's `keyboardNotification` group, which this branch now takes from meta#8484. The file is meta#8076's with meta#8484's change applied (the group, its binding and the `keyboardNotificationMs` shorthand), nothing else. - The UIKit table index's header lists `keyboard` beside meta#8076's per-spring constants. - The interaction harness README takes meta#8076's table and keeps this branch's `traceRendering` invalidations option in the `trace.ts` row.
…keyboard notification export
uikit/uikit.test.ts: main's leavesOf (a one-line export is a row, sourced
or not) keeps this branch's check of values bound from an exported group
(const {curve: move} = group). uikit/timing.ts merged cleanly.
There was a problem hiding this comment.
Recommending approval: keyboardNotification gives the keyboard notifications' duration its own export while timing.keyboardNotificationMs keeps its path, value and role exclusion, as architecture:component-style-authoring INV6 asks of shared value tables. The new bundle test shows that a module reading the export carries no other timing row, and the table test still requires the measured source on the moved value. Nothing more is needed here; the saving reaches a shipped component once the keyboard layout guide in #8176 imports the export.
[Automated review]
…03 specs) into the keyboard notification export uikit/timing.ts: both sides added a group with its own export at the same place, this branch's keyboard notification and main's Reduce Motion fade; both stay, each composed into timing from its binding as before. uikit/uikit.test.ts: main now checks values bound from an exported group, as this branch did; its copy stays and this branch's duplicate goes.
meta#8076's head is a fast-forward of the ee34b25 meta#8176 carries: meta#8073's began-elsewhere for untracked fingers and a recognizer's own scroll, and the motion roles' table values in the docs. No conflicts; `timing.ts` is meta#8076's with exactly meta#8484's change. This branch takes it ahead of meta#8176, whose next head brings it too.
meta#8076's head folds main through meta#8055's landing (the compositor animator) and is a fast-forward of the 0d1a829 meta#8287 brought in. It merges cleanly with main dbcfc30, which this branch's previous base no longer did. No conflicts; `timing.ts` is meta#8076's with exactly meta#8484's change.
meta#8076's head is a fast-forward of the ee34b25 meta#8176 carries: meta#8073's began-elsewhere for untracked fingers and a recognizer's own scroll, and the motion roles' table values in the docs. No conflicts; `timing.ts` is meta#8076's with exactly meta#8484's change. This branch takes it ahead of meta#8176, whose next head brings it too.
meta#8176 now carries meta#8287's Chrome-on-Android timing (Gboard's measured slides), the keyboard module's shared reveal-pan watch (a browser's own pan to the focused composer put back, which Chrome 124 animated by the keyboard's whole height), the scrollbar thickness measured once, and meta#8076's 2267ee5 (main through meta#8055's landing). Conflicts, resolved as the union of both sides: - keyboard/index.ts and useMobileKeyboard.ts headers and exports: rideKeyboard, keyboardProfileOf and watchRevealPan beside observeKeyboardDismiss and blurAsDismissal; the README's file table lists rideKeyboard.ts and keyboardDismiss.ts. - useChatKeyboard.ts: the header names both; the reveal-pan watch uses this branch's `isTouch` (one touch-presentation read). - ChatLayout.doc.mjs (EN, ZH, dense): meta#8176's ride sentence (in step in Safari and in Chrome on Gboard, once reported in Firefox) with this branch's drag sentence, and no drag dismissing under `resizes-content`. - ChatLayoutKeyboard spec: `openChat` takes both `android` and `args`. `timing.ts` is meta#8176's, with meta#8484's keyboard export.
There was a problem hiding this comment.
Requesting changes: the new keyboardNotification export is the right shape for the keyboard layout guide, but timing is filled through a top-level destructuring (timing.ts:106) that esbuild keeps in every bundle of the module, so the animator's reducedMotionCrossFade import now carries the keyboard row, and the keyboard-only bundle in the new test still carries the Reduce Motion fade row from main's matching binding. Please compose the table from one sourced constant instead of a destructuring (for the fade as well, or narrow the test's claim to the rows it checks), and make the bundle fixtures name the rows that can leak: durationMs:500 in keyboardNotification.test.ts and 383.3 in the animator's fixture.
[Automated review]
|
|
||
| // Composed from a binding, so a bundle that reads only the keyboard's | ||
| // duration drops the rest of the table. | ||
| const {durationMs: keyboardNotificationMs} = keyboardNotification; |
There was a problem hiding this comment.
esbuild keeps this top-level destructuring in every bundle that includes timing.ts (it can't rule out a getter), so the keyboard row now rides into the animator's reducedMotionCrossFade import (97 → 112 B gzip, minified) and every press read. Declaring the duration once as a sourced module constant that both keyboardNotification.durationMs and timing.keyboardNotificationMs name lets esbuild drop it from bundles that don't read it; uikit.test.ts would then check a leaf that names a sourced constant where the constant is declared, as it already does for bindings.
There was a problem hiding this comment.
Fixed in 9865edf: each value is now one sourced constant (KEYBOARD_NOTIFICATION_MS, REDUCED_MOTION_CROSS_FADE_MS, REDUCED_MOTION_CROSS_FADE_CURVE) that its group and timing both name, with no top-level destructuring left, so neither group rides into the other's bundle. The keyboard bundle test now asserts no durationMs:500 and the animator's bundle test no 383.3; both failed on 216d05c and pass here. The table test takes a constant documented with @source as a row of its own.
| expect(code).not.toContain('navigationBarHideShowMs'); | ||
| expect(code).not.toContain('buttonReleaseMs'); |
There was a problem hiding this comment.
At this head this build is var e={durationMs:383.3},{durationMs:s}=e,o={durationMs:500,curve:[.42,0,.58,1]},…: it carries the Reduce Motion fade row through the binding at timing.ts:144-145, which these two assertions don't name, so the test passes while the bundle still carries another row. Asserting, for example, expect(code).not.toContain('durationMs:500') makes it fail today.
… constants timing.ts filled its table from top-level destructuring bindings of the keyboard notification group and of the Reduce Motion fade group (main's). esbuild keeps a top-level destructuring in every bundle, since a property read may run a getter, so each binding carried its group into a bundle that read only the other: the keyboard layout guide's bundle carried the fade, and the animator's carried the keyboard's duration. Each value is now one sourced constant that its group and the table both name, and the table test's source check takes a constant documented with @source as a row of its own. The keyboard bundle test now names the fade's row and the animator's bundle test the keyboard's, and both fail on the previous code.
There was a problem hiding this comment.
Requesting changes for one test assertion: the plain constants fix the leak in both directions, so the keyboard-only bundle now carries only 383.3, the animator's bundle no keyboard row, and a press read no longer carries the Reduce Motion fade. With the fade's values now constants, esbuild prints a leaked fade as e=500,…,{durationMs:e,…}, so expect(code).not.toContain('durationMs:500') in keyboardNotification.test.ts stays green if main's fade destructuring comes back. A marker that does not depend on that shape, such as expect(code).not.toMatch(/\b500\b/) together with expect(code).not.toContain('.42,0,.58,1'), fails on that regression and on the previous composition and passes here.
[Automated review]
| // not the Reduce Motion fade that shares the table's composition. | ||
| expect(code).not.toContain('navigationBarHideShowMs'); | ||
| expect(code).not.toContain('buttonReleaseMs'); | ||
| expect(code).not.toContain('durationMs:500'); |
There was a problem hiding this comment.
Now that the fade's values are constants, esbuild prints a leaked fade as e=500,t=[.42,0,.58,1],i={durationMs:e,curve:t}, so this never matches: with const {durationMs: reducedMotionCrossFadeMs} = reducedMotionCrossFade; (or const {curve: move} = reducedMotionCrossFade;) restored in timing.ts, the keyboard-only bundle carries the fade and this test still passes. A marker that doesn't depend on where esbuild prints the value catches both:
expect(code).not.toMatch(/\b500\b/);
expect(code).not.toContain('.42,0,.58,1');There was a problem hiding this comment.
Fixed in 1adf75c: the test now asserts no bare 500 (/\b500\b/) and no .42,0,.58,1. With a fade destructuring put back over the constants, the old durationMs:500 check stays green and the new assertion fails on s=500,….
…e keyboard module The pull request's head carries the maintainer's merge of meta#8076's 0400cb2 on 4693f5e, the same meta#8076 head this branch merged in 0c6671d. Merging it makes the branch a fast-forward of the pull request's head; its tree changes nothing here (the two merges resolve alike), and `timing.ts` is meta#8076's with exactly meta#8484's change.
meta#8176 now rides a Tall BottomSheet on Gboard's slides in Chrome: the reveal lands the body at once and the content box rides up on the show's curve, and through the hide the room stays while the content box rides down by the scroll the room's drop takes back (the range the room adds, measured), both cleared in one commit at rest. It also takes meta#8076's 0400cb2 (meta#8505 with meta#8446's harness line). No conflicts; `timing.ts` is meta#8176's, with meta#8484's keyboard export.
… prints it
With the fade's values now shared constants, a returning destructuring of
the fade group bundles as `s=500,…{durationMs:s,…}`, which the
`durationMs:500` check missed. The test now looks for the fade's length as a
bare 500 and for its curve, and fails when a destructuring of the fade
group is put back.
There was a problem hiding this comment.
Recommending approval: keyboardNotification gives the keyboard notifications' duration its own export, and the plain constants keep each esbuild read of timing.ts to its own rows, so a keyboard-only read carries just 383.3 and neither the animator's fade read nor a press read carries another group, as architecture:component-style-authoring INV6 asks. The keyboard fixture's new markers (/\b500\b/ and .42,0,.58,1) fail when the fade destructuring comes back in either form, and both leak fixtures fail on the earlier composition. Nothing more is needed here; when #8176 rebases, its copy of timing.ts should take this composition, since it still composes the table through the earlier keyboard binding.
[Automated review]
…8549 and meta#8554 into ChatLayout ChatLayout's keyboard ride now stacks on its dependencies, merged as one base (58f88dc): the keyboard module (meta#8559, on main), the sheet's ride on Gboard's slides (on meta#8559, meta#8076 and meta#8484), the split frame evidence (meta#8548 under meta#8549 and meta#8554), and the rendering trace's invalidations with the compositor-update classifier (meta#8554). Every file those carry takes their version; ChatLayout's parts are put back on top: - `useMobileKeyboard.ts`: the sheet ride's version (its coarse-pointer gate, main's Reduce Motion query), plus `claimTapFocus` and the ChatLayout lines of its header; its tests and `index.ts` likewise. - The keyboard README and the guide's, model test's, rideKeyboard's and layoutViewport's headers: theirs, plus ChatLayout's lines. - `keyboardLayoutGuide.ts` takes the frame-watch fix, and `timing.ts` meta#8484's constants with meta#8076's change. - The harness is meta#8554's, beside meta#8549's and meta#8446's content. - The browser-support guide names ChatLayout's composer beside the sheet; the modules' changesets stay theirs, and ChatLayout's own changeset names the guide (and the CLI).
…eta#8566) and meta#8554 A dependency-only merge. The base (ad271a7) is the sheet's Gboard ride (meta#8566, on meta#8559 39d37ef, meta#8484 and meta#8076 62ecda1) merged with meta#8554 21ea4dc, which carries cixzhang's meta#8446 66c651f, meta#8549, meta#8548 and meta#8045 at their current heads, so no harness file is resolved here. ChatLayout's own change (this branch's commits since 58f88dc) applies on it with meta#8559's names (`subscribeKeyboardHeight`, `getKeyboardHeight`, `keyboardHeight.ts`).
…ss renders; the base moves to the foundation heads The base (3f66f76) is the sheet's Gboard ride (meta#8566, on meta#8559 6be79f4, meta#8484 and meta#8076 62ecda1) merged with meta#8554 e4f3c8d, on cixzhang's meta#8446 87e516b, meta#8549, meta#8548 and meta#8045, with no conflict. meta#8559 moved `useKeyboardHeight` here, to its first consumer: ChatLayout lays its dock and transcript padding out from it. Its subscribe and snapshot functions are now stable for the component's life (per window), so React keeps one subscription across renders; before, every render unsubscribed and resubscribed, and a lone reader detached the module's listeners and measured again on each keyboard change (INV11). Red first: "reads the layout once per keyboard change for a lone component" expected no listener re-added and one read, and got two `addEventListener` calls. The hook's tests (one re-render per change, a keyboard already up, StrictMode's replay leaving no listener) and the README's layout example come with it.
…d notification export Main's timing table keeps the Reduce Motion fade's own export, now 200 ms with its new reasoning (design:reduced-motion DR2's cap, DEC-1). This side composes the table's own-export groups from plain constants, so a bundle that reads only the keyboard's duration drops the rest of the table: the fade's constant takes main's value and comment. The keyboard bundle test now looks for the fade's 200 ms; with main's destructured composition it fails, finding the fade in a keyboard-only bundle.
There was a problem hiding this comment.
Recommending approval: keyboardNotification gives the keyboard notifications' duration its own export, and the plain constants keep each esbuild read of timing.ts to its own rows, as architecture:component-style-authoring INV6 asks of shared value tables. Both bundle fixtures fail when either group's destructuring or a property read in the table comes back, and the table test still requires the measured source on every moved value. Nothing more is needed in the code; the description's Evidence bullet still names the fade's pre-merge durationMs:500, while the fixture now checks the 200 ms fade with /\b200\b/.
[Automated review]
Review only this PR's change: vjeux/astryx@0cbddea...218ca70
Why
The keyboard layout guide reads one number from the UIKit timing table, how long the software keyboard takes to move. Reading it as a key of the
timingobject pulls the whole table into every bundle that uses the guide, because object properties do not tree-shake.What led to it
This comes from the work to make Astryx's touch components behave as native iOS ones do (sheets, menus, gestures and their motion).
architecture:component-style-authoringINV6 asks that a module reading one tuning value ships only that value; a bundle test showed the guide's bundle carrying the whole timing table.Options and tradeoffs
pressalready has: one sourced constant, readable alone.Why this choice
One constant,
KEYBOARD_NOTIFICATION_MS = 383.3, backs both the newkeyboardNotificationexport and the table'skeyboardNotificationMs, so nothing is copied. The Reduce Motion fade's group (from #8055) gets the same treatment, because a top-level destructuring in the same file made each bundle carry the other group's values.How to check it
pnpm exec vitest run packages/core/src/motion/uikit/keyboardNotification.test.ts.keyboardNotification.durationMsalone with esbuild: the output carries 383.3 and no other timing row, not even the fade's 200 ms.Evidence
@sourceas a sourced row.[chore]changeset covers the release check.Scope