feat(desktop): let the app icon differ between light and dark

.github/workflows/ci.yml

@@ -163,6 +163,15 @@ jobs:
         if: steps.plan.outputs.code == 'true'
         run: npm run typecheck
 
+      # Two drift contracts over the shipped app-icon artwork, sitting beside
+      # the theme drift check for the same reason: the committed bytes are a
+      # build output that nothing else re-derives, so without this a change to
+      # the generator, to DEFAULT_APP_ICON, or to the packaging config can go
+      # green while the artwork it names no longer matches.
+      - name: App icon artwork drift
+        if: steps.plan.outputs.code == 'true'
+        run: node --test scripts/verify-packaged-app-icons.test.mjs scripts/generate-app-icons.test.mjs
+
       - name: Astryx theme drift
         if: steps.plan.outputs.code == 'true'
         run: npm run astryx:theme -- --check

apps/desktop/electron-builder.config.mjs

@@ -174,7 +174,14 @@ export default {
       { target: 'zip', arch: ['arm64'] },
     ],
     category: 'public.app-category.productivity',
-    icon: 'assets/icon.png',
+    // The bundle icon is what Finder, Launchpad and the installer show, and
+    // none of those run our code — so it cannot follow the user's choice and
+    // has to be the shipped default. `assets/icon.png` is the original mascot
+    // mark, which is still selectable as the `default` id but is no longer the
+    // default; pointing the bundle at it would leave every surface outside the
+    // running app on the old artwork. Kept in step with `DEFAULT_APP_ICON` by
+    // a test in scripts/verify-packaged-app-icons.test.mjs.
+    icon: 'assets/app-icons/sky.png',
     forceCodeSigning: true,
     hardenedRuntime: true,
     notarize: true,
@@ -208,7 +215,9 @@ export default {
       { target: 'zip', arch: ['x64'] },
     ],
     artifactName: 'Maka-${version}-win-${arch}.${ext}',
-    icon: 'assets/icon.png',
+    // Same reason as `mac.icon` above: the .exe, the installer and the
+    // shortcut are drawn by the OS from this file, not by us.
+    icon: 'assets/app-icons/sky.png',
     // No Authenticode certificate yet. Being unsigned is the absence of one:
     // electron-builder skips signing when no certificate is configured, and
     // `forceCodeSigning` is left off so that skip is not an error. Nothing here

apps/desktop/src/main/__tests__/app-icon-ipc.test.ts

@@ -23,6 +23,7 @@ import { tmpdir } from 'node:os';
 import { join } from 'node:path';
 import { test } from 'node:test';
 import type { IpcMainInvokeEvent } from 'electron';
+import { DEFAULT_APP_ICON } from '@maka/core/settings';
 import type { AppSettings, UpdateAppSettingsInput } from '@maka/core/settings';
 import { registerAppIconIpc } from '../app-icon-ipc.js';
 import { customAppIconDirectory, resolveCustomAppIconPath } from '../custom-app-icon-store.js';
@@ -36,13 +37,23 @@ async function harness(selected: string, options: {
     onCompareAndSet?: () => void;
     onApply?: () => void;
     onShowOpenDialog?: () => Promise<void>;
+    /** Seeds `appearance.appIconDark`; absent means the split is off. */
+    dark?: string;
+    /** Makes the conditional write throw, standing in for a disk failure. */
+    failWrite?: boolean;
   } = {}) {
   const root = await mkdtemp(join(tmpdir(), 'maka-icon-ipc-'));
   await mkdir(customAppIconDirectory(root), { recursive: true });
   await writeFile(resolveCustomAppIconPath(root, ID), 'x');
 
   const handlers = new Map<string, Handler>();
-  let settings = { appearance: { theme: 'auto', appIcon: selected } } as unknown as AppSettings;
+  let settings = {
+    appearance: {
+      theme: 'auto',
+      appIcon: selected,
+      ...(options.dark === undefined ? {} : { appIconDark: options.dark }),
+    },
+  } as unknown as AppSettings;
   const applied: AppSettings[] = [];
 
   registerAppIconIpc({
@@ -65,15 +76,22 @@ async function harness(selected: string, options: {
       },
       updateIf: async (
         predicate: (current: AppSettings) => boolean,
-        patch: UpdateAppSettingsInput,
+        patch: UpdateAppSettingsInput | ((current: AppSettings) => UpdateAppSettingsInput),
       ) => {
         // The real store evaluates the predicate and writes on one queue. The
         // hook stands in for whatever else reached that queue first.
         options.onCompareAndSet?.();
         if (!predicate(settings)) return { applied: false, settings };
+        // Thrown after the predicate, where the real store would fail: the
+        // decision to write has been made and the write is what breaks.
+        if (options.failWrite) throw new Error('disk is full');
+        // Spread, like the real `mergeSettings`: an explicit `undefined` in a
+        // patch overwrites rather than being skipped, which is how a slot is
+        // cleared.
+        const resolved = typeof patch === 'function' ? patch(settings) : patch;
         settings = {
           ...settings,
-          appearance: { ...settings.appearance, ...patch.appearance },
+          appearance: { ...settings.appearance, ...resolved.appearance },
         } as AppSettings;
         return { applied: true, settings };
       },
@@ -91,6 +109,7 @@ async function harness(selected: string, options: {
     remove: (icon: unknown) =>
       handlers.get('app:removeIcon')!(undefined as unknown as IpcMainInvokeEvent, icon),
     current: () => settings.appearance.appIcon,
+    currentDark: () => settings.appearance.appIconDark,
     select: (icon: unknown) =>
       handlers.get('app:selectIcon')!(undefined as unknown as IpcMainInvokeEvent, icon),
     importIcon: () =>
@@ -111,9 +130,10 @@ test('removing the current icon resets the selection before the file goes away',
   const result = (await h.remove(ICON)) as { ok: boolean; selection?: string };
 
   assert.equal(result.ok, true);
-  assert.equal(result.selection, 'default');
+  // The shipped default, which is no longer the id literally named `default`.
+  assert.equal(result.selection, DEFAULT_APP_ICON);
   // Both halves moved, and the setting is the half that moved first.
-  assert.equal(h.current(), 'default');
+  assert.equal(h.current(), DEFAULT_APP_ICON);
   assert.deepEqual(await readdir(customAppIconDirectory(h.root)), []);
   // The OS surface was told, so the dock is not still holding the deleted art.
   assert.equal(h.applied.length, 1);
@@ -194,9 +214,11 @@ test('a selection issued mid-removal cannot land between reset, apply and delete
   // It ran after the removal, not inside it, and the artwork was already gone.
   assert.equal(selected.ok, false);
   assert.equal(selected.reason, 'missing_artwork');
-  assert.equal(h.current(), 'default');
+  assert.equal(h.current(), DEFAULT_APP_ICON);
   assert.deepEqual(await readdir(customAppIconDirectory(h.root)), []);
-  // Nothing ran between the reset and the delete.
+  // Nothing ran between the reset and the delete, and both slots move in a
+  // single compare-and-set: a second conditional write here would be a window
+  // where the light slot is already committed and the dark one is not.
   assert.deepEqual(observed, ['compare-and-set', 'apply']);
 });
 
@@ -221,3 +243,60 @@ test('an open file dialog does not hold the queue', async () => {
   releaseDialog();
   await importing;
 });
+
+test('removing an icon used only in dark mode clears the dark slot', async () => {
+  // The dangling-reference case: the light slot names something else, so the
+  // light-slot predicate does not match and an earlier version of this handler
+  // deleted the file while leaving `appIconDark` pointing at it.
+  const h = await harness('sky', { dark: ICON });
+
+  const result = (await h.remove(ICON)) as { ok: boolean; darkSelection?: string };
+
+  assert.equal(result.ok, true);
+  assert.equal(h.current(), 'sky', 'the light choice is untouched');
+  assert.equal(h.currentDark(), undefined, 'the dark slot no longer names deleted artwork');
+  assert.equal(result.darkSelection, undefined);
+  assert.equal(h.applied.length, 1, 'the dock was re-applied for the cleared slot');
+});
+
+test('removing an icon used in both slots clears both', async () => {
+  const h = await harness(ICON, { dark: ICON });
+
+  const result = (await h.remove(ICON)) as { ok: boolean; selection: string };
+
+  assert.equal(result.ok, true);
+  assert.equal(h.current(), 'sky');
+  assert.equal(h.currentDark(), undefined);
+  assert.equal(result.selection, 'sky');
+});
+
+test('removing an unrelated icon leaves a dark choice alone', async () => {
+  const other = `custom:${'d'.repeat(32)}`;
+  const h = await harness('sky', { dark: 'ink' });
+
+  await h.remove(other);
+
+  assert.equal(h.current(), 'sky');
+  assert.equal(h.currentDark(), 'ink', 'an unrelated removal must not disturb the split');
+});
+
+test('a failed reset commits nothing and leaves the dock alone', async () => {
+  // The partial-commit case: both slots name the icon, so a two-step reset
+  // would have written the light slot before the second write could fail.
+  // One queued write means a failure leaves the persisted state untouched,
+  // which is what makes `reset_failed` an honest answer.
+  const h = await harness(ICON, { dark: ICON, failWrite: true });
+
+  const result = (await h.remove(ICON)) as { ok: boolean; reason?: string };
+
+  assert.equal(result.ok, false);
+  assert.equal(result.reason, 'reset_failed');
+  assert.equal(h.current(), ICON, 'the light slot was not half-reset');
+  assert.equal(h.currentDark(), ICON, 'the dark slot was not half-reset');
+  assert.equal(h.applied.length, 0, 'nothing was applied to the dock');
+  assert.deepEqual(
+    await readdir(customAppIconDirectory(h.root)),
+    [`${ID}.png`],
+    'the artwork survives a failed reset',
+  );
+});

apps/desktop/src/main/__tests__/client-settings-effects.test.ts

@@ -39,6 +39,7 @@ test('applies each client settings snapshot once across local writes and file wa
     applyAppIcon: async (icon) => {
       appIcons.push(icon);
     },
+    systemPrefersDark: () => false,
     observeLocale: () => undefined,
     emitExternalChanged: () => {
       rendererEvents += 1;
@@ -73,6 +74,7 @@ test('applies a chosen app icon once, and again only when the choice changes', a
     applyAppIcon: async (icon) => {
       appIcons.push(icon);
     },
+    systemPrefersDark: () => false,
     observeLocale: () => undefined,
     emitExternalChanged: () => undefined,
   });
@@ -88,3 +90,91 @@ test('applies a chosen app icon once, and again only when the choice changes', a
 
   assert.deepEqual(appIcons, ['mono', 'default']);
 });
+
+
+test('an OS appearance flip re-applies the icon without any setting changing', async () => {
+  // The whole reason `refresh` is wired to nativeTheme: nothing in the
+  // settings object moves when the OS flips, so the fingerprint comparison
+  // that guards every other effect would report "no change" and the dock
+  // would keep the light tile.
+  let systemDark = false;
+  const current = createDefaultSettings();
+  current.appearance.theme = 'auto';
+  current.appearance.appIcon = 'sky';
+  current.appearance.appIconDark = 'midnight';
+  const applied: string[] = [];
+  const effects = createClientSettingsEffects({
+    settingsStore: { get: async () => current },
+    applyKeepSystemAwake: async () => undefined,
+    applyBotSettings: async () => undefined,
+    applyAppIcon: async (icon) => {
+      applied.push(icon);
+    },
+    systemPrefersDark: () => systemDark,
+    observeLocale: () => undefined,
+    emitExternalChanged: () => undefined,
+  });
+
+  await effects.refresh(false);
+  assert.deepEqual(applied, [], 'the light tile is already up from startup');
+
+  systemDark = true;
+  assert.equal(await effects.refresh(false), true);
+  assert.deepEqual(applied, ['midnight']);
+
+  // Idempotent: a second notification for the same appearance must not cost
+  // another 1024px decode.
+  assert.equal(await effects.refresh(false), false);
+  assert.deepEqual(applied, ['midnight']);
+
+  systemDark = false;
+  await effects.refresh(false);
+  assert.deepEqual(applied, ['midnight', 'sky']);
+});
+
+test('with one icon for both appearances a theme flip changes nothing', async () => {
+  let systemDark = false;
+  const current = createDefaultSettings();
+  current.appearance.theme = 'auto';
+  current.appearance.appIcon = 'forest';
+  delete current.appearance.appIconDark;
+  const applied: string[] = [];
+  const effects = createClientSettingsEffects({
+    settingsStore: { get: async () => current },
+    applyKeepSystemAwake: async () => undefined,
+    applyBotSettings: async () => undefined,
+    applyAppIcon: async (icon) => {
+      applied.push(icon);
+    },
+    systemPrefersDark: () => systemDark,
+    observeLocale: () => undefined,
+    emitExternalChanged: () => undefined,
+  });
+
+  await effects.refresh(false);
+  assert.deepEqual(applied, ['forest']);
+  systemDark = true;
+  assert.equal(await effects.refresh(false), false);
+  assert.deepEqual(applied, ['forest'], 'no second tile was ever chosen');
+});
+
+test('an explicit dark preference ignores what the OS reports', async () => {
+  const current = createDefaultSettings();
+  current.appearance.theme = 'dark';
+  current.appearance.appIcon = 'sky';
+  current.appearance.appIconDark = 'ink';
+  const applied: string[] = [];
+  const effects = createClientSettingsEffects({
+    settingsStore: { get: async () => current },
+    applyKeepSystemAwake: async () => undefined,
+    applyBotSettings: async () => undefined,
+    applyAppIcon: async (icon) => {
+      applied.push(icon);
+    },
+    systemPrefersDark: () => false,
+    observeLocale: () => undefined,
+    emitExternalChanged: () => undefined,
+  });
+  await effects.refresh(false);
+  assert.deepEqual(applied, ['ink']);
+});

apps/desktop/src/main/__tests__/settings-ownership-app-icon.test.ts

@@ -47,3 +47,20 @@ test('the rest of appearance still travels', () => {
   const patch = clientOwnedSettingsPatch({ appearance: { theme: 'light', palette: 'nord' } });
   assert.deepEqual(patch.appearance, { theme: 'light', palette: 'nord' });
 });
+
+test('the dark slot cannot travel on the generic channel either', () => {
+  // It names artwork on exactly the same terms as `appIcon`, so leaving it
+  // unfiltered reopens the removal/write race through the other slot.
+  const patch = clientOwnedSettingsPatch({
+    appearance: { theme: 'dark', appIconDark: `custom:${'b'.repeat(32)}` },
+  });
+
+  assert.deepEqual(patch.appearance, { theme: 'dark' });
+});
+
+test('an appearance patch of only icon slots drops out entirely', () => {
+  assert.equal(
+    clientOwnedSettingsPatch({ appearance: { appIcon: 'sky', appIconDark: 'ink' } }).appearance,
+    undefined,
+  );
+});