docs: update docs and templates for category reclassification#83
Closed
docs: update docs and templates for category reclassification#83
Conversation
Reclassify categories: layout + ai-readability → structure, handoff-risk → behavior. Merge overlapping rules: no-auto-layout absorbs ambiguous-structure + missing-layout-hint, fixed-size-in-auto-layout absorbs fixed-width-in-responsive-context, missing-min-width + missing-max-width → missing-size-constraint, invisible-layer + empty-frame → unnecessary-node, hardcode-risk removed (covered by absolute-position-in-auto-layout). Add 3 new rules: overflow-behavior-unknown, wrap-behavior-unknown, variant-structure-mismatch. Final count: structure(9) token(7) component(4) naming(5) behavior(4) = 29 rules. Closes #79 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…-size) Merged rules should provide more detailed fix guidance that covers all absorbed symptoms, not just the base rule's original fix. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Was too vague ("Use a consistent naming convention"). Now includes
specific examples and explains why AI needs consistent naming.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…checks, test fixes - Add INSTANCE to isContainerNode helper (structure rules) - Guard noUncheckedIndexedAccess: early continue for undefined children - Skip wrap-behavior-unknown when children lack bounding box data - Add +1 tolerance comment for floating-point rounding in overflow check - Fix misleading test title for hidden overlapping children case - Add 48x48 boundary test for unnecessary-node placeholder exemption Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…c assertions - Add boundary-condition tests for text-truncation-unhandled (length=50, width=300, length=51) - Fix non-deterministic test in responsive-fields: remove if-guard, assert deterministically - Rename misleading test title to match actual expectation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TypeScript strict types + JSDoc on public APIs serve as documentation. The 80% threshold wasn't respecting the configured 40% override anyway. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… accuracy - Rename test titles to match actual scenarios (both missing vs one missing) - Add minWidth to fixture that tests maxWidth-only path - Add "only minWidth missing" test for complete coverage of 3 violation paths Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…fication Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughRefactors rule taxonomy from six categories to five: merges "Layout" + "AI Readability" → "Structure", renames "Handoff Risk" → "Behavior", moves/merges many rules (deleted old modules, added Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/agents/gap-rule-report.test.ts (1)
80-81:⚠️ Potential issue | 🟡 MinorInconsistent test fixture uses invalid category
"spacing".This test fixture uses
category: "spacing", which is not a valid category in the new taxonomy (structure | token | component | naming | behavior). If this test is intentionally validating handling of unknown/invalid categories, consider adding a comment to clarify. Otherwise, update to a valid category like"token"or"structure".🔧 Suggested fix if this should use a valid category
gaps: [ { - category: "spacing", + category: "token", description: "Padding off by 4px", actionable: true, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/gap-rule-report.test.ts` around lines 80 - 81, The test fixture in src/agents/gap-rule-report.test.ts is using an invalid category value "spacing"; update the fixture's category field to one of the valid taxonomy values (e.g., "token" or "structure") or, if the intent is to test handling of unknown categories, add a concise inline comment next to the fixture's category field clarifying that this is deliberately invalid; locate the fixture by the object containing category and description ("Padding off by 4px") and either replace "spacing" with the chosen valid category or add the explanatory comment.src/core/rules/structure/z-index-dependent-layout.test.ts (1)
58-61:⚠️ Potential issue | 🟡 MinorAdd test for 20% overlap boundary condition.
The rule flags overlaps exceeding 20% of the smaller element's area (implementation:
overlapArea > smallerArea * 0.2), but the test suite lacks a case for exactly 20% overlap. Add a test verifying that 20% overlap returnsnullto prevent regression if the threshold comparison is inadvertently changed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/rules/structure/z-index-dependent-layout.test.ts` around lines 58 - 61, Add a test in z-index-dependent-layout.test.ts that constructs two sibling nodes with sizes and positions producing exactly 20% overlap of the smaller element (use the existing helpers makeNode and makeContext) and call zIndexDependentLayout.check(node, makeContext()) asserting it returns null; ensure the test description mentions the "20% overlap boundary" and use the same FRAME parent pattern as the other tests so it guards against changing the strict greater-than comparison.src/core/rules/structure/responsive-fields.test.ts (1)
129-152: 🧹 Nitpick | 🔵 TrivialConsider parameterizing the duplicated “missing max-width with only minWidth” scenarios.
These two cases validate the same condition and could be folded into a table-driven test to reduce maintenance overhead.
Also applies to: 177-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/rules/structure/responsive-fields.test.ts` around lines 129 - 152, Two tests duplicate the same assertion for "missing max-width when minWidth is set" (one at the shown block and another at 177-203); replace them with a single table-driven test (using test.each or similar) that iterates scenarios (e.g., different container widths or layoutMode) and calls makeFile/makeNode and analyzeFile for each; for each row assert the result includes an issue with rule id "missing-size-constraint" and that the first violation message contains "max-width". Ensure you reference the existing helpers makeFile, makeNode, and analyzeFile and keep the same filter on i.rule.definition.id === "missing-size-constraint".docs/REFERENCE.md (1)
109-119:⚠️ Potential issue | 🟡 MinorDocumentation severity/score mismatches for token rules.
Several token rules have incorrect severity and score values compared to
src/core/rules/rule-config.ts:
Rule Doc Actual (rule-config.ts) raw-shadow-7 / risk -3 / missing-info raw-opacity-5 / risk -2 / missing-info 📝 Proposed fix
| `magic-number-spacing` | -3 | missing-info | -| `raw-shadow` | -7 | risk | -| `raw-opacity` | -5 | risk | +| `raw-shadow` | -3 | missing-info | +| `raw-opacity` | -2 | missing-info | | `multiple-fill-colors` | -3 | missing-info |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/REFERENCE.md` around lines 109 - 119, Update the documentation entries for the token rules `raw-shadow` and `raw-opacity` so their Default Score and Default Severity match the canonical values defined in the rule configuration: change `raw-shadow` from "-7 / risk" to "-3 / missing-info" and change `raw-opacity` from "-5 / risk" to "-2 / missing-info" in the table of token rules; ensure the table cells for those two rules exactly reflect the rule-config values for score and severity.src/core/rules/rule-config.ts (1)
229-261: 🧹 Nitpick | 🔵 TrivialConsider whether the string-matching heuristics sufficiently capture the intended rules.
The preset filtering uses
ruleId.includes()substring matching:
dev-friendly: matchesauto-layout,responsive,truncation,overflow,wrap,sizeai-ready: matchesauto-layout,structure,name,unnecessaryThis approach is fragile—future rule IDs containing these substrings would be inadvertently included/boosted. Consider using an explicit allowlist of rule IDs for more predictable behavior.
♻️ Example using explicit allowlists
case "dev-friendly": - // Focus on structure and behavior issues - for (const [id, config] of Object.entries(configs)) { - const ruleId = id as RuleId; - if ( - !ruleId.includes("auto-layout") && - !ruleId.includes("responsive") && - !ruleId.includes("truncation") && - !ruleId.includes("overflow") && - !ruleId.includes("wrap") && - !ruleId.includes("size") - ) { - configs[ruleId] = { ...config, enabled: false }; - } - } + const devFriendlyRules: RuleId[] = [ + "no-auto-layout", + "absolute-position-in-auto-layout", + "fixed-size-in-auto-layout", + "missing-size-constraint", + "missing-responsive-behavior", + "text-truncation-unhandled", + "overflow-behavior-unknown", + "wrap-behavior-unknown", + ]; + for (const [id, config] of Object.entries(configs)) { + const ruleId = id as RuleId; + if (!devFriendlyRules.includes(ruleId)) { + configs[ruleId] = { ...config, enabled: false }; + } + } break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/rules/rule-config.ts` around lines 229 - 261, The substring-matching in the "dev-friendly" and "ai-ready" blocks (inside the switch cases labeled "dev-friendly" and "ai-ready", iterating over configs and using ruleId.includes(...)) is fragile; replace the heuristic with explicit allowlists of rule IDs (e.g., const devFriendlyAllowlist = [...]; const aiReadyAllowlist = [...]) and test membership with devFriendlyAllowlist.includes(ruleId) and aiReadyAllowlist.includes(ruleId) respectively, then apply the existing mutations (set enabled: false for dev-friendly, adjust score for ai-ready) only when the ruleId is in the corresponding allowlist; update the allowlists to enumerate the intended RuleId values so future rule IDs won’t be accidentally matched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.coderabbit.yaml:
- Around line 96-98: The pre-merge check configuration uses the wrong keys:
replace the pre_merge_checks entry named docstring_coverage with the
schema-correct key docstrings and change the disabling property from enabled:
false to mode: "off" so the check is actually disabled; update the block that
references pre_merge_checks -> docstring_coverage to use pre_merge_checks ->
docstrings and set mode: "off".
In `@docs/REFERENCE.md`:
- Line 128: The docs entry for `variant-structure-mismatch` currently lists a
score of `-5` but the source constant in src/core/rules/rule-config.ts defines
it as `-4`; update the table row in docs/REFERENCE.md so the score matches the
authoritative value (`-4`) for the `variant-structure-mismatch` rule (or
alternatively update the constant in `rule-config.ts` if the intended canonical
score is `-5`)—locate the identifier `variant-structure-mismatch` in the docs
and make the numeric value consistent with the `rule-config.ts` definition.
- Around line 104-107: The docs table entry for `unnecessary-node` is out of
sync with the implementation; update the documentation to match the canonical
score in src/core/rules/rule-config.ts by changing the `unnecessary-node` score
in REFERENCE.md from `-1` to `-2` (or alternatively change the value in
rule-config.ts if the intended score is `-1`) so both sources agree; target the
`unnecessary-node` identifier in the docs or the `rule-config` definition to
resolve the mismatch.
- Around line 140-147: Update the docs table under "Behavior (4 rules)" so the
score and severity for the listed rules match the canonical values from
src/core/rules/rule-config.ts: change `prototype-link-in-design` severity from
"suggestion" to "missing-info" (score remains -2), change
`overflow-behavior-unknown` score from -5 to -3 and severity from "risk" to
"missing-info", and change `wrap-behavior-unknown` score from -5 to -3 and
severity from "risk" to "missing-info"; edit the entries for the rule IDs
`prototype-link-in-design`, `overflow-behavior-unknown`, and
`wrap-behavior-unknown` in the docs to reflect these exact values.
In `@src/core/rules/behavior/prototype-link-in-design.test.ts`:
- Line 1: Remove the unused import by editing the import statement that
currently reads "import { makeNode, makeFile, makeContext } from
"../test-helpers.js";" in prototype-link-in-design.test.ts — drop "makeFile" so
it becomes "import { makeNode, makeContext } from \"../test-helpers.js\"";
ensure there are no other references to makeFile in the file before committing.
In `@src/core/rules/behavior/wrap-behavior-unknown.test.ts`:
- Line 1: The import list includes an unused symbol makeFile; remove makeFile
from the import statement (leaving makeNode and makeContext) in the top-of-file
import that currently reads "import { makeNode, makeFile, makeContext } from
\"../test-helpers.js\";" so the test file only imports used helpers and
eliminates the unused makeFile import.
In `@src/core/rules/structure/index.ts`:
- Around line 167-236: The horizontal-only branch in fixedSizeInAutoLayoutCheck
has confusing/partially unreachable checks: when hFixed is true we should first
test whether node.layoutSizingHorizontal is defined and, if so, require it to
equal "FIXED"; otherwise (undefined) use the fallback checks on
node.layoutAlign. Update the hFixed && !vFixed block in
fixedSizeInAutoLayoutCheck to: if node.layoutSizingHorizontal !== undefined then
return null unless it equals "FIXED"; else (layoutSizingHorizontal undefined)
perform the existing layoutAlign checks (skip for "STRETCH", skip unless
"INHERIT"), keeping the excluded-name and return message logic unchanged.
In `@src/core/rules/structure/no-auto-layout.test.ts`:
- Line 1: The import statement in this test file includes an unused symbol:
remove the unused import makeFile from the import list (currently importing
makeNode, makeFile, makeContext) so only the used helpers (makeNode and
makeContext) remain; update the import line that references makeFile to exclude
it.
In `@src/core/rules/structure/unnecessary-node.test.ts`:
- Line 1: The import statement includes an unused symbol makeFile; update the
import at the top that currently reads "import { makeNode, makeFile, makeContext
} from \"../test-helpers.js\";" to remove makeFile so it only imports makeNode
and makeContext, ensuring tests still compile and no unused imports remain.
---
Outside diff comments:
In `@docs/REFERENCE.md`:
- Around line 109-119: Update the documentation entries for the token rules
`raw-shadow` and `raw-opacity` so their Default Score and Default Severity match
the canonical values defined in the rule configuration: change `raw-shadow` from
"-7 / risk" to "-3 / missing-info" and change `raw-opacity` from "-5 / risk" to
"-2 / missing-info" in the table of token rules; ensure the table cells for
those two rules exactly reflect the rule-config values for score and severity.
In `@src/agents/gap-rule-report.test.ts`:
- Around line 80-81: The test fixture in src/agents/gap-rule-report.test.ts is
using an invalid category value "spacing"; update the fixture's category field
to one of the valid taxonomy values (e.g., "token" or "structure") or, if the
intent is to test handling of unknown categories, add a concise inline comment
next to the fixture's category field clarifying that this is deliberately
invalid; locate the fixture by the object containing category and description
("Padding off by 4px") and either replace "spacing" with the chosen valid
category or add the explanatory comment.
In `@src/core/rules/rule-config.ts`:
- Around line 229-261: The substring-matching in the "dev-friendly" and
"ai-ready" blocks (inside the switch cases labeled "dev-friendly" and
"ai-ready", iterating over configs and using ruleId.includes(...)) is fragile;
replace the heuristic with explicit allowlists of rule IDs (e.g., const
devFriendlyAllowlist = [...]; const aiReadyAllowlist = [...]) and test
membership with devFriendlyAllowlist.includes(ruleId) and
aiReadyAllowlist.includes(ruleId) respectively, then apply the existing
mutations (set enabled: false for dev-friendly, adjust score for ai-ready) only
when the ruleId is in the corresponding allowlist; update the allowlists to
enumerate the intended RuleId values so future rule IDs won’t be accidentally
matched.
In `@src/core/rules/structure/responsive-fields.test.ts`:
- Around line 129-152: Two tests duplicate the same assertion for "missing
max-width when minWidth is set" (one at the shown block and another at 177-203);
replace them with a single table-driven test (using test.each or similar) that
iterates scenarios (e.g., different container widths or layoutMode) and calls
makeFile/makeNode and analyzeFile for each; for each row assert the result
includes an issue with rule id "missing-size-constraint" and that the first
violation message contains "max-width". Ensure you reference the existing
helpers makeFile, makeNode, and analyzeFile and keep the same filter on
i.rule.definition.id === "missing-size-constraint".
In `@src/core/rules/structure/z-index-dependent-layout.test.ts`:
- Around line 58-61: Add a test in z-index-dependent-layout.test.ts that
constructs two sibling nodes with sizes and positions producing exactly 20%
overlap of the smaller element (use the existing helpers makeNode and
makeContext) and call zIndexDependentLayout.check(node, makeContext()) asserting
it returns null; ensure the test description mentions the "20% overlap boundary"
and use the same FRAME parent pattern as the other tests so it guards against
changing the strict greater-than comparison.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c089e591-38f3-4869-b93c-201901371ce1
📒 Files selected for processing (52)
.claude/agents/calibration/converter.md.claude/agents/rule-discovery/designer.md.coderabbit.yaml.github/ISSUE_TEMPLATE/bug.ymldocs/CALIBRATION.mddocs/MCP-VS-CLI.mddocs/REFERENCE.mddocs/SCORING.mddocs/fixtures/ANALYSIS-VALIDITY-FEEDBACK.mdsrc/agents/analysis-agent.test.tssrc/agents/evidence-collector.test.tssrc/agents/gap-rule-report.test.tssrc/agents/orchestrator.test.tssrc/agents/report-generator.test.tssrc/core/contracts/category.tssrc/core/contracts/rule.tssrc/core/engine/integration.test.tssrc/core/engine/rule-engine.test.tssrc/core/engine/scoring.test.tssrc/core/engine/scoring.tssrc/core/report-html/index.tssrc/core/rules/ai-readability/ambiguous-structure.test.tssrc/core/rules/ai-readability/empty-frame.test.tssrc/core/rules/ai-readability/index.tssrc/core/rules/ai-readability/invisible-layer.test.tssrc/core/rules/ai-readability/missing-layout-hint.test.tssrc/core/rules/behavior/index.tssrc/core/rules/behavior/overflow-behavior-unknown.test.tssrc/core/rules/behavior/prototype-link-in-design.test.tssrc/core/rules/behavior/text-truncation-unhandled.test.tssrc/core/rules/behavior/wrap-behavior-unknown.test.tssrc/core/rules/component/index.tssrc/core/rules/component/variant-structure-mismatch.test.tssrc/core/rules/custom/custom-rule-loader.test.tssrc/core/rules/handoff-risk/hardcode-risk.test.tssrc/core/rules/handoff-risk/prototype-link-in-design.test.tssrc/core/rules/index.tssrc/core/rules/layout/absolute-position.test.tssrc/core/rules/layout/deep-nesting.test.tssrc/core/rules/layout/fixed-width-in-responsive-context.test.tssrc/core/rules/layout/group-usage.test.tssrc/core/rules/layout/index.tssrc/core/rules/layout/missing-responsive-behavior.test.tssrc/core/rules/layout/no-auto-layout.test.tssrc/core/rules/naming/index.tssrc/core/rules/rule-config.tssrc/core/rules/structure/index.tssrc/core/rules/structure/no-auto-layout.test.tssrc/core/rules/structure/responsive-fields.test.tssrc/core/rules/structure/unnecessary-node.test.tssrc/core/rules/structure/z-index-dependent-layout.test.tssrc/core/ui-constants.ts
💤 Files with no reviewable changes (14)
- src/core/rules/ai-readability/empty-frame.test.ts
- src/core/rules/layout/group-usage.test.ts
- src/core/rules/layout/no-auto-layout.test.ts
- src/core/rules/layout/absolute-position.test.ts
- src/core/rules/layout/fixed-width-in-responsive-context.test.ts
- src/core/rules/layout/missing-responsive-behavior.test.ts
- src/core/rules/ai-readability/ambiguous-structure.test.ts
- src/core/rules/handoff-risk/hardcode-risk.test.ts
- src/core/rules/ai-readability/missing-layout-hint.test.ts
- src/core/rules/layout/deep-nesting.test.ts
- src/core/rules/ai-readability/index.ts
- src/core/rules/handoff-risk/prototype-link-in-design.test.ts
- src/core/rules/ai-readability/invisible-layer.test.ts
- src/core/rules/layout/index.ts
| @@ -0,0 +1,54 @@ | |||
| import { makeNode, makeFile, makeContext } from "../test-helpers.js"; | |||
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused import makeFile.
makeFile is imported but never used in any test case. This will likely trigger lint warnings.
🧹 Proposed fix
-import { makeNode, makeFile, makeContext } from "../test-helpers.js";
+import { makeNode, makeContext } from "../test-helpers.js";📝 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 { makeNode, makeFile, makeContext } from "../test-helpers.js"; | |
| import { makeNode, makeContext } from "../test-helpers.js"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/rules/behavior/prototype-link-in-design.test.ts` at line 1, Remove
the unused import by editing the import statement that currently reads "import {
makeNode, makeFile, makeContext } from "../test-helpers.js";" in
prototype-link-in-design.test.ts — drop "makeFile" so it becomes "import {
makeNode, makeContext } from \"../test-helpers.js\""; ensure there are no other
references to makeFile in the file before committing.
| @@ -0,0 +1,121 @@ | |||
| import { makeNode, makeFile, makeContext } from "../test-helpers.js"; | |||
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused import makeFile.
makeFile is imported but never used in this test file.
🧹 Proposed fix
-import { makeNode, makeFile, makeContext } from "../test-helpers.js";
+import { makeNode, makeContext } from "../test-helpers.js";📝 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 { makeNode, makeFile, makeContext } from "../test-helpers.js"; | |
| import { makeNode, makeContext } from "../test-helpers.js"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/rules/behavior/wrap-behavior-unknown.test.ts` at line 1, The import
list includes an unused symbol makeFile; remove makeFile from the import
statement (leaving makeNode and makeContext) in the top-of-file import that
currently reads "import { makeNode, makeFile, makeContext } from
\"../test-helpers.js\";" so the test file only imports used helpers and
eliminates the unused makeFile import.
Comment on lines
+167
to
+236
| // ============================================ | ||
| // fixed-size-in-auto-layout (merged: absorbs fixed-width-in-responsive-context) | ||
| // ============================================ | ||
|
|
||
| const fixedSizeInAutoLayoutDef: RuleDefinition = { | ||
| id: "fixed-size-in-auto-layout", | ||
| name: "Fixed Size in Auto Layout", | ||
| category: "structure", | ||
| why: "Fixed sizing inside Auto Layout contradicts the flexible layout intent", | ||
| impact: "AI generates a rigid element inside a flex container — the layout won't respond to content changes", | ||
| fix: "Use 'Hug' or 'Fill' for at least one axis. Both-axes FIXED → layout completely rigid; horizontal-only FIXED → width won't adapt to parent resize", | ||
| }; | ||
|
|
||
| const fixedSizeInAutoLayoutCheck: RuleCheckFn = (node, context) => { | ||
| if (!context.parent) return null; | ||
| if (!hasAutoLayout(context.parent)) return null; | ||
| if (!isContainerNode(node)) return null; | ||
| if (!node.absoluteBoundingBox) return null; | ||
|
|
||
| // Skip if it's intentionally a small fixed element (icon, avatar, etc.) | ||
| const { width, height } = node.absoluteBoundingBox; | ||
| if (width <= 48 && height <= 48) return null; | ||
|
|
||
| // Check both axes FIXED (stronger case) | ||
| const hFixed = | ||
| node.layoutSizingHorizontal === "FIXED" || node.layoutSizingHorizontal === undefined; | ||
| const vFixed = | ||
| node.layoutSizingVertical === "FIXED" || node.layoutSizingVertical === undefined; | ||
|
|
||
| if (hFixed && vFixed) { | ||
| // Skip if it has its own auto-layout | ||
| if (node.layoutMode && node.layoutMode !== "NONE") return null; | ||
|
|
||
| return { | ||
| ruleId: fixedSizeInAutoLayoutDef.id, | ||
| nodeId: node.id, | ||
| nodePath: context.path.join(" > "), | ||
| message: `Container "${node.name}" (${width}×${height}) uses fixed size on both axes inside Auto Layout. Consider HUG or FILL for at least one axis.`, | ||
| }; | ||
| } | ||
|
|
||
| // Check horizontal-only FIXED (lighter case, from fixed-width-in-responsive-context) | ||
| if (hFixed && !vFixed) { | ||
| // Use layoutSizingHorizontal if available (accurate) | ||
| if (node.layoutSizingHorizontal) { | ||
| if (node.layoutSizingHorizontal !== "FIXED") return null; | ||
| } else { | ||
| // Fallback: STRETCH means fill, skip | ||
| if (node.layoutAlign === "STRETCH") return null; | ||
| if (node.layoutAlign !== "INHERIT") return null; | ||
| } | ||
|
|
||
| // Excluded names (nav, header, etc.) | ||
| if (isExcludedName(node.name)) return null; | ||
|
|
||
| return { | ||
| ruleId: fixedSizeInAutoLayoutDef.id, | ||
| nodeId: node.id, | ||
| nodePath: context.path.join(" > "), | ||
| message: `"${node.name}" has fixed width inside Auto Layout`, | ||
| }; | ||
| } | ||
|
|
||
| return null; | ||
| }; | ||
|
|
||
| export const fixedSizeInAutoLayout = defineRule({ | ||
| definition: fixedSizeInAutoLayoutDef, | ||
| check: fixedSizeInAutoLayoutCheck, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent logic path in the horizontal-only FIXED check.
Lines 209-228: When hFixed && !vFixed, the logic has an inconsistency:
- Line 211 checks if
node.layoutSizingHorizontalis truthy - Line 212 returns
nullif it's NOT"FIXED"(correct) - But line 211's truthiness check already passed, so
layoutSizingHorizontalexists - Lines 214-216 in the
elsebranch will never execute because iflayoutSizingHorizontalisundefined, it would fail the truthiness check buthFixedwould betruedue to line 191-192's fallback
The else branch (lines 213-217) handles the case where layoutSizingHorizontal is undefined but hFixed is true due to the fallback on line 192. However, the current condition structure is confusing.
♻️ Clearer condition structure
// Check horizontal-only FIXED (lighter case, from fixed-width-in-responsive-context)
if (hFixed && !vFixed) {
- // Use layoutSizingHorizontal if available (accurate)
- if (node.layoutSizingHorizontal) {
- if (node.layoutSizingHorizontal !== "FIXED") return null;
- } else {
- // Fallback: STRETCH means fill, skip
- if (node.layoutAlign === "STRETCH") return null;
- if (node.layoutAlign !== "INHERIT") return null;
- }
+ // If layoutSizingHorizontal is explicitly set and not FIXED, skip
+ if (node.layoutSizingHorizontal !== undefined && node.layoutSizingHorizontal !== "FIXED") {
+ return null;
+ }
+ // Fallback for older Figma data: use layoutAlign heuristic
+ if (node.layoutSizingHorizontal === undefined) {
+ if (node.layoutAlign === "STRETCH") return null;
+ if (node.layoutAlign !== "INHERIT") return null;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/rules/structure/index.ts` around lines 167 - 236, The
horizontal-only branch in fixedSizeInAutoLayoutCheck has confusing/partially
unreachable checks: when hFixed is true we should first test whether
node.layoutSizingHorizontal is defined and, if so, require it to equal "FIXED";
otherwise (undefined) use the fallback checks on node.layoutAlign. Update the
hFixed && !vFixed block in fixedSizeInAutoLayoutCheck to: if
node.layoutSizingHorizontal !== undefined then return null unless it equals
"FIXED"; else (layoutSizingHorizontal undefined) perform the existing
layoutAlign checks (skip for "STRETCH", skip unless "INHERIT"), keeping the
excluded-name and return message logic unchanged.
| @@ -0,0 +1,140 @@ | |||
| import { makeNode, makeFile, makeContext } from "../test-helpers.js"; | |||
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused import makeFile.
makeFile is imported but never used in this test file.
🧹 Proposed fix
-import { makeNode, makeFile, makeContext } from "../test-helpers.js";
+import { makeNode, makeContext } from "../test-helpers.js";📝 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 { makeNode, makeFile, makeContext } from "../test-helpers.js"; | |
| import { makeNode, makeContext } from "../test-helpers.js"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/rules/structure/no-auto-layout.test.ts` at line 1, The import
statement in this test file includes an unused symbol: remove the unused import
makeFile from the import list (currently importing makeNode, makeFile,
makeContext) so only the used helpers (makeNode and makeContext) remain; update
the import line that references makeFile to exclude it.
| @@ -0,0 +1,110 @@ | |||
| import { makeNode, makeFile, makeContext } from "../test-helpers.js"; | |||
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unused import makeFile.
makeFile is imported but never used in this test file.
🧹 Proposed fix
-import { makeNode, makeFile, makeContext } from "../test-helpers.js";
+import { makeNode, makeContext } from "../test-helpers.js";📝 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 { makeNode, makeFile, makeContext } from "../test-helpers.js"; | |
| import { makeNode, makeContext } from "../test-helpers.js"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/rules/structure/unnecessary-node.test.ts` at line 1, The import
statement includes an unused symbol makeFile; update the import at the top that
currently reads "import { makeNode, makeFile, makeContext } from
\"../test-helpers.js\";" to remove makeFile so it only imports makeNode and
makeContext, ensuring tests still compile and no unused imports remain.
- REFERENCE.md: fix scores/severity to match rule-config.ts - unnecessary-node: -1 → -2 - variant-structure-mismatch: -5 → -4 - prototype-link-in-design: suggestion → missing-info - overflow/wrap-behavior-unknown: -5 risk → -3 missing-info - .coderabbit.yaml: fix pre_merge_checks schema (docstrings.mode: off) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/REFERENCE.md`:
- Around line 174-175: Add a blank line between the preceding paragraph
"**Mobile-first (focus on structure):**" and the fenced code block opener
"```json" so the markdown has an empty line before the code fence; update the
docs/REFERENCE.md content to insert that single blank line immediately before
the "```json" fence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90e8116e-785a-4d94-afa4-d0e378bd0aa0
📒 Files selected for processing (2)
.coderabbit.yamldocs/REFERENCE.md
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
Replaced by new PR with clean rebase on main (after #81 merge). |
2 tasks
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.
Summary
layout+ai-readabilitymerged intostructure,handoff-riskrenamed tobehavior(6 categories to 5, 32 rules to 29)Closes #79
Test plan
pnpm test:runpasses (590 tests, 0 failures)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation
Refactor