Fix popup rendering, bounds, pinned-paste crash, and search swallow#1378
Open
p3ngu1nx wants to merge 7 commits into
Open
Fix popup rendering, bounds, pinned-paste crash, and search swallow#1378p3ngu1nx wants to merge 7 commits into
p3ngu1nx wants to merge 7 commits into
Conversation
The app would freeze ("Not Responding" at 100% CPU) when hovering over
clipboard items because the preview panel triggered expensive synchronous
operations on the main thread:
- hasImage was creating a full NSImage just to check existence; now uses
the cheap imageData lookup instead
- Image decode and resize (NSImage(data:) + .resized()) ran on @mainactor
via inherited Task context; now uses Task.detached to run on background
thread, assigning results back on MainActor
- HTML/RTF text preview parsed NSAttributedString synchronously in the
view body; now wrapped in AsyncView with background Task.detached and
caching
The previous fix (0e13280) moved heavy work off the main thread but introduced two issues: 1. AsyncView used .task{} which fires once on appear — navigating between items showed stale preview content because the task never re-fired. The initial fix (.id(item.id) on PreviewItemView) caused full view teardown/recreation on every hover, which froze the app when the user moved the mouse rapidly across items. Fix: Added an optional id parameter to AsyncView that uses .task(id:) internally. This re-fires the async operation when the item changes WITHOUT destroying the view — just cancels the old task and starts a new one. 2. asyncGetPreviewText() read ALL content types (text, RTF, HTML, fileURLs) upfront on the main thread even when only one was needed. For browser-copied items with both text+HTML, this doubled the SwiftData reads unnecessarily. Fix: Lazy content reading following the priority chain — returns immediately for plain text (most common case), only falls through to Task.detached for RTF/HTML parsing. Also marked cachedPreviewText as @ObservationIgnored to prevent unnecessary view updates.
The popup window and its preview slideout could extend beyond the screen edges — most visibly when the popup appeared near the bottom or left edge of the screen: - FloatingPanel.open(): added screen bounds clamping after computing the popup origin so the window never starts below, left of, or right of the visible screen area - SlideoutController.togglePreview(): added screen bounds clamping after the left-shift for left-side slideout placement so the preview panel doesn't go off the left or bottom edge
- Complete Y-axis clamping in FloatingPanel.open() and SlideoutController.togglePreview() — also clamp the top edge so the window can't extend above visibleFrame.maxY when height is larger than the screen can accommodate below the origin. - In asyncGetPreviewText(), skip caching an empty fileURL result so pathological clipboard items fall through to the slow path instead of being permanently cached as empty.
Rows containing image clipboard items could trigger a runaway SwiftUI layout transaction loop, producing a 12s+ main-thread hang at 100% CPU on rows holding large composite screenshots. A CPU resource diagnostic captured the freeze in LazySubviewPlacements.placeSubviews -> LazyStack.place -> LazyLayoutViewCache.commitPlacedSubviews -> Array.sortForDisplayLarge, with no time in image decoding (decoding is already off-thread). Two contributing factors: 1. The history row rendered Image(nsImage:) without an explicit frame and switched between image / accessoryImage / title-fallback branches depending on whether the async thumbnail had completed. The view tree shape changed when thumbnails finished loading, which forced LazyVStack to re-measure the row and invalidate its row-height cache for everything below. 2. NSImage+Resized.resized(to:) returned an NSImage(size:flipped: drawingHandler:) whose contents were a deferred draw. The intrinsic size reported to SwiftUI varied subtly per redraw, defeating the lazy layout cache. Fix: * ListItemView wraps the image / accessoryImage branches in a single Group with an explicit frame capped at imageMaxHeight, so the row's view shape is stable across async thumbnail completion. * NSImage+Resized.resized(to:) eagerly bakes an NSBitmapImageRep at the highest active screen's backingScaleFactor so the cached bitmap stays sharp on any display the popup is dragged to, and falls back to self if either the bitmap or the graphics context can't be allocated. Trade-off: the bake target is colorSpaceName: .deviceRGB, which converts P3-tagged source screenshots to deviceRGB at thumbnail size. Visually imperceptible at <=340pt but a real change vs. the prior deferred-draw path which preserved the source colorspace until composite time. .resizable().scaledToFit() is intentionally omitted on the SwiftUI Image - Popup.itemHeight = 24 would otherwise become the row floor and shrink the thumbnail into a 24pt cell. The eager bake gives the NSImage a correct intrinsic point size, and the explicit frame is the only sizing control needed.
When a pinned item was pasted, the pasteboard-poll timer would re-enter History.add(_:) with a duplicate item. The existing pinned row is removed from `all` and its index is captured, then limitHistorySize(to:) runs and can mutate `all` further by deleting unpinned rows. The captured index becomes stale, and the subsequent Array.insert(at:) can trap with "index out of range". Crash signature: _assertionFailure Array._checkIndex Array.insert(_:at:) History.add(_:) History.swift:181 Clipboard.checkForChangesInPasteboard() __NSFireTimer Fix: clamp the captured index to the current bounds of `all` before inserting. Array.insert(at:) accepts 0...count inclusive, so min(index, count) is always valid. When the original slot was trimmed away, the re-pinned item now lands at the end of `all`; in all other cases the behaviour is unchanged. Adds a regression test that fills the unpinned history to capacity and re-adds a duplicate of a pinned item with pinTo=.bottom, which reliably reproduced the crash on the pre-fix code path.
`Popup.handleKeyDown` only checked `event.keyCode` against the popup shortcut, ignoring modifiers. When the popup hotkey is `⌘⇧V` (the default), pressing plain "v" in the search field still satisfied `isHotKeyCode == true` and, depending on popup state, routed to the cycle/highlightNext branch instead of the search field. The "v" keystroke was consumed and the selection moved down — making it impossible to search for any term containing the letter V. Add a `guard isHotKeyModifiers(event.modifierFlags)` after the `pressedShortcutItem` block so non-hotkey-letter presses fall through to the search field. Drop the now-redundant modifier check from the `.toggle` branch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the "Not Responding" freeze that occurs when hovering over clipboard items with the preview panel, a separate list-scrolling freeze on rows that contain large image thumbnails, two positioning bugs where the popup could extend beyond the visible screen, a crash when re-pasting a pinned clipboard item while the unpinned history is at capacity, and a regression where the search field would swallow keystrokes matching the popup hotkey's letter.
The hover freeze is reproducible by scrolling through the history list and rapidly moving the mouse up and down across items — the main thread blocks hard enough that macOS's watchdog will eventually kill the process. The list-scrolling freeze reproduces independently by scrolling to a clipboard row that holds a large composite screenshot — the row's first appearance pegs the main thread at 100% CPU for 12+ seconds. The pinned-paste crash reproduces by filling the unpinned history to
Defaults[.size]and then re-pasting a pinned item —History.addtraps inArray.insert(_:at:)with "index out of range" (three production.ipsreports captured this signature on May 2-3). The search-swallow bug reproduces by opening the popup with the default⌘⇧Vhotkey, releasing modifiers, and typing "v" in the search field — the keystroke moves the selection down instead of inserting "v" into the query, making it impossible to search for any term containing the letter V.Root causes
Six separate main-thread / event-handling issues:
hasImageallocated a fullNSImagejust to check existence.item.image != nildecodes the entire image blob;item.imageData != nilis a cheap content-type lookup.Image decode and resize ran on
@MainActor. TheTask { }wrapper inherited the actor context from its@MainActorcaller, soNSImage(data:)and.resized(to:)still executed on the main thread. Replaced withTask.detached+ rawDatacapture.NSAttributedString(html:)parsed synchronously in the view body. HTML clipboard content (from browsers) invokes WebKit internally, which blocks rendering for hundreds of milliseconds. Moved behind an asyncAsyncViewoperation.LazyVStackrow-layout cycling on rows with image thumbnails. Separate from the hover path. The history row renderedImage(nsImage:)without an explicit frame and switched betweenimage/accessoryImage/title-fallbackbranches depending on whether the async thumbnail had completed. The view-tree shape changed when thumbnails finished loading, forcingLazyVStackto re-measure and invalidating its row-height cache for everything below. Compounding this,NSImage+Resized.resized(to:)returned anNSImage(size:flipped:drawingHandler:)whose intrinsic size varied subtly per redraw, defeating the lazy cache. The combined effect was a runaway SwiftUI transaction loop — captured by macOS's CPU resource diagnostic showing time spent inLazySubviewPlacements.placeSubviews→LazyStack.place→LazyLayoutViewCache.commitPlacedSubviews→Array.sortForDisplayLarge, with no time in image decoding (decoding is already off-thread after fix Cannot select with arrow keys after using mouse #2).History.addtrapped on stale index afterlimitHistorySizemutation. When a pinned clipboard item was re-pasted, the function capturedremovedItemIndexfor the existing pinned row, removed it, then calledlimitHistorySizewhich can mutateallfurther by deleting unpinned rows. The captured index then becomes stale, and the subsequentArray.insert(_:at:)traps with "index out of range" if the new array length is smaller than the captured index. Hits when the unpinned history is at capacity.Popup.handleKeyDownmatched the popup hotkey by keyCode only. The localNSEventmonitor's gateif isHotKeyCode(Int(event.keyCode))compared onlyevent.keyCodeagainst the popup shortcut's key, ignoring modifier flags. With the default⌘⇧Vpopup hotkey, plain "v" with no modifiers still satisfied this check; combined with akeyDown-vs-flagsChangedevent-ordering race that could leavestate == .openingorstate == .cycle, the keystroke routed to the cycle/highlightNextbranch andreturn nilswallowed it before the search field'sonKeyPressever ran. Once stuck in.cycle(and modifiers were already empty so no furtherflagsChangedwould fire), every subsequent "v" repeated the cycle behavior.A seventh issue emerged during the preview fixes: with the preview open,
AsyncView's.task { }fires only on appear, so navigating between items showed stale content. An initial attempt using.id(item.id)onPreviewItemViewcaused full view teardown/recreation on every hover — this itself froze the app under rapid mouse movement. The final fix adds ataskIdparameter toAsyncViewthat uses.task(id:), which cancels and restarts the async operation on id change without destroying the view.Changes
HistoryItemDecorator:hasImageusesimageData, image generation usesTask.detached, newasyncGetPreviewText()with lazy content reading and@ObservationIgnoredcache.HistoryItem:generateTitle()usesimageData == nilinstead ofimage == nil.AsyncView: Addedidinit parameter that switches to.task(id:)internally. Prior init withoutidis preserved for backwards compat.PreviewItemView: Text branch wrapped inAsyncView; both image and textAsyncViews passitem.id.FloatingPanel.open()andSlideoutController.togglePreview(): Clamp window origin to the visible screen frame on both axes so the popup/slideout can't extend off-screen.ListItemView:image/accessoryImagebranches wrapped in a singleGroupwith.frame(maxWidth: 340, maxHeight: imageMaxHeight), so the row's view shape stays stable across async thumbnail completion.NSImage+Resized.resized(to:): replaces the deferred-drawNSImage(size:flipped:drawingHandler:)with an eagerNSBitmapImageRepbake at the highest active screen'sbackingScaleFactor(so cached bitmaps stay sharp on multi-display setups). Falls back toselfif either the bitmap or graphics context can't be allocated.History.add(_:):all.insert(itemDecorator, at: removedItemIndex)becomesall.insert(itemDecorator, at: min(removedItemIndex, all.count)).Array.insert(at:)accepts0...count, so clamping tocountis always valid; when the original slot was trimmed, the re-pinned item lands at the end ofallinstead of crashing.Popup.handleKeyDown: adds aguard isHotKeyModifiers(event.modifierFlags) else { return event }after thepressedShortcutItemblock so non-hotkey-letter keystrokes fall through to the search field. The redundant&& isHotKeyModifiers(...)is dropped from thestate == .togglebranch (the new guard already enforces it). The cycle-paste workflow (⌘⇧Vheld →highlightNext) is unchanged because both keyCode and modifiers match in that case.HistoryDecoratorTests: Image tests madeasyncto await the detached generation; added tests forhasImageandasyncGetPreviewText.HistoryTests.testRepinningAtCapacityDoesNotCrash: regression test that fills unpinned history to capacity and re-adds a duplicate of a pinned item withpinTo = .bottom— reliably reproduced the crash on the pre-fix code path.Verification
Tested by copying a mix of content types (plain text, HTML from browsers, large images, file URLs) and:
Defaults[.size] = 3, pinning an item, filling the unpinned slots with three copies, and re-pasting the pinned item — pre-fix this crashed with "index out of range"; post-fix the pin survives and remains in the popup.⌘⇧V, releasing the modifiers, and typing "valley" / "vivid" / "vvvvv" in the search field — pre-fix the V keystrokes were swallowed and the selection cycled down; post-fix the characters insert into the query and history filters correctly. Holding⌘⇧V(the intended cycle gesture) still cycles the selection as before; pressing⌘⇧Vwhile the popup is in.togglestate still closes it.Unit tests pass.
Notes for reviewers
Databytes and value types cross intoTask.detached.NSImage+Resizedpath: the bake target iscolorSpaceName: .deviceRGB, which converts P3-tagged source screenshots to deviceRGB at thumbnail size. Visually imperceptible at ≤340pt but a real change vs. the prior deferred-draw path, which preserved the source colorspace until composite time..resizable().scaledToFit()is intentionally omitted on the SwiftUIImageinListItemView—Popup.itemHeight = 24would otherwise become the row floor and shrink the thumbnail into a 24pt cell. The eager bake gives the NSImage a correct intrinsic point size, and the explicit frame is the only sizing control needed.ListItemViewpreviously rendered theaccessoryImageand the mainimageas two stacked positions inside the row when both were non-nil. TheGroup { if let image … else if let accessoryImage … }wrapper picks one. In practice only one of the two is ever set on a given row, so this should be invisible in normal usage; calling it out for transparency.Popup.handleKeyDown: the existing call ordering is preserved on purpose.pressedShortcutItemruns before the new modifier guard so list-item shortcuts (⌘1, ⌘2, …) still resolve through their ownHistoryItemAction(modifierFlags)validation. The pre-existing requirement thatpressedShortcutItemis only consulted when the keyCode equals the popup hotkey's keyCode is left as-is — behaviour-preserving and out of scope for a one-line bugfix.