Closed isagoico closed 2 years ago
Triggered auto assignment to @timszot (Engineering
), see https://stackoverflow.com/c/expensify/questions/4319 for more details.
Triggered auto assignment to @JmillsExpensify (External
), see https://stackoverflow.com/c/expensify/questions/8582 for more details.
Nice find! this one was really hard to reproduce.
This happens when we have a ReportAction
that failed to be delivered to our API (we can see the network error on the log). It's hard to reproduce because normally this ReportAction
is sent to our NetworkRequestQueue
and retried, but that doesn't happen in all cases. (There's an ongoing conversation about refactoring this queue at https://github.com/Expensify/Expensify.cash/issues/3608)
One of the easiest reproductions paths that I found is:
isLoadingAfterReconnect
but not enough for the API calls of our queue to complete, and then go offline againNetworkRequestQueue
anymore (just C is) , but also haven't been sent yet.The reason the messages get stuck in the bottom is because in our ReportActionsView
we order our ReportActions
by their sequenceNumber
https://github.com/Expensify/Expensify.cash/blob/bb7d3d3f797d0463bf12ddfacdaf25d3ff181ccb/src/pages/home/report/ReportActionsView.js#L253-L266
and when we optimistically save a new ReportAction
, we pass the key that we use to save them on storage as our temporary sequenceNumber
. Since this number is purposefully long, when we sort our ReportAction
s these messages will always end up in the last place.
https://github.com/Expensify/Expensify.cash/blob/bb7d3d3f797d0463bf12ddfacdaf25d3ff181ccb/src/libs/actions/Report.js#L1003-L1004
https://github.com/Expensify/Expensify.cash/blob/bb7d3d3f797d0463bf12ddfacdaf25d3ff181ccb/src/libs/actions/Report.js#L1028-L1030
So our stuck messages are optimistically saved messages with really long sequenceNumber
s that are not being corrected by the API (caused at least in this reproduction path by not being on the retry queue anymore)
We can solve this on the front by changing the sequenceNumber
of our optimistically saved ReportActions to be the highestSequenceNumber
of the Report. By doing this, when the messages get stuck they will retain the order in which they were sent, and I also think it would help reduce jumps in more normal scenarios.
While I think that we really should make our frontend fail more gracefully here, this issue really highlights the importance of #3608
Sorry @rdjuric I missed this one last week. Adding the Exported label so a colleague can assess your proposal and then I'll get a job posting going on Upwork.
Triggered auto assignment to @NikkiWines (Exported
), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
Done! Upwork posting is here for reference when Nikki approves your proposal. https://www.upwork.com/jobs/~0135a8456084eb691e
This proposal seems good to me, updating the sequenceNumber
s so they align with the sent order sounds like a good solution.
cc: @marcaaron and @tgolen, in case I'm off base with approving this since I'm not the most familiar with this logic. As @rdjuric mentioned in their proposal, this relates to https://github.com/Expensify/Expensify.cash/issues/3608 and the potential refactor for the network request queue
I haven't looked at the proposal too deeply, but at first blush modifying the sequenceNumber
as suggested does not seem like a clear solution. The "next highest sequence number" is what we were using before the temporary client ids (that yes, are higher than all other sequenceNumber
so they appear at the bottom). But we did this to avoid collisions between presumed sequence number and actual sequence number.
So, I think it would be more valuable to look into improving the queue logic + maybe start sending messages synchronously instead of async (as first message wins and appears higher than the last is what we have now).
I agree that it's important to improve the queue logic, but i think it would also be nice for the app to deal better with a message that has failed to be sent (which might still happen after we improve the queue).
Right now, using our optimisticReportActionID
Using the highestSequenceNumber
just changes the "resting place" of the message to be the sent order, and not the bottom. This helps with point 2.
To address point 1, we could differentiate a message that was sent and one that is still being sent, maybe using a lighter tone of black/grey in the unsent one?
I know you're busy @marcaaron, but I really wanna hear your thought on this.
If you feel like changing the way we deal with a failed sent message is part of the problem, I can start a PR. If you don't, it might make more sense to see how #3608 unveils.
This is happening to me again now, I think a hard refresh fixed it. @NikkiWines and/or @marcaaron , can you reply to @rdjuric's comment above and let Mills know if/when we can hire? Thanks.
To address point 1, we could differentiate a message that was sent and one that is still being sent, maybe using a lighter tone of black/grey in the unsent one?
I think we discussed this internally in the past and decided that the user should not be aware that a message did or did not send. Probably best to bring up a change like this in Slack as more people will be interested in product changes. Then we can discuss technical solutions here.
If you feel like changing the way we deal with a failed sent message is part of the problem, I can start a PR.
Sorry, I don't think there is enough information to move forward with a solution.
I'm adding a planning label to this one because while it seems like a bug that messages would "persist at the bottom" this is the expected behavior since messages are created in the order they are requested to be created.
@marcaaron
I think the closest thing we have to a bug here is that our NetworkRequestQueue
is being cleared even if there're requests in it that haven't been sent yet (that's the root cause of our stuck messages). Do you think I should submit a proposal to improve this here or it's something for #3608?
I'm not following that other issue very closely, but it doesn't seem like it proposes to fix any specific problem.
If a message fails to send for any reason it will appear at the bottom. This is by design since we are showing you the most accurate order we have based on what is saved in the server. Are we trying to change that behavior? Or are we trying to figure out why the message was not successfully created?
the closest thing we have to a bug here is that our NetworkRequestQueue is being cleared even if there're requests in it that haven't been sent yet (that's the root cause of our stuck messages)
Perhaps we should create another issue to discuss this. I'm having trouble seeing how the two are related. This issue is that messages that are "stuck" to the bottom of the chat. So it would appear that the design is working correctly. If we want to change it we can, but we need to discuss more generally how it should work first. I suggest we bring it to Slack.
I'm adding a planning label to this one because while it seems like a bug that messages would "persist at the bottom" this is the expected behavior since messages are created in the order they are requested to be created.
This is not expected behavior, new messages should always persist at the bottom and old messages should be above them. The video in the OP shows how a message is stuck at the bottom even as I send new messages. The issue from this morn was similar but it wasn't a single message stuck at the bottom, it was a few messages. Any new messages I sent showed above the few old stuck messages
This is not expected behavior, new messages should always persist at the bottom and old messages should be above them
Based on the current design if a message fails to send then... yes... it will appear below ones that succeed before it does. This is how things are coded up and aligns with my expectations. This could be improved and we can change it, but it's not clear to me how this will work.
Any new messages I sent showed above the few old stuck messages
I think we are dealing with two problems here...
I agree that we have two different problems here. It would be nice to have a clear idea of what the scope of this issue is.
Why did a message fail to send?
When a message fails to send it normally goes to our NetworkRequestQueue
and is retried. The only way we have sequenceNumber
s that are not being updated by our API (while the user is online) is by
I don't have access to the BE, so no idea if 1. is possible.
2. can be caused by the app incorrectly clearing our NetworkRequestQueue
as I described on my https://github.com/Expensify/Expensify.cash/issues/3623#issuecomment-866455297
When messages fail to send (either once or repeatedly) how should we handle them?
My expectation as a user is that these messages should stay on the order in which they were sent and somehow indicate that they failed to be sent. I think staying stuck on the bottom is bad UX because it really looks like a bug - the message is not where we expect it to be and it provides no indication that something went wrong.
I think staying stuck on the bottom is bad UX because it really looks like a bug - the message is not where we expect it to be and it provides no indication that something went wrong.
Maybe we should bring this to Slack for discussion?
There is no way for us to action a proposal like this without figuring out...
As far as the scope of this issue...
So to solve this issue we'd need to reproduce that bug and then make sure it can't happen.
Whether a message should retain its position or always move to the bottom of the chat feels out of scope + there should be some broader discussion about how that should work and a new issue created.
New messages should always display at the bottom.
Is stated as the issue but I think it's actually - "messages aren't showing up in the order that they're being sent". Here's an example from this morn where I sent these in order and it this is how it shows, even after refreshing.
And, while I was sending them, they were 'moving around' and not showing in correct order or the final order in the screenshot.
I'm pretty sure this happens when I awaken from computer from sleep and e.cash has been open. I'm on Version 1.0.76-2 now. If it never happens to anyone else, it could be some legacy weirdness specific to me(?).
Posted in #expensify-open-source (link) asking others to comment on this issue if it happens to them.
[LOCAL_NOTIFICATION] No notification because it was a comment for the current report
2
app-7d872b1….bundle.js:143 [LOCAL_NOTIFICATION] No notification because comment is from the currently logged in user
2
app-7d872b1….bundle.js:143 [LOCAL_NOTIFICATION] No notification because it was a comment for the current report
4
app-7d872b1….bundle.js:143 [LOCAL_NOTIFICATION] No notification because comment is from the currently logged in user
app-7d872b1….bundle.js:143 [LOCAL_NOTIFICATION] Creating notification
4
app-7d872b1….bundle.js:143 [LOCAL_NOTIFICATION] No notification because comment is from the currently logged in user
app-7d872b1….bundle.js:143 Timing:expensify.cash.search_render 11
app-7d872b1….bundle.js:143 [Pusher] Unsubscribing from channel true
Object
app-7d872b1….bundle.js:143 [Pusher] Attempting to subscribe to channel true
Object
10
app-7d872b1….bundle.js:143 [LOCAL_NOTIFICATION] No notification because comment is from the currently logged in user
DevTools failed to load SourceMap: Could not parse content for app://-/app-7d872b1….bundle.js.map: Unexpected end of JSON input
DevTools failed to load SourceMap: Could not parse content for app://-/pdf.worker.entry-b22bab8….bundle.worker.js.map: Unexpected end of JSON input
app-7d872b1….bundle.js:143 [Violation] 'setTimeout' handler took 79ms
app-7d872b1….bundle.js:16 [Violation] 'requestAnimationFrame' handler took 63ms
I also logged into my test account I was sending the messages too and they're showing up out of order there too. Thanks to @rdjuric for the ping in #expensify-open-source to check this
From Ioni - "We had several server semi outages today, so it is possible that this is related to that. ie: you sent 5 requests to the server, they usually arrive in order but since things were randomly slow, they got processed in a random order."
The timing looks to match up, site was reported down at the same time I was having issues.
So to solve this issue we'd need to reproduce that bug and then make sure it can't happen.
I agree with @marcaaron here, I think we need to be able to reliably reproduce this issue in order to move forward with proposals, otherwise, it's going to be quite difficult to find a proper solution for this. I would recommend closing this until we can reproduce the issue consistently.
Made Upwork posting private in the meantime.
https://expensify.slack.com/archives/C01GTK53T8Q/p1632266047028300
This has come back again for both Mills, I and and I suspect, others. Reopening since it's an issue, added Needs Reproduction
label. From me in Slack thread
I don't have consistent reproduction steps. I thought it might have to do with either coming back from a computer being asleep or from unstable internet. I tested 'coming back from sleep' with no luck.
Offering $500 if someone can document steps to reproduce this issue consistently https://expensify.slack.com/archives/C01GTK53T8Q/p1632841830172100
cc: @mallenexpensify
The issue is reproducible on the web itself.
Steps to Reproduce:
I Managed to reproduce it two ways.
~I Will update the video link here~ Posted in theared https://expensify.slack.com/archives/C01GTK53T8Q/p1632852884204500?thread_ts=1632841830.172100&cid=C01GTK53T8Q
Note: See few messages are duplicated here.
Ignore texts sent are not meaningful.
See message that start with '/' will jumble.
This not meaning full I sent these in order only 0, 1, 23, 4, 5, 566, 7, 8, 9, 0, 0 See how its jumbled,
Similar to way 1
mentioned in https://github.com/Expensify/App/issues/3623#issuecomment-929505373
Steps:
Steps to Simulate a connection drop:
Video is posted in this comment: https://expensify.slack.com/archives/C01GTK53T8Q/p1632853780205000?thread_ts=1632841830.172100&cid=C01GTK53T8Q
Adding your video here too @Santhosh-Sellavel .
and.. going back to a much earlier comment from @marcaaron
This is not expected behavior, new messages should always persist at the bottom and old messages should be above them
The issue I'm specifically interested in fixing is when a chat gets stuck at the bottom and, when sending new messages, they show up above the stuck chat, even once a user is back online. The only way to remove the stuck chat is a combo of refresh and/or close the app. I imagine out of order chats based on the internet connection state is a different issue, correct me if I'm wrong and they're related
do you have any idea how to fix this?
cc: @mallenexpensify Just my thoughts, without much technical details.
In case of network failure/ offline mode.
We should not add more than one request or any request that tries to add/update comment to the networkRequestQueue
,
we should show users that message is failed to send, provide an option in UI with retry.
We should sync messages in the background when the network is available, bulk add/update messages that are not sent/updated.
@isagoico Were you able to reproduce this one! From the steps, I provided in the comments https://github.com/Expensify/App/issues/3623#issuecomment-929505373 https://github.com/Expensify/App/issues/3623#issuecomment-929523719
@isagoico Any update on this?
Oh sorry I missed to update here. I have not been able to reproduce this on my device but I think it's because of the terrible mobile connection I have at my place. Still, I tried throttling and was unable to reproduce too. On another note @stitesExpensify Was just able to reproduce this morning here https://expensify.slack.com/archives/C01GTK53T8Q/p1633453299410400
I will try again today and will post here if I'm able to finally reproduce
Update on this: I was finally able to reproduce the out of order messages (via connecting and disconnecting in Network tab using slow and fast 3G) but I was still unable to reproduce the message persisting at the bottom.
The out of order messages sent while offline is being addressed here https://github.com/Expensify/App/issues/2713
From Way 2 mentioned this comment https://github.com/Expensify/App/issues/3623#issuecomment-929505373
I see the message sdjvkjqdsigrw
stuck in bottom for a while (two new messages)
@mallenexpensify Any update on this?
@Santhosh-Sellavel , thanks for the persistence on this issue. I just tried to reproduce by turning off/on my wifi and some messages did show out of order but I wasn't able to get one to be stuck at the bottom like you were in the vid below. Do you have recommendations on ways to connect/disconnect in order to reliably reproduce? Are there any shortcut keys to use for doing so?
@mallenexpensify Steps to Simulate a connection drop:
Now enable/disable mobile internet in the phone, I find this is easiest to simulate connection drop.
Thanks @Santhosh-Sellavel , using that method with my iPhone I was able to reproduce 🎉 I created a job to pay you $500, can you please apply and comment here when you've done so? https://www.upwork.com/jobs/~0147d4b6a84e5185b9
@marcaaron @NikkiWines @stitesExpensify now that we have reliable reproducible steps, what's the next best step here? Should I assign back to Nikki since she was initially assigned as CME via the Exported
label?
I can stay assigned as CM, post in #expensify-open-source to get more eyes on the issue, create a new job in Upwork (old one is closed) and double the price to $500
@mallenexpensify I am getting this error can you please invite me to the job or make the job open for all so I can apply.
Can you try again @Santhosh-Sellavel , for some reason it defaulted to private but I opened up to public, likely right after you tried
Still not working @mallenexpensify
Was able to hire and pay @Santhosh-Sellavel specifically for documenting reproducible steps.
I did some analysis and found that Rduric has the best and proper explanation of this issue. Now two questions that we have to think about is:
What should we do if the message is failed to send? (Slack, Whatsapp says message failed to send try again). Do we need this functionality? It is possible to do if we want. (Should be a separate issue.)
Another thing is the order of messages, even if a message is failed to send the order should be the same, and failed msg should show that it was failed to send (# 1). (Current issue)
I see that Using the timestamp instead of the sequence number for ordering the message could fix this issue.
@cead22 can I get quick 👀 on the above? I know you were looking into " timestamp instead of the sequence number for ordering the message could fix this issue." or similar
I'm not sure about this because I'm not super familiar with this code, but I imagine sorting by timestamp could solve this issue, or we could keep track of what order these messages were sent in and display them in that order.
cc @marcaaron @roryabraham @tgolen
sorting by timestamp could solve this issue
It seems to me like this would work, but I wonder if there is some performance implication with comparing timestamps vs comparing integers, because this is a very high-frequency operation.
Unless I am missing something, it also seems like sorting report actions by timestamp rather than sequence number would deprecate the need for any sequence number. Also I think this would get us better (or at least more predictable) offline behavior, where messages are displayed in the chat in the order in which they were written, rather than the order in which they were received by the server. I think that this lends itself to the philosophy that users shouldn't have to be aware when their message did or did not send.
I think we use epoch timestamp which is also an integer so I would not worry about that.
@arielgreen was able to reproduce this issue too:
I have some messages I sent awhile ago that never went through. Now they're just hanging out at the bottom of my message window, never getting sent but also never disappearing. This is happening across mobile, web, and desktop. E.g., see attached on web and mobile.
@parasharrajat @Santhosh-Sellavel @roryabraham @cead22 Any idea how we can move this forward to specifically fix the chats that get stuck at the bottom? (and not chats out of order)
@johnmlee101 reported too https://expensify.slack.com/archives/C01GTK53T8Q/p1634658084467100
No idea, I believe this is all front-end logic, and I'm most familiar with API and backend, but not so much with App
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
New messages should always display at the bottom.
Actual Result:
When sending new chats in Desktop, an old message persists at the bottom.
Workaround:
Hard refreshing solves the problem.
Platform:
Where is this issue occurring?
Web iOS Android Desktop App ✔️ Mobile Web
Version Number: 1.0.68-4
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: I was unable to reproduce the issue.
https://user-images.githubusercontent.com/44479856/122284241-4f684c00-cee5-11eb-80c5-994f6ca28bbf.mp4
Logs:
Expensify/Expensify Issue URL:
View all open jobs on Upwork
From @mallenexpensify https://expensify.slack.com/archives/C01GTK53T8Q/p1623805090370500