reya/signed
1
0
Fork 0
Code Issues Pull Requests 1 Actions 4 Releases Activity

refactor

Browse Source
This commit is contained in:
reya
2026-09-10 10:00:32 +07:00
parent 0e9f0a33ac
commit 68dbc5a731
13 changed files with 1065 additions and 720 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/backend-rearchitecture.md
+201 -6
View File
@@ -122,6 +122,41 @@ everywhere, no exceptions.
## 2. The "send an event" functions — there are 8, there should be roughly 2
> **Status: done.** `Backend::send`, `Backend::publish_event`, `Backend::publish_task`,
> `Backend::send_fire_and_forget` and the free fn `broadcast_event` are all
> deleted. In their place: `require_relay_accepted(output, event)` (the one
> "empty success ⇒ Err" check, `pub(crate)` so `repo.rs` can use it too) and
> `Backend::announce_published(event, cx)` (one line, emits
> `BackendEvent::Published`). Every call site now calls
> `client.send_event(&event).broadcast()` directly — the `.broadcast()` is
> the additive gossip-bypass from §4, added here since every one of these
> call sites already has a well-defined target (the relays this app
> explicitly added). `RepoStore::send` is also gone; its 4 identical
> one-shot callers (`open_issue`, `reply`, `set_status`,
> `publish_applied_status`) now call a private `RepoStore::publish` that
> does the same sign+send+check+`last_error` bookkeeping — kept as **one**
> small store-local helper rather than inlining the same ~20 lines 4 times,
> since all 4 call sites have byte-for-byte identical post-conditions (this
> is a deliberate, narrow exception to "delete `RepoStore::send`";
> `stage_event_on_relay` was already the same shape of exception before this
> change). The 3 call sites with genuinely divergent control flow
> (`open_pull_request`, `update_pull_request`, `publish_patch_series`) now
> call `client.send_event(...)`/`require_relay_accepted` directly inline,
> fixing the inconsistent error surfacing this section originally flagged
> (all three now set `last_error` on failure, like every other `RepoStore`
> mutation). `retract_events` is rewritten per the NIP-09 section below.
> `stage_event_on_relay` is untouched, it already followed this pattern.
> Verified against the pinned `nostr-sdk` source that `SendEventOutput`
> (`= Output<EventId, EventSendStatus, String>`) and `EventDeletionRequest`
> (`nostr/src/nips/nip09.rs`) have the shapes assumed here, and that
> `UniversalSigner` implements the `AsyncGetPublicKey + AsyncSignEvent`
> bounds `FinalizeEventAsync` requires. `cargo check --workspace`,
> `cargo clippy --workspace --all-targets` and `cargo test --workspace` all
> pass unchanged (165+ tests, no failures) — none of the deleted/rewritten
> functions had direct unit test coverage (they all require a live relay),
> so this was verified by compilation plus a careful line-by-line diff
> against the previous control flow for each of the 8 call sites.
Grep for anything that ends up calling `client.send_event`:
| Function | File:line | What it adds over `client.send_event` |
@@ -326,6 +361,13 @@ invariant, not something to paper over with a fingerprint cache.
## 4. Gossip is enabled, and stays enabled — but today's git-domain sends should bypass it explicitly
> **Status: done**, implemented as part of §2's send-path consolidation.
> Every direct `client.send_event(...)` call added while deleting the 8
> send-path layers uses `.broadcast()` explicitly (repository announcements,
> state, issues, PRs, patches, comments, statuses and NIP-09 deletions).
> `.gossip(...)` stays configured in `signed_nostr::backend::with_database`
> for future NIP-17/NIP-65 features, per the recommendation below.
Per team direction: gossip is a deliberate, load-bearing choice for this
client (it's not fully wired up to a feature yet, but it's not incidental
configuration either). `.gossip(...)` stays in `signed_nostr::backend::with_database`
@@ -784,6 +826,27 @@ source of truth.
## 10. Bootstrap-on-construction should go through `cx.defer`, not run synchronously in `new`
> **Status: done.** All six constructors listed in the table below now build
> `Self` with no side effects, capture `cx.entity().downgrade()`, and defer
> the bootstrap call(s) with `cx.defer(move |cx| { weak.update(cx, |this,
> cx| ...).ok-or-log(); })`. Verified `cx.entity()` is safe to call before
> the entity is registered: `App::new`'s `cx.entities.reserve()` bumps the
> ref count to 1 before `build_entity` runs (`app/entity_map.rs:114-117`),
> so `weak_entity().upgrade()` succeeds throughout construction, and the
> deferred closure only runs after `cx.new`'s `insert_entity` call has fully
> populated the entity, so the weak upgrade inside the deferred closure
> always succeeds too (barring the caller synchronously dropping the
> just-created `Entity` before yielding, an edge case worth a log line, not
> a crash). `RepoStore::new` bundles its three previously-sequential calls
> (`subscribe_remote`, `connect_announced_relays`, `refresh`) into one
> deferred closure to preserve their relative order. Failure to upgrade is
> logged with `log::warn!` rather than silently discarded with `.ok()`, per
> this project's error-handling rule. `cargo check --workspace`,
> `cargo clippy -p signed_state --all-targets` and `cargo test --workspace`
> all pass unchanged — none of the existing tests construct these stores
> through a `TestAppContext` and assert state immediately after `cx.new`,
> so no test needed a `cx.run_until_parked()` addition.
Verified against the pinned GPUI revision
(`crates/gpui/src/app.rs:1999-2005`, `crates/gpui/src/app/context.rs:296-315`).
@@ -840,6 +903,39 @@ talk to other entities" in the same synchronous call, which is exactly what
## 11. Split independently-observed state into child entities
> **Status: done**, with one correction to the approach originally sketched
> below. `Backend::pushing_repos` is now `Entity<HashSet<RepoAddr>>`,
> created with `cx.new(|_| HashSet::new())` in `Backend::new` and exposed
> via `Backend::pushing_repos() -> Entity<HashSet<RepoAddr>>` 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<T>, f: impl FnOnce(&mut T,
> &mut Context<T>) + 'static) -> Deferred<impl FnOnce()>`
> (`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<Backend>`), 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<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
@@ -887,6 +983,33 @@ This principle is also the reason **not** to merge `LocalReposStore` and
## 12. One debounce at the source, not one per store
> **Status: done.** `Backend::new`'s pump now batches: it waits for the
> first `ClientNotification::Event`, then races `notifications.next()`
> against a `PUMP_DEBOUNCE` (200ms) timer in a loop, collecting every event
> that arrives before the deadline into one `Vec<Update>`, then emits a
> single `BackendEvent::NostrUpdate(Vec<Update>)`. 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<Box<dyn Stream<Item =
> 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<Update>`; its 3
> actual subscribers (`ProfileStore`, `RepoStore`, `RepoListStore` —
> `CheckoutsStore` only observes `RepoListStore`/`LocalReposStore`, it never
> matched on `NostrUpdate` directly) were updated to iterate the batch
> (`.any(...)` for the two relevance checks, a `for` loop over the
> metadata-kind updates in `ProfileStore`). Also fixed a `let _ =` silently
> discarding a `WeakEntity::update` result in `ProfileStore::handle_requests`,
> found while touching this file, replaced with the `.ok()` idiom used
> everywhere else in this crate for the same "entity may already be gone"
> case. Downstream per-store `RefreshGate` debounce windows are left
> unchanged for now, per the "measure before resizing" note below.
> `cargo check --workspace`, `cargo clippy --workspace --all-targets` and
> `cargo test --workspace` all pass unchanged.
Flagged example — the notification pump (`backend.rs:126-146`):
```rust
@@ -962,6 +1085,16 @@ than speculatively resizing four timers up front.
## 13. `local_repos.rs` + `repo_list.rs`: merge the files, not the entities
> **Status: done.** Merged both files into `signed_state/src/repos.rs`, keeping
> `LocalReposStore` and `RepoListStore` as two fully independent structs, each
> still its own `Entity`/`Global` with the same `global()`/`set_global()` pairs
> and public API as before — zero call-site churn beyond fixing the `use`
> paths (`crate::local_repos`/`crate::repo_list` → `crate::repos`) in
> `checkouts.rs`, `repo.rs` and `lib.rs`. `cargo check --workspace`,
> `cargo clippy -p signed_state --all-targets` and `cargo test --workspace`
> (signed_state 24 tests, workspace 14 tests, full suite 165+ tests) all pass
> unchanged.
These two are structurally near-identical: both hold an `Arc<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()`
@@ -1167,6 +1300,47 @@ than avoidable duplication.
## 17. Business logic that leaked into `crates/workspace` and should move to `signed_core`/`signed_state`
> **Status: done.** All four sub-items landed:
>
> - `current_commit_of` is now `pub fn` in `signed_core::model`; the
> byte-for-byte duplicate in `pull_request_detail.rs` is deleted, replaced
> by an import.
> - `merge_base_of`, `clone_urls_of`, `branch_name_of` and `latest_update`
> moved to `signed_core::model` as `pub fn`s with their tests (the same
> `signed()`/`pr_root()`-style fixtures the doc predicted, renamed
> `signed_at`/`pr_root` to avoid colliding with `model.rs`'s existing
> single-owner `keys()`/`announcement_event` fixtures used by unrelated
> `is_fork_of` tests in the same file). `pull_request_detail.rs` lost the
> now-unused `Nip34Tag`/`Url` imports as a result.
> - `fork_candidates` moved to `signed_core::model` as planned. The doc's
> wording was ambivalent about where `fork_namespace` should go ("safe,
> low-risk move to `signed_core`" vs. "pairs naturally with `signed_git`'s
> ref-naming conventions" in the same paragraph) — turns out only one is
> actually possible: `fork_namespace` calls `signed_git::sanitize_path_component`,
> and `signed_git` **depends on** `signed_core` (`signed_git/Cargo.toml`),
> so moving it to `signed_core` would be a circular dependency. It moved to
> `signed_git` instead, next to `sanitize_path_component`, with a new unit
> test (it had none before). `fork_candidates` has no such constraint (only
> touches `Announcement`/`RepoAddr`/`PublicKey`) and moved to `signed_core`
> as planned, tests included. `new_pull_request.rs`'s entire `mod tests`
> block was deleted — both moved functions were the only things it tested.
> - `NewPullRequestView::submit` no longer calls `format_patch_between`
> itself: `RepoStore::open_pull_request_from_refs(repo_path, merge_base,
> compare_ref, subject, description, branch_name, draft, cx) ->
> Task<Result<(), Error>>` does the `format_patch_between` + empty-check +
> `open_pull_request` sequence internally, with the exact same two error
> messages ( "No commits between the branches to propose" /
> "Failed to generate the patch: {error}") the view used to produce
> inline, now surfaced through the returned `Task`'s `Err` and displayed
> via the view's existing `self.error` field — no observable UI change.
> `submit` shrank to gathering form values and awaiting the store call;
> `format_patch_between` is no longer imported in `new_pull_request.rs`.
>
> `cargo check --workspace`, `cargo clippy --workspace --all-targets` and
> `cargo test --workspace` all pass; test counts moved with the functions
> (`signed_core` 41 → 48, `signed_git` 67 → 68 for the new `fork_namespace`
> test, `workspace` 14 → 7), no failures, no coverage lost.
Direct answer to "can the view side be thinner": yes, and not speculatively —
found one confirmed duplicate, one cluster of misplaced domain parsing, and
one mutating-flow split across the view/store boundary. The test used to
@@ -1350,15 +1524,22 @@ method.
Done: see §9. Manual create-repository-then-open-detail-view pass still
recommended before shipping, since it depends on the grasp push actually
succeeding end-to-end against a live server.
7. **Merge `local_repos.rs` and `repo_list.rs` into one file** (§13),
7. ✅ **Merge `local_repos.rs` and `repo_list.rs` into one file** (§13),
keeping both stores as independent entities. Purely organizational, zero
call-site changes, safe to do any time.
8. **Route construction-time bootstrap through `cx.defer`** (§10) in all
Done: merged into `signed_state/src/repos.rs`, see §13 for verification
notes.
8. ✅ **Route construction-time bootstrap through `cx.defer`** (§10) in all
six stores listed there. Mechanical per store, but touch them one at a
time and re-run each store's test suite, since ordering-sensitive
assumptions (e.g. a test that asserts state right after `cx.new`) may
need `cx.run_until_parked()` inserted where they didn't before.
9. **Consolidate the send paths** (§2): introduce the single
Done: `Backend`, `RepoStore`, `RepoListStore`, `LocalReposStore`,
`CheckoutsStore` and `ProfileStore` all defer their bootstrap now, see
§10 for verification notes.
9. ✅ **Consolidate the send paths** (§2): introduce the single
`require_relay_accepted` helper, delete
`Backend::send`/`publish_event`/`send_fire_and_forget`/`broadcast_event`/
`RepoStore::send`, switch `retract_events` to `EventDeletionRequest`
@@ -1367,13 +1548,23 @@ method.
This is the biggest diff and touches every publish call site (`repo.rs`,
`backend.rs`), so do it as its own PR with full test-suite coverage
before/after.
10. **Split `pushing_repos` (and similar fields) into a child entity** (§11).
Done: see §2 and §4 for the full list of call sites, the one deliberate
narrow exception (`RepoStore::publish`), and verification notes.
10. ✅ **Split `pushing_repos` (and similar fields) into a child entity** (§11).
Small, isolated change once §9's `PushGuard` rewrite is in flight — do
them together since both touch `PushGuard`.
11. **Centralize the notification-pump debounce** (§12). This one is the
Done: see §11 — implemented via `AsyncApp::on_drop`, not the plain
`Drop` impl originally sketched, which turned out not to be possible.
11. ✅ **Centralize the notification-pump debounce** (§12). This one is the
most speculative of the batch — land it after §7's `SyncProgress`
decision and re-measure whether each store's own `RefreshGate` window
can shrink, rather than assuming the exact shape up front.
Done: see §12. Landed without waiting on §7 since it doesn't depend on
that decision — downstream `RefreshGate` windows were deliberately left
unresized, so there's nothing here for §7 to invalidate either way.
12. **Optional, product call:** drop `SyncProgress` from
`RepoListStore`'s relevant-event match if progressive reveal during
bootstrap sync isn't a feature you want (§7).
@@ -1383,7 +1574,7 @@ method.
parallel if you have a second contributor, otherwise last since it's
the largest and riskiest single change (needs fixture-by-fixture
verification against the existing test suite).
14. **Move the misplaced `workspace` domain logic to `signed_core`/`signed_state`** (§17):
14. ✅ **Move the misplaced `workspace` domain logic to `signed_core`/`signed_state`** (§17):
make `current_commit_of` `pub` in `signed_core` and delete the
`workspace` duplicate; move `merge_base_of`/`clone_urls_of`/`branch_name_of`/
`latest_update` and `fork_candidates`/`fork_namespace` there too, tests
@@ -1392,6 +1583,10 @@ method.
itself. Low risk, no behavior change, best done as its own small PR per
function cluster rather than one big move.
Done: see §17. `fork_namespace` ended up in `signed_git`, not
`signed_core`, to avoid a circular crate dependency — everything else
landed exactly as planned.
Everything **not** listed above (per-repo/per-list `Entity` stores, the
`RefreshGate` debounce/coalesce pattern, `Nip34Tag`/`Coordinate`/`Filter`
usage in `signed_core`, the GRASP push-retry state machine in