-
Notifications
You must be signed in to change notification settings - Fork 14
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
Fixes alerts when uploading big files and suggests users subscribe to nostr.build #1321
Conversation
@rabble unfortunately that error message is usually a lie and is hiding the real error. Try commenting out some of your changes until you narrow down the line that is wrong. If that doesn't help maybe @joshuatbrown or I can take a look on Monday. |
@rabble thanks for working on this! It's a really annoying bug. |
Also we've picked up the habit as marking WIP PRs as "drafts" in Github. We'll still review them, but it's just an extra signal that it's not ready for merging yet. You can click the "convert to draft" button up by the reviewers to make this PR a draft. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, this looks great! It works exactly as expected, and it's super nice to have this message to tell people what's going on. In addition to my comment, I see a few SwiftLint errors to address, but this looks like it's really close.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Rabble, this is close but there are a few things missing.
- Right now it will return the size error no matter what error we get back from the nostr.build API. So even if the user is offline or something it will tell them they hit the size limit. I think we'll want to make a new case in the
FileStorageAPIClientError
type that is likefileTooBig
, and then when we get an error from nostr.build we should check to see if the error message is about the file size. We could check for the text "File size exceeds the limit" in the message. Search for the linethrow FileStorageAPIClientError.uploadFailed(response.message)
and replace it with something likeif itLooksLIkeASizeError { throw fileTooBigError } else { throw uploadFailed }
. - There are some linting errors. You can see this locally if you
brew install swiftlint
- Can you add a line to the top of the changelog?
There is also a bug I see that XCStringsTool isn't generating accessors for |
…StorageAPIClient and also some of the lint issues
…StorageAPIClient and also some of the lint issues
Ah, i wasn't aware xcode had a way of editing those files visually... i was editing the json.. i've now tried opening the file from teh CLI and it shows an editor.... i added spanish too. Should be fixed.. I think it's ready for you guys to look at again. |
Nos.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
Outdated
Show resolved
Hide resolved
Co-authored-by: Josh Brown <[email protected]>
spelling fix Co-authored-by: Josh Brown <[email protected]>
grammar fix Co-authored-by: Josh Brown <[email protected]>
embed format fix Co-authored-by: Josh Brown <[email protected]>
Co-authored-by: Josh Brown <[email protected]>
Co-authored-by: Josh Brown <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Rabble, I checked this out and fixed the linter errors for you. However I'm seeing one more bug. If I turn my internet off and try to upload a photo the resulting error message looks correct now the but the "Get Account" button is shown instead of "Ok".
One more request: can the "Get Account" button open https://nostr.build/plans instead of https://nostr.build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works perfectly and I'm glad we're looking for the HTTP 413! I just have some comments on how it's implemented, and I'd love to see a unit test or two. I have some ideas for how to do that, so I'm happy to help if needed.
@joshuatbrown, this is something I have been looking forward to do. I would be happy to have a guide on the unit testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works perfectly, and I love the updates! Everything looks great, though I'd love to see one very small tweak. Thanks!
Co-authored-by: Josh Brown <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update! This is perfect!
Thank you @joshuatbrown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I blocked the merge on this. Nice work!
Issues covered
#1275
Description
Provides a better error message and gives the option to pay for nostr.build
How to test
Screenshots/Video
Screen.Recording.2024-09-11.at.6.04.56.PM.mov