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(signin-loading): removed auth middleware #4936

Closed
wants to merge 12 commits into from
Closed

Conversation

ThomBos
Copy link
Contributor

@ThomBos ThomBos commented Sep 19, 2022

What this PR does πŸ“–

  • fixes "forever linking" issue

Which issue(s) this PR fixes πŸ”¨

Special notes for reviewers πŸ—’οΈ

Additional comments 🎀

@ThomBos ThomBos marked this pull request as draft September 19, 2022 20:57
@github-actions github-actions bot added the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Sep 19, 2022
@stavares843
Copy link
Member

since the last deploy i haven't had that linking issue

@stavares843 stavares843 added the draft A developer wants eyes on this PR, but they don't think it's ready to merge. label Sep 19, 2022
@ThomBos ThomBos force-pushed the fix-signin-loading branch 3 times, most recently from c63f5a3 to cb49ce7 Compare September 21, 2022 15:37
@ThomBos ThomBos marked this pull request as ready for review September 21, 2022 15:37
@ThomBos ThomBos removed the draft A developer wants eyes on this PR, but they don't think it's ready to merge. label Sep 21, 2022
Copy link
Member

@stavares843 stavares843 left a comment

Choose a reason for hiding this comment

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

Captura de ecrã 2022-09-21, às 21 37 47

when using this PR, it goes right away to this page, instead of the create account view

and then if you try to create an account, it returns an error because the user didn't have the previous view to setup a PIN

Captura de ecrã 2022-09-21, às 21 38 47

@stavares843 stavares843 added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. do not merge and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Sep 21, 2022
@stavares843
Copy link
Member

can we reuse the same logic we did here instead? when we were using solana, we added a modal that would appear after 30s, can we reuse that but for the nodes timeout?

@ThomBos
Copy link
Contributor Author

ThomBos commented Sep 22, 2022

when using this PR, it goes right away to this page, instead of the create account view

and then if you try to create an account, it returns an error because the user didn't have the previous view to setup a PIN

oh yeah, didn't noticed when refreshing, this happens the first time you open the app.
i'm on it

@stavares843
Copy link
Member

is this ready to review again? @ThomBos πŸ”¨

@ThomBos
Copy link
Contributor Author

ThomBos commented Sep 22, 2022

is this ready to review again? @ThomBos πŸ”¨

yes, check if you see any issue πŸ‘

@stavares843 stavares843 added Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Sep 22, 2022
components/views/friends/friend/Friend.vue Outdated Show resolved Hide resolved
components/views/friends/search/Search.vue Outdated Show resolved Hide resolved
@molimauro molimauro added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Sep 23, 2022
@ThomBos ThomBos removed the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Sep 23, 2022
@ThomBos
Copy link
Contributor Author

ThomBos commented Sep 27, 2022

Is there a way we could approach more simply that doesn't spread copy pasted code across so many different components?

Perhaps adding a check on the Global.vue component (which is rendered in desktop and mobile layouts. we can add it to default as well). It could determine if iridium is ready on mount, then kick the user back to the main login page if they aren't on another auth page

Everytime we call "loadaccount" the catch is necessary, that's why that pasted code can't be removed. Maybe making changes on Global.vue allows code to be removed from layouts components πŸ‘

@molimauro
Copy link
Contributor

Why do we want to ditch the middleware? Any particular reason?

@ThomBos
Copy link
Contributor Author

ThomBos commented Sep 27, 2022

Why do we want to ditch the middleware? Any particular reason?

@aewing was it to make it more simple, or you had something to integrate?

@ThomBos
Copy link
Contributor Author

ThomBos commented Sep 27, 2022

Is there a way we could approach more simply that doesn't spread copy pasted code across so many different components?

Perhaps adding a check on the Global.vue component (which is rendered in desktop and mobile layouts. we can add it to default as well). It could determine if iridium is ready on mount, then kick the user back to the main login page if they aren't on another auth page

Can tell you it didn't change when putting this code in Global.vue

@aewing
Copy link
Contributor

aewing commented Sep 29, 2022

Why do we want to ditch the middleware? Any particular reason?

We see some strange redirect loops and errors with the middleware approach. I think throwing a catch-all solution at this isn't necessarily the answer. We should be able to implement a view on the homepage (/) that checks for stored pin & attempts to restore, failing that it could redirect to the import/create account screen, which can redirect to profile entry if one isn't present. We shouldn't need to globally check state every time it changes if we think about when to redirect the user in the UI.

@github-actions github-actions bot added the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Oct 3, 2022
@ThomBos
Copy link
Contributor Author

ThomBos commented Oct 3, 2022

Why do we want to ditch the middleware? Any particular reason?

We see some strange redirect loops and errors with the middleware approach. I think throwing a catch-all solution at this isn't necessarily the answer. We should be able to implement a view on the homepage (/) that checks for stored pin & attempts to restore, failing that it could redirect to the import/create account screen, which can redirect to profile entry if one isn't present. We shouldn't need to globally check state every time it changes if we think about when to redirect the user in the UI.

What if the user doesn't enter the homepage? (like he enters "/chat/xyz" directly in the url)

@ThomBos ThomBos removed the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Oct 4, 2022
@github-actions github-actions bot added the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Oct 4, 2022
@molimauro
Copy link
Contributor

I think we should consider closing this one and:

  • introducing a timeout for the signin starting from what we have in dev
  • keep the middleware (its purpose is to run before every page so more reliable then a custom one) but refactor it since it is based on a old logic

Let me know your thoughts!

@josephmcg
Copy link
Contributor

yeah it seems like we could write some middleware that checks your current route + iridium status and knocks back appropriately.

According to the doc it runs before render
https://nuxtjs.org/docs/directory-structure/middleware/

@ThomBos
Copy link
Contributor Author

ThomBos commented Oct 6, 2022

I think we should consider closing this one and:

  • introducing a timeout for the signin starting from what we have in dev
  • keep the middleware (its purpose is to run before every page so more reliable then a custom one) but refactor it since it is based on a old logic

Let me know your thoughts!

I agree, removing middleware takes too much work to get the same behaviour and isn't as reliable (at least with this code)

@molimauro
Copy link
Contributor

Opened the tickets for those, closing this one

@molimauro molimauro closed this Oct 7, 2022
@josephmcg josephmcg deleted the fix-signin-loading branch October 14, 2022 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge blocked Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add timeout to 'linking satellites'
5 participants