fix(review): save draft notes exactly once under rapid Ctrl+S

.changeset/rapid-save-duplicate-note.md

@@ -0,0 +1,5 @@
+---
+"hunkdiff": patch
+---
+
+Rapidly pressing Ctrl+S (or double-clicking Save) while saving a draft note no longer saves the same note twice, and saved note ids stay unique even within one millisecond.

src/ui/hooks/useReviewController.test.tsx

@@ -1,4 +1,4 @@
-import { describe, expect, test } from "bun:test";
+import { describe, expect, spyOn, test } from "bun:test";
 import { testRender } from "@opentui/react/test-utils";
 import { act, StrictMode, useEffect, useState } from "react";
 import { SourceTextTooLargeError } from "../../core/fileSource";
@@ -658,6 +658,60 @@ describe("useReviewController", () => {
     }
   });
 
+  test("rapid duplicate saves persist exactly one user note with a unique id", async () => {
+    const { controllerRef, setup } = await renderReviewController([createAlphaFile()]);
+    const fixedNow = 1_700_000_000_000;
+    const dateNowSpy = spyOn(Date, "now").mockReturnValue(fixedNow);
+
+    try {
+      await flush(setup);
+
+      await act(async () => {
+        expectValue(controllerRef.current).startUserNote();
+        expectValue(controllerRef.current).updateDraftNote("Save me once.");
+      });
+      await flush(setup);
+
+      // Coalesced Ctrl+S key events invoke save twice before the draft-clearing
+      // state update commits; only the first call may persist a note.
+      const savedIds: { first?: string; second?: string; followUp?: string } = {};
+      await act(async () => {
+        const controller = expectValue(controllerRef.current);
+        savedIds.first = controller.saveDraftNote()?.id;
+        savedIds.second = controller.saveDraftNote()?.id;
+      });
+      await flush(setup);
+
+      expect(savedIds.first).toBe(`user:${fixedNow}-1`);
+      expect(savedIds.second).toBeUndefined();
+      expect(expectValue(controllerRef.current).userNotesByFileId.alpha).toHaveLength(1);
+
+      // A follow-up draft saved within the same millisecond still gets a unique id.
+      await act(async () => {
+        expectValue(controllerRef.current).startUserNote();
+      });
+      await flush(setup);
+      await act(async () => {
+        expectValue(controllerRef.current).updateDraftNote("Save me too.");
+      });
+      await flush(setup);
+
+      await act(async () => {
+        savedIds.followUp = expectValue(controllerRef.current).saveDraftNote()?.id;
+      });
+      await flush(setup);
+
+      expect(savedIds.followUp).toBe(`user:${fixedNow}-2`);
+      expect(savedIds.followUp).not.toBe(savedIds.first);
+      expect(expectValue(controllerRef.current).userNotesByFileId.alpha).toHaveLength(2);
+    } finally {
+      dateNowSpy.mockRestore();
+      await act(async () => {
+        setup.renderer.destroy();
+      });
+    }
+  });
+
   test("session clear can include human user notes", async () => {
     const { controllerRef, setup } = await renderReviewController([createTwoHunkFile()]);
 

src/ui/hooks/useReviewController.ts

@@ -216,6 +216,11 @@ export function useReviewController({
   );
   const [userNotesByFileId, setUserNotesByFileId] = useState<Record<string, UserReviewNote[]>>({});
   const [draftNote, setDraftNote] = useState<DraftReviewNote | null>(null);
+  // Track the last saved draft id so coalesced save key events dedup synchronously
+  // without waiting for the draft-clearing state update to commit.
+  const savedDraftIdRef = useRef<string | null>(null);
+  // Monotonic suffix that keeps `user:*` note ids unique within one millisecond.
+  const userNoteSequenceRef = useRef(0);
   const [expandedGapsByFileId, setExpandedGapsByFileId] = useState<
     Record<string, ReadonlySet<string>>
   >({});
@@ -871,6 +876,7 @@ export function useReviewController({
         newRange: target.side === "new" ? [target.line, target.line] : undefined,
         body: "",
       };
+      savedDraftIdRef.current = null;
       setDraftNote(draft);
       selectHunk(
         file.id,
@@ -892,9 +898,9 @@ export function useReviewController({
     setDraftNote(null);
   }, []);
 
-  /** Persist the active draft into the in-memory user note collection. */
+  /** Persist the active draft into the in-memory user note collection exactly once. */
   const saveDraftNote = useCallback((): UserReviewNote | null => {
-    if (!draftNote) {
+    if (!draftNote || savedDraftIdRef.current === draftNote.id) {
       return null;
     }
 
@@ -904,8 +910,10 @@ export function useReviewController({
       return null;
     }
 
+    savedDraftIdRef.current = draftNote.id;
+
     const savedNote: UserReviewNote = {
-      id: `user:${Date.now()}`,
+      id: `user:${Date.now()}-${++userNoteSequenceRef.current}`,
       source: "user",
       filePath: draftNote.filePath,
       hunkIndex: draftNote.hunkIndex,

test/pty/notes.test.ts

@@ -116,6 +116,40 @@ describe("PTY notes", () => {
     }
   });
 
+  test("rapid Ctrl+S presses save a draft note exactly once", async () => {
+    const fixture = harness.createLongWrapFilePair();
+    const session = await harness.launchHunk({
+      args: ["diff", fixture.before, fixture.after, "--mode", "split"],
+      cols: 120,
+      rows: 24,
+    });
+
+    try {
+      await session.waitForText(/View\s+Navigate\s+Agent\s+Help/, { timeout: 15_000 });
+
+      await session.press("c");
+      await session.waitForText(/Draft note/, { timeout: 5_000 });
+      await session.type("Save exactly one note.");
+      await session.waitForText(/Save exactly one note\./, { timeout: 5_000 });
+
+      // Send both Ctrl+S bytes in one PTY write so the second save request runs
+      // before the draft-clearing state update commits.
+      session.writeRaw("\x13\x13");
+      await session.waitIdle();
+
+      const saved = await session.waitForText(/Your note/, { timeout: 5_000 });
+      expect(saved).toContain("Save exactly one note.");
+
+      // A duplicated save renders numbered "Your note 1/2" cards; a single save must not.
+      await sleep(250);
+      const settled = await session.text({ immediate: true });
+      expect(settled).not.toContain("Your note 1/");
+      expect((settled.match(/Your note/g) ?? []).length).toBe(1);
+    } finally {
+      session.close();
+    }
+  });
+
   test("add-note affordance appears only after mouse movement in a real PTY", async () => {
     const fixture = harness.createScrollableFilePair();
     const session = await harness.launchHunk({