Adds logs when importing songs and Refreshes unplayed songs when loading new playlist #744
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves playlist management by adding progress logging during playlist imports and ensuring the unplayed songs queue is properly refreshed when loading playlists.
- Adds progress logging to track song import operations
- Fixes the unplayed songs queue to refresh when loading a new playlist
- Clears the unplayed songs queue before repopulating to avoid duplicates
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }; | ||
| } | ||
|
|
||
| var totalSongs = songLinks.Length; |
There was a problem hiding this comment.
The total song count represents all lines in the file, not the number of songs that will actually be imported. Songs can be skipped if they're null or already exist in the playlist (lines 654-661). This makes the progress log misleading since the denominator doesn't represent the actual work to be done. Consider either using a more accurate description in the log message or calculating the total after filtering.
| importedSongs++; | ||
| _logger.LogInformation("Imported {importedSongs}/{totalSongs} songs", importedSongs, totalSongs); | ||
| playList.Songs.Add(song); |
There was a problem hiding this comment.
The counter is incremented before the song is actually added to the playlist. If an exception occurs during the Add operation on line 665, the counter will be inaccurate. Consider moving the increment after the song has been successfully added to the playlist.
| importedSongs++; | |
| _logger.LogInformation("Imported {importedSongs}/{totalSongs} songs", importedSongs, totalSongs); | |
| playList.Songs.Add(song); | |
| playList.Songs.Add(song); | |
| importedSongs++; | |
| _logger.LogInformation("Imported {importedSongs}/{totalSongs} songs", importedSongs, totalSongs); |
| } | ||
| song.RequestedBy = ServiceBackbone.BotName ?? "TheBot"; | ||
| importedSongs++; | ||
| _logger.LogInformation("Imported {importedSongs}/{totalSongs} songs", importedSongs, totalSongs); |
There was a problem hiding this comment.
Logging on every iteration could produce excessive log output for large playlists. For a playlist with hundreds or thousands of songs, this will create a log entry for each song imported. Consider logging at intervals (e.g., every 10 or 100 songs) or using LogDebug instead of LogInformation, and only logging a summary at the end.
| _logger.LogInformation("Imported {importedSongs}/{totalSongs} songs", importedSongs, totalSongs); | |
| if (importedSongs % 50 == 0 || importedSongs == totalSongs) | |
| { | |
| _logger.LogInformation("Imported {importedSongs}/{totalSongs} songs", importedSongs, totalSongs); | |
| } |
No description provided.