diff --git a/Yattee/Models/RemotePlaylistEntry.swift b/Yattee/Models/RemotePlaylistEntry.swift index 60a58475..928337f2 100644 --- a/Yattee/Models/RemotePlaylistEntry.swift +++ b/Yattee/Models/RemotePlaylistEntry.swift @@ -21,7 +21,9 @@ import Foundation /// - ``invidious(playlistID:indexID:)`` wraps the server's `indexId` token, /// which names the same row across refetches even as videos above it are /// removed. -/// - ``piped(playlistID:position:)`` is **snapshot-scoped only**. Piped +/// - ``piped(playlistID:position:)`` is **snapshot-scoped only**, and the +/// position is the server's own index over every row it sent — not the row +/// number the app draws. Piped /// removes by position, so an id minted from one fetch is meaningless once /// the playlist changes. This is safe because the caching layer refetches /// the whole playlist after every mutation — never persist these, and never @@ -47,13 +49,26 @@ struct RemotePlaylistEntry: Identifiable, Hashable, Sendable { /// The video shown for this row. let video: Video - /// Zero-based position within the playlist as the snapshot saw it. + /// Zero-based position within the playlist **as the server counts it** — + /// over every row it sent, including the ones the app could not parse and + /// does not display. This is the number a removal is addressed by, so it + /// must never be renumbered over the filtered display list. let position: Int - init(id: RemotePlaylistEntryID, video: Video, position: Int) { + /// Index of this entry in ``RemotePlaylistSnapshot/playlist``'s `videos`. + /// + /// Separate from ``position`` on purpose: rows the app cannot display are + /// dropped from `playlist.videos` but still occupy a server position, so the + /// two numbers diverge the moment a playlist holds one. Carried explicitly + /// rather than recovered by matching video ids, which cannot tell two rows + /// of the same video apart. + let displayRow: Int + + init(id: RemotePlaylistEntryID, video: Video, position: Int, displayRow: Int) { self.id = id self.video = video self.position = position + self.displayRow = displayRow } } @@ -61,6 +76,10 @@ struct RemotePlaylistEntry: Identifiable, Hashable, Sendable { /// /// `entries` may be shorter than `playlist.videos`: a video the server did not /// hand an identity to is still displayed, it just cannot be a mutation target. +/// It can also be shorter than what the server sent: a row that failed to parse +/// is neither displayed nor addressable, but it still holds a server position, +/// which is why ``RemotePlaylistEntry/position`` and +/// ``RemotePlaylistEntry/displayRow`` are two different numbers. struct RemotePlaylistSnapshot: Hashable, Sendable { /// The playlist for display. let playlist: Playlist diff --git a/Yattee/Services/API/InvidiousAPI.swift b/Yattee/Services/API/InvidiousAPI.swift index 382408d5..a8d60580 100644 --- a/Yattee/Services/API/InvidiousAPI.swift +++ b/Yattee/Services/API/InvidiousAPI.swift @@ -327,16 +327,27 @@ actor InvidiousAPI: InstanceAPI { // and `index` places it. Rows missing either are still displayed — // they stay in `playlist.videos` — they just never appear in `entries`, // so no removal can be aimed at a row the server refuses to name. - let entries: [RemotePlaylistEntry] = allVideos.compactMap { item in - guard case .video(let video) = item, - let indexID = video.indexId, - let index = video.index - else { return nil } + // + // `displayRow` is counted over exactly what `toPlaylist` keeps, so an + // entry says which visible row it belongs to without anyone having to + // match video ids — which could not tell two rows of the same video + // apart anyway. + var displayRow = 0 + var entries: [RemotePlaylistEntry] = [] + for item in allVideos { + guard case .video(let video) = item else { continue } + let row = displayRow + displayRow += 1 - return RemotePlaylistEntry( - id: .invidious(playlistID: playlistID, indexID: indexID), - video: video.toVideo(baseURL: baseURL), - position: index + guard let indexID = video.indexId, let index = video.index else { continue } + + entries.append( + RemotePlaylistEntry( + id: .invidious(playlistID: playlistID, indexID: indexID), + video: video.toVideo(baseURL: baseURL), + position: index, + displayRow: row + ) ) } diff --git a/Yattee/Services/API/PipedAPI.swift b/Yattee/Services/API/PipedAPI.swift index f491281b..da6ddcd3 100644 --- a/Yattee/Services/API/PipedAPI.swift +++ b/Yattee/Services/API/PipedAPI.swift @@ -444,16 +444,26 @@ actor PipedAPI: InstanceAPI { nextpage: nil ).toPlaylist(instanceURL: instanceURL, playlistID: playlistID) - // Positions run 0-based across every page in fetch order and are taken - // over the same list the playlist displays, so `entries[n]` and - // `playlist.videos[n]` are always the same row. Piped removes by + // Positions run 0-based across every page in fetch order and are counted + // over the *raw* stream list, malformed rows included: that is the index + // `POST /user/playlists/remove` addresses, and renumbering over the + // filtered display list would aim a removal one row off for every row + // the app could not parse. `displayRow` is the separate number that says + // where the entry appears in `playlist.videos`. Piped removes by // position, so these ids die with this snapshot. - let entries = playlist.videos.enumerated().map { offset, video in - RemotePlaylistEntry( - id: .piped(playlistID: playlistID, position: offset), - video: video, - position: offset + var displayRow = 0 + var entries: [RemotePlaylistEntry] = [] + for (position, item) in allStreams.enumerated() { + guard case .video(let video) = item else { continue } + entries.append( + RemotePlaylistEntry( + id: .piped(playlistID: playlistID, position: position), + video: video.toVideo(instanceURL: instanceURL), + position: position, + displayRow: displayRow + ) ) + displayRow += 1 } return RemotePlaylistSnapshot(playlist: playlist, entries: entries) diff --git a/Yattee/Services/AccountService.swift b/Yattee/Services/AccountService.swift index 7c23e216..c1f0112a 100644 --- a/Yattee/Services/AccountService.swift +++ b/Yattee/Services/AccountService.swift @@ -146,6 +146,7 @@ final class AccountService { let (instance, credential) = try resolve(account, requiring: \.supportsPlaylistWrites) let privacy = supportedPrivacy(privacy, for: account) + try requireStillCurrent(credential, for: account) return try await tracking(account, credential: credential) { try await transport.createPlaylist( title: title, @@ -168,6 +169,7 @@ final class AccountService { let (instance, credential) = try resolve(account, requiring: \.supportsPlaylistWrites) let privacy = supportedPrivacy(privacy, for: account) + try requireStillCurrent(credential, for: account) try await tracking(account, credential: credential) { try await transport.updatePlaylist( id: id, @@ -184,6 +186,7 @@ final class AccountService { func deletePlaylist(id: String, for account: Account) async throws { let (instance, credential) = try resolve(account, requiring: \.supportsPlaylistWrites) + try requireStillCurrent(credential, for: account) try await tracking(account, credential: credential) { try await transport.deletePlaylist(id: id, instance: instance, credential: credential) } @@ -193,6 +196,7 @@ final class AccountService { func addVideo(videoID: String, toPlaylist id: String, for account: Account) async throws { let (instance, credential) = try resolve(account, requiring: \.supportsPlaylistWrites) + try requireStillCurrent(credential, for: account) try await tracking(account, credential: credential) { try await transport.addVideoToPlaylist( playlistID: id, @@ -212,6 +216,10 @@ final class AccountService { /// and the removal the playlist can still change, and only the caller's /// refetch afterwards can show what actually happened. Invidious needs none /// of this: its `indexId` names the row itself, whatever moved around it. + /// + /// The preflight is also where the credential can change out from under the + /// removal — it is the one place here that awaits a server before writing — + /// so the credential is read again before the removal is sent. func removeEntry( _ entry: RemotePlaylistEntry, fromPlaylist id: String, @@ -237,6 +245,7 @@ final class AccountService { } } + try requireStillCurrent(credential, for: account) try await tracking(account, credential: credential) { try await transport.removeEntry(entry.id, instance: instance, credential: credential) } @@ -274,6 +283,20 @@ final class AccountService { return (instance, credential) } + /// Refuses to send a write with a credential that is no longer the + /// account's. + /// + /// Resolving a credential and sending the request are not the same instant: + /// a sign-out and a sign-in as somebody else can both happen in between — + /// Piped's removals in particular await a whole playlist fetch first. A + /// stale *read* is merely wasted; a stale *write* changes the previous + /// user's data, so every mutation checks again immediately before it goes. + private func requireStillCurrent(_ credential: String, for account: Account) throws { + guard credentialReader(account) == credential else { + throw AccountServiceError.notAuthenticated + } + } + /// The privacy to send, or `nil` where the backend has no such setting. private func supportedPrivacy(_ privacy: PlaylistPrivacy?, for account: Account) -> PlaylistPrivacy? { guard accountsManager.capabilities(for: account)?.supportsPlaylistPrivacy == true else { return nil } diff --git a/Yattee/Services/Handoff/HandoffManager.swift b/Yattee/Services/Handoff/HandoffManager.swift index 4c871b91..f19cec82 100644 --- a/Yattee/Services/Handoff/HandoffManager.swift +++ b/Yattee/Services/Handoff/HandoffManager.swift @@ -32,6 +32,7 @@ final class HandoffManager { static let channelSource = "channelSource" static let playlistID = "playlistID" static let playlistSource = "playlistSource" + static let playlistAccountID = "playlistAccountID" static let localPlaylistUUID = "localPlaylistUUID" static let searchQuery = "searchQuery" static let playbackTime = "playbackTime" @@ -164,18 +165,23 @@ final class HandoffManager { userInfo[UserInfoKey.destinationType] = DestinationType.localPlaylist.rawValue userInfo[UserInfoKey.localPlaylistUUID] = uuid.uuidString activity.title = "Playlist" - // The owning account is deliberately left out of the payload. The - // userInfo is assembled key by key rather than encoded from the - // enum, so an extra associated value changes nothing about the - // coding shape either way — but an account is only usable on a - // device that holds its credential, and the receiving device - // resolves ownership for itself from the instance it lands on. - case .remote(let playlistID, _, _, _): + // The owning account travels with the playlist. Account ids are the + // same on every device the login is synced to, so the receiver can + // resolve one — and without it a Piped playlist arrives as nobody's: + // its id carries no hint of ownership the way an Invidious `IVPL` + // one does, so a private playlist would be loaded through the public + // endpoint and fail. A receiver that does not know the account is no + // worse off than before: the key is simply absent, which is also + // what an older payload looks like. + case .remote(let playlistID, _, _, let accountID): userInfo[UserInfoKey.destinationType] = DestinationType.playlist.rawValue userInfo[UserInfoKey.playlistID] = playlistID.playlistID if let contentSource = playlistID.source { userInfo[UserInfoKey.playlistSource] = encodeSource(contentSource) } + if let accountID { + userInfo[UserInfoKey.playlistAccountID] = accountID.uuidString + } activity.title = "Playlist" } @@ -312,7 +318,20 @@ final class HandoffManager { } else { source = nil } - return (.playlist(.remote(PlaylistID(source: source, playlistID: playlistID), instance: nil)), nil) + // An account this device does not have resolves to nil in the + // detail view, which falls back to the ownership it can infer — + // exactly what a payload without the key does. + let accountID = (userInfo[UserInfoKey.playlistAccountID] as? String).flatMap(UUID.init(uuidString:)) + return ( + .playlist( + .remote( + PlaylistID(source: source, playlistID: playlistID), + instance: nil, + accountID: accountID + ) + ), + nil + ) case .localPlaylist: guard let uuidString = userInfo[UserInfoKey.localPlaylistUUID] as? String, diff --git a/Yattee/Services/RemotePlaylistsCache.swift b/Yattee/Services/RemotePlaylistsCache.swift index 95fd5375..b6cfade6 100644 --- a/Yattee/Services/RemotePlaylistsCache.swift +++ b/Yattee/Services/RemotePlaylistsCache.swift @@ -163,8 +163,9 @@ final class RemotePlaylistsCache { } catch { // The delete may have landed even though the answer did not, so the // snapshot is refetched rather than assumed: whichever way it went, - // the server's answer replaces it. - await reconcileSnapshot(id: id, for: account) + // the server's answer replaces it — and a server that no longer has + // the playlist is answering that the delete did land. + await reconcileSnapshotAfterDelete(id: id, for: account) await reconcileList(for: account) throw error } @@ -334,11 +335,50 @@ final class RemotePlaylistsCache { try? await fetchSnapshot(id: id, for: account) } + /// The same refetch, after a delete that reported failure — where the + /// refetch is also the verdict. + /// + /// A playlist the server says it does not have is gone, whatever the delete + /// said, so the stored snapshot is dropped rather than left to be shown by + /// a detail view that is still open on it. Every other failure is swallowed + /// like any reconciling refetch: not reaching the server says nothing about + /// whether the playlist still exists. + private func reconcileSnapshotAfterDelete(id: String, for account: Account) async { + do { + try await fetchSnapshot(id: id, for: account) + } catch let error as APIError where Self.saysPlaylistIsGone(error) { + forgetSnapshot(id: id, for: account.id) + forgetListEntry(id: id, for: account.id) + } catch { + // Swallowed: see the doc comment on `reconcileList(for:)`. + } + } + + private static func saysPlaylistIsGone(_ error: APIError) -> Bool { + switch error { + case .notFound: + return true + case .httpError(let statusCode, _): + return statusCode == 404 + default: + return false + } + } + private func forgetSnapshot(id: String, for accountID: UUID) { snapshots[accountID]?[id] = nil snapshotFetchDates[accountID]?[id] = nil } + /// Takes a playlist out of the account's last-fetched list. + /// + /// The list is refetched right after, which normally says the same thing — + /// but a refetch that cannot reach the server would otherwise leave a + /// playlist that is known to be gone sitting in it. + private func forgetListEntry(id: String, for accountID: UUID) { + lists[accountID]?.removeAll { $0.id.playlistID == id } + } + // MARK: - Fences private func isCurrent(_ generation: Int, for accountID: UUID) -> Bool { diff --git a/Yattee/Views/Instances/InstanceBrowseView.swift b/Yattee/Views/Instances/InstanceBrowseView.swift index 919ddfa7..3745a85e 100644 --- a/Yattee/Views/Instances/InstanceBrowseView.swift +++ b/Yattee/Views/Instances/InstanceBrowseView.swift @@ -29,6 +29,9 @@ struct InstanceBrowseView: View { /// Which login the account-scoped content belongs to, so a page still in /// flight cannot be appended to the next one's feed. @State private var accountContentGeneration = 0 + /// Which account the displayed — or in-flight — account-scoped content is + /// for, so a change of effective account can be noticed at all. + @State private var accountContentAccountID: UUID? @State private var feedVideos: [Video] = [] @State private var feedSubscriptions: [Channel] = [] @@ -443,9 +446,16 @@ struct InstanceBrowseView: View { } // A credential can arrive over iCloud Keychain, or an account be added, // while this stays on screen — the Feed tab depends on it and would - // otherwise stay missing until the view was rebuilt. + // otherwise stay missing until the view was rebuilt. A status change can + // also move *which* account is effective here, since the resolution + // falls back to a sibling login on the same instance, so the content + // loaded for the previous one has to go with it. .onReceive(NotificationCenter.default.publisher(for: .accountStatusesDidChange)) { _ in + let accountChanged = discardAccountScopedContentIfAccountChanged() refreshLoginState() + if accountChanged { + Task { await startContentLoad() } + } } .onReceive(NotificationCenter.default.publisher(for: .accountsDidChange)) { _ in refreshLoginState() @@ -1228,6 +1238,21 @@ struct InstanceBrowseView: View { // would have to exceed a page it never loaded before it could ask for // a second. feedLoadedVideoCount = 0 + accountContentAccountID = nil + } + + /// Drops account-scoped content when the account it belongs to is no longer + /// the one this instance resolves to. + /// + /// - Returns: whether anything was discarded, so the caller can reload. + @discardableResult + private func discardAccountScopedContentIfAccountChanged() -> Bool { + guard let loadedFor = accountContentAccountID, loadedFor != playlistsAccount?.id else { + return false + } + + discardAccountScopedContent() + return true } private func refreshLoginState() { @@ -1280,6 +1305,13 @@ struct InstanceBrowseView: View { // are unauthenticated and deliberately not fenced. let generation = accountContentGeneration + // Recorded before the request goes out, not after it lands: an account + // change while the fetch is in flight has to be able to see whose fetch + // it was. + if selectedTab == .feed || selectedTab == .playlists { + accountContentAccountID = playlistsAccount?.id + } + do { switch selectedTab { case .popular: diff --git a/Yattee/Views/Playlist/PlaylistFormSheet.swift b/Yattee/Views/Playlist/PlaylistFormSheet.swift index fddecba1..8f334390 100644 --- a/Yattee/Views/Playlist/PlaylistFormSheet.swift +++ b/Yattee/Views/Playlist/PlaylistFormSheet.swift @@ -299,9 +299,24 @@ struct PlaylistFormSheet: View { let trimmedTitle = title.trimmingCharacters(in: .whitespacesAndNewlines) let trimmedDescription = descriptionText.trimmingCharacters(in: .whitespacesAndNewlines) - onSave(trimmedTitle, trimmedDescription.isEmpty ? nil : trimmedDescription, privacy) + onSave(trimmedTitle, savedDescription(trimmedDescription), privacy) dismiss() } + + /// What an emptied description field means to the caller. + /// + /// Editing a server-side playlist is the one case where the difference + /// matters: `nil` means "leave whatever is there" all the way down to the + /// write layer, so an emptied field has to travel as `""` or clearing a + /// description would silently keep it. Everywhere else — creating anything, + /// editing a local playlist — an empty description *is* no description. + private func savedDescription(_ trimmed: String) -> String? { + guard !trimmed.isEmpty else { + if case .editRemote = mode { return "" } + return nil + } + return trimmed + } } // MARK: - Preview diff --git a/Yattee/Views/Playlist/UnifiedPlaylistDetailView.swift b/Yattee/Views/Playlist/UnifiedPlaylistDetailView.swift index 6f001f00..75a40f63 100644 --- a/Yattee/Views/Playlist/UnifiedPlaylistDetailView.swift +++ b/Yattee/Views/Playlist/UnifiedPlaylistDetailView.swift @@ -945,23 +945,18 @@ struct UnifiedPlaylistDetailView: View { /// Pairs each displayed row with the entry that can name it to the server. /// - /// `entries` is a *subsequence* of the playlist's videos — a row the server - /// refused to hand an identity to is still displayed, it just cannot be a - /// target — so the two lists are walked together rather than indexed - /// against each other. Matching on the video id alone would not do either: - /// a playlist may hold the same video twice, and each copy is its own row. + /// The correspondence is the one the snapshot builder recorded, not one + /// recovered here: `entries` is a *subsequence* of the playlist's videos — + /// a row the server refused to hand an identity to is still displayed, it + /// just cannot be a target — and matching the two lists back up by video id + /// cannot tell two rows of the same video apart, so a duplicate row without + /// an identity would steal its twin's entry and remove the wrong one. private static func rowEntries(for snapshot: RemotePlaylistSnapshot) -> [Int: RemotePlaylistEntry] { - let videos = snapshot.playlist.videos + let rowCount = snapshot.playlist.videos.count var map: [Int: RemotePlaylistEntry] = [:] - var row = 0 - for entry in snapshot.entries { - while row < videos.count, videos[row].id != entry.video.id { - row += 1 - } - guard row < videos.count else { break } - map[row] = entry - row += 1 + for entry in snapshot.entries where entry.displayRow >= 0 && entry.displayRow < rowCount { + map[entry.displayRow] = entry } return map diff --git a/YatteeTests/AccountServiceTests.swift b/YatteeTests/AccountServiceTests.swift index 135c1312..e3111832 100644 --- a/YatteeTests/AccountServiceTests.swift +++ b/YatteeTests/AccountServiceTests.swift @@ -154,6 +154,12 @@ private final class AccountServiceHarness { /// What the credential reader reports right now. var credential: String? = "secret-original" + /// Installed as ``credential`` the moment it is next read, so a test can + /// place a sign-out exactly between resolving a credential and using it — + /// the window the pre-transport recheck exists for, which is otherwise not + /// reachable from outside on the paths that do no awaiting first. + var credentialSwapOnNextRead: String? + init(type: InstanceType = .invidious, isEnabled: Bool = true) { instances = InstancesManager() instance = Instance( @@ -174,11 +180,20 @@ private final class AccountServiceHarness { service = AccountService( accountsManager: accounts, - credentialReader: { [weak self] _ in self?.credential }, + credentialReader: { [weak self] _ in self?.readCredential() }, transport: transport ) } + private func readCredential() -> String? { + let value = credential + if let swap = credentialSwapOnNextRead { + credential = swap + credentialSwapOnNextRead = nil + } + return value + } + /// Instances live in the shared defaults, so each harness takes its own /// back out again. func tearDown() { @@ -224,7 +239,8 @@ private func makeSnapshot( ? .invidious(playlistID: playlistID, indexID: "TOKEN-\(video.id.videoID)") : .piped(playlistID: playlistID, position: offset), video: video, - position: offset + position: offset, + displayRow: offset ) } @@ -416,6 +432,65 @@ struct AccountServiceTests { #expect(await harness.transport.calls == [.snapshot(id: "pl", credential: "secret-original")]) } + // MARK: - Credential recheck before a write + + @Test("A credential replaced during the Piped verify step sends no removal") + func pipedRemovalStopsWhenTheCredentialChangesMidVerify() async throws { + let harness = AccountServiceHarness(type: .piped) + defer { harness.tearDown() } + let snapshot = makeSnapshot(playlistID: "pl", videoIDs: ["aaa", "bbb"], kind: .piped) + await harness.transport.setSnapshot(snapshot) + await harness.transport.holdCalls() + + let removal = Task { [service = harness.service!, account = harness.account] in + try await service.removeEntry(snapshot.entries[1], fromPlaylist: "pl", for: account) + } + + // The verify fetch is out. Somebody signs out and back in as another + // user before it answers. + try await harness.transport.waitForCalls(1) + harness.credential = "secret-somebody-else" + await harness.transport.release() + + await #expect(throws: AccountServiceError.notAuthenticated) { + try await removal.value + } + + // The verify happened; the removal never did. + #expect(await harness.transport.calls == [.snapshot(id: "pl", credential: "secret-original")]) + } + + @Test("A mutation whose credential changed since it was resolved is not sent") + func mutationStopsWhenTheCredentialChangesBeforeTransport() async throws { + let harness = AccountServiceHarness() + defer { harness.tearDown() } + // Resolution reads "secret-original"; the recheck immediately before the + // write reads the replacement. + harness.credentialSwapOnNextRead = "secret-replacement" + + await #expect(throws: AccountServiceError.notAuthenticated) { + _ = try await harness.service.createPlaylist( + title: "Watch later", + description: nil, + privacy: nil, + for: harness.account + ) + } + + #expect(await harness.transport.callCount == 0) + } + + @Test("A read is not stopped by a credential change, only a write is") + func readsAreNotRechecked() async throws { + let harness = AccountServiceHarness() + defer { harness.tearDown() } + harness.credentialSwapOnNextRead = "secret-replacement" + + _ = try await harness.service.userPlaylists(for: harness.account) + + #expect(await harness.transport.calls == [.userPlaylists(credential: "secret-original")]) + } + @Test("An entry minted by another playlist is refused") func entryFromAnotherPlaylistIsRefused() async throws { let harness = AccountServiceHarness(type: .invidious) @@ -494,6 +569,54 @@ struct RemotePlaylistsCacheTests { #expect(calls.contains(.snapshot(id: "pl", credential: "secret-original"))) } + @Test("A delete that reported failure forgets a playlist the server no longer has") + func failedDeleteForgetsAVanishedPlaylist() async throws { + let harness = AccountServiceHarness() + defer { harness.tearDown() } + let cache = harness.makeCache() + await harness.transport.setSnapshot(makeSnapshot(playlistID: "pl", videoIDs: ["aaa"], kind: .invidious)) + await harness.transport.setPlaylists([Playlist(id: .global("pl"), title: "Playlist")]) + + try await cache.refreshSnapshot(id: "pl", for: harness.account) + try await cache.refreshList(for: harness.account) + #expect(cache.snapshot(id: "pl", for: harness.account.id) != nil) + + // The delete answers with an error, and every refetch after it 404s: + // the playlist is gone, whatever the delete said. + await harness.transport.setError(APIError.notFound(nil)) + + await #expect(throws: APIError.self) { + try await cache.deletePlaylist(id: "pl", for: harness.account) + } + + #expect(cache.snapshot(id: "pl", for: harness.account.id) == nil) + #expect(cache.snapshotFetchDate(id: "pl", for: harness.account.id) == nil) + #expect(cache.playlists(for: harness.account.id)?.contains { $0.id.playlistID == "pl" } != true) + } + + @Test("A delete that could not reach the server keeps the playlist it knows") + func failedDeleteKeepsThePlaylistWhenOffline() async throws { + let harness = AccountServiceHarness() + defer { harness.tearDown() } + let cache = harness.makeCache() + await harness.transport.setSnapshot(makeSnapshot(playlistID: "pl", videoIDs: ["aaa"], kind: .invidious)) + await harness.transport.setPlaylists([Playlist(id: .global("pl"), title: "Playlist")]) + + try await cache.refreshSnapshot(id: "pl", for: harness.account) + try await cache.refreshList(for: harness.account) + + await harness.transport.setError(APIError.noConnection) + + await #expect(throws: APIError.self) { + try await cache.deletePlaylist(id: "pl", for: harness.account) + } + + // Not reaching the server says nothing about whether the playlist still + // exists — the last fetch stays, as it does for every other read. + #expect(cache.snapshot(id: "pl", for: harness.account.id) != nil) + #expect(cache.playlists(for: harness.account.id)?.map(\.id.playlistID) == ["pl"]) + } + @Test("Concurrent refreshes of the same list cost one round trip") func concurrentRefreshesAreCoalesced() async throws { let harness = AccountServiceHarness() diff --git a/YatteeTests/RemotePlaylistEntryTests.swift b/YatteeTests/RemotePlaylistEntryTests.swift index 50f5ef0c..0ad49078 100644 --- a/YatteeTests/RemotePlaylistEntryTests.swift +++ b/YatteeTests/RemotePlaylistEntryTests.swift @@ -76,6 +76,15 @@ private func pipedStreamJSON(videoId: String, title: String) -> String { """ } +/// A stream row Piped sent that the app cannot turn into a video — it still +/// occupies a server position, and removals are addressed by that position. +private let pipedMalformedStreamJSON = """ +{ + "type": "channel", + "name": "Not a video at all" +} +""" + /// Builds the first page of `/playlists/{id}`. private func pipedFirstPageJSON(streams: [String], nextpage: String? = nil) -> Data { let nextpageValue = nextpage.map { "\"\($0)\"" } ?? "null" @@ -176,6 +185,34 @@ struct InvidiousRemotePlaylistSnapshotTests { #expect(snapshot.playlist.videos.map(\.id.videoID) == ["aaa", "nameless", "ccc"]) #expect(snapshot.entries.map(\.video.id.videoID) == ["aaa", "ccc"]) #expect(snapshot.entries.map(\.position) == [0, 2]) + // The unnamed row is skipped, not squeezed out: the rows that follow it + // keep the display positions they are actually drawn at. + #expect(snapshot.entries.map(\.displayRow) == [0, 2]) + } + + @Test("A nameless duplicate never borrows the entry of its twin") + func namelessDuplicateKeepsItsTwinsEntry() throws { + // The same video twice, and only the second row has an identity. Pairing + // rows to entries by video id would hand row 0 the entry that belongs to + // row 1, so removing the first would delete the second. + let page = invidiousPageJSON(videoCount: 2, videos: [ + invidiousVideoJSON(videoId: "dup", title: "Duplicated", index: 0, indexId: nil), + invidiousVideoJSON(videoId: "dup", title: "Duplicated", index: 1, indexId: "TOKEN1") + ]) + + let snapshot = try InvidiousAPI.authPlaylistSnapshot( + fromPageJSON: [page], + playlistID: "IVPLtest", + baseURL: testBaseURL + ) + + #expect(snapshot.playlist.videos.map(\.id.videoID) == ["dup", "dup"]) + #expect(snapshot.entries.count == 1) + #expect(snapshot.entries[0].id == .invidious(playlistID: "IVPLtest", indexID: "TOKEN1")) + #expect(snapshot.entries[0].position == 1) + // The row the server named is row 1, and no entry claims row 0. + #expect(snapshot.entries[0].displayRow == 1) + #expect(!snapshot.entries.contains { $0.displayRow == 0 }) } @Test("A row without an index is display-only and sorts last") @@ -343,7 +380,7 @@ struct PipedRemotePlaylistSnapshotTests { #expect(snapshot.playlist.id.playlistID == "playlist-uuid") } - @Test("Entry n is the same row as playlist.videos[n]") + @Test("An entry's displayRow is the row it is shown as") func entriesAlignWithDisplayedVideos() throws { let page = pipedFirstPageJSON(streams: [ pipedStreamJSON(videoId: "aaa", title: "First"), @@ -359,12 +396,64 @@ struct PipedRemotePlaylistSnapshotTests { #expect(snapshot.entries.count == snapshot.playlist.videos.count) for entry in snapshot.entries { - #expect(snapshot.playlist.videos[entry.position].id == entry.video.id) + #expect(snapshot.playlist.videos[entry.displayRow].id == entry.video.id) } + // With nothing malformed in the way, the two numbers agree. + #expect(snapshot.entries.map(\.position) == [0, 1, 2]) + #expect(snapshot.entries.map(\.displayRow) == [0, 1, 2]) // Two rows of the same video stay two distinct entries. #expect(Set(snapshot.entries).count == 3) } + @Test("A malformed row still occupies the server position it was sent at") + func malformedRowKeepsTheServerPositionsHonest() throws { + let page = pipedFirstPageJSON(streams: [ + pipedMalformedStreamJSON, + pipedStreamJSON(videoId: "aaa", title: "First"), + pipedStreamJSON(videoId: "bbb", title: "Second") + ]) + + let snapshot = try PipedAPI.userPlaylistSnapshot( + fromPageJSON: [page], + playlistID: "playlist-uuid", + instanceURL: pipedBaseURL + ) + + // The malformed row is not displayed and cannot be removed… + #expect(snapshot.playlist.videos.map(\.id.videoID) == ["aaa", "bbb"]) + #expect(snapshot.entries.map(\.video.id.videoID) == ["aaa", "bbb"]) + // …but it still shifts every server position after it, which is what a + // removal is addressed by. + #expect(snapshot.entries.map(\.position) == [1, 2]) + #expect(snapshot.entries.map(\.id) == [ + .piped(playlistID: "playlist-uuid", position: 1), + .piped(playlistID: "playlist-uuid", position: 2) + ]) + // The displayed rows are unaffected by it. + #expect(snapshot.entries.map(\.displayRow) == [0, 1]) + } + + @Test("Malformed rows shift positions across page boundaries too") + func malformedRowsShiftPositionsAcrossPages() throws { + let firstPage = pipedFirstPageJSON(streams: [ + pipedStreamJSON(videoId: "aaa", title: "First"), + pipedMalformedStreamJSON + ], nextpage: "token") + let secondPage = pipedNextPageJSON(streams: [ + pipedStreamJSON(videoId: "bbb", title: "Second") + ]) + + let snapshot = try PipedAPI.userPlaylistSnapshot( + fromPageJSON: [firstPage, secondPage], + playlistID: "playlist-uuid", + instanceURL: pipedBaseURL + ) + + #expect(snapshot.entries.map(\.video.id.videoID) == ["aaa", "bbb"]) + #expect(snapshot.entries.map(\.position) == [0, 2]) + #expect(snapshot.entries.map(\.displayRow) == [0, 1]) + } + @Test("A playlist with no streams yields no entries") func emptyPlaylistYieldsNoEntries() throws { let page = pipedFirstPageJSON(streams: []) @@ -456,12 +545,14 @@ struct RemotePlaylistEntryIDTests { let first = RemotePlaylistEntry( id: .invidious(playlistID: "one", indexID: "TOKEN0"), video: video, - position: 0 + position: 0, + displayRow: 0 ) let second = RemotePlaylistEntry( id: .invidious(playlistID: "one", indexID: "TOKEN2"), video: video, - position: 2 + position: 2, + displayRow: 2 ) #expect(first != second)