specs/008-commit-browser/review.md

Ref: main   Size: 5.0 KiB

# Pre-Implementation Review: 008-commit-browser

**Reviewer**: Claude Opus 4.6 (cross-model review)
**Date**: 2026-03-21

## Summary Table

| Criterion               | Status  | Notes                                                        |
|--------------------------|---------|--------------------------------------------------------------|
| Spec-Plan Alignment      | PASS    | All 3 user stories and edge cases addressed                  |
| Plan-Tasks Completeness  | PASS    | 19 tasks cover all plan items                                |
| Dependency Ordering      | PASS    | Phases correctly sequenced; parallel opportunities accurate  |
| Feasibility              | FAIL    | Plan references `signing::verify_dag()` which does not exist |
| Risk                     | MEDIUM  | One naming error; `'c'` key is available; single-file scope  |

## Detailed Findings

### 1. Spec-Plan Alignment: PASS

All three user stories are addressed:
- **US1** (event history list): Plan section 2 (event data loading) + section 3 (event list rendering) + section 5 (navigation)
- **US2** (individual event details): Plan section 4 (event detail rendering) + section 5 (Enter/Escape navigation)
- **US3** (signature status): Plan section 3 (sig_icon in list rows) + section 4 (signature info in detail)

All edge cases from the spec are covered by Phase 5 polish tasks (T018, T019) and T009 (error handling).

### 2. Plan-Tasks Completeness: PASS

Every plan section maps to at least one task:
- ViewMode extension -> T001
- Event data loading -> T005, T015
- Event list rendering -> T008, T016
- Event detail rendering -> T012, T013, T017
- Navigation -> T005-T007, T010-T011, T014
- Error handling -> T009
- Edge cases -> T018, T019

No plan items are missing task coverage.

### 3. Dependency Ordering: PASS

Phase ordering is correct:
- Phase 1 (setup) has no dependencies -- correct
- Phase 2 (US1) depends on Phase 1 for ViewMode variants and fields -- correct
- Phase 3 (US2) depends on Phase 2 for loaded event list -- correct
- Phase 4 (US3) depends on Phase 2 but not Phase 3 -- correct, can parallelize with Phase 3
- Phase 5 (polish) depends on all -- correct

### 4. Feasibility: FAIL - Function Name Mismatch

**Critical issue**: The plan references `signing::verify_dag()` in the summary and in design decision 2. This function does not exist. The actual function is:

```
signing::verify_ref(repo: &Repository, ref_name: &str) -> Result<Vec<SignatureVerificationResult>, Error>
```

This appears in tasks T003, T015, T016, and T017 (which reference `signing::verify_dag()`). The function signature and return type are otherwise exactly as the plan describes -- the only error is the name.

**Impact**: Low severity. The fix is a simple rename in the plan/tasks (`verify_dag` -> `verify_ref`). The actual implementation will need to call `signing::verify_ref()`.

**Other feasibility checks -- all pass**:
- `dag::walk_events(repo, ref_name) -> Result<Vec<(Oid, Event)>, Error>` -- confirmed, exists at `dag.rs:59` with exactly the signature the plan describes
- `ViewMode` enum at `tui.rs:30` has `Details` and `Diff` variants -- matches plan
- `App` struct at `tui.rs:68` has `mode: ViewMode`, `status_msg: Option<String>`, `scroll: u16`, `list_state: ListState` -- matches plan assumptions
- `InputMode` enum has `Normal`, `Search`, `CreateTitle` -- matches T019 (but plan should also list `CreateBody` if it exists)
- `Pane` enum has `ItemList` and `Detail` -- matches plan's navigation model
- `SignatureVerificationResult` struct and `VerifyStatus` enum exist in `signing.rs` with the fields described
- Key `'c'` is not bound in Normal mode (only `Ctrl+c`) -- available as planned

### 5. Risk Assessment

| Risk | Severity | Mitigation |
|------|----------|------------|
| `verify_dag` does not exist (it's `verify_ref`) | Low | Rename in implementation; same signature/return type |
| All 19 tasks in single file (`tui.rs`) limits parallelism | Low | Acceptable for feature scope; phases provide natural ordering |
| No test tasks included | Medium | Tasks doc notes "test tasks omitted" -- consider adding integration tests for event loading at minimum |
| `walk_events` + `verify_ref` both walk the DAG independently | Low | Performance acceptable for <100 events per spec SC-003; could optimize later by combining walks |
| `CreateBody` input mode not mentioned in T019 | Low | The grep shows `CreateTitle` exists in InputMode; verify `CreateBody` also exists and add to T019 guard |

## Recommendations

1. **Fix the function name**: Replace all references to `signing::verify_dag()` with `signing::verify_ref()` in plan.md and tasks.md.
2. **Add basic tests**: Even though tests were not requested, consider at least one test for the `action_type_label` helper (T004) and the `format_event_detail` helper (T012) since they are pure functions.
3. **Check CreateBody**: Verify whether `InputMode::CreateBody` exists and ensure T019 guards against it as well.
4. **Consider combined DAG walk**: `walk_events()` and `verify_ref()` both do full revwalks. A future optimization could combine them, but this is not a blocker.