test: add comprehensive coverage for TUI workflow phases

[james-m]

Summary

• Extract Transcriber, Writer, and EditorLauncher interfaces for dependency injection in workflow phases
• Add comprehensive test coverage for all 7 TUI workflow phases (recording, transcribing, view transcript, first draft, edit draft, copy edit, copy edit file)
• Create shared test infrastructure with mock implementations in workflow_test.go
• Test both happy path and existing output detection scenarios where applicable

Net change: +641 lines (16 files changed)

Test plan

• Run make test - all 11 new workflow tests pass
• Run make lint - no linter issues
• Verify voice CLI still works end-to-end

🤖 Generated with Claude Code

[claude]

PR Review: Test Coverage for TUI Workflow Phases

Summary

This is an excellent PR that significantly improves the codebase's testability and test coverage. The changes are well-structured, follow best practices, and demonstrate thoughtful engineering. The interface extraction is clean, and the test coverage is comprehensive.

✅ Strengths

1. Clean Interface Design

The extracted interfaces (Transcriber, Writer, EditorLauncher) follow the Interface Segregation Principle beautifully. Each interface is focused and represents a single responsibility:

// internal/tui/workflow/interfaces.go
type Transcriber interface {
    TranscribeFile(audioFile io.Reader) (string, error)
}

type Writer interface {
    GenerateFirstDraft(transcript string, mode content.Mode) (string, error)
    GenerateCopyEdit(firstDraft, currentDate string, mode content.Mode) (*content.CopyEditResult, error)
}

type EditorLauncher interface {
    Launch(filePath string) tea.Cmd
}

This is excellent Go idiom – the interfaces live in the consumer package (workflow), not the provider package (content). 👍

2. Comprehensive Test Coverage

All 7 workflow phases now have test coverage:

• ✅ Recording (happy path + existing output)
• ✅ Transcribing (happy path + existing output)
• ✅ View Transcript (happy path)
• ✅ First Draft (happy path + existing output)
• ✅ Edit Draft (happy path)
• ✅ Copy Edit (happy path)
• ✅ Copy Edit File (happy path + discard scenario)

The tests cover both success paths and edge cases like existing file handling and user cancellation.

3. Well-Structured Test Infrastructure

The shared test helpers in workflow_test.go are excellent:

• outputChecker with configurable timeouts
• Mock implementations with call tracking
• Consistent test patterns across all phases

The init() function setting lipgloss.SetColorProfile(termenv.Ascii) is a nice touch for reproducible terminal output testing.

4. DefaultEditorLauncher Extraction

Moving the editor launching logic from editDraftPhase into DefaultEditorLauncher makes the code more testable while preserving the original behavior. The struct embedding pattern allows easy mocking.

5. Dependency Injection Throughout

The refactoring to inject dependencies through constructors (e.g., NewFirstDraftPhase(writer Writer, ...)) is textbook clean code. This makes the phases testable without requiring real API calls.

6. Test Quality

• File operations use t.TempDir() for automatic cleanup
• Appropriate use of //nolint:gosec with justification for test files
• Eventually loops with reasonable timeouts for async behavior
• Assertions verify both side effects (files created) and state (mock call tracking)

🔍 Areas for Consideration

1. Mock Pointer Receivers vs Value Receivers

The mocks use pointer receivers for mutation tracking, but this creates a minor inconsistency with the interface methods. Consider being explicit about which methods need mutation:

// Current (works, but mixes semantics)
type mockWriter struct {
    firstDraftCalled bool
    copyEditCalled   bool
}

func (m *mockWriter) GenerateFirstDraft(...) (string, error) {
    m.firstDraftCalled = true  // mutation requires pointer
    return m.firstDraftResult, m.err
}

This is fine for tests, but consider if some methods could use value receivers if they don't track state.

2. Error Path Testing

The current tests focus on happy paths and user cancellation. Consider adding tests for:

• API failures (mock returns errors)
• File permission errors
• Malformed API responses

Example addition:

func TestFirstDraftPhase_APIError(t *testing.T) {
    writer := &mockWriter{err: errors.New("API rate limit exceeded")}
    // Verify error handling...
}

3. Test Timing Sensitivity

Several tests use require.Eventually() with 3-second timeouts. While this works for mocks that return immediately, consider:

• Documenting why specific timeouts were chosen
• Using shorter timeouts for unit tests (100ms may be sufficient)
• Potentially making the checker timeout configurable per-test

// Consider:
func quickChecker() outputChecker {
    return outputChecker{
        intervl: 10 * time.Millisecond,
        timeout: 500 * time.Millisecond,  // Faster for unit tests
    }
}

4. Missing Test: DefaultEditorLauncher.Launch()

The DefaultEditorLauncher itself isn't unit tested. While editdraft_test.go tests the integration, consider adding a unit test for the Launch() method's branching logic (empty vs non-empty EditorCmd).

5. Code Coverage Gaps

Per the PR checklist, "Verify voice CLI still works end-to-end" is unchecked. While unit tests are excellent, consider:

• Adding integration tests or a smoke test script
• Documenting manual testing procedures
• Creating a make test-integration target

🛡️ Security & Quality

• ✅ No security concerns
• ✅ Proper file permissions (0o644 for files, 0o755 for directories)
• ✅ Appropriate use of gosec suppressions with clear justifications
• ✅ No hardcoded credentials or sensitive data
• ✅ Test isolation via temp directories

📊 Code Quality Metrics

• Lines Added: +641
• Lines Removed: -33
• Net Change: +608 lines
• Files Changed: 16
• Test Files Added: 7 new test files
• All tests pass: ✅
• Linter issues: N/A (linter not available in CI environment, but code follows project conventions)

🎯 Recommendations

Priority 1 (Before Merge)

1. ✅ Complete the test plan checklist – Verify the voice CLI works end-to-end manually
2. ✅ Run make lint locally to ensure no style violations

Priority 2 (Follow-up PRs)

1. Add error path testing for API failures and I/O errors
2. Add unit test for DefaultEditorLauncher.Launch() branching logic
3. Consider shorter timeouts for unit tests (document reasoning)
4. Add integration test or smoke test for full workflow

Priority 3 (Nice to Have)

1. Document test strategy in docs/testing.md
2. Add code coverage reporting to CI
3. Consider property-based testing for mock implementations

📝 Style Guide Compliance

This PR follows the project's Go Style Guide excellently:

• ✅ Guideline Phase 1a: Local Dev Setup #1: All errors are properly wrapped with context
• ✅ Guideline Security Hardening #3: Uses io.Reader interface for file operations (already in place)
• ✅ Guideline Security Fix #4: Uses SDK-provided types (anthropic.Model, etc.)
• ✅ Guideline feat: First Post Post #8: Consistent use of strings.Builder in view functions (existing code)

No violations detected.

🎉 Conclusion

This is high-quality work that significantly improves the codebase. The interface extraction is clean, the tests are comprehensive, and the code follows Go best practices throughout. The PR demonstrates strong understanding of:

• Dependency injection
• Interface-driven design
• TUI testing patterns
• Test infrastructure design

Recommendation: Approve after completing the manual test plan verification. The code quality is excellent, and the minor suggestions above can be addressed in follow-up PRs.

Great work, @james-m! 🚀

---

Test Results: All 11 new workflow tests pass ✅

ok  	github.com/alkime/memos/internal/tui/workflow	1.570s