[$500] Desktop - Chat isn't marked as read if user clicks on notification from Notification Center

[lanitochka17]

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: 1.4.28-0 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: Applause - Internal Team Slack conversation:

Action Performed:

1. Open Desktop App
2. Login as User A
3. From the second device with account of User B sent a message to User A
4. User A received Notification
5. Wait until Notification goes to the Notification center
6. Open Notification center and click on this received notification

Expected Result:

User goes to the correct chat, new messages are marked as read and this chat is no longer marked in bold

Actual Result:

Chat is marked in bold

If User clicks on a notification BEFORE it goes to the notification center, the chat will be marked as read.

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
• [x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/aff3d35b-99ca-4985-a32b-ed1885ec91fc

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~011a42b3684380c039
• Upwork Job ID: 1748411136068030464
• Last Price Increase: 2024-01-19
• Automatic offers:
• ishpaul777 | Reviewer | 28118184

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Triggered auto assignment to @garrettmknight (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot[bot]]

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

[mirimichaelson]

Proposal

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

We want to make sure the chat is mark ed as read when messages have gone into the Notification Center and are clicked from there.

What is the root cause of that problem?

The root of the problem is that the App is not in the ‘Visible’ state when the Notification Center message is clicked, by virtue of the Notification Centre being in front of it. There’s a small if statement in ReportActionsList.js which ensures that if the app isn’t visible (Visibility.isVisible()), messages will not be marked as read, and therefore will remain bold. (Even if they've been clicked on through the Notification Center. The app being visible happens just a little too late.)

if (Visibility.isVisible() && scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD) {
     Report.readNewestAction(report.reportID);
} else {
     readActionSkipped.current = true;
}

Checking the visibility & preventing messages from being marked as read is useful in some cases, ie receiving messages if you’re afk, but unhelpful in multiple other cases where the same bug shows up;

1. You click on a notification within the Notification Center

2. Click on a notification before it goes into the Notification Center, if you have another tab open in front of Expensify (same bug as this ticket)

3. You’re already chatting to someone and receive new messages. The bolding of the text doesn’t go away, until you click to another chat and back again. So again, the same bug. (See the attached video) Screen Recording 2024-01-20 at 04.10.11.mov.zip

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

This is the first bug I’m looking into, so I don’t know exactly what your desired behaviour is. I have some suggestions, so I'm game to listen to what makes the most sense to you in terms of chat functionality :) !

1. Add a visibility check to the useEffect that runs this code: This does not cause any extra re-runs, and fixes the problem.

What alternative solutions did you explore? (Optional)

1. The simple code fix is removing isVisible , although this will mean that messages will be marked as read, even if the user is viewing other apps in front of it. However, this will only happen if they are on the chat page already, and they'll open it up first thing.

2. Adding to the functionality that makes chats visible:

Right now, the functionality for updating to the ‘Read’ state requires clicking on a message in the sidebar, (although this doesn't work for the chat you're currently on.). An option is adding additional functionality so that if a user clicks within their chat, and/or on the chat title in the left hand sidebar, it will trigger the useEffect to update, and mark messages as read.

Let me know what you think! :)

[melvin-bot[bot]]

📣 @mirimichaelson! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[mirimichaelson]

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~01cce98dffb320182a

[melvin-bot[bot]]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[JeremyCroff]

Proposal

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

The unread status styling of a report does not disappear when clicking on the OS notification when it's docked in the Notification Center.

What is the root cause of that problem?

At https://github.com/Expensify/App/blob/main/src/pages/home/report/ReportActionsList.js#L211 The utility that exposes the Visibility has a stale reference to the document state. Since it's returning false even when it is visible, it won't call the update last read timestamp call keeping it unread.

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

If you replace the Visibility.isVisible() with the actual document.visibilityState === 'visible', it will behave as expected. I believe the stale reference does not reference the actual electron window document.

What alternative solutions did you explore? (Optional)

I explored removing the visibility, but this impact the user experience. Also looked at other window methods such as hidden, and focus.

[garrettmknight]

@sobitneupane what do you think of this first proposal?

[jeremy-croff]

My original proposal on this is temporarily hidden because my primary github account is suspended while I am recovering it. Reposting proposal below on this new account.

[jeremy-croff]

Proposal

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

The Chat does not get marked as read if the OS notification gets clicked from the docked notification center state.

What is the root cause of that problem?

We have a Visibility.isVisible() utility function that maps to platform specific implementations. for Electron apps it unreliably returns false when the notification is clicked from the docked notification center because going into the notification center causes the electron app to lose focus. The root cause is because the isVisible logic for electron checks with the main process for focus, rather than visibility. This code can be found here: https://github.com/Expensify/App/blob/main/desktop/main.js#L561

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

We can update the main process logic to check on visibility, rather than focus. This will keep the Visibility.isVisible() semantically accurate, and keep the behavior consistent between platforms as the other platforms like web use this utility for a document.visibilityState === 'visible' check.

Visibility is supported: https://github.com/electron/electron/blob/main/docs/api/browser-window.md#winisvisible

Update after impact analysis: https://github.com/Expensify/App/blob/c3f7bccc1050988c6352ea63fa00bf327761587a/src/libs/Visibility/index.desktop.ts#L11 IsFocus is short circuited to true, because isVisible handles this logic already for electron.

When we change to isVisible logic the following will behave different ( and there are a few more examples like it ) https://github.com/Expensify/App/blob/c3f7bccc1050988c6352ea63fa00bf327761587a/src/libs/actions/Report.ts#L1834 We should implement isFocused seperately, instead of the short circuit. And then add a listener on the main electron process again. To achieve the same logical behaviour here.

[sobitneupane]

I am not around and won't be able to review it soon. @garrettmknight Can we get someone else to review proposal?

[garrettmknight]

Yeah, no problem.

[melvin-bot[bot]]

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

[garrettmknight]

Hey @ishpaul777 - we've got a couple proposals for this one when you've got a minute to review!

[ishpaul777]

sure, reviewing..

[ishpaul777]

Thanks for your proposal @jeremy-croff, RCA makes and solution tests well, Can you do a impact analysis of your solution just want to make sure we didn't break current usage ofVisibility.isVisible.

[jeremy-croff]

Thanks for your proposal @jeremy-croff, RCA makes and solution tests well, Can you do a impact analysis of your solution just want to make sure we didn't break current usage ofVisibility.isVisible.

• Identified that the original visibility to focus mapped logic was implemented when electron had no support for visibility yet. https://github.com/Expensify/App/commit/e7150f92bfd50e5d46ba65e85163bdac808a2229 This gives good indication is was a tech limitation rather than an intentional choice.

• When reviewing out uses of isVisibility, visibility is a more optimistic truthy value than isFocused, so I am reviewing it with the lens of are any unintentional behaviours happening that shouldn't.

• When reviewing impact analysis, the change would be scoped to changes in the electron main file's interpration of visibility which can be found here https://github.com/Expensify/App/blob/main/desktop/main.js#L558, so it wouldn't impact other platforms.

Line by line analysis: https://github.com/Expensify/App/blob/5edf172457de71f5fa77d9ddf30e03726db346a2/src/pages/signin/LoginForm/BaseLoginForm.js#L280 will behave the same because of the last condition will be truthy always

https://github.com/Expensify/App/blob/c3f7bccc1050988c6352ea63fa00bf327761587a/src/libs/actions/Report.ts#L1834 will behave different, but we can implement isFocused seperate for electron ( currently short circuits to true ) to achieve the same logical behaviour here https://github.com/Expensify/App/blob/c3f7bccc1050988c6352ea63fa00bf327761587a/src/libs/Visibility/index.desktop.ts#L11, and then a listener on the main process again.

https://github.com/Expensify/App/blob/c3f7bccc1050988c6352ea63fa00bf327761587a/src/libs/getIsReportFullyVisible.ts#L6 will benefit from similarly from also having an seperate focus implementation

https://github.com/Expensify/App/blob/c3f7bccc1050988c6352ea63fa00bf327761587a/src/components/Form/FormProvider.js#L324 will benefit from similarly from also having an seperate focus implementation

In short, having a seperate isFocus implementation will further align the behavior between web and focus, and mitigate any impact.

[ishpaul777]

Thanks for a detailed analysis. @jeremy-croff

but we can implement isFocused seperate for electron (currently short circuits to true ) to achieve the same logical behaviour here

Can you update your solution to include the changes required for the other cases to work as expected

[jeremy-croff]

Thanks for a detailed analysis. @jeremy-croff

but we can implement isFocused seperate for electron (currently short circuits to true ) to achieve the same logical behaviour here

Can you update your solution to include the changes required for the other cases to work as expected

Updated

[ishpaul777]

Thanks @jeremy-croff, Proposal from @jeremy-croff LGTM.

🎀 👀 🎀 C+ reviewed

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

📣 @jeremy-croff 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 📖

[blimpich]

Sounds good. @jeremy-croff feel free to put up a PR when you have it ready.

@mirimichaelson thank you for posting your first proposal! It looks like you put a lot of effort into figuring out a possible solution, and I appreciate the detail. Although your proposal wasn't chosen I'd encourage you to keep looking around for possible issues to pick up 🙂. Thank you for contributing to this issue's discussion and welcome to the repository!

[blimpich]

Update: we're waiting to see if we can get an Electron maintainer to complete an upstream issue that will impact this issue. Comment here. If no word comes from them then we'll look into this again sometime next week.

[garrettmknight]

Sounds good - will leave this daily for checkins.

[melvin-bot[bot]]

@garrettmknight, @blimpich, @ishpaul777, @jeremy-croff Whoops! This issue is 2 days overdue. Let's get this updated quick!

[blimpich]

We have confirmation from the Electron dev that they are working on the upstream issue. They said they hope to get to it next week.

cc: @ishpaul777 @jeremy-croff.

[jeremy-croff]

We have confirmation from the Electron dev that they are working on the upstream issue. They said they hope to get to it next week.

cc: @ishpaul777 @jeremy-croff.

Okay great. So what we can do for this PR is follow the original proposal, and just implement the correct electron visibility method. End of PR. Simple.

I had referenced some other github issues that the onfocus listener implementation would fix, but i'll pull it out comment on those issues with the proposal seperate.