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 file upload #556 #558

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

Conversation

ticruz38
Copy link
Collaborator

@ticruz38 ticruz38 commented Jan 30, 2025

A few things here:

  • The loading state was no longer bound to editorLoading in NoteReply, this allowed us to send a reply while a file was being uploaded, NoteCreate editor was instantiated correctly so that we could NOT send a note while a file was being uploaded.
  • The blossom endpoint that we use by default (cdn.satellite.earth) has a very low free tier, after 2 uploads, it returns an error (payment required)
  • If a file upload returns an error, the editor loads forever, we need to catch those errors and update the editor content accordingly
  • When saving a draft, if a file is being uploaded, the blob will be saved and never resolved to its actual upload URL

@ticruz38 ticruz38 marked this pull request as draft January 30, 2025 17:01
@ticruz38
Copy link
Collaborator Author

The last 2 points would be worth a PR in nostr-editor eventually.
It can be fixed in Coracle too, but with some convoluted tiptap code.

@staab
Copy link
Collaborator

staab commented Jan 30, 2025

That all makes sense, we've needed to add errors to file uploads for a long time. Is there a way to get the error from nostr-editor when an upload fails and show it? Feel free to approach this problem however you like, either via PR to nostr-editor/@welshman/editor or directly here.

@ticruz38
Copy link
Collaborator Author

ticruz38 commented Jan 31, 2025

At the time the only blossom endpoint working was cdn.satellite.earth, all others endpoint were CORs protected. Probably still the case but it would be nice to take another round and see if one blossom provider is somehow more generous than satellite.

  • Handle error with a message
Screenshot 2025-01-31 at 11 57 03 Screenshot 2025-01-31 at 11 57 20
  • Clean up blobs before saving draft and once an error is returned.

  • A PR is opened to add a removeBlobs command to the fileUpload extension in nostr-editor.

@@ -75,6 +88,11 @@ export const getEditor = ({
onComplete() {
uploading?.set(false)
},
onUploadError(currentEditor, file) {
removeBlobs(currentEditor as Editor)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should work fine, but ideally it would be targeted to the specific file that failed. Otherwise, if there are multiple concurrent uploads and one fails, the other will get removed as well too right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes that's right, onUploadFail gives me the file that failed so it should be possible to remove just this one

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nostr-editor keep an uploadError attribute on each node. I have added an errorsOnly attribute to the removeBlobs command that will only remove blobs in an error state.

src/app/shared/NoteReply.svelte Outdated Show resolved Hide resolved
- add the upload url to the error message
@ticruz38
Copy link
Collaborator Author

ticruz38 commented Feb 3, 2025

  • use toast to display upload error
  • add an option to only remove blobs in error state

@ticruz38 ticruz38 marked this pull request as ready for review February 3, 2025 14:34
@ticruz38
Copy link
Collaborator Author

ticruz38 commented Feb 3, 2025

once the PR is merged in nostr-editor, we will be able to remove some code here too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants