[HOLD for payment 2024-10-04] [$250] Chat - New message line marker shown in the wrong position

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 9.0.38-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): testpayment935@gmail.com
Issue reported by: Applause Internal Team

Action Performed:

1. Navigate to staging.new.expensify.com
2. Receive multiple messages from another account
3. Open another report and open the chat again to remove the unread marker
4. Reply in thread on one of the sent messages
5. Send a reply and go back by using the from link

Expected Result:

No new line marker gets shown

Actual Result:

New line marker shown under the message which has a reply

Workaround:

Unknown

Platforms:

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6608923_1726754307057.video_2024-09-19_16-54-39.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021838488357896477944
• Upwork Job ID: 1838488357896477944
• Last Price Increase: 2024-09-24

Issue Owner

Current Issue Owner: @kadiealexander

[melvin-bot]

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[IuliiaHerets]

@kadiealexander FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[melvin-bot]

@kadiealexander Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

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

[melvin-bot]

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

[klajdipaja]

We have a PR that addresses a different bug in the mark as unread functionality from issue#47817 that I worked on. The result of that PR is that the marker line does not show on the main chat when a message in a thread is marked unread.

If it is expected that the thread itself also shows as unread than I can work on this issue after the initial one is merged. IMO the New Message marker here would be confusing as it creates the idea that all messages in the main chat are unread were in fact only in the thread there are unread messages. We should add an indicator in the thread itself, for example if the user has 4 replies and 1 is new based the mark unread functionality, we can show: "1 New reply" instead of "4 Replies".

FYI @mollfpr

[bernhardoj]

Edited by proposal-police: This proposal was edited at 2024-09-25 08:33:00 UTC.

Proposal

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

New message marker is shown after coming back from a thread.

What is the root cause of that problem?

When we coming back from a thread, only part of the report actions are shown in the list. Let say we have message 1-5, go to thread 2 and come back, only message 1-2 are initially visible becuase of comment linking. There is a useEffect that updates the unreadMarkerTime to the last action created time. In our case, the unreadMarkerTime is updated to message 2 created time.

App/src/pages/home/report/ReportActionsList.tsx

Lines 276 to 286 in accea60

	useEffect(() => {
	if (unreadMarkerReportActionID) {
	return;
	}
	const mostRecentReportActionCreated = sortedVisibleReportActions[0]?.created ?? '';
	if (mostRecentReportActionCreated === unreadMarkerTime) {
	return;
	}
	setUnreadMarkerTime(mostRecentReportActionCreated);

When all report actions are rendered, the unreadMarkerReportActionID will return the 3rd message ID because the created value is bigger than unreadMarkerTime.

App/src/pages/home/report/ReportActionsList.tsx

Lines 223 to 246 in accea60

	const unreadMarkerReportActionID = useMemo(() => {
	const shouldDisplayNewMarker = (reportAction: OnyxTypes.ReportAction, index: number): boolean => {
	const nextMessage = sortedVisibleReportActions[index + 1];
	const isCurrentMessageUnread = isMessageUnread(reportAction, unreadMarkerTime);
	const isNextMessageRead = !nextMessage || !isMessageUnread(nextMessage, unreadMarkerTime);
	let shouldDisplay = isCurrentMessageUnread && isNextMessageRead && !ReportActionsUtils.shouldHideNewMarker(reportAction);
	if (shouldDisplay && !messageManuallyMarkedUnread) {
	// Prevent displaying a new marker line when report action is of type "REPORT_PREVIEW" and last actor is the current user
	const isFromCurrentUser = accountID === (ReportActionsUtils.isReportPreviewAction(reportAction) ? !reportAction.childLastActorAccountID : reportAction.actorAccountID);
	const isWithinVisibleThreshold = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < (userActiveSince.current ?? '') : true;
	shouldDisplay = !isFromCurrentUser && isWithinVisibleThreshold;
	}
	return shouldDisplay;
	};
	// Scan through each visible report action until we find the appropriate action to show the unread marker
	for (let index = 0; index < sortedVisibleReportActions.length; index++) {
	const reportAction = sortedVisibleReportActions[index];
	if (shouldDisplayNewMarker(reportAction, index)) {
	return reportAction.reportActionID;
	}
	}

This happens after #48445. Previously, we only update unreadMarkerTime when receiving new message, that is if the last action created value is bigger than the current report lastReadTime. But it's not complete because it doesn't update the unreadMarkerTime when the last action is deleted, which is what #48445 is fixing.

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

Revert the changes from #48445 since #47735 is not happening anymore.

What alternative solutions did you explore? (Optional)

Since the useEffect purpose is updating the unreadMarkerTime when adding/deleting message, then we can ignore effect when on mount.

useEffect(() => {
    if (firstRenderRef.current) {
        firstRenderRef.current = false;
        return;
    }
    ...

    setUnreadMarkerTime(mostRecentReportActionCreated);
    setMessageManuallyMarkedUnread(0);
}, [sortedVisibleReportActions]);

[s77rt]

@bernhardoj Thanks for the proposal.

There is a useEffect that updates the unreadMarkerTime to the last action created time. In our case, the unreadMarkerTime is updated to message 2 created time.

This seems to be where things take a wrong turn. The previous logic makes more sense because the unread marker should not be less than the report last read time. i think it's best to revert the recent change and find another solution to #47735

[bernhardoj]

You're right. I wasn't considering the case where the action created is smaller than the last read time because of comment linking. I have updated my proposal to revert #48445 and after reverting it, I can't repro #47735 anymore after #49367 is merged, specifically this line of changes.

[bernhardoj]

So, looks like #49367 causes this issue, but reverting #48445 will solve this and there is a dupe issue here.

[klajdipaja]

@bernhardoj this issue was opened before #49367 was merged so I don't think it caused it

[s77rt]

@bernhardoj After discussion we will just have the assigned C+ on the linked issue revert the PR

[s77rt]

🎀 👀 🎀 For engineer PR revert review

[melvin-bot]

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

[dangrous]

Left one comment on the linked PR.

Do I need to assign anyone here or are we all set?

[s77rt]

All set

[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 9.0.40-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Revert "Merge pull request #48445" #49770

If no regressions arise, payment will be issued on 2024-10-04. 🎊

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

• @s77rt requires payment through NewDot Manual Requests

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

• [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
• [@s77rt] 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:
• [@s77rt] 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:
• [@s77rt] Determine if we should create a regression test for this bug.
• [@s77rt] 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.
• [@kadiealexander] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[s77rt]

No checklist needed. We just reverted the offending PR.