[$250] mWeb - chat-After deleting mark us unread msg, new green line shown in new msg sent.

[lanitochka17]

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.22
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to https://staging.new.expensify.com/
2. Tap on a chat
3. Send a message
4. Long press the message and select mark as unread
5. Now long press the message and delete the message
6. Send a new message now

Expected Result:

Mark as unread &delete a message, after that if we send a new message, new message green line must not be shown above

Actual Result:

Mark as unread &delete a message, after that if we send a new message, new message green line is shown above

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6577188_1724169473802.screenrecorder-2024-08-20-17-17-51-730.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01fb6fb45334b2068a
• Upwork Job ID: 1828326431288711476
• Last Price Increase: 2024-08-27
• Automatic offers:
• c3024 | Reviewer | 103784660

[melvin-bot]

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

[lanitochka17]

We think that this bug might be related to #vip-vsp

[lanitochka17]

@alexpensify 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

[excile1]

Proposal

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

Deleting the message that is manually marked as unread makes any new messages sent also become marked as unread.

What is the root cause of that problem?

The root cause of this problem is the messageManuallyMarkedUnread state, which does not reset after a message is deleted, and that leads to any messages sent after also be treated as unread.

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

Line 172 in 72a7593

const [messageManuallyMarkedUnread, setMessageManuallyMarkedUnread] = useState(0);

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

Lines 231 to 236 in 72a7593

	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;
	}

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

Since there is already a callback that triggers when the user sends a new message, scrollToBottomForCurrentUserAction:

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

Lines 362 to 376 in 72a7593

	const scrollToBottomForCurrentUserAction = useCallback(
	(isFromCurrentUser: boolean) => {
	// If a new comment is added and it's from the current user scroll to the bottom otherwise leave the user positioned where
	// they are now in the list.
	if (!isFromCurrentUser) {
	return;
	}
	if (!hasNewestReportActionRef.current) {
	Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.reportID));
	return;
	}
	InteractionManager.runAfterInteractions(() => reportScrollManager.scrollToBottom());
	},
	[reportScrollManager, report.reportID],
	);

we can just add an additional check into this callback to reset the messageManuallyMarkedUnread state if we're not displaying the new message marker (probably is also worth giving the callback a rename to reflect this change). I added in a const shouldDisplayNewMarkerRef = useRef(false), which gets set in unreadMarkerReportActionID useMemo:

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

Lines 241 to 247 in 72a7593

	// 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;
	}
	}

(gets set to false before the for loop, and gets set to true if shouldDisplayNewMarker ends up being true.
Then, in the scrollToBottomForCurrentUserAction callback add in a check to setMessageManuallyMarkedUnread to 0 if shouldDisplayNewMarkerRef.current is false.

Before:

Screen.Recording.2024-08-20.at.22.53.04.mp4

After:

Screen.Recording.2024-08-20.at.22.53.49.mp4

[wildan-m]

/## Proposal

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

What is the root cause of that problem?

We are not updating unread marker time when the last unread report action deleted

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

We can update unread marker time when some of the report actions deleted, we can add new condition to indicate some report action deleted add this condition: mostRecentReportActionCreated >= prevMostRecentReportActionCreated

Change this code to:

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

What alternative solutions did you explore? (Optional)

N/A

[bernhardoj]

Proposal

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

The unread line indicator shows again after deleting the unread message and add a message.

What is the root cause of that problem?

When we marked a message as unread, we set the unreadMarkerTime state to the message created time - 1.

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

Lines 255 to 259 in 685bb4c

	useEffect(() => {
	const unreadActionSubscription = DeviceEventEmitter.addListener(`unreadAction_${report.reportID}`, (newLastReadTime: string) => {
	setUnreadMarkerTime(newLastReadTime);
	setMessageManuallyMarkedUnread(new Date().getTime());
	});

which will trigger the recalculation of the unread report action ID.

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

Lines 224 to 250 in 685bb4c

	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;
	}
	}
	return null;
	}, [accountID, sortedVisibleReportActions, unreadMarkerTime, messageManuallyMarkedUnread]);

When we delete the message, the unreadMarkerTime state stays the same. So, when a new message is added, the unreadMarkerTime will still be smaller than the new message created time, thus, the new message is considered unread.

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

We need to update the unreadMarkerTime whenever the report action is deleted. We already have this useEffect that updates the unreadMarkerTime to the latest report action, only if there is no current unread report action (which means the report is read) AND if the latest report action created is bigger than the current unread marker time, which means the current logic only covers updating the unread marker time when adding a new message.

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

Lines 277 to 291 in 685bb4c

	useEffect(() => {
	if (unreadMarkerReportActionID) {
	return;
	}
	const mostRecentReportActionCreated = sortedVisibleReportActions[0]?.created ?? '';
	if (mostRecentReportActionCreated <= unreadMarkerTime) {
	return;
	}
	setUnreadMarkerTime(mostRecentReportActionCreated);
	setMessageManuallyMarkedUnread(0);
	// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
	}, [sortedVisibleReportActions]);

We can expand this by covering both delete and add cases by removing this condition (or changing the condition to strictly equal so we won't call the set state if the value is the same).

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

Lines 283 to 285 in 685bb4c

	if (mostRecentReportActionCreated <= unreadMarkerTime) {
	return;
	}

This way, if a report is read, adding a new message or deleting the last message will update the unread marker time to the latest report action.

As a note, when we delete the last unread message, the unreadMarkerReportActionID is empty, so the effect above will work.

[alexpensify]

I'll add this to my task list to test

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[alexpensify]

@c3024, when you get a chance, please review the proposals and determine which one will fix this issue. Thanks!

---

Heads up, I will be offline until Tuesday, September 3, 2024, and will not actively watch over this GitHub during that period.

If anything urgent is needed here, please ask for help in the #expensify-open-source Slack Room-- thanks!

[melvin-bot]

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

[c3024]

Thank you all for your proposals.

I think we should set setUnreadMarkerTime and setMessageManuallyMarkedUnread without necessarily waiting to send another message. Therefore, resetting messageManuallyMarkedUnread in the scrollToBottomForCurrentUserAction does not seem to be the best choice, as suggested here.

The change in the useEffect as suggested in the other solutions looks correct to me. Simply removing this check here

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

Lines 283 to 285 in 685bb4c

	if (mostRecentReportActionCreated <= unreadMarkerTime) {
	return;
	}

is not appropriate because it was deliberately added here to minimize state changes.

Both solutions suggested here and here fix the issue correctly and are somewhat equivalent because for a read report, the unreadMarkerTime is essentially the same as prevMostRecentReportActionCreated.

Since the solution by @bernhardoj appears simpler to me, I think we can go with his solution here of strictly equal comparison of mostRecentReportActionCreated and unreadMarkerTime.

🎀 👀 🎀 C+ Reviewed

[melvin-bot]

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

[melvin-bot]

📣 @c3024 🎉 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

[bernhardoj]

PR is ready

cc: @c3024

[alexpensify]

Catching up from being OOO, I see the PR is waiting to go to production.

[c3024]

Automation failed here. PR Deployed to production on 6-Sep. Should be on HOLD till 13-Sep or 14-Sep for payment.

cc: @alexpensify

[alexpensify]

Thank you for this summary and I've noted the payment date.

[alexpensify]

Payouts due: 2024-09-12

• Contributor: $250 @bernhardoj - please submit a request in Chat. Thanks!
• Contributor+: $250 @c3024 - Paying via Upwork

Upwork job is here.

[alexpensify]

Closing - I've completed the process in Upwork.

[bernhardoj]

Requested in ND.

[JmillsExpensify]

$250 approved for @bernhardoj