From 3fd523e1b9988cd640ec82140bcc08fa9a4aeecd Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:43:25 +0800 Subject: [PATCH] feat(diff): make the sidebar diff preview optional and bound its cost MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clicking a sidebar row's `+N −N` opens the working-tree diff overlay. On a big tree that could stall the window, and not everyone wants an in-app diff viewer in the first place. Two halves, matching the report. The setting: `sidebar_diff_preview` (Settings → Window & Tabs, on by default, persisted in `config.json`). Off, the branch and the counts stay exactly where they are and read exactly the same; they lose only the pointer cursor and the `toggle_diff_overlay` handler, so the press falls through to ordinary tab activation. Both come off one value — `diff_click_cwd` — so they cannot get out of step. The performance work. All five of the reporter's hypotheses held up against v26.7.6, and each fix is measured on a 300-file / 90 000-line / 4.5 MB diff (release, macOS arm64): 1. The full diff was buffered before parsing — `git_status::git` uses `Command::output()`. Now streamed line by line through the new `git_status::git_lines` into an incremental `DiffParser`: peak transient buffer 4 552 060 bytes → 50 bytes, at ~1.7× the parse CPU (3.97 ms → 6.76 ms) on the background thread, where it never touches a frame. 2. The snapshot was deep-cloned per holder inside `this.update`, i.e. on the UI thread. Now shared behind `Arc`: 2.41 ms → 11 ns per holder. 3. The element tree is not virtualized — confirmed, not cured. Rendering is not being redesigned here; instead the element count is bounded (see 4) and `MAX_RENDERED_FILES` caps the cards built at all, with a "… and N more" line for the tail. 4. Auto-collapse was per file, and counted only +/− while the rendered body also has context lines. Added `AUTO_COLLAPSE_TOTAL_LINES` over *retained* lines: sixty forty-line files, none individually large, went from 2400 side-by-side rows to zero, under a summary saying the diff is too large to render efficiently and pointing at expanding individual files or `git diff`. 5. The Changes panel probed independently and kept its own snapshot. Both now go through `spawn_shared_diff_probe`, which dedupes by cwd and installs one `Arc` into every watcher; opening the overlay while the panel already shows that repo now paints from the panel's snapshot instead of re-probing. Plus a repo-wide retention budget (`MAX_TOTAL_LINES`, `MAX_FILES_WITH_HUNKS`): 90 000 lines / 6.2 MiB of line text → 20 000 / 1.2 MiB. The `+N −N` totals deliberately escape every cap — they are compared against `--numstat` to detect staleness, so a capped total would disagree forever and re-probe in a loop. Small diffs are untouched: a forty-file, twelve-lines-each tree is not oversized and still opens expanded, asserted directly. Not verified: anything requiring the GUI. No frame timings, no visual check of the oversized banner or the settings row, and `AUTO_COLLAPSE_TOTAL_LINES` is a judgement call anchored on row count rather than a measured frame budget. Refs #239. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 41 +++ crates/tty7-core/src/core/config.rs | 33 ++ docs/features.md | 2 +- docs/features.zh-CN.md | 2 +- src/terminal/git_diff.rs | 470 +++++++++++++++++++++++++--- src/ui/app.rs | 17 + src/ui/diff_overlay.rs | 399 ++++++++++++++++++++--- src/ui/right_panel.rs | 53 ++-- src/ui/settings.rs | 20 ++ src/ui/tab_sidebar.rs | 76 ++++- 10 files changed, 984 insertions(+), 129 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59039984..9a0ae1bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 session" means: paste it into `codex resume`, a bug report, or another tool. (#211) +- **Sidebar diff preview is optional** — clicking a sidebar row's `+N −N` + working-tree counts opens the diff overlay, which is the point of them for + most people but not for everyone. Settings → Window & Tabs → *Open diff + preview from sidebar counts* turns the click off (`sidebar_diff_preview` in + `config.json`, on by default). Off, the branch and the counts stay exactly + where they are and read exactly the same; they simply stop being their own + click target, so the press falls through to ordinary tab activation like any + other part of the row. (#239) + ### Changed - **The prompt editor's soft newline is now a rebindable action** — `Shift+Enter` @@ -97,6 +106,38 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 the 80px grab handle, so chips reach their minimum width and truncate a tab or two sooner. (#221) +- **Large working-tree diffs no longer stall the window** — the diff overlay + had four costs that all scaled with the size of the tree rather than with + what it could show, and issue #239's source-level analysis found each of + them. Measured on a 300-file, 90 000-line, 4.5 MB working-tree diff: + + - The full `git diff HEAD` was read into one `String` before parsing began. + It is now streamed off the pipe a line at a time, so peak transient memory + is the longest line — 4 552 060 bytes resident → 50 bytes — at a cost of + ~1.7× the parse CPU (3.97 ms → 6.76 ms) on the background thread, where it + never touches a frame. + - The parsed snapshot was deep-cloned onto every tab's overlay *on the UI + update path*. It is now shared behind an `Arc`: 2.41 ms of main-thread + copying per holder → 11 ns. + - The per-file 2000-line cap bounded one pathological file but not the sum, + so two hundred ordinary files could retain 90 000 `DiffLine`s. A repo-wide + budget now caps retained lines and files-with-hunks — 90 000 lines / 6.2 MiB + of line text → 20 000 lines / 1.2 MiB — while `+N −N` keeps counting the + whole diff, so the numbers stay exact and the overlay doesn't re-probe in a + loop chasing a total it can no longer reach. + - The 400-line auto-collapse rule was per-file, so sixty forty-line files all + opened at once (2400 side-by-side rows, none of them individually large). + Past a repo-wide total the overlay now opens with every file collapsed — + zero rows built — leads with a summary saying the diff is too large to + render efficiently, and points at expanding individual files or `git diff` + in the terminal. + +- **One `git diff` per repository, not two** — the Changes panel and the diff + overlay each ran their own full-diff probe and kept their own snapshot of the + same repository. They now share one probe and one snapshot, so opening both + costs one shell-out and one parse, and opening the overlay while the panel is + already showing that repo paints immediately instead of re-probing. (#239) + ### Fixed - **Rounded UI controls no longer square off their corners** diff --git a/crates/tty7-core/src/core/config.rs b/crates/tty7-core/src/core/config.rs index abcd940c..cbe4e8fd 100644 --- a/crates/tty7-core/src/core/config.rs +++ b/crates/tty7-core/src/core/config.rs @@ -285,6 +285,16 @@ pub struct Config { /// (`repo`, the default), or one flat list (`none`). #[serde(default, deserialize_with = "de_lenient")] pub sidebar_grouping: SidebarGrouping, + /// Whether clicking a sidebar row's `+N −N` working-tree counts opens the + /// diff overlay. Off leaves the branch and the counts exactly as they are — + /// they are a readout worth having on their own — and only takes away the + /// click target and its pointer cursor, so the press falls through to + /// ordinary tab activation. On by default: the overlay is the reason the + /// counts are there for most people, and the large-diff cost it used to + /// carry is now bounded by the diff parser's budgets. This is the escape + /// hatch for anyone who wants the numbers without the viewer. + #[serde(default = "default_true")] + pub sidebar_diff_preview: bool, /// When to post a desktop notification after a long foreground command /// finishes. #[serde(default, deserialize_with = "de_lenient")] @@ -751,6 +761,8 @@ impl Default for Config { right_panel_width: default_right_panel_width(), right_panel_tab: RightPanelTab::Info, sidebar_grouping: SidebarGrouping::Repo, + // Today's behaviour, unchanged: the counts open the overlay. + sidebar_diff_preview: true, notify_on_command_finish: NotifyMode::Unfocused, // Opt-out, not opt-in: a stale terminal that never tells you it's // outdated is the status quo we're fixing. One cheap GET at startup. @@ -1196,6 +1208,27 @@ mod tests { assert_eq!(back.ssh_profile_frecency.get(&id).unwrap().count, 4); } + /// The sidebar diff preview is opt-*out*: a config written before the switch + /// existed keeps today's clickable counts, and turning it off survives a + /// write/read cycle of `config.json` (issue #239). + #[test] + fn sidebar_diff_preview_defaults_on_and_round_trips() { + assert!(Config::default().sidebar_diff_preview); + + let old: Config = serde_json::from_str(r#"{"font_size": 15.0}"#).unwrap(); + assert!( + old.sidebar_diff_preview, + "absent key means today's behaviour" + ); + + let off: Config = serde_json::from_str(r#"{"sidebar_diff_preview": false}"#).unwrap(); + assert!(!off.sidebar_diff_preview); + let json = serde_json::to_string(&off).unwrap(); + assert!(json.contains("\"sidebar_diff_preview\":false"), "persisted"); + let back: Config = serde_json::from_str(&json).unwrap(); + assert!(!back.sidebar_diff_preview); + } + /// Opt-*out*, unlike most flags here: a config written before this setting /// existed must keep the prompt, or an update would silently take away the /// one thing telling people their sessions survive a quit. diff --git a/docs/features.md b/docs/features.md index 10d35378..278bf194 100644 --- a/docs/features.md +++ b/docs/features.md @@ -58,7 +58,7 @@ it never wraps or replaces the agent. - **Brand avatars** — the tab chip / sidebar row shows which agent runs where; custom wrappers map in via `agent_commands` in `config.json` - **Status dot** — working (blue) / needs your input (amber) / done (green), driven by agent-reported events over an OSC channel; Settings → Agents installs the hooks that feed it (Claude Code, Codex, Copilot CLI, OpenCode, Pi, Grok Build) - **Notifications** — "needs your permission…" the moment an agent blocks on you, and "finished after Ns" per turn, honoring your notification policy -- **Branch at a glance** — each sidebar row shows its pane's git branch and working-tree diff (`+N −M`), refreshed on `cd` and when a command finishes +- **Branch at a glance** — each sidebar row shows its pane's git branch and working-tree diff (`+N −M`), refreshed on `cd` and when a command finishes; clicking the counts opens the diff overlay, and turning that off (Settings → Window & Tabs, or `sidebar_diff_preview: false` in `config.json`) keeps the readout while making it non-clickable - **Session resume** — panes lost to a reboot re-launch their agent conversation on restore, carrying the original launch flags (`claude --dangerously-skip-permissions --resume …`) (`restore_agent_sessions`, on by default) - **Fork session** — branch a live agent conversation into a second, independent one by shelling the agent's own fork command (`codex fork `, `claude --resume --fork-session`, also OpenCode and Grok Build); the original is untouched and both continue separately. Right-click a pane to pick a split placement, or right-click the tab / sidebar row to open the fork in a new tab. Needs the agent's hooks installed, since the fork targets the session id they report; a remote pane can't fork, because the command would run against the local agent — and note a fork copies the whole transcript, so repeated forking costs real disk in the agent's own session store - **Copy Session ID** — put the agent's native session id on the clipboard, beside *Copy Working Directory*, for pasting into `codex resume`, a bug report, or another tool diff --git a/docs/features.zh-CN.md b/docs/features.zh-CN.md index 988bd1b1..4669c04a 100644 --- a/docs/features.zh-CN.md +++ b/docs/features.zh-CN.md @@ -55,7 +55,7 @@ Aider、Amp、OpenCode 等约 17 个）并在其外围加功能 —— 绝不包 - **品牌头像** —— 标签 chip / 侧栏行显示每个 pane 跑的是哪个 agent；自定义包装命令可通过 `config.json` 的 `agent_commands` 映射 - **状态点** —— 工作中（蓝）/ 等你输入（琥珀）/ 完成（绿），由 agent 自己上报的 OSC 事件驱动；在 设置 → Agents 一键装好对应 hooks（Claude Code、Codex、Copilot CLI、OpenCode、Pi、Grok Build） - **通知** —— agent 卡在等你批准的那一刻弹 "needs your permission…"，每轮结束弹 "finished after Ns"，遵循你的通知策略 -- **一眼看分支** —— 侧栏每行显示该 pane 的 git 分支和工作区改动（`+N −M`），`cd` 或命令跑完时自动刷新 +- **一眼看分支** —— 侧栏每行显示该 pane 的 git 分支和工作区改动（`+N −M`），`cd` 或命令跑完时自动刷新；点改动数字会打开 diff 浮层，关掉它（设置 → 窗口与标签，或 `config.json` 的 `sidebar_diff_preview: false`）分支和数字照常显示，只是不再可点 - **会话恢复** —— 重启后无法重连的 pane 会自动续上 agent 对话，并带上原始启动 flags（`claude --dangerously-skip-permissions --resume …`；`restore_agent_sessions`，默认开启） - **Fork 会话** —— 直接调 agent 自己的 fork 命令（`codex fork `、`claude --resume --fork-session`，OpenCode 和 Grok Build 同样支持），把当前对话分叉成一个独立会话；原会话原封不动，两边各自往下走。在 pane 上右键可选择分屏位置，在标签 / 侧栏行上右键则直接开新标签。需要先装好该 agent 的 hooks（fork 认的是 hooks 上报的 session id）；远程 pane 不能 fork，因为命令会跑在本机的 agent 上；另外 fork 会整份复制对话历史，反复 fork 会在 agent 自己的会话目录里占掉不少磁盘 - **复制 Session ID** —— 把 agent 的原生 session id 复制到剪贴板，就在 *Copy Working Directory* 旁边，方便粘进 `codex resume`、bug 报告或别的工具 diff --git a/src/terminal/git_diff.rs b/src/terminal/git_diff.rs index 52de682c..a82677f7 100644 --- a/src/terminal/git_diff.rs +++ b/src/terminal/git_diff.rs @@ -10,6 +10,14 @@ //! previous snapshot (or a loading state) until a probe lands. Asking the pane's //! host rather than this machine is also what makes the overlay work at all for //! a pane whose repository lives somewhere else. +//! +//! And never trusted to be *small*, either. `git diff HEAD` is the one git read +//! in this app whose output scales with the working tree rather than with what +//! the UI can show, so the parser retains at most [`MAX_LINES_PER_FILE`] per +//! file and [`MAX_TOTAL_LINES`] / [`MAX_FILES_WITH_HUNKS`] across the +//! repository. The `+`/`−` counts deliberately escape all of it: they are +//! compared against `git diff --numstat` to decide whether the overlay is +//! stale, so a capped total would disagree forever and re-probe in a loop. use std::path::{Path, PathBuf}; @@ -22,10 +30,54 @@ use crate::ui::host_ops::Host; /// tree. Generous enough that real hand-written changes never hit it. pub const MAX_LINES_PER_FILE: usize = 2000; +/// Repo-wide cap on retained diff lines. [`MAX_LINES_PER_FILE`] bounds one +/// pathological file; this bounds the *sum*, which is the shape a working tree +/// full of agent edits actually takes — two hundred files of three hundred +/// lines each never trip the per-file cap yet retain 60k `DiffLine`s, each an +/// owned `String`. Past this the parser keeps counting `+`/`−` (the header +/// numbers must stay honest — see the note in [`parse_unified`]) but stops +/// retaining line text. +pub const MAX_TOTAL_LINES: usize = 20_000; + +/// Repo-wide cap on how many files keep their hunks. A branch that renames a +/// vendored tree can list thousands of files whose diffs are each tiny; every +/// one of them still costs a `Vec`. Files past this keep their header row +/// (path, status, counts) and lose only the body. +pub const MAX_FILES_WITH_HUNKS: usize = 500; + /// A file's added+removed size at which the overlay collapses it by default /// (GitHub's "Load diff" treatment) — the user can still expand it by click. pub const AUTO_COLLAPSE_LINES: u32 = 400; +/// Repo-wide counterpart to [`AUTO_COLLAPSE_LINES`]: once the snapshot's +/// *retained* lines exceed this, every file starts collapsed and the overlay +/// leads with the oversized-diff summary. Two differences from the per-file +/// threshold matter here — this counts context lines too (they are rendered, +/// so they are what costs), and it is a sum, so many medium files add up the +/// way one big file does. +pub const AUTO_COLLAPSE_TOTAL_LINES: usize = 2_000; + +/// The same idea by file count. Set well clear of a busy-but-ordinary tree — +/// forty changed files of a few lines each is a normal afternoon and must still +/// open expanded — because rows, not cards, are what actually cost: this axis +/// only catches the tree so wide that a card per file is itself the problem. +pub const AUTO_COLLAPSE_TOTAL_FILES: usize = 100; + +/// Hard ceiling on file cards the overlay builds at all. Past this the list is +/// cut and a "… and N more" line stands in for the tail, so the element tree +/// stays bounded no matter what the working tree looks like. +pub const MAX_RENDERED_FILES: usize = 300; + +/// Why a file's hunks stop short of its real diff. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Truncation { + /// This one file exceeded [`MAX_LINES_PER_FILE`]. + PerFile, + /// The repo-wide budget ([`MAX_TOTAL_LINES`] / [`MAX_FILES_WITH_HUNKS`]) + /// ran out — the file itself may be small. + Budget, +} + /// One parsed `git diff HEAD` for a repo, plus the untracked files `diff` /// itself can't see. This is the overlay's whole model. #[derive(Clone, PartialEq, Eq, Debug, Default)] @@ -47,11 +99,47 @@ impl DiffSnapshot { /// Total added/removed line counts across all files — the overlay's /// header numbers, matching the sidebar's `+N −N` by construction (both /// sum per-file counts of the same `HEAD` diff). + /// + /// Deliberately *not* affected by any truncation: the parser keeps counting + /// past every cap, because this number is compared against the status + /// cache's `git diff --numstat` totals to decide whether the overlay is + /// stale. A capped total would never match, and the overlay would re-probe + /// in a loop. pub fn totals(&self) -> (u32, u32) { self.files .iter() .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() + } + + /// 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. + pub fn oversized(&self) -> bool { + self.files.len() > 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)) + } } /// How a file changed vs `HEAD` — drives the status glyph in its header row. @@ -76,9 +164,10 @@ pub struct FileDiff { pub removed: u32, /// Binary file — no hunks, the header row says "binary" instead. pub binary: bool, - /// Hunk parsing stopped at [`MAX_LINES_PER_FILE`]; the overlay appends a - /// "truncated" footer under the last hunk. - pub truncated: bool, + /// Hunk parsing stopped short of the file's real diff, and why; the overlay + /// appends a "truncated" footer under the last hunk. `None` means the body + /// is complete. + pub truncated: Option, pub hunks: Vec, } @@ -152,117 +241,174 @@ pub fn probe(host: &dyn Host, cwd: &Path) -> Option { /// construction: unrecognized metadata lines between the `diff --git` header /// and the first hunk (modes, index, similarity) are simply skipped, so a git /// version printing extra headers degrades to "fewer facts", never a panic. +/// +/// The whole-string form the tests drive the parser through; [`probe`] streams +/// the same [`DiffParser`] line by line off git's stdout instead, so no caller +/// in the app ever holds the full diff as one `String`. +#[cfg(test)] pub fn parse_unified(out: &str) -> Vec { - let mut files: Vec = Vec::new(); - // Line-number counters for the hunk currently being filled. - let (mut old_no, mut new_no) = (0u32, 0u32); - // Lines consumed by the current file's hunks, for the per-file cap. - let mut file_lines = 0usize; - + let mut parser = DiffParser::default(); for line in out.lines() { + parser.push_line(line); + } + parser.finish() +} + +/// The unified-diff parser as an incremental state machine, so `git diff`'s +/// output can be consumed a line at a time off a pipe rather than buffered +/// whole. Enforces both the per-file and the repo-wide retention budgets; see +/// [`MAX_LINES_PER_FILE`] and [`MAX_TOTAL_LINES`]. +#[derive(Default)] +pub struct DiffParser { + files: Vec, + /// Line-number counters for the hunk currently being filled. + old_no: u32, + new_no: u32, + /// Lines consumed by the current file's hunks, for the per-file cap. + file_lines: usize, + /// Lines retained across every file so far, for the repo-wide cap. + total_lines: usize, + /// Files that actually kept at least one hunk, for the repo-wide file cap. + /// Counted at the first `@@`, not at the file header: a pure rename or a + /// binary blob has no body and must not spend the budget on nothing. + files_with_hunks: usize, + /// Whether the last `@@` header opened a body we're still inside. Tracked + /// explicitly rather than inferred from "the file has hunks": a file the + /// budget truncated never gets a `Hunk` pushed, but its lines still have to + /// be *counted*, and a removed line whose own text starts with `--- ` must + /// not be mistaken for the file header it looks like. + in_hunk: bool, +} + +impl DiffParser { + /// Feed one line of `git diff` output (no trailing newline). + pub fn push_line(&mut self, line: &str) { if let Some(rest) = line.strip_prefix("diff --git ") { let (old_p, new_p) = parse_git_header_paths(rest); - files.push(FileDiff { + self.files.push(FileDiff { path: new_p.clone(), old_path: (old_p != new_p).then_some(old_p), status: FileStatus::Modified, added: 0, removed: 0, binary: false, - truncated: false, + truncated: None, hunks: Vec::new(), }); - file_lines = 0; - continue; + self.file_lines = 0; + self.in_hunk = false; + return; } - let Some(file) = files.last_mut() else { - continue; // preamble before any header (shouldn't happen) + let Some(file) = self.files.last_mut() else { + return; // preamble before any header (shouldn't happen) }; // ── File-level metadata between the header and the first hunk ────── if line.starts_with("new file mode") { file.status = FileStatus::Added; - continue; + return; } if line.starts_with("deleted file mode") { file.status = FileStatus::Deleted; - continue; + return; } if line.starts_with("rename from ") { file.status = FileStatus::Renamed; - continue; + return; } if line.starts_with("Binary files ") || line.starts_with("GIT binary patch") { file.binary = true; - continue; + return; } // `--- a/x` / `+++ b/x` repeat what the header said; `rename to`, // `index`, modes and similarity scores add nothing we render. But only // skip them *outside* hunk bodies — a removed line legitimately starts // with `--- ` inside one. - if file.hunks.is_empty() + if !self.in_hunk && (line.starts_with("--- ") || line.starts_with("+++ ") || !is_hunk_line(line)) && !line.starts_with("@@") { - continue; + return; } // ── Hunks ─────────────────────────────────────────────────────────── if line.starts_with("@@") { - if file.truncated { - continue; // past the cap: swallow the rest of this file + // A truncated file still enters the body — its lines have to be + // counted — it just doesn't get a `Hunk` to keep them in. + self.in_hunk = true; + if file.truncated.is_some() { + return; + } + // The repo-wide budget is charged here rather than at the file + // header, so a file with no body at all (a pure rename, a binary + // blob) neither consumes the file budget nor gets flagged as + // truncated for a body it never had. + let first_hunk = file.hunks.is_empty(); + if (first_hunk && self.files_with_hunks >= MAX_FILES_WITH_HUNKS) + || self.total_lines >= MAX_TOTAL_LINES + { + file.truncated = Some(Truncation::Budget); + return; + } + if first_hunk { + self.files_with_hunks += 1; } let (o, n) = parse_hunk_starts(line).unwrap_or((0, 0)); - old_no = o; - new_no = n; + self.old_no = o; + self.new_no = n; file.hunks.push(Hunk { header: line.to_string(), lines: Vec::new(), }); - continue; + return; } - if file.hunks.is_empty() { - continue; // stray content outside any hunk + if !self.in_hunk { + return; // stray content outside any hunk } let (kind, text) = match line.as_bytes().first() { Some(b'+') => (LineKind::Added, &line[1..]), Some(b'-') => (LineKind::Removed, &line[1..]), Some(b' ') => (LineKind::Context, &line[1..]), // `\ No newline at end of file` and anything else: not a diff line. - _ => continue, + _ => return, }; - // Count added/removed *before* the truncation gate: the cap is about - // element volume, but the header numbers must stay honest, so lines - // past the cap still count even though they're never kept. + // Count added/removed *before* the truncation gate: the caps are about + // element volume, but the header numbers must stay honest (they are + // compared against `--numstat` to detect staleness), so lines past a cap + // still count even though they're never kept. match kind { LineKind::Added => file.added += 1, LineKind::Removed => file.removed += 1, LineKind::Context => {} } - if file.truncated { - continue; + if file.truncated.is_some() { + return; } - file_lines += 1; - if file_lines > MAX_LINES_PER_FILE { - file.truncated = true; - continue; + self.file_lines += 1; + if self.file_lines > MAX_LINES_PER_FILE { + file.truncated = Some(Truncation::PerFile); + return; + } + if self.total_lines >= MAX_TOTAL_LINES { + file.truncated = Some(Truncation::Budget); + return; } let Some(hunk) = file.hunks.last_mut() else { - continue; + return; }; let (o, n) = match kind { LineKind::Added => { - let n = new_no; - new_no += 1; + let n = self.new_no; + self.new_no += 1; (None, Some(n)) } LineKind::Removed => { - let o = old_no; - old_no += 1; + let o = self.old_no; + self.old_no += 1; (Some(o), None) } LineKind::Context => { - let (o, n) = (old_no, new_no); - old_no += 1; - new_no += 1; + let (o, n) = (self.old_no, self.new_no); + self.old_no += 1; + self.new_no += 1; (Some(o), Some(n)) } }; @@ -272,10 +418,15 @@ pub fn parse_unified(out: &str) -> Vec { new_no: n, text: text.to_string(), }); + self.total_lines += 1; + } + + /// The parsed files. A truncated file still counts +/− for its whole diff + /// (the parser keeps counting past every cap), so totals stay consistent + /// with `--numstat`. + pub fn finish(self) -> Vec { + self.files } - // A truncated file still counts +/− for its whole diff (the loop above - // keeps counting past the cap), so totals stay consistent with numstat. - files } /// Whether a line can only belong to a hunk body (`+`/`-`/space/`\` lead). @@ -486,7 +637,7 @@ index 1111111..2222222 100644 out.push_str(&format!("+line {i}\n")); } let files = parse_unified(&out); - assert!(files[0].truncated); + assert_eq!(files[0].truncated, Some(Truncation::PerFile)); assert_eq!(files[0].added, 3000); let kept: usize = files[0].hunks.iter().map(|h| h.lines.len()).sum(); assert_eq!(kept, MAX_LINES_PER_FILE); @@ -520,4 +671,223 @@ index 1..2 100644 }; assert_eq!(snap.totals(), (4, 2)); } + + /// Build `files` synthetic modified files of `lines_each` added lines — + /// the "many medium files" shape the per-file cap alone can't bound. + fn many_files(files: usize, lines_each: usize) -> String { + let mut out = String::new(); + for f in 0..files { + out.push_str(&format!( + "diff --git a/f{f}.rs b/f{f}.rs\nindex 1..2 100644\n--- a/f{f}.rs\n+++ b/f{f}.rs\n@@ -0,0 +1,{lines_each} @@\n" + )); + for i in 0..lines_each { + out.push_str(&format!("+file {f} line {i}\n")); + } + } + out + } + + /// The repo-wide budget bounds retained lines even when no single file is + /// anywhere near [`MAX_LINES_PER_FILE`] — the case the reporter of #239 + /// called out as the one the per-file cap misses. + #[test] + fn repo_wide_budget_caps_retained_lines() { + // 300 files × 300 lines = 90k lines, none of which trips the 2000-line + // per-file cap. + let files = parse_unified(&many_files(300, 300)); + assert_eq!(files.len(), 300, "every file keeps its header row"); + let retained: usize = files + .iter() + .flat_map(|f| &f.hunks) + .map(|h| h.lines.len()) + .sum(); + assert!( + retained <= MAX_TOTAL_LINES, + "retained {retained} lines, budget is {MAX_TOTAL_LINES}" + ); + assert!( + files + .iter() + .any(|f| f.truncated == Some(Truncation::Budget)) + ); + } + + /// Budget or no budget, the +/− totals must stay exact: they are compared + /// against `git diff --numstat` to decide whether the overlay is stale, and + /// a short count would make every comparison disagree and re-probe forever. + #[test] + fn repo_wide_budget_keeps_totals_exact() { + let snap = DiffSnapshot { + files: parse_unified(&many_files(300, 300)), + ..Default::default() + }; + assert_eq!(snap.totals(), (90_000, 0)); + assert!(snap.budget_exhausted()); + } + + /// The file cap keeps a rename-the-world diff from allocating a `Vec` + /// per file, while every file still lists its path and counts. + #[test] + fn repo_wide_budget_caps_files_with_hunks() { + // One line each: far under the line budget, so only the file cap can + // stop this. + let files = parse_unified(&many_files(MAX_FILES_WITH_HUNKS + 50, 1)); + assert_eq!(files.len(), MAX_FILES_WITH_HUNKS + 50); + let with_hunks = files.iter().filter(|f| !f.hunks.is_empty()).count(); + assert_eq!(with_hunks, MAX_FILES_WITH_HUNKS); + // The tail still counts, so totals stay honest. + assert_eq!( + files.iter().map(|f| f.added).sum::(), + (MAX_FILES_WITH_HUNKS + 50) as u32 + ); + assert_eq!(files.last().unwrap().truncated, Some(Truncation::Budget)); + } + + /// A small diff is untouched by the budget — the setting-enabled, + /// small-working-tree case must behave exactly as before. + #[test] + fn small_diff_is_not_truncated() { + let snap = DiffSnapshot { + files: parse_unified(SAMPLE), + ..Default::default() + }; + assert!(snap.files.iter().all(|f| f.truncated.is_none())); + assert!(!snap.oversized()); + assert!(!snap.budget_exhausted()); + } + + /// `oversized` trips on either axis: many files, or many retained lines. + #[test] + fn oversized_trips_on_files_or_lines() { + let by_files = DiffSnapshot { + files: parse_unified(&many_files(AUTO_COLLAPSE_TOTAL_FILES + 1, 1)), + ..Default::default() + }; + assert!(by_files.oversized()); + + // Few files, but past the line threshold. + let by_lines = DiffSnapshot { + files: parse_unified(&many_files(2, AUTO_COLLAPSE_TOTAL_LINES)), + ..Default::default() + }; + assert!(by_lines.files.len() <= AUTO_COLLAPSE_TOTAL_FILES); + assert!(by_lines.retained_lines() > AUTO_COLLAPSE_TOTAL_LINES); + assert!(by_lines.oversized()); + } + + /// Even a budget-truncated file keeps a removed line whose *content* starts + /// with `--- ` out of the metadata skip — that line still has to count. + #[test] + fn truncated_file_counts_dash_prefixed_content() { + let mut out = many_files(MAX_FILES_WITH_HUNKS, 1); + out.push_str( + "diff --git a/late.md b/late.md\nindex 1..2 100644\n--- a/late.md\n+++ b/late.md\n@@ -1,2 +1,1 @@\n keep\n--- a heading rule\n", + ); + let files = parse_unified(&out); + let late = files.last().unwrap(); + assert_eq!(late.path, "late.md"); + assert_eq!(late.truncated, Some(Truncation::Budget)); + assert!(late.hunks.is_empty(), "no body kept past the file cap"); + assert_eq!((late.added, late.removed), (0, 1), "but the line counts"); + } + + /// Measurement harness for issue #239 finding 1 — run with + /// `cargo test --release -- --ignored --nocapture bench_stream_vs_buffer`. + /// + /// Stands in for `Command::output()` vs [`git_status::git_lines`] with a + /// file on disk, so the two paths differ only in whether the whole diff is + /// materialised as one `String` before parsing starts. + #[test] + #[ignore = "measurement, not an assertion"] + fn bench_stream_vs_buffer() { + use std::io::{BufRead as _, BufReader, Write as _}; + use std::time::Instant; + + let path = std::env::temp_dir().join("tty7-diff-bench.patch"); + { + let mut f = std::io::BufWriter::new(std::fs::File::create(&path).unwrap()); + // ~90k changed lines over 300 files — a big but not absurd agent + // session's working tree. + for file in 0..300 { + write!( + f, + "diff --git a/f{file}.rs b/f{file}.rs\nindex 1..2 100644\n--- a/f{file}.rs\n+++ b/f{file}.rs\n@@ -0,0 +1,300 @@\n" + ) + .unwrap(); + for i in 0..300 { + writeln!(f, "+file {file} line {i} of a fairly typical source line").unwrap(); + } + } + } + let size = std::fs::metadata(&path).unwrap().len(); + + let t = Instant::now(); + let buffered = std::fs::read_to_string(&path).unwrap(); + let read = t.elapsed(); + let t = Instant::now(); + let a = parse_unified(&buffered); + println!( + "buffered: read {size} bytes in {read:?} (all resident), parse {:?}, {} files", + t.elapsed(), + a.len() + ); + drop(buffered); + + let t = Instant::now(); + let mut parser = DiffParser::default(); + // Same capacity `git_lines` uses. + let mut reader = BufReader::with_capacity(64 * 1024, std::fs::File::open(&path).unwrap()); + let mut buf = Vec::new(); + let mut longest = 0usize; + loop { + buf.clear(); + if reader.read_until(b'\n', &mut buf).unwrap() == 0 { + break; + } + while buf.last().is_some_and(|b| *b == b'\n' || *b == b'\r') { + buf.pop(); + } + longest = longest.max(buf.len()); + parser.push_line(&String::from_utf8_lossy(&buf)); + } + let b = parser.finish(); + println!( + "streamed: read+parse {:?}, {} files, peak transient buffer {longest} bytes \ + (vs {size} buffered)", + t.elapsed(), + b.len() + ); + assert_eq!(a.len(), b.len()); + let _ = std::fs::remove_file(&path); + } + + /// Measurement harness for issue #239, not a correctness gate — run with + /// `cargo test -- --ignored --nocapture bench_parse_budget`. + #[test] + #[ignore = "measurement, not an assertion"] + fn bench_parse_budget() { + use std::time::Instant; + let out = many_files(300, 300); + println!("input: {} bytes, 300 files × 300 lines", out.len()); + let t = Instant::now(); + let files = parse_unified(&out); + let elapsed = t.elapsed(); + let retained: usize = files + .iter() + .flat_map(|f| &f.hunks) + .map(|h| h.lines.len()) + .sum(); + let bytes: usize = files + .iter() + .flat_map(|f| &f.hunks) + .flat_map(|h| &h.lines) + .map(|l| l.text.capacity() + std::mem::size_of::()) + .sum(); + println!( + "parse {elapsed:?} → {retained} retained lines, ~{} KiB of DiffLine text \ + (unbudgeted would be 90000 lines / ~{} KiB)", + bytes / 1024, + 90_000 * (24 + std::mem::size_of::()) / 1024, + ); + } } diff --git a/src/ui/app.rs b/src/ui/app.rs index 9af5cdbc..0c908804 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -734,6 +734,14 @@ pub struct Tty7App { pub(crate) sftp_panel: crate::ui::sftp::SftpPanelState, /// Right detail panel (info / changes / files) docked beside the terminal. pub(crate) right_panel: crate::ui::right_panel::RightPanelState, + /// Repositories with a `git diff HEAD` probe in flight, keyed by machine + /// *and* working directory — the same path on two hosts is two different + /// work trees. One probe answers everyone: the diff overlay on any number of + /// tabs and the Changes panel all read the same result, instead of each + /// running its own copy of the same invocation and parse. See + /// [`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)>, /// 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). @@ -1207,6 +1215,7 @@ impl Tty7App { }, sftp_panel, right_panel: Default::default(), + diff_probes_inflight: Default::default(), file_tree, editor, sidebar_width: Rc::new(Cell::new(sidebar_width)), @@ -2850,6 +2859,14 @@ impl Tty7App { self.update_config(cx, |cfg| cfg.sidebar_grouping = grouping); } + /// Set whether the sidebar's `+N −N` counts open the diff overlay + /// (Settings → Window & Tabs). The counts themselves are unaffected either + /// way — this only governs the click. Persists the choice; the sidebar + /// re-derives from the `Config` global on the next render. + pub(crate) fn set_sidebar_diff_preview(&mut self, on: bool, cx: &mut Context) { + self.update_config(cx, |cfg| cfg.sidebar_diff_preview = on); + } + /// `ToggleTabSidebar`: flip the tab bar between the horizontal title-bar strip /// (`Top`) and the vertical left sidebar (`Left`), persisting the choice. pub(crate) fn toggle_tab_sidebar(&mut self, cx: &mut Context) { diff --git a/src/ui/diff_overlay.rs b/src/ui/diff_overlay.rs index ecf0d97a..5183e318 100644 --- a/src/ui/diff_overlay.rs +++ b/src/ui/diff_overlay.rs @@ -17,9 +17,20 @@ //! snapshot whose branch or counts disagree with what's shown — so a finishing //! command or agent turn refreshes the overlay through the exact trigger //! machinery the sidebar numbers already use. +//! +//! One probe, one snapshot, however many watchers: every tab's overlay and the +//! Changes panel go through +//! [`spawn_shared_diff_probe`](Tty7App::spawn_shared_diff_probe) and hold the +//! result behind an `Arc`. The element tree, meanwhile, is *not* virtualized — +//! so what keeps a big working tree from stalling the window is refusing to +//! build the rows in the first place: past +//! [`AUTO_COLLAPSE_TOTAL_LINES`](git_diff::AUTO_COLLAPSE_TOTAL_LINES) every +//! file opens collapsed under a summary, and at most +//! [`MAX_RENDERED_FILES`] cards are built at all. use std::collections::HashSet; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; +use std::sync::Arc; use gpui::{ AnyElement, FocusHandle, FontWeight, KeyDownEvent, Pixels, Window, div, prelude::*, px, @@ -28,7 +39,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, + self, AUTO_COLLAPSE_LINES, DiffSnapshot, FileDiff, FileStatus, LineKind, MAX_RENDERED_FILES, + Truncation, }; use crate::ui::app::Tty7App; use crate::ui::rounding; @@ -39,7 +51,11 @@ use crate::ui::rounding::RoundedCorners as _; pub(crate) enum DiffLoad { /// First probe still in flight. Loading, - Ready(DiffSnapshot), + /// A landed snapshot, shared rather than owned: one probe result reaches + /// every tab whose overlay watches this cwd *and* the Changes panel, and + /// the snapshot is a deep tree of owned strings — cloning it per holder on + /// the UI update path is exactly the cost issue #239 measured as a stall. + Ready(Arc), /// The probe came back "not a work tree" (repo deleted, dir gone). NotARepo, } @@ -147,6 +163,18 @@ impl Tty7App { } None => {} } + // The Changes panel may already hold this very repo's snapshot — it is + // the same `git diff HEAD`. Opening on it makes the overlay paint + // immediately instead of flashing "Reading diff…" for a probe whose + // answer is already in the process, and costs an `Arc` bump. A refresh + // probe still flies below, so the seeded view is never the last word. + // Read here, before the `&mut` borrow of the tab. + let seed = match (&self.right_panel.diff_cwd, &self.right_panel.diff) { + (Some(panel_cwd), Some(Some(snap))) if *panel_cwd == cwd => { + DiffLoad::Ready(Arc::clone(snap)) + } + _ => DiffLoad::Loading, + }; // The overlay steals focus (it needs Esc); snapshot the active pane so // closing lands back on the same terminal — same discipline as Settings. self.remember_active_pane(window, cx); @@ -158,7 +186,7 @@ impl Tty7App { host_id: host, cwd, focus_handle: focus_handle.clone(), - load: DiffLoad::Loading, + load: seed, loading: false, toggled: HashSet::new(), focus, @@ -219,39 +247,91 @@ impl Tty7App { return; }; overlay.loading = true; + self.spawn_shared_diff_probe(host, cwd, cx); + } + + /// One `git diff HEAD` per repository, however many things are waiting on + /// it — where "repository" is the machine *and* the path, since the same + /// path on two hosts is two different work trees. + /// + /// The overlay and the Changes panel used to probe the same repository + /// independently and each keep its own `DiffSnapshot` — issue #239's fifth + /// finding. Deduping here means opening both costs one invocation and one + /// parse, and [`install_diff_snapshot`](Self::install_diff_snapshot) hands + /// the *same* `Arc` to both rather than a second copy. + /// + /// Callers still mark themselves as waiting first (the overlay's `loading` + /// flag, the panel's `diff_pending`): that's the "refreshing…" hint, and it + /// is cleared by whichever probe lands for this repo, not necessarily the + /// one the caller thought it started. + pub(crate) fn spawn_shared_diff_probe( + &mut self, + host: crate::ui::host_ops::SharedHost, + cwd: PathBuf, + cx: &mut Context, + ) { + let key = (host.id(), cwd.clone()); + if !self.diff_probes_inflight.insert(key.clone()) { + return; // someone is already asking this exact question + } let probe_cwd = cwd.clone(); crate::ui::host_ops::HostOps::run( host, cx, move |h| git_diff::probe(h, &probe_cwd), move |app, result, cx| { - // Land on every tab whose overlay shows this repo on this - // machine — the spawning tab may no longer be active, and - // sibling tabs on the same repo are equally stale. A slot - // closed or swapped to another repo while we flew is skipped. - let mut landed = false; - for tab in app.tabs.iter_mut() { - let Some(overlay) = tab - .diff_overlay - .as_mut() - .filter(|o| o.cwd == cwd && o.host_id == id) - else { - continue; - }; - overlay.loading = false; - overlay.load = match &result { - Some(snap) => DiffLoad::Ready(snap.clone()), - None => DiffLoad::NotARepo, - }; - landed = true; - } - if landed { - cx.notify(); - } + app.diff_probes_inflight.remove(&key); + app.install_diff_snapshot(key.0, &cwd, result.map(Arc::new), cx); }, ); } + /// Hand a landed probe to everything watching `cwd`: every tab whose + /// overlay shows it (the spawning tab may no longer be active, and sibling + /// tabs on the same repo are equally stale) and the Changes panel when it + /// is on the same cwd. Slots that closed or swapped repos while the probe + /// flew are skipped. + fn install_diff_snapshot( + &mut self, + host: crate::ui::host_ops::HostId, + cwd: &Path, + snap: Option>, + cx: &mut Context, + ) { + let mut landed = false; + for tab in self.tabs.iter_mut() { + let Some(overlay) = tab + .diff_overlay + .as_mut() + .filter(|o| o.cwd == cwd && o.host_id == host) + else { + continue; + }; + overlay.loading = false; + // `Arc::clone`, not a deep copy of the file/hunk/line tree. + overlay.load = match &snap { + Some(snap) => DiffLoad::Ready(Arc::clone(snap)), + None => DiffLoad::NotARepo, + }; + landed = true; + } + // The panel's wait is cleared by the answer it asked for, whoever + // actually ran it. `diff_cwd` is checked separately: the panel can have + // navigated to another repo — or another machine — since, in which case + // this result is stale and only the wait is over. + let key = (host, cwd.to_path_buf()); + if self.right_panel.diff_pending.as_ref() == Some(&key) { + self.right_panel.diff_pending = None; + if self.right_panel.diff_cwd.as_ref() == Some(&key) { + self.right_panel.diff = Some(snap); + } + landed = true; + } + if landed { + cx.notify(); + } + } + /// Re-probe the open overlay when the shared status cache learned /// something newer than what's shown — called from the app's /// `observe_global::` hook, i.e. on the very triggers @@ -525,21 +605,50 @@ impl Tty7App { focused: Option, cx: &mut Context, ) -> AnyElement { + // 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 mut list = v_flex().gap_3().p_4().w_full(); + if oversized { + list = list.child(self.diff_oversized_notice(snap, 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. + let shown = snap.files.len().min(MAX_RENDERED_FILES); for (idx, file) in snap.files.iter().enumerate() { if focused.is_some_and(|f| f != idx) { continue; } + if focused.is_none() && idx >= shown { + break; + } // A file opened by name was asked for explicitly — show its body // even when it's over the auto-collapse threshold. The header still // toggles, so a huge file can be folded back down. let expanded = if focused == Some(idx) { !toggled.contains(&file.path) } else { - file_expanded(file, toggled) + file_expanded(file, toggled, oversized) }; list = list.child(self.diff_file_card(idx, file, expanded, cx)); } + if focused.is_none() && snap.files.len() > shown { + let rest = snap.files.len() - shown; + list = list.child( + div() + .w_full() + .px_2p5() + .py_1p5() + .text_xs() + .text_color(cx.theme().muted_foreground) + .child(format!( + "… and {rest} more changed file{} — run `git diff` in the terminal to see them.", + if rest == 1 { "" } else { "s" } + )), + ); + } // Untracked files are a property of the tree, not of the focused file. if focused.is_none() && !snap.untracked.is_empty() { list = list.child(self.diff_untracked_section(&snap.untracked, cx)); @@ -553,6 +662,35 @@ impl Tty7App { .into_any_element() } + /// The banner an oversized diff leads with: says why every file is folded + /// shut and points at the two ways out (expand one file, or use the + /// 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 { + let mut text = format!( + "This diff is too large to render efficiently ({} changed files, {} diff lines). \ + Every file is collapsed — expand individual files, or run `git diff` in the terminal.", + snap.files.len(), + snap.retained_lines(), + ); + if snap.budget_exhausted() { + text.push_str(" Some files' contents were dropped to keep tty7 responsive."); + } + div() + .w_full() + .px_2p5() + .py_2() + .rounded_md() + .border_1() + .border_color(cx.theme().border) + .bg(cx.theme().secondary) + .text_xs() + .text_color(cx.theme().muted_foreground) + .child(text) + .into_any_element() + } + /// One file's card: a clickable header row and, when expanded, the hunks. fn diff_file_card( &self, @@ -562,8 +700,11 @@ impl Tty7App { cx: &mut Context, ) -> AnyElement { // Binary files and pure renames have no hunk body to reveal; their - // header is inert (no chevron, no click). - let expandable = !file.binary && !file.hunks.is_empty(); + // header is inert (no chevron, no click). A file the repo-wide budget + // emptied *is* expandable even with no hunks — what it reveals is the + // note explaining why, which is otherwise unreachable. + let expandable = + !file.binary && (!file.hunks.is_empty() || file.truncated == Some(Truncation::Budget)); let (glyph, glyph_color) = match file.status { FileStatus::Added => ("A", cx.theme().success), FileStatus::Modified => ("M", cx.theme().warning), @@ -722,7 +863,21 @@ impl Tty7App { body = body.child(self.diff_split_row(row, closing_row == Some((h, r)), cx)); } } - if file.truncated { + if let Some(reason) = file.truncated { + let note = match reason { + Truncation::PerFile => format!( + "Diff truncated at {} lines — run `git diff` in the terminal for the rest.", + git_diff::MAX_LINES_PER_FILE + ), + // Naming the repo-wide budget matters: this file may be + // three lines long, and "truncated" without a why reads as + // tty7 having lost the change. + Truncation::Budget => { + "Body not loaded — this working tree is past tty7's diff budget. \ + Run `git diff` in the terminal for this file." + .to_string() + } + }; body = body.child( div() .w_full() @@ -730,10 +885,7 @@ impl Tty7App { .py_1() .text_xs() .text_color(cx.theme().muted_foreground) - .child(format!( - "Diff truncated at {} lines — run `git diff` in the terminal for the rest.", - git_diff::MAX_LINES_PER_FILE - )), + .child(note), ); } card = card.child(body); @@ -888,8 +1040,15 @@ fn focused_name(overlay: &DiffOverlayState) -> Option { /// Whether a file's body shows: small text diffs default open, big ones (and /// anything the user explicitly flipped) invert via the `toggled` set. -fn file_expanded(file: &FileDiff, toggled: &HashSet) -> bool { - let default_open = file.added + file.removed <= AUTO_COLLAPSE_LINES; +/// +/// `collapse_all` is the repo-wide override: past +/// [`AUTO_COLLAPSE_TOTAL_LINES`](git_diff::AUTO_COLLAPSE_TOTAL_LINES) nothing +/// opens by default, because the per-file threshold can't see that forty +/// innocent files are about to expand at once. The user's explicit toggles +/// still win over it — that's the "expand individual files" the oversized +/// notice promises. +fn file_expanded(file: &FileDiff, toggled: &HashSet, collapse_all: bool) -> bool { + let default_open = !collapse_all && file.added + file.removed <= AUTO_COLLAPSE_LINES; default_open != toggled.contains(&file.path) } @@ -1034,4 +1193,170 @@ mod tests { assert_eq!(rows[0].right.as_ref().unwrap().text, " indented"); assert!(rows[0].left.is_none()); } + + /// A file of `added` changed lines, small enough to open by default on its + /// own. + fn small_file(path: &str, added: u32) -> FileDiff { + FileDiff { + path: path.to_string(), + old_path: None, + status: FileStatus::Modified, + added, + removed: 0, + binary: false, + truncated: None, + hunks: vec![git_diff::Hunk { + header: "@@ -1,1 +1,1 @@".to_string(), + lines: (0..added) + .map(|i| line(LineKind::Added, None, Some(i + 1), "x")) + .collect(), + }], + } + } + + /// The per-file threshold on its own: a small file opens, a big one doesn't, + /// and an explicit toggle inverts either. Unchanged behaviour — this is the + /// small-working-tree case that must feel identical. + #[test] + fn per_file_collapse_is_unchanged_below_the_repo_threshold() { + let small = small_file("small.rs", 10); + let big = small_file("big.rs", AUTO_COLLAPSE_LINES + 1); + let none = HashSet::new(); + assert!(file_expanded(&small, &none, false)); + assert!(!file_expanded(&big, &none, false)); + + let toggled: HashSet = ["small.rs".to_string(), "big.rs".to_string()].into(); + assert!(!file_expanded(&small, &toggled, false)); + assert!(file_expanded(&big, &toggled, false)); + } + + /// The repo-wide override: past the total threshold nothing opens by + /// default, however small each file is — the case a per-file rule can't see + /// (issue #239, finding 4). An explicit toggle still wins, which is what + /// "expand individual files" in the oversized notice means. + #[test] + fn repo_wide_collapse_overrides_the_per_file_default() { + let small = small_file("small.rs", 10); + let none = HashSet::new(); + assert!(file_expanded(&small, &none, false)); + assert!(!file_expanded(&small, &none, true), "collapsed en masse"); + + let toggled: HashSet = ["small.rs".to_string()].into(); + assert!( + file_expanded(&small, &toggled, true), + "the user's own click still opens it" + ); + } + + /// Sixty files of forty lines each never trip the per-file threshold (each + /// is a tenth of it), yet would open 2400 diff rows at once. `oversized` + /// catches it on the line axis, and with everything collapsed the overlay + /// builds zero rows. + #[test] + fn many_medium_files_are_oversized_and_build_no_rows() { + let snap = DiffSnapshot { + files: (0..60) + .map(|i| small_file(&format!("f{i}.rs"), 40)) + .collect(), + ..Default::default() + }; + assert!( + snap.files.iter().all(|f| f.added <= AUTO_COLLAPSE_LINES), + "no single file is over the per-file threshold" + ); + assert!(snap.oversized()); + + let none = HashSet::new(); + let rows_expanded: usize = snap + .files + .iter() + .filter(|f| file_expanded(f, &none, false)) + .flat_map(|f| &f.hunks) + .map(|h| split_hunk(&h.lines).len()) + .sum(); + let rows_collapsed: usize = snap + .files + .iter() + .filter(|f| file_expanded(f, &none, true)) + .flat_map(|f| &f.hunks) + .map(|h| split_hunk(&h.lines).len()) + .sum(); + assert_eq!(rows_expanded, 2400, "what the old rule would have built"); + assert_eq!(rows_collapsed, 0); + } + + /// The other side of the same coin: a busy-but-ordinary afternoon — forty + /// files, a few lines each — is *not* oversized and opens expanded exactly + /// as it does today. The thresholds must not tax a normal working tree. + #[test] + fn an_ordinary_busy_tree_is_not_oversized() { + let snap = DiffSnapshot { + files: (0..40) + .map(|i| small_file(&format!("f{i}.rs"), 12)) + .collect(), + ..Default::default() + }; + assert!(!snap.oversized()); + let none = HashSet::new(); + assert!(snap.files.iter().all(|f| file_expanded(f, &none, false))); + } + + /// However many files change, the overlay builds at most + /// [`MAX_RENDERED_FILES`] cards and says so. + #[test] + fn file_cards_are_capped() { + let snap = DiffSnapshot { + files: (0..MAX_RENDERED_FILES + 25) + .map(|i| small_file(&format!("f{i}.rs"), 1)) + .collect(), + ..Default::default() + }; + let shown = snap.files.len().min(MAX_RENDERED_FILES); + assert_eq!(shown, MAX_RENDERED_FILES); + assert_eq!( + snap.files.len() - shown, + 25, + "the tail gets one summary line" + ); + } + + /// Measurement harness for issue #239, finding 2 — run with + /// `cargo test -- --ignored --nocapture bench_snapshot_share`. + #[test] + #[ignore = "measurement, not an assertion"] + fn bench_snapshot_share() { + use std::time::Instant; + + // Two sizes: what v26.7.5 would have held for a big agent session + // (300 files × 300 lines, no repo-wide budget), and what this build + // retains for the same tree once the budget applies. + for (label, files, per_file) in [ + ("unbudgeted (v26.7.5 shape)", 300, 300), + ("budgeted (this build retains)", 300, 67), + ] { + let snap = Arc::new(DiffSnapshot { + files: (0..files) + .map(|i| small_file(&format!("f{i}.rs"), per_file)) + .collect(), + ..Default::default() + }); + let lines: usize = snap.retained_lines(); + + let t = Instant::now(); + for _ in 0..10 { + let _deep = (*snap).clone(); + } + let deep = t.elapsed() / 10; + + let t = Instant::now(); + for _ in 0..100 { + let _shared = Arc::clone(&snap); + } + let shared = t.elapsed() / 100; + println!( + "{label}: {files} files / {lines} lines — deep clone {deep:?} vs \ + Arc::clone {shared:?}, per holder on the UI thread" + ); + } + } } diff --git a/src/ui/right_panel.rs b/src/ui/right_panel.rs index 88e10be5..98e95733 100644 --- a/src/ui/right_panel.rs +++ b/src/ui/right_panel.rs @@ -22,10 +22,11 @@ use gpui_component::{ ActiveTheme as _, Icon, IconName, InteractiveElementExt as _, Sizable as _, h_flex, v_flex, }; use std::path::PathBuf; +use std::sync::Arc; use crate::core::config::{Config, RightPanelTab}; use crate::daemon::protocol::PaneProcs; -use crate::terminal::git_diff::{self, DiffSnapshot}; +use crate::terminal::git_diff::DiffSnapshot; use crate::ui::app::{ CONTENT_INSET, TILE_GLYPH_SM, TILE_SIZE_SM, Tty7App, tile_trailing_inset, tile_trailing_inset_sm, @@ -61,10 +62,17 @@ pub(crate) struct RightPanelState { /// machines is two repositories. pub(crate) diff_cwd: Option<(crate::ui::host_ops::HostId, PathBuf)>, /// Last completed probe. `Some(None)` and `None` are different answers: - /// "probed, not a work tree" versus "never probed". - pub(crate) diff: Option>, - /// A probe is in flight; keeps the render path from spawning a second one. - pub(crate) diff_loading: bool, + /// "probed, not a work tree" versus "never probed". Shared with the diff + /// overlay rather than a second copy of the same tree — see + /// [`Tty7App::spawn_shared_diff_probe`]. + pub(crate) diff: Option>>, + /// The machine-and-cwd this panel is waiting on a probe for; keeps the + /// render path from spawning a second one. A key rather than a flag because + /// the shared probe (see [`Tty7App::spawn_shared_diff_probe`]) lands per + /// repo: the panel has to know *which* answer clears its wait, or a probe + /// for the repo it just navigated away from would leave it stuck on + /// "Loading…". + pub(crate) diff_pending: Option<(crate::ui::host_ops::HostId, PathBuf)>, /// The pane `procs` describes, so a pane switch invalidates it rather than /// showing the previous pane's processes under the new pane's name. pub(crate) procs_pane: Option, @@ -1171,8 +1179,8 @@ impl Tty7App { if self.right_panel.diff_cwd.as_ref() != Some(&key) { self.right_panel.diff_cwd = Some(key); self.right_panel.diff = None; - self.spawn_right_panel_diff(host.clone(), cwd.clone(), cx); - } else if self.right_panel.diff.is_none() && !self.right_panel.diff_loading { + self.spawn_right_panel_diff(cwd.clone(), cx); + } else if self.right_panel.diff.is_none() && self.right_panel.diff_pending.is_none() { // Nothing cached and nothing in flight: a probe for a previous cwd // landed after we had already moved on and dropped its result, so // no one is left to answer for this one. Without this the tab would @@ -1316,33 +1324,24 @@ impl Tty7App { self.panel_scroll(inner, title) } - /// Off-thread `git diff` for the panel, mirroring the diff overlay's probe. + /// Off-thread `git diff` for the panel — the *same* probe the diff overlay + /// uses. This used to be its own `git_diff::probe` call keeping its own + /// `DiffSnapshot`, so a repo with both open generated, parsed and stored + /// its full diff twice (issue #239, finding 5). Now both go through + /// [`Tty7App::spawn_shared_diff_probe`], which dedupes by machine-and-cwd + /// and installs one `Arc` into whoever is watching — including this panel, + /// which is why there is no result handler left here. fn spawn_right_panel_diff( &mut self, host: crate::ui::host_ops::SharedHost, cwd: PathBuf, cx: &mut Context, ) { - if self.right_panel.diff_loading { + if self.right_panel.diff_pending.is_some() { return; } - self.right_panel.diff_loading = true; - let key = (host.id(), cwd.clone()); - crate::ui::host_ops::HostOps::run( - host, - cx, - move |h| git_diff::probe(h, &cwd), - move |app, result, cx| { - app.right_panel.diff_loading = false; - // Drop the result if the panel moved on to another repo — or - // another machine — while we flew; otherwise a slow probe would - // overwrite a newer one. - if app.right_panel.diff_cwd.as_ref() == Some(&key) { - app.right_panel.diff = Some(result); - cx.notify(); - } - }, - ); + self.right_panel.diff_pending = Some((host.id(), cwd.clone())); + self.spawn_shared_diff_probe(host, cwd, cx); } /// Re-probe the Changes list when the shared status cache learned something @@ -1357,7 +1356,7 @@ impl Tty7App { /// Comparing branch + totals first keeps the quiet case free, and re-probing /// in place leaves the rows on screen until the new snapshot lands. pub(crate) fn right_panel_refresh_changes(&mut self, cx: &mut Context) { - if self.right_panel.diff_loading { + if self.right_panel.diff_pending.is_some() { return; } let Some((id, cwd)) = self.right_panel.diff_cwd.clone() else { diff --git a/src/ui/settings.rs b/src/ui/settings.rs index f7bd3c7e..2734f060 100644 --- a/src/ui/settings.rs +++ b/src/ui/settings.rs @@ -380,6 +380,11 @@ fn settings_search_entries() -> &'static [SearchEntry] { title: "Sidebar grouping", keywords: "tabs group repo repository git scratch header sidebar flat", }, + SearchEntry { + section: WindowTabs, + title: "Open diff preview from sidebar counts", + keywords: "diff overlay preview sidebar counts git changes click branch lines", + }, SearchEntry { section: WindowTabs, title: "Notify on command finish", @@ -4458,6 +4463,7 @@ impl Tty7App { TabBarPosition::Top => 0, TabBarPosition::Left => 1, }; + let sidebar_diff_preview = cfg.sidebar_diff_preview; let sidebar_grouping_idx = match cfg.sidebar_grouping { crate::core::config::SidebarGrouping::Repo => 0, crate::core::config::SidebarGrouping::None => 1, @@ -4568,6 +4574,10 @@ impl Tty7App { this.set_tab_bar_position(pos, cx); }, ); + let sidebar_diff_switch = crate::ui::theme::switch("wt-sidebar-diff-preview", cx) + .checked(sidebar_diff_preview) + .on_click(cx.listener(|this, on: &bool, _w, cx| this.set_sidebar_diff_preview(*on, cx))) + .into_any_element(); let sidebar_grouping_radio = self.segmented( "wt-sidebar-grouping", &["By repo", "Flat"], @@ -4645,6 +4655,16 @@ impl Tty7App { sidebar_grouping_radio, cx, )) + // Phrased around what *stays*: the worry this row answers is "will + // turning it off cost me the branch and the numbers", and the + // answer is no — only the click goes. + .child(self.settings_row( + "Open diff preview from sidebar counts", + "Click a row's +N −N to open the working-tree diff in an overlay. Off keeps the \ + branch and the counts on the row and just stops them being clickable.", + sidebar_diff_switch, + cx, + )) .child(self.section_rule(cx)) .child(self.section_header("Notifications", cx)) .child(self.settings_row( diff --git a/src/ui/tab_sidebar.rs b/src/ui/tab_sidebar.rs index 3bda5060..0da4d819 100644 --- a/src/ui/tab_sidebar.rs +++ b/src/ui/tab_sidebar.rs @@ -245,19 +245,22 @@ impl Tty7App { // a change never disturbs the title; grouping keys on the repo // root only, so a branch switch never relocates the row either // (see `Tab::sidebar_group`). The diff counts are a quiet - // green/red readout and double as the diff-overlay toggle: click - // them to peek another session's changes in an overlay without - // activating this row's tab. The cwd they probe is the same one - // the status resolved through, so overlay and counts always - // describe the same repo. - let git_cwd = tab.pane.focused_or_first(window, cx).and_then(|leaf| { - let view = leaf.read(cx); - let cwd = view.git_status_cwd()?.to_path_buf(); - // The id, not the host: opening the overlay needs no live - // connection — a disconnected machine's last diff is still - // worth showing, and the re-probe resolves the id itself. - Some((view.host_id(), cwd)) - }); + // green/red readout and, unless `sidebar_diff_preview` is off, + // double as the diff-overlay toggle: click them to peek another + // session's changes in an overlay without activating this row's + // tab. The cwd they probe is the same one the status resolved + // through, so overlay and counts always describe the same repo. + let git_cwd = diff_click_cwd( + cx.global::(), + tab.pane.focused_or_first(window, cx).and_then(|leaf| { + let view = leaf.read(cx); + let cwd = view.git_status_cwd()?.to_path_buf(); + // The id, not the host: opening the overlay needs no live + // connection — a disconnected machine's last diff is still + // worth showing, and the re-probe resolves the id itself. + Some((view.host_id(), cwd)) + }), + ); let git_line = tab.git_status(Some(window), cx).map(|g| { let mut line = h_flex() .id(("sidebar-git", i)) @@ -1285,6 +1288,22 @@ fn group_names(roots: &[&PathBuf]) -> Vec { } } +/// The machine-and-cwd a sidebar row's `+N −N` counts should open the diff +/// overlay for, or `None` when they're a plain readout. Generic over the +/// payload so it stays indifferent to what identifies a repo — that pair grew a +/// `HostId` when panes learned to live on other machines, and this gate did not +/// need to know. +/// +/// One value drives both halves of the interaction: the render path hangs the +/// pointer cursor *and* the `toggle_diff_overlay` mouse handler off the same +/// `when_some`, so a `None` here provably removes both and the press falls +/// through to the row's own activate handler like any other part of the label. +/// The branch and the counts themselves don't consult this — turning the +/// preview off must not cost you the readout (issue #239). +fn diff_click_cwd(cfg: &Config, target: Option) -> Option { + cfg.sidebar_diff_preview.then_some(target).flatten() +} + #[cfg(test)] mod tests { use super::*; @@ -1293,6 +1312,37 @@ mod tests { PathBuf::from(s) } + /// With the preview on (the default) the counts carry the repo's cwd, which + /// is what gives them the pointer cursor and the `toggle_diff_overlay` + /// handler; with it off they carry nothing and neither is attached. + #[test] + fn diff_preview_setting_gates_the_click_target() { + let mut cfg = Config::default(); + assert!(cfg.sidebar_diff_preview, "default is today's behaviour"); + assert_eq!( + diff_click_cwd(&cfg, Some(p("/w/repo"))), + Some(p("/w/repo")), + "enabled: the counts are a click target" + ); + + cfg.sidebar_diff_preview = false; + assert_eq!( + diff_click_cwd(&cfg, Some(p("/w/repo"))), + None, + "disabled: no cwd, so no cursor and no toggle_diff_overlay" + ); + } + + /// A pane outside a git work tree has no cwd to open either way — the + /// setting doesn't invent one. + #[test] + fn diff_click_target_needs_a_repo_either_way() { + let mut cfg = Config::default(); + assert_eq!(diff_click_cwd(&cfg, None), None); + cfg.sidebar_diff_preview = false; + assert_eq!(diff_click_cwd(&cfg, None), None); + } + /// Groups appear in first-appearance order with Scratch pinned last, and /// an all-`None` key set renders as one headerless flat section. #[test]