diff --git a/crates/workspace/src/views/repo_detail/diff.rs b/crates/workspace/src/views/repo_detail/diff.rs index a03da9b..bcd6474 100644 --- a/crates/workspace/src/views/repo_detail/diff.rs +++ b/crates/workspace/src/views/repo_detail/diff.rs @@ -325,8 +325,6 @@ pub struct CommitDiffView { error: Option, /// Changed-files explorer and per-file diff, also used by the new PR panel's compare view. pane: Entity, - /// In-flight tasks, pruned on every push. - tasks: Vec>>, } impl CommitDiffView { @@ -358,7 +356,6 @@ impl CommitDiffView { loading: true, error: None, pane, - tasks: Vec::new(), } } @@ -371,42 +368,43 @@ impl CommitDiffView { let worktree = self.worktree.clone(); let id = self.commit.id.clone(); - let task = cx.spawn_in(window, async move |this, cx| { - let commit = cx - .background_spawn({ - let worktree = worktree.clone(); - let id = id.clone(); - async move { signed_git::worktree_commit(&worktree, &id) } - }) - .await; - let diff = cx - .background_spawn({ - let worktree = worktree.clone(); - let id = id.clone(); - async move { signed_git::worktree_commit_diff(&worktree, &id) } - }) - .await; + let task: gpui::Task> = + cx.spawn_in(window, async move |this, cx| { + let commit = cx + .background_spawn({ + let worktree = worktree.clone(); + let id = id.clone(); + async move { signed_git::worktree_commit(&worktree, &id) } + }) + .await; + let diff = cx + .background_spawn({ + let worktree = worktree.clone(); + let id = id.clone(); + async move { signed_git::worktree_commit_diff(&worktree, &id) } + }) + .await; - this.update_in(cx, |this, _window, cx| { - this.loading = false; - if let Ok(Some(commit)) = commit { - this.commit = commit; - } - match diff { - Ok(diff) => { - this.pane.update(cx, |pane, cx| pane.set_diff(diff, cx)); + this.update_in(cx, |this, _window, cx| { + this.loading = false; + if let Ok(Some(commit)) = commit { + this.commit = commit; } - Err(error) => { - this.error = Some(error.to_string().into()); + match diff { + Ok(diff) => { + this.pane.update(cx, |pane, cx| pane.set_diff(diff, cx)); + } + Err(error) => { + this.error = Some(error.to_string().into()); + } } - } - cx.notify(); - })?; + cx.notify(); + })?; - Ok(()) - }); + Ok(()) + }); - self.tasks.push(task); + task.detach(); } /// Header with the commit id, summary, author/time and overall change stats. diff --git a/crates/workspace/src/views/repo_detail/mod.rs b/crates/workspace/src/views/repo_detail/mod.rs index 0446d16..2201f06 100644 --- a/crates/workspace/src/views/repo_detail/mod.rs +++ b/crates/workspace/src/views/repo_detail/mod.rs @@ -10,8 +10,8 @@ use gix::Repository; use gpui::prelude::*; use gpui::{ Action, Anchor, AnyElement, App, ClipboardItem, Context, Entity, EventEmitter, FocusHandle, - Focusable, PathPromptOptions, Pixels, Render, SharedString, Size, Subscription, Task, - WeakEntity, Window, div, px, relative, size, transparent_white, + Focusable, PathPromptOptions, Pixels, Render, SharedString, Size, Subscription, WeakEntity, + Window, div, px, relative, size, transparent_white, }; use gpui_base::{Button as BaseButton, Disableable, Popover}; use gpui_component::alert::Alert; @@ -175,9 +175,6 @@ pub struct RepoDetailView { /// Bumped on every branch/tag switch. /// In-flight loads with an older generation are discarded when they complete. ref_generation: u64, - /// In-flight tasks, finished tasks are pruned on every push. - /// The vec stays bounded by the number of concurrent loads. - tasks: Vec>>, /// Subscriptions keeping the selectors' confirm events alive. _subscriptions: Vec, /// `(path, branch)` ready-suggestions dismissed by the user, per panel. @@ -346,7 +343,6 @@ impl RepoDetailView { push_statuses: Vec::new(), pending_upstream: None, focus_handle: cx.focus_handle(), - tasks: Vec::new(), _subscriptions: subscriptions, } } @@ -363,7 +359,7 @@ impl RepoDetailView { // Local repositories live on disk at their scan path. // No clone step or network refresh applies here. if let Some(local_path) = self.local_path.clone() { - let task = cx.spawn_in(window, async move |this, cx| { + let task: gpui::Task> = cx.spawn_in(window, async move |this, cx| { let data = cx .background_spawn(async move { let repo = gix::open(&local_path)?; @@ -383,7 +379,7 @@ impl RepoDetailView { Ok(()) }); - self.tasks.push(task); + task.detach(); return; } @@ -411,7 +407,7 @@ impl RepoDetailView { }) }; - let task = cx.spawn_in(window, async move |this, cx| { + let task: gpui::Task> = cx.spawn_in(window, async move |this, cx| { let disk = disk.await; let had_clone = matches!(&disk, Ok(Some(_))); @@ -531,7 +527,7 @@ impl RepoDetailView { Ok(()) }); - self.tasks.push(task); + task.detach(); } /// Apply the loaded repository data. @@ -619,7 +615,7 @@ impl RepoDetailView { prompt: Some("Clone".into()), }); - let task = cx.spawn_in(window, async move |this, cx| { + let task: gpui::Task> = cx.spawn_in(window, async move |this, cx| { // `Ok(Ok(Some(paths)))` means the user picked a folder. // A cancel or picker failure resolves to anything else. let picked = match prompt.await { @@ -649,7 +645,7 @@ impl RepoDetailView { Ok(()) }); - self.tasks.push(task); + task.detach(); } /// Preview the file at `path`, relative to the worktree root. @@ -703,7 +699,7 @@ impl RepoDetailView { self.load_commit(&path, cx); let generation = self.ref_generation; - let task = cx.spawn_in(window, async move |this, cx| { + let task: gpui::Task> = cx.spawn_in(window, async move |this, cx| { let path_for_read = path.clone(); let content = cx .background_spawn(async move { @@ -772,7 +768,7 @@ impl RepoDetailView { Ok(()) }); - self.tasks.push(task); + task.detach(); } /// Queue `path` for the per-file commit query. @@ -804,7 +800,7 @@ impl RepoDetailView { let paths = std::mem::take(&mut self.pending_commits); let generation = self.ref_generation; - let task = cx.spawn(async move |this, cx| { + let task: gpui::Task> = cx.spawn(async move |this, cx| { let rels: Vec = paths.iter().map(PathBuf::from).collect(); let result = cx .background_spawn( @@ -834,7 +830,7 @@ impl RepoDetailView { Ok(()) }); - self.tasks.push(task); + task.detach(); } /// Walk all commits reachable from HEAD on a background task. @@ -852,7 +848,7 @@ impl RepoDetailView { self.loading_all_commits = true; let generation = self.ref_generation; - let task = cx.spawn(async move |this, cx| { + let task: gpui::Task> = cx.spawn(async move |this, cx| { let result = cx .background_spawn(async move { signed_git::worktree_all_commits(&worktree) }) .await; @@ -876,7 +872,7 @@ impl RepoDetailView { Ok(()) }); - self.tasks.push(task); + task.detach(); } /// Open a new panel showing the diff of `commit_id`. @@ -909,8 +905,9 @@ impl RepoDetailView { self.error = None; cx.notify(); - self.tasks - .push(store.update(cx, |store, cx| store.push_repository(cx))); + store + .update(cx, |store, cx| store.push_repository(cx)) + .detach(); } /// Push the unpushed commits of the local checkout at `path`. @@ -931,7 +928,7 @@ impl RepoDetailView { self.error = None; cx.notify(); - let task = cx.spawn_in(window, async move |this, cx| { + let task: gpui::Task> = cx.spawn_in(window, async move |this, cx| { // The store owns the push, its busy flag and error reporting. let push = this.update_in(cx, |_this, _window, cx| { store.update(cx, |store, cx| store.push_checkout(path.clone(), cx)) @@ -948,7 +945,7 @@ impl RepoDetailView { Ok(()) }); - self.tasks.push(task); + task.detach(); } /// Delete the repository from nostr, announcement, state and activity. @@ -956,8 +953,9 @@ impl RepoDetailView { let Some(store) = self.store.clone() else { return; }; - self.tasks - .push(store.update(cx, |store, cx| store.delete_repository(cx))); + store + .update(cx, |store, cx| store.delete_repository(cx)) + .detach(); } /// Open the issues list panel in the dock area. @@ -1025,7 +1023,7 @@ impl RepoDetailView { }); self.pending_upstream = Some(addr); - let task = cx.spawn_in(window, async move |this, cx| { + let task: gpui::Task> = cx.spawn_in(window, async move |this, cx| { for _ in 0..60 { cx.background_executor() .timer(Duration::from_millis(250)) @@ -1060,7 +1058,7 @@ impl RepoDetailView { Ok(()) }); - self.tasks.push(task); + task.detach(); } /// Check out `name`, a branch or tag picked in the header. @@ -1101,7 +1099,7 @@ impl RepoDetailView { cx.notify(); let checkout_name = name.clone(); - let task = cx.spawn_in(window, async move |this, cx| { + let task: gpui::Task> = cx.spawn_in(window, async move |this, cx| { let result = cx .background_spawn(async move { match kind { @@ -1131,7 +1129,7 @@ impl RepoDetailView { Ok(()) }); - self.tasks.push(task); + task.detach(); } /// Restore a selector to `previous`, or clear it after a failed switch. @@ -1157,7 +1155,7 @@ impl RepoDetailView { return; }; - let task = cx.spawn(async move |this, cx| { + let task: gpui::Task> = cx.spawn(async move |this, cx| { let result = cx .background_spawn(async move { let snapshot = signed_git::worktree_snapshot(&worktree)?; @@ -1218,7 +1216,7 @@ impl RepoDetailView { Ok(()) }); - self.tasks.push(task); + task.detach(); } /// Refresh the file explorer, previews and commit list after the mirror @@ -1233,7 +1231,7 @@ impl RepoDetailView { return; }; - let task = cx.spawn(async move |this, cx| { + let task: gpui::Task> = cx.spawn(async move |this, cx| { let result = cx .background_spawn(async move { let snapshot = signed_git::worktree_snapshot(&worktree)?; @@ -1314,7 +1312,7 @@ impl RepoDetailView { Ok(()) }); - self.tasks.push(task); + task.detach(); } /// Drop the cached preview, editor and commit state of `path`. diff --git a/crates/workspace/src/views/repo_detail/new_pull_request.rs b/crates/workspace/src/views/repo_detail/new_pull_request.rs index 6512ac4..643a01d 100644 --- a/crates/workspace/src/views/repo_detail/new_pull_request.rs +++ b/crates/workspace/src/views/repo_detail/new_pull_request.rs @@ -6,8 +6,7 @@ use dock::{BasePanel, DockArea, Panel, PanelEvent, add_center_panel, panel_handl use gpui::prelude::*; use gpui::{ AnyElement, App, Context, Entity, EventEmitter, FocusHandle, Focusable, PathPromptOptions, - Pixels, Render, SharedString, Size, Subscription, Task, WeakEntity, Window, div, px, relative, - size, + Pixels, Render, SharedString, Size, Subscription, WeakEntity, Window, div, px, relative, size, }; use gpui_base::{Button as BaseButton, StyledExt}; use gpui_component::button::{Button, ButtonVariants}; @@ -79,7 +78,6 @@ pub struct NewPullRequestView { scroll_handle: VirtualListScrollHandle, item_sizes: Rc>>, _subscriptions: Vec, - tasks: Vec>>, } /// A fork-backed compare. @@ -363,7 +361,6 @@ impl NewPullRequestView { scroll_handle: VirtualListScrollHandle::new(), item_sizes: Rc::new(Vec::new()), _subscriptions: subscriptions, - tasks: Vec::new(), }; // Prefill with the store's freshest associated checkout, no folder dialog. @@ -426,25 +423,26 @@ impl NewPullRequestView { prompt: Some("Choose local checkout".into()), }); - let task = cx.spawn_in(window, async move |this, cx| { - // `Ok(Ok(Some(paths)))` means the user picked a folder. - // A cancel or picker failure resolves to anything else. - let picked = match prompt.await { - Ok(Ok(Some(mut paths))) => paths.pop(), - _ => None, - }; + let task: gpui::Task> = + cx.spawn_in(window, async move |this, cx| { + // `Ok(Ok(Some(paths)))` means the user picked a folder. + // A cancel or picker failure resolves to anything else. + let picked = match prompt.await { + Ok(Ok(Some(mut paths))) => paths.pop(), + _ => None, + }; - let Some(path) = picked else { - return Ok(()); - }; + let Some(path) = picked else { + return Ok(()); + }; - this.update_in(cx, |this, window, cx| { - this.apply_folder_path(path, window, cx); - })?; + this.update_in(cx, |this, window, cx| { + this.apply_folder_path(path, window, cx); + })?; - Ok(()) - }); - self.tasks.push(task); + Ok(()) + }); + task.detach(); } /// Apply `path` as the local checkout, no picker. @@ -453,28 +451,29 @@ impl NewPullRequestView { fn apply_folder_path(&mut self, path: PathBuf, window: &mut Window, cx: &mut Context) { let path = path.to_string_lossy().to_string(); - let task = cx.spawn_in(window, async move |this, cx| { - // Branches and the current branch are read off the UI thread. - let info = cx - .background_spawn({ - let path = path.clone(); - async move { - let repo = gix::open(Path::new(&path)).ok()?; - let branches = - signed_git::worktree_branches(Path::new(&path)).unwrap_or_default(); - let current = signed_git::current_branch(&repo).ok().flatten(); - Some((branches, current)) - } - }) - .await; + let task: gpui::Task> = + cx.spawn_in(window, async move |this, cx| { + // Branches and the current branch are read off the UI thread. + let info = cx + .background_spawn({ + let path = path.clone(); + async move { + let repo = gix::open(Path::new(&path)).ok()?; + let branches = + signed_git::worktree_branches(Path::new(&path)).unwrap_or_default(); + let current = signed_git::current_branch(&repo).ok().flatten(); + Some((branches, current)) + } + }) + .await; - this.update_in(cx, |this, window, cx| { - this.apply_checkout(path, info, window, cx); - })?; + this.update_in(cx, |this, window, cx| { + this.apply_checkout(path, info, window, cx); + })?; - Ok(()) - }); - self.tasks.push(task); + Ok(()) + }); + task.detach(); } /// Apply a picked checkout, filling the selectors and loading the compare. @@ -615,88 +614,89 @@ impl NewPullRequestView { self.error = None; cx.notify(); - let task = cx.spawn_in(window, async move |this, cx| { - // The fork and base must share history for a merge-base to exist. - // The target's mirror is the object store both sides land in. - // `ensure_clone` fetches `origin` when the mirror already exists. - let result = cx - .background_spawn({ - let cache = cache.clone(); - let base = base.clone(); - let base_clone_urls = base_clone_urls.clone(); - let namespace = namespace.clone(); - let clone_urls = clone_urls.clone(); - let mirror_path = mirror_path.clone(); - async move { - // The fork and base must share history for a merge-base to exist. - // The target's mirror is the object store both sides land in. - // `ensure_clone` fetches `origin` when the mirror already exists. - cache.ensure_clone(&base, &base_clone_urls)?; + let task: gpui::Task> = + cx.spawn_in(window, async move |this, cx| { + // The fork and base must share history for a merge-base to exist. + // The target's mirror is the object store both sides land in. + // `ensure_clone` fetches `origin` when the mirror already exists. + let result = cx + .background_spawn({ + let cache = cache.clone(); + let base = base.clone(); + let base_clone_urls = base_clone_urls.clone(); + let namespace = namespace.clone(); + let clone_urls = clone_urls.clone(); + let mirror_path = mirror_path.clone(); + async move { + // The fork and base must share history for a merge-base to exist. + // The target's mirror is the object store both sides land in. + // `ensure_clone` fetches `origin` when the mirror already exists. + cache.ensure_clone(&base, &base_clone_urls)?; - // Prune stale imports of any fork. - // Then import this fork's heads under its namespace. - delete_refs_with_prefix(&mirror_path, "refs/fork")?; + // Prune stale imports of any fork. + // Then import this fork's heads under its namespace. + delete_refs_with_prefix(&mirror_path, "refs/fork")?; - fetch_repo_refs( - &mirror_path, - &clone_urls, - &format!("+refs/heads/*:refs/fork/{namespace}/*"), - )?; + fetch_repo_refs( + &mirror_path, + &clone_urls, + &format!("+refs/heads/*:refs/fork/{namespace}/*"), + )?; - // Both branch lists are short names, sorted like the checkout's. - let strip = |refs: Vec, prefix: &str| { - let mut names: Vec = refs - .into_iter() - .filter_map(|name| { - name.strip_prefix(prefix) - .map(|rest| rest.trim_start_matches('/').to_owned()) - }) - .filter(|name| !name.is_empty()) - .collect(); - names.sort(); - names - }; + // Both branch lists are short names, sorted like the checkout's. + let strip = |refs: Vec, prefix: &str| { + let mut names: Vec = refs + .into_iter() + .filter_map(|name| { + name.strip_prefix(prefix) + .map(|rest| rest.trim_start_matches('/').to_owned()) + }) + .filter(|name| !name.is_empty()) + .collect(); + names.sort(); + names + }; - let base_branches = strip( - refs_with_prefix(&mirror_path, "refs/remotes/origin")?, - "refs/remotes/origin", - ); + let base_branches = strip( + refs_with_prefix(&mirror_path, "refs/remotes/origin")?, + "refs/remotes/origin", + ); - let compare_branches = strip( - refs_with_prefix(&mirror_path, &format!("refs/fork/{namespace}"))?, - &format!("refs/fork/{namespace}"), - ); + let compare_branches = strip( + refs_with_prefix(&mirror_path, &format!("refs/fork/{namespace}"))?, + &format!("refs/fork/{namespace}"), + ); - Ok::<_, anyhow::Error>((base_branches, compare_branches)) + Ok::<_, anyhow::Error>((base_branches, compare_branches)) + } + }) + .await; + + this.update_in(cx, |this, window, cx| { + // A source switch mid-flight discards the stale result. + // E.g. the user picked a folder while the fork was fetching. + let applied = this.fork.as_ref().map(|fork| fork.announcement.addr()); + if applied != expected_fork { + this.loading = false; + cx.notify(); + return; } - }) - .await; - this.update_in(cx, |this, window, cx| { - // A source switch mid-flight discards the stale result. - // E.g. the user picked a folder while the fork was fetching. - let applied = this.fork.as_ref().map(|fork| fork.announcement.addr()); - if applied != expected_fork { - this.loading = false; - cx.notify(); - return; - } + this.apply_fork( + announcement, + mirror_path, + namespace, + result, + keep_base, + keep_compare, + window, + cx, + ); + })?; - this.apply_fork( - announcement, - mirror_path, - namespace, - result, - keep_base, - keep_compare, - window, - cx, - ); - })?; - - Ok(()) - }); - self.tasks.push(task); + Ok(()) + }); + task.detach(); } /// Apply an imported fork, filling the selectors and loading the compare. @@ -834,66 +834,68 @@ impl NewPullRequestView { return; } - let task = cx.spawn_in(window, async move |this, cx| { - let result = cx - .background_spawn({ - let repo_path = repo_path.clone(); - let base = base.clone(); - let compare = compare.clone(); - let base_name = base_name.clone(); - let compare_name = compare_name.clone(); - async move { - let merge_base = merge_base(Path::new(&repo_path), &base, &compare)? - .ok_or_else(|| { - anyhow::anyhow!( - "{base_name} and {compare_name} share no common ancestor" - ) - })?; - let commits = worktree_commit_range_commits( - Path::new(&repo_path), - &merge_base, - &compare, - )?; - let diff = worktree_commit_range_diff( - Path::new(&repo_path), - &merge_base, - &compare, - )?; - Ok::<_, anyhow::Error>((merge_base, commits, diff)) + let task: gpui::Task> = + cx.spawn_in(window, async move |this, cx| { + let result = cx + .background_spawn({ + let repo_path = repo_path.clone(); + let base = base.clone(); + let compare = compare.clone(); + let base_name = base_name.clone(); + let compare_name = compare_name.clone(); + async move { + let merge_base = merge_base(Path::new(&repo_path), &base, &compare)? + .ok_or_else(|| { + anyhow::anyhow!( + "{base_name} and {compare_name} share no common ancestor" + ) + })?; + let commits = worktree_commit_range_commits( + Path::new(&repo_path), + &merge_base, + &compare, + )?; + let diff = worktree_commit_range_diff( + Path::new(&repo_path), + &merge_base, + &compare, + )?; + Ok::<_, anyhow::Error>((merge_base, commits, diff)) + } + }) + .await; + + this.update_in(cx, |this, _window, cx| { + // A stale result, branches changed mid-flight, must not clobber a newer compare. + if generation != this.compare_generation { + return; } - }) - .await; + this.loading = false; - this.update_in(cx, |this, _window, cx| { - // A stale result, branches changed mid-flight, must not clobber a newer compare. - if generation != this.compare_generation { - return; - } - this.loading = false; - - match result { - Ok((merge_base, commits, diff)) => { - this.merge_base = Some(merge_base); - let count = commits.len(); - this.item_sizes = Rc::new(vec![size(px(0.), px(COMMIT_ROW_HEIGHT)); count]); - this.commits = Some(commits); - this.pane.update(cx, |pane, cx| pane.set_diff(diff, cx)); + match result { + Ok((merge_base, commits, diff)) => { + this.merge_base = Some(merge_base); + let count = commits.len(); + this.item_sizes = + Rc::new(vec![size(px(0.), px(COMMIT_ROW_HEIGHT)); count]); + this.commits = Some(commits); + this.pane.update(cx, |pane, cx| pane.set_diff(diff, cx)); + } + Err(error) => { + this.merge_base = None; + this.commits = None; + this.pane.update(cx, |pane, cx| pane.clear(cx)); + this.error = Some(error.to_string().into()); + } } - Err(error) => { - this.merge_base = None; - this.commits = None; - this.pane.update(cx, |pane, cx| pane.clear(cx)); - this.error = Some(error.to_string().into()); - } - } - cx.notify(); - })?; + cx.notify(); + })?; - Ok(()) - }); + Ok(()) + }); - self.tasks.push(task); + task.detach(); } /// Publish the pull request. @@ -927,76 +929,78 @@ impl NewPullRequestView { self.error = None; cx.notify(); - let task = cx.spawn_in(window, async move |this, cx| { - // Regenerate the series at submit time. - // The published patch covers the current tip of the compare branch. - let patch = cx - .background_spawn({ - let repo_path = repo_path.clone(); - let merge_base = merge_base.clone(); - let compare_ref = compare_ref.clone(); - async move { - format_patch_between(Path::new(&repo_path), &merge_base, &compare_ref) - } - }) - .await; - - let patch = match patch { - Ok(patch) if !patch.is_empty() => patch, - Ok(_) => { - this.update_in(cx, |this, _window, cx| { - this.submitting = false; - this.error = Some("No commits between the branches to propose".into()); - cx.notify(); - })?; - return Ok(()); - } - Err(error) => { - this.update_in(cx, |this, _window, cx| { - this.submitting = false; - this.error = Some(format!("Failed to generate the patch: {error}").into()); - cx.notify(); - })?; - return Ok(()); - } - }; - - this.update_in(cx, |this, window, cx| { - this.submitting = false; - - store.update(cx, |store, cx| { - store.open_pull_request( - (!subject.is_empty()).then_some(subject), - description, - Some(branch_name), - patch, - false, - Some(merge_base), - Some(repo_path), - cx, - ); - }); - - // Close the panel once the publish is underway. - cx.defer_in(window, { - let dock_area = dock_area.clone(); - let entity = entity.clone(); - move |_, window, cx| { - if let Some(dock_area) = dock_area.upgrade() { - dock_area.update(cx, |dock, cx| { - dock.remove_panel(entity, window, cx); - }); + let task: gpui::Task> = + cx.spawn_in(window, async move |this, cx| { + // Regenerate the series at submit time. + // The published patch covers the current tip of the compare branch. + let patch = cx + .background_spawn({ + let repo_path = repo_path.clone(); + let merge_base = merge_base.clone(); + let compare_ref = compare_ref.clone(); + async move { + format_patch_between(Path::new(&repo_path), &merge_base, &compare_ref) } + }) + .await; + + let patch = match patch { + Ok(patch) if !patch.is_empty() => patch, + Ok(_) => { + this.update_in(cx, |this, _window, cx| { + this.submitting = false; + this.error = Some("No commits between the branches to propose".into()); + cx.notify(); + })?; + return Ok(()); } - }); + Err(error) => { + this.update_in(cx, |this, _window, cx| { + this.submitting = false; + this.error = + Some(format!("Failed to generate the patch: {error}").into()); + cx.notify(); + })?; + return Ok(()); + } + }; - cx.notify(); - })?; + this.update_in(cx, |this, window, cx| { + this.submitting = false; - Ok(()) - }); + store.update(cx, |store, cx| { + store.open_pull_request( + (!subject.is_empty()).then_some(subject), + description, + Some(branch_name), + patch, + false, + Some(merge_base), + Some(repo_path), + cx, + ); + }); - self.tasks.push(task); + // Close the panel once the publish is underway. + cx.defer_in(window, { + let dock_area = dock_area.clone(); + let entity = entity.clone(); + move |_, window, cx| { + if let Some(dock_area) = dock_area.upgrade() { + dock_area.update(cx, |dock, cx| { + dock.remove_panel(entity, window, cx); + }); + } + } + }); + + cx.notify(); + })?; + + Ok(()) + }); + + task.detach(); } /// Open the diff of `commit_id`, from the Commits tab, in a new panel. diff --git a/crates/workspace/src/views/repo_detail/pull_request_detail.rs b/crates/workspace/src/views/repo_detail/pull_request_detail.rs index e6409ce..73c4755 100644 --- a/crates/workspace/src/views/repo_detail/pull_request_detail.rs +++ b/crates/workspace/src/views/repo_detail/pull_request_detail.rs @@ -6,7 +6,7 @@ use dock::{BasePanel, DockArea, DockPlacement, Panel, PanelEvent, panel_handle}; use gpui::prelude::*; use gpui::{ AnyElement, App, Context, Entity, EventEmitter, FocusHandle, Focusable, Pixels, Render, - SharedString, Size, Task, WeakEntity, Window, div, px, relative, size, + SharedString, Size, WeakEntity, Window, div, px, relative, size, }; use gpui_component::button::{Button, ButtonVariants}; use gpui_component::clipboard::Clipboard; @@ -66,8 +66,6 @@ pub struct PullRequestDetailView { commit_item_sizes: Rc>>, /// Virtual list state of the commits tab. commit_scroll_handle: VirtualListScrollHandle, - /// In-flight tasks, finished tasks are pruned on every push. - tasks: Vec>>, } impl PullRequestDetailView { @@ -106,7 +104,6 @@ impl PullRequestDetailView { pane, commit_item_sizes: Rc::new(Vec::new()), commit_scroll_handle: VirtualListScrollHandle::new(), - tasks: Vec::new(), } } @@ -162,101 +159,103 @@ impl PullRequestDetailView { self.description = description.into(); - let task = cx.spawn_in(window, async move |this, cx| { - let nostr_diff = cx - .background_spawn({ - let patch = patch.clone(); - async move { patch_diffs(&patch) } - }) - .await; - - let nostr_commits = cx - .background_spawn({ - let patch = patch.clone(); - async move { patch_commits(&patch) } - }) - .await; - - // PRs without patch events, e.g. published by ngit, carry their changes in git. - // Fetch the clone and diff the `merge-base..tip` range. - let use_nostr = match &nostr_diff { - Ok(diff) => has_patch_link || !diff.files.is_empty(), - Err(_) => true, - }; - - let git = if use_nostr { - None - } else { - let cache = cache.clone(); - let addr = addr.clone(); - let clone_urls = clone_urls.clone(); - let base = merge_base.clone(); - let tip = current_commit.clone(); - - Some( - cx.background_spawn(async move { - let repo = cache.ensure_clone(&addr, &clone_urls)?; - - let workdir = repo - .workdir() - .ok_or_else(|| anyhow::anyhow!("repository has no worktree"))? - .to_path_buf(); - - let tip = - tip.ok_or_else(|| anyhow::anyhow!("pull request has no tip commit"))?; - - let base = match base { - Some(base) => base, - // No `merge-base` tag. Use the merge base of the tip and the default branch. - None => { - let head = repo - .head_id() - .map_err(|_| anyhow::anyhow!("repository has no HEAD"))?; - let tip_id = repo.rev_parse_single(tip.as_bytes())?; - repo.merge_base(tip_id, head)?.to_string() - } - }; - - let diff = signed_git::worktree_commit_range_diff(&workdir, &base, &tip)?; - let commits = - signed_git::worktree_commit_range_commits(&workdir, &base, &tip)?; - - Ok::<_, anyhow::Error>((diff, commits, workdir)) + let task: gpui::Task> = + cx.spawn_in(window, async move |this, cx| { + let nostr_diff = cx + .background_spawn({ + let patch = patch.clone(); + async move { patch_diffs(&patch) } }) - .await, - ) - }; + .await; - let (diff, commits, worktree) = match git { - Some(Ok((diff, commits, worktree))) => (Ok(diff), commits, Some(worktree)), - Some(Err(error)) => (Err(error), Vec::new(), None), - None => (nostr_diff, nostr_commits, None), - }; + let nostr_commits = cx + .background_spawn({ + let patch = patch.clone(); + async move { patch_commits(&patch) } + }) + .await; - this.update_in(cx, |this, _window, cx| { - this.loading = false; - this.worktree = worktree; - this.current_commit = current_commit.map(SharedString::from); - this.commit_item_sizes = Rc::new(vec![size(px(0.), px(ROW_HEIGHT)); commits.len()]); - this.commits = commits; + // PRs without patch events, e.g. published by ngit, carry their changes in git. + // Fetch the clone and diff the `merge-base..tip` range. + let use_nostr = match &nostr_diff { + Ok(diff) => has_patch_link || !diff.files.is_empty(), + Err(_) => true, + }; - match diff { - Ok(diff) => { - this.pane.update(cx, |pane, cx| pane.set_diff(diff, cx)); + let git = if use_nostr { + None + } else { + let cache = cache.clone(); + let addr = addr.clone(); + let clone_urls = clone_urls.clone(); + let base = merge_base.clone(); + let tip = current_commit.clone(); + + Some( + cx.background_spawn(async move { + let repo = cache.ensure_clone(&addr, &clone_urls)?; + + let workdir = repo + .workdir() + .ok_or_else(|| anyhow::anyhow!("repository has no worktree"))? + .to_path_buf(); + + let tip = tip + .ok_or_else(|| anyhow::anyhow!("pull request has no tip commit"))?; + + let base = match base { + Some(base) => base, + // No `merge-base` tag. Use the merge base of the tip and the default branch. + None => { + let head = repo + .head_id() + .map_err(|_| anyhow::anyhow!("repository has no HEAD"))?; + let tip_id = repo.rev_parse_single(tip.as_bytes())?; + repo.merge_base(tip_id, head)?.to_string() + } + }; + + let diff = + signed_git::worktree_commit_range_diff(&workdir, &base, &tip)?; + let commits = + signed_git::worktree_commit_range_commits(&workdir, &base, &tip)?; + + Ok::<_, anyhow::Error>((diff, commits, workdir)) + }) + .await, + ) + }; + + let (diff, commits, worktree) = match git { + Some(Ok((diff, commits, worktree))) => (Ok(diff), commits, Some(worktree)), + Some(Err(error)) => (Err(error), Vec::new(), None), + None => (nostr_diff, nostr_commits, None), + }; + + this.update_in(cx, |this, _window, cx| { + this.loading = false; + this.worktree = worktree; + this.current_commit = current_commit.map(SharedString::from); + this.commit_item_sizes = + Rc::new(vec![size(px(0.), px(ROW_HEIGHT)); commits.len()]); + this.commits = commits; + + match diff { + Ok(diff) => { + this.pane.update(cx, |pane, cx| pane.set_diff(diff, cx)); + } + Err(error) => { + this.error = Some(error.to_string().into()); + } } - Err(error) => { - this.error = Some(error.to_string().into()); - } - } - cx.notify(); - })?; + cx.notify(); + })?; - Ok(()) - }); + Ok(()) + }); - self.tasks.retain(|task| !task.is_ready()); - self.tasks.push(task); + task.detach(); } /// Open the diff of `commit_id` in the bottom dock of the area. diff --git a/docs/backend-rearchitecture.md b/docs/backend-rearchitecture.md new file mode 100644 index 0000000..52a4a3c --- /dev/null +++ b/docs/backend-rearchitecture.md @@ -0,0 +1,1296 @@ +# Backend re-architecture: findings and plan + +This is a follow-up to an initial architecture review. It re-checks every claim +against the **actual `nostr`/`nostr-sdk` source pinned by `Cargo.lock`** +(`rev 0c6fad2ac8ce934747096953f6dba355e3532614`, checked out locally at +`~/.cargo/git/checkouts/nostr-9dff06fa64f758da/0c6fad2/{nostr,nostr-sdk}/src`) +and the actual **GPUI source pinned by `Cargo.lock`** +(`git+https://github.com/zed-industries/zed#1870e269ad88802147f2baec3086abb67d17260a`, +checked out at `~/.cargo/git/checkouts/zed-a70e2ad075855582/1870e26/crates/{gpui,scheduler}/src`), +not from general knowledge of either. Every API claim below cites the file it +was verified against. + +Scope: `crates/signed_nostr`, `crates/signed_state`, `crates/signed_core`, +`crates/signed_git`, and `crates/workspace` (the actual call sites of the +backend, audited for business-logic flaws and redundant conversions). + +## Summary of the ask + +1. Never call `fetch_events`. Bootstrap only via `subscribe`/`sync` (negentropy), read from `client.database()`. +2. Collapse the multiple "send an event" functions into direct `nostr-sdk` calls, no house wrappers. +3. Verify every API claim against the locally checked-out SDK/GPUI source. +4. Remove unnecessary logic (relay add/connect round trips, the fetch/sync dedup cache, unbounded task lists). +5. Re-evaluate `signed_git`'s dependence on `gix` — how much of it duplicates functionality `gix` (or another crate) already provides. +6. Check for unnecessary `cx.notify()` / over-broad re-renders vs. partial re-render. +7. Audit `crates/workspace` (the real UI call sites) for business-logic flaws of the same shape as `create_repository`, and for unnecessary string/type conversions and clones. + +Each is addressed below with concrete file:line references and a verified replacement. + +--- + +## 1. `fetch_events` — one call site, and it should go too + +``` +grep -rn "fetch_events" crates/ +crates/signed_state/src/backend.rs:1065 +``` + +The **only** use in the whole workspace is `Backend::bootstrap_user` +(`crates/signed_state/src/backend.rs:1059-1086`): + +```rust +fn bootstrap_user(&mut self, public_key: PublicKey, cx: &mut Context) { + let client = self.client.clone(); + self.push_task(cx.spawn(async move |this, cx| { + let result = async { + let events: Vec = client + .fetch_events(filters::grasp_list(public_key)) + .await? + .into_iter() + .collect(); + for url in latest_grasp_list_servers(events) { + client.add_relay(url.as_str()).await.ok(); + } + client.connect().await; + Ok::<_, Error>(()) + }.await; + ... + })); +} +``` + +Verified against `nostr-sdk/src/client/mod.rs:963-1018` (doc comment on +`Client::fetch_events`): it's explicitly the "buffer events, return a `Vec`" +sibling of `stream_events`, both explicitly documented as **short-lived** +subscriptions for one-off reads — the SDK's own guidance ("for long-lived +subscriptions use `Client::subscribe`") doesn't forbid `fetch_events` +outright, but the project rule you want is stricter: never bypass the +database. That's achievable here too, because `client.sync` degrades +gracefully to a plain fetch-and-store when the local DB has nothing yet. + +**Replacement** — sync against the bootstrap relays (same relays already +used for every other bootstrap query, see `BOOTSTRAP_RELAYS`, +`backend.rs:28-33`) and then read the result out of the database, exactly +like every other store in this codebase already does: + +```rust +// Also drops push_task/tasks in favor of .detach() — see §6. +fn bootstrap_user(&mut self, public_key: PublicKey, cx: &mut Context) { + let client = self.client.clone(); + cx.spawn(async move |this, cx| { + let result = async { + client + .sync(filters::grasp_list(public_key)) + .with(BOOTSTRAP_RELAYS) + .await?; + + let events = client.database().query(filters::grasp_list(public_key)).await?; + for url in latest_grasp_list_servers(events) { + client.add_relay(url).and_connect().await.ok(); // see §8 + } + Ok::<_, Error>(()) + }.await; + ... + }) + .detach(); +} +``` + +Verified against `nostr-sdk/src/client/api/sync.rs:1-32` and +`nostr-sdk/src/client/mod.rs:1020-1030` (`Client::sync` doc: "Performs a +negentropy-based reconciliation between the local database and one or more +relays" — this is exactly a bootstrap-and-store operation, no separate +"first fetch" step needed). No other code changes: `filters::grasp_list` and +`latest_grasp_list_servers` are unaffected. + +This also removes the last inconsistency in the codebase between "how we get +data:" everywhere else is sync-then-query; now it's sync-then-query +everywhere, no exceptions. + +--- + +## 2. The "send an event" functions — there are 8, there should be roughly 2 + +Grep for anything that ends up calling `client.send_event`: + +| Function | File:line | What it adds over `client.send_event` | +|---|---|---| +| `Backend::send` | `backend.rs:1308-1322` | signs with the current signer, then calls `broadcast_event` | +| `Backend::publish_event` | `backend.rs:1325-1332` | calls `broadcast_event` on an already-signed event | +| `Backend::publish_task` | `backend.rs:1335-1360` | wraps a future, emits `BackendEvent::Published`/`Error` | +| `Backend::send_fire_and_forget` | `backend.rs:1363-1375` | calls `send`, drops the result except logging | +| `Backend::retract_events` | `backend.rs:1378-1398` | hand-builds NIP-09 tags, calls `send` | +| `broadcast_event` (free fn) | `backend.rs:1404-1418` | calls `client.send_event`, turns "0 relays accepted" into an `Err` | +| `stage_event_on_relay` | `backend.rs:1768-1795` | calls `client.send_event(..).to([relay])`, same 0-accept-is-Err logic, different error type (`String`) | +| `RepoStore::send` | `repo.rs:1334-1344` | calls `Backend::send`, tracks `last_error` — **but several `RepoStore` methods bypass it** and call `Backend::send`/`Backend::publish_event` directly (`repo.rs:803`, `repo.rs:935`), so error surfacing is inconsistent across `RepoStore` methods | + +That's 8 layers for what the SDK already does in one call. Verified against +`nostr-sdk/src/client/api/send_event.rs:119-350`: + +- `client.send_event(&event)` **already** verifies the signature, saves the + event to the local database (`save_into_database`, default `true`), and + broadcasts — all before you touch anything (`send_event.rs:337-345`). +- Zero-relay-accepted is *not* an error from the SDK's point of view — it + returns `Ok` with `output.success` empty and `output.failed` populated. + Turning that into an app-level error is legitimate domain logic (the repo + already gets this right), it just doesn't need 3 separate functions + (`broadcast_event`, `stage_event_on_relay`, and the implicit success check + buried in `RepoStore::send`) doing the same "empty success ⇒ error" check. + +### Recommended shape: one helper, and direct SDK calls everywhere else + +Keep exactly **one** small helper because the "empty success ⇒ Err" rule is +real, repeated, app-specific policy (the SDK intentionally leaves that +decision to the caller): + +```rust +/// The event was accepted by at least one relay, or a descriptive error otherwise. +async fn require_relay_accepted(output: SendEventOutput) -> Result { + if output.success.is_empty() && !output.failed.is_empty() { + let reasons = output.failed.values().cloned().collect::>().join(", "); + bail!("event not accepted by any relay: {reasons}"); + } + Ok(event) +} +``` + +Then delete `Backend::send`, `Backend::publish_event`, +`Backend::send_fire_and_forget`, `broadcast_event`, and `RepoStore::send`. +Call `client.send_event(...)` **directly** at each call site, exactly like +`stage_event_on_relay` already does for the GRASP staging path — that +function is the one place in the codebase that already follows this +pattern (`.to([relay.clone()])`, explicit target, no extra wrapper beyond +the accept-check). Generalize *that* pattern instead of routing everything +through `Backend`. + +```rust +// A GPUI call site, e.g. RepoStore::open_issue, today: +self.send(builder, cx); + +// direct SDK call instead. No task list to push into and prune either — +// see §6, `.detach()` is the right default here. +let signer = Backend::global(cx).read(cx).signer(); +let client = Backend::global(cx).read(cx).client(); +cx.spawn(async move |this, cx| { + let event = builder.finalize_async(&signer).await?; + let output = client.send_event(&event).await?; + let event = require_relay_accepted(output, event).await?; + this.update(cx, |this, cx| { /* apply + cx.notify() */ }) +}) +.detach(); +``` + +`Backend` still owns the `Client`/`UniversalSigner` (a real, load-bearing +type — see §4 for why it must stay), but it should expose them +(`Backend::client()`/`Backend::signer()`, both already exist, +`backend.rs:1089-1096`) rather than mediate every publish through 4 layers +of wrapper. Emitting `BackendEvent::Published` for cross-store invalidation +(e.g. so `RepoListStore` refreshes when a new announcement lands) is the one +piece of `publish_task` worth keeping — but it can be a single `fn` taking +`&Event` that any call site invokes after its own `send_event`, not the +thing that *does* the sending. + +### `Backend::retract_events` — use the SDK's own NIP-09 builder, one deletion event per target + +`nostr` already ships `EventDeletionRequest` (verified in +`nostr/src/nips/nip09.rs:15-92`), which implements `IntoEventBuilder` exactly +like `GitRepositoryAnnouncement`/`GitIssue`/etc. already used elsewhere in +this codebase. Today's code hand-builds the tags for **one** deletion event +covering every target, plus a `k` tag per target: + +```rust +// today, backend.rs:1378-1398 +let mut tags: Vec = Vec::with_capacity(events.len() * 2); +for event in events { + tags.push(Tag::event(event.id)); + tags.push(Tag::parse(["k", &event.kind.to_string()]).expect("valid kind tag")); +} +let task = self.send(EventBuilder::new(Kind::EventDeletion, "").tags(tags), cx); +``` + +Per direction from the team: no `k` tag, and each event gets its own +deletion event rather than one deletion event listing multiple `e` tags. +`EventDeletionRequest` (`nip09.rs:15-92`) supports exactly that shape +already — call `.id(event.id)` once per event and send each independently: + +```rust +async fn retract_event(client: &Client, signer: &UniversalSigner, event: &Event) -> Result<(), Error> { + let builder = EventDeletionRequest::new().id(event.id).into_event_builder(); + let deletion = builder.finalize_async(signer).await?; + client.send_event(&deletion).await?; + Ok(()) +} + +fn retract_events(&mut self, events: &[Event], cx: &mut Context) { + let client = self.client.clone(); + let signer = self.signer.clone(); + + for event in events.to_vec() { + let client = client.clone(); + let signer = signer.clone(); + + cx.spawn(async move |_this, _cx| { + if let Err(e) = retract_event(&client, &signer, &event).await { + log::warn!("failed to retract event {}: {e}", event.id); + } + }) + .detach(); + } +} +``` + +No hand-rolled tag construction, no batching multiple targets into one +event, no `k` tag, and no task list to maintain (§6). Each deletion is +independent: a relay rejecting or dropping one doesn't affect the others. + +--- + +## 3. Remove the fetch/sync dedup cache — it duplicates state that already exists elsewhere + +`Backend` carries: + +```rust +recent_fetches: HashMap, // backend.rs:86 +const FETCH_DEDUP_WINDOW: Duration = ...; // backend.rs:39 +fn fetch_recently_started(&mut self, fingerprint: u64) -> bool { ... } // backend.rs:1178-1186 +fn fetch_fingerprint(relays: &[&str], filters: &[Filter]) -> u64 { ... } // backend.rs:1423-1433 +``` + +used at 3 call sites (`connect_repo_relays`, `sync_bootstrap`, and +indirectly wherever those are called), e.g.: + +```rust +pub fn sync_bootstrap(&mut self, filter: Filter, cx: &mut Context) { + let fingerprint = fetch_fingerprint(&BOOTSTRAP_RELAYS, std::slice::from_ref(&filter)); + if self.fetch_recently_started(fingerprint) { + log::debug!("skipping duplicate bootstrap sync"); + return; + } + ... +} +``` + +This is a generic "have I already asked for this filter recently" +cache, sorting + hashing relay lists and filters, pruning on a 5-minute +window, and un-inserting on error so a failed sync can retry immediately. +It exists purely to avoid redundant `sync`/`subscribe` calls — but every +call site that calls into `Backend::sync_bootstrap`/`connect_repo_relays` +**already has its own, more precise state for exactly this purpose**: + +- `RepoStore` tracks `repo_relays: HashSet` (`repo.rs:77`) — "have + I already connected+fetched this repo's relays" — and `root_fetches: + HashSet` (`repo.rs:81`) for per-root fetches. +- `RepoListStore` and `CheckoutsStore` each already run every refresh + through `RefreshGate` (`refresh.rs`), which itself exists to coalesce + bursts of refresh requests — that's the same "don't do this again right + now" idea, at the right granularity (per-store, per-purpose), not a + generic cross-cutting cache keyed by a hash of relays+filters. + +The `Backend`-level cache is solving the same problem a second time, at a +coarser and more error-prone granularity (a hash collision or an +order-sensitivity bug silently drops a legitimate sync; the 5-minute window +is a magic number with no connection to how often any of the 3 call sites +actually fire). Delete `recent_fetches`, `fetch_recently_started`, +`fetch_fingerprint`, `FETCH_DEDUP_WINDOW`, and `DefaultHasher`/`Hash`/`Hasher` +imports they pull in. Let each caller guard itself the way `RepoStore` +already does for `repo_relays`: + +```rust +// RepoStore, once per repo — this pattern already exists (repo.rs:189ish), +// just needs to also gate the *bootstrap* sync calls the same way instead +// of relying on a Backend-side cache. +if self.repo_relays.insert(relay.clone()) { + backend.update(cx, |backend, cx| backend.connect_repo_relays(vec![relay], filters, cx)); +} +``` + +`sync_bootstrap` for repo-independent filters (announcements, deletions) is +called from exactly one place today (`RepoListStore::subscribe_remote`, +`repo_list.rs:144-153`), on store construction — i.e., once per app +session. It does not need a dedup cache at all; if you're worried about a +second `RepoListStore` instance ever existing, that's a `Global`-uniqueness +invariant, not something to paper over with a fingerprint cache. + +--- + +## 4. Gossip is enabled, and stays enabled — but today's git-domain sends should bypass it explicitly + +Per team direction: gossip is a deliberate, load-bearing choice for this +client (it's not fully wired up to a feature yet, but it's not incidental +configuration either). `.gossip(...)` stays in `signed_nostr::backend::with_database` +(`crates/signed_nostr/src/backend.rs:31-51`). This section is scoped down +to what falls out of that: how the currently-implemented send paths +interact with gossip being on, verified against the SDK source. + +```rust +let client = ClientBuilder::default() + .database(database) + .authenticator(authenticator) + .gossip(NostrGossipMemory::unbounded()) + .gossip_config(GossipConfig::default().no_background_refresh()) + ... + .build(); +``` + +Verified against `nostr-sdk/src/client/api/send_event.rs:337-388` and the +doc comment on `Client::send_event` (`client/mod.rs:1097-1130`): **when no +explicit target is set** (no `.to()`/`.broadcast()`/`.to_nip17()`/`.to_nip65()`), +and gossip is configured, `send_event` resolves the destination via the +gossip engine (NIP-65 relay discovery for the event's author + tagged +pubkeys), not simply "every relay you `add_relay`'d". Every one of the 8 +send-paths in §2 calls `client.send_event(&event)` with **no explicit +target** — meaning every one of them is going through gossip-based relay +resolution today, on top of the relays this app added on purpose +(`BOOTSTRAP_RELAYS`, the repo's own `relays` tag, GRASP servers). + +That happens not to lose anything today, because `gossip_prepare_urls` +(`send_event.rs:229-320`) *also* unions in `client.pool().write_relay_urls()` +at the end — so events still reach every WRITE relay in the pool, gossip +only adds more relays on top. But it's not free: every plain `send_event` +call (opening an issue, commenting, reacting to a PR) does gossip +relay-list resolution — potentially a network round trip to fetch a NIP-65 +list — for events whose target set is already fully determined by the +repo's own `relays` tag or the bootstrap relay list, and where the extra +NIP-65 relays gossip adds are not places NIP-34 consumers are expected to +look. + +**Recommendation:** keep `.gossip(...)` configured (it's wanted for +whatever's next — NIP-17 DMs, NIP-65 profile/relay-list features, etc.), +but make the git-domain sends that already have a well-defined target +explicit about it, the same way `stage_event_on_relay` already is +(`.to([relay.clone()])`, `backend.rs:1768-1795`): + +- Repository-scoped events (announcements, state, issues, PRs, patches, + comments, statuses, deletions) know their target relays already (the + repo's `relays` tag, or `BOOTSTRAP_RELAYS` for repo-independent + discovery events) — send them with `.broadcast()` or `.to(relays)` so + they don't pay for gossip resolution and don't silently depend on the + sender's NIP-65 list being fresh. +- Anything that *should* use gossip once it exists (e.g. a future NIP-17 + DM, or explicit NIP-65 profile publishing) keeps the default routing, or + calls `.to_nip17()`/`.to_nip65()` explicitly. + +This is a small, additive change (one `.broadcast()`/`.to(...)` call per +send site as part of the §2 consolidation), not a removal — do it while +touching each call site for the send-path cleanup below, so gossip stays +fully available for the features that are meant to use it, while today's +repo/issue/PR/patch traffic stays deterministic about where it goes. + +--- + +## 5. `signed_git` vs `gix` — split verdict, not "throw it all out" + +`crates/signed_git/src/lib.rs` is 4118 lines. Checked the actual `gix` +version pinned (`gix = "0.87.1"`, feature set in the root `Cargo.toml`) and +its `gix-diff 0.67.1` dependency against what `signed_git` hand-rolls. + +### Already correct, idiomatic `gix` usage — keep as-is + +`tree_diff` (`signed_git/src/lib.rs:1468-1583`) generates commit-to-commit +diffs by calling `repo.diff_tree_to_tree(...)`, then +`gix::diff::blob::diff_with_slider_heuristics(...)`, then feeding the result +through `gix::diff::blob::UnifiedDiff::new(&diff, &input, collector, ..)` +where `collector` implements gix's own `ConsumeHunk` trait +(`signed_git/src/lib.rs:1963-2027`, matching `gix-diff-0.67.1/src/blob/unified_diff/mod.rs:70-84` +exactly). This *is* the documented, intended way to consume `gix`'s diff +engine — there is no simpler API to fall back to, and no unnecessary +reimplementation here. Same for the porcelain wrappers around `gix::Repository` +for refs, branches, tags, worktree checkout, etc. — that's inherent surface +area for a git-porcelain layer, not bloat. + +### Real duplication — the `git format-patch` text parser + +The other ~700 lines (`parse_diff_section`, `parse_hunk`, `hunk_header`, +`header_paths`, `diff_line_path`, `take_quoted`, `unquote_path`, +`strip_patch_prefix`, `name_from_address`, `signed_git/src/lib.rs:1660-2027`) +are a hand-rolled parser for **already-rendered** `git format-patch`/unified +diff text — this is necessary because a NIP-34 patch event's content *is* +the raw text output of `git format-patch`, arriving over Nostr with no +backing git objects to hand to `gix`'s diff engine. `gix-diff` only +*generates* unified diffs from git objects; it has no facility to *parse* +unified-diff text back into structured hunks, so this isn't a case of +"gix already does this and we reimplemented it." + +However, a maintained crate already exists for exactly this parsing job: +[`diffy`](https://docs.rs/diffy)'s `PatchSet` module +(`diffy::patch_set::PatchSet::parse(text, ParseOptions::gitdiff())`) +explicitly parses "the output of `git diff` or `git format-patch`", +supporting `diff --git` headers, extended headers (`new file mode`, +`deleted file mode`, etc.), rename/copy detection via `rename from`/`rename +to`/`copy from`/`copy to`, and binary-file detection — i.e., the exact +feature list `signed_git`'s hand-rolled parser reimplements +(`FileDiff::status` has `Renamed`/`Copied`/`Added`/`Deleted`/`Modified` +variants, `signed_git/src/lib.rs:1373-1379`; binary detection at +`signed_git/src/lib.rs:1395`). + +**Recommendation:** spike replacing `patch_diffs`/`parse_diff_section`/ +`parse_hunk`/`unquote_path`/etc. with `diffy::patch_set::PatchSet`, mapping +its `FileOperation`/`Hunk` types onto this codebase's existing `FileDiff`/ +`DiffHunk` (which downstream UI code already depends on, so keep those +public types and only replace the parsing internals). This is the single +biggest concrete size reduction available in the whole backend — a ~700 +line hand-rolled parser (plus ~1300 lines of tests for it, +`signed_git/src/lib.rs:3681-4053` and surrounding) collapses to a thin +adapter over a well-tested crate. Budget a spike first: `diffy`'s renamed +path handling and quoted-path unescaping need to be checked against this +project's test fixtures (`signed_git/src/lib.rs:3917-3962`, +octal-escaped/non-ASCII quoted paths) before committing to the swap. + +--- + +## 6. Remove the `tasks: Vec>` + `push_task` boilerplate — use `Task::detach()` + +Verified against the actual pinned GPUI revision +(`~/.cargo/git/checkouts/zed-a70e2ad075855582/1870e26/crates/scheduler/src/executor.rs:375-573` +and `crates/gpui/src/executor.rs:32-63`). + +Six different stores carry the exact same field and method, copy-pasted: + +```rust +tasks: Vec>>, + +fn push_task(&mut self, task: Task>) { + self.tasks.retain(|task| !task.is_ready()); + self.tasks.push(task); +} +``` + +at `backend.rs:87-91,164-169`, `checkouts.rs:106-110,180-185`, +`local_repos.rs:13-23`, `profile.rs:72-80,136-141`, `repo.rs:82-86` (plus an +inlined copy of the same retain-then-push at `repo.rs:236-240` and +`repo.rs:373-377`), and `repo_list.rs:56-60,137-142`. + +`Task`'s own doc comment (`scheduler/src/executor.rs:375-380`) says exactly +what this boilerplate exists to avoid: "If you drop a task it will be +cancelled immediately. Calling `Task::detach` allows the task to continue +running, but with no way to return a value." `Task::detach(self)` +(`executor.rs:552-559`) does precisely that, and `TaskExt::detach_and_log_err` +(`gpui/src/executor.rs:35-61`, already referenced in this project's own +`.rules` file) additionally logs an `Err` without any manual `match`. None +of these stores' spawned tasks need cancel-on-drop semantics: every +continuation already does `this.update(cx, ...).ok()` or propagates through +`?`, so if the owning entity is gone by the time the task finishes, the +update is a harmless no-op — exactly the "tolerate the entity being gone" +pattern already used everywhere in this codebase (see the `.ok()` calls +throughout `backend.rs`). Storing the task and pruning it on every push +buys nothing here; `.detach()` (or `.detach_and_log_err(cx)` where the +continuation only logs on failure) replaces both the field and the method: + +```rust +// today +self.push_task(cx.spawn(async move |this, cx| { + if let Err(e) = task.await { + this.update(cx, |_this, cx| cx.emit(BackendEvent::error(e.to_string()))).ok(); + } + Ok(()) +})); + +// replacement — no field, no prune, no manual match +cx.spawn(async move |this, cx| { + if let Err(e) = task.await { + this.update(cx, |_this, cx| cx.emit(BackendEvent::error(e.to_string()))).ok(); + } +}) +.detach(); +``` + +Delete the `tasks` field and `push_task` method from all six stores, and +change every `self.push_task(cx.spawn(...))` call to `cx.spawn(...).detach()` +(or `.detach_and_log_err(cx)` when the closure's only job is to log the +error). The one place that must **not** just detach is `push_repo_from`'s +returned `Task>` (`backend.rs:815-902`) — that +task is deliberately returned to the UI caller (so the panel can `.await` +it and show a spinner) and already isn't stored in a `tasks` list today, so +it's unaffected by this cleanup. + +`crates/workspace` has the same pattern too, and there it's a real bug, not +just style — see §14. + +--- + +## 7. Render granularity / `cx.notify()` audit + +Checked every `cx.notify()` call in `signed_state` (18 call sites) and how +`workspace` views consume each store. Overall this is **already +well-partitioned**, not a smell: + +- Every panel (`IssuesView`, `PullRequestsView`, `IssueDetailView`, + `CommitDiffView`, `RepoDetailView`, `PullRequestDetailView`, + `NewPullRequestView`, `DiffPane`) is its own `Entity`/`Render` impl — + `cx.notify()` on a store only invalidates the views actually observing + that store's `Entity`, not a monolithic root view. +- `IssuesView`/`PullRequestsView` already memoize derived rows behind a + `(store.version(), filter)` cache key (`issues.rs:68-72`, `323-333`; + `pull_requests.rs:75-79`, `331-341`), and both use + `VirtualListScrollHandle` for virtualization — so a store `notify()` + doesn't force rebuilding or laying out off-screen rows. +- `sync_bootstrap`'s per-percent progress `cx.notify()` + (`backend.rs:1257-1264`) is already throttled to *distinct percentage + points* (`if progress.current > 0 && percent != last_percent`, + `backend.rs:1254`), and nothing in `workspace` reads `Backend::sync_progress()` + directly (`grep -rn "sync_progress()" crates/workspace` → no matches), so + this never drives a visible re-render on its own. + +### One real waste found: `RepoListStore` re-queries the DB on every sync tick + +`RepoListStore`'s backend subscription (`repo_list.rs:76-109`) treats +`BackendEvent::SyncProgress { .. }` as relevant on its own: + +```rust +BackendEvent::Synced | BackendEvent::SyncProgress { .. } => true, +``` + +Every distinct percentage tick of the bootstrap announcements/deletions +sync calls `this.refresh(cx)`, which is debounced 300ms +(`REFRESH_DEBOUNCE`, `repo_list.rs:16`) and coalesced by `RefreshGate` — so +it's not literally one DB round-trip per percent, but it is several +(bounded by sync duration / 300ms) full re-scans of announcements + +deletions + state events + activity + counts (`run_refresh`, +`repo_list.rs:184-294`) while a single sync is still in flight, instead of +one at the end. This is a deliberate trade-off for progressive reveal (the +repo list fills in live instead of jumping once at 100%), so it's not a +bug, but if that progressive reveal isn't a feature you actually want, +dropping `SyncProgress` from the "relevant" match (keep only `Synced`) removes +several redundant background-thread DB scans per sync for free. Worth a +product decision, not just a code fix. + +No other store subscribes to `SyncProgress` (`RepoStore`, `CheckoutsStore` +do not — checked their subscription callbacks), so this is fully isolated +to `RepoListStore`. + +--- + +## 8. Relay add/connect: stop round-tripping through strings, stop reconnecting the whole pool + +Flagged example (`backend.rs:1071-1074`): + +```rust +for url in latest_grasp_list_servers(events) { + client.add_relay(url.as_str()).await.ok(); +} +client.connect().await; +``` + +Two separate problems, both verified against `nostr-sdk/src/client/url.rs:40-50` +and `nostr-sdk/src/client/api/connect.rs:1-49`: + +1. **`.as_str()` is a pointless round trip.** `latest_grasp_list_servers` + already returns `RelayUrl` values (parsed, validated). `RelayUrlArg` + (what `add_relay` actually accepts) has a direct `impl From` + and `impl From<&RelayUrl>` (`client/url.rs:40-50`) — passing the + `RelayUrl` itself skips a second `RelayUrl::parse` that `.as_str()` + forces (`client/url.rs:26,35`, the `String` variant of `RelayUrlArg` + re-parses on `try_into_relay_url`). Just pass `url`, not `url.as_str()`. +2. **`client.connect()` connects every relay in the pool, not just the one + you added.** Verified in `connect.rs:36-48`: `Client::connect()`'s + `IntoFuture` unconditionally calls `self.client.pool().connect()`, with + no target selection at all — it iterates every relay currently in the + pool. Calling it after adding 1-2 new relays re-issues a connect + attempt to *every* relay already connected too. The `AddRelay` builder + already has the right primitive: `.and_connect()` (`client/api/add.rs:127-132`), + which is threaded straight into `pool.add_relay(url, capabilities, connect, opts)`. + Verified in `pool/mod.rs:157-197` that this is correct **even when the + relay already exists** in the pool: the pool's `add_relay` checks for an + existing entry and, if `connect` is `true`, calls `relay.connect()` on + the existing relay too (`pool/mod.rs:191-194`) — so `.and_connect()` is + never wrong to use, whether the relay is new or already known. + +```rust +// replacement +for url in latest_grasp_list_servers(events) { + client.add_relay(url).and_connect().await.ok(); +} +``` + +The same two problems repeat at every other relay-add call site — fix all +of them the same way: + +- `Backend::bootstrap` (`backend.rs:178-187`): the `BOOTSTRAP_RELAYS` loop + and the `INDEXER_RELAYS` loop (which also sets `.capabilities(...)`, + chain `.and_connect()` onto the same builder) both currently defer to one + trailing `client.connect().await`. +- `connect_repo_relays` (`backend.rs:1446-1449`): today calls `client.add_relay(url).await?;` + then `client.connect_relay(url).await?;` as two separate round trips — + collapse to one `client.add_relay(url).and_connect().await?;`. +- `stage_event_on_relay` (`backend.rs:1772-1782`): same fix, and this one + currently calls the pool-wide `client.connect().await` just to connect + the single relay it's about to stage an event on. + +### Delete the `add_relays` wrapper (`Backend::add_relays`, `backend.rs:1149-1173`) + +Its only two callers (`create_repository`, `backend.rs:505-508`; +`publish_local_repo`, `backend.rs:652-655`) do this today: + +```rust +this.update(cx, |this, cx| { + let urls: Vec = servers.iter().map(ToString::to_string).collect(); + this.add_relays(urls, cx); +})?; +``` + +`servers` is already `Vec` at both call sites — stringifying it +only to have `add_relays` parse it straight back into `RelayUrl` inside +`client.add_relay(&url)` is pure waste, on top of the wrapper itself being +another `cx.spawn` + `push_task` + error-emit layer (§6) around what is, +with the fix above, a two-line loop. Both call sites are already inside a +`cx.spawn(async move |this, cx| ...)` with `client` reachable — inline it: + +```rust +let client = this.update(cx, |this, _cx| this.client.clone())?; +for relay in &servers { + client.add_relay(relay).and_connect().await.ok(); +} +``` + +Delete `Backend::add_relays` entirely once both call sites are inlined. + +--- + +## 9. `create_repository`'s flow is backwards: it inits a mirror, then clones it into the real destination + +This is a real business-logic flaw, not just a style issue. Today +(`backend.rs:437-501`): + +1. `signed_git::init_repository(&path, &name, &description)` — `path` is + `GitCache::repo_path(&addr)`, the app's **internal mirror cache** + location (`crates/signed_git/src/lib.rs:29-33`), not anywhere the user + asked for. This creates a full worktree with an initial commit *there*. +2. `signed_git::clone_repo(&[mirror_url], &destination)` — `destination` is + `folder.join(dir_name)`, the folder the user actually picked. This + clones the mirror just created in step 1 into the real target, via a + `file://` URL (`Url::from_file_path(&path)`, `backend.rs:481-483`). +3. The push (`push_staged_to_grasps`, called with `path` = the **mirror**, + not `destination`) pushes the mirror's objects to the grasp servers. +4. `origin` gets set on *both* the mirror (`backend.rs:463-466`) and the + destination (`backend.rs:492-495`). + +So a brand-new repository gets initialized twice and checked out twice for +what is, at that point, one README and one commit — and the thing that +actually gets pushed (the mirror) isn't the thing the user is left looking +at (the destination). + +Checked `signed_git::init_repository` itself (`signed_git/src/lib.rs:401-477`): +it already creates the target directory (`std::fs::create_dir_all(path)`), +runs `gix::init(path)`, and leaves a fully checked-out worktree with the +README written to disk and the index populated — i.e., it already produces +exactly what step 2's clone is redundantly reproducing. There is no reason +step 1 and step 2 are two different paths. + +Compare with `publish_local_repo` (`backend.rs:599-754`), the sibling flow +for an *existing* local repo: it operates on the user's real folder +directly (`signed_git::worktree_ref_state(&path)`, `root_commit(&path)`) — +no mirror, no extra clone. `create_repository` is the odd one out. + +**The mirror doesn't need to be pre-populated at creation time at all.** +`GitCache::ensure_clone(addr, clone_urls)` (`signed_git/src/lib.rs:45-63`) +already exists precisely to populate the mirror lazily — open it if it's +there, clone it from the announcement's `clone_urls` if it's not — and +it's already what `RepoDetailView::load_repo` calls for every repo, +including the user's own (`workspace/src/views/repo_detail/mod.rs:425-428`). +By the time the UI navigates to the new repo's detail view after +`create_repository` returns, the push has already succeeded, so +`ensure_clone` will clone straight from the just-pushed grasp server — +exactly the same lazy path every other repo already takes. No special +casing needed. + +I checked whether any `workspace` call site compounds this (e.g. by cloning +*again* right after `create_repository` returns) — it doesn't: +`sidebar/create_repo_dialog.rs`'s `create_repository` handler +(`create_repo_dialog.rs:193-222`) just calls `backend.create_repository(...)` +and applies the returned `Announcement`; the flaw is fully contained inside +`Backend::create_repository` itself. + +**Replacement:** initialize directly at `destination`, push from +`destination`, set `origin` once: + +```rust +let commit = signed_git::init_repository(&destination, &name, &description)?; +// ... build the announcement using `commit` as before ... +// push_staged_to_grasps(..., path = &destination, ...) instead of the mirror path +if let Some(base) = servers.first().and_then(grasp_base_url) { + signed_git::set_origin(&destination, &format!("{base}/{owner}/{repo_id}.git"))?; +} +``` + +Delete the mirror `init_repository` call, the `clone_repo` call, the +`Url::from_file_path` mirror-URL construction, and the mirror-side +`ensure_origin` call. This removes a full extra `gix::init` + checkout + +clone from repo creation, and makes `create_repository` consistent with +how `publish_local_repo` already treats the user's working copy as the one +source of truth. + +--- + +## 10. Bootstrap-on-construction should go through `cx.defer`, not run synchronously in `new` + +Verified against the pinned GPUI revision +(`crates/gpui/src/app.rs:1999-2005`, `crates/gpui/src/app/context.rs:296-315`). + +`App::defer(&mut self, f: impl FnOnce(&mut App) + 'static)` — "Schedules +the given function to be run at the end of the current effect cycle, +**allowing entities that are currently on the stack to be returned to the +app**." That's precisely the situation every one of these constructors is +in: `Self` is still being built inside the `cx.new(|cx| ...)` closure when +it reaches out and kicks off real work. `Context::defer_in` also exists +(`app/context.rs:296-315`) but takes a `&Window` — it's for window-bound +views, not the headless global stores below, none of which are constructed +with a `Window` in scope. For these, the applicable API is the window-less +`cx.defer(...)`, reached through `Context`'s `Deref` +(`app/context.rs:25-34`), capturing a `WeakEntity` to get back into +`Self` once deferred: + +```rust +// today, backend.rs:148-160 +let mut this = Self { /* ... */ }; +this.bootstrap(cx); +this + +// replacement +let mut this = Self { /* ... */ }; +let weak = cx.entity().downgrade(); +cx.defer(move |cx| { + weak.update(cx, |this, cx| this.bootstrap(cx)).ok(); +}); +this +``` + +The same pattern — a constructor that calls its own bootstrap-ish method, +or reaches into another entity, before returning `Self` — repeats in every +store: + +| Store | Constructor call site | What it kicks off synchronously | +|---|---|---| +| `Backend` | `backend.rs:159` | `bootstrap(cx)` — adds/connects `BOOTSTRAP_RELAYS`/`INDEXER_RELAYS`, restores the session | +| `RepoListStore` | `repo_list.rs:120-123` | `subscribe_remote(cx)` (negentropy sync against bootstrap relays) + `refresh_initial(cx)` | +| `RepoStore` | `repo.rs:154-159` | `subscribe_remote`, `connect_announced_relays`, `refresh` — each one reaches into the global `Backend` entity | +| `CheckoutsStore` | `checkouts.rs:172-174` | `refresh(cx)` | +| `LocalReposStore` | `local_repos.rs:44` | `rescan(cx)` | +| `ProfileStore` | `profile.rs:119-121` | spawns the batched profile-fetch loop | + +Wrap each of these the same way `Backend::new` is shown above. This isn't +about a currently-observed crash (nothing panics today, because everything +past the initial synchronous field assignment already goes through +`cx.spawn`/`cx.background_spawn`, which only runs later anyway) — it's +about not mixing "construct plain state" with "kick off side effects that +talk to other entities" in the same synchronous call, which is exactly what +`defer` exists to separate, per its own doc comment. + +--- + +## 11. Split independently-observed state into child entities + +`Backend::pushing_repos` (`backend.rs:88`) is `Arc>>` +— it bypasses GPUI's entity system entirely. A view that wants to show "is +repository X currently pushing" has no way to `cx.observe` this; it can +only poll a `Mutex` by hand, and any UI update requires some *other* +notify to happen to piggyback on. Meanwhile every view that only cares +about, say, `current_user` still gets re-invoked on `Backend::notify()` +fired for unrelated reasons (a `sync_progress` tick, a new relay connecting), +because the whole `Backend` is one entity and `cx.notify()` invalidates all +of its observers indiscriminately. + +GPUI's own model is built for exactly this split: an `Entity` works for +any `T: 'static`, not just `Render`-able view state (see the project's own +GPUI notes: "Whenever you need to store application state that +communicates between different parts of your application, you'll want to +use GPUI's entities"). Where a piece of a bigger store's state changes on +its own schedule and has its own, narrower set of observers, pull it out +into a child entity: + +```rust +pub struct Backend { + client: Client, + signer: UniversalSigner, + current_user: Option, + sync_progress: Option<(u64, u64)>, + passphrase_required: bool, + pushing_repos: Entity>, // was Arc>> +} +``` + +A view that only cares whether repo `X` is pushing does +`cx.observe(&backend.read(cx).pushing_repos, |this, pushing, cx| ...)` and +is left alone by every other `Backend` change. `PushGuard` +(`backend.rs:99-110`) becomes a guard that calls +`pushing_repos.update(cx, |set, cx| { set.remove(&addr); cx.notify(); })` +on drop instead of locking a raw `Mutex` — same RAII shape, but now it's a +real, observable GPUI entity instead of a side channel next to the entity +system. Apply the same split to any other `Backend`/store field where the +set of interested observers is a strict subset of the store's full +observer list. + +This principle is also the reason **not** to merge `LocalReposStore` and +`RepoListStore` into one entity — see §13. + +--- + +## 12. One debounce at the source, not one per store + +Flagged example — the notification pump (`backend.rs:126-146`): + +```rust +let mut notifications = pump_client.notifications(); +while let Some(notification) = notifications.next().await { + let ClientNotification::Event { event, .. } = notification else { continue }; + let update = Update::from_event(&event); + if this.update(cx, |_, cx| cx.emit(BackendEvent::NostrUpdate(update))).is_err() { + break; + } +} +``` + +Every single relay-delivered event is emitted as its own +`BackendEvent::NostrUpdate`, immediately. During a negentropy sync +(exactly the bursty case §6/§7 already discuss), this can be hundreds of +emits in a short window. Four different stores (`RepoStore`, +`RepoListStore`, `CheckoutsStore`, and transitively `ProfileStore`) each +subscribe to `Backend` and independently run their own `RefreshGate` +debounce/coalesce dance in response — the same burst gets debounced four +times, once per listener, instead of once at the point it actually enters +the system. + +Centralize it: batch what the pump itself emits, and let each store react +to a batch instead of a stream of singles. The pump already owns the one +place where the burst originates, so it's the natural place to coalesce: + +```rust +let pump = cx.spawn(async move |this, cx| { + let mut notifications = pump_client.notifications(); + let mut pending: Vec = Vec::new(); + + loop { + let next = cx.background_executor().timer(PUMP_DEBOUNCE).fuse(); + futures::select_biased! { + notification = notifications.next() => { + let Some(notification) = notification else { break }; + let ClientNotification::Event { event, .. } = notification else { continue }; + pending.push(Update::from_event(&event)); + } + _ = next => { + if pending.is_empty() { continue; } + let batch = std::mem::take(&mut pending); + if this.update(cx, |_, cx| cx.emit(BackendEvent::NostrUpdate(batch))).is_err() { + break; + } + } + } + } + Ok(()) +}); +``` + +(Sketch — the real version needs `BackendEvent::NostrUpdate` to carry +`Vec` instead of `Update`, and every subscriber's relevance check — +`RepoStore`, `RepoListStore`, `CheckoutsStore`, `ProfileStore` — to check +"does *any* update in the batch match" instead of one `Update`. That's a +mechanical change to four match arms.) + +This doesn't make each store's own `RefreshGate` fully redundant: +`Published`/`Synced`/`SyncProgress` events are emitted directly by +whichever method triggered them (a local `send`, a sync completing), not +through the pump, and can still arrive close together independently of +relay traffic. But those are one-off, user-triggered events, not the +hundred-events-in-a-burst case — so once the pump absorbs the dominant +source of bursts, each store's debounce window can likely shrink +significantly (or, for stores that only ever see one trigger at a time in +practice, be dropped in favor of "fold into the in-flight run" without a +timer at all). Worth measuring after the pump-side batching lands, rather +than speculatively resizing four timers up front. + +--- + +## 13. `local_repos.rs` + `repo_list.rs`: merge the files, not the entities + +These two are structurally near-identical: both hold an `Arc>` +snapshot, refresh it in the background on a trigger, swap it in with +`cx.notify()`, and carry their own `Global` wrapper + `global()`/`set_global()` +pair + `tasks`/`push_task` boilerplate (§6). That similarity is real and +worth collapsing — but checked who actually reads each one before deciding +*how*: + +``` +grep -rn "RepoListStore::global" crates/ → 9 call sites +grep -rn "LocalReposStore::global" crates/ → 5 call sites +``` + +Only **two** places read both together: `CheckoutsStore::new`/`run_refresh` +(`checkouts.rs:126-136`, `checkouts.rs:339-344`) and `SidebarPanel::new`/`refresh` +(`sidebar/mod.rs:55-65`, `sidebar/mod.rs:128-142`). Everywhere else reads +exactly one: + +- `RepoListStore` alone: `RepoStore::action_announcement` (`repo.rs:1071-1078`), + `RepoDetailView::open_upstream` (×2, `mod.rs:1009-1013`, `1034-1044`), + `RepoDetailView::fork_row` (`mod.rs:2596-2606`), + `NewPullRequestView::fork_candidates` (`new_pull_request.rs:569-577`), + `RepoListView::new` (`views/repo_list.rs:111-121`). +- `LocalReposStore` alone: `RepoDetailView::apply_announcement` + (`mod.rs:1875-1877`), `SidebarPanel::render_repos`'s rescan button + (`sidebar/mod.rs:306-309`). + +Given that, collapsing them into **one `Entity`** (one struct holding both +`Vec`s, one `cx.notify()` for both) would make every one of those ~12 +single-store readers pay for the other store's unrelated refreshes — +exactly what §11 says not to do. Wrapping them in a parent that holds two +child entities (`RepoDirectory { local: Entity, remote: +Entity }`) avoids that specific problem, but then every one of +those same ~12 call sites has to change from `RepoListStore::global(cx)` to +`RepoDirectory::global(cx).read(cx).remote` — an extra hop added everywhere, +in exchange for saving exactly one `Global` wrapper struct. Not a good +trade for a codebase this size. + +**Recommendation:** merge the two **files** into one module +(e.g. `repos.rs`), keeping `LocalReposStore` and `RepoListStore` as two +fully independent structs, each still its own `Entity`/`Global` exactly as +today — same public API, same `global()`/`set_global()` pairs, zero +call-site churn. The merge is justified purely as "these are the app's two +repo-listing stores, they belong next to each other," per the project's own +`.rules` guidance to avoid many small files for closely related logic — +not as a reason to share a notify cycle between two things with almost +entirely disjoint observers. + +--- + +## 14. `crates/workspace` has the same task-list pattern as §6 — and there it's an actual bug + +§6 covers `signed_state`'s 6 stores, where the unpruned-`Vec` pattern +is a style/complexity concern with no observed failure, because +`push_task` always pruned before pushing. `crates/workspace` has the exact +same field-and-push shape in 4 views, but **most of it never prunes**: + +``` +grep -rn "tasks.push(task)" crates/workspace/ → 17 call sites +grep -rn "tasks.retain" crates/workspace/ → 1 call site (pull_request_detail.rs:258) +``` + +- `RepoDetailView.tasks` (`mod.rs:178-179`, doc comment: "finished tasks are + pruned on every push" — **this is stale/incorrect**, no `.retain()` + precedes any of its 11 push sites: `mod.rs:384-388`, `532-536`, `650-654`, + `773-777`, `835-839`, `877-881`, `949-953`, `1061-1065`, `1132-1136`, + `1219-1223`, `1315-1319`). +- `NewPullRequestView.tasks` (`new_pull_request.rs:80-84`): 5 push sites, + none pruned (`445-449`, `475-479`, `697-701`, `894-898`, `997-1001`). +- `CommitDiffView` (`diff.rs:407-411`): 1 push site, not pruned. +- `PullRequestDetailView.tasks` (`pull_request_detail.rs:68-72`): the one + correct one — `load` (`pull_request_detail.rs:256-260`) does + `self.tasks.retain(|task| !task.is_ready()); self.tasks.push(task);`. + +So `RepoDetailView.tasks` and `NewPullRequestView.tasks` grow **unbounded** +for as long as the panel stays open: every file preview, ref switch, commit +load, worktree reload, or fork comparison appends one more `Task` that is +never removed. This is a real memory-growth bug, not just a style +preference — a repo detail panel left open through a long session +accumulates one `Task` per interaction, forever. + +Apply the same fix as §6: delete the `tasks` field from all four views and +`.detach()` (or `.detach_and_log_err(cx)`) at every one of the 17 call +sites. Every continuation already tolerates the view being gone +(`this.update_in(cx, ...).ok()`/`?`, same pattern as `signed_state`), so +nothing here needs cancel-on-drop semantics either. Worth noting +`repo_detail/init_dialog.rs`'s `init_repository` (`init_dialog.rs:187-206`) +already does exactly this — `cx.spawn(...).detach()`, no task list at all — +so the fix is bringing the other 4 views in line with a pattern that +already exists once in the same crate. + +--- + +## 15. `Vec` → `Vec` conversion sprawl — fix the 3 `signed_git` signatures, not the 8 call sites + +`Announcement::clone` is `Vec` (`signed_core/src/model.rs`, `Url` being +`nostr`'s re-export of the `url` crate's `Url`, `nostr/src/types/url.rs:15`, +`pub use url::*;`). Every call site that needs to hand those URLs to +`signed_git` first stringifies them: + +``` +grep -rn "\.map(ToString::to_string)\.collect" crates/signed_state crates/workspace +``` + +finds it at `repo.rs:1005-1008` (`merge_pull_request`), `repo.rs:1227` +(`clone_to_folder`), `workspace/repo_detail/mod.rs:397` (`load_repo`), +`new_pull_request.rs:595` and `602` (`choose_fork`, twice — once for the +fork, once for the base), and `pull_request_detail.rs:146-149` and +`738-741` (`load`, `clone_urls_of`). Seven call sites, all producing a +`Vec` that gets handed straight to `signed_git::clone_repo`, +`GitCache::ensure_clone`, or `fetch_repo_refs`. + +The root cause is those three functions' signatures, not the call sites. +Verified in `signed_git/src/lib.rs`: + +```rust +pub fn clone_repo(clone_urls: &[String], path: &Path) -> Result<()> { ... } // lib.rs:125 +pub fn ensure_clone(&self, addr: &RepoAddr, clone_urls: &[String]) -> Result<...> // lib.rs:45 +pub fn fetch_repo_refs(repo_path: &Path, urls: &[String], refspec: &str) -> ... // lib.rs:701 +``` + +all three only ever read each URL as `&str` internally, through the shared +`try_each_url(urls: &[String], ...)` helper (`lib.rs:350`), which does +`attempt(url)` where `url: &String` auto-derefs. Checked whether `Url` could +be passed directly instead of allocating a `String` per URL: **yes** — +`url::Url` implements `AsRef` directly (verified in the pinned `url` +crate source, `url-2.5.8/src/lib.rs:2867`). Making the three functions +generic removes the conversion at every call site instead of patching each +one: + +```rust +fn try_each_url, F>(urls: &[U], verb: &str, mut attempt: F) -> Result<()> +where + F: FnMut(&str) -> Result<()>, +{ + for url in urls { + match attempt(url.as_ref()) { /* ... */ } + } + /* ... */ +} + +pub fn clone_repo>(clone_urls: &[U], path: &Path) -> Result<()> { ... } +pub fn ensure_clone>(&self, addr: &RepoAddr, clone_urls: &[U]) -> Result { ... } +pub fn fetch_repo_refs>(repo_path: &Path, urls: &[U], refspec: &str) -> Result<()> { ... } +``` + +After this, every one of the 7 call sites above passes `&announcement.clone` +directly (a `&[Url]`), deleting the `.iter().map(ToString::to_string).collect::>()` +line entirely — no allocation, no `Display`-then-reparse round trip, +7 fewer lines of boilerplate for free. (`about.rs`'s `url.to_string()` calls +for on-screen display, `about.rs:55-103`, are unrelated — that's genuine +`Url → SharedString` rendering, not a `signed_git` call, and stays as-is.) + +The `Vec → Vec` conversions for `add_relays`/`add_relay` +(§8) are a separate root cause (`RelayUrl` doesn't implement `AsRef`, +checked `nostr/src/types/url.rs`) and are already fixed by §8's move to +`RelayUrlArg`'s native `From`/`From<&RelayUrl>` — no further +change needed there. + +--- + +## 16. `.clone()` audit: the dense clusters in `backend.rs` are the correct idiom, not a flaw + +Went through every `.clone()` in `create_repository`, `publish_local_repo`, +and `push_repo_from` (the three functions with the highest clone density) +looking for copies that could be replaced by a reference. All of them are +`Client`/`UniversalSigner`/`PathBuf`/`String`/`RelayUrl` values being moved +into a separate `'static async move` block for `cx.background_spawn`, which +Rust's ownership rules require to own its captures — this is exactly the +shadowing-clone pattern the project's own `.rules` file endorses ("Use +variable shadowing to scope clones in async contexts for clarity, minimizing +the lifetime of borrowed references"). `Client` itself is a cheap `Arc` +handle clone (`Client(Arc)`, verified `nostr-sdk/src/client/mod.rs:74`), +so even the frequent `client.clone()`/`signer.clone()` pairs before each +`background_spawn` are not doing a deep copy. No changes recommended here — +noting this so it's clear the dense clone clusters were checked, not +skipped, and found to be inherent to the async-boundary structure rather +than avoidable duplication. + +--- + +## 17. Business logic that leaked into `crates/workspace` and should move to `signed_core`/`signed_state` + +Direct answer to "can the view side be thinner": yes, and not speculatively — +found one confirmed duplicate, one cluster of misplaced domain parsing, and +one mutating-flow split across the view/store boundary. The test used to +tell "fine to stay in the view" from "should move": read-only git/data +queries that only shape *what one specific view renders* (diffs, commit +lists, tree snapshots — already audited clean in §5/§7) are fine where they +are; anything that **parses a Nostr event's domain tags**, **decides what's +NIP-34-valid/eligible**, or **builds the payload of a mutating operation** +is domain logic and belongs in `signed_core`/`signed_state`, reusable and +testable without GPUI. + +### Confirmed duplicate: `current_commit_of` + +`signed_core/src/model.rs:183-190` (private, used internally by +`pull_request_patches`) and `workspace/repo_detail/pull_request_detail.rs:709-716` +are **the same function, byte-for-byte**: + +```rust +fn current_commit_of(event: &Event) -> Option { + event + .tags + .iter() + .find_map(|tag| match Nip34Tag::parse(tag.as_slice()) { + Ok(Nip34Tag::CurrentCommit(commit)) => Some(commit.to_string()), + _ => None, + }) +} +``` + +It was reimplemented in `workspace` because `signed_core`'s copy is private. +Fix: make `signed_core`'s `current_commit_of` `pub fn`, delete +`workspace`'s copy, import the shared one. + +### A whole cluster of NIP-34 tag parsing lives next to it, same shape, same problem + +Still in `pull_request_detail.rs`, zero GPUI/UI dependency in any of them: + +- `merge_base_of(event: &Event) -> Option` (`pull_request_detail.rs:721-729`) +- `clone_urls_of(event: &Event) -> Option>` (`pull_request_detail.rs:734-742`) +- `branch_name_of(event: &Event) -> Option` (`pull_request_detail.rs:745-753`) +- `latest_update<'a>(events: impl Iterator, root: &Event) -> Option<&'a Event>` (`pull_request_detail.rs:756-766`) — + walks a PR's `GitPullRequestUpdate` events to find the newest revision from + the root's author, the exact same *shape* of problem `signed_core::model::pull_request_patches` + already solves for patch series (`model.rs:95-135`, forward/backward + reply-chain walking). + +These all take a plain `&Event` (or an iterator of them) and return plain +data — nothing here needs `Context`/`Window`/`cx`. They belong next to +`Announcement::from_event`, `parse_state`, and `pull_request_patches` in +`signed_core`, as `pub fn`s with their own unit tests (this file's test +module, `pull_request_detail.rs:840+`, already builds fixture events with a +local `signed()`/`pr_root()` helper — `signed_core`'s test module has the +same fixture-building pattern already; the tests move with the functions, +no new test infrastructure needed). + +### A mutating flow split across the view/store boundary: patch generation in `submit` + +`NewPullRequestView::submit` (`new_pull_request.rs:900-1000`) does this +before calling into the store: + +```rust +let patch = cx.background_spawn({ + /* ... */ + async move { format_patch_between(Path::new(&repo_path), &merge_base, &compare_ref) } +}).await; + +let patch = match patch { + Ok(patch) if !patch.is_empty() => patch, + Ok(_) => { /* "No commits between the branches to propose" */ return Ok(()); } + Err(error) => { /* "Failed to generate the patch: {error}" */ return Ok(()); } +}; + +store.update(cx, |store, cx| { + store.open_pull_request(/* subject, description, branch_name, patch, ... */) +}); +``` + +`RepoStore::open_pull_request` (`repo.rs:554-558`) and `update_pull_request` +(`repo.rs:829-833`) both already take a ready-made `patch: String` — a +reasonable, uniform boundary in general (it's also exactly right for +`pull_request_detail.rs`'s "update PR" dialog, `pull_request_detail.rs:648-700`, +where the patch is literally pasted by the user into a textarea, no git +involved). But for the "compare two branches" flow, *generating* that patch +text — calling `signed_git::format_patch_between`, deciding empty-diff is +an error, and wording that error — is exactly the same kind of "turn git +state into the payload of a Nostr publish" work `Backend::create_repository`/ +`publish_local_repo` already do internally (`worktree_ref_state`, +`root_commit`), just for a different event kind. It shouldn't be the one +case where that responsibility sits in the view instead of the store. + +**Recommendation:** give `RepoStore` (or a free function in `signed_state` +it calls) a method that takes the two refs instead of a ready-made patch, +e.g. `RepoStore::open_pull_request_from_refs(repo_path, base_ref, compare_ref, +subject, description, draft, cx) -> Task>`, which does +the `format_patch_between` + empty-check + `open_pull_request` sequence +internally and returns one descriptive error on failure. `submit` shrinks to +gathering the text-field values and calling it, then closing the panel — +no `signed_git` import needed in `new_pull_request.rs` at all for this path. + +### Borderline, worth doing while touching the same file: `fork_candidates`/`fork_namespace` + +`fork_candidates` (`new_pull_request.rs:117-135`) filters/partitions +`&[Announcement]` into "own" vs. "others" fork sources using the +already-domain `Announcement::is_fork_of` predicate (`signed_core/src/model.rs:281-285`, +correctly reused, not reimplemented) — it's pure data transformation with no +GPUI dependency, and has its own private unit tests in `new_pull_request.rs` +building fixture announcements, again duplicating test-fixture machinery +`signed_core`'s own test module already has. `fork_namespace` +(`new_pull_request.rs:108-114`, formats the `refs/fork//` +namespace string) is the same shape — small, but it's the one place that +convention is decided, and it pairs naturally with `signed_git`'s ref-naming +conventions. Both are safe, low-risk moves to `signed_core`: unlike +`fork_display_name`/`shorten_owner`/`truncate_label`/the `*_source_item` +builders in the same file (genuine presentation logic — `SharedString` +truncation, `PopupMenuItem` construction — correctly left where they are), +these two don't touch a single GPUI type. + +### What's already thin and should stay exactly where it is + +For contrast, checked `RepoDetailView`'s git-touching methods +(`load_repo`, `load_commits`, `switch_ref`, `reload_worktree`, +`catch_up_worktree`, `push_unpushed_checkout`) and `NewPullRequestView::reload_compare` +(`new_pull_request.rs:808-897`, computing `merge_base`/commit +list/diff purely to populate the compare pane): these call `signed_git` +directly too, but only to compute **read-only data this one view renders** +— nothing here is parsed from a Nostr event, decides NIP-34 eligibility, or +builds a publish payload. Moving these into `signed_state` would just add +an indirection layer with no reuse benefit, contradicting "keep it simple." +Same verdict as `create_repo_dialog.rs`'s and `init_dialog.rs`'s handlers +(§9): they already do nothing but gather form input and call one `Backend` +method. + +--- + +## Action plan, in order of risk/reward + +1. **Delete the fetch/sync dedup cache** (§3). Pure removal, no behavior + change for the intended usage pattern (each call site already has, or + trivially gets, its own guard). Lowest risk, do first. +2. **Remove the `tasks: Vec>` + `push_task` boilerplate**, in + both `signed_state` (§6) and `crates/workspace` (§14), in favor of + `.detach()`/`.detach_and_log_err(cx)`. Independent of every other change + here, touches 10 files, all mechanical — and fixes a real unbounded-growth + bug in `RepoDetailView`/`NewPullRequestView` along the way. +3. **Fix the relay add/connect calls** (§8): drop `.as_str()`/`ToString` + round trips, replace `add_relay` + blanket `client.connect()`/ + `connect_relay` pairs with `add_relay(url).and_connect()`, and delete + `Backend::add_relays`. Mechanical, no behavior change beyond "connect + only what was just added." +4. **Fix `bootstrap_user`** to sync+query instead of `fetch_events` (§1). + One function, fully covered by existing tests for + `latest_grasp_list_servers`. +5. **Generalize the 3 `signed_git` URL-list signatures** to `&[impl AsRef]` + (§15), then delete the now-redundant `.map(ToString::to_string).collect()` + at all 7 call sites. Self-contained to `signed_git`'s public API plus a + one-line change per call site; re-run `signed_git`'s existing tests + (`clone_repo`/`fetch_repo_refs` already have coverage). +6. **Fix `create_repository`'s init/clone ordering** (§9): initialize and + push directly at the user's chosen destination, drop the mirror + pre-population entirely and let `ensure_clone` populate it lazily like + every other repo. Self-contained to one function; verify against this + crate's existing `init_repository`/push tests plus a manual + create-repository-then-open-detail-view pass. +7. **Merge `local_repos.rs` and `repo_list.rs` into one file** (§13), + keeping both stores as independent entities. Purely organizational, zero + call-site changes, safe to do any time. +8. **Route construction-time bootstrap through `cx.defer`** (§10) in all + six stores listed there. Mechanical per store, but touch them one at a + time and re-run each store's test suite, since ordering-sensitive + assumptions (e.g. a test that asserts state right after `cx.new`) may + need `cx.run_until_parked()` inserted where they didn't before. +9. **Consolidate the send paths** (§2): introduce the single + `require_relay_accepted` helper, delete + `Backend::send`/`publish_event`/`send_fire_and_forget`/`broadcast_event`/ + `RepoStore::send`, switch `retract_events` to `EventDeletionRequest` + (one deletion event per target, no `k` tag), and make each send site + explicit about bypassing gossip (§4) with `.broadcast()`/`.to(relays)`. + This is the biggest diff and touches every publish call site (`repo.rs`, + `backend.rs`), so do it as its own PR with full test-suite coverage + before/after. +10. **Split `pushing_repos` (and similar fields) into a child entity** (§11). + Small, isolated change once §9's `PushGuard` rewrite is in flight — do + them together since both touch `PushGuard`. +11. **Centralize the notification-pump debounce** (§12). This one is the + most speculative of the batch — land it after §7's `SyncProgress` + decision and re-measure whether each store's own `RefreshGate` window + can shrink, rather than assuming the exact shape up front. +12. **Optional, product call:** drop `SyncProgress` from + `RepoListStore`'s relevant-event match if progressive reveal during + bootstrap sync isn't a feature you want (§7). +13. **Spike `diffy::patch_set::PatchSet`** to replace `signed_git`'s + hand-rolled `git format-patch` parser (§5). Separate PR, separate + crate, no interaction with the nostr-facing changes above — do this in + parallel if you have a second contributor, otherwise last since it's + the largest and riskiest single change (needs fixture-by-fixture + verification against the existing test suite). +14. **Move the misplaced `workspace` domain logic to `signed_core`/`signed_state`** (§17): + make `current_commit_of` `pub` in `signed_core` and delete the + `workspace` duplicate; move `merge_base_of`/`clone_urls_of`/`branch_name_of`/ + `latest_update` and `fork_candidates`/`fork_namespace` there too, tests + included; give `RepoStore` a refs-in-patch-out method so + `NewPullRequestView::submit` stops calling `format_patch_between` + itself. Low risk, no behavior change, best done as its own small PR per + function cluster rather than one big move. + +Everything **not** listed above (per-repo/per-list `Entity` stores, the +`RefreshGate` debounce/coalesce pattern, `Nip34Tag`/`Coordinate`/`Filter` +usage in `signed_core`, the GRASP push-retry state machine in +`push_staged_to_grasps`, the `UniversalSigner` abstraction, and the dense +`.clone()` clusters audited in §16) was checked and already matches "use +the SDK directly, no unnecessary wrapper" — those should be left alone.