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.

Sign up for GitHub

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

Jump to bottom

Set the leader on Page Focus #2954

Merged
merged 1 commit into from
May 18, 2021
Merged

Set the leader on Page Focus #2954

merged 1 commit into from
May 18, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented May 16, 2021 •
edited
Loading

Details

When page focus changes, we set the focused client as Client leader.

  1. Listen for AppState#change on Expensify.js.
  2. When change emit , we call client.init on Visibility.isVisible(). This makes sure that we always have an active leader.

Fixed Issues

Fixes #2711

Tests / QA Steps

  1. Open the Expensify.cash app on the browser.
  2. Now open another tab with Expensify.cash app.
  3. focus over the new tab, then focus back on the old tab.
  4. Now without going to the new tab just close it from the top bar.

Screenshot 2021-05-06 11:42:26

  1. Now send messages to one of the not active chats from another users' device.
  2. We should receive the notifications.

Tested On

  • Web (affected Platform)
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Before
noti-web.mp4
After
noti-fix-web.mp4

@parasharrajat parasharrajat marked this pull request as ready for review May 16, 2021 20:04
@parasharrajat parasharrajat requested a review from a team as a code owner May 16, 2021 20:04
@MelvinBot MelvinBot requested review from nickmurray47 and removed request for a team May 16, 2021 20:05
@parasharrajat
Copy link
Member Author

Ready to roll.

@parasharrajat
Copy link
Member Author

@roryabraham It seems you are the in-charge here.

roryabraham
roryabraham approved these changes May 18, 2021
View reviewed changes
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Nice, these changes are pretty simple and tested well. LGTM 👍

@roryabraham
Copy link
Contributor

All yours @nickmurray47

@nickmurray47 nickmurray47 merged commit 418add9 into Expensify:main May 18, 2021
@OSBotify OSBotify mentioned this pull request May 18, 2021
Closed
26 tasks
@parasharrajat parasharrajat deleted the activeClient branch November 20, 2023 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickmurray47 nickmurray47 nickmurray47 approved these changes

@roryabraham roryabraham roryabraham approved these changes

Assignees

@parasharrajat parasharrajat

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No client is set as leader to perform tasks.
3 participants
@parasharrajat @roryabraham @nickmurray47