Repository navigation
docs(spec): Theme, MediaTheme, and SyntaxTheme record their shipped contracts as current - #9350
Conversation
…ontracts as current
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
the
needs:spec-owner-review
cixzhang
changed the title
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
These three records can't be approved yet. Three rows describe behaviour that doesn't ship, and no engineering owner has signed off on the spec. Please correct the rows below. An engineering owner (the author counts, as an engineering owner) can then approve the corrected head by commenting /approve-spec <full head SHA>, or reject specific requirements.
SyntaxTheme.spec.mdFR1: presets anddefineSyntaxTheme()results fill every diff slot, so<SyntaxTheme theme={dracula}>writes--color-syntax-added-markerand--color-syntax-diff-headerinline, and only a hand-built definition leaves a slot unwritten.SyntaxTheme.spec.mdFR2:useSyntaxThemetakes unset markers from the syntax theme's punctuation and picks light or dark fromprefers-color-scheme, so under<Theme mode="dark">on a light OS it disagrees with the wrapper's variables.MediaTheme.spec.mdFR3:autoalso renders no marker when the surface's own text already reads at 3:1 (pickMediaModereturns'off', covered inuseAutoMediaMode.test.ts), and it re-measures on each render rather than on every surface change.
[Automated review]
|
|
||
| | ID | Invariant | Basis | | ||
| | --- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------ | | ||
| | FR1 | SyntaxTheme renders one block `div` wrapper carrying `data-astryx-syntax-theme` with the theme's name and, as inline custom properties, every syntax token the theme sets; an unset diff slot is left to its default. | Current source and tests | |
There was a problem hiding this comment.
Presets and defineSyntaxTheme() results carry every diff slot. defineSyntaxTheme.ts:198-211 fills unset markers with var(--color-syntax-punctuation), an unset header with transparent, and unset row paints with the semantic defaults, so <SyntaxTheme theme={dracula}> writes all of them inline. Only a hand-built definition missing a slot leaves it to the CSS default. Suggest: "...every slot the definition carries; presets and defineSyntaxTheme() fill unset diff slots..." Update the "Unset diff slot" state at line 92 to match.
| | ID | Invariant | Basis | | ||
| | --- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------ | | ||
| | FR1 | SyntaxTheme renders one block `div` wrapper carrying `data-astryx-syntax-theme` with the theme's name and, as inline custom properties, every syntax token the theme sets; an unset diff slot is left to its default. | Current source and tests | | ||
| | FR2 | Descendants read the syntax theme through context, so the syntax theme hook resolves the same tokens, with unset diff slots derived from the page theme. | Current source and tests | |
There was a problem hiding this comment.
Two mismatches with SyntaxTheme.tsx. Unset markers resolve to the syntax theme's own punctuation (:120-121, and the FR2 test's own name), and only added/removed derive from the page theme. The hook also picks light or dark from prefers-color-scheme (:96-97), so under <Theme mode="dark"> on a light OS it returns the light side while the wrapper's light-dark() variables paint the dark one. Please state what ships, and record the forced-mode mismatch as a known gap rather than a requirement.
| | --- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------ | | ||
| | FR1 | MediaTheme always renders one wrapper with `display: contents`, so switching to or from `off` never remounts its children. | Current source and tests | | ||
| | FR2 | `dark` and `light` mark the wrapper with that surface and the primary text color; `off` renders no marker. | Current source and tests | | ||
| | FR3 | `auto` measures the surface behind the wrapper and marks it dark or light, using `fallback` when the surface cannot be measured; it re-measures when the surface changes and leaves no probe element. `fallback` is ignored for explicit modes. | Current source and tests | |
There was a problem hiding this comment.
auto has a third result: pickMediaMode returns 'off' when the surface's own text already reads at 3:1 (useAutoMediaMode.ts:151-156), and MediaTheme then renders no marker (MediaTheme.tsx:100, :108). The prop doc and MediaTheme.doc.mjs:96 say so, and useAutoMediaMode.test.ts covers it. The effect also re-measures on each render rather than on any surface change. Suggest adding "or no marker when the surface's own text already reads on it", plus that state in the table at line 93 and in the verification map.
…me records slot filling and the forced-mode gap
There was a problem hiding this comment.
These three records can't be approved yet: they add new current requirements that no engineering owner has approved at this head, and two SyntaxTheme rows still need a fix. The earlier MediaTheme FR3 and SyntaxTheme FR1 issues are resolved.
- SyntaxTheme FR2 (
SyntaxTheme.spec.md:91) states the hook'sprefers-color-schemereading as the requirement, thougharchitecture:theme-applicationINV1, cited in the record's own Current-state impact, has descendants read the nearest Theme, so state the INV1 behaviour and move the OS reading after "Implementation gap:", as other component records do. - The hand-built representative state (
:102) says the hook derives a missing diff slot, but a missingdiff-headerresolves to an empty string; narrow it to the marker and row-paint slots.
Once those are in, an engineering owner records the decision by commenting /approve-spec <full head SHA> on the corrected head; the author may do so as an engineering owner.
[Automated review]
| | ID | Invariant | Basis | | ||
| | --- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------ | | ||
| | FR1 | SyntaxTheme renders one block `div` wrapper carrying `data-astryx-syntax-theme` with the theme's name and, as inline custom properties, every syntax token the definition carries. Presets and `defineSyntaxTheme()` fill every diff slot; only a hand-built definition missing a slot leaves it to the CSS default. | Current source and tests | | ||
| | FR2 | Descendants read the syntax theme through context. The syntax theme hook resolves each token for light or dark from `prefers-color-scheme`; unset marker slots take the theme's own punctuation, and unset `added` and `removed` derive from the page theme. Implementation gap: under a Theme with a forced mode, the hook can resolve the other side from the wrapper's variables (Current-state impact). | Current source and tests | |
There was a problem hiding this comment.
This makes the hook's prefers-color-scheme reading the requirement, while architecture:theme-application INV1 (cited in the Current-state impact above) has descendants read the nearest Theme. State the INV1 behaviour here, that the hook resolves each token for the nearest Theme's effective mode, and move the OS reading after "Implementation gap:", as Avatar.spec.md FR9 does.
| | State | Required invariant | Allowed variation | | ||
| | ----------------------------------------- | ------------------------------------------------ | ----------------- | | ||
| | Preset theme | Wrapper carries the preset's variables and name. | Preset. | | ||
| | Hand-built definition missing a diff slot | No variable written; the hook derives it. | Theme. | |
There was a problem hiding this comment.
For a missing diff-header the hook returns '' (SyntaxTheme.tsx:119, :128-129); only markers and added/removed are derived. Suggest: "No variable written; the hook derives a missing marker or row paint, and a missing header resolves to an empty string."
There was a problem hiding this comment.
These three records can't be approved yet: they add new current requirements that no engineering owner has approved at this head, and one MediaTheme row still misstates shipped behaviour. The SyntaxTheme FR2 and hand-built-state fixes from the last round are in.
- MediaTheme FR3 (
MediaTheme.spec.md:84) and the "auto, unmeasured" state (:96) promisefallbackfor any unmeasurable surface, but onceuseAutoMediaModehas measured a surface it keeps the last measured side when that surface later becomes unmeasurable (useAutoMediaMode.ts:197-200), so add an "Implementation gap:" clause as SyntaxTheme FR2 does, or narrow FR3 to "fallback until a measurement succeeds".
Once that is in, an engineering owner records the decision by commenting /approve-spec <full head SHA> on the corrected head; the author may do so as an engineering owner.
[Automated review]
| | --- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------ | | ||
| | FR1 | MediaTheme always renders one wrapper with `display: contents`, so switching to or from `off` never remounts its children. | Current source and tests | | ||
| | FR2 | `dark` and `light` mark the wrapper with that surface and the primary text color; `off` renders no marker. | Current source and tests | | ||
| | FR3 | `auto` measures the surface behind the wrapper on each render and marks it dark or light, renders no marker when the surface's own text already reads on it at 3:1, and uses `fallback` when the surface cannot be measured; it leaves no probe element. `fallback` is ignored for explicit modes. | Current source and tests | |
There was a problem hiding this comment.
Shipped auto only uses fallback until a measurement succeeds. useAutoMediaMode sets its state only from a successful measurement (useAutoMediaMode.ts:228) and returns early when the backdrop becomes unknowable (:197-200), so a surface measured as dark that later gains a background-image keeps dark even with fallback="light". The consumer docs (MediaTheme.tsx:71-78, MediaTheme.doc.mjs:103) promise fallback "whenever the backdrop is not knowable from CSS". Suggest keeping this row and adding: "Implementation gap: once a measurement has succeeded, a surface that becomes unmeasurable keeps the last measured side instead of fallback." Apply the same to the auto, unmeasured state at :96, or narrow both to "fallback until a measurement succeeds" if the sticky result is intended.
There was a problem hiding this comment.
The MediaTheme FR3 correction is in: the sticky auto result is now recorded as an implementation gap, and every row in the three records now matches what ships or names where shipped code falls short, so the records need no further change. They add new current requirements (Theme FR1–FR4, MediaTheme FR1–FR3, SyntaxTheme FR1–FR2), so approval waits on an engineering owner's decision. An engineering owner records it by commenting /approve-spec <full head SHA> on this pull request, or rejects specific requirements; the author may do so as an engineering owner.
[Automated review]
|
/approve-spec f18de3a |
github-actions
Bot
removed
the
needs:spec-owner-review
Exact decision needed
Record the shipped behavior of the three public theme providers — Theme, MediaTheme, and SyntaxTheme — as new current component records at
docs/specs/components/theme/{Theme,MediaTheme,SyntaxTheme}.spec.md. No behavior, prop, default, or export changes.Existing authority searched
Theme.doc.mjs,MediaTheme.doc.mjs,SyntaxTheme.doc.mjs) in thethemecomponent root; none had a record.check:knowledgeplaces a component record as a direct child of its component root, and component records live underdocs/specs/components/<root>/, so these sit indocs/specs/components/theme/.architecture:theme-application(current) owns provider nesting, root document sync, runtime style lifetime,systemresolution, and what a MediaTheme surface marker changes (INV1–INV9).architecture:theme-compilationowns compiled CSS;architecture:theme-authoring-contractowns syntax theme construction. The new records cite those rules rather than restating them, and own only what each component renders.useThemestays a hook with its own doc and is not a component record.Decision
component:Theme: a layout-neutral wrapper with theme and mode attributes, context, and root<html>sync (FR1–FR4).component:MediaTheme: a layout-neutral wrapper that marks a dark or light surface, measuresautowith a fallback, and never remounts children (FR1–FR3).component:SyntaxTheme: a wrapper carrying syntax variables and the theme name, plus context (FR1–FR2).All
authority: current, approved by cixzhang.Rejected alternatives
None.
Claim scope and non-goals
Theme authoring, tokens, compiled CSS, and surface-token behavior stay with their architecture owners. Claims read from source without a focused test are listed under each verification map.
Evidence and implementation relationship
Theme.test.tsx(wrapper attribute, root and nesteddata-themeanddata-astryx-theme, unmount cleanup),MediaTheme.dom.test.tsx(markers,off, fallback, explicit mode, probe cleanup),useAutoMediaMode.test.ts(surface picking, including no marker where text already reads),defineSyntaxTheme.diff.test.ts, anduseSyntaxTheme.diff.test.tsx. Two current-state gaps: MediaThemeautokeeps the last measured side when a surface becomes unmeasurable, though its docs promisefallback; and the syntax theme hook picks light or dark fromprefers-color-schemerather than the nearest Theme's mode (architecture:theme-applicationINV1), each recorded in its component record.Writing rubric results
Scope
Validation
pnpm check:knowledge: alignedgit diff --check: clean