fix: degrade change review instead of failing - #173
Conversation
The review card parsed the git diff patch without guards. An unexpected patch shape or a broken diff driver crashed card rendering and blanked the whole review widget. - parseReviewPatchFiles returns an ok flag instead of throwing - multiline-diff parse failures degrade to a fallback file list using the card's per-file summary, with an explanatory note - binary files render a file summary row with a hidden-diff note - ToolResultCard gains an optional error field the card can render
show_changes threw, failing the whole tool call, when the git-backed review could not produce a patch. The widget stayed blank at best. Catch review failures, log them as failed tool calls, and return an error card the widget expands to show the failure message.
git diff honors user-configured external diff drivers and textconv filters from ~/.gitconfig, which can produce unparseable output or launch interactive processes. Add --no-ext-diff and --no-textconv so review patches always come back as plain unified diffs.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe review flow now disables external diff transformations, returns structured MCP errors, parses patches without throwing, and renders binary files, parse fallbacks, expandable errors, and updated review styling. ChangesReview diff resilience
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant show_changes
participant reviewChanges
participant GitDiff
participant parseReviewPatchFiles
participant ReviewPayload
show_changes->>reviewChanges: request review changes
reviewChanges->>GitDiff: compare snapshots without external transformations
GitDiff-->>reviewChanges: patch or failure
reviewChanges-->>show_changes: review result or structured error
show_changes->>parseReviewPatchFiles: parse patch
parseReviewPatchFiles-->>ReviewPayload: files, binary paths, and parse status
ReviewPayload-->>ReviewPayload: render text, binary, fallback, or error content
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Waishnav
changed the title
Greptile SummaryThe PR hardens show_changes by disabling external Git diff processing, returning structured error cards, and adding resilient review-patch rendering and file-summary fallbacks.
Confidence Score: 3/5The PR should not merge until zero-hunk metadata changes are represented accurately and patch parsing preserves boundary whitespace. The new UI can mislabel valid mode-only or rename-only changes as binary, while full-string trimming can make the displayed final line differ from the actual Git patch. Files Needing Attention: src/ui/review-payload.tsx, src/ui/patch-display.ts
|
| Filename | Overview |
|---|---|
| src/review-checkpoints.ts | Disables external diff drivers and text conversion when generating review patches. |
| src/server.ts | Converts show_changes failures into logged, structured error cards rather than propagating exceptions. |
| src/ui/patch-display.ts | Adds guarded patch parsing, but whole-patch trimming can alter whitespace in the final diff line. |
| src/ui/review-payload.tsx | Adds error and fallback rendering, but incorrectly classifies all zero-hunk changes as binary. |
| src/ui/card-types.ts | Adds card error metadata and permits failed review cards to expand. |
| src/ui/patch-display.test.ts | Covers valid, empty, malformed, and CRLF parser inputs but not boundary whitespace or metadata-only diffs. |
| src/ui/workspace-app.css | Styles static fallback rows and explanatory review notices. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[show_changes] --> B[Create Git snapshot]
B --> C[git diff with ext-diff and textconv disabled]
C -->|failure| D[Structured error card]
C -->|success| E[Parse review patch]
E -->|throws| F[File-summary fallback]
E -->|parsed files| G{Has hunks?}
G -->|yes| H[Interactive diff preview]
G -->|no| I[Static no-preview summary]
Reviews (1): Last reviewed commit: "fix: disable external diff drivers in re..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (fileDiff.hunks.length === 0) { | ||
| return <BinaryFileList files={[fileDiff]} card={card} />; |
There was a problem hiding this comment.
When a mode-only change, pure rename, or other metadata-only diff has no content hunks, this branch labels it as “Binary file — diff preview hidden,” causing the card to misrepresent the change and conceal its actual metadata.
| */ | ||
| export function parseReviewPatchFiles(patch: string | undefined): ReviewPatchParse { | ||
| if (!patch) return { files: [], ok: true }; | ||
| const normalized = patch.replace(/\r\n/g, "\n").trim(); |
There was a problem hiding this comment.
When the final diff line contains trailing whitespace or is itself whitespace-only, trimming the entire patch removes that content before parsing, causing the rendered preview to differ from the actual working-tree change.
| const normalized = patch.replace(/\r\n/g, "\n").trim(); | |
| const normalized = patch.replace(/\r\n/g, "\n"); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ui/review-payload.tsx`:
- Around line 71-75: In src/ui/review-payload.tsx lines 71-75, update the
single-file branch to classify a file as binary only when its explicit binary
metadata indicates it, not merely when fileDiff.hunks is empty; render hunkless
renames and other non-binary files as a normal static summary. Apply the same
explicit binary check in src/ui/review-payload.tsx lines 114-122 before adding
the binary-file note, preserving the existing handling for actual binary files.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 05705fd1-85e9-4517-b7c7-0cc682ac3b9e
📒 Files selected for processing (7)
src/review-checkpoints.tssrc/server.tssrc/ui/card-types.tssrc/ui/patch-display.test.tssrc/ui/patch-display.tssrc/ui/review-payload.tsxsrc/ui/workspace-app.css
Hunkless diffs are normal for renames, mode changes, and binary files, but the review card treated every hunkless file as binary. Detect binary files from their explicit git markers instead, render hunkless renames and mode changes as ordinary static summaries, and stop blanking the card when a journal-only file (e.g. an oversized diff) has no parseable hunks. Also stop trimming patch whitespace: stripping leading and trailing whitespace from a patch could silently drop a trailing-space change on the final line. Only blank line runs at the patch edges are removed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ui/patch-display.ts`:
- Around line 190-195: Update binary path extraction in the patch parsing loop
around binaryFiles so quoted Git headers preserve the complete path, including
spaces, and match fileDiff.name in ReviewPayload. Use quote-aware Git path
parsing or parsed file metadata instead of splitting the header on spaces, and
add a regression test covering a binary filename with spaces through the
review-payload consumption path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 59cb9d2a-471e-4c79-aaa1-f5f25b90b0d9
📒 Files selected for processing (3)
src/ui/patch-display.test.tssrc/ui/patch-display.tssrc/ui/review-payload.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/review-payload.tsx
The space-split fallback for binary path extraction could record a fragment of a quoted header as the file name. Git diff headers already carry the new path after the 'b/' prefix, quoted when the path contains special characters, so the header match alone is sufficient and test coverage now locks in paths containing spaces, both plain and quoted.
1 participant
Hardens the change-review card so a bad diff can never blank the widget again.
What changed
parseReviewPatchFilesin the UI now guards parsing and returns anokflag instead of throwing. Unparseable patches, external diff-driver output, and binary diffs degrade to a file-summary list with an explanatory note instead of crashing the renderer.show_changescatches review failures and returns an error card (viacard.error) rather than failing the whole tool call.Why
Each of these was a real blank-widget or failed-call path in change review. The card layer should degrade gracefully; the tool call should surface the failure to the host.
No behavior change to what changes get computed — only how they render and fail.
Summary by CodeRabbit
New Features
Bug Fixes
Tests