-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5543
Versions: Review Loop 1.0.0 · Component Audit Rubric 1.10
Verdict: request changes + human architecture decision
#5543 feat(theme): responsive width tiers in defineTheme by imdreamrunner (bucket: collaborator → internal treatment)
4dacfe493d75e3eed1db53b76ef19be8e3645f12
Prior review: rubyycheung approved 1a254e86 with no review body. The current 4dacfe49 head then added 1,031 and deleted 153 lines across 11 files, so that approval states no conclusion about the current implementation.
LOOP VERSION: 1.0.0 AUDIT RUBRIC: 1.10
Theme authors who want global design values to vary with viewport width currently write media queries outside the theme or swap themes. The width use case is plausible but no shipped theme or concrete adopter is included; the one concrete example—the iOS input-font floor—is already owned per focusable input at (pointer: coarse) across every width.
VERDICT: note — the width need is asserted; the pointer need is already served at a narrower owner
A theme declares selected named viewport bands. Each matching CSS media query replaces only the declared theme values, while optional pointer refinements further narrow a band; runtime and built themes use the same generated rules.
SOLUTION (5 decisions · 1,538 runtime additions of 3,243 total additions)
- Fix four viewport names and defaults:
mobile ≤756,tablet ≤1024,desktop ≤1440,wideabove. - Make only declared tiers boundaries; the lowest declared bounded tier reaches to zero and sparse higher tiers absorb omitted bands.
- Add tier-to-tier
extends, including inherited pointer refinements and cross-theme tier inheritance. - Add nested
pointer: coarse|finerefinements under each width tier. - Publish the generator, breakpoint helpers/constants, and eight tier types from the core theme barrel.
Concrete split: (1) the behavior-preserving resolveThemeValues extraction can land alone if wanted; (2) the minimal width-tier surface and both distribution paths follow only after the architecture decision; (3) pointer refinements need a separate demonstrated non-component use; (4) the unused public helpers/types stay out until a caller exists. Nothing from the new tier surface should land before the decision.
VERDICT: [Needs human judgement] — five decisions establish a permanent responsive-theme subsystem; split the independent groundwork and pointer/export surface
OWNER: global design-token variation belongs in theme resolution; container-dependent component layout belongs with the component/consumer @container; the iOS font floor belongs on each focusable input.
TIER 1: defineTheme/Theme/theme build are reused; CSS media range queries are native and SSR-safe.
TIER 2: none.
SEAMS: runtime injection, built CSS/module output, whole-theme extends, tier extends, on-media surfaces, custom variants, and nested themes.
BEHAVIOR UNIT: pure utility — themeTiers.ts resolves deterministic input into query/value layers and is directly testable.
Driven seams: runtime and built CSS selected the intended tier at 756/757, 1024/1025, and 1440/1441 in Chromium; build/runtime ordering agreed. Whole-theme inheritance failed: an unchanged child re-resolved an inherited mobile layer and changed the base’s explicit accent red→blue and heading override 99px→the generated token.
VERDICT: BLOCKS — one distinct defect: tier inheritance does not preserve the base theme’s effective explicit precedence
No shipped theme opts in, so existing rendered CSS is unchanged. A theme author who adopts tiers gets viewport-wide token/component changes; a 320px component inside a 1200px page receives the desktop tier, while a 1000px component in a 600px viewport receives mobile, because these are media queries, not container queries.
Every built theme pays new metadata even without tiers: neutral’s generated JS grew 18,526→19,304 bytes raw and 3,078→3,219 gzip.
VERDICT: [Needs human judgement] — opt-in rendering, but permanent global semantics and an always-on built-output cost
const acmeTheme = defineTheme({
name: 'acme',
mobile: {
tokens: {'--spacing-4': '12px'},
'@media (pointer: coarse)': {typography: {scale: {base: 16}}},
},
tablet: {extends: 'mobile'},
});| change | public? | class | doc’d? | verdict |
|---|---|---|---|---|
+ DefineThemeInput.mobile/tablet/desktop/wide?: ThemeTier |
yes | no landed theme-level responsive condition | CLI docs; canonical Theming Infrastructure missing | [Needs human judgement] |
+ ThemeTier.maxWidth?: number |
yes | AppShell uses sm/md/lg at 640/768/1024; templates use local 768/1024; no shared 756/1024/1440 set |
CLI docs | [Needs human judgement] |
| `+ ThemeTier.extends?: WidthTier | 'default'` | yes | top-level extends exists but accepts a theme and means whole-theme inheritance |
CLI docs |
| `+ '@media (pointer: coarse)' | '@media (pointer: fine)'` nested values | yes | 28 production core files use flat pointer: coarse; none nests it under a theme width band |
CLI docs |
+ generateTierCSS, WIDTH_TIERS, DEFAULT_TIER_MAX_WIDTH, tierWidthQuery
|
yes, core theme barrel | only generateTierCSS has a non-test cross-package caller (CLI) |
no caller docs | finding — three unused values/functions |
+ WidthTier, ThemeTier, TierValues, TierTypeScale, TierTypographyConfig, TierConditionKey, TierBreakpoints, ThemeGenerativeAxes
|
yes, core theme barrel | no non-implementation consumers | no direct docs | finding — eight unused type exports |
~ DefinedTheme.__tiers/__tierInput/__axes |
public interface, marked internal; serialized by build | internal IR for runtime/build/extends | internal comments | note — couples generated themes to this IR |
Ossification: the four names and three default bounds have no landed shared vocabulary; AppShell’s nearest public breakpoint vocabulary differs. Removing or redefining a tier later changes every theme silently. Eleven of the twelve new named barrel exports have no consumer outside their implementation/tests; only generateTierCSS is used by the CLI.
VERDICT: [Needs human judgement] — new concept with no landed class; trim unused exports regardless
No new component theme targets. The change lives in the theme layer for global token variation, and the emitted CSS preserves token/component precedence within one theme. However, an extending theme passes only its own input.tokens/input.components into tier re-resolution (defineTheme.ts:616), so inherited explicit overrides lose to regenerated axes (themeTiers.ts:693-694).
The docs also teach a global typography-scale change to solve an input-only floor, contrary to API Conventions: “This is a per-component concern, not a global token. Each input component owns its own floor.”
VERDICT: BLOCKS — cross-reference to the inheritance defect; the example also assigns an input concern to global typography
API — additive; no existing call stops compiling, but 12 named exports and four root keys become permanent once released. Visual — no existing theme opts in, so existing apps do not move; opted-in themes change at CSS viewport boundaries. Theme — existing no-tier CSS is byte-identical, but a child extending a tiered base is not: at 600px its inherited accent changed from explicit red to generated blue, and its heading override was lost. Behavior states outside theme resolution are not reachable.
VERDICT: BLOCKS — cross-reference to the same inheritance defect; the documented whole-theme inheritance contract fails
Effects: 0 added. Listeners/observers: 0. Layout reads/writes: 0; CSS media evaluation is native. Built themes add no runtime generation; an unbuilt theme generates CSS once in the existing insertion effect.
Measured counts: a no-tier neutral module doing the same job grew 778 bytes raw / 141 gzip (4.2% / 4.6%) from serialized __axes; CSS stayed 19,498 bytes. A two-token sample grew from 2,997→3,954 CSS bytes and 1,220→3,000 JS bytes when six resolved layers were enabled. Generation performs one full rule pass for the base plus one per resolved layer: 2 passes for one tier, up to 13 for four tiers with both pointer refinements.
VERDICT: BLOCKS — no-tier built themes regress 4.6% gzip unless Cindy explicitly accepts the subsystem cost
Real Chromium, generated runtime CSS:
- 756/757px switched mobile red → tablet green; 1024/1025 green → desktop blue; 1440/1441 blue → wide purple.
- a 320px container at a 1200px viewport stayed desktop blue; viewport width, not container width, owns the result.
- mobile-only at 1024px coarse matched
pointer: coarsebut retained the base pointer value; the refinement is not independent of width.
Real Chromium, CLI-built CSS:
-
browser-built/fine-{756,757,1024,1025,1440,1441}.pngandcoarse-{600,1024}.png; computed spacing was 12/14/14/18/18/24px, and inherited coarse spacing was 10px at both 600 and 1024. -
inheritance-browser/mobile-600.png: base red and unchanged child blue at the same 600px viewport.
The range-query boundaries and source-order behavior are correct in Chromium. The screenshots were passed through the read tool, but this client returned image metadata rather than pixels; the computed style output is the verified evidence, and no visual-design judgement is claimed.
VERDICT: BLOCKS — cross-reference to the same inheritance defect; CSS boundary mechanics otherwise work as implemented
No rendered controls, roles, focus paths, user-facing runtime strings, locale logic, or RTL-sensitive layout change. CI’s component a11y/RTL jobs were skipped because no components changed. The input-font-floor guidance is a responsive/theme ownership problem here; shipped inputs already carry the correct all-width coarse-pointer floor.
VERDICT: clear — no new a11y or i18n path; the motivating workaround remains component-owned
| slot | verdict |
|---|---|
| PROBLEM | note — width need asserted; pointer need already served |
| SOLUTION | [Needs human judgement] — responsive-theme subsystem |
| ARCHITECTURE | BLOCKS — inheritance seam loses effective precedence |
| IMPACT | [Needs human judgement] — global semantics and no-tier cost |
| API | [Needs human judgement] — new concept; unused exports |
| THEMING | BLOCKS — same inheritance defect; wrong floor owner |
| BREAKING | BLOCKS — same inheritance defect |
| PERFORMANCE | BLOCKS — +141 gzip on no-tier neutral |
| VISUAL EVIDENCE | BLOCKS — same inheritance defect rendered |
| A11Y & I18N | clear |
GOAL: partly met — generated CSS switches correctly at every tested boundary and runtime/build agree, but the same theme is not stable through extends, and the pointer example cannot express the existing all-width input contract without extra width tiers.
DISPOSITION:
- inherited explicit token/component loss → blocks now; preserve effective declared precedence through whole-theme inheritance and add the red→blue /
99pxregression. - fixed viewport-tier subsystem, global-vs-container ownership, and +141 gzip no-tier metadata → one human decision; no reviewer acceptance has been supplied.
- nested pointer refinements and eleven unused exports → remove from this slice unless that decision explicitly includes them; the prior direction was flat pointer conditions and a smaller exported surface.
- contradictory “below the narrowest” docs → blocks if the model remains; implementation intentionally lets the narrowest declared bounded tier reach zero.
ADVICE: bounded direction — keep CSS-native media evaluation, but first settle whether global viewport tiers belong in defineTheme; keep container-responsive layout in component/consumer @container, and keep the input floor at the focusable input’s flat coarse-pointer rule. If tiers remain, preserve inherited explicit declarations separately from generated values and drive the regression through runtime and built themes.
WORST OUTCOME: “a variant theme that changes nothing responsive renders a different brand accent and heading at the same mobile width” → request changes.
JUDGEMENT NEEDED: API/architecture — should Astryx adopt a fixed four-name viewport tier system in defineTheme now? YES means global viewport—not container—semantics and accepting metadata on every built theme; NO means keep responsive layout/container behavior local and wait for a concrete global-width theme adopter. I recommend NO; keep the flat pointer condition and the resolver extraction separate.
request changes
-
[BLOCKS] whole-theme
extendsloses the base’s explicit token/component precedence inside inherited tiers → a variant theme that changes nothing responsive renders a different brand accent and heading at the same mobile width ·packages/core/src/theme/defineTheme.ts:616 -
[BLOCKS] the responsive example moves an input-only floor into a width-scoped global type scale → a theme author changes every heading/body size and can still miss a coarse-pointer device outside the declared bands ·
packages/cli/assets/docs/theme.doc.mjs:281 -
[BLOCKS pending acceptance] every built no-tier theme serializes the new axis metadata → every adopter downloads 141 more gzip bytes before using the feature ·
packages/core/src/theme/defineTheme.ts:653
Thanks—runtime/build parity and the boundary CSS are carefully covered. This needs an architecture call and a correctness fix.
This returns to #4930’s flat-condition ruling. The motivating input floor is already owned by each focusable control: “This is a per-component concern, not a global token.” Nesting it under a tier changes the whole type scale and needs extra bands not to miss coarse pointers. I’d keep pointer conditions flat and container-responsive layout in component/consumer CSS.
The resolver also changes an unchanged child theme: at 600px its inherited explicit accent goes red→blue and its heading override is lost. Please preserve effective inherited overrides and add that regression.
The barrel adds 12 names, 11 unused; neutral’s no-tier module grows 141B gzip. Can we hold this until Cindy decides whether defineTheme should own fixed mobile/tablet/desktop/wide bands, then keep only the surface she chooses?
MERGE: hold(inheritance regression and new responsive-theme architecture)
-
packages/core/src/theme/defineTheme.ts:616— This drops inherited explicit overrides when the tier re-resolves; the saved repro loses both token and component values. -
packages/cli/assets/docs/theme.doc.mjs:281— This input-only floor already belongs on each focusable control, at every coarse-pointer width. -
packages/core/src/theme/index.ts:50— Eleven of these new names have no caller outside the implementation/tests; can we keep them private?
- Chromium accepted CSS range syntax and selected exactly one declared tier at all six integer boundary checks.
- The CLI’s 12 tests and core’s 151 targeted theme tests pass; typecheck, changesets, diff-check, and all 21 CI signals are green/skipped as expected.
- The docs say “below the narrowest” falls back to base, while the test intentionally gives a lone
desktoptier every width ≤1440px.