fix: address critical concurrency bugs and architectural anti-patterns (#77)

Author: sozercan

AGENTS.md

@@ -189,6 +189,177 @@ swiftlint --strict && swiftformat .
 - Mark `@Observable` classes with `@MainActor`
 - Never use `DispatchQueue` — use `async`/`await`, `MainActor`
 
+### Common Bug Patterns to Avoid
+
+These patterns have caused bugs in this codebase. **Always check for these during code review.**
+
+#### ❌ Fire-and-Forget Tasks
+
+```swift
+// ❌ BAD: Task not tracked, errors lost, can't cancel
+func likeTrack() {
+    Task { await api.like(trackId) }
+}
+
+// ✅ GOOD: Track task, handle errors, support cancellation
+private var likeTask: Task<Void, Error>?
+
+func likeTrack() async throws {
+    likeTask?.cancel()
+    likeTask = Task {
+        try await api.like(trackId)
+    }
+    try await likeTask?.value
+}
+```
+
+#### ❌ Optimistic Updates Without Proper Rollback
+
+```swift
+// ❌ BAD: CancellationError not handled, cache permanently wrong
+func rate(_ song: Song, status: LikeStatus) async {
+    let previous = cache[song.id]
+    cache[song.id] = status  // Optimistic update
+    do {
+        try await api.rate(song.id, status)
+    } catch {
+        cache[song.id] = previous  // Doesn't run on cancellation!
+    }
+}
+
+// ✅ GOOD: Handle ALL errors including cancellation
+func rate(_ song: Song, status: LikeStatus) async {
+    let previous = cache[song.id]
+    cache[song.id] = status
+    do {
+        try await api.rate(song.id, status)
+    } catch let error as CancellationError {
+        cache[song.id] = previous  // Rollback on cancel
+        throw error  // Propagate original cancellation
+    } catch {
+        cache[song.id] = previous  // Rollback on error
+        throw error
+    }
+}
+```
+
+#### ❌ Static Shared Singletons with Mutable Assignment
+
+```swift
+// ❌ BAD: Race condition if multiple instances created
+class LibraryViewModel {
+    static var shared: LibraryViewModel?
+    init() { Self.shared = self }  // Overwrites previous!
+}
+
+// ✅ GOOD: Use SwiftUI Environment for dependency injection
+@Observable @MainActor
+class LibraryViewModel { /* ... */ }
+
+// In parent view:
+.environment(libraryViewModel)
+
+// In child view:
+@Environment(LibraryViewModel.self) var viewModel
+```
+
+#### ❌ `.onAppear` Instead of `.task` for Async Work
+
+```swift
+// ❌ BAD: Task not cancelled on disappear, can update stale view
+.onAppear {
+    Task { await viewModel.load() }
+}
+
+// ✅ GOOD: Lifecycle-managed, auto-cancelled on disappear
+.task {
+    await viewModel.load()
+}
+
+// ✅ GOOD: With ID for re-execution on change
+.task(id: playlistId) {
+    await viewModel.load(playlistId)
+}
+```
+
+#### ❌ ForEach with Unstable Identity
+
+```swift
+// ❌ BAD: Index-based identity causes wrong views during mutations
+ForEach(tracks.indices, id: \.self) { index in
+    TrackRow(track: tracks[index])
+}
+
+// ❌ BAD: Array enumeration recreates identity on every change
+ForEach(Array(tracks.enumerated()), id: \.offset) { index, track in
+    TrackRow(track: track, rank: index + 1)
+}
+
+// ✅ GOOD: Use stable model identity
+ForEach(tracks) { track in
+    TrackRow(track: track)
+}
+
+// ✅ GOOD: If you need index for display (charts), use element ID
+ForEach(Array(tracks.enumerated()), id: \.element.id) { index, track in
+    TrackRow(track: track, rank: index + 1)
+}
+```
+
+#### ❌ Background Tasks Not Cancelled on Deinit
+
+```swift
+// ❌ BAD: Task continues after ViewModel is deallocated
+@Observable @MainActor
+class HomeViewModel {
+    private var backgroundTask: Task<Void, Never>?
+    
+    func startLoading() {
+        backgroundTask = Task { /* ... */ }
+    }
+    // Missing deinit cleanup!
+}
+
+// ✅ GOOD: Cancel tasks in deinit
+@Observable @MainActor
+class HomeViewModel {
+    private var backgroundTask: Task<Void, Never>?
+    
+    func startLoading() {
+        backgroundTask?.cancel()
+        backgroundTask = Task { [weak self] in
+            guard !Task.isCancelled else { return }
+            // ...
+        }
+    }
+    
+    deinit {
+        backgroundTask?.cancel()
+    }
+}
+```
+
+#### ❌ Shared Continuation Tokens Across Different Requests
+
+```swift
+// ❌ BAD: Single token for all search types causes conflicts
+class YTMusicClient {
+    private var searchContinuationToken: String?  // Shared!
+    
+    func searchSongs() { /* sets token */ }
+    func searchAlbums() { /* overwrites token! */ }
+}
+
+// ✅ GOOD: Scope tokens by request type or return in response
+class YTMusicClient {
+    private var continuationTokens: [String: String] = [:]
+    
+    func searchSongs() -> (songs: [Song], continuation: String?) {
+        // Return token with response, let caller manage
+    }
+}
+```
+
 ### WebKit Patterns
 
 - Use `WebKitManager`'s shared `WKWebsiteDataStore` for cookie persistence
@@ -264,6 +435,19 @@ Before completing non-trivial features, verify:
 - [ ] ForEach uses stable identity (avoid `Array(enumerated())` unless rank is needed)
 - [ ] Frequently updating UI (e.g., progress) caches formatted strings
 
+## Concurrency Safety Checklist
+
+Before completing features with async code, verify:
+
+- [ ] No fire-and-forget `Task { }` without error handling
+- [ ] Optimistic updates handle `CancellationError` explicitly
+- [ ] Background tasks cancelled in `deinit`
+- [ ] Using `.task` instead of `.onAppear { Task { } }`
+- [ ] Continuation tokens scoped per-request (not shared across types)
+- [ ] No `static var shared` pattern with mutable assignment in `init`
+- [ ] WebView message handlers removed in `dismantleNSView`
+- [ ] `WKNavigationDelegate` implements `webViewWebContentProcessDidTerminate`
+
 > 📚 See [docs/architecture.md#performance-guidelines](docs/architecture.md#performance-guidelines) for detailed patterns.
 
 ## Task Planning: Phases with Exit Criteria

App/AppDelegate.swift

@@ -29,6 +29,47 @@ final class AppDelegate: NSObject, NSApplicationDelegate {
             try? await Task.sleep(for: .milliseconds(500))
             self.setupWindowDelegate()
         }
+
+        // Register for system sleep/wake notifications
+        self.registerForSleepWakeNotifications()
+    }
+
+    /// Registers for system sleep and wake notifications to handle playback appropriately.
+    private func registerForSleepWakeNotifications() {
+        let notificationCenter = NSWorkspace.shared.notificationCenter
+
+        notificationCenter.addObserver(
+            self,
+            selector: #selector(self.systemWillSleep),
+            name: NSWorkspace.willSleepNotification,
+            object: nil
+        )
+
+        notificationCenter.addObserver(
+            self,
+            selector: #selector(self.systemDidWake),
+            name: NSWorkspace.didWakeNotification,
+            object: nil
+        )
+    }
+
+    /// Tracks whether audio was playing before system sleep (for resume on wake).
+    private var wasPlayingBeforeSleep: Bool = false
+
+    @objc private func systemWillSleep(_: Notification) {
+        // Remember playback state and pause before sleep
+        self.wasPlayingBeforeSleep = self.playerService?.isPlaying ?? false
+        if self.wasPlayingBeforeSleep {
+            DiagnosticsLogger.player.info("System going to sleep, pausing playback")
+            SingletonPlayerWebView.shared.pause()
+        }
+    }
+
+    @objc private func systemDidWake(_: Notification) {
+        // Optionally resume playback after wake if it was playing before sleep
+        // Note: We don't auto-resume by default as it could be surprising
+        // Just log the wake event for now
+        DiagnosticsLogger.player.info("System woke from sleep, wasPlayingBeforeSleep: \(self.wasPlayingBeforeSleep)")
     }
 
     func applicationDidBecomeActive(_: Notification) {

Core/Services/API/YTMusicClient.swift

@@ -1090,6 +1090,7 @@ final class YTMusicClient: YTMusicClientProtocol {
             "target": ["playlistId": playlistId],
         ]
 
+        self.logger.debug("Calling like/removelike with playlistId=\(playlistId)")
         _ = try await self.request("like/removelike", body: body)
         self.logger.info("Successfully unsubscribed from podcast \(showId)")
 

Core/Services/FavoritesManager.swift

@@ -29,6 +29,9 @@ final class FavoritesManager {
         return kasetDir.appendingPathComponent("favorites.json")
     }
 
+    /// Task for the current save operation - cancelled when new save is triggered.
+    private var saveTask: Task<Void, Never>?
+
     // MARK: - Initialization
 
     private init() {
@@ -73,18 +76,28 @@ final class FavoritesManager {
 
     /// Saves items to disk asynchronously on a background thread.
     /// Captures current state and writes without blocking the main thread.
+    /// Cancels any pending save and debounces to coalesce rapid changes.
     /// Test instances (skipPersistence=true) never write to disk.
     private func save() {
         // Skip persistence for test instances to avoid overwriting user data
         guard !self.skipPersistence else { return }
 
+        // Cancel any pending save to avoid race conditions
+        self.saveTask?.cancel()
+
         // Capture current state for background write
         let itemsSnapshot = self.items
         let targetURL = self.fileURL
 
-        // Perform disk I/O off the main actor.
-        // Fire-and-forget: failures are logged but not propagated.
-        Task(priority: .utility) {
+        // Perform disk I/O off the main actor with debounce.
+        // Slight delay coalesces rapid successive saves.
+        self.saveTask = Task(priority: .utility) {
+            // Debounce: wait briefly to coalesce rapid changes
+            try? await Task.sleep(for: .milliseconds(100))
+
+            // Check if cancelled (a newer save superseded this one)
+            guard !Task.isCancelled else { return }
+
             do {
                 // Ensure directory exists
                 let directory = targetURL.deletingLastPathComponent()

Core/Services/SongLikeStatusManager.swift

@@ -99,6 +99,14 @@ final class SongLikeStatusManager {
         do {
             try await client.rateSong(videoId: song.videoId, rating: status)
             DiagnosticsLogger.api.info("Rated song \(song.videoId) as \(status.rawValue)")
+        } catch is CancellationError {
+            // Task was cancelled - rollback optimistic update
+            if let previous = previousStatus {
+                self.statusCache[song.videoId] = previous
+            } else {
+                self.statusCache.removeValue(forKey: song.videoId)
+            }
+            DiagnosticsLogger.api.debug("Rating cancelled for song \(song.videoId), rolled back")
         } catch {
             // Revert on failure
             if let previous = previousStatus {

Core/ViewModels/LibraryViewModel.swift

@@ -6,9 +6,6 @@ import os
 @MainActor
 @Observable
 final class LibraryViewModel {
-    /// Shared instance for checking library status from other views.
-    static var shared: LibraryViewModel?
-
     /// Current loading state.
     private(set) var loadingState: LoadingState = .idle
 
@@ -36,8 +33,6 @@ final class LibraryViewModel {
 
     init(client: any YTMusicClientProtocol) {
         self.client = client
-        // Set shared instance for global access
-        LibraryViewModel.shared = self
     }
 
     /// Checks if a playlist is in the user's library.

Tools/api-explorer.swift

@@ -327,6 +327,7 @@ let authRequiredEndpoints = Set([
     "FEmusic_library_albums",
     "FEmusic_library_artists",
     "FEmusic_library_songs",
+    "FEmusic_library_non_music_audio_list",
     "FEmusic_recently_played",
     "FEmusic_offline",
     "FEmusic_library_privately_owned_landing",

Views/macOS/LibraryView.swift

@@ -27,6 +27,7 @@ struct LibraryView: View {
     @State var viewModel: LibraryViewModel
     @Environment(PlayerService.self) private var playerService
     @Environment(FavoritesManager.self) private var favoritesManager
+    @Environment(LibraryViewModel.self) private var libraryViewModelEnv: LibraryViewModel?
     @State private var networkMonitor = NetworkMonitor.shared
 
     @State private var navigationPath = NavigationPath()
@@ -66,8 +67,9 @@ struct LibraryView: View {
                     )
                 )
             }
-            .navigationDestination(for: PodcastShow.self) { show in
+            .navigationDestination(for: PodcastShow.self) { [libraryViewModelEnv] show in
                 PodcastShowView(show: show, client: self.viewModel.client)
+                    .environment(libraryViewModelEnv)
             }
         }
         .safeAreaInset(edge: .bottom, spacing: 0) {

Views/macOS/LikedMusicView.swift

@@ -89,8 +89,7 @@ struct LikedMusicView: View {
                 if self.viewModel.songs.isEmpty {
                     self.emptyStateView
                 } else {
-                    ForEach(self.viewModel.songs.indices, id: \.self) { index in
-                        let song = self.viewModel.songs[index]
+                    ForEach(Array(self.viewModel.songs.enumerated()), id: \.element.id) { index, song in
                         self.songRow(song, index: index)
                             .onAppear {
                                 // Load more when reaching the last few items