Start refactoring convo view model

[yewreeka]

Refactor ConversationViewModel to source repositories and writers from MessagingService.conversationStateManager(for:) for existing and draft conversations

Route ConversationViewModel dependencies through ConversationStateManager, update initializers to drop myProfileRepository in favor of draftConversationRepository, initialize editing state from the repository-fetched conversation, add MessagingService.conversationStateManager(for:), and extend the state machine with useExisting(conversationId:) plus tests. Also add a cacheUpdates publisher to the mock image cache.

📍Where to Start

Start with ConversationViewModel wiring in Convos/Conversation Detail/ConversationViewModel.swift, then review the factory in ConvosCore/Sources/ConvosCore/Messaging/MessagingService.swift and the state transition in ConvosCore/Sources/ConvosCore/Inboxes/ConversationStateMachine.swift.

---

Macroscope summarized 9ce42ef.

[claude]

Claude finished @yewreeka's task —— View job

---

Code Review Complete ✅

Todo List

• Read and understand core refactoring changes
• Review code quality and best practices
• Identify potential bugs or issues
• Assess performance considerations
• Check for security concerns
• Evaluate test coverage
• Provide comprehensive feedback

---

Summary

This PR successfully refactors the ConversationViewModel initialization to use a centralized ConversationStateManager and adds support for using existing conversations via the new .useExisting state machine action. The changes improve dependency management and code organization. Overall, the refactoring is well-executed with excellent test coverage.

Code Quality & Best Practices ✅

Strengths:

• Clean separation of concerns with ConversationStateManager centralizing conversation lifecycle dependencies
• Consistent API design with both conversationStateManager() for new conversations and conversationStateManager(for:) for existing ones
• Good use of Swift protocols for dependency injection
• Code follows established Swift patterns and the project's observable architecture

Minor Observations:

• ConversationViewModel now has two initializers (lines 137-183 and 186-226), which is appropriate for the different use cases (existing vs draft conversations)
• The alternative initializer at line 186 could benefit from a clearer documentation comment explaining when to use it vs the primary initializer

Potential Bugs or Issues ⚠️

1. Missing .error state handling for .useExisting action (ConversationStateMachine.swift:305)

• Issue: The state machine handles .create and .validate from .error state, but .useExisting is not handled
• Impact: If the state machine is in .error state and useExisting is called, it will fall through to default and log a warning without transitioning
• Recommendation: Add error state handling similar to other actions:

case (.error, let .useExisting(conversationId)):
    handleStop()
    handleUseExisting(conversationId: conversationId)

• Location: ConversationStateMachine.swift around line 305 (after the existing .error handlers)

2. Potential memory leak in ConversationStateManager initialization (ConversationStateManager.swift:143-145)

• Issue: Macroscope flagged that the Task created at lines 143-145 captures self strongly but isn't tracked for cancellation like stateObservationTask
• Impact: Low - The task is very short-lived (just calls useExisting), but creates inconsistency with the pattern used for stateObservationTask
• Recommendation: Consider storing and cancelling in deinit for consistency, or document why this is safe:

initializationTask = Task { [weak self, stateMachine] in
    await stateMachine.useExisting(conversationId: conversationId)
}

• Already cancelled in deinit (line 152), so this is good ✓

3. MockMessagingService.conversationStateManager(for:) doesn't use injected dependency (MockMessagingService.swift:64)

• Issue: The method creates a new MockConversationStateManager instead of returning the injected _conversationStateManager
• Impact: Tests that inject a custom conversation state manager won't see it used when calling this method
• Recommendation: Either return the injected instance or document why per-conversation instances are intentional
• Location: MockMessagingService.swift:63-65

Performance Considerations ✅

Positive:

• The .useExisting action transitions directly to .ready state without async operations, providing instant access to existing conversations
• No redundant database queries or network calls introduced
• State machine properly queues and processes messages in order through AsyncStream

Observations:

• The refactoring adds an extra layer of indirection through ConversationStateManager, but this is negligible and improves code organization
• ConversationStateManager initializes multiple writers upfront (lines 103-122), which is fine for the current architecture

Security Concerns ✅

No security issues identified. The refactoring maintains existing security patterns:

• Proper use of identity store and keychain for sensitive data
• No exposure of internal IDs or tokens
• Appropriate validation of conversation IDs through existing mechanisms

Test Coverage ⭐ Excellent

Strengths:

• Comprehensive test suite added with 358 new lines in ConversationStateMachineTests.swift
• Tests cover all major flows: create, useExisting, join, delete, stop
• Tests include edge cases: cancellation, network disconnection, error states
• Well-structured tests with clear naming and documentation
• Good use of test helpers like waitForMessages for reliable async testing

Test Coverage Highlights:

• ✅ .useExisting flow tested (lines 1076-1162)
• ✅ Messages can be sent after useExisting (lines 1164-1247)
• ✅ State sequence validation (lines 1249-1351)
• ✅ useExisting after stop (lines 1353-1430)
• ✅ Cancellation scenarios
• ✅ Network disconnection/reconnection

Potential Gap:

• Consider adding a test for calling useExisting from .error state once that's implemented

Recommendations

High Priority:

1. Add .error state handling for .useExisting action to match other action patterns

Medium Priority:
2. Review MockMessagingService.conversationStateManager(for:) to ensure it behaves correctly in tests
3. Add a doc comment to the alternative ConversationViewModel initializer explaining its use case

Low Priority:
4. Consider adding a test case for useExisting from .error state

Architectural Notes

This refactoring demonstrates good architectural thinking:

• Before: ConversationViewModel directly constructed all its dependencies from the messaging service
• After: ConversationStateManager serves as a coordination layer that bundles related dependencies
• Benefits:
  • Clearer separation between new conversation creation and existing conversation usage
  • Easier testing with the centralized manager
  • More maintainable as conversation-related dependencies evolve

The .useExisting action is a smart addition that allows immediate access to existing conversations without going through the create/validate/join flows.

---

[yewreeka]

• Remove max convos limit #301
• Start refactoring convo view model #280 👈 (View in Graphite)
• dev

---

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.