Repository navigation
docs(spec): AppShell records its shipped contract as current - #8262
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
PR Analysis Report
The exact-head preview is not available yet. Generated by PR Enrichment workflow | View current CI run |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This record is on hold for an engineering-owner decision: it makes AppShell's shipped behavior current requirements (FR1–FR16, AR1–AR4, PR1–PR2, ORD1–ORD2) that no current spec states yet, and the spec-owner-approval check is still pending. Any engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner; the decision should settle in particular whether FR12's mobileNav={false} clause, which leaves the side nav unreachable below the breakpoint, should be a requirement while OQ1 is open. Before that approval, please correct the record:
- The verification map credits visual baselines with every FR7 variant, but AppShell's baselines render only
elevated, and FR12'smobileNav={false},contentandhasToggle: falsebranches and FR15 have no test, so narrow those rows or add the checks. - TopNav's modes are delegated to
component:TopNav, which does not exist, so say they are uncontracted. - FR10's "plus 50 ms" and PR1's "shared resize observer" name implementation details that
architecture:knowledge-contractsINV18 keeps out of product contracts, and FR10 is stricter than AV3. - FR3 should say the sidenav-only mobile top bar keeps
app-shell-headerbut dropsrole="banner"when a banner header renders, and FR8 should citespec:AST-012FR8 instead of restating it.
[Automated review]
| **Does not own / non-goals** | ||
|
|
||
| - How TopNav renders in its full, mobile-bar, and drawer modes — owned by | ||
| `component:TopNav`. AppShell only selects the mode. |
There was a problem hiding this comment.
No component:TopNav record exists at the head or on main (the navigation-destinations family lists only TopNav's sub-components), so this delegation points nowhere while FR9 and FR10 depend on these modes. Suggest stating that TopNav's render modes are uncontracted, as the record does for useAppShellMobile.
| | ---- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------ | | ||
| | FR1 | The root element carries `app-shell`, the forwarded `ref`, `data-testid`, and other DOM props, and contains, in order: the skip link, the Layout frame (header region, inline side region, content region), then any mobile drawer. | Current source and tests | | ||
| | FR2 | A slot is present when its value is renderable: not `null`, `undefined`, a boolean, or an empty string. A component that renders nothing still counts as present. | Current source and public types | | ||
| | FR3 | The header region renders only when `topNav` or `banner` is present, or for the sidenav-only mobile top bar (FR9). It carries `app-shell-header` and `role="banner"`, places `banner` above `topNav`, and is never inside another landmark. AppShell renders at most one banner landmark. | Current source and tests | |
There was a problem hiding this comment.
With banner and sideNav but no topNav below the breakpoint, the sidenav-only mobile top bar also carries app-shell-header but gets no role (AppShell.tsx:838-841), so the sentence does not hold for the top bar in that state. Suggest: the top bar takes role=banner only when no banner header renders, and both elements carry app-shell-header.
| | FR5 | The skip link is the first focusable element in the shell. It links to `#astryx-app-shell-main`, shows the translated `@astryx.appShell.skipToContent` text, is visually hidden until focused, and on activation moves focus to the main region. | WCAG 2.4.1; current source and tests | | ||
| | FR6 | `fill` sizes the root to `100dvh` and the inline side region and main region scroll on their own. `auto` gives the root a `100dvh` minimum, makes the header sticky at the block start, keeps the inline side region sticky below the measured header height with its own scrolling, and lets the page scroll. Header height is measured only in `auto`. | Current source and tests | | ||
| | FR7 | Variant backgrounds: `wash` paints body background on the frame, nav areas, and content; `surface` paints surface on all three; `section` paints surface on the frame and divides the nav areas from content, and in `auto` the header paints surface itself; `elevated` paints body background on the frame and nav areas and surface on content (FR16 for the corner). | Current source and tests | | ||
| | FR8 | The mobile layout applies while the viewport width is below the effective width of `mobileNav.breakpoint` (default `md`); equality is the wider layout; `none` never applies it. `defaultIsMobile` seeds the first render, and is ignored for `none`. The width map comes from the nearest Theme as `spec:AST-012` FR8 and `architecture:theme-application` INV10 state. | `spec:AST-012` FR8; tests | |
There was a problem hiding this comment.
This restates spec:AST-012 FR8 (exclusive edge, none, nearest Theme, SSR hint), which line 241 says AST-012 owns. Keeping only AppShell's own facts (default md, defaultIsMobile ignored for none) and citing AST-012 FR8 avoids two current records stating one rule (knowledge-contracts INV2, INV9).
| | FR7 | Variant backgrounds: `wash` paints body background on the frame, nav areas, and content; `surface` paints surface on all three; `section` paints surface on the frame and divides the nav areas from content, and in `auto` the header paints surface itself; `elevated` paints body background on the frame and nav areas and surface on content (FR16 for the corner). | Current source and tests | | ||
| | FR8 | The mobile layout applies while the viewport width is below the effective width of `mobileNav.breakpoint` (default `md`); equality is the wider layout; `none` never applies it. `defaultIsMobile` seeds the first render, and is ignored for `none`. The width map comes from the nearest Theme as `spec:AST-012` FR8 and `architecture:theme-application` INV10 state. | `spec:AST-012` FR8; tests | | ||
| | FR9 | In the mobile layout the side nav leaves the inline region. A present `topNav` renders in TopNav's mobile-bar mode, given the side nav content for its drawer, unless `mobileNav` is `false` or a node. With `sideNav`, no `topNav`, `mobileNav` not `false`, and `hasToggle` not `false`, a mobile top bar renders the side nav in SideNav's top-bar mode beside the auto toggle, in a navigation region named by `@astryx.appShell.mobileNavigation`. | Current source and tests | | ||
| | FR10 | The auto drawer renders only in the mobile layout and only when `mobileNav` is not `false`, not a node, and has no `content`. Without `topNav` it is SideNav in drawer mode; with `topNav` it is TopNav in drawer mode carrying the side nav content; with a heading-only TopNav the side nav items render exactly once. It stays mounted while closed and is hidden after a close only once MobileNav's longest close delay plus 50 ms has passed (immediately under reduced motion); an open is visible in the commit that carries it. | Current source and tests | |
There was a problem hiding this comment.
The plus 50 ms is ACTIVITY_HIDE_BUFFER_MS (AppShell.tsx:97), a race margin for React's Activity boundary, and it is stricter than AV3's as long as the hold outlasts that delay. knowledge-contracts INV18 keeps internal mechanisms out of product requirements; suggest is not hidden before MobileNav's close delay has elapsed.
| ### Performance and resources | ||
|
|
||
| - **PR1 — Header measurement.** AppShell observes the header's size only in | ||
| `auto`, through the shared resize observer, and observes nothing in `fill`. |
There was a problem hiding this comment.
through the shared resize observer names an internal utility (observeResize); the observable constraint is that AppShell measures the header only in auto and observes nothing in fill.
| | FR9–FR12 | `AppShell.test.tsx` mobile suites; `MobileNavCloseEdgeCases.test.tsx` AppShell drawer suites | Sidenav only; both navs; heading-only TopNav; custom node | A missing toggle, duplicated items, a stale drawer, or an early hide fails. | `audit:AppShell/behavior` | | ||
| | FR13, AR3 | `AppShell.test.tsx` keyboard drawer and `useAppShellMobile` suites | Outside a shell; inside, closed and open | A wrong id link or stale state fails. | `audit:AppShell/accessibility` | | ||
| | FR14, AR4 | `AppShell.test.tsx` breakpoint continuity test | Desktop to mobile and back with a focused input | Remounting content loses the value or focus. | `audit:AppShell/behavior` | | ||
| | FR7, FR16 | `AppShell.stories.tsx` and visual baselines | Each variant; elevated with both navs | A changed background or missing corner shows in visual review. | `audit:AppShell/visual` | |
There was a problem hiding this comment.
The AppShell visual baselines (core-appshell--playground, --full-featured, --with-mobile-nav) all render the default elevated variant, so Each variant has no check that would fail for wash, surface or fill-height section. FR12's mobileNav={false}, content and hasToggle: false branches and FR15 also have no test. Suggest narrowing this row to elevated and marking the rest source-only, or adding the baselines and tests.
There was a problem hiding this comment.
This record is on hold for an engineering-owner decision: it makes AppShell's shipped behavior current requirements (FR1–FR16, AR1–AR4, PR1–PR2, ORD1–ORD2) that no current spec states yet, and the spec-owner-approval check is still pending. Any engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner; the decision should settle in particular whether FR12's mobileNav={false} clause, which leaves the side nav unreachable below the breakpoint, should be a requirement while OQ1 is open. Thanks for the corrections so far; before approval, please also fix:
- FR9 says the sidenav-only mobile top bar renders beside the auto toggle whenever
mobileNavis notfalse, but with a drawer node the shell renders that top bar without a toggle, becauseMobileNavTogglerenders nothing while mobile navigation is disabled. - Layout regions, padding and region scrolling are owned by
family:layout-regions, notspec:AST-025; link it, and have FR14 cite its FR9, which already says AppShell keeps main content mounted across the breakpoint. - PR2 has no verification row and is missing from the source-only note under the verification map.
[Automated review]
| | FR6 | `fill` sizes the root to `100dvh` and the inline side region and main region scroll on their own. `auto` gives the root a `100dvh` minimum, makes the header sticky at the block start, keeps the inline side region sticky below the measured header height with its own scrolling, and lets the page scroll. Header height is measured only in `auto`. | Current source and tests | | ||
| | FR7 | Variant backgrounds: `wash` paints body background on the frame, nav areas, and content; `surface` paints surface on all three; `section` paints surface on the frame and divides the nav areas from content, and in `auto` the header paints surface itself; `elevated` paints body background on the frame and nav areas and surface on content (FR16 for the corner). | Current source and tests | | ||
| | FR8 | The mobile layout follows `mobileNav.breakpoint` under `spec:AST-012` FR8 and `architecture:theme-application` INV10; the breakpoint defaults to `md`, and `defaultIsMobile` is ignored when it is `none`. | `spec:AST-012` FR8; tests | | ||
| | FR9 | In the mobile layout the side nav leaves the inline region. A present `topNav` renders in TopNav's mobile-bar mode, given the side nav content for its drawer, unless `mobileNav` is `false` or a node. With `sideNav`, no `topNav`, `mobileNav` not `false`, and `hasToggle` not `false`, a mobile top bar renders the side nav in SideNav's top-bar mode beside the auto toggle, in a navigation region named by `@astryx.appShell.mobileNavigation`. | Current source and tests | |
There was a problem hiding this comment.
With a drawer node as mobileNav, AppShell still renders this sidenav-only top bar, but the toggle inside renders nothing: a node disables mobile navigation and MobileNavToggle returns null while it is disabled. Suggest saying the auto toggle renders only while mobile navigation is enabled (FR11), so a node gets the top bar without a toggle.
| - Layout regions, their padding, scroll containers, and keyboard access to an | ||
| overflowing region — owned by Layout (`LayoutHeader`, `LayoutPanel`, | ||
| `LayoutContent`) under `spec:AST-025`. |
There was a problem hiding this comment.
Layout regions, their padding and region scrolling are owned by family:layout-regions (its Shared owner list, FR3, FR4 and FR7); spec:AST-025 is the scroll-container capability and itself defers region scrolling to that family, keeping only keyboard access (FR12). Suggest naming the family as the owner here and in Family and system relationships.
| | FR11 | The drawer is closed by default and AppShell owns its state unless `mobileNav.isOpen` is defined. Every requested change calls `mobileNav.onOpenChange`. Open and toggle requests do nothing unless mobile navigation is enabled: a nav slot is present, `mobileNav` is not `false`, and `mobileNav` is not a node. | Current source and tests | | ||
| | FR12 | `mobileNav={false}` renders no drawer and no toggle; in the mobile layout the side nav then renders neither inline nor in a drawer. A node (element or string) renders as given at every width, replaces the auto drawer, and leaves TopNav in its full mode. A config object's `content` replaces the auto drawer in the mobile layout when a nav slot is present. `hasToggle: false` removes the auto toggle. | Current source and tests | | ||
| | FR13 | AppShell provides its subtree with: whether the mobile layout applies, whether the drawer is open, one drawer id shared by the drawer's `id` and the toggle's `aria-controls`, open, close, and toggle actions, whether mobile navigation is enabled, and whether auto toggles render. | Current source and tests | | ||
| | FR14 | Crossing the breakpoint does not remount the main region or its content: input values and focus inside `children` survive in both directions. | Current source and tests | |
There was a problem hiding this comment.
family:layout-regions FR9 already says "AppShell keeps main content mounted (state preserved) when crossing the mobile breakpoint." Suggest citing it here and keeping only what this record adds (input values and focus survive in both directions), so the two current records cannot drift apart.
| - **PR2 — One media query.** The breakpoint switch uses one media query | ||
| subscription per shell. |
There was a problem hiding this comment.
PR2 has no verification row and no test, and the source-only note under the verification map does not list it. Suggest adding it to that note or naming its evidence.
There was a problem hiding this comment.
This record is on hold for an engineering-owner decision: it makes AppShell's shipped behavior current requirements (FR1–FR16, AR1–AR4, PR1–PR2, ORD1–ORD2) that no current spec states yet, and the spec-owner-approval check is still pending. Any engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner; the decision should settle in particular whether FR12's mobileNav={false} clause, which leaves the side nav unreachable below the breakpoint, should be a requirement while OQ1 is open. Thanks for the corrections to FR9, the Layout delegation and PR2; before approval, please also fix:
- FR4 requires main to stay
tabIndex=-1and never be a tab stop, while the ownership boundary gives keyboard access to an overflowing region tospec:AST-025; limit FR4 to what the skip link needs, or record the gap under Current-state impact, so the open keyboard-scrolling fix (#8111) does not contradict it. - The source-only note under the verification map should also list FR9's drawer-node clause, FR11's
onOpenChangecallback and FR10's reduced-motion hold, which no cited test checks.
[Automated review]
| | FR1 | The root element carries `app-shell`, the forwarded `ref`, `data-testid`, and other DOM props, and contains, in order: the skip link, the Layout frame (header region, inline side region, content region), then any mobile drawer. | Current source and tests | | ||
| | FR2 | A slot is present when its value is renderable: not `null`, `undefined`, a boolean, or an empty string. A component that renders nothing still counts as present. | Current source and public types | | ||
| | FR3 | The header region renders only when `topNav` or `banner` is present; it carries `app-shell-header` and `role="banner"`, places `banner` above `topNav`, and is never inside another landmark. The sidenav-only mobile top bar (FR9) also carries `app-shell-header` and takes `role="banner"` only when no header region renders. AppShell renders at most one banner landmark. | Current source and tests | | ||
| | FR4 | `children` render in one region with `role="main"`, `id="astryx-app-shell-main"`, and `tabIndex=-1`. Its padding is `contentPadding`, default `0`. Programmatic focus on it draws no focus ring because it is never a sequential tab stop. | Current source and tests | |
There was a problem hiding this comment.
FR4 pins main to tabIndex=-1 and says it is never a sequential tab stop, while lines 107–109 hand keyboard access to an overflowing region to spec:AST-025, whose FR12 requires effective scroll content to stay keyboard reachable. In height="fill" main is that region, and the open fix in #8111 makes it a tab stop while it overflows; as worded, FR4 would forbid it. Suggest keeping only what the skip link needs (programmatically focusable, no ring on programmatic focus) and leaving tab-stop behavior to AST-025 FR12, or recording the gap under Current-state impact.
| The `wash`, `surface`, and fill-height `section` backgrounds in FR7, FR12's | ||
| `mobileNav={false}`, `content`, and `hasToggle: false` branches, FR15, and PR2 | ||
| are read from source; no focused test or baseline checks them yet. |
There was a problem hiding this comment.
Three more claims have no test in either cited file: FR9's "with a drawer node the top bar has no toggle" (the node test only counts dialogs), FR11's mobileNav.onOpenChange call (every onOpenChange in the AppShell suites belongs to a MobileNav node or is never asserted), and FR10's reduced-motion hold (the reduced-motion case renders MobileNav alone). Suggest adding them to this note.
There was a problem hiding this comment.
This record is on hold for an engineering-owner decision: it makes AppShell's shipped behavior current requirements (FR1–FR16, AR1–AR4, PR1–PR2, ORD1–ORD2) that no current spec states yet, and the spec-owner-approval check is still pending. Any engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner; the decision should settle in particular whether FR12's mobileNav={false} clause, which leaves the side nav unreachable below the breakpoint (zoomed desktop windows included), should be a requirement while OQ1 is open. Thanks for the FR4 and source-only note corrections; before approval, please also fix:
- FR13 calls
hasAutoToggle"whether auto toggles render", but AppShell sets it fromhasToggle !== falsealone, so it is true at every desktop width and undermobileNav={false}. - The drawer's modal presentation, focus and dismissal are handed to
component:MobileNav, a draft that says it does not cover them; namefamily:overlay-dismissalfor Escape and mark the rest uncontracted, as the record does for TopNav and SideNav. - The source-only note still omits six untested clauses: FR8's default
md, FR11's no-op requests while disabled, FR12's full-mode TopNav beside a drawer node, the mobile top bar's navigation name, FR13'shasAutoToggle, and FR4'scontentPaddingdefault.
[Automated review]
| | FR10 | The auto drawer renders only in the mobile layout and only when `mobileNav` is not `false`, not a node, and has no `content`. Without `topNav` it is SideNav in drawer mode; with `topNav` it is TopNav in drawer mode carrying the side nav content; with a heading-only TopNav the side nav items render exactly once. It stays mounted while closed and, after a close, is not hidden before MobileNav's close delay has elapsed (hidden at once under reduced motion); an open is visible in the commit that carries it. | Current source and tests | | ||
| | FR11 | The drawer is closed by default and AppShell owns its state unless `mobileNav.isOpen` is defined. Every requested change calls `mobileNav.onOpenChange`. Open and toggle requests do nothing unless mobile navigation is enabled: a nav slot is present, `mobileNav` is not `false`, and `mobileNav` is not a node. | Current source and tests | | ||
| | FR12 | `mobileNav={false}` renders no drawer and no toggle; in the mobile layout the side nav then renders neither inline nor in a drawer. A node (element or string) renders as given at every width, replaces the auto drawer, and leaves TopNav in its full mode. A config object's `content` replaces the auto drawer in the mobile layout when a nav slot is present. `hasToggle: false` removes the auto toggle. | Current source and tests | | ||
| | FR13 | AppShell provides its subtree with: whether the mobile layout applies, whether the drawer is open, one drawer id shared by the drawer's `id` and the toggle's `aria-controls`, open, close, and toggle actions, whether mobile navigation is enabled, and whether auto toggles render. | Current source and tests | |
There was a problem hiding this comment.
hasAutoToggle is set from mobileNav.hasToggle !== false alone (AppShell.tsx:516, :680), so it is true at every desktop width and under mobileNav={false} or a drawer node, where no auto toggle renders; the hook's docs call it "Whether AppShell auto-toggle behavior is enabled". Suggest: "whether auto toggles are enabled (hasToggle is not false)".
| - The drawer dialog itself: modal presentation, focus containment and return, | ||
| Escape and backdrop dismissal, slide motion, and its close delay — owned by | ||
| `component:MobileNav`. The hamburger control is MobileNav's | ||
| `MobileNavToggle`. |
There was a problem hiding this comment.
component:MobileNav is a draft whose accessibility contract "does not change or extend MobileNav's existing dialog naming, focus, Toggle relationship, Button labels, modal behavior, or dismissal behavior", and it names family:overlay-dismissal (current, MobileNav a member) as the owner of Escape and platform-close ordering. Suggest delegating Escape to that family and marking the rest owned by MobileNav and uncontracted here, as for TopNav and SideNav; AR3's last sentence needs the same change.
| The `wash`, `surface`, and fill-height `section` backgrounds in FR7; FR9's | ||
| drawer-node clause; FR10's reduced-motion hold; FR11's `mobileNav.onOpenChange` | ||
| call; FR12's `mobileNav={false}`, `content`, and `hasToggle: false` branches; | ||
| FR15; and PR2 are read from source; no focused test or baseline checks them | ||
| yet. |
There was a problem hiding this comment.
Six more clauses have no test in either cited file: FR8's default md (the only 768px assertion passes breakpoint: 'md', and sm is never exercised although the FR8 row says "Every name"), FR11's no-op open and toggle requests while disabled, FR12's "leaves TopNav in its full mode" (no node test has a topNav), the mobile top bar's @astryx.appShell.mobileNavigation name, FR13's hasAutoToggle, and FR4's contentPadding default (the padding tests assert only that main renders). Suggest adding them here.
…wer to its owners
There was a problem hiding this comment.
This record is on hold for an engineering-owner decision: it makes AppShell's shipped behavior current requirements (FR1–FR16, AR1–AR4, PR1–PR2, ORD1–ORD2) that no current spec states yet, and the spec-owner-approval check is still pending. Any engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner; the decision should settle in particular FR12's mobileNav={false} result while OQ1 is open, and FR6 keeping the banner pinned in auto although the banner prop's docs say it scrolls away with the page. Thanks for the earlier corrections; before approval, please also fix the verification map, whose cited suites still pass with each of these clauses removed:
- FR1's prop pass-through, FR2's
falseand''slot rule, FR4's no-focus-ring clause, FR6's sticky side region inauto(its test only checks for a grandparent element) and AR2's shared focus outline need to be listed in the source-only note or covered by assertions.
[Automated review]
| | ------------------- | -------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------- | -------------------------------------------------------------------------------- | ------------------------------ | | ||
| | FR1–FR4, AR1 | `AppShell.test.tsx` slot, landmark, banner, and ref suites | Content only; each slot; mobile top bar with banner | A missing or duplicate landmark, or a nested banner, fails role queries. | `audit:AppShell/anatomy` | | ||
| | FR5, AR2 | `AppShell.test.tsx` skip-link and keyboard suites | First Tab; Enter; click; translated text | Losing the href, the focus move, or tab order fails. | `audit:AppShell/accessibility` | | ||
| | FR6, PR1 | `AppShell.test.tsx` height-mode suites | `fill`, `auto`, section header in `auto` | Dropping sticky placement or measuring in `fill` fails. | `audit:AppShell/behavior` | |
There was a problem hiding this comment.
"Dropping sticky placement … fails" holds only for the header: the side-region test ("wraps sideNav in sticky container in auto mode") asserts only that the side nav has a grandparent element, so the AppShell suite still passes with the sticky wrapper around the auto side region removed. Suggest limiting this expectation to the header, or asserting the wrapper's computed position: sticky.
| | FR7, FR16 | `AppShell.stories.tsx` and visual baselines | `elevated` with both navs and with the mobile nav | A changed `elevated` background or missing corner shows in visual review. | `audit:AppShell/visual` | | ||
| | Theming anatomy map | `scripts/check-knowledge.mjs` | Consumer anatomy and current targets | Missing, extra, stale, or multiply assigned mappings fail repository validation. | `audit:AppShell/theming` | | ||
|
|
||
| Read from source, with no focused test or baseline yet: the placement of the |
There was a problem hiding this comment.
This note still omits five clauses that no cited test checks; the AppShell suite passes with each removed: FR1's other DOM props on the root, FR2's false and '' slots being absent, FR4's no focus ring on main, FR6's sticky side region in auto, and AR2's shared focus-outline treatment on the skip link. Suggest adding them here or adding the assertions.
There was a problem hiding this comment.
This record is on hold for an engineering-owner decision: it makes AppShell's shipped behavior current requirements (FR1–FR16, AR1–AR4, PR1–PR2, ORD1–ORD2) that no current spec states yet, and the spec-owner-approval check is still pending. Any engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner; the decision should settle in particular FR12's mobileNav={false} result while OQ1 is open and FR6 keeping the banner pinned in auto. Thanks for the source-only corrections; before approval, please also fix:
- FR6 says the
autoside region sticks below the measured header, but AppShell never measures a header that first renders after mount (a banner or top nav added later), so the side nav's first rows then slide under it; record this under Current-state impact or narrow FR6. - AR3 and FR13 say the auto toggle's
aria-controlsnames the drawer, but with rawmobileNav.content, which its docs allow, the toggle renders and names no element; scope both to the auto drawer and aMobileNavpassed ascontent, or record the gap. - The source-only note still omits five unchecked clauses: FR1's drawer-last order, FR2's renders-nothing rule, FR7's
sectiondividers, FR12's string node and FR16's no-corner branch.
[Automated review]
| | FR3 | The header region renders only when `topNav` or `banner` is present; it carries `app-shell-header` and `role="banner"`, places `banner` above `topNav`, and is never inside another landmark. The sidenav-only mobile top bar (FR9) also carries `app-shell-header` and takes `role="banner"` only when no header region renders. AppShell renders at most one banner landmark. | Current source and tests | | ||
| | FR4 | `children` render in one region with `role="main"` and `id="astryx-app-shell-main"`, programmatically focusable so the skip link can move focus to it; programmatic focus on it draws no focus ring. Its padding is `contentPadding`, default `0`. Whether it is a sequential tab stop is keyboard access to a scroll region (`spec:AST-025` FR12). | Current source and tests | | ||
| | FR5 | The skip link is the first focusable element in the shell. It links to `#astryx-app-shell-main`, shows the translated `@astryx.appShell.skipToContent` text, is visually hidden until focused, and on activation moves focus to the main region. | WCAG 2.4.1; current source and tests | | ||
| | FR6 | `fill` sizes the root to `100dvh` and the inline side region and main region scroll on their own. `auto` gives the root a `100dvh` minimum, makes the header sticky at the block start, keeps the inline side region sticky below the measured header height with its own scrolling, and lets the page scroll. Header height is measured only in `auto`. | Current source and tests | |
There was a problem hiding this comment.
AppShell measures the header in an effect keyed only on the height mode that returns early without a header (AppShell.tsx:636-650), so a banner or top nav that first renders after mount is never measured and the auto side region sticks at 0, under the sticky header (reproduced in Chromium: the side nav's first rows slide under a 44px banner). Suggest recording this under Current-state impact, or limiting this clause to a header present when the shell mounts.
| - **AR3 — Drawer wiring.** The auto toggle's `aria-controls` names the drawer that | ||
| opens and its `aria-expanded` reflects the drawer state (FR13). Focus | ||
| containment and return inside the drawer belong to MobileNav and are | ||
| uncontracted here. |
There was a problem hiding this comment.
MobileNavConfig.content is documented to take raw children (AppShell.tsx:147-149); AppShell renders them as given (:905) while the auto toggle still renders, so its aria-controls names no element and nothing opens. Suggest scoping this, and FR13's shared drawer id, to the auto drawer and a MobileNav passed as content, or recording raw content under Current-state impact.
| | FR7, FR16 | `AppShell.stories.tsx` and visual baselines | `elevated` with both navs and with the mobile nav | A changed `elevated` background or missing corner shows in visual review. | `audit:AppShell/visual` | | ||
| | Theming anatomy map | `scripts/check-knowledge.mjs` | Consumer anatomy and current targets | Missing, extra, stale, or multiply assigned mappings fail repository validation. | `audit:AppShell/theming` | | ||
|
|
||
| Read from source, with no focused test or baseline yet: the placement of the |
There was a problem hiding this comment.
Five more clauses have no check in either cited file or the visual baselines (the suites still pass with each of the first three broken): FR7's section dividers, FR1's drawer-last order, FR12's string node, FR2's "renders nothing still counts as present", and FR16's no-corner branch (the three AppShell baselines are captured 1024px wide with both navs inline, so each shows the corner). Suggest adding them here or adding the assertions.
There was a problem hiding this comment.
This record is on hold for an engineering-owner decision: it makes AppShell's shipped behavior current requirements (FR1–FR16, AR1–AR4, PR1–PR2, ORD1–ORD2) that no current spec states yet, and the spec-owner-approval check is still pending. Any engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner; the decision should settle in particular FR12's mobileNav={false} result while OQ1 is open and FR6 keeping the banner pinned in auto. Thanks for the late-header and raw-content corrections; before approval, please also fix:
- The source-only note still omits five clauses whose cited suites still pass when the clause is broken: FR1's header, side and content order, FR2's
nullandtrueslots, FR3's header staying absent with onlysideNav, and FR13 and AR3's shared drawer id on aMobileNavpassed ascontentor as the node. - The late-header entry says the header is measured only when the height mode is set, but AppShell re-measures a header present at mount on every resize; only a header region that first renders after mount is missed.
[Automated review]
| - **A header added after mount is not measured.** In `auto`, the header height | ||
| is measured only when the height mode is set; a banner or top nav that first | ||
| renders later is never measured, so the side region sticks at the top edge and | ||
| its first rows slide under the sticky header. |
There was a problem hiding this comment.
AppShell keeps observing a header that exists when auto takes effect and re-measures it on every resize, so a banner added later beside a top nav that was there from mount is measured; only a header region that first renders after mount is missed. Suggest: "In auto, AppShell measures only a header region present when auto takes effect; a header region that first renders later, such as a banner or top nav on a page that had neither, is never measured, so …"
| | FR7, FR16 | `AppShell.stories.tsx` and visual baselines | `elevated` with both navs and with the mobile nav | A changed `elevated` background or missing corner shows in visual review. | `audit:AppShell/visual` | | ||
| | Theming anatomy map | `scripts/check-knowledge.mjs` | Consumer anatomy and current targets | Missing, extra, stale, or multiply assigned mappings fail repository validation. | `audit:AppShell/theming` | | ||
|
|
||
| Read from source, with no focused test or baseline yet: |
There was a problem hiding this comment.
Five more clauses pass the cited suites when broken and are missing from this list: FR1's header, side and content order; FR2's null and true slots; FR3's header staying absent with only sideNav; and FR13 and AR3's shared drawer id on a MobileNav passed as content or as the node. The FR7/FR16 row's "with the mobile nav" baseline is captured at 1024px, so it shows the desktop layout.
There was a problem hiding this comment.
This record is on hold for an engineering-owner decision: it makes AppShell's shipped behavior current requirements (FR1–FR16, AR1–AR4, PR1–PR2, ORD1–ORD2) that no current spec states yet, and the spec-owner-approval check is still pending. Any engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner; the decision should settle in particular FR12's mobileNav={false} result while OQ1 is open, and FR6 keeping the banner pinned in auto. Thanks for listing the remaining source-only clauses and narrowing the late-header entry. One correction remains before approval: the ownership boundary says TopNav and SideNav name their landmarks through label props, but SideNav has no label prop (it uses its translated "Side navigation" name), so that line should credit TopNav's label and SideNav's translated name.
[Automated review]
| - Navigation landmark names inside the TopNav and SideNav slots — owned by those | ||
| components and their `label` props. |
There was a problem hiding this comment.
SideNav has no label prop: SideNavProps declares none, and its nav names itself with the translated @astryx.sideNav.label string ("Side navigation"). TopNav's landmark name does come from its label. Suggest: "owned by TopNav (its label prop) and SideNav (its translated name)".
There was a problem hiding this comment.
This record is on hold for an engineering-owner decision: it makes AppShell's shipped behavior current requirements (FR1–FR16, AR1–AR4, PR1–PR2, ORD1–ORD2) that no current spec states yet, and the spec-owner-approval check is still pending for this head. Thanks for correcting the SideNav landmark attribution; no other correction remains. Any engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner; the decision should settle in particular FR12's mobileNav={false} result while OQ1 is open, and FR6 keeping the banner pinned in auto although the banner prop's docs say it scrolls away.
[Automated review]
|
/approve-spec 1a5cc06 |
github-actions
Bot
added
spec-auto-merge
Exact decision needed
Record AppShell's shipped behavior as a current component contract (
component:AppShell). No behavior, prop, default, target, or export changes.Existing authority searched
pnpm specs:for packages/core/src/AppShell/AppShell.tsx: current records that govern AppShell arespec:AST-012(FR8, width points),architecture:theme-application(INV10, nearest effective width map),spec:AST-027(local stacking; its inventory names the AppShell header and skip link), andarchitecture:component-theming-surface. The applies-to-everything architecture records (style authoring, test sufficiency, public component API, runtime, interaction modality) add no AppShell-specific claim.spec:AST-025names Layout, not AppShell, as the owner of region scrolling.packages/core/src/AppShell/. Four open PRs change AppShell code (keyboard scrolling of a fill-height main region, a touch header bar, safe-area insets, an adaptive height mode); none touches a record, and this record describes main as it is today.Decision
One record,
packages/core/src/AppShell/AppShell.spec.md,authority: current, approved by cixzhang.It owns: the page frame and slot-presence rule; the skip link; the banner and main landmarks;
fillandautoheight modes; variant backgrounds and the elevated corner; the breakpoint switch and the render mode it picks for each nav slot; the auto mobile drawer composition, its open state, and its close hold; the mobile context AppShell provides; and content continuity across the switch.It delegates: TopNav and SideNav render modes (uncontracted here), the drawer dialog and its focus behavior (
component:MobileNav), Layout regions and their scrolling (family:layout-regions; keyboard access underspec:AST-025), width points (spec:AST-012), and theuseAppShellMobilehook contract (an uncontracted module).AppShell.doc.mjshas no inlinecomponents:entries, so no other record lives in this root.Rejected alternatives
None.
Claim scope and non-goals
Only the claims inside the record's ownership boundary. The hook's public contract, the drawer's own behavior, and Layout's keyboard access to an overflowing region stay uncontracted here. OQ1 (whether
mobileNav={false}should keep the side nav reachable in the mobile layout) stays open; the record states what ships.Evidence and implementation relationship
AppShell.tsx;AppShell.test.tsxslot, landmark, banner, and ref suites.AppShell.test.tsxskip-link and keyboard suites; the link uses the shared focus-outline helper.auto):AppShell.test.tsxheight-mode suites.AppShell.tsxstyle selection;AppShell.stories.tsxand the visual baselines; the auto-height section header test.none, nearest theme, SSR hint):AppShell.test.tsxbreakpoint suites.mobileNavshapes, close hold):AppShell.tsx;AppShell.test.tsxmobile suites;MobileNavCloseEdgeCases.test.tsxAppShell drawer suites (Escape, backdrop, close hold, controlled open in the same commit). FR12'smobileNav={false}clause is read from source:showSideNavInlineignores the disabled flag and no drawer renders.AppShell.test.tsxkeyboard drawer anduseAppShellMobilesuites.AppShell.test.tsxcontinuity test.Current-state impact (two items): (1) the root is not an isolation owner; the
autosticky header paints atz-index: 1and the focused skip link atz-index: 9999against page content.spec:AST-027FR2–FR4 require local values inside a surface-owned isolation boundary; AST-027's own inventory lists the same AppShell coupling. (2) Inheight="fill"the main region scrolls but is never a sequential tab stop;spec:AST-025FR12 requires effective scroll content to stay keyboard reachable. An open code PR addresses it; the record states today's behavior and leaves tab-stop behavior to AST-025.Source-only claims (no focused test yet) are listed under the record's verification map.
Writing rubric results
#+ digits,/pull/,/issues/).Scope
idand the skip link'shrefare named because callers and the skip link depend on them.Validation
pnpm check:knowledge: aligned (theming anatomy and delegations validated)git diff --check: clean