Repository navigation
perf(desktop-gpui): decode settings thumbnails only for visible rows and downsample tray PNGs at decode - #2754
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). |
| .collect(); | ||
| assert_eq!(found, [49].into_iter().chain(490..500).collect::<Vec<_>>()); | ||
|
|
||
| // A row without a thumbnail file has nothing to decode. |
There was a problem hiding this comment.
This comment only restates the assignment and assertion below it. AGENTS.md prohibits comments that narrate what the code does. Remove it before merging to meet that requirement; the test already makes the case clear.
| // A row without a thumbnail file has nothing to decode. |
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/settings_window.rs
Line: 5090
Comment:
**The comment repeats the test**
This comment only restates the assignment and assertion below it. `AGENTS.md` prohibits comments that narrate what the code does. Remove it before merging to meet that requirement; the test already makes the case clear.
```suggestion
```
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Removed in 8079013, together with one more narrating comment in the same test ("Load more" queues only the newly shown rows…) that a second pass flagged. The remaining added comments were checked one by one and each captures a non-obvious invariant (unfiltered indices, search order, task-drop cancellation, PNG header alpha check).
Reviewed by DeepSeek V4.1 Flash and Gemini 3.8 Flash.
|
Thanks @celeroncoder! Greptile hasn't scored this one yet, so I've asked it to review below. We only merge once Greptile reaches 5/5, so once its review is in, please address its findings (or reply to them if you think one is wrong) and comment @greptileai review |
Two thumbnail paths in the GPUI app did far more decoding than they painted.
Settings → Recordings / Screenshots.
refresh_recordingsandrefresh_screenshotsscan the full library, andset_recordings/set_screenshotsthen queued a thumbnail decode for every row without a cached image, throughspawn_decode_pool. The lists only rendervisible_countrows (20, plus 20 per "Show more", reset on tab switch and search). On a library of a few thousand items that is thousands of decodes to show 20, repeated on every 2 s poll for rows that weren't cached yet.Tray → Previous.
load_thumbnail_pngrouted JPEG sources throughlibrary::thumbnail_rgba(the on-disk thumbnail cache, downsampled at decode) but decoded every PNG source at native resolution withimage::load_from_memoryand then resized it to a 32 px icon. Every screenshot is a PNG, and the menu is rebuilt after each capture and each recording stop.Change
Commit 1, settings pages:
visible_rows(matches, has_search, visible_count)is one pure helper that returns the indices of the rows that will be painted. Both renderers, both "Show more" handlers and the decode scheduling use it, so decodes and paints agree.pending_thumbnail_decodesqueues decodes only for visible rows (a stale recording row with a path, or a screenshot row without an image).decodetask separate fromscan. A scan always starts a decode of the visible pending rows; "Show more", a tab click and search edits start one only if something visible is pending; a new decode replaces the running one, and the pool's workers stop after their current job once the receiver is dropped. The "install only if the row still exists and the path matches" check is unchanged.Visible change: rows beyond the current page get their thumbnails when they are shown, so "Show more" briefly shows placeholders that fill in.
Commit 2, tray:
source_may_have_alphareads only the image header. JPEG is opaque; a PNG counts as alpha if its colour type has an alpha channel, which withimage'sEXPANDtransformation also covers palette and RGB PNGs with atRNSchunk.library::thumbnail_rgba(cached, downsample-at-decode, uniform scale so the cover crop is the same as before), with the full decode kept as the fallback. Sources that can carry alpha, such as window screenshots with transparent corners, still decode directly.Tests:
visible_rowsacross no search / search / show-more; a 500-row list queues 20 jobs, "Show more" queues rows 20-39, search keeps match order; a transparent PNG keeps its corner alpha and writes no cache file; an opaque PNG creates the cache file and a marker overwrite proves the warm path reads it.Results
Debug build (dependencies at opt-level 2), 2 vCPU aarch64 Linux box.
main@ 2a61427Medians of 5 runs. The live UI wasn't run; macOS wasn't built.
Overlap
spawn_decode_pool; this bounds what gets queued into that pool.{random}/{random:N}project name placeholder's #2431 (open) touch the same files and apply cleanly on top of this branch. feat: desktop design system, stabilization and dependency migration #2587 touchessettings_window.rsand will need a rebase whichever lands second.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.mainand took the measurements above.filtered()call site, the decode-result install path, the pool's drop semantics, the 2 s poll re-arm, thetRNShandling in thepng/imagecrates and the crop equivalence, re-rancargo check, clippy and thesettings_window::,tray::andlibrary::tests, and approved both.🤖 Generated with Claude Code