Walkthrough

Updates server webpack configuration to use source maps instead of eval-based devtools, refactors browser-opening logic to distinguish explicit versus automatic opens with WSL detection, improves marker file handling, and updates related tests and documentation strings.

Changes

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~28 minutes

Poem

🐰 Hop along, dear browsers bright,
No more eval's twisted plight!
WSL knows where you dwell,
Explicit opens break the spell—
Source maps guide our errors true,
The rail-stack hops ahead for you! 🚂✨

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 @coderabbitai help to get the list of available commands and usage tips.}

Overall this is a well-structured cleanup. The motivations are sound (lazy marker resolution, narrowed exception handling, WSL support, explicit-vs-auto TTY distinction). A few items worth addressing before merge.

Greptile Summary

This PR addresses deferred review items from #2849 by cleaning up the browser-open thread in server_manager.rb (removing non-idiomatic next/report_on_exception = false, adding WSL launcher detection, lazily resolving the once-marker path, narrowing socket exception rescues, and letting explicit --open-browser bypass TTY/CI gating) and fixing a misleading generator note that implied Pro works without a license.

Confidence Score: 5/5

Safe to merge — all findings are P2 style suggestions with no correctness impact.

All three changed files are logically correct. The thread refactor, WSL support, lazy marker path, and narrowed exception handling are all well-reasoned and pass the 93-example spec suite. The only gaps are a missing spec for the --open-browser explicit-bypass path and a subtle API-level ambiguity when both flags are true simultaneously — neither affects runtime correctness given CLI mutual exclusivity.

No files require special attention.

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[schedule_browser_open_if_requested] --> B{open_browser or open_browser_once?}
    B -- No --> Z[return]
    B -- Yes --> C["explicit = open_browser && !open_browser_once"]
    C --> D["schedule_browser_open(port, route, once, explicit)"]
    D --> E{"browser_auto_open_allowed?(explicit)"}
    E -- "explicit=true" --> G[Skip CI/TTY check → allowed]
    E -- "explicit=false" --> F{"!CI && stdin.tty? && stdout.tty?"}
    F -- No --> Z
    F -- Yes --> G
    G --> H[Thread.new]
    H --> I{wait_for_app_route?}
    I -- No/timeout --> Z
    I -- Yes --> J[prepare_browser_open_once_marker]
    J -- :already_opened --> Z
    J -- :claimed/:not_requested --> K[open_browser url]
    K -- success --> Z
    K -- fail --> L[clear marker if claimed]
    L --> M[warn user with URL]

_{Reviews (1): Last reviewed commit: "fix: address deferred review items from ..." | Re-trigger Greptile}

Review

Overall this is a solid cleanup of the server manager browser-open code. The changes are well-motivated, the refactoring improves correctness (removing report_on_exception = false, lazy marker path, narrowed exception handling), and the WSL support fills a real gap. A few things to address:

Must fix

• Errno::EPIPE missing from LOCALHOST_CONNECT_ERRORS — a server that accepts the TCP connection but drops it mid-request will raise EPIPE on the write side; this is still a "not ready" condition and should be retried silently. See inline comment.

Should add

• Missing test: explicit: true path — the core new behavior (--open-browser bypassing TTY/CI gating) has no spec coverage. Neither schedule_browser_open_if_requested with open_browser: true nor browser_auto_open_allowed?(explicit: true) in a CI/non-tty context is tested. See inline comment.

• Missing WSL tests — wsl?, linux_browser_command fallback ordering, and the WSL diagnostic hint in open_browser are all new code with no specs. See inline comment.

Minor

• WSLENV as a WSL signal — WSL_DISTRO_NAME is the authoritative indicator; WSLENV is also set by Windows-side tooling and could produce a false positive in Docker images that inherit a Windows host env. Low risk, but worth a comment in wsl?. See inline comment.

Looks good

• Thread refactor (removing next, replacing report_on_exception = false with a rescue that surfaces a warning) is strictly better.
• open_browser_once_marker as a method instead of a constant correctly fixes the load-time path resolution bug.
• devtool change to cheap-module-source-map is the right fix for the webpack 5.106+ ESM regression and the comment explains the reason clearly.
• Generator copy fix is accurate.

size-limit report 📦

Overall this is a solid set of fixes. The webpack 5.106+ regression fix, WSL browser support, narrowed exception handling, and generator copy update are all well-reasoned. See inline comments for specifics.

Review feedback addressed in 1f04d40

Fixed (5 items):

1. Added missing exceptions to LOCALHOST_CONNECT_ERRORS — EOFError, Errno::EPIPE, and Errno::EAFNOSUPPORT are now included. These transient boot-time errors were previously caught by the broad rescue StandardError; the narrowed list omitted them, which could crash the polling thread and permanently prevent browser auto-open.

2. Fixed double warning when no browser launcher found — Removed the new warning from open_browser and consolidated into the caller's single message, which now includes both the URL and the WSL-specific hint.

3. Added spec coverage for explicit: true path — Tests verify that --open-browser propagates explicit: true through schedule_browser_open_if_requested, and that browser_auto_open_allowed?(explicit: true) returns true even when CI is set.

4. Added WSL detection specs — Coverage for wsl? (WSL_DISTRO_NAME, WSLENV, neither) and linux_browser_command fallback ordering (wslview → wsl-open → xdg-open → nil).

5. Added comment clarifying WSLENV — Documented that WSL_DISTRO_NAME is the authoritative WSL indicator; WSLENV is a weaker fallback.

Declined with rationale (4 items):

• Both-flags-true guard (2 comments) — CLI enforces mutual exclusivity
• Errno::EADDRNOTAVAIL removal — keeping it is defensive and harmless
• Dir.pwd mutability concern — extremely unlikely in dev-server context

All 12 review threads resolved. 104 specs passing, RuboCop clean.

Review: fix: address deferred review items from PR #2849

Overall this is a solid, well-reasoned set of fixes. The refactors are well-motivated and the test additions are thorough. A few targeted notes below.

What works well

• report_on_exception removal + rescue StandardError: Correct trade. The old report_on_exception = false suppressed all crashes; the new rescue surfaces them as warn messages without crashing the parent process. The Ruby-idiomatic if/else replaces next cleanly.
• Lazy open_browser_once_marker: Good fix for the load-time vs. run-time Dir.pwd race. The allow(described_class).to receive(:open_browser_once_marker) stub in specs is cleaner than stub_const.
• LOCALHOST_CONNECT_ERRORS constant: Narrowing from StandardError to a named list is the right call. The inclusion of Errno::EAFNOSUPPORT, Errno::EPIPE, and EOFError covers the realistic startup-polling failure modes.
• scriptSanitizedVal named function: Adding a function.name assertion is a good regression guard for the webpack 5.106+ compatibility fix.
• cheap-module-source-map over eval: Correct response to the webpack regression. The comment in the template/dummy configs is informative.

Issues (inline comments posted)

1. explicit flag when both --open-browser and --open-browser-once are passed (server_manager.rb:864): explicit = open_browser && !open_browser_once → false, so the TTY guard applies and the marker is written. Behaviour is defensible but undocumented and untested.

2. Misleading WSL install hint via WSLENV false positive (server_manager.rb:898): WSLENV can be present in a Docker container that inherited the Windows host env (Docker-in-WSL). If wslview/wsl-open don't exist there, the hint says "install wslu (for wslview) or wsl-open" — wrong advice. Using ENV.key?("WSL_DISTRO_NAME") (kernel-set, not inheritable) would be more precise.

3. No install hint for non-WSL Linux without xdg-open (server_manager.rb:989): linux_browser_command returns nil silently; the user only sees the generic "Could not open browser automatically." message with no guidance on what to install (xdg-utils).

Minor / non-blocking

• browser_auto_open_allowed? TTY test coverage: The two new specs only exercise the CI env var path. The $stdin.tty?/$stdout.tty? branches can't be easily tested in RSpec, which is fine — just worth a comment in the spec explaining why they're omitted.
• open_browser_once_marker called multiple times in a single open sequence: prepare_browser_open_once_marker and clear_browser_open_once_marker_if_claimed each call Dir.pwd independently. If something changes Dir.pwd between calls (edge case), the paths diverge. A local variable capturing the result once at the top of schedule_browser_open would make this airtight, but it's theoretical.

Review feedback addressed in 73b9415

Fixed (2 items):

1. Uninitialized saved variable in WSL detection spec — Added saved = {} initialization before ENV.to_h.slice so the ensure block always has a valid hash, even if setup raises.

2. cheap-module-source-map set unconditionally — Devtool is now conditional: false in production (no .map files), cheap-module-source-map in development (SSR error traces). Both avoid the webpack 5.106+ eval regression. Applied consistently across the template, all spec dummy configs, gen-examples, and the generator spec helper.

Declined with rationale (6 items):

3. Duplicate of TODO for first version #1 (second saved variable comment)
4. Test for open_browser: true, open_browser_once: true — flags are documented as mutually exclusive; codifying unsupported combos isn't useful
5. WSL hint uses wsl? instead of strict WSL_DISTRO_NAME — intentional design choice; false positive consequence is benign since the hint lists all relevant tools
6. Dir.chdir between marker ops — extremely theoretical; marker mechanism is best-effort by design
7. Test fails on Linux CI (cursor[bot]) — incorrect finding: test already uses regex matcher with .* wildcard
8. Duplicate of Add Unit tests #5

All 8 review threads resolved.

Review posted via inline comments. See discussion threads on server_manager.rb lines 864 and 1004, and configuration.rb lines 277 and 279 for specific observations. Overall the PR is well-structured with no blocking issues.

Code Review

Overall this is a clean, well-structured PR. The changes are well-scoped, the test coverage is solid, and the reasoning in the PR description and inline comments is clear. A few observations worth addressing:

Substantive items

explicit flag when both --open-browser and --open-browser-once are set

In schedule_browser_open_if_requested, explicit = open_browser && !open_browser_once, so passing both flags at once produces explicit: false — the TTY/CI guard is respected even though the user explicitly passed --open-browser. The behavior is arguably correct (once-semantics wins), but it silently overrides an explicit user request without any warning or documentation. If both can be set from bin/dev, a small comment or guard would clarify the intent.

open_browser_once_marker called twice inside the same thread

The lazy Dir.pwd approach correctly solves the class-load-time path issue. However, prepare_browser_open_once_marker and clear_browser_open_once_marker_if_claimed each call open_browser_once_marker separately, evaluating Dir.pwd twice within the same thread execution. If something changes Dir.pwd between those two calls (unlikely in a running Rails app, but possible in tests), the delete silently no-ops via the Errno::ENOENT rescue. Consider capturing the path once at the start of the Thread.new block and threading it through.

wsl? with WSLENV false-positive

The code comment already calls this out. Worth noting: the false-positive path (Docker-on-Windows with WSLENV set) is safe at runtime because WSL launchers won't be found and the code falls back to xdg-open. No code change needed, just flagging it for awareness.

Minor items

• spec/react_on_rails/dev/server_manager_spec.rb line 326: saved = {} at class-evaluation time inside the describe block is redundant — the around block immediately re-initialises it.
• The browser_auto_open_allowed? tests cover CI gating but not $stdin.tty? / $stdout.tty? — hard to unit-test, so this is understandable.
• The webpack devtool change and the generator destination_root fix are both clean and the test snapshot update is correctly kept in sync.

Review summary

Overall this is a solid, well-motivated set of changes. The refactoring is clear, the new tests are thorough, and the webpack devtool fix addresses a real regression. A few items worth addressing:

Medium

explicit flag logic (server_manager.rb:864) — When both open_browser and open_browser_once are true, explicit becomes false and TTY/CI guards apply even though the user explicitly requested --open-browser. This is probably unreachable in practice (the flags are mutually exclusive at the CLI), but it's a silent edge case. A comment or a guard would make the intent unambiguous. See inline.

Low

WSLENV false positive → misleading error hint (server_manager.rb:1004) — The comment already acknowledges this, but the downstream consequence is concrete: a Docker container that inherits WSLENV from a Windows host will get "On WSL, install wslu…" instead of "Install xdg-utils…" when no browser launcher is found. Dropping WSLENV as a fallback, or deferring the WSL-specific hint until at least one WSL launcher was attempted, would avoid the misdirection. See inline.

saved = {} dead code in spec (server_manager_spec.rb:326) — The describe-level saved = {} is immediately overwritten inside the around block (Ruby closures mutate the outer binding). It can be removed. See inline.

File.join style nit (install_generator.rb:448) — Minor: prefer File.join(destination_root, "bin", filename) over string interpolation. See inline.

Positive notes

• Removing report_on_exception = false is a great catch — suppressing thread exceptions in development makes real bugs invisible.
• The lazy open_browser_once_marker method is the right fix for the class-load-time Dir.pwd issue.
• Narrowing http_get_localhost rescue to LOCALHOST_CONNECT_ERRORS instead of StandardError is a good hardening step.
• The webpack cheap-module-source-map change is well-justified and the test guard (scriptSanitizedVal.name) is a clever way to pin the behaviour that webpack 5.106+ broke.
• The bin_scripts_to_chmod destination_root fix is correct and the added comment explains the why clearly.