Skip to content

fix(desktop): update Windows taskbar app icon - #5202

Open
Dante-dan wants to merge 2 commits into
apache:mainfrom
Dante-dan:fix/5199-windows-taskbar-icon
Open

Dante-dan wants to merge 2 commits into
apache:mainfrom
Dante-dan:fix/5199-windows-taskbar-icon

Conversation

@Dante-dan

Copy link
Copy Markdown
Contributor

Summary

  • keep the existing window HICON update and also set Windows taskbar AppUserModel details
  • persist the selected or fallback artwork as a content-addressed 256px ICO under app-owned user data so Explorer has a stable relaunch icon resource
  • apply the same taskbar metadata before a new window is shown and on later icon or theme changes

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 lint on the four changed TypeScript files
  • node scripts/asf-license-headers.mjs check
  • git diff --check

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex traced the Windows icon surfaces, implemented the change, and added the regression tests.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

Generated-by: Codex
Signed-off-by: Dante <duanjl.china@gmail.com>
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 11, 2026

@me2seeks me2seeks left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR 5202 Review

结论

APPROVE
Windows taskbar 图标通过 per-window setAppDetails + content-addressed ICO 持久化解决,appId 与 electron-builder 的 com.maka.desktop 一致,实现与测试都扎实。

@me2seeks me2seeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. [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 against electron-builder.config.mjs in the existing test would pin it.

Verdict

merge-ready — minimal dependency-free encoder with rollback coverage; only the duplicated appId authority nit.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(desktop): Windows taskbar icon does not follow app icon selection at the default installation path

3 participants