Two lost-work defects behind the preferences flake family (#333, #341)

docs/testing.md

@@ -969,6 +969,51 @@ The general form, across all three: **ask what writes the thing you are about to
 something that cannot be true before that write has happened.** "Near it", "usually after it", and
 "set when the work was requested" are all the same bug.
 
+### When waiting correctly is not enough, because the app is telling the renderer something untrue
+
+The three above are all the test's fault: it read something before the thing that writes it had run.
+The two below are **not**, and the distinction is worth holding onto, because the first instinct on
+meeting them is to add another wait — and no wait fixes either. The test waited correctly and was
+told a lie.
+
+Both surfaced in `preferences-reset.e2e.ts` chasing [#341](https://github.com/Bidthedog/throng/issues/341),
+and both turned out to be product defects that cost a user real work rather than harness problems.
+
+- **A broadcast can be older than the write it lands after.** A watcher read is not instantaneous —
+  it opens three documents and enumerates the icon-pack directory, and 032 FR-008 makes it retry for
+  up to ~100 ms when it catches a partial write. A config write can commit inside that window, so the
+  payload describes a file that has stopped saying it, and it reaches the renderer *after* the
+  renderer adopted the write that superseded it. The renderer cannot defend itself: `onChange`
+  replaces the whole state because a broadcast is supposed to *be* the truth, and nothing in the
+  payload ever said which moment it was the truth at.
+
+  The damage is not a stale render. The preferences tabs compose their next edit from what they were
+  last told, so the next whole-document write puts the reverted value **back on disk**. Remove a
+  chord from `zoom.in`, get reverted, remove one from `zoom.out`, and that second write restores
+  `zoom.in` to its shipped value. The Reset control then correctly reports the row as un-overridden,
+  which is why the failure presents as a click that never becomes actionable rather than as a wrong
+  value. `ConfigPayload.generation` now carries the store's commit count as at the read's *start*,
+  and a read the counter has outrun is never broadcast.
+
+- **A window that is interactive before it has loaded cannot honour "revert".** The preferences
+  editors rendered from the shipped defaults while `config.get()` was in flight. An edit made in that
+  gap is already in the payload that resolves the load, so the on-entry snapshot — captured on the
+  render where `loaded` first turns true — records the *edited* value, and "revert every editor to
+  its state when this window opened" restores the very thing the user was discarding. Retrying the
+  read cannot help: a re-read returns fresher content, never the state the window opened with.
+
+  The worse half never reached the tracker on its own: the Key Bindings tab composes **whole
+  documents** from what it currently holds, so one edit in that gap writes the default keybindings
+  over the user's real ones. The editors now wait for `loaded`; the toolbar does not.
+
+Measured, `preferences-reset.e2e.ts` alone, idle machine, one worker, retries off: **5 failed / 14**
+before, **1 / 14** after the first fix, **0 / 16** after the second — then a full `npm run gate` at
+560 E2E, 0 flaky.
+
+The general form of *these* two: **ask whether the thing telling you is entitled to be believed.**
+A payload with no notion of when it was true, and a window with no notion of what it opened with,
+are both saying "this is the state" while holding no evidence for it.
+
 ### Reproducing a flake: separate invocations, never `--repeat-each`
 
 The rule above says stress the one test until it fails on demand. **How you stress it decides whether

packages/ui/src/main/config-store.ts

@@ -92,6 +92,26 @@ export class FileConfigStore implements IConfigStore {
    *  to the same document never race on a shared `.tmp` (rapid theme edits). */
   private writeSeq = 0;
 
+  /**
+   * How many config writes this process has COMMITTED (#341, #333).
+   *
+   * The ordering token between the write path and the watcher's read, and it exists because those
+   * two are the only writers/readers of these files and neither could otherwise tell whose turn it
+   * was. A read that begins at generation N and finishes after the counter has moved was overtaken
+   * by a write, so the document it holds is already history — see `readConfigOnce`, which captures
+   * this before reading, and the store's consumers, which refuse to broadcast a payload the counter
+   * has outrun.
+   *
+   * Bumped on COMMIT rather than on entry, so a write that fails to land never makes a live read
+   * look stale. Monotonic and process-local: it is compared only against itself.
+   */
+  private commitGeneration = 0;
+
+  /** The number of committed config writes — see {@link commitGeneration}. */
+  get generation(): number {
+    return this.commitGeneration;
+  }
+
   /**
    * One in-flight chain per document path (031, FR-013c).
    *
@@ -203,6 +223,7 @@ export class FileConfigStore implements IConfigStore {
       tmp = `${path}.${(this.writeSeq += 1)}.tmp`; // unique per write (no shared-tmp race)
       await writeFile(tmp, FileConfigStore.serialize(value), 'utf8');
       await renameWithRetry(tmp, path); // atomic replace (libuv MoveFileEx w/ replace on Windows)
+      this.commitGeneration += 1;
       return { ok: true };
     } catch (err) {
       // Never throw (contract) — but never claim success either (issue #75): the caller decides
@@ -345,6 +366,7 @@ export class FileConfigStore implements IConfigStore {
     for (const s of snaps) {
       try {
         await rename(s.tmp, s.path);
+        this.commitGeneration += 1;
         committed.push(s);
       } catch (err) {
         await this.rollback(committed);

packages/ui/src/main/config-watcher.ts

@@ -35,6 +35,27 @@ export interface ConfigPayload {
    * (FR-005) rather than merely eventual.
    */
   iconPacks: LoadedIconPack[];
+  /**
+   * The store's commit generation as it was when this read STARTED (#341, #333).
+   *
+   * ══ WHAT IT IS FOR ══
+   *
+   * A watcher read is not instantaneous: it opens several files, and 032 FR-008 makes it retry for
+   * up to ~100 ms when it catches a partial write. A config write can therefore commit while a read
+   * is in progress, and the payload that read produces describes a file that no longer exists —
+   * while arriving AFTER the renderer has already adopted the write that superseded it.
+   *
+   * That is not a theoretical ordering. Measured on an idle machine, the Key Bindings tab lost an
+   * edit this way once in every seven or so attempts: the renderer reverted to the pre-write
+   * document, and the NEXT whole-document edit was then composed from it and wrote the reverted
+   * value back to disk.
+   *
+   * Captured before the read rather than after, which is the only order that is safe. Capturing
+   * afterwards would stamp a stale document with a fresh generation and make it look current;
+   * capturing first can only ever make a current document look stale, and the cost of that is one
+   * suppressed broadcast that the write's own file event immediately replaces.
+   */
+  generation: number;
 }
 
 /** A payload, plus whether the settings document could actually be read (032, FR-008). */
@@ -100,6 +121,8 @@ export async function readConfigOnce(
   store: IConfigStore,
   loadIconPacks: () => Promise<LoadedIconPack[]> = async () => [],
 ): Promise<ConfigReadResult> {
+  // BEFORE the first read, so a write that commits from here on makes this payload provably stale.
+  const generation = storeGeneration(store);
   const rawSettings = await store.readRaw({ kind: 'settings' });
   let settingsUnreadable = false;
   if (rawSettings.trim().length === 0) {
@@ -153,7 +176,32 @@ export async function readConfigOnce(
   );
   const keybindings = await store.read({ kind: 'keybindings' }, DEFAULT_KEYBINDINGS, parseKeybindings);
   const iconPacks = await loadIconPacks();
-  return { payload: { settings, theme, keybindings, iconPacks }, settingsUnreadable };
+  return { payload: { settings, theme, keybindings, iconPacks, generation }, settingsUnreadable };
+}
+
+/**
+ * The store's commit generation, for stores that keep one.
+ *
+ * Optional rather than part of `IConfigStore`, because the ordering token is an implementation
+ * concern of a store that writes files — an in-memory fake has no partial write to be overtaken by,
+ * and every test double in the repository would otherwise have to grow a counter it never reads.
+ * A store without one reports 0 forever, which never suppresses anything: the guard degrades to
+ * exactly the behaviour that shipped before it.
+ */
+function storeGeneration(store: IConfigStore): number {
+  const gen = (store as { generation?: unknown }).generation;
+  return typeof gen === 'number' ? gen : 0;
+}
+
+/**
+ * Whether `payload` was overtaken by a write while it was being read (#341, #333).
+ *
+ * The whole guard, in one comparison. A caller that broadcasts a payload this returns true for is
+ * telling every window that the file says something it stopped saying — and, because the Preferences
+ * tabs compose their next edit from what they were last told, that lie is then written back to disk.
+ */
+export function isSupersededPayload(store: IConfigStore, payload: ConfigPayload): boolean {
+  return storeGeneration(store) > payload.generation;
 }
 
 /**
@@ -206,7 +254,21 @@ export function startConfigWatcher(deps: {
 }): Disposable {
   return deps.watcher.watch(deps.config.configRoot, () => {
     void readConfigWithRetry(deps.store, deps.policy ?? DEFAULT_CONFIG_WATCH_POLICY, deps.loadIconPacks).then(
-      (result) => deps.broadcast(result.payload),
+      (result) => {
+        /*
+         * Drop a read a write overtook (#341, #333).
+         *
+         * Checked HERE, at the moment of sending, rather than when the read returned: the read and
+         * the broadcast are separated by the retry policy and by the icon-pack load, and the write
+         * we are racing can commit anywhere in that gap. Testing as late as possible makes the
+         * remaining window a single synchronous step.
+         *
+         * Nothing is lost by dropping it. The write that superseded this read changed a file the
+         * watcher is watching, so it has already queued the event whose read WILL be current.
+         */
+        if (isSupersededPayload(deps.store, result.payload)) return;
+        deps.broadcast(result.payload);
+      },
     );
   });
 }

packages/ui/src/renderer/preferences/preferences-app.tsx

@@ -328,7 +328,29 @@ function PreferencesShell({ initialTab }: { initialTab: Tab }): ReactElement {
               scrollTops.current[tab] = e.currentTarget.scrollTop;
             }}
           >
-            {mode === 'json' ? (
+            {/*
+              Nothing is editable until the configuration has actually loaded (#341).
+
+              The window renders from the SHIPPED DEFAULTS while `config.get()` is in flight, and
+              that read is a genuine round trip — it re-reads three documents and enumerates the
+              icon-pack directory — so the gap is not theoretical. Two things went wrong inside it,
+              and neither is recoverable afterwards:
+
+              - **The on-entry snapshot is captured on the render where `loaded` first turns true.**
+                An edit made before then is already in the payload that resolves the load, so the
+                snapshot records the EDITED value and "revert to how this window opened" restores
+                the very thing the user was discarding. `preferences-reset.e2e.ts:217` fails exactly
+                this way — a poll for `false` that reads `true` for its whole budget.
+              - **The Key Bindings tab composes WHOLE documents from what it currently holds.** Held
+                before the load, that is the shipped defaults, so one edit would write the default
+                keybindings over the user's real ones.
+
+              The window cannot repair either after the fact: it never saw the state it would have
+              to revert to. So it must not accept the edit in the first place, which is what this
+              gate does. In practice the read resolves in a few milliseconds and nothing is visible;
+              what it removes is the window between the frame being interactive and it being right.
+            */}
+            {!loaded ? null : mode === 'json' ? (
               <JsonTab
                 docId={
                   tab === 'settings'

packages/ui/tests/component/preferences-on-entry-snapshot.test.ts

@@ -0,0 +1,157 @@
+import { render, screen, waitFor } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import { createElement } from 'react';
+import { afterEach, describe, expect, it, vi } from 'vitest';
+import { applyConfigPatch, DEFAULT_APP_SETTINGS, DEFAULT_KEYBINDINGS, THRONG_THEME } from '@throng/core';
+import { PreferencesApp } from '../../src/renderer/preferences/preferences-app.js';
+
+/**
+ * `preferences-reset.e2e.ts:217` — "reset-all reverts the session to on-entry" (#341).
+ *
+ * ══ THE DEFECT ══
+ *
+ * The window's on-entry snapshot is captured on the render where `useConfigLoaded()` FIRST reports
+ * true, and `loaded` is set by whichever payload arrives first — `config.get()` at mount, or a
+ * watcher broadcast. Neither is ordered against the user, and the window is interactive before
+ * either arrives: the settings tab renders from the shipped defaults immediately.
+ *
+ * So a user who opens Preferences and changes something before the config has loaded gets a
+ * snapshot taken AFTER their edit. "Revert every editor to its state when this window opened" then
+ * reverts to the edited state, which is to say it does nothing, for ever — and `planRevertAll`
+ * emits a change for every revertable leaf rather than diffing, so it is not that the write is
+ * skipped; it is that the value written is the one the user was trying to get rid of.
+ *
+ * That is the E2E's exact symptom: a poll for `false` that returns `true` for its whole 30s budget,
+ * in a test that otherwise finishes in a second. The adoption path (`onConfigWritten`) never sets
+ * `loaded`, which is what lets the edit land first while the window still believes it has not
+ * opened yet.
+ *
+ * ══ WHY THIS LAYER ══
+ *
+ * `PreferencesApp` mounts its own providers and needs only `window.throng.config`, so the ordering
+ * that is a race in the E2E is a decision here: the bridge below simply does not resolve `get()`
+ * until the test says so.
+ */
+vi.mock('../../src/renderer/editor/standalone-editor.js', async () => {
+  const { createElement: h } = await import('react');
+  return {
+    StandaloneEditor: ({
+      value,
+      onChange,
+      testId,
+    }: {
+      value: string;
+      onChange: (v: string) => void;
+      testId?: string;
+    }) =>
+      h('textarea', {
+        'data-testid': testId ?? 'json-editor',
+        value,
+        onChange: (e: { target: { value: string } }) => onChange(e.target.value),
+      }),
+  };
+});
+
+const CFG = { appearance: { theme: 'throng' } };
+
+afterEach(() => {
+  Reflect.deleteProperty(window, 'throng');
+});
+
+/** The preferences window over a bridge whose `get()` resolves only when the test releases it. */
+function mountWithDeferredLoad(editorOverrides: Record<string, unknown> = {}) {
+  const patched: Array<{ id: unknown; changes: unknown }> = [];
+  let settings: unknown = {
+    ...DEFAULT_APP_SETTINGS,
+    ...CFG,
+    editor: { ...DEFAULT_APP_SETTINGS.editor, ...editorOverrides },
+  };
+  const theme = structuredClone(THRONG_THEME);
+  let push: ((payload: unknown) => void) | null = null;
+
+  let release!: () => void;
+  const gate = new Promise<void>((resolve) => {
+    release = resolve;
+  });
+
+  Reflect.set(window, 'throng', {
+    config: {
+      // Resolves with whatever the document says AT THE MOMENT IT IS RELEASED — which is what a
+      // real read does when it happens to run after the user's first edit has landed.
+      get: () => gate.then(() => ({ settings, theme, keybindings: DEFAULT_KEYBINDINGS })),
+      onChange: (cb: (payload: unknown) => void) => {
+        push = cb;
+        return () => {
+          push = null;
+        };
+      },
+      write: (id: unknown, json: string) => {
+        try {
+          // DOCUMENT-ADDRESSED. Revert All writes keybindings and each captured theme through this
+          // same call, so parsing every one of them into `settings` would replace the settings
+          // document with a Keybindings and make every row read `undefined`.
+          const kind = (id as { kind?: string } | undefined)?.kind;
+          if (kind === undefined || kind === 'settings') settings = JSON.parse(json);
+          push?.({ settings, theme, keybindings: DEFAULT_KEYBINDINGS });
+        } catch {
+          /* an invalid document should never reach the write path */
+        }
+        return Promise.resolve({ ok: true });
+      },
+      writePatch: (id: unknown, changes: unknown) => {
+        patched.push({ id, changes });
+        const applied = applyConfigPatch(settings as never, changes as never);
+        if (applied.ok) {
+          settings = applied.value;
+          push?.({ settings, theme, keybindings: DEFAULT_KEYBINDINGS });
+        }
+        return Promise.resolve({ ok: true });
+      },
+      listThemes: () => Promise.resolve(['throng']),
+      listFonts: () => Promise.resolve([]),
+      listIconPacks: () => Promise.resolve([]),
+    },
+  });
+
+  render(createElement(PreferencesApp, { initialTab: 'settings' as const }));
+  return {
+    patched,
+    releaseLoad: release,
+    autoSave: () => (settings as { editor: { autoSave: boolean } }).editor.autoSave,
+  };
+}
+
+describe('the on-entry snapshot when the config loads late (#341, preferences-reset.e2e.ts:217)', () => {
+  it('accepts no edit until the configuration has loaded', async () => {
+    /*
+     * The gate itself. Before this, the tab rendered from the shipped defaults while `config.get()`
+     * was still in flight, so the window was interactive while it was still wrong — and an edit
+     * made there is unrecoverable, because the window never saw the state it would have to revert
+     * to. The toolbar stays mounted; it is the EDITORS that wait.
+     */
+    mountWithDeferredLoad();
+    expect(screen.queryByTestId('settings-tab')).toBeNull();
+    expect(screen.queryByTestId('control-editor.autoSave')).toBeNull();
+  });
+
+  it('reverts to the value the window opened with, not to one edited after it', async () => {
+    const user = userEvent.setup();
+    // The user's REAL saved setting — the thing "revert to how this window opened" owes back.
+    const { patched, releaseLoad, autoSave } = mountWithDeferredLoad({ autoSave: true });
+
+    releaseLoad();
+    await waitFor(() => expect(screen.getByTestId('settings-tab')).toBeInTheDocument());
+    expect(autoSave()).toBe(true);
+
+    // Now edit it, the way the E2E does.
+    await user.click(screen.getByTestId('control-editor.autoSave'));
+    await waitFor(() => expect(autoSave()).toBe(false));
+
+    // "Revert every editor to its state when this window opened."
+    await user.click(screen.getByTestId('prefs-revert-all'));
+    await user.click(screen.getByTestId('prefs-reset-confirm-yes'));
+
+    await waitFor(() => expect(patched.length).toBeGreaterThan(0));
+    await waitFor(() => expect(autoSave()).toBe(true));
+  });
+});