diff --git a/desktop/Desktop/Sources/AppState.swift b/desktop/Desktop/Sources/AppState.swift index e6bb165214..5823d7b697 100644 --- a/desktop/Desktop/Sources/AppState.swift +++ b/desktop/Desktop/Sources/AppState.swift @@ -1995,7 +1995,7 @@ class AppState: ObservableObject { NotificationCenter.default.post(name: .conversationsPageDidLoad, object: nil) } - /// Refresh conversations silently (for auto-refresh timer and app-activate). + /// Refresh conversations silently (for app-activation and Cmd+R event-driven refreshes). /// Fetches from API only, merges in-place, and only triggers @Published if data actually changed. func refreshConversations() async { // Skip if user is signed out (tokens are cleared) @@ -2057,7 +2057,6 @@ class AppState: ObservableObject { } } - // Update total count do { let count = try await APIClient.shared.getConversationsCount(includeDiscarded: false) if totalConversationsCount != count { @@ -3065,6 +3064,8 @@ extension Notification.Name { static let navigateToTasks = Notification.Name("navigateToTasks") /// Posted by keyboard shortcuts to navigate sidebar. userInfo: ["rawValue": Int] static let navigateToSidebarItem = Notification.Name("navigateToSidebarItem") + /// Posted by Cmd+R to refresh all data (conversations, chat, tasks, memories) + static let refreshAllData = Notification.Name("refreshAllData") /// Posted by the local desktop automation bridge to request semantic navigation. static let desktopAutomationNavigateRequested = Notification.Name( "desktopAutomationNavigateRequested") diff --git a/desktop/Desktop/Sources/MainWindow/CrispManager.swift b/desktop/Desktop/Sources/MainWindow/CrispManager.swift index 53149f8738..ba9c36f1dd 100644 --- a/desktop/Desktop/Sources/MainWindow/CrispManager.swift +++ b/desktop/Desktop/Sources/MainWindow/CrispManager.swift @@ -1,6 +1,7 @@ +import AppKit import Foundation -/// Polls the backend Crisp API endpoint for unread operator messages, +/// Fetches Crisp operator messages on app activation and Cmd+R, /// fires macOS notifications, and tracks unread count for the sidebar badge. @MainActor class CrispManager: ObservableObject { @@ -23,14 +24,16 @@ class CrispManager: ObservableObject { /// Timestamp of the most recent operator message we've already notified about. /// Persisted to UserDefaults so unread messages survive app restarts. /// Stored as Double because UserDefaults can't round-trip UInt64. - private var lastSeenTimestamp: UInt64 { + /// Non-`private` so `CrispManagerLifecycleTests` can assert `markAsRead()` advances it. + var lastSeenTimestamp: UInt64 { get { UInt64(UserDefaults.standard.double(forKey: "crisp_lastSeenTimestamp")) } set { UserDefaults.standard.set(Double(newValue), forKey: "crisp_lastSeenTimestamp") } } /// Track the latest operator message timestamp from any poll. /// Persisted to UserDefaults so we don't re-notify after restart. - private var latestOperatorTimestamp: UInt64 { + /// Non-`private` so `CrispManagerLifecycleTests` can seed it before `markAsRead()`. + var latestOperatorTimestamp: UInt64 { get { UInt64(UserDefaults.standard.double(forKey: "crisp_latestOperatorTimestamp")) } set { UserDefaults.standard.set(Double(newValue), forKey: "crisp_latestOperatorTimestamp") } } @@ -38,16 +41,32 @@ class CrispManager: ObservableObject { /// Track message texts we've already sent notifications for (to avoid duplicates) private var notifiedMessages = Set() - /// Polling timer - private var pollTimer: Timer? - - /// Whether polling has started - private var isStarted = false - - private init() {} - - /// Call once after sign-in to start polling for Crisp messages - func start() { + /// Whether start() has been called. Non-`private` so lifecycle tests can + /// assert idempotency after calling `start()` twice. + var isStarted = false + + /// Non-`private` so lifecycle tests can assert `stop()` clears both observers. + var activationObserver: NSObjectProtocol? + var refreshAllObserver: NSObjectProtocol? + + /// Counter bumped at the top of `pollForMessages()`, before the auth-backoff + /// guard and the network task. Lets `CrispManagerLifecycleTests` prove that + /// posting `didBecomeActive` / `.refreshAllData` actually reaches the poll + /// method — if an observer subscribes to the wrong notification name or a + /// future edit drops the wiring, the counter stays flat and the test fails. + /// Deliberately **not** `@Published` — publishing on every activation/Cmd+R + /// refresh would emit `objectWillChange` and invalidate any SwiftUI view + /// observing `CrispManager`, which is a pure production cost for a value + /// nothing drives UI from. + private(set) var pollInvocations: Int = 0 + + /// Call once after sign-in to fetch Crisp messages and listen for activation/Cmd+R. + /// + /// - Parameter performInitialPoll: If `true` (default), kicks off an immediate + /// `pollForMessages()` call that hits `APIClient.shared`. Pass `false` only + /// from lifecycle unit tests that want to exercise observer registration + /// without touching the network, auth state, or firing real notifications. + func start(performInitialPoll: Bool = true) { guard !isStarted else { return } isStarted = true @@ -58,15 +77,20 @@ class CrispManager: ObservableObject { lastSeenTimestamp = UInt64(Date().timeIntervalSince1970) } - // Poll immediately, then every 2 minutes - pollForMessages() - pollTimer = Timer.scheduledTimer(withTimeInterval: 120, repeats: true) { [weak self] _ in - Task { @MainActor in - self?.pollForMessages() - } + if performInitialPoll { + pollForMessages() } - log("CrispManager: started polling for operator messages") + // Refresh on app activation and Cmd+R (no periodic timer) + activationObserver = NotificationCenter.default.addObserver( + forName: NSApplication.didBecomeActiveNotification, object: nil, queue: .main + ) { [weak self] _ in Task { @MainActor in self?.pollForMessages() } } + + refreshAllObserver = NotificationCenter.default.addObserver( + forName: .refreshAllData, object: nil, queue: .main + ) { [weak self] _ in Task { @MainActor in self?.pollForMessages() } } + + log("CrispManager: started (event-driven, no polling timer)") } /// Mark messages as read (called when user opens Help tab) @@ -75,10 +99,12 @@ class CrispManager: ObservableObject { lastSeenTimestamp = latestOperatorTimestamp } - /// Stop polling (called on sign-out) + /// Stop observing (called on sign-out) func stop() { - pollTimer?.invalidate() - pollTimer = nil + if let obs = activationObserver { NotificationCenter.default.removeObserver(obs) } + if let obs = refreshAllObserver { NotificationCenter.default.removeObserver(obs) } + activationObserver = nil + refreshAllObserver = nil isStarted = false unreadCount = 0 // Clear persisted timestamps so next sign-in starts fresh @@ -90,6 +116,7 @@ class CrispManager: ObservableObject { // MARK: - Private private func pollForMessages() { + pollInvocations += 1 Task { // Skip if in auth backoff period (recent 401 errors) guard !AuthBackoffTracker.shared.shouldSkipRequest() else { return } diff --git a/desktop/Desktop/Sources/MainWindow/DesktopHomeView.swift b/desktop/Desktop/Sources/MainWindow/DesktopHomeView.swift index 3158011da0..2a09ec7584 100644 --- a/desktop/Desktop/Sources/MainWindow/DesktopHomeView.swift +++ b/desktop/Desktop/Sources/MainWindow/DesktopHomeView.swift @@ -32,6 +32,7 @@ struct DesktopHomeView: View { @State private var showTryAskingPopup = false @State private var previousIndexBeforeSettings: Int = 0 @State private var logoPulse = false + @State private var lastActivationRefresh = Date.distantPast // Pre-loaded hero logo to avoid NSImage init crashes during SwiftUI body evaluation private static let heroLogoImage: NSImage? = { @@ -199,7 +200,12 @@ struct DesktopHomeView: View { .onReceive( NotificationCenter.default.publisher(for: NSApplication.didBecomeActiveNotification) ) { _ in - Task { await appState.refreshConversations() } + // Cooldown: only refresh conversations if last activation was 60+ seconds ago + let now = Date() + if PollingConfig.shouldAllowActivationRefresh(now: now, lastRefresh: lastActivationRefresh) { + lastActivationRefresh = now + Task { await appState.refreshConversations() } + } // Auto-start monitoring when returning to app if screen analysis is enabled // but monitoring is not running. Handles the case where the user granted // screen recording permission in System Settings and switched back. @@ -234,8 +240,8 @@ struct DesktopHomeView: View { } } } - // Periodic refresh every 30s to pick up conversations from other devices (e.g. Omi Glass) - .onReceive(Timer.publish(every: 30, on: .main, in: .common).autoconnect()) { _ in + // Cmd+R: refresh all data (conversations, chat, tasks, memories) + .onReceive(NotificationCenter.default.publisher(for: .refreshAllData)) { _ in Task { await appState.refreshConversations() } } // On sign-out: reset @AppStorage-backed onboarding flag and stop transcription. diff --git a/desktop/Desktop/Sources/MainWindow/Pages/MemoriesPage.swift b/desktop/Desktop/Sources/MainWindow/Pages/MemoriesPage.swift index db3e10b27b..f661fcd661 100644 --- a/desktop/Desktop/Sources/MainWindow/Pages/MemoriesPage.swift +++ b/desktop/Desktop/Sources/MainWindow/Pages/MemoriesPage.swift @@ -139,6 +139,17 @@ class MemoriesViewModel: ObservableObject { /// Memories loaded from SQLite with filters applied @Published private(set) var filteredFromDatabase: [ServerMemory] = [] @Published private(set) var isLoadingFiltered = false + + /// Counter bumped at the top of `refreshMemoriesIfNeeded()`, before any of + /// the early-exit guards. Lets `MemoriesViewModelObserverTests` prove that + /// posting `didBecomeActive` / `.refreshAllData` actually reaches the refresh + /// method — if the observer rewire regresses, the counter stays flat and the + /// test fails. + /// Deliberately **not** `@Published` — publishing on every activation/Cmd+R + /// refresh would emit `objectWillChange` and invalidate any SwiftUI view + /// observing `MemoriesViewModel`, which is a pure production cost for a + /// value nothing drives UI from. + private(set) var refreshInvocations: Int = 0 @Published var showingAddMemory = false @Published var newMemoryText = "" @Published var editingMemory: ServerMemory? = nil @@ -206,9 +217,15 @@ class MemoriesViewModel: ObservableObject { // MARK: - Initialization init() { - // Auto-refresh memories every 30 seconds - Timer.publish(every: 30.0, on: .main, in: .common) - .autoconnect() + // Refresh memories when app becomes active + NotificationCenter.default.publisher(for: NSApplication.didBecomeActiveNotification) + .sink { [weak self] _ in + Task { await self?.refreshMemoriesIfNeeded() } + } + .store(in: &cancellables) + + // Cmd+R: refresh memories on demand + NotificationCenter.default.publisher(for: .refreshAllData) .sink { [weak self] _ in Task { await self?.refreshMemoriesIfNeeded() } } @@ -217,6 +234,7 @@ class MemoriesViewModel: ObservableObject { /// Refresh memories if already loaded (for auto-refresh) private func refreshMemoriesIfNeeded() async { + refreshInvocations += 1 // Skip if user is signed out (tokens are cleared) guard AuthState.shared.isSignedIn else { return } // Skip if in auth backoff period (recent 401 errors) diff --git a/desktop/Desktop/Sources/OmiApp.swift b/desktop/Desktop/Sources/OmiApp.swift index e20eaad665..5fda969184 100644 --- a/desktop/Desktop/Sources/OmiApp.swift +++ b/desktop/Desktop/Sources/OmiApp.swift @@ -191,6 +191,13 @@ struct OMIApp: App { } .keyboardShortcut(",", modifiers: .command) } + + CommandGroup(after: .toolbar) { + Button("Refresh") { + NotificationCenter.default.post(name: .refreshAllData, object: nil) + } + .keyboardShortcut("r", modifiers: .command) + } } // Note: Menu bar is now handled by NSStatusBar in AppDelegate.setupMenuBar() diff --git a/desktop/Desktop/Sources/PollingConfig.swift b/desktop/Desktop/Sources/PollingConfig.swift new file mode 100644 index 0000000000..63903f8d6d --- /dev/null +++ b/desktop/Desktop/Sources/PollingConfig.swift @@ -0,0 +1,18 @@ +import Foundation + +/// Centralized configuration for event-driven data refresh. +/// All periodic polling timers have been removed — data refreshes on +/// app activation (didBecomeActiveNotification) and manual Cmd+R. +enum PollingConfig { + /// Minimum time between app-activation conversation refreshes (seconds). + /// Prevents cmd-tab spam from flooding the API. + static let activationCooldown: TimeInterval = 60.0 + + /// Returns `true` when enough time has passed since `lastRefresh` to allow + /// another activation-triggered refresh. Used by DesktopHomeView to throttle + /// didBecomeActiveNotification bursts. Shared between production and tests + /// so a regression (e.g. `>=` → `>`) is caught by the unit tests. + static func shouldAllowActivationRefresh(now: Date = Date(), lastRefresh: Date) -> Bool { + now.timeIntervalSince(lastRefresh) >= activationCooldown + } +} diff --git a/desktop/Desktop/Sources/Providers/ChatProvider.swift b/desktop/Desktop/Sources/Providers/ChatProvider.swift index 032d98a0c6..002717996e 100644 --- a/desktop/Desktop/Sources/Providers/ChatProvider.swift +++ b/desktop/Desktop/Sources/Providers/ChatProvider.swift @@ -537,12 +537,9 @@ A screenshot may be attached — use it silently only if relevant. Never mention private var multiChatObserver: AnyCancellable? private var playwrightExtensionObserver: AnyCancellable? private var sessionGroupingObserver: AnyCancellable? + private var activationObserver: AnyCancellable? - // MARK: - Cross-Platform Message Polling - /// Polls for new messages from other platforms (mobile) every 15 seconds. - /// Similar to TasksStore's 30-second polling pattern. - private var messagePollTimer: AnyCancellable? - private static let messagePollInterval: TimeInterval = 15.0 + private var refreshAllObserver: AnyCancellable? // MARK: - Streaming Buffer /// Accumulates text deltas during streaming and flushes them to the published @@ -638,9 +635,16 @@ A screenshot may be attached — use it silently only if relevant. Never mention } } - // Poll for new messages from other platforms (mobile) every 15 seconds - messagePollTimer = Timer.publish(every: Self.messagePollInterval, on: .main, in: .common) - .autoconnect() + // Refresh messages when app becomes active + activationObserver = NotificationCenter.default.publisher(for: NSApplication.didBecomeActiveNotification) + .sink { [weak self] _ in + Task { @MainActor in + await self?.pollForNewMessages() + } + } + + // Cmd+R: refresh messages on demand + refreshAllObserver = NotificationCenter.default.publisher(for: .refreshAllData) .sink { [weak self] _ in Task { @MainActor in await self?.pollForNewMessages() @@ -1920,11 +1924,17 @@ A screenshot may be attached — use it silently only if relevant. Never mention isLoading = false } - // MARK: - Cross-Platform Message Polling + // MARK: - Cross-Platform Message Sync + + /// Prevents overlapping fetches when activation + Cmd+R fire back-to-back. + private let pollGate = ReentrancyGate() - /// Poll for new messages from other platforms (e.g. mobile). + /// Fetch new messages from other platforms (e.g. mobile). /// Merges new messages into the existing array without disrupting the UI. private func pollForNewMessages() async { + // Prevent overlapping fetches from activation + Cmd+R firing together + guard pollGate.tryEnter() else { return } + defer { pollGate.exit() } // Skip if user is signed out (tokens are cleared) guard AuthState.shared.isSignedIn else { return } // Skip if in auth backoff period (recent 401 errors) diff --git a/desktop/Desktop/Sources/ReentrancyGate.swift b/desktop/Desktop/Sources/ReentrancyGate.swift new file mode 100644 index 0000000000..58955f4cac --- /dev/null +++ b/desktop/Desktop/Sources/ReentrancyGate.swift @@ -0,0 +1,45 @@ +import Foundation + +/// Single-entry reentrancy gate for preventing overlapping async operations. +/// +/// Use when two independent triggers (e.g. `didBecomeActive` + Cmd+R) can fire +/// back-to-back and would otherwise cause duplicate fetches/inserts. Call +/// `tryEnter()` at the start of the critical section — if it returns `false`, +/// another caller is already in-flight and the current caller should bail out +/// **without** calling `exit()`. Only the caller that got `true` from +/// `tryEnter()` owns the gate and must release it. +/// +/// The canonical usage is a `guard` + `defer` pair, which ensures `exit()` is +/// only scheduled once the guard has admitted the caller: +/// +/// ```swift +/// guard gate.tryEnter() else { return } // non-owners return here, no exit() +/// defer { gate.exit() } // only the owner reaches this line +/// // … critical section … +/// ``` +/// +/// `exit()` does not validate ownership — a stray call will reopen the gate +/// while another caller is still inside the critical section. Follow the +/// `guard`/`defer` pattern above and the contract holds. +/// +/// Tested in `ReentrancyGateTests`. +@MainActor +final class ReentrancyGate { + private var isInFlight = false + + /// Attempts to enter the critical section. + /// - Returns: `true` if the caller acquired the gate (must call `exit()` when done), + /// `false` if another operation is already in-flight (caller must **not** call `exit()`). + func tryEnter() -> Bool { + guard !isInFlight else { return false } + isInFlight = true + return true + } + + /// Releases the gate. **Caller contract:** only call this after a matching + /// `tryEnter()` returned `true`. Calling `exit()` without ownership will + /// reopen the gate while another caller is still inside the critical section. + func exit() { + isInFlight = false + } +} diff --git a/desktop/Desktop/Sources/Stores/TasksStore.swift b/desktop/Desktop/Sources/Stores/TasksStore.swift index 1a2c7bba42..2c4f748a67 100644 --- a/desktop/Desktop/Sources/Stores/TasksStore.swift +++ b/desktop/Desktop/Sources/Stores/TasksStore.swift @@ -28,6 +28,17 @@ class TasksStore: ObservableObject { @Published var hasMoreDeletedTasks = true @Published var error: String? + /// Counter bumped at the top of `refreshTasksIfNeeded()`, before any of the + /// early-exit guards. Lets `TasksStoreObserverTests` prove that posting + /// `didBecomeActive` / `.refreshAllData` actually reaches the refresh method + /// — if the observer rewire regresses (wrong notification name, dropped + /// subscription), the counter stays flat and the test fails. + /// Deliberately **not** `@Published` — publishing on every activation/Cmd+R + /// refresh would emit `objectWillChange` and invalidate SwiftUI views + /// observing `TasksStore`, which is a pure production cost for a value + /// nothing drives UI from. + private(set) var refreshInvocations: Int = 0 + // Legacy compatibility - combines both lists var tasks: [TaskActionItem] { incompleteTasks + completedTasks @@ -161,9 +172,15 @@ class TasksStore: ObservableObject { // MARK: - Initialization private init() { - // Auto-refresh tasks every 30 seconds - Timer.publish(every: 30.0, on: .main, in: .common) - .autoconnect() + // Refresh tasks when app becomes active + NotificationCenter.default.publisher(for: NSApplication.didBecomeActiveNotification) + .sink { [weak self] _ in + Task { await self?.refreshTasksIfNeeded() } + } + .store(in: &cancellables) + + // Cmd+R: refresh tasks on demand + NotificationCenter.default.publisher(for: .refreshAllData) .sink { [weak self] _ in Task { await self?.refreshTasksIfNeeded() } } @@ -175,6 +192,7 @@ class TasksStore: ObservableObject { /// Uses local-first pattern: sync API to cache, then reload from cache /// Merges changes in-place to avoid wholesale array replacement (which kills SwiftUI gestures) private func refreshTasksIfNeeded() async { + refreshInvocations += 1 // Skip if not signed in guard AuthService.shared.isSignedIn else { return } // Skip if in auth backoff period (recent 401 errors) diff --git a/desktop/Desktop/Tests/CrispManagerLifecycleTests.swift b/desktop/Desktop/Tests/CrispManagerLifecycleTests.swift new file mode 100644 index 0000000000..9a0e729d14 --- /dev/null +++ b/desktop/Desktop/Tests/CrispManagerLifecycleTests.swift @@ -0,0 +1,165 @@ +import AppKit +import XCTest +@testable import Omi_Computer + +/// Tests for `CrispManager` event-driven lifecycle (#6500). +/// +/// After removing the 120s polling timer, `CrispManager` relies entirely on +/// `start()`/`stop()` registering/unregistering `didBecomeActive` + `.refreshAllData` +/// observers. These tests cover the highest-risk branch: the observer lifecycle +/// and `markAsRead()` timestamp advancement. +@MainActor +final class CrispManagerLifecycleTests: XCTestCase { + + // Save and restore the UserDefaults-backed timestamps so each test runs + // against a known state without clobbering real app data. + private var savedLastSeen: Double = 0 + private var savedLatestOperator: Double = 0 + + override func setUp() async throws { + try await super.setUp() + savedLastSeen = UserDefaults.standard.double(forKey: "crisp_lastSeenTimestamp") + savedLatestOperator = UserDefaults.standard.double(forKey: "crisp_latestOperatorTimestamp") + // Reset the singleton to a clean state — previous tests may have called start() + CrispManager.shared.stop() + } + + override func tearDown() async throws { + CrispManager.shared.stop() + UserDefaults.standard.set(savedLastSeen, forKey: "crisp_lastSeenTimestamp") + UserDefaults.standard.set(savedLatestOperator, forKey: "crisp_latestOperatorTimestamp") + try await super.tearDown() + } + + func testStartIsIdempotent() { + let manager = CrispManager.shared + XCTAssertFalse(manager.isStarted, "Manager must be stopped after setUp()") + + manager.start(performInitialPoll: false) + XCTAssertTrue(manager.isStarted, "start() must set isStarted true") + let firstActivationObs = manager.activationObserver + let firstRefreshObs = manager.refreshAllObserver + XCTAssertNotNil(firstActivationObs, "start() must register activation observer") + XCTAssertNotNil(firstRefreshObs, "start() must register refreshAllData observer") + + // Second start() call must be a no-op — observers must NOT be replaced. + // A new token would mean we leaked the first registration. + manager.start(performInitialPoll: false) + XCTAssertTrue(manager.isStarted) + XCTAssertTrue( + manager.activationObserver === firstActivationObs as AnyObject, + "Second start() must not replace the activation observer (leak guard)" + ) + XCTAssertTrue( + manager.refreshAllObserver === firstRefreshObs as AnyObject, + "Second start() must not replace the refreshAllData observer (leak guard)" + ) + } + + func testStopRemovesBothObservers() { + let manager = CrispManager.shared + manager.start(performInitialPoll: false) + XCTAssertNotNil(manager.activationObserver) + XCTAssertNotNil(manager.refreshAllObserver) + XCTAssertTrue(manager.isStarted) + + manager.stop() + XCTAssertNil(manager.activationObserver, "stop() must nil the activation observer") + XCTAssertNil(manager.refreshAllObserver, "stop() must nil the refreshAllData observer") + XCTAssertFalse(manager.isStarted, "stop() must clear isStarted so start() can run again") + + // After stop(), a subsequent start() must succeed (observer lifecycle reusable). + manager.start(performInitialPoll: false) + XCTAssertTrue(manager.isStarted, "start() after stop() must re-register observers") + XCTAssertNotNil(manager.activationObserver) + XCTAssertNotNil(manager.refreshAllObserver) + } + + func testStopIsIdempotent() { + let manager = CrispManager.shared + manager.start(performInitialPoll: false) + manager.stop() + // Second stop() must not crash or change state + manager.stop() + XCTAssertFalse(manager.isStarted) + XCTAssertNil(manager.activationObserver) + XCTAssertNil(manager.refreshAllObserver) + } + + func testMarkAsReadAdvancesPersistedTimestamp() { + let manager = CrispManager.shared + manager.latestOperatorTimestamp = 999_999 + manager.lastSeenTimestamp = 111_111 + + manager.markAsRead() + + XCTAssertEqual( + manager.lastSeenTimestamp, 999_999, + "markAsRead() must advance lastSeenTimestamp to latestOperatorTimestamp" + ) + XCTAssertEqual(manager.unreadCount, 0, "markAsRead() must clear unreadCount") + } + + func testMarkAsReadIsSafeWhenNoNewMessages() { + let manager = CrispManager.shared + manager.latestOperatorTimestamp = 0 + manager.lastSeenTimestamp = 0 + + manager.markAsRead() + + XCTAssertEqual(manager.lastSeenTimestamp, 0) + XCTAssertEqual(manager.unreadCount, 0) + } + + func testDidBecomeActiveNotificationTriggersPoll() async { + let manager = CrispManager.shared + manager.start(performInitialPoll: false) + let baseline = manager.pollInvocations + + NotificationCenter.default.post( + name: NSApplication.didBecomeActiveNotification, object: nil + ) + // Observer posts on main queue; yield so the block runs. + await Task.yield() + try? await Task.sleep(nanoseconds: 50_000_000) + + XCTAssertEqual( + manager.pollInvocations, baseline + 1, + "didBecomeActive must route to pollForMessages() via the activation observer" + ) + } + + func testRefreshAllDataNotificationTriggersPoll() async { + let manager = CrispManager.shared + manager.start(performInitialPoll: false) + let baseline = manager.pollInvocations + + NotificationCenter.default.post(name: .refreshAllData, object: nil) + await Task.yield() + try? await Task.sleep(nanoseconds: 50_000_000) + + XCTAssertEqual( + manager.pollInvocations, baseline + 1, + ".refreshAllData (Cmd+R) must route to pollForMessages() via the refresh observer" + ) + } + + func testStoppedManagerDoesNotRespondToNotifications() async { + let manager = CrispManager.shared + manager.start(performInitialPoll: false) + manager.stop() + let baseline = manager.pollInvocations + + NotificationCenter.default.post( + name: NSApplication.didBecomeActiveNotification, object: nil + ) + NotificationCenter.default.post(name: .refreshAllData, object: nil) + await Task.yield() + try? await Task.sleep(nanoseconds: 50_000_000) + + XCTAssertEqual( + manager.pollInvocations, baseline, + "After stop(), neither notification should reach pollForMessages()" + ) + } +} diff --git a/desktop/Desktop/Tests/MemoriesViewModelObserverTests.swift b/desktop/Desktop/Tests/MemoriesViewModelObserverTests.swift new file mode 100644 index 0000000000..eca08ec458 --- /dev/null +++ b/desktop/Desktop/Tests/MemoriesViewModelObserverTests.swift @@ -0,0 +1,62 @@ +import AppKit +import XCTest +@testable import Omi_Computer + +/// Tests for `MemoriesViewModel` auto-refresh observer wiring (#6500). +/// +/// After replacing the 30-second `Timer.publish` with `didBecomeActive` + +/// `.refreshAllData` subscribers, the view model must still refresh when +/// those notifications fire. Because `MemoriesViewModel` is not a singleton, +/// each test constructs a fresh instance (triggering `init()` which registers +/// the two subscribers into its private `cancellables`) and asserts that +/// posting each notification advances `refreshInvocations` by one. +@MainActor +final class MemoriesViewModelObserverTests: XCTestCase { + + func testDidBecomeActiveNotificationTriggersRefresh() async { + let viewModel = MemoriesViewModel() + XCTAssertEqual(viewModel.refreshInvocations, 0, "Fresh instance must start at zero") + + NotificationCenter.default.post( + name: NSApplication.didBecomeActiveNotification, object: nil + ) + await Task.yield() + try? await Task.sleep(nanoseconds: 50_000_000) + + XCTAssertEqual( + viewModel.refreshInvocations, 1, + "didBecomeActive must route to refreshMemoriesIfNeeded() via the activation subscriber" + ) + } + + func testRefreshAllDataNotificationTriggersRefresh() async { + let viewModel = MemoriesViewModel() + + NotificationCenter.default.post(name: .refreshAllData, object: nil) + await Task.yield() + try? await Task.sleep(nanoseconds: 50_000_000) + + XCTAssertEqual( + viewModel.refreshInvocations, 1, + ".refreshAllData (Cmd+R) must route to refreshMemoriesIfNeeded() via the refresh subscriber" + ) + } + + func testDeallocatedViewModelDoesNotLeakObservers() async { + // Ensures the `[weak self]` capture in the Combine sinks lets the + // view model deallocate cleanly — no crash when the notifications + // fire after the instance is gone. + do { + let viewModel = MemoriesViewModel() + XCTAssertEqual(viewModel.refreshInvocations, 0) + } + // viewModel is out of scope and should be deallocated. + NotificationCenter.default.post( + name: NSApplication.didBecomeActiveNotification, object: nil + ) + NotificationCenter.default.post(name: .refreshAllData, object: nil) + await Task.yield() + try? await Task.sleep(nanoseconds: 50_000_000) + // If the weak capture misbehaved we'd crash above; reaching here is the assertion. + } +} diff --git a/desktop/Desktop/Tests/PollingFrequencyTests.swift b/desktop/Desktop/Tests/PollingFrequencyTests.swift new file mode 100644 index 0000000000..7f67005537 --- /dev/null +++ b/desktop/Desktop/Tests/PollingFrequencyTests.swift @@ -0,0 +1,126 @@ +import XCTest +@testable import Omi_Computer + +/// Tests for event-driven refresh architecture (#6500). +/// All polling timers are removed — refreshes happen on app activation +/// (didBecomeActiveNotification) and manual Cmd+R (.refreshAllData). +/// +/// Tests call `PollingConfig.shouldAllowActivationRefresh(now:lastRefresh:)`, +/// which is the same function used by DesktopHomeView's activation handler. +/// A regression in that predicate (e.g. `>=` → `>`) is caught here. +final class PollingFrequencyTests: XCTestCase { + + // MARK: - No Polling Timers + + func testPollingConfigHasNoPollIntervals() { + // PollingConfig only exposes activationCooldown + the shared predicate. + // If someone reintroduces a poll interval constant, update this test. + XCTAssertEqual(PollingConfig.activationCooldown, 60.0) + } + + // MARK: - Activation Cooldown (shared predicate) + + func testFirstActivationAlwaysAllowed() { + let now = Date() + XCTAssertTrue( + PollingConfig.shouldAllowActivationRefresh(now: now, lastRefresh: .distantPast), + "First activation (distantPast) must always be allowed" + ) + } + + func testActivationWithinCooldownIsBlocked() { + let lastRefresh = Date() + let now = lastRefresh.addingTimeInterval(PollingConfig.activationCooldown - 0.001) + XCTAssertFalse( + PollingConfig.shouldAllowActivationRefresh(now: now, lastRefresh: lastRefresh), + "Activation just under cooldown must be blocked" + ) + } + + func testActivationAtExactCooldownBoundaryIsAllowed() { + let lastRefresh = Date() + let now = lastRefresh.addingTimeInterval(PollingConfig.activationCooldown) + // Production uses >= — boundary must be inclusive. Guards against >= → > regressions. + XCTAssertTrue( + PollingConfig.shouldAllowActivationRefresh(now: now, lastRefresh: lastRefresh), + "Activation at exactly cooldown boundary must be allowed (>= comparison)" + ) + } + + func testActivationAfterCooldownIsAllowed() { + let lastRefresh = Date() + let now = lastRefresh.addingTimeInterval(PollingConfig.activationCooldown + 30) + XCTAssertTrue( + PollingConfig.shouldAllowActivationRefresh(now: now, lastRefresh: lastRefresh), + "Activation 30s past cooldown must be allowed" + ) + } + + func testBackwardClockSkewIsAllowed() { + // If system clock jumps backward, elapsed is negative < cooldown. + // Predicate returns false (blocked). This documents current behavior — + // a user with clock skew just won't get an auto-refresh that cycle. + let lastRefresh = Date() + let now = lastRefresh.addingTimeInterval(-10) + XCTAssertFalse( + PollingConfig.shouldAllowActivationRefresh(now: now, lastRefresh: lastRefresh), + "Negative elapsed (backward clock skew) must be treated as within cooldown" + ) + } + + func testRapidActivationsAreThrottled() { + // Simulate cmd-tab spam: 10 activations 1 second apart. + // Driven by the same predicate as production. + let start = Date() + var lastAllowed = Date.distantPast + var allowedCount = 0 + for i in 0..<10 { + let activation = start.addingTimeInterval(Double(i)) + if PollingConfig.shouldAllowActivationRefresh(now: activation, lastRefresh: lastAllowed) { + allowedCount += 1 + lastAllowed = activation + } + } + XCTAssertEqual(allowedCount, 1, "Rapid activations (1s apart) should only allow 1 refresh") + } + + func testCooldownResetsAfterExpiry() { + // Sequence [allowed, blocked, allowed] — the third activation is past cooldown. + let start = Date() + var lastAllowed = Date.distantPast + var results: [Bool] = [] + + let activations = [ + start, // t=0: allowed (first) + start.addingTimeInterval(30), // t=30: blocked + start.addingTimeInterval(PollingConfig.activationCooldown + 1) // t=61: allowed + ] + + for activation in activations { + let allowed = PollingConfig.shouldAllowActivationRefresh( + now: activation, lastRefresh: lastAllowed + ) + results.append(allowed) + if allowed { lastAllowed = activation } + } + + XCTAssertEqual(results, [true, false, true]) + } + + // MARK: - Refresh All Notification + + func testRefreshAllDataNotificationNameExists() { + XCTAssertEqual(Notification.Name.refreshAllData.rawValue, "refreshAllData") + } + + func testRefreshAllDataNotificationIsReceivable() { + let expectation = XCTestExpectation(description: "Notification received") + let observer = NotificationCenter.default.addObserver( + forName: .refreshAllData, object: nil, queue: .main + ) { _ in expectation.fulfill() } + defer { NotificationCenter.default.removeObserver(observer) } + + NotificationCenter.default.post(name: .refreshAllData, object: nil) + wait(for: [expectation], timeout: 1.0) + } +} diff --git a/desktop/Desktop/Tests/ReentrancyGateTests.swift b/desktop/Desktop/Tests/ReentrancyGateTests.swift new file mode 100644 index 0000000000..f2a62fc4da --- /dev/null +++ b/desktop/Desktop/Tests/ReentrancyGateTests.swift @@ -0,0 +1,113 @@ +import XCTest +@testable import Omi_Computer + +/// Tests for `ReentrancyGate`, the single-entry gate that prevents overlapping +/// `ChatProvider.pollForNewMessages()` fetches when `didBecomeActive` and +/// `.refreshAllData` fire back-to-back. +@MainActor +final class ReentrancyGateTests: XCTestCase { + + func testFirstEnterSucceeds() { + let gate = ReentrancyGate() + XCTAssertTrue(gate.tryEnter(), "First enter on a fresh gate must succeed") + } + + func testSecondEnterWithoutExitIsBlocked() { + let gate = ReentrancyGate() + XCTAssertTrue(gate.tryEnter()) + XCTAssertFalse( + gate.tryEnter(), + "Second enter without matching exit must be blocked (in-flight)" + ) + } + + func testEnterAfterExitSucceeds() { + let gate = ReentrancyGate() + XCTAssertTrue(gate.tryEnter()) + gate.exit() + XCTAssertTrue( + gate.tryEnter(), + "Enter after exit must succeed — gate should be reopened" + ) + } + + func testRepeatedEnterExitCyclesAllSucceed() { + // Simulates activation + Cmd+R firing sequentially (not overlapping) — + // every cycle should complete cleanly. + let gate = ReentrancyGate() + for cycle in 0..<5 { + XCTAssertTrue( + gate.tryEnter(), + "Cycle \(cycle): enter should succeed after prior exit" + ) + gate.exit() + } + } + + func testOverlappingTriggersResultInOneEntry() { + // Simulates the exact race `ChatProvider.pollGate` guards against: + // activation + Cmd+R fire while a fetch is in flight — only one caller + // may enter, the rest must bail out until the in-flight caller exits. + let gate = ReentrancyGate() + var enteredCount = 0 + + // Caller A starts the fetch + if gate.tryEnter() { enteredCount += 1 } + // Caller B (overlapping) tries while A is still in flight + if gate.tryEnter() { enteredCount += 1 } + // Caller C (overlapping) tries while A is still in flight + if gate.tryEnter() { enteredCount += 1 } + + XCTAssertEqual(enteredCount, 1, "Only one of 3 overlapping callers may enter") + + // Caller A completes + gate.exit() + + // Caller D arrives after A exited — should be allowed + XCTAssertTrue(gate.tryEnter(), "New caller after exit must be allowed") + gate.exit() + } + + func testGuardDeferPatternNonOwnerDoesNotCallExit() { + // Models the canonical ChatProvider.pollForNewMessages() usage: + // guard gate.tryEnter() else { return } + // defer { gate.exit() } + // + // Regression guard: if a future edit swapped `defer` above `guard`, a + // non-owning caller would still fire `exit()` and reopen the gate while + // the owner was still inside the critical section. This test forces an + // overlap — caller A holds the gate, caller B invokes the critical + // section while A is still in-flight — and asserts B neither enters + // nor reopens the gate. + let gate = ReentrancyGate() + var exitCalls = 0 + + func criticalSection() { + guard gate.tryEnter() else { return } + defer { + gate.exit() + exitCalls += 1 + } + } + + // Caller A (the test itself) acquires the gate directly. + XCTAssertTrue(gate.tryEnter(), "Precondition: caller A must acquire the gate") + + // Caller B invokes the critical section while A is still in-flight. + // Under guard/defer, B's guard short-circuits and no `defer` is registered, + // so exit() must not fire. + criticalSection() + XCTAssertEqual(exitCalls, 0, "Non-owner caller B must not register an exit") + + // Gate must still be owned by A — B must not have reopened it. + XCTAssertFalse( + gate.tryEnter(), + "Gate must still be held by A; a regressed guard/defer order would have reopened it" + ) + + // A releases; caller C can now run through the critical section normally. + gate.exit() + criticalSection() + XCTAssertEqual(exitCalls, 1, "Owner caller C must register exactly one exit") + } +} diff --git a/desktop/Desktop/Tests/TasksStoreObserverTests.swift b/desktop/Desktop/Tests/TasksStoreObserverTests.swift new file mode 100644 index 0000000000..9d68ce22f6 --- /dev/null +++ b/desktop/Desktop/Tests/TasksStoreObserverTests.swift @@ -0,0 +1,64 @@ +import AppKit +import XCTest +@testable import Omi_Computer + +/// Tests for `TasksStore` auto-refresh observer wiring (#6500). +/// +/// After replacing the 30-second `Timer.publish` with `didBecomeActive` + +/// `.refreshAllData` subscribers, the store must still refresh when those +/// notifications fire. These tests post each notification and assert the +/// baseline-diffed `refreshInvocations` counter advances by one, proving the +/// observer reaches `refreshTasksIfNeeded()` — the early-exit guards inside +/// that method run after the counter bumps, so tests don't need local auth +/// or any prior `loadTasks()` call. +@MainActor +final class TasksStoreObserverTests: XCTestCase { + + func testDidBecomeActiveNotificationTriggersRefresh() async { + let store = TasksStore.shared + let baseline = store.refreshInvocations + + NotificationCenter.default.post( + name: NSApplication.didBecomeActiveNotification, object: nil + ) + // Sink runs on the main queue; yield so the task enqueues and fires. + await Task.yield() + try? await Task.sleep(nanoseconds: 50_000_000) + + XCTAssertEqual( + store.refreshInvocations, baseline + 1, + "didBecomeActive must route to refreshTasksIfNeeded() via the activation observer" + ) + } + + func testRefreshAllDataNotificationTriggersRefresh() async { + let store = TasksStore.shared + let baseline = store.refreshInvocations + + NotificationCenter.default.post(name: .refreshAllData, object: nil) + await Task.yield() + try? await Task.sleep(nanoseconds: 50_000_000) + + XCTAssertEqual( + store.refreshInvocations, baseline + 1, + ".refreshAllData (Cmd+R) must route to refreshTasksIfNeeded() via the refresh observer" + ) + } + + func testBothNotificationsTriggerIndependentRefreshes() async { + let store = TasksStore.shared + let baseline = store.refreshInvocations + + NotificationCenter.default.post( + name: NSApplication.didBecomeActiveNotification, object: nil + ) + NotificationCenter.default.post(name: .refreshAllData, object: nil) + await Task.yield() + try? await Task.sleep(nanoseconds: 100_000_000) + + XCTAssertEqual( + store.refreshInvocations, baseline + 2, + "Both observers must fire independently — posting both notifications yields two refresh calls" + ) + } +}