Review 4053
#4053 feat(layout): add shellWidth to cap and center the entire shell by jiunshinn (bucket: contributor)
a9a0d1b53d01f243fb76d86d8033d722c8bb9196
REFRESH: the branch was CONFLICTING/DIRTY at 0ee614bbce79f7d34e9c47b035083d5cf38bd5cf. Current main 2458b03b35a45512b996b0c2c764d9530a36a052 was merged in as an ordinary merge commit — no rebase, no history rewrite — and pushed non-force to jiunshinn/astryx:feat/2625-layout-shellwidth. The PR head moved 0ee614bb → a9a0d1b5 and the PR is now MERGEABLE. Every claim below is verified at the new head.
Two conflicts, both mechanical:
-
File location.
mainrenamedpackages/cli/templates→packages/cli/assets/templates. The branch's two newLayoutShellWidth.{tsx,doc.mjs}files were added inside the old directory; git flagged the location and the resolution keeps them at the new path. No content changed. The branch's edit topages/settings/page.tsxfollowed the rename automatically. -
Content,
Layout.doc.mjs. Both sides independently added acontentWidthprops entry, which did not exist at the merge base. The resolution keepsmain's landed entry (type: 'SizeValue', plus its newpaddinganddefaultHasDividersentries) and keeps the branch'sshellWidthentry beside it. The branch's own duplicatecontentWidthentry is dropped as superseded. No author logic, type or wording was altered — theshellWidthentry is byte-identical to what the author wrote.
No product, API or behaviour choice was needed to resolve either.
PRIOR REVIEW: none. pulls/4053/reviews and pulls/4053/comments are both empty — the only comments on the PR are the Vercel bot and the PR Analysis Report. There is nothing to extend, narrow or contradict.
LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13
LANE: full WHY: new permanent public API on a core component; an intended visual change to a shipped page template; a 40-day refresh conflict; and more than one decisive check (geometry, RTL, composition, far-side-of-the-cap, CI).
WHY 1: a builder capping a settings or app page on a large monitor can narrow the content inside each slot, but cannot make the header and footer rules stop where the page stops — they keep running to the viewport edge.
WHY 2: to get the design they leave the system and hand-write maxWidth + marginInline: auto on the Layout, which is CSS no theme can reach and the template rubric penalises.
WHY 3: Layout exists so a page shell is expressible through the system. A shell width only custom CSS can say means the primitive does not describe the page it is for.
USER-FACING PROBLEM: a builder laying out a settings page on a wide monitor gets header and footer rules running edge to edge across the viewport when the design has them ending at the page edge, and the only way to get the design is CSS the design system cannot see.
PROBLEM SEVERITY: missing capability — #2625's shell cap has no supported path today. The demonstrated consumer is the settings page template, which carried exactly that custom CSS until #2998 replaced it with contentWidth={1440} and, in doing so, lost the capped-divider look.
The PR's own honesty note is accurate and worth crediting: it says plainly that #2625's original rubric evidence is stale, and re-states the motivation as the remaining capability gap.
VERDICT: clear
Layout already knows two boxes: the whole shell, and the content inside each slot. Until now only the inner one could be capped, so a narrowed page still drew its rules across the full width. The new prop caps the outer box instead, so everything the shell draws — including its rules — stops at the same edge, and the page sits centred in whatever space is left over. The two caps stack, so a page can be narrow overall and narrower still in its reading column.
SOLUTION (2 decisions · ~24 runtime lines of 439)
-
shellWidthcaps and centres the shell wrapper — the common parent of header, middle row and footer — serving the #2625 capability gap. - the
settingspage template adopts it in place ofcontentWidth={1440}, serving the stated regression #2998 left behind.
Both trace to a problem written down in the PR body. Neither is piggybacked, and the author marks (2) as droppable on its own.
BURDEN: low — one optional public prop and one static StyleX declaration. No state, no Effects, no listeners, no timers, no observers, no shared-system coupling. BURDEN MATCH: proportionate — a missing capability answered by one declaration.
VERDICT: clear
OWNER: Layout — the component that renders the shell wrapper the cap attaches to.
TIER 1: StyleX dynamic styles and the shared SizeValue type; no new shared behaviour system.
TIER 2: none.
SEAMS: header / footer / start / end / content slots · LayoutDividerContext · nested Layout · consumer xstyle on the root · the container-padding escape on layoutOuter.
BEHAVIOR UNIT: inline — one conditional static declaration, directly observable in the DOM and in the browser geometry.
| seam | driven result |
|---|---|
| header + footer with dividers | works — both boxes are 640px at x162→802 inside an 896px root; the rules end at the shell edge, not the container edge |
| start panel + content | works — panel 160px (x162→322) and content 480px (x322→802) tile the shell exactly; no gap, no overflow |
composed with contentWidth
|
works — shell 800px centred in 1196px, content column 560px centred inside the shell at x352→912 |
| RTL | works — computed direction rtl, panel mirrored to x1118→1278, shell still centred (gaps 130/130), docOverflowX 0 |
| cap wider than the container (far side of the bound) | works — shellWidth 2000 in a 350px container renders 346px, margin-inline 0/0, docOverflowX 0 |
contentWidth alone, unchanged |
works — shell 896px, header 896px full-bleed, content 640px centred; the existing contract is untouched |
The placement question the PR raises itself — inner wrapper rather than root — is answered correctly and for the stated reason: layoutOuter carries negative inline margins (calc(-1 * var(--container-padding-*))) to escape container padding, and margin-inline: auto there would fight them. Attaching to the inner wrapper is what makes the header, footer, panels and their dividers cap together, which is the whole point of the prop.
VERDICT: clear
Builders: nobody, until they opt in. shellWidth is optional and unset by default; every existing Layout call site renders identically. The contentWidth-only story measures the same before and after — shell 896px, header 896px, content 640px.
One exception, and it ships to people who never asked. Anyone who scaffolds the settings page template gets a visibly different page: today its header and footer rules run the full width of the viewport; after this they stop at 1440px with the page background showing on both sides. That is a restoration of the design the template had before #2998, and the author says so — but it is a change a person sees, on a shipped template, and no design owner has looked at it yet. The repo's own needs:design-review label is on the PR for exactly this.
Nothing is newly exposed elsewhere. contentWidth keeps its full-bleed divider behaviour, so the two treatments now sit side by side deliberately rather than one making the other look wrong.
VERDICT: note — the settings template's appearance changes for every scaffold of it, and that half is a design owner's to accept.
// packages/cli/assets/templates/pages/settings/page.tsx — the real call site
<Layout height="fill" shellWidth={1440} header={…} start={…} content={…} />| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
+ |
Layout.shellWidth?: SizeValue (no default) |
yes — core barrel |
Layout.contentWidth?: SizeValue — the same cap-and-centre axis on the same component |
en; docsZh carries no per-prop table, so there is no zh row to add |
ok |
OSSIFICATION. The class exists and has a landed member: contentWidth is the same idea applied to the inner box, with the same type and the same centring. The capability itself is not a new concept — @cixzhang reserved it in the layout-prop standardization: "➕ (Tier E) maxWidth / shellWidth: SizeValue to cap + center the whole shell → #2625". Being wrong costs a deprecation cycle on one optional prop; there is no meaning change and no default to move.
The naming question the PR asks, answered from the code. #3223 reserved both candidate names without choosing, and the author explicitly offers to rename. The grep settles it:
grep -rn "maxWidth?:" packages/core/src --include=*.tsx → 7 components
Card · Center · ClickableCard · Grid · Section · SelectableCard · Stack (all SizeValue)
grep -rn "marginInline: 'auto'" packages/core/src → 5 hits, 3 files
Chat/ChatLayout.tsx (x2) · Layout/Layout.tsx · Layout/LayoutFooter.tsx · Layout/LayoutHeader.tsx
Seven landed components spell maxWidth for a cap that does not centre, and none of them appears in the centring list. Reusing maxWidth on Layout would give one name two behaviours inside one package. shellWidth pairs with the landed contentWidth on the same component and says which box it caps. Keep shellWidth — this is a citation, not a preference, and it does not need a ruling.
One nit, builder-facing. The Layout.doc.mjs entry declares type: 'number | string' while the TypeScript prop is SizeValue, and main's neighbouring contentWidth entry now says SizeValue. astryx component Layout therefore prints a type the code does not use, and the two rows disagree about a shared type on the same component. One word.
VERDICT: note — the documented type does not match the shipped type.
No theme target, component variable, token or style contract is added. themeProps('layout', {height}) is unchanged, so shellWidth is not reflected as a data attribute — which matches contentWidth, also absent from themeProps, and is correct: a continuous value is not a theming key. The grep for raw paint on the changed core runtime is clean:
git diff origin/main -- packages/core/src/Layout/Layout.tsx \
| grep -nE "#[0-9a-fA-F]{3,8}|rgba?\(|hsla?\(|boxShadow|light-dark\(" → 0
No existing target changes the element it points at: the cap lands on the shell wrapper, which carries no astryx-* target — astryx-layout stays on the root, and astryx-layout-header/-footer/-panel/-content stay on their own elements. A theme selecting any of them still resolves.
VERDICT: clear
BEHAVIOR: no. The prop is optional with no default; the diff adds no conditional, early return or state setter on an existing path. Walked against the states this diff can reach: empty (no slots) renders as before, since the declaration is skipped when the prop is unset; loading / error / disabled / controlled-vs-uncontrolled are not reachable — Layout has no such states and the diff adds none. Boundary is the interesting one and it is driven below.
API: yes, and this is the blocker — the package's own template-docs project stops compiling. packages/cli/assets/templates/blocks/components/Layout/LayoutShellWidth.doc.mjs:3 reads:
/** @type {import('../../../../../core/src/docs-types').TemplateDoc} */That path was correct where the author wrote the file. It is not correct now: main both renamed packages/cli/templates → packages/cli/assets/templates (one directory deeper, so the five .. land in packages/cli/) and moved these types to @astryxdesign/cli/authoring, deleting packages/core/src/docs-types. 682 template doc files import from @astryxdesign/cli/authoring; this one file is the only other spelling in the tree. CI at the exact head proves the consequence:
assets/templates/blocks/components/Layout/LayoutShellWidth.doc.mjs(3,19):
error TS2307: Cannot find module '../../../../../core/src/docs-types'
typecheck:template-docs fails → build-storybook fails → build fails → pr-a11y, pr-rtl and Stable visual regression all skip. Everything else at this head is green: test, Lint, check-components, check-scope, theme-layers, docsite-test, build-sandbox, CLI Smoke Test, Internal Registry.
Not the author's mistake — the rename landed after they wrote the file — but it is the state of the branch, and it is theirs to push.
This is a class, not an instance. A concurrent run of this loop hit the identical failure on #4158 the same night: "doc-types module does not exist → build-storybook". Every aged PR that added a *.doc.mjs before main moved these types to @astryxdesign/cli/authoring now fails the same gate the moment it is refreshed, and the failure is invisible until someone merges main in. Worth a codemod or a one-line note on the migration PR rather than N contributors each losing a round trip — raised here as a system observation, not as anything this PR owes. The prop itself is additive: nothing removed, nothing retyped, no default moved, and no consumer call site stops compiling.
VISUAL: yes, in one place — the settings page template. Frames below. Everywhere else, no: the contentWidth-only story measures identically at the new head.
THEME: no. No target added, removed or moved; no token or override changes resolution.
The far side of the bound, since a cap only shows what it does past its own limit: shellWidth={2000} in a 350px container renders the shell at 346px with margin-inline: 0px/0px and docOverflowX: 0. The cap degrades to 100% rather than forcing a horizontal scrollbar, which is the behaviour a max-width should have and the one worth checking.
VERDICT: BLOCKS — the template-docs project does not typecheck at this head, and it takes the whole visual and accessibility half of CI down with it. (Also, separately: one shipped template changes appearance — see IMPACT.)
EFFECTS: zero added, changed, kept or moved.
git diff origin/main -- . | grep -nE "^\+.*use(Layout)?Effect" → 0
RENDER: one extra conditional entry in an existing stylex.props(...) merge on an element already being styled. No state, no subscription, no extra render pass, no dependency whose identity changes.
LISTENERS/OBSERVERS: none added; nothing on document or window, no ResizeObserver or MutationObserver.
LAYOUT: no JS layout reads or writes. No getComputedStyle, offsetWidth or getBoundingClientRect in the diff; the cap is native CSS max-width + margin-inline: auto, resolved by the browser.
BUNDLE: no dependency. One additional StyleX rule, and it deduplicates against contentWidth's — the two declarations are identical, so StyleX emits one set of classes for both.
VERDICT: clear
VISUAL CHECK: manual frames required
WHY: pixels are intended to change; the prop's entire claim is a visual one ("dividers end at the shell edge"); and the settings template's appearance changes on a shipped surface. The exact-head visual-regression job did not run — it is skipped behind the failing build-storybook, so there is no gate to lean on.
shellWidth={640} versus contentWidth={640}, same 900px container, same viewport. This pair is the settings template change: the left column is what the template renders today, the right is what it renders after commit 2.
contentWidth={640} — rules run full width |
shellWidth={640} — rules stop at the shell |
|---|---|
 |
 |
Measured, same run: with contentWidth the shell is 896px and the header is 896px (x34→930) while the content column is 640px. With shellWidth the shell is 640px at x162→802 and the header and footer are 640px on the same bounds, with 130px of container background on each side. The difference is intentional and its source is the PR's own sentence: "a capped shell whose dividers end at the shell edge is still inexpressible without custom CSS".
SENSOR RECEIPT (both frames): exact head a9a0d1b5; stories core-layout--content-width-with-dividers and core-layout--shell-width-capped-shell; theme neutral; color mode light; direction ltr; viewport 1440×900 @1; forced-colors off, prefers-reduced-motion off; target count 1; semantic state {shellWithinContainer: true, shellCentred: true, shellHasCap: false|true}; subject visible and inside the viewport; fonts loaded; zero page and Storybook errors. Receipts are *.png.sensors.json beside each frame. Every sensor matches across the pair except Story and shellHasCap, which are the variables under test.
| RTL — panel mirrors, shell stays centred |
shellWidth={800} + contentWidth={560}
|
|---|---|
 |
 |
RTL: computed direction rtl, the start panel moves to x1118→1278, the shell is still 640px with gaps of 130px on both sides, and docOverflowX is 0. margin-inline: auto is direction-agnostic, so there is nothing here that could go one-sided — driven rather than assumed. Composed: the shell caps at 800px (x232→1032, gaps 200/200) and the content column at 560px centred inside it (x352→912), which is 800 − 560 halved. Both behave as the docs describe.
At a 375px viewport this frame shows the shell clipped and docOverflowX: 557. That is the story, not the prop: ShellWidthCappedShell hard-codes a 900px container, so it overflows a 375px viewport whichever width prop is set. The genuine narrow-container behaviour is the wider-1440 row above — cap 2000 in a 350px container, shell 346px, overflow 0. Worth knowing when reading the shipped stories: none of the three uses a fluid container, so none of them demonstrates the responsive degrade on its own.
Rows not owed: text zoom 200%, forced colors and icon-only do not apply — the diff writes one max-width and one margin-inline, adds no paint, no glyph and no text.
VERDICT: note — one shipped template's appearance changes; the mechanism itself is clean at every driven case.
REMEDY SEARCH: not triggered — no proven visual defect.
Structural in the core runtime, and the greps say so rather than prose:
git diff origin/main -- packages/core/src/Layout/Layout.tsx \
| grep -cE "aria-|role=|useTranslator|t\('@astryx" → 0
git diff --stat origin/main -- packages/core/locales → no changes
-
§1, the automated half: no ARIA, no role, no accessible-name surface is touched;
.github/a11y-baseline.jsonis not modified.pr-a11yis skipped at this head, not green — it sits behind the failing Storybook build, so the automated half is unrun and I am not claiming it. Nothing in the diff can produce a violation (no interactive element, no state, no text), but that is an argument, not the audit. - §1, the manual half: no focus surface, no keyboard path, no announcement, no hover-only affordance, no target size is added or moved. A cap on a wrapper does not remove anything from the DOM or reorder it — driven in the browser: focus order and tab stops in the capped shell are the panel items then the footer buttons, the same sequence and the same elements as without the prop.
-
§9, strings: no user-visible or AT-facing string is added to
packages/core. The Storybook stories and the CLI block carry English prose, which is the convention for both surfaces — the siblingLayoutContentWidth.tsxblock does the same and uses no translator. -
§9, direction:
margin-inlineandmax-widthare logical properties; no physical property, notranslate, no directional glyph. Driven in RTL above, since the diff touches inline sizing.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | clear |
| ARCHITECTURE | clear |
| IMPACT | note — one shipped template's look changes |
| API | note — documented type disagrees with the shipped type |
| THEMING | clear |
| BREAKING | BLOCKS — the template-docs project does not typecheck at this head |
| PERFORMANCE | clear |
| VISUAL | note — as IMPACT |
| A11Y & I18N | clear |
The one blocking finding is filed in BREAKING, on its API axis: the branch's own template-docs project does not typecheck at this head, and that one line takes build-storybook, build, pr-a11y, pr-rtl and the visual regression job with it. Local checks are otherwise clean — @astryxdesign/core typecheck, eslint on every changed file, and vitest run packages/core/src/Layout at 7 files / 98 tests passing.
GOAL: met — the stated capability works at every driven case. shellWidth={640} puts the shell at 640px centred with 130px on each side, and the header and footer boxes at 640px on the same bounds, against the contentWidth control's 896px full-bleed header in the same run.
DISPOSITION:
- broken template-doc type import (
LayoutShellWidth.doc.mjs:3) → blocks now - documented
shellWidthtype saysnumber | string, code saysSizeValue→ carried with the blocking ask, not independently blocking: one word, in a doc file the author is already opening for finding 1. A note only counts if it carries the fix, so it goes in the review text rather than into a follow-up nobody tracks. -
settingstemplate appearance changes on a shipped surface → accepted as gated, not by me: the repo'sneeds:design-reviewlabel routes it to a design owner before merge, and the author has offered to drop the commit. Named in the review so the choice is made rather than discovered. - shipped stories all use fixed-width containers, so none demonstrates the responsive degrade → accepted: the capability is proven by the wider-than-container story and the behaviour is correct; a fluid-container story is a nice-to-have on a story file, not a merge condition.
ADVICE: proven existing pattern — @astryxdesign/cli/authoring, cited with its 682 landed members and with the sibling LayoutContentWidth.doc.mjs in the same directory.
AUTHOR CAN PROCEED: yes — change line 3 of LayoutShellWidth.doc.mjs to import('@astryxdesign/cli/authoring') and the shellWidth doc entry's type to 'SizeValue'. Verified when typecheck:template-docs passes and build-storybook, pr-a11y, pr-rtl and the visual job run instead of skipping. No design or API decision is owed by the author: the naming question they raised is answered above from landed code, and the settings commit stays as it is — the repo's needs:design-review label already routes its appearance to a design owner before merge, and nothing about it gates their next push.
WORST OUTCOME: "typecheck:template-docs fails → build-storybook fails → build fails → pr-a11y, pr-rtl and Stable visual regression all skip" → request changes. Nobody can merge this, and worse, the checks that would catch an accessibility or RTL regression do not run at all — so the branch is not merely red, it is unmeasured on exactly the axes this prop touches. That sentence cannot coexist with an approve.
The strongest harm to an end user the diff makes possible is second: anyone scaffolding the settings template gets a page that looks different from the one they got yesterday. Real, intended, restoring a documented earlier design, and already routed to design review by the repo's own label — a note, not a block.
JUDGEMENT NEEDED: none — the capability was reserved by the maintainer in #3223, the class member (contentWidth) is landed, and the open naming question is settled by seven landed maxWidth call sites that do not centre.
request changes
-
LayoutShellWidth.doc.mjs:3imports../../../../../core/src/docs-types, which no longer exists aftermain's template-tree rename → the PR's whole visual and a11y CI half does not run:typecheck:template-docsfails,build-storybookandbuildfail with it, andpr-a11y,pr-rtland the visual regression job skip ·packages/cli/assets/templates/blocks/components/Layout/LayoutShellWidth.doc.mjs:3 -
(carried with 1, not independently blocking) the
shellWidthdoc entry declarestype: 'number | string'while the prop isSizeValue→ a builder runningastryx component Layoutreads a type the code does not use, and sees the two width props on one component disagreeing about a shared type ·packages/core/src/Layout/Layout.doc.mjs:104
Thanks for this, and for the write-up — the contentWidth / shellWidth split is the right distinction for #2625, and the implementation checks out.
I merged main in to clear the conflict, since packages/cli/templates moved to packages/cli/assets/templates. One line didn't survive the move:
// LayoutShellWidth.doc.mjs:3 — the other 682 template docs use this
/** @type {import('@astryxdesign/cli/authoring').TemplateDoc} */Yours still points at ../../../../../core/src/docs-types. It's the only red thing and it's expensive — typecheck:template-docs fails, which takes build-storybook and build with it and skips the a11y, RTL and visual jobs. While you're in there, the shellWidth doc entry says number | string where the prop is SizeValue.
On naming: keep shellWidth. Seven components already spell maxWidth for a cap that doesn't centre.
I'll leave the settings commit in for design to look at. Could you push those two lines?
If you'd rather talk it through with someone, we're in Discord.
-
packages/cli/assets/templates/blocks/components/Layout/LayoutShellWidth.doc.mjs:3— This path broke when the template tree moved.@astryxdesign/cli/authoring, like the block next door. -
packages/core/src/Layout/Layout.doc.mjs:104— Type saysnumber | string, prop saysSizeValue.
- The PR's verification list checks out where I re-ran it:
vitest run packages/core/src/Layoutis 7 files / 98 tests green at the merged head, core typecheck clean, eslint clean on the changed files. - The block's
doc.mjsis otherwise byte-shaped like its siblingLayoutContentWidth.doc.mjs— same fields, sameaspectRatio, samecomponentsUsed. Only the type import differs. -
packages/core/src/Layout/__tests__/shellWidth.test.tsxfinds the shell by walking fourparentElementhops from a test id. It passes, and it will need editing the first time the Layout DOM gains or loses a level — a maintainer-facing cost, not worth a round trip.
TIME total 45m
setup 6m worktree at the exact head, fast-install (11s, lockfile-matched donor), one @astryxdesign/build build, Storybook dev on 6253; warm main reused: yes (port 6100 checked live, used as the control reference only — the contentWidth control was measured at the review head instead, same run)
reading 11m loop version, brief, critic rules, presentation, loop mechanics, measurement harness, rulings, the #3935 precedent, PR body, #2625, #3223, the diff and Layout source
refresh 4m fetch, merge, resolve two conflicts, commit, non-force push, verify the live head moved
measuring 13m 4 probe runs (3 discarded), 5 sensor-backed frames, 6 geometry cases
writing 11m presentation, two critic passes, wiki record
waste 9m three probe runs thrown away: the first used StyleX-hashed class selectors that never match, the second and third failed sensors I had under-specified (theme/colorMode, then the full media object). The probe is banked at probe-kit/layout-shell-geometry.cjs so the next Layout width review starts from a working one.
-
pr-a11y,pr-rtland the visual regression job are skipped at this head, not green — they sit behind the failing Storybook build. The a11y and RTL automated halves are unrun; my RTL evidence is a driven frame, and the a11y argument is from the diff's contents, not from axe. - The
settingspage template is not rendered in Storybook, so its before/after is shown through the equivalentcontentWidth/shellWidthstory pair at the same container width rather than the template itself.
Not yet posted — read-only run; the loop does not post to public PRs.