Improve cover editing and playlist windows - #38
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces several UI and backend enhancements, including drag-and-drop reordering for playlist tabs (backed by a new sort_order database column), refactoring YouTube discover and download dialogs to use immediate viewports, and adding support for changing track cover art directly from the UI. Feedback on these changes highlights a bug where clearing the dragging state inside the loop breaks rightward tab dragging, as well as performance concerns regarding marking all playlists as dirty during reordering and executing multiple database updates outside of a single transaction.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for playlist in playlists.iter_mut() { | ||
| playlist.is_dirty = true; | ||
| } |
There was a problem hiding this comment.
Setting playlist.is_dirty = true for all playlists on every drag-and-drop reorder operation is a significant performance bottleneck.\n\nWhen a playlist is marked as dirty, the subsequent save_state() call will completely rewrite the playlist's metadata and all of its tracks (deleting and re-inserting them) in the database. If a user has multiple playlists or large playlists, this will cause a noticeable UI freeze during dragging.\n\nSince save_playlists already updates the sort_order column for all playlists regardless of their dirty status, we can safely remove this loop entirely.
| if tab_response.drag_stopped() { | ||
| ctx.ui_state.playlist_tab_dragging = None; | ||
| } |
There was a problem hiding this comment.
Clearing the playlist_tab_dragging state immediately inside the loop when tab_response.drag_stopped() is detected introduces a bug where dragging a tab to the right (to a higher index) fails or is extremely unreliable.\n\nWhen dragging tab A (lower index) to the right over tab B (higher index) and releasing:\n1. Tab A is processed first in the loop. Since the drag has stopped, tab_response.drag_stopped() is true, which immediately clears playlist_tab_dragging to None.\n2. When tab B is processed later in the same loop, playlist_tab_dragging is already None, so the drop/reorder logic is never triggered.\n\nSince you already have a global cleanup check at the end of show_tabs (lines 139-141) that clears the dragging state when the pointer is released, you can safely remove this drag_stopped() check from inside the loop. This ensures the dragging state remains available for all tabs during the drop frame.
| for (sort_order, playlist) in playlists.iter_mut().enumerate() { | ||
| playlist.save_to_db_and_update_id(db_conn)?; | ||
| if let Some(id) = playlist.id { | ||
| let conn = db_conn.lock().unwrap(); | ||
| conn.execute( | ||
| "UPDATE playlists SET sort_order = ?1 WHERE id = ?2", | ||
| rusqlite::params![sort_order as i64, id], | ||
| )?; | ||
| } | ||
| } |
There was a problem hiding this comment.
Updating the sort_order of each playlist by locking the database connection and executing a separate query inside the loop is highly inefficient. In SQLite, executing multiple independent update statements outside of a transaction results in a separate transaction (and disk sync) for each statement, which can be extremely slow.\n\nWe can optimize this by first saving any dirty playlists, and then locking the connection once to update all sort_order values within a single database transaction.
for playlist in playlists.iter_mut() {
playlist.save_to_db_and_update_id(db_conn)?;
}
let mut conn = db_conn.lock().unwrap();
let tx = conn.transaction()?;
{
let mut stmt = tx.prepare("UPDATE playlists SET sort_order = ?1 WHERE id = ?2")?;
for (sort_order, playlist) in playlists.iter().enumerate() {
if let Some(id) = playlist.id {
stmt.execute(rusqlite::params![sort_order as i64, id])?;
}
}
}
tx.commit()?;
Summary
Verification