From 4608c977b749b325c24f703a3a86543ec3f3da0a Mon Sep 17 00:00:00 2001
From: Lucas Winther <lucwin@ece.au.dk>
Date: Fri, 30 Oct 2026 22:14:02 +0100
Subject: [PATCH] fix(web): keep review text on a failed save; review round 2

- Review draft sync is a pure helper (format.ts): incoming values are
  ignored while a save is in flight, and a failed save restores the
  previous synced value, so the typed text is kept whichever of the
  rollback and the error callback lands first. Tests cover both orders.
- A failed background refetch keeps showing the cached entry instead of
  the error screen (it unmounted the review box mid-outage).
- History restore joins the entry-patch mutation scope.
- updatePreferences: read-modify-write in a transaction, row locked on
  Postgres.
- Invalidate auth-status after register; out-of-range rating errors speak
  in the user's scale.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 apps/web/src/components/DetailBoxes.tsx   | 56 ++++++++++++++++-------
 apps/web/src/components/EntryDetail.tsx   |  6 ++-
 apps/web/src/components/HistoryDrawer.tsx |  2 +
 apps/web/src/format.ts                    | 47 ++++++++++++++++++-
 apps/web/src/routes/register.tsx          |  1 +
 apps/web/test/format.test.ts              | 34 +++++++++++++-
 packages/core/src/preferences.ts          | 24 +++++++---
 7 files changed, 140 insertions(+), 30 deletions(-)

diff --git a/apps/web/src/components/DetailBoxes.tsx b/apps/web/src/components/DetailBoxes.tsx
index 304c111..af46f7e 100644
--- a/apps/web/src/components/DetailBoxes.tsx
+++ b/apps/web/src/components/DetailBoxes.tsx
@@ -6,6 +6,10 @@ import { orderEngagements, setCustomValue } from "../edits.ts";
 import {
   actorConnectionId,
   describeRating,
+  draftFailedSave,
+  draftIncoming,
+  draftSaved,
+  draftStartSave,
   engagementNouns,
   formatDateRange,
   formatPartialDate,
@@ -16,6 +20,7 @@ import {
   rollupLabel,
   statusLabel,
   timeAgo,
+  type Draft,
   type PartialDate,
 } from "../format.ts";
 import { useEntryPatch, useSources } from "../hooks.ts";
@@ -357,6 +362,7 @@ export function SourcesBox({ entry, engagements }: { entry: EntryView; engagemen
 export function CustomFieldsBox({ entry, type }: { entry: EntryView; type: string }) {
   const defs = useQuery({ queryKey: ["custom-fields", type], queryFn: () => api.customFields(type), staleTime: 60_000 });
   const patch = useEntryPatch(entry.id);
+  const scale = useRatingScale();
   if (!defs.data?.length) return null;
   const save = (def: CustomFieldDef, value: unknown) =>
     patch.edit((cur) => {
@@ -370,7 +376,7 @@ export function CustomFieldsBox({ entry, type }: { entry: EntryView; type: strin
           <CustomField key={d.id} def={d} value={entry.customFields[d.id]} onSave={(v) => save(d, v)} />
         ))}
       </div>
-      {patch.error && <p className="error-text" role="alert">{friendlyError(patch.error)}</p>}
+      {patch.error && <p className="error-text" role="alert">{friendlyError(patch.error, { ratingScale: scale })}</p>}
     </Box>
   );
 }
@@ -494,21 +500,33 @@ 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);
-  const [text, setText] = useState(entry.review ?? "");
-  // The server value this text was last in sync with. When the review changes elsewhere (Restore,
-  // another tab) and the user has not typed since, show the new value, so a later blur can't undo it.
-  const synced = useRef(entry.review ?? "");
-  useEffect(() => {
-    const incoming = entry.review ?? "";
-    if (incoming === synced.current) return;
-    setText((cur) => (cur === synced.current ? incoming : cur));
-    synced.current = incoming;
-  }, [entry.review]);
+  // 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);
+  useEffect(() => setDraft((d) => draftIncoming(d, entry.review ?? "")), [entry.review]);
   const save = () => {
-    const next = text.trim() === "" ? null : text;
-    if ((next ?? "") === (entry.review ?? "")) return;
-    synced.current = next ?? "";
-    patch.mutate({ review: next }, { onSuccess: () => setSaved(true) });
+    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)));
+        },
+      },
+    );
   };
   return (
     <Box
@@ -521,9 +539,13 @@ export function ReviewBox({ entry }: { entry: EntryView }) {
       <label htmlFor="review-text" className="sr-only">Your review (Markdown)</label>
       <textarea
         id="review-text"
-        value={text}
+        value={draft.text}
         placeholder="What stayed with you? Markdown works here."
-        onChange={(e) => (setText(e.target.value), setSaved(false))}
+        onChange={(e) => {
+          const text = e.target.value;
+          setDraft((d) => ({ ...d, text }));
+          setSaved(false);
+        }}
         onBlur={save}
         onKeyDown={(e) => {
           if (e.key === "Enter" && (e.metaKey || e.ctrlKey)) save();
diff --git a/apps/web/src/components/EntryDetail.tsx b/apps/web/src/components/EntryDetail.tsx
index 673d46a..082696e 100644
--- a/apps/web/src/components/EntryDetail.tsx
+++ b/apps/web/src/components/EntryDetail.tsx
@@ -18,7 +18,9 @@ const TIMES = ["", "once", "twice", "three times"];
 export function EntryDetail({ id }: { id: string }) {
   const entry = useQuery({ queryKey: ["entry", id], queryFn: () => api.entry(id) });
   if (entry.isPending) return <div className="detail" aria-busy="true" />;
-  if (entry.error) {
+  // A failed background refetch (server briefly down) keeps showing the cached entry, so nothing the
+  // user is typing gets unmounted; only a first load that fails shows the error screen.
+  if (!entry.data) {
     const gone = entry.error instanceof ApiError && entry.error.status === 404;
     return (
       <div className="detail-empty" role="alert">
@@ -96,7 +98,7 @@ function Detail({ entry }: { entry: EntryView }) {
             onAdd={(t) => patch.edit((cur) => ({ tags: addTag(cur.tags, t) }))}
             onRemove={(t) => patch.edit((cur) => ({ tags: removeTag(cur.tags, t) }))}
           />
-          {patch.error && <p className="error-text" role="alert">{friendlyError(patch.error)}</p>}
+          {patch.error && <p className="error-text" role="alert">{friendlyError(patch.error, { ratingScale: scale })}</p>}
         </div>
       </header>
 
diff --git a/apps/web/src/components/HistoryDrawer.tsx b/apps/web/src/components/HistoryDrawer.tsx
index 3e48ff7..aca8b5f 100644
--- a/apps/web/src/components/HistoryDrawer.tsx
+++ b/apps/web/src/components/HistoryDrawer.tsx
@@ -30,6 +30,8 @@ export function HistoryDrawer({ entry, onClose }: { entry: EntryView; onClose: (
   const closeBtn = useRef<HTMLButtonElement>(null);
   const history = useQuery({ queryKey: ["history", entry.id], queryFn: () => api.history(entry.id) });
   const restore = useMutation({
+    // Same scope as the entry's PATCHes (useEntryPatch), so a restore never races a pending blur-save.
+    scope: { id: `entry-patch:${entry.id}` },
     mutationFn: api.restore,
     onSuccess: () => {
       for (const key of [["entry", entry.id], ["history", entry.id], ["engagements", entry.id], ["entries"]]) void qc.invalidateQueries({ queryKey: key });
diff --git a/apps/web/src/format.ts b/apps/web/src/format.ts
index 4fab9f3..d3db6c6 100644
--- a/apps/web/src/format.ts
+++ b/apps/web/src/format.ts
@@ -231,8 +231,9 @@ const TOO_LARGE = "That file is too large. The limit is 25 MB.";
 /**
  * An API error in the product's voice: what happened and how to fix it. Validation messages arrive as
  * `field: reason` (core) or `field: Zod message`, several joined by "; "; the first one is described.
+ * Pass the user's rating scale so out-of-range ratings are described in the scale they see.
  */
-export function friendlyError(err: unknown): string {
+export function friendlyError(err: unknown, opts: { ratingScale?: RatingScale } = {}): string {
   if (!err || typeof err !== "object" || !("code" in err) || !("message" in err)) return "Something went wrong. Try again.";
   const { status, code, message } = err as { status?: number; code: string; message: string };
   if (status === 413) return TOO_LARGE;
@@ -249,7 +250,11 @@ export function friendlyError(err: unknown): string {
   if (field === "currency" || reason === "not_cents") return "Price needs a number like 14 or 14.99, and a three-letter currency like GBP.";
   if (reason === "not_url") return `${label} needs a full web address, starting with https://.`;
   if (reason === "not_date" || reason === "bad_precision") return `${label} needs a date like 2025, 2025-03 or 2025-03-14.`;
-  if (reason === "out_of_range") return `${label} is out of range. Ratings go from 0 to 100.`;
+  if (reason === "out_of_range") {
+    const scale = opts.ratingScale ?? "5half";
+    const range = scale === "10" ? "0 and 10" : scale === "100" ? "0 and 100" : "0 and 5 stars";
+    return label === "Rating" ? `Pick a rating between ${range}.` : `Pick a rating for ${label} between ${range}.`;
+  }
   if (reason === "required" || /^Too small: .*>=1 (character|item)/.test(reason) || /received undefined$/.test(reason)) return `${label} can't be empty.`;
   if (reason === "too_long" || reason.startsWith("Too big")) return `${label} is too long. Shorten it and try again.`;
   if (reason.startsWith("Too small")) return `${label} is too short.`;
@@ -258,3 +263,41 @@ export function friendlyError(err: unknown): string {
   if (reason === "duplicate") return `${label} is already in use. Pick another.`;
   return `${label} wasn't accepted. Check it and try again.`;
 }
+
+// ---------------------------------------------------------------------------------------------
+// Review draft sync
+// ---------------------------------------------------------------------------------------------
+
+/**
+ * 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.
+ */
+export interface Draft {
+  text: string;
+  synced: string;
+  saving: boolean;
+}
+
+/** A server value arrived (refetch, Restore, rollback). Untouched text follows it; edited text is kept. */
+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 };
+}
+
+/** 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 };
+}
+
+/** 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 };
+}
diff --git a/apps/web/src/routes/register.tsx b/apps/web/src/routes/register.tsx
index 59e5a34..a11c238 100644
--- a/apps/web/src/routes/register.tsx
+++ b/apps/web/src/routes/register.tsx
@@ -29,6 +29,7 @@ export function Register() {
     setError(null);
     try {
       await api.register({ username, password, displayName: displayName || undefined, inviteCode: inviteCode || undefined });
+      void qc.invalidateQueries({ queryKey: ["auth-status"] });
       qc.setQueryData(["me"], await api.login(username, password));
       navigate("/", { replace: true });
     } catch (err) {
diff --git a/apps/web/test/format.test.ts b/apps/web/test/format.test.ts
index faf74f1..d7de425 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 { formatDateRange, formatPartialDate, formatRating, friendlyError, parsePartialDate, rollupLabel, shortDate } from "../src/format.ts";
+import { draftFailedSave, draftIncoming, draftStartSave, formatDateRange, formatPartialDate, formatRating, friendlyError, parsePartialDate, rollupLabel, shortDate } from "../src/format.ts";
 
 describe("formatRating", () => {
   test("5 stars with halves", () => {
@@ -72,7 +72,9 @@ describe("friendlyError", () => {
   test("maps validation reasons to plain copy", () => {
     expect(friendlyError(v("customFields.Website: not_url"))).toBe("Website needs a full web address, starting with https://.");
     expect(friendlyError(v("finishedAt: not_date"))).toBe("Finished needs a date like 2025, 2025-03 or 2025-03-14.");
-    expect(friendlyError(v("rating: out_of_range"))).toBe("Rating is out of range. Ratings go from 0 to 100.");
+    expect(friendlyError(v("rating: out_of_range"))).toBe("Pick a rating between 0 and 5 stars.");
+    expect(friendlyError(v("rating: out_of_range"), { ratingScale: "10" })).toBe("Pick a rating between 0 and 10.");
+    expect(friendlyError(v("customFields.Mood: out_of_range"), { ratingScale: "5" })).toBe("Pick a rating for Mood between 0 and 5 stars.");
     expect(friendlyError(v("title: required"))).toBe("Title can't be empty.");
     expect(friendlyError(v("review: too_long"))).toBe("Review is too long. Shorten it and try again.");
     expect(friendlyError(v("password: Too small: expected string to have >=8 characters"))).toBe("Passwords need at least 8 characters.");
@@ -94,3 +96,31 @@ describe("friendlyError", () => {
 test("shortDate", () => {
   expect(shortDate("2026-10-12T10:00:00")).toBe("12 Oct");
 });
+
+describe("review draft sync", () => {
+  const typed = { text: "Second thoughts.", synced: "First thoughts.", saving: false };
+
+  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 (error callback first)", () => {
+    let d = draftStartSave(typed);
+    d = draftFailedSave(d, typed.synced);
+    d = draftIncoming(d, "First thoughts.");
+    expect(d.text).toBe("Second thoughts.");
+  });
+
+  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.");
+  });
+});
diff --git a/packages/core/src/preferences.ts b/packages/core/src/preferences.ts
index 8a75767..adcfc01 100644
--- a/packages/core/src/preferences.ts
+++ b/packages/core/src/preferences.ts
@@ -1,4 +1,5 @@
 import type { Db } from "@sttracker/db/db";
+import type { Executor } from "./history.ts";
 import { fromJson, toJson } from "@sttracker/db/json";
 import { nowIso } from "@sttracker/db/ids";
 import type { Visibility } from "@sttracker/db/schema";
@@ -16,15 +17,20 @@ export interface Preferences {
 
 export const DEFAULT_PREFERENCES: Preferences = { ratingScale: "5half", defaultVisibility: "private" };
 
-async function readRaw(db: Db, userId: string): Promise<Record<string, unknown>> {
-  const u = await db.k.selectFrom("users").select("preferences").where("id", "=", userId).executeTakeFirst();
+async function readRaw(k: Executor, userId: string, lock = false): Promise<Record<string, unknown>> {
+  let q = k.selectFrom("users").select("preferences").where("id", "=", userId);
+  if (lock) q = q.forUpdate();
+  const u = await q.executeTakeFirst();
   if (!u) throw new NotFoundError("user");
   return fromJson<Record<string, unknown>>(u.preferences, {});
 }
 
 /** The user's preferences; missing or invalid stored values fall back to the defaults. */
 export async function getPreferences(db: Db, userId: string): Promise<Preferences> {
-  const raw = await readRaw(db, userId);
+  return toPreferences(await readRaw(db.k, userId));
+}
+
+function toPreferences(raw: Record<string, unknown>): Preferences {
   return {
     ratingScale: (RATING_SCALES as readonly unknown[]).includes(raw.ratingScale) ? (raw.ratingScale as RatingScale) : DEFAULT_PREFERENCES.ratingScale,
     defaultVisibility: (VISIBILITIES as readonly unknown[]).includes(raw.defaultVisibility)
@@ -37,8 +43,12 @@ export async function getPreferences(db: Db, userId: string): Promise<Preference
 export async function updatePreferences(db: Db, userId: string, patch: Partial<Preferences>): Promise<Preferences> {
   if (patch.ratingScale !== undefined && !RATING_SCALES.includes(patch.ratingScale)) throw new ValidationError("ratingScale", "invalid");
   if (patch.defaultVisibility !== undefined && !VISIBILITIES.includes(patch.defaultVisibility)) throw new ValidationError("defaultVisibility", "invalid");
-  const raw = await readRaw(db, userId);
-  const next = { ...raw, ...Object.fromEntries(Object.entries(patch).filter(([, v]) => v !== undefined)) };
-  await db.k.updateTable("users").set({ preferences: toJson(next), updated_at: nowIso() }).where("id", "=", userId).execute();
-  return getPreferences(db, userId);
+  // Read-modify-write in one transaction, with the user row locked on Postgres (SQLite runs one
+  // connection), so concurrent updates of different keys don't drop each other.
+  return db.k.transaction().execute(async (tx) => {
+    const raw = await readRaw(tx, userId, db.dialect === "postgres");
+    const next = { ...raw, ...Object.fromEntries(Object.entries(patch).filter(([, v]) => v !== undefined)) };
+    await tx.updateTable("users").set({ preferences: toJson(next), updated_at: nowIso() }).where("id", "=", userId).execute();
+    return toPreferences(next);
+  });
 }