-
Notifications
You must be signed in to change notification settings - Fork 2
Fix Windows console-window flash on browser/runtime auto-launch #471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e700489
728099d
179947f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,10 +49,21 @@ export async function autoLaunchBusinessHub(projectRoot, opts = {}) { | |
| if (!opts.noBrowser && process.env.RSTACK_NO_BROWSER !== '1') openBrowser(url); | ||
| } | ||
|
|
||
| function openBrowser(url) { | ||
| const cmd = process.platform === 'win32' ? 'start' | ||
| : process.platform === 'darwin' ? 'open' : 'xdg-open'; | ||
| // platform/spawnImpl are injectable (mirrors sandbox.js's spawnImpl convention) | ||
| // so #470's windowsHide behavior is testable per-platform without actually | ||
| // spawning a browser. | ||
| export function openBrowser(url, { platform = process.platform, spawnImpl = spawn } = {}) { | ||
| const cmd = platform === 'win32' ? 'start' | ||
| : platform === 'darwin' ? 'open' : 'xdg-open'; | ||
| try { | ||
| spawn(cmd, [url], { stdio: 'ignore', detached: true, shell: process.platform === 'win32' }).unref(); | ||
| // On win32 this runs through `cmd.exe /c start <url>` (shell: true); without | ||
| // windowsHide, that intermediate cmd.exe briefly flashes a visible console | ||
| // window before handing off to the actual browser process. | ||
| spawnImpl(cmd, [url], { | ||
| stdio: 'ignore', | ||
| detached: true, | ||
| shell: platform === 'win32', | ||
| windowsHide: true, | ||
| }).unref(); | ||
|
Comment on lines
+59
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. Duplicated openbrowser/openurl spawn logic The PR repeats near-identical spawn(..., { shell: win32, windowsHide: true, ... }) browser-launch
logic (and explanatory comments) across multiple files, increasing maintenance overhead and drift
risk. This violates the DRY compliance requirement to refactor duplicated logic into a shared
helper/module.
Agent Prompt
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Fixed — exported |
||
| } catch { /* best-effort */ } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| /** | ||
| * #470: on win32, spawning a browser/runtime process via `cmd.exe` (shell: | ||
| * true) or directly (cmd.exe/podman.exe) briefly flashes a visible console | ||
| * window unless `windowsHide: true` is passed — Node's spawn() defaults it | ||
| * to false. Pins that every one of the four affected spawn call sites | ||
| * passes windowsHide on win32, and that non-win32 platforms are unaffected | ||
| * (the option is harmless there, but shell must stay false). | ||
| * | ||
| * owner: RStack developed by Richardson Gunde | ||
| */ | ||
|
|
||
| // src/observability/dashboard/server.js has an identical openBrowser copy, | ||
| // but that module calls server.listen() unguarded at the top level (a | ||
| // pre-existing design predating this PR, out of scope to refactor here) — | ||
| // importing it for its openBrowser export would start a real HTTP server as | ||
| // a side effect, so it's covered by code-review parity with the two | ||
| // call sites below instead of a direct import. | ||
| import test from 'node:test'; | ||
| import assert from 'node:assert/strict'; | ||
| import { openBrowser as openBrowserAutoLaunch } from '../src/hooks/auto-launch.js'; | ||
| import { openUrl } from '../src/integrations/pi/rstack-sdlc.ts'; | ||
| import { startContainerRuntime } from '../src/core/harness/sandbox.js'; | ||
|
|
||
| function fakeChild() { | ||
| return { on() {}, unref() {} }; | ||
| } | ||
|
|
||
| function recordingSpawn(calls) { | ||
| return (cmd, args, options) => { | ||
| calls.push({ cmd, args, options }); | ||
| return fakeChild(); | ||
| }; | ||
| } | ||
|
|
||
| test('auto-launch.js openBrowser: windowsHide:true + shell:true on win32', () => { | ||
| const calls = []; | ||
| openBrowserAutoLaunch('http://localhost:3008', { platform: 'win32', spawnImpl: recordingSpawn(calls) }); | ||
| assert.equal(calls.length, 1); | ||
| assert.equal(calls[0].cmd, 'start'); | ||
| assert.equal(calls[0].options.shell, true); | ||
| assert.equal(calls[0].options.windowsHide, true); | ||
| }); | ||
|
|
||
| test('auto-launch.js openBrowser: shell:false on macOS/Linux, windowsHide harmless', () => { | ||
| const calls = []; | ||
| openBrowserAutoLaunch('http://localhost:3008', { platform: 'darwin', spawnImpl: recordingSpawn(calls) }); | ||
| assert.equal(calls[0].cmd, 'open'); | ||
| assert.equal(calls[0].options.shell, false); | ||
| }); | ||
|
|
||
| // openUrl early-returns when process.env.CI is set (intentional — avoids | ||
| // popping a browser during a CI run), which GitHub Actions always sets. Clear | ||
| // it for the duration of these two tests so they exercise the real spawn | ||
| // path there too, restoring it afterward so the guard itself stays covered. | ||
| function withoutCiEnv(fn) { | ||
| const had = Object.prototype.hasOwnProperty.call(process.env, 'CI'); | ||
| const prior = process.env.CI; | ||
| delete process.env.CI; | ||
| try { | ||
| fn(); | ||
| } finally { | ||
| if (had) process.env.CI = prior; | ||
| } | ||
| } | ||
|
|
||
| test('pi/rstack-sdlc.ts openUrl: windowsHide:true + shell:true on win32', () => { | ||
| withoutCiEnv(() => { | ||
| const calls = []; | ||
| openUrl('http://localhost:3008', { platform: 'win32', spawnImpl: recordingSpawn(calls) }); | ||
| assert.equal(calls[0].cmd, 'start'); | ||
| assert.equal(calls[0].options.shell, true); | ||
| assert.equal(calls[0].options.windowsHide, true); | ||
| }); | ||
| }); | ||
|
|
||
| test('pi/rstack-sdlc.ts openUrl: linux uses xdg-open, shell:false', () => { | ||
| withoutCiEnv(() => { | ||
| const calls = []; | ||
| openUrl('http://localhost:3008', { platform: 'linux', spawnImpl: recordingSpawn(calls) }); | ||
| assert.equal(calls[0].cmd, 'xdg-open'); | ||
| assert.equal(calls[0].options.shell, false); | ||
| }); | ||
| }); | ||
|
|
||
| test('pi/rstack-sdlc.ts openUrl: no-ops under CI (never pops a browser during automated runs)', () => { | ||
| const calls = []; | ||
| const original = process.env.CI; | ||
| process.env.CI = '1'; | ||
| try { | ||
| openUrl('http://localhost:3008', { platform: 'win32', spawnImpl: recordingSpawn(calls) }); | ||
| } finally { | ||
| if (original === undefined) delete process.env.CI; else process.env.CI = original; | ||
| } | ||
| assert.equal(calls.length, 0, 'openUrl must not spawn anything when CI is set'); | ||
| }); | ||
|
|
||
| test('sandbox.js startContainerRuntime: windowsHide:true on the engine auto-start spawn', async () => { | ||
| const calls = []; | ||
| const result = await startContainerRuntime({ | ||
| installedProbe: () => 'docker', | ||
| readyProbe: () => false, | ||
| spawnImpl: (cmd, args, options) => { calls.push({ cmd, args, options }); return fakeChild(); }, | ||
| platform: 'win32', | ||
| timeoutMs: 0, | ||
| }); | ||
| assert.equal(calls.length, 1); | ||
| assert.equal(calls[0].options.windowsHide, true); | ||
| assert.equal(result.started, true); | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.