git_graph: Polish UX (#50123)

Anthony Eid and Remco Smits created

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 <djsmits12@gmail.com>

Change summary

crates/git_graph/src/git_graph.rs | 155 +++++++++++++++++++++-----------
crates/project/src/git_store.rs   | 157 ++++++++++++++++++++++----------
2 files changed, 206 insertions(+), 106 deletions(-)

Detailed changes

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<SplitState>,
     selected_repo_id: Option<RepositoryId>,
     changed_files_scroll_handle: UniformListScrollHandle,
-    pending_select_sha: Option<(String, usize)>,
+    pending_select_sha: Option<Oid>,
 }
 
 impl GitGraph {
@@ -965,20 +967,62 @@ impl GitGraph {
         cx: &mut Context<Self>,
     ) {
         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>) {
+        self.open_selected_commit_view(window, cx);
+    }
+
     fn select_entry(&mut self, idx: usize, cx: &mut Context<Self>) {
         if self.selected_entry_idx == Some(idx) {
             return;
@@ -1193,31 +1239,21 @@ impl GitGraph {
         let Ok(oid) = sha.parse::<Oid>() 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<Self>) {
-        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::<Oid>() {
-            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<Self>) {
@@ -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(),

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<SharedString>,
+    pub commit_data: Vec<Arc<InitialGraphCommitData>>,
+    pub commit_oid_to_index: HashMap<Oid, usize>,
+}
+
+pub struct GraphDataResponse<'a> {
+    pub commits: &'a [Arc<InitialGraphCommitData>],
+    pub is_loading: bool,
+    pub error: Option<SharedString>,
+}
+
 pub struct Repository {
     this: WeakEntity<Self>,
     snapshot: RepositorySnapshot,
@@ -311,13 +324,7 @@ pub struct Repository {
     askpass_delegates: Arc<Mutex<HashMap<u64, AskPassDelegate>>>,
     latest_askpass_id: u64,
     repository_state: Shared<Task<Result<RepositoryState, String>>>,
-    pub initial_graph_data: HashMap<
-        (LogOrder, LogSource),
-        (
-            Task<Result<(), SharedString>>,
-            Vec<Arc<InitialGraphCommitData>>,
-        ),
-    >,
+    initial_graph_data: HashMap<(LogSource, LogOrder), InitialGitGraphData>,
     graph_commit_data_handler: GraphCommitHandlerState,
     commit_data: HashMap<Oid, CommitDataState>,
 }
@@ -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<PendingOps> },
-    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<usize>,
         cx: &mut Context<Self>,
-    ) -> (&[Arc<InitialGraphCommitData>], 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(())
     }