Add ability to reorder playlists on sidebar using drag and drop - #209
Add ability to reorder playlists on sidebar using drag and drop#209ggtrigg wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughAdds durable playlist sidebar ordering. GTK drag-and-drop emits validated reorder actions, ChangesPlaylist sidebar ordering
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Complexity | 4 medium 2 minor |
🟢 Metrics 147 complexity · 22 duplication
Metric Results Complexity 147 Duplication 22
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/db/migration/m20260807_000019_playlist_sidebar_order.rs (1)
176-201: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant named arguments.
TABLEandwhenare already in scope, so the inline captures resolve without the explicitTABLE = TABLEandwhen = whenarguments. Removing them shortens the call and avoids the impression thatTABLEis a local binding.♻️ Proposed cleanup
name = trigger.name, operation = trigger.operation, - TABLE = TABLE, - when = when, revision_table = REVISION_TABLE, singleton = REVISION_SINGLETON, max_revision = i64::MAX,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/migration/m20260807_000019_playlist_sidebar_order.rs` around lines 176 - 201, Remove the redundant TABLE = TABLE and when = when named arguments from the format! call constructing the trigger in the playlist sidebar migration; rely on the existing in-scope inline captures while preserving all other arguments and generated SQL.src/local/playlist_manager.rs (1)
865-879: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider one multi-row INSERT instead of a loop.
The loop runs one statement per playlist. Each statement is a separate round trip and fires the revision trigger once. A single
INSERT ... VALUES (?,?),(?,?),...reduces the round trips to one. Sidebar playlist counts are normally small, so this is a throughput improvement rather than a correctness fix. If you keep the loop, the current behavior stays correct because the whole sequence runs in one transaction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/local/playlist_manager.rs` around lines 865 - 879, In the sidebar ordering update around the transaction’s DELETE and playlist insertion loop, replace the per-playlist INSERT statements with one multi-row INSERT containing all ordered_ids values, while preserving the existing i64 position conversion and empty-list behavior. Keep the operation within the same transaction and retain the existing overflow error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ui/playlist_actions.rs`:
- Around line 487-505: Serialize playlist reorder persistence so action-receipt
order is preserved through PlaylistManager::set_sidebar_order; prevent a later
reorder from being overtaken by an earlier database task. Update the reorder
dispatch around rt_handle.spawn to queue writes or enforce a monotonic request
sequence, while retaining existing success and error outcomes. Add a test that
delays the first write and verifies the second reorder is the final stored
order.
---
Nitpick comments:
In `@src/db/migration/m20260807_000019_playlist_sidebar_order.rs`:
- Around line 176-201: Remove the redundant TABLE = TABLE and when = when named
arguments from the format! call constructing the trigger in the playlist sidebar
migration; rely on the existing in-scope inline captures while preserving all
other arguments and generated SQL.
In `@src/local/playlist_manager.rs`:
- Around line 865-879: In the sidebar ordering update around the transaction’s
DELETE and playlist insertion loop, replace the per-playlist INSERT statements
with one multi-row INSERT containing all ordered_ids values, while preserving
the existing i64 position conversion and empty-list behavior. Keep the operation
within the same transaction and retain the existing overflow error.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cf8db42a-59ae-4c1e-980c-41a72266def5
📒 Files selected for processing (6)
src/db/migration/m20260807_000019_playlist_sidebar_order.rssrc/db/migration/mod.rssrc/local/playlist_manager.rssrc/local/playlist_sidebar.rssrc/ui/playlist_actions.rssrc/ui/sidebar.rs
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This PR implements drag-and-drop playlist reordering with persistent storage, but it is currently not up to standards according to Codacy analysis. The main concerns involve high cyclomatic complexity in the migration logic and the PlaylistManager, along with a logic gap in how the UI handles no-op reorder operations.
While the backend supports short-circuiting identical permutations, the UI does not consistently prevent dispatching these redundant updates. This can lead to unnecessary database transactions and potential error dialogs being shown to the user when a successful no-op occurs. These issues should be addressed to ensure the efficiency and stability of the reordering feature.
About this PR
- The implementation of the 'ignore no-op' requirement is incomplete. Redundant database writes and potential false-positive error dialogs occur because the UI does not verify if a permutation has actually changed before dispatching the reorder action.
Test suggestions
- Database migration correctly installs the sidebar order table and the three triggers for revision bumping.
- PlaylistManager rejects invalid reorder requests (missing IDs, duplicates) and rolls back changes.
- PlaylistManager short-circuits no-op reorder requests to avoid unnecessary database operations and revision increments.
- Sidebar SQL query correctly implements the sort priority: explicit positions first, followed by unordered items by creation date.
- UI logic correctly calculates the new ID list permutation when dragging a playlist before or after a target row.
- UI logic correctly extracts only playlist IDs from the sidebar store, ignoring headers or other source types.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| Ok(()) | ||
| } | ||
|
|
||
| async fn validate_triggers(manager: &SchemaManager<'_>) -> Result<(), DbErr> { |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The function exceeds complexity and length limits because it combines individual trigger validation with global table-state verification. Splitting these into two separate functions will make the migration easier to debug.
| /// participate: sidebar order is presentation state, never a content edit. | ||
| /// Rows are stored as contiguous positions; a playlist without an order | ||
| /// row keeps the historical `created_at` fallback ordering. | ||
| pub async fn set_sidebar_order(&self, ordered_ids: &[String]) -> Result<(), DbErr> { |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The method logic should be split into validation and persistence phases. Moving the permutation check and the insertion loop into private helper methods will improve readability and maintainability.
| Some(ids) | ||
| } |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Avoid dispatching a database update if the drag-and-drop operation doesn't actually change the permutation. Adding an equality check here prevents redundant transactions.
| execute( | ||
| db, | ||
| format!( | ||
| "INSERT INTO playlist_sidebar_order (playlist_id, position) VALUES ({},{})", |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: Add a space after the comma in the VALUES clause for consistency with standard SQL formatting used elsewhere in the project.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ui/playlist_actions.rs (1)
1216-1222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a paused clock for the delay.
The test sleeps 200 ms of real time.
#[tokio::test(start_paused = true)]makestokio::time::sleepauto-advance, so the ordering assertion stays valid and the test finishes immediately.♻️ Proposed change
- #[tokio::test] + #[tokio::test(start_paused = true)] async fn reorder_writes_commit_in_receipt_order_when_first_is_slow() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/playlist_actions.rs` around lines 1216 - 1222, Update the reorder worker test around the slow_first delay to run with Tokio’s paused clock by enabling start_paused on the tokio test, while retaining tokio::time::sleep for the 200 ms delay so virtual time advances and the ordering assertion remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ui/playlist_actions.rs`:
- Around line 541-553: Update the comment above the reorder_tx.try_send failure
path to state that a closed queue causes an immediate silent return during
teardown and does not reach the receiver; reserve the receiver’s failure
description for dropped result_tx cases. Add diagnostic logging when try_send
fails, while preserving the existing return behavior.
---
Nitpick comments:
In `@src/ui/playlist_actions.rs`:
- Around line 1216-1222: Update the reorder worker test around the slow_first
delay to run with Tokio’s paused clock by enabling start_paused on the tokio
test, while retaining tokio::time::sleep for the 200 ms delay so virtual time
advances and the ordering assertion remains unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bbc53145-fc19-400e-8561-a51cdc8883ab
📒 Files selected for processing (2)
src/local/playlist_sidebar.rssrc/ui/playlist_actions.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/local/playlist_sidebar.rs
| // Queue the write behind any earlier reorder so the final stored order | ||
| // matches the last received action. If the worker is gone (window | ||
| // teardown) the queue is closed and the dropped result channel is | ||
| // treated as a failure by the receiver below. | ||
| if reorder_tx | ||
| .try_send(SidebarReorderRequest { | ||
| ordered_ids, | ||
| result_tx, | ||
| }) | ||
| .is_err() | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the comment: the closed-queue path never reaches the receiver.
The comment states that a closed queue is "treated as a failure by the receiver below". That is not what the code does. When try_send fails, handle_reorder returns at Line 552, before spawn_local. The receiver never runs and no alert appears. The dropped-result_tx case is the only path that reaches the receiver.
Keeping the silent return is reasonable during window teardown. Fix the comment so it describes the actual behavior, and log the drop for diagnosis.
📝 Proposed comment and logging fix
// Queue the write behind any earlier reorder so the final stored order
- // matches the last received action. If the worker is gone (window
- // teardown) the queue is closed and the dropped result channel is
- // treated as a failure by the receiver below.
+ // matches the last received action. If the worker is gone (window
+ // teardown) the queue is closed and the reorder is dropped without an
+ // alert, because the window is already going away. A worker that is
+ // dropped mid-write closes `result_tx` instead, and the receiver below
+ // reports that as a failure.
if reorder_tx
.try_send(SidebarReorderRequest {
ordered_ids,
result_tx,
})
.is_err()
{
+ warn!("Sidebar reorder queue closed; dropping reorder request");
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Queue the write behind any earlier reorder so the final stored order | |
| // matches the last received action. If the worker is gone (window | |
| // teardown) the queue is closed and the dropped result channel is | |
| // treated as a failure by the receiver below. | |
| if reorder_tx | |
| .try_send(SidebarReorderRequest { | |
| ordered_ids, | |
| result_tx, | |
| }) | |
| .is_err() | |
| { | |
| return; | |
| } | |
| // Queue the write behind any earlier reorder so the final stored order | |
| // matches the last received action. If the worker is gone (window | |
| // teardown) the queue is closed and the reorder is dropped without an | |
| // alert, because the window is already going away. A worker that is | |
| // dropped mid-write closes `result_tx` instead, and the receiver below | |
| // reports that as a failure. | |
| if reorder_tx | |
| .try_send(SidebarReorderRequest { | |
| ordered_ids, | |
| result_tx, | |
| }) | |
| .is_err() | |
| { | |
| warn!("Sidebar reorder queue closed; dropping reorder request"); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/playlist_actions.rs` around lines 541 - 553, Update the comment above
the reorder_tx.try_send failure path to state that a closed queue causes an
immediate silent return during teardown and does not reach the receiver; reserve
the receiver’s failure description for dropped result_tx cases. Add diagnostic
logging when try_send fails, while preserving the existing return behavior.
1 participant
For your consideration.
This PR allows the playlists listed in the sidebar to be reordered using drag and drop.
A functionality I, at least, find useful.
Summary by CodeRabbit