From 14d692ff0de8df03ef73eb5bc28aaf61e7662313 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Wed, 29 Jul 2026 19:02:06 +0800 Subject: [PATCH] fix(diff): stop the overlay re-walking the tree per frame, and re-probe a folded-in refresh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things the shared-probe work left on the render path. The overlay asks six whole-snapshot questions while building its element tree — oversized, totals, retained lines, budget fired, per-file cap fired, untracked count — and each accessor walked `files` on its own, `oversized` walking the hunks too. `files` is deliberately uncapped (only hunks are), so that was six walks over a list whose length is the size of the working tree, on the UI thread, on exactly the tree this module exists to keep responsive. `DiffSnapshot::stats` answers all six in one pass and the per-question accessors are gone, so nothing can drift from it. Computed rather than stored, because the snapshot is built by hand with `..Default::default()` throughout the tests and a cached count would read as zero for every one of them. Deduping probes per repository is what makes one `git diff` answer every watcher, but a probe describes the tree as it was when it *started*. A refresh triggered after that — a command finished, an agent turn ended — folded into the running probe and was answered with a snapshot already known to be stale, with nothing left to trigger another look: the overlay's own re-check is gated on `loading`, which the landing clears, and the `GitStatusCache` change that would have re-armed it has been spent. A folded-in request is now remembered and re-issued when that probe lands. It converges rather than loops, because a quiet tree never sets the flag. --- src/terminal/git_diff.rs | 114 +++++++++++++++++++++++++++------------ src/ui/app.rs | 14 +++++ src/ui/diff_overlay.rs | 96 ++++++++++++++++++++++----------- 3 files changed, 157 insertions(+), 67 deletions(-) diff --git a/src/terminal/git_diff.rs b/src/terminal/git_diff.rs index e8ad2ac8..cf079a1c 100644 --- a/src/terminal/git_diff.rs +++ b/src/terminal/git_diff.rs @@ -132,18 +132,6 @@ impl DiffSnapshot { .fold((0, 0), |(a, r), f| (a + f.added, r + f.removed)) } - /// Diff lines actually kept, summed over every file. This — not - /// [`totals`](Self::totals) — is what the overlay would have to build rows - /// for if every file were expanded, so it's what the render-side thresholds - /// compare against. Cheap: one `len()` per hunk, not per line. - pub fn retained_lines(&self) -> usize { - self.files - .iter() - .flat_map(|f| &f.hunks) - .map(|h| h.lines.len()) - .sum() - } - /// The true number of untracked paths, whether or not the retained list was /// capped. Falls back to the retained length so a snapshot built by hand /// (tests, `..Default::default()`) can't under-report — the fallback is @@ -152,10 +140,71 @@ impl DiffSnapshot { self.untracked_total.max(self.untracked.len()) } - /// Whether this diff is too big to open expanded: every file starts - /// collapsed and the overlay leads with the "too large to render - /// efficiently" summary. Not a refusal — individual files still expand by - /// click, which is the escape hatch the summary points at. + /// Every whole-snapshot number the render path needs, in one pass. + /// + /// The overlay asks six questions of a landed snapshot — is it oversized, + /// what are the totals, how many lines were retained, did the budget fire, + /// did the per-file cap fire, how many untracked — and it asks them while + /// building the element tree, so they run on the UI thread on every render. + /// `files` is deliberately *not* capped (only hunks are, by + /// [`MAX_FILES_WITH_HUNKS`]), so answering them one accessor at a time is + /// six walks over a list whose length is the size of the working tree, on + /// exactly the tree this whole module exists to keep responsive. Hence one + /// walk answering all of them, and no per-question accessors to drift from + /// it. + /// + /// Computed rather than cached in the struct on purpose: the snapshot is + /// `PartialEq` and built by hand all over the tests with + /// `..Default::default()`, and a stored count would silently read as zero + /// for every one of them. + pub fn stats(&self) -> DiffStats { + let mut added = 0u32; + let mut removed = 0u32; + let mut retained_lines = 0usize; + let mut budget_exhausted = false; + let mut per_file_truncated = false; + for file in &self.files { + added += file.added; + removed += file.removed; + retained_lines += file.hunks.iter().map(|h| h.lines.len()).sum::(); + match file.truncated { + Some(Truncation::Budget) => budget_exhausted = true, + Some(Truncation::PerFile) => per_file_truncated = true, + None => {} + } + } + let untracked_count = self.untracked_count(); + DiffStats { + totals: (added, removed), + retained_lines, + untracked_count, + oversized: self.files.len() + untracked_count > AUTO_COLLAPSE_TOTAL_FILES + || retained_lines > AUTO_COLLAPSE_TOTAL_LINES, + budget_exhausted, + per_file_truncated, + } + } +} + +/// Everything [`DiffSnapshot::stats`] answers in one walk. See that method for +/// why the render path wants them together. +#[derive(Clone, Copy, PartialEq, Eq, Debug, Default)] +pub struct DiffStats { + /// `(added, removed)` — exact, never affected by truncation. See + /// [`DiffSnapshot::totals`]. + pub totals: (u32, u32), + /// Diff lines actually kept, summed over every file. This — not + /// [`totals`](Self::totals) — is what the overlay would have to build rows + /// for if every file were expanded, so it's what the render-side thresholds + /// compare against. Counted one `len()` per hunk, not per line. + pub retained_lines: usize, + /// True untracked count, cap or no cap. See + /// [`DiffSnapshot::untracked_count`]. + pub untracked_count: usize, + /// Too big to open expanded: every file starts collapsed and the overlay + /// leads with the "too large to render efficiently" summary. Not a refusal — + /// individual files still expand by click, which is the escape hatch the + /// summary points at. /// /// Untracked paths count toward the file axis because they cost the same /// thing a collapsed file card costs — one row each, in the same @@ -164,19 +213,14 @@ impl DiffSnapshot { /// `ls-files`, not through the diff. The summary names whichever axis /// tripped, so a big untracked list never reads as a claim that the *diff* /// is big. - pub fn oversized(&self) -> bool { - self.files.len() + self.untracked_count() > AUTO_COLLAPSE_TOTAL_FILES - || self.retained_lines() > AUTO_COLLAPSE_TOTAL_LINES - } - - /// Whether the repo-wide budget dropped hunks that a smaller diff would - /// have kept — the overlay says so, so a missing body reads as a cap rather - /// than as tty7 losing the change. - pub fn budget_exhausted(&self) -> bool { - self.files - .iter() - .any(|f| f.truncated == Some(Truncation::Budget)) - } + pub oversized: bool, + /// The repo-wide budget dropped hunks that a smaller diff would have kept — + /// the overlay says so, so a missing body reads as a cap rather than as tty7 + /// losing the change. + pub budget_exhausted: bool, + /// Any file was cut at [`MAX_LINES_PER_FILE`] — the sibling axis, which the + /// oversized banner has to name separately because the two compose. + pub per_file_truncated: bool, } /// How a file changed vs `HEAD` — drives the status glyph in its header row. @@ -785,7 +829,7 @@ index 1..2 100644 ..Default::default() }; assert_eq!(snap.totals(), (90_000, 0)); - assert!(snap.budget_exhausted()); + assert!(snap.stats().budget_exhausted); } /// The file cap keeps a rename-the-world diff from allocating a `Vec` @@ -815,8 +859,8 @@ index 1..2 100644 ..Default::default() }; assert!(snap.files.iter().all(|f| f.truncated.is_none())); - assert!(!snap.oversized()); - assert!(!snap.budget_exhausted()); + assert!(!snap.stats().oversized); + assert!(!snap.stats().budget_exhausted); } /// `oversized` trips on either axis: many files, or many retained lines. @@ -826,7 +870,7 @@ index 1..2 100644 files: parse_unified(&many_files(AUTO_COLLAPSE_TOTAL_FILES + 1, 1)), ..Default::default() }; - assert!(by_files.oversized()); + assert!(by_files.stats().oversized); // Few files, but past the line threshold. let by_lines = DiffSnapshot { @@ -834,8 +878,8 @@ index 1..2 100644 ..Default::default() }; assert!(by_lines.files.len() <= AUTO_COLLAPSE_TOTAL_FILES); - assert!(by_lines.retained_lines() > AUTO_COLLAPSE_TOTAL_LINES); - assert!(by_lines.oversized()); + assert!(by_lines.stats().retained_lines > AUTO_COLLAPSE_TOTAL_LINES); + assert!(by_lines.stats().oversized); } /// Even a budget-truncated file keeps a removed line whose *content* starts diff --git a/src/ui/app.rs b/src/ui/app.rs index 0c908804..6e7e383e 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -742,6 +742,19 @@ pub struct Tty7App { /// [`Tty7App::spawn_shared_diff_probe`](crate::ui::app::Tty7App::spawn_shared_diff_probe). pub(crate) diff_probes_inflight: std::collections::HashSet<(crate::ui::host_ops::HostId, std::path::PathBuf)>, + /// Repositories whose in-flight probe was already stale when someone asked + /// again, so it has to be re-run the moment that one lands. + /// + /// Deduping by repo is what makes one `git diff` answer every watcher, but a + /// probe describes the tree at the moment it *started*. A refresh triggered + /// after that — a command finished, an agent turn ended — folds into the + /// running probe and would otherwise be answered with a snapshot already + /// known to be out of date, with nothing left to trigger another look: the + /// overlay re-checks only on a `GitStatusCache` change, and that one has + /// been spent. See + /// [`Tty7App::spawn_shared_diff_probe`](crate::ui::app::Tty7App::spawn_shared_diff_probe). + pub(crate) diff_probes_restale: + std::collections::HashSet<(crate::ui::host_ops::HostId, std::path::PathBuf)>, /// Local project file tree (left column of the body). pub(crate) file_tree: crate::ui::file_tree::FileTreeState, /// Code-editor panel (right column of the body). @@ -1216,6 +1229,7 @@ impl Tty7App { sftp_panel, right_panel: Default::default(), diff_probes_inflight: Default::default(), + diff_probes_restale: Default::default(), file_tree, editor, sidebar_width: Rc::new(Cell::new(sidebar_width)), diff --git a/src/ui/diff_overlay.rs b/src/ui/diff_overlay.rs index 8eaf9f98..70841072 100644 --- a/src/ui/diff_overlay.rs +++ b/src/ui/diff_overlay.rs @@ -44,8 +44,8 @@ use gpui_component::button::Button; use gpui_component::{ActiveTheme as _, Icon, IconName, Sizable as _, h_flex, v_flex}; use crate::terminal::git_diff::{ - self, AUTO_COLLAPSE_LINES, DiffSnapshot, FileDiff, FileStatus, LineKind, MAX_RENDERED_FILES, - Truncation, + self, AUTO_COLLAPSE_LINES, DiffSnapshot, DiffStats, FileDiff, FileStatus, LineKind, + MAX_RENDERED_FILES, Truncation, }; use crate::ui::app::Tty7App; use crate::ui::rounding; @@ -286,8 +286,17 @@ impl Tty7App { ) { let key = (host.id(), cwd.clone()); if !self.diff_probes_inflight.insert(key.clone()) { - return; // someone is already asking this exact question + // Someone is already asking this exact question — but they asked it + // *earlier*, and the answer in flight describes the tree as it was + // then. This caller only got here because something changed since, + // so folding it into that request would hand it a snapshot already + // known to be stale and leave nothing to trigger another look: the + // overlay's own re-check is gated on `loading`, which the landing + // clears. Remember to ask again instead. + self.diff_probes_restale.insert(key); + return; } + let host_for_retry = host.clone(); let probe_cwd = cwd.clone(); crate::ui::host_ops::HostOps::run( host, @@ -296,6 +305,13 @@ impl Tty7App { move |app, result, cx| { app.diff_probes_inflight.remove(&key); app.install_diff_snapshot(key.0, &cwd, result.map(Arc::new), cx); + // Re-ask for whoever was folded in above. Cleared first, so the + // fresh probe starts with a clean slate and a request that + // arrives while *it* flies marks the flag again — this converges + // rather than looping, because a quiet tree never sets it. + if app.diff_probes_restale.remove(&(key.0, cwd.clone())) { + app.spawn_shared_diff_probe(host_for_retry, cwd, cx); + } }, ); } @@ -628,14 +644,18 @@ impl Tty7App { focused: Option, cx: &mut Context, ) -> AnyElement { + // One walk for every whole-snapshot number this render needs, rather + // than one walk per question over a file list whose length is the size + // of the working tree — see `DiffSnapshot::stats`. + let stats = snap.stats(); // An oversized tree opens fully collapsed: the file rows are still the // useful part, and the bodies are what cost. A file the user opened by // name is exempt — that's an explicit request for one body, not for the // whole tree. See `DiffSnapshot::oversized`. - let oversized = focused.is_none() && snap.oversized(); + let oversized = focused.is_none() && stats.oversized; let mut list = v_flex().gap_3().p_4().w_full(); if oversized { - list = list.child(self.diff_oversized_notice(snap, cx)); + list = list.child(self.diff_oversized_notice(snap, &stats, cx)); } // Hard ceiling on cards built at all: even collapsed, one card per file // is one card per file, and the list is not virtualized. @@ -690,11 +710,16 @@ impl Tty7App { /// terminal). Deliberately *above* the list rather than instead of it — the /// file rows with their `+N −N` are the part that still reads fine at this /// size. - fn diff_oversized_notice(&self, snap: &DiffSnapshot, cx: &Context) -> AnyElement { + fn diff_oversized_notice( + &self, + snap: &DiffSnapshot, + stats: &DiffStats, + cx: &Context, + ) -> AnyElement { let text = format!( "This working tree is too large to render efficiently ({}). Every file is \ collapsed — expand individual files, or run `git diff` in the terminal.", - oversized_summary(snap), + oversized_summary(snap, stats), ); div() .w_full() @@ -1124,20 +1149,17 @@ fn file_expanded(file: &FileDiff, expanded: &HashMap, collapse_all /// the banner agrees with the `+N −N` in the header directly above it; the /// retained figure is named as rendered rows rather than joined to it by "of", /// because it is not a fraction of it. -fn oversized_summary(snap: &DiffSnapshot) -> String { +fn oversized_summary(snap: &DiffSnapshot, stats: &DiffStats) -> String { let mut parts = vec![format!( "{} changed file{}", snap.files.len(), if snap.files.len() == 1 { "" } else { "s" } )]; - let (added, removed) = snap.totals(); + let (added, removed) = stats.totals; let total_lines = (added + removed) as usize; - let loaded = snap.retained_lines(); - let budget = snap.budget_exhausted(); - let per_file = snap - .files - .iter() - .any(|f| f.truncated == Some(Truncation::PerFile)); + let loaded = stats.retained_lines; + let budget = stats.budget_exhausted; + let per_file = stats.per_file_truncated; parts.push(match (budget, per_file) { (false, false) => format!("{total_lines} diff lines"), _ => { @@ -1151,8 +1173,8 @@ fn oversized_summary(snap: &DiffSnapshot) -> String { ) } }); - if snap.untracked_count() > 0 { - parts.push(format!("{} untracked", snap.untracked_count())); + if stats.untracked_count > 0 { + parts.push(format!("{} untracked", stats.untracked_count)); } parts.join(", ") } @@ -1348,6 +1370,13 @@ mod tests { pairs.into_iter().map(|(p, v)| (p.to_string(), v)).collect() } + /// The banner text for a snapshot, deriving its stats the way the render + /// path does — so these tests exercise the same numbers the overlay shows + /// rather than a hand-assembled set. + fn banner(snap: &DiffSnapshot) -> String { + oversized_summary(snap, &snap.stats()) + } + /// The per-file threshold on its own: a small file opens, a big one doesn't, /// and an explicit choice overrides either. Unchanged behaviour — this is /// the small-working-tree case that must feel identical. @@ -1424,7 +1453,7 @@ mod tests { snap.files.iter().all(|f| f.added <= AUTO_COLLAPSE_LINES), "no single file is over the per-file threshold" ); - assert!(snap.oversized()); + assert!(snap.stats().oversized); let none = HashMap::new(); let rows_expanded: usize = snap @@ -1456,7 +1485,7 @@ mod tests { .collect(), ..Default::default() }; - assert!(!snap.oversized()); + assert!(!snap.stats().oversized); let none = HashMap::new(); assert!(snap.files.iter().all(|f| file_expanded(f, &none, false))); } @@ -1475,9 +1504,12 @@ mod tests { untracked_total: 40_000, ..Default::default() }; - assert!(snap.retained_lines() < git_diff::AUTO_COLLAPSE_TOTAL_LINES); + assert!(snap.stats().retained_lines < git_diff::AUTO_COLLAPSE_TOTAL_LINES); assert!(snap.files.len() < git_diff::AUTO_COLLAPSE_TOTAL_FILES); - assert!(snap.oversized(), "the untracked list alone must trip it"); + assert!( + snap.stats().oversized, + "the untracked list alone must trip it" + ); } /// The untracked section builds at most [`MAX_RENDERED_FILES`] rows while @@ -1556,12 +1588,12 @@ mod tests { let (added, removed) = snap.totals(); let total = (added + removed) as usize; assert!( - snap.retained_lines() > total, + snap.stats().retained_lines > total, "the context lines outweigh the changed ones — the shape that slipped through" ); - assert!(snap.budget_exhausted()); + assert!(snap.stats().budget_exhausted); - let summary = oversized_summary(&snap); + let summary = banner(&snap); assert!(summary.contains("budget"), "{summary}"); assert!( summary.contains(&format!("{total} changed lines")), @@ -1573,8 +1605,8 @@ mod tests { files, ..Default::default() }; - assert!(!whole.budget_exhausted()); - assert!(!oversized_summary(&whole).contains("budget")); + assert!(!whole.stats().budget_exhausted); + assert!(!banner(&whole).contains("budget")); } /// The sibling axis, blind in exactly the same place: one file cut at @@ -1602,15 +1634,15 @@ mod tests { let (added, removed) = snap.totals(); let total = (added + removed) as usize; assert!( - snap.retained_lines() > total, + snap.stats().retained_lines > total, "the shape the comparison reads backwards" ); assert!( - !snap.budget_exhausted(), + !snap.stats().budget_exhausted, "the budget axis is not what fired" ); - let summary = oversized_summary(&snap); + let summary = banner(&snap); assert!(summary.contains("per-file cap"), "{summary}"); assert!( summary.contains(&format!("{total} changed lines")), @@ -1622,7 +1654,7 @@ mod tests { files: files.clone(), ..Default::default() }; - assert!(!oversized_summary(&whole).contains("per-file")); + assert!(!banner(&whole).contains("per-file")); // Both kinds in one snapshot: neither clause may mask the other. let mut both = files; @@ -1635,7 +1667,7 @@ mod tests { truncated: Some(Truncation::Budget), ..context_heavy_file("dropped.rs") }); - let summary = oversized_summary(&DiffSnapshot { + let summary = banner(&DiffSnapshot { files: both, ..Default::default() }); @@ -1663,7 +1695,7 @@ mod tests { .collect(), ..Default::default() }); - let lines: usize = snap.retained_lines(); + let lines: usize = snap.stats().retained_lines; let t = Instant::now(); for _ in 0..10 {