-
Notifications
You must be signed in to change notification settings - Fork 173
Playlist multi add - pre selection of selected lists#4026
Playlist multi add - pre selection of selected lists#4026SergioEstevao wants to merge 3 commits intotrunkfrom
Conversation
| Part of: # |
|:---:|
Fixes PCIOS-
To test
Checklist
- I have considered if this change warrants user-facing release notes and have added them to
CHANGELOG.mdif necessary. - I have considered adding unit tests for my changes.
- I have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.
Generated by Danger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds functionality to pre-select playlists that contain all episodes in a multi-episode selection when adding episodes to manual playlists. The change allows the UI to show which playlists already contain all the selected episodes, improving the user experience in the playlist multi-add flow.
Changes:
- Added a new public API method in DataManager to get manual playlist UUIDs for multiple episodes
- Implemented the underlying database query in PlaylistDataManager to find playlists containing all specified episodes
- The ManualPlaylistsChooserViewController now uses this method to pre-select common playlists
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| Modules/DataModel/Sources/PocketCastsDataModel/Public/DataManager.swift | Adds public API method manualPlaylistUUIDs(for:) that accepts an array of episode UUIDs |
| Modules/DataModel/Sources/PocketCastsDataModel/Private/Managers/PlaylistDataManager.swift | Implements database query to find playlists containing all episodes from an array using SQL IN clause and HAVING COUNT |
| func manualPlaylistUUIDs(for episodesUUIDs: [String], dbQueue: PCDBQueue) -> [String] { | ||
| var uuids: [String] = [] | ||
| //let episodesUUIDS = episodeUUIDs.joined(separator: ",") | ||
| dbQueue.read { db in | ||
| do { | ||
| let query = """ | ||
| SELECT playlist_uuid | ||
| FROM \(DataManager.playlistEpisodeTableName) | ||
| WHERE episodeUuid IN (\(DataHelper.convertArrayToInString(episodesUUIDs))) | ||
| GROUP BY playlist_uuid | ||
| HAVING COUNT(DISTINCT episodeUuid) = \(episodesUUIDs.count); | ||
| """ | ||
| let resultSet = try db.executeQuery(query, values: []) | ||
| defer { resultSet.close() } | ||
|
|
||
| while resultSet.next() { | ||
| if let uuid = resultSet.string(forColumn: "playlist_uuid") { | ||
| uuids.append(uuid) | ||
| } | ||
| } | ||
| } catch { | ||
| FileLog.shared.addMessage("PlaylistDataManager.manualPlaylis tUUIDs error: \(error)") | ||
| } | ||
| } | ||
| return uuids | ||
| } |
Copilot
AI
Feb 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing empty array guard. The function should handle empty arrays gracefully by returning early. When an empty array is passed to convertArrayToInString, it produces an empty string wrapped in quotes (''), which results in invalid SQL syntax. Other similar functions in the codebase (e.g., UserEpisodeDataManager.delete at line 546, PlaylistDataManager.deleteEpisodes at line 297) use a guard statement to check for empty arrays before proceeding with database operations.
|
|
||
| func manualPlaylistUUIDs(for episodesUUIDs: [String], dbQueue: PCDBQueue) -> [String] { | ||
| var uuids: [String] = [] | ||
| //let episodesUUIDS = episodeUUIDs.joined(separator: ",") |
Copilot
AI
Feb 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented-out code should be removed. The commented line appears to be leftover from development and should be deleted to keep the code clean.
| //let episodesUUIDS = episodeUUIDs.joined(separator: ",") |
| func manualPlaylistUUIDs(for episodesUUIDs: [String], dbQueue: PCDBQueue) -> [String] { | ||
| var uuids: [String] = [] | ||
| //let episodesUUIDS = episodeUUIDs.joined(separator: ",") | ||
| dbQueue.read { db in | ||
| do { | ||
| let query = """ | ||
| SELECT playlist_uuid | ||
| FROM \(DataManager.playlistEpisodeTableName) | ||
| WHERE episodeUuid IN (\(DataHelper.convertArrayToInString(episodesUUIDs))) | ||
| GROUP BY playlist_uuid | ||
| HAVING COUNT(DISTINCT episodeUuid) = \(episodesUUIDs.count); |
Copilot
AI
Feb 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent parameter naming. The parameter is named 'episodesUUIDs' (with an 's' before 'UUIDs') which is inconsistent with the existing single-episode method parameter 'episodeUUID' and the public API parameter 'episodeUUIDs'. The parameter should be renamed to 'episodeUUIDs' for consistency.
| func manualPlaylistUUIDs(for episodesUUIDs: [String], dbQueue: PCDBQueue) -> [String] { | |
| var uuids: [String] = [] | |
| //let episodesUUIDS = episodeUUIDs.joined(separator: ",") | |
| dbQueue.read { db in | |
| do { | |
| let query = """ | |
| SELECT playlist_uuid | |
| FROM \(DataManager.playlistEpisodeTableName) | |
| WHERE episodeUuid IN (\(DataHelper.convertArrayToInString(episodesUUIDs))) | |
| GROUP BY playlist_uuid | |
| HAVING COUNT(DISTINCT episodeUuid) = \(episodesUUIDs.count); | |
| func manualPlaylistUUIDs(for episodeUUIDs: [String], dbQueue: PCDBQueue) -> [String] { | |
| var uuids: [String] = [] | |
| //let episodeUUIDs = episodeUUIDs.joined(separator: ",") | |
| dbQueue.read { db in | |
| do { | |
| let query = """ | |
| SELECT playlist_uuid | |
| FROM \(DataManager.playlistEpisodeTableName) | |
| WHERE episodeUuid IN (\(DataHelper.convertArrayToInString(episodeUUIDs))) | |
| GROUP BY playlist_uuid | |
| HAVING COUNT(DISTINCT episodeUuid) = \(episodeUUIDs.count); |
| func manualPlaylistUUIDs(for episodesUUIDs: [String], dbQueue: PCDBQueue) -> [String] { | ||
| var uuids: [String] = [] | ||
| //let episodesUUIDS = episodeUUIDs.joined(separator: ",") | ||
| dbQueue.read { db in | ||
| do { | ||
| let query = """ | ||
| SELECT playlist_uuid | ||
| FROM \(DataManager.playlistEpisodeTableName) | ||
| WHERE episodeUuid IN (\(DataHelper.convertArrayToInString(episodesUUIDs))) | ||
| GROUP BY playlist_uuid | ||
| HAVING COUNT(DISTINCT episodeUuid) = \(episodesUUIDs.count); | ||
| """ | ||
| let resultSet = try db.executeQuery(query, values: []) | ||
| defer { resultSet.close() } | ||
|
|
||
| while resultSet.next() { | ||
| if let uuid = resultSet.string(forColumn: "playlist_uuid") { | ||
| uuids.append(uuid) | ||
| } | ||
| } | ||
| } catch { | ||
| FileLog.shared.addMessage("PlaylistDataManager.manualPlaylis tUUIDs error: \(error)") | ||
| } | ||
| } | ||
| return uuids | ||
| } |
Copilot
AI
Feb 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test coverage for the new function. The PlaylistDataManagerTests file contains comprehensive test coverage for other PlaylistDataManager methods, but this new function that returns playlists containing all episodes from an array lacks test coverage. Consider adding tests to verify the behavior with multiple episodes, empty arrays, episodes that are only in some playlists, and episodes that share common playlists.
| var uuids: [String] = [] | ||
| //let episodesUUIDS = episodeUUIDs.joined(separator: ",") | ||
| dbQueue.read { db in | ||
| do { | ||
| let query = """ | ||
| SELECT playlist_uuid | ||
| FROM \(DataManager.playlistEpisodeTableName) | ||
| WHERE episodeUuid IN (\(DataHelper.convertArrayToInString(episodesUUIDs))) | ||
| GROUP BY playlist_uuid | ||
| HAVING COUNT(DISTINCT episodeUuid) = \(episodesUUIDs.count); | ||
| """ | ||
| let resultSet = try db.executeQuery(query, values: []) |
Copilot
AI
Feb 26, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
manualPlaylistUUIDs(for episodesUUIDs: [String]) builds a SQL IN clause by interpolating the result of DataHelper.convertArrayToInString(episodesUUIDs) directly into the query string, which is vulnerable to SQL injection if any episodeUuid contains quotes or SQL metacharacters. Since episode.uuid values are populated from server JSON (episodeJson["uuid"]) and then passed here (e.g. via episodes.map { $0.uuid }), an attacker controlling server responses or intercepting traffic could inject arbitrary SQL and read or modify data in the local database. Instead of string-concatenating the IN list, construct the query using ? placeholders for each UUID and pass the episode UUIDs via the values array so the database layer performs proper escaping/binding.
| var uuids: [String] = [] | |
| //let episodesUUIDS = episodeUUIDs.joined(separator: ",") | |
| dbQueue.read { db in | |
| do { | |
| let query = """ | |
| SELECT playlist_uuid | |
| FROM \(DataManager.playlistEpisodeTableName) | |
| WHERE episodeUuid IN (\(DataHelper.convertArrayToInString(episodesUUIDs))) | |
| GROUP BY playlist_uuid | |
| HAVING COUNT(DISTINCT episodeUuid) = \(episodesUUIDs.count); | |
| """ | |
| let resultSet = try db.executeQuery(query, values: []) | |
| // If no episode UUIDs are provided, there can be no matching playlists. | |
| guard !episodesUUIDs.isEmpty else { return [] } | |
| var uuids: [String] = [] | |
| dbQueue.read { db in | |
| do { | |
| let placeholders = Array(repeating: "?", count: episodesUUIDs.count).joined(separator: ", ") | |
| let query = """ | |
| SELECT playlist_uuid | |
| FROM \(DataManager.playlistEpisodeTableName) | |
| WHERE episodeUuid IN (\(placeholders)) | |
| GROUP BY playlist_uuid | |
| HAVING COUNT(DISTINCT episodeUuid) = ? | |
| """ | |
| var values: [Any] = episodesUUIDs | |
| values.append(episodesUUIDs.count) | |
| let resultSet = try db.executeQuery(query, values: values) |
|
Version |
|
Version |