Repository navigation
Conversation
… a runtime budget; long lists skip offscreen rows
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:spec-owner-review
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
# Conflicts: # docs/architecture/react-component-runtime.md
# Conflicts: # docs/architecture/layer-runtime.md
![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 engineering owner's decision on the three requirements this pull request adds to current records with Decider: pending: architecture:layer-runtime INV20 and DEC-2, and architecture:react-component-runtime INV19 with DEC-1 and INV20 with DEC-2. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. These fixes stay required after the decision:
- Layer-runtime INV20 makes Tooltip mount its layer on first show, but the trigger's
aria-describedbynames that layer's id, so before first show it names no element, againstcomponent:TooltipAR1; keep the description element mounted or amend AR1. - Layer-runtime INV20 contradicts
component:Popover's rule that the default context layer keeps its mounted host while closed; amend that record or drop theusePopoversurfaces from INV20. - Layer-runtime INV20 ships only for ContextMenu and runtime INV20 for no list, yet neither is listed as an implementation gap, and DEC-2 and DEC-1 state the adoption as done ("Core surfaces now use it", "now meet one measured budget") although nothing measures INV19; list the unmet rules as gaps and drop the adoption claims.
- INV20 and the description say it uses
useLayer'slazyMount, but publiclazyMountunmounts on every hide while keep-after-show is the internalkeepsMountAfterShow, and "Every Core surface built onuseLayerin context mode" lists only eight of them; name the option and say which surfaces change and how. - Runtime INV19 lists "Checkbox" and "Radio", which Core does not export; name the real components (for example
CheckboxListItemandRadioListItem). - Runtime INV20 applies
content-visibilityto "Grid rows", butcomponent:Gridowns no content or rows; drop Grid or name the intended collection. - The description has no
## Writing rubric results, whicharchitecture:knowledge-contractsrequires of a pull request that changes a record.
INV20 also keeps Selector's list mounted after its first open, while open #9586 removes it on every close, so the decision should settle which one governs Selector.
[Automated review]
| - **INV18 — Only a shown layer's trigger is an anchor.** | ||
| A trigger carries its CSS anchor name only while its layer is shown; a closed trigger is not an anchor, so a surface that moves around it keeps its transform animation off the main thread. | ||
| A layer that animates out through a CSS transition or keyframe animation it declares for its closed state counts as shown until those animations end; an animation a script starts is not tracked. | ||
| - **INV20 — A closed layer costs nothing until first shown.** Every Core |
There was a problem hiding this comment.
Mounting Tooltip's layer on first show leaves the trigger's aria-describedby naming no element until then (the id belongs to the layer, and a closed lazy layer renders only an empty marker), which contradicts component:Tooltip AR1. It also contradicts component:Popover's rule that the default context layer keeps its mounted host while closed. Keep Tooltip's description element mounted or amend AR1, and amend the Popover record or drop the usePopover surfaces.
| DropdownMenu, ContextMenu, Selector's and MultiSelector's lists, and the | ||
| date pickers) mounts its layer on its first show, not with its trigger, and | ||
| keeps it mounted after that first show. A closed trigger renders only the | ||
| trigger. This uses `useLayer`'s existing `lazyMount` internally and adds no |
There was a problem hiding this comment.
Public lazyMount unmounts the layer on every hide; staying mounted after the first show is the internal keepsMountAfterShow option that ContextMenu sets. Name that option here, and say whether the surface list is exhaustive: Carousel, Tokenizer, the keyboard hint, DropdownMenu's submenu and other usePopover callers are context-mode layers too.
|
|
||
| A person opening a table where each row has a few tooltips waits seconds, | ||
| because every tooltip mounts its layer with its trigger. `useLayer` already | ||
| supports mounting on first show. Core surfaces now use it, so a page pays for a |
There was a problem hiding this comment.
Only ContextMenu does this today, so "Core surfaces now use it" states unshipped work as done. List INV20 as an implementation gap (the record's opening lists the unmet invariants) and describe the system as it is.
| about 49 Core source files that call `getBoundingClientRect` are not yet | ||
| checked against this rule.) | ||
| - **INV19 — Repeated controls meet a runtime budget.** A control that pages | ||
| repeat in large numbers (CheckboxInput, Checkbox, Radio, Switch, Button, |
There was a problem hiding this comment.
Core exports no Checkbox or Radio component (it has CheckboxInput, CheckboxListItem and RadioListItem), so the budget cannot be verified for these names. Name the real components.
| of main-thread work with no task over 100 ms. Such a control runs no layer, | ||
| tooltip, or transition work for a feature that instance does not use (DEC-1). | ||
| - **INV20 — Long lists render rows near the viewport.** List, ChatMessageList, | ||
| and Grid rows outside the viewport skip rendering through |
There was a problem hiding this comment.
component:Grid owns no content, surface, or structural page semantics, so it has no rows to style. Drop Grid or name the intended collection. INV20 is also unbuilt for List and ChatMessageList; mark it as an implementation gap.
|
|
||
| A team building a 50,000-cell permission matrix cannot use Astryx checkboxes, | ||
| because each one costs about 20 ms. Controls that pages repeat by the hundred | ||
| now meet one measured budget, and they skip work for features they do not use. |
There was a problem hiding this comment.
No harness measures this budget yet, so "now meet one measured budget" states unshipped work as done. Record INV19 as an implementation gap and keep the decision in the present tense about the rule.
Summary
Pages with hundreds of rows, checkboxes, or tooltips freeze for seconds. This draft makes three changes. Closed layers mount on first show, using
useLayer's existinglazyMountinternally. Controls that pages repeat meet one measured runtime budget. Long lists skip offscreen rows withcontent-visibility. None of the three adds a prop.Spec explainer (human view): kept internally with the owner review.
Owner decisions (draft until decided; parked, not yet queued)
architecture:layer-runtimeINV20, DEC-2). Recommended A: mount on first show, then keep. Alternatives: B, a publiclazyMountprop; C, today.architecture:react-component-runtimeINV19, DEC-1). Recommended A: mounting or changing 100 instances stays under 300 ms of main-thread work with no task over 100 ms (Chromium, 4x CPU throttle, median of three). Alternatives: B, dense variants; C, no budget.content-visibility: autoon List, ChatMessageList, and Grid rows. Alternatives: B, a windowing API on every list; C, apps window themselves.Current-state impact
architecture:layer-runtime: INV20, a verification row, and DEC-2. Numbered to avoid draft docs(spec): a clip never cuts a focus indicator; content in a layer sizes against the layer #10124's INV19 and DEC-1.architecture:react-component-runtime: INV19, INV20, two verification rows, DEC-1, and DEC-2. Main added its own INV18.