Address code review feedback: improve robustness and code quality

Code Review Improvements:
- Enhanced auto-detection to warn when user explicitly configures
  server_bundle_output_path differently from Shakapacker's private_output_path
- Improved webpack config template readability by extracting path logic
  to named constant instead of inline ternary
- Added warning for edge case of absolute paths outside Rails.root
- Enhanced documentation for shakapacker_version_9_or_higher? method
  explaining optimistic default behavior and fallback logic
- Fixed long line in configuration.md documentation (187 chars -> multi-line)

Test Coverage:
- Added 5 tests for auto-detection warning functionality
- Added 2 tests for absolute path warning in normalize_to_relative_path
- All tests pass, zero RuboCop offenses

Changes maintain backward compatibility while providing better user guidance
through actionable warning messages.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>

Author: justin808

docs/api-reference/configuration.md

@@ -127,8 +127,14 @@ ReactOnRails.configure do |config|
   # `enforce_private_server_bundles` to ensure server bundles are only loaded from private directories
   # config.server_bundle_output_path = "ssr-generated"
 
-  # When set to true, React on Rails will only load server bundles from private, explicitly configured directories (such as `ssr-generated`), and will raise an error if a server bundle is found in a public or untrusted location. This helps prevent accidental or malicious execution of untrusted JavaScript on the server, and is strongly recommended for production environments. And prevent leakage of server-side code to the client (Especially in the case of RSC).
-  # Default is false for backward compatibility, but enabling this option is a best practice for security.
+  # When set to true, React on Rails will only load server bundles from private, explicitly
+  # configured directories (such as `ssr-generated`), and will raise an error if a server
+  # bundle is found in a public or untrusted location. This helps prevent accidental or
+  # malicious execution of untrusted JavaScript on the server, and is strongly recommended
+  # for production environments. Also prevents leakage of server-side code to the client
+  # (especially important for React Server Components).
+  # Default is false for backward compatibility, but enabling this option is a best practice
+  # for security.
   config.enforce_private_server_bundles = false
 
   ################################################################################

lib/generators/react_on_rails/generator_helper.rb

@@ -98,6 +98,18 @@ def component_extension(options)
 
   # Check if Shakapacker 9.0 or higher is available
   # Returns true if Shakapacker >= 9.0, false otherwise
+  #
+  # This method is used during code generation to determine which configuration
+  # patterns to use in generated files (e.g., config.privateOutputPath vs hardcoded paths).
+  #
+  # @return [Boolean] true if Shakapacker 9.0+ is available or likely to be installed
+  #
+  # @note Default behavior: Returns true when Shakapacker is not yet installed
+  #   Rationale: During fresh installations, we optimistically assume users will install
+  #   the latest Shakapacker version. This ensures new projects get best-practice configs.
+  #   If users later install an older version, the generated webpack config includes
+  #   fallback logic (e.g., `config.privateOutputPath || hardcodedPath`) that prevents
+  #   breakage, and validation warnings guide them to fix any misconfigurations.
   def shakapacker_version_9_or_higher?
     return @shakapacker_version_9_or_higher if defined?(@shakapacker_version_9_or_higher)
 

lib/generators/react_on_rails/templates/base/base/config/webpack/serverWebpackConfig.js.tt

@@ -48,17 +48,22 @@ const configureServer = () => {
 <% if shakapacker_version_9_or_higher? -%>
   // Using Shakapacker 9.0+ privateOutputPath for automatic sync with shakapacker.yml
   // This eliminates manual path configuration and keeps configs in sync.
+  // Falls back to hardcoded path if private_output_path is not configured.
+  const serverBundleOutputPath = config.privateOutputPath ||
+    require('path').resolve(__dirname, '../../ssr-generated');
 <% else -%>
   // Using hardcoded path (Shakapacker < 9.0)
   // For Shakapacker 9.0+, consider using config.privateOutputPath instead
   // to automatically sync with shakapacker.yml private_output_path.
+  const serverBundleOutputPath = require('path').resolve(__dirname, '../../ssr-generated');
 <% end -%>
+
   serverWebpackConfig.output = {
     filename: 'server-bundle.js',
     globalObject: 'this',
     // If using the React on Rails Pro node server renderer, uncomment the next line
     // libraryTarget: 'commonjs2',
-    path: <%= shakapacker_version_9_or_higher? ? "(config.privateOutputPath != null ? config.privateOutputPath : require('path').resolve(__dirname, '../../ssr-generated'))" : "require('path').resolve(__dirname, '../../ssr-generated')" %>,
+    path: serverBundleOutputPath,
     // No publicPath needed since server bundles are not served via web
     // https://webpack.js.org/configuration/output/#outputglobalobject
   };
@@ -77,9 +82,8 @@ const configureServer = () => {
   // 1. Ensure config/initializers/react_on_rails.rb has: config.server_bundle_output_path = "ssr-generated"
   // 2. Run: rails react_on_rails:doctor to verify configuration
   const fs = require('fs');
-  const serverBundlePath = serverWebpackConfig.output.path;
-  if (!fs.existsSync(serverBundlePath)) {
-    console.warn(`⚠️  Server bundle output directory does not exist: ${serverBundlePath}`);
+  if (!fs.existsSync(serverBundleOutputPath)) {
+    console.warn(`⚠️  Server bundle output directory does not exist: ${serverBundleOutputPath}`);
     console.warn('   It will be created during build, but ensure React on Rails is configured:');
     console.warn('   config.server_bundle_output_path = "ssr-generated" in config/initializers/react_on_rails.rb');
     console.warn('   Run: rails react_on_rails:doctor to validate your configuration');

lib/react_on_rails/configuration.rb

@@ -260,36 +260,56 @@ def validate_enforce_private_server_bundles
     end
 
     # Auto-detect server_bundle_output_path from Shakapacker 9.0+ private_output_path
-    # Only sets if user hasn't explicitly configured server_bundle_output_path
+    # Checks if user explicitly set a value and warns them to use auto-detection instead
     def auto_detect_server_bundle_path_from_shakapacker
-      # Skip if user explicitly set server_bundle_output_path to something other than default
-      return if server_bundle_output_path != ReactOnRails::DEFAULT_SERVER_BUNDLE_OUTPUT_PATH
-
       # Skip if Shakapacker is not available
       return unless defined?(::Shakapacker)
 
       # Check if Shakapacker config has private_output_path method (9.0+)
       return unless ::Shakapacker.config.respond_to?(:private_output_path)
 
-      apply_shakapacker_private_output_path
-    end
-
-    def apply_shakapacker_private_output_path
+      # Get the private_output_path from Shakapacker
       private_path = ::Shakapacker.config.private_output_path
       return unless private_path
 
-      # Convert from Pathname to relative string path
       relative_path = ReactOnRails::Utils.normalize_to_relative_path(private_path)
-      self.server_bundle_output_path = relative_path
 
-      Rails.logger&.info("ReactOnRails: Auto-detected server_bundle_output_path from " \
-                         "shakapacker.yml private_output_path: '#{relative_path}'")
+      # Check if user explicitly configured server_bundle_output_path
+      if server_bundle_output_path != ReactOnRails::DEFAULT_SERVER_BUNDLE_OUTPUT_PATH
+        warn_about_explicit_configuration(relative_path)
+        return
+      end
+
+      apply_shakapacker_private_output_path(relative_path)
     rescue StandardError => e
       # Fail gracefully - if auto-detection fails, keep the default
       Rails.logger&.debug("ReactOnRails: Could not auto-detect server bundle path from " \
                           "Shakapacker: #{e.message}")
     end
 
+    def warn_about_explicit_configuration(shakapacker_path)
+      # Normalize both paths for comparison
+      normalized_config = server_bundle_output_path.to_s.chomp("/")
+      normalized_shakapacker = shakapacker_path.to_s.chomp("/")
+
+      # Only warn if there's a mismatch
+      return if normalized_config == normalized_shakapacker
+
+      Rails.logger&.warn(
+        "ReactOnRails: server_bundle_output_path is explicitly set to '#{server_bundle_output_path}' " \
+        "but shakapacker.yml private_output_path is '#{shakapacker_path}'. " \
+        "Consider removing server_bundle_output_path from your React on Rails initializer " \
+        "to use the auto-detected value from shakapacker.yml."
+      )
+    end
+
+    def apply_shakapacker_private_output_path(relative_path)
+      self.server_bundle_output_path = relative_path
+
+      Rails.logger&.debug("ReactOnRails: Auto-detected server_bundle_output_path from " \
+                          "shakapacker.yml private_output_path: '#{relative_path}'")
+    end
+
     def check_minimum_shakapacker_version
       ReactOnRails::PackerUtils.raise_shakapacker_version_incompatible_for_basic_pack_generation unless
         ReactOnRails::PackerUtils.supports_basic_pack_generation?

lib/react_on_rails/utils.rb

@@ -479,6 +479,14 @@ def self.normalize_to_relative_path(path)
         path_str.sub(%r{^#{Regexp.escape(rails_root_str)}/?}, "")
       else
         # Path is already relative or doesn't contain Rails.root
+        # Warn if it's an absolute path outside Rails.root (edge case)
+        if path_str.start_with?("/") && !path_str.start_with?(rails_root_str)
+          Rails.logger&.warn(
+            "ReactOnRails: Detected absolute path outside Rails.root: '#{path_str}'. " \
+            "Server bundles are typically stored within Rails.root. " \
+            "Verify this is intentional."
+          )
+        end
         path_str
       end
     end

spec/react_on_rails/configuration_spec.rb

@@ -550,6 +550,82 @@ module ReactOnRails
         end
       end
     end
+
+    describe "auto_detect_server_bundle_path_from_shakapacker" do
+      let(:shakapacker_module) { Module.new }
+      let(:shakapacker_config) { double("ShakapackerConfig") } # rubocop:disable RSpec/VerifiedDoubles
+
+      before do
+        config = shakapacker_config
+        stub_const("::Shakapacker", shakapacker_module)
+        shakapacker_module.define_singleton_method(:config) { config }
+        allow(shakapacker_config).to receive(:respond_to?).with(:private_output_path).and_return(true)
+      end
+
+      context "when user explicitly set server_bundle_output_path to different value" do
+        it "warns about configuration mismatch" do
+          allow(shakapacker_config).to receive(:private_output_path)
+            .and_return(Pathname.new("/fake/rails/root/shakapacker-bundles"))
+
+          expect(Rails.logger).to receive(:warn).with(
+            /server_bundle_output_path is explicitly set.*shakapacker\.yml private_output_path/
+          )
+
+          ReactOnRails.configure do |config|
+            config.server_bundle_output_path = "custom-path"
+          end
+        end
+
+        it "does not warn when paths match after normalization" do
+          allow(shakapacker_config).to receive(:private_output_path)
+            .and_return(Pathname.new("/fake/rails/root/ssr-generated"))
+
+          expect(Rails.logger).not_to receive(:warn)
+
+          ReactOnRails.configure do |config|
+            config.server_bundle_output_path = "ssr-generated"
+          end
+        end
+      end
+
+      context "when user has not explicitly set server_bundle_output_path" do
+        it "auto-detects from Shakapacker private_output_path" do
+          allow(shakapacker_config).to receive(:private_output_path)
+            .and_return(Pathname.new("/fake/rails/root/shakapacker-bundles"))
+
+          config = nil
+          ReactOnRails.configure do |c|
+            config = c
+          end
+
+          expect(config.server_bundle_output_path).to eq("shakapacker-bundles")
+        end
+
+        it "logs debug message on successful auto-detection" do
+          allow(shakapacker_config).to receive(:private_output_path)
+            .and_return(Pathname.new("/fake/rails/root/auto-detected"))
+
+          expect(Rails.logger).to receive(:debug).with(
+            /Auto-detected server_bundle_output_path.*auto-detected/
+          )
+
+          ReactOnRails.configure { |_config| } # rubocop:disable Lint/EmptyBlock
+        end
+      end
+
+      context "when Shakapacker private_output_path is nil" do
+        it "keeps default value" do
+          allow(shakapacker_config).to receive(:private_output_path).and_return(nil)
+
+          config = nil
+          ReactOnRails.configure do |c|
+            config = c
+          end
+
+          expect(config.server_bundle_output_path).to eq("ssr-generated")
+        end
+      end
+    end
   end
 end
 

spec/react_on_rails/utils_spec.rb

@@ -966,6 +966,18 @@ def self.configuration=(config)
           expect(described_class.normalize_to_relative_path("/other/path/ssr-generated"))
             .to eq("/other/path/ssr-generated")
         end
+
+        it "logs warning for absolute path outside Rails.root" do
+          expect(Rails.logger).to receive(:warn).with(
+            %r{ReactOnRails: Detected absolute path outside Rails\.root: '/other/path/ssr-generated'}
+          )
+          described_class.normalize_to_relative_path("/other/path/ssr-generated")
+        end
+
+        it "does not warn for relative paths" do
+          expect(Rails.logger).not_to receive(:warn)
+          described_class.normalize_to_relative_path("ssr-generated")
+        end
       end
 
       context "with path containing Rails.root as substring" do