fix: use env-var-driven ports in Procfile templates to support multiple worktrees#2539
fix: use env-var-driven ports in Procfile templates to support multiple worktrees#2539
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 automatic dev port selection: new ReactOnRails::Dev::PortSelector probes and returns Rails/webpack port pairs (honoring ENV overrides), ServerManager applies them to ENV before starting, Procfile templates and generator use env-driven ports with a new Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (bin/dev)
participant SM as ServerManager
participant PS as PortSelector
participant Rails as Rails server
participant Webpack as webpack-dev-server
Dev->>SM: start dev
SM->>PS: select_ports()
PS-->>SM: { rails_port, webpack_port }
SM->>SM: set ENV["PORT"], ENV["SHAKAPACKER_DEV_SERVER_PORT"]
SM->>Rails: launch (reads ENV["PORT"])
SM->>Webpack: launch (reads ENV["SHAKAPACKER_DEV_SERVER_PORT"])
Rails-->>Dev: server bound
Webpack-->>Dev: dev-server bound
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 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}
react_on_rails/spec/react_on_rails/support/shared_examples/base_generator_examples.rb
Show resolved
Hide resolved
react_on_rails/lib/generators/react_on_rails/templates/base/base/.env.example
Show resolved
Hide resolved
![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: 60573e791f
ℹ️ 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".
react_on_rails/lib/generators/react_on_rails/templates/base/base/Procfile.dev
Show resolved
Hide resolved
Greptile SummaryThis PR replaces hardcoded port numbers in all Procfile templates with POSIX
Confidence Score: 4/5
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant Env as .env file
participant FM as Foreman / Overmind
participant Rails as Rails server
participant WDS as shakapacker-dev-server
Dev->>Env: Copy .env.example → .env<br/>(set PORT=3001, SHAKAPACKER_DEV_SERVER_PORT=3036)
Dev->>FM: bin/dev (reads Procfile.dev)
FM->>Env: Auto-load .env variables
FM->>Rails: bundle exec rails s -p ${PORT:-3000}<br/>resolves to 3001
FM->>WDS: bin/shakapacker-dev-server<br/>picks up SHAKAPACKER_DEV_SERVER_PORT=3036
Rails-->>Dev: Listening on :3001
WDS-->>Dev: Listening on :3036
Last reviewed commit: 60573e7 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
react_on_rails/spec/react_on_rails/support/shared_examples/base_generator_examples.rb
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
react_on_rails/spec/react_on_rails/support/shared_examples/base_generator_examples.rb (1)
26-28: Consider adding test coverage forProcfile.dev-prod-assets.
Procfile.dev-prod-assetsuses${PORT:-3001}(a different default), but there's no corresponding test assertion. For completeness, you could add:it "uses env-var-driven port in Procfile.dev-prod-assets" do assert_file "Procfile.dev-prod-assets", /\$\{PORT:-3001\}/ end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/spec/react_on_rails/support/shared_examples/base_generator_examples.rb` around lines 26 - 28, Add a parallel test to the existing spec that asserts Procfile.dev-prod-assets uses the env-var-driven port default ${PORT:-3001}; specifically, add an example similar to the existing it "uses env-var-driven port in Procfile.dev-static-assets" that calls assert_file "Procfile.dev-prod-assets", /\$\{PORT:-3001\}/ so the generator spec covers both Procfile.dev-static-assets and Procfile.dev-prod-assets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@react_on_rails/spec/react_on_rails/support/shared_examples/base_generator_examples.rb`:
- Around line 26-28: Add a parallel test to the existing spec that asserts
Procfile.dev-prod-assets uses the env-var-driven port default ${PORT:-3001};
specifically, add an example similar to the existing it "uses env-var-driven
port in Procfile.dev-static-assets" that calls assert_file
"Procfile.dev-prod-assets", /\$\{PORT:-3001\}/ so the generator spec covers both
Procfile.dev-static-assets and Procfile.dev-prod-assets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af442d57-4e15-4abf-9f68-5e7f785fea48
📒 Files selected for processing (7)
docs/building-features/process-managers.mdreact_on_rails/lib/generators/react_on_rails/base_generator.rbreact_on_rails/lib/generators/react_on_rails/templates/base/base/.env.examplereact_on_rails/lib/generators/react_on_rails/templates/base/base/Procfile.devreact_on_rails/lib/generators/react_on_rails/templates/base/base/Procfile.dev-prod-assetsreact_on_rails/lib/generators/react_on_rails/templates/base/base/Procfile.dev-static-assetsreact_on_rails/spec/react_on_rails/support/shared_examples/base_generator_examples.rb
Uses TCPServer.new to probe for a free Rails+webpack port pair starting at 3000/3035. Respects existing PORT/SHAKAPACKER_DEV_SERVER_PORT env vars and skips detection when either is already set. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Calls PortSelector.select_ports before spawning foreman/overmind in both run_development and run_static_development, setting ENV['PORT'] and ENV['SHAKAPACKER_DEV_SERVER_PORT'] so the process manager passes them to all child processes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review SummaryThe approach is sound — using Bugs1. URL is displayed before ports are configured (affects UX correctness) In both 2.
Code Quality3. 4. Design / Documentation5.
6. Documentation overstates auto-detection ( The docs say |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
react_on_rails/lib/react_on_rails/dev/server_manager.rb (1)
616-618:⚠️ Potential issue | 🟡 MinorConfigure ports before printing the access URL.
Line 617 and Line 650 print the URL before Line 643 and Line 658 select/update
ENV["PORT"], so the banner can show a stale/default port when auto-shifting occurs.🧭 Suggested ordering fix
def run_static_development(procfile, verbose: false, route: nil, skip_database_check: false) - print_procfile_info(procfile, route: route) - # Check database setup before starting exit 1 unless DatabaseChecker.check_database(skip: skip_database_check) # Check required services before starting exit 1 unless ServiceChecker.check_services + + configure_ports + print_procfile_info(procfile, route: route) @@ - configure_ports PackGenerator.generate(verbose: verbose)def run_development(procfile, verbose: false, route: nil, skip_database_check: false) - print_procfile_info(procfile, route: route) - # Check database setup before starting exit 1 unless DatabaseChecker.check_database(skip: skip_database_check) # Check required services before starting exit 1 unless ServiceChecker.check_services + + configure_ports + print_procfile_info(procfile, route: route) - configure_ports PackGenerator.generate(verbose: verbose)Also applies to: 643-643, 649-651, 658-658
🤖 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 616 - 618, The banner URL is printed by print_procfile_info before the code selects/updates ENV["PORT"], so it can show a stale port; in run_static_development (and the other affected call sites) move the port-selection/configuration logic (the code that selects/updates ENV["PORT"]) to run before print_procfile_info so the printed access URL uses the updated port value; update all analogous places (the other calls that print the URL at lines noted) to ensure ENV["PORT"] is configured prior to calling print_procfile_info.
🧹 Nitpick comments (1)
react_on_rails/spec/react_on_rails/dev/server_manager_spec.rb (1)
101-104: Restore prior ENV values instead of always deleting.Lines 101-104 can mutate global test process state when
PORT/SHAKAPACKER_DEV_SERVER_PORTwere already set before this context.♻️ Suggested test isolation improvement
- after do - ENV.delete("PORT") - ENV.delete("SHAKAPACKER_DEV_SERVER_PORT") - end + around do |example| + old_port = ENV.fetch("PORT", nil) + old_webpack_port = ENV.fetch("SHAKAPACKER_DEV_SERVER_PORT", nil) + example.run + ENV["PORT"] = old_port + ENV["SHAKAPACKER_DEV_SERVER_PORT"] = old_webpack_port + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/spec/react_on_rails/dev/server_manager_spec.rb` around lines 101 - 104, The teardown currently deletes ENV["PORT"] and ENV["SHAKAPACKER_DEV_SERVER_PORT"], which can wipe pre-existing environment values; instead capture and store the original values at the start of the example/context (e.g. prev_port and prev_shakapacker_port in the surrounding before/let) and in the after hook restore them (ENV["PORT"] = prev_port if prev_port; ENV.delete("PORT") if prev_port.nil?, same for "SHAKAPACKER_DEV_SERVER_PORT") so the after hook restores prior ENV state rather than always deleting; update the spec's after block that references ENV.delete("PORT") / ENV.delete("SHAKAPACKER_DEV_SERVER_PORT") accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/building-features/process-managers.md`:
- Around line 343-345: The fenced code block containing the text "Default ports
in use. Using Rails :3001, webpack :3036" lacks a language tag and triggers
MD040; update that fenced block by adding a language identifier (e.g., "text")
after the opening triple backticks so the block becomes ```text ... ```,
ensuring the linter passes while preserving the existing content.
In `@react_on_rails/lib/react_on_rails/dev/server_manager.rb`:
- Around line 692-696: The configure_ports method currently lets
PortSelector.select_ports propagate NoPortAvailable; catch NoPortAvailable
inside configure_ports, emit a clear user-facing error (e.g., using STDERR.puts
or the existing logger/say helper) that explains no free ports were found and
lists the attempted ports if available, then terminate with a non-zero exit
(exit 1) so the CLI exits cleanly; keep the normal behavior (setting ENV["PORT"]
and ENV["SHAKAPACKER_DEV_SERVER_PORT"]) when select_ports succeeds and reference
PortSelector.select_ports, configure_ports, and the NoPortAvailable exception in
your change.
---
Outside diff comments:
In `@react_on_rails/lib/react_on_rails/dev/server_manager.rb`:
- Around line 616-618: The banner URL is printed by print_procfile_info before
the code selects/updates ENV["PORT"], so it can show a stale port; in
run_static_development (and the other affected call sites) move the
port-selection/configuration logic (the code that selects/updates ENV["PORT"])
to run before print_procfile_info so the printed access URL uses the updated
port value; update all analogous places (the other calls that print the URL at
lines noted) to ensure ENV["PORT"] is configured prior to calling
print_procfile_info.
---
Nitpick comments:
In `@react_on_rails/spec/react_on_rails/dev/server_manager_spec.rb`:
- Around line 101-104: The teardown currently deletes ENV["PORT"] and
ENV["SHAKAPACKER_DEV_SERVER_PORT"], which can wipe pre-existing environment
values; instead capture and store the original values at the start of the
example/context (e.g. prev_port and prev_shakapacker_port in the surrounding
before/let) and in the after hook restore them (ENV["PORT"] = prev_port if
prev_port; ENV.delete("PORT") if prev_port.nil?, same for
"SHAKAPACKER_DEV_SERVER_PORT") so the after hook restores prior ENV state rather
than always deleting; update the spec's after block that references
ENV.delete("PORT") / ENV.delete("SHAKAPACKER_DEV_SERVER_PORT") accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f2c26a46-af15-4e4d-a80c-f45c0a76e014
📒 Files selected for processing (6)
docs/building-features/process-managers.mdreact_on_rails/lib/react_on_rails/dev.rbreact_on_rails/lib/react_on_rails/dev/port_selector.rbreact_on_rails/lib/react_on_rails/dev/server_manager.rbreact_on_rails/spec/react_on_rails/dev/port_selector_spec.rbreact_on_rails/spec/react_on_rails/dev/server_manager_spec.rb
- Assert webpack defaults to 3035 when only PORT is set - Assert rails defaults to 3000 when only SHAKAPACKER_DEV_SERVER_PORT is set - Assert no port probing in both single-env-var cases - Add context for both env vars set simultaneously - Cover procfile_port dynamic PORT reading and all four combinations Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR Review: env-var-driven ports for worktree supportOverall this is a well-thought-out feature with good test coverage. The approach of using env-var-driven Procfile defaults and delegating port discovery to Critical: The generator copies Significant:
Significant:
Minor: redundant Minor: TOCTOU race in |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/spec/react_on_rails/dev/server_manager_spec.rb`:
- Around line 306-307: The tests for .procfile_port leak ENV["PORT"] because
only an after hook deletes it; add a before hook (or an around that
saves/restores ENV["PORT"]) to ensure PORT is cleared before each example so
examples that assume "PORT is unset" aren't order-dependent—specifically update
the hooks around the .procfile_port examples (replace or augment the existing
after { ENV.delete("PORT") } with before { ENV.delete("PORT") } or an around
that saves original_port = ENV["PORT"]; ENV.delete("PORT"); example.run;
ENV["PORT"] = original_port) to guarantee a clean environment for each test.
- Around line 101-104: The test currently only deletes ENV["PORT"] and
ENV["SHAKAPACKER_DEV_SERVER_PORT"] in the after do block, which allows leaked
env vars to affect the example; add a before do that clears ENV["PORT"] and
ENV["SHAKAPACKER_DEV_SERVER_PORT"] as well so each example starts with a clean
environment. Update the same context/spec where the existing after do is defined
(referencing the after do that calls ENV.delete for those keys) to include a
matching before do that calls ENV.delete for the same keys before running the
example.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c2a67669-5fe2-4d21-80ac-be208206b89f
📒 Files selected for processing (2)
react_on_rails/spec/react_on_rails/dev/port_selector_spec.rbreact_on_rails/spec/react_on_rails/dev/server_manager_spec.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- react_on_rails/spec/react_on_rails/dev/port_selector_spec.rb
…port Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The example snippet showed wp-client/wp-server/SERVER_BUNDLE_ONLY=true but the generated template uses dev-server/server-bundle/SERVER_BUNDLE_ONLY=yes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Foreman injects PORT=5000 into all subprocesses when run directly,
causing \${PORT:-3000} to evaluate to 5000 instead of the fallback.
Document the workaround (set PORT explicitly) and note that bin/dev
avoids this entirely by setting PORT before foreman starts.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…anner print_procfile_info reads ENV["PORT"] to build the URL. With the old ordering it was called before configure_ports ran, so the banner always showed the fallback default (3000) even when auto-detection shifted to 3001. Move configure_ports above print_procfile_info in both run_development and run_static_development. Add regression tests that capture ENV["PORT"] at banner-print time and assert it equals the auto-detected value. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… method call WEBPACK_OFFSET was defined but never referenced — the offset between rails and webpack ports is maintained implicitly by the two DEFAULT_* constants in find_free_pair. Also replace the redundant explicit_webpack_port call on the rails-only branch with the already-assigned webpack_port variable, which is guaranteed nil at that point. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ean exit Previously a NoPortAvailable exception would propagate uncaught and produce a Ruby backtrace. Now configure_ports rescues it, prints the message to stderr, and exits 1 with a user-facing explanation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nfig context ENV.delete would wipe pre-existing values if PORT or SHAKAPACKER_DEV_SERVER_PORT were set before the test context ran. Use around with save/restore instead, matching the pattern already used in port_selector_spec.rb. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The probe-then-use pattern has an inherent race: another process can claim the port between server.close and the caller binding to it. Adding a comment so future readers understand it is a known limitation, not a bug to be fixed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ally committed The generator copies .env.example and users are expected to copy it to .env to set custom ports. Without .env in .gitignore a simple git add . would commit port config (or any secrets the developer adds later). Uses a line-anchored regex to avoid matching .env.example or .env.production. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dependent failures The procfile_port context only had an after hook, so a leaked PORT from a prior test could cause seed-dependent failures on examples that assume PORT is unset. The configuring-ports context had an around that saved and restored but did not clear before running, so leaked env vars could still pollute the example. Both contexts now use save -> clear -> run -> restore. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 48-54: The current explicit_rails_port and explicit_webpack_port
helpers only check for positive integers, letting values > 65,535 slip through;
update both methods to validate the TCP port range by accepting the parsed
integer only if it falls within 1..65_535 (e.g., replace the positive? check
with a range/between? check) so they return the port when valid and nil
otherwise.
- Around line 22-29: The current logic in port selection returns a fixed default
for the unset side which can still collide or produce identical rails/webpack
ports; change the single-ENV branches in the port selection logic so that when
only rails_port is set you anchor rails_port and call a new helper (e.g.,
find_available_port) to probe from DEFAULT_WEBPACK_PORT upward (skipping the
anchored rails_port) and vice versa when only webpack_port is set probe for
rails starting at DEFAULT_RAILS_PORT (skipping the anchored webpack_port);
implement find_available_port(start_port, exclude:) that uses port_available?
and raises NoPortAvailable if it cannot find a free port within a reasonable
range so the returned hash { rails: ..., webpack: ... } never collides or
returns identical ports.
In `@react_on_rails/lib/react_on_rails/dev/server_manager.rb`:
- Around line 623-625: The startup prints a stale URL because print_server_info
still falls back to 3000 instead of using the port chosen by configure_ports;
update the call site to pass the resolved port into print_server_info (or modify
print_server_info signature to read ENV["PORT"] after configure_ports) so both
print_procfile_info and print_server_info use the same port—refer to
configure_ports, print_procfile_info, and print_server_info and change the call
to pass the selected port (e.g., port: ENV["PORT"] or the local port variable)
so the banner URLs match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c563bcc2-aa4b-4679-b025-f05ed814c083
📒 Files selected for processing (7)
docs/building-features/process-managers.mdreact_on_rails/lib/generators/react_on_rails/base_generator.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/react_on_rails/dev/server_manager_spec.rbreact_on_rails/spec/react_on_rails/support/shared_examples/base_generator_examples.rb
✅ Files skipped from review due to trivial changes (1)
- react_on_rails/lib/generators/react_on_rails/templates/base/base/.env.example
🚧 Files skipped from review as they are similar to previous changes (4)
- react_on_rails/spec/react_on_rails/dev/server_manager_spec.rb
- react_on_rails/spec/react_on_rails/support/shared_examples/base_generator_examples.rb
- docs/building-features/process-managers.md
- react_on_rails/lib/generators/react_on_rails/base_generator.rb
The single-ENV path returned a fixed default for the unset side without
checking availability, causing two problems:
- PORT=3035 produced {rails: 3035, webpack: 3035} — identical ports,
both processes fail to bind
- PORT=<any> with the default webpack port occupied returned an
unavailable port without error
Add find_available_port(start_port, exclude:) and use it for both
single-ENV branches so the unset side is always probed and never
collides with the anchored port.
Also change positive? to between?(1, 65_535) in explicit_rails_port and
explicit_webpack_port so out-of-range values like PORT=99999 are treated
as unset and fall back to auto-detection rather than silently wrapping.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…static mode print_server_info was called without a port argument, so it always used the default 3000 even after configure_ports selected a different port. This meant run_static_development printed two contradictory URLs: print_procfile_info showed the correct port, print_server_info showed 3000. Pass procfile_port(procfile) explicitly. Add regression test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
react_on_rails/lib/react_on_rails/dev/port_selector.rb (1)
15-18: Keepselect_portsdocs aligned with actual probing behavior.The comment on Line 17 says probing happens only when both env vars are unset, but Lines 26-33 also probe when only one side is set.
✏️ Suggested doc fix
- # Only probes when both are unset (i.e. user hasn't configured them). + # Probes when one or both ports are unset (i.e. incomplete/no user configuration).🤖 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 15 - 18, The docstring for select_ports is inaccurate: it says probing occurs only when both ENV['PORT'] and ENV['SHAKAPACKER_DEV_SERVER_PORT'] are unset, but the implementation (see select_ports) also probes for a free port for whichever side is unset while respecting the other env var; update the comment to state that the method returns { rails: Integer, webpack: Integer }, respects any set ENV['PORT'] or ENV['SHAKAPACKER_DEV_SERVER_PORT'], and probes only for the unset side(s) (i.e., will probe one side if the other is configured, or both if neither is set).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@react_on_rails/lib/react_on_rails/dev/port_selector.rb`:
- Around line 15-18: The docstring for select_ports is inaccurate: it says
probing occurs only when both ENV['PORT'] and ENV['SHAKAPACKER_DEV_SERVER_PORT']
are unset, but the implementation (see select_ports) also probes for a free port
for whichever side is unset while respecting the other env var; update the
comment to state that the method returns { rails: Integer, webpack: Integer },
respects any set ENV['PORT'] or ENV['SHAKAPACKER_DEV_SERVER_PORT'], and probes
only for the unset side(s) (i.e., will probe one side if the other is
configured, or both if neither is set).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9a5aefe-ebb1-4419-bcda-bf08798377c6
📒 Files selected for processing (4)
react_on_rails/lib/react_on_rails/dev/port_selector.rbreact_on_rails/lib/react_on_rails/dev/server_manager.rbreact_on_rails/spec/react_on_rails/dev/port_selector_spec.rbreact_on_rails/spec/react_on_rails/dev/server_manager_spec.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- react_on_rails/spec/react_on_rails/dev/server_manager_spec.rb
- react_on_rails/spec/react_on_rails/dev/port_selector_spec.rb
PR Review:
|
| raise NoPortAvailable, "No available port found starting at #{start_port}." | ||
| end | ||
|
|
||
| def find_free_pair |
There was a problem hiding this comment.
Bug: lockstep pairing misses valid port combinations
Both rails and webpack offsets advance together (i is shared). If 3035 is the only busy port, the algorithm skips the perfectly valid pair 3000+3036 and jumps to 3001+3036. Consider finding a free Rails port first, then scanning independently for a free webpack port (excluding the chosen Rails port):
def find_free_pair
MAX_ATTEMPTS.times do |i|
rails_port = DEFAULT_RAILS_PORT + i
next unless port_available?(rails_port)
webpack_port = find_available_port(DEFAULT_WEBPACK_PORT, exclude: rails_port)
shifted = rails_port != DEFAULT_RAILS_PORT || webpack_port != DEFAULT_WEBPACK_PORT
puts "Default ports in use. Using Rails :#{rails_port}, webpack :#{webpack_port}" if shifted
return { rails: rails_port, webpack: webpack_port }
rescue NoPortAvailable
next
end
raise NoPortAvailable, "No available port pair found ..."
end
| class NoPortAvailable < StandardError; end | ||
|
|
||
| class << self | ||
| # Returns { rails: Integer, webpack: Integer }. |
There was a problem hiding this comment.
The comment on line 17 says "Only probes when both are unset" but the code below actually probes even when only one is set (the if rails_port / if webpack_port branches both call find_available_port). The comment is misleading — suggest updating it to accurately describe the three-branch logic.
|
|
||
| next unless port_available?(rails_port) && port_available?(webpack_port) | ||
|
|
||
| puts "Default ports in use. Using Rails :#{rails_port}, webpack :#{webpack_port}" if i.positive? |
There was a problem hiding this comment.
puts writes to stdout in library code. Stdout is often captured (e.g., by test frameworks, CI log parsers, or Foreman). Consider using warn (stderr) or accepting an optional logger/output IO as a constructor/method argument so callers can suppress or redirect the message.
| old = ENV.fetch("PORT", nil) | ||
| ENV["PORT"] = "4000" | ||
| example.run | ||
| ENV["PORT"] = old |
There was a problem hiding this comment.
Latent test flake: ENV[key] = nil raises TypeError
When PORT is not set before the test suite runs, old is nil, and ENV["PORT"] = old at restore time raises TypeError: no implicit conversion of nil into String.
This only triggers when PORT is already set in the environment (e.g., CI injecting it), making it hard to reproduce locally. Fix:
| ENV["PORT"] = old | |
| old.nil? ? ENV.delete("PORT") : ENV["PORT"] = old |
| allow(ReactOnRails::Dev::PortSelector).to receive(:select_ports) | ||
| .and_raise(ReactOnRails::Dev::PortSelector::NoPortAvailable, "No available port pair found") | ||
|
|
||
| expect_any_instance_of(Kernel).to receive(:exit).with(1) |
There was a problem hiding this comment.
Fragile test pattern for exit
expect_any_instance_of(Kernel).to receive(:exit).with(1) is unreliable. In Ruby, calling exit(1) raises a SystemExit exception — RSpec's any_instance interceptor may or may not fire depending on how the method lookup resolves in the call chain.
The idiomatic RSpec pattern is:
| expect_any_instance_of(Kernel).to receive(:exit).with(1) | |
| expect { described_class.start(:development) } | |
| .to raise_error(SystemExit) { |e| expect(e.status).to eq(1) } |
| exit 1 | ||
| end | ||
|
|
||
| def procfile_port(procfile) |
There was a problem hiding this comment.
Inconsistent default for Procfile.dev-prod-assets
configure_ports always scans starting from DEFAULT_RAILS_PORT = 3000, so when no ports are occupied it sets ENV["PORT"] = "3000". That overrides the Procfile's own ${PORT:-3001} fallback, silently changing the traditional default for prod-assets from 3001 to 3000.
Consider either: (a) having PortSelector accept a default_rails_port argument that callers can set to 3001 for prod-assets, or (b) keeping procfile_port aware that prod-assets defaults to 3001 before ENV["PORT"] is set.
1 participant
Summary
${PORT:-N}instead of hardcoded ports, so developers can runbin/devin multiple git worktrees simultaneously without port conflicts.env.exampletemplate (generated byrails g react_on_rails:install) documentingPORTandSHAKAPACKER_DEV_SERVER_PORTfor per-worktree port configuration.env.exampleinto the base generator so it's copied during install${PORT:-3000}content and.env.examplepresenceHow it works
Foreman and Overmind both read
.envautomatically. Shakapacker already supportsSHAKAPACKER_DEV_SERVER_PORTas an env var override on both the Ruby proxy side (Shakapacker::DevServer#port) and the JS webpack-dev-server config side. Noshakapacker.ymlchanges are needed in the generated app.Each worktree gets its own gitignored
.env:# worktree 2 .env PORT=3001 SHAKAPACKER_DEV_SERVER_PORT=3036Test Plan
base_generator_commonexamples assert${PORT:-3000}and.env.example)Procfile.devcontains${PORT:-3000}instead of hardcoded3000.env.exampleis present with port documentationbin/devin two worktrees with different.envfiles starts without port conflictsSummary by CodeRabbit
New Features
Documentation
Tests