feat(playback): enhance playback functionality and service management#74
feat(playback): enhance playback functionality and service management#74Skeptic-systems merged 1 commit intomainfrom
Conversation
- Updated useCurrentlyPlaying hook to start and stop autoplay and keep-alive services for improved playback management. - Added playPlaylistFromIndex method to MusicProvider interface for better playlist control. - Implemented track playback from a specified index in both Spotify and YouTube providers. - Enhanced PlaylistView to handle track selection and playback more effectively, including index tracking. - Integrated video ended event handling in YouTubePlayer to automatically advance to the next track.
📝 WalkthroughWalkthroughThis PR introduces continuous music playback automation services for seamless playlist traversal. New services handle autoplay recommendations, playback queue management for playlists, and Spotify connection keep-alive. The AI DJ system prompt is refined to better distinguish single-track from continuous-queue requests. Provider interfaces and implementations add support for playing playlists from a specific track index, with YouTube player enhancements to trigger automatic track advancement. Changes
Sequence DiagramssequenceDiagram
participant Monitor as Autoplay Monitor
participant Store as Playback Store
participant Provider as Active Provider
participant API as Spotify/YouTube API
participant UI as App UI
Monitor->>Store: Check current track state (every 5s)
alt Track has ended
Monitor->>Provider: Fetch recommendations/related
API-->>Provider: Return suggested tracks
Provider->>Store: Enqueue recommended tracks
Store->>UI: Update queue display
else AI Queue active
Monitor->>Monitor: Skip (bypass autoplay)
else Track still playing
Monitor->>Monitor: Continue monitoring
end
sequenceDiagram
participant User as User
participant PlaylistView as PlaylistView
participant Provider as Provider
participant QueueService as Queue Service
participant QueueMonitor as Queue Monitor (YouTube)
participant Player as YouTube Player
User->>PlaylistView: Click track at index N
PlaylistView->>Provider: playPlaylistFromIndex(id, N)
alt Spotify
Provider->>Provider: Play playlist context at offset
else YouTube
Provider->>QueueService: startPlaylistPlayback(id, tracks, N)
QueueService->>QueueMonitor: Start monitoring (2s interval)
QueueService->>Player: Play track at index N
end
Player->>Player: Video plays and ends
Player->>QueueMonitor: onVideoEnded triggered
QueueMonitor->>QueueService: Advance queue, get next track
QueueService->>Player: Play next track
QueueMonitor->>QueueMonitor: Continue monitoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/ui/index.tsx (1)
357-390: Move dynamic imports to static imports at file top and ensure single advancement path for YouTube queue.Dynamic imports violate the static-import guideline. Additionally,
handleYouTubeVideoEndedraces with the YouTube queue monitor inplaybackQueueService.ts(which runs every 2s and also callsadvanceToNext()), creating potential for double-advancement or inconsistent queue state. MoveusePlaybackQueueStoreto static imports at the file top and either disable the monitor when using the handler or consolidate advancement to a single path.🔧 Proposed fix (static imports)
+import { usePlaybackQueueStore } from "../lib/playback/playbackQueueStore"; const handleYouTubeVideoEnded = async () => { - const { usePlaybackQueueStore } = await import("../lib/playback/playbackQueueStore"); - const { getActiveProvider } = await import("../providers"); - const store = usePlaybackQueueStore.getState(); const nextTrack = store.advanceToNext(); if (nextTrack) { const provider = await getActiveProvider(); await provider.playTrack(nextTrack.uri); } };apps/desktop/src/ui/views/PlaylistView.tsx (1)
153-172: Replace dynamic import with static import.The dynamic import on line 162 violates the coding guidelines. Since
playbackQueueServiceis used conditionally but from a known module, use a static import at the top of the file.Additionally, the YouTube-specific fallback (lines 161-163) appears redundant since
YouTubeProviderImplimplementsplayPlaylistFromIndex, meaning the first condition (line 159) should always handle YouTube playlists.♻️ Proposed fix
Add static import at the top of the file:
import { ArrowLeft, MusicNotes, Play, SpinnerGap, Warning } from "@phosphor-icons/react"; import { useCallback, useEffect, useRef, useState } from "react"; import useWindowLayout from "../../hooks/useWindowLayout"; import { getActiveProvider, getActiveProviderType } from "../../providers"; import type { MusicProviderType, UnifiedPlaylist, UnifiedTrack } from "../../providers/types"; +import { startPlaylistPlayback } from "../../lib/playback/playbackQueueService";Then simplify the handler:
const handlePlayTrack = async (track: UnifiedTrack, trackIndex: number) => { setPlayingId(track.id); try { const provider = await getActiveProvider(); - const type = await getActiveProviderType(); if (selectedPlaylist && provider.playPlaylistFromIndex) { await provider.playPlaylistFromIndex(selectedPlaylist.id, trackIndex); - } else if (type === "youtube" && selectedPlaylist) { - const { startPlaylistPlayback } = await import("../../lib/playback/playbackQueueService"); - await startPlaylistPlayback(selectedPlaylist.id, tracks, trackIndex); } else { await provider.playTrack(track.uri); }As per coding guidelines, static imports should be used instead of dynamic imports for better performance and code clarity.
🤖 Fix all issues with AI agents
In `@apps/desktop/src/hooks/useCurrentlyPlaying.ts`:
- Around line 162-166: The keep-alive is only started when the initial provider
equals "spotify", so switching to Spotify later never triggers it; update
useCurrentlyPlaying to start the keep-alive unconditionally (call startKeepAlive
alongside startAutoplayMonitor) or add an effect that calls startKeepAlive when
provider changes—since startKeepAlive already self-guards by provider, the
simplest fix is to remove the provider check and invoke startKeepAlive
unconditionally where startAutoplayMonitor is called (refer to startKeepAlive
and startAutoplayMonitor in useCurrentlyPlaying).
In `@apps/desktop/src/lib/playback/autoplayService.ts`:
- Around line 115-160: In handleYouTubeAutoplay, guard against unknown/zero
durations before computing isEnded: if durationMs is 0 or otherwise invalid (<=
0 or not finite) return early to avoid treating live/unknown streams as ended;
add this check just before const isEnded = ... so the rest of the autoplay logic
(getRelatedVideos, videoItemToTrackData, playbackQueue.appendTracks,
provider.playTrack) only runs for tracks with a valid duration.
In `@apps/desktop/src/lib/playback/playbackQueueService.ts`:
- Around line 8-26: startPlaylistPlayback currently calls
usePlaybackQueueStore.setPlaylistQueue with an unchecked startIndex which can be
out of bounds; validate startIndex against tracks.length first and either clamp
it to the valid range (0..tracks.length-1) or return early if invalid before
calling setPlaylistQueue. Update the local startIndex value used when calling
setPlaylistQueue and when selecting startTrack, so functions like
getActiveProvider(), provider.playTrack(startTrack.uri), lastTrackedVideoId
assignment and startYouTubeQueueMonitor() only run with a valid startTrack;
reference startPlaylistPlayback, usePlaybackQueueStore.setPlaylistQueue,
getActiveProvider, and startYouTubeQueueMonitor when making the change.
In `@apps/desktop/src/lib/playback/spotifyKeepAlive.ts`:
- Around line 168-179: In recoverAndPlay, spotifyPlay() is asynchronous so call
and await it inside the try block (e.g., await spotifyPlay()) so any rejection
is caught by the surrounding try/catch; keep the ensureActiveDevice() check and
return true only after the awaited call succeeds, and return false in the catch
branch to correctly surface failures from spotifyPlay().
In `@apps/desktop/src/providers/spotify/index.ts`:
- Around line 183-185: Add a defensive check in playPlaylistFromIndex to guard
against negative trackIndex values before calling spotifyPlayPlaylistContext:
validate that trackIndex is a non-negative integer (or clamp negative values to
0) and reject or normalize invalid inputs, then call
spotifyPlayPlaylistContext(playlistId, validatedIndex); this prevents passing a
negative offset to the Spotify API via spotifyPlayPlaylistContext.
In `@apps/desktop/src/providers/youtube/index.ts`:
- Around line 269-283: playPlaylistFromIndex lacks validation for the incoming
trackIndex and may pass an out-of-range index into startPlaylistPlayback; update
playPlaylistFromIndex to check that trackIndex is >= 0 and < tracks.length
(remember tracks may be limited by getPlaylistTracks to 200), and if out of
bounds either throw a clear error (e.g. "trackIndex out of range") or clamp to a
valid index before calling startPlaylistPlayback(playlistId, tracks,
trackIndex); reference the playPlaylistFromIndex function, cachedPlaylistTracks,
getPlaylistTracks, and startPlaylistPlayback when making the change.
🧹 Nitpick comments (8)
apps/desktop/src/lib/playback/playbackQueueService.ts (1)
48-91: Prevent overlapping async interval ticks.
setIntervalwith anasynccallback can overlap if the provider calls are slow, which risks double-advancing. Add an in-flight guard or switch to a self-schedulingsetTimeout.♻️ Proposed fix
function startYouTubeQueueMonitor(): void { if (monitorInterval) { clearInterval(monitorInterval); } + let inFlight = false; monitorInterval = setInterval(async () => { + if (inFlight) return; + inFlight = true; try { const store = usePlaybackQueueStore.getState(); const providerType = await getActiveProviderType(); @@ - } catch (err) { + } catch (err) { console.error("YouTube queue monitor error:", err); + } finally { + inFlight = false; } }, 2000); }apps/desktop/src/providers/types.ts (1)
104-105: DocumentplayPlaylistFromIndexindex semantics.Add a short JSDoc explaining whether
trackIndexis zero-based and how out-of-range values are handled.As per coding guidelines, document public APIs and interfaces.📝 Proposed addition
addToPlaylist(playlistId: string, trackUri: string): Promise<void>; + /** + * Play a playlist starting at the given zero-based track index. + */ playPlaylistFromIndex?(playlistId: string, trackIndex: number): Promise<void>;apps/desktop/src/providers/youtube/index.ts (1)
25-25: Consider cache invalidation for playlist track changes.The module-level cache is a reasonable optimization, but playlist contents may become stale if tracks are added/removed externally. Consider adding a mechanism to invalidate the cache (e.g., when the playlist is re-selected or after a TTL).
apps/desktop/src/lib/playback/playbackQueueStore.ts (1)
33-49: Consider adding bounds validation insetPlaylistQueue.If
startIndexis out of bounds relative totracks.length, subsequent calls togetCurrentTrack()will returnnullunexpectedly. While callers should validate, defensive bounds clamping here would improve robustness.♻️ Optional defensive fix
setPlaylistQueue: (playlistId, tracks, startIndex, provider) => set({ playlistId, tracks, - currentIndex: startIndex, + currentIndex: Math.max(0, Math.min(startIndex, tracks.length - 1)), provider, isPlaylistMode: true, }),apps/desktop/src/lib/playback/spotifyKeepAlive.ts (2)
30-41: Add JSDoc for the public keep-alive API.Public exports like
setKeepAliveEnabled/isKeepAliveEnabledare part of the module API but lack inline docs. Please add brief JSDoc to clarify behavior and side effects. As per coding guidelines, ...
43-61: Prevent overlapping keep-alive pings.Because the interval callback is async, a slow ping can overlap with the next tick. Add an in-flight guard to avoid concurrent recovery attempts.
♻️ Proposed fix (in-flight guard)
-let keepAliveInterval: ReturnType<typeof setInterval> | null = null; +let keepAliveInterval: ReturnType<typeof setInterval> | null = null; +let keepAliveInFlight = false; export function startKeepAlive(): void { if (keepAliveInterval) return; keepAliveInterval = setInterval(async () => { const providerType = await getActiveProviderType(); if (providerType !== "spotify" || !state.enabled) return; - await performKeepAlivePing(); + if (keepAliveInFlight) return; + keepAliveInFlight = true; + try { + await performKeepAlivePing(); + } finally { + keepAliveInFlight = false; + } }, PING_INTERVAL_MS);apps/desktop/src/lib/playback/autoplayService.ts (2)
26-37: Add JSDoc for the public autoplay API.Exports like
setAutoplayEnabled/isAutoplayEnabledare public; please add brief JSDoc to describe behavior and side effects. As per coding guidelines, ...
39-58: Avoid overlapping autoplay checks.
setInterval+ async can overlap if a check exceeds 5s. Add an in-flight guard to prevent concurrentcheckAndTriggerAutoplay()runs.♻️ Proposed fix (in-flight guard)
-let autoplayMonitorInterval: ReturnType<typeof setInterval> | null = null; +let autoplayMonitorInterval: ReturnType<typeof setInterval> | null = null; +let autoplayInFlight = false; export function startAutoplayMonitor(): void { if (autoplayMonitorInterval) return; autoplayMonitorInterval = setInterval(async () => { if (!state.enabled) return; try { - await checkAndTriggerAutoplay(); + if (autoplayInFlight) return; + autoplayInFlight = true; + try { + await checkAndTriggerAutoplay(); + } finally { + autoplayInFlight = false; + } } catch (err) { console.error("Autoplay monitor error:", err); } }, 5000); }
| // Start playback services | ||
| startAutoplayMonitor(); | ||
| if (provider === "spotify") { | ||
| startKeepAlive(); | ||
| } |
There was a problem hiding this comment.
Start keep-alive regardless of initial provider.
Right now, keep-alive only starts if the initial provider is Spotify. If the user starts on YouTube and later switches to Spotify, keep-alive never starts. Start it unconditionally (it already self-guards by provider type) or add an effect that responds to provider changes.
🔧 Proposed fix (unconditional start)
- startAutoplayMonitor();
- if (provider === "spotify") {
- startKeepAlive();
- }
+ startAutoplayMonitor();
+ startKeepAlive();🤖 Prompt for AI Agents
In `@apps/desktop/src/hooks/useCurrentlyPlaying.ts` around lines 162 - 166, The
keep-alive is only started when the initial provider equals "spotify", so
switching to Spotify later never triggers it; update useCurrentlyPlaying to
start the keep-alive unconditionally (call startKeepAlive alongside
startAutoplayMonitor) or add an effect that calls startKeepAlive when provider
changes—since startKeepAlive already self-guards by provider, the simplest fix
is to remove the provider check and invoke startKeepAlive unconditionally where
startAutoplayMonitor is called (refer to startKeepAlive and startAutoplayMonitor
in useCurrentlyPlaying).
| async function handleYouTubeAutoplay( | ||
| currentTrack: UnifiedTrack, | ||
| isPlaying: boolean, | ||
| progressMs: number, | ||
| durationMs: number | ||
| ): Promise<void> { | ||
| const playbackQueue = usePlaybackQueueStore.getState(); | ||
|
|
||
| if (playbackQueue.getRemainingCount() > 0) { | ||
| return; | ||
| } | ||
|
|
||
| const isEnded = !isPlaying && progressMs >= durationMs - 2000; | ||
| if (!isEnded) return; | ||
|
|
||
| try { | ||
| const videoId = currentTrack.id; | ||
| const relatedVideos = await getRelatedVideos(videoId, 10); | ||
|
|
||
| if (relatedVideos.length === 0) return; | ||
|
|
||
| const nextVideo = relatedVideos[0]; | ||
| const trackData = videoItemToTrackData(nextVideo); | ||
|
|
||
| const nextTrack: UnifiedTrack = { | ||
| id: trackData.id, | ||
| name: trackData.name, | ||
| durationMs: trackData.durationMs, | ||
| artists: trackData.artists.map((name, idx) => ({ id: `yt-artist-${idx}`, name })), | ||
| album: { | ||
| id: "youtube-music", | ||
| name: trackData.album, | ||
| images: trackData.albumArt ? [{ url: trackData.albumArt, width: 640, height: 640 }] : [], | ||
| }, | ||
| uri: trackData.uri, | ||
| provider: "youtube", | ||
| }; | ||
|
|
||
| playbackQueue.appendTracks([nextTrack]); | ||
|
|
||
| const provider = await getActiveProvider(); | ||
| await provider.playTrack(nextTrack.uri); | ||
| } catch (err) { | ||
| console.error("YouTube autoplay failed:", err); | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard against unknown durations for YouTube end detection.
If durationMs is 0/unknown (e.g., live streams), isEnded can evaluate true and autoplay prematurely. Add a duration guard.
🐛 Proposed fix
- const isEnded = !isPlaying && progressMs >= durationMs - 2000;
+ if (durationMs <= 0) return;
+ const isEnded = !isPlaying && progressMs >= durationMs - 2000;📝 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.
| async function handleYouTubeAutoplay( | |
| currentTrack: UnifiedTrack, | |
| isPlaying: boolean, | |
| progressMs: number, | |
| durationMs: number | |
| ): Promise<void> { | |
| const playbackQueue = usePlaybackQueueStore.getState(); | |
| if (playbackQueue.getRemainingCount() > 0) { | |
| return; | |
| } | |
| const isEnded = !isPlaying && progressMs >= durationMs - 2000; | |
| if (!isEnded) return; | |
| try { | |
| const videoId = currentTrack.id; | |
| const relatedVideos = await getRelatedVideos(videoId, 10); | |
| if (relatedVideos.length === 0) return; | |
| const nextVideo = relatedVideos[0]; | |
| const trackData = videoItemToTrackData(nextVideo); | |
| const nextTrack: UnifiedTrack = { | |
| id: trackData.id, | |
| name: trackData.name, | |
| durationMs: trackData.durationMs, | |
| artists: trackData.artists.map((name, idx) => ({ id: `yt-artist-${idx}`, name })), | |
| album: { | |
| id: "youtube-music", | |
| name: trackData.album, | |
| images: trackData.albumArt ? [{ url: trackData.albumArt, width: 640, height: 640 }] : [], | |
| }, | |
| uri: trackData.uri, | |
| provider: "youtube", | |
| }; | |
| playbackQueue.appendTracks([nextTrack]); | |
| const provider = await getActiveProvider(); | |
| await provider.playTrack(nextTrack.uri); | |
| } catch (err) { | |
| console.error("YouTube autoplay failed:", err); | |
| } | |
| } | |
| async function handleYouTubeAutoplay( | |
| currentTrack: UnifiedTrack, | |
| isPlaying: boolean, | |
| progressMs: number, | |
| durationMs: number | |
| ): Promise<void> { | |
| const playbackQueue = usePlaybackQueueStore.getState(); | |
| if (playbackQueue.getRemainingCount() > 0) { | |
| return; | |
| } | |
| if (durationMs <= 0) return; | |
| const isEnded = !isPlaying && progressMs >= durationMs - 2000; | |
| if (!isEnded) return; | |
| try { | |
| const videoId = currentTrack.id; | |
| const relatedVideos = await getRelatedVideos(videoId, 10); | |
| if (relatedVideos.length === 0) return; | |
| const nextVideo = relatedVideos[0]; | |
| const trackData = videoItemToTrackData(nextVideo); | |
| const nextTrack: UnifiedTrack = { | |
| id: trackData.id, | |
| name: trackData.name, | |
| durationMs: trackData.durationMs, | |
| artists: trackData.artists.map((name, idx) => ({ id: `yt-artist-${idx}`, name })), | |
| album: { | |
| id: "youtube-music", | |
| name: trackData.album, | |
| images: trackData.albumArt ? [{ url: trackData.albumArt, width: 640, height: 640 }] : [], | |
| }, | |
| uri: trackData.uri, | |
| provider: "youtube", | |
| }; | |
| playbackQueue.appendTracks([nextTrack]); | |
| const provider = await getActiveProvider(); | |
| await provider.playTrack(nextTrack.uri); | |
| } catch (err) { | |
| console.error("YouTube autoplay failed:", err); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/playback/autoplayService.ts` around lines 115 - 160, In
handleYouTubeAutoplay, guard against unknown/zero durations before computing
isEnded: if durationMs is 0 or otherwise invalid (<= 0 or not finite) return
early to avoid treating live/unknown streams as ended; add this check just
before const isEnded = ... so the rest of the autoplay logic (getRelatedVideos,
videoItemToTrackData, playbackQueue.appendTracks, provider.playTrack) only runs
for tracks with a valid duration.
| export async function startPlaylistPlayback( | ||
| playlistId: string, | ||
| tracks: UnifiedTrack[], | ||
| startIndex: number | ||
| ): Promise<void> { | ||
| const providerType = await getActiveProviderType(); | ||
| const store = usePlaybackQueueStore.getState(); | ||
|
|
||
| store.setPlaylistQueue(playlistId, tracks, startIndex, providerType); | ||
|
|
||
| if (providerType === "youtube") { | ||
| const provider = await getActiveProvider(); | ||
| const startTrack = tracks[startIndex]; | ||
| if (startTrack) { | ||
| await provider.playTrack(startTrack.uri); | ||
| lastTrackedVideoId = startTrack.id; | ||
| startYouTubeQueueMonitor(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate startIndex before queuing playback.
If startIndex is out of bounds, the queue is set to an invalid currentIndex and playback never starts. Clamp or early-return before setting the queue.
🔧 Proposed fix
export async function startPlaylistPlayback(
playlistId: string,
tracks: UnifiedTrack[],
startIndex: number
): Promise<void> {
const providerType = await getActiveProviderType();
const store = usePlaybackQueueStore.getState();
- store.setPlaylistQueue(playlistId, tracks, startIndex, providerType);
+ if (tracks.length === 0) return;
+ const safeIndex = Math.min(Math.max(startIndex, 0), tracks.length - 1);
+ store.setPlaylistQueue(playlistId, tracks, safeIndex, providerType);
if (providerType === "youtube") {
const provider = await getActiveProvider();
- const startTrack = tracks[startIndex];
+ const startTrack = tracks[safeIndex];
if (startTrack) {
await provider.playTrack(startTrack.uri);
lastTrackedVideoId = startTrack.id;
startYouTubeQueueMonitor();
}
}
}📝 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.
| export async function startPlaylistPlayback( | |
| playlistId: string, | |
| tracks: UnifiedTrack[], | |
| startIndex: number | |
| ): Promise<void> { | |
| const providerType = await getActiveProviderType(); | |
| const store = usePlaybackQueueStore.getState(); | |
| store.setPlaylistQueue(playlistId, tracks, startIndex, providerType); | |
| if (providerType === "youtube") { | |
| const provider = await getActiveProvider(); | |
| const startTrack = tracks[startIndex]; | |
| if (startTrack) { | |
| await provider.playTrack(startTrack.uri); | |
| lastTrackedVideoId = startTrack.id; | |
| startYouTubeQueueMonitor(); | |
| } | |
| } | |
| export async function startPlaylistPlayback( | |
| playlistId: string, | |
| tracks: UnifiedTrack[], | |
| startIndex: number | |
| ): Promise<void> { | |
| const providerType = await getActiveProviderType(); | |
| const store = usePlaybackQueueStore.getState(); | |
| if (tracks.length === 0) return; | |
| const safeIndex = Math.min(Math.max(startIndex, 0), tracks.length - 1); | |
| store.setPlaylistQueue(playlistId, tracks, safeIndex, providerType); | |
| if (providerType === "youtube") { | |
| const provider = await getActiveProvider(); | |
| const startTrack = tracks[safeIndex]; | |
| if (startTrack) { | |
| await provider.playTrack(startTrack.uri); | |
| lastTrackedVideoId = startTrack.id; | |
| startYouTubeQueueMonitor(); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/playback/playbackQueueService.ts` around lines 8 - 26,
startPlaylistPlayback currently calls usePlaybackQueueStore.setPlaylistQueue
with an unchecked startIndex which can be out of bounds; validate startIndex
against tracks.length first and either clamp it to the valid range
(0..tracks.length-1) or return early if invalid before calling setPlaylistQueue.
Update the local startIndex value used when calling setPlaylistQueue and when
selecting startTrack, so functions like getActiveProvider(),
provider.playTrack(startTrack.uri), lastTrackedVideoId assignment and
startYouTubeQueueMonitor() only run with a valid startTrack; reference
startPlaylistPlayback, usePlaybackQueueStore.setPlaylistQueue,
getActiveProvider, and startYouTubeQueueMonitor when making the change.
| export async function recoverAndPlay(): Promise<boolean> { | ||
| const hasDevice = await ensureActiveDevice(); | ||
| if (!hasDevice) return false; | ||
|
|
||
| try { | ||
| spotifyPlay(); | ||
| return true; | ||
| } catch (err) { | ||
| console.error("Failed to resume playback after recovery:", err); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Await spotifyPlay() to surface failures.
spotifyPlay() is async; without await, errors bypass the try/catch and the function returns true even on failure.
🐛 Proposed fix
try {
- spotifyPlay();
+ await spotifyPlay();
return true;
} catch (err) {
console.error("Failed to resume playback after recovery:", err);
return false;
}📝 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.
| export async function recoverAndPlay(): Promise<boolean> { | |
| const hasDevice = await ensureActiveDevice(); | |
| if (!hasDevice) return false; | |
| try { | |
| spotifyPlay(); | |
| return true; | |
| } catch (err) { | |
| console.error("Failed to resume playback after recovery:", err); | |
| return false; | |
| } | |
| } | |
| export async function recoverAndPlay(): Promise<boolean> { | |
| const hasDevice = await ensureActiveDevice(); | |
| if (!hasDevice) return false; | |
| try { | |
| await spotifyPlay(); | |
| return true; | |
| } catch (err) { | |
| console.error("Failed to resume playback after recovery:", err); | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/playback/spotifyKeepAlive.ts` around lines 168 - 179, In
recoverAndPlay, spotifyPlay() is asynchronous so call and await it inside the
try block (e.g., await spotifyPlay()) so any rejection is caught by the
surrounding try/catch; keep the ensureActiveDevice() check and return true only
after the awaited call succeeds, and return false in the catch branch to
correctly surface failures from spotifyPlay().
| async playPlaylistFromIndex(playlistId: string, trackIndex: number): Promise<void> { | ||
| await spotifyPlayPlaylistContext(playlistId, trackIndex); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the spotifyPlayPlaylistContext function definition
rg "spotifyPlayPlaylistContext" --type ts --type tsx -B 2 -A 10Repository: ModioStudio/MiniFy
Length of output: 89
🏁 Script executed:
# Also check the file structure to understand the context better
fd "spotify" --type f | head -20Repository: ModioStudio/MiniFy
Length of output: 661
🏁 Script executed:
# Look for how playPlaylistFromIndex is called from the UI
rg "playPlaylistFromIndex" --type ts --type tsx -B 2 -A 2Repository: ModioStudio/MiniFy
Length of output: 89
🏁 Script executed:
# Search for spotifyPlayPlaylistContext without file type restriction
rg "spotifyPlayPlaylistContext" -B 2 -A 10Repository: ModioStudio/MiniFy
Length of output: 1946
🏁 Script executed:
# Read the main Spotify provider file
cat -n apps/desktop/src/providers/spotify/index.ts | head -200Repository: ModioStudio/MiniFy
Length of output: 6251
🏁 Script executed:
# Check spotifyTools and spotifyClient for context
cat -n apps/desktop/src/lib/spotifyTools.tsRepository: ModioStudio/MiniFy
Length of output: 21340
🏁 Script executed:
# Find and read the client file where playPlaylistContext is implemented
cat -n apps/desktop/src/providers/spotify/client.ts | head -300Repository: ModioStudio/MiniFy
Length of output: 108
🏁 Script executed:
# Also check if there are more lines in that client file
wc -l apps/desktop/src/providers/spotify/client.tsRepository: ModioStudio/MiniFy
Length of output: 108
🏁 Script executed:
# Search for how playPlaylistFromIndex is called from UI
rg "playPlaylistFromIndex" -B 2 -A 2Repository: ModioStudio/MiniFy
Length of output: 1796
🏁 Script executed:
# Read the spotifyClient.ts file where playPlaylistContext is defined
cat -n apps/desktop/src/ui/spotifyClient.ts | head -400Repository: ModioStudio/MiniFy
Length of output: 13924
🏁 Script executed:
# Check full length to see if we need more content
wc -l apps/desktop/src/ui/spotifyClient.tsRepository: ModioStudio/MiniFy
Length of output: 102
🏁 Script executed:
# Search specifically for playPlaylistContext function
rg "playPlaylistContext" -B 2 -A 15 apps/desktop/src/ui/spotifyClient.tsRepository: ModioStudio/MiniFy
Length of output: 563
🏁 Script executed:
# Read PlaylistView.tsx around the playPlaylistFromIndex call to understand trackIndex origin
cat -n apps/desktop/src/ui/views/PlaylistView.tsx | grep -A 20 -B 20 "playPlaylistFromIndex"Repository: ModioStudio/MiniFy
Length of output: 1670
🏁 Script executed:
# Check YouTube provider to see if it validates trackIndex
cat -n apps/desktop/src/providers/youtube/index.ts | grep -A 20 "playPlaylistFromIndex"Repository: ModioStudio/MiniFy
Length of output: 793
Add validation to guard against negative trackIndex values.
The spotifyPlayPlaylistContext function passes offset directly to the Spotify API without bounds checking. While the UI typically passes valid array indices, defensive programming should prevent negative values from reaching the API.
🔧 Suggested guard
async playPlaylistFromIndex(playlistId: string, trackIndex: number): Promise<void> {
- await spotifyPlayPlaylistContext(playlistId, trackIndex);
+ const safeIndex = Math.max(0, trackIndex);
+ await spotifyPlayPlaylistContext(playlistId, safeIndex);
}🤖 Prompt for AI Agents
In `@apps/desktop/src/providers/spotify/index.ts` around lines 183 - 185, Add a
defensive check in playPlaylistFromIndex to guard against negative trackIndex
values before calling spotifyPlayPlaylistContext: validate that trackIndex is a
non-negative integer (or clamp negative values to 0) and reject or normalize
invalid inputs, then call spotifyPlayPlaylistContext(playlistId,
validatedIndex); this prevents passing a negative offset to the Spotify API via
spotifyPlayPlaylistContext.
| async playPlaylistFromIndex(playlistId: string, trackIndex: number): Promise<void> { | ||
| let tracks = cachedPlaylistTracks.get(playlistId); | ||
|
|
||
| if (!tracks) { | ||
| const result = await this.getPlaylistTracks(playlistId, 200, 0); | ||
| tracks = result.tracks; | ||
| cachedPlaylistTracks.set(playlistId, tracks); | ||
| } | ||
|
|
||
| if (tracks.length === 0) { | ||
| throw new Error("Playlist has no tracks"); | ||
| } | ||
|
|
||
| await startPlaylistPlayback(playlistId, tracks, trackIndex); | ||
| } |
There was a problem hiding this comment.
Missing bounds validation for trackIndex.
If trackIndex exceeds the fetched tracks length (limited to 200), startPlaylistPlayback will attempt to access an undefined track. Add bounds validation before delegating to playback.
🐛 Proposed fix
async playPlaylistFromIndex(playlistId: string, trackIndex: number): Promise<void> {
let tracks = cachedPlaylistTracks.get(playlistId);
if (!tracks) {
const result = await this.getPlaylistTracks(playlistId, 200, 0);
tracks = result.tracks;
cachedPlaylistTracks.set(playlistId, tracks);
}
if (tracks.length === 0) {
throw new Error("Playlist has no tracks");
}
+ if (trackIndex < 0 || trackIndex >= tracks.length) {
+ throw new Error(`Track index ${trackIndex} is out of bounds (0-${tracks.length - 1})`);
+ }
+
await startPlaylistPlayback(playlistId, tracks, trackIndex);
}📝 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.
| async playPlaylistFromIndex(playlistId: string, trackIndex: number): Promise<void> { | |
| let tracks = cachedPlaylistTracks.get(playlistId); | |
| if (!tracks) { | |
| const result = await this.getPlaylistTracks(playlistId, 200, 0); | |
| tracks = result.tracks; | |
| cachedPlaylistTracks.set(playlistId, tracks); | |
| } | |
| if (tracks.length === 0) { | |
| throw new Error("Playlist has no tracks"); | |
| } | |
| await startPlaylistPlayback(playlistId, tracks, trackIndex); | |
| } | |
| async playPlaylistFromIndex(playlistId: string, trackIndex: number): Promise<void> { | |
| let tracks = cachedPlaylistTracks.get(playlistId); | |
| if (!tracks) { | |
| const result = await this.getPlaylistTracks(playlistId, 200, 0); | |
| tracks = result.tracks; | |
| cachedPlaylistTracks.set(playlistId, tracks); | |
| } | |
| if (tracks.length === 0) { | |
| throw new Error("Playlist has no tracks"); | |
| } | |
| if (trackIndex < 0 || trackIndex >= tracks.length) { | |
| throw new Error(`Track index ${trackIndex} is out of bounds (0-${tracks.length - 1})`); | |
| } | |
| await startPlaylistPlayback(playlistId, tracks, trackIndex); | |
| } |
🤖 Prompt for AI Agents
In `@apps/desktop/src/providers/youtube/index.ts` around lines 269 - 283,
playPlaylistFromIndex lacks validation for the incoming trackIndex and may pass
an out-of-range index into startPlaylistPlayback; update playPlaylistFromIndex
to check that trackIndex is >= 0 and < tracks.length (remember tracks may be
limited by getPlaylistTracks to 200), and if out of bounds either throw a clear
error (e.g. "trackIndex out of range") or clamp to a valid index before calling
startPlaylistPlayback(playlistId, tracks, trackIndex); reference the
playPlaylistFromIndex function, cachedPlaylistTracks, getPlaylistTracks, and
startPlaylistPlayback when making the change.
1 participant
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Fixes #(issue number)
Closes #(issue number)
Related to #(issue number)
Changes Made
Testing
Screenshots (if applicable)
Add screenshots to help explain your changes.
Checklist
Additional Notes
Add any additional notes about the PR here.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.