Fix #8614: Playlist auto download not working

[ahmed-shehata]

Summary of Changes

This pull request fixes #8614

Current state

It seems that PlaylistItem.updateCache fails to find the playlist item stored in CoreData by uuid, this is due to the uuid getting changed after initially being added to the playlist.

1. PlaylistItem.addItem gets called with correct initial uuid (item.tagId)
2. PlaylistItem.updateItem is triggered (possibly by a script?), overrides the stored item.uuid in CoreData
3. Video file gets downloaded
4. PlaylistItem.updateCache gets called, trying to link the cachedData using the original uuid, doesn't find, silently fails.
5. Video file still exists, however not linked to any PlaylistItem (dangling disk space)
6. User attempts to download manually by clicking download in the playlist table view
7. Another Video file gets downloaded (duplicate, old one stays), this time the 2nd (updated) uuid is used, so the video file and playlist get linked successfully.

This PR

I've added another check using pageSrc to attempt to find the PlaylistItem in case the uuid check fails. This allows the video file to be linked successfully to the PlaylistItem

Submitter Checklist:

• [x] Unit Tests are updated to cover new or changed functionality
• [x] User-facing strings use NSLocalizableString()
• [x] New or updated UI has been tested across:
  • [x] Light & dark mode
  • [x] Different size classes (iPhone, landscape, iPad)
  • [x] Different dynamic type sizes

Test Plan:

1. Open Youtube.com
2. Open a video
3. Click on add to playlist
4. Video will now download properly

Screenshots:

https://github.com/brave/brave-ios/assets/4982099/8d9d843a-9767-44f6-9e07-f54023844a28

Reviewer Checklist:

• [ ] Issues include necessary QA labels:
  • QA/(Yes|No)
  • bug / enhancement
• [ ] Necessary security reviews have taken place.
• [ ] Adequate unit test coverage exists to prevent regressions.
• [ ] Adequate test plan exists for QA to validate (if applicable).
• [ ] Issue and pull request is assigned to a milestone (should happen at merge time).

[Brandon-T]

Hi, thanks for the changes, but where did you notice the uuid being changed? I checked downloading and it works during my tests.

Also: https://github.com/brave/brave-ios/blob/development/Sources/Data/models/PlaylistItem.swift

Where it has UpdateItem, it first checks if the item exists before it ever updates it. It does a check both by pageSrc and UUID, so it covers the case where UUID could change (such as on YouTube).

In the downloaders, when a download cannot be linked to CoreData, it calls "cleanup" and deletes the downloaded file and throws an error for the user.

Just curious how you ran into such an issue.

[ahmed-shehata]

Thanks for the review,

I checked downloading and it works during my tests.

I have found that the very first video always works fine, could you possibly try out adding multiple videos at once to see if you can repro? I have also added a video in the issue body: https://github.com/brave/brave-ios/issues/8614

it first checks if the item exists before it ever updates it.

True, however it does eventually replace the uuid in this line: https://github.com/brave/brave-ios/blob/development/Sources/Data/models/PlaylistItem.swift#L276

Which is causing the problem, since the updateCache method is called with the old (now replaced) uuid, doesn’t find it, leaves the file dangling (Application Support/Library/Playlists still has the mp4 file even if you remove the playlist item)

[Brandon-T]

updateCache

Ahh I see what you mean. The downloading still works for me with multiple items, but I can see it having a bug if the ID changes before the download completes, which you've indicated. It shouldn't be the case though as the ID usually changes on page refresh and only if the page modifies history via the Javascript (Youtube does this a lot). It is also possible if the page itself does some weird changes to the video node/tag after the download already triggered, awful bug :( I believe on every launch we do iterate the folder and check for dangling file to delete, so shouldn't be any after re-launch if all is working well.

Good find and fix! Thanks a lot :)