Fix media uploads error handling

[kean]

Part of #21190.

Fixes missing video upload limits reported by @momo-ozawa here. The fix requires a lot of changes because I found multiple other production issues on the Media screen that had to be fixed.

@osullivanchris, if you have a better idea of how to handle this scenario, given the technical limitations, please let me know. It may also be worth re-visiting this entire flow in the scope of the Media changes.

Handling Upload Limits

One of the advantages of using a custom picker is that it allows us to support scenarios like the upload limits. The custom picker displays the errors in the picker itself (recording.mp4).

Unfortunately, it wasn't possible with PHPickerViewController. I implemented it by handling this as other export errors (recording.mp4)

To implement it, I added duration limit checks to MediaVideoExporter. It was supposed to just work, but I encountered a few issues I had to fix.

1. Fix slow exports from NSItemProvider (0376ca1)

The NSItemProvider returned by PHPickerViewController doesn't return the video duration – you have to load the file first. But loadFileRepresentation was too slow: it was taking more than a minute for my 500 MB video. The reason was transcoding.

I followed the advice of an Apple engineer to disable transcoding on PHPickerViewController. The loadFileRepresentation was now taking around 200 ms. It’s the correct course of action because MediaVideoExporter.swift also performs its own transcoding. We definitely don’t want to do it twice.

2. Fix an issue with MediaImportService not reporting errors (13285a3)

MediaImportService was incorrectly calling a completion closure with .success on export failures. The errors were never reported to the Media screen.

3. Fix invalid localizedDescription of exporter errors (c832575)

Before -> After

4. Fix incorrect error banner displayed when upload fails c144f66

Before -> After

To test:

• Add a long video (5 minutes to Photos)
• Select a site with a free plan
• Open Media and try uploading the video
• Verify that the error is shown

Ideally we also need to re-run the tests from #21343

Regression Notes

1. Potential unintended areas of impact: Media
2. What I did to test those areas of impact (or what existing automated tests I relied on): manual
3. What automated tests I added (or what prevented me from doing so): n/a

PR submission checklist:

• I have completed the Regression Notes.
• I have considered adding unit tests for my changes.
• I have considered adding accessibility improvements for my changes.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

• Portrait and landscape orientations.
• Light and dark modes.
• Fonts: Larger, smaller and bold text.
• High contrast.
• VoiceOver.
• Languages with large words or with letters/accents not frequently used in English.
• Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
• iPhone and iPad.
• Multi-tasking: Split view and Slide over. (iPad)

[wpmobilebot]

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr21411-4a2e176
	Version	23.1
	Bundle ID	com.jetpack.alpha
	Commit	4a2e176
	App Center Build	jetpack-installable-builds #5909

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[wpmobilebot]

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr21411-2f0611c
	Version	23.1
	Bundle ID	org.wordpress.alpha
	Commit	2f0611c
	App Center Build	WPiOS - One-Offs #6863

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[kean]

It was basically testing configuration, repeating the logic one-to-one – not very useful.
Since I removed the blog.canUploadAsset variant the test was using, I decided to remove the test as well.
I'm planning to add some more tests for Media features as I start working on the screen itself.

[momo-ozawa]

• When I add a long video via the Media screen, then tap "retry" on the notice, the video is automatically deleted. However, tapping "retry" on the media thumbnail brings up the action sheet. Wdyt of showing the action sheet when tapping "retry" on the notice for consistency?
• On the action sheet, "cancel upload" and "delete" both seem to delete the video. Should we show only one option, if they both do the same thing?
• When adding a long video via the video block, you get a "failed to insert media" message. If you tap on it, an action sheet comes up. If you tap "retry", nothing seems to happen - and the action sheet doesn't come up again when you tap the video block.

[osullivanchris]

It feels like we are really bending the limits of what these components are intended for. Its a bit of a strange UX. I mean, already, not just what we are adding.

My first idea was that for the upload limit, its not really an error. Its more of a flow/feature. We could be more proactive and show something like a sheet which informs the user about what happens. Potentially even with an upsell, if they are willing to subscribe to upload larger videos.

e1)

The second idea I had was that we are really limiting ourselves by trying to show an error within the grid. What if we didn't show an image within the tiled grid until it finished uploading. Provides us a space to show actual information about an unsuccessful upload and what actions to take, rather than having to squeeze info into snackbar messages, on top of the tiles, or in the action sheet heading.

Maybe these ideas are too large. I don't think you're making things worse with the approach you've taken. It's following the current convention. So I'm ok if we just want to do that. Maybe only show one negative action as @momo-ozawa mentioned if they do the same thing.

[kean]

Wdyt of showing the action sheet when tapping "retry" on the notice for consistency?

Good catch with the notice view. I didn't check what it does.
There is no "Retry" at this point. The only thing you can do is delete the media item. I'll see how I can fix it.

My first idea was that for the upload limit, its not really an error.

Absolutely. Ideally, it should give you a way to upgrade and automatically resume the upload. But it will require some significant work.

Maybe these ideas are too large.

I'd suggest considering these changes in the scope of the other Media work. This PR's goal is to address the immediate issue of bringing the native picker integration to a state where we can ship it.

Again, we could take some inspiration from some of the native apps.

[kean]

Thanks again for bringing up these issues @momo-ozawa.

I reviewed more of this code. There are two technical limitations/factors to consider:

• The notices are handled by the MediaCoordinator, which is global and has no connection to the media screen
• The "retry" action is only applicable when the export is successful (localURL != nil). MediaCoordinator will not try to re-export the asset, and in most/all cases there is no point to.

Wdyt of showing the action sheet when tapping "retry" on the notice for consistency?

Given the technical limitations, I decided to not show the "Retry" button in this scenario (screenshot).

Should we show only one option, if they both do the same thing?

I removed the "Delete" action (screenshot).

the action sheet doesn't come up again when you tap the video block.

I removed the "Retry" action. It's not consistent with the Media screen, and tapping other actions seems to work the way you would expect (screenshot).

P.S. I also want to add Sentry error tracking for export errors to monitor it in the upcoming release, but that's going to be a separate PR.

[momo-ozawa]

Verified

• "retry" button isn't shown in the notices
• "delete" action has been removed
• "retry" action has been removed for video block

LGTM! 🚀

[momo-ozawa]

TIL allSatisfy 👀