From 09c1ca3c1f11013540feb1c046622ac5b356d5b4 Mon Sep 17 00:00:00 2001 From: Moncef Naji Date: Thu, 30 Jul 2026 00:07:17 -0400 Subject: [PATCH 1/3] refactor(app): unify the file-collapse predicate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Whether a file's diff body is hidden was decided independently in five places: the annotation builder, both diff renderers, and two scroll-height helpers. They have to agree or the cursor lands on rows that were never drawn, which is the failure mode behind several past scroll fixes. Extract `App::is_file_collapsed` and route all five through it. No behavior change. The predicate deliberately says nothing about single-file view, because the call sites genuinely disagree: `file_render_height` ignores it while the renderers honor it. Folding the check in would have silently changed `file_render_height` for its two direct callers, so each site keeps its own gate and the asymmetry is preserved rather than accidentally "fixed". No added per-frame cost. Three sites had cached a reviewed bool and used it to decide two different things -- whether to collapse, and whether to draw the "Marked reviewed" banner -- which a predicate call would have turned into a second map lookup per file per frame. Those two decisions are mutually exclusive on `is_single_file_view`, so they are now one if/else instead of two guarded conditions. Each branch performs exactly one file-state lookup, matching the previous count: multi-file: file_header_prefix_text + is_file_collapsed (2, unchanged) single-file: is_file_reviewed (1, unchanged) hunk_positions: one lookup per file (1, unchanged) Uses of `is_file_reviewed` that drive reviewed-specific decoration -- the tree checkbox, the `✓` header mark, and the banner -- are left alone; only the collapse gates moved. Co-Authored-By: Claude Opus 5 (1M context) --- src/app/annotations.rs | 8 +++--- src/app/navigation.rs | 15 +++++------ src/app/reviewed.rs | 22 +++++++++++++++ src/app/tests/single_file_view_tests.rs | 36 +++++++++++++++++++++++++ src/ui/diff_side_by_side.rs | 18 ++++++------- src/ui/diff_unified.rs | 25 ++++++++--------- 6 files changed, 91 insertions(+), 33 deletions(-) diff --git a/src/app/annotations.rs b/src/app/annotations.rs index 3d3029d6..6fdea5af 100644 --- a/src/app/annotations.rs +++ b/src/app/annotations.rs @@ -163,10 +163,10 @@ impl App { .push(AnnotatedLine::FileHeader { file_idx }); } - // If reviewed, skip all content for this file. Single-file - // view ignores the reviewed-collapse since the user - // explicitly focused this file. - if self.session.is_file_reviewed(path) && !self.is_single_file_view { + // If collapsed, skip all content for this file. Single-file + // view ignores the collapse since the user explicitly focused + // this file. + if self.is_file_collapsed(file) && !self.is_single_file_view { continue; } diff --git a/src/app/navigation.rs b/src/app/navigation.rs index 74ff89f4..d3576e3d 100644 --- a/src/app/navigation.rs +++ b/src/app/navigation.rs @@ -841,16 +841,15 @@ impl App { continue; } let path = file.display_path(); - let is_reviewed = self.session.is_file_reviewed(path); + // Mutually exclusive, so only one file-state lookup per file. if !single { cumulative += 1; // File header - } - if !single && is_reviewed { - // multi-file collapsed: no body, no trailing spacing - continue; - } - if single && is_reviewed { + if self.is_file_collapsed(file) { + // multi-file collapsed: no body, no trailing spacing + continue; + } + } else if self.session.is_file_reviewed(path) { cumulative += 1; // banner } if let Some(review) = self.session.files.get(path) { @@ -994,7 +993,7 @@ impl App { } pub(in crate::app) fn file_render_height(&self, file_idx: usize, file: &DiffFile) -> usize { - if self.session.is_file_reviewed(file.display_path()) { + if self.is_file_collapsed(file) { return 1; // collapsed: header only } 1 + self.file_render_body_height(file_idx, file) // header + body diff --git a/src/app/reviewed.rs b/src/app/reviewed.rs index 0ed7133a..04919113 100644 --- a/src/app/reviewed.rs +++ b/src/app/reviewed.rs @@ -273,6 +273,28 @@ impl App { } } + /// Whether a file's diff body is hidden, leaving only its header row. + /// + /// Reviewed files collapse so a long review stream shrinks as it is + /// worked through. The annotation builder, both diff renderers, and the + /// scroll-height math all have to agree on this or the cursor lands on + /// rows that aren't drawn, so they share this one predicate. + /// + /// Deliberately says nothing about single-file view: the call sites + /// disagree about it (`file_render_height` ignores it, the renderers + /// honor it), so each keeps its own gate rather than having one folded + /// in here. + /// + /// Both renderers and `hunk_positions` call this once per file per + /// frame, so it has to stay cheap — currently a single map lookup. + /// Callers reach it through a `!is_single_file_view` branch that is + /// exclusive with the reviewed-banner branch, so no site pays for two + /// lookups to decide one thing. + #[inline] + pub fn is_file_collapsed(&self, file: &DiffFile) -> bool { + self.session.is_file_reviewed(file.display_path()) + } + pub fn file_count(&self) -> usize { self.diff_files.len() } diff --git a/src/app/tests/single_file_view_tests.rs b/src/app/tests/single_file_view_tests.rs index da7fb8d0..cfc7b86b 100644 --- a/src/app/tests/single_file_view_tests.rs +++ b/src/app/tests/single_file_view_tests.rs @@ -516,6 +516,42 @@ fn release_then_press_walks_when_keyboard_enhancement_supported() { assert!(!app.down_released_since_arm); } +#[test] +fn is_file_collapsed_tracks_reviewed_state() { + let files = vec![ + file("a.rs", vec![hunk(1, 3)]), + file("b.rs", vec![hunk(1, 3)]), + ]; + let mut app = app_with(files); + + let a = app.diff_files[0].clone(); + let b = app.diff_files[1].clone(); + assert!(!app.is_file_collapsed(&a)); + assert!(!app.is_file_collapsed(&b)); + + app.toggle_reviewed_for_file_idx(0, false); + assert!(app.is_file_collapsed(&a)); + assert!(!app.is_file_collapsed(&b)); +} + +#[test] +fn is_file_collapsed_ignores_single_file_view() { + // The predicate deliberately says nothing about single-file view: its + // consumers disagree about it (`file_render_height` ignores it, the + // renderers honor it), so each keeps its own gate. Locking that here + // stops a future change from folding the check in and silently + // altering `file_render_height`. + let files = vec![file("a.rs", vec![hunk(1, 3)])]; + let mut app = app_with(files); + app.toggle_reviewed_for_file_idx(0, false); + let a = app.diff_files[0].clone(); + + app.is_single_file_view = false; + assert!(app.is_file_collapsed(&a)); + app.is_single_file_view = true; + assert!(app.is_file_collapsed(&a)); +} + #[test] fn effective_file_height_is_zero_for_non_current_in_single_file_view() { let files = vec![ diff --git a/src/ui/diff_side_by_side.rs b/src/ui/diff_side_by_side.rs index a3fc6384..a18844b2 100644 --- a/src/ui/diff_side_by_side.rs +++ b/src/ui/diff_side_by_side.rs @@ -404,8 +404,9 @@ pub(super) fn render_side_by_side_diff(frame: &mut Frame, app: &mut App, area: R continue; } let path = file.display_path(); - let is_reviewed = app.session.is_file_reviewed(path); + // Mutually exclusive branches, as in the unified renderer: each + // side pays for only the file-state lookup it actually needs. if !app.is_single_file_view { let indicator = cursor_indicator_spaced(line_idx, ctx.current_line_idx); let header_text = crate::ui::diff_view::file_header_prefix_text(app, file); @@ -418,15 +419,14 @@ pub(super) fn render_side_by_side_diff(frame: &mut Frame, app: &mut App, area: R ), ])); line_idx += 1; - } - // If file is reviewed (and we're in multi-file view), skip the - // body. Single-file view keeps the focused file visible under a - // dimmed banner. - if is_reviewed && !app.is_single_file_view { - continue; - } - if is_reviewed && app.is_single_file_view { + // Collapsed files render as the header alone. + if app.is_file_collapsed(file) { + continue; + } + } else if app.session.is_file_reviewed(path) { + // Single-file view keeps the focused file visible under a + // dimmed banner. let indicator = cursor_indicator(line_idx, ctx.current_line_idx); lines.push(Line::from(vec![ Span::styled(indicator, styles::current_line_indicator_style(&app.theme)), diff --git a/src/ui/diff_unified.rs b/src/ui/diff_unified.rs index ea8b05e5..e1573706 100644 --- a/src/ui/diff_unified.rs +++ b/src/ui/diff_unified.rs @@ -246,12 +246,14 @@ pub(super) fn render_unified_diff(frame: &mut Frame, app: &mut App, area: Rect) continue; } let path = file.display_path(); - let is_reviewed = app.session.is_file_reviewed(path); - // The `═══ filename ═══` separator is redundant in single-file - // view: the status bar and file list already name the file, and - // the wide bar of `═` characters confuses horizontal scrolling. + // Multi-file and single-file view are mutually exclusive here, so + // this is one branch rather than two guarded conditions: each side + // then pays for only the file-state lookup it actually needs. if !app.is_single_file_view { + // The `═══ filename ═══` separator is redundant in single-file + // view: the status bar and file list already name the file, and + // the wide bar of `═` characters confuses horizontal scrolling. let indicator = cursor_indicator_spaced(line_idx, current_line_idx); let header_text = crate::ui::diff_view::file_header_prefix_text(app, file); lines.push(Line::from(vec![ @@ -263,15 +265,14 @@ pub(super) fn render_unified_diff(frame: &mut Frame, app: &mut App, area: Rect) ), ])); line_idx += 1; - } - // If file is reviewed (and we're in multi-file view), skip - // rendering the body. In single-file view the user explicitly - // focused this file, so show its content under a dimmed banner. - if is_reviewed && !app.is_single_file_view { - continue; - } - if is_reviewed && app.is_single_file_view { + // Collapsed files render as the header alone. + if app.is_file_collapsed(file) { + continue; + } + } else if app.session.is_file_reviewed(path) { + // Single-file view shows a reviewed file's content anyway -- + // the user explicitly focused it -- under a dimmed banner. let indicator = cursor_indicator(line_idx, current_line_idx); lines.push(Line::from(vec![ Span::styled(indicator, styles::current_line_indicator_style(&app.theme)), From 220ad7d69e673bfcdb1761e425cf2d536e820193 Mon Sep 17 00:00:00 2001 From: Moncef Naji Date: Thu, 30 Jul 2026 11:03:59 -0400 Subject: [PATCH 2/3] feat: collapse generated files marked in .gitattributes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A review of hand-written code gets buried when a regenerated protobuf or API client lands in the same diff. Recognize the marker the forges already define and let the reviewer opt into collapsing those files, and into dropping them from review progress. Detection is `.gitattributes` only -- `linguist-generated` (GitHub's Linguist) or `gitlab-generated` -- resolved through libgit2's `get_attr`. That buys nested `.gitattributes`, `.git/info/attributes`, `core.attributesFile`, and `[attr]` macros behaving exactly as `git check-attr` does, for free. A header-regex heuristic was rejected: the marker sits at line 1, outside every hunk, so it needs full file content that PR/MR mode doesn't have. The value has to go through `AttrValue` rather than being compared directly, because `get_attr` returns a sentinel string for a set-but- valueless attribute. GitHub documents `linguist-generated=true` and GitLab documents bare `gitlab-generated`, which land in different variants (`String("true")` vs `True`); a naive `is_some()` check would read `-gitlab-generated` as an opt-in and hide code the user asked to see. An explicit opt-out on either attribute beats an opt-in on the other, and an undocumented value counts as not generated -- both fail toward showing the diff, because a needlessly shown diff is an annoyance and a needlessly hidden one is unreviewed code. Both behaviors are opt-in via `[generated]`: collapse = false hide the diff body, `Space` expands one file count = true count toward the reviewed/total indicator Count-exclusion is deliberately not gated by `collapse`: expanding a generated file to peek at it must not add it to the denominator, or progress would regress as a side effect of looking. Both halves of the fraction drop the file, or marking one reviewed would report 2/1. With both at their defaults tuicr behaves exactly as before and pays no detection cost -- no repository is opened and no attribute is read. The detected set is kept when the feature is switched off, so re-enabling costs no libgit2 work. That makes it the wrong thing to key the decoration off: the `[generated]` labels, the dimmed tree rows, and the counter are gated on the same condition as detection instead, so they are a function of the current settings rather than of what the session detected earlier. Otherwise `:set generated` followed by `:set nogenerated` would uncollapse the files but leave every label behind, never returning to how the session started. No added per-frame cost in the default configuration. `is_file_collapsed` orders its checks so the reviewed lookup still short-circuits first and `collapse_generated` is a bare bool that is false unless the user opted in: default: is_file_reviewed (1 lookup, unchanged) opted in: + generated_files (1 per unreviewed file) + expanded_generated (only for generated files) `file_header_prefix_text` gains one lookup per file per frame when opted in, for the `[generated]` label. Cancelling that out would mean threading the already-known reviewed state into it, which is worth doing separately -- multi-file view pays two lookups there on main already. Detection is hooked into `rebuild_annotations` rather than into each of the fifteen `self.diff_files = ...` assignments. Every one of them rebuilds annotations afterwards, so a single hook cannot drift out of sync with the render, where a missed assignment site would silently desynchronize the annotations from what is drawn. Probed paths are memoized, which doubles as the staleness test: a repeat call over an unchanged file set costs one hash lookup per file and no libgit2 work. Reload discards the memo so an edited `.gitattributes` takes effect without a restart. State lives on `App` keyed by display path, never by index -- reloads, watch ticks, and commit-selection changes replace `diff_files` wholesale and shift every index. No `DiffFile` field: it would touch all 31 struct literals for no benefit, since the expanded-state set has to live on `App` regardless. `Space` in the diff panel expands the generated file under the cursor. It was unbound there and already means "expand what's under the cursor" in the file list and the commit selector; when the cursor is not on a collapsed generated file it falls through to the shared handler as before. It requires collapse to be on rather than merely that the file is generated, because with `count = false` alone nothing is hidden and reporting an expansion would change nothing on screen. The tree dims generated filenames rather than adding a glyph -- `▶` already means "collapsed directory" in that panel, and `▣`/`▢` stays about reviewed state alone. Repos libgit2 cannot open -- mercurial, non-colocated jujutsu, a pull request with no matching local checkout -- report nothing as generated rather than failing. PR mode reuses the existing local-checkout resolution, which is only set when the checkout matches the PR's target repository, so a foreign checkout cannot mis-mark files. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 1 + docs/CONFIG.md | 43 ++++ docs/KEYBINDINGS.md | 4 + src/app/annotations.rs | 7 + src/app/diff_load.rs | 8 + src/app/generated.rs | 194 +++++++++++++++ src/app/init.rs | 5 + src/app/mod.rs | 30 ++- src/app/reviewed.rs | 25 +- src/app/tests/generated_tests.rs | 400 +++++++++++++++++++++++++++++++ src/app/tests/mod.rs | 1 + src/config/mod.rs | 200 ++++++++++++++++ src/generated.rs | 170 +++++++++++++ src/handler.rs | 27 +++ src/lib.rs | 1 + src/main.rs | 1 + src/ui/diff_view.rs | 14 +- src/ui/file_list.rs | 26 +- src/ui/help_popup.rs | 28 +++ 19 files changed, 1167 insertions(+), 18 deletions(-) create mode 100644 src/app/generated.rs create mode 100644 src/app/tests/generated_tests.rs create mode 100644 src/generated.rs diff --git a/README.md b/README.md index 1f809cc4..92ae7aa8 100644 --- a/README.md +++ b/README.md @@ -250,6 +250,7 @@ A first-session cheatsheet. Press `?` inside tuicr for the full reference. | `r` | Toggle file reviewed | | `R` | Toggle hunk reviewed | | `e` | Open focused file in `$EDITOR` | +| `Space` | Expand / collapse a generated file | | `y` | Copy review to clipboard | | `:edit` | Open focused file in `$EDITOR` | | `:submit` | Push review to GitHub or GitLab | diff --git a/docs/CONFIG.md b/docs/CONFIG.md index f30fce94..5ca6e1e4 100644 --- a/docs/CONFIG.md +++ b/docs/CONFIG.md @@ -58,6 +58,10 @@ pr_metadata = true comments_header = "## Local tuicr Comments" remote_comments_header = "## Existing GitHub Comments" legend = true + +[generated] +collapse = false +count = true ``` ## Options @@ -269,6 +273,45 @@ The example above produces an export that opens directly on the comment list: The top-level `export_legend` key predates this section and still works. When both are set, `legend` wins. When `[export]` omits `legend`, `export_legend` stays in force, so adding an `[export]` block to trim the intro will not switch the legend back on. +## Generated files + +Files marked as code-generated in `.gitattributes` can be collapsed so a review of hand-written code is not buried under regenerated protobufs, lockfiles, or API clients. + +A file counts as generated when `.gitattributes` sets either `linguist-generated` (GitHub's Linguist) or `gitlab-generated`. Both documented spellings work: + +```gitattributes +*.pb.go linguist-generated=true +schema.generated.ts gitlab-generated +vendor/** linguist-generated=true +``` + +Resolution is delegated to git itself, so nested `.gitattributes` files, `.git/info/attributes`, `core.attributesFile`, and `[attr]` macros all behave the way `git check-attr` does. An explicit opt-out — `-linguist-generated` or `linguist-generated=false` — wins over an opt-in on the other attribute, so a directory-wide rule can be excepted per file. + +```toml +[generated] +collapse = true +count = false +``` + +| Key | Default | Description | +| ---------- | ------- | --------------------------------------------------------------------------------------------------------------------------------- | +| `collapse` | `false` | Hide the diff body of generated files, leaving the header row. `Space` expands one file; `:set generated!` toggles all of them. | +| `count` | `true` | Count generated files toward the `Files · reviewed/total` progress indicator. When `false`, the file list also reports `N generated`. | + +`count` is deliberately independent of `collapse`: expanding a generated file to look at it should not add it to the denominator, or review progress would drop as a side effect of looking. You can also exclude generated files from progress without collapsing them. + +Runtime commands: + +| Command | Effect | +| ------------------- | ------------------------------------------------- | +| `:set generated` | Collapse generated files. | +| `:set nogenerated` | Show generated files. | +| `:set generated!`, `:generated` | Toggle. | + +`.gitattributes` is read from the local worktree, so this needs a git repository: mercurial and non-colocated jujutsu repos report nothing as generated, and in pull request mode the attributes come from your local checkout rather than the pull request's head. Attributes are re-read on reload (`:e`), so editing `.gitattributes` mid-session takes effect without a restart. + +To hide files from a review entirely rather than collapse them, use `.tuicrignore` below. + ## .tuicrignore tuicr reads `.tuicrignore` from the repository root and excludes matching files from all review diffs. Rules follow gitignore-style pattern matching, including `!` negation. diff --git a/docs/KEYBINDINGS.md b/docs/KEYBINDINGS.md index cf628d91..cf7c51ea 100644 --- a/docs/KEYBINDINGS.md +++ b/docs/KEYBINDINGS.md @@ -76,6 +76,7 @@ Shown below the file tree when local comments or visible remote PR threads exist |-----|--------| | `r` | Toggle file reviewed | | `R` | Toggle hunk reviewed | +| `Space` | Expand / collapse the generated file under the cursor (diff panel) | | `c` | Add line comment (or file comment if not on a diff line) | | `C` | Add file comment | | `c` | Add review comment | @@ -143,6 +144,9 @@ In command mode, | `:set commits` | Show inline commit selector | | `:set nocommits` | Hide inline commit selector | | `:set commits!` | Toggle inline commit selector | +| `:set generated` | Collapse files marked generated in `.gitattributes` | +| `:set nogenerated` | Show generated files | +| `:set generated!`, `:generated` | Toggle generated file collapse | | `:clear` | Clear all comments | | `:clearc` | Clear comments without clearing reviewed marks | | `:version` | Show tuicr version | diff --git a/src/app/annotations.rs b/src/app/annotations.rs index 6fdea5af..5d336237 100644 --- a/src/app/annotations.rs +++ b/src/app/annotations.rs @@ -60,6 +60,13 @@ impl App { if self.file_line_count_cache.is_empty() { self.populate_file_line_count_cache(); } + // Which files are code-generated has to be known before the walk + // below decides whose body to emit. Detection is hooked here rather + // than at each of the fifteen `self.diff_files = ...` assignments + // because every one of them rebuilds annotations afterwards, and a + // missed call site would desynchronize the annotations from the + // render. Repeat calls over an unchanged file set do no work. + self.detect_generated_files(); self.line_annotations.clear(); diff --git a/src/app/diff_load.rs b/src/app/diff_load.rs index 52066e03..7cd68e7b 100644 --- a/src/app/diff_load.rs +++ b/src/app/diff_load.rs @@ -581,6 +581,14 @@ impl App { self.diff_files = diff_files; self.clear_expanded_gaps(); + // Reload is the point where an edited `.gitattributes` should take + // effect, so discard the probe memo and re-detect eagerly — the height + // math below runs before `rebuild_annotations` would have done it. + // `expanded_generated` survives: it is the user's override, not + // detected state. + self.invalidate_generated_detection(); + self.detect_generated_files(); + self.sort_files_by_directory(false); self.populate_file_line_count_cache(); self.expand_all_dirs(); diff --git a/src/app/generated.rs b/src/app/generated.rs new file mode 100644 index 00000000..768cd36d --- /dev/null +++ b/src/app/generated.rs @@ -0,0 +1,194 @@ +use super::*; + +impl App { + /// Apply resolved `[generated]` settings and detect matching files. + /// + /// Called from startup after config load, because `App::build` runs before + /// the config is applied and would otherwise detect against the defaults. + pub fn apply_generated_config(&mut self, config: &GeneratedConfig) { + self.collapse_generated = config.collapse(); + self.count_generated = config.count(); + // Annotations were built during `App::build`, before detection could + // run, so any newly collapsed file is not reflected in them yet. + if self.detect_generated_files() { + self.rebuild_annotations(); + } + } + + /// Whether `.gitattributes` has to be consulted at all. + /// + /// Both features off means the detected set stays empty, which is what + /// makes the default configuration free: no repository is opened, and the + /// render path's guards short-circuit before any lookup. + fn generated_detection_enabled(&self) -> bool { + self.collapse_generated || !self.count_generated + } + + /// Repository root to resolve `.gitattributes` against, if there is one. + /// + /// PR mode's `root_path` is the synthetic `forge:host/owner/repo` identity + /// rather than a directory, so it falls back to the local checkout the + /// forge backend resolved — which is only set when the checkout matches + /// the PR's target repository, so a foreign checkout can't mis-mark files. + fn generated_attributes_root(&self) -> Option { + if self.vcs_info.root_path.is_absolute() { + return Some(self.vcs_info.root_path.clone()); + } + self.forge_backend + .as_deref() + .and_then(|backend| backend.local_checkout_path()) + } + + /// Look up `.gitattributes` for every diff file not probed yet, returning + /// whether that grew the generated set. + /// + /// Probed paths are remembered, so a repeat call over an unchanged file + /// set costs one hash lookup per file and no libgit2 work. That memo is + /// also the staleness test, which is what lets `rebuild_annotations` keep + /// detection current without each of the fifteen `diff_files` assignments + /// having to remember to trigger it. + pub(in crate::app) fn detect_generated_files(&mut self) -> bool { + if !self.generated_detection_enabled() { + return false; + } + if self + .diff_files + .iter() + .all(|file| self.generated_probed.contains(file.display_path())) + { + return false; + } + let Some(root) = self.generated_attributes_root() else { + return false; + }; + + let unprobed: Vec = self + .diff_files + .iter() + .map(|file| file.display_path()) + .filter(|path| !self.generated_probed.contains(*path)) + .cloned() + .collect(); + + let detected = crate::profile::time("generated.detect", || { + crate::generated::detect_generated(&root, &unprobed) + }); + + self.generated_probed.extend(unprobed); + let changed = !detected.is_empty(); + self.generated_files.extend(detected); + changed + } + + /// Forget which paths were probed so the next detection re-reads + /// `.gitattributes`. Called on explicit reload, so editing the attributes + /// file takes effect without restarting tuicr. + pub(in crate::app) fn invalidate_generated_detection(&mut self) { + self.generated_probed.clear(); + self.generated_files.clear(); + } + + /// Whether to surface this path as code-generated. + /// + /// Gated on the same condition as detection rather than on the detected + /// set being non-empty, so the decoration is a function of the current + /// settings and not of what the session has detected in the past. The set + /// is deliberately *not* discarded when the feature is switched off — that + /// is what makes re-enabling free — so keying the decoration off it would + /// make `:set generated` / `:set nogenerated` asymmetric: the labels, the + /// dimmed tree rows, and the counter would all survive a toggle back off + /// and never return to how the session started. + #[inline] + pub fn is_generated_file(&self, path: &Path) -> bool { + self.generated_detection_enabled() && self.generated_files.contains(path) + } + + /// Number of diff files surfaced as code-generated. + pub fn generated_file_count(&self) -> usize { + if !self.generated_detection_enabled() { + return 0; + } + self.diff_files + .iter() + .filter(|file| self.generated_files.contains(file.display_path())) + .count() + } + + /// `(reviewed, total)` file counts for the review progress indicator. + /// + /// Generated files drop out of both halves when `[generated] count` is + /// off — out of the numerator too, or marking one reviewed would push the + /// count past its own total. + pub fn review_progress(&self) -> (usize, usize) { + if self.count_generated || self.generated_files.is_empty() { + return (self.reviewed_count(), self.file_count()); + } + let mut reviewed = 0; + let mut total = 0; + for file in &self.diff_files { + let path = file.display_path(); + if self.generated_files.contains(path) { + continue; + } + total += 1; + if self.session.is_file_reviewed(path) { + reviewed += 1; + } + } + (reviewed, total) + } + + /// Toggle the collapsed state of the generated file under the cursor. + /// + /// Keyed by path rather than index so the override survives the reloads + /// and commit-selection changes that replace `diff_files` wholesale. + /// + /// Requires collapse to be on, not merely that the file is generated: + /// with `count = false` alone nothing is hidden, so claiming `Space` there + /// would report an expansion that changed nothing on screen. + pub fn toggle_generated_expansion(&mut self) -> bool { + if !self.collapse_generated { + return false; + } + let Some(path) = self + .current_file() + .map(|file| file.display_path().clone()) + .filter(|path| self.is_generated_file(path)) + else { + return false; + }; + + if self.expanded_generated.remove(&path) { + self.set_message("Generated file collapsed"); + } else { + self.expanded_generated.insert(path); + self.set_message("Generated file expanded"); + } + self.rebuild_annotations(); + let file_idx = self.diff_state.current_file_idx; + self.diff_state.cursor_line = self.calculate_file_scroll_offset(file_idx); + self.ensure_cursor_visible(); + true + } + + /// Set the runtime collapse toggle, running detection if it was skipped + /// while both features were off. + pub fn set_collapse_generated(&mut self, collapse: bool) { + self.collapse_generated = collapse; + self.detect_generated_files(); + self.rebuild_annotations(); + + let count = self.generated_file_count(); + if !collapse { + self.set_message("Showing generated files"); + } else if count == 0 { + self.set_warning("No files marked generated in .gitattributes"); + } else { + self.set_message(format!("Collapsing {count} generated file(s)")); + } + } + + pub fn toggle_collapse_generated(&mut self) { + self.set_collapse_generated(!self.collapse_generated); + } +} diff --git a/src/app/init.rs b/src/app/init.rs index c6ccd979..60e54635 100644 --- a/src/app/init.rs +++ b/src/app/init.rs @@ -559,6 +559,11 @@ impl App { saved_inline_selection: None, path_filter: path_filter.map(|s| s.to_string()), export: ExportConfig::default(), + collapse_generated: false, + count_generated: true, + generated_files: HashSet::new(), + expanded_generated: HashSet::new(), + generated_probed: HashSet::new(), }; // Auto-hide file list when path filter matches exactly one file if app.path_filter.is_some() && app.diff_files.len() == 1 { diff --git a/src/app/mod.rs b/src/app/mod.rs index c088c57e..18d48d07 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -6,7 +6,7 @@ use chrono::Utc; use ratatui::style::Color; use crate::comment_vim::CommentVimEditor; -use crate::config::{CommentTypeConfig, ExportConfig}; +use crate::config::{CommentTypeConfig, ExportConfig, GeneratedConfig}; use crate::editor::EditorTarget; use crate::error::{Result, TuicrError}; use crate::forge::context::{ContextProvider, ForgeContextProvider, VcsContextProvider}; @@ -1289,6 +1289,33 @@ pub struct App { pub path_filter: Option, /// Resolved `[export]` settings shaping the generated review markdown. pub export: ExportConfig, + /// Whether files `.gitattributes` marks as code-generated have their diff + /// body hidden. Seeded from `[generated] collapse`, flipped at runtime by + /// `:set generated!`. Off by default, so tuicr ships behaving as before. + pub collapse_generated: bool, + /// Whether generated files count toward review progress. Seeded from + /// `[generated] count`, which defaults to true. Independent of + /// `collapse_generated`: expanding a generated file to peek at it must + /// not add it to the denominator. + pub count_generated: bool, + /// Diff files that `.gitattributes` marks as code-generated, keyed by + /// display path. + /// + /// Path-keyed rather than index-keyed because reloads, watch ticks, and + /// commit-selection changes all replace `diff_files` wholesale and shift + /// every index. Empty whenever detection is disabled, which is what lets + /// the render path short-circuit for free. + pub generated_files: HashSet, + /// Generated files the user expanded with `Space`, overriding collapse. + pub expanded_generated: HashSet, + /// Display paths already looked up in `.gitattributes`. + /// + /// Doubles as the staleness test for detection: a diff file set with no + /// unprobed path needs no work, which is how `rebuild_annotations` can + /// keep detection current without every `diff_files` assignment having to + /// remember to trigger it. Cleared on explicit reload so an edited + /// `.gitattributes` takes effect. + pub generated_probed: HashSet, } #[derive(Debug, Clone, PartialEq, Eq)] @@ -1503,6 +1530,7 @@ mod comments; mod commits; mod diff_load; mod gaps; +mod generated; mod init; mod modes; mod navigation; diff --git a/src/app/reviewed.rs b/src/app/reviewed.rs index 04919113..2372d5bc 100644 --- a/src/app/reviewed.rs +++ b/src/app/reviewed.rs @@ -275,8 +275,10 @@ impl App { /// Whether a file's diff body is hidden, leaving only its header row. /// - /// Reviewed files collapse so a long review stream shrinks as it is - /// worked through. The annotation builder, both diff renderers, and the + /// Two reasons a body hides. Reviewed files collapse so a long review + /// stream shrinks as it is worked through. Files `.gitattributes` marks + /// as code-generated collapse when the user opted in, until `Space` + /// expands one. The annotation builder, both diff renderers, and the /// scroll-height math all have to agree on this or the cursor lands on /// rows that aren't drawn, so they share this one predicate. /// @@ -285,14 +287,21 @@ impl App { /// honor it), so each keeps its own gate rather than having one folded /// in here. /// - /// Both renderers and `hunk_positions` call this once per file per - /// frame, so it has to stay cheap — currently a single map lookup. - /// Callers reach it through a `!is_single_file_view` branch that is - /// exclusive with the reviewed-banner branch, so no site pays for two - /// lookups to decide one thing. + /// Both renderers and `hunk_positions` call this once per file per frame, + /// so it has to stay cheap. The checks are ordered so the default + /// configuration still costs exactly one map lookup: `collapse_generated` + /// is false unless the user opted in, and a bare bool test short-circuits + /// the rest. Opting in adds one hash lookup per non-reviewed file, and a + /// second only for the files that are actually generated. #[inline] pub fn is_file_collapsed(&self, file: &DiffFile) -> bool { - self.session.is_file_reviewed(file.display_path()) + let path = file.display_path(); + if self.session.is_file_reviewed(path) { + return true; + } + self.collapse_generated + && self.generated_files.contains(path) + && !self.expanded_generated.contains(path) } pub fn file_count(&self) -> usize { diff --git a/src/app/tests/generated_tests.rs b/src/app/tests/generated_tests.rs new file mode 100644 index 00000000..400e37f4 --- /dev/null +++ b/src/app/tests/generated_tests.rs @@ -0,0 +1,400 @@ +use std::fs; +use std::path::{Path, PathBuf}; + +use tempfile::{TempDir, tempdir}; + +use crate::app::*; +use crate::config::GeneratedConfig; +use crate::model::{DiffFile, DiffHunk, DiffLine, FileStatus, LineOrigin}; +use crate::vcs::traits::{VcsBackend, VcsInfo, VcsType}; + +struct StubVcs(VcsInfo); +impl VcsBackend for StubVcs { + fn info(&self) -> &VcsInfo { + &self.0 + } + fn get_working_tree_diff( + &self, + _hl: &crate::syntax::SyntaxHighlighter, + ) -> crate::error::Result> { + Ok(Vec::new()) + } + fn fetch_context_lines( + &self, + _path: &Path, + _status: FileStatus, + _ref_commit: Option<&str>, + _start: u32, + _end: u32, + ) -> crate::error::Result> { + Ok(Vec::new()) + } + fn file_line_count( + &self, + _path: &Path, + _status: FileStatus, + _ref_commit: Option<&str>, + ) -> crate::error::Result { + Ok(0) + } +} + +fn hunk() -> DiffHunk { + let lines = (1..=3) + .map(|i| DiffLine { + origin: LineOrigin::Context, + content: format!("line {i}"), + old_lineno: Some(i), + new_lineno: Some(i), + highlighted_spans: None, + }) + .collect(); + DiffHunk { + header: "@@ -1,3 +1,3 @@".to_string(), + lines, + old_start: 1, + old_count: 3, + new_start: 1, + new_count: 3, + } +} + +fn file(path: &str) -> DiffFile { + let hunks = vec![hunk()]; + let content_hash = DiffFile::compute_content_hash(&hunks); + DiffFile { + old_path: None, + new_path: Some(PathBuf::from(path)), + status: FileStatus::Modified, + hunks, + is_binary: false, + is_too_large: false, + is_commit_message: false, + content_hash, + } +} + +/// A git repository whose root `.gitattributes` marks `api.pb.go` generated. +fn repo() -> TempDir { + let dir = tempdir().expect("tempdir"); + git2::Repository::init(dir.path()).expect("init repo"); + fs::write( + dir.path().join(".gitattributes"), + "*.pb.go linguist-generated=true\n", + ) + .expect("write .gitattributes"); + dir +} + +fn app_in(dir: &TempDir, paths: &[&str]) -> App { + let vcs_info = VcsInfo { + root_path: dir.path().to_path_buf(), + head_commit: "head".into(), + branch_name: Some("main".into()), + vcs_type: VcsType::Git, + }; + let session = ReviewSession::new( + vcs_info.root_path.clone(), + vcs_info.head_commit.clone(), + vcs_info.branch_name.clone(), + SessionDiffSource::WorkingTree, + ); + App::build( + Box::new(StubVcs(vcs_info.clone())), + vcs_info, + crate::theme::Theme::dark(), + None, + false, + paths.iter().copied().map(file).collect(), + session, + DiffSource::WorkingTree, + InputMode::Normal, + Vec::new(), + None, + None, + ) + .expect("build app") +} + +fn collapsing_app(dir: &TempDir, paths: &[&str]) -> App { + let mut app = app_in(dir, paths); + app.apply_generated_config(&GeneratedConfig { + collapse: Some(true), + count: None, + }); + app +} + +fn file_named(app: &App, path: &str) -> DiffFile { + app.diff_files + .iter() + .find(|f| f.display_path() == Path::new(path)) + .expect("file in diff set") + .clone() +} + +#[test] +fn default_config_never_reads_gitattributes() { + // Both features default off, so detection must not run at all — not even + // to open the repository. An empty probe set is the observable proof. + let dir = repo(); + let app = app_in(&dir, &["api.pb.go", "src/main.rs"]); + + assert!(app.generated_probed.is_empty()); + assert!(app.generated_files.is_empty()); + assert!(!app.is_file_collapsed(&file_named(&app, "api.pb.go"))); +} + +#[test] +fn collapse_hides_generated_files_only() { + let dir = repo(); + let app = collapsing_app(&dir, &["api.pb.go", "src/main.rs"]); + + assert!(app.is_generated_file(Path::new("api.pb.go"))); + assert!(!app.is_generated_file(Path::new("src/main.rs"))); + assert!(app.is_file_collapsed(&file_named(&app, "api.pb.go"))); + assert!(!app.is_file_collapsed(&file_named(&app, "src/main.rs"))); +} + +#[test] +fn space_expansion_overrides_collapse() { + let dir = repo(); + let mut app = collapsing_app(&dir, &["api.pb.go"]); + app.diff_state.current_file_idx = 0; + + assert!(app.toggle_generated_expansion()); + assert!(!app.is_file_collapsed(&file_named(&app, "api.pb.go"))); + + assert!(app.toggle_generated_expansion()); + assert!(app.is_file_collapsed(&file_named(&app, "api.pb.go"))); +} + +#[test] +fn space_is_inert_on_a_file_that_is_not_generated() { + // The diff panel's Space handler falls through to the shared action when + // this returns false, so it must not claim non-generated files. + let dir = repo(); + let mut app = collapsing_app(&dir, &["src/main.rs"]); + app.diff_state.current_file_idx = 0; + + assert!(!app.toggle_generated_expansion()); +} + +#[test] +fn turning_collapse_off_stops_collapsing_already_detected_files() { + // The detected set is not cleared when the feature is switched off, so + // the predicate has to gate on the runtime flag rather than on the set + // being empty. + let dir = repo(); + let mut app = collapsing_app(&dir, &["api.pb.go"]); + assert!(app.is_file_collapsed(&file_named(&app, "api.pb.go"))); + + app.toggle_collapse_generated(); + + assert!(!app.generated_files.is_empty()); + assert!(!app.is_file_collapsed(&file_named(&app, "api.pb.go"))); +} + +#[test] +fn toggling_collapse_off_returns_to_the_starting_appearance() { + // The detected set is kept when the feature is switched off so that + // re-enabling is free, which means the decoration must not be keyed off + // it: `:generated` twice from a default start has to land back exactly + // where it began, labels and counter included. + let dir = repo(); + let mut app = app_in(&dir, &["api.pb.go", "src/main.rs"]); + + let initial = ( + app.is_generated_file(Path::new("api.pb.go")), + app.generated_file_count(), + app.review_progress(), + app.is_file_collapsed(&file_named(&app, "api.pb.go")), + ); + assert_eq!(initial, (false, 0, (0, 2), false)); + + app.toggle_collapse_generated(); + assert_eq!( + ( + app.is_generated_file(Path::new("api.pb.go")), + app.generated_file_count(), + app.review_progress(), + app.is_file_collapsed(&file_named(&app, "api.pb.go")), + ), + (true, 1, (0, 2), true) + ); + + app.toggle_collapse_generated(); + assert_eq!( + ( + app.is_generated_file(Path::new("api.pb.go")), + app.generated_file_count(), + app.review_progress(), + app.is_file_collapsed(&file_named(&app, "api.pb.go")), + ), + initial, + "toggling collapse off must restore the starting appearance" + ); + // The set itself is retained, so re-enabling costs no libgit2 work. + assert!(!app.generated_files.is_empty()); +} + +#[test] +fn count_exclusion_alone_still_labels_without_claiming_space() { + // `count = false, collapse = false` is a legitimate configuration: the + // files are surfaced and dropped from progress, but nothing is hidden, so + // there is nothing for `Space` to expand. + let dir = repo(); + let mut app = app_in(&dir, &["api.pb.go", "src/main.rs"]); + app.apply_generated_config(&GeneratedConfig { + collapse: None, + count: Some(false), + }); + app.diff_state.current_file_idx = app + .diff_files + .iter() + .position(|f| f.display_path() == Path::new("api.pb.go")) + .expect("generated file"); + + assert!(app.is_generated_file(Path::new("api.pb.go"))); + assert_eq!(app.generated_file_count(), 1); + assert!(!app.is_file_collapsed(&file_named(&app, "api.pb.go"))); + assert!(!app.toggle_generated_expansion()); +} + +#[test] +fn expansion_survives_a_diff_file_rebuild_that_shifts_indices() { + // Reloads, watch ticks, and commit-selection changes all replace + // `diff_files` wholesale. State keyed by index would follow the wrong + // file afterwards. + let dir = repo(); + let mut app = collapsing_app(&dir, &["api.pb.go"]); + app.diff_state.current_file_idx = 0; + app.toggle_generated_expansion(); + + app.diff_files = vec![file("src/main.rs"), file("api.pb.go")]; + app.rebuild_annotations(); + + assert_eq!( + app.diff_files[1].display_path(), + &PathBuf::from("api.pb.go") + ); + assert!(!app.is_file_collapsed(&file_named(&app, "api.pb.go"))); +} + +#[test] +fn detection_runs_for_count_exclusion_without_collapse() { + // `count` is deliberately not gated by `collapse`: excluding generated + // files from review progress is useful on its own. + let dir = repo(); + let mut app = app_in(&dir, &["api.pb.go", "src/main.rs"]); + app.apply_generated_config(&GeneratedConfig { + collapse: None, + count: Some(false), + }); + + assert_eq!(app.generated_file_count(), 1); + assert!(!app.is_file_collapsed(&file_named(&app, "api.pb.go"))); + assert_eq!(app.review_progress(), (0, 1)); +} + +#[test] +fn progress_counts_generated_files_by_default() { + let dir = repo(); + let app = collapsing_app(&dir, &["api.pb.go", "src/main.rs"]); + + assert_eq!(app.generated_file_count(), 1); + assert_eq!(app.review_progress(), (0, 2)); +} + +#[test] +fn reviewing_an_excluded_generated_file_cannot_overrun_the_total() { + // Both halves of the fraction have to drop the file, or marking it + // reviewed would report 2/1. + let dir = repo(); + let mut app = app_in(&dir, &["api.pb.go", "src/main.rs"]); + app.apply_generated_config(&GeneratedConfig { + collapse: None, + count: Some(false), + }); + + let generated_idx = app + .diff_files + .iter() + .position(|f| f.display_path() == Path::new("api.pb.go")) + .expect("generated file"); + app.toggle_reviewed_for_file_idx(generated_idx, false); + + assert_eq!(app.review_progress(), (0, 1)); +} + +#[test] +fn invalidation_re_reads_edited_gitattributes() { + let dir = repo(); + let mut app = collapsing_app(&dir, &["api.pb.go"]); + assert!(app.is_file_collapsed(&file_named(&app, "api.pb.go"))); + + fs::write( + dir.path().join(".gitattributes"), + "*.pb.go -linguist-generated\n", + ) + .expect("rewrite .gitattributes"); + app.invalidate_generated_detection(); + app.rebuild_annotations(); + + assert!(!app.is_generated_file(Path::new("api.pb.go"))); + assert!(!app.is_file_collapsed(&file_named(&app, "api.pb.go"))); +} + +#[test] +fn set_generated_commands_drive_the_runtime_toggle() { + let dir = repo(); + let mut app = app_in(&dir, &["api.pb.go"]); + + for (command, expected) in [ + ("set generated", true), + ("set nogenerated", false), + ("set generated!", true), + ("generated", false), + ] { + app.enter_command_mode(); + app.command_buffer = command.to_string(); + crate::handler::handle_command_action(&mut app, crate::input::Action::SubmitInput); + + assert_eq!( + app.collapse_generated, expected, + "`:{command}` should leave collapse={expected}" + ); + assert_eq!(app.input_mode, InputMode::Normal); + } + // The commands have to work from the `collapse = false` default, or the + // opt-in default would be intolerable: the very first `:set generated` + // must trigger detection that startup skipped. + assert!(app.generated_files.contains(Path::new("api.pb.go"))); +} + +#[test] +fn space_in_the_diff_panel_expands_the_generated_file_under_the_cursor() { + let dir = repo(); + let mut app = collapsing_app(&dir, &["api.pb.go"]); + app.focused_panel = FocusedPanel::Diff; + app.diff_state.current_file_idx = 0; + + crate::handler::handle_diff_action(&mut app, crate::input::Action::ToggleExpand); + + assert!(!app.is_file_collapsed(&file_named(&app, "api.pb.go"))); +} + +#[test] +fn detection_is_skipped_when_there_is_no_local_repository_root() { + // PR mode's root is the synthetic `forge:host/owner/repo` identity, and + // without a matching local checkout there is nothing to read attributes + // from. It must degrade to "nothing is generated", not panic. + let dir = repo(); + let mut app = collapsing_app(&dir, &["api.pb.go"]); + app.vcs_info.root_path = PathBuf::from("forge:github.com/agavra/tuicr"); + app.invalidate_generated_detection(); + app.rebuild_annotations(); + + assert!(app.generated_files.is_empty()); + assert!(!app.is_file_collapsed(&file_named(&app, "api.pb.go"))); +} diff --git a/src/app/tests/mod.rs b/src/app/tests/mod.rs index cb97dafc..d6455301 100644 --- a/src/app/tests/mod.rs +++ b/src/app/tests/mod.rs @@ -5,6 +5,7 @@ mod decoration_skip_tests; mod diff_source_tests; mod expand_gap_tests; mod find_source_line_tests; +mod generated_tests; mod persistence_merge_tests; mod pr_info_tests; mod scroll_behavior_tests; diff --git a/src/config/mod.rs b/src/config/mod.rs index bb49fca9..40763546 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -94,6 +94,36 @@ impl ExportConfig { } } +/// `[generated]` section settings for files that `.gitattributes` marks as +/// code-generated (`linguist-generated` or `gitlab-generated`). +/// +/// Both fields are optional so the shipped defaults stay distinguishable from +/// an explicit setting, matching `[export]`. The defaults reproduce tuicr's +/// behavior before generated-file support existed, so an absent section is +/// exactly today's behavior — and skips attribute detection entirely. +#[derive(Debug, Clone, Default, Deserialize, Serialize, PartialEq, Eq)] +#[serde(default)] +pub struct GeneratedConfig { + /// Whether to hide the diff body of generated files, leaving the header. + /// `Space` still expands an individual file. + pub collapse: Option, + /// Whether generated files count toward the reviewed/total progress + /// indicator. Deliberately independent of `collapse`: expanding a + /// generated file to peek at it must not add it to the denominator, or + /// review progress would regress as a side effect of looking. + pub count: Option, +} + +impl GeneratedConfig { + pub fn collapse(&self) -> bool { + self.collapse.unwrap_or(false) + } + + pub fn count(&self) -> bool { + self.count.unwrap_or(true) + } +} + #[derive(Debug, Clone, Default, Deserialize, Serialize, PartialEq, Eq)] #[serde(default)] pub struct AppConfig { @@ -146,6 +176,9 @@ pub struct AppConfig { /// `[export]` section settings. `None` means "no override"; downstream /// code should treat it as `ExportConfig::default()`. pub export: Option, + /// `[generated]` section settings. `None` means "no override"; downstream + /// code should treat it as `GeneratedConfig::default()`. + pub generated: Option, } impl AppConfig { @@ -193,6 +226,7 @@ const KNOWN_KEYS: &[&str] = &[ "username", "forge", "export", + "generated", ]; const FORGE_KNOWN_KEYS: &[&str] = &["comment_type_prefix"]; @@ -206,6 +240,8 @@ const EXPORT_KNOWN_KEYS: &[&str] = &[ "legend", ]; +const GENERATED_KNOWN_KEYS: &[&str] = &["collapse", "count"]; + #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct ConfigLoadOutcome { pub config: Option, @@ -431,6 +467,9 @@ fn load_config_from_path(path: &Path) -> Result { export: table .get("export") .and_then(|v| parse_export(v, &mut warnings)), + generated: table + .get("generated") + .and_then(|v| parse_generated(v, &mut warnings)), }; for key in table.keys() { @@ -512,6 +551,36 @@ fn parse_export(value: &Value, warnings: &mut Vec) -> Option) -> Option { + let Some(table) = value.as_table() else { + warnings + .push("Warning: Config key 'generated' must be a table; ignoring value".to_string()); + return None; + }; + + for key in table.keys() { + if !GENERATED_KNOWN_KEYS.contains(&key.as_str()) { + warnings.push(format!( + "Warning: Unknown config key 'generated.{key}', ignoring" + )); + } + } + + let cfg = GeneratedConfig { + collapse: read_section_bool(table, "generated", "collapse", warnings), + count: read_section_bool(table, "generated", "count", warnings), + }; + + if cfg == GeneratedConfig::default() { + None + } else { + Some(cfg) + } +} + /// Like `read_bool`, but emits a `
.` qualified warning so the /// user can locate the misconfigured field. fn read_section_bool( @@ -1651,6 +1720,137 @@ scope_line = "no" ); } + // generated + + #[test] + fn generated_accessors_reproduce_pre_feature_behavior() { + // Both defaults are load-bearing: they are what makes an absent + // `[generated]` section behave exactly as tuicr did before generated + // files were recognized, and skip attribute detection entirely. + let cfg = GeneratedConfig::default(); + assert!(!cfg.collapse()); + assert!(cfg.count()); + } + + #[test] + fn should_default_generated_to_none_when_section_missing() { + let outcome = parse_config(""); + assert_eq!( + outcome + .config + .as_ref() + .and_then(|cfg| cfg.generated.clone()), + None + ); + assert!(outcome.warnings.is_empty()); + } + + #[test] + fn should_default_generated_to_none_when_section_is_empty_table() { + let outcome = parse_config("[generated]\n"); + assert_eq!( + outcome + .config + .as_ref() + .and_then(|cfg| cfg.generated.clone()), + None + ); + assert!(outcome.warnings.is_empty()); + } + + #[test] + fn should_parse_generated_section_overriding_defaults() { + let outcome = parse_config( + r#"[generated] +collapse = true +count = false +"#, + ); + let generated = outcome + .config + .as_ref() + .and_then(|cfg| cfg.generated.clone()) + .expect("generated section should parse"); + assert!(generated.collapse()); + assert!(!generated.count()); + assert!(outcome.warnings.is_empty()); + } + + #[test] + fn should_leave_unset_generated_keys_as_none() { + let outcome = parse_config( + r#"[generated] +collapse = true +"#, + ); + let generated = outcome + .config + .as_ref() + .and_then(|cfg| cfg.generated.clone()) + .expect("generated section should parse"); + assert_eq!(generated.count, None); + assert!(generated.count()); + } + + #[test] + fn should_warn_on_unknown_generated_keys() { + let outcome = parse_config( + r#"[generated] +collapse = true +patterns = ["*.pb.go"] +"#, + ); + let generated = outcome + .config + .as_ref() + .and_then(|cfg| cfg.generated.clone()) + .expect("generated section should parse"); + assert!(generated.collapse()); + assert_eq!( + outcome.warnings, + vec!["Warning: Unknown config key 'generated.patterns', ignoring".to_string()] + ); + } + + #[test] + fn should_warn_and_ignore_generated_bool_with_invalid_type() { + let outcome = parse_config( + r#"[generated] +collapse = "yes" +"#, + ); + assert_eq!( + outcome + .config + .as_ref() + .and_then(|cfg| cfg.generated.clone()), + None + ); + assert_eq!( + outcome.warnings, + vec![ + "Warning: Config key 'generated.collapse' must be a boolean; ignoring value" + .to_string() + ] + ); + } + + #[test] + fn should_warn_when_generated_is_not_a_table() { + let outcome = parse_config("generated = true\n"); + assert_eq!( + outcome + .config + .as_ref() + .and_then(|cfg| cfg.generated.clone()), + None + ); + assert_eq!( + outcome.warnings, + vec!["Warning: Config key 'generated' must be a table; ignoring value".to_string()] + ); + } + // resolved export precedence #[test] diff --git a/src/generated.rs b/src/generated.rs new file mode 100644 index 00000000..aa84fc61 --- /dev/null +++ b/src/generated.rs @@ -0,0 +1,170 @@ +use std::collections::HashSet; +use std::path::{Path, PathBuf}; + +use git2::{AttrCheckFlags, AttrValue, Repository}; + +/// Attributes that mark a file as code-generated. GitHub's Linguist and +/// GitLab each define their own name; a file marked by either counts. +const GENERATED_ATTRS: [&str; 2] = ["linguist-generated", "gitlab-generated"]; + +/// Which of `paths` the repository's gitattributes mark as code-generated. +/// +/// Delegating to libgit2 rather than parsing `.gitattributes` ourselves means +/// nested `.gitattributes`, `.git/info/attributes`, `core.attributesFile`, and +/// `[attr]` macros all resolve the way git resolves them, for free. +/// +/// Returns an empty set — never an error — when there is no git repository to +/// ask: mercurial, non-colocated jujutsu, and pull requests without a local +/// checkout all land here, and none of them should block a review. +pub fn detect_generated(repo_root: &Path, paths: &[PathBuf]) -> HashSet { + let mut generated = HashSet::new(); + if paths.is_empty() { + return generated; + } + let Ok(repo) = Repository::discover(repo_root) else { + return generated; + }; + for path in paths { + if is_generated(&repo, path) { + generated.insert(path.clone()); + } + } + generated +} + +/// Whether either generated attribute is affirmatively set for `path`. +/// +/// An explicit opt-out on *either* attribute beats an opt-in on the other, and +/// a value we don't recognize counts as "not generated". Both rules fail toward +/// showing the diff: a needlessly shown diff is an annoyance, a needlessly +/// hidden one is unreviewed code. +fn is_generated(repo: &Repository, path: &Path) -> bool { + let mut opted_in = false; + for name in GENERATED_ATTRS { + let Ok(raw) = repo.get_attr(path, name, AttrCheckFlags::FILE_THEN_INDEX) else { + continue; + }; + // `get_attr` returns a sentinel string for a set-but-valueless + // attribute, so the raw value has to be interpreted through + // `AttrValue` rather than compared directly. Getting this wrong would + // read GitLab's bare `gitlab-generated` and its `-gitlab-generated` + // opt-out as the same thing. + match AttrValue::from_string(raw) { + // `attr` (GitLab's documented form) or `attr=true` (GitHub's). + AttrValue::True | AttrValue::String("true") => opted_in = true, + // `-attr` or `attr=false`. + AttrValue::False | AttrValue::String("false") => return false, + // `!attr`, absent, or a value neither forge documents. + _ => {} + } + } + opted_in +} + +#[cfg(test)] +mod tests { + use std::fs; + + use tempfile::{TempDir, tempdir}; + + use super::*; + + /// A git repository with the given `.gitattributes` content at its root. + fn repo_with_attributes(contents: &str) -> TempDir { + let dir = tempdir().expect("failed to create temp dir"); + Repository::init(dir.path()).expect("failed to init repo"); + fs::write(dir.path().join(".gitattributes"), contents) + .expect("failed to write .gitattributes"); + dir + } + + fn detect(dir: &TempDir, paths: &[&str]) -> HashSet { + let paths: Vec = paths.iter().map(PathBuf::from).collect(); + detect_generated(dir.path(), &paths) + } + + #[test] + fn detects_github_and_gitlab_attribute_forms() { + // `attr=true` yields `AttrValue::String("true")` while bare `attr` + // yields `AttrValue::True`. Both mean generated, and a naive + // `is_some()` check on the raw value would conflate them with the + // opt-out forms below. + let dir = repo_with_attributes( + "github.pb.go linguist-generated=true\ngitlab.pb.go gitlab-generated\n", + ); + + let generated = detect(&dir, &["github.pb.go", "gitlab.pb.go", "src/main.rs"]); + + assert_eq!( + generated, + HashSet::from([PathBuf::from("github.pb.go"), PathBuf::from("gitlab.pb.go"),]) + ); + } + + #[test] + fn treats_explicit_opt_out_forms_as_not_generated() { + let dir = repo_with_attributes( + "dash.pb.go -linguist-generated\nvalue.pb.go linguist-generated=false\nbang.pb.go !linguist-generated\n", + ); + + let generated = detect(&dir, &["dash.pb.go", "value.pb.go", "bang.pb.go"]); + + assert!(generated.is_empty(), "unexpected: {generated:?}"); + } + + #[test] + fn ignores_values_neither_forge_documents() { + let dir = repo_with_attributes("weird.pb.go linguist-generated=maybe\n"); + + assert!(detect(&dir, &["weird.pb.go"]).is_empty()); + } + + #[test] + fn opt_out_on_either_attribute_beats_opt_in_on_the_other() { + let dir = repo_with_attributes( + "a.pb.go linguist-generated=true -gitlab-generated\nb.pb.go -linguist-generated gitlab-generated\n", + ); + + let generated = detect(&dir, &["a.pb.go", "b.pb.go"]); + + assert!(generated.is_empty(), "unexpected: {generated:?}"); + } + + #[test] + fn resolves_nested_gitattributes() { + // Proves libgit2 is doing the attribute resolution rather than us + // reading only the root file. + let dir = repo_with_attributes(""); + let nested = dir.path().join("proto"); + fs::create_dir(&nested).expect("failed to create nested dir"); + fs::write( + nested.join(".gitattributes"), + "*.pb.go linguist-generated=true\n", + ) + .expect("failed to write nested .gitattributes"); + + let generated = detect(&dir, &["proto/api.pb.go", "root.pb.go"]); + + assert_eq!(generated, HashSet::from([PathBuf::from("proto/api.pb.go")])); + } + + #[test] + fn returns_empty_set_outside_a_git_repository() { + let dir = tempdir().expect("failed to create temp dir"); + fs::write( + dir.path().join(".gitattributes"), + "a.pb.go linguist-generated\n", + ) + .expect("failed to write .gitattributes"); + let paths = vec![PathBuf::from("a.pb.go")]; + + assert!(detect_generated(dir.path(), &paths).is_empty()); + } + + #[test] + fn returns_empty_set_without_opening_a_repository_for_no_paths() { + let dir = repo_with_attributes("*.pb.go linguist-generated=true\n"); + + assert!(detect_generated(dir.path(), &[]).is_empty()); + } +} diff --git a/src/handler.rs b/src/handler.rs index 2cf015e5..168cc96c 100644 --- a/src/handler.rs +++ b/src/handler.rs @@ -45,6 +45,15 @@ const COMMAND_SPECS: &[CommandSpec] = &[ CommandSpec::new(&["set commits"], CommandKind::SetCommitsVisible(true)), CommandSpec::new(&["set nocommits"], CommandKind::SetCommitsVisible(false)), CommandSpec::new(&["set commits!"], CommandKind::ToggleCommits), + CommandSpec::new(&["set generated"], CommandKind::SetGeneratedCollapsed(true)), + CommandSpec::new( + &["set nogenerated"], + CommandKind::SetGeneratedCollapsed(false), + ), + CommandSpec::new( + &["generated", "set generated!"], + CommandKind::ToggleGenerated, + ), CommandSpec::new(&["diff"], CommandKind::Diff), CommandSpec::new(&["focus", "f"], CommandKind::Focus), CommandSpec::new(&["stage"], CommandKind::Stage), @@ -117,6 +126,8 @@ enum CommandKind { SetVim(bool), SetCommitsVisible(bool), ToggleCommits, + SetGeneratedCollapsed(bool), + ToggleGenerated, Diff, Focus, Stage, @@ -810,6 +821,14 @@ fn dispatch_command(app: &mut App, kind: CommandKind) -> CommandAfterDispatch { app.toggle_commit_selector(); CommandAfterDispatch::ExitCommandMode } + CommandKind::SetGeneratedCollapsed(collapse) => { + app.set_collapse_generated(collapse); + CommandAfterDispatch::ExitCommandMode + } + CommandKind::ToggleGenerated => { + app.toggle_collapse_generated(); + CommandAfterDispatch::ExitCommandMode + } CommandKind::Diff => { app.toggle_diff_view_mode(); CommandAfterDispatch::ExitCommandMode @@ -1398,6 +1417,14 @@ pub fn handle_diff_action(app: &mut App, action: Action) { } } } + // Space already means "expand what's under the cursor" in the file + // list and the commit selector; in the diff panel it was unbound, so + // it takes on the same meaning for a collapsed generated file. + Action::ToggleExpand => { + if !app.toggle_generated_expansion() { + handle_shared_normal_action(app, action); + } + } Action::SelectFileFull => { if let Some(hit) = app.get_gap_at_cursor() { match hit { diff --git a/src/lib.rs b/src/lib.rs index 94483a9b..1fdcf4b3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -5,6 +5,7 @@ pub mod config; pub mod editor; pub mod error; pub mod forge; +pub mod generated; pub mod handler; pub mod hash; pub mod input; diff --git a/src/main.rs b/src/main.rs index 2190b723..d001cf70 100644 --- a/src/main.rs +++ b/src/main.rs @@ -303,6 +303,7 @@ fn main() -> anyhow::Result<()> { app.toggle_single_file_view(); } app.export = cfg.resolved_export(); + app.apply_generated_config(&cfg.generated.clone().unwrap_or_default()); if cfg.cursor_line == Some(false) { app.cursor_line_highlight = false; } diff --git a/src/ui/diff_view.rs b/src/ui/diff_view.rs index 15ef48dd..b429137f 100644 --- a/src/ui/diff_view.rs +++ b/src/ui/diff_view.rs @@ -30,14 +30,22 @@ pub(super) fn file_header_prefix_text(app: &App, file: &DiffFile) -> String { let path = file.display_path(); let is_reviewed = app.session.is_file_reviewed(path); let review_mark = if is_reviewed { "✓ " } else { "" }; + // Shown whether or not the body is collapsed: once a generated file is + // expanded the label is the only thing left saying why it was hidden. + let generated_mark = if app.is_generated_file(path) { + " [generated]" + } else { + "" + }; if file.is_commit_message || app.is_pristine_mode { - format!("═══ {}{} ", review_mark, path.display()) + format!("═══ {}{}{} ", review_mark, path.display(), generated_mark) } else { format!( - "═══ {}{} [{}] ", + "═══ {}{} [{}]{} ", review_mark, path.display(), - file.status.as_char() + file.status.as_char(), + generated_mark ) } } diff --git a/src/ui/file_list.rs b/src/ui/file_list.rs index 6a605ed6..2b1003b4 100644 --- a/src/ui/file_list.rs +++ b/src/ui/file_list.rs @@ -19,11 +19,13 @@ const UNREVIEWED_BOX: &str = "\u{25a2}"; // ▢ pub(super) fn render_file_list(frame: &mut Frame, app: &mut App, area: Rect) { let focused = app.focused_panel == FocusedPanel::FileList; - let title = format!( - " Files \u{00b7} {}/{} ", - app.reviewed_count(), - app.file_count() - ); + let (reviewed, total) = app.review_progress(); + let generated = app.generated_file_count(); + let title = if generated == 0 { + format!(" Files \u{00b7} {reviewed}/{total} ") + } else { + format!(" Files \u{00b7} {reviewed}/{total} \u{00b7} {generated} generated ") + }; let block = Block::default() .title(title) .borders(Borders::ALL) @@ -148,7 +150,19 @@ pub(super) fn render_file_list(frame: &mut Frame, app: &mut App, area: Rect) { styles::file_status_style(&app.theme, status), )); } - spans.push(Span::raw(filename.to_string())); + // Generated files read as dimmed so the tree shows + // what is skippable at a glance. Deliberately not a + // glyph: `▶` already means "collapsed directory" in + // this panel, and the ▣/▢ checkbox stays about + // reviewed state alone. + if app.is_generated_file(path) { + spans.push(Span::styled( + filename.to_string(), + styles::dim_style(&app.theme), + )); + } else { + spans.push(Span::raw(filename.to_string())); + } Line::from(spans) } } diff --git a/src/ui/help_popup.rs b/src/ui/help_popup.rs index 93c6f77c..dcc3f811 100644 --- a/src/ui/help_popup.rs +++ b/src/ui/help_popup.rs @@ -395,6 +395,13 @@ pub fn render_help(frame: &mut Frame, app: &mut App) { ), Span::raw("Toggle hunk reviewed"), ]), + Line::from(vec![ + Span::styled( + " Space ", + Style::default().add_modifier(Modifier::BOLD), + ), + Span::raw("Expand / collapse a generated file"), + ]), Line::from(vec![ Span::styled( " c ", @@ -746,6 +753,27 @@ pub fn render_help(frame: &mut Frame, app: &mut App) { ), Span::raw(" Toggle inline commit selector"), ]), + Line::from(vec![ + Span::styled( + " :set generated", + Style::default().add_modifier(Modifier::BOLD), + ), + Span::raw(" Collapse files marked generated in .gitattributes"), + ]), + Line::from(vec![ + Span::styled( + " :set nogenerated", + Style::default().add_modifier(Modifier::BOLD), + ), + Span::raw(" Show generated files"), + ]), + Line::from(vec![ + Span::styled( + " :set generated!", + Style::default().add_modifier(Modifier::BOLD), + ), + Span::raw(" Toggle generated file collapse"), + ]), Line::from(vec![ Span::styled( " :clear ", From ea7b697e9d9e5a72582a59883e2a2a40041eff19 Mon Sep 17 00:00:00 2001 From: Moncef Naji Date: Mon, 3 Aug 2026 11:07:49 -0400 Subject: [PATCH 3/3] feat: let Space toggle collapse independent of reviewed status Space was already able to override collapse for generated files (expanded_generated); this generalizes that override into collapse_override so it applies to reviewed-file collapse too. The only way to peek at a reviewed file's diff used to be un-reviewing it with r, which also cleared its reviewed status. Space now toggles visibility without touching reviewed or generated state. --- docs/KEYBINDINGS.md | 2 +- src/app/diff_load.rs | 2 +- src/app/generated.rs | 33 --------------------- src/app/init.rs | 2 +- src/app/mod.rs | 7 +++-- src/app/reviewed.rs | 51 +++++++++++++++++++++++++------- src/app/tests/generated_tests.rs | 43 ++++++++++++++++++++------- src/handler.rs | 6 ++-- src/ui/help_popup.rs | 2 +- 9 files changed, 85 insertions(+), 63 deletions(-) diff --git a/docs/KEYBINDINGS.md b/docs/KEYBINDINGS.md index cf7c51ea..8365d008 100644 --- a/docs/KEYBINDINGS.md +++ b/docs/KEYBINDINGS.md @@ -76,7 +76,7 @@ Shown below the file tree when local comments or visible remote PR threads exist |-----|--------| | `r` | Toggle file reviewed | | `R` | Toggle hunk reviewed | -| `Space` | Expand / collapse the generated file under the cursor (diff panel) | +| `Space` | Expand / collapse the file under the cursor, independent of reviewed/generated status (diff panel) | | `c` | Add line comment (or file comment if not on a diff line) | | `C` | Add file comment | | `c` | Add review comment | diff --git a/src/app/diff_load.rs b/src/app/diff_load.rs index 7cd68e7b..0d32fecd 100644 --- a/src/app/diff_load.rs +++ b/src/app/diff_load.rs @@ -584,7 +584,7 @@ impl App { // Reload is the point where an edited `.gitattributes` should take // effect, so discard the probe memo and re-detect eagerly — the height // math below runs before `rebuild_annotations` would have done it. - // `expanded_generated` survives: it is the user's override, not + // `collapse_override` survives: it is the user's override, not // detected state. self.invalidate_generated_detection(); self.detect_generated_files(); diff --git a/src/app/generated.rs b/src/app/generated.rs index 768cd36d..422670ac 100644 --- a/src/app/generated.rs +++ b/src/app/generated.rs @@ -138,39 +138,6 @@ impl App { (reviewed, total) } - /// Toggle the collapsed state of the generated file under the cursor. - /// - /// Keyed by path rather than index so the override survives the reloads - /// and commit-selection changes that replace `diff_files` wholesale. - /// - /// Requires collapse to be on, not merely that the file is generated: - /// with `count = false` alone nothing is hidden, so claiming `Space` there - /// would report an expansion that changed nothing on screen. - pub fn toggle_generated_expansion(&mut self) -> bool { - if !self.collapse_generated { - return false; - } - let Some(path) = self - .current_file() - .map(|file| file.display_path().clone()) - .filter(|path| self.is_generated_file(path)) - else { - return false; - }; - - if self.expanded_generated.remove(&path) { - self.set_message("Generated file collapsed"); - } else { - self.expanded_generated.insert(path); - self.set_message("Generated file expanded"); - } - self.rebuild_annotations(); - let file_idx = self.diff_state.current_file_idx; - self.diff_state.cursor_line = self.calculate_file_scroll_offset(file_idx); - self.ensure_cursor_visible(); - true - } - /// Set the runtime collapse toggle, running detection if it was skipped /// while both features were off. pub fn set_collapse_generated(&mut self, collapse: bool) { diff --git a/src/app/init.rs b/src/app/init.rs index 60e54635..60afb78e 100644 --- a/src/app/init.rs +++ b/src/app/init.rs @@ -562,7 +562,7 @@ impl App { collapse_generated: false, count_generated: true, generated_files: HashSet::new(), - expanded_generated: HashSet::new(), + collapse_override: HashMap::new(), generated_probed: HashSet::new(), }; // Auto-hide file list when path filter matches exactly one file diff --git a/src/app/mod.rs b/src/app/mod.rs index 18d48d07..0e304b56 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -1306,8 +1306,11 @@ pub struct App { /// every index. Empty whenever detection is disabled, which is what lets /// the render path short-circuit for free. pub generated_files: HashSet, - /// Generated files the user expanded with `Space`, overriding collapse. - pub expanded_generated: HashSet, + /// Per-file collapse override set with `Space`, taking precedence over + /// the reviewed/generated defaults `is_file_collapsed` would otherwise + /// derive. Keyed by path rather than index so it survives the reloads + /// and commit-selection changes that replace `diff_files` wholesale. + pub collapse_override: HashMap, /// Display paths already looked up in `.gitattributes`. /// /// Doubles as the staleness test for detection: a diff file set with no diff --git a/src/app/reviewed.rs b/src/app/reviewed.rs index 2372d5bc..46bb9940 100644 --- a/src/app/reviewed.rs +++ b/src/app/reviewed.rs @@ -275,10 +275,13 @@ impl App { /// Whether a file's diff body is hidden, leaving only its header row. /// - /// Two reasons a body hides. Reviewed files collapse so a long review - /// stream shrinks as it is worked through. Files `.gitattributes` marks - /// as code-generated collapse when the user opted in, until `Space` - /// expands one. The annotation builder, both diff renderers, and the + /// Checked in override order. `Space` sets an explicit per-file entry in + /// `collapse_override` that always wins, because it is what lets a + /// reviewed or generated file be peeked at (or re-hidden) without + /// touching either status. Failing that, reviewed files collapse so a + /// long review stream shrinks as it is worked through. Failing that, + /// files `.gitattributes` marks as code-generated collapse when the user + /// opted in. The annotation builder, both diff renderers, and the /// scroll-height math all have to agree on this or the cursor lands on /// rows that aren't drawn, so they share this one predicate. /// @@ -289,19 +292,45 @@ impl App { /// /// Both renderers and `hunk_positions` call this once per file per frame, /// so it has to stay cheap. The checks are ordered so the default - /// configuration still costs exactly one map lookup: `collapse_generated` - /// is false unless the user opted in, and a bare bool test short-circuits - /// the rest. Opting in adds one hash lookup per non-reviewed file, and a - /// second only for the files that are actually generated. + /// configuration still costs exactly one map lookup: `collapse_override` + /// is empty unless the user has pressed `Space`, and `collapse_generated` + /// is false unless the user opted in, so a bare bool test short-circuits + /// the rest. #[inline] pub fn is_file_collapsed(&self, file: &DiffFile) -> bool { let path = file.display_path(); + if let Some(&collapsed) = self.collapse_override.get(path) { + return collapsed; + } if self.session.is_file_reviewed(path) { return true; } - self.collapse_generated - && self.generated_files.contains(path) - && !self.expanded_generated.contains(path) + self.collapse_generated && self.generated_files.contains(path) + } + + /// Toggle whether the file under the cursor shows its diff body, + /// independent of its reviewed or generated status — those only decide + /// the default before `Space` is pressed. + /// + /// Keyed by path rather than index so the override survives the reloads + /// and commit-selection changes that replace `diff_files` wholesale. + pub fn toggle_file_collapse(&mut self) -> bool { + let Some(file) = self.current_file() else { + return false; + }; + let path = file.display_path().clone(); + let currently_collapsed = self.is_file_collapsed(file); + self.collapse_override.insert(path, !currently_collapsed); + self.rebuild_annotations(); + let file_idx = self.diff_state.current_file_idx; + self.diff_state.cursor_line = self.calculate_file_scroll_offset(file_idx); + self.ensure_cursor_visible(); + self.set_message(if currently_collapsed { + "File expanded" + } else { + "File collapsed" + }); + true } pub fn file_count(&self) -> usize { diff --git a/src/app/tests/generated_tests.rs b/src/app/tests/generated_tests.rs index 400e37f4..111a427e 100644 --- a/src/app/tests/generated_tests.rs +++ b/src/app/tests/generated_tests.rs @@ -162,22 +162,24 @@ fn space_expansion_overrides_collapse() { let mut app = collapsing_app(&dir, &["api.pb.go"]); app.diff_state.current_file_idx = 0; - assert!(app.toggle_generated_expansion()); + assert!(app.toggle_file_collapse()); assert!(!app.is_file_collapsed(&file_named(&app, "api.pb.go"))); - assert!(app.toggle_generated_expansion()); + assert!(app.toggle_file_collapse()); assert!(app.is_file_collapsed(&file_named(&app, "api.pb.go"))); } #[test] -fn space_is_inert_on_a_file_that_is_not_generated() { - // The diff panel's Space handler falls through to the shared action when - // this returns false, so it must not claim non-generated files. +fn space_manually_collapses_a_file_that_is_not_generated() { + // Space is not scoped to generated files: it toggles any file's body, + // manually collapsing one that defaulted to expanded. let dir = repo(); let mut app = collapsing_app(&dir, &["src/main.rs"]); app.diff_state.current_file_idx = 0; - assert!(!app.toggle_generated_expansion()); + assert!(!app.is_file_collapsed(&file_named(&app, "src/main.rs"))); + assert!(app.toggle_file_collapse()); + assert!(app.is_file_collapsed(&file_named(&app, "src/main.rs"))); } #[test] @@ -239,10 +241,10 @@ fn toggling_collapse_off_returns_to_the_starting_appearance() { } #[test] -fn count_exclusion_alone_still_labels_without_claiming_space() { +fn count_exclusion_alone_still_labels_without_being_collapsed() { // `count = false, collapse = false` is a legitimate configuration: the - // files are surfaced and dropped from progress, but nothing is hidden, so - // there is nothing for `Space` to expand. + // file is surfaced and dropped from progress, but nothing is hidden by + // default. let dir = repo(); let mut app = app_in(&dir, &["api.pb.go", "src/main.rs"]); app.apply_generated_config(&GeneratedConfig { @@ -258,7 +260,6 @@ fn count_exclusion_alone_still_labels_without_claiming_space() { assert!(app.is_generated_file(Path::new("api.pb.go"))); assert_eq!(app.generated_file_count(), 1); assert!(!app.is_file_collapsed(&file_named(&app, "api.pb.go"))); - assert!(!app.toggle_generated_expansion()); } #[test] @@ -269,7 +270,7 @@ fn expansion_survives_a_diff_file_rebuild_that_shifts_indices() { let dir = repo(); let mut app = collapsing_app(&dir, &["api.pb.go"]); app.diff_state.current_file_idx = 0; - app.toggle_generated_expansion(); + app.toggle_file_collapse(); app.diff_files = vec![file("src/main.rs"), file("api.pb.go")]; app.rebuild_annotations(); @@ -281,6 +282,26 @@ fn expansion_survives_a_diff_file_rebuild_that_shifts_indices() { assert!(!app.is_file_collapsed(&file_named(&app, "api.pb.go"))); } +#[test] +fn space_expands_a_reviewed_file_without_un_reviewing_it() { + // The whole point of a per-file override: peeking at a reviewed file's + // diff must not touch its reviewed status, unlike the old behavior where + // `r` was the only way to see it again. + let dir = repo(); + let mut app = app_in(&dir, &["src/main.rs"]); + app.diff_state.current_file_idx = 0; + app.toggle_reviewed(); + assert!(app.is_file_collapsed(&file_named(&app, "src/main.rs"))); + + assert!(app.toggle_file_collapse()); + assert!(!app.is_file_collapsed(&file_named(&app, "src/main.rs"))); + assert!(app.session.is_file_reviewed(&PathBuf::from("src/main.rs"))); + + assert!(app.toggle_file_collapse()); + assert!(app.is_file_collapsed(&file_named(&app, "src/main.rs"))); + assert!(app.session.is_file_reviewed(&PathBuf::from("src/main.rs"))); +} + #[test] fn detection_runs_for_count_exclusion_without_collapse() { // `count` is deliberately not gated by `collapse`: excluding generated diff --git a/src/handler.rs b/src/handler.rs index 168cc96c..fde0b100 100644 --- a/src/handler.rs +++ b/src/handler.rs @@ -1419,9 +1419,11 @@ pub fn handle_diff_action(app: &mut App, action: Action) { } // Space already means "expand what's under the cursor" in the file // list and the commit selector; in the diff panel it was unbound, so - // it takes on the same meaning for a collapsed generated file. + // it takes on the same meaning here: toggle whether the current + // file's diff body is shown, regardless of why it defaulted to + // collapsed or expanded. Action::ToggleExpand => { - if !app.toggle_generated_expansion() { + if !app.toggle_file_collapse() { handle_shared_normal_action(app, action); } } diff --git a/src/ui/help_popup.rs b/src/ui/help_popup.rs index dcc3f811..addb0e0b 100644 --- a/src/ui/help_popup.rs +++ b/src/ui/help_popup.rs @@ -400,7 +400,7 @@ pub fn render_help(frame: &mut Frame, app: &mut App) { " Space ", Style::default().add_modifier(Modifier::BOLD), ), - Span::raw("Expand / collapse a generated file"), + Span::raw("Expand / collapse the current file"), ]), Line::from(vec![ Span::styled(