diff --git a/MonitorLizard/Constants.swift b/MonitorLizard/Constants.swift index f9d2feb..086c6fc 100644 --- a/MonitorLizard/Constants.swift +++ b/MonitorLizard/Constants.swift @@ -33,5 +33,6 @@ enum Constants { static let settingsWindowHeight = 500.0 // Voice announcement - static let defaultVoiceAnnouncementText = "Build ready for Q A" + static let defaultVoiceAnnouncementTextBuildComplete = "Build ready for Q A" + static let defaultVoiceAnnouncementTextPRUpdated = "PR updated" } diff --git a/MonitorLizard/DependencyRegistration.swift b/MonitorLizard/DependencyRegistration.swift index 0a4acf4..1bb6f3b 100644 --- a/MonitorLizard/DependencyRegistration.swift +++ b/MonitorLizard/DependencyRegistration.swift @@ -221,6 +221,11 @@ private struct UnimplementedWatchlistService: WatchlistServicing { return [] } + func checkForUpdates(currentPRs: [PullRequest]) -> [PullRequest] { + reportIssue("Unimplemented: WatchlistServicing.checkForUpdates called without a test override") + return [] + } + func getWatchedStatus(for prId: String) -> WatchlistService.WatchedPRInfo? { reportIssue("Unimplemented: WatchlistServicing.getWatchedStatus called without a test override") return nil @@ -239,6 +244,10 @@ private struct UnimplementedNotificationService: NotificationServicing { func notifyBuildComplete(pr: PullRequest, status: BuildStatus) { reportIssue("Unimplemented: NotificationServicing.notifyBuildComplete called without a test override") } + + func notifyPRUpdated(pr: PullRequest) { + reportIssue("Unimplemented: NotificationServicing.notifyPRUpdated called without a test override") + } } private struct UnimplementedPRCacheService: PRCacheServicing { diff --git a/MonitorLizard/MonitorLizardApp.swift b/MonitorLizard/MonitorLizardApp.swift index 55e3d58..e72f03f 100644 --- a/MonitorLizard/MonitorLizardApp.swift +++ b/MonitorLizard/MonitorLizardApp.swift @@ -36,7 +36,8 @@ struct MonitorLizardApp: App { PreferenceKeys.enableSounds.rawValue: true, PreferenceKeys.enableVoice.rawValue: true, PreferenceKeys.showNotifications.rawValue: true, - PreferenceKeys.voiceAnnouncementText.rawValue: Constants.defaultVoiceAnnouncementText, + PreferenceKeys.voiceAnnouncementTextBuildComplete.rawValue: Constants.defaultVoiceAnnouncementTextBuildComplete, + PreferenceKeys.voiceAnnouncementTextPRUpdated.rawValue: Constants.defaultVoiceAnnouncementTextPRUpdated, PreferenceKeys.enableInactiveBranchDetection.rawValue: false, PreferenceKeys.hideInactivePRs.rawValue: false, PreferenceKeys.inactiveBranchThresholdDays.rawValue: Constants.defaultInactiveBranchThreshold, diff --git a/MonitorLizard/PreferenceKeys.swift b/MonitorLizard/PreferenceKeys.swift index 0d25fa1..8fedaf1 100644 --- a/MonitorLizard/PreferenceKeys.swift +++ b/MonitorLizard/PreferenceKeys.swift @@ -6,7 +6,8 @@ enum PreferenceKeys: String, Sendable { case showReviewPRs case enableSounds case enableVoice - case voiceAnnouncementText + case voiceAnnouncementTextBuildComplete + case voiceAnnouncementTextPRUpdated case showNotifications case enableInactiveBranchDetection case hideInactivePRs diff --git a/MonitorLizard/Services/NotificationService.swift b/MonitorLizard/Services/NotificationService.swift index cbbfdf6..7ff3c81 100644 --- a/MonitorLizard/Services/NotificationService.swift +++ b/MonitorLizard/Services/NotificationService.swift @@ -7,6 +7,7 @@ import UserNotifications protocol NotificationServicing: Sendable { func requestAuthorization() async throws func notifyBuildComplete(pr: PullRequest, status: BuildStatus) + func notifyPRUpdated(pr: PullRequest) } /// Posts system notifications, plays sounds, and speaks announcements for build completions. @@ -34,9 +35,14 @@ final class NotificationService: NotificationServicing, @unchecked Sendable { return defaults.bool(forKey: PreferenceKeys.showNotifications) } - private var voiceAnnouncementText: String { + private var voiceAnnouncementTextBuildComplete: String { assertMainThread() - return defaults.string(forKey: PreferenceKeys.voiceAnnouncementText) ?? Constants.defaultVoiceAnnouncementText + return defaults.string(forKey: PreferenceKeys.voiceAnnouncementTextBuildComplete) ?? Constants.defaultVoiceAnnouncementTextBuildComplete + } + + private var voiceAnnouncementTextPRUpdated: String { + assertMainThread() + return defaults.string(forKey: PreferenceKeys.voiceAnnouncementTextPRUpdated) ?? Constants.defaultVoiceAnnouncementTextPRUpdated } func requestAuthorization() async throws { @@ -44,52 +50,65 @@ final class NotificationService: NotificationServicing, @unchecked Sendable { try await center.requestAuthorization(options: [.alert, .sound, .badge]) } - func notifyBuildComplete(pr: PullRequest, status: BuildStatus) { - if notificationsEnabled { - showNotification(pr: pr, status: status) - } - - if soundsEnabled { - playSound(for: status) - } + func notifyPRUpdated(pr: PullRequest) { + let content = UNMutableNotificationContent() + content.title = "PR Updated" + content.subtitle = pr.displayTitle + content.body = "\(pr.repository.name) #\(pr.number)" + content.sound = .default - if voiceEnabled && status == .success { - speak(text: voiceAnnouncementText) - } + notify( + identifier: "\(pr.id)-updated", + content: content, + soundName: "Glass", + speakText: voiceAnnouncementTextPRUpdated + ) } - private func showNotification(pr: PullRequest, status: BuildStatus) { + func notifyBuildComplete(pr: PullRequest, status: BuildStatus) { let content = UNMutableNotificationContent() content.title = "\(status.icon) Build \(status.displayName)" content.subtitle = pr.title content.body = "PR #\(pr.number) in \(pr.repository.name)" content.sound = status == .success ? .default : .defaultCritical - let request = UNNotificationRequest( - identifier: pr.id, - content: content, - trigger: nil - ) - - UNUserNotificationCenter.current().add(request) { error in - if let error = error { - print("Error showing notification: \(error.localizedDescription)") - } - } - } - - private func playSound(for status: BuildStatus) { - let soundName: String - + let soundName: String? switch status { case .success: soundName = "Glass" case .failure, .error: soundName = "Basso" default: - return + soundName = nil + } + + notify( + identifier: "\(pr.id)-build", + content: content, + soundName: soundName, + speakText: status == .success ? voiceAnnouncementTextBuildComplete : nil + ) + } + + private func notify(identifier: String, content: UNMutableNotificationContent, soundName: String?, speakText: String?) { + if notificationsEnabled { + let request = UNNotificationRequest(identifier: identifier, content: content, trigger: nil) + UNUserNotificationCenter.current().add(request) { error in + if let error = error { + print("Error showing notification: \(error.localizedDescription)") + } + } } + if soundsEnabled, let soundName { + play(soundNamed: soundName) + } + if voiceEnabled, let speakText { + speak(text: speakText) + } + } + + private func play(soundNamed soundName: String) { if let soundURL = NSSound(named: soundName) { soundURL.play() } else if let soundPath = Bundle.main.path(forResource: soundName, ofType: "aiff") { diff --git a/MonitorLizard/Services/WatchlistService.swift b/MonitorLizard/Services/WatchlistService.swift index 80cf207..4eb70fa 100644 --- a/MonitorLizard/Services/WatchlistService.swift +++ b/MonitorLizard/Services/WatchlistService.swift @@ -6,6 +6,7 @@ protocol WatchlistServicing: Sendable { func unwatch(_ pr: PullRequest) func isWatched(_ pr: PullRequest) -> Bool func checkForCompletions(currentPRs: [PullRequest]) -> [PullRequest] + func checkForUpdates(currentPRs: [PullRequest]) -> [PullRequest] func clearAll() func getWatchedStatus(for prId: String) -> WatchlistService.WatchedPRInfo? } @@ -66,7 +67,7 @@ final class WatchlistService: WatchlistServicing, @unchecked Sendable { completed.append(pr) } - if watched.lastStatus != pr.buildStatus || watched.lastUpdatedAt != pr.updatedAt { + if watched.lastStatus != pr.buildStatus { watchedPRs[pr.id] = WatchedPRInfo( lastStatus: pr.buildStatus, timestamp: Date(), @@ -87,6 +88,24 @@ final class WatchlistService: WatchlistServicing, @unchecked Sendable { return completed } + /// Check for watched PRs whose updatedAt has changed since last check. + /// Returns PRs that were updated (new comment, push, review, etc.) + func checkForUpdates(currentPRs: [PullRequest]) -> [PullRequest] { + var updated: [PullRequest] = [] + for pr in currentPRs { + guard let watched = watchedPRs[pr.id] else { continue } + if pr.updatedAt > watched.lastUpdatedAt { + updated.append(pr) + watchedPRs[pr.id] = WatchedPRInfo( + lastStatus: watched.lastStatus, + timestamp: watched.timestamp, + lastUpdatedAt: pr.updatedAt + ) + } + } + return updated + } + func getWatchedStatus(for prId: String) -> WatchedPRInfo? { assertMainThread() return watchedPRs[prId] diff --git a/MonitorLizard/ViewModels/PRMonitorViewModel.swift b/MonitorLizard/ViewModels/PRMonitorViewModel.swift index 80f57e6..91e5d84 100644 --- a/MonitorLizard/ViewModels/PRMonitorViewModel.swift +++ b/MonitorLizard/ViewModels/PRMonitorViewModel.swift @@ -226,6 +226,14 @@ class PRMonitorViewModel: ObservableObject { notificationService.notifyBuildComplete(pr: pr, status: pr.buildStatus) } + // Check for watched PR updates (new comments, reviews, pushes, etc.) + // Exclude PRs that already triggered a build completion notification. + let completedIDs = Set(completed.map { $0.id }) + let updated = watchlistService.checkForUpdates(currentPRs: dedupedPRs + fetchedOther) + for pr in updated where !completedIDs.contains(pr.id) { + notificationService.notifyPRUpdated(pr: pr) + } + unsortedPullRequests = applyCustomNames(dedupedPRs.map { pr in var updated = pr updated.isWatched = watchlistService.isWatched(pr) diff --git a/MonitorLizard/Views/SettingsView.swift b/MonitorLizard/Views/SettingsView.swift index e275535..cfbdf0a 100644 --- a/MonitorLizard/Views/SettingsView.swift +++ b/MonitorLizard/Views/SettingsView.swift @@ -4,6 +4,10 @@ import SwiftUI struct SettingsView: View { @Dependency(UserDefaultsStore.self) private var defaults + @State private var enableInactiveBranchDetectionValue: Bool = UserDefaults.standard.bool(forKey: PreferenceKeys.enableInactiveBranchDetection.rawValue) + @State private var hideInactivePRsValue: Bool = UserDefaults.standard.bool(forKey: PreferenceKeys.hideInactivePRs.rawValue) + @State private var inactiveBranchThresholdDaysValue: Int = UserDefaults.standard.object(forKey: PreferenceKeys.inactiveBranchThresholdDays.rawValue) as? Int ?? Constants.defaultInactiveBranchThreshold + private var refreshInterval: Binding { Binding( get: { defaults.object(forKey: PreferenceKeys.refreshInterval) as? Int ?? Constants.defaultRefreshInterval }, @@ -39,10 +43,17 @@ struct SettingsView: View { ) } - private var voiceAnnouncementText: Binding { + private var voiceAnnouncementTextBuildComplete: Binding { Binding( - get: { defaults.string(forKey: PreferenceKeys.voiceAnnouncementText) ?? Constants.defaultVoiceAnnouncementText }, - set: { defaults.set($0, forKey: PreferenceKeys.voiceAnnouncementText) } + get: { defaults.string(forKey: PreferenceKeys.voiceAnnouncementTextBuildComplete) ?? Constants.defaultVoiceAnnouncementTextBuildComplete }, + set: { defaults.set($0, forKey: PreferenceKeys.voiceAnnouncementTextBuildComplete) } + ) + } + + private var voiceAnnouncementTextPRUpdated: Binding { + Binding( + get: { defaults.string(forKey: PreferenceKeys.voiceAnnouncementTextPRUpdated) ?? Constants.defaultVoiceAnnouncementTextPRUpdated }, + set: { defaults.set($0, forKey: PreferenceKeys.voiceAnnouncementTextPRUpdated) } ) } @@ -55,22 +66,22 @@ struct SettingsView: View { private var enableInactiveBranchDetection: Binding { Binding( - get: { defaults.bool(forKey: PreferenceKeys.enableInactiveBranchDetection) }, - set: { defaults.set($0, forKey: PreferenceKeys.enableInactiveBranchDetection) } + get: { enableInactiveBranchDetectionValue }, + set: { enableInactiveBranchDetectionValue = $0; defaults.set($0, forKey: PreferenceKeys.enableInactiveBranchDetection) } ) } private var hideInactivePRs: Binding { Binding( - get: { defaults.bool(forKey: PreferenceKeys.hideInactivePRs) }, - set: { defaults.set($0, forKey: PreferenceKeys.hideInactivePRs) } + get: { hideInactivePRsValue }, + set: { hideInactivePRsValue = $0; defaults.set($0, forKey: PreferenceKeys.hideInactivePRs) } ) } private var inactiveBranchThresholdDays: Binding { Binding( - get: { defaults.object(forKey: PreferenceKeys.inactiveBranchThresholdDays) as? Int ?? Constants.defaultInactiveBranchThreshold }, - set: { defaults.set($0, forKey: PreferenceKeys.inactiveBranchThresholdDays) } + get: { inactiveBranchThresholdDaysValue }, + set: { inactiveBranchThresholdDaysValue = $0; defaults.set($0, forKey: PreferenceKeys.inactiveBranchThresholdDays) } ) } @@ -198,15 +209,26 @@ struct SettingsView: View { if enableVoice.wrappedValue { VStack(alignment: .leading, spacing: 4) { - Text("Announcement text") + Text("Build announcement text") .font(.caption) .foregroundColor(.secondary) - TextField("", text: voiceAnnouncementText, prompt: Text("Build ready for Q A")) + TextField("", text: voiceAnnouncementTextBuildComplete, prompt: Text(Constants.defaultVoiceAnnouncementTextBuildComplete)) .textFieldStyle(.roundedBorder) .help("The text that will be spoken when a watched build completes successfully") } .padding(.leading, 20) + + VStack(alignment: .leading, spacing: 4) { + Text("PR update announcement text") + .font(.caption) + .foregroundColor(.secondary) + + TextField("", text: voiceAnnouncementTextPRUpdated, prompt: Text(Constants.defaultVoiceAnnouncementTextPRUpdated)) + .textFieldStyle(.roundedBorder) + .help("The text that will be spoken when a watched PR is updated") + } + .padding(.leading, 20) } } diff --git a/MonitorLizardTests/PRMonitorViewModelTests.swift b/MonitorLizardTests/PRMonitorViewModelTests.swift index 2d7175f..0bbb157 100644 --- a/MonitorLizardTests/PRMonitorViewModelTests.swift +++ b/MonitorLizardTests/PRMonitorViewModelTests.swift @@ -948,4 +948,128 @@ struct PRMonitorViewModelTests { let repos = vm.availableRepositories #expect(repos.count == 2, "Repo list should still include all repos even when inactive PRs are hidden") } + + // MARK: - Notification deduplication + + @Test + func buildCompleteDoesNotAlsoTriggerPRUpdatedNotification() async { + let t1 = Date(timeIntervalSince1970: 1_000_000) + let t2 = Date(timeIntervalSince1970: 2_000_000) + + // PR watched while pending at t1 + let pendingPR = makePR(number: 1, nameWithOwner: "acme/widget", buildStatus: .pending) + let watchlist = withDependencies { $0.userDefaults = UserDefaultsStore.testSuite() } operation: { WatchlistService() } + watchlist.watch(pendingPR) + + // Same PR now completed (success) and updatedAt changed to t2 + let completedPR = makePR(number: 1, nameWithOwner: "acme/widget", type: .authored, updatedAt: t2) + + let spy = SpyNotificationService() + let github = StubGitHubService(prs: [completedPR]) + + let vm = withDependencies { + $0.userDefaults = UserDefaultsStore.testSuite() + $0.watchlistService = watchlist + $0.notificationService = spy + $0.otherPRsService = OtherPRsService() + $0.customNamesService = CustomNamesService() + $0.cacheService = PRCacheService() + $0[GitHubServiceKey.self] = github + } operation: { + let vm = PRMonitorViewModel(isDemoMode: false) + vm.stopPolling() + return vm + } + + await vm.refresh() + + #expect(spy.notifyBuildCompleteCallCount == 1) + #expect(spy.notifyPRUpdatedCallCount == 0) + } + + @Test + func prUpdatedNotificationFiredWhenNotBuildComplete() async { + let t1 = Date(timeIntervalSince1970: 1_000_000) + let t2 = Date(timeIntervalSince1970: 2_000_000) + + // PR watched while pending at t1 + let pendingPR = makePR(number: 1, nameWithOwner: "acme/widget", type: .authored, updatedAt: t1) + let watchlist = withDependencies { $0.userDefaults = UserDefaultsStore.testSuite() } operation: { WatchlistService() } + var watchedPending = pendingPR + watchedPending.buildStatus = .pending + watchlist.watch(watchedPending) + + // Same PR still pending but updatedAt changed to t2 (e.g. new comment) + let updatedPR = makePR(number: 1, nameWithOwner: "acme/widget", buildStatus: .pending) + let github = StubGitHubService(prs: [updatedPR], updatedAt: t2) + + let spy = SpyNotificationService() + + let vm = withDependencies { + $0.userDefaults = UserDefaultsStore.testSuite() + $0.watchlistService = watchlist + $0.notificationService = spy + $0.otherPRsService = OtherPRsService() + $0.customNamesService = CustomNamesService() + $0.cacheService = PRCacheService() + $0[GitHubServiceKey.self] = github + } operation: { + let vm = PRMonitorViewModel(isDemoMode: false) + vm.stopPolling() + return vm + } + + await vm.refresh() + + #expect(spy.notifyBuildCompleteCallCount == 0) + #expect(spy.notifyPRUpdatedCallCount == 1) + } +} + +@MainActor +private final class SpyNotificationService: NotificationServicing, @unchecked Sendable { + private(set) var notifyBuildCompleteCallCount = 0 + private(set) var notifyPRUpdatedCallCount = 0 + + func requestAuthorization() async throws {} + func notifyBuildComplete(pr: PullRequest, status: BuildStatus) { notifyBuildCompleteCallCount += 1 } + func notifyPRUpdated(pr: PullRequest) { notifyPRUpdatedCallCount += 1 } +} + +@MainActor +private final class StubGitHubService: GitHubServicing { + private let prs: [PullRequest] + private let updatedAt: Date? + + init(prs: [PullRequest], updatedAt: Date? = nil) { + self.prs = prs + self.updatedAt = updatedAt + } + + func checkGHAvailable() async throws {} + func invalidateHostsCache() {} + + func fetchAllOpenPRs(enableInactiveDetection: Bool, inactiveThresholdDays: Int, isDemoMode: Bool) async throws -> PRFetchResult { + let result = updatedAt.map { date in + prs.map { pr in + PullRequest( + number: pr.number, title: pr.title, repository: pr.repository, + url: pr.url, author: pr.author, headRefName: pr.headRefName, + updatedAt: date, buildStatus: pr.buildStatus, isWatched: pr.isWatched, + labels: pr.labels, type: pr.type, isDraft: pr.isDraft, + statusChecks: pr.statusChecks, reviewDecision: pr.reviewDecision, + host: pr.host, customName: pr.customName + ) + } + } ?? prs + return PRFetchResult(pullRequests: result, isPartial: false) + } + + func fetchPRStatus(owner: String, repo: String, number: Int, updatedAt: Date, enableInactiveDetection: Bool, inactiveThresholdDays: Int, host: String) async throws -> (status: BuildStatus, headRefName: String, statusChecks: [StatusCheck], reviewDecision: ReviewDecision?) { + (.success, "", [], nil) + } + + func fetchOtherPR(_ id: OtherPRIdentifier, enableInactiveDetection: Bool, inactiveThresholdDays: Int) async throws -> PullRequest? { + nil + } } diff --git a/MonitorLizardTests/WatchlistServiceTests.swift b/MonitorLizardTests/WatchlistServiceTests.swift index 9f46575..e6b6a2e 100644 --- a/MonitorLizardTests/WatchlistServiceTests.swift +++ b/MonitorLizardTests/WatchlistServiceTests.swift @@ -72,7 +72,7 @@ struct WatchlistServiceTests { #expect(service.getWatchedStatus(for: pr.id)?.lastUpdatedAt == t) } - @Test func lastUpdatedAtUpdatesAfterCheckForCompletions() { + @Test func lastUpdatedAtNotUpdatesAfterCheckForCompletions() { let service = makeService() let t1 = Date(timeIntervalSince1970: 1_000_000) let t2 = Date(timeIntervalSince1970: 2_000_000) @@ -81,7 +81,7 @@ struct WatchlistServiceTests { _ = service.checkForCompletions(currentPRs: [makePR(number: 1, updatedAt: t2)]) - #expect(service.getWatchedStatus(for: pr.id)?.lastUpdatedAt == t2) + #expect(service.getWatchedStatus(for: pr.id)?.lastUpdatedAt == t1) } @Test func statusChangeUpdatesStoredStatus() { @@ -104,10 +104,11 @@ struct WatchlistServiceTests { service.watch(pr) let updatedPR = makePR(number: 1, buildStatus: .success, updatedAt: t2) - _ = service.checkForCompletions(currentPRs: [updatedPR]) + _ = service.checkForUpdates(currentPRs: [updatedPR]) let info = service.getWatchedStatus(for: pr.id) #expect(info?.lastStatus == .success) + #expect(info?.lastUpdatedAt == t2) #expect((info?.timestamp ?? Date.distantPast) >= t1) } @@ -156,4 +157,39 @@ struct WatchlistServiceTests { #expect(service2.getWatchedStatus(for: pr.id)?.lastStatus == .pending) #expect(service2.getWatchedStatus(for: pr.id)?.lastUpdatedAt == Date(timeIntervalSince1970: 1_000_000)) } + + // MARK: - Update detection + + @Test func updatedAtChangedIsReportedAsUpdate() { + let service = makeService() + let t1 = Date(timeIntervalSince1970: 1_000_000) + let t2 = Date(timeIntervalSince1970: 2_000_000) + let pr = makePR(number: 1, updatedAt: t1) + service.watch(pr) + + let updated = service.checkForUpdates(currentPRs: [makePR(number: 1, updatedAt: t2)]) + + #expect(updated.count == 1) + #expect(updated[0].number == 1) + } + + @Test func unchangedUpdatedAtIsNotReportedAsUpdate() { + let service = makeService() + let t = Date(timeIntervalSince1970: 1_000_000) + let pr = makePR(number: 1, updatedAt: t) + service.watch(pr) + + let updated = service.checkForUpdates(currentPRs: [makePR(number: 1, updatedAt: t)]) + + #expect(updated.isEmpty) + } + + @Test func unwatchedPRIsNotReportedAsUpdate() { + let service = makeService() + let t2 = Date(timeIntervalSince1970: 2_000_000) + + let updated = service.checkForUpdates(currentPRs: [makePR(number: 1, updatedAt: t2)]) + + #expect(updated.isEmpty) + } } \ No newline at end of file