new esbuild and removal of prebuild running twice #1214
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 7 minutes and 31 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 (3)
""" WalkthroughA new Node.js build script using esbuild was introduced to automate bundling of UI components, replacing the previous Rollup-based build process. The build orchestration in Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant NPM as npm
participant Script as build-components.cjs
participant Esbuild as esbuild
participant FS as File System
Dev->>NPM: run build:components
NPM->>Script: node build-components.cjs
Script->>FS: Read src/components/ui/*
Script->>Esbuild: Bundle each component .tsx
Esbuild->>FS: Output bundled JS to dist/temp-cleanup
Script->>NPM: Log success/failure
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
""" 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
🧹 Nitpick comments (3)
src/components/ui/Accordion/Accordion.tsx (1)
16-16: Remove trailing space from console warning message.The trailing space at the end of the console.warn message appears to be unintentional and should be removed for consistency.
- console.warn('Direct usage of Accordion is not supported. Please use Accordion.Root, Accordion.Item, etc. instead. '); + console.warn('Direct usage of Accordion is not supported. Please use Accordion.Root, Accordion.Item, etc. instead.');build-components.cjs (2)
11-12: Consider using asynchronous file operations for better performance.The script uses synchronous file system operations which could block the event loop. For better performance and scalability, consider using async operations.
-const components = fs.readdirSync(componentsPath) - .filter((file) => fs.statSync(path.join(componentsPath, file)).isDirectory()); +const components = await fs.promises.readdir(componentsPath); +const componentDirs = []; +for (const file of components) { + const stat = await fs.promises.stat(path.join(componentsPath, file)); + if (stat.isDirectory()) { + componentDirs.push(file); + } +}Note: This would require wrapping the logic in an async function.
23-23: Document the external dependencies list for future maintenance.The external dependencies list is hard-coded and may need updates as the project evolves. Consider adding a comment explaining why these specific packages are externalized.
- external: ['react', 'react-dom', 'react/jsx-runtime'], + external: [ + 'react', // Core React library + 'react-dom', // React DOM bindings + 'react/jsx-runtime' // JSX runtime for React 17+ + ],
📜 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)
build-components.cjs(1 hunks)package.json(2 hunks)src/components/ui/Accordion/Accordion.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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.
src/components/ui/Accordion/Accordion.tsx (3)
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`.
Learnt from: kotAPI
PR: rad-ui/ui#1031
File: src/components/ui/Accordion/fragments/AccordionRoot.tsx:41-44
Timestamp: 2025-04-07T04:38:34.864Z
Learning: The Accordion component in rad-ui/ui supports both controlled and uncontrolled modes through props like `value`, `defaultValue`, and `onValueChange`. When implementing controlled components, remember to: 1) Initialize state from defaultValue, 2) Update internal state when value changes (controlled mode), 3) Call onValueChange callback, and 4) Prevent internal state updates when in controlled mode.
Learnt from: decipher-cs
PR: rad-ui/ui#417
File: src/components/ui/Dropdown/Dropdown.tsx:0-0
Timestamp: 2024-12-12T08:22:59.375Z
Learning: The `Dropdown.Trigger` component is customizable and needs to be used with `Dropdown.Root`.
package.json (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.
🔇 Additional comments (1)
package.json (1)
59-59: LGTM: Updated to use the new esbuild-based build script.The script change correctly points to the new build-components.cjs file, aligning with the PR objective to replace Rollup with esbuild.
| Promise.all(components.map(component => { | ||
| const entry = path.join(componentsPath, component, `${component}.tsx`); | ||
| const outfile = path.join(outDir, `${component}.js`); | ||
| return esbuild.build({ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for entry file existence.
The script assumes all components follow the naming convention ${component}/${component}.tsx but doesn't validate that the entry file exists before attempting to build it. This could cause the entire build to fail if a component doesn't follow this convention.
Promise.all(components.map(component => {
const entry = path.join(componentsPath, component, `${component}.tsx`);
+ if (!fs.existsSync(entry)) {
+ console.warn(`Skipping ${component}: Entry file ${entry} not found`);
+ return Promise.resolve();
+ }
const outfile = path.join(outDir, `${component}.js`);
return esbuild.build({📝 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.
| Promise.all(components.map(component => { | |
| const entry = path.join(componentsPath, component, `${component}.tsx`); | |
| const outfile = path.join(outDir, `${component}.js`); | |
| return esbuild.build({ | |
| Promise.all(components.map(component => { | |
| const entry = path.join(componentsPath, component, `${component}.tsx`); | |
| if (!fs.existsSync(entry)) { | |
| console.warn(`Skipping ${component}: Entry file ${entry} not found`); | |
| return Promise.resolve(); | |
| } | |
| const outfile = path.join(outDir, `${component}.js`); | |
| return esbuild.build({ | |
| // ...rest of build options | |
| }); | |
| })); |
🤖 Prompt for AI Agents
In build-components.cjs around lines 14 to 17, the code assumes the entry file
`${component}/${component}.tsx` exists without validation, risking build failure
if missing. Add a check to verify the existence of the entry file before calling
esbuild.build. If the file does not exist, skip building that component or
handle the error gracefully to prevent the entire build from failing.
…o build-improvements
uses tsc --noEmit to point out the issues in prebuild failing it in first place if having types error
currently the script is adding a 'use client' ---- not sure of how it helps or is it right way (can be removed)
we have faced problems of using rad ui in ssr comps, might be helpful in clearing those issues
Summary by CodeRabbit
Summary by CodeRabbit