Skip to content

Feat/typography preflight detector rules#237

Open
kaushalrog wants to merge 5 commits into
pbakaus:mainfrom
kaushalrog:feat/typography-preflight-detector-rules
Open

Feat/typography preflight detector rules#237
kaushalrog wants to merge 5 commits into
pbakaus:mainfrom
kaushalrog:feat/typography-preflight-detector-rules

Conversation

@kaushalrog

@kaushalrog kaushalrog commented Jun 9, 2026

Copy link
Copy Markdown

Summary

Type of change

  • New command
  • New / updated skill reference
  • New anti-pattern guidance
  • Bug fix
  • Documentation update
  • Build system / tooling
  • Other:

Checklist

  • Source files updated in source/
  • bun run build ran successfully
  • bun test passes
  • Tested with at least one provider (Cursor / Claude Code / Gemini CLI / Codex / Copilot / Kiro / OpenCode / Qoder)
  • README / DEVELOP.md updated if needed

Note

Low Risk
Documentation and additive detector rules with fixture tests; no auth, data, or runtime API changes beyond new antipattern findings.

Overview
Adds a Mechanical Pre-Scan step to layout.md and typeset.md (mirrored across provider skill trees): agents must run documented grep patterns for off-scale spacing, arbitrary typography, hex colors, etc., and report hits before visual review.

Introduces a label-line-height quality antipattern in the registry and implements it in browser detection (detect-antipatterns-browser.js) and static HTML checks (checks.mjs) for small UI text (≤13px on buttons, badges, chips, labels) when line-height ratio exceeds 1.7. Coverage includes a fixture HTML page and a detect-antipatterns-fixtures test that asserts flag vs pass cases.

Reviewed by Cursor Bugbot for commit 9000401. Bugbot is set up for automated code reviews on this repo. Configure here.

@kaushalrog kaushalrog requested a review from pbakaus as a code owner June 9, 2026 04:36

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 3 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 9000401. Configure here.

Comment thread skill/reference/typeset.md
Comment thread skill/reference/layout.md
Comment thread cli/engine/detect-antipatterns-browser.js

@pbakaus pbakaus left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for this. There are really two separate things in here: the label-line-height detector rule, and the Mechanical Pre-Scan grep block in layout.md/typeset.md. I'd split them, because they're in pretty different shape.

My main problem is with the pre-scan. Almost all of it is Tailwind arbitrary-value syntax (p-[, text-[, leading-[, opacity-[, z-[...]), and we don't assume Tailwind anywhere. This repo is plain CSS, and the skill runs on whatever frontend the user has. So on a non-Tailwind project these greps just find nothing, the "required" pre-scan quietly does nothing, and the agent walks away thinking it came back clean. The handful that aren't Tailwind (padding:…px, font-size:…px, hex) are pretty blunt too, e.g. gap: 8px is on most scales but would get flagged.

The selection also feels kind of arbitrary. Why z-index 999 but not !important, why opacity but not arbitrary colors, etc. I can't really see the line back to the design laws.

The bigger thing: we already have a detector for this. npx impeccable detect exists, and polish.md already tells the agent to run it when it's there. Reimplementing a weaker version of it in raw grep inside the skill prose is the wrong direction. If we want a preflight, it should call the detector.

And it cuts against the debias stuff we already do. polish.md literally says don't treat a clean mechanical result as proof the work is polished, and critique runs two independent sub-agents for exactly this reason. A grep gate that says "run this before visual review, surface every hit" is the thing that makes the model think it's in the clear once the checks return. No second pass, no independent look.

Small one: the grep blocks aren't in code fences, so the # lines render as H1s, and there's a stray +\ in layout.md and a ./. typo in the last typeset.md grep.

The label-line-height rule itself I'm fine with, it's reasonable and the fixture/test are solid. Couple of fixes before it lands though:

  • don't hand-edit detect-antipatterns-browser.js, it's generated. Run bun run build:browser instead (the hand-edited version is also missing the jsdom fallback that checks.mjs has).
  • new rule also needs the extension and DETECTION_COUNT synced (bun run build:extension), neither is here.
  • drop all the .claude/.cursor/.gemini/... provider edits, those are generated, feature PRs should just touch skill/ and let the sync workflow regenerate them.

So: pull the rule out into its own PR with the build artifacts regenerated properly, and let's rethink the pre-scan. If we do want one, base it on the detector, make it framework-neutral, and don't let it stand in for the actual review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants