fix(desktop): show the canonical Maka icon in the permission guide - #3464
Conversation
Adversarial review record (pre-submission)An independent review pass probed this change with running experiments before submission. All seven vectors CONFIRMED-SAFE; the evidence was active, not read-only:
The reviewer's two suggestions — resolving the icon lazily at its only consumer (sparing non-darwin |
jackwener
left a comment
There was a problem hiding this comment.
Automated review of exact head f82c0736acfc29de0854cbfbff7e5d9183ee3296 against main@f1f4e71a9540a4da23159052c924fee72202e989.
No actionable implementation issue found. The canonical PNG is resolved through the same packaged/dev asset authority already used by the main window; empty images degrade to the existing no-icon drag path; both getFileIcon crash/generic-icon paths are removed; and current macOS/Windows packaged verification requires the same assets/icon.png that extraResources ships. The legacy Windows baseline alone opts out, while its post-upgrade current-contract verification still requires the icon.
Required conclusions:
- Optimal for the actual problem: yes; this deletes the wrong native identity path and reuses the canonical packaged asset.
- Production code to delete: the PR already deletes
BUNDLE_ICON_OPTIONS,loadNativeBundleIcon, both native lookups, and the redundant drag fallback; none further identified. - Tests to delete/replace: none identified. The fault-injected packaged-resource tests protect the regression #3451 left uncovered.
- Deeper refactor: no.
- Ready to merge: not yet: no required hosted
testresult or independent human approval is present on the current PR state. - Residual risks: packaged visual/drag behavior still benefits from a human macOS smoke check; no additional code gap was found.
This changes user-visible permission-guide presentation. Independent human review is required; this automated review is not approval.
Packaged macOS smoke — this PR's head, real deviceAddressing the residual risk from the automated review ("packaged visual/drag behavior still benefits from a human macOS smoke check"): I built this exact head into a packaged app locally and drove the permission guide on real hardware. Setup. Observed, in order:
Not covered by this smoke: the physical drag gesture into System Settings needs a human hand on an unmanaged pointer, so I did not exercise the drop itself. The drag replica, however, is composited from the same verified Happy to re-run any variation of this if it helps the human review. |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — this removes the crash-prone native icon lookup and reuses the canonical packaged assets/icon.png path through the existing desktop asset seam. I reviewed exact head f82c0736acfc29de0854cbfbff7e5d9183ee3296; the main-process bundle authority, renderer drag-image boundary, current/legacy resource contracts, and cleanup are consistent. I found no P0–P3 issues, and both current-head test and package checks are green.
AI-assisted review disclosure: OpenAI Codex performed an independent exact-head review. I verified the source and packaged-resource paths, cross-platform scope, tests, live reviews, mergeability, and current-head CI, and I independently made the approval decision.
app.getFileIcon() was the wrong identity source: macOS reduces the path to its UTType and returns the generic application icon, never this app's own. Load the canonical 1024px assets/icon.png (shipped since the window-icon fix) through desktopAssetPath + nativeImage.createFromPath instead, resolved lazily at the card payload so non-darwin starts never pay the decode. This retires the whole native icon chain: both getFileIcon() calls, BUNDLE_ICON_OPTIONS, loadNativeBundleIcon() and its packaged-only gate, the drag-time native fallback, and the unbounded icon await in start(). Dev builds now show the icon too instead of a deliberate blank. The PNG is now a load-bearing packaged resource, so assertPackagedResources requires it on current builds, gated off for the legacy Windows upgrade baseline that predates it. Implements the follow-up agreed in apache#3455 review. Refs apache#3352. Generated-by: Claude Code
f82c073 to
3aa9043
Compare
jackwener
left a comment
There was a problem hiding this comment.
First-principles: app.getFileIcon() is the wrong identity source on macOS (UTType generic app icon, and 'large' SIGTRAPs packaged builds). Loading the canonical assets/icon.png through the existing desktopAssetPath seam is the minimal fix, and it deletes the native icon chain.
Rebased onto current main. Packaged-resource contract now requires the icon on current builds and still forbids bundled Git; the legacy Windows baseline opts out of both.
Approve.
Summary
Follow-up agreed in the #3455 review (M4n5ter's request-changes, Astro-Han's P2): the permission guide now shows the canonical Maka icon instead of a macOS file-type icon.
app.getFileIcon()was the wrong identity source — macOS reduces the path to its UTType and returns the generic application-bundle icon, never this particular app's own. The guide now loads the canonical 1024×1024assets/icon.png(shipped as an extra resource since #3451, and the same PNG electron-builder stamps onto the bundle) viadesktopAssetPath()+nativeImage.createFromPath(), resolved lazily at the card payload — its only consumer — so a non-darwinstart()never pays the decode, and a missing asset degrades to the pre-existing no-icon path instead of caching the failure.This lands the simplify-audit the review outlined: both
app.getFileIcon()calls,BUNDLE_ICON_OPTIONS,loadNativeBundleIcon()and its packaged-only gate, the drag-time native fallback, and the unbounded icon await instart()are all gone.resolveAppBundle()remains solely responsible for the file being dragged. Dev builds now show the icon too, instead of the deliberate blank the #1920 gate imposed.Since the PNG is now a load-bearing packaged resource,
assertPackagedResourcesrequiresassets/icon.png— gated behindrequireCanonicalIcon(same shape asrequireDisclaimer) because the Windows upgrade-lifecycle baseline predates #3451; the lifecycle's post-upgrade re-verification still runs under the current contract and does require it.Refs #3352.
Verification
Empirical, with this repo's Electron 43.2.0 on an Apple silicon Mac:
nativeImage.createFromPath(assets/icon.png)decodes 1024×1024;resize({64,64})→toDataURL()yields a valid PNG data URL; a missing path returns an empty image without throwing (degrades tonull, same as before).dist/main/desktop-assets.jsresolves the dev layout toapps/desktop/assets/icon.png(exists) and the packaged layout to<resourcesPath>/assets/icon.png— the call site is byte-identical to the existing one inmain-window.ts.webContents.startDrag({ file, icon: nativeImage.createEmpty() })does not throw — Electron's'icon' parameter is requiredguard fires only on a missing key, so the deleted drag fallback loses nothing.assertPackagedResourcesfault-injection: current contract fails on a missing icon;requireCanonicalIcon: false(legacy baseline) passes — now pinned as two unit tests driven through the injectablerequirePath(no packaging, no platform dependence).Checks run locally (Node 24.18):
npm --workspace @maka/desktop run typecheck— passnpm --workspace @maka/desktop run test— 1036 pass / 0 failnode --test scripts/verify-packaged-app.test.mjs— 16 pass / 0 failbiome checkon changed files — clean;knipdesktop workspace — cleanNot run: a full packaged DMG build (the packaged path split is covered by the existing
desktop-assetsunit tests plus the release verifiers this PR extends).An independent adversarial review pass probed the change with fault-injection experiments before submission (path resolution from the built layout,
start()payload regression, caching semantics, drag-fallback deletion, theassertPackagedResourcescaller matrix including the legacy-baseline gate, destructure coherence, comment truthfulness) — no defects; its two suggestions (lazy resolution at the consumer, theassertPackagedResourcesunit tests) are incorporated.AI use
Select exactly one:
Tool(s) and scope: Claude Code implemented the maintainer-specified follow-up, ran the Electron-level verification, and wrote the tests; an independent adversarial review pass (also Claude) verified the change with running experiments. I reviewed and verified the result.
Checklist
Does this PR entail a change in behavior?