[$500] Web - Thread - Thread disappears from the LHN when Delete the message in the thread.

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: v1.3.92-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause-Internal Team Slack conversation: @

Action Performed:

1. Go to https://staging.new.expensify.com/
2. Log in with any account.
3. On LHN, click on any chat and send a message.
4. Create a thread and comment.
5. Delete a comment.
6. Look on to LHN not navigate away from the thread conversation.

Expected Result:

Created thread remains in LHN.

Actual Result:

The thread disappears from the LHN when Delete the message in the thread and not navigate away from the thread conversation.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [ ] Android: Native
• [ ] Android: mWeb Chrome
• [ ] iOS: Native
• [ ] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

https://github.com/Expensify/App/assets/115492554/2e8e35ee-3e5a-4f00-b931-cf3de828f8b6

MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e5dff96428521da3
• Upwork Job ID: 1717868047921123328
• Last Price Increase: 2023-10-27

Issue Owner

Current Issue Owner: @robertjchen

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~01e5dff96428521da3

[melvin-bot[bot]]

Triggered auto assignment to @johncschuster (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot[bot]]

Bug0 Triage Checklist (Main S/O)

• [ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [ ] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [ ] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @getusha (External)

[dukenv0307]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The thread disappears (moves to the last) from the LHN when Delete the message in the thread and not navigate away from the thread conversation.

What is the root cause of that problem?

When deleting message in ‘DeleteComment’, if the lastMessageText is empty here, we're not resetting the lastVisibleActionCreated to the created of last visible action. This is not correct because in the case the report only has 1 CREATED action and 1 message, and we delete the last message, we should still record the created date of that created action as the lastVisibleActionCreated, although the lastMessageText will still be empty.

This causes the message to come to the last position because lastVisibleActionCreated is not set (aaka empty)

What changes do you think we should make in order to solve the problem?

In the case the report only has 1 CREATED action and 1 message, and we delete the last message, we should still record the created date of that created action as the lastVisibleActionCreated.

if (lastMessageText || lastMessageTranslationKey || ReportActionsUtils.isCreatedAction(lastVisibleAction)) {
    optimisticReport.lastVisibleActionCreated = lodashGet(lastVisibleAction, 'created');
}

We need to fix the back-end following the same logic as well. The back-end is returning lastVisibleActionCreated in the DeleteComment API, same as front-end. So we should fix the back-end to return correct lastVisibleActionCreated according to the created of the create report action, similar to the front-end fix above.

What alternative solutions did you explore? (Optional)

We can consider always setting the lastVisibleActionCreated as long as there’s lastVisibleAction, without checking the lastMessageText, lastMessageTranslationKey, …

optimisticReport.lastVisibleActionCreated = lodashGet(lastVisibleAction, 'created');

[getusha]

We need to fix the back-end following the same logic as well.

@dukenv0307 I'm not sure how the back-end fix will work, could you add more explanation on this and the alternative solution?

[bernhardoj]

Same root cause as https://github.com/Expensify/App/issues/18678, but it gets closed, so reposting my proposal here.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Two issues.

1. When we delete the last reply from the thread, it will move to the bottom of LHN
2. When we reopen the chat (either by refresh or anything that will trigger OpenReport), the chat will move back to its place.

This issue will also happen when we delete the last message from a chat which is what this issue https://github.com/Expensify/App/issues/18678 is originally about.

What is the root cause of that problem?

The LHN is sorted by lastVisibleActionCreated and name. Every time we delete a message, we will update the lastVisibleActionCreated with the previous visible action created time. https://github.com/Expensify/App/blob/a59d56f00a67607a1dbae1b422807f76f548f1f0/src/libs/actions/Report.js#L1090-L1108

As you can see from the code above we will update if the previous visible action message is not empty.

Now, when we delete the last message in a chat, the previous visible action is the CREATED action.

However, CREATED action will return an empty lastMessageText, https://github.com/Expensify/App/blob/a59d56f00a67607a1dbae1b422807f76f548f1f0/src/libs/ReportActionsUtils.ts#L403-L407

which will fail the if check, so the lastVisibleActionCreated is empty, pushing the chat down to the bottom of LHN.

Next, when we refresh the page, we will reload the data from the server and it will give us the correct lastVisibleActionCreated, which is the CREATED action created time.

What changes do you think we should make in order to solve the problem?

1. I might say this is a regression from https://github.com/Expensify/App/pull/16672. So, what we need to do is to use the old logic here https://github.com/Expensify/App/pull/16672/files#diff-8afe5b71ee0436c21364148c86dadd640f2bff3e3d916addbb1f1f6f7e5b6a43L714-L722 The reason behind the PR is to solve this issue https://github.com/Expensify/App/issues/16652. But I think it's more like a BE issue because you can see in the video we show "No activity yet" for a very short time which means the optimistic data is applied correctly, but then the BE response changes the text. Also, even after I use the old logic, I can't reproduce the issue anymore. And, if you compare carefully between the old and new logic, it will always have the same lastMessageText result.

const {lastMessageText = '', lastMessageTranslationKey = ''} = ReportUtils.getLastVisibleMessage(originalReportID, optimisticReportActions);
const lastVisibleAction = ReportActionsUtils.getLastVisibleAction(originalReportID, optimisticReportActions);
const lastVisibleActionCreated = lodashGet(lastVisibleAction, 'created', '');
const lastActorAccountID = lodashGet(lastVisibleAction, 'actorAccountID', 0);
const optimisticReport = {
    lastMessageTranslationKey,
    lastMessageText,
    lastVisibleActionCreated,
    lastActorAccountID,
};

2. However, this isn't enough. When the DeleteComment API request is complete, it returns a response with empty lastVisibleActionCreated which will replace the correct optimistic lastVisibleActionCreated. I guess the BE use the same new logic (that check if the last message is empty or not), or maybe not. So, we also need to update the BE.

[tsa321]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Thread disappears from the LHN when all message in the thread deleted

What is the root cause of that problem?

The LHN ordering is based on comparison of lastVisibleActionCreated field of a report or display name when the lastVisibleActionCreated is empty. Specified in here:

https://github.com/Expensify/App/blob/a59d56f00a67607a1dbae1b422807f76f548f1f0/src/libs/SidebarUtils.ts#L210-L214

When user delete all message inside a thread the back end return empty string for lastVisibleActionCreated of the report and the ordering will be incorrect for the thread.

What changes do you think we should make in order to solve the problem?

We can use created report action of the thread as comparison value if the lastVisibleActionCreated is empty and if the report id is currentReportId (I am pretty sure we don't need to check on cuurrentReportId but just to make sure I am including it in code below). The code could be:

if (isInDefaultMode) {
        nonArchivedReports.sort((a, b) => {
            const aDate = a?.lastVisibleActionCreated || (currentReportId === a.reportID ? ReportActionsUtils.getCreatedReportAction(a.reportID)?.created : null);
            const bDate = b?.lastVisibleActionCreated || (currentReportId === b.reportID ? ReportActionsUtils.getCreatedReportAction(b.reportID)?.created : null);
            const compareDates = aDate && bDate ? compareStringDates(bDate, aDate) : 0;

            const compareDisplayNames = a?.displayName && b?.displayName ? a.displayName.toLowerCase().localeCompare(b.displayName.toLowerCase()) : 0;
            return compareDates || compareDisplayNames;
        });

and ReportActionsUtils.getCreatedReportAction could be:

function getCreatedReportAction(reportID: string): ReportAction {
    return _.find(getAllReportActions(reportID), (reportAction) => isCreatedAction(reportAction));
}

What alternative solutions did you explore? (Optional)

For the active thread or currently viewed thread /currentReportId, we could use lastReadTime field of the report.

Result:

https://github.com/Expensify/App/assets/114975543/3c75e3d9-0cb5-4944-8a27-de1fb657c5ee

[dukenv0307]

@dukenv0307 I'm not sure how the back-end fix will work, could you add more explanation on this and the alternative solution?

@getusha thanks for the feedback, more explanation added to the proposal.

My draft changes is here in case you'd like to check it out 🚀

[getusha]

🎀 👀 🎀 to get more eyes on the backend fix from internal engineer.

[melvin-bot[bot]]

Triggered auto assignment to @robertjchen, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[getusha]

@robertjchen could you check if you can fix DeleteComment to return the correct lastVisibleActionCreated value? check this https://github.com/Expensify/App/issues/30498#issuecomment-1782786628 and this proposal for more detail https://github.com/Expensify/App/issues/30498#issuecomment-1783718781

[tsa321]

Or if you want to fix it in the front end, you could take a look at my proposal. Thank you...

[johncschuster]

@robertjchen can you take a look when you get a moment?

[getusha]

Friendly bump @robertjchen https://github.com/Expensify/App/issues/30498#issuecomment-1784531439

[robertjchen]

Yes, I can look into this on the backend

[kbecciv]

Issue is reproducible, build v1.3.96-6

https://github.com/Expensify/App/assets/93399543/d405cd47-7a07-45ef-ab19-6f8b720457b6

[melvin-bot[bot]]

@robertjchen @johncschuster @getusha this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot[bot]]

@robertjchen, @johncschuster, @getusha Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot[bot]]

@robertjchen @johncschuster @getusha this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot[bot]]

@robertjchen, @johncschuster, @getusha Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[johncschuster]

Hmm... Melvin seems to be suffering from amnesia.

[robertjchen]

Didn't get to it last week, still need to look in the backend.

[robertjchen]

Still a backend issue

[melvin-bot[bot]]

@robertjchen @johncschuster @getusha this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is today
• If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[melvin-bot[bot]]

Current assignee @getusha is eligible for the Internal assigner, not assigning anyone new.

[melvin-bot[bot]]

@robertjchen, @johncschuster, @getusha Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[getusha]

Waiting for @robertjchen, to take a look at this on the backend.

[robertjchen]

Will look into this on the backend 👍

[johncschuster]

How's this going, @robertjchen?