[$1000] New messages aren't showing up as unread and bold in LHN

[sakluger]

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. I received a push notification of a new message
2. I looked in NewDot but the chat doesn't show up in the LHN (I'm in Focus mode)
3. When I switch to Most Recent mode, the chat shows up as read

Expected Result:

New messages should always show up as unread and appear in the LHN

Actual Result:

The message shows as read in the LHN in Most Recent mode, and doesn't show up at all in the LHN in focus mode.

Workaround:

I get these messages as desktop notifications, but I still have missed some.

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: v1.3.45-3
Reproducible in staging?: Yes
Reproducible in production?: Not sure

Email or phone of affected tester (no customers): sasha@expensify.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Desktop notification:

LHN in Most Recent mode:

Issue reported by: @sakluger
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690309413291369

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01519e1ea6dde7dd1f
• Upwork Job ID: 1683968671592755200
• Last Price Increase: 2023-07-25

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[sakluger]

@NikkiWines reproduced this, so I'm going to go ahead and add the external label myself to move it along.

[tienifr]

Proposal

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

New messages received via Pusher are marked as read in focus mode.

What is the root cause of that problem?

This only happens when you previously open the target report in default mode then switch to focus mode.

If we open the target report, we push its reportID to the new report action subscribers:

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

Lines 127 to 129 in 0fec944

	// 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.
	unsubscribeFromNewActionEvent.current = Report.subscribeToNewActionEvent(props.report.reportID, (isFromCurrentUser, newActionID) => {

The event subscriber is supposed to be removed when the report unmount. However, since navigation refactor we allow multiple report screens to be on top of each other. As a result the report screens wouldn't unmount.

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

Lines 155 to 163 in 1fac96b

	return () => {
	if (unsubscribeFromNewActionEvent.current) {
	unsubscribeFromNewActionEvent.current();
	}
	Report.unsubscribeFromReportChannel(props.report.reportID);
	};
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, []);

So, even when the report was hidden in focus mode, the new action subscriber for that report has not been removed; and eventually triggered readNewestAction.

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

Lines 139 to 144 in 0fec944

	// We use the scroll position to determine whether the report should be marked as read and the new line indicator reset.
	// If the user is scrolled up and no new line marker is set we will set it otherwise we will do nothing so the new marker
	// stays in it's previous position.
	if (currentScrollOffset.current === 0) {
	Report.readNewestAction(props.report.reportID);
	setNewMarkerReportActionID('');

There's another issue that if you have previously opened a report then navigate to another one, when a new message is notified, it'll be marked as read. On the other hand, if you haven't opened that report before, it is unread. I think whenever we're not focusing on the report, any new message to the report should be unread. It's here: #23446.

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

Unsubscribe the new action event when reportID changes here:

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

Lines 155 to 163 in 1fac96b

	return () => {
	if (unsubscribeFromNewActionEvent.current) {
	unsubscribeFromNewActionEvent.current();
	}
	Report.unsubscribeFromReportChannel(props.report.reportID);
	};
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, []);

But we cannot simply add props.report.reportID to the dependency list as it's not the current visible report. Instead, I use Navigation.getTopmostReportId to retrieve the current reportID then use as the dependency.

What alternative solutions did you explore? (Optional)

The currentScrollOffset was initialized to 0 which make the check mentioned in the RCA correct.

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

Line 76 in 0fec944

const currentScrollOffset = useRef(0);

Thus, we can initialize currentScrollOffset to undefined instead of 0.

const currentScrollOffset = useRef(undefined);

[sakluger]

@tienifr thanks for the proposal, do you know if this is a regression? I never noticed this behavior until yesterday (or maybe Monday at the earliest).

[StevenKKC]

I think his issue has the same root cause as #23446, and I have already posted my proposal in #23446 (comment).

Proposal

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

Unread not working on the LHN.

What is the root cause of that problem?

If user B received a message from user A, the following code is executed, and then calculate new marker actions.
If the scroll position is 0, then call Report.readNewestAction in order to update report's lastReadTime.

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

Lines 129 to 153 in c779139

	unsubscribeFromNewActionEvent.current = Report.subscribeToNewActionEvent(props.report.reportID, (isFromCurrentUser, newActionID) => {
	const isNewMarkerReportActionIDSet = !_.isEmpty(newMarkerReportActionID);
	// 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) {
	reportScrollManager.scrollToBottom();
	// If the current user sends a new message in the chat we clear the new marker since they have "read" the report
	setNewMarkerReportActionID('');
	} else if (isReportFullyVisible) {
	// We use the scroll position to determine whether the report should be marked as read and the new line indicator reset.
	// If the user is scrolled up and no new line marker is set we will set it otherwise we will do nothing so the new marker
	// stays in it's previous position.
	if (currentScrollOffset.current === 0) {
	Report.readNewestAction(props.report.reportID);
	setNewMarkerReportActionID('');
	} else if (!isNewMarkerReportActionIDSet) {
	// The report is not in view and we received a comment from another user while the new marker is not set
	// so we will set the new marker now.
	setNewMarkerReportActionID(newActionID);
	}
	} else if (!isNewMarkerReportActionIDSet) {
	setNewMarkerReportActionID(newActionID);
	}
	});

In this issue, user B already navigates to another report, but previous report view is not unmounted, and still receive push event.
So when receive new action, Report.readNewestAction is called, and lastReadTime is updated.
Therefore, previous report does not marked as unread in LHN.

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

We should add checking whether the report is visible (i.e. the current report) in new action event handler as below.

    unsubscribeFromNewActionEvent.current = Report.subscribeToNewActionEvent(props.report.reportID, (isFromCurrentUser, newActionID) => {
+       if (Navigation.getTopmostReportId() === props.report.reportID) {
+           return;
+       }

What alternative solutions did you explore? (Optional)

None.

[sakluger]

FYI we're treating this as a fire and so I've applied the hourly label.

[tienifr]

I think it's a regression from #22645. Before that the new action subscriber was an object and allowed only one subscriber (i.e. reportID) at a time. Thus the later report subscriber would overwrite the previous ones. But then it was migrated to a list, which requires a manual un-subscription.

[mallenexpensify]

Thanks for treating as a fire and assigning hourly, this is happening to me too. I updated the title to include and bold since bold is what I searched for yesterday when I filed a similar bug report.

[lschurr]

Should we take this internal @sakluger @mallenexpensify or are we waiting for proposals here?

[lschurr]

@Santhosh-Sellavel are you able to review the proposal here?

[Santhosh-Sellavel]

I think it's a regression from #22645. Before that the new action subscriber was an object and allowed only one subscriber (i.e. reportID) at a time. Thus the later report subscriber would overwrite the previous ones. But then it was migrated to a list, which requires a manual un-subscription.

@mountiny @allroundexperts Seems its a regression from #22645, please take care.

[allroundexperts]

@lazar-stamenkovic Can you please confirm this?

[mountiny]

The issue in op has been fixed by the PR from Hanno, right @allroundexperts ?

[allroundexperts]

The issue in op has been fixed by the PR from Hanno, right @allroundexperts ?

That's correct, yes. Still, I would want to check if this was really related to #22645

[lazar-stamenkovic]

@lazar-stamenkovic Can you please confirm this?

@allroundexperts confirming. seemed it's related to #22645. but while working on the PR #22645, subscribeToNewActionEvent and unsubscribeFromNewActionEvent in ReportActionsView was correctly called and it doesn't have this issue.

But now unsubscribeFromNewActionEvent isn't called and subscribeToNewActionEvent is called for previous report id when move to other pages.
But I can see some properties and actions are updated in ReportActionsView. not sure which PR it was updated in.
for example, unsubscribeFromNewActionEvent is removed in ReportActionsView.
today i will check this in detail.

[lschurr]

To clarify, are you working on this @allroundexperts? Should this GH be assigned to you?

[allroundexperts]

@lschurr This issue has been fixed already by a PR from @hannojg. I was just confirming if this is a regression from #22645 but according to the comment from @lazar-stamenkovic, it seems like this was not the case.

[mountiny]

I think we can close this issue now

[lschurr]

Closing!