-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Android: fix GetMissingOnyxUpdates in the background #40022
Conversation
This is nearly ready however there are some changes to the OnyxUpdateManager being prepared in #fireroom-2024-04-01-chats-do-not-load that will cause some conflicts so I'll hold until those are done. |
…round-missing-updates
We have another related ongoing PR here #39683 |
Thanks for the heads up that will probably cause some conflicts. I'm not sure which should be merged first. This one is ready to go though. |
We can merge this one first. The other one is more risky |
Feel free to review if you're available @rushatgabhane |
Reviewer Checklist
Screenshots/VideosAndroid: NativeWhatsApp.Video.2024-04-17.at.21.19.02.mp4Android: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
sorry im not receiving notifications on android dev build. i tried
asked for help here - https://expensify.slack.com/archives/C01GTK53T8Q/p1713298080966799 |
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.
LGTM
@@ -132,29 +132,23 @@ function saveUpdateInformation(updateParams: OnyxUpdatesFromServer) { | |||
* This function will receive the previousUpdateID from any request/pusher update that has it, compare to our current app state | |||
* and return if an update is needed | |||
* @param previousUpdateID The previousUpdateID contained in the response object | |||
* @param clientLastUpdateID an optional override for the lastUpdateIDAppliedToClient |
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.
What's the purpose of this change?
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 explained in the description above. In the background, Onyx.connect
in this module doesn't run so we need to pass the value for lastUpdateIDAppliedToClient
as a param.
@@ -597,7 +597,7 @@ function subscribeToUserEvents() { | |||
updates: pushJSON.updates ?? [], | |||
previousUpdateID: Number(pushJSON.previousUpdateID || 0), | |||
}; | |||
OnyxUpdates.applyOnyxUpdatesReliably(updates); | |||
applyOnyxUpdatesReliably(updates); |
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'm just curious: why'd we move this into its own file?
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.
Also explained in the description 😄
✋ 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 production by https://github.com/mountiny in version: 1.4.64-6 🚀
|
// When ONYX_UPDATES_FROM_SERVER is set, we pause the queue. Let's unpause | ||
// it so the app is not stuck forever without processing requests. | ||
SequentialQueue.unpause(); | ||
console.debug(`[OnyxUpdateManager] Ignoring Onyx updates while OpenApp hans't finished yet.`); |
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.
*hasn't 🤷
cc @danieldoglas
Details
Fixes an issue with our background process on Android quitting before onyx updates can finish processing in the background. This is accomplished by
handleOnyxUpdateGap()
and running it synchronouslylastUpdateIDAppliedToClient
from Onyx since otherOnyx.connect
callbacks do not run in the backgroundAdditionally,
OnyxUpdates.applyOnyxUpdatesReliably
was moved to it's own lib to avoid a dependency cycle between OnyxUpdates.ts and OnyxUpdateManager.tsFixed Issues
$ #39544
PROPOSAL: #39544 (comment)
Tests
Offline tests
None
QA Steps
None
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