feat(desktop): add app icon switcher - #3563
Closed
ARE404 wants to merge 15 commits into
Closed
Conversation
Settings → 外观 gains an 应用图标 picker: the dock tile on macOS, the window/taskbar icon on Windows and Linux. `appearance.appIcon` is a closed enum normalized fail-closed the same way `appearance.palette` is — the renderer names an id, never a path, so a hand-edited settings file can only ever select artwork that ships with the build. Applying it hangs off the client-settings effect that already owns the OS-level side effects of a settings write (keep-awake, bot channels), so a switch lands live and is deduped against the file watcher echoing the same write back. Startup keeps setting the default mark synchronously — awaiting the settings store there would reintroduce the generic Electron rocket that PR-GRAY-CARD-LIFT-0 removed — and the persisted choice lands a tick later from the same effect. New windows are born with the chosen icon, which is what Windows and Linux need since they draw it per window. Two icons ship: the existing brand mark and a grayscale variant derived from it. The variant is placeholder artwork — the mechanism is the point, and adding an id is one entry in APP_ICONS plus one PNG. The picker gets its thumbnails from the main process (ids plus 128px data URLs, computed once) rather than importing the 1024px masters into the renderer bundle.
Three things the picker needed once it shipped more than two icons. **A set to choose from.** 14 recolours of one geometric mark, in four families. The artwork carries macOS's 824/1024 margin itself, because the OS scales the canvas to the dock tile without adding any — a full-bleed tile sits 24% wider than every neighbour. **Groups.** 16 tiles in one grid gives the eye nowhere to start, the same problem the palette picker solved with `PALETTE_GROUPS`; this follows that shape, down to the per-group `role="group"` and its label id. Membership is a renderer concern: the main process reports what artwork loaded, and anything no group claims falls into the trailing group rather than disappearing. **Import.** `appearance.appIcon` was a closed enum precisely so a settings file could not name a path, and imported art must not weaken that. So a custom icon is referenced as `custom:<32 hex>`: the main process generates the id, the id IS the whole file name, and both core's normalization and the store re-check its shape before anything reaches `join`. The renderer never learns a path; the file dialog runs in the main process. Imported files are re-encoded rather than copied — decode, centre-crop to square, scale to 1024, write PNG — so the stored shape is the only one the rest of the app reasons about, and nothing rides along inside the original. Non-square art is cropped rather than letterboxed: silently adding a margin would reframe art the user already framed. Deleting the artwork under the current choice hands the dock back to the brand mark first, so it is never left pointing at a file that no longer exists.
… removal Review findings on the icon picker, all reachable at runtime. **A settings value could become a path outside the asset root.** `normalizeSettings` runs when settings are READ from disk; `SettingsStore.update` merges and writes without it, so the object the effects act on carries whatever a patch put there. `appIconPath` then cast that string to a shipped id, and `../../../../tmp/owned` resolved to `/tmp/owned.png` on its way to Electron's decoder. Every runtime ingress now coerces through `toAppIconChoice`, and the declared type is no longer treated as a guarantee. **Removal was sequenced by the renderer, in the order its comment denied.** It deleted the file and then persisted the reset, so a failed write left the setting pointing at artwork that was gone. Selection and artwork are one pair of state, so the operation that can break them moved to a single main-process owner: it resets the selection, applies it, and only then deletes. `app-icon-ipc.ts` takes its Electron-touching capabilities as parameters, which is what lets the removal contract be tested without booting a browser process. **Dropping the artwork from a package was silent.** `assertPackagedResources` now requires `assets/icon.png` and every `APP_ICONS` file, with a test that the check actually fails when one goes missing — Electron reports an unreadable file as an empty image, so nothing downstream would have noticed. Also drops `desktopAssetPath`, whose only production caller became `appIconPath`, and the trailing blank line that failed `git diff --check`.
Three more review findings, all about trusting something that had not been checked yet. **A byte cap does not bound a bitmap.** `stat()` capped the compressed form, so a few hundred KB of PNG declaring 30000×30000 reached the platform decoder and allocated the full image inside the main process. Reading the file also happened twice — once to size it, once to decode it — leaving a window where the path could be swapped in between. Both close the same way: read one capped snapshot, parse the header out of those bytes, reject the dimensions there, and decode that same buffer. The parser is pure and covers the fixture the reviewer asked for — a small file declaring a huge bitmap. **The dialog offered formats the decoder does not guarantee.** `nativeImage` guarantees PNG and JPEG on every platform; TIFF and WebP were advertised anyway, so a Windows or Linux user could pick a file the app then refused. The picker now offers PNG and JPEG, and the header parser recognises exactly those two — the dialog filter and the validator can no longer drift apart. **A window could be born with an icon path that decodes to nothing.** A persisted custom id whose file was deleted resolves to a perfectly valid path; the dock fell back through `loadAppIcon`, but `BrowserWindow` got the raw path, which matters on Windows and Linux where the icon is per window. Window creation now walks the same fallback, via a pure helper so the missing-file case is covered without a browser process.
…ifier Self-review of the previous two commits. Three of these are mine. **Crop framed the wrong rectangle.** Validating dimensions from the header was right; using those same numbers as the crop geometry was not. A JPEG carrying an EXIF orientation comes out of the decoder rotated, so the pre-rotation rectangle frames the wrong part of the picture or falls outside it. The header decides admission; `getSize()` decides geometry. **A single `read()` may return early.** It is one syscall and can hand back fewer bytes than asked for. The truncated buffer still passed the header check — which only reads the first bytes — and then failed to decode, surfacing a valid file as an unreadable image. Now it reads to EOF through one handle, so the snapshot property that closes the swap window is kept while the result is actually whole. Growing in chunks also stops a 20 KB icon from allocating the whole 16 MB cap up front. **Deleting the selected icon left the picker showing it.** The main process owns the reset, and nothing notifies this surface of a client-settings write it did not make, so the tile stayed highlighted over artwork that was gone. The renderer now persists the selection the operation reports. **The packaged-resource verifier could no longer load without a build.** Importing `APP_ICONS` from `packages/core/dist` — build output, not in the repository — broke the existing `verify-packaged-app.test.mjs` in a clean checkout, and it runs in `check:release`. The required list now comes from the artwork directory; that the catalog matches the shipped ids is already the desktop suite's job, where `APP_ICONS` is walked against the same files. Also drops the `as never` in the IPC harness, which was letting the stub drift from the real dependency shape unnoticed.
**The new packaged-resource requirement broke upgrade verification.** The Windows upgrade lifecycle verifies the previously released installer under `artifactContract: 'legacy-baseline'`, and every one of those files is new to this branch — `assets/` was not packaged at all before it, so the check would fail an artifact that was correct when it shipped, before the upgrade was even attempted. It now rides on `requireAppIconCatalog`, threaded from `requiresCurrentContract` exactly as `requireDisclaimer` already is, with the current contract left strict. Note the gate covers `assets/icon.png` as well as the catalog: the whole directory postdates every previous release. **A click could land inside the removal window.** Removal reads the current selection in the main process before deleting, and only the remove button was disabled — so a selection made during that read could persist the very icon being deleted. Every card is fenced while a removal is in flight. **And the renderer no longer writes the selection back.** `SettingsSurface` already subscribes to `settings:clientChanged`, and the effects emit it for this write, so the reset propagates on its own; the second write only added a way to stamp a stale value over a newer choice. My earlier reasoning that nothing notified the surface was wrong — I had found `settings:externalChanged` and stopped looking.
The icon picker's import and remove controls added the first Button to appearance-settings-page.tsx, and the generated inventory tracks which Astryx components each surface uses. Regenerated with astryx:surface-inventory:write.
`CustomAppIconImportResult` stayed behind in the store when the handlers moved to app-icon-ipc.ts, which declares its own. Knip flags it, and that check only ran once the stale surface inventory stopped failing ahead of it.
The 外观 story threw on mount: the picker asks the bridge for its previews as soon as it opens, and the fixture had no `iconPreviews`. Calling a method the fixture does not define is a synchronous TypeError, so the effect's own `.catch()` never saw it and the story rendered nothing at all. This has been broken since the picker landed on this branch — the visual smoke sits behind fifteen steps that a stale generated file was failing ahead of, so nothing reached it until now. The fixture spans one shipped icon per group plus an imported one, so the smoke covers the group headings and the remove affordance rather than an empty picker. `importIcon` answers `cancelled`, the one outcome that needs no dialog.
Removal read the selection and then wrote unconditionally. Between those two calls another surface can select a different icon, and the reset would then stamp `default` over a choice the user had just made. The renderer's busy flag cannot help: the window is on the far side of the IPC boundary. `settingsStore.updateIf` evaluates its predicate and writes on one queue, which is the only place the pair can be made atomic. The reset now applies only while the current choice is still the icon being removed; when it is not, the newer selection stands and is what the caller is told. The file is deleted either way — it is no longer in use, which is what the caller asked for — and the OS surface is only re-applied when the selection actually moved. This also removes a call: `updateIf` reports the settings whether or not it applied, so the separate `get()` is gone and `SettingsStore` is narrowed to the one seam this needs.
The compare-and-set covered the settings write and nothing after it. Applying the icon yields, and a selection arriving in that gap — from another renderer, or already queued on the generic settings channel — would be left pointing at artwork this sequence was about to delete. Guarding that window would only move it, so selection joins the seam instead of racing it: `app:selectIcon` persists and applies the choice, and select, import and remove now run one at a time under a single owner. The renderer no longer writes `appearance.appIcon` through the generic settings channel at all, which is what let a selection interleave in the first place. Selection can now also be refused: the generic channel would happily persist an id whose file is gone, while this seam checks the artwork is still there. The regression test issues a selection mid-removal without awaiting it and pins the whole sequence — the selection runs after, not inside, and is refused because the artwork it names no longer exists.
It was removed as unused — at the time its only caller was its own test. The permission overlay now resolves its icon through it on main, so the reason for deleting it no longer holds and the branch cannot compile without it.
The header sweep and its `check:asf-headers` gate landed upstream while this branch was open, so the eleven files it introduces predate the requirement. Generated with `npm run write:asf-headers`; no other content changed.
… on a dialog Two follow-ups from review, both about the seam owning what it claims to own. **The comment on the seam was not true as written.** It said the icon owner is the only thing that can refuse a choice whose artwork is gone, but `clientOwnedSettingsPatch` forwarded `appearance` as one object, so a well-formed `custom:<32hex>` could still reach the store through `settings:client:update` — from a second window, an older preload, or a direct `settings.update`. The id shape was still checked, so the failure was never a malformed value; it was a valid id landing between a removal's settings apply and its file deletion. `appearance` is now filtered field by field, the way `personalization` beside it already was, which makes the comment true rather than adding another compare-and-set. **The file dialog was holding the serialized owner.** It sits open for as long as the user looks at it, which blocked selection and removal in every other window on someone reading a file list. The dialog changes nothing — only the copy-into-place does — so it now runs before the queue, and only the write path is serialized. Covered by a test that selects while a dialog is deliberately left open.
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What changed
Testing
npm run build:renderernpm run typechecknpx playwright test e2e/settings.spec.ts --config e2e/playwright.config.ts --workers=1(4 passed)