-
Notifications
You must be signed in to change notification settings - Fork 290
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
Bug: opening URL works intermittently #2429
Comments
@jb55 @alltheseas I am working on this one as it's pretty serious and it becomes a more visible issue when we rollout push notifications |
Sorry @jb55 and @alltheseas, I started a draft on Friday for this but forgot about it until now, and ended up doing #751 instead 🤦♂️ I will continue working on this next! (Or please let me know if something more urgent comes up) |
Descoping this from 1.10. I paused my work on this ticket. For anyone who picks this up, here is the draft I was working on https://github.com/danieldaquino/damus/tree/%232429 |
This commit improves reliability on the handling of external URLs. This was achieved through the following improvements: 1. The URL handler interface is now well-defined, with more clear inputs and outputs, to avoid silent failures and error paths that are hard to see within convoluted logic paths 2. Side effects during URL parsing were almost completely removed for more predictable behavior 3. Error handling logic was added to present errors to the user in a user-friendly manner, instead of silently failing 4. Event loading logic was moved into a special new thread view, which makes its own internal state evident to the user (i.e. whether the note is loading, loaded, or if the note could not be found) These changes make the URL opening logic more predictable, easy to refactor, and helps ensure the user always gets some outcome from opening a URL, even if it means showing a "not found" or "error" screen, to eliminate cases where nothing seems to happen. Closes: damus-io#2429 Changelog-Fixed: Improved robustness of the URL handler Signed-off-by: Daniel D’Aquino <[email protected]>
This commit improves reliability on the handling of external URLs. This was achieved through the following improvements: 1. The URL handler interface is now well-defined, with more clear inputs and outputs, to avoid silent failures and error paths that are hard to see within convoluted logic paths 2. Side effects during URL parsing were almost completely removed for more predictable behavior 3. Error handling logic was added to present errors to the user in a user-friendly manner, instead of silently failing 4. Event loading logic was moved into a special new thread view, which makes its own internal state evident to the user (i.e. whether the note is loading, loaded, or if the note could not be found) These changes make the URL opening logic more predictable, easy to refactor, and helps ensure the user always gets some outcome from opening a URL, even if it means showing a "not found" or "error" screen, to eliminate cases where nothing seems to happen. Closes: damus-io#2429 Changelog-Fixed: Improved robustness of the URL handler Changelog-Added: Added user-friendly error view for errors around the app that would not fit in other places Signed-off-by: Daniel D’Aquino <[email protected]>
This commit improves reliability on the handling of external URLs. This was achieved through the following improvements: 1. The URL handler interface is now well-defined, with more clear inputs and outputs, to avoid silent failures and error paths that are hard to see within convoluted logic paths 2. Side effects during URL parsing were almost completely removed for more predictable behavior 3. Error handling logic was added to present errors to the user in a user-friendly manner, instead of silently failing 4. Event loading logic was moved into a special new thread view, which makes its own internal state evident to the user (i.e. whether the note is loading, loaded, or if the note could not be found) These changes make the URL opening logic more predictable, easy to refactor, and helps ensure the user always gets some outcome from opening a URL, even if it means showing a "not found" or "error" screen, to eliminate cases where nothing seems to happen. Closes: damus-io#2429 Changelog-Fixed: Improved robustness of the URL handler Changelog-Added: Added user-friendly error view for errors around the app that would not fit in other places Signed-off-by: Daniel D’Aquino <[email protected]>
This commit improves reliability on the handling of external URLs. This was achieved through the following improvements: 1. The URL handler interface is now well-defined, with more clear inputs and outputs, to avoid silent failures and error paths that are hard to see within convoluted logic paths 2. Side effects during URL parsing were almost completely removed for more predictable behavior 3. Error handling logic was added to present errors to the user in a user-friendly manner, instead of silently failing 4. Event loading logic was moved into a special new thread view, which makes its own internal state evident to the user (i.e. whether the note is loading, loaded, or if the note could not be found) These changes make the URL opening logic more predictable, easy to refactor, and helps ensure the user always gets some outcome from opening a URL, even if it means showing a "not found" or "error" screen, to eliminate cases where nothing seems to happen. Closes: damus-io#2429 Changelog-Fixed: Improved robustness of the URL handler Changelog-Added: Added user-friendly error view for errors around the app that would not fit in other places Signed-off-by: Daniel D’Aquino <[email protected]>
This commit improves reliability on the handling of external URLs. This was achieved through the following improvements: 1. The URL handler interface is now well-defined, with more clear inputs and outputs, to avoid silent failures and error paths that are hard to see within convoluted logic paths 2. Side effects during URL parsing were almost completely removed for more predictable behavior 3. Error handling logic was added to present errors to the user in a user-friendly manner, instead of silently failing 4. Event loading logic was moved into a special new thread view, which makes its own internal state evident to the user (i.e. whether the note is loading, loaded, or if the note could not be found) These changes make the URL opening logic more predictable, easy to refactor, and helps ensure the user always gets some outcome from opening a URL, even if it means showing a "not found" or "error" screen, to eliminate cases where nothing seems to happen. Closes: damus-io#2429 Changelog-Fixed: Improved robustness of the URL handler Changelog-Added: Added user-friendly error view for errors around the app that would not fit in other places Signed-off-by: Daniel D’Aquino <[email protected]>
This commit improves reliability on the handling of external URLs. This was achieved through the following improvements: 1. The URL handler interface is now well-defined, with more clear inputs and outputs, to avoid silent failures and error paths that are hard to see within convoluted logic paths 2. Side effects during URL parsing were almost completely removed for more predictable behavior 3. Error handling logic was added to present errors to the user in a user-friendly manner, instead of silently failing 4. Event loading logic was moved into a special new thread view, which makes its own internal state evident to the user (i.e. whether the note is loading, loaded, or if the note could not be found) These changes make the URL opening logic more predictable, easy to refactor, and helps ensure the user always gets some outcome from opening a URL, even if it means showing a "not found" or "error" screen, to eliminate cases where nothing seems to happen. Closes: damus-io#2429 Changelog-Fixed: Improved robustness of the URL handler Changelog-Added: Added user-friendly error view for errors around the app that would not fit in other places Signed-off-by: Daniel D’Aquino <[email protected]>
@alltheseas, @jb55, I sent a patch via email to resolve this! Please let me know if there are any suggestions, questions, or concerns. |
@alltheseas, @jb55, I sent a patch via email to resolve this!
Please let me know if there are any suggestions, questions, or concerns.
Thank you!
haven't had the chance to test it but code and video look good! other
than that weird quirk where navigation doesn't work if you're alreay
navigating? but we can fix that later.
|
On Jan 10, 2025, at 23:33, William Casarin ***@***.***> wrote:
> @alltheseas, @jb55, I sent a patch via email to resolve this!
>
> Please let me know if there are any suggestions, questions, or concerns.
> Thank you!
haven't had the chance to test it but code and video look good! other
than that weird quirk where navigation doesn't work if you're alreay
navigating? but we can fix that later.
What weird quirk are you referring to?
|
Daniel D’Aquino ***@***.***> writes:
> > @alltheseas, @jb55, I sent a patch via email to resolve this!
> >
> > Please let me know if there are any suggestions, questions, or concerns.
> > Thank you!
>
> haven't had the chance to test it but code and video look good! other
> than that weird quirk where navigation doesn't work if you're alreay
> navigating? but we can fix that later.
What weird quirk are you referring to?
just noticed that the links weren't working when you were already
navigated somewhere, unless I'm mistaken.
|
On Sun, Jan 12, 2025 at 23:17, William Casarin ***@***.***(mailto:On Sun, Jan 12, 2025 at 23:17, William Casarin <<a href=)> wrote:
Daniel D’Aquino ***@***.***> writes:
>> > @alltheseas, @jb55, I sent a patch via email to resolve this!
>> >
>> > Please let me know if there are any suggestions, questions, or concerns.
>> > Thank you!
>>
>> haven't had the chance to test it but code and video look good! other
>> than that weird quirk where navigation doesn't work if you're alreay
>> navigating? but we can fix that later.
>
> What weird quirk are you referring to?
just noticed that the links weren't working when you were already
navigated somewhere, unless I'm mistaken.
—
Reply to this email directly, [view it on GitHub](#2429 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AF4MLDEF4Z4SNW57MRGPYXL2KJ2PVAVCNFSM6AAAAABNZDRRJGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKOBVG42TEOBXGI).
You are receiving this because you were assigned.
Ah, I believe that happened because I put the same event ID on multiple links, so the app was already at the destination in some cases, and so I had to go back to the home screen and try again to see the navigation happening
Example:
1:08: I click on a valid damus.io/nevent link, Damus navigates to the event
1:17: I click on a valid nostr:nevent link, which points to the same exact same event ID, so the app doesn't navigate to the new route, because it's already at that desired destination
1:20: I realize that fact, so I redo the test on the two links, this time moving away from the thread before clicking on the second valid link
I believe that when the user is on a thread and they click on another event link (with a different event ID), it will push that second route into the navigation stack. I believe I have seen that happening while I was dev testing, but it seems that I did not include that in my official test.
So I will double-check the behaviour in that case to be sure!
|
Daniel D’Aquino ***@***.***> writes:
>> What weird quirk are you referring to?
>
> just noticed that the links weren't working when you were already
> navigated somewhere, unless I'm mistaken.
Ah, I believe that happened because I put the same event ID on multiple links, so the app was already at the destination in some cases, and so I had to go back to the home screen and try again to see the navigation happening
Example:
1:08: I click on a valid damus.io/nevent link, Damus navigates to the event
1:17: I click on a valid nostr:nevent link, which points to the same exact same event ID, so the app doesn't navigate to the new route, because it's already at that desired destination
1:20: I realize that fact, so I redo the test on the two links, this time moving away from the thread before clicking on the second valid link
I believe that when the user is on a thread and they click on another event link (with a different event ID), it will push that second route into the navigation stack. I believe I have seen that happening while I was dev testing, but it seems that I did not include that in my official test.
So I will double-check the behaviour in that case to be sure!
ah ok, wasn't sure. can't wait to test this!
|
On Jan 14, 2025, at 00:51, William Casarin ***@***.***> wrote:
Daniel D’Aquino ***@***.***> writes:
>>> What weird quirk are you referring to?
>>
>> just noticed that the links weren't working when you were already
>> navigated somewhere, unless I'm mistaken.
>
> Ah, I believe that happened because I put the same event ID on multiple links, so the app was already at the destination in some cases, and so I had to go back to the home screen and try again to see the navigation happening
>
> Example:
>
> 1:08: I click on a valid damus.io/nevent link, Damus navigates to the event
> 1:17: I click on a valid nostr:nevent link, which points to the same exact same event ID, so the app doesn't navigate to the new route, because it's already at that desired destination
> 1:20: I realize that fact, so I redo the test on the two links, this time moving away from the thread before clicking on the second valid link
>
> I believe that when the user is on a thread and they click on another event link (with a different event ID), it will push that second route into the navigation stack. I believe I have seen that happening while I was dev testing, but it seems that I did not include that in my official test.
> So I will double-check the behaviour in that case to be sure!
ah ok, wasn't sure. can't wait to test this!
Just double-checked this, and it works! Clicking on links pointing to different events will push those new thread routes on the navigation stack as expected
|
On Jan 15, 2025, at 11:39, Daniel D’Aquino ***@***.***> wrote:
> On Jan 14, 2025, at 00:51, William Casarin ***@***.***> wrote:
>
> Daniel D’Aquino ***@***.***> writes:
>
>>>> What weird quirk are you referring to?
>>>
>>> just noticed that the links weren't working when you were already
>>> navigated somewhere, unless I'm mistaken.
>>
>> Ah, I believe that happened because I put the same event ID on multiple links, so the app was already at the destination in some cases, and so I had to go back to the home screen and try again to see the navigation happening
>>
>> Example:
>>
>> 1:08: I click on a valid damus.io/nevent link, Damus navigates to the event
>> 1:17: I click on a valid nostr:nevent link, which points to the same exact same event ID, so the app doesn't navigate to the new route, because it's already at that desired destination
>> 1:20: I realize that fact, so I redo the test on the two links, this time moving away from the thread before clicking on the second valid link
>>
>> I believe that when the user is on a thread and they click on another event link (with a different event ID), it will push that second route into the navigation stack. I believe I have seen that happening while I was dev testing, but it seems that I did not include that in my official test.
>> So I will double-check the behaviour in that case to be sure!
>
> ah ok, wasn't sure. can't wait to test this!
Just double-checked this, and it works! Clicking on links pointing to different events will push those new thread routes on the navigation stack as expected
@jb55, can I go ahead and merge this change?
Thanks!
|
What happens
When I click on Nostr URLs or on push notifications, the client intermittently does not open the url
What I expect to happen
I expect URLs to be opened flawlessly (if it cannot find an event right away, it should retry or show a loading screen)
Link to noteID, npub
N/A
Screenshots/video recording
N/A
Versions
Damus version: At least 1.10
Operating system version: At least iOS 17+
Device: Any
Steps To Reproduce
Steps to reproduce the behavior:
Additional context
The text was updated successfully, but these errors were encountered: