[HOLD for payment 2024-02-07] [$500] CRITICAL: Mark the chat as "read" immediately upon switching to the tab

[quinthar]

Current behaviour:

1. User is viewing a chat A and opens new tab.
2. User receives messages in chat A and comes back to the tab with chat A being open.
3. Currently the LHN shows as bold, but there is no Unread green line marker.
4. If the user switches to another chat and comes back to the chat A, now chat A behaves as showing a new unread chat. The LHN bold is gone and unread green marker is shown.

Expected behaviour:

In step 3, LHN shouldn't be bold, though it should have the green unread marker should be shown to symbolize the new messages received.

When the user switches away, the green unread marker should be removed.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012a54894c108d9212
• Upwork Job ID: 1745132109856296960
• Last Price Increase: 2024-01-10
• Automatic offers:
• abdulrahuman5196 | Reviewer | 28093546
• FitseTLT | Contributor | 28093547

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~01962a536f7d747895

[melvin-bot[bot]]

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

[melvin-bot[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[bot]]

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

[rayane-djouah]

Proposal

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

chats are not marked as "read" immediately when a user switches to the chat tab.

What is the root cause of that problem?

the read status of a chat is not updated in real-time as the user views it. Instead, the status update appears to be dependent on a state change triggered by navigating away from and back to the report

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

React Native's AppState can be used to determine if the app is in the foreground or background, and the useEffect hook can be employed to run side effects based on state changes. Here's a proposed solution:

Import AppState from React Native: This module helps to determine the current state of the app.

Add AppState Listener in useEffect: We will add a listener to AppState to detect when the app is brought to the foreground.

Update Read Status: When the app comes to the foreground and the relevant chat tab is active, we trigger the action to mark the chat as read.

import { AppState } from 'react-native';

useEffect(() => {
    const handleAppStateChange = (nextAppState) => {
        // Check if the app is coming to the foreground 
        if (nextAppState === 'active') {
                if (Visibility.isVisible() && Visibility.hasFocus() && ReportUtils.isUnread(report)) {
                    // mark the chat as read
                    Report.readNewestAction(report.reportID);
                }
        }
    };

    AppState.addEventListener('change', handleAppStateChange);

    return () => {
        AppState.removeEventListener('change', handleAppStateChange);
    };
}, [report.reportID]);

What alternative solutions did you explore? (Optional)

[tienifr]

Proposal

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

This is new feature, we want to mark chat as read when the chat gains focus.

What is the root cause of that problem?

This is new feature.

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

1. First we want to listen to the screen visibility in order to run things like reseting marker, mark as read, ... We can define a new useEffect like below (we can add the Visibility.hasFocus as well if we want to, because even if the app is visible but it doesn't have focus, we might not want to consider it as "read")

const useScreenVisibility = () => {
    const [isVisible, setIsVisible] = useState(Visibility.isVisible());

    useEffect(() => {
        const unsubscribeVisibilityListener = Visibility.onVisibilityChange(() => {
            setIsVisible(Visibility.isVisible());
        });

        return unsubscribeVisibilityListener;
    }, []);

    return isVisible;
}

This can be reused for various use case in the app

2. We want to read the report right after the user comes back to view the screen, so can add this to here

useEffect(() => {
        if (!isScreenVisible || scrollingVerticalOffset.current >= MSG_VISIBLE_THRESHOLD) {
            return;
        }
                    
        Report.readNewestAction(report.reportID);
        
        if (!currentUnreadMarker) {
            userActiveSince.current = DateUtils.getDBTime();
        }
    }, [report.reportID, isScreenVisible]);

Basically if the user comes back to the screen and the user is viewing the latest messages, then call Report.readNewestAction to read the report. We'll also set userActiveSince.current if the currentUnreadMarker is not present, to acknowledge that the user gets active in the page at that time, this will be used for step 3 below

3. In here, we should add isScreenVisible to the dependency list so that the marker will be recalculated if the user becomes active in the page again, this will make sure the green marker shows correctly (because the userActiveSince.current needs to be correct for this condition to work, we already set userActiveSince.current to the correct value in step 2)

We can optionally add

if (!isScreenVisible) {
    return;
}

To here because we don't need to recalculate the green marker if the screen is not visible.

Inside readNewestAction we can optionally early return if the report is already read.

Note:

• scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD (If the user is scrolled out of view, the user didn't see the new message yet so we can't consider it as read, this is the same condition as in here).

What alternative solutions did you explore? (Optional)

We can modify this existing logic to handle the above cases, and we need to make sure to add screen visibility to the dependency list here

[tienifr]

Proposal updated to clarify and add example code changes

[FitseTLT]

@quinthar I have some suggestions

1. Alice is viewing the Bob chat in her NewDot tab, but switches to a different tab to do something else. She gets a message from Bob, and switches back to the tab to view it.

Nope. Now unread marker will only display if you open the report while it has unread messages. If you are already on the report and you receive message while you 're on another tab and switch it back it will not display the marker.

2. Mark chats as read immediately upon becoming visible in the foreground.

Then what is the reason to display the marker if we remove it immediately, in any other social media apps (e.g. telegram) the marker will exist until you open other chat and I think the current behaviour is correct.

[bfitzexpensify]

@abdulrahuman5196 this is one marked as critical, so please prioritise reviewing the proposals here - thanks!

[quinthar]

Nope. Now unread marker will only display if you open the report while it has unread messages. If you are already on the report and you receive message while you 're on another tab and switch it back it will not display the marker.

Ah, good questions! To clarify, I'm not referring to the "unread marker" (eg, the horizontal line across the chat that designates new from old messages). I'm referring to it being marked as unread (bold) in the LHN. I'm sorry for being ambiguous in the original description, does that clarify things?

[FitseTLT]

Proposal

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

Mark the chat as "read" immediately upon switching to the tab

What is the root cause of that problem?

New Feature: We already have the logic here https://github.com/Expensify/App/blob/88d837c2e88c8c4c22a4c80f93b622d634e67290/src/pages/home/report/ReportActionsList.js#L207-L209

but it only marks the report read if it is Visibility.isVisible so once we return from other tab the useEffect will not be invoked as it only depends on report.lastVisibleActionCreated and report.reportID not visibility state

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

We should add visibility change listener and should Report.readNewestAction when Visibility.isVisible and the scroll is below the MSG_VISIBLE_THRESHOLD

CAUTION: we should not reset currentUnreadMarker as that will remove the new message green unread marker line which should not be removed until we open another report (we only want the report to be read i.e. remove the boldness in LHN)

So inside ReportActionsList we add this code

useEffect(() => {
    const unsubscribeVisibilityListener = Visibility.onVisibilityChange(() => {

        InteractionManager.runAfterInteractions(() => {
            if (!Visibility.isVisible() || scrollingVerticalOffset.current >= MSG_VISIBLE_THRESHOLD) {
                return;
            }
           if (!currentUnreadMarker) {
                    userActiveSince.current = DateUtils.getDBTime();

                    _.each(sortedReportActions, (reportAction, index) => {
                        if (!shouldDisplayNewMarker(reportAction, index)) {
                            return;
                        }

                        if (!currentUnreadMarker && currentUnreadMarker !== reportAction.reportActionID) {
                            cacheUnreadMarkers.set(report.reportID, reportAction.reportActionID);
                            setCurrentUnreadMarker(reportAction.reportActionID);
                        }
                    });
                }
            Report.readNewestAction(report.reportID);
        });
        
    })

    return unsubscribeVisibilityListener;
}, [report.reportID]);

or We can also use this code https://github.com/Expensify/App/blob/88d837c2e88c8c4c22a4c80f93b622d634e67290/src/pages/home/report/ReportActionsList.js#L207-L209 but we should add visibility state as a dependency here

https://github.com/Expensify/App/blob/88d837c2e88c8c4c22a4c80f93b622d634e67290/src/pages/home/report/ReportActionsList.js#L223-L223

What alternative solutions did you explore? (Optional)

[bfitzexpensify]

Bump @abdulrahuman5196 - please review these proposals, thank you!

[tienifr]

Proposal updated with minor change, based on the requirement update

[FitseTLT]

Proposal updated with minor change, based on the requirement update

       // put this if we want to clear the unread green marker as well after coming back, if not then ignore it

@abdulrahuman5196 please take a note that this is not a minor change but the main point of the new feature we are implementing. As only making the report unread and not resetting currentUnreadMarker is the critical and main point; and resetting currentUnreadMarker will make the unread marker useless as it will make it disappear as soon as the user opens the chat (as the visibility will be focused then) although it should be displayed until the user moves to other report or chat

[abdulrahuman5196]

Reviewing now

[abdulrahuman5196]

Current Experience:

https://github.com/Expensify/App/assets/46707890/23d3d4fd-0f9e-4bde-822c-5c3f63aa62da

1. User is viewing a chat A and opens new tab.
2. User receives messages in chat A and comes back to the tab with chat A being open.
3. Currently the LHN shows as bold, but there is no Unread green line marker.
4. If the user switches to another chat and comes back to the chat A, now chat A behaves as showing a new unread chat. Like the LHN bold is gone and unread green marker is shown.

I think there are 2 issues here regarding the LHN and unread marker here

I think in the step 3, we should change the experience like. LHN shouldn't be bold, but green marker should be shown to symbolize the new messages received (which is like a user going to new unread chat)

If we fix step 3, in step 4 the chat would automatically act as already read chat.

Kindly let me if we should change the unread marker as suggested or just have the LHN issue as mentioned in OP.

[melvin-bot[bot]]

@bfitzexpensify, @abdulrahuman5196 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[bfitzexpensify]

@abdulrahuman5196 in step 4, when the user switches away, that would remove the green unread marker?

[abdulrahuman5196]

@abdulrahuman5196 in step 4, when the user switches away, that would remove the green unread marker?

That should be the case IMO, but not happening right now. Atleast in this case if we switch to chat B and come back to chat A after step 3, we are seeing unread marker. @bfitzexpensify

[bfitzexpensify]

Cool, yep, I agree that that should be the case. I think the only other thing to mention is that if the user sends a message on chat A, then we should remove the unread marker, too.

[FitseTLT]

Updated proposal according to this expectation/comment

[bfitzexpensify]

Bump on https://github.com/Expensify/App/issues/33680#issuecomment-1874000153 @abdulrahuman5196. Do you agree with that?

[abdulrahuman5196]

Cool, yep, I agree that that should be the case. I think the only other thing to mention is that if the user sends a message on chat A, then we should remove the unread marker, too.

Yeah agree. On the user sending a message, I didn't check it, but it should behave the same way as how it behaves now when we send message in a unread chat.

[abdulrahuman5196]

@bfitzexpensify Could you kindly update OP clearly as well as per the discussion here https://github.com/Expensify/App/issues/33680#issuecomment-1873426860

And for contributors kindly update your proposals as per the updated requirements. And for the proposals already updated I will check it in my morning along with if any new.

[bfitzexpensify]

OP updated.

[bfitzexpensify]

@abdulrahuman5196 were you able to review the existing proposals?

[abdulrahuman5196]

Reviewing now

[tienifr]

And for contributors kindly update your proposals as per the updated requirements.

I'll provide an update based on the new requirements shortly.

[tienifr]

@abdulrahuman5196 Proposal updated based on new requirements.

[quinthar]

Hey all, what's the ETA? Who is selecting proposals, and when?