Folders#92
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ab9a515ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // Update app state with complete data | ||
| let mut app = app.lock().await; | ||
| app.all_playlists.extend(remaining_playlists); |
There was a problem hiding this comment.
The background completion path appends only app.all_playlists and never refreshes app.playlists, so the two playlist sources diverge after page 1. My Playlists flows that still resolve IDs from app.playlists[selected_playlist_index] (for example in src/handlers/track_table.rs and src/handlers/sort_menu.rs) can then target the wrong playlist or no playlist when the selection comes from folder views or playlists beyond the first page.
Useful? React with 👍 / 👎.
| #[cfg(feature = "streaming")] | ||
| { | ||
| let streaming_player = self.streaming_player.clone(); | ||
| tokio::spawn(async move { |
There was a problem hiding this comment.
Each call to get_current_user_playlists launches a detached background fetch without any generation/cancellation guard, and refreshes are triggered repeatedly by follow/unfollow actions. If a second refresh starts before the first task finishes, both tasks can race to mutate playlist state, and because the task appends results later, stale or duplicate entries can be introduced depending on completion order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR restores proper playlist folder support in the “My Playlists” panel and fixes missing playlists beyond the first page by introducing an all-playlists store and (when streaming is enabled) building a folder hierarchy from Spotify rootlist data.
Changes:
- Fetch first page immediately, then paginate remaining playlists in a background task; maintain a flat
all_playlistslist. - Add folder-aware playlist navigation (
PlaylistFolderNode/PlaylistFolderItem) and render folder entries in the UI. - Update playlist open/play/unfollow flows to use
all_playlistsindexing (instead of first-page indexing).
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/network.rs | Adds background pagination + optional rootlist folder parsing and folder-item construction. |
| src/app.rs | Adds new playlist state (all_playlists, folder nodes/items) and helpers for folder-aware display. |
| src/ui/mod.rs | Renders folder-aware playlist list with folder/back entries. |
| src/handlers/playlist.rs | Updates playlist panel navigation and actions to operate on folder-aware items. |
| src/handlers/track_table.rs | Updates playback context lookup to use all_playlists via active_playlist_index. |
| src/player/streaming.rs | Exposes librespot Session for rootlist calls. |
| Cargo.toml | Adds optional librespot-protocol + protobuf under the streaming feature. |
| Cargo.lock | Locks new streaming-related dependencies. |
|
|
||
| app.playlist_folder_nodes = folder_nodes; | ||
| app.playlist_folder_items = folder_items; |
There was a problem hiding this comment.
After rebuilding playlist_folder_items from rootlist, the ordering/contents of the visible playlist list can change (folders inserted, playlists moved). Updating playlist_folder_items without reconciling current_playlist_folder_id and selected_playlist_index can cause the UI selection to point at a different item than before (and potentially make the current folder id invalid). Consider resetting selection/folder to a safe default or preserving the selected playlist/folder by identity when replacing the list.
| }); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
parse_rootlist_items doesn’t unwind any still-open start-group sections at the end of the loop. If the rootlist data is inconsistent (missing end-group), this will drop all previously accumulated siblings stored in stack and return only the innermost partial group. Consider draining name_stack/stack after the loop to wrap remaining children and restore the full root vector.
| // If there are any still-open groups (missing `end-group`), | |
| // unwind them now so we don't lose accumulated siblings in `stack`. | |
| while let Some((group_id, name)) = name_stack.pop() { | |
| let children = std::mem::take(&mut root); | |
| root = stack.pop().unwrap_or_default(); | |
| root.push(PlaylistFolderNode { | |
| name: Some(name), | |
| node_type: "folder".to_string(), | |
| uri: format!("spotify:folder:{}", group_id), | |
| children, | |
| }); | |
| } |
| let playlist_items: Vec<String> = if display_items.is_empty() { | ||
| // Fallback: show flat list from legacy playlists field | ||
| match &app.playlists { | ||
| Some(p) => p.items.iter().map(|item| item.name.to_owned()).collect(), | ||
| None => vec![], | ||
| } | ||
| } else { |
There was a problem hiding this comment.
The fallback to the legacy flat playlist list is gated on display_items.is_empty(), which can also happen when playlist_folder_items exists but current_playlist_folder_id doesn’t match any entries (or if a folder view is temporarily empty). In that case the UI would unexpectedly show the legacy flat list while handlers still operate on folder-aware indices. Consider basing the fallback on whether playlist_folder_items is empty/uninitialized (or resetting current_playlist_folder_id when it becomes invalid) rather than on the filtered result.
| pub struct PlaylistFolderNode { | ||
| pub name: Option<String>, | ||
| pub node_type: String, // "folder" or "playlist" | ||
| pub uri: String, | ||
| pub children: Vec<PlaylistFolderNode>, |
There was a problem hiding this comment.
PlaylistFolderNode.node_type is a free-form String but the code relies on specific values ("folder" vs "playlist") for control flow. Using a string here makes invalid states representable and adds allocations; consider making this an enum (and parsing into that) to improve type-safety and avoid unexpected fallthroughs.
| pub fn get_playlist_display_items(&self) -> Vec<&PlaylistFolderItem> { | ||
| self | ||
| .playlist_folder_items | ||
| .iter() | ||
| .filter(|item| match item { | ||
| PlaylistFolderItem::Folder(f) => f.current_id == self.current_playlist_folder_id, |
There was a problem hiding this comment.
get_playlist_display_items allocates a new Vec of references and linearly scans playlist_folder_items on every call. This method is invoked in render paths and key handlers, so with large libraries it can become a noticeable CPU hot spot. Consider returning an iterator (or cached per-folder index lists) to avoid repeated allocations and full scans each frame.
| let context_id = match &app.active_playlist_index { | ||
| Some(active_playlist_index) => app | ||
| .all_playlists | ||
| .get(active_playlist_index.to_owned()) | ||
| .map(|selected_playlist| playlist_context_id_from_ref(&selected_playlist.id)), | ||
| _ => None, |
There was a problem hiding this comment.
selected_playlist_index now refers to an entry in the folder-aware display list, but much of this file still uses (&app.playlists, &app.selected_playlist_index) to resolve the active playlist for pagination and random playback. Once folders are enabled or playlists exceed the first page, those code paths can fetch/play tracks from the wrong playlist or fail to paginate. Consider resolving playlist IDs via active_playlist_index (set when opening) and all_playlists, and using that consistently for all TrackTableContext::MyPlaylists actions.
| /// Fetch all user playlists (all pages) and populate folder hierarchy | ||
| #[allow(dead_code)] | ||
| GetAllPlaylistsAndFolders, |
There was a problem hiding this comment.
GetAllPlaylistsAndFolders is added to IoEvent but is not dispatched anywhere in the codebase (only referenced in this match). Keeping an unused public event (even with allow(dead_code)) increases surface area and maintenance overhead; either wire it up where playlists are requested or remove it and rely on the existing background pagination path.
| /// Fetch all user playlists (all pages) and populate folder hierarchy | |
| #[allow(dead_code)] | |
| GetAllPlaylistsAndFolders, |
| offset += items_count; | ||
| } | ||
| Err(_) => break, | ||
| } |
There was a problem hiding this comment.
This background pagination loop silently stops on errors (Err(_) => break) without logging or surfacing anything to the user. That can leave the app permanently showing an incomplete playlist list with no indication why. Consider at least logging the error (or calling handle_error) and/or retrying with backoff.
…on, and clean warnings
This PR fixes playlist folder support in My Playlists and resolves the root cause of missing playlists beyond the first page.
It now fetches all user playlists (not just the first 50), builds folder-aware playlist navigation from Spotify rootlist data when streaming is enabled, and updates playlist actions (open/play/unfollow) to work correctly inside nested folders.
It also includes follow-up stability fixes:
Testing
Manual verification performed:
Additional notes