Skip to content

Fix Spotify Connect: playlist advance, seek support, log noise #210

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hansherlighed wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix Spotify Connect: playlist advance, seek support, log noise #210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
149 changes: 149 additions & 0 deletions API.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,11 @@ sub player {
$result->{track} = $libraryCache->normalize($result->{item});
}

# keep track of MAC -> ID mapping
if ( Plugins::Spotty::Plugin->canSpotifyConnect() ) {
Plugins::Spotty::Connect::DaemonManager->checkAPIConnectPlayer($self, $result);
}

$cb->($result);
return;
}
Expand All @@ -221,6 +226,143 @@ sub player {
)
}

sub playerTransfer {
my ( $self, $cb, $device ) = @_;

$self->withIdFromMac(sub {
my $deviceId = shift;

if (!$deviceId) {
$cb->() if $cb;
return;
}

$self->_call('me/player',
sub {
$cb->() if $cb;
},
PUT => {
body => encode_json({
device_ids => [ $deviceId ],
play => 1
})
}
);
}, $device);
}

sub playerPause {
my ( $self, $cb, $device ) = @_;

$self->withIdFromMac(sub {
my $args = {};
$args->{device_id} = $_[0] if $_[0];

$self->_call('me/player/pause',
sub {
$cb->() if $cb;
},
PUT => $args
);
}, $device);
Comment on lines +254 to +267
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

For the Spotify Web API endpoints /me/player/pause, /me/player/next, /me/player/seek, and /me/player/volume, what device is targeted when device_id is omitted?

💡 Result:

When device_id is omitted, the command targets the user’s currently active Spotify Connect playback device (the device with is_active: true). This is explicitly stated in the docs for Pause, Next, Seek, and Set Volume. [1][2][3][4]

Sources:
[1] Pause Playback (/me/player/pause) (developer.spotify.com)
[2] Skip To Next (/me/player/next) (developer.spotify.com)
[3] Seek To Position (/me/player/seek) (developer.spotify.com)
[4] Set Playback Volume (/me/player/volume) (developer.spotify.com)

Citations:

🏁 Script executed:

#!/bin/bash
# Search for the playerTransfer method to see its guard implementation
rg -A 15 'sub playerTransfer' API.pm

Repository: michaelherger/Spotty-Plugin

Length of output: 309

🏁 Script executed:

#!/bin/bash
# Search for withIdFromMac to understand its behavior on failure
rg -B 2 -A 10 'sub withIdFromMac' API.pm

Repository: michaelherger/Spotty-Plugin

Length of output: 334

🏁 Script executed:

#!/bin/bash
# Search for the four methods to confirm they lack the guard
rg -A 12 'sub playerPause|sub playerNext|sub playerSeek|sub playerVolume' API.pm

Repository: michaelherger/Spotty-Plugin

Length of output: 1040

Abort targeted playback commands when MAC lookup fails.

playerTransfer() already short-circuits on an unresolved LMS MAC. These four methods don't, so a failed/stale MAC→Spotify ID lookup sends the request without device_id and targets whichever Spotify device is currently active instead of the intended player. Mirror playerTransfer()'s guard pattern:

if (!$deviceId) {
    $cb->() if $cb;
    return;
}

Also applies to: 270-283, 286-303, 305-322

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@API.pm` around lines 254 - 267, The playerPause handler (and the other
playback-targeted methods in the same area such as playerPlay, playerNext,
playerPrevious) must abort when the MAC→Spotify device lookup fails; modify the
anonymous callback passed to withIdFromMac in playerPause (and the equivalent
callbacks in playerPlay/playerNext/playerPrevious) to check the resolved device
id (e.g., $_[0] or $deviceId) and if it's falsey call $cb->() if $cb and then
return immediately, mirroring playerTransfer()'s guard so no request is sent
without a valid device_id.

}

sub playerNext {
my ( $self, $cb, $device ) = @_;

$self->withIdFromMac(sub {
my $args = {};
$args->{device_id} = $_[0] if $_[0];

$self->_call('me/player/next',
sub {
$cb->() if $cb;
},
POST => $args
);
}, $device);
}

sub playerSeek {
my ( $self, $cb, $device, $songtime ) = @_;

$self->withIdFromMac(sub {
my $args = {
position_ms => int($songtime * 1000),
};

$args->{device_id} = $_[0] if $_[0];

$self->_call('me/player/seek',
sub {
$cb->() if $cb;
},
PUT => $args
);
}, $device);
}

sub playerVolume {
my ( $self, $cb, $device, $volume ) = @_;

$self->withIdFromMac(sub {
my $args = {
volume_percent => int($volume),
};

$args->{device_id} = $_[0] if $_[0];

$self->_call('me/player/volume',
sub {
$cb->() if $cb;
},
PUT => $args
);
}, $device);
}

sub idFromMac {
my ( $class, $mac ) = @_;

return Plugins::Spotty::Plugin->canSpotifyConnect()
&& Plugins::Spotty::Connect::DaemonManager->idFromMac($mac);
}

sub withIdFromMac {
my ( $self, $cb, $mac ) = @_;

my $id = $self->idFromMac($mac);

if ( $id || $mac !~ /((?:[a-f0-9]{2}:){5}[a-f0-9]{2})/i ) {
$cb->($id || $mac);
}
else {
# ID wasn't in the cache yet, let's get the playerlist
$self->devices(sub {
$cb->($self->idFromMac($mac));
});
}
}

sub devices {
my ( $self, $cb ) = @_;

$self->_call('me/player/devices',
sub {
my ($result) = @_;

if ( Plugins::Spotty::Plugin->canSpotifyConnect() ) {
Plugins::Spotty::Connect::DaemonManager->checkAPIConnectPlayers($self, $result);
}

$cb->() if $cb;
},
GET => {
_nocache => 1,
}
);
}

sub search {
my ( $self, $cb, $args ) = @_;

Expand Down Expand Up @@ -1463,6 +1605,13 @@ sub canPodcast {
$self->_canPodcast(Plugins::Spotty::Helper->getCapability('podcasts') || 0);
}

sub doesAutoplay {
my $self = $_[0];

return unless $prefs->client($self->client)->get('enableAutoplay');
return Plugins::Spotty::Helper->getCapability('autoplay');
}

sub _DEFAULT_LIMIT {
Plugins::Spotty::Plugin->hasDefaultIcon() ? DEFAULT_LIMIT : MAX_LIMIT;
};
Expand Down
3 changes: 1 addition & 2 deletions AccountHelper.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,13 +132,12 @@ sub renameCacheFolder {
$newId = substr( md5_hex(Slim::Utils::Unicode::utf8toLatin1Transliterate($credentials->{username} || '')), 0, 8 );
}

main::INFOLOG && $log->info("Trying to rename $oldId to $newId");

if (main::DEBUGLOG && $log->is_debug && !$newId) {
Slim::Utils::Log::logBacktrace("No newId found in '$oldId'");
}

if ($oldId && $newId) {
main::INFOLOG && $log->info("Trying to rename $oldId to $newId");
my $from = $class->cacheFolder($oldId);

if (!-e $from) {
Expand Down
Loading