fix: Address code review issues and optimize package_json gem usage

- 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]>

Author: github-actions[bot]

lib/generators/react_on_rails/base_generator.rb

@@ -109,10 +109,6 @@ def add_base_gems_to_gemfile
         run "bundle"
       end
 
-      def add_js_dependencies
-        setup_js_dependencies
-      end
-
       def update_gitignore_for_auto_registration
         gitignore_path = File.join(destination_root, ".gitignore")
         return unless File.exist?(gitignore_path)

lib/generators/react_on_rails/js_dependency_manager.rb

@@ -14,6 +14,7 @@ def setup_js_dependencies
         @added_dependencies_to_package_json ||= false
         @ran_direct_installs ||= false
         add_js_dependencies
+        # Only run final install if package_json gem was used and no direct installs ran
         install_js_dependencies if @added_dependencies_to_package_json && !@ran_direct_installs
       end
 
@@ -27,25 +28,24 @@ def add_js_dependencies
       def add_react_on_rails_package
         major_minor_patch_only = /\A\d+\.\d+\.\d+\z/
 
-        # Try to use package_json gem first, fall back to direct npm commands
         react_on_rails_pkg = if ReactOnRails::VERSION.match?(major_minor_patch_only)
-                               ["react-on-rails@#{ReactOnRails::VERSION}"]
+                               "react-on-rails@#{ReactOnRails::VERSION}"
                              else
                                puts "Adding the latest react-on-rails NPM module. " \
                                     "Double check this is correct in package.json"
-                               ["react-on-rails"]
+                               "react-on-rails"
                              end
 
         puts "Installing React on Rails package..."
-        if add_npm_dependencies(react_on_rails_pkg)
+        if add_js_dependency(react_on_rails_pkg)
           @added_dependencies_to_package_json = true
-          return
+        else
+          # Fallback to direct npm install
+          puts "Using direct npm commands as fallback"
+          success = system("npm", "install", react_on_rails_pkg)
+          @ran_direct_installs = true if success
+          handle_npm_failure("react-on-rails package", [react_on_rails_pkg]) unless success
         end
-
-        puts "Using direct npm commands as fallback"
-        success = system("npm", "install", *react_on_rails_pkg)
-        @ran_direct_installs = true if success
-        handle_npm_failure("react-on-rails package", react_on_rails_pkg) unless success
       end
 
       def add_react_dependencies
@@ -58,14 +58,15 @@ def add_react_dependencies
           babel-plugin-transform-react-remove-prop-types
           babel-plugin-macros
         ]
-        if add_npm_dependencies(react_deps)
+
+        if add_js_dependencies_batch(react_deps)
           @added_dependencies_to_package_json = true
-          return
+        else
+          # Fallback to direct npm install
+          success = system("npm", "install", *react_deps)
+          @ran_direct_installs = true if success
+          handle_npm_failure("React dependencies", react_deps) unless success
         end
-
-        success = system("npm", "install", *react_deps)
-        @ran_direct_installs = true if success
-        handle_npm_failure("React dependencies", react_deps) unless success
       end
 
       def add_css_dependencies
@@ -76,14 +77,15 @@ def add_css_dependencies
           mini-css-extract-plugin
           style-loader
         ]
-        if add_npm_dependencies(css_deps)
+
+        if add_js_dependencies_batch(css_deps)
           @added_dependencies_to_package_json = true
-          return
+        else
+          # Fallback to direct npm install
+          success = system("npm", "install", *css_deps)
+          @ran_direct_installs = true if success
+          handle_npm_failure("CSS dependencies", css_deps) unless success
         end
-
-        success = system("npm", "install", *css_deps)
-        @ran_direct_installs = true if success
-        handle_npm_failure("CSS dependencies", css_deps) unless success
       end
 
       def add_dev_dependencies
@@ -92,29 +94,72 @@ def add_dev_dependencies
           @pmmmwh/react-refresh-webpack-plugin
           react-refresh
         ]
-        if add_npm_dependencies(dev_deps, dev: true)
+
+        if add_js_dependencies_batch(dev_deps, dev: true)
           @added_dependencies_to_package_json = true
-          return
+        else
+          # Fallback to direct npm install
+          success = system("npm", "install", "--save-dev", *dev_deps)
+          @ran_direct_installs = true if success
+          handle_npm_failure("development dependencies", dev_deps, dev: true) unless success
         end
+      end
+
+      # Add a single dependency using package_json gem
+      def add_js_dependency(package, dev: false)
+        return false unless package_json_available?
+
+        pj = package_json
+        return false unless pj
+
+        begin
+          if dev
+            pj.manager.add(package, type: :dev)
+          else
+            pj.manager.add(package)
+          end
+          true
+        rescue StandardError => e
+          puts "Warning: Could not add #{package} via package_json gem: #{e.message}"
+          false
+        end
+      end
 
-        success = system("npm", "install", "--save-dev", *dev_deps)
-        @ran_direct_installs = true if success
-        handle_npm_failure("development dependencies", dev_deps, dev: true) unless success
+      # Add multiple dependencies at once using package_json gem
+      def add_js_dependencies_batch(packages, dev: false)
+        return false unless package_json_available?
+
+        # Use the add_npm_dependencies helper from GeneratorHelper
+        add_npm_dependencies(packages, dev: dev)
+      end
+
+      # Check if package_json gem is available and loaded
+      def package_json_available?
+        # Check if Shakapacker or package_json gem is available
+        return true if defined?(PackageJson)
+
+        begin
+          require "package_json"
+          true
+        rescue LoadError
+          false
+        end
       end
 
       def install_js_dependencies
-        # Detect which package manager to use
-        success = if File.exist?(File.join(destination_root, "yarn.lock"))
-                    system("yarn", "install")
-                  elsif File.exist?(File.join(destination_root, "pnpm-lock.yaml"))
-                    system("pnpm", "install")
-                  elsif File.exist?(File.join(destination_root, "package-lock.json")) ||
-                        File.exist?(File.join(destination_root, "package.json"))
-                    # Use npm for package-lock.json or as default fallback
-                    system("npm", "install")
-                  else
-                    true # No package manager detected, skip
-                  end
+        # First try to use package_json gem's install method if available
+        if package_json_available? && package_json
+          begin
+            package_json.manager.install
+            return true
+          rescue StandardError => e
+            puts "Warning: package_json gem install failed: #{e.message}"
+            # Fall through to manual detection
+          end
+        end
+
+        # Fallback to detecting package manager and running install
+        success = detect_and_run_package_manager_install
 
         unless success
           GeneratorMessages.add_warning(<<~MSG.strip)
@@ -131,6 +176,21 @@ def install_js_dependencies
         success
       end
 
+      def detect_and_run_package_manager_install
+        # Detect which package manager to use based on lock files
+        if File.exist?(File.join(destination_root, "yarn.lock"))
+          system("yarn", "install")
+        elsif File.exist?(File.join(destination_root, "pnpm-lock.yaml"))
+          system("pnpm", "install")
+        elsif File.exist?(File.join(destination_root, "package-lock.json")) ||
+              File.exist?(File.join(destination_root, "package.json"))
+          # Use npm for package-lock.json or as default fallback
+          system("npm", "install")
+        else
+          true # No package manager detected, skip
+        end
+      end
+
       def handle_npm_failure(dependency_type, packages, dev: false)
         install_command = dev ? "npm install --save-dev" : "npm install"
         GeneratorMessages.add_warning(<<~MSG.strip)

spec/react_on_rails/generators/message_deduplication_spec.rb

@@ -27,8 +27,10 @@
 
         # Count occurrences of the success message
         success_count = output_text.scan("🎉 React on Rails Successfully Installed!").count
-        expect(success_count).to eq(1),
-                                 "Expected success message to appear exactly once, but appeared #{success_count} times"
+        expect(success_count).to(
+          eq(1),
+          "Expected success message to appear exactly once, but appeared #{success_count} times"
+        )
 
         # Ensure post-install message components are present
         expect(output_text).to include("📋 QUICK START:")
@@ -59,14 +61,17 @@
   end
 
   describe "NPM install execution" do
-    let(:install_generator) { InstallGenerator.new }
+    let(:install_generator) { ReactOnRails::Generators::InstallGenerator.new }
 
     before do
       # Mock the system to track NPM install calls
+      allow(install_generator).to receive_messages(
+        system: true,
+        add_npm_dependencies: false,
+        destination_root: "/test/path"
+      )
       allow(File).to receive(:exist?).and_return(false)
-      allow(File).to receive(:exist?).with(File.join(anything, "package.json")).and_return(true)
-      allow(install_generator).to receive_messages(system: true, add_npm_dependencies: false,
-                                                   destination_root: "/test/path")
+      allow(File).to receive(:exist?).with(a_string_matching(/package\.json$/)).and_return(true)
 
       # Initialize instance variables
       install_generator.instance_variable_set(:@added_dependencies_to_package_json, false)
@@ -105,15 +110,13 @@
 
   describe "JS dependency method organization" do
     it "uses the shared JsDependencyManager module in base_generator" do
-      expect(ReactOnRails::Generators::BaseGenerator.ancestors).to(
-        include(ReactOnRails::Generators::JsDependencyManager)
-      )
+      expect(ReactOnRails::Generators::BaseGenerator.ancestors)
+        .to include(ReactOnRails::Generators::JsDependencyManager)
     end
 
     it "uses the shared JsDependencyManager module in install_generator" do
-      expect(ReactOnRails::Generators::InstallGenerator.ancestors).to(
-        include(ReactOnRails::Generators::JsDependencyManager)
-      )
+      expect(ReactOnRails::Generators::InstallGenerator.ancestors)
+        .to include(ReactOnRails::Generators::JsDependencyManager)
     end
 
     it "does not duplicate JS dependency methods between generators" do
@@ -124,15 +127,13 @@
       shared_methods = %i[setup_js_dependencies add_js_dependencies install_js_dependencies]
 
       shared_methods.each do |method|
-        expect(base_generator).to respond_to(method, true)
-        expect(install_generator).to respond_to(method, true)
-
-        # The methods should come from the same module
-        base_method = base_generator.method(method)
-        install_method = install_generator.method(method)
-
-        expect(base_method.owner).to eq(ReactOnRails::Generators::JsDependencyManager)
-        expect(install_method.owner).to eq(ReactOnRails::Generators::JsDependencyManager)
+        expect(base_generator.respond_to?(method, true)).to be(true)
+        expect(install_generator.respond_to?(method, true)).to be(true)
+        # Verify the methods are defined by the shared module
+        expect(ReactOnRails::Generators::BaseGenerator.instance_method(method).owner)
+          .to eq(ReactOnRails::Generators::JsDependencyManager)
+        expect(ReactOnRails::Generators::InstallGenerator.instance_method(method).owner)
+          .to eq(ReactOnRails::Generators::JsDependencyManager)
       end
     end
   end