Upgrade Shakapacker from 9.0.0 to 9.1.0 #1921
Conversation
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 34 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 (4)
WalkthroughBumped Shakapacker to 9.1.0 across gem and package manifests, added Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer / Commit
participant Lefthook as lefthook pre-commit
participant ESLintScript as bin/lefthook/eslint-lint
participant Git as Git staging
participant WebpackCfg as commonWebpackConfig.js
Dev->>Lefthook: commit
Lefthook->>ESLintScript: run eslint-lint (staged or all-changed)
ESLintScript->>Git: list changed JS/TS files
ESLintScript->>ESLintScript: run yarn eslint --fix
ESLintScript->>Git: git add fixed files
Note right of ESLintScript: exits early if no files
Dev->>WebpackCfg: build/test exec (config load)
WebpackCfg->>WebpackCfg: iterate module.rules
alt rule.test matches .scss/.module.scss
WebpackCfg->>WebpackCfg: rule.use.push(sassResources)
end
WebpackCfg->>Dev: augmented config returned
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 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: Shakapacker 9.0.0 to 9.1.0 UpgradeThank you for this upgrade! I've reviewed the PR and have the following feedback: OverviewThis PR upgrades Shakapacker from 9.0.0 to 9.1.0 across the main project's spec/dummy and react_on_rails_pro/spec/dummy applications. The changes are focused and include appropriate lockfile updates. ✅ Positives
|
PR Review: Upgrade Shakapacker from 9.0.0 to 9.1.0SummaryThis PR upgrades Shakapacker dependency from 9.0.0 to 9.1.0 across the main project and Pro dummy applications. Overall, this is a straightforward dependency upgrade that appears safe to merge once CI passes. Code Quality: ✅ Good
Notable Observations:
Potential Issues:
|
Code Review: Upgrade Shakapacker from 9.0.0 to 9.1.0SummaryThis PR upgrades Shakapacker from 9.0.0 to 9.1.0 across the project. Overall, this is a straightforward dependency update that looks good. However, there are a few important observations and recommendations. Positive Aspects
Issues and Recommendations1. CRITICAL: Missing CHANGELOG EntryAccording to CLAUDE.md, this dependency upgrade should be documented in the CHANGELOG:
2. Test Coverage Concern
3. Breaking Changes Investigation
4. Pro Package Major JumpThe Pro spec/dummy app jumping from 8.0.0 to 9.1.0 is significant:
Security Considerations✓ No security concerns identified - this is a routine dependency upgrade from a trusted source (ShakaCode's own package) Performance Considerations✓ Dependency updates often include performance improvements, though specifics depend on Shakapacker's changelog Code Quality✓ The changes follow the project's conventions:
Pre-Merge ChecklistBefore merging, please ensure:
Final VerdictApprove with minor modifications: This is a solid dependency update, but please address the CHANGELOG requirement and ensure comprehensive testing, especially for the Pro package's major version jump from 8.x to 9.x. |
Pull Request Review: Upgrade Shakapacker 9.0.0 → 9.1.0SummaryThis PR upgrades Shakapacker from 9.0.0 to 9.1.0 across the Ruby gem and both dummy apps (main and Pro). The upgrade appears straightforward and well-executed. Code Quality & Best Practices ✅Strengths:
Important Note:
Potential Issues & Concerns
|
Code Review - Shakapacker 9.0.0 → 9.1.0 UpgradeOverall Assessment✅ APPROVED - This is a well-executed dependency upgrade with proper lock file updates. Code Quality & Best PracticesStrengths:
Notable Changes:
Potential IssuesNone identified, but recommendations:
Performance Considerations✅ No performance concerns expected. Minor version bumps typically include:
The explicit Security Concerns✅ No security issues identified Upgrading dependencies is generally a security best practice. Recommend:
Test CoverageCurrent Status:
Recommendations:
Questions/Suggestions
Compliance with CLAUDE.md✅ All requirements met:
Once CI passes, this PR is ready to merge! 🚀 📋 Review Checklist
|
justin808
force-pushed
the
justin808/shakapacker-9.1.0
branch
from
e252ca0 to
c07e001
Compare
Pull Request Review: Upgrade Shakapacker from 9.0.0 to 9.1.0Overall Assessment✅ LGTM with minor observations - This is a straightforward dependency upgrade that follows best practices. Code Quality & Best PracticesStrengths:
Configuration Changes:
This is a breaking change requirement in Shakapacker 9.x that must be explicitly set. Potential Issues & Recommendations1. Missing Changelog Entry
2. Incomplete CI Verification
3. Version Consistency Question 🤔
Security Concerns✅ No security concerns identified
Performance Considerations✅ No performance concerns
Test CoverageCurrent Status:
Recommendations:
Action Items Before Merge
Compliance with CLAUDE.mdChecklist:
Conclusion: This is a well-executed dependency upgrade. Once CI passes and the changelog is updated, this should be safe to merge. The 🤖 Generated by Claude Code Review |
Code Review: Shakapacker 9.0.0 → 9.1.0 UpgradeOverviewThis PR upgrades Shakapacker from 9.0.0 to 9.1.0 across the project. Overall, the changes are clean and straightforward, but I have several observations and recommendations. ✅ Positive Aspects
🔍 Observations & Potential Issues1. Version Inconsistency Issue
|
Code Review - PR #1921: Upgrade Shakapacker from 9.0.0 to 9.1.0Overall AssessmentThis is a straightforward dependency upgrade PR that updates Shakapacker from 9.0.0 to 9.1.0. The changes are minimal and focused on version bumps across the project. Positive Aspects
Issues & ConcernsCritical: Incomplete Dependency UpdatesThe PR has NOT updated all Shakapacker references in the codebase. Several lock files still reference older versions: react_on_rails_pro directory still has outdated versions:
Recommendation: Run Missing: CHANGELOG UpdateAccording to the project's CLAUDE.md guidelines, dependency upgrades from 9.0.0 to 9.1.0 should be documented in CHANGELOG.md under the The CHANGELOG already has an entry for the 9.0.0 upgrade (line 36-41). Consider adding a similar entry for the 9.1.0 upgrade, such as: - **Shakapacker 9.1.0 Upgrade**: Upgraded Shakapacker from 9.0.0 to 9.1.0. [PR 1921](https://github.com/shakacode/react_on_rails/pull/1921) by [justin808](https://github.com/justin808).Configuration ChangesThe PR adds
Note: The main Dependencies AddedThe upgrade introduces Test CoverageThe test plan mentions:
Recommendation: Wait for CI to pass before merging to ensure no regressions. Action Items Before Merge
Security & PerformanceNo security concerns identified. This is a minor version bump that should maintain backward compatibility. Performance impact should be minimal. Code QualityThe PR follows the project's conventions and the changes are clean. However, the incomplete lock file updates need to be addressed before merging. Verdict: Approve with changes required. Please update the react_on_rails_pro lock files before merging. |
Pull Request Review: Upgrade Shakapacker from 9.0.0 to 9.1.0SummaryThis PR upgrades Shakapacker dependency across the project from versions 8.0.0/9.0.0 to 9.1.0. The changes include version bumps in Gemfiles, package.json files, and important configuration updates. Code Quality & Best Practices✅ Strengths:
Potential Issues
Testing RecommendationsBefore merging, ensure:
Performance ConsiderationsPositive:
To verify:
Security Concerns✅ No immediate security concerns identified The dependency upgrade should be reviewed for:
Documentation & ChangelogAccording to #### [PR 1921](https://github.com/shakacode/react_on_rails/pull/1921) by [justin808](https://github.com/justin808)
- Updated Shakapacker dependency from 9.0.0 to 9.1.0 (Pro specs upgraded from 8.0.0)
- Added `javascript_transpiler: babel` configuration to ensure compatibilityNote: The changelog entry format should follow the pattern: Additional Recommendations
Checklist for Author
ConclusionRecommendation: Approve with minor actions This is a clean dependency upgrade with proper configuration updates. The main action items are:
Great work on the comprehensive update across all test environments! 🚀 Review generated with Claude Code. Based on repository conventions from CLAUDE.md. |
Pull Request Review: Upgrade Shakapacker from 9.0.0 to 9.1.0✅ Code Quality and Best PracticesPositive aspects:
📋 Key Changes Analysis1. Dependency Updates (✅ Good)
2. Configuration Changes (
|
justin808
force-pushed
the
justin808/shakapacker-9.1.0
branch
from
e56d827 to
0d253df
Compare
Code Review: Shakapacker 9.0.0 to 9.1.0 UpgradeOverviewThis PR upgrades Shakapacker from 9.0.0 to 9.1.0 across the project. Overall, this is a straightforward and well-executed dependency upgrade with appropriate configuration updates. The changes are consistent and properly applied across all test environments. Positive Observations
Issues and Concerns🔴 Critical: Incomplete ConfigurationProblem: The
🟡 Missing CHANGELOG EntryProblem: According to CLAUDE.md, changelog entries should be added for dependency upgrades like this. The CHANGELOG.md already documents the 9.0.0 upgrade at lines 36-41, but there's no entry for the 9.1.0 upgrade. Recommendation: Add a changelog entry under the - **Shakapacker 9.1.0 Upgrade**: Upgraded Shakapacker from 9.0.0 to 9.1.0. This minor version update includes bug fixes and improvements. Updated webpack configuration in Pro dummy apps to use forEach pattern for better compatibility with multiple SCSS rules. [PR 1921](https://github.com/shakacode/react_on_rails/pull/1921) by [justin808](https://github.com/justin808).🟢 Minor: Webpack Configuration ImprovementThe change in Before (implicit from diff): const scssConfigIndex = baseClientWebpackConfig.module.rules.findIndex((config) =>
'.scss'.match(config.test),
);
baseClientWebpackConfig.module.rules[scssConfigIndex].use.push(sassLoaderConfig);After: baseClientWebpackConfig.module.rules.forEach((rule) => {
if (Array.isArray(rule.use) && rule.test && '.scss'.match(rule.test)) {
rule.use.push(sassLoaderConfig);
}
});This is more robust because:
Note that spec/dummy/config/webpack/commonWebpackConfig.js already uses the better forEach pattern (lines 25-54). Security Considerations✅ No security concerns identified:
Test CoverageThe PR description indicates:
Recommendation: Ensure CI passes, particularly:
Performance ConsiderationsNo performance concerns. Shakapacker 9.1.0 should maintain or improve upon 9.0.0 performance. Best Practices ComplianceBased on CLAUDE.md requirements: ✅ Follows project conventions:
RecommendationAPPROVE with minor changes: This is a solid upgrade PR. The only action item is adding the changelog entry. Once that's done and CI passes, this is ready to merge. Great work maintaining consistency across all the dummy apps and properly handling the configuration requirements! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Gemfile.lockis excluded by!**/*.lockreact_on_rails_pro/spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockreact_on_rails_pro/spec/execjs-compatible-dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockspec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
Gemfile.development_dependencies(1 hunks)react_on_rails_pro/Gemfile.development_dependencies(1 hunks)react_on_rails_pro/spec/dummy/config/shakapacker.yml(1 hunks)react_on_rails_pro/spec/dummy/config/webpack/commonWebpackConfig.js(1 hunks)react_on_rails_pro/spec/dummy/package.json(1 hunks)react_on_rails_pro/spec/execjs-compatible-dummy/config/shakapacker.yml(1 hunks)react_on_rails_pro/spec/execjs-compatible-dummy/package.json(1 hunks)spec/dummy/package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
📚 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:
react_on_rails_pro/spec/execjs-compatible-dummy/package.jsonspec/dummy/package.jsonreact_on_rails_pro/spec/dummy/package.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:
react_on_rails_pro/spec/execjs-compatible-dummy/package.jsonreact_on_rails_pro/Gemfile.development_dependenciesreact_on_rails_pro/spec/execjs-compatible-dummy/config/shakapacker.ymlreact_on_rails_pro/spec/dummy/package.jsonreact_on_rails_pro/spec/dummy/config/webpack/commonWebpackConfig.js
📚 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:
react_on_rails_pro/spec/execjs-compatible-dummy/package.jsonreact_on_rails_pro/Gemfile.development_dependenciesreact_on_rails_pro/spec/execjs-compatible-dummy/config/shakapacker.ymlreact_on_rails_pro/spec/dummy/package.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:
react_on_rails_pro/spec/execjs-compatible-dummy/package.jsonreact_on_rails_pro/Gemfile.development_dependenciesreact_on_rails_pro/spec/execjs-compatible-dummy/config/shakapacker.ymlreact_on_rails_pro/spec/dummy/config/shakapacker.ymlreact_on_rails_pro/spec/dummy/config/webpack/commonWebpackConfig.js
⏰ 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). (6)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build (22)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20, minimum)
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (6)
spec/dummy/package.json (1)
54-54: LGTM! Version consistency maintained.The shakapacker upgrade to 9.1.0 is consistent across all package.json files in the codebase.
Based on learnings
react_on_rails_pro/spec/dummy/package.json (1)
66-66: Version upgrade looks good.The shakapacker upgrade from 8.0.0 to 9.1.0 maintains consistency with other package.json files. The test plan indicates tests pass locally.
Based on learnings
Gemfile.development_dependencies (1)
3-3: LGTM!The shakapacker gem version update to 9.1.0 is consistent with the package.json updates across the project.
react_on_rails_pro/spec/execjs-compatible-dummy/package.json (1)
33-33: Version update is consistent.The shakapacker upgrade to 9.1.0 maintains version consistency across the project.
Based on learnings
react_on_rails_pro/Gemfile.development_dependencies (1)
10-10: LGTM!The shakapacker gem version update is consistent with the corresponding package.json updates in the react_on_rails_pro directory.
react_on_rails_pro/spec/dummy/config/shakapacker.yml (1)
9-9:javascript_transpiler: babelis optional—Babel is the default transpiler in Shakapacker.Babel is the default/transpiler Shakapacker expects, but it's not strictly required. Shakapacker ships a Babel preset and lists Babel packages as peer dependencies, yet you can opt into alternatives (SWC, esbuild-loader) or provide your own Babel config. Adding this setting explicitly works fine for clarity about the configured transpiler, or it can be removed as redundant if the default is sufficient.
Update Shakapacker dependency to version 9.1.0 across the project: - Updated Gemfile.development_dependencies - Updated spec/dummy/package.json - Updated react_on_rails_pro/spec/dummy/package.json - Ran bundle install and yarn install to update lock files 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Update Shakapacker from 8.0.0 to 9.1.0 in: - react_on_rails_pro/Gemfile.development_dependencies - react_on_rails_pro/spec/execjs-compatible-dummy/package.json This fixes the version mismatch error in CI where the gem was 8.0.0 but the node package was detected as 9.1.0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Shakapacker 9.1.0 defaults to SWC transpiler, but the Pro dummy apps don't have swc-loader installed. Configure them to use Babel instead by adding javascript_transpiler: babel to shakapacker.yml. This fixes the build error: "Your Shakapacker config specified using swc, but swc-loader package is not installed." Updated: - react_on_rails_pro/spec/dummy/config/shakapacker.yml - react_on_rails_pro/spec/execjs-compatible-dummy/config/shakapacker.yml 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Shakapacker 9.1.0 changed how webpack rules are structured. The findIndex approach was failing because it returned -1 when no SCSS rule was found, causing "Cannot read properties of undefined". Updated to use forEach pattern (matching main spec/dummy) which: - Safely iterates through all rules - Checks if rule.use is an array before accessing it - Handles multiple SCSS rules if present This fixes the build error: "TypeError: Cannot read properties of undefined (reading 'push')" 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Document the Shakapacker 9.1.0 upgrade in the Changed section, including the webpack configuration improvements for Pro dummy apps. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Changed from incorrect '.scss'.match(rule.test) to correct
rule.test.test('.scss') in webpack configuration.
The previous code was calling match() on a string with a RegExp
parameter, when it should use the RegExp's test() method to check
if a string matches the pattern.
Fixed in both:
- spec/dummy/config/webpack/commonWebpackConfig.js
- react_on_rails_pro/spec/dummy/config/webpack/commonWebpackConfig.js
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
Changed rule.test.test('.scss') to rule.test.test('example.scss')
because webpack SCSS rules typically use patterns like /\.scss$/
which require a full filename to match properly, not just the
extension.
The string '.scss' doesn't match /\.scss$/ but 'example.scss' does.
This fixes the test failures where CSS wasn't being loaded, making
React components appear as empty divs (not visible).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
The forEach approach with RegExp.test() broke Pro dummy tests.
Reverting to the original findIndex approach that was working
on master, which uses '.scss'.match(rule.test).
While '.scss'.match(rule.test) is technically backwards (should be
rule.test.test('.scss')), it works correctly and tests pass.
The main spec/dummy still uses the improved forEach pattern since
it was already updated as part of the Shakapacker 9.0 upgrade.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
justin808
force-pushed
the
justin808/shakapacker-9.1.0
branch
from
d9e719e to
dee2edf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Gemfile.lockis excluded by!**/*.lockreact_on_rails_pro/spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockreact_on_rails_pro/spec/execjs-compatible-dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockspec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
CHANGELOG.md(1 hunks)Gemfile.development_dependencies(1 hunks)react_on_rails_pro/Gemfile.development_dependencies(1 hunks)react_on_rails_pro/spec/dummy/config/shakapacker.yml(1 hunks)react_on_rails_pro/spec/dummy/package.json(1 hunks)react_on_rails_pro/spec/execjs-compatible-dummy/config/shakapacker.yml(1 hunks)react_on_rails_pro/spec/execjs-compatible-dummy/package.json(1 hunks)spec/dummy/config/webpack/commonWebpackConfig.js(1 hunks)spec/dummy/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- CHANGELOG.md
- react_on_rails_pro/Gemfile.development_dependencies
- react_on_rails_pro/spec/execjs-compatible-dummy/package.json
- react_on_rails_pro/spec/execjs-compatible-dummy/config/shakapacker.yml
- react_on_rails_pro/spec/dummy/config/shakapacker.yml
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
📚 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:
spec/dummy/package.jsonreact_on_rails_pro/spec/dummy/package.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:
react_on_rails_pro/spec/dummy/package.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:
react_on_rails_pro/spec/dummy/package.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:
react_on_rails_pro/spec/dummy/package.jsonspec/dummy/config/webpack/commonWebpackConfig.js
⏰ 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). (9)
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: markdown-link-check
- GitHub Check: claude-review
🔇 Additional comments (3)
spec/dummy/package.json (1)
54-54: LGTM! Version upgrade is consistent.The shakapacker upgrade from 9.0.0 to 9.1.0 aligns with the PR objective and maintains version consistency with other package.json files in the codebase.
Gemfile.development_dependencies (1)
3-3: LGTM! Gem version consistent with package updates.The shakapacker gem upgrade to 9.1.0 is consistent with the JavaScript package updates throughout the PR.
react_on_rails_pro/spec/dummy/package.json (1)
66-66: Verify the major version upgrade.This upgrades shakapacker from 8.0.0 to 9.1.0, which includes a major version bump. While the related babel transpiler configuration changes suggest migration work has been done, please ensure this environment's tests pass with the upgrade.
Based on learnings
|
@claude review this PR. |
|
Claude finished @justin808's task —— View job PR Review: Upgrade Shakapacker from 9.0.0 to 9.1.0I've reviewed this PR and found both positive aspects and critical issues that need to be addressed before merging. ✅ Positive Aspects
🔴 Critical Issue: CI FailuresThe CI is currently failing due to a webpack configuration error in the Pro dummy app. Here's the problem: Error DetailsRoot CauseThe file Current code (lines 29-32): const scssConfigIndex = baseClientWebpackConfig.module.rules.findIndex((config) =>
'.scss'.match(config.test),
);
baseClientWebpackConfig.module.rules[scssConfigIndex].use.push(sassLoaderConfig);Problems:
Solution RequiredThe Pro dummy app's webpack config needs to be updated to use the same forEach approach that was correctly applied to the main spec/dummy app at // Process webpack rules in single pass for efficiency
baseClientWebpackConfig.module.rules.forEach((rule) => {
if (Array.isArray(rule.use)) {
// Add sass-resources-loader to all SCSS rules (both .scss and .module.scss)
if (rule.test && rule.test.test('example.scss')) {
rule.use.push(sassLoaderConfig);
}
}
});Note: The test should use
|
Updated the rule detection to test against both 'example.scss' and 'example.module.scss' so sass-resources-loader is applied to both global SCSS rules and CSS Module SCSS rules. This fixes test failures where CSS Modules weren't being processed correctly because the detection only matched regular .scss files. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The findIndex approach with '.scss'.match(config.test) was failing in Shakapacker 9.1.0 because the webpack rule structure changed. Updated Pro dummy to use the same forEach pattern as main dummy: - Tests both 'example.scss' and 'example.module.scss' - Safely checks if rule.use is an array before pushing - Handles both regular SCSS and CSS Module SCSS files This fixes the build error: "TypeError: Cannot read properties of undefined (reading 'push')" 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
react_on_rails_pro/spec/dummy/config/webpack/commonWebpackConfig.js (1)
32-33: Consider guarding against duplicate loaders.If this configuration is processed multiple times (e.g., during hot reloads or when imported by multiple configs),
sassLoaderConfigcould be added repeatedly to the same rule.Apply this diff to prevent duplicates:
- if (Array.isArray(rule.use) && rule.test instanceof RegExp && (rule.test.test('example.scss') || rule.test.test('example.module.scss'))) { + const hasSassResourcesLoader = rule.use.some(use => use.loader === 'sass-resources-loader'); + if (Array.isArray(rule.use) && rule.test instanceof RegExp && !hasSassResourcesLoader && (rule.test.test('example.scss') || rule.test.test('example.module.scss'))) { rule.use.push(sassLoaderConfig); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
react_on_rails_pro/spec/dummy/config/webpack/commonWebpackConfig.js(1 hunks)spec/dummy/config/webpack/commonWebpackConfig.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/dummy/config/webpack/commonWebpackConfig.js
🧰 Additional context used
🧠 Learnings (3)
📚 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:
react_on_rails_pro/spec/dummy/config/webpack/commonWebpackConfig.js
📚 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:
react_on_rails_pro/spec/dummy/config/webpack/commonWebpackConfig.js
📚 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:
react_on_rails_pro/spec/dummy/config/webpack/commonWebpackConfig.js
⏰ 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). (9)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build
- GitHub Check: lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
- GitHub Check: markdown-link-check
| baseClientWebpackConfig.module.rules.forEach((rule) => { | ||
| if (Array.isArray(rule.use) && rule.test && (rule.test.test('example.scss') || rule.test.test('example.module.scss'))) { | ||
| rule.use.push(sassLoaderConfig); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Add a type guard to prevent potential runtime errors.
The code assumes rule.test is a RegExp, but if it's a function or another type (which webpack allows), calling .test() will throw a TypeError.
Apply this diff to add a safety check:
// Add sass-resources-loader to all SCSS rules (both .scss and .module.scss)
baseClientWebpackConfig.module.rules.forEach((rule) => {
- if (Array.isArray(rule.use) && rule.test && (rule.test.test('example.scss') || rule.test.test('example.module.scss'))) {
+ if (Array.isArray(rule.use) && rule.test instanceof RegExp && (rule.test.test('example.scss') || rule.test.test('example.module.scss'))) {
rule.use.push(sassLoaderConfig);
}
});📝 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.
| baseClientWebpackConfig.module.rules.forEach((rule) => { | |
| if (Array.isArray(rule.use) && rule.test && (rule.test.test('example.scss') || rule.test.test('example.module.scss'))) { | |
| rule.use.push(sassLoaderConfig); | |
| } | |
| }); | |
| baseClientWebpackConfig.module.rules.forEach((rule) => { | |
| if (Array.isArray(rule.use) && rule.test instanceof RegExp && (rule.test.test('example.scss') || rule.test.test('example.module.scss'))) { | |
| rule.use.push(sassLoaderConfig); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In react_on_rails_pro/spec/dummy/config/webpack/commonWebpackConfig.js around
lines 31 to 35, the loop calls rule.test.test(...) assuming rule.test is a
RegExp; add a type guard to avoid TypeError when rule.test is a function or
other type. Update the condition to verify Array.isArray(rule.use) && rule.test
instanceof RegExp && (rule.test.test('example.scss') ||
rule.test.test('example.module.scss')), so .test is only invoked when rule.test
is a RegExp.
The commonWebpackConfig.js file had a long conditional that needed to be split across multiple lines according to Prettier rules in the Pro package. This was not caught by pre-commit hooks because the root .prettierignore excludes react_on_rails_pro/, but CI runs linting separately in that directory with its own rules. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Previously, pre-commit hooks only ran RuboCop, Prettier, and trailing newline checks, but did NOT run ESLint. This meant ESLint errors could slip through and only be caught in CI. Changes: - Created bin/lefthook/eslint-lint hook script that auto-fixes ESLint issues - Added ESLint hook to .lefthook.yml pre-commit configuration - Updated CLAUDE.md to document: - The complete list of pre-commit hooks that run - The monorepo structure with separate Pro package linting - Why CI lints both directories separately The hook follows the same pattern as other hooks: - Detects changed JS/TS files (staged + unstaged + untracked) - Runs ESLint with --fix to auto-correct issues - Re-stages fixed files automatically This prevents the frustrating situation where local commits succeed but CI fails on ESLint violations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.lefthook.yml(1 hunks)CLAUDE.md(2 hunks)bin/lefthook/eslint-lint(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.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). (9)
- GitHub Check: build
- GitHub Check: lint-js-and-ruby
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: markdown-link-check
🔇 Additional comments (2)
bin/lefthook/eslint-lint (1)
1-3: Good use of bash strict mode.The shebang and
set -euo pipefailare excellent practices for catching errors early in shell scripts..lefthook.yml (1)
14-16: LGTM: ESLint hook integration is consistent.The new ESLint hook follows the existing pattern and is properly integrated alongside other linting hooks. The
all-changedcontext aligns with the other hooks in this file.
bin/lefthook/eslint-lint
Outdated
|
|
||
| # Re-stage files if running on staged or all-changed context | ||
| if [ "$CONTEXT" = "staged" ] || [ "$CONTEXT" = "all-changed" ]; then | ||
| echo $files | xargs -r git add |
There was a problem hiding this comment.
Consider portability: xargs -r is GNU-specific.
The -r flag on line 25 prevents xargs from running when there's no input, but it's a GNU extension not available on macOS by default. The revised solution above using -0 with null-delimited input addresses this, as the array expansion will naturally handle the empty case.
🤖 Prompt for AI Agents
In bin/lefthook/eslint-lint around line 25, avoid the GNU-only xargs -r flag;
instead either guard the call so it only runs when there are files or feed
null-delimited input to a portable xargs. Replace the current pipeline that uses
xargs -r with a pattern that prints/outputs filenames separated by NUL and
invokes xargs -0 git add, or wrap the git-add pipeline in a simple shell
conditional that checks for non-empty files before calling xargs; this removes
the -r dependency and keeps behavior safe on macOS.
Shakapacker 9 changed the CSS Modules default configuration from default exports to named exports (namedExport: true). This breaks existing code that uses `import styles from './style.module.css'` syntax. This commit adds webpack configuration to override Shakapacker 9's CSS Modules settings and restore backward compatibility with the v8 behavior. Changes: - Override namedExport: false to support default imports - Set exportLocalsConvention: 'camelCase' to match existing usage - Add configuration in Pro dummy commonWebpackConfig - Matches the fix already applied to regular dummy in previous commits This fixes CI test failures where React components weren't rendering because CSS module imports were failing at runtime. Related to Shakapacker upgrade from 8.0.0 to 9.1.0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add comprehensive debugging guide for Webpack/Shakapacker configuration issues, including: - Debug script templates for inspecting webpack rules - Step-by-step debugging workflow - Common issues and solutions (especially CSS Modules in Shakapacker 9) - Best practices for analyzing webpack configuration This documentation captures the debugging approach used to identify and fix the CSS Modules issue during the Shakapacker 9.1.0 upgrade. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The previous hooks only linted files in the root directory, but the react_on_rails_pro/ directory has its own ESLint and Prettier configs that need to be run separately. This caused CI failures when Pro files had linting issues that weren't caught locally. Changes: 1. Updated bin/lefthook/eslint-lint to: - Separate files into root and Pro directories - Run ESLint in Pro directory with Pro's config for Pro files - Track exit codes and fail if either check fails 2. Updated bin/lefthook/prettier-format to: - Separate files into root and Pro directories - Run Prettier in Pro directory with Pro's config for Pro files 3. Fixed Prettier formatting in commonWebpackConfig.js: - Condensed multi-line conditional to single line per Pro's config This ensures local pre-commit hooks match CI behavior exactly, preventing the frustrating situation where commits succeed locally but fail in CI. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Code Review: Shakapacker 9.0.0 to 9.1.0 UpgradeThank you for this upgrade PR! Overall, this is a well-structured dependency upgrade with good attention to backward compatibility. Here's my detailed review: ✅ Strengths1. Comprehensive Dependency Updates
2. Excellent Documentation
3. Improved Developer Experience
4. Backward Compatibility
🔍 Code Quality ObservationsWebpack Configuration Consistencyspec/dummy/config/webpack/commonWebpackConfig.js:28 - Good improvement: if (rule.test && (rule.test.test('example.scss') || rule.test.test('example.module.scss'))) {Using react_on_rails_pro/spec/dummy/config/webpack/commonWebpackConfig.js:31-39 - Consider consolidating: Recommendation: Consider refactoring the Pro version to use the same single-pass pattern as the open-source version for consistency and better performance. 🐛 Potential Issues1. Bash Script Quoting (bin/lefthook/eslint-lint)Lines 26, 34, 45, 56: printf " %s\n" $root_files # Missing quotes
echo $root_files | xargs -r git add # Missing quotesIssue: Unquoted variables can cause word splitting if filenames contain spaces. Fix: Quote the variables: printf " %s\n" "$root_files"
echo "$root_files" | xargs -r git addSame issue in bin/lefthook/prettier-format at lines 24, 33, 52, 61. 2. Error Handling in Bash Scriptsbin/lefthook/eslint-lint:28, 50: if \! yarn run eslint $root_files --report-unused-disable-directives --fix; then
exit_code=1
fiIssue: The script continues executing even after failures, potentially masking issues. Consideration: With 🔒 Security Considerations✅ No security concerns identified:
⚡ Performance ConsiderationsPositive:
Suggestion:
🧪 Test CoverageCurrent State:
Recommendations:
📝 Additional Suggestions1. CHANGELOG.md FormatLine 44: Good formatting! The PR link format matches project conventions. Consider adding a note about the breaking change potential if users were relying on Shakapacker defaults. 2. Missing Trailing Newlines?According to CLAUDE.md requirements, all files must end with newlines. The CI will catch this, but verify: bundle exec rubocop3. Testing the New ESLint HookThe new
📋 Checklist Before Merge
🎯 Overall AssessmentRating: 8.5/10 This is a solid upgrade PR with good attention to compatibility and developer experience. The main issues are:
The comprehensive documentation additions are excellent and will help future maintainers troubleshoot webpack issues. Recommendation: Approve with minor fixes ✅ 📚 References
|
Summary
Test plan
🤖 Generated with Claude Code
This change is
Summary by CodeRabbit