diff --git a/CLAUDE.md b/CLAUDE.md index 1a84c19..f7661fe 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -81,6 +81,7 @@ These bite you if you ignore them — they're the contract the rest of the codeb - **Modal accessibility**: every modal calls [`useModalA11y(isOpen, onClose)`](src/hooks/useModalA11y.ts) — Escape-close, Tab focus trap, focus restoration. Container gets `role="dialog"` + `aria-modal="true"` + `aria-labelledby` (stable heading id) or `aria-label` (conditional heading). Don't roll bespoke `useEffect` Escape handlers. - **Right panels are flex siblings, not overlays**: `NowPlayingPanel` / `QueuePanel` / `LyricsPanel` are mounted as flex children of the outer row in `AppLayout`. The center column has `min-w-0` so wide tables collapse instead of pushing the panel off-screen. - **Process-wide offline mode**: every outbound HTTP path (Deezer, Last.fm, similar, LRCLIB) checks `offline::is_offline()` first and short-circuits to an empty payload or cache. Persisted in `app_setting['network.offline_mode']`. Treat new HTTP code paths the same way. +- **Outbound `playlist + field: "tracks"` ops carry a snapshot map (Phase 1.j.b)**: every command in [`commands/playlist.rs`](src-tauri/crates/app/src/commands/playlist.rs) that inserts tracks (`add_track_to_playlist`, `add_tracks_to_playlist`, `add_source_to_playlist`) calls [`sync::track_snapshots::build_snapshots(conn, &track_ids)`](src-tauri/crates/app/src/sync/track_snapshots.rs) inside the same SQLite transaction and folds the result into the outbound payload as `snapshots: { "": { title, artist?, duration_ms? } }`. The server stores the snapshot in `playlist_track.snapshot_*` and filters its public share preview on `snapshot_title IS NOT NULL`, so tracks without one stay invisible to the wider web. **Don't shadow this** — emitting a `tracks` insert op without `snapshots` regresses the share preview for that playlist until any other client re-syncs it. `delete` + `set` (reorder) ops don't need snapshots (no display change on the receiving side). - **Adding a new player-bar action**: default it into the overflow ("⋯") menu via [`MoreActionsMenu`](src/components/player/MoreActionsMenu.tsx) first; promote to primary only when usage warrants it; add a Settings pin toggle if both modes make sense. See [`docs/features/ui.md`](docs/features/ui.md#player-bar-layout). ## Feature catalogue diff --git a/src-tauri/crates/app/src/commands/playlist.rs b/src-tauri/crates/app/src/commands/playlist.rs index f01cd31..f687937 100644 --- a/src-tauri/crates/app/src/commands/playlist.rs +++ b/src-tauri/crates/app/src/commands/playlist.rs @@ -363,6 +363,11 @@ pub async fn add_track_to_playlist( let mut tx = pool.begin().await?; append_track_conn(&mut tx, playlist_id, track_id, now).await?; let entity_id = crate::sync::canonical::ensure_local_playlist(&mut tx, playlist_id).await?; + // Phase 1.j.b — fold per-track snapshots into the outbound + // payload so the server's `playlist_track.snapshot_*` columns + // land populated and the public share preview can render the + // track without resolving the local-i64 id cross-device. + let snapshots = crate::sync::track_snapshots::build_snapshots(&mut tx, &[track_id]).await?; crate::sync::hooks::enqueue_op_in_tx( &mut tx, &crate::sync::hooks::PendingOpDraft { @@ -370,7 +375,10 @@ pub async fn add_track_to_playlist( entity_id, field: Some("tracks".into()), op: "insert".into(), - payload: Some(serde_json::json!({ "track_ids": [track_id] })), + payload: Some(serde_json::json!({ + "track_ids": [track_id], + "snapshots": snapshots, + })), }, ) .await?; @@ -403,6 +411,9 @@ pub async fn add_tracks_to_playlist( let mut tx = pool.begin().await?; let inserted = append_tracks_conn(&mut tx, playlist_id, &track_ids, now).await?; let entity_id = crate::sync::canonical::ensure_local_playlist(&mut tx, playlist_id).await?; + // Phase 1.j.b — per-track snapshots for the public share + // preview. See [`add_track_to_playlist`] for the rationale. + let snapshots = crate::sync::track_snapshots::build_snapshots(&mut tx, &track_ids).await?; // One coalesced op for the whole batch — emitting N ops would // cost N Lamport draws and bloat the queue without giving the // server side any extra signal. @@ -413,7 +424,10 @@ pub async fn add_tracks_to_playlist( entity_id, field: Some("tracks".into()), op: "insert".into(), - payload: Some(serde_json::json!({ "track_ids": track_ids })), + payload: Some(serde_json::json!({ + "track_ids": track_ids, + "snapshots": snapshots, + })), }, ) .await?; @@ -560,6 +574,9 @@ pub async fn add_source_to_playlist( let mut tx = pool.begin().await?; let inserted = append_tracks_conn(&mut tx, playlist_id, &track_ids, now_millis()).await?; let entity_id = crate::sync::canonical::ensure_local_playlist(&mut tx, playlist_id).await?; + // Phase 1.j.b — per-track snapshots for the public share + // preview. + let snapshots = crate::sync::track_snapshots::build_snapshots(&mut tx, &track_ids).await?; crate::sync::hooks::enqueue_op_in_tx( &mut tx, &crate::sync::hooks::PendingOpDraft { @@ -569,6 +586,7 @@ pub async fn add_source_to_playlist( op: "insert".into(), payload: Some(serde_json::json!({ "track_ids": track_ids, + "snapshots": snapshots, "via_source": { "type": source_type, "id": source_id }, })), }, diff --git a/src-tauri/crates/app/src/commands/share.rs b/src-tauri/crates/app/src/commands/share.rs index ee7b193..2f2fc7a 100644 --- a/src-tauri/crates/app/src/commands/share.rs +++ b/src-tauri/crates/app/src/commands/share.rs @@ -110,7 +110,9 @@ pub async fn share_link_mint( url, }) } - reqwest::StatusCode::NOT_FOUND => Err(AppError::Other("playlist not found or not owned by the active profile".into())), + reqwest::StatusCode::NOT_FOUND => Err(AppError::Other( + "playlist not found or not owned by the active profile".into(), + )), other => Err(AppError::Other(format!( "share mint returned {other} ({})", resp.text().await.unwrap_or_default() @@ -150,7 +152,9 @@ pub async fn share_link_revoke( write_cached_token(&pool, &playlist_canonical, None).await?; Ok(()) } - reqwest::StatusCode::NOT_FOUND => Err(AppError::Other("playlist not found or not owned by the active profile".into())), + reqwest::StatusCode::NOT_FOUND => Err(AppError::Other( + "playlist not found or not owned by the active profile".into(), + )), other => Err(AppError::Other(format!( "share revoke returned {other} ({})", resp.text().await.unwrap_or_default() @@ -172,7 +176,9 @@ pub async fn share_link_status( let playlist_canonical = canonical::canonical_for_local(&mut conn, canonical::ENTITY_PLAYLIST, playlist_id) .await? - .ok_or(AppError::Other("playlist not found or not owned by the active profile".into()))?; + .ok_or(AppError::Other( + "playlist not found or not owned by the active profile".into(), + ))?; drop(conn); let token = read_cached_token(&pool, &playlist_canonical).await?; @@ -208,7 +214,9 @@ async fn resolve_canonicals( let playlist_canonical = canonical::canonical_for_local(&mut conn, canonical::ENTITY_PLAYLIST, playlist_id) .await? - .ok_or(AppError::Other("playlist not found or not owned by the active profile".into()))?; + .ok_or(AppError::Other( + "playlist not found or not owned by the active profile".into(), + ))?; drop(conn); let profile_canonical = crate::db::profile_meta::canonical_id_for(&state.app_db, profile_id) @@ -240,7 +248,10 @@ async fn build_share_url(app_db: &SqlitePool, token: &str) -> AppResult const CACHE_KEY_PREFIX: &str = "share.token."; -async fn read_cached_token(pool: &SqlitePool, playlist_canonical: &str) -> AppResult> { +async fn read_cached_token( + pool: &SqlitePool, + playlist_canonical: &str, +) -> AppResult> { let key = format!("{CACHE_KEY_PREFIX}{playlist_canonical}"); let value: Option = sqlx::query_scalar("SELECT value FROM profile_setting WHERE key = ?") diff --git a/src-tauri/crates/app/src/db/profile_meta.rs b/src-tauri/crates/app/src/db/profile_meta.rs index 6d2cf8b..8e8f46a 100644 --- a/src-tauri/crates/app/src/db/profile_meta.rs +++ b/src-tauri/crates/app/src/db/profile_meta.rs @@ -37,11 +37,10 @@ use crate::error::AppResult; /// boot after the first post-migration boot) hits zero rows and /// commits an empty transaction, which is essentially free. pub async fn backfill_canonical_ids(pool: &SqlitePool) -> AppResult { - let needs_uuid: Vec = sqlx::query_scalar( - "SELECT id FROM profile WHERE canonical_id IS NULL ORDER BY id", - ) - .fetch_all(pool) - .await?; + let needs_uuid: Vec = + sqlx::query_scalar("SELECT id FROM profile WHERE canonical_id IS NULL ORDER BY id") + .fetch_all(pool) + .await?; if needs_uuid.is_empty() { return Ok(0); @@ -80,21 +79,16 @@ pub async fn ensure_canonical_id(pool: &SqlitePool, profile_id: i64) -> AppResul // Re-read — either we wrote `candidate` or a racing caller wrote // its own value first. In both cases the row now has a non-NULL // canonical, and the read returns the winning value. - canonical_id_for(pool, profile_id) - .await? - .ok_or_else(|| { - crate::error::AppError::Other(format!( - "profile {profile_id} disappeared mid-ensure_canonical_id" - )) - }) + canonical_id_for(pool, profile_id).await?.ok_or_else(|| { + crate::error::AppError::Other(format!( + "profile {profile_id} disappeared mid-ensure_canonical_id" + )) + }) } /// Look up the canonical id of a given profile. `None` for rows that /// don't exist (deleted) or haven't been backfilled yet. -pub async fn canonical_id_for( - pool: &SqlitePool, - profile_id: i64, -) -> AppResult> { +pub async fn canonical_id_for(pool: &SqlitePool, profile_id: i64) -> AppResult> { let row: Option> = sqlx::query_scalar("SELECT canonical_id FROM profile WHERE id = ?") .bind(profile_id) diff --git a/src-tauri/crates/app/src/sync/drain.rs b/src-tauri/crates/app/src/sync/drain.rs index 0bca579..578f4cc 100644 --- a/src-tauri/crates/app/src/sync/drain.rs +++ b/src-tauri/crates/app/src/sync/drain.rs @@ -390,9 +390,7 @@ mod tests { field: Some("name".into()), op: "set".into(), payload: Some(serde_json::json!({ "value": "Soirée" })), - profile_canonical_id: Some( - "11111111-2222-4333-8444-555555555555".into(), - ), + profile_canonical_id: Some("11111111-2222-4333-8444-555555555555".into()), }], }; let v = serde_json::to_value(&body).unwrap(); diff --git a/src-tauri/crates/app/src/sync/mod.rs b/src-tauri/crates/app/src/sync/mod.rs index bf66497..d24d919 100644 --- a/src-tauri/crates/app/src/sync/mod.rs +++ b/src-tauri/crates/app/src/sync/mod.rs @@ -56,4 +56,5 @@ pub mod hooks; pub mod lamport; pub mod mode; pub mod queue; +pub mod track_snapshots; pub mod ws; diff --git a/src-tauri/crates/app/src/sync/track_snapshots.rs b/src-tauri/crates/app/src/sync/track_snapshots.rs new file mode 100644 index 0000000..a8e7ae0 --- /dev/null +++ b/src-tauri/crates/app/src/sync/track_snapshots.rs @@ -0,0 +1,253 @@ +//! Per-track snapshots for outbound `playlist + field: "tracks"` ops +//! (Phase 1.j.b — wire bump to populate the server's +//! `playlist_track.snapshot_*` columns). +//! +//! ## Why a snapshot +//! +//! The `track_id` field in the outbound payload is the SOURCE +//! desktop's local-i64 id. The server can't resolve it cross-device +//! (a track with id=42 on device A is unrelated to id=42 on device +//! B), so a remote viewer would see only an opaque integer. The +//! snapshot carries the displayable columns (`title`, `artist`, +//! `duration_ms`) alongside the id, which is what the server's +//! [`db::playlist_track`](https://github.com/InstaZDLL/waveflow-server/blob/main/src/db.rs) +//! materialiser stores and what the public share preview at +//! `/api/v1/share/playlists/{token}` renders to the wider web. +//! +//! ## Wire shape +//! +//! Returns a JSON object keyed by track id as a STRING (JSON object +//! keys can't be ints, and the server-side parser does the same +//! conversion): +//! +//! ```json +//! { "42": { "title": "One More Time", "artist": "Daft Punk", "duration_ms": 320000 }, +//! "43": { "title": "Around the World", "duration_ms": 280000 } } +//! ``` +//! +//! Empty input → empty object. A track id whose row was deleted +//! between the playlist write and the snapshot SELECT is silently +//! dropped from the map; the server's apply pipeline tolerates the +//! id-without-snapshot case (the row lands NULL-snapshot and is +//! invisible to the public preview until the next sync re-emits). +//! +//! ## Atomicity +//! +//! Called from inside the same `&mut SqliteConnection` transaction +//! as the playlist write + the outbox enqueue. The SELECT is a +//! pure read against the `track` + `track_artist` tables — neither +//! is mutated by the playlist path — so the snapshot reflects the +//! state the user just acted on without an extra pool acquire. + +use serde_json::{json, Map, Value}; +use sqlx::SqliteConnection; + +use crate::error::AppResult; + +/// Build the `snapshots` payload for a batch of track ids. Always +/// returns a JSON object — empty when `track_ids` is empty, partial +/// when some ids resolve to no row. The caller folds the result +/// into the outbound payload alongside `track_ids`. +pub async fn build_snapshots(conn: &mut SqliteConnection, track_ids: &[i64]) -> AppResult { + if track_ids.is_empty() { + return Ok(Value::Object(Map::new())); + } + + // Build the IN-clause placeholders. We can't bind a slice + // directly to SQLite — sqlx 0.9 has no `Encode for Vec` on + // the SQLite backend — so we expand `?, ?, ?, …` and bind one + // by one. The id list comes from server-trusted internal state + // (the caller already validated it against the playlist), so + // the placeholder count is the upper bound on the SQL string + // size, not the user input. + // `iter::repeat().take(n)` rather than the newer `repeat_n` + // helper — the latter is stable only since Rust 1.82 and the + // repo's MSRV is 1.80. Mirrors the same pattern in + // commands/radio.rs:203. + let placeholders = std::iter::repeat("?") + .take(track_ids.len()) + .collect::>() + .join(", "); + let sql = format!( + "SELECT t.id, t.title, t.duration_ms, + (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 + FROM track t + WHERE t.id IN ({placeholders})" + ); + + // `AssertSqlSafe` is the repo's audited path for dynamic + // `IN (?, ?, …)` expansions (see commands/radio.rs:225 for the + // mirror pattern). The placeholder string is built from + // `track_ids.len()` only, so user input never reaches the SQL + // text — only the bind values. + let mut query = + sqlx::query_as::<_, (i64, String, i64, Option)>(sqlx::AssertSqlSafe(sql)); + for id in track_ids { + query = query.bind(*id); + } + let rows = query.fetch_all(&mut *conn).await?; + + let mut snapshots = Map::with_capacity(rows.len()); + for (id, title, duration_ms, artist) in rows { + let mut entry = Map::new(); + entry.insert("title".to_string(), Value::String(title)); + if let Some(a) = artist { + entry.insert("artist".to_string(), Value::String(a)); + } + entry.insert("duration_ms".to_string(), json!(duration_ms)); + snapshots.insert(id.to_string(), Value::Object(entry)); + } + Ok(Value::Object(snapshots)) +} + +#[cfg(test)] +mod tests { + use super::*; + use sqlx::SqlitePool; + + /// Bootstrap a minimal `track` + `artist` + `track_artist` schema + /// for the snapshot SELECT. We don't run the full migration set — + /// just the two tables the query touches — to keep the test + /// surface tight. A future repo-wide test harness can replace + /// this with `sqlx::test`. + async fn setup(pool: &SqlitePool) { + sqlx::query( + "CREATE TABLE track ( + id INTEGER PRIMARY KEY, + title TEXT NOT NULL, + duration_ms INTEGER NOT NULL + )", + ) + .execute(pool) + .await + .unwrap(); + sqlx::query("CREATE TABLE artist (id INTEGER PRIMARY KEY, name TEXT NOT NULL)") + .execute(pool) + .await + .unwrap(); + sqlx::query( + "CREATE TABLE track_artist ( + track_id INTEGER NOT NULL, + artist_id INTEGER NOT NULL, + position INTEGER NOT NULL, + PRIMARY KEY (track_id, artist_id) + )", + ) + .execute(pool) + .await + .unwrap(); + } + + #[tokio::test] + async fn empty_input_yields_empty_object() { + let pool = SqlitePool::connect(":memory:").await.unwrap(); + let mut conn = pool.acquire().await.unwrap(); + let out = build_snapshots(&mut conn, &[]).await.unwrap(); + assert_eq!(out, json!({})); + } + + #[tokio::test] + async fn populates_title_artist_duration() { + let pool = SqlitePool::connect(":memory:").await.unwrap(); + setup(&pool).await; + sqlx::query( + "INSERT INTO track (id, title, duration_ms) VALUES (1, 'One More Time', 320000)", + ) + .execute(&pool) + .await + .unwrap(); + sqlx::query("INSERT INTO artist (id, name) VALUES (10, 'Daft Punk')") + .execute(&pool) + .await + .unwrap(); + sqlx::query("INSERT INTO track_artist (track_id, artist_id, position) VALUES (1, 10, 0)") + .execute(&pool) + .await + .unwrap(); + + let mut conn = pool.acquire().await.unwrap(); + let out = build_snapshots(&mut conn, &[1]).await.unwrap(); + assert_eq!( + out, + json!({ + "1": { "title": "One More Time", "artist": "Daft Punk", "duration_ms": 320000 } + }) + ); + } + + #[tokio::test] + async fn artist_collapses_multi_with_join_string() { + let pool = SqlitePool::connect(":memory:").await.unwrap(); + setup(&pool).await; + sqlx::query("INSERT INTO track (id, title, duration_ms) VALUES (2, 'Get Lucky', 369000)") + .execute(&pool) + .await + .unwrap(); + sqlx::query( + "INSERT INTO artist (id, name) VALUES (10, 'Daft Punk'), (11, 'Pharrell Williams')", + ) + .execute(&pool) + .await + .unwrap(); + sqlx::query( + "INSERT INTO track_artist (track_id, artist_id, position) VALUES (2, 10, 0), (2, 11, 1)", + ) + .execute(&pool) + .await + .unwrap(); + + let mut conn = pool.acquire().await.unwrap(); + let out = build_snapshots(&mut conn, &[2]).await.unwrap(); + let entry = out.get("2").unwrap(); + assert_eq!( + entry["artist"].as_str(), + Some("Daft Punk, Pharrell Williams") + ); + } + + #[tokio::test] + async fn missing_track_id_silently_dropped() { + let pool = SqlitePool::connect(":memory:").await.unwrap(); + setup(&pool).await; + sqlx::query("INSERT INTO track (id, title, duration_ms) VALUES (5, 'A', 100)") + .execute(&pool) + .await + .unwrap(); + let mut conn = pool.acquire().await.unwrap(); + // Ask for 5 (exists) + 999 (doesn't). Output covers only 5. + let out = build_snapshots(&mut conn, &[5, 999]).await.unwrap(); + let obj = out.as_object().unwrap(); + assert_eq!(obj.len(), 1); + assert!(obj.contains_key("5")); + } + + #[tokio::test] + async fn artist_absent_yields_no_artist_field() { + let pool = SqlitePool::connect(":memory:").await.unwrap(); + setup(&pool).await; + sqlx::query("INSERT INTO track (id, title, duration_ms) VALUES (3, 'Untitled', 60000)") + .execute(&pool) + .await + .unwrap(); + let mut conn = pool.acquire().await.unwrap(); + let out = build_snapshots(&mut conn, &[3]).await.unwrap(); + let entry = out.get("3").unwrap().as_object().unwrap(); + assert_eq!( + entry.get("title").and_then(|v| v.as_str()), + Some("Untitled") + ); + assert_eq!( + entry.get("duration_ms").and_then(|v| v.as_i64()), + Some(60000) + ); + assert!( + !entry.contains_key("artist"), + "artist must be omitted, not null" + ); + } +}