diff --git a/.changeset/rapid-save-duplicate-note.md b/.changeset/rapid-save-duplicate-note.md new file mode 100644 index 00000000..0088a73b --- /dev/null +++ b/.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. diff --git a/src/ui/hooks/useReviewController.test.tsx b/src/ui/hooks/useReviewController.test.tsx index 91c56efc..3814e624 100644 --- a/src/ui/hooks/useReviewController.test.tsx +++ b/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()]); diff --git a/src/ui/hooks/useReviewController.ts b/src/ui/hooks/useReviewController.ts index b25938e7..a0185291 100644 --- a/src/ui/hooks/useReviewController.ts +++ b/src/ui/hooks/useReviewController.ts @@ -216,6 +216,11 @@ export function useReviewController({ ); const [userNotesByFileId, setUserNotesByFileId] = useState>({}); const [draftNote, setDraftNote] = useState(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(null); + // Monotonic suffix that keeps `user:*` note ids unique within one millisecond. + const userNoteSequenceRef = useRef(0); const [expandedGapsByFileId, setExpandedGapsByFileId] = useState< Record> >({}); @@ -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, diff --git a/test/pty/notes.test.ts b/test/pty/notes.test.ts index f30957b1..5838e529 100644 --- a/test/pty/notes.test.ts +++ b/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({