Conversation
…rty-unused, prototype-link-in-design (#74) All 3 rules were registered but always returned null (stub). Now they have real detection logic: 1. overflow-hidden-abuse: flags non-auto-layout containers with clipsContent that have children — likely hiding overflow instead of fixing layout. Skips auto-layout frames, small elements (icons). 2. component-property-unused: checks INSTANCE nodes against their master component's propertyDefinitions. Flags instances that don't customize any available properties. 3. prototype-link-in-design (REDEFINED): was "flag if interactions exist" (useless). Now: "flag interactive-looking elements that LACK prototype interactions". Detects by name patterns (button, link, tab, CTA, etc.) and component state variants (hover, pressed, active). Also adds `interactions` field to AnalysisNode schema and collects it in figma-transformer. Tests: 648 → 664 (+16), todo: 6 → 3. Closes #74 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional Changes
Sequence Diagram(s)sequenceDiagram
participant Adapter as Figma Adapter (transformNode)
participant Schema as AnalysisNode Schema
participant Engine as Rule Engine
participant Rule as prototype-link-in-design
Adapter->>Schema: transformNode(node) — map base fields\nif node.interactions is Array and non-empty -> set interactions (rgba(0,128,0,0.5))
Schema->>Engine: emit AnalysisNode (may include interactions)
Engine->>Rule: prototype-link-in-design.check(node, ctx)
alt node looks interactive && no interactions
Rule->>Engine: return finding (prototype-link-in-design)
else interactions present or not interactive
Rule->>Engine: return null
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…rules Both rules measured "design hygiene" rather than "AI implementation difficulty" — the core question this tool answers. - overflow-hidden-abuse: AI implements overflow:hidden trivially - component-property-unused: default property values are often intentional Removed from: rule definitions, rule-config, RuleId type, scoring constants (Layout 11→10, Component 6→5), tests, README, REFERENCE.md. 37 rules remain (was 39). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/rules/component/index.ts`:
- Around line 371-383: The check currently treats any non-empty
node.componentProperties as proof that the instance customizes properties and
returns the componentPropertyUnusedDef finding can be skipped; instead, iterate
the keys of node.componentProperties and verify at least one key matches a
master-defined property from masterDef.componentPropertyDefinitions (use
definedProps or Object.keys(masterDef.componentPropertyDefinitions) for the
reference set). If no keys intersect, return the same finding object
(componentPropertyUnusedDef) as when componentProperties is missing, otherwise
return null; add/test a regression case where componentProperties exists but
none of its keys are present in masterDef.componentPropertyDefinitions to ensure
the new matching logic works.
In `@src/core/rules/handoff-risk/index.ts`:
- Around line 167-173: The rule currently flags FRAME nodes by name patterns
(INTERACTIVE_NAME_PATTERNS) which causes noisy hits on container frames; update
the logic in the handoff-risk check(s) so that when encountering a FRAME you
only consider it interactive if it is leaf-like: ensure the FRAME has no child
or descendant nodes that themselves are interactive or already host prototype
interactions (i.e., bail out if any descendant has interactions or matches
interactive types), otherwise apply the name-based match; change the same
behavior in the related check(s) that reference INTERACTIVE_NAME_PATTERNS (the
other block noted in lines 205-219) so container frames like "Main Nav" or
"Button Group" are skipped when their interactive children exist.
In `@src/core/rules/layout/index.ts`:
- Around line 403-427: The rule function overflowHiddenAbuseCheck flags
non-auto-layout containers with clipsContent=true even when they are deliberate
scroll containers; update overflowHiddenAbuseCheck to first check
node.overflowDirection (or the AnalysisNode property representing scrolling) and
bail out (return null) when overflowDirection indicates scrolling/scrollable
before emitting the overflowHiddenAbuseDef finding; keep existing guards
(isContainerNode, hasAutoLayout, small bounding boxes) intact. Also add a
regression test that constructs a scrolling frame (clipsContent=true with
overflowDirection set to a scrolling value) and assert that
overflowHiddenAbuseCheck does not return a finding for that node.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 36be9c67-7c36-447e-8f85-0b60f3775930
📒 Files selected for processing (8)
src/core/adapters/figma-transformer.tssrc/core/contracts/figma-node.tssrc/core/rules/component/component-property-unused.test.tssrc/core/rules/component/index.tssrc/core/rules/handoff-risk/index.tssrc/core/rules/handoff-risk/prototype-link-in-design.test.tssrc/core/rules/layout/index.tssrc/core/rules/layout/overflow-hidden-abuse.test.ts
src/core/rules/component/index.ts
Outdated
Comment on lines
+371
to
+383
| const definedProps = Object.keys(masterDef.componentPropertyDefinitions); | ||
| if (definedProps.length === 0) return null; | ||
|
|
||
| // This would require checking if properties are actually bound | ||
| // Simplified for now | ||
| // Check if the instance has any property overrides | ||
| const instanceProps = node.componentProperties; | ||
| if (!instanceProps || Object.keys(instanceProps).length === 0) { | ||
| return { | ||
| ruleId: componentPropertyUnusedDef.id, | ||
| nodeId: node.id, | ||
| nodePath: context.path.join(" > "), | ||
| message: `Instance "${node.name}" does not customize any of ${definedProps.length} available component properties`, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
This never verifies that a master-defined property is actually used.
After definedProps is computed, any non-empty node.componentProperties object suppresses the finding. That means the rule can return null even when none of masterDef.componentPropertyDefinitions are actually being used, which misses the stated goal of the check.
Please compare the instance against the master's defined properties before returning null here, and add a regression test where componentProperties exists but no exposed property is actually used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/rules/component/index.ts` around lines 371 - 383, The check
currently treats any non-empty node.componentProperties as proof that the
instance customizes properties and returns the componentPropertyUnusedDef
finding can be skipped; instead, iterate the keys of node.componentProperties
and verify at least one key matches a master-defined property from
masterDef.componentPropertyDefinitions (use definedProps or
Object.keys(masterDef.componentPropertyDefinitions) for the reference set). If
no keys intersect, return the same finding object (componentPropertyUnusedDef)
as when componentProperties is missing, otherwise return null; add/test a
regression case where componentProperties exists but none of its keys are
present in masterDef.componentPropertyDefinitions to ensure the new matching
logic works.
src/core/rules/layout/index.ts
Outdated
Comment on lines
403
to
427
| const overflowHiddenAbuseCheck: RuleCheckFn = (node, context) => { | ||
| if (!isContainerNode(node)) return null; | ||
| if (!node.clipsContent) return null; | ||
|
|
||
| // Clip content on auto-layout frames is normal (scrollable containers, cards) | ||
| if (hasAutoLayout(node)) return null; | ||
|
|
||
| // Clip content on small elements (icons, avatars) is expected | ||
| if (node.absoluteBoundingBox) { | ||
| const { width, height } = node.absoluteBoundingBox; | ||
| if (width <= 48 && height <= 48) return null; | ||
| } | ||
|
|
||
| // Non-auto-layout container with clipsContent = suspicious | ||
| // It may be hiding children that overflow due to missing auto-layout | ||
| if (node.children && node.children.length > 0) { | ||
| return { | ||
| ruleId: overflowHiddenAbuseDef.id, | ||
| nodeId: node.id, | ||
| nodePath: context.path.join(" > "), | ||
| message: `"${node.name}" uses clip content without Auto Layout — may be hiding overflow instead of fixing layout`, | ||
| }; | ||
| } | ||
|
|
||
| return null; |
Contributor
There was a problem hiding this comment.
Skip explicit scroll containers in overflow-hidden-abuse.
overflowDirection is already available on AnalysisNode, but this check ignores it. A non-auto-layout container with clipsContent: true and scrolling enabled is an intentional viewport, so the current logic will flag legitimate carousels and scroll regions as abuse.
🔧 Suggested guard
const overflowHiddenAbuseCheck: RuleCheckFn = (node, context) => {
if (!isContainerNode(node)) return null;
if (!node.clipsContent) return null;
+ if (node.overflowDirection && node.overflowDirection !== "NONE") return null;
// Clip content on auto-layout frames is normal (scrollable containers, cards)
if (hasAutoLayout(node)) return null;Please add a regression test for a scrolling frame as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/rules/layout/index.ts` around lines 403 - 427, The rule function
overflowHiddenAbuseCheck flags non-auto-layout containers with clipsContent=true
even when they are deliberate scroll containers; update overflowHiddenAbuseCheck
to first check node.overflowDirection (or the AnalysisNode property representing
scrolling) and bail out (return null) when overflowDirection indicates
scrolling/scrollable before emitting the overflowHiddenAbuseDef finding; keep
existing guards (isContainerNode, hasAutoLayout, small bounding boxes) intact.
Also add a regression test that constructs a scrolling frame (clipsContent=true
with overflowDirection set to a scrolling value) and assert that
overflowHiddenAbuseCheck does not return a finding for that node.
Single-use component doesn't affect AI implementation difficulty — AI implements what it sees regardless of component reuse count. This is design hygiene, not implementation readiness. 37 → 36 rules. Component: 5 → 4. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Dev status is a process/workflow metadata, not an AI implementation difficulty signal. It was already disabled by default. 36 → 35 rules. Handoff Risk: 5 → 4. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… rationale Every rule's why/impact/fix now answers "how does this affect AI's ability to implement the design?" instead of generic design quality language. Examples: - raw-color: "Raw hex values repeated across nodes increase the chance of AI mismatching colors" (was: "not connected to design system") - default-name: "gives AI no semantic context to choose HTML tags" (was: "provide no context about purpose") - deep-nesting: "consumes AI context exponentially" (was: "hard to understand for developers") Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…-direction rules - image-no-placeholder: placeholder is loading/error UX, not AI implementation accuracy. Image itself renders pixel-perfect regardless of placeholder. - inconsistent-sibling-layout-direction: mixed directions among siblings is normal in real designs (e.g., card grids). High false positive rate, AI implements each sibling independently anyway. 35 → 33 rules. Layout: 10→9, Handoff Risk: 4→3. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/core/contracts/rule.ts (2)
92-102:⚠️ Potential issue | 🟡 MinorComment count is incorrect — Layout has 9 rules, not 11.
The comment on line 93 states
// Layout (11)but only 9 rule IDs are listed in this section (no-auto-layoutthroughdeep-nesting). This appears to be a stale comment that wasn't updated when rules were removed.📝 Proposed fix
- // Layout (11) + // Layout (9)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/contracts/rule.ts` around lines 92 - 102, The inline comment for the RuleId union is out of date: the comment "// Layout (11)" above the layout rule list in the RuleId type is wrong since only nine layout rule IDs (no-auto-layout through deep-nesting) are present; update that comment to reflect the correct count (e.g., "// Layout (9)") or remove the count entirely to avoid future drift. Locate the RuleId type declaration and change the comment text accordingly so it matches the actual number of entries.
102-102:⚠️ Potential issue | 🟡 Minor
deep-nestingis listed under Layout but categorized ashandoff-risk.The
deep-nestingrule ID is grouped in the "Layout" section of theRuleIdtype insrc/core/contracts/rule.ts, but its definition insrc/core/rules/layout/index.tsspecifiescategory: "handoff-risk". This mismatch between organizational grouping and actual category could cause confusion. Consider either movingdeep-nestingto align with other handoff-risk rules or updating its category to"layout"if Layout is the intended classification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/contracts/rule.ts` at line 102, The RuleId union in RuleId (the "Layout" group containing the "deep-nesting" literal) does not match the rule's declared category in the rule definition (the deep-nesting entry in src/core/rules/layout/index.ts sets category: "handoff-risk"); update them to be consistent by either moving "deep-nesting" out of the Layout group in the RuleId type to the appropriate handoff-risk group or change the deep-nesting rule object's category to "layout" so the RuleId grouping and the rule definition agree (search for the RuleId type and the deep-nesting rule export to make the edit).docs/REFERENCE.md (1)
95-107:⚠️ Potential issue | 🟡 MinorAdd missing
deep-nestingrule to Layout table in REFERENCE.md.The Layout section header states "Layout (9 rules)" but only 8 rules are listed in the table. The
deep-nestingrule (score: -4, severity: risk) is missing and should be added to the table.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/REFERENCE.md` around lines 95 - 107, Add the missing Layout rule entry for `deep-nesting` in the Layout table in docs/REFERENCE.md: insert a table row for the rule ID `deep-nesting` with Default Score `-4` and Default Severity `risk` so the table actually contains 9 rules matching the "Layout (9 rules)" header; ensure the row follows the same pipe-delimited format as the other rows.
♻️ Duplicate comments (1)
src/core/rules/handoff-risk/index.ts (1)
156-171:⚠️ Potential issue | 🟠 MajorPotential false positives: wrapper frames and descendants with interactions.
Container frames like
"Main Nav"or"Button Group"that match name patterns will be flagged even when their child/descendant elements correctly own the prototype interactions. This will generate noise on composed controls.Consider adding a check to bail out when any descendant has interactions defined, or restrict to leaf-like frames.
🔧 Suggested approach
const prototypeLinkInDesignCheck: RuleCheckFn = (node, context) => { // Only check components and instances (interactive elements are typically components) if (node.type !== "COMPONENT" && node.type !== "INSTANCE" && node.type !== "FRAME") return null; if (!looksInteractive(node)) return null; // If interactions exist, the element has prototype behavior defined if (node.interactions && node.interactions.length > 0) return null; + // Skip container frames if any child has interactions (interaction lives on descendant) + if (node.type === "FRAME" && node.children) { + const hasDescendantInteraction = (n: AnalysisNode): boolean => { + if (n.interactions && n.interactions.length > 0) return true; + return n.children?.some(hasDescendantInteraction) ?? false; + }; + if (node.children.some(hasDescendantInteraction)) return null; + } + return { ruleId: prototypeLinkInDesignDef.id,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/rules/handoff-risk/index.ts` around lines 156 - 171, The prototypeLinkInDesignCheck currently flags any FRAME/COMPONENT/INSTANCE that looksInteractive even if its descendants own the interactions; modify prototypeLinkInDesignCheck to bail out when any descendant has interactions (e.g., use node.findAll or equivalent to check for descendant nodes with interactions.length > 0) or alternatively restrict the check to leaf-like nodes (no children or only non-interactive children) before calling looksInteractive; update the logic so that before returning the violation you scan descendants and return null if any descendant has interactions, referencing prototypeLinkInDesignCheck, looksInteractive, and prototypeLinkInDesignDef in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/engine/rule-engine.test.ts`:
- Around line 265-267: When overriding the "no-auto-layout" entry on
RULE_CONFIGS, guard against it being undefined by first reading the entry into a
variable and checking its existence (or use a non-null assertion) before
spreading; e.g., get const noAuto = disabledConfigs["no-auto-layout"]; if
(noAuto) disabledConfigs["no-auto-layout"] = { ...noAuto, enabled: false }; else
disabledConfigs["no-auto-layout"] = { enabled: false } as typeof noAuto; this
avoids a potential undefined spread and satisfies strict
noUncheckedIndexedAccess while keeping the call to analyzeFile(file, { configs:
disabledConfigs }) unchanged.
In `@src/core/engine/scoring.test.ts`:
- Line 278: The component rule list in the stress test is missing one expected
rule from TOTAL_RULES_PER_CATEGORY; update the component array in the failing
test in scoring.test.ts to include "missing-component-description" (so
component:
["missing-component","detached-instance","variant-not-used","missing-component-description"])
or, if the omission is intentional, add a clarifying comment next to the
component array explaining why that rule is excluded; reference the test's
component array and TOTAL_RULES_PER_CATEGORY when making the change.
---
Outside diff comments:
In `@docs/REFERENCE.md`:
- Around line 95-107: Add the missing Layout rule entry for `deep-nesting` in
the Layout table in docs/REFERENCE.md: insert a table row for the rule ID
`deep-nesting` with Default Score `-4` and Default Severity `risk` so the table
actually contains 9 rules matching the "Layout (9 rules)" header; ensure the row
follows the same pipe-delimited format as the other rows.
In `@src/core/contracts/rule.ts`:
- Around line 92-102: The inline comment for the RuleId union is out of date:
the comment "// Layout (11)" above the layout rule list in the RuleId type is
wrong since only nine layout rule IDs (no-auto-layout through deep-nesting) are
present; update that comment to reflect the correct count (e.g., "// Layout
(9)") or remove the count entirely to avoid future drift. Locate the RuleId type
declaration and change the comment text accordingly so it matches the actual
number of entries.
- Line 102: The RuleId union in RuleId (the "Layout" group containing the
"deep-nesting" literal) does not match the rule's declared category in the rule
definition (the deep-nesting entry in src/core/rules/layout/index.ts sets
category: "handoff-risk"); update them to be consistent by either moving
"deep-nesting" out of the Layout group in the RuleId type to the appropriate
handoff-risk group or change the deep-nesting rule object's category to "layout"
so the RuleId grouping and the rule definition agree (search for the RuleId type
and the deep-nesting rule export to make the edit).
---
Duplicate comments:
In `@src/core/rules/handoff-risk/index.ts`:
- Around line 156-171: The prototypeLinkInDesignCheck currently flags any
FRAME/COMPONENT/INSTANCE that looksInteractive even if its descendants own the
interactions; modify prototypeLinkInDesignCheck to bail out when any descendant
has interactions (e.g., use node.findAll or equivalent to check for descendant
nodes with interactions.length > 0) or alternatively restrict the check to
leaf-like nodes (no children or only non-interactive children) before calling
looksInteractive; update the logic so that before returning the violation you
scan descendants and return null if any descendant has interactions, referencing
prototypeLinkInDesignCheck, looksInteractive, and prototypeLinkInDesignDef in
your change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 682a78b8-f710-463f-97ab-be36ebc33747
📒 Files selected for processing (20)
CLAUDE.mdREADME.mddocs/REFERENCE.mdsrc/core/contracts/rule.tssrc/core/engine/rule-engine.test.tssrc/core/engine/scoring.test.tssrc/core/engine/scoring.tssrc/core/rules/component/component-property-unused.test.tssrc/core/rules/component/index.tssrc/core/rules/component/single-use-component.test.tssrc/core/rules/handoff-risk/image-no-placeholder.test.tssrc/core/rules/handoff-risk/index.tssrc/core/rules/handoff-risk/no-dev-status.test.tssrc/core/rules/layout/inconsistent-sibling-layout-direction.test.tssrc/core/rules/layout/index.tssrc/core/rules/layout/overflow-hidden-abuse.test.tssrc/core/rules/naming/index.tssrc/core/rules/rule-config.tssrc/core/rules/token/index.tssrc/core/rules/token/raw-color.test.ts
💤 Files with no reviewable changes (7)
- src/core/rules/component/single-use-component.test.ts
- src/core/rules/handoff-risk/no-dev-status.test.ts
- src/core/rules/handoff-risk/image-no-placeholder.test.ts
- src/core/rules/layout/inconsistent-sibling-layout-direction.test.ts
- src/core/rules/rule-config.ts
- src/core/rules/component/component-property-unused.test.ts
- src/core/rules/layout/overflow-hidden-abuse.test.ts
- prototype-link-in-design: skip container FRAME nodes whose descendants already have interactions (fixes false positives on "Button Group", "Main Nav" wrappers). Added hasDescendantInteractions() helper + test. - rule-engine.test.ts: guard noUncheckedIndexedAccess with expect+non-null assertion when overriding rule config. - scoring.test.ts: add missing-component-description to component rules in stress test for completeness. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/rules/handoff-risk/index.ts`:
- Around line 141-149: The code casts p["variantOptions"] to string[] and calls
options.some(...) which can throw if the value is not an array of strings;
update the check in the loop that handles VARIANT properties (the block
referencing propValues, p["type"], p["variantOptions"], and
STATE_VARIANT_PATTERNS) to first verify variantOptions is an array
(Array.isArray) and that its entries are strings (or coerce/filter non-strings)
before calling .some; if the check fails, treat it as no matching options (i.e.,
continue without throwing) so malformed data won't cause runtime errors.
In `@src/core/rules/handoff-risk/prototype-link-in-design.test.ts`:
- Around line 18-23: Tests for prototypeLinkInDesign.check are missing coverage
for INSTANCE nodes; add an additional loop or expand the existing loop to create
nodes with type "INSTANCE" (e.g., makeNode({ type: "INSTANCE", name })) using
the same interactive name cases and assert
expect(prototypeLinkInDesign.check(node, makeContext())).not.toBeNull(); ensure
you include the same names array and keep the existing FRAME/COMPONENT checks so
all three types (COMPONENT, INSTANCE, FRAME) are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 23305ae2-ea0b-4f3e-a1ad-9958827350ce
📒 Files selected for processing (4)
src/core/engine/rule-engine.test.tssrc/core/engine/scoring.test.tssrc/core/rules/handoff-risk/index.tssrc/core/rules/handoff-risk/prototype-link-in-design.test.ts
- variantOptions: add Array.isArray + typeof string guard before .some() - prototype-link-in-design test: add INSTANCE type coverage alongside FRAME Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…76) Both rules were stubs (return null). Now implemented: 1. multiple-fill-colors: compares solid fill colors across siblings using Euclidean RGB distance. Flags near-duplicate colors within tolerance (default: 10). Skips tokenized fills (style reference). 2. variant-not-used: checks if INSTANCE nodes override any VARIANT properties from the master component. Flags instances using default variant when multiple options are available. All it.todo converted to real tests. 0 todo remaining. Closes #75, closes #76 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/core/rules/handoff-risk/index.ts (1)
156-176:⚠️ Potential issue | 🟠 MajorTreat interactive descendants as a bailout for wrapper frames.
A frame like
"Button Group"is still flagged when its children are the real interactive targets but are merely missing their own prototype links, because this helper only looks for descendantinteractions. That adds a noisy container-level finding on top of the leaf findings and inflates penalties on composed controls.Suggested fix
-/** Check if any descendant has interactions defined */ -function hasDescendantInteractions(node: AnalysisNode): boolean { - if (node.interactions && node.interactions.length > 0) return true; - for (const child of node.children ?? []) { - if (hasDescendantInteractions(child)) return true; - } - return false; -} +/** Check if any descendant already represents the interactive target */ +function hasInteractiveDescendant(node: AnalysisNode): boolean { + for (const child of node.children ?? []) { + if (child.interactions?.length > 0 || looksInteractive(child)) return true; + if (hasInteractiveDescendant(child)) return true; + } + return false; +} @@ - if (node.type === "FRAME" && node.children && node.children.length > 0) { - if (hasDescendantInteractions(node)) return null; + if (node.type === "FRAME" && node.children?.length > 0) { + if (hasInteractiveDescendant(node)) return null; }Based on learnings, "Token consumption is a first-class metric in design analysis and penalty scoring."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/rules/handoff-risk/index.ts` around lines 156 - 176, The current bailout only checks for descendant.interactions, which misses interactive descendants (e.g., buttons) and causes noisy wrapper-frame findings; update hasDescendantInteractions to return true if any descendant is interactive by checking looksInteractive(child) OR (child.interactions && child.interactions.length>0) recursively (preserve the same signature), then keep using this function from prototypeLinkInDesignCheck so FRAME wrappers with interactive descendants are skipped; reference functions: hasDescendantInteractions, prototypeLinkInDesignCheck, and looksInteractive.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/rules/handoff-risk/prototype-link-in-design.test.ts`:
- Around line 51-87: Add two edge-case tests to prototype-link-in-design.test.ts
that call prototypeLinkInDesign.check with nodes created by makeNode/makeContext
whose componentPropertyDefinitions include a variantOptions value that's not an
array (e.g., null or an object) and another where variantOptions is an array
containing non-string entries (e.g., [123, null]). In each test assert that
calling prototypeLinkInDesign.check(...) does not throw and that it returns null
when no valid string variant names are present; reference the
componentPropertyDefinitions.variantOptions path and the
prototypeLinkInDesign.check function to locate where behavior should be
exercised.
---
Duplicate comments:
In `@src/core/rules/handoff-risk/index.ts`:
- Around line 156-176: The current bailout only checks for
descendant.interactions, which misses interactive descendants (e.g., buttons)
and causes noisy wrapper-frame findings; update hasDescendantInteractions to
return true if any descendant is interactive by checking looksInteractive(child)
OR (child.interactions && child.interactions.length>0) recursively (preserve the
same signature), then keep using this function from prototypeLinkInDesignCheck
so FRAME wrappers with interactive descendants are skipped; reference functions:
hasDescendantInteractions, prototypeLinkInDesignCheck, and looksInteractive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c85f75a8-816a-4630-8687-48a0b5437d67
📒 Files selected for processing (2)
src/core/rules/handoff-risk/index.tssrc/core/rules/handoff-risk/prototype-link-in-design.test.ts
Comment on lines
+51
to
+87
| it("flags component with state variants but no interactions", () => { | ||
| const node = makeNode({ | ||
| type: "COMPONENT", | ||
| name: "Chip", | ||
| componentPropertyDefinitions: { | ||
| State: { type: "VARIANT", variantOptions: ["default", "hover", "pressed"] }, | ||
| }, | ||
| }); | ||
| const result = prototypeLinkInDesign.check(node, makeContext()); | ||
| expect(result).not.toBeNull(); | ||
| expect(result!.message).toContain("looks interactive"); | ||
| }); | ||
|
|
||
| it("returns null for container frame whose children have interactions", () => { | ||
| const child = makeNode({ | ||
| id: "c:1", | ||
| type: "COMPONENT", | ||
| name: "Button", | ||
| interactions: [{ trigger: { type: "ON_CLICK" }, actions: [{ type: "NAVIGATE" }] }], | ||
| }); | ||
| const container = makeNode({ | ||
| type: "FRAME", | ||
| name: "Button Group", | ||
| children: [child], | ||
| }); | ||
| expect(prototypeLinkInDesign.check(container, makeContext())).toBeNull(); | ||
| }); | ||
|
|
||
| it("returns null for component with non-state variants", () => { | ||
| const node = makeNode({ | ||
| type: "COMPONENT", | ||
| name: "Icon", | ||
| componentPropertyDefinitions: { | ||
| Size: { type: "VARIANT", variantOptions: ["small", "medium", "large"] }, | ||
| }, | ||
| }); | ||
| expect(prototypeLinkInDesign.check(node, makeContext())).toBeNull(); |
Contributor
There was a problem hiding this comment.
Cover the malformed variantOptions path.
These tests only exercise string-array variant options, so the new Array.isArray / typeof opt === "string" guard can regress silently. Please add one case where variantOptions is not an array and another with non-string entries, and assert check(...) does not throw (and returns null for the all-invalid case).
As per coding guidelines, "Verify edge cases for noUncheckedIndexedAccess (undefined checks)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/rules/handoff-risk/prototype-link-in-design.test.ts` around lines 51
- 87, Add two edge-case tests to prototype-link-in-design.test.ts that call
prototypeLinkInDesign.check with nodes created by makeNode/makeContext whose
componentPropertyDefinitions include a variantOptions value that's not an array
(e.g., null or an object) and another where variantOptions is an array
containing non-string entries (e.g., [123, null]). In each test assert that
calling prototypeLinkInDesign.check(...) does not throw and that it returns null
when no valid string variant names are present; reference the
componentPropertyDefinitions.variantOptions path and the
prototypeLinkInDesign.check function to locate where behavior should be
exercised.
Using the default variant in a design is normal — hover/pressed states exist in the component definition, not on the canvas. Flagging "default variant used" penalizes correct Figma usage. 33 → 32 rules. Component: 4 → 3. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/rules/component/variant-not-used.test.ts`:
- Around line 49-85: Add a unit test covering the edge case where a variant
property has exactly one option so the rule's filter that skips single-option
variants is exercised; specifically, create an AnalysisFile with a
componentDefinition (id "comp:3") whose componentPropertyDefinitions contains a
property like Theme with type "VARIANT" and variantOptions ["light"], then
create an INSTANCE node with componentId "comp:3" and assert
variantNotUsed.check(node, makeContext({ file })) returns null; this will target
the logic in index.ts that filters out single-option VARIANT properties.
In `@src/core/rules/token/index.ts`:
- Around line 317-343: multipleFillColorsCheck currently flags both directions
for a near-duplicate pair (nodeA→nodeB and nodeB→nodeA), which can double-count
the same color pair; update the implementation by adding a concise comment above
the multipleFillColorsCheck function (referencing multipleFillColorsCheck,
multipleFillColorsDef, and extractSolidColor) that documents this bidirectional
flagging behavior and the intended rationale (or note that it is intentional to
highlight both affected nodes), or alternatively implement a deterministic
single-direction guard (e.g. compare node.id and sibling.id or use a seen-pairs
set) if you want to avoid duplicate findings—include which approach you chose in
the comment.
In `@src/core/rules/token/multiple-fill-colors.test.ts`:
- Around line 58-65: Add a test that passes an explicit tolerance option to
multipleFillColors.check to exercise the override path instead of relying on the
default in rule-config.ts: create two nodes (nodeA, nodeB) with fills whose
color distance is ~5, then call multipleFillColors.check(nodeA, makeContext({
siblings }), { tolerance: 3 }) and assert it returns null, and call it again
with { tolerance: 10 } and assert it returns non-null; this verifies the custom
tolerance option is honored by the check implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73813724-ebba-4a97-af69-000efb5331c8
📒 Files selected for processing (4)
src/core/rules/component/index.tssrc/core/rules/component/variant-not-used.test.tssrc/core/rules/token/index.tssrc/core/rules/token/multiple-fill-colors.test.ts
Comment on lines
+49
to
+85
| it("returns null when instance overrides a variant property", () => { | ||
| const file = makeFileWithVariantComponent(); | ||
| const node = makeNode({ | ||
| type: "INSTANCE", | ||
| name: "MyButton", | ||
| componentId: "comp:1", | ||
| componentProperties: { State: { value: "hover" } }, | ||
| }); | ||
| expect(variantNotUsed.check(node, makeContext({ file }))).toBeNull(); | ||
| }); | ||
|
|
||
| it("returns null for non-INSTANCE nodes", () => { | ||
| const node = makeNode({ type: "FRAME" }); | ||
| expect(variantNotUsed.check(node, makeContext())).toBeNull(); | ||
| }); | ||
|
|
||
| it("returns null when component has no variant properties", () => { | ||
| const file: AnalysisFile = { | ||
| ...makeFile(), | ||
| componentDefinitions: { | ||
| "comp:2": makeNode({ | ||
| id: "comp:2", | ||
| type: "COMPONENT", | ||
| componentPropertyDefinitions: { | ||
| label: { type: "TEXT" }, | ||
| }, | ||
| }), | ||
| }, | ||
| }; | ||
| const node = makeNode({ type: "INSTANCE", componentId: "comp:2" }); | ||
| expect(variantNotUsed.check(node, makeContext({ file }))).toBeNull(); | ||
| }); | ||
|
|
||
| it("returns null when no componentDefinitions in file", () => { | ||
| const node = makeNode({ type: "INSTANCE", componentId: "comp:1" }); | ||
| expect(variantNotUsed.check(node, makeContext())).toBeNull(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM with minor coverage suggestion.
Good negative case coverage. Consider adding an edge case test for a variant property with exactly one option (which the rule's filter on Line 343 of index.ts intentionally skips):
📝 Optional: Additional edge case test
it("returns null when variant has only one option", () => {
const file: AnalysisFile = {
...makeFile(),
componentDefinitions: {
"comp:3": makeNode({
id: "comp:3",
type: "COMPONENT",
componentPropertyDefinitions: {
Theme: { type: "VARIANT", variantOptions: ["light"] }, // single option
},
}),
},
};
const node = makeNode({ type: "INSTANCE", componentId: "comp:3" });
expect(variantNotUsed.check(node, makeContext({ file }))).toBeNull();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/rules/component/variant-not-used.test.ts` around lines 49 - 85, Add
a unit test covering the edge case where a variant property has exactly one
option so the rule's filter that skips single-option variants is exercised;
specifically, create an AnalysisFile with a componentDefinition (id "comp:3")
whose componentPropertyDefinitions contains a property like Theme with type
"VARIANT" and variantOptions ["light"], then create an INSTANCE node with
componentId "comp:3" and assert variantNotUsed.check(node, makeContext({ file
})) returns null; this will target the logic in index.ts that filters out
single-option VARIANT properties.
…ge, tolerance test - prototype-link-in-design: add tests for malformed variantOptions (non-array, non-string entries) to exercise defensive guards - multiple-fill-colors: add bidirectional flagging comment + custom tolerance option test - variant-not-used review comments: N/A (rule deleted) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Was always returning null. Now checks for opacity < 1 without variable binding. Also adds opacity field to AnalysisNode and collects it in figma-transformer. 0 stub rules remaining — all 32 rules have real detection logic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 25, 2026
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
Rule audit for v1: implement stub rules, redefine or delete rules that don't serve the core question "Can AI implement this pixel-perfectly?"
Changes
Implemented: 2 stub rules
prototype-link-in-design(handoff-risk, -2): Redefined from "flag if interactions exist" → "flag interactive-looking elements that LACK prototype interactions". Detects by name patterns + state variants. Skips container FRAMEs with interactive descendants. Addedinteractionsfield to AnalysisNode + figma-transformer.multiple-fill-colors(token, -3): Compares solid fill colors across siblings using Euclidean RGB distance. Flags near-duplicates within tolerance. Skips tokenized fills.Deleted: 8 rules that don't affect AI implementation difficulty
overflow-hidden-abuseoverflow: hiddentriviallycomponent-property-unusedsingle-use-componentno-dev-statusimage-no-placeholderinconsistent-sibling-layout-directionvariant-not-usedUpdated: all rule descriptions
Every rule's
why/impact/fixnow explains its effect on AI implementation.Rule count: 39 → 32
Test plan
pnpm lint— passespnpm test:run— 632 tests passpnpm build— succeedsCloses #74, closes #75, closes #76
🤖 Generated with Claude Code