Skip to content

feat(lint): add cta-hierarchy-mismatch rule to lintArtifact#2287

Open
EthanGuo-coder wants to merge 8 commits into
nexu-io:mainfrom
EthanGuo-coder:feat/issue-2251
Open

feat(lint): add cta-hierarchy-mismatch rule to lintArtifact#2287
EthanGuo-coder wants to merge 8 commits into
nexu-io:mainfrom
EthanGuo-coder:feat/issue-2251

Conversation

@EthanGuo-coder
Copy link
Copy Markdown
Contributor

Closes #2251

Why

Adds a cta-hierarchy-mismatch lint rule so commerce CTAs in header/nav/hero regions that use a non-primary button class are flagged as a P1 advisory.

What users will see

A new advisory finding cta-hierarchy-mismatch (severity P1) appears in the artifact lint badge when a <header>, <nav>, or <section class="hero"> contains a commerce CTA (Buy / Purchase / Checkout / Order now / Get started / Shop now / Add to cart / Subscribe / Sign up / Start free / Book now / Reserve) whose class list omits the artifact's detected primary button class. P1 surfaces as a warning in the chat UI and does not gate emit — outline CTAs in the header are a legitimate design choice. No change to existing findings, no new exports, no API surface change.

Surface area

  • UI — new page / dialog / panel / menu item / setting / empty state in apps/web or apps/desktop (including Electron menu bar)
  • Keyboard shortcut — new or changed
  • CLI / env var — new od subcommand or flag, new tools-dev / tools-pack / tools-pr flag, or new OD_* env var
  • API / contract — new /api/* endpoint, new SSE event, or changed shape in packages/contracts
  • Extension point — new entry under skills/, design-systems/, design-templates/, or craft/, or change to the skills protocol
  • i18n keys — added new translation keys (see TRANSLATIONS.md for the locale workflow)
  • New top-level dependency — adding any new entry to the root package.json (dependencies or devDependencies); workspace-package package.json files are out of scope. Include a paragraph on what we get vs. what bytes we ship (see CONTRIBUTING.md → Code style)
  • Default behavior change — changes what existing users experience without opting in (default model, default setting, file/SQLite schema, auto-network on startup, auto-install)
  • None — internal refactor, docs, tests, or translation update only

Daemon-internal: one rule block + five private helpers inside the existing apps/daemon/src/lint-artifact.ts seam (the maintainer-identified home for new lint rules). No new file, no new export, no contract change. The finding rides the existing POST /api/artifacts/lint response.

Validation

  • pnpm --filter @open-design/daemon test -- lint-artifact.test — 109 tests pass (was 96, +13 new for cta-hierarchy-mismatch).
  • pnpm --filter @open-design/daemon typecheck — clean.
  • pnpm guard — clean.

Flags commerce CTAs in <header>/<nav>/<section.hero> whose class list omits the artifact's detected primary button class; emits a single advisory P1 finding per artifact.

Closes nexu-io#2251
@lefarcen lefarcen requested a review from nettee May 19, 2026 16:17
@lefarcen lefarcen added size/L PR changes 300-700 lines risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps type/feature New feature labels May 19, 2026
Copy link
Copy Markdown
Contributor

@nettee nettee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a few correctness issues in the new CTA hierarchy lint that can produce false positives or unstable counts. Details are inline.

🔁 Powered by Looper · runner=reviewer · agent=opencode · An autonomous AI dev team for your GitHub repos.

Comment thread apps/daemon/src/lint-artifact.ts
Comment thread apps/daemon/src/lint-artifact.ts Outdated
Comment thread apps/daemon/src/lint-artifact.ts
Nested CTAs now count once and transparent backgrounds via rgba/hsla alpha=0 are no longer flagged.

Co-Authored-By: Claude <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@nettee nettee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one blocking correctness issue in the primary-class detector and one smaller follow-up in the new alpha parser. Details are inline.

🔁 Powered by Looper · runner=reviewer · agent=opencode · An autonomous AI dev team for your GitHub repos.

Comment thread apps/daemon/src/lint-artifact.ts Outdated
Comment thread apps/daemon/src/lint-artifact.ts Outdated
Co-Authored-By: Claude <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@nettee nettee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one remaining blocking correctness issue in the primary-class detector. Details are inline.

🔁 Powered by Looper · runner=reviewer · agent=opencode · An autonomous AI dev team for your GitHub repos.

Comment thread apps/daemon/src/lint-artifact.ts Outdated
@EthanGuo-coder
Copy link
Copy Markdown
Contributor Author

@nettee All three threads are addressed: nested-region dedup and alpha-aware isPaintedBackground in 453e1bb9, and the interactive-class filter on findPrimaryButtonClass in 4e779e50.

…cta-hierarchy-mismatch

Parse the full selector prelude so grouped rules like `.btn-primary, .btn-primary:visited { ... }` enter the candidate set, parse `rgba(... / calc(0))` and `calc(0%)` alpha values via `Number.isFinite`, and tighten the rgba/hsla parser to balance nested parens.
@EthanGuo-coder
Copy link
Copy Markdown
Contributor Author

@nettee Pushed a single follow-up that addresses both round-2 threads and an unrelated CI tsc break I introduced in the prior commit:

  • Grouped selectors (#3270972993 / #3271386686): findPrimaryButtonClass now parses each rule's full prelude, splits on commas, and extracts class tokens from every branch — .btn-primary, .btn-primary:visited { ... } now enters the candidate set; regression test reproduces your <header><button class="btn"> + <main><button class="btn-primary"> + grouped CSS layout.
  • calc()/NaN alpha (#3270972994): extracted a parseAlpha helper using Number.isFinite that recognizes calc(0) and calc(0%); the rgba/hsla parser was also tightened to balance nested parens (previously truncated args at the first nested ), which made the calc path unreachable). New matrix rows cover rgba(... calc(0)), rgb(... / calc(0%)), and rgba(... calc(0.5)) as a positive control.
  • CI tsc: prior commit's alpha branches had unguarded args[...] accesses (TS18048). Strict guards added so pnpm --filter @open-design/daemon build is green; the local validator only ran vitest before, which is why this slipped — fixed in the run's tooling now too.

128 lint-artifact tests pass (vitest 129/129 including the new rows). The earlier review-summary acknowledgment was premature — both round-2 threads are addressed in 3840e9cd.

@EthanGuo-coder EthanGuo-coder requested a review from nettee May 20, 2026 06:06
Copy link
Copy Markdown
Contributor

@nettee nettee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one blocking correctness issue in the primary-button detector. Details are inline.

🔁 Powered by Looper · runner=reviewer · agent=opencode · An autonomous AI dev team for your GitHub repos.

Comment thread apps/daemon/src/lint-artifact.ts Outdated
…nd-color in cta-hierarchy-mismatch

Pick the last declaration in source order whose prop is `background` or `background-color` so `.btn-primary { background: none; background-color: #2f6feb; ... }` is correctly seen as painted.
@EthanGuo-coder
Copy link
Copy Markdown
Contributor Author

@nettee Fixed in eb0f47c2: findPrimaryButtonClass now picks the last declaration in source order whose prop is background or background-color (a single-pass scan over decls), so reset-then-fill rules like .btn-primary { background: none; background-color: #2f6feb; ... } are correctly recognized as painted. Added two regressions in apps/daemon/tests/lint-artifact.test.ts — the reviewer's exact fixture, plus the reverse order (background-color then background: none) to guard against over-correction. 131/131 lint-artifact tests pass; tsc clean.

@EthanGuo-coder EthanGuo-coder requested a review from nettee May 20, 2026 06:20
Copy link
Copy Markdown
Contributor

@nettee nettee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one remaining blocking correctness issue in the primary-class detector. Details are inline.

🔁 Powered by Looper · runner=reviewer · agent=opencode · An autonomous AI dev team for your GitHub repos.

Comment thread apps/daemon/src/lint-artifact.ts Outdated
…TA class

Skip descendant, child, and pseudo-element branches so `.btn-primary .icon { background: ... }` no longer treats `.btn-primary` as painted; pseudo-classes like `:hover` and type+class compounds are still honored.
@EthanGuo-coder
Copy link
Copy Markdown
Contributor Author

@nettee Fixed in 2be5fc68: extractSelectorClassNames now splits each branch by combinator (\s / > / + / ~), takes the rightmost compound only, and discards branches whose last compound carries a pseudo-element marker (:: or the legacy single-colon :before / :after / :first-line / :first-letter). Pseudo-classes (:hover, :focus, :visited, etc.) and type+class compounds (button.btn-primary) still nominate the host class. Added five regressions in apps/daemon/tests/lint-artifact.test.ts — your exact .btn-primary .icon fixture (no false positive), child combinator (>), pseudo-element (::before), plus positive controls for :hover and button.btn-primary so the next pass doesn't over-correct. 136/136 lint-artifact tests pass; tsc clean.

@EthanGuo-coder EthanGuo-coder requested a review from nettee May 20, 2026 06:41
Copy link
Copy Markdown
Contributor

@nettee nettee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one remaining correctness issue in the new selector parsing for cta-hierarchy-mismatch. Details are inline.

🔁 Powered by Looper · runner=reviewer · agent=opencode · An autonomous AI dev team for your GitHub repos.

Comment thread apps/daemon/src/lint-artifact.ts Outdated
const out: string[] = [];
const seen = new Set<string>();
for (const branch of prelude.split(',')) {
const compounds = branch.trim().split(/[\s>+~]+/).filter(Boolean);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extractSelectorClassNames() now identifies the last compound with branch.trim().split(/[\s>+~]+/), but that split is not selector-aware. Any valid selector that contains whitespace inside an attribute selector or functional pseudo gets chopped in the middle before we choose last. For example, button.btn-primary[aria-label="Buy now"] { ... } becomes ["button.btn-primary[aria-label="Buy", "now"]"], so .btn-primary never reaches eligible and cta-hierarchy-mismatch silently disappears even though the real primary button class is still present. That breaks the rule this PR is adding on common selectors that annotate buttons with accessible labels or :is(...) branches containing spaces. Please replace the raw split with a right-to-left scan that ignores combinators inside [], (), and quoted strings, and add a regression in apps/daemon/tests/lint-artifact.test.ts for a selector like button.btn-primary[aria-label="Buy now"] { background: ... }.

🔁 Powered by Looper · runner=reviewer · agent=opencode · An autonomous AI dev team for your GitHub repos.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nettee Done — extractSelectorClassNames now uses splitTopLevel(',') and a []/()/quote-aware right-to-left scan, with a regression test for button.btn-primary[aria-label="Buy now"] plus an :is(...) guard in apps/daemon/tests/lint-artifact.test.ts. e03ea6fa2c377c7f9c466f350ab882356396c999

Split selector lists with the existing `splitTopLevel(',')` helper and walk the rightmost compound with a `[]`/`()`/quote-depth scan so attribute-selector values like `button.btn-primary[aria-label="Buy now"]` and `:is(...)` grouping no longer drop the primary class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps size/L PR changes 300-700 lines type/feature New feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Design QA checks for CTA hierarchy

3 participants