feat: interaction category — missing-interaction-state + missing-prototype (#151)#158
Merged
feat: interaction category — missing-interaction-state + missing-prototype (#151)#158
Conversation
) New category: interaction — detects missing state variants on interactive components (buttons, links, tabs, inputs, toggles). Rule: missing-interaction-state - subTypes: hover, disabled, active, focus - Detects via: variant properties, component sibling names, prototype interactions - Deduplicates per componentId + subType - Score: -3, severity: missing-info Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Detects interactive elements (links, tabs, dropdowns) without ON_CLICK prototype interactions. Rule: missing-prototype - subTypes: navigation, tab, dropdown - Checks: ON_CLICK interaction presence - Score: -3, severity: missing-info Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds a new "interaction" category and two rules— Changes
Sequence Diagram(s)sequenceDiagram
participant Analyzer
participant RuleEngine
participant FileStore
participant AnalysisState
participant Reporter
Analyzer->>RuleEngine: run rules for node
RuleEngine->>FileStore: fetch componentDefinitions (node.componentId) if needed
RuleEngine->>AnalysisState: dedupe check (componentId|nodeId + subtype)
alt missing variant/prototype detected and not deduped
RuleEngine->>Reporter: emit violation (ruleId, subType, nodeId, nodePath, message)
else already reported or not applicable
RuleEngine->>Analyzer: no-op / skip
end
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 docstrings
🧪 Generate unit tests (beta)
Comment |
This was referenced Mar 29, 2026
…iblings Bug: checked ALL components in file for state pattern match — any hover variant anywhere would exempt all interactive components. Replaced with hasStateInComponentMaster that checks the fetched component master's variant properties instead. Also fixed duplicate regex in focus state pattern. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Buttons can be navigation, modal trigger, or state change — separate subType "button" instead of lumping into "navigation". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tests for missing-interaction-state and missing-prototype covering: - Interactive name pattern matching - Variant property detection + component master fallback - ON_HOVER/ON_CLICK interaction exemption - Deduplication per componentId + subType - FRAME node handling (detached instances) - Non-interactive name skip - Input/toggle skip for prototype rule Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
hover/disabled/active/focus — exact CSS names. No Figma aliases like "selected", "current", "hovered". Mismatches will be handled via guide messages (#155) telling designers to use CSS-standard names. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…hlighted) active pattern now matches: active (CSS), pressed (Material), selected (common), highlighted (UIKit). focus also accepts "focused" variant. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Prototype (ON_HOVER) defines behavior, variant defines visual state — different concerns. missing-interaction-state only checks variants. missing-prototype handles prototype presence separately. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/engine/scoring.test.ts (2)
224-241:⚠️ Potential issue | 🟡 MinorLGTM!
The test correctly updates the weighted average calculation to account for 6 categories. The test name on line 224 says "all 5 categories" but the implementation correctly uses 6 — consider updating the test name for accuracy.
📝 Proposed fix for test name
- it("overall score is weighted average of all 5 categories", () => { + it("overall score is weighted average of all 6 categories", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/engine/scoring.test.ts` around lines 224 - 241, The test name string "overall score is weighted average of all 5 categories" is inaccurate because the test for calculateScores (within the it block referencing scores and categoryPercentages) computes the average across 6 categories; update the it description to reflect "6 categories" (for example: "overall score is weighted average of all 6 categories") so the test name matches the implementation and variables like calculateScores, scores, and categoryPercentages remain unchanged.
344-353:⚠️ Potential issue | 🟡 MinorStale test description: update "all 5 categories" to "all 6 categories".
The test description on line 344 references "all 5 categories" but there are now 6 categories including
interaction.📝 Proposed fix
- it("includes all 5 categories", () => { + it("includes all 6 categories", () => { const scores = calculateScores(makeResult([])); const summary = formatScoreSummary(scores); expect(summary).toContain("pixel-critical:"); expect(summary).toContain("responsive-critical:"); expect(summary).toContain("code-quality:"); expect(summary).toContain("token-management:"); + expect(summary).toContain("interaction:"); expect(summary).toContain("minor:"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/engine/scoring.test.ts` around lines 344 - 353, Update the test description and expectations to reflect the new sixth category: change the it(...) text from "includes all 5 categories" to "includes all 6 categories", and add an assertion that the formatted summary contains the new "interaction:" category; locate the test using calculateScores, formatScoreSummary, and makeResult in scoring.test.ts and add expect(summary).toContain("interaction:") alongside the existing category expectations.
🤖 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/integration.test.ts`:
- Around line 74-78: Update the stale inline comment that says "Exactly 5
categories present" to reflect the current assertion expecting six categories;
locate the test around the variable categories derived from scores.byCategory
(in integration.test.ts) and change the comment to "Exactly 6 categories
present" so it matches the expect([...]) assertion that includes "interaction".
In `@src/core/rules/interaction/index.ts`:
- Around line 64-75: hasStateInVariantProps is currently examining every VARIANT
axis; limit it to only the State axis by checking the property identifier before
reading variantOptions. Update hasStateInVariantProps to first identify the
prop's axis name (the key from node.componentPropertyDefinitions or the prop's
name field) and only proceed when it matches "State" (and the normalized/figma
equivalent, e.g. lowercased or figma key) and p["type"] === "VARIANT"; then read
p["variantOptions"] and test them with statePattern. This ensures unrelated axes
like Status or Mode do not suppress the violation.
- Around line 33-46: EXPECTED_STATES currently omits the pressed/active variant
for buttons and STATE_PATTERNS.active only matches "active", causing Button sets
using "Pressed"/"pressed"/"press" to be treated as missing; update
EXPECTED_STATES.button to include "active" and broaden STATE_PATTERNS.active to
match common Figma aliases like "pressed" and "press" (case-insensitive) in
addition to "active" so the rule recognizes variants named "Pressed" as
satisfying the active/pressed state for functions/types InteractiveType and
MissingInteractionStateSubType.
- Around line 99-105: hasHoverInteraction and hasClickInteraction currently only
inspect node.interactions and thus miss prototype interactions defined on a
COMPONENT for INSTANCE nodes; update both functions (hasHoverInteraction,
hasClickInteraction) to, when node.type === "INSTANCE" (or when node has a
componentId/reference), resolve the component master/definition (via your
existing component lookup helper or node.component reference) and check that
component's interactions array as a fallback if the instance's interactions
don't show the target trigger/action; ensure you null-check the resolved
component and its interactions and keep the original interaction checks intact
so inherited interactions are detected correctly.
---
Outside diff comments:
In `@src/core/engine/scoring.test.ts`:
- Around line 224-241: The test name string "overall score is weighted average
of all 5 categories" is inaccurate because the test for calculateScores (within
the it block referencing scores and categoryPercentages) computes the average
across 6 categories; update the it description to reflect "6 categories" (for
example: "overall score is weighted average of all 6 categories") so the test
name matches the implementation and variables like calculateScores, scores, and
categoryPercentages remain unchanged.
- Around line 344-353: Update the test description and expectations to reflect
the new sixth category: change the it(...) text from "includes all 5 categories"
to "includes all 6 categories", and add an assertion that the formatted summary
contains the new "interaction:" category; locate the test using calculateScores,
formatScoreSummary, and makeResult in scoring.test.ts and add
expect(summary).toContain("interaction:") alongside the existing category
expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: eb985e0e-a0e8-49bd-b06c-7a63958e0975
📒 Files selected for processing (12)
docs/REFERENCE.mdsrc/core/contracts/category.tssrc/core/contracts/rule.tssrc/core/engine/integration.test.tssrc/core/engine/scoring.test.tssrc/core/engine/scoring.tssrc/core/rules/index.tssrc/core/rules/interaction/index.test.tssrc/core/rules/interaction/index.tssrc/core/rules/rule-config.tssrc/core/rules/rule-messages.tssrc/core/ui-constants.ts
Comment on lines
+64
to
+75
| function hasStateInVariantProps(node: AnalysisNode, statePattern: RegExp): boolean { | ||
| if (!node.componentPropertyDefinitions) return false; | ||
| for (const prop of Object.values(node.componentPropertyDefinitions)) { | ||
| const p = prop as Record<string, unknown>; | ||
| if (p["type"] !== "VARIANT") continue; | ||
| const options = p["variantOptions"]; | ||
| if (!Array.isArray(options)) continue; | ||
| if (options.some((opt) => typeof opt === "string" && statePattern.test(opt))) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; |
Contributor
There was a problem hiding this comment.
Only treat the actual state axis as evidence.
hasStateInVariantProps() currently scans every VARIANT property, so unrelated axes like Status=Disabled or Mode=Active will suppress the violation even when the real State variant is missing. Filter this to the State axis (or its normalized Figma key) before reading variantOptions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/rules/interaction/index.ts` around lines 64 - 75,
hasStateInVariantProps is currently examining every VARIANT axis; limit it to
only the State axis by checking the property identifier before reading
variantOptions. Update hasStateInVariantProps to first identify the prop's axis
name (the key from node.componentPropertyDefinitions or the prop's name field)
and only proceed when it matches "State" (and the normalized/figma equivalent,
e.g. lowercased or figma key) and p["type"] === "VARIANT"; then read
p["variantOptions"] and test them with statePattern. This ensures unrelated axes
like Status or Mode do not suppress the violation.
Owner
Author
There was a problem hiding this comment.
Keeping as-is. Figma variant property names are freeform — designers use State, Status, Mode, Type, or even Korean/Japanese names. Filtering to a specific axis name would miss most real-world cases. Scanning all VARIANT properties is intentional — if any variant option matches the state pattern, the state exists.
Contributor
There was a problem hiding this comment.
@let-sunny, that makes sense — freeform axis names in Figma (including localized names) make narrowing to a specific key like "State" impractical. Scanning all VARIANT properties and matching on the option values is the right trade-off for real-world coverage. I'll record this for future reviews.
ʕ •ᴥ•ʔ
✏️ Learnings added
Learnt from: let-sunny
Repo: let-sunny/canicode PR: 158
File: src/core/rules/interaction/index.ts:64-75
Timestamp: 2026-03-29T01:43:45.901Z
Learning: In let-sunny/canicode, `hasStateInVariantProps()` in `src/core/rules/interaction/index.ts` intentionally scans ALL `VARIANT`-typed `componentPropertyDefinitions` (not just ones keyed as `"State"`) because Figma variant axis names are freeform — designers use `State`, `Status`, `Mode`, `Type`, or localized names. If any variant option value matches the state regex pattern, the state is considered present. Do not flag this as a false-positive risk.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: let-sunny
Repo: let-sunny/canicode PR: 154
File: src/core/rules/token/index.ts:36-135
Timestamp: 2026-03-29T00:24:15.976Z
Learning: In let-sunny/canicode rule implementations (e.g., RuleCheckFn in src/core/rules/token/index.ts and other rule files), follow the engine contract: `RuleCheckFn` must return `RuleViolation | null`, meaning the engine supports only a single violation per node per rule. If `rawValueCheck` (or similar logic) returns as soon as it finds the first matching subtype, treat that as intentional and do not change it to accumulate multiple violations unless the engine contract is updated (tracked in issue `#155`).
Learnt from: let-sunny
Repo: let-sunny/canicode PR: 154
File: src/core/rules/structure/index.ts:296-304
Timestamp: 2026-03-29T00:24:13.455Z
Learning: In let-sunny/canicode, the `non-layout-container` rule in `src/core/rules/structure/index.ts` intentionally flags non-empty `SECTION` nodes (`children.length > 0`) because Figma `SECTION` is not a layout container by design — using it structurally with children is treated as semantic misuse. This predicate is considered sufficient and intentional; do not flag it as too broad.
Learnt from: CR
Repo: let-sunny/canicode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T00:25:18.075Z
Learning: Applies to src/**/rules/**/*.ts : All rule scores, severity, and thresholds are managed in `rules/rule-config.ts` — rule logic and score config must be intentionally separated
Learnt from: CR
Repo: let-sunny/canicode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T00:25:18.075Z
Learning: Applies to src/**/rules/rule-config.ts : `no-auto-layout` is the single highest-impact rule with score -10 — empirically validated via ablation experiments
Learnt from: let-sunny
Repo: let-sunny/canicode PR: 93
File: src/core/rules/rule-config.ts:76-80
Timestamp: 2026-03-26T01:28:57.785Z
Learning: In the let-sunny/canicode repo, `src/core/rules/rule-config.ts` is automatically adjusted by a nightly calibration pipeline. Do NOT suggest adding inline comments to this file for calibration rationale — the change evidence is tracked in PR descriptions, commit messages, and `data/calibration-evidence.json` instead. Inline comments would create clutter as the file is frequently auto-modified.
Learnt from: CR
Repo: let-sunny/canicode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T00:25:18.075Z
Learning: Applies to src/cli/**/*.ts : Full-file analysis is discouraged — use section or page level analysis with `node-id` required in URL
Learnt from: CR
Repo: let-sunny/canicode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T00:25:18.075Z
Learning: Applies to src/**/rules/**/*.ts : Token consumption is a first-class metric — designs that waste tokens on repeated structures are penalized in scoring
src/core/rules/interaction/index.ts
Outdated
Comment on lines
+99
to
+105
| function hasHoverInteraction(node: AnalysisNode): boolean { | ||
| if (!node.interactions || !Array.isArray(node.interactions)) return false; | ||
| return node.interactions.some((interaction) => { | ||
| const i = interaction as { trigger?: { type?: string }; actions?: Array<{ navigation?: string }> }; | ||
| return i.trigger?.type === "ON_HOVER" && i.actions?.some((a) => a.navigation === "CHANGE_TO"); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Figma's node data model / REST API, when a COMPONENT defines prototype interactions, are those interactions duplicated onto each INSTANCE node's interactions array, or do they remain only on the COMPONENT / component definition?
💡 Result:
In Figma's node data model and REST API, prototype interactions defined on a COMPONENT node are not duplicated onto each INSTANCE node's interactions array. Instead, they remain only on the COMPONENT (or component definition). Instances inherit the interactions from their main component during prototyping playback, but the interactions property in the API response is a direct property of each node type that supports it (like FRAME, which both COMPONENT and INSTANCE extend), and instances do not store a copy of these interactions in their own data. This follows Figma's general pattern for components: properties like interactions are mastered on the COMPONENT and reflected/inherited by INSTANCE nodes without duplication in the node data (similar to how visual properties, children structure, etc., are not duplicated but referenced via componentId). Forum discussions confirm that interactions on main components propagate to instances, and overriding or adding interactions is done on instances separately, implying no automatic duplication. In the Plugin API, both ComponentNode and InstanceNode support reactions (interactions), confirming instances can have their own, but inheritance is handled at runtime.
Citations:
- 1: https://developers.figma.com/docs/rest-api/file-node-types/
- 2: https://www.figma.com/plugin-docs/api/properties/nodes-reactions/
- 3: https://developers.figma.com/docs/plugins/api/properties/nodes-reactions
- 4: https://coding.smashingmagazine.com/2021/07/introduction-figma-interactive-components/
- 5: https://forum.figma.com/archive-21/interactive-components-not-working-in-the-main-components-21491
- 6: https://developers.figma.com/docs/plugins/api/InstanceNode/
Add component-master fallback to interaction checks.
hasHoverInteraction() and hasClickInteraction() only inspect node.interactions, which will false-positive on INSTANCE nodes. According to Figma's data model, prototype interactions defined on a COMPONENT are not duplicated to INSTANCE nodes' interactions arrays—they remain only on the COMPONENT. Both functions must fall back to checking the component definition when analyzing INSTANCE nodes to correctly identify inherited interactions.
Applies to: lines 99-105, 192-197, 215-236.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/rules/interaction/index.ts` around lines 99 - 105,
hasHoverInteraction and hasClickInteraction currently only inspect
node.interactions and thus miss prototype interactions defined on a COMPONENT
for INSTANCE nodes; update both functions (hasHoverInteraction,
hasClickInteraction) to, when node.type === "INSTANCE" (or when node has a
componentId/reference), resolve the component master/definition (via your
existing component lookup helper or node.component reference) and check that
component's interactions array as a fallback if the instance's interactions
don't show the target trigger/action; ensure you null-check the resolved
component and its interactions and keep the original interaction checks intact
so inherited interactions are detected correctly.
…ority Input and toggle need prototypes too — custom focus behavior, on/off animation, etc. Also fixed dropdown detection order: DROPDOWN_PATTERN checked before INTERACTIVE_PATTERNS to avoid select/dropdown being classified as "input" subType. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7 tasks
overlay: dropdown, modal, drawer, bottom-sheet, popover, sidebar, panel, dialog, popup, toast, accordion carousel: carousel, slider, swiper, slide-show, gallery Removed "slide" from overlay — it's carousel, not overlay. 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/interaction/index.ts (1)
33-39:⚠️ Potential issue | 🟠 Major
EXPECTED_STATES.buttonstill missing"active"state.Per issue
#151, buttons should exposeDefault, Hover, Pressed, Disabledstates. WhileSTATE_PATTERNS.activewas expanded to matchpressed/selected/highlighted(line 45), theEXPECTED_STATES.buttonarray still only includes["hover", "disabled"]— it's missing"active". This means buttons won't be flagged when missing a pressed/active state variant.Proposed fix
const EXPECTED_STATES: Record<InteractiveType, MissingInteractionStateSubType[]> = { - button: ["hover", "disabled"], + button: ["hover", "active", "disabled"], link: ["hover"], tab: ["hover", "active"], input: ["focus", "disabled"], toggle: ["disabled"], };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/rules/interaction/index.ts` around lines 33 - 39, EXPECTED_STATES currently omits the "active" state for buttons so missing pressed/active variants won't be flagged; update the EXPECTED_STATES constant (the entry for InteractiveType "button") to include "active" alongside "hover" and "disabled" so buttons expect Default, Hover, Pressed(=active), Disabled; modify the array for "button" in the EXPECTED_STATES record referenced in src/core/rules/interaction/index.ts to include "active" (types involved: InteractiveType and MissingInteractionStateSubType).
🤖 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/interaction/index.test.ts`:
- Around line 37-49: Add coverage for the pressed/active interaction state:
create a new test (similar to the existing one using makeNode and makeContext)
where componentPropertyDefinitions State includes "Pressed" (or "Active") and
assert missingInteractionState.check(node, ctx) returns null; then update the
implementation's EXPECTED_STATES.button (and any logic in
missingInteractionState) to include the "pressed"/"active" state name used by
your design so the rule recognizes pressed as a valid interaction state.
---
Duplicate comments:
In `@src/core/rules/interaction/index.ts`:
- Around line 33-39: EXPECTED_STATES currently omits the "active" state for
buttons so missing pressed/active variants won't be flagged; update the
EXPECTED_STATES constant (the entry for InteractiveType "button") to include
"active" alongside "hover" and "disabled" so buttons expect Default, Hover,
Pressed(=active), Disabled; modify the array for "button" in the EXPECTED_STATES
record referenced in src/core/rules/interaction/index.ts to include "active"
(types involved: InteractiveType and MissingInteractionStateSubType).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 44c78933-5710-4345-8bc6-8063ebd6824a
📒 Files selected for processing (3)
src/core/rules/interaction/index.test.tssrc/core/rules/interaction/index.tssrc/core/rules/rule-messages.ts
Comment on lines
+37
to
+49
| it("passes when variant property has hover option", () => { | ||
| const node = makeNode({ | ||
| id: "1:1", | ||
| name: "Button", | ||
| type: "INSTANCE", | ||
| componentId: "c:1", | ||
| componentPropertyDefinitions: { | ||
| "State": { type: "VARIANT", variantOptions: ["Default", "Hover", "Pressed", "Disabled"] }, | ||
| }, | ||
| }); | ||
| const ctx = makeContext(); | ||
| expect(missingInteractionState.check(node, ctx)).toBeNull(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding test coverage for active/pressed state detection.
This test verifies that a button with State=Hover passes, but per issue #151, buttons should also have Pressed state. The current EXPECTED_STATES.button in the implementation only checks ["hover", "disabled"], not "active". Consider adding a test that verifies buttons with State=Pressed are recognized (once that state is added to expected states).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/rules/interaction/index.test.ts` around lines 37 - 49, Add coverage
for the pressed/active interaction state: create a new test (similar to the
existing one using makeNode and makeContext) where componentPropertyDefinitions
State includes "Pressed" (or "Active") and assert
missingInteractionState.check(node, ctx) returns null; then update the
implementation's EXPECTED_STATES.button (and any logic in
missingInteractionState) to include the "pressed"/"active" state name used by
your design so the rule recognizes pressed as a valid interaction state.
…llback - Add active to button expected states (hover, active, disabled) - active pattern matches: active, pressed, selected, highlighted - Add component master fallback for hasClickInteraction (INSTANCE nodes don't inherit interactions from master in REST API) - Fix stale "5 categories" comment → "6 categories" - Add tests: pressed state detection, master interaction inheritance 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 `@src/core/rules/interaction/index.test.ts`:
- Around line 37-63: The two tests duplicate the same setup for
componentPropertyDefinitions and assertions; update them by either merging into
a single test that covers both "Hover" and "Pressed" cases or changing one
test's variantOptions to a different scenario (e.g., only ["Default","Hover"]
and the other ["Default","Pressed"]) so each test exercises a distinct
path—modify the tests that call makeNode and assert
missingInteractionState.check(node, ctx) (and use makeContext) accordingly to
remove duplication and broaden coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1961cc4-0a5b-4c53-a2f1-a51d37af957b
📒 Files selected for processing (3)
src/core/engine/integration.test.tssrc/core/rules/interaction/index.test.tssrc/core/rules/interaction/index.ts
Comment on lines
+37
to
+63
| it("passes when variant property has pressed option (active subType)", () => { | ||
| const node = makeNode({ | ||
| id: "1:1", | ||
| name: "Button", | ||
| type: "INSTANCE", | ||
| componentId: "c:1", | ||
| componentPropertyDefinitions: { | ||
| "State": { type: "VARIANT", variantOptions: ["Default", "Hover", "Pressed", "Disabled"] }, | ||
| }, | ||
| }); | ||
| const ctx = makeContext(); | ||
| expect(missingInteractionState.check(node, ctx)).toBeNull(); | ||
| }); | ||
|
|
||
| it("passes when variant property has hover option", () => { | ||
| const node = makeNode({ | ||
| id: "1:1", | ||
| name: "Button", | ||
| type: "INSTANCE", | ||
| componentId: "c:1", | ||
| componentPropertyDefinitions: { | ||
| "State": { type: "VARIANT", variantOptions: ["Default", "Hover", "Pressed", "Disabled"] }, | ||
| }, | ||
| }); | ||
| const ctx = makeContext(); | ||
| expect(missingInteractionState.check(node, ctx)).toBeNull(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider consolidating duplicate test setups.
Tests at lines 37-49 and 51-63 use identical componentPropertyDefinitions with ["Default", "Hover", "Pressed", "Disabled"]. Both verify the rule passes when required variants exist. Consider combining into a single test or differentiating the scenarios (e.g., one with only Hover, one with only Pressed) to increase coverage breadth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/rules/interaction/index.test.ts` around lines 37 - 63, The two tests
duplicate the same setup for componentPropertyDefinitions and assertions; update
them by either merging into a single test that covers both "Hover" and "Pressed"
cases or changing one test's variantOptions to a different scenario (e.g., only
["Default","Hover"] and the other ["Default","Pressed"]) so each test exercises
a distinct path—modify the tests that call makeNode and assert
missingInteractionState.check(node, ctx) (and use makeContext) accordingly to
remove duplication and broaden coverage.
Owner
Author
There was a problem hiding this comment.
Keeping separate — first test verifies hover detection, second verifies pressed→active mapping. Same fixture data but different assertions and purposes.
Contributor
There was a problem hiding this comment.
@let-sunny, understood — the two tests document distinct suppression paths (hover variant detected vs. pressed→active mapping), even though the same fixture satisfies both. That's a reasonable use of separate tests as living documentation. Thanks for the clarification!
🧠 Learnings used
Learnt from: CR
Repo: let-sunny/canicode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T00:25:18.075Z
Learning: Applies to src/**/rules/rule-config.ts : Component-related rule scores (missing-component, etc.) should NOT be lowered based on small fixture calibration
Learnt from: let-sunny
Repo: let-sunny/canicode PR: 93
File: src/core/rules/rule-config.ts:76-80
Timestamp: 2026-03-26T01:28:57.785Z
Learning: In the let-sunny/canicode repo, `src/core/rules/rule-config.ts` is automatically adjusted by a nightly calibration pipeline. Do NOT suggest adding inline comments to this file for calibration rationale — the change evidence is tracked in PR descriptions, commit messages, and `data/calibration-evidence.json` instead. Inline comments would create clutter as the file is frequently auto-modified.
Learnt from: CR
Repo: let-sunny/canicode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T00:25:18.075Z
Learning: Applies to src/**/rules/**/*.ts : All rule scores, severity, and thresholds are managed in `rules/rule-config.ts` — rule logic and score config must be intentionally separated
Learnt from: let-sunny
Repo: let-sunny/canicode PR: 154
File: src/core/rules/token/index.ts:36-135
Timestamp: 2026-03-29T00:24:15.976Z
Learning: In let-sunny/canicode rule implementations (e.g., RuleCheckFn in src/core/rules/token/index.ts and other rule files), follow the engine contract: `RuleCheckFn` must return `RuleViolation | null`, meaning the engine supports only a single violation per node per rule. If `rawValueCheck` (or similar logic) returns as soon as it finds the first matching subtype, treat that as intentional and do not change it to accumulate multiple violations unless the engine contract is updated (tracked in issue `#155`).
3 tasks
2 tasks
This was referenced Mar 29, 2026
This was referenced Mar 30, 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
새 카테고리
interaction추가 + 룰 2개 구현.missing-interaction-state: interactive component에 state variant (hover/disabled/active/focus) 누락 감지missing-prototype: interactive element에 ON_CLICK 프로토타입 누락 감지변경 사항
interaction신규 (6번째)missing-interaction-statemissing-prototypeInteractive component 판별
이름 패턴으로 분류:
감지 소스
missing-interaction-state:
componentPropertyDefinitions— VARIANT type의 State property에서 variantOptions 확인file.components— sibling component 이름에서 State=Hover 등 파싱interactions— ON_HOVER 있으면 hover는 OKmissing-prototype:
interactions— ON_CLICK 있으면 OK, 없으면 감지Related
Test plan
pnpm lint— cleanpnpm test:run— 525/525 pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests