diff --git a/web/src/App.tsx b/web/src/App.tsx index d1f5765..1270162 100644 --- a/web/src/App.tsx +++ b/web/src/App.tsx @@ -19,7 +19,7 @@ import { ComposerDock } from "@/views/compose/ComposerDock"; import { requestNotificationPermission, setBaseTitle, setUnreadBadge } from "@/lib/notify/notify"; import { publishWorkerFacts } from "@/lib/sw/swFacts"; import { PAINTED_FROM_CACHE, useSettings, syncedPart } from "@/store/settings"; -import { armSettingsSync, loadRemoteSettings, queueSettingsPush, settingsAlreadyLoadedFor, settingsSyncAvailable } from "@/lib/settingsSync"; +import { armSettingsSync, loadRemoteSettings, loadSettingsOnce, queueSettingsPush, settingsSyncAvailable } from "@/lib/settingsSync"; import { loadSettingsPolicy } from "@/lib/settingsPolicy"; import { listenForVerification, renewWebPush } from "@/lib/notify/webpushEnable"; import { plural, t, useLanguageVersion, whenLanguageReady } from "@/lib/i18n"; @@ -141,22 +141,22 @@ function AuthedApp() { * Once per account, not once per mount: this subtree is keyed on the * language version, so picking a language throws it away and builds it * again. Re-reading the settings file there would apply a copy written - * before the change and undo it. + * before the change and undo it. And the load is not this mount's to + * cancel: the settings file choosing a language remounts the tree midway + * through it, and a load cut off there never armed the pushes, so nothing + * changed afterwards was saved (Gitea issue #23). `loadSettingsOnce` runs + * it to the end and lets every mount wait on it. */ const [ready, setReady] = useState(PAINTED_FROM_CACHE); useEffect(() => { - if (settingsAlreadyLoadedFor(accountId)) { - setReady(true); - return; - } let canceled = false; - void (async () => { + void loadSettingsOnce(accountId, async (isCurrent) => { /* Before the account's own settings, so both the seeding below and the enforcement inside `hydrate` have something to apply. */ await loadSettingsPolicy(); - if (canceled) return; + if (!isCurrent()) return; const remote = await loadRemoteSettings(); - if (canceled) return; + if (!isCurrent()) return; if (remote) useSettings.getState().hydrate(remote); // No settings file: this account has never had settings of its own, so // the installation's defaults are what it starts on rather than @@ -178,15 +178,16 @@ function AuthedApp() { // The catalog for whatever language that turned out to be. Hydrating // asks for it; this is waiting for the answer. await whenLanguageReady(); - if (canceled) return; - setReady(true); + if (!isCurrent()) return; // Pushes were held back until now so they could not race the load. A // change made while it was in flight was kept, and goes out here. armSettingsSync(); // No file yet — seed one from what this browser has, so the next device // to sign in starts from these rather than from the defaults. if (!remote && settingsSyncAvailable()) queueSettingsPush(syncedPart(useSettings.getState().settings)); - })(); + }).then(() => { + if (!canceled) setReady(true); + }); return () => { canceled = true; }; diff --git a/web/src/lib/__tests__/settingsSync.test.ts b/web/src/lib/__tests__/settingsSync.test.ts index 283efc0..892112b 100644 --- a/web/src/lib/__tests__/settingsSync.test.ts +++ b/web/src/lib/__tests__/settingsSync.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from "vitest"; import { DEFAULT_SETTINGS, DEVICE_KEYS, acceptRemote, mergeRemote, syncedPart, type Settings } from "@/store/settings"; import { isAppFolder } from "../appFolder"; -import { settingsAlreadyLoadedFor, stopSettingsSync } from "../settingsSync"; +import { loadSettingsOnce, stopSettingsSync } from "../settingsSync"; /** * Settings used to live only in localStorage, so nothing followed the user @@ -116,20 +116,67 @@ describe("a change made but not yet written up", () => { expect(merged.uiLanguage).toBe("en"); }); - it("reads the file again for an account after a sign-out", () => { + it("reads the file again for an account after a sign-out", async () => { stopSettingsSync(); - expect(settingsAlreadyLoadedFor("a1")).toBe(false); + let loads = 0; + const load = async () => { loads++; }; + await loadSettingsOnce("a1", load); // The remount that a language change causes must not read it a second time. - expect(settingsAlreadyLoadedFor("a1")).toBe(true); + await loadSettingsOnce("a1", load); + expect(loads).toBe(1); // Signing out drops the claim, so signing back in reads the file rather // than trusting whatever the previous session left behind. stopSettingsSync(); - expect(settingsAlreadyLoadedFor("a1")).toBe(false); + await loadSettingsOnce("a1", load); + expect(loads).toBe(2); stopSettingsSync(); }); - it("treats a missing account as already loaded, so nothing is fetched", () => { - expect(settingsAlreadyLoadedFor(null)).toBe(true); - expect(settingsAlreadyLoadedFor(undefined)).toBe(true); + it("finishes a load that a remount interrupts, so changes are saved (Gitea #23)", async () => { + stopSettingsSync(); + let release!: () => void; + const gate = new Promise((r) => { release = r; }); + let loads = 0; + let armed = false; + const load = async (isCurrent: () => boolean) => { + loads++; + await gate; + if (!isCurrent()) return; + armed = true; + }; + // First mount starts the load; the settings file picks a language and the + // tree remounts before the load has finished. + const first = loadSettingsOnce("a1", load); + const second = loadSettingsOnce("a1", load); + expect(second).toBe(first); + release(); + await second; + expect(loads).toBe(1); + // This is what used to be skipped: the remounted tree took the + // already-loaded path and nothing armed the pushes. + expect(armed).toBe(true); + stopSettingsSync(); + }); + + it("stops a load that a sign-out overtakes", async () => { + stopSettingsSync(); + let release!: () => void; + const gate = new Promise((r) => { release = r; }); + let applied = false; + const pending = loadSettingsOnce("a1", async (isCurrent) => { + await gate; + if (isCurrent()) applied = true; + }); + stopSettingsSync(); + release(); + await pending; + expect(applied).toBe(false); + }); + + it("treats a missing account as already loaded, so nothing is fetched", async () => { + let loads = 0; + await loadSettingsOnce(null, async () => { loads++; }); + await loadSettingsOnce(undefined, async () => { loads++; }); + expect(loads).toBe(0); }); }); diff --git a/web/src/lib/settingsSync.ts b/web/src/lib/settingsSync.ts index 9bf1f0b..471139f 100644 --- a/web/src/lib/settingsSync.ts +++ b/web/src/lib/settingsSync.ts @@ -34,6 +34,9 @@ let inFlight: Promise | null = null; /** Nothing is pushed before the first load has settled, or we would race it. */ let armed = false; let loadedFor: string | null = null; +let loading: Promise | null = null; +/** Bumped by every new load and every sign-out; a load checks it is still the latest. */ +let generation = 0; let listenersBound = false; export function settingsSyncAvailable(): boolean { @@ -64,21 +67,34 @@ export async function loadRemoteSettings(): Promise | nu } /** - * Has this account's settings file already been read on this page load? + * Load this account's settings, once per page load, however many times the + * caller mounts. * - * Claims the account as a side effect, so two callers cannot both start a - * read. The subtree that does the reading is keyed on the language version - * and so is deliberately remounted whenever somebody picks a language; - * without this the remount re-reads a file written before the change and - * applies it, putting the old language back. + * The subtree that does the reading is keyed on the language version, so it + * is remounted whenever the language changes -- including when the settings + * file itself picks one. Tying the load to a mount meant that remount canceled + * it halfway: the new mount saw the account already claimed and skipped the + * load, and nothing armed the pushes, so every later change was silently + * dropped (Gitea issue #23). Now the load belongs to the account, not the + * mount: every mount waits on the same promise, and the load runs to the end. * + * Re-reading on a remount is still wrong -- it would apply a file written + * before the change and undo it -- which is why it is shared, not repeated. + * + * `isCurrent` turns false once the session that started the load signs out, + * so a load overtaken by a sign-out stops instead of applying stale settings. * Cleared by `stopSettingsSync`, so signing out and back in reads again. */ -export function settingsAlreadyLoadedFor(accountId: string | null | undefined): boolean { - if (!accountId) return true; - if (loadedFor === accountId) return true; +export function loadSettingsOnce( + accountId: string | null | undefined, + load: (isCurrent: () => boolean) => Promise, +): Promise { + if (!accountId) return Promise.resolve(); + if (loadedFor === accountId && loading) return loading; loadedFor = accountId; - return false; + const mine = ++generation; + loading = load(() => generation === mine); + return loading; } /** Allow pushes. Called once the first load has settled, either way. */ @@ -93,6 +109,8 @@ export function armSettingsSync(): void { export function stopSettingsSync(): void { armed = false; loadedFor = null; + loading = null; + generation++; pending = null; if (timer !== null) { window.clearTimeout(timer);