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

Show loading indicator on file assembling #1458

Conversation

Koc
Copy link
Contributor

@Koc Koc commented Oct 27, 2024

Background:
We've started using s3 storage instead of local files. And it performs much slower comparing to local filesystem. Now when we upload large file it can took up to 30 seconds for merging chunks into a single file. The problem is there is no any indicator of this activity and users can't understand what happening between progress bar hide and file appears in list.

You can observe how it works after the patch on the video below:

nextcloud-upload-2024-12-08_01.56.49.mp4

Copy link

codecov bot commented Oct 27, 2024

Bundle Report

Changes will increase total bundle size by 1.37kB (0.17%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
@nextcloud/upload-esm 395.76kB 640 bytes (0.16%) ⬆️
@nextcloud/upload-cjs 399.21kB 726 bytes (0.18%) ⬆️

Copy link

codecov bot commented Oct 27, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@070d762). Learn more about missing BASE report.
Report is 21 commits behind head on main.

Files with missing lines Patch % Lines
lib/uploader.ts 0.00% 3 Missing ⚠️
lib/components/UploadPicker.vue 80.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1458   +/-   ##
=======================================
  Coverage        ?   27.33%           
=======================================
  Files           ?       14           
  Lines           ?     1489           
  Branches        ?       48           
=======================================
  Hits            ?      407           
  Misses          ?     1082           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Koc Koc force-pushed the feature/show-loading-indicator-on-file-assembling branch from 5749296 to c468ae8 Compare October 27, 2024 12:22
lib/uploader.ts Outdated Show resolved Hide resolved
lib/uploader.ts Outdated Show resolved Hide resolved
@Koc Koc force-pushed the feature/show-loading-indicator-on-file-assembling branch 2 times, most recently from cd20d7f to 6fe84d3 Compare October 31, 2024 16:40
@skjnldsv
Copy link
Contributor

skjnldsv commented Nov 5, 2024

Hey, thanks for the PR!
Unfortunately, design wise, adding some text to tell the user about the assembly is not acceptable.

Imho, to simplify this, we should :

  • keep the progress bar active (if we don't do this already)
  • show a new status Processing... only if there is no other upload ongoing (if the only async tasks left are move requests). If there is still some upload ongoing, they are more important than a MOVE request ongoing in the background

@skjnldsv skjnldsv added enhancement New feature or request 2. developing Work in progress papercut Small issues that doesn't break the ux/ui labels Nov 5, 2024
@Koc Koc force-pushed the feature/show-loading-indicator-on-file-assembling branch 3 times, most recently from 41b71dc to 7856dae Compare December 8, 2024 00:59
@Koc
Copy link
Contributor Author

Koc commented Dec 8, 2024

@skjnldsv I've did requested changes and updated PR description with new video that shows hot it works

Copy link

@nfebe nfebe left a comment

Choose a reason for hiding this comment

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

I left a comment to help fix the linting, so it's easy to see the actual changes.

If you must add valid lint changes preferable to add in a separate commit.

Looks good-ish though

lib/uploader.ts Outdated Show resolved Hide resolved
@Koc Koc force-pushed the feature/show-loading-indicator-on-file-assembling branch 3 times, most recently from e88f321 to 4bc4f97 Compare December 9, 2024 17:37
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
@Koc Koc force-pushed the feature/show-loading-indicator-on-file-assembling branch from 4bc4f97 to 701639d Compare December 17, 2024 16:11
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
@Koc Koc force-pushed the feature/show-loading-indicator-on-file-assembling branch from 701639d to 06f3802 Compare December 17, 2024 17:33
@Koc
Copy link
Contributor Author

Koc commented Dec 17, 2024

@skjnldsv it works worse after latest changes 😿 . Can you please checkout my branch and take a look?

Here how can we test it:

# remote.php
if ($_SERVER['REQUEST_METHOD'] === 'PUT') {
	sleep(3);
} elseif ($_SERVER['REQUEST_METHOD'] === 'MOVE') {
	sleep(8);
}

Max chunk size is set to 10Mb. I'm uploading 33 + 150 Mb files.

@skjnldsv
Copy link
Contributor

Hey @Koc let me have a look later.

@skjnldsv
Copy link
Contributor

We've started using s3 storage instead of local files. And it performs much slower comparing to local filesystem. Now when we upload large file it can took up to 30 seconds for merging chunks into a single file.

Btw, we do support s3 chunk assembling if your s3 backend supports it :)
nextcloud/server#27034

@skjnldsv
Copy link
Contributor

skjnldsv commented Dec 18, 2024

Hey Koc let me have a look later.

#1530

@Koc Koc closed this Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress enhancement New feature or request papercut Small issues that doesn't break the ux/ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants