From a4dcbbc560f5822a92301a550dc5019ddc9dd814 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 13 Sep 2026 11:54:37 +0700 Subject: [PATCH] refactor --- crates/signed_state/src/repo.rs | 28 +- crates/signed_state/src/repos.rs | 31 +- crates/workspace/src/views/issues/detail.rs | 17 +- crates/workspace/src/views/issues/mod.rs | 115 +++--- .../src/views/pull_requests/detail.rs | 336 +++++++++++------- .../workspace/src/views/pull_requests/mod.rs | 129 ++++--- docs/repo-state-plan.md | 330 +++++++++++++++++ 7 files changed, 719 insertions(+), 267 deletions(-) create mode 100644 docs/repo-state-plan.md diff --git a/crates/signed_state/src/repo.rs b/crates/signed_state/src/repo.rs index 854a2a1..206189b 100644 --- a/crates/signed_state/src/repo.rs +++ b/crates/signed_state/src/repo.rs @@ -1,7 +1,6 @@ use std::borrow::Cow; use std::collections::{HashMap, HashSet}; use std::path::PathBuf; -use std::time::Duration; use anyhow::{Error, bail}; use bitcoin_hashes::sha1::Hash as Sha1Hash; @@ -22,9 +21,6 @@ use crate::git_store::GitStore; use crate::refresh::{RefreshGate, RefreshRequest}; use crate::repos::RepoListStore; -/// Delay between a refresh request and the actual re-query. -const REFRESH_DEBOUNCE: Duration = Duration::from_millis(300); - /// Maximum size of one patch event. /// /// NIP-34 suggests patches when each event is under 60kb. @@ -37,6 +33,10 @@ const MAX_PATCH_EVENT_BYTES: usize = 60 * 1024; pub struct RepoStore { addr: RepoAddr, pub announcement: Option, + /// The first local pass has been applied. + /// + /// Views distinguish "no data yet" from a genuinely empty repository with it. + pub loaded: bool, /// Branch pointed to by `HEAD` in the latest state announcement. pub head: Option, pub issues: Vec, @@ -143,6 +143,7 @@ impl RepoStore { Self { addr, announcement: None, + loaded: false, head: None, issues: Vec::new(), patches: Vec::new(), @@ -228,17 +229,15 @@ impl RepoStore { } /// Re-query the local database and update all fields. + /// + /// Runs immediately. The backend pump already batches the relay events that + /// trigger a refresh, so no per-store debounce is needed. pub fn refresh(&mut self, cx: &mut Context) { if self.refresh.request() != RefreshRequest::Schedule { return; } - cx.spawn(async move |this, cx| { - cx.background_executor().timer(REFRESH_DEBOUNCE).await; - - this.update(cx, |this, cx| this.run_refresh(cx)) - }) - .detach(); + self.run_refresh(cx); } fn run_refresh(&mut self, cx: &mut Context) { @@ -402,10 +401,16 @@ impl RepoStore { // is polled in bursts while a sync is in flight; notifying on // every identical pass would re-render the repository panel // several times for no visible change. + // + // The first pass is the exception: it must notify even when it + // found nothing, so views can leave their loading state and show + // the empty result. + let first_pass = !this.loaded; let head_changed = state .as_ref() .is_some_and(|(_, head)| this.head.as_deref() != head.as_deref()); - let changed = this.announcement != announcement + let changed = first_pass + || this.announcement != announcement || head_changed || this.issues != issues || this.patches != patches @@ -435,6 +440,7 @@ impl RepoStore { this.status_by_root = status_by_root; this.open_issue_count = open_issue_count; this.open_pr_count = open_pr_count; + this.loaded = true; this.version = this.version.wrapping_add(1); // Comments and statuses without an `a` tag. diff --git a/crates/signed_state/src/repos.rs b/crates/signed_state/src/repos.rs index 4189909..dbebf58 100644 --- a/crates/signed_state/src/repos.rs +++ b/crates/signed_state/src/repos.rs @@ -117,11 +117,6 @@ impl LocalReposStore { } } -/// Delay between a refresh request and the actual re-query. -/// -/// Bursts of events, e.g. sync progress ticks, collapse into one query. -const REFRESH_DEBOUNCE: Duration = Duration::from_millis(300); - /// How far back activity events count toward a repository's last activity. const ACTIVITY_WINDOW: Duration = Duration::from_secs(90 * 86_400); @@ -220,7 +215,7 @@ impl RepoListStore { cx.defer(move |cx| { weak.update(cx, |this, cx| { this.subscribe_remote(cx); - this.refresh_initial(cx); + this.refresh(cx); }) .ok(); }); @@ -254,33 +249,19 @@ impl RepoListStore { }); } - /// One-shot initial load. - /// - /// Query the local database immediately, no debounce. - /// Stored announcements appear as soon as the app opens. - fn refresh_initial(&mut self, cx: &mut Context) { - debug_assert!(!self.refresh.debouncing()); - if self.refresh.running() { - self.refresh.request(); - return; - } - self.run_refresh(cx); - } - /// Re-query the local database. + /// + /// Runs immediately. The backend pump already batches the relay events that + /// trigger a refresh, so no per-store debounce is needed. pub fn refresh(&mut self, cx: &mut Context) { if self.refresh.request() != RefreshRequest::Schedule { return; } - cx.spawn(async move |this, cx| { - cx.background_executor().timer(REFRESH_DEBOUNCE).await; - this.update(cx, |this, cx| this.run_refresh(cx)) - }) - .detach(); + self.run_refresh(cx); } - /// One query and apply cycle, the debounced entry point. + /// One query and apply cycle, the refresh entry point. fn run_refresh(&mut self, cx: &mut Context) { self.refresh.begin(); diff --git a/crates/workspace/src/views/issues/detail.rs b/crates/workspace/src/views/issues/detail.rs index d6911f4..c80a69a 100644 --- a/crates/workspace/src/views/issues/detail.rs +++ b/crates/workspace/src/views/issues/detail.rs @@ -1,8 +1,8 @@ use dock::{BasePanel, Panel, PanelEvent}; use gpui::prelude::*; use gpui::{ - App, Context, Entity, EventEmitter, FocusHandle, Focusable, Render, SharedString, Window, div, - relative, + App, Context, Entity, EventEmitter, FocusHandle, Focusable, Render, SharedString, Subscription, + Window, div, relative, }; use gpui_component::input::TextareaState; use gpui_component::scroll::ScrollableElement; @@ -17,12 +17,13 @@ use crate::views::repo::helpers::{comment_form, comments_section, issue_roots, s /// Detail panel of a single issue. pub struct IssueDetailView { + focus_handle: FocusHandle, /// Repo store holding the issues and their statuses. store: Entity, issue_id: EventId, /// Input state of the comment textarea. comment_input: Entity, - focus_handle: FocusHandle, + _subscription: Subscription, } impl IssueDetailView { @@ -35,11 +36,14 @@ impl IssueDetailView { let comment_input = cx.new(|cx| TextareaState::new(window, cx).placeholder("Leave a comment...")); + let subscription = cx.observe(&store, |_this, _store, cx| cx.notify()); + Self { focus_handle: cx.focus_handle(), store, issue_id, comment_input, + _subscription: subscription, } } } @@ -81,7 +85,12 @@ impl Render for IssueDetailView { let store = self.store.read(cx); let Some(issue) = store.issues.iter().find(|issue| issue.id == self.issue_id) else { - return placeholder("Issue not found", cx); + // The store has not applied its first pass yet, the issue may still arrive. + return if store.loaded { + placeholder("Issue not found", cx) + } else { + placeholder("Loading issue...", cx) + }; }; let (title, author, picture, status, age, issue_id, content) = { diff --git a/crates/workspace/src/views/issues/mod.rs b/crates/workspace/src/views/issues/mod.rs index 6aceaec..311487e 100644 --- a/crates/workspace/src/views/issues/mod.rs +++ b/crates/workspace/src/views/issues/mod.rs @@ -4,8 +4,8 @@ use assets::CustomIconName; use dock::{BasePanel, DockArea, Panel, PanelEvent, add_center_panel, panel_handle}; use gpui::prelude::*; use gpui::{ - AnyElement, App, Context, Entity, EventEmitter, FocusHandle, Focusable, Pixels, Render, - SharedString, Size, WeakEntity, Window, div, px, size, + AnyElement, App, AppContext, Context, Entity, EventEmitter, FocusHandle, Focusable, Pixels, + Render, SharedString, Size, Subscription, WeakEntity, Window, div, px, size, }; use gpui_component::button::{Button, ButtonVariants}; use gpui_component::dialog::{DialogDescription, DialogFooter, DialogHeader, DialogTitle}; @@ -62,27 +62,38 @@ pub struct IssuesView { filter: IssueFilter, /// Per-row heights of the virtual list. item_sizes: Rc>>, - /// The filtered issue count [`Self::item_sizes`] was built for. - issue_len: usize, /// Indices into the store's `issues` matching [`Self::filter`]. visible_issues: Vec, /// Header counts `(total, open, closed)`, rebuilt with [`Self::visible_issues`]. counts: (usize, usize, usize), - /// Store version and filter the cached rows/counts were built from. - cache_key: Option<(u64, IssueFilter)>, + /// Filter [`Self::visible_issues`] was last rebuilt for. + /// + /// A filter change notifies even when the visible rows are unchanged, + /// e.g. switching between two empty filters. + synced_filter: IssueFilter, /// Virtual list state of the issues list. scroll_handle: VirtualListScrollHandle, + /// Rebuild the rows and re-render when the store's data changes. + _subscription: Subscription, } impl IssuesView { pub fn new( dock_area: WeakEntity, store: Entity, - _window: &mut Window, + window: &mut Window, cx: &mut Context, ) -> Self { let repo_name = store.read(cx).name(); + let subscription = cx.observe(&store, |this, _store, cx| { + this.rebuild(cx); + }); + + cx.defer_in(window, |this, _window, cx| { + this.rebuild(cx); + }); + Self { focus_handle: cx.focus_handle(), dock_area, @@ -90,14 +101,60 @@ impl IssuesView { repo_name, filter: IssueFilter::Open, item_sizes: Rc::new(Vec::new()), - issue_len: 0, visible_issues: Vec::new(), counts: (0, 0, 0), - cache_key: None, + synced_filter: IssueFilter::Open, scroll_handle: VirtualListScrollHandle::new(), + _subscription: subscription, } } + fn rebuild(&mut self, cx: &mut Context) { + let filter = self.filter; + + let (visible_issues, counts) = { + let store = self.store.read(cx); + let mut counts = (0usize, 0usize, 0usize); + + let visible_issues: Vec = store + .issues + .iter() + .enumerate() + .filter_map(|(ix, issue)| { + let status = store.status_of(issue); + counts.0 += 1; + match status { + RepoStatus::Open => counts.1 += 1, + RepoStatus::Closed => counts.2 += 1, + RepoStatus::Draft | RepoStatus::Applied => {} + } + filter.matches(status).then_some(ix) + }) + .collect(); + + (visible_issues, counts) + }; + + let filter_changed = self.synced_filter != filter; + let visible_issues_changed = self.visible_issues != visible_issues; + let counts_changed = self.counts != counts; + + if !filter_changed && !visible_issues_changed && !counts_changed { + return; + } + + self.item_sizes = Rc::new(vec![ + size(px(0.), px(ISSUE_ROW_HEIGHT)); + visible_issues.len() + ]); + + self.synced_filter = filter; + self.visible_issues = visible_issues; + self.counts = counts; + + cx.notify(); + } + /// Open the detail panel of `issue_id` in the dock area. fn open_issue_detail( &mut self, @@ -199,7 +256,7 @@ impl IssuesView { .selected(self.filter == IssueFilter::All) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = IssueFilter::All; - cx.notify(); + this.rebuild(cx); })), ) .child( @@ -209,7 +266,7 @@ impl IssuesView { .selected(self.filter == IssueFilter::Open) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = IssueFilter::Open; - cx.notify(); + this.rebuild(cx); })), ) .child( @@ -219,7 +276,7 @@ impl IssuesView { .selected(self.filter == IssueFilter::Closed) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = IssueFilter::Closed; - cx.notify(); + this.rebuild(cx); })), ), ) @@ -324,41 +381,7 @@ impl Focusable for IssuesView { impl Render for IssuesView { fn render(&mut self, _window: &mut Window, cx: &mut Context) -> impl IntoElement { let filter = self.filter; - - // Rows and counts are rebuilt only when the store refreshed or filter changed. - let version = self.store.read(cx).version(); - - if self.cache_key != Some((version, filter)) { - let store = self.store.read(cx); - let mut counts = (0usize, 0usize, 0usize); - self.visible_issues = store - .issues - .iter() - .enumerate() - .filter_map(|(ix, issue)| { - let status = store.status_of(issue); - counts.0 += 1; - match status { - RepoStatus::Open => counts.1 += 1, - RepoStatus::Closed => counts.2 += 1, - RepoStatus::Draft | RepoStatus::Applied => {} - } - filter.matches(status).then_some(ix) - }) - .collect(); - self.counts = counts; - self.cache_key = Some((version, filter)); - } - let count = self.visible_issues.len(); - - // The virtual list's item count comes from `item_sizes`. - // Rebuild it whenever the filtered issue count changes. - if count != self.issue_len { - self.issue_len = count; - self.item_sizes = Rc::new(vec![size(px(0.), px(ISSUE_ROW_HEIGHT)); count]); - } - let sizes = self.item_sizes.clone(); let scroll_handle = self.scroll_handle.clone(); diff --git a/crates/workspace/src/views/pull_requests/detail.rs b/crates/workspace/src/views/pull_requests/detail.rs index 8439a3b..f4d7c4d 100644 --- a/crates/workspace/src/views/pull_requests/detail.rs +++ b/crates/workspace/src/views/pull_requests/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, WeakEntity, Window, div, px, relative, size, + SharedString, Size, Subscription, WeakEntity, Window, div, px, relative, size, }; use gpui_component::button::{Button, ButtonVariants}; use gpui_component::clipboard::Clipboard; @@ -20,9 +20,9 @@ use gpui_component::{ ActiveTheme, Sizable, StyledExt, VirtualListScrollHandle, WindowExt, h_flex, v_flex, v_virtual_list, }; -use nostr::prelude::{Event, EventId, Kind}; +use nostr::prelude::{Event, EventId, Kind, Url}; use signed_core::{ - activity_subject, branch_name_of, clone_urls_of, current_commit_of, latest_update, + RepoAddr, activity_subject, branch_name_of, clone_urls_of, current_commit_of, latest_update, merge_base_of, pull_request_patch, }; use signed_git::{FileCommit, patch_commits, patch_diffs}; @@ -36,6 +36,23 @@ use crate::views::repo::helpers::{comment_form, comments_section, pr_roots, side /// Height of one commit row in the commits tab's virtual list. const ROW_HEIGHT: f32 = 37.; +/// Shown once the store's first pass is applied and the root PR is still absent. +const NOT_FOUND: &str = "Pull request not found"; + +/// Root PR inputs one diff load is keyed to. +/// +/// A store refresh re-binds the panel, and reloads only when these change. +#[derive(Clone, PartialEq, Eq)] +struct PrBinding { + description: String, + patch: String, + tip: Option, + base: Option, + clone_urls: Vec, + addr: RepoAddr, + has_patch_link: bool, +} + /// Detail panel of a single pull request. pub struct PullRequestDetailView { focus_handle: FocusHandle, @@ -61,6 +78,10 @@ pub struct PullRequestDetailView { /// The patch is being parsed on a background task. loading: bool, error: Option, + /// Root PR inputs the in-flight diff load was started for. + bound: Option, + /// Generation of the in-flight diff load. Stale results are discarded. + load_generation: u64, /// Active header tab, 0 = Discussion, 1 = Files, 2 = Commits. active_tab: usize, /// Changed-files explorer and per-file diff, like the commit and compare views. @@ -69,6 +90,10 @@ pub struct PullRequestDetailView { commit_item_sizes: Rc>>, /// Virtual list state of the commits tab. commit_scroll_handle: VirtualListScrollHandle, + /// Re-render when the store's first pass or a later refresh lands. + /// Item panels are cached by the dock, so without this observer a panel + /// opened before the store loaded would stay on its placeholder. + _subscription: Subscription, } impl PullRequestDetailView { @@ -85,9 +110,11 @@ impl PullRequestDetailView { let comment_input = cx.new(|cx| TextareaState::new(window, cx).placeholder("Leave a comment...")); + let subscription = cx.observe(&store, |this, _store, cx| this.sync(cx)); + // Defer loading until the window is ready, like the commit diff view. - cx.defer_in(window, |this, window, cx| { - this.load(window, cx); + cx.defer_in(window, |this, _window, cx| { + this.sync(cx); }); Self { @@ -103,156 +130,215 @@ impl PullRequestDetailView { commits: Vec::new(), loading: true, error: None, + bound: None, + load_generation: 0, active_tab: 0, pane, commit_item_sizes: Rc::new(Vec::new()), commit_scroll_handle: VirtualListScrollHandle::new(), + _subscription: subscription, } } - /// Snapshot the PR events from the store. - fn load(&mut self, window: &mut Window, cx: &mut Context) { + /// Snapshot the root PR from the store and reload the diff when it changed. + /// + /// Re-runs on construction and on every store refresh. Item panels are + /// cached by the dock, so this is the only way a panel opened before the + /// store's first pass learns about its PR. + fn sync(&mut self, cx: &mut Context) { + let loaded = self.store.read(cx).loaded; + + let binding = { + let store = self.store.read(cx); + + store + .pull_requests + .iter() + .find(|pr| pr.id == self.pr_id && pr.kind == Kind::GitPullRequest) + .map(|root| { + let update = latest_update(store.pull_requests.iter(), root); + + let tip = update + .and_then(current_commit_of) + .or_else(|| current_commit_of(root)); + + let base = update + .and_then(merge_base_of) + .or_else(|| merge_base_of(root)); + + let clone_urls = clone_urls_of(root) + .or_else(|| store.announcement.as_ref().map(|a| a.clone.clone())) + .unwrap_or_default(); + + PrBinding { + description: root.content.clone(), + patch: pull_request_patch(root, store.patches.iter()), + tip, + base, + clone_urls, + addr: store.addr().clone(), + has_patch_link: root.tags.event_ids().next().is_some(), + } + }) + }; + + let Some(binding) = binding else { + self.sync_missing(loaded, cx); + return; + }; + + if self.bound.as_ref() == Some(&binding) { + return; + } + + self.bound = Some(binding.clone()); + self.load_diff(binding, cx); + } + + /// The store does not hold the root PR yet, or at all. + /// + /// Loading until the first pass is applied, not found afterwards. + fn sync_missing(&mut self, loaded: bool, cx: &mut Context) { + self.bound = None; + + if !loaded { + if !self.loading || self.error.is_some() { + self.loading = true; + self.error = None; + cx.notify(); + } + return; + } + + if self.error.as_deref() != Some(NOT_FOUND) { + self.loading = false; + self.error = Some(NOT_FOUND.into()); + cx.notify(); + } + } + + /// Load the bound PR's changed files and commits. + /// + /// Nostr-backed pull requests parse the patch series, git-backed ones fetch + /// the clone and diff the `merge-base..tip` range. + fn load_diff(&mut self, binding: PrBinding, cx: &mut Context) { self.loading = true; self.error = None; + self.description = binding.description.clone().into(); + self.current_commit = binding.tip.clone().map(SharedString::from); cx.notify(); let cache = GitStore::global(cx).cache().clone(); - let (description, patch, current_commit, merge_base, clone_urls, addr, has_patch_link) = { - let store = self.store.read(cx); + self.load_generation = self.load_generation.wrapping_add(1); + let generation = self.load_generation; - let Some(root) = store - .pull_requests - .iter() - .find(|pr| pr.id == self.pr_id && pr.kind == Kind::GitPullRequest) - else { - self.loading = false; - self.error = Some("Pull request not found".into()); - cx.notify(); - return; + let PrBinding { + patch, + tip, + base, + clone_urls, + addr, + has_patch_link, + .. + } = binding; + + let task: gpui::Task> = cx.spawn(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 update = latest_update(store.pull_requests.iter(), root); + let git = if use_nostr { + None + } else { + let cache = cache.clone(); + let addr = addr.clone(); + let clone_urls = clone_urls.clone(); + let base = base.clone(); + let tip = tip.clone(); - let tip = update - .and_then(current_commit_of) - .or_else(|| current_commit_of(root)); + Some( + cx.background_spawn(async move { + let repo = cache.ensure_clone(&addr, &clone_urls)?; - let base = update - .and_then(merge_base_of) - .or_else(|| merge_base_of(root)); + let workdir = repo + .workdir() + .ok_or_else(|| anyhow::anyhow!("repository has no worktree"))? + .to_path_buf(); - let clone_urls = clone_urls_of(root) - .or_else(|| store.announcement.as_ref().map(|a| a.clone.clone())); + let tip = + tip.ok_or_else(|| anyhow::anyhow!("pull request has no tip commit"))?; - ( - root.content.clone(), - pull_request_patch(root, store.patches.iter()), - tip, - base, - clone_urls.unwrap_or_default(), - store.addr().clone(), - root.tags.event_ids().next().is_some(), - ) - }; + 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() + } + }; - self.description = description.into(); + let diff = signed_git::worktree_commit_range_diff(&workdir, &base, &tip)?; + let commits = + signed_git::worktree_commit_range_commits(&workdir, &base, &tip)?; - 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) } + Ok::<_, anyhow::Error>((diff, commits, workdir)) }) - .await; + .await, + ) + }; - let nostr_commits = cx - .background_spawn({ - let patch = patch.clone(); - async move { patch_commits(&patch) } - }) - .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), + }; - // 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, - }; + this.update(cx, |this, cx| { + // A newer binding superseded this load. + if this.load_generation != generation { + return; + } - 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(); + this.loading = false; + this.worktree = worktree; + this.commit_item_sizes = Rc::new(vec![size(px(0.), px(ROW_HEIGHT)); commits.len()]); + this.commits = commits; - 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()); - } + 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(()) + }); task.detach(); } diff --git a/crates/workspace/src/views/pull_requests/mod.rs b/crates/workspace/src/views/pull_requests/mod.rs index e32bdda..4af7ce9 100644 --- a/crates/workspace/src/views/pull_requests/mod.rs +++ b/crates/workspace/src/views/pull_requests/mod.rs @@ -4,8 +4,8 @@ use assets::CustomIconName; use dock::{BasePanel, DockArea, Panel, PanelEvent, add_center_panel, panel_handle}; use gpui::prelude::*; use gpui::{ - AnyElement, App, Context, Entity, EventEmitter, FocusHandle, Focusable, Pixels, Render, - SharedString, Size, WeakEntity, Window, div, px, size, + AnyElement, App, AppContext, Context, Entity, EventEmitter, FocusHandle, Focusable, Pixels, + Render, SharedString, Size, Subscription, WeakEntity, Window, div, px, size, }; use gpui_base::Button as BaseButton; use gpui_component::alert::Alert; @@ -70,27 +70,38 @@ pub struct PullRequestsView { filter: PullRequestFilter, /// Per-row heights of the virtual list. item_sizes: Rc>>, - /// The filtered pull request count [`Self::item_sizes`] was built for. - pr_len: usize, /// Indices into the store's `pull_requests` matching [`Self::filter`]. visible_prs: Vec, /// Header counts `(total, open, closed, draft, merged)`. counts: (usize, usize, usize, usize, usize), - /// Store version and filter the cached rows/counts were built from. - cache_key: Option<(u64, PullRequestFilter)>, + /// Filter [`Self::visible_prs`] was last rebuilt for. + /// + /// A filter change notifies even when the visible rows are unchanged, + /// e.g. switching between two empty filters. + synced_filter: PullRequestFilter, /// Virtual list state of the pull requests list. scroll_handle: VirtualListScrollHandle, + /// Rebuild the rows and re-render when the store's data changes. + _subscription: Subscription, } impl PullRequestsView { pub fn new( dock_area: WeakEntity, store: Entity, - _window: &mut Window, + window: &mut Window, cx: &mut Context, ) -> Self { let repo_name = store.read(cx).name(); + let subscription = cx.observe(&store, |this, _store, cx| { + this.rebuild(cx); + }); + + cx.defer_in(window, |this, _window, cx| { + this.rebuild(cx); + }); + Self { focus_handle: cx.focus_handle(), dock_area, @@ -98,14 +109,62 @@ impl PullRequestsView { repo_name, filter: PullRequestFilter::Open, item_sizes: Rc::new(Vec::new()), - pr_len: 0, visible_prs: Vec::new(), counts: (0, 0, 0, 0, 0), - cache_key: None, + synced_filter: PullRequestFilter::Open, scroll_handle: VirtualListScrollHandle::new(), + _subscription: subscription, } } + /// Rebuild the visible rows, header counts and virtual-list sizes. + fn rebuild(&mut self, cx: &mut Context) { + let filter = self.filter; + + let (visible_prs, counts) = { + let store = self.store.read(cx); + let mut counts = (0usize, 0usize, 0usize, 0usize, 0usize); + + let visible_prs: Vec = store + .pull_requests + .iter() + .enumerate() + .filter_map(|(ix, pr)| { + if pr.kind != Kind::GitPullRequest { + return None; + } + + let status = store.status_of(pr); + counts.0 += 1; + + match status { + RepoStatus::Open => counts.1 += 1, + RepoStatus::Closed => counts.2 += 1, + RepoStatus::Draft => counts.3 += 1, + RepoStatus::Applied => counts.4 += 1, + } + + filter.matches(status).then_some(ix) + }) + .collect(); + + (visible_prs, counts) + }; + + let filter_changed = self.synced_filter != filter; + + if !filter_changed && self.visible_prs == visible_prs && self.counts == counts { + return; + } + + self.synced_filter = filter; + self.item_sizes = Rc::new(vec![size(px(0.), px(ROW_HEIGHT)); visible_prs.len()]); + self.visible_prs = visible_prs; + self.counts = counts; + + cx.notify(); + } + /// Open the detail panel of `pr_id` in the dock area. fn open_pull_request_detail( &mut self, @@ -220,7 +279,7 @@ impl PullRequestsView { .selected(self.filter == PullRequestFilter::All) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = PullRequestFilter::All; - cx.notify(); + this.rebuild(cx); })), ) .child( @@ -230,7 +289,7 @@ impl PullRequestsView { .selected(self.filter == PullRequestFilter::Open) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = PullRequestFilter::Open; - cx.notify(); + this.rebuild(cx); })), ) .child( @@ -240,7 +299,7 @@ impl PullRequestsView { .selected(self.filter == PullRequestFilter::Closed) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = PullRequestFilter::Closed; - cx.notify(); + this.rebuild(cx); })), ) .child( @@ -250,7 +309,7 @@ impl PullRequestsView { .selected(self.filter == PullRequestFilter::Draft) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = PullRequestFilter::Draft; - cx.notify(); + this.rebuild(cx); })), ) .child( @@ -260,7 +319,7 @@ impl PullRequestsView { .selected(self.filter == PullRequestFilter::Merged) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = PullRequestFilter::Merged; - cx.notify(); + this.rebuild(cx); })), ), ) @@ -333,49 +392,7 @@ impl Focusable for PullRequestsView { impl Render for PullRequestsView { fn render(&mut self, _window: &mut Window, cx: &mut Context) -> impl IntoElement { let filter = self.filter; - - // Rows and counts are rebuilt only when the store refreshed or filter changed. - let version = self.store.read(cx).version(); - - if self.cache_key != Some((version, filter)) { - let store = self.store.read(cx); - let mut counts = (0usize, 0usize, 0usize, 0usize, 0usize); - self.visible_prs = store - .pull_requests - .iter() - .enumerate() - .filter_map(|(ix, pr)| { - if pr.kind != Kind::GitPullRequest { - return None; - } - - let status = store.status_of(pr); - counts.0 += 1; - - match status { - RepoStatus::Open => counts.1 += 1, - RepoStatus::Closed => counts.2 += 1, - RepoStatus::Draft => counts.3 += 1, - RepoStatus::Applied => counts.4 += 1, - } - - filter.matches(status).then_some(ix) - }) - .collect(); - - self.counts = counts; - self.cache_key = Some((version, filter)); - } - let count = self.visible_prs.len(); - - // The virtual list's item count comes from `item_sizes`. - // Rebuild it whenever the filtered pull request count changes. - if count != self.pr_len { - self.pr_len = count; - self.item_sizes = Rc::new(vec![size(px(0.), px(ROW_HEIGHT)); count]); - } - let sizes = self.item_sizes.clone(); let scroll_handle = self.scroll_handle.clone(); let view = cx.entity().clone(); diff --git a/docs/repo-state-plan.md b/docs/repo-state-plan.md new file mode 100644 index 0000000..bd4deee --- /dev/null +++ b/docs/repo-state-plan.md @@ -0,0 +1,330 @@ +# Repository state and panel flow plan + +Status: proposed (2026-09-13) + +Builds on `docs/backend-rearchitecture.md`, especially §7 (notify audit), +§11 (split independently-observed state), §12 (one debounce at the source) +and §13 (merge the repo listing files, not the entities). + +Scope: `signed_state::{repo, repos}`, `workspace::views::{repo, issues, +pull_requests, inbox}`. + +## Goal + +1. Open an issue or PR directly, from any surface (inbox entry, notification, + future deep link), without walking Explore -> repository panel first. +2. One per-repository entity that spans both identities: the local git + repository and the NIP-34 announcement, instead of today's separate + `LocalReposStore` entry / `Option` + `Option>` + view state. + +## What the code does today (verified against the current tree) + +### 1. Item panels are cached and never observe their store + +The dock renders the active panel through `panel.cached(...)` +(`crates/dock/src/tab_panel.rs:777-795`), so a panel re-renders only when its +own entity notifies. Cross-entity reads are not reactive. + +`RepoDetailView` observes its store (`views/repo/store.rs:35-43`), but the +panels that read the same store do not: + +- `IssueDetailView` (`views/issues/detail.rs:79-85`) renders + `placeholder("Issue not found", cx)` when the issue is absent, with no + observer. When a directly opened store's first pass lands later and notifies, + nothing re-renders the panel: it stays on "Issue not found". +- `PullRequestDetailView::load` runs once from `cx.defer_in` + (`views/pull_requests/detail.rs:88-91`) and caches + `error = "Pull request not found"` when the store is empty + (`:124-133`). It can never recover. +- `IssuesView` / `PullRequestsView` memoize rows behind + `(store.version(), filter)` (`views/issues/mod.rs:324-351`, + `views/pull_requests/mod.rs:334-369`) but nothing re-renders them when the + version changes, so an open list does not see new events either. +- `NewPullRequestView` reads `store.head` / the announcement in `new`, so a + late first pass does not reach its defaults. + +The normal flow hides this because the repository panel is opened first: by +the time the issues list or an item panel is created, the store has already +applied its first pass. Opening an item directly is the case where the store +is still empty at construction. + +### 2. The initial pass waits on a timer the backend already provides + +`RepoStore::new` defers `refresh` (`signed_state/src/repo.rs:130-141`), and +`refresh` always waits `REFRESH_DEBOUNCE` = 300 ms (`repo.rs:25-26`, +`:230-242`) before the local database query. `RepoListStore` has the same +timer (`repos.rs:120-123`, `:270-281`) plus a separate `refresh_initial` +that skips it (`:257-268`). + +Both stores only refresh on `Backend` events: + +- `NostrUpdate` is already batched by the notification pump with its own + `PUMP_DEBOUNCE` = 200 ms (`backend.rs:37`, `:112-166`). Per + `backend-rearchitecture.md` §12, the per-store timers were to be dropped + once the pump absorbed the bursts. +- `Published` and `Synced` are one-off events; `RefreshGate` already folds + them into an in-flight run. + +The timers are therefore pure added latency for these two stores: ~300 ms +(batched updates) to ~500 ms (pump window + store window) before local data +appears. + +### 3. Opening an item requires a hydrated `Announcement` + +`repo_store(announcement)` (`views/repo/actions.rs:216-219`) and the inbox's +lookup in `RepoListStore` (`views/inbox.rs:384-404`) need the announcement in +hand and silently no-op when it is missing (`RepoListStore` not synced yet, +deleted repo, inbox section without a matching list entry). The store itself +can load the announcement from the local database; `run_refresh` already +queries `filters::announcement(&addr)` (`repo.rs:251-261`). + +### 4. Local and announced repositories have no shared identity + +- `LocalReposStore` holds scan paths; `RepoListStore` holds announcements; + `CheckoutsStore` joins them (`checkouts.rs:211-213`, `:569-609`). +- `RepoDetailView` encodes both modes in three option fields: + `initial: Option`, `store: Option>`, + `local_path: Option` (`views/repo/mod.rs:74-85`), with + `apply_announcement` moving between them (`views/repo/store.rs:13-31`). + The invalid combinations and the `initial` fallback in `announcement()` + (`views/repo/mod.rs:322-331`) are the cost of the missing per-repo entity. + +## Design + +### 1. Panels observe, derive into local state, notify on change + +Every panel that reads a store keeps a local snapshot of exactly the slice it +renders, updates it in an observer, and calls `cx.notify()` only when the +slice changed. This is the pattern `RepoDetailView::refresh_statuses` +(`views/repo/store.rs:88-101`) and `SidebarPanel::refresh` already use; the +item panels are missing the observer half. + +| View | Observed entity | Local snapshot | +|---|---|---| +| `IssueDetailView` | `Entity` | root issue, status, comments | +| `PullRequestDetailView` | `Entity` | root PR, description, tip, base, clone urls | +| `IssuesView` | `Entity` | `visible_issues`, `counts`, `item_sizes`, `cache_key` | +| `PullRequestsView` | `Entity` | `visible_prs`, `counts`, `item_sizes`, `cache_key` | +| `NewPullRequestView` | `Entity` | announced head, default base | + +Sketch, matching the existing idiom: + +```rust +// new() +let subscription = cx.observe(&store, |this, store, cx| this.sync(store, cx)); + +// Copy the slice; notify only when it changed. +fn sync(&mut self, store: &Entity, cx: &mut Context) { + let store = store.read(cx); + let issue = store.issues.iter().find(|issue| issue.id == self.issue_id).cloned(); + let comments: Vec = store.comments_of(&self.issue_id).cloned().collect(); + let status = issue.as_ref().map(|issue| store.status_of(issue)); + + if self.issue != issue || self.comments != comments || self.status != status { + self.issue = issue; + self.comments = comments; + self.status = status; + cx.notify(); + } +} +``` + +`PullRequestDetailView` needs its one-shot `load` split in two: + +- `bind` finds the root PR in the store and snapshots description, tip, base + and clone urls. Re-runnable on every store version change. +- `load_diff` does the async patch/git work. Runs once bound, and again when + the bound tip changes (a PR update arriving late). + +### 2. One debounce, at the backend pump + +Delete the `REFRESH_DEBOUNCE` timers from `RepoStore` and `RepoListStore`. +Their triggers all come from `Backend`; the pump batches relay traffic and +`RefreshGate` folds one-off events into an in-flight run. Keep `RefreshGate` +exactly as is, used without a timer: + +```rust +pub fn refresh(&mut self, cx: &mut Context) { + if self.refresh.request() != RefreshRequest::Schedule { + return; + } + self.run_refresh(cx); +} +``` + +`RepoListStore::refresh_initial` collapses into `refresh`; the +`new`-time `cx.defer` call becomes the initial load, with no timer. + +Add `pub loaded: bool` to `RepoStore`, set when the first pass applies. It +separates "no data yet" from "genuinely empty": panels render a loading state +while `!loaded`, and "not found" only after `loaded`. + +`CheckoutsStore` and `InboxView` keep their timers in this plan. Their +refresh request sources are not only the backend pump (checkout requests, +settings and scan observations), so the same argument does not hold +unchanged; revisit separately if measurements show the timers redundant. + +### 3. `RepoStore` is the one per-repository entity + +Keep the name `RepoStore` (`Repository` collides with `gix::Repository`, +already imported in `views/repo/loading.rs`). Shape: + +```rust +pub struct RepoStore { + /// NIP-34 address. `None` while the repository is local-only. + addr: Option, + /// Latest announcement. Seeded from the open-time hint, replaced by the + /// database's latest on the first pass. `None` while local-only. + pub announcement: Option, + /// Local working copy: the scan path for a local repo, an associated + /// checkout for an announced one. A snapshot; `CheckoutsStore` stays the + /// authority for the full list of checkouts. + pub path: Option, + /// The first local pass has been applied. + pub loaded: bool, + // issues, patches, pull_requests, comments, status_by_root, head, flags... + _subscription: Option, +} +``` + +`addr` is required in addition to `announcement`: `(announcement: None, +path: Some(_))` is otherwise ambiguous between "local-only" and "announced, +first pass pending", and the store needs the address to run its query. + +Constructors and the state transition: + +```rust +impl RepoStore { + /// Announced repository. Resolves `path` from `CheckoutsStore` if the + /// user already has a checkout. + pub fn new(addr: RepoAddr, hint: Option, cx: &mut Context) -> Self; + + /// Local repository discovered by the scan, not announced yet. + pub fn new_local(path: PathBuf, cx: &mut Context) -> Self; + + /// Local -> NIP-34 in place. Keeps `path`, so the panel keeps its worktree. + pub fn announce(&mut self, announcement: Announcement, cx: &mut Context); + + pub fn addr(&self) -> Option<&RepoAddr>; +} +``` + +- `new`: subscribes to `Backend`, seeds `announcement` from the hint, defers + remote subscribe/connect, runs the local pass immediately. +- `new_local`: no `Backend` subscription, `loaded = true`, path set. +- `announce`: sets `addr`, `announcement`, keeps `path`; installs the + `Backend` subscription, connects the announced relays and refreshes. Called + from `apply_announcement`, which loses its field surgery. +- Nostr-side actions (`push_repository`, `clone_to_folder`, + `open_issue`/`open_pull_request`, status changes) already have + `action_error("Repository announcement is not loaded yet")` + (`repo.rs:1368-1378`); they now also handle `addr == None` the same way. +- `announce` should also `CheckoutsStore::record(path, addr)` for the scan + path, so the association exists immediately instead of waiting for the + origin/EUC match in `resolve_associations`. Optional, verify behavior. + +`RepoDetailView` then holds `store: Entity` plus explorer state +only. `initial` and `local_path` are deleted; `announcement()` reads the +store; local-mode checks become `store.read(cx).addr().is_none()`; +`load_repo` opens `path` when not announced, and keeps today's cache-mirror +flow for announced repositories. + +### 4. Opening a repository or item needs only a `RepoAddr` + +- `repo_store(addr: &RepoAddr, hint: Option<&Announcement>, cx) -> + Entity`. +- `open_repo_item(dock_area, addr: &RepoAddr, item, window, cx)`. +- Inbox passes its already-parsed `address` (`signed_core::InboxItem.address`, + the root event's `a` tag) and drops the `RepoListStore` lookup. +- `open_upstream` drops the 60 x 250 ms wait loop: the store's + `repo_filters` already include the announcement filter and + `subscribe_remote` runs on creation, so the panel opens immediately and + fills in. +- `RepoItem::Patch` behavior is unchanged. + +## Phases + +### Phase 1 - make item panels react to the store (fixes the reported flow) + +Status: implemented, except step 6. + +1. `signed_state/src/repo.rs`: `REFRESH_DEBOUNCE` and the timer spawn are + gone; `refresh` runs immediately. `loaded` was added and is set in the + apply closure. A first pass notifies even when it found nothing, so views + leave their loading state and show the empty result. +2. `signed_state/src/repos.rs`: same timer removal; `refresh_initial` folded + into `refresh`. +3. `views/issues/detail.rs`: observes the store and re-renders; loading + placeholder while `!store.loaded()`. +4. `views/pull_requests/detail.rs`: observes the store; `sync`/`sync_missing` + bind the root and `load_diff` does the async work, keyed to a `PrBinding` + and guarded by a generation so late results are discarded. Loading vs not + found is decided by `store.loaded()`. +5. `views/issues/mod.rs`, `views/pull_requests/mod.rs`: observe the store; + `rebuild` recomputes rows/counts/item sizes and `sync` notifies on change. + Filter buttons call `rebuild` before notifying. +6. `views/pull_requests/new.rs`: not done. Its store-derived inputs are + defaults for the compare/base selectors; re-applying them on a late store + pass would clobber a selection the user already made. Left for a follow-up + once the defaults can be derived without resetting the selectors. + +### Phase 2 - entry points by identity + +1. `views/repo/actions.rs`: `repo_store(addr, hint, cx)`; + `open_repo_item(addr, ...)`; `open_upstream` opens directly. +2. `views/inbox.rs`: pass `address`, delete the announcement lookup and its + silent early-return. + +### Phase 3 - one entity for local and NIP-34 + +1. `signed_state/src/repo.rs`: `addr`/`path` options, `new_local`, + `announce`, `Option`, action guards. +2. `views/repo/mod.rs`: single `store` field; `new`/`new_local`; header, + display name, `load_repo`, `open_init_dialog` derive from the store. +3. `views/repo/store.rs`: always observe; `refresh_statuses` returns false + when not announced. +4. `views/repo/actions.rs`, `header.rs`, `banners.rs`: drop + `Option>` guards, guard on `addr()` instead. +5. `LocalReposStore` stays as the scan index; `CheckoutsStore` stays the + association authority. + +### Phase 4 - deferred, only if duplicate stores become a problem + +One store per address via `HashMap>` inside +`RepoListStore`, so an item panel opened while the repository panel is open +shares the same store and its subscriptions. Not needed for correctness once +Phase 1 lands; each open then loads from the local database immediately. + +## Non-goals + +- No per-store debounce; the pump is the one debounce (§12). `RepoStore` and + `RepoListStore` must not grow timers again. +- No new global store, and no merging `LocalReposStore`, `RepoListStore` + and `CheckoutsStore` into one entity (§13). +- Views never query the database directly; `RepoStore` stays the single + projection so status/comment derivation is not duplicated. +- Explorer state (tree, refs, commits, scroll) stays in `RepoDetailView`. +- No `Repository` rename. + +## Validation + +- `cargo check -p signed_state -p workspace`, then clippy. +- Manual scenarios: + 1. Fresh database, never open Explore: click an issue in the inbox. Panel + shows a loading state, then the issue with comments and status. + 2. Same for a PR, including the patch diff loading once the root binds. + 3. Open the issues list, then receive a new issue (second client or + publish); the row appears without reopening. + 4. Local repo: open from the sidebar, Init, panel keeps the worktree and + gains the nostr header; reopening from Explore shows the same data. + 5. Existing flows: Explore list, ready-to-contribute and ready-to-push + banners, new issue/PR dialogs. + +## Open questions + +1. Should an announced repository with an associated checkout browse that + checkout instead of the cache mirror? Today the panel always mirrors. + `RepoStore.path` makes this a one-line decision later. +2. Store sharing (Phase 4): worth it only if duplicate subscriptions show up + in practice.