signed/docs/over-optimization-action-plan.md

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:

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.

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<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:

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?