Merge pull request #160 from Coffey-Labs/i18n-switch-and-translate-prompt

Stop a language change undoing itself, and stop the translate prompt

web/src/App.tsx

@@ -18,7 +18,7 @@ import { MailView } from "@/views/mail/MailView";
 import { ComposerDock } from "@/views/compose/ComposerDock";
 import { setUnreadBadge } from "@/lib/notify";
 import { useSettings, syncedPart } from "@/store/settings";
-import { armSettingsSync, loadRemoteSettings, queueSettingsPush, settingsSyncAvailable } from "@/lib/settingsSync";
+import { armSettingsSync, loadRemoteSettings, queueSettingsPush, settingsAlreadyLoadedFor, settingsSyncAvailable } from "@/lib/settingsSync";
 import { listenForVerification, renewWebPush } from "@/lib/webpushEnable";
 import { useLanguageVersion } from "@/lib/i18n";
 
@@ -68,8 +68,13 @@ function AuthedApp() {
 
   // Settings that live with the account rather than the browser. The cached
   // ones have already painted, so this only has to correct them (issue #54).
+  //
+  // 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.
   useEffect(() => {
-    if (!accountId) return;
+    if (settingsAlreadyLoadedFor(accountId)) return;
     let cancelled = false;
     void (async () => {
       const remote = await loadRemoteSettings();

web/src/lib/__tests__/settingsSync.test.ts

@@ -1,6 +1,7 @@
 import { describe, expect, it } from "vitest";
-import { DEFAULT_SETTINGS, DEVICE_KEYS, acceptRemote, syncedPart, type Settings } from "@/store/settings";
+import { DEFAULT_SETTINGS, DEVICE_KEYS, acceptRemote, mergeRemote, syncedPart, type Settings } from "@/store/settings";
 import { isAppFolder } from "../appFolder";
+import { settingsAlreadyLoadedFor, stopSettingsSync } from "../settingsSync";
 
 /**
  * Settings used to live only in localStorage, so nothing followed the user
@@ -81,3 +82,47 @@ describe("the client's own folder", () => {
     expect(isAppFolder({ name: "ihasmail", parentId: null, nodeType: "file" })).toBe(false);
   });
 });
+
+/**
+ * Picking a language used to come undone.
+ *
+ * The subtree that reads the account's settings file is keyed on the language
+ * version, so choosing a language throws it away and builds it again. The
+ * remount re-read the file — which still held the old language, because the
+ * write is debounced by three seconds — and applied it, putting the old
+ * language back. Reported as "sometimes it takes several clicks": the click
+ * that appeared to work was the one made after the previous write had landed.
+ */
+describe("a change made but not yet written up", () => {
+  it("is not read back over by the file it has not reached yet", () => {
+    const current: Settings = { ...DEFAULT_SETTINGS, uiLanguage: "ja" };
+    const file = { uiLanguage: "en", theme: "dark" };
+    const merged = mergeRemote(current, file, new Set(["uiLanguage"]));
+    expect(merged.uiLanguage).toBe("ja");
+    // Only the queued key is held back; the rest of the file still applies.
+    expect(merged.theme).toBe("dark");
+  });
+
+  it("applies the whole file when nothing is queued", () => {
+    const current: Settings = { ...DEFAULT_SETTINGS, uiLanguage: "ja" };
+    const merged = mergeRemote(current, { uiLanguage: "en" });
+    expect(merged.uiLanguage).toBe("en");
+  });
+
+  it("reads the file again for an account after a sign-out", () => {
+    stopSettingsSync();
+    expect(settingsAlreadyLoadedFor("a1")).toBe(false);
+    // The remount that a language change causes must not read it a second time.
+    expect(settingsAlreadyLoadedFor("a1")).toBe(true);
+    // 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);
+    stopSettingsSync();
+  });
+
+  it("treats a missing account as already loaded, so nothing is fetched", () => {
+    expect(settingsAlreadyLoadedFor(null)).toBe(true);
+    expect(settingsAlreadyLoadedFor(undefined)).toBe(true);
+  });
+});

web/src/lib/settingsSync.ts

@@ -33,6 +33,7 @@ let pending: Record<string, unknown> | null = null;
 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 listenersBound = false;
 
 export function settingsSyncAvailable(): boolean {
@@ -62,6 +63,24 @@ export async function loadRemoteSettings(): Promise<Record<string, unknown> | nu
   }
 }
 
+/**
+ * Has this account's settings file already been read on this page load?
+ *
+ * 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.
+ *
+ * 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;
+  loadedFor = accountId;
+  return false;
+}
+
 /** Allow pushes. Called once the first load has settled, either way. */
 export function armSettingsSync(): void {
   armed = true;
@@ -71,6 +90,7 @@ export function armSettingsSync(): void {
 /** Stop syncing and drop anything queued (logout). */
 export function stopSettingsSync(): void {
   armed = false;
+  loadedFor = null;
   pending = null;
   if (timer !== null) {
     window.clearTimeout(timer);
@@ -93,6 +113,20 @@ export function queueSettingsPush(synced: Record<string, unknown>): void {
   }, DEBOUNCE_MS);
 }
 
+/**
+ * The keys of a change that has been made but not yet written up.
+ *
+ * `hydrate` needs these: a settings file read from the server is older than an
+ * unflushed local change by definition, so applying it wholesale hands the
+ * user back the value they just replaced. Switching language made that visible
+ * — it remounts the tree, the remount re-reads the file, and the file still
+ * says the old language — but the race is general and a slow read would lose
+ * any click made inside the debounce window.
+ */
+export function pendingSettingsKeys(): ReadonlySet<string> {
+  return new Set(pending ? Object.keys(pending) : []);
+}
+
 /** Write anything queued now, rather than waiting out the debounce. */
 export async function flushSettingsPush(): Promise<void> {
   if (timer !== null) {

web/src/store/settings.ts

@@ -1,7 +1,7 @@
 import { useEffect, useState } from "react";
 import { create } from "zustand";
 import { loadJson, saveJson } from "@/lib/storage";
-import { queueSettingsPush } from "@/lib/settingsSync";
+import { pendingSettingsKeys, queueSettingsPush } from "@/lib/settingsSync";
 import { setDateTimePrefs, setUiLanguageForFormatting, type DateFormat, type TimeFormat } from "@/lib/datetime";
 import type { SwipeAction } from "@/lib/swipe";
 import { resolveUiLanguage } from "@/lib/languages";
@@ -280,6 +280,26 @@ export function acceptRemote(remote: Record<string, unknown>): Partial<Settings>
   return out as Partial<Settings>;
 }
 
+/**
+ * The settings file laid over the ones in hand, minus anything still queued.
+ *
+ * A change that has not been written up yet is newer than the file by
+ * definition, so it wins. Picking a language is where this showed: that
+ * remounts the tree, the remount re-reads the file, and the file still holds
+ * the language from before the click, so the click came undone. Reported as
+ * "sometimes it takes several clicks" — the click that stuck was the one made
+ * after the previous write had landed.
+ */
+export function mergeRemote(
+  current: Settings,
+  remote: Record<string, unknown>,
+  held: ReadonlySet<string> = new Set(),
+): Settings {
+  const incoming = acceptRemote(remote);
+  for (const key of held) delete incoming[key as keyof Settings];
+  return { ...current, ...incoming };
+}
+
 interface SettingsState {
   settings: Settings;
   update(patch: Partial<Settings>): void;
@@ -333,7 +353,7 @@ export const useSettings = create<SettingsState>((set, get) => ({
     }
   },
   hydrate(remote) {
-    const settings = { ...get().settings, ...acceptRemote(remote) };
+    const settings = mergeRemote(get().settings, remote, pendingSettingsKeys());
     // Cache it, so the next first frame on this browser is already right.
     saveJson("settings", settings);
     set({ settings });

web/src/views/mail/MessageList.tsx

@@ -648,7 +648,7 @@ const Row = memo(function Row({ email: e, threadEmails, top, height, selected, f
       {twoLine ? (
         <div className="msg-body">
           <div className="msg-line1">
-            <span className="msg-from truncate">
+            <span className="msg-from truncate notranslate" translate="no">
               <span className="truncate">{who}</span>
               {count > 1 && <span className="thread-count"> {count}</span>}
             </span>
@@ -659,8 +659,8 @@ const Row = memo(function Row({ email: e, threadEmails, top, height, selected, f
           </div>
           <div className="msg-main">
             {isDrafts && <span style={{ color: "var(--danger)" }}>{t("Draft")}</span>}
-            <span className="msg-subject">{e.subject || "(no subject)"}</span>
-            {showPreview && <span className="msg-preview">{latest.preview}</span>}
+            <span className="msg-subject notranslate" translate="no">{e.subject || "(no subject)"}</span>
+            {showPreview && <span className="msg-preview notranslate" translate="no">{latest.preview}</span>}
             <button className={`msg-star ${starred ? "on" : ""}`} style={{ marginLeft: "auto" }} onClick={(ev) => { ev.stopPropagation(); onStar(e.id, !starred); }} aria-label={t("Star")}>
               <Star size={16} fill={starred ? "currentColor" : "none"} />
             </button>
@@ -669,15 +669,15 @@ const Row = memo(function Row({ email: e, threadEmails, top, height, selected, f
         </div>
       ) : (
         <>
-          <span className="msg-from" title={who}>
+          <span className="msg-from notranslate" translate="no" title={who}>
             <span className="truncate">{who}</span>
             {count > 1 && <span className="thread-count">{count}</span>}
           </span>
           <span className="msg-main">
             {isDrafts && <span style={{ color: "var(--danger)", flex: "0 0 auto" }}>{t("Draft")}</span>}
             {rowLabels.length > 0 && <span className="msg-labels">{rowLabels.map((l) => <span key={l.keyword} className="tag" style={{ background: l.color }}>{l.name}</span>)}</span>}
-            <span className="msg-subject">{e.subject || "(no subject)"}</span>
-            {showPreview && <span className="msg-preview">{latest.preview}</span>}
+            <span className="msg-subject notranslate" translate="no">{e.subject || "(no subject)"}</span>
+            {showPreview && <span className="msg-preview notranslate" translate="no">{latest.preview}</span>}
           </span>
           <span className="msg-meta">
             {(answered || forwarded) && <span className="msg-answered" title={answered ? "Replied" : "Forwarded"}>{answered ? <Reply size={14} /> : <Forward size={14} />}</span>}

web/src/views/mail/MessageView.tsx

@@ -149,7 +149,7 @@ export const MessageView = memo(function MessageView({ email: e, expanded, wasUn
         <Avatar who={from ?? null} />
         <div className="who">
           <div className="from" onContextMenu={(ev) => from && addrMenu.open(ev, from)}>
-            <span className="addr">{displayName(from)}</span>
+            <span className="addr notranslate" translate="no">{displayName(from)}</span>
             {/* An address, not a sentence. */}
             {expanded && from && <span className="email addr notranslate" translate="no">&lt;{from.email}&gt;</span>}
             {isHighPriority && <span className="tag" style={{ background: "var(--danger)" }}>{translate("Important")}</span>}

web/src/views/mail/ThreadView.tsx

@@ -260,7 +260,7 @@ export function ThreadView({ threadId, mailboxId, onBack, actions, onNavigate, h
       <div className="thread-scroll" ref={scrollRef}>
         <div className="thread-subject">
           <div className="grow">
-            <h1>{subject}</h1>
+            <h1 className="notranslate" translate="no">{subject}</h1>
             {(threadLabels.length > 0 || threadMailboxes.length > 0) && (
               <div className="labels">
                 {threadMailboxes.map((n) => <span key={n} className="chip">{n}</span>)}