Fix duplicate messages in install generator and address code review issues #2030
Conversation
WalkthroughCentralizes JavaScript dependency management into a new JsDependencyManager and updates generators to delegate to it; renames a gitignore-related method; switches Shakapacker defaults to SWC; replaces scss-lint with stylelint (configs, CI, rake, dev deps); adds tests and bumps package/library versions. Changes
Sequence Diagram(s)sequenceDiagram
participant Gen as Generator (Base/Install)
participant JsDM as JsDependencyManager
participant Pkg as package_json gem
Note over Gen,JsDM: Generator delegates JS dependency work
Gen->>JsDM: setup_js_dependencies(options)
JsDM->>JsDM: assemble dependency lists (REACT, CSS, DEV, RSPACK?, TYPESCRIPT?)
JsDM->>Pkg: add/ensure packages + install_js_dependencies()
Pkg-->>JsDM: install result (success/failure)
JsDM-->>Gen: status / user guidance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 - PR #2030SummaryThis PR successfully addresses the code review feedback from #1788 by refactoring JavaScript dependency management into a shared module and improving error handling. The changes demonstrate solid software engineering practices with good test coverage. ✅ Strengths1. Excellent Code Reuse via Module ExtractionThe new
2. Improved Error Messages with ContextError messages now include:
This is a major improvement over generic error messages. 3. Comprehensive Test CoverageThe new
Particularly good: 4. Clean Instance Variable InitializationThe 5. Consistent TypeScript Dependency HandlingThe refactoring in 🔍 Observations & Minor Suggestions1. Potential Redundant Instance Variable SettingIn
Observation: This works correctly but sets the same variable 4 times. Consider setting it once in 2. Error Handling Strategy: Raise vs WarningThe module now raises exceptions when dependency installation fails ( Question for consideration: Should the behavior be consistent? The comment says "This should not happen" which suggests these are exceptional conditions worth raising. This seems reasonable given shakapacker is a hard dependency. 3. YARD Documentation QualityThe module has excellent high-level documentation ( Minor suggestion: The module documentation could mention that methods raise exceptions on failure (though the method implementations make this clear). 4. Test Mock SpecificityIn allow(install_generator).to receive_messages(add_js_dependency: true, add_js_dependencies_batch: true,
install_js_dependencies: true)This is clean, but consider whether you want to test each method's success/failure independently in future tests. 🔒 Security Considerations✅ No security concerns identified
⚡ Performance Considerations✅ No performance issues
🧪 Test Coverage AssessmentStrong test coverage with room for one addition: ✅ Well covered:
💡 Potential addition (not blocking):
📋 Code Quality & Best PracticesAdheres to React on Rails Standards:
Ruby Best Practices:
🎯 Final AssessmentThis is high-quality refactoring work that addresses all the code review feedback comprehensively. Recommendation: ✅ ApproveThe PR:
Minor Follow-ups (Non-blocking):
Great work on this refactoring! 🎉 The codebase is much cleaner and more maintainable now. Review performed by Claude Code - analyzed code structure, tests, error handling, and adherence to React on Rails conventions per CLAUDE.md |
Code Review for PR #2030OverviewThis PR successfully addresses the code review feedback from #1788 by extracting duplicated JS dependency management logic into a shared module and improving error handling. The refactoring is well-executed and improves code maintainability. ✅ Strengths1. Excellent Code Organization
2. Improved Error Handling
3. Better Type Safety & Initialization
4. Excellent Test Coverage
🔍 Observations & Minor Suggestions1. Hardcoded Constants vs ConfigurationREACT_DEPENDENCIES = %w[
react
react-dom
@babel/preset-react
# ...
].freezeObservation: These dependencies are now hardcoded constants in the module. This is fine for stable dependencies, but consider if there's ever a need to override these per-generator. Recommendation: Current approach is good. If customization is needed later, could add optional override parameters. 2. Error Handling PhilosophyThe new code raises exceptions when raise "Failed to add React dependencies (...) via package_json gem. " \
"This indicates shakapacker dependency may not be properly installed."Observation: This is a breaking change from the previous warning-based approach. The old code used Questions:
Recommendation: The strict approach is better if shakapacker is required. Consider enhancing the error message: raise "Failed to add React dependencies via package_json gem.\n" \
"This indicates shakapacker may not be properly installed.\n" \
"Please ensure 'shakapacker' is in your Gemfile and run 'bundle install'."3. Method VisibilityAll methods in Recommendation: Current approach is fine. The documentation helps maintainers understand the module's internal structure. 4. Version Handling SimplificationThe old code had this logic: react_on_rails_pkg = if ReactOnRails::VERSION.match?(major_minor_patch_only)
["react-on-rails@#{ReactOnRails::VERSION}"]
else
puts "Adding the latest react-on-rails NPM module..."
["react-on-rails"]
endThe new code always uses exact version: react_on_rails_pkg = "react-on-rails@#{ReactOnRails::VERSION}"Question: Was the version regex check unnecessary? What if Recommendation: If pre-release versions exist, consider keeping the version validation. Otherwise, the simplification is good. 5. Trailing Newline# No longer needed since package_json gem handles package manager detection
end
end
endCritical: According to CLAUDE.md: "ALWAYS ensure files end with a newline character" Action Required: Verify 🧪 Testing RecommendationsCurrent Testing ✅
Additional Test Scenarios to Consider
🚨 Pre-Merge ChecklistBased on CLAUDE.md requirements:
🎯 Final VerdictRecommendation: APPROVE with minor verification This is a solid refactoring that:
Before Merge:
Post-Merge Considerations:
Great work on this refactoring! The extraction of 🤖 Review generated with Claude Code |
Code Review - PR #2030I've reviewed this PR addressing code review feedback from #1788. Overall, this is a high-quality refactoring that successfully addresses the original feedback while improving code organization and maintainability. ✅ Strengths1. Excellent Code Organization
2. Improved Error Handling
3. Better Instance Variable Management
4. Comprehensive Test Coverage
5. TypeScript Dependency Consistency
🔍 Areas for Consideration1. Module Dependency Assumption (Minor) In @added_dependencies_to_package_json = false unless defined?(@added_dependencies_to_package_json)Observation: This assumes the instance variable might already be defined by the including class. While the comment explains this, it creates a subtle coupling. Suggestion: Consider whether this state tracking is necessary at all, since the module documentation states "package_json gem is always available via shakapacker". If it's always available, why track whether it was used? 2. Error Handling Strategy (Minor) The module uses raise "Failed to add react-on-rails package via package_json gem..."Observation: While the comment says "This should not happen", raising exceptions will halt the generator entirely. The previous implementation used warnings and allowed continuation. Question: Is this the desired behavior? Should we:
Recommendation: The fail-fast approach is actually better for catching misconfigurations early. However, consider adding a more helpful error message suggesting how to verify shakapacker installation. 3. Constant Definitions (Style) The constants ( Observation: react_on_rails_pkg = "react-on-rails@#{ReactOnRails::VERSION}"Suggestion: For consistency, consider if Rspack dependencies should also be constants (currently they're defined in the deleted code, so this might need to be addressed separately if Rspack support is still needed). 4. Test Coverage Gap (Minor) The new spec file tests message deduplication and method organization excellently, but: Observation: No integration tests verifying that the actual package installations work correctly through the new module. Suggestion: Consider adding a test that mocks 5. Documentation Completeness Observation: The module documentation states including classes must provide Suggestion: Update the "Required Methods" section to explicitly state: # == Required Modules
# Including classes must also include GeneratorHelper module🔒 Security ConsiderationsNo security concerns identified. The code:
⚡ Performance ConsiderationsPositive impact:
📋 Code Quality & Best PracticesExcellent adherence to project standards:
🎯 RecommendationsBefore Merging:
Future Enhancements (not blockers):
✅ Final VerdictLGTM with minor suggestions. This PR successfully:
The refactoring is well-executed and represents a significant improvement to the codebase. The minor suggestions above are non-blocking and can be addressed in follow-up PRs if desired. Great work! 🎉 |
Code Review - PR #2030Overall Assessment✅ Excellent refactoring work! This PR successfully addresses the code review feedback from #1788 and implements a clean, DRY solution by extracting duplicate JS dependency management logic into a shared module. ✅ Strengths1. Clean Module Extraction
2. Improved Error Messages
3. Comprehensive Test Coverage
4. Better Code Organization
🔍 Issues & Concerns1. Critical: Instance Variable Management Pattern
|
justin808
force-pushed
the
claude/pr-1788-rebased
branch
from
d988757 to
850deed
Compare
|
/run-skipped-ci |
|
🚀 Full CI Mode Enabled ✅ All checks are already running - nothing to do! All CI checks are already running on this PR. Added the |
Additional Update: SWC as Default Webpack LoaderPer code review feedback, I've made additional changes to properly separate SWC from Babel and leverage Shakapacker for build tool management: Key Changes
Impact
All tests continue to pass! ✅ |
Addresses code review feedback from PR #1788: 1. Created shared JsDependencyManager module to eliminate code duplication between base_generator.rb and install_generator.rb 2. Added comprehensive test coverage for: - Message deduplication (ensuring messages appear exactly once) - NPM install command deduplication - Proper module inclusion and method organization 3. Improved method organization in install_generator.rb This refactoring follows DRY principles and makes the codebase more maintainable by consolidating shared dependency management logic. Co-authored-by: Justin Gordon <[email protected]>
- Fixed missing newline at end of js_dependency_manager.rb - Removed recursive add_js_dependencies wrapper in BaseGenerator - Fixed namespace issues in tests (ReactOnRails::Generators::InstallGenerator) - Improved RSpec test structure with proper matchers - Optimized package_json gem usage: - Added package_json_available? check - Better separation between single and batch dependency adds - Improved fallback handling for when package_json is unavailable - Use package_json.manager.install when available Co-authored-by: Justin Gordon <[email protected]>
- Add missing generator class declaration for proper test initialization - Fix package_json gem mocking to match actual implementation behavior - Adjust system call expectations to align with fallback path execution Co-authored-by: Justin Gordon <[email protected]>
- Fixed package_json gem integration by ensuring arrays are passed to manager.add() - Fixed test mocking issues with webpack config file reading - Corrected test expectations to match actual behavior - Fixed mock setup for package_json manager and fallback scenarios - All message_deduplication_spec tests now pass 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Fix RuboCop VerifiedDoubles violations by using proper test doubles - Simplify test mocking approach to focus on intended behavior - Remove accidental react-on-rails dependency from package.json that broke version checker tests - Use receive_messages for cleaner multiple stub setup - Fix test to properly verify package_json integration without over-mocking The tests now correctly verify: - Message deduplication works properly - package_json gem integration works when available - Fallback to direct npm commands works when needed 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…e documentation Address Justin's code review suggestions: - Extract REACT_DEPENDENCIES, CSS_DEPENDENCIES, and DEV_DEPENDENCIES as frozen module constants - Add comprehensive module documentation explaining required instance variables and methods - Update all dependency methods to use the new constants for better maintainability 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…y conversions Address Justin's minor optimization suggestion: - Use add_js_dependencies_batch instead of add_js_dependency for single package - Work with arrays consistently throughout the method flow - Eliminates unnecessary string→array→string conversions - Maintains identical functionality with improved efficiency 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Address CodeRabbit suggestion #1: - Remove conditional logic that could skip final install - Always run install_js_dependencies to ensure packages are properly installed - Update test to reflect the improved behavior - Prevents scenarios where no install runs when package_json methods fail 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…e_json checks Since react_on_rails requires shakapacker (>= 6.0) and shakapacker includes package_json as a dependency, the package_json gem is always available. Changes: - Remove package_json_available? method and all fallback logic - Simplify version detection to always use exact gem version - Remove detect_and_run_package_manager_install (package_json handles this) - Remove @ran_direct_installs tracking (no longer needed) - Update tests to remove fallback scenarios - Update documentation to reflect guaranteed package_json availability This eliminates ~60 lines of redundant code and makes the module more maintainable and less error-prone. Co-authored-by: Justin Gordon <[email protected]>
This commit addresses all code review feedback: 1. Improved instance variable initialization pattern - Changed from @added_dependencies_to_package_json ||= false - To: @added_dependencies_to_package_json = false unless defined?(@added_dependencies_to_package_json) - Added clear documentation explaining the initialization is done by the module 2. Made error messages more specific - All raise statements now include which packages failed - Added context that failure indicates shakapacker may not be properly installed - Error messages now help users understand what went wrong 3. Documented add_js_dependency method usage - Added YARD-style documentation - Clarified it's used for single package with version (react-on-rails@VERSION) - Explained add_js_dependencies_batch is for batch operations - Both methods now have clear @param and @return documentation 4. Fixed TypeScript dependency installation inconsistency - Removed manual instance variable setting - Now uses consistent pattern with shared module approach - Raises specific error instead of just warning if installation fails - Matches the error handling pattern of other dependency methods 5. Fixed RuboCop violations - Removed redundant begin block in install_js_dependencies - All files now pass bundle exec rubocop with zero offenses All tests pass successfully (6 examples, 0 failures). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
This commit addresses critical issues identified in code review: ### 1. Added Missing Rspack Support - **Problem**: Rspack dependencies were lost during the refactor to shared module - **Solution**: Added RSPACK_DEPENDENCIES and RSPACK_DEV_DEPENDENCIES constants - **Implementation**: - Created `add_rspack_dependencies` method in shared module - Modified `add_dev_dependencies` to use Rspack-specific dev deps when `options.rspack?` is true - Conditional installation in `add_js_dependencies` checks for `options.rspack?` - **Impact**: Users with --rspack flag now get proper Rspack packages installed ### 2. Fixed TypeScript Dependency Consistency - **Problem**: TypeScript dependency handling was inconsistent with shared module pattern - **Solution**: Extracted TypeScript dependencies into shared module - **Implementation**: - Added TYPESCRIPT_DEPENDENCIES constant - Created `add_typescript_dependencies` method following same pattern as other deps - Updated `install_generator.rb` to delegate to shared method - Removed manual instance variable setting - **Impact**: Consistent error handling and pattern across all dependency types ### 3. Documented Unconditional Install Behavior - **Problem**: Unclear why `install_js_dependencies` is called unconditionally - **Solution**: Added comprehensive documentation explaining the design decision - **Rationale**: - package_json gem's install is idempotent - only installs what's needed - Prevents edge cases where package.json modified but dependencies not installed - Safer and simpler than conditional logic that could miss cases - **Impact**: Future maintainers understand why the unconditional call is correct ### 4. Enhanced Module Documentation - **Updated Instance Variables section**: Clarified module initializes variables - **Added Optional Methods section**: Documents options.rspack? and options.typescript? - **Added Installation Behavior section**: Explains idempotent install behavior - **Impact**: Clear contract for classes including the module ### 5. Constants Usage Verification - Verified all constants (REACT_DEPENDENCIES, CSS_DEPENDENCIES, DEV_DEPENDENCIES) are properly used - All dependency methods use their corresponding constants correctly ### Testing - ✅ All tests pass (6 examples, 0 failures) - ✅ bundle exec rubocop passes with zero offenses - ✅ Git hooks (pre-commit, pre-push) verified clean ### Files Changed - `lib/generators/react_on_rails/js_dependency_manager.rb`: Added Rspack, TypeScript support, enhanced docs - `lib/generators/react_on_rails/install_generator.rb`: Simplified TypeScript method to delegate to shared module 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…apacker This commit addresses the code review comment about separating SWC from Babel and leveraging Shakapacker for webpack/rspack configurations. ### Changes #### 1. Updated Default Webpack Loader to SWC **File**: `lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml` - Changed `webpack_loader` from 'babel' to 'swc' - Added comment recommending SWC for better performance - Shakapacker will now use swc-loader instead of babel-loader by default **Rationale**: SWC is significantly faster than Babel and is now mature enough to be the recommended default for new React on Rails projects. #### 2. Removed Babel-Specific Dependencies from REACT_DEPENDENCIES **Removed packages**: - `@babel/preset-react` - Shakapacker's babel-loader provides this when needed - `babel-plugin-transform-react-remove-prop-types` - Build tool specific - `babel-plugin-macros` - Build tool specific **Kept packages**: - `react` - Core React library - `react-dom` - Core React DOM library - `prop-types` - Runtime prop validation (React-specific, not build tool) **Rationale**: - Shakapacker manages webpack loader configuration (babel-loader vs swc-loader) - Users configure their preference via `shakapacker.yml` webpack_loader setting - SWC (now default) doesn't need Babel presets - Babel users get babel-loader and dependencies from Shakapacker - This avoids conflicts and duplicate installations #### 3. Removed @babel/preset-typescript from TYPESCRIPT_DEPENDENCIES **Removed**: `@babel/preset-typescript` **Kept**: - `typescript` - TypeScript compiler - `@types/react` - React type definitions - `@types/react-dom` - React DOM type definitions **Rationale**: - SWC has built-in TypeScript support (no preset needed) - Shakapacker handles the loader configuration - Babel users can add `@babel/preset-typescript` manually if they choose Babel #### 4. Added Comprehensive Documentation All dependency constants now include clear comments explaining: - Why certain packages are excluded - How Shakapacker manages build tool dependencies - The relationship between webpack_loader setting and dependencies ### Impact **For New Projects**: - Faster build times with SWC by default - Cleaner dependency tree (no Babel packages unless explicitly needed) - Easier to understand which dependencies are React-specific vs build tool specific **For Existing Projects**: - No breaking changes - existing projects keep their current webpack_loader setting - Projects can opt into SWC by changing `webpack_loader: 'swc'` in shakapacker.yml - Babel users continue to work as before (Shakapacker provides babel-loader) **For Rspack Users**: - No changes - Rspack has its own loader mechanism - Rspack-specific dependencies remain unchanged ### Testing - ✅ All tests pass (6 examples, 0 failures) - ✅ bundle exec rubocop passes with zero offenses - ✅ Git hooks verified clean ### Files Changed - `lib/generators/react_on_rails/js_dependency_manager.rb`: Removed Babel deps, added docs - `lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml`: Changed default to SWC 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
justin808
force-pushed
the
claude/pr-1788-rebased
branch
from
44cca73 to
62d06f0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
lib/generators/react_on_rails/base_generator.rb(2 hunks)lib/generators/react_on_rails/install_generator.rb(3 hunks)lib/generators/react_on_rails/js_dependency_manager.rb(1 hunks)lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml(1 hunks)spec/react_on_rails/generators/message_deduplication_spec.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 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:
lib/generators/react_on_rails/templates/base/base/config/shakapacker.ymllib/generators/react_on_rails/install_generator.rblib/generators/react_on_rails/js_dependency_manager.rblib/generators/react_on_rails/base_generator.rb
📚 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:
lib/generators/react_on_rails/templates/base/base/config/shakapacker.ymlspec/react_on_rails/generators/message_deduplication_spec.rblib/generators/react_on_rails/install_generator.rblib/generators/react_on_rails/js_dependency_manager.rblib/generators/react_on_rails/base_generator.rb
📚 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:
lib/generators/react_on_rails/templates/base/base/config/shakapacker.ymlspec/react_on_rails/generators/message_deduplication_spec.rblib/generators/react_on_rails/install_generator.rblib/generators/react_on_rails/js_dependency_manager.rblib/generators/react_on_rails/base_generator.rb
📚 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/react_on_rails/generators/message_deduplication_spec.rb
📚 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:
lib/generators/react_on_rails/install_generator.rblib/generators/react_on_rails/js_dependency_manager.rblib/generators/react_on_rails/base_generator.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
lib/generators/react_on_rails/install_generator.rblib/generators/react_on_rails/js_dependency_manager.rb
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
lib/generators/react_on_rails/base_generator.rb
🧬 Code graph analysis (4)
spec/react_on_rails/generators/message_deduplication_spec.rb (2)
spec/react_on_rails/support/generator_spec_helper.rb (1)
run_generator_test_with_args(62-72)lib/generators/react_on_rails/generator_messages.rb (1)
output(5-7)
lib/generators/react_on_rails/install_generator.rb (1)
lib/generators/react_on_rails/js_dependency_manager.rb (2)
setup_js_dependencies(92-104)add_typescript_dependencies(166-176)
lib/generators/react_on_rails/js_dependency_manager.rb (1)
lib/generators/react_on_rails/generator_helper.rb (2)
package_json(7-20)add_npm_dependencies(23-39)
lib/generators/react_on_rails/base_generator.rb (2)
lib/generators/react_on_rails/install_generator.rb (1)
include(12-435)lib/generators/react_on_rails/react_with_redux_generator.rb (1)
include(9-142)
🪛 ast-grep (0.39.9)
spec/react_on_rails/generators/message_deduplication_spec.rb
[warning] 33-36: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: expect(success_count).to(
eq(1),
"Expected success message to appear exactly once, but appeared #{success_count} times"
)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 51-54: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: expect(success_count).to(
eq(1),
"Expected success message to appear exactly once with Redux, but appeared #{success_count} times"
)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
⏰ 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). (11)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: examples (3.4, latest)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
| def add_js_dependency(package, dev: false) | ||
| pj = package_json | ||
| return false unless pj | ||
|
|
||
| begin | ||
| # Ensure package is in array format for package_json gem | ||
| packages_array = [package] | ||
| if dev | ||
| pj.manager.add(packages_array, type: :dev) | ||
| else | ||
| pj.manager.add(packages_array) | ||
| end | ||
| true |
There was a problem hiding this comment.
Pin the npm package to the gem’s exact version.
add_js_dependency adds react-on-rails@#{ReactOnRails::VERSION} without exact: true, so the package_json gem will record it with a caret range (e.g., ^12.3.0). That defeats the intent of keeping the npm package in sync with the Ruby gem and can pull in incompatible releases. GeneratorHelper’s batch path already sets exact: true; this single-package path needs the same treatment.
Apply this diff to keep the versions aligned:
- if dev
- pj.manager.add(packages_array, type: :dev)
- else
- pj.manager.add(packages_array)
+ if dev
+ pj.manager.add(packages_array, type: :dev, exact: true)
+ else
+ pj.manager.add(packages_array, exact: true)📝 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.
| def add_js_dependency(package, dev: false) | |
| pj = package_json | |
| return false unless pj | |
| begin | |
| # Ensure package is in array format for package_json gem | |
| packages_array = [package] | |
| if dev | |
| pj.manager.add(packages_array, type: :dev) | |
| else | |
| pj.manager.add(packages_array) | |
| end | |
| true | |
| def add_js_dependency(package, dev: false) | |
| pj = package_json | |
| return false unless pj | |
| begin | |
| # Ensure package is in array format for package_json gem | |
| packages_array = [package] | |
| if dev | |
| pj.manager.add(packages_array, type: :dev, exact: true) | |
| else | |
| pj.manager.add(packages_array, exact: true) | |
| end | |
| true |
🤖 Prompt for AI Agents
In lib/generators/react_on_rails/js_dependency_manager.rb around lines 206 to
218, the add_js_dependency method calls pj.manager.add without passing exact:
true, causing package_json to record caret ranges; update both pj.manager.add
calls (dev and non-dev branches) to include exact: true (e.g.,
pj.manager.add(packages_array, type: :dev, exact: true) and
pj.manager.add(packages_array, exact: true)) so the npm package is pinned to the
gem’s exact version.
lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml
Outdated
Show resolved
Hide resolved
- Restore version validation for pre-release versions in add_react_on_rails_package - Clarify TypeScript dependencies comment for SWC-first approach - Add explanatory comment about package_json availability - Fix test mocks to match actual implementation using add_npm_dependencies - Remove stale react-on-rails@^16.1.0 entry from yarn.lock 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
## Summary - Replace deprecated scss-lint with modern stylelint - Remove scss_lint gem dependency and configuration files - Add stylelint and stylelint-config-standard-scss npm packages - Create .stylelintrc.json configuration matching original scss-lint rules - Update rake tasks to use stylelint instead of scss-lint - Add stylelint to CI workflow (lint-js-and-ruby.yml) - Include SCSS linting in main rake lint task and autofix task - Add yarn script 'lint:scss' for running stylelint ## Configuration Philosophy Maintains the original minimal approach - only enables essential CSS validation rules while disabling formatting rules, matching the original scss-lint configuration philosophy. ## Test Plan - [x] Stylelint runs without errors on all SCSS files - [x] Configuration disables unwanted formatting rules - [x] Rake tasks work correctly with new linter - [x] CI integration functions properly Fixes #1688 🤖 Generated with [Claude Code](https://claude.ai/code) <!-- Reviewable:start --> - - - This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/shakacode/react_on_rails/1825) <!-- Reviewable:end --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - Chores - Migrated SCSS linting from the legacy linter to Stylelint with the standard SCSS config. - Added Stylelint dev dependencies, a lint:scss script and autofix support. - Updated lint tasks and Docker orchestration to run Stylelint alongside other linters. - Removed legacy SCSS linter configuration, development dependency and binstub. - Added ignore patterns to scope Stylelint to relevant files. - CI - Added a CI step to run SCSS linting with Stylelint. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Justin Gordon <[email protected]> Co-authored-by: Claude <[email protected]> Co-authored-by: Justin Gordon <[email protected]>
## Summary Replaces the `spec/dummy/bin/dev` symlink with a wrapper script that passes `--route=/` to the base dev script. ## Problem The `spec/dummy/bin/dev` was a symlink to the template script at `lib/generators/react_on_rails/templates/base/base/bin/dev`, which defaults to `DEFAULT_ROUTE = "hello_world"`. This is incorrect for the dummy app which needs to use the root route `/`. ## Solution Replace the symlink with a wrapper script that: - Calls the base dev script - Forces `--route=/` for the dummy app - Passes through all other command-line arguments ## Test Plan - Verified `bundle exec rubocop` passes with no offenses - Verified `bin/dev --help` works correctly in spec/dummy - Verified file is executable and has correct permissions Generated with Claude Code (https://claude.com/claude-code) <!-- Reviewable:start --> - - - This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/shakacode/react_on_rails/2029) <!-- Reviewable:end --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated internal development script configuration to improve development environment setup. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude <[email protected]>
…f webpack_loader - Rename webpack_loader to javascript_transpiler (Shakapacker 9.3+ standard) - Add assets_bundler configuration option - Update comments to reflect current Shakapacker API - Add note about automatic SWC usage with rspack 🤖 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
♻️ Duplicate comments (1)
lib/generators/react_on_rails/js_dependency_manager.rb (1)
216-227: Still need to pin the npm package version inadd_js_dependency(duplicate of prior feedback)
add_js_dependencystill callspj.manager.addwithoutexact: true, soreact-on-rails@#{ReactOnRails::VERSION}will be recorded as a caret range (e.g.^16.2.0), whileadd_npm_dependenciesalready pins versions exactly. That breaks the intent of keeping the npm package in lockstep with the gem version.Align this single-package path with the batch helper:
# Ensure package is in array format for package_json gem packages_array = [package] if dev - pj.manager.add(packages_array, type: :dev) + pj.manager.add(packages_array, type: :dev, exact: true) else - pj.manager.add(packages_array) + pj.manager.add(packages_array, exact: true) endYou can confirm how
package_jsonrecords the specifier by running the generator and inspectingpackage.jsonto ensurereact-on-railsis added with an exact version (no leading^).
🧹 Nitpick comments (1)
package.json (1)
3-3: Stylelint deps andlint:scssscript are correctly configured; refactoring to centralize glob patterns is optionalVerification confirms the added devDependencies and
lint:scssscript are set up correctly. The glob patterns match exactly between the package.json script andstylelint_commandinrakelib/lint.rake. To reduce duplication and keep glob patterns centralized, you could optionally refactor the Rake task to callyarn run lint:scssinstead of hardcoding the same patterns. This is a recommended improvement but not required for functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
Gemfile.lockis excluded by!**/*.lockreact_on_rails_pro/Gemfile.lockis excluded by!**/*.lockreact_on_rails_pro/spec/dummy/Gemfile.lockis excluded by!**/*.lockspec/dummy/Gemfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
.github/workflows/lint-js-and-ruby.yml(1 hunks).scss-lint.yml(0 hunks).stylelintrc.json(1 hunks)Gemfile.development_dependencies(0 hunks)knip.ts(2 hunks)lib/generators/react_on_rails/js_dependency_manager.rb(1 hunks)lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml(1 hunks)lib/react_on_rails/version.rb(1 hunks)package.json(3 hunks)packages/react-on-rails-pro/package.json(2 hunks)packages/react-on-rails/package.json(1 hunks)rakelib/docker.rake(1 hunks)rakelib/lint.rake(1 hunks)react_on_rails_pro/lib/react_on_rails_pro/version.rb(1 hunks)react_on_rails_pro/package.json(1 hunks)react_on_rails_pro/spec/dummy/bin/dev(1 hunks)spec/dummy/bin/dev(0 hunks)spec/dummy/bin/dev(1 hunks)spec/dummy/bin/scss-lint(0 hunks)
💤 Files with no reviewable changes (3)
- Gemfile.development_dependencies
- .scss-lint.yml
- spec/dummy/bin/scss-lint
✅ Files skipped from review due to trivial changes (2)
- rakelib/docker.rake
- spec/dummy/bin/dev
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml
🧰 Additional context used
🧠 Learnings (16)
📓 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-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/package.jsonreact_on_rails_pro/spec/dummy/bin/devpackage.jsonpackages/react-on-rails-pro/package.jsonreact_on_rails_pro/lib/react_on_rails_pro/version.rblib/react_on_rails/version.rblib/generators/react_on_rails/js_dependency_manager.rbpackages/react-on-rails/package.jsonknip.tsspec/dummy/bin/dev
📚 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/package.jsonreact_on_rails_pro/spec/dummy/bin/devpackage.jsonpackages/react-on-rails-pro/package.jsonreact_on_rails_pro/lib/react_on_rails_pro/version.rblib/react_on_rails/version.rblib/generators/react_on_rails/js_dependency_manager.rbpackages/react-on-rails/package.jsonknip.tsspec/dummy/bin/dev
📚 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:
react_on_rails_pro/package.jsonpackages/react-on-rails-pro/package.jsonreact_on_rails_pro/lib/react_on_rails_pro/version.rblib/react_on_rails/version.rblib/generators/react_on_rails/js_dependency_manager.rb
📚 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/package.jsonreact_on_rails_pro/spec/dummy/bin/devpackage.jsonpackages/react-on-rails-pro/package.jsonreact_on_rails_pro/lib/react_on_rails_pro/version.rb.stylelintrc.jsonlib/react_on_rails/version.rblib/generators/react_on_rails/js_dependency_manager.rbpackages/react-on-rails/package.jsonrakelib/lint.rakeknip.ts
📚 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/package.jsonpackage.jsonpackages/react-on-rails-pro/package.jsonlib/generators/react_on_rails/js_dependency_manager.rbpackages/react-on-rails/package.json
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
react_on_rails_pro/package.jsonpackages/react-on-rails-pro/package.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:
react_on_rails_pro/package.jsonpackages/react-on-rails-pro/package.jsonreact_on_rails_pro/lib/react_on_rails_pro/version.rblib/react_on_rails/version.rblib/generators/react_on_rails/js_dependency_manager.rb
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
react_on_rails_pro/spec/dummy/bin/dev
📚 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.stylelintrc.json.github/workflows/lint-js-and-ruby.ymlrakelib/lint.rake
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
package.jsonpackages/react-on-rails-pro/package.jsonreact_on_rails_pro/lib/react_on_rails_pro/version.rblib/react_on_rails/version.rbpackages/react-on-rails/package.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:
packages/react-on-rails-pro/package.jsonreact_on_rails_pro/lib/react_on_rails_pro/version.rblib/react_on_rails/version.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
packages/react-on-rails-pro/package.jsonreact_on_rails_pro/lib/react_on_rails_pro/version.rblib/react_on_rails/version.rblib/generators/react_on_rails/js_dependency_manager.rb
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.
Applied to files:
packages/react-on-rails-pro/package.jsonknip.ts
📚 Learning: 2025-02-13T16:50:26.861Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Applied to files:
react_on_rails_pro/lib/react_on_rails_pro/version.rblib/react_on_rails/version.rblib/generators/react_on_rails/js_dependency_manager.rbpackages/react-on-rails/package.json
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
react_on_rails_pro/lib/react_on_rails_pro/version.rblib/react_on_rails/version.rb
🧬 Code graph analysis (1)
lib/generators/react_on_rails/js_dependency_manager.rb (1)
lib/generators/react_on_rails/generator_helper.rb (2)
package_json(7-20)add_npm_dependencies(23-39)
🔇 Additional comments (12)
react_on_rails_pro/package.json (1)
3-3: LGTM! Version bump is consistent across the codebase.The version update from 16.2.0-beta.4 to 16.2.0-beta.5 aligns with the coordinated version bumps across all related packages (packages/react-on-rails/package.json, packages/react-on-rails-pro/package.json, lib/react_on_rails/version.rb, and react_on_rails_pro/lib/react_on_rails_pro/version.rb).
packages/react-on-rails/package.json (1)
3-3: LGTM! Version bump is consistent.The version update aligns with the coordinated beta release across the codebase.
react_on_rails_pro/spec/dummy/bin/dev (1)
1-5: LGTM! Clean delegation pattern.The script correctly delegates to the centralized base dev script using
execand forwards arguments appropriately. The relative path is correct for this file's location.packages/react-on-rails-pro/package.json (1)
3-3: LGTM! Version and dependency updates are synchronized.Both the package version and the react-on-rails dependency are correctly updated to 16.2.0-beta.5, maintaining consistency across the monorepo.
Also applies to: 59-59
spec/dummy/bin/dev (1)
1-5: LGTM! Consistent delegation pattern.The script correctly delegates to the base dev script with the appropriate relative path for this location and forwards arguments properly.
react_on_rails_pro/lib/react_on_rails_pro/version.rb (1)
4-4: LGTM! Ruby version constant updated consistently.The VERSION constant is correctly bumped to match the package.json version updates across the codebase.
lib/react_on_rails/version.rb (1)
4-4: LGTM! Ruby version constant updated consistently.The VERSION constant matches the coordinated version bump across all packages.
knip.ts (2)
129-134: LGTM! Appropriate ignore patterns for dynamically loaded code.These ignore patterns correctly exclude React on Rails components that are registered and loaded dynamically at runtime, which Knip cannot trace through static analysis.
165-176: LGTM! Valid dependencies for production builds.The added dependencies are all legitimate runtime or transitive dependencies that Knip's static analysis doesn't detect:
- Build tooling: @babel/runtime, mini-css-extract-plugin, css-loader, sass, sass-loader
- Runtime libraries: create-react-class, react-helmet, @types/react-helmet, react-redux, react-router-dom
These align with the PR's centralized dependency management and the dynamic nature of React on Rails component registration.
.stylelintrc.json (1)
1-45: SCSS lint configuration looks reasonable and aligned with the new toolingExtending the SCSS preset and relaxing the pattern rules/ignoreFiles to match existing code is consistent and should keep noise low while still enforcing basics; no changes needed from my side.
lib/generators/react_on_rails/js_dependency_manager.rb (1)
40-89: Dependency sets and SWC/TypeScript separation look consistent with Shakapacker’s roleThe React/CSS/dev/Rspack/TypeScript dependency groups, plus the comments about SWC as the default transpiler and omitting Babel presets/@babel/preset-typescript, line up well with shakapacker.yml being the source of truth for loader/transpiler choice. The conditional use of RSPACK_DEV_DEPENDENCIES when
options.rspack?is set and dev-only TypeScript deps viaadd_typescript_dependenciesalso look correct.If you’d like, verify the generated app’s
package.jsonfor both webpack and rspack + TypeScript combinations to ensure the expected packages appear in the right (dev vs prod) sections.Also applies to: 140-205
.github/workflows/lint-js-and-ruby.yml (1)
137-138: CI SCSS lint step is correctly wired and placedThe
yarn run lint:scsscommand is properly defined in package.json, uses the correct stylelint and stylelint-config-standard-scss dependencies, and the file glob patterns reference existing directories. No configuration issues found.
| require_relative "task_helpers" | ||
|
|
||
| namespace :lint do | ||
| namespace :lint do # rubocop:disable Metrics/BlockLength |
There was a problem hiding this comment.
🧩 Analysis chain
Rake stylelint integration works; consider reusing the Yarn script and updating descriptions
The new scss task and autofix hooks are functionally solid. Two small maintainability ideas:
- To avoid duplicated glob patterns between Rake and
package.json, you could makestylelint_commandsimply runyarn run lint:scssandstylelint_fix_commandrunyarn run lint:scss --fix, letting the script own the file list. - The top-level
autofixtask description still only mentions eslint/prettier/rubocop; consider updating it to mention SCSS/stylelint now thatstylelint_fix_commandruns as well.
You can verify behavior after such a change with:
Also applies to: 13-16, 23-25, 32-45
🏁 Script executed:
#!/bin/bash
bundle exec rake lint:scss
bundle exec rake autofixLength of output: 89
🏁 Script executed:
#!/bin/bash
# Examine the lint.rake file to see the current implementation
cat rakelib/lint.rakeLength of output: 1418
🏁 Script executed:
#!/bin/bash
# Check if package.json exists and look for lint:scss script
cat package.json | grep -A 20 '"scripts"'Length of output: 939
🏁 Script executed:
#!/bin/bash
# Search for any yarn/npm scripts related to stylelint
rg -i 'stylelint|lint:scss' package.jsonLength of output: 187
🏁 Script executed:
#!/bin/bash
# Look for the glob patterns used in rake file
rg -n 'glob|@' rakelib/lint.rakeLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check the exact content around lines 5, 13-16, 23-25, 32-45
sed -n '1,50p' rakelib/lint.rakeLength of output: 1303
🏁 Script executed:
#!/bin/bash
# Look for any documentation or comments about autofix
rg -n 'autofix|description' rakelib/lint.rake -C2Length of output: 309
🏁 Script executed:
#!/bin/bash
# Check if there's a Gemfile to understand rake setup
cat Gemfile | grep -i rakeLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for all task descriptions to see if autofix is mentioned
rg -n 'task|desc' rakelib/lint.rakeLength of output: 588
🏁 Script executed:
#!/bin/bash
# Find the complete autofix implementation
ast-grep --pattern 'task :autofix'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for stylelint_command and stylelint_fix_command definitions
rg -n 'stylelint_command|stylelint_fix_command' rakelib/lint.rakeLength of output: 189
Update autofix description to include stylelint, and optionally refactor to reuse the Yarn script
The review comment is valid. Two concrete issues found:
-
Line 51 — Incomplete description: The global
autofixtask description omits stylelint/scss, though the task itself callsstylelint_fix_commandon line 32. Update to:"Auto-fix all linting violations (eslint --fix, prettier --write, rubocop -A, stylelint --fix)". -
Lines 39–40 — Glob pattern duplication: The glob patterns in
stylelint_commandare identical to thelint:scssscript in package.json. Consider refactoringstylelint_commandto callyarn run lint:scssandstylelint_fix_commandto callyarn run lint:scss --fix, letting package.json own the file list.
🤖 Prompt for AI Agents
In rakelib/lint.rake around lines 5 and 39–51: update the global autofix task
description on line 51 to read "Auto-fix all linting violations (eslint --fix,
prettier --write, rubocop -A, stylelint --fix)" to accurately include stylelint,
and refactor the stylelint_command and stylelint_fix_command (lines ~39–40 and
~32) to invoke the package.json script instead of duplicating globs—replace the
direct stylelint invocations with calls to "yarn run lint:scss" and "yarn run
lint:scss --fix" respectively so the file list is maintained in package.json.
|
Closing this PR in favor of #2051 which has a clean branch without merge conflicts. The new PR contains the same JS dependency manager refactoring changes, just on a fresh branch created from the latest master. |
## Summary This PR refactors JavaScript dependency management in React on Rails generators by extracting duplicated code into a shared `JsDependencyManager` module. ## Changes - **Eliminates ~270 lines of duplicated code** between `BaseGenerator` and `InstallGenerator` - **Centralizes dependency definitions** in frozen constants with clear documentation - **Removes Babel dependencies** that are now managed by Shakapacker - **Changes default transpiler to SWC** in generated shakapacker.yml for better performance - **Adds comprehensive test coverage** for the new module - **Simplifies dependency installation flow** using package_json gem ## Key Improvements ### Code Organization - All JS dependencies are now defined as frozen constants (REACT_DEPENDENCIES, CSS_DEPENDENCIES, etc.) - Clear separation of concerns with well-documented module - Consistent error handling across all generators ### Dependency Management - Leverages package_json gem (always available via Shakapacker) for reliable cross-platform package management - Removes Babel-related packages that Shakapacker now handles automatically - Adds support for Rspack and TypeScript dependencies ### Default Configuration -⚠️ **Breaking Change**: Default `webpack_loader` changed from `'babel'` to `'swc'` in generated shakapacker.yml - SWC provides better performance and has built-in TypeScript support - Users who need Babel can still configure it manually ## Testing New test file `spec/react_on_rails/generators/message_deduplication_spec.rb` verifies: - No duplicate post-install messages - Proper module inclusion in both generators - Correct dependency installation flow ## Migration Notes For new installations, this change is transparent. Existing projects are not affected as this only impacts the generator output for new setups. --- Supersedes and closes #2030 (that PR had merge conflict issues, so starting fresh with a clean branch) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Centralized JavaScript dependency management across generators for more consistent installation and simplified generator behavior. * **Configuration** * Installer config adds javascript_transpiler and assets_bundler options; previous webpack-specific key removed. * **Tests** * New specs for post-install messages, message deduplication, and JS dependency flows to validate installation messaging and dependency handling. * **Changelog** * Generator no longer auto-installs Babel presets; SWC is the default transpiler and Babel must be added manually. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude <[email protected]>
Summary
This PR addresses code review feedback from the initial PR #1788 and subsequent detailed analysis.
Critical Issues Fixed
1. ✅ Added Missing Rspack Support
Problem: Rspack dependencies were accidentally lost during the refactor to shared module
Solution:
add_rspack_dependenciesmethod in shared moduleadd_dev_dependenciesto use Rspack-specific dev deps when--rspackflag is setadd_js_dependencieschecks foroptions.rspack?Impact: Users installing with
--rspackflag now get proper Rspack packages (@rspack/core, rspack-manifest-plugin, @rspack/cli, etc.)2. ✅ Fixed TypeScript Dependency Consistency
Problem: TypeScript dependency handling was inconsistent with shared module pattern
Solution:
add_typescript_dependenciesmethod following same pattern as other dependenciesinstall_generator.rbto delegate to shared methodImpact: Consistent error handling and pattern across all dependency types (React, CSS, Dev, Rspack, TypeScript)
3. ✅ Documented Unconditional Install Behavior
Problem: Unclear why
install_js_dependenciesis called unconditionallySolution: Added comprehensive documentation explaining the design decision
Rationale:
4. ✅ Enhanced Module Documentation
Updates:
unless defined?patternoptions.rspack?andoptions.typescript?supportImpact: Future maintainers understand the module's behavior and requirements
Initial Code Review Issues Addressed
✅ Instance Variable Initialization Pattern
||=tounless defined?pattern for clarity✅ Error Messages Made More Specific
"Failed to add React dependencies (react, react-dom, ...) via package_json gem. This indicates shakapacker dependency may not be properly installed."✅ Documented add_js_dependency Method
✅ Fixed RuboCop Violations
Testing
bundle exec rubocoppasses with zero offensesFiles Changed
lib/generators/react_on_rails/js_dependency_manager.rb: Added Rspack & TypeScript support, enhanced documentationlib/generators/react_on_rails/install_generator.rb: Simplified TypeScript method to delegate to shared modulelib/generators/react_on_rails/base_generator.rb: Method rename for clarityspec/react_on_rails/generators/message_deduplication_spec.rb: Updated testsTest plan
bundle exec rspec spec/react_on_rails/generators/message_deduplication_spec.rbbundle exec rubocop🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
Configuration
Improvements
Styling / Lint
Tests
Version