Conversation
…ty, and documentation accuracy Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR improves Swift 6 concurrency safety, adds API parsing observability, updates documentation, and implements several bug fixes and security enhancements.
Changes:
- Added proper task cancellation in deinit for 7 ViewModels and NotificationService using
nonisolated(unsafe)pattern - Added debug logging to 4 API parsers that previously failed silently (LyricsParser, RadioQueueParser, SearchResponseParser, SearchSuggestionsParser)
- Updated Foundation Models API to handle new LanguageModelSession.GenerationError cases and async prewarm API
- Enhanced documentation with Parsers Reference and Error Handling sections, moved 4 endpoints to "Implemented" status
- Fixed NSKeyedArchiver security with
requiringSecureCoding: truefor outer cookie array - Added error state display with retry button in PlayerBar
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Core/ViewModels/ChartsViewModel.swift | Added deinit with task cancellation, marked task as nonisolated(unsafe) |
| Core/ViewModels/ExploreViewModel.swift | Added deinit with task cancellation, marked task as nonisolated(unsafe) |
| Core/ViewModels/HomeViewModel.swift | Added deinit with task cancellation, marked task as nonisolated(unsafe) |
| Core/ViewModels/MoodsAndGenresViewModel.swift | Added deinit with task cancellation, marked task as nonisolated(unsafe) |
| Core/ViewModels/NewReleasesViewModel.swift | Added deinit with task cancellation, marked task as nonisolated(unsafe) |
| Core/ViewModels/PodcastsViewModel.swift | Added deinit with task cancellation, marked task as nonisolated(unsafe) |
| Core/ViewModels/SearchViewModel.swift | Added deinit with task cancellation for two tasks, marked as nonisolated(unsafe) |
| Core/Services/Notification/NotificationService.swift | Updated comment for task cancellation pattern |
| Core/Services/NetworkMonitor.swift | Removed nonisolated(unsafe) from immutable NWPathMonitor property |
| Core/Services/API/Parsers/LyricsParser.swift | Added DiagnosticsLogger and debug logging for parse failures |
| Core/Services/API/Parsers/RadioQueueParser.swift | Added DiagnosticsLogger and debug logging for parse failures |
| Core/Services/API/Parsers/SearchResponseParser.swift | Added DiagnosticsLogger and debug logging for parse failures |
| Core/Services/API/Parsers/SearchSuggestionsParser.swift | Added DiagnosticsLogger and debug logging for parse failures |
| Core/Services/API/Parsers/SongMetadataParser.swift | Added DiagnosticsLogger declaration for consistency |
| Core/Services/AI/FoundationModelsService.swift | Updated prewarm to use new async API without try-catch |
| Core/Services/AI/AIErrorHandler.swift | Added handling for 7 new GenerationError cases, fixed property access |
| Core/Services/WebKit/WebKitManager.swift | Fixed NSKeyedArchiver to use secure coding for outer cookie array |
| Core/Utilities/ImageCache.swift | Marked evictDiskCacheIfNeeded as async with explanatory comment |
| Views/macOS/PlayerBar.swift | Added error state display with retry button |
| Views/macOS/SharedViews/FavoritesSection.swift | Added explicit return statement in dropDestination closure |
| Tools/api-explorer.swift | Added diagnostic output for cookie loading failures, removed music/get_queue from auth-required list |
| docs/api-discovery.md | Moved 4 endpoints to Implemented section, added Parsers Reference and Error Handling sections, fixed search filter documentation |
| Tests/KasetTests/NetworkMonitorTests.swift | Added new test file with basic NetworkMonitor tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let watchNextTabbedResults = tabbedRenderer["watchNextTabbedResultsRenderer"] as? [String: Any], | ||
| let tabs = watchNextTabbedResults["tabs"] as? [[String: Any]] | ||
| else { | ||
| self.logger.debug("LyricsParser: Failed to extract lyrics browse ID structure") |
There was a problem hiding this comment.
The parse failure logging uses self.logger.debug but should be Self.logger.debug for consistency with static context (enum with static methods). The capital 'S' in 'Self' is the convention for static contexts.
docs/api-discovery.md
Outdated
| try await client.fetchData() | ||
| } | ||
| ``` | ||
| ``` |
There was a problem hiding this comment.
There is an extra closing triple backtick (```) on this line, which will break the markdown rendering. The code block on line 959 already has its closing triple backtick on line 969, so this line should be removed.
Suggested change
| ``` |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Summary
This PR addresses multiple issues identified in a comprehensive API and codebase audit, focusing on:
nonisolated(unsafe)patterns for task cancellation indeinitLanguageModelSessionAPI changes (prewarm, error handling)Changes
Swift Concurrency (ViewModels & Services)
deinitwith task cancellation to 6 ViewModels:ChartsViewModel,ExploreViewModel,HomeViewModel,MoodsAndGenresViewModel,NewReleasesViewModel,PodcastsViewModel,SearchViewModelnonisolated(unsafe)for proper deinit accessNetworkMonitorto use simpler immutable pattern forNWPathMonitorAPI Parsers
DiagnosticsLoggerto:LyricsParser,RadioQueueParser,SearchResponseParser,SearchSuggestionsParser,SongMetadataParserFoundation Models (macOS 26+)
AIErrorHandlerto handle newLanguageModelSession.GenerationErrorcasesFoundationModelsService.prewarmSession()to match new async API (no longer throws)Documentation
FEmusic_charts,FEmusic_moods_and_genres,FEmusic_new_releases,FEmusic_library_landing)UI & Player
PlayerBarFavoritesSectiondrop handler return valueAPI Explorer Tool
music/get_queuefrom auth-required list (works without auth)Security
WebKitManagerto userequiringSecureCoding: truefor outer cookie array archivalTesting
NetworkMonitorTests.swiftwith basic test coverageAI 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