fix: address review feedback on base port feature

Addresses review comments on PR #3142 (REACT_ON_RAILS_BASE_PORT):

- Validate base port against the largest derived offset so base+N stays
  within the valid TCP port range (1..65_535). A base of 65_534 previously
  passed validation but produced an invalid renderer port of 65_536.
- Enforce the documented priority chain ("base port > explicit per-service
  env vars") in ServerManager.configure_ports. The former `||=` assignments
  silently preserved a pre-existing PORT/SHAKAPACKER_DEV_SERVER_PORT, making
  the effective Rails port disagree with the "Base port detected..." log line.
  When a base port is active, per-service env vars are now assigned
  unconditionally.
- Preserve RENDERER_PORT and REACT_RENDERER_URL across Bundler.with_unbundled_env
  by adding them to ProcessManager::ENV_KEYS_TO_PRESERVE. Previously they
  were dropped for process managers launched outside the bundler context.
- Derive REACT_RENDERER_URL from an explicit RENDERER_PORT in non-base-port
  worktrees when the user set the port but not the URL.
- Remove the unused DEFAULT_RENDERER_PORT constant (the 3800 fallback only
  lives in Procfile shell expressions).
- Add port_selector_spec cases for the max-safe and overflow base ports,
  and server_manager_spec cases for base-port priority override and
  explicit-RENDERER_PORT URL sync.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: justin808

react_on_rails/lib/react_on_rails/dev/port_selector.rb

@@ -5,17 +5,17 @@
 module ReactOnRails
   module Dev
     class PortSelector
-      DEFAULT_RAILS_PORT    = 3000
-      DEFAULT_WEBPACK_PORT  = 3035
-      DEFAULT_RENDERER_PORT = 3800
-      MAX_ATTEMPTS          = 100
+      DEFAULT_RAILS_PORT   = 3000
+      DEFAULT_WEBPACK_PORT = 3035
+      MAX_ATTEMPTS         = 100
 
       # Offsets from the base port when REACT_ON_RAILS_BASE_PORT (or a recognized
       # tool-specific equivalent like CONDUCTOR_PORT) is set. The base port block
       # is typically 10 consecutive ports allocated per workspace.
       BASE_PORT_RAILS_OFFSET    = 0
       BASE_PORT_WEBPACK_OFFSET  = 1
       BASE_PORT_RENDERER_OFFSET = 2
+      MAX_BASE_PORT = 65_535 - BASE_PORT_RENDERER_OFFSET
 
       # Env vars checked (in order) for a base port value.
       BASE_PORT_ENV_VARS = %w[REACT_ON_RAILS_BASE_PORT CONDUCTOR_PORT].freeze
@@ -96,9 +96,11 @@ def find_available_port(start_port, exclude: nil)
         private
 
         def base_port
+          # Upper bound accounts for the largest derived offset so base + N stays
+          # within the valid TCP port range (1..65_535).
           BASE_PORT_ENV_VARS.each do |var|
             val = ENV[var]&.to_i
-            return val if val&.between?(1, 65_535)
+            return val if val&.between?(1, MAX_BASE_PORT)
           end
           nil
         end

react_on_rails/lib/react_on_rails/dev/process_manager.rb

@@ -14,7 +14,12 @@ class ProcessManager
       # before entering the block and pass them explicitly to system().
       # This follows the same pattern used by Rails' bundle_command (railties),
       # Spring's process spawning, and this codebase's own PackGenerator.
-      ENV_KEYS_TO_PRESERVE = %w[PORT SHAKAPACKER_DEV_SERVER_PORT].freeze
+      ENV_KEYS_TO_PRESERVE = %w[
+        PORT
+        SHAKAPACKER_DEV_SERVER_PORT
+        RENDERER_PORT
+        REACT_RENDERER_URL
+      ].freeze
 
       class << self
         # Check if a process is available and usable in the current execution context

react_on_rails/lib/react_on_rails/dev/server_manager.rb

@@ -835,17 +835,36 @@ def print_procfile_info(procfile, route: nil)
 
         def configure_ports
           selected = PortSelector.select_ports
-          ENV["PORT"] ||= selected[:rails].to_s
-          ENV["SHAKAPACKER_DEV_SERVER_PORT"] ||= selected[:webpack].to_s
           if selected[:renderer]
-            ENV["RENDERER_PORT"] ||= selected[:renderer].to_s
-            ENV["REACT_RENDERER_URL"] ||= "http://localhost:#{selected[:renderer]}"
+            apply_base_port_env(selected)
+          else
+            apply_explicit_port_env(selected)
           end
         rescue PortSelector::NoPortAvailable => e
           warn e.message
           exit 1
         end
 
+        # Base port is active. Priority: base port > explicit per-service env vars.
+        # Assign unconditionally so the effective ports match the "Base port
+        # detected..." log line even when PORT/RENDERER_PORT were pre-set.
+        def apply_base_port_env(selected)
+          ENV["PORT"] = selected[:rails].to_s
+          ENV["SHAKAPACKER_DEV_SERVER_PORT"] = selected[:webpack].to_s
+          ENV["RENDERER_PORT"] = selected[:renderer].to_s
+          ENV["REACT_RENDERER_URL"] = "http://localhost:#{selected[:renderer]}"
+        end
+
+        def apply_explicit_port_env(selected)
+          ENV["PORT"] ||= selected[:rails].to_s
+          ENV["SHAKAPACKER_DEV_SERVER_PORT"] ||= selected[:webpack].to_s
+          # Non-base-port worktree: if a user set RENDERER_PORT explicitly but
+          # not REACT_RENDERER_URL, keep them in sync so Rails reaches the right port.
+          return unless ENV["RENDERER_PORT"] && !ENV["REACT_RENDERER_URL"]
+
+          ENV["REACT_RENDERER_URL"] = "http://localhost:#{ENV.fetch('RENDERER_PORT')}"
+        end
+
         def procfile_port(procfile)
           if procfile == "Procfile.dev-prod-assets"
             ENV.fetch("PORT", 3001).to_i

react_on_rails/spec/react_on_rails/dev/port_selector_spec.rb

@@ -88,6 +88,40 @@
       end
     end
 
+    context "when base port would push derived renderer port above 65535" do
+      around do |example|
+        old = ENV.fetch("REACT_ON_RAILS_BASE_PORT", nil)
+        # 65_534 + BASE_PORT_RENDERER_OFFSET (2) = 65_536, which is invalid.
+        ENV["REACT_ON_RAILS_BASE_PORT"] = "65534"
+        example.run
+        ENV["REACT_ON_RAILS_BASE_PORT"] = old
+      end
+
+      it "falls back to normal auto-detection instead of deriving an invalid port" do
+        allow(described_class).to receive(:port_available?).and_return(true)
+        result = described_class.select_ports
+        expect(result[:rails]).to eq(3000)
+        expect(result[:renderer]).to be_nil
+      end
+    end
+
+    context "when base port is at the maximum safe value" do
+      around do |example|
+        old = ENV.fetch("REACT_ON_RAILS_BASE_PORT", nil)
+        ENV["REACT_ON_RAILS_BASE_PORT"] = described_class::MAX_BASE_PORT.to_s
+        example.run
+        ENV["REACT_ON_RAILS_BASE_PORT"] = old
+      end
+
+      it "derives all three ports within the valid TCP range" do
+        result = described_class.select_ports
+        expect(result[:rails]).to eq(described_class::MAX_BASE_PORT)
+        expect(result[:webpack]).to eq(described_class::MAX_BASE_PORT + 1)
+        expect(result[:renderer]).to eq(described_class::MAX_BASE_PORT + 2)
+        expect(result[:renderer]).to be <= 65_535
+      end
+    end
+
     context "when default ports are free" do
       it "returns the default Rails port 3000" do
         allow(described_class).to receive(:port_available?).and_return(true)

react_on_rails/spec/react_on_rails/dev/server_manager_spec.rb

@@ -261,6 +261,73 @@ def mock_system_calls
         expect { described_class.start(:development) }.not_to raise_error
       end
     end
+
+    context "when configuring ports with a base port active" do
+      before do
+        mock_system_calls
+        allow(ReactOnRails::Dev::PortSelector).to receive(:select_ports)
+          .and_return({ rails: 5000, webpack: 5001, renderer: 5002 })
+      end
+
+      around do |example|
+        keys = %w[PORT SHAKAPACKER_DEV_SERVER_PORT RENDERER_PORT REACT_RENDERER_URL]
+        old = keys.to_h { |k| [k, ENV.fetch(k, nil)] }
+        keys.each { |k| ENV.delete(k) }
+        example.run
+      ensure
+        old.each { |k, v| v.nil? ? ENV.delete(k) : ENV[k] = v }
+      end
+
+      it "overrides a pre-existing PORT with the base-derived Rails port" do
+        ENV["PORT"] = "3000"
+        described_class.start(:development)
+        expect(ENV.fetch("PORT", nil)).to eq("5000")
+      end
+
+      it "overrides a pre-existing SHAKAPACKER_DEV_SERVER_PORT with the base-derived webpack port" do
+        ENV["SHAKAPACKER_DEV_SERVER_PORT"] = "3035"
+        described_class.start(:development)
+        expect(ENV.fetch("SHAKAPACKER_DEV_SERVER_PORT", nil)).to eq("5001")
+      end
+
+      it "overrides pre-existing RENDERER_PORT and REACT_RENDERER_URL with base-derived values" do
+        ENV["RENDERER_PORT"] = "3800"
+        ENV["REACT_RENDERER_URL"] = "http://localhost:3800"
+        described_class.start(:development)
+        expect(ENV.fetch("RENDERER_PORT", nil)).to eq("5002")
+        expect(ENV.fetch("REACT_RENDERER_URL", nil)).to eq("http://localhost:5002")
+      end
+    end
+
+    context "when RENDERER_PORT is set explicitly without a base port" do
+      before do
+        mock_system_calls
+        allow(ReactOnRails::Dev::PortSelector).to receive(:select_ports)
+          .and_return({ rails: 3000, webpack: 3035, renderer: nil })
+      end
+
+      around do |example|
+        keys = %w[PORT SHAKAPACKER_DEV_SERVER_PORT RENDERER_PORT REACT_RENDERER_URL]
+        old = keys.to_h { |k| [k, ENV.fetch(k, nil)] }
+        keys.each { |k| ENV.delete(k) }
+        example.run
+      ensure
+        old.each { |k, v| v.nil? ? ENV.delete(k) : ENV[k] = v }
+      end
+
+      it "derives REACT_RENDERER_URL from the explicit RENDERER_PORT" do
+        ENV["RENDERER_PORT"] = "3801"
+        described_class.start(:development)
+        expect(ENV.fetch("REACT_RENDERER_URL", nil)).to eq("http://localhost:3801")
+      end
+
+      it "leaves a pre-existing REACT_RENDERER_URL untouched" do
+        ENV["RENDERER_PORT"] = "3801"
+        ENV["REACT_RENDERER_URL"] = "http://renderer.internal:3801"
+        described_class.start(:development)
+        expect(ENV.fetch("REACT_RENDERER_URL", nil)).to eq("http://renderer.internal:3801")
+      end
+    end
   end
 
   describe "browser auto-open readiness" do