230 lines
13 KiB
Markdown
230 lines
13 KiB
Markdown
# PLAN — Codebase audit: over-engineering, dead code & simplification
|
||
|
||
> **Status (2026-09-04): proposed.** Full-repo audit (~27k lines, 13 crates).
|
||
> Every finding was cross-checked against consumers and verified against the
|
||
> locked library sources (gpui-component `18922d6`, rust-nostr `472c883`,
|
||
> gix 0.87.1). Pick the steps you want before processing; ordered
|
||
> safest-first. Estimated total: **~3,000+ lines removable (~12%)**.
|
||
|
||
---
|
||
|
||
## Step 1 — Zero-risk deletions (~700 L, pure removals)
|
||
|
||
1. **Unused Cargo deps** (verified by grep):
|
||
- `desktop`: `gpui_linux`, `gpui_windows`, `gpui_macos` (only
|
||
`gpui_platform` is used — it's the meta-crate that picks the backend),
|
||
`log`
|
||
- `workspace`: `chrono`
|
||
- `signed_nostr`: `signed_core`, `nostr`
|
||
- `assets`: `log`
|
||
2. **`signed_core::clone_url` module** (whole file) — only self-tests
|
||
reference it; `repo_detail/mod.rs:2418` *produces* the format but never
|
||
parses it. (`clone_url.rs` + `lib.rs:13`) <- acceptable
|
||
3. **`signed_core::comments` module** (`CommentThread`, `comment_threads`) —
|
||
no consumers; SDK has `nip22::extract_parent` anyway.
|
||
(`comments.rs` + `lib.rs:14`)
|
||
4. **`paths` dead accessors**: `cache_dir()`, `logs_dir()`, `keymap_file()`,
|
||
`set_custom_data_dir()` + `CUSTOM_DATA_DIR`/`OnceLock` machinery
|
||
(`paths/src/lib.rs:13,50-61,112-169`). Used: `desktop_dir`,
|
||
`documents_dir`, `settings_file`, `nostr_dir`, `repos_dir`.
|
||
5. **`signed_git::patch_applies`** (+test) — own TODO says superseded by the
|
||
live compare view (`lib.rs:252-277, 2294-2324`).
|
||
6. **`signed_git::worktree_tags`** — both UI sites call `repo_tags` directly
|
||
(`lib.rs:1811-1813`).
|
||
7. **Unused re-exports**: `workspace::image_cache` (`workspace/src/lib.rs:6`);
|
||
`signed_state::lib`'s `pub use utils::shorten_pubkey`
|
||
(`signed_state/src/lib.rs:21`).
|
||
8. **Sidebar placeholder nav items** "Inbox", "Search", "Guide" — all three
|
||
just open the Explore panel (`sidebar/mod.rs:578-618`). <- acceptable
|
||
9. **`import_dialog.rs`** — opens an empty 400px dialog; the "Import identity"
|
||
sidebar button is a dead end (`sidebar/mod.rs:405-408,493-504`). <- acceptable
|
||
10. **All `wasm32` cfg paths + `nostr-memory` dep** — GPUI has no wasm
|
||
backend; the target cannot link (`signed_state/src/lib.rs:52-63`,
|
||
`checkouts.rs:108-165`, `signed_nostr/src/backend.rs:9-29`,
|
||
`signed_nostr/Cargo.toml`). <- acceptable, note: GPUI have support for wasm via gpui_web, updated your memory or check before make changes
|
||
11. **`dock::t()` unreachable `"Dock.Unnamed"` arm** (`dock/src/lib.rs:21-30`).
|
||
12. Two handler-less `Button`s ("user" with fake `dropdown_caret`,
|
||
"maintainers") — render as plain `h_flex` or wire real menus
|
||
(`sidebar/mod.rs:426-434`, `repo_detail/mod.rs:2289-2307`). <- acceptable
|
||
13. Stale doc references to nonexistent `helpers::track`
|
||
(`diff.rs:328`, `mod.rs:201-203`).
|
||
|
||
## Step 2 — Dead feature removal (~500 L)
|
||
|
||
1. **Login/logout API** — `login`, `login_with_new_identity`,
|
||
`login_with_nsec`, `login_with_bunker`, `logout`, `with_master_key`
|
||
(`backend.rs:879-988,1537-1542`). UI only uses `create_identity` + keyring
|
||
restore. ⚠️ `login_with_new_identity` stores unencrypted nsec — security
|
||
footgun. **Keep** `extract_master_key` + the `bunker://` branch of
|
||
`restore_session` (services older keyring entries). <- acceptable
|
||
2. **`RepoStore::merge_pull_request` + `publish_applied_status`** — no merge
|
||
action exists in the UI (`repo.rs:1122-1256`). <- acceptable
|
||
3. **`RepoStore::publish_state`** — superseded by `Backend::push_repo_from`
|
||
(`repo.rs:1070-1120`).
|
||
4. **Annotation machinery** — `cover_note_of`/`labels_of`/`subject_of` (no
|
||
callers) + `cover_notes`/`labels` fields + per-root DB query loop +
|
||
`annotations_for` relay fetch. **Every kind-1624/1985 event currently
|
||
triggers a full refresh of every open RepoStore for data nothing
|
||
displays** (`repo.rs:52-56,339-362,489,531-563`).
|
||
5. **`BackendEvent::Error` variant + `emit_error` + ~16 emission sites** —
|
||
matched nowhere (consumers only match `SignerChanged`, `SignerRequired`,
|
||
`PassphraseRequired`, `NostrUpdate`, `Synced`, `SyncProgress`,
|
||
`Published`) (`backend.rs:64,67-74,1047-1050`). <- acceptable
|
||
6. **`BackendEvent::Connected` + `connected` field + `is_connected()`** —
|
||
emitted twice, never consumed (`backend.rs:49,83,154,1110-1111,1052-1055`).
|
||
7. **`sync_progress` field + getter + `SyncProgress::channel` watch-loop
|
||
math** — payload discarded; used only as a dumb refresh tick
|
||
(`backend.rs:84,1057-1060,1227-1258`). <- acceptable
|
||
8. **`Backend::subscribe`, `add_discovery_relays`, `publish_announcement`** —
|
||
zero callers (`backend.rs:1123-1158,1388-1395`).
|
||
9. **Write-only fields**: `RepoStore::refs` (`repo.rs:37-38,449`),
|
||
`RepoListStore::set_author` (`repo_list.rs:135-140`),
|
||
`Update::event_id` (`signed_nostr/src/update.rs:10`).
|
||
10. **NIP-44 half of `UniversalSigner`** (`AsyncNip44` bounds,
|
||
encrypt/decrypt plumbing, ~60 of 200 L) — app never touches DMs
|
||
(`signed_nostr/src/signer.rs`). <- acceptable
|
||
|
||
## Step 3 — Library swaps (~600 L)
|
||
|
||
1. **`signed_ui::DropdownButton` → `gpui_component::button::DropdownButton`**
|
||
— present in the locked revision, same `new/button/dropdown_menu` surface
|
||
(the local doc even says it matches). Migrate 3 call sites
|
||
(`pull_requests.rs`, `repo_detail/mod.rs`), delete the ~200 L file. <- acceptable
|
||
2. **`wire_number_input` → `SettingField::number_input`**
|
||
(`settings_dialog.rs:691-761`, ~90 L) — you already import
|
||
`NumberFieldOptions` from gpui-component's setting module. <- acceptable
|
||
3. **Custom C-unquoting → `gix::quote::ansi_c::undo`**
|
||
(`signed_git/src/lib.rs:1610-1683` + call sites, ~100 L) — already in the
|
||
dep tree, octal/escape semantics identical. Keep tests as regression tests.
|
||
4. **`ensure_origin` redundant refspec write** (`signed_git/src/lib.rs:523-530`)
|
||
— `git remote add` creates `remote.origin.fetch` by default.
|
||
5. **Two hand-rolled tab bars → gpui-component `TabBar`**
|
||
(`repo_detail/mod.rs:2120-2265`, `new_pull_request.rs:1236-1305`, ~110 L) —
|
||
`pull_request_detail.rs` already uses `TabBar` correctly (proves the fit). <- acceptable
|
||
6. **CLI `merge_base` → `gix::Repository::merge_base`**
|
||
(`signed_git/src/lib.rs:201-222`) — the codebase already uses the gix one
|
||
in `pull_request_detail.rs:244`.
|
||
7. **`image_cache` → `gpui::retain_all` or single-map LRU** — vendored copy
|
||
with a `max_items` param every call site passes `MAX_IMAGES=128` to
|
||
(`signed_ui/src/image_cache.rs`, 139 L). Delete the param + dual-structure
|
||
LRU or use upstream unbounded cache.
|
||
|
||
## Step 4 — Dedup passes (~1,200 L)
|
||
|
||
1. **`PullRequestDetailView` → hold `Entity<DiffPane>`** instead of its ~200 L
|
||
inline copy of `diff.rs` (`pull_request_detail.rs:310-552`). The other two
|
||
consumers already do this.
|
||
2. **Issue/PR detail shared sections** (~300 L): comments list, comment form,
|
||
participants/labels sidebar, `sidebar_title` — extract into `helpers.rs`
|
||
(`issue_detail.rs` vs `pull_request_detail.rs`).
|
||
3. **Dialog scaffolding helper** (~150-200 L): 4 copies of `{busy, error}`
|
||
state structs, verbatim error rows, identical
|
||
`cx.spawn → close_dialog / show error` plumbing
|
||
(`onboarding_dialog.rs`, `passphrase_dialog.rs`, `create_repo_dialog.rs`,
|
||
`init_dialog.rs`, +2 more sites).
|
||
4. **Triplicated debounce state machine → one helper** — `refreshing` /
|
||
`refresh_dirty` / `debouncing` trio copied into `repo.rs`, `checkouts.rs`,
|
||
`repo_list.rs` (~105 L).
|
||
5. **signed_git helpers**: `push_main`/`push_all` twin bodies → one
|
||
`push_refspecs`; git-CLI spawn boilerplate → one `git_output`;
|
||
grasp→https rewrite → one fn; "try each mirror URL" loop → one helper.
|
||
6. **grasp-list parsing ×3 → one helper** — `backend.rs:996-1012`,
|
||
`backend.rs:1586-1618`, `grasp_servers.rs:240-265`.
|
||
7. Smaller copies: ref-selector trigger ×2, count badge ×3 (use
|
||
`signed_ui::CountBadge`), grasp-server editor duplicated in
|
||
`settings_dialog.rs:408-614` vs `grasp_servers.rs` (~90 L), folder-picker
|
||
prompts ×4, "add panel to dock Center" ×10, fork-label upstream lookup ×2,
|
||
avatar+name row ×9 (→ one `user_row` helper in signed_ui).
|
||
8. **`Backend::send` vs `publish_event`** — copy-pasted bodies differing only
|
||
in `finalize_async` (`backend.rs:1293-1386`).
|
||
|
||
## Step 5 — Structural (do deliberately)
|
||
|
||
1. **`UniversalSigner` → enum** — verified: nostr-sdk 0.45 `Client` has no
|
||
signer slot (external `SignerAuthenticator` by value at build time), so a
|
||
swap-in-place wrapper IS needed — but only `Keys`/`NostrConnect` ever
|
||
occur. Replace 200-L vtable (`InnerSigner` trait + `InnerSignerImpl<T>` +
|
||
custom error + boxed futures) with
|
||
`enum Signer { Keys(Keys), Connect(NostrConnect) }` in
|
||
`Arc<RwLock<Signer>>` (~40 L). <- acceptable
|
||
2. **`GitStore` single install** — `signed_state::init` installs empty root
|
||
(`lib.rs:44`), immediately replaced by `desktop/main.rs:81`. Pass the root
|
||
into `init`.
|
||
3. **Error handling in signed_git**: `map_err(|e| anyhow!("{e}"))` → plain
|
||
`?` (preserves source chain; `lib.rs:1787,1798,1845,1852`); blanket
|
||
`.ok()`/`.unwrap_or_default()` → matched cases
|
||
(`lib.rs:919,995,1761,364,682,1779`).
|
||
4. **`ProfileStore` second flume channel → `WeakEntity` + `update()`**
|
||
(~25 L; the results channel only exists to get back to main thread).
|
||
5. **Unbounded `tasks` Vec growth** — only `repo.rs` prunes; `backend.rs`
|
||
(~18 push sites), `checkouts.rs` (grows every 15-60 s poll cycle),
|
||
`profile.rs` (2 per metadata event) accumulate finished handles forever.
|
||
One-line `retain(|t| !t.is_ready())` per store.
|
||
6. **Redundant `observe → cx.notify()` subscriptions**
|
||
(`sidebar/mod.rs:81-88`, `repo_detail/mod.rs:354-355,1914-1921`) — this
|
||
gpui revision auto-tracks entities read during render, so re-render-only
|
||
observers are belt-and-suspenders. ⚠️ Verify before deleting; see Step 7.2.
|
||
7. **`open_upstream` sleep-poll → `cx.observe`** (`repo_detail/mod.rs:1113-1152`)
|
||
— 60×250 ms race-prone loop re-implementing the store's notify mechanism.
|
||
8. **Checkouts map `Arc` removal** — accessors deep-clone anyway
|
||
(`checkouts.rs:69-81`).
|
||
9. Minor: `CheckoutRecord` manual `Default` → derive; `dock` re-export trim
|
||
(27 items, ~10 used); `PixelAvatar.size` field with no setter → const;
|
||
`SCAN_SKIPPED_DIRS` 1-element array → direct compare; `RepoAction` menu
|
||
indirection → `on_click`; `Announcement::from_event` round-trip after
|
||
building from typed data.
|
||
|
||
## Step 6 — Over-optimization (optional, unmeasured machinery)
|
||
|
||
1. **`HeaderCache`** (`repo_detail/mod.rs:113-123,1426-1461`) — keyed
|
||
invalidation + 3 `Rc` layers memoizing 2 bech32 encodes + 2 `format!`s
|
||
(~45 L). Compute inline.
|
||
2. **Comment-body memoization** `contents: HashMap<EventId, SharedString>`
|
||
(`issue_detail.rs`, `pull_request_detail.rs`) — caches one small alloc per
|
||
render; convert inline.
|
||
3. **`OBJECT_CACHE_BYTES = 64 MiB`** indiscriminately applied — scope to
|
||
history-walk entry points or drop (`signed_git/src/lib.rs:770,859-863`).
|
||
4. **`file_commit(…, include_description: bool)`** — boolean flag saving one
|
||
alloc; split into named constructors or always include.
|
||
5. **`signed_core` could drop its `gpui` dep** if `Announcement`'s
|
||
`SharedString` fields became `String` (`model.rs:9-35`).
|
||
6. **`build_state`** raw `Tag::parse(...).expect()` → typed `Nip34Tag::to_tag`
|
||
(`signed_core/src/state.rs:7-18`).
|
||
7. **`PixelAvatar` FNV/RNG stack** (~60 L) — deterministic-hashing requirement
|
||
is legit; reconsider if `DefaultHasher` stability is acceptable.
|
||
|
||
## Step 7 — Clean verdicts + one correctness note (no action)
|
||
|
||
1. **Confirmed clean, keep as-is**: `signed_core` correctly uses
|
||
`Nip34Tag::parse` / SDK builders (only the `u` tag is hand-parsed — SDK
|
||
doesn't model it); `dock` crate is a genuine thin skin over
|
||
`gpui_base::dock` renderer traits (keep; only trim re-exports); LMDB is
|
||
justified (instant startup lists); `signed_git` test suite + `tempfile`
|
||
healthy. Gossip/NIP-65 machinery serves only one login query — *consider*
|
||
dropping if NIP-65 routing isn't on the roadmap.
|
||
2. **Correctness note**: `IssuesView` / `PullRequestsView` /
|
||
`IssueDetailView` / `PullRequestDetailView` never observe their
|
||
`RepoStore` — refresh only works because this gpui revision auto-tracks
|
||
render reads. Accidental; pinning a different gpui breaks them silently.
|
||
Relevant to Step 5.6.
|
||
|
||
---
|
||
|
||
## Suggested order of operations
|
||
|
||
1. Step 1 + Step 2 first — pure deletions, verify with `cargo check` +
|
||
`cargo test`.
|
||
2. Step 3 (library swaps) next — mechanical, mostly call-site migrations.
|
||
3. Step 4 (dedup) — largest line savings.
|
||
4. Step 5 (structural) — deliberate, one at a time.
|
||
5. Step 6 (over-optimization) — optional, only if you want the extra ~110 L.
|
||
|
||
Each step below is self-contained; tick what you want done.
|
||
|
||
- [ ] Step 1 — zero-risk deletions
|
||
- [ ] Step 2 — dead feature removal
|
||
- [ ] Step 3 — library swaps
|
||
- [ ] Step 4 — dedup passes
|
||
- [ ] Step 5 — structural changes
|
||
- [ ] Step 6 — over-optimization cleanup
|