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

afterFill() - add loading progress #5288

Merged
merged 2 commits into from
Jul 2, 2024
Merged

Conversation

lakesare
Copy link
Contributor

@lakesare lakesare commented Jun 29, 2024

Reintroduces @mifi's loading progress report.

Screen.Recording.2024-06-29.at.14.01.30.mov

Copy link
Contributor

Diff output files
diff --git a/packages/@uppy/provider-views/lib/Browser.js b/packages/@uppy/provider-views/lib/Browser.js
index 2b692ca..d8a3bfb 100644
--- a/packages/@uppy/provider-views/lib/Browser.js
+++ b/packages/@uppy/provider-views/lib/Browser.js
@@ -33,7 +33,7 @@ function Browser(props) {
   if (isLoading) {
     return h("div", {
       className: "uppy-Provider-loading",
-    }, h("span", null, i18n("loading")));
+    }, typeof isLoading === "string" ? isLoading : i18n("loading"));
   }
   if (displayedPartialTree.length === 0) {
     return h("div", {
diff --git a/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js b/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js
index e57827c..cc91b31 100644
--- a/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js
+++ b/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js
@@ -258,10 +258,19 @@ export default class ProviderView {
     } = this.plugin.getPluginState();
     this.setLoading(true);
     await _classPrivateFieldLooseBase(this, _withAbort)[_withAbort](async signal => {
-      const enrichedTree = await PartialTreeUtils.afterFill(partialTree, path =>
-        this.provider.list(path, {
-          signal,
-        }), this.validateSingleFile);
+      const enrichedTree = await PartialTreeUtils.afterFill(
+        partialTree,
+        path =>
+          this.provider.list(path, {
+            signal,
+          }),
+        this.validateSingleFile,
+        n => {
+          this.setLoading(this.plugin.uppy.i18n("addedNumFiles", {
+            numFiles: n,
+          }));
+        },
+      );
       const aggregateRestrictionError = this.validateAggregateRestrictions(enrichedTree);
       if (aggregateRestrictionError) {
         this.plugin.setPluginState({
diff --git a/packages/@uppy/provider-views/lib/utils/PartialTreeUtils/afterFill.js b/packages/@uppy/provider-views/lib/utils/PartialTreeUtils/afterFill.js
index f6b93a1..59d73ee 100644
--- a/packages/@uppy/provider-views/lib/utils/PartialTreeUtils/afterFill.js
+++ b/packages/@uppy/provider-views/lib/utils/PartialTreeUtils/afterFill.js
@@ -37,7 +37,7 @@ const recursivelyFetch = async (queue, poorTree, poorFolder, apiList, validateSi
     queue.add(() => recursivelyFetch(queue, poorTree, folder, apiList, validateSingleFile));
   });
 };
-const afterFill = async (partialTree, apiList, validateSingleFile) => {
+const afterFill = async (partialTree, apiList, validateSingleFile, reportProgress) => {
   const queue = new PQueue({
     concurrency: 6,
   });
@@ -48,6 +48,10 @@ const afterFill = async (partialTree, apiList, validateSingleFile) => {
   poorFolders.forEach(poorFolder => {
     queue.add(() => recursivelyFetch(queue, poorTree, poorFolder, apiList, validateSingleFile));
   });
+  queue.on("completed", () => {
+    const nOfFilesChecked = poorTree.filter(i => i.type === "file" && i.status === "checked").length;
+    reportProgress(nOfFilesChecked);
+  });
   await queue.onIdle();
   return poorTree;
 };

@lakesare lakesare marked this pull request as ready for review June 29, 2024 09:06
@lakesare lakesare requested a review from mifi June 29, 2024 09:13
@lakesare
Copy link
Contributor Author

lakesare commented Jul 2, 2024

@Murderlon, how do we treat End-to-end tests / Browser tests (pull_request_target) now, do we consider failing tests that do not relate to our changes flakey tests that can be ignored?

This PR had 4 failing tests:

1) Dashboard with Tus: should upload cat image successfully
2) Dashboard with Tus: should start exponential backoff when receiving HTTP 429
3) Dashboard with Tus: should upload remote image with Unsplash plugin
4) Dashboard with Tus: should upload remote image with URL plugin

Now I reran it, and it gets 2 failing tests:

1) Dashboard with Transloadit: should complete when resuming after pause
2) Dashboard with @uppy/aws-s3-multipart: should upload cat image successfully

@Murderlon
Copy link
Member

Ideally the tests aren't flaky and we only merge green CI. We should look into the flakyness.

@lakesare lakesare merged commit dd11264 into 4.x Jul 2, 2024
17 checks passed
@lakesare lakesare deleted the lakesare/reporting-loading-progress branch July 2, 2024 09:05
github-actions bot added a commit that referenced this pull request Jul 2, 2024
| Package                |       Version | Package                |       Version |
| ---------------------- | ------------- | ---------------------- | ------------- |
| @uppy/companion        | 5.0.0-beta.12 | @uppy/form             |  4.0.0-beta.6 |
| @uppy/companion-client |  4.0.0-beta.9 | @uppy/provider-views   | 4.0.0-beta.11 |
| @uppy/core             | 4.0.0-beta.12 | uppy                   | 4.0.0-beta.14 |
| @uppy/drag-drop        |  4.0.0-beta.5 |                        |               |

- @uppy/companion: make `oauthOrigin` option required (Mikael Finstad / #5276)
- @uppy/provider-views: `afterFill()` - add loading progress (Evgenia Karunus / #5288)
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.

2 participants