Add bundle size CI check using size-limit #2148
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds bundle-size monitoring: a new GitHub Actions workflow that runs size checks on PRs, a Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (push/PR)
participant GH as GitHub Actions
participant Runner as Actions Runner (ubuntu-22.04)
participant PNPM as pnpm store/cache
participant SL as size-limit Action
participant API as GitHub API
Dev->>GH: open PR (paths match)
GH->>Runner: start "Bundle Size" job
Runner->>Runner: setup Node 22, pnpm 9
Runner->>Runner: checkout base branch & install deps
Runner->>SL: run build & size-limit (base) -> produce JSON
Runner->>Runner: checkout PR branch & install deps
Runner->>Runner: adjust .size-limit.json limits (base + 0.5KB)
Runner->>SL: run build & size-limit (PR) with pnpm
SL->>API: report results/comments using GITHUB_TOKEN
API-->>Dev: update PR with size results
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: Bundle Size CI CheckThanks for adding bundle size monitoring! This is a valuable addition to prevent performance regressions. Here's my review: ✅ Strengths
|
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
1-118: Add trailing newline to match coding guidelines.Per the coding guidelines, all files must end with a newline character.
Apply this diff to the end of the file:
} } -} +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
.github/workflows/bundle-size.yml(1 hunks).size-limit.json(1 hunks)package.json(4 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:
package.json
🧠 Learnings (13)
📓 Common learnings
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
📚 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:
.size-limit.jsonpackage.json
📚 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:
.size-limit.jsonpackage.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 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:
.size-limit.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:
.size-limit.jsonpackage.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: The `react_on_rails_pro/` directory has its own Prettier/ESLint configuration and must be linted separately
Applied to files:
.size-limit.jsonpackage.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: Exclude `/coverage`, `/tmp`, `/gen-examples`, `/packages/react-on-rails/lib`, `/node_modules`, `/spec/dummy/node_modules`, `/spec/dummy/tmp`, `/spec/dummy/app/assets/webpack`, `/spec/dummy/log`, `/spec/dummy/e2e/playwright-report`, `/spec/dummy/test-results` from IDE indexing
Applied to files:
.size-limit.json
📚 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:
.size-limit.jsonpackage.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 sig/react_on_rails/**/*.rbs : In RBS type signature files, define type signatures that correspond to Ruby files in `lib/react_on_rails/`
Applied to files:
.size-limit.json
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
.size-limit.json
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
.size-limit.json
📚 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-09-25T17:44:48.481Z
Learnt from: ihabadham
Repo: shakacode/react_on_rails PR: 1825
File: package.json:74-75
Timestamp: 2025-09-25T17:44:48.481Z
Learning: stylelint-config-standard-scss automatically includes stylelint-scss as a transitive dependency through stylelint-config-recommended-scss, so there's no need to add stylelint-scss explicitly to devDependencies when using stylelint-config-standard-scss.
Applied to files:
package.json
⏰ 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). (8)
- GitHub Check: examples (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: build
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
🔇 Additional comments (6)
package.json (2)
89-90: Size-limit scripts added correctly.The "size" and "size:json" scripts are properly configured and align with the workflow's invocation. The scripts will leverage the .size-limit.json configuration.
24-24: Size-limit and GitHub Actions configuration is compatible.The workflow properly configures andresz1/size-limit-action@v1 with size-limit v12.0.0: Node 22 is pinned (exceeds v12's Node 20+ requirement), pnpm is explicitly set as the package_manager parameter, and the action supports this configuration. No version updates needed.
.github/workflows/bundle-size.yml (4)
3-21: Workflow triggers and path filtering look appropriate.The pull_request and push triggers on the master branch with filtered paths (packages/**, package.json, pnpm-lock.yaml, .size-limit.json, and the workflow file) ensure the workflow runs only when relevant bundle or configuration changes occur.
26-28: Permissions are correctly scoped.The workflow properly requests read access to contents and write access to pull-requests, enabling the size-limit action to post comments on PRs.
37-56: Setup and caching strategy follows best practices.Node setup, pnpm installation, and pnpm store caching with hash-based key are well-configured and will optimize build times on repeated runs.
57-62: Verify that the "build" script generates all required artifacts at the paths specified in .size-limit.json.The workflow invokes the "build" script (which runs workspace builds for react-on-rails, react-on-rails-pro, and react-on-rails-pro-node-renderer). Ensure:
- The build script generates .js files in each
lib/directory matching the glob patterns and specific paths in .size-limit.json- No additional build steps are needed before the size-limit action runs
- The build script is deterministic and produces consistent output
Run the verification script provided in the .size-limit.json review to confirm build output locations.
| [ | ||
| { | ||
| "name": "react-on-rails (full)", | ||
| "path": "packages/react-on-rails/lib/*.js", | ||
| "gzip": true, | ||
| "limit": "20 kB" | ||
| }, | ||
| { | ||
| "name": "react-on-rails (client)", | ||
| "path": "packages/react-on-rails/lib/ReactOnRails.client.js", | ||
| "gzip": true, | ||
| "limit": "1 kB" | ||
| }, | ||
| { | ||
| "name": "react-on-rails-pro (full)", | ||
| "path": "packages/react-on-rails-pro/lib/*.js", | ||
| "gzip": true, | ||
| "limit": "45 kB" | ||
| }, | ||
| { | ||
| "name": "react-on-rails-pro (client)", | ||
| "path": "packages/react-on-rails-pro/lib/ReactOnRails.client.js", | ||
| "gzip": true, | ||
| "limit": "2 kB" | ||
| }, | ||
| { | ||
| "name": "react-on-rails-pro-node-renderer", | ||
| "path": "packages/react-on-rails-pro-node-renderer/lib/*.js", | ||
| "gzip": true, | ||
| "limit": "15 kB" | ||
| } | ||
| ] |
There was a problem hiding this comment.
Add trailing newline to match coding guidelines.
Per the coding guidelines, all files must end with a newline character. The file currently ends at the closing bracket on line 32.
Apply this diff:
{
"name": "react-on-rails-pro-node-renderer",
"path": "packages/react-on-rails-pro-node-renderer/lib/*.js",
"gzip": true,
"limit": "15 kB"
}
-]
+]📝 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.
| [ | |
| { | |
| "name": "react-on-rails (full)", | |
| "path": "packages/react-on-rails/lib/*.js", | |
| "gzip": true, | |
| "limit": "20 kB" | |
| }, | |
| { | |
| "name": "react-on-rails (client)", | |
| "path": "packages/react-on-rails/lib/ReactOnRails.client.js", | |
| "gzip": true, | |
| "limit": "1 kB" | |
| }, | |
| { | |
| "name": "react-on-rails-pro (full)", | |
| "path": "packages/react-on-rails-pro/lib/*.js", | |
| "gzip": true, | |
| "limit": "45 kB" | |
| }, | |
| { | |
| "name": "react-on-rails-pro (client)", | |
| "path": "packages/react-on-rails-pro/lib/ReactOnRails.client.js", | |
| "gzip": true, | |
| "limit": "2 kB" | |
| }, | |
| { | |
| "name": "react-on-rails-pro-node-renderer", | |
| "path": "packages/react-on-rails-pro-node-renderer/lib/*.js", | |
| "gzip": true, | |
| "limit": "15 kB" | |
| } | |
| ] | |
| [ | |
| { | |
| "name": "react-on-rails (full)", | |
| "path": "packages/react-on-rails/lib/*.js", | |
| "gzip": true, | |
| "limit": "20 kB" | |
| }, | |
| { | |
| "name": "react-on-rails (client)", | |
| "path": "packages/react-on-rails/lib/ReactOnRails.client.js", | |
| "gzip": true, | |
| "limit": "1 kB" | |
| }, | |
| { | |
| "name": "react-on-rails-pro (full)", | |
| "path": "packages/react-on-rails-pro/lib/*.js", | |
| "gzip": true, | |
| "limit": "45 kB" | |
| }, | |
| { | |
| "name": "react-on-rails-pro (client)", | |
| "path": "packages/react-on-rails-pro/lib/ReactOnRails.client.js", | |
| "gzip": true, | |
| "limit": "2 kB" | |
| }, | |
| { | |
| "name": "react-on-rails-pro-node-renderer", | |
| "path": "packages/react-on-rails-pro-node-renderer/lib/*.js", | |
| "gzip": true, | |
| "limit": "15 kB" | |
| } | |
| ] |
🤖 Prompt for AI Agents
.size-limit.json lines 1 to 32: the file is missing a trailing newline at EOF;
to fix, open .size-limit.json and add a single newline character after the final
closing bracket on line 32 so the file ends with a newline, then save the file.
| [ | ||
| { | ||
| "name": "react-on-rails (full)", | ||
| "path": "packages/react-on-rails/lib/*.js", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .size-limit.jsonRepository: shakacode/react_on_rails
Length of output: 1042
🏁 Script executed:
# Check current state of lib directories
echo "=== Current lib directory contents ==="
for dir in packages/react-on-rails/lib packages/react-on-rails-pro/lib packages/react-on-rails-pro-node-renderer/lib; do
echo "Directory: $dir"
if [ -d "$dir" ]; then
ls -la "$dir" 2>/dev/null || echo "Directory exists but empty or no read permission"
else
echo "Directory does not exist (likely needs build)"
fi
echo ""
done
# Check the build scripts
echo "=== Checking package.json build scripts ==="
fd package.json | head -5 | xargs -I {} sh -c 'echo "File: {}"; head -30 {}'Repository: shakacode/react_on_rails
Length of output: 5177
🏁 Script executed:
# Check tsconfig.json files to understand build output
for file in packages/react-on-rails/tsconfig.json packages/react-on-rails-pro/tsconfig.json packages/react-on-rails-pro-node-renderer/tsconfig.json; do
echo "=== $file ==="
if [ -f "$file" ]; then
cat "$file"
else
echo "File not found"
fi
echo ""
doneRepository: shakacode/react_on_rails
Length of output: 477
🏁 Script executed:
# List source files for each package
echo "=== packages/react-on-rails/src ==="
if [ -d "packages/react-on-rails/src" ]; then
find packages/react-on-rails/src -type f \( -name "*.ts" -o -name "*.tsx" \) | sort
else
echo "Directory not found"
fi
echo ""
echo "=== packages/react-on-rails-pro/src ==="
if [ -d "packages/react-on-rails-pro/src" ]; then
find packages/react-on-rails-pro/src -type f \( -name "*.ts" -o -name "*.tsx" \) | sort
else
echo "Directory not found"
fi
echo ""
echo "=== packages/react-on-rails-pro-node-renderer/src ==="
if [ -d "packages/react-on-rails-pro-node-renderer/src" ]; then
find packages/react-on-rails-pro-node-renderer/src -type f \( -name "*.ts" -o -name "*.tsx" \) | sort
else
echo "Directory not found"
fiRepository: shakacode/react_on_rails
Length of output: 5131
🏁 Script executed:
# Check size-limit documentation or look at similar projects
# Also check if there's any build bundling happening (webpack, vite, etc.)
echo "=== Checking for build config files ==="
find packages/react-on-rails -maxdepth 2 -type f \( -name "webpack.config.*" -o -name "vite.config.*" -o -name "rollup.config.*" \) 2>/dev/null
echo ""
echo "=== Checking size-limit package ==="
rg -A 5 "size-limit" package.json | head -20
echo ""
echo "=== Checking if lib entries are meant to be entry point files ==="
grep -r "outFile\|bundle" packages/react-on-rails/tsconfig.json packages/react-on-rails-pro/tsconfig.json 2>/dev/null || echo "No bundling config found"
echo ""
echo "=== How TypeScript compiles multiple files ==="
echo "TypeScript with outDir compiles each .ts file to a separate .js file"
echo "The glob '*.js' will match ALL .js files in the directory, not just entry points"Repository: shakacode/react_on_rails
Length of output: 154
Replace glob patterns with specific entry point file names to ensure accurate size measurements.
The TypeScript configuration outputs all files from src/**/* to separate .js files in lib/. The glob patterns *.js (lines 4, 16, 28) will match not only the entry point files (ReactOnRails.full.js, ReactOnRails.client.js) but also all internal module files (e.g., Authenticity.js, ClientRenderer.js, ComponentRegistry.js). This causes size-limit to measure the sum of all .js files rather than just the intended entry points.
Change the glob patterns to specific file names:
- Line 4:
"packages/react-on-rails/lib/ReactOnRails.full.js" - Line 16:
"packages/react-on-rails-pro/lib/ReactOnRails.full.js" - Line 28:
"packages/react-on-rails-pro-node-renderer/lib/ReactOnRailsProNodeRenderer.js"
🤖 Prompt for AI Agents
In .size-limit.json around line 4 (and similarly adjust lines 16 and 28), the
glob "packages/react-on-rails/lib/*.js" causes size-limit to pick up all
compiled module files instead of the intended entry points; replace the glob
with the explicit entry path "packages/react-on-rails/lib/ReactOnRails.full.js"
(and likewise set line 16 to
"packages/react-on-rails-pro/lib/ReactOnRails.full.js" and line 28 to
"packages/react-on-rails-pro-node-renderer/lib/ReactOnRailsProNodeRenderer.js")
so size-limit measures only the intended bundle files.
Code Review: Bundle Size CI CheckThank you for implementing bundle size monitoring! This is a valuable addition. Here's my review: Critical Issues That Must Be Fixed1. Missing pnpm install stepThe workflow sets up pnpm caching but never runs 2. Inconsistent Node setup patternThis repo uses a custom 3. Duplicate build executionThe workflow runs build twice: once via Important Concerns4. Verify paths locallyPer CLAUDE.md testing requirements, please verify locally before merging: pnpm run build
pnpm run size # Should show 5 entriesThe test plan shows "CI workflow runs successfully on this PR" is still unchecked. 5. Consider SHA pinningUsing Minor Suggestions
SummaryMust fix: Add pnpm install step, use setup-node-with-retry action, fix duplicate build Let me know if you'd like help with these fixes! |
PR Review: Bundle Size CI CheckThanks for adding bundle size monitoring! This is a valuable addition to the CI pipeline. Here's my detailed review: ✅ Strengths
🔍 Issues & Recommendations1. CRITICAL: Package.json Scripts Don't Build First ❌In the final version, the "size": "size-limit",
"size:json": "size-limit --json"But the PR description says:
Problem: Running Evidence from commit history: One commit message says "Ensure size scripts build packages first", but the final code doesn't include the build step. Fix needed: Update package.json scripts to: "size": "pnpm run build && size-limit",
"size:json": "pnpm run build && size-limit --json"Or document that users must run 2. Workflow Action Version Pinning
|
Code Review: Bundle Size CI CheckThank you for adding bundle size monitoring! This is a valuable addition to prevent unexpected size regressions. Here's my detailed review: ✅ Strengths
🔴 Critical Issues1. Missing checkout before Node/pnpm setupIssue: Steps 21-29 set up Node and pnpm BEFORE checking out any code. This will fail because:
Fix: Move checkout BEFORE setup steps |
Critical Issue #2: Build runs twiceThe workflow runs Fix: Either remove the manual build on line 42, or change the Critical Issue #3: No pnpm cachingInstalling dependencies twice (base + PR branch) without caching adds 1-2 minutes to CI. See other workflows like |
AbanoubGhadban
changed the base branch from
master
to
feature/bundle-size-ci-base
|
📋 Testing RecommendationsAccording to CLAUDE.md guidelines, before merging you MUST:
From CLAUDE.md:
|
SummaryThis is a solid foundation for bundle size monitoring! 🎉 Priority fixes before merge:
Nice-to-haves for follow-up:
🔒 Security & Performance✅ Security: Good use of Great work on implementing this feature! The main blocker is Issue #1 (checkout order). Looking forward to seeing this merged! Review performed by Claude Code following project guidelines in CLAUDE.md |
size-limit report 📦
|
Bundle Size CI Check - Code ReviewThank you for implementing bundle size monitoring! This is a valuable addition to the CI pipeline. Here's my detailed feedback: ✅ Strengths
🔍 Issues & Concerns1. Dependencies ✅The workflow uses
Status: Dependencies are correctly included. Good! 2. Missing Error Handling in Node Script
|
Code Review: Bundle Size CI CheckHi @AbanoubGhadban! Thanks for adding bundle size monitoring. This is a valuable addition to prevent bundle size regressions. Here's my review: ✅ Positive Aspects
🔍 Observations & Suggestions1. Consistency Check (Minor)Line 86 uses 2. Error Handling (Enhancement)Consider adding error handling if - name: Build and measure base branch
run: |
pnpm run build
pnpm exec size-limit --json > /tmp/base-sizes.json
echo "Base branch sizes:"
cat /tmp/base-sizes.json
# Verify the file is valid JSON
node -e "JSON.parse(require('fs').readFileSync('/tmp/base-sizes.json', 'utf8'))"This would catch build failures early rather than failing in the dynamic limits step. 3. Testing Verification (Critical per CLAUDE.md)Your test plan shows:
Per the project's CRITICAL REQUIREMENTS in CLAUDE.md:
I see the CI is currently
4. Configuration CompletenessThe [
{
"name": "react-on-rails (gzip)",
"path": "packages/react-on-rails/lib/*.js",
"gzip": true
// Note: limits are set dynamically in CI (base + 0.5KB)
}
]5. Missing CHANGELOG Entry?This is a user-visible addition (new CI check, new npm scripts). Per CLAUDE.md:
Consider adding an entry under a "Developer Experience" or "Chores" section in the next release section of CHANGELOG.md. 🎯 Action ItemsBefore merging:
🚀 Overall AssessmentThis is a clean, focused PR that improves the project's CI infrastructure. The change from Security: ✅ No security concerns Great work! 🎉 |
Code Review: Bundle Size CI CheckThank you for adding bundle size monitoring! This is a valuable addition to catch performance regressions. Here's my review: ✅ What's Good
🚨 Critical Issue: Test File Left in Source CodeThe Location: Problem:
Evidence it's a test artifact: // Test file to verify bundle size CI catches increases
export const BLOAT_DATA_1 = 'xyzabc123456789!@#$%^&*()_+QWERTYUIOPASDFGHJKLZXCVBNM';
// ... 9 more similar exportsRequired action: This file must be deleted before merging. The CI workflow has already been tested (it correctly failed when bloat was added), so the test succeeded. 📋 Recommendations1. Remove the Test Filegit rm packages/react-on-rails/src/bloat.ts
git commit --amend --no-edit
git push --force-with-lease2. Consider Tree-Shaking VerificationThe current
3. DocumentationConsider adding to the README or CLAUDE.md:
4. CI Failure HandlingThe PR description shows:
This is expected to fail because bloat.ts was added intentionally to test detection. Once removed, this should pass. 🔍 Minor ObservationsWorkflow Design QuestionThe workflow checks out the base branch first, measures it, then checks out PR branch. This is clever but means:
Alternative approach to consider (for future iteration):
Size Threshold ValueThe 0.5KB threshold is reasonable for catching regressions, but:
✅ After FixingOnce
SummaryMust fix before merge:
Good to have:
Overall: Excellent addition to the CI pipeline! Just needs the test file cleaned up. Review performed by Claude Code following project conventions from CLAUDE.md |
Code Review✅ Overall AssessmentThis is a minimal, focused fix that improves consistency by using 📋 Review DetailsCode Quality: Excellent
Best Practices: Good
Potential Issues: None Found
Testing: Verified
💡 Suggestions1. Complete Test Plan ChecklistThe PR body shows: - [ ] CI workflow runs successfully on this PRSince the CI has now run and passed, consider updating this to: - [x] CI workflow runs successfully on this PR2. Consider Consistency Check Across WorkflowsWhile this change is correct, you might want to verify other workflow files don't have similar grep -r "npx" .github/workflows/ --exclude-dir=node_modulesIf found, those could be addressed in a follow-up PR for consistency. 🎯 RecommendationAPPROVE - This is a clean, minimal fix that improves tooling consistency. The change is:
Great work keeping the project's tooling consistent! 🚀 |
Code Review FeedbackThanks for implementing bundle size monitoring! This is a valuable addition to the CI pipeline. Here's my review: ✅ Strengths
🔍 Issues & Suggestions1. Missing size limits (Potential CI failure)Your {
"name": "react-on-rails (gzip)",
"path": "packages/react-on-rails/lib/*.js",
"gzip": true,
"limit": "20 kB" // ⚠️ Missing in actual file
}Why this matters: While the workflow sets dynamic limits at runtime, the local Recommendation: Add static limits as a fallback safety net, even though they'll be overridden by the workflow. 2. Triple measurement may be excessiveYou're measuring raw, gzip, AND brotli for each package (9 total measurements for 3 packages). Why this matters:
Recommendation: Consider tracking only gzip (or gzip + brotli) unless there's a specific reason to track raw sizes. 3. Workflow order could be optimizedThe workflow checks out the base branch first, then the PR branch. This works but means any workflow changes in the PR won't be used when measuring the base. Why this matters: If you update the workflow in the same PR (like you did here), the base measurement uses the old workflow logic. Recommendation: This is actually fine for your use case since you want consistency. Just noting it for awareness. 4. Missing validation stepThe PR description shows checklist item Critical per CLAUDE.md: The project guidelines emphasize NEVER claiming something is fixed without local verification, and avoiding CI iteration cycles. Recommendation:
5. No documentation updateUsers may not know about the new Recommendation: Consider adding a brief mention in the main README or developer docs about bundle size monitoring. 🧪 Testing RecommendationsPer CLAUDE.md's local testing requirements: # 1. Test size commands work
pnpm run size
pnpm run size:json
# 2. Verify paths are correct
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/*.js
# 3. Test that limits work (if you add them back)
# Temporarily set a very low limit and verify it fails
# 4. Verify workflow YAML syntax
gh workflow view "Bundle Size" --yaml📋 Security & Performance
🎯 Action Items Before Merge
Overall AssessmentThis is a solid implementation of bundle size monitoring! The dynamic limit approach is elegant. Main concerns are around completeness (missing limits, unchecked test plan) and potential noise from too many measurements. Once CI passes and you verify it works as expected, this will be a valuable addition to the project. Great work! 🚀 |
Code Review: Bundle Size CI CheckGreat work on adding bundle size monitoring! This is a valuable addition to the CI pipeline. Here's my detailed review: ✅ Strengths
🐛 Critical Issue: Missing Execute PermissionFile: bin/compare-bundle-sizes The script is missing executable permissions. Without this, users will get Permission denied when trying to run it. Fix: Run chmod +x bin/compare-bundle-sizes and commit the change.
|
Code Review: Bundle Size CI CheckThank you for this PR! Adding automated bundle size monitoring is a valuable addition. Strengths
Critical Issues1. Shell scripts missing executable permissions The bin scripts are not executable. Fix with:
This is critical - users won't be able to run these scripts directly. 2. Path references need verification The .size-limit.json references paths like packages/react-on-rails/lib/*.js. Per CLAUDE.md guidelines on managing file paths, these MUST be verified after build. If the directory structure changes in the future, this could silently break. Important Issues3. CI workflow skip-check logic - The tr -d removes ALL whitespace. Using xargs for trimming would be safer. 4. Missing validation in skip-bundle-size-check script - Should verify the branch hasn't already been skipped. 5. Inconsistent error handling - Line 91 in scripts/bundle-size.mjs has unnecessary return before process.exit(1). 6. Magic numbers - The calculation maxNameLen + 2 + 12 + 12 + 20 + 12 is fragile. Consider named constants. 7. Hardcoded ANSI colors - Consider using chalk library for better cross-platform compatibility. 8. No automated tests - Missing unit/integration tests for the utility scripts. 9. Missing documentation - Should add bundle size monitoring section to CLAUDE.md. 10. Command injection risk - compare-bundle-sizes accepts user input without validation. Action ItemsMust fix before merge:
Should fix: Overall AssessmentThis is a solid PR! The implementation is generally clean and well-thought-out. Main issues are missing executable permissions (critical) and path verification needed (per project guidelines). Once the critical issues are addressed, this will be a great addition to the CI pipeline. The local tooling especially is excellent for developers. Recommendation: Request changes for items 1-3, then approve after fixes. |
Code Review - Bundle Size CI CheckThank you for implementing bundle size monitoring! This is a valuable addition to prevent performance regressions. Here's my detailed review: ✅ Strengths
🐛 Critical Issues1. Missing Error Handling in Workflow (bundle-size.yml:67,79)The workflow assumes Recommendation: Add error handling: - name: Build and measure base branch
run: |
pnpm run build
pnpm exec size-limit --json > /tmp/base-sizes.json || (echo "Failed to measure base sizes" && exit 1)2. Shell Script Portability (bin/compare-bundle-sizes:51-52)The script uses Recommendation: Be more explicit about expected failures.
|
Summary
What's Tracked
Local Commands
pnpm run size- Check bundle sizespnpm run size:json- Get JSON outputTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.