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((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((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(projectId: string, run: () => Promise): Promise(); + +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(projectId: string, run: () => Promise): Promise { + 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;