Repository navigation
architecture(layer-runtime): one generic layer runtime, and one opt-in lift-from-trigger module (INV14, INV15) - #8448
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:spec-owner-review
meta-cla
Bot
added
the
CLA Signed
cixzhang
changed the title
PR Analysis Report
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This proposal can't be approved yet. INV14 and INV15 add new invariants to the current layer-runtime record, so an engineering owner needs to decide them, including whether INV14 is meant to cover the native-dialog surfaces and ToastViewport. They record that decision by commenting /approve-spec <full head SHA> on this pull request, and the author can do this as an engineering owner. Whatever the decision, these corrections are still needed:
- Narrow INV14, or amend the claims it contradicts: INV9 and the Owning code lines in this record, the overlay-dismissal family's AV3/AV4/AV6, and AST-003 FR10.
- Reconcile INV15's clone option with the single-copy rule in component-style-authoring INV7, and state that any copy is inert and hidden from assistive technology.
- Replace the new ContextMenu gap bullet, because ContextMenu does not lift on main yet; its lift is still pending implementation.
- Remove the "Proposed; pending owner decision" markers, update the record's shipped-only scope sentence, and define the presentation hold and the shared rectangle morph that the new invariants rely on.
[Automated review]
| - **INV14 — The runtime owns every layer's shared mechanics.** **Proposed; | ||
| pending owner decision.** Internally, the layer runtime owns top-layer | ||
| mounting, dismissal, the presentation hold, the one shared exit copy every | ||
| layer uses (the single Core mechanism `architecture:component-style-authoring` | ||
| INV7 names), and the backdrop tint, drawn with `::backdrop`. No component | ||
| mounts its own top-layer host, exit copy, or backdrop element. The public | ||
| surface stays `useLayer` and `LayerProvider`. |
There was a problem hiding this comment.
As worded, INV14 conflicts with claims this record keeps: INV9 (outside channels stay component-owned) and the Owning code lines that give Dialog families their backdrop presentation and Drawer its showModal()/showPopover() hosting. It also conflicts with the overlay-dismissal family's AV3/AV4/AV6 and AST-003 FR10. Dialog, Drawer, BottomSheet, Lightbox, MobileNav and ToastViewport mount their own top-layer hosts today. Either scope INV14 to layers mounted through useLayer, with "dismissal" meaning the Escape and platform-close routing INV8 names, or amend those claims and list these surfaces as gaps.
| internal module separate from the layer runtime: the lift from the trigger, | ||
| using the real trigger or a clone, whichever meets the performance outcomes; |
There was a problem hiding this comment.
A trigger clone held while the menu is open mounts the trigger's content twice. component-style-authoring INV7 allows a second copy only as the transient copy during an exit, a presentation switch or a sheet handoff. Nothing here says the clone is inert or hidden from assistive technology, and "the performance outcomes" are not defined anywhere in the record.
| - Under the proposed INV14 and INV15, ContextMenu's touch lift, the one surface | ||
| that lifts today, grows its menu | ||
| with its own copy of the rectangle morph, and its trigger clone, manual | ||
| popover, and `::backdrop` mount are ContextMenu-local rather than shared. |
There was a problem hiding this comment.
On main, ContextMenu does not lift yet: its record marks the lift as pending implementation, and there is no lift, clone or ::backdrop in its source. This list holds observable facts, and the paragraph below hands its gaps to AST-003, which does not cover lifts.
| | INV8 | `useLayerDismissal.test.tsx`, `layerDismissalInvariants.test.tsx`, and `useFocusTrap.test.tsx` | Current top registered layer is skipped, two layers close, or a blocker leaks through | | ||
| | INV9 | Representative Dialog, ContextMenu, Tooltip/HoverCard, and BottomSheet source/tests | A current local channel silently changes ownership or policy | | ||
| | INV10 | `LayerProvider.tsx`, `LayerProvider.test.tsx`, `useToast.tsx`, `ToastViewport.test.tsx`, and `ToastViewport.modalHost.test.tsx` | Provider begins relocating ordinary layers, Toast fallback loses its current lifecycle, or a default provider renders differently from no provider | | ||
| | INV14, INV15 (proposed) | Source boundary check over Core for top-layer hosts, exit copies, backdrop elements, lift, and rectangle-morph code outside the runtime and the lift module | A component mounting its own top-layer host, exit copy, or backdrop element, or re-implementing the lift or the grow | |
There was a problem hiding this comment.
No source boundary check like this exists in the repository yet, and verified_by is unchanged; one that did what this row describes would fail today on the dialog-style surfaces. Together with the "Proposed; pending owner decision" markers, this row would merge into a current record that still describes INV14 and INV15 as pending.
cixzhang
changed the title
|
Posted by Cindy's agent on her behalf. Cindy's decision (2026-10-25): approve. The layer runtime owns the shared mechanism every layer uses (top-layer mounting, close-request delivery, the presentation hold, the one exit copy, the |
|
/approve-spec 29dc337 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
The owner-approved direction for INV14 and INV15 is clear, but three corrections are needed before this record can merge:
- INV15 calls the trigger clone the transient copy that component-style-authoring INV7 allows, while INV7 allows only one copy during an exit, a presentation switch or a sheet handoff, made by one mechanism every layer uses; amend INV7 here or keep the real trigger.
- The record still says it describes only the shipped runtime, yet no presentation hold, exit copy, runtime backdrop tint or lift module exists today, and Dialog, Drawer, BottomSheet, Lightbox, MobileNav, ToastViewport and Selector build their own; update that sentence and list these under Current gaps.
- The runtime-owned backdrop tint and top-layer mounting still have component owners in the overlay-dismissal family's AV6, this record's boundary sentence and its Owning code; amend those lines or narrow INV14.
A new commit invalidates the exact-head approval, so an engineering owner (the author included) will need to comment /approve-spec <full head SHA> again on the corrected head.
[Automated review]
| trigger is the one transient copy `architecture:component-style-authoring` | ||
| INV7 allows: inert, hidden from assistive technology, and removed when the |
There was a problem hiding this comment.
component-style-authoring INV7 allows only one transient copy "during an exit, a presentation switch, or a handoff between sheets", made "by one Core mechanism that every layer uses". INV14 already names the runtime's exit copy as that mechanism, and this module is separate from the runtime and used only by lifting surfaces. Amend INV7 in this pull request to admit the lift clone, or keep the real trigger; "the performance outcomes" that choose between them are not defined anywhere.
| `architecture:component-style-authoring` INV7 names), and the backdrop tint, | ||
| drawn with `::backdrop`. A component that needs one of these uses the | ||
| runtime's rather than building its own. Each component still decides when it |
There was a problem hiding this comment.
The runtime now owns the backdrop tint and top-layer mounting, but the overlay-dismissal family's AV6 ("Members own animation, visual treatment, and theming anatomy"), this record's boundary sentence ("It does not own ... visual treatment") and its Owning code ("Dialog families own native modal/backdrop presentation"; "Drawer owns modal showModal() and non-modal showPopover() hosting") still give those to components. Except the runtime's tint and mounting there, or narrow this claim.
| | INV8 | `useLayerDismissal.test.tsx`, `layerDismissalInvariants.test.tsx`, and `useFocusTrap.test.tsx` | Current top registered layer is skipped, two layers close, or a blocker leaks through | | ||
| | INV9 | Representative Dialog, ContextMenu, Tooltip/HoverCard, and BottomSheet source/tests | A current local channel silently changes ownership or policy | | ||
| | INV10 | `LayerProvider.tsx`, `LayerProvider.test.tsx`, `useToast.tsx`, `ToastViewport.test.tsx`, and `ToastViewport.modalHost.test.tsx` | Provider begins relocating ordinary layers, Toast fallback loses its current lifecycle, or a default provider renders differently from no provider | | ||
| | INV14, INV15 | Source boundary check over Core for top-layer hosts, exit copies, backdrop elements, lift, and rectangle-morph code outside the runtime and the lift module | A component building its own copy of a mechanism INV14 lists (top-layer mounting, close-request delivery, hold, exit copy, or backdrop element), or re-implementing the lift or the grow | |
There was a problem hiding this comment.
This failure signal is met by shipped code as soon as it merges: Dialog, Drawer, BottomSheet, Lightbox and MobileNav draw their own ::backdrop tint, Selector keeps its own presentation hold, and six surfaces call showModal() or showPopover() themselves. Meanwhile the record still says it describes only the shipped runtime and lists none of these under Current gaps.
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 0acc952 |
github-actions
Bot
removed
the
needs:spec-owner-review
|
/approve-spec f7567a7 |
There was a problem hiding this comment.
The owner-approved direction for INV14 and INV15 is clear, and the INV14/INV15 gap now sits outside the AST-003 paragraph, but one wording correction is needed before this record can merge. INV14 ends "The public surface stays useLayer and LayerProvider", while the published @astryxdesign/core/Layer entry also exports useLayerDismissal (the close-request delivery INV14 calls internal), LayerDepthProvider, useTouchTrigger, isActionTrigger, LayerContext, useLayerContext and layerAnimations, which the public-component-api record counts as public contract; saying instead that these mechanisms add no public API and the published exports are unchanged fixes it. Two optional clarifications would help implementers: where the shared rectangle morph lives for surfaces that grow without lifting (AST-094 FR31), and which mechanism makes the lift clone. A new commit invalidates the exact-head approval, so an engineering owner (the author included) will need to comment /approve-spec <full head SHA> again on the corrected head.
[Automated review]
| `family:overlay-dismissal` AV3, AV4, AV6; `spec:AST-003` FR10). The public | ||
| surface stays `useLayer` and `LayerProvider`. |
There was a problem hiding this comment.
The published @astryxdesign/core/Layer entry (packages/core/package.json, ./Layer) also exports useLayerDismissal (the close-request delivery this invariant calls internal), LayerDepthProvider, useTouchTrigger, isActionTrigger, LayerContext, useLayerContext and layerAnimations, and the public-component-api record counts published subpaths and exported hooks as public contract. Saying "These mechanisms add no public API; the runtime's published exports are unchanged" keeps the claim true.
… shared mechanics, and one presentation module owns lift and grow (INV14, INV15)
…trigger module, opt-in per surface
…t copy; non-lifting layers do not use the module
…ft clone is INV7's one inert copy; drop the not-yet-shipped ContextMenu gap
…nts keep dismissal, hosting, and presentation policy
…visual treatment to components; the record lists INV14/INV15's current gaps
… list; the lift's choice is compositor-only motion
f7567a7 to
d795cd8
Compare
|
/approve-spec d795cd8 |
There was a problem hiding this comment.
This looks ready to merge: INV14 and INV15 carry engineering-owner approval for this exact head, and they conflict with none of the current records they touch. One optional clarification would help implementers: say where the shared rectangle morph lives, because Popover and DropdownMenu must grow from their trigger without lifting, while INV15 keeps layers that do not lift out of the lift module. If you add it, an engineering owner needs to comment /approve-spec <full head SHA> again on the new head.
[Automated review]
| internal module separate from the layer runtime: the lift from the trigger, | ||
| using the real trigger or a clone, whichever keeps the motion on the | ||
| compositor (`spec:AST-094` FR5); | ||
| the grow from the trigger's rectangle through the shared rectangle morph |
There was a problem hiding this comment.
Optional clarification: say where the shared rectangle morph lives. Popover must grow from its trigger without lifting (Popover FR9, from the touch-presentations spec's FR31), INV15 keeps layers that do not lift out of this module, and the Verification row fails rectangle-morph code outside the runtime and the lift module, so the runtime is its only conforming home.
Exact decision needed
Does the layer runtime own every layer's shared mechanics, and does lift-from-trigger live in one separate, opt-in module?
Existing authority searched
architecture:layer-runtimeINV1–INV12 (current): hosting, positioning, native close reconciliation, Escape delivery; no shared exit copy, hold, backdrop, or presentation module.architecture:component-style-authoringINV7 (current): one transient copy made by one Core mechanism every layer uses; until it exists, a layer's own copy is a gap.spec:AST-017FR21 (current, as merged in spec(AST-017): no file path or subpath makes an API experimental #8357): modules without a public contract stay internal.component:PopoverDEC-6 (current, spec(Popover): presentation is popover, bottom-sheet, or adaptive, following the menus (DEC-6) #8440): Popover's presentation prop.placeLayerExitCopy). feat(Layer): one input hold, first used by Dialog, Drawer and MobileNav (spec:AST-094 FR35) #8342 (open): the presentation hold.Decision
architecture:layer-runtimeINV14 and INV15 (Cindy, 2026-10-25, 13:18 placement; approved 14:20):::backdrop. A component that needs one uses the runtime's, never its own. Each component still decides when it dismisses, how it is hosted, and how it is presented (INV9; overlay-dismissal AV3, AV4, AV6; AST-003 FR10). Public surface staysuseLayerandLayerProvider.Current-state evidence
Lane 4's audit of the ContextMenu lift (#8201):
ContextMenuLift.tsx:257-273duplicates the rect morph for the menu grow; the clone plus manual popover plus::backdropmount (:281,:399-405) are ContextMenu-local.Rejected alternatives
Claim scope and non-goals
Internal ownership only. No public API changes. Lab's access path to internal modules is a separate, pending decision.
Writing rubric results
Scope
Validation
pnpm check:knowledge(CI lint)git diff --check