Repository navigation
docs(spec): Indicator records its shipped contract as current - #9081
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis ReportView Storybook for this PR · View Sandbox for this PR
Generated by PR Enrichment workflow | View current CI run |
There was a problem hiding this comment.
These three Indicator records need a few corrections before they can become current. After the fixes, an engineering owner records approval by commenting /approve-spec <full head SHA> on this pull request; the author may do this as an engineering owner.
Indicator.spec.mdsays a theme'sindicatorsentry "replaces it everywhere" (line 39, FR2), butTokenizerBottomSheet.tsxand Lab'sMobileTokenizerandMobilePowerSearchValueEditorrender the built-ins directly. Scope the claim and list those hosts under Current-state impact.- Three verification rows (
CheckIndicator.spec.md:143,CheckboxIndicator.spec.md:149,Indicator.spec.md:167) credit tests that never exercise glyph-pathclassName/style/xstyle, checkboxdata-disabled, or radiodata-sizeandref. List these as evidence gaps. CheckIndicator.spec.md:71hands radio chrome tocomponent:RadioIndicator, which does not exist. Namecomponent:Indicatorinstead.
[Automated review]
| | Area | Contract | | ||
| | ----------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | Public contract | None changes. `@astryxdesign/core/Indicator` exports the three built-in indicators (`CheckboxIndicator`, `CheckIndicator`, `RadioIndicator`), the shared indicator props and types, the default registry, and name-based resolution. | | ||
| | Behavior | Hosts draw a decorative control visual by name (`check`, `checkbox`, `radio`). A theme's `indicators` entry for that name replaces it everywhere; otherwise the built-in renders. Every indicator is hidden from assistive technology. | |
There was a problem hiding this comment.
"Replaces it everywhere" doesn't hold for every shipped host. TokenizerBottomSheet.tsx (Core, not yet exposed) and Lab's MobileTokenizer and MobilePowerSearchValueEditor import CheckboxIndicator/RadioIndicator directly, so a theme's checkbox or radio entry never reaches them. Please scope this row and FR2 to hosts that resolve by name, and list these under Current-state impact.
| | ------------- | ------------------------------------------------------------------------------------------------------ | ----------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------ | | ||
| | FR1, FR2, FR3 | `Indicator.test.tsx` CheckIndicator state and replacement cases; falsy-children cases | Checked, unchecked, empty scalar, visible child, `0` | Removing the glyph, adding unchecked chrome, or choosing the wrong children branch fails. | `audit:CheckIndicator/behavior` | | ||
| | FR1, AR1 | `Indicator.test.tsx` decorative-contract cases | Glyph and replacement roots; a caller `aria-hidden={false}` | A root that a caller can expose fails. Evidence gap: no test asserts that replacement content is not the only state expression (AR2). | `audit:CheckIndicator/accessibility` | | ||
| | FR4 | `Indicator.test.tsx` "forwards caller props and styling on both render paths" | Both render paths | A caller prop, `className`, `style`, or `xstyle` that stops reaching a rendered root fails. Evidence gap: no test asserts the ref on either path (the checked-path ref is the FR4 gap). | `audit:CheckIndicator/api` | |
There was a problem hiding this comment.
This test never passes style or xstyle, and it passes className only on the children path. Removing those props from CheckIndicator.tsx:166-170 (and xstyle/style at :127-130) leaves every test passing. Please list them as evidence gaps, as CheckboxIndicator's FR4 row does.
| | FR1, AR1 | `Indicator.test.tsx` decorative-contract cases | Every built-in path; a caller `aria-hidden={false}` | A root that a caller can expose fails. Evidence gap: no test asserts that replacement content is not the only state expression (AR3). | `audit:CheckboxIndicator/accessibility` | | ||
| | FR2, FR3 | `Indicator.test.tsx` default-indicator, replacement, and falsy-children cases | Unchecked, checked, indeterminate, empty scalar, visible child | A missing state mark or the wrong children branch fails. | `audit:CheckboxIndicator/behavior` | | ||
| | FR4 | `Indicator.test.tsx` decorative-contract "still forwards ordinary props" cases | Neutral DOM props | A dropped DOM prop fails. Evidence gap: no test asserts `ref`, `className`, `style`, or `xstyle` on the root. | `audit:CheckboxIndicator/api` | | ||
| | FR5, FR6 | `Indicator.test.tsx` checkbox-target, disabled-state, and canonical-target cases; `themingTargets.test.ts` | Canonical target; size, checked, disabled | A missing or aliased target, or a missing reflected state, fails. Evidence gap: no test asserts the `checkbox-indicator-check` and `checkbox-indicator-dash` targets. | `audit:CheckboxIndicator/theming` | |
There was a problem hiding this comment.
The only disabled-state case (Indicator.test.tsx:66-73) renders RadioIndicator, and themingTargets.test.ts only checks that documented targets are a superset of rendered ones. Removing disabled: from CheckboxIndicator.tsx:205 still passes. Please add the checkbox data-disabled gap here.
| | FR1, FR2 | `indicatorRegistry.test.tsx`; `Indicator.test.tsx` useIndicator suite | No theme; theme override; augmented names | A wrong default, an ignored override, or a resolved unsupplied name fails. | `audit:Indicator/behavior` | | ||
| | FR3 | `Indicator.test.tsx` decorative-contract suite | Caller `aria-hidden={false}`; spread props | An exposed indicator or a dropped ordinary prop fails. | `audit:Indicator/accessibility` | | ||
| | FR4 | `Indicator.test.tsx` falsy-children suite | `null`, `false`, `''`, `0`, real children | A falsy child that suppresses the mark, or `0` treated as empty, fails. | `audit:Indicator/behavior` | | ||
| | FR5, FR6, AR2 | `Indicator.test.tsx` radio-dot, disabled, and target-name cases | Checked and unchecked; disabled; canonical targets | A dot when unchecked, a missing state attribute, or an alias target fails. Evidence gap: no test asserts the radio sizes, the disabled opacity and border, the hover tint, the forced-colors dot (the RadioList forced-colors case passes on any matching rule), or caller `className`, `style`, and `xstyle` on the root. | `audit:Indicator/theming` | |
There was a problem hiding this comment.
FR6 cites tests for radio data-size (root and dot) and ref, but no test asserts any of them. Please add both to this gap list.
…arrows evidence claims
There was a problem hiding this comment.
Thanks for scoping FR2 to by-name hosts and tightening the verification rows; the earlier findings are fixed. These records still add requirements that are not yet current, including FR2's new rule that every host MUST resolve indicators by name, so approval waits on an engineering owner commenting /approve-spec <full head SHA> on this pull request (the author may do this as an engineering owner) or saying which requirement should change. Two record fixes are needed either way:
CheckboxIndicator.spec.md:150: the cited forced-colors tests still pass with both CanvasText rules removed fromCheckboxIndicator.tsx, so list that as an evidence gap, asIndicator.spec.mddoes for the radio dot.Indicator.spec.md:102:architecture:theme-authoring-contractdoes not say hosts must resolve indicators by name; record the rule as a component-local decision, cite a claim that states it, or keep FR2 descriptive.
[Automated review]
| | FR2, FR3 | `Indicator.test.tsx` default-indicator, replacement, and falsy-children cases | Unchecked, checked, indeterminate, empty scalar, visible child | A missing state mark or the wrong children branch fails. | `audit:CheckboxIndicator/behavior` | | ||
| | FR4 | `Indicator.test.tsx` decorative-contract "still forwards ordinary props" cases | Neutral DOM props | A dropped DOM prop fails. Evidence gap: no test asserts `ref`, `className`, `style`, or `xstyle` on the root. | `audit:CheckboxIndicator/api` | | ||
| | FR5, FR6 | `Indicator.test.tsx` checkbox-target and canonical-target cases; `themingTargets.test.ts` | Canonical target; size and checked | A missing or aliased target, or a missing size or checked reflection, fails. Evidence gap: no test asserts checkbox `data-disabled` (the disabled-state case renders RadioIndicator) or the `checkbox-indicator-check` and `checkbox-indicator-dash` targets. | `audit:CheckboxIndicator/theming` | | ||
| | FR7, AR2 | `CheckboxInput.test.tsx` forced-colors cases ("compiles a forced-colors fill so the indeterminate mark survives Windows High Contrast", "compiles a forced-colors color so the checkmark survives Windows High Contrast") | Checked and indeterminate under forced colors | A mark that loses its forced-colors rule fails. Evidence gap: no test asserts the reduced-motion transition. | `audit:CheckboxIndicator/visual` | |
There was a problem hiding this comment.
Both cited forced-colors cases still pass with the two '@media (forced-colors: active)': 'CanvasText' rules removed from CheckboxIndicator.tsx (checkmark and dash): getForcedColorsRules() matches any injected rule, and other loaded components emit the same declarations. Please add this to the row's evidence gap, as Indicator.spec.md does for the radio dot ("passes on any matching rule").
| | ID | Invariant | Basis | | ||
| | --- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------- | | ||
| | FR1 | The default registry maps exactly `check` → CheckIndicator, `checkbox` → CheckboxIndicator, and `radio` → RadioIndicator. | Current tests | | ||
| | FR2 | A host resolves an indicator by name from the nearest theme: the theme's `indicators` entry for that name wins, otherwise the built-in. One override reaches every host that resolves that name. Hosts MUST resolve indicators by name so a theme override reaches them (`architecture:theme-authoring-contract`). Implementation gap: the Tokenizer bottom sheet and the Lab MobileTokenizer and MobilePowerSearch value editor render CheckboxIndicator and RadioIndicator directly, so a theme's `checkbox` or `radio` entry never reaches them. An augmented name resolves only when a theme supplies it. | `architecture:theme-authoring-contract`; current tests | |
There was a problem hiding this comment.
architecture:theme-authoring-contract covers theme input and normalization ("An explicit icon, indicator, or componentIcons entry replaces the inherited entry for the same key whole") and says nothing about hosts resolving by name, and the Decision log says "No component-local decision". Please record "Hosts MUST resolve indicators by name" as a component-local decision, cite a claim that states it, or keep FR2 descriptive.
… forced-colors evidence
There was a problem hiding this comment.
Thanks for keeping FR2 descriptive and listing the forced-colors gap; both earlier findings are fixed. These records still add requirements that are not yet current, so approval waits on an engineering owner commenting /approve-spec <full head SHA> on the commit that carries the final text (the author may do this as an engineering owner) or saying which requirement should change. Two record fixes are needed first:
Indicator.spec.md:120still requires every host to draw a theme override, and:43rejects an override that some hosts ignore, while FR2 now records three hosts it does not reach; scope both to hosts that resolve by name, or restore the rule in FR2.Indicator.spec.md:167does not list FR5's radio state colours or reduced motion as evidence gaps, though the cited tests still pass with both broken inRadioIndicator.tsx.
[Automated review]
| | State | Required invariant | Allowed variation | | ||
| | ----------------- | -------------------------------------------- | ----------------- | | ||
| | No theme override | Built-ins render for each name. | — | | ||
| | Theme override | Every host draws the override for that name. | Override design. | |
There was a problem hiding this comment.
This row requires every host to draw the override, but FR2 (line 102) now records that the Tokenizer bottom sheet and the Lab MobileTokenizer and MobilePowerSearch value editor do not. Please scope it to hosts that resolve the name (and the Review checks line 43 to match), or restore the stricter rule in FR2 with a decider.
| | FR1, FR2 | `indicatorRegistry.test.tsx`; `Indicator.test.tsx` useIndicator suite | No theme; theme override; augmented names | A wrong default, an ignored override, or a resolved unsupplied name fails. | `audit:Indicator/behavior` | | ||
| | FR3 | `Indicator.test.tsx` decorative-contract suite | Caller `aria-hidden={false}`; spread props | An exposed indicator or a dropped ordinary prop fails. | `audit:Indicator/accessibility` | | ||
| | FR4 | `Indicator.test.tsx` falsy-children suite | `null`, `false`, `''`, `0`, real children | A falsy child that suppresses the mark, or `0` treated as empty, fails. | `audit:Indicator/behavior` | | ||
| | FR5, FR6, AR2 | `Indicator.test.tsx` radio-dot, disabled, and target-name cases | Checked and unchecked; disabled; canonical targets | A dot when unchecked, a missing state attribute, or an alias target fails. Evidence gap: no test asserts the radio sizes, the disabled opacity and border, the hover tint, the forced-colors dot (the RadioList forced-colors case passes on any matching rule), radio `data-size` on the root and dot, `ref`, or caller `className`, `style`, and `xstyle` on the root. | `audit:Indicator/theming` | |
There was a problem hiding this comment.
FR5's state colours (emphasized border, accent fill, on-accent dot) and "Transitions are instant under reduced motion" are untested too: with the reduced-motion rule removed and the checked border, fill and dot recoloured in RadioIndicator.tsx, every cited test still passes. Please add both to this row's evidence gap, as CheckboxIndicator's FR7 row does for reduced motion.
… lists radio paint gaps
There was a problem hiding this comment.
Thanks for scoping the override statements to hosts that resolve by name and listing the radio paint gaps; both earlier findings are fixed. These records still add requirements that are not yet current, so approval waits on an engineering owner commenting /approve-spec <full SHA> on the commit that carries the final text (the author may do this as an engineering owner) or saying which requirement should change. One record fix is needed first:
CheckboxIndicator.spec.md:147says a missing state mark fails the cited tests, but they still pass with the checkmark and dash swapped inCheckboxIndicator.tsx; narrow the failure signal and list which mark shows in each state as an evidence gap.
[Automated review]
| | Contract | Verification | Representative states | Mutation or failure expectation | Audit section | | ||
| | --------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------- | | ||
| | FR1, AR1 | `Indicator.test.tsx` decorative-contract cases | Every built-in path; a caller `aria-hidden={false}` | A root that a caller can expose fails. Evidence gap: no test asserts that replacement content is not the only state expression (AR3). | `audit:CheckboxIndicator/accessibility` | | ||
| | FR2, FR3 | `Indicator.test.tsx` default-indicator, replacement, and falsy-children cases | Unchecked, checked, indeterminate, empty scalar, visible child | A missing state mark or the wrong children branch fails. | `audit:CheckboxIndicator/behavior` | |
There was a problem hiding this comment.
This row says "A missing state mark … fails", but CheckboxIndicator renders both marks in every state and only toggles their display (CheckboxIndicator.tsx:222-250); the cited tests check that an svg exists and read data-checked. With the checkmark and dash swapped (:229, :247), every cited test still passes. Please narrow the failure signal to a removed mark element or the wrong children branch, and add the gap: no test asserts which mark is visible in each state.
There was a problem hiding this comment.
Thanks for dropping the claim that the visible mark in each state is tested; that finding is fixed. These records still add requirements that are not yet current, so approval waits on an engineering owner commenting /approve-spec <full SHA> on the commit that carries the final text (the author may do this as an engineering owner) or saying which requirement should change. Three verification rows also need narrowing so their failure signals match what the tests catch:
CheckboxIndicator.spec.md:147: deleting the dash<span>leaves every cited test passing, so say "a removed checkmark element" and list the dash as a gap.CheckboxIndicator.spec.md:149: onlydata-checked="indeterminate"is tested, so a checked box that losesdata-checkedstill passes; list checked-state reflection as a gap.Indicator.spec.md:166: the0case runs on CheckIndicator only, so RadioIndicator treating0as empty still passes; scope the signal and list RadioIndicator's case as a gap.
[Automated review]
| | Contract | Verification | Representative states | Mutation or failure expectation | Audit section | | ||
| | --------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------- | | ||
| | FR1, AR1 | `Indicator.test.tsx` decorative-contract cases | Every built-in path; a caller `aria-hidden={false}` | A root that a caller can expose fails. Evidence gap: no test asserts that replacement content is not the only state expression (AR3). | `audit:CheckboxIndicator/accessibility` | | ||
| | FR2, FR3 | `Indicator.test.tsx` default-indicator, replacement, and falsy-children cases | Unchecked, checked, indeterminate, empty scalar, visible child | A removed mark element or the wrong children branch fails. Evidence gap: both marks render in every state with their display toggled, and no test asserts which mark is visible in each state. | `audit:CheckboxIndicator/behavior` | |
There was a problem hiding this comment.
"A removed mark element … fails" holds for the checkmark (the falsy-children cases select svg), but no cited test looks for the dash: with the dash <span> deleted from CheckboxIndicator.tsx:241-250, every test in the cited files still passes. Please say "A removed checkmark element" and add the dash element to this row's evidence gap.
| | FR1, AR1 | `Indicator.test.tsx` decorative-contract cases | Every built-in path; a caller `aria-hidden={false}` | A root that a caller can expose fails. Evidence gap: no test asserts that replacement content is not the only state expression (AR3). | `audit:CheckboxIndicator/accessibility` | | ||
| | FR2, FR3 | `Indicator.test.tsx` default-indicator, replacement, and falsy-children cases | Unchecked, checked, indeterminate, empty scalar, visible child | A removed mark element or the wrong children branch fails. Evidence gap: both marks render in every state with their display toggled, and no test asserts which mark is visible in each state. | `audit:CheckboxIndicator/behavior` | | ||
| | FR4 | `Indicator.test.tsx` decorative-contract "still forwards ordinary props" cases | Neutral DOM props | A dropped DOM prop fails. Evidence gap: no test asserts `ref`, `className`, `style`, or `xstyle` on the root. | `audit:CheckboxIndicator/api` | | ||
| | FR5, FR6 | `Indicator.test.tsx` checkbox-target and canonical-target cases; `themingTargets.test.ts` | Canonical target; size and checked | A missing or aliased target, or a missing size or checked reflection, fails. Evidence gap: no test asserts checkbox `data-disabled` (the disabled-state case renders RadioIndicator) or the `checkbox-indicator-check` and `checkbox-indicator-dash` targets. | `audit:CheckboxIndicator/theming` | |
There was a problem hiding this comment.
The only reflection case renders state="indeterminate" (Indicator.test.tsx:23-32). With CheckboxIndicator.tsx:200-201 changed so a checked box carries no data-checked, every test still passes. Please name the indeterminate value as what is tested and add checked-state data-checked to the gap list.
| | --------------- | ---------------------------------------------------------------------------------------------------------- | -------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------- | | ||
| | FR1, FR2 | `indicatorRegistry.test.tsx`; `Indicator.test.tsx` useIndicator suite | No theme; theme override; augmented names | A wrong default, an ignored override, or a resolved unsupplied name fails. | `audit:Indicator/behavior` | | ||
| | FR3 | `Indicator.test.tsx` decorative-contract suite | Caller `aria-hidden={false}`; spread props | An exposed indicator or a dropped ordinary prop fails. | `audit:Indicator/accessibility` | | ||
| | FR4 | `Indicator.test.tsx` falsy-children suite | `null`, `false`, `''`, `0`, real children | A falsy child that suppresses the mark, or `0` treated as empty, fails. | `audit:Indicator/behavior` | |
There was a problem hiding this comment.
The 0 case renders only CheckIndicator (Indicator.test.tsx:203-211). With RadioIndicator's guard at RadioIndicator.tsx:183 changed to plain truthiness, so 0 keeps the dot, every test still passes. Please scope "0 treated as empty, fails" to CheckIndicator and list RadioIndicator's 0 case as a gap.
There was a problem hiding this comment.
The three Indicator records now hold up: the earlier verification-row fixes are in, and every failure signal and evidence gap they list matches what the cited tests catch. Approval waits only on an engineering owner approving the spec content (the new component:Indicator record and the promotion of component:CheckIndicator and component:CheckboxIndicator to current) by commenting /approve-spec <full SHA> on the commit that carries the final text; the author may do this as an engineering owner, or an owner can say which requirement should change.
[Automated review]
|
/approve-spec 833099c |
Exact decision needed
Record
component:Indicatorand promotecomponent:CheckIndicatorandcomponent:CheckboxIndicatorfromdrafttocurrent, recording their shipped behavior. No behavior, API, default, or DOM change.Existing authority searched
pnpm specs:for packages/core/src/Indicator/indicatorRegistry.tsand the three built-ins → current records that govern them:architecture:component-theming-surface,architecture:public-component-api,architecture:theme-authoring-contract, andcomponent:Iconfor the check glyph.CheckIndicator.spec.mdandCheckboxIndicator.spec.mdexisted asdraft; nocomponent:Indicatorrecord existed.packages/core/src/Indicator/*.spec.mdrecord except the repository-wide schema migration.Decision
Three current records (
approved_by: cixzhang,approved_at: 2026-10-31), one per root.doc.mjsplus the two existing sub-component records:Indicator.spec.md(new): the indicator names and default registry, theme replacement by name, the shared decorative prop contract, and RadioIndicator with its targets.CheckIndicator.spec.mdandCheckboxIndicator.spec.md(promoted, existing content kept): each built-in's state marks, replacement content, decorative ownership, pass-through, and targets. Verification maps now name only real tests and list evidence gaps.Rejected alternatives
None.
Claim scope and non-goals
Shipped behavior only, traced from
indicatorRegistry.ts,useIndicator.ts, the three built-ins,Indicator.doc.mjs,Indicator.test.tsx,indicatorRegistry.test.tsx, and the CheckboxInput forced-colors cases. No requirement is added; no code is fixed.useIndicatorandgetIndicatorstay uncontracted as API beyond the resolution the record states.Evidence and implementation relationship
indicatorRegistry.test.tsxand theuseIndicatorcases.Indicator.test.tsx.CheckboxInput.test.tsxforced-colors cases.Current-state impact:
2. Indicator: the Tokenizer bottom sheet and the Lab MobileTokenizer and MobilePowerSearch value editor render CheckboxIndicator and RadioIndicator directly, so a theme override by name does not reach them.
nullon the checked-glyph path, againstarchitecture:public-component-apiINV8 (OQ1).Writing rubric results
Scope
Validation
pnpm check:knowledge: alignedgit diff --check: clean