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

[MiMoHo]

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 updateMenu → updateMenuContaining:, 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) — and, importantly, after menuDidClose:. 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

Seven commits. The first five deal with the status menu: defensive guards at the point of paste, prevention of the mid-selection rebuild, making the selection independent of menu geometry altogether, a correction to how that last part establishes identity, and closing two ways the freeze from Commit 2 could outlive its surface. The last two apply the same idea to the bezel, which turned out to have the same defect.

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.
• 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

A rebuild landing during menu tracking also shifts items under the cursor, so a click can hit the visual neighbour of the intended entry.

• 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.

Trade-off: a clipping that arrives while the menu is open is picked up on close rather than appearing live in the open menu.

Commit 3 — resolve the selection by clipping identity

Commits 1 and 2 turned out to be necessary but not sufficient, which is what this commit addresses:

• The freeze cannot close the window that matters. AppKit delivers the item action after menuDidClose:, and menuDidClose: is exactly where the freeze is lifted and the catch-up pollPB: is fired — straight into the unresolved-selection window. Other rebuild triggers ignore isMenuOpen entirely (the store delegate's endUpdates schedules updateMenu via NSRunLoopCommonModes; the in-menu search field calls updateMenuContaining: directly; and pollPB: checks the flag only on entry, so a read started before the menu opened still completes into addClipping).
• Commit 1 could only bail out. Once the item was detached there was nothing left to resolve, so the click did nothing.

So the row now carries what it stands for instead of relying on where it sits:

• updateMenuContaining: attaches [store position, display string] as each item's representedObject. The store position is known right there because previousDisplayStrings:containing: and previousIndexes:containing: are element-wise aligned (both newest-first, same predicate, same limit).
• storePositionForMenuItem: resolves a click from that, treating the position only as a hint that is re-validated against the current store. A rebuild between click and delivery is therefore harmless rather than merely detected.
• pasteIndexAndUpdate: becomes pasteStorePositionAndUpdate: and no longer reads the menu's search box at paste time. That box is cleared asynchronously by updateMenu, so a click arriving after the clearing used to resolve a filtered row number against the unfiltered store — another way to paste the newest clipping. The bezel path benefits too: moveItemAtStackPositionToTopOfStack passes a store position, which the old method would have run through the menu's search mapping.
• Menu items removed by a rebuild are held for one generation. jcMenu is their only owner (there is a -release right after -insertItem:), so a sender delivered after the rebuild would otherwise be a freed object — which Commit 1's nil check would already have had to touch.
• When a click still cannot be resolved, nothing is pasted, the failure is logged without any clipping contents, and NSBeep gives feedback — so "nothing happened" can be told apart from a wrong paste in a bug report.

Commit 4 — correct the identity: the clipping, not its display string

Commit 3 initially used the display string as the identity, on the assumption that -[FlycutClipping displayString] hands out a pointer-unique string per clipping. That assumption is wrong for short strings. A display string is the first line truncated to displayLen, and short strings are tagged pointers, so two different clippings whose first lines happen to match compare pointer-equal. A test program built against FlycutClipping shows "test\nZeile A" and "test\nZeile B" both yielding 0x802e93b4acce3b26.

The consequence was the very bug this PR is about, only rarer: with the hint invalidated by a store change, resolution could land on the wrong twin — and the hint check itself could accept a position that a same-looking clipping had moved into.

So the row now carries the FlycutClipping object. Object pointers are exact and the comparison is cheaper than a string compare; a contents comparison remains as a fallback for a store reloaded from disk, where the objects are new but two clippings with equal contents are interchangeable for pasting anyway. This needs one read-only accessor on FlycutOperator, -clippingAtPosition:, forwarding to the store. The clipping is fetched before the NSMenuItem is allocated, so a store change mid-build cannot leak an item.

Verified with a test program covering unchanged store, shifted store, a same-looking twin sitting at the hint position, a store reloaded from disk, and a deleted clipping (which must resolve to −1 and paste nothing). All six resolve to the clipping the user clicked.

Commit 5 — the freeze must not outlive its surface

An audit of the branch turned up two ways isMenuOpen / isSearchWindowDisplayed can be left standing. Because Commit 2 makes pollPB: return early on either flag, a stale flag means Flycut silently stops recording clippings, with nothing in the UI or the log to say why. That is a worse failure than the one this PR set out to fix, so it is fixed here rather than left for later.

• Option-click on the status icon is the documented way to pause capture while copying a password (help.md, readme.md). It cancels the menu from inside menuWillOpen:, and AppKit does not send menuDidClose: for a menu cancelled there — verified with a standalone AppKit program: menuWillOpen fires, menuDidClose never does. So isMenuOpen stayed set forever. The first option-click was harmless, because pollPB: also requires the store to be enabled; the second one re-enabled the store while capture stayed frozen. Now cleared in that branch, where the menu is not opening anyway.
• windowDidResignKey: routed every window to hideApp, which does not close the search window properly, so isSearchWindowDisplayed survived clicking away from it. It now closes the search window through hideSearchWindow. Before the freeze this only leaked a bit of state; with it, capture died.
• Both flags are additionally cleared in hideApp as a backstop, since hiding the app tears down the menu and the search window anyway.

Commits 6 and 7 — the bezel has the same defect

The bezel is the third selection surface, and it was exempt from both fixes above: pollPB: keeps capturing while it is open (deliberately — with stickyBezel it can stay up indefinitely, so freezing capture there is not an option), and addClipping: reset stackPosition to 0 on every insert. A clipping arriving mid-selection therefore moved the selection onto it, and the user pasted the newest entry instead of the one they had picked — the same user-visible bug, on a different surface.

Commit 6 makes the position follow its clipping. addClipping: anchors on the clipping at stackPosition before changing the store and looks up where it ended up afterwards. indexOfClipping: matches on contents, which is what is wanted: two clippings with equal contents are interchangeable for pasting, and it also covers removeDuplicates, where the existing copy is moved to the top rather than a new one being inserted. The anchor is retained, because the store can drop clippings while inserting.

Position 0 is deliberately not anchored. It is where every selection starts, so it expresses no choice, and the bezel always shows whatever sits at the top — following the old top clipping from there would move the selection away from what the user is looking at. An earlier revision of this commit did anchor at 0, and that was a regression for the default flow (stickyBezel off: hold, tap, release, without ever navigating).

Because the bezel opening at the newest clipping used to be a side effect of that reset, hitMainHotKey: now says so explicitly. Not in showBezel:, which the favourites toggle also calls after toggleToFromFavoritesStore has swapped in that store's own remembered position.

No new state is introduced, so there is no new way for a flag to be left standing and stop capture — which is what Commit 5 had to repair twice.

Commit 7 fixes an ordering problem the same change exposed. FlycutStore redraws the bezel synchronously from inside its own mutations (delegateEndUpdates → -[AppController endUpdates], which always has needBezelUpdate set via noteChangeAtIndex:). Any code that mutates the store and adjusts stackPosition afterwards therefore draws once with a position that no longer matches the contents. Two places did:

• addClipping: — cannot settle the position before the insert, since the new index is only known afterwards, so the clipping-added callback redraws once it is settled.
• getPasteFromIndex: with pasteMovesToTop — moved the clipping and then set the position to 0, so the redraw used the pre-move position, which after the move points at the neighbour of the clipping being pasted. Reported from a visual test as a brief flash of the wrong entry on Return; the paste itself was correct. Assigning the position first fixes it, because once the clipping has been moved to the top, position 0 is that clipping.

Verification

• Builds with Xcode 26.5 (xcodebuild -scheme Flycut -configuration Release, BUILD SUCCEEDED) with all seven commits. Warning count goes down, 75 → 73, with no new warnings introduced.
• The bezel change is covered by a test program against the real FlycutStore/FlycutClipping: a clipping arriving mid-selection, two in a row, an untouched bezel at position 0 (also with a single stored clipping, and after navigating back up to 0), the anchor pushed out by the rememberNum limit, and removeDuplicates both with a duplicate above the selection and of the selection itself. A second program stands in for the store delegate and records what the bezel would draw at endUpdates, which reproduces the reported flash and confirms Commit 7 removes it.
• Both bezel commits were confirmed by a human visual test before submission: pasting an older entry while a clipping arrives mid-selection, the untouched-bezel case, a control run, and a re-check that the flash is gone. Keyboard synthesis is TCC-blocked from a terminal context, so the bezel cannot be driven programmatically — this part genuinely needed a person.
• Commit 3 has been running in daily use for 3.5 days in a build that is source-identical in the menu-selection path. Over that period the unified log shows menu pastes completing normally, zero unresolved menu item bail-outs and zero exceptions or crashes — i.e. the bail-out path is not being hit in normal operation. Commit 4 changes only how the row's identity is established, not the control flow around it.
• Commit 2 was separately confirmed live before Commit 3 existed.
• The standalone AppKit mechanism test above still stands for the nil-menu path; a second test program covers the identity resolution (see Commit 4).

Honest caveat on Commit 3's motivation: the specific user-reported incidents that prompted it could not be proven to be the orphan race from the log alone — the unified log cannot observe which index was used, which is precisely why the new failure path now logs. What is proven is that the defect was present in the shipped binary (verified by disassembly of processMenuClippingSelection:) and that the selection no longer depends on menu geometry.

---

Authored and reviewed with Claude Code using Claude Opus 5 at xhigh reasoning effort. Root cause analysis, the fix design and the code review were cross-checked by a multi-agent adversarial pass (independent Opus 5 agents tasked with refuting each claim; findings that two of three refuted were dropped). Binary-level verification (section hashing and disassembly) was used to confirm which code was actually running.

🤖 Generated with Claude Code