Skip to content

perf(desktop-gpui): bound capped screenshot scans like recordings #2753

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
celeroncoder wants to merge 3 commits into
base: main
Choose a base branch
from
+190 −49
Open

perf(desktop-gpui): bound capped screenshot scans like recordings #2753

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0fdb3ff
perf(desktop-gpui): bound capped screenshot scans like recordings
celeroncoder Oct 20, 2026
34c4f1c
test(desktop-gpui): make the bounded screenshot test tolerate timesta…
celeroncoder Oct 20, 2026
1e66bc0
Merge branch 'main' into perf/gpui-bounded-screenshot-and-recovery-scans
richiemcilroy Oct 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
235 changes: 188 additions & 47 deletions apps/desktop-gpui/src/library.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,11 @@ pub struct RecentItem {
/// `media_sort_time_millis` (`lib.rs:3966-3972`): the filesystem `created()`
/// time of the path, falling back to `modified()`, then to the epoch.
fn media_sort_time_millis(path: &Path) -> f64 {
let Ok(metadata) = path.metadata() else {
return 0.0;
};
path.metadata()
.map_or(0.0, |metadata| metadata_sort_time_millis(&metadata))
}

fn metadata_sort_time_millis(metadata: &std::fs::Metadata) -> f64 {
metadata
.created()
.or_else(|_| metadata.modified())
Expand Down Expand Up @@ -675,44 +677,61 @@ pub struct ScreenshotItem {
/// `list_screenshots` (`lib.rs:4055-4091`): `*.cap` directories only, and the
/// sort key is the PNG's timestamp rather than the directory's.
pub fn list_screenshots_in(dir: &Path) -> Vec<ScreenshotItem> {
let Ok(entries) = std::fs::read_dir(dir) else {
return Vec::new();
};
screenshot_items_in(dir).collect()
}

let mut out = Vec::new();
for entry in entries.flatten() {
let bundle = entry.path();
if !bundle.is_dir() || bundle.extension().and_then(|ext| ext.to_str()) != Some("cap") {
continue;
fn screenshot_items_in(dir: &Path) -> impl Iterator<Item = ScreenshotItem> {
let mut bundles = Vec::new();
if let Ok(entries) = std::fs::read_dir(dir) {
for entry in entries.flatten() {
let bundle = entry.path();
if bundle.extension().and_then(|ext| ext.to_str()) != Some("cap") || !bundle.is_dir() {
continue;
}
if let Some((png, sort_time_millis)) = screenshot_png(&bundle) {
bundles.push((bundle, png, sort_time_millis));
}
}
let Ok(meta) = RecordingMeta::load_for_project(&bundle) else {
continue;
};
let Some(png) = std::fs::read_dir(&bundle).ok().and_then(|entries| {
entries
.flatten()
.map(|entry| entry.path())
.find(|path| path.extension().and_then(|ext| ext.to_str()) == Some("png"))
}) else {
continue;
};
}
bundles.sort_by(|(_, _, a), (_, _, b)| b.total_cmp(a));
bundles
.into_iter()
.filter_map(|(bundle, png, sort_time_millis)| {
let meta = RecordingMeta::load_for_project(&bundle).ok()?;
Some(ScreenshotItem {
path: png.clone(),
pretty_name: meta.pretty_name,
sort_time_millis,
thumbnail: Some(png),
bundle,
})
})
}

out.push(ScreenshotItem {
path: png.clone(),
pretty_name: meta.pretty_name,
sort_time_millis: media_sort_time_millis(&png),
thumbnail: Some(png),
bundle,
});
fn screenshot_png(bundle: &Path) -> Option<(PathBuf, f64)> {
let original = bundle.join("original.png");
if let Ok(metadata) = original.metadata() {
return Some((original, metadata_sort_time_millis(&metadata)));
}
out.sort_by(|a, b| b.sort_time_millis.total_cmp(&a.sort_time_millis));
out
let png = std::fs::read_dir(bundle)
.ok()?
.flatten()
.map(|entry| entry.path())
.find(|path| path.extension().and_then(|ext| ext.to_str()) == Some("png"))?;
let sort_time_millis = media_sort_time_millis(&png);
Some((png, sort_time_millis))
}

pub fn list_screenshots() -> Vec<ScreenshotItem> {
list_screenshots_in(&screenshots_dir())
}

pub fn list_screenshots_limited(limit: usize) -> Vec<ScreenshotItem> {
screenshot_items_in(&screenshots_dir())
.take(limit)
.collect()
}

/// Delete the `.cap` bundle that owns `path` (the PNG or the directory).
pub fn delete_screenshot(path: &Path) -> Result<(), String> {
let bundle = if path.is_dir() {
Expand Down Expand Up @@ -753,16 +772,20 @@ pub fn delete_screenshot(path: &Path) -> Result<(), String> {
}

fn scan_screenshots(dir: &Path, out: &mut Vec<RecentItem>) {
out.extend(list_screenshots_in(dir).into_iter().map(|item| RecentItem {
kind: MediaKind::Screenshot,
bundle: item.bundle,
pretty_name: item.pretty_name,
clip_count: 1,
sort_time_millis: item.sort_time_millis,
thumbnail_version: item.thumbnail.as_deref().and_then(source_mtime_nanos),
thumbnail: item.thumbnail,
sharing: None,
}));
out.extend(
screenshot_items_in(dir)
.take(RECENT_MEDIA_LIMIT)
.map(|item| RecentItem {
kind: MediaKind::Screenshot,
bundle: item.bundle,
pretty_name: item.pretty_name,
clip_count: 1,
sort_time_millis: item.sort_time_millis,
thumbnail_version: item.thumbnail.as_deref().and_then(source_mtime_nanos),
thumbnail: item.thumbnail,
sharing: None,
}),
);
}

fn newest_first(items: &mut [RecentItem]) {
Expand All @@ -780,13 +803,8 @@ pub fn recent_media_in(recording_dirs: &[PathBuf], screenshots_dir: &Path) -> Ve
let mut recordings = Vec::new();
scan_recordings(recording_dirs, &mut recordings);

let mut screenshots = Vec::new();
scan_screenshots(screenshots_dir, &mut screenshots);
newest_first(&mut screenshots);
screenshots.truncate(RECENT_MEDIA_LIMIT);

let mut merged = recordings;
merged.extend(screenshots);
scan_screenshots(screenshots_dir, &mut merged);
newest_first(&mut merged);
merged.truncate(RECENT_MEDIA_LIMIT);
merged
Expand Down Expand Up @@ -1736,6 +1754,129 @@ mod tests {
std::fs::remove_dir_all(root).unwrap();
}

#[test]
fn bounded_screenshots_match_the_full_listing_and_parse_lazily() {
fn eager_listing(dir: &Path) -> Vec<ScreenshotItem> {
let mut out = Vec::new();
for entry in std::fs::read_dir(dir).unwrap().flatten() {
let bundle = entry.path();
if !bundle.is_dir()
|| bundle.extension().and_then(|ext| ext.to_str()) != Some("cap")
{
continue;
}
let Ok(meta) = RecordingMeta::load_for_project(&bundle) else {
continue;
};
let Some(png) = std::fs::read_dir(&bundle)
.unwrap()
.flatten()
.find_map(|entry| {
let path = entry.path();
(path.extension().and_then(|ext| ext.to_str()) == Some("png"))
.then_some(path)
})
else {
continue;
};
out.push(ScreenshotItem {
path: png.clone(),
pretty_name: meta.pretty_name,
sort_time_millis: media_sort_time_millis(&png),
thumbnail: Some(png),
bundle,
});
}
out.sort_by(|a, b| b.sort_time_millis.total_cmp(&a.sort_time_millis));
out
}

fn write_shot(dir: &Path, name: &str) -> PathBuf {
write_bundle(
dir,
name,
&format!(r#"{{"pretty_name":"{name}","sharing":null,"fps":0}}"#),
)
}

let root = temp_dir("bounded-screenshots");
let legacy = write_shot(&root, "legacy");
std::fs::write(legacy.join("shot.png"), b"png").unwrap();
write_shot(&root, "no-png");
let not_a_bundle = root.join("not-a-bundle");
std::fs::create_dir_all(&not_a_bundle).unwrap();
std::fs::write(not_a_bundle.join("original.png"), b"png").unwrap();
std::fs::write(root.join("loose.cap"), b"").unwrap();
for i in 0..30 {
std::thread::sleep(std::time::Duration::from_millis(6));
let bundle = write_shot(&root, &format!("shot-{:02}", (i * 7) % 30));
std::fs::write(bundle.join("original.png"), b"png").unwrap();
}
std::thread::sleep(std::time::Duration::from_millis(6));
let tie_a = write_shot(&root, "tie-a");
std::fs::write(tie_a.join("original.png"), b"png").unwrap();
let tie_b = write_shot(&root, "tie-b");
std::fs::hard_link(tie_a.join("original.png"), tie_b.join("original.png")).unwrap();
std::thread::sleep(std::time::Duration::from_millis(6));
let corrupt = write_bundle(&root, "corrupt", "{ not json ");
std::fs::write(corrupt.join("original.png"), b"png").unwrap();

let full = list_screenshots_in(&root);
assert_eq!(full, eager_listing(&root));
assert_eq!(full.len(), 33);
assert!(full.iter().all(|item| item.bundle != corrupt));
let legacy_item = full.iter().find(|item| item.bundle == legacy).unwrap();
assert_eq!(legacy_item.path, legacy.join("shot.png"));

let read_dir_order: Vec<PathBuf> = std::fs::read_dir(&root)
.unwrap()
.flatten()
.map(|entry| entry.path())
.collect();
let read_dir_position = |bundle: &Path| {
read_dir_order
.iter()
.position(|path| path == bundle)
.unwrap()
};
let tie_time = |bundle: &Path| {
full.iter()
.find(|item| item.bundle == bundle)
.unwrap()
.sort_time_millis
};
assert_eq!(tie_time(&tie_a), tie_time(&tie_b));
assert!(
full.windows(2).all(|pair| {
pair[0].sort_time_millis > pair[1].sort_time_millis
|| (pair[0].sort_time_millis == pair[1].sort_time_millis
&& read_dir_position(&pair[0].bundle) < read_dir_position(&pair[1].bundle))
}),
"newest first, equal timestamps keep directory order"
);
Comment on lines +1849 to +1856

@greptile-apps greptile-apps Bot Oct 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Timestamp ties fail the test

If the temporary filesystem has timestamp precision coarser than 6 ms, consecutive PNGs can share a timestamp despite the sleeps. The strict > assertion then fails even when the listing is correct. The assertions that the hard-linked pair occupies the first two positions have the same dependency.

Use controlled sort times, or allow other ties and locate the hard-linked pair by path.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: apps/desktop-gpui/src/library.rs
Line: 1589-1593

Comment:
**Timestamp ties fail the test**

If the temporary filesystem has timestamp precision coarser than 6 ms, consecutive PNGs can share a timestamp despite the sleeps. The strict `>` assertion then fails even when the listing is correct. The assertions that the hard-linked pair occupies the first two positions have the same dependency.

Use controlled sort times, or allow other ties and locate the hard-linked pair by path.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

@celeroncoder celeroncoder Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 34c4f1c. set_modified is not an option here because the sort key prefers created(), so the test now tolerates ties instead: the hard-linked pair is located by bundle path and must share a timestamp, and every adjacent pair in the listing must be strictly newer-to-older or, on equal timestamps, keep read_dir order (the stable-sort property the production code relies on). The assertions that assumed the sleeps produce distinct times (legacy last, the pair at positions 0-1, the strict > chain) are gone; the equivalence with the old listing, the take(K) prefix loop and the laziness check are unchanged. The pair is deliberately not asserted adjacent, since on a coarse filesystem a third bundle could share the timestamp and sit between them in directory order.

Fix written by Claude Opus 5.5, reviewed by DeepSeek V4.1 Flash and Gemini 3.8 Flash.


for limit in [0, 1, 9, 20, 100] {
assert_eq!(
screenshot_items_in(&root).take(limit).collect::<Vec<_>>(),
full[..limit.min(full.len())],
"limit {limit}"
);
}

let mut lazy = screenshot_items_in(&root);
let head: Vec<_> = lazy.by_ref().take(RECENT_MEDIA_LIMIT).collect();
assert_eq!(head, full[..RECENT_MEDIA_LIMIT]);
for item in &full[RECENT_MEDIA_LIMIT..] {
std::fs::remove_file(item.bundle.join("recording-meta.json")).unwrap();
}
assert!(
lazy.next().is_none(),
"metas past the consumed prefix are read on demand"
);

std::fs::remove_dir_all(&root).unwrap();
}

#[test]
#[ignore = "manual filesystem benchmark; set CAP_GPUI_LIBRARY_BENCH_DIR and CAP_GPUI_LIBRARY_BENCH_MODE"]
fn recording_scan_benchmark() {
Expand Down
4 changes: 2 additions & 2 deletions apps/desktop-gpui/src/main_window.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4204,8 +4204,8 @@ impl MainWindow {
(Some(items), None)
}
LibraryKind::Screenshots => {
let mut items = library::list_screenshots();
items.truncate(library::LIBRARY_PANEL_LIMIT);
let items =
library::list_screenshots_limited(library::LIBRARY_PANEL_LIMIT);
(None, Some(items))
}
}
Expand Down
Loading