Repository navigation
Feat/better diff viewer - #4
Conversation
Expose view-only ProjectDiff entry points for embedders while preserving Zed's default interactive diff behavior.
Objective
Solution
Testing
Self-Review Checklist:
ShowcaseN/A Release Notes:
Summary By MatterAI
🔄 What ChangedThis PR enhances the diff viewer by ensuring repository context is correctly propagated during 🔍 Impact of the ChangeImproves the reliability of historical diff previews and read-only views. It prevents repository context loss during item creation and simplifies internal state management, leading to more maintainable and robust git integration. 📁 Total Files ChangedClick to Expand
🧪 Test Added/RecommendedAdded
Recommended
🔒 Security VulnerabilitiesN/A - No security vulnerabilities detected. Logic focuses on internal state propagation and path safety. 🛡️ |
📝 WalkthroughWalkthroughAdds a commit diff preview workspace item, threads options through ChangesDiff preview and options-driven ProjectDiff
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
PR Summary by QodoAdd embedded, view-only diff previews for project and commit diffs
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
There was a problem hiding this comment.
Pull request overview
This PR extends Zed’s git diff UI to support an embeddable, view-only “ProjectDiff” mode (via configurable options), introduces a new multi-file commit diff preview item, and improves git status refresh batching while hardening conflict UI behavior against missing addons.
Changes:
- Added
GitStore::refresh_status_for_project_pathsto batch per-repo status refreshes from a set ofProjectPaths. - Introduced
ProjectDiffOptionsand new deploy helpers to allow embeddingProjectDiffin a view-only configuration (disabling hunk controls, review button, git panel addon, index mutations, and conflict view). - Added
CommitDiffPreview(new item) to render multi-file commit diffs in a read-only editor-backed view, and exposed it viagit_uimodule exports.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/project/src/git_store.rs | Adds a batched status refresh API for multiple project paths. |
| crates/git_ui/src/project_diff.rs | Adds ProjectDiffOptions and deploy helpers to support embedded/view-only diffs and safer conflict handling. |
| crates/git_ui/src/git_ui.rs | Exposes the new commit_diff_preview module. |
| crates/git_ui/src/conflict_view.rs | Avoids panics by handling missing ConflictAddon more defensively. |
| crates/git_ui/src/commit_diff_preview.rs | New multi-file commit diff preview item with excerpted hunks and basic tests. |
| crates/editor/src/split.rs | Adds a SplittableEditor helper to delegate stage/restore behavior to callers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let updates_tx = match &self.state { | ||
| GitStoreState::Local { downstream, .. } => downstream | ||
| .as_ref() | ||
| .map(|downstream| downstream.updates_tx.clone()), | ||
| GitStoreState::Remote { .. } => None, | ||
| }; |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new CommitDiffPreview component in git_ui for previewing commit diffs, adds configuration options to ProjectDiff via ProjectDiffOptions to support view-only embedded diffs, and implements refresh_status_for_project_paths in GitStore. Additionally, it refactors conflict_view.rs to safely handle the absence of ConflictAddon. Review feedback focused on adhering to the repository style guide in the new commit_diff_preview.rs file, specifically recommending importing util::ResultExt to log errors, handling the Result of the spawned task in populate_entries with .log_err(), and replacing panicking .unwrap() calls with safe fallbacks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| use util::paths::PathStyle; | ||
| use util::rel_path::RelPath; |
There was a problem hiding this comment.
Import util::ResultExt to allow logging of errors using .log_err() on fallible operations, adhering to the repository style guide.
use util::paths::PathStyle;
use util::rel_path::RelPath;
use util::ResultExt as _;References
- Never silently discard errors with
let _ =on fallible operations. Always handle errors appropriately: Use.log_err()or similar when you need to ignore errors but want visibility. (link)
| let populate_task = cx.spawn(async move |_, cx| { | ||
| populate_entries(multibuffer, entries, context_lines, language_registry, cx).await | ||
| }); |
There was a problem hiding this comment.
The task returned by cx.spawn is stored in _populate_task to prevent cancellation, but its inner Result<()> is never awaited or handled. If populate_entries fails, the error will be silently discarded. Use .log_err() to ensure visibility of any errors, adhering to the repository style guide.
let populate_task = cx.spawn(async move |_, cx| {
populate_entries(multibuffer, entries, context_lines, language_registry, cx)
.await
.log_err();
anyhow::Ok(())
});References
- Never silently discard errors with
let _ =on fallible operations. Always handle errors appropriately: Use.log_err()or similar when you need to ignore errors but want visibility. (link)
| RelPath::new(Path::new(display_path), PathStyle::Posix) | ||
| .unwrap_or_else(|_| RelPath::new(Path::new("untitled"), PathStyle::Posix).unwrap()) | ||
| .into_owned() |
There was a problem hiding this comment.
Avoid using panicking functions like .unwrap() on fallible operations, even when the fallback path is statically guaranteed to succeed. Use safe fallbacks or unwrap_or_else with unreachable!() to adhere to the repository style guide.
RelPath::new(Path::new(display_path), PathStyle::Posix)
.unwrap_or_else(|_| {
RelPath::new(Path::new("untitled"), PathStyle::Posix)
.unwrap_or_else(|_| unreachable!("'untitled' is a valid relative path"))
})
.into_owned()References
- Avoid using functions that panic like
unwrap(), instead use mechanisms like?to propagate errors. (link)
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/git_ui/src/project_diff.rs (1)
1017-1033: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFall back to diff hunk ranges when conflict view is disabled.
excerpt_rangesstill prefersconflict_setranges whenever conflicts exist, but this block skips conflict UI updates whenenable_conflict_viewis false. Embedded/view-only diffs can then show only conflict regions and omit other changed hunks in the file.Proposed fix
- let excerpt_ranges = { + let enable_conflict_view = self.options.enable_conflict_view; + let excerpt_ranges = { let diff_hunk_ranges = diff_snapshot .hunks_intersecting_range( Anchor::min_max_range_for_buffer(snapshot.remote_id()), &snapshot, ) .map(|diff_hunk| diff_hunk.buffer_range.to_point(&snapshot)); - let conflicts = conflict_set.read(cx).snapshot(); - let mut conflicts = conflicts - .conflicts - .iter() - .map(|conflict| conflict.range.to_point(&snapshot)) - .peekable(); - - if conflicts.peek().is_some() { - conflicts.collect::<Vec<_>>() + + if enable_conflict_view { + let conflicts = conflict_set.read(cx).snapshot(); + let mut conflicts = conflicts + .conflicts + .iter() + .map(|conflict| conflict.range.to_point(&snapshot)) + .peekable(); + + if conflicts.peek().is_some() { + conflicts.collect::<Vec<_>>() + } else { + diff_hunk_ranges.collect() + } } else { diff_hunk_ranges.collect() } }; let mut needs_fold = None; - let enable_conflict_view = self.options.enable_conflict_view;🤖 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 `@crates/git_ui/src/project_diff.rs` around lines 1017 - 1033, The excerpt selection in update logic still prioritizes conflict_set ranges even when enable_conflict_view is off, so embedded/view-only diffs can miss non-conflict hunks. Update the range selection around editor.update_excerpts_for_path in project_diff.rs to fall back to normal diff hunk ranges whenever conflict view is disabled, while keeping conflict_view::buffer_ranges_updated limited to the enabled case. Make sure the excerpt_ranges passed into update_excerpts_for_path reflects all changed hunks when self.options.enable_conflict_view is false.
🤖 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 `@crates/git_ui/src/commit_diff_preview.rs`:
- Around line 76-87: Preview registration is losing the real metadata for
renamed/copied and deleted files because PreviewEntry only keeps a display path
and PreviewBufferFile is built without the original paths or deletion flags.
Update PreviewEntry and PreviewBufferFile to carry a metadata path plus
was_deleted (and preserve old_path/new_path where needed), then thread those
values through the PreviewEntry creation and PreviewBufferFile construction in
commit_diff_preview so language/metadata lookup uses the real file path and
deleted entries are reported correctly.
In `@crates/git_ui/src/project_diff.rs`:
- Around line 636-638: Split-pane stage/restore is still being handled in
SplittableEditor before ProjectDiff can block it, so left-pane keybindings can
mutate the index/worktree even when allow_index_mutations is false. Add a gate
in SplittableEditor for the left-pane stage/restore path, or stop translating
those events directly into do_stage_or_unstage and restore_diff_hunks when
mutations are disallowed, so ProjectDiff’s set_delegate_stage_and_restore
control is respected everywhere.
---
Outside diff comments:
In `@crates/git_ui/src/project_diff.rs`:
- Around line 1017-1033: The excerpt selection in update logic still prioritizes
conflict_set ranges even when enable_conflict_view is off, so embedded/view-only
diffs can miss non-conflict hunks. Update the range selection around
editor.update_excerpts_for_path in project_diff.rs to fall back to normal diff
hunk ranges whenever conflict view is disabled, while keeping
conflict_view::buffer_ranges_updated limited to the enabled case. Make sure the
excerpt_ranges passed into update_excerpts_for_path reflects all changed hunks
when self.options.enable_conflict_view is false.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c71c91a-6629-46ae-8ae0-23a0a7d127e3
📒 Files selected for processing (6)
crates/editor/src/split.rscrates/git_ui/src/commit_diff_preview.rscrates/git_ui/src/conflict_view.rscrates/git_ui/src/git_ui.rscrates/git_ui/src/project_diff.rscrates/project/src/git_store.rs
| if !options.allow_index_mutations { | ||
| diff_display_editor.set_delegate_stage_and_restore(true, cx); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Block split-pane stage/restore before it reaches the RHS editor.
ProjectDiff swallows delegated stage/restore events here, but split-mode left-pane events are handled inside SplittableEditor and translated directly into RHS do_stage_or_unstage / restore_diff_hunks calls before ProjectDiff sees them. Embedded view-only diffs can still mutate the index/worktree via left-pane keybindings. Add a SplittableEditor-level gate, or emit those left-pane events upward instead of translating when allow_index_mutations is false.
Also applies to: 909-915
🤖 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 `@crates/git_ui/src/project_diff.rs` around lines 636 - 638, Split-pane
stage/restore is still being handled in SplittableEditor before ProjectDiff can
block it, so left-pane keybindings can mutate the index/worktree even when
allow_index_mutations is false. Add a gate in SplittableEditor for the left-pane
stage/restore path, or stop translating those events directly into
do_stage_or_unstage and restore_diff_hunks when mutations are disallowed, so
ProjectDiff’s set_delegate_stage_and_restore control is respected everywhere.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/git_ui/src/git_ui.rs">
<violation number="1" location="crates/git_ui/src/git_ui.rs:34">
P3: The new `pub mod commit_diff_preview;` module (548 lines) is declared but has zero references across the codebase — no imports, no re-exports, no symbol usage anywhere. If this module is intended to be used in the same PR, wire it up before merging. If it's a public API for future consumers, consider adding a TODO or an explicit re-export path so the intent is clear. Otherwise it's dead code that adds compile-time overhead without providing value.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| mod askpass_modal; | ||
| pub mod branch_picker; | ||
| pub mod commit_diff_preview; |
There was a problem hiding this comment.
P3: The new pub mod commit_diff_preview; module (548 lines) is declared but has zero references across the codebase — no imports, no re-exports, no symbol usage anywhere. If this module is intended to be used in the same PR, wire it up before merging. If it's a public API for future consumers, consider adding a TODO or an explicit re-export path so the intent is clear. Otherwise it's dead code that adds compile-time overhead without providing value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/git_ui/src/git_ui.rs, line 34:
<comment>The new `pub mod commit_diff_preview;` module (548 lines) is declared but has zero references across the codebase — no imports, no re-exports, no symbol usage anywhere. If this module is intended to be used in the same PR, wire it up before merging. If it's a public API for future consumers, consider adding a TODO or an explicit re-export path so the intent is clear. Otherwise it's dead code that adds compile-time overhead without providing value.</comment>
<file context>
@@ -31,6 +31,7 @@ use crate::{commit_view::CommitView, git_panel::GitPanel, text_diff_view::TextDi
mod askpass_modal;
pub mod branch_picker;
+pub mod commit_diff_preview;
mod commit_modal;
pub mod commit_tooltip;
</file context>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/project/src/git_store.rs (1)
1414-1423: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant double-match on
self.state.The remote check at Line 1414-1416 and the second
match &self.stateat Line 1418-1423 both inspect the same unmutated state. Collapsing into a singlelet-elseavoids theunreachable!()branch entirely.♻️ Suggested consolidation
- if matches!(&self.state, GitStoreState::Remote { .. }) { - return 0; - } - - let updates_tx = match &self.state { - GitStoreState::Local { downstream, .. } => downstream - .as_ref() - .map(|downstream| downstream.updates_tx.clone()), - GitStoreState::Remote { .. } => unreachable!("remote git store returned above"), - }; + let GitStoreState::Local { downstream, .. } = &self.state else { + return 0; + }; + let updates_tx = downstream.as_ref().map(|downstream| downstream.updates_tx.clone());🤖 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 `@crates/project/src/git_store.rs` around lines 1414 - 1423, The `self.state` logic in the `updates_tx` path is doing a redundant second match after already returning for `GitStoreState::Remote`; collapse this into a single `let-else` or equivalent in the same function so `unreachable!()` is no longer needed. Update the `updates_tx` extraction to handle only the `GitStoreState::Local { downstream, .. }` case directly, keeping the existing remote early return behavior intact.
🤖 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.
Nitpick comments:
In `@crates/project/src/git_store.rs`:
- Around line 1414-1423: The `self.state` logic in the `updates_tx` path is
doing a redundant second match after already returning for
`GitStoreState::Remote`; collapse this into a single `let-else` or equivalent in
the same function so `unreachable!()` is no longer needed. Update the
`updates_tx` extraction to handle only the `GitStoreState::Local { downstream,
.. }` case directly, keeping the existing remote early return behavior intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2aaa9091-07a6-44c2-8efd-bcb53867e41e
📒 Files selected for processing (5)
crates/editor/src/split.rscrates/git_ui/src/commit_diff_preview.rscrates/git_ui/src/git_ui.rscrates/git_ui/src/project_diff.rscrates/project/src/git_store.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/git_ui/src/git_ui.rs
- crates/git_ui/src/commit_diff_preview.rs
- crates/git_ui/src/project_diff.rs
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/git_ui/src/project_diff.rs">
<violation number="1" location="crates/git_ui/src/project_diff.rs:1375">
P2: Splitting a ProjectDiff can show the wrong repository when the original diff was pinned to a non-active repo, because the cloned BranchDiff is recreated from DiffBase only and falls back to active_repository(). Preserving self.branch_diff.read(cx).repo() and applying set_repo in the new BranchDiff would keep split panes on the same repo.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Objective
Solution
Testing
Self-Review Checklist:
Showcase
While a showcase should aim to be brief and digestible, you can use a toggleable section to save space on longer showcases:
Click to view showcase
My super cool demos here
Release Notes:
Summary by cubic
Adds an embeddable, view-only diff and a new commit diff preview, while keeping the default interactive diff unchanged. Hardens conflict handling, improves targeted git status refresh, fixes diff preview integration, and adds opt-in performance tracing.
New Features
ProjectDiffOptionswith toggles for hunk controls, review button, git panel addon, index mutations, and conflict view; newdeploy_at_with_options/deploy_at_project_path_with_optionswithProjectDiffOptions::embedded_view_only()for safe, view-only diffs.CommitDiffPreviewworkspace item for multi-file commit diffs with syntax highlighting and excerpted hunks; preview buffers are named by display path and thecommit_diff_previewmodule is exported for embedders.SplittableEditor::set_delegate_stage_and_restoreto delegate stage/unstage/restore actions (used to disable index mutations in embedded mode).CommitDiffPreviewviaCHERRYPICK_DIFF_PREVIEW_TRACE.Bug Fixes
GitStore::refresh_status_for_project_paths, batching path refreshes and setting a fallback active repo when needed.ProjectDiffbehavior remains interactive.Written for commit 700662a. Summary will update on new commits.
Summary by CodeRabbit