mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-01 07:21:17 +00:00
fix(ui): prevent transient jump to wrong song on play (#5441 regression)
When playing a song, the web player would sometimes briefly skip to a different song, attempt to play it, then snap back to the chosen track. Root cause: the music player can emit a PLAYER_SYNC_QUEUE carrying its previous (old) queue while it loads the newly selected one. Since #5441 widened reduceSyncQueue's hasPendingSwitch to keep playIndex alive whenever clear=true, that stale list was adopted into state with playIndex pointing at a track the user never chose. The library then played that track for an instant before the correct queue settled. Fix: when a switch is pending, only adopt a synced queue that actually contains the pending track (state.queue[playIndex], matched by trackId). A sync missing it is stale and is ignored, keeping the intended queue and pending switch alive so the next, correct sync is adopted normally.
This commit is contained in:
parent
13e96a0e81
commit
239d5d3975
@ -173,6 +173,24 @@ const reduceSyncQueue = (state, { data: { audioInfo, audioLists } }) => {
|
||||
const hasPendingSwitch =
|
||||
state.playIndex != null &&
|
||||
(state.clear || state.playIndex !== state.savedPlayIndex)
|
||||
|
||||
// The music player can emit a SYNC_QUEUE carrying its previous (old) queue
|
||||
// while it loads the new one. Adopting that stale list would point playIndex
|
||||
// at a track the user never chose, causing a transient jump to a "random"
|
||||
// song before the correct queue settles. Detect a stale sync by checking that
|
||||
// the synced list actually contains the pending track (state.queue[playIndex]);
|
||||
// if not, ignore it: keep our intended queue and the pending switch so the
|
||||
// next, correct sync is adopted normally.
|
||||
if (hasPendingSwitch) {
|
||||
const pendingTrack = state.queue[state.playIndex]
|
||||
const syncHasPendingTrack =
|
||||
pendingTrack != null &&
|
||||
audioLists.some((item) => item.trackId === pendingTrack.trackId)
|
||||
if (!syncHasPendingTrack) {
|
||||
return state
|
||||
}
|
||||
}
|
||||
|
||||
return {
|
||||
...state,
|
||||
queue: audioLists,
|
||||
|
||||
@ -224,4 +224,125 @@ describe('playerReducer', () => {
|
||||
expect(result.playIndex).toBe(0)
|
||||
})
|
||||
})
|
||||
|
||||
// Regression context for the 0.62 report:
|
||||
// "When I play a song, it will sometimes skip to a random song, attempt to
|
||||
// play said song, and then immediately back to the song I chose."
|
||||
//
|
||||
// Root cause: commit 2307a64da (#5441, 0.62-only) widened reduceSyncQueue's
|
||||
// hasPendingSwitch to also keep playIndex alive whenever clear=true. This let
|
||||
// a pending playIndex survive a SYNC_QUEUE that carries the OLD queue (the
|
||||
// music player library can emit a stale onAudioListsChange mid-transition),
|
||||
// leaving playIndex pointing at a track in the WRONG queue. Downstream, the
|
||||
// library's updatePlayIndex(playIndex) played that stale track for an instant
|
||||
// ("random song") before the new queue settled ("back to the song I chose").
|
||||
// The race was timing-dependent, hence "sometimes". The FIX block below guards
|
||||
// reduceSyncQueue against adopting a stale list; these tests pin the delta.
|
||||
describe('0.62 regression context', () => {
|
||||
// 0.61 logic, for contrast: a switch is pending ONLY when the index moved.
|
||||
const hasPendingSwitch061 = (s) =>
|
||||
s.playIndex != null && s.playIndex !== s.savedPlayIndex
|
||||
|
||||
it('0.61 vs 0.62 differ ONLY when playIndex===savedPlayIndex && clear', () => {
|
||||
const hasPendingSwitch062 = (s) =>
|
||||
s.playIndex != null && (s.clear || s.playIndex !== s.savedPlayIndex)
|
||||
|
||||
const cases = [
|
||||
{ playIndex: 0, savedPlayIndex: 0, clear: true }, // the regression case
|
||||
{ playIndex: 0, savedPlayIndex: 3, clear: true },
|
||||
{ playIndex: 2, savedPlayIndex: 2, clear: false },
|
||||
{ playIndex: 5, savedPlayIndex: 0, clear: false },
|
||||
]
|
||||
const diffs = cases.filter(
|
||||
(c) => hasPendingSwitch061(c) !== hasPendingSwitch062(c),
|
||||
)
|
||||
expect(diffs).toEqual([{ playIndex: 0, savedPlayIndex: 0, clear: true }])
|
||||
})
|
||||
})
|
||||
|
||||
// FIX for the 0.62 stale-playIndex regression. A SYNC_QUEUE that does NOT
|
||||
// contain the pending track (state.queue[playIndex]) is a stale snapshot of
|
||||
// the library's previous queue. Adopting it would point playIndex at a track
|
||||
// the user never chose. The fix ignores such a stale sync: it keeps our
|
||||
// intended queue and keeps the pending switch alive so the next (correct)
|
||||
// sync — once the library finishes loading the new queue — is adopted.
|
||||
describe('FIX: SYNC_QUEUE ignores a stale list missing the pending track', () => {
|
||||
it('keeps the intended queue and pending switch when sync is stale', () => {
|
||||
const intendedQueue = [{ trackId: 'b0', uuid: 'B0', name: 'Chosen Song' }]
|
||||
const stateAfterSetTrack = {
|
||||
queue: intendedQueue,
|
||||
current: {},
|
||||
playIndex: 0,
|
||||
savedPlayIndex: 0,
|
||||
clear: true,
|
||||
volume: 1,
|
||||
}
|
||||
const staleOldQueue = [
|
||||
{ trackId: 'a0', uuid: 'A0', name: 'Old Song 0' },
|
||||
{ trackId: 'a1', uuid: 'A1', name: 'Old Song 1' },
|
||||
{ trackId: 'a2', uuid: 'A2', name: 'Old Song 2' },
|
||||
]
|
||||
const result = playerReducer(stateAfterSetTrack, {
|
||||
type: PLAYER_SYNC_QUEUE,
|
||||
data: { audioInfo: {}, audioLists: staleOldQueue },
|
||||
})
|
||||
|
||||
// Intended queue is preserved; the stale old queue is NOT adopted.
|
||||
expect(result.queue).toBe(intendedQueue)
|
||||
// Pending switch stays alive for the next, correct sync.
|
||||
expect(result.playIndex).toBe(0)
|
||||
expect(result.clear).toBe(true)
|
||||
// playIndex now points at the chosen track, never an old one.
|
||||
expect(result.queue[result.playIndex].name).toBe('Chosen Song')
|
||||
})
|
||||
|
||||
it('adopts a valid sync that contains the pending track (by trackId)', () => {
|
||||
// Mirrors the real flow: state.queue and the synced list share trackIds
|
||||
// but have library-assigned uuids. The pending track (trackId s1) is
|
||||
// present, so the sync is valid and must be adopted.
|
||||
const stateAfterPlayTracks = {
|
||||
queue: [
|
||||
{ trackId: 's1', uuid: 'aaa', name: 'Song 1' },
|
||||
{ trackId: 's2', uuid: 'bbb', name: 'Song 2' },
|
||||
{ trackId: 's3', uuid: 'ccc', name: 'Song 3' },
|
||||
],
|
||||
current: { uuid: 'ccc', name: 'Song 3' },
|
||||
playIndex: 0,
|
||||
savedPlayIndex: 2,
|
||||
clear: true,
|
||||
volume: 1,
|
||||
}
|
||||
const validSync = [
|
||||
{ trackId: 's1', uuid: 'xxx', name: 'Song 1' },
|
||||
{ trackId: 's2', uuid: 'yyy', name: 'Song 2' },
|
||||
{ trackId: 's3', uuid: 'zzz', name: 'Song 3' },
|
||||
]
|
||||
const result = playerReducer(stateAfterPlayTracks, {
|
||||
type: PLAYER_SYNC_QUEUE,
|
||||
data: { audioInfo: {}, audioLists: validSync },
|
||||
})
|
||||
expect(result.queue).toBe(validSync)
|
||||
expect(result.playIndex).toBe(0)
|
||||
expect(result.clear).toBe(true)
|
||||
})
|
||||
|
||||
it('adopts the sync normally when no switch is pending', () => {
|
||||
const stateNoPending = {
|
||||
queue: [{ trackId: 's1', uuid: 'aaa', name: 'Song 1' }],
|
||||
current: {},
|
||||
playIndex: undefined,
|
||||
savedPlayIndex: 0,
|
||||
clear: false,
|
||||
volume: 1,
|
||||
}
|
||||
const reordered = [{ trackId: 's1', uuid: 'aaa', name: 'Song 1' }]
|
||||
const result = playerReducer(stateNoPending, {
|
||||
type: PLAYER_SYNC_QUEUE,
|
||||
data: { audioInfo: {}, audioLists: reordered },
|
||||
})
|
||||
expect(result.queue).toBe(reordered)
|
||||
expect(result.playIndex).toBeUndefined()
|
||||
expect(result.clear).toBe(false)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user