testing parallel builds #1202
testing parallel builds #1202
Conversation
|
Warning Rate limit exceeded@GoldGroove06 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 49 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
## Walkthrough
The GitHub Actions workflow for the RAD UI project was refactored from a single build job into a multi-stage pipeline. The new workflow introduces separate jobs for setup, testing, CSS building, and component building, with explicit dependencies and parallel execution for the build steps. The final build job now only outputs a completion message.
## Changes
| File(s) | Change Summary |
|-------------------------------------------|------------------------------------------------------------------------------------------------|
| .github/workflows/build-rad-ui.yml | Refactored workflow: split single build job into `setup`, `test`, `build_css`, `build_components`, and a final `build` job; added job dependencies, parallelization, and explicit permissions. |
| package.json | Added `esbuild` and `rollup-plugin-esbuild` as new devDependencies for enhanced build tooling. |
| rollup.config.cjs | Rewrote Rollup config: replaced per-component builds with a single multi-entry build; replaced Babel/TypeScript plugins with `rollup-plugin-esbuild`; added PostCSS; simplified externals; updated banner plugin. |
| src/components/ui/Accordion/fragments/AccordionRoot.tsx | Removed unused `useState` import from React. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant GitHub Actions
participant Setup Job
participant Test Job
participant Build CSS Job
participant Build Components Job
participant Final Build Job
GitHub Actions->>Setup Job: Start setup (checkout, Node.js, npm ci)
Setup Job-->>Test Job: On success, trigger test
Test Job->>Test Job: Run tests (checkout, Node.js, npm ci, npm test)
Test Job-->>Build CSS Job: On success, trigger build_css
Test Job-->>Build Components Job: On success, trigger build_components
Build CSS Job->>Build CSS Job: Build CSS (checkout, Node.js, npm ci, clean, build CSS)
Build Components Job->>Build Components Job: Build components (checkout, Node.js, npm ci, clean, build components)
Build CSS Job-->>Final Build Job: On success, notify build
Build Components Job-->>Final Build Job: On success, notify build
Final Build Job->>Final Build Job: Output completion messageSuggested labels
Suggested reviewers
Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.github/workflows/build-rad-ui.yml (3)
29-42: Testing job still duplicates checkout / node setupIf you keep the current structure, you can speed things up by re-using the workspace from
setupviauses: actions/upload-artifact/download-artifactrather than repeating the same three steps.Example sketch:
# in setup - name: Package workspace uses: actions/upload-artifact@v4 with: name: workspace path: | node_modules .npmrc # in test / build_* jobs - name: Restore workspace uses: actions/download-artifact@v4 with: name: workspaceThis keeps the pipeline parallelism while eliminating redundant work.
44-77: Consider mergingbuild_cssandbuild_componentsinto a matrix jobBoth jobs are structurally identical except for the final
npm run build:*command.
Using a matrix with a single job definition cuts YAML noise and is easier to maintain:build_assets: runs-on: ubuntu-latest needs: test strategy: matrix: target: [css, components] steps: # …checkout / setup / install… - run: npm run build:${{ matrix.target }}This preserves parallelism while reducing duplication.
80-83: Finalbuildjob does nothing actionableThe last job only echoes a message. If its sole purpose is to gate merge conditions, consider:
- Renaming it to
doneorcito make intent clearer.- Removing it entirely and instead relying on required-status checks for
build_cssandbuild_components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build-rad-ui.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kotAPI
PR: rad-ui/ui#640
File: .github/workflows/build-rad-ui.yml:5-5
Timestamp: 2024-12-14T02:25:41.034Z
Learning: In the `rad-ui/ui` repository, the `security-fixes` branch only updates dependencies, so linting and Chromatic workflows are not required for this branch.
.github/workflows/build-rad-ui.yml (1)
Learnt from: kotAPI
PR: rad-ui/ui#640
File: .github/workflows/build-rad-ui.yml:5-5
Timestamp: 2024-12-14T02:25:41.034Z
Learning: In the `rad-ui/ui` repository, the `security-fixes` branch only updates dependencies, so linting and Chromatic workflows are not required for this branch.
🪛 actionlint (1.7.7)
.github/workflows/build-rad-ui.yml
18-18: property "cache" is not defined in object type {}
(expression)
| - uses: actions/checkout@v4 | ||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '20' | ||
| cache: 'npm' | ||
| - name: Install dependencies | ||
| run: npm ci |
There was a problem hiding this comment.
🛠️ Refactor suggestion
npm ci repeated in every job – burns minutes and cache storage
Each downstream job re-installs dependencies even though setup already did so.
Instead, cache node_modules (with actions/cache) or upload them as an artifact in setup and download in dependent jobs. This will shave several minutes from every run.
🤖 Prompt for AI Agents
In .github/workflows/build-rad-ui.yml around lines 20 to 27, the workflow runs
`npm ci` in every job, causing redundant dependency installations and wasting
time and cache storage. To fix this, remove the repeated `npm ci` commands from
downstream jobs and instead cache the `node_modules` directory using
`actions/cache` or upload `node_modules` as an artifact in the setup job and
download it in dependent jobs. This will reuse installed dependencies and speed
up the workflow.
| outputs: | ||
| node-modules-cache: ${{ steps.cache.outputs.cache-hit }} | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '20' | ||
| cache: 'npm' |
There was a problem hiding this comment.
Broken outputs reference – workflow will fail at runtime
steps.cache.outputs.cache-hit points to a step ID that doesn’t exist.
Because the actions/setup-node step has no id, the expression resolves to null and the whole job definition is syntactically invalid – GitHub Actions will refuse to run the workflow.
Fix by either (a) removing the unnecessary custom output or (b) giving the setup-node step an id and referencing that:
- outputs:
- node-modules-cache: ${{ steps.cache.outputs.cache-hit }}
+ outputs:
+ node-modules-cache: ${{ steps.setup-node.outputs.cache-hit }}
- - name: Setup Node.js
- uses: actions/setup-node@v4
+ - name: Setup Node.js
+ id: setup-node
+ uses: actions/setup-node@v4Without this change the whole workflow is dead-on-arrival.
🧰 Tools
🪛 actionlint (1.7.7)
18-18: property "cache" is not defined in object type {}
(expression)
🤖 Prompt for AI Agents
In .github/workflows/build-rad-ui.yml around lines 17 to 25, the outputs section
references steps.cache.outputs.cache-hit, but there is no step with the ID
"cache" defined, causing the workflow to fail. Fix this by either removing the
outputs section if it is unnecessary or by adding an id: cache to the setup-node
step and ensuring the output reference matches that id. This will make the
output reference valid and prevent the workflow from being syntactically
invalid.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rollup.config.cjs (1)
37-46: Consider using either esbuild or terser for minification, not both.The configuration has
minify: falsein esbuild settings but then adds the terser plugin. This creates redundant processing.For better performance, choose one approach:
Option 1: Use esbuild's built-in minification (faster)
esbuild({ include: /\.[jt]sx?$/, exclude: /node_modules/, sourceMap: true, - minify: false, + minify: true, target: 'esnext', jsx: 'automatic', tsconfig: 'tsconfig.json', }), - terser(),Option 2: Keep terser for more aggressive optimization
Keep the current setup if terser provides better minification for your use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(2 hunks)rollup.config.cjs(1 hunks)src/components/ui/Accordion/fragments/AccordionRoot.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/ui/Accordion/fragments/AccordionRoot.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: kotAPI
PR: rad-ui/ui#640
File: .github/workflows/build-rad-ui.yml:5-5
Timestamp: 2024-12-14T02:25:41.034Z
Learning: In the `rad-ui/ui` repository, the `security-fixes` branch only updates dependencies, so linting and Chromatic workflows are not required for this branch.
🪛 Biome (1.9.4)
rollup.config.cjs
[error] 1-1: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 1-2: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 2-3: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 3-4: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 4-5: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 5-6: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 6-7: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 7-8: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 14-50: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
🔇 Additional comments (1)
package.json (1)
109-109: LGTM! Build dependencies added correctly.The addition of
esbuildandrollup-plugin-esbuildas development dependencies aligns with the rollup configuration refactoring to use esbuild for transpilation.Also applies to: 126-126
rollup.config.cjs
Outdated
| // Auto-generate entry points from `src/components` (if needed) | ||
| const componentFiles = readdirSync('src/components') | ||
| .filter(f => f.endsWith('.tsx') || f.endsWith('.ts')) | ||
| .map(f => `src/components/${f}`); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify: Component discovery only reads root-level files.
The new configuration only discovers .ts and .tsx files directly under src/components/, not in nested directories. According to the AI summary, the previous configuration discovered components under src/components/ui/ subdirectories.
Is this change intentional? If components exist in subdirectories, they won't be built.
🏁 Script executed:
#!/bin/bash
# Description: Check if there are components in nested directories that might be missed
# Find all .tsx and .ts files under src/components, including subdirectories
echo "All TypeScript files under src/components:"
fd -e tsx -e ts . src/components --type f | sort
# Check specifically for the ui subdirectory mentioned in the summary
echo -e "\nChecking for src/components/ui directory:"
if [ -d "src/components/ui" ]; then
echo "ui subdirectory exists. Files:"
fd -e tsx -e ts . src/components/ui --type f | sort
else
echo "No ui subdirectory found"
fi
# Compare with what the current config would capture
echo -e "\nFiles the current config would capture:"
ls src/components/*.{ts,tsx} 2>/dev/null || echo "No root-level .ts/.tsx files found"Length of output: 24744
Fix component discovery to include nested directories
The current logic in rollup.config.cjs (lines 10–13) only reads files directly under src/components, so it discovers nothing—the entire codebase lives in subfolders (e.g. src/components/ui/** and src/components/tools/**).
This will prevent any components from being built. Update the entry-point generation to recurse into subdirectories. For example:
- const componentFiles = readdirSync('src/components')
- .filter(f => f.endsWith('.tsx') || f.endsWith('.ts'))
- .map(f => `src/components/${f}`);
+ import glob from 'glob';
+
+ // Auto-generate entry points from all .ts/.tsx files under src/components
+ const componentFiles = glob
+ .sync('src/components/**/*.@(ts|tsx)')
+ .map(path => path);Points to address:
- rollup.config.cjs: replace
readdirSynclogic with a recursive glob (or equivalent). - Ensure new dependency (e.g.
glob) is installed or use a built-in solution. - Confirm that entry points now include all nested component files.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In rollup.config.cjs around lines 10 to 13, the current component file discovery
only reads files directly under src/components and misses nested directories.
Replace the readdirSync logic with a recursive file search using a glob pattern
or equivalent to include all .ts and .tsx files in subdirectories. If using a
new dependency like glob, ensure it is installed and imported properly. Verify
that the updated entry points array includes all nested component files under
src/components.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
rollup.config.cjs (2)
1-9: Critical: ES module syntax in CommonJS file will cause runtime errors.This issue was previously identified but still exists. The file uses ES module syntax (
import) but has a.cjsextension indicating CommonJS, which will fail at runtime.
11-14: Fix component discovery to include nested directories.This issue was previously identified but still exists. The current logic only reads files directly under
src/components, missing all nested directories where components actually exist.
🧹 Nitpick comments (1)
rollup.config.cjs (1)
4-4: Simplify the esbuild plugin initialization.The fallback pattern
esbuild.default || esbuildis unusual and likely unnecessary. Consider using a standard import approach.-const esbuildPlugin = esbuild.default || esbuild;Then use
esbuilddirectly on line 38:- esbuildPlugin({ + esbuild({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rollup.config.cjs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kotAPI
PR: rad-ui/ui#640
File: .github/workflows/build-rad-ui.yml:5-5
Timestamp: 2024-12-14T02:25:41.034Z
Learning: In the `rad-ui/ui` repository, the `security-fixes` branch only updates dependencies, so linting and Chromatic workflows are not required for this branch.
rollup.config.cjs (1)
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.stories.tsx:43-50
Timestamp: 2024-12-12T08:34:33.079Z
Learning: Ensure to verify existing ARIA attributes in components before suggesting additions during code reviews, especially in the `Dropdown.Trigger` component in `src/components/ui/Dropdown/Dropdown.stories.tsx`.
🪛 Biome (1.9.4)
rollup.config.cjs
[error] 1-1: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 1-2: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 2-3: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 4-5: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 5-6: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 6-7: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 7-8: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 8-9: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 15-51: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
⏰ 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). (2)
- GitHub Check: build_css
- GitHub Check: build_components
Summary by CodeRabbit