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

Don't clear toasts when changing users #3843

Merged
merged 5 commits into from
May 3, 2024
Merged

Don't clear toasts when changing users #3843

merged 5 commits into from
May 3, 2024

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented May 3, 2024

While working on the session code I've noticed we have a bunch of weird setTimeouts around toasts, and that toasts work poorly when switching accounts in general. Turns out, it's because we unmount them.

In this PR, I've relaxed it so that toasts are placed outside the area that gets remounted on user change. As a result, I was able to remove the fragile setTimeout hacks. See individual commits.

Reviewing

  • dcb1475?w=1: Puts legacy theme provider next to Alf provider.
  • 38a02b8?w=1: Moves RootSiblingParent upwards. This is where toasts get mounted on native. I'll assume we only use it for toasts so it's fine to bleed previous users' UI on account change.
  • b54e358?w=1: On web, we use this to render toasts. Make sure it also doesn't get remounted.
  • 1d8ad44?w=1: We can remove the hacks now.
  • 06e20b2: Our session code does emitSessionDropped in the onAgentSessionChange handler. It seems like it makes sense to show a toast whenever that happens, regardless of the reason (since it can happen out of the blue too). The native App.native.tsx component already does this by listenSessionDropped so this copy-pastes the same approach into App.web.tsx. I presume it wasn't previously enabled due to the remounting issues.
    • This also lets us remove the toast in useAccountSwitcher. It will be taken care of by the emitSessionDropped logic. In fact, this toast isn't even being used yet anyway. It was added in c020a4c#r1584118170 but not hooked up (cause initSession never throws atm), and I think we don't need it now either due to the new strategy.

Test Plan

Switch accounts on iOS and web. Observe the toast showing up. Try doing it via Settings, via quick switcher, and via the Choose Account screen.

Force session expiry on web using the trick I described in the test plan for #3838. Observe the expiry toast showing up.

Copy link

render bot commented May 3, 2024

Copy link

github-actions bot commented May 3, 2024

Old size New size Diff
6.86 MB 6.86 MB 70 B (0.00%)

Copy link
Member

@mozzius mozzius left a comment

Choose a reason for hiding this comment

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

Makes sense, and looks good

@gaearon gaearon merged commit 85b3441 into main May 3, 2024
6 checks passed
@gaearon gaearon deleted the fix-toasts branch May 3, 2024 15:37
Copy link
Collaborator

@pfrazee pfrazee left a comment

Choose a reason for hiding this comment

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

Oh nice

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.

4 participants