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.
2025-04-11 22:58:02 +02:00 · 2025-03-24 13:26:03 -04:00 · 2025-03-24 13:26:03 -04:00 · a0c6b2c1ea
commit a0c6b2c1ea
parent 7502daecd0
3 changed files with 142 additions and 81 deletions
--- a/Datamodel/GroupedNotificationFeedLoader.swift
+++ b/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,13 +116,10 @@ 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)
                }
            }
    }
@ -118,7 +127,7 @@ final public class GroupedNotificationFeedLoader {
    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,49 +185,82 @@ final public class GroupedNotificationFeedLoader {
        return deduped
    }
    
-
-    public func asyncLoadMore(
-        olderThan: String?,
-        newerThan: String?
-    ) async {
-        guard !isFetching 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)
-        }
-    }
-    
-    private func loadCached() async throws {
+    private func loadCached() throws {
        guard !isFetching, let cacheManager else { return }
        isFetching = true
        defer {
            isFetching = false
        }
-        let currentResults = await cacheManager.currentResults()
-        try await replaceRecordsAfterFiltering(rowViewModels(from: currentResults), canLoadOlder: true)
+        let currentResults = cacheManager.currentResults()
+        try replaceRecordsAfterFiltering(rowViewModels(from: currentResults), canLoadOlder: true)
    }

-    private func load(_ request: FeedLoadRequest) async throws
-    -> NotificationsResultType
+    public func requestLoad(_ request: FeedLoadRequest) {
+        if !loadRequestQueue.contains(request) {
+            loadRequestQueue.append(request)
+        }
+        if let nextDoableRequest = nextRequestThatCanBeLoadedNow() {
+            Task {
+                await load(nextDoableRequest)
+            }
+        }
+    }
+    
+    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
+        }
+    }
+    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)
        }
    }
 }
@ -220,11 +268,22 @@ final public class GroupedNotificationFeedLoader {
 // 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
    }
 }
--- a/Datamodel/NotificationListViewController.swift
+++ b/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()
    }
--- a/Datamodel/NotificationsCacheManager.swift
+++ b/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 {