[HOLD for payment 2023-09-20] [$1000] Temporary thread message disappearance when deleting a thread message

[kavimuru]

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:

1. Open a chat
2. Send a message in the chat
3. Click on "Reply in thread" to create a thread message
4. Delete the threaded message

Expected Result:

The thread message should remain visible without any disappearance upon deletion

Actual Result:

When deleting a thread message, it momentarily disappears for a second before reappearing

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.42-21
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
Notes/Photos/Videos: Any additional supporting documentation

screen-recording-2023-07-17-at-71701-pm_7JOAkxG1.mp4

Recording.1290.mp4

Expensify/Expensify Issue URL:
Issue reported by: @ayazhussain79
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689603772547949

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01ed092abd8ef6cd10
• Upwork Job ID: 1681482271492681728
• Last Price Increase: 2023-08-09
• Automatic offers:
• fedirjh | Reviewer | 26432430
• Nodebrute | Contributor | 26432431

[melvin-bot]

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

[melvin-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

[jeffmchang]

Proposal

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

It looks like the bug here is directed to the chat system where a new 'chat tab' is created when you reply in a thread. When that new tab is created, it allows a new conversation to take place.:

The first message of this new 'thread' will refer to the comment being replied to:

The problem here is when you have messages being sent post-thread creation, they act as normal messages and can be deleted without issues. But when you attempt to delete the first message (comment being replied to), it results in a standard disappearance of the message, but then the message comes back with [Deleted message]. Once the first message in the new thread has been deleted, that results in the original message in the main conversation being deleted as well.

Normal Delete:

Bugged Delete:

This also leads to a bugged/empty thread:

If the message is deleted but the in-thread messages are still there, the [Deleted message] remains and keeps the thread open:

What is the root cause of that problem?

From a surface-level standpoint, it looks like the [Deleted message] is showing up because the component is not updating and removing the thread altogether.
I'm having trouble finding the exact location of the bug, if someone is able to provide me with a time-efficient way to locate the file that this bug is occurring on, that would be much appreciated.

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

I think we should change the action of deleting the messages. If the main thread comment is deleted, it confirms deletion and prompts that deleting the main thread comment will result in the entire thread being deleted. Upon deletion, it will automatically remove the thread as a conversation tab in the left column.

[MitchExpensify]

I see some discussion related to this here https://expensify.slack.com/archives/C049HHMV9SM/p1685965494942699?thread_ts=1684354260.051229&cid=C049HHMV9SM

[MitchExpensify]

Also related to the first thread message and deletion but not quite this issue #19363

[MitchExpensify]

Not a dupe from what I can tell

[melvin-bot]

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

[melvin-bot]

Current assignee @MitchExpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[adityaaa-r]

Proposal

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

When we delete a Thread parent, It disappears for a moment, and then the [Deleted message] message appears.

What is the root cause of that problem?

The root cause is in the following data
We are pushing in optimisticData :

{
    "onyxMethod": "merge",
    "key": "reportActions_3529565171134454",
    "value": {
        "6902849697016981155": {
            "childVisibleActionCount": 0,
            "childCommenterCount": 0,
            "childLastVisibleActionCreated": "",
            "childOldestFourAccountIDs": ""
        }
    }
}

• As soon as the action gets triggered, we dispatch this "default" value template to the state.
• This "default" value remains in state till the API provides a response.
• This empty value is the root cause for disappearing of Thread message before getting updated to [Deleted message].
• to be specific childVisibleActionCount is being set to 0, causing disappearance of thread message.

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

Removing the following act of pushing this object with empty values solves the issue. (but this may affect some other part of code, so adding the alternate solution is better and more efficient.)

if (parentReportAction && parentReportAction.reportActionID) {
            optimisticData.push({
                onyxMethod: Onyx.METHOD.MERGE,
                key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${report.parentReportID}`,
                value: {
                    [parentReportAction.reportActionID]: ReportUtils.updateOptimisticParentReportAction(
                        parentReportAction,
                        optimisticReport.lastVisibleActionCreated,
                        CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
                    ),
                },
            });
        }

What alternative solutions did you explore? (Optional)

In updateOptimisticParentReportAction :

What's the purpose of this? Removing this part of code also solves the problem.

if (childVisibleActionCount > 0) {
            childVisibleActionCount -= 1;
        }

or we can add a condition here that if childVisibleActionCount is 1, then no need to decrement to 0.

- if (childVisibleActionCount > 0)
+ if (childVisibleActionCount > 0 && childVisibleActionCount !== 1)

[melvin-bot]

📣 @aditya-ragi! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[adityaaa-r]

Contributor details
Your Expensify account email: hireadityaragi@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01f8498acbc5e8f6e4

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[adityaaa-r]

just added alternate solution to the proposal now.

[Nodebrute]

Proposal

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

Temporary thread message disappearance when deleting a thread message

What is the root cause of that problem?

Should hide on delete is true by default. It is only false if a message has thread child.

App/src/pages/home/report/ReportActionItem.js

Line 494 in 5419d9b

shouldHideOnDelete={!ReportActionsUtils.hasCommentThread(props.action)}

Previously hasCommentThread was working fine until lodashGet(reportAction, 'childVisibleActionCount', 0) > 0 was added.
But this was added to deal with another issue #22690.
Actually this is regression from this issue.

As thread first chat does not has childVisibleActionCount so this function is returning false.

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

We should also add another check here to see if it's thread first message we can do something like this

 shouldHideOnDelete={ !ReportUtils.isThreadFirstChat(props.action,props.report.reportID) && !ReportActionsUtils.hasCommentThread(props.action) }

This issue will fix this. We can also add this change in other place where we are using hasCommentThread

Additional changes
We have another problem I think we should fix it here too.

App/src/libs/ReportUtils.js

Line 1390 in 48edd31

if (childVisibleActionCount > 0) {

here we are decrementing the childVisibleActionCount. Let's say we have 2 comments in thread

1. The parent message
2. another text

Here childVisibleActionCount will be 1 but if we delete the parent message the visibility count will be "0". This is not logically correct because we still have the child text.

We can pass reportID here

App/src/libs/ReportUtils.js

Line 1373 in 48edd31

function updateOptimisticParentReportAction(parentReportAction, lastVisibleActionCreated, type) {

and add a check here

App/src/libs/ReportUtils.js

Line 1390 in 48edd31

if (childVisibleActionCount > 0) {

 !ReportUtils.isThreadFirstChat(parentReportAction,reportID)

only then we will decrement the childVisibleActionCount

What alternative solutions did you explore? (Optional)

Or we can fix it directly in hasCommentThread like this

function hasCommentThread(reportAction,reportID) {
lodashGet(reportAction, 'childVisibleActionCount', 0) > 0 || !_.isUndefined(reportAction.childReportID) && reportAction.childReportID.toString() === reportID;
}

[jigartankhupp]

Proposal

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

The thread message should remain visible without any disappearance upon deletion

What is the root cause of that problem?

I've found the old commit code where the thread message delete functionality working fine. #22690

If you revert the code with this:

function hasCommentThread(reportAction) {
    return lodashGet(reportAction, 'childType', '') === CONST.REPORT.TYPE.CHAT;
}

Then it's working fine.
I've attached the video here that things are working fine.
https://github.com/Expensify/App/assets/38810839/6d5ee745-3606-4b70-90f7-2375a03cfc62

I hope this will solve the problem.

Thanks

[melvin-bot]

📣 @jigartankhupp! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[mehvash-Khan]

Proposal

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

In a thread of a message, when we try to delete the message when thread has no other replies/messages, it disappears before showing [Deleted Message]

What is the root cause of that problem?

The message disappears momentarily on deleting a thread message in its thread room while it waits for the response from the DeleteComment API call. Once we get the response, the message is updated to [Deleted Message]. This means it is not being handled well using optimistic data updates.

This is not the case when we are deleting a thread message(with replies) in the main chat room. When we delete a thread message with children(or replies) in main chatroom, [Deleted message] is being displayed optimistically and when we try to delete that message without any children, the entire message disappears(this is expected). This scenario has been taken care in issue #22690. If we look at these changes closely, they work well when the thread with no replies is deleted in the main chatroom but they don't work when the thread is being deleted in its own chatroom where it is expected to display [Deleted Message] instead of being hidden.

While waiting for the API response, the message gets hidden by the shouldHideOnDelete prop of the OfflineWithFeedback component as can be seen here. This condition does not consider the fact that the message may be open in its own thread/chatroom and hence should not be hidden even if it has no replies.

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

To tackle this we can the following condition to prop shouldHideOnDelete

 shouldHideOnDelete={!(ReportActionsUtils.hasCommentThread(props.action) || props.action.reportActionID === props.report.parentReportActionID)}

When a report action/message is being rendered in its own chat room/thread room, the action passed to ReportActionItem is actually the parent report action of the current report as can be seen here and here. Basically the 2nd part of the condition checks that the message/report action being deleted is in a thread room of its own having a parentReportActionID equal to the id of the report action/message.

This change will prevent the message that is deleted from disappearing momentarily but there would still be a problem where the initial message will continue to be displayed until the Api request is complete after which [Deleted Message] is displayed.

To avoid this, we can add a similar condition here. I would propose we rename the hasCommentThread prop to the component ReportActionItemFragment to shouldDisplayDeletedWhenPending(or some better name) and pass to it the following:

  shouldDisplayDeletedWhenPending={ReportActionsUtils.hasCommentThread(props.action) || props.action.reportActionID === props.report.parentReportActionID}

We replace this prop in with hasCommentThread prop being passed in condition here.

Please node that we would also need to pass report as a prop from ReportActionItem to ReportActionItemMessage.

[jigartankhupp]

Contributor details
Your Expensify account email: jigarjs@hupp.in
Upwork Profile Link: https://www.upwork.com/freelancers/~012a5ed1b4a78d3b2c

[melvin-bot]

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

[jigartankhupp]

Contributor details
Your Expensify account email: jigarjs@hupp.in
Upwork Profile Link: https://www.upwork.com/freelancers/~012a5ed1b4a78d3b2c

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[daanish-shaikh]

Proposal

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

First thread message disappears for a few seconds before reappearing as [Deleted message]. Ideally it should just change and not disappear and reappear.

What is the root cause of that problem?

Adding optimisticReportActions to the optimisticData array is what causes the message to disappear. And after the ‘DeleteComment’ api call is done, report is updated with first thread message appearing again as [Delete Message].

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

Removing optimisticReportActions from the optimisticData array will solve the problem.

App/src/libs/actions/Report.js

Lines 950 to 961 in be2c81f

	const optimisticData = [
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${originalReportID}`,
	value: optimisticReportActions,
	},
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.REPORT}${originalReportID}`,
	value: optimisticReport,
	},
	];

This solution will also not affect any other delete comment behaviour as this action only removes the comment from the report and anyways that will happen on the ‘DeleteComment’ api call as the whole report gets updated.

Remove following code

App/src/libs/actions/Report.js

Lines 951 to 955 in be2c81f

	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${originalReportID}`,
	value: optimisticReportActions,
	},

After removing

const optimisticData = [
    {
        onyxMethod: Onyx.METHOD.MERGE,
        key: `${ONYXKEYS.COLLECTION.REPORT}${originalReportID}`,
        value: optimisticReport,
    },
];

[melvin-bot]

📣 @fedirjh 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @Nodebrute 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot]

📣 @ayazhussain79 We're missing your Upwork ID to automatically send you an offer for the Reporter role.
Once you apply to the Upwork job, your Upwork ID will be stored and you will be automatically hired for future jobs!

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @Nodebrute got assigned: 2023-08-31 13:19:39 Z
• when the PR got merged: 2023-09-11 03:18:22 UTC
• days elapsed: 6

On to the next one 🚀

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.68-17 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix deleting parent thread message temporary disappearing #26548

If no regressions arise, payment will be issued on 2023-09-20. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @fedirjh requires payment offer (Reviewer)
• @Nodebrute requires payment offer (Contributor)
• @ayazhussain79 requires payment

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@fedirjh] The PR that introduced the bug has been identified. Link to the PR:
• [@fedirjh] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@fedirjh] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@fedirjh] Determine if we should create a regression test for this bug.
• [@fedirjh] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@MitchExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[MitchExpensify]

Payment summary:

$1000 - @fedirjh requires payment offer (Reviewer)
$1000 - @Nodebrute requires payment offer (Contributor)
$250 - @ayazhussain79 requires payment

[MitchExpensify]

Friendly bump on the BZ steps @fedirjh

[MitchExpensify]

Paid:
$1000 - @fedirjh requires payment offer (Reviewer)
$1000 - @Nodebrute requires payment offer (Contributor)

Pending:
$250 - @ayazhussain79 requires payment - I've resent an offer, please let me know once you accept https://www.upwork.com/nx/wm/offer/26799902

[ayazhussain79]

@MitchExpensify Offer accepted, Thank you

[MitchExpensify]

Paid! Thanks @ayazhussain79

Waiting on BZ steps before closing this out cc @fedirjh

[fedirjh]

BugZero Checklist:

• Link to the PR: If a deleted parent doesn't have a thread anymore, hide it #22690
• Link to comment: If a deleted parent doesn't have a thread anymore, hide it #22690 (comment)
• Link to discussion: I don't think there is anything to add to the checklist.
• Determine if we should create a regression test for this bug:

---

Regression Test Proposal

1. Navigate to any report
2. Send a message.
3. Click on "Reply in Thread."
4. In the thread, delete the parent message.
5. Verify that the parent message does not disappear.
6. Navigate back to parent report
7. Send new message.
8. Click on "Reply in Thread."
9. Send some messages in the thread.
10. Go back to the parent report.
11. Delete the message with thread replies.
12. Verify that the message does not disappear.

• Do we agree 👍 or 👎

[MitchExpensify]

Awesome added a request to add this! https://github.com/Expensify/Expensify/issues/319505