specs/009-open-file-at-comment/review.md

Ref: main   Size: 5.6 KiB

# Pre-Implementation Review

**Feature**: Open File at Inline Comment Location from Dashboard
**Artifacts reviewed**: spec.md, plan.md, tasks.md, checklists/requirements.md
**Review model**: Claude Opus 4.6 (1M context)
**Generating model**: Unknown (spec/plan), Claude Opus 4.6 (tasks)

## Summary

| Dimension | Verdict | Issues |
|-----------|---------|--------|
| Spec-Plan Alignment | PASS | All 7 FRs, both user stories, and all edge cases covered |
| Plan-Tasks Completeness | PASS | Every plan phase has corresponding tasks; all architectural components covered |
| Dependency Ordering | PASS | Correct phase ordering: setup -> US1 -> US2 -> polish |
| Parallelization Correctness | PASS | No [P] markers used; correct since all tasks touch src/tui.rs |
| Feasibility & Risk | WARN | Two design concerns with colorize_diff refactor and render_detail mutability |
| Standards Compliance | PASS | Constitution is a template (no rules to violate); no new dependencies |
| Implementation Readiness | WARN | Two tasks need more specificity around mutability and terminal lifecycle |

**Overall**: READY WITH WARNINGS

## Findings

### Critical (FAIL -- must fix before implementing)

None.

### Warnings (WARN -- recommend fixing, can proceed)

1. **T005 mutability conflict with render_detail**: The current `render_detail()` function takes `app: &App` (immutable reference). Storing `comment_positions` from `colorize_diff()` back into `app` during rendering requires either (a) changing `render_detail` to take `&mut App`, (b) moving the comment-position computation out of the render path into the pre-render cache block in `run_loop()` (around lines 241-252 where diff_cache is populated), or (c) using interior mutability (`RefCell`). Option (b) is the cleanest approach and aligns with the existing `diff_cache` pattern. The task description acknowledges this with "(requires `app` to be `&mut App` or positions stored via a different mechanism)" but should prescribe a specific approach to avoid implementation ambiguity.

2. **T008 terminal lifecycle**: The `open_editor_at()` function takes a `Terminal` reference for suspend/resume, but the actual `terminal` variable is owned by `run_loop()`, not directly accessible from a standalone function. The function signature should also include `stdout` handling (the current `run()` function calls `stdout().execute(LeaveAlternateScreen)` directly). The implementation should follow the exact pattern from lines 222-229 of `src/tui.rs` for consistency. Consider passing `terminal` as `&mut Terminal<CrosstermBackend<io::Stdout>>` and using `crossterm::execute!(io::stdout(), LeaveAlternateScreen)` directly rather than going through the terminal backend.

3. **Edge case: `$EDITOR` with arguments**: The spec (edge case 4) calls out handling editors like `code --wait`. T007 says "split on whitespace" which is a basic approach but could break paths with spaces (e.g., `/usr/local/My Editor/bin/editor`). This is an acceptable tradeoff for v1 and matches common Unix tool behavior, but worth noting.

4. **No test tasks**: The spec does not explicitly request tests and the tasks omit them. However, `find_comment_at_scroll()` and `resolve_editor()` are pure functions that would benefit from unit tests. The user's memory notes a TDD preference. Consider adding test tasks for these two helpers at minimum.

### Observations (informational)

1. **Verified struct existence**: `InlineComment` exists at `src/state.rs:50` with fields `author`, `file`, `line`, `body`, `timestamp` -- matches spec's Key Entities section exactly. No changes to `state.rs` or `event.rs` needed.

2. **Verified colorize_diff exists**: `colorize_diff()` at `src/tui.rs:706` already tracks `current_file` and `current_new_line` internally, so building the comment-position mapping (T004) is a natural extension of existing logic.

3. **No existing status_msg or InputMode**: The current `tui.rs` has no status message mechanism or input mode enum. T001 introduces `status_msg` which is the minimal approach. If feature 008 (commit browser) lands first, there may be a `status_msg` field already added -- check before implementing.

4. **Scope is well-contained**: 21 tasks, all in `src/tui.rs`, estimated at 50-100 lines of new code. This is proportional to the feature complexity.

5. **Plan phase numbering vs task phases**: The plan describes 5 phases (track positions, find nearest, editor launch, keybinding, status message) while tasks consolidate into 4 phases (setup, US1, US2, polish). This is a valid simplification -- the plan's phases 1-4 map to tasks Phase 1+2 (US1), and plan phase 5 maps to tasks Phase 3 (US2).

6. **`render_detail` currently calls `colorize_diff` inline**: The diff text is computed inside `render_detail()` at line 477. Since `render_detail` takes `&App`, the comment positions cannot be stored there. The recommended approach is to compute comment positions in the same pre-render block as `diff_cache` (lines 241-252 of `run_loop()`), or compute them alongside the diff and store both in the cache.

## Recommended Actions

- [ ] Clarify T005: prescribe computing comment positions in the `run_loop()` pre-render block (alongside `diff_cache` population at lines 241-252) rather than inside `render_detail()`
- [ ] Clarify T008: specify that `terminal` is passed as `&mut Terminal<CrosstermBackend<io::Stdout>>` and that suspend/resume uses `crossterm::execute!(io::stdout(), ...)` directly
- [ ] Consider adding unit test tasks for `find_comment_at_scroll()` and `resolve_editor()` given user's TDD preference
- [ ] Check whether feature 008 has already introduced a `status_msg` field to `App` before implementing T001