Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load Google Drive / OneDrive lists 5-10x faster & always load all files #4513

Merged
merged 20 commits into from
Jul 10, 2023

Conversation

Murderlon
Copy link
Member

@Murderlon Murderlon commented Jun 21, 2023

Supersedes #4480

Problem

Users using Google Drive with massive folders (10K files) only see 100 files. They then have to endlessly scroll to load more files. Filtering may also not work as expected, as you it won't filter using the API, only on the loaded files, so you are forced to endlessly scroll.

Solution

  • GoogleDrive: increase to 1000 from limited 100 (by splitting user info call and list call, allowing for higher page sizes)
  • OneDrive: increase to 1000 from default 200.
  • Always load all files when entering a folder. Note that the 90% use case, which is what we should optimise for, is people with a moderate amount of files per folder. In this case waiting a little bit longer outweighs the complexity and added problems mentioned below.
  • Lazy load image thumbnails
  • Abort request when closing the panel or uppy. A new (internal) event dashboard:close-panel was required for this as there was no other way to know when "cancel" has been clicked.

Alternatives

Implementing a "load all files and folders" button. This brings along a whole new series of problems:

  • We would have to indicate more clearly in the UI that filtering is only on loaded files
  • Every time a new request comes in, files/folders are added and client-side sorting kicks in, messing up your scroll position and confusing the user where they are.
  • Designing a new state to indicate new files are gradually added. Potentially scroll blocking to avoid confusion.
  • Designing a new state where a new button is added.
  • Designing a new state where a new button is added together with the "select" and "cancel" buttons.

Instead we can greatly simplify the UX and code by always loading all files. Filtering works perfectly and as expected too.

@Murderlon Murderlon requested review from mifi, arturi and aduh95 June 21, 2023 11:33
@arturi
Copy link
Contributor

arturi commented Jun 21, 2023

I wonder who found out about splitting the permissions call and the list call ;-)

I do like this option as well I think. What about OneDrive and other file list providers? Will they all allow for more than a 100/request? If not, we'll be back to the Load All idea.

What about grids, Insta and Unsplash? We keep lazy loading there, right? Do we remove lazy loading in list providers, if we will always load all?

Should we set loading="lazy" to thumbnail images at least? We don't have VirtualList in provider-views like we do in Uppy file list.

Dumping all ideas unstructured, apologizes, vacation mode. Will appreciate if this is not merged until I can test as well.

Copy link
Contributor

@mifi mifi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good solution, as long as it will work for all other providers too

Though I think we need to have a way to abort (if the user doesn't wait for all to load), e.g. they want to go back instead after realising the folder is taking too long to load.

@Murderlon
Copy link
Member Author

I wonder who found out about splitting the permissions call and the list call ;-)

Just following instructions yes 👍

I do like this option as well I think. What about OneDrive and other file list providers? Will they all allow for more than a 100/request?

  • Google Drive: 1000 (increased in this PR from default 100)
  • Dropbox: 2000
  • OneDrive: 999 (increased in this PR from the default 200)
  • Box: 1000

What about grids, Insta and Unsplash? We keep lazy loading there, right? Do we remove lazy loading in list providers, if we will always load all?

Everything can be kept as is. For lists, it simply won't fetch when scrolling automatically. For grids, we want to keep things as is because images are heavy. Or think about that and improve it in a separate PR.

Should we set at least loading="lazy" to thumbnail images at least? We don't have VirtualList in provider-views like we do in Uppy file list.

Very good point 👍

Will appreciate if this is not merged until I can test as well.

We can wait yes.

Though I think we need to have a way to abort

Looking into it 👍

@Murderlon Murderlon changed the title Load Google Drive lists 10x faster & always load all files Load Google Drive / OneDrive lists 5-10x faster & always load all files Jun 21, 2023
@arturi
Copy link
Contributor

arturi commented Jun 22, 2023

Backwards-compat should be ok on this, right? Do we need fallback for when new Uppy client is using old Companion, which doesn't have the username endpoint?

@tcgj
Copy link
Contributor

tcgj commented Jun 23, 2023

Just popping in because I've seen users run into problems with seeing fewer files than expected before. And I have some questions about what the previous behaviour is:

  1. Does this behaviour of loading partially only happen when selecting files from within a folder?

  2. What if I selected a folder containing 2000 files? Is the current behaviour, for e.g. OneDrive, only uploading 200 of the 2000 files in that folder?

  3. What happens if I select multiple folders? Does that upload 200 from each folder?

  4. Also, regarding your solution, OneDrive: increase to 1000 from default 200. and Always load all files when entering a folder. seems to mean different things. What do the 1000 files mean here?

  5. And lastly I guess, does this PR solve the problems above?

@aduh95
Copy link
Contributor

aduh95 commented Jun 24, 2023

  1. Does this behaviour of loading partially only happen when selecting files from within a folder?

Before that PR, when a user enters a directory with more than 100 files, only the first 100 files are loaded at first. More files are loaded if the user scrolls to the end of the list (lazy-loading).

2. What if I selected a folder containing 2000 files? Is the current behaviour, for e.g. OneDrive, only uploading 200 of the 2000 files in that folder?

No when selecting a folder it would have selected all the files from this folder, this PR doesn't change that (it makes it faster though, see 4.).
What this PR change is the behavior of Uppy when the user enters a folder with more than 100 files (previously it would show the first 100 files, and load more on scroll; now it loads all the files at once).

3. What happens if I select multiple folders? Does that upload 200 from each folder?

It depends on how many files there are in each folder, if there are 200 files in them yes, if it's a different number, no. Again, this PR doesn't change that.

4. What do the 1000 files mean here?

It's the number of files per API request. Uppy used to fetch lists of 100 files (so 20 requests were needed for a folder with 2000 files), with this per it's getting lists of 1000 files (so 2 requests are needed for a folder with 2000 files).

5. And lastly I guess, does this PR solve the problems above?

You tell us 🤷‍♂️ it's certainly the goal.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it locally, and I like it. There's one user story that might be improved:

  1. The user navigates their remote file tree.
  2. They click by mistake on a folder containing lots of file.

They are faced with two bad options:

  • Click cancel, and go back at the start. They need to re-click through all the file tree to go back where they were.
  • Wait for the whole folder to load then go back. They've lost time.

Could we make a cancel button just for the folder loading which does not reset navigation? It'd be nice to have I think.

@aduh95
Copy link
Contributor

aduh95 commented Jun 24, 2023

Also, it looks like cancelling a folder loading breaks the breadcrumbs:
screenshot of the breadcrumbs showing some folder twice

It looks like it's no longer able to reset its state.

@Murderlon
Copy link
Member Author

They are faced with two bad options:

  • Click cancel, and go back at the start. They need to re-click through all the file tree to go back where they were.
  • Wait for the whole folder to load then go back. They've lost time.

Could we make a cancel button just for the folder loading which does not reset navigation? It'd be nice to have I think.

I understand the concern. However if you click cancel and click the provider again, you are back at the folder you were in before cancelling the folder. So if you just entered /foo/bar/baz and baz is loading long, cancel and reentering would take you to /foo/bar.

Waiting very long is also quite uncommon. Adding this there would take quite a bit more code to try to orchestrate things happening inside getFolder with some deeper component. Perhaps it might be better to do some caching, so that rentering is quick? And otherwise you just have to wait a little longer or spend two clicks instead of one.

Also, it looks like cancelling a folder loading breaks the breadcrumbs:

Fixed!

Murderlon and others added 2 commits June 28, 2023 14:43
Co-authored-by: Mikael Finstad <finstaden@gmail.com>
Co-authored-by: Mikael Finstad <finstaden@gmail.com>
@Murderlon
Copy link
Member Author

Thinking about adding the virtual list in a separate PR btw.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some testing, we can end up in a lock state if we lose connection while loading the files, but I don't think there's much we can do about that as the browser keeps the request in a pending state and never shows the request as failed. Users can still use the Cancel button to unlock themselves so I think we're good here.

@arturi
Copy link
Contributor

arturi commented Jul 3, 2023

todo for myself: test with Facebook, as it is a combo of folders (list) with images (grid). Does it load all images?

@arturi
Copy link
Contributor

arturi commented Jul 4, 2023

Nice work! As discussed and after trying other solutions, this is probably a better way than certain folders only appearing after clicking “Load all”. This does come with a huge performance hit on 10k files, but so does Load all after it's clicked. Virtual list is probably needed indeed.

image

“Added” instead of “Loaded”? We probably need a different i18n string.

I scrolled to the middle of the 10k list (took about 40 seconds to load on a slow connection), thumbnails not loading due to local Companion, but the list jumps probably due to lazy-loading still 🤔 Maybe we need width/height attributes on the image preview thing as well, but careful as it's shared between grid and list.

Screencast.from.2023-07-04.20-18-50.webm

(I am not doing anything to scroll at least the first 6 seconds or so)

@Murderlon
Copy link
Member Author

Murderlon commented Jul 5, 2023

Virtual list is probably needed indeed.

I was thinking to do it in a follow up PR as it's a but more involved and the changes are already quite big here.

“Added” instead of “Loaded”? We probably need a different i18n string.

I thought in this case we can get away with it to not keep introducing new locale strings with only slight differences. I'd like to keep it the same. I'm okay with changing it to "Loaded" but then we also have that when you confirm your selection and it's added to Uppy.

Maybe we need width/height attributes on the image preview thing as well

I first want to see if virtual list solves this already for us.

@arturi
Copy link
Contributor

arturi commented Jul 5, 2023

I thought in this case we can get away with it to not keep introducing new locale strings with only slight differences. I'd like to keep it the same. I'm okay with changing it to "Loaded" but then we also have that when you confirm your selection and it's added to Uppy.

I think it's ok to have to locale strings. Yes, I also wanted to avoid this, but lying to users is worse. Users come first, double string is a developer problem, and it's a small one, too. I thought at first it's a bug and it will actually add all those 10,000 files.

https://github.com/transloadit/uppy/pull/4480/files#diff-f42276d336fddf1fee816f626d3ad4f9bbcace168c658f1d7c8e238ef47a16afR45

@Murderlon
Copy link
Member Author

Added a new string 🆕

…les option — skip loading all for Insta, Facebook, make it opt-in

Co-authored-by: Mikael Finstad <finstaden@gmail.com>
@arturi
Copy link
Contributor

arturi commented Jul 6, 2023

Do one more test on your side, please, and I think it's ready to ship!

* main: (24 commits)
  Add missing pt-BR locales for ImageEditor plugin (#4558)
  Release: uppy@3.11.0 (#4550)
  @uppy/companion: fix infinite recursion in uploader test (#4536)
  @uppy/xhr-upload: export `Headers` type (#4549)
  @uppy/aws-s3-multipart: increase priority of abort and complete (#4542)
  @uppy/aws-s3: fix remote uploads (#4546)
  meta: use `corepack yarn` instead of `npm` to launch E2E (#4545)
  @uppy/aws-s3-multipart: fix upload retry using an outdated ID (#4544)
  @uppy/status-bar: remove throttled component (#4396)
  @uppy/aws-s3-multipart: fix Golden Retriever integration (#4526)
  examples/aws-nodejs: merge multipart and non-multipart examples (#4521)
  @uppy/companion: bump semver from 7.3.7 to 7.5.3 (#4529)
  @uppy/aws-s3-multipart: add types to internal fields (#4535)
  examples/aws-nodejs: update README (#4534)
  examples/aws-nodejs: showcase an example without preflight requests (#4516)
  @uppy/aws-s3-multipart: fix pause/resume (#4523)
  @uppy/status-bar: fix ETA when Uppy recovers its state (#4525)
  @uppy/aws-s3-multipart: fix resume single-chunk multipart uploads (#4528)
  @uppy/companion: fix part listing in s3 (#4524)
  example/aws-php: make it forward-compatible with the next Uppy major (#4522)
  ...
@Murderlon Murderlon merged commit 9efd865 into main Jul 10, 2023
18 checks passed
@Murderlon Murderlon deleted the provider-view-load-all-files branch July 10, 2023 08:15
Murderlon added a commit that referenced this pull request Jul 11, 2023
* main:
  meta: upgrade dev dependencies
  meta: Don't use triage label (#4552)
  meta: update Cypress (#4562)
  Load Google Drive / OneDrive lists 5-10x faster & always load all files (#4513)
@github-actions github-actions bot mentioned this pull request Jul 13, 2023
github-actions bot added a commit that referenced this pull request Jul 13, 2023
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/aws-s3-multipart |   3.5.0 | @uppy/locales          |   3.2.3 |
| @uppy/box              |   2.1.2 | @uppy/onedrive         |   3.1.2 |
| @uppy/companion        |   4.7.0 | @uppy/provider-views   |   3.4.0 |
| @uppy/companion-client |   3.2.1 | @uppy/react            |   3.1.3 |
| @uppy/core             |   3.3.1 | @uppy/status-bar       |   3.2.2 |
| @uppy/dashboard        |   3.4.2 | @uppy/transloadit      |   3.2.0 |
| @uppy/dropbox          |   3.1.2 | @uppy/utils            |   5.4.1 |
| @uppy/google-drive     |   3.2.0 | uppy                   |  3.12.0 |

- @uppy/transloadit: fix error message (Antoine du Hamel / #4572)
- @uppy/provider-views: add support for remote file paths (Mikael Finstad / #4537)
- @uppy/transloadit: implement Server-sent event API (Antoine du Hamel / #4098)
- @uppy/aws-s3-multipart: add support for signing on the client (Antoine du Hamel / #4519)
- @uppy/react: allow `id` from props (Merlijn Vos / #4570)
- @uppy/aws-s3-multipart: fix lint warning (Antoine du Hamel / #4569)
- @uppy/status-bar: listen to `upload` event instead of button click (Antoine du Hamel / #4563)
- @uppy/aws-s3-multipart: fix support for non-multipart PUT upload (Antoine du Hamel / #4568)
- @uppy/companion: fix esm imports in production/transpiled builds (Dominik Schmidt / #4561)
- @uppy/locales: fix expression and spelling errors in es_ES (Rubén / #4567)
- meta: upgrade dev dependencies (dependabot\[bot\])
- meta: Don't use triage label (Artur Paikin / #4552)
- meta: update Cypress (Antoine du Hamel / #4562)
- @uppy/box,@uppy/companion,@uppy/dropbox,@uppy/google-drive,@uppy/onedrive,@uppy/provider-views: Load Google Drive / OneDrive lists 5-10x faster & always load all files (Merlijn Vos / #4513)
- @uppy/locales: Add missing pt-BR locales for ImageEditor plugin (Mateus Cruz / #4558)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants