-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add check if client is active before applying pusher updates #39990
Add check if client is active before applying pusher updates #39990
Conversation
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 assume active client is the most recent tab or Desktop app? It seems like we already rely on this function and that it's not used for mobile 👍
- Given this is relatively critical I'd suggest we get a C+ review
Yes, most recent tab - in desktop/android/iOS this won't really matter since you'll always be on the most recent one. It also doesn't compare cross clients, only in the same client that shares the storage. |
Taking over! |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeScreen.Recording.2024-04-10.at.2.07.00.PM.moviOS: NativeScreen.Recording.2024-04-10.at.2.46.01.PM.moviOS: mWeb SafariScreen.Recording.2024-04-10.at.1.58.28.PM.movMacOS: Chrome / SafariScreen.Recording.2024-04-10.at.1.45.10.PM.movMacOS: DesktopScreen.Recording.2024-04-10.at.1.55.57.PM.mov |
Thanks for clarification, I was wondering if an edge-case would occur for internal users with Desktop prod and staging |
It should not, since those clients will have separate storage |
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 good!
@johnmlee101 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Hey @allroundexperts, did you test multiple tabs for web/mWeb? |
@Julesssss yep: 321174966-10869b8f-0f59-4cd2-ad1d-abb1ef13c043.mov |
It kinda looks like one dev tab, one staging tab to me |
There are 3 tabs. Two dev, and 1 staging. The console log appears on the first tab, while second tab shows the message. At around 19th second, I'm switching back to the first tab (which was not active). |
Ah thanks for the explanation 👍 |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.4.62-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.4.62-0 🚀
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.4.62-0 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.4.62-17 🚀
|
@@ -30,6 +31,10 @@ Onyx.connect({ | |||
*/ | |||
export default function subscribeToReportCommentPushNotifications() { | |||
PushNotification.onReceived(PushNotification.TYPE.REPORT_COMMENT, ({reportID, reportActionID, onyxData, lastUpdateID, previousUpdateID}) => { | |||
if (!ActiveClientManager.isClientTheLeader()) { |
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.
Just an FYI, mobile is always the leader (there's only one client) so we'll never hit this code block
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.
Ok. This is not creating a problem, right?
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.
Nope just noticed this while refactoring. I'll probably remove it.
Details
Problem
When we have multiple tabs open on new dot, all of them receive pusher updates. That's causing confusion because we're trying to apply them all, so depending on the tab you are, you'll see that they were applied, or that we've skipped them because they were already applied (in another tab).
This is also a problem when you receive an update that you are not up to date. If that happens, we will trigger N calls to GetMissingOnyxMessages.
a. Tab A receives it first, so it applied the update
b. Tab B and Tab C will log Update received was older than or the same as current state, returning without applying the updates other than successData and failureData
a. All 3 tabs will receive that update
b. All 3 tabs will call saveUpdateInformation after checking that the previousUpdateID do not match
c. Because we execute the call for GetMissingOnyxUpdate on a callback of updates on the key ONYX_UPDATES_FROM_SERVER, and we updated that key 3 times on 3.b, each tab will call GetMissingOnyxUpdates 3 times (for a total of 9 calls to our servers)
We've solved issue 3.c on PR #39943, but we were still doing 3 requests (because we triggered the call back 3 times). This PR is solving that.
Fixed Issues
$ #39992
$ https://github.com/Expensify/Expensify/issues/386379
Tests
Open the app in 2 separate tabs and open the console
Receive a message on the account logged in those tabs
Confirm you can find logs with
but ignoring it since this is not the active client
in one of the clientsConfirm that the message is visible in both clients
Verify that no errors appear in the JS console
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop