Rename webpackConfig to serverClientOrBoth

[Judahmeek]

I think this file name might be more useful than having webpackConfig.js & webpack.config.js right next to each other, which I have heard is confusing directly from clients.

---

This change is 

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing Touches

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch judahmeek/serverClientOrBoth

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[justin808]

@Judahmeek use shakacode/react_on_rails-demo-v15-ssr-auto-registration-bundle-splitting@db72f30

[claude]

Code Review Feedback for PR #1772

🎯 Overall Assessment

This PR makes significant improvements to developer experience with multiple enhancements. The file rename from webpackConfig.js to serverClientOrBoth.js addresses the confusion mentioned in the description. However, there are several areas that need attention.

✅ Strengths

1. Clear Intent: The file rename from webpackConfig.js to serverClientOrBoth.js provides better clarity about the file's purpose
2. Enhanced Developer Experience: The enhanced bin/dev script with multiple modes (HMR, static, production-like) is excellent
3. Good Test Coverage: Comprehensive test coverage for the new functionality
4. Automatic Shakapacker Installation: Smart detection and auto-installation of Shakapacker dependencies

⚠️ Areas for Improvement

Code Quality Issues

1. Ruby Style Inconsistency (spec/dummy/bin/dev:16)

   # Current
   unless $?.success?
   
   # Should be
   unless $CHILD_STATUS.success?

   Use $CHILD_STATUS instead of $? for better readability (RuboCop will flag this)

2. Error Handling in bin/dev Scripts

   • Both scripts have inconsistent error handling patterns
   • Some use exit! (immediate), others use exit 1 (allows cleanup)
   • Consider standardizing on exit 1 unless immediate termination is specifically needed

Potential Bugs

1. Race Condition Risk (lib/generators/react_on_rails/install_generator.rb:106-120)

   def ensure_shakapacker_installed
     return if shakapacker_installed?
     # ... installation logic
   end

   The check and installation aren't atomic. Consider adding a lock mechanism for concurrent generator runs.

2. Missing Error Propagation (spec/dummy/bin/dev:16)

   unless $?.success?  # Should check $CHILD_STATUS instead
     puts "Pack generation failed"
     exit 1
   end

Security Considerations

1. Command Injection Prevention: The scripts use system() calls with string interpolation, but the current usage appears safe as it doesn't interpolate user input
2. File Path Validation: The Procfile references are hardcoded, which is secure

Performance Considerations

1. Redundant Gem Lookups: shakapacker_installed? method could cache results to avoid repeated gem specification lookups
2. Blocking System Calls: The bin/dev scripts make several blocking system calls that could be optimized with background processes

Configuration Changes Impact

1. Default Configuration Changes:

   # config/initializers/react_on_rails.rb.tt:52
   config.components_subdirectory = "ror_components"  # Now enabled by default
   
   # config/shakapacker.yml:10
   nested_entries: true  # Now enabled by default

   These defaults change existing behavior. Consider documenting migration path for users.

2. Package Manager Switch: The switch from Yarn to npm as default might break existing workflows. Consider:

   • Adding a deprecation notice for Yarn users
   • Documenting the change in release notes

🔧 Recommendations

1. Fix RuboCop Issues: Run rake lint:rubocop and address the $? vs $CHILD_STATUS issues
2. Add Integration Tests: Consider adding integration tests for the new bin/dev modes
3. Documentation: Add migration notes for the default configuration changes
4. Error Messages: Improve error messages to be more actionable (include specific next steps)
5. Backward Compatibility: Consider feature flags for the new defaults to ease migration

📊 Test Coverage Assessment

The test coverage is comprehensive:

• ✅ New bin/dev functionality is well tested
• ✅ Shakapacker installation logic has good coverage
• ✅ Generator changes have corresponding spec updates
• ✅ Multiple scenarios (success/failure) are covered

🚀 Suggestions for Follow-up

1. Consider adding a configuration option to choose between npm/yarn
2. Add logging for debugging the pack generation process
3. Consider adding a --verbose flag for troubleshooting

Overall: This is a solid improvement that addresses real developer pain points. With the minor fixes suggested above, this will be a great addition to React on Rails v15. 👍

[justin808]

@Judahmeek please update spec/dummy