From 100d7c412c21fa2be174fcd28e6abd324aed8d5f Mon Sep 17 00:00:00 2001 From: systemblue Date: Sat, 13 Jun 2026 14:01:56 +0200 Subject: [PATCH] fix: de-duplicate involved-scope results across pages in single-instance overviews (#70) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Why.** The Issues and Pull Requests overviews fire one search request per involvement flag (created/assigned/mentioned/review-requested) and merge the results, so the same issue can resurface on a later page — created on page 1, assigned on page 2. The merged multi-instance overview already de-duplicates across pages with a pagination dedupe key, but the single-instance overview never set one, so loading more appended duplicate rows with duplicate Identifiable ids. **What changed.** `SearchableOverviewView.init` sets the same `dedupeKey` the merged overview sets (`MergedSearchableOverviewView` line 71), routing pages through `PaginationState`'s existing cross-page dedupe. The `dedupeKey` doc comment is updated to match. `hasMore` switches to the contributed-new-items heuristic, which fits combined per-flag sources whose merged page size never matches the request limit. **Verification.** New `PaginationStateDedupeTests` pin the keyed path: a key repeated across pages is filtered out on the later page, `hasMore` follows `!newItems.isEmpty` when `dedupeKey` is set (including a full page of already-seen keys ending pagination), and a reload resets the seen keys. Full `ForjiTests` suite passes (0 failed, iPhone 17 Pro, iOS 26.5). SwiftLint clean with the repo config. I grant Stefan Hausotte an irrevocable, worldwide, royalty-free license to use, sublicense, and distribute my contribution, including through Apple's App Store under the project's App Store exception. Reviewed-on: https://codeberg.org/secana/Forji/pulls/70 --- Forji/Forji/Helpers/PaginationState.swift | 8 ++-- .../Forji/Views/SearchableOverviewView.swift | 1 + Forji/ForjiTests/PaginationStateTests.swift | 47 +++++++++++++++++++ 3 files changed, 52 insertions(+), 4 deletions(-) diff --git a/Forji/Forji/Helpers/PaginationState.swift b/Forji/Forji/Helpers/PaginationState.swift index b5b66c2..865629b 100644 --- a/Forji/Forji/Helpers/PaginationState.swift +++ b/Forji/Forji/Helpers/PaginationState.swift @@ -16,10 +16,10 @@ 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`. + /// Optional key extractor enabling cross-page de-duplication. Views that + /// combine several paginated sources set this (merged multi-instance lists, + /// or the per-flag involvement queries), because the same item can resurface + /// on a later page and the combined page size no longer matches `pageSize`. @ObservationIgnored var dedupeKey: ((Item) -> AnyHashable)? @ObservationIgnored private var seenKeys = Set() diff --git a/Forji/Forji/Views/SearchableOverviewView.swift b/Forji/Forji/Views/SearchableOverviewView.swift index e912f30..f746bc3 100644 --- a/Forji/Forji/Views/SearchableOverviewView.swift +++ b/Forji/Forji/Views/SearchableOverviewView.swift @@ -59,6 +59,7 @@ struct SearchableOverviewView: View { let state = PaginationState() state.cacheName = issueType + state.dedupeKey = { $0.id } state.loadFromCache() _pagination = State(initialValue: state) } diff --git a/Forji/ForjiTests/PaginationStateTests.swift b/Forji/ForjiTests/PaginationStateTests.swift index d1dcd45..b71a9e5 100644 --- a/Forji/ForjiTests/PaginationStateTests.swift +++ b/Forji/ForjiTests/PaginationStateTests.swift @@ -475,3 +475,50 @@ struct PaginationStateLoadMoreTests { #expect(!pagination.isLoading) } } + +// MARK: - Keyed de-duplication + +struct PaginationStateDedupeTests { + + @Test @MainActor func loadMoreFiltersKeysSeenOnEarlierPages() async { + let pagination = PaginationState(pageSize: 2) + pagination.dedupeKey = { $0 } + await pagination.reload { _, _ in ["a", "b"] }.value + + // "b" resurfaces on page 2 — only "c" is new + await pagination.loadMore { _, _ in ["b", "c"] } + #expect(pagination.items == ["a", "b", "c"]) + #expect(pagination.hasMore) // the page contributed a new item + } + + @Test @MainActor func hasMoreFollowsNewItemsNotPageSize() async { + let pagination = PaginationState(pageSize: 5) + pagination.dedupeKey = { $0 } + await pagination.reload { _, _ in ["a", "b"] }.value + #expect(pagination.hasMore) // 2 < pageSize 5, but the page contributed new items + } + + @Test @MainActor func hasMoreFalseWhenFullPageIsAllDuplicates() async { + let pagination = PaginationState(pageSize: 2) + pagination.dedupeKey = { $0 } + await pagination.reload { _, _ in ["a", "b"] }.value + #expect(pagination.hasMore) + + // 2 >= pageSize 2, but every key was already seen -> pagination ends + await pagination.loadMore { _, _ in ["a", "b"] } + #expect(pagination.items == ["a", "b"]) + #expect(!pagination.hasMore) + } + + @Test @MainActor func reloadResetsSeenKeys() async { + let pagination = PaginationState(pageSize: 2) + pagination.dedupeKey = { $0 } + await pagination.reload { _, _ in ["a", "b"] }.value + await pagination.loadMore { _, _ in ["b", "c"] } + #expect(pagination.items == ["a", "b", "c"]) + + // A fresh reload must accept keys seen in the previous cycle + await pagination.reload { _, _ in ["a", "b"] }.value + #expect(pagination.items == ["a", "b"]) + } +}