diff --git a/Cargo.lock b/Cargo.lock index be49088..0582a4a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1690,6 +1690,15 @@ dependencies = [ "syn 2.0.119", ] +[[package]] +name = "diffy" +version = "0.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3e3dc2f773b6aaa63b1a7684b8589f670a8a0146a510b74d23a401c882364b49" +dependencies = [ + "hashbrown 0.17.1", +] + [[package]] name = "digest" version = "0.10.7" @@ -7958,6 +7967,7 @@ name = "signed_git" version = "0.1.0-alpha" dependencies = [ "anyhow", + "diffy", "gix", "gix-worktree", "gix-worktree-state", diff --git a/crates/signed_git/Cargo.toml b/crates/signed_git/Cargo.toml index ef29ece..d52f8eb 100644 --- a/crates/signed_git/Cargo.toml +++ b/crates/signed_git/Cargo.toml @@ -12,6 +12,7 @@ gix = { workspace = true, features = ["revision", "blob-diff"] } gix-worktree = "0.56" gix-worktree-state = "0.34" anyhow.workspace = true +diffy = "0.5" [dev-dependencies] tempfile = "3" diff --git a/crates/signed_git/src/lib.rs b/crates/signed_git/src/lib.rs index 573ef66..fc81b8d 100644 --- a/crates/signed_git/src/lib.rs +++ b/crates/signed_git/src/lib.rs @@ -4,6 +4,8 @@ use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; use anyhow::{Context, Result, bail}; +use diffy::patch_set::{FileOperation, FilePatch, ParseOptions, PatchSet}; +use diffy::{Hunk, Line}; use gix::diff::blob::unified_diff::{ConsumeHunk, DiffLineKind as GixLineKind, HunkHeader}; use gix::interrupt::IS_INTERRUPTED; use gix::progress::Discard; @@ -1596,24 +1598,145 @@ fn tree_diff( } /// Parse `git format-patch` output, a single patch or a series. +/// +/// Backed by [`diffy::patch_set`], which implements git's extended diff format: +/// `diff --git` headers, rename and copy detection, binary detection, and +/// C-style quoted or octal-escaped paths. pub fn patch_diffs(patch: &str) -> Result { - let lines: Vec<&str> = patch.lines().collect(); - let mut files = Vec::new(); - let mut i = 0; + // `PatchSet` reports an error when the input holds no patch at all, + // while a patch without git diff sections is simply empty here. + if !patch.lines().any(|line| line.starts_with("diff --git ")) { + return Ok(CommitDiff { files: Vec::new() }); + } - while i < lines.len() { - let Some(header) = lines[i].strip_prefix("diff --git ") else { - i += 1; - continue; - }; - let (file, next) = parse_diff_section(header, &lines, i + 1)?; - files.push(file); - i = next; + let mut files = Vec::new(); + + for file in PatchSet::parse(patch, ParseOptions::gitdiff()) { + files.push(file_diff(file?)?); } Ok(CommitDiff { files }) } +/// The [`FileDiff`] of one parsed file patch. +fn file_diff(file: FilePatch<'_, str>) -> Result { + // The `---`/`+++` paths carry the `a/`/`b/` prefix, so the first path + // component is dropped, the same way `git apply -p1` does. + // Rename and copy paths come from their own headers, unprefixed. + let stripped; + let operation = match file.operation() { + operation @ (FileOperation::Rename { .. } | FileOperation::Copy { .. }) => operation, + operation => { + stripped = operation.strip_prefix(1); + &stripped + } + }; + + let (path, old_path, status) = match operation { + FileOperation::Create(path) => (path.as_ref(), None, DiffStatus::Added), + FileOperation::Delete(path) => (path.as_ref(), None, DiffStatus::Deleted), + FileOperation::Modify { modified, .. } => (modified.as_ref(), None, DiffStatus::Modified), + FileOperation::Rename { from, to } => { + (to.as_ref(), Some(from.as_ref()), DiffStatus::Renamed) + } + FileOperation::Copy { from, to } => (to.as_ref(), Some(from.as_ref()), DiffStatus::Copied), + }; + + let mut insertions = 0usize; + let mut deletions = 0usize; + let mut hunks = Vec::new(); + let patch = file.patch(); + if let Some(text) = patch.as_text() { + for hunk in text.hunks() { + let hunk = hunk_diff(hunk); + insertions += hunk + .lines + .iter() + .filter(|line| line.kind == DiffLineKind::Addition) + .count(); + deletions += hunk + .lines + .iter() + .filter(|line| line.kind == DiffLineKind::Deletion) + .count(); + hunks.push(hunk); + } + } + + Ok(FileDiff { + path: path.to_owned(), + old_path: old_path.map(str::to_owned), + status, + insertions, + deletions, + binary: patch.is_binary(), + hunks, + }) +} + +/// The [`DiffHunk`] of one parsed hunk, including the line number of every line. +/// +/// `diffy` reports only the hunk header ranges. The per-line numbers are +/// counted from them the way the header encodes them: context lines advance +/// both sides, deletions only the old, insertions only the new. +fn hunk_diff(hunk: &Hunk<'_, str>) -> DiffHunk { + let old_range = hunk.old_range(); + let new_range = hunk.new_range(); + let mut old = old_range.start() as u32; + let mut new = new_range.start() as u32; + + let mut lines = Vec::with_capacity(hunk.lines().len()); + for line in hunk.lines() { + let (kind, text) = match line { + Line::Context(text) => (DiffLineKind::Context, *text), + Line::Delete(text) => (DiffLineKind::Deletion, *text), + Line::Insert(text) => (DiffLineKind::Addition, *text), + }; + + let (old_no, new_no) = match kind { + DiffLineKind::Context => { + let numbers = (Some(old), Some(new)); + old += 1; + new += 1; + numbers + } + DiffLineKind::Addition => { + let number = Some(new); + new += 1; + (None, number) + } + DiffLineKind::Deletion => { + let number = Some(old); + old += 1; + (number, None) + } + }; + + lines.push(DiffLine { + kind, + old: old_no, + new: new_no, + text: line_text(text), + }); + } + + DiffHunk { + old_start: old_range.start() as u32, + old_lines: old_range.len() as u32, + new_start: new_range.start() as u32, + new_lines: new_range.len() as u32, + lines, + } +} + +/// The content of a parsed line without its line ending. +/// +/// `diffy` keeps the trailing `\n`, the way `str::lines` splits it off. +fn line_text(text: &str) -> String { + let text = text.strip_suffix('\n').unwrap_or(text); + text.strip_suffix('\r').unwrap_or(text).to_owned() +} + /// Commits of a `git format-patch` output, a single patch or a series. /// /// Entries appear in patch order, oldest first as `git format-patch` produces them. @@ -1695,277 +1818,6 @@ fn strip_patch_prefix(subject: &str) -> String { } } -/// Parse one file's diff section. -/// -/// Returns the section and the index of the first unconsumed line. -fn parse_diff_section(header: &str, lines: &[&str], start: usize) -> Result<(FileDiff, usize)> { - let (header_old, header_new) = header_paths(header)?; - // The `---` and `+++` lines name the two sides unambiguously. - // The `diff --git` header cannot distinguish spaces in paths. - // Fall back to the header for sections without them, pure renames and mode changes. - let mut old_path = header_old; - let mut new_path = header_new; - - let mut status = DiffStatus::Modified; - let mut binary = false; - let mut hunks = Vec::new(); - let mut insertions = 0usize; - let mut deletions = 0usize; - let mut i = start; - - while i < lines.len() { - let line = lines[i]; - - // The next file's section starts at this line. - if line.starts_with("diff --git ") { - break; - } - i += 1; - - if line.starts_with("@@ -") { - let (hunk, next) = parse_hunk(lines, i - 1)?; - i = next; - insertions += hunk - .lines - .iter() - .filter(|line| line.kind == DiffLineKind::Addition) - .count(); - deletions += hunk - .lines - .iter() - .filter(|line| line.kind == DiffLineKind::Deletion) - .count(); - hunks.push(hunk); - } else if let Some(rest) = line.strip_prefix("--- ") { - if rest == "/dev/null" { - status = DiffStatus::Added; - } else { - old_path = diff_line_path(rest, "a/")?; - } - } else if let Some(rest) = line.strip_prefix("+++ ") { - if rest == "/dev/null" { - status = DiffStatus::Deleted; - } else { - new_path = diff_line_path(rest, "b/")?; - } - } else if line.starts_with("new file mode ") { - status = DiffStatus::Added; - } else if line.starts_with("deleted file mode ") { - status = DiffStatus::Deleted; - } else if line.starts_with("copy from ") { - status = DiffStatus::Copied; - } else if line.starts_with("rename from ") { - status = DiffStatus::Renamed; - } else if line.starts_with("Binary files ") || line.starts_with("GIT binary patch") { - binary = true; - // A literal binary patch may follow. - // Skip it without consuming the next section's header. - while i < lines.len() && !lines[i].starts_with("diff --git ") { - i += 1; - } - break; - } - // Everything else, index, mode and similarity lines, is ignored. - } - - Ok(( - FileDiff { - path: new_path, - old_path: matches!(status, DiffStatus::Renamed | DiffStatus::Copied) - .then_some(old_path), - status, - insertions, - deletions, - binary, - hunks, - }, - i, - )) -} - -/// Parse one hunk, the `@@ -a,b +c,d @@` header plus every body line. -/// -/// Returns the hunk and the index of the first unconsumed line. -fn parse_hunk(lines: &[&str], start: usize) -> Result<(DiffHunk, usize)> { - let (old_start, old_lines, new_start, new_lines) = hunk_header(lines[start])?; - - let mut diff_lines = Vec::new(); - let mut old = old_start; - let mut new = new_start; - let mut i = start + 1; - - while i < lines.len() { - let line = lines[i]; - let Some(kind) = line_prefix_kind(line) else { - break; - }; - i += 1; - - // Context lines advance both counters. - // Deletions advance only the old counter, additions only the new one. - // Every line then carries its real number in both versions. - let (old_no, new_no) = match kind { - DiffLineKind::Context => { - let numbers = (Some(old), Some(new)); - old += 1; - new += 1; - numbers - } - DiffLineKind::Addition => { - let number = Some(new); - new += 1; - (None, number) - } - DiffLineKind::Deletion => { - let number = Some(old); - old += 1; - (number, None) - } - }; - diff_lines.push(DiffLine { - kind, - old: old_no, - new: new_no, - text: line[1..].to_owned(), - }); - } - - Ok(( - DiffHunk { - old_start, - old_lines, - new_start, - new_lines, - lines: diff_lines, - }, - i, - )) -} - -/// The kind of a hunk body line, from its first character. -/// -/// Lines outside a hunk, headers, `\ No newline...` and the next section, yield `None`. -fn line_prefix_kind(line: &str) -> Option { - match line.as_bytes().first()? { - b' ' => Some(DiffLineKind::Context), - b'+' => Some(DiffLineKind::Addition), - b'-' => Some(DiffLineKind::Deletion), - _ => None, - } -} - -/// Parse a unified-diff hunk header, `@@ -a,b +c,d @@`. -/// -/// Omitted line counts default to 1. -fn hunk_header(header: &str) -> Result<(u32, u32, u32, u32)> { - let rest = header - .strip_prefix("@@ ") - .context("malformed hunk header")?; - let (old_spec, rest) = rest.split_once(' ').context("malformed hunk header")?; - let new_spec = rest.split_once(' ').map(|(new, _)| new).unwrap_or(rest); - - fn parse(spec: &str) -> Result<(u32, u32)> { - let spec = spec.strip_prefix(['-', '+']).unwrap_or(spec); - let (start, count) = match spec.split_once(',') { - Some((start, count)) => (start, count.parse::()?), - None => (spec, 1), - }; - Ok((start.parse::()?, count)) - } - - let (old_start, old_lines) = parse(old_spec)?; - let (new_start, new_lines) = parse(new_spec)?; - Ok((old_start, old_lines, new_start, new_lines)) -} - -/// The old and new paths of a `diff --git a/X b/Y` header. -fn header_paths(header: &str) -> Result<(String, String)> { - if header.starts_with('"') { - // Quoted paths include the `a/` / `b/` prefix inside the quotes. - let (old, rest) = take_quoted(header).context("unterminated quoted path")?; - let rest = rest.trim_start(); - let new = if rest.starts_with('"') { - take_quoted(rest).context("unterminated quoted path")?.0 - } else { - rest.split_whitespace().next().unwrap_or(rest) - }; - let old = old - .strip_prefix("a/") - .context("old path without `a/` prefix")?; - let new = new - .strip_prefix("b/") - .context("new path without `b/` prefix")?; - Ok((unquote_path(old)?, unquote_path(new)?)) - } else { - let (old, rest) = header - .rsplit_once(" b/") - .context("malformed diff --git header")?; - let old = old - .strip_prefix("a/") - .context("old path without `a/` prefix")?; - Ok((old.to_owned(), rest.to_owned())) - } -} - -/// The path of a `--- a/X` or `+++ b/Y` line. -fn diff_line_path(line: &str, prefix: &str) -> Result { - let line = line.trim_end_matches('\t'); - if line.starts_with('"') { - let (path, _) = take_quoted(line).context("unterminated quoted path")?; - let path = path - .strip_prefix(prefix) - .context("diff line path without `a/` or `b/` prefix")?; - unquote_path(path) - } else { - Ok(line - .strip_prefix(prefix) - .context("diff line path without `a/` or `b/` prefix")? - .to_owned()) - } -} - -/// The content of a git C-style quoted path and the rest of the input. -/// The path spans the opening `"`, escaped content and closing `"`. -/// -/// `None` if unterminated. -fn take_quoted(input: &str) -> Option<(&str, &str)> { - let mut end = 1; // byte after the opening quote - let mut rest = &input[1..]; - while let Some(ch) = rest.chars().next() { - let len = ch.len_utf8(); - match ch { - '\\' => { - // Consume the escaped character too, it may be multi-byte. - let escaped = rest[len..].chars().next()?; - let consumed = len + escaped.len_utf8(); - end += consumed; - rest = &rest[consumed..]; - } - '"' => return Some((&input[1..end], &input[end + len..])), - _ => { - end += len; - rest = &rest[len..]; - } - } - } - None -} - -/// Undo git's C-style path quoting, `\NNN` octal escapes, `\"` and `\\`. -/// -/// Delegates to gitoxide's C-style quote implementation, `gix::quote::ansi_c::undo`. -/// It expects the surrounding double quotes, which are re-added around the interior. -fn unquote_path(path: &str) -> Result { - if !path.contains('\\') { - return Ok(path.to_owned()); - } - - let quoted = format!("\"{path}\""); - let (unquoted, _) = gix::quote::ansi_c::undo(gix::bstr::BStr::new(quoted.as_bytes())) - .map_err(|e| anyhow::anyhow!("malformed quoted path: {e}"))?; - String::from_utf8(unquoted.into_owned().to_vec()).context("invalid UTF-8 in quoted path") -} - /// Collects the hunks of one blob diff while tracking per-line numbers. struct HunkCollector<'a> { hunks: &'a mut Vec, diff --git a/crates/signed_state/src/repos.rs b/crates/signed_state/src/repos.rs index 47d3625..0ece0ad 100644 --- a/crates/signed_state/src/repos.rs +++ b/crates/signed_state/src/repos.rs @@ -203,7 +203,10 @@ impl RepoListStore { announcement || deletion } - BackendEvent::Synced | BackendEvent::SyncProgress { .. } => true, + // Only a completed sync refreshes the list. + // Progress ticks would re-scan the whole database several times + // per sync to reveal entries incrementally. + BackendEvent::Synced => true, _ => false, }; diff --git a/docs/backend-rearchitecture.md b/docs/backend-rearchitecture.md index f88725e..966fc2d 100644 --- a/docs/backend-rearchitecture.md +++ b/docs/backend-rearchitecture.md @@ -1,4 +1,4 @@ -# Backend re-architecture: findings and plan +# Backend re-architecture: findings and outcome This is a follow-up to an initial architecture review. It re-checks every claim against the **actual `nostr`/`nostr-sdk` source pinned by `Cargo.lock`** @@ -14,6 +14,12 @@ Scope: `crates/signed_nostr`, `crates/signed_state`, `crates/signed_core`, `crates/signed_git`, and `crates/workspace` (the actual call sites of the backend, audited for business-logic flaws and redundant conversions). +**Status: complete.** All 14 items of the plan are implemented and verified; +the compact record is the **Outcome** section at the end. Sections 1–17 are +kept as the analysis each change was based on — they describe the code as it +was *before* the change, so read them as rationale, not as current +documentation. + ## Summary of the ask 1. Never call `fetch_events`. Bootstrap only via `subscribe`/`sync` (negentropy), read from `client.database()`. @@ -30,17 +36,6 @@ Each is addressed below with concrete file:line references and a verified replac ## 1. `fetch_events` — one call site, and it should go too -> **Status: done.** `bootstrap_user` now calls `sync_bootstrap_only` (the -> same helper `Backend::sync_bootstrap` already used) against -> `filters::grasp_list(public_key)`, then reads the result back out through -> the existing `user_grasp_list_servers` query helper instead of hand-rolling -> a second `BTreeSet` → `Vec` collect. `client.add_relay(url)` -> no longer round-trips through `.as_str()`, and connects with -> `.and_connect()` (see §8) instead of a trailing pool-wide `client.connect()`. -> `grep -rn "fetch_events" crates/` now returns nothing in the whole -> workspace. `cargo check --workspace`, `cargo clippy -p signed_state`, and -> `cargo test -p signed_state` (24 tests, unchanged) all pass. - ``` grep -rn "fetch_events" crates/ crates/signed_state/src/backend.rs:1065 @@ -122,41 +117,6 @@ 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` | @@ -361,13 +321,6 @@ 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` @@ -489,22 +442,52 @@ path handling and quoted-path unescaping need to be checked against this project's test fixtures (`signed_git/src/lib.rs:3917-3962`, octal-escaped/non-ASCII quoted paths) before committing to the swap. +**Outcome of the spike: the swap is sound, and was committed.** Both specific +risks flagged above checked out: + +- **Renamed paths.** `diffy` produces `FileOperation::Rename { from, to }` from + the `rename from`/`rename to` extended headers, and those paths are *not* + `a/`/`b/`-prefixed, unlike `Create`/`Delete`/`Modify`, which come from the + `---`/`+++` lines *with* the prefix. The adapter therefore calls + `FileOperation::strip_prefix(1)` (git's `-p1`) only for the non-rename + variants — exactly the split `FileOperation`'s own doc comment and + `diffy`'s `examples/apply.rs` describe. This is the one place the two APIs + differ in shape, and the one place a naive port would have broken. +- **Quoted-path unescaping.** `diffy` decodes git's full C-style quoting — + named escapes *and* 3-digit octal — via `escaped_filename`, and rejects + non-UTF-8 in the `str` variant with `InvalidUtf8Path`. That matches the old + `gix::quote::ansi_c::undo` + `String::from_utf8` behavior exactly, error + case included. + +Two encoding details had to be matched rather than assumed: + +- `HunkRange::start()`/`len()` are the **literal hunk-header numbers** + (`@@ -1,3 +1,3 @@` → `start == 1`), not 0-based indices — `diffy`'s own + `diff/mod.rs` adds 1 when *building* a range from an index. So + `old_start`/`new_start`/`old_lines`/`new_lines` map across directly, and the + `@@ -0,0 +1 @@` empty-range case falls out for free. +- `Line`'s text **keeps** the trailing newline and has already had the + `+`/`-`/` ` prefix stripped. The adapter re-derives `DiffLine.old`/`new` by + counting from the hunk header (context advances both, deletion only old, + insertion only new, same as before) and strips the line ending the way + `str::lines` does. + +One deliberate behavior difference: `PatchSet` yields a single +`Err("no valid patches found")` for input containing no patch at all, where a +patch with no `diff --git` section used to yield an empty file list. +`patch_diffs` now short-circuits to an empty `CommitDiff` when no line starts +with `diff --git ` — the same guard `diffy`'s internal `find_gitdiff_start` +uses — so `empty_or_unparseable_patch_yields_no_files` still holds. + +Note: the earlier estimate above ("~700 line hand-rolled parser") was too +high; the parser itself was 370 lines, and the ~1300 lines of tests for it +remain, now serving as the fixture-by-fixture verification for the +crate-backed implementation. + --- ## 6. Remove the `tasks: Vec>` + `push_task` boilerplate — use `Task::detach()` -> **Status: done.** Removed the `tasks` field and `push_task` from all six -> stores (`backend.rs`, `checkouts.rs`, `local_repos.rs`, `profile.rs`, -> `repo.rs`, `repo_list.rs`); every call site now ends in `.detach()` -> instead. As with §14, most `cx.spawn` sites lost their type-inference -> anchor and needed an explicit `let task: Task> = ...` -> (or `gpui::Task<...>` where `Task` wasn't imported) before `.detach()`. -> A few closures that captured a variable also named `task` (the awaited -> inner task) were given a distinct outer name (`notify_task`, `publish`, -> `fetch`, `sync`) to avoid a confusing shadow. `cargo check --workspace` -> and `cargo test -p signed_state` (24 tests) / `cargo test -p workspace` -> (14 tests) all pass. - Verified against the actual pinned GPUI revision (`~/.cargo/git/checkouts/zed-a70e2ad075855582/1870e26/crates/scheduler/src/executor.rs:375-573` and `crates/gpui/src/executor.rs:32-63`). @@ -627,26 +610,6 @@ to `RepoListStore`. ## 8. Relay add/connect: stop round-tripping through strings, stop reconnecting the whole pool -> **Status: done.** Every `add_relay` call site now passes the `RelayUrl` -> directly (no `.as_str()`/`ToString` round trip) and chains `.and_connect()` -> instead of a separate, pool-wide `client.connect()`/`client.connect_relay()` -> call: `Backend::bootstrap` (both the `BOOTSTRAP_RELAYS` and `INDEXER_RELAYS` -> loops), `bootstrap_user`, `create_repository`, `publish_local_repo`, -> `connect_repo_relays`, and `stage_event_on_relay`. `Backend::add_relays` is -> deleted entirely — its two callers (`create_repository`, `publish_local_repo`) -> now capture `client` once before the surrounding `cx.spawn` and loop -> `client.add_relay(url).and_connect().await.ok();` directly over the -> `Vec` they already had, with no `Vec` conversion at all. -> Verified `.and_connect()` (`client/api/add.rs`) and pool-wide `.connect()` -> semantics (`client/api/connect.rs`) against the pinned nostr-sdk source -> before making the change (see chat history), plus that `pool.sync()` -> requires relays to already be present in the pool -> (`pool/mod.rs:679-693`, `relays.get(&url).ok_or_else(...)`), which is why -> `sync_bootstrap_only`/`bootstrap_user` can safely assume `BOOTSTRAP_RELAYS` -> are already added by `Backend::bootstrap` before any sync runs. -> `cargo check --workspace`, `cargo clippy -p signed_state`, and -> `cargo test -p signed_state` (24 tests) all pass. - Flagged example (`backend.rs:1071-1074`): ```rust @@ -733,24 +696,6 @@ Delete `Backend::add_relays` entirely once both call sites are inlined. ## 9. `create_repository`'s flow is backwards: it inits a mirror, then clones it into the real destination -> **Status: done.** `Backend::create_repository` now computes `destination` -> (`folder.join(dir_name)`) up front and calls `signed_git::init_repository` -> directly on it — no mirror path, no `Url::from_file_path`, no `clone_repo` -> call, no double `origin` setup. An explicit `destination.exists()` check -> (mirroring what `clone_repo` used to guard for free) replaces the removed -> clone step's own guard. The push at the end now runs against `destination` -> instead of the mirror path, and the task returns `(announcement, -> destination)` as before — no caller-visible signature change. This also -> dropped `GitStore`/`GitCache::repo_path`/`repo_addr` usage from the -> function entirely, since no mirror is created there anymore; `repo_addr` -> and `Context as AnyhowContext` became unused imports in `backend.rs` and -> were removed. Updated a stale comment in `signed_git`'s -> `working_copy_cloned_from_the_mirror_matches_head_and_origin` test, which -> referenced this flow by name even though it's a generic `clone_repo` -> fixture unrelated to `Backend::create_repository`. `cargo check --workspace`, -> `cargo clippy --workspace`, and `cargo test --workspace` (signed_git 67, -> signed_state 24, workspace 14) all pass. - This is a real business-logic flaw, not just a style issue. Today (`backend.rs:437-501`): @@ -826,27 +771,6 @@ 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`). @@ -903,39 +827,6 @@ 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 @@ -983,33 +874,6 @@ 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 @@ -1085,16 +949,6 @@ 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()` @@ -1146,14 +1000,6 @@ entirely disjoint observers. ## 14. `crates/workspace` has the same task-list pattern as §6 — and there it's an actual bug -> **Status: done.** The `tasks` field and all 17 push sites were removed from -> `RepoDetailView`, `NewPullRequestView`, `CommitDiffView` and -> `PullRequestDetailView`, replaced with `.detach()`. `cargo check -p workspace` -> and `cargo test -p workspace` (14 tests) pass. Removing the field cost each -> `cx.spawn`/`cx.spawn_in` call site its type-inference anchor, so every -> remaining spawn site needed an explicit `let task: gpui::Task> = ...` -> annotation — expect the same when doing §6's `signed_state` half. - §6 covers `signed_state`'s 6 stores, where the unpruned-`Vec` pattern is a style/complexity concern with no observed failure, because `push_task` always pruned before pushing. `crates/workspace` has the exact @@ -1197,21 +1043,6 @@ already exists once in the same crate. ## 15. `Vec` → `Vec` conversion sprawl — fix the 3 `signed_git` signatures, not the 8 call sites -> **Status: done.** `try_each_url`, `clone_repo`, `GitCache::ensure_clone` -> and `fetch_repo_refs` are now generic over `U: AsRef`. All 7 call -> sites (`repo.rs::merge_pull_request`/`clone_to_folder`, -> `repo_detail/mod.rs::load_repo`, `new_pull_request.rs::choose_fork` x2, -> `pull_request_detail.rs::load`/`clone_urls_of`) now pass the -> `Vec`/`Vec`-derived value straight through with a plain -> `.clone()` of the field, no `.iter().map(ToString::to_string).collect()` -> anywhere left in non-test code. One test in `signed_git` passed an empty -> untyped `&[]` literal to `fetch_repo_refs`, which lost its type-inference -> anchor once the function went generic — fixed with an explicit -> `&[] as &[String]` annotation. `cargo check --workspace`, -> `cargo clippy -p signed_state -p workspace -p signed_git --all-targets`, -> and `cargo test --workspace` (signed_git 67, signed_state 24, workspace 14) -> all pass. - `Announcement::clone` is `Vec` (`signed_core/src/model.rs`, `Url` being `nostr`'s re-export of the `url` crate's `Url`, `nostr/src/types/url.rs:15`, `pub use url::*;`). Every call site that needs to hand those URLs to @@ -1300,47 +1131,6 @@ 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 @@ -1475,121 +1265,120 @@ method. --- -## Action plan, in order of risk/reward +## Outcome -1. ✅ **Delete the fetch/sync dedup cache** (§3). Pure removal, no behavior - change for the intended usage pattern (each call site already has, or - trivially gets, its own guard). Lowest risk, do first. +All 14 items below are implemented and verified, listed in the order they were +done — mechanical removals first, the largest diff (§2) and the riskiest swap +(§5) last. - Done: removed `recent_fetches`/`fetch_recently_started`/`fetch_fingerprint`/ - `FETCH_DEDUP_WINDOW` and the now-unused `DefaultHasher`/`Hash`/`Hasher`/ - `Instant` imports from `signed_state/src/backend.rs`. `connect_repo_relays` - and `sync_bootstrap` no longer fingerprint or gate on a cache; callers keep - their own guards (`RepoStore::repo_relays`, one-shot construction-time call - in `RepoListStore::subscribe_remote`). `cargo check --workspace` and - `cargo test -p signed_state` (24 tests) both pass unchanged. -2. ✅ **Remove the `tasks: Vec>` + `push_task` boilerplate**, in - both `signed_state` (§6) and `crates/workspace` (§14), in favor of - `.detach()`/`.detach_and_log_err(cx)`. Independent of every other change - here, touches 10 files, all mechanical — and fixes a real unbounded-growth - bug in `RepoDetailView`/`NewPullRequestView` along the way. +1. **Delete the fetch/sync dedup cache** (§3). `recent_fetches`, + `fetch_recently_started`, `fetch_fingerprint` and `FETCH_DEDUP_WINDOW` are + gone, along with the `DefaultHasher`/`Hash`/`Hasher`/`Instant` imports they + needed. Each call site keeps its own guard. +2. **Remove the `tasks: Vec>` + `push_task` boilerplate** on both + sides (§6, §14) — all six `signed_state` stores and all four + `crates/workspace` views, 17 push sites, most never pruned. In the views + this was a real unbounded-growth bug, not just style. +3. **Fix the relay add/connect calls** (§8). No `.as_str()`/`ToString` round + trips; `add_relay(url).and_connect()` instead of a separate, pool-wide + `connect()`; `Backend::add_relays` deleted. +4. **Fix `bootstrap_user`** (§1) to sync via negentropy and query the local + database. `grep -rn "fetch_events" crates/` now returns nothing. +5. **Generalize the three `signed_git` URL-list signatures** to + `U: AsRef` (§15) and drop the seven + `.iter().map(ToString::to_string).collect()` call sites, which now pass + `&[Url]` straight through. +6. **Fix `create_repository`'s init/clone ordering** (§9). It initializes and + pushes directly at the destination; no mirror, no `clone_repo`, no double + `origin`. +7. **Merge `local_repos.rs` and `repo_list.rs`** into + `signed_state/src/repos.rs` (§13), keeping both stores independent + `Entity`/`Global`s — no call-site change beyond `use` paths. +8. **Route construction-time bootstrap through `cx.defer`** in all six stores + (§10), with a failed weak upgrade logged rather than silently dropped. +9. **Consolidate the send paths** (§2, §4). The eight layers (`Backend::send`, + `publish_event`, `publish_task`, `send_fire_and_forget`, `broadcast_event`, + `RepoStore::send`, …) collapsed to direct + `client.send_event(&event).broadcast()` calls plus one + `require_relay_accepted` helper. `retract_events` now sends one NIP-09 + deletion per target, with no `k` tag. The rewrite also fixed the + inconsistent error surfacing this section flagged: the three call sites + with divergent control flow now set `last_error` on failure like every + other `RepoStore` mutation. +10. **Split `pushing_repos` into a child entity** (§11). It is now an + observable `Entity>`; the old `PushGuard` and the + `Arc`/`Mutex` around it are gone. +11. **Centralize the notification-pump debounce** (§12). The pump batches into + a single `BackendEvent::NostrUpdate(Vec)` behind a 200 ms window, + and its three subscribers iterate the batch. +12. **Drop progressive reveal** (§7). `RepoListStore` refreshes once per + completed sync instead of several times mid-sync. +13. **Replace the hand-rolled `git format-patch` parser with + `diffy::patch_set`** (§5). 370 lines to 216, no test changed. +14. **Move the misplaced `workspace` domain logic** (§17) to + `signed_core`/`signed_git`, and give `RepoStore` a refs-in-patch-out + method so `NewPullRequestView::submit` no longer generates patches itself. - Done: both halves are complete, see §6 and §14 for details. -3. ✅ **Fix the relay add/connect calls** (§8): drop `.as_str()`/`ToString` - round trips, replace `add_relay` + blanket `client.connect()`/ - `connect_relay` pairs with `add_relay(url).and_connect()`, and delete - `Backend::add_relays`. Mechanical, no behavior change beyond "connect - only what was just added." +### Deviations, corrections, and findings worth keeping - Done: see §8 for the full list of call sites and verification notes. -4. ✅ **Fix `bootstrap_user`** to sync+query instead of `fetch_events` (§1). - One function, fully covered by existing tests for - `latest_grasp_list_servers`. +Most items landed exactly as planned. These are the ones that did not, plus +the non-obvious findings that were only ever recorded in the per-item status +notes this section replaced: - Done: see §1. `fetch_events` no longer appears anywhere in the workspace. -5. ✅ **Generalize the 3 `signed_git` URL-list signatures** to `&[impl AsRef]` - (§15), then delete the now-redundant `.map(ToString::to_string).collect()` - at all 7 call sites. Self-contained to `signed_git`'s public API plus a - one-line change per call site; re-run `signed_git`'s existing tests - (`clone_repo`/`fetch_repo_refs` already have coverage). +- **`RepoStore::publish` was deliberately kept** (§2), a narrow exception to + "delete `RepoStore::send`": its four callers (`open_issue`, `reply`, + `set_status`, `publish_applied_status`) have byte-for-byte identical + sign+send+check+`last_error` post-conditions. The three callers with + genuinely divergent control flow (`open_pull_request`, + `update_pull_request`, `publish_patch_series`) call the SDK inline. +- **`fork_namespace` went to `signed_git`, not `signed_core`** as §17 + sketched. It calls `signed_git::sanitize_path_component`, and `signed_git` + already depends on `signed_core`, so the sketched direction would have been + a circular crate dependency. +- **`pushing_repos` was made observable with `AsyncApp::on_drop`, not a `Drop` + impl** (§11). `Drop::drop(&mut self)` has no `cx`, so it cannot update a GPUI + entity; Zed's own codebase hits the same wall and falls back to a raw + `Mutex` (`crates/project/src/project.rs`, `RemotelyCreatedModelGuard`). +- **Calling `cx.entity()` before the entity is registered is safe** (§10) — + what makes the deferred bootstrap sound. `App::new`'s `cx.entities.reserve()` + bumps the ref count before `build_entity` runs + (`app/entity_map.rs:114-117`), and the deferred closure only runs after + `cx.new`'s `insert_entity`. +- **Gossip stays enabled** in `ClientBuilder` for future NIP-17/NIP-65 work; + every git-domain send bypasses it explicitly with `.broadcast()` (§4). +- **`pool.sync()` requires the relays to already be in the pool** + (`pool/mod.rs:679-693`), which is why `Backend::bootstrap` adds + `BOOTSTRAP_RELAYS` before `sync_bootstrap_only`/`bootstrap_user` run — the + precondition §8's change relies on. +- **`pushing_repos` has no readers today** (§11): it is `push_repo_from`'s + internal re-entrancy guard. The UI-facing "is pushing" indicator is the + pre-existing, already-observable `RepoStore::pushing` boolean. +- **`patch_diffs` short-circuits on input with no `diff --git ` line** (§5), + because `PatchSet` yields `Err("no valid patches found")` for input holding + no patch at all, where the old parser returned an empty list. +- **A `let _ =` on a `WeakEntity::update` in `ProfileStore::handle_requests` + became `.ok()`** (§12), found while touching that file. It now follows the + project's error-handling rule. +- **Two type-inference anchors had to be re-added by hand**, a recurring cost + of both §6/§14 and §15: removing a `Vec>` field and going generic + over `AsRef` both strip the anchor from call sites with untyped `&[]` + literals, fixed with explicit `Task>` and + `&[] as &[String]` annotations. +- **Not covered by tests:** the deleted send paths (§2) and the + `create_repository` fix (§9) need a live relay or a live GRASP server, so + they were verified by compilation plus a line-by-line diff against the old + control flow. A manual create-repository-then-open-detail-view pass is still + the recommended pre-ship check for §9. - Done: see §15 for the full list of call sites and verification notes. -6. ✅ **Fix `create_repository`'s init/clone ordering** (§9): initialize and - push directly at the user's chosen destination, drop the mirror - pre-population entirely and let `ensure_clone` populate it lazily like - every other repo. Self-contained to one function; verify against this - crate's existing `init_repository`/push tests plus a manual - create-repository-then-open-detail-view pass. - - 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), - keeping both stores as independent entities. Purely organizational, zero - call-site changes, safe to do any time. - - 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. - - 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` - (one deletion event per target, no `k` tag), and make each send site - explicit about bypassing gossip (§4) with `.broadcast()`/`.to(relays)`. - This is the biggest diff and touches every publish call site (`repo.rs`, - `backend.rs`), so do it as its own PR with full test-suite coverage - before/after. - - 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`. - - 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). -13. **Spike `diffy::patch_set::PatchSet`** to replace `signed_git`'s - hand-rolled `git format-patch` parser (§5). Separate PR, separate - crate, no interaction with the nostr-facing changes above — do this in - parallel if you have a second contributor, otherwise last since it's - the largest and riskiest single change (needs fixture-by-fixture - verification against the existing test suite). -14. ✅ **Move the misplaced `workspace` domain logic to `signed_core`/`signed_state`** (§17): - make `current_commit_of` `pub` in `signed_core` and delete the - `workspace` duplicate; move `merge_base_of`/`clone_urls_of`/`branch_name_of`/ - `latest_update` and `fork_candidates`/`fork_namespace` there too, tests - included; give `RepoStore` a refs-in-patch-out method so - `NewPullRequestView::submit` stops calling `format_patch_between` - itself. Low risk, no behavior change, best done as its own small PR per - function cluster rather than one big move. - - 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. +Verification: `cargo check --workspace`, `cargo clippy --workspace +--all-targets` and `cargo test --workspace` pass — 167 tests, 0 failures — +re-run after each item landed rather than once at the end. Test counts moved +between crates as functions moved (`signed_core` 41 → 48, `signed_git` +67 → 68, `workspace` 14 → 7); no coverage was lost. Everything **not** listed above (per-repo/per-list `Entity` stores, the `RefreshGate` debounce/coalesce pattern, `Nip34Tag`/`Coordinate`/`Filter` usage in `signed_core`, the GRASP push-retry state machine in `push_staged_to_grasps`, the `UniversalSigner` abstraction, and the dense -`.clone()` clusters audited in §16) was checked and already matches "use -the SDK directly, no unnecessary wrapper" — those should be left alone. +`.clone()` clusters audited in §16) was checked and already matches "use the +SDK directly, no unnecessary wrapper" — those were left alone.