Skip to content

Commit 2fd498c

Browse files
fix(app): resolve the playing item through the owner (#511)
# Summary The four actions on "the item playing now" (jump to album `a`, jump to artist `A`, jump to context `o`, add the playing track to a playlist `w`/`W`) read the cached Spotify playback context directly. While a decoded source (Local Files, Subsonic, Qobuz, Internet Radio, YouTube) or the native queue slot owns the sink, that context names the *suspended* Spotify track, so the keys fetched or staged the wrong item: with a Qobuz track audible, `a` opened the suspended track's album with a real `albums/{id}/tracks` request, and `W` offered to add a song the user was not hearing to a Spotify playlist. They now resolve through one `App::playing_item()` in `core/app/playback_routing.rs`, which folds `playback_owner()` and the queue slot into one answer, the way Like already did. Like goes through the same resolver, so its own predicate and `queue_now_spotify_track_uri` are gone. A queued Spotify track resolves through the slot: its URI for the picker, its album id (or the album lookup from the track id when the slot track carries none), its first artist. A decoded owner answers with a status message. A new ratchet counter, `direct_playback_context_reads`, counts every read of `App::current_playback_context` outside the ownership resolver and the snapshot builder, pins at 93 (97 before this PR), and may only fall. Deliberate behaviour changes: 1. With a decoded source or a decoded queue item playing, the five keys show a status message instead of acting on the suspended Spotify track. 2. A queued Spotify track now jumps to its own album and artist and stages itself in the picker. `o` on a queued track says the slot has no play context. 3. The three jump keys say "Nothing is playing" where they were silent (no session, no item, or no context). A context with no item (an ad plays) still opens. 4. Jump to artist on a Spotify track with no artist id says so instead of doing nothing. The copy-URL keys keep their handler-side guard and still refuse a queued Spotify track; converting them needs the resolver outside `core/app/` and is a follow-up. # Testing - `cargo fmt --all -- --check` - `cargo clippy -- -D warnings` on slim, headless, headless-streaming, mcp-only, ai-dj-only, default, and the Windows stand-in for all-sources (`audio-viz-cpal,windows-media` in place of `audio-viz,mpris`): clean. - `cargo test` on slim (953), headless (588), mcp-only (1076), ai-dj-only (1230), default (1260), five sources without cover-art (1730, plus the pre-existing Windows-only `uri_round_trip` failure). - `tools/check_gates_ratchet.sh main`: ok. `direct_playback_context_reads` new at 93, `test_attribute_total` 1816 -> 1826, the rest unchanged. - New tests: the queue slot beats the suspended context for album, artist, context and the picker; the album lookup fallback; no context and a context without an item; a pure test over all five owners; the counter matcher. - Not run here: the Linux `all-sources` leg and macOS. # Additional notes Follow-ups, not in this PR: under native streaming with a client-side shuffle session or a direct URI-list load, `current_playback_context.context` is `None` or stale, so `o` can open the wrong context; the copy-URL asymmetry above; the stale Like clause in the playbar-controls comment in `tui/ui/player.rs`. --- <sub>💬 Questions or want to chat with other contributors? Join the [spotatui Discord](https://spotatui.com/discord).</sub> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Jump-to-album, artist, and context actions now correctly handle queued Spotify tracks. - Playlist and save actions use the currently playing queued track when applicable. - Actions provide clear status messages when nothing is playing or the source is not Spotify. - **Bug Fixes** - Improved track resolution across native queue and Spotify playback, preventing stale playback information. - **Documentation** - Updated the unreleased changelog and playback guidance to reflect source-aware playback actions. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 4c01587 commit 2fd498c

14 files changed

Lines changed: 450 additions & 147 deletions

File tree

.github/copilot-instructions.md

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -217,8 +217,12 @@ session exists.
217217
librespot - the native flag stays true, so driving librespot directly resumes
218218
the wrong player.
219219
- While the native queue slot owns the sink, `current_playback_context` names the
220-
*suspended* context's track; resolve track-level actions through
221-
`queue_now_spotify_track_uri()` / `queue_now_track()`.
220+
*suspended* context's track. Inside `core/app/`, resolve the playing *track*
221+
through `App::playing_item()` (`core/app/playback_routing.rs`): it answers
222+
with the slot's `TrackInfo` (`uri`, `album_id`, `artist_refs`; a slot has
223+
no play context) and refuses a decoded owner. Outside `core/app/` read the
224+
`PlaybackSnapshot` (`infra/media_metadata.rs`), never the field:
225+
`direct_playback_context_reads` counts every other reader and may only fall.
222226
- Radio is in `active_decoded_source` but deliberately out of
223227
`active_queueable_decoded_source` (repeat/shuffle) and
224228
`active_source_position_ms` (seek).

AGENTS.md

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -219,8 +219,12 @@ session exists.
219219
librespot - the native flag stays true, so driving librespot directly resumes
220220
the wrong player.
221221
- While the native queue slot owns the sink, `current_playback_context` names the
222-
*suspended* context's track; resolve track-level actions through
223-
`queue_now_spotify_track_uri()` / `queue_now_track()`.
222+
*suspended* context's track. Inside `core/app/`, resolve the playing *track*
223+
through `App::playing_item()` (`core/app/playback_routing.rs`): it answers
224+
with the slot's `TrackInfo` (`uri`, `album_id`, `artist_refs`; a slot has
225+
no play context) and refuses a decoded owner. Outside `core/app/` read the
226+
`PlaybackSnapshot` (`infra/media_metadata.rs`), never the field:
227+
`direct_playback_context_reads` counts every other reader and may only fall.
224228
- Radio is in `active_decoded_source` but deliberately out of
225229
`active_queueable_decoded_source` (repeat/shuffle) and
226230
`active_source_position_ms` (seek).

CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@
1010

1111
### Fixed
1212

13+
- **The jump and add-to-playlist keys follow the track that plays**: with a Spotify track playing from the cross-source queue, jump to album (`a`), jump to artist (`A`) and add the playing track to a playlist (`w`/`W`) acted on the track of the *suspended* Spotify context, so they opened the album, the artist and the picker for a song you were not hearing; they now use the queued track itself, and jump to context (`o`) says the queue slot has no play context. Under Local Files, Subsonic, Internet Radio, YouTube or Qobuz those five keys (`a`, `A`, `o`, `w`, `W`) say they need a Spotify track playing instead of acting on the suspended one, and with nothing playing the three jump keys say so instead of doing nothing. Like (`F`) already followed the queued track and keeps doing so; it now shares the same resolution.
14+
1315
- **Spotify's rate limit no longer crashes spotatui at startup**: with a cached login, startup verifies the token with one `/me` request, and that request went straight through rspotify with none of the pacing and `Retry-After` retries every other Spotify call gets. A 429 there ended the process with `Error: http error: status code 429 Too Many Requests` before the UI existed, and relaunching only added more unpaced hits on a rate limit that is shared by everyone using the same client ID. The check now goes through the same paced, retrying request path as the rest of the app, and when Spotify still answers with anything other than a rejected token (a rate limit that outlasts the retries, an outage, no network) spotatui keeps the cached token and starts instead of quitting; the native-streaming account check asks again with its own retries ([#504](https://github.com/LargeModGames/spotatui/issues/504)).
1416

1517
- **Logging in to Spotify no longer fails with "Unable to connect" when the browser was not already running**: both login flows (the startup wizard and `d` then Spotify in the app) opened the browser first and only then started listening for the OAuth redirect on `127.0.0.1:8989`. On Linux desktops where `xdg-open` runs the browser in the foreground (Hyprland and other sessions it does not recognise), a browser that was not already open kept that launch call from returning until the browser was closed, so the listener never started and the redirect landed on a closed port; inside the app, the network layer was stuck for the same time. The listener is now bound before the browser opens, and every browser launch (Spotify login, Qobuz login, the listening recap) is detached so a launcher that waits can never stall spotatui.

CLAUDE.md

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -219,8 +219,12 @@ session exists.
219219
librespot - the native flag stays true, so driving librespot directly resumes
220220
the wrong player.
221221
- While the native queue slot owns the sink, `current_playback_context` names the
222-
*suspended* context's track; resolve track-level actions through
223-
`queue_now_spotify_track_uri()` / `queue_now_track()`.
222+
*suspended* context's track. Inside `core/app/`, resolve the playing *track*
223+
through `App::playing_item()` (`core/app/playback_routing.rs`): it answers
224+
with the slot's `TrackInfo` (`uri`, `album_id`, `artist_refs`; a slot has
225+
no play context) and refuses a decoded owner. Outside `core/app/` read the
226+
`PlaybackSnapshot` (`infra/media_metadata.rs`), never the field:
227+
`direct_playback_context_reads` counts every other reader and may only fall.
224228
- Radio is in `active_decoded_source` but deliberately out of
225229
`active_queueable_decoded_source` (repeat/shuffle) and
226230
`active_source_position_ms` (seek).

src/core/action/mod.rs

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -259,19 +259,20 @@ pub enum Action {
259259
track_id: Option<String>,
260260
track_name: String,
261261
},
262-
/// The picker for the item playing now, resolved at apply time.
262+
/// The picker for the item playing now, resolved through the ownership
263+
/// order at apply time.
263264
OpenAddPlayingTrackDialog,
264265
/// Stage a remove-track-from-playlist confirmation for the track table's
265266
/// current selection; resolved from the selection at apply time.
266267
OpenRemoveTrackDialog,
267-
/// Open the album page of the item that is playing now; resolved from the
268-
/// current playback context at apply time (episodes open their show).
268+
/// Open the album page of the item that is playing now; resolved through
269+
/// the ownership order at apply time (episodes open their show).
269270
JumpToAlbum,
270271
/// Open the album list of the first artist of the track that is playing
271-
/// now; resolved from the current playback context at apply time.
272+
/// now; resolved through the ownership order at apply time.
272273
JumpToArtist,
273274
/// Open the context (album/artist/playlist) that playback runs in;
274-
/// resolved from the current playback context at apply time.
275+
/// resolved through the ownership order at apply time.
275276
JumpToContext,
276277
/// Copy a share URL for the item playing now to the clipboard; a silent
277278
/// no-op without playback or a clipboard.

src/core/action/tests.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -345,12 +345,13 @@ fn jump_to_album_opens_the_current_tracks_album() {
345345
}
346346

347347
#[test]
348-
fn jump_to_album_without_playback_is_a_noop() {
348+
fn jump_to_album_without_playback_reports_nothing_playing() {
349349
let (mut app, rx) = app_with_channel();
350350

351351
app.apply(Action::JumpToAlbum);
352352

353353
assert!(rx.try_recv().is_err(), "expected no IoEvent dispatched");
354+
assert_eq!(app.status_message(), Some(NOTHING_PLAYING_STATUS));
354355
}
355356

356357
#[test]

src/core/app/library.rs

Lines changed: 11 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -490,42 +490,18 @@ impl App {
490490

491491
/// Save or unsave the item playing right now, through the ownership order.
492492
pub fn toggle_save_current_item(&mut self) {
493-
let queue_now_is_spotify = self.queue_now_is_spotify();
494-
let queued_spotify_track_uri = queue_now_is_spotify
495-
.then(|| self.queue_now_spotify_track_uri())
496-
.flatten();
497-
498-
if spotify_context_is_suspended(
499-
self.queue_owns_playback(),
500-
queue_now_is_spotify,
501-
self.active_decoded_source(),
502-
) {
503-
self.set_status_message("The current playback source cannot be liked", 4);
504-
return;
505-
}
506-
507-
// A queued Spotify track plays via a direct `player.load` outside the Spirc
508-
// context, so the cached playback context still names the suspended context's
509-
// track — resolve the queue slot's own track instead of falling through.
510-
if queue_now_is_spotify {
511-
match queued_spotify_track_uri {
512-
Some(uri) => self.dispatch(IoEvent::ToggleSaveTrack(uri)),
513-
None => self.set_status_message("The current playback source cannot be liked", 4),
514-
}
515-
return;
516-
}
517-
518-
let uri = match self
519-
.current_playback_context
520-
.as_ref()
521-
.and_then(|context| context.item.as_ref())
522-
{
523-
Some(PlayableItem::Track(track)) => track.id.as_ref().map(|id| id.uri()),
524-
Some(PlayableItem::Episode(episode)) => Some(episode.id.uri()),
525-
_ => None,
493+
const REFUSED: &str = "The current playback source cannot be liked";
494+
let uri = match self.playing_item() {
495+
PlayingItem::Spotify(PlayableItem::Track(track)) => Ok(track.id.as_ref().map(|id| id.uri())),
496+
PlayingItem::Spotify(PlayableItem::Episode(episode)) => Ok(Some(episode.id.uri())),
497+
PlayingItem::Spotify(_) | PlayingItem::Nothing => Ok(None),
498+
PlayingItem::QueuedSpotify(track) => track.uri.clone().ok_or(REFUSED).map(Some),
499+
PlayingItem::NotSpotify => Err(REFUSED),
526500
};
527-
if let Some(uri) = uri {
528-
self.dispatch(IoEvent::ToggleSaveTrack(uri));
501+
match uri {
502+
Ok(Some(uri)) => self.dispatch(IoEvent::ToggleSaveTrack(uri)),
503+
Ok(None) => {}
504+
Err(status) => self.set_status_message(status, 4),
529505
}
530506
}
531507

@@ -615,18 +591,6 @@ impl App {
615591
}
616592
}
617593

618-
/// Whether Like must not consult the cached Spotify playback context. A queue
619-
/// slot playing a *decoded* item (or any decoded per-source playback) suspends
620-
/// the context; a queue slot playing a *Spotify* track stays eligible — it is
621-
/// liked via the slot's own track, never the cached context.
622-
fn spotify_context_is_suspended(
623-
queue_owns_playback: bool,
624-
queue_now_is_spotify: bool,
625-
decoded_source_active: bool,
626-
) -> bool {
627-
(queue_owns_playback && !queue_now_is_spotify) || decoded_source_active
628-
}
629-
630594
#[cfg(test)]
631595
mod tests {
632596
use super::*;
@@ -699,17 +663,6 @@ mod tests {
699663
);
700664
}
701665

702-
#[test]
703-
fn non_spotify_playback_cannot_use_cached_spotify_item_for_like() {
704-
// A decoded queue slot or any decoded per-source playback suspends Like.
705-
assert!(spotify_context_is_suspended(true, false, false));
706-
assert!(spotify_context_is_suspended(false, false, true));
707-
// A queue slot playing a *Spotify* track stays eligible.
708-
assert!(!spotify_context_is_suspended(true, true, false));
709-
// Plain Spotify context playback.
710-
assert!(!spotify_context_is_suspended(false, false, false));
711-
}
712-
713666
#[cfg(feature = "streaming")]
714667
mod queued_spotify_like {
715668
use super::*;

src/core/app/playback_routing.rs

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,55 @@ pub enum PlaybackOwner {
2222
None,
2323
}
2424

25+
/// The item a track-level action on "what is playing now" can act on.
26+
pub(super) enum PlayingItem<'a> {
27+
/// Spotify owns playback and the cached context names the item. Under
28+
/// native streaming it lags the player after a skip until the next poll.
29+
Spotify(&'a PlayableItem),
30+
/// A Spotify track plays through the native queue slot; the cached context
31+
/// names the suspended context's track, so the slot's own track is the item.
32+
QueuedSpotify(&'a TrackInfo),
33+
/// A decoded source or a decoded queue item owns the sink.
34+
NotSpotify,
35+
/// No owner, or a Spotify owner with no item.
36+
Nothing,
37+
}
38+
39+
/// The pure half of [`App::playing_item`], so every owner is testable without
40+
/// an audio device.
41+
fn resolve_playing_item<'a>(
42+
owner: PlaybackOwner,
43+
slot_track: Option<&'a TrackInfo>,
44+
slot_is_spotify: bool,
45+
cached_item: Option<&'a PlayableItem>,
46+
) -> PlayingItem<'a> {
47+
match owner {
48+
PlaybackOwner::Queue => match slot_track {
49+
Some(track) if slot_is_spotify => PlayingItem::QueuedSpotify(track),
50+
_ => PlayingItem::NotSpotify,
51+
},
52+
PlaybackOwner::Decoded => PlayingItem::NotSpotify,
53+
PlaybackOwner::NativeSpotify | PlaybackOwner::Spotify => {
54+
cached_item.map_or(PlayingItem::Nothing, PlayingItem::Spotify)
55+
}
56+
PlaybackOwner::None => PlayingItem::Nothing,
57+
}
58+
}
59+
2560
impl App {
61+
/// Resolve the item playing now through the ownership order.
62+
pub(super) fn playing_item(&self) -> PlayingItem<'_> {
63+
resolve_playing_item(
64+
self.playback_owner(),
65+
self.queue_now_track(),
66+
self.queue_now_is_spotify(),
67+
self
68+
.current_playback_context
69+
.as_ref()
70+
.and_then(|context| context.item.as_ref()),
71+
)
72+
}
73+
2674
pub(crate) fn playback_owner(&self) -> PlaybackOwner {
2775
if self.queue_owns_playback() {
2876
return PlaybackOwner::Queue;
@@ -396,6 +444,37 @@ mod tests {
396444
assert!(app.active_decoded_player().is_none());
397445
}
398446

447+
#[test]
448+
fn playing_item_follows_the_owner() {
449+
let slot = queue_track(Some("spotify:track:queued"), "Queued");
450+
let cached = PlayableItem::Track(full_track("0000000000000000000001", "Cached"));
451+
452+
assert!(matches!(
453+
resolve_playing_item(PlaybackOwner::Decoded, None, false, Some(&cached)),
454+
PlayingItem::NotSpotify
455+
));
456+
assert!(matches!(
457+
resolve_playing_item(PlaybackOwner::Queue, Some(&slot), false, Some(&cached)),
458+
PlayingItem::NotSpotify
459+
));
460+
assert!(matches!(
461+
resolve_playing_item(PlaybackOwner::Queue, Some(&slot), true, Some(&cached)),
462+
PlayingItem::QueuedSpotify(track) if track.name == "Queued"
463+
));
464+
assert!(matches!(
465+
resolve_playing_item(PlaybackOwner::Spotify, None, false, Some(&cached)),
466+
PlayingItem::Spotify(PlayableItem::Track(track)) if track.name == "Cached"
467+
));
468+
assert!(matches!(
469+
resolve_playing_item(PlaybackOwner::NativeSpotify, None, false, None),
470+
PlayingItem::Nothing
471+
));
472+
assert!(matches!(
473+
resolve_playing_item(PlaybackOwner::None, None, false, Some(&cached)),
474+
PlayingItem::Nothing
475+
));
476+
}
477+
399478
#[test]
400479
fn playback_owner_is_none_without_a_session() {
401480
let (app, _rx) = session_free_app();

src/core/app/playlists.rs

Lines changed: 45 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -239,25 +239,28 @@ impl App {
239239
self.begin_add_track_to_playlist_flow(track_id, track_name);
240240
}
241241

242-
/// Open the add-to-playlist picker for the item playing now. Reads only
243-
/// `current_playback_context`, so a native queue slot stages the suspended
244-
/// context's track.
242+
/// Open the add-to-playlist picker for the item playing now, through the
243+
/// ownership order. A queued Spotify track stages the slot's own track.
245244
pub fn begin_add_playing_track_to_playlist_flow(&mut self) {
246-
match self
247-
.current_playback_context
248-
.as_ref()
249-
.and_then(|context| context.item.as_ref())
250-
{
251-
Some(PlayableItem::Track(track)) => {
245+
match self.playing_item() {
246+
PlayingItem::Spotify(PlayableItem::Track(track)) => {
252247
let track_id = track.id.as_ref().map(|id| id.uri());
253248
let name = track.name.clone();
254249
self.begin_add_track_to_playlist_flow(track_id, name);
255250
}
256-
Some(PlayableItem::Episode(_)) => {
251+
PlayingItem::Spotify(PlayableItem::Episode(_)) => {
257252
self.set_status_message("Only tracks can be added to playlists".to_string(), 4);
258253
}
259-
Some(_) => {}
260-
None => {
254+
PlayingItem::Spotify(_) => {}
255+
PlayingItem::QueuedSpotify(track) => {
256+
let track_id = track.uri.clone();
257+
let name = track.name.clone();
258+
self.begin_add_track_to_playlist_flow(track_id, name);
259+
}
260+
PlayingItem::NotSpotify => {
261+
self.set_status_message("Add to playlist needs a Spotify track playing", 4);
262+
}
263+
PlayingItem::Nothing => {
261264
self.set_status_message("No track currently playing".to_string(), 4);
262265
}
263266
}
@@ -410,6 +413,36 @@ impl App {
410413
mod tests {
411414
use super::*;
412415

416+
#[cfg(feature = "streaming")]
417+
#[test]
418+
fn add_playing_track_stages_the_queued_track_not_the_suspended_one() {
419+
use crate::core::app::test_support::*;
420+
let (tx, rx) = channel();
421+
let mut app = App::new(tx, UserConfig::new(), Some(SystemTime::now()));
422+
app.active_source = Source::YouTube;
423+
app.youtube_playlists = vec![PlaylistInfo {
424+
uri: "youtube:playlist:y1".to_string(),
425+
..playlist_info("y1", "Local List", "owner", false)
426+
}];
427+
app.current_playback_context = Some(playing_track_context(full_track(
428+
"0000000000000000000001",
429+
"Suspended",
430+
)));
431+
app.queue_now = Some(crate::infra::queue::QueueNowPlaying::Spotify {
432+
track: queue_track(Some("spotify:track:queued"), "Queued"),
433+
});
434+
435+
app.begin_add_playing_track_to_playlist_flow();
436+
437+
let pending = app
438+
.pending_playlist_track_add
439+
.as_ref()
440+
.expect("staged for the picker");
441+
assert_eq!(pending.track_id, "spotify:track:queued");
442+
assert_eq!(pending.track_name, "Queued");
443+
assert!(rx.try_recv().is_err());
444+
}
445+
413446
#[test]
414447
fn editable_playlists_include_owned_and_collaborative_only() {
415448
let (tx, _rx) = channel();

src/core/app/queue.rs

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -150,32 +150,6 @@ impl App {
150150
}
151151
}
152152

153-
/// The Spotify URI of the track playing through the queue slot, when the slot
154-
/// holds a native-streamed Spotify track. `None` for a decoded slot, an empty
155-
/// slot, or a slot whose track carries no URI. This is the resolution target
156-
/// for track-level actions (e.g. Like) while the slot owns playback: the
157-
/// cached `current_playback_context` still names the suspended context's
158-
/// track, so it must not be consulted.
159-
pub(crate) fn queue_now_spotify_track_uri(&self) -> Option<String> {
160-
#[cfg(feature = "streaming")]
161-
{
162-
match self.queue_now.as_ref()? {
163-
#[cfg(any(
164-
feature = "local-files",
165-
feature = "subsonic",
166-
feature = "qobuz",
167-
feature = "youtube"
168-
))]
169-
QueueNowPlaying::Decoded(_) => None,
170-
QueueNowPlaying::Spotify { track } => track.uri.clone(),
171-
}
172-
}
173-
#[cfg(not(feature = "streaming"))]
174-
{
175-
None
176-
}
177-
}
178-
179153
/// The queue slot's player when it is playing a *decoded* queued track (local /
180154
/// Subsonic / Qobuz / YouTube). `None` for a Spotify slot or an empty slot. Gated
181155
/// on exactly those four sources: they are the decoded ones a queue item can

0 commit comments

Comments
 (0)