[HOLD for payment 2023-11-29] [Redesign Unread Markers] Fix unread markers and migrate logic to ReportActionsList component

[MonilBhavsar]

Tracking issue #15212

As mentioned in the doc, implement client side changes to update unread marker https://docs.google.com/document/d/144r92kq3lvJlT2sg9wQGzFHdq5PPyai4eCs5jsBWrZw/edit

1. Move the logic to handle unread marker from ReportActionsView to ReportActionsList component
2. While calling openReport command, use optimistic data if possible

---

Root cause:

1. lastReadTime being updated by the server and optimistically, too early.

Currently, when fetching report through OpenReport API command, the client optimistically marks the message as read or update the lastReadTime here, and the server also updates the lastReadTime and returns updated lastReadTime here. This makes lastReadTime greater than created time, and since reportAction.created < report.lastReadTime, we fail to render unread marker

2. Taking into consideration incorrect reportActions when calculating whether to display unread marker

Currently, when a user navigates to chat from LHN, or directly through URL, or from notification, we render ReportScreen component, and then render ReportActionsView component by passing reportActions as props here.
We calculate whether or where we should display an unread marker in the chat, when the chat is loaded initially in the constructor of the ReportActionsView component here. This calculation is based on the reportActions(messages) associated with that chat.
When switching chats, we observed that there is a slight delay in getting reportActions of new chat or the chat one is switching to, and it seems like the first couple of renders happen with the reportActions one is switching from. As a result, we incorrectly calculate unread marker based on the reportActions of the previous chat.

3. Some Performance concerns

We calculate where we should display unread marker in the ReportUtils.getNewMarkerReportActionID function here.
This function is called multiple times - one from the constructor of the ReportActionsView component and multiple times from the componentDidUpdate equivalent method in the ReportActionsView component. The function has complexity of O(n), and it is being performed during component re-rendering.

Proposed solution:

Right now, the whole logic to render new markers is in the ReportActionsView component. We will remove that logic from the ReportActionsView component and move it to the ReportActionsList
component, where we render reportActions(messages).

Removing logic from ReportActionsView

From the ReportActionsView we are removing the newMarkerReportActionID from the state.
Also we are removing some functions to handle and update that marker, ReportUtils.isUnread function here and ReportUtils.getNewMarkerReportActionID function here. Additionally, any logic that uses them will be removed too.

Handling unread marker

Now that we’re using hooks, we need to have an indicator to check if a user is active on a chat or not. This is because if the user is active on chat and receives messages in real time, then we don’t want to display unread marker.
To accomplish this, we will utilize a ref userActiveSince that stores the timestamp when users enter the chat.

useEffect(() => {
   userActiveSince.current = DateUtils.getDBTime();
}, []);

The logic to know if and where we should display an unread marker will be calculated into the renderItem function in the ReportActionsList component here, while the items in the list are being created.

We display an unread marker if a message was created(reportAction.created) after the user last read the chat(report.lastReadTime).The basic function to check if we should display an unread message will be as below:

function isUnreadMessage(message, lastRead) {
    return Boolean(message.created && lastRead < message.created);
}

To determine where we should display an unread marker, we check if the current message is read and the next message is unread. If such is the case, we will display an unread marker there.
The logic to determine the position of unread marker is as below:

const nextMessage = sortedReportActions[index + 1];

const isCurrentMessageUnread = isUnreadMessage(reportAction, report.lastReadTime);

const shouldDisplayNewMarker = isCurrentMessageUnread && !isUnreadMessage(nextMessage, report.lastReadTime);

To keep track of when a green marker is being displayed, we will store the information in a ref called currentUnreadMarker that holds reportActionID above which we should display unread marker. If the marker is already rendered and we have information about the marker, we will reuse it.

if (!currentUnreadMarker.reportActionIDcurrent) {
    // ...

	if (shouldDisplayNewMarker && canDisplayMarker) {
   		currentUnreadMarker.reportActionIDcurrent = reportAction.reportActionID;
     }
} else {
     shouldDisplayNewMarker = reportAction.reportActionID === currentUnreadMarker.reportActionIDcurrent;
}

Marking messages as read explicitly

Since the server will no longer update the report’s lastReadTime data in OpenReport API command, we need to mark the report as read from the client's side in all cases. After receiving reportActions, we will inform the server to update that lastReadTime by calling Report.readNewestAction. This new function will invoke the ReadNewestAction API command, passing the reportID and the lastReadTime to be set.

There are some cases to take into consideration:
We first check if there is an unread message using the ReportUtils.isUnread
We would need to take into consideration the scrolling position. If the user is scrolled up, that means they have’t read the message and we won’t mark it as read.

useEffect(() => {
    if (ReportUtils.isUnread(report)) {
        if (scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD) {
            Report.readNewestAction(report.reportID);
    	   } else { ...

Marking message as unread manually

The user has the ability to mark any message as unread, which is achieved by modifying the lastReadTime in markCommentAsUnread function here. For manually marking message as unread, we subtract 1 millisecond from the lastReadTime.
To determine if a user has manually marked a message as unread, we check if the lastReadTime is less than the current time using DateUtils.getDBTime() and if there is an unread message using ReportUtils.isUnread. If this condition is true, we clear the currentUnreadMarker to ensure the unread marker can be re-calculated, and update the messageManuallyMarkedAsUnread flag.

Also, we don’t need to display an unread marker on the sender’s side when they are sending a new message, unless they manually marked the message as unread.
We’ll have the following check to ensure this behavior:

if (!messageManuallyMarkedAsUnread) {
   shouldDisplayNewMarker = shouldDisplayNewMarker && reportAction.actorAccountID !== Report.getCurrentUserAccountID();
}

Scrolling behaviors

Currently logic to track scrolling is handled in the ReportActionsView component here.
We will be transferring the scrolling tracking functionality from ReportActionsView to ReportActionsList and reuse it.

Additionally, we display a floating element when the user is scrolled up and there are new latest messages to read. It is rendered using the FloatingMessageCounter in ReportActionsView component. We’ll also move it to ReportActionsList component and its visibility(whether we should display floating element or not) will be managed using a useState hook, which will be calculated based on the user's scrolling behavior.

We also need logic to determine if we should display an unread marker or floating element considering the user's position in the chat. If the user has scrolled a little bit and the latest messages are visible then we display an unread marker, else if the user has scrolled much and messages are not visible then we display a floating element to allow the user to jump to the latest messages.
A constant canDisplayMarker will handle this behavior as described below

const canDisplayMarker = scrollingVerticalOffset.current < MSG_VISIBLE_THRESHOLD ? reportAction.created < userActiveSince.current : true;

Fixing delayed rendering issue

When switching between Reports, the ReportScreen component passes old reportActions from Onyx, during initial renders which can lead to incorrect calculation of the unread marker. To address this issue and avoid any unusual loading delays when fetching the reportActions. We will update Report.openReport to check if we already have the reportActions data, we use an empty object in the optimisticData to prevent unnecessary re-renders and ensure that the correct data is passed. However, if the reportActions data is not available, we handle it as normal and fetch the data as needed.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c3eb8d200e91874e
• Upwork Job ID: 1681647663368384512
• Last Price Increase: 2023-07-19

[melvin-bot]

Triggered auto assignment to @trjExpensify (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/~01c3eb8d200e91874e

[melvin-bot]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @mananjadhav (Internal)

[mananjadhav]

@MonilBhavsar Is there a way that the access for the design doc can be shared?

[MonilBhavsar]

Happy to get required content out of the doc. Just a minute

[gedu]

Hey I'm Edu from Callstack - expert contributor group, I wanna take care of this one

[MonilBhavsar]

@mananjadhav I have updated OP with problem/solution format and with all the details required for this issue. Please let me know if that sounds good

[mananjadhav]

Thanks for the details. I'll go through it.

[trjExpensify]

Did that help clarify @mananjadhav?

[MonilBhavsar]

@gedu is out for the week, so I'll be working on this in parallel

[MonilBhavsar]

Testing the PR's

[trjExpensify]

How's the PR testing going?

[MonilBhavsar]

Going good!
We need to fix scrolling behavior. @gedu is back from ooo and will be working on this from today.

[trjExpensify]

Where was that confirmed, sorry?

[allroundexperts]

@trjExpensify I think @shubham1206agra mean't that this was a redesign / refactor and the checklist is not applicable here.

[trjExpensify]

Hm gotcha, so @MonilBhavsar I take it with the project you had a bunch of manual tests to handover to Applause to update/add/remove based on the changes, and you'll do that as part of the parent issue of the project?

In terms of payment then guys, can we confirm how many regressions came from this issue to tally up what's due?

P.S as a heads up, tomorrow is my last day for the holidays and I'll be back on Jan 3rd. So let me know tomorrow and I can settle up ahead of then. :)

[melvin-bot]

@gedu, @trjExpensify, @allroundexperts, @MonilBhavsar, @shubham1206agra Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

@gedu, @trjExpensify, @allroundexperts, @MonilBhavsar, @shubham1206agra 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[MonilBhavsar]

@trjExpensify yes, we add tests for all cases together https://docs.google.com/document/d/1j1UKjl_zVmS_EVVtpeH2ugrlt6INuWKlnp3X_o_WXRY/edit#bookmark=id.v1lsuitewwbd

There were couple of small regressions from this mega refactor PR https://github.com/Expensify/App/pull/18637/files
Given it refactored base ReportActionsList component and we tried our best to avoid them, I think we should pay full amount to @allroundexperts for reviewing it

[shubham1206agra]

@MonilBhavsar me too :)

[MonilBhavsar]

Oh right 👍 @shubham1206agra finished the review and checklist and @allroundexperts reviewed initially.
@trjExpensify do you think we should split the payment here?

[melvin-bot]

@gedu, @trjExpensify, @allroundexperts, @MonilBhavsar, @shubham1206agra Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@gedu, @trjExpensify, @allroundexperts, @MonilBhavsar, @shubham1206agra Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[trjExpensify]

@trjExpensify yes, we add tests for all cases together https://docs.google.com/document/d/1j1UKjl_zVmS_EVVtpeH2ugrlt6INuWKlnp3X_o_WXRY/edit#bookmark=id.v1lsuitewwbd

Has this been handed off to Applause to add to TestRail? (Can't access the doc btw).

[trjExpensify]

@trjExpensify do you think we should split the payment here?

Yeah, makes sense. I'm assuming this was at the old $1k bounty level, so this is $500 each? Once confirmed, I'll send the offers.

Payment summary as follows:

• $500 to @allroundexperts
• $500 to @shubham1206agra

[trjExpensify]

Let me know on the above!

[trjExpensify]

Same, Melv.

[MonilBhavsar]

@trjExpensify do you think we should split the payment here?

Yeah, makes sense. I'm assuming this was at the old $1k bounty level, so this is $500 each? Once confirmed, I'll send the offers.

Sounds good and fair to me 👍

[trjExpensify]

Cool, offer has been sent to @shubham1206agra. @allroundexperts, you can request, right?

[allroundexperts]

Yep, requested. Feel free to close. Thanks!

[trjExpensify]

Still need @shubham1206agra to accept :)

[shubham1206agra]

Accepted

[trjExpensify]

Paid!

[JmillsExpensify]

$500 payment approved for @allroundexperts based on this comment.