feat(review): collapse lockfiles, minified, and generated files by default#470
Open
benvinegar wants to merge 1 commit into
Open
feat(review): collapse lockfiles, minified, and generated files by default#470benvinegar wants to merge 1 commit into
benvinegar wants to merge 1 commit into
Conversation
…fault Agent changesets routinely touch lockfiles, minified bundles, and generated code alongside real edits, burying the changes a human needs to read. Detect these as review "noise" and render them as a single expandable placeholder so the substantive diffs stand out. - Classify files via filename/path rules plus a capped scan for generator markers (@generated, DO NOT EDIT); recompute on every load so it survives watch reloads. - Collapse at the review-state layer by swapping in an empty-hunk placeholder variant, reusing the existing zero-hunk render/geometry path rather than threading a flag through the renderer. - Toggle with `x` or by clicking the placeholder (mouse parity); toggling re-pins the file's header so a height change can't scroll it out of view. - Persist collapse intent across watch reloads (keyed by file identity, only forgotten when a file leaves the changeset). - Control with `collapse_generated` config or --collapse-generated / --no-collapse-generated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Greptile SummaryThis PR collapses lockfiles, minified bundles, and generated files by default in the interactive review stream, replacing each with a single expandable placeholder. The implementation is layered cleanly: filename/content-based classification happens at load time, collapse policy resolution (noise-default + per-file manual overrides) lives in a pure derivation layer, and the renderer reuses the existing zero-hunk code path rather than introducing a new flag through the deep pipeline.
Confidence Score: 4/5Safe to merge; the new feature is additive and falls back gracefully — false negatives just show the file normally, false positives hide it behind a one-key expand. The architecture is well-considered and the coverage of unit, component, and PTY tests is thorough. The only gaps are in fileClassification.ts: *.generated.d.ts files slip past the path pattern due to the double-extension regex, and patch.split with a limit processes the entire patch string before truncating. Neither causes incorrect behaviour — missed classifications are false negatives that degrade gracefully — but both are worth tightening before the pattern solidifies. src/core/fileClassification.ts — the regex for the .generated. convention and the split-based line scan are the two spots worth a second look. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[DiffFile loaded by buildDiffFile] --> B{resolvedIsBinary?}
B -- yes --> C[noiseKind = undefined]
B -- no --> D[classifyNoiseFile path patch]
D --> E{LOCKFILE_NAMES.has basename?}
E -- yes --> F[noiseKind = lockfile]
E -- no --> G{MINIFIED_PATTERN.test path?}
G -- yes --> H[noiseKind = minified]
G -- no --> I{looksGeneratedByPath?}
I -- yes --> J[noiseKind = generated]
I -- no --> K{patch and looksGeneratedByContent?}
K -- yes --> L[noiseKind = generated]
K -- no --> M[noiseKind = null]
F & H & J & L & M --> N[DiffFile in changeset]
N --> O[resolveCollapsedFileIds]
O --> P{collapseGenerated and noiseKind?}
P -- yes --> Q{manuallyExpandedFileIds has id?}
Q -- no --> R[Add to collapsedFileIds]
Q -- yes --> S[Keep expanded]
P -- no --> T{manuallyCollapsedFileIds has id?}
T -- yes --> R
T -- no --> S
R --> U[buildReviewState: swap to collapsedFileVariant]
S --> V[buildReviewState: use original DiffFile]
U --> W[PierreDiffView: diffMessage placeholder]
W --> X{User clicks or presses x}
X --> Y[toggleFileCollapsed]
Y --> Z[Update manuallyExpanded/CollapsedFileIds]
Z --> O
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[DiffFile loaded by buildDiffFile] --> B{resolvedIsBinary?}
B -- yes --> C[noiseKind = undefined]
B -- no --> D[classifyNoiseFile path patch]
D --> E{LOCKFILE_NAMES.has basename?}
E -- yes --> F[noiseKind = lockfile]
E -- no --> G{MINIFIED_PATTERN.test path?}
G -- yes --> H[noiseKind = minified]
G -- no --> I{looksGeneratedByPath?}
I -- yes --> J[noiseKind = generated]
I -- no --> K{patch and looksGeneratedByContent?}
K -- yes --> L[noiseKind = generated]
K -- no --> M[noiseKind = null]
F & H & J & L & M --> N[DiffFile in changeset]
N --> O[resolveCollapsedFileIds]
O --> P{collapseGenerated and noiseKind?}
P -- yes --> Q{manuallyExpandedFileIds has id?}
Q -- no --> R[Add to collapsedFileIds]
Q -- yes --> S[Keep expanded]
P -- no --> T{manuallyCollapsedFileIds has id?}
T -- yes --> R
T -- no --> S
R --> U[buildReviewState: swap to collapsedFileVariant]
S --> V[buildReviewState: use original DiffFile]
U --> W[PierreDiffView: diffMessage placeholder]
W --> X{User clicks or presses x}
X --> Y[toggleFileCollapsed]
Y --> Z[Update manuallyExpanded/CollapsedFileIds]
Z --> O
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/core/fileClassification.ts:50
The regex `/\.generated\.[a-z]+$/i` won't match double-extension files like `types.generated.d.ts` because the `d.ts` segment contains a dot, which breaks the `[a-z]+$` anchor. TypeScript declaration files emitted by code generators (gRPC-web, protoc-gen-ts, graphql-codegen) commonly carry this double extension, so they'd be missed by path detection and would only be caught by content scanning — or not at all if they lack a generator header.
```suggestion
/\.generated(\.[a-z]+)+$/i, // explicit ".generated." convention (e.g. .generated.ts, .generated.d.ts)
```
### Issue 2 of 2
src/core/fileClassification.ts:77-95
`String.prototype.split(separator, limit)` in V8 processes the full string before applying the limit — it does not short-circuit at N lines. For a large generated file not caught by path patterns (e.g. a 5 MB GraphQL codegen output that only carries a `@generated` header), this allocates and immediately discards a huge array. Scanning up to `MAX_CONTENT_SCAN_LINES` newlines manually would cap the work to the first ~80 lines regardless of patch size.
Reviews (1): Last reviewed commit: "feat(review): collapse lockfiles, minifi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /\.g\.dart$/i, // build_runner (Dart) | ||
| /\.freezed\.dart$/i, // freezed (Dart) | ||
| /\.designer\.cs$/i, // WinForms designer (C#) | ||
| /\.generated\.[a-z]+$/i, // explicit ".generated." convention |
Contributor
There was a problem hiding this comment.
/\.generated\.[a-z]+$/i won't match double-extension files like types.generated.d.ts because the d.ts segment contains a dot, which breaks the [a-z]+$ anchor. TypeScript declaration files emitted by code generators (gRPC-web, protoc-gen-ts, graphql-codegen) commonly carry this double extension, so they'd be missed by path detection and would only be caught by content scanning — or not at all if they lack a generator header.
Suggested change
| /\.generated\.[a-z]+$/i, // explicit ".generated." convention | |
| /\.generated(\.[a-z]+)+$/i, // explicit ".generated." convention (e.g. .generated.ts, .generated.d.ts) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/fileClassification.ts
Line: 50
Comment:
The regex `/\.generated\.[a-z]+$/i` won't match double-extension files like `types.generated.d.ts` because the `d.ts` segment contains a dot, which breaks the `[a-z]+$` anchor. TypeScript declaration files emitted by code generators (gRPC-web, protoc-gen-ts, graphql-codegen) commonly carry this double extension, so they'd be missed by path detection and would only be caught by content scanning — or not at all if they lack a generator header.
```suggestion
/\.generated(\.[a-z]+)+$/i, // explicit ".generated." convention (e.g. .generated.ts, .generated.d.ts)
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+77
to
+95
| function looksGeneratedByContent(patch: string): boolean { | ||
| const lines = patch.split("\n", MAX_CONTENT_SCAN_LINES); | ||
|
|
||
| for (const line of lines) { | ||
| // Only consider added (`+foo`, not the `+++` header) or context (` foo`) lines. | ||
| const isAddition = line[0] === "+" && line[1] !== "+"; | ||
| const isContext = line[0] === " "; | ||
| if (!isAddition && !isContext) { | ||
| continue; | ||
| } | ||
|
|
||
| const haystack = line.slice(1).toLowerCase(); | ||
| if (GENERATED_CONTENT_MARKERS.some((marker) => haystack.includes(marker))) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
String.prototype.split(separator, limit) in V8 processes the full string before applying the limit — it does not short-circuit at N lines. For a large generated file not caught by path patterns (e.g. a 5 MB GraphQL codegen output that only carries a @generated header), this allocates and immediately discards a huge array. Scanning up to MAX_CONTENT_SCAN_LINES newlines manually would cap the work to the first ~80 lines regardless of patch size.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/fileClassification.ts
Line: 77-95
Comment:
`String.prototype.split(separator, limit)` in V8 processes the full string before applying the limit — it does not short-circuit at N lines. For a large generated file not caught by path patterns (e.g. a 5 MB GraphQL codegen output that only carries a `@generated` header), this allocates and immediately discards a huge array. Scanning up to `MAX_CONTENT_SCAN_LINES` newlines manually would cap the work to the first ~80 lines regardless of patch size.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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
1 participant
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.
Closes #462.
What
Agent changesets routinely touch lockfiles, minified bundles, and generated code alongside real edits, burying the changes a human actually needs to read. This detects those as review "noise" and renders each as a single expandable placeholder so the substantive diffs stand out.
src/core/fileClassification.ts): lockfiles (20+ ecosystems by basename), minified/source-map filenames, generated path shapes (*.pb.go,*_pb2.py,*.g.dart,*.generated.*), and a capped scan of the patch for generator markers (@generated,DO NOT EDIT). Recomputed on every load, so it survives watch reloads.x(keyboard) or by clicking the placeholder (mouse parity, mirroring GitHub's "Load diff"). Toggling re-pins the file's header to the top so the height change can't scroll it out of view.collapse_generatedconfig key (default on) and--collapse-generated/--no-collapse-generatedflags. Sidebar de-emphasizes collapsed files.Why this shape
A collapsed file is just a file with no hunks — a state the renderer, geometry, and windowing already handle. Swapping in a placeholder variant at the derivation layer keeps the deep render path as the single source of truth instead of duplicating "is this collapsed" logic through it.
Testing
AppHost.collapse.test.tsxdrives default-collapse →xexpand → re-collapse through the real host.test/pty/collapse.test.ts): default collapse +xtoggle + a real SGR mouse click-to-reveal in a terminal.🤖 Generated with Claude Code