fix(desktop): keep e2e windows visible without stealing focus - #4579
Conversation
`MAKA_E2E_SHOW_WINDOW` conflated two things: whether the window appears
and whether the app takes the foreground. Every fixture that sets it —
and `e2e/fixtures.ts` sets it for eight of them, plus every run on a CI
Linux display — got a dock tile, an activated app, and a window that
jumped in front of whatever the developer was doing, repeatedly, for the
length of the suite. Nothing in those fixtures wants focus: they want the
compositor (a hidden window is throttled to ~1fps under xvfb) and a real
layout for geometry assertions.
So the flag now lifts the window's visibility and stops there. The single
`startHidden` boolean becomes the run's reveal mode — `hidden`,
`inactive`, `active` — resolved once in `window-reveal.ts` and consumed
by both the reveal gate and the dock rule:
- `hidden`: unchanged. E2E captures paint the hidden window.
- `inactive`: reveal with `showInactive()`, stay an accessory app, and
answer a focus request with a reveal and nothing more. `maximize()`
reveals a hidden window and that reveal activates, so an inactive
reveal comes first and leaves it nothing to show.
- `active`: the product, unchanged.
The E2E harness had the same conflation of its own: the prompt-rail
worker window is re-revealed before every test in the file with
`window.show()`, which activates the app each time no matter what the
main process decided. It reveals inactively now.
Verified on macOS by sampling the frontmost application while running
`playwright test e2e/prompt-rail.spec.ts`: before, the Electron app held
the foreground in 14 of 130 samples; after, in 0 of 130, with all nine
tests still passing.
Generated-by: Claude Code
445192e to
e19aac5
Compare
jackwener
left a comment
There was a problem hiding this comment.
I reviewed exact head e19aac5b20860ecdcd8203aca03465caef6ffc63. No P0 or P1. One P2 and two P3s. Not approving: the P2 is introduced here and has a fix already sitting in this repo.
The split this PR makes is the right one. "Should a window appear" and "should the app take the foreground" were genuinely one flag, and separating them into hidden / inactive / active resolved in one place is a real simplification rather than a flag added beside the old one. The product path is untouched: a non-E2E run still resolves active, still show()s, and still honors a deferred focus request. The 14 new cases are also not tautological — reverting the gate to the old semantics fails 5 of them, so they test the rule rather than transcribing the implementation.
P2 — showInactive() is unsupported on native Wayland, and this repo already says so
scripts/fixture-window.mjs:60-71 carries this note, and it predates this PR:
Electron 43 defaults to native Wayland when
XDG_SESSION_TYPE=wayland, whereBrowserWindow.showInactive()is unsupported. Keep inactive fixtures on XWayland.
It ships the remedy next to the note — inactiveWindowElectronArgs(), which appends --ozone-platform=x11. That helper has exactly one caller today: scripts/fixture-window.mjs:163.
This PR routes two more launch paths into showInactive() without it:
apps/desktop/e2e/fixtures.ts:460-461launches with a hardcodedargs: ['.'], and every fixture there that asks for a visible window now reveals inactively (nineshowWindow: truecall sites in that file), both through the main-process reveal mode and through the explicitwindow.showInactive()atapps/desktop/e2e/fixtures.ts:532.scripts/desktop-real-window-smoke.mjs:318assembles['.', '--user-data-dir=…']directly and always passesshowWindow: true, so it resolves toinactivetoo.
On a Linux developer machine running a Wayland session, those windows go through an API the platform does not support and may simply not appear — which puts back the ~1fps compositor throttling and the geometry failures that asking for a visible window exists to avoid. Hosted CI is not affected: .github/workflows/ci.yml:333-369 creates an Xvfb/X11 display, where showInactive() is supported. So the description's claim that the Linux CI path is untouched holds; it is the ordinary local Wayland path that regresses.
The smallest fix is to reuse inactiveWindowElectronArgs() in both launchers, keeping their own extra arguments, and add a launch-policy case for it. Please don't grow a second Wayland detection beside the existing one.
On evidence: this rests on Electron's documented contract and on this repo's own existing note, not on an observed failure — no Wayland machine was available, so nothing here was reproduced on one. The 14 new cases cover the mode rule, the dock rule and the gate; none of them covers launcher arguments, which is exactly why this got through.
P3 — for inactive, the first on-screen frame is no longer the maximized one
createWindowRevealGate's comment at window-reveal.ts:114-118 still states that deferring the maximize means "the window's first on-screen frame is already maximized". That was true when maximizeNow called maximize() on a still-hidden window. In inactive mode it now reveals first and maximizes second, so the first visible frame is the unmaximized bounds and the window then zooms — the two-stage reveal the deferral was written to prevent.
Reaching it needs saved maximized bounds together with a visible-window run, and E2E user data is isolated and does not persist them, so the practical impact is small and it is graded accordingly. But the comment and the code now say different things, and one of them should move.
P3 — the "one authority" claim holds at the resolver, not at the consumers (pre-existing)
resolveWindowRevealMode() is a single authority, but its value is not what both consumers read. main-window.ts:179 overrides it to active for packaged builds, and that override is module-local; desktop-shell-presentation.ts:37-40 passes the un-overridden value from runtime-host-boot.ts to resolveDockPresentation(). A packaged build carrying a stray E2E variable therefore gets a window that reveals and can take focus, while the dock rule treats the same run as an accessory app with no tile.
This is not a regression. main-window.ts:178 on main applies !app.isPackaged && startHidden while desktop-shell-presentation.ts:39 passes the raw startHidden, so the same split is already there and this PR preserves it in new spelling. It is raised only because the description presents the change as "one authority for how far a run may go when it reveals its window," and that is accurate for the resolver but not for what the two consumers actually use. The new cases contain no isPackaged coverage, so nothing guards the boundary either way.
test passed on this head; package was still running when I posted, so this is not a merge-readiness judgement. The PR is still a draft. I am not approving and not merging.
简体中文
我审的是 e19aac5b20860ecdcd8203aca03465caef6ffc63,没有 P0/P1,一条 P2、两条 P3。不给 approve:那条 P2 是这次改动引入的,而修法就在本仓库里现成放着。
这个 PR 的拆分方向是对的。「窗口要不要出现」和「应用要不要抢前台」本来确实被塞在同一个开关里,拆成 hidden / inactive / active 三态、并在一处解析,是真正的化简,而不是在旧开关旁边又加了一个。产品路径没有被动到。新增的 14 个用例也不是空转——把闸门改回旧语义,其中 5 个会失败。
P2:showInactive() 在原生 Wayland 上不受支持,而这一点仓库自己早就写着。 scripts/fixture-window.mjs:60-71 的注释(早于本 PR)明确写了 Electron 43 在 XDG_SESSION_TYPE=wayland 下走原生 Wayland,showInactive() 不受支持,并且就在旁边给了解法 inactiveWindowElectronArgs()(追加 --ozone-platform=x11)。这个函数今天只有一个调用者,就是它自己所在文件的第 163 行。本 PR 让另外两条启动路径开始调用 showInactive(),却都没有用它:apps/desktop/e2e/fixtures.ts:460-461 硬编码 args: ['.'],scripts/desktop-real-window-smoke.mjs:318 直接拼参数且始终 showWindow: true。在 Wayland 会话的 Linux 开发机上,这些窗口可能根本不出现,于是「可见窗口」本来要规避的合成器降频和几何断言失败又回来了。托管 CI 不受影响,因为 .github/workflows/ci.yml:333-369 建的是 Xvfb/X11。最小修法是让这两个启动器复用现成的 inactiveWindowElectronArgs(),别再造第二套 Wayland 判断。证据边界要说清楚:这条依据的是 Electron 的文档契约和仓库自己的既有注释,没有在真实 Wayland 机器上复现过。
P3:inactive 下第一帧不再是最大化的那一帧。 window-reveal.ts:114-118 的注释仍然写着延迟 maximize 是为了「第一帧就已经最大化」,但 inactive 模式现在先 reveal 再 maximize,第一帧是未最大化的,然后才缩放——正是这个延迟当初要避免的两段式显示。触发需要保存过的最大化边界,E2E 用的是隔离的用户数据、不会留存,所以实际影响很小,按 P3 记。但注释和代码现在说的不是一回事,总得改一个。
P3:「单一权威」这句话在解析器成立,在消费者不成立(既有问题,非本次引入)。 main-window.ts:179 对打包构建覆盖成 active,但这个覆盖是模块内局部的;desktop-shell-presentation.ts:37-40 拿的是 runtime-host-boot.ts 里没被覆盖的那个值。于是打包构建带着游离的 E2E 变量时,窗口会显示并可能抢焦点,而 dock 规则把同一次运行当作 accessory app。这不是回归——main 上 main-window.ts:178 和 desktop-shell-presentation.ts:39 已经是同样的分叉,本 PR 只是换了写法。提出来只因为描述把这次改动说成「一个权威」,那对解析器成立,对两个消费者实际读到的值不成立。新增用例里没有任何 isPackaged 覆盖。
test 在这个 head 上已通过,package 在我发布时还在跑,所以这不是合并就绪判断。PR 目前仍是 draft。我不 approve,也不合并。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
jackwener
left a comment
There was a problem hiding this comment.
Approving at exact head e19aac5b20860ecdcd8203aca03465caef6ffc63. No P0 or P1.
My findings are in the review above at this same head and stand unchanged — one P2 and two P3s, none of them blocking:
- P2 (introduced) —
showInactive()is unsupported on native Wayland, andscripts/fixture-window.mjs:60-71already says so and already ships the remedy (inactiveWindowElectronArgs()). The two launchers this PR routes into inactive reveals don't reuse it. Worth fixing before it bites someone on a Wayland desktop; hosted CI is not affected. - P3 (introduced) — in
inactivemode the first on-screen frame is no longer the maximized one, which contradicts the comment retained atwindow-reveal.ts:114-118. - P3 (pre-existing) — the packaged-build override applies to the window gate but not the dock rule. Not a regression; raised only because the description presents the change as one authority.
All three checks are green on this head (label, package, test). The separation this PR makes — whether a window appears, versus whether the app takes the foreground — is the right cut, and it replaces the old flag rather than sitting beside it.
I am not merging.
简体中文
在 e19aac5b20860ecdcd8203aca03465caef6ffc63 上批准。没有 P0/P1。
我的发现都在同一个 head 的上一条评审里,结论不变——一条 P2、两条 P3,都不构成阻塞:
- P2(本次引入):
showInactive()在原生 Wayland 上不受支持,而scripts/fixture-window.mjs:60-71早就写着这件事、也早就给了解法inactiveWindowElectronArgs();本 PR 新接入 inactive 显示的两个启动器都没有复用它。建议在有人在 Wayland 桌面上踩到之前修掉;托管 CI 不受影响。 - P3(本次引入):
inactive模式下第一帧不再是最大化的那一帧,和window-reveal.ts:114-118保留的注释自相矛盾。 - P3(既有问题):打包构建的覆盖只作用于窗口闸门,不作用于 dock 规则。不是回归,提出来只因为描述把这次改动说成「单一权威」。
这个 head 上三项检查全绿(label、package、test)。这个 PR 做的拆分——窗口要不要出现,和应用要不要抢前台——方向是对的,而且是替换掉了旧开关,不是在旁边又加一个。
我不合并。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
…sion `showInactive()` is unsupported when Electron runs on native Wayland, so a window revealed that way can simply fail to appear — which brings back the ~1fps compositor throttling and the geometry failures that asking for a visible window exists to avoid. The repo already knew this and already carried the remedy, but it lived inside one launcher and only that launcher used it, so routing two more launch paths into inactive reveals walked straight past it. Move the rule next to `isCiLinuxDisplay` in `fixture-env.mjs`, which is already the shared authority for what a fixture launch environment looks like, and have it return only the extra arguments so each launcher composes it with its own. All three inactive launchers now go through it. Hosted CI is unaffected either way: it runs on an Xvfb X11 display, where showInactive() is supported. Two corrections from the same review: The reveal gate's comment claimed the deferred maximize makes the window's first on-screen frame the maximized one. That is only true in `active` mode; an `inactive` window is revealed first and maximized second, because the reveal maximize() performs is an activating one. Say what the code does, and why the visible zoom is the cheaper half of that trade. `resolveWindowRevealMode` was described as the single authority for how far a run may go, but the packaged-build override sat in `main-window.ts` where only the reveal gate could see it — the dock rule read the un-overridden value, so a packaged build carrying a stray E2E variable got a window that may take focus and a dock that hides its tile. Fold `isPackaged` into the resolver and delete the module-local override, so both consumers read one answer. Generated-by: Claude Code
|
All three addressed at P2 — Wayland. Verified against the contract and the repo's own note; like you, I have no Wayland host, so nothing here is a reproduction either. I did not put const launchArgs = ['.', ...inactiveWindowPlatformArgs(), `--user-data-dir=${userDataDir}`];All three inactive launchers go through it and the old copy is deleted, so there is still exactly one Wayland detection. In P3 — first frame. The comment was the wrong half. The ordering is deliberate: the reveal P3 — one authority. Fixed rather than reworded. Verification: |
jackwener
left a comment
There was a problem hiding this comment.
Approving at exact head 926275f69679c3cf1e52b0c4075502cec5293765. No P0 or P1. All three findings from my earlier review are closed. Two new comments below, neither blocking.
The Wayland fix converged rather than forked, which was the thing to get right. The old helper returned ['.', '--ozone-platform=x11'] — bundling the entry point with the platform argument, which is precisely why two other launchers with their own arguments could not reuse it. Splitting that into inactiveWindowPlatformArgs() in scripts/fixture-env.mjs, returning only the extra argument, is what made one rule serve all three call sites. inactiveWindowElectronArgs is gone from the repo; apps/desktop/e2e/fixtures.ts:470, scripts/desktop-real-window-smoke.mjs:320 and scripts/fixture-window.mjs:150 all compose the same predicate. That fixed the shape that blocked reuse instead of copying the old helper to two more places.
The maximize comment now matches the code — active is the mode that gets a maximized first frame, and inactive cannot have both. And the packaged/dock split is gone: resolveWindowRevealMode takes isPackaged directly, main-window.ts no longer overrides locally, and the dock rule and the reveal gate read one value. That one was pre-existing and explicitly not charged to this PR, so fixing it was more than was asked.
P2 — this PR turns the repo's own real-window smoke red on macOS
scripts/desktop-real-window-smoke.mjs:509 asserts diagnostic?.dockVisible === true on darwin. That assertion is pre-existing and untouched here, but the behaviour it checks is deliberately inverted by this change: the script passes showWindow: true (:311-314), which now resolves to inactive, and the new dock rule hides the Dock for every mode except active.
Run on Electron 43.4.1, npm --workspace @maka/desktop run smoke:programmatic-window reports isVisible=true, isFocused=false, dockVisible=false — six checks pass and programmatic-dock-visible fails, exiting 1. Changing that one expectation to false gives 7/7 and exit 0.
The product behaviour is right; the acceptance command that is supposed to confirm it can no longer go green. This is invisible to hosted CI because the check is vacuous off darwin and the hosted runners are Linux, so it only surfaces for a maintainer running the smoke on a Mac. The PR description's own evidence table already records dockVisible: true → false as intended, which makes the stale assertion the one place that still expects the old value. The fix is to expect false there and keep the visible-and-unfocused checks as they are.
P3 — the new Wayland test proves the helper, not the wiring, and does not run in CI
scripts/fixture-env.test.mjs is falsifiable for what it covers: 4/4 normally, and making the Wayland branch return [] fails a case. But deleting the helper's use in apps/desktop/e2e/fixtures.ts still leaves it 4/4 — it tests the predicate, not that the three launchers are wired to it, which is the thing that actually regressed.
It also never executes. Root-level scripts/*.test.mjs files in this repo need an explicit entry point — test:product-release and astryx:surface-inventory:test each have one — and this file has none in package.json or in the workflows. Hosted Linux is Xvfb/X11 besides, so the Wayland branch cannot be reached indirectly. Worth giving it a script entry and adding a case that asserts the composed argument list at all three launch sites.
package and test are both green on this head.
I am not merging.
简体中文
在 926275f69679c3cf1e52b0c4075502cec5293765 上批准。没有 P0/P1。上一条评审里的三条发现全部收口。 下面两条新意见都不阻塞。
Wayland 那条修成了收敛而不是分叉,这正是最该做对的地方。 旧 helper 返回的是 ['.', '--ozone-platform=x11']——把入口点和平台参数捆在一起,而这恰恰就是另外两个各有自己参数的启动器没法复用它的原因。把它拆成 scripts/fixture-env.mjs 里的 inactiveWindowPlatformArgs()、只返回额外参数,才让一条规则能同时服务三个调用点。仓库里已经没有 inactiveWindowElectronArgs;三处都拼同一个判据。这是把「挡住复用的形状」改掉了,而不是把旧 helper 复制到另外两个地方。
maximize 的注释现在和代码一致了;打包/dock 那条分叉也没了——resolveWindowRevealMode 直接接收 isPackaged,main-window.ts 不再自己覆盖,dock 规则和 reveal 闸门读同一个值。那条本来是既有问题、我们明说不算这个 PR 的账,他顺手修了。
P2:这个 PR 会让仓库自带的真实窗口冒烟在 macOS 上变红。 scripts/desktop-real-window-smoke.mjs:509 断言 darwin 上 dockVisible === true。这行断言是既有的、本次没动,但它检查的行为被这次改动有意反转了:脚本传的 showWindow: true(:311-314)现在解析成 inactive,而新的 dock 规则对除 active 外的所有模式都隐藏 Dock。
在 Electron 43.4.1 上跑 npm --workspace @maka/desktop run smoke:programmatic-window,得到 isVisible=true、isFocused=false、dockVisible=false——6 项通过,programmatic-dock-visible 失败,exit 1;把那一处预期改成 false 就是 7/7、exit 0。
产品行为是对的,但用来确认它的验收命令已经不可能变绿。托管 CI 看不见,因为这条检查在非 darwin 上是空过的,而托管跑的是 Linux——所以只有维护者在 Mac 上跑冒烟时才会撞上。PR 描述自己的证据表里已经把 dockVisible: true → false 记为预期结果,这就使那行过时断言成了唯一还在期待旧值的地方。
P3:新增的 Wayland 测试只证明了 helper,没证明接线,而且根本不在 CI 里跑。 scripts/fixture-env.test.mjs 对它覆盖的部分是可证伪的(正常 4/4,把 Wayland 分支改成返回 [] 会挂一条)。但把 apps/desktop/e2e/fixtures.ts 里对 helper 的使用整个删掉,它仍然 4/4——它测的是那个判据,不是「三个启动器都接上了」,而后者才是真正出过问题的地方。
它也从来没被执行过。这个仓库里根目录的 scripts/*.test.mjs 需要显式的入口(test:product-release、astryx:surface-inventory:test 都各有一个),而这个文件在 package.json 和工作流里都没有。何况托管 Linux 是 Xvfb/X11,也无法间接触发 Wayland 分支。建议给它加一个脚本入口,并补一条断言三个启动点最终参数列表的用例。
这个 head 上 package 和 test 都是绿的。我不合并。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
Summary
MAKA_E2E_SHOW_WINDOWconflated two things: whether the Desktop window appears, and whether the app takes the foreground. Eight fixtures inapps/desktop/e2e/fixtures.tsset it, and every run on a CI Linux display sets it too — so a local suite run gave the Electron app a dock tile, activated it, and threw its window in front of whatever the developer was doing, over and over for the length of the run. None of those fixtures wants focus. They want the compositor (a hidden window is throttled to ~1fps under xvfb) and a real layout for geometry assertions.The flag now lifts the window's visibility and stops there. The single
startHiddenboolean becomes the run's reveal mode —hidden,inactive,active— resolved once inwindow-reveal.tsand consumed by both the reveal gate and the dock rule:hidden: unchanged. E2E captures paint the hidden window viapaintWhenInitiallyHidden.inactive: reveal withshowInactive(), stay an accessory app, and answer a focus request with a reveal and nothing more.maximize()reveals a hidden window and that reveal activates the app, so an inactive reveal runs first and leaves the maximize nothing to show.active: the product. Unchanged — a normal launch still gets its dock icon, its reveal, and its focus.The E2E harness carried the same conflation: the prompt-rail worker window is re-revealed before every test in the file with
window.show(), which activates the app regardless of what the main process decided. It reveals inactively now.The Linux CI path is untouched: those runs are
inactiverather thanhidden, so the window is on screen and the compositor runs at full rate exactly as before; only the macOS foreground behavior changes, and nobody is watching a headless X display.This deletes the
startHiddenconcept rather than adding a flag next to it — one authority for how far a run may go when it reveals its window. The packaged-build override now lives in that resolver too, rather than inmain-window.tswhere only the reveal gate could see it: a packaged build carrying a stray E2E variable used to get a window that may take focus and a dock that hides its tile, because the two consumers read different values.showInactive()is unsupported when Electron runs on native Wayland, so an inactive reveal there can fail to appear — exactly the throttling and geometry failures a visible window exists to avoid.scripts/fixture-window.mjsalready carried that rule and its remedy, but only its own launch used it. It moves next toisCiLinuxDisplayinscripts/fixture-env.mjs, returns only the extra arguments so each launcher keeps its own, and all three inactive launchers go through it. Hosted CI runs on an Xvfb X11 display and is unaffected either way.Verification
apps/desktop/src/main/__tests__/window-reveal-mode.test.ts(new, 15 cases): the mode rule, the dock rule, and the reveal gate. Every "inactive" case fails against the old code, which calledshow()/focus()and resolved the dock toicon; the packaged case pins the reveal gate and the dock rule to the same answer.scripts/fixture-env.test.mjs(new, 4 cases): the Wayland launch rule, including that it composes with a launcher's own arguments.npm run build,npm run format,npm run lint,npm exec -w @maka/desktop -- tsctypecheck (all four projects): pass.npm exec -w @maka/desktop -- playwright test e2e/prompt-rail.spec.ts: 9 passed.lsappinfo front) 130 times during that same spec run, before and after:And a single fixture window launched directly, before and after, reporting from inside the main process:
AI use
Select exactly one:
Tool(s) and scope: Claude Code drafted the change and this description; verified by the author.
Checklist
Does this PR entail a change in behavior?