Merge pull request #26 from jasonlong/advisor/008-handle-same-thread-and-mixed-bulk-actions

Apply every checked activity in bulk triage

Author: jasonlong

Octodot/App/AppState.swift

@@ -20,6 +20,8 @@ final class AppState {
     private static let inboxModeStorageKey = "AppState.inboxMode.v1"
     private static let groupByRepoStorageKey = "AppState.groupByRepo.v1"
     private static let visibleSubjectStateBatchSize = 20
+    private static let unsupportedSecurityAlertActionMessage =
+        "Security alerts can only be opened or marked done"
     static let pageJumpCount = 8
     static let halfPageJumpCount = 4
 
@@ -459,27 +461,39 @@ final class AppState {
         if let batch = checkedNotificationsBatch() {
             let originalVisibleOrder = filteredNotifications
             let originalSelectionID = selectedNotificationID
+            var acceptedItems: [GitHubNotification] = []
             clearChecked()
-            for notification in batch {
-                if notification.source == .dependabotAlert {
-                    dismissSecurityAlert(notification, updatesSelection: false)
-                } else {
-                    startThreadAction(.done, target: notification, updatesSelection: false)
+
+            for notification in batch where notification.source == .dependabotAlert {
+                dismissSecurityAlert(notification, updatesSelection: false)
+                acceptedItems.append(notification)
+            }
+            for group in groupedThreadNotifications(from: batch) {
+                guard let representative = group.first else { continue }
+                if startThreadAction(
+                    .done,
+                    target: representative,
+                    activityIdentities: group.map(\.activityIdentity),
+                    updatesSelection: false
+                ) {
+                    acceptedItems.append(contentsOf: group)
                 }
             }
+
             restoreSelectionAfterBulkMutation(
                 originalSelectionID: originalSelectionID,
                 originalVisibleOrder: originalVisibleOrder
             )
-            presentActionToast(verb: .done, items: batch)
-            return
-        }
-        if dismissSelectedSecurityAlertIfNeeded() {
+            presentActionToast(verb: .done, items: acceptedItems)
             return
         }
         guard let target = selectedNotification else { return }
-        startThreadAction(.done)
-        presentActionToast(verb: .done, items: [target])
+        if target.source == .dependabotAlert {
+            dismissSecurityAlert(target)
+            presentActionToast(verb: .done, items: [target])
+        } else if startThreadAction(.done) {
+            presentActionToast(verb: .done, items: [target])
+        }
     }
 
     func markRead() {
@@ -490,29 +504,44 @@ final class AppState {
         if let batch = checkedNotificationsBatch() {
             let originalVisibleOrder = filteredNotifications
             let originalSelectionID = selectedNotificationID
+            let hasUnsupportedAlerts = batch.contains { $0.source == .dependabotAlert }
+            var acceptedItems: [GitHubNotification] = []
             clearChecked()
-            for notification in batch {
-                if notification.source == .dependabotAlert {
-                    dismissSecurityAlert(notification, updatesSelection: false)
-                } else {
-                    inboxStore.muteThread(notification.threadId)
-                    startThreadAction(.unsubscribe, target: notification, updatesSelection: false)
+
+            for group in groupedThreadNotifications(from: batch) {
+                guard let representative = group.first else { continue }
+                if startThreadAction(
+                    .unsubscribe,
+                    target: representative,
+                    activityIdentities: group.map(\.activityIdentity),
+                    updatesSelection: false
+                ) {
+                    inboxStore.muteThread(representative.threadId)
+                    clampSelection()
+                    acceptedItems.append(contentsOf: group)
                 }
             }
+
             restoreSelectionAfterBulkMutation(
                 originalSelectionID: originalSelectionID,
                 originalVisibleOrder: originalVisibleOrder
             )
-            presentActionToast(verb: .unsub, items: batch)
+            presentActionToast(verb: .unsub, items: acceptedItems)
+            if hasUnsupportedAlerts {
+                errorMessage = Self.unsupportedSecurityAlertActionMessage
+            }
             return
         }
-        if dismissSelectedSecurityAlertIfNeeded() {
+        guard let notification = selectedNotification else { return }
+        guard notification.source == .thread else {
+            errorMessage = Self.unsupportedSecurityAlertActionMessage
             return
         }
-        guard let notification = selectedNotification else { return }
-        inboxStore.muteThread(notification.threadId)
-        startThreadAction(.unsubscribe)
-        presentActionToast(verb: .unsub, items: [notification])
+        if startThreadAction(.unsubscribe) {
+            inboxStore.muteThread(notification.threadId)
+            clampSelection()
+            presentActionToast(verb: .unsub, items: [notification])
+        }
     }
 
     func copyURL() {
@@ -625,6 +654,24 @@ final class AppState {
         return ordered.isEmpty ? nil : ordered
     }
 
+    private func groupedThreadNotifications(
+        from notifications: [GitHubNotification]
+    ) -> [[GitHubNotification]] {
+        var groups: [[GitHubNotification]] = []
+        var groupIndexByThreadID: [String: Int] = [:]
+
+        for notification in notifications where notification.source == .thread {
+            if let groupIndex = groupIndexByThreadID[notification.threadId] {
+                groups[groupIndex].append(notification)
+            } else {
+                groupIndexByThreadID[notification.threadId] = groups.count
+                groups.append([notification])
+            }
+        }
+
+        return groups
+    }
+
     func refresh(force: Bool = false) {
         refresh(using: .uniform(force: force))
     }
@@ -875,20 +922,22 @@ final class AppState {
         selectedThreadID = nil
     }
 
+    @discardableResult
     private func startThreadAction(
         _ kind: ThreadActionStore.ActionKind,
         target explicitTarget: GitHubNotification? = nil,
+        activityIdentities: [String]? = nil,
         delayNanosecondsOverride: UInt64? = nil,
         updatesSelection: Bool = true
-    ) {
-        guard let client = apiClient else { return }
-        guard let target = explicitTarget ?? selectedNotification else { return }
+    ) -> Bool {
+        guard let client = apiClient else { return false }
+        guard let target = explicitTarget ?? selectedNotification else { return false }
         guard target.source == .thread else {
-            errorMessage = "Security alerts can only be opened or marked done"
-            return
+            errorMessage = Self.unsupportedSecurityAlertActionMessage
+            return false
         }
-        guard !threadActions.hasPendingAction(for: target.threadId) else { return }
-        if kind == .markRead && !target.isUnread { return }
+        guard !threadActions.hasPendingAction(for: target.threadId) else { return false }
+        if kind == .markRead && !target.isUnread { return false }
 
         DebugTrace.log(
             "start action kind=\(kind.rawValue) target.id=\(target.id) target.thread=\(target.threadId) " +
@@ -901,6 +950,7 @@ final class AppState {
         let pending = threadActions.start(
             kind,
             notification: target,
+            activityIdentities: activityIdentities,
             originalServerIndex: originalServerIndex
         )
 
@@ -923,15 +973,6 @@ final class AppState {
         } else {
             scheduleBatchDispatch(client: client, delayNanoseconds: delayNanoseconds)
         }
-    }
-
-    @discardableResult
-    private func dismissSelectedSecurityAlertIfNeeded() -> Bool {
-        guard let target = selectedNotification,
-              target.source == .dependabotAlert else {
-            return false
-        }
-        dismissSecurityAlert(target)
         return true
     }
 
@@ -1060,7 +1101,9 @@ final class AppState {
             "target.thread=\(pending.notification.threadId) server=\(serverNotifications.map(\.id).joined(separator: ","))"
         )
 
-        errorMessage = nil
+        if errorMessage != Self.unsupportedSecurityAlertActionMessage {
+            errorMessage = nil
+        }
         rebuildDerivedState()
         DebugTrace.log(
             "after rebuild success kind=\(pending.kind.rawValue) selected=\(selectedNotificationID ?? "nil") " +

Octodot/App/ThreadActionStore.swift

@@ -38,6 +38,7 @@ struct ThreadActionStore {
         let requestID: UUID
         let kind: ActionKind
         let notification: GitHubNotification
+        let activityIdentities: [String]
         let originalServerIndex: Int
         var phase: PendingActionPhase
     }
@@ -46,14 +47,15 @@ struct ThreadActionStore {
         let kind: ActionKind
         let threadId: String
         let updatedAt: Date
-        let activityIdentity: String?
+        let activityIdentities: [String]
     }
 
     private struct PersistedCommittedAction: Codable {
         let kind: ActionKind
         let threadId: String
         let updatedAt: Date
         let activityIdentity: String?
+        let activityIdentities: [String]?
     }
 
     private let userDefaults: UserDefaults
@@ -89,12 +91,14 @@ struct ThreadActionStore {
     mutating func start(
         _ kind: ActionKind,
         notification: GitHubNotification,
+        activityIdentities: [String]? = nil,
         originalServerIndex: Int
     ) -> PendingAction {
         let pending = PendingAction(
             requestID: UUID(),
             kind: kind,
             notification: notification,
+            activityIdentities: activityIdentities ?? [notification.activityIdentity],
             originalServerIndex: originalServerIndex,
             phase: .queued
         )
@@ -122,27 +126,27 @@ struct ThreadActionStore {
                 kind: .markRead,
                 threadId: pending.notification.threadId,
                 updatedAt: pending.notification.updatedAt,
-                activityIdentity: pending.notification.activityIdentity
+                activityIdentities: pending.activityIdentities
             )
             persistCommittedActions()
 
         case .done:
-            serverNotifications.removeAll { $0.matchesActivity(as: pending.notification) }
+            removeActivities(pending.activityIdentities, from: &serverNotifications)
             committedActions[pending.notification.threadId] = CommittedAction(
                 kind: .done,
                 threadId: pending.notification.threadId,
                 updatedAt: pending.notification.updatedAt,
-                activityIdentity: pending.notification.activityIdentity
+                activityIdentities: pending.activityIdentities
             )
             persistCommittedActions()
 
         case .unsubscribe:
-            serverNotifications.removeAll { $0.matchesActivity(as: pending.notification) }
+            removeActivities(pending.activityIdentities, from: &serverNotifications)
             committedActions[pending.notification.threadId] = CommittedAction(
                 kind: .unsubscribe,
                 threadId: pending.notification.threadId,
                 updatedAt: pending.notification.updatedAt,
-                activityIdentity: pending.notification.activityIdentity
+                activityIdentities: pending.activityIdentities
             )
             persistCommittedActions()
         }
@@ -164,11 +168,11 @@ struct ThreadActionStore {
                     projected[index].isUnread = false
                 }
             case .done, .unsubscribe:
-                hideDismissedActivity(
-                    activityIdentity: committed.activityIdentity,
+                hideDismissedActivities(
+                    activityIdentities: committed.activityIdentities,
                     threadId: committed.threadId,
                     updatedAt: committed.updatedAt,
-                    useLegacyThreadFallback: committed.activityIdentity == nil,
+                    useLegacyThreadFallback: committed.activityIdentities.isEmpty,
                     in: &projected
                 )
             }
@@ -182,8 +186,8 @@ struct ThreadActionStore {
                 }
 
             case .done, .unsubscribe:
-                hideDismissedActivity(
-                    activityIdentity: pending.notification.activityIdentity,
+                hideDismissedActivities(
+                    activityIdentities: pending.activityIdentities,
                     threadId: pending.notification.threadId,
                     updatedAt: pending.notification.updatedAt,
                     useLegacyThreadFallback: false,
@@ -212,8 +216,9 @@ struct ThreadActionStore {
                 guard !fetchedSnapshots.isEmpty else {
                     return true
                 }
-                if let activityIdentity = committed.activityIdentity {
-                    if fetchedSnapshots.contains(where: { $0.activityIdentity == activityIdentity }) {
+                if !committed.activityIdentities.isEmpty {
+                    let representedIdentities = Set(committed.activityIdentities)
+                    if fetchedSnapshots.contains(where: { representedIdentities.contains($0.activityIdentity) }) {
                         return true
                     }
                     if fetchedSnapshots.count == 1,
@@ -237,38 +242,40 @@ struct ThreadActionStore {
         pendingActions.removeAll()
     }
 
-    private func hideDismissedActivity(
-        activityIdentity: String?,
+    private func hideDismissedActivities(
+        activityIdentities: [String],
         threadId: String,
         updatedAt: Date,
         useLegacyThreadFallback: Bool,
         in notifications: inout [GitHubNotification]
     ) {
-        if let activityIdentity,
-           let exactIndex = notifications.firstIndex(where: { $0.activityIdentity == activityIdentity }) {
-            notifications.remove(at: exactIndex)
-            return
-        }
-
-        let threadSnapshotIndices = notifications.indices.filter {
-            notifications[$0].threadId == threadId
-        }
+        if !activityIdentities.isEmpty {
+            let representedIdentities = Set(activityIdentities)
+            let originalCount = notifications.count
+            notifications.removeAll { representedIdentities.contains($0.activityIdentity) }
+            if notifications.count < originalCount {
+                return
+            }
 
-        guard threadSnapshotIndices.count == 1 else {
-            if useLegacyThreadFallback {
-                notifications.removeAll {
-                    $0.threadId == threadId && $0.updatedAt <= updatedAt
-                }
+            let threadSnapshots = notifications.filter { $0.threadId == threadId }
+            if threadSnapshots.count == 1, threadSnapshots[0].updatedAt <= updatedAt {
+                notifications.removeAll { $0.id == threadSnapshots[0].id }
             }
             return
         }
 
-        let snapshotIndex = threadSnapshotIndices[0]
-        guard notifications[snapshotIndex].updatedAt <= updatedAt else {
-            return
+        guard useLegacyThreadFallback else { return }
+        notifications.removeAll {
+            $0.threadId == threadId && $0.updatedAt <= updatedAt
         }
+    }
 
-        notifications.remove(at: snapshotIndex)
+    private func removeActivities(
+        _ activityIdentities: [String],
+        from notifications: inout [GitHubNotification]
+    ) {
+        let representedIdentities = Set(activityIdentities)
+        notifications.removeAll { representedIdentities.contains($0.activityIdentity) }
     }
 
     private static func loadCommittedActions(from userDefaults: UserDefaults) -> [String: CommittedAction] {
@@ -291,7 +298,7 @@ struct ThreadActionStore {
                         kind: $0.kind,
                         threadId: $0.threadId,
                         updatedAt: $0.updatedAt,
-                        activityIdentity: $0.activityIdentity
+                        activityIdentities: $0.activityIdentities ?? $0.activityIdentity.map { [$0] } ?? []
                     )
                 )
             }
@@ -309,7 +316,8 @@ struct ThreadActionStore {
                 kind: $0.kind,
                 threadId: $0.threadId,
                 updatedAt: $0.updatedAt,
-                activityIdentity: $0.activityIdentity
+                activityIdentity: $0.activityIdentities.first,
+                activityIdentities: $0.activityIdentities
             )
         }