fix(web): settle every review save, not just the latest

Per-call mutate() callbacks fire only for an observer's latest call, so
with overlapping saves the in-flight count could stay above zero and the
review would ignore every later server value, including a Restore.

Saves now settle through useEntryPatch's hook-level onSettled (fires for
every mutation), in FIFO order (the entry-patch scope is serial). The
draft counts pending saves, remembers the latest incoming value, skips our
own optimistic/rollback echoes, and applies an outside change once the
saves settle.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

apps/web/src/components/DetailBoxes.tsx

@@ -6,15 +6,15 @@ import { orderEngagements, setCustomValue } from "../edits.ts";
 import {
   actorConnectionId,
   describeRating,
-  draftFailedSave,
   draftIncoming,
-  draftSaved,
+  draftSettled,
   draftStartSave,
   engagementNouns,
   formatDateRange,
   formatPartialDate,
   formatRating,
   friendlyError,
+  newDraft,
   ordinal,
   parsePartialDate,
   rollupLabel,
@@ -498,35 +498,29 @@ export function ChildrenBox({ entry, type }: { entry: EntryView; type: string })
 // ---------------------------------------------------------------------------------------------
 
 export function ReviewBox({ entry }: { entry: EntryView }) {
-  const patch = useEntryPatch(entry.id);
   const [saved, setSaved] = useState(false);
   // Local text plus the server value it last agreed with (see `Draft` in format.ts). A Restore shows
   // up here unless the user has typed since; a failed save keeps what they typed.
-  const [draft, setDraft] = useState<Draft>(() => ({ text: entry.review ?? "", synced: entry.review ?? "", saving: false }));
-  const inFlight = useRef(0);
+  const [draft, setDraft] = useState<Draft>(() => newDraft(entry.review ?? ""));
+  // Saves in flight, oldest first. The entry-patch scope runs them serially, so they settle in order.
+  const saves = useRef<{ was: string; stored: string }[]>([]);
+  const patch = useEntryPatch(entry.id, {
+    onSettled: (_p, error) => {
+      const save = saves.current.shift();
+      if (!save) return;
+      setDraft((d) => draftSettled(d, save, error === null));
+      setSaved(error === null && saves.current.length === 0);
+    },
+  });
   useEffect(() => setDraft((d) => draftIncoming(d, entry.review ?? "")), [entry.review]);
   const save = () => {
     const next = draft.text.trim() === "" ? null : draft.text;
     const stored = next ?? "";
     if (stored === draft.synced && stored === (entry.review ?? "")) return;
-    const was = draft.synced;
-    inFlight.current += 1;
-    setDraft((d) => ({ ...draftStartSave(d), synced: stored }));
-    const settle = (d: Draft) => ({ ...d, saving: inFlight.current > 0 });
-    patch.mutate(
-      { review: next },
-      {
-        onSuccess: () => {
-          inFlight.current -= 1;
-          setDraft((d) => settle(draftSaved(d, stored)));
-          setSaved(true);
-        },
-        onError: () => {
-          inFlight.current -= 1;
-          setDraft((d) => settle(draftFailedSave(d, was)));
-        },
-      },
-    );
+    const s = { was: draft.synced, stored };
+    saves.current.push(s);
+    setDraft((d) => draftStartSave(d, s));
+    patch.mutate({ review: next });
   };
   return (
     <Box

apps/web/src/format.ts

@@ -270,34 +270,45 @@ export function friendlyError(err: unknown, opts: { ratingScale?: RatingScale }
 
 /**
  * A text field edited locally and saved on blur. `synced` is the server value the text last agreed
- * with; text that differs from it is unsaved and is never replaced. While a save is in flight,
- * incoming values (the optimistic update, then a rollback on failure) are ignored, so the outcome
- * does not depend on whether the rollback or the error callback lands first.
+ * with; text that differs from it is unsaved and is never replaced. `pending` counts saves in flight
+ * (one entry's saves run serially, so they settle in order). While any is pending, incoming server
+ * values are only remembered in `incoming`. Values that are our own saves' echoes (optimistic updates
+ * show a save's `stored`, rollbacks show its `was`) are listed in `echoes` and ignored; any other
+ * value (an outside change, a Restore) is applied once the saves settle. So the outcome does not
+ * depend on the order in which rollbacks, refetches and settle callbacks land.
  */
 export interface Draft {
   text: string;
   synced: string;
-  saving: boolean;
+  pending: number;
+  incoming: string;
+  echoes: string[];
 }
 
-/** A server value arrived (refetch, Restore, rollback). Untouched text follows it; edited text is kept. */
+export function newDraft(value: string): Draft {
+  return { text: value, synced: value, pending: 0, incoming: value, echoes: [] };
+}
+
+/** A server value arrived. Untouched text follows it; edited text is kept. Deferred while saving. */
 export function draftIncoming(d: Draft, incoming: string): Draft {
-  if (d.saving || incoming === d.synced) return d;
-  const was = d.synced;
-  return { text: d.text === was ? incoming : d.text, synced: incoming, saving: false };
+  if (d.pending > 0 || incoming === d.synced) return { ...d, incoming };
+  return { ...d, text: d.text === d.synced ? incoming : d.text, synced: incoming, incoming };
 }
 
-/** Saving the current text: it becomes the synced value unless the save fails. */
-export function draftStartSave(d: Draft): Draft {
-  return { text: d.text, synced: d.text, saving: true };
+/** A save of `stored` started (`was` is the synced value before it): it counts as synced unless it fails. */
+export function draftStartSave(d: Draft, save: { was: string; stored: string }): Draft {
+  return { ...d, synced: save.stored, pending: d.pending + 1, echoes: [...d.echoes, save.was, save.stored] };
 }
 
-/** The save succeeded with this stored value. */
-export function draftSaved(d: Draft, stored: string): Draft {
-  return { ...d, synced: stored, saving: false };
-}
-
-/** The save failed: go back to the value before it, so the local text still counts as unsaved. */
-export function draftFailedSave(d: Draft, was: string): Draft {
-  return { ...d, synced: was, saving: false };
+/**
+ * The oldest pending save settled. Success: `stored` is on the server. Failure: back to `was`, so the
+ * local text still counts as unsaved. When the last pending save settles, a remembered incoming value
+ * that is not one of our own echoes is applied.
+ */
+export function draftSettled(d: Draft, save: { was: string; stored: string }, ok: boolean): Draft {
+  const pending = Math.max(0, d.pending - 1);
+  const next: Draft = { ...d, synced: ok ? save.stored : save.was, pending };
+  if (pending > 0) return next;
+  const settled: Draft = { ...next, echoes: [] };
+  return d.echoes.includes(d.incoming) ? settled : draftIncoming(settled, d.incoming);
 }

apps/web/src/hooks.ts

@@ -1,5 +1,5 @@
 import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query";
-import { useEffect, useState } from "react";
+import { useEffect, useRef, useState } from "react";
 import { api, type ChangeEvent, type EntryPatch, type EntryView } from "./api.ts";
 import { applyEntryPatch } from "./edits.ts";
 
@@ -37,9 +37,15 @@ export function useDebounced<T>(value: T, ms = 220): T {
  * error) and PATCHes for one entry run one at a time (mutation scope), in the order they were made.
  * Use `edit(cur => patch)` when the next value depends on the current one (tags, custom fields): it
  * builds from the latest cache, so quick successive edits compose.
+ *
+ * `onSettled` fires once for EVERY mutation of this hook (it is a hook-level option). Per-call
+ * `mutate(patch, { onSuccess })` callbacks only fire for the observer's latest call, so never use
+ * them for bookkeeping.
  */
-export function useEntryPatch(id: string) {
+export function useEntryPatch(id: string, opts: { onSettled?: (patch: EntryPatch, error: Error | null) => void } = {}) {
   const qc = useQueryClient();
+  const onSettledRef = useRef(opts.onSettled);
+  onSettledRef.current = opts.onSettled;
   const key = ["entry", id];
   const mutationKey = ["entry-patch", id];
   const m = useMutation({
@@ -60,9 +66,10 @@ export function useEntryPatch(id: string) {
       // Only the last queued edit writes the server's answer, so earlier answers don't flash old values.
       if (qc.isMutating({ mutationKey }) <= 1) qc.setQueryData(key, entry);
     },
-    onSettled: () => {
+    onSettled: (_entry, error, patch) => {
       void qc.invalidateQueries({ queryKey: ["entries"] });
       void qc.invalidateQueries({ queryKey: ["history", id] });
+      onSettledRef.current?.(patch, error);
     },
   });
   const edit = (build: (cur: EntryView) => EntryPatch | null) => {

apps/web/test/format.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, test } from "bun:test";
-import { draftFailedSave, draftIncoming, draftStartSave, formatDateRange, formatPartialDate, formatRating, friendlyError, parsePartialDate, rollupLabel, shortDate } from "../src/format.ts";
+import { draftIncoming, draftSettled, draftStartSave, newDraft, type Draft, formatDateRange, formatPartialDate, formatRating, friendlyError, parsePartialDate, rollupLabel, shortDate } from "../src/format.ts";
 
 describe("formatRating", () => {
   test("5 stars with halves", () => {
@@ -98,29 +98,51 @@ test("shortDate", () => {
 });
 
 describe("review draft sync", () => {
-  const typed = { text: "Second thoughts.", synced: "First thoughts.", saving: false };
+  const draft = (text: string, synced: string): Draft => ({ ...newDraft(synced), text });
+  const pick = (d: Draft) => [d.text, d.synced, d.pending];
 
-  test("a rollback after a failed save keeps the local text (rollback lands first)", () => {
-    let d = draftStartSave(typed); // optimistic cache now says "Second thoughts."
-    d = draftIncoming(d, "Second thoughts.");
-    const was = typed.synced;
-    d = draftIncoming(d, "First thoughts."); // onError rolled the cache back
-    d = draftFailedSave(d, was);
-    expect(d.text).toBe("Second thoughts.");
-    expect(d.synced).toBe("First thoughts."); // so the text still counts as unsaved
-    expect(d.saving).toBe(false);
+  test("a rollback after a failed save keeps the local text", () => {
+    const save = { was: "First thoughts.", stored: "Second thoughts." };
+    let d = draftStartSave(draft("Second thoughts.", save.was), save);
+    d = draftIncoming(d, save.stored); // optimistic update
+    d = draftIncoming(d, save.was); // rollback
+    d = draftSettled(d, save, false);
+    expect(pick(d)).toEqual(["Second thoughts.", "First thoughts.", 0]);
   });
 
-  test("a rollback after a failed save keeps the local text (error callback first)", () => {
-    let d = draftStartSave(typed);
-    d = draftFailedSave(d, typed.synced);
-    d = draftIncoming(d, "First thoughts.");
-    expect(d.text).toBe("Second thoughts.");
+  // Two overlapping saves: "a" (was "old"), then "b" (was "a"). Incoming values mirror what the cache does.
+  const A = { was: "old", stored: "a" };
+  const B = { was: "a", stored: "b" };
+  const twoSaves = () => draftIncoming(draftIncoming(draftStartSave({ ...draftStartSave(draft("a", "old"), A), text: "b" }, B), "a"), "b");
+
+  test("two overlapping saves both settle and the flag clears (success, success)", () => {
+    let d = twoSaves();
+    expect(d.pending).toBe(2);
+    d = draftSettled(draftSettled(d, A, true), B, true);
+    expect(pick(d)).toEqual(["b", "b", 0]);
+    expect(d.echoes).toEqual([]);
   });
 
-  test("a change from elsewhere (Restore) replaces untouched text but not edited text", () => {
-    const clean = { text: "Second thoughts.", synced: "Second thoughts.", saving: false };
-    expect(draftIncoming(clean, "First thoughts.").text).toBe("First thoughts.");
-    expect(draftIncoming(typed, "Something else").text).toBe("Second thoughts.");
+  test("two overlapping saves: fail then success ends synced; success then fail keeps text unsaved", () => {
+    let d = draftIncoming(twoSaves(), "old"); // A's rollback
+    d = draftSettled(draftSettled(d, A, false), B, true);
+    expect(pick(d)).toEqual(["b", "b", 0]);
+    let e = draftIncoming(twoSaves(), "a"); // B's rollback
+    e = draftSettled(draftSettled(e, A, true), B, false);
+    expect(pick(e)).toEqual(["b", "a", 0]);
+  });
+
+  test("an incoming value that arrives while saving is applied once saving ends", () => {
+    const save = { was: "x", stored: "y" };
+    let d = draftStartSave(draft("y", "x"), save);
+    d = draftIncoming(d, "Restored.");
+    expect(d.text).toBe("y");
+    d = draftSettled(d, save, true);
+    expect(pick(d)).toEqual(["Restored.", "Restored.", 0]);
+  });
+
+  test("a change from elsewhere replaces untouched text but not edited text", () => {
+    expect(draftIncoming(draft("Second.", "Second."), "First.").text).toBe("First.");
+    expect(draftIncoming(draft("Typed.", "Second."), "Other.").text).toBe("Typed.");
   });
 });