From a051cb165e5242646bdf92ca03088abb528da368 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 13 Sep 2026 17:11:26 +0700 Subject: [PATCH 01/11] remove dead code --- crates/signed_core/src/annotations.rs | 241 -------------------- crates/signed_core/src/clone_url.rs | 87 -------- crates/signed_core/src/filters.rs | 12 - crates/signed_core/src/lib.rs | 4 +- crates/signed_state/src/backend.rs | 8 - crates/signed_state/src/profile.rs | 2 +- crates/signed_state/src/repo.rs | 14 -- crates/signed_ui/src/dropdown_button.rs | 31 +-- crates/utils/src/pubkey.rs | 11 +- docs/TODO.md | 19 ++ docs/over-engineering-cleanup-plan.md | 283 ++++++++++++++++++++++++ 11 files changed, 320 insertions(+), 392 deletions(-) delete mode 100644 crates/signed_core/src/clone_url.rs create mode 100644 docs/TODO.md create mode 100644 docs/over-engineering-cleanup-plan.md diff --git a/crates/signed_core/src/annotations.rs b/crates/signed_core/src/annotations.rs index e45365d..0a49935 100644 --- a/crates/signed_core/src/annotations.rs +++ b/crates/signed_core/src/annotations.rs @@ -5,244 +5,3 @@ use nostr::prelude::*; /// A markdown note attached to an issue, patch or PR by its author or a maintainer, /// not part of the NIP-34 draft, read support for interop. pub const COVER_NOTE_KIND: Kind = Kind::Custom(1624); - -/// Whether a kind-1985 label event is a valid annotation of `root`. -/// -/// The event references the root with a lowercase `e` tag, -/// its author must be the root author or a maintainer. -fn label_targets_root(event: &Event, root: &Event, maintainers: &[PublicKey]) -> bool { - if event.kind != Kind::Label { - return false; - } - if event.pubkey != root.pubkey && !maintainers.contains(&event.pubkey) { - return false; - } - let root_id = root.id.to_hex(); - event - .tags - .iter() - .any(|tag| tag.kind() == "e" && tag.content().is_some_and(|content| content == root_id)) -} - -/// Whether a kind-1985 label event declares the `#t` namespace, -/// it must also carry at least one `["l", "", "#t"]` label. -fn has_hashtag_labels(event: &Event) -> bool { - event.tags.iter().any(|tag| tag.as_slice() == ["L", "#t"]) - && event.tags.iter().any(|tag| { - let slice = tag.as_slice(); - slice.len() >= 3 && slice[0] == "l" && slice[2] == "#t" && !slice[1].is_empty() - }) -} - -/// Effective hashtag labels of `root`, -/// the `t` tags on the event itself, self-reported by its author, -/// authorized NIP-32 kind-1985 events in the `#t` namespace add more. -/// -/// Labels are additive, so all valid label events contribute, -/// there is no latest-wins semantics. -pub fn labels(root: &Event, label_events: &[Event], maintainers: &[PublicKey]) -> Vec { - let mut labels: Vec = root - .tags - .hashtags() - .map(|hashtag| hashtag.to_string()) - .collect(); - - for event in label_events { - if !label_targets_root(event, root, maintainers) || !has_hashtag_labels(event) { - continue; - } - for tag in event.tags.iter() { - let slice = tag.as_slice(); - if slice.len() >= 3 && slice[0] == "l" && slice[2] == "#t" && !slice[1].is_empty() { - let label = &slice[1]; - if !labels.contains(label) { - labels.push(label.clone()); - } - } - } - } - - labels -} - -/// Subject or title override of `root` from authorized kind-1985 label events, -/// only label events in the `#subject` namespace count. -/// -/// Returns `None` when no valid override exists. -pub fn subject_override( - root: &Event, - label_events: &[Event], - maintainers: &[PublicKey], -) -> Option { - label_events - .iter() - .filter(|event| label_targets_root(event, root, maintainers)) - .filter(|event| { - event - .tags - .iter() - .any(|tag| tag.as_slice() == ["L", "#subject"]) - && event.tags.iter().any(|tag| { - let slice = tag.as_slice(); - slice.len() >= 3 - && slice[0] == "l" - && slice[2] == "#subject" - && !slice[1].is_empty() - }) - }) - .max_by(|a, b| { - a.created_at - .cmp(&b.created_at) - .then_with(|| a.id.to_string().cmp(&b.id.to_string())) - }) - .and_then(|event| { - event.tags.iter().find_map(|tag| { - let slice = tag.as_slice(); - (slice.len() >= 3 - && slice[0] == "l" - && slice[2] == "#subject" - && !slice[1].is_empty()) - .then(|| slice[1].clone()) - }) - }) -} - -/// Effective hashtag labels and subject override of `root` in one pass, -/// mirrors ngit's `get_labels_and_subject`. -pub fn labels_and_subject( - root: &Event, - label_events: &[Event], - maintainers: &[PublicKey], -) -> (Vec, Option) { - ( - labels(root, label_events, maintainers), - subject_override(root, label_events, maintainers), - ) -} - -/// Effective cover note of `root`. -/// -/// Returns `None` when no valid cover note exists. -pub fn cover_note<'a>( - root: &Event, - cover_notes: &'a [Event], - maintainers: &[PublicKey], -) -> Option<&'a Event> { - let root_id = root.id.to_hex(); - - cover_notes - .iter() - .filter(|event| { - event.kind == COVER_NOTE_KIND - && (event.pubkey == root.pubkey || maintainers.contains(&event.pubkey)) - && event.tags.iter().any(|tag| { - tag.kind() == "e" && tag.content().is_some_and(|content| content == root_id) - }) - }) - .max_by(|a, b| { - a.created_at - .cmp(&b.created_at) - .then_with(|| a.id.to_string().cmp(&b.id.to_string())) - }) -} - -#[cfg(test)] -mod tests { - use super::*; - - fn keys_from_hex(hex: &str) -> Keys { - Keys::new(SecretKey::from_hex(hex).expect("valid secret key")) - } - - fn signed(author: &Keys, kind: Kind, tags: Vec, created_at: u64) -> Event { - EventBuilder::new(kind, "") - .tags(tags) - .custom_created_at(Timestamp::from(created_at)) - .finalize(author) - .expect("signed event") - } - - fn root_event() -> Event { - signed( - &keys_from_hex("0000000000000000000000000000000000000000000000000000000000000001"), - Kind::GitIssue, - vec![Tag::hashtag("bug")], - 100, - ) - } - - fn e_tag(event: &Event) -> Tag { - Tag::parse(["e", &event.id.to_hex()]).expect("valid e tag") - } - - #[test] - fn labels_take_inline_hashtags_and_external_label_events() { - let root = root_event(); - let maintainer = - keys_from_hex("0000000000000000000000000000000000000000000000000000000000000002"); - let labels_event = signed( - &maintainer, - Kind::Label, - vec![ - e_tag(&root), - Tag::parse(["L", "#t"]).expect("valid L tag"), - Tag::parse(["l", "help-wanted", "#t"]).expect("valid l tag"), - ], - 200, - ); - - let labels = labels(&root, &[labels_event], &[maintainer.public_key()]); - - assert_eq!(labels, vec!["bug", "help-wanted"]); - } - - #[test] - fn subject_override_latest_authorized_event_wins() { - let root = root_event(); - let maintainer = - keys_from_hex("0000000000000000000000000000000000000000000000000000000000000002"); - let older = signed( - &maintainer, - Kind::Label, - vec![ - e_tag(&root), - Tag::parse(["L", "#subject"]).expect("valid L tag"), - Tag::parse(["l", "Old title", "#subject"]).expect("valid l tag"), - ], - 200, - ); - let newer = signed( - &maintainer, - Kind::Label, - vec![ - e_tag(&root), - Tag::parse(["L", "#subject"]).expect("valid L tag"), - Tag::parse(["l", "New title", "#subject"]).expect("valid l tag"), - ], - 300, - ); - - assert_eq!( - subject_override(&root, &[newer, older], &[maintainer.public_key()]), - Some("New title".to_owned()) - ); - } - - #[test] - fn cover_note_latest_authorized_event_wins() { - let root = root_event(); - let maintainer = - keys_from_hex("0000000000000000000000000000000000000000000000000000000000000002"); - let stranger = - keys_from_hex("0000000000000000000000000000000000000000000000000000000000000003"); - let older = signed(&maintainer, COVER_NOTE_KIND, vec![e_tag(&root)], 200); - let newer = signed(&maintainer, COVER_NOTE_KIND, vec![e_tag(&root)], 300); - let unauthorized = signed(&stranger, COVER_NOTE_KIND, vec![e_tag(&root)], 400); - - let newer_id = newer.id; - let events = [older, unauthorized, newer]; - let maintainers = [maintainer.public_key()]; - let note = cover_note(&root, &events, &maintainers); - assert_eq!(note.map(|event| event.id), Some(newer_id)); - } -} diff --git a/crates/signed_core/src/clone_url.rs b/crates/signed_core/src/clone_url.rs deleted file mode 100644 index b66bf66..0000000 --- a/crates/signed_core/src/clone_url.rs +++ /dev/null @@ -1,87 +0,0 @@ -use nostr::prelude::*; - -use crate::RepoAddr; - -/// Target of a `nostr://` clone URL, as defined by NIP-34. -#[derive(Debug, Clone, PartialEq, Eq)] -pub enum CloneTarget { - /// `nostr://` encodes a direct repository address. - Addr(RepoAddr), - /// `nostr:///[relay-hint/]` - UserRepo { - /// `npub1...` or a NIP-05 identifier. - user: String, - relay_hint: Option, - /// `d` tag identifier of the repository. - identifier: String, - }, -} - -/// Parse a `nostr://` clone URL. Returns `None` for other URL schemes. -pub fn parse_clone_url(url: &str) -> Option { - let rest = url.strip_prefix("nostr://")?; - let mut parts = rest.split('/'); - - let first = parts.next()?; - let second = parts.next()?; - let third = parts.next(); - - if first.starts_with("naddr1") { - let coordinate = Nip19Coordinate::from_bech32(first).ok()?; - return Some(CloneTarget::Addr(coordinate.coordinate)); - } - - let (relay_hint, identifier) = match third { - Some(id) => ( - RelayUrl::parse(&percent_decode(second)).ok(), - percent_decode(id), - ), - None => (None, percent_decode(second)), - }; - - Some(CloneTarget::UserRepo { - user: first.to_owned(), - relay_hint, - identifier, - }) -} - -fn percent_decode(input: &str) -> String { - let bytes = input.as_bytes(); - let mut out = Vec::with_capacity(bytes.len()); - let mut i = 0; - while i < bytes.len() { - if bytes[i] == b'%' && i + 2 < bytes.len() { - let hex = &input[i + 1..i + 3]; - if let Ok(v) = u8::from_str_radix(hex, 16) { - out.push(v); - i += 3; - continue; - } - } - out.push(bytes[i]); - i += 1; - } - String::from_utf8_lossy(&out).into_owned() -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn decodes_percent_encoded_parts() { - let target = parse_clone_url( - "nostr://danconwaydev.com/ws%3A%2F%2Flocalhost%3A7334/my-local-only-repo", - ) - .unwrap(); - assert_eq!( - target, - CloneTarget::UserRepo { - user: "danconwaydev.com".to_owned(), - relay_hint: RelayUrl::parse("ws://localhost:7334").ok(), - identifier: "my-local-only-repo".to_owned(), - } - ); - } -} diff --git a/crates/signed_core/src/filters.rs b/crates/signed_core/src/filters.rs index 04a6f18..829a0ab 100644 --- a/crates/signed_core/src/filters.rs +++ b/crates/signed_core/src/filters.rs @@ -91,18 +91,6 @@ pub fn statuses_for(roots: impl IntoIterator) -> Filter { .events(roots) } -/// Cover notes and NIP-32 label events referencing any of the given root events. -/// These are kinds 1624 and 1985, matched via the `#e` tag. -/// -/// Because they carry no repository `a` tag, they are fetched by root like comments. -/// -/// Batched, like [`statuses_for`]. -pub fn annotations_for(roots: impl IntoIterator) -> Filter { - Filter::new() - .kinds([crate::COVER_NOTE_KIND, Kind::Label]) - .events(roots) -} - /// A user's grasp list, kind `10317`. pub fn grasp_list(public_key: PublicKey) -> Filter { Filter::new() diff --git a/crates/signed_core/src/lib.rs b/crates/signed_core/src/lib.rs index e7ecb69..2bc3ca6 100644 --- a/crates/signed_core/src/lib.rs +++ b/crates/signed_core/src/lib.rs @@ -1,6 +1,5 @@ pub mod addr; pub mod annotations; -pub mod clone_url; pub mod deletions; pub mod filters; pub mod inbox; @@ -9,8 +8,7 @@ pub mod state; pub mod status; pub use addr::{RepoAddr, identifier_from_name, repo_addr}; -pub use annotations::{COVER_NOTE_KIND, cover_note, labels_and_subject, subject_override}; -pub use clone_url::{CloneTarget, parse_clone_url}; +pub use annotations::COVER_NOTE_KIND; pub use deletions::Deletions; pub use filters::{ NOTIFICATION_KINDS, authored_activity, is_git_activity, notification_comments, notifications, diff --git a/crates/signed_state/src/backend.rs b/crates/signed_state/src/backend.rs index 8873381..6d3cab4 100644 --- a/crates/signed_state/src/backend.rs +++ b/crates/signed_state/src/backend.rs @@ -1086,10 +1086,6 @@ impl Backend { self.signer.clone() } - pub fn pushing_repos(&self) -> Entity> { - self.pushing_repos.clone() - } - pub fn inbox(&self) -> Entity { self.inbox.clone() } @@ -1102,10 +1098,6 @@ impl Backend { self.passphrase_required } - pub fn emit_error(&mut self, message: impl Into, cx: &mut Context) { - cx.emit(BackendEvent::error(message)); - } - fn sync_inbox(&mut self, cx: &mut Context) { let client = self.client.clone(); let me = self.current_user; diff --git a/crates/signed_state/src/profile.rs b/crates/signed_state/src/profile.rs index d9ae4c4..2e25ac5 100644 --- a/crates/signed_state/src/profile.rs +++ b/crates/signed_state/src/profile.rs @@ -50,7 +50,7 @@ impl Profile { return SharedString::from(name.trim().to_owned()); } - SharedString::from(shorten_pubkey(self.public_key, 4)) + SharedString::from(shorten_pubkey(self.public_key)) } pub fn picture(&self) -> Option { diff --git a/crates/signed_state/src/repo.rs b/crates/signed_state/src/repo.rs index 4770353..395f1e2 100644 --- a/crates/signed_state/src/repo.rs +++ b/crates/signed_state/src/repo.rs @@ -56,10 +56,6 @@ pub struct RepoStore { /// Computed with [`Self::status_by_root`] on every refresh. open_issue_count: usize, open_pr_count: usize, - /// Incremented on every applied refresh. - /// - /// Views key their derived-data caches to it instead of recomputing on every render. - version: u64, pub last_error: Option, /// Non-fatal warning of the last action, if any. /// @@ -125,7 +121,6 @@ impl RepoStore { status_by_root: HashMap::new(), open_issue_count: 0, open_pr_count: 0, - version: 0, last_error: None, last_warning: None, last_push_warning: None, @@ -153,7 +148,6 @@ impl RepoStore { status_by_root: HashMap::new(), open_issue_count: 0, open_pr_count: 0, - version: 0, last_error: None, last_warning: None, last_push_warning: None, @@ -401,9 +395,6 @@ impl RepoStore { } } - // Cover notes, 1624, and label events, 1985, carry no `a` tag. - // Query them per root like comments and statuses. - // The events are only stored for interop and nothing displays them. sort_newest_first(&mut issues); sort_newest_first(&mut patches); sort_newest_first(&mut pull_requests); @@ -522,7 +513,6 @@ impl RepoStore { this.open_issue_count = open_issue_count; this.open_pr_count = open_pr_count; this.loaded = true; - this.version = this.version.wrapping_add(1); // Comments and statuses without an `a` tag. // None are addressed to the repository. @@ -580,10 +570,6 @@ impl RepoStore { status_of(&self.status_by_root, root) } - pub fn version(&self) -> u64 { - self.version - } - /// Number of open issues. /// /// Issues whose resolved status is [`RepoStatus::Open`]. diff --git a/crates/signed_ui/src/dropdown_button.rs b/crates/signed_ui/src/dropdown_button.rs index e14d37c..1a257b1 100644 --- a/crates/signed_ui/src/dropdown_button.rs +++ b/crates/signed_ui/src/dropdown_button.rs @@ -8,22 +8,18 @@ use gpui_component::menu::PopupMenu; use gpui_component::{ActiveTheme, Icon, IconName, Sizable, h_flex}; /// A split dropdown button built on `gpui_base::Popover`. -/// An action element with a separate caret trigger that opens a [`PopupMenu`]. -/// The action and the caret are caller-supplied elements, so the look stays in the app. -/// This component only owns the popover wiring. +/// An action element next to a caret that opens a [`PopupMenu`]. #[derive(IntoElement)] pub struct DropdownButton { id: ElementId, style: StyleRefinement, anchor: Anchor, action: Option, - caret: Option, menu: Option, } type MenuBuilder = Box) -> PopupMenu + 'static>; -type CaretBuilder = Box AnyElement>; impl DropdownButton { pub fn new(id: impl Into) -> Self { @@ -32,7 +28,6 @@ impl DropdownButton { style: StyleRefinement::default(), anchor: Anchor::TopRight, action: None, - caret: None, menu: None, } } @@ -54,14 +49,6 @@ impl DropdownButton { self.menu = Some(Box::new(builder)); self } - - /// Which corner of the caret the menu anchors to. - /// Defaults to [`Anchor::TopRight`], lining the menu's right edge up with the caret's. - #[allow(dead_code)] // API knob, current call sites use the default anchor. - pub fn anchor(mut self, anchor: impl Into) -> Self { - self.anchor = anchor.into(); - self - } } impl Styled for DropdownButton { @@ -91,28 +78,24 @@ impl RenderOnce for DropdownButton { let menu_state = window.use_keyed_state(popover_id.clone(), cx, |_, _| DropdownMenuState::default()); - let caret = self.caret.unwrap_or_else(|| { - let id = popover_id.clone(); - Box::new(move |is_open, _, cx| { - let caret = default_caret(id.clone(), cx); - let selected = caret.is_selected(); - caret.selected(selected || is_open).into_any_element() - }) - }); - h_flex() .id(self.id) .refine_style(&self.style) .gap_0p5() .when_some(self.action, |this, action| this.child(action)) .when_some(self.menu, |this, builder| { + let caret_id = popover_id.clone(); this.child( Popover::new(popover_id) .anchor(anchor) // The menu dismisses itself on outside click or Escape. // The subscription below closes the popover along with it. .overlay_closable(false) - .trigger_with(caret) + .trigger_with(move |is_open, _, cx| { + let caret = default_caret(caret_id.clone(), cx); + let selected = caret.is_selected(); + caret.selected(selected || is_open).into_any_element() + }) .content( move |_, window, cx| match menu_state.read(cx).menu.clone() { Some(menu) => menu, diff --git a/crates/utils/src/pubkey.rs b/crates/utils/src/pubkey.rs index 3b2d588..4e0d19f 100644 --- a/crates/utils/src/pubkey.rs +++ b/crates/utils/src/pubkey.rs @@ -1,7 +1,14 @@ use nostr::prelude::*; /// Shorten a [`PublicKey`] to `npub1abc...wxyz` form. -pub fn shorten_pubkey(public_key: PublicKey, len: usize) -> String { +pub fn shorten_pubkey(public_key: PublicKey) -> String { + const HEAD_CHARS: usize = 9; + const TAIL_CHARS: usize = 4; + let npub = public_key.to_bech32().unwrap(); - format!("{}...{}", &npub[..(len + 5)], &npub[npub.len() - len..]) + format!( + "{}...{}", + &npub[..HEAD_CHARS], + &npub[npub.len() - TAIL_CHARS..] + ) } diff --git a/docs/TODO.md b/docs/TODO.md new file mode 100644 index 0000000..a12edc4 --- /dev/null +++ b/docs/TODO.md @@ -0,0 +1,19 @@ +# TODO + +Deferred from `docs/over-engineering-cleanup-plan.md`. + +## `BackendEvent::SyncProgress` + +File: `crates/signed_state/src/backend.rs` + +The variant has no subscriber. Remove the variant, the `sync_progress` field and +its accessor, and the `progress_task` in `sync_bootstrap`. Keep the terminal +`Synced` emission. + +## `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`. diff --git a/docs/over-engineering-cleanup-plan.md b/docs/over-engineering-cleanup-plan.md new file mode 100644 index 0000000..2dcb908 --- /dev/null +++ b/docs/over-engineering-cleanup-plan.md @@ -0,0 +1,283 @@ +# 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 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>` 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` 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. + +- [ ] Delete the `if !open && !placement.is_bottom() { return div(); }` guard. +- [ ] Stop re-applying `.flex().flex_none().relative().overflow_hidden()` and the + per-placement width/height; base already applies them. +- [ ] Keep the closed-bottom strip height override, but confirm against base's + `CLOSED_BOTTOM_STRIP` that the intended height is `TAB_BAR_HEIGHT`. +- [ ] 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` + +- [ ] 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. + +- [ ] Delete the `REFRESH_DEBOUNCE` constant. +- [ ] In `refresh`, drop the spawned timer; call `run_refresh` directly after + `refresh.request()` returns `Schedule`, matching `RepoStore::refresh`. +- [ ] 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` + +- [ ] The only call sites pass `Left`, `Bottom`, `Right`. Collapse the `Center` + arms to `unreachable!()` or restructure so the match is exhaustive without + a dead branch. + +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` + +- [ ] Confirm the filter enum, visible-index rebuild, counts tuple, and + virtual-list resize are the same shape. +- [ ] If so, extract one small helper for the filtered index + counts + notify + decision and use it in both. + +### 3.2 Relay URL normalize/display + +Files: `crates/workspace/src/views/sidebar/settings_dialog.rs`, +`crates/workspace/src/views/sidebar/grasp_servers.rs` + +- [ ] Confirm both pairs do prepend-scheme, parse, dedupe, and host-display. +- [ ] Extract one normalize helper and one display helper. Decide the crate + (check whether `signed_ui` may depend on `nostr`). + +### 3.3 `crates/dock` vs the pinned `gpui_component` dock renderer + +Files: `crates/dock/src/*` vs the pinned rev's `crates/ui/src/dock/*` + +- [ ] Spike only: pick one part (`SignedTabGroupSkin` or `SignedTilesSkin`) and + determine whether it can delegate to the upstream `DockSkin` trait + implementation and keep only the Signed deltas (window controls in the tab + bar, plain-sidebar detection, prev/next, i18n). +- [ ] Report effort and risk before doing any replacement. Do not start a + rewrite of this crate in this cleanup. + +--- + +## 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>` 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. -- 2.54.0 From 2741ab6ac6b9a10f51d9233f9bb273904ee00f23 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 13 Sep 2026 17:14:45 +0700 Subject: [PATCH 02/11] clean up --- crates/dock/src/dock_area.rs | 22 ++++------------------ crates/dock/src/tab_panel.rs | 2 +- crates/signed_state/src/backend.rs | 4 ---- crates/workspace/src/views/inbox.rs | 7 +------ docs/over-engineering-cleanup-plan.md | 27 +++++++++++++++------------ 5 files changed, 21 insertions(+), 41 deletions(-) diff --git a/crates/dock/src/dock_area.rs b/crates/dock/src/dock_area.rs index 0195955..de017bd 100644 --- a/crates/dock/src/dock_area.rs +++ b/crates/dock/src/dock_area.rs @@ -15,7 +15,7 @@ use gpui_base::dock::{ }; use gpui_base::resize_handle; use gpui_component::scroll::ScrollbarMode; -use gpui_component::{ActiveTheme as _, Side, StyledExt as _}; +use gpui_component::{ActiveTheme as _, Side}; use crate::invalid_panel::InvalidPanel; use crate::tab_panel::SignedTabGroupSkin; @@ -155,27 +155,13 @@ impl DockAreaRenderer for SignedDockSkin { cx: &mut App, ) -> AnyElement { let placement = dock.placement(); - let open = dock.is_open(); - - // A closed left or right dock takes no space. - // A closed bottom dock keeps a strip so its tab bar stays clickable. - if !open && !placement.is_bottom() { - return div().into_any_element(); - } div() .flex() - .flex_none() + .size_full() .relative() - .overflow_hidden() - .map(|this| match placement { - DockPlacement::Left | DockPlacement::Right => this.h_flex().h_full().w(dock.size()), - DockPlacement::Bottom => this.w_full().h(dock.size()), - // Base never builds a dock for the centre. - DockPlacement::Center => this, - }) - // The closed bottom dock's strip is the tab bar itself, a full tab bar tall. - .when(!open && placement.is_bottom(), |this| { + // A closed bottom dock keeps a strip, and that strip is the tab bar. + .when(!dock.is_open() && placement.is_bottom(), |this| { this.h(TAB_BAR_HEIGHT) }) .child(content) diff --git a/crates/dock/src/tab_panel.rs b/crates/dock/src/tab_panel.rs index 1403ae8..774fd9c 100644 --- a/crates/dock/src/tab_panel.rs +++ b/crates/dock/src/tab_panel.rs @@ -198,7 +198,7 @@ impl SignedTabGroupSkin { DockPlacement::Bottom => area .layout(DockPlacement::Bottom) .and_then(|tree| left_top_group(tree.root())), - DockPlacement::Center => None, + DockPlacement::Center => return None, }; if designated != Some(group.node()) { return None; diff --git a/crates/signed_state/src/backend.rs b/crates/signed_state/src/backend.rs index 6d3cab4..3fe9696 100644 --- a/crates/signed_state/src/backend.rs +++ b/crates/signed_state/src/backend.rs @@ -1707,10 +1707,6 @@ async fn push_staged_to_grasps( ) -> PushOutcome { let mut outcome = PushOutcome::default(); - if refs.is_empty() { - return outcome; - } - for relay in servers { let Some(base) = grasp_base_url(relay) else { outcome.servers.push(GraspServerResult::failed( diff --git a/crates/workspace/src/views/inbox.rs b/crates/workspace/src/views/inbox.rs index 8ccc501..880a30f 100644 --- a/crates/workspace/src/views/inbox.rs +++ b/crates/workspace/src/views/inbox.rs @@ -1,6 +1,5 @@ use std::collections::HashMap; use std::sync::Arc; -use std::time::Duration; use anyhow::Error; use dock::{BasePanel, DockArea, Panel, PanelEvent}; @@ -21,7 +20,6 @@ use utils::relative_time; use super::{RepoItem, open_repo_item}; -const REFRESH_DEBOUNCE: Duration = Duration::from_millis(300); const LIST_OVERDRAW: Pixels = px(400.); const MAX_SUB_ACTIVITIES: usize = 5; @@ -196,10 +194,7 @@ impl InboxView { return; } - self.tasks.push(cx.spawn(async move |this, cx| { - cx.background_executor().timer(REFRESH_DEBOUNCE).await; - this.update(cx, |this, cx| this.run_refresh(cx)) - })); + self.run_refresh(cx); } fn run_refresh(&mut self, cx: &mut Context) { diff --git a/docs/over-engineering-cleanup-plan.md b/docs/over-engineering-cleanup-plan.md index 2dcb908..7a3940a 100644 --- a/docs/over-engineering-cleanup-plan.md +++ b/docs/over-engineering-cleanup-plan.md @@ -145,11 +145,14 @@ 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. -- [ ] Delete the `if !open && !placement.is_bottom() { return div(); }` guard. -- [ ] Stop re-applying `.flex().flex_none().relative().overflow_hidden()` and the - per-placement width/height; base already applies them. -- [ ] Keep the closed-bottom strip height override, but confirm against base's - `CLOSED_BOTTOM_STRIP` that the intended height is `TAB_BAR_HEIGHT`. +- [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. @@ -157,7 +160,7 @@ is precisely the condition of the early return here. File: `crates/signed_state/src/backend.rs` -- [ ] Delete the `if refs.is_empty() { return outcome; }` guard. +- [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. @@ -169,19 +172,19 @@ 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. -- [ ] Delete the `REFRESH_DEBOUNCE` constant. -- [ ] In `refresh`, drop the spawned timer; call `run_refresh` directly after +- [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`. -- [ ] Keep `RefreshGate` for fold/overlap. +- [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` -- [ ] The only call sites pass `Left`, `Bottom`, `Right`. Collapse the `Center` - arms to `unreachable!()` or restructure so the match is exhaustive without - a dead branch. +- [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. -- 2.54.0 From 4be75253cde57aead1b5e8328cfac18c60d06649 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 13 Sep 2026 17:18:54 +0700 Subject: [PATCH 03/11] update --- crates/workspace/src/views/issues/mod.rs | 37 +++++--------- crates/workspace/src/views/mod.rs | 1 + .../workspace/src/views/pull_requests/mod.rs | 49 ++++++------------- .../src/views/sidebar/grasp_servers.rs | 24 +++------ crates/workspace/src/views/sidebar/mod.rs | 21 ++++++++ .../src/views/sidebar/settings_dialog.rs | 22 +++------ crates/workspace/src/views/status_list.rs | 45 +++++++++++++++++ docs/over-engineering-cleanup-plan.md | 40 ++++++++++----- 8 files changed, 136 insertions(+), 103 deletions(-) create mode 100644 crates/workspace/src/views/status_list.rs diff --git a/crates/workspace/src/views/issues/mod.rs b/crates/workspace/src/views/issues/mod.rs index a3741ed..5100175 100644 --- a/crates/workspace/src/views/issues/mod.rs +++ b/crates/workspace/src/views/issues/mod.rs @@ -24,6 +24,7 @@ use utils::relative_time; pub(super) mod detail; use self::detail::IssueDetailView; +use super::status_list::{StatusCounts, filter_by_status}; const ISSUE_ROW_HEIGHT: f32 = 73.; @@ -52,7 +53,7 @@ pub struct IssuesView { filter: IssueFilter, item_sizes: Rc>>, visible_issues: Vec, - counts: (usize, usize, usize), + counts: StatusCounts, // A filter change notifies even when the visible rows are unchanged, // e.g. switching between two empty filters. synced_filter: IssueFilter, @@ -85,7 +86,7 @@ impl IssuesView { filter: IssueFilter::Open, item_sizes: Rc::new(Vec::new()), visible_issues: Vec::new(), - counts: (0, 0, 0), + counts: StatusCounts::default(), synced_filter: IssueFilter::Open, scroll_handle: VirtualListScrollHandle::new(), _subscription: subscription, @@ -97,25 +98,11 @@ impl IssuesView { let (visible_issues, counts) = { let store = self.store.read(cx); - let mut counts = (0usize, 0usize, 0usize); - - let visible_issues: Vec = store - .issues - .iter() - .enumerate() - .filter_map(|(ix, issue)| { - let status = store.status_of(issue); - counts.0 += 1; - match status { - RepoStatus::Open => counts.1 += 1, - RepoStatus::Closed => counts.2 += 1, - RepoStatus::Draft | RepoStatus::Applied => {} - } - filter.matches(status).then_some(ix) - }) - .collect(); - - (visible_issues, counts) + filter_by_status( + &store.issues, + |issue| store.status_of(issue), + |status| filter.matches(status), + ) }; let filter_changed = self.synced_filter != filter; @@ -218,7 +205,7 @@ impl IssuesView { } fn render_header(&self, cx: &mut Context) -> AnyElement { - let (total, open, closed) = self.counts; + let counts = self.counts; h_flex() .px_4() @@ -234,7 +221,7 @@ impl IssuesView { .child( SegmentButton::new("all", "All") .icon(Icon::new(CustomIconName::GitIssueDone)) - .count(total) + .count(counts.total) .selected(self.filter == IssueFilter::All) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = IssueFilter::All; @@ -244,7 +231,7 @@ impl IssuesView { .child( SegmentButton::new("open", "Open") .icon(Icon::new(CustomIconName::GitIssueOpen)) - .count(open) + .count(counts.open) .selected(self.filter == IssueFilter::Open) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = IssueFilter::Open; @@ -254,7 +241,7 @@ impl IssuesView { .child( SegmentButton::new("closed", "Closed") .icon(Icon::new(CustomIconName::GitIssueClosed)) - .count(closed) + .count(counts.closed) .selected(self.filter == IssueFilter::Closed) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = IssueFilter::Closed; diff --git a/crates/workspace/src/views/mod.rs b/crates/workspace/src/views/mod.rs index ba027ad..6c8caa1 100644 --- a/crates/workspace/src/views/mod.rs +++ b/crates/workspace/src/views/mod.rs @@ -8,6 +8,7 @@ mod repo; mod repo_list; mod send_patch; pub(crate) mod sidebar; +mod status_list; pub(crate) mod tree; pub use inbox::InboxView; diff --git a/crates/workspace/src/views/pull_requests/mod.rs b/crates/workspace/src/views/pull_requests/mod.rs index a1a4949..f27af31 100644 --- a/crates/workspace/src/views/pull_requests/mod.rs +++ b/crates/workspace/src/views/pull_requests/mod.rs @@ -25,6 +25,7 @@ pub(super) mod new; use self::detail::PullRequestDetailView; use self::new::open_new_pull_panel; use super::send_patch::open_send_patch_panel; +use super::status_list::{StatusCounts, filter_by_status}; use crate::views::repo::RepoAction; const ROW_HEIGHT: f32 = 73.; @@ -59,8 +60,7 @@ pub struct PullRequestsView { item_sizes: Rc>>, /// Indices into the store's `pull_requests` matching [`Self::filter`]. visible_prs: Vec, - /// Header counts `(total, open, closed, draft, merged)`. - counts: (usize, usize, usize, usize, usize), + counts: StatusCounts, // A filter change notifies even when the visible rows are unchanged, // e.g. switching between two empty filters. synced_filter: PullRequestFilter, @@ -93,7 +93,7 @@ impl PullRequestsView { filter: PullRequestFilter::Open, item_sizes: Rc::new(Vec::new()), visible_prs: Vec::new(), - counts: (0, 0, 0, 0, 0), + counts: StatusCounts::default(), synced_filter: PullRequestFilter::Open, scroll_handle: VirtualListScrollHandle::new(), _subscription: subscription, @@ -105,32 +105,15 @@ impl PullRequestsView { let (visible_prs, counts) = { let store = self.store.read(cx); - let mut counts = (0usize, 0usize, 0usize, 0usize, 0usize); - - let visible_prs: Vec = store + let roots = store .pull_requests .iter() - .enumerate() - .filter_map(|(ix, pr)| { - if pr.kind != Kind::GitPullRequest { - return None; - } - - let status = store.status_of(pr); - counts.0 += 1; - - match status { - RepoStatus::Open => counts.1 += 1, - RepoStatus::Closed => counts.2 += 1, - RepoStatus::Draft => counts.3 += 1, - RepoStatus::Applied => counts.4 += 1, - } - - filter.matches(status).then_some(ix) - }) - .collect(); - - (visible_prs, counts) + .filter(|pr| pr.kind == Kind::GitPullRequest); + filter_by_status( + roots, + |pr| store.status_of(pr), + |status| filter.matches(status), + ) }; let filter_changed = self.synced_filter != filter; @@ -236,7 +219,7 @@ impl PullRequestsView { } fn render_header(&self, cx: &mut Context) -> AnyElement { - let (total, open, closed, draft, merged) = self.counts; + let counts = self.counts; h_flex() .px_4() @@ -252,7 +235,7 @@ impl PullRequestsView { .child( SegmentButton::new("all", "All") .icon(Icon::new(CustomIconName::GitPullRequest)) - .count(total) + .count(counts.total) .selected(self.filter == PullRequestFilter::All) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = PullRequestFilter::All; @@ -262,7 +245,7 @@ impl PullRequestsView { .child( SegmentButton::new("open", "Open") .icon(Icon::new(CustomIconName::GitPullRequest)) - .count(open) + .count(counts.open) .selected(self.filter == PullRequestFilter::Open) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = PullRequestFilter::Open; @@ -272,7 +255,7 @@ impl PullRequestsView { .child( SegmentButton::new("closed", "Closed") .icon(Icon::new(CustomIconName::GitPullRequestClosed)) - .count(closed) + .count(counts.closed) .selected(self.filter == PullRequestFilter::Closed) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = PullRequestFilter::Closed; @@ -282,7 +265,7 @@ impl PullRequestsView { .child( SegmentButton::new("draft", "Draft") .icon(Icon::new(CustomIconName::GitPullRequestDraft)) - .count(draft) + .count(counts.draft) .selected(self.filter == PullRequestFilter::Draft) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = PullRequestFilter::Draft; @@ -292,7 +275,7 @@ impl PullRequestsView { .child( SegmentButton::new("merged", "Merged") .icon(Icon::new(CustomIconName::GitPullRequestMerged)) - .count(merged) + .count(counts.applied) .selected(self.filter == PullRequestFilter::Merged) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = PullRequestFilter::Merged; diff --git a/crates/workspace/src/views/sidebar/grasp_servers.rs b/crates/workspace/src/views/sidebar/grasp_servers.rs index 84cd0cf..2776b4c 100644 --- a/crates/workspace/src/views/sidebar/grasp_servers.rs +++ b/crates/workspace/src/views/sidebar/grasp_servers.rs @@ -8,6 +8,8 @@ use nostr::prelude::*; use settings::{DEFAULT_GRASP_SERVERS, GraspServersSettings}; use signed_state::Backend; +use super::{normalize_server, server_host}; + /// State of the grasp-server section of a publish dialog, so async results can be rendered. #[derive(Default)] pub struct GraspServersState { @@ -155,7 +157,7 @@ fn render_server_row( .text_color(cx.theme().muted_foreground) .text_sm() .rounded(cx.theme().radius) - .child(display_server(relay)), + .child(server_host(relay)), ) .child( Button::new(format!("remove-relay:{ix}")) @@ -174,14 +176,6 @@ fn render_server_row( ) } -/// Shows only the host, since grasp servers are entered without a scheme. -fn display_server(relay: &RelayUrl) -> SharedString { - relay - .domain() - .map(SharedString::from) - .unwrap_or_else(|| SharedString::from(relay.to_string())) -} - /// Accepts a bare host as well as a full URL. fn add_relay( state: &Entity, @@ -194,14 +188,8 @@ fn add_relay( return; } - let normalized = if value.contains("://") { - value.clone() - } else { - format!("wss://{value}") - }; - - match RelayUrl::parse(&normalized) { - Ok(relay) => { + match normalize_server(&value) { + Some((_, relay)) => { state.update(cx, |state, _| { state.error = None; if !state.grasp_servers.contains(&relay) { @@ -210,7 +198,7 @@ fn add_relay( }); input.update(cx, |input, cx| input.set_value("", window, cx)); } - Err(_) => { + None => { state.update(cx, |state, _| { state.error = Some(format!("Invalid grasp server URL: {value}").into()); }); diff --git a/crates/workspace/src/views/sidebar/mod.rs b/crates/workspace/src/views/sidebar/mod.rs index 6dbb3e9..2da0a3e 100644 --- a/crates/workspace/src/views/sidebar/mod.rs +++ b/crates/workspace/src/views/sidebar/mod.rs @@ -17,6 +17,7 @@ use gpui_base::Button as BaseButton; use gpui_component::button::{Button, ButtonVariants}; use gpui_component::input::InputState; use gpui_component::{ActiveTheme, Icon, IconName, Sizable, StyledExt, h_flex, v_flex}; +use nostr::prelude::RelayUrl; use signed_core::{Announcement, RepoAddr, identifier_from_name}; use signed_state::{ Backend, BackendEvent, CheckoutsStore, LocalReposStore, Profile, ProfileStore, RepoListStore, @@ -542,6 +543,26 @@ impl SidebarPanel { } } +/// Normalize a user-typed grasp server, adding a `wss://` scheme when none is given. +/// +/// Returns the text to store and the parsed relay URL, or `None` when it is not a valid relay URL. +pub(super) fn normalize_server(input: &str) -> Option<(String, RelayUrl)> { + let text = if input.contains("://") { + input.to_owned() + } else { + format!("wss://{input}") + }; + RelayUrl::parse(&text).ok().map(|relay| (text, relay)) +} + +/// The host of a relay URL, which is what the server lists show; the scheme is implied. +pub(super) fn server_host(relay: &RelayUrl) -> SharedString { + relay + .domain() + .map(SharedString::from) + .unwrap_or_else(|| SharedString::from(relay.to_string())) +} + fn pick_banner() -> SharedString { let num = SystemTime::now() .duration_since(UNIX_EPOCH) diff --git a/crates/workspace/src/views/sidebar/settings_dialog.rs b/crates/workspace/src/views/sidebar/settings_dialog.rs index a0822f5..51e4e9c 100644 --- a/crates/workspace/src/views/sidebar/settings_dialog.rs +++ b/crates/workspace/src/views/sidebar/settings_dialog.rs @@ -18,10 +18,11 @@ use gpui_component::{ ActiveTheme, IconName, IndexPath, Sizable, Theme, ThemeMode, ThemeRegistry, WindowExt, h_flex, v_flex, }; -use nostr::prelude::RelayUrl; use settings::{AppearanceMode, Settings, SettingsStore}; use signed_ui::{SelectOption, setting_block, setting_row}; +use super::{normalize_server, server_host}; + /// Looks up the option index used to seed a [`SelectState`]. fn selected_index(options: &[SelectOption], value: &str) -> Option { options @@ -454,13 +455,11 @@ fn grasp_server_editor( } /// Shows only the host, since grasp servers are entered without a scheme. -/// Matches how the publish dialogs display servers. fn display_server(server: &str) -> SharedString { - RelayUrl::parse(server) - .ok() - .and_then(|relay| relay.domain().map(|domain| domain.to_owned())) - .map(SharedString::from) - .unwrap_or_else(|| SharedString::from(server.to_owned())) + match normalize_server(server) { + Some((_, relay)) => server_host(&relay), + None => SharedString::from(server.to_owned()), + } } fn repositories_section( @@ -568,14 +567,9 @@ fn add_server(input: &Entity, window: &mut Window, cx: &mut App) { if value.is_empty() { return; } - let normalized = if value.contains("://") { - value - } else { - format!("wss://{value}") - }; - if RelayUrl::parse(&normalized).is_err() { + let Some((normalized, _)) = normalize_server(&value) else { return; - } + }; let store = SettingsStore::global(cx); store.update(cx, |store, cx| { diff --git a/crates/workspace/src/views/status_list.rs b/crates/workspace/src/views/status_list.rs new file mode 100644 index 0000000..83badd4 --- /dev/null +++ b/crates/workspace/src/views/status_list.rs @@ -0,0 +1,45 @@ +use nostr::prelude::Event; +use signed_core::RepoStatus; + +/// Root events counted by their resolved status. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub(crate) struct StatusCounts { + pub(crate) total: usize, + pub(crate) open: usize, + pub(crate) closed: usize, + pub(crate) draft: usize, + pub(crate) applied: usize, +} + +impl StatusCounts { + fn record(&mut self, status: RepoStatus) { + self.total += 1; + match status { + RepoStatus::Open => self.open += 1, + RepoStatus::Closed => self.closed += 1, + RepoStatus::Draft => self.draft += 1, + RepoStatus::Applied => self.applied += 1, + } + } +} + +/// Indices of `roots` whose status `keep` accepts, counting every root's status. +pub(crate) fn filter_by_status<'a>( + roots: impl IntoIterator, + status_of: impl Fn(&Event) -> RepoStatus, + keep: impl Fn(RepoStatus) -> bool, +) -> (Vec, StatusCounts) { + let mut counts = StatusCounts::default(); + + let visible = roots + .into_iter() + .enumerate() + .filter_map(|(index, root)| { + let status = status_of(root); + counts.record(status); + keep(status).then_some(index) + }) + .collect(); + + (visible, counts) +} diff --git a/docs/over-engineering-cleanup-plan.md b/docs/over-engineering-cleanup-plan.md index 7a3940a..d15b11a 100644 --- a/docs/over-engineering-cleanup-plan.md +++ b/docs/over-engineering-cleanup-plan.md @@ -199,30 +199,44 @@ Verify the duplication before extracting; each could be intentional. Files: `crates/workspace/src/views/pull_requests/mod.rs`, `crates/workspace/src/views/issues/mod.rs` -- [ ] Confirm the filter enum, visible-index rebuild, counts tuple, and - virtual-list resize are the same shape. -- [ ] If so, extract one small helper for the filtered index + counts + notify - decision and use it in both. +- [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` -- [ ] Confirm both pairs do prepend-scheme, parse, dedupe, and host-display. -- [ ] Extract one normalize helper and one display helper. Decide the crate - (check whether `signed_ui` may depend on `nostr`). +- [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` vs persisted `Vec`) 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/*` -- [ ] Spike only: pick one part (`SignedTabGroupSkin` or `SignedTilesSkin`) and - determine whether it can delegate to the upstream `DockSkin` trait - implementation and keep only the Signed deltas (window controls in the tab - bar, plain-sidebar detection, prev/next, i18n). -- [ ] Report effort and risk before doing any replacement. Do not start a - rewrite of this crate in this cleanup. +- [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` 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. --- -- 2.54.0 From 534d572154512c7103416c9a4e814756c638b718 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 13 Sep 2026 17:26:07 +0700 Subject: [PATCH 04/11] . --- crates/assets/src/lib.rs | 7 +- crates/signed_core/src/inbox.rs | 15 +- crates/signed_core/src/model.rs | 11 +- crates/signed_git/src/history.rs | 11 - crates/signed_git/src/lib.rs | 4 +- crates/signed_git/src/repo.rs | 12 +- crates/signed_git/src/tests.rs | 20 +- crates/signed_git/src/worktree.rs | 16 +- crates/signed_nostr/src/signer.rs | 55 ++--- crates/signed_state/src/backend.rs | 27 +-- crates/workspace/src/views/inbox.rs | 2 +- docs/TODO.md | 2 - docs/over-engineering-cleanup-plan.md | 300 -------------------------- 13 files changed, 54 insertions(+), 428 deletions(-) delete mode 100644 docs/over-engineering-cleanup-plan.md diff --git a/crates/assets/src/lib.rs b/crates/assets/src/lib.rs index ba31461..82dd078 100644 --- a/crates/assets/src/lib.rs +++ b/crates/assets/src/lib.rs @@ -39,12 +39,7 @@ impl Assets { .filter_map(|path| { let data = Self::get(path.as_ref())?; let name = path.strip_prefix("themes/").unwrap_or(path.as_ref()); - let content = match data.data { - std::borrow::Cow::Borrowed(bytes) => { - std::str::from_utf8(bytes).ok()?.to_owned() - } - std::borrow::Cow::Owned(bytes) => String::from_utf8(bytes).ok()?, - }; + let content = std::str::from_utf8(data.data.as_ref()).ok()?.to_owned(); Some((name.to_owned(), content)) }) .collect() diff --git a/crates/signed_core/src/inbox.rs b/crates/signed_core/src/inbox.rs index 138e7d6..582e9b0 100644 --- a/crates/signed_core/src/inbox.rs +++ b/crates/signed_core/src/inbox.rs @@ -19,8 +19,6 @@ pub struct InboxItem { pub root: EventId, /// The root event itself, when it is known locally. pub root_event: Option, - /// Kind of the root event, when it is known locally. - pub root_kind: Option, /// Repository the root belongs to, from the root's `a` tag. pub address: Option, /// Notification events directed at the user, newest first. @@ -45,13 +43,11 @@ impl InboxItem { } pub fn kind(&self) -> Option { - self.root_kind.or_else(|| { - self.root_event - .as_ref() - .or_else(|| self.own_events.first()) - .or_else(|| self.events.first()) - .map(|event| event.kind) - }) + self.root_event + .as_ref() + .or_else(|| self.own_events.first()) + .or_else(|| self.events.first()) + .map(|event| event.kind) } /// Timestamp of the newest event in the thread. @@ -202,7 +198,6 @@ where let mut item = InboxItem { root, - root_kind: root_event.as_ref().map(|event| event.kind), address: root_event .as_ref() .and_then(|event| event.tags.coordinates().next()), diff --git a/crates/signed_core/src/model.rs b/crates/signed_core/src/model.rs index b5bc37b..365864f 100644 --- a/crates/signed_core/src/model.rs +++ b/crates/signed_core/src/model.rs @@ -41,12 +41,10 @@ pub struct Upstream { /// Upstream repository coordinate when the `u` tag names a NIP-34 repository. /// `None` for the git-URL form. pub addr: Option, - /// Relay hint for the upstream, if the `u` tag carries one. - pub relay_hint: Option, } impl Upstream { - fn parse(raw: &str, relay_hint: Option<&str>) -> Self { + fn parse(raw: &str) -> Self { let coordinate = raw.split('|').next().unwrap_or(raw); let addr = coordinate .parse::() @@ -55,7 +53,6 @@ impl Upstream { Self { raw: raw.to_owned(), addr, - relay_hint: relay_hint.and_then(|hint| RelayUrl::parse(hint).ok()), } } @@ -317,7 +314,7 @@ impl Announcement { let values = tag.as_slice(); let raw = values.get(1).map(String::as_str).unwrap_or_default(); if !raw.is_empty() { - upstream = Some(Upstream::parse(raw, values.get(2).map(String::as_str))); + upstream = Some(Upstream::parse(raw)); } } } @@ -511,10 +508,6 @@ mod tests { upstream.raw, "30617:68d81165918100b7da43fc28f7d1fc12554466e1115886b9e7bb326f65ec4272:upstream|https://example.com/upstream.git" ); - assert_eq!( - upstream.relay_hint, - Some(RelayUrl::parse("wss://relay.example.com").expect("valid relay")) - ); assert_eq!( upstream.display().to_string(), "30617:68d81165918100b7da43fc28f7d1fc12554466e1115886b9e7bb326f65ec4272:upstream" diff --git a/crates/signed_git/src/history.rs b/crates/signed_git/src/history.rs index 5c8ca1f..f99a105 100644 --- a/crates/signed_git/src/history.rs +++ b/crates/signed_git/src/history.rs @@ -72,17 +72,6 @@ fn file_commit_with_description( }) } -/// Find the most recent commit that changed `rel`, a path relative to the worktree. -/// -/// `Ok(None)` when no commit touched the file, e.g. an untracked file. -pub fn last_commit(repo: &gix::Repository, rel: &Path) -> Result> { - let rel = rel.to_path_buf(); - Ok(last_commits(repo, std::slice::from_ref(&rel))? - .into_iter() - .next() - .map(|(_, commit)| commit)) -} - /// Newest commit touching each of `rels`, like `git log -1 -- ` per path. /// `rels` are paths relative to the worktree. /// diff --git a/crates/signed_git/src/lib.rs b/crates/signed_git/src/lib.rs index 19c668a..5d8dd65 100644 --- a/crates/signed_git/src/lib.rs +++ b/crates/signed_git/src/lib.rs @@ -16,8 +16,8 @@ pub use diff::{ worktree_commit_range_diff, }; pub use history::{ - CommitList, FileCommit, MAX_LISTED_COMMITS, all_commits, head_commit, last_commit, - worktree_all_commits, worktree_commit, worktree_commit_range_commits, worktree_last_commits, + CommitList, FileCommit, MAX_LISTED_COMMITS, all_commits, head_commit, worktree_all_commits, + worktree_commit, worktree_commit_range_commits, worktree_last_commits, }; pub use patch::{ apply_patch, format_patch_between, patch_commits, patch_diffs, split_patch_series, diff --git a/crates/signed_git/src/repo.rs b/crates/signed_git/src/repo.rs index 324b5d0..93d5998 100644 --- a/crates/signed_git/src/repo.rs +++ b/crates/signed_git/src/repo.rs @@ -1,6 +1,6 @@ use std::path::Path; -use anyhow::{Context, Result, bail}; +use anyhow::{Context, Result}; use crate::history::open_with_cache; use crate::worktree::{force_checkout, worktree_dirty}; @@ -171,12 +171,7 @@ pub fn init_repository(path: &Path, name: &str, description: &str) -> Result Result> { { let info = info?; if info.parent_ids().next().is_none() { - let id = info.id().to_string(); - return Ok((id.len() == 40).then_some(id)); + return Ok(Some(info.id().to_string())); } } diff --git a/crates/signed_git/src/tests.rs b/crates/signed_git/src/tests.rs index e157b47..890efb3 100644 --- a/crates/signed_git/src/tests.rs +++ b/crates/signed_git/src/tests.rs @@ -1,5 +1,5 @@ use std::collections::HashMap; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::process::Command; use super::*; @@ -578,7 +578,7 @@ fn git_run(dir: &Path, args: &[&str]) { } #[test] -fn last_commit_returns_most_recent_change() { +fn worktree_last_commits_returns_most_recent_change() { let (dir, repo) = fixture(&[("a.txt", b"one")]); commit_all(&repo, "initial"); @@ -589,9 +589,12 @@ fn last_commit_returns_most_recent_change() { std::fs::write(dir.path().join("b.txt"), b"other").expect("write"); commit_all(&repo, "add b"); - let commit = last_commit(&repo, Path::new("a.txt")) + let commit = worktree_last_commits(dir.path(), &[PathBuf::from("a.txt")]) .expect("lookup") - .expect("found"); + .into_iter() + .next() + .expect("found") + .1; assert_eq!(commit.summary, "change a"); assert_eq!(commit.author, "Test Author"); assert!(!commit.id.is_empty()); @@ -621,7 +624,7 @@ fn all_commits_lists_every_commit() { } #[test] -fn last_commit_reports_merge_commits() { +fn worktree_last_commits_reports_merge_commits() { let (dir, repo) = fixture(&[("a.txt", b"base")]); commit_all(&repo, "initial"); @@ -645,9 +648,12 @@ fn last_commit_reports_merge_commits() { // `--no-ff` forces a merge commit, it is the latest commit changing a.txt. run(&["merge", "--no-ff", "--no-edit", "feature"]); - let commit = last_commit(&repo, Path::new("a.txt")) + let commit = worktree_last_commits(dir.path(), &[PathBuf::from("a.txt")]) .expect("lookup") - .expect("found"); + .into_iter() + .next() + .expect("found") + .1; assert_eq!( commit.id, repo.head_id().expect("head").shorten_or_id().to_string() diff --git a/crates/signed_git/src/worktree.rs b/crates/signed_git/src/worktree.rs index 97a170a..dd68418 100644 --- a/crates/signed_git/src/worktree.rs +++ b/crates/signed_git/src/worktree.rs @@ -64,20 +64,10 @@ pub fn worktree_commits_ahead(workdir: &Path, base: &str, branch: &str) -> u32 { walk.filter_map(Result::ok).count().min(u32::MAX as usize) as u32 } -/// Resolve `rev` to a commit id, accepting full refs, -/// symbolic refs and the bare branch names callers pass, like git's DWIM. +/// Resolve `rev` to a commit id, accepting full refs or the bare branch names +/// callers pass. `gix`'s revision parser already applies git's ref DWIM. fn resolve_commit<'a>(repo: &'a gix::Repository, rev: &str) -> Option> { - if let Ok(id) = repo.rev_parse_single(rev.as_bytes()) { - return Some(id); - } - - // Branch names arrive bare, like git resolving `main`. - if rev.contains('/') { - return None; - } - - repo.rev_parse_single(format!("refs/heads/{rev}").as_bytes()) - .ok() + repo.rev_parse_single(rev.as_bytes()).ok() } /// Relative paths of all entries in the worktree, files and directories. diff --git a/crates/signed_nostr/src/signer.rs b/crates/signed_nostr/src/signer.rs index 6f00bf6..8ebf43e 100644 --- a/crates/signed_nostr/src/signer.rs +++ b/crates/signed_nostr/src/signer.rs @@ -5,32 +5,9 @@ use std::pin::Pin; use std::sync::{Arc, RwLock}; use nostr_connect::client::AuthUrlHandler; +use nostr_sdk::error::Error as SignerError; use nostr_sdk::prelude::*; -#[derive(Debug)] -pub struct UniversalSignerError(Box); - -impl fmt::Display for UniversalSignerError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}", self.0) - } -} - -impl Error for UniversalSignerError { - fn source(&self) -> Option<&(dyn Error + 'static)> { - Some(&*self.0) - } -} - -impl UniversalSignerError { - pub fn new(err: E) -> Self - where - E: Error + Send + Sync + 'static, - { - UniversalSignerError(Box::new(err)) - } -} - /// A type-erased signer whose inner signer can be swapped in-place. #[derive(Clone, Debug)] pub struct UniversalSigner { @@ -65,21 +42,21 @@ impl UniversalSigner { trait InnerSigner: fmt::Debug + Send + Sync + 'static { fn get_public_key_async( &self, - ) -> Pin> + Send + '_>>; + ) -> Pin> + Send + '_>>; fn sign_event_async( &self, unsigned: UnsignedEvent, - ) -> Pin> + Send + '_>>; + ) -> Pin> + Send + '_>>; fn nip44_encrypt_async<'a>( &'a self, public_key: &'a PublicKey, content: &'a str, - ) -> Pin> + Send + 'a>>; + ) -> Pin> + Send + 'a>>; fn nip44_decrypt_async<'a>( &'a self, public_key: &'a PublicKey, payload: &'a str, - ) -> Pin> + Send + 'a>>; + ) -> Pin> + Send + 'a>>; } #[derive(Debug)] @@ -94,22 +71,22 @@ where { fn get_public_key_async( &self, - ) -> Pin> + Send + '_>> { + ) -> Pin> + Send + '_>> { Box::pin(async move { AsyncGetPublicKey::get_public_key_async(&self.0) .await - .map_err(UniversalSignerError::new) + .map_err(SignerError::other) }) } fn sign_event_async( &self, unsigned: UnsignedEvent, - ) -> Pin> + Send + '_>> { + ) -> Pin> + Send + '_>> { Box::pin(async move { AsyncSignEvent::sign_event_async(&self.0, unsigned) .await - .map_err(UniversalSignerError::new) + .map_err(SignerError::other) }) } @@ -117,11 +94,11 @@ where &'a self, public_key: &'a PublicKey, content: &'a str, - ) -> Pin> + Send + 'a>> { + ) -> Pin> + Send + 'a>> { Box::pin(async move { AsyncNip44::nip44_encrypt_async(&self.0, public_key, content) .await - .map_err(UniversalSignerError::new) + .map_err(SignerError::other) }) } @@ -129,17 +106,17 @@ where &'a self, public_key: &'a PublicKey, payload: &'a str, - ) -> Pin> + Send + 'a>> { + ) -> Pin> + Send + 'a>> { Box::pin(async move { AsyncNip44::nip44_decrypt_async(&self.0, public_key, payload) .await - .map_err(UniversalSignerError::new) + .map_err(SignerError::other) }) } } impl AsyncGetPublicKey for UniversalSigner { - type Error = UniversalSignerError; + type Error = SignerError; fn get_public_key_async( &self, @@ -150,7 +127,7 @@ impl AsyncGetPublicKey for UniversalSigner { } impl AsyncSignEvent for UniversalSigner { - type Error = UniversalSignerError; + type Error = SignerError; fn sign_event_async( &self, @@ -162,7 +139,7 @@ impl AsyncSignEvent for UniversalSigner { } impl AsyncNip44 for UniversalSigner { - type Error = UniversalSignerError; + type Error = SignerError; fn nip44_encrypt_async<'a>( &'a self, diff --git a/crates/signed_state/src/backend.rs b/crates/signed_state/src/backend.rs index 3fe9696..3e6193c 100644 --- a/crates/signed_state/src/backend.rs +++ b/crates/signed_state/src/backend.rs @@ -1490,24 +1490,21 @@ const GRASP_RETRY_DELAY: Duration = Duration::from_secs(1); #[derive(Debug, Clone)] pub struct GraspServerResult { pub relay: RelayUrl, - pub git_url: String, /// `None` when the server accepted the data, the reason otherwise. pub reason: Option, } impl GraspServerResult { - fn ok(relay: RelayUrl, git_url: String) -> Self { + fn ok(relay: RelayUrl) -> Self { Self { relay, - git_url, reason: None, } } - fn failed(relay: RelayUrl, git_url: String, reason: impl Into) -> Self { + fn failed(relay: RelayUrl, reason: impl Into) -> Self { Self { relay, - git_url, reason: Some(reason.into()), } } @@ -1709,11 +1706,9 @@ async fn push_staged_to_grasps( for relay in servers { let Some(base) = grasp_base_url(relay) else { - outcome.servers.push(GraspServerResult::failed( - relay.clone(), - relay.to_string(), - "no domain", - )); + outcome + .servers + .push(GraspServerResult::failed(relay.clone(), "no domain")); continue; }; let git_url = format!("{base}/{owner}/{repo_id}.git"); @@ -1794,11 +1789,9 @@ async fn push_staged_to_grasps( log::warn!("grasp push failed: {relay}: {reason}"); outcome .servers - .push(GraspServerResult::failed(relay.clone(), git_url, reason)); + .push(GraspServerResult::failed(relay.clone(), reason)); } - None => outcome - .servers - .push(GraspServerResult::ok(relay.clone(), git_url)), + None => outcome.servers.push(GraspServerResult::ok(relay.clone())), } } @@ -1956,13 +1949,9 @@ mod tests { fn push_outcome_reports_partial_failures() { let outcome = PushOutcome { servers: vec![ - GraspServerResult::ok( - RelayUrl::parse("wss://gitnostr.com").expect("url"), - "https://gitnostr.com/npub1owner/repo.git".to_owned(), - ), + GraspServerResult::ok(RelayUrl::parse("wss://gitnostr.com").expect("url")), GraspServerResult::failed( RelayUrl::parse("wss://relay.ngit.dev").expect("url"), - "https://relay.ngit.dev/npub1owner/repo.git".to_owned(), "remote: ERR authorisation failed: No state events in purgatory\nfatal: ...", ), ], diff --git a/crates/workspace/src/views/inbox.rs b/crates/workspace/src/views/inbox.rs index 880a30f..fd062ad 100644 --- a/crates/workspace/src/views/inbox.rs +++ b/crates/workspace/src/views/inbox.rs @@ -403,7 +403,7 @@ impl InboxView { }; let root = item.root; - let kind = item.root_kind; + let kind = item.root_event.as_ref().map(|event| event.kind); let address = section.address.clone(); let first = entry_ix == 0; let last = entry_ix + 1 == section.entries.len(); diff --git a/docs/TODO.md b/docs/TODO.md index a12edc4..6bff069 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -1,7 +1,5 @@ # TODO -Deferred from `docs/over-engineering-cleanup-plan.md`. - ## `BackendEvent::SyncProgress` File: `crates/signed_state/src/backend.rs` diff --git a/docs/over-engineering-cleanup-plan.md b/docs/over-engineering-cleanup-plan.md deleted file mode 100644 index d15b11a..0000000 --- a/docs/over-engineering-cleanup-plan.md +++ /dev/null @@ -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 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>` 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` 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` vs persisted `Vec`) 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` 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>` 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. -- 2.54.0 From f5cc2ea60f97ca5471949a85cf6f5d87a30cebd6 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 13 Sep 2026 20:46:51 +0700 Subject: [PATCH 05/11] utils: Make pubkey shortening panic-free --- crates/utils/src/pubkey.rs | 47 +++++++++++++++++++++++++++++++++----- 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/crates/utils/src/pubkey.rs b/crates/utils/src/pubkey.rs index 4e0d19f..65b7b01 100644 --- a/crates/utils/src/pubkey.rs +++ b/crates/utils/src/pubkey.rs @@ -2,13 +2,48 @@ use nostr::prelude::*; /// Shorten a [`PublicKey`] to `npub1abc...wxyz` form. pub fn shorten_pubkey(public_key: PublicKey) -> String { + let encoded = public_key + .to_bech32() + .unwrap_or_else(|_| public_key.to_hex()); + + truncate_middle(&encoded) +} + +fn truncate_middle(value: &str) -> String { const HEAD_CHARS: usize = 9; const TAIL_CHARS: usize = 4; - let npub = public_key.to_bech32().unwrap(); - format!( - "{}...{}", - &npub[..HEAD_CHARS], - &npub[npub.len() - TAIL_CHARS..] - ) + let length = value.chars().count(); + if length <= HEAD_CHARS + TAIL_CHARS + 3 { + return value.to_owned(); + } + + let head: String = value.chars().take(HEAD_CHARS).collect(); + let tail: String = value.chars().skip(length - TAIL_CHARS).collect(); + + format!("{head}...{tail}") +} + +#[cfg(test)] +mod tests { + use super::*; + + const PUBLIC_KEY_HEX: &str = "68d81165918100b7da43fc28f7d1fc12554466e1115886b9e7bb326f65ec4272"; + + #[test] + fn shortens_a_valid_pubkey() { + let public_key = PublicKey::from_hex(PUBLIC_KEY_HEX).expect("valid pubkey"); + let npub = public_key.to_bech32().expect("valid pubkey encodes"); + + assert_eq!( + shorten_pubkey(public_key), + format!("{}...{}", &npub[..9], &npub[npub.len() - 4..]) + ); + } + + #[test] + fn leaves_short_values_intact() { + assert_eq!(truncate_middle("npub1short"), "npub1short"); + assert_eq!(truncate_middle("thirteenchars"), "thirteenchars"); + } } -- 2.54.0 From 7aee19f3aada81436db47c4f1d7186ea36d18850 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 13 Sep 2026 20:46:51 +0700 Subject: [PATCH 06/11] docs: Record over-optimization decisions --- docs/TODO.md | 7 +- docs/over-optimization-action-plan.md | 294 ++++++++++++++++++++++++++ 2 files changed, 298 insertions(+), 3 deletions(-) create mode 100644 docs/over-optimization-action-plan.md diff --git a/docs/TODO.md b/docs/TODO.md index 6bff069..6393d0b 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -4,9 +4,10 @@ File: `crates/signed_state/src/backend.rs` -The variant has no subscriber. Remove the variant, the `sync_progress` field and -its accessor, and the `progress_task` in `sync_bootstrap`. Keep the terminal -`Synced` emission. +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 diff --git a/docs/over-optimization-action-plan.md b/docs/over-optimization-action-plan.md new file mode 100644 index 0000000..5d93d30 --- /dev/null +++ b/docs/over-optimization-action-plan.md @@ -0,0 +1,294 @@ +# Over-optimization and over-engineering action plan + +- **Status:** phase 1 implemented; phases 2 and 3 still draft, for manual review +- **Date:** 2026-09-13 +- **Basis:** optimization inventory audit of branch `remove-ai-slop` (HEAD `534d572`) +- **Scope:** non-UI crates only — `signed_state`, `signed_git`, `signed_core`, `utils`, + `settings`, `paths`. +- **Moved out:** all UI-layer findings (workspace views, `signed_ui` components, dock chrome) + were removed from this plan and are tracked with the separate UI effort. See the appendix + for the mapping, so nothing is lost. +- **Owner decisions already made:** + - `BackendEvent::SyncProgress` **stays**. It is not removed by this plan. + - `Cargo.toml` build profiles and dependency feature selections are **out of scope**. + +Each task is written so it can be implemented and verified on its own. + +## Goals + +1. Remove speculative and dead machinery in the state/data layer that no behavior depends on. +2. Fix the places where an existing optimization is subtly incorrect or self-defeating. +3. Record, for every finding that is *not* acted on, why it is kept (see "Reviewed and kept"). + +## Non-goals + +- No UI work. Anything that changes rendering, view state, or widget behavior belongs to the + separate UI effort, even when the edit itself lands in a view file. +- No speculative performance work. Tasks in phase 3 are marked "measure first" and stay + optional until profiling or a concrete bug justifies them. +- No rewrite of the backend/store architecture. The coalescing model is kept; only its dead + state and one blocking sleep are touched. +- No dependency changes. In particular, no `smol`, no `ArcSwap`, no `indexmap`, no `dashmap`. + +## How to read a task + +- **Where** — files and current line numbers. Line numbers drift; re-locate symbols by name. +- **Size** — S (under an hour), M (half a day), L (a day or more). +- **Risk** — chance of a behavior change a user can notice. +- **Change** — what to do. +- **Acceptance** — how to know it is done. + +Implementers must follow the repository `.rules`: no `unwrap()` on fallible paths, propagate +errors where an operation can fail, full variable names, comments only for non-obvious "why", +and GPUI executor timers in tests when a test needs time to pass. + +## Baseline validation + +Run before and after every phase: + +```sh +cargo check --workspace +cargo test --workspace +``` + +Manual checks for phases 2 and 3 are in the validation section at the end. + +--- + +## Phase 1 — Trivial cleanups + +### OV-01 — Make `shorten_pubkey` panic-free and char-safe + +- **Where:** `crates/utils/src/pubkey.rs:4-14` +- **Evidence:** `to_bech32().unwrap()` and byte slicing `&npub[..9]` / `&npub[len-4..]`. + `crates/signed_ui/src/util.rs:4` has a char-safe `middle_truncate` with the same shape, but + moving it into `utils` would add a cross-crate dependency direction, so this task stays + local. +- **Change:** encode with `to_bech32().unwrap_or_else(|_| public_key.to_hex())` and truncate + by chars (collect to `Vec`, or iterate), leaving values too short for the ellipsis to + save space intact (the same `head + tail + ellipsis` guard as `signed_ui::middle_truncate`). + Output for normal keys must remain `npub1xxxx...yyyy`. +- **Acceptance:** add a unit test for a valid key and for a short fallback value; output of + the existing call site (`crates/signed_state/src/profile.rs:53`) is visibly unchanged. +- **Size:** S. **Risk:** low. +- **Status:** done 2026-09-13 — `crates/utils/src/pubkey.rs`, tests passing. + +### OV-02 — Align `docs/TODO.md` with the decision to keep `SyncProgress` + +- **Where:** `docs/TODO.md:3-9` +- **Evidence:** the TODO currently instructs a future session to delete the variant, the + `sync_progress` field, its accessor, and the progress task. That contradicts the owner + decision above. +- **Change:** replace that section with a short "kept intentionally" entry: the pipeline is + retained for a planned sync progress indicator; no subscriber exists yet. Do not touch the + `login`/`logout` section (separate decision). +- **Acceptance:** the file no longer instructs removal; `SyncProgress` code untouched. +- **Size:** S. **Risk:** low. +- **Status:** done 2026-09-13 — `docs/TODO.md` now marks the pipeline as kept intentionally. + +--- + +## Phase 2 — Runtime behavior + +### OV-03 — Simplify `RefreshGate` to running/dirty and give `CheckoutsStore` its own debounce flag + +- **Where:** `crates/signed_state/src/refresh.rs:1-56`; users at + `crates/signed_state/src/repos.rs:247,255,373,384`, + `crates/signed_state/src/repo.rs:300,312,455,554`, + `crates/signed_state/src/checkouts.rs:284,303,378,402,456,518`, and the one UI call site + `crates/workspace/src/views/inbox.rs:180-205,219,225,234`. +- **Evidence:** only `CheckoutsStore` ever leaves `debouncing` set across a call: it schedules + a 300 ms timer between `request()` and `begin()`. The other three users call `run_refresh` + synchronously, so the flag is set and cleared within one call. `inbox.rs:180` even asserts + the flag is false at its entry point. +- **Change:** + 1. Reduce `RefreshGate` to `running: bool` plus `dirty: bool`. `request()` returns + `Fold` while running, otherwise `Schedule`. Keep `running()`, `begin()`, `finish()` + (returns and clears `dirty`), `abort()` (keeps pending requests, matching today). + 2. Add `debounce_pending: bool` to `CheckoutsStore`. `refresh()` returns early when + `debounce_pending` or when `request()` returns `Fold`; otherwise it sets the flag and + spawns the existing `REFRESH_DEBOUNCE` timer. `run_refresh` clears the flag at its start. + `local_tick` and `run_local_statuses` check `refresh.running() || debounce_pending`. + 3. The other three users keep calling `run_refresh` directly after `Schedule`. + 4. The inbox view needs a mechanical adaptation only: delete the + `debug_assert!(!self.refresh.debouncing())` and replace it with a plain + `if self.refresh.running() { self.refresh.request(); return; }`. No rendering change. + If the UI effort is editing that file concurrently, coordinate the edit rather than + duplicating it. +- **Acceptance:** + - add unit tests in `refresh.rs` for: fold while running, follow-up after finish, abort + keeps the pending request, non-running request schedules; + - `cargo test -p signed_state`; + - manual: commit in a checkout, badges update within a few seconds; sidebar scan while + scanning coalesces; inbox refresh after a sync does not double-run. +- **Size:** M. **Risk:** medium. The debounce timing of `CheckoutsStore` must not regress; the + timer is preserved exactly, only its flag moves. + +### OV-04 — Replace the blocking sleep in the grasp push retry + +- **Where:** `crates/signed_state/src/backend.rs:1722-1726` + (`std::thread::sleep(GRASP_RETRY_DELAY)` inside `push_staged_to_grasps`) +- **Evidence:** the function is async and runs on GPUI's background executor. A blocking sleep + occupies a pool thread for up to two seconds per grasp server and can delay unrelated + background work. +- **Change:** await a GPUI executor timer for the same duration. `signed_state` has no + `smol` dependency and adding one is out of scope, but the executor is already reachable: + each caller captures `cx.background_executor().clone()` before `background_spawn` and + passes it into `push_staged_to_grasps` as a parameter, which then awaits + `executor.timer(GRASP_RETRY_DELAY)`. The accessor is already used this way at + `crates/signed_state/src/checkouts.rs:289`, and `BackgroundExecutor` is `Clone`. +- **Acceptance:** `cargo check -p signed_state`; a push that hits a transient denial still + retries with the same spacing. +- **Size:** S. **Risk:** low. + +--- + +## Phase 3 — Measure first (optional) + +These are optimization gaps, not over-engineering removals. Do not start any of them without +the measurement named in the task, and land them as separate PRs. + +### OV-05 — Batch the per-root status queries + +- **Where:** `crates/signed_state/src/repo.rs:390-396` issues one database query per root while + comments are batched at `:371-377`. `filters::statuses_for` already accepts many roots + (`crates/signed_core/src/filters.rs:83`). +- **Change:** collapse the loop into one query over all roots, mirroring the comment batching + above it. +- **Acceptance:** record refresh time on a repository with many issues/PRs before and after + the change; no change in resolved statuses (existing tests plus a manual comparison of a + repository with mixed open/closed/applied roots). +- **Size:** S. **Risk:** low. + +### OV-06 — Consolidate the per-checkout repository opens + +- **Where:** `crates/signed_state/src/checkouts.rs:598-668` calls five `signed_git` helpers per + checkout (`worktree_branches`, `worktree_dirty`, `worktree_current_branch`, + `head_commit_id`, `worktree_commits_ahead`), and each opens the repository separately. The + local poll runs every 2 s for up to `MAX_STATUS_CHECKOUTS` checkouts. +- **Change:** add a small `signed_git` API that opens the worktree once and returns the facts + the status computation needs (branches, dirty flag, current branch, head id, ahead count), + then use it from both status paths. Measure the poll cost before and after. +- **Acceptance:** checkouts tests in `crates/signed_state/src/checkouts.rs` pass; the + ready-to-push and ready-to-contribute statuses remain identical on a repository with a + dirty worktree, a clean feature branch, and a pushed branch. +- **Size:** M. **Risk:** medium (worktree state semantics). + +### OV-07 — Index URL matching in association resolution + +- **Where:** `crates/signed_state/src/checkouts.rs:573-593` is O(paths × announcements) with + repeated URL parsing on every full pass. +- **Change:** normalize and index the announcement clone URLs once per pass, then look up each + scanned origin instead of scanning all announcements. Keep the EUC match as a fallback. +- **Acceptance:** existing `resolve_associations` unit tests pass; record full-pass duration + on a large scan set before and after. +- **Size:** M. **Risk:** low to medium (URL identity rules are tested at `:754-777`). + +### OV-08 — Decide the commit total semantics + +- **Where:** `crates/signed_git/src/history.rs:159-190` caps materialization at + `MAX_LISTED_COMMITS` (20 000) but still walks all commits to compute `total`. +- **Change:** either accept the full walk and document it, or stop the walk at the cap and + expose whether the total is capped so callers can render `20000+`. The badge rendering + itself belongs to the UI effort; this task only changes the data layer and its contract. +- **Acceptance:** a decision is recorded here; if the cap is adopted, `CommitList` carries the + capped flag and the walk stops at the cap. Measure `worktree_all_commits` on the largest + available repository before deciding. +- **Size:** S to M. **Risk:** low. + +--- + +## Reviewed and kept (no action) + +These were flagged during the audit and reviewed; they should not be "fixed" without new +evidence: + +- `SyncProgress` pipeline — kept by owner decision (see OV-02). +- `Cargo.toml` release profile and dependency feature selection — out of scope. +- `GitCache` on-disk mirrors and `ensure_clone` fetch-on-open — core product behavior. +- The 64 MiB gix object cache for history walks and the plain opens for single-object reads. +- `CommitList` cap plus summary-only commits — memory bound is deliberate; the CPU question is + OV-08. +- Backend notification pump debounce and the day-quantized deletion filter — required for + sync dedup. +- `CheckoutsStore` polling design as a whole — revisit only via OV-06 and OV-07. +- `UniversalSigner` (`Arc>>`) — needed for in-place signer swap. +- Grasp transient-denial classification and the stale-advertisement convergence probe — + behavior justified by real races; only the blocking sleep is changed (OV-04). +- Debounced/batched profile sync (`ProfileStore`) and the typed filters in `signed_core`. + +## Validation + +Automated, per phase: + +```sh +cargo check --workspace +cargo test --workspace +``` + +Manual checks for the tasks that change runtime behavior: + +- **OV-03:** make a commit in a tracked checkout; the "ready to push" badge updates within a + few seconds. Push and confirm the badge clears without waiting for the next poll. Let a + repository scan overlap a manual rescan and confirm only one follow-up runs. +- **OV-04:** push to a grasp server that produces a transient denial (or simulate one) and + confirm the retry still happens at the same spacing and the push result is unchanged. +- **OV-05:** open a repository with many issues and PRs; statuses and counts match the + previous build. +- **OV-06/OV-07:** with several associated checkouts, confirm ready-to-push and + ready-to-contribute statuses match the previous build and the poll CPU cost drops. +- **OV-08:** open the Commits tab for a repository larger than the cap; the list behaves as + decided. + +## Suggested commit and PR breakdown + +Follow the repository PR hygiene rules: imperative titles, no conventional prefixes, a final +`Release Notes:` section with exactly one bullet. + +1. **`signed_state: Simplify refresh coalescing and stop blocking the executor`** + (OV-03, OV-04) + `Release Notes:` `- N/A` +2. **`utils: Make pubkey shortening panic-free`** (OV-01) + `Release Notes:` `- N/A` +3. **`docs: Record over-optimization decisions`** (OV-02 and the kept list) + `Release Notes:` `- N/A` + +OV-05 to OV-08 become separate PRs only after their measurement/decision step. + +## Appendix — moved UI tasks + +The following findings from the audit were removed from this plan and belong to the separate +UI effort. They are listed so they can be folded into that plan rather than lost. + +- Dock chrome: dead `add_bottom_panel`, unused visibility/scrollbar setters, `#[inline]` noise, + unreachable `Center` arm, duplicated `InvalidPanel` name, per-placement resize-handle + element ids (correctness), Linux window-controls decoration guard, per-frame layout walks, + never-set `tiles_scrollbar_mode`. +- Workspace views: file preview cache invariant drift, eager 1 MiB clone on cache hit, + `refs.rs` clone, repo header per-render announcement/share clones, eager share links, + About-dialog clone, PR row worktree clone, discussion participant dedup, Popular-sort + address recomputation, unbounded `tasks` vectors, duplicated close-panel helper, `defer_in` + for dock cleanup, PR alert state location, `open_pull_request` validation return value. +- `signed_ui`: per-render dropdown popover id allocation, pixel avatar RNG simplification. +- Retention/performance: `retain_all` image-cache memory profiling. + +Renumbering, for traceability from the first review round: + +| First-round ID | This plan | +|---|---| +| OV-08 | OV-01 | +| OV-09 | OV-02 | +| OV-10 | OV-03 | +| OV-11 | OV-04 | +| OV-30 | OV-05 | +| OV-31 | OV-06 | +| OV-32 | OV-07 | +| OV-34 | OV-08 | +| All other IDs | Moved to the UI effort (see above) | + +## Open questions for the owner + +1. OV-03: accept the two-flag `RefreshGate` plus a `CheckoutsStore`-owned debounce flag, or + keep the shared state machine as documentation-only? +2. OV-08: is an approximate (`20000+`) commit total acceptable for the badge contract? +3. OV-03: who owns the one mechanical `inbox.rs` edit — this plan or the UI effort? -- 2.54.0 From 22b96c354615384117186f81ddb280b4ee3e8977 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 13 Sep 2026 20:48:52 +0700 Subject: [PATCH 07/11] signed_state: Simplify refresh coalescing and stop blocking the executor --- crates/signed_state/src/backend.rs | 11 ++++- crates/signed_state/src/checkouts.rs | 15 +++++-- crates/signed_state/src/refresh.rs | 62 +++++++++++++++++++++------ crates/workspace/src/views/inbox.rs | 1 - docs/over-optimization-action-plan.md | 15 ++++--- 5 files changed, 79 insertions(+), 25 deletions(-) diff --git a/crates/signed_state/src/backend.rs b/crates/signed_state/src/backend.rs index 3e6193c..95d30e5 100644 --- a/crates/signed_state/src/backend.rs +++ b/crates/signed_state/src/backend.rs @@ -5,7 +5,7 @@ use std::time::{Duration, Instant}; use anyhow::{Error, anyhow, bail}; use bitcoin_hashes::sha1::Hash as Sha1Hash; -use gpui::{App, AppContext, Context, Entity, EventEmitter, Global, Task}; +use gpui::{App, AppContext, BackgroundExecutor, Context, Entity, EventEmitter, Global, Task}; use nostr::event::IntoEventBuilder; use nostr_connect::prelude::*; use nostr_sdk::client::SyncSummary; @@ -531,6 +531,7 @@ impl Backend { let repo_id = repo_id.clone(); let servers = servers.clone(); let refs = refs.clone(); + let executor = cx.background_executor().clone(); async move { push_staged_to_grasps( &client, @@ -541,6 +542,7 @@ impl Backend { &destination, &owner, &servers, + &executor, signed_git::push_main, ) .await @@ -686,6 +688,7 @@ impl Backend { let servers = servers.clone(); let refs = refs.clone(); let head = head.clone(); + let executor = cx.background_executor().clone(); async move { push_staged_to_grasps( &client, @@ -696,6 +699,7 @@ impl Backend { &path, &owner, &servers, + &executor, signed_git::push_all, ) .await @@ -854,6 +858,7 @@ impl Backend { let relays = relays.clone(); let refs = refs.clone(); let head = head.clone(); + let executor = cx.background_executor().clone(); async move { push_staged_to_grasps( &client, @@ -864,6 +869,7 @@ impl Backend { &path, &owner, &relays, + &executor, signed_git::push_all, ) .await @@ -1700,6 +1706,7 @@ async fn push_staged_to_grasps( path: &Path, owner: &str, servers: &[RelayUrl], + executor: &BackgroundExecutor, push: fn(&Path, &str, &str, &str) -> Result<(), Error>, ) -> PushOutcome { let mut outcome = PushOutcome::default(); @@ -1722,7 +1729,7 @@ async fn push_staged_to_grasps( 'server: for attempt in 1..=GRASP_PUSH_ATTEMPTS { if attempt > 1 { // Give the server's ingest a moment before re-staging. - std::thread::sleep(GRASP_RETRY_DELAY); + executor.timer(GRASP_RETRY_DELAY).await; } let (event, created_at) = diff --git a/crates/signed_state/src/checkouts.rs b/crates/signed_state/src/checkouts.rs index f7ed9ee..3f6dd50 100644 --- a/crates/signed_state/src/checkouts.rs +++ b/crates/signed_state/src/checkouts.rs @@ -93,6 +93,8 @@ pub struct CheckoutsStore { /// A recompute defaults the base the same way. requested_head: HashMap>, refresh: RefreshGate, + /// True while the timer between a scheduled refresh and its run is pending. + debounce_pending: bool, local_pending: bool, /// When the last full pass (with a remote refresh) completed. /// @@ -163,6 +165,7 @@ impl CheckoutsStore { push_statuses: HashMap::new(), requested_head: HashMap::new(), refresh: RefreshGate::default(), + debounce_pending: false, local_pending: false, last_full_sync: None, _subscriptions: subscriptions, @@ -279,12 +282,15 @@ impl CheckoutsStore { /// Re-resolve the associations and the requested statuses. /// - /// Requests arriving while a pass runs fold into a follow-up. + /// Requests arriving while a pass runs fold into a follow-up, requests + /// arriving while the debounce timer is pending are dropped. pub fn refresh(&mut self, cx: &mut Context) { - if self.refresh.request() != RefreshRequest::Schedule { + if self.debounce_pending || self.refresh.request() != RefreshRequest::Schedule { return; } + self.debounce_pending = true; + cx.spawn(async move |this, cx| { cx.background_executor().timer(REFRESH_DEBOUNCE).await; this.update(cx, |this, cx| this.run_refresh(cx)) @@ -300,6 +306,7 @@ impl CheckoutsStore { /// remote reconciliation cadence ([`Self::local_tick`]); they also restart /// the fast local pass. fn run_refresh(&mut self, cx: &mut Context) { + self.debounce_pending = false; self.refresh.begin(); let records = { @@ -453,7 +460,7 @@ impl CheckoutsStore { } // A full pass or a fresh request covers this tick, skip it. - if self.refresh.running() || self.refresh.debouncing() { + if self.refresh.running() || self.debounce_pending { self.schedule_local_pass(cx); return; } @@ -515,7 +522,7 @@ impl CheckoutsStore { this.update(cx, |this, cx| { // A full pass or a fresh request will apply fresher data // (the tracking refs move only when a full pass fetches). - if this.refresh.running() || this.refresh.debouncing() { + if this.refresh.running() || this.debounce_pending { return; } diff --git a/crates/signed_state/src/refresh.rs b/crates/signed_state/src/refresh.rs index d2564b9..e527e0f 100644 --- a/crates/signed_state/src/refresh.rs +++ b/crates/signed_state/src/refresh.rs @@ -3,14 +3,13 @@ pub struct RefreshGate { running: bool, dirty: bool, - debouncing: bool, } #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum RefreshRequest { - /// No run or timer covers the request, start the debounce timer. + /// No run covers the request, start one now. Schedule, - /// A run or pending timer already covers the request. + /// A run is in flight and covers the request, fold it into a follow-up. Fold, } @@ -19,28 +18,20 @@ impl RefreshGate { self.running } - pub fn debouncing(&self) -> bool { - self.debouncing - } - /// A new refresh request arrived. /// - /// Folded into a follow-up run while one is in flight, dropped while the - /// debounce timer is pending, otherwise starts the timer. + /// Folded into a follow-up run while one is in flight, otherwise the + /// caller starts the run itself. pub fn request(&mut self) -> RefreshRequest { if self.running { self.dirty = true; RefreshRequest::Fold - } else if self.debouncing { - RefreshRequest::Fold } else { - self.debouncing = true; RefreshRequest::Schedule } } pub fn begin(&mut self) { - self.debouncing = false; self.running = true; } @@ -55,3 +46,48 @@ impl RefreshGate { self.running = false; } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_request_while_running_folds_into_a_follow_up() { + let mut gate = RefreshGate::default(); + gate.begin(); + + assert_eq!(gate.request(), RefreshRequest::Fold); + assert!(gate.finish()); + } + + #[test] + fn a_request_without_a_run_schedules() { + let mut gate = RefreshGate::default(); + + assert_eq!(gate.request(), RefreshRequest::Schedule); + assert!(!gate.running()); + } + + #[test] + fn a_request_after_a_run_schedules_again() { + let mut gate = RefreshGate::default(); + gate.begin(); + assert_eq!(gate.request(), RefreshRequest::Fold); + assert!(gate.finish()); + + assert_eq!(gate.request(), RefreshRequest::Schedule); + } + + #[test] + fn abort_keeps_the_pending_request() { + let mut gate = RefreshGate::default(); + gate.begin(); + assert_eq!(gate.request(), RefreshRequest::Fold); + + gate.abort(); + assert!(!gate.running()); + + gate.begin(); + assert!(gate.finish()); + } +} diff --git a/crates/workspace/src/views/inbox.rs b/crates/workspace/src/views/inbox.rs index fd062ad..ad96522 100644 --- a/crates/workspace/src/views/inbox.rs +++ b/crates/workspace/src/views/inbox.rs @@ -177,7 +177,6 @@ impl InboxView { } fn refresh_initial(&mut self, cx: &mut Context) { - debug_assert!(!self.refresh.debouncing()); if self.refresh.running() { self.refresh.request(); return; diff --git a/docs/over-optimization-action-plan.md b/docs/over-optimization-action-plan.md index 5d93d30..ba0f447 100644 --- a/docs/over-optimization-action-plan.md +++ b/docs/over-optimization-action-plan.md @@ -1,6 +1,6 @@ # Over-optimization and over-engineering action plan -- **Status:** phase 1 implemented; phases 2 and 3 still draft, for manual review +- **Status:** phases 1 and 2 implemented; phase 3 still draft, for manual review - **Date:** 2026-09-13 - **Basis:** optimization inventory audit of branch `remove-ai-slop` (HEAD `534d572`) - **Scope:** non-UI crates only — `signed_state`, `signed_git`, `signed_core`, `utils`, @@ -123,6 +123,8 @@ Manual checks for phases 2 and 3 are in the validation section at the end. scanning coalesces; inbox refresh after a sync does not double-run. - **Size:** M. **Risk:** medium. The debounce timing of `CheckoutsStore` must not regress; the timer is preserved exactly, only its flag moves. +- **Status:** done 2026-09-13 — `RefreshGate` is `running`/`dirty` only, `CheckoutsStore` owns + `debounce_pending`, and the one-line `inbox.rs` adaptation landed here. ### OV-04 — Replace the blocking sleep in the grasp push retry @@ -140,6 +142,8 @@ Manual checks for phases 2 and 3 are in the validation section at the end. - **Acceptance:** `cargo check -p signed_state`; a push that hits a transient denial still retries with the same spacing. - **Size:** S. **Risk:** low. +- **Status:** done 2026-09-13 — `push_staged_to_grasps` takes a `BackgroundExecutor` and awaits + `executor.timer(GRASP_RETRY_DELAY)`. --- @@ -288,7 +292,8 @@ Renumbering, for traceability from the first review round: ## Open questions for the owner -1. OV-03: accept the two-flag `RefreshGate` plus a `CheckoutsStore`-owned debounce flag, or - keep the shared state machine as documentation-only? -2. OV-08: is an approximate (`20000+`) commit total acceptable for the badge contract? -3. OV-03: who owns the one mechanical `inbox.rs` edit — this plan or the UI effort? +1. OV-08: is an approximate (`20000+`) commit total acceptable for the badge contract? + +OV-03's questions were settled during implementation: the gate keeps two flags (`running`, +`dirty`), `CheckoutsStore` owns `debounce_pending`, and the mechanical `inbox.rs` edit landed +in this phase. -- 2.54.0 From 2414095725a61526f56748bffff389058ae0f4f6 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 13 Sep 2026 21:05:47 +0700 Subject: [PATCH 08/11] docs: Remove over-optimization action plan --- docs/over-optimization-action-plan.md | 299 -------------------------- 1 file changed, 299 deletions(-) delete mode 100644 docs/over-optimization-action-plan.md diff --git a/docs/over-optimization-action-plan.md b/docs/over-optimization-action-plan.md deleted file mode 100644 index ba0f447..0000000 --- a/docs/over-optimization-action-plan.md +++ /dev/null @@ -1,299 +0,0 @@ -# Over-optimization and over-engineering action plan - -- **Status:** phases 1 and 2 implemented; phase 3 still draft, for manual review -- **Date:** 2026-09-13 -- **Basis:** optimization inventory audit of branch `remove-ai-slop` (HEAD `534d572`) -- **Scope:** non-UI crates only — `signed_state`, `signed_git`, `signed_core`, `utils`, - `settings`, `paths`. -- **Moved out:** all UI-layer findings (workspace views, `signed_ui` components, dock chrome) - were removed from this plan and are tracked with the separate UI effort. See the appendix - for the mapping, so nothing is lost. -- **Owner decisions already made:** - - `BackendEvent::SyncProgress` **stays**. It is not removed by this plan. - - `Cargo.toml` build profiles and dependency feature selections are **out of scope**. - -Each task is written so it can be implemented and verified on its own. - -## Goals - -1. Remove speculative and dead machinery in the state/data layer that no behavior depends on. -2. Fix the places where an existing optimization is subtly incorrect or self-defeating. -3. Record, for every finding that is *not* acted on, why it is kept (see "Reviewed and kept"). - -## Non-goals - -- No UI work. Anything that changes rendering, view state, or widget behavior belongs to the - separate UI effort, even when the edit itself lands in a view file. -- No speculative performance work. Tasks in phase 3 are marked "measure first" and stay - optional until profiling or a concrete bug justifies them. -- No rewrite of the backend/store architecture. The coalescing model is kept; only its dead - state and one blocking sleep are touched. -- No dependency changes. In particular, no `smol`, no `ArcSwap`, no `indexmap`, no `dashmap`. - -## How to read a task - -- **Where** — files and current line numbers. Line numbers drift; re-locate symbols by name. -- **Size** — S (under an hour), M (half a day), L (a day or more). -- **Risk** — chance of a behavior change a user can notice. -- **Change** — what to do. -- **Acceptance** — how to know it is done. - -Implementers must follow the repository `.rules`: no `unwrap()` on fallible paths, propagate -errors where an operation can fail, full variable names, comments only for non-obvious "why", -and GPUI executor timers in tests when a test needs time to pass. - -## Baseline validation - -Run before and after every phase: - -```sh -cargo check --workspace -cargo test --workspace -``` - -Manual checks for phases 2 and 3 are in the validation section at the end. - ---- - -## Phase 1 — Trivial cleanups - -### OV-01 — Make `shorten_pubkey` panic-free and char-safe - -- **Where:** `crates/utils/src/pubkey.rs:4-14` -- **Evidence:** `to_bech32().unwrap()` and byte slicing `&npub[..9]` / `&npub[len-4..]`. - `crates/signed_ui/src/util.rs:4` has a char-safe `middle_truncate` with the same shape, but - moving it into `utils` would add a cross-crate dependency direction, so this task stays - local. -- **Change:** encode with `to_bech32().unwrap_or_else(|_| public_key.to_hex())` and truncate - by chars (collect to `Vec`, or iterate), leaving values too short for the ellipsis to - save space intact (the same `head + tail + ellipsis` guard as `signed_ui::middle_truncate`). - Output for normal keys must remain `npub1xxxx...yyyy`. -- **Acceptance:** add a unit test for a valid key and for a short fallback value; output of - the existing call site (`crates/signed_state/src/profile.rs:53`) is visibly unchanged. -- **Size:** S. **Risk:** low. -- **Status:** done 2026-09-13 — `crates/utils/src/pubkey.rs`, tests passing. - -### OV-02 — Align `docs/TODO.md` with the decision to keep `SyncProgress` - -- **Where:** `docs/TODO.md:3-9` -- **Evidence:** the TODO currently instructs a future session to delete the variant, the - `sync_progress` field, its accessor, and the progress task. That contradicts the owner - decision above. -- **Change:** replace that section with a short "kept intentionally" entry: the pipeline is - retained for a planned sync progress indicator; no subscriber exists yet. Do not touch the - `login`/`logout` section (separate decision). -- **Acceptance:** the file no longer instructs removal; `SyncProgress` code untouched. -- **Size:** S. **Risk:** low. -- **Status:** done 2026-09-13 — `docs/TODO.md` now marks the pipeline as kept intentionally. - ---- - -## Phase 2 — Runtime behavior - -### OV-03 — Simplify `RefreshGate` to running/dirty and give `CheckoutsStore` its own debounce flag - -- **Where:** `crates/signed_state/src/refresh.rs:1-56`; users at - `crates/signed_state/src/repos.rs:247,255,373,384`, - `crates/signed_state/src/repo.rs:300,312,455,554`, - `crates/signed_state/src/checkouts.rs:284,303,378,402,456,518`, and the one UI call site - `crates/workspace/src/views/inbox.rs:180-205,219,225,234`. -- **Evidence:** only `CheckoutsStore` ever leaves `debouncing` set across a call: it schedules - a 300 ms timer between `request()` and `begin()`. The other three users call `run_refresh` - synchronously, so the flag is set and cleared within one call. `inbox.rs:180` even asserts - the flag is false at its entry point. -- **Change:** - 1. Reduce `RefreshGate` to `running: bool` plus `dirty: bool`. `request()` returns - `Fold` while running, otherwise `Schedule`. Keep `running()`, `begin()`, `finish()` - (returns and clears `dirty`), `abort()` (keeps pending requests, matching today). - 2. Add `debounce_pending: bool` to `CheckoutsStore`. `refresh()` returns early when - `debounce_pending` or when `request()` returns `Fold`; otherwise it sets the flag and - spawns the existing `REFRESH_DEBOUNCE` timer. `run_refresh` clears the flag at its start. - `local_tick` and `run_local_statuses` check `refresh.running() || debounce_pending`. - 3. The other three users keep calling `run_refresh` directly after `Schedule`. - 4. The inbox view needs a mechanical adaptation only: delete the - `debug_assert!(!self.refresh.debouncing())` and replace it with a plain - `if self.refresh.running() { self.refresh.request(); return; }`. No rendering change. - If the UI effort is editing that file concurrently, coordinate the edit rather than - duplicating it. -- **Acceptance:** - - add unit tests in `refresh.rs` for: fold while running, follow-up after finish, abort - keeps the pending request, non-running request schedules; - - `cargo test -p signed_state`; - - manual: commit in a checkout, badges update within a few seconds; sidebar scan while - scanning coalesces; inbox refresh after a sync does not double-run. -- **Size:** M. **Risk:** medium. The debounce timing of `CheckoutsStore` must not regress; the - timer is preserved exactly, only its flag moves. -- **Status:** done 2026-09-13 — `RefreshGate` is `running`/`dirty` only, `CheckoutsStore` owns - `debounce_pending`, and the one-line `inbox.rs` adaptation landed here. - -### OV-04 — Replace the blocking sleep in the grasp push retry - -- **Where:** `crates/signed_state/src/backend.rs:1722-1726` - (`std::thread::sleep(GRASP_RETRY_DELAY)` inside `push_staged_to_grasps`) -- **Evidence:** the function is async and runs on GPUI's background executor. A blocking sleep - occupies a pool thread for up to two seconds per grasp server and can delay unrelated - background work. -- **Change:** await a GPUI executor timer for the same duration. `signed_state` has no - `smol` dependency and adding one is out of scope, but the executor is already reachable: - each caller captures `cx.background_executor().clone()` before `background_spawn` and - passes it into `push_staged_to_grasps` as a parameter, which then awaits - `executor.timer(GRASP_RETRY_DELAY)`. The accessor is already used this way at - `crates/signed_state/src/checkouts.rs:289`, and `BackgroundExecutor` is `Clone`. -- **Acceptance:** `cargo check -p signed_state`; a push that hits a transient denial still - retries with the same spacing. -- **Size:** S. **Risk:** low. -- **Status:** done 2026-09-13 — `push_staged_to_grasps` takes a `BackgroundExecutor` and awaits - `executor.timer(GRASP_RETRY_DELAY)`. - ---- - -## Phase 3 — Measure first (optional) - -These are optimization gaps, not over-engineering removals. Do not start any of them without -the measurement named in the task, and land them as separate PRs. - -### OV-05 — Batch the per-root status queries - -- **Where:** `crates/signed_state/src/repo.rs:390-396` issues one database query per root while - comments are batched at `:371-377`. `filters::statuses_for` already accepts many roots - (`crates/signed_core/src/filters.rs:83`). -- **Change:** collapse the loop into one query over all roots, mirroring the comment batching - above it. -- **Acceptance:** record refresh time on a repository with many issues/PRs before and after - the change; no change in resolved statuses (existing tests plus a manual comparison of a - repository with mixed open/closed/applied roots). -- **Size:** S. **Risk:** low. - -### OV-06 — Consolidate the per-checkout repository opens - -- **Where:** `crates/signed_state/src/checkouts.rs:598-668` calls five `signed_git` helpers per - checkout (`worktree_branches`, `worktree_dirty`, `worktree_current_branch`, - `head_commit_id`, `worktree_commits_ahead`), and each opens the repository separately. The - local poll runs every 2 s for up to `MAX_STATUS_CHECKOUTS` checkouts. -- **Change:** add a small `signed_git` API that opens the worktree once and returns the facts - the status computation needs (branches, dirty flag, current branch, head id, ahead count), - then use it from both status paths. Measure the poll cost before and after. -- **Acceptance:** checkouts tests in `crates/signed_state/src/checkouts.rs` pass; the - ready-to-push and ready-to-contribute statuses remain identical on a repository with a - dirty worktree, a clean feature branch, and a pushed branch. -- **Size:** M. **Risk:** medium (worktree state semantics). - -### OV-07 — Index URL matching in association resolution - -- **Where:** `crates/signed_state/src/checkouts.rs:573-593` is O(paths × announcements) with - repeated URL parsing on every full pass. -- **Change:** normalize and index the announcement clone URLs once per pass, then look up each - scanned origin instead of scanning all announcements. Keep the EUC match as a fallback. -- **Acceptance:** existing `resolve_associations` unit tests pass; record full-pass duration - on a large scan set before and after. -- **Size:** M. **Risk:** low to medium (URL identity rules are tested at `:754-777`). - -### OV-08 — Decide the commit total semantics - -- **Where:** `crates/signed_git/src/history.rs:159-190` caps materialization at - `MAX_LISTED_COMMITS` (20 000) but still walks all commits to compute `total`. -- **Change:** either accept the full walk and document it, or stop the walk at the cap and - expose whether the total is capped so callers can render `20000+`. The badge rendering - itself belongs to the UI effort; this task only changes the data layer and its contract. -- **Acceptance:** a decision is recorded here; if the cap is adopted, `CommitList` carries the - capped flag and the walk stops at the cap. Measure `worktree_all_commits` on the largest - available repository before deciding. -- **Size:** S to M. **Risk:** low. - ---- - -## Reviewed and kept (no action) - -These were flagged during the audit and reviewed; they should not be "fixed" without new -evidence: - -- `SyncProgress` pipeline — kept by owner decision (see OV-02). -- `Cargo.toml` release profile and dependency feature selection — out of scope. -- `GitCache` on-disk mirrors and `ensure_clone` fetch-on-open — core product behavior. -- The 64 MiB gix object cache for history walks and the plain opens for single-object reads. -- `CommitList` cap plus summary-only commits — memory bound is deliberate; the CPU question is - OV-08. -- Backend notification pump debounce and the day-quantized deletion filter — required for - sync dedup. -- `CheckoutsStore` polling design as a whole — revisit only via OV-06 and OV-07. -- `UniversalSigner` (`Arc>>`) — needed for in-place signer swap. -- Grasp transient-denial classification and the stale-advertisement convergence probe — - behavior justified by real races; only the blocking sleep is changed (OV-04). -- Debounced/batched profile sync (`ProfileStore`) and the typed filters in `signed_core`. - -## Validation - -Automated, per phase: - -```sh -cargo check --workspace -cargo test --workspace -``` - -Manual checks for the tasks that change runtime behavior: - -- **OV-03:** make a commit in a tracked checkout; the "ready to push" badge updates within a - few seconds. Push and confirm the badge clears without waiting for the next poll. Let a - repository scan overlap a manual rescan and confirm only one follow-up runs. -- **OV-04:** push to a grasp server that produces a transient denial (or simulate one) and - confirm the retry still happens at the same spacing and the push result is unchanged. -- **OV-05:** open a repository with many issues and PRs; statuses and counts match the - previous build. -- **OV-06/OV-07:** with several associated checkouts, confirm ready-to-push and - ready-to-contribute statuses match the previous build and the poll CPU cost drops. -- **OV-08:** open the Commits tab for a repository larger than the cap; the list behaves as - decided. - -## Suggested commit and PR breakdown - -Follow the repository PR hygiene rules: imperative titles, no conventional prefixes, a final -`Release Notes:` section with exactly one bullet. - -1. **`signed_state: Simplify refresh coalescing and stop blocking the executor`** - (OV-03, OV-04) - `Release Notes:` `- N/A` -2. **`utils: Make pubkey shortening panic-free`** (OV-01) - `Release Notes:` `- N/A` -3. **`docs: Record over-optimization decisions`** (OV-02 and the kept list) - `Release Notes:` `- N/A` - -OV-05 to OV-08 become separate PRs only after their measurement/decision step. - -## Appendix — moved UI tasks - -The following findings from the audit were removed from this plan and belong to the separate -UI effort. They are listed so they can be folded into that plan rather than lost. - -- Dock chrome: dead `add_bottom_panel`, unused visibility/scrollbar setters, `#[inline]` noise, - unreachable `Center` arm, duplicated `InvalidPanel` name, per-placement resize-handle - element ids (correctness), Linux window-controls decoration guard, per-frame layout walks, - never-set `tiles_scrollbar_mode`. -- Workspace views: file preview cache invariant drift, eager 1 MiB clone on cache hit, - `refs.rs` clone, repo header per-render announcement/share clones, eager share links, - About-dialog clone, PR row worktree clone, discussion participant dedup, Popular-sort - address recomputation, unbounded `tasks` vectors, duplicated close-panel helper, `defer_in` - for dock cleanup, PR alert state location, `open_pull_request` validation return value. -- `signed_ui`: per-render dropdown popover id allocation, pixel avatar RNG simplification. -- Retention/performance: `retain_all` image-cache memory profiling. - -Renumbering, for traceability from the first review round: - -| First-round ID | This plan | -|---|---| -| OV-08 | OV-01 | -| OV-09 | OV-02 | -| OV-10 | OV-03 | -| OV-11 | OV-04 | -| OV-30 | OV-05 | -| OV-31 | OV-06 | -| OV-32 | OV-07 | -| OV-34 | OV-08 | -| All other IDs | Moved to the UI effort (see above) | - -## Open questions for the owner - -1. OV-08: is an approximate (`20000+`) commit total acceptable for the badge contract? - -OV-03's questions were settled during implementation: the gate keeps two flags (`running`, -`dirty`), `CheckoutsStore` owns `debounce_pending`, and the mechanical `inbox.rs` edit landed -in this phase. -- 2.54.0 From 1e19fb6b1769726df984a10a3b987a74b7614700 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 13 Sep 2026 21:13:13 +0700 Subject: [PATCH 09/11] refactor git cache --- crates/paths/src/lib.rs | 6 +++- crates/signed_state/src/backend.rs | 4 +-- crates/signed_state/src/checkouts.rs | 4 +-- crates/signed_state/src/git_store.rs | 33 ++++++------------- crates/signed_state/src/lib.rs | 19 +++++++++-- crates/signed_state/src/repo.rs | 4 +-- .../src/views/pull_requests/detail.rs | 4 +-- .../workspace/src/views/pull_requests/new.rs | 4 +-- crates/workspace/src/views/repo/mod.rs | 6 ++-- 9 files changed, 44 insertions(+), 40 deletions(-) diff --git a/crates/paths/src/lib.rs b/crates/paths/src/lib.rs index f370bad..c5b3da6 100644 --- a/crates/paths/src/lib.rs +++ b/crates/paths/src/lib.rs @@ -90,9 +90,13 @@ pub fn nostr_dir() -> &'static PathBuf { } /// Returns the path to the local git clone cache, the grasp mirrors. +/// +/// The mirrors are disposable and re-cloned from their grasp server on +/// demand, so the cache lives in the OS temp directory for the system to +/// reclaim. pub fn repos_dir() -> &'static PathBuf { static REPOS_DIR: OnceLock = OnceLock::new(); - REPOS_DIR.get_or_init(|| data_dir().join("repos")) + REPOS_DIR.get_or_init(|| std::env::temp_dir().join(APP_NAME_LOWERCASE).join("repos")) } pub fn settings_file() -> &'static PathBuf { diff --git a/crates/signed_state/src/backend.rs b/crates/signed_state/src/backend.rs index 95d30e5..9c44ad5 100644 --- a/crates/signed_state/src/backend.rs +++ b/crates/signed_state/src/backend.rs @@ -13,7 +13,7 @@ use nostr_sdk::prelude::*; use signed_core::{Announcement, RepoAddr, build_state, filters, identifier_from_name}; use signed_nostr::{SignedAuthUrlHandler, UniversalSigner, Update}; -use crate::git_store::GitStore; +use crate::git_store::git_cache; use crate::inbox::Inbox; use crate::repos::RepoListStore; @@ -756,7 +756,7 @@ impl Backend { announcement: Announcement, cx: &mut Context, ) -> Task> { - let cache = GitStore::global(cx).cache().clone(); + let cache = git_cache(); let path = cache.repo_path(&announcement.addr()); self.push_repo_from(announcement, path, None, cx) } diff --git a/crates/signed_state/src/checkouts.rs b/crates/signed_state/src/checkouts.rs index 3f6dd50..d3fe96c 100644 --- a/crates/signed_state/src/checkouts.rs +++ b/crates/signed_state/src/checkouts.rs @@ -9,7 +9,7 @@ use settings::{CheckoutRecord, SettingsStore}; use signed_core::{Announcement, RepoAddr}; use crate::backend::{Backend, BackendEvent}; -use crate::git_store::GitStore; +use crate::git_store::git_cache; use crate::refresh::{RefreshGate, RefreshRequest}; use crate::repos::{LocalReposStore, RepoListStore}; @@ -328,7 +328,7 @@ impl CheckoutsStore { let announcements = RepoListStore::global(cx).read(cx).announcements.clone(); let scanned = LocalReposStore::global(cx).read(cx).repos.clone(); - let cache_root = GitStore::global(cx).cache().root().canonicalize().ok(); + let cache_root = git_cache().root().canonicalize().ok(); let requested: Vec<(RepoAddr, Option)> = self .status_requested diff --git a/crates/signed_state/src/git_store.rs b/crates/signed_state/src/git_store.rs index f284482..1193954 100644 --- a/crates/signed_state/src/git_store.rs +++ b/crates/signed_state/src/git_store.rs @@ -1,32 +1,19 @@ use std::path::PathBuf; +use std::sync::OnceLock; -use gpui::{App, Global}; use signed_git::GitCache; -struct GlobalGitStore(GitCache); - -impl Global for GlobalGitStore {} +static GIT_CACHE: OnceLock = OnceLock::new(); /// Global access to the on-disk git clone cache, the grasp mirrors. -#[derive(Debug, Clone)] -pub struct GitStore(GitCache); +pub fn git_cache() -> &'static GitCache { + GIT_CACHE + .get() + .expect("git cache is initialized by signed_state::init") +} -impl GitStore { - pub fn set_global(root: impl Into, cx: &mut App) -> Self { - let store = Self::new(root); - cx.set_global(GlobalGitStore(store.0.clone())); - store - } - - pub fn global(cx: &App) -> Self { - Self(cx.global::().0.clone()) - } - - fn new(root: impl Into) -> Self { - Self(GitCache::new(root.into())) - } - - pub fn cache(&self) -> &GitCache { - &self.0 +pub(crate) fn set_git_cache(root: impl Into) { + if GIT_CACHE.set(GitCache::new(root.into())).is_err() { + log::warn!("git cache root is already set, keeping the first one"); } } diff --git a/crates/signed_state/src/lib.rs b/crates/signed_state/src/lib.rs index d3dc6c4..d80a3d9 100644 --- a/crates/signed_state/src/lib.rs +++ b/crates/signed_state/src/lib.rs @@ -11,7 +11,8 @@ use std::path::{Path, PathBuf}; pub use backend::{Backend, BackendEvent, user_grasp_list_servers}; pub use checkouts::{CheckoutStatus, CheckoutsStore, pr_proposes_checkout}; -pub use git_store::GitStore; +pub use git_store::git_cache; +use git_store::set_git_cache; use gpui::{App, AppContext}; pub use inbox::{Inbox, query_inbox}; pub use nostr_sdk::prelude::Timestamp; @@ -31,6 +32,7 @@ pub fn init( // rustls uses the `aws_lc_rs` provider by default. let _ = rustls::crypto::aws_lc_rs::default_provider().install_default(); + // Initialize the nostr client and signer let (client, signer) = cx.foreground_executor().block_on(async move { let path = db_path.as_ref().to_path_buf(); new_backend(path) @@ -38,11 +40,22 @@ pub fn init( .expect("failed to initialize nostr backend") }); + // Set Git cache for the repos root + set_git_cache(repos_root); + + // Set global stores for the backend Backend::set_global(cx.new(|cx| Backend::new(client, signer, cx)), cx); + + // Set global stores for the profile ProfileStore::set_global(cx.new(ProfileStore::new), cx); + + // Set global stores for the repo list and local repos RepoListStore::set_global(cx.new(RepoListStore::new), cx); - GitStore::set_global(repos_root, cx); + + // Set global stores for the local repos LocalReposStore::set_global(cx.new(|cx| LocalReposStore::new(scan_paths, cx)), cx); + + // Set global stores for the checkouts CheckoutsStore::set_global(cx.new(CheckoutsStore::new), cx); } @@ -50,10 +63,10 @@ pub fn init( #[cfg(target_arch = "wasm32")] pub fn init(cx: &mut App) { let (client, signer) = new_backend().expect("failed to initialize nostr backend"); + set_git_cache(PathBuf::new()); Backend::set_global(cx.new(|cx| Backend::new(client, signer, cx)), cx); ProfileStore::set_global(cx.new(ProfileStore::new), cx); RepoListStore::set_global(cx.new(RepoListStore::new), cx); - GitStore::set_global(PathBuf::new(), cx); LocalReposStore::set_global(cx.new(|cx| LocalReposStore::new(Vec::new(), cx)), cx); CheckoutsStore::set_global(cx.new(|cx| CheckoutsStore::new(cx)), cx); } diff --git a/crates/signed_state/src/repo.rs b/crates/signed_state/src/repo.rs index 395f1e2..f7cfcc7 100644 --- a/crates/signed_state/src/repo.rs +++ b/crates/signed_state/src/repo.rs @@ -17,7 +17,7 @@ use crate::backend::{ user_grasp_list_servers, }; use crate::checkouts::CheckoutsStore; -use crate::git_store::GitStore; +use crate::git_store::git_cache; use crate::refresh::{RefreshGate, RefreshRequest}; use crate::repos::RepoListStore; @@ -1196,7 +1196,7 @@ impl RepoStore { return; } - let cache = GitStore::global(cx).cache().clone(); + let cache = git_cache(); let clone_urls: Vec = self .announcement diff --git a/crates/workspace/src/views/pull_requests/detail.rs b/crates/workspace/src/views/pull_requests/detail.rs index 392ad41..3ddeac1 100644 --- a/crates/workspace/src/views/pull_requests/detail.rs +++ b/crates/workspace/src/views/pull_requests/detail.rs @@ -26,7 +26,7 @@ use signed_core::{ merge_base_of, pull_request_patch, }; use signed_git::{FileCommit, patch_commits, patch_diffs}; -use signed_state::{Backend, GitStore, ProfileStore, RepoStore}; +use signed_state::{Backend, ProfileStore, RepoStore, git_cache}; use signed_ui::{CountBadge, UserAvatar, placeholder, status_badge}; use utils::{relative_time, relative_time_secs}; @@ -217,7 +217,7 @@ impl PullRequestDetailView { self.current_commit = binding.tip.clone().map(SharedString::from); cx.notify(); - let cache = GitStore::global(cx).cache().clone(); + let cache = git_cache(); self.load_generation = self.load_generation.wrapping_add(1); let generation = self.load_generation; diff --git a/crates/workspace/src/views/pull_requests/new.rs b/crates/workspace/src/views/pull_requests/new.rs index 2241dee..fd57427 100644 --- a/crates/workspace/src/views/pull_requests/new.rs +++ b/crates/workspace/src/views/pull_requests/new.rs @@ -26,7 +26,7 @@ use signed_git::{ delete_refs_with_prefix, fetch_repo_refs, fork_namespace, merge_base, refs_with_prefix, worktree_commit_range_commits, worktree_commit_range_diff, }; -use signed_state::{Backend, CheckoutsStore, GitStore, RepoListStore, RepoStore}; +use signed_state::{Backend, CheckoutsStore, RepoListStore, RepoStore, git_cache}; use signed_ui::{CountBadge, placeholder, ref_selector_trigger}; use crate::views::commit_diff::{COMMIT_ROW_HEIGHT, CommitDiffView, DiffPane, commit_row}; @@ -533,7 +533,7 @@ impl NewPullRequestView { let Some((base, _euc)) = self.base_repo(cx) else { return; }; - let cache = GitStore::global(cx).cache().clone(); + let cache = git_cache(); let mirror_path = cache.repo_path(&base); let namespace = fork_namespace(&announcement); let clone_urls = announcement.clone.clone(); diff --git a/crates/workspace/src/views/repo/mod.rs b/crates/workspace/src/views/repo/mod.rs index eacdae1..8b94914 100644 --- a/crates/workspace/src/views/repo/mod.rs +++ b/crates/workspace/src/views/repo/mod.rs @@ -26,8 +26,8 @@ use nostr::prelude::{RelayUrl, ToBech32, Url}; use signed_core::{Announcement, RepoAddr, RepoStatus}; use signed_git::FileCommit; use signed_state::{ - Backend, CheckoutStatus, CheckoutsStore, GitStore, LocalReposStore, ProfileStore, - RepoListStore, RepoStore, pr_proposes_checkout, + Backend, CheckoutStatus, CheckoutsStore, LocalReposStore, ProfileStore, RepoListStore, + RepoStore, git_cache, pr_proposes_checkout, }; use signed_ui::{ CountBadge, DropdownButton, PixelAvatar, UserAvatar, copy_row, menu_copy_row, middle_truncate, @@ -343,7 +343,7 @@ impl RepoDetailView { self.repo_started = true; - let cache = GitStore::global(cx).cache().clone(); + let cache = git_cache(); let addr = announcement.addr(); let clone_urls: Vec = announcement.clone.clone(); -- 2.54.0 From db010da4b9679de795466a11624b7aa88d5a2e2e Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 13 Sep 2026 21:22:13 +0700 Subject: [PATCH 10/11] refactor git cache --- Cargo.lock | 1 + crates/signed_state/Cargo.toml | 1 + crates/signed_state/src/backend.rs | 5 ++-- crates/signed_state/src/checkouts.rs | 4 +-- crates/signed_state/src/git_store.rs | 26 +++++++++++++++++-- crates/signed_state/src/lib.rs | 2 +- crates/signed_state/src/repo.rs | 6 ++--- .../src/views/pull_requests/detail.rs | 7 ++--- .../workspace/src/views/pull_requests/new.rs | 14 +++++----- crates/workspace/src/views/repo/mod.rs | 12 +++------ 10 files changed, 46 insertions(+), 32 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a67be4f..7a27540 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8006,6 +8006,7 @@ dependencies = [ "bitcoin_hashes 1.2.0", "flume 0.11.1", "futures", + "gix", "gpui", "log", "nostr", diff --git a/crates/signed_state/Cargo.toml b/crates/signed_state/Cargo.toml index 554c38b..7e23203 100644 --- a/crates/signed_state/Cargo.toml +++ b/crates/signed_state/Cargo.toml @@ -17,6 +17,7 @@ nostr-connect.workspace = true bitcoin_hashes = "1" +gix.workspace = true gpui.workspace = true flume.workspace = true futures.workspace = true diff --git a/crates/signed_state/src/backend.rs b/crates/signed_state/src/backend.rs index 9c44ad5..fa9fccf 100644 --- a/crates/signed_state/src/backend.rs +++ b/crates/signed_state/src/backend.rs @@ -13,7 +13,7 @@ use nostr_sdk::prelude::*; use signed_core::{Announcement, RepoAddr, build_state, filters, identifier_from_name}; use signed_nostr::{SignedAuthUrlHandler, UniversalSigner, Update}; -use crate::git_store::git_cache; +use crate::git_store::repo_mirror_path; use crate::inbox::Inbox; use crate::repos::RepoListStore; @@ -756,8 +756,7 @@ impl Backend { announcement: Announcement, cx: &mut Context, ) -> Task> { - let cache = git_cache(); - let path = cache.repo_path(&announcement.addr()); + let path = repo_mirror_path(&announcement.addr()); self.push_repo_from(announcement, path, None, cx) } diff --git a/crates/signed_state/src/checkouts.rs b/crates/signed_state/src/checkouts.rs index d3fe96c..52d0214 100644 --- a/crates/signed_state/src/checkouts.rs +++ b/crates/signed_state/src/checkouts.rs @@ -9,7 +9,7 @@ use settings::{CheckoutRecord, SettingsStore}; use signed_core::{Announcement, RepoAddr}; use crate::backend::{Backend, BackendEvent}; -use crate::git_store::git_cache; +use crate::git_store::repo_mirror_root; use crate::refresh::{RefreshGate, RefreshRequest}; use crate::repos::{LocalReposStore, RepoListStore}; @@ -328,7 +328,7 @@ impl CheckoutsStore { let announcements = RepoListStore::global(cx).read(cx).announcements.clone(); let scanned = LocalReposStore::global(cx).read(cx).repos.clone(); - let cache_root = git_cache().root().canonicalize().ok(); + let cache_root = repo_mirror_root().canonicalize().ok(); let requested: Vec<(RepoAddr, Option)> = self .status_requested diff --git a/crates/signed_state/src/git_store.rs b/crates/signed_state/src/git_store.rs index 1193954..00cc6da 100644 --- a/crates/signed_state/src/git_store.rs +++ b/crates/signed_state/src/git_store.rs @@ -1,17 +1,39 @@ use std::path::PathBuf; use std::sync::OnceLock; +use anyhow::Result; +use gix::Repository; +use signed_core::RepoAddr; use signed_git::GitCache; static GIT_CACHE: OnceLock = OnceLock::new(); -/// Global access to the on-disk git clone cache, the grasp mirrors. -pub fn git_cache() -> &'static GitCache { +fn git_cache() -> &'static GitCache { GIT_CACHE .get() .expect("git cache is initialized by signed_state::init") } +/// The root directory of the repository mirrors. +pub(crate) fn repo_mirror_root() -> PathBuf { + git_cache().root().to_path_buf() +} + +/// The on-disk path of the mirror of `addr`. +pub fn repo_mirror_path(addr: &RepoAddr) -> PathBuf { + git_cache().repo_path(addr) +} + +/// Open the mirror of `addr`, if it has been cloned. +pub fn open_repo_mirror(addr: &RepoAddr) -> Result> { + git_cache().open(addr) +} + +/// Open the mirror of `addr`, cloning it first when it does not exist yet. +pub fn ensure_repo_mirror>(addr: &RepoAddr, clone_urls: &[U]) -> Result { + git_cache().ensure_clone(addr, clone_urls) +} + pub(crate) fn set_git_cache(root: impl Into) { if GIT_CACHE.set(GitCache::new(root.into())).is_err() { log::warn!("git cache root is already set, keeping the first one"); diff --git a/crates/signed_state/src/lib.rs b/crates/signed_state/src/lib.rs index d80a3d9..97afe9f 100644 --- a/crates/signed_state/src/lib.rs +++ b/crates/signed_state/src/lib.rs @@ -11,8 +11,8 @@ use std::path::{Path, PathBuf}; pub use backend::{Backend, BackendEvent, user_grasp_list_servers}; pub use checkouts::{CheckoutStatus, CheckoutsStore, pr_proposes_checkout}; -pub use git_store::git_cache; use git_store::set_git_cache; +pub use git_store::{ensure_repo_mirror, open_repo_mirror, repo_mirror_path}; use gpui::{App, AppContext}; pub use inbox::{Inbox, query_inbox}; pub use nostr_sdk::prelude::Timestamp; diff --git a/crates/signed_state/src/repo.rs b/crates/signed_state/src/repo.rs index f7cfcc7..8ad9539 100644 --- a/crates/signed_state/src/repo.rs +++ b/crates/signed_state/src/repo.rs @@ -17,7 +17,7 @@ use crate::backend::{ user_grasp_list_servers, }; use crate::checkouts::CheckoutsStore; -use crate::git_store::git_cache; +use crate::git_store::ensure_repo_mirror; use crate::refresh::{RefreshGate, RefreshRequest}; use crate::repos::RepoListStore; @@ -1196,8 +1196,6 @@ impl RepoStore { return; } - let cache = git_cache(); - let clone_urls: Vec = self .announcement .as_ref() @@ -1223,7 +1221,7 @@ impl RepoStore { let root = root.clone(); let apply = cx.background_spawn(async move { - let repo = cache.ensure_clone(&addr, &clone_urls)?; + let repo = ensure_repo_mirror(&addr, &clone_urls)?; let workdir = repo .workdir() .ok_or_else(|| anyhow::anyhow!("repository has no worktree"))? diff --git a/crates/workspace/src/views/pull_requests/detail.rs b/crates/workspace/src/views/pull_requests/detail.rs index 3ddeac1..0a45185 100644 --- a/crates/workspace/src/views/pull_requests/detail.rs +++ b/crates/workspace/src/views/pull_requests/detail.rs @@ -26,7 +26,7 @@ use signed_core::{ merge_base_of, pull_request_patch, }; use signed_git::{FileCommit, patch_commits, patch_diffs}; -use signed_state::{Backend, ProfileStore, RepoStore, git_cache}; +use signed_state::{Backend, ProfileStore, RepoStore, ensure_repo_mirror}; use signed_ui::{CountBadge, UserAvatar, placeholder, status_badge}; use utils::{relative_time, relative_time_secs}; @@ -217,8 +217,6 @@ impl PullRequestDetailView { self.current_commit = binding.tip.clone().map(SharedString::from); cx.notify(); - let cache = git_cache(); - self.load_generation = self.load_generation.wrapping_add(1); let generation = self.load_generation; @@ -257,7 +255,6 @@ impl PullRequestDetailView { let git = if use_nostr { None } else { - let cache = cache.clone(); let addr = addr.clone(); let clone_urls = clone_urls.clone(); let base = base.clone(); @@ -265,7 +262,7 @@ impl PullRequestDetailView { Some( cx.background_spawn(async move { - let repo = cache.ensure_clone(&addr, &clone_urls)?; + let repo = ensure_repo_mirror(&addr, &clone_urls)?; let workdir = repo .workdir() diff --git a/crates/workspace/src/views/pull_requests/new.rs b/crates/workspace/src/views/pull_requests/new.rs index fd57427..4eac45a 100644 --- a/crates/workspace/src/views/pull_requests/new.rs +++ b/crates/workspace/src/views/pull_requests/new.rs @@ -26,7 +26,9 @@ use signed_git::{ delete_refs_with_prefix, fetch_repo_refs, fork_namespace, merge_base, refs_with_prefix, worktree_commit_range_commits, worktree_commit_range_diff, }; -use signed_state::{Backend, CheckoutsStore, RepoListStore, RepoStore, git_cache}; +use signed_state::{ + Backend, CheckoutsStore, RepoListStore, RepoStore, ensure_repo_mirror, repo_mirror_path, +}; use signed_ui::{CountBadge, placeholder, ref_selector_trigger}; use crate::views::commit_diff::{COMMIT_ROW_HEIGHT, CommitDiffView, DiffPane, commit_row}; @@ -73,7 +75,7 @@ struct ForkCompare { announcement: Announcement, /// Import namespace of the form `/`. namespace: String, - /// Path of the target repository's GitCache mirror. + /// Path of the target repository's mirror. mirror_path: PathBuf, } @@ -533,8 +535,7 @@ impl NewPullRequestView { let Some((base, _euc)) = self.base_repo(cx) else { return; }; - let cache = git_cache(); - let mirror_path = cache.repo_path(&base); + let mirror_path = repo_mirror_path(&base); let namespace = fork_namespace(&announcement); let clone_urls = announcement.clone.clone(); @@ -563,17 +564,16 @@ impl NewPullRequestView { cx.spawn_in(window, async move |this, cx| { // The fork and base must share history for a merge-base to exist. // The target's mirror is the object store both sides land in. - // `ensure_clone` fetches `origin` when the mirror already exists. + // `ensure_repo_mirror` fetches `origin` when the mirror already exists. let result = cx .background_spawn({ - let cache = cache.clone(); let base = base.clone(); let base_clone_urls = base_clone_urls.clone(); let namespace = namespace.clone(); let clone_urls = clone_urls.clone(); let mirror_path = mirror_path.clone(); async move { - cache.ensure_clone(&base, &base_clone_urls)?; + ensure_repo_mirror(&base, &base_clone_urls)?; // Prune stale imports of any fork. // Then import this fork's heads under its namespace. diff --git a/crates/workspace/src/views/repo/mod.rs b/crates/workspace/src/views/repo/mod.rs index 8b94914..747ca4d 100644 --- a/crates/workspace/src/views/repo/mod.rs +++ b/crates/workspace/src/views/repo/mod.rs @@ -27,7 +27,7 @@ use signed_core::{Announcement, RepoAddr, RepoStatus}; use signed_git::FileCommit; use signed_state::{ Backend, CheckoutStatus, CheckoutsStore, LocalReposStore, ProfileStore, RepoListStore, - RepoStore, git_cache, pr_proposes_checkout, + RepoStore, ensure_repo_mirror, open_repo_mirror, pr_proposes_checkout, }; use signed_ui::{ CountBadge, DropdownButton, PixelAvatar, UserAvatar, copy_row, menu_copy_row, middle_truncate, @@ -343,15 +343,13 @@ impl RepoDetailView { self.repo_started = true; - let cache = git_cache(); let addr = announcement.addr(); let clone_urls: Vec = announcement.clone.clone(); let disk = { - let cache = cache.clone(); let addr = addr.clone(); cx.background_spawn(async move { - match cache.open(&addr)? { + match open_repo_mirror(&addr)? { Some(repo) => Ok(Some(load_repo_data(&repo)?)), None => Ok(None), } @@ -365,11 +363,10 @@ impl RepoDetailView { let data = match disk { Ok(Some(data)) => Ok(data), Ok(None) => { - let cache = cache.clone(); let addr = addr.clone(); let clone_urls = clone_urls.clone(); cx.background_spawn(async move { - let repo = cache.ensure_clone(&addr, &clone_urls)?; + let repo = ensure_repo_mirror(&addr, &clone_urls)?; load_repo_data(&repo) }) .await @@ -393,11 +390,10 @@ impl RepoDetailView { } let refresh = { - let cache = cache.clone(); let addr = addr.clone(); cx.background_spawn(async move { - let Some(repo) = cache.open(&addr)? else { + let Some(repo) = open_repo_mirror(&addr)? else { return Ok::<_, Error>(None); }; -- 2.54.0 From e08a96a106eb717e17b71bdb99d104a0ec103983 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 13 Sep 2026 21:41:51 +0700 Subject: [PATCH 11/11] clean up --- .../workspace/src/views/pull_requests/new.rs | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/crates/workspace/src/views/pull_requests/new.rs b/crates/workspace/src/views/pull_requests/new.rs index 4eac45a..5a5f238 100644 --- a/crates/workspace/src/views/pull_requests/new.rs +++ b/crates/workspace/src/views/pull_requests/new.rs @@ -562,9 +562,6 @@ impl NewPullRequestView { let task: gpui::Task> = cx.spawn_in(window, async move |this, cx| { - // The fork and base must share history for a merge-base to exist. - // The target's mirror is the object store both sides land in. - // `ensure_repo_mirror` fetches `origin` when the mirror already exists. let result = cx .background_spawn({ let base = base.clone(); @@ -575,10 +572,10 @@ impl NewPullRequestView { async move { ensure_repo_mirror(&base, &base_clone_urls)?; - // Prune stale imports of any fork. - // Then import this fork's heads under its namespace. + // Prune stale imports of any fork. Then import this fork's heads under its namespace. delete_refs_with_prefix(&mirror_path, "refs/fork")?; + // Fetch the fork's refs and import them under the fork's namespace. fetch_repo_refs( &mirror_path, &clone_urls, @@ -658,8 +655,6 @@ impl NewPullRequestView { let (base_branches, compare_branches) = match result { Ok(branches) => branches, Err(error) => { - // Keep the previous source, if any. - // The error shows inline next to the compare bar. self.error = Some(format!("Could not compare against the fork: {error}").into()); cx.notify(); return; @@ -673,8 +668,7 @@ impl NewPullRequestView { } if base_branches.is_empty() { - self.error = - Some("Could not list the target repository's branches; try again later".into()); + self.error = Some("Could not list the target repository's branches.".into()); cx.notify(); return; } @@ -687,11 +681,8 @@ impl NewPullRequestView { .map(SharedString::from) .collect(); - // Base defaults to the announced HEAD branch when the mirror has it. - // Otherwise `main`, then the first branch. - // The fork's `main` is the compare default, else the first branch. - // A refresh keeps the previous selection when the branch still exists. let announced = self.store.read(cx).head.clone(); + let contains = |name: &str, list: &[SharedString]| list.iter().any(|branch| branch.as_ref() == name); @@ -790,16 +781,19 @@ impl NewPullRequestView { "{base_name} and {compare_name} share no common ancestor" ) })?; + let commits = worktree_commit_range_commits( Path::new(&repo_path), &merge_base, &compare, )?; + let diff = worktree_commit_range_diff( Path::new(&repo_path), &merge_base, &compare, )?; + Ok::<_, anyhow::Error>((merge_base, commits, diff)) } }) @@ -923,6 +917,7 @@ impl NewPullRequestView { let Some(repo_path) = self.work_path() else { return; }; + let Some(dock_area) = self.dock_area.upgrade() else { return; }; -- 2.54.0