[HOLD] [WIP] Introduce "Mark as unread" functionality in Expensify.cash

[Gonals]

cc @roryabraham

HELD on #1977

Details

The "New" marker has been modified so that it no longer disappears after a few seconds. It will behave like Slack's marker, which remains until the user leaves the chat.

Fixed Issues

Tests

1. Log into e.cash and open a conversation with existing messages
2. On the hover menu of one of the messages, select "mark as unread":

3. The `new` marker should show up over that message and the chat in the LHN should become bolded:

5. Switch chats. It should remain bolded
6. Switch back to the original chat. The new marker should be correctly in place and the chat in the LHN should unbold in about 3 seconds. The new marker should remain in place after those 3 seconds.
7. Switch out and back in. the marker should be gone.
8. Do some regression testing to make sure the new marker shows correctly for actual new messages

Tested On

• Web
• Mobile Web
• Desktop
• iOS
• Android

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[roryabraham]

Overall, I think we could clean this up by leveraging Onyx a bit more to manage unreads and determine if the UnreadActionIndicator should show. This is what I'm thinking:

onMarkAsUnread in ReportActionsView is really just using the updateLastReadActionID Onyx action in Reports.js, then setting the unreadActionCount state. That state is ultimately only used to determine if a ReportActionItem should show the UnreadActionIndicator (something that's not even done in this component). What we could do instead is:

1. Add a callback field to each object in the CONTEXT_ACTIONS array in ReportActionContextMenu, and use that as the onPress handler in render. That eliminates the need for the extra prop, or to dynamically check the text or ID of the contextAction to determine the callback to use.
2. Make that callback just use the updateLastReadActionID Onyx action ... the ReportActionContextMenu already has all the info it needs in props to supply the parameters for that function.
3. You can then bind the ReportActionItem to the report it's attached to in Onyx and check the unreadActionCount field of that report to determine whether or not to show the UnreadActionIndicator

I think this will keep things a lot cleaner, especially once we have a half-dozen different context actions. What do you think?

[Gonals]

Overall, I think we could clean this up by leveraging Onyx a bit more to manage unreads and determine if the UnreadActionIndicator should show. This is what I'm thinking:

onMarkAsUnread in ReportActionsView is really just using the updateLastReadActionID Onyx action in Reports.js, then setting the unreadActionCount state. That state is ultimately only used to determine if a ReportActionItem should show the UnreadActionIndicator (something that's not even done in this component). What we could do instead is:

1. Add a callback field to each object in the CONTEXT_ACTIONS array in ReportActionContextMenu, and use that as the onPress handler in render. That eliminates the need for the extra prop, or to dynamically check the text or ID of the contextAction to determine the callback to use.
2. Make that callback just use the updateLastReadActionID Onyx action ... the ReportActionContextMenu already has all the info it needs in props to supply the parameters for that function.
3. You can then bind the ReportActionItem to the report it's attached to in Onyx and check the unreadActionCount field of that report to determine whether or not to show the UnreadActionIndicator

I think this will keep things a lot cleaner, especially once we have a half-dozen different context actions. What do you think?

I... think I'm not a fan 😅
1/2. While we could create all the callback functions in ReportActionContextMenu, that goes against our standard practices where we have the bulk of the logic in a larger component higher up and pass down whichever callbacks we need to (pinging @tgolen, since I had this discussion with him a while ago 😆)

3. Now that we have moved the marker inside ReportActionItem, we may be able to do something like this, although we'll still need to somehow make sure we are not displaying it for new messages that arrive while the chat is open, so we might still need the logic in ReportActionsView (not sure about this one. I'd have to play around for a bit, but that's my. gut feeling)

[Gonals]

I have read the CLA Document and I hereby sign the CLA

[Gonals]

Off WIP for now with a couple of caveats:

• If we decide to implement @roryabraham's suggested changes, it might go WIP again for a little bit
• I haven't been able to get the menu to show on mobile, so I haven't tested there yet

[marcaaron]

Rather than add an ignoreOrder flag that basically says "don't run this function" would it make more sense to not call the function in the first place.

[Gonals]

? It only skips checking the order, though. It runs the rest of the function. Basically, it prevents the function from checking that the new "latest read" is at the end of the conversation. That check is relevant when new messages come in, but not when we are manually marking messages as unread.

[marcaaron]

Hmm maybe we should remove that logic as well.. this logic is confusing and even with the added comment I'm having some trouble understanding in what other contexts this should be used.

[marcaaron]

Sorry, I'm trying to think of a better suggestion for this...

[marcaaron]

Ok so here is my confusion and would like to get @tgolen's thoughts to see if I'm missing something.

I'm not sure I understand why we need to get the last action from the sorted actions here to mark everything as read...

https://github.com/Expensify/Expensify.cash/blob/c5cb5076dc092ee5246e3410e4557fc2d55b4a88/src/pages/home/report/ReportActionsView.js#L266-L278

Here we are looking at the sequenceNumber on the last action... but this information is already available to the actions/Report and maxVisibleSequenceNumber is kind of a misnomer since it has nothing to do with visibility.

We are filtering out the "loading" actions, but that too could be done in the actions/Report file I think.

If that's true then we wouldn't need an ignoreOrder and could make sequenceNumber optional and the logic for this method could then end up being

function updateLastReadActionID(reportID, sequenceNumber) {
    const currentMaxSequenceNumber = reportMaxSequenceNumbers[reportID];
    const lastReadSequenceNumber = sequenceNumber 
        ? sequenceNumber 
        : currentMaxSequenceNumber;
    //... set the local last read 
}

Not sure if we need to do this change right now, but it seems more intuitive to "set to the most recent sequenceNumber when none is passed or use the one specified".

[tgolen]

I want to dig into this a little so I'm going to pull the branch down and dive into it. I want to make sure I understand everything before offering up further comments on this thread. Thanks for being patient with me!

[tgolen]

it seems more intuitive to "set to the most recent sequenceNumber when none is passed or use the one specified"

I love coming at it from this angle, and I agree it is much more intuitive.

but that too could be done in the actions/Report file I think.

I briefly looked at this, and I am pretty concerned that changing reportMaxSequenceNumbers to not include loading actions has a high risk of breaking things. I would be more comfortable with keeping a separate reportMaxSequenceNumbersForNonLoadingActions (lol at that ugly name).

It makes a lot of sense to me to move this logic from the view into the action file: https://github.com/Expensify/Expensify.cash/blob/c5cb5076dc092ee5246e3410e4557fc2d55b4a88/src/pages/home/report/ReportActionsView.js#L266-L278

[Gonals]

I'm afraid I'm the one who is lost now.
What do you mean by loading/not loading actions?

[tgolen]

In the ReportActionsView.js, when it calls updateLastReadActionID, it is passing the maxVisibileSequenceNumber. It gets that number by getting all the report actions and filtering out the ones with action.loading. This is what we are referring to.

What I would like to see for a solution is to have ReportActionsView.js updated so that all it does is this:

    recordMaxAction() {
        updateLastReadActionID(this.props.reportID);
    }

and also:

onMarkAsUnread={() => updateLastReadActionID(this.props.reportID, item.action.sequenceNumber - 1)}

All the rest of the logic will then be encapsulated into the actions/Report.js file.

Does that help clarify what we are asking for? It doesn't spell out the changes needed in Report.js, so let me know if you need help with that as well. I think this will be enough to point you in the right direction though.

[tgolen]

Add a callback field to each object in the CONTEXT_ACTIONS array in ReportActionContextMenu, and use that as the onPress handler in render. That eliminates the need for the extra prop, or to dynamically check the text or ID of the contextAction to determine the callback to use.

I very much agree with this suggestion. This is exactly what we are doing with the LHN and it has proven to be valuable.

While we could create all the callback functions in ReportActionContextMenu, that goes against our standard practices where we have the bulk of the logic in a larger component higher up and pass down whichever callbacks we need to

I see this a little bit differently. I view Rory's suggestion as following exactly what you are saying the standard practice is. The higher-up component will be doing the logic (by virtue of the callback).

dynamically check the text or ID of the contextAction to determine the callback to use

I think this is the part that stood out to me the most about this PR. It like an anti-pattern to have a dynamic callback as opposed to an explicit one. I think it also makes the component more difficult to use and implement because you need to have pretty in-depth knowledge of how the component works in order to implement callbacks for any of the other context menu items.

[tgolen]

Just looking at the method docs, I'm not sure what ignoreOrder implies. Would you mind adding a more detailed description to the param?

[Gonals]

Yup. Basically, it prevents the function from checking that the new "latest read" is at the end of the conversation. That check is relevant when new messages come in, but not when we are manually marking messages as unread.

[marcaaron]

As for the broader conversation above

You can then bind the ReportActionItem to the report it's attached to in Onyx and check the unreadActionCount field of that report to determine whether or not to show the UnreadActionIndicator

I think this is good advice. The other suggestion I am neutral on.

especially once we have a half-dozen different context actions

Maybe it is better to wait for this scenario.

I very much agree with this suggestion. This is exactly what we are doing with the LHN and it has proven to be valuable.

Full disclosure, I actually removed this logic because tightly coupling the a generic "option" with a hardcoded callback behavior made them harder to use in different contexts. In this case, I don't think it matters as much since these are less likely to be re-used so might not actually be important to change yet.

[Gonals]

Ok! I think I have addressed all comments.
I've changed both how we check the unreadActions (now we check the report from Onyx) and how the onMarAsRead callback works.
I think it is working fine, but I plan to do more testing tomorrow, since I get the feeling SOMETHING must be failing with how things are now (I haven't found what, yet).

Thanks, everyone!

[marcaaron]

NAB, one small side effect of this change is that we are now recreating this object with each render. I don't think it will cause problems tho.

[roryabraham]

Rather than recreating this object with every render, I think we should convert this to a class component, make contextActions a property of the class, then set it once in componentDidMount

[marcaaron]

Or the constructor()

[roryabraham]

Overall, I'm still not understanding the benefit to declaring the onMarkAsRead prop in ReportActionsView and drilling it down. All onMarkAsRead is doing is:

() => updateLastReadActionID(this.props.reportID, item.action.sequenceNumber - 1, true)

So it's using an Onyx action with data that's all available in ReportActionContextMenu, where the callback could just be:

onPress: () => updateLastReadActionID(this.props.reportID, this.props.reportActionID - 1, true)

That eliminates the extra clutter and unnecessary complexity of having an additional prop and drilling it down.

that goes against our standard practices where we have the bulk of the logic in a larger component higher up and pass down whichever callbacks we need to

Overall I'm not really sure I see this pattern as beneficial in an environment where we have a global data store to control our UI. Since we've already agreed to manage the unread data in Onyx, then the manipulation of that data should be controlled where the manipulation is done. Why should the already very complex ReportActionsView be responsible for managing all the functionality of the ReportActionContextMenu as well, when it's just writing data to the global store anyways?

Also, it looks like you accidentally committed a log file.

[Gonals]

Overall, I'm still not understanding the benefit to declaring the onMarkAsRead prop in ReportActionsView and drilling it down. All onMarkAsRead is doing is:

() => updateLastReadActionID(this.props.reportID, item.action.sequenceNumber - 1, true)

So it's using an Onyx action with data that's all available in ReportActionContextMenu, where the callback could just be:

onPress: () => updateLastReadActionID(this.props.reportID, this.props.reportActionID - 1, true)

That eliminates the extra clutter and unnecessary complexity of having an additional prop and drilling it down.

that goes against our standard practices where we have the bulk of the logic in a larger component higher up and pass down whichever callbacks we need to

Overall I'm not really sure I see this pattern as beneficial in an environment where we have a global data store to control our UI. Since we've already agreed to manage the unread data in Onyx, then the manipulation of that data should be controlled where the manipulation is done. Why should the already very complex ReportActionsView be responsible for managing all the functionality of the ReportActionContextMenu as well, when it's just writing data to the global store anyways?

Also, it looks like you accidentally committed a log file.

I'm partial to having the logic "higher up", especially since we'll be introducing other actions soon, but I don't really feel strongly about it. @marcaaron, @tgolen, any preferences on your side? I'm fine moving this down the line :)

[Gonals]

Sending this back to WIP as removing the local variable did break a few things as I expected. I'll try to fix it (or bring back the local variable if I can't). Let's keep the conversation above going in the meantime, though!

[Gonals]

Hey people! Since we were having several discussions and I kept reworking this, I have decided to split it in two!
For now, I'm sending you all to #1977 for the "New" marker fix. I'm still relying in a local variable, but I'm open to suggestions!

[Gonals]

Moving to #2433, since we kinda redid the whole thing!