feat(ui): add an opt-in, agent-first design-system linter - #1479
Conversation
|
Design notes: the class-to-group resolver, opacity stripping, and two scanner calls the issue left open The issue settles the taxonomy (shadcn's The Two scanner calls the issue's D2 did not pin down, both decided toward fewer false positives. A recognised The |
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Read the whole diff plus the scanner, grammar, rules, orchestrator, command, extractor and the eval harness. The scanner is careful (the tag-region rule, the hole-fragment rule, the mute for helper args, position tracking through escapes) and the tests cover the shapes the description claims. Nothing here blocks the opt-in guarantee.
What I would fix before merging:
- Two prefix-ordering bugs in
groupOf:text-shadow-*andbg-blend-*both resolve to a colour group (the dedicated branches are dead), so anallow: ["effects"]never admits them and the messages call them colours. Suggestions inline. --jsonis not JSON on the two early exits (missing / invalidcomponents.json), andrun.mjsturns any non-JSON lint output intocount: 0, so a broken linter reads as a clean eval. Since the harness is the phase-3 gate, that one matters more than its size.- The no-raw-colors message says "error text" for a
border-/ring-/fill-offender.
Smaller: non-class string literals in a class hole are collected as classes (only layout saves them), commented-out markup inside an html template is still scanned, no-restyle names only the first helper of a site, a redundant ternary on cnNames, and in no-raw-colors ordered / list are rebuilt per token and could sit above the loop.
I did not apply any of these (the review-only rule); happy to fold them in on the branch if you want that as a separate ask.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Review of the linter at f7ddfe88, run inline (no reviewer subagents) at high effort for recall. Ten findings, most severe first, each anchored where it fails; the extractor, scanner, grammar and import-list ones were reproduced by running the code against the kit button.ts and small templates.
Not fixed on the branch: the webjs-ui-lint worktree had uncommitted edits landing from another session while this review ran (several of them address the same lines: the text-shadow order, the HTML-comment region, the message wording, the eval double lint, and a --json error document), so applying fixes there would have collided. The findings stand against the pushed head; whichever session owns the worktree can fold them in.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Third pass at 5194c88c, run inline at high effort for recall, after the two earlier rounds were fixed. Every finding below was reproduced against the branch (a probe script over parseToken / scanClassSites / the rules, plus a real webjsui lint run on a scratch copy of gallery with the harness's own LINT_BLOCK). Ten findings, most severe first. The scanner held up on all three in-repo apps (1481 sites, no desync, no odd tokens), so the scanner items here are coverage gaps rather than misreads.
| const before = scratchCopy(); | ||
| const b = agent(before, task.prompt); | ||
| const beforeLint = lint(before); | ||
| result.before = { findings: beforeLint.summary.count, cost: b.cost }; |
There was a problem hiding this comment.
The harness counts findings over the WHOLE scratch app, and gallery is not clean under this exact LINT_BLOCK: a plain webjsui lint on an untouched copy reports 90 warnings (p-6 beside cardClass, text-[13px], no-underline beside buttonClass, ...). So before.findings starts at 90 before the agent writes a byte, the after prompt says "the file you produced" and then lists 90 diagnostics in files the agent never touched, and after.findings === 0 is reachable only by rewriting the gallery. Both correction arms will run to the round cap on every task, so after.meanRounds < rulesOnly.meanRounds and zeroFindings === ok.length cannot come out true and the phase 3 gate measures nothing.
Lint the pristine copy once, keep that as a baseline multiset keyed on file|rule|class (line numbers shift when the agent edits app/page.ts), and have every arm count and print only the violations not covered by the baseline.
| column: token.column, | ||
| class: token.name, | ||
| message, | ||
| ...(roleDeclared ? { fix: `${prefix}-${role}` } : {}), |
There was a problem hiding this comment.
class is the raw token and fix is built from the stripped base, so the pair is not a drop-in replacement. Measured: hover:text-red-600/50 reports class: "hover:text-red-600/50", fix: "text-destructive", and dark:md:bg-gray-100 reports fix: "bg-muted". An agent loop that applies fix over class (the point of a machine-readable field) silently drops the variants, the opacity modifier and a !, turning a hover colour into an always-on one. Rebuild fix from token.name with only the base swapped, so the variants, /50 and ! survive.
| } | ||
| if (close === -1) break; | ||
| const body = css.slice(open + 1, close); | ||
| const declRe = /--color-([A-Za-z0-9_-]+)\s*:/g; |
There was a problem hiding this comment.
The reader is not comment-aware, which breaks the rule's one promise ("never names a token the theme does not declare"). Measured on @theme inline { --color-a: red; /* } */ --color-b: blue; } preceded by a commented-out /* @theme { --color-fake: red; } */: the result is ['fake', 'a']. So a commented-out token (/* --color-old: ...; */, common while iterating on a theme) is offered as text-old, which Tailwind does not generate, and a } inside a comment closes the block early and loses every token after it. Strip /* ... */ from the CSS before matching.
| if (site && mute === 0 && !isComparisonOperand(quoteAt, i)) pushTokens(site, body, start); | ||
| markValue(); | ||
| }; | ||
| const scanPlainTemplate = () => { |
There was a problem hiding this comment.
A plain template literal inside a cn() call or a class hole is lexed for its holes but its static text is never collected, so the backtick spelling evades all three rules. Measured: cn('a', tpl-[3px]) yields only a, and class=${cn(buttonClass(), `w-[9px] ${y}`)} yields no site at all. cn(buttonClass(), bg-pink-500) therefore passes no-restyle, and class=${`p-4 bg-red-500 ${extra}`} passes no-raw-colors, while the docs say every string literal in those two sites is read. When a collector is active and not muted, collect the static runs here with the same hole-fragment rule the attribute site uses.
| if (v === '') return 'border-w'; | ||
| if (BORDER_STYLES.has(v)) return 'border-style'; | ||
| if (v === 'collapse' || v === 'separate') return 'border-collapse'; | ||
| if (v.startsWith('spacing')) return v === 'spacing' ? 'border-spacing' : `border-spacing-${v.slice(8)}`; |
There was a problem hiding this comment.
v.slice(8) is the VALUE, not the axis, so this returns a group id that does not exist: border-spacing-2 resolves to border-spacing-2, border-spacing-x-2 to border-spacing-x-2, border-spacing-[3px] to border-spacing-[3px]. GROUP_CATEGORY has none of them, so the whole family reads as layout instead of spacing, no-arbitrary-values with allow: ["layout"] admits border-spacing-[3px], and the border-spacing* entries in SIMPLE_GROUPS are dead because the border head gets there first.
| if (v.startsWith('spacing')) return v === 'spacing' ? 'border-spacing' : `border-spacing-${v.slice(8)}`; | |
| if (v.startsWith('spacing')) { | |
| const axis = /^spacing-([xy])(?:-|$)/.exec(v); | |
| return axis ? `border-spacing-${axis[1]}` : 'border-spacing'; | |
| } |
| if (/^(?:wrap|nowrap|balance|pretty)$/.test(v)) return 'text-wrap'; | ||
| if (/^(?:ellipsis|clip)$/.test(v)) return 'text-overflow'; | ||
| if (v === 'base' || T_SHIRT.test(v) || isArbitraryLength(v)) return 'font-size'; | ||
| if (/^(?:base|xs|sm|lg|\dxl|xl)\/[\w.]+$/.test(v)) return 'font-size'; |
There was a problem hiding this comment.
The line-height modifier only matches [\w.]+, so the arbitrary and variable spellings fall through to text-color: text-sm/[17px] and text-lg/(--lh) both parse as category color. The report then says "(color, group text-color)" for a font size, allow: ["typography"] does not admit it, and allow: ["color"] wrongly does. The same fallthrough exists under bg- a few lines down: bg-[length:200px] (and the size: / position: / percentage: hints) resolves to bg-color instead of bg-size / bg-position. Accept [...] and (...) after the slash here, and route the typed bg-[...] hints before the bg-color default.
| return 'flex'; | ||
| } | ||
| if (utility.startsWith('mask-')) return 'mask-image'; | ||
| if (utility.startsWith('scrollbar-')) return utility.includes('thumb') ? 'scrollbar-thumb-color' : 'scrollbar-track-color'; |
There was a problem hiding this comment.
This catch-all runs before SIMPLE_GROUPS, so its scrollbar-gutter and scrollbar-w entries are unreachable and scrollbar-gutter-stable resolves to scrollbar-track-color, category color (measured). Upstream places both groups in layout (null), so beside a helper the token is reported under allow: ["layout"] with a message telling the author to allow "color".
| if (utility.startsWith('scrollbar-')) return utility.includes('thumb') ? 'scrollbar-thumb-color' : 'scrollbar-track-color'; | |
| if (utility.startsWith('scrollbar-') && !/^scrollbar-(?:gutter|w)(?:-|$)/.test(utility)) return utility.includes('thumb') ? 'scrollbar-thumb-color' : 'scrollbar-track-color'; |
| const piece = part.trim().replace(/^type\s+/, ''); | ||
| if (!piece) continue; | ||
| const [imported, local = imported] = piece.split(/\s+as\s+/).map((s) => s.trim()); | ||
| if (inUi && /Class$/.test(local)) { helpers.push(local); helperFiles[local] = target; } |
There was a problem hiding this comment.
Recognition keys on the LOCAL name, and the axes lookup in lint/index.js (r.axes[helper]) keys on it too, while extractHelperAxes keys on the EXPORTED name. Measured: import { buttonClass as bc } from '#components/ui/button.ts' recognizes no helper, so everything composed beside bc() passes no-restyle, and import { buttonClass as primaryButtonClass } is recognized but gets the no-value-list fallback message because axes['primaryButtonClass'] is undefined. Test imported against /Class$/ and carry the exported name alongside the file. Related, same regex: import Def, { cn } from '#lib/utils/cn.ts' does not match at all, so that module's cn() calls are never opened.
| /** A colour or typography override reads as a variant; a size-ish one as a size, when the helper has one. */ | ||
| function pickAxis(parsed, axisNames) { | ||
| const cat = parsed.category; | ||
| const sizeLike = cat === null || cat === 'spacing' || cat === 'shape' || cat === 'typography'; |
There was a problem hiding this comment.
The doc comment one line up says a typography override reads as a VARIANT, and this line files all of typography under size. The real gallery run shows the effect: class="${buttonClass()} no-underline" reports "Use a buttonClass size: default, sm, xs, none", and no size touches the underline (the link variant does). Only the font-size and line-height groups are carried by a kit size (text-xs in the registry button's xs), so narrow it to those:
| const sizeLike = cat === null || cat === 'spacing' || cat === 'shape' || cat === 'typography'; | |
| const sizeLike = cat === null || cat === 'spacing' || cat === 'shape' || parsed.group === 'font-size' || parsed.group === 'leading'; |
| if (entry.startsWith('!')) unignore.push(entry.slice(1)); | ||
| else ignore.push(entry); | ||
| } | ||
| const ignoreRes = ignore.map(globToRegExp); |
There was a problem hiding this comment.
An ignore entry is a full-path glob and nothing else, so the directory spellings people write from .gitignore habit match no file and are silent no-ops. Measured with globToRegExp: app/legacy, app/legacy/ and ./app/** all fail to match app/legacy/x.ts, and the negated !components/ui un-ignores nothing. The schema is strict precisely so a config mistake is not silent, and this one is. Normalize each entry (drop a leading ./ and a trailing /) and also match <entry>/**, so a bare directory covers its subtree.
rawConfigSchema is strict, so an app that adds a lint key today makes getConfig throw. The new lintConfigSchema makes the block optional and keeps it strict, so a typo in a rule name or severity is a config error rather than a silent no-op. extractHelperAxes reads the variant and size VALUES a helper exposes from its own source text, which is what a no-restyle message has to name, and it lives beside extractHelperSignatures so the two projections cannot drift.
The grammar parses one token into variants and a utility, decides
arbitrary VALUE against arbitrary VARIANT on whether the utility segment
carries a bracket, and resolves the utility to a class group and to the
category taxonomy transcribed verbatim from shadcn-ui/lint. The scanner
reads class sites from html tagged templates, cn() arguments and
class=${} holes with a hand-rolled lexer, and only inside an open tag, so
an entity-escaped code sample in a docs page is never a site. The theme
reader parses --color-* tokens from both @theme and @theme inline.
…ules Each rule is a pure function over a class site so its tests are a string in and an array out. no-raw-colors names only tokens the app theme declares and adds a role suggestion only when unambiguous. no-restyle names the helper variants and sizes the shared projector reads from the app copy, guarded by a drift test against button.ts, and the skill sanctioned one-off passes under allow layout plus rounded.
The orchestrator walks app, components, modules and lib, skips the resolved ui directory by default (a copied primitive owns structural values no variant expresses and is what other files are measured against), reads the theme once and turns no-raw-colors off with a warning when it yields no tokens. The command mirrors webjs check in report shape and 0/1 exit posture, adds a severity marker, --json and --max-warnings, and reports nothing with no lint block so no existing install changes behaviour.
The skill, the /ui page and the styling docs describe the command and the lint block; styling.md names the allow configuration under which its sanctioned icon-button one-off passes, so the rule and the skill agree. The three-arm eval under packages/ui/test/evals runs on demand and is the gate before any surface tells an agent to run the linter.
The wrapper spawns the ui bin with the arguments verbatim, so lint already dispatches; only the usage string and the two command tables named the old six.
A longer head is matched before the prefix it starts with, so text-shadow and bg-blend resolve to their own groups. The projector blanks comments before matching braces, so a comment in an app copy of a variant map cannot swallow the value list. A comparison operand or a case label in a class hole is not a class, commented-out markup opens no tag, and a comment inside an import list is not a binding. A negated ignore entry un-ignores what it matches rather than one exact string, --json answers the early exits with an error document, a non-integer --max-warnings is refused, the role sentence names the surface by prefix, and no-restyle names every composed helper. The eval harness throws on a run that yields no document and lints once per round. The command test pins the blog shape inline instead of live line numbers.
The scanner missed static text in plain template literals inside cn() and class holes, keyed helper recognition on the local import name so an aliased or default-plus-named import went unseen, and kept its html lexer state into a hole so a leading regex or template was misread. The grammar filed border-spacing, scrollbar-gutter, and slash or typed text and bg values under the wrong category, which made the allow list admit or reject the wrong classes. The reported fix dropped variants, opacity, and the important flag, the theme reader treated commented-out tokens as live, no-restyle named the size axis for any typography class, and directory-style ignore entries matched nothing without saying so.
The pristine gallery copy already reports about 90 warnings under the harness's own lint block, so a whole-app count started every arm far above zero and the gate could never pass. Each arm now subtracts a baseline taken from the pristine copy, keyed without positions because an edit shifts the lines below it.
ce47ea1 to
5e9ca9e
Compare
1 participant
Closes #1478
Adds
webjsui lint, an opt-in design-system linter in@webjsdev/ui. It reads the Tailwind classes an app writes insidehtmltemplates,cn()calls andclass=${...}holes, and reports at the exact line where the app drifts off its own theme, with a message built from the app's real tokens and helper variants. Three rules ship (no-raw-colors,no-arbitrary-values,no-restyle), each off unless alintblock incomponents.jsonturns it on, so no existing install changes behaviour.What changed
rawConfigSchemaaccepts an optional, strictlintblock (it is.strict(), so alintkey threw before).extractHelperAxesin the shared projector reads a helper's variant and size values from source, so ano-restylemessage names what the app's copy actually declares. Drift-guarded against the kit button.htmltagged templates,cn()arguments andclass=${...}holes. Aclass=is a site only inside an open tag, which is what keeps the entity-escaped code sample in the file-storage docs page from being reported.shadcn-ui/lintsoallow: ["layout"]means the same thing in both tools.@themeand@theme inline. No tokens turnsno-raw-colorsoff for the run with one warning naming the path.webjsui lintcommand: awebjs check-shaped text report with a severity marker,--json,--max-warnings, and 0/1 exit codes.components/ui/**is skipped by default; a negated ignore entry ("!components/ui/**") widens the scope.packages/ui/test/evals/(before, after with diagnostics, rules-only control), run on demand through theclaudeCLI. It is the gate before any surface tells an agent to run the linter.Against the blog with a
lintblock, all fourtext-red-600feedback lines are reported and the escaped docs sample is not.Deliberately excluded
webjs check, which stays correctness-only.@webjsdev/serveror@webjsdev/mcp.Test plan
npm test --workspace=@webjsdev/ui(275 pass after the review fixes) and rootnpm test(4688 of 4694; the 5 failures are the known linked-worktree set, listener, listener-overhead and three elision assertions, which pass in the primary and in CI)no-raw-colorsat line 29 and clears withtext-destructive(lint-command.test.js, proven atcdea21cc, re-proven at5194c88cwith the shape pinned inline)webjs checkpasses in gallery, examples/blog and website;webjs doctorin website has 0 failures/uiand/docs/stylingat 200, 12 preloads each, none broken, both carrying the new contentnode:fsandnode:path, already used throughoutpackages/ui/src)Note for a linked worktree:
webjs ui lintthrough the CLI wrapper resolves@webjsdev/uiinto the primary checkout, so it reportsunknown command 'lint'there. The wrapper spawns the ui bin with the arguments verbatim, andnode packages/ui/bin/webjsui.js lintfrom the branch works.Docs
packages/ui/README.md(command row),packages/ui/AGENTS.md(lede, module map, command row, a section on the config, rules, taxonomy, scope and phases).agents/skills/webjs/references/ui-kit.md(the command and its block),references/styling.md(theallow: ["layout", "rounded"]sentence at the sanctioned one-off)website/app/ui/page.ts(command row),website/app/docs/styling/page.ts(a subsection at#lint)AGENTS.mdCLI reference,packages/cli/AGENTS.mdand thewebjs uiusage string listlintcreate.js, MCP, editor plugins: N/A in phase 1 by design (no scaffold wiring until the gate), and the MCPuitool projects the kit, not the linter