-
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
Fix replay effect #16545
Fix replay effect #16545
Conversation
Adding myself as a reviewer since I'm interested in this change 👀 |
@aimane-chnaif @cristipaval One of you needs to 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] |
Woohoo this is ready for review @jasperhuangg! |
Bump @aimane-chnaif, @cristipaval |
started reviewing |
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.
Few minor changes, overall looks really good haven't had a chance to test yet tho
Updated, thanks for the reviewing Jasper! Can we get this merged today? It's been up for review for almost a week. @aimane-chnaif will you be able to finish your review today? @cristipaval are you planning to review? |
@aimane-chnaif the Web-E PR is held on this PR. Please only run the tests without the backend changes, or message one of the internal employees on this PR and we can set up a ngrok url for you to hit our dev environment with the backend changes. Please review again today. |
@neil-marcellini ok, does #16545 (comment) also require backend deploy? |
Yes. The replay effect isn't fixed for any case without the backend portion as well |
Looks good so far "without backend changes". One thing I noticed that this code is run once more unnecessarily at the end after bulk requests (so 2 times total) @neil-marcellini please pull main as branch is very behind |
Can anyone please help me on this to test with backend changes from the Web-Expensify PR? |
@jasperhuangg says he will help you test. I'm going to merge main now. I'll also look into why the finally block runs twice. |
@aimane-chnaif I added some console logs and did some debugging. I think you probably encountered this in the case of the Response Onyx updates work test right? Regardless I tested by opening a single report once. What I found is that we make two identical API.write calls to OpenReport when the report screen updates and when the ReportActionsView mounts. However, looking at the Network tab I only saw one request get made, which means we are actually de-duplicating identical requests somewhere. When we finishing processing the first request we remove all persisted requests here that are equal to the request we want to remove. Since we have two identical OpenReport requests they both get removed as you can see here. Full OpenReport logs
So that's super interesting, thank you for pointing that out! The end result is that the SequentialQueue only processes one request but it still calls flush and process twice, which calls this finally block twice. Flush is called twice because every time we push a request we eventually call flush, and for the second request we call flush once the queue is ready here. I'll think about how to fix this, but it's an existing problem and probably not a blocker for this PR. |
@aimane-chnaif @jasperhuangg Ok that's resolved now, we only call process and update Onyx when there are actually requests remaining to be processed. I tested and verified that the duplicate calls to OpenReport only result in Onyx.update in the finally block getting called once. |
I've performed the back-end changes, they all work as expected! Ignore the Onyx error with creating a workspace, that's a different, unrelated error: Screen.Recording.2023-05-04.at.8.44.26.PM.mov |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-05-04.at.8.44.26.PM.movMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
oh great! I will also test in a few hrs against ngrok url you shared. |
@aimane-chnaif cool, I was only able to test against Web, so feel free to update my comment above with tests on the other platforms, thanks! |
@jasperhuangg I tried with your ngrok url but stuck on login. I never received email with magic code. |
Thanks so much @jasperhuangg! |
✋ 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/neil-marcellini in version: 1.3.11-0 🚀
|
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.
@neil-marcellini can you please add app issue link to Fixed Issues
section?
Sorry which App issue? |
Nvm, you already answered to #12775 (comment) |
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.12-0 🚀
|
@neil-marcellini is the backend PR for this issue merged and deployed to staging? I think it might resolve another issue caused by the replay effect but cannot test without backend changes as you have mentioned above. |
@Nikhil-Vats the backend PR is not on staging yet. We'll keep you updated in the tracking issue #12775 |
Thanks @neil-marcellini. I asked because I found out that the root cause of this issue #18492 is also same. The emojis appear correctly first but then disappear because of the pusher events received. Although what I didn't understand was why the pusher events are always out of order, the response from the first API call is always returned last due to which the emojis on the UI are wrong (only 1 emoji is added since the first AddEmojiReaction pusher event was received at the end and it has only 1 reaction). |
@@ -13,23 +15,27 @@ function SaveResponseInOnyx(response, request) { | |||
return; | |||
} | |||
|
|||
// For most requests we can immediately update Onyx. For write requests we queue the updates and apply them after the sequential queue has flushed to prevent a replay effect in | |||
// the UI. See https://github.com/Expensify/App/issues/12775 for more info. |
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.
fwiw it is easy for people to find linked issues by using git blame. usually there is no need to add a link to a github issue.
Details
The replay effect happens when a user takes multiple actions while offline, the requests are queued up in the SequentialQueue, and then they go back online and each request is processed on at a time. Since optimistic data has already been set while offline, each Pusher event that arrives updates Onyx with outdated data, causing the offline actions to "replay".
In order to fix the replay effect from the App side we are first sending the Pusher
socket_id
with all requests. Then, while the SequentialQueue is processing, create a list of Onyx updates from each response. Once the SequentialQueue is empty, batch update Onyx. Together with the backend changes to send thesocket_id
with Pusher events, preventing them from being delivered to the requesting client, and sending Onyx updates for the requesting client back in the response, the replay effect is solved.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/270219
PROPOSAL: GH_LINK_ISSUE(COMMENT)
Tests
Without backend changes
Response Onyx updates work
lastReadTime
.queuedOnyxUpdates
key is empty in the Onyx DB: Application > IndexedDB > OnyxDB > keyvaluepairs > Search forqueuedOnyxUpdates
and verify that nothing comes upPusher Onyx updates work
Offline tests
Without backend changes
Send offline messages
With backend changes from the Web-Expensify PR
Workspace rename
New chat
Workspace member
QA Steps
Most of the QA will be handled with the backend PR QA, so just run the Pusher Onyx updates work and Send offline messages tests.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
The changes are platform independent so I only tested on web.
Web
Without backend changes
Response Onyx updates work
response-onyx-updates.mov
Pusher Onyx updates work
pusher-onyx-updates.mov
Send offline messages
offline-messages.mov
With backend changes from the Web-Expensify PR
Workspace rename
replayA.mov
New chat
replay-new-chat.mov
Workspace member
replay-member.mov
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android