mirror of
https://github.com/l0ng-ai/tty7.git
synced 2026-09-22 00:02:23 +00:00
fix(diff): stop the overlay re-walking the tree per frame, and re-probe a folded-in refresh
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.
This commit is contained in:
+79
-35
@@ -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::<usize>();
|
||||
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<Hunk>`
|
||||
@@ -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
|
||||
|
||||
@@ -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)),
|
||||
|
||||
+64
-32
@@ -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<usize>,
|
||||
cx: &mut Context<Self>,
|
||||
) -> 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<Self>) -> AnyElement {
|
||||
fn diff_oversized_notice(
|
||||
&self,
|
||||
snap: &DiffSnapshot,
|
||||
stats: &DiffStats,
|
||||
cx: &Context<Self>,
|
||||
) -> 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<String, bool>, 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 {
|
||||
|
||||
Reference in New Issue
Block a user