# Code Quality Review — WIP Guard Feature

**Date:** 2026-04-18  
**Scope:** `recover.rs`, WIP Guard additions in `intent.rs`, `git.rs`, `commands.rs`, `main.rs`, `radar.rs`  
**Reviewer:** code-analyzer

---

## Findings

### 1. [Medium] Double intent-log read in `handle_recover_list`
- **File:** `src/recover.rs:76–96`
- **Why it matters:** `collect_snapshots()` already loads the intent log. Then `handle_recover_list` loads it a second time just to check the stale-branch warning. This is two filesystem reads + two JSON parses for the same data.
- **Suggested change:** Have `collect_snapshots` return the loaded `IntentLog` alongside the entries (or accept an already-loaded log), and reuse it for the branch check.

### 2. [Medium] `last_sync_snapshot` is overwritten, not accumulated
- **File:** `src/intent.rs:227–237`
- **Why it matters:** Each call to `record_sync_snapshot` replaces the previous value. If a developer runs `tbdflow sync` twice in quick succession (e.g. after aborting and retrying), the first pre-sync snapshot is silently lost. Since the feature's goal is "never lose data," this is a design gap. The intent snapshots accumulate (one per note), but the sync snapshot does not.
- **Suggested change:** Consider storing `last_sync_snapshot` as a `Vec<String>` (or adding a timestamped note like the radar hook does), so multiple sync snapshots coexist. Alternatively, store each sync snapshot as a synthetic note with `SnapshotKind::PreSync` to keep the data model uniform.

### 3. [Medium] Pre-sync snapshot timestamp is wrong in `collect_snapshots`
- **File:** `src/recover.rs:55–62`
- **Why it matters:** The pre-sync snapshot entry uses `log.started_at` as its timestamp. This is the time the intent log was *created*, not when the sync snapshot was captured. If the log was created hours ago, the displayed timestamp is misleading.
- **Suggested change:** Store a `last_sync_snapshot_at: Option<String>` timestamp alongside the hash in `IntentLog`, or convert the sync snapshot to a regular note entry (which already carries its own timestamp).

### 4. [Low] Missing doc comments (`# Errors` section) on public functions
- **File:** `src/recover.rs:32`, `src/recover.rs:75`, `src/git.rs:902`, `src/git.rs:911`, `src/git.rs:920`, `src/intent.rs:161`, `src/intent.rs:227`
- **Why it matters:** Clippy pedantic flags these. Public functions returning `Result` should document failure modes.
- **Suggested change:** Add `/// # Errors` doc sections describing when each function fails.

### 5. [Low] Wildcard import in `recover.rs`
- **File:** `src/recover.rs:3`
- **Why it matters:** `use colored::*` imports all trait extensions. The rest of the codebase uses both `colored::*` (commands.rs, radar.rs) and `colored::Colorize` (intent.rs). The `Colorize` trait import is more explicit and preferred by clippy.
- **Suggested change:** Replace with `use colored::Colorize;` for consistency with `intent.rs`.

### 6. [Low] `SnapshotEntry.branch` field is never used
- **File:** `src/recover.rs:11`
- **Why it matters:** The `branch` field is populated in `collect_snapshots` but never read — not in the table display, not in the apply logic. Dead data.
- **Suggested change:** Either display it in the table (useful for the stale-branch context) or remove it.

### 7. [Low] No confirmation prompt before `recover apply`
- **File:** `src/recover.rs:161–166`
- **Why it matters:** The function prints a warning but doesn't actually prompt for confirmation. Other destructive commands in the codebase (`sync` with red CI, `undo`) use `dialoguer::Confirm`. Applying a snapshot over a dirty working directory could discard current changes.
- **Suggested change:** Add a `Confirm` prompt, consistent with the rest of the CLI. Skip it with `--dry-run`.

### 8. [Low] `radar_snapshot` silently swallows all errors
- **File:** `src/radar.rs:352–390`
- **Why it matters:** The function uses `unwrap_or`, `unwrap_or_default`, and `let _ =` to discard every possible error. While the intent is "silent background work," a verbose mode should surface failures so developers can diagnose issues.
- **Suggested change:** In verbose mode, log errors instead of discarding them. Example: `if verbose { eprintln!("Radar snapshot failed: {}", e); }`.

### 9. [Info] `stash_create` returns `Ok(None)` for clean directories — expected by callers
- **File:** `src/git.rs:902–908`
- **Why it matters:** Just confirming this is correct behaviour. `git stash create` outputs nothing when there's nothing to stash, and all callers (`main.rs`, `commands.rs`, `radar.rs`) handle `None` correctly with `if let Ok(Some(hash))`.

---

## Clippy / Compiler Warnings (WIP Guard scope)

| File | Warning | Severity |
|------|---------|----------|
| `recover.rs:3` | Wildcard import `colored::*` | pedantic |
| `recover.rs:32` | Missing `# Errors` doc section | pedantic |
| `recover.rs:75` | Missing `# Errors` doc section | pedantic |
| `git.rs:902` | Missing `# Errors` doc section | pedantic |
| `git.rs:911` | Missing `# Errors` doc section | pedantic |
| `git.rs:920` | Missing `# Errors` doc section | pedantic |
| `intent.rs:161` | Missing `# Errors` doc section | pedantic |
| `intent.rs:227` | Missing `# Errors` doc section | pedantic |

---

## Positives

- **Clean separation of concerns.** The snapshot logic lives in `recover.rs` and `git.rs`, with intent storage staying in `intent.rs`. No god-module tendencies.
- **Backward-compatible schema.** All new fields use `#[serde(skip_serializing_if = "Option::is_none")]`, so existing intent logs from before this feature load without error. The `legacy_log_without_branch_field_still_loads` test confirms this.
- **Consistent error handling.** All new `git.rs` functions use `Result` with `run_git_command` and avoid `.unwrap()`. The `?` operator is used correctly throughout.
- **`apply` not `pop`.** Using `git stash apply` instead of `pop` is the right call — it preserves the snapshot for repeated recovery.
- **Anti-collision check is well-placed.** Running it at the top of `handle_sync` and `handle_undo` before any git state mutation is the correct sequencing.
- **`SnapshotKind` enum with `Display`.** Clean, extensible, avoids stringly-typed data.

---

## Risks / Follow-ups

1. **No unit tests for `recover.rs`.** The module has zero tests. At minimum, test `collect_snapshots` with: no log, log with no snapshots, log with intent snapshots, log with sync snapshot, mixed. These are pure data-transformation functions that don't need a git repo.
2. **No unit test for `record_sync_snapshot`.** The function is straightforward but untested. Verify it creates the log when none exists and overwrites an existing snapshot.
3. **No test for `add_note_with_snapshot` preserving the hash.** The existing `add_note` tests don't verify `snapshot_hash` is stored and round-trips through JSON.
4. **`check_git_operation_in_progress` is untestable without fixtures.** Consider extracting the path-checking logic into a function that accepts a `&Path` (the git dir), making it unit-testable with tempdir fixtures.
5. **Radar hook staleness check only looks at last note timestamp, not last *snapshot* timestamp.** A note without a snapshot (e.g. added via `add_note` directly) would reset the 30-minute timer even though no snapshot was taken.

