From 4bae7005cdaf8d16cee4369b93034b567ec1a858 Mon Sep 17 00:00:00 2001 From: cucadmuh <49571317+cucadmuh@users.noreply.github.com> Date: Thu, 9 Jul 2026 17:46:07 +0300 Subject: [PATCH] fix(sync): self-heal failed play-queue push without a nagging LED (#1264) --- src/app/hooks/useIdlePlayQueuePull.ts | 2 ++ .../store/applyServerPlayQueue.idle.test.ts | 18 ++++++++++ .../playback/store/applyServerPlayQueue.ts | 9 +++-- .../playback/store/queuePlaybackIdle.test.ts | 36 +++++++++++++++++++ .../playback/store/queuePlaybackIdle.ts | 23 ++++++++++++ src/features/playback/store/queueSync.test.ts | 36 +++++++++++++++++-- src/features/playback/store/queueSync.ts | 23 ++++++++---- 7 files changed, 135 insertions(+), 12 deletions(-) diff --git a/src/app/hooks/useIdlePlayQueuePull.ts b/src/app/hooks/useIdlePlayQueuePull.ts index f216193e..c341e9b4 100644 --- a/src/app/hooks/useIdlePlayQueuePull.ts +++ b/src/app/hooks/useIdlePlayQueuePull.ts @@ -6,6 +6,7 @@ import { usePlayerStore } from '@/features/playback/store/playerStore'; import { getPlaybackIdleSinceMs, isIdleQueuePullSuspended, + isQueuePushFailed, isQueueNaturallyEnded, isPlaybackIdleLongEnough, markPlaybackIdle, @@ -39,6 +40,7 @@ export function useIdlePlayQueuePull(status: ConnectionStatus) { if (isPlaying) return; if (!isPlaybackIdleLongEnough(IDLE_THRESHOLD_MS)) return; if (isIdleQueuePullSuspended()) return; + if (isQueuePushFailed()) return; if (isQueueNaturallyEnded()) return; if (hasPendingQueueSync()) return; if (!activeServerId) return; diff --git a/src/features/playback/store/applyServerPlayQueue.idle.test.ts b/src/features/playback/store/applyServerPlayQueue.idle.test.ts index e8ff8a96..b2c841fd 100644 --- a/src/features/playback/store/applyServerPlayQueue.idle.test.ts +++ b/src/features/playback/store/applyServerPlayQueue.idle.test.ts @@ -57,6 +57,7 @@ import { _resetQueuePlaybackIdleForTest, getIdlePullGeneration, isIdleQueuePullSuspended, + markQueuePushFailed, touchQueueMutationClock, } from '@/features/playback/store/queuePlaybackIdle'; @@ -94,6 +95,23 @@ describe('applyServerPlayQueue idle guards', () => { expect(isIdleQueuePullSuspended()).toBe(true); }); + it('does not apply server queue in idle mode while a failed push blocks pull', async () => { + getPlayQueueForServerMock.mockResolvedValue({ + songs: [{ id: 'remote-a' }, { id: 'remote-b' }], + current: 'remote-a', + position: 5000, + }); + markQueuePushFailed(); + + const result = await applyServerPlayQueue('srv-a', { mode: 'idle' }); + + expect(result).toBe('noop'); + expect(getPlayQueueForServerMock).not.toHaveBeenCalled(); + expect(playerState.queueItems).toEqual([{ serverId: 'srv-a', trackId: 'local-only' }]); + // The failed-push guard blocks pull without implying a user-edit suspension. + expect(isIdleQueuePullSuspended()).toBe(false); + }); + it('ignores stale idle pull responses after a local mutation during fetch', async () => { const generationAtFetch = getIdlePullGeneration(); getPlayQueueForServerMock.mockImplementation(async () => { diff --git a/src/features/playback/store/applyServerPlayQueue.ts b/src/features/playback/store/applyServerPlayQueue.ts index fe68257c..858f8b3e 100644 --- a/src/features/playback/store/applyServerPlayQueue.ts +++ b/src/features/playback/store/applyServerPlayQueue.ts @@ -13,6 +13,8 @@ import { refreshWaveformForTrack } from '@/features/playback/store/waveformRefre import { getIdlePullGeneration, isIdleQueuePullSuspended, + isQueuePushFailed, + clearQueuePushFailed, resumeIdleQueuePull, clearQueueNaturallyEnded, } from '@/features/playback/store/queuePlaybackIdle'; @@ -131,7 +133,7 @@ export async function applyServerPlayQueue( const profileId = resolveServerProfileId(serverId); if (!profileId) return 'error'; - if (options.mode === 'idle' && isIdleQueuePullSuspended()) { + if (options.mode === 'idle' && (isIdleQueuePullSuspended() || isQueuePushFailed())) { return 'noop'; } const idleGenerationAtStart = options.mode === 'idle' ? getIdlePullGeneration() : null; @@ -142,7 +144,7 @@ export async function applyServerPlayQueue( const preferServerPosition = options.preferServerPosition ?? options.mode !== 'startup'; if (options.mode === 'idle') { - if (isIdleQueuePullSuspended()) return 'noop'; + if (isIdleQueuePullSuspended() || isQueuePushFailed()) return 'noop'; if (idleGenerationAtStart !== getIdlePullGeneration()) return 'noop'; const serverFp = fingerprintFromServer(q); const localFp = fingerprintFromLocalQueue(); @@ -186,6 +188,7 @@ export async function pullPlayQueueFromActiveServer(): Promise { }); }); +describe('queue push failed flag', () => { + beforeEach(() => { + _resetQueuePlaybackIdleForTest(); + }); + + it('starts clear and toggles independently of idle-pull suspension', () => { + expect(isQueuePushFailed()).toBe(false); + markQueuePushFailed(); + expect(isQueuePushFailed()).toBe(true); + // The failed-push flag must not imply user-edit suspension (no yellow LED). + expect(isIdleQueuePullSuspended()).toBe(false); + clearQueuePushFailed(); + expect(isQueuePushFailed()).toBe(false); + }); + + it('does not notify idle-pull suspension subscribers (LED stays put)', () => { + let count = 0; + const unsub = subscribeIdleQueuePullSuspended(() => { + count += 1; + }); + markQueuePushFailed(); + clearQueuePushFailed(); + expect(count).toBe(0); + unsub(); + }); + + it('is reset by the test reset helper', () => { + markQueuePushFailed(); + _resetQueuePlaybackIdleForTest(); + expect(isQueuePushFailed()).toBe(false); + }); +}); + describe('natural queue end', () => { beforeEach(() => { _resetQueuePlaybackIdleForTest(); diff --git a/src/features/playback/store/queuePlaybackIdle.ts b/src/features/playback/store/queuePlaybackIdle.ts index 4d4e6c9d..11180679 100644 --- a/src/features/playback/store/queuePlaybackIdle.ts +++ b/src/features/playback/store/queuePlaybackIdle.ts @@ -4,6 +4,14 @@ let playbackIdleSinceMs = 0; let lastQueueMutationAt = 0; /** When true, idle auto-pull is disabled until manual pull re-enables it. */ let idleQueuePullSuspended = false; +/** + * True when the last queue push to the server failed (offline / unreachable / + * URI-too-long). Blocks idle auto-pull so a stale server snapshot cannot rewind + * local playback, but is transient and self-clearing: any later successful push + * (or a manual pull) clears it. Distinct from `idleQueuePullSuspended` — it does + * NOT drive the handoff LED, so a single transient failure does not nag the user. + */ +let queuePushFailed = false; /** Set when repeat-off playback reaches the queue tail — blocks idle pull until play resumes. */ let queueNaturallyEnded = false; /** Bumped on each local queue mutation; stale in-flight idle pulls must not apply. */ @@ -71,6 +79,20 @@ export function isIdleQueuePullSuspended(): boolean { return idleQueuePullSuspended; } +/** Mark the last server push as failed (blocks idle pull until a push succeeds). */ +export function markQueuePushFailed(): void { + queuePushFailed = true; +} + +/** Clear the failed-push guard — called on a successful push or a manual pull. */ +export function clearQueuePushFailed(): void { + queuePushFailed = false; +} + +export function isQueuePushFailed(): boolean { + return queuePushFailed; +} + export function getIdlePullGeneration(): number { return idlePullGeneration; } @@ -95,6 +117,7 @@ export function _resetQueuePlaybackIdleForTest(): void { playbackIdleSinceMs = 0; lastQueueMutationAt = 0; idleQueuePullSuspended = false; + queuePushFailed = false; queueNaturallyEnded = false; idlePullGeneration = 0; } diff --git a/src/features/playback/store/queueSync.test.ts b/src/features/playback/store/queueSync.test.ts index dccd4849..56c09238 100644 --- a/src/features/playback/store/queueSync.test.ts +++ b/src/features/playback/store/queueSync.test.ts @@ -48,6 +48,7 @@ import { import { _resetQueuePlaybackIdleForTest, isIdleQueuePullSuspended, + isQueuePushFailed, isQueueNaturallyEnded, } from '@/features/playback/store/queuePlaybackIdle'; @@ -119,12 +120,13 @@ describe('syncUserQueueMutationToServer (debounced)', () => { expect(isIdleQueuePullSuspended()).toBe(true); }); - it('keeps idle pull suspended when debounced push fails', async () => { + it('keeps idle pull suspended and flags the failed push when debounced push fails', async () => { savePlayQueueMock.mockRejectedValueOnce(new Error('offline')); syncUserQueueMutationToServer(queue, track('a'), 30); vi.advanceTimersByTime(5000); await Promise.resolve(); expect(isIdleQueuePullSuspended()).toBe(true); + expect(isQueuePushFailed()).toBe(true); }); }); @@ -148,6 +150,7 @@ describe('pushQueueOnPlaybackStart', () => { await vi.runAllTimersAsync(); expect(savePlayQueueMock).toHaveBeenCalled(); expect(isIdleQueuePullSuspended()).toBe(true); + expect(isQueuePushFailed()).toBe(true); }); it('debounces when idle pull is not suspended', () => { @@ -158,11 +161,38 @@ describe('pushQueueOnPlaybackStart', () => { }); describe('flushQueueSyncToServer failure', () => { - it('suspends idle pull when an immediate push fails', async () => { + it('flags the failed push (blocking idle pull) without lighting the handoff LED', async () => { savePlayQueueMock.mockRejectedValueOnce(new Error('offline')); const ok = await flushQueueSyncToServer([ref('a')], track('a'), 12); expect(ok).toBe(false); - expect(isIdleQueuePullSuspended()).toBe(true); + // Blocks idle auto-pull (safety) but is a separate, transient flag — a bare + // push failure must not drive the user-edit handoff suspension / yellow LED. + expect(isQueuePushFailed()).toBe(true); + expect(isIdleQueuePullSuspended()).toBe(false); + }); + + it('self-heals: a later successful push clears the failed-push flag', async () => { + savePlayQueueMock.mockRejectedValueOnce(new Error('offline')); + await flushQueueSyncToServer([ref('a')], track('a'), 12); + expect(isQueuePushFailed()).toBe(true); + + const ok = await flushQueueSyncToServer([ref('a')], track('a'), 20); + expect(ok).toBe(true); + expect(isQueuePushFailed()).toBe(false); + expect(isIdleQueuePullSuspended()).toBe(false); + }); + + it('debounced playback push failure flags without suspending, then self-heals', async () => { + savePlayQueueMock.mockRejectedValueOnce(new Error('offline')); + syncQueueToServer([ref('a')], track('a'), 12); + vi.advanceTimersByTime(5000); + await Promise.resolve(); + expect(isQueuePushFailed()).toBe(true); + expect(isIdleQueuePullSuspended()).toBe(false); + + const ok = await flushQueueSyncToServer([ref('a')], track('a'), 20); + expect(ok).toBe(true); + expect(isQueuePushFailed()).toBe(false); }); }); diff --git a/src/features/playback/store/queueSync.ts b/src/features/playback/store/queueSync.ts index 900f0272..9491e738 100644 --- a/src/features/playback/store/queueSync.ts +++ b/src/features/playback/store/queueSync.ts @@ -12,7 +12,9 @@ import { touchQueueMutationClock, isIdleQueuePullSuspended, resumeIdleQueuePull, - suspendIdleQueuePull, + markQueuePushFailed, + clearQueuePushFailed, + isQueuePushFailed, markQueueNaturallyEnded, } from '@/features/playback/store/queuePlaybackIdle'; import { usePlayerStore } from '@/features/playback/store/playerStore'; @@ -60,11 +62,18 @@ function pushRefsForServer( const ids = refs.slice(0, QUEUE_ID_LIMIT).map(r => r.trackId); const pos = Math.floor(currentTime * 1000); return savePlayQueue(ids, currentTrack.id, pos, serverId).then( - () => true, () => { - // Offline / unreachable / URI-too-long: keep local queue authoritative - // so idle auto-pull cannot rewind to the last successful server snapshot. - suspendIdleQueuePull(); + // Server accepted the queue: local and server agree, so any prior failed + // push is resolved and idle auto-pull can safely resume. + clearQueuePushFailed(); + return true; + }, + () => { + // Offline / unreachable / URI-too-long: keep local queue authoritative so + // idle auto-pull cannot rewind to the last successful server snapshot. + // Transient and self-clearing (next successful push), and does not light + // the handoff LED — unlike a user edit's `idleQueuePullSuspended`. + markQueuePushFailed(); return false; }, ); @@ -187,7 +196,7 @@ export function pushQueueOnPlaybackStart( currentTime: number, ): void { if (!currentTrack || queue.length === 0) return; - if (isIdleQueuePullSuspended()) { + if (isIdleQueuePullSuspended() || isQueuePushFailed()) { void flushQueueSyncToServer(queue, currentTrack, currentTime).then(ok => { if (ok) resumeIdleQueuePull(); }); @@ -197,7 +206,7 @@ export function pushQueueOnPlaybackStart( } export function flushLocalQueueWhenTakingPlayback(): Promise { - if (!isIdleQueuePullSuspended()) return Promise.resolve(); + if (!isIdleQueuePullSuspended() && !isQueuePushFailed()) return Promise.resolve(); const s = usePlayerStore.getState(); if (s.currentRadio || !s.currentTrack || s.queueItems.length === 0) { return Promise.resolve();