public/galaxy
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Improve large diff and merge conflict workflows

Browse Source
This commit is contained in:
rkw6086
2026-08-28 21:08:55 -05:00
parent 88c1ef9716
commit e8166012f4
15 changed files with 908 additions and 76 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/src/code_review/code_review_header/header_revamp.rs
+38 -1
View File
@@ -9,7 +9,7 @@ use galaxy_core::features::FeatureFlag;
use galaxyui::elements::{
ChildAnchor, ChildView, Clipped, ConstrainedBox, Container, CrossAxisAlignment, Flex,
MainAxisAlignment, MainAxisSize, OffsetPositioning, ParentAnchor, ParentElement,
ParentOffsetBounds, Shrinkable, Stack,
ParentOffsetBounds, Shrinkable, Stack, Text,
};
use galaxyui::{Element, ViewHandle};
use pathfinder_geometry::vector::vec2f;
@@ -40,6 +40,43 @@ impl CodeReviewHeader {
right_section.add_child(git_button);
}
if code_review_header_fields.conflict_count > 0 {
right_section.add_child(
Container::new(
Flex::row()
.with_cross_axis_alignment(CrossAxisAlignment::Center)
.with_child(
ChildView::new(&code_review_header_fields.conflict_filter_button)
.finish(),
)
.with_child(
Container::new(
Text::new(
format!(
"{} unresolved",
code_review_header_fields.conflict_count
),
appearance.ui_font_family(),
appearance.ui_font_size() * 0.85,
)
.with_color(
appearance
.theme()
.sub_text_color(appearance.theme().surface_2())
.into(),
)
.finish(),
)
.with_margin_left(6.)
.finish(),
)
.finish(),
)
.with_margin_right(4.)
.finish(),
);
}
if let Some(nav_button) = &code_review_header_fields.file_nav_button {
right_section.add_child(Self::render_file_nav_button(nav_button));
}
app/src/code_review/code_review_header/mod.rs
+72 -7
View File
@@ -5,7 +5,7 @@ use galaxyui::elements::{
Align, ChildAnchor, ChildView, Clipped, ConstrainedBox, Container, CrossAxisAlignment, Flex,
Hoverable, MainAxisAlignment, MainAxisSize, MouseStateHandle, OffsetPositioning, ParentAnchor,
ParentElement, ParentOffsetBounds, Shrinkable, SizeConstraintCondition, SizeConstraintSwitch,
Stack,
Stack, Text,
};
use galaxyui::fonts::{Properties, Weight};
use galaxyui::platform::Cursor;
@@ -87,6 +87,7 @@ impl CodeReviewHeader {
code_review_header_fields: &CodeReviewHeaderFields,
app: &AppContext,
) -> Box<dyn Element> {
let theme = appearance.theme();
let mut left_section_wide = Flex::row()
.with_main_axis_size(MainAxisSize::Min)
.with_cross_axis_alignment(CrossAxisAlignment::Center);
@@ -113,8 +114,40 @@ impl CodeReviewHeader {
let mut right_section_wide = Flex::row()
.with_main_axis_alignment(MainAxisAlignment::End)
.with_cross_axis_alignment(CrossAxisAlignment::Center)
.with_child(ChildView::new(&code_review_header_fields.diff_selector).finish());
.with_cross_axis_alignment(CrossAxisAlignment::Center);
if code_review_header_fields.conflict_count > 0 {
right_section_wide.add_child(
Container::new(
Flex::row()
.with_cross_axis_alignment(CrossAxisAlignment::Center)
.with_child(
ChildView::new(&code_review_header_fields.conflict_filter_button)
.finish(),
)
.with_child(
Container::new(
Text::new(
format!(
"{} unresolved",
code_review_header_fields.conflict_count
),
appearance.ui_font_family(),
appearance.ui_font_size() * 0.85,
)
.with_color(theme.sub_text_color(theme.surface_2()).into())
.finish(),
)
.with_margin_left(6.)
.finish(),
)
.finish(),
)
.with_margin_right(4.)
.finish(),
);
}
right_section_wide
.add_child(ChildView::new(&code_review_header_fields.diff_selector).finish());
let has_no_changes = state.to_diff_stats().has_no_changes();
@@ -169,6 +202,7 @@ impl CodeReviewHeader {
code_review_header_fields: &CodeReviewHeaderFields,
app: &AppContext,
) -> Box<dyn Element> {
let theme = appearance.theme();
let mut left_section_compact = Flex::row()
.with_main_axis_size(MainAxisSize::Min)
.with_cross_axis_alignment(CrossAxisAlignment::Center);
@@ -228,12 +262,43 @@ impl CodeReviewHeader {
));
}
let right_section_compact = Flex::row()
let mut right_section_compact = Flex::row()
.with_main_axis_alignment(MainAxisAlignment::SpaceBetween)
.with_main_axis_size(MainAxisSize::Max)
.with_cross_axis_alignment(CrossAxisAlignment::Center)
.with_child(ChildView::new(&code_review_header_fields.diff_selector).finish())
.with_child(Container::new(right_subsection_compact.finish()).finish());
.with_cross_axis_alignment(CrossAxisAlignment::Center);
if code_review_header_fields.conflict_count > 0 {
right_section_compact.add_child(
Container::new(
Flex::row()
.with_cross_axis_alignment(CrossAxisAlignment::Center)
.with_child(
ChildView::new(&code_review_header_fields.conflict_filter_button)
.finish(),
)
.with_child(
Container::new(
Text::new(
format!(
"{} unresolved",
code_review_header_fields.conflict_count
),
appearance.ui_font_family(),
appearance.ui_font_size() * 0.85,
)
.with_color(theme.sub_text_color(theme.surface_2()).into())
.finish(),
)
.with_margin_left(6.)
.finish(),
)
.finish(),
)
.with_margin_right(4.)
.finish(),
);
}
right_section_compact.add_child(ChildView::new(&code_review_header_fields.diff_selector).finish());
right_section_compact.add_child(Container::new(right_subsection_compact.finish()).finish());
Clipped::new(
Shrinkable::new(
app/src/code_review/code_review_view.rs
+252 -3
View File
@@ -20,7 +20,7 @@ use pathfinder_geometry::vector::{vec2f, Vector2F};
use rand::distributions::Alphanumeric;
use rand::Rng;
use string_offset::CharOffset;
use vec1::Vec1;
use vec1::{vec1, Vec1};
use warp_editor::content::buffer::{AutoScrollBehavior, InitialBufferState, SelectionOffsets};
use warp_editor::model::CoreEditorModel;
use warp_editor::render::element::VerticalExpansionBehavior;
@@ -104,6 +104,7 @@ use crate::code_review::find_model::CodeReviewFindModel;
use crate::code_review::git_repo_model::{GitRepoModels, GitRepoStatusEvent, GitRepoStatusModel};
use crate::code_review::github_repo_model::{GitHubRepoEvent, GitHubRepoModel};
use crate::code_review::hidden_lines::calculate_hidden_lines;
use crate::code_review::merge_conflicts::{parse_conflicts, resolve_conflict, ConflictResolution};
#[cfg(feature = "local_fs")]
use crate::code_review::telemetry_event::DiffSetContextScope;
use crate::code_review::telemetry_event::{
@@ -160,6 +161,8 @@ pub struct CodeReviewHeaderFields {
pub diff_state_model: ModelHandle<DiffStateModel>,
pub maximize_button: ViewHandle<ActionButton>,
pub diff_selector: ViewHandle<DiffSelector>,
pub conflict_filter_button: ViewHandle<ActionButton>,
pub conflict_count: usize,
pub header_menu: ViewHandle<Menu<CodeReviewAction>>,
pub header_menu_open: bool,
pub header_dropdown_button: ViewHandle<ActionButton>,
@@ -353,6 +356,12 @@ pub enum CodeReviewAction {
ViewPr(String),
PublishBranch,
SubmitReviewComments,
ToggleConflictsOnly,
ResolveConflict {
path: String,
conflict_index: usize,
resolution: ConflictResolution,
},
}
pub struct FileState {
@@ -366,6 +375,10 @@ pub struct FileState {
discard_button: ViewHandle<ActionButton>,
add_context_button: ViewHandle<ActionButton>,
copy_path_button: ViewHandle<ActionButton>,
/// Stable actions for the next unresolved conflict block. The live editor
/// is reparsed before each action, so edits remain safe after typing or
/// resolving another block.
conflict_action_buttons: Option<[ViewHandle<ActionButton>; 4]>,
}
pub(crate) struct LoadedState {
@@ -612,6 +625,11 @@ pub trait ReviewActionTargetProvider {
/// State shared among the entire code review view.
pub struct CodeReviewView {
active_repo: Option<RepositoryState>,
conflict_filter_button: ViewHandle<ActionButton>,
/// When enabled, the file navigator shows only files with unresolved
/// conflict markers. This keeps merge work front and center without
/// changing the selected diff set.
conflicts_only: bool,
focus_handle: Option<PaneFocusHandle>,
maximize_button: ViewHandle<ActionButton>,
@@ -1165,6 +1183,13 @@ impl CodeReviewView {
.on_click(|ctx| ctx.dispatch_typed_action(CodeReviewAction::ToggleFileSidebar))
});
let conflict_filter_button = ctx.add_typed_action_view(|_ctx| {
ActionButton::new("Conflicts only", SecondaryTheme)
.with_size(ButtonSize::Small)
.with_tooltip("Show only files with unresolved merge conflicts")
.on_click(|ctx| ctx.dispatch_typed_action(CodeReviewAction::ToggleConflictsOnly))
});
let git_primary_action_button = ctx.add_typed_action_view(|_ctx| {
ActionButton::new("Commit", SecondaryTheme)
.with_size(ButtonSize::Small)
@@ -1320,6 +1345,8 @@ impl CodeReviewView {
let mut view = Self {
active_repo,
conflict_filter_button,
conflicts_only: false,
ui_state_handles,
diff_state_model,
focus_handle: None,
@@ -2703,6 +2730,56 @@ impl CodeReviewView {
})
});
let conflict_action_buttons = if file.file_diff.status.is_conflicted()
&& file.file_diff.conflict_count > 0
&& FeatureFlag::CodeReviewSaveChanges.is_enabled()
{
let conflict_path = file.file_diff.file_path.clone();
Some(
[
("Left", ConflictResolution::Ours),
("Right", ConflictResolution::Theirs),
("Both", ConflictResolution::Both),
("Smart", ConflictResolution::Smart),
]
.map(|(label, resolution)| {
let path = conflict_path.clone();
ctx.add_typed_action_view(move |_ctx| {
ActionButton::new(label, NakedTheme)
.with_size(ButtonSize::InlineActionHeader)
.with_tooltip(match resolution {
ConflictResolution::Ours => {
"Take left / ours for the next conflict"
}
ConflictResolution::Theirs => {
"Take right / theirs for the next conflict"
}
ConflictResolution::Both => {
"Keep both sides for the next conflict"
}
ConflictResolution::Smart => {
"Smart merge the next conflict using the common base"
}
})
.on_click({
let path = path.clone();
move |ctx| {
ctx.dispatch_typed_action(
CodeReviewAction::ResolveConflict {
path: path.clone(),
conflict_index: 0,
resolution,
},
)
}
})
})
}),
)
} else {
None
};
file_states.push(FileState {
file_diff: file.file_diff.clone(),
editor_state,
@@ -2712,6 +2789,7 @@ impl CodeReviewView {
discard_button,
add_context_button,
copy_path_button,
conflict_action_buttons,
sidebar_mouse_state: MouseStateHandle::default(),
header_mouse_state: MouseStateHandle::default(),
})
@@ -2739,7 +2817,6 @@ impl CodeReviewView {
let Some((_, file_state)) = diff_state.get_index(index) else {
return Empty::new().finish();
};
self.render_file_diff(file_state, index, scroll_offset, appearance, app)
}
@@ -2876,6 +2953,20 @@ impl CodeReviewView {
Some(&mut self.active_repo.as_mut()?.state)
}
fn loaded_state(&self) -> Option<&LoadedState> {
match self.state() {
CodeReviewViewState::Loaded(state) => Some(state),
_ => None,
}
}
fn loaded_state_mut(&mut self) -> Option<&mut LoadedState> {
match self.state_mut()? {
CodeReviewViewState::Loaded(state) => Some(state),
_ => None,
}
}
#[cfg(not(target_family = "wasm"))]
fn session_env(&self, app: &AppContext) -> Option<GitSessionState> {
let terminal_view = self.focused_terminal(app)?;
@@ -4189,6 +4280,12 @@ impl CodeReviewView {
is_in_split_pane,
maximize_button: self.maximize_button.clone(),
diff_selector: self.diff_selector.clone(),
conflict_filter_button: self.conflict_filter_button.clone(),
conflict_count: state
.file_states
.values()
.map(|file| file.file_diff.conflict_count)
.sum(),
header_menu: self.header_menu.clone(),
header_menu_open: self.header_menu_open,
diff_state_model: self.diff_state_model.clone(),
@@ -4562,7 +4659,16 @@ impl CodeReviewView {
.with_main_axis_alignment(MainAxisAlignment::Start)
.with_cross_axis_alignment(CrossAxisAlignment::Start);
for (file_index, file_state) in state.file_states.values().enumerate() {
let visible_files = state
.file_states
.values()
.enumerate()
.filter(|(_, file_state)| {
!self.conflicts_only || file_state.file_diff.conflict_count > 0
})
.collect::<Vec<_>>();
for (file_index, file_state) in visible_files {
let file_row = self.render_file_sidebar_row(file_state, appearance);
column.add_child(
Hoverable::new(file_state.sidebar_mouse_state.clone(), |mouse_state| {
@@ -4985,6 +5091,36 @@ impl CodeReviewView {
.with_main_axis_alignment(MainAxisAlignment::End)
.with_cross_axis_alignment(CrossAxisAlignment::Center);
if file.file_diff.conflict_count > 0 {
if let Some(conflict_action_buttons) = &file.conflict_action_buttons {
right_row.add_child(
Container::new(
Text::new(
"Next conflict",
appearance.ui_font_family(),
appearance.ui_font_size() * 0.85,
)
.with_color(theme.sub_text_color(theme.surface_2()).into())
.finish(),
)
.with_margin_left(4.)
.finish(),
);
for button in conflict_action_buttons {
right_row.add_child(
EventHandler::new(
Container::new(ChildView::new(button).finish())
.with_margin_left(4.)
.finish(),
)
.on_left_mouse_up(|_, _, _| DispatchEventResult::StopPropagation)
.on_left_mouse_down(|_, _, _| DispatchEventResult::StopPropagation)
.finish(),
);
}
}
}
// Add file diff as context button (before remove button)
if FeatureFlag::DiffSetAsContext.is_enabled() {
right_row.add_child(
@@ -5666,6 +5802,7 @@ impl CodeReviewView {
);
}
CodeEditorEvent::ContentChanged { origin, .. } if origin.from_user() => {
self.update_conflict_count_for_file(&file_path, &editor, ctx);
if let Some((view_handle, content_version)) = self.last_revert.take() {
let same_content_version = content_version == editor.as_ref(ctx).version(ctx);
@@ -6248,6 +6385,100 @@ impl CodeReviewView {
diff_lines.join("\n")
}
fn update_conflict_count_for_file(
&mut self,
path: &str,
editor: &ViewHandle<CodeEditorView>,
ctx: &mut ViewContext<Self>,
) {
let should_track = self
.loaded_state()
.and_then(|state| state.file_states.get(path))
.is_some_and(|file| file.file_diff.status.is_conflicted());
if !should_track {
return;
}
let count = parse_conflicts(&editor.as_ref(ctx).text(ctx).into_string()).len();
if let Some(state) = self.loaded_state_mut() {
if let Some(file) = state.file_states.get_mut(path) {
if file.file_diff.conflict_count != count {
file.file_diff.conflict_count = count;
ctx.notify();
}
}
}
if self.loaded_state().is_some_and(|state| {
state
.file_states
.iter()
.all(|(_, file)| file.file_diff.conflict_count == 0)
}) && self.conflicts_only
{
self.conflicts_only = false;
self.conflict_filter_button.update(ctx, |button, ctx| {
button.set_active(false, ctx);
});
ctx.notify();
}
}
fn resolve_conflict_for_file(
&mut self,
path: &str,
conflict_index: usize,
resolution: ConflictResolution,
ctx: &mut ViewContext<Self>,
) {
let Some(editor) = self
.loaded_state()
.and_then(|state| state.file_states.get(path))
.and_then(|file| file.editor_state.as_ref())
.map(|editor_state| editor_state.editor.clone())
else {
return;
};
let current_text = editor
.as_ref(ctx)
.editor()
.as_ref(ctx)
.text(ctx)
.into_string();
let Some((byte_range, replacement)) =
resolve_conflict(&current_text, conflict_index, resolution)
else {
return;
};
let mut resolved_text = current_text.clone();
resolved_text.replace_range(byte_range.clone(), &replacement);
let remaining_conflicts = parse_conflicts(&resolved_text).len();
editor.update(ctx, |local_editor, ctx| {
let start = CharOffset::from(current_text[..byte_range.start].chars().count());
let end = CharOffset::from(current_text[..byte_range.end].chars().count());
local_editor.editor().update(ctx, |editor, ctx| {
editor.apply_edits(vec1![(replacement, start..end)], ctx);
});
});
if let Some(state) = self.loaded_state_mut() {
if let Some(file) = state.file_states.get_mut(path) {
file.file_diff.conflict_count = remaining_conflicts;
if remaining_conflicts == 0 {
file.conflict_action_buttons = None;
}
}
}
if let Some(index) = self
.loaded_state()
.and_then(|state| state.file_states.get_index_of(path))
{
self.viewported_list_state
.invalidate_height_for_index(index);
}
ctx.notify();
}
fn save_files(&mut self, paths: &[String], ctx: &mut ViewContext<Self>) {
for path in paths {
self.save_file(path, ctx);
@@ -7200,6 +7431,24 @@ impl TypedActionView for CodeReviewView {
CodeReviewAction::SetDiffMode(mode) => {
self.apply_diff_mode(mode.clone(), ctx);
}
CodeReviewAction::ToggleConflictsOnly => {
if !self.file_sidebar_expanded {
self.open_file_sidebar(ctx);
self.update_file_nav_button_tooltip(ctx);
}
self.conflicts_only = !self.conflicts_only;
self.conflict_filter_button.update(ctx, |button, ctx| {
button.set_active(self.conflicts_only, ctx);
});
ctx.notify();
}
CodeReviewAction::ResolveConflict {
path,
conflict_index,
resolution,
} => {
self.resolve_conflict_for_file(path, *conflict_index, *resolution, ctx);
}
CodeReviewAction::ToggleFileSidebar => {
if self.file_sidebar_expanded {
self.file_sidebar_expanded = false;
app/src/code_review/code_review_view_tests.rs
+13
View File
@@ -31,6 +31,7 @@ use crate::code_review::diff_size_limits::DiffSize;
use crate::code_review::diff_state::{DiffStateModel, FileDiff, GitFileStatus};
use crate::code_review::editor_state::CodeReviewEditorState;
use crate::code_review::git_repo_model::GitRepoModels;
use crate::code_review::merge_conflicts::{parse_conflicts, resolve_conflict, ConflictResolution};
use crate::code_review::GlobalCodeReviewModel;
use crate::pane_group::WorkingDirectoriesModel;
use crate::server::server_api::team::MockTeamClient;
@@ -321,6 +322,7 @@ fn create_loaded_state_with_editors(
is_autogenerated: false,
max_line_number: 0,
has_hidden_bidi_chars: false,
conflict_count: 0,
size: DiffSize::Normal,
},
editor_state: Some(CodeReviewEditorState::new_loaded(editor)),
@@ -332,6 +334,7 @@ fn create_loaded_state_with_editors(
discard_button,
add_context_button,
copy_path_button,
conflict_action_buttons: None,
};
(file_path, state)
})
@@ -345,6 +348,16 @@ fn create_loaded_state_with_editors(
}
}
#[test]
fn test_conflict_resolution_helpers_are_safe_for_review_edits() {
let text = "before\n<<<<<<< HEAD\nours\n=======\ntheirs\n>>>>>>> feature\nafter\n";
assert_eq!(parse_conflicts(text).len(), 1);
let (range, replacement) = resolve_conflict(text, 0, ConflictResolution::Both).unwrap();
let mut resolved = text.to_string();
resolved.replace_range(range, &replacement);
assert_eq!(resolved, "before\nours\ntheirs\nafter\n");
}
#[test]
fn test_relocate_comments_empty_input() {
App::test((), |mut app| async move {
app/src/code_review/diff_menu.rs
+15 -5
View File
@@ -1,6 +1,7 @@
//! Overlay menu for the code review diff selector: pinned search input and
//! a filtered list of label-only rows with a left check slot.
use std::cmp;
use std::sync::Arc;
use fuzzy_match::{match_indices_case_insensitive, FuzzyMatchResult};
use galaxy_core::ui::theme::Fill;
@@ -85,6 +86,9 @@ pub struct CodeReviewDiffMenu {
/// current filter. Original target order is preserved; the match result
/// carries indices for bolding matched characters in the label.
filtered: Vec<(usize, Option<FuzzyMatchResult>)>,
/// Cached render rows. The Arc lets each virtualized list closure retain
/// stable data without cloning every target on every frame.
filtered_snapshot: Arc<Vec<(DiffTarget, Option<FuzzyMatchResult>)>>,
/// Index into `filtered` of the keyboard-focused row.
selected_index: Option<usize>,
search_input: ViewHandle<EditorView>,
@@ -135,6 +139,7 @@ impl CodeReviewDiffMenu {
Self {
targets: Vec::new(),
filtered: Vec::new(),
filtered_snapshot: Arc::new(Vec::new()),
selected_index: None,
search_input,
search_query: String::new(),
@@ -145,6 +150,9 @@ impl CodeReviewDiffMenu {
/// Replace the row set and reset filter/scroll to the top.
pub fn set_targets(&mut self, targets: Vec<DiffTarget>, ctx: &mut ViewContext<Self>) {
if self.targets == targets {
return;
}
self.targets = targets;
self.refresh_filtered();
self.scroll_list_to_top();
@@ -192,6 +200,12 @@ impl CodeReviewDiffMenu {
})
.collect();
}
self.filtered_snapshot = Arc::new(
self.filtered
.iter()
.filter_map(|(i, m)| self.targets.get(*i).cloned().map(|t| (t, m.clone())))
.collect(),
);
self.selected_index = if self.filtered.is_empty() {
None
} else {
@@ -302,11 +316,7 @@ impl CodeReviewDiffMenu {
let appearance = Appearance::as_ref(ctx);
let theme = appearance.theme();
let selected = self.selected_index;
let filtered_snapshot: Vec<(DiffTarget, Option<FuzzyMatchResult>)> = self
.filtered
.iter()
.filter_map(|(i, m)| self.targets.get(*i).cloned().map(|t| (t, m.clone())))
.collect();
let filtered_snapshot = self.filtered_snapshot.clone();
let filtered_len = filtered_snapshot.len();
let list = UniformList::new(
app/src/code_review/diff_selector.rs
+1 -1
View File
@@ -24,7 +24,7 @@ use crate::code_review::diff_state::DiffMode;
use crate::ui_components::icons::Icon;
/// A single selectable target in the diff selector menu.
#[derive(Debug, Clone)]
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct DiffTarget {
pub label: String,
pub mode: DiffMode,
app/src/code_review/diff_state/local.rs
+122 -59
View File
@@ -19,6 +19,7 @@ cfg_if::cfg_if! {
use std::fs;
}
}
use futures::stream::{self, StreamExt, TryStreamExt};
#[cfg(not(target_family = "wasm"))]
use galaxy_core::channel::ChannelState;
use galaxy_core::send_telemetry_from_ctx;
@@ -31,6 +32,7 @@ use galaxyui::AppContext;
use galaxyui::{ModelContext, SingletonEntity};
use crate::code_review::diff_size_limits::{compute_diff_size, DiffSize};
use crate::code_review::merge_conflicts::parse_conflicts;
use crate::code_review::{git_actions, is_file_autogenerated, CodeReviewTelemetryEvent};
use crate::features::FeatureFlag;
use crate::server::server_api::ServerApiProvider;
@@ -1783,34 +1785,14 @@ impl LocalDiffStateModel {
// Get binary file information using git diff --numstat
let binary_files = Self::get_binary_files(repo_path).await?;
// Then get the diff for each file
let mut files = Vec::new();
let mut total_additions = 0;
let mut total_deletions = 0;
for (file_path, status) in changed_files {
let is_binary = binary_files.contains(&file_path);
let mut file_diff =
Self::get_file_diff(repo_path, &file_path, &status, is_binary, None).await?;
// Never read or ship base content for binary files: it can't be
// inline-rendered and, after lossy UTF-8 decoding, can balloon ~3x.
let content_at_head = if is_binary {
None
} else {
Self::get_file_content_at_head(repo_path, &file_path, &status).await
};
file_diff.is_autogenerated =
is_file_autogenerated(&file_path, content_at_head.as_deref());
total_additions += file_diff.additions();
total_deletions += file_diff.deletions();
files.push(FileDiffAndContent {
file_diff,
content_at_head,
});
}
// Git operations are independent per file. Keep concurrency bounded so
// a large changeset gets parallel I/O without spawning an unbounded
// number of subprocesses, and collect in status order for stable UI.
let files =
Self::load_file_diffs_concurrently(repo_path, changed_files, binary_files, None)
.await?;
let total_additions = files.iter().map(|file| file.file_diff.additions()).sum();
let total_deletions = files.iter().map(|file| file.file_diff.deletions()).sum();
Ok(GitDiffWithBaseContent {
files_changed: files.len(),
@@ -1820,6 +1802,62 @@ impl LocalDiffStateModel {
})
}
async fn load_file_diffs_concurrently(
repo_path: &Path,
changed_files: Vec<(String, GitFileStatus)>,
binary_files: std::collections::HashSet<String>,
merge_base: Option<&str>,
) -> Result<Vec<FileDiffAndContent>> {
const MAX_CONCURRENT_FILE_DIFFS: usize = 8;
let repo_path = repo_path.to_path_buf();
let merge_base = merge_base.map(str::to_owned);
let files: Vec<Option<FileDiffAndContent>> =
stream::iter(changed_files.into_iter().map(|(file_path, status)| {
let repo_path = repo_path.clone();
let merge_base = merge_base.clone();
let is_binary = binary_files.contains(&file_path);
async move {
let file = match merge_base.as_deref() {
Some(base) => {
Self::file_diff_for_path(
is_binary,
&repo_path,
&file_path,
&status,
Some(base),
)
.await?
}
None => {
let mut file_diff = Self::get_file_diff(
&repo_path, &file_path, &status, is_binary, None,
)
.await?;
let content_at_head = if is_binary && !status.is_conflicted() {
None
} else {
Self::get_file_content_at_head(&repo_path, &file_path, &status)
.await
};
file_diff.is_autogenerated =
is_file_autogenerated(&file_path, content_at_head.as_deref());
Some(FileDiffAndContent {
file_diff,
content_at_head,
})
}
};
Ok::<_, anyhow::Error>(file)
}
}))
.buffered(MAX_CONCURRENT_FILE_DIFFS)
.try_collect()
.await?;
Ok(files.into_iter().flatten().collect())
}
async fn diff_state_against_base_branch(
repo_path: &Path,
should_fetch_base: bool,
@@ -1969,13 +2007,14 @@ impl LocalDiffStateModel {
&& (file_diff.hunks.is_empty() || file_diff.is_empty())
&& !status.is_renamed()
&& !status.is_new_file()
&& !status.is_conflicted()
{
return Ok(None);
}
// Never read or ship base content for binary files: it can't be
// inline-rendered and, after lossy UTF-8 decoding, can balloon ~3x.
let content_at_head = if is_binary {
let content_at_head = if file_diff.is_binary {
None
} else {
match &merge_base {
@@ -2037,9 +2076,20 @@ impl LocalDiffStateModel {
let status_files = Self::parse_git_status(&status_output)?;
// Add untracked files to the changed files list
// Add untracked files and preserve unmerged status in the branch diff.
// `git diff --name-status` can report a conflicted path as modified,
// while `git status` is authoritative for whether it needs resolving.
for (file_path, status) in status_files {
if matches!(status, GitFileStatus::Untracked) {
if status.is_conflicted() {
if let Some((_, existing_status)) = changed_files
.iter_mut()
.find(|(path, _)| path == &file_path)
{
*existing_status = status;
} else {
changed_files.push((file_path, status));
}
} else if matches!(status, GitFileStatus::Untracked) {
changed_files.push((file_path, status));
}
}
@@ -2087,29 +2137,15 @@ impl LocalDiffStateModel {
// Get binary file information using git diff --numstat against merge base
let binary_files = Self::get_binary_files_vs_commit(repo_path, &merge_base).await?;
// Get the diff for each file
let mut files = Vec::new();
let mut total_additions = 0;
let mut total_deletions = 0;
for (file_path, status) in changed_files {
let is_binary = binary_files.contains(&file_path);
let file_diff = Self::file_diff_for_path(
is_binary,
repo_path,
&file_path,
&status,
Some(&merge_base),
)
.await?;
if let Some(file_diff) = file_diff {
total_additions += file_diff.file_diff.additions();
total_deletions += file_diff.file_diff.deletions();
files.push(file_diff);
}
}
let files = Self::load_file_diffs_concurrently(
repo_path,
changed_files,
binary_files,
Some(&merge_base),
)
.await?;
let total_additions = files.iter().map(|file| file.file_diff.additions()).sum();
let total_deletions = files.iter().map(|file| file.file_diff.deletions()).sum();
Ok(GitDiffWithBaseContent {
files_changed: files.len(),
@@ -2376,9 +2412,16 @@ impl LocalDiffStateModel {
) -> Result<FileDiff> {
let mut hunks = Vec::new();
let mut max_line_number = 0;
let conflict_count = if status.is_conflicted() {
Self::conflict_count(repo_path, file_path).await
} else {
0
};
// If it's a binary file, don't fetch or parse the diff content
if is_binary {
// If it's a binary file, don't fetch or parse the diff content. A
// conflicted text file may be reported as binary by `git diff`, so
// only bypass this path when no conflict markers are present.
if is_binary && conflict_count == 0 {
return Ok(FileDiff {
file_path: file_path.to_owned(),
status: status.clone(),
@@ -2387,6 +2430,7 @@ impl LocalDiffStateModel {
is_autogenerated: false,
max_line_number: 0,
has_hidden_bidi_chars: false,
conflict_count: 0,
size: DiffSize::Normal,
});
}
@@ -2404,6 +2448,7 @@ impl LocalDiffStateModel {
is_autogenerated: false,
max_line_number: 0,
has_hidden_bidi_chars: false,
conflict_count: 0,
size: DiffSize::Normal,
});
}
@@ -2511,15 +2556,19 @@ impl LocalDiffStateModel {
"Failed to get file diff for {file_path}{}: {error}",
commit.map(|c| format!(" vs {c}")).unwrap_or_default()
);
// If diff fails, treat as binary or empty
// Unmerged files may not have a normal `git diff HEAD` patch,
// but their working-tree contents are still safe to render and
// resolve in the editor.
let is_conflicted = status.is_conflicted();
return Ok(FileDiff {
file_path: file_path.to_owned(),
status: status.clone(),
hunks: Arc::new(hunks),
is_binary: true,
is_binary: !is_conflicted,
is_autogenerated: false,
max_line_number: 0,
has_hidden_bidi_chars: false,
conflict_count,
size: DiffSize::Normal,
});
}
@@ -2530,6 +2579,7 @@ impl LocalDiffStateModel {
if diff_output
.lines()
.any(|line| line.starts_with("Binary files ") && line.contains(" differ"))
&& conflict_count == 0
{
return Ok(FileDiff {
file_path: file_path.to_owned(),
@@ -2539,6 +2589,7 @@ impl LocalDiffStateModel {
is_autogenerated: false,
max_line_number: 0,
has_hidden_bidi_chars: false,
conflict_count: 0,
size: DiffSize::Normal,
});
}
@@ -2566,14 +2617,26 @@ impl LocalDiffStateModel {
file_path: file_path.to_owned(),
status: status.clone(),
hunks: Arc::new(hunks),
is_binary,
is_binary: is_binary && conflict_count == 0,
is_autogenerated: false,
max_line_number,
has_hidden_bidi_chars,
conflict_count,
size,
})
}
async fn conflict_count(repo_path: &Path, file_path: &str) -> usize {
let path = repo_path.join(file_path);
if !path.is_file() {
return 0;
}
async_fs::read_to_string(path)
.await
.map(|content| parse_conflicts(&content).len())
.unwrap_or(0)
}
/// Parses diff hunks from git diff output
pub(crate) fn parse_diff_hunks(diff_output: &str) -> Result<Vec<DiffHunk>> {
let mut hunks = Vec::new();
app/src/code_review/diff_state/mod.rs
+8
View File
@@ -109,6 +109,10 @@ impl GitFileStatus {
pub fn is_new_file(&self) -> bool {
matches!(self, Self::New | Self::Untracked)
}
pub fn is_conflicted(&self) -> bool {
matches!(self, Self::Conflicted)
}
}
#[derive(Clone, Debug)]
@@ -177,6 +181,10 @@ pub struct FileDiff {
pub is_autogenerated: bool,
pub max_line_number: usize,
pub has_hidden_bidi_chars: bool,
/// Number of complete conflict-marker blocks in the working-tree file.
/// This is metadata only; the live editor remains the source of truth for
/// applying resolutions.
pub conflict_count: usize,
pub size: DiffSize,
}
app/src/code_review/diff_state/remote_tests.rs
+2
View File
@@ -99,6 +99,7 @@ fn simple_file_with_content(path: &str, content_at_base: Option<&str>) -> FileDi
is_autogenerated: false,
max_line_number: 10,
has_hidden_bidi_chars: false,
conflict_count: 0,
size: DiffSize::Normal,
},
content_at_head: content_at_base.map(str::to_string),
@@ -691,6 +692,7 @@ fn apply_file_delta_none_removes_file() {
is_autogenerated: false,
max_line_number: 0,
has_hidden_bidi_chars: false,
conflict_count: 0,
size: DiffSize::Normal,
}],
total_additions: 0,
app/src/code_review/merge_conflicts.rs
+323
View File
@@ -0,0 +1,323 @@
//! Parsing and resolution helpers for Git's conflict-marker format.
//!
//! This is deliberately separate from unified-diff parsing. A conflicted
//! working tree contains the current file with marker blocks, while a unified
//! diff describes two versions of a file. Keeping the representations separate
//! makes the merge actions safe to apply to the live editor buffer.
use std::ops::Range;
use similar::{DiffOp, TextDiff};
#[derive(Clone, Debug, Eq, PartialEq)]
pub(crate) struct ConflictBlock {
/// Byte range in the original text covering the complete marker block.
pub(crate) byte_range: Range<usize>,
/// One-indexed line containing the opening marker.
pub(crate) start_line: usize,
pub(crate) ours: String,
pub(crate) base: Option<String>,
pub(crate) theirs: String,
}
impl ConflictBlock {
/// Apply Git's common three-way merge rules. When both sides changed the
/// same base differently, preserving both sides is safer than silently
/// choosing one.
pub(crate) fn smart_merge(&self) -> String {
let Some(base) = self.base.as_deref() else {
return if self.ours == self.theirs {
self.ours.clone()
} else {
merge_both(&self.ours, &self.theirs)
};
};
if self.ours == self.theirs {
return self.ours.clone();
}
if self.ours == base {
return self.theirs.clone();
}
if self.theirs == base {
return self.ours.clone();
}
three_way_merge(base, &self.ours, &self.theirs)
.unwrap_or_else(|| merge_both(&self.ours, &self.theirs))
}
pub(crate) fn resolve(&self, resolution: ConflictResolution) -> String {
match resolution {
ConflictResolution::Ours => self.ours.clone(),
ConflictResolution::Theirs => self.theirs.clone(),
ConflictResolution::Both => merge_both(&self.ours, &self.theirs),
ConflictResolution::Smart => self.smart_merge(),
}
}
}
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub(crate) enum ConflictResolution {
Ours,
Theirs,
Both,
Smart,
}
/// Return all complete conflict blocks in `text`, preserving each side's
/// original line endings. Malformed or incomplete marker sequences are ignored
/// so a partially written file is never destructively rewritten.
pub(crate) fn parse_conflicts(text: &str) -> Vec<ConflictBlock> {
let lines = text_lines(text);
let mut conflicts = Vec::new();
let mut index = 0;
while index < lines.len() {
let (line_start, _, line) = lines[index];
if !line_content(line).starts_with("<<<<<<<") {
index += 1;
continue;
}
let start_line = index + 1;
let mut cursor = index + 1;
let mut ours = String::new();
while cursor < lines.len() {
let content = line_content(lines[cursor].2);
if content.starts_with("|||||||") || content == "=======" {
break;
}
if content.starts_with("<<<<<<<") || content.starts_with(">>>>>>>") {
break;
}
ours.push_str(lines[cursor].2);
cursor += 1;
}
let mut base = None;
if cursor < lines.len() && line_content(lines[cursor].2).starts_with("|||||||") {
cursor += 1;
let mut base_content = String::new();
while cursor < lines.len() {
let content = line_content(lines[cursor].2);
if content == "=======" || content.starts_with(">>>>>>>") {
break;
}
base_content.push_str(lines[cursor].2);
cursor += 1;
}
base = Some(base_content);
}
if cursor >= lines.len() || line_content(lines[cursor].2) != "=======" {
index += 1;
continue;
}
cursor += 1;
let mut theirs = String::new();
while cursor < lines.len() {
let content = line_content(lines[cursor].2);
if content.starts_with(">>>>>>>") {
break;
}
if content.starts_with("<<<<<<<") {
break;
}
theirs.push_str(lines[cursor].2);
cursor += 1;
}
if cursor >= lines.len() || !line_content(lines[cursor].2).starts_with(">>>>>>>") {
index += 1;
continue;
}
conflicts.push(ConflictBlock {
byte_range: line_start..lines[cursor].1,
start_line,
ours,
base,
theirs,
});
index = cursor + 1;
}
conflicts
}
/// Resolve one block in `text`, returning the complete replacement text.
/// Callers should reparse after every edit because previous resolutions change
/// subsequent offsets.
pub(crate) fn resolve_conflict(
text: &str,
conflict_index: usize,
resolution: ConflictResolution,
) -> Option<(Range<usize>, String)> {
let conflict = parse_conflicts(text).into_iter().nth(conflict_index)?;
let replacement = conflict.resolve(resolution);
Some((conflict.byte_range, replacement))
}
#[derive(Debug)]
struct LineEdit {
base_range: Range<usize>,
replacement: String,
}
fn three_way_merge(base: &str, ours: &str, theirs: &str) -> Option<String> {
let base_lines = split_lines(base);
let ours_lines = split_lines(ours);
let theirs_lines = split_lines(theirs);
let ours_edits = line_edits(&base_lines, &ours_lines);
let theirs_edits = line_edits(&base_lines, &theirs_lines);
let mut merged = String::new();
let mut base_index = 0;
let mut ours_index = 0;
let mut theirs_index = 0;
while ours_index < ours_edits.len() || theirs_index < theirs_edits.len() {
let ours_edit = ours_edits.get(ours_index);
let theirs_edit = theirs_edits.get(theirs_index);
let next_start = match (ours_edit, theirs_edit) {
(Some(ours), Some(theirs)) => ours.base_range.start.min(theirs.base_range.start),
(Some(ours), None) => ours.base_range.start,
(None, Some(theirs)) => theirs.base_range.start,
(None, None) => break,
};
merged.push_str(&base_lines[base_index..next_start].concat());
match (ours_edit, theirs_edit) {
(Some(ours), Some(theirs)) if ranges_overlap(&ours.base_range, &theirs.base_range) => {
if ours.base_range == theirs.base_range && ours.replacement == theirs.replacement {
merged.push_str(&ours.replacement);
base_index = ours.base_range.end;
ours_index += 1;
theirs_index += 1;
} else {
return None;
}
}
(Some(ours), Some(theirs)) if ours.base_range.start <= theirs.base_range.start => {
merged.push_str(&ours.replacement);
base_index = ours.base_range.end;
ours_index += 1;
}
(Some(_), Some(theirs)) => {
merged.push_str(&theirs.replacement);
base_index = theirs.base_range.end;
theirs_index += 1;
}
(Some(ours), None) => {
merged.push_str(&ours.replacement);
base_index = ours.base_range.end;
ours_index += 1;
}
(None, Some(theirs)) => {
merged.push_str(&theirs.replacement);
base_index = theirs.base_range.end;
theirs_index += 1;
}
(None, None) => unreachable!(),
}
}
merged.push_str(&base_lines[base_index..].concat());
Some(merged)
}
fn line_edits(base: &[&str], changed: &[&str]) -> Vec<LineEdit> {
TextDiff::configure()
.algorithm(similar::Algorithm::Patience)
.diff_lines(&base.concat(), &changed.concat())
.ops()
.iter()
.filter_map(|op| match op {
DiffOp::Equal { .. } => None,
DiffOp::Delete {
old_index, old_len, ..
} => Some(LineEdit {
base_range: *old_index..*old_index + *old_len,
replacement: String::new(),
}),
DiffOp::Insert {
old_index,
new_index,
new_len,
} => Some(LineEdit {
base_range: *old_index..*old_index,
replacement: changed[*new_index..*new_index + *new_len].concat(),
}),
DiffOp::Replace {
old_index,
old_len,
new_index,
new_len,
} => Some(LineEdit {
base_range: *old_index..*old_index + *old_len,
replacement: changed[*new_index..*new_index + *new_len].concat(),
}),
})
.collect()
}
fn ranges_overlap(left: &Range<usize>, right: &Range<usize>) -> bool {
if left.start == left.end || right.start == right.end {
left.start == right.start
} else {
left.start < right.end && right.start < left.end
}
}
fn split_lines(text: &str) -> Vec<&str> {
let mut lines: Vec<_> = text.split_inclusive('\n').collect();
if lines.last().is_some_and(|line| !line.ends_with('\n')) {
return lines;
}
if text.is_empty() {
lines.clear();
}
lines
}
fn merge_both(ours: &str, theirs: &str) -> String {
if ours.is_empty() {
return theirs.to_string();
}
if theirs.is_empty() {
return ours.to_string();
}
if ours.ends_with('\n') || theirs.starts_with('\n') {
format!("{ours}{theirs}")
} else {
format!("{ours}\n{theirs}")
}
}
fn line_content(line: &str) -> &str {
line.strip_suffix('\n')
.unwrap_or(line)
.strip_suffix('\r')
.unwrap_or_else(|| line.strip_suffix('\n').unwrap_or(line))
}
fn text_lines(text: &str) -> Vec<(usize, usize, &str)> {
let mut lines = Vec::new();
let mut start = 0;
for line in text.split_inclusive('\n') {
let end = start + line.len();
lines.push((start, end, line));
start = end;
}
if start < text.len() || text.is_empty() {
lines.push((start, text.len(), &text[start..]));
}
lines
}
#[cfg(test)]
#[path = "merge_conflicts_tests.rs"]
mod tests;
app/src/code_review/merge_conflicts_tests.rs
+54
View File
@@ -0,0 +1,54 @@
use super::{parse_conflicts, resolve_conflict, ConflictResolution};
#[test]
fn parses_multiple_conflicts_and_optional_base() {
let text = "before\n<<<<<<< HEAD\nours\n||||||| base\nbase\n=======\ntheirs\n>>>>>>> feature\nafter\n<<<<<<< HEAD\nleft\n=======\nright\n>>>>>>> feature\n";
let conflicts = parse_conflicts(text);
assert_eq!(conflicts.len(), 2);
assert_eq!(conflicts[0].start_line, 2);
assert_eq!(conflicts[0].ours, "ours\n");
assert_eq!(conflicts[0].base.as_deref(), Some("base\n"));
assert_eq!(conflicts[0].theirs, "theirs\n");
assert_eq!(conflicts[1].base, None);
}
#[test]
fn ignores_incomplete_conflict_blocks() {
let text = "before\n<<<<<<< HEAD\nours\n=======\ntheirs\n";
assert!(parse_conflicts(text).is_empty());
}
#[test]
fn resolves_a_conflict_using_byte_ranges() {
let text = "prefix\n<<<<<<< HEAD\n左\n=======\n右\n>>>>>>> feature\nsuffix\n";
let (range, replacement) = resolve_conflict(text, 0, ConflictResolution::Theirs)
.expect("complete conflict should resolve");
assert_eq!(
&text[range],
"<<<<<<< HEAD\n左\n=======\n右\n>>>>>>> feature\n"
);
assert_eq!(replacement, "右\n");
}
#[test]
fn smart_merge_prefers_the_changed_side_when_other_side_matches_base() {
let text = "<<<<<<< HEAD\nbase\n||||||| base\nbase\n=======\nchanged\n>>>>>>> feature\n";
let (_, replacement) = resolve_conflict(text, 0, ConflictResolution::Smart).unwrap();
assert_eq!(replacement, "changed\n");
}
#[test]
fn smart_merge_preserves_both_for_divergent_changes() {
let text = "<<<<<<< HEAD\nours\n||||||| base\nbase\n=======\ntheirs\n>>>>>>> feature\n";
let (_, replacement) = resolve_conflict(text, 0, ConflictResolution::Smart).unwrap();
assert_eq!(replacement, "ours\ntheirs\n");
}
#[test]
fn smart_merge_combines_non_overlapping_line_edits() {
let text = "<<<<<<< HEAD\none\nours-two\nthree\n||||||| base\none\ntwo\nthree\n=======\none\ntwo\ntheirs-three\n>>>>>>> feature\n";
let (_, replacement) = resolve_conflict(text, 0, ConflictResolution::Smart).unwrap();
assert_eq!(replacement, "one\nours-two\ntheirs-three\n");
}
app/src/code_review/mod.rs
+1
View File
@@ -23,6 +23,7 @@ pub(crate) mod diff_menu;
pub(crate) mod diff_selector;
#[cfg_attr(not(feature = "local_fs"), allow(dead_code))]
pub(crate) mod file_invalidation_queue;
pub(crate) mod merge_conflicts;
use std::path::{Path, PathBuf};
app/src/remote_server/diff_state_proto.rs
+2
View File
@@ -263,6 +263,7 @@ impl TryFrom<&proto::FileDiff> for FileDiff {
is_autogenerated: file.is_autogenerated,
max_line_number: file.max_line_number as usize,
has_hidden_bidi_chars: file.has_hidden_bidi_chars,
conflict_count: file.conflict_count as usize,
size,
})
}
@@ -630,6 +631,7 @@ pub fn file_diff_to_proto(f: &FileDiff, content_at_base: Option<&str>) -> proto:
is_autogenerated: f.is_autogenerated,
max_line_number: f.max_line_number as u64,
has_hidden_bidi_chars: f.has_hidden_bidi_chars,
conflict_count: f.conflict_count as u64,
size: proto::DiffSize::from(&size).into(),
content_at_base: content_at_base.map(|s| s.to_string()),
}
app/src/remote_server/diff_state_proto_tests.rs
+3
View File
@@ -119,6 +119,7 @@ fn file_diff_to_proto_preserves_repo_relative_path() {
is_autogenerated: false,
max_line_number: 0,
has_hidden_bidi_chars: false,
conflict_count: 0,
size: DiffSize::Normal,
};
@@ -138,6 +139,7 @@ fn build_diff_state_snapshot_preserves_repo_relative_file_paths() {
is_autogenerated: false,
max_line_number: 0,
has_hidden_bidi_chars: false,
conflict_count: 0,
size: DiffSize::Normal,
},
content_at_head: None,
@@ -187,6 +189,7 @@ fn text_file_diff(file_path: &str) -> FileDiff {
is_autogenerated: false,
max_line_number: 0,
has_hidden_bidi_chars: false,
conflict_count: 0,
size: DiffSize::Normal,
}
}