Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@

### Fixed

- **Cover art no longer sticks on the previous album with native streaming**: With spotatui as the playback device, the cover pane kept showing the last album for seconds after a skip, and for a track started from the native queue it showed the wrong art for the entire song. The art was read from the polled Spotify context, which lags behind a skip and never reports a natively queued track at all - those play through a direct `player.load` that Spirc does not surface - so the pane stayed pinned to whatever the context still held. librespot already hands over the album art in the very `TrackChanged` event that starts the song, so that art now wins. Each URL it carries is already the same resolved `i.scdn.co` form the Web API returns, so the cover-art cache key stays stable when the polled context later catches up and takes over. The context item is still used when librespot supplies no covers at all, which is the normal case for local files, whose art is embedded in the file ([#402](https://github.com/LargeModGames/spotatui/issues/402)).

- **Plugins: `spotatui.get_lyrics` no longer returns the previous track's lyrics**: Calling it from a `track_change` handler - the obvious place to call it - delivered the outgoing track's lyrics and spent the one-shot callback, so a plugin's lyrics never updated on skip. The runner ticks the script engine before its shared track-change detector, so at that moment `lyrics_status` was still `Found` for the old track and the request resolved synchronously against it. The immediate-delivery path now also checks that the stored lyrics belong to the item playing right now, falling back to waiting for the in-flight fetch when they do not.

- **No more "Access token missing" error screen between songs**: With playback on an external Spotify Connect device, a track transition could throw up the full-screen error route with a 401 `Access token missing` from `me/player`, even though the music never stopped and the very next poll succeeded with the same token. Spotify's player service intermittently rejects a valid token, and spotatui amplified it: every 401 forced a full token refresh (which rotates the PKCE refresh token, so a whole new token family was minted every few seconds), the single retry fired immediately and landed back inside the same failure window, and the playback poll then treated the result as fatal. Forced refreshes are now rate limited to one per 30 seconds and a 401 inside that window is retried with the token already in hand, the post-401 retry backs off first so it clears the transition, and the playback poll tolerates a short run of 401s before surfacing anything. Every non-2xx Spotify response is now logged with its endpoint, status, body, and the age of the token that was attached (never the token itself), so the remaining server-side behaviour is diagnosable from a log. Startup now also says which Spotify app the session actually signed in as: the setup wizard makes the shared ncspot client ID the primary for *both* of its options and keeps your own app as the fallback, so "I set up my own app" has never meant "I am using my own app" — and nothing said so ([#395](https://github.com/LargeModGames/spotatui/issues/395)).
Expand Down
6 changes: 6 additions & 0 deletions src/core/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -983,6 +983,12 @@ pub struct NativeTrackInfo {
pub album: String, // Reserved for future use (e.g., displaying album in playbar)
pub duration_ms: u32,
pub kind: NativeTrackKind,
/// Album art URL carried by librespot's own `TrackChanged` payload, so cover
/// art follows the track librespot is actually decoding. The polled Spotify
/// context is not a usable source here: it lags by seconds after a skip, and
/// for a natively queued track (played via a direct `player.load`, which Spirc
/// never reports) it stays on the *previous* track for the whole song. (#402)
pub image_url: Option<String>,
}

/// A node in the playlist folder hierarchy from Spotify's rootlist
Expand Down
89 changes: 88 additions & 1 deletion src/infra/media_metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,16 @@ pub fn current_playback_snapshot(app: &App) -> Option<PlaybackSnapshot> {
title: native_info.name.clone(),
artists: vec![native_info.artists_display.clone()],
album: native_info.album.clone(),
image_url: image_url_from_context_item(context.and_then(|ctx| ctx.item.as_ref())),
// Prefer the art librespot handed us with this very track. The polled
// context is a per-track race: after a skip it still holds the previous
// item for seconds, and for a natively queued track it never reports the
// right one at all, so reading art from it pinned the *previous* album's
// cover (#402). Fall back to the context item only when librespot gave
// us no covers, which keeps the old behavior for that corner.
image_url: native_info
.image_url
.clone()
.or_else(|| image_url_from_context_item(context.and_then(|ctx| ctx.item.as_ref()))),
duration_ms: native_info.duration_ms,
},
item_kind,
Expand Down Expand Up @@ -602,6 +611,7 @@ mod tests {
album: "Native Album".to_string(),
duration_ms: 123_000,
kind: NativeTrackKind::Track,
image_url: None,
});
app.native_playback_origin = Some(NativePlaybackOrigin::RawList);

Expand All @@ -617,6 +627,82 @@ mod tests {
assert!(snapshot.is_playing);
}

/// #402: after a skip (and for the whole of a natively queued track) the
/// polled context still holds the *previous* item, so its cover art belongs to
/// the previous album. librespot's own `TrackChanged` art must win.
#[test]
fn native_track_art_wins_over_a_stale_context_item() {
let mut app = app();
app.is_streaming_active = true;
app.native_track_info = Some(NativeTrackInfo {
name: "Native Track".to_string(),
artists_display: "Native Artist".to_string(),
album: "Native Album".to_string(),
duration_ms: 123_000,
kind: NativeTrackKind::Track,
image_url: Some("https://i.scdn.co/image/current".to_string()),
});
// The context lags on the previous track, whose art is a different album's.
app.current_playback_context = Some(playback_context(PlayableItem::Track(track()), true));

let snapshot = current_playback_snapshot(&app).unwrap();

assert_eq!(
snapshot.metadata.image_url.as_deref(),
Some("https://i.scdn.co/image/current"),
"the stale context item's art must not override librespot's"
);
}

/// Some payloads carry no covers at all (notably local files, whose art is
/// embedded in the file rather than on the CDN). The context item then stays
/// the only art source, so the pre-#402 fallback must survive. The context
/// here describes the *same* track that is playing, which is the only case
/// where trusting it is right.
#[test]
fn native_track_falls_back_to_context_art_without_librespot_covers() {
let mut app = app();
app.is_streaming_active = true;
app.native_track_info = Some(NativeTrackInfo {
name: "Track".to_string(),
artists_display: "Artist".to_string(),
album: "Album".to_string(),
duration_ms: 123_000,
kind: NativeTrackKind::Track,
image_url: None,
});
app.current_playback_context = Some(playback_context(PlayableItem::Track(track()), true));

let snapshot = current_playback_snapshot(&app).unwrap();

assert_eq!(
snapshot.metadata.image_url.as_deref(),
Some("https://example.com/cover.jpg")
);
}

/// Boundary of the #402 fix: the librespot art is only consulted while native
/// metadata is authoritative. Once `get_current_playback` decides the API has
/// caught up it clears `native_track_info`, and the snapshot goes back to the
/// context item wholesale. That handoff is correct when the API really did
/// catch up, but note that `api_confirms_native_info_is_current` accepts a
/// bare *title* match (see `playback.rs`), so two consecutive same-titled
/// tracks still hand over early and show the previous album's art.
#[test]
fn context_art_is_used_once_native_info_is_cleared() {
let mut app = app();
app.is_streaming_active = true;
app.native_track_info = None;
app.current_playback_context = Some(playback_context(PlayableItem::Track(track()), true));

let snapshot = current_playback_snapshot(&app).unwrap();

assert_eq!(
snapshot.metadata.image_url.as_deref(),
Some("https://example.com/cover.jpg")
);
}

#[test]
fn ignores_stale_native_play_state_for_api_metadata() {
let mut app = app();
Expand All @@ -639,6 +725,7 @@ mod tests {
album: "Native Album".to_string(),
duration_ms: 123_000,
kind: NativeTrackKind::Track,
image_url: None,
});
app.current_playback_context = Some(playback_context(PlayableItem::Track(track()), true));

Expand Down
16 changes: 16 additions & 0 deletions src/infra/player/events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -685,6 +685,22 @@ async fn handle_player_events(
album: album.clone(),
duration_ms: audio_item.duration_ms,
kind,
// `covers` arrives widest-first: the pinned librespot fork sorts it
// descending by width in `get_covers` (metadata/src/audio/item.rs)
// before building the list, and the filter that follows preserves that
// order, so `.first()` is the largest. Each url is already a fully
// resolved `https://i.scdn.co/image/<file_id>` — the same CDN form the
// Web API returns, so the cover-art key stays stable when the polled
// context later catches up and takes over. The empty-string filter
// matters: librespot substitutes into a session-supplied url template,
// so a blank template yields `Some("")`, which would defeat the
// fallback below instead of deferring to it. Local files legitimately
// carry no covers at all (their art is embedded in the file).
image_url: audio_item
.covers
.first()
.map(|cover| cover.url.clone())
.filter(|url| !url.is_empty()),
});

app.song_progress_ms = 0;
Expand Down
2 changes: 2 additions & 0 deletions src/infra/scripting/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1890,6 +1890,7 @@ mod data_read_tests {
album: "The Album".to_string(),
duration_ms: 180_000,
kind: NativeTrackKind::Track,
image_url: None,
});
}

Expand Down Expand Up @@ -1950,6 +1951,7 @@ mod data_read_tests {
album: "The Album".to_string(),
duration_ms: 180_000,
kind: NativeTrackKind::Track,
image_url: None,
});
app.desired_lyrics_identity = Some(("Track A".to_string(), "The Artist".to_string()));
app.lyrics_status = LyricsStatus::Found;
Expand Down
2 changes: 2 additions & 0 deletions src/tui/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,7 @@ mod tests {
album: "The Album".to_string(),
duration_ms: 180_000,
kind: crate::core::app::NativeTrackKind::Track,
image_url: None,
});

assert_eq!(playback_window_title(&app), "The Track — The Artist");
Expand All @@ -327,6 +328,7 @@ mod tests {
album: "The Album".to_string(),
duration_ms: 180_000,
kind: crate::core::app::NativeTrackKind::Track,
image_url: None,
});

assert_eq!(playback_window_title(&app), "The]2;Bad Track — TheArtist");
Expand Down
Loading