Finish the settings load when the tree remounts mid-load (ihasmail #29)

(cherry picked from commit 5ec06f40125cc320fba64edaa4ee1a1fb23d22d2)

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;
     };

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<void>((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<void>((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);
   });
 });

web/src/lib/settingsSync.ts

@@ -34,6 +34,9 @@ let inFlight: Promise<void> | 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<void> | 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<Record<string, unknown> | 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<void>,
+): Promise<void> {
+  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);