Conversation
| @@ -0,0 +1,215 @@ | |||
| # CSS Variable Analyzer | |||
There was a problem hiding this comment.
The skill files are at plugins/pf-design-tokens/css-var-analyzer/ but need to be under plugins/pf-design-tokens/skills/css-var-analyzer/ to be discovered as a skill.
| @@ -0,0 +1,470 @@ | |||
| #!/usr/bin/env node | |||
There was a problem hiding this comment.
Not all users will have Node.js installed, and if they don't, these will fail silently with command not found.
Bash scripts are more portable since every user has bash. If keeping Node, adding a runtime check at the top would help. I'll be updating CONTRIBUTING-SKILLS.md separately to clarify our policy on scripts in skills.
srambach
force-pushed
the
css-variable-analysis
branch
from
1dd3c64 to
1632464
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
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 12 minutes and 58 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: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces a new Changes
Sequence DiagramsequenceDiagram
actor User
participant Bash Wrapper
participant Analyzer
participant FileSystem as File System
participant Formatter
User->>Bash Wrapper: analyze-css-vars.sh analyze Component1
activate Bash Wrapper
Bash Wrapper->>Analyzer: css-var-analyzer.js Component1
deactivate Bash Wrapper
activate Analyzer
Analyzer->>FileSystem: Discover SCSS files<br/>(PascalCase conversion)
FileSystem-->>Analyzer: File list
loop For each file
Analyzer->>FileSystem: Read & parse SCSS
FileSystem-->>Analyzer: File content
Analyzer->>Analyzer: Extract definitions,<br/>usages, context
end
Analyzer->>Analyzer: Build redefinition chains<br/>Compute issues<br/>(undefined/unused/naming)
Analyzer-->>User: JSON output<br/>(result + metrics)
deactivate Analyzer
User->>Bash Wrapper: analyze-css-vars.sh format output.json Component1
activate Bash Wrapper
Bash Wrapper->>Formatter: format-css-report.js output.json...
deactivate Bash Wrapper
activate Formatter
Formatter->>Formatter: Select formatter<br/>(summary/variable/modifier)
Formatter->>Formatter: Transform JSON→Markdown<br/>Filter/rank/format data
Formatter-->>User: Markdown report
deactivate Formatter
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (4)
plugins/pf-styling/skills/css-var-analyzer/README.md (2)
9-19: Add language specifier to fenced code blocks.Static analysis flags these blocks for missing language specifiers. Since these are natural language prompts, use
textor leave empty with just triple backticks (thoughtextsilences the linter).📝 Proposed fix
-``` +```text Analyze CSS variables for the tabs component-
+text
Show me the redefinition chain for the inset variable in tabs-``` +```text Show me variables for the vertical modifier in tabs</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@plugins/pf-styling/skills/css-var-analyzer/README.mdaround lines 9 - 19,
The fenced code blocks in README.md
(plugins/pf-styling/skills/css-var-analyzer/README.md) are missing language
specifiers; update each triple-backtick block containing the natural-language
prompts to use a language tag such as "text" (e.g., replacewithtext for
the three prompt blocks) so the linter stops flagging them and rendering stays
correct.</details> --- `30-35`: **Consider adding the bash wrapper to the Files section.** The `scripts/analyze-css-vars.sh` wrapper is the actual CLI entrypoint per SKILL.md documentation but isn't listed here. Users may benefit from knowing about it. <details> <summary>📝 Proposed addition</summary> ```diff ## Files - **SKILL.md** - Skill definition and instructions +- **scripts/analyze-css-vars.sh** - CLI wrapper with Node.js validation - **css-var-analyzer.js** - SCSS parsing and analysis engine - **format-css-report.js** - Markdown report formatter ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@plugins/pf-styling/skills/css-var-analyzer/README.md` around lines 30 - 35, Update the Files list in README.md to include the CLI wrapper script by adding an entry for scripts/analyze-css-vars.sh (the actual CLI entrypoint referenced in SKILL.md), briefly describing its purpose (e.g., "CLI wrapper entrypoint to invoke css-var-analyzer.js"); ensure the new line appears alongside the existing entries for SKILL.md, css-var-analyzer.js, and format-css-report.js so users can find the wrapper easily. ``` </details> </blockquote></details> <details> <summary>plugins/pf-styling/skills/css-var-analyzer/css-var-analyzer.js (1)</summary><blockquote> `102-104`: **Consider adding error handling for file read operations.** If a file exists but can't be read (permissions, etc.), `fs.readFileSync` will throw without a helpful message. Similar to the pattern at line 69-71, wrapping this in try/catch would improve robustness. <details> <summary>🛡️ Proposed improvement</summary> ```diff parseFile(filePath) { - const content = fs.readFileSync(filePath, 'utf8'); + let content; + try { + content = fs.readFileSync(filePath, 'utf8'); + } catch (error) { + console.error(`Error reading ${filePath}: ${error.message}`); + return; + } const lines = content.split('\n'); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@plugins/pf-styling/skills/css-var-analyzer/css-var-analyzer.js` around lines 102 - 104, The parseFile function uses fs.readFileSync to read file content without error handling; wrap the read operation in a try/catch (similar to the existing pattern around lines ~69-71) so that errors (permissions, unreadable file) are caught, logged with context (include filePath and the err.message), and either rethrown or handled gracefully (e.g., return null or an empty lines array) before proceeding to split into lines; update references in parseFile to handle the possible null/empty result. ``` </details> </blockquote></details> <details> <summary>plugins/pf-styling/skills/css-var-analyzer/SKILL.md (1)</summary><blockquote> `240-253`: **Add language specifier to the error output code block.** The error message example block at line 242 would benefit from a language specifier (e.g., `text` or `plaintext`) to satisfy linting. <details> <summary>📝 Proposed fix</summary> ```diff -``` +```text Error: This skill requires Node.js. Node.js was not found in your PATH. ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@plugins/pf-styling/skills/css-var-analyzer/SKILL.mdaround lines 240 - 253,
Update the fenced code block in SKILL.md that shows the Node.js missing error so
it includes a language specifier (e.g., usetext orplaintext instead of
plain ```); locate the block containing the "Error: This skill requires
Node.js." message and change the opening backticks to include the specifier to
satisfy the linter.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@plugins/pf-styling/skills/css-var-analyzer/css-var-analyzer.js:
- Around line 396-401: The returned object includes Map properties
(this.redefinitions and this.scssVariables) which JSON.stringify serializes as
empty objects; convert those Maps to plain objects before returning so
downstream formatting can see their entries — replace uses of this.redefinitions
and this.scssVariables in the return value with
Object.fromEntries(this.redefinitions) and
Object.fromEntries(this.scssVariables) (or an equivalent Map-to-plain-object
helper) so the redefinitions and scssVariables data survive JSON serialization.- Around line 81-84: The current name normalization in the loop over
componentNames (building componentDir from name via name.charAt(0).toUpperCase()
- name.slice(1).toLowerCase()) mangles multi-word component names (e.g.,
"datalist" → "Datalist" instead of "DataList") and silently skips missing
directories; update the normalization in css-var-analyzer.js to robustly map
inputs to PatternFly component folders by: accept and normalize kebab-case and
PascalCase (e.g., convert "data-list" or "datalist" to "DataList") before
building componentPath (use a function like normalizeComponentName that splits
on hyphens or camelCase boundaries and capitalizes each segment), then check
fs.existsSync(componentPath) and log an explicit error via the same logger when
the directory is not found; alternatively (or additionally) document the
accepted input formats in SKILL.md so callers know expected casing.In
@plugins/pf-styling/skills/css-var-analyzer/format-css-report.js:
- Around line 228-230: The filter for computing modifierDefs can throw if
def.selector is undefined; update the filter callback used on result.definitions
(the one that assigns modifierDefs) to first check that def.selector is a
string/non-null before calling .includes (e.g., guard with def.selector &&
def.selector.includes(modifierClass) or typeof def.selector === 'string' &&
def.selector.includes(modifierClass)) so undefined selectors are skipped and
behavior otherwise remains the same.- Line 293: Wrap the synchronous file read/parse around a try/catch so missing
files or invalid JSON produce a clear message instead of a raw stack trace:
catch errors from fs.readFileSync(jsonFile, 'utf8') and JSON.parse and handle
them (e.g., log a descriptive error mentioning jsonFile and the error.message,
then exit or rethrow). Update the code that assigns const data =
JSON.parse(fs.readFileSync(jsonFile, 'utf8')); to a guarded block that
distinguishes file-not-found (ENOENT) from JSON syntax errors where useful and
ensures downstream code only runs when data is valid.- Around line 74-89: The code assumes redefinitions is a Map but after JSON
parsing it's a plain object, causing .size and .entries() to fail; update the
block that references redefinitions in format-css-report.js to treat it as an
object: use Object.keys(redefinitions).length > 0 for the presence check and use
Object.entries(redefinitions).map(([varName, defs]) => ({ varName, count:
defs.length })) (or Object.entries(...).slice/sort) instead of
Array.from(redefinitions.entries()), preserving the same sorting and slicing
logic and the loop that builds the report.
Nitpick comments:
In@plugins/pf-styling/skills/css-var-analyzer/css-var-analyzer.js:
- Around line 102-104: The parseFile function uses fs.readFileSync to read file
content without error handling; wrap the read operation in a try/catch (similar
to the existing pattern around lines ~69-71) so that errors (permissions,
unreadable file) are caught, logged with context (include filePath and the
err.message), and either rethrown or handled gracefully (e.g., return null or an
empty lines array) before proceeding to split into lines; update references in
parseFile to handle the possible null/empty result.In
@plugins/pf-styling/skills/css-var-analyzer/README.md:
- Around line 9-19: The fenced code blocks in README.md
(plugins/pf-styling/skills/css-var-analyzer/README.md) are missing language
specifiers; update each triple-backtick block containing the natural-language
prompts to use a language tag such as "text" (e.g., replacewithtext for
the three prompt blocks) so the linter stops flagging them and rendering stays
correct.- Around line 30-35: Update the Files list in README.md to include the CLI
wrapper script by adding an entry for scripts/analyze-css-vars.sh (the actual
CLI entrypoint referenced in SKILL.md), briefly describing its purpose (e.g.,
"CLI wrapper entrypoint to invoke css-var-analyzer.js"); ensure the new line
appears alongside the existing entries for SKILL.md, css-var-analyzer.js, and
format-css-report.js so users can find the wrapper easily.In
@plugins/pf-styling/skills/css-var-analyzer/SKILL.md:
- Around line 240-253: Update the fenced code block in SKILL.md that shows the
Node.js missing error so it includes a language specifier (e.g., usetext orplaintext instead of plain ```); locate the block containing the "Error: This
skill requires Node.js." message and change the opening backticks to include the
specifier to satisfy the linter.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `e93a7384-30fa-4efa-819e-e5898bf125f9` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 37aa4b4b45399b83440d566151cd782f587abf3a and 1632464fa7ccd996b4a8a7556782017fb19186b3. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `plugins/pf-styling/skills/css-var-analyzer/README.md` * `plugins/pf-styling/skills/css-var-analyzer/SKILL.md` * `plugins/pf-styling/skills/css-var-analyzer/css-var-analyzer.js` * `plugins/pf-styling/skills/css-var-analyzer/format-css-report.js` * `plugins/pf-styling/skills/css-var-analyzer/scripts/analyze-css-vars.sh` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
plugins/pf-styling/skills/css-var-analyzer/format-css-report.js
Outdated
Show resolved
Hide resolved
plugins/pf-styling/skills/css-var-analyzer/format-css-report.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (1)
plugins/pf-styling/skills/css-var-analyzer/css-var-analyzer.js (1)
81-93:⚠️ Potential issue | 🟡 MinorMulti-word component name handling remains broken.
The normalization at line 83 still mangles multi-word PatternFly components (e.g., "datalist" → "Datalist" instead of "DataList", "simplelist" → "Simplelist" instead of "SimpleList"). Additionally, when a component directory doesn't exist, it silently skips without user feedback.
Per CONTRIBUTING-SKILLS.md, skill instructions should be "clear enough that any AI tool would produce the same quality output." Consider normalizing kebab-case input (e.g., "data-list" → "DataList") or documenting the expected input format in SKILL.md.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/pf-styling/skills/css-var-analyzer/css-var-analyzer.js` around lines 81 - 93, The code currently builds componentDir by capitalizing only the first character (componentDir = name.charAt(0).toUpperCase() + name.slice(1).toLowerCase()), which mangles multi-word names and silently skips missing dirs; change the normalization to split the input name on hyphens/underscores/spaces (e.g., name.split(/[-_\s]+/)), Capitalize each segment and join them (so "data-list" → "DataList", "simple-list" → "SimpleList"), use that result as componentDir when forming componentPath, and if fs.existsSync(componentPath) is false emit a clear warning (e.g., console.warn or a logger) including the original name and resolved componentPath so missing directories are visible.
🧹 Nitpick comments (1)
plugins/pf-styling/skills/css-var-analyzer/css-var-analyzer.js (1)
1-1: Skill directory should usepf-prefix for PatternFly-specific skills.This skill analyzes PatternFly component SCSS files, making it PatternFly-specific. Per coding guidelines, it should be named
pf-css-var-analyzerrather thancss-var-analyzer. The directory, SKILL.md frontmatter name, and any CLI references should all use thepf-css-var-analyzername consistently.As per coding guidelines:
plugins/*/+(skills|agents)/**: Use thepf-prefix on skill and agent names that are PatternFly-specific. Based on learnings: "Skills/agents that are PatternFly-specific use thepf-prefix (e.g.,pf-unit-test-generator)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/pf-styling/skills/css-var-analyzer/css-var-analyzer.js` at line 1, Rename the skill from css-var-analyzer to pf-css-var-analyzer everywhere: rename the skill directory, update the SKILL.md frontmatter "name" value, change any package.json "name" or "bin" entries and CLI command strings referencing "css-var-analyzer" to "pf-css-var-analyzer", and update any import/require or registration references that use css-var-analyzer (e.g., skill loader, skill registry, or CLI wrappers) so all filenames, metadata names, and command invocations consistently use the pf- prefix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@plugins/pf-styling/skills/css-var-analyzer/css-var-analyzer.js`:
- Around line 81-93: The code currently builds componentDir by capitalizing only
the first character (componentDir = name.charAt(0).toUpperCase() +
name.slice(1).toLowerCase()), which mangles multi-word names and silently skips
missing dirs; change the normalization to split the input name on
hyphens/underscores/spaces (e.g., name.split(/[-_\s]+/)), Capitalize each
segment and join them (so "data-list" → "DataList", "simple-list" →
"SimpleList"), use that result as componentDir when forming componentPath, and
if fs.existsSync(componentPath) is false emit a clear warning (e.g.,
console.warn or a logger) including the original name and resolved componentPath
so missing directories are visible.
---
Nitpick comments:
In `@plugins/pf-styling/skills/css-var-analyzer/css-var-analyzer.js`:
- Line 1: Rename the skill from css-var-analyzer to pf-css-var-analyzer
everywhere: rename the skill directory, update the SKILL.md frontmatter "name"
value, change any package.json "name" or "bin" entries and CLI command strings
referencing "css-var-analyzer" to "pf-css-var-analyzer", and update any
import/require or registration references that use css-var-analyzer (e.g., skill
loader, skill registry, or CLI wrappers) so all filenames, metadata names, and
command invocations consistently use the pf- prefix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1aabf7cd-977f-4b9e-b2cb-7d106e522654
📒 Files selected for processing (3)
plugins/pf-styling/skills/css-var-analyzer/SKILL.mdplugins/pf-styling/skills/css-var-analyzer/css-var-analyzer.jsplugins/pf-styling/skills/css-var-analyzer/format-css-report.js
✅ Files skipped from review due to trivial changes (1)
- plugins/pf-styling/skills/css-var-analyzer/SKILL.md
jpuzz0
left a comment
jpuzz0
left a comment
There was a problem hiding this comment.
Addressed coderabbitai comments. LGTM
2 participants
Add CSS Variable Analyzer Skill
Adds a new skill to the
pf-design-tokensplugin for analyzing CSS variable usage in PatternFly SCSS components.What it does
--#{$component}--property) instead of hardcoded namesUse cases
Example usage
Analyze CSS variables for the tabs component
Show me the redefinition chain for the inset variable in tabs
Show me variables for the vertical modifier in tabs
Files included
SKILL.md- Skill definition with instructions and examplescss-var-analyzer.js- SCSS parsing and analysis engineformat-css-report.js- Markdown report formatterREADME.md- Quick reference guideTested with PatternFly tabs component. Generates detailed markdown reports with statistics, issue detection, and redefinition chains.
Summary by CodeRabbit
New Features
Documentation