Conversation
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR implements infinite artist mix and song radio functionality, allowing users to play personalized music mixes that automatically load more songs as they approach the end of the queue. The implementation includes API client methods for fetching mix queues with continuation tokens, parser updates to extract continuation data, PlayerService enhancements for managing infinite playback, and UI additions for starting radio from song context menus.
- Adds infinite mix loading with automatic continuation fetching when ≤10 songs remain
- Implements
playWithMix()andplayWithRadio()methods in PlayerService with proper state management - Adds "Start Radio" context menu option across multiple views (Search, Queue, Playlists, etc.)
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/playback.md | Documents infinite mix feature including how it works, key components, and state management |
| docs/api-discovery.md | Adds API documentation for continuation tokens and mix queue fetching |
| Views/macOS/SharedViews/StartRadioContextMenu.swift | New reusable context menu component for starting radio from any song |
| Views/macOS/TopSongsView.swift | Adds Start Radio context menu item to song actions |
| Views/macOS/SearchView.swift | Adds Start Radio context menu item to song actions |
| Views/macOS/QueueView.swift | Adds Start Radio context menu item and passes PlayerService to QueueRowView |
| Views/macOS/PlaylistDetailView.swift | Adds Start Radio context menu item to playlist track actions |
| Views/macOS/LikedMusicView.swift | Adds Start Radio context menu item to liked songs |
| Views/macOS/HomeView.swift | Adds Start Radio context menu item to home view songs |
| Views/macOS/ArtistDetailView.swift | Replaces play-all Mix button with personalized mix, updates shuffle to fetch all songs |
| Tools/api-explorer.swift | Enhances continuation command to support both browse and next endpoints |
| Tests/KasetTests/Helpers/MockYTMusicClient.swift | Adds mock implementations for getMixQueue and getMixQueueContinuation |
| Kaset.xcodeproj/project.pbxproj | Adds StartRadioContextMenu.swift to build phases and file references |
| Core/Services/Protocols.swift | Adds protocol methods for mix queue fetching with continuation support |
| Core/Services/Player/PlayerService.swift | Implements infinite mix with continuation token management and automatic fetching |
| Core/Services/API/YTMusicClient.swift | Adds getMixQueue and getMixQueueContinuation methods, updates getRadioQueue to use RadioQueueResult |
| Core/Services/API/Parsers/RadioQueueParser.swift | Updates parser to return RadioQueueResult with continuation token support, adds parseContinuation |
| Core/Services/API/Parsers/ArtistParser.swift | Adds parsing for mix playlist ID and video ID from artist header startRadioButton |
| Core/Services/API/MockUITestYTMusicClient.swift | Adds mock implementations returning test data for mix queue methods |
| Core/Models/ArtistDetail.swift | Adds mixPlaylistId and mixVideoId properties for artist mix support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else if self.mixContinuationToken != nil { | ||
| // At end of queue but have continuation - fetch more and continue | ||
| await self.fetchMoreMixSongsIfNeeded() | ||
| if self.currentIndex < self.queue.count - 1 { | ||
| self.currentIndex += 1 | ||
| if let nextSong = queue[safe: currentIndex] { | ||
| await self.play(song: nextSong) | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic may not handle the case when fetchMoreMixSongsIfNeeded completes but doesn't add any songs to the queue (e.g., all new songs were duplicates or the continuation returned zero songs). In this case, currentIndex will still be at queue.count - 1, but you're checking if currentIndex < queue.count - 1, which will be false, so no song will play. The user would be stuck at the end of the queue.
Consider checking if new songs were actually added before attempting to play the next song, or restructure this to avoid getting into an unplayable state.
| // Shuffle the queue to get a different order each time | ||
| // YouTube's API returns a personalized but consistent order per session, | ||
| // so we shuffle to give the user variety on each Mix button click | ||
| let shuffledSongs = result.songs.shuffled() | ||
|
|
||
| // Set up the queue and play the first song | ||
| self.queue = shuffledSongs | ||
| self.currentIndex = 0 | ||
| self.currentTrack = shuffledSongs[0] | ||
|
|
||
| // Start playback | ||
| await self.play(videoId: shuffledSongs[0].videoId) | ||
|
|
||
| self.logger.info("Mix queue loaded with \(shuffledSongs.count) songs, hasContinuation: \(result.continuationToken != nil)") |
There was a problem hiding this comment.
The shuffle logic shuffles the songs returned from the API, but if the user has already started listening and then more songs are fetched via continuation, those continuation songs are appended without shuffling. This creates an inconsistent experience where the initial batch is shuffled but subsequent batches maintain their API order. Consider either not shuffling initially, or also shuffling the continuation songs before appending them, or documenting this intentional behavior.
| // Shuffle the queue to get a different order each time | |
| // YouTube's API returns a personalized but consistent order per session, | |
| // so we shuffle to give the user variety on each Mix button click | |
| let shuffledSongs = result.songs.shuffled() | |
| // Set up the queue and play the first song | |
| self.queue = shuffledSongs | |
| self.currentIndex = 0 | |
| self.currentTrack = shuffledSongs[0] | |
| // Start playback | |
| await self.play(videoId: shuffledSongs[0].videoId) | |
| self.logger.info("Mix queue loaded with \(shuffledSongs.count) songs, hasContinuation: \(result.continuationToken != nil)") | |
| // Set up the queue and play the first song in the order provided by the API | |
| self.queue = result.songs | |
| self.currentIndex = 0 | |
| self.currentTrack = result.songs[0] | |
| // Start playback | |
| await self.play(videoId: result.songs[0].videoId) | |
| self.logger.info("Mix queue loaded with \(result.songs.count) songs, hasContinuation: \(result.continuationToken != nil)") |
| func playWithMix(playlistId: String, startVideoId: String?) async { | ||
| self.logger.info("Playing mix playlist: \(playlistId), startVideoId: \(startVideoId ?? "nil (random)")") | ||
|
|
||
| guard let client = self.ytMusicClient else { | ||
| self.logger.warning("No YTMusicClient available for playing mix") | ||
| return | ||
| } | ||
|
|
||
| do { | ||
| // Fetch mix queue from API | ||
| let result = try await client.getMixQueue(playlistId: playlistId, startVideoId: startVideoId) | ||
| guard !result.songs.isEmpty else { | ||
| self.logger.warning("Mix queue returned empty") | ||
| return | ||
| } | ||
|
|
||
| // Store continuation token for infinite mix | ||
| self.mixContinuationToken = result.continuationToken | ||
|
|
||
| // Shuffle the queue to get a different order each time | ||
| // YouTube's API returns a personalized but consistent order per session, | ||
| // so we shuffle to give the user variety on each Mix button click | ||
| let shuffledSongs = result.songs.shuffled() | ||
|
|
||
| // Set up the queue and play the first song | ||
| self.queue = shuffledSongs | ||
| self.currentIndex = 0 | ||
| self.currentTrack = shuffledSongs[0] | ||
|
|
||
| // Start playback | ||
| await self.play(videoId: shuffledSongs[0].videoId) | ||
|
|
||
| self.logger.info("Mix queue loaded with \(shuffledSongs.count) songs, hasContinuation: \(result.continuationToken != nil)") | ||
| } catch { | ||
| self.logger.warning("Failed to fetch mix queue: \(error.localizedDescription)") | ||
| } | ||
| } |
There was a problem hiding this comment.
The new playWithMix functionality lacks test coverage. The repository has comprehensive tests for playWithRadio in PlayerServiceTests.swift (lines 370-513), but no tests for playWithMix, getMixQueue, or getMixQueueContinuation. Consider adding tests that verify:
- playWithMix starts playback immediately
- Mix queue fetching handles empty responses
- Continuation tokens are properly stored and cleared
- fetchMoreMixSongsIfNeeded triggers at the right threshold
- Duplicate songs are filtered when fetching continuations
| } catch { | ||
| self.logger.warning("Failed to fetch more mix songs: \(error.localizedDescription)") | ||
| } |
There was a problem hiding this comment.
When fetching more mix songs fails (line 636), the continuation token is not cleared. This means the next call to fetchMoreMixSongsIfNeeded will try again with the same token, which could repeatedly fail. While this provides retry behavior, it could also lead to repeated failed API calls if the token is invalid or expired. Consider clearing the continuation token on persistent failures or implementing exponential backoff to avoid excessive failed requests.
Views/macOS/ArtistDetailView.swift
Outdated
| Label("Mix", systemImage: "play.circle") | ||
| // Mix button - plays personalized radio with mix of artists | ||
| // Only shown if mix data is available from the API | ||
| // Passing nil for startVideoId lets the API pick a random starting point each time |
There was a problem hiding this comment.
The comment mentions "Passing nil for startVideoId lets the API pick a random starting point each time" which is accurate based on the implementation. However, since the code then shuffles the returned songs on line 579, the randomization happens both from the API (random start) and from local shuffling. This double-randomization might not be immediately clear to future maintainers. Consider clarifying whether both levels of randomization are intentional or if one could be removed.
| // Passing nil for startVideoId lets the API pick a random starting point each time | |
| // Passing nil for startVideoId lets the API pick a random starting point on the server | |
| // in addition to any client-side shuffling applied when the mix tracks are played. |
Views/macOS/ArtistDetailView.swift
Outdated
| } | ||
| .buttonStyle(.bordered) | ||
| .controlSize(.large) | ||
| .disabled(detail.songs.isEmpty) |
There was a problem hiding this comment.
The Shuffle button now fetches all songs if needed before shuffling, but it's disabled when detail.songs.isEmpty. This check may not account for the case where detail.songs is empty but detail.hasMoreSongs is true, meaning there are songs available via API that could be fetched. Consider checking if there are any songs at all (loaded or fetchable) rather than just checking if the current list is empty, or document that the button should remain disabled until at least some songs are loaded.
| .disabled(detail.songs.isEmpty) | |
| .disabled(detail.songs.isEmpty && !detail.hasMoreSongs) |
fixes #21