Fix critical cross-project data corruption bug in useNotes hook

When a save is in flight for project A and the user switches to project B before it resolves, the stale closure still has projectId=A. When A's save resolves, the post-save re-read of listNotes(projectId) runs with the stale closed-over projectId, and setNotes(reloaded) overwrites B's displayed notes with A's list—the same cross-project contamination class as Finding 2 but reintroduced through the fix itself.

Fix: Add a currentProjectId ref updated on every render, and guard both saveNote and deleteNote callbacks with a check before replacing/filtering the whole list. If the project changed while the async operation was in flight, bail out of the state update but still report success (the operation itself succeeded on the backend; only the stale list update is skipped).

Added test: a save in flight for one project, a switch to another, then the first save resolving—asserts the second project's notes are still displayed.
This commit is contained in:
2026-09-01 12:50:23 -07:00
parent b6ba6deb09
commit a1f4eee9a3
2 changed files with 57 additions and 2 deletions
+43
View File
@@ -163,4 +163,47 @@ describe("useNotes", () => {
// listNotes should be called again after the save
expect(listNotes).toHaveBeenCalledTimes(callCountBefore + 1);
});
it("does not overwrite the new project's notes when a stale save resolves", 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[0].id).toBe("n1");
// Start a save for p1 that hangs
let resolveSave: ((note: Note) => void) | undefined;
saveNote.mockImplementationOnce(
() =>
new Promise((resolve) => {
resolveSave = resolve;
}),
);
let savePromise: Promise<boolean> | undefined;
await act(async () => {
savePromise = result.current.saveNote(note({ id: "n1" }));
});
// Switch to p2 while the save is in flight
listNotes.mockResolvedValueOnce([note({ id: "n2", title: "Project 2 Note" })]);
rerender({ projectId: "p2" });
await waitFor(() => expect(result.current.loading).toBe(false));
// Now p2's note should be displayed
expect(result.current.notes).toHaveLength(1);
expect(result.current.notes[0].id).toBe("n2");
// Resolve the stale p1 save
listNotes.mockResolvedValueOnce([note({ id: "n1", body: "edited" })]);
await act(async () => {
resolveSave?.(note({ id: "n1", body: "edited" }));
await savePromise;
});
// p2's note should still be displayed, not p1's
expect(result.current.notes).toHaveLength(1);
expect(result.current.notes[0].id).toBe("n2");
});
});
+14 -2
View File
@@ -34,6 +34,8 @@ export function useNotes(projectId: string) {
const [saveState, setSaveState] = useState<SaveState>({ status: "idle", error: null });
const pushToast = useAppState((s) => s.pushToast);
const resetTimer = useRef<ReturnType<typeof setTimeout> | null>(null);
const currentProjectId = useRef(projectId);
currentProjectId.current = projectId;
useEffect(() => {
if (!projectId) {
@@ -94,7 +96,12 @@ export function useNotes(projectId: string) {
// what a reload would show.
try {
const reloaded = await commands.listNotes(projectId);
setNotes(reloaded);
// Guard against a stale callback: if the project changed while the save was in
// flight, don't overwrite the new project's list with the old one. The save itself
// still succeeded, so succeeded() still fires; only the list replacement is skipped.
if (currentProjectId.current === 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.
@@ -126,7 +133,12 @@ export function useNotes(projectId: string) {
async (noteId: string) => {
try {
await commands.deleteNote(projectId, noteId);
setNotes((current) => current.filter((n) => n.id !== noteId));
// Guard against a stale callback: if the project changed while the delete was in
// flight, don't modify the list. Note ids are UUIDs so a stale filter cannot match
// another project's note, but applying the same guard for consistency.
if (currentProjectId.current === projectId) {
setNotes((current) => current.filter((n) => n.id !== noteId));
}
return true;
} catch (e) {
pushToast({ kind: "error", message: "Could not delete note", detail: String(e) });