Conversation
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses critical concurrency bugs and architectural anti-patterns in the codebase, focusing on proper task cancellation, optimistic update rollback, and dependency injection. The changes improve app stability and maintainability through better concurrency patterns and comprehensive documentation.
Changes:
- Removed static shared singleton anti-pattern from LibraryViewModel and migrated to SwiftUI environment-based dependency injection
- Added proper CancellationError handling and rollback in SongLikeStatusManager and FavoritesManager
- Implemented WebView crash recovery with
webViewWebContentProcessDidTerminatedelegate method - Fixed ForEach identity stability issues in PlaylistDetailView and LikedMusicView using element-based IDs
- Added system sleep/wake notification handling in AppDelegate to gracefully pause playback
- Added comprehensive documentation in AGENTS.md with common bug patterns and concurrency safety checklist
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Core/ViewModels/LibraryViewModel.swift | Removed static var shared singleton pattern to enable proper dependency injection |
| Views/macOS/SharedViews/SongActionsHelper.swift | Added libraryViewModel parameter to playlist management functions for dependency injection |
| Views/macOS/SearchView.swift | Added environment injection for LibraryViewModel and updated SongActionsHelper calls |
| Views/macOS/PlaylistDetailView.swift | Added environment injection and fixed ForEach to use stable element IDs instead of indices |
| Views/macOS/LikedMusicView.swift | Fixed ForEach to use stable element IDs with enumeration |
| Core/Services/FavoritesManager.swift | Added debouncing and race condition prevention with cancellable save tasks |
| Core/Services/SongLikeStatusManager.swift | Added explicit CancellationError handling to rollback optimistic cache updates |
| Views/macOS/MiniPlayerWebView.swift | Added crash recovery handler and proper message handler cleanup in dismantleNSView |
| App/AppDelegate.swift | Added system sleep/wake notification handlers to pause playback before sleep |
| Views/macOS/MainWindow.swift | Wrapped viewForNavigationItem in Group to inject LibraryViewModel via environment |
| AGENTS.md | Added extensive documentation on common bug patterns, examples, and concurrency safety checklist |
Comments suppressed due to low confidence (1)
Core/Services/FavoritesManager.swift:113
- The FavoritesManager now tracks a saveTask that can be running when the app terminates. While FavoritesManager is a singleton that typically lives for the entire app lifetime, it's good practice to add a deinit that cancels any pending saveTask to ensure clean shutdown, especially for test instances. Consider adding:
deinit {
saveTask?.cancel()
}This aligns with the new "Background tasks cancelled in deinit" checklist item in AGENTS.md.
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 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()
try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true)
let data = try JSONEncoder().encode(itemsSnapshot)
try data.write(to: targetURL, options: .atomic)
DiagnosticsLogger.ui.debug("Saved \(itemsSnapshot.count) favorite items")
} catch {
DiagnosticsLogger.ui.error("Failed to save favorites: \(error.localizedDescription)")
}
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func likeTrack() async throws { | ||
| likeTask?.cancel() | ||
| likeTask = Task { | ||
| do { | ||
| try await api.like(trackId) | ||
| } catch { | ||
| // Handle error, update UI | ||
| throw error | ||
| } | ||
| } | ||
| try await likeTask?.value | ||
| } |
There was a problem hiding this comment.
The example code for "GOOD" fire-and-forget tasks has a logical issue. The function is marked as async throws, but inside it creates a non-throwing Task (Task<Void, Never>), then tries to throw errors from within that Task. This won't work as expected because errors thrown inside a Task<Void, Never> are not propagated to the caller.
The example should either:
- Use
Task<Void, Error>and await the task properly, or - Not mark the function as
async throwsand handle errors internally
AGENTS.md
Outdated
| } catch is CancellationError { | ||
| cache[song.id] = previous // Rollback on cancel | ||
| throw CancellationError() |
There was a problem hiding this comment.
The example code shows throw CancellationError() after handling cancellation. In Swift concurrency, CancellationError is typically not re-thrown after being caught and handled. The cancellation has already been rolled back, and re-throwing would require the catch block to be marked as throwing. Consider removing the throw statement or clarifying the intent.
| } catch is CancellationError { | |
| cache[song.id] = previous // Rollback on cancel | |
| throw CancellationError() | |
| } catch let error as CancellationError { | |
| cache[song.id] = previous // Rollback on cancel | |
| throw error // Propagate original cancellation |
| 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 | ||
| ) | ||
| } |
There was a problem hiding this comment.
The AppDelegate registers for system sleep/wake notifications but never removes these observers. While AppDelegate lives for the entire app lifetime, it's best practice to clean up observers in deinit to prevent potential issues if the AppDelegate is ever deallocated or during testing scenarios. Consider adding a deinit method that calls NSWorkspace.shared.notificationCenter.removeObserver(self).
Description
Bug Fixes
1. FavoritesManager: Save debouncing and race condition prevention
2. SongLikeStatusManager: CancellationError handling in optimistic updates
3. LibraryViewModel: Remove static shared singleton anti-pattern
static var sharedwith mutable assignment in init()4. LikedMusicView: Fix ForEach unstable identity
.indices, id: \.self)id: \.element.id) with enumeration5. MiniPlayerWebView: WebView crash recovery and handler cleanup
webViewWebContentProcessDidTerminatefor crash recoverydismantleNSViewto prevent memory leaks6. AppDelegate: System sleep/wake notification handling
Documentation
Add "Common Bug Patterns to Avoid" section to AGENTS.md with examples:
Add "Concurrency Safety Checklist" to AGENTS.md for code review
AI Prompt (Optional)
🤖 AI Prompt Used
AI Tool:
Type of Change
Related Issues
Changes Made
Testing
xcodebuild test -only-testing:KasetTests)Checklist
swiftlint --strict && swiftformat .Screenshots
Additional Notes