Skip to content
This repository has been archived by the owner on May 22, 2021. It is now read-only.

abort uploads over maxfilesize #266

Merged
merged 2 commits into from
Jul 21, 2017
Merged

abort uploads over maxfilesize #266

merged 2 commits into from
Jul 21, 2017

Conversation

dannycoates
Copy link
Contributor

@dannycoates dannycoates commented Jul 20, 2017

Fixes #180

  • adds an alert error to the ui if the selected file's size is too big
  • aborts uploads on the server that are too big
  • adds the bytes module cuz im lazy

@johngruen
Copy link
Contributor

R+ on the string

@dannycoates dannycoates requested a review from abhinadduri July 20, 2017 20:07
max_file_size: {
format: Number,
default: (1024 * 1024 * 1024) * 2,
env: 'P2P_MAX_FILE_SIZE'
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we were removing all "P2P" references - rename?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeeeaaaah, i wanted to rename them all at once, but not in this PR, so... 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

but it's a new variable, is it not? unless you have an .env file that I am unaware of?

},
max_file_size: {
format: Number,
default: (1024 * 1024 * 1024) * 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment about what real-world value this is? I assume its 2GB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comments always tend to diverge from reality. do the math 💪

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. But to be clear, the UI says the recommended file limit is 1GB for uploads, but the server will allow up to ~2GB max?

firefox_send

Copy link
Contributor Author

Choose a reason for hiding this comment

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

both hard limits are the same. the recommendation is just a suggestion

`${filename} (${(progress[0] / 1000000).toFixed(1)}MB of ${(progress[1] / 1000000000).toFixed(1)}GB)`
);
}
$('.progress-text').text(`${filename} (${bytes(progress[0])} of ${bytes(progress[1])})`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this may be causing some UI jank as well. You can see in the following UI screenshot that the filesizes are rounded to 2 decimal places. I think in some cases during upload, it's rounding the ".00" to ".0" which causes the UI to slightly shift very briefly.

This may all be academic though since I believe @johngruen also has a PR to tweak the UI, so not sure who's PR will "win".

firefox_send

Copy link
Collaborator

@abhinadduri abhinadduri left a comment

Choose a reason for hiding this comment

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

Looks good, we might be able to close some error handling server side bugs with this.

@dannycoates dannycoates merged commit fbf906f into master Jul 21, 2017
@pdehaan pdehaan mentioned this pull request Jul 21, 2017
@dannycoates dannycoates deleted the sizelimit branch July 31, 2017 21:39
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants