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.

Sign up for GitHub

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

Jump to bottom

@uppy/core: Don't set late (throttled) progress event on a file that is 100% complete #4507

Merged
merged 1 commit into from
Jun 19, 2023
Merged

@uppy/core: Don't set late (throttled) progress event on a file that is 100% complete #4507

merged 1 commit into from
Jun 19, 2023

Conversation

arturi
Copy link
Contributor

@arturi arturi commented Jun 16, 2023 •
edited
Loading

Without this, first file-complete event arrives, setting progress to full total amount of bytesUploaded and complete: true, then late throttled upload-progress re-sets that bytesUploaded to a new value, making calculateTotalProgress confused.

@arturi @aduh95
Don't set late (throttled) progress event on a file that is 100% comp…
0251f51 
…lete

Co-authored-by: Antoine du Hamel <antoine@transloadit.com>
@arturi arturi changed the title Don't set late (throttled) progress event on a file that is 100% complete @uppy/core: Don't set late (throttled) progress event on a file that is 100% complete Jun 16, 2023
Murderlon
Murderlon reviewed Jun 16, 2023
View reviewed changes
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Good catch!

packages/@uppy/core/src/Uppy.js
this.log(`Not setting progress for a file that has been removed: ${file?.id}`)
return
}

if (fileInState.progress.percentage === 100) {
this.log(`Not setting progress for a file that has been already uploaded: ${file.id}`)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to log this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do above, so why not here? This way we know something is up, ideally should not be happening. When you set debug: false in prod, you don't see any of those logs, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

should we >= 100 just to be sure?

@aduh95 aduh95 merged commit ad96bdb into main Jun 19, 2023
@aduh95 aduh95 deleted the fix-progress-complete-file branch June 19, 2023 13:11
@arturi
Copy link
Contributor Author

arturi commented Jun 19, 2023 via email

@mifi
Copy link
Contributor

mifi commented Jun 19, 2023

yes, so it would be nice to not screw up more, if that happens :)

@github-actions github-actions bot mentioned this pull request Jun 19, 2023
Merged
@aduh95
Copy link
Contributor

aduh95 commented Jun 19, 2023

I think if the state is 120% (or anything above 100%), we definitely do want to overwrite it, so the current check LGTM.

github-actions bot added a commit that referenced this pull request Jun 19, 2023
@github-actions
Release: uppy@3.10.0 (#4511)
147e5da 
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/aws-s3           |   3.2.0 | @uppy/status-bar       |   3.2.0 |
| @uppy/aws-s3-multipart |   3.4.0 | @uppy/transloadit      |   3.1.6 |
| @uppy/companion        |   4.5.1 | @uppy/tus              |   3.1.1 |
| @uppy/core             |   3.2.1 | @uppy/url              |   3.3.2 |
| @uppy/dashboard        |   3.4.1 | @uppy/utils            |   5.4.0 |
| @uppy/golden-retriever |   3.0.4 | @uppy/xhr-upload       |   3.3.0 |
| @uppy/locales          |   3.2.2 | uppy                   |  3.10.0 |
| @uppy/provider-views   |   3.3.1 |                        |         |

- @uppy/aws-s3-multipart: fix the chunk size calculation (Antoine du Hamel / #4508)
- @uppy/aws-s3: add `shouldUseMultipart` option (Antoine du Hamel / #4299)
- @uppy/companion: switch from aws-sdk v2 to @aws-sdk/* (v3) (Scott Bessler / #4285)
- @uppy/companion,@uppy/core,@uppy/dashboard,@uppy/golden-retriever,@uppy/status-bar,@uppy/utils: Migrate all lodash' per-method-packages usage to lodash. (LinusMain / #4274)
- @uppy/core: Don't set late (throttled) progress event on a file that is 100% complete (Artur Paikin / #4507)
- @uppy/companion: revert randomness from file names (Mikael Finstad / #4509)
- @uppy/companion: Custom provider fixes (Mikael Finstad / #4498)
- @uppy/transloadit: ensure `fields` is not nullish when there no uploaded files (Antoine du Hamel / #4487)
- @uppy/aws-s3-multipart,@uppy/aws-s3,@uppy/tus,@uppy/utils,@uppy/xhr-upload: When file is removed (or all are canceled), controller.abort queued requests (Artur Paikin / #4504)
- @uppy/provider-views: Fix range selection not resetting and computing correctly (Terence C / #4415)
- meta: disallow use of `.only` in tests (Antoine du Hamel / #4494)
- @uppy/companion: fix 500 when file name contains non-ASCII chars (Antoine du Hamel / #4493)
- @uppy/locales: update `fr_FR.js` (Samuel De Backer / #4499)
- @uppy/aws-s3-multipart,@uppy/tus,@uppy/xhr-upload: Don't close socket while upload is still in progress (Artur Paikin / #4479)
- meta: bump `luxon` from 1.28.0 to 1.28.1 (dependabot[bot] / #4497)
- @uppy/utils: rename `EventTracker` -> `EventManager` (Stephen Wooten / #4481)
- meta: bump cookiejar from 2.1.3 to 2.1.4 (dependabot[bot] / #4496)
- meta: make `pre-commit` use `corepack yarn` instead of `npm run` (Antoine du Hamel / #4495)
- meta: bump ua-parser-js from 0.7.31 to 0.7.35 (dependabot[bot] / #4474)
- meta: bump @sideway/formula from 3.0.0 to 3.0.1 (dependabot[bot] / #4473)
- meta: bump http-cache-semantics from 4.1.0 to 4.1.1 (dependabot[bot] / #4472)
- @uppy/companion: Use filename from content-disposition instead of relying on url, with fallback (Artur Paikin / #4489)
- meta: bump `babel`, `esbuild`, and `vite` (dependabot[bot] / #4485)
- @uppy/dashboard: include the old state when setting new (Artur Paikin / #4490)
- @uppy/companion: fix companion implicitpath (Mikael Finstad / #4484)
- @uppy/companion: fix undefined protocol and example page (Mikael Finstad / #4483)
- meta: upgrade Cypress 12.9.0 -> 12.14.0 (Antoine du Hamel / #4491)
- @uppy/core: remove `state` getter from types (Antoine du Hamel / #4477)
- examples/php-xhr: Added filename sanitation and file size check before saving (neuronet77 / #4432)
- examples/php-xhr: update PHP dependencies (dependabot[bot])
- @uppy/xhr-upload: add support for arrays in metadata (Vasiliy Matyushin / #4431)
- @uppy/status-bar: Filtered ETA (stduhpf / #4458)
- @uppy/aws-s3-multipart: fix `getUploadParameters` option (Antoine du Hamel / #4465)
Murderlon added a commit that referenced this pull request Jun 20, 2023
@Murderlon
Merge branch 'main' into fix-throttle-global-state
438e6d6 
* main: (61 commits)
  Release: uppy@3.10.0 (#4511)
  @uppy/aws-s3-multipart: fix the chunk size calculation (#4508)
  @uppy/aws-s3: add `shouldUseMultipart` option (#4299)
  @uppy/companion: switch from aws-sdk v2 to @aws-sdk/* (v3) (#4285)
  Migrate all lodash' per-method-packages usage to lodash. (#4274)
  @uppy/core: Don't set late (throttled) progress event on a file that is 100% complete (#4507)
  @uppy/companion: revert randomness from file names (#4509)
  Custom provider fixes (#4498)
  @uppy/transloadit: ensure `fields` is not nullish when there no uploaded files (#4487)
  When file is removed (or all are canceled), controller.abort queued requests (#4504)
  @uppy/provider-views: Fix range selection not resetting and computing correctly (#4415)
  meta: disallow use of `.only` in tests (#4494)
  @uppy/companion: fix 500 when file name contains non-ASCII chars (#4493)
  @uppy/locales: update `fr_FR.js` (#4499)
  Don't close socket while upload is still in progress (#4479)
  meta: bump `luxon` from 1.28.0 to 1.28.1 (#4497)
  @uppy/utils: rename `EventTracker` -> `EventManager` (#4481)
  meta: bump cookiejar from 2.1.3 to 2.1.4 (#4496)
  meta: make `pre-commit` use `corepack yarn` instead of `npm run` (#4495)
  meta: bump ua-parser-js from 0.7.31 to 0.7.35 (#4474)
  ...
Murderlon added a commit that referenced this pull request Jun 20, 2023
@Murderlon
Merge branch 'main' into hide-pause-resume-remote
2e2afb7 
* main:
  Release: uppy@3.10.0 (#4511)
  @uppy/aws-s3-multipart: fix the chunk size calculation (#4508)
  @uppy/aws-s3: add `shouldUseMultipart` option (#4299)
  @uppy/companion: switch from aws-sdk v2 to @aws-sdk/* (v3) (#4285)
  Migrate all lodash' per-method-packages usage to lodash. (#4274)
  @uppy/core: Don't set late (throttled) progress event on a file that is 100% complete (#4507)
  @uppy/companion: revert randomness from file names (#4509)
  Custom provider fixes (#4498)
  @uppy/transloadit: ensure `fields` is not nullish when there no uploaded files (#4487)
  When file is removed (or all are canceled), controller.abort queued requests (#4504)
  @uppy/provider-views: Fix range selection not resetting and computing correctly (#4415)
  meta: disallow use of `.only` in tests (#4494)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mifi mifi mifi left review comments

@Murderlon Murderlon Murderlon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@arturi @mifi @aduh95 @Murderlon