refactor: Extract explosion handling into ExplosionCoordinator

[yewreeka]

Summary

Extract explosion-related code from ConversationViewModel into a dedicated ExplosionCoordinator class for better separation of concerns.

Changes

New file: ExplosionCoordinator.swift

• Owns ExplodeState management
• Manages explosion task lifecycle
• Provides explode() and scheduleExplosion() operations
• Self-contained, testable unit

Updated: ConversationViewModel.swift

• Holds ExplosionCoordinator as a dependency
• Forwards explodeState as computed property
• Delegates explosion actions to coordinator
• Removes direct explosion task management

Benefits

• Testability: Explosion logic can be tested in isolation
• Separation of concerns: ConversationViewModel is ~50 lines smaller
• Clarity: Explosion behavior is encapsulated in one place

Part of

Tech debt paydown plan for post-1.1.0 release (see PR #525 for documentation).

Note

Extract explosion orchestration into ExplosionCoordinator and update ConversationViewModel to delegate explode and scheduleExplosion to ExplosionCoordinator in ConversationViewModel.swift and add new coordinator in ExplosionCoordinator.swift

Move explosion state and task management from ConversationViewModel to ExplosionCoordinator, proxy explodeState through coordinator, and route explodeConvo and scheduleExplosion to coordinator APIs.

📍Where to Start

Start with the ExplosionCoordinator class and its explode and scheduleExplosion methods in ExplosionCoordinator.swift. Then see how ConversationViewModel delegates in ConversationViewModel.swift.

^{Macroscope summarized 83c6f30.}

[claude]

Claude finished @yewreeka's task —— View job

---

Code Review Complete ✅

Tasks

• Read changed files
• Analyze code quality and best practices
• Check for potential bugs
• Review performance considerations
• Assess security concerns
• Evaluate test coverage

---

Summary

This refactoring successfully extracts explosion logic into a dedicated coordinator, improving separation of concerns. The implementation is solid with good error handling and task lifecycle management. A few minor improvements are recommended below.

---

Code Quality & Best Practices

✅ Strengths:

• Clean separation of concerns - ExplosionCoordinator is now a focused, single-responsibility class
• Proper use of @Observable and @MainActor following modern Swift concurrency patterns
• Good task lifecycle management with cancellation in deinit
• Consistent with project conventions (dependency injection via protocols, use of @ObservationIgnored)

⚠️ Recommendations:

1. ExplosionCoordinator.swift:16-18 - Consider making explosionWriter dependency injectable via protocol:

   // Current: Protocol already used ✅
   init(explosionWriter: any ConversationExplosionWriterProtocol)

   This is already good practice for testability.

2. Computed property forwarding - ConversationViewModel.swift:213 forwards explodeState:

   var explodeState: ExplodeState { explosionCoordinator.state }

   This is a clean approach that maintains the existing API surface while delegating to the coordinator.

---

Potential Bugs & Issues

⚠️ Minor Issue - State Machine Guard Logic:

ExplosionCoordinator.swift:30 and ExplosionCoordinator.swift:68 have slightly different guard conditions:

// explode() allows error OR scheduled
guard state.isReady || state.isError || state.isScheduled else { return }

// scheduleExplosion() only allows error
guard state.isReady || state.isError else { return }

Question: Should scheduleExplosion() also allow transitioning from .scheduled state? If a user schedules an explosion, then changes the time, it seems reasonable to allow rescheduling. Consider:

guard state.isReady || state.isError || state.isScheduled else { return }

⚠️ Task Cancellation Race Condition:

Both methods follow this pattern:

explodeTask?.cancel()
explodeTask = Task { [weak self] in
    guard let self else { return }
    // async work...
}

This creates a potential race: the previous task is cancelled, but the new task starts immediately. If the previous task hasn't finished its error handling (lines 56-58 in explode()), there could be state conflicts. Consider using Task.yield() after cancellation or storing the previous task and awaiting its cancellation.

---

Performance Considerations

✅ Good:

• Task cancellation prevents unnecessary work when operations are superseded
• [weak self] captures prevent retain cycles
• State transitions are synchronous (no async gaps)

ℹ️ Note:

• The 2-second error display delay (ExplosionCoordinator.swift:56,92) is reasonable for UX but means the coordinator holds state longer. Not a concern for typical usage patterns.

---

Security Concerns

✅ No security issues identified:

• No sensitive data exposed in logs (only conversation IDs, which are internal references)
• Proper error handling without leaking implementation details
• No force unwrapping or unsafe operations

---

Test Coverage

⚠️ Testing Recommendations:

The ExplosionCoordinator is now independently testable. Suggested test cases:

1. State Transitions:

   • Test all valid state transitions (.ready → .exploding → .exploded)
   • Test guard conditions prevent invalid state transitions
   • Test error state recovery after 2-second delay

2. Task Lifecycle:

   • Verify task cancellation on deinit
   • Verify task cancellation when new operation starts
   • Test [weak self] behavior when coordinator is deallocated mid-operation

3. Error Handling:

   • Mock explosionWriter failures
   • Verify error state is set with correct message
   • Verify recovery to .ready state after delay

4. Integration with ConversationViewModel:

   • Verify computed property forwarding works correctly
   • Test that callbacks (onExploded, onImmediateExplosion) are invoked

Example test structure:

@MainActor
final class ExplosionCoordinatorTests: XCTestCase {
    var coordinator: ExplosionCoordinator!
    var mockWriter: MockConversationExplosionWriter!
    
    override func setUp() async throws {
        mockWriter = MockConversationExplosionWriter()
        coordinator = ExplosionCoordinator(explosionWriter: mockWriter)
    }
    
    func testExplodeTransitionsToExplodingState() async {
        // Given: coordinator in ready state
        XCTAssertTrue(coordinator.state.isReady)
        
        // When: explode is called
        coordinator.explode(/* ... */)
        
        // Then: state transitions to exploding
        XCTAssertEqual(coordinator.state, .exploding)
    }
}

---

Additional Notes

1. Line count reduction: The ViewModel went from ~1195 lines to ~1125 lines (70 lines removed), while the new coordinator is 98 lines. Net reduction of ~50 lines of complexity in the ViewModel.

2. Consistency with CLAUDE.md conventions: ✅

   • Uses @Observable pattern correctly
   • Follows dependency injection via protocols
   • Proper use of @MainActor and @ObservationIgnored
   • Clean naming conventions

3. Future improvements: Consider extracting other coordinators from ConversationViewModel using the same pattern (e.g., OnboardingCoordinator is already partially extracted).

---

Verdict

Approved with minor recommendations. This is a clean refactoring that improves testability and maintainability. The identified issues are minor and non-blocking:

• State machine logic is consistent enough for current use case
• Task cancellation race is theoretical and unlikely to cause issues in practice
• Test coverage recommendations are for future work

Great job on reducing complexity in ConversationViewModel! 🎉

---

[yewreeka]

Closing - the extraction doesn't provide enough value to justify the churn.