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?