feat: non-standard-naming rule — detect dev-unfriendly state names (#159)#162
Merged
feat: non-standard-naming rule — detect dev-unfriendly state names (#159)#162
Conversation
#159) Flags variant state names that aren't in the web+mobile platform standard set (hover, active, pressed, selected, highlighted, disabled, focus, focused). Suggests the closest standard name (e.g., "Clicked" → "pressed", "Inactive" → "disabled"). This ensures missing-interaction-state can properly detect state variants after the designer fixes the names. Standard names defined in node-semantics.ts (STANDARD_STATE_NAMES) shared with interaction rules. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 49 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new minor rule Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Research verified against official specs: - CSS: :hover, :active, :focus, :disabled, :enabled (MDN) - Material Design 3: pressed, dragged, hovered, focused (m3.material.io) - UIKit: highlighted, selected, disabled, focused (Apple Developer) Added "enabled" and "dragged" to STANDARD_STATE_NAMES. Removed "enabled" from non-standard suggestions (it's a CSS standard). 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
🤖 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/naming/index.ts`:
- Around line 148-155: The user-facing "fix" text in the nonStandardNamingDef
RuleDefinition is out of sync with the rule allowlist; update the fix string on
nonStandardNamingDef so it lists all accepted state names (include enabled,
dragged and the suggested default) alongside the platform-standard names (hover,
active, pressed, selected, highlighted, disabled, focus, focused) so the
remediation text matches the rule behavior.
In `@src/core/rules/naming/non-standard-naming.test.ts`:
- Around line 82-93: The test case in non-standard-naming.test.ts that verifies
platform-standard variant names is missing the newly allowed "Enabled" and
"Dragged" values, so update the makeNode call in the "accepts platform-standard
names (pressed, selected, highlighted, focused)" test to include "Enabled" and
"Dragged" in componentPropertyDefinitions["State"].variantOptions, then run the
nonStandardNaming.check(node, ctx) expectation as-is to ensure the rule accepts
those names too.
- Around line 29-44: The test doesn't exercise "Inactive" because
nonStandardNaming.check stops at the first matching variant option; update the
test so the "Inactive" case is actually matched—either create a separate test
where componentPropertyDefinitions for "State" has variantOptions: ["Inactive"]
(or only ["Inactive"]), or reorder the array to ["Inactive", "On"] so
nonStandardNaming.check will return the disabled suggestion; ensure the test
still calls nonStandardNaming.check(node, ctx) and asserts result!.message
contains "Inactive" and "disabled"/"disabled" suggestion accordingly.
In `@src/core/rules/node-semantics.ts`:
- Around line 134-146: STATE_NAME_SUGGESTIONS currently maps "off" =>
"disabled", which misclassifies an interactive "off" state; update the mapping
in the STATE_NAME_SUGGESTIONS object so that the key "off" maps to "default"
instead of "disabled" (update the string value in the exported constant to
"default" and run any affected tests/linting).
🪄 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: e382081b-d79c-40c1-96c8-de04a593cb1d
📒 Files selected for processing (8)
docs/REFERENCE.mdsrc/core/contracts/rule.tssrc/core/engine/scoring.test.tssrc/core/rules/naming/index.tssrc/core/rules/naming/non-standard-naming.test.tssrc/core/rules/node-semantics.tssrc/core/rules/rule-config.tssrc/core/rules/rule-messages.ts
- off → default (not disabled — off is toggle's base state) - Fix text updated with full standard name list including enabled/dragged - Separate test for Inactive → disabled (was hidden behind On) - Added enabled/dragged acceptance test 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/naming/index.ts`:
- Around line 157-192: The rule nonStandardNamingCheck uses node.name when
building the message (via nonStandardNamingMsg.stateName) but AnalysisNode.name
may be undefined; update nonStandardNamingCheck to guard for missing names by
returning null early if node.name is falsy or by supplying a safe fallback
(e.g., "<unnamed>" or node.id) when calling nonStandardNamingMsg.stateName so
the message construction never receives undefined; ensure the change is applied
where the object with ruleId nonStandardNamingDef.id and subType "state-name" is
created.
In `@src/core/rules/node-semantics.ts`:
- Around line 106-118: There are two consecutive JSDoc blocks above the
STANDARD_STATE_NAMES constant; remove the duplicate by merging them into a
single JSDoc comment immediately preceding STANDARD_STATE_NAMES that contains
the brief description plus the detailed source references (CSS, Material Design
3, UIKit) so only one consolidated documentation block remains above the
STANDARD_STATE_NAMES definition.
🪄 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: 9be2127d-e140-456f-acbd-a8ea79426539
📒 Files selected for processing (3)
src/core/rules/naming/index.tssrc/core/rules/naming/non-standard-naming.test.tssrc/core/rules/node-semantics.ts
Comment on lines
+157
to
+192
| const nonStandardNamingCheck: RuleCheckFn = (node, context) => { | ||
| // Only check COMPONENT_SET (variant container) | ||
| if (node.type !== "COMPONENT_SET") return null; | ||
| if (!node.componentPropertyDefinitions) return null; | ||
|
|
||
| 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; | ||
|
|
||
| for (const opt of options) { | ||
| if (typeof opt !== "string") continue; | ||
| const lower = opt.toLowerCase().trim(); | ||
|
|
||
| // Skip if it's a standard name | ||
| if (STANDARD_STATE_NAMES.has(lower)) continue; | ||
|
|
||
| // Check if it matches a known non-standard state name | ||
| if (STATE_LIKE_PATTERN.test(opt)) { | ||
| const suggestion = STATE_NAME_SUGGESTIONS[lower]; | ||
| if (suggestion) { | ||
| return { | ||
| ruleId: nonStandardNamingDef.id, | ||
| subType: "state-name" as const, | ||
| nodeId: node.id, | ||
| nodePath: context.path.join(" > "), | ||
| message: nonStandardNamingMsg.stateName(node.name, opt, suggestion), | ||
| }; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| }; |
Contributor
There was a problem hiding this comment.
Guard against undefined node.name in message construction.
node.name is used at line 184 without a guard. While COMPONENT_SET nodes typically have names, the AnalysisNode type allows name to be undefined. Add an early return or provide a fallback.
Suggested fix
const nonStandardNamingCheck: RuleCheckFn = (node, context) => {
// Only check COMPONENT_SET (variant container)
if (node.type !== "COMPONENT_SET") return null;
+ if (!node.name) return null;
if (!node.componentPropertyDefinitions) return null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/rules/naming/index.ts` around lines 157 - 192, The rule
nonStandardNamingCheck uses node.name when building the message (via
nonStandardNamingMsg.stateName) but AnalysisNode.name may be undefined; update
nonStandardNamingCheck to guard for missing names by returning null early if
node.name is falsy or by supplying a safe fallback (e.g., "<unnamed>" or
node.id) when calling nonStandardNamingMsg.stateName so the message construction
never receives undefined; ensure the change is applied where the object with
ruleId nonStandardNamingDef.id and subType "state-name" is created.
Owner
Author
There was a problem hiding this comment.
Keeping as-is. AnalysisNode.name is z.string() (required) in the Zod schema at src/core/contracts/figma-node.ts:75. It cannot be undefined at runtime.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify AnalysisNode.name field definition in the Zod schema
rg -n "name" src/core/contracts/figma-node.ts -C2
Length of output: 554
@let-sunny, confirmed — Line 75 of src/core/contracts/figma-node.ts shows name: z.string() with no .optional() or .nullable(), so node.name is always a defined string by the time any rule check function is invoked. My comment was a false positive. Thanks for the clarification!
✏️ Learnings added
Learnt from: let-sunny
Repo: let-sunny/canicode PR: 162
File: src/core/rules/naming/index.ts:157-192
Timestamp: 2026-03-29T03:02:35.383Z
Learning: In let-sunny/canicode, `AnalysisNode.name` is declared as `name: z.string()` (required, non-optional) at line 75 of `src/core/contracts/figma-node.ts`. It is guaranteed to be a defined string at runtime for all nodes reaching rule check functions. Do not flag `node.name` usage in rule files as potentially undefined.
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: 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: 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.
Learnt from: let-sunny
Repo: let-sunny/canicode PR: 161
File: src/core/rules/node-semantics.ts:102-104
Timestamp: 2026-03-29T02:24:54.672Z
Learning: In let-sunny/canicode (`src/core/rules/node-semantics.ts`), `isInteractiveNode()` is intentionally narrow: it returns `true` only for state-variant interactive elements (button, link, tab, input, toggle) detected via `getInteractiveType()`. Overlay and carousel nodes are a separate semantic category called "behavior triggers" — they require prototype interactions but do NOT have hover/focus/disabled state variants. Do not suggest broadening `isInteractiveNode` to include overlay/carousel; the three categories (state-variant interactive, overlay, carousel) are siblings, not a hierarchy.
Learnt from: CR
Repo: let-sunny/canicode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T00:25:18.075Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Constants must use SCREAMING_SNAKE_CASE naming convention
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/rule-config.ts : `no-auto-layout` is the single highest-impact rule with score -10 — empirically validated via ablation experiments
Learnt from: CR
Repo: let-sunny/canicode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T00:25:18.075Z
Learning: Applies to **/*.{ts,tsx} : Types and Interfaces must use PascalCase naming convention
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 : If no `node-id` is provided to CLI, print a warning
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: 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 : Rule scores can be tuned in `rule-config.ts` without touching rule logic
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
variant state 이름이 웹+앱 플랫폼 표준과 안 맞으면 감지.
missing-interaction-state가 제대로 동작하려면 표준 이름이 필요.흐름
표준 이름 (STANDARD_STATE_NAMES)
default,hover,active,pressed,selected,highlighted,disabled,focus,focused비표준 → 제안 매핑 (STATE_NAME_SUGGESTIONS)
변경
non-standard-naming추가)node-semantics.ts—STANDARD_STATE_NAMES,STATE_NAME_SUGGESTIONS,STATE_LIKE_PATTERN공유 상수rule-messages.ts—nonStandardNamingMsg.stateNameRelated
Test plan
pnpm lint— cleanpnpm test:run— 559/559 pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests