From c99c9612689ba6287a2b1b56e2d12a5e65e312f3 Mon Sep 17 00:00:00 2001 From: Cassio Rossi Date: Wed, 29 Jul 2026 23:37:21 +0100 Subject: [PATCH 1/5] fix(#304): stop deduplicate() from discarding favorited FeedDB/PodcastDB records FeedDB.deduplicate and PodcastDB.deduplicate picked the survivor of a duplicate postId group purely by recency (highest modifiedAt), with no regard for favorite state. Any time a local feed refresh raced an in-flight iCloud import (multi-device sync, or a cold launch right after an app update) and inserted a fresh, non-favorited copy, that fresh copy's modifiedAt would beat the older favorited copy and the favorite was silently deleted. PodcastDB.deduplicate additionally grouped by pubDate instead of postId, so two distinct episodes sharing a pubDate could wrongly be treated as duplicates and have one deleted outright. Both types now share ModelPrioritizable.isLessAuthoritative, which ranks a favorited copy above a merely more-recent one, and merge favorite/read state onto the surviving record before deleting the rest. Co-Authored-By: Claude --- .../Sources/FeedLibrary/Database/FeedDB.swift | 15 ++++-- .../FeedLibrary/Database/PodcastDB.swift | 14 ++++-- .../Tests/FeedLibraryTests/FeedDBTests.swift | 37 +++++++++++++++ .../FeedLibraryTests/PodcastDBTests.swift | 47 +++++++++++++------ .../MacMagazineLibrary/ModelProtocols.swift | 12 +++++ 5 files changed, 102 insertions(+), 23 deletions(-) diff --git a/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/FeedDB.swift b/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/FeedDB.swift index e68554fd..05df6eee 100644 --- a/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/FeedDB.swift +++ b/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/FeedDB.swift @@ -90,17 +90,24 @@ extension FeedDB: ModelReadable { } } +extension FeedDB: ModelPrioritizable {} + extension FeedDB: ModelDuplicable { public static func deduplicate(using context: ModelContext?) { let descriptor = FetchDescriptor(sortBy: [SortDescriptor(\FeedDB.pubDate, order: .reverse)]) guard let context, let data = try? context.fetch(descriptor) else { return } - let recordsToDelete = Dictionary(grouping: data, by: \.postId) - .values - .flatMap { $0.sorted { $0.modifiedAt > $1.modifiedAt }.dropFirst() } + for group in Dictionary(grouping: data, by: \.postId).values where group.count > 1 { + guard let survivor = group.max(by: FeedDB.isLessAuthoritative) else { continue } + survivor.favorite = group.contains { $0.favorite } + survivor.read = group.contains { $0.read } + + for record in group where record !== survivor { + context.delete(record) + } + } - recordsToDelete.forEach { context.delete($0) } try? context.save() } } diff --git a/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/PodcastDB.swift b/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/PodcastDB.swift index f88f8e97..7d1ab103 100644 --- a/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/PodcastDB.swift +++ b/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/PodcastDB.swift @@ -62,17 +62,23 @@ extension PodcastDB: ModelFavoritable { } } +extension PodcastDB: ModelPrioritizable {} + extension PodcastDB: ModelDuplicable { public static func deduplicate(using context: ModelContext?) { let descriptor = FetchDescriptor() guard let context, let data = try? context.fetch(descriptor) else { return } - let recordsToDelete = Dictionary(grouping: data, by: \.pubDate) - .values - .flatMap { $0.sorted { $0.modifiedAt > $1.modifiedAt }.dropFirst() } + for group in Dictionary(grouping: data, by: \.postId).values where group.count > 1 { + guard let survivor = group.max(by: PodcastDB.isLessAuthoritative) else { continue } + survivor.favorite = group.contains { $0.favorite } + + for record in group where record !== survivor { + context.delete(record) + } + } - recordsToDelete.forEach { context.delete($0) } try? context.save() } } diff --git a/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/FeedDBTests.swift b/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/FeedDBTests.swift index e2c084b3..c7aa1006 100644 --- a/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/FeedDBTests.swift +++ b/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/FeedDBTests.swift @@ -436,6 +436,43 @@ struct FeedDBTests { #expect(remaining.contains { $0.title == "Y-new" }) } + @Test("deduplicate keeps the favorited copy even when it is not the most recently modified") + func deduplicateKeepsFavoriteOverRecency() { + let storage = Database(models: [FeedDB.self], inMemory: true) + let favorited = FeedDB(postId: "dup-1", title: "Favorited", favorite: true, modifiedAt: Date(timeIntervalSince1970: 1000)) + let freshSync = FeedDB(postId: "dup-1", title: "Fresh Sync", favorite: false, modifiedAt: Date(timeIntervalSince1970: 2000)) + + storage.context.insert(favorited) + storage.context.insert(freshSync) + try? storage.context.save() + + FeedDB.deduplicate(using: storage.context) + + let remaining = storage.fetch(FeedDB.self) + #expect(remaining.count == 1) + #expect(remaining.first?.title == "Favorited") + #expect(remaining.first?.favorite == true) + } + + @Test("deduplicate merges favorite and read state onto the surviving record") + func deduplicateMergesFavoriteAndReadState() { + let storage = Database(models: [FeedDB.self], inMemory: true) + let favoritedButUnread = FeedDB(postId: "dup-1", favorite: true, modifiedAt: Date(timeIntervalSince1970: 1000)) + let readButNotFavorited = FeedDB(postId: "dup-1", favorite: false, modifiedAt: Date(timeIntervalSince1970: 2000)) + readButNotFavorited.read = true + + storage.context.insert(favoritedButUnread) + storage.context.insert(readButNotFavorited) + try? storage.context.save() + + FeedDB.deduplicate(using: storage.context) + + let remaining = storage.fetch(FeedDB.self) + #expect(remaining.count == 1) + #expect(remaining.first?.favorite == true) + #expect(remaining.first?.read == true) + } + @Test("deduplicate is safe on empty database") func deduplicateSafeOnEmpty() { let storage = Database(models: [FeedDB.self], inMemory: true) diff --git a/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift b/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift index 7e7ad875..263321a8 100644 --- a/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift +++ b/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift @@ -362,12 +362,11 @@ struct PodcastDBTests { // MARK: - ModelDuplicable Tests - @Test("deduplicate removes duplicates grouped by pubDate keeping most recently modified") + @Test("deduplicate removes duplicate postIds keeping most recently modified") func deduplicateKeepsMostRecentlyModified() { let storage = Database(models: [PodcastDB.self], inMemory: true) - let sharedPubDate = Date(timeIntervalSince1970: 5000) - let older = PodcastDB(postId: "p1", pubDate: sharedPubDate, modifiedAt: Date(timeIntervalSince1970: 1000)) - let newer = PodcastDB(postId: "p2", pubDate: sharedPubDate, modifiedAt: Date(timeIntervalSince1970: 2000)) + let older = PodcastDB(postId: "dup-1", title: "Older", modifiedAt: Date(timeIntervalSince1970: 1000)) + let newer = PodcastDB(postId: "dup-1", title: "Newer", modifiedAt: Date(timeIntervalSince1970: 2000)) storage.context.insert(older) storage.context.insert(newer) @@ -377,15 +376,16 @@ struct PodcastDBTests { let remaining = storage.fetch(PodcastDB.self) #expect(remaining.count == 1) - #expect(remaining.first?.postId == "p2") + #expect(remaining.first?.title == "Newer") } - @Test("deduplicate preserves podcasts with distinct pubDates") - func deduplicatePreservesDistinctPubDates() { + @Test("deduplicate preserves podcasts with distinct postIds sharing the same pubDate") + func deduplicatePreservesDistinctPostIds() { let storage = Database(models: [PodcastDB.self], inMemory: true) - storage.context.insert(PodcastDB(postId: "1", pubDate: Date(timeIntervalSince1970: 1000))) - storage.context.insert(PodcastDB(postId: "2", pubDate: Date(timeIntervalSince1970: 2000))) - storage.context.insert(PodcastDB(postId: "3", pubDate: Date(timeIntervalSince1970: 3000))) + let sharedPubDate = Date(timeIntervalSince1970: 5000) + storage.context.insert(PodcastDB(postId: "1", pubDate: sharedPubDate)) + storage.context.insert(PodcastDB(postId: "2", pubDate: sharedPubDate)) + storage.context.insert(PodcastDB(postId: "3", pubDate: sharedPubDate)) try? storage.context.save() PodcastDB.deduplicate(using: storage.context) @@ -394,15 +394,14 @@ struct PodcastDBTests { #expect(remaining.count == 3) } - @Test("deduplicate handles triple duplicates for same pubDate") + @Test("deduplicate handles triple duplicates for same postId") func deduplicateHandlesTripleDuplicates() { let storage = Database(models: [PodcastDB.self], inMemory: true) - let sharedDate = Date(timeIntervalSince1970: 9000) let now = Date() - storage.context.insert(PodcastDB(postId: "a", pubDate: sharedDate, modifiedAt: now.addingTimeInterval(-200))) - storage.context.insert(PodcastDB(postId: "b", pubDate: sharedDate, modifiedAt: now.addingTimeInterval(-100))) - storage.context.insert(PodcastDB(postId: "c", title: "Winner", pubDate: sharedDate, modifiedAt: now)) + storage.context.insert(PodcastDB(postId: "dup-1", modifiedAt: now.addingTimeInterval(-200))) + storage.context.insert(PodcastDB(postId: "dup-1", modifiedAt: now.addingTimeInterval(-100))) + storage.context.insert(PodcastDB(postId: "dup-1", title: "Winner", modifiedAt: now)) try? storage.context.save() PodcastDB.deduplicate(using: storage.context) @@ -412,6 +411,24 @@ struct PodcastDBTests { #expect(remaining.first?.title == "Winner") } + @Test("deduplicate keeps the favorited copy even when it is not the most recently modified") + func deduplicateKeepsFavoriteOverRecency() { + let storage = Database(models: [PodcastDB.self], inMemory: true) + let favorited = PodcastDB(postId: "dup-1", title: "Favorited", favorite: true, modifiedAt: Date(timeIntervalSince1970: 1000)) + let freshSync = PodcastDB(postId: "dup-1", title: "Fresh Sync", favorite: false, modifiedAt: Date(timeIntervalSince1970: 2000)) + + storage.context.insert(favorited) + storage.context.insert(freshSync) + try? storage.context.save() + + PodcastDB.deduplicate(using: storage.context) + + let remaining = storage.fetch(PodcastDB.self) + #expect(remaining.count == 1) + #expect(remaining.first?.title == "Favorited") + #expect(remaining.first?.favorite == true) + } + @Test("deduplicate is safe on empty database") func deduplicateSafeOnEmpty() { let storage = Database(models: [PodcastDB.self], inMemory: true) diff --git a/MacMagazine/Features/MacMagazineLibrary/Sources/MacMagazineLibrary/ModelProtocols.swift b/MacMagazine/Features/MacMagazineLibrary/Sources/MacMagazineLibrary/ModelProtocols.swift index 22e3ce4a..9f6f1076 100644 --- a/MacMagazine/Features/MacMagazineLibrary/Sources/MacMagazineLibrary/ModelProtocols.swift +++ b/MacMagazine/Features/MacMagazineLibrary/Sources/MacMagazineLibrary/ModelProtocols.swift @@ -12,3 +12,15 @@ public protocol ModelReadable: AnyObject, PersistentModel { public protocol ModelDuplicable: AnyObject, PersistentModel { static func deduplicate(using context: ModelContext?) } + +public protocol ModelPrioritizable { + var favorite: Bool { get } + var modifiedAt: Date { get } +} + +public extension ModelPrioritizable { + static func isLessAuthoritative(_ lhs: Self, _ rhs: Self) -> Bool { + guard lhs.favorite == rhs.favorite else { return rhs.favorite } + return lhs.modifiedAt < rhs.modifiedAt + } +} From 0b1eee7d9b0390e22e265dd65a75239cb32e24e1 Mon Sep 17 00:00:00 2001 From: Cassio Rossi Date: Wed, 29 Jul 2026 23:41:29 +0100 Subject: [PATCH 2/5] fix(#304): preserve podcast playback progress across deduplicate() PodcastDB has no read flag - current (playback position) is its equivalent per-device engagement state, updated via PodcastDBExtensions.save(current:). deduplicate() picked a survivor without merging it, so a duplicate racing in from iCloud sync or a cold-launch refresh could reset a listener's progress back to zero. Co-Authored-By: Claude --- .../FeedLibrary/Database/PodcastDB.swift | 1 + .../Tests/FeedLibraryTests/PodcastDBTests.swift | 17 +++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/PodcastDB.swift b/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/PodcastDB.swift index 7d1ab103..cc2c3820 100644 --- a/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/PodcastDB.swift +++ b/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/PodcastDB.swift @@ -73,6 +73,7 @@ extension PodcastDB: ModelDuplicable { for group in Dictionary(grouping: data, by: \.postId).values where group.count > 1 { guard let survivor = group.max(by: PodcastDB.isLessAuthoritative) else { continue } survivor.favorite = group.contains { $0.favorite } + survivor.current = group.map(\.current).max() ?? survivor.current for record in group where record !== survivor { context.delete(record) diff --git a/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift b/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift index 263321a8..74e4fc70 100644 --- a/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift +++ b/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift @@ -429,6 +429,23 @@ struct PodcastDBTests { #expect(remaining.first?.favorite == true) } + @Test("deduplicate keeps the furthest playback progress across duplicates") + func deduplicateKeepsFurthestPlaybackProgress() { + let storage = Database(models: [PodcastDB.self], inMemory: true) + let listenedFurther = PodcastDB(postId: "dup-1", current: 900, modifiedAt: Date(timeIntervalSince1970: 1000)) + let freshSync = PodcastDB(postId: "dup-1", current: 0, modifiedAt: Date(timeIntervalSince1970: 2000)) + + storage.context.insert(listenedFurther) + storage.context.insert(freshSync) + try? storage.context.save() + + PodcastDB.deduplicate(using: storage.context) + + let remaining = storage.fetch(PodcastDB.self) + #expect(remaining.count == 1) + #expect(remaining.first?.current == 900) + } + @Test("deduplicate is safe on empty database") func deduplicateSafeOnEmpty() { let storage = Database(models: [PodcastDB.self], inMemory: true) From b7f7fd7d44661efa6e29921e6fb4f3d26ee15ad9 Mon Sep 17 00:00:00 2001 From: Cassio Rossi Date: Wed, 29 Jul 2026 23:47:38 +0100 Subject: [PATCH 3/5] test(#304): cover the full favorite/read/progress merge matrix for deduplicate() Prior coverage only exercised favorite-vs-recency and one combined favorite+read case. Add a parameterized FeedDB test covering favorite-only, read-only, both-true, and tie-on-favorite merges, plus PodcastDB cases where the favorite winner and the furthest-progress duplicate are different records, and where both duplicates are favorited. Co-Authored-By: Claude --- .../Tests/FeedLibraryTests/FeedDBTests.swift | 37 ++++++++++++++----- .../FeedLibraryTests/PodcastDBTests.swift | 36 ++++++++++++++++++ 2 files changed, 64 insertions(+), 9 deletions(-) diff --git a/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/FeedDBTests.swift b/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/FeedDBTests.swift index c7aa1006..5ea1c084 100644 --- a/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/FeedDBTests.swift +++ b/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/FeedDBTests.swift @@ -4,6 +4,15 @@ import StorageLibrary import SwiftData import Testing +private struct FavoriteReadCase: Sendable { + let olderFavorite: Bool + let olderRead: Bool + let newerFavorite: Bool + let newerRead: Bool + let expectedFavorite: Bool + let expectedRead: Bool +} + @Suite("FeedDB Tests") @MainActor struct FeedDBTests { @@ -454,23 +463,33 @@ struct FeedDBTests { #expect(remaining.first?.favorite == true) } - @Test("deduplicate merges favorite and read state onto the surviving record") - func deduplicateMergesFavoriteAndReadState() { + @Test( + "deduplicate OR-merges favorite and read across every combination", + arguments: [ + FavoriteReadCase(olderFavorite: true, olderRead: false, newerFavorite: false, newerRead: false, expectedFavorite: true, expectedRead: false), + FavoriteReadCase(olderFavorite: false, olderRead: true, newerFavorite: false, newerRead: false, expectedFavorite: false, expectedRead: true), + FavoriteReadCase(olderFavorite: true, olderRead: true, newerFavorite: false, newerRead: false, expectedFavorite: true, expectedRead: true), + FavoriteReadCase(olderFavorite: true, olderRead: true, newerFavorite: true, newerRead: false, expectedFavorite: true, expectedRead: true), + FavoriteReadCase(olderFavorite: false, olderRead: false, newerFavorite: false, newerRead: false, expectedFavorite: false, expectedRead: false) + ] + ) + fileprivate func deduplicateMergesFavoriteAndReadState(_ testCase: FavoriteReadCase) { let storage = Database(models: [FeedDB.self], inMemory: true) - let favoritedButUnread = FeedDB(postId: "dup-1", favorite: true, modifiedAt: Date(timeIntervalSince1970: 1000)) - let readButNotFavorited = FeedDB(postId: "dup-1", favorite: false, modifiedAt: Date(timeIntervalSince1970: 2000)) - readButNotFavorited.read = true + let older = FeedDB(postId: "dup-1", favorite: testCase.olderFavorite, modifiedAt: Date(timeIntervalSince1970: 1000)) + older.read = testCase.olderRead + let newer = FeedDB(postId: "dup-1", favorite: testCase.newerFavorite, modifiedAt: Date(timeIntervalSince1970: 2000)) + newer.read = testCase.newerRead - storage.context.insert(favoritedButUnread) - storage.context.insert(readButNotFavorited) + storage.context.insert(older) + storage.context.insert(newer) try? storage.context.save() FeedDB.deduplicate(using: storage.context) let remaining = storage.fetch(FeedDB.self) #expect(remaining.count == 1) - #expect(remaining.first?.favorite == true) - #expect(remaining.first?.read == true) + #expect(remaining.first?.favorite == testCase.expectedFavorite) + #expect(remaining.first?.read == testCase.expectedRead) } @Test("deduplicate is safe on empty database") diff --git a/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift b/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift index 74e4fc70..59f16cce 100644 --- a/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift +++ b/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift @@ -446,6 +446,42 @@ struct PodcastDBTests { #expect(remaining.first?.current == 900) } + @Test("deduplicate keeps furthest progress even when it belongs to the non-favorited duplicate") + func deduplicateMergesProgressAcrossFavoriteWinner() { + let storage = Database(models: [PodcastDB.self], inMemory: true) + let favoritedButNotListened = PodcastDB(postId: "dup-1", favorite: true, current: 0, modifiedAt: Date(timeIntervalSince1970: 1000)) + let listenedButNotFavorited = PodcastDB(postId: "dup-1", favorite: false, current: 900, modifiedAt: Date(timeIntervalSince1970: 2000)) + + storage.context.insert(favoritedButNotListened) + storage.context.insert(listenedButNotFavorited) + try? storage.context.save() + + PodcastDB.deduplicate(using: storage.context) + + let remaining = storage.fetch(PodcastDB.self) + #expect(remaining.count == 1) + #expect(remaining.first?.favorite == true) + #expect(remaining.first?.current == 900) + } + + @Test("deduplicate keeps the furthest progress when both duplicates are favorited") + func deduplicateMergesProgressWhenBothFavorited() { + let storage = Database(models: [PodcastDB.self], inMemory: true) + let olderFavorited = PodcastDB(postId: "dup-1", favorite: true, current: 900, modifiedAt: Date(timeIntervalSince1970: 1000)) + let newerFavorited = PodcastDB(postId: "dup-1", favorite: true, current: 0, modifiedAt: Date(timeIntervalSince1970: 2000)) + + storage.context.insert(olderFavorited) + storage.context.insert(newerFavorited) + try? storage.context.save() + + PodcastDB.deduplicate(using: storage.context) + + let remaining = storage.fetch(PodcastDB.self) + #expect(remaining.count == 1) + #expect(remaining.first?.favorite == true) + #expect(remaining.first?.current == 900) + } + @Test("deduplicate is safe on empty database") func deduplicateSafeOnEmpty() { let storage = Database(models: [PodcastDB.self], inMemory: true) From 31a96485903964381240bf724ddbfb25faa47f73 Mon Sep 17 00:00:00 2001 From: Cassio Rossi Date: Thu, 30 Jul 2026 00:16:30 +0100 Subject: [PATCH 4/5] fix(#304): give favorite/read/progress their own authority timestamps Merging duplicates by a single shared modifiedAt (or OR-ing favorite/read) can't tell a spurious blank sync row apart from a genuine, more recent user action - including the case where the user's real latest action was to *remove* a favorite or mark something unread again. A boolean OR-merge can only turn these on, never off, silently resurrecting an intentional un-favorite made on another device. FeedDB/PodcastDB now carry favoriteModifiedAt/readModifiedAt (and PodcastDB's progressModifiedAt for playback position) alongside their existing modifiedAt, stamped only by the explicit action that changed that specific field. deduplicate() picks the content survivor by plain modifiedAt recency, then merges favorite/read/progress independently by whichever duplicate has the latest timestamp for that field - so a spurious sync row (whose action timestamp defaults to .distantPast) never outranks a real action, and a genuine newer un-favorite/rewind correctly overrides an older favorite/progress. SwiftData's @Model macro silently ignores didSet/willSet (confirmed via the guide-swiftdata skill's core rules), so each model now exposes an explicit toggleFavorite()/toggleRead()/markAsRead()/updateProgress() that stamps both the field and its timestamp together, replacing the duplicated three-line pattern at each call site. Co-Authored-By: Claude --- .../Sources/FeedLibrary/Database/FeedDB.swift | 40 +++++- .../FeedLibrary/Database/PodcastDB.swift | 33 ++++- .../Tests/FeedLibraryTests/FeedDBTests.swift | 126 ++++++++++++++---- .../FeedLibraryTests/PodcastDBTests.swift | 95 +++++++++---- .../MacMagazineLibrary/ModelProtocols.swift | 15 ++- .../Extensions/FeedDBExtensions.swift | 6 +- .../Extensions/PodcastDBExtensions.swift | 6 +- 7 files changed, 251 insertions(+), 70 deletions(-) diff --git a/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/FeedDB.swift b/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/FeedDB.swift index 05df6eee..845ea3f4 100644 --- a/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/FeedDB.swift +++ b/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/FeedDB.swift @@ -16,7 +16,9 @@ public final class FeedDB { public var excerpt: String = "" public var fullContent: String = "" public var favorite: Bool = false + public var favoriteModifiedAt: Date = Date.distantPast public var read: Bool = false + public var readModifiedAt: Date = Date.distantPast public var modifiedAt: Date = Date() public init( @@ -31,7 +33,9 @@ public final class FeedDB { excerpt: String = "", fullContent: String = "", favorite: Bool = false, + favoriteModifiedAt: Date = Date.distantPast, ead: Bool = false, + readModifiedAt: Date = Date.distantPast, modifiedAt: Date = Date() ) { self.postId = postId @@ -45,7 +49,9 @@ public final class FeedDB { self.excerpt = excerpt self.fullContent = fullContent self.favorite = favorite + self.favoriteModifiedAt = favoriteModifiedAt self.read = read + self.readModifiedAt = readModifiedAt self.modifiedAt = modifiedAt } } @@ -75,6 +81,12 @@ extension FeedDB: ModelFavoritable { data.forEach { context.delete($0) } try? context.save() } + + public func toggleFavorite() { + favorite.toggle() + favoriteModifiedAt = Date() + modifiedAt = Date() + } } extension FeedDB: ModelReadable { @@ -83,11 +95,22 @@ extension FeedDB: ModelReadable { guard let context, let data = try? context.fetch(descriptor) else { return } for post in data { - post.read = true - post.modifiedAt = Date() + post.markAsRead() } try? context.save() } + + public func toggleRead() { + read.toggle() + readModifiedAt = Date() + modifiedAt = Date() + } + + public func markAsRead() { + read = true + readModifiedAt = Date() + modifiedAt = Date() + } } extension FeedDB: ModelPrioritizable {} @@ -99,9 +122,16 @@ extension FeedDB: ModelDuplicable { let data = try? context.fetch(descriptor) else { return } for group in Dictionary(grouping: data, by: \.postId).values where group.count > 1 { - guard let survivor = group.max(by: FeedDB.isLessAuthoritative) else { continue } - survivor.favorite = group.contains { $0.favorite } - survivor.read = group.contains { $0.read } + guard let survivor = group.max(by: { $0.modifiedAt < $1.modifiedAt }) else { continue } + + if let latestFavorite = FeedDB.latest(in: group, value: { $0.favorite }, modifiedAt: { $0.favoriteModifiedAt }) { + survivor.favorite = latestFavorite.value + survivor.favoriteModifiedAt = latestFavorite.modifiedAt + } + if let latestRead = FeedDB.latest(in: group, value: { $0.read }, modifiedAt: { $0.readModifiedAt }) { + survivor.read = latestRead.value + survivor.readModifiedAt = latestRead.modifiedAt + } for record in group where record !== survivor { context.delete(record) diff --git a/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/PodcastDB.swift b/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/PodcastDB.swift index cc2c3820..09272d9b 100644 --- a/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/PodcastDB.swift +++ b/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/PodcastDB.swift @@ -15,8 +15,10 @@ public final class PodcastDB { public var duration: String = "" public var podcastFrame: String = "" public var favorite: Bool = false + public var favoriteModifiedAt: Date = Date.distantPast public var playable: Bool = false public var current: Double = 0.0 + public var progressModifiedAt: Date = Date.distantPast public var modifiedAt: Date = Date() public init( @@ -31,8 +33,10 @@ public final class PodcastDB { duration: String = "", podcastFrame: String = "", favorite: Bool = false, + favoriteModifiedAt: Date = Date.distantPast, playable: Bool = false, current: Double = 0.0, + progressModifiedAt: Date = Date.distantPast, modifiedAt: Date = Date() ) { self.postId = postId @@ -46,8 +50,10 @@ public final class PodcastDB { self.duration = duration self.podcastFrame = podcastFrame self.favorite = favorite + self.favoriteModifiedAt = favoriteModifiedAt self.playable = playable self.current = current + self.progressModifiedAt = progressModifiedAt self.modifiedAt = modifiedAt } } @@ -60,6 +66,20 @@ extension PodcastDB: ModelFavoritable { data.forEach { context.delete($0) } try? context.save() } + + public func toggleFavorite() { + favorite.toggle() + favoriteModifiedAt = Date() + modifiedAt = Date() + } +} + +extension PodcastDB { + public func updateProgress(_ current: Double) { + self.current = current + progressModifiedAt = Date() + modifiedAt = Date() + } } extension PodcastDB: ModelPrioritizable {} @@ -71,9 +91,16 @@ extension PodcastDB: ModelDuplicable { let data = try? context.fetch(descriptor) else { return } for group in Dictionary(grouping: data, by: \.postId).values where group.count > 1 { - guard let survivor = group.max(by: PodcastDB.isLessAuthoritative) else { continue } - survivor.favorite = group.contains { $0.favorite } - survivor.current = group.map(\.current).max() ?? survivor.current + guard let survivor = group.max(by: { $0.modifiedAt < $1.modifiedAt }) else { continue } + + if let latestFavorite = PodcastDB.latest(in: group, value: { $0.favorite }, modifiedAt: { $0.favoriteModifiedAt }) { + survivor.favorite = latestFavorite.value + survivor.favoriteModifiedAt = latestFavorite.modifiedAt + } + if let latestProgress = PodcastDB.latest(in: group, value: { $0.current }, modifiedAt: { $0.progressModifiedAt }) { + survivor.current = latestProgress.value + survivor.progressModifiedAt = latestProgress.modifiedAt + } for record in group where record !== survivor { context.delete(record) diff --git a/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/FeedDBTests.swift b/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/FeedDBTests.swift index 5ea1c084..02acaab6 100644 --- a/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/FeedDBTests.swift +++ b/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/FeedDBTests.swift @@ -4,11 +4,15 @@ import StorageLibrary import SwiftData import Testing -private struct FavoriteReadCase: Sendable { - let olderFavorite: Bool - let olderRead: Bool - let newerFavorite: Bool - let newerRead: Bool +private struct FieldMergeCase: Sendable { + let aFavorite: Bool + let aFavoriteModifiedAt: Date + let aRead: Bool + let aReadModifiedAt: Date + let bFavorite: Bool + let bFavoriteModifiedAt: Date + let bRead: Bool + let bReadModifiedAt: Date let expectedFavorite: Bool let expectedRead: Bool } @@ -330,6 +334,22 @@ struct FeedDBTests { #expect(remaining.isEmpty) } + @Test("toggleFavorite flips favorite and stamps favoriteModifiedAt without touching read") + func toggleFavoriteStampsFavoriteModifiedAt() { + let feed = FeedDB(postId: "1") + feed.read = true + let readModifiedAtBefore = feed.readModifiedAt + + feed.toggleFavorite() + + #expect(feed.favorite == true) + #expect(feed.favoriteModifiedAt != Date.distantPast) + #expect(feed.readModifiedAt == readModifiedAtBefore) + + feed.toggleFavorite() + #expect(feed.favorite == false) + } + // MARK: - ModelReadable Tests @Test("markAllAsRead should mark all unread posts as read") @@ -392,6 +412,30 @@ struct FeedDBTests { #expect(fetched?.modifiedAt != oldDate) } + @Test("markAsRead sets read and stamps readModifiedAt without touching favorite") + func markAsReadStampsReadModifiedAt() { + let feed = FeedDB(postId: "1", favorite: true) + let favoriteModifiedAtBefore = feed.favoriteModifiedAt + + feed.markAsRead() + + #expect(feed.read == true) + #expect(feed.readModifiedAt != Date.distantPast) + #expect(feed.favoriteModifiedAt == favoriteModifiedAtBefore) + } + + @Test("toggleRead flips read and stamps readModifiedAt") + func toggleReadStampsReadModifiedAt() { + let feed = FeedDB(postId: "1") + + feed.toggleRead() + #expect(feed.read == true) + #expect(feed.readModifiedAt != Date.distantPast) + + feed.toggleRead() + #expect(feed.read == false) + } + // MARK: - ModelDuplicable Tests @Test("deduplicate removes duplicate postIds keeping most recently modified") @@ -445,13 +489,13 @@ struct FeedDBTests { #expect(remaining.contains { $0.title == "Y-new" }) } - @Test("deduplicate keeps the favorited copy even when it is not the most recently modified") - func deduplicateKeepsFavoriteOverRecency() { + @Test("deduplicate keeps the most recently modified content even when a different duplicate wins the favorite merge") + func deduplicateDecouplesContentSurvivorFromFavoriteMerge() { let storage = Database(models: [FeedDB.self], inMemory: true) - let favorited = FeedDB(postId: "dup-1", title: "Favorited", favorite: true, modifiedAt: Date(timeIntervalSince1970: 1000)) + let originallyFavorited = FeedDB(postId: "dup-1", title: "Original", favorite: true, favoriteModifiedAt: Date(timeIntervalSince1970: 5000), modifiedAt: Date(timeIntervalSince1970: 1000)) let freshSync = FeedDB(postId: "dup-1", title: "Fresh Sync", favorite: false, modifiedAt: Date(timeIntervalSince1970: 2000)) - storage.context.insert(favorited) + storage.context.insert(originallyFavorited) storage.context.insert(freshSync) try? storage.context.save() @@ -459,29 +503,63 @@ struct FeedDBTests { let remaining = storage.fetch(FeedDB.self) #expect(remaining.count == 1) - #expect(remaining.first?.title == "Favorited") + #expect(remaining.first?.title == "Fresh Sync") #expect(remaining.first?.favorite == true) } + @Test("deduplicate respects an explicit cross-device unfavorite over an older favorite") + func deduplicateRespectsExplicitUnfavorite() { + let storage = Database(models: [FeedDB.self], inMemory: true) + let favoritedOnDeviceA = FeedDB(postId: "dup-1", favorite: true, favoriteModifiedAt: Date(timeIntervalSince1970: 1000), modifiedAt: Date(timeIntervalSince1970: 1000)) + let unfavoritedOnDeviceB = FeedDB(postId: "dup-1", favorite: false, favoriteModifiedAt: Date(timeIntervalSince1970: 2000), modifiedAt: Date(timeIntervalSince1970: 2000)) + + storage.context.insert(favoritedOnDeviceA) + storage.context.insert(unfavoritedOnDeviceB) + try? storage.context.save() + + FeedDB.deduplicate(using: storage.context) + + let remaining = storage.fetch(FeedDB.self) + #expect(remaining.count == 1) + #expect(remaining.first?.favorite == false) + } + @Test( - "deduplicate OR-merges favorite and read across every combination", + "deduplicate merges favorite and read using each field's own timestamp", arguments: [ - FavoriteReadCase(olderFavorite: true, olderRead: false, newerFavorite: false, newerRead: false, expectedFavorite: true, expectedRead: false), - FavoriteReadCase(olderFavorite: false, olderRead: true, newerFavorite: false, newerRead: false, expectedFavorite: false, expectedRead: true), - FavoriteReadCase(olderFavorite: true, olderRead: true, newerFavorite: false, newerRead: false, expectedFavorite: true, expectedRead: true), - FavoriteReadCase(olderFavorite: true, olderRead: true, newerFavorite: true, newerRead: false, expectedFavorite: true, expectedRead: true), - FavoriteReadCase(olderFavorite: false, olderRead: false, newerFavorite: false, newerRead: false, expectedFavorite: false, expectedRead: false) + FieldMergeCase( + aFavorite: true, aFavoriteModifiedAt: Date(timeIntervalSince1970: 1000), aRead: true, aReadModifiedAt: Date(timeIntervalSince1970: 1000), + bFavorite: false, bFavoriteModifiedAt: .distantPast, bRead: false, bReadModifiedAt: .distantPast, + expectedFavorite: true, expectedRead: true + ), + FieldMergeCase( + aFavorite: true, aFavoriteModifiedAt: Date(timeIntervalSince1970: 1000), aRead: false, aReadModifiedAt: Date(timeIntervalSince1970: 1000), + bFavorite: false, bFavoriteModifiedAt: Date(timeIntervalSince1970: 2000), bRead: true, bReadModifiedAt: Date(timeIntervalSince1970: 2000), + expectedFavorite: false, expectedRead: true + ), + FieldMergeCase( + aFavorite: true, aFavoriteModifiedAt: Date(timeIntervalSince1970: 2000), aRead: false, aReadModifiedAt: Date(timeIntervalSince1970: 1000), + bFavorite: false, bFavoriteModifiedAt: Date(timeIntervalSince1970: 1000), bRead: true, bReadModifiedAt: Date(timeIntervalSince1970: 2000), + expectedFavorite: true, expectedRead: true + ), + FieldMergeCase( + aFavorite: false, aFavoriteModifiedAt: .distantPast, aRead: false, aReadModifiedAt: .distantPast, + bFavorite: false, bFavoriteModifiedAt: .distantPast, bRead: false, bReadModifiedAt: .distantPast, + expectedFavorite: false, expectedRead: false + ) ] ) - fileprivate func deduplicateMergesFavoriteAndReadState(_ testCase: FavoriteReadCase) { + fileprivate func deduplicateMergesFieldsByOwnTimestamp(_ testCase: FieldMergeCase) { let storage = Database(models: [FeedDB.self], inMemory: true) - let older = FeedDB(postId: "dup-1", favorite: testCase.olderFavorite, modifiedAt: Date(timeIntervalSince1970: 1000)) - older.read = testCase.olderRead - let newer = FeedDB(postId: "dup-1", favorite: testCase.newerFavorite, modifiedAt: Date(timeIntervalSince1970: 2000)) - newer.read = testCase.newerRead - - storage.context.insert(older) - storage.context.insert(newer) + let recordA = FeedDB(postId: "dup-1", favorite: testCase.aFavorite, favoriteModifiedAt: testCase.aFavoriteModifiedAt, modifiedAt: Date(timeIntervalSince1970: 1000)) + recordA.read = testCase.aRead + recordA.readModifiedAt = testCase.aReadModifiedAt + let recordB = FeedDB(postId: "dup-1", favorite: testCase.bFavorite, favoriteModifiedAt: testCase.bFavoriteModifiedAt, modifiedAt: Date(timeIntervalSince1970: 2000)) + recordB.read = testCase.bRead + recordB.readModifiedAt = testCase.bReadModifiedAt + + storage.context.insert(recordA) + storage.context.insert(recordB) try? storage.context.save() FeedDB.deduplicate(using: storage.context) diff --git a/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift b/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift index 59f16cce..e37cf2d9 100644 --- a/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift +++ b/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift @@ -360,6 +360,33 @@ struct PodcastDBTests { #expect(remaining.isEmpty) } + @Test("toggleFavorite flips favorite and stamps favoriteModifiedAt without touching progress") + func toggleFavoriteStampsFavoriteModifiedAt() { + let podcast = PodcastDB(postId: "1", current: 900) + let progressModifiedAtBefore = podcast.progressModifiedAt + + podcast.toggleFavorite() + + #expect(podcast.favorite == true) + #expect(podcast.favoriteModifiedAt != Date.distantPast) + #expect(podcast.progressModifiedAt == progressModifiedAtBefore) + + podcast.toggleFavorite() + #expect(podcast.favorite == false) + } + + @Test("updateProgress sets current and stamps progressModifiedAt without touching favorite") + func updateProgressStampsProgressModifiedAt() { + let podcast = PodcastDB(postId: "1", favorite: true) + let favoriteModifiedAtBefore = podcast.favoriteModifiedAt + + podcast.updateProgress(450) + + #expect(podcast.current == 450) + #expect(podcast.progressModifiedAt != Date.distantPast) + #expect(podcast.favoriteModifiedAt == favoriteModifiedAtBefore) + } + // MARK: - ModelDuplicable Tests @Test("deduplicate removes duplicate postIds keeping most recently modified") @@ -411,13 +438,13 @@ struct PodcastDBTests { #expect(remaining.first?.title == "Winner") } - @Test("deduplicate keeps the favorited copy even when it is not the most recently modified") - func deduplicateKeepsFavoriteOverRecency() { + @Test("deduplicate keeps the most recently modified content even when a different duplicate wins the favorite merge") + func deduplicateDecouplesContentSurvivorFromFavoriteMerge() { let storage = Database(models: [PodcastDB.self], inMemory: true) - let favorited = PodcastDB(postId: "dup-1", title: "Favorited", favorite: true, modifiedAt: Date(timeIntervalSince1970: 1000)) + let originallyFavorited = PodcastDB(postId: "dup-1", title: "Original", favorite: true, favoriteModifiedAt: Date(timeIntervalSince1970: 5000), modifiedAt: Date(timeIntervalSince1970: 1000)) let freshSync = PodcastDB(postId: "dup-1", title: "Fresh Sync", favorite: false, modifiedAt: Date(timeIntervalSince1970: 2000)) - storage.context.insert(favorited) + storage.context.insert(originallyFavorited) storage.context.insert(freshSync) try? storage.context.save() @@ -425,17 +452,34 @@ struct PodcastDBTests { let remaining = storage.fetch(PodcastDB.self) #expect(remaining.count == 1) - #expect(remaining.first?.title == "Favorited") + #expect(remaining.first?.title == "Fresh Sync") #expect(remaining.first?.favorite == true) } - @Test("deduplicate keeps the furthest playback progress across duplicates") - func deduplicateKeepsFurthestPlaybackProgress() { + @Test("deduplicate respects an explicit cross-device unfavorite over an older favorite") + func deduplicateRespectsExplicitUnfavorite() { let storage = Database(models: [PodcastDB.self], inMemory: true) - let listenedFurther = PodcastDB(postId: "dup-1", current: 900, modifiedAt: Date(timeIntervalSince1970: 1000)) + let favoritedOnDeviceA = PodcastDB(postId: "dup-1", favorite: true, favoriteModifiedAt: Date(timeIntervalSince1970: 1000), modifiedAt: Date(timeIntervalSince1970: 1000)) + let unfavoritedOnDeviceB = PodcastDB(postId: "dup-1", favorite: false, favoriteModifiedAt: Date(timeIntervalSince1970: 2000), modifiedAt: Date(timeIntervalSince1970: 2000)) + + storage.context.insert(favoritedOnDeviceA) + storage.context.insert(unfavoritedOnDeviceB) + try? storage.context.save() + + PodcastDB.deduplicate(using: storage.context) + + let remaining = storage.fetch(PodcastDB.self) + #expect(remaining.count == 1) + #expect(remaining.first?.favorite == false) + } + + @Test("deduplicate never lets a blank sync duplicate override real playback progress") + func deduplicatePreservesProgressAgainstBlankDuplicate() { + let storage = Database(models: [PodcastDB.self], inMemory: true) + let listenedTo = PodcastDB(postId: "dup-1", current: 900, progressModifiedAt: Date(timeIntervalSince1970: 1000), modifiedAt: Date(timeIntervalSince1970: 1000)) let freshSync = PodcastDB(postId: "dup-1", current: 0, modifiedAt: Date(timeIntervalSince1970: 2000)) - storage.context.insert(listenedFurther) + storage.context.insert(listenedTo) storage.context.insert(freshSync) try? storage.context.save() @@ -446,32 +490,37 @@ struct PodcastDBTests { #expect(remaining.first?.current == 900) } - @Test("deduplicate keeps furthest progress even when it belongs to the non-favorited duplicate") - func deduplicateMergesProgressAcrossFavoriteWinner() { + @Test("deduplicate honors a deliberate rewind even though it lowers the playback position") + func deduplicateHonorsDeliberateRewind() { let storage = Database(models: [PodcastDB.self], inMemory: true) - let favoritedButNotListened = PodcastDB(postId: "dup-1", favorite: true, current: 0, modifiedAt: Date(timeIntervalSince1970: 1000)) - let listenedButNotFavorited = PodcastDB(postId: "dup-1", favorite: false, current: 900, modifiedAt: Date(timeIntervalSince1970: 2000)) + let listenedFurtherButStale = PodcastDB(postId: "dup-1", current: 900, progressModifiedAt: Date(timeIntervalSince1970: 1000), modifiedAt: Date(timeIntervalSince1970: 1000)) + let rewoundOnAnotherDevice = PodcastDB(postId: "dup-1", current: 100, progressModifiedAt: Date(timeIntervalSince1970: 2000), modifiedAt: Date(timeIntervalSince1970: 2000)) - storage.context.insert(favoritedButNotListened) - storage.context.insert(listenedButNotFavorited) + storage.context.insert(listenedFurtherButStale) + storage.context.insert(rewoundOnAnotherDevice) try? storage.context.save() PodcastDB.deduplicate(using: storage.context) let remaining = storage.fetch(PodcastDB.self) #expect(remaining.count == 1) - #expect(remaining.first?.favorite == true) - #expect(remaining.first?.current == 900) + #expect(remaining.first?.current == 100) } - @Test("deduplicate keeps the furthest progress when both duplicates are favorited") - func deduplicateMergesProgressWhenBothFavorited() { + @Test("deduplicate merges favorite and progress independently by their own timestamps") + func deduplicateMergesFavoriteAndProgressIndependently() { let storage = Database(models: [PodcastDB.self], inMemory: true) - let olderFavorited = PodcastDB(postId: "dup-1", favorite: true, current: 900, modifiedAt: Date(timeIntervalSince1970: 1000)) - let newerFavorited = PodcastDB(postId: "dup-1", favorite: true, current: 0, modifiedAt: Date(timeIntervalSince1970: 2000)) + let favoritedButNotListened = PodcastDB( + postId: "dup-1", favorite: true, favoriteModifiedAt: Date(timeIntervalSince1970: 2000), + current: 0, modifiedAt: Date(timeIntervalSince1970: 1000) + ) + let listenedButNotFavorited = PodcastDB( + postId: "dup-1", favorite: false, favoriteModifiedAt: Date(timeIntervalSince1970: 1000), + current: 900, progressModifiedAt: Date(timeIntervalSince1970: 2000), modifiedAt: Date(timeIntervalSince1970: 2000) + ) - storage.context.insert(olderFavorited) - storage.context.insert(newerFavorited) + storage.context.insert(favoritedButNotListened) + storage.context.insert(listenedButNotFavorited) try? storage.context.save() PodcastDB.deduplicate(using: storage.context) diff --git a/MacMagazine/Features/MacMagazineLibrary/Sources/MacMagazineLibrary/ModelProtocols.swift b/MacMagazine/Features/MacMagazineLibrary/Sources/MacMagazineLibrary/ModelProtocols.swift index 9f6f1076..2a821c1f 100644 --- a/MacMagazine/Features/MacMagazineLibrary/Sources/MacMagazineLibrary/ModelProtocols.swift +++ b/MacMagazine/Features/MacMagazineLibrary/Sources/MacMagazineLibrary/ModelProtocols.swift @@ -13,14 +13,15 @@ public protocol ModelDuplicable: AnyObject, PersistentModel { static func deduplicate(using context: ModelContext?) } -public protocol ModelPrioritizable { - var favorite: Bool { get } - var modifiedAt: Date { get } -} +public protocol ModelPrioritizable {} public extension ModelPrioritizable { - static func isLessAuthoritative(_ lhs: Self, _ rhs: Self) -> Bool { - guard lhs.favorite == rhs.favorite else { return rhs.favorite } - return lhs.modifiedAt < rhs.modifiedAt + static func latest( + in group: [Self], + value: (Self) -> Value, + modifiedAt: (Self) -> Date + ) -> (value: Value, modifiedAt: Date)? { + guard let winner = group.max(by: { modifiedAt($0) < modifiedAt($1) }) else { return nil } + return (value(winner), modifiedAt(winner)) } } diff --git a/MacMagazine/Features/NewsLibrary/Sources/NewsLibrary/Extensions/FeedDBExtensions.swift b/MacMagazine/Features/NewsLibrary/Sources/NewsLibrary/Extensions/FeedDBExtensions.swift index 12805507..798084a7 100644 --- a/MacMagazine/Features/NewsLibrary/Sources/NewsLibrary/Extensions/FeedDBExtensions.swift +++ b/MacMagazine/Features/NewsLibrary/Sources/NewsLibrary/Extensions/FeedDBExtensions.swift @@ -31,8 +31,7 @@ public extension FeedDB { aspectRatio: aspectRatio, favoriteAction: { [weak self] in guard let self, let context else { return } - self.favorite.toggle() - self.modifiedAt = Date() + self.toggleFavorite() try? context.save() analytics?.track(.buttonTap( buttonId: AnalyticsConstants.ButtonID.newsFavorite.id, @@ -41,8 +40,7 @@ public extension FeedDB { }, readAction: { [weak self] in guard let self, let context else { return } - self.read.toggle() - self.modifiedAt = Date() + self.toggleRead() try? context.save() } ) diff --git a/MacMagazine/Features/PodcastLibrary/Sources/Podcast/Extensions/PodcastDBExtensions.swift b/MacMagazine/Features/PodcastLibrary/Sources/Podcast/Extensions/PodcastDBExtensions.swift index 78fc901e..fb60be6d 100644 --- a/MacMagazine/Features/PodcastLibrary/Sources/Podcast/Extensions/PodcastDBExtensions.swift +++ b/MacMagazine/Features/PodcastLibrary/Sources/Podcast/Extensions/PodcastDBExtensions.swift @@ -22,8 +22,7 @@ public extension PodcastDB { favorite: self.favorite, favoriteAction: { [weak self] in guard let self, let context else { return } - self.favorite.toggle() - self.modifiedAt = Date() + self.toggleFavorite() try? context.save() analytics?.track(.buttonTap( buttonId: AnalyticsConstants.ButtonID.podcastFavorite.id, @@ -35,8 +34,7 @@ public extension PodcastDB { func save(current: Double, using context: ModelContext?) { guard let context else { return } - self.current = current - self.modifiedAt = Date() + updateProgress(current) try? context.save() } } From 8d0c7872b1dda3392c6d8a463cf0ddac959c566a Mon Sep 17 00:00:00 2001 From: Cassio Rossi Date: Thu, 30 Jul 2026 00:41:06 +0100 Subject: [PATCH 5/5] =?UTF-8?q?fix(#304):=20address=20code=20review=20?= =?UTF-8?q?=E2=80=94=20bypassed=20call=20sites,=20tie-break,=20dedup,=20do?= =?UTF-8?q?cs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - NewsView's article .task and DeepLinkNewsDetailView's onAppear/favorite action mutated read/favorite/modifiedAt directly instead of calling markAsRead()/toggleFavorite(), leaving readModifiedAt/favoriteModifiedAt stale - the exact class of bug this PR fixes, reachable through the primary reading flow and push-notification deep links. - Fixed FeedDB.init's `ead: Bool` parameter typo (missing leading r): self.read = read was silently self-assigning since no `read` parameter existed, so read could never be set to true through the initializer. - ModelPrioritizable.latest() tie-broke on an exact timestamp match using Array.max(by:)'s last-checked-wins behavior, which depends on Dictionary(grouping:) iteration order - unspecified by SwiftData. It now takes an explicit preferOnTie closure and resolves ties independent of input order (favorite/read prefer true; podcast progress prefers the larger value). - Extracted the shared fetch/group/survivor/delete skeleton into ModelPrioritizable.resolveDuplicates(), which FeedDB and PodcastDB now both call - the per-field merge logic was the only genuinely type-specific part. - Added DocC comments to the new public API per CLAUDE.md's comment policy. Co-Authored-By: Claude --- .../Sources/FeedLibrary/Database/FeedDB.swift | 28 +++++++------ .../FeedLibrary/Database/PodcastDB.swift | 24 +++++++----- .../Tests/FeedLibraryTests/FeedDBTests.swift | 27 +++++++++++++ .../FeedLibraryTests/PodcastDBTests.swift | 21 ++++++++++ .../MacMagazineLibrary/ModelProtocols.swift | 39 +++++++++++++++++-- .../Sources/NewsLibrary/Views/NewsView.swift | 3 +- .../News/DeepLinkNewsDetailView.swift | 6 +-- 7 files changed, 118 insertions(+), 30 deletions(-) diff --git a/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/FeedDB.swift b/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/FeedDB.swift index 845ea3f4..6b5226a3 100644 --- a/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/FeedDB.swift +++ b/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/FeedDB.swift @@ -34,7 +34,7 @@ public final class FeedDB { fullContent: String = "", favorite: Bool = false, favoriteModifiedAt: Date = Date.distantPast, - ead: Bool = false, + read: Bool = false, readModifiedAt: Date = Date.distantPast, modifiedAt: Date = Date() ) { @@ -82,6 +82,8 @@ extension FeedDB: ModelFavoritable { try? context.save() } + /// Flips `favorite` and stamps `favoriteModifiedAt`, so `deduplicate()` can tell this + /// explicit action apart from a blank sync-created duplicate. public func toggleFavorite() { favorite.toggle() favoriteModifiedAt = Date() @@ -100,12 +102,16 @@ extension FeedDB: ModelReadable { try? context.save() } + /// Flips `read` and stamps `readModifiedAt`, so `deduplicate()` can tell this explicit + /// action apart from a blank sync-created duplicate. public func toggleRead() { read.toggle() readModifiedAt = Date() modifiedAt = Date() } + /// Sets `read` to `true` and stamps `readModifiedAt`, so `deduplicate()` can tell this + /// explicit action apart from a blank sync-created duplicate. public func markAsRead() { read = true readModifiedAt = Date() @@ -121,22 +127,22 @@ extension FeedDB: ModelDuplicable { guard let context, let data = try? context.fetch(descriptor) else { return } - for group in Dictionary(grouping: data, by: \.postId).values where group.count > 1 { - guard let survivor = group.max(by: { $0.modifiedAt < $1.modifiedAt }) else { continue } - - if let latestFavorite = FeedDB.latest(in: group, value: { $0.favorite }, modifiedAt: { $0.favoriteModifiedAt }) { + FeedDB.resolveDuplicates(in: data, postId: \.postId, modifiedAt: \.modifiedAt) { survivor, group in + if let latestFavorite = FeedDB.latest( + in: group, value: { $0.favorite }, modifiedAt: { $0.favoriteModifiedAt }, + preferOnTie: { candidate, _ in candidate } + ) { survivor.favorite = latestFavorite.value survivor.favoriteModifiedAt = latestFavorite.modifiedAt } - if let latestRead = FeedDB.latest(in: group, value: { $0.read }, modifiedAt: { $0.readModifiedAt }) { + if let latestRead = FeedDB.latest( + in: group, value: { $0.read }, modifiedAt: { $0.readModifiedAt }, + preferOnTie: { candidate, _ in candidate } + ) { survivor.read = latestRead.value survivor.readModifiedAt = latestRead.modifiedAt } - - for record in group where record !== survivor { - context.delete(record) - } - } + } delete: { context.delete($0) } try? context.save() } diff --git a/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/PodcastDB.swift b/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/PodcastDB.swift index 09272d9b..1121ce49 100644 --- a/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/PodcastDB.swift +++ b/MacMagazine/Features/FeedLibrary/Sources/FeedLibrary/Database/PodcastDB.swift @@ -67,6 +67,8 @@ extension PodcastDB: ModelFavoritable { try? context.save() } + /// Flips `favorite` and stamps `favoriteModifiedAt`, so `deduplicate()` can tell this + /// explicit action apart from a blank sync-created duplicate. public func toggleFavorite() { favorite.toggle() favoriteModifiedAt = Date() @@ -75,6 +77,8 @@ extension PodcastDB: ModelFavoritable { } extension PodcastDB { + /// Sets `current` and stamps `progressModifiedAt`, so `deduplicate()` can tell this + /// explicit action apart from a blank sync-created duplicate. public func updateProgress(_ current: Double) { self.current = current progressModifiedAt = Date() @@ -90,22 +94,22 @@ extension PodcastDB: ModelDuplicable { guard let context, let data = try? context.fetch(descriptor) else { return } - for group in Dictionary(grouping: data, by: \.postId).values where group.count > 1 { - guard let survivor = group.max(by: { $0.modifiedAt < $1.modifiedAt }) else { continue } - - if let latestFavorite = PodcastDB.latest(in: group, value: { $0.favorite }, modifiedAt: { $0.favoriteModifiedAt }) { + PodcastDB.resolveDuplicates(in: data, postId: \.postId, modifiedAt: \.modifiedAt) { survivor, group in + if let latestFavorite = PodcastDB.latest( + in: group, value: { $0.favorite }, modifiedAt: { $0.favoriteModifiedAt }, + preferOnTie: { candidate, _ in candidate } + ) { survivor.favorite = latestFavorite.value survivor.favoriteModifiedAt = latestFavorite.modifiedAt } - if let latestProgress = PodcastDB.latest(in: group, value: { $0.current }, modifiedAt: { $0.progressModifiedAt }) { + if let latestProgress = PodcastDB.latest( + in: group, value: { $0.current }, modifiedAt: { $0.progressModifiedAt }, + preferOnTie: { candidate, current in candidate > current } + ) { survivor.current = latestProgress.value survivor.progressModifiedAt = latestProgress.modifiedAt } - - for record in group where record !== survivor { - context.delete(record) - } - } + } delete: { context.delete($0) } try? context.save() } diff --git a/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/FeedDBTests.swift b/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/FeedDBTests.swift index 02acaab6..14e4457c 100644 --- a/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/FeedDBTests.swift +++ b/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/FeedDBTests.swift @@ -64,6 +64,12 @@ struct FeedDBTests { #expect(feed.favorite == favorite) } + @Test("FeedDB initializer's read parameter actually sets the read property") + func initializationSetsReadFromParameter() { + let feed = FeedDB(postId: "1", read: true) + #expect(feed.read == true) + } + @Test("FeedDB should initialize with default values") func initializationWithDefaults() { // When @@ -570,6 +576,27 @@ struct FeedDBTests { #expect(remaining.first?.read == testCase.expectedRead) } + @Test("deduplicate breaks an exact favoriteModifiedAt tie by preferring true, regardless of insertion order") + func deduplicateBreaksFavoriteTieDeterministically() { + let insertionOrders: [(first: Bool, second: Bool)] = [(true, false), (false, true)] + + for order in insertionOrders { + let storage = Database(models: [FeedDB.self], inMemory: true) + let first = FeedDB(postId: "dup-1", favorite: order.first, modifiedAt: Date(timeIntervalSince1970: 1000)) + let second = FeedDB(postId: "dup-1", favorite: order.second, modifiedAt: Date(timeIntervalSince1970: 1000)) + + storage.context.insert(first) + storage.context.insert(second) + try? storage.context.save() + + FeedDB.deduplicate(using: storage.context) + + let remaining = storage.fetch(FeedDB.self) + #expect(remaining.count == 1) + #expect(remaining.first?.favorite == true) + } + } + @Test("deduplicate is safe on empty database") func deduplicateSafeOnEmpty() { let storage = Database(models: [FeedDB.self], inMemory: true) diff --git a/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift b/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift index e37cf2d9..f7ee18e8 100644 --- a/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift +++ b/MacMagazine/Features/FeedLibrary/Tests/FeedLibraryTests/PodcastDBTests.swift @@ -507,6 +507,27 @@ struct PodcastDBTests { #expect(remaining.first?.current == 100) } + @Test("deduplicate breaks an exact progressModifiedAt tie by preferring the larger value, regardless of insertion order") + func deduplicateBreaksProgressTieDeterministically() { + let insertionOrders: [(first: Double, second: Double)] = [(900, 100), (100, 900)] + + for order in insertionOrders { + let storage = Database(models: [PodcastDB.self], inMemory: true) + let first = PodcastDB(postId: "dup-1", current: order.first, modifiedAt: Date(timeIntervalSince1970: 1000)) + let second = PodcastDB(postId: "dup-1", current: order.second, modifiedAt: Date(timeIntervalSince1970: 1000)) + + storage.context.insert(first) + storage.context.insert(second) + try? storage.context.save() + + PodcastDB.deduplicate(using: storage.context) + + let remaining = storage.fetch(PodcastDB.self) + #expect(remaining.count == 1) + #expect(remaining.first?.current == 900) + } + } + @Test("deduplicate merges favorite and progress independently by their own timestamps") func deduplicateMergesFavoriteAndProgressIndependently() { let storage = Database(models: [PodcastDB.self], inMemory: true) diff --git a/MacMagazine/Features/MacMagazineLibrary/Sources/MacMagazineLibrary/ModelProtocols.swift b/MacMagazine/Features/MacMagazineLibrary/Sources/MacMagazineLibrary/ModelProtocols.swift index 2a821c1f..8b218488 100644 --- a/MacMagazine/Features/MacMagazineLibrary/Sources/MacMagazineLibrary/ModelProtocols.swift +++ b/MacMagazine/Features/MacMagazineLibrary/Sources/MacMagazineLibrary/ModelProtocols.swift @@ -13,15 +13,48 @@ public protocol ModelDuplicable: AnyObject, PersistentModel { static func deduplicate(using context: ModelContext?) } -public protocol ModelPrioritizable {} +public protocol ModelPrioritizable: AnyObject {} public extension ModelPrioritizable { + /// Picks the most authoritative value of a field across a group of duplicates, by the + /// field's own timestamp. `preferOnTie` breaks an exact timestamp tie deterministically - + /// independent of the group's iteration order, which SwiftData does not guarantee stable - + /// by returning `true` when `candidate` should replace `current`. static func latest( in group: [Self], value: (Self) -> Value, - modifiedAt: (Self) -> Date + modifiedAt: (Self) -> Date, + preferOnTie: (_ candidate: Value, _ current: Value) -> Bool ) -> (value: Value, modifiedAt: Date)? { - guard let winner = group.max(by: { modifiedAt($0) < modifiedAt($1) }) else { return nil } + guard var winner = group.first else { return nil } + for candidate in group.dropFirst() { + let winnerDate = modifiedAt(winner) + let candidateDate = modifiedAt(candidate) + if candidateDate > winnerDate { + winner = candidate + } else if candidateDate == winnerDate, preferOnTie(value(candidate), value(winner)) { + winner = candidate + } + } return (value(winner), modifiedAt(winner)) } + + /// Groups `data` by `postId`, keeps the most recently modified record per group as the + /// survivor, lets `merge` reconcile per-field state onto it, then hands every other record + /// in the group to `delete`. + static func resolveDuplicates( + in data: [Self], + postId: (Self) -> String, + modifiedAt: (Self) -> Date, + merge: (_ survivor: Self, _ group: [Self]) -> Void, + delete: (Self) -> Void + ) { + for group in Dictionary(grouping: data, by: postId).values where group.count > 1 { + guard let survivor = group.max(by: { modifiedAt($0) < modifiedAt($1) }) else { continue } + merge(survivor, group) + for record in group where record !== survivor { + delete(record) + } + } + } } diff --git a/MacMagazine/Features/NewsLibrary/Sources/NewsLibrary/Views/NewsView.swift b/MacMagazine/Features/NewsLibrary/Sources/NewsLibrary/Views/NewsView.swift index ff0d0690..da33c595 100644 --- a/MacMagazine/Features/NewsLibrary/Sources/NewsLibrary/Views/NewsView.swift +++ b/MacMagazine/Features/NewsLibrary/Sources/NewsLibrary/Views/NewsView.swift @@ -230,8 +230,7 @@ extension NewsView { .sharedBackgroundVisibility(.hidden) } .task { - viewModel.selectedNews?.read = true - viewModel.selectedNews?.modifiedAt = Date() + viewModel.selectedNews?.markAsRead() try? modelContext.save() } } diff --git a/MacMagazine/MacMagazine/Features/News/DeepLinkNewsDetailView.swift b/MacMagazine/MacMagazine/Features/News/DeepLinkNewsDetailView.swift index 01f246b2..c997dfba 100644 --- a/MacMagazine/MacMagazine/Features/News/DeepLinkNewsDetailView.swift +++ b/MacMagazine/MacMagazine/Features/News/DeepLinkNewsDetailView.swift @@ -44,8 +44,7 @@ struct DeepLinkNewsDetailView: View { .sharedBackgroundVisibility(.hidden) } .onAppear { - post?.read = true - post?.modifiedAt = Date() + post?.markAsRead() try? modelContext.save() } } @@ -57,8 +56,7 @@ struct DeepLinkNewsDetailView: View { FavoriteButton( favorite: post.favorite, action: { - post.favorite.toggle() - post.modifiedAt = Date() + post.toggleFavorite() try? modelContext.save() analytics.track(.buttonTap( buttonId: AnalyticsConstants.ButtonID.newsFavorite.id,