#3293 update last message details on message deletion #3526

dklymenk · 2021-06-10T21:18:08Z

Details

Fixes incorrect display of last message on LHN after deleting last message of the chat.

Fixed Issues

Fixes #3293

Tests/QA Steps

Log in to the app
Navigate to a conversation
Send a message
Delete the message
Message in LHN should be updated in a few moments.
Refresh the page
Message shown on LHN should be the last one you have in that chat.

Tested On

Screenshots

Web

screen.mp4

Mobile Web

screen1.mp4

Desktop

Screen.Recording.2021-06-10.at.18.24.04.mov

iOS

Simulator.Screen.Recording.-.iPhone.12.-.2021-06-10.at.18.20.53.mp4

Android

3293.mp4

Beamanator · 2021-06-11T11:45:26Z

I'm sorry if I'm missing lots of the context behind why you did this a certain way, but why can't we just let the pusher event (Pusher.TYPE.REPORT_COMMENT_EDIT) update the LHN when deleting a comment, as it does when we edit a comment, since they're basically the same thing & call the same API?

dklymenk · 2021-06-11T12:00:41Z

@Beamanator This particular MR and the linked issue are about deletion on sender's screen, and not on participants screens.

There is an issue with deleting messages on other people screens which I have reported here: #3524

Or do you think that once that issue is resolved we will be able to move this whole logic into updateReportActionMessage (function that ultimately handlesPusher.TYPE.REPORT_COMMENT_EDIT action)?

dklymenk · 2021-06-11T12:14:26Z

Actually, I agree. Since we are not doing an optimistic update here, there is no reason to do it the way I did it.

However, I would assume down the line you would want every API action to have an optimistic update. This a quote from @NikkiWines from the original issue:

we typically prefer an optimistic approach but it looks like updateReportActionMessage isn't being used optimistically at the moment. You can go with the simpler non-optimistic approach

Not really sure if that implies, that down the line the behavior of updateReportActionMessage might be changed.

Anyway, I do not mind moving my code to updateReportActionMessage, however I will only be able to do it after #3524 is resolved, because at the moment sequenceNumber client gets from BE on comment deletion over WS is incorrect.

Beamanator · 2021-06-12T13:44:04Z

However, I would assume down the line you would want every API action to have an optimistic update. This a quote from @NikkiWines from the original issue:

we typically prefer an optimistic approach but it looks like updateReportActionMessage isn't being used optimistically at the moment. You can go with the simpler non-optimistic approach

Not really sure if that implies, that down the line the behavior of updateReportActionMessage might be changed.

Yeah I believe you're right - for now I think we can go the simpler way as long as we still get the necessary fix we need 😅

Anyway, I do not mind moving my code to updateReportActionMessage, however I will only be able to do it after #3524 is resolved, because at the moment sequenceNumber client gets from BE on comment deletion over WS is incorrect.

Oof yeah good point, I've been seeing those weird -1 sequenceNumber things so I was wondering where that came from, good catch!

NikkiWines

Couple of small comments

src/libs/actions/Report.js

… reportAction variable names in one function

src/libs/actions/Report.js

parasharrajat · 2021-06-20T18:27:22Z

src/libs/actions/Report.js

+                // Update last message information in the report object
+                Onyx.connect({
+                    key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
+                    callback: _.once((reportActionList) => {


Sorry but the use of _.once is very bad here. You are creating n number of subscriptions without releasing them. Where n === no of deleted comments.

I'm sorry but _.once is required there. If you remove _.once and put console.log inside that callback, you'll see that the callback is called 2-3 times per message delete call. Can you please explain what exactly is the big downside to this approach? Is it using extra memory?

So you can call Onyx.disconnect here after the operation is done. Otherwise, this subscription will remain, and On the next delete operation, both old ones and the new ones will be called. And I believe that this _.once will not be required then. But it is possible that while you are listening to this key, other factors update the Store, and thus this callback fire multiple times.
So yeah just Onyx.disconnect will be fine. _.once has not drawbacks just I was thinking that it won't be required.

Thanks. I'll check that out and update PR accordingly if that helps.

That actually works flawlessly. Thanks.

PR is updated.

parasharrajat · 2021-06-20T18:27:27Z

src/libs/actions/Report.js

+                // Update last message information in the report object
+                Onyx.connect({
+                    key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
+                    callback: _.once((reportActionList) => {
+                        // Find last non-empty message


Sorry to pitch in very late but this solution does not seem to follow the optimistic approach.

The decision to ditch optimistic approach is validated with Expensify engineers. The tl;dr is since the edit comment doesn't update last message optimistically, there is no reason the delete comment should.

https://github.com/Expensify/Expensify.cash/blob/3c54c020bfab47a66f75d3dd8a8cc158b2337e42/src/libs/actions/Report.js#L1061-L1072
This is happening optimistically. In simpler words, we delete the comment from the Onyx even though we have not called the API yet. and then based on API response if there was a failure we revert it.

Sir, how is that code snippet you linked relevant to what I said?

the edit comment doesn't update last message optimistically

The code snippet you sent doesn't have anything to do with edit comment action or LHN.

Ah, just saw a post from Nikki about this. It's fine then. But can't we do this before the API call. and revert those changes if the response is not 200?

Yes, we can. That's actually what I was going to do initially, but since Nikki said there is no need to do it optimistically, I happily abandoned that idea. Anyway I didn't want to think about any potential race conditions with both optimistic update and then revert update happening in async manner, deleting multiple comments in a quick succession, etc.

parasharrajat · 2021-06-20T18:28:22Z

src/libs/actions/Report.js

+                            lastMessageDetails.lastActorEmail = '';
+                        }
+
+                        Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, lastMessageDetails);


This statement causes a recursion.

Can you please explain how does it cause a recursion? Is there some specific edge case I didn't see. The code runs only once after the response from BE arrives.

Oh sorry didn't notice at first sight that this is a different subscription key.

parasharrajat · 2021-06-20T18:30:20Z

src/libs/actions/Report.js

+                            lastMessageDetails.lastActorEmail = '';
+                        }
+
+                        Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, lastMessageDetails);


Please release the connection here before this statement. And remove _.once. Could you please try to think of your solution in an optimistic way?

I am aware this is not the best approach. As I stated in one of the messages above, ideally we move this entire code to updateReportActionMessage function and have the handling for delete action next to edit action. However, until #3524 is resolved this cannot be done for delete action.

dklymenk · 2021-06-23T18:02:47Z

Hello, @NikkiWines.

Pinging you just in case you forgot about this PR. There has been no activity from Expensify team members here for more than 5 days now.

Please let me know if this is blocked because of some other PR and if I need to do any more changes.

Thanks.

NikkiWines · 2021-06-23T23:57:56Z

hi @dklymenk, apologies for the delay on our end.

Just so you're aware, it looks like #3524 has been resolved, so I'm not sure if you'd like to make additional modifications based on that.

Additionally, I would recommend posting to #expensify-open-source (as recommended here) regarding the usage of _.once here to get some more insight on whether or not this is the best strategy to handle multiple callbacks.

…n-message-deletion

dklymenk · 2021-06-24T10:18:42Z

Hello

Additionally, I would recommend posting to #expensify-open-source (as recommended here) regarding the usage of _.once here to get some more insight on whether or not this is the best strategy to handle multiple callbacks.

Actually, thanks to a suggestion by @parasharrajat, there is no more _.once in this PR. So actually it is good as is.

Just so you're aware, it looks like #3524 has been resolved, so I'm not sure if you'd like to make additional modifications based on that.

A change that I would have made here is taking all the code this PR adds to deleteReportComment and move it to updateReportActionMessage. However, I have an issue with that function: if you delete a message, reportMaxSequenceNumbers is not updated.

This means that if you delete last message and then edit the next last message, the LHN is not updated because you are editing a message that does not have maxSequenceNumber according to reportMaxSequenceNumbers even though it is technically the last message.

So, I suggest we merge this PR as is. There is no bugs and it doesn't do anything sub-optimal. I, however, think that moving that code to updateReportActionMessage would be cleaner. So I'll create a separate issue with proposal on how we can get a reliable maxSequenceNumber in there and if my proposal gets accepted I'll move the code to updateReportActionMessage in the new PR, because as things are now, moving the code to updateReportActionMessage will only make things worse.

dklymenk · 2021-06-24T15:47:16Z

@NikkiWines I have created a new issue #3743 for updateReportActionMessage changes. As you can see, my proposal there already has a part of code from this PR. So in the end it should look very clean after solutions for both issues are merged.

NikkiWines · 2021-06-24T22:40:02Z

Thanks for the update @dklymenk, that sounds like a good plan.

NikkiWines

Works well, @Beamanator final review is all yours

NikkiWines · 2021-06-24T22:58:15Z

src/libs/actions/Report.js

+                const connectionID = Onyx.connect({
+                    key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`,
+                    callback: (reportActionList) => {
+                        // To make sure the callback is only called once


NAB: // Disconnect to ensure the callback is only called once

Beamanator

Hmm I'm getting some pretty unexpected behavior while testing on Web, are either of you getting this?

My first test was:

Send a new message. Notice LHN updates correctly
Refresh the page, LHN shows the previous message (not good!)
Refresh again, the LHN now shows the latest message correctly

The second test:

Deleting a message I just sent shows the phone number in the LHN (bad)
Deleting a previous message shows the first message I deleted (bad)

Screen.Recording.2021-06-25.at.1.02.49.PM.mov

dklymenk · 2021-06-25T11:58:36Z

I have merged main into this branch yesterday to test how the updateReportActionMessage works now after a fix for #3524 is merged. And the behavior you see in the second test is basically this code running for any message update regardless of whether it's an edit or deletion.

    // If this is the most recent message, update the lastMessageText in the report object as well
    if (sequenceNumber === reportMaxSequenceNumbers[reportID]) {
        Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
            lastMessageText: message.html,
        });
    }

There a lot wrong with this function now and it is a cause of multiple issues at this point. That's why I created issue #3743 with a proposal for it's refactor, that should solve everything.

I didn't realize at the time that #3524 broke this PR, so I think it would be better for me to first implement the proposal for #3743, and then update this PR accordingly. What do you think?

As for your first test, I would assume it caused by some some code interruption issue, where you see an old version of localstorage because new one didn't persist because you've refreshed the page too fast. I actually cannot reproduce that one. The timing must be very precise by the looks of it.

dklymenk · 2021-06-25T11:59:53Z

I will move the PR state to draft. Good thing I have merged main into this branch and you have noticed the new issues now and not after merge.

NikkiWines · 2021-07-23T00:54:13Z

@dklymenk does this still need to be open?

dklymenk · 2021-07-23T06:47:52Z

@NikkiWines sorry, I have completely forgot about this one. Closed.

Expensify#3293 update last message details on message deletion

d7250c5

dklymenk marked this pull request as ready for review June 10, 2021 21:52

dklymenk requested a review from a team as a code owner June 10, 2021 21:52

MelvinBot requested review from Beamanator and removed request for a team June 10, 2021 21:52

dklymenk mentioned this pull request Jun 10, 2021

[HOLD for payment 2021-12-13] [HOLD for payment 2021-12-06] Delete comment - Deleted comment is still visible on LHN #3293

Closed

Beamanator assigned dklymenk Jun 11, 2021

NikkiWines requested changes Jun 15, 2021

View reviewed changes

src/libs/actions/Report.js Show resolved Hide resolved

src/libs/actions/Report.js Show resolved Hide resolved

Expensify#3293 add comment for last message html manipulations

71867f6

dklymenk requested a review from NikkiWines June 15, 2021 10:25

NikkiWines reviewed Jun 17, 2021

View reviewed changes

src/libs/actions/Report.js Show resolved Hide resolved

dklymenk added 2 commits June 17, 2021 14:08

Expensify#3293 remove comma after CREATED

0c9d2fa

Expensify#3293 use destructuring to avoid having reportActionItem and…

f480ca5

… reportAction variable names in one function

dklymenk requested a review from NikkiWines June 17, 2021 11:13

Beamanator reviewed Jun 18, 2021

View reviewed changes

src/libs/actions/Report.js Outdated Show resolved Hide resolved

Expensify#3293 use Str.stripHTML

3c54c02

parasharrajat reviewed Jun 20, 2021

View reviewed changes

parasharrajat suggested changes Jun 20, 2021

View reviewed changes

Expensify#3293 replace _.once with Onyx.disconnect

e0d282d

Merge branch 'Expensify:main' into 3293-update-last-message-details-o…

6c57d8d

…n-message-deletion

NikkiWines mentioned this pull request Jun 24, 2021

[HOLD for payment 2021-12-13] [HOLD for payment 2021-12-06] After deleting the last message, editing the next last message doesn't update the message displayed in LHN #3743

Closed

NikkiWines approved these changes Jun 24, 2021

View reviewed changes

Beamanator self-requested a review June 25, 2021 10:39

Beamanator requested changes Jun 25, 2021

View reviewed changes

dklymenk marked this pull request as draft June 25, 2021 12:00

dklymenk closed this Jul 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#3293 update last message details on message deletion #3526

#3293 update last message details on message deletion #3526

dklymenk commented Jun 10, 2021 •

edited

Loading

Beamanator commented Jun 11, 2021

dklymenk commented Jun 11, 2021

dklymenk commented Jun 11, 2021

Beamanator commented Jun 12, 2021

NikkiWines left a comment

parasharrajat Jun 20, 2021

dklymenk Jun 20, 2021

parasharrajat Jun 20, 2021

dklymenk Jun 20, 2021

dklymenk Jun 20, 2021

parasharrajat Jun 20, 2021

dklymenk Jun 20, 2021

parasharrajat Jun 20, 2021 •

edited

Loading

dklymenk Jun 20, 2021

parasharrajat Jun 20, 2021

dklymenk Jun 20, 2021 •

edited

Loading

parasharrajat Jun 20, 2021

dklymenk Jun 20, 2021

parasharrajat Jun 20, 2021 •

edited

Loading

parasharrajat Jun 20, 2021

dklymenk Jun 20, 2021

dklymenk commented Jun 23, 2021

NikkiWines commented Jun 23, 2021 •

edited

Loading

dklymenk commented Jun 24, 2021

dklymenk commented Jun 24, 2021

NikkiWines commented Jun 24, 2021

NikkiWines left a comment

NikkiWines Jun 24, 2021

Beamanator left a comment

dklymenk commented Jun 25, 2021 •

edited

Loading

dklymenk commented Jun 25, 2021

NikkiWines commented Jul 23, 2021

dklymenk commented Jul 23, 2021

#3293 update last message details on message deletion #3526

#3293 update last message details on message deletion #3526

Conversation

dklymenk commented Jun 10, 2021 • edited Loading

Details

Fixed Issues

Tests/QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Beamanator commented Jun 11, 2021

dklymenk commented Jun 11, 2021

dklymenk commented Jun 11, 2021

Beamanator commented Jun 12, 2021

NikkiWines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat Jun 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dklymenk Jun 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat Jun 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dklymenk commented Jun 23, 2021

NikkiWines commented Jun 23, 2021 • edited Loading

dklymenk commented Jun 24, 2021

dklymenk commented Jun 24, 2021

NikkiWines commented Jun 24, 2021

NikkiWines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Beamanator left a comment

Choose a reason for hiding this comment

dklymenk commented Jun 25, 2021 • edited Loading

dklymenk commented Jun 25, 2021

NikkiWines commented Jul 23, 2021

dklymenk commented Jul 23, 2021

dklymenk commented Jun 10, 2021 •

edited

Loading

parasharrajat Jun 20, 2021 •

edited

Loading

dklymenk Jun 20, 2021 •

edited

Loading

parasharrajat Jun 20, 2021 •

edited

Loading

NikkiWines commented Jun 23, 2021 •

edited

Loading

dklymenk commented Jun 25, 2021 •

edited

Loading