.
This commit is contained in:
@@ -1,7 +1,5 @@
|
||||
# TODO
|
||||
|
||||
Deferred from `docs/over-engineering-cleanup-plan.md`.
|
||||
|
||||
## `BackendEvent::SyncProgress`
|
||||
|
||||
File: `crates/signed_state/src/backend.rs`
|
||||
|
||||
@@ -1,300 +0,0 @@
|
||||
# Over-engineering cleanup plan
|
||||
|
||||
## Goal
|
||||
|
||||
Remove code that exists but cannot be reached, and machinery that guards states no
|
||||
caller can produce. Findings came from a four-way read-only audit of
|
||||
`signed_state`, `workspace/views`, `signed_git`/`signed_core`, and
|
||||
`dock`/`signed_ui`/misc, cross-checked against the pinned dependency sources in
|
||||
`~/.cargo/git/checkouts/`.
|
||||
|
||||
The main claim in each task below was verified by grepping callers, not by
|
||||
reading the definition alone. Items that were only reported by the audit and not
|
||||
independently reproduced are in Phase 4 and must be verified before deletion.
|
||||
|
||||
## Rules for every step
|
||||
|
||||
- Line numbers are from the current working tree and will drift. Re-grep before
|
||||
editing; do not trust a number from this file after other tasks land.
|
||||
- Delete code, do not comment it out, do not add `#[allow(dead_code)]`.
|
||||
- If a symbol looks dead but is part of a public API or a feature that is only
|
||||
not wired yet, stop and ask.
|
||||
- Do not reintroduce the `views/repo` generation counters that were removed in
|
||||
this working tree.
|
||||
- Keep comments out. Remove any comment that describes the code being deleted.
|
||||
|
||||
## Verification commands
|
||||
|
||||
Always pass `--offline`; a plain `cargo` invocation re-resolves and fails in the
|
||||
sandbox.
|
||||
|
||||
```
|
||||
cargo fmt -p <crate> 2>/dev/null
|
||||
cargo check --offline --workspace --all-targets
|
||||
cargo clippy --offline --workspace --all-targets
|
||||
cargo test --offline --workspace
|
||||
```
|
||||
|
||||
`cargo fmt -- --check` prints unrelated "unstable features" noise on stable.
|
||||
Filter with `grep -E "^Diff in"`. Never hand-reformat; rustfmt is authoritative.
|
||||
|
||||
---
|
||||
|
||||
## Phase 1 - delete dead code
|
||||
|
||||
No behavior change. Each task is independent; commit per crate.
|
||||
|
||||
### 1.1 `RepoStore::version`
|
||||
|
||||
File: `crates/signed_state/src/repo.rs`
|
||||
|
||||
- [x] Remove field `version: u64` and its doc comment (the claim that views key
|
||||
caches to it is false).
|
||||
- [x] Remove initializers `version: 0` in both constructors.
|
||||
- [x] Remove the bump `this.version = this.version.wrapping_add(1);`.
|
||||
- [x] Remove `pub fn version(&self) -> u64`.
|
||||
|
||||
Evidence: `grep -rn "\.version()" crates` has no call sites; the only read is the
|
||||
accessor itself.
|
||||
|
||||
Acceptance: `grep -rn "version" crates/signed_state/src/repo.rs` shows only
|
||||
unrelated uses (none of the four removed sites).
|
||||
|
||||
### 1.2 `clone_url` module
|
||||
|
||||
Files: `crates/signed_core/src/clone_url.rs`, `crates/signed_core/src/lib.rs`
|
||||
|
||||
- [x] Delete `clone_url.rs`.
|
||||
- [x] Remove `mod clone_url;` and the `pub use clone_url::{CloneTarget, parse_clone_url};`
|
||||
re-export.
|
||||
|
||||
Evidence: only definition, its own test, and the re-export reference these. It
|
||||
also reimplements percent-decoding.
|
||||
|
||||
### 1.3 NIP-32 labels / cover-note helpers
|
||||
|
||||
Files: `crates/signed_core/src/annotations.rs`, `crates/signed_core/src/filters.rs`,
|
||||
`crates/signed_core/src/lib.rs`
|
||||
|
||||
- [x] Verify each of `labels`, `subject_override`, `labels_and_subject`,
|
||||
`cover_note`, `COVER_NOTE_KIND`, `annotations_for` for references outside
|
||||
this crate.
|
||||
- [x] Delete the ones with no production caller and drop them from the `lib.rs`
|
||||
re-exports.
|
||||
- [x] Keep anything still needed (for example `cover_note` /
|
||||
`COVER_NOTE_KIND` may be used by the inbox view).
|
||||
|
||||
Evidence: the UI uses `tags.hashtags()` directly in `views/discussion.rs`, not
|
||||
these helpers.
|
||||
|
||||
### 1.4 `Backend::emit_error`
|
||||
|
||||
File: `crates/signed_state/src/backend.rs`
|
||||
|
||||
- [x] Delete the method. No callers.
|
||||
|
||||
### 1.5 `Backend::pushing_repos()` accessor
|
||||
|
||||
File: `crates/signed_state/src/backend.rs`
|
||||
|
||||
- [x] Delete the getter. The `Entity<HashSet<RepoAddr>>` is used internally; only
|
||||
the accessor is unused.
|
||||
- [ ] Optional follow-up (separate task): nothing observes that entity, so it
|
||||
could be a plain `HashSet` field. Defer; it is a refactor, not a deletion.
|
||||
|
||||
### 1.6 `DropdownButton` speculative knobs
|
||||
|
||||
File: `crates/signed_ui/src/dropdown_button.rs`
|
||||
|
||||
- [x] Remove the `caret: Option<CaretBuilder>` field and the `CaretBuilder` type
|
||||
alias; it is never set, so the `unwrap_or_else` default always runs.
|
||||
Inline the default caret.
|
||||
- [x] Remove the `anchor()` builder method (never called; it is already marked
|
||||
`#[allow(dead_code)]`). Keep the `anchor` field, which is set in the
|
||||
constructor and used when rendering.
|
||||
|
||||
### 1.7 `utils::shorten_pubkey`
|
||||
|
||||
File: `crates/utils/src/pubkey.rs`
|
||||
|
||||
- [x] Drop the `len` parameter; its only call site passes `4`.
|
||||
- [x] Rename to a fixed-width helper if that reads better, or leave the name.
|
||||
|
||||
It duplicates `signed_ui::middle_truncate` conceptually, but `utils` has no gpui
|
||||
dependency, so do not move `middle_truncate`; just remove the speculative
|
||||
parameter.
|
||||
|
||||
### 1.8 Comment artifacts
|
||||
|
||||
- [x] `crates/signed_state/src/repo.rs` - delete the comment that describes
|
||||
querying cover notes and labels per root; no such query exists.
|
||||
- [x] Remove any comment left dangling by the tasks above.
|
||||
|
||||
---
|
||||
|
||||
## Phase 2 - remove guards that cannot fire
|
||||
|
||||
Each changes behavior on paper but not in practice. Smoke test after each.
|
||||
|
||||
### 2.1 `SignedDockSkin::render_dock` early return and frame duplication
|
||||
|
||||
File: `crates/dock/src/dock_area.rs`
|
||||
|
||||
Base (`gpui_base::dock`) computes `dock_extent`, returns before calling the
|
||||
renderer when the extent is `px(0.)`, and wraps the renderer's output in
|
||||
`dock_frame`. `dock_extent` is `px(0.)` exactly when `!open && !is_bottom`, which
|
||||
is precisely the condition of the early return here.
|
||||
|
||||
- [x] Delete the `if !open && !placement.is_bottom() { return div(); }` guard.
|
||||
- [x] Stop re-applying the box. The chrome is now `.flex().size_full().relative()`,
|
||||
the same shape the pinned reference skin (`crates/ui/src/dock/dock.rs`) uses;
|
||||
base's `dock_frame` supplies the extent and the overflow clip.
|
||||
- [x] Keep the closed-bottom strip height override. Note: base's `dock_frame`
|
||||
hard-codes `CLOSED_BOTTOM_STRIP` (29px) with `overflow_hidden`, so the
|
||||
`TAB_BAR_HEIGHT` (44px) override is clipped and has no visible effect. The
|
||||
strip is 29px today; changing it needs an upstream change.
|
||||
- [ ] Smoke test: open and close left, bottom, and right docks; check widths,
|
||||
the bottom strip height, and resize handles.
|
||||
|
||||
### 2.2 `push_staged_to_grasps` empty-refs guard
|
||||
|
||||
File: `crates/signed_state/src/backend.rs`
|
||||
|
||||
- [x] Delete the `if refs.is_empty() { return outcome; }` guard.
|
||||
|
||||
All three call sites pass a non-empty `refs`: one passes a literal one-element
|
||||
vec, one is inside `if !refs.is_empty()`, one is the `else` of that check.
|
||||
|
||||
### 2.3 `InboxView` per-view debounce
|
||||
|
||||
File: `crates/workspace/src/views/inbox.rs`
|
||||
|
||||
The backend pump already coalesces relay bursts into one `NostrUpdate`, and
|
||||
`query_inbox` reads only the local database.
|
||||
|
||||
- [x] Delete the `REFRESH_DEBOUNCE` constant.
|
||||
- [x] In `refresh`, drop the spawned timer; call `run_refresh` directly after
|
||||
`refresh.request()` returns `Schedule`, matching `RepoStore::refresh`.
|
||||
- [x] Keep `RefreshGate` for fold/overlap.
|
||||
- [ ] Smoke test: inbox updates live as relay events land, with no added delay.
|
||||
|
||||
### 2.4 `DockPlacement::Center` arms in `dock_toggle_button` (optional)
|
||||
|
||||
File: `crates/dock/src/tab_panel.rs`
|
||||
|
||||
- [x] The only call sites pass `Left`, `Bottom`, `Right`. The `designated`
|
||||
match's `Center` arm now returns early instead of yielding a dead `None`;
|
||||
the icon match keeps `Center => return None` for exhaustiveness.
|
||||
|
||||
Low value; skip if it makes the match less readable.
|
||||
|
||||
---
|
||||
|
||||
## Phase 3 - consolidate duplication (needs a decision)
|
||||
|
||||
Verify the duplication before extracting; each could be intentional.
|
||||
|
||||
### 3.1 `PullRequestsView` and `IssuesView`
|
||||
|
||||
Files: `crates/workspace/src/views/pull_requests/mod.rs`,
|
||||
`crates/workspace/src/views/issues/mod.rs`
|
||||
|
||||
- [x] Confirm the shape. The two `rebuild`s are the same mechanic: one pass over a
|
||||
root list, per-status counts, keep matching indices, early-return when
|
||||
filter/indices/counts are unchanged, resize the item sizes, notify.
|
||||
- [x] Extract `crates/workspace/src/views/status_list.rs` with `StatusCounts` and
|
||||
`filter_by_status`. Both views now use it; the tuple counts were replaced by
|
||||
`StatusCounts`. The notify decision stays local because it would need a trait
|
||||
over the two filter enums.
|
||||
|
||||
### 3.2 Relay URL normalize/display
|
||||
|
||||
Files: `crates/workspace/src/views/sidebar/settings_dialog.rs`,
|
||||
`crates/workspace/src/views/sidebar/grasp_servers.rs`
|
||||
|
||||
- [x] Confirm both pairs do prepend-scheme, parse, dedupe, and host-display.
|
||||
- [x] Extract `normalize_server` and `server_host` into `sidebar/mod.rs`. They live
|
||||
in `workspace`, not `signed_ui`: `signed_ui` does not depend on `nostr`, and
|
||||
these are used only by the two sidebar modules. Dedupe differs per caller
|
||||
(`Vec<RelayUrl>` vs persisted `Vec<String>`) and stays at the call site.
|
||||
|
||||
### 3.3 `crates/dock` vs the pinned `gpui_component` dock renderer
|
||||
|
||||
Files: `crates/dock/src/*` vs the pinned rev's `crates/ui/src/dock/*`
|
||||
|
||||
- [x] Spike: `SignedTabGroupSkin` cannot delegate to the pinned upstream skin.
|
||||
- `TabGroupSkin`, `TilesSkin` and `SkinShared` are `pub(crate)` in
|
||||
`gpui_component::ui`; only the opaque `DockSkin` renderer is public, and it
|
||||
holds that private shared state.
|
||||
- `TabGroupRenderer`/`TilesRenderer` are all-or-nothing per method. The
|
||||
Signed deltas (window controls, prev/next, plain-sidebar detection, i18n)
|
||||
live *inside* `render_tab_bar` and `frame`. There is no hook below the whole
|
||||
method, so "delegate and keep only the deltas" has no seam to hang on.
|
||||
- Composing `Rc<DockSkin>` would still leave `render_tab_bar` a near-full
|
||||
reimplementation while adding a dependency on upstream internals, for no
|
||||
line reduction.
|
||||
- [x] Effort/risk: high effort, high churn, no achievable reduction on this rev.
|
||||
`SignedTilesSkin` is the same shape. Recommend keeping the fork as-is. A
|
||||
future upstream change (public `DockSkin` with per-part hooks) would be the
|
||||
precondition for any delegation.
|
||||
|
||||
---
|
||||
|
||||
## Phase 4 - triage flagged items
|
||||
|
||||
Verify each, then delete or dismiss. These were reported by the audit but not
|
||||
independently reproduced.
|
||||
|
||||
- [ ] `signed_git/src/worktree.rs` - manual `refs/heads/{rev}` fallback; check
|
||||
whether gix's ref DWIM already covers it.
|
||||
- [ ] `signed_git/src/repo.rs` - `refs_with_prefix`; check
|
||||
`repo.references()?.prefixed(prefix)`.
|
||||
- [ ] `signed_git/src/patch.rs` - the mbox envelope is scanned twice; check
|
||||
whether `patch_commits` can consume `split_patch_series` output, and
|
||||
whether the hard-coded 40-hex checks should use `gix::ObjectId::from_hex`.
|
||||
- [ ] `assets/src/lib.rs` - `themes` handles a `Cow::Owned` case the build
|
||||
features cannot produce.
|
||||
- [ ] `signed_state/src/backend.rs` - `GraspServerResult::git_url` populated but
|
||||
never read.
|
||||
- [ ] `signed_nostr/src/signer.rs` - `UniversalSignerError` vs
|
||||
`nostr::Error::other`. Keep the `InnerSigner` erasure shim; only the error
|
||||
wrapper is replaceable.
|
||||
- [ ] `signed_git/src/history.rs` - `last_commit` referenced only from tests.
|
||||
- [ ] `signed_git/src/repo.rs` - `init_repository` / `root_commit` 40-length
|
||||
guards on an `ObjectId` string.
|
||||
- [ ] `signed_core/src/model.rs` - `Upstream.relay_hint` parsed but unused in
|
||||
production.
|
||||
- [ ] `signed_core/src/inbox.rs` - `root_kind` duplicates `root_event`; confirm
|
||||
before removing, it is read by the inbox view.
|
||||
|
||||
---
|
||||
|
||||
## Do not touch
|
||||
|
||||
- The two `pull_requests` generation counters (`load_generation` in
|
||||
`detail.rs`, `compare_generation` in `new.rs`). Both were verified reachable.
|
||||
- Tasks stored in a `Vec<Task<..>>` for lifetime cancellation. This is
|
||||
intentional.
|
||||
- `RefreshGate` on `CheckoutsStore`; its timer-driven debounce is load-bearing.
|
||||
- The `dev`-time `init_dialog.rs` `.detach()`; the task owns a window-scoped
|
||||
dialog and has no owning struct.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- Every removed symbol returns empty for a repo-wide grep.
|
||||
- No new `#[allow(dead_code)]`.
|
||||
- `cargo fmt -- --check` diff-free, `cargo check --offline --workspace
|
||||
--all-targets` clean, `cargo clippy --offline --workspace --all-targets`
|
||||
clean, `cargo test --offline --workspace` green.
|
||||
- Manual smoke: open and close docks, watch the inbox update live, open a repo
|
||||
and switch branches, publish a repo.
|
||||
|
||||
## Suggested commit sequence
|
||||
|
||||
1. `signed_state`: 1.1, 1.4, 1.5 (dead code), plus 1.8 comments in the same files.
|
||||
2. `signed_core`: 1.2, 1.3 (dead modules).
|
||||
3. `signed_ui` + `utils`: 1.6, 1.7.
|
||||
4. `dock`: 2.1, 2.4.
|
||||
5. `signed_state`: 2.2.
|
||||
6. `workspace`: 2.3.
|
||||
7. Phases 3 and 4 as separate, individually reviewed changes.
|
||||
Reference in New Issue
Block a user