Conversation
|
Updated 4:55 PM PT - May 6th, 2026
❌ @robobun, your commit 6ed52a8 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 29106That installs a local version of the PR into your bun-29106 --bun |
|
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:
WalkthroughChrome/WebView constructor now returns specific failure reasons via a new Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29102.test.ts`:
- Around line 73-93: The child process must fail fast if new Bun.WebView({})
does not throw: update the inline script passed to Bun.spawn (the command that
runs new Bun.WebView({}) inside the -e string) so that in the try block, after
constructing the WebView, it explicitly emits a unique failure marker and exits
non‑zero (e.g., console.error or console.log a distinct token such as
"UNEXPECTED_SUCCESS" and call process.exit(1)); keep the existing catch behavior
that logs e.code and e.message so the test can assert on the expected error;
reference the Bun.spawn invocation, the inline try/catch around new
Bun.WebView({}), and proc/exited to ensure the test observes the non‑zero exit
when the WebView unexpectedly succeeds.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 676c519c-89a8-4669-9c31-2b4a42590df3
📒 Files selected for processing (3)
src/bun.js/webview/JSWebViewConstructor.cpptest/js/bun/webview/webview.test.tstest/regression/issue/29102.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29102.test.ts`:
- Around line 22-71: The tests assume new (Bun as any).WebView(...) always
throws, but if it unexpectedly succeeds the created view remains open and can
hang the suite; update each test block (the three places that call new (Bun as
any).WebView(...) — the implicit/default, backend:"chrome", and backend.path
cases) to capture the returned instance (e.g. let view: any = undefined; inside
the try set view = new (Bun as any).WebView(...)), and after the try/catch add a
short fail-fast path: if (view) { await or call view.close() (or the appropriate
shutdown method on WebView) to clean up the in-process view, then explicitly
fail the test (throw new Error or call fail("...")/expect(false).toBe(true)) so
the test reports a clear failure instead of hanging. Ensure you reference the
same symbol new (Bun as any).WebView and close/cleanup the instance before
failing.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f283e4fa-9e78-478b-a223-006595ed0a3c
📒 Files selected for processing (1)
test/regression/issue/29102.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The previous fix threw ERR_METHOD_NOT_IMPLEMENTED for ALL chrome backend
calls on Windows, but only the spawn path (Bun__Chrome__ensure -> POSIX
socketpair) is actually broken there. The WebSocket connect path
(ensureConnected -> WebCore::WebSocket) works fine on Windows, and
Bun__Chrome__autoDetect already has a Windows branch that reads
DevToolsActivePort from %LOCALAPPDATA%.
Move the guard inside createChrome so it only fires when the call
actually reaches ensureSpawned. backend: { url: "ws://..." } and
auto-detect-a-running-Chrome now work on Windows as they did before
PR #29106.
createChrome gains an outparam ChromeCreateFailure so the constructor
can throw the right error: NotImplementedOnWindows vs SpawnFailed vs
ConnectFailed — each with a message scoped to its actual cause.
Also: on Windows, the backend:"webkit" error pointed users at
"use backend: chrome", but chrome's spawn path is now also not
implemented there. Updated the Windows-specific message to point at
the ws:// connect workaround instead, so users don't hit a second
not-implemented error following the hint.
Tests: added a "backend.url:'ws://...' is NOT blocked" case to make
sure the guard doesn't regress back to blocking WebSocket. Tightened
in-process tests with fail-fast view.close() + throw on unexpected
success (no more hangs if the constructor stops throwing). Added the
missing /backend.path/ check and asserted stderr === "" in the
spawned-child test.
|
✅ No merge conflicts detected when merging into Your branch is good to go! |
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 `@test/regression/issue/29102.test.ts`:
- Around line 43-83: The tests are asserting that auto-detection paths (new (Bun
as any).WebView({}) and new (Bun as any).WebView({ backend: "chrome" })) always
throw, but those can succeed if DevToolsActivePort points to a running Chrome;
change the two affected cases to force the spawn path deterministically (e.g.,
use the spawn-forcing input shapes referenced in the review such as new (Bun as
any).WebView({ backend: { path: "nonexistent-path" } }) or new (Bun as
any).WebView({ url: false })) so the Windows spawn guard is hit reliably, keep
the same cleanup and expectNotImplementedError(err) assertions, or alternatively
run the original default-path checks in an isolated child environment where
auto-detect cannot succeed.
- Around line 204-210: Remove the brittle assertion that stderr is empty in the
test by deleting or disabling the expect(stderr).toBe("") assertion; instead,
either omit any check on stderr or relax it to ignore ASAN JSC warnings (e.g.,
do not assert on the stderr variable produced by
Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]) so the test
no longer fails when ASAN emits "WARNING: ASAN interferes with JSC signal
handlers...").
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 44172709-0f1d-4067-a9c6-b4ce7b9accb7
📒 Files selected for processing (5)
src/bun.js/webview/JSWebView.cppsrc/bun.js/webview/JSWebView.hsrc/bun.js/webview/JSWebViewConstructor.cpptest/js/bun/webview/webview.test.tstest/regression/issue/29102.test.ts
There was a problem hiding this comment.
LGTM — my two prior blocking concerns were addressed: the Windows guard is now correctly scoped to spawn-only, and the webkit fallback message no longer points Windows users at a broken Chrome spawn path.
Extended reasoning...
Overview
This PR fixes a misleading ERR_DLOPEN_FAILED error on Windows when new Bun.WebView({}) fails. The root cause was that Bun__Chrome__ensure short-circuits on Windows before BUN_CHROME_PATH or backend.path are consulted, but the old error message told users to set those knobs. The fix narrows the Windows guard to the spawn subpath only (via a trySpawn lambda with a #if OS(WINDOWS) block) while leaving the WebSocket connect path and auto-detect path functional on Windows.
Security risks
None. This is a UX/error-message fix. No auth, crypto, or permission-sensitive code is touched. The change adds a platform guard that throws ERR_METHOD_NOT_IMPLEMENTED instead of misleading users.
Level of scrutiny
Moderate. The changes touch C++ WebView constructor code and introduce a ChromeCreateFailure enum outparam. The logic is straightforward — trySpawn returns false on Windows with NotImplementedOnWindows, and the constructor switches on the failure type to emit the right error. The webkit fallback on Windows now also correctly points at ws:// instead of chrome spawn.
Other factors
My two previous red-flag inline comments were both resolved: (1) the #if OS(WINDOWS) guard was narrowed to spawn-only rather than blocking the entire Chrome backend; (2) the webkit error on Windows now gives a ws:// hint rather than 'use backend: chrome'. The test coverage is solid — expectNotImplementedError helper covers all three constructor variants with both negative assertions. The remaining bug from the current review session is a minor comment/message nit about ConnectFailed in an edge case (malformed DevToolsActivePort on auto-detect path) that does not block correctness.
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 `@test/regression/issue/29102.test.ts`:
- Line 55: The helper function runInScrubbedChild declares an unused parameter
extraEnv which should be removed to eliminate dead code; update the function
signature in runInScrubbedChild to drop the extraEnv parameter and remove/adjust
any call sites that pass an extraEnv argument (if any) so they match the new
signature, ensuring no behavior changes in tests (notably Test 6 which already
constructs its own Bun.spawn).
- Around line 5-23: Remove the large multi-line header comment that restates bug
history and fix scope (the verbose comment block immediately after the
single-line URL at the top of the test) and leave only the single-line URL
reference; keep any concise inline comments that document the test's design
rationale or asserts. Ensure the test file (the header comment in
issue/29102.test.ts) is cleaned by deleting the verbose paragraphs while
preserving any short, necessary explanatory comments that affect understanding
of the test behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e2f0398-54fd-4aa8-a85d-f7d9cb7c3666
📒 Files selected for processing (1)
test/regression/issue/29102.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/webview/JSWebView.cpp`:
- Around line 343-352: The Windows branch in trySpawn currently returns
NotImplementedOnWindows before letting Transport::ensureSpawned short-circuit an
already-open transport; change trySpawn to call t.ensureSpawned(zig,
userDataDir, path, extraArgv, stdoutInherit, stderrInherit) first on all
platforms and if that returns true return true, otherwise on Windows map the
failure to setFailure(ChromeCreateFailure::NotImplementedOnWindows) and return
false (on non-Windows keep the existing SpawnFailed mapping). In short: move the
ensureSpawned call to happen unconditionally at the top of the lambda (or at
least before the Windows guard), use its boolean result to short-circuit
success, and only translate a failed ensureSpawned into NotImplementedOnWindows
inside the Windows-specific branch via setFailure.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7e987c3a-ea29-40d4-9306-3b7490d732e3
📒 Files selected for processing (4)
src/bun.js/webview/JSWebView.cppsrc/bun.js/webview/JSWebView.hsrc/bun.js/webview/JSWebViewConstructor.cpptest/regression/issue/29102.test.ts
There was a problem hiding this comment.
LGTM — the core production changes are correct and the major issues from prior review rounds have been addressed.
Extended reasoning...
Overview
This PR fixes a misleading error message on Windows when using the Chrome backend for Bun.WebView. It introduces a ChromeCreateFailure enum with four distinct failure modes, properly scopes the Windows guard to the spawn path only (preserving ws:// connect and auto-detect flows), and adds Windows-specific tests and a regression file for issue #29102.
Security risks
None. The changes are confined to error message selection and test infrastructure. No auth, crypto, or permission code is touched.
Level of scrutiny
Low-to-medium. The production logic changes are well-contained: new enum, narrowed Windows guard in trySpawn (which correctly calls ensureSpawned first to preserve singleton transport reuse), and split error messages. The webkit Windows branch correctly redirects to ws:// instead of the now-broken chrome spawn suggestion. Test structure follows established patterns.
Other factors
The two remaining nit-level issues (old webkit regex matching Windows message incidentally; missing positive ws:// assertion in chrome spawn helpers) are latent fragilities, not current bugs. They are called out as inline comments. All prior major concerns from earlier review rounds — the transport singleton reuse regression, the too-wide Windows guard, the ConnectFailed/AutoDetectConnectFailed split, non-deterministic test cases — have been addressed.
There was a problem hiding this comment.
LGTM — all red-flag issues from prior rounds addressed (webkit Windows message, ws:// guard scope, transport singleton reuse). The two remaining yellow nits (webview.test.ts missing ws:// assertion in chrome spawn loop, old non-darwin webkit test incidentally matching Windows message) are covered adequately by the companion 29102.test.ts helpers.
Extended reasoning...
Overview
PR fixes issue #29102 where Windows users got a misleading 'set BUN_CHROME_PATH / backend.path' hint for a code path that ignores those options entirely. Changes touch JSWebView.cpp/h (ChromeCreateFailure enum + trySpawn lambda), JSWebViewConstructor.cpp (switch on failure mode, Windows webkit message), and new Windows-only tests in both webview.test.ts and test/regression/issue/29102.test.ts.
Security risks
None. This is error-message and error-code routing code; no auth, crypto, or privilege paths involved.
Level of scrutiny
Medium-low. The production logic change is small and surgical: a trySpawn lambda that calls ensureSpawned unconditionally first (preserving the singleton fast-path) and only maps failure to NotImplementedOnWindows on Windows. The broader createChrome routing logic (wsUrl → connect, path/skipAutoDetect → spawn, auto-detect) is unchanged. The test surface is Windows-only CI, so regressions on other platforms are not possible.
Other factors
All three red issues from earlier rounds are addressed in commits 3c1caf9/5cae167/b4ee549: webkit Windows message now points at ws:// instead of chrome, the guard is scoped to trySpawn not the whole Chrome backend, and transport singleton reuse is preserved. The 29102.test.ts helpers now positively assert the ws:// hint is present (b4ee549). Two yellow nits remain: (1) the pre-existing non-darwin webkit test incidentally matches the new Windows message for structural rather than semantic reasons — latent fragility but not a current failure; (2) the webview.test.ts chrome spawn loop lacks the ws:// positive assertion that the 29102.test.ts helpers now have. Both are adequately covered by the 29102.test.ts suite and do not block approval.
There was a problem hiding this comment.
LGTM — all previously raised issues have been addressed.
Extended reasoning...
Overview
This PR fixes the misleading error message on Windows when users try to use new Bun.WebView({}) with the Chrome backend. The fix adds a ChromeCreateFailure enum with four distinct failure modes (SpawnFailed, ConnectFailed, AutoDetectConnectFailed, NotImplementedOnWindows), moves the Windows guard to the spawn subpath only (not the entire Chrome backend), and updates error messages to give actionable ws:// connect workarounds instead of inert BUN_CHROME_PATH hints.
Security risks
None. This is purely error-message and error-routing logic. No auth, crypto, or permissions are touched.
Level of scrutiny
Moderate. The changes touch C++ backend dispatch, a new enum, and test infrastructure. The logic is well-scoped: the spawn-vs-connect distinction is preserved correctly (trySpawn calls ensureSpawned unconditionally so the singleton reuse fast-path works on Windows), the webkit fallback message is correctly platform-split (#if OS(WINDOWS) vs #elif \!OS(DARWIN)), and ConnectFailed vs AutoDetectConnectFailed is properly distinguished so error messages never hint at backend.url when the user never set it.
Other factors
All bugs I raised in prior reviews were addressed: the webkit error message on Windows was fixed, the Windows guard was narrowed to the spawn subpath, the ConnectFailed/AutoDetectConnectFailed split was added, the non-deterministic auto-detect test cases were moved to the scrubbed-child helper, the ws:// hint is positively asserted in both test files, and UNEXPECTED_SUCCESS guards were added. No new bugs were found by the bug hunting system. Test coverage is comprehensive for all four Windows-visible shapes.
`new Bun.WebView({})` on Windows (or `{ backend: "chrome" }`, or even
with an explicit `backend.path`) threw ERR_DLOPEN_FAILED with
Failed to spawn Chrome (set BUN_CHROME_PATH, backend.path, or
install Chrome/Chromium)
ChromeProcess.zig's `Bun__Chrome__ensure` short-circuits to -1 on
Windows before any of BUN_CHROME_PATH / backend.path / findChrome run,
so the hint is actively wrong — the user follows it and the same
error comes back. The socketpair + --remote-debugging-pipe fd plumbing
has no direct Windows equivalent yet (needs named pipes or libuv).
Throw ERR_METHOD_NOT_IMPLEMENTED up front on Windows with a clear
platform-status message, mirroring the existing pattern for
backend:"webkit" on non-Darwin.
Fixes #29102
If new Bun.WebView({}) ever stops throwing here, the child would keep a
live view open and this test would time out instead of failing cleanly.
Log UNEXPECTED_SUCCESS and exit non-zero so the harness sees the
regression as a test failure, not a 2-minute hang.
The previous fix threw ERR_METHOD_NOT_IMPLEMENTED for ALL chrome backend
calls on Windows, but only the spawn path (Bun__Chrome__ensure -> POSIX
socketpair) is actually broken there. The WebSocket connect path
(ensureConnected -> WebCore::WebSocket) works fine on Windows, and
Bun__Chrome__autoDetect already has a Windows branch that reads
DevToolsActivePort from %LOCALAPPDATA%.
Move the guard inside createChrome so it only fires when the call
actually reaches ensureSpawned. backend: { url: "ws://..." } and
auto-detect-a-running-Chrome now work on Windows as they did before
PR #29106.
createChrome gains an outparam ChromeCreateFailure so the constructor
can throw the right error: NotImplementedOnWindows vs SpawnFailed vs
ConnectFailed — each with a message scoped to its actual cause.
Also: on Windows, the backend:"webkit" error pointed users at
"use backend: chrome", but chrome's spawn path is now also not
implemented there. Updated the Windows-specific message to point at
the ws:// connect workaround instead, so users don't hit a second
not-implemented error following the hint.
Tests: added a "backend.url:'ws://...' is NOT blocked" case to make
sure the guard doesn't regress back to blocking WebSocket. Tightened
in-process tests with fail-fast view.close() + throw on unexpected
success (no more hangs if the constructor stops throwing). Added the
missing /backend.path/ check and asserted stderr === "" in the
spawned-child test.
…-brittle stderr assert
coderabbit flagged two issues on the regression test:
1. The 'default' and 'backend: "chrome"' in-process tests assumed
the constructor always throws, but auto-detect could succeed on a
CI runner where Chrome is running with --remote-debugging-port
(Bun__Chrome__autoDetect reads DevToolsActivePort from
%LOCALAPPDATA%). Moved those two cases to spawned children with
LOCALAPPDATA pointing at an empty tempDir, so autoDetect is
guaranteed to miss and the constructor falls through to the spawn
path deterministically. backend.path and backend.url:false stay
in-process — they force spawn without consulting auto-detect.
2. expect(stderr).toBe("") is brittle on ASAN-enabled builds,
which emit 'WARNING: ASAN interferes with JSC signal handlers'
to stderr on otherwise-successful runs. Dropped that assertion
but still drain the pipe to avoid backpressure deadlocks.
claude[bot] noted the JSWebView.h comment for ConnectFailed said it fires only on an explicit user ws:// url, but the same reason was also set from the auto-detect branch — where the user never touched backend.url and the error message's 'check backend.url' hint is misleading. Split into ConnectFailed (explicit user URL) and AutoDetectConnectFailed (malformed DevToolsActivePort file, user never set backend.url). The latter now throws a neutral 'malformed DevToolsActivePort file' message. coderabbit nits on the test file: - runInScrubbedChild had an unused extraEnv parameter — removed. - The 18-line header comment block restated bug history already covered by the PR description. Trimmed to just the issue URL.
coderabbit caught a real bug: my Windows guard in trySpawn refused before giving Transport::ensureSpawned a chance to short-circuit on an already-live singleton. The transport has 'first call wins' semantics — once a view connects via backend.url, its WebSocket transport is reused by subsequent views regardless of their backend shape. ensureSpawned's first line is `if (m_mode != TransportMode::None && !m_dead) return true;`, which handles that reuse. My previous version skipped that fast path and threw ERR_METHOD_NOT_IMPLEMENTED for a second view with backend.url:false or backend.path even though no spawn was needed. Fix: call ensureSpawned unconditionally; only translate its failure (Bun__Chrome__ensure returning -1 because there's no Windows spawn path) into NotImplementedOnWindows. The live-singleton fast path now works correctly on all platforms.
claude[bot] caught that webview.test.ts's 'backend: chrome spawn
throws on Windows' test iterated over {} and { backend: 'chrome' }
in-process — both hit Bun__Chrome__autoDetect which reads
DevToolsActivePort from %LOCALAPPDATA%. On a Windows CI runner with
Chrome already running with --remote-debugging-port, auto-detect
finds the file, ensureConnected succeeds, and the test fails with
UNEXPECTED_SUCCESS — a false positive.
Those two shapes are already covered deterministically in
test/regression/issue/29102.test.ts via runInScrubbedChild (which
points LOCALAPPDATA at an empty tempdir). Removed them from the
webview.test.ts loop; kept only backend.path and backend.url:false
which force spawn without touching auto-detect.
claude[bot] noted that the helpers only asserted the OLD misleading hints (BUN_CHROME_PATH, set.*backend.path) were absent, but never checked that the NEW ws:// connect workaround hint was present. Stripping the ws:// guidance from the NotImplementedOnWindows message would pass all four tests silently — the whole point of the fix (a clear actionable error) could regress undetected. Add expect(...).toMatch(/ws:\/\//) to both expectNotImplementedError and expectChildStdoutNotImplemented. Mirrors the pattern already used in webview.test.ts's webkit-on-Windows test.
…me spawn loop
Two nits from claude[bot]:
1. The pre-existing 'backend: webkit throws on non-darwin' test's
regex /only available on macOS.*backend.*chrome/i incidentally
matched the new Windows webkit message (which mentions
`backend: { type: "chrome", url: "ws://..." }` in its hint).
That's the right answer for the wrong reason — a future simpli-
fication of the Windows message could silently break this test.
Gate the non-darwin branch to skip Windows (the Windows-specific
companion test at lines 125+ already covers that platform), and
narrow the regex to /only available on macOS.*use backend.*chrome/i
so it only matches the Linux 'use backend: chrome' phrasing.
2. The chrome spawn test loop asserted the OLD misleading hints were
absent but never positively asserted that the NEW ws:// workaround
hint was present. Stripping ws:// from the message would pass all
four existing assertions silently. Added the ws:// positive check,
matching the pattern in 29102.test.ts's helpers.
f71d267 to
5a1797b
Compare
debian-13-x64-asan-test-bun failed on 5a1797b with exit status 2. Cannot access BuildKite job log without a token. All webview tests pass locally on bun bd (ASAN debug). Retriggering to determine if flaky vs real. If it fails again with same signature, the issue is in this PR; otherwise flake.
claude[bot] caught a latent test ordering trap: the in-process ws:// connect test left Transport::m_mode = WebSocket with the singleton alive after view.close() (because registerView never set m_sockRefd, so updateKeepAlive's early-return skipped the WebSocket teardown). After commit 10c571c made trySpawn reuse live singletons on Windows, a later in-process spawn-forcing test would short-circuit ensureSpawned's fast-path on that leaked singleton and fail with UNEXPECTED_SUCCESS. Doesn't fail today because bun:test runs in declaration order (spawn-forcing tests are declared before the ws:// test), but a future reorder or a new in-process test added after ws:// would silently break. Fix: move the ws:// test into runInScrubbedChild. Each test now runs in its own process; no singleton leaks to trap siblings. The child forces process.exit(0) after logging the outcome to ensure it terminates even if the WebSocket to port 1 pins the event loop.
There was a problem hiding this comment.
All prior review feedback has been addressed and I didn't find anything further, but this refactors platform-specific control flow in createChrome (the trySpawn lambda, the Windows autoDetectedFallback=false decision, and singleton-reuse semantics) — worth a maintainer's eye on the design before merge.
Extended reasoning...
Overview
This PR fixes #29102 by replacing a misleading "Failed to spawn Chrome (set BUN_CHROME_PATH...)" error on Windows with an accurate ERR_METHOD_NOT_IMPLEMENTED that points users at the working ws:// connect workaround. It touches three C++ files in src/runtime/webview/ (~100 net lines: a new ChromeCreateFailure enum, a trySpawn lambda in createChrome, a switch on failure mode in the constructor, and a Windows-specific webkit error message) plus ~300 lines of new Windows-gated tests across webview.test.ts and a new 29102.test.ts regression file.
Security risks
None identified. The change is entirely error-message routing and platform-conditional control flow in the WebView constructor; no auth, crypto, permissions, or untrusted-input parsing is involved. The new error strings are static literals.
Level of scrutiny
Medium. This is not a mechanical change — it refactors transport-selection logic in JSWebView::createChrome with platform-specific branching (the trySpawn lambda now calls ensureSpawned unconditionally to preserve singleton reuse, and autoDetectedFallback is forced to false on Windows to avoid falling back to a nonexistent spawn path). The non-Windows behavior is intended to be unchanged, and reading the diff that appears to hold, but the interaction between singleton state, auto-detect, and the new failure-enum routing is subtle enough that a maintainer who owns the WebView/Chrome backend should confirm the design.
Other factors
The PR went through ~10 rounds of bot review (CodeRabbit + my own inline comments), all of which are now resolved — including narrowing the Windows guard to spawn-only, splitting ConnectFailed/AutoDetectConnectFailed, preserving live-transport reuse before returning NotImplementedOnWindows, fixing test determinism around Bun__Chrome__autoDetect, and isolating the ws:// positive test in a subprocess. Test coverage for the new error paths is thorough. The one CI failure (test-http-should-emit-close-when-connection-is-aborted.ts timeout on Windows lanes) is unrelated to this change. Given the iteration depth and the runtime C++ surface area, I'm deferring rather than approving.
|
This PR has been closed because it was flagged as AI slop. Many AI-generated PRs are fine, but this one was identified as having one or more of the following issues:
If you believe this was done in error, please leave a comment explaining why. |
github-actions
Bot
changed the title
|
Context for whoever reviews this flag:
If maintainers prefer a different shape for this fix (or want the spawn path implemented rather than cleanly rejected), happy to rework it. Otherwise I believe the flag is a false positive. |
2 participants
This PR has been marked as AI slop and the description has been updated to avoid confusion or misleading reviewers.
Many AI PRs are fine, but sometimes they submit a PR too early, fail to test if the problem is real, fail to reproduce the problem, or fail to test that the problem is fixed. If you think this PR is not AI slop, please leave a comment.