From 9546fa13f233144b78f573827d6d2bec250270a4 Mon Sep 17 00:00:00 2001 From: Lucas Winther Date: Fri, 30 Oct 2026 22:19:15 +0100 Subject: [PATCH] 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 --- apps/web/src/components/DetailBoxes.tsx | 40 +++++++--------- apps/web/src/format.ts | 49 +++++++++++-------- apps/web/src/hooks.ts | 13 ++++-- apps/web/test/format.test.ts | 62 +++++++++++++++++-------- 4 files changed, 99 insertions(+), 65 deletions(-) diff --git a/apps/web/src/components/DetailBoxes.tsx b/apps/web/src/components/DetailBoxes.tsx index af46f7e..2f70d34 100644 --- a/apps/web/src/components/DetailBoxes.tsx +++ b/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(() => ({ text: entry.review ?? "", synced: entry.review ?? "", saving: false })); - const inFlight = useRef(0); + const [draft, setDraft] = useState(() => 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 ( 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); } diff --git a/apps/web/src/hooks.ts b/apps/web/src/hooks.ts index 7868ba4..ba05c4e 100644 --- a/apps/web/src/hooks.ts +++ b/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(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) => { diff --git a/apps/web/test/format.test.ts b/apps/web/test/format.test.ts index d7de425..1b356bb 100644 --- a/apps/web/test/format.test.ts +++ b/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."); }); });