Add a "last unread" indicator for reports

[Christinadobrzyn]

If you haven’t already, check out our contributing guidelines for onboarding!

---

Platform - version: web, ios, android, desktop - version number Version 1.0.1-444 (1.0.1-444)

Action Performed:

• Create a group chat in e.cash desktop, web, ios, android
• Send a message to the group chat
• Go offline
• Come back to the chat and look for the last message you read
• Create a marker for responses to a thread that happened since the last time you read the thread.

Expected Result:

• User should see a new message marker in all chat threads - even non-group chats
• The new message marker needs to have a height of 0 so it can fit nicely between the chats

Actual Result:

• At this time, the only method to see the last read message is looking at the time stamp.

• Currently, the only method to determine the last message you've read is by the date of the messages

And some guidelines for a solution in the code:

UnreadActionIndicator

This simple component will render an <Animated.View /> as a horizontally-oriented flexbox containing:

• A <Text /> component containing the word “new”
• An empty <View/> with a width but no height, and a green borderBottom (AKA a horizontal rule in React Native).

It should have a very simple animation to fade out when being hidden.

ReportActionsView

We need to make sure that the last unread comment on the report remains unread until the user leaves the report and comes back. We’ll start by writing a new function called recordLastUnreadAction. It will grab the lastUnreadAction from the report. If none is found, we’ll use the highest visible sequence number, just like recordMaxAction does now.

Then we’ll rename recordMaxAction to recordMaxVisibleAction, and replace all uses of recordMaxAction with recordLastUnreadAction, except for this one, which is triggered when an inactive report becomes active again. In that case, we’ll call scrollToAction with the sequence number of the last unread action so that we display the last unread comment in the report, then call recordMaxVisibleAction.

We’ll then create a new function called insertUnreadIndicator, which will inject an object with {type: ‘unreadIndicator’} into the sortedReportAction array in the position immediately following the lastUnreadAction.

Then we’ll update renderItem to check the type of the item, and if it is unreadIndicator, it will render an <UnreadActionIndicator /> instead of a <ReportActionItem />. In markAllRead, we’ll trigger the hide animation on the <UnreadActionIndicator />, then when it’s completed remove it from the sortedReportActions array.

[Christinadobrzyn]

External Upwork post - https://www.upwork.com/jobs/~0168a5128a533e2505
Internal Upwork job post - https://www.upwork.com/ab/applicants/1362207152733396992/job-details

[hepiska]

here how i would solve this bugs

a. backend heavy approach

1. when user is leaving the chat room it will send request to backend and mark user as leave the room or going offline
2. create table or collection to save unread message id for each user table need have userId and
3. every new message will check the recipient of the message and if the recipient if offline in will add the message id to the unread message table
4. when user open the app it will get unreadmessage for the user, normalize the data from array to hash map using normalzr and then save it in redux
5. when we open the chat list it will get the using the chat id to check if the chat included in unread hasmap (ex state.unReadMessage[chatid]) if chat id available than it will mark as unread
6. when user mark message as read will trigger action removing redux state and send delete request to be

b. frontend heavy

1. when the user leave chat room or going offline will save data goingOffline on persistence storage
2. when the user online and open the chat room in the chat bubble component will check if the chat timeStamp > goingOffline if true it will trigger action to save chat id on unreadmessage redux state as hash map with chatId as key and empty obj as value and send the data to be so it will effect on any device the user use.
3. in every chat bubble it will listen unreadmessage redux state if the chat id is it that it will mark as unread
4. if user mark chat as read it will add the filed on the unreadmessage[chatId].isRead = true so it will change the bubble status
5. when the user go offline or leave the room it will replace goingOffline value and remove unreadmessage with isRead = true

upwork proposal https://www.upwork.com/ab/proposals/1363015601945636865

[lucas-neuhaus-dev]

@Christinadobrzyn

At this time, the only method to see the last read message is looking at the time stamp.
Currently, the only method to determine the last message you've read is by the date of the messages

What about the unreadActionCount returned by ONYXKEYS.COLLECTION.REPORT? Can't we use that to determine the last unread message?
I'm not sure of how it works, but it is being used already to change the read / unread style on the Sidebar reports.
It's probably unreliable to have two separate methods to track unread messages.

[marcaaron]

That makes sense to me Lucas, but maybe I am missing something about the existing proposal.

cc @roryabraham since you have proposed the original solution

[roryabraham]

@lucas-neuhaus-dev Yep, the unreadActionCount should work 👍

[lucas-neuhaus-dev]

@roryabraham
I have quite a few questions, this description on the ReportActionsView is not making that much sense to me.

We need to make sure that the last unread comment on the report remains unread until the user leaves the report and comes back. We’ll start by writing a new function called recordLastUnreadAction. It will grab the lastUnreadAction from the report. If none is found, we’ll use the highest visible sequence number, just like recordMaxAction does now.

This new function recordLastUnreadAction...
From my understanding, we want this function basically to call updateLastReadActionID() with (the reportID and) either the sequence number of the last unread message or the maxVisibleSequenceNumber, right? Depending on whether there are unread messages or not. Hopefully got at least this one right.

Then we’ll rename recordMaxAction to recordMaxVisibleAction, and replace all uses of recordMaxAction with recordLastUnreadAction, except for this one, which is triggered when an inactive report becomes active again.

The exception mentioned happens only when _.size(prevProps.reportActions) !== _.size(this.props.reportActions), in other words, when a new message is sent. That would mean that updateLastReadActionID() gets called with maxVisibleSequenceNumber just when a new message is sent.

In that case, we’ll call scrollToAction with the sequence number of the last unread action so that we display the last unread comment in the report, then call recordMaxVisibleAction

This scrollToAction, what exactly is it? Is it a new function that has to be created, and gets called when an inactive report becomes active? And scrolls the report view to the point of the last unread action?

I spent some reasonable time reading the explanation and testing things out on the code, but can't seem to figure this stuff out.

One more thing:

We’ll then create a new function called insertUnreadIndicator, which will inject an object with {type: ‘unreadIndicator’} into the sortedReportAction array in the position immediately following the lastUnreadAction.

What about, instead of inserting an object in the array, we don't leave it as it is, and add some conditional logic on the renderItem() function, based on the index? I did some tests and it seems to work just fine. We check if the index corresponds to the last action.
If we add a new object in the array, we need to make sure of a bunch of things before sending it to the <InvertedFlatList />.

---

Sorry for the long list of questions. I'm not sure of how strict I have to follow the instructions on the PR description, so I wanted to make sure I understood it well before anything really.

[roryabraham]

@lucas-neuhaus-dev sorry for the confusion. This implementation plan was copy/pasted from a document which outlines some features that do not yet exist, including the ability to manually mark conversations as unread. That was an oversight on my part, so I'm sorry to have wasted some of your time. I should have been more careful in copy/pasting this out of its original context.

Allow me to clarify...

This new function recordLastUnreadAction...
From my understanding, we want this function basically to call updateLastReadActionID() with (the reportID and) either the sequence number of the last unread message or the maxVisibleSequenceNumber, right?

Actually, you shouldn't need to create this function or call updateLastReadActionID, because that would change a value in the API that we don't need changed. I think this can be safely ignored since you're using the unreadActionCount to determine last unread action.

This scrollToAction, what exactly is it?

Your understanding of this is correct. This should be a new function similar to scrollToListBottom, or you can just use FlatList's scrollToIndex function directly. The goal is that when a user opens a report with unread chats, the FlatList will scroll to the oldest unread chat (where the new indicator will be).

What about, instead of inserting an object in the array, we don't leave it as it is, and add some conditional logic on the renderItem() function, based on the index? I did some tests and it seems to work just fine. We check if the index corresponds to the last action.

If you think it would be more complicated to inject an object into the sortedReportAction array, that's fine with me. But the reason I had opted for that approach (avoiding the need for conditional logic in the renderItem function) is because renderItem is pretty performance-critical, and I thought it would be better to avoid adding conditional logic in there if possible. I'll leave that up to you and/or @marcaaron

---

Again, sorry for the unnecessary confusion I caused here @lucas-neuhaus-dev. There is a lot of context missing in that explanation, so it makes no sense on its own.

[lucas-neuhaus-dev]

No problem! Thanks for the explanation.
I should be good to go now (test some stuff out and create a good proposal).

If you think it would be more complicated to inject an object into the sortedReportAction array, that's fine with me. But the reason I had opted for that approach (avoiding the need for conditional logic in the renderItem function) is because renderItem is pretty performance-critical, and I thought it would be better to avoid adding conditional logic in there if possible. I'll leave that up to you and/or @marcaaron

I see, that's a good point.
I'm going to take a closer look at this and verify which solution I think fits best.

[shawnborton]

Just a heads up that I updated the mockup in the original comment of this issue - not a huge change, just moves the "New" over to the right.

[lucas-neuhaus-dev]

@shawnborton since you mentioned that, let me ask:

When an unread message is within a group of recent messages (when it's not the first message with an avatar on the left), like on the image below:

Do you want it to be like the image, or do you prefer tagging that message as a new group? I mean, show the name, avatar and time again after the NEW view.

[shawnborton]

I think the general rule of thumb is that the unread indicator would immediately precede the last unread individual message. So basically what you have mocked up is perfect.

[lucas-neuhaus-dev]

@shawnborton just thought of another question 😅 haha

For large screens, don't you think that it will look weird? The NEW on the far right of the screen?
You wouldn't have a mock of that for me, would you?

[shawnborton]

Here you go:

I think it's not too bad. What do you think?

[lucas-neuhaus-dev]

Hmmmm I think your mock is a little bias.

It is a 1180px width screen, instead of other very normal 1920px, so it is more of a medium screen.
And you have a lot of text both at the top and at the bottom, so the text meets the end of the screen, and the NEW does not get so "alone" at one border.

I would encourage you to try a 1920px screen with very short messages. I believe that if we brought the NEW maybe to the center on large screens, it could be better.

[shawnborton]

I don't think we should be optimizing for screens that are quite that large, I would consider something in the 1300-1500px range to be normal.

With that being said, it would look like this:

I don't think it looks too bad, so I would be fine moving forward with implementing this as we have it mocked up.

The alternative you mentioned in centering it feels a bit strange and cramped when wedged between two individual messages, so I think we should avoid this:

[lucas-neuhaus-dev]

Yeah, it really doesn't look great at the center, I had a different image in my head.

I'm on it, thanks!

[shawnborton]

Awesome, thanks for your feedback though and working through the mockups with me!

[lucas-neuhaus-dev]

@roryabraham @marcaaron

Obs.: I have one unrelated question at the end, I would appreciate it if someone could kill my curiosity on that, not necessary tho

Proposal

1. Create a new component called UnreadActionIndicator, that has height = 0. It receives an animatedOpacity prop;
2. In ReportActionsView, get the reports object from the Onyx store;
3. Use it to create a function getUnreadActionCountAndAnimateIndicator (I might want to re-think this name). This function gets the unreadActionCount of the report and returns it. If it is greater than 0, it also starts a setTimeout of three seconds, which fades out the unread indicator at the end of it. It does a couple of verifications to make sure the indicator just shows up when intended;
4. I did opt for not inserting anything in the sortedReportActions array as suggested in the description. From my tests it brings too many problems within the component children. Instead, I opt to check for the item's index and compare it with the unreadActionCount on the renderItem() function. If we think about it, we are just changing one conditional render with a different conditional render. For this reason, I don't think we should worry about performance;
5. For now, I'm kind of stuck with the scrollToAction function, I found a lot of problems and still haven't found a proper solution to it (mainly because we still don't have the FlatList ref at componentDidMount time, I'm doing some studying to try to figure something out).

I know it is better to show what I have if I'm having problems with something, that's why I'm writing this proposal without the scrollToAction solution. Maybe someone has a good idea of how to solve it with the PR in hands.

Unrelated question
On ReportScreen.js, why do we render other reports aside from the active one? I can't seem to figure out the benefit from that. I know that the other reports get a display of none, but it is still a lot of processing going on, which can easily be seen if we write a console.log on the renderItem function on ReportActionsView (and you have large pinned and unread reports).

[marcaaron]

create a function getUnreadActionCountAndAnimateIndicator (I might want to re-think this name). This function gets the unreadActionCount of the report and returns it

Do we need to "get" the unreadActionCount I think it should just be attached to the report object?

I did opt for not inserting anything in the sortedReportActions array as suggested in the description. From my tests it brings too many problems within the component children. Instead, I opt to check for the item's index and compare it with the unreadActionCount on the renderItem() function

🎉

I'm kind of stuck with the scrollToAction function, I found a lot of problems and still haven't found a proper solution to it (mainly because we still don't have the FlatList ref at componentDidMount time, I'm doing some studying to try to figure something out).

I think FlatList having a dynamic height items could also make this difficult to do. But I would just try to see how far we can get and avoid anything hacky. If it can't be done properly then we will have to re-think this aspiration.

On ReportScreen.js, why do we render other reports aside from the active one? I can't seem to figure out the benefit from that. I know that the other reports get a display of none, but it is still a lot of processing going on, which can easily be seen if we write a console.log on the renderItem function on ReportActionsView (and you have large pinned and unread reports).

Yes so this is a practice that will end soon since we are switching to react-navigation the initial motivation as I understand it was so that these reports would get "pre-rendered" and make switching between them faster. After implementing pagination and react-navigation we are anticipating it to be much faster to switch between chats so the rendering cost will not need to be front loaded on app init.

[lucas-neuhaus-dev]

Do we need to "get" the unreadActionCount I think it should just be attached to the report object?

Yeah, but we still have to write a little function to get it, since ONYXKEYS.COLLECTION.REPORT returns an array with all the reports. But the name is because I was actually returning this value from the function, but I am not anymore. I'll think of something better.
Below is how I extracted the unreadActionCount:

this.unreadActionCount = _.find(this.props.reports, report => (
      report.reportID === this.props.reportID
)).unreadActionCount;

I think FlatList having a dynamic height items could also make this difficult to do.

It does. I did some tests and it misses the action index by a decent margin.
The other way I can think of achieving this would be something like this. Try to calculate the height of each item on render time, do some calculations and call FlatList.scrollToOffset().

Yes so this is a practice that will end soon since we are switching to react-navigation the initial motivation as I understand it was so that these reports would get "pre-rendered" and make switching between them faster.

I see, thanks for the explanation.

[marcaaron]

Try to calculate the height of each item on render time, do some calculations and call FlatList.scrollToOffset()

I have fallen down this rabbit hole before :) Be careful not to lose yourself in there.

[iwiznia]

I noticed that in some cases (sorry, I have no idea how to reproduce) the app is showing the green unread line for message I sent myself (in the same browser window), which should never happen:

@lucas-neuhaus-dev seems like a bug in the implementation, can you take a look please?

[lucas-neuhaus-dev]

Hi @iwiznia, I can already imagine what caused this, my bad.

I'll take a look at this in just a few minutes and open a new PR once I got a solution. Thanks for pointing that out.

[iwiznia]

Awesome, thanks for the fast response!

[lucas-neuhaus-dev]

I was able to reproduce it when:

• Open a report with unread messages;
• Answer something before the indicator finishes fading out;
• The indicator "jumps" to the next message for every new message sent in this timestamp;

Hopefully that is the only edge case, if you think what you are facing is a different issue, please let me know.

[iwiznia]

I think it is a different issue, since I had not sent any new messages, but let's fix the issue you found and se how that goes.

[lucas-neuhaus-dev]

Just an update. I started a full-time job yesterday, so I didn't really have time to work on this.
But I promise I will do my best to get everything fixed this weekend and keep working on the following reviews. 👍
I apologize for this.

[Christinadobrzyn]

Hi @lucas-neuhaus-dev - Just checking it to see if you have an update on these changes!

[Christinadobrzyn]

Contributor has been paid, the contract has been completed, and the Upwork post has been closed.