Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
|
AI review status for this pull request.
|
There was a problem hiding this comment.
Thanks for the detailed TabBar work. The review is on hold for an engineering owner, and it also needs changes.
Needs an owner decision. This pull request changes Core, the CLI, the interaction harness and Storybook, so it is not Lab-only. Its TabBar and SearchField play UIKit's measured springs and durations as literals, which spec:AST-094 FR4 forbids. Their behavior has no current spec, and minimizeBehavior and automaticallyActivates change meaning by input device without the 'adaptive' value architecture:public-component-api INV3 calls for. An engineering owner needs to approve this as Lab direction, by commenting /approve-spec <full head SHA>, or update AST-094 FR4. The author may do this as an engineering owner. Landing the Lab code in Lab-only pull requests also resolves the hold.
Blocking changes:
- The diff against
mainis 19,202 authored lines, mostly from the four open stacked PRs. That is far over the limit even for an owner-directed batch. Land the stack first and rebase, then split this PR's own 6,823-line commit or show why its parts cannot be separated. SearchFieldTouch.ts:505-547adds a second document-level click swallower beside Core's click guard, and it already eats clicks the guard lets through (withoutClickGuardreplays withdetail: 1). ExtendclickGuard.tsto cover the compatibility mouse events and callarmClickGuardinstead.useTabBarMinimize.ts:106-109keeps the Document in React state and sets it from a layout effect, so every mount renders twice. Own it through the capsule's callback ref instead.useTabBarSearch.ts:187-189runs a dependency-free layout effect to do what the Cancel button's callback ref at:183-185can do. Move the assignment into that ref.
[Automated review]
| } | ||
| }; | ||
| for (const type of types) { | ||
| doc.addEventListener(type, swallow, true); |
There was a problem hiding this comment.
This is a second document-level stray-click swallower beside Core's click guard (packages/core/src/utils/clickGuard.ts), which this code already reads through isClickGuardArmed. The guard lets through clicks dispatched inside withoutClickGuard, such as the gesture host's replays with detail: 1, but this swallow eats them while it is armed. Extend clickGuard.ts with an option that also swallows the compatibility mousedown/mouseup until that click, call armClickGuard here, and delete armSwallow.
There was a problem hiding this comment.
| const [doc, setDoc] = useState<Document | null>(null); | ||
| useLayoutEffect(() => { | ||
| setDoc(capsuleRef.current?.ownerDocument ?? null); | ||
| }, [capsuleRef]); |
There was a problem hiding this comment.
A Document is a DOM node, and architecture:react-component-runtime INV1 keeps DOM nodes out of React state. Setting it from a layout effect also renders every TabBar twice, first with no document. Own it through the capsule's callback ref (TabBar already merges capsule refs) and give the pan and useFingerScroll refs that this ref fills. That also removes the second effect at :122-124.
There was a problem hiding this comment.
Fixed in 3fe213b. The bar's document and root now live in refs that a capsule callback ref fills. TabBar merges it into the capsule's refs, so there's no state, no second render, and no layout effect. useFingerScroll takes that ref; #9699 widens its type to a ref holding a document, which it already handled. The callback also restarts the downward run when the bar lands in another document.
| useLayoutEffect(() => { | ||
| pressTargetRef.current = isCancel ? cancelElementRef.current : null; | ||
| }); |
There was a problem hiding this comment.
This dependency-free layout effect runs on every render and names no external system. The Cancel button mounts only while isSearchCancel, the same condition as isCancel, so pressTargetRef.current = element in the cancelRef callback above does the same job. Then this effect can go.
There was a problem hiding this comment.
Fixed in 3fe213b. The Cancel button's callback ref now sets the press tracker's target, and the effect is gone. The button mounts exactly while isCancel holds: both mean the search is active under a finger.
…ternal touch entry Internal entries land with their first user, per meta#8562. Lab's SearchField animates with Core's animator, solves UIKit's timing curves with Core's cubic-bézier solver, listens for taps with Core's gesture hook, reads the writing direction as Core does and claims focus moves as BottomSheet's keyboard hook does, so `@astryxdesign/core/internal/touch` re-exports, by name: - the animator's internal factory as `createMotionAnimator`, a name no public factory has (architecture:react-component-runtime INV13; spec:AST-107 FR6's public factory is `createAnimator`), with its types `MotionAnimator`, `MotionAnimatorOptions`, `AnimateOptions`, `MotionCurve` and `MotionHandle`; - `cubicBezier` and UIKit's `timing` table; - `useGestureRecognizer`, `useTouchPresentation` and `isRtlElement`; - `claimFocusMoves`, new in `utils/`: the capture-phase `blur` claim that BottomSheet's keyboard hook made inline, which that hook now calls. Its BottomSheet tests prove the move unchanged: with the claim's focus removed, four of them fail. Named re-exports of code already on main, plus that one extraction: no behavior changes, no docs, no root re-export, and the `@astryx/no-core-internal-import` lint rule already limits the entry to Core and Lab. The entry's test checks it re-exports the same values Core imports by path, and that it carries no public factory's name. Two Lab modules that other Lab components will share come with them: `internal/useMotionAnimator`, a stable animator bound to an element through a callback ref, and `keyboard/useKeyboardHeight`, Lab's listener for the software keyboard's height: spec:AST-099 FR2's measurement from the visual viewport, in every browser (DEC-4).
e3d3436 to
3fe213b
Compare
|
Round 1 is addressed in 3fe213b, restacked on #9399's new head and on #9700's round:
|
…it's clock Lab's SearchField is the phone search field of iOS 26's UISearchController, in three placements: - stacked: under a navigation bar's title, a tap activates it into the bar over UIKit's 383 ms ease-in-ease-out: the field narrows, a glass close button slides in beside it, and the large title rides up and fades; the close button cancels on the same clock; - bottom: iOS 26's toolbar field, resting above the keyboard by the keyboard's height as spec:AST-099 FR2 measures it from the visual viewport, in every browser (DEC-4), never overlaying the keyboard on the page (DEC-2); its gel plays the activation and cancel frames measured on iOS, and a change of mind mid-motion turns from where the field is, without a snap; - tab: the content a tab bar's search tab holds. In the bottom and tab placements, under a finger or a pen on a touch screen, wherever the keyboard covers the page (Mobile Safari, Chrome on Android), the field claims a tap on its text, so it focuses without the browser's reveal, which would pan the page (FR4), and the caret lands where iOS puts it; Core's click guard keeps the claimed tap's late mouse events from the page. The stacked field keeps the browser's own tap. With a fine primary pointer (a mouse, a desktop pen tablet) the browser keeps its own focus and click, and the parts take their places at once. With reduced motion the travel becomes a cross-fade under a finger (design:reduced-motion DR2). The values measured on the iOS 26.2 simulator live in Lab (internal/searchMotion.ts), where the bars that host the field can read them too. Its close and clear buttons' names are in the shared message catalog, as Lab's other components' are. `astryx discover` lists it. Its unit tests cover the model, the motion and the field; its interaction specs prove the stacked, bottom and tab placements in Chromium and WebKit, including the change-of-mind turns on the motion's own clock and, with Chrome on Android's keyboard report present, the claimed tap and the visual-viewport measurement.
There was a problem hiding this comment.
Thanks for the round-1 fixes, which check out; this pull request now waits on an engineering owner and still needs two changes.
Needs an owner decision: because it also changes Core, the CLI, the interaction harness and Storybook, it is not Lab-only, so TabBar's and SearchField's literal UIKit springs (which spec:AST-094 FR4 forbids), their behavior with no current spec, and minimizeBehavior and automaticallyActivates changing meaning by input device without the 'adaptive' value architecture:public-component-api INV3 requires all need an engineering owner to approve them as Lab direction or update the governing spec. An engineering owner records that decision by commenting /approve-spec <full head SHA>, and the author may do so as an engineering owner; landing the Lab code in Lab-only pull requests also resolves the hold.
Blocking changes:
- The diff against
mainis 19,956 authored lines, mostly from the four open stacked pull requests, so land the stack first, rebase, and then split this pull request's own 6,817-line commit or show why its parts cannot be separated under an approved direction. useTabBarLens.ts:817-845anduseTabBarMinimize.ts:478-500each create their ownResizeObserveron the capsule, while Core's sharedobserveResizealready owns resize observation and this pull request's SearchField uses it, so subscribe throughobserveResizeand delete both observers.
The inline notes on TabBarSearchContext and TabBar's JSDoc are non-blocking.
[Automated review]
| return; | ||
| } | ||
| let width = element.offsetWidth; | ||
| const observer = new ResizeObserver(() => { |
There was a problem hiding this comment.
This is a second resize observer on the capsule (useTabBarMinimize.ts keeps another). Core's observeResize (@astryxdesign/core) already owns resize observation, and SearchField in this PR uses it. Subscribe through observeResize(element, …) and drop this observer, so a fix to the shared observer reaches TabBar too.
| return; | ||
| } | ||
| let width = element.offsetWidth; | ||
| const observer = new ResizeObserver(() => { |
There was a problem hiding this comment.
Same as the lens: this watches the same capsule for the same width change. One observeResize subscription (here or shared with the lens) can keep both the trailing cap and the lens on the circle.
| /> | ||
| </div> | ||
| <div {...stylex.props(styles.searchFieldContent)}> | ||
| <TabBarSearchContext.Provider |
There was a problem hiding this comment.
Non-blocking: nothing reads TabBarSearchContext (SearchField never consumes it), and the value is a new object on every render. Delete the context and its provider, or have SearchField read it, and fix the comments at :989 and in tabBarSearchContext.ts that say the field reads it.
| // TabBar | ||
| // ============================================================================= | ||
|
|
||
| /** |
There was a problem hiding this comment.
Non-blocking: this JSDoc (with the example) is TabBar's, but it now sits above Layer's JSDoc and function Layer, so export function TabBar carries none. Move it to directly above export function TabBar.
3fe213b to
94dbca7
Compare
There was a problem hiding this comment.
This pull request waits on an engineering-owner decision and also needs changes.
Needs an owner decision. Because it also changes Core, the CLI, the interaction harness and Storybook, it is not Lab-only, so an engineering owner needs to rule on three things: TabBar's and SearchField's literal UIKit springs, which spec:AST-094 FR4 forbids (update FR4 or conform); minimizeBehavior and automaticallyActivates, which act only under a finger without the 'adaptive' value architecture:public-component-api INV3 requires; and the governing record for TabBar's and SearchField's behavior, or a request that these Lab components land in Lab-only pull requests. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner.
Changes needed:
- Land the stacked pull requests first and rebase so this diff is only its own commit (19,954 authored lines against
maintoday, above the 8,000-line owner-directed batch limit), then split it or show why its parts cannot be separated. - Replace the two component-level
ResizeObservers inuseTabBarLens.tsanduseTabBarMinimize.tswith Core's sharedobserveResize, which SearchField in this pull request already uses. - Export one badge helper from
TabBar.tsxand call it fromTabBarMoreList.tsxinstead of its copiedhasBadgeand badged-name code.
Non-blocking notes (a doubled search landmark, the unused TabBarSearchContext, and misplaced JSDoc and comments) are inline.
[Automated review]
| return; | ||
| } | ||
| let width = element.offsetWidth; | ||
| const observer = new ResizeObserver(() => { |
There was a problem hiding this comment.
This is one of two resize observers on the capsule (useTabBarMinimize.ts:488 keeps the other). Core's observeResize (@astryxdesign/core) already owns resize observation, and SearchField in this pull request uses it. Subscribe through observeResize(element, …) and drop this observer; its synchronous first call returns early here because the width is unchanged.
There was a problem hiding this comment.
Fixed in 52ecd8b. The lens subscribes through observeResize from @astryxdesign/core, and its observer is gone. The synchronous first call returns early on the unchanged width.
| return; | ||
| } | ||
| let width = element.offsetWidth; | ||
| const observer = new ResizeObserver(() => { |
There was a problem hiding this comment.
Same concern as the lens's observer: one observeResize subscription, here or shared with the lens, can keep both the trailing cap and the lens on the circle.
There was a problem hiding this comment.
Fixed in 52ecd8b, the same way: the trailing cap's watch is an observeResize subscription, so the capsule has no component-level observer left.
| }, | ||
| }); | ||
|
|
||
| function hasBadge(badge: string | number | undefined): boolean { |
There was a problem hiding this comment.
This copies hasBadge from TabBar.tsx:233, and lines 109-117 rebuild the badged name that TabBar.tsx:1452-1458 builds. Export one helper from TabBar.tsx beside overflowItems (for example badgeName(item, t)) and call it in both places, so a tab and its More row cannot disagree.
There was a problem hiding this comment.
Fixed in 52ecd8b. TabBar.tsx exports hasBadge and badgedName(item, t) beside overflowItems. The tab's name and the More row's name both come from badgedName, and the More list's copy is gone.
| {search != null ? ( | ||
| <div | ||
| ref={searchHostRef} | ||
| role="search" |
There was a problem hiding this comment.
Non-blocking: this host is a search landmark, and the placement="tab" SearchField inside it renders its own role="search" root, so assistive technology lists two search landmarks for one field. Keep one of the two roles for the tab placement.
There was a problem hiding this comment.
Fixed in 52ecd8b. The bar's host is no longer a landmark, so the SearchField's own root is the one search landmark. The children docs now name placement="tab", and the interaction spec checks there is exactly one search landmark. With the host's role back it finds two (2/2).
| /> | ||
| </div> | ||
| <div {...stylex.props(styles.searchFieldContent)}> | ||
| <TabBarSearchContext.Provider |
There was a problem hiding this comment.
Non-blocking: nothing reads TabBarSearchContext (SearchField never consumes it), and the value is a new object on every render. Delete the context and its provider, or have SearchField read it, and fix the comments at :989-990 and in tabBarSearchContext.ts.
There was a problem hiding this comment.
Deleted in 52ecd8b, with its provider, its file and the comments that named it.
| // TabBar | ||
| // ============================================================================= | ||
|
|
||
| /** |
There was a problem hiding this comment.
Non-blocking: this JSDoc, with its example, is TabBar's, but it sits above Layer's JSDoc and function Layer, so export function TabBar carries none. Move it to directly above export function TabBar.
There was a problem hiding this comment.
Moved in 52ecd8b: TabBar's JSDoc now sits directly above export function TabBar.
| backdropFilter: 'blur(16px) saturate(180%)', | ||
| boxShadow: shadowVars['--shadow-med'], | ||
| }, | ||
| // The bar never collapsing, the glass is one piece. |
There was a problem hiding this comment.
Non-blocking: this line describes capsuleGlass (:365), not platterStart, and the comment at :357 describes capsuleMinimized (:371), not capsuleAlone. Move each above the style it describes.
There was a problem hiding this comment.
Moved in 52ecd8b: each comment now sits above the style it describes, and the glass one is reworded.
… reads, and its strings join the catalog Re-exports only, no new behavior: @astryxdesign/core/internal/touch adds the pan recognizer, the press tracker, the action-group tracker, the finger-scroll reader, the text-selection lock, the touch query, the rubber band and UIKit's gesture values, none under a public factory's name. Core's message catalog gains TabBar's three strings: the More tab's label, the search's Cancel button and a badged tab's name. They land ahead of the Lab component, so the Lab batches change only Lab. The entry stays outside the public API: no docs, no root re-export, and the @astryx/no-core-internal-import rule keeps it to Core and Lab. One type widens: useFingerScroll also takes a ref that holds a document. It already watched a document passed to it directly, so nothing runs differently. A component can then fill the ref from its own element and watch the page of the document that element is in.
… scene's own time A proof that must touch a component mid-animation, or sample an animation from its start, races the test process: on a loaded runner the slide has moved on, or ended, before the next command reaches the page. The page freeze stops the scene instead. Every animation pauses where it stands, and timers and frame callbacks wait in the timer gate. The page's clocks stay at the freeze instant: performance.now(), Date.now(), event times, frame times and the animation timeline (new Date() keeps the real clock). Timeouts the page set before the freeze stop counting too, and come due on the page's clock after the thaw. Only the document timeline's animations are held: a scroll- or view-driven one follows its scroller. Input still arrives. Thawed, the scene plays on from where it stood and the held timers run in the order they came due. - installPageFreeze (before the page loads, then installTimerGate), freezePage, thawPage. - freezePageOn(page, type): the install adds one window capture listener for each pointer, touch, mouse, key, wheel and scroll event before any page script runs, and the call arms it, so the freeze lands in the input's own task, before any page listener for it. - freezePageWhen(page, predicate, arg): freezes on the first real frame the predicate holds, and rejects after its timeout, which runs on the real clock. - advanceFrozenPage(page, ms): the page's clock and every held animation move on together by ms; timers stay held. The timer gate gets freeze() and thaw() methods, which freezeTimers, thawTimers and the page freeze all call, and the real clock: the harness's own waits (sampleRects, armHold, waitForRest, startBusyControl, blockMainThread) time themselves on it, so they still run on a frozen page. The freeze exposes the frozen span. The curve recorder keeps real times, each page time with the span in force when it was recorded, and the harness adds the current span only to live page times, so after a thaw its epoch stamps share the trace's clock. An interval skips its ticks while frozen. A CSS animation the page resumes while frozen is held where it stood at the freeze, and a run the page cancels while frozen (a reversed CSS transition) stays cancelled at the thaw. At the thaw every held animation, one whose start the page wrote while frozen included, resumes from the time it held on the timeline's own time, so it is drawn where it stood in the thaw frame. The frozen span ends at that same frame time, so the page's clock and its runs stay together across freezes. Self-tests in Chromium and WebKit: a frozen scene stands still and thaws from where it stood; an armed freeze lands before the page's own listener (against a control where that listener runs first and a late read finds the run well along); an advance moves the clock and the held animation by its step while timers stay held; a freeze on a condition stops on the first frame it holds and times out otherwise, on a frozen page too; a start the page dates while frozen resumes exactly where it held in the thaw task; a timeout pending across a freeze comes due on the page's clock; a scroll-driven animation follows its scroller while frozen; the harness samples, arms a hold and blocks the main thread on a frozen page; a CSS animation resumed while frozen holds its time; a condition that throws rejects freezePageWhen; a CSS transition reversed while frozen plays its reversal after the thaw; after a thaw blockMainThread, waitForAnimation and sampleRects stamp on the real clock; a CSS animation dated and then removed while frozen stays removed; a run that finished before a freeze is dated where it ran when read after the thaw; the page's clock and its runs stay together across ten freeze and thaw cycles; a page without the freeze refuses one. Opt-in: a page that never calls installPageFreeze keeps its own clock. It lands ahead of its first user, TabBar's interaction proofs, so the Lab batches change only Lab.
… and press tracking TabBar shows an app's top-level sections as iOS 26's floating capsule above the bottom edge, with a selection lens that slides to the chosen tab. Each TabBarItem has an icon, a label and an optional badge. Under a finger, on iOS and Android alike, the lens moves on the springs measured from UIKit on iOS 26.2: it lifts into glass as a finger lands, follows a finger dragged across the tabs and settles on the tab nearest the lift, while the bar flexes. Under a mouse or a pen a click chooses a tab and the lens follows the theme's motion; a press dragged across the tabs carries the lens on the finger's tracking spring, and its release settles the lens from the drag's own speed. Under every pointer, sliding off the bar cancels: a press lifted off it chooses nothing. Choosing the selected tab again is reported apart, so a screen can return to its root. Core's action-group tracker owns the press: the tab under it, and running that tab at the lift. TabBar gives it the target rule and draws the lens. TabBar's UIKit values live beside it in uikitTabBar.ts, each with its source, and uikitTabBar.test.ts checks that every value names one. Core's gesture and motion pieces come through Core's internal touch entry.
…d holds a search tab and a More tab With minimizing on, the bar collapses into a small circle as the page scrolls down under a finger and expands at the top or on a tap, and a bottom accessory moves from above the bar to inline beside the circle. A disabled tab takes no choice, and a finger landing on one moves nothing until it drags. A search tab sits apart at the trailing end: chosen, it turns into a search field that rises above the keyboard, measured from the visual viewport, with a Cancel button that ends search. Past four tabs a More tab holds the rest, which TabBarMoreList lists. Each motion plays on the springs measured from UIKit on iOS 26.2, from TabBar's own table.
94dbca7 to
52ecd8b
Compare
|
Round 2 is addressed in 52ecd8b, restacked on #9399's head 7b86fe2 and on #9700's round:
|
There was a problem hiding this comment.
This pull request waits on an engineering-owner decision and needs one change.
Owner decision: because it also changes Core, the CLI, the interaction harness and Storybook, it is not Lab-only, so TabBar's and SearchField's literal UIKit springs and frame tables need an engineering owner to approve them as Lab direction against spec:AST-094 FR4 ("A component MUST read touch motion through these roles and MUST NOT play a literal"), amend FR4, or ask for motion roles, recorded by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner).
Change needed:
- Land the four stacked pull requests and rebase so this diff is only its own commit: it is 20,147 authored lines against
maintoday, above the 8,000-line owner-directed batch limit, and this commit alone is 6,795.
The round-2 fixes check out, the earlier INV3 point is withdrawn because architecture:public-component-api reaches Lab only through INV23, and two non-blocking notes are inline.
[Automated review]
| name: 'automaticallyActivates', | ||
| type: 'boolean', | ||
| description: | ||
| 'Whether choosing the search tab activates its field at once. When false, the field waits inline in the bar until it is tapped.', |
There was a problem hiding this comment.
Non-blocking: under a mouse the search tab always activates its field (TabBar.tsx:1135, !isTouch || (search.automaticallyActivates ?? true)), so false is ignored there. Say so here and in the JSDoc at TabBar.tsx:187-189, as minimizeBehavior's docs already do ("Only under a finger").
There was a problem hiding this comment.
Taken: both the doc and the JSDoc will say it acts only under a finger, and that under a mouse the search tab always activates its field. It lands with the rebase push after #9399 merges.
| return; | ||
| } | ||
| const isRtl = isRtlElement(element); | ||
| // The trailing cap's travel onto the leading one. |
There was a problem hiding this comment.
Non-blocking (INV29): this comment explains a name. Rename travel to capTravel and drop the comment.
There was a problem hiding this comment.
Taken: travel becomes capTravel in both places, and the comment goes. It lands with the rebase push after #9399 merges.
Stacked on #9700. Review only this PR's change: vjeux/astryx@6f87bcd...52ecd8b
User need
On iOS 26 a tab bar gets out of the way as the page scrolls, holds a bottom accessory such as a mini player, keeps search one tap away, and folds extra sections into a More tab. On Astryx each is built by hand, without UIKit's motion.
Evidence
Recorded from the drafts this replaces (iOS 26.2 simulator, Android emulator). Since then the field reads the keyboard from the visual viewport only, and a drag over a disabled tab returns the lens to the selected tab.
Problem and solution fit
minimizeBehavior="onScrollDown": a finger's downward scroll collapses the bar into a circle; the top or a tap expands it.bottomAccessory: content above the bar that moves beside the circle.isDisabledon an item: visible, never chosen.TabBarSearchTab: a circle whose field rises above the keyboard, the circle then Cancel.moreValueandTabBarMoreList: past five tabs, four plus More.Current specification
astryx discoverlists TabBarSearchTab and TabBarMoreList becauseofficial-packages.jsonis regenerated from the repository, asspec:AST-051FR11 requires.Owner decisions
minimizeFadeis a guess (@measure M-TB1) until measured.spec:AST-094FR4).family:action-groupsFR2).architecture:public-component-apiINV3:minimizeBehaviorandautomaticallyActivatestake effect only under a finger (a mouse keeps the bar expanded and always activates the field), with no'adaptive'value. Their names wait on this decision.Also: each glass (accessory, search field) is three elements: an
aria-hiddencontaining block that draws the start cap, and two pieces the motion animates by ref. The finger-only code is 6.5 KB gzip of TabBar's 12.6 KB.Public delta and caller burden
New Lab exports
TabBarSearchTabandTabBarMoreList;TabBargainsminimizeBehavior,bottomAccessoryandmoreValue, andTabBarItemgainsisDisabled. All optional; a bar without them is unchanged.Design and alternatives
Scope
Testing
Local gates at 52ecd8b (on the first batch, over #9399's 7b86fe2), node 24.22.1 from
.nvmrc:Still owed before ready: the WebKit CI profile ×10 and the feel check on a phone.
Batch: L1 · Lab: iOS chrome (TabBar, SearchField, NavigationStack, PageControl)
Size: 6,600 added lines, about half tests.
Supersedes #8154, #8172, #8192, #8210, #8222, #8243 and #8248.
Stack: #9399 → #9699 → #9698 → #9700 → this PR.