From 239d5d3975eb0cf732a6bdbe48a149f3f0fb1d84 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 27 Jun 2026 11:01:59 -0400 Subject: [PATCH] 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. --- ui/src/reducers/playerReducer.js | 18 ++++ ui/src/reducers/playerReducer.test.js | 121 ++++++++++++++++++++++++++ 2 files changed, 139 insertions(+) diff --git a/ui/src/reducers/playerReducer.js b/ui/src/reducers/playerReducer.js index d6ab7484b..c18db3dc8 100644 --- a/ui/src/reducers/playerReducer.js +++ b/ui/src/reducers/playerReducer.js @@ -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, diff --git a/ui/src/reducers/playerReducer.test.js b/ui/src/reducers/playerReducer.test.js index 110ce8c53..2872a3f08 100644 --- a/ui/src/reducers/playerReducer.test.js +++ b/ui/src/reducers/playerReducer.test.js @@ -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) + }) + }) })