Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
MonilBhavsar opened this issue Jul 19, 2023 · 90 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff

Comments

@MonilBhavsar
Copy link
Contributor

MonilBhavsar commented Jul 19, 2023

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

  1. 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.

  1. 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:

UnreadMarker

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
@MonilBhavsar MonilBhavsar self-assigned this Jul 19, 2023
@MonilBhavsar MonilBhavsar added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 19, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 19, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jul 19, 2023

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

@MonilBhavsar MonilBhavsar added the Internal Requires API changes or must be handled by Expensify staff label Jul 19, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 19, 2023

Job added to Upwork: https://www.upwork.com/jobs/~01c3eb8d200e91874e

@melvin-bot
Copy link

melvin-bot bot commented Jul 19, 2023

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

@mananjadhav
Copy link
Collaborator

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

@MonilBhavsar
Copy link
Contributor Author

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

@gedu
Copy link
Contributor

gedu commented Jul 19, 2023

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

@MonilBhavsar
Copy link
Contributor Author

@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
Copy link
Collaborator

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

@melvin-bot melvin-bot bot added the Overdue label Jul 24, 2023
@trjExpensify
Copy link
Contributor

Did that help clarify @mananjadhav?

@melvin-bot melvin-bot bot removed the Overdue label Jul 24, 2023
@MonilBhavsar
Copy link
Contributor Author

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

@MonilBhavsar
Copy link
Contributor Author

Testing the PR's

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jul 28, 2023
@trjExpensify
Copy link
Contributor

How's the PR testing going?

@melvin-bot melvin-bot bot removed the Overdue label Jul 31, 2023
@MonilBhavsar
Copy link
Contributor Author

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

@melvin-bot melvin-bot bot added Reviewing Has a PR in review and removed Daily KSv2 labels Aug 1, 2023
@trjExpensify
Copy link
Contributor

Where was that confirmed, sorry?

@allroundexperts
Copy link
Contributor

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

@trjExpensify
Copy link
Contributor

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 melvin-bot bot added the Overdue label Dec 25, 2023
Copy link

melvin-bot bot commented Dec 26, 2023

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

Copy link

melvin-bot bot commented Dec 28, 2023

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

@MonilBhavsar
Copy link
Contributor Author

@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

@melvin-bot melvin-bot bot removed the Overdue label Dec 28, 2023
@shubham1206agra
Copy link
Contributor

@MonilBhavsar me too :)

@MonilBhavsar
Copy link
Contributor Author

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

@melvin-bot melvin-bot bot added the Overdue label Jan 1, 2024
Copy link

melvin-bot bot commented Jan 1, 2024

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

Copy link

melvin-bot bot commented Jan 1, 2024

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

@trjExpensify
Copy link
Contributor

@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).

@melvin-bot melvin-bot bot removed the Overdue label Jan 3, 2024
@trjExpensify
Copy link
Contributor

trjExpensify commented Jan 3, 2024

@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:

@melvin-bot melvin-bot bot added the Overdue label Jan 5, 2024
@trjExpensify
Copy link
Contributor

Let me know on the above!

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jan 5, 2024
@trjExpensify
Copy link
Contributor

Same, Melv.

@melvin-bot melvin-bot bot removed the Overdue label Jan 8, 2024
@MonilBhavsar
Copy link
Contributor Author

@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
Copy link
Contributor

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

@allroundexperts
Copy link
Contributor

Yep, requested. Feel free to close. Thanks!

@trjExpensify
Copy link
Contributor

Still need @shubham1206agra to accept :)

@shubham1206agra
Copy link
Contributor

Accepted

@trjExpensify
Copy link
Contributor

Paid!

@JmillsExpensify
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests

8 participants