2026-09-13 14:48:51 +00:00
1 changed files with 0 additions and 299 deletions
@@ -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<char>`, 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<RwLock<Arc<dyn InnerSigner>>>`) — 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.