test(skill): guard colour values and CSS custom properties in examples - #533
Merged
Conversation
Closes #345, starting where the issue says to start — colours, then CSS custom properties as the cheapest sibling. Prop names are the harder half and are left for a separate pass, as the issue itself notes. **Both guards found a live defect, which is the argument for having them.** `data-tables.md` shipped `color="neutral"` on `B24Badge`. That was in #345's own table, was fixed by hand in #343 to `air-secondary-no-accent` — a key `Button` declares and `Badge` does not — and sat wrong for another month. Measured by rendering it: a valid colour carries `style-tinted`, the default carries `style-filled`, and this one carries no `style-*` class at all. The Inactive badge has been rendering uncoloured the whole time. Now `air-tertiary`, which `Badge` does declare. `forms.md` reached for `--ui-color-typography-secondary` to label a `role="group"`. It is declared nowhere, and there is no `--ui-color-typography-*` family in `src/` at all. Replaced with `text-legend`, the semantic utility the skill's own design-system guideline describes as being for group titles — and which `src/theme/timeline.ts` and `src/theme/dropdown-menu.ts` already use. That is the pattern #345 describes: a name inside a fenced block, contradicted by `src/`, that renders without complaint. The colour one is worse than the issue knew, because the manual fix for it was also wrong. **The checks.** Both follow the shape of the existing component and icon checks — read the truth from the tree, anchor on a positive and a negative sentinel, then assert the scan finds nothing. - Colours are read from each `src/theme/*.ts` `color` variant map by brace matching, not a line regex, because the map nests and a flat scan picks up slot names from inside it. Static `color="x"` attributes are checked, and so are quoted literals inside a bound `:color="…"` — which is where the Badge defect actually lived, in a ternary rather than an attribute. - CSS custom properties are matched only where they are *used*: inside `var(--x)` or Tailwind's `text-(--x)`. The closing paren is what separates a real reference from the placeholders the prose writes — `--ui-color-base-N`, `…-bg-gradient-{1,2,3}`, `…-bg-content-*` — which otherwise all read as phantom tokens. Without it the check reports five false positives and one real one. Each was verified to go red on its own defect: restoring `air-secondary-no-accent` and `--ui-color-typography-secondary` fails exactly the matching check, naming the file and the value. Re-verified after `eslint --fix` rewrote one of the regexes. Refs #345 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
`/review` found six holes in the first commit. Four were coverage gaps that made the checks weaker than their own comments said; one was a false positive that would have failed a legitimate example; one was a wrong statement in a comment. Each fix was probed by injecting the case and confirming the check goes red. **Colours** - `colourKeys` hardcoded a four-space indent. A factory theme nests the map one level deeper — `input-number.ts` declares it at six — so the map was found and *zero* keys were read out of it, silently dropping the component. The indent is now derived from the match, and key depth with it. - The tag pattern was `[^>]*`, which ends the tag at the first `>` inside an attribute value. `<B24Badge v-if="a > b" color="bogus" />` passed green. Quoted values are now skipped over rather than scanned for tag ends. - The `:color` scan flagged every lowercase quoted string in the expression, so a comparison operand — `row.status === 'active'` — failed as an unknown colour. Only ternary branches are read now. The shipped example survived only because its operand happens to be capitalised. - `active-color` was dismissed in a comment as an "other prop". It takes the same map (`Button['variants']['color']`, and `ButtonProps['color']` on `Pagination` and `EditorToolbar`). Both attributes are checked now. **CSS custom properties** - The pattern matched only the bare `(--x)` form, missing `var(--x, fallback)` and Tailwind's typed arbitrary properties `text-(length:--x)` / `font-(family-name:--x)`. That is 27 of the references in this package — including the untouched half of the very line the phantom token was found on, `text-(length:--ui-font-size-sm)`. - Worse, the comment cited `--b24ui-header-heights` as a defect this check guards, and it does not: that one is written as a *declaration*, which the usage-only pattern cannot see. Declarations of `--b24ui-*` / `--ui-*` are now checked too — a reader defining `--my-thing` is left alone, but writing the library's namespace is a claim the library reads it. `--b24ui-header-height` and its plural are now the check's sentinels. Probes, all confirmed: a bogus `B24InputNumber` colour, a bogus `active-color`, a `>` inside an attribute, a phantom typed token, a phantom token inside `var(… , fallback)` and a phantom declaration each fail the matching check — and `row.status === 'active'` stays green. Refs #345 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked issue
Resolves #345
Type of change
revert(Scope): ...)Description
Two new checks in
test/utils/skill-manifest.spec.ts, and the two live defects they found. Starts where #345 says to start — colours, then CSS custom properties as the cheapest sibling. Prop names are the harder half and are left for a separate pass, as the issue itself notes.Both guards found something on their first run, which is the argument for having them at all.
The two defects
B24Badgehas been rendering uncoloured for a monthdata-tables.mdshippedcolor="neutral"onB24Badge. That is in #345's own table. It was fixed by hand in #343 toair-secondary-no-accent— a keyButtondeclares andBadgedoes not — and sat wrong ever since.Measured by rendering all three:
style-*classstyle-filledair-secondary(valid)style-tintedair-secondary-no-accent(shipped)The Inactive badge has no colour class at all. Nothing throws, nothing warns. Now
air-tertiary, whichBadgedoes declare (style-outline-no-accent— the muted outline that suits an inactive status next to a filled green one).The manual fix for the defect in the issue was itself wrong. That is the case for doing this mechanically.
A phantom CSS token
forms.mdusedtext-(--ui-color-typography-secondary)to label arole="group". It is declared nowhere, and there is no--ui-color-typography-*family insrc/at all. Replaced withtext-legend— the semantic utility the skill's own design-system guideline describes as being for group titles, and whichsrc/theme/timeline.tsandsrc/theme/dropdown-menu.tsalready use.The checks
Both follow the shape the file already uses: read the truth from the tree, anchor on a positive and a negative sentinel, then assert the scan finds nothing.
Colours come from each
src/theme/*.tscolorvariant map, found by brace matching rather than a line regex — the map nests, and a flat scan picks up slot names from inside it. Staticcolor=/active-color=attributes are checked, and so are the ternary branches of a bound:color=, which is where the Badge defect actually lived.CSS custom properties are checked in every shape the skill writes:
var(--x),var(--x, fallback),text-(--x), and the typed arbitrary propertiestext-(length:--x)/font-(family-name:--x)— plus declarations, since writing--b24ui-*in your own CSS is a claim the library reads it.Prose placeholders are deliberately not flagged:
--ui-color-base-N,…-bg-gradient-{1,2,3}and…-bg-content-*are documentation, not references, and requiring a closing)or,separates them cleanly. Without that the check reports five false positives to one real finding.What
/reviewcaughtSix holes in the first draft. Four made the checks weaker than their own comments claimed, one was a false positive, one was a wrong statement:
colourKeyshardcoded a 4-space indentinput-number.tsnests its map at 6 — map found, zero keys read, component silently unchecked[^>]*<B24Badge v-if="a > b" color="bogus" />passed green:colorflagged every quoted stringrow.status === 'active'would fail as an unknown colouractive-colordismissed as an "other prop"(--x)--b24ui-header-heightsAll fixed, and each fix probed by injecting the case:
The two original defects were also each restored and confirmed to fail exactly their matching check, naming the file and the value — and re-verified after
eslint --fixrewrote one of the regexes.Checks
pnpm lint,pnpm typecheckgreenChecklist
🤖 Generated with Claude Code
https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
Generated by Claude Code