fix(desktop): restore the macOS window after closing it - #8802
Conversation
macOS UI verificationVerified the packaged
The screenshot below was captured from the restored window itself, so it does not include the desktop, other apps, a pairing QR code, or a LAN IP. |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not explored to full depth (tool budget reached): PR #8802 changes packages/desktop-shell/src-tauri/src/mai...: empirical macOS verification of the fullscreen close→hide→reopen sequence (Linux runner, no macOS available) — the finding above is code-trace evidence only.; PR #8802 changes packages/desktop-shell/src-tauri/src/mai...: could not run the macOS fullscreen hide/show cycle on a real Mac from this Linux runner (finding 2's Space behavior is inferred from tao source, not observed).. Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
已审查。 建议见行内评论。 未探索到全部深度（达到工具调用预算）：PR #8802 changes packages/desktop-shell/src-tauri/src/mai...：empirical macOS verification of the fullscreen close→hide→reopen sequence (Linux runner, no macOS available) — the finding above is code-trace evidence only.；PR #8802 changes packages/desktop-shell/src-tauri/src/mai...：could not run the macOS fullscreen hide/show cycle on a real Mac from this Linux runner (finding 2's Space behavior is inferred from tao source, not observed).。 未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
The current head keeps the bounded Dock-reopen focus guard. This pass changed no code and corrected the PR body so the no-focus-steal claim applies only to Dock reopen; explicit Finder/launch activation retains the existing focus behavior. All threads are resolved; automatic review is still in progress. 中文摘要当前 head 保留有限的 Dock 重开聚焦守卫。本轮未改代码，只修正 PR body：不抢焦点仅适用于 Dock reopen；Finder/显式启动仍保持现有聚焦行为。所有 thread 已解决，自动 review 仍在运行。 |
|
All prior threads are resolved at this head, and my independent verification is complete. Cleanup done. Finalizing the review summary. Review: no blockersScope check — one file ( Verification at head
Code trace of the changed paths:
One thing I noticed (non-blocking): the Prior review threads: the three ci-bot suggestions are either fixed at this head (the focus-steal gate — which I verified) or answered with rationale I checked against the source (CI matrix scope, Verdict: LGTM from my side — no blocking findings. 中文说明单文件改动，范围与标题/描述一致。由于 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): Context: PR #8802 (QwenLM/qwen-code) fixes the macOS Desk...: live macOS verification of Reopen-on-Cmd+Tab behavior (no macOS machine; relies on verified tauri/tao source mapping + standard AppKit semantics).; Context: PR #8802 (QwenLM/qwen-code) fixes the macOS Desk...: live macOS verification of hide() on a native-fullscreen window (outcome asserted from AppKit knowledge, not executed)..
中文说明
未探索到全部深度（达到工具调用预算）：Context: PR #8802 (QwenLM/qwen-code) fixes the macOS Desk...：live macOS verification of Reopen-on-Cmd+Tab behavior (no macOS machine; relies on verified tauri/tao source mapping + standard AppKit semantics).；Context: PR #8802 (QwenLM/qwen-code) fixes the macOS Desk...：live macOS verification of hide() on a native-fullscreen window (outcome asserted from AppKit knowledge, not executed).。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
Closeout for the current head
中文摘要
|
|
@qwen-code /review |
|
Qwen Code review request accepted. Review is queued in workflow run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): Context: PR #8802 (QwenLM/qwen-code) changes macOS Deskto...: compile-check of the crate on non-macOS targets (no rustc/cargo or cargo registry on this runner) — specifically whether the write-only static on Windows/Linux …; Context: PR #8802 (QwenLM/qwen-code) changes macOS Deskto...: verification of tao's cross-thread hide() dispatch behavior (no vendored tao source available) — this uncertainty is carried in Finding 1's confidence.; Context: PR #8802 (QwenLM/qwen-code) changes macOS Deskto...: live macOS verification of the fullscreen close→reopen sequence (Linux runner; code-trace evidence only)..
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
未探索到全部深度（达到工具调用预算）：Context: PR #8802 (QwenLM/qwen-code) changes macOS Deskto...：compile-check of the crate on non-macOS targets (no rustc/cargo or cargo registry on this runner) — specifically whether the write-only static on Windows/Linux …；Context: PR #8802 (QwenLM/qwen-code) changes macOS Deskto...：verification of tao's cross-thread hide() dispatch behavior (no vendored tao source available) — this uncertainty is carried in Finding 1's confidence.；Context: PR #8802 (QwenLM/qwen-code) changes macOS Deskto...：live macOS verification of the fullscreen close→reopen sequence (Linux runner; code-trace evidence only).。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.21.8)
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Not reviewed: This PR adds a macOS window lifecycle feature: closing th... — pointed at diff lines it never opened: it made tool calls, but none of them read the diff.
[Critical] R3-1 (still stands): A Dock/Finder reopen arriving during the 2-second fullscreen-hide-pending window cannot cancel the pending hide. During the 2s window the window is still visible and unminimized, so the Reopen guard evaluates false, focus_main_window never runs, and the timer hides the window right after the user explicitly reopened it. This contradicts the PR body's promise that 'a reopen or second launch cancels the pending hide'. The existing comment at line 149 covers this; no new inline comment is posted to avoid duplication.
中文说明
未审查：This PR adds a macOS window lifecycle feature: closing th...——它被指向 diff 的行却从未打开：有工具调用，但没有一次读取 diff。
[Critical] R3-1 (still stands): A Dock/Finder reopen arriving during the 2-second fullscreen-hide-pending window cannot cancel the pending hide. During the 2s window the window is still visible and unminimized, so the Reopen guard evaluates false, focus_main_window never runs, and the timer hides the window right after the user explicitly reopened it. This contradicts the PR body's promise that 'a reopen or second launch cancels the pending hide'. The existing comment at line 149 covers this; no new inline comment is posted to avoid duplication.
— glm-5.2 via Qwen Code /review (v0.21.8)
|
Qwen Code review timed out. Qwen review timed out after 10800 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): This PR adds macOS window lifecycle handling: on CloseReq...: 无（已完成 ~20 次调用，低于 35 次的上限——所有检查均已执行，未跳过任何内容）。; This PR adds macOS window lifecycle handling: on CloseReq...: (none — all checks completed within the available budget).
[Critical] R3-1 (still stands): A Dock/Finder reopen arriving during the 2-second fullscreen-hide-pending window cannot cancel the pending hide. While the window is exiting fullscreen it is still visible and unminimized, so the Reopen guard evaluates false (has_visible_windows == true, main visible and unminimized), focus_main_window never runs, FULLSCREEN_HIDE_PENDING stays true, and the timer hides the window right after the user explicitly reopened it. This contradicts the PR body's promise that 'a reopen or second launch cancels the pending hide'. The suggestion is to add || FULLSCREEN_HIDE_PENDING.load(Ordering::Relaxed) to the Reopen guard. (No new inline comment posted — the existing comment at line 149 already covers this.)
中文说明
未探索到全部深度（达到工具调用预算）：This PR adds macOS window lifecycle handling: on CloseReq...：无（已完成 ~20 次调用，低于 35 次的上限——所有检查均已执行，未跳过任何内容）。；This PR adds macOS window lifecycle handling: on CloseReq...：(none — all checks completed within the available budget)。
[Critical] R3-1 (still stands): A Dock/Finder reopen arriving during the 2-second fullscreen-hide-pending window cannot cancel the pending hide. While the window is exiting fullscreen it is still visible and unminimized, so the Reopen guard evaluates false (has_visible_windows == true, main visible and unminimized), focus_main_window never runs, FULLSCREEN_HIDE_PENDING stays true, and the timer hides the window right after the user explicitly reopened it. This contradicts the PR body's promise that 'a reopen or second launch cancels the pending hide'. The suggestion is to add || FULLSCREEN_HIDE_PENDING.load(Ordering::Relaxed) to the Reopen guard. (No new inline comment posted — the existing comment at line 149 already covers this.)
— deepseek-v4-flash via Qwen Code /review (v0.21.8)
|
Changed: fixed Dock/Finder reopen during the pending fullscreen hide and added one focused state regression check at head Verified: Desktop Rust tests passed 27/27, Clippy passed with warnings denied, and 中文摘要已修复 fullscreen-hide pending 期间 Dock/Finder reopen 仍被隐藏的问题，并补一条聚焦回归检查。Rust 27/27、Clippy 与 diff 检查通过。直接 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — the desktop-shell Rust suite could not be compiled on this Linux runner (missing webkit2gtk/gtk/dbus system dependencies, no sudo); the macOS-gated surface is not compiled or tested by any per-PR CI leg (see the R4-2 inline finding). Not explored to full depth (tool budget reached): PR #8802 (QwenLM/qwen-code) changes the macOS Desktop win...: could not compile or run the crate for the macOS target (Linux runner, no Apple toolchain), so the race finding rests on reading Tauri's threading model rather …. Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — the desktop-shell Rust suite could not be compiled on this Linux runner (missing webkit2gtk/gtk/dbus system dependencies, no sudo); the macOS-gated surface is not compiled or tested by any per-PR CI leg (see the R4-2 inline finding)。 未探索到全部深度（达到工具调用预算）：PR #8802 (QwenLM/qwen-code) changes the macOS Desktop win...：could not compile or run the crate for the macOS target (Linux runner, no Apple toolchain), so the race finding rests on reading Tauri's threading model rather …。 未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
Closeout for this feedback pass:
中文摘要两条新增测试硬化意见已回复并 resolve，未扩大代码。PR 可合并且产品检查通过；直接 Focused(true) 生命周期语义仍保留给维护者决定。 |
|
Closed the remaining refocus finding at Verified on macOS with the locked Tao lifecycle sequence: programmatic fullscreen exit emitted no focus event, while focus-away-and-return emitted 中文摘要已在 macOS 原生探针基于锁定的 Tao 事件序列确认：程序化退全屏不产生 focus 事件，切走再返回依次产生 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not explored to full depth (tool budget reached): PR #8802 (QwenLM/qwen-code, review round 5): on macOS, cl...: did not run cargo check for the Windows/Linux targets (gating verified by inspection only; macOS compilation is not possible from this runner).; PR #8802 (QwenLM/qwen-code, review round 5): on macOS, cl...: could not execute the macOS scenarios empirically — both findings rest on code tracing plus AppKit/Tauri focus-event semantics inferred from the PR's own behavi….
中文说明
已审查。 建议见行内评论。 未探索到全部深度（达到工具调用预算）：PR #8802 (QwenLM/qwen-code, review round 5): on macOS, cl...：did not run cargo check for the Windows/Linux targets (gating verified by inspection only; macOS compilation is not possible from this runner).；PR #8802 (QwenLM/qwen-code, review round 5): on macOS, cl...：could not execute the macOS scenarios empirically — both findings rest on code tracing plus AppKit/Tauri focus-event semantics inferred from the PR's own behavi…。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
Changed: merged the latest main branch without rebasing and preserved both fullscreen-hide lifecycle state and the incoming workspace-directory state. Verified: cargo test passed 36/36. cargo fmt --check still reports pre-existing formatting drift outside this conflict resolution. Intentionally not changed: the round-six suggestion to move the process-wide main-window flag into managed state; no production regression was demonstrated. Pending: exact-head Web Shell smoke and automatic review are running. 中文摘要已普通合并 latest main 并机械保留双方行为；Rust 测试 36/36。第 6 轮状态重构建议已说明不在本 PR 引入。CI/自动评审运行中。 |
|
@qwen-code-review-bot pls review and approve if no blockers |
qwen-code-review-bot
left a comment
qwen-code-review-bot
left a comment
There was a problem hiding this comment.
LGTM — verified at ee72554.
Both Critical findings are closed at this head; I checked the code, not just the replies:
- Reopen/refocus during the fullscreen hide-pending window (R3-1 and the follow-up serialization Critical):
should_restore_main_windownow returns true whileFULLSCREEN_HIDE_PENDINGis set,focus_main_window/Focused(true)clear the flag, and the timer's swap+hide is dispatched viarun_on_main_threadso it serializes with reopen events on the main loop. I traced both orderings (reopen-before-timer and timer-before-reopen) and neither can leave the window hidden after a reopen. - The three Minors (atomic ordering,
set_fullscreen(false)failure leaving the flag armed, missing cfg gating) are all fixed inf13ce1f06as described — Release/Acquire/AcqRel at the sync points, error path clears the flag and returns, static is#[cfg(target_os = "macos")].
Local verification on macOS arm64 at this exact head: cargo test 36/36 pass, including the 8-case should_restore_main_window truth table. Desktop Shell CI legs (ubuntu/windows) green; no failing checks.
Remaining open threads are Suggestion-level only (dedicated macOS CI leg, moving the flag into managed ApplicationState), with the author's dispositions on record — reasonable as follow-ups.
Note: this records my approval only. The outstanding changes-requested states predate the fix commits (9487fa958, ff91c5fcb, f13ce1f06) and need to be superseded by those reviewers before merge can unblock.
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 47 passed · 0 failed · 47 total 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：47 通过 · 0 失败 · 47 总计 Verification reportVerification report — PR #8802
|
| Evidence cell | Oracle | Result |
|---|---|---|
| API surface vs pinned crates (tauri 2.11.5, tao 0.35.3, single-instance 2.4.3) | 24 scripted checks on the downloaded crate sources | 24/24 — every symbol the diff calls exists with the exact signature used (01-api-surface-24-of-24.png) |
| New truth-table test vs the verbatim-extracted function | rustc --test + mutation matrix |
control green; 3/3 clause mutants killed (02-truth-table-mutation-matrix.png) |
| Fullscreen-hide flag/timer logic, PR shape vs generation-counter shape | threaded 1:1 port with real timers | 6/6; the PR shape exhibits the stale-timer race (03-race-model-pr-vs-fix.png) |
| CI gates vs the cfg layout | scripted workflow/source checks | 11/11 (04-ci-gates-no-macos-runner.png) |
| What non-macOS CI actually checks about cfg(macos) items | rustc demonstration | 2/2 (05-cfg-strip-demo.png) |
Base→head static delta on non-macOS targets: zero. The #[cfg(not(target_os = "macos"))] CloseRequested arm is byte-identical to base, and every added item (static, Focused(true) arm, macos CloseRequested arm, Reopen arm, predicate, and the line inside focus_main_window) is #[cfg(target_os = "macos")]. "Windows and Linux keep their existing close behavior" therefore holds by cfg construction, and the ubuntu/windows desktop_shell jobs compile exactly the unchanged surface.
The base-bug mechanism is confirmed against tauri-plugin-single-instance 2.4.3 source: a second launch notifies the singleton over its socket and then std::process::exit(0) (platform_impl/macos.rs:28) — matching the PR's "Before" evidence (launch returns 0, no window) — and the singleton's callback runs focus_main_window, which the fix now has a live (hidden) window to restore. tao 0.35.3's applicationShouldHandleReopen returns has_visible_windows verbatim (app_delegate.rs:215), so AppKit adds no competing default behavior and RunEvent::Reopen.has_visible_windows is exactly the AppKit parameter.
Findings
F1 — moderate: a stale fullscreen-hide timer can consume a later close's pending flag, hiding the window mid-transition
The pending state is a single boolean shared across close episodes (main.rs:32), and a timer thread from an earlier, cancelled close still runs FULLSCREEN_HIDE_PENDING.swap(false) at its deadline (main.rs:154-159). Interleaving (times relative to the first close):
- t=0 — close pre-release: fix ci #1 while fullscreen:
prevent_close,set_fullscreen(false), pending=true, timer T1 spawned (fires t=2 s) —main.rs:150-159. - t≈0.5 s — any cancel path clears pending: Dock reopen →
Reopenguard →focus_main_window(main.rs:719), refocus →Focused(true)(main.rs:135), or second launch → single-instance callback →focus_main_window. - t≈1.5 s — user re-fullscreens and closes again: pending is false, so the handler proceeds;
is_fullscreen()true → pending=true, timer T2 spawned (fires t=3.5 s). - t=2 s — T1 fires on the main thread:
swap(false)returns true (the flag set by close Where is the config saved? #2) →win.hide()— 0.5 s after close Where is the config saved? #2's fullscreen exit began, instead of 2 s after it.
The 2 s delay exists precisely to wait out the AppKit fullscreen-exit transition (comment main.rs:153; PR body: hiding during fullscreen "leaves the fullscreen Space active"). A stale timer defeats that wait, so the exact symptom this PR fixes can recur, and the user's most recent close loses its grace period. Commit f13ce1f069 ("close fullscreen-hide races") closed the races it named; this is a sibling of the same class.
Evidence: harness/race-model.rs ports the flag/timer logic 1:1 (window/AppHandle calls → counters) and drives the interleaving with real threads at 1/5 time scale: PR shape hides 100 ms after close #2 where the mandated delay is 400 ms; a generation-counter shape hides on schedule at 400 ms (03-race-model-pr-vs-fix.png, pr-race hides=1 delay_after_close2_ms=100 vs fix-race ... =400). The model demonstrates the interleaving under real atomic/thread semantics; the trace above is from reading the shipped code. Caveat, stated plainly: neither cell compiled the crate (impossible off-macOS here), so this is a demonstrated interleaving of the diff's logic shape, not an on-device reproduction.
Blast radius: no crash, no data loss; requires cancel + re-fullscreen + close inside one 2 s window (fast human input, trivial for scripted input); worst case is recurrence of the pre-PR hidden-but-fullscreen symptom. The non-fullscreen sibling is safe: close #2 windowed hides immediately and T1's later swap sees false.
Minimal suggested fix (measured in the model only, not compiled into the crate)
Replace the boolean with a generation counter so a timer can only act on the close that spawned it: static GEN: AtomicU64; the fullscreen-close path stores gen = GEN.fetch_add(1)+1 and the timer hides only if GEN.load() == gen; every cancel path (Focused(true), focus_main_window) does GEN.fetch_add(1). In the same harness the fix shape passes all six cells including the race (fix-race hides once, on schedule). The suite that would pin this on-device is a macOS test that performs close → cancel → re-fullscreen → close and asserts the hide lands ≥2 s after the second close; the existing truth-table test cannot see it (it pins only the pure predicate).
F2 — moderate: the PR's entire body is macOS-gated code that PR CI never compiles, and its new test never runs in CI
- The
desktop_shelljob matrix is['ubuntu-22.04', 'windows-2022'](.github/workflows/ci.yml, job at ~1074) — no macOS runner. - On those runners,
#[cfg(target_os = "macos")]items are parsed but never type-checked. Demonstrated with rustc 1.97.1: a cfg(macos) item containinglet x: u32 = "string"; nonexistent_method_call();compiles cleanly on Linux, while a syntax error under the same cfg fails (05-cfg-strip-demo.png). - The only macOS compile of this crate is
desktop-release.yml, which isworkflow_dispatch-only; ci.yml's own comment says desktop-release is "otherwise the sole place this crate is built". - The new test
should_restore_main_window_truth_table(main.rs:890-913) is itself#[cfg(target_os = "macos")], so no CI job can ever run it. The PR body's "Rust suite and Clippy pass" reflects the author's local macOS run only.
The gap is pre-existing — the job was designed that way — but this PR places all 87 changed lines inside it: a typo'd method name in the macos block would pass PR CI and first surface at a manual release build. Suggested fix: add a macos-* runner to the desktop_shell matrix; cargo test there compiles the macos code and runs the new test in one step.
Not covered
- Reviewer Test Plan steps 1–6: each requires the packaged macOS app and a window server (launch PID, red-button close, fullscreen close, Finder/Dock reopen, focus-steal with Local Control, Cmd+Q). Not executable in this Linux container — reported as environmentally out of reach, not skipped by choice; the author's macOS evidence in the PR body stands unverified by this lane.
- macOS-target compilation of the crate: impossible on Linux (no Apple SDK). Mitigated by the 24-check API-surface oracle against the exact pinned crate sources, which covers every symbol the diff calls but is not a whole-crate type check of the macos handlers.
- Real-crate
cargo test/ clippy: not run (a Linux tauri build needs WebKit/GTK system deps and would not compile the macos code anyway). The vacuity proof ran against a verbatim extraction (region sha256dbc82f69d6f21e85973bd5f36723ee3b482ec8a081938b450a7698ab0997ea0b, only cfg attribute lines stripped), the closest executable proxy. - Author's tao focus-event probe (programmatic fullscreen exit emits no focus event on tao 0.35.3): accepted as author evidence, not reproduced.
- Per-commit attribution: checkout is depth-2 (8 commits in metadata, 1 locally reachable); only the aggregate
HEAD^1..HEADdiff was verified. - Local Control transport, Windows/Linux behavior: unchanged by cfg construction; the diff contains no non-macOS code path.
Methodology
Environment: CI verify container (node:22-bookworm, Linux x86_64), merge-ref checkout (HEAD f0191c9405, base e60d182241, head ee72554d0e). Rust 1.97.1 installed via rustup under /__w/_temp; pinned crate sources (tauri 2.11.5, tao 0.35.3, tauri-plugin-single-instance 2.4.3) downloaded from static.crates.io. Harnesses live in harness/ (01-api-surface.mjs, 02-truth-table.mjs, 03-race-model.mjs, 04-ci-gates.mjs, 05-cfg-strip-demo.mjs), raw logs in logs/, evidence PNGs in evidence/. 02 extracts the new static/function/test verbatim from main.rs by marker-anchored line extraction; 03 is a 1:1 threaded port of the flag/timer logic with window/AppHandle calls replaced by counters — it demonstrates interleavings of the shipped logic's shape, not execution of the crate. All 47 assertions are scripted comparisons that ran; expected-red cells (killed mutants, the race cell) are encoded as passing expectations, so fail=0 means no unexpected outcome.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not explored to full depth (tool budget reached): Context: PR #8802 (QwenLM/qwen-code) changes the macOS De...: did not run cargo check / cargo test on the crate — the only new code is macOS-gated and cannot be type-checked on this Linux runner (Linux would compile only…; Context: PR #8802 (QwenLM/qwen-code) changes the macOS De...: could not run cargo check --target aarch64-apple-darwin (macOS std/target not available on this Linux runner), so compile-correctness of the macOS arms is ver…; Context: PR #8802 (QwenLM/qwen-code) changes the macOS De...: could not empirically confirm whether tao emits Focused(true) during macOS fullscreen exit (finding 1's trigger)..
中文说明
已审查。 建议见行内评论。 未探索到全部深度（达到工具调用预算）：Context: PR #8802 (QwenLM/qwen-code) changes the macOS De...：did not run cargo check / cargo test on the crate — the only new code is macOS-gated and cannot be type-checked on this Linux runner (Linux would compile only…；Context: PR #8802 (QwenLM/qwen-code) changes the macOS De...：could not run cargo check --target aarch64-apple-darwin (macOS std/target not available on this Linux runner), so compile-correctness of the macOS arms is ver…；Context: PR #8802 (QwenLM/qwen-code) changes the macOS De...：could not empirically confirm whether tao emits Focused(true) during macOS fullscreen exit (finding 1's trigger).。
— qwen3.8-max via Qwen Code /review (v0.21.8)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
@qwen-code-review-bot pls review this pr and approve it if no blockers |
|
Already reviewed and approved at this exact head (
So: no blockers, two approvals in. What still holds the PR in |
ae37717
|
Closeout for Changed
Verified
Intentionally not changed
Pending
中文摘要已普通合并最新 |
|
@qwen-code /triage |
|
@qwen-code /verify |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 56 passed · 0 failed · 56 total 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：56 通过 · 0 失败 · 56 总计 Verification reportVerification report (round 2) — PR #8802
|
| # | Finding (round 1) | Severity | Status at head ae377172 |
|---|---|---|---|
| F1 | Stale fullscreen-hide timer consumes a later close's pending flag, hiding the window mid-transition | moderate | fixed — delta commit ae377172 ("isolate delayed fullscreen hides") implements the generation-counter shape round 1 suggested. The exact F1 interleaving was re-driven on both shapes: old shape still premature at 100 ms after close#2 (control cell A1), new shape hides once on schedule at 402 ms (A2); 240-run stress: 0 violations new vs 40/240 old (cells G1/G2). The new test pins the generation check (mutant M1 killed by assert!(!take_pending_fullscreen_hide(first_hide))). See "Race model A/B". |
| F2 | Entire PR body is cfg(macos) code PR CI never compiles; new test never runs in CI | moderate | stands — all premises re-measured at the new head and unchanged: matrix still ['ubuntu-22.04', 'windows-2022'], desktop-release still workflow_dispatch-only, 118/118 added lines macOS-gated, the replacement test fullscreen_hide_lifecycle_state is itself cfg(macos). Pre-existing gap; the PR grew from 87 to 118 lines inside it. Mitigated for THIS head by the new darwin type-check (cell 06 table) — that evidence is per-head, not a CI gate, and the test still never runs in CI. |
Central claim and A/B evidence
Central claim (unchanged from round 1): on macOS the red close button hides (not destroys) the main window; Dock/Finder/second launch restores it; a visible-window Dock reopen does not steal focus; a fullscreen close exits fullscreen first and hides only after the AppKit transition. The behavioral half (real windows on a real Mac) remains out of reach in this Linux lane — the author's packaged-app evidence stands unverified here, as in round 1. What this lane executes, re-measured at the new head:
| Evidence cell | Oracle | Result |
|---|---|---|
Race model A/B: old boolean shape vs new generation shape, F1 interleaving + neighbors + stress (03-race-model.stdout.log, 01-race-ab-f1-interleaving-flips.png) |
threaded 1:1 port of the flag/timer logic, real threads/timers, main-thread executor modeling run_on_main_thread serialization |
13/13: A1 old shape premature (100 ms after close#2, mandated 400); A2 new shape on schedule (402 ms, hides once, flag clean); baselines/cancel/re-close/reopen/failure-path cells green; stress 240 runs: new 0 violations vs old 40; ns-window boundary probe clean |
New test vacuity: verbatim extraction + mutation matrix (02-lifecycle-test.stdout.log, 02b-m3-fixture.stdout.log, 02-mutation-matrix-and-m3-fixture.png) |
rustc --test on the marker-anchored extraction (region sha256 aba7ade8… with cfg attribute lines stripped) + 4 single-point mutants + pinning fixture |
9/9: control green (1 passed); M1 (gen check deleted) and M4 (predicate mutated) killed by their intended assertions; M2/M3 survived as predicted and are classified below (F3/F4); the M3 pinning fixture is green unmutated and red under M3 |
Darwin check (new this round): cargo check --target aarch64-apple-darwin --tests on the real tree + liveness mutations in a scratch copy (06-darwin-check.stdout.log, 06-darwin-liveness.log, 05-darwin-cargo-check-and-liveness.png) |
real rustc 1.97.1 with the aarch64-apple-darwin std target; planted mutations in a scratch copy | 5/5: real tree exit 0 (whole crate incl. all cfg(macos) items and the cfg(macos) test target type-check); baseline scratch green; broken cfg(macos) test caught at main.rs:943; typo'd method in cfg(macos) production code (swapp) caught at main.rs:740 — the exact F2 risk scenario; scratch restored byte-identical |
API surface vs pinned crates (tauri 2.11.5, tao 0.35.3, tauri-plugin-single-instance 2.4.3 per Cargo.lock) (01-api-surface.stdout.log, 03-api-surface-20-of-20.png) |
20 scripted checks on the downloaded crate sources | 20/20 — every symbol the diff calls exists with the exact signature used, including this round's new call AppHandle::run_on_main_thread; tao's application_should_handle_reopen returns has_visible_windows verbatim; second launch notifies then exit(0); the single-instance callback runs inside tauri::async_runtime::spawn (tokio thread — the one cross-thread cancel source) |
F2 premises re-measured (04-ci-gates.stdout.log, 05-cfg-strip.stdout.log, 04-f2-premises-hold-at-new-head.png) |
scripted workflow/diff/source checks + rustc cfg-strip demo | 9/9 — see F2 row above; cfg-strip demo re-run on rustc 1.97.1: type-invalid code under cfg(macos) compiles cleanly on Linux, syntax errors still fail, no-cfg control fails |
Base→head delta on non-macOS targets remains zero: every added line is macOS-gated except one #[cfg(not(target_os = "macos"))] attribute placed above the byte-identical pre-existing CloseRequested arm (checks 04/06 of 04-ci-gates). The base moved since round 1 (e60d1822 → e4658678, one web-shell commit #8696 that touches no desktop-shell files); the aggregate diff against the new base is still exactly packages/desktop-shell/src-tauri/src/main.rs.
Race model A/B detail (F1)
Schedule (1/5 time scale; mandated delay 400 ms ≙ 2 s): close#1 fullscreen at t=0 → cancel (Dock reopen/refocus/second launch) at t=100 ms → re-fullscreen + close#2 at t=300 ms.
| build | hide count | delay after close#2 | verdict cell |
|---|---|---|---|
previous head shape (boolean flag; timer does swap(false)) |
1 | 100 ms — stale timer T1 consumes close#2's flag mid-transition | A1 (control, expected race) — identical to round 1's measurement |
current head (FULLSCREEN_HIDE_GENERATION + take_pending_fullscreen_hide(gen) + run_on_main_thread) |
1 | 402 ms — on schedule; flag clean at quiesce | A2 |
| stress, 240 randomized cancel/reclose interleavings per shape | — | new: 0 invariant violations (hide count, ownership, schedule, no stuck flag); old: 40/240 premature/wrong hides | G1 / G2 |
Neighbor cells on the new shape: baseline fullscreen close hides on schedule (B2); cancel stops the hide (C2); a second close during the pending transition early-returns and the single hide stays on close#1's schedule (D1); a Dock reopen mid-transition evaluates should_restore_main_window(true, false)=true via the pending flag and cancels the hide (E1), while the same reopen with nothing pending evaluates false — no focus steal (E2); a failed set_fullscreen(false) clears the flag, leaves no timer, and a following windowed close hides immediately (F1 cell).
Boundary probe (H1): the single-instance plugin delivers its callback on a tokio worker thread (verified in platform_impl/macos.rs), so cancel_pending_fullscreen_hide can race the main-thread take_pending_fullscreen_hide. The race window is the gap between GEN.load and pending.swap inside take() — nanoseconds in production. Widening it to 5 ms over 400 iterations made the cancel land inside the window in 400/400 runs, and the final state stayed clean in every case: no stuck flag, no double hide. A hit means the hide wins against a same-instant cancel — one more reopen restores the window. Classified as a bounded, benign boundary, not a finding.
Mutation matrix on the new test (vacuity)
Extracted verbatim: statics (lines 31–34), helper fns (731–746), fullscreen_hide_lifecycle_state (908–944); only #[cfg(target_os = "macos")] attribute lines stripped (region sha256 aba7ade8bda82198b791ba2e4225bdb9bdfbb3e824e060d1813f682e6acaa58c).
| mutant | change | result | classification |
|---|---|---|---|
| control | none | green, 1 passed |
positive control |
| M1 | take(): generation check → true |
killed by assertion failed: !take_pending_fullscreen_hide(first_hide) (the intended assertion) |
generation check is load-bearing and pinned |
| M2 | cancel(): delete the GENERATION.fetch_add |
survived | dead clause under the current writer set (F4) |
| M3 | take(): pending.swap(false) → pending.load() |
survived | coverage gap — nothing pins that take() clears the flag (F3) |
| M4 | should_restore(): predicate → !has_visible && needs_restore |
killed by truth-table case has_visible=true, needs_restore=false, pending=true (expected-vs-actual named in the panic) |
positive control for the predicate half |
Findings
F2 (carried over, moderate) — the PR's whole body is still macOS-gated code that PR CI never compiles; the new test never runs in CI
Re-measured at the new head, all premises stand: desktop_shell matrix ['ubuntu-22.04', 'windows-2022'] (ci.yml:1084); on those runners cfg(macos) items are parsed but never type-checked (cfg-strip demo 3/3); desktop-release remains workflow_dispatch-only; the replacement test fullscreen_hide_lifecycle_state is itself cfg(macos). The PR now places 118 added lines inside the gap (87 in round 1).
What changed this round: the darwin check above proves the CURRENT head compiles for macOS at the type level, including the test target — so the round-1 concrete risk ("a typo'd method name in the macos block would pass PR CI and first surface at a manual release build") is empirically retired for this head. The structural gap remains: the next commit to this file gets no macOS compilation from PR CI, and the test has never executed in any CI job ("Rust suite and Clippy pass" in the PR body is the author's local macOS run). Suggested fix (unchanged from round 1): add a macos-* runner to the desktop_shell matrix; cargo test there compiles the macos code and runs the new test in one step.
F3 (new, Suggestion) — no test pins that take_pending_fullscreen_hide clears the flag
Mutant M3 (swap(false) → load()) survives the new test: all assertions stay green because the test calls take(second_hide) exactly once. The cleared flag matters in production: if a future change stops clearing it, FULLSCREEN_HIDE_PENDING stays true after every fullscreen close, and every later CloseRequested early-returns at the pending check (main.rs:144) — the red button would never hide the window again until a cancel path runs. The code today is correct (the swap is there); only the assertion is missing. Measured pinning fixture — green on the unmutated region, red under M3 naming assertion failed: !take_pending_fullscreen_hide(second_hide):
Fixture (validated in 02b, not applied to the tree)
assert!(take_pending_fullscreen_hide(second_hide));
assert!(!take_pending_fullscreen_hide(second_hide)); // take() must clear the flagF4 (new, informational) — cancel_pending_fullscreen_hide's generation bump is a dead clause under the current writer set
Mutant M2 (delete GENERATION.fetch_add from cancel) survives. Enumeration of writers shows why: the only code that sets pending=true is the fullscreen-close path, and it bumps the generation immediately before, capturing the new value for its own timer — so any timer holding an older generation already sees GEN != captured whenever a later close happened; and when no later close happened, cancel's store(false) makes the stale timer's swap return false regardless. The bump in cancel therefore decides no outcome any other clause doesn't already decide. This is defense-in-depth, not a defect: it protects against a future writer that sets the flag without bumping the generation. No action recommended; recorded so the surviving mutant is classified (dead clause) rather than left ambiguous.
Not covered
- Reviewer Test Plan steps 1–6: each needs the packaged macOS app and a window server (launch PID, red-button close, fullscreen close, Finder/Dock reopen, focus-steal with Local Control, Cmd+Q). Structural in this lane, not incidental — Linux container, no macOS has ever booted here (round 1 and round 2 observe the same environment). The author's packaged-app evidence and tao focus-event probe stand as author evidence, unverified by this lane. This round reproduced the logic shape of steps 3/5's cancel semantics in the race model (cells E1/E2, A2), not the AppKit behavior itself.
- Running the new test: the darwin check type-checks the test target; executing it needs a macOS host. The verbatim-extraction run on Linux covers the same assertions but is not the crate's own test binary.
- Whole-crate darwin link/build:
cargo checkdoes not link; the Apple SDK is unavailable here. Disclosed workaround: the C/asm build steps of three leaf dependencies (network-interface,ring,objc2-exception-helper) were stubbed with empty objects — cross-compiling C needs an Apple toolchain. Their build scripts emit nocargo:rustc-cfg/rustc-env(verified in theiroutputfiles), so the Rust type-check is unaffected; the stubs are in the artifact dir (/__w/_temp/pr8802-fake-cc/cc). - Per-commit attribution: depth-2 checkout —
git rev-list HEAD^1..HEAD^2yields 1 commit, the metadata lists 10 (including two merges of origin/main); the delta commit's parents (1b805ce6,ee72554d) are unreachable locally, so the delta's own diff could not be isolated. Verified: the aggregateHEAD^1..HEADdiff at the new head. - Host-target
cargo check/cargo test: needs WebKit/GTK dev headers this container does not install; it would compile no macos code anyway, and the PR's non-macos surface is byte-identical to base (check 06) — the ubuntudesktop_shelljob covers it in PR CI. - Windows/Linux close behavior: unchanged by cfg construction; the diff contains no non-macOS code path (checks 04/06 of the CI-gates harness).
Methodology
Environment: CI verify container (node:22-bookworm, Linux x86_64), merge-ref checkout (merge d4b1d18e58, base e46586782c, head ae37717233); Rust 1.97.1 installed via rustup under /__w/_temp/verify-rust with the aarch64-apple-darwin std target; pinned crate sources downloaded from static.crates.io. The API-surface oracle is a fresh derivation for the current diff (20 checks covering every symbol it calls, including this round's new run_on_main_thread); round 1 ran a 24-check variant of the same oracle against the same pinned crate versions, which are unchanged (the PR touches no lockfile). Harnesses live in harness/ (01-api-surface.mjs, 02-lifecycle-test.mjs, 02b-m3-fixture.mjs, race-model.rs, 04-ci-gates.mjs, 05-cfg-strip-demo.mjs, 06-darwin-check.mjs), raw logs in logs/, evidence PNGs in evidence/ (each captured live via scripts/verify-capture.mjs). race-model.rs ports the flag/timer logic 1:1 — statics, take/cancel/should_restore, the timer thread, and run_on_main_thread as a serial main-thread executor; CloseRequested/Focused/Reopen bodies are posted to that executor as tauri delivers them; window/AppHandle calls become counters with timestamps. It demonstrates interleavings of the shipped logic's shape under real atomic/thread semantics — it does not execute the crate. 02/02b extract the production region + test verbatim by marker-anchored line ranges and drive rustc --test plus single-point mutants; the M3 pinning fixture was validated in scratch, never applied to the tree. 06 ran cargo check --target aarch64-apple-darwin --tests on the real tree (exit 0) and proved the gate live by planting one mutation in the cfg(macos) test and one in cfg(macos) production code inside a scratch copy, catching both, and restoring the copy byte-identical to HEAD. The repo working tree was never modified (git status clean throughout). All 56 assertions are scripted comparisons that executed; expected-red cells (A1's premature hide, G2's old-shape violations, killed mutants, liveness mutations) are encoded as passing expectations, so fail=0 means no unexpected outcome.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
@qwen-code-review-bot pls review this pr and approve it if no blockers |
qwen-code-review-bot
left a comment
qwen-code-review-bot
left a comment
There was a problem hiding this comment.
Re-review after the new push. The only PR-specific change since my last approval is fix(desktop): isolate delayed fullscreen hides — verified:
- The generation counter isolates each delayed-hide cycle:
cancel_pending_fullscreen_hidebumps the generation, so a stale timer from a cancelled cycle can no longer consume a newer cycle's pending flag;take_pending_fullscreen_hiderequires both a generation match and the pending swap, and all flag access is serialized on the main thread viarun_on_main_thread. - The rest of the delta is the merge from main (already reviewed in their own PRs).
Local verification at this exact head on macOS arm64: cargo test 36/36 pass, including the extended fullscreen_hide_lifecycle_state isolation sequence. Desktop Shell CI legs green, no failing checks. No blockers — re-approving.
@/tmp/pr8802-dismiss-wenshao.txt
|
Done — the PR is now fully unblocked. What happened since my last comment:
Current state: |
|
Sandboxed verification: Skipped because the PR is not open for verification (state=MERGED, draft=false). 中文 — 判定：
|
|
Released in v0.21.9. |
6 participants
What this PR does
On macOS, closing the main Desktop window now hides it instead of destroying it. Reopening from the Dock, Finder, or another launch restores and focuses the same window when it is hidden or minimized. A Dock reopen does not steal focus from Local Control when the main window is already visible; an explicit Finder or launch request keeps the existing behavior of focusing the main window. Native-fullscreen close first exits fullscreen, then hides the window after the AppKit transition completes.
Why it's needed
The macOS event loop remains alive after its last window closes. Before this change, the red close button destroyed the main window while the single-instance process kept running. A later launch was forwarded to that process, but there was no window left to show, so the new launch returned immediately and looked like a crash. Hiding a native-fullscreen window directly also leaves the fullscreen Space active. These are Desktop window-lifecycle issues surfaced while validating Local Control.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: after closing the main window, the Desktop PID remained alive with no windows; opening the same bundle returned exit code 0 but did not show a window.
After: packaged-app verification observed PID 48383 and window 80618 transition from visible → hidden → visible again with the same PID and window ID. Cmd+Q then removed both the process and window. No crash report was generated.
Fullscreen follow-up: the previous head reproduced the hidden-window/still-fullscreen state. A native macOS lifecycle probe using the locked Tao 0.35.3 sequence confirmed that programmatic fullscreen exit emits resize/move events but no focus event, while leaving the app and returning emits
Focused(false)thenFocused(true). The current fix therefore cancels the pending hide on actual refocus without canceling the intended fullscreen close. The Rust suite and Clippy pass, but the packaged fullscreen cycle has not yet been re-run.Tested on
Environment (optional)
macOS arm64, unsigned release
.appbundle built from this branch.Risk & Scope
Linked Issues
Related to #8092. Follow-up to #8727.
中文说明
这个 PR 做了什么
在 macOS 上，关闭 Desktop 主窗口时不再销毁窗口，而是将它隐藏。从 Dock、Finder 或再次启动应用时，会在主窗口隐藏或最小化时恢复并聚焦同一个窗口。主窗口已经可见时，Dock reopen 不会从 Local Control 抢走焦点；显式从 Finder 或再次启动仍保留聚焦主窗口的现有行为。原生全屏状态下关闭窗口时，会先退出全屏，再等待 AppKit 转场完成后隐藏。
为什么需要
macOS 在最后一个窗口关闭后仍会保留应用事件循环。修改前，红色关闭按钮会销毁主窗口，但单实例进程仍在运行。后续启动会被转发到这个旧进程，可此时已经没有窗口可显示，因此新启动会立即返回，看起来就像闪退。直接隐藏原生全屏窗口还会让全屏 Space 保持激活。这些都是在验证 Local Control 时暴露的 Desktop 窗口生命周期问题。
Reviewer 测试计划
如何验证
Before / After 证据
修改前：关闭主窗口后，Desktop PID 仍然存活但已经没有窗口；再次打开同一个 bundle 会以退出码 0 返回，但不会显示窗口。
修改后：对打包应用的验证观察到 PID 48383 和窗口 80618 按“可见 → 隐藏 → 再次可见”变化，并且 PID 和窗口 ID 均保持不变。随后 Cmd+Q 会正常移除进程和窗口，系统也没有生成崩溃报告。
全屏补充：上一版 head 已复现窗口隐藏后仍保留全屏状态。基于锁定版本 Tao 0.35.3 事件序列的 macOS 原生探针确认：程序化退出全屏只产生 resize/move 事件，不产生 focus 事件；切离应用再返回会依次产生
Focused(false)和Focused(true)。因此当前修复只在真实重新聚焦时取消待隐藏，不会误取消预期的全屏关闭。Rust 测试与 Clippy 已通过，但尚未重新执行打包应用的全屏循环验证。测试平台
环境
macOS arm64，从该分支构建的无签名 release
.appbundle。风险与范围
关联事项
关联 #8092，是 #8727 的后续修复。