Address PR #775 review feedback

- Remove AI-generated documentation files (REFACTORING_COMPLETE.md, plans/*.md)
- Update minimum iOS version from 16 to 17 (enables @Observable in future)
- Convert SystemMessagingService to BitchatMessage.system() factory method
  * Removes method overload ambiguity
  * Cleaner API as suggested in review
  * Deleted SystemMessagingService.swift
- Clean up deinit comments (remove unnecessary removeAll() calls)
- Remove conditional OS checks in GeohashBookmarksStore (iOS/macOS only)

All tests passing (23/23)
This commit is contained in:
jack
2025-10-09 17:09:51 +02:00
parent 1a569cfb80
commit 45fec95af2
9 changed files with 20 additions and 1326 deletions
+1 -1
View File
@@ -6,7 +6,7 @@ let package = Package(
name: "bitchat", name: "bitchat",
defaultLocalization: "en", defaultLocalization: "en",
platforms: [ platforms: [
.iOS(.v16), .iOS(.v17),
.macOS(.v13) .macOS(.v13)
], ],
products: [ products: [
-446
View File
@@ -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!
+14
View File
@@ -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 { extension Array where Element == BitchatMessage {
/// Filters out empty ones and deduplicate by ID while preserving order (from oldest to newest) /// Filters out empty ones and deduplicate by ID while preserving order (from oldest to newest)
func cleanedAndDeduped() -> [Element] { func cleanedAndDeduped() -> [Element] {
+1 -8
View File
@@ -111,22 +111,15 @@ final class NostrRelayManager: ObservableObject {
} }
deinit { deinit {
// Cancel reconnection timer // Clean up timers and active connections
reconnectionTimer?.invalidate() reconnectionTimer?.invalidate()
// Cancel all EOSE tracker timers
for (_, tracker) in eoseTrackers { for (_, tracker) in eoseTrackers {
tracker.timer?.invalidate() tracker.timer?.invalidate()
} }
// Close all WebSocket connections
for (_, task) in connections { for (_, task) in connections {
task.cancel(with: .goingAway, reason: nil) task.cancel(with: .goingAway, reason: nil)
} }
// Clear subscriptions
cancellables.removeAll() cancellables.removeAll()
SecureLogger.debug("NostrRelayManager deinitialized", category: .session) SecureLogger.debug("NostrRelayManager deinitialized", category: .session)
} }
@@ -1,9 +1,7 @@
import BitLogger import BitLogger
import Foundation import Foundation
import Combine import Combine
#if os(iOS) || os(macOS)
import CoreLocation import CoreLocation
#endif
/// Stores a user-maintained list of bookmarked geohash channels. /// Stores a user-maintained list of bookmarked geohash channels.
/// - Persistence: UserDefaults (JSON string array) /// - Persistence: UserDefaults (JSON string array)
@@ -17,10 +15,8 @@ final class GeohashBookmarksStore: ObservableObject {
private let storeKey = "locationChannel.bookmarks" private let storeKey = "locationChannel.bookmarks"
private let namesStoreKey = "locationChannel.bookmarkNames" private let namesStoreKey = "locationChannel.bookmarkNames"
private var membership: Set<String> = [] private var membership: Set<String> = []
#if os(iOS) || os(macOS)
private let geocoder = CLGeocoder() private let geocoder = CLGeocoder()
private var resolving: Set<String> = [] private var resolving: Set<String> = []
#endif
private let storage: UserDefaults private let storage: UserDefaults
@@ -30,10 +26,8 @@ final class GeohashBookmarksStore: ObservableObject {
} }
deinit { deinit {
#if os(iOS) || os(macOS)
// Cancel any pending geocoding operations // Cancel any pending geocoding operations
geocoder.cancelGeocode() geocoder.cancelGeocode()
#endif
SecureLogger.debug("GeohashBookmarksStore deinitialized", category: .session) SecureLogger.debug("GeohashBookmarksStore deinitialized", category: .session)
} }
@@ -127,7 +121,6 @@ final class GeohashBookmarksStore: ObservableObject {
let gh = Self.normalize(geohash) let gh = Self.normalize(geohash)
guard !gh.isEmpty else { return } guard !gh.isEmpty else { return }
if bookmarkNames[gh] != nil { return } if bookmarkNames[gh] != nil { return }
#if os(iOS) || os(macOS)
if resolving.contains(gh) { return } if resolving.contains(gh) { return }
resolving.insert(gh) resolving.insert(gh)
// For very coarse geohashes, sample multiple points to capture multiple admin areas // 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]) { private func resolveCompositeAdminName(geohash gh: String, points: [CLLocation]) {
var uniqueAdmins = OrderedSet<String>() var uniqueAdmins = OrderedSet<String>()
var idx = 0 var idx = 0
@@ -224,7 +215,6 @@ final class GeohashBookmarksStore: ObservableObject {
return pm.subLocality ?? pm.locality ?? pm.administrativeArea ?? pm.country return pm.subLocality ?? pm.locality ?? pm.administrativeArea ?? pm.country
} }
} }
#endif
#if DEBUG #if DEBUG
/// Testing-only reset helper /// Testing-only reset helper
@@ -28,10 +28,6 @@ final class PrivateChatManager: ObservableObject {
} }
deinit { deinit {
// Clean state on deinitialization
privateChats.removeAll()
unreadMessages.removeAll()
sentReadReceipts.removeAll()
SecureLogger.debug("PrivateChatManager deinitialized", category: .session) SecureLogger.debug("PrivateChatManager deinitialized", category: .session)
} }
@@ -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
)
}
}
+4 -9
View File
@@ -123,11 +123,6 @@ final class ChatViewModel: ObservableObject, BitchatDelegate {
// Computed property for backward compatibility // Computed property for backward compatibility
var geohashPeople: [GeoPerson] { geohashParticipantsService.geohashPeople } var geohashPeople: [GeoPerson] { geohashParticipantsService.geohashPeople }
// MARK: - System Messaging
/// Service for creating system messages
private let systemMessaging = SystemMessagingService()
// MARK: - Delivery Tracking // MARK: - Delivery Tracking
/// Service for tracking message delivery status /// 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()) { 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) messages.append(systemMessage)
} }
@@ -4131,7 +4126,7 @@ final class ChatViewModel: ObservableObject, BitchatDelegate {
/// If mesh is currently active, also append to the visible `messages`. /// If mesh is currently active, also append to the visible `messages`.
@MainActor @MainActor
private func addMeshOnlySystemMessage(_ content: String) { private func addMeshOnlySystemMessage(_ content: String) {
let systemMessage = systemMessaging.createSystemMessage(content: content) let systemMessage = BitchatMessage.system(content)
// Persist to mesh timeline // Persist to mesh timeline
meshTimeline.append(systemMessage) meshTimeline.append(systemMessage)
trimMeshTimelineIfNeeded() 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. /// Also persists the message into the active channel's backing store so it survives timeline rebinds.
@MainActor @MainActor
func addPublicSystemMessage(_ content: String) { func addPublicSystemMessage(_ content: String) {
let systemMessage = systemMessaging.createSystemMessage(content: content) let systemMessage = BitchatMessage.system(content)
// Append to current visible messages // Append to current visible messages
messages.append(systemMessage) messages.append(systemMessage)
// Persist into the backing store for the active channel to survive rebinds // Persist into the backing store for the active channel to survive rebinds
-806
View File
@@ -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?**