Prevent race condition caused by loading newer notification groups at the same time as older

Possible when very few notifications groups exist for this user, so the bottom loader is visible as soon as the notifications tab is opened.

Author: whattherestimefor

Mastodon/In Progress New Layout and Datamodel/GroupedNotificationFeedLoader.swift

@@ -20,16 +20,18 @@ final public class GroupedNotificationFeedLoader {
         let canLoadOlder: Bool
     }
 
-    struct FeedLoadRequest: Equatable {
-        let olderThan: String?
-        let newerThan: String?
+    public enum FeedLoadRequest {
+        case older
+        case newer
+        case reload
 
         var resultsInsertionPoint: InsertLocation {
-            if olderThan != nil {
+            switch self {
+            case .older:
                 return .end
-            } else if newerThan != nil {
+            case .newer:
                 return .start
-            } else {
+            case .reload:
                 return .replace
             }
         }
@@ -44,6 +46,8 @@ final public class GroupedNotificationFeedLoader {
         subsystem: "GroupedNotificationFeedLoader", category: "Data")
     private static let entryNotFoundMessage =
         "Failed to find suitable record. Depending on the context this might result in errors (data not being updated) or can be discarded (e.g. when there are mixed data sources where an entry might or might not exist)."
+    
+    private var loadRequestQueue = [FeedLoadRequest]()
 
     @Published private(set) var records: FeedLoadResult = FeedLoadResult(
         allRecords: [], canLoadOlder: true)
@@ -53,7 +57,15 @@ final public class GroupedNotificationFeedLoader {
 
     private let timestampUpdater = TimestampUpdater(TimeInterval(30))
 
-    private var isFetching: Bool = false
+    private var isFetching: Bool = false {
+        didSet {
+            if !isFetching, let waitingRequest = nextRequestThatCanBeLoadedNow() {
+                Task {
+                    await load(waitingRequest)
+                }
+            }
+        }
+    }
 
     public let useGroupedNotificationsApi: Bool
     private let cacheManager: (any NotificationsCacheManager)?
@@ -104,21 +116,18 @@ final public class GroupedNotificationFeedLoader {
             .$activeFilterBox
             .sink { filterBox in
                 if filterBox != nil {
-                    Task { [weak self] in
-                        guard let self else { return }
-                        let curAllRecords = self.records.allRecords
-                        let curCanLoadOlder = self.records.canLoadOlder
-                        await self.replaceRecordsAfterFiltering(
-                            curAllRecords, canLoadOlder: curCanLoadOlder)
-                    }
+                    let curAllRecords = self.records.allRecords
+                    let curCanLoadOlder = self.records.canLoadOlder
+                    self.replaceRecordsAfterFiltering(
+                        curAllRecords, canLoadOlder: curCanLoadOlder)
                 }
             }
     }
 
     public func doFirstLoad() {
         Task {
             do {
-                try await loadCached()
+                try loadCached()
             } catch {
             }
             do {
@@ -128,18 +137,24 @@ final public class GroupedNotificationFeedLoader {
                 }
             } catch {
             }
-            await asyncLoadMore(olderThan: nil, newerThan: records.allRecords.first?.newestID)
+            requestLoad(.newer)
         }
     }
 
     public func commitToCache() async {
         await cacheManager?.commitToCache()
     }
+    
+    private func noMoreResultsToFetch() {
+        if records.canLoadOlder {
+            records = FeedLoadResult(allRecords: records.allRecords, canLoadOlder: false)
+        }
+    }
 
-    private func replaceRecordsAfterFiltering(_ unfiltered: [NotificationRowViewModel], canLoadOlder: Bool? = nil) async {
+    private func replaceRecordsAfterFiltering(_ unfiltered: [NotificationRowViewModel], canLoadOlder: Bool? = nil) {
         let filtered: [NotificationRowViewModel]
         if let filterBox = StatusFilterService.shared.activeFilterBox {
-            filtered = await filter(unfiltered, forFeed: kind, with: filterBox)
+            filtered = filter(unfiltered, forFeed: kind, with: filterBox)
         } else {
             filtered = unfiltered
         }
@@ -170,61 +185,105 @@ final public class GroupedNotificationFeedLoader {
         return deduped
     }
 
-
-    public func asyncLoadMore(
-        olderThan: String?,
-        newerThan: String?
-    ) async {
-        guard !isFetching else { return }
+    private func loadCached() throws {
+        guard !isFetching, let cacheManager else { return }
         isFetching = true
         defer {
             isFetching = false
         }
-        let request = FeedLoadRequest(
-            olderThan: olderThan, newerThan: newerThan)
-        do {
-            let newlyFetched = try await load(request)
-            await updateAfterInserting(newlyFetchedResults: newlyFetched, at: request.resultsInsertionPoint)
-        } catch {
-            presentError?(error)
+        let currentResults = cacheManager.currentResults()
+        try replaceRecordsAfterFiltering(rowViewModels(from: currentResults), canLoadOlder: true)
+    }
+
+    public func requestLoad(_ request: FeedLoadRequest) {
+        if !loadRequestQueue.contains(request) {
+            loadRequestQueue.append(request)
+        }
+        if let nextDoableRequest = nextRequestThatCanBeLoadedNow() {
+            Task {
+                await load(nextDoableRequest)
+            }
         }
     }
 
-    private func loadCached() async throws {
-        guard !isFetching, let cacheManager else { return }
-        isFetching = true
-        defer {
-            isFetching = false
+    public var permissionToLoadImmediately: Bool {
+        // This is only intended for use with pull to refresh, in order to properly update the progress spinner.
+        if isFetching {
+            return false
+        } else {
+            isFetching = true
+            return true
         }
-        let currentResults = await cacheManager.currentResults()
-        try await replaceRecordsAfterFiltering(rowViewModels(from: currentResults), canLoadOlder: true)
     }
-
-    private func load(_ request: FeedLoadRequest) async throws
-    -> NotificationsResultType
+    public func loadImmediately(_ request: FeedLoadRequest) async {
+        // This is only intended for use with pull to refresh, in order to properly update the progress spinner.
+        guard isFetching else { assertionFailure("request permissionToLoadImmediately before calling loadImmediately"); return }
+        await load(request)
+    }
+    
+    private func nextRequestThatCanBeLoadedNow() -> FeedLoadRequest? {
+        guard !isFetching else { return nil }
+        guard !loadRequestQueue.isEmpty else { return nil }
+        let nextRequest = loadRequestQueue.removeFirst()
+        isFetching = true
+        return nextRequest
+    }
+    
+    private func load(_ request: FeedLoadRequest) async
     {
-        switch kind {
-        case .notificationsAll:
-            return try await loadNotifications(
-                withScope: .everything, olderThan: request.olderThan, newerThan: request.newerThan)
-        case .notificationsMentionsOnly:
-            return try await loadNotifications(
-                withScope: .mentions, olderThan: request.olderThan, newerThan: request.newerThan)
-        case .notificationsWithAccount(let accountID):
-            return try await loadNotifications(
-                withAccountID: accountID, olderThan: request.olderThan, newerThan: request.newerThan)
+        defer { isFetching = false }
+        do {
+            let olderThan: String?
+            let newerThan: String?
+            switch request {
+            case .newer:
+                olderThan = nil
+                newerThan = records.allRecords.first?.newestID
+            case .older:
+                olderThan = records.allRecords.last?.oldestID
+                newerThan = nil
+            case .reload:
+                olderThan = nil
+                newerThan = nil
+            }
+            let results: NotificationsResultType
+            switch kind {
+            case .notificationsAll:
+                results = try await loadNotifications(
+                    withScope: .everything, olderThan: olderThan, newerThan: newerThan)
+            case .notificationsMentionsOnly:
+                results = try await loadNotifications(
+                    withScope: .mentions, olderThan: olderThan, newerThan: newerThan)
+            case .notificationsWithAccount(let accountID):
+                results = try await loadNotifications(
+                    withAccountID: accountID, olderThan: olderThan, newerThan: newerThan)
+            }
+            updateAfterInserting(newlyFetchedResults: results, at: request.resultsInsertionPoint)
+        } catch {
+            presentError?(error)
         }
     }
 }
 
 // MARK: - Filtering
 extension GroupedNotificationFeedLoader {
     private func updateAfterInserting(newlyFetchedResults: NotificationsResultType,
-                                      at insertionPoint: GroupedNotificationFeedLoader.FeedLoadRequest.InsertLocation) async {
+                                      at insertionPoint: GroupedNotificationFeedLoader.FeedLoadRequest.InsertLocation) {
+        switch insertionPoint {
+        case .start:
+            guard newlyFetchedResults.hasContents else { return }
+        case .replace:
+            break
+        case .end:
+            guard newlyFetchedResults.hasContents else {
+                noMoreResultsToFetch()
+                return
+            }
+        }
         guard let cacheManager else { assertionFailure(); return }
         do {
             cacheManager.updateByInserting(newlyFetched: newlyFetchedResults, at: insertionPoint)
-            let currentResults = await cacheManager.currentResults()
+            let currentResults = cacheManager.currentResults()
             let unfiltered = try rowViewModels(from: currentResults)
 
             let canLoadOlder: Bool? = {
@@ -238,7 +297,7 @@ extension GroupedNotificationFeedLoader {
                 }
             }()
 
-            await replaceRecordsAfterFiltering(unfiltered, canLoadOlder: canLoadOlder)
+            replaceRecordsAfterFiltering(unfiltered, canLoadOlder: canLoadOlder)
         } catch {
             presentError?(error)
         }
@@ -248,7 +307,7 @@ extension GroupedNotificationFeedLoader {
         _ records: [NotificationRowViewModel],
         forFeed feedKind: MastodonFeedKind,
         with filterBox: Mastodon.Entity.FilterBox
-    ) async -> [NotificationRowViewModel] {
+    ) -> [NotificationRowViewModel] {
         return records
     }
 }

Mastodon/In Progress New Layout and Datamodel/NotificationListViewController.swift

@@ -146,9 +146,7 @@ struct NotificationListView: View {
                     viewDidDisappear()
                 }
                 .accessibilityAction(named: L10n.Common.Controls.Actions.seeMore) {
-                    Task {
-                        await viewModel.refreshFeedFromTop()
-                    }
+                    viewModel.requestLoad(.newer)
                 }
             }
         }
@@ -219,9 +217,7 @@ struct NotificationListView: View {
 
     func viewDidAppear() {
         NotificationService.shared.clearNotificationCountForActiveUser()
-        Task {
-            await viewModel.refreshFeedFromTop()
-        }
+        viewModel.requestLoad(.newer)
     }
 
     func viewDidDisappear() {
@@ -232,7 +228,7 @@ struct NotificationListView: View {
     }
 
     func loadMore() {
-        viewModel.loadOlder()
+        viewModel.requestLoad(.older)
     }
 
     func didTap(item: NotificationListItem) {
@@ -369,9 +365,7 @@ private class NotificationListViewModel: ObservableObject {
             notificationPolicyBannerRow
             + withoutFilteredRow
 
-        Task {
-            await feedLoader.asyncLoadMore(olderThan: nil, newerThan: nil)
-        }
+        feedLoader.requestLoad(.reload)
     }
 
     func isUnread(_ item: NotificationListItem) -> Bool? {
@@ -436,17 +430,15 @@ private class NotificationListViewModel: ObservableObject {
     }
 
     public func refreshFeedFromTop() async {
-        let newestKnown = feedLoader.records.allRecords.first?.newestID
-        await feedLoader.asyncLoadMore(olderThan: nil, newerThan: newestKnown)
-    }
-
-    public func loadOlder() {
-        let oldestKnown = feedLoader.records.allRecords.last?.oldestID
-        Task {
-            await feedLoader.asyncLoadMore(olderThan: oldestKnown, newerThan: nil)
+        if feedLoader.permissionToLoadImmediately {
+            await feedLoader.loadImmediately(.newer)
         }
     }
 
+    public func requestLoad(_ loadRequest: GroupedNotificationFeedLoader.FeedLoadRequest) {
+        feedLoader.requestLoad(loadRequest)
+    }
+    
     public func commitToCache() async {
         await feedLoader.commitToCache()
     }

Mastodon/In Progress New Layout and Datamodel/NotificationsCacheManager.swift

@@ -7,7 +7,7 @@ import MastodonCore
 protocol NotificationsCacheManager<T> {
     associatedtype T: NotificationsResultType
 
-    func currentResults() async -> T?
+    func currentResults() -> T?
     var currentLastReadMarker: LastReadMarkers.MarkerPosition? { get }
     var mostRecentlyFetchedResults: T? { get }
     func updateByInserting(newlyFetched: NotificationsResultType, at insertionPoint: GroupedNotificationFeedLoader.FeedLoadRequest.InsertLocation)
@@ -16,9 +16,19 @@ protocol NotificationsCacheManager<T> {
     func commitToCache() async
 }
 
-protocol NotificationsResultType {}
-extension Mastodon.Entity.GroupedNotificationsResults: NotificationsResultType {}
-extension Array<Mastodon.Entity.Notification>: NotificationsResultType {}
+protocol NotificationsResultType {
+    var hasContents: Bool { get }
+}
+extension Mastodon.Entity.GroupedNotificationsResults: NotificationsResultType {
+    var hasContents: Bool {
+        return notificationGroups.isNotEmpty
+    }
+}
+extension Array<Mastodon.Entity.Notification>: NotificationsResultType {
+    var hasContents: Bool {
+        return isNotEmpty
+    }
+}
 
 @MainActor
 class UngroupedNotificationCacheManager: NotificationsCacheManager {
@@ -41,7 +51,7 @@ class UngroupedNotificationCacheManager: NotificationsCacheManager {
         self.mostRecentMarkers = nil
     }
 
-    func currentResults() async -> T? {
+    func currentResults() -> T? {
         if let mostRecentlyFetchedResults {
             return mostRecentlyFetchedResults
         } else if let staleResults {
@@ -273,7 +283,7 @@ class GroupedNotificationCacheManager: NotificationsCacheManager {
         mostRecentMarkers = updatable
     }
 
-    func currentResults() async -> T? {
+    func currentResults() -> T? {
         if let mostRecentlyFetchedResults {
             return mostRecentlyFetchedResults
         } else if let staleResults {