Keep a settings change made before the first read, and wait for it
Two defects on the path that decides what language the app starts in. **A change made before the settings file came back was thrown away.** `queueSettingsPush` returned early while unarmed, dropping the value instead of holding it, so a language picked in the second or so after a page load was never written up: it survived until the next reload and no further. That is a better account of "sometimes it takes several clicks" than the remount race fixed in #160 — the click that stuck was one made after the read had finished. Keeping it is safe because hydrate already refuses to overwrite a key that is still queued. Proof it was real: before this, no `ihasmail` folder was ever created in the account's files, because the seed write never fired. After it, the folder appears. **Without a cached settings object the tree painted too early.** The cache is not read on an untrusted device, and it is cleared by the sign-out that every deploy causes, so in both cases the first frame is the defaults — and the defaults are English. Anything computed in that window is computed in the wrong language. The interface recovers, since it is rebuilt when the catalogue lands, but a string emitted once does not: this is why the stale-folder toast came out in English on an otherwise German screen. So without a cache the authenticated tree now waits for the account's settings and their catalogue, which costs nothing — there was nothing worth painting yet. With a cache it does not wait, and the first frame is as quick as it was. Neither fix makes the toast German yet: the account settings file is neither written nor read successfully in the mock, and both failures are swallowed. That is a third problem, and this commit does not touch it.
This commit is contained in:
+44
-10
@@ -17,7 +17,7 @@ import { AppShell } from "@/views/AppShell";
|
|||||||
import { MailView } from "@/views/mail/MailView";
|
import { MailView } from "@/views/mail/MailView";
|
||||||
import { ComposerDock } from "@/views/compose/ComposerDock";
|
import { ComposerDock } from "@/views/compose/ComposerDock";
|
||||||
import { setUnreadBadge } from "@/lib/notify";
|
import { setUnreadBadge } from "@/lib/notify";
|
||||||
import { useSettings, syncedPart } from "@/store/settings";
|
import { PAINTED_FROM_CACHE, useSettings, syncedPart } from "@/store/settings";
|
||||||
import { armSettingsSync, loadRemoteSettings, queueSettingsPush, settingsAlreadyLoadedFor, settingsSyncAvailable } from "@/lib/settingsSync";
|
import { armSettingsSync, loadRemoteSettings, queueSettingsPush, settingsAlreadyLoadedFor, settingsSyncAvailable } from "@/lib/settingsSync";
|
||||||
import { listenForVerification, renewWebPush } from "@/lib/webpushEnable";
|
import { listenForVerification, renewWebPush } from "@/lib/webpushEnable";
|
||||||
import { useLanguageVersion, whenLanguageReady } from "@/lib/i18n";
|
import { useLanguageVersion, whenLanguageReady } from "@/lib/i18n";
|
||||||
@@ -82,21 +82,45 @@ function AuthedApp() {
|
|||||||
const accountId = useSession((s) => s.accountId);
|
const accountId = useSession((s) => s.accountId);
|
||||||
const [location] = useLocation();
|
const [location] = useLocation();
|
||||||
|
|
||||||
// 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).
|
* Settings that live with the account rather than the browser.
|
||||||
//
|
*
|
||||||
// Once per account, not once per mount: this subtree is keyed on the
|
* When this browser has them cached they have already painted, and this only
|
||||||
// language version, so picking a language throws it away and builds it
|
* has to correct them (issue #54). When it does not -- an untrusted device,
|
||||||
// again. Re-reading the settings file there would apply a copy written
|
* or the sign-out that every deploy causes -- the first frame is the
|
||||||
// before the change and undo it.
|
* defaults, and the defaults are English. Rendering then means anything
|
||||||
|
* computed before the settings land is computed in the wrong language: not
|
||||||
|
* the interface, which is rebuilt when the catalogue arrives, but a string
|
||||||
|
* emitted once, like a toast. That is why the stale-folder toast came out
|
||||||
|
* in English on an otherwise German screen.
|
||||||
|
*
|
||||||
|
* So without a cache the tree waits, which costs nothing: there was nothing
|
||||||
|
* worth painting yet. With one it does not wait, and the screen is as quick
|
||||||
|
* as it was.
|
||||||
|
*
|
||||||
|
* 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.
|
||||||
|
*/
|
||||||
|
const [ready, setReady] = useState(PAINTED_FROM_CACHE);
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
if (settingsAlreadyLoadedFor(accountId)) return;
|
if (settingsAlreadyLoadedFor(accountId)) {
|
||||||
|
setReady(true);
|
||||||
|
return;
|
||||||
|
}
|
||||||
let cancelled = false;
|
let cancelled = false;
|
||||||
void (async () => {
|
void (async () => {
|
||||||
const remote = await loadRemoteSettings();
|
const remote = await loadRemoteSettings();
|
||||||
if (cancelled) return;
|
if (cancelled) return;
|
||||||
if (remote) useSettings.getState().hydrate(remote);
|
if (remote) useSettings.getState().hydrate(remote);
|
||||||
// Pushes were held back until now so they could not race the load.
|
// The catalogue for whatever language that turned out to be. Hydrating
|
||||||
|
// asks for it; this is waiting for the answer.
|
||||||
|
await whenLanguageReady();
|
||||||
|
if (cancelled) return;
|
||||||
|
setReady(true);
|
||||||
|
// 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();
|
armSettingsSync();
|
||||||
// No file yet — seed one from what this browser has, so the next device
|
// 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.
|
// to sign in starts from these rather than from the defaults.
|
||||||
@@ -188,6 +212,16 @@ function AuthedApp() {
|
|||||||
if (notif) void import("@/lib/notify").then((m) => m.requestNotificationPermission());
|
if (notif) void import("@/lib/notify").then((m) => m.requestNotificationPermission());
|
||||||
}, [notif]);
|
}, [notif]);
|
||||||
|
|
||||||
|
// Nothing worth painting until the account's settings are in force; see the
|
||||||
|
// comment on `ready` above. With a cache this was true from the first frame.
|
||||||
|
if (!ready) {
|
||||||
|
return (
|
||||||
|
<div className="center" style={{ height: "100%" }}>
|
||||||
|
<Spinner size="lg" />
|
||||||
|
</div>
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<AppShell>
|
<AppShell>
|
||||||
<Suspense fallback={<Spinner size="lg" />}>
|
<Suspense fallback={<Spinner size="lg" />}>
|
||||||
|
|||||||
@@ -85,6 +85,8 @@ export function settingsAlreadyLoadedFor(accountId: string | null | undefined):
|
|||||||
export function armSettingsSync(): void {
|
export function armSettingsSync(): void {
|
||||||
armed = true;
|
armed = true;
|
||||||
bindFlushListeners();
|
bindFlushListeners();
|
||||||
|
// A change made while the read was in flight has been waiting for this.
|
||||||
|
if (pending) void flushSettingsPush();
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Stop syncing and drop anything queued (logout). */
|
/** Stop syncing and drop anything queued (logout). */
|
||||||
@@ -104,8 +106,22 @@ export function stopSettingsSync(): void {
|
|||||||
* one request goes out once the changes stop.
|
* one request goes out once the changes stop.
|
||||||
*/
|
*/
|
||||||
export function queueSettingsPush(synced: Record<string, unknown>): void {
|
export function queueSettingsPush(synced: Record<string, unknown>): void {
|
||||||
if (!armed || !settingsSyncAvailable()) return;
|
if (!settingsSyncAvailable()) return;
|
||||||
|
/*
|
||||||
|
* Held, not dropped, before the first load has settled.
|
||||||
|
*
|
||||||
|
* This used to return here, which silently threw the change away: a
|
||||||
|
* language picked in the second or so before the settings file came back
|
||||||
|
* was never written, so it survived until the next reload and no further.
|
||||||
|
* That is the other half of "sometimes it takes several clicks" -- the
|
||||||
|
* click that stuck was one made after the read had finished.
|
||||||
|
*
|
||||||
|
* Keeping it is safe because `hydrate` refuses to overwrite a key that is
|
||||||
|
* still queued, so the newer local change wins over the older file rather
|
||||||
|
* than racing it. `armSettingsSync` writes whatever is waiting.
|
||||||
|
*/
|
||||||
pending = synced;
|
pending = synced;
|
||||||
|
if (!armed) return;
|
||||||
if (timer !== null) window.clearTimeout(timer);
|
if (timer !== null) window.clearTimeout(timer);
|
||||||
timer = window.setTimeout(() => {
|
timer = window.setTimeout(() => {
|
||||||
timer = null;
|
timer = null;
|
||||||
|
|||||||
@@ -107,6 +107,25 @@ export function loadJson<T>(key: string, fallback: T): T {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Did `loadJson` have something to return, or did it hand back the fallback?
|
||||||
|
*
|
||||||
|
* The difference decides whether a first paint is worth anything. With a
|
||||||
|
* cached value the screen can be right immediately and the account's copy only
|
||||||
|
* has to correct it; without one -- an untrusted device, or the sign-out that
|
||||||
|
* every deploy causes -- the first paint is the defaults, and painting it
|
||||||
|
* before the account's settings arrive shows English to somebody who chose
|
||||||
|
* otherwise.
|
||||||
|
*/
|
||||||
|
export function hasCachedJson(key: string): boolean {
|
||||||
|
if (!trusted) return false;
|
||||||
|
try {
|
||||||
|
return localStorage.getItem(PREFIX + key) != null;
|
||||||
|
} catch {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
export function loadRaw<T>(key: string, fallback: T): T {
|
export function loadRaw<T>(key: string, fallback: T): T {
|
||||||
if (!trusted) return fallback;
|
if (!trusted) return fallback;
|
||||||
try {
|
try {
|
||||||
|
|||||||
@@ -1,6 +1,6 @@
|
|||||||
import { useEffect, useState } from "react";
|
import { useEffect, useState } from "react";
|
||||||
import { create } from "zustand";
|
import { create } from "zustand";
|
||||||
import { loadJson, saveJson } from "@/lib/storage";
|
import { hasCachedJson, loadJson, saveJson } from "@/lib/storage";
|
||||||
import { pendingSettingsKeys, queueSettingsPush } from "@/lib/settingsSync";
|
import { pendingSettingsKeys, queueSettingsPush } from "@/lib/settingsSync";
|
||||||
import { setDateTimePrefs, setUiLanguageForFormatting, type DateFormat, type TimeFormat } from "@/lib/datetime";
|
import { setDateTimePrefs, setUiLanguageForFormatting, type DateFormat, type TimeFormat } from "@/lib/datetime";
|
||||||
import type { SwipeAction } from "@/lib/swipe";
|
import type { SwipeAction } from "@/lib/swipe";
|
||||||
@@ -311,6 +311,17 @@ interface SettingsState {
|
|||||||
}
|
}
|
||||||
|
|
||||||
const initialSettings = loadJson<Settings>("settings", DEFAULT_SETTINGS);
|
const initialSettings = loadJson<Settings>("settings", DEFAULT_SETTINGS);
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Whether the first frame is this account's settings or merely the defaults.
|
||||||
|
*
|
||||||
|
* False after every deploy, because deploys sign everyone out and sign-out
|
||||||
|
* clears the cache -- and false on any untrusted device, where the cache is
|
||||||
|
* never read. In that state `uiLanguage` starts as English and only becomes
|
||||||
|
* the account's choice once the settings file lands, which is why the
|
||||||
|
* authenticated tree waits for it.
|
||||||
|
*/
|
||||||
|
export const PAINTED_FROM_CACHE = hasCachedJson("settings");
|
||||||
applyDateTimePrefs(initialSettings);
|
applyDateTimePrefs(initialSettings);
|
||||||
|
|
||||||
export const useSettings = create<SettingsState>((set, get) => ({
|
export const useSettings = create<SettingsState>((set, get) => ({
|
||||||
|
|||||||
Reference in New Issue
Block a user