- Surface backend errors as error notifications in the workspace instead of silently dropping them
- Break ties in repository activity lists by event id, so same-second events order deterministically
- Restructure the backend around domain types: git operations behind a `Repo` type, the grasp push pipeline behind `GraspPush`, nostr connectivity behind `NostrBackend`, and shared helpers consolidated into `utils`
### Fixed
### Removed
- Remove dead code: the unused `login`/`logout` family, the unwired `SyncProgress` pipeline, `merge_pull_request`, inbox mark-read/archive APIs and the wasm32-only code paths
Scope: `signed_core`, `signed_git`, `signed_nostr`, `signed_state`, `utils`, `paths`, `settings` (~13,000 lines). `workspace`, `signed_ui`, `dock`, `desktop` are touched only as call sites. `.delta/` is ignored throughout.
## Goals
1. Behavior lives in `impl` blocks on a named domain type. No free functions in domain crates.
2. Free functions are allowed only in `utils` (and `paths`, which is the same kind of crate) — pure, stateless helpers.
3. Comments only where they carry non-obvious "why": protocol rules, safety invariants, races, ordering guarantees. No summaries, no field-name restatements.
4. Tests only for protocol interop and user-visible contracts. Delete tests that lock internal structure.
Everything below is behavior-preserving except the explicit deletions in Phase 0.
- **`signed_git` is 52 free functions and 3 impl blocks.** `gix::open` is called 29×; nearly every entry point re-opens the repository. Eight near-duplicate `worktree_*`/`repo_*` pairs exist only to bridge `&Path` ↔ `&gix::Repository` (`worktree_all_commits`/`all_commits`, `worktree_branches`/`repo_branches`, `worktree_ref_state`/`repo_ref_state`, `worktree_current_branch`/`current_branch`, `head_commit_id`/`head_commit`, …). The `worktree_` prefix means two different things (path-based vs `&gix::Repository`-based). `GitCache` is the one type done right — it is the template for the refactor.
- **`signed_state/backend.rs` (2,020 lines) is a god object plus 24 free helpers.** `create_repository` (L370–546) and `publish_local_repo` (L549–722) duplicate ~120 lines of validation/announce/push/retract/fan-out; `push_repo_from` (L752) is already the factored third variant. The grasp push loop (`push_staged_to_grasps` + 6 helpers) and the grasp URL family float freely. `Backend::connect_repo_relays` (method) calls a free fn of the same name (L1375).
- **`signed_core` operates on foreign types.** 47 free functions take `&Event`/`&RepoAddr`-alias first args or are module-bound constructors; inherent impls are impossible without introducing types. `RepoAddr` is a bare alias for the SDK's `Coordinate`, so it can carry no methods.
- **`signed_nostr::new_backend` returns a tuple** `(Client, UniversalSigner)` whose halves are always created and consumed together.
- Duplication across crates: three middle-truncation helpers (`utils` private `truncate_middle`, `signed_ui::middle_truncate`, `workspace::truncate_label`), two hand-rolled mbox envelope parsers, three git-CLI spawn patterns, and `workspace/src/views/repo/mod.rs:1955` re-assembling by hand what `worktree_snapshot` should provide.
### Dead code (grep-verified, zero callers outside own crate)
| Item | Location | Note |
|---|---|---|
| `login`, `login_with_new_identity`, `login_with_nsec`, `login_with_bunker`, `logout`, `with_master_key` | backend.rs L917–1025, L1451 | No UI path calls these; `import_dialog::open` is an empty stub (TODO.md confirms) |
| `BackendEvent::SyncProgress` pipeline (variant, `sync_progress` field + accessor, progress task) | backend.rs L43, L64, L1104, L1191–1263 | No subscriber; TODO.md retains it for an unbuilt indicator |
-`RepoStore::set_status` and `reply` are pub but only used internally (draft-PR flow / `comment`).
---
## 2. Target types
New or promoted types that absorb the free functions. Each row is a home, not a new abstraction layer — they are mostly newtypes over data the code already passes around.
| Crate | Type | Absorbs |
|---|---|---|
| `signed_core` | `RepoAddr` (newtype over `Coordinate`, replacing the alias) | `repo_addr` → `RepoAddr::new`; `identifier_from_name` → `RepoAddr::identifier_from_name` (NIP-34 identifier policy stays here, not utils); enables `addr.announcement_filter()` etc. |
| `signed_core` | `trait Nip34Event` (extension on `Event`) | `current_commit_of`, `merge_base_of`, `clone_urls_of`, `branch_name_of`, `activity_subject`, `references_root`, `is_git_activity` — needed because the same accessors apply to PR roots *and* updates, where `PullRequest` can't go |
| `signed_git` | `Repo` (wrapper over `gix::Repository`) | ~46 of the 52 free functions — see Appendix A. `Repo::open`, `Repo::open_cached` (object cache for history walks), `Repo::init`, `Repo::clone`. One `gix::open` per operation instead of 29 scattered calls. The `worktree_*`/`repo_*` pairs collapse |
`settings` needs nothing: it is already plain data + one store. `default_scan_paths` folds into the `Default` impl.
---
## 3. Phases
Each phase ends with `cargo check --workspace && cargo test --workspace` green and its own commit, so review and bisection stay cheap. Call-site updates in `workspace`/`signed_ui`/`desktop` happen in the same commit as the API change.
### Phase 0 — Delete dead code and non-critical tests
The cheapest phase; shrinks every later diff.
1. Delete the `login`/`logout` family + `with_master_key` (backend.rs). Either delete the empty `import_dialog` stub and its menu entry, or leave the stub — **decision D1**.
2. Delete the `SyncProgress` pipeline (variant, field, accessor, progress task). TODO.md's planned indicator can re-add from git history when it is actually built — **decision D2, default delete**.
3.`BackendEvent::Error`: 9 emit sites, 0 subscribers. Either wire one subscriber in `workspace` (a banner/toast, ~15 lines, makes existing error emissions user-visible) or delete variant + emissions — **decision D3, default: wire the subscriber** (AGENTS.md requires errors reach the UI; the emissions already exist).
4. Delete `merge_pull_request` + `publish_applied_status` (recoverable from history if the merge UI is built) — **decision D4, default delete**.
8. Remove wasm32 paths + `nostr-memory` target dep — **decision D5, default remove** (no wasm target exists). Revert is trivial if a web build materializes.
9. Replace `U: AsRef<str>` generics with concrete `&[String]`/`&str` signatures.
10.**Delete all NON-CRITICAL tests** (list in §5). Doing this before the structural phases means the structure changes never touch them.
### Phase 1 — `utils` consolidation
- Add `middle_truncate`; delete `signed_ui`'s private copy and `workspace::truncate_label`; `shorten_pubkey` calls it.
- These are drop-in; domain crates already depend on `utils`.
### Phase 2 — `signed_core`
Dependency bottom, consumed by everything else.
1.`RepoAddr`: alias → newtype with `new`, `identifier_from_name`, `Display`/`From`/`FromStr` delegation. Mechanical fallout across all crates (~30 sites).
2.`Nip34Event` extension trait for the tag accessors; `PullRequest<'a>` newtype; `Announcement::forks_in`.
3.`RepoState` (replaces the tuple return; one consumer in `signed_state/repo.rs` refresh).
4.`ThreadResolver<'a>` absorbing the seven inbox/threading helpers.
5.`Filters` unit struct + repo-scoped filters as `RepoAddr` methods; `RepoStatus::resolve` assoc fn.
### Phase 3 — `signed_nostr`
`NostrBackend::open` / `in_memory`; `signed_state::init` constructs it. ~30 lines.
### Phase 4 — `signed_git` → `Repo`
The biggest mechanical phase.
1. Introduce `Repo` wrapping `gix::Repository` with `open`, `open_cached`, `init`, `clone`.
2. Move every function from Appendix A into methods; collapse the 8 duplicate pairs; the `worktree_` prefix disappears (`repo.dirty()`, `repo.snapshot()`, `repo.branches()` …).
3. Fold remote operations into methods (`push_ref`, `push_all`, `fetch_refs`, `origin_url`, `ensure_origin`, `set_origin`, `remote_has_refs`); one git-CLI spawn helper instead of three patterns.
4. One mbox envelope parser shared by `split_patch_series`/`patch_commits` (stays in `signed_git` as an impl on the patch parser, not utils — it is not generic).
5.`GitCache::open`/`ensure_clone` return `Repo`.
6. Extend `Repo::snapshot()` with branches/tags/current_branch so `workspace/src/views/repo/mod.rs:1955` (`load_repo_data`) collapses into it.
7. Keep pure string parsing (`sanitize_path_component`, `fork_namespace`, `split_patch_series`, `patch_diffs`, `patch_commits`) as assoc fns on the owning type (`Repo::sanitize_path_component`, `PatchParser::…`) or move to `utils` if they carry no git semantics — Appendix A marks each.
### Phase 5 — `signed_state`
1. New `push.rs`: `GraspServer` newtype + `GraspPush` + `PushOutcome`/`PushRejection`, moving ~400 lines out of `backend.rs`.
2. Factor the shared announce→authorize-push→retract-on-failure→fan-out orchestration out of `create_repository`/`publish_local_repo`, using `push_repo_from` as the template (~120 lines saved). Add `publish_one` (sign → send → `require_relay_accepted`, returning the `Event`) and collapse its 3 open-coded copies in repo.rs; merge the shadowed `connect_repo_relays` pair.
3.`StatusIndex`, `PatchSeries`, `Mirrors`; `Inbox`/`CheckoutsStore`/`LocalReposStore`/`Backend`/`RepoStore` method moves per §2.
4. Fold `push_repository`/`push_checkout` wrapper bodies into one helper.
5. Consider whether `backend.rs` (~2,020 → ~1,200 lines) still warrants a file split after the push extraction; stop there — the remaining methods are genuinely `Backend`'s job.
### Phase 6 — Comment stripping
Apply §4 across all backend crates. Mechanical; do it last so it never masks a behavioral diff.
### Phase 7 — Final sweep
-`cargo fmt`, `cargo clippy --workspace`, full test run.
- Invariant grep: `grep -rn '^\(pub \)\?\(async \)\?fn ' crates/{signed_core,signed_git,signed_nostr,signed_state}/src` returns only `lib.rs``init` and test helpers.
- Update `docs/TODO.md` (both entries are consumed by this plan) and this document's status.
---
## 4. Comment policy
Delete by default. Keep only (one line each, unless truly needed):
- **Protocol/format rules**: NIP-34 `u`-tag parsing workaround (model.rs L311 — the SDK's `Nip34Tag` doesn't model the `u` tag); `subject`-tag fallback; `refs/heads` publication form in state events; NIP-10 `e` vs NIP-22 uppercase `E` semantics; kind→root mapping table in inbox.rs L115–124; `comments_for` returning two filters because `#E`+`#e` would AND (filters.rs L109–112); ngit URL parity notes (`grasp_base_url` scheme mapping, `/prs/` namespace, `clone` tag ordering).
- **Races / retry taxonomy**: grasp denial classification (backend.rs L1620–1638 — the single most valuable comment in the codebase); stale-advertisement race; same-second resend dedup on `sign_state_event`; convergence probe rationale.
- **Non-obvious `None`/error contracts** where callers depend on them (merge-base `Ok(None)` = unrelated history, unborn HEAD → `None`).
- **Perf "why"**: object-cache rationale in history walks; commit cap for virtual lists.
Delete: every field doc that restates the field name, method summary one-liners, `/// Get the profile.`-style docs, organizational comments, test-helper docs, per-constant restatements, duplicated platform tables in `paths`.
Rule: a test earns its place if breaking the behavior would silently break a user (protocol wire formats, interop with `git`/`ngit`/`nak`, state machines users hit). Delete tests of internal ordering, trivial helpers, and coalescing machinery.
Consolidate retained helpers while there: three git-env helpers → one, five bare-server `git init --bare` blocks → one fixture, delete the inline `run` closure at tests.rs L634.
Out of scope but flagged: `signed_ui/pixel_avatar` (identicon determinism — borderline, visual), `dock/tests/render_smoke.rs` (UI smoke).
---
## 6. Risks
- **`PullRequest<'a>` lifetimes** at `pull_requests/detail.rs` call sites — mitigated by keeping it a borrow newtype, not an owning copy.
- **`Repo` wrapper vs background tasks**: `RepoStore` spawns git work via `cx.background_spawn`. The wrapper must stay `Send` when moved into tasks exactly as `gix::Repository` is today; no interior mutation planned, so this is mechanical.
- **Phase 2 `RepoAddr` newtype** touches ~30 call sites in one commit — small each, many files; consider `impl From<Coordinate>` bridges to keep the diff mechanical.
- **Backend push orchestration dedup** (Phase 5.2) is the only phase that reorders logic; land it alone in its own commit behind the existing tests (URL-format + denial-classification + `push_all`/`fetch_repo_refs` interop tests are the net).
## 7. Open decisions
| # | Question | Default |
|---|---|---|
| D1 | Delete the empty `import_dialog` stub + menu entry, or keep the shell? | Keep shell (UI territory, 0 cost) |
| D2 | `SyncProgress` pipeline | Delete; re-add with the feature |
Kept intentionally. The progress pipeline (the `SyncProgress` variant, the `sync_progress`
field and its accessor, and the progress task in `sync_bootstrap`) is retained for a planned
sync progress indicator. No subscriber exists yet. Do not remove it without revisiting that
plan.
## `login` / `logout` family
File: `crates/signed_state/src/backend.rs`
No UI path calls these. `import_dialog::open` is an empty stub. Decide whether to
delete the family (`login`, `login_with_new_identity`, `login_with_nsec`,
`login_with_bunker`, `logout`) or wire the stub to `Backend::login`.
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.