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

Replace updateCurrentAccount() with refreshSession() #3910

Merged
merged 1 commit into from
May 8, 2024
Merged

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented May 8, 2024

Extracted-ish from #3728.

When you change email, handle, emailConfirmed, or other fields present in the session object, it would previously get updated in local state. However, the underlying agent.session was not updated. So there could be cases where an agent.session would reflect back into the app state and clobber it (despite being out of date).

Ideally the agent should take care of updating its own agent.session (and emitting an update event) when we call API methods that are known to mutate email, handle, etc. Until it does that, we're going to explicitly call agent.resumeSession in those places to force a session refetch which will go through the usual mechanism. We don't need any special state handling because it'll be handled in our existing handler for the update event.

Since we don't need any special handling for the state, and because most likely this logic will later move into the agent itself anyway, I ripped updateCurrentAccount out of the session API interface.

Test Plan

Changed email, verified email, changed handle, changed 2FA setting. Everything seems to be reflected in the UI.

These "feel" a bit slower (due to a serial request) but we should really fix that in the agent itself. I think it's OK. We can also add better pending states (e.g. disable the 2FA switch while toggling it), but I didn't do it in this PR.

Replace updateCurrentAccount() with resumeSession()
Copy link

render bot commented May 8, 2024

Copy link

github-actions bot commented May 8, 2024

Old size New size Diff
6.89 MB 6.89 MB -422 B (-0.01%)

@gaearon gaearon merged commit 0c6bf27 into main May 8, 2024
6 checks passed
@gaearon gaearon deleted the rm-updated branch May 8, 2024 22:11
@gaearon gaearon mentioned this pull request May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants