Skip to content

refactor: unify missing-component with 4-stage checks (#17)#22

Merged
let-sunny merged 5 commits intomainfrom
refactor/missing-component-overhaul
Mar 24, 2026
Merged

refactor: unify missing-component with 4-stage checks (#17)#22
let-sunny merged 5 commits intomainfrom
refactor/missing-component-overhaul

Conversation

@let-sunny
Copy link
Copy Markdown
Owner

@let-sunny let-sunny commented Mar 24, 2026

Summary

repeated-frame-structurenested-instance-overridemissing-component에 통합하여 4단계 체크로 고도화.

4-Stage Check

Stage 대상 감지 메시지
1 FRAME 컴포넌트 존재하는데 안 씀 "Component 'X' exists — use instances"
2 FRAME 이름 반복 N회 "appears N times — make it a component"
3 FRAME 형제 구조 동일 "sibling frames share structure — extract component"
4 INSTANCE 같은 컴포넌트인데 override 다름 "divergent overrides — add variant"

흡수된 룰

  • repeated-frame-structure → Stage 3으로 흡수, RuleId 제거
  • nested-instance-override → Stage 4로 흡수 (override 개수→override 내용 비교로 개선), RuleId 제거

design-tree 컴포넌트 정보 반영

INSTANCE 노드에 [component: Name] 어노테이션 추가:

MyButton (INSTANCE, 120x40) [component: Button]

변경 파일

  • src/core/contracts/rule.ts — RuleId 2개 제거
  • src/core/rules/rule-config.ts — config 2개 제거, missing-component에 옵션 추가
  • src/core/rules/component/index.ts — 4-stage 통합 구현
  • src/core/engine/design-tree.ts — INSTANCE 컴포넌트 어노테이션
  • docs/REFERENCE.md — 룰 테이블 업데이트
  • 테스트 26개 신규 (missing-component) + 3개 신규 (design-tree)

Test plan

  • pnpm test:run — 292 tests passed
  • pnpm lint — clean
  • pnpm build — clean
  • design-tree fixture 검증: material3-kit-1 INSTANCE에 [component: ...] 출력 확인

Closes #17

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Design trees now display component annotations for instance nodes
    • Component validation enhanced with multi-stage detection for name, structure, and instance differences
  • Documentation

    • Reference and configuration docs updated to reflect the revised component rule set
  • Tests

    • New and updated test coverage added for the consolidated component validation; obsolete tests removed
  • Chores

    • Component-related rules consolidated and deduplication/state handling introduced

Merge repeated-frame-structure and nested-instance-override into a
unified missing-component rule with 4 stages:

1. Component exists but not used — FRAME name matches component metadata
2. Name-based repetition — same-name FRAME N+ times (existing)
3. Structure-based repetition — sibling FRAMEs with identical fingerprint
4. Instance style divergence — same-componentId instances with different overrides

Also:
- design-tree annotates INSTANCE nodes with [component: Name]
- Remove repeated-frame-structure and nested-instance-override from RuleId
- 26 new missing-component tests + 3 design-tree tests
- 292 total tests passing

Closes #17

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@let-sunny let-sunny marked this pull request as draft March 24, 2026 14:51
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 24, 2026

📝 Walkthrough

Walkthrough

Consolidates component-related checks into a single, 4-stage missing-component rule (name match, name-based repetition, sibling structural fingerprinting, instance divergence), removes the separate nested-instance-override and repeated-frame-structure rules/configs/tests, adds module-level dedup reset, and threads component metadata into design-tree rendering to annotate INSTANCE nodes.

Changes

Cohort / File(s) Summary
Rule Types & Docs
src/core/contracts/rule.ts, docs/REFERENCE.md
Removed nested-instance-override and repeated-frame-structure IDs from the exported RuleId union and reference docs; updated rule count comments.
Rule Implementations
src/core/rules/component/index.ts, src/core/rules/index.ts
Rewrote missing-component to a 4-stage detector (component name match, name repetition, sibling structural fingerprint, instance-style divergence), folded logic from removed rules into it, added resetMissingComponentState() and removed standalone nested-instance-override and repeated-frame-structure.
Rule Config
src/core/rules/rule-config.ts
Removed nested-instance-override and repeated-frame-structure entries; extended missing-component.options with structureMinRepetitions and maxFingerprintDepth.
Design-tree Rendering
src/core/engine/design-tree.ts, src/core/engine/design-tree.test.ts
Extended renderNode() to accept components map and annotate INSTANCE headers with [component: <name>] when a match exists; updated generateDesignTree() and added tests for component annotations.
Tests Added
src/core/rules/component/missing-component.test.ts
Added comprehensive Jest suite covering all 4 stages, dedup behavior, and resetMissingComponentState(); exercises name, structure, and instance-diff cases.
Tests Removed
src/core/rules/component/repeated-frame-structure.test.ts
Deleted the previous repeated-frame-structure test suite (logic now absorbed into missing-component).
Contracts Update
src/core/contracts/figma-node.ts
Added "SLOT" to AnalysisNodeTypeSchema/AnalysisNodeType.
Engine State Reset
src/core/engine/rule-engine.ts
Call to resetMissingComponentState() added at the start of each analysis run to clear module-level deduplication state.

Sequence Diagram(s)

sequenceDiagram
  participant CLI as Client
  participant Engine as RuleEngine
  participant File as AnalysisFile
  participant Rule as missing-component
  participant Components as file.components
  participant Reporter as Reporter

  CLI->>Engine: analyze(file)
  Engine->>Rule: resetMissingComponentState()
  Engine->>File: traverse nodes
  File->>Rule: for each node -> check(node, file, options)
  alt node is FRAME/INSTANCE
    Rule->>Components: consult components metadata
    Rule-->>Rule: Stage1: name match -> emit (if match & repetition)
    Rule-->>Rule: Stage2: global name repetition -> emit
    Rule-->>Rule: Stage3: sibling fingerprint -> emit
    Rule-->>Rule: Stage4: instance divergence vs master -> emit
  end
  Rule->>Reporter: emit violations (deduped per-module)
  Engine->>Reporter: collect results
  Reporter->>CLI: output report
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

Poem

🐰 I hopped through nodes both far and near,
I stitched three checks into one with cheer.
Fingerprints, names, and instance glance—
Duplicates hide, now given a chance.
A tiny hop for cleaner design here! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 78.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the main change: unifying the missing-component rule with a 4-stage check implementation that consolidates related rules.
Linked Issues check ✅ Passed All PR objectives from issue #17 are met: missing-component rule enhanced with 4-stage checks covering component existence, name-based repetition, structure-based repetition, and instance overrides; repeated-frame-structure and nested-instance-override rules removed and absorbed; design-tree annotated with component metadata.
Out of Scope Changes check ✅ Passed All changes are in scope: SLOT addition to AnalysisNodeType supports components using slots (mentioned in commit summary); resetMissingComponentState() integration ensures test isolation per PR objectives; all modifications directly support the 4-stage missing-component implementation.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/missing-component-overhaul

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 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 92-108: The module-level dedup Sets seenStage1ComponentNames and
seenStage4ComponentIds persist across runs and rely on callers to call
resetMissingComponentState(); change the implementation so dedup state is owned
by the analysis run instead of module-level globals: remove the module-level
Sets and instead create and pass per-run Sets from the analysis engine into the
missing-component rule (or return a factory that creates a fresh pair), and
delete/resetMissingComponentState(); alternatively, if you must keep
module-level state, add a prominent comment and ensure the analysis engine
invokes resetMissingComponentState() at analysis start (call the exported
resetMissingComponentState function from the analysis initialization path) so
state is cleared for each run.
- Around line 159-169: The comparison sameNameFrames[0] === node.id can produce
an undefined-index type under noUncheckedIndexedAccess; change the block in the
sameNameFrames handling to explicitly guard the indexed access: after confirming
sameNameFrames && sameNameFrames.length >= minRepetitions, read the first
element into a local (e.g. const first = sameNameFrames[0]) and check first !==
undefined before comparing to node.id and returning the missingComponentDef.id
payload (include nodeId/nodePath/message as before).
- Around line 140-148: The check uses sameNameFrames[0] directly which under
noUncheckedIndexedAccess is string | undefined; update the conditional in the
block that returns the missingComponentDef result to explicitly guard the
indexed access (e.g., ensure sameNameFrames.length > 0 or sameNameFrames[0] !==
undefined before comparing to node.id). Modify the condition around
sameNameFrames[0] === node.id in the function that produces the object with
ruleId: missingComponentDef.id and nodePath: context.path.join(" > ") so the
comparison only runs when sameNameFrames[0] is guaranteed defined.

In `@src/core/rules/component/missing-component.test.ts`:
- Around line 282-296: The test reuses the same child object instance for both
frameA and frameB which can hide issues; update the test so each sibling has its
own child node instance (e.g., call makeChildFrame twice or clone the child)
instead of assigning the same child object to frameA.children and
frameB.children; adjust the setup around makeChildFrame, makeNode, frameA,
frameB (the input to missingComponent.check) so children are distinct objects
while preserving identical structure for fingerprinting.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6832e488-dc4e-42c6-9420-7bb8904edf3f

📥 Commits

Reviewing files that changed from the base of the PR and between e9cfa59 and de25d04.

📒 Files selected for processing (9)
  • docs/REFERENCE.md
  • src/core/contracts/rule.ts
  • src/core/engine/design-tree.test.ts
  • src/core/engine/design-tree.ts
  • src/core/rules/component/index.ts
  • src/core/rules/component/missing-component.test.ts
  • src/core/rules/component/repeated-frame-structure.test.ts
  • src/core/rules/index.ts
  • src/core/rules/rule-config.ts
💤 Files with no reviewable changes (1)
  • src/core/rules/component/repeated-frame-structure.test.ts

Comment on lines +92 to +108
/**
* Module-level dedup Sets for missing-component stages.
* These prevent duplicate violations when the same pattern is encountered
* multiple times during a single analysis run.
*/
const seenStage1ComponentNames = new Set<string>();
const seenStage4ComponentIds = new Set<string>();

/**
* Reset deduplication state for missing-component between analysis runs.
* Call this at the start of each analysis if the process is long-running
* (e.g. MCP server mode).
*/
export function resetMissingComponentState(): void {
seenStage1ComponentNames.clear();
seenStage4ComponentIds.clear();
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Module-level mutable state requires careful lifecycle management.

The module-level Sets for deduplication persist across multiple analysis runs in long-running processes (e.g., MCP server mode). The resetMissingComponentState() function is provided, but callers must remember to invoke it. Consider documenting this requirement prominently or integrating the reset into the analysis engine's lifecycle.

🤖 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 92 - 108, The module-level
dedup Sets seenStage1ComponentNames and seenStage4ComponentIds persist across
runs and rely on callers to call resetMissingComponentState(); change the
implementation so dedup state is owned by the analysis run instead of
module-level globals: remove the module-level Sets and instead create and pass
per-run Sets from the analysis engine into the missing-component rule (or return
a factory that creates a fresh pair), and delete/resetMissingComponentState();
alternatively, if you must keep module-level state, add a prominent comment and
ensure the analysis engine invokes resetMissingComponentState() at analysis
start (call the exported resetMissingComponentState function from the analysis
initialization path) so state is cleared for each run.

let-sunny and others added 3 commits March 24, 2026 23:56
…ence

Stage 4 now compares instance styles (fills, strokes, effects,
cornerRadius, strokeWeight, individualStrokeWeights) against the
component master from componentDefinitions. Any style override is
flagged — use variants instead of direct style changes.

Requires componentDefinitions to be present (from save-fixture #16).
Without it, Stage 4 is silently skipped.

- Remove collectInstancesByComponentId (no longer needed)
- Add detectStyleOverrides helper
- 8 new Stage 4 tests for master comparison

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Figma's SLOT node type was missing from the schema, causing
componentDefinitions validation to fail for components that
use slots (e.g. Navigation Pill List).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add explicit undefined guard for sameNameFrames[0] in Stage 1 and 2
  (noUncheckedIndexedAccess compliance)
- Call resetMissingComponentState() in RuleEngine.analyze() to clear
  dedup state between runs (important for MCP server mode)
- Use separate child object instances in Stage 3 tests to avoid
  hiding issues from shared references
- Add SLOT to AnalysisNodeType schema (Figma SLOT node type)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@let-sunny let-sunny marked this pull request as ready for review March 24, 2026 15:11
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 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.ts`:
- Around line 148-151: The analyze method in rule-engine.ts resets module-level
dedup state via resetMissingComponentState() but misses the companion reset for
the `missing-component-description` rule; call the exported
resetMissingComponentDescriptionState() (from the component rules module)
alongside resetMissingComponentState() at the start of analyze(file:
AnalysisFile): AnalysisResult to clear the seenMissingDescriptionComponentIds
Set between runs, ensuring both module-level caches are reset.

In `@src/core/rules/component/index.ts`:
- Around line 254-277: The code marks node.componentId in seenStage4ComponentIds
before checking for style overrides, which causes later instances with overrides
to be skipped; change the logic in the INSTANCE handling so you call
detectStyleOverrides(master, node) first, and only add node.componentId to
seenStage4ComponentIds when you are about to return the violation object (i.e.,
when overrides.length > 0 and you emit the missingComponentDef.id violation
using componentMeta/name), leaving the ID unmarked and allowing other instances
to be evaluated if no violation is found.
- Around line 139-182: The code calls collectFrameNames(context.file.document)
twice (in the Stage 1 block and again in Stage 2) causing an unnecessary
traversal; compute the frame names once before the Stage 1/Stage 2 logic and
reuse that Map for both checks. Modify the function to declare a single const
frameNames = collectFrameNames(context.file.document) above the existing Stage 1
code, replace frameNames2/sameNameFrames2/first2 with the reused
frameNames/sameNameFrames/first variables, and keep the existing conditions
involving seenStage1ComponentNames, missingComponentDef, node, options, and
getRuleOption unchanged. Ensure behavior and rule messages remain identical
while removing the redundant call.

In `@src/core/rules/component/missing-component.test.ts`:
- Around line 592-625: Add a test that verifies deduplication when the first
instance has no overrides but a later instance does: create a master node and
two instances (inst1 with fills identical to master, inst2 with different
fills), assemble a file with componentDefinitions and components, build separate
contexts ctx1 and ctx2, then assert missingComponent.check(inst1, ctx1) returns
null and missingComponent.check(inst2, ctx2) returns not null; reference
makeNode, makeFile, makeContext and missingComponent.check to locate where to
add this case in missing-component.test.ts.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3f430198-96cd-426e-ae61-a7c92017bcc7

📥 Commits

Reviewing files that changed from the base of the PR and between de25d04 and b3d952e.

📒 Files selected for processing (4)
  • src/core/contracts/figma-node.ts
  • src/core/engine/rule-engine.ts
  • src/core/rules/component/index.ts
  • src/core/rules/component/missing-component.test.ts

Comment on lines 148 to +151
analyze(file: AnalysisFile): AnalysisResult {
// Reset module-level dedup state for rules that track seen patterns
resetMissingComponentState();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Missing reset for missingComponentDescription state.

The resetMissingComponentState() call correctly clears dedup state for the missing-component rule. However, src/core/rules/component/index.ts also exports resetMissingComponentDescriptionState() (line 494) for the missing-component-description rule's seenMissingDescriptionComponentIds Set, which has the same module-level persistence issue.

For consistency and correctness in long-running processes, consider also resetting this state:

🛡️ Proposed fix
-import { resetMissingComponentState } from "../rules/component/index.js";
+import { resetMissingComponentState, resetMissingComponentDescriptionState } from "../rules/component/index.js";
   analyze(file: AnalysisFile): AnalysisResult {
     // Reset module-level dedup state for rules that track seen patterns
     resetMissingComponentState();
+    resetMissingComponentDescriptionState();
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/engine/rule-engine.ts` around lines 148 - 151, The analyze method in
rule-engine.ts resets module-level dedup state via resetMissingComponentState()
but misses the companion reset for the `missing-component-description` rule;
call the exported resetMissingComponentDescriptionState() (from the component
rules module) alongside resetMissingComponentState() at the start of
analyze(file: AnalysisFile): AnalysisResult to clear the
seenMissingDescriptionComponentIds Set between runs, ensuring both module-level
caches are reset.

Comment on lines +139 to +182
if (matchingComponent) {
const frameNames = collectFrameNames(context.file.document);
const sameNameFrames = frameNames.get(node.name);

const first = sameNameFrames?.[0];
if (
sameNameFrames &&
first !== undefined &&
sameNameFrames.length >= 2 &&
!seenStage1ComponentNames.has(node.name.toLowerCase())
) {
seenStage1ComponentNames.add(node.name.toLowerCase());
if (first === node.id) {
return {
ruleId: missingComponentDef.id,
nodeId: node.id,
nodePath: context.path.join(" > "),
message: `Component "${matchingComponent.name}" exists — use instances instead of repeated frames (${sameNameFrames.length} found)`,
};
}
}
}

const minRepetitions = (options?.["minRepetitions"] as number) ??
getRuleOption("missing-component", "minRepetitions", 3);
// Stage 2: Name-based repetition (existing logic)
const minRepetitions =
(options?.["minRepetitions"] as number | undefined) ??
getRuleOption("missing-component", "minRepetitions", 3);

{
const frameNames2 = collectFrameNames(context.file.document);
const sameNameFrames2 = frameNames2.get(node.name);
const first2 = sameNameFrames2?.[0];

if (sameNameFrames2 && first2 !== undefined && sameNameFrames2.length >= minRepetitions) {
if (first2 === node.id) {
return {
ruleId: missingComponentDef.id,
nodeId: node.id,
nodePath: context.path.join(" > "),
message: `"${node.name}" appears ${sameNameFrames2.length} times — consider making it a component`,
};
}
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

collectFrameNames is called twice for the same document in Stages 1 and 2.

Both Stage 1 (line 140) and Stage 2 (line 168) call collectFrameNames(context.file.document) separately. Since the document doesn't change between stages, this redundant traversal can be avoided by computing it once.

♻️ Proposed refactor: Compute frame names once
   if (node.type === "FRAME") {
+    // Collect frame names once for both Stage 1 and Stage 2
+    const frameNames = collectFrameNames(context.file.document);
+
     // Stage 1: Component exists but not used — FRAME name matches a component in metadata AND frame is repeated
     const components = context.file.components;
     const matchingComponent = Object.values(components).find(
       (c) => c.name.toLowerCase() === node.name.toLowerCase()
     );

     if (matchingComponent) {
-      const frameNames = collectFrameNames(context.file.document);
       const sameNameFrames = frameNames.get(node.name);
       // ... Stage 1 logic
     }

     // Stage 2: Name-based repetition (existing logic)
     const minRepetitions =
       (options?.["minRepetitions"] as number | undefined) ??
       getRuleOption("missing-component", "minRepetitions", 3);

-    {
-      const frameNames2 = collectFrameNames(context.file.document);
-      const sameNameFrames2 = frameNames2.get(node.name);
+    const sameNameFrames2 = frameNames.get(node.name);
🤖 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 139 - 182, The code calls
collectFrameNames(context.file.document) twice (in the Stage 1 block and again
in Stage 2) causing an unnecessary traversal; compute the frame names once
before the Stage 1/Stage 2 logic and reuse that Map for both checks. Modify the
function to declare a single const frameNames =
collectFrameNames(context.file.document) above the existing Stage 1 code,
replace frameNames2/sameNameFrames2/first2 with the reused
frameNames/sameNameFrames/first variables, and keep the existing conditions
involving seenStage1ComponentNames, missingComponentDef, node, options, and
getRuleOption unchanged. Ensure behavior and rule messages remain identical
while removing the redundant call.

Comment on lines +254 to +277
if (node.type === "INSTANCE" && node.componentId) {
if (seenStage4ComponentIds.has(node.componentId)) return null;
seenStage4ComponentIds.add(node.componentId);

const componentDefs = context.file.componentDefinitions;
if (!componentDefs) return null;

// Collect frame names in the file (cached per analysis run would be better)
const frameNames = collectFrameNames(context.file.document);
const sameNameFrames = frameNames.get(node.name);
const master = componentDefs[node.componentId];
if (!master) return null;

// Compare style properties between master and instance
const overrides = detectStyleOverrides(master, node);
if (overrides.length > 0) {
const componentMeta = context.file.components[node.componentId];
const componentName = componentMeta?.name ?? node.name;

if (sameNameFrames && sameNameFrames.length >= minRepetitions) {
// Only report on the first occurrence to avoid duplicate issues
if (sameNameFrames[0] === node.id) {
return {
ruleId: missingComponentDef.id,
nodeId: node.id,
nodePath: context.path.join(" > "),
message: `"${node.name}" appears ${sameNameFrames.length} times - consider making it a component`,
message: `"${componentName}" instance has style overrides (${overrides.join(", ")}) — use a variant instead of direct style changes`,
};
}
return null;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Stage 4 dedup marks componentId as seen before checking for overrides, potentially missing violations.

The componentId is added to seenStage4ComponentIds (line 256) before determining whether there are style overrides. If the first instance encountered has no overrides (line 277 returns null), subsequent instances of the same component with actual overrides will be incorrectly skipped.

🐛 Proposed fix: Only mark as seen when violation is emitted
   if (node.type === "INSTANCE" && node.componentId) {
     if (seenStage4ComponentIds.has(node.componentId)) return null;
-    seenStage4ComponentIds.add(node.componentId);

     const componentDefs = context.file.componentDefinitions;
     if (!componentDefs) return null;

     const master = componentDefs[node.componentId];
     if (!master) return null;

     // Compare style properties between master and instance
     const overrides = detectStyleOverrides(master, node);
     if (overrides.length > 0) {
+      seenStage4ComponentIds.add(node.componentId);
       const componentMeta = context.file.components[node.componentId];
       const componentName = componentMeta?.name ?? node.name;

       return {
         ruleId: missingComponentDef.id,
         nodeId: node.id,
         nodePath: context.path.join(" > "),
         message: `"${componentName}" instance has style overrides (${overrides.join(", ")}) — use a variant instead of direct style changes`,
       };
     }
     return null;
   }
🤖 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 254 - 277, The code marks
node.componentId in seenStage4ComponentIds before checking for style overrides,
which causes later instances with overrides to be skipped; change the logic in
the INSTANCE handling so you call detectStyleOverrides(master, node) first, and
only add node.componentId to seenStage4ComponentIds when you are about to return
the violation object (i.e., when overrides.length > 0 and you emit the
missingComponentDef.id violation using componentMeta/name), leaving the ID
unmarked and allowing other instances to be evaluated if no violation is found.

Comment on lines +592 to +625
it("deduplicates per componentId", () => {
const master = makeNode({
id: "comp:1",
name: "Button",
type: "COMPONENT",
fills: [{ type: "SOLID", color: { r: 0, g: 0, b: 1 } }],
});
const inst1 = makeNode({
id: "inst:1",
name: "Button",
type: "INSTANCE",
componentId: "comp:1",
fills: [{ type: "SOLID", color: { r: 1, g: 0, b: 0 } }],
});
const inst2 = makeNode({
id: "inst:2",
name: "Button",
type: "INSTANCE",
componentId: "comp:1",
fills: [{ type: "SOLID", color: { r: 0, g: 1, b: 0 } }],
});

const file = makeFile({
document: makeNode({ id: "0:1", name: "Doc", type: "DOCUMENT", children: [inst1, inst2] }),
components: { "comp:1": { key: "k", name: "Button", description: "" } },
componentDefinitions: { "comp:1": master },
});

const ctx1 = makeContext({ file });
const ctx2 = makeContext({ file });

expect(missingComponent.check(inst1, ctx1)).not.toBeNull();
expect(missingComponent.check(inst2, ctx2)).toBeNull();
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Missing test case for dedup edge case where first instance has no overrides.

The current test verifies deduplication when the first instance has overrides. However, given the bug identified in the implementation (see index.ts review), a test case should verify behavior when the first instance matches the master exactly (no overrides) but subsequent instances have overrides.

🧪 Suggested additional test case
it("flags second instance when first instance has no overrides", () => {
  const master = makeNode({
    id: "comp:1",
    name: "Button",
    type: "COMPONENT",
    fills: [{ type: "SOLID", color: { r: 0, g: 0, b: 1 } }],
  });
  // inst1 matches master exactly (no overrides)
  const inst1 = makeNode({
    id: "inst:1",
    name: "Button",
    type: "INSTANCE",
    componentId: "comp:1",
    fills: [{ type: "SOLID", color: { r: 0, g: 0, b: 1 } }],
  });
  // inst2 has overrides
  const inst2 = makeNode({
    id: "inst:2",
    name: "Button",
    type: "INSTANCE",
    componentId: "comp:1",
    fills: [{ type: "SOLID", color: { r: 1, g: 0, b: 0 } }],
  });

  const file = makeFile({
    document: makeNode({ id: "0:1", name: "Doc", type: "DOCUMENT", children: [inst1, inst2] }),
    components: { "comp:1": { key: "k", name: "Button", description: "" } },
    componentDefinitions: { "comp:1": master },
  });

  const ctx1 = makeContext({ file });
  const ctx2 = makeContext({ file });

  // First instance has no overrides — should return null
  expect(missingComponent.check(inst1, ctx1)).toBeNull();
  // Second instance has overrides — should be flagged (not deduped)
  expect(missingComponent.check(inst2, ctx2)).not.toBeNull();
});

Note: This test will fail with the current implementation due to the bug where componentId is marked as seen before checking for overrides.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/rules/component/missing-component.test.ts` around lines 592 - 625,
Add a test that verifies deduplication when the first instance has no overrides
but a later instance does: create a master node and two instances (inst1 with
fills identical to master, inst2 with different fills), assemble a file with
componentDefinitions and components, build separate contexts ctx1 and ctx2, then
assert missingComponent.check(inst1, ctx1) returns null and
missingComponent.check(inst2, ctx2) returns not null; reference makeNode,
makeFile, makeContext and missingComponent.check to locate where to add this
case in missing-component.test.ts.

- Reset seenMissingDescriptionComponentIds in engine.analyze() alongside
  missing-component state
- Stage 4: only mark componentId as seen when override found, allowing
  later instances to be checked if first has no override
- Compute collectFrameNames once for Stage 1+2 instead of twice
- Add test: first instance no override, second has override → flags

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor: missing-component 룰 고도화 — 이름+구조+컴포넌트 존재 여부 통합

1 participant