refactor

2026-09-13 11:54:37 +07:00
parent d68c098818
commit a4dcbbc560
7 changed files with 719 additions and 267 deletions
@@ -0,0 +1,330 @@
+# Repository state and panel flow plan
+
+Status: proposed (2026-09-13)
+
+Builds on `docs/backend-rearchitecture.md`, especially §7 (notify audit),
+§11 (split independently-observed state), §12 (one debounce at the source)
+and §13 (merge the repo listing files, not the entities).
+
+Scope: `signed_state::{repo, repos}`, `workspace::views::{repo, issues,
+pull_requests, inbox}`.
+
+## Goal
+
+1. Open an issue or PR directly, from any surface (inbox entry, notification,
+   future deep link), without walking Explore -> repository panel first.
+2. One per-repository entity that spans both identities: the local git
+   repository and the NIP-34 announcement, instead of today's separate
+   `LocalReposStore` entry / `Option<Announcement>` + `Option<Entity<RepoStore>>`
+   view state.
+
+## What the code does today (verified against the current tree)
+
+### 1. Item panels are cached and never observe their store
+
+The dock renders the active panel through `panel.cached(...)`
+(`crates/dock/src/tab_panel.rs:777-795`), so a panel re-renders only when its
+own entity notifies. Cross-entity reads are not reactive.
+
+`RepoDetailView` observes its store (`views/repo/store.rs:35-43`), but the
+panels that read the same store do not:
+
+- `IssueDetailView` (`views/issues/detail.rs:79-85`) renders
+  `placeholder("Issue not found", cx)` when the issue is absent, with no
+  observer. When a directly opened store's first pass lands later and notifies,
+  nothing re-renders the panel: it stays on "Issue not found".
+- `PullRequestDetailView::load` runs once from `cx.defer_in`
+  (`views/pull_requests/detail.rs:88-91`) and caches
+  `error = "Pull request not found"` when the store is empty
+  (`:124-133`). It can never recover.
+- `IssuesView` / `PullRequestsView` memoize rows behind
+  `(store.version(), filter)` (`views/issues/mod.rs:324-351`,
+  `views/pull_requests/mod.rs:334-369`) but nothing re-renders them when the
+  version changes, so an open list does not see new events either.
+- `NewPullRequestView` reads `store.head` / the announcement in `new`, so a
+  late first pass does not reach its defaults.
+
+The normal flow hides this because the repository panel is opened first: by
+the time the issues list or an item panel is created, the store has already
+applied its first pass. Opening an item directly is the case where the store
+is still empty at construction.
+
+### 2. The initial pass waits on a timer the backend already provides
+
+`RepoStore::new` defers `refresh` (`signed_state/src/repo.rs:130-141`), and
+`refresh` always waits `REFRESH_DEBOUNCE` = 300 ms (`repo.rs:25-26`,
+`:230-242`) before the local database query. `RepoListStore` has the same
+timer (`repos.rs:120-123`, `:270-281`) plus a separate `refresh_initial`
+that skips it (`:257-268`).
+
+Both stores only refresh on `Backend` events:
+
+- `NostrUpdate` is already batched by the notification pump with its own
+  `PUMP_DEBOUNCE` = 200 ms (`backend.rs:37`, `:112-166`). Per
+  `backend-rearchitecture.md` §12, the per-store timers were to be dropped
+  once the pump absorbed the bursts.
+- `Published` and `Synced` are one-off events; `RefreshGate` already folds
+  them into an in-flight run.
+
+The timers are therefore pure added latency for these two stores: ~300 ms
+(batched updates) to ~500 ms (pump window + store window) before local data
+appears.
+
+### 3. Opening an item requires a hydrated `Announcement`
+
+`repo_store(announcement)` (`views/repo/actions.rs:216-219`) and the inbox's
+lookup in `RepoListStore` (`views/inbox.rs:384-404`) need the announcement in
+hand and silently no-op when it is missing (`RepoListStore` not synced yet,
+deleted repo, inbox section without a matching list entry). The store itself
+can load the announcement from the local database; `run_refresh` already
+queries `filters::announcement(&addr)` (`repo.rs:251-261`).
+
+### 4. Local and announced repositories have no shared identity
+
+- `LocalReposStore` holds scan paths; `RepoListStore` holds announcements;
+  `CheckoutsStore` joins them (`checkouts.rs:211-213`, `:569-609`).
+- `RepoDetailView` encodes both modes in three option fields:
+  `initial: Option<Announcement>`, `store: Option<Entity<RepoStore>>`,
+  `local_path: Option<PathBuf>` (`views/repo/mod.rs:74-85`), with
+  `apply_announcement` moving between them (`views/repo/store.rs:13-31`).
+  The invalid combinations and the `initial` fallback in `announcement()`
+  (`views/repo/mod.rs:322-331`) are the cost of the missing per-repo entity.
+
+## Design
+
+### 1. Panels observe, derive into local state, notify on change
+
+Every panel that reads a store keeps a local snapshot of exactly the slice it
+renders, updates it in an observer, and calls `cx.notify()` only when the
+slice changed. This is the pattern `RepoDetailView::refresh_statuses`
+(`views/repo/store.rs:88-101`) and `SidebarPanel::refresh` already use; the
+item panels are missing the observer half.
+
+| View | Observed entity | Local snapshot |
+|---|---|---|
+| `IssueDetailView` | `Entity<RepoStore>` | root issue, status, comments |
+| `PullRequestDetailView` | `Entity<RepoStore>` | root PR, description, tip, base, clone urls |
+| `IssuesView` | `Entity<RepoStore>` | `visible_issues`, `counts`, `item_sizes`, `cache_key` |
+| `PullRequestsView` | `Entity<RepoStore>` | `visible_prs`, `counts`, `item_sizes`, `cache_key` |
+| `NewPullRequestView` | `Entity<RepoStore>` | announced head, default base |
+
+Sketch, matching the existing idiom:
+
+```rust
+// new()
+let subscription = cx.observe(&store, |this, store, cx| this.sync(store, cx));
+
+// Copy the slice; notify only when it changed.
+fn sync(&mut self, store: &Entity<RepoStore>, cx: &mut Context<Self>) {
+    let store = store.read(cx);
+    let issue = store.issues.iter().find(|issue| issue.id == self.issue_id).cloned();
+    let comments: Vec<Event> = store.comments_of(&self.issue_id).cloned().collect();
+    let status = issue.as_ref().map(|issue| store.status_of(issue));
+
+    if self.issue != issue || self.comments != comments || self.status != status {
+        self.issue = issue;
+        self.comments = comments;
+        self.status = status;
+        cx.notify();
+    }
+}
+```
+
+`PullRequestDetailView` needs its one-shot `load` split in two:
+
+- `bind` finds the root PR in the store and snapshots description, tip, base
+  and clone urls. Re-runnable on every store version change.
+- `load_diff` does the async patch/git work. Runs once bound, and again when
+  the bound tip changes (a PR update arriving late).
+
+### 2. One debounce, at the backend pump
+
+Delete the `REFRESH_DEBOUNCE` timers from `RepoStore` and `RepoListStore`.
+Their triggers all come from `Backend`; the pump batches relay traffic and
+`RefreshGate` folds one-off events into an in-flight run. Keep `RefreshGate`
+exactly as is, used without a timer:
+
+```rust
+pub fn refresh(&mut self, cx: &mut Context<Self>) {
+    if self.refresh.request() != RefreshRequest::Schedule {
+        return;
+    }
+    self.run_refresh(cx);
+}
+```
+
+`RepoListStore::refresh_initial` collapses into `refresh`; the
+`new`-time `cx.defer` call becomes the initial load, with no timer.
+
+Add `pub loaded: bool` to `RepoStore`, set when the first pass applies. It
+separates "no data yet" from "genuinely empty": panels render a loading state
+while `!loaded`, and "not found" only after `loaded`.
+
+`CheckoutsStore` and `InboxView` keep their timers in this plan. Their
+refresh request sources are not only the backend pump (checkout requests,
+settings and scan observations), so the same argument does not hold
+unchanged; revisit separately if measurements show the timers redundant.
+
+### 3. `RepoStore` is the one per-repository entity
+
+Keep the name `RepoStore` (`Repository` collides with `gix::Repository`,
+already imported in `views/repo/loading.rs`). Shape:
+
+```rust
+pub struct RepoStore {
+    /// NIP-34 address. `None` while the repository is local-only.
+    addr: Option<RepoAddr>,
+    /// Latest announcement. Seeded from the open-time hint, replaced by the
+    /// database's latest on the first pass. `None` while local-only.
+    pub announcement: Option<Announcement>,
+    /// Local working copy: the scan path for a local repo, an associated
+    /// checkout for an announced one. A snapshot; `CheckoutsStore` stays the
+    /// authority for the full list of checkouts.
+    pub path: Option<PathBuf>,
+    /// The first local pass has been applied.
+    pub loaded: bool,
+    // issues, patches, pull_requests, comments, status_by_root, head, flags...
+    _subscription: Option<Subscription>,
+}
+```
+
+`addr` is required in addition to `announcement`: `(announcement: None,
+path: Some(_))` is otherwise ambiguous between "local-only" and "announced,
+first pass pending", and the store needs the address to run its query.
+
+Constructors and the state transition:
+
+```rust
+impl RepoStore {
+    /// Announced repository. Resolves `path` from `CheckoutsStore` if the
+    /// user already has a checkout.
+    pub fn new(addr: RepoAddr, hint: Option<Announcement>, cx: &mut Context<Self>) -> Self;
+
+    /// Local repository discovered by the scan, not announced yet.
+    pub fn new_local(path: PathBuf, cx: &mut Context<Self>) -> Self;
+
+    /// Local -> NIP-34 in place. Keeps `path`, so the panel keeps its worktree.
+    pub fn announce(&mut self, announcement: Announcement, cx: &mut Context<Self>);
+
+    pub fn addr(&self) -> Option<&RepoAddr>;
+}
+```
+
+- `new`: subscribes to `Backend`, seeds `announcement` from the hint, defers
+  remote subscribe/connect, runs the local pass immediately.
+- `new_local`: no `Backend` subscription, `loaded = true`, path set.
+- `announce`: sets `addr`, `announcement`, keeps `path`; installs the
+  `Backend` subscription, connects the announced relays and refreshes. Called
+  from `apply_announcement`, which loses its field surgery.
+- Nostr-side actions (`push_repository`, `clone_to_folder`,
+  `open_issue`/`open_pull_request`, status changes) already have
+  `action_error("Repository announcement is not loaded yet")`
+  (`repo.rs:1368-1378`); they now also handle `addr == None` the same way.
+- `announce` should also `CheckoutsStore::record(path, addr)` for the scan
+  path, so the association exists immediately instead of waiting for the
+  origin/EUC match in `resolve_associations`. Optional, verify behavior.
+
+`RepoDetailView` then holds `store: Entity<RepoStore>` plus explorer state
+only. `initial` and `local_path` are deleted; `announcement()` reads the
+store; local-mode checks become `store.read(cx).addr().is_none()`;
+`load_repo` opens `path` when not announced, and keeps today's cache-mirror
+flow for announced repositories.
+
+### 4. Opening a repository or item needs only a `RepoAddr`
+
+- `repo_store(addr: &RepoAddr, hint: Option<&Announcement>, cx) ->
+  Entity<RepoStore>`.
+- `open_repo_item(dock_area, addr: &RepoAddr, item, window, cx)`.
+- Inbox passes its already-parsed `address` (`signed_core::InboxItem.address`,
+  the root event's `a` tag) and drops the `RepoListStore` lookup.
+- `open_upstream` drops the 60 x 250 ms wait loop: the store's
+  `repo_filters` already include the announcement filter and
+  `subscribe_remote` runs on creation, so the panel opens immediately and
+  fills in.
+- `RepoItem::Patch` behavior is unchanged.
+
+## Phases
+
+### Phase 1 - make item panels react to the store (fixes the reported flow)
+
+Status: implemented, except step 6.
+
+1. `signed_state/src/repo.rs`: `REFRESH_DEBOUNCE` and the timer spawn are
+   gone; `refresh` runs immediately. `loaded` was added and is set in the
+   apply closure. A first pass notifies even when it found nothing, so views
+   leave their loading state and show the empty result.
+2. `signed_state/src/repos.rs`: same timer removal; `refresh_initial` folded
+   into `refresh`.
+3. `views/issues/detail.rs`: observes the store and re-renders; loading
+   placeholder while `!store.loaded()`.
+4. `views/pull_requests/detail.rs`: observes the store; `sync`/`sync_missing`
+   bind the root and `load_diff` does the async work, keyed to a `PrBinding`
+   and guarded by a generation so late results are discarded. Loading vs not
+   found is decided by `store.loaded()`.
+5. `views/issues/mod.rs`, `views/pull_requests/mod.rs`: observe the store;
+   `rebuild` recomputes rows/counts/item sizes and `sync` notifies on change.
+   Filter buttons call `rebuild` before notifying.
+6. `views/pull_requests/new.rs`: not done. Its store-derived inputs are
+   defaults for the compare/base selectors; re-applying them on a late store
+   pass would clobber a selection the user already made. Left for a follow-up
+   once the defaults can be derived without resetting the selectors.
+
+### Phase 2 - entry points by identity
+
+1. `views/repo/actions.rs`: `repo_store(addr, hint, cx)`;
+   `open_repo_item(addr, ...)`; `open_upstream` opens directly.
+2. `views/inbox.rs`: pass `address`, delete the announcement lookup and its
+   silent early-return.
+
+### Phase 3 - one entity for local and NIP-34
+
+1. `signed_state/src/repo.rs`: `addr`/`path` options, `new_local`,
+   `announce`, `Option<Subscription>`, action guards.
+2. `views/repo/mod.rs`: single `store` field; `new`/`new_local`; header,
+   display name, `load_repo`, `open_init_dialog` derive from the store.
+3. `views/repo/store.rs`: always observe; `refresh_statuses` returns false
+   when not announced.
+4. `views/repo/actions.rs`, `header.rs`, `banners.rs`: drop
+   `Option<Entity<RepoStore>>` guards, guard on `addr()` instead.
+5. `LocalReposStore` stays as the scan index; `CheckoutsStore` stays the
+   association authority.
+
+### Phase 4 - deferred, only if duplicate stores become a problem
+
+One store per address via `HashMap<RepoAddr, WeakEntity<RepoStore>>` inside
+`RepoListStore`, so an item panel opened while the repository panel is open
+shares the same store and its subscriptions. Not needed for correctness once
+Phase 1 lands; each open then loads from the local database immediately.
+
+## Non-goals
+
+- No per-store debounce; the pump is the one debounce (§12). `RepoStore` and
+  `RepoListStore` must not grow timers again.
+- No new global store, and no merging `LocalReposStore`, `RepoListStore`
+  and `CheckoutsStore` into one entity (§13).
+- Views never query the database directly; `RepoStore` stays the single
+  projection so status/comment derivation is not duplicated.
+- Explorer state (tree, refs, commits, scroll) stays in `RepoDetailView`.
+- No `Repository` rename.
+
+## Validation
+
+- `cargo check -p signed_state -p workspace`, then clippy.
+- Manual scenarios:
+  1. Fresh database, never open Explore: click an issue in the inbox. Panel
+     shows a loading state, then the issue with comments and status.
+  2. Same for a PR, including the patch diff loading once the root binds.
+  3. Open the issues list, then receive a new issue (second client or
+     publish); the row appears without reopening.
+  4. Local repo: open from the sidebar, Init, panel keeps the worktree and
+     gains the nostr header; reopening from Explore shows the same data.
+  5. Existing flows: Explore list, ready-to-contribute and ready-to-push
+     banners, new issue/PR dialogs.
+
+## Open questions
+
+1. Should an announced repository with an associated checkout browse that
+   checkout instead of the cache mirror? Today the panel always mirrors.
+   `RepoStore.path` makes this a one-line decision later.
+2. Store sharing (Phase 4): worth it only if duplicate subscriptions show up
+   in practice.