From b4fe63b097be0f410e358cd0664ce1fc7b4e7290 Mon Sep 17 00:00:00 2001 From: Anthony Eid <56899983+Anthony-Eid@users.noreply.github.com> Date: Wed, 25 Feb 2026 20:45:01 +0100 Subject: [PATCH] git_graph: Polish UX (#50123) This is a follow-up on #50027 I address my comments by adding a hash map look-up to find the selected pending commit. I also removed the limitation where we would only retry finding the pending commit 5 times. The pending selection is removed when the graph is fully loaded and doesn't contain the pending commit. This PR also cleans up some internal code structure and starts work to enable search and propagating git log error messages to the UI. UI wise I made the git graph item show the repository name instead of "Git Graph" in Zed. Before you mark this PR as ready for review, make sure that you have: - [ ] Added a solid test coverage and/or screenshots from doing manual testing - [x] Done a self-review taking into account security and performance aspects - [x] Aligned any UI changes with the [UI checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) Release Notes: - N/A --------- Co-authored-by: Remco Smits --- crates/git_graph/src/git_graph.rs | 155 ++++++++++++++++++----------- crates/project/src/git_store.rs | 157 ++++++++++++++++++++---------- 2 files changed, 206 insertions(+), 106 deletions(-) diff --git a/crates/git_graph/src/git_graph.rs b/crates/git_graph/src/git_graph.rs index 3bdb2b0d717ca4cae181fee9dd690755e29075d0..0052d58f5985a29f11043f0bd97edb76bb8d2124 100644 --- a/crates/git_graph/src/git_graph.rs +++ b/crates/git_graph/src/git_graph.rs @@ -18,7 +18,10 @@ use language::line_diff; use menu::{Cancel, SelectNext, SelectPrevious}; use project::{ Project, - git_store::{CommitDataState, GitStoreEvent, Repository, RepositoryEvent, RepositoryId}, + git_store::{ + CommitDataState, GitGraphEvent, GitStoreEvent, GraphDataResponse, Repository, + RepositoryEvent, RepositoryId, + }, }; use settings::Settings; use smallvec::{SmallVec, smallvec}; @@ -48,7 +51,6 @@ const LANE_WIDTH: Pixels = px(16.0); const LEFT_PADDING: Pixels = px(12.0); const LINE_WIDTH: Pixels = px(1.5); const RESIZE_HANDLE_WIDTH: f32 = 8.0; -const PENDING_SELECT_MAX_RETRIES: usize = 5; const COPIED_STATE_DURATION: Duration = Duration::from_secs(2); struct CopiedState { @@ -853,7 +855,7 @@ pub struct GitGraph { commit_details_split_state: Entity, selected_repo_id: Option, changed_files_scroll_handle: UniformListScrollHandle, - pending_select_sha: Option<(String, usize)>, + pending_select_sha: Option, } impl GitGraph { @@ -965,20 +967,62 @@ impl GitGraph { cx: &mut Context, ) { match event { - RepositoryEvent::GitGraphCountUpdated((order, source), commit_count) => { - if order != &self.log_order || source != &self.log_source { - return; - } + RepositoryEvent::GraphEvent((source, order), event) + if source == &self.log_source && order == &self.log_order => + { + match event { + GitGraphEvent::FullyLoaded => { + if let Some(pending_sha_index) = + self.pending_select_sha.take().and_then(|oid| { + repository + .read(cx) + .get_graph_data(source.clone(), *order) + .and_then(|data| data.commit_oid_to_index.get(&oid).copied()) + }) + { + self.select_entry(pending_sha_index, cx); + } + } + GitGraphEvent::LoadingError => { + // todo(git_graph): Wire this up with the UI + } + GitGraphEvent::CountUpdated(commit_count) => { + let old_count = self.graph_data.commits.len(); + + if let Some(pending_selection_index) = + repository.update(cx, |repository, cx| { + let GraphDataResponse { + commits, + is_loading, + error: _, + } = repository.graph_data( + source.clone(), + *order, + old_count..*commit_count, + cx, + ); + self.graph_data.add_commits(commits); - let old_count = self.graph_data.commits.len(); + let pending_sha_index = self.pending_select_sha.and_then(|oid| { + repository.get_graph_data(source.clone(), *order).and_then( + |data| data.commit_oid_to_index.get(&oid).copied(), + ) + }); - repository.update(cx, |repository, cx| { - let (commits, _) = - repository.graph_data(source.clone(), *order, old_count..*commit_count, cx); - self.graph_data.add_commits(commits); - }); - cx.notify(); - self.retry_pending_select(cx); + if !is_loading && pending_sha_index.is_none() { + self.pending_select_sha.take(); + } + + pending_sha_index + }) + { + self.select_entry(pending_selection_index, cx); + self.pending_select_sha.take(); + } + + cx.notify(); + } + } } RepositoryEvent::BranchChanged | RepositoryEvent::MergeHeadsChanged => { self.pending_select_sha = None; @@ -990,6 +1034,7 @@ impl GitGraph { cx.notify(); } } + RepositoryEvent::GraphEvent(_, _) => {} _ => {} } } @@ -997,12 +1042,9 @@ impl GitGraph { fn fetch_initial_graph_data(&mut self, cx: &mut App) { if let Some(repository) = self.get_selected_repository(cx) { repository.update(cx, |repository, cx| { - let (commits, _) = repository.graph_data( - self.log_source.clone(), - self.log_order, - 0..usize::MAX, - cx, - ); + let commits = repository + .graph_data(self.log_source.clone(), self.log_order, 0..usize::MAX, cx) + .commits; self.graph_data.add_commits(commits); }); } @@ -1145,6 +1187,10 @@ impl GitGraph { } } + fn confirm(&mut self, _: &menu::Confirm, window: &mut Window, cx: &mut Context) { + self.open_selected_commit_view(window, cx); + } + fn select_entry(&mut self, idx: usize, cx: &mut Context) { if self.selected_entry_idx == Some(idx) { return; @@ -1193,31 +1239,21 @@ impl GitGraph { let Ok(oid) = sha.parse::() else { return; }; - for (idx, commit) in self.graph_data.commits.iter().enumerate() { - if commit.data.sha == oid { - self.pending_select_sha = None; - self.select_entry(idx, cx); - return; - } - } - self.pending_select_sha = Some((sha.to_string(), PENDING_SELECT_MAX_RETRIES)); - } - fn retry_pending_select(&mut self, cx: &mut Context) { - let Some((sha, retries_remaining)) = self.pending_select_sha.take() else { + let Some(selected_repository) = self.get_selected_repository(cx) else { return; }; - if let Ok(oid) = sha.parse::() { - for (idx, commit) in self.graph_data.commits.iter().enumerate() { - if commit.data.sha == oid { - self.select_entry(idx, cx); - return; - } - } - } - if retries_remaining > 0 { - self.pending_select_sha = Some((sha, retries_remaining - 1)); - } + + let Some(index) = selected_repository + .read(cx) + .get_graph_data(self.log_source.clone(), self.log_order) + .and_then(|data| data.commit_oid_to_index.get(&oid)) + .copied() + else { + return; + }; + + self.select_entry(index, cx); } fn open_selected_commit_view(&mut self, window: &mut Window, cx: &mut Context) { @@ -2033,7 +2069,11 @@ impl Render for GitGraph { if let Some(repository) = self.get_selected_repository(cx) { repository.update(cx, |repository, cx| { // Start loading the graph data if we haven't started already - let (commits, is_loading) = repository.graph_data( + let GraphDataResponse { + commits, + is_loading, + error: _, + } = repository.graph_data( self.log_source.clone(), self.log_order, 0..usize::MAX, @@ -2212,16 +2252,17 @@ impl Render for GitGraph { }; div() - .size_full() - .bg(cx.theme().colors().editor_background) .key_context("GitGraph") .track_focus(&self.focus_handle) + .size_full() + .bg(cx.theme().colors().editor_background) .on_action(cx.listener(|this, _: &OpenCommitView, window, cx| { this.open_selected_commit_view(window, cx); })) .on_action(cx.listener(Self::cancel)) .on_action(cx.listener(Self::select_prev)) .on_action(cx.listener(Self::select_next)) + .on_action(cx.listener(Self::confirm)) .child(content) .children(self.context_menu.as_ref().map(|(menu, position, _)| { deferred( @@ -2270,8 +2311,15 @@ impl Item for GitGraph { })))) } - fn tab_content_text(&self, _detail: usize, _cx: &App) -> SharedString { - "Git Graph".into() + fn tab_content_text(&self, _detail: usize, cx: &App) -> SharedString { + self.get_selected_repository(cx) + .and_then(|repo| { + repo.read(cx) + .work_directory_abs_path + .file_name() + .map(|name| name.to_string_lossy().to_string()) + }) + .map_or_else(|| "Git Graph".into(), |name| SharedString::from(name)) } fn show_toolbar(&self) -> bool { @@ -3049,7 +3097,7 @@ mod tests { 0..usize::MAX, cx, ) - .0 + .commits .to_vec() }); @@ -3132,13 +3180,10 @@ mod tests { .any(|event| matches!(event, RepositoryEvent::MergeHeadsChanged)), "initial repository scan should emit MergeHeadsChanged" ); - - let graph_data_key = (crate::LogOrder::default(), crate::LogSource::default()); let commit_count_after = repository.read_with(cx, |repo, _| { - repo.initial_graph_data - .get(&graph_data_key) - .map(|(_, data)| data.len()) - .unwrap_or(0) + repo.get_graph_data(crate::LogSource::default(), crate::LogOrder::default()) + .map(|data| data.commit_data.len()) + .unwrap() }); assert_eq!( commits.len(), diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 67bc21c94227e8f53356ef1b7f626ff922326d29..3113163cbaec65d7b439e0cbf46603d60ac3fae0 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -60,7 +60,7 @@ use settings::WorktreeId; use smol::future::yield_now; use std::{ cmp::Ordering, - collections::{BTreeSet, HashSet, VecDeque}, + collections::{BTreeSet, HashSet, VecDeque, hash_map::Entry}, future::Future, mem, ops::Range, @@ -296,6 +296,19 @@ enum GraphCommitHandlerState { Closed, } +pub struct InitialGitGraphData { + fetch_task: Task<()>, + pub error: Option, + pub commit_data: Vec>, + pub commit_oid_to_index: HashMap, +} + +pub struct GraphDataResponse<'a> { + pub commits: &'a [Arc], + pub is_loading: bool, + pub error: Option, +} + pub struct Repository { this: WeakEntity, snapshot: RepositorySnapshot, @@ -311,13 +324,7 @@ pub struct Repository { askpass_delegates: Arc>>, latest_askpass_id: u64, repository_state: Shared>>, - pub initial_graph_data: HashMap< - (LogOrder, LogSource), - ( - Task>, - Vec>, - ), - >, + initial_graph_data: HashMap<(LogSource, LogOrder), InitialGitGraphData>, graph_commit_data_handler: GraphCommitHandlerState, commit_data: HashMap, } @@ -390,6 +397,13 @@ pub enum RepositoryState { Remote(RemoteRepositoryState), } +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum GitGraphEvent { + CountUpdated(usize), + FullyLoaded, + LoadingError, +} + #[derive(Clone, Debug, PartialEq, Eq)] pub enum RepositoryEvent { StatusesChanged, @@ -397,7 +411,7 @@ pub enum RepositoryEvent { BranchChanged, StashEntriesChanged, PendingOpsChanged { pending_ops: SumTree }, - GitGraphCountUpdated((LogOrder, LogSource), usize), + GraphEvent((LogSource, LogOrder), GitGraphEvent), } #[derive(Clone, Debug)] @@ -4404,47 +4418,82 @@ impl Repository { }) } + pub fn get_graph_data( + &self, + log_source: LogSource, + log_order: LogOrder, + ) -> Option<&InitialGitGraphData> { + self.initial_graph_data.get(&(log_source, log_order)) + } + pub fn graph_data( &mut self, log_source: LogSource, log_order: LogOrder, range: Range, cx: &mut Context, - ) -> (&[Arc], bool) { - let (loading_task, initial_commit_data) = self + ) -> GraphDataResponse<'_> { + let initial_commit_data = self .initial_graph_data - .entry((log_order, log_source.clone())) + .entry((log_source.clone(), log_order)) .or_insert_with(|| { let state = self.repository_state.clone(); let log_source = log_source.clone(); - ( - cx.spawn(async move |repository, cx| { - let state = state.await; - match state { - Ok(RepositoryState::Local(LocalRepositoryState { - backend, .. - })) => { - Self::local_git_graph_data( - repository, backend, log_source, log_order, cx, - ) - .await - } - Ok(RepositoryState::Remote(_)) => { - Err("Git graph is not supported for collab yet".into()) - } - Err(e) => Err(SharedString::from(e)), + + let fetch_task = cx.spawn(async move |repository, cx| { + let state = state.await; + let result = match state { + Ok(RepositoryState::Local(LocalRepositoryState { backend, .. })) => { + Self::local_git_graph_data( + repository.clone(), + backend, + log_source.clone(), + log_order, + cx, + ) + .await } - }), - vec![], - ) + Ok(RepositoryState::Remote(_)) => { + Err("Git graph is not supported for collab yet".into()) + } + Err(e) => Err(SharedString::from(e)), + }; + + if let Err(fetch_task_error) = result { + repository + .update(cx, |repository, _| { + if let Some(data) = repository + .initial_graph_data + .get_mut(&(log_source, log_order)) + { + data.error = Some(fetch_task_error); + } else { + debug_panic!( + "This task would be dropped if this entry doesn't exist" + ); + } + }) + .ok(); + } + }); + + InitialGitGraphData { + fetch_task, + error: None, + commit_data: Vec::new(), + commit_oid_to_index: HashMap::default(), + } }); - let max_start = initial_commit_data.len().saturating_sub(1); - let max_end = initial_commit_data.len(); - ( - &initial_commit_data[range.start.min(max_start)..range.end.min(max_end)], - !loading_task.is_ready(), - ) + let max_start = initial_commit_data.commit_data.len().saturating_sub(1); + let max_end = initial_commit_data.commit_data.len(); + + GraphDataResponse { + commits: &initial_commit_data.commit_data + [range.start.min(max_start)..range.end.min(max_end)], + is_loading: !initial_commit_data.fetch_task.is_ready(), + error: initial_commit_data.error.clone(), + } } async fn local_git_graph_data( @@ -4467,32 +4516,38 @@ impl Repository { } }); - let graph_data_key = (log_order, log_source.clone()); + let graph_data_key = (log_source, log_order); while let Ok(initial_graph_commit_data) = request_rx.recv().await { this.update(cx, |repository, cx| { let graph_data = repository .initial_graph_data - .get_mut(&graph_data_key) - .map(|(_, graph_data)| graph_data); - debug_assert!( - graph_data.is_some(), - "This task should be dropped if data doesn't exist" - ); + .entry(graph_data_key.clone()) + .and_modify(|graph_data| { + for commit_data in initial_graph_commit_data { + graph_data + .commit_oid_to_index + .insert(commit_data.sha, graph_data.commit_data.len()); + graph_data.commit_data.push(commit_data); + + cx.emit(RepositoryEvent::GraphEvent( + graph_data_key.clone(), + GitGraphEvent::CountUpdated(graph_data.commit_data.len()), + )); + } + }); - if let Some(graph_data) = graph_data { - graph_data.extend(initial_graph_commit_data); - cx.emit(RepositoryEvent::GitGraphCountUpdated( - graph_data_key.clone(), - graph_data.len(), - )); + match &graph_data { + Entry::Occupied(_) => {} + Entry::Vacant(_) => { + debug_panic!("This task should be dropped if data doesn't exist"); + } } }) .ok(); } task.await?; - Ok(()) }