Repository navigation
Conversation
Generated-by: Codex Signed-off-by: Dante <duanjl.china@gmail.com>
Generated-by: Codex
me2seeks
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.
Summary
Updates the Windows taskbar app icon: a new windows-taskbar-icon.ts persists the selected artwork as a content-addressed ICO file (Explorer may read the relaunch icon after process exit, so not a temp file), and encodePngAsIco hand-rolls the minimal PNG-in-ICO directory entry to avoid a native image-conversion dependency for one resource. The encoder layout checks out: ICONDIR (reserved=0, type=1, count=1) plus one 16-byte ICONDIRENTRY with width/height 0 (=256px), true colour, 32bpp, correct byte length and image offset — matches the ICO spec Windows' property store consumes. WINDOWS_APP_USER_MODEL_ID is documented to stay aligned with electron-builder's appId and the installed shortcut. package and test lanes are green, and the rollback harness (verify-windows-installer-rollback.mjs) is updated to cover the new file.
Findings
- [P3]
WINDOWS_APP_USER_MODEL_ID = 'com.maka.desktop'is a second hard-coded copy of the appId that electron-builder owns; if the builder config's appId ever changes, the taskbar icon silently binds to the old identity with no build-time check. A tiny consistency assertion againstelectron-builder.config.mjsin the existing test would pin it.
Verdict
merge-ready — minimal dependency-free encoder with rollback coverage; only the duplicated appId authority nit.
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
No P0–P2 issue was found at 7ef20b515aed3f44afe857cb0c385e0d8344ba6e. The icon update preserves the existing window HICON and additionally sets the Windows AppUserModel icon resource. It persists a content-addressed PNG-in-ICO file under userData, applies it before the new window is revealed, and updates existing windows when the selected artwork or theme changes. The encoder directory layout and appId agree with the current builder configuration. Electron's setAppDetails contract and Microsoft's relaunch icon resource contract support this property-store boundary.
I also checked the follow-up rollback-harness change. The installer already permits exit 103 when a populated new tree cannot be moved aside, retaining that launchable tree and the complete backup. Requiring a failed-upgrade sibling in every such case was too narrow. The harness still checks the installed product version, backup marker and recovery note, then reruns the installer and checks the recovered version, uninstall registration and removal of recovery residue. This does not turn a failed install into an unconditional success.
All 60 selected tests passed: 16 icon/settings tests and 44 Windows harness tests. Seven workspace packages and Desktop main rebuilt, and Biome passed all four changed TypeScript files. A separate Linux Electron probe used real NativeImage decoding/resizing with captured window methods to check four artwork choices, two recipients, 256px ICO payloads, stable cache reuse, missing-artwork fallback and the non-Windows guard. This is not a Windows Explorer or pin/unpin test, and no native Windows installer run was performed locally.
[P3] The prior duplicated-appId finding remains. The runtime constant matches electron-builder.config.mjs today, but the existing test compares it only with another value derived from itself. Add a builder/runtime consistency assertion to catch future identity drift.
The exact head is unchanged, GitHub reports MERGEABLE, automatic merging with main bacb1caed is clean, and both exact-head test and Windows package runs succeeded. No Runtime Host protocol epoch is changed.
| import type { AppDetailsOptions, BaseWindow, NativeImage } from 'electron'; | ||
|
|
||
| /** Must stay aligned with electron-builder's appId and installed shortcut. */ | ||
| export const WINDOWS_APP_USER_MODEL_ID = 'com.maka.desktop'; |
There was a problem hiding this comment.
[P3] This currently matches electron-builder's appId, but the test only compares windowsTaskbarAppDetails().appId with this same constant. It would stay green if the builder identity changed independently. Add an assertion against the builder configuration so the installed shortcut and per-window AppUserModel identity cannot silently drift.
Summary
Fixes #5199
Verification
node --test apps/desktop/dist/main/__tests__/windows-taskbar-icon.test.js apps/desktop/dist/main/__tests__/app-icon.test.js apps/desktop/dist/main/__tests__/client-settings-effects.test.js(16 passed)biome linton the four changed TypeScript filesnode scripts/asf-license-headers.mjs checkgit diff --checkAI use
Select exactly one:
Tool(s) and scope: OpenAI Codex traced the Windows icon surfaces, implemented the change, and added the regression tests.
Checklist
Does this PR entail a change in behavior?