Skip to content

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

Merged
benvinegar merged 2 commits into
modem-dev:mainfrom
endotakuya:fix/rapid-save-duplicate-note
Jul 21, 2026
Merged

fix(review): save draft notes exactly once under rapid Ctrl+S#581
benvinegar merged 2 commits into
modem-dev:mainfrom
endotakuya:fix/rapid-save-duplicate-note

Conversation

@endotakuya

Copy link
Copy Markdown
Contributor

Summary

  • Guard saveDraftNote re-entry with a ref keyed by the draft id, so save events that arrive before the draft-clearing state update commits are no-ops.
  • Suffix saved note ids with a monotonic counter so user:* ids stay unique within one millisecond.

Closes #580.

Why

React state is a per-render snapshot, so the old if (!draftNote) guard could not reject a second Ctrl+S delivered in the same stdin chunk (or a Save (^S) double-click): both events saw the still-non-null draft and each appended a note. The duplicates also shared one user:${Date.now()} id, so id-addressed removal (the [x] close button, hunk session comment remove) silently deleted both copies.

Testing

  • bun run typecheck
  • bun run lint
  • bun test src/ui/hooks/useReviewController.test.tsx (24 passed)
  • bun test ./test/pty/notes.test.ts (13 passed, includes the new rapid Ctrl+S regression test, which also reproduces the bug against the released v0.17.3 binary via HUNK_TEST_EXECUTABLE)
  • bun test ./src ./packages ./scripts ./test/cli (1,180 passed; ./test/session fails on this machine identically on origin/main due to a live local hunk daemon, so relying on CI there)

Rapid save events (coalesced stdin chunks, double-clicks) re-entered
saveDraftNote before the draft-clearing state update committed, saving
duplicate notes that shared one user:<ms> id. Guard re-entry with a ref
keyed by draft id and suffix note ids with a monotonic counter.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YJ4Wfy9erR3MnYVe9jAtvG
@endotakuya
endotakuya marked this pull request as ready for review July 20, 2026 17:00
@greptile-apps

greptile-apps Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

PR author is not in the allowed authors list.

@benvinegar benvinegar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix itself looks good, and I reproduced the issue and verified the fix in tmux. Could you tighten up these two tests before merging?

  • In the PTY test, session.type("\x13\x13") sends the two characters separately with a short delay, so it doesn't guarantee the coalesced input described in the comment. await session.write("\x13\x13") would send both bytes in one PTY write and make the regression test deterministic.
  • In the controller test, please freeze or mock Date.now() for both saves. As written, the IDs may differ because the clock advanced between renders, so it doesn't actually prove IDs stay unique within the same millisecond.

This comment was generated by Pi using OpenAI GPT-5.4

@benvinegar benvinegar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed the two test fixes: the PTY test now sends both Ctrl+S bytes in one write, and the controller test holds Date.now() constant while checking the exact sequence IDs. The focused tests, typecheck, lint, and formatting all pass.

This comment was generated by Pi using OpenAI GPT-5.4

@benvinegar
benvinegar merged commit b94aab6 into modem-dev:main Jul 21, 2026
8 checks passed
@endotakuya

Copy link
Copy Markdown
Contributor Author

@benvinegar Thank you for the review, the test fixes, and the merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rapid save (Ctrl+S or double-click) duplicates a draft note

2 participants