fix: use focus listeners for message read status #35004
Conversation
…ments focus seperately
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
melvin-bot
bot
requested review from
Julesssss
and removed request for
a team
|
@Julesssss Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
jeremy-croff
changed the title
|
@Julesssss I want to make a special note on this test case I ran: When the screen is occluded, but the chat is open on the report it receives a message from - The customer does not receive an notification because the app thinks it's still visible. This seems to be a recent bug with electron visibility returning as true even when occluded on certain devices electron/electron#38955 . This is a regression because the previous electron isFocus would have still sent a notification. I have added additional code in a draft PR to address this: There's also another benefit of that change:
I believe the additional PR is the best customer experience, and addresses all use cases. Since the additional change were out of scope of the original proposal, I am seeking guidance on whether I can include them or not. |
|
I was incorrectly assigned here. Reassigning to @blimpich, and manually assigning @ishpaul777 for the C+ review |
|
@jeremy-croff Can you please record a video scenario explained, with and without the additional changes For future reference: please make sure to add full link to issue, before opening PR for review so automation can work properly |
Thank you for all the tips! Will be sure to do so. Vids have audio with context Before:
Uploading Before additional changes.mp4… After
With.additional.changes.mov |
|
Thanks! |
Because the regression is caused by the usage of isVisible per electron/electron#38955 with no known workarounds. Reverting back to isFocus undoes my proposal. In order to leverage isFocus as a solution, the onfocus listener must be utilized and Bug 2 comes free out of the box! I think my hands are tied here. I have merged in the additional proposed changed as this is both defect free and fixes other issues. and possibly #34698 will have to test later. Heading into office now! |
| const readNewestReportActionOnFocus = () => { | ||
| if(!ReportUtils.isUnread(report)){ | ||
| return | ||
| } | ||
| if (Visibility.hasFocus() && scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD) { | ||
| Report.readNewestAction(report.reportID); | ||
| } else { | ||
| readActionSkipped.current = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we rewrite this like so?
diff --git a/src/pages/home/report/ReportActionsList.js b/src/pages/home/report/ReportActionsList.js
index 05f73032d7..b2ea93da28 100644
@@ -204,10 +204,7 @@ function ReportActionsList({
}, [report.reportID]);
const readNewestReportActionOnFocus = () => {
- if(!ReportUtils.isUnread(report)){
- return
- }
- if (Visibility.hasFocus() && scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD) {
+ if (ReportUtils.isUnread(report) && Visibility.hasFocus() && scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD) {
Report.readNewestAction(report.reportID);
} else {
readActionSkipped.current = true;
There was a problem hiding this comment.
Makes sense, updated.
Thanks!
|
Hey @jeremy-croff, Changes seems to be causing regressions
expected result, chat with B is unread And Honestly I am not confident with changes applied, the ideal solution is to hold for a upstream fix for electron/electron#38955 then revisit this issue instead of workaroud, @blimpich any thoughts ? |
|
@ishpaul777 Yeah ideally we should fix the bug in Electron since it is the root cause. It doesn't look like there is much movement there though so having this sit here seems like not a great option either. I just posted a comment there asking a maintainer who seemed close to fixing it whether they will end up fixing it anytime soon. Lets see if they respond. If we don't think that it'll get fixed upstream anytime soon then I think we should try to make this PR work |
|
Thanks for your thoughts! @blimpich, Lets put this for hold for response and revisit the issue next week. |
I've moved the logic that only reads the active report to be also used in the onfocus hook to address this. Good catch. I'm still pending a full regression and can do so to have this ready while waiting on clarification if we should go with an upstream fix. A case to make for not going with an upstream fix is because I believe visibility is not the right trigger for marking things as read. I currently believe focus is. |
jeremy-croff
force-pushed
the
fix/34829
branch
from
ee25562 to
da88368
Compare
jeremy-croff
changed the title
|
So far no response from upstream repo. I just emailed the engineer in case the GH comment didn't get through to them. I say we give that a day or two and if no response then lets go ahead with this. |
|
Agree 👍 @jeremy-croff Can you please prepare your PR just incase we don't recieve any response |
desktop/main.js
Outdated
| // eslint-disable-next-line no-param-reassign | ||
| event.returnValue = browserWindow && !browserWindow.isDestroyed() && browserWindow.isVisible(); | ||
| }); | ||
|
|
||
| ipcMain.on(ELECTRON_EVENTS.REQUEST_HAS_FOCUS, (event) => { |
There was a problem hiding this comment.
The change is not required if we are checking for focus instead of visibilty right ?
There was a problem hiding this comment.
I believe this change is desired, this implements the correct focus logic for electron. Before it defaulted to true.
There was a problem hiding this comment.
browserWindow.isVisible() is not yet bug free right, this may introduce another unidentified regression somewhere else, so i'd say if not necessary for this solution we remove it, as before we are already checking focus when we check for visibility. we continue using it and keep an eye until electron/electron#38982 is merged upstream, does that makes sense ?
There was a problem hiding this comment.
This makes sense
There was a problem hiding this comment.
@ishpaul777 Updated PR. thanks for the suggestion.
It's indeed not necessary for this solution. The initial thought that moving other logic over to focus would be more consistent in cases where windows are occluded and are still marking it as read. But it's out of this issues scope.
|
We were told that this PR #33680 will resolve the issue, when the i last checked PR it was not staging. the issue was weekly so it got lost in my K2 sorry.. I retested the issue is still reproducable so we should move forward here |
|
@iwiznia this issue is logistically messy. We originally were waiting for an upstream maintainer of Electron to patch Electron, and eventually they did but then the fix got reverted, then we also thought #33680 might fix this issue's root cause. @ishpaul777 all good! It got lost in my k2 as well, I should have been keeping a closer eye on the issue we were holding on. I've lost faith that the upstream maintainers of Electron are going to fix this for us, so lets move forward with this PR. cc: @jeremy-croff |
|
Yes, i retested still reproducable. but i'd like to see a updated proposal from @jeremy-croff as this PR dont have the approved changes initially. Lets move the conv. in the issue.. |
Details
Implement the correct isVisible and isFocus methods for the electron Visibility Util.
When this util was originally implemented, electron did not have win.isVisible yet. original implementation for reference:
e7150f9
Fixed Issues
$ #34829
PROPOSAL: #34829 (comment)
Tests
QA steps;
Given two logged in users, atleast one the electron app
Given created chat
5. Click the green plus icon in the bottom left
6. press Start Chat
7. type the email from person you want to chat with
8. hit add to group to create the chat
Scenario receives notifications and marks as read when clicking on it
When chat is not focused
9. From User A select a chat of Concierge or another one that's not user B
10. From User B sent a message to User A
11. User A received Notification, wait until it docks
12. Open Notification center and click on this received notification
13. Verify the chat is marked as read
When chat is focused and desktop app is minimized
14. From User A select a chat of user B
15. Minimize Desktop App of User A
15. From User B sent a message to User A
16. User A received Notification, wait until it docks
17. Open Notification center and click on this received notification
18. Verify the chat is marked as read
When chat is not focused and desktop app is already visibile
19. Open Desktop App of User A to front view ( visible state on monitor )
20. From User A select a chat of Concierge or another one that's not user B
21. From User B sent a message to User A
22. User A received Notification, wait until it docks
23. Open Notification center and click on this received notification
24. Verify the chat is marked as read
Scenario: chat doesn't receive notifications, but chat marks as read
When: chat is focused and desktop app is visibile
25. Open Desktop App of User A to front view ( visible state on monitor )
26. From User A select a chat of user B
27. From User B sent a message to User A
28. User A doesnt receive notification
29. Verify the chat is marked as read
When: chat is focused and desktop app is occluded but not minimized
30. Open Desktop App of User A to front view ( visible state on monitor )
31. From User Aselect a chat of user B
32. From User B sent a message to User A
33. User A doesnt receive notification
34. Verify the chat is marked as read
Offline tests
QA Steps
Given the
Scenario: two logged in users, atleast one the electron app
and
Scenario: created chat between the users
( or see steps above on how to create)
When user A is on another chat like Concierge, or has the electron app minimized:
-Send a message from User B to user A
-User A received Notification
Wait until Notification goes to the Notification center dock
-Open Notification center and click on this received notification
Verify the chat is marked as read
Verify that no errors appear in the JS console
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android.Native.mp4
Android: mWeb Chrome
AndroidWeb.mp4
iOS: Native
IOS.Native.test.mp4
iOS: mWeb Safari
IosMweb.mp4
MacOS: Chrome / Safari
macOsWeb.mp4
MacOS: Desktop
macOs.Electron.mp4