Repository navigation
Support per-file review diff truncation - #3945
jakeleventhal wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| }, | ||
| [collapseScopeKey], | ||
| ); | ||
| const loadDiffFile = useCallback( |
There was a problem hiding this comment.
🟡 Medium components/DiffPanel.tsx:512
loadDiffFile keeps appending paths to expandedDiffFiles.filePaths without any size cap. If a preview has at least 101 truncated files and the user loads the 101st one, the next diffPreview request sends 101 entries in expandedFilePaths, exceeds the contract's 100-item maximum, and schema validation fails — replacing the usable preview with an error instead of loading that file. Consider clamping the array length (or rejecting further additions) before it reaches the 100-item limit.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/DiffPanel.tsx around line 512:
`loadDiffFile` keeps appending paths to `expandedDiffFiles.filePaths` without any size cap. If a preview has at least 101 truncated files and the user loads the 101st one, the next `diffPreview` request sends 101 entries in `expandedFilePaths`, exceeds the contract's 100-item maximum, and schema validation fails — replacing the usable preview with an error instead of loading that file. Consider clamping the array length (or rejecting further additions) before it reaches the 100-item limit.
| }, []); | ||
| const annotations = | ||
| draft?.fileKey === fileKey ? [...persisted, draft.annotation] : persisted; | ||
| !truncated && draft?.fileKey === fileKey ? [...persisted, draft.annotation] : persisted; |
There was a problem hiding this comment.
🟡 Medium diffs/AnnotatableCodeView.tsx:159
When a file becomes truncated while a draft comment is open, the draft annotation stops being rendered (line 159 guards on !truncated), but draft is never cleared. hasOpenComment stays true, so line selection and the gutter utility remain disabled for the entire CodeView, and the user is left with no visible annotation to cancel or submit the draft — commenting is stuck until the component is remounted. This can happen after a diff refresh or whitespace-setting change that pushes the file over the preview limit. Consider clearing draft (and selectedLines) when the target file becomes truncated, or guarding beginComment against truncated files so a draft can never open on one.
| !truncated && draft?.fileKey === fileKey ? [...persisted, draft.annotation] : persisted; | |
| const annotations = | |
| !truncated && draft?.fileKey === fileKey ? [...persisted, draft.annotation] : persisted; |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/diffs/AnnotatableCodeView.tsx around line 159:
When a file becomes truncated while a draft comment is open, the draft annotation stops being rendered (line 159 guards on `!truncated`), but `draft` is never cleared. `hasOpenComment` stays `true`, so line selection and the gutter utility remain disabled for the entire `CodeView`, and the user is left with no visible annotation to cancel or submit the draft — commenting is stuck until the component is remounted. This can happen after a diff refresh or whitespace-setting change that pushes the file over the preview limit. Consider clearing `draft` (and `selectedLines`) when the target file becomes truncated, or guarding `beginComment` against truncated files so a draft can never open on one.
| const pathspecs = fields.slice(index, index + pathCount); | ||
| const filePath = pathspecs.at(-1); | ||
| if (!status || !filePath || pathspecs.length < pathCount) break; | ||
| files.push({ filePath, pathspecs }); |
There was a problem hiding this comment.
🟠 High vcs/GitVcsDriverCore.ts:242
parseReviewDiffFiles returns tracked filenames directly as Git pathspecs, so paths containing glob metacharacters are misinterpreted by Git. A file like app/users/[id]/page.tsx is treated as a character-class pattern rather than a literal path, so its per-file git diff returns an empty result and the file disappears from the review preview. Prefix each pathspec with :(literal) so Git matches the exact path.
| files.push({ filePath, pathspecs }); | |
| if (!status || !filePath || pathspecs.length < pathCount) break; | |
| files.push({ filePath, pathspecs: pathspecs.map(pathspec => `:(literal)${pathspec}`) }); |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/vcs/GitVcsDriverCore.ts around line 242:
`parseReviewDiffFiles` returns tracked filenames directly as Git pathspecs, so paths containing glob metacharacters are misinterpreted by Git. A file like `app/users/[id]/page.tsx` is treated as a character-class pattern rather than a literal path, so its per-file `git diff` returns an empty result and the file disappears from the review preview. Prefix each pathspec with `:(literal)` so Git matches the exact path.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 4 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a91d660. Configure here.
| <> | ||
| <div className="diff-panel-viewport flex min-h-0 min-w-0 flex-1 flex-col overflow-hidden"> | ||
| {isSelectedPatchTruncated && ( | ||
| {isSelectedPatchTruncated && truncatedFilePaths.length === 0 && ( |
There was a problem hiding this comment.
List truncation warning hidden
Medium Severity
The incomplete-diff banner only renders when truncatedFilePaths is empty. If the changed-file name list is capped and any listed file also hits the per-file byte limit, truncated stays true but the banner is skipped, so files never fetched from a truncated list vanish with no global warning.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a91d660. Configure here.
| const filePaths = current.scopeKey === collapseScopeKey ? current.filePaths : []; | ||
| return filePaths.includes(filePath) | ||
| ? current | ||
| : { scopeKey: collapseScopeKey, filePaths: [...filePaths, filePath] }; |
There was a problem hiding this comment.
Load diff becomes no-op
Medium Severity
loadDiffFile stops updating state once a path is already in expandedFilePaths. If the refetch still returns that path in truncatedFilePaths (e.g. diff exceeds REVIEW_DIFF_EXPANDED_FILE_MAX_OUTPUT_BYTES), the Load diff control stays visible but further clicks do nothing.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a91d660. Configure here.
| .filter(({ result }) => result.stdoutTruncated) | ||
| .map(({ filePath }) => filePath), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Truncated file list drops paths
Medium Severity
When git diff --name-status or untracked ls-files output hits WORKSPACE_FILES_MAX_OUTPUT_BYTES, later changed paths are never fetched or listed in truncatedFilePaths. The source can still be marked truncated, so the review panel may look complete while entire files are missing with no per-file load action.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a91d660. Configure here.
| }; | ||
| }), | ||
| [collapsedDiffFileKeys, renderableFiles], | ||
| [collapsedDiffFileKeys, renderableFiles, truncatedFilePathSet], |
There was a problem hiding this comment.
Truncated paths lack UI rows
Medium Severity
Per-file truncation is surfaced only for paths that appear in the parsed patch. codeViewFiles is built exclusively from renderableFiles, so a path listed in truncatedFilePaths but omitted from the joined diff (e.g. empty trimmed stdout) has no row and no “Load diff” control despite expandedFilePaths support on the server.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a91d660. Configure here.
ApprovabilityVerdict: Needs human review 3 blocking correctness issues found. This PR introduces a new per-file diff truncation feature with user-facing UI changes, new API parameters, and backend processing changes. Multiple unresolved review comments identify potential bugs including a high-severity pathspec handling issue that could cause files to disappear from diffs. You can customize Macroscope's approvability policy. Learn more. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a91d660431
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ...whitespaceArgs, | ||
| input.range, | ||
| "--", | ||
| ...pathspecs, |
There was a problem hiding this comment.
When a changed filename contains Git pathspec syntax or metacharacters (for example :(top)foo or a*b), spreading the raw names here makes git diff ... -- <pathspec> interpret them as pathspecs instead of literal files. Git's glossary documents that pathspecs are used by git diff, */? are patterns, and a leading : has special meaning (https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-aiddefpathspecapathspec); with git 2.43 I verified a modified :(top)foo is listed by --name-status but this per-file diff emits no patch, so the review preview hides that file. Prefix the generated pathspecs with :(literal) or use literal pathspec handling before passing file names back to Git.
Useful? React with 👍 / 👎.
| ) { | ||
| return { | ||
| diff: diffs | ||
| .map(({ result }) => result.stdout.trim()) |
There was a problem hiding this comment.
When a per-file patch ends with a changed line whose content has trailing spaces (for example an EOF line changed to foo with a final newline), trim() removes those spaces before the diff parser sees the patch. Because this now runs on every file diff before joining, whitespace-sensitive review previews can misrepresent trailing-space changes in any file, not just remove the separator newline; use a newline-only trim such as trimEnd() with care or keep the raw stdout for rendering.
Useful? React with 👍 / 👎.
| const changedFilesResult = yield* executeGit( | ||
| `${input.operation}.list`, | ||
| input.cwd, | ||
| ["diff", "--no-ext-diff", "--name-status", "-z", ...whitespaceArgs, input.range, "--"], |
There was a problem hiding this comment.
When cwd is a subdirectory of the repo, this listing returns repo-root paths, but the follow-up per-file git diff ... -- <path> runs from the same subdirectory and treats those as cwd-relative pathspecs. I checked this with git 2.43 (sub/f is listed from cwd=sub, but git diff HEAD -- sub/f emits no patch), and the Git docs note that --relative is the option that makes subdirectory diff pathnames relative to the current directory (https://git-scm.com/docs/git-diff). In that context the review preview drops tracked changes under the selected subdirectory unless the paths are relativized or passed as top-level pathspecs.
Useful? React with 👍 / 👎.
| return filePaths.includes(filePath) | ||
| ? current | ||
| : { scopeKey: collapseScopeKey, filePaths: [...filePaths, filePath] }; |
There was a problem hiding this comment.
When a review contains multiple large truncated files, every Load diff click appends to this persistent list, and subsequent preview queries resend all expanded paths; the server then allows up to 10 MB per expanded file. A normal user loading many large files can therefore make each refresh return hundreds of MB of diff JSON and reparse it in the UI, which regresses the preview’s bounded-output behavior. Consider expanding only the requested file or enforcing a total expanded preview cap.
Useful? React with 👍 / 👎.
1 participant
Summary
Why
Review previews previously used one output budget for the entire tracked diff. A large file such as a lockfile could consume that budget before Git emitted later files, making valid changes disappear from the review panel.
The server now discovers changed paths first and generates bounded patches per file. Truncated files render as lightweight placeholders with an explicit load action, while smaller files remain available immediately.
Validation
pnpm exec vp checkpnpm exec vp run typecheckpnpm exec vp test apps/server/src/vcs/GitVcsDriverCore.test.ts apps/web/src/lib/diffRendering.test.tsNote
Medium Risk
Changes review diff generation (more git invocations, larger expanded output) and API contract fields; behavior is covered by integration tests but expanded loads can be heavy for huge files.
Overview
Review diff previews no longer use one global output cap for the whole patch. Git VcsDriver lists changed paths first, then fetches a bounded patch per file (tracked and untracked), merges them, and reports
truncatedFilePathsalongside the existingtruncatedflag. Paths inexpandedFilePathsuse a much higher per-file byte limit (up to 100 paths in the contract).The review contract adds optional
expandedFilePathson input andtruncatedFilePathson each preview source. DiffPanel tracks expanded files per review scope, passes them intodiffPreview, marks truncated files in the code view, shows a Load diff action instead of hunks, and only shows the global “incomplete diff” banner when truncation isn’t attributed to specific files.AnnotatableCodeView renders truncated files as empty hunks with header metadata and disables line comments on those placeholders. A diff rendering test covers parsing patches where one file’s output ends with
[truncated]and later files still parse.Reviewed by Cursor Bugbot for commit a91d660. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add per-file diff truncation and on-demand expansion to review diff preview
REVIEW_DIFF_FILE_MAX_OUTPUT_BYTES/REVIEW_DIFF_EXPANDED_FILE_MAX_OUTPUT_BYTES) applied independently to each tracked and untracked file inGitVcsDriverCore.ts.expandedFilePaths(up to 100 paths) to fetch full content for specific files, and responses includetruncatedFilePathsto identify oversized files.DiffPanel.tsx, truncated files are marked individually; a 'Load diff' button lets users expand a single file on demand without reloading the whole diff.AnnotatableCodeView.tsxrenders truncated files as empty placeholders and disables annotations until the file is expanded.📊 Macroscope summarized a91d660. 4 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.