feat: add REACT_ON_RAILS_BASE_PORT for concurrent worktree port management

[justin808]

Summary

• Adds a tool-agnostic REACT_ON_RAILS_BASE_PORT env var that derives all service ports from a single base value (Rails = base+0, webpack = base+1, renderer = base+2)
• Also recognizes CONDUCTOR_PORT as a fallback, so Conductor.build works out of the box
• Makes all Procfiles use ${PORT:-3000} and ${RENDERER_PORT:-3800} patterns instead of hardcoded values
• Propagates renderer port via RENDERER_PORT and REACT_RENDERER_URL env vars for automatic Pro Node Renderer configuration

This enables coding agent tools (Conductor.build, Codex, Quad Code, etc.) to run multiple worktrees concurrently without port conflicts — just set one env var and everything works.

Changes

Core logic:

• PortSelector: new base port priority tier (checked before explicit env vars and auto-detect)
• ServerManager.configure_ports: sets RENDERER_PORT and REACT_RENDERER_URL when base port is active

Procfile templates (all now use env var fallbacks):

• OSS: Procfile.dev, Procfile.dev-prod-assets, Procfile.dev-static-assets
• Pro: Procfile.dev, Procfile.prod, Procfile.static
• Generator (pro_setup.rb): uses ${RENDERER_PORT:-3800} in generated Procfile lines

Documentation:

• .env.example: documents base port, RENDERER_PORT, and REACT_RENDERER_URL
• process-managers.md: new "Coding Agent / CI Integration" and "Manual Worktree Port Setup (Pro)" sections

Test plan

• PortSelector unit tests: base port derivation, CONDUCTOR_PORT fallback, priority ordering, out-of-range fallback, nil renderer when no base port
• RuboCop passes on all changed files
• All pre-commit and pre-push hooks pass
• CI passes
• Manual test: REACT_ON_RAILS_BASE_PORT=4000 bin/dev prints correct port assignments
• Generator tests pass (updated assertions for new Procfile pattern)

🤖 Generated with Claude Code

---

Note

Medium Risk
Touches core bin/dev port/env orchestration and introduces unconditional env overrides in base-port mode, which can change local dev behavior and process spawning in subtle ways. Well-covered by new tests, but misconfigurations could still break dev startup for some setups (especially Pro renderer URL/port expectations).

Overview
bin/dev can now run in a deterministic base-port mode via REACT_ON_RAILS_BASE_PORT (or CONDUCTOR_PORT), deriving Rails/webpack/Pro node-renderer ports from a single value and overriding any conflicting per-service env vars with warnings; bin/dev kill also targets the derived ports.

Tightens env handling around ports: invalid PORT/SHAKAPACKER_DEV_SERVER_PORT values are rejected/normalized, runtime port env is preserved correctly across with_unbundled_env, and Pro renderer configuration is made more robust by warning on legacy RENDERER_URL, auto-deriving REACT_RENDERER_URL from RENDERER_PORT, and warning on URL/port mismatches.

Updates Procfiles and generator templates to use shell defaults (${PORT:-...}, ${RENDERER_PORT:-...}), standardizes docs/examples on REACT_RENDERER_URL, expands .env.example and process-manager docs for coding-agent/worktree setup, and adds comprehensive specs covering base-port behavior and validation.

^{Reviewed by Cursor Bugbot for commit 61f0ad2. Bugbot is set up for automated code reviews on this repo. Configure here.}

Summary by CodeRabbit

• New Features

  • Dev ports (Rails, webpack, renderer) can be driven from a single base-port env var, deterministically deriving offsets.

• Behavior Changes

  • Procfiles and examples now honor PORT/RENDERER_PORT via shell-defaults so runtime ports can be overridden.
  • Dev tooling forwards renderer env vars to spawned processes and synchronizes renderer URL/port when derived.

• Documentation

  • Added guidance on base-port usage, precedence, bounds, and manual Pro renderer setup.

• Tests

  • Expanded coverage for base-port derivation, precedence, bounds, invalid values, and fallbacks.

• Chores

  • Minor .gitignore addition.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Adds deterministic base-port support via REACT_ON_RAILS_BASE_PORT (fallback CONDUCTOR_PORT), deriving Rails/webpack/renderer at offsets +0/+1/+2; surfaces renderer in port selection; updates Procfiles, templates, docs, process/env preservation, and specs to prefer environment-driven port values with shell-default expansion.

Changes

Cohort / File(s)	Summary
Core Port Selection & Server Manager react_on_rails/lib/react_on_rails/dev/port_selector.rb, react_on_rails/lib/react_on_rails/dev/server_manager.rb	Adds deterministic base-port mode using REACT_ON_RAILS_BASE_PORT (then CONDUCTOR_PORT) to derive { rails, webpack, renderer } = base+0/+1/+2; adds parsing, validation, offset constants, and branches ServerManager to apply base-derived envs or fall back to explicit/env-default logic with renderer synchronization.
Generators & Templates react_on_rails/lib/generators/react_on_rails/pro_setup.rb, react_on_rails/lib/generators/react_on_rails/templates/base/base/.env.example	Generator output and .env.example document REACT_ON_RAILS_BASE_PORT and use shell-defaults for renderer (RENDERER_PORT=${RENDERER_PORT:-3800}); adds renderer example vars and mapping notes.
Process Env Preservation react_on_rails/lib/react_on_rails/dev/process_manager.rb	Expanded ENV_KEYS_TO_PRESERVE to include RENDERER_PORT and REACT_RENDERER_URL so those vars are forwarded when running processes outside Bundler.
Procfile Fixtures (dummy / spec) react_on_rails/spec/dummy/Procfile.dev, react_on_rails/spec/dummy/Procfile.*, react_on_rails_pro/spec/.../Procfile.*	Procfile entries now honor env-driven ports with shell-default fallbacks (e.g., -p ${PORT:-3000}, RENDERER_PORT=${RENDERER_PORT:-3800}) instead of hardcoded ports.
Tests / Specs react_on_rails/spec/react_on_rails/dev/port_selector_spec.rb, react_on_rails/spec/react_on_rails/dev/server_manager_spec.rb, react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb, react_on_rails/spec/react_on_rails/support/...	Adds/expands specs covering base-port behavior, precedence (REACT_ON_RAILS_BASE_PORT over CONDUCTOR_PORT), out-of-range and invalid-value fallbacks, asserts no port probing when base is used, and updates generator expectations for shell-default renderer port.
Docs & Misc docs/oss/building-features/process-managers.md, .gitignore	Documentation added/expanded describing base-port usage, precedence, and manual Pro worktree setup; .gitignore updated to ignore .gstack/.

Sequence Diagram(s)

sequenceDiagram
    participant Dev as bin/dev
    participant PS as ReactOnRails::Dev::PortSelector
    participant SM as ReactOnRails::Dev::ServerManager
    participant Svc as Services (Rails, Webpack, Node-Renderer)

    Dev->>PS: select_ports()
    alt REACT_ON_RAILS_BASE_PORT detected
        PS->>PS: validate base_port (1..MAX_BASE_PORT)
        PS->>PS: derive rails=base+0, webpack=base+1, renderer=base+2
        PS-->>Dev: {rails, webpack, renderer}
    else CONDUCTOR_PORT detected
        PS->>PS: parse conductor as base
        PS->>PS: derive rails=base+0, webpack=base+1, renderer=base+2
        PS-->>Dev: {rails, webpack, renderer}
    else
        PS->>PS: use explicit envs or probe free ports
        PS-->>Dev: {rails, webpack, nil}
    end

    Dev->>SM: configure_ports(selected)
    alt selected.renderer present
        SM->>SM: set ENV["PORT"], ENV["SHAKAPACKER_DEV_SERVER_PORT"] (overwrite)
        SM->>SM: set ENV["RENDERER_PORT"], ENV["REACT_RENDERER_URL"]
        SM-->>Svc: processes inherit envs
    else
        SM->>SM: apply explicit/env-default PORT and dev-server using ||= 
        SM->>SM: set REACT_RENDERER_URL only if RENDERER_PORT present and URL unset
        SM-->>Svc: processes inherit envs (no renderer)
    end

    Svc->>Svc: bind services to assigned ports

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• fix: use env-var-driven ports in Procfile templates to support multiple worktrees #2539 — Modifies the same port-selection and ServerManager code paths and Procfile/.env templates; closely related.
• Improve create-react-on-rails-app fresh app onboarding #2849 — Overlaps changes in ServerManager and bin/dev startup/env handling.
• 🚀 Enhance bin/dev with configurable routes and improved production asset handling #1790 — Related edits to dev server tooling and ServerManager behavior.

Suggested labels

codex

Suggested reviewers

• AbanoubGhadban
• alexeyr-ci2

Poem

🐰 I found a base port in the glen,
Rails, Webpack, Renderer — lined by ten,
Offsets hopped and ports took their place,
ENV whispers tied each little space,
I thump a drum — dev servers race 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: introducing REACT_ON_RAILS_BASE_PORT for managing concurrent worktree ports, which is the primary feature across all modified files.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jg/concurrent-worktree-ports

---

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.}

[claude]

Code Review

Overview

This PR adds a REACT_ON_RAILS_BASE_PORT env var (with a CONDUCTOR_PORT fallback) that lets coding-agent tools assign all service ports from a single value — Rails = base+0, webpack = base+1, Pro renderer = base+2. All Procfiles are standardized to use ${PORT:-N} patterns and generator output is updated to match. The design is clean and the scope is well-contained.

What Works Well

• Priority chain is sensible — base port > explicit per-service vars > auto-detect. The BASE_PORT_ENV_VARS list makes it easy to add new tool-specific vars later.
• No probing when base port is set — deterministic and fast, which is exactly what CI/agent environments need.
• Test coverage is solid — all priority tiers, edge cases (out-of-range base, both vars set), and the renderer-nil path are covered. The around blocks cleanly save/restore env state.
• Procfile standardization is a good cleanup — ${PORT:-3000} should have been the pattern all along; this PR makes it consistent across OSS and Pro fixtures.
• configure_ports uses ||= correctly — won't stomp on env vars a user already set intentionally.

Issues

Bug (medium) — derived ports can overflow 65535.
base_port validates the raw env var is in 1..65535 but doesn't check that base + 2 (the renderer offset) also stays in range. REACT_ON_RAILS_BASE_PORT=65534 would produce a renderer port of 65536, which is invalid. The fix is to cap the valid base range at 65535 - BASE_PORT_RENDERER_OFFSET. See inline comment.

Misleading log when PORT is already in the environment.
select_ports prints "Base port 4000 detected. Using Rails :4000, webpack :4001, renderer :4002" unconditionally when a base port is found, but configure_ports uses ||= — so a pre-existing PORT=3000 would still be honoured and Rails would actually run on 3000. The log then contradicts reality. Harmless in the primary coding-agent use case (where PORT wouldn't be pre-set), but worth tightening. See inline comment.

Minor — DEFAULT_RENDERER_PORT constant is unused.
It's defined at the top of PortSelector but never referenced in code. The Procfiles and generator both hardcode 3800 inline. Either use the constant or remove it. See inline comment.

Nice-to-have — REACT_RENDERER_URL not auto-derived from explicit RENDERER_PORT.
Manual-worktree users who set RENDERER_PORT=3801 also have to remember to set REACT_RENDERER_URL=http://localhost:3801. A one-liner in configure_ports could derive it automatically. The docs cover it, but easy to miss. See inline comment (can defer to follow-up).

Test Coverage Gap

There's no test for the scenario where REACT_ON_RAILS_BASE_PORT is set and PORT is already in the environment. Given the misleading-log issue above, a test that asserts configure_ports honours the pre-existing PORT (or alternatively overrides it — whichever the intended behavior is) would prevent regressions once the behavior is clarified.

Docs

The new process-managers.md section and .env.example comments are clear and accurate. The offset table is a nice touch. One small nit: the "Priority chain" callout in the docs says "base port > explicit per-service env vars" — worth noting that this is the select_ports priority, not the final configure_ports priority (which still lets pre-existing env vars win via ||=).

Summary

The approach is sound and the feature is useful. The port-overflow bug is the one concrete correctness issue to fix before merging. The log/priority conflict is worth at least a decision: either document that base port always wins (and change ||= to = in configure_ports) or document that pre-existing env vars take final precedence (and fix the log). Everything else can be a follow-up.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 37a9ae17d4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[greptile-apps]

Greptile Summary

Adds REACT_ON_RAILS_BASE_PORT (with CONDUCTOR_PORT as a fallback) to derive all dev service ports from a single base value, enabling concurrent worktree usage without manual per-service config. Procfiles are updated to shell-default patterns (${PORT:-3000}), renderer env vars are propagated through the process manager, and docs/.env.example are expanded with coding-agent guidance.

Prior review concerns — derived renderer port exceeding 65535 and test-environment isolation — have been addressed. Two minor points remain:

• base_port warns on non-integer values but silently ignores out-of-range integers; a consistent warning would help diagnose near-max typos.
• sync_renderer_port_and_url uses String#include? for the port-in-URL check, which can miss a mismatch when the renderer port number is a numeric prefix of the URL's actual port (e.g. 3800 vs. :38000).

Confidence Score: 5/5

Safe to merge — no blocking issues; remaining findings are minor UX/edge-case P2s.

Previous P0/P1 concerns (renderer port > 65535, test env isolation) have been fully addressed. The two remaining issues are P2: a missing warning for out-of-range base ports (silent fallback is safe) and a false-negative in the port-prefix substring check (only affects the warning, not actual behavior). Test coverage is thorough and the core port-selection logic is correct.

react_on_rails/lib/react_on_rails/dev/port_selector.rb and react_on_rails/lib/react_on_rails/dev/server_manager.rb have the minor findings noted above.

Important Files Changed

Filename	Overview
react_on_rails/lib/react_on_rails/dev/port_selector.rb	New base-port logic: validates integer format (warns on bad value) but silently ignores out-of-range integers without a warning; otherwise well-structured with correct MAX_BASE_PORT guard.
react_on_rails/lib/react_on_rails/dev/server_manager.rb	Port configuration split into base-port (unconditional override) and explicit paths; sync_renderer_port_and_url mismatch check uses substring matching which can produce false negatives for port-prefix collisions.
react_on_rails/lib/react_on_rails/dev/process_manager.rb	Correctly extends ENV_KEYS_TO_PRESERVE with RENDERER_PORT and REACT_RENDERER_URL so the vars survive with_unbundled_env when spawning the process manager.
react_on_rails/spec/react_on_rails/dev/port_selector_spec.rb	Top-level around block properly isolates all relevant env vars; comprehensive new coverage for base-port derivation, precedence, out-of-range, and overflow boundary.
react_on_rails/spec/react_on_rails/dev/server_manager_spec.rb	New contexts cover base-port override, explicit RENDERER_PORT derivation, URL-port consistency warning, and internal-host URL (renderer.internal) — good coverage of the new sync logic.
react_on_rails/lib/generators/react_on_rails/pro_setup.rb	Generator line updated to RENDERER_PORT=${RENDERER_PORT:-3800} — allows runtime override while preserving the previous default, consistent with Procfile changes.
docs/oss/building-features/process-managers.md	Adds clear "Coding Agent / CI Integration" and "Manual Worktree Port Setup (Pro)" sections with port-offset table, priority chain, and configuration examples.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A([bin/dev start]) --> B{REACT_ON_RAILS_BASE_PORT or CONDUCTOR_PORT set?}
    B -->|Yes - first valid var wins| C[base_port returns Integer]
    C --> D[select_ports: derive rails=base+0, webpack=base+1, renderer=base+2]
    D --> E[apply_base_port_env: unconditionally set PORT, SHAKAPACKER_DEV_SERVER_PORT, RENDERER_PORT, REACT_RENDERER_URL]
    B -->|No| F{PORT or SHAKAPACKER_DEV_SERVER_PORT set?}
    F -->|Yes| G[Use explicit per-service vars]
    F -->|No| H[Auto-detect free ports starting from defaults]
    G --> I[apply_explicit_port_env: PORT and WEBPACK assigned conditionally]
    H --> I
    I --> J{RENDERER_PORT set?}
    J -->|No| K([Done - renderer not configured])
    J -->|Yes| L{REACT_RENDERER_URL set?}
    L -->|No| M[Derive URL from RENDERER_PORT]
    L -->|Yes - port matches| N([Done - consistent])
    L -->|Yes - port mismatch| O[Warn: vars disagree]
    E --> P([ENV_KEYS_TO_PRESERVE propagated to spawned process manager])
    M --> P
    N --> P
    O --> P

Loading

_{Reviews (3): Last reviewed commit: "fix: second round of port selector revie..." | Re-trigger Greptile}

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@react_on_rails/lib/react_on_rails/dev/port_selector.rb`:
- Around line 98-102: The base_port method currently allows ENV values up to
65_535 but then derives additional ports as base+1 and base+2; update the
validation in base_port (and any use of BASE_PORT_ENV_VARS) to ensure the chosen
base plus the highest offset (2) stays ≤ 65_535 (i.e., accept bases in the range
1..65_533), so change the conditional that returns val to check val between 1
and (65_535 - 2) and reject/skip values outside that safe range.

In `@react_on_rails/lib/react_on_rails/dev/server_manager.rb`:
- Around line 842-845: The current code may set ENV["REACT_RENDERER_URL"] using
selected[:renderer] even when an existing ENV["RENDERER_PORT"] was preserved;
update the logic so REACT_RENDERER_URL is constructed from the effective port
value (the ENV["RENDERER_PORT"] after the ||= assignment) rather than from
selected[:renderer]; locate the block that references selected[:renderer] in
server_manager.rb (the if selected[:renderer] branch) and change the URL
construction to use ENV["RENDERER_PORT"] (or a local port variable assigned from
ENV["RENDERER_PORT"]) so the URL always matches the actual RENDERER_PORT in use.

In `@react_on_rails/spec/react_on_rails/dev/port_selector_spec.rb`:
- Around line 10-15: Wrap each around hook's example.run in a begin...ensure so
ENV cleanup always runs (e.g., change the around do |example| block that sets
ENV["REACT_ON_RAILS_BASE_PORT"] to use begin; example.run; ensure ... end), and
in the "CONDUCTOR_PORT only" and "out-of-range base port" contexts explicitly
unset or restore both ENV["REACT_ON_RAILS_BASE_PORT"] and ENV["CONDUCTOR_PORT"]
(capture old values, set the test values, run example, then in ensure restore
originals or delete the keys when nil) so base_port's lookup order cannot be
influenced by ambient env vars.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e7ac5b86-7952-413d-89b3-278d22119277

📥 Commits

Reviewing files that changed from the base of the PR and between de5b53a and 37a9ae1.

📒 Files selected for processing (17)

• docs/oss/building-features/process-managers.md
• react_on_rails/lib/generators/react_on_rails/pro_setup.rb
• react_on_rails/lib/generators/react_on_rails/templates/base/base/.env.example
• react_on_rails/lib/react_on_rails/dev/port_selector.rb
• react_on_rails/lib/react_on_rails/dev/server_manager.rb
• react_on_rails/spec/dummy/Procfile.dev
• react_on_rails/spec/dummy/Procfile.dev-prod-assets
• react_on_rails/spec/dummy/Procfile.dev-static-assets
• react_on_rails/spec/dummy/Procfile.dev.no.turbolinks
• react_on_rails/spec/react_on_rails/dev/port_selector_spec.rb
• react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
• react_on_rails/spec/react_on_rails/support/shared_examples/pro_generator_examples.rb
• react_on_rails_pro/spec/dummy/Procfile.dev
• react_on_rails_pro/spec/dummy/Procfile.prod
• react_on_rails_pro/spec/dummy/Procfile.static
• react_on_rails_pro/spec/execjs-compatible-dummy/Procfile.dev
• react_on_rails_pro/spec/execjs-compatible-dummy/Procfile.dev-static

[claude]

Code Review

Overall this is a clean, well-motivated feature. The implementation is straightforward and the test coverage is solid. I found a few issues worth addressing before merge.

Bug: base-port priority not enforced in configure_ports

The documented priority chain states "base port > explicit per-service env vars", but the implementation doesn't honour this. In server_manager.rb:

ENV["PORT"] ||= selected[:rails].to_s

If PORT is already set in the environment (e.g. by a CI system or a shell export), ||= is a no-op. select_ports still returns the base-derived port in selected[:rails], but the Procfile reads ${PORT:-3000} — so Rails actually starts on the pre-existing PORT value, silently disagreeing with the log line that says "Using Rails :<base+0>". Either:

• Override explicitly when a base port is detected: ENV["PORT"] = selected[:rails].to_s (only in the base-port branch), or
• Downgrade the documented priority to say base port only wins when PORT is not already set.

Bug: derived ports are not range-validated

base_port validates that the base itself is in 1..65_535, but the derived ports are not checked. A base port of 65_534 would produce renderer port 65_536, which is invalid and would cause a confusing runtime error rather than a clear message.

Dead code: DEFAULT_RENDERER_PORT = 3800

This constant is defined but never referenced anywhere in the Ruby source — the renderer default of 3800 lives only in the Procfile shell fallback (${RENDERER_PORT:-3800}). Either use the constant in code or remove it to avoid misleading future readers.

Missing test: base port takes priority over pre-set PORT

There is no test covering the interaction where REACT_ON_RAILS_BASE_PORT is set AND PORT is already set. This gap would have caught the ||= priority issue above.

Minor: CONDUCTOR_PORT assumption needs verification

The PR assumes CONDUCTOR_PORT is the workspace's allocated base port. If Conductor.build uses that variable for its own internal API rather than the app's port, silently treating it as a Rails base port could cause surprising conflicts. Worth a note in the docs or a code comment citing the Conductor.build docs page that confirms this.

---

None of these are blockers for the feature concept, but the ||= priority bug and the overflow bug could cause hard-to-diagnose issues in the exact concurrent-workspace scenario this PR is designed to solve.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8da5a8a004

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[coderabbitai]

♻️ Duplicate comments (2)

react_on_rails/lib/react_on_rails/dev/server_manager.rb (1)

836-843: ⚠️ Potential issue | 🟠 Major

Apply selected ports as the effective env values.

PortSelector now gives the base port higher priority, but ||= lets pre-existing PORT / SHAKAPACKER_DEV_SERVER_PORT / RENDERER_PORT override that selection before child processes start. This can make REACT_ON_RAILS_BASE_PORT=4000 still boot on stale per-service ports, and it also preserves the previously flagged renderer URL drift.

🐛 Proposed fix

 def configure_ports
   selected = PortSelector.select_ports
-  ENV["PORT"] ||= selected[:rails].to_s
-  ENV["SHAKAPACKER_DEV_SERVER_PORT"] ||= selected[:webpack].to_s
+  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]}"
+    ENV["RENDERER_PORT"] = selected[:renderer].to_s
+    ENV["REACT_RENDERER_URL"] ||= "http://localhost:#{ENV['RENDERER_PORT']}"
   end
 rescue PortSelector::NoPortAvailable => e

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@react_on_rails/lib/react_on_rails/dev/server_manager.rb` around lines 836 -
843, The configure_ports method currently uses ||= which lets pre-existing ENV
values override PortSelector.select_ports; change the assignments in
configure_ports (the selected = PortSelector.select_ports block) to always set
ENV["PORT"], ENV["SHAKAPACKER_DEV_SERVER_PORT"], and when selected[:renderer]
set ENV["RENDERER_PORT"] and ENV["REACT_RENDERER_URL"] using direct assignment
(=) so the selected ports become the effective env values for child processes
and the renderer URL is updated consistently.

react_on_rails/lib/react_on_rails/dev/port_selector.rb (1)

98-102: ⚠️ Potential issue | 🟠 Major

Validate the base port against the highest derived offset.

Line 101 accepts 65_534 and 65_535, but select_ports derives webpack = base + 1 and renderer = base + 2, producing invalid ports above 65_535.

🐛 Proposed fix

 def base_port
+  max_base_port = 65_535 - BASE_PORT_RENDERER_OFFSET
   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

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@react_on_rails/lib/react_on_rails/dev/port_selector.rb` around lines 98 -
102, The base_port method currently accepts ENV values up to 65_535 but
select_ports computes webpack = base + 1 and renderer = base + 2, which can
yield ports > 65_535; update the validation in base_port (method name base_port,
referenced by select_ports) to enforce an upper bound of 65_533 (i.e., ensure
val.between?(1, 65_533) or subtract the maximum derived offset before
validation) so that adding +1 and +2 cannot produce invalid ports.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@react_on_rails/lib/react_on_rails/dev/port_selector.rb`:
- Around line 98-102: The base_port method currently accepts ENV values up to
65_535 but select_ports computes webpack = base + 1 and renderer = base + 2,
which can yield ports > 65_535; update the validation in base_port (method name
base_port, referenced by select_ports) to enforce an upper bound of 65_533
(i.e., ensure val.between?(1, 65_533) or subtract the maximum derived offset
before validation) so that adding +1 and +2 cannot produce invalid ports.

In `@react_on_rails/lib/react_on_rails/dev/server_manager.rb`:
- Around line 836-843: The configure_ports method currently uses ||= which lets
pre-existing ENV values override PortSelector.select_ports; change the
assignments in configure_ports (the selected = PortSelector.select_ports block)
to always set ENV["PORT"], ENV["SHAKAPACKER_DEV_SERVER_PORT"], and when
selected[:renderer] set ENV["RENDERER_PORT"] and ENV["REACT_RENDERER_URL"] using
direct assignment (=) so the selected ports become the effective env values for
child processes and the renderer URL is updated consistently.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 89dd322b-8ac2-4e3f-9f75-b9ef2476038a

📥 Commits

Reviewing files that changed from the base of the PR and between 4216467 and 8da5a8a.

📒 Files selected for processing (18)

• .gitignore
• docs/oss/building-features/process-managers.md
• react_on_rails/lib/generators/react_on_rails/pro_setup.rb
• react_on_rails/lib/generators/react_on_rails/templates/base/base/.env.example
• react_on_rails/lib/react_on_rails/dev/port_selector.rb
• react_on_rails/lib/react_on_rails/dev/server_manager.rb
• react_on_rails/spec/dummy/Procfile.dev
• react_on_rails/spec/dummy/Procfile.dev-prod-assets
• react_on_rails/spec/dummy/Procfile.dev-static-assets
• react_on_rails/spec/dummy/Procfile.dev.no.turbolinks
• react_on_rails/spec/react_on_rails/dev/port_selector_spec.rb
• react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
• react_on_rails/spec/react_on_rails/support/shared_examples/pro_generator_examples.rb
• react_on_rails_pro/spec/dummy/Procfile.dev
• react_on_rails_pro/spec/dummy/Procfile.prod
• react_on_rails_pro/spec/dummy/Procfile.static
• react_on_rails_pro/spec/execjs-compatible-dummy/Procfile.dev
• react_on_rails_pro/spec/execjs-compatible-dummy/Procfile.dev-static

✅ Files skipped from review due to trivial changes (13)

• .gitignore
• react_on_rails_pro/spec/execjs-compatible-dummy/Procfile.dev-static
• react_on_rails/spec/dummy/Procfile.dev-static-assets
• react_on_rails/spec/dummy/Procfile.dev.no.turbolinks
• react_on_rails/spec/dummy/Procfile.dev-prod-assets
• react_on_rails_pro/spec/execjs-compatible-dummy/Procfile.dev
• react_on_rails/lib/generators/react_on_rails/pro_setup.rb
• react_on_rails_pro/spec/dummy/Procfile.prod
• react_on_rails/lib/generators/react_on_rails/templates/base/base/.env.example
• react_on_rails/spec/react_on_rails/generators/install_generator_spec.rb
• react_on_rails/spec/react_on_rails/support/shared_examples/pro_generator_examples.rb
• react_on_rails/spec/dummy/Procfile.dev
• docs/oss/building-features/process-managers.md

🚧 Files skipped from review as they are similar to previous changes (3)

• react_on_rails_pro/spec/dummy/Procfile.static
• react_on_rails_pro/spec/dummy/Procfile.dev
• react_on_rails/spec/react_on_rails/dev/port_selector_spec.rb

[justin808]

Review Feedback Addressed (commit b6e16df)

I (Claude Code) went through all inline review comments from @claude, @cursor, @chatgpt-codex-connector, @coderabbitai, and @greptile and addressed every real issue raised. After triage (deduplicating overlapping comments across reviewers), six distinct concerns were identified and all six are now fixed in a single commit.

Fixes

1. Base port overflow (P1/Major from multiple reviewers) — PortSelector.base_port now validates against MAX_BASE_PORT (65_535 - BASE_PORT_RENDERER_OFFSET). A base of 65_534 previously passed the 1..65_535 check but produced an invalid renderer port of 65_536.
2. Base port priority violated by ||= (cursor High, claude) — ServerManager.configure_ports now splits the base-port and explicit-port paths. When a base port is active, PORT / SHAKAPACKER_DEV_SERVER_PORT / RENDERER_PORT / REACT_RENDERER_URL are assigned unconditionally so the effective ports match the Base port … detected log line even when those env vars were pre-set (e.g., from a .env file).
3. RENDERER_PORT / REACT_RENDERER_URL lost across Bundler.with_unbundled_env (codex P1) — Added both keys to ProcessManager::ENV_KEYS_TO_PRESERVE so they survive run_process_outside_bundle.
4. Missing test for configure_ports when PORT is pre-set with base port active (claude) — Added a when configuring ports with a base port active context to server_manager_spec that covers the override.
5. Unused DEFAULT_RENDERER_PORT constant (claude ×2, cursor Low) — Removed. The 3800 fallback only lives in the Procfile shell expression ${RENDERER_PORT:-3800}.
6. REACT_RENDERER_URL can drift from RENDERER_PORT (coderabbit Major, claude) — In the non-base-port path, configure_ports now derives REACT_RENDERER_URL from an explicit RENDERER_PORT when only the port is set. Added a regression spec.

Tests

Added 5 new specs to port_selector_spec.rb and server_manager_spec.rb covering the max-safe base port, overflow fallback, base-port override of pre-existing env vars, and explicit-RENDERER_PORT URL derivation. Full suite (176 examples) passes locally.

On the bot summary comments (@claude, @greptile, @coderabbitai)

Their summary bodies restate the inline findings that are already addressed above — no separate action needed. The earlier CodeRabbit "processing" notice is a stale status post.

All 16 unresolved review threads are now resolved.

[chatgpt-codex-connector]

To use Codex here, create an environment for this repo.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

Let me quickly verify the key changes in the updated code.

---

🏁 Script executed:

#!/bin/bash
echo "=== PortSelector: base_port method + MAX_BASE_PORT ==="
rg -n "MAX_BASE_PORT|base_port|BASE_PORT_RENDERER_OFFSET" react_on_rails/lib/react_on_rails/dev/port_selector.rb -A2 -B2

echo ""
echo "=== ServerManager: configure_ports ==="
cat react_on_rails/lib/react_on_rails/dev/server_manager.rb

echo ""
echo "=== ProcessManager: ENV_KEYS_TO_PRESERVE ==="
rg -n "ENV_KEYS_TO_PRESERVE" -A5 -B2

echo ""
echo "=== DEFAULT_RENDERER_PORT removed? ==="
rg -n "DEFAULT_RENDERER_PORT" react_on_rails/lib/react_on_rails/dev/port_selector.rb

Length of output: 58432

---

@justin808 — Thank you for the thorough write-up! I've verified all six fixes in the updated code:

1. MAX_BASE_PORT overflow guard ✅ — base_port now validates against MAX_BASE_PORT = 65_535 - BASE_PORT_RENDERER_OFFSET, correctly rejecting values like 65,534 that would produce an out-of-range renderer port.

2. Unconditional base-port assignment ✅ — configure_ports is cleanly split into apply_base_port_env (assigns all four env vars unconditionally) and apply_explicit_port_env (uses ||=), so the "Base port detected" log line always matches the effective ports.

3. ENV_KEYS_TO_PRESERVE ✅ — Both RENDERER_PORT and REACT_RENDERER_URL are present in ProcessManager::ENV_KEYS_TO_PRESERVE, ensuring they survive Bundler.with_unbundled_env.

4. DEFAULT_RENDERER_PORT removed ✅ — No matches found; the 3800 fallback now lives exclusively in the Procfile shell expression.

5. REACT_RENDERER_URL/RENDERER_PORT drift (my Major comment) ✅ — apply_explicit_port_env now derives REACT_RENDERER_URL from an explicit RENDERER_PORT when the URL is unset.

Everything looks solid. The apply_base_port_env / apply_explicit_port_env split is a nice clean separation of concerns too. Great work resolving all of the feedback!

---

🧠 Learnings used

Learnt from: justin808
Repo: shakacode/react_on_rails PR: 0
File: :0-0
Timestamp: 2026-04-13T02:29:20.920Z
Learning: In `react_on_rails_pro/lib/react_on_rails_pro/engine.rb` and `react_on_rails/lib/react_on_rails/configuration.rb` (PR `#3094`): Do NOT reset `rendering_strategy` or `js_code_builder` inside `ReactOnRails.configure`. The Pro engine sets these exactly once during `after_initialize`; resetting them on reconfigure would silently drop Pro's NodeStrategy/ProJsCodeBuilder assignments. A proper reconfigure mechanism for these accessors is deferred to Step 3 of the strategy-pattern refactor (`#2905`).

---

Learnt from: ihabadham
Repo: shakacode/react_on_rails PR: 2284
File: react_on_rails/lib/generators/react_on_rails/templates/pro/base/config/initializers/react_on_rails_pro.rb.tt:10-10
Timestamp: 2026-02-12T20:43:26.843Z
Learning: In React on Rails Pro initializer templates (e.g., `react_on_rails_pro.rb.tt`), the `config.renderer_password` uses `ENV.fetch("RENDERER_PASSWORD", "devPassword")` as a development starter configuration. This pattern is intentional: the node renderer requires explicit production deployment setup, and adding a production-only raise would break generator execution since initializers are loaded during `rails generate`. The pattern is consistent with existing manual setup documentation.

---

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: react_on_rails_pro/CLAUDE.md:0-0
Timestamp: 2026-02-24T16:52:47.694Z
Learning: Applies to react_on_rails_pro/spec/dummy/commonWebpackConfig.js : Changes to `commonWebpackConfig.js` affect all three webpack bundles (client, server, and RSC) in the Pro dummy app

---

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-12T23:15:11.806Z
Learning: Applies to CHANGELOG.md : Changelog format: `[PR 1818](https://github.com/shakacode/react_on_rails/pull/1818) by [username](https://github.com/username)` (no hash before PR number)

---

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: react_on_rails_pro/CLAUDE.md:0-0
Timestamp: 2026-02-24T16:52:47.694Z
Learning: Applies to react_on_rails_pro/**/*.{js,ts,tsx,jsx,json,css,md} : Pro Prettier check should be run with: `cd react_on_rails_pro && pnpm run prettier --check .`

---

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: react_on_rails_pro/CLAUDE.md:0-0
Timestamp: 2026-02-24T16:52:47.694Z
Learning: Applies to react_on_rails_pro/spec/react_on_rails_pro/**/*.rb : Pro gem unit tests should be run with: `cd react_on_rails_pro && bundle exec rspec spec/react_on_rails_pro/`

---

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: react_on_rails_pro/CLAUDE.md:0-0
Timestamp: 2026-02-24T16:52:47.694Z
Learning: Applies to react_on_rails_pro/spec/dummy/spec/**/*.rb : Pro integration tests should be run with: `cd react_on_rails_pro/spec/dummy && bundle exec rspec spec/`

---

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-17T22:10:33.821Z
Learning: Applies to react_on_rails_pro/** : Ask before modifying the Pro package (`react_on_rails_pro/`)

---

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: react_on_rails_pro/CLAUDE.md:0-0
Timestamp: 2026-02-24T16:52:47.694Z
Learning: Applies to react_on_rails_pro/packages/react-on-rails-pro/**/*.test.rsc.{ts,tsx} : Pro RSC tests should be run with: `pnpm --filter react-on-rails-pro run test:rsc`

---

Learnt from: CR
Repo: shakacode/react_on_rails PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-12T23:15:11.806Z
Learning: Applies to react_on_rails/spec/dummy/e2e/playwright/e2e/**/*.{js,ts} : Use Playwright for E2E tests in `react_on_rails/spec/dummy/e2e/playwright/e2e/`

[justin808]

Review round 13 — all 10 unresolved threads addressed

I (Claude Code) swept the 10 open review threads and resolved each one across two commits on this branch.

00c58af — fix: honor REACT_ON_RAILS_BASE_PORT in bin/dev prod mode

• run_production_like now routes through the same base-port path as dev/static (apply_base_port_if_active shared across all three modes). Closes the Codex P2 gap where parallel worktrees running bin/dev prod collided on 3001 instead of honoring REACT_ON_RAILS_BASE_PORT / CONDUCTOR_PORT.
• Extracted PortSelector.base_port_ports as a public non-probing accessor.
• sync_renderer_port_and_url now uses PortSelector.valid_port_string?, so RENDERER_PORT=" 3800 " is accepted consistently (whitespace stripping matches).
• warn_if_legacy_renderer_url_env_used now also warns when both RENDERER_URL and REACT_RENDERER_URL are set but their values disagree (previously silent mid-migration drift).
• All pro_setup.rb manual-update hints/warnings (lines 232, 264, 276, 291) now emit RENDERER_PORT=${RENDERER_PORT:-3800} so users following the guidance end up with the base-port-compatible form.
• New specs: prod-assets + base port env vars, whitespace-padded RENDERER_PORT, RENDERER_URL divergence warning.

9617025 — fix: thirteenth round of port selector review feedback

• url_port_mismatch? regex replaced with %r{://(?:[^@/]*@)?[^@/:]+:\d+}. Explicitly steps past an optional userinfo@ prefix and anchors the port to the authority host — no backtracking into password digits. Resolves both the false-positive for URLs like http://user:3800@localhost and the related style nit asking for a :-excluded host charset. Added a regression spec.
• sync_renderer_port_and_url derived-URL announcement moved from puts → warn, matching every other env-mutation message in the file (so bin/dev 2>/dev/null silences it consistently). Updated the matching spec.
• PortSelector.parse_explicit_port_env renamed to consume_explicit_port_env so the ENV.delete side effect is visible in the method name. The tagged-result + configure_ports split suggested as an alternative was left for a follow-up — it was explicitly flagged "not blocking" and would touch every select_ports caller.
• spec/dummy/package.json node-renderer and node-renderer-debug scripts now use RENDERER_PORT=${RENDERER_PORT:-3800}, structurally matching Procfile.static. Both entry points honor REACT_ON_RAILS_BASE_PORT / RENDERER_PORT identically; Procfile comment updated to note the mirror.

Verification

• bundle exec rspec spec/react_on_rails/dev/{server_manager,port_selector}_spec.rb → 212 examples, 0 failures
• bundle exec rspec spec/react_on_rails/generators/pro_generator_spec.rb → 95 examples, 0 failures
• bundle exec rubocop lib/react_on_rails/dev/ spec/react_on_rails/dev/ → clean
• pnpm run prettier --check spec/dummy/package.json → clean

All 10 threads have been replied to and resolved inline.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

• ✅ Fixed: IPv6 URLs cause false mismatch warning in port check
  • Updated the port-presence guard regex to accept bracketed IPv6 hosts and added a spec proving matching IPv6 URL ports no longer trigger a false mismatch warning.

Or push these changes by commenting:

@cursor push eb5650762c

Preview (eb5650762c)

diff --git a/react_on_rails/lib/react_on_rails/dev/server_manager.rb b/react_on_rails/lib/react_on_rails/dev/server_manager.rb
--- a/react_on_rails/lib/react_on_rails/dev/server_manager.rb
+++ b/react_on_rails/lib/react_on_rails/dev/server_manager.rb
@@ -1065,11 +1065,12 @@
         # Regex anatomy: the optional `(?:[^@/]*@)?` steps past a userinfo
         # prefix like `user:pass@` so a URL such as
         # `http://user:3800@localhost` does not match its password as a host
-        # port via backtracking. The host charset excludes both `/` and `:`
-        # so the `:\d+` port anchor lands on the authority separator without
+        # port via backtracking. The host matcher accepts either a normal host
+        # token (excluding `/` and `:`) or a bracketed IPv6 literal so the
+        # `:\d+` port anchor lands on the authority separator without
         # backtracking into the host.
         def url_port_mismatch?(url, port)
-          return true unless url.match?(%r{://(?:[^@/]*@)?[^@/:]+:\d+})
+          return true unless url.match?(%r{://(?:[^@/]*@)?(?:\[[^\]]+\]|[^@/:]+):\d+})
 
           URI.parse(url).port != port.to_i
         rescue URI::InvalidURIError

diff --git a/react_on_rails/spec/react_on_rails/dev/server_manager_spec.rb b/react_on_rails/spec/react_on_rails/dev/server_manager_spec.rb
--- a/react_on_rails/spec/react_on_rails/dev/server_manager_spec.rb
+++ b/react_on_rails/spec/react_on_rails/dev/server_manager_spec.rb
@@ -729,6 +729,12 @@
           .to output(%r{Clearing REACT_RENDERER_URL=http://\[::1\]:3900}).to_stderr
         expect(ENV.fetch("REACT_RENDERER_URL", nil)).to be_nil
       end
+
+      it "does not warn when RENDERER_PORT matches an IPv6 localhost URL port" do
+        ENV["RENDERER_PORT"] = "3800"
+        ENV["REACT_RENDERER_URL"] = "http://[::1]:3800"
+        expect { described_class.start(:development) }.not_to output(/does not match/).to_stderr
+      end
     end
 
     context "when REACT_RENDERER_URL has no explicit port" do

_{You can send follow-ups to the cloud agent here.}

[claude]

Bug: IPv6 URLs always reported as a port mismatch

[^@/:]+ in the host group excludes :, so for an IPv6 URL like http://[::1]:3800 the regex never matches — [^@/:]+ matches only the leading [ and stops at the first :. The method then hits return true (mismatch) before reaching URI.parse, so REACT_RENDERER_URL=http://[::1]:3800 with RENDERER_PORT=3800 will always trigger a spurious override warning.

localhost_renderer_url? (line 1083) handles ::1 correctly via URI.parse(url).hostname, so the same user who hits this branch gets a localhost-URL warning that's technically wrong too (the URL/port actually agree).

Suggested fix — extend the host alternatives to accept bracketed IPv6:

Suggested change

	def url_port_mismatch?(url, port)
	return true unless url.match?(%r{://(?:[^@/]*@)?[^@/:]+:\d+})
	URI.parse(url).port != port.to_i
	rescue URI::InvalidURIError
	false
	end
	def url_port_mismatch?(url, port)
	return true unless url.match?(%r{://(?:[^@/]*@)?(?:\[[^\]]+\]|[^@/:]+):\d+})
	URI.parse(url).port != port.to_i
	rescue URI::InvalidURIError
	false
	end

The (?:\[[^\]]+\]|[^@/:]+) alternative matches either an IPv6 literal ([::1], [::ffff:…]) or a regular hostname/IPv4 address.

[claude]

Minor: unquoted existing value in warning differs from every other env-mutation warning in this file

consume_explicit_port_env, overwrite_invalid_port_env, and warn_if_renderer_url_will_be_overridden all use #{existing.inspect} (or #{raw.inspect}) so whitespace and special characters show up visibly. This warning uses bare #{existing}, so a value like " 3000" or an empty-looking string looks identical to "3000" and "".

Suggested change

	def warn_if_port_will_be_overridden(var_name, derived_port)
	existing = ENV.fetch(var_name, nil)
	return if existing.nil? || existing.strip.empty? || existing == derived_port.to_s
	warn "WARNING: Overriding #{var_name}=#{existing} with #{derived_port} " \
	"because base port mode is active."
	def warn_if_port_will_be_overridden(var_name, derived_port)
	existing = ENV.fetch(var_name, nil)
	return if existing.nil? || existing.strip.empty? || existing == derived_port.to_s
	warn "WARNING: Overriding #{var_name}=#{existing.inspect} with #{derived_port} " \
	"because base port mode is active."
	end

[claude]

Design note: base_port_ports has stdout + TCP side-effects on every call, and select_ports calls it internally

base_port_ports (a) prints the "Base port detected…" log line and (b) does three TCP probes via warn_if_derived_ports_in_use. Since select_ports delegates to base_port_ports on line 52-53, any caller that invokes both base_port_ports directly and select_ports later (or vice-versa) will double-print and double-probe.

The current configure_ports flow avoids this with return if apply_base_port_if_active (so select_ports is never called when base-port mode is active), but base_port_ports is public and the guard is invisible at the call site.

Two options worth considering:

1. Cache the result — memoize base_port_ports for the lifetime of the process so repeated calls are idempotent.
2. Split detection from application — keep a side-effect-free base_port? / detect_base_port predicate and move the print + TCP check into the site that first applies the result.

As-is the code is correct, but this is a latent footgun if a future caller uses select_ports without checking base_port_ports first.

[claude]

Parallel port-setup path in run_production_like vs configure_ports

This method has its own inline port-setup logic (apply base port, then validate/auto-select PORT) while all other modes go through configure_ports. The two paths are functionally equivalent today, but they'll diverge the next time someone adds a new env-var check to configure_ports and forgets this branch.

Worth adding a comment that explicitly ties the two paths together, e.g.:

# NOTE: run_production_like has its own port-setup path instead of calling
# configure_ports because it also needs to run assets:precompile before foreman
# starts. Keep this in sync with configure_ports when the port-setup logic changes.

Also, this block validates PORT but not SHAKAPACKER_DEV_SERVER_PORT (unlike configure_ports → apply_explicit_port_env). That's probably intentional since webpack-dev-server isn't started in prod-like mode, but a comment would clarify the intentional omission.

[justin808]

Review round 14 — IPv6 port-mismatch false positive fixed

I (Claude Code) addressed the single new review comment posted after the round 13 sweep.

Fixed

• cursor[bot] — IPv6 URLs cause false mismatch warning — url_port_mismatch? regex host alternative now accepts bracketed IPv6 literals, so RENDERER_PORT=3800 + REACT_RENDERER_URL=http://[::1]:3800 no longer emits a spurious "does not match" warning. Commit fd5162694. Added two regression tests (agree + disagree) under the IPv6 spec context.

[claude]

Code Review

Overall assessment: This is a well-designed feature with thorough test coverage and good defensive coding (validation, warnings for every env-var override, explicit handling of legacy RENDERER_URL). The architecture is sound. A few issues worth addressing before merge:

---

Bug: IPv6 URL false-positive mismatch

Flagged inline on url_port_mismatch?. The regex [^@/:]+ excludes :, so it cannot consume an IPv6 bracketed address like [::1]. For REACT_RENDERER_URL=http://[::1]:3800 the regex fails to match, the method hits return true (mismatch), and URI.parse is never reached — so a correctly-configured IPv6 renderer URL always fires a spurious override warning. localhost_renderer_url? next door handles ::1 correctly via URI.parse(url).hostname, so the two methods are inconsistent. Fix is one line in the host group regex — see the inline suggestion.

---

Design: base_port_ports has side-effects on every call

Flagged inline on PortSelector#base_port_ports. The method both prints to stdout and does three TCP probes each time it's invoked. select_ports calls it internally, and ServerManager also calls it directly via apply_base_port_if_active. The return if apply_base_port_if_active guard in configure_ports prevents the current double-call, but the guard is invisible to future callers of select_ports. Memoizing the result (or splitting detection from application) would make this safer.

---

Maintenance: run_production_like has a parallel port-setup path

Flagged inline. All other modes go through configure_ports; prod-like has its own copy. The two paths are in sync today but will silently diverge on the next env-var addition. A comment pointing to configure_ports as the canonical reference, and noting the intentional omission of SHAKAPACKER_DEV_SERVER_PORT validation, would help future maintainers.

---

Minor: inconsistent .inspect in override warning

Flagged inline on warn_if_port_will_be_overridden. Every other env-mutation warning in this file uses #{value.inspect} so whitespace in env var values is visible; this one uses bare #{existing}. One-line fix in the suggestion.

---

Notes (not blocking)

• CONDUCTOR_PORT semantics: Clearly documented as an empirical interpretation with an explicit escape hatch (REACT_ON_RAILS_BASE_PORT takes precedence). The documentation and code comments are honest about the fragility. Reasonable trade-off.
• Generator specs: The RENDERER_PORT=3800 instances remaining in pro_generator_spec.rb are all test-setup data for legacy/existing Procfile content that the generator deliberately leaves untouched — not assertions about generated output. Those tests are correctly updated.
• Test isolation: The outer around block in port_selector_spec.rb that saves and restores all port-related env vars is a good pattern; the inner per-context around blocks that set but don't restore are correct by design (the outer ensure covers them).

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 4 total unresolved issues (including 3 from previous reviews).

Bugbot Autofix prepared a fix for the issue found in the latest run.

• ✅ Fixed: Base port rejects whitespace-padded values unlike other port parsers
  • base_port_with_source now strips surrounding whitespace before integer validation/parsing and a spec verifies whitespace-padded base port values are accepted.

Or push these changes by commenting:

@cursor push dd1567d448

Preview (dd1567d448)

diff --git a/react_on_rails/lib/react_on_rails/dev/port_selector.rb b/react_on_rails/lib/react_on_rails/dev/port_selector.rb
--- a/react_on_rails/lib/react_on_rails/dev/port_selector.rb
+++ b/react_on_rails/lib/react_on_rails/dev/port_selector.rb
@@ -157,14 +157,17 @@
           # within the valid TCP port range (1..65_535).
           BASE_PORT_ENV_VARS.each_with_index do |var, idx|
             raw = ENV.fetch(var, nil)
-            next if raw.nil? || raw.empty?
+            next if raw.nil?
 
-            unless raw.match?(/\A\d+\z/)
+            stripped = raw.strip
+            next if stripped.empty?
+
+            unless stripped.match?(/\A\d+\z/)
               warn invalid_base_port_warning(var, raw, "not a valid integer", idx)
               next
             end
 
-            val = raw.to_i
+            val = stripped.to_i
             unless val.between?(1, MAX_BASE_PORT)
               warn invalid_base_port_warning(var, raw, "out of range (1..#{MAX_BASE_PORT})", idx)
               next

diff --git a/react_on_rails/spec/react_on_rails/dev/port_selector_spec.rb b/react_on_rails/spec/react_on_rails/dev/port_selector_spec.rb
--- a/react_on_rails/spec/react_on_rails/dev/port_selector_spec.rb
+++ b/react_on_rails/spec/react_on_rails/dev/port_selector_spec.rb
@@ -71,6 +71,20 @@
       end
     end
 
+    context "when REACT_ON_RAILS_BASE_PORT contains surrounding whitespace" do
+      around do |example|
+        ENV["REACT_ON_RAILS_BASE_PORT"] = "  5000  "
+        example.run
+      end
+
+      before { allow(described_class).to receive(:port_available?).and_return(true) }
+
+      it "accepts the value and derives ports from the stripped integer" do
+        result = described_class.select_ports
+        expect(result).to include(rails: 5000, webpack: 5001, renderer: 5002, base_port_mode: true)
+      end
+    end
+
     context "when CONDUCTOR_PORT is set (without REACT_ON_RAILS_BASE_PORT)" do
       around do |example|
         ENV["CONDUCTOR_PORT"] = "6000"

_{You can send follow-ups to the cloud agent here.}

^{Reviewed by Cursor Bugbot for commit fd51626. Configure here.}

[claude]

Code Review — PR #3142: REACT_ON_RAILS_BASE_PORT for concurrent worktree port management

Overview

A well-motivated feature for coding-agent and multi-worktree scenarios. The priority chain (base port → explicit env vars → auto-detect) is clearly designed, the tests are thorough, and the documentation is honest about the CONDUCTOR_PORT assumption being empirical. Below are a few issues worth addressing before merge.

---

Bugs / Correctness Issues

1. sync_renderer_port_and_url is never called in production-like mode without base port

In run_development and run_static_development, configure_ports is called, which (when base port is inactive) calls apply_explicit_port_env → sync_renderer_port_and_url. This means:

• RENDERER_PORT=3800 set without REACT_RENDERER_URL → REACT_RENDERER_URL is auto-derived ✅
• Mismatch warnings between RENDERER_PORT and REACT_RENDERER_URL are surfaced ✅

But in run_production_like, configure_ports is not called. Without base port, only PORT is touched — RENDERER_PORT/REACT_RENDERER_URL synchronization is skipped entirely. The spec comment on line 220 acknowledges this: "production-like mode does not call configure_ports".

This is an inconsistency: bin/dev prod behaves differently from bin/dev for Pro users relying on RENDERER_PORT auto-derivation. If Procfile.dev-prod-assets doesn't start a node renderer (OSS case) this is harmless, but it becomes a real issue if users build a Pro-flavored production-assets Procfile that includes the renderer.

Simplest fix: call sync_renderer_port_and_url (or the full configure_ports) in run_production_like alongside the existing warn_if_legacy_renderer_url_env_used.

---

2. kill_processes hardcodes ports [3000, 3001]

killed_any = kill_running_processes || kill_port_processes([3000, 3001]) || cleanup_socket_files

With base port mode, a worktree running on port 5000 calling bin/dev kill will miss the port-based kill for 5000/5001/5002. The process-pattern kill (kill_running_processes) is likely to catch most cases anyway, but kill_port_processes is the last-resort fallback. Consider reading PortSelector.base_port_ports here to augment the kill list with derived ports.

---

Design Concerns

3. base_port_ports is a public method with stdout/stderr side effects

base_port_ports does two things: returns a hash and prints "Base port X detected via Y..." to stdout (plus potential warn calls from warn_if_derived_ports_in_use). Being public and side-effecting means any external caller — or a future refactor that calls it twice — would double-print the log.

Both callers (via apply_base_port_if_active) each call it exactly once today, so this is not a current bug. But tests have to stub it explicitly to avoid the side effects, which signals the coupling is load-bearing. Consider splitting into a pure computation method and a thin logging wrapper, or memoizing after first call.

4. consume_explicit_port_env mutates ENV as a side effect

Named consume_* precisely because it deletes from ENV for invalid values. The comment is clear, but this is an unusual contract for a private method and creates invisible coupling between PortSelector and ServerManager. The current tests handle it correctly; worth flagging as technical debt.

---

Minor / Positive Notes

• Test coverage is excellent. The around env-save/restore blocks are the right pattern, include_context "with clean port env" avoids repetition, and boundary conditions (0, 1, 1023, 1024, MAX_BASE_PORT, MAX_BASE_PORT+1) are all exercised.
• The CONDUCTOR_PORT comment is appropriately honest about the empirical nature of the contract and the escape hatch. Good defensive documentation.
• valid_port_string? is correctly shared via delegation rather than duplicated. The comment explaining the String#to_i truncation hazard is valuable.
• preserve_runtime_env_vars always includes nil values — the right choice to ensure with_unbundled_env doesn't resurrect scrubbed env vars in child processes.
• The procfile changes to ${PORT:-3000} / ${RENDERER_PORT:-3800} are backward-compatible and correct.

---

Summary

The core feature is sound and well-tested. Two correctness issues worth addressing before merge:

1. sync_renderer_port_and_url not called in production-like mode without base port (affects Pro users running bin/dev prod with RENDERER_PORT set).
2. kill_processes unaware of base-port-derived ports (minor; process-pattern kill compensates in most cases).

[justin808]

Review round 15 — 7 fixes + 1 rationale reply

I (Claude Code) addressed the new review comments posted after round 14. All changes are in d8dc1cce8.

Fixed (code + specs)

1. cursor[bot] — whitespace in base port env var — base_port_with_source now strips raw before the integer regex and to_i, matching consume_explicit_port_env / valid_port_string?. REACT_ON_RAILS_BASE_PORT=" 5000 " is now accepted as 5000.
2. claude[bot] — sync_renderer_port_and_url in production-like — run_production_like now calls sync_renderer_port_and_url in the non-base-port branch so bin/dev prod auto-derives REACT_RENDERER_URL from RENDERER_PORT and surfaces mismatch warnings.
3. claude[bot] — base_port_ports public side effects — split into pure base_port_hash (no logging) + logging base_port_ports wrapper; they share a private derive_ports_from_base helper.
4. claude[bot] — spec for sync in production-like — closed the gap rather than locking it in. Added specs asserting auto-derivation + mismatch warning in production-like mode without base port.
5. claude[bot] — .inspect in override warnings (code review 1) — warn_if_port_will_be_overridden and warn_if_renderer_url_will_be_overridden now .inspect the existing value, matching every other env-mutation warning in the file. Updated corresponding specs.
6. claude[bot] — run_production_like divergence comment (code review 1) — added a comment block explaining why this path is structurally similar to configure_ports but intentionally different (scans from 3001, omits SHAKAPACKER_DEV_SERVER_PORT).
7. claude[bot] — kill_processes hardcodes [3000, 3001] (code review 2) — new killable_ports helper reads PortSelector.base_port_hash so bin/dev kill targets the worktree's actual ports in base-port mode.

Rationale reply (no code change)

• claude[bot] — consume_explicit_port_env ENV mutation — reviewer explicitly noted the current approach is acceptable; I replied explaining why the sentinel-return refactor is being left as deferred design debt rather than bundled into this PR.

Overall suite: 316 dev/ specs passing, RuboCop clean.

[claude]

Code Review: PR #3142 — REACT_ON_RAILS_BASE_PORT deterministic port allocation

Overview

This PR adds a single-env-var port-allocation mechanism for concurrent worktree development (coding agents, CI). The design is sound: one base port → three derived ports via fixed offsets, with a CONDUCTOR_PORT fallback and an escape hatch where REACT_ON_RAILS_BASE_PORT always wins. Procfiles, generators, docs, and specs are all updated consistently. Test coverage is excellent throughout.

---

Bugs / Correctness Concerns

1. base_port_ports is called twice in the non-base-port path

configure_ports calls apply_base_port_if_active → PortSelector.base_port_ports. When that returns false it then calls PortSelector.select_ports, which internally also calls base_port_ports. So when no base port is set, base_port_with_source runs twice. It's cheap (just ENV reads) and harmless, but it's unnecessary. Consider having select_ports accept the result of base_port_ports as an argument, or restructure configure_ports to call select_ports directly and let select_ports own the branching.

2. ${RENDERER_PORT:-3800} in package.json won't expand on Windows

package.json scripts run via cmd.exe on Windows, which doesn't support POSIX shell parameter expansion. The change in react_on_rails_pro/spec/dummy/package.json will silently pass the literal string ${RENDERER_PORT:-3800} to node on Windows. If Windows support matters (even for CI), consider a cross-platform wrapper script or note the limitation explicitly.

---

Design / Code Quality Observations

3. ENV mutation as a side effect of consume_explicit_port_env is surprising

Deleting from ENV inside what appears to be a parse/validate helper creates hidden coupling to ServerManager. The coupling is documented (the naming, the comment), but anyone reading explicit_rails_port in isolation won't expect it to delete env vars. Consider renaming the private method to parse_and_clear_invalid_port_env! to make the mutation obvious from the call site, or extracting the deletion into a dedicated cleanup step before either selector is called.

4. SHAKAPACKER_DEV_SERVER_PORT is set in production-like mode under base port

When base-port mode is active and start(:production_like) runs, apply_base_port_env unconditionally sets SHAKAPACKER_DEV_SERVER_PORT even though prod-like mode doesn't start webpack-dev-server. The test comment explains the rationale (tooling consistency), but this could confuse users inspecting their env after bin/dev prod. At minimum, a code comment at the call site in run_production_like would help future readers.

5. CONDUCTOR_PORT contract is an assumption, not a public API

This is well-documented in both code and docs, and the escape hatch via REACT_ON_RAILS_BASE_PORT is the right mitigation. Worth adding a note that if Conductor changes the semantics of CONDUCTOR_PORT in the future, users should pin REACT_ON_RAILS_BASE_PORT explicitly. The existing NOTE in the docs is close but could be more direct: "treat CONDUCTOR_PORT support as best-effort until Conductor documents this contract."

---

Positive Highlights

• Comprehensive test coverage: edge cases for privilege ranges, non-integer values, whitespace, out-of-range, IPv6 URLs, basic-auth URLs, and the full precedence chain. The around block for env isolation is cleaner than the old manual save/restore pattern.
• base_port_hash vs base_port_ports: splitting the silent-derivation path (for kill) from the logged path (for startup) is a good design.
• url_port_mismatch? regex: handles IPv6 and user:password@ correctly; the URI.parse fallback prevents crashes on malformed URLs.
• warn_if_legacy_renderer_url_env_used: a genuinely helpful migration aid for the RENDERER_URL → REACT_RENDERER_URL rename.
• preserve_runtime_env_vars now passes nil for unset keys: prevents scrubbed-but-invalid values from being resurrected by Bundler's env snapshot in child processes — a subtle correctness fix.
• Procfile env-var fallback pattern (${PORT:-3000}): strictly more flexible than hardcoded ports and backwards-compatible.

---

Minor Nits

• warn_url_without_port only warns for localhost URLs. A remote URL without a port (e.g. http://renderer.internal) could also be a misconfiguration — the renderer binds to 3800 via the Procfile default, but Rails would target port 80 (http default). Worth a broader warning or at least a doc note.
• The base_port_mode: false key added to the non-base-port select_ports return value is useful, but the existing "default ports are free" spec does not assert it — only the new contexts check it.
• killable_ports in the non-base-port path returns only [3000, 3001] with no renderer port. This is a pre-existing gap rather than a regression, but since this PR adds renderer awareness to the kill path in base-port mode, a follow-up to add a configurable renderer port to the fallback list would round things out.

---

Summary

The feature is well-implemented and meaningfully improves the multi-worktree development experience. The two correctness concerns (redundant base_port_ports call in the non-base-port path; Windows shell expansion in package.json) are low severity. The ENV-mutation coupling in consume_explicit_port_env is the most significant design smell and worth at minimum a naming change to make the side-effect self-documenting at the call site.

[justin808]

Review round 16 — 5 design/doc polish fixes

I (Claude Code) addressed the 4 new inline review comments posted after round 15 plus the two summary-only items from the latest code review. All changes are in 61f0ad286.

Fixed (code + specs + docs)

1. claude[bot] — consume_explicit_port_env naming — Renamed consume_explicit_port_env → consume_explicit_port_env!. The ! suffix signals the ENV-mutation side effect at both call sites (explicit_rails_port, explicit_webpack_port) per Ruby convention. Updated the method doc and the spec comment that referenced the old name.
2. claude[bot] — redundant base_port_ports call — configure_ports now calls PortSelector.select_ports once and branches on :base_port_mode, so base_port_ports runs exactly once per invocation. apply_base_port_if_active is retained for run_production_like (which doesn't go through select_ports). Updated the "with clean port env" base-port spec context to stub both base_port_ports and select_ports consistently.
3. claude[bot] — POSIX shell expansion in package.json — Added a "//" comment key at the top of the Pro dummy scripts object noting that ${RENDERER_PORT:-3800} requires POSIX shell and won't expand on Windows cmd.exe, and that this dummy app is Linux/macOS only. No JS wrapper since the dummy spec app only runs on Linux/macOS CI — a contributor-facing note is enough.
4. claude[bot] — localhost-only warn_url_without_port — Added a doc comment above warn_url_without_port explaining why remote URLs are intentionally excluded: this process doesn't launch remote renderers, so scheme-default ports (80/443) may be correct behind a reverse proxy, and remote-side port mismatches are a deployment concern. Kept the warning localhost-only rather than broadening it to avoid false positives on legitimate remote-renderer setups.
5. claude[bot] — code review summary, two doc touches:
   • apply_base_port_env doc now explains why SHAKAPACKER_DEV_SERVER_PORT is set even in production-like mode (tooling consistency: a subsequent bin/dev in the same shell sees the base-port-derived value rather than a stale explicit one).
   • CONDUCTOR_PORT note in both docs/oss/building-features/process-managers.md and port_selector.rb tightened to say "best-effort until Conductor documents the contract," matching the reviewer's wording.

Test / lint status

• react_on_rails/spec/react_on_rails/dev/: 316 examples, 0 failures
• RuboCop on lib/react_on_rails/dev/ + spec/react_on_rails/dev/: clean

All 4 inline threads resolved. No deferred items from this round.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 61f0ad2865

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Preserve legacy renderer URL env var in base-port mode

In base-port mode we derive and export REACT_RENDERER_URL but never set RENDERER_URL. That breaks Pro apps that still configure config.renderer_url from the legacy env name (a migration path this same commit acknowledges via warn_if_legacy_renderer_url_env_used): the node renderer is moved to RENDERER_PORT=base+2, while Rails can still target the old/default URL and fail SSR requests. Mirroring the derived URL to RENDERER_URL (or equivalent compatibility handling) would avoid this regression for older initializers.

Useful? React with 👍 / 👎.

[claude]

OSS users: renderer port is always included in the kill list

When base-port mode is active, killable_ports always returns [rails, webpack, renderer], even for apps that don't use the Pro Node Renderer. Calling kill_port_processes on base + 2 is a no-op if nothing is listening there, but it silently kills any unrelated process that happens to occupy that port — which is more likely here than on the default port list because the port was never claimed by a renderer.

Consider guarding with a Pro check, or at least document this expansion:

Suggested change

	[base[:rails], base[:webpack], base[:renderer]]
	[base[:rails], base[:webpack], base[:renderer]].compact

This at least handles the hypothetical future case where renderer could be nil, but doesn't fully address the OSS concern. A more explicit fix would check whether the Pro renderer is actually configured before including base[:renderer].

[claude]

ENV mutation inside a "selector" class

PortSelector is conceptually a pure-computation layer (select ports from inputs, return a hash), but consume_explicit_port_env! writes a side-effect into ENV by deleting invalid values. The ! suffix and the block comment above document this, but it still means PortSelector and ServerManager are coupled through shared global state (ENV) rather than through return values.

This is a design tradeoff that's acceptable here for the "warn once" guarantee, but worth flagging if this class gets reused in other contexts (e.g., tests that isolate ENV carefully, or future background threads). The comment should be preserved exactly as written so future readers can trace why the coupling exists.

No immediate action needed — just documenting the tradeoff for reviewers.

[claude]

User-facing warning message uses MAX_BASE_PORT (65533) not 65535

The warning prints "out of range (1..65533)". A developer who sets REACT_ON_RAILS_BASE_PORT=65534 will see that message and likely be confused — 65534 is a perfectly valid TCP port, but it's rejected here because 65534 + 2 = 65536 would overflow the renderer offset.

Consider making the reason explicit in the warning:

Suggested change

	warn invalid_base_port_warning(var, raw, "out of range (1..#{MAX_BASE_PORT})", idx)
	warn invalid_base_port_warning(var, raw, "out of range (1..#{MAX_BASE_PORT}; must leave room for +#{BASE_PORT_RENDERER_OFFSET} renderer offset)", idx)

Or alternatively, just call out the constraint in the warning body inside invalid_base_port_warning.

[claude]

SHAKAPACKER_DEV_SERVER_PORT set in production-like mode

apply_base_port_env is called from both the development/static and production-like code paths. In production-like mode (static assets, no webpack-dev-server), setting SHAKAPACKER_DEV_SERVER_PORT is harmless but surprising to users who inspect their environment after bin/dev prod.

The block comment at line 957–962 explains this as "tooling consistency," which is a reasonable justification. However, this means the test at server_manager_spec.rb:1627 is asserting a somewhat non-obvious property:

"applies SHAKAPACKER_DEV_SERVER_PORT from base+1 even though prod mode doesn't use webpack-dev-server"

It might be worth adding a comment inside the test itself (not just the it description) explaining that this is intentional rather than a test of a misguided implementation, so future readers don't remove the behaviour thinking it's a bug.

[claude]

Review: PR #3142 — REACT_ON_RAILS_BASE_PORT for concurrent worktree port management

Overview

This is a well-scoped, well-tested feature. The priority chain (base port → explicit per-service env vars → auto-detect) is clear, the edge-case coverage is excellent, and the documentation is honest about the CONDUCTOR_PORT contract being unofficial/best-effort. The base_port_hash vs base_port_ports distinction (silent vs. logged) shows careful attention to callers that should not produce output (e.g. kill flows). Procfile templates now properly honor runtime env vars instead of hardcoding ports.

---

Issues

1. killable_ports includes renderer port unconditionally — potential unintended kill for OSS users

(inline comment on server_manager.rb:73)

The old fallback was [3000, 3001]; the new base-port path returns [rails, webpack, renderer] unconditionally. For OSS users without the Pro Node Renderer, nothing owns base + 2, so kill_port_processes is a no-op in the normal case. But if anything unrelated happens to be on that port (more likely given nothing claimed it), it gets silently killed. Worth a .compact at minimum, and ideally a guard that only includes the renderer port when the Pro renderer is actually in use.

2. User-visible warning uses MAX_BASE_PORT (65533) instead of 65535 — confusing near the upper boundary

(inline comment on port_selector.rb:201)

A developer setting REACT_ON_RAILS_BASE_PORT=65534 sees "out of range (1..65533)". That number is surprising because 65534 is a valid TCP port. The underlying constraint (must leave room for the +2 renderer offset) is not surfaced. Suggest adding the reason, e.g. "out of range (1..65533; must leave room for +2 renderer offset)".

3. consume_explicit_port_env! mutates ENV inside PortSelector — layer boundary crossed

(inline comment on port_selector.rb:244)

The existing comment documents this intentionally, so no action is strictly required. Flagging for future readers: if PortSelector is extracted or reused, the hidden ENV side-effect is a trap. The coupling is kept in one tight place which is the right tradeoff.

4. apply_base_port_env sets SHAKAPACKER_DEV_SERVER_PORT in production-like mode

(inline comment on server_manager.rb:970)

Setting a webpack-dev-server port env var when running static assets is harmless but surprising. The decision is documented and tested. Suggestion: add a brief inline comment to the test that asserts this so future maintainers do not accidentally remove the behavior thinking it was a test of a bug.

---

What is done well

• Test isolation: the around blocks in port_selector_spec.rb and the shared "with clean port env" context in server_manager_spec.rb robustly handle developer environments where REACT_ON_RAILS_BASE_PORT or CONDUCTOR_PORT may already be set. This is exactly the right approach.
• IPv6 + basic-auth URL handling: the url_port_mismatch? regex correctly prevents userinfo-colon and IPv6-bracket backtracking bugs, and dedicated tests verify both. Thorough.
• Bundler env resurrection fix: changing preserve_runtime_env_vars to pass nil for unset keys (rather than omitting them) is a subtle but correct fix — it prevents with_unbundled_env from restoring a pre-Bundler invalid value that PortSelector deliberately cleared. The test name explains the invariant clearly.
• base_port_hash / base_port_ports split: the silent vs. logging distinction is clean and well-documented.
• Documentation: the CONDUCTOR_PORT disclaimers ("treat as best-effort", "escape hatch: REACT_ON_RAILS_BASE_PORT takes precedence") are appropriately hedged for an unofficial contract.

---

Minor nits

• procfile_port in run_production_like calls ENV.fetch("PORT", 3001).to_i after the invalid-PORT normalization block has already cleared bad values. A brief inline comment that ENV["PORT"] is guaranteed clean at that point would aid readability.
• The warning range string "1..65535" in consume_explicit_port_env! (line 259) is inconsistent with "1..#{MAX_BASE_PORT}" used for base port validation. Both are correct for their context (per-service ports are validated against the full TCP range), but a shared constant for the display string would make the inconsistency explicit.

Overall: approve with the renderer-port kill issue flagged as the one item worth addressing before merge. The rest are documentation or cosmetic suggestions.