From b6ba6deb0924bf8ac19d67c2d4945b8aa4e2cd0c Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Tue, 1 Sep 2026 12:45:35 -0700 Subject: [PATCH] Fix critical data corruption and stale-data bugs in useNotes hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Finding 1 (saveNote): After a successful save, re-read the canonical list from the backend instead of patching in place. A successful save stamps a new updated_at, and the backend sorts by updated_at descending, so the record's position has changed and positional patching would disagree with what a reload would show. If the re-read fails, keep the save reported as successful and leave the existing list alone. - Finding 2 (stale notes): Clear notes on projectId change (not only when empty) and on load failure. Previously, switching from project A to project B would leave A's notes on screen until B's fetch resolved, and if a user edited one, A's note would be written into B's notes file—cross-project data corruption. If a load fails, A's notes stay visible under B indefinitely. - Added four new tests covering these scenarios: projectId change clears old notes, failed load leaves no stale notes, saving a new note ends with the backend's list, and saves re-read the list rather than patching. --- app/src/hooks/useNotes.test.ts | 70 ++++++++++++++++++++++++++++++++++ app/src/hooks/useNotes.ts | 27 ++++++++----- 2 files changed, 88 insertions(+), 9 deletions(-) diff --git a/app/src/hooks/useNotes.test.ts b/app/src/hooks/useNotes.test.ts index 3a173af..7d58b75 100644 --- a/app/src/hooks/useNotes.test.ts +++ b/app/src/hooks/useNotes.test.ts @@ -67,6 +67,9 @@ describe("useNotes", () => { const { result } = renderHook(() => useNotes("p1")); await waitFor(() => expect(result.current.loading).toBe(false)); + // Mock the re-read to return the edited note + listNotes.mockResolvedValueOnce([note({ body: "edited" })]); + await act(async () => { await result.current.saveNote(note({ body: "edited" })); }); @@ -93,4 +96,71 @@ describe("useNotes", () => { renderHook(() => useNotes("")); await waitFor(() => expect(listNotes).not.toHaveBeenCalled()); }); + + it("clears the first project's notes when the projectId changes to another non-empty value", async () => { + const { result, rerender } = renderHook( + ({ projectId }: { projectId: string }) => useNotes(projectId), + { initialProps: { projectId: "p1" } }, + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(result.current.notes).toHaveLength(1); + + // Change to a different project before the new fetch resolves + listNotes.mockImplementationOnce(() => new Promise(() => {})); // never resolves + rerender({ projectId: "p2" }); + + // The old notes should be cleared immediately + expect(result.current.notes).toHaveLength(0); + }); + + it("leaves no stale notes on screen when a load fails", async () => { + listNotes.mockResolvedValueOnce([note()]); + const { result, rerender } = renderHook( + ({ projectId }: { projectId: string }) => useNotes(projectId), + { initialProps: { projectId: "p1" } }, + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(result.current.notes).toHaveLength(1); + + // Switch to a project whose load fails + listNotes.mockRejectedValueOnce(new Error("load failed")); + rerender({ projectId: "p2" }); + await waitFor(() => expect(result.current.loading).toBe(false)); + + expect(result.current.notes).toHaveLength(0); + expect(pushToast).toHaveBeenCalled(); + }); + + it("ends with the list the backend returned when saving a new note", async () => { + // Initially one note + const { result } = renderHook(() => useNotes("p1")); + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(result.current.notes).toHaveLength(1); + + // Saving a new note (not in the current list) re-reads and ends with the backend's list + const newNote = note({ id: "n2", title: "New" }); + listNotes.mockResolvedValueOnce([newNote, note()]); + + await act(async () => { + await result.current.saveNote(newNote); + }); + + expect(result.current.notes).toHaveLength(2); + expect(result.current.notes[0].id).toBe("n2"); + }); + + it("re-reads the list after a successful save rather than patching in place", async () => { + const { result } = renderHook(() => useNotes("p1")); + await waitFor(() => expect(result.current.loading).toBe(false)); + + const callCountBefore = listNotes.mock.calls.length; + listNotes.mockResolvedValueOnce([note({ body: "edited" })]); + + await act(async () => { + await result.current.saveNote(note({ body: "edited" })); + }); + + // listNotes should be called again after the save + expect(listNotes).toHaveBeenCalledTimes(callCountBefore + 1); + }); }); diff --git a/app/src/hooks/useNotes.ts b/app/src/hooks/useNotes.ts index 1d4f3fd..74320e7 100644 --- a/app/src/hooks/useNotes.ts +++ b/app/src/hooks/useNotes.ts @@ -43,6 +43,7 @@ export function useNotes(projectId: string) { } let cancelled = false; setLoading(true); + setNotes([]); commands .listNotes(projectId) .then((loaded) => { @@ -50,6 +51,7 @@ export function useNotes(projectId: string) { }) .catch((e) => { if (cancelled) return; + setNotes([]); pushToast({ kind: "error", message: "Could not load notes for this project", @@ -85,14 +87,18 @@ export function useNotes(projectId: string) { if (resetTimer.current) clearTimeout(resetTimer.current); setSaveState({ status: "saving", error: null }); try { - const saved = await commands.saveNote(projectId, note); - setNotes((current) => { - const index = current.findIndex((n) => n.id === saved.id); - if (index === -1) return [saved, ...current]; - const next = [...current]; - next[index] = saved; - return next; - }); + await commands.saveNote(projectId, note); + // Re-read the canonical list from the backend. A successful save stamps a new + // `updated_at`, and the backend sorts unpinned notes by `updated_at` descending, + // so the record's position has changed and positional patching would disagree with + // what a reload would show. + try { + const reloaded = await commands.listNotes(projectId); + setNotes(reloaded); + } catch { + // Keep the save reported as successful (it was) and leave the existing list alone + // rather than clearing it if the re-read fails. + } succeeded(); return true; } catch (e) { @@ -108,7 +114,10 @@ export function useNotes(projectId: string) { const createNote = useCallback(async () => { const note = draft(); // Held locally first so the editor can focus it immediately; the save - // happens on blur like every other edit. + // happens on blur like every other edit. The note does not exist backend-side, + // so no re-read can place it and no backend ordering applies to it yet. Prepending + // puts it at the top where the user can see it immediately, and on the first save + // its canonical position is established. setNotes((current) => [note, ...current]); return note; }, []);