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

fix: avoid out of memory when minting large files #346

Merged
merged 3 commits into from
Oct 27, 2023
Merged

Conversation

Zir0h
Copy link
Contributor

@Zir0h Zir0h commented Oct 27, 2023

I tried to fix the out of memory bug from #280 with the suggestion @melMass posted. I don't know if I implemented it correctly tough. It seems to work as expected.

@github-actions
Copy link

github-actions bot commented Oct 27, 2023

PR Preview Action v1.4.4
Preview removed because the pull request was closed.
2023-10-27 21:10 UTC

…ile, the message still reads 'No file selected' which confuses visitors
@Zir0h Zir0h changed the title fix: possible fix for #280 fix: avoid out of memory when minting large files Oct 27, 2023
@melMass
Copy link
Member

melMass commented Oct 27, 2023

You are on fire ❤️!
I just missed my train so I'll have a look a bit later today

@melMass melMass added the 🫡 Priority: High After critical issues are fixed, these should be dealt with before any further issues. label Oct 27, 2023
@melMass
Copy link
Member

melMass commented Oct 27, 2023

I'm getting

Uncaught (in promise) DOMException: The requested file could not be read, typically due to permission problems that have occurred after a reference to a file was acquired.

For a 5Gb file.

As I'm looking into it it seems that chrome for instance has an ArrayBuffer size limit of 2GB... So we need to chunk it somehow 😓

@Zir0h
Copy link
Contributor Author

Zir0h commented Oct 27, 2023

I'm getting

Uncaught (in promise) DOMException: The requested file could not be read, typically due to permission problems that have occurred after a reference to a file was acquired.

For a 5Gb file.

As I'm looking into it it seems that chrome for instance has an ArrayBuffer size limit of 2GB... So we need to chunk it somehow 😓

Good thing we limited minting to 2GB 🤣

@melMass
Copy link
Member

melMass commented Oct 27, 2023

Oops! But with a 1.5GB I get this without errors in the console though

image

@melMass
Copy link
Member

melMass commented Oct 27, 2023

Ohhh ok there was just a delay between the time it catched the first one to then display the cover... For large file there should probably be some kind of loader to indicate that

@melMass
Copy link
Member

melMass commented Oct 27, 2023

In any case not related to this PR, it works!
But we should improve the mint UX a bit imo

@Zir0h
Copy link
Contributor Author

Zir0h commented Oct 27, 2023

But we should improve the mint UX a bit imo

I agree 100%! But I'm not good with frontend stuff 😆

Copy link
Member

@melMass melMass left a comment

Choose a reason for hiding this comment

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

Perfect!

In a next PR we should probably:

  • check the size of the file and alert about that
  • Add some kind of load state after selecting the main mint file, as the UI Isn't blocked one can click submit which fails the validation, not a big deal since once loaded the button for the cover appears and the mint state is restored

@melMass melMass merged commit c89df4f into main Oct 27, 2023
3 checks passed
@melMass melMass deleted the fix-out-of-memory branch October 27, 2023 21:09
melMass added a commit that referenced this pull request Nov 28, 2023
* fix: re-enable preview for interactive objkts, regression from #346

* avoid mixed content by providing the fullpath with trailing /

* fix width/height

---------

Co-authored-by: Mel Massadian <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🫡 Priority: High After critical issues are fixed, these should be dealt with before any further issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants