Add bundle size CI check using size-limit #2149
Conversation
WalkthroughThis PR introduces automated bundle size monitoring to the CI pipeline. It adds a GitHub Actions workflow that measures bundle sizes across PR and base branches, compares them against configurable thresholds, and provides local tooling and a skip mechanism for intentional increases. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
Code Review: Empty Commit PROverviewThis PR contains an empty commit merging Stats:
AnalysisPurpose AssessmentBased on the PR title and content, this appears to be an intentional empty commit. Empty commits are sometimes used for:
Issues IdentifiedCritical:
RecommendationsIf this is for testing CI workflows:
If this was created accidentally:
General Best Practices:
Questions
Review generated by Claude Code - following repository conventions from CLAUDE.md |
Code Review: Bundle Size CI TestOverviewThis PR adds a test file to verify that the bundle size CI workflow properly catches bundle size increases. Changes:
AnalysisPurposeBased on the branch name ( How It Works
Issues & RecommendationsCritical:
Best Practices per CLAUDE.md:
Security & Performance
Suggested Next Steps
Questions
Overall Assessment: This appears to be a well-intentioned test of the bundle size CI, but it should be clearly marked as a test PR and should not be merged. The approach is sound for verifying the CI works correctly. Review generated following repository conventions from CLAUDE.md |
size-limit report 📦
|
AbanoubGhadban
changed the base branch from
feature/bundle-size-ci-base
to
master
AbanoubGhadban
changed the title
PR Review: Bundle Size CI CheckThank you for adding bundle size monitoring! This is a valuable addition. Here is my detailed review: Strengths
Critical IssuesMissing PR Description: Still shows template. Please provide summary, motivation, and update checklist Missing Tests: No unit tests for bundle-size.mjs script (parseArgs, formatSize, formatDiff). No validation tests for CI workflow Medium Priority IssuesPath Dependencies: .size-limit.json has hardcoded paths that will silently fail if directories move. Add validation step and document in CLAUDE.md Error Handling Issues:
Documentation: Missing CHANGELOG entry, no CLAUDE.md docs, undocumented package.json scripts Code Quality Issues
SecurityGood: Using frozen-lockfile. Consider: Pin size-limit-action to SHA not v1 Checklist Status
Before Merging
Test locally: pnpm install --frozen-lockfile && pnpm run build && pnpm run size && bundle exec rubocop Overall solid foundation! Main gaps are docs and tests. 🚀 |
PR Review: Bundle Size CI CheckThank you for adding bundle size monitoring! Here is my comprehensive review: Strengths
Critical Issues (Must Fix)1. Missing CHANGELOG Entry (BLOCKER) 2. Missing PR Description (BLOCKER) 3. Path Verification Required
4. Testing Documentation Missing 5. Bug in Skip Check Logic 6. Removed globals Dependency Suggestions
Overall AssessmentValuable feature with good code quality. Main gaps are documentation and testing verification per CLAUDE.md requirements. Once critical issues addressed, will be great addition! |
## Summary - Add size-limit and its plugins (@size-limit/file, @size-limit/webpack) to enable bundle size measurement - Add `size` and `size:json` npm scripts for local bundle size checking This is a prerequisite for the bundle size CI workflow in PR #2149. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
AbanoubGhadban
force-pushed
the
feature/bundle-size-ci
branch
from
178cab7 to
d6cec1a
Compare
Code Review FeedbackThank you for adding bundle size monitoring to the project! This is a valuable addition that will help prevent unexpected size regressions. Here is my detailed review: ✅ Strengths
🔴 Critical Issues1. Missing executable permissions on bin scripts ❗The new bash scripts in Fix required: chmod +x bin/compare-bundle-sizes bin/skip-bundle-size-check
git add bin/compare-bundle-sizes bin/skip-bundle-size-check2. Incomplete PR description ❗The PR description is still using the template placeholders. According to CLAUDE.md guidelines, you should:
This is a user-facing feature that affects CI/CD workflows and needs a changelog entry.
|
Code Review: Add bundle size CI check using size-limitExecutive SummaryThis PR introduces automated bundle size monitoring with dynamic thresholds and a skip mechanism. Implementation is solid but has security and design issues to address. Critical Security Issues1. Script Injection via github.head_ref - ✅ FIXEDStatus: Fixed in commit 178cab7 by moving to environment variable. Good catch! 2. Incomplete Input Validation in Skip CheckLocation: .github/workflows/bundle-size.yml:27-28 Problem: tr -d removes ALL whitespace including newlines. Multiple branch names would concatenate. Fix: Use head -n1 instead and validate line count. High Priority Issues3. Missing JSON Validationscripts/bundle-size.mjs needs to validate JSON structure from size-limit 4. Shell Script Error Handlingbin/compare-bundle-sizes needs set -o pipefail to catch pipe errors 5. Race Condition in Workflownode_modules from base branch may contaminate PR build. Clean between checkouts. Medium Priority Issues
What's Done Well ✅
RecommendationsMust Fix:
Should Fix:
Nice to Have:
Final VerdictGreat work! Security fix was spot-on. Please address issues 2-4 and this will be ready to merge. |
Code Review: Bundle Size CI CheckThank you for this contribution! This is a well-thought-out approach to tracking bundle size. Here's my detailed review: ✅ Strengths
🚨 Critical Issues1. Missing executable permissions on scriptsThe bash scripts in -rw-r--r-- bin/compare-bundle-sizes
-rw-r--r-- bin/skip-bundle-size-checkFix: chmod +x bin/compare-bundle-sizes bin/skip-bundle-size-check
git add bin/compare-bundle-sizes bin/skip-bundle-size-checkThis is critical - without executable permissions, users can't run these scripts directly. 2. Missing CHANGELOG entryAccording to Add to CHANGELOG.md: #### Developer Experience
- [PR 2149](https://github.com/shakacode/react_on_rails/pull/2149) by [AbanoubGhadban](https://github.com/AbanoubGhadban): Add bundle size CI check with dynamic limits and comparison utilities3. Unrelated package.json changeThe PR moves
|
Code Review - Bundle Size MonitoringThis is a well-implemented bundle size monitoring system! The PR adds comprehensive size tracking with smart dynamic limits. Here's my detailed feedback: ✅ Strengths
|
Pull Request Review: Bundle Size CI CheckThank you for implementing bundle size monitoring! This is a valuable addition to prevent performance regressions. I have reviewed the changes and have feedback organized by category. 🔴 Critical Issues1. Missing Dependency in package.jsonThe workflow uses Required fix: "devDependencies": {
"size-limit": "^11.0.0",
"@size-limit/file": "^11.0.0",
"@size-limit/webpack": "^11.0.0"
}2. Build Artifact Path VerificationThe workflow verifies build artifacts exist at Required before merge: # From CLAUDE.md: "Mandatory Testing After ANY Changes"
rm -rf node_modules
pnpm install --frozen-lockfile
pnpm run build
pnpm run prepack
ls -la packages/react-on-rails/lib/*.js
ls -la packages/react-on-rails-pro/lib/*.js
ls -la packages/react-on-rails-pro-node-renderer/lib/*.js3. Incomplete PR ChecklistPer CLAUDE.md requirements, the PR description shows:
🟡 Code Quality & Best Practices4. CI Job Failure Needs InvestigationThe PR shows
Per CLAUDE.md guidelines:
5. Bash Script Robustnessbin/compare-bundle-sizes: trap cleanup EXIT INT TERM6. Unused Environment VariableIn env:
CI_JOB_NUMBER: 1Question: This is not referenced anywhere in the workflow. If unused, remove it. 🟢 Performance Considerations7. Workflow EfficiencyThe workflow:
Suggestion: Consider caching 🔒 Security Concerns8. Permissionspermissions:
contents: read
pull-requests: writeGood practice! Following principle of least privilege. ✅ 📝 Documentation & User Experience9. Missing CHANGELOG EntryPer CLAUDE.md:
This is a user-facing feature (developers will see bundle size checks in PRs). Add to CHANGELOG.md: #### Developer Experience
[PR 2149](https://github.com/shakacode/react_on_rails/pull/2149) by [AbanoubGhadban](https://github.com/AbanoubGhadban): Add bundle size CI check using size-limit. PRs that increase bundle size by more than 0.5KB will now fail CI with a detailed report. Use `bin/skip-bundle-size-check` for intentional size increases.10. User-Facing Documentation NeededUsers need to know:
Consider adding documentation explaining the feature. 🧪 Test Coverage11. No Automated TestsThe
Recommendation: Consider adding Jest tests for the utility functions. 🎯 Specific Implementation Notes12. Magic Number Documentationconst DEFAULT_THRESHOLD = 512;Consider adding a comment explaining why 512 bytes: // Default threshold: 0.5 KB (512 bytes)
// This represents a ~2% increase for typical bundle sizes (20-30KB gzipped)
const DEFAULT_THRESHOLD = 512;13. Handling New Config EntriesThe workflow copies the PR config to the base branch. This is clever, but what happens when:
Consider how ✅ What is Done Well
📋 Action Items Before MergeMust Fix:
Should Fix:
Nice to Have:
SummaryThis is a valuable addition that will help prevent bundle size regressions. The implementation is generally solid with good separation of concerns and useful developer tools. Critical items to address:
Recommendation: Request changes for the critical issues, then approve after fixes. Generated by Claude Code PR Review |
Add .size-limit.json with configuration for measuring bundle sizes of: - react-on-rails (raw, gzip, brotli) - react-on-rails-pro (raw, gzip, brotli) - react-on-rails-pro-node-renderer (raw, gzip, brotli) - Client bundled sizes for webpack imports This is a prerequisite for the bundle size CI workflow in PR #2149. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
## Summary - Add `.size-limit.json` with configuration for measuring bundle sizes of all packages - Includes raw, gzip, and brotli measurements - Includes webpack bundled sizes for client imports This is a prerequisite for the bundle size CI workflow in PR #2149. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
- Install size-limit and @size-limit/file packages - Add .size-limit.json configuration tracking all package outputs - Create GitHub Actions workflow for PR comments with size comparison - Add npm scripts: `pnpm run size` and `pnpm run size:json` Bundle size limits: - react-on-rails (full): 20 kB gzipped - react-on-rails (client): 1 kB gzipped - react-on-rails-pro (full): 45 kB gzipped - react-on-rails-pro (client): 2 kB gzipped - react-on-rails-pro-node-renderer: 15 kB gzipped The CI will post comparison comments on PRs showing size changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The size-limit tool only measures files, it doesn't build them. Update the size scripts to run build before measuring. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Compare all three compression levels for each package: - Raw (uncompressed) - actual file size on disk - Gzip - most common server compression - Brotli - best compression, newer standard 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove static limits from .size-limit.json - Workflow now fetches base branch sizes first - Dynamically calculates limits as base_size + 512 bytes - Supports any PR base branch, not just master - Simplified to track only gzip sizes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
npx doesn't find pnpm-installed packages, causing CI failure. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…ses" This reverts commit 03e5aef.
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…ases" This reverts commit 1aa7f9c.
…on increases"" This reverts commit 6343d50.
This reverts commit bd43ed8.
Add @size-limit/webpack to measure realistic bundle sizes when users import from the packages: - react-on-rails/client - react-on-rails-pro/client - react-on-rails-pro/registerServerComponent/client - react-on-rails-pro/wrapServerComponentRenderer/client Use "webpack": false for glob-based entries to prevent bundling server files that have Node.js dependencies. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…ses" This reverts commit bf10b05.
Move github.head_ref from inline script interpolation to an environment variable to prevent potential command injection attacks from malicious branch names containing shell metacharacters. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The base branch (master) may not have .size-limit.json config file. Copy it from the PR branch before checking out and measuring the base branch sizes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add verification step to check build artifacts exist before measuring - Remove unreachable return statement before process.exit(1) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
AbanoubGhadban
force-pushed
the
feature/bundle-size-ci
branch
from
e32789c to
fe55a99
Compare
The function calls process.exit(1) which never returns, making ESLint's consistent-return rule a false positive. Added disable comment. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review: Bundle Size CI CheckOverviewThis PR adds automated bundle size checking to CI, building on the size-limit packages added in #2150. The implementation is well-thought-out with dynamic threshold-based checking and skip mechanisms. ✅ Strengths
|
Code Review: Bundle Size CI CheckExcellent work on implementing bundle size tracking! This is a valuable addition to the CI pipeline. Here's my detailed review: ✅ Strengths1. Well-architected solution
2. Security conscious
3. Good error handling
4. Code quality
🔍 Issues & Suggestions1. CRITICAL: PR Description is Empty
|
Code Review: Bundle Size CI CheckThank you for this comprehensive PR! This adds valuable CI infrastructure. Here's my detailed review: ✅ StrengthsArchitecture & Design
Code Quality
|
- Add eslint-disable for consistent-return false positive (process.exit never returns) - Add Bundle Size Checks section to CONTRIBUTING.md documenting: - How to run size checks locally - How to use bin/compare-bundle-sizes script - How to bypass the check for intentional increases - What packages get measured 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add CHANGELOG entry for bundle size CI monitoring feature - Document DEFAULT_THRESHOLD explaining it's intentionally strict 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review: Bundle Size CI CheckThanks for this PR! This is a great addition to prevent unexpected bundle size regressions. I've reviewed the implementation and have some feedback organized by category: ✅ Strengths
🔍 Potential Issues1. Critical: Build Artifact Verification TimingThe build artifact verification happens BEFORE measuring sizes, but it should also verify artifacts exist for the PR branch before running size-limit. Without this, size-limit might measure empty directories or fail cryptically. 2. Security: Shell Injection in skip-check scriptThe branch name is used directly without sanitization. While git branch names have restrictions, this could be hardened with regex validation. 3. Race Condition: Concurrent base branch changesIf the base branch is updated between checking out base and PR, the comparison might be against outdated sizes. Consider using github.event.pull_request.base.sha instead of github.base_ref to use the specific commit SHA. 4. Error Handling: Silent failures in compare scriptThe readJsonFileOrExit function exits silently on parse errors. Consider adding more context about what went wrong. 🎯 Minor Improvements
📝 Testing RecommendationsBefore merging, please verify:
🎓 Adherence to CLAUDE.md
🚀 RecommendationThis is a solid PR with good architecture. The issues above are mostly minor, with issue #1 (PR build verification) being the most critical to address. Must fix: Add build artifact verification for PR branch Overall: Approve with minor changes 👍 |
![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 (5)
bin/compare-bundle-sizes (1)
53-54: Suppressing errors may hide real build failures.Using
|| trueafterpnpm installandpnpm run buildwill suppress genuine errors. If the build fails, the script will continue and produce misleading size comparisons.Consider letting critical commands fail:
-pnpm install --frozen-lockfile 2>&1 | grep -v "^$" | head -5 || true -pnpm run build 2>&1 | grep -v "^$" | tail -3 || true +pnpm install --frozen-lockfile 2>&1 | grep -v "^$" | head -5 +pnpm run build 2>&1 | grep -v "^$" | tail -3Or add explicit error handling to report which step failed.
scripts/bundle-size.mjs (2)
180-184: Consider handling new entries more explicitly.When a current entry has no matching base entry (new package), it defaults to
{ size: 0 }, making any size appear as 100% increase. This is likely intentional (flag new large bundles), but consider logging a notice for clarity.- const base = baseSizes.find((b) => b.name === current.name) || { size: 0 }; + const baseEntry = baseSizes.find((b) => b.name === current.name); + const base = baseEntry || { size: 0 }; + const isNew = !baseEntry;Then optionally mark new entries in the output.
240-240: Threshold display text is slightly confusing.The text
"Threshold: ${formatSize(threshold)} (base + ${formatSize(threshold)})"reads oddly since the second part just repeats the threshold value. Consider simplifying:- console.log(`Threshold: ${formatSize(threshold)} (base + ${formatSize(threshold)})`); + console.log(`Threshold: ${formatSize(threshold)} allowed increase per package`);bin/skip-bundle-size-check (1)
1-34: Ensure skip file is always written at repo rootRight now
SKIP_FILE=".bundle-size-skip-branch"is relative to the current working directory, so runningbin/skip-bundle-size-checkfrom a subdirectory will create the skip file in the wrong place and CI won’t see it. Consider resolving andcd‑ing to the Git repo root before writing:-set -e - -SKIP_FILE=".bundle-size-skip-branch" -BRANCH=$(git branch --show-current) +set -e + +REPO_ROOT=$(git rev-parse --show-toplevel) +cd "$REPO_ROOT" + +SKIP_FILE=".bundle-size-skip-branch" +BRANCH=$(git branch --show-current)Optionally, you could also harden the script with
set -euo pipefailfor better failure behavior. Also, please ensure this file ends with a trailing newline per the coding guidelines..github/workflows/bundle-size.yml (1)
22-34: Clarify.bundle-size-skip-branchformat and parsing assumptionsThe skip logic strips comments/blank lines and then removes all whitespace:
SKIP_BRANCH=$(grep -v '^[[:space:]]*#' "$SKIP_FILE" 2>/dev/null | grep -v '^[[:space:]]*$' | tr -d '[:space:]' || echo "")This effectively assumes a single, whitespace‑free branch name in the file. If someone later adds multiple branch lines or extra text, they’ll get concatenated into one string, and matching may silently stop working.
Since the intended contract is “one branch per file,” consider:
- documenting that explicitly in
.bundle-size-skip-branch, and/or- guarding against multiple non‑comment lines (e.g., take only the first) to avoid surprising behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.bundle-size-skip-branch(1 hunks).github/workflows/bundle-size.yml(1 hunks)CHANGELOG.md(1 hunks)CONTRIBUTING.md(1 hunks)bin/compare-bundle-sizes(1 hunks)bin/skip-bundle-size-check(1 hunks)package.json(1 hunks)scripts/bundle-size.mjs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
ALWAYS ensure files end with a newline character before committing/pushing
Files:
bin/skip-bundle-size-checkscripts/bundle-size.mjsbin/compare-bundle-sizesCONTRIBUTING.mdpackage.jsonCHANGELOG.md
🧠 Learnings (10)
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: The `react_on_rails_pro/` directory has its own Prettier/ESLint configuration and must be linted separately
Applied to files:
CONTRIBUTING.mdpackage.json
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
CONTRIBUTING.md
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
CONTRIBUTING.md
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to **/*.{rb,rake} : ALWAYS run `bundle exec rubocop` and fix ALL violations before every commit/push
Applied to files:
CONTRIBUTING.md
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to lib/react_on_rails/*.rb : Create RBS signature files for new Ruby files in `lib/react_on_rails/` by adding corresponding `.rbs` file in `sig/react_on_rails/`, adding to Steepfile, and validating before committing
Applied to files:
CONTRIBUTING.md
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to /CHANGELOG.md : Update `/CHANGELOG.md` for user-visible changes (features, bug fixes, breaking changes, deprecations, performance improvements) to the open-source React on Rails gem and npm package
Applied to files:
CONTRIBUTING.mdCHANGELOG.md
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
CONTRIBUTING.md
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
CONTRIBUTING.md
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
package.json
📚 Learning: 2025-11-25T08:05:17.804Z
Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T08:05:17.804Z
Learning: Applies to /CHANGELOG*.md : Use changelog format: `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)` (no hash in PR number)
Applied to files:
CHANGELOG.md
⏰ 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). (7)
- GitHub Check: package-js-tests
- GitHub Check: rspec-dummy-app-node-renderer
- GitHub Check: dummy-app-node-renderer-e2e-tests
- GitHub Check: dummy-app-integration-tests (3.4, 22, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: examples (3.4, latest)
- GitHub Check: build
🔇 Additional comments (8)
.bundle-size-skip-branch (1)
1-6: LGTM!Clear documentation for the bundle size skip mechanism. The file properly explains its purpose, usage, and when to use it.
CHANGELOG.md (1)
28-29: LGTM!The changelog entry follows the correct format (no hash in PR number) and accurately describes the bundle size monitoring feature with all key capabilities.
CONTRIBUTING.md (1)
308-370: Documentation is comprehensive and well-structured.Clear instructions for local testing, bypassing checks, and understanding what's measured. The guidance to document intentional size increases in PR descriptions is a good practice.
bin/compare-bundle-sizes (1)
27-35: Good cleanup trap implementation.The cleanup function properly restores the working tree state and pops the stash when needed. Using
2>/dev/null || truefor cleanup commands is appropriate since cleanup should be best-effort.package.json (1)
49-49: No functional change - dependency reordering.The
globalsentry appears to have been reordered alphabetically withindevDependencies. No version or functional changes.scripts/bundle-size.mjs (3)
38-43: LGTM - formatSize utility.Clean implementation. For bundle size monitoring, KB granularity is appropriate.
102-132: setLimits command implementation is correct.Properly reads base sizes, computes dynamic limits (base + threshold), and writes back to config. Good logging for debugging.
293-314: Main command dispatch is well-structured.Clean switch statement with help fallback. Exits appropriately for unknown commands.
| # 1. Get PR's size-limit config first (base branch may not have it) | ||
| - name: Checkout PR branch for config | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Save size-limit config | ||
| run: cp .size-limit.json /tmp/size-limit-config.json | ||
|
|
||
| # 2. Get base branch sizes | ||
| - name: Checkout base branch | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ github.base_ref }} | ||
|
|
||
| - name: Copy size-limit config to base branch | ||
| run: cp /tmp/size-limit-config.json .size-limit.json | ||
|
|
||
| - name: Install base dependencies | ||
| run: pnpm install --frozen-lockfile | ||
|
|
||
| - name: Build base branch | ||
| run: pnpm run build | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the workflow file ends with a trailing newline
tail -c 1 .github/workflows/bundle-size.yml | od -An -tx1Repository: shakacode/react_on_rails
Length of output: 110
🏁 Script executed:
# Get the file size and last few lines to understand the ending
wc -l .github/workflows/bundle-size.yml
tail -5 .github/workflows/bundle-size.yml | cat -ARepository: shakacode/react_on_rails
Length of output: 267
🌐 Web query:
size-limit behavior missing target files npm package
💡 Result:
Likely cause
- Size Limit reports "missing target files" when the config "path" (or glob) doesn't match any files — e.g., you run size-limit before your build produces the bundle, the glob is wrong, or you asked Size Limit to use webpack but your project already has no bundle to inspect. (npmjs.com)
Quick fixes to try
- Make sure you build before running size-limit (add "npm run build && size-limit" or run your build step in CI). (npmjs.com)
- Verify the config "path" matches actual files (try the glob on the shell or point to the exact file). If using patterns, ensure they expand in the CI environment. (npmjs.com)
- If your project is an application with its own bundler, set "webpack": false in the size-limit section so Size Limit checks the produced bundle instead of trying to build one. If you want Size Limit to create a bundle for a library, leave webpack enabled or use the @size-limit/webpack plugin. (npmjs.com)
- Add an explicit entry array (entry) or use the @size-limit/file plugin for checking specific files if needed. (npmjs.com)
If that doesn't help
- Run size-limit locally with verbose/--why to get more details and confirm which path(s) are failing, and paste the exact error + your .size-limit / package.json config if you want me to inspect it. (npmjs.com)
Sources
- size-limit README and config docs (npm / GitHub). (npmjs.com)
Citations:
- 1: https://www.npmjs.com/package/size-limit/v/1.1.2?utm_source=openai
- 2: https://www.npmjs.com/package/size-limit/v/1.1.2?utm_source=openai
- 3: https://www.npmjs.com/package/size-limit/v/1.3.2?utm_source=openai
- 4: https://www.npmjs.com/package/size-limit/v/1.3.2?utm_source=openai
- 5: https://www.npmjs.com/package/size-limit/v/1.3.2?utm_source=openai
- 6: https://www.npmjs.com/package/size-limit/v/1.3.2?utm_source=openai
- 7: https://www.npmjs.com/package/size-limit/v/1.1.2?utm_source=openai
🏁 Script executed:
# Search the codebase for how size-limit is configured and used
rg "size-limit" --type yaml --type json -A 2 -B 2Repository: shakacode/react_on_rails
Length of output: 3685
🏁 Script executed:
# Check if there are any test cases or scripts that show size-limit error handling
rg "size-limit.*error\|missing.*target" -iRepository: shakacode/react_on_rails
Length of output: 50
🏁 Script executed:
# Check the .size-limit.json file to understand its structure
cat .size-limit.jsonRepository: shakacode/react_on_rails
Length of output: 3062
🏁 Script executed:
# Check the bundle-size.mjs script to see how it handles size-limit execution
cat scripts/bundle-size.mjsRepository: shakacode/react_on_rails
Length of output: 9246
🏁 Script executed:
# View the full workflow file to understand the complete logic and error handling
cat .github/workflows/bundle-size.ymlRepository: shakacode/react_on_rails
Length of output: 3402
🏁 Script executed:
# Check if the "Verify build artifacts" step is part of the reviewed code (lines 56-77)
# The snippet shows lines 56-77, let's count and see where verify step falls
sed -n '56,77p' .github/workflows/bundle-size.ymlRepository: shakacode/react_on_rails
Length of output: 734
🏁 Script executed:
# Get line numbers of key steps to understand the code review scope
grep -n "Measure base branch sizes\|Verify build artifacts" .github/workflows/bundle-size.ymlRepository: shakacode/react_on_rails
Length of output: 150
Size-limit may fail when PR adds new bundle entries not present on base
You copy the PR's .size-limit.json to the base branch and measure sizes without checking whether all configured paths exist. If a PR introduces new bundle entries pointing to files that don't exist on the base branch, size-limit will error before producing base-sizes.json, breaking the job.
While the workflow includes a "Verify build artifacts" step (line 78), it only checks the three existing packages and doesn't validate all entries in the .size-limit.json config. Consider either:
- Extending the verify step to check all configured paths, or
- Falling back to the base branch's own
.size-limit.jsonfor measurement if the PR's config is incompatible.
🤖 Prompt for AI Agents
In .github/workflows/bundle-size.yml around lines 56 to 77, the workflow blindly
copies the PR's .size-limit.json onto the checked-out base branch which will
cause size-limit to fail if the PR added new bundle entry paths that don't exist
on the base; add a validation step after "Copy size-limit config to base branch"
that parses /tmp/size-limit-config.json and verifies every configured path/file
exists in the checked-out base branch, and if any are missing restore the base
branch's own .size-limit.json (or skip overwriting) so size measurements run
against a compatible config; alternatively (or additionally) extend the later
"Verify build artifacts" step to iterate over all entries from the active
.size-limit.json and fail early with a clear message if any paths are absent
before invoking size-limit.
| set -e | ||
|
|
||
| BASE_BRANCH="${1:-master}" | ||
| CURRENT_BRANCH=$(git branch --show-current) |
There was a problem hiding this comment.
Detached HEAD state may cause issues in CI environments.
git branch --show-current returns an empty string in detached HEAD state, which is common in CI. This would cause the cleanup function and the checkout back to the current branch to fail silently or behave unexpectedly.
Consider adding a fallback to use the commit SHA:
-CURRENT_BRANCH=$(git branch --show-current)
+CURRENT_BRANCH=$(git branch --show-current)
+if [ -z "$CURRENT_BRANCH" ]; then
+ # Fallback to commit SHA for detached HEAD (common in CI)
+ CURRENT_BRANCH=$(git rev-parse HEAD)
+fi📝 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.
| CURRENT_BRANCH=$(git branch --show-current) | |
| CURRENT_BRANCH=$(git branch --show-current) | |
| if [ -z "$CURRENT_BRANCH" ]; then | |
| # Fallback to commit SHA for detached HEAD (common in CI) | |
| CURRENT_BRANCH=$(git rev-parse HEAD) | |
| fi |
🤖 Prompt for AI Agents
In bin/compare-bundle-sizes around line 19, CURRENT_BRANCH is set using git
branch --show-current which yields an empty string in detached HEAD CI runs;
change the logic to record a safe fallback (use git rev-parse --short HEAD or
git rev-parse --verify HEAD) if the branch name is empty, store that ref in
CURRENT_BRANCH, and update any cleanup/checkout code to handle restoring either
a branch name or the commit SHA accordingly (i.e., if CURRENT_BRANCH is
empty-string fallback then checkout the saved commit SHA). Ensure the script
treats the saved ref consistently everywhere it currently uses CURRENT_BRANCH.
| bin/compare-bundle-sizes --hierarchical # Group results by package | ||
| ``` |
There was a problem hiding this comment.
The --hierarchical flag is documented but not implemented.
The bin/compare-bundle-sizes script doesn't pass the --hierarchical flag to the underlying scripts/bundle-size.mjs, and the Node script doesn't have this option implemented. Either remove this from the documentation or implement the feature.
Options:
```sh
bin/compare-bundle-sizes main # Compare against 'main' instead of 'master'
-bin/compare-bundle-sizes --hierarchical # Group results by package
<details>
<summary>🤖 Prompt for AI Agents</summary>
In CONTRIBUTING.md around lines 343-344 the docs mention a --hierarchical flag
that is not implemented; either remove the example or implement it. To
implement: update bin/compare-bundle-sizes to forward the --hierarchical flag
(and any value) to scripts/bundle-size.mjs when spawning/execing the Node
script, and modify scripts/bundle-size.mjs to accept and parse a --hierarchical
boolean option from process.argv (or the existing arg parser) and implement the
grouping logic (or toggle an existing code path) so that when --hierarchical is
present results are grouped by package; alternatively, if you prefer not to
implement, remove the --hierarchical example line from CONTRIBUTING.md so docs
reflect current behavior.
</details>
<!-- fingerprinting:phantom:medusa:ocelot -->
<!-- This is an auto-generated comment by CodeRabbit -->
Code Review: Bundle Size CI CheckOverall Assessment✅ Excellent PR! This is a well-implemented feature that adds automated bundle size monitoring to the CI pipeline. The implementation is clean, well-documented, and follows best practices. Strengths
Minor Issues and Suggestions1. Missing Build Artifact VerificationIn .github/workflows/bundle-size.yml:86-95, you verify build artifacts exist for the base branch. Consider adding the same verification after building the PR branch (around line 102) for symmetry and to catch issues on both branches. 2. Hardcoded Threshold (Design Choice)scripts/bundle-size.mjs:22 defines a 0.5KB threshold for all packages. Consider whether this is appropriate for all package sizes - it might be too strict for small utilities or too lenient for large bundles. This could be a future enhancement to make configurable per-package. 3. Grep Pattern Fragility.github/workflows/bundle-size.yml:28 uses tr -d [:space:] which removes ALL whitespace, including within branch names. Consider using xargs instead to trim only leading/trailing whitespace while preserving internal spaces. 4. Package.json ChangeThe only change in package.json is alphabetically sorting globals. This is fine but unrelated to the PR main purpose - consider mentioning this in the PR description. 5. Error Handling in bin/compare-bundle-sizesLine 51: If neither checkout succeeds, the script continues silently. The second git checkout should exit on failure with a clear error message. 6. CI Path FiltersConsider adding scripts/bundle-size.mjs to the workflow path filters, so changes to the size comparison logic trigger the workflow for validation. Security Considerations✅ No security concerns identified
Performance Considerations✅ Well optimized
Potential improvement: Consider caching pnpm store to speed up installs by 30-60 seconds. Test Coverage✅ Adequate for infrastructure PR
Compliance with CLAUDE.md✅ Follows all guidelines
Final RecommendationAPPROVED ✅ with minor suggestions for future improvement. This PR is production-ready. The issues mentioned above are minor and can be addressed in follow-up PRs if desired. The core functionality is solid, well-tested, and well-documented. Questions for the author:
Great work, @AbanoubGhadban! This will be valuable for preventing bundle size regressions. 🎉 |
Code Review: Bundle Size CI CheckThanks for this excellent addition! This is a valuable feature for preventing bundle size regressions. ✅ StrengthsArchitecture & Design
Code Quality
🔍 Potential Issues & Suggestions1. Missing Newline Requirement Violation
|
## Summary - Add size-limit and its plugins (@size-limit/file, @size-limit/webpack) to enable bundle size measurement - Add `size` and `size:json` npm scripts for local bundle size checking This is a prerequisite for the bundle size CI workflow in PR #2149. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
## Summary - Add `.size-limit.json` with configuration for measuring bundle sizes of all packages - Includes raw, gzip, and brotli measurements - Includes webpack bundled sizes for client imports This is a prerequisite for the bundle size CI workflow in PR #2149. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
## Summary Add automated bundle size monitoring to CI using [size-limit](https://github.com/ai/size-limit). This helps prevent unexpected bundle size regressions by comparing PR bundle sizes against the base branch. ### How It Works 1. CI checks out the base branch and measures bundle sizes 2. CI checks out the PR branch and measures bundle sizes 3. Compares sizes - fails if any package increases by more than 0.5KB 4. Posts a size report comment on the PR ### What Gets Measured - **react-on-rails**: Raw, gzip, and brotli compressed sizes - **react-on-rails-pro**: Raw, gzip, and brotli compressed sizes - **react-on-rails-pro-node-renderer**: Raw, gzip, and brotli compressed sizes - **Webpack bundled imports**: Client-side bundle sizes when importing via webpack ### Local Testing ```sh # Check current bundle sizes pnpm run size # Compare your branch against master bin/compare-bundle-sizes ``` ### Bypassing the Check For intentional size increases (e.g., adding new features): ```sh bin/skip-bundle-size-check git add .bundle-size-skip-branch git commit -m "Skip bundle size check for intentional size increase" git push ``` ## Pull Request checklist - [x] Add/update test to cover these changes - N/A (CI workflow) - [x] Update documentation - Added to CONTRIBUTING.md - [x] Update CHANGELOG file <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Automated bundle size CI monitoring compares PR bundle sizes against the base branch and fails if any package exceeds a 0.5KB growth threshold. * Added bypass mechanism for intentional bundle size increases. * **Documentation** * Bundle size checking and comparison commands documented. * Local tooling available to measure and compare package sizes against a base branch. * **Chores** * Bundle size monitoring workflow set up for CI. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
Add automated bundle size monitoring to CI using size-limit. This helps prevent unexpected bundle size regressions by comparing PR bundle sizes against the base branch.
How It Works
What Gets Measured
Local Testing
Bypassing the Check
For intentional size increases (e.g., adding new features):
bin/skip-bundle-size-check git add .bundle-size-skip-branch git commit -m "Skip bundle size check for intentional size increase" git pushPull Request checklist
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.