From 2708772bf9ab57212af835a71d1b9185a7c2bd95 Mon Sep 17 00:00:00 2001
From: Josh Knapp <jknapp85@gmail.com>
Date: Tue, 1 Sep 2026 14:23:55 -0700
Subject: [PATCH] Order the notes cache by sequence, not by who resolves last

One gesture puts two requests in flight. With the tab already loaded, clicking
the dock toggle while the textarea has focus fires `blur` -> `saveNote` and the
dock's mount -> `list_notes` in the same tick. The save finishes and its re-read
writes the post-save list; the mount's read -- issued earlier, still out -- then
lands its pre-save snapshot on top, and both panels show stale text until
something else refreshes.

`mutationChains` could not have caught this: it orders a project's writes
against each other and the mount load is a read outside it. Putting the read on
the chain would work, but it buys correctness with latency the user feels -- a
panel mount waiting behind `save_note`'s double-fsync write -- and leaves a
"mutation chain" holding reads.

The two requests are not competing for a resource; the loser's result is simply
older. So every write into `notesByProject[p]` now claims a per-project sequence
when the request behind it is issued, and `commitNotes` drops one whose sequence
predates what is already cached. Reads take their sequence at issue time, since
being ordered by resolution is the bug. Local patches -- the filter behind a
confirmed delete, the prepend behind a failed re-read -- take a fresh one at
commit time, because they are authoritative then rather than derived from an
earlier read, and anything still in flight behind them is genuinely stale. A
failed read commits under its *own* sequence, not a fresh one, so its empty list
cannot beat a later read that has the real answer.

`isCurrent()` stays, and is not folded in. It guards `setSaveState`, not the
cache: it asks whether this *panel* is still showing the project a save was made
for, which a per-project counter cannot answer -- two panels on one project share
every sequence value. Ordering and panel identity are two questions.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HjL1E2JFNctUqCYotUwqqb
---
 app/src/hooks/useNotes.test.ts | 74 ++++++++++++++++++++++++++
 app/src/hooks/useNotes.ts      | 96 +++++++++++++++++++++++++++++++---
 2 files changed, 163 insertions(+), 7 deletions(-)

diff --git a/app/src/hooks/useNotes.test.ts b/app/src/hooks/useNotes.test.ts
index a7a7182..208a354 100644
--- a/app/src/hooks/useNotes.test.ts
+++ b/app/src/hooks/useNotes.test.ts
@@ -369,4 +369,78 @@ describe("useNotes", () => {
     expect(dock.result.current.loading).toBe(false);
     expect(dock.result.current.notes).toHaveLength(1);
   });
+
+  it("does not let a slow mount read overwrite a fresher post-save refresh", async () => {
+    // One gesture, two requests. The tab is already loaded; the user clicks the
+    // dock toggle with the textarea focused, so `blur` → `saveNote` and the
+    // dock's mount → `list_notes` are issued in the same tick. The save's
+    // re-read writes the post-save list; the mount's read — issued earlier,
+    // still in flight — must not then land its pre-save snapshot on top of it.
+    const files = fakeBackend({ p1: [note({ body: "before" })] });
+    const tab = renderHook(() => useNotes("p1"));
+    await waitFor(() => expect(tab.result.current.loading).toBe(false));
+    expect(tab.result.current.notes[0].body).toBe("before");
+
+    // The dock's mount read: it snapshots the list as it is *now* (pre-save)
+    // and hangs, standing in for a plain read that is slower than
+    // `save_note`'s double-fsync write plus the re-read behind it.
+    let releaseMountRead: (() => void) | undefined;
+    listNotes.mockImplementationOnce(async (p: string) => {
+      const preSave = [...(files[p] ?? [])];
+      await new Promise<void>((resolve) => {
+        releaseMountRead = resolve;
+      });
+      return preSave;
+    });
+    const dock = renderHook(() => useNotes("p1"));
+    expect(releaseMountRead).toBeDefined();
+
+    // The save and its re-read complete while that read is still out.
+    await act(async () => {
+      await tab.result.current.saveNote(note({ body: "after" }));
+    });
+    expect(tab.result.current.notes[0].body).toBe("after");
+
+    // Now the stale read lands.
+    await act(async () => {
+      releaseMountRead!();
+      await Promise.resolve();
+    });
+
+    expect(tab.result.current.notes[0].body).toBe("after");
+    expect(dock.result.current.notes[0].body).toBe("after");
+  });
+
+  it("does not let a slow mount read resurrect a note deleted while it was in flight", async () => {
+    // The other half of the same ordering rule: a confirmed delete is newer
+    // than any read issued before it finished, so the read's pre-delete list
+    // must not be written back over the shortened one.
+    const files = fakeBackend({ p1: [note()] });
+    const tab = renderHook(() => useNotes("p1"));
+    await waitFor(() => expect(tab.result.current.loading).toBe(false));
+
+    let releaseMountRead: (() => void) | undefined;
+    listNotes.mockImplementationOnce(async (p: string) => {
+      const preDelete = [...(files[p] ?? [])];
+      await new Promise<void>((resolve) => {
+        releaseMountRead = resolve;
+      });
+      return preDelete;
+    });
+    const dock = renderHook(() => useNotes("p1"));
+    expect(releaseMountRead).toBeDefined();
+
+    await act(async () => {
+      await tab.result.current.deleteNote("n1");
+    });
+    expect(tab.result.current.notes).toHaveLength(0);
+
+    await act(async () => {
+      releaseMountRead!();
+      await Promise.resolve();
+    });
+
+    expect(tab.result.current.notes).toHaveLength(0);
+    expect(dock.result.current.notes).toHaveLength(0);
+  });
 });
diff --git a/app/src/hooks/useNotes.ts b/app/src/hooks/useNotes.ts
index 07cac3c..dcb3876 100644
--- a/app/src/hooks/useNotes.ts
+++ b/app/src/hooks/useNotes.ts
@@ -56,6 +56,78 @@ function enqueueMutation<T>(projectId: string, run: () => Promise<T>): Promise<T
   return result;
 }
 
+/**
+ * Per-project write ordering for the shared notes cache.
+ *
+ * `mutationChains` orders a project's *writes* against each other. It says
+ * nothing about reads, and the mount load is a read that runs outside it — so
+ * one gesture can put two requests in flight at once and let the slower one
+ * win. Clicking the dock toggle with the textarea focused fires `blur` →
+ * `saveNote` and the dock's mount → `list_notes` in the same tick: the save
+ * finishes, its re-read writes the post-save list, and then the mount's read —
+ * issued earlier, still out — lands its pre-save snapshot on top. Both panels
+ * show stale text until something else refreshes. It needs the plain read to
+ * be slower than `save_note`'s double-fsync write plus a second read, so it is
+ * narrow, but it was reproduced.
+ *
+ * The fix is a sequence number rather than a chain, because the two requests
+ * are not competing for a resource — the loser's result is simply *older*, and
+ * the cheapest correct thing to do with it is throw it away. Every write
+ * claims a sequence when the request behind it is issued, and `commitNotes`
+ * drops one whose sequence predates what is already cached. That also closes a
+ * hole identity comparison cannot: on a `p1 → p2 → p1` switch a read from the
+ * *first* p1 era is indistinguishable from a current one by project id, and
+ * would land its stale list on the second era's.
+ *
+ * Note what this deliberately does **not** replace. `isCurrent()` asks whether
+ * this *panel* is still showing the project a save was made for, which governs
+ * a per-panel `SaveIndicator` and not the shared cache at all; a per-project
+ * counter cannot answer it. Ordering and panel identity are two questions, and
+ * they keep two guards.
+ *
+ * Entries are two integers per project and are never pruned: they must outlive
+ * every request that could still land, and the map is monotone, so a stale
+ * sequence can never be reissued.
+ */
+const notesSequences = new Map<string, { issued: number; committed: number }>();
+
+function sequenceFor(projectId: string): { issued: number; committed: number } {
+  let seq = notesSequences.get(projectId);
+  if (!seq) notesSequences.set(projectId, (seq = { issued: 0, committed: 0 }));
+  return seq;
+}
+
+/**
+ * Claim the sequence for a write about to be issued.
+ *
+ * Called immediately before the request whose result it will commit, so that
+ * ordering is by *issue* time. Resolution order is exactly what cannot be
+ * trusted here.
+ */
+function issueNotesWrite(projectId: string): number {
+  const seq = sequenceFor(projectId);
+  seq.issued += 1;
+  return seq.issued;
+}
+
+/**
+ * Write a list into the cache under the sequence it was issued at, unless
+ * something newer has already been committed.
+ *
+ * A local patch — the filter behind a confirmed delete, say — is authoritative
+ * at the moment it applies rather than derived from an earlier read, so it
+ * claims its sequence here: `issued` is never below `committed`, so a freshly
+ * claimed one always wins, and anything still in flight behind it is correctly
+ * treated as stale.
+ */
+function commitNotes(projectId: string, seq: number, notes: Note[]): boolean {
+  const sequence = sequenceFor(projectId);
+  if (seq <= sequence.committed) return false;
+  sequence.committed = seq;
+  useAppState.getState().setProjectNotes(projectId, notes);
+  return true;
+}
+
 /**
  * Re-read the canonical list into the shared cache.
  *
@@ -63,11 +135,16 @@ function enqueueMutation<T>(projectId: string, run: () => Promise<T>): Promise<T
  * the record's position has changed and positional patching would disagree
  * with what a reload would show. The backend owns the order; the webview never
  * sorts. A failed re-read leaves the cache alone rather than clearing it.
+ *
+ * `true` means "the cache is current", which is why a superseded commit still
+ * returns it: whatever beat this read was issued later and therefore read the
+ * same write or a later one.
  */
 async function refresh(projectId: string): Promise<boolean> {
+  const seq = issueNotesWrite(projectId);
   try {
     const reloaded = await commands.listNotes(projectId);
-    useAppState.getState().setProjectNotes(projectId, reloaded);
+    commitNotes(projectId, seq, reloaded);
     return true;
   } catch {
     return false;
@@ -104,10 +181,11 @@ export function useNotes(projectId: string) {
     const store = useAppState.getState();
     if (store.notesLoading[projectId]) return;
     store.setNotesLoading(projectId, true);
+    const seq = issueNotesWrite(projectId);
     commands
       .listNotes(projectId)
       .then((loaded) => {
-        useAppState.getState().setProjectNotes(projectId, loaded);
+        commitNotes(projectId, seq, loaded);
       })
       .catch((e) => {
         // A project that has never been read caches the empty list, so a panel
@@ -117,9 +195,12 @@ export function useNotes(projectId: string) {
         // must not blank both of them. Same rule as `refresh()`. Neither
         // branch has a stale-project hazard, because the write is keyed by the
         // project it belongs to.
-        const store = useAppState.getState();
-        if (store.notesByProject[projectId] === undefined) {
-          store.setProjectNotes(projectId, []);
+        //
+        // The commit goes under this read's own sequence, not a fresh one: a
+        // *later* read still in flight has the newer answer and must not be
+        // dropped in favour of this failure's empty list.
+        if (useAppState.getState().notesByProject[projectId] === undefined) {
+          commitNotes(projectId, seq, []);
         }
         pushToast({
           kind: "error",
@@ -216,7 +297,7 @@ export function useNotes(projectId: string) {
             // The note exists; only the re-read failed. Show it rather than
             // leaving the user with a button that did nothing visible.
             const store = useAppState.getState();
-            store.setProjectNotes(projectId, [
+            commitNotes(projectId, issueNotesWrite(projectId), [
               saved,
               ...(store.notesByProject[projectId] ?? []),
             ]);
@@ -240,8 +321,9 @@ export function useNotes(projectId: string) {
         try {
           await commands.deleteNote(projectId, noteId);
           const store = useAppState.getState();
-          store.setProjectNotes(
+          commitNotes(
             projectId,
+            issueNotesWrite(projectId),
             (store.notesByProject[projectId] ?? []).filter((n) => n.id !== noteId),
           );
           return true;