address feedback

Author: GianniCarlo

BookPlayer/Generated/AutoMockable.generated.swift

@@ -1687,6 +1687,32 @@ class SyncServiceProtocolMock: SyncServiceProtocol {
         set(value) { underlyingDownloadErrorPublisher = value }
     }
     var underlyingDownloadErrorPublisher: PassthroughSubject<(String, Error), Never>!
+    //MARK: - updateSyncEnabled
+
+    var updateSyncEnabledCallsCount = 0
+    var updateSyncEnabledCalled: Bool {
+        return updateSyncEnabledCallsCount > 0
+    }
+    var updateSyncEnabledReceivedEnabled: Bool?
+    var updateSyncEnabledReceivedInvocations: [Bool] = []
+    var updateSyncEnabledClosure: ((Bool) -> Void)?
+    func updateSyncEnabled(_ enabled: Bool) {
+        updateSyncEnabledCallsCount += 1
+        updateSyncEnabledReceivedEnabled = enabled
+        updateSyncEnabledReceivedInvocations.append(enabled)
+        updateSyncEnabledClosure?(enabled)
+    }
+    //MARK: - logout
+
+    var logoutCallsCount = 0
+    var logoutCalled: Bool {
+        return logoutCallsCount > 0
+    }
+    var logoutClosure: (() async -> Void)?
+    func logout() async {
+        logoutCallsCount += 1
+        await logoutClosure?()
+    }
     //MARK: - queuedJobsCount
 
     var queuedJobsCountCallsCount = 0

BookPlayer/Library/ItemList/Views/BookView.swift

@@ -86,9 +86,12 @@ struct BookView: View {
     let audioMetadataService = AudioMetadataService()
     let libraryService = LibraryService()
     libraryService.setup(dataManager: dataManager, audioMetadataService: audioMetadataService)
+    let accountService = AccountService()
+    accountService.setup(dataManager: dataManager)
     syncService.setup(
       isActive: true,
       libraryService: libraryService,
+      accountService: accountService,
       dataManager: dataManager
     )
 

BookPlayer/MainView.swift

@@ -19,7 +19,6 @@ struct MainView: View {
   @Environment(\.libraryService) private var libraryService
   @Environment(\.playerState) private var playerState
   @Environment(\.syncService) private var syncService
-  @Environment(\.accountService) private var accountService
   @Environment(\.jellyfinService) private var jellyfinService
   @Environment(\.audiobookshelfService) private var audiobookshelfService
   @Environment(\.playbackService) private var playbackService
@@ -124,22 +123,14 @@ struct MainView: View {
         playerState.showPlayer = false
       }
     }
-    .onReceive(
-      NotificationCenter.default.publisher(for: .accountUpdate, object: nil)
-    ) { _ in
-      guard accountService.hasAccount() else { return }
-
-      if accountService.hasSyncEnabled() {
-        if !syncService.isActive {
-          syncService.isActive = true
-          Task {
-            try? await listSyncRefreshService.syncList(at: nil)
-            listState.reloadAll()
-          }
-        }
-      } else if syncService.isActive {
-        syncService.isActive = false
-        syncService.cancelAllJobs()
+    // SyncService owns the active/inactive decision (it observes account/subscription
+    // changes itself). The view only reacts to sync becoming active to refresh the
+    // library list — it no longer writes `isActive`.
+    .onChange(of: syncService.isActive) { _, isActive in
+      guard isActive else { return }
+      Task {
+        try? await listSyncRefreshService.syncList(at: nil)
+        listState.reloadAll()
       }
     }
   }

BookPlayer/Profile/Profile/ProfileSyncTasksSectionView.swift

@@ -96,8 +96,10 @@ struct ProfileSyncTasksSectionView: View {
     let audioMetadataService = AudioMetadataService()
     let libraryService = LibraryService()
     libraryService.setup(dataManager: dataManager, audioMetadataService: audioMetadataService)
+    let accountService = AccountService()
+    accountService.setup(dataManager: dataManager)
     let syncService = SyncService()
-    syncService.setup(isActive: true, libraryService: libraryService, dataManager: dataManager)
+    syncService.setup(isActive: true, libraryService: libraryService, accountService: accountService, dataManager: dataManager)
 
     return syncService
   }()

BookPlayer/Utils/AppServices.swift

@@ -76,7 +76,11 @@ final class AppServices: BPLogger {
       let accountService = makeAccountService(dataManager: dataManager)
       let audioMetadataService = makeAudioMetadataService()
       let libraryService = makeLibraryService(dataManager: dataManager, audioMetadataService: audioMetadataService)
-      let syncService = makeSyncService(accountService: accountService, libraryService: libraryService, dataManager: dataManager)
+      let syncService = makeSyncService(
+        accountService: accountService,
+        libraryService: libraryService,
+        dataManager: dataManager
+      )
       let playbackService = makePlaybackService(libraryService: libraryService)
       let playerManager = PlayerManager(
         libraryService: libraryService,
@@ -214,15 +218,27 @@ final class AppServices: BPLogger {
     return AudioMetadataService()
   }
 
-  private func makeLibraryService(dataManager: DataManager, audioMetadataService: AudioMetadataServiceProtocol) -> LibraryService {
+  private func makeLibraryService(
+    dataManager: DataManager,
+    audioMetadataService: AudioMetadataServiceProtocol
+  ) -> LibraryService {
     let service = LibraryService()
     service.setup(dataManager: dataManager, audioMetadataService: audioMetadataService)
     return service
   }
 
-  private func makeSyncService(accountService: AccountService, libraryService: LibraryService, dataManager: DataManager) -> SyncService {
+  private func makeSyncService(
+    accountService: AccountService,
+    libraryService: LibraryService,
+    dataManager: DataManager
+  ) -> SyncService {
     let service = SyncService()
-    service.setup(isActive: accountService.hasSyncEnabled(), libraryService: libraryService, dataManager: dataManager)
+    service.setup(
+      isActive: accountService.hasSyncEnabled(),
+      libraryService: libraryService,
+      accountService: accountService,
+      dataManager: dataManager
+    )
     return service
   }
 

BookPlayerWatch/CoreServices.swift

@@ -48,6 +48,6 @@ class CoreServices: ObservableObject {
 
   func updateSyncEnabled(_ enabled: Bool) {
     hasSyncEnabled = enabled
-    syncService.isActive = enabled
+    syncService.updateSyncEnabled(enabled)
   }
 }

BookPlayerWatch/ExtensionDelegate.swift

@@ -75,6 +75,7 @@ class ExtensionDelegate: NSObject, WKApplicationDelegate, ObservableObject {
       syncService.setup(
         isActive: accountService.hasSyncEnabled(),
         libraryService: libraryService,
+        accountService: accountService,
         dataManager: dataManager
       )
       let playbackService = PlaybackService()

Shared/Services/Sync/SyncService.swift

@@ -21,8 +21,15 @@ public enum BPSyncError: Error {
 
 /// sourcery: AutoMockable
 public protocol SyncServiceProtocol {
-  /// Flag to check if it can sync or not
-  var isActive: Bool { get set }
+  /// Flag to check if it can sync or not. Owned by `SyncService`; mutate it through
+  /// `updateSyncEnabled(_:)` / `logout()` rather than assigning directly.
+  var isActive: Bool { get }
+  /// Enable or disable syncing in response to account/subscription state. Disabling
+  /// also cancels any queued jobs.
+  func updateSyncEnabled(_ enabled: Bool)
+  /// Tear down sync state on logout/account deletion (stop syncing, clear the queue
+  /// and the scheduled-contents flag).
+  func logout() async
   /// Completion publisher for ongoing-download tasks
   var downloadCompletedPublisher: PassthroughSubject<(String, String, String?), Never> { get }
   /// Progress publisher for ongoing-download tasks
@@ -101,10 +108,16 @@ public protocol SyncServiceProtocol {
 @Observable
 public final class SyncService: SyncServiceProtocol, BPLogger {
   private var libraryService: LibrarySyncProtocol!
+  private var accountService: AccountServiceProtocol!
   private var tasksCountService: SyncTasksCountService!
   var jobManager: JobSchedulerProtocol!
   private var client: NetworkClientProtocol!
-  public var isActive: Bool = false
+  /// Owned here: writes go through `updateSyncEnabled(_:)` / `logout()`, mutated on the
+  /// main actor. External callers read only.
+  public private(set) var isActive: Bool = false
+  /// In-flight logout teardown, awaited before an initial library sync re-schedules,
+  /// so a re-login can't begin scheduling until the queue reset has finished.
+  private var teardownTask: Task<Void, Never>?
 
   /// Dictionary holding the initiating item relative path as key and the download tasks as value
   private var downloadTasksDictionary = [String: [URLSessionTask]]()
@@ -132,11 +145,13 @@ public final class SyncService: SyncServiceProtocol, BPLogger {
   public func setup(
     isActive: Bool,
     libraryService: LibrarySyncProtocol,
+    accountService: AccountServiceProtocol,
     client: NetworkClientProtocol = NetworkClient(),
     dataManager: DataManager
   ) {
     self.isActive = isActive
     self.libraryService = libraryService
+    self.accountService = accountService
     let tasksDataManager = TasksDataManager()
     self.tasksCountService = SyncTasksCountService(tasksDataManager: tasksDataManager)
     self.jobManager = SyncJobScheduler(tasksDataManager: tasksDataManager, dataManager: dataManager)
@@ -186,7 +201,18 @@ public final class SyncService: SyncServiceProtocol, BPLogger {
       .sink(receiveValue: { [weak self] _ in
         /// Covers every logout path (iOS sign-out, account deletion, Watch), since
         /// they all post `.logout`. Tears down the queue, the flag, and `isActive`.
-        Task { await self?.logout() }
+        /// Held in `teardownTask` so a re-login's initial sync awaits it first.
+        self?.teardownTask = Task { await self?.logout() }
+      })
+      .store(in: &disposeBag)
+
+    /// Sync ownership lives here, not in the views: any account/subscription change
+    /// re-derives whether syncing should be active. All logout paths post `.logout`
+    /// (handled above), so account-present-but-sync-disabled is the case we map here.
+    NotificationCenter.default.publisher(for: .accountUpdate, object: nil)
+      .sink(receiveValue: { [weak self] _ in
+        guard let self, self.accountService.hasAccount() else { return }
+        self.updateSyncEnabled(self.accountService.hasSyncEnabled())
       })
       .store(in: &disposeBag)
 
@@ -254,6 +280,10 @@ public final class SyncService: SyncServiceProtocol, BPLogger {
       throw BookPlayerError.networkError("Sync is not enabled")
     }
 
+    /// Wait for any in-flight logout teardown to finish before scheduling, so a fast
+    /// logout→login can't have a late `resetAllJobs()` wipe freshly-scheduled jobs.
+    await teardownTask?.value
+
     if await queuedJobsCount() > 0 {
       Self.logger.trace("Clearing orphaned tasks before initial library sync")
       await resetAllJobs()
@@ -486,6 +516,20 @@ public final class SyncService: SyncServiceProtocol, BPLogger {
     await jobManager.resetAllJobs()
   }
 
+  /// Enables or disables syncing in response to account/subscription state. Disabling
+  /// also cancels any queued jobs. Idempotent — a no-op when the state is unchanged.
+  /// `isActive` is mutated on the main actor since it's an `@Observable` value read by
+  /// SwiftUI; the actual sync-content refresh is triggered by observers of `isActive`.
+  public func updateSyncEnabled(_ enabled: Bool) {
+    Task { @MainActor in
+      guard self.isActive != enabled else { return }
+      self.isActive = enabled
+      if !enabled {
+        self.cancelAllJobs()
+      }
+    }
+  }
+
   /// Tears down sync state when the account logs out (or is deleted): stops syncing,
   /// clears the persisted task queue, and resets the "scheduled library contents" flag
   /// so the next login runs a fresh initial sync from an empty queue. Idempotent.

Shared/SwiftData/TasksDataManager.swift

@@ -79,10 +79,11 @@ public final class TasksDataManager {
     try context.delete(model: ArtworkUploadTaskModel.self)
     try context.delete(model: MatchUuidsTaskModel.self)
 
-    // SyncTaskReferenceModel has a mandatory inverse to SyncTasksContainer, which a
-    // store-level batch delete can't nullify — it trips a constraint-trigger /
-    // optimistic-lock error. Delete through the object graph instead: removing each
-    // container cascades to its task references and maintains the inverse in memory.
+    // SyncTaskReferenceModel.container participates in a cascade relationship with
+    // SyncTasksContainer. A store-level batch delete runs below the object graph and
+    // skips relationship-maintenance (cascade/nullify) entirely, which trips a
+    // constraint-trigger / optimistic-lock error on that inverse. Delete through the
+    // object graph instead: removing each container cascades to its task references.
     let containers = try context.fetch(FetchDescriptor<SyncTasksContainer>())
     for container in containers {
       context.delete(container)