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

Correct loaded & progress values when file is uploaded #1002

Merged
merged 6 commits into from
Oct 23, 2023

Conversation

mkszepp
Copy link
Contributor

@mkszepp mkszepp commented Oct 12, 2023

fix #1001

@gilest
Copy link
Collaborator

gilest commented Oct 18, 2023

Hey @mkszepp thanks for your contribution. I'm sure it's at least more correct than what we have at present 😅

Just one thing... it would be really nice to add some test coverage for this logic.

I will grant that this area is not well tested, and maybe a little complicated to cover given the objects involved.

Anyway, I'll see if I can stand up some tests myself soon but feel free to add your own here

@gilest gilest added the bug label Oct 21, 2023
@gilest gilest changed the title Fix loaded & progress values when file is upload Correct loaded & progress values when file is uploaded Oct 21, 2023
@gilest
Copy link
Collaborator

gilest commented Oct 21, 2023

Hey 👋🏻 @mkszepp

It was more difficult than I thought but I have managed to (I think) reproduce part of this issue through a regression test in #1005

I've already validated that your changes in this PR fix the issue expressed in that test.

Could you please 👀 review that and let me know your thoughts 🙇🏻

@mkszepp
Copy link
Contributor Author

mkszepp commented Oct 21, 2023

Hi @gilest

sry, i haven't got time to write tests in the last days.

thank you ❤️ for your PR, it looks good! Also the notes are perfect and clearly. 😀.

I think its ready for release... after the release, i will try it in my app.

@gilest
Copy link
Collaborator

gilest commented Oct 21, 2023

Thanks for your review 😄

I'm happy to merge this bugfix now. Could you please:

  • Rebase and resolve conflicts with the upstream HttpRequest changes
  • Un skip the regression test by replacing module.skip with module test-app/tests/integration/progress-test.ts:39

@mkszepp
Copy link
Contributor Author

mkszepp commented Oct 23, 2023

Thank you @gilest!

now i have fixed merge confliects und activated tests.

@gilest gilest merged commit ce201a5 into adopted-ember-addons:master Oct 23, 2023
14 checks passed
@gilest
Copy link
Collaborator

gilest commented Oct 23, 2023

Released in 8.2.1. Thanks again for your contribution

@mkszepp
Copy link
Contributor Author

mkszepp commented Oct 23, 2023

@gilest thank!

I have updated the package in my app and this is now the result of the progress bar (there are 3 files in queue, which will be uploaded sequential). It looks better than before, but i think there is something, not to 100% correct... (i'm not sure if there is now a my code part or something of ember-file-upload). i will check this and make a report.

It looks like, when the a file upload is completed, the percentage are not correct... but i will debug it

3
6
9
12
15
19
22
24
0 --> for short moment there is zero...
24
24
27
31
34
37
40
43
47
50
53
56
59
62
66
69
72
75
78
82
85
86
24 --> for short moment there is 24...
86
86
89
92
95
98
99
86 --> at the end there is 86...
``` 

@mkszepp
Copy link
Contributor Author

mkszepp commented Oct 23, 2023

now i have debugged and found out, why this occures...

When the upload is completed, request.onprogress will be fired at the same time like request.onloadend.
As the upload is completed with request.onprogress the server returns the Content-Length and the browser takes this size... In my case the returned Content-Length is lower, because i don't return the image and so the progress bar is going back.

My debug console.logs:
grafik

To fix this behavier we should replace the file.loaded set in request.onprogress to:
file.loaded = Math.max(evt.loaded, file.loaded); instead of file.loaded = evt.loaded;

PR for this fix: #1007

@RobbieTheWagner
Copy link
Member

@mkszepp after this PR, it seems all our file uploads just go straight to 100% progress. Every version before 8.2.1, which this was in, worked correctly. Any thoughts on how we can fix things?

@mkszepp
Copy link
Contributor Author

mkszepp commented Nov 8, 2023

@RobbieTheWagner i have also seen the issue from @gilest (#1013)

I have updated the addon some days ago in my app and tested with firefox and it looks perfect (no up/down jumping of progress bar like before)...

I have now tested the upload again in Chrome and i think that the browser behavier is different... it looks like in Firefox works correctly but on Chrome it looks buggy 😢

I will try to debug it deeper to make a PR to fix this behavier... sry for this incident

Progress bar in chrome while uploading 3 files:

0
0
1
1
3
3
5
5
6
6
8
8
10
10
11
11
13
13
15
15
16
16
18
18
20
20
21
21
23
23
24
24
24
24
24
24
24
24 // first file upload completed
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67
67 // 2nd file completed uploaded
72
72
77
77
81
81
86
86
91
91
95
95
100
100
100
100
100
100

Progress bar in Firefox while uploading 3 files:

3
6
9
12
15
19
22
24
24 // first file upload completed
27
31
34
37
40
43
47
50
53
56
59
62
66 // 2nd file upload completed
69
72
75
78
82
85
86
86
89
92
95
98
100

@mkszepp
Copy link
Contributor Author

mkszepp commented Nov 8, 2023

it looks like Chrome returns on my secound file a wrong filesize onloadstart:

Chrome:
grafik

Firefox:
grafik

@mkszepp
Copy link
Contributor Author

mkszepp commented Nov 8, 2023

added a new PR that shoud fix the bug in Chrome and that works also in Firefox (#1016)

@RobbieTheWagner
Copy link
Member

@mkszepp thanks for looking into this! Have you tried with just one really large file? When I do one really large file on Chrome and set my Network to "Slow 3G" in devtools so that I can better see the progress, it's just always 100%. Perhaps your changes fix that, but I just want to make sure.

@mkszepp
Copy link
Contributor Author

mkszepp commented Nov 9, 2023

@RobbieTheWagner yes i have tested with a 30MB image in "Fast 3G" on Chrome & Firefox (the upload needs in this case more than 5 mins).

Also all previous tests were done in Fast 3G mode

gilest pushed a commit that referenced this pull request Nov 10, 2023
Sometimes the browser doesn't has the correct file size onloadstart. like described here: #1002 (comment)

During debugging i have found out, that this bug is present in Chrome. In Chrome there is often a total wrong total size (see comment) and in Firefox there is sometime a little difference between the total size onloadstart and onprogress.

We should set file.size like in earlier versions also in onprogress event and check in progress event, if the upload is already complete.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

loaded & progress are not correct when file is upload
3 participants