fix(rules): do not flag a lone control in a ButtonGroup - #30
Merged
Conversation
The rule reported any text control inside a ButtonGroup, including a
group holding nothing else. That is a false positive: ButtonGroup joins
with [&>*:not(:first-child)], so a single child is joined to nothing —
no squared corner, no dropped border, no seam, and no ring to bisect.
Report only when the group has more than one meaningful direct child. A
conditional child counts, because {loading ? <Spinner/> : <Button/>}
renders a sibling either way.
Found by a question rather than by the burn-in, which happened to
contain no lone-control groups; the earlier "no false positives across
nine projects" was true of that sample but overstated as a property of
the rule. Re-ran the burn-in after the fix: the same four true positives
still report, the same five projects stay silent.
Folded into 0.10.0 rather than a follow-up, since 0.10.0 has not been
published and the rule has not reached anyone yet.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe ChangesButtonGroup rule refinement
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/cli/src/rules/button-group-holds-only-buttons.ts`:
- Around line 164-179: Update countJoinedChildren to count rendered JSX children
only: ignore empty or comment-only JsxExpression containers, recursively flatten
JsxFragment children, and count their rendered descendants before the existing
>1 check. Add regression tests covering a comment alongside an element and a
fragment containing multiple elements.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d0a9593-bcf4-4c83-b79c-f12c50ff9511
📒 Files selected for processing (4)
CHANGELOG.mdchangelog/0.10.0.mdpackages/cli/src/rules/button-group-holds-only-buttons.tspackages/cli/test/button-group-holds-only-buttons.test.ts
Comment on lines
+164
to
+179
| const countJoinedChildren = (node: Node): number => { | ||
| if (!isJsxElement(node)) { | ||
| return 0; | ||
| } | ||
|
|
||
| return node.children.filter((child) => { | ||
| if (isJsxText(child)) { | ||
| return child.text.trim() !== ""; | ||
| } | ||
|
|
||
| return ( | ||
| isJsxElement(child) || | ||
| isJsxSelfClosingElement(child) || | ||
| isJsxExpression(child) | ||
| ); | ||
| }).length; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)button-group-holds-only-buttons\.ts$|node_modules/next/dist/docs|tsconfig|package.json' | sed -n '1,120p'
echo "== file outline =="
ast-grep outline packages/cli/src/rules/button-group-holds-only-buttons.ts --view expanded || true
echo "== relevant file lines =="
cat -n packages/cli/src/rules/button-group-holds-only-buttons.ts | sed -n '1,260p'
echo "== search helper definitions/usages =="
rg -n "countJoinedChildren|isJsxFragment|isJsxText|JsxFragment|ButtonGroup" packages/cli/src -S || trueRepository: TheOrcDev/shadscan
Length of output: 14474
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== TypeScript version declarations =="
node -e "const p=require('./package.json'); console.log(JSON.stringify({root:p.dependencies?.typescript||p.devDependencies?.typescript, cli:p.packages?.find?.??.dependencies||p.packages?.cli?.dependencies}, null, 2))" 2>/dev/null || cat package.json | rg -n '"typescript"|"`@types/node`"|workspace|name' || true
cat package.json | sed -n '1,120p'
cat packages/cli/package.json | sed -n '1,120p'
cat packages/cli/tsconfig.json | sed -n '1,120p'
cat tsconfig.json | sed -n '1,120p'
echo "== local typescript availability and helpers =="
node - <<'JS'
try {
const ts = require('./packages/cli/node_modules/typescript');
console.log('has packages/cli/node_modules/typescript', !!ts, ts.version);
console.log('isJsxFragment', typeof ts.isJsxFragment);
console.log('isJsxText', typeof ts.isJsxText);
console.log('isJsxExpression', typeof ts.isJsxExpression);
console.log('has JsxEmptyExpression?', ts.SyntaxKind && ts.SyntaxKind.JsxEmptyExpression);
} catch (e) {
console.log('no local packages/cli/node_modules/typescript', e && e.message);
}
try {
const ts = require('typescript');
console.log('has global typescript', !!ts, ts.version);
console.log('isJsxFragment', typeof ts.isJsxFragment);
console.log('isJsxText', typeof ts.isJsxText);
console.log('isJsxExpression', typeof ts.isJsxExpression);
console.log('has JsxEmptyExpression?', ts.SyntaxKind && ts.SyntaxKind.JsxEmptyExpression);
} catch (e) {
console.log('no global typescript', e && e.message);
}
JS
echo "== relevant AST helper implementations =="
cat -n packages/cli/src/ast.ts | sed -n '1,80p'
cat -n packages/cli/src/ast.ts | sed -n '420,490p'
cat -n packages/cli/src/component-render-graph/client-surface-planning.ts | sed -n '150,210p'
cat -n packages/cli/src/component-render-graph/template-extraction.ts | sed -n '660,690p'
echo "== AST behavior probe if TypeScript is available globally =="
node - <<'JS'
const ts = (() => { try { return require('typescript'); } catch { return null } })();
if (!ts) {
console.log('skipping behavioral TypeScript probe because global TypeScript is unavailable');
process.exit(0);
}
const samples = {
'comment expression': '<ButtonGroup>{/* comment */}<Input /></ButtonGroup>',
'fragment with two inputs': '<ButtonGroup><><Input /><Input /></></ButtonGroup>',
'single input': '<ButtonGroup><Input /></ButtonGroup>',
'fragment containing InputGroup': '<ButtonGroup><><InputGroup /><Button /></></ButtonGroup>',
};
for (const [name, src] of Object.entries(samples)) {
const sf = ts.createSourceFile('/x.ts', src, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX);
const childCounts = {
directChildren: sf.getChildAt(0).children.length,
directChildrenNames: sf.getChildAt(0).children.map(n => ts.SyntaxKind[n.kind]).join(', '),
};
const firstJsxElement = sf.getChildAt(0).children.find(n => ts.isJsxElement(n));
console.log(JSON.stringify({
sample: name,
src,
childCounts,
buttonGroupChildren: firstJsxElement ? firstJsxElement.children.map(n => ({
kind: ts.SyntaxKind[n.kind],
expressionKind: ts.isJsxExpression(n) && n.expression && ts.SyntaxKind[n.expression.kind],
fragmentChildCount: ts.isJsxFragment(n) && n.children.length,
renderedCount: ts.isJsxExpression(n) ? (n.expression && n.expression.escapedText) : null,
})) : null,
}, null, 2));
}
JSRepository: TheOrcDev/shadscan
Length of output: 16249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const ts = require('typescript');
const samples = {
'comment expression': '<ButtonGroup>{/* comment */}<Input /></ButtonGroup>',
'fragment with two inputs': '<ButtonGroup><><Input /><Input /></></ButtonGroup>',
'single input': '<ButtonGroup><Input /></ButtonGroup>',
'fragment containing InputGroup and Button': '<ButtonGroup><><InputGroup /><Button /></></ButtonGroup>',
};
for (const [name, src] of Object.entries(samples)) {
const sf = ts.createSourceFile('/x.tsx', src, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX);
const topChildren = sf.statements[0].children;
const buttonGroup = topChildren.find((n) => ts.isJsxElement(n) && ts.getFullyQualifiedName(n.name) === 'ButtonGroup');
const countJoinedChildrenCurrent = (node) => {
if (!ts.isJsxElement(node)) return 0;
let count = 0;
for (const child of node.children) {
if (ts.isJsxText(child)) {
if (child.text.trim() !== '') count++;
continue;
}
if (
ts.isJsxElement(child) ||
ts.isJsxSelfClosingElement(child) ||
ts.isJsxExpression(child)
) {
count++;
}
}
return count;
};
console.log(JSON.stringify({
sample: name,
src,
topChildrenCount: topChildren.length,
topChildrenKinds: topChildren.map(n => ts.SyntaxKind[n.kind]).join(', '),
hasButtonGroup: !!buttonGroup,
buttonGroupDirectChildrenCount: buttonGroup ? buttonGroup.children.length : 0,
buttonGroupDirectChildMatchesCurrent: buttonGroup ? buttonGroup.children.map((child) => ({
kind: ts.SyntaxKind[child.kind],
includedByCurrent: !!child && (
ts.isJsxElement(child) ||
ts.isJsxSelfClosingElement(child) ||
ts.isJsxExpression(child) ||
(ts.isJsxText(child) && child.text.trim() !== '')
),
expressionKind: ts.isJsxExpression(child) && child.expression ? ts.SyntaxKind[child.expression.kind] : undefined,
fragmentChildCount: ts.isJsxFragment(child) && child.children.length,
})) : null,
buttonGroupCountWithCurrentImpl: buttonGroup ? countJoinedChildrenCurrent(buttonGroup) : 0,
}, null, 2));
}
JSRepository: TheOrcDev/shadscan
Length of output: 896
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const ts = require('typescript');
const samples = {
'comment expression': '<ButtonGroup>{/* comment */}<Input /></ButtonGroup>',
'fragment with two inputs': '<ButtonGroup><><Input /><Input /></></ButtonGroup>',
'single input': '<ButtonGroup><Input /></ButtonGroup>',
'fragment containing InputGroup and Button': '<ButtonGroup><><InputGroup /><Button /></></ButtonGroup>',
};
for (const [name, src] of Object.entries(samples)) {
const sf = ts.createSourceFile('/x.tsx', src, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX);
const stmt = sf.statements[0];
const topChildren = stmt && stmt.children;
const buttonGroup = topChildren && topChildren.find((n) => ts.isJsxElement(n) && n.name && n.name.text === 'ButtonGroup');
const countJoinedChildrenCurrent = (node) => {
if (!ts.isJsxElement(node)) return 0;
let count = 0;
for (const child of node.children) {
if (ts.isJsxText(child)) {
if (child.text.trim() !== '') count++;
continue;
}
if (
ts.isJsxElement(child) ||
ts.isJsxSelfClosingElement(child) ||
ts.isJsxExpression(child)
) {
count++;
}
}
return count;
};
const filtered = [];
for (const child of (buttonGroup ? buttonGroup.children : [])) {
filtered.push({
kind: ts.SyntaxKind[child.kind],
includedByCurrent: !!child && (
ts.isJsxElement(child) ||
ts.isJsxSelfClosingElement(child) ||
ts.isJsxExpression(child) ||
(ts.isJsxText(child) && child.text.trim() !== '')
),
expressionKind: ts.isJsxExpression(child) && child.expression ? ts.SyntaxKind[child.expression.kind] : undefined,
fragmentChildCount: ts.isJsxFragment(child) && child.children.length,
});
}
console.log(JSON.stringify({
sample: name,
src,
topStatementIsJsxElement: ts.isJsxElement(stmt),
topStatementChildrenCount: topChildren ? topChildren.length : 0,
topChildrenKinds: topChildren ? topChildren.map((n) => ts.SyntaxKind[n.kind]).join(', ') : '',
hasButtonGroup,
buttonGroupDirectChildrenCount: buttonGroup ? buttonGroup.children.length : 0,
buttonGroupChildMatchesCurrent: filtered,
buttonGroupCountWithCurrentImpl: buttonGroup ? countJoinedChildrenCurrent(buttonGroup) : 0,
}, null, 2));
}
JSRepository: TheOrcDev/shadscan
Length of output: 746
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const ts = require('typescript');
const samples = {
'comment expression': '<ButtonGroup>{/* comment */}<Input /></ButtonGroup>',
'fragment with two inputs': '<ButtonGroup><><Input /><Input /></></ButtonGroup>',
'single input': '<ButtonGroup><Input /></ButtonGroup>',
'fragment containing InputGroup and Button': '<ButtonGroup><><InputGroup /><Button /></></ButtonGroup>',
};
for (const [name, src] of Object.entries(samples)) {
const sf = ts.createSourceFile('/x.tsx', src, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX);
const stmt = sf.statements[0];
const topChildren = (stmt && stmt.children) || [];
const buttonGroup = topChildren.find((n) => ts.isJsxElement(n) && n.name && n.name.text === 'ButtonGroup');
const hasButtonGroup = !!buttonGroup;
const countJoinedChildrenCurrent = (node) => {
if (!ts.isJsxElement(node)) return 0;
let count = 0;
for (const child of node.children) {
if (ts.isJsxText(child)) {
if (child.text.trim() !== '') count++;
continue;
}
if (
ts.isJsxElement(child) ||
ts.isJsxSelfClosingElement(child) ||
ts.isJsxExpression(child)
) {
count++;
}
}
return count;
};
const filtered = [];
for (const child of buttonGroup ? buttonGroup.children : []) {
filtered.push({
kind: ts.SyntaxKind[child.kind],
includedByCurrent: ts.isJsxElement(child) ||
ts.isJsxSelfClosingElement(child) ||
ts.isJsxExpression(child) ||
((ts.isJsxText(child) && child.text.trim() !== '')),
expressionKind: ts.isJsxExpression(child) && child.expression ? ts.SyntaxKind[child.expression.kind] : undefined,
fragmentChildCount: ts.isJsxFragment(child) && child.children.length,
});
}
console.log(JSON.stringify({
sample: name,
src,
topStatementIsJsxElement: ts.isJsxElement(stmt),
topChildrenCount: topChildren.length,
topChildrenKinds: topChildren.map((n) => ts.SyntaxKind[n.kind]).join(', '),
hasButtonGroup,
buttonGroupDirectChildrenCount: buttonGroup ? buttonGroup.children.length : 0,
buttonGroupChildMatchesCurrent: filtered,
buttonGroupCountWithCurrentImpl: buttonGroup ? countJoinedChildrenCurrent(buttonGroup) : 0,
}, null, 2));
}
JSRepository: TheOrcDev/shadscan
Length of output: 1508
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const ts = require('typescript');
const src = '<ButtonGroup>{/* comment */}<Input /></ButtonGroup>';
const sf = ts.createSourceFile('/x.tsx', src, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX);
console.log(sf.transformFlags ? 'has transformFlags' : 'no transformFlags');
console.log('raw source length', src.length);
console.log('sf.text', JSON.stringify(sf.text));
for (const key of Object.keys(sf).sort()) {
if (key[0] === '_' || key === 'statements' || key === 'end' || key === 'flags' || key === 'kind' || key === 'transformFlags') {
const val = sf[key];
if (Array.isArray(val)) {
console.log(`${key}: [${val.length}]`, val.map((n) => n && { kind: ts.SyntaxKind[n.kind], text: (n.text || '') }).slice(0, 3));
} else {
console.log(`${key}:`, val);
}
}
}
const exprs = sf.statements[0].expression.expressions;
for (const e of exprs) {
console.log('statement expression child', ts.SyntaxKind[e.kind], e.toString && e.toString(), e.children && e.children.length, e.text && JSON.stringify(e.text), e.expression && { kind: ts.SyntaxKind[e.expression.kind] });
}
JSRepository: TheOrcDev/shadscan
Length of output: 960
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const ts = require('typescript');
const src = '<ButtonGroup>{/* comment */}<Input /></ButtonGroup>';
const sf = ts.createSourceFile('/x.tsx', src, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX);
const stmt = sf.statements[0];
console.log('statement kind', ts.SyntaxKind[stmt.kind]);
console.log('statement expression kind', stmt.expression && ts.SyntaxKind[stmt.expression.kind]);
console.log('statement expression type kind', stmt.expression && stmt.expression.type && ts.SyntaxKind[stmt.expression.type.kind]);
console.log('statement expression expressions?', Array.isArray(stmt.expression.expressions));
for (const key of ['expression', 'childList', 'elements', 'properties', 'arguments', 'expressions']) {
const val = stmt.expression && stmt.expression[key];
if (val) console.log(`expression.${key}`, typeof val, Array.isArray(val) ? val.length : val);
}
console.log('has forEachChild', typeof ts.forEachChild);
if (typeof ts.forEachChild === 'function') {
const expr = stmt.expression;
const collected = [];
ts.forEachChild(expr, (child) => {
collected.push({ kind: ts.SyntaxKind[child.kind], expressionKind: child.expression && ts.SyntaxKind[child.expression && child.expression.kind] });
});
console.log('forEachChild collected', collected);
}
const jsxFragments = sf.statements[0].expression;
for (const prop of ['jsxFragments', 'jsxTypeArgumentElements', 'jsxAttributes']) {
console.log('expression.', prop, Array.isArray(jsxFragments[prop]));
}
JSRepository: TheOrcDev/shadscan
Length of output: 706
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const ts = require('typescript');
const cases = [
{ label: 'comment expression', src: '<ButtonGroup>{/* comment */}<Input /></ButtonGroup>' },
{ label: 'fragment with two inputs', src: '<ButtonGroup><><Input /><Input /></></ButtonGroup>' },
{ label: 'single input', src: '<ButtonGroup><Input /></ButtonGroup>' },
{ label: 'fragment containing InputGroup and Button', src: '<ButtonGroup><><InputGroup /><Button /></></ButtonGroup>' },
];
for (const { label, src } of cases) {
const sf = ts.createSourceFile('/x.tsx', src, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX);
const stmt = sf.statements[0];
const buttonGroup = stmt && stmt.expression;
const currentFn = (node) => {
if (!ts.isJsxElement(node)) return 0;
let count = 0;
for (const child of node.children) {
if (ts.isJsxText(child)) {
if (child.text.trim() !== '') count++;
continue;
}
if (ts.isJsxElement(child) || ts.isJsxSelfClosingElement(child) || ts.isJsxExpression(child)) {
count++;
}
}
return count;
};
console.log(JSON.stringify({
label,
src,
countJoinedChildren: currentFn(buttonGroup),
directChildren: buttonGroup.children.map((child) => {
const included = ts.isJsxElement(child) || ts.isJsxSelfClosingElement(child) || ts.isJsxExpression(child) || ((ts.isJsxText(child) && child.text.trim() !== ''));
return {
kind: ts.SyntaxKind[child.kind],
included,
expressionKind: ts.isJsxExpression(child) && child.expression ? ts.SyntaxKind[child.expression.kind] : undefined,
fragmentChildCount: ts.isJsxFragment(child) && child.children.length,
};
}),
}, null, 2));
}
JSRepository: TheOrcDev/shadscan
Length of output: 1300
Count rendered children, not every JSX child node.
countJoinedChildren treats {/* comment */} as a joined child, so <ButtonGroup>{/* comment */}<Input /></ButtonGroup> reports as joined even though the comment does not render. It also skips JsxFragment, so <ButtonGroup><><Input /><Button /></></ButtonGroup> is treated as a lone child while it joins two rendered elements. Ignore expression containers without an expression and flatten fragment children before applying the > 1 check. Add regression tests for both cases.
🤖 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 `@packages/cli/src/rules/button-group-holds-only-buttons.ts` around lines 164 -
179, Update countJoinedChildren to count rendered JSX children only: ignore
empty or comment-only JsxExpression containers, recursively flatten JsxFragment
children, and count their rendered descendants before the existing >1 check. Add
regression tests covering a comment alongside an element and a fragment
containing multiple elements.
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.
button-group-holds-only-buttonsreported a text control inside anyButtonGroup, including one holding nothing else. That's a false positive.ButtonGroupjoins its children with[&>*:not(:first-child)]:rounded-l-noneandborder-l-0. With a single child those selectors never match — no squared corner, no dropped border, no seam, and therefore no ring to bisect. There is nothing to report.The fix
Report only when the group has more than one meaningful direct child. A conditional child counts —
{loading ? <Spinner/> : <Button/>}renders a sibling either way — so the common loading-state shape still reports.How it was found, and what that says about the burn-in
Not by the burn-in. By the question "this rule is only when there are button and input right?" — which I could have answered from memory and got wrong, so I probed it instead and the probe came back
advisory.The original burn-in ran across nine projects and found no false positives, which I reported as a property of the rule. It was a property of that sample: none of the nine contained a lone-control
ButtonGroup. Worth stating plainly, because the same caveat still applies to what's left — nine projects, one author.Verification
orcdev,github-creature,star-history,youtubetoblog) and the same five stay silent. The fix narrowed the rule without blunting it.cli:smoke. Self-audit 100/100 A.Release handling
Folded into 0.10.0 rather than shipped as a follow-up: 0.10.0 is prepared but not yet published to npm, so the rule has not reached anyone. The alternative — publishing a known false positive and fixing it in 0.10.1 — would be strictly worse. Both changelog entries updated to describe the lone-child exclusion as part of the rule's contract.
Summary by CodeRabbit
Bug Fixes
ButtonGroupcomponents containing only one rendered child.Documentation