fix(settings): sync across tabs; merge-on-write instead of clobbering

The settings atom was a load-time snapshot with no storage listener and
wrote the whole blob, so two tabs silently reverted each other. It now
re-reads on storage events and writes only the keys that changed
relative to the previous value. Unit-tested.

Fixes #42

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PPmy3tPq869XDW4njjVaKA

src/app/state/settings.test.ts

@@ -1,6 +1,6 @@
 import { test } from 'node:test';
 import assert from 'node:assert/strict';
-import { getSettings } from './settings';
+import { getSettings, setSettings, Settings } from './settings';
 
 // getSettings() reads localStorage; node has none, so install a controllable
 // mock per case. (The module already loaded safely with no localStorage thanks
@@ -12,6 +12,21 @@ const setStored = (value: string | null): void => {
     removeItem: () => undefined,
   };
 };
+// Map-backed mock (unlike setStored above, get/set actually round-trip)
+// for the setSettings merge tests below, which need writes to persist.
+const installStorage = (): Map<string, string> => {
+  const map = new Map<string, string>();
+  (globalThis as { localStorage?: unknown }).localStorage = {
+    getItem: (k: string) => (map.has(k) ? map.get(k)! : null),
+    setItem: (k: string, v: string) => {
+      map.set(k, v);
+    },
+    removeItem: (k: string) => {
+      map.delete(k);
+    },
+  };
+  return map;
+};
 const setThrowingStorage = (): void => {
   (globalThis as { localStorage?: unknown }).localStorage = {
     getItem: () => {
@@ -89,3 +104,46 @@ test('returns defaults on malformed JSON', () => {
   assert.doesNotThrow(() => getSettings());
   assert.equal(getSettings().callNoiseSuppression, 'browser');
 });
+
+// Gitea #42: two tabs, each holding its own stale snapshot, must not clobber
+// a key the *other* tab changed. setSettings() is given `previous` (this
+// tab's own pre-update snapshot) and `next` (its whole-object update) and must
+// apply only the keys that actually differ, on top of the freshest stored blob
+// — not overwrite the blob wholesale with its own stale view.
+test('setSettings merges only the changed key onto the freshest stored blob', () => {
+  installStorage();
+  const original = getSettings();
+
+  // Tab A reads settings, then tab B (concurrently) changes an unrelated key
+  // and writes it to the shared storage.
+  const tabB = { ...original, gifPickerEnabled: true };
+  setSettings(original, tabB);
+
+  // Tab A, still holding its original snapshot, now changes a different key.
+  const tabA = { ...original, isMarkdown: !original.isMarkdown };
+  setSettings(original, tabA);
+
+  // Both survive: tab B's change was not reverted by tab A's stale write.
+  const result = getSettings();
+  assert.equal(result.gifPickerEnabled, true);
+  assert.equal(result.isMarkdown, !original.isMarkdown);
+});
+
+test('setSettings is a no-op for keys unchanged relative to previous', () => {
+  const store = installStorage();
+  store.set('settings', JSON.stringify({ isMarkdown: false }));
+  const current = getSettings();
+  assert.equal(current.isMarkdown, false);
+
+  // `next` differs from `current` in object identity/composerToolbarButtons
+  // etc. but not in any actual value vs `previous` — nothing should be
+  // reapplied, so an out-of-band external change to storage survives.
+  const externallyChanged: Settings = { ...current, isMarkdown: true };
+  localStorage.setItem('settings', JSON.stringify(externallyChanged));
+
+  const previous = current;
+  const next = { ...current }; // identical values to `previous`
+  setSettings(previous, next);
+
+  assert.equal(getSettings().isMarkdown, true);
+});

src/app/state/settings.ts

@@ -473,19 +473,50 @@ export const getSettings = (): Settings => {
   }
 };
 
-export const setSettings = (settings: Settings) => {
+// Gitea #42 — merge-on-write. `next` is always this tab's whole-object view
+// (a shallow copy of what it last read, with one key changed — see
+// useSetSetting), which is stale the moment another tab has written since. Re-
+// reading the stored blob and reapplying only the keys that actually changed
+// relative to `previous` (this tab's own pre-update snapshot) means a change
+// this tab didn't make — e.g. a different setting toggled in another tab —
+// survives instead of being clobbered by this tab's stale snapshot of it.
+export const setSettings = (previous: Settings, next: Settings) => {
   try {
-    localStorage.setItem(STORAGE_KEY, JSON.stringify(settings));
+    const stored = getSettings();
+    const merged: Settings = { ...stored };
+    (Object.keys(next) as (keyof Settings)[]).forEach((key) => {
+      if (next[key] !== previous[key]) {
+        (merged as Record<keyof Settings, unknown>)[key] = next[key];
+      }
+    });
+    localStorage.setItem(STORAGE_KEY, JSON.stringify(merged));
   } catch {
     /* quota */
   }
 };
 
 const baseSettings = atom<Settings>(getSettings());
+// Gitea #42 — settingsAtom used to be a one-shot snapshot with no cross-tab
+// sync at all (unlike atomWithLocalStorage.ts, which this mirrors). Without
+// this, a tab left open never sees settings changed in another tab until it
+// reloads, and (before the merge-on-write above) its next save would revert
+// them.
+baseSettings.onMount = (setAtom) => {
+  const handleStorageChange = (evt: StorageEvent) => {
+    if (evt.key !== STORAGE_KEY) return;
+    setAtom(getSettings());
+  };
+  window.addEventListener('storage', handleStorageChange);
+  return () => {
+    window.removeEventListener('storage', handleStorageChange);
+  };
+};
+
 export const settingsAtom = atom<Settings, [Settings], undefined>(
   (get) => get(baseSettings),
   (get, set, update) => {
+    const previous = get(baseSettings);
     set(baseSettings, update);
-    setSettings(update);
+    setSettings(previous, update);
   },
 );