Settings: make setSetting idempotent on unchanged values

`setSetting` used to unconditionally schedule a save, notify listeners, and emit the cross-window `settings:changed` event even when the new value matched the cached value. The cascade was load-bearing in production for real changes (cross-window sync, applier-driven Tauri commands), but for no-op writes it was pure waste.

Under parallel-shard E2E load this surfaced as a flake on `network-toggle.spec.ts > toggling back on restores 'Network'`: the test body did ON→OFF→ON, then `afterEach` did another `setSetting('network.enabled', true)`. Each `network.enabled` toggle cascades through the applier to `setNetworkEnabled` IPC, and the OFF path synchronously stops the mDNS daemon and emits 14 `network-host-lost` events. The redundant afterEach call queued more event-loop work right when an `mcp_round_trip` was waiting on `mcp-response`, so the FE missed the 5s budget ~25% of the time.

The fix is one guard: skip the cascade when `settingsCache[id] === value`. `===` is the right comparator because every registered setting is either a primitive or a pinned-shape JSON object that callers replace by reference when mutating — same-reference always means no change. If a future setting needs deep-equality, narrow the comparison there rather than dropping the guard.

Wins beyond the test fix: no disk write for redundant clicks on a toggle, no cross-window emit, no applier-driven Tauri command. The new `settings-store.test.ts` pins the behavior — including that real value flips after a no-op still fire correctly.

Author: vdavid

apps/desktop/src/lib/settings/settings-store.test.ts

@@ -0,0 +1,88 @@
+import { describe, it, expect, vi, beforeEach } from 'vitest'
+import { emit } from '@tauri-apps/api/event'
+import { setSetting, onSettingChange } from './settings-store'
+
+// `test-setup.ts` already mocks `@tauri-apps/api/event` globally. We don't
+// reset the module here: settings-store's `settingsCache` is module-scoped,
+// so each test picks a setting ID nobody else in this file uses to avoid
+// leaking cache state between tests.
+const mockedEmit = vi.mocked(emit)
+
+const settingsChangedEmits = () => mockedEmit.mock.calls.filter((c) => c[0] === 'settings:changed')
+
+describe('setSetting idempotency', () => {
+  beforeEach(() => {
+    mockedEmit.mockClear()
+  })
+
+  it('cascades on a real value change: emits + notifies listeners', () => {
+    // First test in the file: `appearance.uiDensity` is still at default in
+    // the cache, so this is a real change.
+    const listener = vi.fn()
+    const unsub = onSettingChange(listener)
+
+    setSetting('appearance.uiDensity', 'spacious')
+
+    expect(listener).toHaveBeenCalledTimes(1)
+    expect(listener).toHaveBeenCalledWith('appearance.uiDensity', 'spacious')
+    expect(settingsChangedEmits()).toHaveLength(1)
+    expect(settingsChangedEmits()[0]?.[1]).toEqual({ id: 'appearance.uiDensity', value: 'spacious' })
+
+    unsub()
+  })
+
+  it('short-circuits when value is unchanged: no emit, no listener', () => {
+    // Use a fresh setting ID so the cache for it starts undefined.
+    const listener = vi.fn()
+    const unsub = onSettingChange(listener)
+
+    setSetting('network.enabled', false)
+    expect(listener).toHaveBeenCalledTimes(1)
+    listener.mockClear()
+    mockedEmit.mockClear()
+
+    // Same value: must be a complete no-op past validation.
+    setSetting('network.enabled', false)
+
+    expect(listener).not.toHaveBeenCalled()
+    expect(settingsChangedEmits()).toHaveLength(0)
+
+    unsub()
+  })
+
+  it('cascades again when the value flips after a no-op', () => {
+    const listener = vi.fn()
+    const unsub = onSettingChange(listener)
+
+    // Note: `appearance.uiDensity` is already 'spacious' in the cache from
+    // the first test in this file. Setting it again must short-circuit.
+    setSetting('appearance.uiDensity', 'spacious')
+    expect(listener).not.toHaveBeenCalled()
+
+    // Real change: cascade fires.
+    setSetting('appearance.uiDensity', 'compact')
+    expect(listener).toHaveBeenCalledTimes(1)
+    expect(listener).toHaveBeenCalledWith('appearance.uiDensity', 'compact')
+    expect(settingsChangedEmits()).toHaveLength(1)
+
+    unsub()
+  })
+
+  it('handles numbers the same way (=== covers all primitive setting types)', () => {
+    const listener = vi.fn()
+    const unsub = onSettingChange(listener)
+
+    setSetting('advanced.maxLogStorageMb', 123)
+    expect(listener).toHaveBeenCalledTimes(1)
+
+    listener.mockClear()
+    setSetting('advanced.maxLogStorageMb', 123)
+    expect(listener).not.toHaveBeenCalled()
+
+    setSetting('advanced.maxLogStorageMb', 456)
+    expect(listener).toHaveBeenCalledTimes(1)
+    expect(listener).toHaveBeenLastCalledWith('advanced.maxLogStorageMb', 456)
+
+    unsub()
+  })
+})

apps/desktop/src/lib/settings/settings-store.ts

@@ -217,13 +217,35 @@ export function getSetting<K extends SettingId>(id: K): SettingsValues[K] {
 /**
  * Set a setting value. Validates against constraints before storing.
  * Throws SettingValidationError if invalid.
+ *
+ * Idempotent: when `value` strictly equals the currently-cached value, the
+ * call returns after validation without scheduling a save, notifying
+ * listeners, or emitting the cross-window event. This avoids redundant work
+ * for the (common) case of writing the same value twice — e.g. a settings UI
+ * onChange that fires for any click, or test setup/teardown that resets a
+ * setting back to its already-current value. The cascade for `network.enabled`
+ * alone fires 14 `network-host-lost` events through the FE event loop on a
+ * real toggle, so the redundant call used to be heavy enough to occasionally
+ * starve a concurrent `mcp_round_trip` waiting on `mcp-response`.
+ *
+ * `===` is the right comparator here: every registered setting is a primitive
+ * (`boolean | number | string`) or a pinned-shape JSON object that callers
+ * replace by reference when they mutate, so same-reference always means
+ * no-change. If you add a setting that requires deep-equality, narrow the
+ * comparison here instead of dropping the guard.
  */
 export function setSetting<K extends SettingId>(id: K, value: SettingsValues[K]): void {
   log.debug('setSetting({id}, {value})', { id, value })
 
   // Validate the value
   validateSettingValue(id, value)
 
+  // Idempotency: skip the cascade when nothing actually changed.
+  if (settingsCache[id] === value) {
+    log.debug('setSetting({id}): unchanged, skipping notify+save+emit', { id })
+    return
+  }
+
   // Update cache immediately for synchronous access
   settingsCache[id] = value