feat(desktop): let the user choose or import the app icon - #3431
Conversation
6d6c9c7 to
cd6178b
Compare
ARE404
changed the title
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head cd6178b255441496cd13052b5a3deaf9072e1bcd.
The problem definition is sound: users need a persistent app-icon choice, including imported artwork, and the patch correctly separates shipped assets from app-owned custom files. The broad mechanism is also reasonable: persist an opaque icon ID, resolve files only in the main process, update current OS surfaces, and package the shipped masters as extra resources.
However, the current implementation does not yet preserve those boundaries end to end. I found one main-process path escape and four correctness/robustness issues below. In particular, the generic settings IPC accepts an arbitrary runtime string, and appIconPath() then casts it to a shipped icon ID; a value such as ../../../../tmp/owned resolves to /tmp/owned.png and is handed to Electron's native decoder. The delete flow is also ordered opposite to its comment, so a failed settings write can leave the selected file deleted while the setting still points at it.
Additional release gap: assertPackagedResources() in scripts/verify-packaged-app.mjs does not require assets/icon.png or the assets/app-icons/* catalog. The source tests prove that repository files and path construction are correct, but they do not fail if a future packaging change drops the runtime payload. Please make the packaged-resource verifier require the default icon and every APP_ICONS asset. I locally produced the unsigned .app up to the mandatory signing step and confirmed this revision currently copies all 16 PNGs byte-for-byte, but that guarantee is not encoded in CI.
First-principles / Occam assessment: the opaque-ID model is the right simple boundary, but validation must happen at every runtime ingress and destructive operations should have a single main-process owner. A small main-process operation that validates the choice and coordinates selection reset with deletion is simpler and safer than renderer-side sequencing plus swallowed persistence errors. No broad rewrite is required.
Deletion/test quality: desktopAssetPath() appears to be used only by its own test; it and the redundant assertion can be removed unless a production caller is planned. custom-app-icons.ts also leaves a trailing blank line that makes git diff --check fail. The 14 decorative variants are not technically dead, but whether Maka wants to ship a second geometric mark is a product/brand decision and could be separated from the picker mechanics.
Verification performed locally: clean merge-tree against current main; full Desktop production build; all-workspace typecheck; Biome format check; 17 focused settings/icon tests; source PNG dimension checks; and inspection of the generated macOS app resources. GitHub currently reports both workflows as action_required with no jobs, so no hosted CI checks have run for this head.
Verdict: not ready to merge at this revision. Please fix the inline findings, add malformed settings/import/removal/package-boundary tests, then re-request review. Residual platform risk remains around Windows/Linux icon updates and should be covered by at least one packaged smoke check after the cross-platform format filter is corrected.
cd6178b to
17d37be
Compare
|
Thanks — the two runtime findings were both real, and I confirmed them before fixing rather than taking them on faith. Rebased onto current Path escape — confirmed and fixed. Delete ordering — confirmed and fixed. The code did the opposite of its comment. Removal is now one main-process operation that resets the selection, applies it, then deletes; the renderer no longer sequences anything. Packaged-resource verifier.
Trailing blank line — fixed, On the 14 variants: I have raised the brand question with the maintainers separately and was told to go ahead, so they stay for now. If the PMC would rather decide that on its own thread, say the word and I will drop the artwork commit — the picker and the import path stand without it. Not fixed, and I want to be straight about it: I still cannot verify Windows/Linux icon updates or run a packaged smoke check here. This machine cannot start an Electron GUI, so the picker and dock switching were verified by hand on macOS only, and the cross-platform format filter you flagged is untested by me. Verification this round: Re-requesting review. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for continuing to work through this carefully. I reviewed exact head 9d7e2c28d8b71e02743f60516590793dccd5b839. The earlier path, decode, geometry, and packaging-presence findings are addressed. I found one release-contract regression introduced by the new packaged-resource check, plus two non-blocking interaction/accessibility issues below.
The desired final state is small: keep the new icon catalog mandatory for current artifacts, preserve the historical contract when verifying a previous release, and make icon selection a single-choice operation that cannot race deletion.
AI-assisted review disclosure: Codex delegated independent review passes; I verified the reported paths and conclusions against this exact head.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — I re-reviewed exact head 0660b5f29bcc707ecf9d49e72401c8d6e492346e. The legacy artifact-contract regression and the deletion/selection race are both fixed cleanly, and the focused verifier contract passes.
I agree that the remaining single-choice semantics issue belongs at the shared SelectableCard seam. Theme, palette, and app-icon pickers all use the same checkbox-based contract, so fixing only the new picker here would leave one page with inconsistent interaction semantics. Please address all three together in a focused shared-component follow-up.
Approving this exact head with that non-blocking P2. The hosted CI and Release Windows workflows are still awaiting permission to run, so this approval is not a substitute for their results.
AI-assisted review disclosure: OpenAI Codex coordinated earlier independent review passes. I personally re-verified the latest delta, legacy/current artifact contracts, deletion lifecycle, shared accessibility boundary, current-main merge result, and live GitHub state, and I independently made the approval decision.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for getting the previous runtime and packaging issues into a much cleaner final state. I rechecked this exact head with three independent review passes: the earlier path-validation, decode, deletion-ordering, startup-restoration, and legacy-package-contract findings are resolved, and both hosted checks are green.
There is one remaining non-blocking correctness issue in the main-process deletion transaction, noted inline. The clean final state is to make the reset conditional on the selection the deletion observed, so a concurrent newer choice remains authoritative.
AI-assisted review disclosure: OpenAI Codex coordinated three independent review passes. I verified the reported race against this exact head, the existing SettingsStore conditional-update seam, the live review threads, mergeability, and current-head CI, and I made the final review decision.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for closing the original get → update race with the existing conditional-write seam. Two independent exact-head passes confirmed that this CAS is correct for changes that happen before its predicate, but they also reproduced one remaining selection/file lifecycle window after the CAS. I am describing the final authority boundary inline so we can finish this without another local patch.
AI-assisted review disclosure: OpenAI Codex coordinated two independent exact-head re-reviews. I verified the retained interleaving, the current SettingsStore and IPC ownership seams, live threads, mergeability, approval state, and current-head CI progress, and I made the final review decision.
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.
3ce30b6 to
3fdfac7
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — accepting a user-supplied image is the kind of feature where the interesting part is everything around the decoder, and that part is done carefully here.
What this solves / how: there was no way to change the app icon. This adds a preset catalog plus an import path: the main process reads the candidate with a size cap, parses the header to learn the dimensions before decoding, then decodes through Electron's nativeImage, crops to square and normalises to 1024.
On the import path specifically, since it's a new attack surface — checked each of these against the source rather than taking the structure on faith:
- No new dependency. Decoding goes through
electron.nativeImage; the header parse is hand-rolled and small. Nothing new to keep patched. - Bounds are enforced before the decoder ever sees the bytes.
readCappedstreams through a single handle and aborts pastCUSTOM_ICON_MAX_INPUT_BYTES, so a 10GB "png" fails on read rather than in memory.readImageHeaderthen rejects onCUSTOM_ICON_MAX_EDGEbefore decode, so a file declaring 65535×65535 never reaches an allocation — that ordering is the part that matters, and it's right. - The stored filename contains nothing from the user. The id is
randomUUID()with dashes stripped, the path isjoin(userData/app-icons, id.png), and reads validate against a strict hex pattern. The original filename never participates, so there's no traversal to reason about. - Preset weight is proportional: 15 PNGs at ~27KB, ~400KB total, and
verify-packaged-app.mjsgained a catalog assertion so a missing or extra preset fails packaging rather than shipping quietly.
Cross-PR note: this is the third PR today touching verify-packaged-app.mjs. The icon-catalog change sits around lines 699/735; #3473's packaged-Git recursion (now merged) is at 705/759+, and #3472's native-probe rewrite is at 334–640. No overlap, and the Git checks are untouched here.
One thing to be aware of, not a finding: the Release Windows check on this PR is red for reasons unrelated to it — see #3484, a regression from #3001 that breaks the Windows sandbox verifier on every open PR. The CI job on this head is green.
LGTM.
AI-assisted review. The bounds and path handling above were each verified against the source at this head.
|
Status on head
I have not tried to fix it here: it is upstream runtime behaviour, it belongs with #3001, and folding it into this PR would only make this branch harder to review. Happy to open a separate issue with the log if that is useful. For completeness, local verification on this exact head: full workspace typecheck, Biome lint and format, Knip on both workspaces, the Astryx inventory and theme checks, 1130 desktop main-process tests, 592 in Windows and Linux runtime behaviour remains unverified by me — this machine cannot start an Electron GUI, so the picker, dock switching and import were exercised by hand on macOS only. |
3 participants
Settings → 外观 grows an App icon picker: a grouped set of shipped icons,
plus the ability to import your own.
What it does
dock, Windows/Linux per-window
be read
Design notes worth your time
appearance.appIconstays a closed value, not a path. Shipped ids are anenum; an imported icon is
custom:<32 hex>. The main process generates theid, the id is the whole file name, and both core's normalization and the
store re-check its shape before it reaches
join. A hand-edited settingsfile cannot name a file — the worst it can do is name nothing, which
normalizes to the brand mark. The renderer never learns a path; the file
dialog runs in the main process, the way PetPack import already does.
Imported files are re-encoded, not copied. Decode → centre-crop to square
→ scale to 1024 → write PNG. The stored shape is then 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.
Artwork carries the 824/1024 margin itself. The OS scales the canvas to
the dock tile and never adds margin, so a full-bleed tile sits ~24% wider
than every neighbour. Measured against the shipped
icon.pngand againstother installed apps; the import hint says the same thing to the user.
Effects hang off
client-settings-effects.ts, already the single entrypoint for OS-level side effects of a settings write, which gives live apply
and file-watch echo dedupe for free. Startup still sets the icon
synchronously in
desktop-shell-presentation.ts— waiting for the settingsstore would let the dock flash the Electron rocket first, the regression
PR-GRAY-CARD-LIFT-0 fixed.
An unreadable icon file is an EMPTY
NativeImage, not an error. Handingthat to
setIconblanks the dock tile, so emptiness is the failure test andwhat advances the fallback chain.
Grouping
16 tiles in one grid gives the eye nowhere to start — the problem
PALETTE_GROUPSalready solved for palettes, so this follows that shapeincluding the per-group
role="group"and label id. Group membership is arenderer concern: the main process reports what artwork loaded, and anything
no group claims (imported art) lands in the trailing group rather than
disappearing.
Verification
tsc --noEmitclean for main, preload and renderernode --test— 13 main-process tests (icon paths, packaged resolution,imported-id path safety, settings effects), 589 in
@maka/core../../../etc/passwd, wrong length,uppercase, empty, embedded slash — all refused before
joinnpm run devOpen questions
replace the brand mark —
defaultstays the default, and the mascot isuntouched. If a shipped set is a decision that belongs elsewhere, the
import feature stands on its own and I will happily drop the 14 files.
first.