Repository navigation
Feature: add inline diff review panel (#609) - #4020
Closed
austinywang wants to merge 23 commits into
Closed
austinywang wants to merge 23 commits into
austinywang wants to merge 23 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Diff Review panel surfaced via a new RightSidebarMode ChangesDiff Review Feature
Sequence Diagram(s)sequenceDiagram
participant UI as DiffReviewPanelView
participant Store as DiffReviewStore
participant Client as DiffReviewGitClient
participant Parser as DiffReviewPatchParser
participant Git as /usr/bin/git
UI->>Store: setDirectory / selectTarget / refresh / revertHunk
Store->>Client: loadSnapshot(directory, selectedTargetID)
Client->>Git: git rev-parse / branch / diff / ls-files
Git-->>Client: stdout/stderr results
Client->>Parser: parse(diffOutput, untrackedPaths)
Parser-->>Client: [DiffReviewFile[]]
Client-->>Store: DiffReviewSnapshot
Store-->>UI: publish snapshot / phase updates
UI->>Store: revertHunk(hunk)
Store->>Client: revertHunk(repositoryRoot, patch)
Client-->>Store: revert result / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches📝 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 |
Contributor
Greptile SummaryThis PR adds a right-sidebar Review mode that renders inline git diffs (working-tree vs HEAD, or branch vs merge-base) with per-hunk revert, a visibility-aware live-refresh timer, and full CLI/command-palette routing. The implementation is well-structured across dedicated model, parser, git client, store, and view files, and addresses all previous review concerns including concurrent pipe draining,
Confidence Score: 4/5Safe to merge after fixing two issues: @unchecked Sendable on the two git-process helper classes (Swift 6 compile error), and completing the rightSidebar.remote.error.usage.set translations for all 18 missing locales. The git client's GitProcessCancellation and GitProcessCompletion classes are captured in @sendable closures without Sendable conformance — a Swift 6 strict-concurrency compile error on a code path exercised on every diff load and hunk revert. Separately, the rightSidebar.remote.error.usage.set catalog entry was updated in this PR but 18 of 20 supported locales remain without an entry, so those users see outdated English fallback text. Both issues are isolated fixes with no architectural impact. Sources/DiffReviewGitClient.swift (GitProcessCancellation and GitProcessCompletion classes) and Resources/Localizable.xcstrings (rightSidebar.remote.error.usage.set entry). Important Files Changed
Sequence DiagramsequenceDiagram
participant PV as DiffReviewPanelView
participant S as DiffReviewStore (@MainActor)
participant GC as DiffReviewGitClient
participant Git as git process
PV->>S: setDirectory(path)
S->>S: refresh() + startLiveRefreshIfNeeded()
S->>GC: loadSnapshot(directory:selectedTargetID:)
GC->>Git: git rev-parse / diff / ls-files
Git-->>GC: stdout + stderr (concurrent FileHandle.bytes)
GC-->>S: DiffReviewSnapshot
S->>S: "phase = .loaded, snapshot set"
Note over S: Timer fires every 2s (allowsLiveRefresh only)
S->>GC: loadSnapshot() [live refresh]
GC-->>S: updated DiffReviewSnapshot
PV->>S: revertHunk(hunk)
S->>GC: revertHunk(repositoryRoot:patch:)
GC->>Git: git apply -R -
Git-->>GC: exit status
GC-->>S: success / DiffReviewGitError
S->>S: finishRevert → refresh()
PV->>S: stopObserving() (onDisappear)
S->>S: cancel loadTask, cancel revertTasks, stopLiveRefresh()
Reviews (13): Last reviewed commit: "fix: revert staged diff review hunks" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…-panel # Conflicts: # CLI/cmux.swift # cmux.xcodeproj/project.pbxproj
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 `@CLI/cmux.swift`:
- Around line 13163-13167: The three new hardcoded help strings ("set
<files|find|vault|sessions|review|diff|feed|dock>\n
Show, switch mode, and focus", "mode Print
{\"visible\":bool,\"mode\":string}", and
"files|find|vault|sessions|review|diff|feed|dock\n
Alias for show + set + focus") should be routed through localization: replace
each raw literal with String(localized:defaultValue:) (or your project's
localized API) using a distinct key/label and the original English as
defaultValue, and add matching entries to the app’s string catalog; update the
same pattern at the other occurrences referenced (around the other line ranges)
so all user-facing CLI help text is localized.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aa79c340-d2e5-4ee8-8d92-8cb16ea062ef
📒 Files selected for processing (21)
CLI/cmux.swiftResources/Localizable.xcstringsSources/ContentView+RightSidebarCommandPalette.swiftSources/DiffReviewGitClient.swiftSources/DiffReviewModels.swiftSources/DiffReviewPanelView.swiftSources/DiffReviewPatchParser.swiftSources/DiffReviewStore.swiftSources/MainWindowFocusController.swiftSources/RightSidebarMode+Availability.swiftSources/RightSidebarPanelView.swiftSources/RightSidebarRemoteCommand.swiftSources/RightSidebarToolPanel.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CLINotifyProcessIntegrationRegressionTests.swiftcmuxTests/DiffReviewPanelContentStateTests.swiftcmuxTests/DiffReviewPatchParserTests.swiftcmuxTests/FileExplorerStateModePersistenceTests.swiftcmuxTests/RightSidebarCommandPaletteTests.swiftcmuxTests/ShortcutAndCommandPaletteTests.swiftcmuxTests/TerminalControllerSocketSecurityTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/DiffReviewGitClient.swift (2)
215-218: 🧹 Nitpick | 🔵 Trivial | 💤 Low valuePrefer throwing
write(contentsOf:)for robust error handling.The non-throwing
write(_:)silently fails on errors (e.g., broken pipe if the process terminates early). Usingwrite(contentsOf:)surfaces failures so they can propagate ascommandFailed(.generic).♻️ Suggested fix
- inputPipe.fileHandleForWriting.write(Data(standardInput.utf8)) + try inputPipe.fileHandleForWriting.write(contentsOf: Data(standardInput.utf8))🤖 Prompt for 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. In `@Sources/DiffReviewGitClient.swift` around lines 215 - 218, Replace the non-throwing write(Data(_:) ) with the throwing write(contentsOf:) so write failures (e.g., broken pipe) propagate instead of being silently ignored: when writing the String standardInput to inputPipe?.fileHandleForWriting use the throwing API (try fileHandleForWriting.write(contentsOf: Data(standardInput.utf8))) and let the surrounding function propagate the error (or catch and map to commandFailed(.generic)) before closing the file handle; update the code paths referencing standardInput and inputPipe to perform the write with try and proper error propagation.
141-149: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy liftConsider bounded concurrency for untracked file diffs.
Sequential process spawning for up to 100 untracked files may be slow due to fork/exec overhead per file. Using
TaskGroupwith bounded concurrency (e.g., 8-10 concurrent processes) would reduce latency significantly while avoiding resource exhaustion.♻️ Suggested approach using TaskGroup
- var untrackedOutputs: [String] = [] - for path in untrackedPaths.prefix(100) { - if let output = try? await runGit( - in: repositoryRoot, - arguments: ["diff", "--no-ext-diff", "--no-color", "--unified=3", "--no-index", "--", "/dev/null", path], - acceptedStatuses: [0, 1] - ).stdout { - untrackedOutputs.append(output) - } - } + let pathsToProcess = Array(untrackedPaths.prefix(100)) + let untrackedOutputs: [String] = await withTaskGroup(of: (Int, String?).self) { group in + for (index, path) in pathsToProcess.enumerated() { + group.addTask { + let output = try? await runGit( + in: repositoryRoot, + arguments: ["diff", "--no-ext-diff", "--no-color", "--unified=3", "--no-index", "--", "/dev/null", path], + acceptedStatuses: [0, 1] + ).stdout + return (index, output) + } + } + var results = [(Int, String)]() + for await (index, output) in group { + if let output { results.append((index, output)) } + } + return results.sorted { $0.0 < $1.0 }.map(\.1) + }🤖 Prompt for 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. In `@Sources/DiffReviewGitClient.swift` around lines 141 - 149, The loop that calls runGit for each path in untrackedPaths (up to 100) should be parallelized with bounded concurrency to avoid sequential fork/exec overhead: replace the for loop with a withTaskGroup (or TaskGroup) that launches up to N concurrent child tasks (suggest N = 8) which each call runGit(in: repositoryRoot, arguments: ..., acceptedStatuses: [0,1]) for a single path; have each task return its stdout (or nil on failure) and then collect results after awaiting the group, appending non-nil outputs to untrackedOutputs (do not mutate shared untrackedOutputs inside tasks—collect into a local array from the group's returned values or use an actor/lock if you must mutate concurrently). Ensure you still respect the prefix(100) limit on untrackedPaths and preserve the same arguments and status handling used by runGit.
🤖 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 `@cmuxTests/DiffReviewPanelContentStateTests.swift`:
- Around line 90-114: Add two tests that exercise plural forms for the summary
formatter: create testSummaryFormatterUsesPluralFileLabelForZeroFiles and
testSummaryFormatterUsesPluralFileLabelForMultipleFiles that build
DiffReviewSnapshot instances (empty files array for zero, and two DiffReviewFile
entries for multiple) and call
DiffReviewSummaryFormatter.summaryText(snapshot:); assert the returned summary
contains "0 files" and "2 files" respectively to verify the ICU-style .other
plural branch alongside the existing testSummaryFormatterUsesSingularFileLabel.
---
Outside diff comments:
In `@Sources/DiffReviewGitClient.swift`:
- Around line 215-218: Replace the non-throwing write(Data(_:) ) with the
throwing write(contentsOf:) so write failures (e.g., broken pipe) propagate
instead of being silently ignored: when writing the String standardInput to
inputPipe?.fileHandleForWriting use the throwing API (try
fileHandleForWriting.write(contentsOf: Data(standardInput.utf8))) and let the
surrounding function propagate the error (or catch and map to
commandFailed(.generic)) before closing the file handle; update the code paths
referencing standardInput and inputPipe to perform the write with try and proper
error propagation.
- Around line 141-149: The loop that calls runGit for each path in
untrackedPaths (up to 100) should be parallelized with bounded concurrency to
avoid sequential fork/exec overhead: replace the for loop with a withTaskGroup
(or TaskGroup) that launches up to N concurrent child tasks (suggest N = 8)
which each call runGit(in: repositoryRoot, arguments: ..., acceptedStatuses:
[0,1]) for a single path; have each task return its stdout (or nil on failure)
and then collect results after awaiting the group, appending non-nil outputs to
untrackedOutputs (do not mutate shared untrackedOutputs inside tasks—collect
into a local array from the group's returned values or use an actor/lock if you
must mutate concurrently). Ensure you still respect the prefix(100) limit on
untrackedPaths and preserve the same arguments and status handling used by
runGit.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1ec20d6-11b5-4365-a702-d3a8c8640dda
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/DiffReviewGitClient.swiftSources/DiffReviewPanelView.swiftcmuxTests/DiffReviewPanelContentStateTests.swift
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/DiffReviewGitClient.swift (1)
247-253: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider reading pipe data in chunks instead of byte-by-byte.
Iterating
FileHandle.bytesappends one byte at a time, which incurs per-byte overhead. For large diffs (multi-MB in active repos), this can noticeably slow snapshot loading. Reading in chunks (e.g., viaavailableDataorread(upToCount:)in a loop) would be more efficient.♻️ Suggested chunk-based reading
- private static func readData(from handle: FileHandle) async throws -> Data { - var data = Data() - for try await byte in handle.bytes { - data.append(byte) - } - return data - } + private static func readData(from handle: FileHandle) async throws -> Data { + try await withCheckedThrowingContinuation { continuation in + DispatchQueue.global(qos: .utility).async { + do { + let data = try handle.readToEnd() ?? Data() + continuation.resume(returning: data) + } catch { + continuation.resume(throwing: error) + } + } + } + }🤖 Prompt for 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. In `@Sources/DiffReviewGitClient.swift` around lines 247 - 253, The readData(from:) function currently iterates FileHandle.bytes and appends one byte at a time (per-byte overhead); change it to read in chunks instead by looping on FileHandle.read(upToCount:) or FileHandle.availableData until EOF and append each Data chunk to the accumulator to improve performance for large diffs; update the method readData(from handle: FileHandle) to perform chunked reads and return the combined Data (preserve async/throws semantics and close the handle where appropriate).
🤖 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.
Outside diff comments:
In `@Sources/DiffReviewGitClient.swift`:
- Around line 247-253: The readData(from:) function currently iterates
FileHandle.bytes and appends one byte at a time (per-byte overhead); change it
to read in chunks instead by looping on FileHandle.read(upToCount:) or
FileHandle.availableData until EOF and append each Data chunk to the accumulator
to improve performance for large diffs; update the method readData(from handle:
FileHandle) to perform chunked reads and return the combined Data (preserve
async/throws semantics and close the handle where appropriate).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f372efbb-80dc-45d2-9e38-44501bcea958
📒 Files selected for processing (1)
Sources/DiffReviewGitClient.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 39fd20d. Configure here.
Contributor
Author
|
Closing for now; not actively working on this. Branch issue-609-diff-review-panel is preserved if we pick it back up. |
This branch was successfully deployed
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 #609
Summary:
Testing:
Note
Medium Risk
Introduces git subprocesses and
git apply -Rhunk revert that mutates the working tree; mistakes or race conditions during refresh could confuse diff state, though changes are user-initiated in a local repo.Overview
Adds a Review right-sidebar mode that shows inline git diffs for the active workspace, with a Working Tree vs branch comparison picker, file/hunk summaries, and revert hunk on the working tree via
git apply.New SwiftUI stack (
DiffReviewStore,DiffReviewGitClient, patch parser, panel UI) loads snapshots asynchronously, refreshes on a timer while visible, and keeps the last good snapshot when later loads fail. Command palette gains Show Sidebar Review;cmux right-sidebarand remoteright_sidebaracceptreview,diff, andcode-reviewaliases (canonicalreview), with help text split into localized string keys.Unit tests cover patch parsing, panel content state, and CLI mode normalization.
Reviewed by Cursor Bugbot for commit 63692bf. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a right‑sidebar Review mode with inline per‑file/hunk diffs, a working‑tree/branch picker, and per‑hunk revert for working‑tree and staged changes. Includes CLI aliases, localized UI, visibility‑aware refresh, and preserves snapshots on transient errors.
New Features
git apply -R; cancellable with progress; refresh pauses during revert.mode.commandTitle; right‑sidebar CLI addsreview/diff/code-review/code_reviewaliases with localized help and updated focus/availability.Bug Fixes
gitpipe writers to prevent hangs; map git root errors to localized messages.Written for commit 63692bf. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
CLI / Documentation
Platform Integration
Tests