reya/signed
1
0
Fork 0
Code Issues Pull Requests Actions 5 Releases Activity
Files
5f37f2ff2a06d2e61fc2d29e3b2381775b044732
signed/docs/backend-rearchitecture.md
T
reya 5f37f2ff2a wip
2026-09-09 21:13:29 +07:00

63 KiB
Raw Blame History

Backend re-architecture: findings and plan

This is a follow-up to an initial architecture review. It re-checks every claim against the actual nostr/nostr-sdk source pinned by Cargo.lock (rev 0c6fad2ac8ce934747096953f6dba355e3532614, checked out locally at ~/.cargo/git/checkouts/nostr-9dff06fa64f758da/0c6fad2/{nostr,nostr-sdk}/src) and the actual GPUI source pinned by Cargo.lock (git+https://github.com/zed-industries/zed#1870e269ad88802147f2baec3086abb67d17260a, checked out at ~/.cargo/git/checkouts/zed-a70e2ad075855582/1870e26/crates/{gpui,scheduler}/src), not from general knowledge of either. Every API claim below cites the file it was verified against.

Scope: crates/signed_nostr, crates/signed_state, crates/signed_core, crates/signed_git, and crates/workspace (the actual call sites of the backend, audited for business-logic flaws and redundant conversions).

Summary of the ask

  1. Never call fetch_events. Bootstrap only via subscribe/sync (negentropy), read from client.database().
  2. Collapse the multiple "send an event" functions into direct nostr-sdk calls, no house wrappers.
  3. Verify every API claim against the locally checked-out SDK/GPUI source.
  4. Remove unnecessary logic (relay add/connect round trips, the fetch/sync dedup cache, unbounded task lists).
  5. Re-evaluate signed_git's dependence on gix — how much of it duplicates functionality gix (or another crate) already provides.
  6. Check for unnecessary cx.notify() / over-broad re-renders vs. partial re-render.
  7. Audit crates/workspace (the real UI call sites) for business-logic flaws of the same shape as create_repository, and for unnecessary string/type conversions and clones.

Each is addressed below with concrete file:line references and a verified replacement.

1. fetch_events — one call site, and it should go too

grep -rn "fetch_events" crates/
crates/signed_state/src/backend.rs:1065

The only use in the whole workspace is Backend::bootstrap_user (crates/signed_state/src/backend.rs:1059-1086):

fn bootstrap_user(&mut self, public_key: PublicKey, cx: &mut Context<Self>) {
    let client = self.client.clone();
    self.push_task(cx.spawn(async move |this, cx| {
        let result = async {
            let events: Vec<Event> = client
                .fetch_events(filters::grasp_list(public_key))
                .await?
                .into_iter()
                .collect();
            for url in latest_grasp_list_servers(events) {
                client.add_relay(url.as_str()).await.ok();
            }
            client.connect().await;
            Ok::<_, Error>(())
        }.await;
        ...
    }));
}

Verified against nostr-sdk/src/client/mod.rs:963-1018 (doc comment on Client::fetch_events): it's explicitly the "buffer events, return a Vec" sibling of stream_events, both explicitly documented as short-lived subscriptions for one-off reads — the SDK's own guidance ("for long-lived subscriptions use Client::subscribe") doesn't forbid fetch_events outright, but the project rule you want is stricter: never bypass the database. That's achievable here too, because client.sync degrades gracefully to a plain fetch-and-store when the local DB has nothing yet.

Replacement — sync against the bootstrap relays (same relays already used for every other bootstrap query, see BOOTSTRAP_RELAYS, backend.rs:28-33) and then read the result out of the database, exactly like every other store in this codebase already does:

// Also drops push_task/tasks in favor of .detach() — see §6.
fn bootstrap_user(&mut self, public_key: PublicKey, cx: &mut Context<Self>) {
    let client = self.client.clone();
    cx.spawn(async move |this, cx| {
        let result = async {
            client
                .sync(filters::grasp_list(public_key))
                .with(BOOTSTRAP_RELAYS)
                .await?;

            let events = client.database().query(filters::grasp_list(public_key)).await?;
            for url in latest_grasp_list_servers(events) {
                client.add_relay(url).and_connect().await.ok(); // see §8
            }
            Ok::<_, Error>(())
        }.await;
        ...
    })
    .detach();
}

Verified against nostr-sdk/src/client/api/sync.rs:1-32 and nostr-sdk/src/client/mod.rs:1020-1030 (Client::sync doc: "Performs a negentropy-based reconciliation between the local database and one or more relays" — this is exactly a bootstrap-and-store operation, no separate "first fetch" step needed). No other code changes: filters::grasp_list and latest_grasp_list_servers are unaffected.

This also removes the last inconsistency in the codebase between "how we get data:" everywhere else is sync-then-query; now it's sync-then-query everywhere, no exceptions.

2. The "send an event" functions — there are 8, there should be roughly 2

Grep for anything that ends up calling client.send_event:

Function File:line What it adds over client.send_event
Backend::send backend.rs:1308-1322 signs with the current signer, then calls broadcast_event
Backend::publish_event backend.rs:1325-1332 calls broadcast_event on an already-signed event
Backend::publish_task backend.rs:1335-1360 wraps a future, emits BackendEvent::Published/Error
Backend::send_fire_and_forget backend.rs:1363-1375 calls send, drops the result except logging
Backend::retract_events backend.rs:1378-1398 hand-builds NIP-09 tags, calls send
broadcast_event (free fn) backend.rs:1404-1418 calls client.send_event, turns "0 relays accepted" into an Err
stage_event_on_relay backend.rs:1768-1795 calls client.send_event(..).to([relay]), same 0-accept-is-Err logic, different error type (String)
RepoStore::send repo.rs:1334-1344 calls Backend::send, tracks last_error — but several RepoStore methods bypass it and call Backend::send/Backend::publish_event directly (repo.rs:803, repo.rs:935), so error surfacing is inconsistent across RepoStore methods

That's 8 layers for what the SDK already does in one call. Verified against nostr-sdk/src/client/api/send_event.rs:119-350:

  • client.send_event(&event) already verifies the signature, saves the event to the local database (save_into_database, default true), and broadcasts — all before you touch anything (send_event.rs:337-345).
  • Zero-relay-accepted is not an error from the SDK's point of view — it returns Ok with output.success empty and output.failed populated. Turning that into an app-level error is legitimate domain logic (the repo already gets this right), it just doesn't need 3 separate functions (broadcast_event, stage_event_on_relay, and the implicit success check buried in RepoStore::send) doing the same "empty success ⇒ error" check.

Recommended shape: one helper, and direct SDK calls everywhere else

Keep exactly one small helper because the "empty success ⇒ Err" rule is real, repeated, app-specific policy (the SDK intentionally leaves that decision to the caller):

/// The event was accepted by at least one relay, or a descriptive error otherwise.
async fn require_relay_accepted(output: SendEventOutput) -> Result<Event, Error> {
    if output.success.is_empty() && !output.failed.is_empty() {
        let reasons = output.failed.values().cloned().collect::<Vec<_>>().join(", ");
        bail!("event not accepted by any relay: {reasons}");
    }
    Ok(event)
}

Then delete Backend::send, Backend::publish_event, Backend::send_fire_and_forget, broadcast_event, and RepoStore::send. Call client.send_event(...) directly at each call site, exactly like stage_event_on_relay already does for the GRASP staging path — that function is the one place in the codebase that already follows this pattern (.to([relay.clone()]), explicit target, no extra wrapper beyond the accept-check). Generalize that pattern instead of routing everything through Backend.

// A GPUI call site, e.g. RepoStore::open_issue, today:
self.send(builder, cx);

// direct SDK call instead. No task list to push into and prune either —
// see §6, `.detach()` is the right default here.
let signer = Backend::global(cx).read(cx).signer();
let client = Backend::global(cx).read(cx).client();
cx.spawn(async move |this, cx| {
    let event = builder.finalize_async(&signer).await?;
    let output = client.send_event(&event).await?;
    let event = require_relay_accepted(output, event).await?;
    this.update(cx, |this, cx| { /* apply + cx.notify() */ })
})
.detach();

Backend still owns the Client/UniversalSigner (a real, load-bearing type — see §4 for why it must stay), but it should expose them (Backend::client()/Backend::signer(), both already exist, backend.rs:1089-1096) rather than mediate every publish through 4 layers of wrapper. Emitting BackendEvent::Published for cross-store invalidation (e.g. so RepoListStore refreshes when a new announcement lands) is the one piece of publish_task worth keeping — but it can be a single fn taking &Event that any call site invokes after its own send_event, not the thing that does the sending.

Backend::retract_events — use the SDK's own NIP-09 builder, one deletion event per target

nostr already ships EventDeletionRequest (verified in nostr/src/nips/nip09.rs:15-92), which implements IntoEventBuilder exactly like GitRepositoryAnnouncement/GitIssue/etc. already used elsewhere in this codebase. Today's code hand-builds the tags for one deletion event covering every target, plus a k tag per target:

// today, backend.rs:1378-1398
let mut tags: Vec<Tag> = Vec::with_capacity(events.len() * 2);
for event in events {
    tags.push(Tag::event(event.id));
    tags.push(Tag::parse(["k", &event.kind.to_string()]).expect("valid kind tag"));
}
let task = self.send(EventBuilder::new(Kind::EventDeletion, "").tags(tags), cx);

Per direction from the team: no k tag, and each event gets its own deletion event rather than one deletion event listing multiple e tags. EventDeletionRequest (nip09.rs:15-92) supports exactly that shape already — call .id(event.id) once per event and send each independently:

async fn retract_event(client: &Client, signer: &UniversalSigner, event: &Event) -> Result<(), Error> {
    let builder = EventDeletionRequest::new().id(event.id).into_event_builder();
    let deletion = builder.finalize_async(signer).await?;
    client.send_event(&deletion).await?;
    Ok(())
}

fn retract_events(&mut self, events: &[Event], cx: &mut Context<Self>) {
    let client = self.client.clone();
    let signer = self.signer.clone();

    for event in events.to_vec() {
        let client = client.clone();
        let signer = signer.clone();

        cx.spawn(async move |_this, _cx| {
            if let Err(e) = retract_event(&client, &signer, &event).await {
                log::warn!("failed to retract event {}: {e}", event.id);
            }
        })
        .detach();
    }
}

No hand-rolled tag construction, no batching multiple targets into one event, no k tag, and no task list to maintain (§6). Each deletion is independent: a relay rejecting or dropping one doesn't affect the others.

3. Remove the fetch/sync dedup cache — it duplicates state that already exists elsewhere

Backend carries:

recent_fetches: HashMap<u64, Instant>,          // backend.rs:86
const FETCH_DEDUP_WINDOW: Duration = ...;       // backend.rs:39
fn fetch_recently_started(&mut self, fingerprint: u64) -> bool { ... } // backend.rs:1178-1186
fn fetch_fingerprint(relays: &[&str], filters: &[Filter]) -> u64 { ... } // backend.rs:1423-1433

used at 3 call sites (connect_repo_relays, sync_bootstrap, and indirectly wherever those are called), e.g.:

pub fn sync_bootstrap(&mut self, filter: Filter, cx: &mut Context<Self>) {
    let fingerprint = fetch_fingerprint(&BOOTSTRAP_RELAYS, std::slice::from_ref(&filter));
    if self.fetch_recently_started(fingerprint) {
        log::debug!("skipping duplicate bootstrap sync");
        return;
    }
    ...
}

This is a generic "have I already asked for this filter recently" cache, sorting + hashing relay lists and filters, pruning on a 5-minute window, and un-inserting on error so a failed sync can retry immediately. It exists purely to avoid redundant sync/subscribe calls — but every call site that calls into Backend::sync_bootstrap/connect_repo_relays already has its own, more precise state for exactly this purpose:

  • RepoStore tracks repo_relays: HashSet<RelayUrl> (repo.rs:77) — "have I already connected+fetched this repo's relays" — and root_fetches: HashSet<EventId> (repo.rs:81) for per-root fetches.
  • RepoListStore and CheckoutsStore each already run every refresh through RefreshGate (refresh.rs), which itself exists to coalesce bursts of refresh requests — that's the same "don't do this again right now" idea, at the right granularity (per-store, per-purpose), not a generic cross-cutting cache keyed by a hash of relays+filters.

The Backend-level cache is solving the same problem a second time, at a coarser and more error-prone granularity (a hash collision or an order-sensitivity bug silently drops a legitimate sync; the 5-minute window is a magic number with no connection to how often any of the 3 call sites actually fire). Delete recent_fetches, fetch_recently_started, fetch_fingerprint, FETCH_DEDUP_WINDOW, and DefaultHasher/Hash/Hasher imports they pull in. Let each caller guard itself the way RepoStore already does for repo_relays:

// RepoStore, once per repo — this pattern already exists (repo.rs:189ish),
// just needs to also gate the *bootstrap* sync calls the same way instead
// of relying on a Backend-side cache.
if self.repo_relays.insert(relay.clone()) {
    backend.update(cx, |backend, cx| backend.connect_repo_relays(vec![relay], filters, cx));
}

sync_bootstrap for repo-independent filters (announcements, deletions) is called from exactly one place today (RepoListStore::subscribe_remote, repo_list.rs:144-153), on store construction — i.e., once per app session. It does not need a dedup cache at all; if you're worried about a second RepoListStore instance ever existing, that's a Global-uniqueness invariant, not something to paper over with a fingerprint cache.

4. Gossip is enabled, and stays enabled — but today's git-domain sends should bypass it explicitly

Per team direction: gossip is a deliberate, load-bearing choice for this client (it's not fully wired up to a feature yet, but it's not incidental configuration either). .gossip(...) stays in signed_nostr::backend::with_database (crates/signed_nostr/src/backend.rs:31-51). This section is scoped down to what falls out of that: how the currently-implemented send paths interact with gossip being on, verified against the SDK source.

let client = ClientBuilder::default()
    .database(database)
    .authenticator(authenticator)
    .gossip(NostrGossipMemory::unbounded())
    .gossip_config(GossipConfig::default().no_background_refresh())
    ...
    .build();

Verified against nostr-sdk/src/client/api/send_event.rs:337-388 and the doc comment on Client::send_event (client/mod.rs:1097-1130): when no explicit target is set (no .to()/.broadcast()/.to_nip17()/.to_nip65()), and gossip is configured, send_event resolves the destination via the gossip engine (NIP-65 relay discovery for the event's author + tagged pubkeys), not simply "every relay you add_relay'd". Every one of the 8 send-paths in §2 calls client.send_event(&event) with no explicit target — meaning every one of them is going through gossip-based relay resolution today, on top of the relays this app added on purpose (BOOTSTRAP_RELAYS, the repo's own relays tag, GRASP servers).

That happens not to lose anything today, because gossip_prepare_urls (send_event.rs:229-320) also unions in client.pool().write_relay_urls() at the end — so events still reach every WRITE relay in the pool, gossip only adds more relays on top. But it's not free: every plain send_event call (opening an issue, commenting, reacting to a PR) does gossip relay-list resolution — potentially a network round trip to fetch a NIP-65 list — for events whose target set is already fully determined by the repo's own relays tag or the bootstrap relay list, and where the extra NIP-65 relays gossip adds are not places NIP-34 consumers are expected to look.

Recommendation: keep .gossip(...) configured (it's wanted for whatever's next — NIP-17 DMs, NIP-65 profile/relay-list features, etc.), but make the git-domain sends that already have a well-defined target explicit about it, the same way stage_event_on_relay already is (.to([relay.clone()]), backend.rs:1768-1795):

  • Repository-scoped events (announcements, state, issues, PRs, patches, comments, statuses, deletions) know their target relays already (the repo's relays tag, or BOOTSTRAP_RELAYS for repo-independent discovery events) — send them with .broadcast() or .to(relays) so they don't pay for gossip resolution and don't silently depend on the sender's NIP-65 list being fresh.
  • Anything that should use gossip once it exists (e.g. a future NIP-17 DM, or explicit NIP-65 profile publishing) keeps the default routing, or calls .to_nip17()/.to_nip65() explicitly.

This is a small, additive change (one .broadcast()/.to(...) call per send site as part of the §2 consolidation), not a removal — do it while touching each call site for the send-path cleanup below, so gossip stays fully available for the features that are meant to use it, while today's repo/issue/PR/patch traffic stays deterministic about where it goes.

5. signed_git vs gix — split verdict, not "throw it all out"

crates/signed_git/src/lib.rs is 4118 lines. Checked the actual gix version pinned (gix = "0.87.1", feature set in the root Cargo.toml) and its gix-diff 0.67.1 dependency against what signed_git hand-rolls.

Already correct, idiomatic gix usage — keep as-is

tree_diff (signed_git/src/lib.rs:1468-1583) generates commit-to-commit diffs by calling repo.diff_tree_to_tree(...), then gix::diff::blob::diff_with_slider_heuristics(...), then feeding the result through gix::diff::blob::UnifiedDiff::new(&diff, &input, collector, ..) where collector implements gix's own ConsumeHunk trait (signed_git/src/lib.rs:1963-2027, matching gix-diff-0.67.1/src/blob/unified_diff/mod.rs:70-84 exactly). This is the documented, intended way to consume gix's diff engine — there is no simpler API to fall back to, and no unnecessary reimplementation here. Same for the porcelain wrappers around gix::Repository for refs, branches, tags, worktree checkout, etc. — that's inherent surface area for a git-porcelain layer, not bloat.

Real duplication — the git format-patch text parser

The other ~700 lines (parse_diff_section, parse_hunk, hunk_header, header_paths, diff_line_path, take_quoted, unquote_path, strip_patch_prefix, name_from_address, signed_git/src/lib.rs:1660-2027) are a hand-rolled parser for already-rendered git format-patch/unified diff text — this is necessary because a NIP-34 patch event's content is the raw text output of git format-patch, arriving over Nostr with no backing git objects to hand to gix's diff engine. gix-diff only generates unified diffs from git objects; it has no facility to parse unified-diff text back into structured hunks, so this isn't a case of "gix already does this and we reimplemented it."

However, a maintained crate already exists for exactly this parsing job: diffy's PatchSet module (diffy::patch_set::PatchSet::parse(text, ParseOptions::gitdiff())) explicitly parses "the output of git diff or git format-patch", supporting diff --git headers, extended headers (new file mode, deleted file mode, etc.), rename/copy detection via rename from/rename to/copy from/copy to, and binary-file detection — i.e., the exact feature list signed_git's hand-rolled parser reimplements (FileDiff::status has Renamed/Copied/Added/Deleted/Modified variants, signed_git/src/lib.rs:1373-1379; binary detection at signed_git/src/lib.rs:1395).

Recommendation: spike replacing patch_diffs/parse_diff_section/ parse_hunk/unquote_path/etc. with diffy::patch_set::PatchSet, mapping its FileOperation/Hunk types onto this codebase's existing FileDiff/ DiffHunk (which downstream UI code already depends on, so keep those public types and only replace the parsing internals). This is the single biggest concrete size reduction available in the whole backend — a ~700 line hand-rolled parser (plus ~1300 lines of tests for it, signed_git/src/lib.rs:3681-4053 and surrounding) collapses to a thin adapter over a well-tested crate. Budget a spike first: diffy's renamed path handling and quoted-path unescaping need to be checked against this project's test fixtures (signed_git/src/lib.rs:3917-3962, octal-escaped/non-ASCII quoted paths) before committing to the swap.

6. Remove the tasks: Vec<Task<...>> + push_task boilerplate — use Task::detach()

Verified against the actual pinned GPUI revision (~/.cargo/git/checkouts/zed-a70e2ad075855582/1870e26/crates/scheduler/src/executor.rs:375-573 and crates/gpui/src/executor.rs:32-63).

Six different stores carry the exact same field and method, copy-pasted:

tasks: Vec<Task<Result<(), Error>>>,

fn push_task(&mut self, task: Task<Result<(), Error>>) {
    self.tasks.retain(|task| !task.is_ready());
    self.tasks.push(task);
}

at backend.rs:87-91,164-169, checkouts.rs:106-110,180-185, local_repos.rs:13-23, profile.rs:72-80,136-141, repo.rs:82-86 (plus an inlined copy of the same retain-then-push at repo.rs:236-240 and repo.rs:373-377), and repo_list.rs:56-60,137-142.

Task's own doc comment (scheduler/src/executor.rs:375-380) says exactly what this boilerplate exists to avoid: "If you drop a task it will be cancelled immediately. Calling Task::detach allows the task to continue running, but with no way to return a value." Task::detach(self) (executor.rs:552-559) does precisely that, and TaskExt::detach_and_log_err (gpui/src/executor.rs:35-61, already referenced in this project's own .rules file) additionally logs an Err without any manual match. None of these stores' spawned tasks need cancel-on-drop semantics: every continuation already does this.update(cx, ...).ok() or propagates through ?, so if the owning entity is gone by the time the task finishes, the update is a harmless no-op — exactly the "tolerate the entity being gone" pattern already used everywhere in this codebase (see the .ok() calls throughout backend.rs). Storing the task and pruning it on every push buys nothing here; .detach() (or .detach_and_log_err(cx) where the continuation only logs on failure) replaces both the field and the method:

// today
self.push_task(cx.spawn(async move |this, cx| {
    if let Err(e) = task.await {
        this.update(cx, |_this, cx| cx.emit(BackendEvent::error(e.to_string()))).ok();
    }
    Ok(())
}));

// replacement — no field, no prune, no manual match
cx.spawn(async move |this, cx| {
    if let Err(e) = task.await {
        this.update(cx, |_this, cx| cx.emit(BackendEvent::error(e.to_string()))).ok();
    }
})
.detach();

Delete the tasks field and push_task method from all six stores, and change every self.push_task(cx.spawn(...)) call to cx.spawn(...).detach() (or .detach_and_log_err(cx) when the closure's only job is to log the error). The one place that must not just detach is push_repo_from's returned Task<Result<PushOutcome, Error>> (backend.rs:815-902) — that task is deliberately returned to the UI caller (so the panel can .await it and show a spinner) and already isn't stored in a tasks list today, so it's unaffected by this cleanup.

crates/workspace has the same pattern too, and there it's a real bug, not just style — see §14.

7. Render granularity / cx.notify() audit

Checked every cx.notify() call in signed_state (18 call sites) and how workspace views consume each store. Overall this is already well-partitioned, not a smell:

  • Every panel (IssuesView, PullRequestsView, IssueDetailView, CommitDiffView, RepoDetailView, PullRequestDetailView, NewPullRequestView, DiffPane) is its own Entity<T>/Render impl — cx.notify() on a store only invalidates the views actually observing that store's Entity, not a monolithic root view.
  • IssuesView/PullRequestsView already memoize derived rows behind a (store.version(), filter) cache key (issues.rs:68-72, 323-333; pull_requests.rs:75-79, 331-341), and both use VirtualListScrollHandle for virtualization — so a store notify() doesn't force rebuilding or laying out off-screen rows.
  • sync_bootstrap's per-percent progress cx.notify() (backend.rs:1257-1264) is already throttled to distinct percentage points (if progress.current > 0 && percent != last_percent, backend.rs:1254), and nothing in workspace reads Backend::sync_progress() directly (grep -rn "sync_progress()" crates/workspace → no matches), so this never drives a visible re-render on its own.

One real waste found: RepoListStore re-queries the DB on every sync tick

RepoListStore's backend subscription (repo_list.rs:76-109) treats BackendEvent::SyncProgress { .. } as relevant on its own:

BackendEvent::Synced | BackendEvent::SyncProgress { .. } => true,

Every distinct percentage tick of the bootstrap announcements/deletions sync calls this.refresh(cx), which is debounced 300ms (REFRESH_DEBOUNCE, repo_list.rs:16) and coalesced by RefreshGate — so it's not literally one DB round-trip per percent, but it is several (bounded by sync duration / 300ms) full re-scans of announcements + deletions + state events + activity + counts (run_refresh, repo_list.rs:184-294) while a single sync is still in flight, instead of one at the end. This is a deliberate trade-off for progressive reveal (the repo list fills in live instead of jumping once at 100%), so it's not a bug, but if that progressive reveal isn't a feature you actually want, dropping SyncProgress from the "relevant" match (keep only Synced) removes several redundant background-thread DB scans per sync for free. Worth a product decision, not just a code fix.

No other store subscribes to SyncProgress (RepoStore, CheckoutsStore do not — checked their subscription callbacks), so this is fully isolated to RepoListStore.

8. Relay add/connect: stop round-tripping through strings, stop reconnecting the whole pool

Flagged example (backend.rs:1071-1074):

for url in latest_grasp_list_servers(events) {
    client.add_relay(url.as_str()).await.ok();
}
client.connect().await;

Two separate problems, both verified against nostr-sdk/src/client/url.rs:40-50 and nostr-sdk/src/client/api/connect.rs:1-49:

  1. .as_str() is a pointless round trip. latest_grasp_list_servers already returns RelayUrl values (parsed, validated). RelayUrlArg (what add_relay actually accepts) has a direct impl From<RelayUrl> and impl From<&RelayUrl> (client/url.rs:40-50) — passing the RelayUrl itself skips a second RelayUrl::parse that .as_str() forces (client/url.rs:26,35, the String variant of RelayUrlArg re-parses on try_into_relay_url). Just pass url, not url.as_str().
  2. client.connect() connects every relay in the pool, not just the one you added. Verified in connect.rs:36-48: Client::connect()'s IntoFuture unconditionally calls self.client.pool().connect(), with no target selection at all — it iterates every relay currently in the pool. Calling it after adding 1-2 new relays re-issues a connect attempt to every relay already connected too. The AddRelay builder already has the right primitive: .and_connect() (client/api/add.rs:127-132), which is threaded straight into pool.add_relay(url, capabilities, connect, opts). Verified in pool/mod.rs:157-197 that this is correct even when the relay already exists in the pool: the pool's add_relay checks for an existing entry and, if connect is true, calls relay.connect() on the existing relay too (pool/mod.rs:191-194) — so .and_connect() is never wrong to use, whether the relay is new or already known.
// replacement
for url in latest_grasp_list_servers(events) {
    client.add_relay(url).and_connect().await.ok();
}

The same two problems repeat at every other relay-add call site — fix all of them the same way:

  • Backend::bootstrap (backend.rs:178-187): the BOOTSTRAP_RELAYS loop and the INDEXER_RELAYS loop (which also sets .capabilities(...), chain .and_connect() onto the same builder) both currently defer to one trailing client.connect().await.
  • connect_repo_relays (backend.rs:1446-1449): today calls client.add_relay(url).await?; then client.connect_relay(url).await?; as two separate round trips — collapse to one client.add_relay(url).and_connect().await?;.
  • stage_event_on_relay (backend.rs:1772-1782): same fix, and this one currently calls the pool-wide client.connect().await just to connect the single relay it's about to stage an event on.

Delete the add_relays wrapper (Backend::add_relays, backend.rs:1149-1173)

Its only two callers (create_repository, backend.rs:505-508; publish_local_repo, backend.rs:652-655) do this today:

this.update(cx, |this, cx| {
    let urls: Vec<String> = servers.iter().map(ToString::to_string).collect();
    this.add_relays(urls, cx);
})?;

servers is already Vec<RelayUrl> at both call sites — stringifying it only to have add_relays parse it straight back into RelayUrl inside client.add_relay(&url) is pure waste, on top of the wrapper itself being another cx.spawn + push_task + error-emit layer (§6) around what is, with the fix above, a two-line loop. Both call sites are already inside a cx.spawn(async move |this, cx| ...) with client reachable — inline it:

let client = this.update(cx, |this, _cx| this.client.clone())?;
for relay in &servers {
    client.add_relay(relay).and_connect().await.ok();
}

Delete Backend::add_relays entirely once both call sites are inlined.

9. create_repository's flow is backwards: it inits a mirror, then clones it into the real destination

This is a real business-logic flaw, not just a style issue. Today (backend.rs:437-501):

  1. signed_git::init_repository(&path, &name, &description) — path is GitCache::repo_path(&addr), the app's internal mirror cache location (crates/signed_git/src/lib.rs:29-33), not anywhere the user asked for. This creates a full worktree with an initial commit there.
  2. signed_git::clone_repo(&[mirror_url], &destination) — destination is folder.join(dir_name), the folder the user actually picked. This clones the mirror just created in step 1 into the real target, via a file:// URL (Url::from_file_path(&path), backend.rs:481-483).
  3. The push (push_staged_to_grasps, called with path = the mirror, not destination) pushes the mirror's objects to the grasp servers.
  4. origin gets set on both the mirror (backend.rs:463-466) and the destination (backend.rs:492-495).

So a brand-new repository gets initialized twice and checked out twice for what is, at that point, one README and one commit — and the thing that actually gets pushed (the mirror) isn't the thing the user is left looking at (the destination).

Checked signed_git::init_repository itself (signed_git/src/lib.rs:401-477): it already creates the target directory (std::fs::create_dir_all(path)), runs gix::init(path), and leaves a fully checked-out worktree with the README written to disk and the index populated — i.e., it already produces exactly what step 2's clone is redundantly reproducing. There is no reason step 1 and step 2 are two different paths.

Compare with publish_local_repo (backend.rs:599-754), the sibling flow for an existing local repo: it operates on the user's real folder directly (signed_git::worktree_ref_state(&path), root_commit(&path)) — no mirror, no extra clone. create_repository is the odd one out.

The mirror doesn't need to be pre-populated at creation time at all. GitCache::ensure_clone(addr, clone_urls) (signed_git/src/lib.rs:45-63) already exists precisely to populate the mirror lazily — open it if it's there, clone it from the announcement's clone_urls if it's not — and it's already what RepoDetailView::load_repo calls for every repo, including the user's own (workspace/src/views/repo_detail/mod.rs:425-428). By the time the UI navigates to the new repo's detail view after create_repository returns, the push has already succeeded, so ensure_clone will clone straight from the just-pushed grasp server — exactly the same lazy path every other repo already takes. No special casing needed.

I checked whether any workspace call site compounds this (e.g. by cloning again right after create_repository returns) — it doesn't: sidebar/create_repo_dialog.rs's create_repository handler (create_repo_dialog.rs:193-222) just calls backend.create_repository(...) and applies the returned Announcement; the flaw is fully contained inside Backend::create_repository itself.

Replacement: initialize directly at destination, push from destination, set origin once:

let commit = signed_git::init_repository(&destination, &name, &description)?;
// ... build the announcement using `commit` as before ...
// push_staged_to_grasps(..., path = &destination, ...) instead of the mirror path
if let Some(base) = servers.first().and_then(grasp_base_url) {
    signed_git::set_origin(&destination, &format!("{base}/{owner}/{repo_id}.git"))?;
}

Delete the mirror init_repository call, the clone_repo call, the Url::from_file_path mirror-URL construction, and the mirror-side ensure_origin call. This removes a full extra gix::init + checkout + clone from repo creation, and makes create_repository consistent with how publish_local_repo already treats the user's working copy as the one source of truth.

10. Bootstrap-on-construction should go through cx.defer, not run synchronously in new

Verified against the pinned GPUI revision (crates/gpui/src/app.rs:1999-2005, crates/gpui/src/app/context.rs:296-315).

App::defer(&mut self, f: impl FnOnce(&mut App) + 'static) — "Schedules the given function to be run at the end of the current effect cycle, allowing entities that are currently on the stack to be returned to the app." That's precisely the situation every one of these constructors is in: Self is still being built inside the cx.new(|cx| ...) closure when it reaches out and kicks off real work. Context<T>::defer_in also exists (app/context.rs:296-315) but takes a &Window — it's for window-bound views, not the headless global stores below, none of which are constructed with a Window in scope. For these, the applicable API is the window-less cx.defer(...), reached through Context<T>'s Deref<Target = App> (app/context.rs:25-34), capturing a WeakEntity<Self> to get back into Self once deferred:

// today, backend.rs:148-160
let mut this = Self { /* ... */ };
this.bootstrap(cx);
this

// replacement
let mut this = Self { /* ... */ };
let weak = cx.entity().downgrade();
cx.defer(move |cx| {
    weak.update(cx, |this, cx| this.bootstrap(cx)).ok();
});
this

The same pattern — a constructor that calls its own bootstrap-ish method, or reaches into another entity, before returning Self — repeats in every store:

Store Constructor call site What it kicks off synchronously
Backend backend.rs:159 bootstrap(cx) — adds/connects BOOTSTRAP_RELAYS/INDEXER_RELAYS, restores the session
RepoListStore repo_list.rs:120-123 subscribe_remote(cx) (negentropy sync against bootstrap relays) + refresh_initial(cx)
RepoStore repo.rs:154-159 subscribe_remote, connect_announced_relays, refresh — each one reaches into the global Backend entity
CheckoutsStore checkouts.rs:172-174 refresh(cx)
LocalReposStore local_repos.rs:44 rescan(cx)
ProfileStore profile.rs:119-121 spawns the batched profile-fetch loop

Wrap each of these the same way Backend::new is shown above. This isn't about a currently-observed crash (nothing panics today, because everything past the initial synchronous field assignment already goes through cx.spawn/cx.background_spawn, which only runs later anyway) — it's about not mixing "construct plain state" with "kick off side effects that talk to other entities" in the same synchronous call, which is exactly what defer exists to separate, per its own doc comment.

11. Split independently-observed state into child entities

Backend::pushing_repos (backend.rs:88) is Arc<Mutex<HashSet<RepoAddr>>> — it bypasses GPUI's entity system entirely. A view that wants to show "is repository X currently pushing" has no way to cx.observe this; it can only poll a Mutex by hand, and any UI update requires some other notify to happen to piggyback on. Meanwhile every view that only cares about, say, current_user still gets re-invoked on Backend::notify() fired for unrelated reasons (a sync_progress tick, a new relay connecting), because the whole Backend is one entity and cx.notify() invalidates all of its observers indiscriminately.

GPUI's own model is built for exactly this split: an Entity<T> works for any T: 'static, not just Render-able view state (see the project's own GPUI notes: "Whenever you need to store application state that communicates between different parts of your application, you'll want to use GPUI's entities"). Where a piece of a bigger store's state changes on its own schedule and has its own, narrower set of observers, pull it out into a child entity:

pub struct Backend {
    client: Client,
    signer: UniversalSigner,
    current_user: Option<PublicKey>,
    sync_progress: Option<(u64, u64)>,
    passphrase_required: bool,
    pushing_repos: Entity<HashSet<RepoAddr>>, // was Arc<Mutex<HashSet<RepoAddr>>>
}

A view that only cares whether repo X is pushing does cx.observe(&backend.read(cx).pushing_repos, |this, pushing, cx| ...) and is left alone by every other Backend change. PushGuard (backend.rs:99-110) becomes a guard that calls pushing_repos.update(cx, |set, cx| { set.remove(&addr); cx.notify(); }) on drop instead of locking a raw Mutex — same RAII shape, but now it's a real, observable GPUI entity instead of a side channel next to the entity system. Apply the same split to any other Backend/store field where the set of interested observers is a strict subset of the store's full observer list.

This principle is also the reason not to merge LocalReposStore and RepoListStore into one entity — see §13.

12. One debounce at the source, not one per store

Flagged example — the notification pump (backend.rs:126-146):

let mut notifications = pump_client.notifications();
while let Some(notification) = notifications.next().await {
    let ClientNotification::Event { event, .. } = notification else { continue };
    let update = Update::from_event(&event);
    if this.update(cx, |_, cx| cx.emit(BackendEvent::NostrUpdate(update))).is_err() {
        break;
    }
}

Every single relay-delivered event is emitted as its own BackendEvent::NostrUpdate, immediately. During a negentropy sync (exactly the bursty case §6/§7 already discuss), this can be hundreds of emits in a short window. Four different stores (RepoStore, RepoListStore, CheckoutsStore, and transitively ProfileStore) each subscribe to Backend and independently run their own RefreshGate debounce/coalesce dance in response — the same burst gets debounced four times, once per listener, instead of once at the point it actually enters the system.

Centralize it: batch what the pump itself emits, and let each store react to a batch instead of a stream of singles. The pump already owns the one place where the burst originates, so it's the natural place to coalesce:

let pump = cx.spawn(async move |this, cx| {
    let mut notifications = pump_client.notifications();
    let mut pending: Vec<Update> = Vec::new();

    loop {
        let next = cx.background_executor().timer(PUMP_DEBOUNCE).fuse();
        futures::select_biased! {
            notification = notifications.next() => {
                let Some(notification) = notification else { break };
                let ClientNotification::Event { event, .. } = notification else { continue };
                pending.push(Update::from_event(&event));
            }
            _ = next => {
                if pending.is_empty() { continue; }
                let batch = std::mem::take(&mut pending);
                if this.update(cx, |_, cx| cx.emit(BackendEvent::NostrUpdate(batch))).is_err() {
                    break;
                }
            }
        }
    }
    Ok(())
});

(Sketch — the real version needs BackendEvent::NostrUpdate to carry Vec<Update> instead of Update, and every subscriber's relevance check — RepoStore, RepoListStore, CheckoutsStore, ProfileStore — to check "does any update in the batch match" instead of one Update. That's a mechanical change to four match arms.)

This doesn't make each store's own RefreshGate fully redundant: Published/Synced/SyncProgress events are emitted directly by whichever method triggered them (a local send, a sync completing), not through the pump, and can still arrive close together independently of relay traffic. But those are one-off, user-triggered events, not the hundred-events-in-a-burst case — so once the pump absorbs the dominant source of bursts, each store's debounce window can likely shrink significantly (or, for stores that only ever see one trigger at a time in practice, be dropped in favor of "fold into the in-flight run" without a timer at all). Worth measuring after the pump-side batching lands, rather than speculatively resizing four timers up front.

13. local_repos.rs + repo_list.rs: merge the files, not the entities

These two are structurally near-identical: both hold an Arc<Vec<T>> snapshot, refresh it in the background on a trigger, swap it in with cx.notify(), and carry their own Global wrapper + global()/set_global() pair + tasks/push_task boilerplate (§6). That similarity is real and worth collapsing — but checked who actually reads each one before deciding how:

grep -rn "RepoListStore::global" crates/     → 9 call sites
grep -rn "LocalReposStore::global" crates/    → 5 call sites

Only two places read both together: CheckoutsStore::new/run_refresh (checkouts.rs:126-136, checkouts.rs:339-344) and SidebarPanel::new/refresh (sidebar/mod.rs:55-65, sidebar/mod.rs:128-142). Everywhere else reads exactly one:

  • RepoListStore alone: RepoStore::action_announcement (repo.rs:1071-1078), RepoDetailView::open_upstream (×2, mod.rs:1009-1013, 1034-1044), RepoDetailView::fork_row (mod.rs:2596-2606), NewPullRequestView::fork_candidates (new_pull_request.rs:569-577), RepoListView::new (views/repo_list.rs:111-121).
  • LocalReposStore alone: RepoDetailView::apply_announcement (mod.rs:1875-1877), SidebarPanel::render_repos's rescan button (sidebar/mod.rs:306-309).

Given that, collapsing them into one Entity (one struct holding both Vecs, one cx.notify() for both) would make every one of those ~12 single-store readers pay for the other store's unrelated refreshes — exactly what §11 says not to do. Wrapping them in a parent that holds two child entities (RepoDirectory { local: Entity<LocalRepos>, remote: Entity<RemoteRepos> }) avoids that specific problem, but then every one of those same ~12 call sites has to change from RepoListStore::global(cx) to RepoDirectory::global(cx).read(cx).remote — an extra hop added everywhere, in exchange for saving exactly one Global wrapper struct. Not a good trade for a codebase this size.

Recommendation: merge the two files into one module (e.g. repos.rs), keeping LocalReposStore and RepoListStore as two fully independent structs, each still its own Entity/Global exactly as today — same public API, same global()/set_global() pairs, zero call-site churn. The merge is justified purely as "these are the app's two repo-listing stores, they belong next to each other," per the project's own .rules guidance to avoid many small files for closely related logic — not as a reason to share a notify cycle between two things with almost entirely disjoint observers.

14. crates/workspace has the same task-list pattern as §6 — and there it's an actual bug

§6 covers signed_state's 6 stores, where the unpruned-Vec<Task> pattern is a style/complexity concern with no observed failure, because push_task always pruned before pushing. crates/workspace has the exact same field-and-push shape in 4 views, but most of it never prunes:

grep -rn "tasks.push(task)" crates/workspace/   → 17 call sites
grep -rn "tasks.retain"      crates/workspace/   → 1 call site (pull_request_detail.rs:258)
  • RepoDetailView.tasks (mod.rs:178-179, doc comment: "finished tasks are pruned on every push" — this is stale/incorrect, no .retain() precedes any of its 11 push sites: mod.rs:384-388, 532-536, 650-654, 773-777, 835-839, 877-881, 949-953, 1061-1065, 1132-1136, 1219-1223, 1315-1319).
  • NewPullRequestView.tasks (new_pull_request.rs:80-84): 5 push sites, none pruned (445-449, 475-479, 697-701, 894-898, 997-1001).
  • CommitDiffView (diff.rs:407-411): 1 push site, not pruned.
  • PullRequestDetailView.tasks (pull_request_detail.rs:68-72): the one correct one — load (pull_request_detail.rs:256-260) does self.tasks.retain(|task| !task.is_ready()); self.tasks.push(task);.

So RepoDetailView.tasks and NewPullRequestView.tasks grow unbounded for as long as the panel stays open: every file preview, ref switch, commit load, worktree reload, or fork comparison appends one more Task that is never removed. This is a real memory-growth bug, not just a style preference — a repo detail panel left open through a long session accumulates one Task per interaction, forever.

Apply the same fix as §6: delete the tasks field from all four views and .detach() (or .detach_and_log_err(cx)) at every one of the 17 call sites. Every continuation already tolerates the view being gone (this.update_in(cx, ...).ok()/?, same pattern as signed_state), so nothing here needs cancel-on-drop semantics either. Worth noting repo_detail/init_dialog.rs's init_repository (init_dialog.rs:187-206) already does exactly this — cx.spawn(...).detach(), no task list at all — so the fix is bringing the other 4 views in line with a pattern that already exists once in the same crate.

15. Vec<Url> → Vec<String> conversion sprawl — fix the 3 signed_git signatures, not the 8 call sites

Announcement::clone is Vec<Url> (signed_core/src/model.rs, Url being nostr's re-export of the url crate's Url, nostr/src/types/url.rs:15, pub use url::*;). Every call site that needs to hand those URLs to signed_git first stringifies them:

grep -rn "\.map(ToString::to_string)\.collect" crates/signed_state crates/workspace

finds it at repo.rs:1005-1008 (merge_pull_request), repo.rs:1227 (clone_to_folder), workspace/repo_detail/mod.rs:397 (load_repo), new_pull_request.rs:595 and 602 (choose_fork, twice — once for the fork, once for the base), and pull_request_detail.rs:146-149 and 738-741 (load, clone_urls_of). Seven call sites, all producing a Vec<String> that gets handed straight to signed_git::clone_repo, GitCache::ensure_clone, or fetch_repo_refs.

The root cause is those three functions' signatures, not the call sites. Verified in signed_git/src/lib.rs:

pub fn clone_repo(clone_urls: &[String], path: &Path) -> Result<()> { ... }        // lib.rs:125
pub fn ensure_clone(&self, addr: &RepoAddr, clone_urls: &[String]) -> Result<...>  // lib.rs:45
pub fn fetch_repo_refs(repo_path: &Path, urls: &[String], refspec: &str) -> ...   // lib.rs:701

all three only ever read each URL as &str internally, through the shared try_each_url(urls: &[String], ...) helper (lib.rs:350), which does attempt(url) where url: &String auto-derefs. Checked whether Url could be passed directly instead of allocating a String per URL: yes — url::Url implements AsRef<str> directly (verified in the pinned url crate source, url-2.5.8/src/lib.rs:2867). Making the three functions generic removes the conversion at every call site instead of patching each one:

fn try_each_url<U: AsRef<str>, F>(urls: &[U], verb: &str, mut attempt: F) -> Result<()>
where
    F: FnMut(&str) -> Result<()>,
{
    for url in urls {
        match attempt(url.as_ref()) { /* ... */ }
    }
    /* ... */
}

pub fn clone_repo<U: AsRef<str>>(clone_urls: &[U], path: &Path) -> Result<()> { ... }
pub fn ensure_clone<U: AsRef<str>>(&self, addr: &RepoAddr, clone_urls: &[U]) -> Result<gix::Repository> { ... }
pub fn fetch_repo_refs<U: AsRef<str>>(repo_path: &Path, urls: &[U], refspec: &str) -> Result<()> { ... }

After this, every one of the 7 call sites above passes &announcement.clone directly (a &[Url]), deleting the .iter().map(ToString::to_string).collect::<Vec<String>>() line entirely — no allocation, no Display-then-reparse round trip, 7 fewer lines of boilerplate for free. (about.rs's url.to_string() calls for on-screen display, about.rs:55-103, are unrelated — that's genuine Url → SharedString rendering, not a signed_git call, and stays as-is.)

The Vec<RelayUrl> → Vec<String> conversions for add_relays/add_relay (§8) are a separate root cause (RelayUrl doesn't implement AsRef<str>, checked nostr/src/types/url.rs) and are already fixed by §8's move to RelayUrlArg's native From<RelayUrl>/From<&RelayUrl> — no further change needed there.

16. .clone() audit: the dense clusters in backend.rs are the correct idiom, not a flaw

Went through every .clone() in create_repository, publish_local_repo, and push_repo_from (the three functions with the highest clone density) looking for copies that could be replaced by a reference. All of them are Client/UniversalSigner/PathBuf/String/RelayUrl values being moved into a separate 'static async move block for cx.background_spawn, which Rust's ownership rules require to own its captures — this is exactly the shadowing-clone pattern the project's own .rules file endorses ("Use variable shadowing to scope clones in async contexts for clarity, minimizing the lifetime of borrowed references"). Client itself is a cheap Arc handle clone (Client(Arc<InnerClient>), verified nostr-sdk/src/client/mod.rs:74), so even the frequent client.clone()/signer.clone() pairs before each background_spawn are not doing a deep copy. No changes recommended here — noting this so it's clear the dense clone clusters were checked, not skipped, and found to be inherent to the async-boundary structure rather than avoidable duplication.

17. Business logic that leaked into crates/workspace and should move to signed_core/signed_state

Direct answer to "can the view side be thinner": yes, and not speculatively — found one confirmed duplicate, one cluster of misplaced domain parsing, and one mutating-flow split across the view/store boundary. The test used to tell "fine to stay in the view" from "should move": read-only git/data queries that only shape what one specific view renders (diffs, commit lists, tree snapshots — already audited clean in §5/§7) are fine where they are; anything that parses a Nostr event's domain tags, decides what's NIP-34-valid/eligible, or builds the payload of a mutating operation is domain logic and belongs in signed_core/signed_state, reusable and testable without GPUI.

Confirmed duplicate: current_commit_of

signed_core/src/model.rs:183-190 (private, used internally by pull_request_patches) and workspace/repo_detail/pull_request_detail.rs:709-716 are the same function, byte-for-byte:

fn current_commit_of(event: &Event) -> Option<String> {
    event
        .tags
        .iter()
        .find_map(|tag| match Nip34Tag::parse(tag.as_slice()) {
            Ok(Nip34Tag::CurrentCommit(commit)) => Some(commit.to_string()),
            _ => None,
        })
}

It was reimplemented in workspace because signed_core's copy is private. Fix: make signed_core's current_commit_of pub fn, delete workspace's copy, import the shared one.

A whole cluster of NIP-34 tag parsing lives next to it, same shape, same problem

Still in pull_request_detail.rs, zero GPUI/UI dependency in any of them:

  • merge_base_of(event: &Event) -> Option<String> (pull_request_detail.rs:721-729)
  • clone_urls_of(event: &Event) -> Option<Vec<String>> (pull_request_detail.rs:734-742)
  • branch_name_of(event: &Event) -> Option<String> (pull_request_detail.rs:745-753)
  • latest_update<'a>(events: impl Iterator<Item = &'a Event>, root: &Event) -> Option<&'a Event> (pull_request_detail.rs:756-766) — walks a PR's GitPullRequestUpdate events to find the newest revision from the root's author, the exact same shape of problem signed_core::model::pull_request_patches already solves for patch series (model.rs:95-135, forward/backward reply-chain walking).

These all take a plain &Event (or an iterator of them) and return plain data — nothing here needs Context/Window/cx. They belong next to Announcement::from_event, parse_state, and pull_request_patches in signed_core, as pub fns with their own unit tests (this file's test module, pull_request_detail.rs:840+, already builds fixture events with a local signed()/pr_root() helper — signed_core's test module has the same fixture-building pattern already; the tests move with the functions, no new test infrastructure needed).

A mutating flow split across the view/store boundary: patch generation in submit

NewPullRequestView::submit (new_pull_request.rs:900-1000) does this before calling into the store:

let patch = cx.background_spawn({
    /* ... */
    async move { format_patch_between(Path::new(&repo_path), &merge_base, &compare_ref) }
}).await;

let patch = match patch {
    Ok(patch) if !patch.is_empty() => patch,
    Ok(_) => { /* "No commits between the branches to propose" */ return Ok(()); }
    Err(error) => { /* "Failed to generate the patch: {error}" */ return Ok(()); }
};

store.update(cx, |store, cx| {
    store.open_pull_request(/* subject, description, branch_name, patch, ... */)
});

RepoStore::open_pull_request (repo.rs:554-558) and update_pull_request (repo.rs:829-833) both already take a ready-made patch: String — a reasonable, uniform boundary in general (it's also exactly right for pull_request_detail.rs's "update PR" dialog, pull_request_detail.rs:648-700, where the patch is literally pasted by the user into a textarea, no git involved). But for the "compare two branches" flow, generating that patch text — calling signed_git::format_patch_between, deciding empty-diff is an error, and wording that error — is exactly the same kind of "turn git state into the payload of a Nostr publish" work Backend::create_repository/ publish_local_repo already do internally (worktree_ref_state, root_commit), just for a different event kind. It shouldn't be the one case where that responsibility sits in the view instead of the store.

Recommendation: give RepoStore (or a free function in signed_state it calls) a method that takes the two refs instead of a ready-made patch, e.g. RepoStore::open_pull_request_from_refs(repo_path, base_ref, compare_ref, subject, description, draft, cx) -> Task<Result<(), Error>>, which does the format_patch_between + empty-check + open_pull_request sequence internally and returns one descriptive error on failure. submit shrinks to gathering the text-field values and calling it, then closing the panel — no signed_git import needed in new_pull_request.rs at all for this path.

Borderline, worth doing while touching the same file: fork_candidates/fork_namespace

fork_candidates (new_pull_request.rs:117-135) filters/partitions &[Announcement] into "own" vs. "others" fork sources using the already-domain Announcement::is_fork_of predicate (signed_core/src/model.rs:281-285, correctly reused, not reimplemented) — it's pure data transformation with no GPUI dependency, and has its own private unit tests in new_pull_request.rs building fixture announcements, again duplicating test-fixture machinery signed_core's own test module already has. fork_namespace (new_pull_request.rs:108-114, formats the refs/fork/<owner>/<id> namespace string) is the same shape — small, but it's the one place that convention is decided, and it pairs naturally with signed_git's ref-naming conventions. Both are safe, low-risk moves to signed_core: unlike fork_display_name/shorten_owner/truncate_label/the *_source_item builders in the same file (genuine presentation logic — SharedString truncation, PopupMenuItem construction — correctly left where they are), these two don't touch a single GPUI type.

What's already thin and should stay exactly where it is

For contrast, checked RepoDetailView's git-touching methods (load_repo, load_commits, switch_ref, reload_worktree, catch_up_worktree, push_unpushed_checkout) and NewPullRequestView::reload_compare (new_pull_request.rs:808-897, computing merge_base/commit list/diff purely to populate the compare pane): these call signed_git directly too, but only to compute read-only data this one view renders — nothing here is parsed from a Nostr event, decides NIP-34 eligibility, or builds a publish payload. Moving these into signed_state would just add an indirection layer with no reuse benefit, contradicting "keep it simple." Same verdict as create_repo_dialog.rs's and init_dialog.rs's handlers (§9): they already do nothing but gather form input and call one Backend method.

Action plan, in order of risk/reward

  1. Delete the fetch/sync dedup cache (§3). Pure removal, no behavior change for the intended usage pattern (each call site already has, or trivially gets, its own guard). Lowest risk, do first.
  2. Remove the tasks: Vec<Task<...>> + push_task boilerplate, in both signed_state (§6) and crates/workspace (§14), in favor of .detach()/.detach_and_log_err(cx). Independent of every other change here, touches 10 files, all mechanical — and fixes a real unbounded-growth bug in RepoDetailView/NewPullRequestView along the way.
  3. Fix the relay add/connect calls (§8): drop .as_str()/ToString round trips, replace add_relay + blanket client.connect()/ connect_relay pairs with add_relay(url).and_connect(), and delete Backend::add_relays. Mechanical, no behavior change beyond "connect only what was just added."
  4. Fix bootstrap_user to sync+query instead of fetch_events (§1). One function, fully covered by existing tests for latest_grasp_list_servers.
  5. Generalize the 3 signed_git URL-list signatures to &[impl AsRef<str>] (§15), then delete the now-redundant .map(ToString::to_string).collect() at all 7 call sites. Self-contained to signed_git's public API plus a one-line change per call site; re-run signed_git's existing tests (clone_repo/fetch_repo_refs already have coverage).
  6. Fix create_repository's init/clone ordering (§9): initialize and push directly at the user's chosen destination, drop the mirror pre-population entirely and let ensure_clone populate it lazily like every other repo. Self-contained to one function; verify against this crate's existing init_repository/push tests plus a manual create-repository-then-open-detail-view pass.
  7. Merge local_repos.rs and repo_list.rs into one file (§13), keeping both stores as independent entities. Purely organizational, zero call-site changes, safe to do any time.
  8. Route construction-time bootstrap through cx.defer (§10) in all six stores listed there. Mechanical per store, but touch them one at a time and re-run each store's test suite, since ordering-sensitive assumptions (e.g. a test that asserts state right after cx.new) may need cx.run_until_parked() inserted where they didn't before.
  9. Consolidate the send paths (§2): introduce the single require_relay_accepted helper, delete Backend::send/publish_event/send_fire_and_forget/broadcast_event/ RepoStore::send, switch retract_events to EventDeletionRequest (one deletion event per target, no k tag), and make each send site explicit about bypassing gossip (§4) with .broadcast()/.to(relays). This is the biggest diff and touches every publish call site (repo.rs, backend.rs), so do it as its own PR with full test-suite coverage before/after.
  10. Split pushing_repos (and similar fields) into a child entity (§11). Small, isolated change once §9's PushGuard rewrite is in flight — do them together since both touch PushGuard.
  11. Centralize the notification-pump debounce (§12). This one is the most speculative of the batch — land it after §7's SyncProgress decision and re-measure whether each store's own RefreshGate window can shrink, rather than assuming the exact shape up front.
  12. Optional, product call: drop SyncProgress from RepoListStore's relevant-event match if progressive reveal during bootstrap sync isn't a feature you want (§7).
  13. Spike diffy::patch_set::PatchSet to replace signed_git's hand-rolled git format-patch parser (§5). Separate PR, separate crate, no interaction with the nostr-facing changes above — do this in parallel if you have a second contributor, otherwise last since it's the largest and riskiest single change (needs fixture-by-fixture verification against the existing test suite).
  14. Move the misplaced workspace domain logic to signed_core/signed_state (§17): make current_commit_of pub in signed_core and delete the workspace duplicate; move merge_base_of/clone_urls_of/branch_name_of/ latest_update and fork_candidates/fork_namespace there too, tests included; give RepoStore a refs-in-patch-out method so NewPullRequestView::submit stops calling format_patch_between itself. Low risk, no behavior change, best done as its own small PR per function cluster rather than one big move.

Everything not listed above (per-repo/per-list Entity stores, the RefreshGate debounce/coalesce pattern, Nip34Tag/Coordinate/Filter usage in signed_core, the GRASP push-retry state machine in push_staged_to_grasps, the UniversalSigner abstraction, and the dense .clone() clusters audited in §16) was checked and already matches "use the SDK directly, no unnecessary wrapper" — those should be left alone.

Reference in New Issue View Git Blame Copy Permalink