Repository navigation
refactor(desktop): use NSH-native residency - #6723
Conversation
There was a problem hiding this comment.
I reviewed the 33-file change at e531c8c175c6bd6b05cd4ae1e724198f3b695d43 against Rust baseline 09ca38c23ae9d46214c2743e182fe018c6544429. I found one P2 issue, detailed inline: Linux can hide its last workbench without a usable restore entry.
The change replaces destruction/snapshot/recreation on close with one retained engine and window. I checked the Bootstrap-owned close/tray handlers, embedded icon access, plugin reload and removal of the old close protocol, GTK Host replacement, AppKit permission-window ownership, Win32 explicit Quit, native runner removals and dependency changes. Keeping live state avoids reconstructing arbitrary plugin presentation state on every reopen. The Linux fallback is nevertheless a separate requirement: the NSH tray implementation currently logs registration failures and does not restore main when its watcher disappears.
My local checks pass all 169 workbench JavaScript tests, including the nine desktop lifecycle cases and six reload cases. Two additional executions of the production nativeDesktop handler with controlled desktop adapters leave main hidden, plugins active and no exit request when the tray is absent or disappears. This is a controlled handler check, not a real GTK session.
I also read completed hosted logs from CI 42636733495: Ubuntu passes all six Desktop-shell tests, including retained engine/Host/secondary-window behavior; macOS passes 107 platform-library, 10 terminal-native, 22 Desktop (230.367 seconds) and two pixel tests (3.965 seconds). Both check out merge 20b02f607c0e909ecbd6c6b8ca61cc0793329785, whose parents and tree match this head merged with 09ca38c2. Those tests do not cover the missing-tray condition. The same run has since completed successfully, including Windows and all root shards. I read the Windows native log: 226 platform tests pass with one skipped, all 22 Desktop tests pass (350.319 seconds), and both pixel tests pass (1.582 seconds). Its explicit hidden-window Quit test also passes (0.391 seconds), and checkout 20b02f607c0e909ecbd6c6b8ca61cc0793329785 matches the merge identified above. These results do not exercise or resolve the missing-tray P2. Current Rust has since advanced to 12971e440e93af1501872ec8eaf6bd781360ba92 with separate pending CI.
The actual merge into 09ca38c2 succeeds. There is no database-schema change. This and #6725 are overlapping alternative implementations: merging their current heads produces conflicts in 13 files. I did not perform local native compilation, manual desktop interaction, populated historical-session recovery or packaged-app acceptance; author-reported checks remain the author's evidence.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
EnglishThanks for putting this together! #6725 just landed the initial single-engine residency refactor on Rather than closing this PR, could you rebase it onto current
简体中文感谢提交这份重构!#6725 刚刚先一步在 与其直接关闭这个 PR,希望你能基于最新
|
e531c8c to
b5f59bb
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed the complete 19-file change at b5f59bb2813b25606f68dd9549e1bdb499976ff0 against its base 853052bec2f7c31aaefcf0beceeb0c7ad504fbb5, separating inherited residency changes from this follow-up. The old no-tray hiding problem reported on e531c8c1 is fixed in this version: Linux closes through the no-tray exit path, and current registration refusal or watcher absence/loss restores a hidden main window. I found no further demonstrated P0–P2 issue in the inspected change.
Residency now belongs to the persistent Bootstrap rather than a replaceable contribution, with desktop/terminal guards and the existing asset reader supplying the icon. I checked owner cleanup, localized tray handlers, explicit non-Mac Quit and retained native Mac Quit, Linux close delegation and application-scoped shutdown, the generation-fenced tray callbacks, pixel-terminal desktop-command guards on both native platforms, and the added CI routes and regression tests.
Locally, all 159 existing workbench JavaScript tests pass under Node 24.18.1, four selected CI-routing tests pass, and Biome accepts nine changed JavaScript/TypeScript files. Eight controlled checks execute unchanged extracted Rust tray-registration functions against a minimal thread-local adapter; all pass. The same checks with the base's functions give six passes and two failures for refused/absent registration recovery. These extracted-function checks are not a GTK build or a real panel test.
I separately read the hosted logs for CI 42748068828. Ubuntu actually runs all seven desktop-shell tests under Xvfb/private D-Bus, including hidden reload/tray recovery and secondary-window Quit; all pass. Windows runs three desktop-shell tests, including native close → hide → terminal.exit, and 227 NSH tests with one skipped, including pixel-terminal window/tray isolation; those pass. However, the Windows job later fails desktop_steering_freezes_each_message_and_queue_is_one_shot at 72.254s (19 passes, one failure, two not run), and CLI slice 2 fails the copy/undo test at 69.592s (18 passes, one failure, 94 skipped). The unmodified 853052be base also fails that CLI test. The cause of the remaining desktop failure is not established here. Mac results are still pending, so this is not an overall successful CI result. The logs check out 3ce685651aed0736051bce66eaf45ee1309e1969, whose parents and tree match the actual base-plus-head merge.
The newer Rust baseline a75113d8b7d0cccf181a4d4214dab962e9124ac2 now has an actual content conflict in scripts/ci.test.mjs. No database-schema or wire-contract changes are introduced by this PR. I did not perform local native platform builds, manual tray/Alt+F4/IME acceptance, a populated business session, Host replacement, old-database upgrade, or packaged-product acceptance. The author's local results remain attributed to the author.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
b5f59bb to
90060f8
Compare
90060f8 to
c99b257
Compare
There was a problem hiding this comment.
I reviewed the complete 19-file author change at c99b257845d6221fca622733189cce414a824f05 against bb914bde38fc3365f9d3ae666b177ea44e54188f, including a range comparison with reviewed b5f59bb2. I found no new demonstrated P0–P2 defect in the inspected code. The previous Linux no-tray fallback and registration-loss fix remain in place; the new native focus repair still needs current-head native execution evidence.
The new change addresses an additional path in which a rejected Window.open root could subsequently be inserted as a normal terminal modal. Both native mounters now retain only live rejected root identities, prevent their Insert and ModalConfig paths from acquiring modal ownership, release a root that was inserted earlier in the same batch, and clear those identities on Delete and engine reload. I checked both mount orders, the existing detached-input focus admission, ordinary Dialog reuse after Delete/reload, terminal-active and inactive Windows cases, and the expanded Mac/Windows regression sources. The Bootstrap residency owner, no-tray shutdown and watcher-generation fences remain unchanged from the preceding reviewed fix. The CI changes preserve the newer execution-recovery routing while restoring the Linux Xvfb/private-D-Bus consumer.
On Node 24.18.1 with locked, patched dependencies, all 108 current workbench JavaScript tests and all 15 CI-planning/execution tests pass. Biome accepts the nine changed JavaScript/TypeScript files. These counts are for this changed baseline; the previous head's 159-test result is not a current-head test result. I did not locally compile or execute the Mac, Windows or GTK regressions, so the author's 227-test Windows result and before/after native focus reproduction remain attributed to the author.
The original merge into bb914bde succeeded. Rust has since advanced through #6740 and #6733 to 1cf7ea313384010c3423cae34ba7c0f27ea64e34; an actual merge of this unchanged head now conflicts in products/maka/workbench/app/nativeDesktop.ts. CI 42764405990 has completed with failure. I read the original logs: Windows compiled but its Computer Use selection had 0 tests and 97 skipped, then exited with code 4 before the later desktop suites; macOS and the CLI shard failed when agent.publishCheckpoint returned agent_history_checkpoint_unavailable after a cooperative stop. The VM duplicated-prefix assertion and encrypted replay also failed. The corresponding failures occur on the previous bb914bde base, so they do not by themselves establish a new #6723 regression. The native focus regression did not receive a passing execution result in these jobs. Newly merged #6740 changes those CI and recovery paths, but its PR and landed-base runs are still queued at this check. There are no schema or wire changes in #6723.
The already merged copy-key fix in #6731 is an inherited change. The old real-Host character-terminal keyboard test was removed by #6713; its absence does not prove the copy/undo/interruption path fixed. Test-restoration PR #6736 was closed unmerged at the user’s request: its migrated fixture failed during UI initialization before reaching Ctrl+C, so it provides no end-to-end success result. The real-Host keyboard acceptance gap remains outstanding. Manual tray/Alt+F4/IME behavior, retained real business sessions and other windows, Host replacement, old-database upgrades and packaged-product acceptance remain unverified here.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
M4n5ter
left a comment
There was a problem hiding this comment.
[P2] Prevent the deferred close from hiding the window after tray loss
There appears to be a race between the tray availability check here and the deferred close handler. main_close_request() sends ClosePress to the engine, and the resulting hide is applied later on the GTK thread.
If the watcher disappears in between, restore_when_tray_lost() sees the main window still visible and does nothing. The queued hide can then hide it after the tray is gone, leaving a running client without a tray entry to restore the window.
Could we ensure that a deferred hide cannot leave the window inaccessible after tray loss? A regression test could delay the close handler, process watcher disappearance while the window is still visible, then let the hide arrive and verify that the window remains reachable or the application exits.
This is based on a static control-flow review; I haven't reproduced this interleaving in a native GTK session.
Keep the main-window and tray lifecycle independent of the selected root UI, with native owner cleanup and terminal guards. Serve the existing tray icon through HostAssets without adding bootstrap assets. Give non-Mac Quit an explicit Ctrl+Q exit action while preserving the native Mac Quit role. Generated-by: Maka
Post a real WM_CLOSE, wait for the main window to become hidden, and then exercise the existing terminal.exit bridge with closeOnPress disabled. Generated-by: Maka
Route registered-tray close requests through NSH's native main-window policy, retain the no-tray exit path, and make Ctrl+Q application scoped. Remove windows before waiting for bounded Host detach. Notify the existing tray-loss callback for current registration refusals and initial watcher absence, without revealing healthy hidden reloads or honoring stale replies. Restore display-backed Linux CI and real GTK/HostChannel/secondary-window regressions, including delayed private D-Bus registration and recovery. Generated-by: Maka
Reject desktop shell commands before main and secondary window routing in the native mounters. Keep desktop layout, rendering, editing and terminal clipboard transport intact. Cover hidden native windows and the Windows tray click path without source assertions or fixed sleeps. Generated-by: Maka
Exercise the actual OpenWindow then Insert sequence. The terminal editor must retain focus when the native window is rejected, rather than having that modal root presented as a Dialog. Generated-by: Maka
Preserve upstream's new execution recovery routing while retaining this follow-up's display-backed Linux consumer. Generated-by: Maka
Window.open creates a Modal root and inserts it after OpenWindow. Ignoring only Desktop commands accidentally presented that root as a normal Dialog and stole terminal input focus. Retain rejected root identity for its mounted lifetime, suppress root insertion and modal configuration, and clear it on Delete or engine teardown. Exercise ordinary Dialog presentation after node deletion and reload. Generated-by: Maka
Reuse the native OpenWindow release sequence for roots inserted before the command. AppKit releases modal focus separately from unparenting; keep rejected content out of the terminal surface as well. Cover both insertion orders on Windows. Generated-by: Maka
Mount an auto-focused Input under each rejected Window.open root, issue its ordinary Focus command and paste into the retained main editor before deletion or reload. Exercise the same Input through the macOS JS fixture and report the Windows wait phase and input state on failure. Generated-by: Maka
Generated-by: Maka
Generated-by: Maka
c99b257 to
eddc70a
Compare
Keep confirmed tray loss armed through GTK visibility changes, and enter Quitting before hiding windows so recovery cannot resurrect shutdown. Preserve healthy hidden reload and verify two delayed closes with real GTK and private D-Bus. Generated-by: Maka
eddc70a to
f67c106
Compare
|
Maka (AI agent): addressed the deferred-close/tray-loss P2 in f67c106. The GTK owner now records confirmed tray loss and also repairs subsequent hide notifications, rather than checking visibility only at loss time. A new Quitting state is entered before hiding windows, so this recovery cannot resurrect explicit Quit. A real GTK/private-D-Bus regression gates close handling, releases the watcher while main is still visible, then issues two delayed hides; it checks both visible and mapped state and Quit. Test-only 5e08f9b fails at the stranded-window assertion (0.85s); the fix passes (0.28s). All seven Ubuntu desktop-shell tests passed in isolated processes, with healthy hidden reload and no-watcher reload/exit still covered. Workbench 108/108 and CI routing 15/15 also pass. Rebased onto merged #6740 and preserved #6738 tray settings through built-in Host SVG assets. No timeout or sleep was added. Hosted CI and manual Windows/macOS acceptance are not claimed as passing. |
|
Maka (AI agent): current-head Windows acceptance completed on the owner’s machine. MSVC product build passed; Windows NSH tests with js,terminal: 227 passed, one existing ignored, including the terminal isolation/focus regression. Real Alt+F4 hides without exiting; Explorer tray invoke restores the same window with General settings retained; artwork changes and client-storage persistence were checked. The human owner pressed Ctrl+Q and confirmed tray disappearance. I independently verified that the shell exited, launcher returned 0, and Host sessions/active calls both reached zero. The temporary service Host was then stopped; the original user instance was untouched. This is isolated native-UI acceptance, not a claim of macOS/IME/model-turn or packaged-product verification. The owner explicitly authorized merge after local Windows acceptance without waiting for hosted CI. |
|
@me2seeks I’d like to rethink the tests added by this PR from scratch: if none of them existed, what is the smallest set we would add to protect the actual behavior? The new desktop-close.test.mjs spends roughly two-thirds of the file building its own environment, including substitutes for signals, effects, cleanup, Host, and plugin loading. That is a substantial maintenance cost, and it makes it harder to distinguish product guarantees from behavior supplied by the test itself. Rather than preserving this structure and trimming individual assertions, I would start with these behaviors:
Please rebuild the added coverage around those guarantees, using the real runtime or existing fixtures where practical. Avoid a separate mock implementation of the lifecycle machinery, and keep implementation-detail assertions only where they are necessary to catch a specific regression. The real GTK/D-Bus test for the delayed-hide race is a good example of coverage worth retaining: it exercises the failure at the native boundary and checks that the window is actually visible and mapped. I’d prefer fewer tests with clear failure modes and less custom infrastructure. Any unique regression covered by the current tests should still have a trustworthy replacement before its test is removed. |
|
Maka (AI agent) follow-up: #6770 replaces the coverage rather than retaining and trimming the mock environment.
Verification: 27 Workbench Rust tests, 110 remaining Workbench Node tests and 15 CI-routing tests passed. Four temporary mutations independently failed at the close, cleanup, terminal-guard and Quit assertions, then returned green after restoration. The existing CI planner also selects the Root Rust suite for a fixture-only change; no new testing API or CI suite was added. The PR documents the unresolved native-macOS and native-Windows failures from #6723's final CI run rather than removing those tests or claiming that this test-only change fixes them. #6770 is open for review and has not been merged. |

Summary
Refine the single-engine residency already merged in #6725. Current head
f67c10672is based on0c4de9fe3, including #6740 and the configurable monochrome tray artwork from #6738.No Quick Tools feature or second residency engine is introduced.
Verification
The latest review race was reproduced before the fix with real GTK and a private StatusNotifierWatcher: two close events are handled without hiding, the watcher disappears while main is visible, then the engine is allowed to issue its delayed hides. The test fails with
a deferred hide stranded main after tray lossin 0.85s on the test-only commit5e08f9bd6. The fixed test passes in 0.28s, including both visible and mapped state and explicit Quit without resurrection. No timeout, sleep, skip or weakened assertion was added.Native verification used the existing Ubuntu Desktop checkout via SSH, Rust 1.99.1, GTK 4.22.4, Xvfb and private D-Bus sessions. Each test ran in its own process to preserve GTK thread ownership. The complete retained-engine suite also checks secondary-window Host messages, repeated reopen, healthy hidden reload, stale registration replies, pending/refused/lost registration, and no-watcher reload/exit.
Current-head Windows/MSVC product build passed. With both JS and terminal features enabled, the Windows NSH library suite passed 227 tests, with one existing ignored test; the pixel-terminal/native-window/tray/focus regression actually executed.
Windows interactive acceptance used a disposable Host Root and the public desktop launcher attached to a foreground service Host. The agent runner cannot start a detached candidate under its containing Windows job, so no detached-startup acceptance is claimed. Native Alt+F4 hid the main window without exiting its process; invoking its real Explorer tray icon restored the same window with Settings/General still selected. Artwork changed live and the selected line artwork was verified in client storage. The human owner then pressed Ctrl+Q and confirmed that the test tray icon disappeared: the shell process exited, its launcher returned 0, and Host status showed 0 sessions and 0 active calls. The separately served test Host was then stopped; the owner's original desktop was left running. Controls were observed through Computer Use and exercised through native UI Automation/keyboard input; this is not a claim of wholly manual acceptance.
Current-head hosted CI is 42785877128; local acceptance, not waiting for this run, is the owner's authorized merge criterion. macOS physical-desktop/IME acceptance, populated historical-session recovery, packaged-product acceptance and a complete cross-platform/typecheck run were not repeated. The fresh test Root had no model configured, so no model turn or composer/IME acceptance is claimed.
Public API
AppView::main_close_request(&self) -> glib::Propagationso an embedding window can honor NSH close policy.on_tray_lost(callback: impl Fn() + 'static)and extend its notification to confirmed initial watcher absence, current registration refusal and watcher loss during registration. Retired replies and explicit removal do not notify.No database schema, wire contract or source-breaking signature change is introduced.
AI use
Tool(s) and scope: Maka assisted with implementation, diagnosis, regression tests, verification and this description. Material commits carry
Generated-by: Maka.Checklist
Does this PR entail a change in behavior?