[HOLD for payment 2023-09-27] [$500] Desktop - Unread messages get marked as read without me visiting the chats

[kbecciv]

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. Send the message from Account A (mobile) to Account B (Desktop app)
2. Check if message is Unread in Desktop app

Expected Result:

Unread messages displayed in LHN

Actual Result:

Unread messages get marked as read without me visiting the chats

Workaround:

Unknown

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: 1.3.64.0
Reproducible in staging?: n/a
Reproducible in production?: n/a
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
Notes/Photos/Videos: Any additional supporting documentation

n/a

Expensify/Expensify Issue URL:
Issue reported by: @rafecolton
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1693523157400069

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0100b1d4dc36ebfcc0
• Upwork Job ID: 1699281016069742592
• Last Price Increase: 2023-09-06
• Automatic offers:
• tienifr | Contributor | 26649040

[melvin-bot]

Triggered auto assignment to @sophiepintoraetz (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/~0100b1d4dc36ebfcc0

[melvin-bot]

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

[melvin-bot]

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

[sophiepintoraetz]

Given that there are multiple reports, I'm going to go ahead and put this one out there.

[sophiepintoraetz]

cc @danieldoglas - as Rocio's theory is that this is coming from #vip-reliable-updates - do you agree?

[tienifr]

Proposal

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

Desktop - Unread messages get marked as read without me visiting the chats.

What is the root cause of that problem?

We have the logic to call OpenReport API when the visibility change. The result of this API will update the lastReadTime to current time -> unread style will be disappeared

App/src/pages/home/ReportScreen.js

Lines 245 to 253 in 1223e1e

	const unsubscribeVisibilityListener = Visibility.onVisibilityChange(() => {
	// If the report is not fully visible (AKA on small screen devices and LHR is open) or the report is optimistic (AKA not yet created)
	// we don't need to call openReport
	if (!getIsReportFullyVisible(isTopMostReportId) || report.isOptimisticReport) {
	return;
	}
	Report.openReport(report.reportID);
	});

Although, we checked getIsReportFullyVisible(isTopMostReportId), but we're using isTopMostReportId in the callback listener, and inside this callback function we can't detect the latest value of isTopMostReportId, because it will be evaluated only once when the useEffect is run.

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

Solution 1: we can use useRef for isTopMostReportId, so it will be evaluated again when the listener run.

useEffect(() => {
    isTopMostReportId.current = Navigation.getTopmostReportId() === getReportID(route);
}, [route])

Solution 2: Add additional logic to check isTopMostReportId inside onVisibilityChange's callback function as what we did before changing to use functional component

const isTopMostReportId = Navigation.getTopmostReportId() === getReportID(route);
if (!getIsReportFullyVisible(isTopMostReportId) || report.isOptimisticReport) {
    return;
}

Report.openReport(report.reportID);

Or we can wrap it to an external function and call it in both places.

Result

Screen.Recording.2023-09-06.at.14.45.22.mov

[danieldoglas]

@sophiepintoraetz add my comment here, but I don't think it's related to #reliable-updates. And thought the symptoms are similar, I think we're seeing 2 different issues there.

[burczu]

@kbecciv @sophiepintoraetz @tienifr I wasn't able to reproduce it (sending messages from ios app to the desktop app and messages in LHN are not marked as read)... According to this thread on Slack I guess this occurs only in some special circumstances, am I right? What are those circumstances?

[burczu]

Just to inform: I'll be OOO on Thursday and Friday (7-8 September).

[davewish]

Proposal

---

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

Desktop: Unread message gets marked as read , without visiting the opening the message.

What is the root cause of that problem?

In the below , It is not checked whether optionItem is unread or not ,It only checks with mention.

App/src/components/LHNOptionsList/OptionRowLHN.js

Lines 105 to 109 in 57b7606

	const shouldShowGreenDotIndicator =
	!hasBrickError &&
	(optionItem.isUnreadWithMention ||
	ReportUtils.isWaitingForIOUActionFromCurrentUser(optionItem) ||
	(optionItem.isTaskReport && optionItem.isTaskAssignee && !optionItem.isCompletedTaskReport && !optionItem.isArchivedRoom));

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

We should update the line by adding optionItem.isUnread

 const shouldShowGreenDotIndicator =
        !hasBrickError &&
        (optionItem.isUnreadWithMention ||  optionItem.isUnread ||
            ReportUtils.isWaitingForIOUActionFromCurrentUser(optionItem) ||
            (optionItem.isTaskReport && optionItem.isTaskAssignee && !optionItem.isCompletedTaskReport && !optionItem.isArchivedRoom));

what alternative solutions did you explore?(Optional)

N/A

[tienifr]

@burczu you can open the desktop app and web at the same time, and send the message from web to desktop (pls make sure the desktop app is invisible when sending message from web)

[sophiepintoraetz]

@tienifr - are you able to reproduce this?

[tienifr]

Screen.Recording.2023-09-12.at.09.43.12.mp4

@sophiepintoraetz @burczu I still can reproduce

[burczu]

Ok, I've finally reproduced the issue successfully! I didn't notice at first that it is marked as read after some short period of time.

Also reviewed proposals and the solution from @tienifr seem to be correct and I think we could proceed with it.

Solution proposed by @davewish is, I think incorrect - it also fixes the issue but it adds the green dot next to the unread message which shouldn't happen.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

📣 @davewish You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

[davewish]

please , I need clarification , my proposal is reviewed as incorrect by @burczu and job assigned to me . then I will proceed with the job

[tienifr]

@cristipaval I got C+ reviewed of @burczu so I think you assigned wrong contributor

[tienifr]

@davewish I believe there's a mistake of @cristipaval. Let's wait for his response. Thanks

[burczu]

yeah @cristipaval - I've recommended solution from @tienifr

[cristipaval]

Yoo I scrolled up and bottom this issue looking at the proposals but in the end, I messed it up. Sorry all!

[melvin-bot]

📣 @tienifr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[cristipaval]

@tienifr, you should be all set now! 🚀

[melvin-bot]

🎯 ⚡️ Woah @burczu / @tienifr, great job pushing this forwards! ⚡️

The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

• when @tienifr got assigned: 2023-09-13 08:53:32 Z
• when the PR got merged: 2023-09-14 20:35:45 UTC

On to the next one 🚀

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

• [CP Staging] fix: 26824 Unread messages get marked as read without me visiting the chats #27270

If no regressions arise, payment will be issued on 2023-09-27. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @burczu does not require payment (Contractor)
• @tienifr requires payment offer (Contributor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

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

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

[burczu]

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:

• [@burczu] The PR that introduced the bug has been identified. Link to the PR: migrate ReportScreen.js #24111
• [@burczu] 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: migrate ReportScreen.js #24111 (comment)
• [@burczu] 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: Didn't find.
• [@burczu] Determine if we should create a regression test for this bug.
• [@burczu] 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.
• [@sophiepintoraetz] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[sophiepintoraetz]

Payouts due:

Issue Reporter: N/A
Contributor: $750 @tienifr
Contributor+: $750 @burczu - can request seeing as you have completed the BZ checklist, thank you!

Eligible for 50% #urgency bonus? Y

[burczu]

@sophiepintoraetz I'm not eligible for payment as I'm contractor from Callstack.

[sophiepintoraetz]

Oh thanks for confirming, then we are super done!