-
-
Notifications
You must be signed in to change notification settings - Fork 114
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
Account Switching #644
Account Switching #644
Conversation
3de1b7e
to
58ac860
Compare
There is a bug where you can't switch between anon and a user if you're currently looking at an item: https://files.ekzyis.com/public/sn/account_switching_item_bug.mp4 I'll probably need to rewrite the corresponding code which calls hooks conditionally (depending on edit: "fixed" in 0c8893c |
6b8c702
to
b7c59a1
Compare
Current state: https://files.ekzyis.com/public/sn/account_switching_002.mp4 Missing (see PR description):
|
ee772b1
to
9bbc93d
Compare
Mhh, okay, can't find how to access a response to set the required Same problem with refreshing JWTs. For some reason, the jwt callback only has access to request and response on login/signup. Putting this out of draft since everything else works, so I think we can ship as is. Update: Unfortunately, this doesn't improve anon UX a lot since you can't pay from your account balance yet. And the cookies are shared between tabs so if you switch in one tab, you also switch in the other tab (obviously). Switching to anon in one tab also currently breaks the other tab since the state is not properly updated in the other tab (educated guess). The other tab then thinks you're completely signed out. Keeping this as ready for review since I think this is still ready for review (lol) One upside is that with this feature, I can easily keep track of replies to @hn. |
components/fee-button.js
Outdated
@@ -36,8 +36,7 @@ export function postCommentBaseLineItems ({ baseCost = 1, comment = false, me }) | |||
} | |||
} | |||
|
|||
export function postCommentUseRemoteLineItems ({ parentId, me } = {}) { | |||
if (!me) return () => {} |
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 hope this doesn't break anything but afaict, it doesn't. It just does unnecessary requests since anon has fixed fees. But removing this line is required to not run hooks conditionally. When switching to anon, this line changed the hook order and thus broke this rule:
Don’t call Hooks inside loops, conditions, or nested functions. Instead, always use Hooks at the top level of your React function, before any early returns. By following this rule, you ensure that Hooks are called in the same order each time a component renders. That’s what allows React to correctly preserve the state of Hooks between multiple useState and useEffect calls. (If you’re curious, we’ll explain this in depth below.)
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 isn't a hook though. It's a function that returns a hook
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.
It's a function that returns a hook
Yes but conditionally
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.
User reported this hook error in prod on telegram. I'm not sure if it's related but I believe this is already conditional
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.
but I believe this is already conditional
Yes, it is. That's what my change is trying to fix since the bug would manifest when switching to anon. The user report shows that the bug already manifests in some cases.
Example:
This is the full function:
export function postCommentUseRemoteLineItems ({ parentId, me } = {}) {
if (!me) return () => {}
const query = parentId
? gql`{ itemRepetition(parentId: "${parentId}") }`
: gql`{ itemRepetition }`
return function useRemoteLineItems () {
const [line, setLine] = useState({})
const { data } = useQuery(query, SSR ? {} : { pollInterval: 1000, nextFetchPolicy: 'cache-and-network' })
useEffect(() => {
const repetition = data?.itemRepetition
if (!repetition) return setLine({})
setLine({
itemRepetition: {
term: <>x 10<sup>{repetition}</sup></>,
label: <>{repetition} {parentId ? 'repeat or self replies' : 'posts'} in 10m</>,
modifier: (cost) => cost * Math.pow(10, repetition)
}
})
}, [data?.itemRepetition])
return line
}
}
return function useRemoteLineItems ()
is a hook which is only returned if we don't already return () => {}
. That's what I meant with "but conditionally":
It's a function that returns a hook
Yes but conditionally
So yes, this is conditional. That the user reported it means that the bug actually manifests already, too. Not only with my changes, assuming I wouldn't remove this line: if (!me) return () => {}
(which what this conversation here is about)
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 for the detailed and thus maybe snarky answer, I feel like we've been talking past each other for several replies already now, lol :)
Reminder for myself: check if there is still an error (there probably is) if you're on |
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.
Looks great!
I added some high level remarks. After we talk/work through those, I'll do another more detailed pass.
acb1c96
to
8aef88e
Compare
I'll fix the conflicts, give me a sec |
902db6e
to
20eb1ff
Compare
No rush. This won't until I ship territories first |
This is going to be great for my new puppet account @oracle 👀 |
For some reason this doesn't work for me:
This might need migration logic to be added for stackers that are already logged in if that wasn't tested. broken.mov |
5e7eab0
to
20eb1ff
Compare
Mhh, I had this error before. I think I fixed it by clearing all my session cookies since it's related to a (new) cookie being HTTP only when it should not have been set as HTTP only for some reason iirc. I think I also noticed that sometimes, cookies are not overridden but duplicated. This should only happen if the cookie path is different - it defaults to the current path if no path is specified (which should no longer be the case). I don't remember 100% though. Can you show me your cookies when this bug happens to you?
I wasn't able to hook into the OAuth login process and Email auth (and was hard to test locally). So I thought - at least for the initial version - we can provide only Login with Lightning and Nostr if people want to use account switching.
Mhh, I didn't think too much about this but I think this should be 100% backwards compatible. But I will test (again). |
74771a4
to
20eb1ff
Compare
Did a full test (afaict) again. https://files.ekzyis.com/public/sn/account_switching_full_test.mp4
|
Putting in draft until bug is fixed Edit: mhh, can't find how to put in draft in the Github app |
Sessions that existed before we deployed account switching should now also be able to use account switching immediately. We now sync the cookie from their session if there is no 2023-12-20.16-21-43.mp4 |
So this is out of draft now? |
Going to fix conflicts now, just had a call with someone from Alby btw, regarding this:
I noticed that sometimes, it just takes some time (max 5 seconds) for the switch to anon to complete. Not sure if it's because of dev mode or network latency or something else (bug). But yes, in your video, it didn't look like anything is going to happen, no matter how long you wait. Would be interesting to see if update: testing currently again after fixing conflicts |
The previous commit broke the UI update after anon zaps because we actually updated item.meSats in the cache and not item.meAnonSats. Updating item.meAnonSats was not possible because it's a local field. For that, one needs to use reactive variables. We do this now and thus also don't need the useEffect hack in item-info.js anymore.
If we logged in but never switched to any other account, the 'multi_auth.user-id' cookie was not set. This meant that during logout, the other 'multi_auth.*' cookies were not deleted. This broke the account switch modal. This is fixed by setting the 'multi_auth.user-id' cookie on login. Additionally, we now cleanup if cookie pointer OR session is set (instead of only if both are set).
setState is stable and thus only noise in effect dependencies
4d725b6
to
ff378fb
Compare
I've created #1386 to make the change set here a little smaller. Another repetitive change is replacing Here is the list of files sorted by their changes:
Most important changes are all the files above |
Mhh, not really intentional, I just didn't think about closing or not.
I think it's related to if you clicked the link before or not |
It didn't change before or after I clicked it strangely. |
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 got this when switching while editing a post.
Keeping the page the same while switching is a pretty difficult requirement. There might be other such instances.
If it were me I'd probably just go nuclear do a refresh when switching ... so that we don't have to think of account switching anytime we add a personalized page.
For reference (not saying this exactly what we should do), when you switch on X it takes you to For us, I think it might be better to just refresh on switch. |
Mhh yes, getting a hold of these cases turned out to be the most difficult part, not the actual switch, lol.
Yeah, I think that's the right call after I explored that troublesome path of having a nice switch experience ("here be dragons"-like). I'll push that change in a bit.
Ohh, so a refresh on the same page would still be better than X (took me a second to understand you mean Twitter) |
There you go, the modal is now also closed on switch, lol |
a6555ff
to
56b8431
Compare
56b8431
to
cc18fc1
Compare
Closes #489
This PR implements account switching:
Implementation details
If a user wants to add an account, they are redirected to
/login
withmultiAuth
param set:components/switch-account.js:
With this param, a login flow within a session can be initiated. This param is checked in the backend to only set cookies without actually switching the user (
return token
instead ofreturn user
):With these
multi_auth.*
cookies set, we can now show accounts to the user to which they can switch. Themulti_auth
cookie is not HTTP only since we want to access it from JS. This shouldn't be a problem since this is only for reading which accounts can be switched to and render this view:
On click, another cookie
multi_auth.user-id
is created via JS. This cookie is read inside a middleware in the backend to replace the session cookie that will be used to determine the user in the backend:middleware.js:
Showcase
2023-11-19.02-25-56.mp4
TODO
multiAuth
param to all login methodsemail auth(decided to remove this auth method, see Account Switching #644 (comment))github auth(decided to remove this auth method, see Account Switching #644 (comment))twitter auth(decided to remove this auth method, see Account Switching #644 (comment))add account
imagemultiAuth
is used)also refresh JWTs in(?) (how?)multi_auth.*