refactor: centralize node semantic classification (#157)#161
Merged
Conversation
….ts (#157) Move all "what is this node?" predicates and patterns to a single file: - Container: isContainerNode, hasAutoLayout, hasTextContent, hasOverlappingBounds - Visual: isVisualLeafType, isVisualOnlyNode, hasImageFill - Interactive: getInteractiveType, isInteractiveNode, INTERACTIVE_PATTERNS - Overlay/Carousel: isOverlayNode, isCarouselNode, OVERLAY_PATTERN, CAROUSEL_PATTERN - Token: hasStyleReference, hasBoundVariable - Naming: isDefaultName, isNonSemanticName, DEFAULT_NAME_PATTERNS, NON_SEMANTIC_NAMES - Exclusion: isExcludedName, EXCLUDED_NAME_PATTERN Old files (rule-exceptions.ts, excluded-names.ts) re-export for compatibility. Rule files import from node-semantics.ts instead of defining locally. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCentralizes node semantic predicates into Changes
Sequence Diagram(s)(Skipped — changes are primarily refactor/centralization of predicates and targeted rule logic tweaks; no new multi-component sequential flow requiring visualization.) 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 |
No users to maintain backward compatibility for. All consumers now import directly from node-semantics.ts. - Delete excluded-names.ts (was re-export only) - Remove re-exports from rule-exceptions.ts - Update orchestrator.ts and rule-exceptions.test.ts imports 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/interaction/index.ts`:
- Around line 144-147: The carousel nodes are being classified as "carousel" and
routed into missingPrototypeCheck(), which only recognizes click interactions
and thus wrongly flags properly prototyped sliders; change the classification so
carousels are treated like overlays by returning "overlay" for isCarouselNode
(i.e., in the block with isOverlayNode and isCarouselNode), or alternatively
extend the prototype-check logic in missingPrototypeCheck to accept carousel
interaction types — reference isCarouselNode, isOverlayNode, and
missingPrototypeCheck to locate and update the behavior.
In `@src/core/rules/node-semantics.ts`:
- Around line 65-68: isVisualOnlyNode currently treats any node with
hasImageFill(node) as visual-only and returns early, which misclassifies frames
that contain text/buttons and lets isAbsolutePositionExempt/isFixedSizeExempt
skip structural checks; update isVisualOnlyNode so that hasImageFill only marks
a node visual-only when the node has no non-visual children (e.g., no text nodes
or interactive controls). Concretely, change the logic in isVisualOnlyNode to
first inspect node.children and if children exist check whether any child type
is content-bearing (not in VISUAL_LEAF_TYPES) or isInteractive; only if there
are no content-bearing children should hasImageFill(node) cause a true return
(otherwise fall through to normal evaluation using VISUAL_LEAF_TYPES and
children.every). Ensure references: isVisualOnlyNode, hasImageFill,
VISUAL_LEAF_TYPES (and downstream behavior in
isAbsolutePositionExempt/isFixedSizeExempt) are considered.
🪄 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: db70b726-107a-4a11-8a68-c0d0fd527550
📒 Files selected for processing (7)
src/core/rules/excluded-names.tssrc/core/rules/interaction/index.tssrc/core/rules/naming/index.tssrc/core/rules/node-semantics.tssrc/core/rules/rule-exceptions.tssrc/core/rules/structure/index.tssrc/core/rules/token/index.ts
Comment on lines
+157
to
+160
| export const EXCLUDED_NAME_PATTERN = /(badge|close|dismiss|overlay|float|fab|dot|indicator|corner|decoration|tag|status|notification|icon|ico|image|asset|filter|dim|dimmed|bg|background|logo|avatar|divider|separator|nav|navigation|gnb|header|footer|sidebar|toolbar|modal|dialog|popup|toast|tooltip|dropdown|menu|sticky|spinner|loader|cursor|cta|chatbot|thumb|thumbnail|tabbar|tab-bar|statusbar|status-bar)/i; | ||
|
|
||
| export function isExcludedName(name: string): boolean { | ||
| return EXCLUDED_NAME_PATTERN.test(name); |
Contributor
There was a problem hiding this comment.
Don't use the shared exclusion regex to hide interactive controls.
This pattern is no longer naming-only: src/agents/orchestrator.ts uses isExcludedName() as a hard filter on Lines 188-190, and src/core/rules/rule-exceptions.ts uses it for layout exemptions. Entries like cta and filter will now suppress analysis of real controls instead of just decorative nodes.
Owner
Author
There was a problem hiding this comment.
Keeping as-is. isExcludedName is not used in interaction rules at all — interaction detection uses getInteractiveType() which is an independent classification. The exclusion pattern only affects naming rules and absolute-position exemptions, which is the intended scope.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check all usages of isExcludedName across the codebase
rg -n "isExcludedName" --type ts -C3
Length of output: 4553
@let-sunny, you're right — I confirmed via the codebase search that isExcludedName is only called in:
src/core/rules/naming/index.ts(naming rule guards)src/core/rules/rule-exceptions.ts(auto-layout exemptions)src/agents/orchestrator.ts(decorative node filtering)
It is not referenced anywhere in src/core/rules/interaction/index.ts, and getInteractiveType() operates entirely independently through INTERACTIVE_PATTERNS. My original concern was unfounded. Sorry for the noise!
✏️ Learnings added
Learnt from: let-sunny
Repo: let-sunny/canicode PR: 161
File: src/core/rules/node-semantics.ts:157-160
Timestamp: 2026-03-29T02:11:22.940Z
Learning: In let-sunny/canicode, `isExcludedName()` from `src/core/rules/node-semantics.ts` is intentionally NOT used in interaction rules. Interaction detection uses `getInteractiveType()` (via `INTERACTIVE_PATTERNS`) as an independent classification path. `isExcludedName` only affects: naming rules (`src/core/rules/naming/index.ts`), auto-layout exemptions (`src/core/rules/rule-exceptions.ts`), and orchestrator-level node filtering (`src/agents/orchestrator.ts`). Do not flag `cta`, `filter`, or other terms in `EXCLUDED_NAME_PATTERN` as suppressing interactive control analysis.
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/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: 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.
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/**/rules/rule-config.ts : Component-related rule scores (missing-component, etc.) should NOT be lowered based on small fixture calibration
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/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
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`).
- isVisualOnlyNode: image fill no longer short-circuits when node has content children (text, buttons). Only visual-only if children are all visual leaf types. - Carousel prototype check: now accepts ON_DRAG in addition to ON_CLICK (carousels are swipe-based, not click-based). - EXCLUDED_NAME_PATTERN: kept as-is — not used in interaction rules. 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/interaction/index.ts`:
- Around line 181-186: The inline docs and fix text for the missing-prototype
guidance were not updated after adding ON_DRAG to the carousel triggers; update
any JSDoc/inline comments and the user-facing remediation text (the logic that
constructs the "add click interaction" message) to reflect PROTOTYPE_TRIGGERS
and DEFAULT_TRIGGERS so carousels suggest adding ON_DRAG (or the appropriate
trigger(s)) rather than always saying "add click interactions"; locate
references to PROTOTYPE_TRIGGERS, DEFAULT_TRIGGERS and the missingPrototype
remediation function/messages and make the remediation message dynamic based on
PROTOTYPE_TRIGGERS['carousel'] (falling back to DEFAULT_TRIGGERS).
In `@src/core/rules/node-semantics.ts`:
- Around line 102-104: isInteractiveNode currently only checks
getInteractiveType(node) and therefore returns false for nodes your module
classifies as overlay or carousel; update it to either include those predicates
or be renamed. Fix option A: broaden isInteractiveNode to return true when
getInteractiveType(node) !== null OR when the node matches the overlay/carousel
predicates (e.g., add checks against the existing overlay/carousel helpers such
as isOverlayNode and isCarouselNode) so callers get a true “interactive” for all
three categories; Fix option B: if you want a narrower helper, rename
isInteractiveNode to something explicit like isExplicitInteractiveNode and
update all callers to use the correct broader predicate or the new name. Ensure
you update all imports/usages of isInteractiveNode to reflect the change.
- Around line 65-76: Add a focused regression test that asserts isVisualOnlyNode
returns false for a node with an image fill plus a content child (e.g., node
object with fills including IMAGE and children array containing a TEXT child),
so the image-fill plus content-child branch guarded by hasImageFill(...) is
covered; implement the test in src/core/rules/rule-exceptions.test.ts and
reference the isVisualOnlyNode function and hasImageFill helper to construct the
node and assert false.
🪄 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: 91f9d265-a93b-4529-9f5f-acc0946c0dbd
📒 Files selected for processing (6)
src/agents/orchestrator.tssrc/core/rules/excluded-names.tssrc/core/rules/interaction/index.tssrc/core/rules/node-semantics.tssrc/core/rules/rule-exceptions.test.tssrc/core/rules/rule-exceptions.ts
💤 Files with no reviewable changes (1)
- src/core/rules/excluded-names.ts
Comment on lines
+181
to
+186
| /** Trigger types to check per subType */ | ||
| const PROTOTYPE_TRIGGERS: Record<string, string[]> = { | ||
| carousel: ["ON_CLICK", "ON_DRAG"], | ||
| }; | ||
|
|
||
| const DEFAULT_TRIGGERS = ["ON_CLICK"]; |
Contributor
There was a problem hiding this comment.
Update the missing-prototype guidance for drag-supported carousels.
missingPrototype now accepts ON_DRAG for carousel, but the inline docs and surfaced fix text still tell users to add click interactions. That leaves the remediation inaccurate for the new subtype.
📝 Suggested wording update
-/** Interactive types that need click prototype */
+/** Interactive types that need prototype interactions */
const PROTOTYPE_TYPES: Record<InteractiveType, MissingPrototypeSubType> = {
button: "button",
link: "navigation",
tab: "tab",
input: "input",
toggle: "toggle",
};
@@
const missingPrototypeDef: RuleDefinition = {
id: "missing-prototype",
name: "Missing Prototype",
category: "interaction",
- why: "Interactive elements without click prototypes give AI no information about navigation or behavior on click",
- impact: "AI cannot generate click handlers, routing, or state changes — interactive elements become static",
- fix: "Add ON_CLICK prototype interactions to define navigation targets or state changes",
+ why: "Interactive elements without prototype interactions give AI no information about navigation or behavior",
+ impact: "AI cannot generate navigation, gesture handlers, or state changes — interactive elements become static",
+ fix: "Add prototype interactions (for example ON_CLICK, or ON_DRAG for carousels) to define navigation targets or state changes",
};As per coding guidelines, If the code change introduces behavior that contradicts existing documentation (README.md, CLAUDE.md, JSDoc comments), flag it and suggest updating the relevant documentation to stay in sync.
Also applies to: 188-195, 209-211
🤖 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 181 - 186, The inline docs
and fix text for the missing-prototype guidance were not updated after adding
ON_DRAG to the carousel triggers; update any JSDoc/inline comments and the
user-facing remediation text (the logic that constructs the "add click
interaction" message) to reflect PROTOTYPE_TRIGGERS and DEFAULT_TRIGGERS so
carousels suggest adding ON_DRAG (or the appropriate trigger(s)) rather than
always saying "add click interactions"; locate references to PROTOTYPE_TRIGGERS,
DEFAULT_TRIGGERS and the missingPrototype remediation function/messages and make
the remediation message dynamic based on PROTOTYPE_TRIGGERS['carousel'] (falling
back to DEFAULT_TRIGGERS).
- Update carousel message: mention ON_DRAG alongside ON_CLICK - Update missing-prototype rule definition: generic "prototype interactions" - Add regression tests for image-fill + content children (not visual-only) - isInteractiveNode scope: kept as-is — distinct from overlay/carousel Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
InteractiveType → StatefulComponentType getInteractiveType → getStatefulComponentType isInteractiveNode → isStatefulComponent INTERACTIVE_PATTERNS → STATEFUL_PATTERNS "Interactive" was too broad — overlay/carousel are also interactive but don't have CSS states. "StatefulComponent" precisely means components with expected state variants (hover/active/disabled/focus). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/core/rules/interaction/index.ts (1)
188-195: 🧹 Nitpick | 🔵 TrivialMinor inconsistency in impact text.
The
whyandfixfields were updated to reference generic "prototype interactions" and list bothON_CLICKandON_DRAG, but theimpactfield on line 193 still mentions "click handlers" specifically. Consider updating for consistency:why: "Interactive elements without prototype interactions give AI no information about navigation or behavior", - impact: "AI cannot generate click handlers, routing, or state changes — interactive elements become static", + impact: "AI cannot generate interaction handlers, routing, or state changes — interactive elements become static", fix: "Add prototype interactions (ON_CLICK, ON_DRAG) to define navigation targets or state changes",🤖 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 188 - 195, Update the impact text in the missingPrototypeDef RuleDefinition to match the generic "prototype interactions" language used in why and fix: change the reference from "click handlers" to "prototype interactions (e.g., ON_CLICK, ON_DRAG)" so the impact mentions inability to generate prototype interactions, routing, or state changes; update the impact field on the missingPrototypeDef object accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/core/rules/interaction/index.ts`:
- Around line 188-195: Update the impact text in the missingPrototypeDef
RuleDefinition to match the generic "prototype interactions" language used in
why and fix: change the reference from "click handlers" to "prototype
interactions (e.g., ON_CLICK, ON_DRAG)" so the impact mentions inability to
generate prototype interactions, routing, or state changes; update the impact
field on the missingPrototypeDef object accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b0a7b6ec-9d1e-4b20-a294-1afc866bf634
📒 Files selected for processing (2)
src/core/rules/interaction/index.tssrc/core/rules/node-semantics.ts
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 29, 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
노드 시맨틱 분류 함수/패턴이 4개 파일에 분산 →
node-semantics.ts하나로 통합.re-export shim 제거,
excluded-names.ts삭제.node-semantics.ts 구조
주요 변경
node-semantics.ts로 통합excluded-names.ts삭제,rule-exceptions.ts에서 re-export 제거 (사용자 없으므로 호환성 불필요)InteractiveType→StatefulComponentType,getInteractiveType→getStatefulComponentType,isInteractiveNode→isStatefulComponent(overlay/carousel과 혼동 방지)isVisualOnlyNode— image fill + content children 있는 프레임이 visual-only로 잘못 분류되던 것 수정Test plan
pnpm lint— cleanpnpm test:run— 550/550 passRelated
🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Chores