App
App copied to clipboard
Expensify
[Tracking] "Replay effect" when sequential actions are taken offline, and user subsequently comes online
Problem
With Pattern B, offline creation events trigger a "pending" action that clears when the app comes back online. Similarly for deleting or removing content, a "strikethrough" action appears while offline and clears when the app comes back online. The pickle is that if a user takes a creation and deletion action while offline – whether that be uploading/removing an attachment, sending/deleting a message, adding/removing workspace members – content will flash or "replay" because actions in our app happen sequentially.
Here's an example of how this "replay" happens in practice:
- User invites a workspace member offline, we show it as pending to create
- User removes a workspace member offline, we show it as pending to delete
- When the User comes online, we remove the row optimistically because the last action taken was to delete
However, again because of sequentiality, we see flashing in practice. The create action will run first, returning the data and showing the row. Next, the deleted action runs and we remove the row. Here's another pickle: when the creation action is run, we also send an email inviting the workspace member, but then that email is immediately invalidated. Confusing right?
tldr: When a user takes multiple write actions affecting the same Onyx key while offline and then goes back online, the actions are “replayed” which causes the UI to update unnecessarily and can cause flickering. We have already tracked 5 issues related to this problem in the App.
Solution
~~1. https://github.com/Expensify/Expensify/issues/270217 react-native-onyx: Create Onyx.batchUpdate which will do the same thing as Onyx.update, except that it won't notify subscribers until the last update is applied.~~ Not necessary, Onyx already batches updates.
2. https://github.com/Expensify/Expensify/issues/270219
2.1. App: Send the Pusher socket_id with all requests.
2.2. App: [HOLD 1] While the SequentialQueue is processing, create a list of Onyx updates from each response. Once the SequentialQueue is empty, batch update Onyx.
3. https://github.com/Expensify/Expensify/issues/270220
3.1. Web-Expensify Send the Pusher socket_id with all Pusher events, preventing them from being delivered to that client.
3.2. Web-Expensify [HOLD 2] Modify our api to return all Onyx updates in the response that were pushed to the requesting accountID.
Known Issues
- https://github.com/Expensify/App/issues/12743
- https://github.com/Expensify/App/issues/12000
- https://github.com/Expensify/App/issues/13310
- https://github.com/Expensify/App/issues/15550
- https://github.com/Expensify/App/issues/15641
- https://github.com/Expensify/App/issues/16530
- https://github.com/Expensify/App/issues/13250
- https://github.com/Expensify/App/issues/17110
- https://github.com/Expensify/App/issues/17081
- https://github.com/Expensify/App/issues/16776
- https://github.com/Expensify/App/issues/17197
- https://github.com/Expensify/App/issues/16728
Upwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~0110088603c45991d5
- Upwork Job ID: 1625975375525715968
- Last Price Increase: 2023-02-15
I created this issue last week, but it's honestly pretty annoying to have this in the queue given all the immediate work we have in front of us elsewhere. I want to believe that we can come back to this one and remember this issue/convo, though curious for your thoughts.
It's a tracking issue, why does it bother you that exists? We don't have to work on it now...
~Take a seat on this chaise longue, Jason. Talk to me... tell me how it makes you feel?~
I'm with Ioni. I think it's fine, let's put the planning label on it and circle back to it around N7.
I think that this issue with out of order messages when creating 2 bill splits while offline is another instance of this bug. Therefore, I'm going to hold it on this issue.
Thanks! I'm actually going to have that issue closed soon, and the part where the messages are out of order will be fixed in https://github.com/Expensify/App/issues/13310. I'll update the description to link that instead.
We have this held on WAQ, so that should be removed now, but do we want to progress with finding a solution to this atm though?
I vote that we start working on a fix now. It feels like a liability to have a bug in one of our most common offline patterns.
I tend to agree we should figure out how we're going to plug this gap to improve the handling of adding/removing something offline. The "flashing" effect feels like a bug, though it is by design I believe, so technically it's still a New feature right?
though it is by design I believe, so technically it's still a New feature right?
yeah, kind of by design.
Great point. I actually like coming back to this, as I agree that it upholds a level of polish that we should aim for.
Nice let's do it! Neil and Ioni, are you guys looking to collab on it?
Triggered auto assignment to Contributor Plus for review of internal employee PR - @aimane-chnaif (Internal)
Nice let's do it! Neil and Ioni, are you guys looking to collab on it?
I have no ideas on how to solve this issue. I thought about not updating any UI while we are going through the queue, but that sounds like a bad idea, as it would make the UI unresponsive till the queue is finished.
Hmm yeah I agree with that. It'd make the app feel like it was frozen when it's not.
Yeah, it might feel like there's a big delay in the response after coming back online. Is there no way for us to cancel something in the queue if it's effectively a revert of the same UI element? I.e adding/removing a member, changing the priority mode from most recent/focus/most recent.
I had an idea to ignore the Pusher update from the client that made the request. IIRC the Pusher updates that are sent back cause the problem. We should confirm this.
For example if I go offline and flip a toggle on, off, on, then go back online, the Pusher updates will come back with on, off on, resulting in the toggle turning off and back on for me. If we were to ignore those updates because they originated from the same client that is receiving the updates, then the problem would be solved.
If I was simultaneously logged into another device and looking at the same page then I would see the toggle flip on, off, and on when the other device went online, but that makes sense because I'm seeing the actions they took while offline.
What do you guys think of that plan? It might be better to discuss in Slack.
Wow that's pretty interesting, I like it. I think I agree though, let's confirm with the broader group in Slack.
I don't think we can do that. The optimistic data is just that, the data we believe the server will return, but the server can (and will) return different data, data that we need to merge.
I don't think we can do that. The optimistic data is just that, the data we believe the server will return, but the server can (and will) return different data, data that we need to merge.
That's true, but maybe we can mark specific values to ignore pusher updates for. I'm changing this to a weekly. On Friday I will do some investigation into the known issues and raise a discussion in Slack.
I only had time for one small investigation, which was confirming that if you send two messages quickly on a slow connection then they will appear out of order when the first pusher update comes back. I think this is significant because I don't think we could merge requests for the same command together in the Network queue, since the first request could be in flight when the second request comes in.
On Monday I'll investigate more and create the discussion.
I never got around to creating that discussion, but I did do some more investigation.
For the issue where messages are briefly out of order after going online, I confirmed that it doesn't occur if we don't send the Pusher update. Similarly for adding and removing a workspace member, we won't have a reply effect without the Pusher data. I think we can ignore the Pusher update without any problems as long as there is a read api command on the page where the request is made, which ensures that the data is up to date with the server eventually.
I don't think we can do that. The optimistic data is just that, the data we believe the server will return, but the server can (and will) return different data, data that we need to merge.
Could you describe a case where the server returns different data that we must be aware of right away? If we still accept updates sent from other users / clients then we will be aware of important changes.
A potential solution would be to generate a NewDot clientID and send it with all api requests. From the API side, send the clientID back in each Pusher event. When NewDot receives Push events from the same clientID it will discard them but it will accept all others. I also have idea idea for ignoring only specific parts of an update, but I'm not sure that's necessary.
I'm hoping to take this on as my next daily issue sometime next week, and when I'm ready I'll start a Slack conversation. Fridays seem to be a bad day for conversations.
as long as there is a read api command on the page where the request is made, which ensures that the data is up to date with the server eventually.
So we would be relying on something else re-fetching the correct data? Sounds a bit weird to me and kind of hard to determine, no?
Could you describe a case where the server returns different data that we must be aware of right away? If we still accept updates sent from other users / clients then we will be aware of important changes.
Not right now, but we can't assume the optimistic data will match 100% with the server data, right? Like for example the created date of the messages comes from the server IIRC.
A potential solution would be to generate a NewDot clientID and send it with all api requests. From the API side, send the clientID back in each Pusher event. When NewDot receives Push events from the same clientID it will discard them but it will accept all others. I also have idea idea for ignoring only specific parts of an update, but I'm not sure that's necessary.
This is the same as trusting optimistic data over server data, so let's keep discussing that.
So we would be relying on something else re-fetching the correct data? Sounds a bit weird to me and kind of hard to determine, no?
As far as I know, whenever we open the app or a specific page we fetch the latest data needed to display the UI. I think it's pretty easy to figure out where the data is loaded for any UI element.
we can't assume the optimistic data will match 100% with the server data, right? Like for example the created date of the messages comes from the server IIRC.
Yes it's not guaranteed to match. My point is that if the optimistic data is enough the show the correct UI while we are offline or waiting for the response, then it should be sufficient to continue displaying the correct UI once we come back online.