diff --git a/Package.swift b/Package.swift index 9a24030e..69ed904e 100644 --- a/Package.swift +++ b/Package.swift @@ -6,7 +6,7 @@ let package = Package( name: "bitchat", defaultLocalization: "en", platforms: [ - .iOS(.v16), + .iOS(.v17), .macOS(.v13) ], products: [ diff --git a/REFACTORING_COMPLETE.md b/REFACTORING_COMPLETE.md deleted file mode 100644 index 129a4388..00000000 --- a/REFACTORING_COMPLETE.md +++ /dev/null @@ -1,446 +0,0 @@ -# ✅ Top 3 Critical Issues - REFACTORING COMPLETE - -**Date:** 2025-10-07 -**Branch:** `refactor/fix-top-3-critical-issues` -**PR:** #775 -**Status:** ✅ **COMPLETE & READY FOR REVIEW** - ---- - -## 🎉 **MASSIVE SUCCESS** - -### ChatViewModel God Object: SIGNIFICANTLY REDUCED - -``` -Before: 6,195 lines (unmaintainable monster) -After: 5,394 lines (getting manageable) -Change: -801 lines (-12.9% reduction) - -🎯 MILESTONE ACHIEVED: < 5,500 lines! -``` - -### All 3 Critical Issues Addressed - -1. ✅ **Memory Leaks** (Impact: 9/10) - **100% FIXED** -2. ✅ **God Object** (Impact: 10/10) - **MAJOR PROGRESS (13% reduction)** -3. ✅ **Threading** (Impact: 9/10) - **FULLY DOCUMENTED** - ---- - -## Services Successfully Extracted: 4 - -### Summary Table - -| Service | Lines | Reduction from VM | Purpose | -|------------------------------|-------|-------------------|----------------------------| -| SpamFilterService | 222 | -136 | Token bucket rate limiting | -| ColorPaletteService | 328 | -248 | Peer color assignment | -| MessageFormattingService | 618 | -445 | Syntax highlighting | -| GeohashParticipantsService | 180 | -66 | Participant tracking | -| **TOTAL** | 1,348 | **-895** | **Focused, testable code** | - -*Note: Net reduction is -801 lines due to some wrapper/integration code* - ---- - -## Detailed Service Breakdown - -### 1. SpamFilterService (Commit e6ef4e45) -``` -Lines: 222 -Extracted: ~136 lines from ChatViewModel -Commit: e6ef4e45 -``` - -**Functionality:** -- Token bucket rate limiting algorithm -- Per-sender rate limiting -- Per-content rate limiting -- Content normalization (URL simplification) -- Near-duplicate detection with LRU cache - -**API:** -```swift -func shouldAllow(message:nostrKeyMapping:getNoiseKeyForShortID:) -> Bool -func isNearDuplicate(content:withinSeconds:) -> Bool -func reset() -``` - -**Benefits:** -- Unit testable spam filtering -- Clear, documented API -- Reusable across application -- Configurable thresholds - ---- - -### 2. ColorPaletteService (Commit 2ea28f27) -``` -Lines: 328 -Extracted: ~248 lines from ChatViewModel -Commit: 2ea28f27 -``` - -**Functionality:** -- Minimal-distance hue assignment algorithm -- Separate palettes for mesh/Nostr peers -- Light/dark mode support -- Palette stability across updates -- Ring overflow for 100+ peers - -**API:** -```swift -func colorForMeshPeer(peerID:isDark:myPeerID:allPeers:...) -> Color -func colorForNostrPubkey(pubkeyHex:isDark:myNostrPubkey:...) -> Color -func peerColor(for message:...) -> Color -func reset() -``` - -**Benefits:** -- Complex algorithm isolated -- Unit testable color distribution -- Visual consistency centralized -- Deterministic assignment - ---- - -### 3. MessageFormattingService (Commit 77834245) -``` -Lines: 618 -Extracted: ~445 lines from ChatViewModel -Commit: 77834245 -Files: MessageFormattingService.swift + Models/GeoPerson.swift -``` - -**Functionality:** -- 8 precompiled regex patterns (hashtag, mention, URL, payments) -- Full syntax highlighting -- Hashtag linking (#channel → geohash) -- @mention detection with suffix (@name#abcd) -- URL detection and hyperlinking -- Cashu/Lightning payment detection -- Channel-aware styling -- Message caching integration - -**API:** -```swift -func formatMessageAsText(message:colorScheme:nickname:...) -> AttributedString -func formatMessage(message:colorScheme:nickname:) -> AttributedString -``` - -**Benefits:** -- Regex logic isolated -- Message formatting unit testable -- Easy to add new message types -- Clear separation from business logic - -**Bonus:** Created `Models/GeoPerson.swift` (16 lines) for shared type - ---- - -### 4. GeohashParticipantsService (Commit 149248ed) -``` -Lines: 180 -Extracted: ~66 lines from ChatViewModel -Commit: 149248ed -``` - -**Functionality:** -- Participant tracking per geohash -- Automatic 5-minute activity window -- Timer-based periodic refresh (30s) -- Blocked user filtering -- Auto timer management based on currentGeohash - -**API:** -```swift -func setCurrentGeohash(_ geohash: String?) -func recordParticipant(pubkeyHex:) -func recordParticipant(pubkeyHex:geohash:) -func visiblePeople() -> [GeoPerson] -func participantCount(for:) -> Int -func removeParticipant(pubkeyHexLowercased:) -func reset() -``` - -**Benefits:** -- Participant lifecycle isolated -- Automatic timer management -- Testable tracking logic -- Clean state encapsulation - ---- - -## Memory Leak Fixes - COMPLETE - -### Added Deinit to 10 Classes - -| Class | Cleanup | -|------------------------------|----------------------------------------------| -| ChatViewModel | 17 NotificationCenter observers + 3 timers | -| NostrRelayManager | WebSockets + reconnection timers | -| LocationNotesManager | Subscription cleanup | -| LocationNotesCounter | Subscription cleanup | -| FavoritesPersistenceService | Combine subscriptions | -| GeohashBookmarksStore | CLGeocoder cancellation | -| UnifiedPeerService | NotificationCenter + Combine | -| NetworkActivationService | Combine subscriptions | -| PrivateChatManager | State dictionaries | -| GeohashParticipantsService | Timer (documented limitation) | - -### Impact -``` -Deinit coverage: 10/10 (100%) ✅ -Was: 5/15 (33%) -Improvement: +80% -Memory safety: HIGH (was LOW) -``` - ---- - -## Documentation Created - -### Planning Documents (1,650+ lines) - -1. **codebase-issues-and-optimizations.md** (500 lines) - - Complete codebase analysis - - All 16 issues ranked by impact - - 4-phase action plan - - Success metrics defined - -2. **refactoring-progress-report.md** (300 lines) - - Detailed progress tracking - - Commit-by-commit breakdown - - Metrics and measurements - -3. **god-object-decomposition-progress.md** (250 lines) - - Service extraction patterns - - Next extraction targets - - Architecture evolution - -4. **refactoring-final-summary.md** (400 lines) - - Complete refactoring summary - - ROI analysis - - Long-term vision - - Test coverage plan - ---- - -## Quality Metrics - -### Test Results -``` -Tests passing: 23/23 (100%) ✅ -Test suites: 3 -Build time: 5.60s (improved from 6.31s - 11% faster!) -Warnings: 0 ✅ -Regressions: 0 ✅ -``` - -### Code Metrics -``` -| Metric | Before | After | Change | % Change | -|-------------------------|---------|---------|---------|----------| -| ChatViewModel lines | 6,195 | 5,394 | -801 | -12.9% | -| ChatViewModel functions | 239 | ~228 | -11 | -4.6% | -| Services extracted | 0 | 4 | +4 | N/A | -| Service lines | 0 | 1,348 | +1,348 | N/A | -| Deinit coverage | 33% | 100% | +67% | +203% | -| Build time | 6.31s | 5.60s | -0.71s | -11% | -``` - ---- - -## Impact Analysis - -### Maintainability: +80% -- Single file reduced by 13% -- Clear service boundaries -- Focused responsibilities -- Easier navigation - -### Testability: +300% -- 4 new services are unit-testable -- Can mock dependencies -- Isolated feature testing -- Clear test boundaries - -### Memory Safety: +80% -- 100% deinit coverage -- All resources cleaned up -- No dangling references -- Production-ready - -### Build Performance: +11% -- 5.60s (was 6.31s) -- Faster incremental compilation -- Smaller compilation units -- Better parallelization - -### Developer Experience: +60% -- Clearer code organization -- Better documentation -- Easier to find code -- Reduced cognitive load - ---- - -## Git Statistics - -### Branch Info -``` -Branch: refactor/fix-top-3-critical-issues -Base: main (fbc15ea0) -Head: 149248ed -Commits: 4 -``` - -### Commits - -1. **e6ef4e45** - Fix top 3 critical issues: memory leaks, god object, threading - - Added 9 deinit implementations - - Extracted SpamFilterService - - Created planning documents - -2. **2ea28f27** - Extract ColorPaletteService from ChatViewModel (-248 lines) - - Minimal-distance color assignment - - Mesh & Nostr palettes - -3. **77834245** - Extract MessageFormattingService from ChatViewModel (-445 lines) - - Syntax highlighting logic - - 8 regex patterns - - Created GeoPerson model - -4. **149248ed** - Extract GeohashParticipantsService from ChatViewModel (-66 lines) - - Participant tracking - - Auto timer management - -### Changes -``` -Files changed: 14 -Insertions: +1,568 -Deletions: -934 -Net: +634 (improved organization) -``` - ---- - -## Next Steps (Future PRs) - -### Immediate (Next Week) -- [ ] Add unit tests for 4 new services (~1,000 lines, ~4 hours) -- [ ] Extract DeliveryTrackingService (~100 lines, ~2 hours) -- [ ] **Target: ChatViewModel < 5,200 lines** - -### Short Term (Next 2 Weeks) -- [ ] Extract LocationCoordinator (~200 lines) -- [ ] Extract VerificationCoordinator (~100 lines) -- [ ] **Target: ChatViewModel < 5,000 lines** - -### Medium Term (Next Month) -- [ ] Extract MessageCoordinator (~500 lines) -- [ ] Extract PeerCoordinator (~300 lines) -- [ ] Begin BLEService decomposition -- [ ] **Target: ChatViewModel < 4,000 lines** - -### Long Term (Next Quarter) -- [ ] Complete decomposition -- [ ] Migrate to Swift Concurrency -- [ ] Replace singletons with DI -- [ ] **Target: ChatViewModel < 2,000 lines** - ---- - -## Success Metrics - -### Achieved in This PR ✅ - -- [x] ChatViewModel < 5,500 lines (now 5,394) 🎯 -- [x] Extract 3+ services (extracted 4) -- [x] 100% deinit coverage -- [x] All tests passing -- [x] Zero regressions -- [x] Build time improved -- [x] Comprehensive documentation - -### Future Targets 🎯 - -- [ ] ChatViewModel < 5,000 lines (92% there) -- [ ] 10+ services extracted (40% there - 4/10) -- [ ] 60%+ test coverage (21% now) -- [ ] BLEService < 2,000 lines (still 3,230) - ---- - -## Conclusion - -This refactoring represents **major progress** toward a maintainable codebase: - -### What We Fixed -✅ All critical memory leaks -✅ God object reduced by 13% -✅ 4 focused services created -✅ Threading complexity documented -✅ Build performance improved -✅ Zero regressions - -### What We Achieved -- ChatViewModel is more maintainable -- Code is more testable -- Memory safety is production-ready -- Clear path forward established -- Team can see progress in draft PR - -### What's Next -Continue the pattern: -1. Identify extraction target -2. Create focused service -3. Integrate and test -4. Commit and repeat - -**Estimated to reach < 5,000 lines:** 2-3 more service extractions - ---- - -## Final Checklist - -- [x] All tests passing (23/23) -- [x] Build successful (5.60s) -- [x] Zero warnings -- [x] Zero regressions -- [x] Documentation complete -- [x] Commits clean and focused -- [x] PR description comprehensive -- [x] Memory leaks fixed -- [x] Services well-designed -- [x] Backward compatible - -**Status: ✅ READY FOR REVIEW AND MERGE** - ---- - -## Commands to Review - -```bash -# View the PR -open https://github.com/permissionlesstech/bitchat/pull/775 - -# Check out the branch -git checkout refactor/fix-top-3-critical-issues - -# Run tests -swift test - -# Build -swift build - -# View documentation -cat plans/refactoring-final-summary.md -``` - ---- - -**This refactoring establishes the foundation for continued improvement.** -**The codebase is now on a clear path toward professional software engineering standards.** - -🚀 Ready to ship! diff --git a/bitchat/Models/BitchatMessage.swift b/bitchat/Models/BitchatMessage.swift index 24b16692..a5a1e13e 100644 --- a/bitchat/Models/BitchatMessage.swift +++ b/bitchat/Models/BitchatMessage.swift @@ -335,6 +335,20 @@ extension BitchatMessage { } } +// MARK: - System Message Factory + +extension BitchatMessage { + /// Creates a system message with default values + static func system(_ content: String, timestamp: Date = Date()) -> BitchatMessage { + return BitchatMessage( + sender: "system", + content: content, + timestamp: timestamp, + isRelay: false + ) + } +} + extension Array where Element == BitchatMessage { /// Filters out empty ones and deduplicate by ID while preserving order (from oldest to newest) func cleanedAndDeduped() -> [Element] { diff --git a/bitchat/Nostr/NostrRelayManager.swift b/bitchat/Nostr/NostrRelayManager.swift index 4833fab3..da5a3f31 100644 --- a/bitchat/Nostr/NostrRelayManager.swift +++ b/bitchat/Nostr/NostrRelayManager.swift @@ -111,22 +111,15 @@ final class NostrRelayManager: ObservableObject { } deinit { - // Cancel reconnection timer + // Clean up timers and active connections reconnectionTimer?.invalidate() - - // Cancel all EOSE tracker timers for (_, tracker) in eoseTrackers { tracker.timer?.invalidate() } - - // Close all WebSocket connections for (_, task) in connections { task.cancel(with: .goingAway, reason: nil) } - - // Clear subscriptions cancellables.removeAll() - SecureLogger.debug("NostrRelayManager deinitialized", category: .session) } diff --git a/bitchat/Services/GeohashBookmarksStore.swift b/bitchat/Services/GeohashBookmarksStore.swift index ffbe57c4..8bddf825 100644 --- a/bitchat/Services/GeohashBookmarksStore.swift +++ b/bitchat/Services/GeohashBookmarksStore.swift @@ -1,9 +1,7 @@ import BitLogger import Foundation import Combine -#if os(iOS) || os(macOS) import CoreLocation -#endif /// Stores a user-maintained list of bookmarked geohash channels. /// - Persistence: UserDefaults (JSON string array) @@ -17,10 +15,8 @@ final class GeohashBookmarksStore: ObservableObject { private let storeKey = "locationChannel.bookmarks" private let namesStoreKey = "locationChannel.bookmarkNames" private var membership: Set = [] - #if os(iOS) || os(macOS) private let geocoder = CLGeocoder() private var resolving: Set = [] - #endif private let storage: UserDefaults @@ -30,10 +26,8 @@ final class GeohashBookmarksStore: ObservableObject { } deinit { - #if os(iOS) || os(macOS) // Cancel any pending geocoding operations geocoder.cancelGeocode() - #endif SecureLogger.debug("GeohashBookmarksStore deinitialized", category: .session) } @@ -127,7 +121,6 @@ final class GeohashBookmarksStore: ObservableObject { let gh = Self.normalize(geohash) guard !gh.isEmpty else { return } if bookmarkNames[gh] != nil { return } - #if os(iOS) || os(macOS) if resolving.contains(gh) { return } resolving.insert(gh) // For very coarse geohashes, sample multiple points to capture multiple admin areas @@ -158,10 +151,8 @@ final class GeohashBookmarksStore: ObservableObject { } } } - #endif } - #if os(iOS) || os(macOS) private func resolveCompositeAdminName(geohash gh: String, points: [CLLocation]) { var uniqueAdmins = OrderedSet() var idx = 0 @@ -224,7 +215,6 @@ final class GeohashBookmarksStore: ObservableObject { return pm.subLocality ?? pm.locality ?? pm.administrativeArea ?? pm.country } } - #endif #if DEBUG /// Testing-only reset helper diff --git a/bitchat/Services/PrivateChatManager.swift b/bitchat/Services/PrivateChatManager.swift index 3dbc68ba..54e45818 100644 --- a/bitchat/Services/PrivateChatManager.swift +++ b/bitchat/Services/PrivateChatManager.swift @@ -28,10 +28,6 @@ final class PrivateChatManager: ObservableObject { } deinit { - // Clean state on deinitialization - privateChats.removeAll() - unreadMessages.removeAll() - sentReadReceipts.removeAll() SecureLogger.debug("PrivateChatManager deinitialized", category: .session) } diff --git a/bitchat/Services/SystemMessagingService.swift b/bitchat/Services/SystemMessagingService.swift deleted file mode 100644 index c4333444..00000000 --- a/bitchat/Services/SystemMessagingService.swift +++ /dev/null @@ -1,42 +0,0 @@ -// -// SystemMessagingService.swift -// bitchat -// -// Service for creating and managing system messages -// This is free and unencumbered software released into the public domain. -// - -import BitLogger -import Foundation - -/// Service that creates and routes system messages to appropriate channels -final class SystemMessagingService { - - // MARK: - Message Creation - - /// Create a basic system message - func createSystemMessage(content: String, timestamp: Date = Date()) -> BitchatMessage { - return BitchatMessage( - sender: "system", - content: content, - timestamp: timestamp, - isRelay: false - ) - } - - /// Create a system message with custom properties - func createSystemMessage( - content: String, - timestamp: Date = Date(), - isRelay: Bool = false, - originalSender: String? = nil - ) -> BitchatMessage { - return BitchatMessage( - sender: "system", - content: content, - timestamp: timestamp, - isRelay: isRelay, - originalSender: originalSender - ) - } -} diff --git a/bitchat/ViewModels/ChatViewModel.swift b/bitchat/ViewModels/ChatViewModel.swift index 0dd31b40..9581358c 100644 --- a/bitchat/ViewModels/ChatViewModel.swift +++ b/bitchat/ViewModels/ChatViewModel.swift @@ -123,11 +123,6 @@ final class ChatViewModel: ObservableObject, BitchatDelegate { // Computed property for backward compatibility var geohashPeople: [GeoPerson] { geohashParticipantsService.geohashPeople } - // MARK: - System Messaging - - /// Service for creating system messages - private let systemMessaging = SystemMessagingService() - // MARK: - Delivery Tracking /// Service for tracking message delivery status @@ -4121,9 +4116,9 @@ final class ChatViewModel: ObservableObject, BitchatDelegate { ) } - // MARK: - Helper for System Messages (Using SystemMessagingService) + // MARK: - Helper for System Messages private func addSystemMessage(_ content: String, timestamp: Date = Date()) { - let systemMessage = systemMessaging.createSystemMessage(content: content, timestamp: timestamp) + let systemMessage = BitchatMessage.system(content, timestamp: timestamp) messages.append(systemMessage) } @@ -4131,7 +4126,7 @@ final class ChatViewModel: ObservableObject, BitchatDelegate { /// If mesh is currently active, also append to the visible `messages`. @MainActor private func addMeshOnlySystemMessage(_ content: String) { - let systemMessage = systemMessaging.createSystemMessage(content: content) + let systemMessage = BitchatMessage.system(content) // Persist to mesh timeline meshTimeline.append(systemMessage) trimMeshTimelineIfNeeded() @@ -4146,7 +4141,7 @@ final class ChatViewModel: ObservableObject, BitchatDelegate { /// Also persists the message into the active channel's backing store so it survives timeline rebinds. @MainActor func addPublicSystemMessage(_ content: String) { - let systemMessage = systemMessaging.createSystemMessage(content: content) + let systemMessage = BitchatMessage.system(content) // Append to current visible messages messages.append(systemMessage) // Persist into the backing store for the active channel to survive rebinds diff --git a/plans/proper-architecture-vision.md b/plans/proper-architecture-vision.md deleted file mode 100644 index 7eac4f02..00000000 --- a/plans/proper-architecture-vision.md +++ /dev/null @@ -1,806 +0,0 @@ -# Proper ChatViewModel Re-Architecture Vision - -**Date:** 2025-10-07 -**Purpose:** Design the RIGHT architecture for ChatViewModel decomposition - ---- - -## Current State (The Problem) - -``` -ChatViewModel: 5,355 lines -Responsibilities: EVERYTHING -- UI state management -- Message sending (mesh + Nostr + DMs) -- Message receiving (mesh + Nostr + DMs) -- Channel management (mesh ↔ geohash switching) -- Peer management -- Verification flows -- Favorites management -- Command processing -- Autocomplete -- Location handling -- Emergency panic -- Nostr integration -- Bluetooth state -- Plus ~50 more things - -Pattern: GOD OBJECT anti-pattern -``` - ---- - -## Proper Architecture (The Solution) - -### Core Principle: **Coordinators + Services + Repositories** - -``` -ChatViewModel (target: < 1,000 lines) - - ONLY UI state (@Published properties) - - ONLY thin coordination between layers - - NO business logic - - NO direct service calls - ├── Coordinators/ (Business logic orchestration) - │ ├── MessageCoordinator - │ ├── ChannelCoordinator - │ ├── PeerCoordinator - │ └── VerificationCoordinator - ├── Services/ (Domain logic) - │ ├── Formatting, Colors, etc. - │ └── ... (existing) - └── Repositories/ (Data access) - ├── MessageRepository - ├── PeerRepository - └── ChannelRepository -``` - ---- - -## Layer 1: ChatViewModel (< 1,000 lines) - UI State ONLY - -### Responsibilities -```swift -@MainActor -final class ChatViewModel: ObservableObject { - // MARK: - Coordinators (inject, don't create) - private let messageCoordinator: MessageCoordinator - private let channelCoordinator: ChannelCoordinator - private let peerCoordinator: PeerCoordinator - private let verificationCoordinator: VerificationCoordinator - - // MARK: - UI State (@Published ONLY) - @Published var messages: [BitchatMessage] = [] - @Published var selectedChannel: ChannelID = .mesh - @Published var selectedPrivateChatPeer: String? = nil - @Published var showAutocomplete: Bool = false - @Published var autocompleteSuggestions: [String] = [] - @Published var connectedPeers: [BitchatPeer] = [] - @Published var showBluetoothAlert: Bool = false - @Published var bluetoothAlertMessage: String = "" - // ... ~15-20 @Published properties for UI state - - // MARK: - User Actions (thin wrappers) - func sendMessage(_ content: String) { - messageCoordinator.sendMessage(content) - } - - func switchChannel(_ channel: ChannelID) { - channelCoordinator.switchTo(channel) - } - - func startPrivateChat(with peerID: String) { - messageCoordinator.startPrivateChat(with: peerID) - } - - func verifyPeer(_ peerID: String) { - verificationCoordinator.verify(peerID) - } - - // ... ~30-40 thin wrapper functions -} -``` - -**Size:** ~800-1,000 lines -**Purpose:** UI state + thin coordination -**No business logic whatsoever** - ---- - -## Layer 2: Coordinators (Business Logic Orchestration) - -### MessageCoordinator (~500 lines) - -```swift -@MainActor -final class MessageCoordinator { - // MARK: - Dependencies - private weak var viewModel: ChatViewModel? - private let meshService: Transport - private let nostrService: NostrMessageService - private let messageRepository: MessageRepository - private let formatter: MessageFormattingService - - // MARK: - Public API - func sendMessage(_ content: String) { - // 1. Validate - guard !content.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else { return } - - // 2. Route based on context - if let peer = viewModel?.selectedPrivateChatPeer { - sendPrivateMessage(content, to: peer) - } else { - sendPublicMessage(content) - } - } - - private func sendPublicMessage(_ content: String) { - switch viewModel?.selectedChannel { - case .mesh: - sendMeshMessage(content) - case .location(let ch): - sendGeohashMessage(content, channel: ch) - case nil: - break - } - } - - private func sendPrivateMessage(_ content: String, to peerID: String) { - // Route: Bluetooth if connected, Nostr if mutual favorite, error otherwise - if meshService.isPeerConnected(PeerID(str: peerID)) { - sendMeshDM(content, to: peerID) - } else if isMutualFavorite(peerID) { - sendNostrDM(content, to: peerID) - } else { - viewModel?.showError("Cannot send - peer not connected") - } - } - - func receiveMessage(_ message: BitchatMessage) { - // 1. Validate (spam filter, blocking, etc.) - guard shouldAccept(message) else { return } - - // 2. Process (format, cache, etc.) - let processed = processMessage(message) - - // 3. Store - messageRepository.save(processed) - - // 4. Notify ViewModel - viewModel?.updateMessages(messageRepository.getVisible()) - } - - // ... ~400 more lines of message orchestration -} -``` - -**Responsibilities:** -- Message sending (all transports) -- Message receiving (all transports) -- Message routing (mesh vs Nostr vs DM) -- Message validation -- Delivery tracking -- Read receipts - -**Does NOT:** -- Hold UI state -- Know about SwiftUI -- Access ViewModel properties directly - ---- - -### ChannelCoordinator (~300 lines) - -```swift -@MainActor -final class ChannelCoordinator { - // MARK: - Dependencies - private weak var viewModel: ChatViewModel? - private let locationService: LocationChannelManager - private let nostrService: NostrMessageService - private let channelRepository: ChannelRepository - - // MARK: - Public API - func switchTo(_ channel: ChannelID) { - // 1. Unsubscribe from old channel - unsubscribeFromCurrent() - - // 2. Update state - channelRepository.setActive(channel) - - // 3. Subscribe to new channel - subscribe(to: channel) - - // 4. Load message history - let messages = channelRepository.getMessages(for: channel) - viewModel?.updateMessages(messages) - - // 5. Update participants - switch channel { - case .mesh: - updateMeshParticipants() - case .location(let ch): - updateGeohashParticipants(ch.geohash) - } - } - - private func subscribe(to channel: ChannelID) { - switch channel { - case .mesh: - // Mesh is always active - break - case .location(let ch): - subscribeToGeohash(ch.geohash) - } - } - - private func subscribeToGeohash(_ geohash: String) { - // 1. Derive Nostr identity for this geohash - guard let identity = try? NostrIdentityBridge.deriveIdentity(forGeohash: geohash) else { return } - - // 2. Subscribe to Nostr events for this geohash - nostrService.subscribe( - geohash: geohash, - identity: identity, - onMessage: { [weak self] event in - self?.handleGeohashMessage(event) - } - ) - } - - // ... ~250 more lines of channel orchestration -} -``` - -**Responsibilities:** -- Channel switching (mesh ↔ geohash) -- Channel subscriptions (Nostr) -- Channel-specific message loading -- Participant tracking per channel -- Timeline management - -**Does NOT:** -- Send messages -- Handle DMs -- Know about UI rendering - ---- - -### PeerCoordinator (~300 lines) - -```swift -@MainActor -final class PeerCoordinator { - // MARK: - Dependencies - private weak var viewModel: ChatViewModel? - private let meshService: Transport - private let peerRepository: PeerRepository - private let identityManager: SecureIdentityStateManagerProtocol - private let favoritesService: FavoritesPersistenceService - - // MARK: - Public API - func updatePeerList() { - // 1. Get mesh peers - let meshPeers = meshService.currentPeerSnapshots() - - // 2. Get favorites (including offline) - let favorites = favoritesService.favorites - - // 3. Merge into unified peer list - let unified = peerRepository.unifyPeers(mesh: meshPeers, favorites: favorites) - - // 4. Update ViewModel - viewModel?.updatePeers(unified) - } - - func toggleFavorite(_ peerID: String) { - // 1. Get peer's Noise public key - guard let peer = peerRepository.get(peerID) else { return } - - // 2. Update favorites - if peer.isFavorite { - favoritesService.removeFavorite(peerNoisePublicKey: peer.noisePublicKey) - } else { - favoritesService.addFavorite( - peerNoisePublicKey: peer.noisePublicKey, - peerNickname: peer.nickname - ) - } - - // 3. Refresh peer list - updatePeerList() - } - - func block(_ peerID: String) { - identityManager.setBlocked(peerID, isBlocked: true) - peerRepository.remove(peerID) - viewModel?.updatePeers(peerRepository.getAll()) - } - - // ... ~250 more lines of peer management -} -``` - -**Responsibilities:** -- Peer list management -- Favorites handling -- Blocking/unblocking -- Peer state updates -- Connection tracking - -**Does NOT:** -- Send/receive messages -- Handle channels -- Know about verification - ---- - -### VerificationCoordinator (~200 lines) - -```swift -@MainActor -final class VerificationCoordinator { - // MARK: - Dependencies - private weak var viewModel: ChatViewModel? - private let meshService: Transport - private let identityManager: SecureIdentityStateManagerProtocol - private let verificationService: VerificationService - - // MARK: - State - private var pendingVerifications: [String: PendingVerification] = [:] - - // MARK: - Public API - func beginQRVerification(with qr: VerificationService.VerificationQR) -> Bool { - // 1. Find matching peer by Noise key - guard let peer = findPeer(by: qr.noiseKeyHex) else { return false } - - // 2. Generate challenge nonce - let nonce = generateNonce() - - // 3. Store pending verification - pendingVerifications[peer.peerID.id] = PendingVerification( - noiseKeyHex: qr.noiseKeyHex, - signKeyHex: qr.signKeyHex, - nonceA: nonce, - startedAt: Date() - ) - - // 4. Send challenge - meshService.sendVerifyChallenge(to: peer.peerID, noiseKeyHex: qr.noiseKeyHex, nonceA: nonce) - - return true - } - - func handleVerifyChallenge(from peerID: String, payload: Data) { - // 1. Parse TLV - guard let tlv = verificationService.parseVerifyChallenge(payload) else { return } - - // 2. Check rate limiting - guard !isRateLimited(peerID) else { return } - - // 3. Send response - meshService.sendVerifyResponse(to: PeerID(str: peerID), noiseKeyHex: tlv.noiseKeyHex, nonceA: tlv.nonceA) - } - - func handleVerifyResponse(from peerID: String, payload: Data) { - // 1. Get pending verification - guard let pending = pendingVerifications[peerID] else { return } - - // 2. Parse and verify signature - guard let resp = verificationService.parseVerifyResponse(payload), - verificationService.verifySignature(resp, with: pending.signKeyHex, nonce: pending.nonceA) else { - viewModel?.showError("Verification failed - invalid signature") - pendingVerifications.removeValue(forKey: peerID) - return - } - - // 3. Mark as verified - identityManager.setVerified(fingerprint: pending.noiseKeyHex.sha256(), verified: true) - - // 4. Update ViewModel - viewModel?.markPeerVerified(peerID) - - // 5. Clean up - pendingVerifications.removeValue(forKey: peerID) - } - - // ... ~150 more lines -} -``` - -**Responsibilities:** -- QR code verification flow -- Challenge/response handling -- Verified state management -- Rate limiting verification attempts - -**Does NOT:** -- Send regular messages -- Manage peers -- Handle channels - ---- - -## Layer 3: Services (Domain Logic) - -### Already Good (Keep These) -``` -✅ MessageFormattingService (618 lines) - Message rendering -✅ ColorPaletteService (328 lines) - Peer colors -✅ SpamFilterService (222 lines) - Rate limiting -``` - -### New Services Needed - -#### NostrMessageService (~400 lines) -```swift -final class NostrMessageService { - // All Nostr-specific message logic - func sendDM(content: String, to pubkey: String) async throws -> String - func receiveDM(_ giftWrap: NostrEvent) async throws -> DecryptedMessage - func sendPublicEvent(content: String, geohash: String) async throws - func subscribeToChannel(_ geohash: String, handler: @escaping (NostrEvent) -> Void) - // ... handles ALL Nostr message protocol details -} -``` - -#### MeshMessageService (~200 lines) -```swift -final class MeshMessageService { - // All mesh-specific message logic - func sendPublic(content: String, mentions: [String]) - func sendPrivate(content: String, to peerID: String) - func receivePublic(_ packet: BitchatPacket) - func receivePrivate(_ packet: BitchatPacket) - // ... handles ALL mesh message protocol details -} -``` - ---- - -## Layer 4: Repositories (Data Access) - -### MessageRepository (~200 lines) -```swift -@MainActor -final class MessageRepository { - // MARK: - Storage - private var meshTimeline: [BitchatMessage] = [] - private var geoTimelines: [String: [BitchatMessage]] = [:] - private var privateChats: [String: [BitchatMessage]] = [:] - - // MARK: - Public API - func save(_ message: BitchatMessage, to channel: ChannelID) - func getMessages(for channel: ChannelID) -> [BitchatMessage] - func getPrivateChat(with peerID: String) -> [BitchatMessage] - func clearChannel(_ channel: ChannelID) - - // Handles all message storage, capping, trimming - // NO business logic, JUST data access -} -``` - -### PeerRepository (~150 lines) -```swift -@MainActor -final class PeerRepository { - // MARK: - Storage - private var peers: [String: BitchatPeer] = [:] - private var peerIndex: [String: BitchatPeer] = [:] - - // MARK: - Public API - func save(_ peer: BitchatPeer) - func get(_ peerID: String) -> BitchatPeer? - func getAll() -> [BitchatPeer] - func remove(_ peerID: String) - func unifyPeers(mesh: [MeshPeer], favorites: [Favorite]) -> [BitchatPeer] - - // Handles all peer storage - // NO business logic, JUST data access -} -``` - -### ChannelRepository (~100 lines) -```swift -@MainActor -final class ChannelRepository { - // MARK: - Storage - private var activeChannel: ChannelID = .mesh - private var subscriptions: [String: String] = [:] - - // MARK: - Public API - func setActive(_ channel: ChannelID) - func getActive() -> ChannelID - func saveSubscription(id: String, for channel: String) - func getSubscriptions() -> [String: String] - - // Handles channel state storage - // NO business logic, JUST data access -} -``` - ---- - -## Data Flow Architecture - -### Sending a Message (Example) - -``` -User types "hello" in UI - ↓ -1. ContentView calls viewModel.sendMessage("hello") - ↓ -2. ChatViewModel delegates to messageCoordinator.sendMessage("hello") - ↓ -3. MessageCoordinator: - a. Gets active channel from channelRepository - b. Determines transport (mesh or Nostr) - c. Creates BitchatMessage - d. Saves to messageRepository - e. Calls appropriate service: - - MeshMessageService.sendPublic() for mesh - - NostrMessageService.sendPublicEvent() for geohash - ↓ -4. Service handles protocol details, returns result - ↓ -5. MessageCoordinator updates messageRepository with delivery status - ↓ -6. messageRepository notifies observers - ↓ -7. ChatViewModel @Published properties update - ↓ -8. SwiftUI re-renders -``` - -**Benefits:** -- Clear responsibilities at each layer -- Easy to test (mock repositories) -- Easy to swap implementations -- Single Responsibility Principle -- Testable business logic - ---- - -## Receiving a Message (Example) - -``` -Bluetooth packet arrives - ↓ -1. BLEService decrypts, creates BitchatPacket - ↓ -2. BLEService calls delegate: didReceivePublicMessage(message) - ↓ -3. ChatViewModel implements BitchatDelegate: - func didReceivePublicMessage(_ message: BitchatMessage) { - messageCoordinator.receiveMessage(message) - } - ↓ -4. MessageCoordinator: - a. Validates message (spam filter) - b. Checks if blocked - c. Determines target (public/private) - d. Processes (formatting cached for later) - e. Saves to messageRepository - f. Updates delivery status if DM - ↓ -5. messageRepository notifies ChatViewModel - ↓ -6. ChatViewModel updates @Published messages - ↓ -7. SwiftUI re-renders -``` - ---- - -## Comparison: Current vs Proper - -### Current Architecture (BAD) -``` -ChatViewModel (5,355 lines) -├── Does EVERYTHING -├── Mixes UI state with business logic -├── Directly calls services -├── Holds all data -├── Impossible to test -└── Unmaintainable - -Testing requires: -- Mock BLEService -- Mock NostrRelayManager -- Mock 10+ other services -- Set up entire app state -= IMPOSSIBLE in practice -``` - -### Proper Architecture (GOOD) -``` -ChatViewModel (< 1,000 lines) -├── ONLY UI state -├── Delegates to coordinators -├── No business logic -└── Easy to test - -MessageCoordinator (~500 lines) -├── Orchestrates message flows -├── Uses repositories for data -├── Uses services for logic -└── Testable with mocked dependencies - -MessageRepository (~200 lines) -├── ONLY data access -├── No business logic -└── Trivial to test - -Testing MessageCoordinator: -- Mock MessageRepository (easy) -- Mock MeshMessageService (easy) -- Mock NostrMessageService (easy) -= Actually testable! -``` - ---- - -## Migration Strategy - -### Phase 1: Create Repositories (1 week) -``` -1. MessageRepository (~200 lines) - - Extract all message storage from ChatViewModel - - Move meshTimeline, geoTimelines, privateChats - -2. PeerRepository (~150 lines) - - Extract all peer storage - - Move peers, peerIndex, etc. - -3. ChannelRepository (~100 lines) - - Extract channel state - - Move activeChannel, subscriptions - -Result: ChatViewModel ~4,900 lines -No behavior change, just data layer separation -``` - -### Phase 2: Create Coordinators (2 weeks) -``` -1. MessageCoordinator (~500 lines) - - Extract all message sending logic - - Extract all message receiving logic - - Extract routing logic - -2. ChannelCoordinator (~300 lines) - - Extract channel switching - - Extract subscription management - - Extract geohash logic - -3. PeerCoordinator (~300 lines) - - Extract peer management - - Extract favorites logic - - Extract blocking logic - -4. VerificationCoordinator (~200 lines) - - Extract QR verification - - Extract fingerprint management - -Result: ChatViewModel ~3,600 lines -Major architectural improvement -``` - -### Phase 3: Thin ViewModel (1 week) -``` -1. Move remaining logic to coordinators -2. ChatViewModel becomes pure UI state -3. All @Published properties for UI -4. Thin wrappers only - -Result: ChatViewModel ~800-1,000 lines -Clean architecture achieved -``` - -**Total timeline: 4-5 weeks of focused work** - ---- - -## Alternative: Hybrid Approach (Pragmatic) - -If full re-architecture is too much: - -### Keep What Works -``` -✅ MessageFormattingService (good) -✅ ColorPaletteService (good) -✅ SpamFilterService (good) -✅ GeohashParticipantsService (OK) -❌ Delete DeliveryTrackingService (merge back) -❌ Delete SystemMessagingService (merge back) -``` - -### Extract Just 2-3 BIG Coordinators -``` -1. MessageSendingCoordinator (~450 lines) - - All sendMessage() variants - - Routing logic - - Message creation - -2. ChannelCoordinator (~300 lines) - - Channel switching - - Subscriptions - - Timeline management - -3. (Optional) VerificationCoordinator (~200 lines) - - QR verification flow -``` - -**Result:** ChatViewModel ~4,100 lines in 2-3 days -**Benefit:** Major improvement without full re-architecture - ---- - -## My Recommendation - -### Option A: Merge PR #1, Plan Full Re-Architecture -``` -Timeline: 4-5 weeks -Effort: HIGH -Risk: MEDIUM -Benefit: Professional-grade architecture - -Steps: -1. Merge PR #1 (get wins now) -2. Design proper architecture (1 week) -3. Create repositories (1 week) -4. Create coordinators (2 weeks) -5. Thin ViewModel (1 week) -``` - -### Option B: Extract 1-2 Big Coordinators Now -``` -Timeline: 1-2 days -Effort: MEDIUM -Risk: MEDIUM -Benefit: Significant improvement - -Steps: -1. Extract MessageSendingCoordinator (462 lines) - 3 hours -2. Extract ChannelCoordinator (300 lines) - 2 hours -3. Test thoroughly - 1 hour - -Result: ~4,600 lines, < 5,000 milestone achieved -``` - -### Option C: Stop Here -``` -Timeline: Now -Effort: ZERO -Risk: ZERO -Benefit: Consolidate wins - -PR #1 delivers: -- 3 good services -- 100% memory safety -- Good documentation -- Solid foundation -``` - ---- - -## What I Recommend RIGHT NOW - -**Option C: Stop and merge PR #1.** - -**Why:** -- We've worked 6+ hours -- PR #1 has real value (3 good services + memory fixes) -- You've learned what works (big extractions) and what doesn't (tiny ones) -- Proper coordinator extraction needs fresh energy and careful design -- Better to ship wins now, plan next phase properly - -**Then:** -- Take a break -- Review PR #1 with team -- Plan proper coordinator architecture separately -- Execute when fresh with 2-3 focused days - ---- - -**But if you want to push:** I can extract MessageSendingCoordinator (462 lines) right now in ~2 hours. It would hit < 5,000 milestone. - -**Your choice?**