Mirrored_Repos/yattee
1
0
Fork 0
mirror of https://github.com/yattee/yattee.git synced 2026-10-23 21:22:53 +00:00
Code Issues Packages Projects Releases Wiki Activity

Read a playlist back before writing over it

Browse Source 
Ten review findings in the remote-playlist work, all of them the same
shape: something acted on what it remembered instead of what the server
says now.

- A confirmed-gone playlist is forgotten whole — snapshot, list entry,
  and both ordering fences — so a refresh that started before the delete
  can no longer republish it, and a list refetch that fails offline no
  longer leaves it on screen.
- `.accountsDidChange` discards account-scoped content when the account
  it belonged to is no longer the effective one, as the status
  notification already did.
- A tab that stops being offered no longer stays selected: the browse
  view moves to the first available one, which reloads by itself.
- A Handoff account id is accepted only when that account is on the
  instance the playlist lives on.
- Composite writes moved up into `AccountService`, over a transport whose
  every method is exactly one request: create-with-privacy, rename, and
  set-description. The credential is rechecked between each pair, so a
  sign-out between the two halves of a create or an edit can no longer
  send the second half under the previous user's secret.
- Editing a playlist no longer forces it private. Invidious serialises
  visibility as `isListed` — `privacy.public?`, so unlisted and private
  are indistinguishable — which the snapshot now carries; the picker
  starts there, and sends nothing at all unless it was moved. Invidious
  keeps the stored visibility when the PATCH omits it (measured), so an
  untouched picker leaves it exactly as it was.
- Tapping Edit on a server-side playlist awaits an authenticated refetch
  first and populates the form from it; a failed refetch shows an error
  and no sheet.
- Playlist rows are keyed by row rather than by video id, which two rows
  of the same video share.
- The add-to-playlist sheet offers only accounts whose instance is
  enabled and that hold a credential, alongside the write capability.
This commit is contained in:
Arkadiusz Fal
2026-09-02 23:59:31 +02:00
parent 24b43ac866
commit b4855f5be9
12 changed files with 599 additions and 132 deletions
Download Patch File Download Diff File Expand all files Collapse all files

210
YatteeTests/AccountServiceTests.swift
View File

@@ -22,8 +22,9 @@ private actor ScriptedPlaylistTransport: RemotePlaylistTransport {
enum Call: Equatable, Sendable {
case userPlaylists(credential: String)
case snapshot(id: String, credential: String)
case create(title: String, description: String?, privacy: PlaylistPrivacy?, credential: String)
case update(id: String, title: String, description: String?, privacy: PlaylistPrivacy?, credential: String)
case create(title: String, privacy: PlaylistPrivacy?, credential: String)
case rename(id: String, title: String, privacy: PlaylistPrivacy?, credential: String)
case setDescription(id: String, description: String, title: String, credential: String)
case delete(id: String, credential: String)
case addVideo(playlistID: String, videoID: String, credential: String)
case remove(entryID: RemotePlaylistEntryID, credential: String)
@@ -34,6 +35,11 @@ private actor ScriptedPlaylistTransport: RemotePlaylistTransport {
/// Thrown by the next call of any kind, once set.
var nextError: Error?
/// The 1-based index of the first call that throws ``nextError``. Everything
/// before it answers normally, which is how a write that lands and a refetch
/// that then fails are scripted in one run.
private var firstFailingCall = 1
/// What ``userPlaylistSnapshot(id:instance:credential:)`` answers.
var snapshot = RemotePlaylistSnapshot(playlist: Playlist(id: .global("pl"), title: "Playlist"), entries: [])
@@ -45,10 +51,21 @@ private actor ScriptedPlaylistTransport: RemotePlaylistTransport {
private var parked: [CheckedContinuation<Void, Never>] = []
func setError(_ error: Error?) { nextError = error }
/// Fails every call from `index` on, leaving the earlier ones alone.
func setError(_ error: Error, fromCall index: Int) {
nextError = error
firstFailingCall = index
}
func setSnapshot(_ snapshot: RemotePlaylistSnapshot) { self.snapshot = snapshot }
func setPlaylists(_ playlists: [Playlist]) { self.playlists = playlists }
func holdCalls() { holdsCallsOpen = true }
/// Stops parking *new* calls without releasing the ones already parked, so
/// a request can be left in flight while other work runs to completion.
func stopHoldingNewCalls() { holdsCallsOpen = false }
func release() {
holdsCallsOpen = false
let waiting = parked
@@ -74,7 +91,7 @@ private actor ScriptedPlaylistTransport: RemotePlaylistTransport {
await withCheckedContinuation { parked.append($0) }
}
if let error = nextError {
if let error = nextError, calls.count >= firstFailingCall {
throw error
}
}
@@ -95,27 +112,33 @@ private actor ScriptedPlaylistTransport: RemotePlaylistTransport {
func createPlaylist(
title: String,
description: String?,
privacy: PlaylistPrivacy?,
instance _: Instance,
credential: String
) async throws -> Playlist {
try await record(
.create(title: title, description: description, privacy: privacy, credential: credential)
)
return Playlist(id: .global("new-playlist"), title: title, description: description)
try await record(.create(title: title, privacy: privacy, credential: credential))
return Playlist(id: .global("new-playlist"), title: title)
}
func updatePlaylist(
func renamePlaylist(
id: String,
title: String,
description: String?,
privacy: PlaylistPrivacy?,
instance _: Instance,
credential: String
) async throws {
try await record(.rename(id: id, title: title, privacy: privacy, credential: credential))
}
func setPlaylistDescription(
id: String,
description: String,
title: String,
instance _: Instance,
credential: String
) async throws {
try await record(
.update(id: id, title: title, description: description, privacy: privacy, credential: credential)
.setDescription(id: id, description: description, title: title, credential: credential)
)
}
@@ -282,7 +305,7 @@ struct AccountServiceTests {
)
#expect(await harness.transport.calls == [
.create(title: "Watch later", description: nil, privacy: nil, credential: "secret-original")
.create(title: "Watch later", privacy: nil, credential: "secret-original")
])
}
@@ -298,8 +321,119 @@ struct AccountServiceTests {
for: harness.account
)
// Two requests, because no backend takes a description at creation.
#expect(await harness.transport.calls == [
.create(title: "Watch later", description: "Later", privacy: .unlisted, credential: "secret-original")
.create(title: "Watch later", privacy: .unlisted, credential: "secret-original"),
.setDescription(
id: "new-playlist",
description: "Later",
title: "Watch later",
credential: "secret-original"
)
])
}
// MARK: - Composite writes
@Test("A credential replaced between a creation and its description stops the second call")
func creationStopsDescribingWhenTheCredentialChanges() async throws {
let harness = AccountServiceHarness(type: .invidious)
defer { harness.tearDown() }
await harness.transport.holdCalls()
let creation = Task { [service = harness.service!, account = harness.account] in
try await service.createPlaylist(
title: "Watch later",
description: "Later",
privacy: .private,
for: account
)
}
// The creation is out. Somebody signs out and back in as another user
// before it answers — the description must not go to the new login.
try await harness.transport.waitForCalls(1)
harness.credential = "secret-somebody-else"
await harness.transport.release()
await #expect(throws: AccountServiceError.notAuthenticated) {
_ = try await creation.value
}
#expect(await harness.transport.calls == [
.create(title: "Watch later", privacy: .private, credential: "secret-original")
])
}
@Test("An edit sends the description only after rechecking the credential")
func editRechecksBetweenRenameAndDescription() async throws {
let harness = AccountServiceHarness(type: .piped)
defer { harness.tearDown() }
await harness.transport.holdCalls()
let edit = Task { [service = harness.service!, account = harness.account] in
try await service.updatePlaylist(
id: "pl",
title: "Renamed",
description: "New description",
privacy: nil,
for: account
)
}
try await harness.transport.waitForCalls(1)
harness.credential = "secret-somebody-else"
await harness.transport.release()
await #expect(throws: AccountServiceError.notAuthenticated) {
try await edit.value
}
// The rename landed under the old login; the description never went.
#expect(await harness.transport.calls == [
.rename(id: "pl", title: "Renamed", privacy: nil, credential: "secret-original")
])
}
@Test("An edit that keeps its credential renames and then describes")
func editSendsBothHalvesWithTheSameCredential() async throws {
let harness = AccountServiceHarness(type: .invidious)
defer { harness.tearDown() }
try await harness.service.updatePlaylist(
id: "pl",
title: "Renamed",
description: "New description",
privacy: .public,
for: harness.account
)
#expect(await harness.transport.calls == [
.rename(id: "pl", title: "Renamed", privacy: .public, credential: "secret-original"),
.setDescription(
id: "pl",
description: "New description",
title: "Renamed",
credential: "secret-original"
)
])
}
@Test("An edit that names no visibility sends none, rather than inventing one")
func editWithoutPrivacySendsNone() async throws {
let harness = AccountServiceHarness(type: .invidious)
defer { harness.tearDown() }
try await harness.service.updatePlaylist(
id: "pl",
title: "Renamed",
description: nil,
privacy: nil,
for: harness.account
)
#expect(await harness.transport.calls == [
.rename(id: "pl", title: "Renamed", privacy: nil, credential: "secret-original")
])
}
@@ -594,6 +728,56 @@ struct RemotePlaylistsCacheTests {
#expect(cache.playlists(for: harness.account.id)?.contains { $0.id.playlistID == "pl" } != true)
}
@Test("A refresh in flight across a delete cannot bring the playlist back")
func inFlightRefreshCannotResurrectADeletedPlaylist() 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")])
await harness.transport.holdCalls()
let refresh = Task { [account = harness.account] in
try await cache.refreshSnapshot(id: "pl", for: account)
}
try await harness.transport.waitForCalls(1)
// That fetch is parked mid-flight. The delete runs to completion around
// it, and the server stops listing the playlist.
await harness.transport.stopHoldingNewCalls()
await harness.transport.setPlaylists([])
try await cache.deletePlaylist(id: "pl", for: harness.account)
// Only now does the older fetch answer. It describes a playlist that
// existed when it was asked for, and arriving late does not bring it
// back.
await harness.transport.release()
_ = try? await refresh.value
#expect(cache.snapshot(id: "pl", for: harness.account.id) == nil)
#expect(cache.playlists(for: harness.account.id)?.isEmpty == true)
}
@Test("A delete that landed drops the list entry even when the refetch fails")
func successfulDeleteForgetsTheListEntryWhenTheRefetchFails() 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)
// The delete itself lands; the reconciling list refetch right after it
// cannot reach the server.
await harness.transport.setError(APIError.noConnection, fromCall: 4)
try await cache.deletePlaylist(id: "pl", for: harness.account)
#expect(cache.snapshot(id: "pl", for: harness.account.id) == nil)
#expect(cache.playlists(for: harness.account.id)?.isEmpty == true)
}
@Test("A delete that could not reach the server keeps the playlist it knows")
func failedDeleteKeepsThePlaylistWhenOffline() async throws {
let harness = AccountServiceHarness()

27
YatteeTests/RemotePlaylistEntryTests.swift
View File

@@ -46,9 +46,11 @@ private func invidiousVideoJSON(
private func invidiousPageJSON(
playlistID: String = "IVPLtest",
videoCount: Int,
videos: [String]
videos: [String],
isListed: Bool? = nil
) -> Data {
"""
let listedField = isListed.map { ",\n \"isListed\": \($0)" } ?? ""
return """
{
"type": "invidiousPlaylist",
"title": "Watch Later",
@@ -56,7 +58,7 @@ private func invidiousPageJSON(
"author": "arek",
"authorId": "UCowner",
"description": "A private playlist",
"videoCount": \(videoCount),
"videoCount": \(videoCount)\(listedField),
"videos": [\(videos.joined(separator: ", "))]
}
""".data(using: .utf8)!
@@ -321,6 +323,25 @@ struct InvidiousRemotePlaylistSnapshotTests {
#expect(snapshot.playlist.id.playlistID == "IVPLtest")
}
@Test("The listed flag is carried through, and its absence is not a no")
func snapshotCarriesVisibility() throws {
func snapshot(isListed: Bool?) throws -> RemotePlaylistSnapshot {
try InvidiousAPI.authPlaylistSnapshot(
fromPageJSON: [invidiousPageJSON(videoCount: 0, videos: [], isListed: isListed)],
playlistID: "IVPLtest",
baseURL: testBaseURL
)
}
// `true` is exactly public. `false` is unlisted *or* private — Invidious
// serialises `privacy.public?` and nothing else — and a page that says
// nothing leaves the visibility unknown rather than not-listed, which is
// what stops an edit from sending a guess back.
#expect(try snapshot(isListed: true).isListed == true)
#expect(try snapshot(isListed: false).isListed == false)
#expect(try snapshot(isListed: nil).isListed == nil)
}
@Test("An empty page set is a decoding failure, not an empty playlist")
func emptyPageSetThrows() {
#expect(throws: APIError.self) {