Repository navigation
test: lint the docs and skills examples with the plugin's rules - #839
Merged
Merged
Conversation
The guide shows what each rule rejects, so some of its fences are wrong by design: the opening example, and every fence holding a ✗ case that a recommended rule reports. Give those fences `nolint` in their info string, so the example gate in the next commit can tell them from a mistake. The word changes nothing on the rendered page, and the ✗ and ✓ lines in them are still held to their verdicts line by line.
A helper prop given a value it does not recognise renders nothing and says nothing, so a wrong value in a docs example ships green. #676, #677, #678, #689 and #697 were all that, each found by hand, because the fences compile nowhere that would have caught them. scripts/eslint-plugin-docs.test.mjs already ran the real rules over the plugin's own ✗/✓ lines. It now also runs the recommended preset, as shipped, over every jsx and tsx fence under docs/docs and skills, and over the skills' example files. Fences come from fenceSpans, the reader the conformance checks and the API-page generator already share. Most fences carry no import, so one is synthesised for the library exports a fence uses without binding them itself: a fence that declares its own Box, or imports from another library, keeps its meaning. A fence is read as a module, then as bare JSX, then as a list of one-line elements, which is the shape of the ESLint guide's rule lists and of the migration notes that show a lone opening tag. One that parses none of those ways fails, since no rule could read it. `nolint` in the info string exempts a fence that is wrong on purpose, and fails once nothing in the fence is reported. docs/CLAUDE.md and skills/CLAUDE.md say so where contributors look. Each failure names the file and line, the fence, the rule and the rule's own message. Putting textAlign="center" back into the Ionicon variants example fails the gate with "Did you mean `centered`?". Closes #740
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: allxsmith/bestax/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Preview DeploymentPreview URL: https://d0ea8405.bestax.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The gate synthesises a library import only for names a fence leaves
unbound, so a fence that binds a name keeps its own meaning. Two
spellings bound the library itself and hid its elements from every
rule. A subpath import (`@allxsmith/bestax-bulma/elements/Notification`)
bound the name to a specifier the rules do not match. And a `live`
fence's imports bind nothing on the page, because the docs code block
drops those lines and takes every tag from the library, so
`import { Buttons } from './Buttons'` hid a `<Buttons>` the page renders
as the library's.
A subpath of the package is now read as the package, and a live fence
is read as the site runs it, with its import lines blanked so every line
keeps its number. A copied fence's imports still mean what they say.
Putting `mt="1rem"` into the Column and Button examples that used those
spellings now fails the gate; before, it passed.
The guard that the corpus is not empty only asked about fences, so the
walk's file branch could go dead and take the skills' example files,
which nothing else lints, with it. A second guard asks the corpus for at
least one standalone file; renaming the file pattern fails it where the
existing guards stay green.
The Basic Columns example imported `Notification` from `@allxsmith/bestax-bulma/elements/Notification`, a subpath the package does not export, so anyone copying it gets ERR_PACKAGE_PATH_NOT_EXPORTED. Import it from the package with `Columns` and `Column`. The Button colors example imported `Buttons` from `./Buttons`, a path that exists only in the library's source tree. The live preview drops the line, so the page worked, but a reader copying it gets a missing module. `Buttons` needs no import there, like the other examples on the page, so the line goes, along with the trailing semicolon it needed. The MCP index carries both examples and is regenerated.
Owner
Author
Contributor
Preview DeploymentPreview URL: https://3a42015e.bestax.pages.dev |
Contributor
Preview DeploymentPreview URL: https://85684c99.bestax.pages.dev |
There was a problem hiding this comment.
Deep review — 1 blocking · 4 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | Coverage | nolint on a fence that stops parsing is a blanket, never-expiring exemption — the expiry check is skipped when messages is undefined, and the parse-error message routes authors straight into it |
scripts/eslint-plugin-docs.test.mjs:429-449 |
| 2 | 🔵 Advisory | Robustness | pnpm test never rebuilds eslint-plugin/dist (turbo.json gives that package's test only @allxsmith/bestax-bulma#build, and its own test script is jest over src), so a local rule change is gated against the previous build; the existsSync assert catches absent, not stale. CI is safe — pnpm run build precedes pnpm run test. Pre-existing for the first pass, but eslint-plugin/CLAUDE.md's new "a rule change that starts reporting correct examples fails pnpm test" now leans on it |
eslint-plugin/CLAUDE.md:168 |
| 3 | 🔵 Advisory | Coverage | asRendered blanks lines starting with import; the page's transformCode drops export default lines too. Zero live fences contain one today, so the two readings agree in practice — worth knowing if one ever does |
scripts/eslint-plugin-docs.test.mjs:267 |
| 4 | 🔵 Advisory | Coverage | The line-by-line tier skips any line that is not ^\s*<Tag, so a violation on an indented return <Box …/> inside a fence that parses no other way would pass unseen. Measured across the corpus: 44 fences use that tier and exactly 3 skipped lines carry a library tag — all three are // comments |
scripts/eslint-plugin-docs.test.mjs:410 |
| 5 | 🔵 Advisory | Coverage | The library's own source examples (docs/docs/skills/custom-component.mdx:87, skills/bestax-custom-component/references/library-contributor.md) import elements by relative path ('../index'), which correctly keeps them out of the gate — 3 library tags in total, none of them something a consumer copies |
docs/docs/skills/custom-component.mdx:87 |
Overall: the gate is sound and it genuinely bites. I rebuilt both packages, ran it (71 pass, ~2.6 s), and mutation-probed the whole corpus: injecting textAlign="center" into every library-rooted opening tag under docs/docs + skills is caught 5241/5246 times, and the five misses are all explained (a documented trailing-spread silence, plus the relative-path library-source examples in row 5). Line numbers map exactly — a mutation at column.md:45 reports column.md:45. Every class named in #740 that the PR claims is checked, is: textAlign="center", mt={1}, mb={0}, mx="-2", float="Right" and an invalid bgColor all report with the rule's own message; fontSize and <Subtitle> do not, which the PR body scopes out. c55ff74's two fixes hold — a subpath spelling and a live fence's local import both now reach the rules — and I confirmed @allxsmith/bestax-bulma/elements/Notification is absent from the package's exports map, so 264cc54 fixed a real copy-paste failure. nolint costs nothing downstream: the docs build succeeds (exit 0) and gen:mcp:check and check:conformance are both clean with the markers in place. Focus on row 1 — it is the one place the code does not do what three different CLAUDE.md files say it does.
Residual risk:
- A wrong helper value surviving in a fence the gate reads. Refuted empirically — 5241/5246 mutation catch rate, misses enumerated above.
- JSX hiding in a fence language the gate skips. Refuted — a census of all 1704 fences under the two roots found 1423
tsx/ 54jsxand exactly one non-jsx/tsxfence containing a capitalised tag:skills/bestax-custom-component/references/library-contributor.md:334, a four-backtickmdmeta-fence showing a docs-page template. - A fence
fenceSpansnever sees. Refuted — zero jsx/tsx fences are indented four or more spaces (the only shape^ {0,3}would drop), and fences nested inside four-backtick blocks are correctly treated as content. nolinthiding more than it should. Open — row 1. Beyond it, all six markers sit ineslint-plugin.mdand each still reports (3/6/2/6/1/1 messages), so the expiry check is live on every one of them today.- Example surfaces outside the two roots. Open by design —
docs/blog(documented), and, unnamed in the PR,create-bestax/templates,bulma-ui/src/skill-examples/and the package READMEs. Those ship code to users too and no rule reads them. - Failure classes the gate cannot see. Open and declared — a prop that does not exist (#676's
fontSize), a component that is not exported (#737's<Subtitle>), and an import specifier that does not resolve (the verycolumn.mdsubpath this PR fixed by hand). The gate now rewrites any@allxsmith/bestax-bulma/…subpath to the bare package and blanks alivefence's imports entirely, so neither reading can notice a broken specifier — that is option 2 in #740, correctly out of scope here.
🏄 Dude, this one turns a "run the sweep by hand whenever you remember" habit into a wave that just keeps breaking — 5241 out of 5246 synthetic wipeouts caught, line numbers dialled in, docs build still glassy. One little
nolintloophole to patch and it is all-time.
The marker was meant to hold only while a rule reports something in its fence, and three CLAUDE.md files say so. The code skipped that check when the fence did not parse, since there were no messages to count, and the parse failure's own message offered `nolint` as the way out. Taking that advice exempted the fence from every rule for good. A marked fence that does not parse now fails like any other, because it can no longer show it is still wrong on purpose, and the parse failure no longer mentions the marker. An example that is not code at all is labelled `text` rather than `jsx` or `tsx`, which is also what the reader should see; the header and both CLAUDE.md files say so. Breaking a line in the ESLint guide's no-deprecated-props fence now fails the gate; before, it passed. The live preview's `transformCode` drops `export default` lines as well as imports, so the gate blanks both. A live fence that opens with `export default function` is left broken on the page, and now fails to parse here too. `pnpm test` does not rebuild eslint-plugin, so it runs the rules in whatever `dist` holds. The plugin's CLAUDE.md claimed a rule change fails `pnpm test`; it now says CI and `pnpm all`, which build first, and names the build to run before `pnpm test`.
Owner
Author
|
On the advisories:
|
Contributor
Preview DeploymentPreview URL: https://5d717109.bestax.pages.dev |
allxsmith
added
the
needs-human-review
Contributor
📸 Story screenshots at handoff —
|
Contributor
Preview DeploymentPreview URL: https://fbc3d433.bestax.pages.dev |
|
🎉 This PR is included in version 5.19.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.14.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.25.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.13 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
1 participant
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.
Closes #740.
A helper prop given a value it doesn't recognise renders nothing and says nothing, so a wrong value in a docs example ships green. #676, #677, #678, #689 and #697 were all that, found by hand one at a time.
This turns the plugin sweep from #686 into a gate.
scripts/eslint-plugin-docs.test.mjsalready ran the real rules over the plugin's own ✗/✓ lines. It now also runs therecommendedpreset, as shipped, over everyjsxandtsxfence underdocs/docsandskills, and over the skills'examples/files. It runs inpnpm test, so CI picks it up with no workflow change.How it reads a fence:
fenceSpans, the reader the conformance checks already use.Box, or imports from another library, keeps its own meaning.A fence that's wrong on purpose takes
nolintin its info string, which changes nothing on the rendered page, in what readers copy, or in the MCP index. It's per fence, so the rest of the page is still checked. Anolintfence that nothing reports in fails too, so the marker goes when the mistake does. The ESLint guide's wrong-on-purpose fences are the only ones that need it today, and the first pass still checks each ✗/✓ line in them.docs/CLAUDE.mdandskills/CLAUDE.mddocument the marker.Main has no real violations right now, so nothing needed fixing. To check it bites, I put
textAlign="center"andmt={1}back into the Ionicon variants example, andmx="-2",float="Right"andmb={0}into a skills fence. Each failed with the file, line, fence, rule and the rule's own message ("Did you meancentered?").Not in scope: running tsc over the fences (option 2 in the issue). That's what would catch a prop that doesn't exist, like #676's
fontSize, or a component that isn't exported.pnpm allpasses locally.