Repository navigation
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Flagging an overlap so this and your #1863 don't collide: both fix the same defect class — Windows console flashes from They also cover disjoint spawn sites:
So neither PR is complete in isolation — the full fix is the union (outer launcher + inner detached spawn + taskkill + git + icacls). Suggest folding #1863's |
|
Thanks for picking this up — the Bun.spawnSync→Node One gap worth folding in while you're here: function getGitRoot(): string | null {
...
const proc = Bun.spawnSync(['git', 'rev-parse', '--show-toplevel'], { ... });That's the identical |
|
Good to hear it matches what you've been running locally since #1784, that's solid real-world validation. On the overlap flag: #1863 and this PR are complementary rather than competing. #1863 adds windowsHide to the long-lived detached server spawn; this one covers the short-lived cold-start child spawns that flash a console per invocation. Either can land first; no shared lines. |
|
Correcting my earlier comment: the two PRs did overlap after all, both touched the startServer launcher block and both created cli-windows-hide.test.ts. Rather than leave a guaranteed merge conflict, #1863's changes are folded in here (commits 4d30746 and 77ebc14): windowsHide on the inner detached server spawn, the osascript raise helper, and the POSIX server spawn, plus its every-detached-spawn-carries-windowsHide test invariant. #1863 is closed; this PR is now the complete console-flash fix. |
|
@mvanhorn Rewrite this to cover the reported Git spawn in find-browse and add a Windows test confirming no visible console window appears. |
find-browse.ts compiles to its own console-subsystem binary and its getGitRoot() used the same uncovered Bun.spawnSync git rev-parse that config.ts fixed, so it kept flashing a conhost window per invocation. Apply the same Windows-only nodeSpawnSync + windowsHide:true treatment and keep Bun.spawnSync on POSIX. Extend cli-windows-hide.test.ts with a regression pinning the hidden git spawn at that site. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
|
Done (299b3a4). find-browse.ts's getGitRoot() now takes the same Windows-only nodeSpawnSync + windowsHide:true path and keeps Bun.spawnSync on POSIX, matching the config.ts treatment @kaiwulff flagged as the second flicker source. Extended cli-windows-hide.test.ts with a regression pinning the hidden git spawn at that site, so a future revert to Bun.spawnSync there fails the suite; the focused suite passes 8/8. It follows the same static source-assertion approach as the rest of the #1784 coverage rather than spawning a live Windows console in CI. |
|
Thanks @mvanhorn, and thanks @kaiwulff for the real-desktop verification. Closing this one without merging: main already carries its windowsHide hunks (icacls in file-permissions.ts, the launcher and taskkill in cli.ts, the detached server spawn), and @kaiwulff confirmed zero console windows and no focus steal on Windows 11 with an unpatched v1.91.15.0 build (#1784). The remaining Bun.spawnSync to node:child_process swap would not change behavior inside Bun, whose child_process.spawnSync forwards to Bun.spawnSync with the same options. Nothing new from this PR landed in the wave PR #3022; it records #1784 as fixed on main, and adds a TODO for a real-desktop Windows spawn smoke test. |
Summary
Suppresses the focus-stealing console windows that flash on every cold start of
browseon Windows by routing the console-subsystem child spawns through a hidden path.execFileSync('icacls', ...)inbrowse/src/file-permissions.tsgetswindowsHide: true, and theBun.spawnSynclauncher and git-root calls inbrowse/src/cli.tsandbrowse/src/config.tsfall back to Node'schild_processwithwindowsHide: trueon Windows, sinceBun.spawnSyncdoes not honorwindowsHideon bun 1.3.11.Why this matters
Issue #1784 reports that on Windows 11 every cold start of
browseflashes two or more focus-stealingconhost.exewindows that interrupt typing in other apps. AWin32_ProcessStartTracetraced the flashes to console-subsystem children spawned without an effectivewindowsHide: thenodedetach launcher andgit rev-parsecalls go throughBun.spawnSync, which ignoreswindowsHideon bun 1.3.11, and theicacls.execalls useexecFileSyncwithout the flag. The fix addswindowsHide: trueto theexecFileSynccalls infile-permissions.tsand, on Windows only, routes the launcher and git lookups incli.tsandconfig.tsthrough Node'schild_process(which honors the flag), keepingBun.spawnSyncon POSIX behind the existing platform check.Testing
browse/test/cli-windows-hide.test.tsis a static regression test asserting the cold-start spawn sites carrywindowsHideor use the hidden-spawn helper on Windows, and fails if a console-subsystem spawn site reintroduces a non-hidden Windows spawn. POSIX continues to useBun.spawnSyncunchanged. Full suite runs in CI.Fixes #1784