Refactor code to improve unread marker logic by gedu · Pull Request #18637 · Expensify/App

gedu · 2023-05-09T13:24:41Z

Details

Migrates Unread Marker logic from ReportActionsView to ReportActionsList component
Improves the unread marker logic by using hooks

Fixed Issues

$ #23171
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

Have two users A and B

Active chat case

As a user A, open chat with user B and as user B open chat with user A in different tabs
Send a message as a user A to user B
Ensure no marker is displayed to user A and user B

Inactive chat case

As a user A, open chat with user B and as user B open chat other than user A in different tabs
Send a message as a user A to user B
Ensure no marker is displayed to user A but user B sees new marker above new messages

Mark as unread

As a user B, mark any message as unread from user A
Verify new marker appears above it
Go to any other chat and come back to chat with user A
Verify marker still appears
Go to any other chat and come back to chat with user A
Verify marker is not visible now

Deleting a message

As a user A, open chat with user B and as user B open chat other than user A in different tabs
Send two message as a user A to user B
Ensure no marker is displayed to user A but user B sees new marker above new messages
As a user A, delete message
As user B, ensure new marker line is there and message is deleted
As a user A, delete another message
As user B, ensure new marker line and message are deleted

Scrolled up

As a user A, open chat with user B and as user B open chat other than user A in different tabs
Send a message as a user A to user B
Ensure no marker is displayed to user A but user B sees new marker above new messages
Scroll up and ensure you see a floating button
Click on it and ensure you're taken down

Verify that no errors appear in the JS console

Offline tests

None, same as online(As message sending and receiving is halt when offline)

QA Steps

Please see Tests

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

unreadMarker_chrome.mp4

unreadMarker_safari.mp4

Mobile Web - Chrome

Mobile Web - Safari

unreadMarker_iosSafari.mp4

Desktop

unreadMarker_desktop.mp4

iOS

unreadMarker_ios.mp4

Android

unreadMarker_android.mp4

# Conflicts: # src/libs/actions/Report.js # src/pages/home/report/ReportActionsList.js

…/App into monil-fixNewMarkerLine # Conflicts: # src/libs/actions/Report.js

MonilBhavsar · 2023-05-17T07:43:50Z

I have updated base branch so we stay up to date with main

…il-fixNewMarkerLine # Conflicts: # src/libs/actions/Report.js

MonilBhavsar · 2023-08-17T06:43:33Z

Thanks! I can verify on my side

MonilBhavsar · 2023-08-17T18:37:20Z

Confirmed! works for me too
https://drive.google.com/file/d/1HnXkqPt5VKGt0UB2A6dOPnd3UByjDPIf/view?usp=drive_link

MonilBhavsar · 2023-08-17T18:37:33Z

Let's ship it!! 🚀

OSBotify · 2023-08-17T18:42:36Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

Pujan92 · 2023-08-18T06:24:00Z

@gedu I think we should unsubscribe the typing event in the ReportActionsView on unmount instead of keeping it open or restricting multiple subscriptions for the same channel.
https://github.com/Expensify/App/pull/18637/files#diff-3d3d60df30d1761a83ff8413b06ecef63fcd0084053cd70d5686098e71d87362L184

Need to create cleanup function here which calls Report.unsubscribeFromReportChannel(reportID);

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

Lines 86 to 89 in c435734

    
           useEffect(() => { 
        
               openReportIfNecessary(); 
        
               // eslint-disable-next-line react-hooks/exhaustive-deps 
        
           }, []);

gedu · 2023-08-21T11:58:23Z

@gedu I think we should unsubscribe the typing event in the ReportActionsView on unmount instead of keeping it open or restricting multiple subscriptions for the same channel. https://github.com/Expensify/App/pull/18637/files#diff-3d3d60df30d1761a83ff8413b06ecef63fcd0084053cd70d5686098e71d87362L184

Need to create cleanup function here which calls Report.unsubscribeFromReportChannel(reportID);

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

Lines 86 to 89 in c435734

useEffect(() => {

openReportIfNecessary();

// eslint-disable-next-line react-hooks/exhaustive-deps

}, []);

Hey, the next useEffect runs the unsubscribeFromReportChannel.
All those flows, weren't part of unread marker refactor, come from so many merges

hayata-suenaga · 2023-08-21T19:32:32Z

@gedu could you check if this issue was caused by this PR?

OSBotify · 2023-08-21T22:25:40Z

🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 1.3.56-0 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

dukenv0307 · 2023-08-22T10:30:18Z

src/pages/home/report/ReportActionsList.js

+                if (!messageManuallyMarked.read) {
+                    shouldDisplayNewMarker = shouldDisplayNewMarker && reportAction.actorAccountID !== Report.getCurrentUserAccountID();
+                }
+                const canDisplayMarker = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < userActiveSince.current : true;


@MonilBhavsar Actually, canDisplayMarker is wrong whenever we create a new action and mask as unread this action. That makes currentUnreadMarker isn't updated correctly. The reason is the newest action has reportAction.created is always bigger userActiveSince.current.

Screencast.from.22-08-2023.17.26.27.webm

roryabraham · 2023-08-23T16:23:54Z

Evaluating the deploy blockers related to this PR, it seems there are three:

~~Web - New line marker on Task does not disappear when sending or receiving a message #25776~~ Edit: This one was expected behavior
Chat - New marker displayed briefly when chat is opened #25673
~~[HOLD for payment 2023-09-04] Greenline line doesn't appear after the mark of a message as unread (iOS) #25737~~ Edit: This one was unrelated

OSBotify · 2023-08-24T03:08:41Z

🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.56-24 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

mollfpr · 2023-09-07T04:50:37Z

src/pages/home/report/ReportActionsView.js

-
-        // 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.
-        const unsubscribe = Report.subscribeToNewActionEvent(reportID, (isFromCurrentUser, newActionID) => {


Hi, coming from #26432

The issue is a regression from this removed, where the current user doesn't scroll to the bottom after sending a message.

I have several questions regarding these changes:

Are the current users not scrolled to the bottom after sending a message is expected?

If not, can we reintegrate the listener Report.subscribeToNewActionEvent to scroll the current user when sending a message? Or there's a performance consideration to use that listener.

cc @gedu @MonilBhavsar @allroundexperts

Are the above results expected?

Sorry, what result?

Ahh, sorry for not making it clear.

Sorry, what result?

The current user doesn't scroll to the bottom after sending a message when they're not at the bottom of the chat.

That does seem like issue. We should definitely scroll user to the bottom

@MonilBhavsar Is there any drawback or performance issue if we use Report.subscribeToNewActionEvent to get the current report action?

I don't think it will have an impact on performance, it will be mainly just to know when a new message is sent. Because this newActionEvent only listens to comments sent, and not received from others.

alitoshmatov · 2025-01-20T14:53:23Z

src/pages/home/report/ReportActionsList.js

+
+                        return null;
+                    }}
+                    keyboardShouldPersistTaps="handled"


keyboardShouldPersistTaps is set to "handled" is causing keyboard not closing when tapped outside. #53196

hoangzinh · 2025-02-04T10:54:42Z

src/pages/home/report/ReportActionsList.js

+        onScroll(event);
+    };
+
+    const scrollToBottomAndMarkReportAsRead = () => {


Hi team, coming from #54944. We should use useCallback here to avoid unnecessary rerenders of FloatingMessageCounter

allgandalf · 2025-02-19T06:16:46Z

src/libs/actions/Report.js

 * @param {String} reportID
 */
 function readNewestAction(reportID) {
+    const lastReadTime = DateUtils.getDBTime();


We should had used getDBTimeWithSkew here, this caused #54679

MonilBhavsar and others added 17 commits May 8, 2023 01:15

Pass client's lastReadTime to server

1c98457

Listen for new messages

5189784

Mark comments as read

8895b61

Add and update logic to set new marker

7f13144

Update more logic and fix active chat case

87ef4b3

Pass correct report actions

3ce360e

if reportActions exist, use onyx local data to render the messages

3931f71

Merge branch 'main' into monil-fixNewMarkerLine

05cc1be

Don't set lastReadTime optimistically

f39a674

handling the unread line on report list

9a7ef68

using the reportsActions from onyx

e022a7a

Merge branch 'main' into monil-fixNewMarkerLine

9132bc2

# Conflicts: # src/libs/actions/Report.js # src/pages/home/report/ReportActionsList.js

style updated

a79ac80

Some more updates to logic to fix 2 1:1 and switch cases

15377c7

Attempt to fix unread marker logic

279e1a2

Merge branch 'monil-fixNewMarkerLine' of https://github.com/Expensify…

bc15e03

…/App into monil-fixNewMarkerLine # Conflicts: # src/libs/actions/Report.js

Clean up, and handling unread cases

0843bae

MonilBhavsar self-assigned this May 17, 2023

MonilBhavsar changed the base branch from monil-fixNewMarkerLine to main May 17, 2023 07:38

Merge branch 'main' into monil-fixNewMarkerLine

e8712d5

MonilBhavsar and others added 3 commits May 19, 2023 19:16

Revert to another approach

1d899ca

Merge branch 'main' into monil-fixNewMarkerLine

52cf638

Updated marking report as read

41602a4

MonilBhavsar force-pushed the monil-fixNewMarkerLine branch from 41602a4 to 52cf638 Compare May 19, 2023 15:50

gedu and others added 5 commits May 19, 2023 18:04

Merge branch 'monil-fixNewMarkerLine' of github.com:gedu/App into mon…

eea32eb

…il-fixNewMarkerLine # Conflicts: # src/libs/actions/Report.js

Fix openReport call

1cf3101

Add hook to track active chat

009063d

Donot render marker when active on chat

347538c

Donot render marker line for sender

9c286f7

allroundexperts approved these changes Aug 17, 2023

View reviewed changes

MonilBhavsar merged commit 28be10b into Expensify:main Aug 17, 2023

kavimuru mentioned this pull request Aug 18, 2023

" user is typing ... " doesn't appear after opening and closing the RHP #25486

Closed

6 tasks

allroundexperts mentioned this pull request Aug 19, 2023

Mark as unread appears on message that are not new #25533

Closed

6 tasks

MonilBhavsar mentioned this pull request Aug 21, 2023

[Tracking] Redesign Unread Markers #15212

Closed

20 tasks

hayata-suenaga mentioned this pull request Aug 21, 2023

Dev: Delay in removing 'New' marker After deleting newly sent message #25606

Closed

6 tasks

OSBotify mentioned this pull request Aug 21, 2023

Deploy Checklist: New Expensify 2023-08-21 #25625

Closed

dukenv0307 reviewed Aug 22, 2023

View reviewed changes

lanitochka17 mentioned this pull request Aug 22, 2023

Chat - New marker displayed briefly when chat is opened #25673

Closed

6 tasks

marcaaron mentioned this pull request Aug 23, 2023

[HOLD for payment 2023-09-04] Greenline line doesn't appear after the mark of a message as unread (iOS) #25737

Closed

6 tasks

This was referenced Aug 23, 2023

[HOLD] Revert "Refactor code to improve unread marker logic" #25785

Closed

Delay in removing 'New' marker After deleting newly sent message #25771

Merged

MitchExpensify mentioned this pull request Aug 23, 2023

[HOLD] Archived announce and admin channels dropped to bottom of the list with unread indicator #20148

Closed

6 tasks

parasharrajat mentioned this pull request Aug 25, 2023

[HOLD for payment 2023-11-01] [HOLD on PR #25935] [$1000] Two line markers appear for a second #24648

Closed

6 tasks

rayane-d mentioned this pull request Aug 31, 2023

[HOLD for payment 2023-09-29] [$500] Report - Conversation history does not scroll down after sending a message and IOU #26432

Closed

6 tasks

mollfpr reviewed Sep 7, 2023

View reviewed changes

s77rt mentioned this pull request Nov 27, 2023

[HOLD for #20373][$1000] Web - Nothing happens when clicking 'mark as unread' for first message in Task page #22059

Closed

6 tasks

alitoshmatov reviewed Jan 20, 2025

View reviewed changes

hoangzinh reviewed Feb 4, 2025

View reviewed changes

allgandalf reviewed Feb 19, 2025

View reviewed changes

Conversation

gedu commented May 9, 2023 • edited by MonilBhavsar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Active chat case

Inactive chat case

Mark as unread

Deleting a message

Scrolled up

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

MonilBhavsar commented May 17, 2023

Uh oh!

MonilBhavsar commented Aug 17, 2023

Uh oh!

MonilBhavsar commented Aug 17, 2023

Uh oh!

MonilBhavsar commented Aug 17, 2023

Uh oh!

OSBotify commented Aug 17, 2023

Uh oh!

Pujan92 commented Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gedu commented Aug 21, 2023

Uh oh!

hayata-suenaga commented Aug 21, 2023

Uh oh!

OSBotify commented Aug 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roryabraham commented Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OSBotify commented Aug 24, 2023

Uh oh!

mollfpr Sep 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

gedu commented May 9, 2023 •

edited by MonilBhavsar

Loading

Pujan92 commented Aug 18, 2023 •

edited

Loading

roryabraham commented Aug 23, 2023 •

edited

Loading

mollfpr Sep 7, 2023 •

edited

Loading