From d15e95e6e855b6b66261de43d23647a53c043cb1 Mon Sep 17 00:00:00 2001 From: Glenn Trigg Date: Fri, 7 Aug 2026 20:26:37 +1000 Subject: [PATCH 1/4] Add ability to reorder playlists on sidebar using drag and drop --- ...m20260807_000019_playlist_sidebar_order.rs | 650 ++++++++++++++++++ src/db/migration/mod.rs | 5 +- src/local/playlist_manager.rs | 173 ++++- src/local/playlist_sidebar.rs | 38 +- src/ui/playlist_actions.rs | 57 ++ src/ui/sidebar.rs | 184 +++++ 6 files changed, 1103 insertions(+), 4 deletions(-) create mode 100644 src/db/migration/m20260807_000019_playlist_sidebar_order.rs diff --git a/src/db/migration/m20260807_000019_playlist_sidebar_order.rs b/src/db/migration/m20260807_000019_playlist_sidebar_order.rs new file mode 100644 index 00000000..d80a8a67 --- /dev/null +++ b/src/db/migration/m20260807_000019_playlist_sidebar_order.rs @@ -0,0 +1,650 @@ +//! Migration: persist the playlist-sidebar presentation order. +//! +//! The sidebar order is derived UI state, separate from playlist content and +//! from the durable revision table installed by migration 15. Rows record a +//! contiguous position for every playlist that has been explicitly reordered. +//! A missing row means "fall back to `created_at` order", so installs that +//! never reorder keep their historical ordering exactly. +//! +//! Three triggers advance the migration-15 revision on every insert, effective +//! update, and delete so the engine republishes the sidebar snapshot after a +//! reorder, including when the write arrives through raw SQL. + +use std::fmt; + +use sea_orm_migration::prelude::*; +use sea_orm_migration::sea_orm::{ + ConnectionTrait, DatabaseConnection, Statement, TransactionTrait, +}; + +const TABLE: &str = "playlist_sidebar_order"; +const REVISION_TABLE: &str = "playlist_sidebar_revision"; +const REVISION_SINGLETON: i64 = 1; +const POSITION_CHECK: &str = "ck_playlist_sidebar_order_position"; + +const INSERT_TRIGGER: &str = "trg_playlist_sidebar_revision_sidebar_order_insert"; +const UPDATE_TRIGGER: &str = "trg_playlist_sidebar_revision_sidebar_order_update"; +const DELETE_TRIGGER: &str = "trg_playlist_sidebar_revision_sidebar_order_delete"; + +const TRIGGERS: [TriggerDefinition; 3] = [ + TriggerDefinition::new(INSERT_TRIGGER, "INSERT"), + TriggerDefinition::new(UPDATE_TRIGGER, "UPDATE"), + TriggerDefinition::new(DELETE_TRIGGER, "DELETE"), +]; + +#[derive(Clone, Copy)] +struct TriggerDefinition { + name: &'static str, + operation: &'static str, +} + +impl TriggerDefinition { + const fn new(name: &'static str, operation: &'static str) -> Self { + Self { name, operation } + } +} + +#[derive(DeriveMigrationName)] +pub struct Migration; + +#[async_trait::async_trait] +impl MigrationTrait for Migration { + async fn up(&self, manager: &SchemaManager) -> Result<(), DbErr> { + migrate(manager, true).await + } + + async fn down(&self, manager: &SchemaManager) -> Result<(), DbErr> { + migrate(manager, false).await + } +} + +/// Revalidate the mutable order boundary even when the migration ledger is +/// current. The revision triggers are critical schema objects; a missing or +/// altered trigger would silently stop the sidebar from republishing after a +/// reorder. +pub(super) async fn revalidate(connection: &DatabaseConnection) -> Result<(), DbErr> { + validate_installation(&SchemaManager::new(connection)).await +} + +/// Own the complete DDL transaction so validation failures cannot leave a +/// partially altered boundary. +async fn migrate(manager: &SchemaManager<'_>, install: bool) -> Result<(), DbErr> { + let transaction = manager.get_connection().begin().await?; + let result = { + let manager = SchemaManager::new(&transaction); + if install { + create_or_validate(&manager).await + } else { + drop_or_validate_absent(&manager).await + } + }; + + match result { + Ok(()) => transaction.commit().await, + Err(error) => { + let rollback = transaction.rollback().await; + Err(preserve_original_error(error, rollback)) + } + } +} + +fn preserve_original_error(original: DbErr, rollback: Result<(), DbErr>) -> DbErr { + match rollback { + Ok(()) => original, + Err(rollback_error) => DbErr::Migration(format!( + "{original}; additionally failed to roll back playlist-sidebar order migration: \ + {rollback_error}" + )), + } +} + +async fn create_or_validate(manager: &SchemaManager<'_>) -> Result<(), DbErr> { + if target_objects_absent(manager).await? { + manager + .get_connection() + .execute_unprepared(&canonical_table_sql()) + .await?; + for trigger in TRIGGERS { + manager + .get_connection() + .execute_unprepared(&canonical_trigger_sql(trigger)) + .await?; + } + } + + validate_installation(manager).await +} + +async fn drop_or_validate_absent(manager: &SchemaManager<'_>) -> Result<(), DbErr> { + if target_objects_absent(manager).await? { + return Ok(()); + } + + validate_installation(manager).await?; + for trigger in TRIGGERS { + manager + .get_connection() + .execute_unprepared(&format!("DROP TRIGGER {}", trigger.name)) + .await?; + } + manager + .get_connection() + .execute_unprepared(&format!("DROP TABLE {TABLE}")) + .await?; + + if !target_objects_absent(manager).await? { + return Err(DbErr::Migration( + "playlist-sidebar order objects remained after downgrade".to_string(), + )); + } + Ok(()) +} + +async fn target_objects_absent(manager: &SchemaManager<'_>) -> Result { + let mut names = Vec::with_capacity(TRIGGERS.len() + 1); + names.push(TABLE); + names.extend(TRIGGERS.iter().map(|trigger| trigger.name)); + + for name in names { + if !objects_named(manager, name).await?.is_empty() { + return Ok(false); + } + } + Ok(true) +} + +fn canonical_table_sql() -> String { + format!( + "CREATE TABLE {TABLE} ( + playlist_id TEXT PRIMARY KEY NOT NULL + REFERENCES playlists(id) ON DELETE CASCADE, + position INTEGER NOT NULL, + CONSTRAINT {POSITION_CHECK} CHECK ( + typeof(position) = 'integer' AND position >= 0 + ) + ) WITHOUT ROWID" + ) +} + +fn canonical_trigger_sql(trigger: TriggerDefinition) -> String { + let when = if trigger.operation == "UPDATE" { + "WHEN OLD.playlist_id IS NOT NEW.playlist_id + OR OLD.position IS NOT NEW.position" + } else { + "" + }; + format!( + "CREATE TRIGGER {name} + AFTER {operation} ON {TABLE} + {when} + BEGIN + SELECT CASE + WHEN NOT EXISTS ( + SELECT 1 FROM {revision_table} WHERE singleton = {singleton} + ) THEN RAISE(ABORT, 'playlist sidebar revision singleton missing') + WHEN ( + SELECT revision FROM {revision_table} WHERE singleton = {singleton} + ) = {max_revision} + THEN RAISE(ABORT, 'playlist sidebar revision exhausted') + END; + UPDATE {revision_table} + SET revision = revision + 1 + WHERE singleton = {singleton}; + END", + name = trigger.name, + operation = trigger.operation, + TABLE = TABLE, + when = when, + revision_table = REVISION_TABLE, + singleton = REVISION_SINGLETON, + max_revision = i64::MAX, + ) +} + +#[derive(Eq, PartialEq)] +struct SchemaObject { + object_type: String, + table_name: String, + sql: Option, +} + +impl fmt::Debug for SchemaObject { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter + .debug_struct("SchemaObject") + .field("object_type", &self.object_type) + .field("table_name_byte_len", &self.table_name.len()) + .field("sql_present", &self.sql.is_some()) + .finish() + } +} + +async fn objects_named( + manager: &SchemaManager<'_>, + name: &str, +) -> Result, DbErr> { + manager + .get_connection() + .query_all_raw(Statement::from_sql_and_values( + manager.get_database_backend(), + "SELECT type, tbl_name, sql FROM sqlite_master WHERE name = ? ORDER BY type, tbl_name", + [name.into()], + )) + .await? + .into_iter() + .map(|row| { + Ok(SchemaObject { + object_type: row.try_get("", "type")?, + table_name: row.try_get("", "tbl_name")?, + sql: row.try_get("", "sql")?, + }) + }) + .collect() +} + +async fn validate_installation(manager: &SchemaManager<'_>) -> Result<(), DbErr> { + validate_table_object(manager).await?; + validate_columns(manager).await?; + validate_triggers(manager).await?; + validate_revision_boundary(manager).await +} + +async fn validate_table_object(manager: &SchemaManager<'_>) -> Result<(), DbErr> { + let objects = objects_named(manager, TABLE).await?; + let [object] = objects.as_slice() else { + return Err(DbErr::Migration(format!( + "{TABLE} must resolve to exactly one table object, found {objects:?}" + ))); + }; + if object.object_type != "table" || object.table_name != TABLE { + return Err(DbErr::Migration(format!( + "{TABLE} must be a table owned by itself, found {object:?}" + ))); + } + let actual = object + .sql + .as_deref() + .ok_or_else(|| DbErr::Migration(format!("{TABLE} SQL is missing")))?; + if canonical_sql(actual) != canonical_sql(&canonical_table_sql()) { + return Err(DbErr::Migration(format!( + "{TABLE} does not have the exact canonical table definition" + ))); + } + Ok(()) +} + +type ColumnSchema = (i32, String, String, i32, Option, i32); + +async fn validate_columns(manager: &SchemaManager<'_>) -> Result<(), DbErr> { + let columns = manager + .get_connection() + .query_all_raw(Statement::from_string( + manager.get_database_backend(), + format!("PRAGMA table_info('{TABLE}')"), + )) + .await? + .into_iter() + .map(|row| { + Ok::(( + row.try_get("", "cid")?, + row.try_get("", "name")?, + row.try_get::("", "type")?.to_ascii_lowercase(), + row.try_get("", "notnull")?, + row.try_get("", "dflt_value")?, + row.try_get("", "pk")?, + )) + }) + .collect::, _>>()?; + let expected = vec![ + (0, "playlist_id".to_string(), "text".to_string(), 1, None, 1), + (1, "position".to_string(), "integer".to_string(), 1, None, 0), + ]; + if columns != expected { + return Err(DbErr::Migration(format!( + "{TABLE} has an unexpected column schema: {columns:?}" + ))); + } + Ok(()) +} + +async fn validate_triggers(manager: &SchemaManager<'_>) -> Result<(), DbErr> { + for trigger in TRIGGERS { + let objects = objects_named(manager, trigger.name).await?; + let [object] = objects.as_slice() else { + return Err(DbErr::Migration(format!( + "{} must resolve to exactly one trigger object, found {objects:?}", + trigger.name + ))); + }; + if object.object_type != "trigger" || object.table_name != TABLE { + return Err(DbErr::Migration(format!( + "{} has an unexpected object type or owner: {object:?}", + trigger.name + ))); + } + let actual = object + .sql + .as_deref() + .ok_or_else(|| DbErr::Migration(format!("{} SQL is missing", trigger.name)))?; + if canonical_sql(actual) != canonical_sql(&canonical_trigger_sql(trigger)) { + return Err(DbErr::Migration(format!( + "{} does not have the exact canonical trigger definition", + trigger.name + ))); + } + } + + let mut actual = manager + .get_connection() + .query_all_raw(Statement::from_sql_and_values( + manager.get_database_backend(), + "SELECT name FROM sqlite_master WHERE type = 'trigger' AND tbl_name = ?", + [TABLE.into()], + )) + .await? + .into_iter() + .map(|row| row.try_get::("", "name")) + .collect::, _>>()?; + actual.sort(); + let mut expected = TRIGGERS + .iter() + .map(|trigger| (*trigger.name).to_string()) + .collect::>(); + expected.sort(); + if actual != expected { + return Err(DbErr::Migration(format!( + "{TABLE} has an unexpected trigger set (found {}, expected {})", + actual.len(), + expected.len() + ))); + } + Ok(()) +} + +/// The order boundary must stay attached to the revision singleton that +/// migration 15 owns. Losing it would leave reorders invisible to the engine. +async fn validate_revision_boundary(manager: &SchemaManager<'_>) -> Result<(), DbErr> { + let rows = manager + .get_connection() + .query_all_raw(Statement::from_string( + manager.get_database_backend(), + format!("SELECT singleton FROM {REVISION_TABLE}"), + )) + .await?; + let [row] = rows.as_slice() else { + return Err(DbErr::Migration(format!( + "{TABLE} requires exactly one {REVISION_TABLE} singleton row, found {}", + rows.len() + ))); + }; + let singleton: i64 = row.try_get("", "singleton")?; + if singleton != REVISION_SINGLETON { + return Err(DbErr::Migration(format!( + "{TABLE} revision boundary has an invalid singleton" + ))); + } + Ok(()) +} + +/// Normalize formatting and identifier quoting while preserving SQL string +/// literal contents. Validation still compares the complete statement. +fn canonical_sql(sql: &str) -> String { + let mut canonical = String::with_capacity(sql.len()); + let mut characters = sql.chars().peekable(); + + while let Some(character) = characters.next() { + match character { + '\'' => { + canonical.push('\''); + while let Some(literal_character) = characters.next() { + canonical.push(literal_character); + if literal_character == '\'' { + if characters.peek() == Some(&'\'') { + canonical.push(characters.next().expect("peeked quote exists")); + } else { + break; + } + } + } + } + '"' => append_quoted_identifier(&mut canonical, &mut characters, '"'), + '`' => append_quoted_identifier(&mut canonical, &mut characters, '`'), + '[' => append_quoted_identifier(&mut canonical, &mut characters, ']'), + character if character.is_ascii_whitespace() => {} + character => canonical.extend(character.to_lowercase()), + } + } + canonical +} + +fn append_quoted_identifier( + canonical: &mut String, + characters: &mut std::iter::Peekable, + closing_quote: char, +) where + I: Iterator, +{ + while let Some(character) = characters.next() { + if character == closing_quote { + if characters.peek() == Some(&closing_quote) { + canonical.extend(character.to_lowercase()); + characters.next(); + } else { + break; + } + } else { + canonical.extend(character.to_lowercase()); + } + } +} + +#[cfg(test)] +mod tests { + use sea_orm_migration::sea_orm::{ + ConnectionTrait, Database, DatabaseConnection, DbBackend, Statement, + }; + + use super::*; + use crate::db::migration::Migrator; + + async fn migrated_database() -> DatabaseConnection { + let db = Database::connect("sqlite::memory:") + .await + .expect("open in-memory SQLite database"); + Migrator::up(&db, None) + .await + .expect("run playlist sidebar order migrations"); + db + } + + async fn revision(connection: &impl ConnectionTrait) -> i64 { + connection + .query_one_raw(Statement::from_string( + DbBackend::Sqlite, + format!( + "SELECT revision FROM {REVISION_TABLE} WHERE singleton = {REVISION_SINGLETON}" + ), + )) + .await + .expect("query revision") + .expect("singleton revision exists") + .try_get("", "revision") + .expect("revision is an integer") + } + + async fn insert_playlist(connection: &impl ConnectionTrait, id: &str) { + connection + .execute_raw(Statement::from_sql_and_values( + DbBackend::Sqlite, + "INSERT INTO playlists (id, name, created_at, updated_at) + VALUES (?, 'Playlist', '2026-08-07T00:00:00Z', '2026-08-07T00:00:00Z')", + [id.into()], + )) + .await + .expect("insert fixture playlist"); + } + + fn order_insert_sql(playlist_id: &str, position: i64) -> Statement { + Statement::from_sql_and_values( + DbBackend::Sqlite, + format!("INSERT INTO {TABLE} (playlist_id, position) VALUES (?, ?)"), + [playlist_id.into(), position.into()], + ) + } + + async fn row_count(connection: &impl ConnectionTrait) -> i64 { + connection + .query_one_raw(Statement::from_string( + DbBackend::Sqlite, + format!("SELECT COUNT(*) AS count FROM {TABLE}"), + )) + .await + .expect("count rows") + .expect("count returns one row") + .try_get("", "count") + .expect("count is integer") + } + + #[tokio::test] + async fn fresh_up_creates_and_revalidates_the_exact_table_and_three_triggers() { + let db = migrated_database().await; + let manager = SchemaManager::new(&db); + + validate_installation(&manager) + .await + .expect("fresh installation is exact"); + revalidate(&db) + .await + .expect("startup revalidation accepts objects"); + assert_eq!(row_count(&db).await, 0); + + let trigger_rows = db + .query_all_raw(Statement::from_string( + DbBackend::Sqlite, + format!( + "SELECT name, tbl_name FROM sqlite_master + WHERE type = 'trigger' AND tbl_name = '{TABLE}' ORDER BY name" + ), + )) + .await + .expect("inspect triggers"); + assert_eq!(trigger_rows.len(), 3); + for definition in TRIGGERS { + let object = objects_named(&manager, definition.name) + .await + .expect("inspect canonical trigger"); + assert_eq!(object.len(), 1); + assert_eq!(object[0].object_type, "trigger"); + assert_eq!(object[0].table_name, TABLE); + } + } + + #[tokio::test] + async fn order_mutations_advance_and_no_op_updates_do_not() { + let db = migrated_database().await; + insert_playlist(&db, "playlist-a").await; + insert_playlist(&db, "playlist-b").await; + assert_eq!(revision(&db).await, 2); + + db.execute_raw(order_insert_sql("playlist-a", 0)) + .await + .expect("insert order row"); + assert_eq!(revision(&db).await, 3); + db.execute_raw(order_insert_sql("playlist-b", 1)) + .await + .expect("insert order row"); + assert_eq!(revision(&db).await, 4); + + db.execute_unprepared(&format!( + "UPDATE {TABLE} SET position = 1 WHERE playlist_id = 'playlist-a'" + )) + .await + .expect("effective position update"); + assert_eq!(revision(&db).await, 5); + db.execute_unprepared(&format!( + "UPDATE {TABLE} SET position = position WHERE playlist_id = 'playlist-a'" + )) + .await + .expect("actual no-op update"); + assert_eq!(revision(&db).await, 5); + + db.execute_unprepared(&format!( + "DELETE FROM {TABLE} WHERE playlist_id = 'playlist-a'" + )) + .await + .expect("delete order row"); + assert_eq!(revision(&db).await, 6); + } + + #[tokio::test] + async fn negative_positions_are_rejected_and_cascade_removes_order_rows() { + let db = migrated_database().await; + insert_playlist(&db, "playlist-a").await; + insert_playlist(&db, "playlist-b").await; + db.execute_raw(order_insert_sql("playlist-a", 0)) + .await + .expect("insert order row"); + + db.execute_unprepared(&format!( + "INSERT INTO {TABLE} (playlist_id, position) VALUES ('playlist-b', -1)" + )) + .await + .expect_err("negative position is forbidden"); + assert_eq!(revision(&db).await, 3); + assert_eq!(row_count(&db).await, 1); + + db.execute_unprepared("DELETE FROM playlists WHERE id = 'playlist-a'") + .await + .expect("cascade from parent playlist delete"); + assert_eq!(row_count(&db).await, 0); + // The parent delete and the order-row cascade each advance the + // migration-15 revision once, mirroring the per-changed-row trigger + // contract. + assert_eq!(revision(&db).await, 5); + } + + #[tokio::test] + async fn missing_singleton_aborts_order_writes() { + let db = migrated_database().await; + insert_playlist(&db, "playlist-a").await; + db.execute_unprepared(&format!( + "DELETE FROM {REVISION_TABLE} WHERE singleton = {REVISION_SINGLETON}" + )) + .await + .expect("simulate deleted singleton"); + + let error = db + .execute_raw(order_insert_sql("playlist-a", 0)) + .await + .expect_err("missing singleton must abort order insert"); + assert!(error.to_string().contains("singleton missing")); + assert_eq!(row_count(&db).await, 0); + revalidate(&db) + .await + .expect_err("startup revalidation detects missing singleton"); + } + + #[tokio::test] + async fn down_removes_objects_and_keeps_playlists() { + let db = migrated_database().await; + insert_playlist(&db, "playlist-a").await; + db.execute_raw(order_insert_sql("playlist-a", 0)) + .await + .expect("insert order row"); + + Migrator::down(&db, Some(1)) + .await + .expect("downgrade playlist-sidebar order"); + assert!(target_objects_absent(&SchemaManager::new(&db)) + .await + .unwrap()); + assert!(db + .query_one_raw(Statement::from_string( + DbBackend::Sqlite, + "SELECT id FROM playlists WHERE id = 'playlist-a'".to_string(), + )) + .await + .unwrap() + .is_some()); + } +} diff --git a/src/db/migration/mod.rs b/src/db/migration/mod.rs index 36fcdaa5..6536ec97 100644 --- a/src/db/migration/mod.rs +++ b/src/db/migration/mod.rs @@ -20,6 +20,7 @@ mod m20260720_000015_playlist_sidebar_revision; mod m20260720_000016_rhythmbox_import_receipts; mod m20260720_000017_lastfm_scrobble_queue; mod m20260721_000018_lastfm_delivery_pause; +mod m20260807_000019_playlist_sidebar_order; pub struct Migrator; @@ -45,6 +46,7 @@ impl MigratorTrait for Migrator { Box::new(m20260720_000016_rhythmbox_import_receipts::Migration), Box::new(m20260720_000017_lastfm_scrobble_queue::Migration), Box::new(m20260721_000018_lastfm_delivery_pause::Migration), + Box::new(m20260807_000019_playlist_sidebar_order::Migration), ] } } @@ -57,5 +59,6 @@ pub async fn revalidate_critical_objects( m20260720_000015_playlist_sidebar_revision::revalidate(db).await?; m20260720_000016_rhythmbox_import_receipts::revalidate(db).await?; m20260720_000017_lastfm_scrobble_queue::revalidate(db).await?; - m20260721_000018_lastfm_delivery_pause::revalidate(db).await + m20260721_000018_lastfm_delivery_pause::revalidate(db).await?; + m20260807_000019_playlist_sidebar_order::revalidate(db).await } diff --git a/src/local/playlist_manager.rs b/src/local/playlist_manager.rs index 53280cf0..c6c23cea 100644 --- a/src/local/playlist_manager.rs +++ b/src/local/playlist_manager.rs @@ -8,7 +8,9 @@ use std::collections::{HashMap, HashSet}; use sea_orm::prelude::*; use sea_orm::sea_query::Query; -use sea_orm::{ActiveValue::Set, DatabaseTransaction, QueryOrder, TransactionTrait}; +use sea_orm::{ + ActiveValue::Set, ConnectionTrait, DatabaseTransaction, QueryOrder, Statement, TransactionTrait, +}; use tracing::{debug, info, warn}; use uuid::Uuid; @@ -18,6 +20,11 @@ use crate::architecture::{MediaKey, SourceId, TrackId}; use crate::db::entities::server_playlist_link::StoredServerPlaylistLink; use crate::db::entities::{playlist, playlist_entry, server_playlist_link, track}; +/// Durable playlist-sidebar presentation order. Rows are contiguous +/// positions; a missing playlist falls back to the historical `created_at` +/// snapshot ordering. +const SIDEBAR_ORDER_TABLE: &str = "playlist_sidebar_order"; + mod server_playlist_sync; // Record E is the first production consumer of this complete engine surface. @@ -828,6 +835,56 @@ impl PlaylistManager { Ok(()) } + /// Persist the playlist-sidebar presentation order. + /// + /// `ordered_ids` must be one exact, duplicate-free permutation of every + /// current playlist ID. Regular, smart, and linked-mirror playlists all + /// 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> { + let txn = self.db.begin().await?; + let current = playlist::Entity::find().all(&txn).await?; + let requested: HashSet<&str> = ordered_ids.iter().map(String::as_str).collect(); + let current_ids: HashSet<&str> = current + .iter() + .map(|playlist| playlist.id.as_str()) + .collect(); + if requested.len() != ordered_ids.len() || requested != current_ids { + return Err(DbErr::Custom( + "Playlist sidebar reorder must contain each playlist exactly once".to_string(), + )); + } + + let stored = stored_sidebar_order(&txn).await?; + if stored == ordered_ids { + txn.commit().await?; + return Ok(()); + } + + txn.execute_raw(Statement::from_string( + txn.get_database_backend(), + format!("DELETE FROM {SIDEBAR_ORDER_TABLE}"), + )) + .await?; + for (position, playlist_id) in ordered_ids.iter().enumerate() { + let position = i64::try_from(position) + .map_err(|_| DbErr::Custom("Playlist sidebar has too many rows".to_string()))?; + txn.execute_raw(Statement::from_sql_and_values( + txn.get_database_backend(), + format!("INSERT INTO {SIDEBAR_ORDER_TABLE} (playlist_id, position) VALUES (?, ?)"), + [playlist_id.as_str().into(), position.into()], + )) + .await?; + } + txn.commit().await?; + info!( + count = ordered_ids.len(), + "Playlist sidebar order persisted" + ); + Ok(()) + } + /// Load every durable regular-playlist occurrence in stored order. /// Unmatched and currently unavailable entries are retained. pub async fn get_playlist_entries( @@ -1188,6 +1245,28 @@ where Ok(loaded) } +/// Read the persisted playlist-sidebar order, position ascending. +/// +/// A playlist without a row has never been explicitly positioned and keeps +/// the historical `created_at` fallback ordering. +async fn stored_sidebar_order(db: &C) -> Result, DbErr> +where + C: ConnectionTrait, +{ + let rows = db + .query_all_raw(Statement::from_string( + db.get_database_backend(), + format!("SELECT playlist_id FROM {SIDEBAR_ORDER_TABLE} ORDER BY position"), + )) + .await?; + rows.into_iter() + .map(|row| { + row.try_get::("", "playlist_id") + .map_err(|_| DbErr::Custom("Playlist sidebar order row is malformed".to_string())) + }) + .collect() +} + fn orphan_reconciliation_query() -> sea_orm::Select { playlist_entry::Entity::find() .filter(playlist_entry::Column::SourceId.eq(SourceId::local().to_string())) @@ -1481,11 +1560,12 @@ mod tests { use sea_orm::{ ActiveModelTrait, ActiveValue::Set, ColumnTrait, ConnectionTrait, Database, DatabaseBackend, DatabaseConnection, EntityTrait, QueryFilter, QueryOrder, QueryTrait, + Statement, }; use sea_orm_migration::MigratorTrait; use super::{ - orphan_reconciliation_query, recently_played_default_rules, + orphan_reconciliation_query, recently_played_default_rules, stored_sidebar_order, top_25_most_played_default_rules, LocalPlaylistExport, PlaylistEntryAddOutcome, PlaylistEntryInput, PlaylistManager, StoredPlaylistEntry, }; @@ -2914,6 +2994,95 @@ mod tests { assert_eq!(ordered_ids, new_order); } + #[tokio::test] + async fn set_sidebar_order_persists_and_short_circuits_noop_permutations() { + let db = in_memory_db().await; + let manager = PlaylistManager::new(db.clone()); + + let first = manager + .create_regular_playlist("First") + .await + .expect("create first playlist"); + let second = manager + .create_regular_playlist("Second") + .await + .expect("create second playlist"); + let mirror = manager + .create_regular_playlist("Mirror") + .await + .expect("create mirror playlist"); + + let reordered = vec![second.id.clone(), mirror.id.clone(), first.id.clone()]; + manager + .set_sidebar_order(&reordered) + .await + .expect("persist reorder"); + + let stored = stored_sidebar_order(&db).await.expect("read stored order"); + assert_eq!(stored, reordered); + let revision: i64 = db + .query_one_raw(Statement::from_string( + DatabaseBackend::Sqlite, + "SELECT revision FROM playlist_sidebar_revision WHERE singleton = 1".to_string(), + )) + .await + .expect("query revision") + .expect("revision exists") + .try_get("", "revision") + .expect("revision is integer"); + // Two playlist inserts plus the reorder's delete/insert rows bump it. + assert!(revision > 3); + + manager + .set_sidebar_order(&reordered) + .await + .expect("identical reorder short-circuits"); + assert_eq!( + stored_sidebar_order(&db).await.expect("unchanged order"), + reordered + ); + let revision_after: i64 = db + .query_one_raw(Statement::from_string( + DatabaseBackend::Sqlite, + "SELECT revision FROM playlist_sidebar_revision WHERE singleton = 1".to_string(), + )) + .await + .expect("query revision") + .expect("revision exists") + .try_get("", "revision") + .expect("revision is integer"); + assert_eq!( + revision_after, revision, + "no-op reorder must not bump revision" + ); + + let missing = vec![first.id.clone(), second.id.clone()]; + let rejected = manager + .set_sidebar_order(&missing) + .await + .expect_err("subset reorder must be rejected"); + assert!(rejected.to_string().contains("exactly once")); + assert_eq!( + stored_sidebar_order(&db) + .await + .expect("rejected write is rolled back"), + reordered, + "rejected reorder must not alter the persisted order" + ); + let duplicate = vec![ + mirror.id.clone(), + mirror.id.clone(), + first.id.clone(), + second.id.clone(), + ]; + assert!(manager + .set_sidebar_order(&duplicate) + .await + .expect_err("duplicate reorder must be rejected") + .to_string() + .contains("exactly once")); + } + #[tokio::test] async fn reorder_and_remove_require_exact_occurrence_ids_and_rollback_invalid_requests() { let db = in_memory_db().await; diff --git a/src/local/playlist_sidebar.rs b/src/local/playlist_sidebar.rs index db95f9a3..6c41413f 100644 --- a/src/local/playlist_sidebar.rs +++ b/src/local/playlist_sidebar.rs @@ -41,7 +41,8 @@ SELECT l.state_revision AS link_state_revision FROM playlists AS p LEFT JOIN server_playlist_links AS l ON l.playlist_id = p.id -ORDER BY p.created_at ASC, p.id ASC +LEFT JOIN playlist_sidebar_order AS o ON o.playlist_id = p.id +ORDER BY o.position IS NULL ASC, o.position ASC, p.created_at ASC, p.id ASC "; /// Fallback cadence for direct SQL mutations or a lost refresh hint. @@ -621,6 +622,18 @@ mod tests { .await; } + async fn insert_order(db: &DatabaseConnection, playlist_id: &str, position: i64) { + execute( + db, + format!( + "INSERT INTO playlist_sidebar_order (playlist_id, position) VALUES ({},{})", + sql_string(playlist_id), + position, + ), + ) + .await; + } + #[test] fn revision_validation_is_closed_and_nonnegative() { assert_eq!(PlaylistSidebarRevision::new(0).unwrap().value(), 0); @@ -683,6 +696,29 @@ mod tests { assert_eq!(entries[1].kind(), PlaylistSidebarKind::EditableSmart); } + #[tokio::test] + async fn explicit_order_rows_sort_before_unordered_created_at_fallbacks() { + let db = migrated_database().await; + insert_playlist(&db, "u1", "Unordered one", 0, "2026-07-20T00:00:00Z").await; + insert_playlist(&db, "p1", "Positioned one", 0, "2026-07-20T00:00:01Z").await; + insert_playlist(&db, "u2", "Unordered two", 0, "2026-07-20T00:00:02Z").await; + insert_playlist(&db, "p2", "Positioned two", 0, "2026-07-20T00:00:03Z").await; + insert_order(&db, "p2", 1).await; + insert_order(&db, "p1", 0).await; + + let snapshot = load_playlist_sidebar_snapshot(&db).await.unwrap(); + let PlaylistSidebarState::Ready(entries) = snapshot.state() else { + panic!("expected ready sidebar projection"); + }; + assert_eq!( + entries + .iter() + .map(PlaylistSidebarEntry::playlist_id) + .collect::>(), + ["p1", "p2", "u1", "u2"] + ); + } + #[tokio::test] async fn revision_and_rows_share_one_coherent_concurrent_read_snapshot() { let (_directory, db) = migrated_file_database().await; diff --git a/src/ui/playlist_actions.rs b/src/ui/playlist_actions.rs index 009c98eb..90820ee2 100644 --- a/src/ui/playlist_actions.rs +++ b/src/ui/playlist_actions.rs @@ -182,6 +182,15 @@ pub fn setup_playlist_actions( debug!(id = %playlist_id, "ExportPlaylist: playlist_allows_ordinary_actions rejected"); } } + + sidebar::PlaylistAction::Reorder(ordered_ids) => { + debug!(count = ordered_ids.len(), "dispatching Reorder"); + if let Err(e) = catch_unwind(AssertUnwindSafe(|| { + handle_reorder(&win, &rt_handle, &playlist_sidebar_refresh, &ordered_ids); + })) { + error!("Reorder handler panicked: {e:?}"); + } + } } } info!("setup_playlist_actions: async task finished (channel closed or loop exited)"); @@ -460,6 +469,54 @@ fn handle_delete( }); } +/// Persist the user-defined playlist sidebar order. +fn handle_reorder( + win: &adw::ApplicationWindow, + rt_handle: &tokio::runtime::Handle, + playlist_sidebar_refresh: &crate::local::playlist_sidebar::PlaylistSidebarRefresh, + ordered_ids: &[String], +) { + info!(count = ordered_ids.len(), "Reordering playlist sidebar"); + let rt_handle = rt_handle.clone(); + let playlist_sidebar_refresh = playlist_sidebar_refresh.clone(); + let ordered_ids = ordered_ids.to_vec(); + let win_for_result = win.clone(); + + let (result_tx, result_rx) = async_channel::bounded::>(1); + + rt_handle.spawn(async move { + let outcome = match crate::db::connection::init_db().await { + Ok(db) => { + let mgr = crate::local::playlist_manager::PlaylistManager::new(db); + match mgr.set_sidebar_order(&ordered_ids).await { + Ok(()) => PlaylistCrudOutcome::Committed(()), + Err(e) => { + tracing::error!(error = %e, "Failed to reorder playlists"); + PlaylistCrudOutcome::Failed + } + } + } + Err(e) => { + tracing::error!(error = %e, "Failed to open DB"); + PlaylistCrudOutcome::Failed + } + }; + let _ = result_tx.send(outcome).await; + }); + + glib::MainContext::default().spawn_local(async move { + match result_rx.recv().await { + Ok(outcome) + if playlist_sidebar_publication_effect(&outcome) + == PlaylistSidebarPublicationEffect::RequestFullSnapshot => + { + request_playlist_sidebar_refresh(&playlist_sidebar_refresh); + } + Ok(_) | Err(_) => show_playlist_mutation_failed(&win_for_result), + } + }); +} + /// Fetch existing smart rules from DB, show the editor, and save updates. fn handle_edit_smart( win: &adw::ApplicationWindow, diff --git a/src/ui/sidebar.rs b/src/ui/sidebar.rs index 6f578381..4a5ed9e7 100644 --- a/src/ui/sidebar.rs +++ b/src/ui/sidebar.rs @@ -33,6 +33,41 @@ pub enum PlaylistAction { BrowseServerPlaylists, /// Export a playlist to an XSPF file (id). ExportPlaylist(String), + /// Reorder the playlist sidebar presentation order (full ordered id list). + Reorder(Vec), +} + +/// Compute the sidebar playlist order after a drag moves `dragged_id` +/// next to `target_id`. +/// +/// Returns `None` when the drop is a no-op (same row) or either id is +/// unknown. `before` places the dragged playlist in front of the target. +fn reorder_playlist_ids( + current: &[String], + dragged_id: &str, + target_id: &str, + before: bool, +) -> Option> { + if dragged_id == target_id { + return None; + } + let dragged_pos = current.iter().position(|id| id == dragged_id)?; + let mut ids = current.to_vec(); + ids.remove(dragged_pos); + let target_pos = ids.iter().position(|id| id == target_id)?; + let insert_at = if before { target_pos } else { target_pos + 1 }; + ids.insert(insert_at, dragged_id.to_string()); + Some(ids) +} + +/// The playlist ids in their current sidebar display order. +fn playlist_ids_in_store_order(store: >k::gio::ListStore) -> Vec { + (0..store.n_items()) + .filter_map(|position| store.item(position)) + .filter_map(|obj| obj.downcast::().ok()) + .filter(SourceObject::is_playlist) + .map(|src| src.playlist_id()) + .collect() } /// Action represented by the recycled row's trailing button. @@ -474,6 +509,78 @@ pub fn build_sidebar( popover.popup(); }); row_box.add_controller(gesture); + + // ── Playlist drag & drop reorder ────────────────────────────── + // + // Only playlist rows initiate a drag; the payload is the playlist + // id carried as a string value. The drop target is per-row so it + // resolves the exact target position via `list_item.position()` + // (the same mechanism as the context-menu gesture above). + let drag_source = gtk::DragSource::new(); + drag_source.set_actions(gtk::gdk::DragAction::MOVE); + { + let store_for_drag = store_for_setup.clone(); + let list_item_for_drag = list_item.clone(); + drag_source.connect_prepare(move |_, _x, _y| { + let item = store_for_drag.item(list_item_for_drag.position())?; + let src = item.downcast_ref::()?; + if !src.is_playlist() { + return None; + } + let value = glib::Value::from(src.playlist_id()); + Some(gtk::gdk::ContentProvider::for_value(&value)) + }); + } + row_box.add_controller(drag_source); + + let drop_target = gtk::DropTarget::new(glib::Type::STRING, gtk::gdk::DragAction::MOVE); + { + let store_for_enter = store_for_setup.clone(); + let list_item_for_enter = list_item.clone(); + drop_target.connect_enter(move |_, _x, _y| { + let pos = list_item_for_enter.position(); + let Some(item) = store_for_enter.item(pos) else { + return gtk::gdk::DragAction::empty(); + }; + let Some(src) = item.downcast_ref::() else { + return gtk::gdk::DragAction::empty(); + }; + if src.is_playlist() { + gtk::gdk::DragAction::MOVE + } else { + gtk::gdk::DragAction::empty() + } + }); + let store_for_drop = store_for_setup.clone(); + let tx_for_drop = tx_for_setup.clone(); + let list_item_for_drop = list_item.clone(); + let row_box_for_drop = row_box.clone(); + drop_target.connect_drop(move |_, value, _x, y| { + let Ok(dragged_id) = value.get_owned::() else { + return false; + }; + let pos = list_item_for_drop.position(); + let Some(item) = store_for_drop.item(pos) else { + return false; + }; + let Some(src) = item.downcast_ref::() else { + return false; + }; + if !src.is_playlist() { + return false; + } + let current = playlist_ids_in_store_order(&store_for_drop); + let before = y < (row_box_for_drop.height() as f64) / 2.0; + let Some(ordered) = + reorder_playlist_ids(¤t, &dragged_id, &src.playlist_id(), before) + else { + return false; + }; + let _ = tx_for_drop.try_send(PlaylistAction::Reorder(ordered)); + true + }); + } + row_box.add_controller(drop_target); }); } @@ -826,4 +933,81 @@ mod tests { Some(rust_i18n::t!("server_playlists.browse_menu").as_ref()) ); } + + #[test] + fn reorder_playlist_ids_places_dragged_next_to_target() { + let current = ["a", "b", "c", "d"].map(str::to_string).to_vec(); + + // Move "a" before "c" (downward). + assert_eq!( + reorder_playlist_ids(¤t, "a", "c", true).unwrap(), + ["b", "a", "c", "d"] + ); + // Move "a" after "c" (downward). + assert_eq!( + reorder_playlist_ids(¤t, "a", "c", false).unwrap(), + ["b", "c", "a", "d"] + ); + // Move "d" before "b" (upward). + assert_eq!( + reorder_playlist_ids(¤t, "d", "b", true).unwrap(), + ["a", "d", "b", "c"] + ); + // Move "d" after "b" (upward). + assert_eq!( + reorder_playlist_ids(¤t, "d", "b", false).unwrap(), + ["a", "b", "d", "c"] + ); + // Dragging a row onto itself is a no-op. + assert!(reorder_playlist_ids(¤t, "b", "b", true).is_none()); + assert!(reorder_playlist_ids(¤t, "b", "b", false).is_none()); + // Unknown ids are rejected. + assert!(reorder_playlist_ids(¤t, "nope", "b", true).is_none()); + assert!(reorder_playlist_ids(¤t, "b", "nope", true).is_none()); + } + + #[test] + fn reorder_playlist_ids_keeps_permutation_intact() { + let current = ["a", "b", "c", "d", "e"].map(str::to_string).to_vec(); + let reordered = reorder_playlist_ids(¤t, "e", "a", true).unwrap(); + assert_eq!(reordered, ["e", "a", "b", "c", "d"]); + + let mut sorted: Vec = reordered.clone(); + sorted.sort(); + let mut original: Vec = current.clone(); + original.sort(); + assert_eq!(sorted, original, "reorder must be a pure permutation"); + + let reordered = reorder_playlist_ids(¤t, "a", "e", false).unwrap(); + assert_eq!(reordered, ["b", "c", "d", "e", "a"]); + } + + #[test] + fn playlist_ids_in_store_order_skips_non_playlist_rows() { + use crate::local::playlist_sidebar::PlaylistSidebarEntry; + use crate::ui::objects::HeaderKind; + + let store = gtk::gio::ListStore::new::(); + store.append(&SourceObject::header("Local", HeaderKind::Local)); + store.append(&SourceObject::playlist_entry(&PlaylistSidebarEntry::new( + "p1", + "First", + PlaylistSidebarKind::EditableRegular, + ))); + store.append(&SourceObject::playlist_entry(&PlaylistSidebarEntry::new( + "p2", + "Second", + PlaylistSidebarKind::EditableSmart, + ))); + store.append(&SourceObject::discovered( + "DAAP", + "daap", + "http://b.example:3689", + )); + + assert_eq!( + playlist_ids_in_store_order(&store), + ["p1", "p2"].map(str::to_string).to_vec() + ); + } } From e6a66aeb3ac6a1c2b490c6b85ea1183a7d029dfc Mon Sep 17 00:00:00 2001 From: Glenn Trigg Date: Tue, 11 Aug 2026 14:52:50 +1000 Subject: [PATCH 2/4] Address review concern re out of order race condition --- src/local/playlist_sidebar.rs | 2 +- src/ui/playlist_actions.rs | 194 ++++++++++++++++++++++++++++++---- 2 files changed, 173 insertions(+), 23 deletions(-) diff --git a/src/local/playlist_sidebar.rs b/src/local/playlist_sidebar.rs index 6c41413f..94adda23 100644 --- a/src/local/playlist_sidebar.rs +++ b/src/local/playlist_sidebar.rs @@ -626,7 +626,7 @@ mod tests { execute( db, format!( - "INSERT INTO playlist_sidebar_order (playlist_id, position) VALUES ({},{})", + "INSERT INTO playlist_sidebar_order (playlist_id, position) VALUES ({}, {})", sql_string(playlist_id), position, ), diff --git a/src/ui/playlist_actions.rs b/src/ui/playlist_actions.rs index 90820ee2..ecdfdecb 100644 --- a/src/ui/playlist_actions.rs +++ b/src/ui/playlist_actions.rs @@ -50,6 +50,52 @@ fn request_playlist_sidebar_refresh( } } +/// A queued sidebar reorder: the order to persist and the channel that +/// receives the persistence outcome. +struct SidebarReorderRequest { + ordered_ids: Vec, + result_tx: async_channel::Sender>, +} + +/// Serialize sidebar reorder persistence so writes commit in receipt order. +/// +/// A single worker consumes requests FIFO and awaits each write before the +/// next, so a later reorder can never be overtaken by an earlier, slower +/// database task. Closing the queue (window teardown) fails every receiver, +/// matching the documented "dropped worker is treated as failure" contract. +async fn run_sidebar_reorder_worker( + rx: async_channel::Receiver, + mut persist: F, +) where + F: FnMut(Vec) -> Fut, + Fut: std::future::Future>, +{ + while let Ok(request) = rx.recv().await { + let outcome = persist(request.ordered_ids).await; + let _ = request.result_tx.send(outcome).await; + } +} + +/// Persist a sidebar reorder, mapping database failures to `Failed`. +async fn persist_sidebar_reorder(ordered_ids: Vec) -> PlaylistCrudOutcome<()> { + match crate::db::connection::init_db().await { + Ok(db) => { + let mgr = crate::local::playlist_manager::PlaylistManager::new(db); + match mgr.set_sidebar_order(&ordered_ids).await { + Ok(()) => PlaylistCrudOutcome::Committed(()), + Err(e) => { + tracing::error!(error = %e, "Failed to reorder playlists"); + PlaylistCrudOutcome::Failed + } + } + } + Err(e) => { + tracing::error!(error = %e, "Failed to open DB"); + PlaylistCrudOutcome::Failed + } + } +} + /// Wire the playlist action receiver to the sidebar store. /// /// Spawns an async task on the GTK main context that listens for @@ -66,6 +112,15 @@ pub fn setup_playlist_actions( let win = state.window.clone(); let playlist_sidebar_refresh = state.playlist_sidebar_refresh.clone(); + // Serialize sidebar reorder persistence: one worker applies reorder + // writes in receipt order, so a later reorder is never overtaken by an + // earlier, slower database task. + let (reorder_tx, reorder_rx) = async_channel::unbounded::(); + rt_handle.spawn(run_sidebar_reorder_worker( + reorder_rx, + persist_sidebar_reorder, + )); + debug!("setup_playlist_actions: async task spawned"); glib::MainContext::default().spawn_local(async move { debug!("setup_playlist_actions: async task started polling"); @@ -186,7 +241,7 @@ pub fn setup_playlist_actions( sidebar::PlaylistAction::Reorder(ordered_ids) => { debug!(count = ordered_ids.len(), "dispatching Reorder"); if let Err(e) = catch_unwind(AssertUnwindSafe(|| { - handle_reorder(&win, &rt_handle, &playlist_sidebar_refresh, &ordered_ids); + handle_reorder(&win, &reorder_tx, &playlist_sidebar_refresh, &ordered_ids); })) { error!("Reorder handler panicked: {e:?}"); } @@ -472,37 +527,30 @@ fn handle_delete( /// Persist the user-defined playlist sidebar order. fn handle_reorder( win: &adw::ApplicationWindow, - rt_handle: &tokio::runtime::Handle, + reorder_tx: &async_channel::Sender, playlist_sidebar_refresh: &crate::local::playlist_sidebar::PlaylistSidebarRefresh, ordered_ids: &[String], ) { info!(count = ordered_ids.len(), "Reordering playlist sidebar"); - let rt_handle = rt_handle.clone(); let playlist_sidebar_refresh = playlist_sidebar_refresh.clone(); let ordered_ids = ordered_ids.to_vec(); let win_for_result = win.clone(); let (result_tx, result_rx) = async_channel::bounded::>(1); - rt_handle.spawn(async move { - let outcome = match crate::db::connection::init_db().await { - Ok(db) => { - let mgr = crate::local::playlist_manager::PlaylistManager::new(db); - match mgr.set_sidebar_order(&ordered_ids).await { - Ok(()) => PlaylistCrudOutcome::Committed(()), - Err(e) => { - tracing::error!(error = %e, "Failed to reorder playlists"); - PlaylistCrudOutcome::Failed - } - } - } - Err(e) => { - tracing::error!(error = %e, "Failed to open DB"); - PlaylistCrudOutcome::Failed - } - }; - let _ = result_tx.send(outcome).await; - }); + // 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; + } glib::MainContext::default().spawn_local(async move { match result_rx.recv().await { @@ -1125,4 +1173,106 @@ mod tests { } } } + + #[tokio::test] + async fn reorder_writes_commit_in_receipt_order_when_first_is_slow() { + use sea_orm::{ConnectionTrait, Database, DatabaseBackend, Statement}; + use sea_orm_migration::MigratorTrait; + use std::sync::atomic::{AtomicBool, Ordering}; + use std::sync::Arc; + use std::time::Duration; + + use crate::db::migration::Migrator; + use crate::local::playlist_manager::PlaylistManager; + + let db = Database::connect("sqlite::memory:") + .await + .expect("open in-memory sqlite"); + Migrator::up(&db, None).await.expect("run migrations"); + let manager = PlaylistManager::new(db.clone()); + + let first = manager + .create_regular_playlist("First") + .await + .expect("create first playlist"); + let second = manager + .create_regular_playlist("Second") + .await + .expect("create second playlist"); + let third = manager + .create_regular_playlist("Third") + .await + .expect("create third playlist"); + + let earlier = vec![second.id.clone(), third.id.clone(), first.id.clone()]; + let later = vec![third.id.clone(), first.id.clone(), second.id.clone()]; + + let (tx, rx) = async_channel::unbounded::(); + let slow_first = Arc::new(AtomicBool::new(true)); + + let worker = { + let db = db.clone(); + let slow_first = slow_first.clone(); + tokio::spawn(run_sidebar_reorder_worker(rx, move |ordered_ids| { + let db = db.clone(); + let slow_first = slow_first.clone(); + async move { + if slow_first.swap(false, Ordering::SeqCst) { + tokio::time::sleep(Duration::from_millis(200)).await; + } + let manager = PlaylistManager::new(db); + match manager.set_sidebar_order(&ordered_ids).await { + Ok(()) => PlaylistCrudOutcome::Committed(()), + Err(e) => { + tracing::error!(error = %e, "test reorder failed"); + PlaylistCrudOutcome::Failed + } + } + } + })) + }; + + let (first_tx, first_rx) = async_channel::bounded::>(1); + let (later_tx, later_rx) = async_channel::bounded::>(1); + + tx.send(SidebarReorderRequest { + ordered_ids: earlier.clone(), + result_tx: first_tx, + }) + .await + .expect("queue first reorder"); + tx.send(SidebarReorderRequest { + ordered_ids: later.clone(), + result_tx: later_tx, + }) + .await + .expect("queue second reorder"); + + assert_eq!( + first_rx.recv().await, + Ok(PlaylistCrudOutcome::Committed(())) + ); + assert_eq!( + later_rx.recv().await, + Ok(PlaylistCrudOutcome::Committed(())) + ); + drop(tx); + worker.await.expect("worker exits cleanly"); + + let stored = db + .query_all_raw(Statement::from_string( + DatabaseBackend::Sqlite, + "SELECT playlist_id FROM playlist_sidebar_order ORDER BY position".to_string(), + )) + .await + .expect("read stored order") + .into_iter() + .map(|row| row.try_get::("", "playlist_id").expect("stored id")) + .collect::>(); + + assert_eq!( + stored, later, + "second reorder must be the final stored order" + ); + } } From e9c5e7cc98bc36d416ce8999772542a25784c66d Mon Sep 17 00:00:00 2001 From: Glenn Trigg Date: Fri, 21 Aug 2026 19:25:08 +1000 Subject: [PATCH 3/4] Address pertinent review comments --- .../migration/m20260807_000019_playlist_sidebar_order.rs | 2 -- src/ui/playlist_actions.rs | 7 +++++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/db/migration/m20260807_000019_playlist_sidebar_order.rs b/src/db/migration/m20260807_000019_playlist_sidebar_order.rs index d80a8a67..b1e2557a 100644 --- a/src/db/migration/m20260807_000019_playlist_sidebar_order.rs +++ b/src/db/migration/m20260807_000019_playlist_sidebar_order.rs @@ -193,8 +193,6 @@ fn canonical_trigger_sql(trigger: TriggerDefinition) -> String { END", name = trigger.name, operation = trigger.operation, - TABLE = TABLE, - when = when, revision_table = REVISION_TABLE, singleton = REVISION_SINGLETON, max_revision = i64::MAX, diff --git a/src/ui/playlist_actions.rs b/src/ui/playlist_actions.rs index ecdfdecb..4307c6f6 100644 --- a/src/ui/playlist_actions.rs +++ b/src/ui/playlist_actions.rs @@ -540,8 +540,10 @@ fn handle_reorder( // 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. + // 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, @@ -549,6 +551,7 @@ fn handle_reorder( }) .is_err() { + warn!("Sidebar reorder queue closed; dropping reorder request"); return; } From 2356a0d6575f04047f2b285582633c8b19ffe9dc Mon Sep 17 00:00:00 2001 From: Glenn Trigg Date: Sat, 22 Aug 2026 08:31:58 +1000 Subject: [PATCH 4/4] Fix clippy lints newly enforced by Rust 1.98 stable Rust 1.98's clippy added two lints that the floating `stable` CI toolchain now denies (`-D warnings`): - clippy::chunks_exact_to_as_chunks: replace chunks_exact(2) with as_chunks::<2>() (inputs are even-length validated, so the remainder is empty and behavior is unchanged). - clippy::manual_is_variant_and: use Result::is_ok_and instead of .ok().is_some_and(..). These are pre-existing in main's code and only surfaced after the stable toolchain advanced; fixing them gets PR CI green. --- src/architecture/identity.rs | 4 +++- src/lastfm/authorization.rs | 7 +------ src/platform_runtime.rs | 2 +- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/src/architecture/identity.rs b/src/architecture/identity.rs index 61b64740..92ae4c4c 100644 --- a/src/architecture/identity.rs +++ b/src/architecture/identity.rs @@ -330,7 +330,9 @@ fn decode_hex(encoded: &str) -> Result, IdentityError> { } encoded .as_bytes() - .chunks_exact(2) + .as_chunks::<2>() + .0 + .iter() .map(|pair| { let high = decode_hex_nibble(pair[0])?; let low = decode_hex_nibble(pair[1])?; diff --git a/src/lastfm/authorization.rs b/src/lastfm/authorization.rs index 1552ee5f..93f001ab 100644 --- a/src/lastfm/authorization.rs +++ b/src/lastfm/authorization.rs @@ -723,12 +723,7 @@ impl LastFmAuthorizationHandle { impl fmt::Debug for LastFmAuthorizationHandle { fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - let open = self - .inner - .ingress - .lock() - .ok() - .is_some_and(|ingress| ingress.open); + let open = self.inner.ingress.lock().is_ok_and(|ingress| ingress.open); formatter .debug_struct("LastFmAuthorizationHandle") .field("open", &open) diff --git a/src/platform_runtime.rs b/src/platform_runtime.rs index cf2206fe..1acfc9c1 100644 --- a/src/platform_runtime.rs +++ b/src/platform_runtime.rs @@ -1765,7 +1765,7 @@ mod tests { ); let mut bindings = std::collections::BTreeMap::new(); - for pair in tokens[1..].chunks_exact(2) { + for pair in tokens[1..].as_chunks::<2>().0 { assert!( pair[0].starts_with('-'), "invocation {invocation_index} contains a positional argument: {invocation}"