Remove fetch after upload #1848

cvolzke4 · 2020-05-19T01:26:47Z

Summary

Remove fetch after upload. Instead, retrieve the updated resource info from the response of the upload api call.

This solves two problems:

As a part of the larger epic to avoid re-renders, prevent the 'uploadMedia' function reference from being re-created when pagingNum changes.
fetch after upload uses the current 'pagingNum', which doesn't update the latest media if the gallery is scrolled down the page.

codecov · 2020-05-19T01:28:13Z

Codecov Report

Merging #1848 into master will increase coverage by 0.09%.
The diff coverage is 88.88%.

@@             Coverage Diff              @@
##             master    #1848      +/-   ##
============================================
+ Coverage     66.13%   66.22%   +0.09%     
  Complexity      314      314              
============================================
  Files           651      651              
  Lines         10768    10772       +4     
============================================
+ Hits           7121     7134      +13     
+ Misses         3647     3638       -9

Impacted Files	Coverage Δ	Complexity Δ
assets/src/edit-story/app/media/mediaProvider.js	`73.17% <ø> (ø)`	`0.00 <0.00> (ø)`
...t-story/app/media/useUploadMedia/useUploadMedia.js	`63.26% <88.88%> (+23.26%)`	`0.00 <0.00> (ø)`

github-actions · 2020-05-19T01:30:45Z

Size Change: +9 B (0%)

Total Size: 830 kB

Filename	Size	Change
`assets/js/edit-story.js`	372 kB	+9 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`assets/css/edit-story.css`	269 B	0 B
`assets/css/stories-dashboard.css`	305 B	0 B
`assets/css/web-stories-embed-block.css`	501 B	0 B
`assets/js/stories-dashboard.js`	442 kB	0 B
`assets/js/web-stories-embed-block.js`	15.7 kB	0 B

_{compressed-size-action}

spacedmonkey · 2020-05-20T11:18:15Z

The fetch after upload was added for two reasons.

Why video poster is generated and uploaded, we needed up force the media library to reload. But I believe this has since been fixed.
If images have been uploaded in the WP media dialog, we need to force this to reload.

@cvolzke4 you need to take into account this case in this PR, I believe.

cvolzke4 · 2020-05-21T06:44:11Z

The fetch after upload was added for two reasons.

Why video poster is generated and uploaded, we needed up force the media library to reload. But I believe this has since been fixed.

If images have been uploaded in the WP media dialog, we need to force this to reload.

@cvolzke4 you need to take into account this case in this PR, I believe.

In these changes, the return value from 'await uploadFile(localFile.file)' is used to update the media directly. I tested this by drag/drop of an image into the media pane, and checked that the attributes are consistent with a server reload.

When using the 'Upload' button to upload into WP, the media pane is reloaded elsewhere, but it's still reloaded as expected.

spacedmonkey · 2020-05-21T14:43:16Z

Whenever I try and upload an image via drag and drop or pasting, I get this error.

assets/src/edit-story/app/media/useUploadMedia/useUploadMedia.js

spacedmonkey

Even with the new Map fix, this PR didnt work for me. The progress bar on the upload remained forever and was never replaced with uploaded version.

cvolzke4 · 2020-05-21T15:00:46Z

Whenever I try and upload an image via drag and drop or pasting, I get this error.

My bad, I thought I was working in dart again :)
Fixed.

spacedmonkey · 2020-05-21T15:04:46Z

How does this PR work when a filter is active?
Case.

User search for cats
User filter media library by video.
User uploads a image called. dog.jpg.

Currently this just add the image to the top of the list, is that correct? I am not sure how we work around this, unless if does the refetch if a filter or search is set overwise, just adds to the top?

spacedmonkey · 2020-05-21T15:11:29Z

Still seeing the progress bar forever more.

cvolzke4 · 2020-05-21T15:23:16Z

Even with the new Map fix, this PR didnt work for me. The progress bar on the upload remained forever and was never replaced with uploaded version.

I was too fast to reply. This has been fixed, please try again.

spacedmonkey · 2020-05-21T15:32:20Z

Also worthing noting that seems like force pushing here to rewrite commits. Please avoid doing this, as making pulling your change hard as I have to delete the branch and fetch it again.

cvolzke4 · 2020-05-21T15:37:59Z

How does this PR work when a filter is active?
Case.

User search for cats

User filter media library by video.

User uploads a image called. dog.jpg.

Currently this just add the image to the top of the list, is that correct? I am not sure how we work around this, unless if does the refetch if a filter or search is set overwise, just adds to the top?

It's a good question, this may be a case for re-fetching (or re-apply the search in memory, if that's easy to do). Another way to look at it is as a 'feature', so that the user can see what they just uploaded. It may appear to be a glitch if an upload finishes, and then the image (or images) suddenly disappear.

When a search is active inside the WP uploader, the image isn't displayed, but image details appear on a pane to the right of the list (the last uploaded image if multiple were dropped).

It may not matter very much, as the user can re-enter the search term to re-apply the filter if needed.

This sounds like a question for UX, WDYT?

cvolzke4 · 2020-05-21T15:43:44Z

Also worthing noting that seems like force pushing here to rewrite commits. Please avoid doing this, as making pulling your change hard as I have to delete the branch and fetch it again.

Thanks for letting me know. I was under the impression that this only affects a pull if another contributor is making changes to the same branch.

cvolzke4 · 2020-05-21T15:46:56Z

Still seeing the progress bar forever more

I don't think this is the updated version, try refreshing the browser.

spacedmonkey · 2020-05-21T16:02:21Z

Changes like this really need a ticket first and a the solution to be discussed before any code is written. If that was the case I would have pointed out the pitfalls and why we had to be the refetch in the first place.

Can we create a ticket so this work can be tracked.
Confirm UX after upload with Sam.
Write a set of tests, that proves this doesn't break anything. A change this wide reaching will likely have breakages.

We need to validate that this doesn't break the 5 kinds of upload

Upload via drag into media library.
Upload via drag into canvas.
Upload via paste ( only works on Mac OS )
Upload via WP media modal.
Upload via poster generation.

cvolzke4 · 2020-05-21T16:46:28Z

Changes like this really need a ticket first and a the solution to be discussed before any code is written. If that was the case I would have pointed out the pitfalls and why we had to be the refetch in the first place.

Can we create a ticket so this work can be tracked.

Confirm UX after upload with Sam.

Write a set of tests, that proves this doesn't break anything. A change this wide reaching will likely have breakages.

We need to validate that this doesn't break the 5 kinds of upload

Upload via drag into media library.

Upload via drag into canvas.

Upload via paste ( only works on Mac OS )

Upload via WP media modal.

Upload via poster generation.

Thanks Jonny.

This change relates to the performance ticket (which I've linked now, I should have done this earlier). I'll raise another ticket Monday to address the specific change. I had started this PR as a way to communicate this in a comment elsewhere, and then finished the implementation since it wasn't very much work and I could receive feedback on the approach, which you have given (thank you). I'll write some new tests once the behavior has been confirmed with Sam.

Please bear with me while we find our feet working with github, linking issues, getting feedback etc, as you can understand it's a very different environment to what we are used to.

I've just tested the upload types that you mentioned, including upload of multiple files.

There's a few other issues that I noticed while testing:

fetchMedia depends on pagingNum, which causes re-renders during scrolling (the issue I'm trying to fix).
Uploading a video doesn't focus the 'Video' tab, but it probably should.
fetchMedia uses pagingNum to load the last page of results, and prepends them to the list of media. This change would also fix that.
Pasting an mp4 video into the canvas pastes the filename, but not the content.
Uploading a video into the canvas froze chrome (appears to be a once off, I can no longer reproduce it).

dvoytenko · 2020-05-21T18:44:29Z

assets/src/edit-story/app/media/useUploadMedia/useUploadMedia.js

+            fileUploaded,
+          ])
+        );
+        setMedia({


Seems like reshuffling the whole array of media elements is a lot. I think we already have updateMediaElement. Can we use it here?

As it is today, the reducer for updateMediaElement is also O(n) for each media element. But we should be able to refactor it to take more than one media element at a time.

Here is an issue to address this: #2072

dvoytenko · 2020-05-21T18:45:06Z

assets/src/edit-story/app/media/useUploadMedia/useUploadMedia.js

-            ...media,
-          ],
-        });
+        updatedMedia = [


Would it make sense to have instead a targeted method prependMedia and resolve array logic and stuff on the reducer level?

That would be a better way, this is an existing issue that can be addressed in a separate PR. I've raised an issue: #2072

spacedmonkey · 2020-05-22T13:25:44Z

Honestly, there are still a lot of question regarding the UX of the media library about expected behavior. Like

What you are scrolled down say 200th item in the media list, does the preview appear at the top of the list?
Should a preview should for a video when image has been filtered.
What the preview upload appear at all if a search is active?
Are the any cases that where the filters and search need to be reset?

I don't think that we can work with the idea that current UX is correct as most of it was written before any of the current designs were implemented.

Until UX is confirmed, is hard to know if this PR is doing that correct behavior.

…d to uploadVideoPoster

dvoytenko · 2020-06-03T22:06:57Z

@cvolzke4 trying to catch up on this. is this ready to be rereviewed? I'm generally LG with changes, but there's a longish threads with other reviewers...

cvolzke4 · 2020-06-03T23:44:50Z

@cvolzke4 trying to catch up on this. is this ready to be rereviewed? I'm generally LG with changes, but there's a longish threads with other reviewers...

Thanks Dima, I was going to ping you about this today :)
UX has approved this approach (see #1971), so this PR is ready for review.

spacedmonkey

LGTM

googlebot added the cla: yes label May 19, 2020

cvolzke4 mentioned this pull request May 19, 2020

Remove recursive useMedia usage in useUploader #1799

Merged

cvolzke4 changed the title ~~Remove fetch after upload.~~ Remove fetch after upload May 21, 2020

cvolzke4 force-pushed the fix/remove-fetch-after-upload branch from 549e054 to 714f10c Compare May 21, 2020 06:15

cvolzke4 marked this pull request as ready for review May 21, 2020 06:44

cvolzke4 requested a review from spacedmonkey as a code owner May 21, 2020 06:44

spacedmonkey reviewed May 21, 2020

View reviewed changes

assets/src/edit-story/app/media/useUploadMedia/useUploadMedia.js Outdated Show resolved Hide resolved

spacedmonkey suggested changes May 21, 2020

View reviewed changes

cvolzke4 force-pushed the fix/remove-fetch-after-upload branch from 714f10c to af63983 Compare May 21, 2020 14:59

cvolzke4 requested a review from spacedmonkey May 21, 2020 15:01

Remove fetch after upload

0c63ac0

cvolzke4 force-pushed the fix/remove-fetch-after-upload branch from af63983 to 0c63ac0 Compare May 21, 2020 15:21

cvolzke4 linked an issue May 21, 2020 that may be closed by this pull request

Improve render / state update performance when loading media gallery images #1466

Closed

dvoytenko suggested changes May 21, 2020

View reviewed changes

cvolzke4 mentioned this pull request May 25, 2020

Consider avoiding re-fetch of media after upload #1971

Closed

cvolzke4 linked an issue May 25, 2020 that may be closed by this pull request

Consider avoiding re-fetch of media after upload #1971

Closed

cvolzke4 mentioned this pull request May 28, 2020

Remove dependency on state changes from action fns, by using useRef #1729

Merged

cvolzke4 added 3 commits May 28, 2020 18:11

Move processing and processed from media context state to a ref scope…

cdf472a

…d to uploadVideoPoster

Undo previous commit that was pushed by mistake

47c045d

Add a test for updating media resources from the upload rpc response

658d719

cvolzke4 requested a review from swissspidy as a code owner June 1, 2020 07:13

cvolzke4 requested a review from dvoytenko June 2, 2020 01:42

Merge branch 'master' into fix/remove-fetch-after-upload

062bd68

Merge branch 'master' into fix/remove-fetch-after-upload

50a278b

dvoytenko approved these changes Jun 4, 2020

View reviewed changes

swissspidy added Group: Media Type: Enhancement New feature or improvement of an existing feature labels Jun 5, 2020

spacedmonkey approved these changes Jun 5, 2020

View reviewed changes

Merge branch 'master' into fix/remove-fetch-after-upload

cec3916

cvolzke4 merged commit 62f9c8d into master Jun 10, 2020

cvolzke4 deleted the fix/remove-fetch-after-upload branch June 10, 2020 09:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove fetch after upload #1848

Remove fetch after upload #1848

cvolzke4 commented May 19, 2020 •

edited

codecov bot commented May 19, 2020 •

edited

github-actions bot commented May 19, 2020 •

edited

spacedmonkey commented May 20, 2020

cvolzke4 commented May 21, 2020 •

edited

spacedmonkey commented May 21, 2020

spacedmonkey left a comment

cvolzke4 commented May 21, 2020 •

edited

spacedmonkey commented May 21, 2020

spacedmonkey commented May 21, 2020

cvolzke4 commented May 21, 2020

spacedmonkey commented May 21, 2020

cvolzke4 commented May 21, 2020 •

edited

cvolzke4 commented May 21, 2020

cvolzke4 commented May 21, 2020 •

edited

spacedmonkey commented May 21, 2020

cvolzke4 commented May 21, 2020 •

edited

dvoytenko May 21, 2020

cvolzke4 Jun 3, 2020

dvoytenko May 21, 2020

cvolzke4 May 28, 2020 •

edited

spacedmonkey commented May 22, 2020

dvoytenko commented Jun 3, 2020

cvolzke4 commented Jun 3, 2020

spacedmonkey left a comment

Remove fetch after upload #1848

Remove fetch after upload #1848

Conversation

cvolzke4 commented May 19, 2020 • edited

Summary

codecov bot commented May 19, 2020 • edited

Codecov Report

github-actions bot commented May 19, 2020 • edited

spacedmonkey commented May 20, 2020

cvolzke4 commented May 21, 2020 • edited

spacedmonkey commented May 21, 2020

spacedmonkey left a comment

Choose a reason for hiding this comment

cvolzke4 commented May 21, 2020 • edited

spacedmonkey commented May 21, 2020

spacedmonkey commented May 21, 2020

cvolzke4 commented May 21, 2020

spacedmonkey commented May 21, 2020

cvolzke4 commented May 21, 2020 • edited

cvolzke4 commented May 21, 2020

cvolzke4 commented May 21, 2020 • edited

spacedmonkey commented May 21, 2020

cvolzke4 commented May 21, 2020 • edited

dvoytenko May 21, 2020

Choose a reason for hiding this comment

cvolzke4 Jun 3, 2020

Choose a reason for hiding this comment

dvoytenko May 21, 2020

Choose a reason for hiding this comment

cvolzke4 May 28, 2020 • edited

Choose a reason for hiding this comment

spacedmonkey commented May 22, 2020

dvoytenko commented Jun 3, 2020

cvolzke4 commented Jun 3, 2020

spacedmonkey left a comment

Choose a reason for hiding this comment

cvolzke4 commented May 19, 2020 •

edited

codecov bot commented May 19, 2020 •

edited

github-actions bot commented May 19, 2020 •

edited

cvolzke4 commented May 21, 2020 •

edited

cvolzke4 commented May 21, 2020 •

edited

cvolzke4 commented May 21, 2020 •

edited

cvolzke4 commented May 21, 2020 •

edited

cvolzke4 commented May 21, 2020 •

edited

cvolzke4 May 28, 2020 •

edited