From ecfc61d427383a16e3913c1dd25440315784771c Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Wed, 26 Aug 2026 19:27:11 +0200 Subject: [PATCH 1/3] feat(library): browse both sources from one track list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third tab on the pattern the albums proved, and the one that carries a consequence worth stating rather than hiding. 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 carries none of the local user data, and the controls for it are absent rather than inert: no rating, no like, no playlist membership. Five hollow stars that do nothing read as "unrated", which is a different claim from "cannot be rated here" — and the like list, the membership map and the selection all key on a rowid a server track does not have. 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. Two things the smoke tests caught before a human could: - Sorting by rating filed every server track as though it were rated zero. Rating is local-only, so the clause now puts the unratable last in both directions: not rated here and rated badly are different things. - The local half's artist sort key was left bare while the remote half's was coalesced to its display string. A local track with no primary artist therefore sorted ahead of the entire library, NULL first. Coalesced on both sides or on neither. The two shared link components grew an optional remote navigation override rather than being bypassed: a server track's artist and album open the remote views, and the credit is one link because the server credits one artist and there are no local rowids to zip by index. Localised across all 17 locales. Claude-Session: https://claude.ai/code/session_01Ls4aG74DPcE4UUQtrPc5ji --- .../rfcs/RFC-005-remote-source-and-sync-v2.md | 19 +- src-tauri/crates/app/src/commands/browse.rs | 436 ++++++++++++++++++ src-tauri/crates/app/src/lib.rs | 1 + src-tauri/crates/app/src/remote/binding.rs | 3 + src-tauri/crates/app/src/remote/mirror.rs | 3 + src-tauri/crates/app/src/remote/mutation.rs | 3 + src-tauri/crates/app/src/remote/projection.rs | 24 +- src-tauri/crates/app/src/remote/read.rs | 3 + src-tauri/crates/app/src/remote/write.rs | 3 + .../20260826180000_remote_track_sort_keys.sql | 24 + src/components/common/AlbumLink.tsx | 12 +- src/components/common/ArtistLink.tsx | 22 + src/components/views/LibraryView.tsx | 243 +++++++--- src/lib/tauri/browse.ts | 101 ++++ 14 files changed, 821 insertions(+), 76 deletions(-) create mode 100644 src-tauri/migrations/profile/20260826180000_remote_track_sort_keys.sql 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..19ae7a67 100644 --- a/src-tauri/crates/app/src/commands/browse.rs +++ b/src-tauri/crates/app/src/commands/browse.rs @@ -471,6 +471,333 @@ 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 { + let dir_default_desc = matches!( + order_by, + Some("duration") | 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"), "ASC") => "ORDER BY duration_ms ASC", + (Some("duration"), "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 +2638,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 +2654,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 +2780,100 @@ 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())); + } + } + + #[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..2b2801ab 100644 --- a/src-tauri/crates/app/src/remote/binding.rs +++ b/src-tauri/crates/app/src/remote/binding.rs @@ -307,6 +307,9 @@ mod tests { include_str!( "../../../../migrations/profile/20260816120000_remote_track_artist_id.sql" ), + include_str!( + "../../../../migrations/profile/20260826180000_remote_track_sort_keys.sql" + ), ] { sqlx::raw_sql(migration).execute(&pool).await.unwrap(); } diff --git a/src-tauri/crates/app/src/remote/mirror.rs b/src-tauri/crates/app/src/remote/mirror.rs index 23ad5ef5..e73aebb4 100644 --- a/src-tauri/crates/app/src/remote/mirror.rs +++ b/src-tauri/crates/app/src/remote/mirror.rs @@ -951,6 +951,9 @@ mod tests { "../../../../migrations/profile/20260826090000_remote_album_sort_keys.sql" ), include_str!("../../../../migrations/profile/20260826140000_remote_artist_mirror.sql"), + include_str!( + "../../../../migrations/profile/20260826180000_remote_track_sort_keys.sql" + ), ] { sqlx::raw_sql(migration).execute(&pool).await.unwrap(); } diff --git a/src-tauri/crates/app/src/remote/mutation.rs b/src-tauri/crates/app/src/remote/mutation.rs index 6b402e95..2f4d9361 100644 --- a/src-tauri/crates/app/src/remote/mutation.rs +++ b/src-tauri/crates/app/src/remote/mutation.rs @@ -585,6 +585,9 @@ mod tests { include_str!( "../../../../migrations/profile/20260816120000_remote_track_artist_id.sql" ), + include_str!( + "../../../../migrations/profile/20260826180000_remote_track_sort_keys.sql" + ), ] { sqlx::raw_sql(migration).execute(&pool).await.unwrap(); } diff --git a/src-tauri/crates/app/src/remote/projection.rs b/src-tauri/crates/app/src/remote/projection.rs index d2a54254..dc576d1a 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(()) @@ -720,6 +735,9 @@ mod tests { include_str!( "../../../../migrations/profile/20260816120000_remote_track_artist_id.sql" ), + include_str!( + "../../../../migrations/profile/20260826180000_remote_track_sort_keys.sql" + ), ] { sqlx::raw_sql(migration).execute(&pool).await.unwrap(); } diff --git a/src-tauri/crates/app/src/remote/read.rs b/src-tauri/crates/app/src/remote/read.rs index 015c11bd..e297ac78 100644 --- a/src-tauri/crates/app/src/remote/read.rs +++ b/src-tauri/crates/app/src/remote/read.rs @@ -225,6 +225,9 @@ mod tests { include_str!( "../../../../migrations/profile/20260816120000_remote_track_artist_id.sql" ), + include_str!( + "../../../../migrations/profile/20260826180000_remote_track_sort_keys.sql" + ), ] { sqlx::raw_sql(migration).execute(&pool).await.unwrap(); } diff --git a/src-tauri/crates/app/src/remote/write.rs b/src-tauri/crates/app/src/remote/write.rs index 05f91dfa..f0c87718 100644 --- a/src-tauri/crates/app/src/remote/write.rs +++ b/src-tauri/crates/app/src/remote/write.rs @@ -920,6 +920,9 @@ mod tests { include_str!( "../../../../migrations/profile/20260816120000_remote_track_artist_id.sql" ), + include_str!( + "../../../../migrations/profile/20260826180000_remote_track_sort_keys.sql" + ), ] { sqlx::raw_sql(migration).execute(&pool).await.unwrap(); } 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..4535f441 --- /dev/null +++ b/src-tauri/migrations/profile/20260826180000_remote_track_sort_keys.sql @@ -0,0 +1,24 @@ +-- 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); 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..28cad9f8 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 as unknown as Track[], 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,20 @@ export function LibraryView({ onContextMenuRow={trackContextMenu.open} onRowMenuKey={trackContextMenu.openFromKeyboard} isSelected={selection.isSelected} + onNavigateToRemoteAlbum={onNavigateToRemoteAlbum} + onNavigateToRemoteArtist={onNavigateToRemoteArtist} 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", + ) as unknown as Track[]; if (e.shiftKey) { - selection.selectRange(track.id, tracks); + selection.selectRange(track.id, localRows); return; } if (e.ctrlKey || e.metaKey) { @@ -855,10 +904,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; } @@ -1308,7 +1358,7 @@ function SortDropdown({ options, current, onChange, t }: SortDropdownProps) { // ============================================================================= interface TrackTableProps { - tracks: Track[]; + tracks: LibraryTrackRow[]; isLoading: boolean; view: TracksView; t: Translator; @@ -1335,6 +1385,10 @@ interface TrackTableProps { onRowMenuKey: (event: React.KeyboardEvent, track: Track) => boolean; isSelected: (id: number) => boolean; onRowSelect: (track: Track, e: React.MouseEvent) => void; + /** 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 +1411,8 @@ function TrackTable({ onRowMenuKey, isSelected, onRowSelect, + onNavigateToRemoteAlbum, + onNavigateToRemoteArtist, }: TrackTableProps) { "use no memo"; const unknown = t("library.table.unknown"); @@ -1459,18 +1515,29 @@ 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`. Narrowed once here rather than cast at + // every call site. + const asTrack = local ? (track as unknown as 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 { From d75bf691b5fc62e438631936b172c9fbf406700e Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Wed, 26 Aug 2026 19:50:56 +0200 Subject: [PATCH 2/3] fix(library): convert rows to tracks instead of casting their ids MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three kept, and the first was a silent breakage. The row objects the unified listing returns carry **text** identifiers — the two sources do not share an id type — and three places handed them straight to code that compares `id` numerically. A cast satisfies the compiler and changes nothing at runtime, so every comparison was quietly false: no row read as selected, and the queue was built from tracks whose ids the engine could not match. There is now one conversion, used by all three, that actually parses. Sorting by duration did nothing. The dropdown and the persisted preference both carry `duration_ms`, the clause matched `duration`, and the mismatch fell through to the default artist/album ordering — the quietest way for a sort to be broken. A test now pins the key to what the dropdown sends. The sort-key migration left existing rows unfixable. The columns are nullable and the listing coalesces to the display string, so an un-keyed row renders — and sorts on the wrong expression, which is the defect those columns exist to remove. Worse, it would sort that way forever: the walk skips an album whose count is unchanged, so `cache_song` never runs again for its tracks. SQLite cannot compute the keys, so the migration clears the walk stamps instead — every album goes back through the walk once and comes out keyed. An upgrade test builds the schema as it was before the migration, walks an album, applies the migration and asserts it is stale again. And the fixtures stopped listing migrations by hand. Adding a statement that touched `remote_album` broke every fixture whose list happened to omit that table — a failure that teaches nothing about the code. They run the real migrator now, the same one the browse tests use. The upgrade test keeps its hand-written list, because needing the schema as it was before a migration is exactly what that list is for. Claude-Session: https://claude.ai/code/session_01Ls4aG74DPcE4UUQtrPc5ji --- src-tauri/crates/app/src/commands/browse.rs | 41 ++++++++- src-tauri/crates/app/src/remote/binding.rs | 25 ++---- src-tauri/crates/app/src/remote/mirror.rs | 89 ++++++++++++++----- src-tauri/crates/app/src/remote/mutation.rs | 25 ++---- src-tauri/crates/app/src/remote/projection.rs | 25 ++---- src-tauri/crates/app/src/remote/read.rs | 25 ++---- src-tauri/crates/app/src/remote/write.rs | 25 ++---- .../20260826180000_remote_track_sort_keys.sql | 14 +++ src/components/views/LibraryView.tsx | 55 ++++++++++-- 9 files changed, 205 insertions(+), 119 deletions(-) diff --git a/src-tauri/crates/app/src/commands/browse.rs b/src-tauri/crates/app/src/commands/browse.rs index 19ae7a67..52cf2e4c 100644 --- a/src-tauri/crates/app/src/commands/browse.rs +++ b/src-tauri/crates/app/src/commands/browse.rs @@ -560,9 +560,12 @@ struct LibraryTrackRawRow { /// 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") | Some("added_at") | Some("year") | Some("rating") + Some("duration_ms") | Some("added_at") | Some("year") | Some("rating") ); let dir = match direction { Some(d) if d.eq_ignore_ascii_case("asc") => "ASC", @@ -590,8 +593,8 @@ fn library_track_order_clause(order_by: Option<&str>, direction: Option<&str>) - (Some("album"), "DESC") => { "ORDER BY sort_album COLLATE NOCASE DESC, disc_number, track_number" } - (Some("duration"), "ASC") => "ORDER BY duration_ms ASC", - (Some("duration"), "DESC") => "ORDER BY duration_ms DESC", + (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", @@ -2855,6 +2858,38 @@ mod tests { } } + /// 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; diff --git a/src-tauri/crates/app/src/remote/binding.rs b/src-tauri/crates/app/src/remote/binding.rs index 2b2801ab..76e55e17 100644 --- a/src-tauri/crates/app/src/remote/binding.rs +++ b/src-tauri/crates/app/src/remote/binding.rs @@ -296,23 +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" - ), - include_str!( - "../../../../migrations/profile/20260826180000_remote_track_sort_keys.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 e73aebb4..cf5e862f 100644 --- a/src-tauri/crates/app/src/remote/mirror.rs +++ b/src-tauri/crates/app/src/remote/mirror.rs @@ -933,30 +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"), - include_str!( - "../../../../migrations/profile/20260826180000_remote_track_sort_keys.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 } @@ -1405,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 2f4d9361..2293ba14 100644 --- a/src-tauri/crates/app/src/remote/mutation.rs +++ b/src-tauri/crates/app/src/remote/mutation.rs @@ -574,23 +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" - ), - include_str!( - "../../../../migrations/profile/20260826180000_remote_track_sort_keys.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 dc576d1a..b25489c4 100644 --- a/src-tauri/crates/app/src/remote/projection.rs +++ b/src-tauri/crates/app/src/remote/projection.rs @@ -724,23 +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" - ), - include_str!( - "../../../../migrations/profile/20260826180000_remote_track_sort_keys.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 e297ac78..c6b265d6 100644 --- a/src-tauri/crates/app/src/remote/read.rs +++ b/src-tauri/crates/app/src/remote/read.rs @@ -214,23 +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" - ), - include_str!( - "../../../../migrations/profile/20260826180000_remote_track_sort_keys.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 f0c87718..fb0fac48 100644 --- a/src-tauri/crates/app/src/remote/write.rs +++ b/src-tauri/crates/app/src/remote/write.rs @@ -909,23 +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" - ), - include_str!( - "../../../../migrations/profile/20260826180000_remote_track_sort_keys.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 index 4535f441..500cd0ae 100644 --- a/src-tauri/migrations/profile/20260826180000_remote_track_sort_keys.sql +++ b/src-tauri/migrations/profile/20260826180000_remote_track_sort_keys.sql @@ -22,3 +22,17 @@ 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/views/LibraryView.tsx b/src/components/views/LibraryView.tsx index 28cad9f8..a41cec8d 100644 --- a/src/components/views/LibraryView.tsx +++ b/src/components/views/LibraryView.tsx @@ -635,7 +635,7 @@ export function LibraryView({ ); return; } - void playTracks(run as unknown as Track[], Math.max(at, 0), { + void playTracks(run.map(toLocalTrack), Math.max(at, 0), { type: "library", id: null, }); @@ -892,9 +892,9 @@ export function LibraryView({ // 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", - ) as unknown as Track[]; + const localRows = tracks + .filter((row) => row.source === "local") + .map(toLocalTrack); if (e.shiftKey) { selection.selectRange(track.id, localRows); return; @@ -1357,6 +1357,47 @@ 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: LibraryTrackRow[]; isLoading: boolean; @@ -1521,9 +1562,9 @@ function TrackTable({ 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`. Narrowed once here rather than cast at - // every call site. - const asTrack = local ? (track as unknown as Track) : null; + // 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); From e994838063f0037f36415396f110639a5dba5026 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Wed, 26 Aug 2026 21:20:24 +0200 Subject: [PATCH 3/3] fix(library): a plain click on a server track plays it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With single-click play on, every row in the track list responded to a click except the server ones. Selection is keyed on local rowids and a server track has none, so the handler simply returned — which reads as a dead row rather than as an unselectable one. A plain click now plays it, through the same path the double-click and the keyboard already used. Modifier clicks stay inert: shift and control are selection gestures, and there is no selection here to extend. No Rust touched. Claude-Session: https://claude.ai/code/session_01Ls4aG74DPcE4UUQtrPc5ji --- src/components/views/LibraryView.tsx | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/components/views/LibraryView.tsx b/src/components/views/LibraryView.tsx index a41cec8d..92796c03 100644 --- a/src/components/views/LibraryView.tsx +++ b/src/components/views/LibraryView.tsx @@ -885,6 +885,7 @@ export function LibraryView({ 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. @@ -1426,6 +1427,10 @@ 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; @@ -1452,6 +1457,7 @@ function TrackTable({ onRowMenuKey, isSelected, onRowSelect, + singleClickPlay, onNavigateToRemoteAlbum, onNavigateToRemoteArtist, }: TrackTableProps) { @@ -1577,7 +1583,19 @@ function TrackTable({ tabIndex={0} role="button" onClick={(e) => { - if (asTrack) onRowSelect(asTrack, e); + if (asTrack) { + onRowSelect(asTrack, e); + return; + } + // A server row has nothing to select — selection is keyed on + // local rowids. With single-click play on, every other row + // responds and this one would not, which reads as a dead row + // rather than as an unselectable one. Modifier clicks stay + // inert: they are selection gestures, and there is no + // selection here to extend. + if (singleClickPlay && !e.shiftKey && !e.ctrlKey && !e.metaKey) { + onPlayTrack(index); + } }} onDoubleClick={() => onPlayTrack(index)} onKeyDown={(e) => {