Skip to content

Fix menu click sometimes pasting the newest clipping instead of the clicked one#7

Open
MiMoHo wants to merge 2 commits into
haad:masterfrom
MiMoHo:menu-click-paste-race
Open

Fix menu click sometimes pasting the newest clipping instead of the clicked one#7
MiMoHo wants to merge 2 commits into
haad:masterfrom
MiMoHo:menu-click-paste-race

Conversation

@MiMoHo

@MiMoHo MiMoHo commented Jul 5, 2026

Copy link
Copy Markdown

Symptom

Sometimes, clicking an older entry in the status menu pastes the most recently copied clipping instead of the one that was clicked. It is intermittent and more likely the further down the list you click.

Root cause

The chain is fully deterministic once the timing lines up:

  1. pollPB: runs on a 1-second timer scheduled in NSRunLoopCommonModes — deliberately, so Universal Clipboard arrivals are noticed while the menu is open (comment in awakeFromNib).

  2. When it notices a clipboard change, addClipping: fires updateMenuupdateMenuContaining:, which removes every clipping item and inserts brand-new NSMenuItem objects (also scheduled in NSRunLoopCommonModes).

  3. NSMenu dispatches the clicked item's action slightly after mouse-up (the highlight blink). If the rebuild from step 2 lands in that window, the clicked item has been removed from the menu, so in processMenuClippingSelection::

    int index = [[sender menu] indexOfItem:sender];

    [sender menu] is nil, messaging nil yields 0, and pasteIndexAndUpdate:0 pastes stack position 0 — the newest clipping. With menuSelectionPastes defaulting to YES, the fake Cmd-V then pastes it into the frontmost app.

No external clipboard source is needed: with the 1 s poll interval, the user's own Cmd-C is often detected only after the menu is already open (copy → open menu → browse → click lands right around the poll tick).

Standalone proof of the mechanism (AppKit, compiled with clang -fno-objc-arc -framework AppKit):

before rebuild: [sender menu] = 0x100be9e70, indexOfItem = 7 (correct)
after rebuild:  [sender menu] = 0x0, [[sender menu] indexOfItem:sender] = 0
=> pasteIndexAndUpdate:0 pastes the MOST RECENT clipping, not the clicked one

(Test source is a ~50-line program that builds a menu, retains item 7 as sender, replaces all items the way updateMenuContaining: does, and evaluates the exact expression from processMenuClippingSelection:.)

Fix

This PR now contains two commits — defensive guards at the point of paste, and root-cause prevention of the mid-selection rebuild.

Commit 1 — defensive guards

  • processMenuClippingSelection: — resolve the index defensively; if the clicked item is orphaned (menu nil) or its index can't be resolved, do nothing rather than paste the wrong clipping. The menu is current again on the next click.
  • pasteIndexAndUpdate: — now returns whether a clipping was actually placed on the pasteboard. Previously, when it silently found no content, the caller still faked Cmd-V, re-pasting whatever was already on the pasteboard (same visible symptom through a second hole). Also bounds-checks the search mapping, which could throw NSRangeException when the list changed between menu build and click.
  • searchWindowItemSelected: — for double-clicks, use clickedRow instead of selectedRow, so a selection reset between click and action (e.g. updateSearchResults re-selecting row 0 when the search field action fires) can't redirect the paste to the newest entry; double-clicks below the last row are ignored; the search mapping is bounds-checked.

Commit 2 — freeze capture while the menu/search window is open

This is the follow-up flagged as out-of-scope in the original writeup, now included. It attacks the root cause instead of only softening the symptom: a rebuild landing during menu tracking also shifts items under the cursor, so a click can hit the visual neighbour of the intended entry (and the store index the click resolves to no longer matches what the user saw).

  • New BOOL isMenuOpen, set in menuWillOpen: / cleared in menuDidClose:.
  • pollPB: returns early while isMenuOpen || isSearchWindowDisplayed, without touching pbCount, so the clipboard change is still detected and captured the moment the surface closes; menuDidClose: runs a catch-up pollPB: for immediacy.

With capture frozen, the store cannot mutate between menu-open and click, so the item indices the user sees stay valid — the click resolves to the entry they actually clicked (Commit 1's guards then remain as belt-and-suspenders for any residual orphaning). Trade-off: a clipping that arrives while the menu is open is picked up on close rather than appearing live in the open menu — a fair price for never pasting the wrong entry, and it also stops items jumping under the cursor mid-selection.

Verification

  • Builds with Xcode 26.5 (xcodebuild -scheme Flycut -configuration Release, BUILD SUCCEEDED) — both commits together. The original standalone mechanism test above still stands for the nil-menu path.
  • Commit 2 tested live: with the frozen-capture build installed, opening the menu and clicking an older entry now reliably pastes that entry (previously reproduced the newest-instead-of-clicked paste).
  • clang -fsyntax-only diagnostics match master (no new warnings).

🤖 Generated with Claude Code

Milan Hoppe and others added 2 commits July 5, 2026 02:29
…licked one

The pollPB: timer runs in NSRunLoopCommonModes, so a clipboard change can be
noticed while the status menu is open (deliberate, for Universal Clipboard).
That triggers updateMenu, which removes every clipping item and inserts new
NSMenuItem objects.  If the rebuild lands between the user's click and the
action dispatch, the clicked item is no longer in the menu, [sender menu] is
nil, and [[sender menu] indexOfItem:sender] messages nil and yields 0 --
pasting stack position 0, the most recently copied clipping, instead of the
entry the user clicked.  With the 1-second poll interval this commonly
happens when the user copies something and opens the menu right away.

- processMenuClippingSelection: bail out when the sender is orphaned or its
  index can't be resolved, instead of pasting the wrong clipping.
- pasteIndexAndUpdate: now returns whether a clipping was placed on the
  pasteboard, so no Cmd-V is faked when nothing was pasted (previously that
  re-pasted whatever was already on the pasteboard).  Also bounds-check the
  search mapping, which could throw NSRangeException.
- searchWindowItemSelected: use clickedRow for double-clicks so a selection
  change between click and action can't redirect the paste to row 0, and
  bounds-check the search mapping.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pollPB: runs in NSRunLoopCommonModes so it keeps firing during menu
tracking. When it notices a clipboard change mid-selection it inserts a
new clipping at store index 0, shifting every clipping's index by one.
The pending selection is then resolved against the shifted store, so the
user pastes the wrong entry - typically the freshly-arrived newest one -
instead of the one they clicked. The same shift breaks the search
window, whose result list is a snapshot while getPasteFromIndex: uses
live indices.

Guard pollPB: to skip capture while isMenuOpen or isSearchWindowDisplayed,
without touching pbCount so the change is still detected and captured the
moment the surface closes (menuDidClose: fires a catch-up poll). Track
menu open state via menuWillOpen:/menuDidClose:.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.

1 participant