diff --git a/crates/signed_core/src/lib.rs b/crates/signed_core/src/lib.rs index c73a298..b909b86 100644 --- a/crates/signed_core/src/lib.rs +++ b/crates/signed_core/src/lib.rs @@ -11,6 +11,9 @@ pub use addr::{RepoAddr, identifier_from_name, repo_addr}; pub use annotations::{COVER_NOTE_KIND, cover_note, labels_and_subject, subject_override}; pub use clone_url::{CloneTarget, parse_clone_url}; pub use deletions::Deletions; -pub use model::{Announcement, activity_subject, pull_request_patch, pull_request_patches}; +pub use model::{ + Announcement, activity_subject, branch_name_of, clone_urls_of, current_commit_of, + fork_candidates, latest_update, merge_base_of, pull_request_patch, pull_request_patches, +}; pub use state::{build_state, parse_state}; pub use status::{RepoStatus, references_root, resolve_status}; diff --git a/crates/signed_core/src/model.rs b/crates/signed_core/src/model.rs index 8fc87c2..e600c52 100644 --- a/crates/signed_core/src/model.rs +++ b/crates/signed_core/src/model.rs @@ -180,7 +180,7 @@ fn forward_series<'a>(root: &'a Event, patches: &[&'a Event]) -> Vec<&'a Event> } /// The `c` tag of an event, the tip of the proposed branch, as hex. -fn current_commit_of(event: &Event) -> Option { +pub fn current_commit_of(event: &Event) -> Option { event .tags .iter() @@ -190,6 +190,82 @@ fn current_commit_of(event: &Event) -> Option { }) } +/// The `merge-base` tag of an event, the base commit a pull request diffs against. +pub fn merge_base_of(event: &Event) -> Option { + event + .tags + .iter() + .find_map(|tag| match Nip34Tag::parse(tag.as_slice()) { + Ok(Nip34Tag::MergeBase(commit)) => Some(commit.to_string()), + _ => None, + }) +} + +/// The `clone` tag of an event, URLs the tip commit can be fetched from. +pub fn clone_urls_of(event: &Event) -> Option> { + event + .tags + .iter() + .find_map(|tag| match Nip34Tag::parse(tag.as_slice()) { + Ok(Nip34Tag::Clone(urls)) => Some(urls), + _ => None, + }) +} + +/// The `branch-name` tag of an event, the proposed branch's name. +pub fn branch_name_of(event: &Event) -> Option { + event + .tags + .iter() + .find_map(|tag| match Nip34Tag::parse(tag.as_slice()) { + Ok(Nip34Tag::BranchName(name)) => Some(name), + _ => None, + }) +} + +/// The newest `GitPullRequestUpdate` revising `root`, from the root's own author. +/// +/// A pull request's tip is only mutable by its author, per NIP-34; updates +/// from anyone else are ignored even if they are newer. +pub fn latest_update<'a>( + events: impl Iterator, + root: &Event, +) -> Option<&'a Event> { + let root_hex = root.id.to_hex(); + events + .filter(|e| e.kind == Kind::GitPullRequestUpdate) + .filter(|e| e.pubkey == root.pubkey) + .filter(|e| { + e.tags + .iter() + .any(|t| t.kind() == "E" && t.content() == Some(root_hex.as_str())) + }) + .max_by_key(|e| e.created_at) +} + +/// The announced forks of `base` a new pull request compare can be built from. +/// +/// The user's own forks are listed first. +pub fn fork_candidates<'a>( + announcements: &'a [Announcement], + base: &RepoAddr, + base_euc: Option<&str>, + user: Option, +) -> Vec<&'a Announcement> { + let (mut own, mut others) = (Vec::new(), Vec::new()); + for announcement in announcements { + if announcement.clone.is_empty() || !announcement.is_fork_of(base, base_euc) { + continue; + } + if Some(announcement.owner) == user { + own.push(announcement); + } else { + others.push(announcement); + } + } + own.into_iter().chain(others).collect() +} + /// Whether `patch` produces `commit`, found via its `commit` or `r` tag. /// /// It lets clients find existing patches for a specific commit. @@ -727,4 +803,221 @@ mod tests { vec!["patch-one", "patch-two"] ); } + + const COMMIT_HEX: &str = "1111111111111111111111111111111111111111"; + const OTHER_ROOT_HEX: &str = "2222222222222222222222222222222222222222"; + + /// Build a signed event of `kind` with the given tags and `created_at`. + fn signed_at(kind: Kind, tags: Vec, created_at: u64) -> Event { + EventBuilder::new(kind, "") + .tags(tags) + .custom_created_at(Timestamp::from(created_at)) + .finalize(&keys()) + .expect("signed event") + } + + fn pr_root() -> Event { + signed_at( + Kind::GitPullRequest, + vec![ + Tag::parse(["c", COMMIT_HEX]).expect("valid tag"), + Tag::parse(["branch-name", "feature/x"]).expect("valid tag"), + ], + 100, + ) + } + + #[test] + fn reads_current_commit_and_branch_name() { + let pr = pr_root(); + assert_eq!(current_commit_of(&pr).as_deref(), Some(COMMIT_HEX)); + assert_eq!(branch_name_of(&pr).as_deref(), Some("feature/x")); + } + + #[test] + fn returns_none_without_pr_tags() { + let pr = signed_at(Kind::GitPullRequest, vec![], 100); + assert_eq!(current_commit_of(&pr), None); + assert_eq!(branch_name_of(&pr), None); + } + + #[test] + fn latest_update_picks_newest_revision_of_the_root() { + let root = pr_root(); + let root_hex = root.id.to_hex(); + + let revision = |created_at: u64| { + signed_at( + Kind::GitPullRequestUpdate, + vec![Tag::parse(["E", &root_hex]).expect("valid tag")], + created_at, + ) + }; + // An update revising a different PR must be ignored even though it is newer. + let unrelated = signed_at( + Kind::GitPullRequestUpdate, + vec![Tag::parse(["E", OTHER_ROOT_HEX]).expect("valid tag")], + 999, + ); + + let events = [unrelated, revision(200), root.clone(), revision(300)]; + let latest = latest_update(events.iter(), &root).expect("an update"); + + assert_eq!(latest.created_at.as_secs(), 300); + assert_eq!(latest.kind, Kind::GitPullRequestUpdate); + } + + #[test] + fn latest_update_ignores_other_authors() { + let root = pr_root(); + let root_hex = root.id.to_hex(); + let other = Keys::new( + SecretKey::from_hex("0000000000000000000000000000000000000000000000000000000000000002") + .expect("valid secret key"), + ); + let stranger = EventBuilder::new(Kind::GitPullRequestUpdate, "") + .tags([Tag::parse(["E", &root_hex]).expect("valid tag")]) + .custom_created_at(Timestamp::from(999)) + .finalize(&other) + .expect("signed event"); + + // The tip of a PR is only mutable by its author. + // A newer update from anyone else must not win. + assert!(latest_update([&stranger, &root].into_iter(), &root).is_none()); + } + + #[test] + fn latest_update_ignores_roots_without_revisions() { + let root = pr_root(); + assert!(latest_update([&root].into_iter(), &root).is_none()); + } + + const OWNER_KEYS: [&str; 3] = [ + "0000000000000000000000000000000000000000000000000000000000000001", + "0000000000000000000000000000000000000000000000000000000000000002", + "0000000000000000000000000000000000000000000000000000000000000003", + ]; + + /// Build a signed kind-30617 event for `owner` with the given tags. + fn owned_announcement_event(owner: &str, tags: &[&[&str]]) -> Event { + let keys = Keys::new(SecretKey::from_hex(owner).expect("valid secret key")); + let tags: Vec = tags + .iter() + .map(|t| Tag::parse(t.to_vec()).expect("valid tag")) + .collect(); + EventBuilder::new(Kind::GitRepoAnnouncement, "") + .tags(tags) + .finalize(&keys) + .expect("signed event") + } + + fn owned_announcements(owner_ix: usize, tags: &[&[&str]]) -> Vec { + vec![ + Announcement::from_event(&owned_announcement_event(OWNER_KEYS[owner_ix], tags)) + .expect("parses"), + ] + } + + #[test] + fn fork_candidates_orders_own_forks_first() { + let euc = "aa231c4c6a5777dc89b42207b499891a344add5c"; + let clone = "https://grasp.example/npub1x/my-fork.git"; + + let base_addr = crate::repo_addr( + PublicKey::from_hex(OWNER_KEYS[0]).expect("pubkey"), + "upstream", + ); + // Newest first, as RepoListStore keeps them. + // Unrelated repo, the user's fork with the shared EUC, another fork with a `u` tag. + let all = vec![ + owned_announcements( + 2, + &[ + &["d", "other-project"], + &["r", "bb231c4c6a5777dc89b42207b499891a344add5c", "euc"], + ], + ) + .pop() + .unwrap(), + owned_announcements( + 1, + &[&["d", "my-fork"], &["r", euc, "euc"], &["clone", clone]], + ) + .pop() + .unwrap(), + owned_announcements( + 2, + &[ + &["d", "their-fork"], + &["u", &base_addr.to_string()], + &["clone", clone], + ], + ) + .pop() + .unwrap(), + ]; + + let user = PublicKey::from_hex(OWNER_KEYS[1]).expect("pubkey"); + let forks = fork_candidates(&all, &base_addr, Some(euc), Some(user)); + + // The user's fork comes first, then the other author's. + let ids: Vec<&str> = forks.iter().map(|a| a.id.as_str()).collect(); + assert_eq!(ids, vec!["my-fork", "their-fork"]); + } + + #[test] + fn fork_candidates_excludes_base_unrelated_and_unfetchable() { + let euc = "aa231c4c6a5777dc89b42207b499891a344add5c"; + let base_owner = PublicKey::from_hex(OWNER_KEYS[0]).expect("pubkey"); + let base_addr = crate::repo_addr(base_owner, "upstream"); + + let mut all = vec![ + owned_announcements(0, &[&["d", "upstream"], &["r", euc, "euc"]]) + .pop() + .unwrap(), + owned_announcements(1, &[&["d", "no-clone-fork"], &["r", euc, "euc"]]) + .pop() + .unwrap(), + owned_announcements( + 2, + &[ + &["d", "other"], + &["r", "cc231c4c6a5777dc89b42207b499891a344add5c", "euc"], + ], + ) + .pop() + .unwrap(), + owned_announcements( + 2, + &[ + &["d", "mirror"], + &["r", euc, "euc"], + &["clone", "https://grasp.example/x/mirror.git"], + ], + ) + .pop() + .unwrap(), + ]; + + let forks = fork_candidates(&all, &base_addr, Some(euc), Some(base_owner)); + assert_eq!(forks.len(), 1); + assert_eq!(forks[0].id, "mirror"); + + // Without a base EUC only `u`-tag forks match. + all.push( + owned_announcements( + 2, + &[ + &["d", "u-fork"], + &["u", &base_addr.to_string()], + &["clone", "https://grasp.example/x/u-fork.git"], + ], + ) + .pop() + .unwrap(), + ); + let forks = fork_candidates(&all, &base_addr, None, Some(base_owner)); + let ids: Vec<&str> = forks.iter().map(|a| a.id.as_str()).collect(); + assert_eq!(ids, vec!["u-fork"]); + } } diff --git a/crates/signed_git/src/lib.rs b/crates/signed_git/src/lib.rs index 298ce69..573ef66 100644 --- a/crates/signed_git/src/lib.rs +++ b/crates/signed_git/src/lib.rs @@ -7,7 +7,7 @@ use anyhow::{Context, Result, bail}; use gix::diff::blob::unified_diff::{ConsumeHunk, DiffLineKind as GixLineKind, HunkHeader}; use gix::interrupt::IS_INTERRUPTED; use gix::progress::Discard; -use signed_core::RepoAddr; +use signed_core::{Announcement, RepoAddr}; /// On-disk cache of cloned repositories, keyed by owner pubkey / repo id. #[derive(Debug, Clone)] @@ -1061,6 +1061,15 @@ pub fn sanitize_path_component(id: &str) -> String { sanitized } +/// The refs namespace of a fork's import in the target mirror. +pub fn fork_namespace(announcement: &Announcement) -> String { + format!( + "{}/{}", + announcement.owner.to_hex(), + sanitize_path_component(&announcement.id) + ) +} + /// In-memory object cache for history walks, see [`open_with_cache`]. /// /// Without one, a walk re-decodes the same commit objects from the object database. @@ -2378,6 +2387,21 @@ mod tests { assert_eq!(sanitize_path_component("a/../b"), "a_.._b"); } + #[test] + fn fork_namespace_combines_owner_and_sanitized_id() { + let keys = Keys::generate(); + let event = EventBuilder::new(Kind::GitRepoAnnouncement, "") + .tags([Tag::parse(["d", "my/repo"]).expect("valid tag")]) + .finalize(&keys) + .expect("signed event"); + let announcement = Announcement::from_event(&event).expect("parses"); + + assert_eq!( + fork_namespace(&announcement), + format!("{}/my_repo", keys.public_key().to_hex()) + ); + } + #[test] fn repo_path_stays_inside_root() { let cache = GitCache::new("/cache".into()); diff --git a/crates/signed_state/src/backend.rs b/crates/signed_state/src/backend.rs index c34edcd..24babd5 100644 --- a/crates/signed_state/src/backend.rs +++ b/crates/signed_state/src/backend.rs @@ -1,9 +1,7 @@ use std::collections::{HashMap, HashSet}; -use std::future::Future; use std::path::{Path, PathBuf}; use std::str::FromStr; -use std::sync::{Arc, Mutex}; -use std::time::Duration; +use std::time::{Duration, Instant}; use anyhow::{Error, anyhow, bail}; use bitcoin_hashes::sha1::Hash as Sha1Hash; @@ -33,6 +31,13 @@ pub const BOOTSTRAP_RELAYS: [&str; 4] = [ /// Relays used to index the user's NIP-65 relay list. pub const INDEXER_RELAYS: [&str; 2] = ["wss://indexer.coracle.social", "wss://user.kindpag.es"]; +/// Delay the notification pump waits for more events before emitting a batch. +/// +/// A negentropy sync can deliver hundreds of events in a burst; batching +/// them here means every subscriber debounces the burst once, not once per +/// subscriber. +const PUMP_DEBOUNCE: Duration = Duration::from_millis(200); + #[derive(Debug, Clone)] pub enum BackendEvent { /// User has no signer configured. @@ -41,8 +46,13 @@ pub enum BackendEvent { PassphraseRequired, /// The signer changed on login, logout or account switch. SignerChanged, - /// A new event was received from a relay and stored in the database. - NostrUpdate(Update), + /// New events were received from a relay and stored in the database. + /// + /// Batched: [`Backend`]'s notification pump coalesces everything a + /// relay delivers within one debounce window into a single event, + /// instead of emitting per-event and making every subscriber debounce + /// the same burst independently. + NostrUpdate(Vec), /// A negentropy sync completed. Synced, /// A negentropy sync is in flight. @@ -78,29 +88,17 @@ pub struct Backend { /// True when the stored credential is NIP-49 encrypted. passphrase_required: bool, /// Repositories with a push in flight, mirror or checkout based. - pushing_repos: Arc>>, + /// + /// A child entity: views that only care whether one repository is + /// pushing can `cx.observe` it without being invoked on unrelated + /// `Backend` changes (a `sync_progress` tick, a new relay connecting). + pushing_repos: Entity>, } struct GlobalBackend(Entity); impl Global for GlobalBackend {} -/// Removes its repository from the in-flight push set when dropped. -/// -/// A push task cancelled by its panel closing cannot leave the repository locked. -struct PushGuard { - repos: Arc>>, - addr: RepoAddr, -} - -impl Drop for PushGuard { - fn drop(&mut self) { - if let Ok(mut repos) = self.repos.lock() { - repos.remove(&self.addr); - } - } -} - impl EventEmitter for Backend {} impl Backend { @@ -118,16 +116,45 @@ impl Backend { let pump: Task> = cx.spawn(async move |this, cx| { let mut notifications = pump_client.notifications(); + let mut pending: Vec = Vec::new(); - while let Some(notification) = notifications.next().await { - let ClientNotification::Event { event, .. } = notification else { - continue; - }; + 'outer: loop { + // Wait for the first event of a batch. + match notifications.next().await { + Some(ClientNotification::Event { event, .. }) => { + pending.push(Update::from_event(&event)); + } + Some(_) => continue, + None => break, + } - let update = Update::from_event(&event); + // Collect everything else that arrives within the debounce window. + let deadline = Instant::now() + PUMP_DEBOUNCE; + loop { + let now = Instant::now(); + if now >= deadline { + break; + } + let timer = cx.background_executor().timer(deadline - now); + futures::pin_mut!(timer); + let next = notifications.next(); + futures::pin_mut!(next); + match futures::future::select(next, timer).await { + futures::future::Either::Left(( + Some(ClientNotification::Event { event, .. }), + _, + )) => { + pending.push(Update::from_event(&event)); + } + futures::future::Either::Left((Some(_), _)) => continue, + futures::future::Either::Left((None, _)) => break 'outer, + futures::future::Either::Right(_) => break, + } + } + let batch = std::mem::take(&mut pending); if this - .update(cx, |_, cx| cx.emit(BackendEvent::NostrUpdate(update))) + .update(cx, |_, cx| cx.emit(BackendEvent::NostrUpdate(batch))) .is_err() { break; @@ -139,16 +166,21 @@ impl Backend { pump.detach(); - let mut this = Self { + let this = Self { client, signer, current_user: None, sync_progress: None, passphrase_required: false, - pushing_repos: Arc::new(Mutex::new(HashSet::new())), + pushing_repos: cx.new(|_| HashSet::new()), }; - this.bootstrap(cx); + let weak = cx.entity().downgrade(); + cx.defer(move |cx| { + if let Err(error) = weak.update(cx, |this, cx| this.bootstrap(cx)) { + log::warn!("backend dropped before bootstrap could run: {error}"); + } + }); this } @@ -351,24 +383,31 @@ impl Backend { ] .to_vec(); - this.send_fire_and_forget(RelayList::new(relays).into_event_builder(), cx); - let metadata = Metadata::new() .name(&name) .display_name(&name) .into_event_builder(); - this.send_fire_and_forget(metadata, cx); - let grasp_servers: Vec = ["wss://gitnostr.com", "wss://relay.ngit.dev"] .into_iter() .map(|url| RelayUrl::parse(url).expect("valid relay URL")) .collect(); - this.send_fire_and_forget( + let client = this.client.clone(); + let signer = this.signer.clone(); + + for builder in [ + RelayList::new(relays).into_event_builder(), + metadata, GitUserGraspList { grasp_servers }.into_event_builder(), - cx, - ); + ] { + let client = client.clone(); + let signer = signer.clone(); + cx.spawn(async move |_this, _cx| { + publish_best_effort(&client, &signer, builder).await + }) + .detach(); + } })?; Ok(public_key) @@ -486,18 +525,21 @@ impl Backend { maintainers: Vec::new(), }; - let event = this - .update(cx, |this, cx| { - this.send(announcement.into_event_builder(), cx) - })? - .await?; + let signer = this.update(cx, |this, _cx| this.signer.clone())?; + + let event = { + let builder = announcement.into_event_builder(); + let event = builder.finalize_async(&signer).await?; + let output = client.send_event(&event).broadcast().await?; + let event = require_relay_accepted(output, event)?; + this.update(cx, |this, cx| this.announce_published(event.clone(), cx))?; + event + }; // The state event is the push authorization. Stage it on each // grasp server's relay, then push the initial commit. // Creation fails only when no server accepted the push, the announcement // is then retracted so the repository is not left announced without content. - let (client, signer) = - this.update(cx, |this, _cx| (this.client.clone(), this.signer.clone()))?; let refs = vec![("refs/heads/main".to_owned(), commit)]; let push = cx.background_spawn({ @@ -545,9 +587,11 @@ impl Backend { // Staging already stored the event locally, publishing makes it // visible to the other relays and clients. if let Some(state_event) = &outcome.state_event { - broadcast_event(&client, state_event).await.ok(); - this.update(cx, |_this, cx| { - cx.emit(BackendEvent::Published(Box::new(state_event.clone()))); + if let Err(e) = client.send_event(state_event).broadcast().await { + log::warn!("failed to broadcast repository state: {e}"); + } + this.update(cx, |this, cx| { + this.announce_published(state_event.clone(), cx) }) .ok(); } @@ -633,11 +677,16 @@ impl Backend { maintainers: Vec::new(), }; - let event = this - .update(cx, |this, cx| { - this.send(announcement.into_event_builder(), cx) - })? - .await?; + let signer = this.update(cx, |this, _cx| this.signer.clone())?; + + let event = { + let builder = announcement.into_event_builder(); + let event = builder.finalize_async(&signer).await?; + let output = client.send_event(&event).broadcast().await?; + let event = require_relay_accepted(output, event)?; + this.update(cx, |this, cx| this.announce_published(event.clone(), cx))?; + event + }; let refs = state.refs.clone(); let head = state.head.clone(); @@ -647,9 +696,6 @@ impl Backend { // fails only when no server accepted it. The announcement is then // retracted so the repository is not left announced without content. // An empty repository has no state to stage and nothing to push. - let (client, signer) = - this.update(cx, |this, _cx| (this.client.clone(), this.signer.clone()))?; - if !refs.is_empty() { let push = cx.background_spawn({ let client = client.clone(); @@ -695,11 +741,11 @@ impl Backend { // Fan the state out to the relays once a git server holds the objects. // Staging already stored the event locally, publishing makes it visible to the other relays and clients. if let Some(state_event) = &outcome.state_event { - broadcast_event(&client, state_event).await.ok(); - this.update(cx, |_this, cx| { - cx.emit(BackendEvent::Published(Box::new(state_event.clone()))); - }) - .ok(); + if let Err(e) = client.send_event(state_event).broadcast().await { + log::warn!("failed to broadcast repository state: {e}"); + } + this.update(cx, |this, cx| this.announce_published(state_event.clone(), cx)) + .ok(); } } @@ -754,31 +800,34 @@ impl Backend { cx: &mut Context, ) -> Task> { let addr = announcement.addr(); - let guard = { - let mut pushing = self - .pushing_repos - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); - if !pushing.insert(addr.clone()) { - return Task::ready(Err(anyhow!( - "A push to this repository is already in progress" - ))); - } + if self.pushing_repos.read(cx).contains(&addr) { + return Task::ready(Err(anyhow!( + "A push to this repository is already in progress" + ))); + } - PushGuard { - repos: self.pushing_repos.clone(), - addr: addr.clone(), - } - }; + self.pushing_repos.update(cx, |pushing, cx| { + pushing.insert(addr.clone()); + cx.notify(); + }); let owner = announcement.owner.to_bech32().unwrap(); let repo_id = announcement.id.clone(); let relays = announcement.relays.clone(); cx.spawn(async move |this, cx| { - // Held for the whole task. Dropped on completion, on error and on cancellation alike. - let _guard = guard; + // Held for the whole task. Runs on completion, on error and on + // cancellation alike, since dropping the task drops this guard. + let _guard = cx.on_drop(&this, { + let addr = addr.clone(); + move |backend, cx| { + backend.pushing_repos.update(cx, |pushing, cx| { + pushing.remove(&addr); + cx.notify(); + }); + } + }); let mut state = { let work = cx.background_spawn({ @@ -854,9 +903,11 @@ impl Backend { // Staging already stored the event locally, publishing notifies // the repository views and other relays and clients. if let Some(state_event) = &outcome.state_event { - broadcast_event(&client, state_event).await.ok(); - this.update(cx, |_this, cx| { - cx.emit(BackendEvent::Published(Box::new(state_event.clone()))); + if let Err(e) = client.send_event(state_event).broadcast().await { + log::warn!("failed to broadcast repository state: {e}"); + } + this.update(cx, |this, cx| { + this.announce_published(state_event.clone(), cx) }) .ok(); } @@ -1063,6 +1114,13 @@ impl Backend { self.signer.clone() } + /// Repositories with a push in flight, mirror or checkout based. + /// + /// A child entity: `cx.observe` it to react only to push-state changes. + pub fn pushing_repos(&self) -> Entity> { + self.pushing_repos.clone() + } + /// Get the current user's public key. pub fn current_user(&self) -> Option { self.current_user @@ -1221,108 +1279,59 @@ impl Backend { task.detach(); } - /// Sign, broadcast and locally store an event. - pub fn send( - &mut self, - builder: EventBuilder, - cx: &mut Context, - ) -> Task> { + /// Emit [`BackendEvent::Published`] for cross-store invalidation. + /// + /// Callers publish with `client.send_event(...)` directly, then call this + /// so stores like `RepoListStore` refresh without re-querying the relays. + pub fn announce_published(&self, event: Event, cx: &mut Context) { + cx.emit(BackendEvent::Published(Box::new(event))); + } + + /// Publish a NIP-09 deletion for each of `events`, best-effort. + /// + /// Each target gets its own deletion event: a relay rejecting or + /// dropping one does not affect the others. + fn retract_events(&mut self, events: &[Event], cx: &mut Context) { let client = self.client.clone(); let signer = self.signer.clone(); - self.publish_task(cx, async move { - // Sign with the current signer, broadcast and save locally. - // The event is immediately visible to database queries. - let event = builder.finalize_async(&signer).await?; - broadcast_event(&client, &event).await - }) - } + for event in events.iter().cloned() { + let client = client.clone(); + let signer = signer.clone(); - /// Broadcast and locally store an already-signed event. - pub fn publish_event( - &mut self, - event: Event, - cx: &mut Context, - ) -> Task> { - let client = self.client.clone(); - self.publish_task(cx, async move { broadcast_event(&client, &event).await }) - } - - /// Run `work` in the background, then emit its outcome as a [`BackendEvent`]. - fn publish_task( - &mut self, - cx: &mut Context, - work: impl Future> + 'static + Send, - ) -> Task> { - cx.spawn(async move |this, cx| { - let result = cx.background_spawn(work).await; - - match &result { - Ok(event) => { - this.update(cx, |_this, cx| { - cx.emit(BackendEvent::Published(Box::new(event.clone()))); - }) - .ok(); + 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); } - Err(e) => { - this.update(cx, |_this, cx| { - cx.emit(BackendEvent::error(e.to_string())); - }) - .ok(); - } - } - - result - }) - } - - /// Sign, broadcast and store an event without awaiting the result. - fn send_fire_and_forget(&mut self, builder: EventBuilder, cx: &mut Context) { - let publish = self.send(builder, cx); - - let task: Task> = cx.spawn(async move |this, cx| { - if let Err(e) = publish.await { - this.update(cx, |_this, cx| { - cx.emit(BackendEvent::error(e.to_string())); - }) - .ok(); - } - Ok(()) - }); - task.detach(); - } - - /// Publish NIP-09 deletions for `events`, best-effort. - fn retract_events(&mut self, events: &[Event], cx: &mut Context) { - if events.is_empty() { - return; + }) + .detach(); } - - 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 publish = self.send(EventBuilder::new(Kind::EventDeletion, "").tags(tags), cx); - - let task: Task> = cx.spawn(async move |_this, _cx| { - if let Err(e) = publish.await { - log::warn!("failed to retract repository events: {e}"); - } - Ok(()) - }); - task.detach(); } } -/// Broadcast an event and fail when no relay accepted it. -/// -/// The client stores accepted events locally, visible to database queries. -async fn broadcast_event(client: &Client, event: &Event) -> Result { - let output = client.send_event(event).await?; +/// Sign and send a single NIP-09 deletion request for `event`. +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).broadcast().await?; + Ok(()) +} +/// The event was accepted by at least one relay, or a descriptive error otherwise. +/// +/// The SDK does not treat "accepted by zero relays" as an error on its own: +/// [`SendEventOutput::success`] may be empty while the call still returns `Ok`. +/// This turns that case into an error the caller can surface. +pub(crate) fn require_relay_accepted( + output: SendEventOutput, + event: Event, +) -> Result { if output.success.is_empty() && !output.failed.is_empty() { let reasons = output .failed @@ -1330,10 +1339,28 @@ async fn broadcast_event(client: &Client, event: &Event) -> Result .cloned() .collect::>() .join(", "); - return Err(anyhow!("event not accepted by any relay: {reasons}")); + bail!("event not accepted by any relay: {reasons}"); } - Ok(event.clone()) + Ok(event) +} + +/// Sign and broadcast `builder`, logging rather than surfacing failures. +/// +/// Used for best-effort identity bootstrap events, where a relay hiccup +/// should not block sign-up. +async fn publish_best_effort(client: &Client, signer: &UniversalSigner, builder: EventBuilder) { + let result: Result<(), Error> = async { + let event = builder.finalize_async(signer).await?; + let output = client.send_event(&event).broadcast().await?; + require_relay_accepted(output, event)?; + Ok(()) + } + .await; + + if let Err(e) = result { + log::warn!("failed to publish identity bootstrap event: {e}"); + } } /// Add the given relays, connect and fetch the filters. diff --git a/crates/signed_state/src/checkouts.rs b/crates/signed_state/src/checkouts.rs index f38f33d..c14950b 100644 --- a/crates/signed_state/src/checkouts.rs +++ b/crates/signed_state/src/checkouts.rs @@ -10,9 +10,8 @@ use signed_core::{Announcement, RepoAddr}; use crate::backend::{Backend, BackendEvent}; use crate::git_store::GitStore; -use crate::local_repos::LocalReposStore; use crate::refresh::{RefreshGate, RefreshRequest}; -use crate::repo_list::RepoListStore; +use crate::repos::{LocalReposStore, RepoListStore}; /// Delay between a refresh request and the actual re-computation. const REFRESH_DEBOUNCE: Duration = Duration::from_millis(300); @@ -154,7 +153,7 @@ impl CheckoutsStore { })); } - let mut store = Self { + let store = Self { by_repo: HashMap::new(), statuses: HashMap::new(), status_requested: HashSet::new(), @@ -168,7 +167,12 @@ impl CheckoutsStore { }; if !cfg!(target_arch = "wasm32") { - store.refresh(cx); + let weak = cx.entity().downgrade(); + cx.defer(move |cx| { + if let Err(error) = weak.update(cx, |this, cx| this.refresh(cx)) { + log::warn!("checkouts store dropped before initial refresh could run: {error}"); + } + }); } store diff --git a/crates/signed_state/src/lib.rs b/crates/signed_state/src/lib.rs index 9d90403..f7b4bcc 100644 --- a/crates/signed_state/src/lib.rs +++ b/crates/signed_state/src/lib.rs @@ -1,11 +1,10 @@ mod backend; mod checkouts; mod git_store; -mod local_repos; mod profile; mod refresh; mod repo; -mod repo_list; +mod repos; use std::path::{Path, PathBuf}; @@ -13,11 +12,10 @@ pub use backend::{Backend, BackendEvent, user_grasp_list_servers}; pub use checkouts::{CheckoutStatus, CheckoutsStore, pr_proposes_checkout}; pub use git_store::GitStore; use gpui::{App, AppContext, Entity}; -pub use local_repos::LocalReposStore; pub use nostr_sdk::prelude::Timestamp; pub use profile::{Profile, ProfileStore}; pub use repo::RepoStore; -pub use repo_list::{RepoActivityCounts, RepoListStore}; +pub use repos::{LocalReposStore, RepoActivityCounts, RepoListStore}; use signed_nostr::new_backend; /// Initialize the backend and stores, and install them as globals. diff --git a/crates/signed_state/src/local_repos.rs b/crates/signed_state/src/local_repos.rs deleted file mode 100644 index 19ace8f..0000000 --- a/crates/signed_state/src/local_repos.rs +++ /dev/null @@ -1,103 +0,0 @@ -use std::path::{Path, PathBuf}; -use std::sync::Arc; - -use anyhow::Error; -use gpui::{App, AppContext, Context, Entity, Global}; -use signed_git::find_git_repos; - -struct GlobalLocalReposStore(Entity); - -impl Global for GlobalLocalReposStore {} - -/// Store of the git repositories discovered under a set of scan paths. -pub struct LocalReposStore { - /// The directories being scanned. - pub roots: Arc>, - /// Git repositories discovered under [`Self::roots`], sorted by path. - pub repos: Arc>, - /// A scan is currently running. - pub scanning: bool, - /// A scan was requested while one was already running. - scan_dirty: bool, -} - -impl LocalReposStore { - /// Retrieve the global local-repositories store. - pub fn global(cx: &App) -> Entity { - cx.global::().0.clone() - } - - pub(crate) fn set_global(entity: Entity, cx: &mut App) { - cx.set_global(GlobalLocalReposStore(entity)); - } - - /// Create a store scanning `roots` right away. - pub fn new(roots: Vec, cx: &mut Context) -> Self { - let mut store = Self { - roots: Arc::new(roots), - repos: Arc::new(Vec::new()), - scanning: false, - scan_dirty: false, - }; - store.rescan(cx); - store - } - - /// Forget a repository that has just been published to NIP-34. - pub fn remove(&mut self, path: &Path, cx: &mut Context) { - self.repos = Arc::new( - self.repos - .iter() - .filter(|repo| repo.as_path() != path) - .cloned() - .collect(), - ); - cx.notify(); - } - - /// Re-run the scan. - pub fn rescan(&mut self, cx: &mut Context) { - if self.scanning { - self.scan_dirty = true; - return; - } - if self.roots.is_empty() { - return; - } - - self.scanning = true; - cx.notify(); - - let roots = self.roots.clone(); - let work = cx.background_spawn(async move { - let mut repos = Vec::new(); - for root in roots.iter() { - repos.extend(find_git_repos(root)); - } - repos.sort(); - repos.dedup(); - repos - }); - - let task: gpui::Task> = cx.spawn(async move |this, cx| { - let repos = work.await; - let again = this.update(cx, |this, cx| { - this.repos = Arc::new(repos); - this.scanning = false; - cx.notify(); - - let dirty = this.scan_dirty; - this.scan_dirty = false; - dirty - })?; - - // Scans requested while this one ran are coalesced into one follow-up scan. - if again { - this.update(cx, |this, cx| this.rescan(cx))?; - } - - Ok(()) - }); - task.detach(); - } -} diff --git a/crates/signed_state/src/profile.rs b/crates/signed_state/src/profile.rs index f1cf203..7e91087 100644 --- a/crates/signed_state/src/profile.rs +++ b/crates/signed_state/src/profile.rs @@ -96,8 +96,13 @@ impl ProfileStore { let backend = Backend::global(cx); let subscription = cx.subscribe(&backend, |this, _backend, event, cx| match event { - BackendEvent::NostrUpdate(update) if update.kind == Kind::Metadata => { - this.apply_author(update.author, cx); + BackendEvent::NostrUpdate(updates) => { + for update in updates + .iter() + .filter(|update| update.kind == Kind::Metadata) + { + this.apply_author(update.author, cx); + } } BackendEvent::Published(event) if event.kind == Kind::Metadata => { let metadata = Metadata::from_json(&event.content).unwrap_or_default(); @@ -118,14 +123,19 @@ impl ProfileStore { }) .detach(); - let mut store = Self { + let store = Self { profiles: HashMap::new(), seen: RefCell::new(HashSet::new()), sender, _subscription: subscription, }; - store.load(cx); + let weak = cx.entity().downgrade(); + cx.defer(move |cx| { + if let Err(error) = weak.update(cx, |this, cx| this.load(cx)) { + log::warn!("profile store dropped before initial load could run: {error}"); + } + }); store } @@ -329,7 +339,7 @@ impl ProfileStore { // Re-apply from the database afterwards. match sync_bootstrap_only(client, filter, SyncOptions::default()).await { Ok(_) => { - let _ = this.update(cx, |this, cx| this.apply_seen(cx)); + this.update(cx, |this, cx| this.apply_seen(cx)).ok(); } Err(e) => log::warn!("profile sync failed: {e}"), } diff --git a/crates/signed_state/src/repo.rs b/crates/signed_state/src/repo.rs index a5c052d..636c0b4 100644 --- a/crates/signed_state/src/repo.rs +++ b/crates/signed_state/src/repo.rs @@ -3,7 +3,7 @@ use std::collections::{HashMap, HashSet}; use std::path::PathBuf; use std::time::Duration; -use anyhow::Error; +use anyhow::{Error, bail}; use bitcoin_hashes::sha1::Hash as Sha1Hash; use gpui::{App, AppContext, AsyncApp, Context, SharedString, Subscription, Task, WeakEntity}; use nostr::event::IntoEventBuilder; @@ -14,12 +14,13 @@ use signed_core::{ }; use crate::backend::{ - Backend, BackendEvent, grasp_base_url, grasp06_prs_url, pr_clone_urls, user_grasp_list_servers, + Backend, BackendEvent, grasp_base_url, grasp06_prs_url, pr_clone_urls, require_relay_accepted, + user_grasp_list_servers, }; use crate::checkouts::CheckoutsStore; use crate::git_store::GitStore; use crate::refresh::{RefreshGate, RefreshRequest}; -use crate::repo_list::RepoListStore; +use crate::repos::RepoListStore; /// Delay between a refresh request and the actual re-query. const REFRESH_DEBOUNCE: Duration = Duration::from_millis(300); @@ -90,7 +91,7 @@ impl RepoStore { let subscription = cx.subscribe(&backend, |this, _backend, event, cx| { let relevant = match event { - BackendEvent::NostrUpdate(update) => { + BackendEvent::NostrUpdate(updates) => updates.iter().any(|update| { // Deletions may target any event of this repository. let deletion = update.kind == Kind::EventDeletion || update.kind == Kind::RequestToVanish; @@ -106,7 +107,7 @@ impl RepoStore { let status = RepoStatus::from_kind(update.kind).is_some(); deletion || coordinate || (author && kind) || comment || status - } + }), BackendEvent::Published(event) => { let kind = event.kind == Kind::GitRepoAnnouncement; let author = event.pubkey == this.addr.public_key; @@ -126,7 +127,7 @@ impl RepoStore { } }); - let mut store = Self { + let store = Self { addr, announcement: None, head: None, @@ -149,12 +150,20 @@ impl RepoStore { _subscription: subscription, }; - store.subscribe_remote(cx); - // The announcement we opened the repo from may already list its relays. - // Connect to them right away. - // Do not wait for the bootstrap fetch to return the same event. - store.connect_announced_relays(&announced_relays, cx); - store.refresh(cx); + let weak = cx.entity().downgrade(); + cx.defer(move |cx| { + let result = weak.update(cx, |this, cx| { + this.subscribe_remote(cx); + // The announcement we opened the repo from may already list its relays. + // Connect to them right away. + // Do not wait for the bootstrap fetch to return the same event. + this.connect_announced_relays(&announced_relays, cx); + this.refresh(cx); + }); + if let Err(error) = result { + log::warn!("repo store dropped before bootstrap could run: {error}"); + } + }); store } @@ -509,7 +518,7 @@ impl RepoStore { } .into_event_builder(); - self.send(builder, cx); + self.publish(builder, cx); } /// Comments on a root event, an issue or PR, oldest first. @@ -540,7 +549,7 @@ impl RepoStore { .and_then(|a| a.relays.first()) .cloned(); - self.send( + self.publish( comment_builder(root, parent, relay_hint.as_ref(), &self.addr, content), cx, ); @@ -793,12 +802,15 @@ impl RepoStore { } } - let publish_task = this.update(cx, |_this, cx| { - let backend = Backend::global(cx); - backend.update(cx, |backend, cx| backend.publish_event(event, cx)) - })?; + let client = this.update(cx, |_this, cx| Backend::global(cx).read(cx).client())?; - let pr_event = match publish_task.await { + let publish_result: Result = async { + let output = client.send_event(&event).broadcast().await?; + require_relay_accepted(output, event) + } + .await; + + let pr_event = match publish_result { Ok(event) => event, Err(e) => { return this.update(cx, |this, cx| { @@ -808,6 +820,11 @@ impl RepoStore { } }; + this.update(cx, |_this, cx| { + Backend::global(cx) + .update(cx, |backend, cx| backend.announce_published(pr_event.clone(), cx)) + })?; + // A draft PR carries a kind-1633 status event, NIP-34. // Publish it right after the PR event so viewers never show it open. if draft { @@ -821,6 +838,59 @@ impl RepoStore { .detach(); } + /// Generate the patch between `merge_base` and `compare_ref` in `repo_path`, + /// then open a pull request from it. + /// + /// Fails descriptively when there are no commits to propose or the patch + /// could not be generated; otherwise publishes exactly like + /// [`Self::open_pull_request`]. + #[allow(clippy::too_many_arguments)] + pub fn open_pull_request_from_refs( + &mut self, + repo_path: PathBuf, + merge_base: String, + compare_ref: String, + subject: Option, + description: String, + branch_name: Option, + draft: bool, + cx: &mut Context, + ) -> Task> { + cx.spawn(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 { + signed_git::format_patch_between(&repo_path, &merge_base, &compare_ref) + } + }) + .await; + + let patch = match patch { + Ok(patch) if !patch.is_empty() => patch, + Ok(_) => bail!("No commits between the branches to propose"), + Err(error) => bail!("Failed to generate the patch: {error}"), + }; + + this.update(cx, |this, cx| { + this.open_pull_request( + subject, + description, + branch_name, + patch, + draft, + Some(merge_base), + Some(repo_path), + cx, + ); + }) + }) + } + /// Update a pull request. /// /// Other authors must open a new PR. @@ -909,7 +979,7 @@ impl RepoStore { }); } - let update_task = this.update(cx, |this, cx| { + let builder = this.update(cx, |this, _cx| { let builder = GitPullRequestUpdate { repository: this.addr.clone(), pull_request_event: root.id, @@ -922,20 +992,39 @@ impl RepoStore { // The `r` EUC tag lets clients subscribe to all PR updates. // The SDK builder omits it. - let builder = match euc.as_deref() { + match euc.as_deref() { Some(euc) => builder.tag(Tag::parse(["r", euc]).expect("valid r tag")), None => builder, - }; - - let backend = Backend::global(cx); - backend.update(cx, |backend, cx| backend.send(builder, cx)) + } })?; - if let Err(e) = update_task.await { - return this.update(cx, |this, cx| { - this.last_error = Some(e.to_string()); - cx.notify(); - }); + let (client, signer) = this.update(cx, |_this, cx| { + let backend = Backend::global(cx); + let backend = backend.read(cx); + (backend.client(), backend.signer()) + })?; + + let publish_result: Result = async { + let event = builder.finalize_async(&signer).await?; + let output = client.send_event(&event).broadcast().await?; + require_relay_accepted(output, event) + } + .await; + + match publish_result { + Ok(event) => { + this.update(cx, |_this, cx| { + Backend::global(cx).update(cx, |backend, cx| { + backend.announce_published(event.clone(), cx) + }) + })?; + } + Err(e) => { + return this.update(cx, |this, cx| { + this.last_error = Some(e.to_string()); + cx.notify(); + }); + } } Ok(()) @@ -979,7 +1068,7 @@ impl RepoStore { Tag::coordinate(self.addr.clone(), None), ]); - self.send(builder, cx); + self.publish(builder, cx); } /// Merge a pull request. @@ -1326,22 +1415,47 @@ impl RepoStore { } } - self.send(EventBuilder::new(Kind::GitStatusApplied, "").tags(tags), cx); + self.publish(EventBuilder::new(Kind::GitStatusApplied, "").tags(tags), cx); } - fn send(&mut self, builder: EventBuilder, cx: &mut Context) { + /// Sign `builder`, broadcast it and track the outcome in [`Self::last_error`]. + /// + /// Every one-shot repository event (issue, comment, status) goes through + /// this. Multi-step flows (opening or updating a pull request, a patch + /// series) call the SDK directly instead, since their error handling and + /// post-conditions differ per step. + fn publish(&mut self, builder: EventBuilder, cx: &mut Context) { self.last_error = None; let backend = Backend::global(cx); - let publish = backend.update(cx, |backend, cx| backend.send(builder, cx)); + let (client, signer) = { + let backend = backend.read(cx); + (backend.client(), backend.signer()) + }; let task: Task> = cx.spawn(async move |this, cx| { - if let Err(e) = publish.await { - this.update(cx, |this, cx| { - this.last_error = Some(e.to_string()); - cx.notify(); - })?; + let publish_result: Result = async { + let event = builder.finalize_async(&signer).await?; + let output = client.send_event(&event).broadcast().await?; + require_relay_accepted(output, event) } + .await; + + match publish_result { + Ok(event) => { + this.update(cx, |_this, cx| { + Backend::global(cx) + .update(cx, |backend, cx| backend.announce_published(event, cx)) + })?; + } + Err(e) => { + this.update(cx, |this, cx| { + this.last_error = Some(e.to_string()); + cx.notify(); + })?; + } + } + Ok(()) }); task.detach(); @@ -1425,6 +1539,12 @@ async fn publish_patch_series( first_marker: &str, reply_to: Option, ) -> Result { + let (client, signer) = this.update(cx, |_this, cx| { + let backend = Backend::global(cx); + let backend = backend.read(cx); + (backend.client(), backend.signer()) + })?; + let mut root: Option = None; let mut previous = reply_to; @@ -1465,11 +1585,14 @@ async fn publish_patch_series( let builder = EventBuilder::new(Kind::GitPatch, part.clone()).tags(tags); - let task = this.update(cx, |_this, cx| { - let backend = Backend::global(cx); - backend.update(cx, |backend, cx| backend.send(builder, cx)) + let event = builder.finalize_async(&signer).await?; + let output = client.send_event(&event).broadcast().await?; + let event = require_relay_accepted(output, event)?; + this.update(cx, |_this, cx| { + Backend::global(cx).update(cx, |backend, cx| { + backend.announce_published(event.clone(), cx) + }) })?; - let event = task.await?; if root.is_none() { root = Some(event.clone()); diff --git a/crates/signed_state/src/repo_list.rs b/crates/signed_state/src/repos.rs similarity index 75% rename from crates/signed_state/src/repo_list.rs rename to crates/signed_state/src/repos.rs index 92ca7cb..47d3625 100644 --- a/crates/signed_state/src/repo_list.rs +++ b/crates/signed_state/src/repos.rs @@ -1,4 +1,5 @@ use std::collections::HashMap; +use std::path::{Path, PathBuf}; use std::sync::Arc; use std::time::Duration; @@ -6,10 +7,114 @@ use anyhow::Error; use gpui::{App, AppContext, Context, Entity, Global, Subscription}; use nostr_sdk::prelude::*; use signed_core::{Announcement, Deletions, RepoAddr, filters, repo_addr}; +use signed_git::find_git_repos; use crate::backend::{Backend, BackendEvent}; use crate::refresh::{RefreshGate, RefreshRequest}; +struct GlobalLocalReposStore(Entity); + +impl Global for GlobalLocalReposStore {} + +/// Store of the git repositories discovered under a set of scan paths. +pub struct LocalReposStore { + /// The directories being scanned. + pub roots: Arc>, + /// Git repositories discovered under [`Self::roots`], sorted by path. + pub repos: Arc>, + /// A scan is currently running. + pub scanning: bool, + /// A scan was requested while one was already running. + scan_dirty: bool, +} + +impl LocalReposStore { + /// Retrieve the global local-repositories store. + pub fn global(cx: &App) -> Entity { + cx.global::().0.clone() + } + + pub(crate) fn set_global(entity: Entity, cx: &mut App) { + cx.set_global(GlobalLocalReposStore(entity)); + } + + /// Create a store scanning `roots` right away. + pub fn new(roots: Vec, cx: &mut Context) -> Self { + let store = Self { + roots: Arc::new(roots), + repos: Arc::new(Vec::new()), + scanning: false, + scan_dirty: false, + }; + + let weak = cx.entity().downgrade(); + cx.defer(move |cx| { + if let Err(error) = weak.update(cx, |this, cx| this.rescan(cx)) { + log::warn!("local repos store dropped before initial scan could run: {error}"); + } + }); + store + } + + /// Forget a repository that has just been published to NIP-34. + pub fn remove(&mut self, path: &Path, cx: &mut Context) { + self.repos = Arc::new( + self.repos + .iter() + .filter(|repo| repo.as_path() != path) + .cloned() + .collect(), + ); + cx.notify(); + } + + /// Re-run the scan. + pub fn rescan(&mut self, cx: &mut Context) { + if self.scanning { + self.scan_dirty = true; + return; + } + if self.roots.is_empty() { + return; + } + + self.scanning = true; + cx.notify(); + + let roots = self.roots.clone(); + let work = cx.background_spawn(async move { + let mut repos = Vec::new(); + for root in roots.iter() { + repos.extend(find_git_repos(root)); + } + repos.sort(); + repos.dedup(); + repos + }); + + let task: gpui::Task> = cx.spawn(async move |this, cx| { + let repos = work.await; + let again = this.update(cx, |this, cx| { + this.repos = Arc::new(repos); + this.scanning = false; + cx.notify(); + + let dirty = this.scan_dirty; + this.scan_dirty = false; + dirty + })?; + + // Scans requested while this one ran are coalesced into one follow-up scan. + if again { + this.update(cx, |this, cx| this.rescan(cx))?; + } + + Ok(()) + }); + task.detach(); + } +} + /// Delay between a refresh request and the actual re-query. /// /// Bursts of events, e.g. sync progress ticks, collapse into one query. @@ -74,7 +179,7 @@ impl RepoListStore { let subscription = cx.subscribe(&backend, |this, _backend, event, cx| { let relevant = match event { - BackendEvent::NostrUpdate(update) => { + BackendEvent::NostrUpdate(updates) => updates.iter().any(|update| { // Deletions may target anything we list, always refresh. if update.kind == Kind::EventDeletion || update.kind == Kind::RequestToVanish { true @@ -87,7 +192,7 @@ impl RepoListStore { let is_repo_state = update.kind == Kind::RepoState; is_announcement || is_repo_state } - } + }), BackendEvent::Published(event) => { let announcement = event.kind == Kind::GitRepoAnnouncement; @@ -107,7 +212,7 @@ impl RepoListStore { } }); - let mut store = Self { + let store = Self { announcements: Arc::new(Vec::new()), last_activity: Arc::new(HashMap::new()), counts: Arc::new(HashMap::new()), @@ -115,10 +220,18 @@ impl RepoListStore { _subscription: subscription, }; - store.subscribe_remote(cx); - // Query the local database right away. - // The list never waits for the relay syncs started above to finish. - store.refresh_initial(cx); + let weak = cx.entity().downgrade(); + cx.defer(move |cx| { + let result = weak.update(cx, |this, cx| { + this.subscribe_remote(cx); + // Query the local database right away. + // The list never waits for the relay syncs started above to finish. + this.refresh_initial(cx); + }); + if let Err(error) = result { + log::warn!("repo list store dropped before bootstrap could run: {error}"); + } + }); store } 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 94e4918..b2b4a07 100644 --- a/crates/workspace/src/views/repo_detail/new_pull_request.rs +++ b/crates/workspace/src/views/repo_detail/new_pull_request.rs @@ -21,10 +21,10 @@ use gpui_component::{ v_virtual_list, }; use nostr::prelude::*; -use signed_core::{Announcement, RepoAddr}; +use signed_core::{Announcement, RepoAddr, fork_candidates}; use signed_git::{ - delete_refs_with_prefix, fetch_repo_refs, format_patch_between, merge_base, refs_with_prefix, - sanitize_path_component, worktree_commit_range_commits, worktree_commit_range_diff, + delete_refs_with_prefix, fetch_repo_refs, fork_namespace, merge_base, refs_with_prefix, + worktree_commit_range_commits, worktree_commit_range_diff, }; use signed_state::{Backend, CheckoutsStore, GitStore, RepoListStore, RepoStore}; use signed_ui::{CountBadge, placeholder}; @@ -102,36 +102,6 @@ impl ForkCompare { } } -/// The refs namespace of a fork's import in the target mirror. -fn fork_namespace(announcement: &Announcement) -> String { - format!( - "{}/{}", - announcement.owner.to_hex(), - sanitize_path_component(&announcement.id) - ) -} - -/// The announced forks of `base` a New PR compare can be built from. -fn fork_candidates<'a>( - announcements: &'a [Announcement], - base: &RepoAddr, - base_euc: Option<&str>, - user: Option, -) -> Vec<&'a Announcement> { - let (mut own, mut others) = (Vec::new(), Vec::new()); - for announcement in announcements { - if announcement.clone.is_empty() || !announcement.is_fork_of(base, base_euc) { - continue; - } - if Some(announcement.owner) == user { - own.push(announcement); - } else { - others.push(announcement); - } - } - own.into_iter().chain(others).collect() -} - /// The display name of an announcement. /// /// Its human-readable name, falling back to the repository id. @@ -933,54 +903,31 @@ impl NewPullRequestView { 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 publish = store.update(cx, |store, cx| { + store.open_pull_request_from_refs( + repo_path, + merge_base, + compare_ref, + (!subject.is_empty()).then_some(subject), + description, + Some(branch_name), + false, + cx, + ) + }); - 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(()); - } - }; + if let Err(error) = publish.await { + this.update_in(cx, |this, _window, cx| { + this.submitting = false; + this.error = Some(error.to_string().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(); @@ -1463,140 +1410,3 @@ impl Render for NewPullRequestView { ) } } - -#[cfg(test)] -mod tests { - use nostr::prelude::*; - use signed_core::repo_addr; - - use super::*; - - const OWNER_KEYS: [&str; 3] = [ - "0000000000000000000000000000000000000000000000000000000000000001", - "0000000000000000000000000000000000000000000000000000000000000002", - "0000000000000000000000000000000000000000000000000000000000000003", - ]; - - /// Build a signed kind-30617 event for `owner` with the given tags. - fn announcement_event(owner: &str, tags: &[&[&str]]) -> Event { - let keys = Keys::new(SecretKey::from_hex(owner).expect("valid secret key")); - let tags: Vec = tags - .iter() - .map(|t| Tag::parse(t.to_vec()).expect("valid tag")) - .collect(); - EventBuilder::new(Kind::GitRepoAnnouncement, "") - .tags(tags) - .finalize(&keys) - .expect("signed event") - } - - fn announcements(owner_ix: usize, tags: &[&[&str]]) -> Vec { - vec![ - Announcement::from_event(&announcement_event(OWNER_KEYS[owner_ix], tags)) - .expect("parses"), - ] - } - - #[test] - fn fork_candidates_orders_own_forks_first() { - let euc = "aa231c4c6a5777dc89b42207b499891a344add5c"; - let clone = "https://grasp.example/npub1x/my-fork.git"; - - let base_addr = repo_addr( - PublicKey::from_hex(OWNER_KEYS[0]).expect("pubkey"), - "upstream", - ); - // Newest first, as RepoListStore keeps them. - // Unrelated repo, the user's fork with the shared EUC, another fork with a `u` tag. - let all = vec![ - announcements( - 2, - &[ - &["d", "other-project"], - &["r", "bb231c4c6a5777dc89b42207b499891a344add5c", "euc"], - ], - ) - .pop() - .unwrap(), - announcements( - 1, - &[&["d", "my-fork"], &["r", euc, "euc"], &["clone", clone]], - ) - .pop() - .unwrap(), - announcements( - 2, - &[ - &["d", "their-fork"], - &["u", &base_addr.to_string()], - &["clone", clone], - ], - ) - .pop() - .unwrap(), - ]; - - let user = PublicKey::from_hex(OWNER_KEYS[1]).expect("pubkey"); - let forks = fork_candidates(&all, &base_addr, Some(euc), Some(user)); - - // The user's fork comes first, then the other author's. - let ids: Vec<&str> = forks.iter().map(|a| a.id.as_str()).collect(); - assert_eq!(ids, vec!["my-fork", "their-fork"]); - } - - #[test] - fn fork_candidates_excludes_base_unrelated_and_unfetchable() { - let euc = "aa231c4c6a5777dc89b42207b499891a344add5c"; - let base_owner = PublicKey::from_hex(OWNER_KEYS[0]).expect("pubkey"); - let base_addr = repo_addr(base_owner, "upstream"); - - let mut all = vec![ - announcements(0, &[&["d", "upstream"], &["r", euc, "euc"]]) - .pop() - .unwrap(), - announcements(1, &[&["d", "no-clone-fork"], &["r", euc, "euc"]]) - .pop() - .unwrap(), - announcements( - 2, - &[ - &["d", "other"], - &["r", "cc231c4c6a5777dc89b42207b499891a344add5c", "euc"], - ], - ) - .pop() - .unwrap(), - announcements( - 2, - &[ - &["d", "mirror"], - &["r", euc, "euc"], - &["clone", "https://grasp.example/x/mirror.git"], - ], - ) - .pop() - .unwrap(), - ]; - - let forks = fork_candidates(&all, &base_addr, Some(euc), Some(base_owner)); - assert_eq!(forks.len(), 1); - assert_eq!(forks[0].id, "mirror"); - - // Without a base EUC only `u`-tag forks match. - all.push( - announcements( - 2, - &[ - &["d", "u-fork"], - &["u", &base_addr.to_string()], - &["clone", "https://grasp.example/x/u-fork.git"], - ], - ) - .pop() - .unwrap(), - ); - let forks = fork_candidates(&all, &base_addr, None, Some(base_owner)); - let ids: Vec<&str> = forks.iter().map(|a| a.id.as_str()).collect(); - assert_eq!(ids, vec!["u-fork"]); - } -} 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 2827c63..86ed7ba 100644 --- a/crates/workspace/src/views/repo_detail/pull_request_detail.rs +++ b/crates/workspace/src/views/repo_detail/pull_request_detail.rs @@ -20,8 +20,11 @@ use gpui_component::{ ActiveTheme, Sizable, StyledExt, VirtualListScrollHandle, WindowExt, h_flex, v_flex, v_virtual_list, }; -use nostr::prelude::{Event, EventId, Kind, Nip34Tag, Url}; -use signed_core::{activity_subject, pull_request_patch}; +use nostr::prelude::{Event, EventId, Kind}; +use signed_core::{ + 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}; use signed_state::{Backend, GitStore, ProfileStore, RepoStore}; use signed_ui::{CountBadge, UserAvatar, placeholder, status_badge}; @@ -701,66 +704,6 @@ fn open_update_pull_request_dialog( } /// The `c` tag of a PR event, the commit the proposal points at. -fn current_commit_of(root: &Event) -> Option { - root.tags - .iter() - .find_map(|tag| match Nip34Tag::parse(tag.as_slice()) { - Ok(Nip34Tag::CurrentCommit(commit)) => Some(commit.to_string()), - _ => None, - }) -} - -/// The `merge-base` tag of a PR event, as hex. -/// -/// The most recent common ancestor with the target branch. -fn merge_base_of(event: &Event) -> Option { - event - .tags - .iter() - .find_map(|tag| match Nip34Tag::parse(tag.as_slice()) { - Ok(Nip34Tag::MergeBase(commit)) => Some(commit.to_string()), - _ => None, - }) -} - -/// The `clone` tag of a PR event. -/// -/// URLs where the proposed branch can be fetched, or `None` if the PR has none. -fn clone_urls_of(event: &Event) -> Option> { - event - .tags - .iter() - .find_map(|tag| match Nip34Tag::parse(tag.as_slice()) { - Ok(Nip34Tag::Clone(urls)) => Some(urls), - _ => None, - }) -} - -/// The `branch-name` tag of a PR event, if any. -fn branch_name_of(event: &Event) -> Option { - event - .tags - .iter() - .find_map(|tag| match Nip34Tag::parse(tag.as_slice()) { - Ok(Nip34Tag::BranchName(name)) => Some(name), - _ => None, - }) -} - -/// The latest PR update, kind 1619, revising `root`. -fn latest_update<'a>(events: impl Iterator, root: &Event) -> Option<&'a Event> { - let root_hex = root.id.to_hex(); - events - .filter(|e| e.kind == Kind::GitPullRequestUpdate) - .filter(|e| e.pubkey == root.pubkey) - .filter(|e| { - e.tags - .iter() - .any(|t| t.kind() == "E" && t.content() == Some(root_hex.as_str())) - }) - .max_by_key(|e| e.created_at) -} - /// One-line commit metadata for the commits list. /// /// Author and relative time, whichever is available. @@ -821,104 +764,9 @@ impl Render for PullRequestDetailView { #[cfg(test)] mod tests { - use nostr::prelude::{Tag, *}; - use super::*; const COMMIT_HEX: &str = "1111111111111111111111111111111111111111"; - const OTHER_ROOT_HEX: &str = "2222222222222222222222222222222222222222"; - - fn keys() -> Keys { - Keys::new( - SecretKey::from_hex("0000000000000000000000000000000000000000000000000000000000000001") - .expect("valid secret key"), - ) - } - - /// Build a signed event with a controlled `created_at`. - fn signed(kind: Kind, tags: Vec, created_at: u64) -> Event { - EventBuilder::new(kind, "") - .tags(tags) - .custom_created_at(Timestamp::from(created_at)) - .finalize(&keys()) - .expect("signed event") - } - - fn pr_root() -> Event { - signed( - Kind::GitPullRequest, - vec![ - Tag::parse(["c", COMMIT_HEX]).expect("valid tag"), - Tag::parse(["branch-name", "feature/x"]).expect("valid tag"), - ], - 100, - ) - } - - #[test] - fn reads_current_commit_and_branch_name() { - let pr = pr_root(); - assert_eq!(current_commit_of(&pr).as_deref(), Some(COMMIT_HEX)); - assert_eq!(branch_name_of(&pr).as_deref(), Some("feature/x")); - } - - #[test] - fn returns_none_without_pr_tags() { - let pr = signed(Kind::GitPullRequest, vec![], 100); - assert_eq!(current_commit_of(&pr), None); - assert_eq!(branch_name_of(&pr), None); - } - - #[test] - fn latest_update_picks_newest_revision_of_the_root() { - let root = pr_root(); - let root_hex = root.id.to_hex(); - - let revision = |created_at: u64| { - signed( - Kind::GitPullRequestUpdate, - vec![Tag::parse(["E", &root_hex]).expect("valid tag")], - created_at, - ) - }; - // An update revising a different PR must be ignored even though it is newer. - let unrelated = signed( - Kind::GitPullRequestUpdate, - vec![Tag::parse(["E", OTHER_ROOT_HEX]).expect("valid tag")], - 999, - ); - - let events = [unrelated, revision(200), root.clone(), revision(300)]; - let latest = latest_update(events.iter(), &root).expect("an update"); - - assert_eq!(latest.created_at.as_secs(), 300); - assert_eq!(latest.kind, Kind::GitPullRequestUpdate); - } - - #[test] - fn latest_update_ignores_other_authors() { - let root = pr_root(); - let root_hex = root.id.to_hex(); - let other = Keys::new( - SecretKey::from_hex("0000000000000000000000000000000000000000000000000000000000000002") - .expect("valid secret key"), - ); - let stranger = EventBuilder::new(Kind::GitPullRequestUpdate, "") - .tags([Tag::parse(["E", &root_hex]).expect("valid tag")]) - .custom_created_at(Timestamp::from(999)) - .finalize(&other) - .expect("signed event"); - - // The tip of a PR is only mutable by its author. - // A newer update from anyone else must not win. - assert!(latest_update([&stranger, &root].into_iter(), &root).is_none()); - } - - #[test] - fn latest_update_ignores_roots_without_revisions() { - let root = pr_root(); - assert!(latest_update([&root].into_iter(), &root).is_none()); - } #[test] fn commit_meta_combines_author_and_time() { diff --git a/docs/backend-rearchitecture.md b/docs/backend-rearchitecture.md index 850582b..f88725e 100644 --- a/docs/backend-rearchitecture.md +++ b/docs/backend-rearchitecture.md @@ -122,6 +122,41 @@ everywhere, no exceptions. ## 2. The "send an event" functions — there are 8, there should be roughly 2 +> **Status: done.** `Backend::send`, `Backend::publish_event`, `Backend::publish_task`, +> `Backend::send_fire_and_forget` and the free fn `broadcast_event` are all +> deleted. In their place: `require_relay_accepted(output, event)` (the one +> "empty success ⇒ Err" check, `pub(crate)` so `repo.rs` can use it too) and +> `Backend::announce_published(event, cx)` (one line, emits +> `BackendEvent::Published`). Every call site now calls +> `client.send_event(&event).broadcast()` directly — the `.broadcast()` is +> the additive gossip-bypass from §4, added here since every one of these +> call sites already has a well-defined target (the relays this app +> explicitly added). `RepoStore::send` is also gone; its 4 identical +> one-shot callers (`open_issue`, `reply`, `set_status`, +> `publish_applied_status`) now call a private `RepoStore::publish` that +> does the same sign+send+check+`last_error` bookkeeping — kept as **one** +> small store-local helper rather than inlining the same ~20 lines 4 times, +> since all 4 call sites have byte-for-byte identical post-conditions (this +> is a deliberate, narrow exception to "delete `RepoStore::send`"; +> `stage_event_on_relay` was already the same shape of exception before this +> change). The 3 call sites with genuinely divergent control flow +> (`open_pull_request`, `update_pull_request`, `publish_patch_series`) now +> call `client.send_event(...)`/`require_relay_accepted` directly inline, +> fixing the inconsistent error surfacing this section originally flagged +> (all three now set `last_error` on failure, like every other `RepoStore` +> mutation). `retract_events` is rewritten per the NIP-09 section below. +> `stage_event_on_relay` is untouched, it already followed this pattern. +> Verified against the pinned `nostr-sdk` source that `SendEventOutput` +> (`= Output`) and `EventDeletionRequest` +> (`nostr/src/nips/nip09.rs`) have the shapes assumed here, and that +> `UniversalSigner` implements the `AsyncGetPublicKey + AsyncSignEvent` +> bounds `FinalizeEventAsync` requires. `cargo check --workspace`, +> `cargo clippy --workspace --all-targets` and `cargo test --workspace` all +> pass unchanged (165+ tests, no failures) — none of the deleted/rewritten +> functions had direct unit test coverage (they all require a live relay), +> so this was verified by compilation plus a careful line-by-line diff +> against the previous control flow for each of the 8 call sites. + Grep for anything that ends up calling `client.send_event`: | Function | File:line | What it adds over `client.send_event` | @@ -326,6 +361,13 @@ 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 +> **Status: done**, implemented as part of §2's send-path consolidation. +> Every direct `client.send_event(...)` call added while deleting the 8 +> send-path layers uses `.broadcast()` explicitly (repository announcements, +> state, issues, PRs, patches, comments, statuses and NIP-09 deletions). +> `.gossip(...)` stays configured in `signed_nostr::backend::with_database` +> for future NIP-17/NIP-65 features, per the recommendation below. + 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` @@ -784,6 +826,27 @@ source of truth. ## 10. Bootstrap-on-construction should go through `cx.defer`, not run synchronously in `new` +> **Status: done.** All six constructors listed in the table below now build +> `Self` with no side effects, capture `cx.entity().downgrade()`, and defer +> the bootstrap call(s) with `cx.defer(move |cx| { weak.update(cx, |this, +> cx| ...).ok-or-log(); })`. Verified `cx.entity()` is safe to call before +> the entity is registered: `App::new`'s `cx.entities.reserve()` bumps the +> ref count to 1 before `build_entity` runs (`app/entity_map.rs:114-117`), +> so `weak_entity().upgrade()` succeeds throughout construction, and the +> deferred closure only runs after `cx.new`'s `insert_entity` call has fully +> populated the entity, so the weak upgrade inside the deferred closure +> always succeeds too (barring the caller synchronously dropping the +> just-created `Entity` before yielding, an edge case worth a log line, not +> a crash). `RepoStore::new` bundles its three previously-sequential calls +> (`subscribe_remote`, `connect_announced_relays`, `refresh`) into one +> deferred closure to preserve their relative order. Failure to upgrade is +> logged with `log::warn!` rather than silently discarded with `.ok()`, per +> this project's error-handling rule. `cargo check --workspace`, +> `cargo clippy -p signed_state --all-targets` and `cargo test --workspace` +> all pass unchanged — none of the existing tests construct these stores +> through a `TestAppContext` and assert state immediately after `cx.new`, +> so no test needed a `cx.run_until_parked()` addition. + Verified against the pinned GPUI revision (`crates/gpui/src/app.rs:1999-2005`, `crates/gpui/src/app/context.rs:296-315`). @@ -840,6 +903,39 @@ talk to other entities" in the same synchronous call, which is exactly what ## 11. Split independently-observed state into child entities +> **Status: done**, with one correction to the approach originally sketched +> below. `Backend::pushing_repos` is now `Entity>`, +> created with `cx.new(|_| HashSet::new())` in `Backend::new` and exposed +> via `Backend::pushing_repos() -> Entity>` for future +> `cx.observe` callers (nothing reads it today — the actual UI-facing "is +> this repo pushing" indicator is the pre-existing, already-observable +> `RepoStore::pushing: bool`; this field is purely `push_repo_from`'s +> internal re-entrancy guard). +> +> The blocker: `Drop::drop(&mut self)` has no `cx` parameter, so `PushGuard` +> could not literally call `pushing_repos.update(cx, ...)` on drop as first +> sketched below — confirmed by checking Zed's own codebase, which hits the +> same wall and falls back to a raw `Mutex` for exactly this reason +> (`crates/project/src/project.rs`'s `RemotelyCreatedModelGuard`). The fix is +> `AsyncApp::on_drop(&self, entity: &WeakEntity, f: impl FnOnce(&mut T, +> &mut Context) + 'static) -> Deferred` +> (`gpui/src/app/async_context.rs:266-276`), which is exactly what several +> Zed crates already use for this "clean up an entity when a spawned task is +> cancelled" pattern (e.g. `git_ui/src/git_panel.rs`'s +> `_clear_pending_remote_operation = cx.on_drop(&this, |this, cx| ...)`). +> `push_repo_from` now inserts into `pushing_repos` synchronously before +> `cx.spawn` (using the already-available `&mut Context`), and +> holds `let _guard = cx.on_drop(&this, move |backend, cx| { ... remove ... +> });` for the lifetime of the spawned task — removal fires on completion, +> error, or cancellation alike, same as the old `Drop for PushGuard`, but +> now through a real, observable entity update with `cx.notify()`. The old +> `PushGuard` struct and its `Drop` impl are deleted; `Arc`/`Mutex` are no +> longer imported in `backend.rs` at all. `cargo check --workspace`, +> `cargo clippy --workspace --all-targets` and `cargo test --workspace` all +> pass unchanged; no call site outside `signed_state` touched +> `pushing_repos`, confirming it had zero external readers before this +> change. + `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 @@ -887,6 +983,33 @@ This principle is also the reason **not** to merge `LocalReposStore` and ## 12. One debounce at the source, not one per store +> **Status: done.** `Backend::new`'s pump now batches: it waits for the +> first `ClientNotification::Event`, then races `notifications.next()` +> against a `PUMP_DEBOUNCE` (200ms) timer in a loop, collecting every event +> that arrives before the deadline into one `Vec`, then emits a +> single `BackendEvent::NostrUpdate(Vec)`. Implemented with +> `futures::future::select` + `futures::pin_mut!`, matching the existing +> debounce idiom already used by `ProfileStore::handle_requests` in the same +> crate (not `select_biased!`, which isn't used anywhere else here). +> Verified `Client::notifications()` returns a `Pin ClientNotification> + Send>>` backed by a `broadcast::Receiver` +> (`nostr-sdk/src/client/mod.rs:199-205`, `pool/mod.rs:96`), so cancelling a +> `.next()` future mid-poll to race it against the timer cannot drop a +> notification — the broadcast cursor only advances on a completed receive. +> `BackendEvent::NostrUpdate` changed from `Update` to `Vec`; its 3 +> actual subscribers (`ProfileStore`, `RepoStore`, `RepoListStore` — +> `CheckoutsStore` only observes `RepoListStore`/`LocalReposStore`, it never +> matched on `NostrUpdate` directly) were updated to iterate the batch +> (`.any(...)` for the two relevance checks, a `for` loop over the +> metadata-kind updates in `ProfileStore`). Also fixed a `let _ =` silently +> discarding a `WeakEntity::update` result in `ProfileStore::handle_requests`, +> found while touching this file, replaced with the `.ok()` idiom used +> everywhere else in this crate for the same "entity may already be gone" +> case. Downstream per-store `RefreshGate` debounce windows are left +> unchanged for now, per the "measure before resizing" note below. +> `cargo check --workspace`, `cargo clippy --workspace --all-targets` and +> `cargo test --workspace` all pass unchanged. + Flagged example — the notification pump (`backend.rs:126-146`): ```rust @@ -962,6 +1085,16 @@ than speculatively resizing four timers up front. ## 13. `local_repos.rs` + `repo_list.rs`: merge the files, not the entities +> **Status: done.** Merged both files into `signed_state/src/repos.rs`, keeping +> `LocalReposStore` and `RepoListStore` as two fully independent structs, each +> still its own `Entity`/`Global` with the same `global()`/`set_global()` pairs +> and public API as before — zero call-site churn beyond fixing the `use` +> paths (`crate::local_repos`/`crate::repo_list` → `crate::repos`) in +> `checkouts.rs`, `repo.rs` and `lib.rs`. `cargo check --workspace`, +> `cargo clippy -p signed_state --all-targets` and `cargo test --workspace` +> (signed_state 24 tests, workspace 14 tests, full suite 165+ tests) all pass +> unchanged. + 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()` @@ -1167,6 +1300,47 @@ than avoidable duplication. ## 17. Business logic that leaked into `crates/workspace` and should move to `signed_core`/`signed_state` +> **Status: done.** All four sub-items landed: +> +> - `current_commit_of` is now `pub fn` in `signed_core::model`; the +> byte-for-byte duplicate in `pull_request_detail.rs` is deleted, replaced +> by an import. +> - `merge_base_of`, `clone_urls_of`, `branch_name_of` and `latest_update` +> moved to `signed_core::model` as `pub fn`s with their tests (the same +> `signed()`/`pr_root()`-style fixtures the doc predicted, renamed +> `signed_at`/`pr_root` to avoid colliding with `model.rs`'s existing +> single-owner `keys()`/`announcement_event` fixtures used by unrelated +> `is_fork_of` tests in the same file). `pull_request_detail.rs` lost the +> now-unused `Nip34Tag`/`Url` imports as a result. +> - `fork_candidates` moved to `signed_core::model` as planned. The doc's +> wording was ambivalent about where `fork_namespace` should go ("safe, +> low-risk move to `signed_core`" vs. "pairs naturally with `signed_git`'s +> ref-naming conventions" in the same paragraph) — turns out only one is +> actually possible: `fork_namespace` calls `signed_git::sanitize_path_component`, +> and `signed_git` **depends on** `signed_core` (`signed_git/Cargo.toml`), +> so moving it to `signed_core` would be a circular dependency. It moved to +> `signed_git` instead, next to `sanitize_path_component`, with a new unit +> test (it had none before). `fork_candidates` has no such constraint (only +> touches `Announcement`/`RepoAddr`/`PublicKey`) and moved to `signed_core` +> as planned, tests included. `new_pull_request.rs`'s entire `mod tests` +> block was deleted — both moved functions were the only things it tested. +> - `NewPullRequestView::submit` no longer calls `format_patch_between` +> itself: `RepoStore::open_pull_request_from_refs(repo_path, merge_base, +> compare_ref, subject, description, branch_name, draft, cx) -> +> Task>` does the `format_patch_between` + empty-check + +> `open_pull_request` sequence internally, with the exact same two error +> messages ( "No commits between the branches to propose" / +> "Failed to generate the patch: {error}") the view used to produce +> inline, now surfaced through the returned `Task`'s `Err` and displayed +> via the view's existing `self.error` field — no observable UI change. +> `submit` shrank to gathering form values and awaiting the store call; +> `format_patch_between` is no longer imported in `new_pull_request.rs`. +> +> `cargo check --workspace`, `cargo clippy --workspace --all-targets` and +> `cargo test --workspace` all pass; test counts moved with the functions +> (`signed_core` 41 → 48, `signed_git` 67 → 68 for the new `fork_namespace` +> test, `workspace` 14 → 7), no failures, no coverage lost. + 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 @@ -1350,15 +1524,22 @@ method. Done: see §9. Manual create-repository-then-open-detail-view pass still recommended before shipping, since it depends on the grasp push actually succeeding end-to-end against a live server. -7. **Merge `local_repos.rs` and `repo_list.rs` into one file** (§13), +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 + + Done: merged into `signed_state/src/repos.rs`, see §13 for verification + notes. +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 + + Done: `Backend`, `RepoStore`, `RepoListStore`, `LocalReposStore`, + `CheckoutsStore` and `ProfileStore` all defer their bootstrap now, see + §10 for verification notes. +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` @@ -1367,13 +1548,23 @@ method. 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). + + Done: see §2 and §4 for the full list of call sites, the one deliberate + narrow exception (`RepoStore::publish`), and verification notes. +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 + + Done: see §11 — implemented via `AsyncApp::on_drop`, not the plain + `Drop` impl originally sketched, which turned out not to be possible. +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. + + Done: see §12. Landed without waiting on §7 since it doesn't depend on + that decision — downstream `RefreshGate` windows were deliberately left + unresized, so there's nothing here for §7 to invalidate either way. 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). @@ -1383,7 +1574,7 @@ method. 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): +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 @@ -1392,6 +1583,10 @@ method. itself. Low risk, no behavior change, best done as its own small PR per function cluster rather than one big move. + Done: see §17. `fork_namespace` ended up in `signed_git`, not + `signed_core`, to avoid a circular crate dependency — everything else + landed exactly as planned. + 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