Merge pull request #1562 from sferra/fix/hanging-playback-when-no-str…

…eam-available

Fix hanging playback when no stream is available

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({});

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
 ) => {
@@ -168,20 +169,18 @@ 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[] = await getTrackStreams(track, selectedStreamProvider);
 
       if (settings.useStreamVerification) {
         try {
@@ -207,10 +206,14 @@ 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)
-        ];
+        streamData = await resolveSourceUrlForTheFirstStream(streamData, selectedStreamProvider);
+
+        const firstStream = streamData[0];
+        if (!firstStream?.stream) {
+          const remainingStreams = streamData.slice(1);
+          removeFirstStream(track, index, remainingStreams, dispatch);
+          return;
+        }
 
         dispatch(
           updateQueueItem({
@@ -240,6 +243,46 @@ export const findStreamsForTrack = (index: number) => async (dispatch, getState)
   }
 };
 
+async function getTrackStreams(track: QueueItem, streamProvider: StreamProvider): Promise<TrackStream[]> {
+  if (isEmpty(track.streams)) {
+    return resolveTrackStreams(
+      track,
+      streamProvider
+    );
+  }
+  return track.streams;
+}
+
+async function resolveSourceUrlForTheFirstStream(trackStreams: TrackStream[], streamProvider: StreamProvider): Promise<TrackStream[]> {
+  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);
+}
+
+function removeFirstStream(track: QueueItem, trackIndex: number, remainingStreams: TrackStream[], dispatch): void {
+  if (remainingStreams.length === 0) {
+    // no more streams are available
+    dispatch(removeFromQueue(trackIndex));
+  } else {
+    // remove the first (unavailable) stream
+    dispatch(updateQueueItem({
+      ...track,
+      loading: true,
+      error: false,
+      streams: remainingStreams
+    }));
+  }
+}
+
 export function playTrack(streamProviders, item: QueueItem) {
   return (dispatch) => {
     dispatch(clearQueue());

packages/app/app/containers/AlbumViewContainer/AlbumViewContainer.test.tsx

@@ -202,7 +202,7 @@ describe('Album view container', () => {
           artist: 'test artist',
           name: 'test track 1',
           streams: [{
-            data: 'test-stream-data',
+            stream: 'test-stream-url',
             author: {
               name: 'test author'
             }

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,15 +155,17 @@ describe('PlayerBar container', () => {
           streams: [{
             duration: 300,
             title: 'test track 1',
-            skipSegments: []
+            skipSegments: [],
+            stream: 'stream URL 1'
           }]
         }, {
           artist: 'test artist 2',
           name: 'test track 2',
           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'
           }]
         }]
       },
@@ -272,6 +278,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()

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;
+};

packages/app/test/storeBuilders.ts

@@ -279,13 +279,12 @@ 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'
                   }
@@ -296,15 +295,14 @@ 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'
                   }
                 }])),
                 getStreamForId: jest.fn((id: string) => ({
                   id: 'test-stream-id-1',
-                  data: id,
+                  stream: 'different-test-stream-url',
                   source: 'Different Stream Provider'
                 }) as TrackStream)
               }