fix: add ds token mapping #624
Conversation
✅ Deploy Preview for pine-design-system ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
bot
added
bug
|
@coderabbitai review this pr |
|
@QuintonJason I'll review this PR for you. This introduces ESLint and Stylelint plugins to enforce Pine Design System color token usage, which should help maintain consistency and dark mode compatibility across the codebase. ✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds linting tooling and docs to enforce Pine Design System color usage. New files: ESLint plugin 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @libs/core/lint-plugins/pine-color-utils.cjs:
- Around line 159-165: normalizeHex currently only expands 3-digit hex but
ignores 4- and 8-digit alpha forms; update normalizeHex to handle 3, 4, 6 and 8
character inputs by: strip leading '#', lowercase, if length===3 expand each
char to two (e.g. "abc" -> "aabbcc"), if length===4 expand each char to two
(e.g. "abcd" -> "aabbccdd") so alpha is preserved, if length===6 or 8 keep
as-is, and return `#` + the resulting full-length hex; mirror the same expansion
logic used in hexToRgb to ensure consistent handling of alpha channels.
- Around line 17-24: hexToRgb only supports 3- and 6-digit hexes so 4- and
8-digit hexes with alpha (e.g., "#fff0" or "#ffffff00") return null; update the
logic in hexToRgb (the fullHex computation and validation) to handle 4- and
8-digit forms by expanding shorthand 4-digit (RGBA -> RRGGBBAA) and for both 4-
and 8-digit inputs strip or ignore the alpha portion and use the first 6 hex
chars for RGB; ensure you still validate hex chars (0-9a-f) after normalization
and return the rgb values from those first 6 characters.
In @libs/core/lint-plugins/stylelint-plugin-pine-colors.cjs:
- Around line 255-280: The loop over hexMatches mutates decl.value inside the
loop (decl.value = decl.value.replace(...)), causing later iterations to operate
on an already-modified string and producing incorrect auto-fixes for multiple
hexes; instead, collect replacement pairs from getHexSuggestion (use
suggestionData.suggestion) while iterating hexMatches (and still call
stylelint.utils.report for non-auto-fix cases), then after the loop apply all
replacements in one pass to the original value (e.g., build newValue from the
original value or use a single regex replace based on the collected mapping) and
set decl.value once when context && context.fix is true; keep existing calls to
messages.rejectedHex and report unchanged.
In @libs/core/lint-plugins/stylelint-plugin-pine-semantic-tokens.cjs:
- Around line 206-217: The loop mutates decl.value per match which breaks
subsequent replacements when multiple coreTokenMatches exist; instead, collect
replacements (map original match.full to its replacement
`var(--pine-color-${primarySuggestion})`) while iterating coreTokenMatches and
after the loop, apply a single replacement pass to decl.value (or compute a new
value by replacing all originals with their replacements) only when context &&
context.fix and primarySuggestion(s) exist; reference the loop over
coreTokenMatches, getSuggestions(match.token, propertyContext),
primarySuggestion, and decl.value to implement the batched replacement pattern.
🧹 Nitpick comments (8)
libs/core/lint-plugins/README.md (1)
148-149: Clarify ESLint plugin status.The documentation notes that
eslint-plugin-pine-colors.cjsis "not currently enabled". For clarity, consider:
- Adding a note explaining why the ESLint plugin exists but is not enabled
- Indicating when/if the ESLint plugin will be enabled (e.g., in a future PR)
- Documenting whether the ESLint plugin should be avoided or is pending configuration elsewhere
This will help future maintainers understand the intentionality vs. incompleteness of the implementation.
libs/core/lint-plugins/stylelint-plugin-pine-colors.cjs (2)
26-33: Silent error swallowing may hide configuration issues.The catch block silently continues without logging any diagnostic information. When debugging why token mappings aren't loading, there's no way to know which paths were tried or what errors occurred.
🔧 Suggested improvement
for (const tryLoad of possiblePaths) { try { tokenMappings = tryLoad(); break; } catch (e) { - // Continue to next path + // Continue to next path (uncomment for debugging) + // console.debug(`Failed to load token mappings: ${e.message}`); } }
258-262: Comment detection logic is fragile and may miss edge cases.The basic
/* */counting approach doesn't handle nested comments, single-line comments (//), or CSS comment edge cases. Consider that SCSS supports//comments which this logic ignores entirely.libs/core/lint-plugins/stylelint-plugin-pine-semantic-tokens.cjs (2)
11-39: Significant code duplication with stylelint-plugin-pine-colors.cjs.The token loading logic (lines 11-39),
getPropertyContext(lines 46-72),getContextTokens(lines 96-99), andgetSuggestions(lines 104-135) are nearly identical to their counterparts instylelint-plugin-pine-colors.cjs. Consider extracting these shared utilities topine-color-utils.cjsto reduce maintenance burden and ensure consistent behavior.
238-239: Missingmessagesexport for consistency with sibling plugin.
stylelint-plugin-pine-colors.cjsexports bothruleNameandmessages, but this file only exportsruleName. For API consistency, consider adding:module.exports.ruleName = ruleName; +module.exports.messages = stylelint.utils.ruleMessages(ruleName, { + rejectedCore: (message) => message, +});libs/core/lint-plugins/pine-color-utils.cjs (1)
67-93: Module-level singleton cache may complicate testing.The
flattenedTokensCacheandlastHexToCoreare module-level variables. This works for production but could make unit testing harder since the cache persists across tests. Consider documenting this behavior or providing a cache-clearing mechanism for tests.libs/core/lint-plugins/eslint-plugin-pine-colors.cjs (2)
12-40: Token loading logic duplicated for the third time.This is the third copy of the token loading pattern (also in stylelint-plugin-pine-colors.cjs and stylelint-plugin-pine-semantic-tokens.cjs). Extract to a shared module in
pine-color-utils.cjs:// In pine-color-utils.cjs, add: function loadTokenMappings() { const possiblePaths = [ () => require('@kajabi-ui/styles/lint-mappings'), () => require(path.resolve(__dirname, '../../../../node_modules/@kajabi-ui/styles/dist/tokens/pine-token-mappings.json')), () => require(path.resolve(__dirname, '../../../../ds-tokens/packages/styles/dist/tokens/pine-token-mappings.json')), ]; for (const tryLoad of possiblePaths) { try { return tryLoad(); } catch (e) { /* continue */ } } console.error('Failed to load pine-token-mappings.json'); return { mappings: {}, ambiguous: {}, hexToCore: {}, docsUrl: '' }; } module.exports = { ..., loadTokenMappings };
540-546: Static template literal fix changes syntax from backticks to quotes.When fixing a
TemplateLiterallike`var(--pine-color-grey-900)`, the fixer at line 525 converts it to a single-quoted string'var(--pine-color-text)'. This is semantically correct for static strings but changes the code style. Consider preserving the template literal syntax:🔧 Suggested improvement
fix: primarySuggestion ? (fixer) => { const newValue = value.replace( match.full, `var(--pine-color-${primarySuggestion})` ); if (node.type === 'Literal') { const quote = context.getSourceCode().getText(node)[0]; return fixer.replaceText(node, `${quote}${newValue}${quote}`); } - return fixer.replaceText(node, `'${newValue}'`); + if (node.type === 'TemplateLiteral') { + return fixer.replaceText(node, `\`${newValue}\``); + } + return fixer.replaceText(node, `'${newValue}'`); } : null,
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
libs/.stylelintrc.jsonlibs/core/.eslintrc.jsonlibs/core/lint-plugins/README.mdlibs/core/lint-plugins/eslint-plugin-pine-colors.cjslibs/core/lint-plugins/pine-color-utils.cjslibs/core/lint-plugins/stylelint-plugin-pine-colors.cjslibs/core/lint-plugins/stylelint-plugin-pine-semantic-tokens.cjs
🧰 Additional context used
🧬 Code graph analysis (1)
libs/core/lint-plugins/stylelint-plugin-pine-semantic-tokens.cjs (1)
libs/core/lint-plugins/stylelint-plugin-pine-colors.cjs (4)
stylelint(9-9)require(10-10)path(14-14)possiblePaths(17-24)
🔇 Additional comments (7)
libs/core/.eslintrc.json (1)
18-18: Formatting change approved.Adding a trailing newline aligns with common code style conventions.
libs/core/lint-plugins/README.md (3)
1-62: Clear and comprehensive documentation for linting rules.The documentation effectively explains the purpose of semantic tokens, blocked patterns, and token mapping rationale. The tables, examples, and token reference are well-organized and should help developers understand the design system constraints.
119-127: The npm lint scripts referenced in the documentation are properly defined and valid. Thelint.stylesscript exists inlibs/core/package.jsonasstylelint "./src/**/*.*css", and the--fixflag is a standard stylelint option, makingnpm run lint.styles -- --fixa valid invocation. No changes needed to this documentation section.
138-140: No action needed—documentation is accurate.The README correctly states that token mappings are loaded from
@kajabi-ui/styles/lint-mappings. All three lint plugins (eslint-plugin-pine-colors.cjs, stylelint-plugin-pine-colors.cjs, stylelint-plugin-pine-semantic-tokens.cjs) implement this exact pattern, with fallback paths for development scenarios. The plugins gracefully handle missing mappings and expect the structure{ mappings, ambiguous, hexToCore, docsUrl }, which is properly initialized when loading succeeds or fails.Likely an incorrect or invalid review comment.
libs/.stylelintrc.json (1)
7-13: All plugin files and rule exports have been verified. Bothstylelint-plugin-pine-colors.cjsandstylelint-plugin-pine-semantic-tokens.cjsexist at the specified paths, properly export their rules under thepine-design-systemnamespace (no-hardcoded-colorsandprefer-semantic-tokensrespectively), and are valid CommonJS modules. The relative paths in the configuration are correct.libs/core/lint-plugins/eslint-plugin-pine-colors.cjs (2)
374-396: LGTM!The delegation pattern where
Literaldefers to specific handlers (Property,VariableDeclarator,JSXAttribute) for their respective parent types correctly avoids double-reporting while maintaining accurate context detection.
398-415: No auto-fix for template literals is acceptable but worth noting.Template literals don't receive auto-fix due to their complexity (expressions, quasis). This is a reasonable limitation. Consider adding a comment documenting this intentional omission for maintainability.
|
@coderabbitai review this PR |
|
@QuintonJason I'll review the PR changes now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/core/lint-plugins/stylelint-plugin-pine-semantic-tokens.cjs (1)
239-246: Consider using.replaceAll()or.split().join()for more robust replacements.The loop applies replacements sequentially using
.replace(), which only replaces the first occurrence in each iteration. While this works for duplicate tokens due to sequential application, it's fragile and inefficient. For improved clarity and robustness, consider:// Apply all replacements in one pass when fixing if (context && context.fix && replacements.length > 0) { let newValue = value; for (const { original, replacement } of replacements) { // Replace all occurrences, not just the first newValue = newValue.split(original).join(replacement); } decl.value = newValue; }Alternatively, deduplicate the replacements array or use
.replaceAll()if the minimum Node version supports it.libs/core/lint-plugins/stylelint-plugin-pine-colors.cjs (1)
289-296: Consider using.replaceAll()or.split().join()for more robust replacements.The loop applies replacements sequentially using
.replace(), which only replaces the first occurrence in each iteration. While this works for duplicate hex colors due to sequential application, it's fragile and inefficient. For improved clarity and robustness, consider:// Apply all replacements in one pass when fixing if (context && context.fix && replacements.length > 0) { let newValue = value; for (const { original, replacement } of replacements) { // Replace all occurrences, not just the first newValue = newValue.split(original).join(replacement); } decl.value = newValue; }Alternatively, deduplicate the replacements array or use
.replaceAll()if the minimum Node version supports it.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/core/lint-plugins/pine-color-utils.cjslibs/core/lint-plugins/stylelint-plugin-pine-colors.cjslibs/core/lint-plugins/stylelint-plugin-pine-semantic-tokens.cjs
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/core/lint-plugins/pine-color-utils.cjs
🧰 Additional context used
🧬 Code graph analysis (1)
libs/core/lint-plugins/stylelint-plugin-pine-semantic-tokens.cjs (2)
libs/core/lint-plugins/stylelint-plugin-pine-colors.cjs (4)
stylelint(9-9)require(10-10)path(14-14)possiblePaths(17-24)libs/core/lint-plugins/eslint-plugin-pine-colors.cjs (4)
require(12-12)path(16-16)possiblePaths(19-26)regex(85-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test-specs (22)
- GitHub Check: test-specs (20)
- GitHub Check: test-specs (18)
🔇 Additional comments (15)
libs/core/lint-plugins/stylelint-plugin-pine-semantic-tokens.cjs (6)
11-39: LGTM! Resilient token mapping loading strategy.The fallback chain (published package → node_modules → local sibling repo → empty default) handles development and production scenarios well, and the error logging provides visibility when mappings fail to load.
46-72: LGTM! Comprehensive context detection.The function correctly categorizes CSS properties into background, border, and text contexts, with appropriate handling of various property name patterns and edge cases like
box-shadow.
74-91: LGTM! Correct core token detection.The regex pattern accurately matches all core tokens (grey-*, white, black) and returns structured match data for downstream processing.
93-135: LGTM! Robust suggestion logic with comprehensive fallbacks.The suggestion logic intelligently prioritizes context-specific mappings, falls back to ambiguous tokens, and searches across all contexts when needed, ensuring users always receive helpful guidance.
137-181: LGTM! Excellent error message formatting.The error messages are comprehensive and actionable, providing context-aware suggestions, alternative options with usage hints, documentation links, and escape hatch syntax for intentional violations.
183-249: Past review concern has been addressed.The previous mutation issue (mutating
decl.valueinside the loop) has been resolved by collecting all replacements in an array first, then applying them in a separate step. This ensures correct behavior when multiple core tokens are present.libs/core/lint-plugins/stylelint-plugin-pine-colors.cjs (9)
9-43: LGTM! Consistent setup and imports.The token loading strategy matches the semantic tokens plugin (good consistency), and the rule messages setup provides flexibility for custom error message formatting.
52-80: LGTM! Context detection is sound.The property context categorization covers the relevant CSS properties. Minor note: Line 69 uses
includes()checks rather thanstartsWith()+endsWith(), which is slightly less precise but functionally equivalent for well-formed CSS property names.
82-124: LGTM! Consistent suggestion logic.The helper functions match the semantic tokens plugin implementation, providing the same robust fallback strategy and comprehensive suggestion logic.
129-180: LGTM! Nuanced error messages for hex colors.The error message builder provides graduated feedback based on color distance (exact, close, warning, far), helping users understand the severity of the mismatch and make informed decisions about replacements.
185-212: LGTM! Safe auto-fix logic with distance checks.The suggestion logic appropriately prevents auto-fixing colors that are too far from Pine tokens (
isFarcheck), and only enables auto-fix for exact or close matches, reducing the risk of incorrect replacements.
238-241: LGTM! Good optimization to skip already-conforming declarations.The early return when
var(--pine-color-is detected prevents unnecessary processing of declarations already using Pine tokens.
261-264: LGTM! Reasonable comment detection heuristic.The basic comment detection (counting
/*and*/before the hex color) provides a reasonable heuristic to skip colors within comments, avoiding false positives.
255-297: Past review concern has been addressed.The previous mutation issue (mutating
decl.valueinside the loop) has been resolved by collecting all replacements in an array first, then applying them in a separate step. This ensures correct behavior when multiple hex colors are present.
301-302: LGTM! Standard Stylelint plugin exports.The exports follow Stylelint plugin conventions by exposing both the rule name and messages object for external reference.
| @@ -0,0 +1,155 @@ | |||
| # Pine Design System Color Linting Rules | |||
There was a problem hiding this comment.
Make a docs page
Description
Introduces ESLint and Stylelint plugins to enforce Pine Design System color token usage across JavaScript/TypeScript and SCSS files. This helps ensure consistent theming, dark mode compatibility, and design system adherence.
Fixes DSS-60
Type of change
How Has This Been Tested?
In the terminal
Test Configuration:
Checklist:
If not applicable, leave options unchecked.