Repository navigation
Conversation
The API reference pages are hand-written, and the sections that merely restate the source drift from it. Nothing in CI compared a props table to its interface, so the tables diverged in shape (3-column in button.md, 4-column "| Field |" in footer.md, "| Prop |" in the avatar.md exemplar) and in content (button.md renders textColor as "'primary' | ... (see docs)"; hero.md and card.md instead dump 19-member colour unions). Take machine ownership of the four derivable regions, delimited by <!-- bestax:generated <id> --> markers, and leave everything else alone: overview the summary sentence, from the component's TSDoc import from the public barrel props from the <X>Props interfaces (TypeScript compiler API) cssvars new section, parsed from the SCSS Prose outside the markers is preserved byte-for-byte, so a page keeps its admonitions and its hand-written Usage/Accessibility/Related/Additional sections. Section order now runs Overview, Import, Usage, ..., Props, CSS & Sass Variables. Deleting a marker pair opts a region out. Readability is the reason this is not typedoc. Props tables list only the interface's own members plus one catch-all row, because resolving the heritage yields ~250 DOM attributes and ~65 helper props. Types render from AST source text rather than checker.typeToString, which expands (typeof validColors)[number] into 19 literals; wide colour unions become a link to the existing Valid values page. Prop descriptions migrate from the @Property blocks above each interface onto the members as inline TSDoc, seeded from the existing docs tables where those are richer, so no curated prose is lost. @Property was a comment about the interface: unverifiable, and invisible in IntelliSense. Component TSDoc summaries are seeded FROM the pages for the same reason — the existing ones ("Bulma Hero component root.") are worse than the prose they would have replaced. Pilot covers layout/ (6 pages); MANAGED_CATEGORIES gates the rest. The staleness gate lives in check-conformance.mjs as two new sub-checks (docs-generated recomputes each page in memory and diffs; docs-section-order holds the order and marker presence) rather than a new CI step, since that file already runs in CI. registerVarsKeys moves to scripts/lib/scss-vars.mjs so the generator shares one SCSS parser; key extraction is unchanged, verified key-for-key against all 26 partials. Generating requires node_modules/bulma for the stock components' SCSS, a deliberate departure from gen-component-catalog.mjs's no-install rule; it fails with an actionable message rather than an ENOENT. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011c6ipfvBsYgF3Jf91ZzSr7
Reviewing the rendered pages showed a regression against the hand-written tables: container.md documented `fluid`, `widescreen`, `fullhd` and `isMax` as defaulting to `false`, but the generator emitted an em dash. Those props are destructured with no initializer, so there was nothing for the AST default-reader to find. An optional prop typed exactly `boolean` is `undefined` by default, which every consumer treats as off, so `false` is both accurate and what the tables already said. Infer it. Safe to do library-wide: nothing under bulma-ui/src defaults a boolean to true outside its destructuring pattern, and an explicit destructured default or `@defaultValue` still wins. Verified by diffing every prop's Default and Description cell on all six pilot pages against their pre-migration versions: no default changed, no description got shorter, no prop was dropped. The generator additionally documents six props the hand-written tables had missed — Hero's `textColor` and Level's `as`, `hasTextCentered`, `href`, `target` and `rel`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011c6ipfvBsYgF3Jf91ZzSr7
|
Important Review skippedToo many files! This PR contains 199 files, which is 99 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (199)
You can disable this status message by setting the WalkthroughThis change introduces generated API documentation for managed components, shared prop and SCSS metadata extraction, migration and TSDoc codemods, generated-page conformance checks, updated API pages, and revised contribution guidance. ChangesAPI documentation pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
Preview DeploymentPreview URL: https://40f70b64.bestax.pages.dev |
CodeQL reported 2 new alerts (1 high, 1 medium) against the parsers added in
this PR. The flagged shape is unbounded repetition immediately before a
literal — `[\w.-]*getVar(`, `[\w.-]*register-vars`, and a lazy `[\s\S]+?`
before `}` — which the engine retries at every start position, giving
polynomial backtracking (js/polynomial-redos).
These scripts only ever read repo-owned files, so the practical risk is nil,
but the tightened patterns also say what was actually meant: the optional
prefix is a sass module namespace, so match `(?:[\w-]+\.)?` rather than any
run of word characters, and the JSDoc `{Type}` capture cannot span a `}`, so
match `[^}]+` rather than a lazy any-character run. The `@property` line is
matched per line, so `[\s\S]` was equivalent to `.` there anyway.
Also drops a stray `\*ic?` alternative from the JSDoc line-stripper — a typo
that never matched anything the following `\*\s?` did not already handle.
Behaviour is unchanged, verified three ways: registerVarsKeys still matches
the original implementation key-for-key across all 26 partials (366 keys, 0
diffs), the 24 generated layout CSS-variable rows are identical, and `pnpm
gen` produces no diff. The @Property parser is exercised by a dry run over
elements/ — 214 members parsed, the same 6 orphans detected.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011c6ipfvBsYgF3Jf91ZzSr7
Contributor
Preview DeploymentPreview URL: https://921b1c81.bestax.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
scripts/lib/props-extract.mjs (2)
124-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBarrel parsing misses multi-line
export { … } fromblocks.Prettier wraps long named-export lists across lines, and the line-oriented regex only matches single-line forms; a wrapped entry makes
extractComponentthrow "not exported". Since aProgramis already available, walkingsrc/index.ts's export declarations via the AST would be robust.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/lib/props-extract.mjs` around lines 124 - 145, Update exportedModules to parse src/index.ts through the available Program AST instead of matching individual lines, including multi-line ExportNamedDeclaration and export-all declarations. Populate the existing Map with each exported name and its category/module metadata so extractComponent recognizes wrapped named-export blocks.
488-501: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOwn members and expanded base members can produce duplicate rows.
When an interface redeclares a prop it also inherits from an expanded local base, both rows land in the table. A dedupe by
namekeeping the own (non-inherited) row would avoid it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/lib/props-extract.mjs` around lines 488 - 501, Deduplicate the rows assembled around memberRows by prop name after adding expanded base members, retaining the own non-inherited row when both sources define the same name. Apply this only to the combined rows collection and preserve distinct inherited rows and existing ordering where no duplicate exists.bulma-ui/src/layout/Level.tsx (1)
228-240: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider defaulting
relfortarget="_blank"anchors.
LevelItemforwardsrelonly when explicitly supplied. Sincetarget="_blank"withoutrel="noopener noreferrer"is a known reverse-tabnabbing risk, consider defaultingrelto"noopener noreferrer"whentarget === '_blank'and norelis provided.🔒 Proposed default
if (Tag === 'a') { + const resolvedRel = + rel ?? (target === '_blank' ? 'noopener noreferrer' : undefined); return ( <a className={levelItemClasses} href={href} target={target} - rel={rel} + rel={resolvedRel} {...rest} > {children} </a> ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bulma-ui/src/layout/Level.tsx` around lines 228 - 240, Update the anchor branch in LevelItem so rel defaults to "noopener noreferrer" when target is "_blank" and no rel prop was provided, while preserving any explicitly supplied rel value.scripts/migrate-api-pages.mjs (1)
225-239: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
buildOverviewonly extracts the first physical line of the Overview paragraph.
para = lines[i](line 231) takes a single line before runningfirstSentence. If a future category's Overview paragraph is hard-wrapped across multiple lines (unlike the single-line paragraphs in the layout pages shown here), the sentence boundary detection would operate on a truncated fragment instead of the full paragraph, and the remaining wrapped text would fall intotailunprocessed.Since this script is reused for other categories per its own header comment, it would help to confirm whether any pending category's Overview paragraphs are hard-wrapped across lines before the next run.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/migrate-api-pages.mjs` around lines 225 - 239, Update buildOverview to combine all contiguous non-empty lines of the leading Overview paragraph before calling firstSentence, while preserving subsequent blank-line content and admonitions in tail. Ensure wrapped paragraph text is included in sentence detection and only the text after the first sentence is retained as the generated remainder.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/codemod-property-tsdoc.mjs`:
- Around line 199-234: Update the description selection in the member loop using
docs and byName so that fromDocs is always preferred when available, with
fromTag only as the fallback. Remove the length-based comparison while
preserving the existing undocumented reporting and insertion behavior.
- Around line 2-33: Replace the invisible whitespace characters in the nested
`*/` examples within the header comment of the codemod script with an
ESLint-safe representation, such as escaped slashes, while preserving the
rendered documentation. Avoid embedding disable directives inside the block
comment; keep the comment syntactically valid and eliminate all
`no-irregular-whitespace` violations.
In `@scripts/gen-api-docs.mjs`:
- Around line 227-238: Update the body-generation flow around renderCssVars so
CSS variable rendering occurs only when regions.has("cssvars") is true; preserve
the existing rendering and replacement behavior for opted-in regions while
skipping SCSS I/O and errors when the cssvars markers are absent.
In `@scripts/lib/api-page.mjs`:
- Around line 221-245: Update upsertFrontmatter to serialize or quote value
safely before constructing the frontmatter line, ensuring colons followed by
spaces, newlines, and YAML-sensitive leading characters cannot produce invalid
YAML. Also escape key and after when creating the matching regular expressions
so they are treated as literal frontmatter keys rather than regex patterns.
- Around line 267-269: Update escapeCell to escape backslashes before escaping
pipe characters, so a backslash immediately preceding a pipe remains valid in
the generated cell content. Preserve the existing newline replacement and
trimming behavior.
In `@scripts/lib/scss-vars.mjs`:
- Around line 238-255: Update the loop in registerVarsKeys to avoid binding the
unused regex match result m; iterate over the regex execution without assigning
its returned match object, while preserving use of re.lastIndex and the existing
key extraction behavior.
- Around line 71-94: In the scanner loop, move the quoted-string branch in the
function containing the shown while loop ahead of both comment branches. Ensure
skipString handles strings before detecting // or /* so comment markers inside
quoted values are preserved, while actual comments continue using their existing
logic.
In `@scripts/migrate-api-pages.mjs`:
- Around line 31-38: Remove the unused splitLines import from the import list in
migrate-api-pages.mjs, leaving the other API page helpers unchanged, then ensure
the file satisfies the no-unused-vars ESLint check.
- Around line 158-198: Update the JSDoc tail extraction in the migration logic
around `tagsAt`, `tail`, and the `rest` text so it preserves content after the
first summary paragraph, stopping at the first blank comment line or `@` tag as
intended. Ensure non-tagged trailing prose is retained in the generated
replacement and therefore included in the existing `was`/`now` output, without
changing the summary replacement behavior.
---
Nitpick comments:
In `@bulma-ui/src/layout/Level.tsx`:
- Around line 228-240: Update the anchor branch in LevelItem so rel defaults to
"noopener noreferrer" when target is "_blank" and no rel prop was provided,
while preserving any explicitly supplied rel value.
In `@scripts/lib/props-extract.mjs`:
- Around line 124-145: Update exportedModules to parse src/index.ts through the
available Program AST instead of matching individual lines, including multi-line
ExportNamedDeclaration and export-all declarations. Populate the existing Map
with each exported name and its category/module metadata so extractComponent
recognizes wrapped named-export blocks.
- Around line 488-501: Deduplicate the rows assembled around memberRows by prop
name after adding expanded base members, retaining the own non-inherited row
when both sources define the same name. Apply this only to the combined rows
collection and preserve distinct inherited rows and existing ordering where no
duplicate exists.
In `@scripts/migrate-api-pages.mjs`:
- Around line 225-239: Update buildOverview to combine all contiguous non-empty
lines of the leading Overview paragraph before calling firstSentence, while
preserving subsequent blank-line content and admonitions in tail. Ensure wrapped
paragraph text is included in sentence detection and only the text after the
first sentence is retained as the generated remainder.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 417ad40c-8870-4b70-b35c-fb05ee0f684e
📒 Files selected for processing (24)
CONTRIBUTING-COMPONENTS.mdbulma-ui/CLAUDE.mdbulma-ui/src/layout/Container.tsxbulma-ui/src/layout/Footer.tsxbulma-ui/src/layout/Hero.tsxbulma-ui/src/layout/Level.tsxbulma-ui/src/layout/Media.tsxbulma-ui/src/layout/Section.tsxdocs/CLAUDE.mddocs/docs/api/layout/container.mddocs/docs/api/layout/footer.mddocs/docs/api/layout/hero.mddocs/docs/api/layout/level.mddocs/docs/api/layout/media.mddocs/docs/api/layout/section.mdpackage.jsonscripts/check-conformance.mjsscripts/codemod-property-tsdoc.mjsscripts/gen-api-docs.mjsscripts/lib/api-page.mjsscripts/lib/api-sources.mjsscripts/lib/props-extract.mjsscripts/lib/scss-vars.mjsscripts/migrate-api-pages.mjs
Comment on lines
+2
to
+33
| /** | ||
| * ONE-TIME migration: move prop documentation from the `@property` block above | ||
| * each `<X>Props` interface onto the interface members as inline TSDoc. | ||
| * | ||
| * /** /** | ||
| * * Props for the Footer component. * Props for the Footer component. | ||
| * * */ | ||
| * * @property {string} [className] - export interface FooterProps … { | ||
| * * Additional CSS classes. /** Additional CSS classes. */ | ||
| * */ className?: string; | ||
| * | ||
| * Why bother: `@property` is a comment ABOUT the interface. TypeScript cannot | ||
| * check it, so its hand-typed `{Type}` drifts from the real member type, and it | ||
| * never reaches a user's editor. Inline TSDoc is verifiable by position, shows | ||
| * up in IntelliSense, and is what `scripts/lib/props-extract.mjs` reads. | ||
| * | ||
| * Three rules that matter: | ||
| * | ||
| * 1. DESCRIPTIONS COME FROM THE DOCS PAGE FIRST. The hand-written tables in | ||
| * docs/docs/api are consistently richer than the `@property` lines | ||
| * (button.md explains what `ghost` and `text` render as; the source comment | ||
| * just says "Bulma color modifier"). Migrating from the source would quietly | ||
| * downgrade the docs. | ||
| * 2. MERGE, NEVER OVERWRITE. 21 modules already carry some inline member TSDoc | ||
| * and 6 are mixed; a member that already has a comment is left alone. | ||
| * 3. ORPHAN `@property` LINES ARE NOT DROPPED. 38 of them name props that live | ||
| * on a DOM base type rather than the interface. Those are rewritten as | ||
| * `@extraProp`, which props-extract renders as its own rows. | ||
| * | ||
| * Usage: | ||
| * node scripts/codemod-property-tsdoc.mjs --category=layout [--dry-run] | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Irregular whitespace trips no-irregular-whitespace (ESLint).
Lines 8, 10, and 11 use an invisible whitespace character between * and / to render */ literally inside the outer block comment without terminating it. Static analysis flags this as no-irregular-whitespace on all three lines, which will fail the ESLint gate.
Since the offending text is nested text inside one continuous /** ... */ block (lines 2-33), an inline eslint-disable-next-line comment can't be embedded there — wrap the whole header comment with disable/enable directives instead, or replace the invisible character with an escaped \/ that doesn't require special whitespace.
🔧 Proposed fix
#!/usr/bin/env node
+/* eslint-disable no-irregular-whitespace */
/**
* ONE-TIME migration: move prop documentation from the `@property` block above
* each `<X>Props` interface onto the interface members as inline TSDoc.
...
* Usage:
* node scripts/codemod-property-tsdoc.mjs --category=layout [--dry-run]
*/
+/* eslint-enable no-irregular-whitespace */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * ONE-TIME migration: move prop documentation from the `@property` block above | |
| * each `<X>Props` interface onto the interface members as inline TSDoc. | |
| * | |
| * /** /** | |
| * * Props for the Footer component. * Props for the Footer component. | |
| * * */ | |
| * * @property {string} [className] - export interface FooterProps … { | |
| * * Additional CSS classes. /** Additional CSS classes. */ | |
| * */ className?: string; | |
| * | |
| * Why bother: `@property` is a comment ABOUT the interface. TypeScript cannot | |
| * check it, so its hand-typed `{Type}` drifts from the real member type, and it | |
| * never reaches a user's editor. Inline TSDoc is verifiable by position, shows | |
| * up in IntelliSense, and is what `scripts/lib/props-extract.mjs` reads. | |
| * | |
| * Three rules that matter: | |
| * | |
| * 1. DESCRIPTIONS COME FROM THE DOCS PAGE FIRST. The hand-written tables in | |
| * docs/docs/api are consistently richer than the `@property` lines | |
| * (button.md explains what `ghost` and `text` render as; the source comment | |
| * just says "Bulma color modifier"). Migrating from the source would quietly | |
| * downgrade the docs. | |
| * 2. MERGE, NEVER OVERWRITE. 21 modules already carry some inline member TSDoc | |
| * and 6 are mixed; a member that already has a comment is left alone. | |
| * 3. ORPHAN `@property` LINES ARE NOT DROPPED. 38 of them name props that live | |
| * on a DOM base type rather than the interface. Those are rewritten as | |
| * `@extraProp`, which props-extract renders as its own rows. | |
| * | |
| * Usage: | |
| * node scripts/codemod-property-tsdoc.mjs --category=layout [--dry-run] | |
| */ | |
| /* eslint-disable no-irregular-whitespace */ | |
| /** | |
| * ONE-TIME migration: move prop documentation from the `@property` block above | |
| * each `<X>Props` interface onto the interface members as inline TSDoc. | |
| * | |
| * /** /** | |
| * * Props for the Footer component. * Props for the Footer component. | |
| * * */ | |
| * * `@property` {string} [className] - export interface FooterProps … { | |
| * * Additional CSS classes. /** Additional CSS classes. */ | |
| * */ className?: string; | |
| * | |
| * Why bother: `@property` is a comment ABOUT the interface. TypeScript cannot | |
| * check it, so its hand-typed `{Type}` drifts from the real member type, and it | |
| * never reaches a user's editor. Inline TSDoc is verifiable by position, shows | |
| * up in IntelliSense, and is what `scripts/lib/props-extract.mjs` reads. | |
| * | |
| * Three rules that matter: | |
| * | |
| * 1. DESCRIPTIONS COME FROM THE DOCS PAGE FIRST. The hand-written tables in | |
| * docs/docs/api are consistently richer than the `@property` lines | |
| * (button.md explains what `ghost` and `text` render as; the source comment | |
| * just says "Bulma color modifier"). Migrating from the source would quietly | |
| * downgrade the docs. | |
| * 2. MERGE, NEVER OVERWRITE. 21 modules already carry some inline member TSDoc | |
| * and 6 are mixed; a member that already has a comment is left alone. | |
| * 3. ORPHAN `@property` LINES ARE NOT DROPPED. 38 of them name props that live | |
| * on a DOM base type rather than the interface. Those are rewritten as | |
| * `@extraProp`, which props-extract renders as its own rows. | |
| * | |
| * Usage: | |
| * node scripts/codemod-property-tsdoc.mjs --category=layout [--dry-run] | |
| */ | |
| /* eslint-enable no-irregular-whitespace */ |
🧰 Tools
🪛 ESLint
[error] 8-8: Irregular whitespace not allowed.
(no-irregular-whitespace)
[error] 10-10: Irregular whitespace not allowed.
(no-irregular-whitespace)
[error] 11-11: Irregular whitespace not allowed.
(no-irregular-whitespace)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/codemod-property-tsdoc.mjs` around lines 2 - 33, Replace the
invisible whitespace characters in the nested `*/` examples within the header
comment of the codemod script with an ESLint-safe representation, such as
escaped slashes, while preserving the rendered documentation. Avoid embedding
disable directives inside the block comment; keep the comment syntactically
valid and eliminate all `no-irregular-whitespace` violations.
Sources: Path instructions, Linters/SAST tools
Comment on lines
+199
to
+234
|
|
||
| // Descriptions available for this interface's props, docs page first. | ||
| // | ||
| // EXACT title match only. Falling back to the parent's page for a | ||
| // sub-component (HeroHead -> Hero) borrows descriptions that are wrong in | ||
| // context: Hero's `children` is "Hero content (often includes Hero.Head, | ||
| // Hero.Body, Hero.Foot)", which is nonsense on Hero.Head itself. Where a | ||
| // sub-component has no page, its `@property` lines are the better source. | ||
| const component = name.replace(/Props$/, ''); | ||
| const docs = docsByTitle.get(component) ?? new Map(); | ||
| const byName = new Map(tags.map(t => [t.name, t.desc])); | ||
|
|
||
| const ownNames = new Set(decl.members.map(memberName).filter(Boolean)); | ||
|
|
||
| // Members that lack a comment get one. | ||
| for (const member of decl.members) { | ||
| const mName = memberName(member); | ||
| if (!mName) continue; | ||
| if (hasOwnJsDoc(member)) continue; | ||
| const fromDocs = docs.get(mName); | ||
| const fromTag = byName.get(mName); | ||
| const desc = | ||
| fromDocs && fromTag | ||
| ? fromDocs.length >= fromTag.length | ||
| ? fromDocs | ||
| : fromTag | ||
| : (fromDocs ?? fromTag); | ||
| if (!desc) { | ||
| report.undocumented.push(`${name}.${mName}`); | ||
| continue; | ||
| } | ||
| const start = member.getStart(sf); | ||
| const indent = lineIndent(original, start); | ||
| edits.push({ start, end: start, text: `/** ${desc} */\n${indent}` }); | ||
| report.inserted++; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Length-based tie-break contradicts the stated "docs page first" rule.
The file's header (Rule 1) states descriptions should come from the docs page first, but when both fromDocs and fromTag exist, the code just picks whichever string is longer rather than always preferring fromDocs. A verbose @property line could win over a shorter, more accurate docs-page description, silently downgrading documentation quality — the exact outcome Rule 1 says to avoid.
🔧 Proposed fix to always prefer the docs page
- const desc =
- fromDocs && fromTag
- ? fromDocs.length >= fromTag.length
- ? fromDocs
- : fromTag
- : (fromDocs ?? fromTag);
+ const desc = fromDocs ?? fromTag;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Descriptions available for this interface's props, docs page first. | |
| // | |
| // EXACT title match only. Falling back to the parent's page for a | |
| // sub-component (HeroHead -> Hero) borrows descriptions that are wrong in | |
| // context: Hero's `children` is "Hero content (often includes Hero.Head, | |
| // Hero.Body, Hero.Foot)", which is nonsense on Hero.Head itself. Where a | |
| // sub-component has no page, its `@property` lines are the better source. | |
| const component = name.replace(/Props$/, ''); | |
| const docs = docsByTitle.get(component) ?? new Map(); | |
| const byName = new Map(tags.map(t => [t.name, t.desc])); | |
| const ownNames = new Set(decl.members.map(memberName).filter(Boolean)); | |
| // Members that lack a comment get one. | |
| for (const member of decl.members) { | |
| const mName = memberName(member); | |
| if (!mName) continue; | |
| if (hasOwnJsDoc(member)) continue; | |
| const fromDocs = docs.get(mName); | |
| const fromTag = byName.get(mName); | |
| const desc = | |
| fromDocs && fromTag | |
| ? fromDocs.length >= fromTag.length | |
| ? fromDocs | |
| : fromTag | |
| : (fromDocs ?? fromTag); | |
| if (!desc) { | |
| report.undocumented.push(`${name}.${mName}`); | |
| continue; | |
| } | |
| const start = member.getStart(sf); | |
| const indent = lineIndent(original, start); | |
| edits.push({ start, end: start, text: `/** ${desc} */\n${indent}` }); | |
| report.inserted++; | |
| } | |
| // Descriptions available for this interface's props, docs page first. | |
| // | |
| // EXACT title match only. Falling back to the parent's page for a | |
| // sub-component (HeroHead -> Hero) borrows descriptions that are wrong in | |
| // context: Hero's `children` is "Hero content (often includes Hero.Head, | |
| // Hero.Body, Hero.Foot)", which is nonsense on Hero.Head itself. Where a | |
| // sub-component has no page, its `@property` lines are the better source. | |
| const component = name.replace(/Props$/, ''); | |
| const docs = docsByTitle.get(component) ?? new Map(); | |
| const byName = new Map(tags.map(t => [t.name, t.desc])); | |
| const ownNames = new Set(decl.members.map(memberName).filter(Boolean)); | |
| // Members that lack a comment get one. | |
| for (const member of decl.members) { | |
| const mName = memberName(member); | |
| if (!mName) continue; | |
| if (hasOwnJsDoc(member)) continue; | |
| const fromDocs = docs.get(mName); | |
| const fromTag = byName.get(mName); | |
| const desc = fromDocs ?? fromTag; | |
| if (!desc) { | |
| report.undocumented.push(`${name}.${mName}`); | |
| continue; | |
| } | |
| const start = member.getStart(sf); | |
| const indent = lineIndent(original, start); | |
| edits.push({ start, end: start, text: `/** ${desc} */\n${indent}` }); | |
| report.inserted++; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/codemod-property-tsdoc.mjs` around lines 199 - 234, Update the
description selection in the member loop using docs and byName so that fromDocs
is always preferred when available, with fromTag only as the fallback. Remove
the length-based comparison while preserving the existing undocumented reporting
and insertion behavior.
Comment on lines
+227
to
+238
| const bodies = { | ||
| overview: renderOverview(info), | ||
| import: renderImport(info), | ||
| props: renderProps(info), | ||
| cssvars: await renderCssVars(info, { relPath }), | ||
| }; | ||
|
|
||
| let out = src; | ||
| for (const [id, body] of Object.entries(bodies)) { | ||
| if (body == null || !regions.has(id)) continue; | ||
| out = replaceRegion(out, id, body, `docs/docs/api/${relPath}`); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
renderCssVars runs even when the page opted out of the cssvars region.
The header doc says deleting a marker pair opts that region out (lines 12-14), but bodies.cssvars is computed unconditionally before the regions.has(id) gate is checked in the loop below. For a component with a non-empty SCSS_SOURCES entry, this means the SCSS file is still read and renderCssVars can still throw (e.g. missing root, line 172-176) even on a page that intentionally removed the cssvars markers — defeating the opt-out for exactly the region that does I/O and can fail.
🐛 Proposed fix
const bodies = {
overview: renderOverview(info),
import: renderImport(info),
props: renderProps(info),
- cssvars: await renderCssVars(info, { relPath }),
+ cssvars: regions.has('cssvars')
+ ? await renderCssVars(info, { relPath })
+ : null,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const bodies = { | |
| overview: renderOverview(info), | |
| import: renderImport(info), | |
| props: renderProps(info), | |
| cssvars: await renderCssVars(info, { relPath }), | |
| }; | |
| let out = src; | |
| for (const [id, body] of Object.entries(bodies)) { | |
| if (body == null || !regions.has(id)) continue; | |
| out = replaceRegion(out, id, body, `docs/docs/api/${relPath}`); | |
| } | |
| const bodies = { | |
| overview: renderOverview(info), | |
| import: renderImport(info), | |
| props: renderProps(info), | |
| cssvars: regions.has('cssvars') | |
| ? await renderCssVars(info, { relPath }) | |
| : null, | |
| }; | |
| let out = src; | |
| for (const [id, body] of Object.entries(bodies)) { | |
| if (body == null || !regions.has(id)) continue; | |
| out = replaceRegion(out, id, body, `docs/docs/api/${relPath}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/gen-api-docs.mjs` around lines 227 - 238, Update the body-generation
flow around renderCssVars so CSS variable rendering occurs only when
regions.has("cssvars") is true; preserve the existing rendering and replacement
behavior for opted-in regions while skipping SCSS I/O and errors when the
cssvars markers are absent.
Comment on lines
+221
to
+245
| export function upsertFrontmatter(src, key, value, after = null) { | ||
| const m = src.match(/^(---\r?\n)([\s\S]*?)(\r?\n---)/); | ||
| if (!m) throw new Error('page has no frontmatter block'); | ||
| const eol = m[1].includes('\r\n') ? '\r\n' : '\n'; | ||
| const lines = m[2].split(/\r?\n/); | ||
| const line = `${key}: ${value}`; | ||
| const at = lines.findIndex(l => new RegExp(`^${key}:`).test(l)); | ||
|
|
||
| if (at >= 0) { | ||
| lines[at] = line; | ||
| } else { | ||
| const afterAt = after | ||
| ? lines.findIndex(l => new RegExp(`^${after}:`).test(l)) | ||
| : -1; | ||
| if (afterAt >= 0) lines.splice(afterAt + 1, 0, line); | ||
| else lines.push(line); | ||
| } | ||
| return ( | ||
| src.slice(0, m.index) + | ||
| m[1] + | ||
| lines.join(eol) + | ||
| m[3] + | ||
| src.slice(m.index + m[0].length) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Quote/escape the frontmatter value before writing it.
value is interpolated raw into ${key}: ${value}. renderPage passes the generated Overview sentence, so any sentence containing : , a leading #/{/[/quote, or an embedded newline produces invalid YAML and breaks the page build. Also, key/after are interpolated unescaped into new RegExp.
🛠️ Proposed fix
+function yamlScalar(value) {
+ const s = String(value).replace(/\r?\n/g, ' ').trim();
+ return /^[\w([{'"#&*!|>%@`-]|: |:$| `#/.test`(s) || /["']/.test(s)
+ ? `'${s.replace(/'/g, "''")}'`
+ : s;
+}
+
+const escapeRe = s => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+
export function upsertFrontmatter(src, key, value, after = null) {
const m = src.match(/^(---\r?\n)([\s\S]*?)(\r?\n---)/);
if (!m) throw new Error('page has no frontmatter block');
const eol = m[1].includes('\r\n') ? '\r\n' : '\n';
const lines = m[2].split(/\r?\n/);
- const line = `${key}: ${value}`;
- const at = lines.findIndex(l => new RegExp(`^${key}:`).test(l));
+ const line = `${key}: ${yamlScalar(value)}`;
+ const at = lines.findIndex(l => new RegExp(`^${escapeRe(key)}:`).test(l));
if (at >= 0) {
lines[at] = line;
} else {
const afterAt = after
- ? lines.findIndex(l => new RegExp(`^${after}:`).test(l))
+ ? lines.findIndex(l => new RegExp(`^${escapeRe(after)}:`).test(l))
: -1;(The quoting heuristic above is illustrative — a tiny helper or a YAML serializer is fine, as long as : and newlines can't leak through.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function upsertFrontmatter(src, key, value, after = null) { | |
| const m = src.match(/^(---\r?\n)([\s\S]*?)(\r?\n---)/); | |
| if (!m) throw new Error('page has no frontmatter block'); | |
| const eol = m[1].includes('\r\n') ? '\r\n' : '\n'; | |
| const lines = m[2].split(/\r?\n/); | |
| const line = `${key}: ${value}`; | |
| const at = lines.findIndex(l => new RegExp(`^${key}:`).test(l)); | |
| if (at >= 0) { | |
| lines[at] = line; | |
| } else { | |
| const afterAt = after | |
| ? lines.findIndex(l => new RegExp(`^${after}:`).test(l)) | |
| : -1; | |
| if (afterAt >= 0) lines.splice(afterAt + 1, 0, line); | |
| else lines.push(line); | |
| } | |
| return ( | |
| src.slice(0, m.index) + | |
| m[1] + | |
| lines.join(eol) + | |
| m[3] + | |
| src.slice(m.index + m[0].length) | |
| ); | |
| } | |
| function yamlScalar(value) { | |
| const s = String(value).replace(/\r?\n/g, ' ').trim(); | |
| return /^[\w([{'"#&*!|>%@`-]|: |:$| `#/.test`(s) || /["']/.test(s) | |
| ? `'${s.replace(/'/g, "''")}'` | |
| : s; | |
| } | |
| const escapeRe = s => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | |
| export function upsertFrontmatter(src, key, value, after = null) { | |
| const m = src.match(/^(---\r?\n)([\s\S]*?)(\r?\n---)/); | |
| if (!m) throw new Error('page has no frontmatter block'); | |
| const eol = m[1].includes('\r\n') ? '\r\n' : '\n'; | |
| const lines = m[2].split(/\r?\n/); | |
| const line = `${key}: ${yamlScalar(value)}`; | |
| const at = lines.findIndex(l => new RegExp(`^${escapeRe(key)}:`).test(l)); | |
| if (at >= 0) { | |
| lines[at] = line; | |
| } else { | |
| const afterAt = after | |
| ? lines.findIndex(l => new RegExp(`^${escapeRe(after)}:`).test(l)) | |
| : -1; | |
| if (afterAt >= 0) lines.splice(afterAt + 1, 0, line); | |
| else lines.push(line); | |
| } | |
| return ( | |
| src.slice(0, m.index) + | |
| m[1] + | |
| lines.join(eol) + | |
| m[3] + | |
| src.slice(m.index + m[0].length) | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lib/api-page.mjs` around lines 221 - 245, Update upsertFrontmatter to
serialize or quote value safely before constructing the frontmatter line,
ensuring colons followed by spaces, newlines, and YAML-sensitive leading
characters cannot produce invalid YAML. Also escape key and after when creating
the matching regular expressions so they are treated as literal frontmatter keys
rather than regex patterns.
Comment on lines
+267
to
+269
| export function escapeCell(text) { | ||
| return String(text).replace(/\|/g, '\\|').replace(/\r?\n/g, ' ').trim(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Escape backslashes before pipes.
CodeQL flags this (failing check): a cell ending in \ immediately before a | yields \\|, which terminates the escape and splits the row into an extra column. Escape \ first.
🐛 Proposed fix
export function escapeCell(text) {
- return String(text).replace(/\|/g, '\\|').replace(/\r?\n/g, ' ').trim();
+ return String(text)
+ .replace(/\\/g, '\\\\')
+ .replace(/\|/g, '\\|')
+ .replace(/\r?\n/g, ' ')
+ .trim();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function escapeCell(text) { | |
| return String(text).replace(/\|/g, '\\|').replace(/\r?\n/g, ' ').trim(); | |
| } | |
| export function escapeCell(text) { | |
| return String(text) | |
| .replace(/\\/g, '\\\\') | |
| .replace(/\|/g, '\\|') | |
| .replace(/\r?\n/g, ' ') | |
| .trim(); | |
| } |
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 268-268: Incomplete string escaping or encoding
This does not escape backslash characters in the input.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lib/api-page.mjs` around lines 267 - 269, Update escapeCell to escape
backslashes before escaping pipe characters, so a backslash immediately
preceding a pipe remains valid in the generated cell content. Preserve the
existing newline replacement and trimming behavior.
Source: Linters/SAST tools
Comment on lines
+71
to
+94
| while (i < src.length) { | ||
| const ch = src[i]; | ||
|
|
||
| if (ch === '/' && src[i + 1] === '/') { | ||
| while (i < src.length && src[i] !== '\n') i++; | ||
| continue; | ||
| } | ||
| if (ch === '/' && src[i + 1] === '*') { | ||
| const end = src.indexOf('*/', i + 2); | ||
| i = end === -1 ? src.length : end + 2; | ||
| continue; | ||
| } | ||
| if (ch === '"' || ch === "'") { | ||
| const end = skipString(src, i); | ||
| buf += src.slice(i, end); | ||
| i = end; | ||
| continue; | ||
| } | ||
| if (atInterpolation(src, i)) { | ||
| const end = skipInterpolation(src, i); | ||
| buf += src.slice(i, end); | ||
| i = end; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Comment detection runs before string detection, so // inside a quoted value is mis-lexed.
url("https://example.com/x.png") or any string containing // (or /*) truncates the rest of the line, silently dropping the declaration and desynchronising the brace/selector stack. Move the quote branch above the comment branches.
🐛 Proposed fix
while (i < src.length) {
const ch = src[i];
+ if (ch === '"' || ch === "'") {
+ const end = skipString(src, i);
+ buf += src.slice(i, end);
+ i = end;
+ continue;
+ }
if (ch === '/' && src[i + 1] === '/') {
while (i < src.length && src[i] !== '\n') i++;
continue;
}
if (ch === '/' && src[i + 1] === '*') {
const end = src.indexOf('*/', i + 2);
i = end === -1 ? src.length : end + 2;
continue;
}
- if (ch === '"' || ch === "'") {
- const end = skipString(src, i);
- buf += src.slice(i, end);
- i = end;
- continue;
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| while (i < src.length) { | |
| const ch = src[i]; | |
| if (ch === '/' && src[i + 1] === '/') { | |
| while (i < src.length && src[i] !== '\n') i++; | |
| continue; | |
| } | |
| if (ch === '/' && src[i + 1] === '*') { | |
| const end = src.indexOf('*/', i + 2); | |
| i = end === -1 ? src.length : end + 2; | |
| continue; | |
| } | |
| if (ch === '"' || ch === "'") { | |
| const end = skipString(src, i); | |
| buf += src.slice(i, end); | |
| i = end; | |
| continue; | |
| } | |
| if (atInterpolation(src, i)) { | |
| const end = skipInterpolation(src, i); | |
| buf += src.slice(i, end); | |
| i = end; | |
| continue; | |
| } | |
| while (i < src.length) { | |
| const ch = src[i]; | |
| if (ch === '"' || ch === "'") { | |
| const end = skipString(src, i); | |
| buf += src.slice(i, end); | |
| i = end; | |
| continue; | |
| } | |
| if (ch === '/' && src[i + 1] === '/') { | |
| while (i < src.length && src[i] !== '\n') i++; | |
| continue; | |
| } | |
| if (ch === '/' && src[i + 1] === '*') { | |
| const end = src.indexOf('*/', i + 2); | |
| i = end === -1 ? src.length : end + 2; | |
| continue; | |
| } | |
| if (atInterpolation(src, i)) { | |
| const end = skipInterpolation(src, i); | |
| buf += src.slice(i, end); | |
| i = end; | |
| continue; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lib/scss-vars.mjs` around lines 71 - 94, In the scanner loop, move
the quoted-string branch in the function containing the shown while loop ahead
of both comment branches. Ensure skipString handles strings before detecting //
or /* so comment markers inside quoted values are preserved, while actual
comments continue using their existing logic.
Comment on lines
+238
to
+255
| export function registerVarsKeys(src) { | ||
| const keys = new Set(); | ||
| const re = /cv\.register-vars\s*\(/g; | ||
| let m; | ||
| while ((m = re.exec(src))) { | ||
| let depth = 1; | ||
| let i = re.lastIndex; | ||
| while (i < src.length && depth > 0) { | ||
| if (src[i] === '(') depth++; | ||
| else if (src[i] === ')') depth--; | ||
| i++; | ||
| } | ||
| const block = src.slice(re.lastIndex, i); | ||
| for (const k of block.matchAll(/['"]([a-z0-9-]+)['"][ \t]*:/g)) | ||
| keys.add(k[1]); | ||
| } | ||
| return keys; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
ESLint failure: m is assigned but never used.
Only re.lastIndex is consumed, so drop the binding to satisfy no-unused-vars.
🧹 Proposed fix
export function registerVarsKeys(src) {
const keys = new Set();
const re = /cv\.register-vars\s*\(/g;
- let m;
- while ((m = re.exec(src))) {
+ while (re.exec(src) !== null) {
let depth = 1;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function registerVarsKeys(src) { | |
| const keys = new Set(); | |
| const re = /cv\.register-vars\s*\(/g; | |
| let m; | |
| while ((m = re.exec(src))) { | |
| let depth = 1; | |
| let i = re.lastIndex; | |
| while (i < src.length && depth > 0) { | |
| if (src[i] === '(') depth++; | |
| else if (src[i] === ')') depth--; | |
| i++; | |
| } | |
| const block = src.slice(re.lastIndex, i); | |
| for (const k of block.matchAll(/['"]([a-z0-9-]+)['"][ \t]*:/g)) | |
| keys.add(k[1]); | |
| } | |
| return keys; | |
| } | |
| export function registerVarsKeys(src) { | |
| const keys = new Set(); | |
| const re = /cv\.register-vars\s*\(/g; | |
| while (re.exec(src) !== null) { | |
| let depth = 1; | |
| let i = re.lastIndex; | |
| while (i < src.length && depth > 0) { | |
| if (src[i] === '(') depth++; | |
| else if (src[i] === ')') depth--; | |
| i++; | |
| } | |
| const block = src.slice(re.lastIndex, i); | |
| for (const k of block.matchAll(/['"]([a-z0-9-]+)['"][ \t]*:/g)) | |
| keys.add(k[1]); | |
| } | |
| return keys; | |
| } |
🧰 Tools
🪛 ESLint
[error] 242-242: 'm' is assigned a value but never used.
(no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lib/scss-vars.mjs` around lines 238 - 255, Update the loop in
registerVarsKeys to avoid binding the unused regex match result m; iterate over
the regex execution without assigning its returned match object, while
preserving use of re.lastIndex and the existing key extraction behavior.
Source: Linters/SAST tools
Comment on lines
+31
to
+38
| import { | ||
| sectionSpans, | ||
| sectionBody, | ||
| splitLines, | ||
| openMarker, | ||
| closeMarker, | ||
| firstSentence, | ||
| } from './lib/api-page.mjs'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Unused import splitLines — ESLint error.
Flagged by ESLint ('splitLines' is defined but never used, no-unused-vars). splitLines is imported here but never referenced in this file.
As per coding guidelines, **/*.{ts,tsx,js,jsx,mjs,cjs}: "Run and satisfy ESLint, TypeScript checks, tests, formatting checks, and the repository's pre-PR quality gate before submitting changes."
🧹 Proposed fix
import {
sectionSpans,
sectionBody,
- splitLines,
openMarker,
closeMarker,
firstSentence,
} from './lib/api-page.mjs';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { | |
| sectionSpans, | |
| sectionBody, | |
| splitLines, | |
| openMarker, | |
| closeMarker, | |
| firstSentence, | |
| } from './lib/api-page.mjs'; | |
| import { | |
| sectionSpans, | |
| sectionBody, | |
| openMarker, | |
| closeMarker, | |
| firstSentence, | |
| } from './lib/api-page.mjs'; |
🧰 Tools
🪛 ESLint
[error] 34-34: 'splitLines' is defined but never used.
(no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/migrate-api-pages.mjs` around lines 31 - 38, Remove the unused
splitLines import from the import list in migrate-api-pages.mjs, leaving the
other API page helpers unchanged, then ensure the file satisfies the
no-unused-vars ESLint check.
Sources: Coding guidelines, Linters/SAST tools
`--category` went straight from argv into `join(REPO, 'bulma-ui', 'src', …)`, then `readdir` and, on a non-dry run, `writeFile` on everything found there. `--category=../../..` would walk out of the repo and rewrite files outside it. Resolve the argument against the directories that actually exist under src/ instead of interpolating it into a path. A typo now names the valid choices rather than failing with a bare ENOENT. Verified: `--category=../../..` exits 2 without touching the filesystem; `--category=layout` behaves as before. `pnpm gen` produces no diff and all 10 conformance checks pass. Note this does not yet clear the CodeQL check — it reports 1 high + 1 medium and the annotations are not readable from here (the repo uses CodeQL default setup, so there is no workflow file, and no code-scanning API is available to this session). This fixes a real traversal sink found by inspection; the flagged lines still need confirming against the actual report. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011c6ipfvBsYgF3Jf91ZzSr7
Contributor
Preview DeploymentPreview URL: https://0979af3d.bestax.pages.dev |
Groundwork for migrating the remaining API categories. `scripts/gen-api-sources.mjs` discovers the two maps that would otherwise be hand-written across ~95 components: which SCSS partial(s) register each component's CSS variables, and which pages deliberately import companions. Both are committed so the diff stays reviewable, and the script re-runs after a Bulma upgrade. Deriving the root class turned out to be the hard part, and the pilot hid it. Taking the first `usePrefixedClassNames` literal is only correct when a component renders one prefixed element — true for every layout component, false for 18 others. `Input` and `Select` resolved to `help`, `Toast` to `toast-container`, `Badge` to `badge-wrapper`, so those pages would have shown no variables at all. Match the component's own name against its candidate classes instead, resolve wrappers through their `*Base`, and return null when nothing matches confidently. `gen-api-sources` then REFUSES to emit rather than guessing, because a wrong root class silently attaches another component's variables to a page. Six components that the name rule cannot decide are named explicitly in ROOT_CLASS_OVERRIDES with the reason. That correction moves 53 -> 62 components with discoverable variables. A component can also legitimately draw from more than one partial: Bulma's form/shared.scss registers the `--bulma-input-*` set on a selector list covering .control/.input/.textarea/.select, so all four really do respond to them. Keep every match, ordered by path, and dedupe by variable name. Ordering and generation are now separate concerns. ORDERED_CATEGORIES covers every reference page including helpers/; MANAGED_CATEGORIES covers only pages that are really a component props table. Without the split, helpers/config.md and helpers/theme.md would keep Props near the top while every other page moved it down — the same inconsistency this work exists to remove, just smaller. `docs-section-order` also now constrains only the canonical sections, so a page's own `## API` or `## Supported Props` keeps its author's placement. helpers/ is reordered via a new --reorder-only mode (no markers, no codemod); live-example counts are unchanged on all six pages. form/checkbox.md's pre-existing duplicated footer sections are merged, keeping the colon style that matches the avatar exemplar. The layout pilot's generated output is byte-identical after all of this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011c6ipfvBsYgF3Jf91ZzSr7
… tables Wraps only the prop TABLES in the generated region rather than the whole `## Props` section. 15 pages carry hand-written prose there — admonitions, footnotes, and a 32-line prose sub-API in card.md — and the previous behaviour would have put all of it inside a region the generator overwrites. Prose above or below the tables now survives untouched. Prose BETWEEN tables cannot survive, so the migration refuses those pages and names the offending lines instead of eating them. Also fixes a real loss the columns migration exposed: `@deprecated` never reached the table. The hand-written rows led with "**Deprecated.** Use `gapMobile` instead."; the extractor read only the JSDoc comment text, so the five deprecated `gapSize*` props came out documented as ordinary props with no hint they are on the way out — the one thing a reader most needs. The tag is now rendered ahead of the description. columns/ and grid/ migrated (4 pages, 74 member TSDoc comments, 4 seeded summaries). Verified against origin/main: no default changed, no description shortened, no prop dropped, live-example counts identical. Generator is idempotent, typecheck and all 3678 tests pass. Remaining: elements (30), components (22), form (19). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011c6ipfvBsYgF3Jf91ZzSr7
Contributor
Preview DeploymentPreview URL: https://a074c819.bestax.pages.dev |
Completes the rollout started for layout/columns/grid: elements (30), components (22) and form (19) now carry the same generated regions, so all 87 pages share one section order and one table shape. A site with Props near the top on some pages and at the bottom on others is worse than either order applied consistently, which is what the partial rollout guaranteed. The requirement driving this commit is that a generated page carry the SAME OR MORE information than the hand-written one it replaces. Measuring that rather than assuming it turned up losses per-page spot checks had missed, and `scripts/check-docs-parity.mjs` now proves it: every page is diffed against origin/main and the run fails on a dropped prop, default, description word, code span, URL, live example or line of prose. It reports 0 losses across all 87 pages. Extraction fixes found by that gate: - Type aliases as props types. `ControlProps` and `SliderProps` are unions, and treating a props type as necessarily an interface rendered both pages with no table at all (42 props). - Heritage through `Omit`/`Pick` and imported (alias) symbols — 242 props were landing in the catch-all row instead of a table. - Defaults destructured in the body rather than the parameter (`Slider`, `TimeInputBase`), and defaults belonging to a base module a thin wrapper renders (`TimeInput` -> `TimeInputBase`). - Type cells no longer show a bare alias name: short unions inline (including `(typeof xs)[number]` and lookup types), long ones get a `**Types:**` footnote built from the alias's own TSDoc. - `**Subcomponents:**` renders each sub's summary sentence, which the hand-written pages carried and a bare name list dropped. - A sub-component with its own page is linked rather than restated, so `Columns.Column` does not duplicate column.md's 25 rows. - `ref` shows the forwarded element; MDX-hostile bare `<Tag>` text and colon-bearing frontmatter descriptions are escaped rather than breaking the docs build. The codemod now merges rather than replaces: where the page and the source comment say different things, both survive; where they say the same thing differently, the fuller wording wins. Props the tables documented but no interface declares are parked as `@extraProp`, carrying their type and default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011c6ipfvBsYgF3Jf91ZzSr7
Contributor
Preview DeploymentPreview URL: https://e58dadc4.bestax.pages.dev |
The markers tell gen-api-docs.mjs which regions it owns and warn a human editor which lines `pnpm gen` will overwrite. Neither audience reads the built site, but docusaurus-plugin-llms copies markdown from disk without stripping HTML comments, so they were landing ~600 times in llms-full.txt and in all 81 per-page .md twins — the files agents actually ingest. A build step rather than a Docusaurus plugin: postBuild hooks run under Promise.all, so declaring a plugin after docusaurus-plugin-llms does not make it run after. The first attempt did exactly that and stripped nothing, because it raced ahead of the files it was meant to clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011c6ipfvBsYgF3Jf91ZzSr7
Contributor
Preview DeploymentPreview URL: https://c4f89403.bestax.pages.dev |
`pnpm all` runs `test` and `test:coverage` inside one turbo invocation, so two jest processes execute e2e/kitchen-sink.test.ts concurrently. Both copied the fixture app into the same `.e2e-tmp/kitchen-sink` path and each began with an rmSync of it, so one process would wipe and re-copy the fixture while the other was asserting against its migrated output. The failure reads as though the codemod silently did nothing — "Expected substring: not 'react-bulma-components'" on a file that migrates correctly in isolation. mkdtemp gives each process its own directory, so the suites no longer interact under any scheduler. Cleaned up in afterAll; BESTAX_E2E_KEEP=1 keeps it for inspection. Reproduced before fixing rather than assumed: two identical runs stay in lockstep and never collide, but pairing an instrumented `--coverage` run against a plain one (the pnpm all case) failed 2 of 6 runs. The same harness passes 8 of 8 with the fix, and `pnpm all` is green three times over. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011c6ipfvBsYgF3Jf91ZzSr7
Contributor
Preview DeploymentPreview URL: https://5d1f9f1e.bestax.pages.dev |
…page win on wording Two problems, both found by measuring instead of assuming. CSS variables. `Block` showing no table was correct — Bulma's block.scss really does register none — but the audit behind that answer turned up six components that DO own variables and were showing nothing: Skeleton, Code, Pre, Strong, Delete and Column. Nothing to do with bulma-vs-repo (42 components already draw their tables from Bulma). Three causes, all now fixed: the scan skipped `sass/base` and `sass/utilities`, where Bulma keeps `--bulma-skeleton-*`, `--bulma-code|pre|strong-*` and `--bulma-delete-*`; registrations at `$variables-host` or inside a `@mixin` were rejected because they are not on the component's own selector; and the semantic wrappers own a variable family without rendering a Bulma class at all, so `varPrefix` now names that family separately from `rootClass`. Host registrations are claimed by key prefix, which is what keeps `body-*` and `hr-*` out of Code's table. The lead sentence adapts: claiming a component "registers these on its own element" is false when Bulma declares them at `:root`. Field and Navbar each gained a real variable too, and `gen-api-docs` may now CREATE the cssvars section — it is the one region conformance requires, so it was never opt-outable. Wording. The merge preferred whichever text had the larger word set, which let a stale source comment overwrite a hand-corrected page: "Offset for mobile." became "Mobile column offset." on six rows, and 114 props changed that way. The page is now authoritative for any prop it already documented — the comment only wins when it fully contains the page's sentence — and where the two disagree the codemod prints what it discarded, so the difference is reviewed rather than lost. Previously-documented props: 953 unchanged, 117 extended (old sentence intact), 12 merged, 1 replaced (a comma). `scripts/check-docs-wording.mjs` is the review aid: it buckets every previously-documented prop by how its description changed. Parity stays at 0 losses across all 87 pages. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011c6ipfvBsYgF3Jf91ZzSr7
Contributor
Preview DeploymentPreview URL: https://dca11dc8.bestax.pages.dev |
CI was red on this PR for `prettier --check scripts/check-docs-wording.mjs`, which `pnpm all` had reported green. Two gate gaps, same shape: - `pnpm all` ran `turbo run format:check`, which runs the PER-PACKAGE scripts — none of which cover `scripts/`, `docs/scripts/`, or any `.md`. CI runs the ROOT script, which does. `all` now calls the root one, so the pre-PR gate and CI check the same files. - Nothing linted `scripts/` at all: `pnpm lint` is `turbo run lint`, and only bulma-ui/create-bestax/bestax-migrate define a lint script, each scoped to their own src. `lint` now also runs eslint over `scripts` and `docs/scripts`, so CI picks it up without a workflow change. Verified by planting an unused variable and confirming the gate fails. That gap had hidden 7 eslint errors, now fixed: unused `m`, `splitLines`, `contentWords`/`subset` (dead after the mergeDescriptions rewrite), `UNKNOWN_SECTION_RANK`, a pre-existing unused `basename` import in check-conformance.mjs, and three `no-irregular-whitespace` from the zero-width characters used to render a nested `*/` in a comment. Also from CodeRabbit's review: `escapeCell` escapes backslashes before pipes (a cell ending in a backslash before a pipe produced a doubled escape, splitting the row), and `upsertFrontmatter` escapes the key before interpolating it into a RegExp. Its finding that the SCSS scanner mis-lexes `//` inside a quoted string is NOT valid — the scan is left-to-right, so the opening quote is always reached first and skipString consumes the literal. A/B tested with a `url()` containing `//` both inline and in a `!default`: identical output either way. Documented in place so it is not re-filed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011c6ipfvBsYgF3Jf91ZzSr7
This was referenced Jul 30, 2026
Owner
Author
Superseded by a 4-PR stackThis PR grew from the 24-file
The stack tip reproduces this PR's tree byte-for-byte over the paths it owns, rebased onto current Also split out: #419, the Review threads from this PR, resolved against current code
Leaving this open for you to close, since the history and review threads may still be worth referring back to. Generated by Claude Code |
Owner
Author
|
@coderabbitai full review Generated by Claude Code |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 59 minutes. |
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
3 participants
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.
Pull Request
Description
The API reference pages are hand-written, and the sections that merely restate the source drift
from it. Nothing in CI compared a props table to its interface, so the tables diverged in shape
(3-column in
elements/button.md, 4-column| Field |inlayout/footer.md,| Prop |in thecomponents/avatar.mdexemplar) and in content (button.mdrenderstextColoras`'primary'` | ... (see docs);hero.mdandcard.mdinstead dump 19-member colour unions).This takes machine ownership of the four derivable regions and leaves everything else alone.
Regions are delimited by
<!-- bestax:generated <id> -->markers:overviewimportprops<X>Propsinterfaces, via the TypeScript compiler APIcssvarsProse outside the markers is preserved byte-for-byte, so pages keep their admonitions and their
hand-written Usage / Accessibility / Related / Additional sections. Section order is now
Overview → Import → Usage → Accessibility → Related → Additional → Props → CSS & Sass Variables.
Deleting a marker pair opts a region out.
This is not typedoc. Three choices keep the output reading as hand-written:
the generator reads the interface's declared members and emits the one catch-all
...row thepages already wrote by hand.
checker.typeToString— the checker expands(typeof validColors)[number]into 19 literals. Wide colour unions instead become a link to theexisting Valid values page.
@propertyblocks,because the hand-written ones are consistently richer. Nothing curated was lost.
@propertyblocks migrate onto interface members as inline TSDoc, so descriptions are nowverifiable by position and show up in users' editors. Component TSDoc summaries were seeded from
the pages for the same reason — the existing ones ("Bulma Hero component root.") were worse than
the prose they'd have replaced.
Scope:
layout/only (6 pages).MANAGED_CATEGORIESinscripts/lib/api-sources.mjsgates therest; follow-ups add one category at a time.
Verification done
pre-migration version — no default changed, no description got shorter, no prop dropped.
textColor; Levelas,hasTextCentered,href,target,rel. Each verified against the interface.and render, zero runtime errors.
pnpm allgreen;component-catalog.mdregenerated byte-identical (the Overview round-trip isexact).
Two decisions worth a reviewer's opinion
footer.scssdeclares$footer-color: false !default. Rendered verbatim as`false`ratherthan
—, since sass-false means thecolor:declaration is skipped entirely and—would readas "no default". Easy to flip.
llms-full.txt(~46 now, ~350 once all 87 pages migrate);docusaurus-plugin-llmsdoes not strip HTML comments.llms.txtitself stays clean because thegenerator writes a frontmatter
description:. The alternative — an MDX<PropsTable/>— wouldremove the prop tables from that surface entirely, which seemed strictly worse.
Related Issue(s)
Refs #
Type of Change
Affected packages:
@allxsmith/bestax-bulma) — inline TSDoc onsrc/layout/*(comments only; thesummaries ship in the
.d.ts)@allxsmith/bestax-docs) — the six migrated pagesChecklist
CLAUDE.mdfiles are updated (
docs/CLAUDE.md,bulma-ui/CLAUDE.md,CONTRIBUTING-COMPONENTS.md)Screenshots / Demos
Best reviewed on the deploy preview, comparing each
layout/page against production:/docs/api/layout/container— note there is no CSS & Sass Variables section (Container hasSCSS but registers no CSS variables)
/docs/api/layout/footer—cv.getVarresolved tovar(--bulma-scheme-main-bis); thefalsedefault/docs/api/layout/hero—Hero.Head/Body/Footsub-tables, the inlinedHeroSizeunion,and exactly 8 CSS variables (Bulma registers
navbar-item-*,tabs-*,title-*andsubtitle-*inside Hero's per-colour loop; those are correctly excluded)
/docs/api/layout/level— one row, proving the singularcv.register-varpath/docs/api/layout/media— the six-linehsla()collapsed to one lineAdditional Context
CI gate placement. The staleness check lives in
scripts/check-conformance.mjsas two newsub-checks rather than a new workflow step:
docs-generatedrecomputes each managed page in memoryand diffs,
docs-section-orderholds the section order and marker presence. That file already runsin CI, and it keeps this PR out of
.github/**. Both failure modes were verified by tampering.registerVarsKeysmoved toscripts/lib/scss-vars.mjsso the generator shares one SCSS parser;key extraction is unchanged, verified key-for-key across all 26 repo partials (366 keys, 0 diffs).
New requirement: generating reads
node_modules/bulmafor the stock components' SCSS — adeliberate departure from
gen-component-catalog.mjs's no-install rule. CI installs first, and thescript fails with an actionable message rather than an
ENOENT. A Bulma version bump will nowproduce a real diff here, which is the point.
Generated by Claude Code
Summary by CodeRabbit
New Features
relattribute when rendering level items as links.Documentation
Bug Fixes