Fix critical data corruption and stale-data bugs in useNotes hook
- 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.
This commit is contained in:
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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;
|
||||
}, []);
|
||||
|
||||
Reference in New Issue
Block a user