feat(desktop): replace native recovery dialogs - #4474
Conversation
Generated-by: OpenAI Codex
4632e00 to
6c1485c
Compare
jackwener
left a comment
There was a problem hiding this comment.
Approved at exact head 6c1485cdb012489eacecd5a19ea0d478c74ab23d at the explicit direction of M4n5ter. No technical review was performed as part of this action; M4n5ter requested the approval and accepts responsibility for subsequent handling.
Review notice: This approval was submitted by an automated review agent operated by WAWQAQ and is published at the direction of M4n5ter, who requested this action and is the human accountable for it.
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed 6c1485cd. The direction is right — a product-owned dialog that lives inside Chromium is testable and inspectable in ways a native message box never is, and keeping open/save panels and OS permission surfaces native is the correct line to draw. The screenshots look good.
Almost everything I found traces to one shape, so I've grouped the comments that way rather than filing them separately. In five places an authority already existed and a narrower or looser copy was written beside it — and in every one of them the correct value was already in hand at the call site. Two of the five are visible in this diff as your own local patches:
| Existing authority | The copy | Where the fact already was |
|---|---|---|
dialog.showMessageBox's window-all-closed behaviour |
patched at one call site, main.ts:97-101 |
you wrote it down in that comment |
| the same helper's parent-visibility requirement | patched at one call site, runtime-host-boot.ts:446 |
same |
maka-tokens.css + astryx-theme/maka.css |
36 lines of redeclared tokens | appearance.palette sits on the object main-window.ts:284 already reads, two fields from the .theme it uses |
desktopLocale |
a \p{Script=Han} sniff |
every call site had already resolved the locale |
the idle test at runtime-host-desktop-manager.ts:925 |
a looser one at :953 |
result.handshake?.activity is right there, and formatActivity already renders it |
You hit the dialog-helper seam twice and patched the call site both times. That is the signal to move the rule into the helper instead of writing a third patch — and doing that deletes the two patches you already wrote. Each of the five has the same property: fixing it at the owner removes code rather than adding it, which is what turns this from a net-addition PR into one that also retires something.
Findings: P1 ×3 (two in the dialog helper's contract, one in the idle test), P2 ×3, P3 ×4. The P1s are worth landing before merge; nothing here is a release blocker. Grading note: I considered the dialog auto-quit a release blocker at first and stepped it down — it makes startup recovery unusable and is a regression against the native dialogs it replaces, but there is no data loss and relaunching recovers.
Two items outside that root, both small:
runtime-host-boot.ts:450—if (decision === "recover" && reloadMainRenderer()) return; app.quit();. WhenreloadMainRenderer()returns false (window or webContents already destroyed, shutdown signal aborted,reload()throws) the app exits silently, while the dialog copy promises "recovers the interface without restarting Maka". This PR also removes the lastapp.relaunch()in the Desktop app, so there is no longer a degraded restart to fall back to. Either say so before quitting, or relaunch. P2.- The summary says Runtime Host actions "no longer relaunch the whole Desktop process". On
mainthe Runtime Host paths never calledapp.relaunch()— the only call was the renderer one this PR replaces. Worth correcting so the real change (the silent-replace policy) gets read on its own terms. P3.
One thing I went looking for and did not find, recorded so you don't have to wonder. I expected a parser or injection gap in the HTML construction and there isn't one. Every call-site-controlled value (title, message, detail, button labels) lands in a text node — <title> RCDATA, <h1>, .message, .detail — and escapeHtml's five characters are complete for those contexts; nothing reaches an unquoted attribute, a URL attribute, <style>, or a JS string. The unescaped interpolations are module constants, the messageBoxType allow-list, the nonce, and validButtonId-checked integers. I also confirmed empirically on the Electron version pinned here that the meta CSP is enforced under the opaque data: origin: an un-nonced inline script and an inline onerror= handler are both blocked while the nonced script runs. So a forged maka-dialog://response/<n> is not reachable, and no one can turn Cancel into Replace. The one residual is P3-shaped and noted inline.
Evidence boundary: I read the nine production files on this head, the dialog call sites and their position in the top-level startup sequence, main-window.ts, app-quit-coordinator.ts, and the host-side idle predicate in packages/runtime-host/src/server/host-kernel.ts, and diffed the token values against maka-tokens.css and astryx-theme/maka.css. I ran two standalone Electron probes for the auto-quit and CSP behaviour and extracted the built-in window-all-closed listener from the pinned Electron binary. I did not run this repo's suites or E2E, did not build the app, and did not verify the Windows or Linux behaviour on those platforms — the non-darwin reasoning is from reading runtime-host-boot.ts:1836. The four screenshots are hosted on a commit outside this branch, so I could not check them against the code.
Also, unrelated to the change: Release Windows check / package is red on qualify-released-cli-state-root.test.mjs:195, which touches none of these 14 files and looks pre-existing.
AI-assisted review: drafted with Maka; I verified the startup ordering, the token drift, the font-stack and wordmark duplication, and the two idle predicates against the branch source myself, and ran the Electron probes on my own machine.
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
|
Also addressed the two non-inline review-summary notes:
After synchronizing current |
Astro-Han
left a comment
There was a problem hiding this comment.
Approving. I read the follow-ups against the review summary notes and they hold: a failed in-place Renderer reload now stays in recovery and prompts again rather than reporting success, and the retry reloads the same BrowserWindow instead of building a second authority beside it.
Nothing further from me on the change itself.
One thing that isn't yours: the test job's Guard the protocol compatibility epoch step went red at 16:41 because #4460 merged and took epoch 88, and this branch is still on 88. 8319e4d2 hasn't moved it, so the guard will fail again on this run. Bumping to 89 clears it — though #4379 and #4184 are both sitting on 89 too, so it's worth checking main again right before merge rather than picking a number now.
Generated-by: Codex
…overy-dialogs # Conflicts: # apps/desktop/src/main/__tests__/native-diagnostic-dialog.test.ts # apps/desktop/src/main/native-diagnostic-dialog.ts # apps/desktop/src/main/runtime-host-boot.ts
…eness' into feat/maka-browser-recovery-dialogs # Conflicts: # apps/desktop/src/main/runtime-host-desktop-manager.ts
…eness' into feat/maka-browser-recovery-dialogs # Conflicts: # apps/desktop/src/main/runtime-host-desktop-manager.ts
…eness' into feat/maka-browser-recovery-dialogs
English
Summary
BrowserWindow, including workspace repair, Runtime Host recovery and replacement, client-settings confirmation, Renderer recovery, fatal startup, and development second-instance diagnostics.quitAndInstallbehavior remains unchanged because installing a new binary requires an application restart.Screenshots
Verification
npm run build:testnpm run format:checknpm run lintnpm run check:asf-headers, Renderer architecture check, and the Runtime Host protocol-epoch guardERR_OSSL_EVP_NO_DEFAULT_DIGESTunder Electron Node 24.18.1); no affected test failed.AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex — implementation, tests, iterative parallel review, visual verification, screenshots, and PR preparation.
Checklist
Does this PR entail a change in behavior?
中文
概要
BrowserWindow,覆盖工作区修复、Runtime Host 恢复与替换、客户端设置确认、Renderer 恢复、启动失败和开发环境重复实例诊断。quitAndInstall保持不变,因为安装新二进制确实需要重启应用。截图
验证
npm run build:testnpm run format:checknpm run lintnpm run check:asf-headers、Renderer architecture check 与 Runtime Host protocol epoch guardERR_OSSL_EVP_NO_DEFAULT_DIGEST);受影响测试均未失败。AI 使用
选择一项(权威勾选状态见英文版):
工具及范围:OpenAI Codex — 实现、测试、循环并行审查、视觉验证、截图与 PR 整理。
检查清单
此 PR 是否改变行为?