diff --git a/docs/rfcs/RFC-005-remote-source-and-sync-v2.md b/docs/rfcs/RFC-005-remote-source-and-sync-v2.md index 3c67dfa4..8ecf4b54 100644 --- a/docs/rfcs/RFC-005-remote-source-and-sync-v2.md +++ b/docs/rfcs/RFC-005-remote-source-and-sync-v2.md @@ -480,8 +480,23 @@ comparable rather than merely concatenated: A server album keeps none of the local gestures — no playlist, no cover picker, no context menu — because none of them can accept it, and it opens the remote -detail view rather than the local one. The artists tab works the same way, on -the same filter. +detail view rather than the local one. The artists and tracks tabs work the same +way, on the same filter. + +The tracks tab adds one consequence worth stating plainly: **playing a row +queues the run of rows from its own source.** Decision 9 keeps the remote queue +parallel to the local one — they are two structures, and a mixed list cannot +produce a mixed queue. So clicking a local track queues the local rows and +clicking a server track queues the server ones. The chip on every row is what +makes that legible, and narrowing the source filter is how a user gets one +continuous queue. + +A server track also carries none of the local user data: no rating, no like, no +playlist membership, no tag editing. Those are absent rather than inert — five +hollow stars that do nothing read as "unrated", which is a different claim from +"cannot be rated here". Only tracks the catalogue walk mirrored are listed: one +cached because a playlist referenced it is not part of the browsable catalogue +and would appear with no album and no way to reach it. Artists need one thing albums did not: the walk mirrors them into `remote_artist` rather than deriving them by grouping on `artist_id`. Grouping diff --git a/src-tauri/crates/app/src/commands/browse.rs b/src-tauri/crates/app/src/commands/browse.rs index 9c47155f..52cf2e4c 100644 --- a/src-tauri/crates/app/src/commands/browse.rs +++ b/src-tauri/crates/app/src/commands/browse.rs @@ -471,6 +471,336 @@ fn library_album_order_clause(order_by: Option<&str>, direction: Option<&str>) - } } +/// One track of the library, whichever source it comes from. +/// +/// Carries only what the library table renders. A server track has no local +/// row, so it has no rating, no like, no file and no tags — the fields that +/// describe those are absent rather than defaulted, because a `0` rating and +/// "not rated" are different things. +#[derive(Debug, Clone, Serialize)] +pub struct LibraryTrackRow { + pub source: String, + /// Local rowid rendered as text, or the server's track UUID. + pub id: String, + /// Local only — the library a track belongs to. A server track belongs to + /// one of the *server's* libraries, which is not one of these. + pub library_id: Option, + pub title: String, + pub album_id: Option, + pub album_title: Option, + pub artist_id: Option, + pub artist_name: Option, + /// Comma-joined artist ids, local only — the server credits one artist per + /// track in its listings, so a remote row has nothing to split. + pub artist_ids: Option, + pub duration_ms: i64, + pub track_number: Option, + pub disc_number: Option, + pub year: Option, + pub bitrate: Option, + pub sample_rate: Option, + pub bit_depth: Option, + pub channels: Option, + pub codec: Option, + pub musical_key: Option, + /// Local only, and the reason a remote row cannot be edited, rated or + /// hashed: there is no file here to do any of it to. + pub file_path: Option, + pub file_size: Option, + pub added_at: i64, + pub artwork_hash: Option, + pub artwork_format: Option, + pub artwork_has_1x: bool, + pub artwork_has_2x: bool, + /// Local only. `None` on a remote row means "this cannot be rated here", + /// not "unrated". + pub rating: Option, +} + +#[derive(Debug, Clone, Serialize)] +pub struct ListLibraryTracksResponse { + pub artwork_base: String, + pub items: Vec, +} + +#[derive(sqlx::FromRow)] +struct LibraryTrackRawRow { + source: String, + id: String, + library_id: Option, + title: String, + album_id: Option, + album_title: Option, + artist_id: Option, + artist_name: Option, + artist_ids: Option, + duration_ms: i64, + track_number: Option, + disc_number: Option, + year: Option, + bitrate: Option, + sample_rate: Option, + bit_depth: Option, + channels: Option, + codec: Option, + musical_key: Option, + file_path: Option, + file_size: Option, + added_at: i64, + artwork_hash: Option, + artwork_format: Option, + rating: Option, +} + +/// Ordering for the unified track listing. +/// +/// Artist and album sort on the normalised keys both halves now carry; the +/// title sorts on the display string on both sides, because the local half has +/// no canonical form for it either. Consistency per column is what matters — +/// normalising one side of a comparison and not the other is exactly how an +/// artist ends up in two places. +fn library_track_order_clause(order_by: Option<&str>, direction: Option<&str>) -> &'static str { + // `duration_ms` is the column name, and it is what the sort dropdown and + // the persisted preference both carry. Matching on "duration" here sent + // every duration sort to the fallback clause instead. + let dir_default_desc = matches!( + order_by, + Some("duration_ms") | Some("added_at") | Some("year") | Some("rating") + ); + let dir = match direction { + Some(d) if d.eq_ignore_ascii_case("asc") => "ASC", + Some(d) if d.eq_ignore_ascii_case("desc") => "DESC", + _ => { + if dir_default_desc { + "DESC" + } else { + "ASC" + } + } + }; + match (order_by, dir) { + (Some("title"), "ASC") => "ORDER BY title COLLATE NOCASE ASC", + (Some("title"), "DESC") => "ORDER BY title COLLATE NOCASE DESC", + (Some("artist"), "ASC") => { + "ORDER BY sort_artist COLLATE NOCASE ASC, title COLLATE NOCASE" + } + (Some("artist"), "DESC") => { + "ORDER BY sort_artist COLLATE NOCASE DESC, title COLLATE NOCASE" + } + (Some("album"), "ASC") => { + "ORDER BY sort_album COLLATE NOCASE ASC, disc_number, track_number" + } + (Some("album"), "DESC") => { + "ORDER BY sort_album COLLATE NOCASE DESC, disc_number, track_number" + } + (Some("duration_ms"), "ASC") => "ORDER BY duration_ms ASC", + (Some("duration_ms"), "DESC") => "ORDER BY duration_ms DESC", + (Some("year"), "ASC") => "ORDER BY year ASC, title COLLATE NOCASE", + (Some("year"), "DESC") => "ORDER BY year DESC, title COLLATE NOCASE", + (Some("added_at"), "ASC") => "ORDER BY added_at ASC", + (Some("added_at"), "DESC") => "ORDER BY added_at DESC", + // Rating is local-only, so a server track has none. NULLs last in + // either direction: an unratable row is not a badly-rated one. + (Some("rating"), "ASC") => { + "ORDER BY rating IS NULL, rating ASC, title COLLATE NOCASE" + } + (Some("rating"), "DESC") => { + "ORDER BY rating IS NULL, rating DESC, title COLLATE NOCASE" + } + _ => { + "ORDER BY sort_artist COLLATE NOCASE,\n sort_album COLLATE NOCASE,\n disc_number,\n track_number,\n title COLLATE NOCASE" + } + } +} + +/// Both halves of the track listing, as one compound select. +/// +/// Split out of the command for the reason on [`library_albums_sql`]. +fn library_tracks_sql(order_clause: &str) -> String { + format!( + r#" + SELECT source, id, library_id, title, album_id, album_title, artist_id, artist_name, + artist_ids, duration_ms, track_number, disc_number, year, bitrate, sample_rate, + bit_depth, channels, codec, musical_key, file_path, file_size, added_at, + artwork_hash, artwork_format, rating + FROM ( + SELECT 'local' AS source, + CAST(t.id AS TEXT) AS id, + t.library_id AS library_id, + t.title AS title, + CAST(t.album_id AS TEXT) AS album_id, + al.title AS album_title, + CAST(t.primary_artist AS TEXT) AS artist_id, + (SELECT GROUP_CONCAT(name, ', ') FROM ( + SELECT ar2.name FROM track_artist ta2 + JOIN artist ar2 ON ar2.id = ta2.artist_id + WHERE ta2.track_id = t.id + ORDER BY ta2.position + )) AS artist_name, + (SELECT GROUP_CONCAT(id, ',') FROM ( + SELECT ta2.artist_id AS id FROM track_artist ta2 + WHERE ta2.track_id = t.id + ORDER BY ta2.position + )) AS artist_ids, + t.duration_ms AS duration_ms, + t.track_number AS track_number, + t.disc_number AS disc_number, + t.year AS year, + t.bitrate AS bitrate, + t.sample_rate AS sample_rate, + t.bit_depth AS bit_depth, + t.channels AS channels, + t.codec AS codec, + t.musical_key AS musical_key, + t.file_path AS file_path, + t.file_size AS file_size, + t.added_at AS added_at, + aw.hash AS artwork_hash, + aw.format AS artwork_format, + t.rating AS rating, + -- Coalesced on both sides or on neither. The remote half + -- falls back to its display string, so leaving the local + -- one bare would file every track without a primary artist + -- ahead of the entire list, NULL sorting first. + COALESCE(ar.canonical_name, al.album_artist) AS sort_artist, + al.canonical_title AS sort_album + FROM track t + LEFT JOIN album al ON al.id = t.album_id + LEFT JOIN artist ar ON ar.id = t.primary_artist + LEFT JOIN artwork aw ON aw.id = al.artwork_id + WHERE (? IS NULL OR t.library_id = ?) + AND t.is_available = 1 + UNION ALL + SELECT 'remote', + rt.remote_id, + NULL, + rt.title, + rt.album_id, + rt.album, + rt.artist_id, + rt.artist, + NULL, + rt.duration_ms, + rt.track_no, + rt.disc_no, + rt.year, + rt.bitrate, + NULL, + NULL, + NULL, + rt.suffix, + NULL, + NULL, + rt.size, + rt.cached_at, + rt.artwork_hash, + NULL, + NULL, + COALESCE(rt.sort_artist, rt.artist), + COALESCE(rt.sort_album, rt.album) + FROM remote_track rt + WHERE rt.in_catalogue = 1 + -- A local library filter is a filter over local libraries; see + -- `list_library_albums`. + AND ? IS NULL + ) + WHERE (? IS NULL OR source = ?) + {order_clause} +"# + ) +} + +/// Every track the library can show, from the device and from the bound +/// server, as one sorted list. +/// +/// Not merged, on the same terms as the albums and the artists. A server track +/// carries none of the local user data — no rating, no like, no tags — because +/// none of it exists for a row that has no local counterpart. +#[tauri::command] +pub async fn list_library_tracks( + state: tauri::State<'_, AppState>, + library_id: Option, + source: Option, + order_by: Option, + direction: Option, +) -> AppResult { + let pool = state.require_profile_pool().await?; + let profile_id = state.require_profile_id().await?; + let artwork_dir = state.paths.profile_artwork_dir(profile_id); + + let order_clause = library_track_order_clause(order_by.as_deref(), direction.as_deref()); + let sql = library_tracks_sql(order_clause); + + let raw = sqlx::query_as::<_, LibraryTrackRawRow>(sqlx::AssertSqlSafe(sql)) + .bind(library_id) + .bind(library_id) + .bind(library_id) + .bind(source.as_deref()) + .bind(source.as_deref()) + .fetch_all(&*pool) + .await?; + + let items = expand_library_track_rows(raw, artwork_dir.clone()).await?; + + Ok(ListLibraryTracksResponse { + artwork_base: artwork_dir.to_string_lossy().into_owned(), + items, + }) +} + +/// Stitch thumbnail-existence flags onto the local half only. See +/// [`expand_library_album_rows`]. +async fn expand_library_track_rows( + raw: Vec, + artwork_dir: PathBuf, +) -> AppResult> { + tokio::task::spawn_blocking(move || { + raw.into_iter() + .map(|row| { + let local = row.source == "local"; + let (artwork_has_1x, artwork_has_2x) = match row.artwork_hash.as_deref() { + Some(hash) if local => { + let (p1, p2) = crate::thumbnails::thumbnail_paths_for(&artwork_dir, hash); + (p1.is_some(), p2.is_some()) + } + _ => (false, false), + }; + LibraryTrackRow { + source: row.source, + id: row.id, + library_id: row.library_id, + title: row.title, + album_id: row.album_id, + album_title: row.album_title, + artist_id: row.artist_id, + artist_name: row.artist_name, + artist_ids: row.artist_ids, + duration_ms: row.duration_ms, + track_number: row.track_number, + disc_number: row.disc_number, + year: row.year, + bitrate: row.bitrate, + sample_rate: row.sample_rate, + bit_depth: row.bit_depth, + channels: row.channels, + codec: row.codec, + musical_key: row.musical_key, + file_path: row.file_path, + file_size: row.file_size, + added_at: row.added_at, + artwork_hash: row.artwork_hash, + artwork_format: row.artwork_format, + artwork_has_1x, + artwork_has_2x, + rating: row.rating, + } + }) + .collect() + }) + .await + .map_err(|e| AppError::Other(format!("library track row expand join: {e}"))) +} + /// Both halves of the album listing, as one compound select. /// /// Split out of the command so the SQL can be exercised on its own: the @@ -2311,6 +2641,9 @@ mod tests { title, album_id, duration_ms, added_at, is_available, hlc_wall, hlc_logical, rating_hlc_wall, rating_hlc_logical) VALUES (1, 1, '/m/1.flac', 'h1', 1, 0, 'T1', 1, 300000, 500, 1, 0, 0, 0, 0)", + // The scanner always stamps a primary artist; a fixture that omits + // it would exercise a shape the library never holds. + "UPDATE track SET primary_artist = 1 WHERE id = 1", "INSERT INTO track_artist (track_id, artist_id, position) VALUES (1, 1, 0)", "INSERT INTO remote_artist (remote_id, name, artwork_hash, sort_key, mirrored_at) VALUES ('ar-1', 'Aphex Twin', 'aa11', 'aphex twin', 1)", @@ -2324,6 +2657,18 @@ mod tests { "INSERT INTO remote_track (remote_id, title, artist_id, duration_ms, cached_at, in_catalogue) VALUES ('t-2', 'R2', 'ar-1', 0, 1, 1)", + // Fully described, so the track listing has something to sort and + // render: the two above are bare identifiers on purpose. + "UPDATE remote_track + SET artist = 'Aphex Twin', album = 'Drukqs', album_id = 'al-1', + sort_artist = 'aphex twin', sort_album = 'drukqs', + track_no = 1, disc_no = 1, artwork_hash = 'bb22' + WHERE remote_id IN ('t-1', 't-2')", + // Cached for a playlist but never walked: outside the catalogue, + // so the library must not list it. + "INSERT INTO remote_track (remote_id, title, artist, duration_ms, cached_at, + in_catalogue) + VALUES ('t-3', 'Not in the catalogue', 'Someone', 0, 1, 0)", ] { sqlx::raw_sql(statement).execute(pool).await.unwrap(); } @@ -2438,6 +2783,132 @@ mod tests { assert!(albums(&pool, Some(99), None, order).await.is_empty()); } + async fn tracks( + pool: &SqlitePool, + library_id: Option, + source: Option<&str>, + order: &str, + ) -> Vec<(String, String, Option)> { + sqlx::query(sqlx::AssertSqlSafe(library_tracks_sql(order))) + .bind(library_id) + .bind(library_id) + .bind(library_id) + .bind(source) + .bind(source) + .fetch_all(pool) + .await + .unwrap() + .into_iter() + .map(|row| (row.get("source"), row.get("title"), row.get("rating"))) + .collect() + } + + #[tokio::test] + async fn the_track_listing_sorts_both_halves_against_each_other() { + let pool = pool().await; + seed(&pool).await; + + let rows = tracks(&pool, None, None, library_track_order_clause(None, None)).await; + // "aphex twin" before "bjork", from the normalised keys on both sides. + assert_eq!( + rows.iter().map(|row| row.1.as_str()).collect::>(), + vec!["R1", "R2", "T1"] + ); + // A server track has no rating, and that is not the same as unrated. + assert_eq!(rows[0].2, None); + } + + /// A track cached for a playlist is not part of the catalogue, and the + /// library must not list it — it would appear with no album and no way to + /// reach it. + #[tokio::test] + async fn the_track_listing_shows_only_the_mirrored_catalogue() { + let pool = pool().await; + seed(&pool).await; + + let titles: Vec = tracks(&pool, None, None, library_track_order_clause(None, None)) + .await + .into_iter() + .map(|row| row.1) + .collect(); + assert!(!titles.iter().any(|title| title == "Not in the catalogue")); + } + + /// Rating is local-only, so the remote half has none. Sorting by it must + /// not read a missing rating as the worst one. + #[tokio::test] + async fn sorting_by_rating_puts_the_unratable_last_either_way() { + let pool = pool().await; + seed(&pool).await; + sqlx::raw_sql("UPDATE track SET rating = 200 WHERE id = 1") + .execute(&pool) + .await + .unwrap(); + + for direction in ["asc", "desc"] { + let rows = tracks( + &pool, + None, + None, + library_track_order_clause(Some("rating"), Some(direction)), + ) + .await; + assert_eq!(rows[0].1, "T1", "{direction}: the rated track leads"); + assert!(rows[1..].iter().all(|row| row.2.is_none())); + } + } + + /// The sort dropdown and the persisted preference both carry the column + /// name. Matching on anything else sends the sort to the fallback clause + /// and does nothing visible — which is the quietest way for a sort to be + /// broken. + #[tokio::test] + async fn sorting_by_duration_uses_the_key_the_dropdown_sends() { + let pool = pool().await; + seed(&pool).await; + sqlx::raw_sql("UPDATE remote_track SET duration_ms = 999999 WHERE remote_id = 't-1'") + .execute(&pool) + .await + .unwrap(); + + let longest = tracks( + &pool, + None, + None, + library_track_order_clause(Some("duration_ms"), Some("desc")), + ) + .await; + assert_eq!(longest.first().map(|row| row.1.as_str()), Some("R1")); + + let shortest = tracks( + &pool, + None, + None, + library_track_order_clause(Some("duration_ms"), Some("asc")), + ) + .await; + assert_eq!(shortest.last().map(|row| row.1.as_str()), Some("R1")); + } + + #[tokio::test] + async fn the_track_filters_behave_like_the_album_ones() { + let pool = pool().await; + seed(&pool).await; + let order = library_track_order_clause(None, None); + + let local = tracks(&pool, None, Some("local"), order).await; + assert_eq!(local.len(), 1); + assert_eq!(local[0].0, "local"); + + let remote = tracks(&pool, None, Some("remote"), order).await; + assert_eq!(remote.len(), 2); + assert!(remote.iter().all(|row| row.0 == "remote")); + + let scoped = tracks(&pool, Some(1), None, order).await; + assert_eq!(scoped.len(), 1); + assert_eq!(scoped[0].0, "local"); + } + #[tokio::test] async fn the_artist_listing_sorts_and_derives_its_counts() { let pool = pool().await; diff --git a/src-tauri/crates/app/src/lib.rs b/src-tauri/crates/app/src/lib.rs index 7aa3a3dc..3e2efeeb 100644 --- a/src-tauri/crates/app/src/lib.rs +++ b/src-tauri/crates/app/src/lib.rs @@ -657,6 +657,7 @@ pub fn run() { commands::browse::list_albums, commands::browse::list_library_albums, commands::browse::list_library_artists, + commands::browse::list_library_tracks, commands::browse::list_artists, commands::browse::search_albums, commands::browse::search_artists, diff --git a/src-tauri/crates/app/src/remote/binding.rs b/src-tauri/crates/app/src/remote/binding.rs index 980a4f0d..76e55e17 100644 --- a/src-tauri/crates/app/src/remote/binding.rs +++ b/src-tauri/crates/app/src/remote/binding.rs @@ -296,20 +296,14 @@ mod tests { .connect(":memory:") .await .unwrap(); - for migration in [ - include_str!( - "../../../../migrations/profile/20260810120000_remote_source_projection.sql" - ), - include_str!("../../../../migrations/profile/20260810140000_remote_track_cache.sql"), - include_str!( - "../../../../migrations/profile/20260813090000_remote_track_full_hash.sql" - ), - include_str!( - "../../../../migrations/profile/20260816120000_remote_track_artist_id.sql" - ), - ] { - sqlx::raw_sql(migration).execute(&pool).await.unwrap(); - } + // The real migrator rather than a hand-listed subset: a + // migration that touches a table this list happened to omit + // breaks the fixture and not the code, which is a failure that + // teaches nothing. Same fixture the browse tests use. + sqlx::migrate!("../../migrations/profile") + .run(&pool) + .await + .unwrap(); pool } diff --git a/src-tauri/crates/app/src/remote/mirror.rs b/src-tauri/crates/app/src/remote/mirror.rs index 23ad5ef5..cf5e862f 100644 --- a/src-tauri/crates/app/src/remote/mirror.rs +++ b/src-tauri/crates/app/src/remote/mirror.rs @@ -933,27 +933,14 @@ mod tests { .execute(&pool) .await .unwrap(); - for migration in [ - include_str!( - "../../../../migrations/profile/20260810120000_remote_source_projection.sql" - ), - include_str!("../../../../migrations/profile/20260810140000_remote_track_cache.sql"), - include_str!( - "../../../../migrations/profile/20260813090000_remote_track_full_hash.sql" - ), - include_str!( - "../../../../migrations/profile/20260816120000_remote_track_artist_id.sql" - ), - include_str!( - "../../../../migrations/profile/20260824210000_remote_catalogue_mirror.sql" - ), - include_str!( - "../../../../migrations/profile/20260826090000_remote_album_sort_keys.sql" - ), - include_str!("../../../../migrations/profile/20260826140000_remote_artist_mirror.sql"), - ] { - sqlx::raw_sql(migration).execute(&pool).await.unwrap(); - } + // The real migrator, like every other remote fixture: a hand-listed + // subset breaks whenever a migration touches a table the list + // happened to omit. The upgrade test below lists them on purpose, + // because it needs the schema as it was *before* one of them. + sqlx::migrate!("../../migrations/profile") + .run(&pool) + .await + .unwrap(); pool } @@ -1402,6 +1389,63 @@ mod tests { clear(&pool).await.unwrap(); } + /// The upgrade path for the track sort keys: an album already walked, whose + /// count has not changed, would otherwise never be walked again — so its + /// tracks would keep their empty keys and keep sorting on the wrong + /// expression forever. The migration clears the stamps to force one walk. + #[tokio::test] + async fn the_sort_key_migration_makes_every_walked_album_stale() { + let pool = SqlitePoolOptions::new() + .max_connections(1) + .connect(":memory:") + .await + .unwrap(); + // Everything up to, but not including, the migration under test. + for migration in [ + include_str!( + "../../../../migrations/profile/20260810120000_remote_source_projection.sql" + ), + include_str!("../../../../migrations/profile/20260810140000_remote_track_cache.sql"), + include_str!( + "../../../../migrations/profile/20260813090000_remote_track_full_hash.sql" + ), + include_str!( + "../../../../migrations/profile/20260816120000_remote_track_artist_id.sql" + ), + include_str!( + "../../../../migrations/profile/20260824210000_remote_catalogue_mirror.sql" + ), + include_str!( + "../../../../migrations/profile/20260826090000_remote_album_sort_keys.sql" + ), + ] { + sqlx::raw_sql(migration).execute(&pool).await.unwrap(); + } + + let listed = album("al-1", 5); + upsert(&pool, &listed).await; + sqlx::query("UPDATE remote_album SET mirrored_at = 1 WHERE remote_id = 'al-1'") + .execute(&pool) + .await + .unwrap(); + assert!( + album_is_fresh(&pool, &listed).await.unwrap(), + "the album is walked and unchanged before the upgrade" + ); + + sqlx::raw_sql(include_str!( + "../../../../migrations/profile/20260826180000_remote_track_sort_keys.sql" + )) + .execute(&pool) + .await + .unwrap(); + + assert!( + !album_is_fresh(&pool, &listed).await.unwrap(), + "the upgrade must send it back through the walk" + ); + } + /// The defect the `mirrored_at` CASE exists for: an album that gains a /// track and is interrupted before its fetch must not read as fresh. #[tokio::test] diff --git a/src-tauri/crates/app/src/remote/mutation.rs b/src-tauri/crates/app/src/remote/mutation.rs index 6b402e95..2293ba14 100644 --- a/src-tauri/crates/app/src/remote/mutation.rs +++ b/src-tauri/crates/app/src/remote/mutation.rs @@ -574,20 +574,14 @@ mod tests { .connect(":memory:") .await .unwrap(); - for migration in [ - include_str!( - "../../../../migrations/profile/20260810120000_remote_source_projection.sql" - ), - include_str!("../../../../migrations/profile/20260810140000_remote_track_cache.sql"), - include_str!( - "../../../../migrations/profile/20260813090000_remote_track_full_hash.sql" - ), - include_str!( - "../../../../migrations/profile/20260816120000_remote_track_artist_id.sql" - ), - ] { - sqlx::raw_sql(migration).execute(&pool).await.unwrap(); - } + // The real migrator rather than a hand-listed subset: a + // migration that touches a table this list happened to omit + // breaks the fixture and not the code, which is a failure that + // teaches nothing. Same fixture the browse tests use. + sqlx::migrate!("../../migrations/profile") + .run(&pool) + .await + .unwrap(); pool } diff --git a/src-tauri/crates/app/src/remote/projection.rs b/src-tauri/crates/app/src/remote/projection.rs index d2a54254..b25489c4 100644 --- a/src-tauri/crates/app/src/remote/projection.rs +++ b/src-tauri/crates/app/src/remote/projection.rs @@ -37,6 +37,7 @@ use serde_json::Value; use sqlx::SqliteConnection; +use waveflow_core::metadata::name_match::normalize_name; use crate::{ error::AppResult, @@ -233,8 +234,9 @@ pub async fn cache_song(conn: &mut SqliteConnection, song: &SongItem) -> AppResu sqlx::query( "INSERT INTO remote_track (remote_id, title, artist, artist_id, album, album_id, duration_ms, track_no, disc_no, - year, genre, suffix, bitrate, size, artwork_hash, library_id, full_hash, cached_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + year, genre, suffix, bitrate, size, artwork_hash, library_id, full_hash, cached_at, + sort_artist, sort_album) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ON CONFLICT(remote_id) DO UPDATE SET -- A later cache pass for a bare id binds an empty title; keep the -- existing one rather than blanking a row we already labelled. @@ -254,7 +256,12 @@ pub async fn cache_song(conn: &mut SqliteConnection, song: &SongItem) -> AppResu artwork_hash = COALESCE(excluded.artwork_hash, artwork_hash), library_id = COALESCE(excluded.library_id, library_id), full_hash = COALESCE(excluded.full_hash, full_hash), - cached_at = excluded.cached_at", + cached_at = excluded.cached_at, + -- Keyed off the same COALESCE as the strings they are derived + -- from: a sparser later sighting must not blank a key any more + -- than it blanks the title it came from. + sort_artist = COALESCE(excluded.sort_artist, sort_artist), + sort_album = COALESCE(excluded.sort_album, sort_album)", ) .bind(&song.id) // An untitled row is still better than no row: the playlist keeps @@ -276,6 +283,14 @@ pub async fn cache_song(conn: &mut SqliteConnection, song: &SongItem) -> AppResu .bind(song.library_id.as_deref()) .bind(song.full_hash.as_deref()) .bind(chrono::Utc::now().timestamp_millis()) + // The comparison keys the unified track listing sorts artist and album + // on, through the very normaliser the local half's `canonical_*` columns + // went through. SQLite cannot fold a diacritic, so without these the two + // halves interleave wrongly. The title is deliberately absent: it has no + // canonical form locally either, so both halves sort it on their display + // string and stay consistent that way. + .bind(song.artist.as_deref().map(normalize_name)) + .bind(song.album.as_deref().map(normalize_name)) .execute(&mut *conn) .await?; Ok(()) @@ -709,20 +724,14 @@ mod tests { .connect(":memory:") .await .unwrap(); - for migration in [ - include_str!( - "../../../../migrations/profile/20260810120000_remote_source_projection.sql" - ), - include_str!("../../../../migrations/profile/20260810140000_remote_track_cache.sql"), - include_str!( - "../../../../migrations/profile/20260813090000_remote_track_full_hash.sql" - ), - include_str!( - "../../../../migrations/profile/20260816120000_remote_track_artist_id.sql" - ), - ] { - sqlx::raw_sql(migration).execute(&pool).await.unwrap(); - } + // The real migrator rather than a hand-listed subset: a + // migration that touches a table this list happened to omit + // breaks the fixture and not the code, which is a failure that + // teaches nothing. Same fixture the browse tests use. + sqlx::migrate!("../../migrations/profile") + .run(&pool) + .await + .unwrap(); pool } diff --git a/src-tauri/crates/app/src/remote/read.rs b/src-tauri/crates/app/src/remote/read.rs index 015c11bd..c6b265d6 100644 --- a/src-tauri/crates/app/src/remote/read.rs +++ b/src-tauri/crates/app/src/remote/read.rs @@ -214,20 +214,14 @@ mod tests { .connect(":memory:") .await .unwrap(); - for migration in [ - include_str!( - "../../../../migrations/profile/20260810120000_remote_source_projection.sql" - ), - include_str!("../../../../migrations/profile/20260810140000_remote_track_cache.sql"), - include_str!( - "../../../../migrations/profile/20260813090000_remote_track_full_hash.sql" - ), - include_str!( - "../../../../migrations/profile/20260816120000_remote_track_artist_id.sql" - ), - ] { - sqlx::raw_sql(migration).execute(&pool).await.unwrap(); - } + // The real migrator rather than a hand-listed subset: a + // migration that touches a table this list happened to omit + // breaks the fixture and not the code, which is a failure that + // teaches nothing. Same fixture the browse tests use. + sqlx::migrate!("../../migrations/profile") + .run(&pool) + .await + .unwrap(); pool } diff --git a/src-tauri/crates/app/src/remote/write.rs b/src-tauri/crates/app/src/remote/write.rs index 05f91dfa..fb0fac48 100644 --- a/src-tauri/crates/app/src/remote/write.rs +++ b/src-tauri/crates/app/src/remote/write.rs @@ -909,20 +909,14 @@ mod tests { .connect(":memory:") .await .unwrap(); - for migration in [ - include_str!( - "../../../../migrations/profile/20260810120000_remote_source_projection.sql" - ), - include_str!("../../../../migrations/profile/20260810140000_remote_track_cache.sql"), - include_str!( - "../../../../migrations/profile/20260813090000_remote_track_full_hash.sql" - ), - include_str!( - "../../../../migrations/profile/20260816120000_remote_track_artist_id.sql" - ), - ] { - sqlx::raw_sql(migration).execute(&pool).await.unwrap(); - } + // The real migrator rather than a hand-listed subset: a + // migration that touches a table this list happened to omit + // breaks the fixture and not the code, which is a failure that + // teaches nothing. Same fixture the browse tests use. + sqlx::migrate!("../../migrations/profile") + .run(&pool) + .await + .unwrap(); pool } diff --git a/src-tauri/migrations/profile/20260826180000_remote_track_sort_keys.sql b/src-tauri/migrations/profile/20260826180000_remote_track_sort_keys.sql new file mode 100644 index 00000000..500cd0ae --- /dev/null +++ b/src-tauri/migrations/profile/20260826180000_remote_track_sort_keys.sql @@ -0,0 +1,38 @@ +-- Sort keys for the mirrored tracks, for the same reason the albums have them. +-- +-- The unified track listing sorts both halves against each other, and the +-- local half sorts artist and album on `artist.canonical_name` / +-- `album.canonical_title` — forms produced by +-- `waveflow_core::metadata::name_match::normalize_name`, which lowercases, +-- folds diacritics and drops punctuation. SQLite reproduces none of that, so +-- a remote half sorted on its raw display strings interleaves wrongly and +-- splits an artist in two down the middle of the list. +-- +-- Only artist and album. The title has no canonical form on the local side +-- either — it sorts on `track.title COLLATE NOCASE` — so both halves sort the +-- title on their display string, which keeps that column consistent by using +-- the same expression rather than by normalising one side only. +-- +-- Filled by `projection::cache_song`, the single place a remote track row is +-- written: from the snapshot, from a change event, from a search hit and from +-- the catalogue walk alike. Nullable because rows cached before this migration +-- have none; the listing falls back to the display string, and any later +-- sighting of the track fills them in. +ALTER TABLE remote_track ADD COLUMN sort_artist TEXT; +ALTER TABLE remote_track ADD COLUMN sort_album TEXT; + +CREATE INDEX idx_remote_track_sort ON remote_track (sort_artist, sort_album, title); + +-- Existing rows have no keys, and nothing would ever give them any. +-- +-- The columns are nullable and the listing coalesces to the display string, +-- so an un-keyed row still renders — but it sorts on the wrong expression, +-- which is the whole defect these columns exist to fix. And it would sort that +-- way *forever*: the walk skips an album whose `song_count` is unchanged, so +-- `cache_song` never runs again for its tracks and the keys stay NULL. +-- +-- SQLite cannot compute them (it cannot fold a diacritic), so the stamps are +-- cleared instead: every album becomes stale, the next walk re-fetches it, and +-- `cache_song` fills the keys on the way through. One walk, and it is +-- incremental again afterwards. +UPDATE remote_album SET mirrored_at = NULL; diff --git a/src/components/common/AlbumLink.tsx b/src/components/common/AlbumLink.tsx index c7efa44d..2eefba55 100644 --- a/src/components/common/AlbumLink.tsx +++ b/src/components/common/AlbumLink.tsx @@ -6,6 +6,9 @@ interface AlbumLinkProps { * (loose tracks without an album row, e.g. single-file imports). */ albumId: number | null | undefined; onNavigate: (albumId: number) => void; + /** Overrides `onNavigate` when present. A server album has no local rowid, + * so it cannot be reached through `albumId` — it opens its own view. */ + onNavigateRemote?: () => void; fallback?: string; className?: string; } @@ -20,18 +23,23 @@ export function AlbumLink({ title, albumId, onNavigate, + onNavigateRemote, fallback = "—", className = "", }: AlbumLinkProps) { if (!title || !title.trim()) { return {fallback}; } - if (albumId == null) { + if (albumId == null && !onNavigateRemote) { return {title}; } const handleClick = (e: MouseEvent) => { e.stopPropagation(); - onNavigate(albumId); + if (onNavigateRemote) { + onNavigateRemote(); + return; + } + if (albumId != null) onNavigate(albumId); }; return ( diff --git a/src/components/common/ArtistLink.tsx b/src/components/common/ArtistLink.tsx index aace7efc..1b03a403 100644 --- a/src/components/common/ArtistLink.tsx +++ b/src/components/common/ArtistLink.tsx @@ -14,6 +14,10 @@ interface ArtistLinkProps { */ artistIds: string | null | undefined; onNavigate: (artistId: number) => void; + /** Overrides `onNavigate` when present, and makes the whole credit one + * link. A server track credits one artist and has no local rowids, so + * there is nothing to zip by index — it opens its own view. */ + onNavigateRemote?: () => void; /** Fallback text shown when `name` is null/empty (e.g. "—"). */ fallback?: string; /** Optional class applied to the wrapper span. */ @@ -36,6 +40,7 @@ export function ArtistLink({ name, artistIds, onNavigate, + onNavigateRemote, fallback = "—", className = "", }: ArtistLinkProps) { @@ -43,6 +48,23 @@ export function ArtistLink({ return {fallback}; } + if (onNavigateRemote) { + return ( + + + + ); + } + const names = name.split(", "); const ids = (artistIds ?? "") .split(",") diff --git a/src/components/views/LibraryView.tsx b/src/components/views/LibraryView.tsx index 507f752a..92796c03 100644 --- a/src/components/views/LibraryView.tsx +++ b/src/components/views/LibraryView.tsx @@ -37,6 +37,7 @@ import { useTranslation } from "react-i18next"; import type { LibraryTab } from "../../types"; import { Tab } from "../common/Tab"; import { RemoteArtwork } from "../common/RemoteArtwork"; +import { remotePlayTracks } from "../../lib/tauri/remoteServer"; import { useRemoteArtworkSrc } from "../../hooks/useRemoteArtworkSrc"; import { useRemoteSource } from "../../hooks/useRemoteSource"; import { @@ -81,7 +82,6 @@ import { } from "../../lib/tauri/library"; import { formatDuration, - listTracks, listLikedTrackIds, setTrackRating, toggleLikeTrack, @@ -92,8 +92,10 @@ import { listFolders, type LibraryAlbumRow, type LibraryArtistRow, + type LibraryTrackRow, listLibraryAlbums, listLibraryArtists, + listLibraryTracks, type GenreRow, type FolderRow, } from "../../lib/tauri/browse"; @@ -204,7 +206,7 @@ export function LibraryView({ null, ); const [coverReloadKey, setCoverReloadKey] = useState(0); - const [tracks, setTracks] = useState([]); + const [tracks, setTracks] = useState([]); const [albums, setAlbums] = useState([]); const librarySource = useLibrarySource(); const [artists, setArtists] = useState([]); @@ -319,16 +321,22 @@ export function LibraryView({ // "EmptyState flash" disappears because the data lands during the // very first paint instead of after the user picks a tab. useEffect(() => { - if (!tracksSort.isLoaded) return; + // Both preferences gate the fetch, for the reason on the albums effect. + if (!tracksSort.isLoaded || !librarySource.ready) return; let cancelled = false; // eslint-disable-next-line react-hooks/set-state-in-effect setLoading((p) => ({ ...p, morceaux: true })); - listTracks(null, tracksSort.sort) + listLibraryTracks( + null, + librarySource.source === "all" ? null : librarySource.source, + tracksSort.sort, + ) .then((list) => { if (!cancelled) setTracks(list); }) .catch((err) => { - if (!cancelled) console.error("[LibraryView] listTracks failed", err); + if (!cancelled) + console.error("[LibraryView] listLibraryTracks failed", err); }) .finally(() => { if (!cancelled) setLoading((p) => ({ ...p, morceaux: false })); @@ -336,7 +344,14 @@ export function LibraryView({ return () => { cancelled = true; }; - }, [librariesSignature, tracksSort.isLoaded, tracksSort.sort, editRefetch]); + }, [ + librariesSignature, + tracksSort.isLoaded, + tracksSort.sort, + librarySource.ready, + librarySource.source, + editRefetch, + ]); useEffect(() => { // Both preferences gate the fetch: loading with either default and again @@ -597,9 +612,37 @@ export function LibraryView({ // two different answers. const sourceFilterEmptied = librarySource.source !== "all" && - ((activeTab === "albums" && albums.length === 0) || + ((activeTab === "morceaux" && tracks.length === 0) || + (activeTab === "albums" && albums.length === 0) || (activeTab === "artistes" && artists.length === 0)); + // The two engines keep separate queues by design (RFC-005 decision 9), so a + // mixed list cannot produce a mixed queue. Playing a row therefore queues the + // run of rows from *its* source — which is why the chip on every row matters, + // and why narrowing the filter is how you get one continuous queue. + const playRow = useCallback( + (index: number) => { + const row = tracks[index]; + if (!row) return; + const run = tracks.filter((candidate) => candidate.source === row.source); + const at = run.findIndex((candidate) => candidate.id === row.id); + if (row.source === "remote") { + void remotePlayTracks( + run.map((candidate) => candidate.id), + Math.max(at, 0), + ).catch((err: unknown) => + console.error("[LibraryView] remotePlayTracks failed", err), + ); + return; + } + void playTracks(run.map(toLocalTrack), Math.max(at, 0), { + type: "library", + id: null, + }); + }, + [tracks, playTracks], + ); + const hasContent = (activeTab === "morceaux" && tracks.length > 0) || (activeTab === "albums" && albums.length > 0) || @@ -751,13 +794,23 @@ export function LibraryView({ nothing yet empties the list, and a control that disappears with the content it emptied leaves no way back. The sort dropdown has no such problem — it did not cause the emptiness — so it stays gated. */} - {(activeTab === "albums" || activeTab === "artistes") && ( + {(activeTab === "morceaux" || + activeTab === "albums" || + activeTab === "artistes") && (
+ {activeTab === "morceaux" && tracks.length > 0 && ( + + )} {activeTab === "albums" && albums.length > 0 && ( {activeTab === "morceaux" && ( <> -
- -
- playTracks(tracks, index, { - type: "library", - id: null, - }) - } + onPlayTrack={(index) => playRow(index)} currentTrackId={currentTrack?.id ?? null} isPlaying={isPlaying} likedIds={likedIds} @@ -843,11 +883,21 @@ export function LibraryView({ onContextMenuRow={trackContextMenu.open} onRowMenuKey={trackContextMenu.openFromKeyboard} isSelected={selection.isSelected} + onNavigateToRemoteAlbum={onNavigateToRemoteAlbum} + onNavigateToRemoteArtist={onNavigateToRemoteArtist} + singleClickPlay={singleClickPlay} onRowSelect={(track, e) => { // Modifier-driven selection always wins so multi-select // remains accessible even with single-click play on. + // Selection, and everything it feeds, speaks in local + // rowids. The table only hands us local rows here — a remote + // one has no `Track` to pass — so the list it ranges over is + // narrowed to match. + const localRows = tracks + .filter((row) => row.source === "local") + .map(toLocalTrack); if (e.shiftKey) { - selection.selectRange(track.id, tracks); + selection.selectRange(track.id, localRows); return; } if (e.ctrlKey || e.metaKey) { @@ -855,10 +905,11 @@ export function LibraryView({ return; } if (singleClickPlay) { - const idx = tracks.findIndex((tr) => tr.id === track.id); - if (idx >= 0) { - playTracks(tracks, idx, { type: "library", id: null }); - } + const idx = tracks.findIndex( + (row) => + row.source === "local" && Number(row.id) === track.id, + ); + if (idx >= 0) playRow(idx); selection.clear(); return; } @@ -1307,8 +1358,49 @@ function SortDropdown({ options, current, onChange, t }: SortDropdownProps) { // Tab-specific list components // ============================================================================= +/** + * A local library row as the `Track` the player, the selection and the + * playlist calls all speak. + * + * Not a cast. The row's identifiers are **text** — the two sources do not + * share an identifier type, so the unified listing hands both back as strings + * — and handing that object straight to code that compares `id` numerically + * makes every comparison silently false: no row ever reads as selected, and + * the queue is built from tracks the engine cannot match. Only ever called for + * a row whose source is local; a server row has no `Track` to become. + */ +function toLocalTrack(row: LibraryTrackRow): Track { + return { + id: Number(row.id), + library_id: row.library_id ?? 0, + title: row.title, + album_id: row.album_id != null ? Number(row.album_id) : null, + album_title: row.album_title, + artist_id: row.artist_id != null ? Number(row.artist_id) : null, + artist_name: row.artist_name, + artist_ids: row.artist_ids, + duration_ms: row.duration_ms, + track_number: row.track_number, + disc_number: row.disc_number, + year: row.year, + bitrate: row.bitrate, + sample_rate: row.sample_rate, + channels: row.channels, + bit_depth: row.bit_depth, + codec: row.codec, + musical_key: row.musical_key, + file_path: row.file_path ?? "", + file_size: row.file_size ?? 0, + added_at: row.added_at, + artwork_path: row.artwork_path, + artwork_path_1x: row.artwork_path_1x, + artwork_path_2x: row.artwork_path_2x, + rating: row.rating, + }; +} + interface TrackTableProps { - tracks: Track[]; + tracks: LibraryTrackRow[]; isLoading: boolean; view: TracksView; t: Translator; @@ -1335,6 +1427,14 @@ interface TrackTableProps { onRowMenuKey: (event: React.KeyboardEvent, track: Track) => boolean; isSelected: (id: number) => boolean; onRowSelect: (track: Track, e: React.MouseEvent) => void; + /** Whether a plain click plays instead of selecting. The table needs it + * because selection speaks in local rowids and a server row has none: it + * would otherwise be the only row a click does nothing to. */ + singleClickPlay: boolean; + /** A server track opens the remote detail views; the two catalogues are + * never merged, so they are never the same page. */ + onNavigateToRemoteAlbum: (remoteAlbumId: string) => void; + onNavigateToRemoteArtist: (remoteArtistId: string) => void; } function TrackTable({ @@ -1357,6 +1457,9 @@ function TrackTable({ onRowMenuKey, isSelected, onRowSelect, + singleClickPlay, + onNavigateToRemoteAlbum, + onNavigateToRemoteArtist, }: TrackTableProps) { "use no memo"; const unknown = t("library.table.unknown"); @@ -1459,18 +1562,41 @@ function TrackTable({ {virtualizer.getVirtualItems().map((virtualRow) => { const index = virtualRow.index; const track = tracks[index]; - const isCurrent = track.id === currentTrackId; - const isMenuOpen = openMenuTrackId === track.id; - const isRowSelected = isSelected(track.id); + // A server track has no local rowid, and none of the gestures below + // can accept one: it is in no local playlist, its rating lives in a + // file that is not here, and the like list keys on `track.id`. + const localId = track.source === "remote" ? null : Number(track.id); + const local = localId !== null; + // The row the user can act on as a local track, for the handlers + // that still speak `Track`. Converted once here — the identifiers + // are text on the wire, and a cast would leave them text. + const asTrack = local ? toLocalTrack(track) : null; + const isCurrent = localId !== null && localId === currentTrackId; + const isMenuOpen = localId !== null && openMenuTrackId === localId; + const isRowSelected = localId !== null && isSelected(localId); return ( // Row can't be a + )}
+ {/* A local playlist holds local tracks; the picker cannot + accept a server one. */} + {localId !== null && ( + <>
); diff --git a/src/lib/tauri/browse.ts b/src/lib/tauri/browse.ts index a1b3163c..b529d743 100644 --- a/src/lib/tauri/browse.ts +++ b/src/lib/tauri/browse.ts @@ -359,6 +359,107 @@ export async function listLibraryAlbums( }); } +/** A track of the library, from either source. + * + * The local half carries everything a `Track` does, so a row can be handed + * straight to the player. The remote half leaves the file-shaped fields null: + * there is no file here to rate, edit or hash. `rating: null` on a remote row + * means "cannot be rated", which is not the same as unrated. */ +export interface LibraryTrackRow { + source: LibrarySource; + id: string; + library_id: number | null; + title: string; + album_id: string | null; + album_title: string | null; + artist_id: string | null; + artist_name: string | null; + artist_ids: string | null; + duration_ms: number; + track_number: number | null; + disc_number: number | null; + year: number | null; + bitrate: number | null; + sample_rate: number | null; + bit_depth: number | null; + channels: number | null; + codec: string | null; + musical_key: string | null; + file_path: string | null; + file_size: number | null; + added_at: number; + artwork_path: string | null; + artwork_path_1x: string | null; + artwork_path_2x: string | null; + /** Remote only: resolved through the server cover cache. */ + artwork_hash: string | null; + rating: number | null; +} + +interface LibraryTrackRowSlim + extends Omit< + LibraryTrackRow, + "artwork_path" | "artwork_path_1x" | "artwork_path_2x" | "artwork_hash" + > { + artwork_hash: string | null; + artwork_format: string | null; + artwork_has_1x: boolean; + artwork_has_2x: boolean; +} + +interface ListLibraryTracksResponse { + artwork_base: string; + items: LibraryTrackRowSlim[]; +} + +/** + * Every track the library can show, from the device and from the bound + * server, as one sorted list. Not merged, on the same terms as the albums. + * + * Only tracks the catalogue walk mirrored appear: a track cached because a + * playlist referenced it is not part of the server's browsable catalogue and + * would show up with no album and no way to reach it. + */ +export async function listLibraryTracks( + libraryId: number | null, + source: LibrarySource | null, + options?: { orderBy?: string; direction?: "asc" | "desc" }, +): Promise { + const resp = await invoke("list_library_tracks", { + libraryId, + source, + orderBy: options?.orderBy ?? null, + direction: options?.direction ?? null, + }); + const sep = pathSep(resp.artwork_base); + return resp.items.map((item) => { + const local = item.source === "local"; + const { + artwork_format, + artwork_has_1x, + artwork_has_2x, + artwork_hash, + ...rest + } = item; + return { + ...rest, + artwork_path: + local && artwork_hash && artwork_format + ? `${resp.artwork_base}${sep}${artwork_hash}.${artwork_format}` + : null, + artwork_path_1x: + local && artwork_hash && artwork_has_1x + ? `${resp.artwork_base}${sep}${artwork_hash}_1x.jpg` + : null, + artwork_path_2x: + local && artwork_hash && artwork_has_2x + ? `${resp.artwork_base}${sep}${artwork_hash}_2x.jpg` + : null, + artwork_hash: local ? null : artwork_hash, + }; + }); +} + /** An artist of the library, from either source. Same contract as * `LibraryAlbumRow`: text identifier, `source` says how to read it. */ export interface LibraryArtistRow {