[$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).