diff --git a/src/state.rs b/src/state.rs index f9e13de..87644aa 100644 --- a/src/state.rs +++ b/src/state.rs @@ -462,14 +462,42 @@ impl PlaybackState { ] } - /// Cache the URL as Failed. Does not modify tracks directly — the actor - /// fans out per-track `MetadataUpdated` + `TrackFailed` events for removal. + /// Cache the URL as Failed. Does not modify queue/active tracks directly + /// — the actor fans out per-track `MetadataUpdated` + `TrackFailed` + /// events for removal. Playlist entries are updated here so a bad URL + /// in the playlist stops showing "Loading..." with no indication that + /// it failed. We also fire `PersistMetadata` so the failure title + /// lands in `media_cache` and survives rehydration (the playlist load + /// JOINs media_cache to compute title + pending). fn handle_download_failed(&mut self, url: &str, error: &str) -> Vec { self.media_cache .insert(url.to_string(), MediaStatus::Failed(error.to_string())); - // No track mutation here. No publish either — the actor's per-track - // TrackFailed events will trigger their own PublishSnapshot. - Vec::new() + let failed_title = format!("Failed to load: {error}"); + let mut changed = false; + for entry in &mut self.playlist { + if entry.url == url && entry.pending { + entry.title = failed_title.clone(); + entry.duration = "--:--".into(); + entry.thumbnail = None; + entry.pending = false; + changed = true; + } + } + if changed { + vec![ + Effect::PersistMetadata { + item_id: TrackId::nil(), + url: url.to_string(), + title: failed_title, + duration: "--:--".into(), + thumbnail: None, + source: "ytdlp".into(), + }, + Effect::PublishSnapshot, + ] + } else { + Vec::new() + } } /// Add a playlist entry, replacing any existing entry with the same URL. @@ -1967,6 +1995,101 @@ mod tests { assert!(s.queue.is_empty()); assert!(effects.iter().any(|e| matches!(e, Effect::PublishSnapshot))); } + + #[test] + fn test_download_failed_updates_playlist_entry() { + // Bug fix: a failed download for a URL in the playlist must + // mark the entry as failed (title = "Failed to load: ", + // pending = false). Without this, a bad URL in the playlist stays + // "Loading..." forever. + let mut s = PlaybackState::default(); + s.transition( + &Event::PlaylistEntryAdded(placeholder_playlist_entry(1, "https://example.com/bad")), + 0, + ); + assert!(s.playlist[0].pending); + + let effects = s.transition( + &Event::DownloadFailed { + url: "https://example.com/bad".into(), + error: "yt-dlp returned 404".into(), + }, + 0, + ); + + let entry = &s.playlist[0]; + assert_eq!(entry.title, "Failed to load: yt-dlp returned 404"); + assert_eq!(entry.duration, "--:--"); + assert!(entry.thumbnail.is_none()); + assert!(!entry.pending); + // PersistMetadata so the failure title lands in media_cache and + // survives rehydration. + assert!(effects.iter().any(|e| matches!( + e, + Effect::PersistMetadata { url, title, .. } + if url == "https://example.com/bad" + && title == "Failed to load: yt-dlp returned 404" + ))); + assert!(effects.iter().any(|e| matches!(e, Effect::PublishSnapshot))); + } + + #[test] + fn test_download_failed_only_updates_pending_playlist_entries() { + // A non-pending playlist entry (e.g. already resolved) for the same + // URL should NOT be clobbered by a later failure for that URL. + let mut s = PlaybackState::default(); + s.transition( + &Event::PlaylistEntryAdded(placeholder_playlist_entry(1, "https://example.com/ok")), + 0, + ); + s.transition( + &Event::DownloadResolved { + url: "https://example.com/ok".into(), + title: "OK Title".into(), + duration: "5:00".into(), + thumbnail: Some("https://img/ok.jpg".into()), + source: "ytdlp".into(), + }, + 0, + ); + assert!(!s.playlist[0].pending); + let original_title = s.playlist[0].title.clone(); + + let effects = s.transition( + &Event::DownloadFailed { + url: "https://example.com/ok".into(), + error: "boom".into(), + }, + 0, + ); + + // Title preserved — the entry was already resolved. + assert_eq!(s.playlist[0].title, original_title); + // No metadata effect needed (no pending entries to flip). + assert!(!effects + .iter() + .any(|e| matches!(e, Effect::PersistMetadata { .. }))); + assert!(!effects.iter().any(|e| matches!(e, Effect::PublishSnapshot))); + } + + #[test] + fn test_download_failed_with_no_playlist_match_emits_nothing() { + // URL fails but isn't in the playlist and isn't a queue/active + // track. State machine only updates media_cache. No effects. + let mut s = PlaybackState::default(); + let effects = s.transition( + &Event::DownloadFailed { + url: "https://example.com/orphan".into(), + error: "no such host".into(), + }, + 0, + ); + assert!(matches!( + s.media_cache.get("https://example.com/orphan"), + Some(MediaStatus::Failed(_)) + )); + assert!(effects.is_empty()); + } } /// Deterministic simulation testing: random event sequences with invariant @@ -2135,6 +2258,22 @@ mod dst { Some(&crate::state::MediaStatus::Failed(format!("err-{n}"))), "DownloadFailed({url}) did not record Failed in media_cache", ); + // Cross-state invariant: a Failed cache must not leave + // any matching playlist entries in pending state. They + // should be flipped to the failure title. + for entry in &state.playlist { + if entry.url == url { + assert!( + !entry.pending, + "DownloadFailed({url}) left a playlist entry pending: {entry:?}", + ); + assert!( + entry.title.starts_with("Failed to load: "), + "DownloadFailed({url}) did not set failure title; got: {}", + entry.title, + ); + } + } effects } Op::MoveQueueItem { from, to } => { @@ -2461,6 +2600,32 @@ mod dst { "ReorderQueue.new_order still contains just-removed track {removed}" ); } + + // 14. Every non-pending playlist entry's URL must be present in + // media_cache (Resolved or Failed). If a non-pending entry + // has no matching cache row, the entry would render with a + // stale title on rehydration. + for entry in &state.playlist { + if !entry.pending { + let url = entry.url.clone(); + let cached = state.media_cache.get(&url); + assert!( + matches!( + cached, + Some(crate::state::MediaStatus::Resolved { .. }) + | Some(crate::state::MediaStatus::Failed(_)) + ), + "non-pending playlist entry for {url} has no media_cache row (would rehydrate with stale title): {entry:?}", + ); + if let Some(crate::state::MediaStatus::Failed(err)) = cached { + assert_eq!( + entry.title, + format!("Failed to load: {err}"), + "non-pending Failed entry title mismatch for {url}", + ); + } + } + } } fn is_persist_effect(e: &Effect) -> bool {