diff --git a/web/src/App.tsx b/web/src/App.tsx index 3e77187..8fb91d3 100644 --- a/web/src/App.tsx +++ b/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, whenLanguageReady } from "@/lib/i18n"; @@ -84,8 +84,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(); diff --git a/web/src/lib/__tests__/settingsSync.test.ts b/web/src/lib/__tests__/settingsSync.test.ts index bb2f9be..6d7c296 100644 --- a/web/src/lib/__tests__/settingsSync.test.ts +++ b/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); + }); +}); diff --git a/web/src/lib/settingsSync.ts b/web/src/lib/settingsSync.ts index fe5066b..19b46aa 100644 --- a/web/src/lib/settingsSync.ts +++ b/web/src/lib/settingsSync.ts @@ -33,6 +33,7 @@ let pending: Record | null = null; 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 listenersBound = false; export function settingsSyncAvailable(): boolean { @@ -62,6 +63,24 @@ export async function loadRemoteSettings(): Promise | 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): 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 { + return new Set(pending ? Object.keys(pending) : []); +} + /** Write anything queued now, rather than waiting out the debounce. */ export async function flushSettingsPush(): Promise { if (timer !== null) { diff --git a/web/src/store/settings.ts b/web/src/store/settings.ts index 80975f8..dc3ecf5 100644 --- a/web/src/store/settings.ts +++ b/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): Partial return out as Partial; } +/** + * 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, + held: ReadonlySet = 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): void; @@ -333,7 +353,7 @@ export const useSettings = create((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 }); diff --git a/web/src/views/mail/MessageList.tsx b/web/src/views/mail/MessageList.tsx index 75c3f04..8d3eff6 100644 --- a/web/src/views/mail/MessageList.tsx +++ b/web/src/views/mail/MessageList.tsx @@ -648,7 +648,7 @@ const Row = memo(function Row({ email: e, threadEmails, top, height, selected, f {twoLine ? (
- + {who} {count > 1 && {count}} @@ -659,8 +659,8 @@ const Row = memo(function Row({ email: e, threadEmails, top, height, selected, f
{isDrafts && {t("Draft")}} - {e.subject || t("(no subject)")} - {showPreview && {latest.preview}} + {e.subject || t("(no subject)")} + {showPreview && {latest.preview}} @@ -669,15 +669,15 @@ const Row = memo(function Row({ email: e, threadEmails, top, height, selected, f
) : ( <> - + {who} {count > 1 && {count}} {isDrafts && {t("Draft")}} {rowLabels.length > 0 && {rowLabels.map((l) => {l.name})}} - {e.subject || t("(no subject)")} - {showPreview && {latest.preview}} + {e.subject || t("(no subject)")} + {showPreview && {latest.preview}} {(answered || forwarded) && {answered ? : }} diff --git a/web/src/views/mail/MessageView.tsx b/web/src/views/mail/MessageView.tsx index 16ed8a3..8b968fe 100644 --- a/web/src/views/mail/MessageView.tsx +++ b/web/src/views/mail/MessageView.tsx @@ -149,7 +149,7 @@ export const MessageView = memo(function MessageView({ email: e, expanded, wasUn
from && addrMenu.open(ev, from)}> - {displayName(from)} + {displayName(from)} {/* An address, not a sentence. */} {expanded && from && <{from.email}>} {isHighPriority && {translate("Important")}} diff --git a/web/src/views/mail/ThreadView.tsx b/web/src/views/mail/ThreadView.tsx index 1ec2cad..365ad5a 100644 --- a/web/src/views/mail/ThreadView.tsx +++ b/web/src/views/mail/ThreadView.tsx @@ -260,7 +260,7 @@ export function ThreadView({ threadId, mailboxId, onBack, actions, onNavigate, h
-

{subject}

+

{subject}

{(threadLabels.length > 0 || threadMailboxes.length > 0) && (
{threadMailboxes.map((n) => {n})}