feat(connect): full Spotify Connect event dispatch, API control methods, and settings UI

[stiefenm]

Summary

Complete the Spotify Connect stack: implement Connect.pm with full player-event
dispatch (track changes, play/pause, volume), add Connect-specific API methods to
API.pm, and extend the Settings and ProtocolHandler to support Connect flows.

Background

This PR is the final piece of the three-part Connect re-implementation (#220 → #221 → this PR).
With #220 establishing the feature flag and #221 providing daemon lifecycle, this PR adds
the actual event dispatch: Connect.pm receives JSON-RPC callbacks from the spotty
binary (via --lms mode) and forwards them to LMS as player commands.

The API methods added here (playerNext, playerPrev, playerPlay, playerPause,
playerVolume, transferPlayback) are called by Connect.pm to control LMS playback
in response to Spotify Connect commands from remote clients.

Note on PR #210: This PR overlaps with PR #210 (by hansherlighed) on most Connect
files. Both implement Spotify Connect event dispatch but take different architectural
approaches. We leave the decision to the maintainer; the two PRs are not merge-compatible.

Changes

Connect.pm

• Full spottyconnect JSON-RPC handler: parse events (TrackChanged, PlaybackStart,
  PlaybackStop, VolumeChanged) and dispatch to LMS player
• Start/stop Connect bridge on LMS startup/shutdown hooks

API.pm

• Add Connect control methods: playerNext, playerPrev, playerPlay, playerPause,
  playerVolume, transferPlayback
• Add getConnectDevices for listing active Connect endpoints

Connect/*.pm (Context, Daemon, DaemonManager)

• Refinements and event-handling wiring on top of feat(connect): Connect daemon lifecycle — Context, Daemon, DaemonManager modules #221 base

ProtocolHandler.pm

• Handle spotify-connect:// URI scheme for Connect-initiated playback

Plugin.pm / Settings.pm / Settings/Player.pm

• Connect status indicators; "Transfer to this player" button in player settings

HTML / strings.txt

• UI strings and templates for Connect settings and status

Dependency Notes

Merge after #220 and #221. The API methods
added here are called by Connect.pm; the Connect infrastructure from #220/#221 must be
present. Semantically also depends on #218 for the flavor-aware
_call that the new API methods use.

Full merge order recommendation: #216 → #217 → #218 → #219 → #215 → #220 → #221 → #222.

Submitted as draft pending A-series merge.

Tested On

• Lyrion Music Server 9.2.0 (build 1778040950) on Debian Linux x86_64
• Perl 5.38.2 (x86_64-linux-gnu-thread-multi)
• spotty binary v2.1.0 / librespot 0.8.0
• Verified: Spotify app on phone sees LMS as a Connect endpoint; track change from
  Spotify app updates LMS playback; Play/Pause from Spotify app reflected in LMS;
  multiple players (including sync-groups) handled correctly

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR introduces complete Spotify Connect support to the Spotty plugin. It adds a daemon-managed helper process per player, wires event dispatch between Spotify and LMS, implements bidirectional playback synchronization, and extends the Spotify API with device-aware player controls. The feature is gated by plugin version checks and preference flags, with per-player and global configuration options.

Changes

Spotify Connect Feature Implementation

Layer / File(s)	Summary
Plugin initialization and settings configuration Plugin.pm, Settings.pm, Settings/Player.pm, HTML/EN/plugins/Spotty/settings/*.html, strings.txt	Plugin introduces CONNECT_HELPER_VERSION constant and canSpotifyConnect() capability check. Settings expose global disable and per-player enable toggles, with version-gating and helper-update messages. Localization strings added for Spotify Connect UI.
Helper daemon lifecycle management Connect/Daemon.pm, Connect/DaemonManager.pm, Plugin.pm	Daemon manages individual helper lifecycle: command construction, credentials injection, process launch, and crash-backoff with discovery disable. DaemonManager orchestrates multi-daemon setup per sync group, debounced initialization, periodic watchdog, device mapping reconciliation, and pid exclusion during process cleanup.
Spotify Connect event routing and state management Connect.pm, ProtocolHandler.pm	Connect.init wires Spotty JSON-RPC command dispatch and LMS event subscriptions. _connectEvent is the primary dispatcher handling start, change, stop, volume, seek with device matching, stale-URI reconciliation, and Connect-mode initialization. Protocol handler adds action blocking (stop while connected), repeating-stream detection, and next-track delegation.
Spotify API device control methods API.pm, ProtocolHandler.pm	playerTransfer, playerPause, playerNext, playerPrevious, playerSeek, playerVolume, and playerQueue now accept optional MAC parameters for device targeting. idFromMac and withIdFromMac resolve MACs to Spotify device IDs via daemon spotifyId cache or fallback device list. devices endpoint reconciles device mappings with daemon manager. Metadata lookup pulls stream URLs from playingSong for Connect mode.
Context tracking and API handler provisioning Connect/Context.pm, Connect.pm	Context tracks known tracks per playlist/album, maintains play history to detect last-track conditions, and stops playback when autoplay is disabled. MemoryCache provides LRU-based caching. getAPIHandler provisions per-player Spotty API instances with credentials copied into per-player cache folder.
LMS event forwarding and playback synchronization Connect.pm	Event subscribers (_onNewSong, _onPause, _onVolume, _onSeek, _onPlaylistJump) forward LMS changes to Spotify with debouncing and feedback-loop guards. _syncPoll implements periodic drift correction by comparing Spotify URI and progress against LMS state; on mismatch it resets context or seeks. _getNextTrack orchestrates track advancement by peeking Spotify queue, streaming upcoming URI early, and scheduling player-next at track-end.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 A Spotify dream takes flight with Connect,
Daemons hum in sync groups, device checks perfect.
Events bounce 'tween LMS and Spotty streams,
Play history tracked through playlist extremes!
No more missed tracks—just rabbits and beams. 🎵

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main changes: adding Spotify Connect event dispatch via JSON-RPC handler, new API control methods for player operations, and associated settings UI components.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 6

🧹 Nitpick comments (5)

Connect/Daemon.pm (1)

36-48: ⚡ Quick win

Parameter mutation in constructor may cause unexpected behavior.

Line 42 modifies the $id parameter with s/://g, which mutates the caller's variable if they passed a scalar variable. While this may work in the current usage, it's an unexpected side effect.

♻️ Proposed fix to avoid parameter mutation

 sub new {
 	my ($class, $id) = `@_`;
 
 	my $self = $class->SUPER::new();
 
 	$self->mac($id);
-	$id =~ s/://g;
-	$self->id($id);
+	(my $normalizedId = $id) =~ s/://g;
+	$self->id($normalizedId);
 	$self->_startTimes([]);
 	$self->start();
 
 	return $self;
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Connect/Daemon.pm` around lines 36 - 48, The constructor sub new mutates the
incoming $id parameter with $id =~ s/://g, which can unexpectedly modify the
caller's variable; instead make a local copy (e.g., my $clean_id = $id) and
perform the s/://g on that copy, then call $self->id($clean_id) while leaving
the original $id untouched; keep the calls to $self->mac($id),
$self->_startTimes([]) and $self->start() as-is.

Plugin.pm (1)

334-351: ⚡ Quick win

Add error handling around require and init() calls.

The require statement at line 346 could fail if the module is missing or has syntax errors, and Connect->init() at line 348 is called without checking for errors. While these are unlikely in production, defensive error handling would improve robustness.

🛡️ Proposed defensive wrapper

 	require Plugins::Spotty::Connect;
 
-	Plugins::Spotty::Connect->init() unless $dontInit;
+	eval {
+		Plugins::Spotty::Connect->init() unless $dontInit;
+	};
+	
+	if ($@) {
+		$log->error("Failed to initialize Spotify Connect: $@");
+		return;
+	}
 
 	return 1;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Plugin.pm` around lines 334 - 351, The canSpotifyConnect function currently
calls require Plugins::Spotty::Connect and Plugins::Spotty::Connect->init()
without guarding against load or init failures; wrap the require in an eval {
require Plugins::Spotty::Connect; 1 } and check the result, logging any $@ via
$log->error and returning undef if the require failed, and only call
Plugins::Spotty::Connect->init() inside a separate eval so you can catch and log
exceptions from init (again using $log->error and returning undef on failure) —
keep the existing dontInit short-circuit behavior so init() is skipped when
requested.

Connect/Context.pm (1)

198-224: 💤 Low value

MemoryCache uses shared global hash across all instances.

The %memCache tied hash at line 205 is declared at package scope, meaning all MemoryCache instances share the same underlying storage. While this may be intentional for memory efficiency, it means keys from different Context instances can collide. Since Context prefixes keys with $self->_id (line 187), this is mitigated, but the shared state is not obvious from the API and could cause subtle bugs if key prefixes are not unique.

Consider either documenting this behavior or making the cache instance-specific by storing the tied hash reference in the blessed object.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Connect/Context.pm` around lines 198 - 224, The package-level tied hash
%memCache in Plugins::Spotty::Connect::MemoryCache creates shared storage across
all instances; change MemoryCache to be instance-specific by moving the tie out
of package scope into the constructor (new) and store the tied hash reference in
the blessed object (e.g. $self->{_cache}), then update get, set and remove to
operate on that instance cache (use $self->{_cache}{$_[1]} etc.) instead of the
global %memCache; alternatively, if shared global behavior is intended, add a
clear comment in the package header documenting that %memCache is shared and
that callers must namespace keys (Context uses $self->_id).

Connect.pm (2)

340-377: ⚖️ Poor tradeoff

Queue peek mutates song streamUrl before track transition completes.

Line 354 directly assigns the next track URI to $song->streamUrl($uri) while the current track is still playing. This premature mutation could cause metadata inconsistencies if the user seeks, pauses, or if playback is interrupted before the track naturally ends.

Consider storing the peeked URI in pluginData instead of mutating streamUrl, then applying it during the actual track transition in the newTrack flow.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Connect.pm` around lines 340 - 377, The code mutates $song->streamUrl($uri)
inside the $spotty->playerQueue callback while the current track is still
playing; instead, store the peeked URI in the client's/plugin's pluginData (e.g.
set pluginData newTrack or another key on $client or $song) and remove the
immediate $song->streamUrl call; then update the actual $song->streamUrl only
when the transition occurs (in the _firePlayerNext/newTrack flow that currently
consumes pluginData 'newTrack'), ensuring _delayedStop and the existing hasPlay
checks still reference the stored pluginData and that timers set with
Slim::Utils::Timers::setTimer remain unchanged.

---

891-932: 💤 Low value

Indentation issue and potentially redundant pause logic in stop event handler.

Lines 895-932 have an indentation problem that makes the control flow difficult to read. Line 895 starts an if block that should be indented further relative to line 891.

Additionally, both the device-match branch (895-905) and the device-mismatch branch (906-923) issue a pause command when the client is playing. The second branch (lines 906-923) clears Connect state, suggesting playback was transferred elsewhere, but the first branch (895-905) does not. This asymmetry may be intentional (pause without clearing state keeps Connect mode active), but the logic would benefit from a clarifying comment.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Connect.pm` around lines 891 - 932, The nested if starting at the device
check (around Plugins::Spotty::Connect::DaemonManager->idFromMac usage) is
mis-indented — re-indent the block so the inner if/elsif/elsif sit clearly
inside the outer "if ($result->{device})" scope; also remove ambiguity about the
duplicated pause logic by adding a short clarifying comment in the device-match
branch (the branch that calls Slim::Control::Request->new(... ['pause',1]) when
__PACKAGE__->isSpotifyConnect($client) and the device id/name matches)
indicating that we intentionally only pause playback while leaving Connect state
intact, whereas the device-mismatch branch intentionally pauses and then clears
Connect state (clearing via $client->playingSong()->pluginData(context=>0),
$client->pluginData(SpotifyConnect=>0), and
Slim::Utils::Timers::killTimers($client,\&_syncPoll)); keep the existing
behavior but make indentation and intent explicit so reviewers understand the
asymmetry.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@Connect.pm`:
- Around line 279-291: The shutdown function fails to unsubscribe the
playlist-jump handler, leaving _onPlaylistJump active after cleanup; update the
shutdown routine (in Connect::shutdown) to call
Slim::Control::Request::unsubscribe(\&_onPlaylistJump) alongside the existing
unsubscribes so the handler subscribed earlier
(Slim::Control::Request::subscribe(\&_onPlaylistJump)) is properly removed
before clearing $initialized.
- Around line 514-523: The code calls $client->playingSong()->pluginData(context
=> 0) without ensuring playingSong() is defined; guard that call by fetching the
playing song into a variable (e.g. $song = $client->playingSong()) and only call
$song->pluginData(...) if $song is defined to avoid a null-pointer crash, while
leaving the subsequent $client->pluginData(SpotifyConnect => 0),
Slim::Utils::Timers::killTimers(...) calls and
__PACKAGE__->getAPIHandler($client)->playerPause(...) unchanged.
- Around line 266-272: The File::Copy::copy call that copies credentials.json
into the per-player cache (using File::Copy::copy with catfile($cacheFolder,
'credentials.json') => catfile($playerCacheFolder, 'credentials.json'))
currently ignores failures; update the code around that call to check its
boolean return value and handle errors (e.g., log via the existing logger,
croak/die, or return an error) when the copy fails, including the
source/destination and $! in the message so permission/disk errors are visible
and the caller can fail fast instead of proceeding without credentials.
- Around line 232-234: The eval block that reads credentials.json (creating
$credentials from from_json(...) using $credentialsFile) opens a lexical
filehandle $fh but relies on scope to close it, which can leak inside eval;
change the read to explicitly open, slurp into a scalar, close $fh, then call
from_json on that scalar (or use a safe slurp helper like
File::Slurp/Path::Tiny) and keep the surrounding eval for error capture so $fh
is always closed even on error; update the code around the $credentials = eval {
... } expression to perform explicit close of $fh after reading.
- Around line 81-98: The subscriber in Slim::Control::Request::subscribe creates
a 5s timer with Slim::Utils::Timers::setTimer for each client but never cancels
previous timers; fix by tracking the timer id on the $client (e.g.,
$client->{prebuffer_timer}), and before calling setTimer check for and cancel
any existing timer via Slim::Utils::Timers::killTimer($client,
$client->{prebuffer_timer}) (or delete the field) and then store the new timer
id into $client->{prebuffer_timer} so rapid client/new events don't accumulate
timers.

In `@Connect/Daemon.pm`:
- Around line 92-100: The encode_base64 call used when pushing the '--lms-auth'
arg is producing a trailing newline; update the encode_base64 invocation in the
block that builds `@helperArgs` (the line that encodes sprintf("%s:%s",
$serverPrefs->get('username'), $serverPrefs->get('password'))) to pass an empty
string as the second argument so the returned Base64 string has no newline,
ensuring the '--lms-auth' command-line value is not terminated with a newline.

---

Nitpick comments:
In `@Connect.pm`:
- Around line 340-377: The code mutates $song->streamUrl($uri) inside the
$spotty->playerQueue callback while the current track is still playing; instead,
store the peeked URI in the client's/plugin's pluginData (e.g. set pluginData
newTrack or another key on $client or $song) and remove the immediate
$song->streamUrl call; then update the actual $song->streamUrl only when the
transition occurs (in the _firePlayerNext/newTrack flow that currently consumes
pluginData 'newTrack'), ensuring _delayedStop and the existing hasPlay checks
still reference the stored pluginData and that timers set with
Slim::Utils::Timers::setTimer remain unchanged.
- Around line 891-932: The nested if starting at the device check (around
Plugins::Spotty::Connect::DaemonManager->idFromMac usage) is mis-indented —
re-indent the block so the inner if/elsif/elsif sit clearly inside the outer "if
($result->{device})" scope; also remove ambiguity about the duplicated pause
logic by adding a short clarifying comment in the device-match branch (the
branch that calls Slim::Control::Request->new(... ['pause',1]) when
__PACKAGE__->isSpotifyConnect($client) and the device id/name matches)
indicating that we intentionally only pause playback while leaving Connect state
intact, whereas the device-mismatch branch intentionally pauses and then clears
Connect state (clearing via $client->playingSong()->pluginData(context=>0),
$client->pluginData(SpotifyConnect=>0), and
Slim::Utils::Timers::killTimers($client,\&_syncPoll)); keep the existing
behavior but make indentation and intent explicit so reviewers understand the
asymmetry.

In `@Connect/Context.pm`:
- Around line 198-224: The package-level tied hash %memCache in
Plugins::Spotty::Connect::MemoryCache creates shared storage across all
instances; change MemoryCache to be instance-specific by moving the tie out of
package scope into the constructor (new) and store the tied hash reference in
the blessed object (e.g. $self->{_cache}), then update get, set and remove to
operate on that instance cache (use $self->{_cache}{$_[1]} etc.) instead of the
global %memCache; alternatively, if shared global behavior is intended, add a
clear comment in the package header documenting that %memCache is shared and
that callers must namespace keys (Context uses $self->_id).

In `@Connect/Daemon.pm`:
- Around line 36-48: The constructor sub new mutates the incoming $id parameter
with $id =~ s/://g, which can unexpectedly modify the caller's variable; instead
make a local copy (e.g., my $clean_id = $id) and perform the s/://g on that
copy, then call $self->id($clean_id) while leaving the original $id untouched;
keep the calls to $self->mac($id), $self->_startTimes([]) and $self->start()
as-is.

In `@Plugin.pm`:
- Around line 334-351: The canSpotifyConnect function currently calls require
Plugins::Spotty::Connect and Plugins::Spotty::Connect->init() without guarding
against load or init failures; wrap the require in an eval { require
Plugins::Spotty::Connect; 1 } and check the result, logging any $@ via
$log->error and returning undef if the require failed, and only call
Plugins::Spotty::Connect->init() inside a separate eval so you can catch and log
exceptions from init (again using $log->error and returning undef on failure) —
keep the existing dontInit short-circuit behavior so init() is skipped when
requested.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3718bdd5-e94c-4925-bb5c-49022d93ae26

📥 Commits

Reviewing files that changed from the base of the PR and between c1d9dfa and 9637ed1.

📒 Files selected for processing (12)

• API.pm
• Connect.pm
• Connect/Context.pm
• Connect/Daemon.pm
• Connect/DaemonManager.pm
• HTML/EN/plugins/Spotty/settings/basic.html
• HTML/EN/plugins/Spotty/settings/player.html
• Plugin.pm
• ProtocolHandler.pm
• Settings.pm
• Settings/Player.pm
• strings.txt

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Buffer optimization timer may leak on rapid client/new events.

The client/new subscriber sets a 5-second timer without first killing any existing timer for that client. If multiple new events fire in quick succession (e.g., during sync-group reconfiguration), timers could accumulate.

🛡️ Proposed fix

 	Slim::Control::Request::subscribe(sub {
 		my $request = shift;
 		my $client  = $request->client();
 
 		if (!$prefs->get('optimizePreBuffer')) {
+			Slim::Utils::Timers::killTimers($client, sub { shift; });  # kill existing optimization check
 			# Wait a few seconds before the buffer size is known
 			Slim::Utils::Timers::setTimer($client, time() + 5, sub {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Connect.pm` around lines 81 - 98, The subscriber in
Slim::Control::Request::subscribe creates a 5s timer with
Slim::Utils::Timers::setTimer for each client but never cancels previous timers;
fix by tracking the timer id on the $client (e.g., $client->{prebuffer_timer}),
and before calling setTimer check for and cancel any existing timer via
Slim::Utils::Timers::killTimer($client, $client->{prebuffer_timer}) (or delete
the field) and then store the new timer id into $client->{prebuffer_timer} so
rapid client/new events don't accumulate timers.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

File handle leak in credentials.json read.

The inline file read at line 233 opens a filehandle but never explicitly closes it. The do { local $/; open(my $fh, '<', $credentialsFile) or die $!; <$fh> } pattern relies on lexical scope cleanup, but in an eval this is fragile. If the eval succeeds, the filehandle $fh goes out of scope and should close, but explicitly closing after reading is more robust and prevents potential leak if Perl's reference counting is delayed.

🛡️ Proposed fix

 	my $credentials = eval {
-		from_json(do { local $/; open(my $fh, '<', $credentialsFile) or die $!; <$fh> });
+		my $content = do {
+			open(my $fh, '<', $credentialsFile) or die $!;
+			local $/;
+			my $data = <$fh>;
+			close $fh;
+			$data;
+		};
+		from_json($content);
 	};

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Connect.pm` around lines 232 - 234, The eval block that reads
credentials.json (creating $credentials from from_json(...) using
$credentialsFile) opens a lexical filehandle $fh but relies on scope to close
it, which can leak inside eval; change the read to explicitly open, slurp into a
scalar, close $fh, then call from_json on that scalar (or use a safe slurp
helper like File::Slurp/Path::Tiny) and keep the surrounding eval for error
capture so $fh is always closed even on error; update the code around the
$credentials = eval { ... } expression to perform explicit close of $fh after
reading.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

No error handling for credentials.json copy operation.

The File::Copy::copy call at lines 268-270 does not check for errors. If the copy fails (e.g., due to permission issues or disk full), the per-player cache folder will lack credentials, causing subsequent daemon startup or API calls to fail silently or with confusing errors.

🛡️ Proposed fix

 		if (!-e catfile($playerCacheFolder, 'credentials.json')) {
 			require File::Copy;
-			File::Copy::copy(
+			File::Copy::copy(
 				catfile($cacheFolder, 'credentials.json'),
 				catfile($playerCacheFolder, 'credentials.json')
-			);
+			) or $log->error("Failed to copy credentials.json: $!");
 		}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if (!-e catfile($playerCacheFolder, 'credentials.json')) {
	require File::Copy;
	File::Copy::copy(
	catfile($cacheFolder, 'credentials.json'),
	catfile($playerCacheFolder, 'credentials.json')
	);
	}
	if (!-e catfile($playerCacheFolder, 'credentials.json')) {
	require File::Copy;
	File::Copy::copy(
	catfile($cacheFolder, 'credentials.json'),
	catfile($playerCacheFolder, 'credentials.json')
	) or $log->error("Failed to copy credentials.json: $!");
	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Connect.pm` around lines 266 - 272, The File::Copy::copy call that copies
credentials.json into the per-player cache (using File::Copy::copy with
catfile($cacheFolder, 'credentials.json') => catfile($playerCacheFolder,
'credentials.json')) currently ignores failures; update the code around that
call to check its boolean return value and handle errors (e.g., log via the
existing logger, croak/die, or return an error) when the copy fails, including
the source/destination and $! in the message so permission/disk errors are
visible and the caller can fail fast instead of proceeding without credentials.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Missing unsubscribe for _onPlaylistJump in shutdown.

The shutdown method unsubscribes _onNewSong, _onPause, _onVolume, and _onSeek, but does not unsubscribe _onPlaylistJump (subscribed at line 78). This will leave the playlist jump handler active after shutdown, potentially causing callbacks to fire into a partially cleaned-up state.

🛡️ Proposed fix

 		Slim::Control::Request::unsubscribe(\&_onNewSong);
 		Slim::Control::Request::unsubscribe(\&_onPause);
 		Slim::Control::Request::unsubscribe(\&_onVolume);
 		Slim::Control::Request::unsubscribe(\&_onSeek);
+		Slim::Control::Request::unsubscribe(\&_onPlaylistJump);
 
 		$initialized = 0;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	sub shutdown {
	if ($initialized) {
	require Plugins::Spotty::Connect::DaemonManager;
	Plugins::Spotty::Connect::DaemonManager->shutdown();
	Slim::Control::Request::unsubscribe(\&_onNewSong);
	Slim::Control::Request::unsubscribe(\&_onPause);
	Slim::Control::Request::unsubscribe(\&_onVolume);
	Slim::Control::Request::unsubscribe(\&_onSeek);
	$initialized = 0;
	}
	}
	sub shutdown {
	if ($initialized) {
	require Plugins::Spotty::Connect::DaemonManager;
	Plugins::Spotty::Connect::DaemonManager->shutdown();
	Slim::Control::Request::unsubscribe(\&_onNewSong);
	Slim::Control::Request::unsubscribe(\&_onPause);
	Slim::Control::Request::unsubscribe(\&_onVolume);
	Slim::Control::Request::unsubscribe(\&_onSeek);
	Slim::Control::Request::unsubscribe(\&_onPlaylistJump);
	$initialized = 0;
	}
	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Connect.pm` around lines 279 - 291, The shutdown function fails to
unsubscribe the playlist-jump handler, leaving _onPlaylistJump active after
cleanup; update the shutdown routine (in Connect::shutdown) to call
Slim::Control::Request::unsubscribe(\&_onPlaylistJump) alongside the existing
unsubscribes so the handler subscribed earlier
(Slim::Control::Request::subscribe(\&_onPlaylistJump)) is properly removed
before clearing $initialized.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Potential null-pointer on playingSong() when exiting Connect mode.

Line 517 calls $client->playingSong()->pluginData(context => 0) without checking if playingSong() returns a defined song. If the newsong event fires during a track transition (between tracks), playingSong() may return undef, causing a runtime error.

🛡️ Proposed fix

 	main::INFOLOG && $log->is_info && $log->info(
 		"Got a new track event, but this is no longer Spotify Connect"
 	);
-	$client->playingSong()->pluginData(context => 0);
+	my $song = $client->playingSong();
+	$song->pluginData(context => 0) if $song;
 	$client->pluginData(SpotifyConnect => 0);

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	main::INFOLOG && $log->is_info && $log->info(
	"Got a new track event, but this is no longer Spotify Connect"
	);
	$client->playingSong()->pluginData(context => 0);
	$client->pluginData(SpotifyConnect => 0);
	Slim::Utils::Timers::killTimers($client, \&_syncController);
	Slim::Utils::Timers::killTimers($client, \&_getNextTrack);
	Slim::Utils::Timers::killTimers($client, \&_syncPoll);
	__PACKAGE__->getAPIHandler($client)->playerPause(undef, $client->id);
	main::INFOLOG && $log->is_info && $log->info(
	"Got a new track event, but this is no longer Spotify Connect"
	);
	my $song = $client->playingSong();
	$song->pluginData(context => 0) if $song;
	$client->pluginData(SpotifyConnect => 0);
	Slim::Utils::Timers::killTimers($client, \&_syncController);
	Slim::Utils::Timers::killTimers($client, \&_getNextTrack);
	Slim::Utils::Timers::killTimers($client, \&_syncPoll);
	__PACKAGE__->getAPIHandler($client)->playerPause(undef, $client->id);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Connect.pm` around lines 514 - 523, The code calls
$client->playingSong()->pluginData(context => 0) without ensuring playingSong()
is defined; guard that call by fetching the playing song into a variable (e.g.
$song = $client->playingSong()) and only call $song->pluginData(...) if $song is
defined to avoid a null-pointer crash, while leaving the subsequent
$client->pluginData(SpotifyConnect => 0), Slim::Utils::Timers::killTimers(...)
calls and __PACKAGE__->getAPIHandler($client)->playerPause(...) unchanged.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

encode_base64 includes trailing newline by default.

Line 95 uses encode_base64 for Basic authentication, but by default this function appends a newline character. Command-line arguments should not contain newlines. Pass an empty string as the second argument to suppress it.

🐛 Proposed fix

 		if ( Plugins::Spotty::Helper->getCapability('lms-auth') ) {
 			main::INFOLOG && $log->is_info && $log->info("Adding authentication data to Spotty Connect daemon configuration.");
-			push `@helperArgs`, '--lms-auth', encode_base64(sprintf("%s:%s", $serverPrefs->get('username'), $serverPrefs->get('password')));
+			push `@helperArgs`, '--lms-auth', encode_base64(sprintf("%s:%s", $serverPrefs->get('username'), $serverPrefs->get('password')), '');
 		}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if ( $serverPrefs->get('authorize') ) {
	if ( Plugins::Spotty::Helper->getCapability('lms-auth') ) {
	main::INFOLOG && $log->is_info && $log->info("Adding authentication data to Spotty Connect daemon configuration.");
	push @helperArgs, '--lms-auth', encode_base64(sprintf("%s:%s", $serverPrefs->get('username'), $serverPrefs->get('password')));
	}
	else {
	$log->error("Your Lyrion Music Server is password protected, but your spotty helper can't deal with it! Spotty will NOT work. Please update.");
	}
	}
	if ( $serverPrefs->get('authorize') ) {
	if ( Plugins::Spotty::Helper->getCapability('lms-auth') ) {
	main::INFOLOG && $log->is_info && $log->info("Adding authentication data to Spotty Connect daemon configuration.");
	push `@helperArgs`, '--lms-auth', encode_base64(sprintf("%s:%s", $serverPrefs->get('username'), $serverPrefs->get('password')), '');
	}
	else {
	$log->error("Your Lyrion Music Server is password protected, but your spotty helper can't deal with it! Spotty will NOT work. Please update.");
	}
	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Connect/Daemon.pm` around lines 92 - 100, The encode_base64 call used when
pushing the '--lms-auth' arg is producing a trailing newline; update the
encode_base64 invocation in the block that builds `@helperArgs` (the line that
encodes sprintf("%s:%s", $serverPrefs->get('username'),
$serverPrefs->get('password'))) to pass an empty string as the second argument
so the returned Base64 string has no newline, ensuring the '--lms-auth'
command-line value is not terminated with a newline.

[stiefenm]

Closing in favor of consolidated PRs as discussed — will resubmit as focused issues + PRs.