Skip to content

fix(cli): hide Windows console for remaining child-process spawns - #3960

Closed
lorenzozanee wants to merge 1 commit into
heygen-com:mainfrom
lorenzozanee:restore/pr-3956
Closed

lorenzozanee wants to merge 1 commit into
heygen-com:mainfrom
lorenzozanee:restore/pr-3956

Conversation

@lorenzozanee

Copy link
Copy Markdown
Contributor

What

Hide Windows console windows for three remaining child-process spawns in @hyperframes/cli missed by the earlier windowsHide coverage (#3379 in engine, #3529 in cli):

  • telemetry/transport.ts flushSync() — spawns a detached node on every CLI exit to deliver queued events
  • utils/clipboard.ts — where provider probe and the provider copy itself (clip.exe on Windows)
  • server/portUtils.ts windowsListenerPid() — netstat lookup used by preview --list/--stop/conflict logging on Windows

Each site now passes windowsHide: true, a no-op on macOS/Linux. Three regression tests mirror the existing *.windowsHide.test.ts style.

Why

These binaries are console-subsystem on Windows, so spawning them without windowsHide pops a visible console window — including from contexts that own no console (detached preview server, exit-time flush). This closes out the remaining scope of #3500: every spawn/execFile/spawnSync site reachable on Windows in cli, studio-server, lint and engine now carries the flag.

How

One-line option additions only, following the exact pattern already merged in orphanCleanup.ts, manager.ts, preflight.ts, mediaMetadata.ts, waveform.ts, proxyTranscoder.ts, mediaValidation.ts and hevcPreviewLint.ts. No refactor, no new abstraction, no behavior change on non-Windows platforms.

Test plan

  • Unit tests added/updated — 3 new suites (transport, clipboard, portUtils windowsHide tests) fail on base with the flag missing and pass after the fix; existing portUtils.test.ts and all runnable *.windowsHide.test.ts suites pass; oxlint/oxfmt clean; tsc --noEmit shows only pre-existing missing-generated-file errors
  • Manual testing performed — the symptom is Windows-only console flashing; validated via mocked regression tests on Linux (no Windows runner available)
  • Documentation updated (if applicable) — no user-facing behavior or docs change

Closes #3500

Supersedes #3956(原PR分支已删,按原提交重建)

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Two of the three sites are still missing windowsHide on main, and the third is already fixed there, so this merges cleanly and the leftover hunk is a no-op.

What I checked on current main:

  • server/portUtils.ts still calls netstat without windowsHide (line 220), so preview --list/--stop on Windows flashes a console.
  • utils/clipboard.ts still spawns the copy tool (clip.exe on Windows) without it.
  • telemetry/transport.ts already carries windowsHide: true on the exit-time flush (landed with #3996). Git's 3-way merge folds the identical change in, and git merge-tree against main reports no conflicts.

I ran the new tests on main without the fix: the netstat and clipboard tests fail (the telemetry one passes, as expected since main already has it). With the fix all three suites plus the existing portUtils.test.ts pass, 16/16.

One small note, not blocking: on real Windows os.platform() returns win32 and detectProvider goes straight to clip.exe, so the where probe path is not reachable there. The clipboard test reaches it by mocking os.platform() to linux while setting process.platform to win32. That is fine for pinning the option, but the copy spawn is the one that matters.

Need: yes for netstat and clipboard; the telemetry part is already on main · Size: ok · Reuse: same one-option pattern as the existing *.windowsHide fixes · Simpler: already minimal

— Rames

@jrusso1020

Copy link
Copy Markdown
Collaborator

Thanks, @lorenzozanee. This one is approved, and it'll go into the merge queue once these are sorted:

Once it's green and signed, we'll queue it at your new head. If you push code changes beyond a rebase, we'll take another look first.

— Rames

@jrusso1020

Copy link
Copy Markdown
Collaborator

This is approved, but it now conflicts with main, and main requires signed commits. Could you rebase on current main with signed commits and force-push? Once CI passes we can merge it.

— Rames

@miguel-heygen

Copy link
Copy Markdown
Collaborator

Thank you for finding the remaining Windows console flashes. The change is correct: windowsHide: true on the netstat lookup in server/portUtils.ts and on the clipboard spawn in utils/clipboard.ts, with a test for each, and the telemetry one has since landed on main by itself. It was approved, but it has had no update since September 15, it now conflicts with main, the commit is unsigned and CI never ran on the branch. We are closing it for now. A version we would merge is the portUtils and clipboard changes with their tests, rebased on current main, in a signed commit. Please reopen or send a new PR when you can; that is a small change to land.

@miguel-heygen

Copy link
Copy Markdown
Collaborator

Re-landed as #6449 on current main, with you as co-author on the commit. Thank you for the fix.

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

Labels

None yet

Projects

None yet

3 participants