From 9764d3031c3ed1c8e57b77f4a6303af49384060f Mon Sep 17 00:00:00 2001 From: Cristan Beskid <596554+sferra@users.noreply.github.com> Date: Thu, 29 Feb 2024 22:41:59 +0100 Subject: [PATCH 1/9] Fix hanging playback when no stream is available --- packages/app/app/actions/queue.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/packages/app/app/actions/queue.ts b/packages/app/app/actions/queue.ts index f6db772737..f86abbb836 100644 --- a/packages/app/app/actions/queue.ts +++ b/packages/app/app/actions/queue.ts @@ -209,6 +209,12 @@ export const findStreamsForTrack = (index: number) => async (dispatch, getState) ...streamData.slice(1) ]; + const firstStream = streamData[0]; + if (!firstStream?.stream) { + removeFirstStream(track, index, streamData, dispatch); + return; + } + dispatch( updateQueueItem({ ...track, @@ -237,6 +243,21 @@ export const findStreamsForTrack = (index: number) => async (dispatch, getState) } }; +function removeFirstStream(track: QueueItem, trackIndex: number, streams: TrackStream[], dispatch) { + if (streams.length === 1) { + // no more streams are available + dispatch(removeFromQueue(trackIndex)); + } else { + // remove the first (unavailable) stream + dispatch(updateQueueItem({ + ...track, + loading: true, + error: false, + streams: streams.slice(1) + })); + } +} + export function playTrack(streamProviders, item: QueueItem) { return (dispatch) => { dispatch(clearQueue()); From ff2fc0d170dcfcdbb2bfa17ff603c9bd05d92033 Mon Sep 17 00:00:00 2001 From: Cristan Beskid <596554+sferra@users.noreply.github.com> Date: Fri, 1 Mar 2024 00:55:40 +0100 Subject: [PATCH 2/9] Add the missing stream field to the test data --- .../containers/AlbumViewContainer/AlbumViewContainer.test.tsx | 1 + packages/app/test/storeBuilders.ts | 2 ++ 2 files changed, 3 insertions(+) diff --git a/packages/app/app/containers/AlbumViewContainer/AlbumViewContainer.test.tsx b/packages/app/app/containers/AlbumViewContainer/AlbumViewContainer.test.tsx index cc11e21f5d..aa9c0d2f22 100644 --- a/packages/app/app/containers/AlbumViewContainer/AlbumViewContainer.test.tsx +++ b/packages/app/app/containers/AlbumViewContainer/AlbumViewContainer.test.tsx @@ -203,6 +203,7 @@ describe('Album view container', () => { name: 'test track 1', streams: [{ data: 'test-stream-data', + stream: 'test-stream-url', author: { name: 'test author' } diff --git a/packages/app/test/storeBuilders.ts b/packages/app/test/storeBuilders.ts index d0db0f9e47..c6ff11c183 100644 --- a/packages/app/test/storeBuilders.ts +++ b/packages/app/test/storeBuilders.ts @@ -286,6 +286,7 @@ export const buildStoreState = () => { }])), getStreamForId: jest.fn(() => ({ data: 'test-stream-data', + stream: 'test-stream-url', author: { name: 'test author' } @@ -305,6 +306,7 @@ export const buildStoreState = () => { getStreamForId: jest.fn((id: string) => ({ id: 'test-stream-id-1', data: id, + stream: 'different-test-stream-url', source: 'Different Stream Provider' }) as TrackStream) } From 934c75424ee19a578c698505a7a56e270d6fda98 Mon Sep 17 00:00:00 2001 From: Cristan Beskid <596554+sferra@users.noreply.github.com> Date: Fri, 1 Mar 2024 01:05:03 +0100 Subject: [PATCH 3/9] Remove obsolete test data --- .../containers/AlbumViewContainer/AlbumViewContainer.test.tsx | 1 - packages/app/test/storeBuilders.ts | 4 ---- 2 files changed, 5 deletions(-) diff --git a/packages/app/app/containers/AlbumViewContainer/AlbumViewContainer.test.tsx b/packages/app/app/containers/AlbumViewContainer/AlbumViewContainer.test.tsx index aa9c0d2f22..a8acfbb324 100644 --- a/packages/app/app/containers/AlbumViewContainer/AlbumViewContainer.test.tsx +++ b/packages/app/app/containers/AlbumViewContainer/AlbumViewContainer.test.tsx @@ -202,7 +202,6 @@ describe('Album view container', () => { artist: 'test artist', name: 'test track 1', streams: [{ - data: 'test-stream-data', stream: 'test-stream-url', author: { name: 'test author' diff --git a/packages/app/test/storeBuilders.ts b/packages/app/test/storeBuilders.ts index c6ff11c183..7264d9db59 100644 --- a/packages/app/test/storeBuilders.ts +++ b/packages/app/test/storeBuilders.ts @@ -279,13 +279,11 @@ export const buildStoreState = () => { sourceName: 'Test Stream Provider', search: jest.fn(() => ([{ id: 'test-stream-id', - data: 'test-stream-data', author: { name: 'test author' } }])), getStreamForId: jest.fn(() => ({ - data: 'test-stream-data', stream: 'test-stream-url', author: { name: 'test author' @@ -297,7 +295,6 @@ export const buildStoreState = () => { sourceName: 'Different Stream Provider', search: jest.fn(() => ([{ id: 'test-stream-id-1', - data: 'different-test-stream-data', source: 'Different Stream Provider', author: { name: 'test author' @@ -305,7 +302,6 @@ export const buildStoreState = () => { }])), getStreamForId: jest.fn((id: string) => ({ id: 'test-stream-id-1', - data: id, stream: 'different-test-stream-url', source: 'Different Stream Provider' }) as TrackStream) From 59dd7bb1c9b74d9e4304441aed25fae33802b75e Mon Sep 17 00:00:00 2001 From: Cristan Beskid <596554+sferra@users.noreply.github.com> Date: Fri, 1 Mar 2024 01:16:49 +0100 Subject: [PATCH 4/9] Clean up code --- packages/app/app/actions/queue.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/app/app/actions/queue.ts b/packages/app/app/actions/queue.ts index f86abbb836..1f455cd2f9 100644 --- a/packages/app/app/actions/queue.ts +++ b/packages/app/app/actions/queue.ts @@ -211,7 +211,8 @@ export const findStreamsForTrack = (index: number) => async (dispatch, getState) const firstStream = streamData[0]; if (!firstStream?.stream) { - removeFirstStream(track, index, streamData, dispatch); + const remainingStreams = streamData.slice(1); + removeFirstStream(track, index, remainingStreams, dispatch); return; } @@ -243,8 +244,8 @@ export const findStreamsForTrack = (index: number) => async (dispatch, getState) } }; -function removeFirstStream(track: QueueItem, trackIndex: number, streams: TrackStream[], dispatch) { - if (streams.length === 1) { +function removeFirstStream(track: QueueItem, trackIndex: number, remainingStreams: TrackStream[], dispatch): void { + if (remainingStreams.length === 0) { // no more streams are available dispatch(removeFromQueue(trackIndex)); } else { @@ -253,7 +254,7 @@ function removeFirstStream(track: QueueItem, trackIndex: number, streams: TrackS ...track, loading: true, error: false, - streams: streams.slice(1) + streams: remainingStreams })); } } From 3de0b4dc26e075bb989375f66cbe6140c9bd6367 Mon Sep 17 00:00:00 2001 From: Cristan Beskid <596554+sferra@users.noreply.github.com> Date: Wed, 13 Mar 2024 22:31:07 +0100 Subject: [PATCH 5/9] Add test for the removal of tracks for which no stream URLs can be resolved --- .../PlayerBarContainer.test.tsx | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/packages/app/app/containers/PlayerBarContainer/PlayerBarContainer.test.tsx b/packages/app/app/containers/PlayerBarContainer/PlayerBarContainer.test.tsx index 44f8202531..5685881679 100644 --- a/packages/app/app/containers/PlayerBarContainer/PlayerBarContainer.test.tsx +++ b/packages/app/app/containers/PlayerBarContainer/PlayerBarContainer.test.tsx @@ -272,6 +272,53 @@ describe('PlayerBar container', () => { }); }); + it('should remove the track when no more stream URLs can be resolved', async () => { + const { component, store } = mountComponent({ + queue: { + currentSong: 0, + queueItems: [ + { + uuid: 'uuid1', + artist: 'test artist name', + name: 'track without streams' + } + ] + }, + plugin: { + plugins: { + streamProviders: [ + { + sourceName: 'Mocked Stream Provider', + search: jest.fn().mockResolvedValueOnce([ + { + id: 'stream 1 ID', + source: 'Mocked Stream Provider' + }, + { + id: 'stream 2 ID', + source: 'Mocked Stream Provider' + } + ]), + getStreamForId: jest.fn() + .mockResolvedValueOnce(null) + .mockResolvedValueOnce({ + stream: null, + source: 'Mocked Stream Provider' + }) + } + ] + }, + selected: { + streamProviders: 'Mocked Stream Provider' + } + } + }); + await waitFor(() => { + const state = store.getState(); + expect(state.queue.queueItems.length).toBe(0); + }); + }); + const mountComponent = (initialStore?: AnyProps) => { const store = configureMockStore({ ...buildStoreState() From 47353a1c0071a67786cb9a36eec2496e81b2b836 Mon Sep 17 00:00:00 2001 From: Cristan Beskid <596554+sferra@users.noreply.github.com> Date: Fri, 15 Mar 2024 00:57:44 +0100 Subject: [PATCH 6/9] Update stream lookup logic --- packages/app/app/actions/queue.ts | 30 ++++++++++++------- .../containers/PlayerBarContainer/hooks.ts | 27 ++++++++++++----- 2 files changed, 40 insertions(+), 17 deletions(-) diff --git a/packages/app/app/actions/queue.ts b/packages/app/app/actions/queue.ts index ed4ab1bd58..d32e49ac30 100644 --- a/packages/app/app/actions/queue.ts +++ b/packages/app/app/actions/queue.ts @@ -168,20 +168,26 @@ export const selectNewStream = (index: number, streamId: string) => async (dispa export const findStreamsForTrack = (index: number) => async (dispatch, getState) => { const {queue, settings}: RootState = getState(); - const track = queue.queueItems[index]; - if (track && !track.local && isEmpty(track.streams)) { - dispatch(updateQueueItem({ - ...track, - loading: true - })); + if (track && !track.local && trackHasNoFirstStream(track)) { + if (!track.loading) { + dispatch(updateQueueItem({ + ...track, + loading: true + })); + } const selectedStreamProvider = getSelectedStreamProvider(getState); try { - let streamData = await getTrackStreams( - track, - selectedStreamProvider - ); + let streamData: TrackStream[]; + if (isEmpty(track.streams)) { + streamData = await getTrackStreams( + track, + selectedStreamProvider + ); + } else { + streamData = track.streams; + } if (settings.useStreamVerification) { try { @@ -247,6 +253,10 @@ export const findStreamsForTrack = (index: number) => async (dispatch, getState) } }; +export function trackHasNoFirstStream(track: QueueItem): boolean { + return isEmpty(track?.streams) || isEmpty(track.streams[0].stream); +} + function removeFirstStream(track: QueueItem, trackIndex: number, remainingStreams: TrackStream[], dispatch): void { if (remainingStreams.length === 0) { // no more streams are available diff --git a/packages/app/app/containers/PlayerBarContainer/hooks.ts b/packages/app/app/containers/PlayerBarContainer/hooks.ts index 3e48d3c153..c21168d50d 100644 --- a/packages/app/app/containers/PlayerBarContainer/hooks.ts +++ b/packages/app/app/containers/PlayerBarContainer/hooks.ts @@ -14,12 +14,11 @@ import * as playerActions from '../../actions/player'; import * as queueActions from '../../actions/queue'; import * as settingsActions from '../../actions/settings'; import * as favoritesActions from '../../actions/favorites'; -import { favoritesSelectors } from '../../selectors/favorites'; +import { favoritesSelectors, getFavoriteTrack } from '../../selectors/favorites'; import { playerSelectors } from '../../selectors/player'; import { queue as queueSelector } from '../../selectors/queue'; import { settingsSelector } from '../../selectors/settings'; -import { getFavoriteTrack } from '../../selectors/favorites'; -import { QueueItem } from '../../reducers/queue'; +import { QueueItem, QueueStore } from '../../reducers/queue'; export const useSeekbarProps = () => { const dispatch = useDispatch(); @@ -255,12 +254,10 @@ export const useStreamLookup = () => { const queue = useSelector(queueSelector); useEffect(() => { - const isStreamLoading = Boolean(queue.queueItems.find((item) => item.loading)); - - if (!isStreamLoading) { + if (shouldSearchForStreams(queue)) { const currentSong: QueueItem = queue.queueItems[queue.currentSong]; - if (currentSong && isEmpty(currentSong.streams)) { + if (currentSong && queueActions.trackHasNoFirstStream(currentSong)) { dispatch(queueActions.findStreamsForTrack(queue.currentSong)); return; } @@ -273,3 +270,19 @@ export const useStreamLookup = () => { } }, [queue]); }; + +const shouldSearchForStreams = (queue: QueueStore): boolean => { + const currentlyLoadingTrack = queue.queueItems.find((item) => item.loading); + if (!currentlyLoadingTrack) { + // No track is currently "loading": start searching for steams. + return true; + } + if (isEmpty(currentlyLoadingTrack.streams)) { + // Streams are not yet resolved: this happens when the track is being marked as "loading" + // while still resolving streams. We don't need to start a search until that operation completes or fails. + return false; + } + const firstStream = currentlyLoadingTrack.streams[0]; + // A search should be performed if the first stream URL wasn't yet resolved. + return !firstStream?.stream; +}; From 9e55b6d2500b72a455d04c23a578ece7f902ddc3 Mon Sep 17 00:00:00 2001 From: Cristan Beskid <596554+sferra@users.noreply.github.com> Date: Fri, 15 Mar 2024 23:27:11 +0100 Subject: [PATCH 7/9] Don't attempt to resolve stream URLs unless they are missing --- packages/app/app/actions/queue.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/app/app/actions/queue.ts b/packages/app/app/actions/queue.ts index d32e49ac30..afb86e5aff 100644 --- a/packages/app/app/actions/queue.ts +++ b/packages/app/app/actions/queue.ts @@ -213,10 +213,12 @@ export const findStreamsForTrack = (index: number) => async (dispatch, getState) if (streamData?.length === 0) { dispatch(removeFromQueue(index)); } else { - streamData = [ - await selectedStreamProvider.getStreamForId(streamData.find(Boolean)?.id), - ...streamData.slice(1) - ]; + if (!streamData[0].stream) { + streamData = [ + await selectedStreamProvider.getStreamForId(streamData.find(Boolean)?.id), + ...streamData.slice(1) + ]; + } const firstStream = streamData[0]; if (!firstStream?.stream) { From 2fb9ab94e1b26639330fbc0d9dd61e9c93801365 Mon Sep 17 00:00:00 2001 From: Cristan Beskid <596554+sferra@users.noreply.github.com> Date: Fri, 15 Mar 2024 23:28:11 +0100 Subject: [PATCH 8/9] Add missing stream fields to the test data --- .../PlayerBarContainer.test.tsx | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/packages/app/app/containers/PlayerBarContainer/PlayerBarContainer.test.tsx b/packages/app/app/containers/PlayerBarContainer/PlayerBarContainer.test.tsx index 5685881679..56b491de39 100644 --- a/packages/app/app/containers/PlayerBarContainer/PlayerBarContainer.test.tsx +++ b/packages/app/app/containers/PlayerBarContainer/PlayerBarContainer.test.tsx @@ -129,7 +129,8 @@ describe('PlayerBar container', () => { streams: [{ duration: 300, title: 'test track 1', - skipSegments: [] + skipSegments: [], + stream: 'stream URL' }] }] }, @@ -154,7 +155,8 @@ describe('PlayerBar container', () => { streams: [{ duration: 300, title: 'test track 1', - skipSegments: [] + skipSegments: [], + stream: 'stream URL 1' }] }, { artist: 'test artist 2', @@ -162,7 +164,8 @@ describe('PlayerBar container', () => { streams: [{ duration: 300, title: 'test track 2', - skipSegments: [] + skipSegments: [], + stream: 'stream URL 2' }] }] }, @@ -187,7 +190,8 @@ describe('PlayerBar container', () => { streams: [{ duration: 300, title: 'test track 1', - skipSegments: [] + skipSegments: [], + stream: 'stream URL 1' }] }, { artist: 'test artist 2', @@ -200,7 +204,8 @@ describe('PlayerBar container', () => { duration: 10, endTime: 10, startTime: 0 - }] + }], + stream: 'stream URL 2' }] }] }, @@ -225,7 +230,8 @@ describe('PlayerBar container', () => { streams: [{ duration: 300, title: 'test track 1', - skipSegments: [] + skipSegments: [], + stream: 'stream URL' }] }] }, From ef5514749c3f69bf207f851f6ea6fde1cad7afc5 Mon Sep 17 00:00:00 2001 From: Cristan Beskid <596554+sferra@users.noreply.github.com> Date: Sat, 16 Mar 2024 17:50:49 +0100 Subject: [PATCH 9/9] Refactor for readability --- packages/app/app/actions/queue.test.ts | 6 ++-- packages/app/app/actions/queue.ts | 41 ++++++++++++++++---------- 2 files changed, 28 insertions(+), 19 deletions(-) diff --git a/packages/app/app/actions/queue.test.ts b/packages/app/app/actions/queue.test.ts index 24399aebf6..58a58602f6 100644 --- a/packages/app/app/actions/queue.test.ts +++ b/packages/app/app/actions/queue.test.ts @@ -6,16 +6,16 @@ describe('Queue actions tests', () => { describe('finds streams for track', () => { const getSelectedStreamProvider = jest.spyOn(QueueOperations, 'getSelectedStreamProvider'); - const getTrackStreams = jest.spyOn(QueueOperations, 'getTrackStreams'); + const resolveTrackStreams = jest.spyOn(QueueOperations, 'resolveTrackStreams'); afterEach(() => { getSelectedStreamProvider.mockReset(); - getTrackStreams.mockReset(); + resolveTrackStreams.mockReset(); }); test('remote track is removed from the queue when no streams are available', () => { // Mock an empty search result for streams. - getTrackStreams.mockResolvedValueOnce([]); + resolveTrackStreams.mockResolvedValueOnce([]); // Configure a dummy stream provider. It is not actually used in this execution path. getSelectedStreamProvider.mockReturnValueOnce({}); diff --git a/packages/app/app/actions/queue.ts b/packages/app/app/actions/queue.ts index afb86e5aff..3a05083963 100644 --- a/packages/app/app/actions/queue.ts +++ b/packages/app/app/actions/queue.ts @@ -63,7 +63,8 @@ export const getSelectedStreamProvider = (getState) => { return _.find(streamProviders, { sourceName: selected.streamProviders }); }; -export const getTrackStreams = async ( +// Exported to facilitate testing. +export const resolveTrackStreams = async ( track: Track | LocalTrack, selectedStreamProvider: StreamProvider ) => { @@ -179,15 +180,7 @@ export const findStreamsForTrack = (index: number) => async (dispatch, getState) } const selectedStreamProvider = getSelectedStreamProvider(getState); try { - let streamData: TrackStream[]; - if (isEmpty(track.streams)) { - streamData = await getTrackStreams( - track, - selectedStreamProvider - ); - } else { - streamData = track.streams; - } + let streamData: TrackStream[] = await getTrackStreams(track, selectedStreamProvider); if (settings.useStreamVerification) { try { @@ -213,12 +206,7 @@ export const findStreamsForTrack = (index: number) => async (dispatch, getState) if (streamData?.length === 0) { dispatch(removeFromQueue(index)); } else { - if (!streamData[0].stream) { - streamData = [ - await selectedStreamProvider.getStreamForId(streamData.find(Boolean)?.id), - ...streamData.slice(1) - ]; - } + streamData = await resolveSourceUrlForTheFirstStream(streamData, selectedStreamProvider); const firstStream = streamData[0]; if (!firstStream?.stream) { @@ -255,6 +243,27 @@ export const findStreamsForTrack = (index: number) => async (dispatch, getState) } }; +async function getTrackStreams(track: QueueItem, streamProvider: StreamProvider): Promise { + if (isEmpty(track.streams)) { + return resolveTrackStreams( + track, + streamProvider + ); + } + return track.streams; +} + +async function resolveSourceUrlForTheFirstStream(trackStreams: TrackStream[], streamProvider: StreamProvider): Promise { + if (isEmpty(trackStreams[0].stream)) { + return [ + await streamProvider.getStreamForId(trackStreams.find(Boolean)?.id), + ...trackStreams.slice(1) + ]; + } + // The stream URL might already be resolved, for example for a previously played track. + return trackStreams; +} + export function trackHasNoFirstStream(track: QueueItem): boolean { return isEmpty(track?.streams) || isEmpty(track.streams[0].stream); }