Skip to content

allow opening browser on windows through wsl - #43

Merged
The64thRealm merged 1 commit into
mainfrom
dev-command-continues-even-if-browser-fails-to-open
Jul 16, 2026
Merged

allow opening browser on windows through wsl#43
The64thRealm merged 1 commit into
mainfrom
dev-command-continues-even-if-browser-fails-to-open

Conversation

@The64thRealm

Copy link
Copy Markdown
Contributor

No description provided.

@cursor

cursor Bot commented Jul 16, 2026

Copy link
Copy Markdown

Bugbot needs on-demand usage enabled

Bugbot uses usage-based billing for this team and requires on-demand usage to be enabled.

A team admin can enable on-demand usage in the Cursor dashboard.

@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Claude finished @The64thRealm's task in 1m 41s —— View job


Code Review

Nice, focused change. The WSL problem is real and well-diagnosed, and the dev command is now resilient to a browser that won't open (matching the PR intent). Both call sites (auth.rs, dev/mod.rs) are updated consistently, server::run still executes regardless of whether the browser opens, and the PowerShell single-quote escaping is handled correctly. The module doc comment explaining why the open crate falls short under WSL is excellent.

A few observations, none of them blockers:

1. Child process stdio is inherited — may print noise to the terminal (src/browser.rs:50-52)
run_ok uses cmd.status(), which lets the child inherit the CLI's stdout/stderr. If powershell.exe (or wslview) emits a warning/error, it lands in the user's terminal uninvited. Since you only care about the exit code, capturing output keeps things clean:

fn run_ok(cmd: &mut Command) -> bool {
    cmd.output().map(|o| o.status.success()).unwrap_or(false)
}

Fix this →

2. .status()/.output() blocks until the child exits (src/browser.rs:50)
wslview and Start-Process normally fork-and-return immediately, so this is fine in practice. Just flagging that if a launcher were to block (e.g., waits on the spawned browser), the CLI would hang. Low risk given the specific commands used.

3. .gitattributes is unrelated scope creep
The line-ending normalization is reasonable on its own, but it's orthogonal to "open browser on WSL." Two thoughts: (a) it may be cleaner as its own PR/commit; (b) * text=auto eol=lf can produce a large one-time renormalization diff on existing checkouts — consider a follow-up git add --renormalize . commit so the normalization is explicit rather than trickling in file-by-file.

4. Minor: is_wsl() "wsl" check is redundant (src/browser.rs:22)
WSL2 kernel strings already contain microsoft (e.g. ...-microsoft-standard-WSL2), so the extra s.contains("wsl") is harmless but never the deciding factor. Fine to leave.

Correctness check I couldn't fully verify locally: the dev command opens http://localhost:{port}. Under WSL2 localhost is forwarded between Windows and the WSL VM, so opening it from the Windows-side browser should reach the WSL-hosted server — worth a quick manual confirmation on a real WSL host if you haven't already.

Nit: std::io::Error::other requires Rust ≥ 1.74 — fine for any recent toolchain, just make sure your MSRV (if you track one) allows it.

Overall this is a solid, well-documented improvement. Only #1 is worth addressing before merge, and even that is cosmetic.

Note: I reviewed by reading the diff and surrounding context; I did not build/run since this environment doesn't have Doppler configured (doppler run -- cargo ... per CLAUDE.md).

@The64thRealm
The64thRealm merged commit 0da6423 into main Jul 16, 2026
14 of 15 checks passed
@The64thRealm
The64thRealm deleted the dev-command-continues-even-if-browser-fails-to-open branch July 16, 2026 18:35

Copy link
Copy Markdown
Member

looks good! go ahead and merge and I'll cut a release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants