[$1000] Chat - 'New message' pill appears after deleting unread message

[lanitochka17]

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

---

Issue found when executing PR #21557

Action Performed:

1. Go to staging.new.expensify.com
2. Go to any chat
3. Send a message
4. Mark the message unread
5. Scroll up and click on the 'new message' pill
6. Delete the unread message
7. Scroll up

Expected Result:

'New message' pill will not appear as the unread message is deleted

Actual Result:

'New message' pill still appears although the unread message is deleted
If the 'new message' pill is not clicked, this issue will not occur.

Workaround:

Unknown

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.36.2

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

Bug6115722_bandicam_2023-07-04_17-00-30-382.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c35abc455d11baf9
• Upwork Job ID: 1678381225562419200
• Last Price Increase: 2023-07-31

[melvin-bot]

Triggered auto assignment to @anmurali (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

[samh-nl]

Proposal

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

The 'New message' pill remains despite an unread message being deleted.

What is the root cause of that problem?

When deleting a message, pendingAction is set to 'delete'. This is meant to be detected by this useEffect and should lead to the marker being refreshed if it has changed.

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

Lines 208 to 225 in 2dbc1e4

	useEffect(() => {
	// If the last unread message was deleted, remove the *New* green marker and the *New Messages* notification at scroll just as the deletion starts.
	if (
	!(
	ReportUtils.isUnread(props.report) &&
	props.reportActions.length > 0 &&
	props.reportActions[0].pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE &&
	!props.network.isOffline
	)
	) {
	return;
	}
	const reportActionsWithoutPendingOne = lodashCloneDeep(props.reportActions);
	reportActionsWithoutPendingOne.shift();
	if (newMarkerReportActionID !== ReportUtils.getNewMarkerReportActionID(props.report, reportActionsWithoutPendingOne)) {
	setNewMarkerReportActionID(ReportUtils.getNewMarkerReportActionID(props.report, reportActionsWithoutPendingOne));
	}
	}, [props.report, props.reportActions, props.network, newMarkerReportActionID]);

However, when clicking the 'New messages' pill, the report is marked as read, causing the useEffect to not process this change due to ReportUtils.isUnread(props.report) &&.

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

Lines 272 to 275 in 2dbc1e4

	const scrollToBottomAndMarkReportAsRead = () => {
	reportScrollManager.scrollToBottom();
	Report.readNewestAction(props.report.reportID);
	};

As the deletion comes after clicking the pill, the if-statement in the useEffect will evaluate to true and it will therefore return; early, preventing the marker from being refreshed/removed.

The result is that the 'New messages' pill persists, which is the problem we are observing.

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

In the useEffect, we must replaceReportUtils.isUnread(props.report) && with !_.isEmpty(newMarkerReportActionID) &&.

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

Line 212 in 2dbc1e4

ReportUtils.isUnread(props.report) &&

What alternative solutions did you explore? (Optional)

Alternatively, we can add setNewMarkerReportActionID(''); in the scrollToBottomAndMarkReportAsRead function.
This function is called when clicking the 'New messages' pill and marks the report as read. However, performing this change will remove the marker immediately upon clicking the pill, which may not be desired as it leaves the user with no information about what messages were newly added, which seems like the best UX and therefore the intended behavior.

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

Lines 326 to 329 in 2dbc1e4

	<FloatingMessageCounter
	isActive={isFloatingMessageCounterVisible && !_.isEmpty(newMarkerReportActionID)}
	onClick={scrollToBottomAndMarkReportAsRead}
	/>

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

Lines 272 to 275 in 2dbc1e4

	const scrollToBottomAndMarkReportAsRead = () => {
	reportScrollManager.scrollToBottom();
	Report.readNewestAction(props.report.reportID);
	};

A second alternative is to detect a discrepancy between ReportUtils.isUnread(props.report) and newMarkerReportActionID using a useEffect hook and remove the marker if the report is read. However, this suffers from the same drawback as mentioned above. It will immediately reset the marker.

Edit: Updated proposal based on refactored component

[DanutGavrus]

This reported issue has the same root cause as this which was found while testing this PR. We were thinking and decided that the issue is out of scope of that PR.

Here are my findings:

1. This reported bug happens if we remove all the changes introduced by the PR;
2. This reported bug does not happen if we introduce the changes proposed here.

Right now, New Markers Issues are on hold as I've found out from this comment. Lately I've seen quite some issues arising from the same root cause, which is that the New Markers do not get removed when report becomes read. Maybe we want to make an exception to fix them all with 1 Proposal, or we'll keep them all on hold until the refactor is done.

[anmurali]

Reproduced on Chrome/ Mac

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[DanutGavrus]

Based on this I think we'll have to put this on hold for #15212

[melvin-bot]

@anmurali, @0xmiroslav Whoops! This issue is 2 days overdue. Let's get this updated quick!

[anmurali]

@0xmiroslav - do you agree with @DanutGavrus? If yes, I will add the held to the title and degrade this to a weekly while we wait.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[anmurali]

@0xmiroslav bump

[0xmiros]

@samh-nl @DanutGavrus ReportActionsView is now refactored to function component. Please update your proposals accordingly.

[DanutGavrus]

@0xmiroslav

ReportActionsView is now refactored to function component. Please update your proposals accordingly.

Proposal

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

Chat - 'New message' pill appears after deleting unread message.

What is the root cause of that problem?

1. Clicking on the New Messages pill automatically sets the report as read;
2. But, in ReportActionsView.js we do not check if the report automatically became read in order to clear newMarkerReportActionID's value.

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

We should add this logic:

useEffect(() => {
    // If the report automatically became read, remove the *New* green marker and the *New Messages* notification at scroll.
    if (!ReportUtils.isUnread(props.report) && newMarkerReportActionID != '') {
        setNewMarkerReportActionID('');
    }
}, [props.report, newMarkerReportActionID]);

[amyevans]

To be honest I'm not sure which is the expected behavior. @MonilBhavsar I know you've spent much more time than me diving into the logic and expectations of the unread marker, can you weigh in on the expected behavior question here please: #22174 (comment)

[MonilBhavsar]

Option 1 is correct based on the design doc 👍
Just to note, there's an ongoing PR to update the unread functionality. May be if we want to put this issue on HOLD for that, so we don't update code in parallel #25935 cc @gedu

[amyevans]

Okay thanks so much @MonilBhavsar!

[melvin-bot]

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

[amyevans]

Still on hold

[situchan]

Same

[amyevans]

Still holding

[MonilBhavsar]

This issue should be fixed! We can retest and most probably close this

[amyevans]

Thanks Monil! Retested and confirmed it's fixed:

retest-22174.mov