feat: collapse generated files marked in .gitattributes - #525
Open
ceffo wants to merge 3 commits into
Open
Conversation
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A review of hand-written code gets buried when a regenerated protobuf or API client lands in the same diff. This recognizes the marker the forges already define and lets the reviewer opt into collapsing those files, and into dropping them from review progress.
Both behaviors are off-by-default in the sense that matters: with no config, tuicr behaves exactly as it does today and pays no detection cost at all — no repository is opened and no attribute is read.
I want to flag up front that this is unrequested — there's no issue asking for it. I also noticed #230 asked for a built-in generated-file list and was resolved with the user-controlled
.tuicrignoreinstead, which reads as a preference for user-owned declarative config over shipped opinion. This is deliberately in that spirit: the file list comes from the user's.gitattributes, tuicr ships no patterns and no heuristics. If you'd still rather not carry it, #524 stands on its own and I'm happy to have just that one.Detection
.gitattributesonly —linguist-generated(GitHub's Linguist) orgitlab-generated— resolved through libgit2'sget_attr. That buys nested.gitattributes,.git/info/attributes,core.attributesFile, and[attr]macros behaving exactly asgit check-attrdoes, for free.I considered a header regex (
// Code generated by …) and rejected it: the marker sits at line 1, outside every hunk, so it needs full file content that PR/MR mode doesn't have.One trap worth calling out, because it's the kind of thing that silently hides code. GitHub documents
linguist-generated=trueand GitLab documents baregitlab-generated. Those land in differentAttrValuevariants (String("true")vsTrue), andget_attrreturns a sentinel string for a set-but-valueless attribute — git2's own docs warn against interpreting it directly. A naiveis_some()check would read-gitlab-generatedas an opt-in and hide a file the user explicitly asked to see. The whole variant matrix is tested.Two rules, both chosen to fail toward showing the diff, because a needlessly shown diff is an annoyance and a needlessly hidden one is unreviewed code:
Why
countisn't gated bycollapseExpanding 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. Both halves of the fraction drop the file — otherwise marking one reviewed would report
2/1.count = falsealone is also useful without collapsing: label the files, exclude them from progress, still show them.Per-frame cost
is_file_collapsedruns once per file per frame, so the checks are ordered so the default configuration costs exactly what it does today: the reviewed lookup short-circuits first, andcollapse_generatedis a bare bool that's false unless the user opted in.is_file_reviewed— 1, unchangedgenerated_files(1 per unreviewed file), +expanded_generated(only for files that are generated)file_header_prefix_textgains one lookup when opted in, for the[generated]label. Cancelling that out means threading the already-known reviewed state into it — worth doing, but it'd also change the reviewed path, so I'd rather send it separately than bundle it here. Multi-file view already pays two lookups there onmain.Where detection is hooked, and why not at the load sites
Detection runs from
rebuild_annotations, not from eachself.diff_files = …assignment. There are fifteen of those acrosspr.rs,commits.rs, anddiff_load.rs, and every one of them rebuilds annotations afterwards — so one hook can't drift, whereas a missed assignment site would silently desynchronize the annotations from what's drawn, which is exactly the class of bug #524 exists to prevent.Probed paths are memoized, and that memo doubles as the staleness test: a repeat call over an unchanged file set costs one hash lookup per file and no libgit2 work. Reload (
:e) discards it, so editing.gitattributesmid-session takes effect without a restart.State lives on
Appkeyed by display path, never by index — reloads, watch ticks, and commit-selection changes replacediff_fileswholesale and shift every index. There's a test for that specifically. I didn't add aDiffFilefield: it'd touch all 31 struct literals for no benefit, since the expanded-state set has to live onAppregardless.UI
Spacein 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. Off a collapsed generated file it falls through to the shared handler exactly as before.[generated]after the file header rule, shown whether or not the body is collapsed — once expanded, it's the only thing left saying why it was hidden.Files · 0/2 · 2 generated.▶already means "collapsed directory" in that panel, and▣/▢stays about reviewed state alone.:set generated/:set nogenerated/:set generated!/:generated, mirroring the existingset commitstriple. These work even withcollapse = false, so the opt-in default stays tolerable.The decoration is gated on the same condition as detection, not on "was anything ever detected", so
:generatedtwice from a default start lands back exactly where it began. (I got this wrong first time round — the detected set is deliberately retained when the feature is switched off so re-enabling is free, which made the labels and counter survive a toggle-off. There's a round-trip test for it now.)Limitations, stated plainly
.tuicrignorealready accepts. It reuses the existing local-checkout resolution, which is only set when the checkout matches the PR's target repo, so a foreign checkout can't mis-mark files.--all-filesviews; the counter and label still apply.ron an expanded generated file marks it reviewed and re-collapses it, same as any file. Intentional.Testing
cargo fmt --check,cargo clippy --all-targets -- -D warnings,cargo test— clean, 1229 passing (30 new). Rebased on 5675284.Dogfooded per CONTRIBUTING beyond just reviewing the diff: drove the actual TUI against a scratch repo with
*.pb.go linguist-generated=trueand confirmed collapse,Spaceexpand and re-collapse,:set generated!in both directions, the counter, side-by-side mode, the toggle round-trip returning to the exact starting screen, an opt-out picked up mid-session via:e, and that a config with no[generated]section renders identically to before.Follow-ups I'd rather not bundle
file_header_prefix_textdouble lookup above.reload_diff_filescallsfile_render_heightunguarded, so in single-file view a collapsed current file makes reload clamp the cursor to the file top. Pre-existing for reviewed files; this PR makes it reachable for generated ones. Benign, and fixing it changes reviewed-file behavior, so it doesn't belong here.🤖 Generated with Claude Code