Remove max convos limit

[yewreeka]

First pass at inbox lifecycle manager to remove max convos limit

wake clients for push notifs when app is active

[claude]

Claude finished @yewreeka's task —— View job

---

Code Review Complete ✅

I've completed a comprehensive review of PR #301. This is a significant architectural change that removes the max conversation limit by introducing an inbox lifecycle management system. Overall, the implementation is well-structured with good test coverage, but there are several important issues to address.

---

🎯 Architecture & Design

Strengths

• Well-designed lifecycle system: The InboxLifecycleManager provides a clean abstraction for managing inbox wake/sleep states with LRU eviction
• Good separation of concerns: Clear distinction between lifecycle management, activity tracking, and pending invite handling
• Protocol-based design: Proper use of protocols (InboxLifecycleManagerProtocol, repository protocols) enables testability
• Comprehensive test coverage: 542 lines of lifecycle tests, 368 lines of checker tests demonstrate thorough testing

Concerns

• Global mutable state in XMTPAPIOptionsBuilder.swift:15-20: Setting XMTPEnvironment.customLocalAddress globally causes race conditions and makes instances non-thread-safe. This should be scoped per-client or passed via parameters.

---

🐛 Critical Bugs & Issues

1. Race Condition in ConversationsViewModel.swift:92-95

Task { [weak self] in
    guard let self else { return }
    await session.setActiveClientId(self.selectedConversation?.clientId)
}

Issue: The async task can execute out of order if selection changes rapidly. The selectedConversation captured in the closure may not match the current selection.

Fix: Check that selection hasn't changed before setting:

Task { [weak self] in
    guard let self else { return }
    guard self._selectedConversationId == conversation.id else { return }
    await session.setActiveClientId(conversation.clientId)
}

Reference: ConversationsViewModel.swift:92-95

2. Inconsistent Selection State on Error (ConversationsViewModel.swift:72)

When ConversationViewModel.create throws, _selectedConversationId remains set but selectedConversationViewModel is nil, causing state drift.

Fix: Clear selection on error:

} catch {
    Log.error("Failed to create conversation view model: \(error)")
    self.selectedConversationViewModel = nil
    self._selectedConversationId = nil
}

3. Memory Leak in SleepingInboxMessageChecker.swift:70-76

The periodic check task captures self weakly but continues looping after deallocation:

periodicCheckTask = Task { [weak self] in
    while !Task.isCancelled {
        try? await Task.sleep(for: .seconds(self?.checkInterval ?? 60))
        guard !Task.isCancelled else { break }
        await self?.checkNow()  // no-op when self is nil
    }
}

Fix: Break when self becomes nil:

while !Task.isCancelled {
    guard let self else { break }
    try? await Task.sleep(for: .seconds(self.checkInterval))
    guard !Task.isCancelled else { break }
    await self.checkNow()
}

Reference: SleepingInboxMessageChecker.swift:70-76

4. Thread Safety Issue in PendingInviteRepository.swift:128

public final class MockPendingInviteRepository: PendingInviteRepositoryProtocol, @unchecked Sendable {
    public var pendingInvites: [PendingInviteInfo] = []

Issue: @unchecked Sendable with mutable public var risks data races in concurrent test scenarios.

Fix: Make it an actor:

public actor MockPendingInviteRepository: PendingInviteRepositoryProtocol {
    public var pendingInvites: [PendingInviteInfo] = []

Reference: PendingInviteRepository.swift:128

5. Identifiable Violation in PendingInviteRepository.swift:6

public var id: String { clientId }

Issue: Results are grouped by (clientId, inboxId) but id only uses clientId. If a client has multiple inboxes, this creates duplicate IDs.

Fix: Use both fields:

public var id: String { "\(clientId)|\(inboxId)" }

Reference: PendingInviteRepository.swift:6

---

⚠️ Potential Issues

6. Active Inbox Not Protected from Eviction (SessionManager.swift:322)

The comment in the existing review is correct: when waking an inbox for push notification at capacity, the currently active (viewing) inbox could be evicted. This would cause a jarring UX where the conversation the user is viewing suddenly stops working.

Recommendation: Exclude activeClientId in all LRU eviction paths, not just in rebalance().

7. Fail-Open Behavior May Mask Real Errors

In multiple places (e.g., InboxLifecycleManager.swift:178-184), pending invite checks fail open (assume true on error). While defensive, this could mask database corruption or schema issues.

Recommendation: Add metrics/logging to track how often this fallback is triggered.

8. Missing Inbox Waking on App Launch (InboxLifecycleManager.swift:321)

As noted in review comments, initializeOnAppLaunch() only wakes inboxes that have activity records. Pending invite inboxes without activity are now handled (lines 342-356), but the logic is complex and error-prone.

9. Notification State Race (MessagingService+PushNotifications.swift:152)

Notifications fire before conversation state exists. If a user taps the notification immediately, the app may crash or show a blank screen.

Recommendation: Persist conversation before posting notification (suggestion in existing review is correct).

10. Single Point of Failure in Batch Operations

rebalance() and other batch operations can abort on first error. While some paths now have per-inbox do-catch (lines 294-300), this pattern should be consistent everywhere.

---

🔒 Security Concerns

No Critical Security Issues Found ✅

• Proper use of fail-open for availability (pending invites)
• No credential leakage
• Appropriate use of keychain for identity storage

---

🎨 Code Quality

Positive

• Consistent naming conventions (InboxActivityRepository, PendingInviteRepository)
• Good use of modern Swift (actor, async/await, @observable)
• Proper error handling with domain-specific error types
• Clear documentation on complex flows (welcome message handling)

Areas for Improvement

1. Dead Code in SessionManager.swift: The clientId(for:) private method is never used (noted in review comments)
2. Documentation: While welcome message handling is well-documented, other complex flows (LRU eviction, capacity management) could use more inline docs
3. Magic Numbers: maxAwakeInboxes: Int = 20 could use documentation on why 20 was chosen

---

✅ Test Coverage

Excellent Coverage

• InboxLifecycleManager: 542 lines covering wake/sleep, capacity, rebalance, active protection, pending invites
• SleepingInboxMessageChecker: 368 lines covering wake decisions, foreground checks, error handling
• Repositories: Dedicated test files for InboxActivityRepository (178 lines) and PendingInviteRepository (195 lines)
• SessionManager: 320 lines of integration tests

Test Quality

• Good use of mocks (MockInboxActivityRepository, MockPendingInviteRepository)
• Clear test names describing behavior
• Proper setup/teardown patterns

---

🚀 Performance Considerations

Potential Issues

1. Repeated DB Queries: pendingInviteClientIds property (InboxLifecycleManager.swift:86-88) queries the DB on every access. In sleepLeastRecentlyUsed, this happens inside a filter predicate.

   • Fix: Cache the set once per call

2. Synchronous UI Updates: ConversationsViewModel makes several main-thread dictionary accesses that could block during heavy operations

3. N+1 Query Pattern: Welcome message processing syncs all conversations, then iterates groups individually

Positive

• Efficient use of database indices (ORDER BY lastActivity)
• Proper use of background tasks for unused inbox preparation
• Lazy initialization patterns

---

📋 Recommendations

High Priority

1. Fix the race condition in ConversationsViewModel active client ID setting
2. Fix the memory leak in SleepingInboxMessageChecker periodic loop
3. Protect active inbox from LRU eviction
4. Fix MockPendingInviteRepository thread safety
5. Fix PendingInviteInfo Identifiable violation

Medium Priority

6. Cache pendingInviteClientIds to avoid repeated DB queries
7. Ensure consistent error handling in batch operations
8. Address the global mutable state in XMTPAPIOptionsBuilder
9. Clear selection state on ViewModel creation errors

Low Priority

10. Remove dead code (clientId(for:) method)
11. Add documentation for capacity limits and eviction strategy
12. Consider adding metrics for fail-open behaviors

---

📊 Summary

This PR successfully removes the max conversation limit and introduces a sophisticated inbox lifecycle management system. The architecture is sound, test coverage is excellent, and the code quality is generally high. However, there are several critical concurrency issues that need to be addressed before merging:

• Race conditions in active client ID management
• Memory leak in periodic task
• Thread safety issues in mock implementations
• Potential for active inbox eviction causing poor UX

The existing review comments from macroscopeapp are thorough and accurate—many of the issues I identified align with those findings.

Recommendation: Address the high-priority issues before merging. The medium-priority items should be tracked for follow-up.

---

Todo List

• Read repository guidelines (CLAUDE.md)
• Review core architecture changes (InboxLifecycleManager)
• Review session management changes (SessionManager)
• Review UI changes (ConversationsViewModel, ConversationsView)
• Review supporting components (repositories, utilities)
• Review test coverage
• Synthesize findings and provide feedback

---

[yewreeka]

• Remove max convos limit #301 👈 (View in Graphite)
• Start refactoring convo view model #280
• 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.

[macroscopeapp]

Remove conversation cap and route inbox lifecycle to InboxLifecycleManager with async SessionManager APIs across Conversations UI and tests

Replace max-conversations gating in Conversations views/view models; introduce InboxLifecycleManager and SleepingInboxMessageChecker; refactor SessionManager to async/throwing inbox/service access and active client handling; update notification flows to wake inbox by conversation; add repositories for inbox activity and pending invites; move XMTP options building; adjust singleton configuration patterns.

📍Where to Start

Start with lifecycle entry points in SessionManager in SessionManager.swift, then review the lifecycle protocol and actor in InboxLifecycleManager.swift.

---

📊 Macroscope summarized 88925ce. 17 files reviewed, 23 issues evaluated, 23 issues filtered, 0 comments posted. View details

[graphite-app]

Merge activity

• Jan 7, 10:20 PM UTC: yewreeka added this pull request to the Graphite merge queue.
• Jan 7, 10:20 PM UTC: The Graphite merge queue couldn't merge this PR because it had merge conflicts.
• Jan 7, 10:34 PM UTC: yewreeka added this pull request to the Graphite merge queue.
• Jan 7, 10:35 PM UTC: Merged by the Graphite merge queue.