specs/011-dashboard-testing/review.md
Ref: Size: 5.8 KiB
# Fleet Review: 011-dashboard-testing
**Reviewed**: spec.md, plan.md, tasks.md
**Date**: 2026-03-21
---
## 1. Spec-Plan Alignment
**Status**: PASS with minor note
The plan covers all three user stories (US1: unit tests, US2: render tests, US3: integration tests) and maps directly to the spec's acceptance scenarios. All functional requirements (FR-001 through FR-010) are addressed.
**Minor discrepancy**: The plan's project structure section references `specs/010-dashboard-testing/` (wrong number -- should be `011`). This is cosmetic and does not affect implementation.
**Decision on visibility**: The spec (edge cases section, FR-001) suggests making App `pub(crate)` OR using an in-module test submodule. The plan correctly chooses the in-module approach, which is simpler and avoids polluting the crate's public API. Good decision.
---
## 2. Plan-Tasks Completeness
**Status**: PASS
All plan phases are covered by tasks:
| Plan Phase | Tasks | Coverage |
|-----------|-------|----------|
| Phase 1: Refactor for testability | T001-T004 | handle_key extraction, run_loop update, helpers |
| Phase 2: Test helpers | T003, T026, T033 | Spread across phases where needed |
| Phase 3: Unit tests (US1) | T005-T025 | 21 tests -- exceeds spec's SC-001 (10 minimum) |
| Phase 4: Render tests (US2) | T026-T032 | 6 render tests -- exceeds spec's SC-002 (3 minimum) |
| Phase 5: Integration tests (US3) | T033-T036 | 3 integration tests -- exceeds spec's SC-003 (2 minimum) |
| Phase 6: CI/Polish | T037-T039 | Clippy, timing, headless verification |
All spec edge cases are addressed:
- Empty list navigation: T011, T023
- show_all persistence across tabs: T024
- Selection clamping on filter toggle: T025
- Small terminal: T032
---
## 3. Dependency Ordering
**Status**: PASS
- Phase 1 (refactor) has no prerequisites and must complete before any tests.
- T001 (handle_key extraction) is correctly ordered before T017-T022 (handle_key tests) and T033-T036 (integration tests).
- T003 (helpers) is correctly ordered before Phase 2 tests.
- T026 (buffer helpers) is correctly ordered before Phase 3 render tests.
- T033 (apply_keys helper) is correctly ordered before Phase 4 integration tests.
- Phases 2 and 3 can run in parallel after Phase 1, which is correctly noted.
---
## 4. Feasibility
### 4a. ratatui TestBackend in 0.30
**Status**: VERIFIED
`ratatui::backend::TestBackend` is present in the ratatui 0.30.0 source at `~/.cargo/registry`. It is used extensively in ratatui's own test suite and doc examples. Usage pattern: `Terminal::new(TestBackend::new(width, height))`, then inspect `terminal.backend().buffer()`. No feature flags needed.
### 4b. App struct visibility
**Status**: VERIFIED -- No changes needed
The plan correctly identifies that in-module `#[cfg(test)] mod tests` has access to all private types in `src/tui.rs`. The App struct, Tab, Pane, ViewMode enums, and all methods are accessible without any visibility changes. This is the simplest approach.
### 4c. handle_key extraction feasibility
**Status**: VERIFIED -- Straightforward
The current key handling in `run_loop()` (lines 258-308 of `src/tui.rs`) is a self-contained `match` block on `key.code`. It only mutates `app` fields and calls `app.reload(repo)` for the 'r' key. Extraction into `App::handle_key()` is clean with one caveat:
**Caveat**: The 'r' (reload) key calls `app.reload(repo)` which requires a `&Repository`. The extracted `handle_key()` method either needs a `repo` parameter or the reload case needs special handling. The plan's signature `handle_key(&mut self, code: KeyCode, modifiers: KeyModifiers) -> bool` does not include `repo`. Options:
1. Add `repo: Option<&Repository>` parameter
2. Return an enum `HandleResult { Continue, Quit, Reload }` and let `run_loop` handle reload
3. Accept `repo: &Repository` as a parameter
**Recommendation**: Option 2 (return enum) is cleanest -- it keeps handle_key free of git2 dependency for testing, and most tests don't need reload behavior. This is a minor design decision to resolve during T001.
### 4d. Test data construction
**Status**: VERIFIED with note
`IssueState` and `PatchState` are `pub` structs with `pub` fields, so they can be constructed directly in tests. The `Comment` struct contains a `commit_id: Oid` field from git2. This can be satisfied with `Oid::from_bytes(b"00000000000000000000").unwrap()` (20 bytes) without needing a real repository. No blocker.
---
## 5. Risk Assessment
| Risk | Severity | Mitigation |
|------|----------|------------|
| handle_key extraction breaks existing behavior | Low | T004 verifies cargo test + clippy pass after refactor; run_loop logic is straightforward |
| TestBackend API differences in ratatui 0.30 vs docs | Low | Verified in source; ratatui's own tests use the same pattern |
| Comment::commit_id requires Oid construction | Low | Oid::from_bytes with 20 zero bytes works without a repo |
| Test module grows too large in tui.rs | Medium | Plan acknowledges this; can extract to tui/tests.rs later if needed |
| 'r' key (reload) handling in handle_key | Low | Addressed in feasibility 4c; minor design decision for T001 |
---
## 6. Summary
The spec, plan, and tasks are well-aligned and feasible. The feature is low-risk since it adds only test code and a minor refactor. Key findings:
1. **One design decision needed**: handle_key() must handle the 'r'/reload case that requires a Repository. Recommend returning an action enum instead of a bool.
2. **One typo in plan**: `specs/010-dashboard-testing/` should be `specs/011-dashboard-testing/`.
3. **All external dependencies verified**: TestBackend exists in ratatui 0.30, tempfile is in dev-deps, Oid can be constructed without a repo.
4. **Task count exceeds all success criteria minimums**: 21 unit tests (min 10), 6 render tests (min 3), 3 integration tests (min 2).
**Verdict**: Ready for implementation.