[HOLD for payment 2024-11-07] [$250] Chat: "Unread" marker appears when the user sends a new message

[m-natarajan]

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: 9.0.46-2
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: @paultsimura
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1728390912425749

Action Performed:

1. Open the chat with long history.
2. Scroll up until last messages are not visible.
3. Send a message.
4. Immediately scroll up.

• The "Unread" green line marker appears above the new message;

Expected Result:

Since the user is the author of the message:

• The "New messages" pill shouldn't appear;
• The "Unread" green line marker shouldn't appear above the new message;

Actual Result:

The "New messages" pill appears;

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

2024-10-08.-.14_19.-.Screen.Recording.2024-10-08.at.14.18.44.mp4

Recording.640.mp4

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021844272500597248585
• Upwork Job ID: 1844272500597248585
• Last Price Increase: 2024-10-10
• Automatic offers:
• ikevin127 | Reviewer | 104460359

Issue Owner

Current Issue Owner: @lschurr

[melvin-bot]

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

[MonilBhavsar]

I would like to curate this so assigning myself

[lschurr]

Do you need BugZero on this @MonilBhavsar or will it be all internal?

[MonilBhavsar]

We need BugZero and C+ on this. Going to export it

[melvin-bot]

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

[melvin-bot]

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

[daledah]

Edited by proposal-police: This proposal was edited at 2024-10-10 08:56:49 UTC.

Proposal

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

The "New messages" pill appears;

What is the root cause of that problem?

unreadMarkerReportActionID returns the new action when users add new comment, the reason is because we don't have the logic to exclude the message that comes from current user

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

In

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

Lines 483 to 488 in 175af25

	const hasUnreadReportAction = sortedVisibleReportActions.some(
	(reportAction) =>
	newMessageTimeReference &&
	newMessageTimeReference < reportAction.created &&
	(ReportActionsUtils.isReportPreviewAction(reportAction) ? reportAction.childLastActorAccountID : reportAction.actorAccountID) !== Report.getCurrentUserAccountID(),
	);

when checking hasUnreadReportAction we exclude the current user's message so we should do the same in

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

Line 224 in 175af25

const shouldDisplay = isCurrentMessageUnread && isNextMessageRead && !ReportActionsUtils.shouldHideNewMarker(reportAction);

            const shouldDisplay = isCurrentMessageUnread && isNextMessageRead && !ReportActionsUtils.shouldHideNewMarker(reportAction) && (
                (ReportActionsUtils.isReportPreviewAction(reportAction) ? reportAction.childLastActorAccountID : reportAction.actorAccountID) !== Report.getCurrentUserAccountID()
            );

What alternative solutions did you explore? (Optional)

1. We can consider to create the util function to avoid duplicate code
2. We can update isMessageUnread function to exclude the current user's message

[bernhardoj]

Edited by proposal-police: This proposal was edited at 2024-10-15 14:10:25 UTC.

Proposal

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

An unread marker shows every time we add a message.

What is the root cause of that problem?

This happens after #49367. Previously, we ignore any message that the current user sends, unless the user mark it as unread manually.

The unread marker shows because the scroll offset is out of the threshold (250) and isMessageUnread for the new message is true.

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

Lines 224 to 226 in 285bde8

	const shouldDisplay = isCurrentMessageUnread && isNextMessageRead && !ReportActionsUtils.shouldHideNewMarker(reportAction);
	const isWithinVisibleThreshold = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < (userActiveSince.current ?? '') : true;
	return shouldDisplay && isWithinVisibleThreshold;

isMessageUnread will be true if the report action created value is bigger than the unreadMarkerTime.

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

Lines 125 to 131 in 285bde8

	function isMessageUnread(message: OnyxTypes.ReportAction, lastReadTime?: string): boolean {
	if (!lastReadTime) {
	return !ReportActionsUtils.isCreatedAction(message);
	}
	return !!(message && lastReadTime && message.created && lastReadTime < message.created);
	}

The unreadMarkerTime is updated in 3 cases as written on the comment below.

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

Lines 202 to 214 in 285bde8

	* The timestamp for the unread marker.
	*
	* This should ONLY be updated when the user
	* - switches reports
	* - marks a message as read/unread
	* - reads a new message as it is received
	*/
	const [unreadMarkerTime, setUnreadMarkerTime] = useState(report.lastReadTime ?? '');
	useEffect(() => {
	setUnreadMarkerTime(report.lastReadTime ?? '');
	// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
	}, [report.reportID]);

For the 3rd case, we have this useEffect that updates the unreadMarkerTime, but only if unreadMarkerReportActionID doesn't exist, which exist in our case because we are out of the scroll threshold.

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

Lines 265 to 278 in 285bde8

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

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

We will have a new useEffect that is similar to the 3rd case, but in this case, we will only update the unreadMarkerTime if there is a new last action created value from the current user and there is previously no unread marker.

When user adds a new message and there is currently an unread marker, the prevUnreadMarkerReportActionID will always have a value.

For example, the current user mark A as unread:
prevUnreadMarkerReportActionID: undefined
unreadMarkerReportActionID: A

then, the current user sends a message. prevUnreadMarkerReportActionID will now contain A.

const prevUnreadMarkerReportActionID = usePrevious(unreadMarkerReportActionID);
const lastAction = sortedVisibleReportActions.at(0);
useEffect(() => {
    if (prevUnreadMarkerReportActionID || !lastAction) {
        return;
    }

    const isFromCurrentUser = accountID === (ReportActionsUtils.isReportPreviewAction(lastAction) ? !lastAction.childLastActorAccountID : lastAction.actorAccountID);

    if (!isFromCurrentUser || lastAction.created <= unreadMarkerTime) {
        return;
    }

    setUnreadMarkerTime(lastAction.created);
}, [prevUnreadMarkerReportActionID, lastAction?.created]);

What alternative solutions did you explore? (Optional)

We need to add a new case for updating unreadMarkerTime if a new message is added by the current user, even when the user scroll offset is out of threshold.

Luckily, we already have an observer to detect a new message from the current user.

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

Line 371 in 285bde8

const unsubscribe = Report.subscribeToNewActionEvent(report.reportID, scrollToBottomForCurrentUserAction);

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

Lines 341 to 353 in 285bde8

	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());
	},

We can update the unreadMarkerTime inside the callback, scrollToBottomForCurrentUserAction, but that only covers for optimistic case. After the AddComment API success, the BE will send us a new action created which is bigger from the optimistic created, so we still need to update the unreadMarkerTime when receiving the API response.

To do that, we can have a new ref called lastActionAddedByCurrentUser. We set the ref every time we send a message. reportActionID is from the 2nd parameter of the callback (scrollToBottomForCurrentUserAction).

if (!unreadMarkerReportActionID) {
    lastActionAddedByCurrentUser.current = reportActionID;
}

It's important to check for existing unread marker to not remove it when adding a new message, which is the current behavior. If we want to remove the marker when adding a new message, then we can remove the if. Make sure to add unreadMarkerReportActionID to the useCallback deps and add scrollToBottomForCurrentUserAction to this useEffect where it's being registered.

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

Lines 371 to 385 in 285bde8

	const unsubscribe = Report.subscribeToNewActionEvent(report.reportID, scrollToBottomForCurrentUserAction);
	const cleanup = () => {
	if (!unsubscribe) {
	return;
	}
	unsubscribe();
	};
	newActionUnsubscribeMap[report.reportID] = cleanup;
	return cleanup;
	// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
	}, [report.reportID]);

Next, adds a useEffect that updates unreadMarkerTime whenever the last action is updated. If the last action from props is the same as the last action saved to the ref, then we update the unreadMarkerTime to that last action created, so unreadMarkerTime will equal to the last action created.

const lastAction = sortedVisibleReportActions.at(0);
useEffect(() => {
    if (!lastActionAddedByCurrentUser.current || lastAction?.reportActionID !== lastActionAddedByCurrentUser.current) {
        return;
    }
    setUnreadMarkerTime(lastAction.created);

    if (lastAction.isOptimisticAction) {
        return;
    }

    lastActionAddedByCurrentUser.current = undefined;
}, [lastAction]);

lastActionAddedByCurrentUser should be cleared after being used, but we only want to clear that if the last action is not an optimistic action anymore because we still want to update the unreadMarkerTime when receiving the new action from the API response.

Next, return false here if the report action is the same as the last action added by user.

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

Lines 220 to 227 in 285bde8

	const shouldDisplayNewMarker = (reportAction: OnyxTypes.ReportAction, index: number): boolean => {
	const nextMessage = sortedVisibleReportActions.at(index + 1);
	const isCurrentMessageUnread = isMessageUnread(reportAction, unreadMarkerTime);
	const isNextMessageRead = !nextMessage || !isMessageUnread(nextMessage, unreadMarkerTime);
	const shouldDisplay = isCurrentMessageUnread && isNextMessageRead && !ReportActionsUtils.shouldHideNewMarker(reportAction);
	const isWithinVisibleThreshold = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < (userActiveSince.current ?? '') : true;
	return shouldDisplay && isWithinVisibleThreshold;
	};

return shouldDisplay && isWithinVisibleThreshold && reportAction.reportActionID !== lastActionAddedByCurrentUser.current;

Last, we need to clear lastActionAddedByCurrentUser when mark as unread/read manually.

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

Lines 245 to 251 in 285bde8

	const unreadActionSubscription = DeviceEventEmitter.addListener(`unreadAction_${report.reportID}`, (newLastReadTime: string) => {
	setUnreadMarkerTime(newLastReadTime);
	userActiveSince.current = DateUtils.getDBTime();
	});
	const readNewestActionSubscription = DeviceEventEmitter.addListener(`readNewestAction_${report.reportID}`, (newLastReadTime: string) => {
	setUnreadMarkerTime(newLastReadTime);
	});

[ikevin127]

♻️ Reviewing proposals...

[ikevin127]

From my experience with the ReportActionsList.tsx unread marker functionality I know that there's always more to consider and whenever a straight-forward fix was suggested in the past, we always had regressions because there's a lot of functionality which can be disrupted even with small changes.

Given this I'm inclined to go with bernhardoj's proposal since it mentioned the offending PR which caused the issue and also both RCA and solution proposed are more detailed and consider multiple aspects of the current functionality - hence it shows better understanding of the issue at hand.

However, before moving on with assignment here I want test the proposed solution thoroughly and because I don't want to miss any part of the solution, @bernhardoj can you please share a test branch with the complete solution so I can test it ?

♻️ Will get back on this once I get the chance to test the proposed solution.

[bernhardoj]

@ikevin127 here is the test branch

[ikevin127]

@bernhardoj Thanks for putting together the test branch, I got to test the fix implementation and while it does seem to work compared to staging:

Compare Videos

Staging	Test Branch
staging.mov	test-branch.mov

⚠️ However I found an edge case in which the fix doen't work, allow me to explain the steps and show you a video demonstrating the issue:

Tip

Steps to reproduce

1. Login into the same account on 2 separate browser windows, one on local dev (with testing branch) and one on current staging (https://staging.new.expensify.com).
2. On local dev (with testing branch), verify that you can NOT reproduce the issue by following the steps in issue's description.
3. On local dev (with testing branch), make sure there is no new message marker or pill showing and scroll up such that you cannot see the last message.
4. On the same account / report on Staging, verify that you CAN reproduce the issue by following the steps in issue's description.
5. Now returning to the local dev test branc, observe that the new message marker and pill are showing even though you posted them with the same user account from Staging.
6. On local dev (with testing branch), mark as read to ensure there is no new message marker or pill showing then repeat step (2.) and notice that the fix does not work anymore after doing the Staging / Dev back n' forth.

Note

The same thing happens if the steps are followed on 2 different sessions of the local dev (with testing branch) for example if logging in with the same account and following the above steps on 2 different browsers.

Here's a video for better visualisation:

Video

staging-test-issue.mov

Given this, from my side I see two issues here:

1. Regarding step (5.) and according to the issue's expected result, we should not see the new message marker and pill showing-up because the message was posted by the same user, even though it was posted from a different (Staging) session.
2. Regarding step (6.) which seems to somehow be caused and linked to step (5.), it looks like when the edge case steps are performed, something happens which invalidates the solution implementation and causes it to not work anymore.

[bernhardoj]

I updated my proposal with a different solution to cover that (and moved the previous main solution as the alternative).

[ikevin127]

I reviewed @bernhardoj's updated proposal's main solution again and this time it passed the tests.
Given this I think we're good to go with their proposal as they show a deep understanding of the issue / functionality, the RCA is valid and the updated solution fixes the issue as per the expected result, without altering current working functionality.

For reference, here's how I implemented the solution locally:

Solution Code

    const [session] = useOnyx(ONYXKEYS.SESSION);
    const prevUnreadMarkerReportActionID = usePrevious(unreadMarkerReportActionID);
    const currentUserAccountID = Number(session?.accountID);
    const lastAction = sortedVisibleReportActions.at(0);
    
    useEffect(() => {
        if (prevUnreadMarkerReportActionID || !lastAction) {
            return;
        }

        const isFromCurrentUser = currentUserAccountID === (ReportActionsUtils.isReportPreviewAction(lastAction) ? !lastAction.childLastActorAccountID : lastAction.actorAccountID);

        if (!isFromCurrentUser || lastAction.created <= unreadMarkerTime) {
            return;
        }

        setUnreadMarkerTime(lastAction.created);
    }, [prevUnreadMarkerReportActionID, lastAction?.created]);

Tip

Notes for PR

1. Make sure to comment the new useEffect in detail for others to understand why we need it and what it does.
2. Ensure thorough testing on narrow layout devices (native / mweb) since on these devices the unread marker / pill can work slightly different because of the DeviceEventEmitter listeners and LHN / Central Pane not being mounted both at the same time.

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignee @MonilBhavsar is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[MonilBhavsar]

Thanks for the detailed proposal!
Looks good 👍
Let's also make sure to test other unread marker cases to ensure we don't introduce regression

[melvin-bot]

📣 @ikevin127 🎉 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]

I'm still struggling to make the solution fully works. If you see the video below, while doing it offline (easier to repro), the unread marker will still show briefly.

ios.mweb.mp4

With the current solution, we can't really avoid that because the useMemo is always calculated first before we update the unreadMarkerTime.

[bernhardoj]

I'm rethinking to reuse the previous solution. The advantage of the previous solution is that we can tell that there is a new message added by the current user before unreadMarkerReportActionID is recalculated. And to overcome this issue where the unread marker still shows when adding message from another client, we can trigger the observer too when receiving the new message from Pusher.

If we read the comment here, the observer/subscriber is supposed to receive the event either locally or from Pusher.

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

Lines 376 to 378 in 40e2230

	// This callback is triggered when a new action arrives via Pusher and the event is emitted from Report.js. This allows us to maintain
	// a single source of truth for the "new action" event instead of trying to derive that a new action has appeared from looking at props.
	const unsubscribe = Report.subscribeToNewActionEvent(report.reportID, scrollToBottomForCurrentUserAction);

However, currently the event is only triggered inside the notification callback. So, it's only triggered if the user will receive a notification which being guarded by some conditions.

App/src/libs/actions/Report.ts

Line 2498 in 40e2230

notifyNewAction(reportID, reportAction.actorAccountID, reportAction.reportActionID);

App/src/libs/actions/Report.ts

Lines 2467 to 2470 in 40e2230

	function showReportActionNotification(reportID: string, reportAction: ReportAction) {
	if (!shouldShowReportActionNotification(reportID, reportAction)) {
	return;
	}

App/src/libs/actions/User.ts

Lines 746 to 750 in 40e2230

	function triggerNotifications(onyxUpdates: OnyxServerUpdate[]) {
	onyxUpdates.forEach((update) => {
	if (!update.shouldNotify && !update.shouldShowPushNotification) {
	return;
	}

The solution is to move the notifyNewAction calls outside the notification callback.

pushJSON.forEach((update) => {
    if (!update.key.startsWith(ONYXKEYS.COLLECTION.REPORT_ACTIONS)) {
        return;
    }
    const reportID = update.key.replace(ONYXKEYS.COLLECTION.REPORT_ACTIONS, '');
    const reportActions = Object.values((update.value as OnyxCollection<ReportAction>) ?? {});
    reportActions.forEach((action) => action && Report.notifyNewAction(reportID, action.actorAccountID, action.reportActionID));
});
const onyxUpdatePromise = Onyx.update(pushJSON).then(() => {
    triggerNotifications(pushJSON);
});

With the previous + new solutions above, the unread marker won't show if the current user adds a message, either from the current client or a different client.

@ikevin127 what do you think?

NOTE: sending message from the 2nd client will scroll to bottom the first client too. This is the same as WhatsApp.

[ikevin127]

@bernhardoj Thanks for catching the offline behaviour issue, didn't think to test that scenario with the previous solution.

#50469 (comment) sounds good to me, I'd say you can move forward and start working on the PR with the (1st) previous solution + the new solution mentioned in above comment.

Unless @MonilBhavsar has any objections to #50469 (comment), I'd say you're good to go.

Note

We just have to make sure we test properly and also make sure to add code comments for the new code additions in order for other contributors to know why and what the code does and why the notifyNewAction call has to be moved outside the notification callback.

[bernhardoj]

PR is ready

cc: @ikevin127

[ikevin127]

♻️ Status update: We're working on reviewing and fixing edge cases in the PR, hopefully will be merged in 1-2 days.

[MonilBhavsar]

Thanks for the update! The new solution looks right 👍

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

• Fix new message is shown when send message #51113

If no regressions arise, payment will be issued on 2024-11-07. 🎊

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

• @bernhardoj requires payment through NewDot Manual Requests
• @ikevin127 requires payment automatic offer (Reviewer)
• @paultsimura requires payment (Needs manual offer from BZ)

[melvin-bot]

@ikevin127 @lschurr The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[ikevin127]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production
• 2b. Reported on staging (deploy blocker)
• 2c. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] 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: [GH-47817]- remove messageManuallyMarkedUnread and manual handling of… #49367 (comment).

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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: No need as the regression wasn't critical / interrupting a core flow.

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

• [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

  Link to issue: https://github.com/Expensify/Expensify/issues/442577

Regression Test Proposal

Test:

1. Open any chat that is scrollable (has longer conversation history.)
2. Scroll to the top of the chat.
3. Send a message and immediately scroll to the top as the UX will try to scroll to bottom once the message is sent.
4. Now scroll up/down slightly and verify that there's no unread marker above the newly sent message and no New messages pill showing up at the top of the chat report when the newly sent message is not visible (out of scroll view).

Note

An easier way to test this is to go offline / back online after posting a message, then there's no need to immediately scroll to the top, especially on small screen where there's no draggable scrollbar.

Please refer to the issue's description for see of how the issue was reproduced to better understand how to verify that the issue is not reproducible anymore.

Do we agree 👍 or 👎.

[lschurr]

Payment summary:

• Contributor: @bernhardoj $250 (please request via Newdot)
• Reviewer: @ikevin127 $250 (paid in Upwork)

[bernhardoj]

Requested in ND.

[JmillsExpensify]

$250 approved for @bernhardoj