CI Step to run build scripts #561
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request includes changes to enhance accessibility linting in the ESLint configuration by adding multiple rules from the Changes
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
.eslintrc.cjsOops! Something went wrong! :( ESLint: 8.56.0 Error: Failed to load parser '@babel/eslint-parser' declared in '.eslintrc.cjs': Cannot find module '@babel/core/package.json'
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
.eslintrc.cjs (2)
11-11: Consider configuration redundancy with recommended presetYou're extending
plugin:jsx-a11y/recommendedwhile also explicitly defining individual jsx-a11y rules. This could lead to rule conflicts or make it harder to maintain. Consider either:
- Using just the recommended preset and overriding specific rules as needed, or
- Removing the recommended preset and keeping explicit rule definitions for full control
Also applies to: 22-22
81-81: Consider configuring label-has-associated-control ruleThe 'label-has-associated-control' rule often needs configuration to work effectively with custom components.
- 'jsx-a11y/label-has-associated-control': 'warn', + 'jsx-a11y/label-has-associated-control': ['warn', { + labelComponents: ['CustomLabel'], + labelAttributes: ['label'], + controlComponents: ['CustomInput'], + depth: 3, + }],package.json (2)
51-53: Consider making prebuild tests optionalWhile running tests before builds adds safety, it might slow down development iterations. Consider:
- Making the test step optional via an environment variable
- Only running tests in CI, not local development
Example implementation:
- "prebuild": "npm run test && npm run clean", + "prebuild": "npm run clean && [ \"$CI\" = \"true\" ] && npm run test || true",
Build outputs missing and exports need verification
The verification process has revealed critical issues:
- The
distdirectory and its components are missing, yet they are referenced in package.json exports- The build outputs specified in exports (e.g.,
./dist/components/Accordion.js) don't exist- We need to verify the actual component source files as the current search only revealed test and story files
🔗 Analysis chain
Line range hint
6-34: Verify component build outputs match exportsLet's ensure the build process generates all exported components and that no components are missing from exports.
Also applies to: 54-54
The previous results showed only test and story files. Let's verify the actual component source files and check the dist directory to better understand the build outputs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify component exports match built files # Extract component paths from exports echo "Checking exports consistency..." jq -r '.exports | to_entries[] | select(.key | startswith("./") and endswith(".js")) | .value' package.json | sort > /tmp/exported_components.txt # Find actual component files echo "Checking built components..." fd -t f -e js . src/components/ | sort > /tmp/source_components.txt # Compare lists echo "Comparing source components with exports..." diff /tmp/source_components.txt /tmp/exported_components.txt # Verify build outputs echo "Verifying build outputs..." while IFS= read -r component; do if [[ ! -f "${component#./}" ]]; then echo "Missing build output for: $component" fi done < /tmp/exported_components.txtLength of output: 2719
Script:
#!/bin/bash # Let's check the actual component source files, not just stories and tests fd -t f -e jsx -e js . src/components/ui/ | grep -v "stories\|test" | sort # Also check if there are any components in dist echo -e "\nChecking dist directory contents:" fd -t f -e js . dist/components/ | sortLength of output: 305
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
docs/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.eslintrc.cjs(3 hunks).github/workflows/build-rad-ui.yml(1 hunks)docs/package.json(1 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/package.json
🧰 Additional context used
🪛 yamllint
.github/workflows/build-rad-ui.yml
[error] 13-13: trailing spaces
(trailing-spaces)
[error] 19-19: trailing spaces
(trailing-spaces)
[error] 22-22: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
.eslintrc.cjs (1)
68-97: Verify ESLint configuration in CI pipeline
Since this PR adds CI build steps, verify that these new ESLint rules won't cause unexpected build failures.
✅ Verification successful
Let me try a different approach to verify the ESLint configuration since npx is not available in the sandbox environment.
ESLint configuration is safe for CI builds
The ESLint configuration is properly set up and won't cause CI build failures because:
- All jsx-a11y rules are set to 'warn' level
- No rules are configured at 'error' level
- No overrides are present that could escalate rule severity
- Most other rules are either set to 'warn' or 'off'
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing ESLint errors in the codebase that might affect CI
# Find all JavaScript/TypeScript files
echo "Checking for files that might be affected by new ESLint rules..."
fd -e js -e jsx -e ts -e tsx | while read -r file; do
# Count potential accessibility issues
echo "Checking $file..."
npx eslint --no-eslintrc --config .eslintrc.cjs "$file" -f json \
| jq -r '.[] | select(.messages | length > 0) | "\(.filePath): \(.messages | length) issues"'
done
Length of output: 114815
Script:
#!/bin/bash
# Check if the ESLint rules are set to 'warn' level which won't fail the build
echo "Checking ESLint configuration severity levels..."
grep -A 30 "'jsx-a11y/" .eslintrc.cjs | grep -v "^--$" | grep -v "^$"
# Check if there are any 'error' level rules that could fail the build
echo -e "\nChecking for any error-level rules..."
grep -B 1 -A 1 "'error'" .eslintrc.cjs
# Check if there's any override configuration that might change the severity
echo -e "\nChecking for any overrides..."
grep -A 10 "overrides:" .eslintrc.cjs
Length of output: 3424
package.json (1)
49-50: LGTM! Improved script naming clarity
The rename from build-components/copy-components to compile-components/process-components better reflects the actual operations being performed.
| 'jsx-a11y/accessible-emoji': 'warn', | ||
| 'jsx-a11y/anchor-has-content': 'warn', | ||
| 'jsx-a11y/aria-activedescendant-has-tabindex': 'warn', | ||
| 'jsx-a11y/aria-props': 'warn', | ||
| 'jsx-a11y/aria-proptypes': 'warn', | ||
| 'jsx-a11y/aria-role': 'warn', | ||
| 'jsx-a11y/aria-unsupported-elements': 'warn', | ||
| 'jsx-a11y/click-events-have-key-events': 'warn', | ||
| 'jsx-a11y/heading-has-content': 'warn', | ||
| 'jsx-a11y/html-has-lang': 'warn', | ||
| 'jsx-a11y/iframe-has-title': 'warn', | ||
| 'jsx-a11y/img-redundant-alt': 'warn', | ||
| 'jsx-a11y/interactive-supports-focus': 'warn', | ||
| 'jsx-a11y/label-has-associated-control': 'warn', | ||
| 'jsx-a11y/media-has-caption': 'warn', | ||
| 'jsx-a11y/mouse-events-have-key-events': 'warn', | ||
| 'jsx-a11y/no-access-key': 'warn', | ||
| 'jsx-a11y/no-autofocus': 'warn', | ||
| 'jsx-a11y/no-distracting-elements': 'warn', | ||
| 'jsx-a11y/no-interactive-element-to-noninteractive-role': 'warn', | ||
| 'jsx-a11y/no-noninteractive-element-interactions': 'warn', | ||
| 'jsx-a11y/no-noninteractive-element-to-interactive-role': 'warn', | ||
| 'jsx-a11y/no-noninteractive-tabindex': 'warn', | ||
| 'jsx-a11y/no-onchange': 'warn', | ||
| 'jsx-a11y/no-redundant-roles': 'warn', | ||
| 'jsx-a11y/no-static-element-interactions': 'warn', | ||
| 'jsx-a11y/role-has-required-aria-props': 'warn', | ||
| 'jsx-a11y/role-supports-aria-props': 'warn', | ||
| 'jsx-a11y/scope': 'warn', | ||
| 'jsx-a11y/tabindex-no-positive': 'warn' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review accessibility rule severity levels
While it's good to start with 'warn' level during initial implementation, consider:
- Upgrading critical accessibility rules to 'error' level, such as:
- aria-props
- aria-role
- role-has-required-aria-props
- alt-text
- This ensures critical accessibility issues don't make it to production.
| 'jsx-a11y/no-noninteractive-element-interactions': 'warn', | ||
| 'jsx-a11y/no-noninteractive-element-to-interactive-role': 'warn', | ||
| 'jsx-a11y/no-noninteractive-tabindex': 'warn', | ||
| 'jsx-a11y/no-onchange': 'warn', |
There was a problem hiding this comment.
Remove deprecated 'no-onchange' rule
The 'jsx-a11y/no-onchange' rule is deprecated as onChange events are now well-supported by all browsers and screen readers.
- 'jsx-a11y/no-onchange': 'warn',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'jsx-a11y/no-onchange': 'warn', |
| "build-components": "rollup -c --bundleConfigAsCjs --max-old-space-size=8076", | ||
| "copy-components": "mkdirp ./dist/components && cp -r ./dist/temp-cleanup/*js ./dist/components && rimraf dist/temp-cleanup", | ||
| "build": "npm run test && npm run clean && npm run build-css && npm run generate-tokens && npm run bundle-tokens && npm run build-components && npm run copy-components --max-old-space-size=4076", | ||
| "compile-components": "rollup -c --bundleConfigAsCjs --max-old-space-size=8076", |
There was a problem hiding this comment.
Resolve conflicting memory configurations
There's an inconsistency in memory limits:
compile-componentssets--max-old-space-size=8076(8GB)buildscript overrides it to 4GB via NODE_OPTIONS
This could lead to unexpected behavior as NODE_OPTIONS will take precedence.
Suggestions:
- Use a consistent memory limit
- Move the memory configuration to a single location
- "compile-components": "rollup -c --bundleConfigAsCjs --max-old-space-size=8076",
+ "compile-components": "rollup -c --bundleConfigAsCjs",
"build": "npm run prebuild && npm run build:css && NODE_OPTIONS='--max-old-space-size=4076' npm run build:components",Also applies to: 54-54
This PR adds a build step to run build command to ensure build pipelines are intact so we don't introduce any breaking changes
Summary by CodeRabbit
Release Notes
New Features
Dependency Updates
@radui/uidependency version to^0.0.25.Build Process Improvements