From bec427d7da7a958fe6219c1365bee374418fed60 Mon Sep 17 00:00:00 2001 From: Stefan Hausotte Date: Thu, 4 Jun 2026 13:50:40 +0200 Subject: [PATCH] fix: settle merged pagination and drop duplicate rows (#47) PaginationState computed hasMore as `fetched.count >= pageSize`, but merged views return the combined, de-duplicated result of N instances, so the merged count cleared the single-source threshold and kept hasMore true past the real end (extra empty fetches). loadMore also appended without de-duping against already-loaded items, so overlapping involvement queries and shifting pages could produce duplicate TaggedItem.id entries in the ForEach. Add an optional dedupeKey to PaginationState that de-duplicates across pages via a running seen-set, and base hasMore on whether a page contributed new items when dedupeKey is set. Single-source pagination keeps the page-size heuristic unchanged. Merged Issues/PRs, Repositories, and Notifications views opt in with dedupeKey = { $0.id }. --- Forji/Forji/Helpers/PaginationState.swift | 36 ++++++++++++++++--- .../MergedNotificationsOverviewView.swift | 1 + .../Views/MergedRepositoryListView.swift | 1 + .../Views/MergedSearchableOverviewView.swift | 1 + 4 files changed, 35 insertions(+), 4 deletions(-) diff --git a/Forji/Forji/Helpers/PaginationState.swift b/Forji/Forji/Helpers/PaginationState.swift index 340a8bf..af74840 100644 --- a/Forji/Forji/Helpers/PaginationState.swift +++ b/Forji/Forji/Helpers/PaginationState.swift @@ -15,6 +15,13 @@ final class PaginationState { let pageSize: Int var cacheName: String? + /// Optional key extractor enabling cross-page de-duplication. Merged views set + /// this because they combine paginated results from several instances, where + /// the same item can resurface on a later page (overlapping involvement + /// queries) and the merged page size no longer matches `pageSize`. + @ObservationIgnored var dedupeKey: ((Item) -> AnyHashable)? + @ObservationIgnored private var seenKeys = Set() + init(pageSize: Int = 20) { self.pageSize = pageSize } @@ -42,8 +49,10 @@ final class PaginationState { do { let fetched = try await fetch(1, pageSize) guard !Task.isCancelled else { return } - items = fetched - hasMore = fetched.count >= pageSize + if dedupeKey != nil { seenKeys.removeAll() } + let newItems = deduped(fetched) + items = newItems + hasMore = computeHasMore(fetched: fetched, newItems: newItems) currentPage = 2 hasLoaded = true } catch is CancellationError { @@ -72,8 +81,9 @@ final class PaginationState { do { let fetched = try await fetch(currentPage, pageSize) guard !Task.isCancelled else { return } - items.append(contentsOf: fetched) - hasMore = fetched.count >= pageSize + let newItems = deduped(fetched) + items.append(contentsOf: newItems) + hasMore = computeHasMore(fetched: fetched, newItems: newItems) self.currentPage = currentPage + 1 } catch is CancellationError { // Ignore cancellation @@ -95,6 +105,24 @@ final class PaginationState { func invalidate() { hasLoaded = false } + + /// Filters out items already seen on a previous page when `dedupeKey` is set, + /// recording the new keys. Without a `dedupeKey` the page passes through + /// unchanged so single-source pagination keeps its existing behavior. + private func deduped(_ fetched: [Item]) -> [Item] { + guard let dedupeKey else { return fetched } + return fetched.filter { seenKeys.insert(dedupeKey($0)).inserted } + } + + /// For merged sources the de-duplicated, multi-instance page size no longer + /// matches `pageSize`, so "more pages exist" means "this page contributed new + /// items." Single sources keep the page-size heuristic. + private func computeHasMore(fetched: [Item], newItems: [Item]) -> Bool { + if dedupeKey != nil { + return !newItems.isEmpty + } + return fetched.count >= pageSize + } } // MARK: - Disk cache diff --git a/Forji/Forji/Views/MergedNotificationsOverviewView.swift b/Forji/Forji/Views/MergedNotificationsOverviewView.swift index 64476c2..6d4a684 100644 --- a/Forji/Forji/Views/MergedNotificationsOverviewView.swift +++ b/Forji/Forji/Views/MergedNotificationsOverviewView.swift @@ -11,6 +11,7 @@ struct MergedNotificationsOverviewView: View { self.manager = manager let state = PaginationState>() state.cacheName = "merged_notifications" + state.dedupeKey = { $0.id } state.loadFromCache() state.rehydrate(from: manager) _pagination = State(initialValue: state) diff --git a/Forji/Forji/Views/MergedRepositoryListView.swift b/Forji/Forji/Views/MergedRepositoryListView.swift index 9ddb636..18af1bd 100644 --- a/Forji/Forji/Views/MergedRepositoryListView.swift +++ b/Forji/Forji/Views/MergedRepositoryListView.swift @@ -14,6 +14,7 @@ struct MergedRepositoryListView: View { self.manager = manager let state = PaginationState>() state.cacheName = "merged_repos" + state.dedupeKey = { $0.id } state.loadFromCache() state.rehydrate(from: manager) _pagination = State(initialValue: state) diff --git a/Forji/Forji/Views/MergedSearchableOverviewView.swift b/Forji/Forji/Views/MergedSearchableOverviewView.swift index 0dcd769..46dd0c0 100644 --- a/Forji/Forji/Views/MergedSearchableOverviewView.swift +++ b/Forji/Forji/Views/MergedSearchableOverviewView.swift @@ -68,6 +68,7 @@ struct MergedSearchableOverviewView: let state = PaginationState>() state.cacheName = "merged_\(config.issueType)" + state.dedupeKey = { $0.id } state.loadFromCache() state.rehydrate(from: manager) _pagination = State(initialValue: state)