Repository navigation
perf(desktop-gpui): read and decode the screenshot clipboard image off the UI thread - #2756
celeroncoder wants to merge 2 commits into
Conversation
|
@celeroncoder is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
|
cc @richiemcilroy for review (GitHub doesn't let a fork request reviewers, so tagging instead). |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| cx.spawn(async move |cx| { | ||
| let image = cx | ||
| .background_executor() | ||
| .spawn(async move { | ||
| let bytes = std::fs::read(&path) | ||
| .map_err(|error| format!("Could not read the clipboard image: {error}"))?; | ||
| prepare_clipboard_image(bytes) | ||
| }) | ||
| .await?; | ||
| cx.update(|cx| write_clipboard_image(image, cx)) |
There was a problem hiding this comment.
copy_image_to_clipboard now writes in the order background work finishes, not the order users click Copy. The library leaves its Copy buttons active and detaches every request. If a user copies a large screenshot and then a smaller one while the first is still preparing, the older request can finish last and replace the newer image. Track request order and skip stale writes, or queue the copies so an older request cannot overwrite a newer one.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/platform.rs
Line: 565-574
Comment:
**Older copies replace newer ones**
`copy_image_to_clipboard` now writes in the order background work finishes, not the order users click Copy. The library leaves its Copy buttons active and detaches every request. If a user copies a large screenshot and then a smaller one while the first is still preparing, the older request can finish last and replace the newer image. Track request order and skip stale writes, or queue the copies so an older request cannot overwrite a newer one.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Valid, and it is a regression this PR introduced: before the change the read and write were synchronous in the click handler, so writes happened in click order. Two call sites can genuinely invert: the library Copy (main_window.rs, library_action has no disabled state) and the settings Copy (settings_window.rs, row_button is called with disabled: false), and they are different windows in the same process. screenshot_finished is reachable but needs a second capture to start inside the first copy's prepare. The editor export cannot invert: exporting gates the action and disables the buttons, and it does not go through this helper.
Fixed in 779c03c with a process-wide generation counter in the shared helper rather than per call site, since the windows share one clipboard. platform.rs gains a small ClipboardWrites { latest: AtomicU64 } (request() / is_current()); copy_image_to_clipboard stamps its id synchronously before spawning, and the foreground step skips the write (debug log, Ok(())) when a newer request exists, otherwise writes. The compare and the write sit in the same cx.update closure, so only the highest id can ever reach the clipboard. A superseded write is Ok(()) because it is not a failure: the clipboard already holds the newer image, and the helper's call sites alert on Err. The editor path is untouched. ClipboardWrites is unit-tested without an App: newest request wins, a completed write does not invalidate a later request, concurrent requests get distinct ids, ids are strictly increasing.
Fix written by Claude Opus 5.5 from a DeepSeek V4.1 Flash analysis of the finding, reviewed by Gemini 3.8 Flash.
|
Thanks @celeroncoder! Greptile is at 4/5 here. We only merge once Greptile reaches 5/5, so please address its findings in the summary above (or reply to them if you think one is wrong), then comment |
…nish out of order
|
@greptileai review |
|
The red |
This branch has not been deployed
2 participants
After a screenshot,
screenshot_finishedcopied the image to the clipboard on the UI thread:std::fs::readof the full-resolution PNG (tens of MB on a Retina display), then per platform the clipboard conversion, right when the tray, library and editor are also refreshing. On Linux that was a format sniff,bytes.to_vec(), a content hash and another clone insideClipboardItem::new_image; on Windows a fullRustImageData::from_bytesdecode; on macOSNSData+NSImage+ the pasteboard write.Change
copy_image_to_clipboard(path, cx)now returns aTask: the file read andprepare_clipboard_imagerun on the background executor, and onlywrite_clipboard_imageruns on the UI thread. Each platform module exports the same three items (ClipboardImage,prepare_clipboard_image,write_clipboard_image):ClipboardItemfrom the owned bytes (noto_vec(), nonew_imageclone; the hash runs in the background). Write istry_write_to_clipboard.RustImageData::from_bytesdecode in the background (RustImageDatais{u32, u32, Option<DynamicImage>}, so it isSend).ClipboardContext::newandset_imagestay on the UI thread.NSData,NSImageand the pasteboard write stay on the main thread, same body as before.Error strings are unchanged. The same treatment is applied to the library "Copy" action, the settings "Copy" action and the editor's export-to-clipboard; the editor's success toast still fires only after the write succeeds.
Because prepares now finish in completion order rather than click order, a process-wide generation counter (
ClipboardWrites) stamps every request synchronously and the foreground step only writes if no newer request exists; a superseded copy is skipped with a debug log and returnsOk(()), so only the newest copy ever reaches the clipboard. Nothing afterscreenshot_finishedreads the clipboard, so the only observable change is that the clipboard is populated a few milliseconds after the editor opens instead of before.Results
Linux, debug build (dependencies at opt-level 2), synthetic 5120×2880 PNG of 27.8 MB, medians of 5 runs, 2 vCPU aarch64 Linux box.
main@ 2a61427The
try_write_to_clipboardcall itself is unchanged and not included. On Windows,set_imagestill re-encodes to PNG and BMP on the UI thread; that is the largest remaining cost there and is out of scope here. macOS keeps its AppKit main-thread work by design.Overlap
screenshot_editor.rs/app_windows.rs; both apply cleanly on top of this branch. perf(desktop-gpui): encode screenshot save and copy with the parallel PNG writer #2717 (open, parallel PNG writer) speeds up the encode half of copy inkernels/and doesn't overlap.src-tauri/automation.rs); same domain, no shared code.main.main.How this was made
This PR is agent-generated.
mainby OpenCode scouts and Gemini 3.8 Flash, re-checked against the source by the orchestrating Claude session.main, checkedclipboard_rsforSend, and took the Linux measurement above.Sendbounds from theclipboard_rsand gpui sources, the identical signatures across the three platform modules, that every task is detached and every toast fires after the write, re-rancargo check, clippy and theplatform::tests on Linux, and approved. Neither agent had a macOS or Windows machine to build on.🤖 Generated with Claude Code