-
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
Handle njump me links #1202
Handle njump me links #1202
Conversation
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.
I'm happy njump is working now! We should add support for the nostr:
scheme. I think this kind of things are super important.
} else { | ||
/// Check for links like nos:nevent123174 | ||
let firstPathComponent = components.path | ||
let unformattedRegex = /(?:nostr:)?(?<entity>((npub1|note1|nprofile1|nevent1)[a-zA-Z0-9]{58,255}))/ |
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.
Can we move this regex to a new shared place? Even though the regex could be not 100% the same and could not be shared, we are repeating patterns for some things and I found myself sometimes having to fix a problem with one regex and having to search for the other ones to see if the same error could occur there. Just a centralized place where we put all regexes could help to identify potential issues.
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.
I want to add support for nostr:
too. I considered refactoring NoteParser and this code to share a regex, but I didn't because this was just a few minutes of work that I didn't want to throw away because it seemed really useful. I didn't want to (probably) quadruple the time I spent on it to do a refactor. I tried to pull out the shared part of the regex into a constant, but you can't interpolate a regex literal inside another regex literal, so I would have to switch to NSRegularExpression
or something, which would require more code refactoring... and that would only be for the regex itself. To share the code that extracts the hex from bech32 would require some more.
Given that this is the second place we are using the regex I'm ok with duplicating it. I like the rule of waiting to refactor until you've copied code in three places rather than two.
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.
Fair enough. Let's keep the eyes open for the next time we need to add or modify a regex to include that refactor in the scope of the ticket.
Excited to have this! |
Issues covered
This is related to #367, but it doesn't quite cover all of it and that ticket isn't prioritized so I'm not going to move it into the sprint or anything. @rabble was complaining about this so I was going to write up a comment about how to do it but it was so easy that ended up just doing it.
Description
This adds code to handle the links njump.me is using if you tap "Open in Nos" from their site.
How to test
The Nos app should open and display the note or profile.
Screenshots/Video
RPReplay_Final1717080608.MP4