From ceada3e11357f0c93de4a35621b17547221b0215 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Tue, 15 Sep 2026 21:32:06 -0400 Subject: [PATCH] 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 Claude-Session: https://claude.ai/code/session_01PPmy3tPq869XDW4njjVaKA --- src/app/state/settings.test.ts | 60 +++++++++++++++++++++++++++++++++- src/app/state/settings.ts | 37 +++++++++++++++++++-- 2 files changed, 93 insertions(+), 4 deletions(-) diff --git a/src/app/state/settings.test.ts b/src/app/state/settings.test.ts index 829f2418f..6bb2c71c7 100644 --- a/src/app/state/settings.test.ts +++ b/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 => { + const map = new Map(); + (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); +}); diff --git a/src/app/state/settings.ts b/src/app/state/settings.ts index 75a0ad696..78c2b46cb 100644 --- a/src/app/state/settings.ts +++ b/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)[key] = next[key]; + } + }); + localStorage.setItem(STORAGE_KEY, JSON.stringify(merged)); } catch { /* quota */ } }; const baseSettings = atom(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( (get) => get(baseSettings), (get, set, update) => { + const previous = get(baseSettings); set(baseSettings, update); - setSettings(update); + setSettings(previous, update); }, );