feat: add REACT_ON_RAILS_BASE_PORT for concurrent worktree port management#3142
feat: add REACT_ON_RAILS_BASE_PORT for concurrent worktree port management#3142
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds deterministic base-port support via Changes
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
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Code ReviewOverviewThis PR adds a What Works Well
IssuesBug (medium) — derived ports can overflow 65535. Misleading log when Minor — Nice-to-have — Test Coverage GapThere's no test for the scenario where DocsThe new process-managers.md section and SummaryThe 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 🤖 Generated with Claude Code |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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 SummaryAdds Prior review concerns — derived renderer port exceeding 65535 and test-environment isolation — have been addressed. Two minor points remain:
Confidence Score: 5/5Safe 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
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
Reviews (3): Last reviewed commit: "fix: second round of port selector revie..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
📒 Files selected for processing (17)
docs/oss/building-features/process-managers.mdreact_on_rails/lib/generators/react_on_rails/pro_setup.rbreact_on_rails/lib/generators/react_on_rails/templates/base/base/.env.examplereact_on_rails/lib/react_on_rails/dev/port_selector.rbreact_on_rails/lib/react_on_rails/dev/server_manager.rbreact_on_rails/spec/dummy/Procfile.devreact_on_rails/spec/dummy/Procfile.dev-prod-assetsreact_on_rails/spec/dummy/Procfile.dev-static-assetsreact_on_rails/spec/dummy/Procfile.dev.no.turbolinksreact_on_rails/spec/react_on_rails/dev/port_selector_spec.rbreact_on_rails/spec/react_on_rails/generators/install_generator_spec.rbreact_on_rails/spec/react_on_rails/support/shared_examples/pro_generator_examples.rbreact_on_rails_pro/spec/dummy/Procfile.devreact_on_rails_pro/spec/dummy/Procfile.prodreact_on_rails_pro/spec/dummy/Procfile.staticreact_on_rails_pro/spec/execjs-compatible-dummy/Procfile.devreact_on_rails_pro/spec/execjs-compatible-dummy/Procfile.dev-static
## Summary - Applies `prettier --write` to five tracked docs files that are currently out of compliance on `main`. - No content changes — only whitespace/table-alignment fixes produced by Prettier. ## Why this is needed The formatting drift landed via a docs-only merge to `main`. The `lint-js-and-ruby.yml` workflow is conditionally skipped for docs-only commits on `main` (see the job condition at [`.github/workflows/lint-js-and-ruby.yml:79-87`](https://github.com/shakacode/react_on_rails/blob/main/.github/workflows/lint-js-and-ruby.yml#L79-L87)), so the check never ran on merge. It surfaces on every subsequent code-touching PR because the lint job runs for those (e.g. [#3151](#3151), [#3148](#3148), [#3142](#3142), [#3097](#3097)). Merging this unblocks the `build` → `Check formatting` step on those PRs. ## Test plan - [x] `pnpm start format.listDifferent` passes for the five files - [x] Diff is purely whitespace — no link targets, code blocks, or prose changed 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Improved formatting across API reference, core concepts, migration, and RSC docs: refined table layouts, adjusted column alignment, and standardized emphasis for clearer warnings. * Updated link text and phrasing for several guides for clearer navigation. * Reformatted code examples and JSX/Route snippets for more compact, consistent presentation and improved readability. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
justin808
force-pushed
the
jg/concurrent-worktree-ports
branch
from
4216467 to
8da5a8a
Compare
Code ReviewOverall 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
|
There was a problem hiding this comment.
💡 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".
There was a problem hiding this comment.
♻️ Duplicate comments (2)
react_on_rails/lib/react_on_rails/dev/server_manager.rb (1)
836-843:⚠️ Potential issue | 🟠 MajorApply selected ports as the effective env values.
PortSelectornow gives the base port higher priority, but||=lets pre-existingPORT/SHAKAPACKER_DEV_SERVER_PORT/RENDERER_PORToverride that selection before child processes start. This can makeREACT_ON_RAILS_BASE_PORT=4000still 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 | 🟠 MajorValidate the base port against the highest derived offset.
Line 101 accepts
65_534and65_535, butselect_portsderiveswebpack = base + 1andrenderer = base + 2, producing invalid ports above65_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
📒 Files selected for processing (18)
.gitignoredocs/oss/building-features/process-managers.mdreact_on_rails/lib/generators/react_on_rails/pro_setup.rbreact_on_rails/lib/generators/react_on_rails/templates/base/base/.env.examplereact_on_rails/lib/react_on_rails/dev/port_selector.rbreact_on_rails/lib/react_on_rails/dev/server_manager.rbreact_on_rails/spec/dummy/Procfile.devreact_on_rails/spec/dummy/Procfile.dev-prod-assetsreact_on_rails/spec/dummy/Procfile.dev-static-assetsreact_on_rails/spec/dummy/Procfile.dev.no.turbolinksreact_on_rails/spec/react_on_rails/dev/port_selector_spec.rbreact_on_rails/spec/react_on_rails/generators/install_generator_spec.rbreact_on_rails/spec/react_on_rails/support/shared_examples/pro_generator_examples.rbreact_on_rails_pro/spec/dummy/Procfile.devreact_on_rails_pro/spec/dummy/Procfile.prodreact_on_rails_pro/spec/dummy/Procfile.staticreact_on_rails_pro/spec/execjs-compatible-dummy/Procfile.devreact_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
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) <[email protected]>
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
TestsAdded 5 new specs to 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. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
To use Codex here, create an environment for this repo. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me quickly verify the key changes in the updated code. 🏁 Script executed: Length of output: 58432
Everything looks solid. The 🧠 Learnings used |
Addresses the review comments posted after round 14: 1. `base_port_with_source` now strips whitespace before the integer regex so `REACT_ON_RAILS_BASE_PORT=" 5000 "` is accepted the same way `PORT=" 5000 "` already was via `consume_explicit_port_env`. 2. `run_production_like` now runs `sync_renderer_port_and_url` in the non-base-port branch so `bin/dev prod` auto-derives `REACT_RENDERER_URL` from a bare `RENDERER_PORT` and surfaces mismatch warnings — matching `bin/dev` and `bin/dev static`. 3. `PortSelector` gains a pure `#base_port_hash` accessor that returns the derived port tuple without the "Base port X detected" banner or derived-port-collision warnings. `#base_port_ports` still logs; callers that want the quiet path (e.g. `kill_processes`) can use the new helper. 4. `kill_processes` now reads `PortSelector.base_port_hash` so `bin/dev kill` targets the worktree's actual ports in base-port mode instead of the stale `[3000, 3001]` fallback. 5. `warn_if_port_will_be_overridden` and `warn_if_renderer_url_will_be_overridden` now `.inspect` the existing value so whitespace in env var values is visible — matching every other env-mutation warning in this file. 6. `run_production_like` gains a comment pointing at `configure_ports` as the canonical per-mode env-setup reference and explaining the two intentional differences (PORT scan starts at 3001, omits `SHAKAPACKER_DEV_SERVER_PORT` validation). Spec coverage: whitespace-trim test for base port, new IPv6-style spec context for production-like renderer sync, kill-processes base-port spec, and a `#base_port_hash` describe block that locks in the absence of stdout/stderr side effects. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Review round 15 — 7 fixes + 1 rationale replyI (Claude Code) addressed the new review comments posted after round 14. All changes are in d8dc1cce8. Fixed (code + specs)
Rationale reply (no code change)
Overall suite: 316 dev/ specs passing, RuboCop clean. |
Code Review: PR #3142 — REACT_ON_RAILS_BASE_PORT deterministic port allocationOverviewThis 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 Bugs / Correctness Concerns1.
2.
Design / Code Quality Observations3. ENV mutation as a side effect of Deleting from 4. When base-port mode is active and 5. This is well-documented in both code and docs, and the escape hatch via Positive Highlights
Minor Nits
SummaryThe feature is well-implemented and meaningfully improves the multi-worktree development experience. The two correctness concerns (redundant |
1. Rename PortSelector.consume_explicit_port_env → consume_explicit_port_env!
so the ENV-mutation side effect is self-documenting at call sites
(explicit_rails_port, explicit_webpack_port).
2. configure_ports now calls PortSelector.select_ports once and branches on
:base_port_mode, eliminating the redundant base_port_ports invocation
that ran on every non-base-port call. apply_base_port_if_active is
retained for run_production_like, which doesn't go through select_ports.
3. Add POSIX-shell caveat "//" comment to react_on_rails_pro/spec/dummy
package.json noting ${RENDERER_PORT:-3800} won't expand on Windows
cmd.exe; this dummy app is Linux/macOS only.
4. Document why warn_url_without_port deliberately excludes remote URLs
(scheme-default ports may be correct behind a reverse proxy; bin/dev
doesn't launch remote renderers so it can't diagnose safely).
5. Two doc clarifications from the summary review:
- apply_base_port_env comment explains why SHAKAPACKER_DEV_SERVER_PORT
is set even in production-like mode (tooling consistency).
- CONDUCTOR_PORT note tightened to say "best-effort until Conductor
documents the contract" in both docs and port_selector.rb comment.
Specs: 316 dev/ examples passing. RuboCop clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Review round 16 — 5 design/doc polish fixesI (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)
Test / lint status
All 4 inline threads resolved. No deferred items from this round. |
There was a problem hiding this comment.
💡 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".
| 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"] = derived_url |
There was a problem hiding this comment.
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 👍 / 👎.
| base = PortSelector.base_port_hash | ||
| return [3000, 3001] unless base | ||
|
|
||
| [base[:rails], base[:webpack], base[:renderer]] |
There was a problem hiding this comment.
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:
| [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].
| # (explicit_rails_port / explicit_webpack_port); the "warn once + fall | ||
| # back" flow is shared with ServerManager via the cleared env, not via | ||
| # the return value. Kept in one place so the coupling is obvious. | ||
| def consume_explicit_port_env!(var_name) |
There was a problem hiding this comment.
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.
|
|
||
| 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) |
There was a problem hiding this comment.
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:
| 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.
| warn_if_port_will_be_overridden("SHAKAPACKER_DEV_SERVER_PORT", selected[:webpack]) | ||
| warn_if_port_will_be_overridden("RENDERER_PORT", selected[:renderer]) | ||
| ENV["PORT"] = selected[:rails].to_s | ||
| ENV["SHAKAPACKER_DEV_SERVER_PORT"] = selected[:webpack].to_s |
There was a problem hiding this comment.
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.
Review: PR #3142 —
|
Code Review: PR 3142 — REACT_ON_RAILS_BASE_PORT for concurrent worktree port managementOverviewThis PR introduces a clean, tool-agnostic mechanism for running multiple dev stacks concurrently by deriving all service ports from a single base env var. The scope is well-contained, the documentation is clear, and the test coverage is excellent. A few items worth discussing before merging are called out below. What works well
Concern 1:
|
| end | ||
|
|
||
| def working_directory_for_pid(pid) | ||
| path_line = `lsof -a -p #{pid} -d cwd -Fn 2>/dev/null`.lines.find { |line| line.start_with?("n") } |
There was a problem hiding this comment.
lsof is not installed by default on minimal Linux hosts (Alpine, Debian slim, many CI images). When it is absent this returns nil, which is a safe fallback — the PID file is kept rather than deleted. Consider a /proc/PID/cwd readlink path as a zero-dependency primary attempt on Linux before falling back to lsof. This would help users in container environments who copy an app directory and get a stale server.pid they cannot automatically clear.
| def socket_active?(socket_path) | ||
| return false unless File.exist?(socket_path) | ||
|
|
||
| UNIXSocket.open(socket_path, &:close) |
There was a problem hiding this comment.
UNIXSocket.open has no connect timeout. If the socket file exists and the server's accept queue is full (extremely rare for a local overmind socket but theoretically possible), connect(2) will block indefinitely here and stall bin/dev startup. A non-blocking probe using Socket#connect_nonblock + IO::WaitWritable rescue would be more robust.
| next if stripped.empty? | ||
|
|
||
| unless stripped.match?(/\A\d+\z/) | ||
| warn invalid_base_port_warning(var, raw, "not a valid integer", idx) |
There was a problem hiding this comment.
Deleting a global ENV var from within PortSelector as a side-effect of querying port values is a surprising mutation. The ! suffix and surrounding comment explain the coupling to ServerManager's duplicate-warning suppression. One consequence: if select_ports is ever called a second time in the same process (tests guard against this with around blocks), the second call sees a different environment. Consider documenting this at the class level so future contributors know select_ports is not idempotent when invalid env vars are present.
| warn "WARNING: Overriding #{var_name}=#{existing.inspect} with #{derived_port} " \ | ||
| "because base port mode is active." | ||
| end | ||
|
|
There was a problem hiding this comment.
The regex here is complex and well-commented. One small improvement for future readability: extract it to a private constant (e.g. URL_WITH_EXPLICIT_PORT_RE) so the return true unless url.match?(...) guard line reads clearly at a glance without needing to parse the regex inline.
Code ReviewOverviewThis PR adds What Works Well
Issues / Suggestions1.
2. Changing the specific PR exclusion to 3. The comment in 4. Minor: in base-port mode, Missing test coverage
|
| end | ||
|
|
||
| def working_directory_for_pid(pid) | ||
| path_line = `lsof -a -p #{pid} -d cwd -Fn 2>/dev/null`.lines.find { |line| line.start_with?("n") } |
There was a problem hiding this comment.
The lsof call here uses a backtick with 2>/dev/null to swallow errors, but server_manager.rb:140-143 wraps its own lsof call in Open3.capture2 with a rescue Errno::ENOENT, StandardError specifically to handle lsof not being installed.
The fallback is safe (empty string → path_line is nil → method returns nil → PID file is preserved), but the inconsistency is worth aligning. Either approach is fine, but if you want parity:
| path_line = `lsof -a -p #{pid} -d cwd -Fn 2>/dev/null`.lines.find { |line| line.start_with?("n") } | |
| def working_directory_for_pid(pid) | |
| stdout, _status = Open3.capture2("lsof", "-a", "-p", pid.to_s, "-d", "cwd", "-Fn", | |
| err: File::NULL) | |
| path_line = stdout.lines.find { |line| line.start_with?("n") } | |
| path = path_line&.delete_prefix("n")&.strip | |
| return nil if path.nil? || path.empty? | |
| path | |
| rescue Errno::ENOENT, StandardError | |
| nil | |
| end |
| # return 5xx responses to lychee in CI, even when the pages are live. | ||
| '^https://github\.com/shakacode/react_on_rails/compare/', | ||
| '^https://github\.com/shakacode/react_on_rails/pull/2280$', # Intermittent 502 from GitHub PR page in CI | ||
| '^https://github\.com/shakacode/react_on_rails/pull/[0-9]+$', # Intermittent 5xx from GitHub PR pages in CI |
There was a problem hiding this comment.
This wildcard means no PR link anywhere in the repo (CHANGELOG entries, docs, etc.) will ever be checked for validity — a rotted pull/1234 reference in CHANGELOG.md or docs would go undetected forever. The previous line only excluded a single known-flaky PR.
The intermittent 5xx issue is real, but consider whether --max-retries / --retry-wait-time in lychee config would reduce false positives without permanently blinding the checker to dead links. If the blanket exclusion is the right call, at minimum add a comment noting the trade-off so future maintainers don't narrow it back without understanding why it was widened.
| # a subsequent `bin/dev` in the same shell sees the base-port-derived | ||
| # value rather than a stale explicit one, and developers inspecting | ||
| # their env after `bin/dev prod` see the full derived block. | ||
| def apply_base_port_env(selected) |
There was a problem hiding this comment.
The justification ("tooling consistency" for a subsequent bin/dev in the same shell) is reasonable. Worth making the trade-off explicit one more sentence: any script or hook that reads SHAKAPACKER_DEV_SERVER_PORT to decide whether a webpack-dev-server is running will be misled after bin/dev prod in a base-port workspace (the var is set but no server is listening). If that becomes a problem, the mitigation would be clearing SHAKAPACKER_DEV_SERVER_PORT at the end of production-like mode rather than leaving it set — but that's a future concern, not a blocker.
| base = PortSelector.base_port_hash | ||
| return [3000, 3001] unless base | ||
|
|
||
| [base[:rails], base[:webpack], base[:renderer]] |
There was a problem hiding this comment.
The return arity changes silently between modes (3 ports vs 2), and the renderer port being in the kill list is easy to miss. A brief comment would help:
| [base[:rails], base[:webpack], base[:renderer]] | |
| # Include all three derived ports so `bin/dev kill` in a Pro worktree | |
| # also stops the node renderer process, not just Rails and webpack. | |
| [base[:rails], base[:webpack], base[:renderer]] |
1 participant
Summary
REACT_ON_RAILS_BASE_PORTenv var that derives all service ports from a single base value (Rails = base+0, webpack = base+1, renderer = base+2)CONDUCTOR_PORTas a fallback, so Conductor.build works out of the box${PORT:-3000}and${RENDERER_PORT:-3800}patterns instead of hardcoded valuesRENDERER_PORTandREACT_RENDERER_URLenv vars for automatic Pro Node Renderer configurationThis 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: setsRENDERER_PORTandREACT_RENDERER_URLwhen base port is activeProcfile templates (all now use env var fallbacks):
Procfile.dev,Procfile.dev-prod-assets,Procfile.dev-static-assetsProcfile.dev,Procfile.prod,Procfile.staticpro_setup.rb): uses${RENDERER_PORT:-3800}in generated Procfile linesDocumentation:
.env.example: documents base port,RENDERER_PORT, andREACT_RENDERER_URLprocess-managers.md: new "Coding Agent / CI Integration" and "Manual Worktree Port Setup (Pro)" sectionsTest plan
REACT_ON_RAILS_BASE_PORT=4000 bin/devprints correct port assignments🤖 Generated with Claude Code
Note
Medium Risk
Medium risk because it changes
bin/devport selection/env mutation behavior (including overrides and kill targeting), which could affect local/dev workflows and Pro renderer connectivity if env vars are mis-set.Overview
Adds a new base-port mode for
bin/devviaREACT_ON_RAILS_BASE_PORT(withCONDUCTOR_PORTfallback) that deterministically derives Rails/webpack/Pro renderer ports (+0/+1/+2) and propagates them throughPORT,SHAKAPACKER_DEV_SERVER_PORT,RENDERER_PORT, andREACT_RENDERER_URL(overriding conflicting pre-sets with warnings).Hardens dev startup/kill behavior by cleaning stale copied Overmind sockets and
tmp/pids/server.pidfrom other app directories, expandingbin/dev killto target base-port-derived ports, and validating/scrubbing invalid port env vars to avoid silent failures.Updates Procfile templates/generator output to use shell defaults (
${PORT:-…},${RENDERER_PORT:-…}), standardizes docs/examples onREACT_RENDERER_URL, and adds/updates specs and documentation for the new port/env behavior.Reviewed by Cursor Bugbot for commit f949a69. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests
Chores