Fix hanging playback when no stream is available

[sferra]

Addresses #1560.

I can add test coverage for the new code if this solution is generally accepted.

[nuki-chan]

Lines 209-215

Oh, brave soul, marching into the battlefield of splicing arrays! But watch out, you just introduced a magic trick in your code. 🧙‍♀️✨

const firstStream = streamData[0];
if (!firstStream?.stream) {
  removeFirstStream(track, index, streamData, dispatch);
  return;
}

Here's what's happening:

• You create firstStream from streamData[0], and then
• You check if firstStream.stream exists and if not,
• You call removeFirstStream.

Nuki sees what you did there. Trying to avoid duplication? Kawaii, but it could be more elegant. Also, since findStreamsForTrack is an async function, why not use the full power of async/await for better readability?

Consider this glittering refactored snippet ✨:

// If there's no valid stream in the first element, handle appropriately
const [firstStream, ...remainingStreams] = streamData;
if (!firstStream?.stream) {
  await handleInvalidFirstStream(track, index, remainingStreams, dispatch);
  return;
}

And then, Nuki recommends renaming removeFirstStream to handleInvalidFirstStream and making that function async so you can sprinkle some more async/await sugar over your code. ✨

---

Lines 237-252

Oh my, an anonymous function grew up and became a real boy! Or function, in this case. 🎉

function removeFirstStream(track: QueueItem, trackIndex: number, streams: TrackStream[], dispatch) {
  // Code here
}

Here's Nuki's checklist:

• Documentation: You didn't leave any comments! How dare you! (╯°□°）╯︵ ┻━┻ Nuki says, add some docs to let others know why this function is so special!
• Function Name: As mentioned before, handleInvalidFirstStream could be a more descriptive name.
• Code Comments: There are sneaky little comments in there, talking about a // return removeTrackAction(trackIndex); line that's commented out. That's a no-no, clean it up!
• Return Type: Is this function a ghost 👻? It haunts without a trace since it has no return type. Spook it back into the realm of the typed with a proper return type!

And now, spice it up with async flavor:

async function handleInvalidFirstStream(track: QueueItem, trackIndex: number, remainingStreams: TrackStream[], dispatch) {
  // Add some cute and helpful comments here
  if (remainingStreams.length === 0) { // Look ma, no magic number! 🎩
    await dispatch(removeFromQueue(trackIndex));
  } else {
    await dispatch(updateQueueItem({
      ...track,
      loading: true,
      error: false,
      streams: remainingStreams // More descriptive variable name, yay!
    }));
  }
}

Remember, code without tests is just a ticking time bomb! ⏰💣 Make sure you defuse it by writing tests for this new functionality. Nuki demands it! 🧪🔬

Keep coding, keep refining, and let's make this code as kawaii as possible! (づ｡◕‿‿◕｡)づ

[codecov]

Codecov Report

Attention: Patch coverage is 66.66667% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 71.99%. Comparing base (cb07c40) to head (186a31a).

Files	Patch %	Lines
packages/app/app/actions/queue.ts	68.18%	1 Missing and 6 partials ⚠️
...ges/app/app/containers/PlayerBarContainer/hooks.ts	63.63%	0 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1562      +/-   ##
==========================================
- Coverage   72.02%   71.99%   -0.03%     
==========================================
  Files         362      362              
  Lines        6698     6721      +23     
  Branches      484      493       +9     
==========================================
+ Hits         4824     4839      +15     
- Misses       1477     1478       +1     
- Partials      397      404       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nukeop]

Thanks, looks like I missed the data->stream change when I was refactoring this some time ago. This is an alright solution to the problem, and we should go ahead with this PR.

[sferra]

Sorry it's taking so long, but I don't have much time and have quite some issues getting the tests green, even if the change (coincidentally) works in practice. Failing to resolve stream URLs causes an avalanche of updates to the queue, also the current logic for searching streams is hard to test and possibly not adequate for such cases. I might have to make additional changes, a thing which I initially tried to avoid. I would be grateful for any advice on how to move this PR forward, @nukeop .

[nukeop]

No problem, maybe it'll be easier if I do it for you?

[sferra]

I don't mind if you take over.

I assumed that PlayerBarContainer hook useStreamLookup should to be called each time an invalid track stream is removed. This might have been incorrect.

Here's what I found so far:

• In the PlayerBarContainer hooks, useStreamLookup, there's a check which prevents state propagation (!isStreamLoading) when removing invalid track streams (the reason the test is currently failing).
• Unconditionally dispatching "track is loading" events while removing invalid track streams creates event loops. Adding a simple check for not re-dispatching "track is loading" prevents the loops.
• We should probably not attempt to resolve the tracks again if we remove an invalid stream.

Please let me know what you find out, even if it's not the final solution.

[sferra]

Here's my local WIP state if you are curious and want to take a look: sferra#1

BTW, local tests for the app module seem to run indefinitely, even for "master". I don't know why. Do you have a hint?

Edit:
Fixed the tests by adding the missing stream fields.
The WIP PR works and I could merge it into this PR if you agree to the changes, @nukeop.

[nukeop]

Ok, please go ahead. And I might add some sort of a check for missing fields so that these actions fail instead of hanging.

[nukeop]

I see that you added me to your fork as a collaborator. But actually, you only have to open a PR here, and Github will let me push directly to your branch (unless you explicitly forbid it). I'll take a closer look at this today.

[sferra]

Thanks, for the feedback and assistance. There's no hurry from my side.
I'm slow to progress these PRs myself, since I have so little time.

Regarding the fork, I added you as a collaborator to the fork so you can comment and review the linked "WIP PR" more easily. The PR (which is now merged into this one) was within the fork, against the branch of this PR so it contained just the diff to this PR. I just wanted to provide you with a convenient way to verify those changes.

Regarding this PR, I suppose that I should add unit test coverage to the functions I extracted, since the coverage didn't increase with this PR. The component tests cover just specific use cases and it's not possible to cover all combination of execution paths with that type of test. I understand that you prefer the component tests, but IMO it's better to have both. Unit tests help a lot in preventing regressions when making bigger changes or refactoring. They also keep the code tested as it moves between the use cases. And I don't mind writing them. : )

[nukeop]

I'm fine with this amount of coverage to be honest. The functionality you added is covered. Do you want to add anything else, or can this be merged?

[sferra]

You can merge if you feel it's safe enough. Thanks.

[nukeop]

Awesome, thanks for contributing.