Fix "New" marker for the chat #1977

Gonals · 2021-03-22T16:08:23Z

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. It will also display in front of other elements, instead of half-hidden as it is now.

I tried to avoid relying on local variables and, instead, show/hide depending exclusively on report.unreadActionCount, but that basically caused it to hide after three seconds (since report.unreadActionCount gets updated)

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/157517

Tests

Open a chat (chat 1) in the app. Send a message there. No new marker should show up.
Receive a message in the chat. No new marker should show up
Move to a different chat (chat 2) and receive a message in chat 1. Move back to chat 1. You should see the new marker above the message. It should not disappear on its own:

4. After a few seconds, go back to chat 2 and then to chat 1 again. The marker should be gone. 5. If several messages come in while you are not in the chat, the marker should be above the oldest of them:

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Gonals · 2021-03-22T16:54:33Z

Mobile is being annoying. I'll update the screenshots when the emulators to behave again!

src/components/UnreadActionIndicator.js

roryabraham · 2021-03-22T18:41:15Z

It's a bit more complex, but from a very high level I think the ideal functionality for the unread indicator would be slightly different:

By default, the unread indicator only appears for new chats for 3 seconds, then disappears.
In the case then the Mark Unread action has been used on a report, then (and only then) does the unread indicator persist until the user leaves the report and comes back.

Curious what other people think.

marcaaron · 2021-03-22T19:18:11Z

By default, the unread indicator only appears for new chats for 3 seconds, then disappears.

I'm not sure I fully understand this feature tbh, but I think having it disappear after 3 seconds should not happen.

When you navigate to an unread chat then we should bring you to the "New" indicator and the chat itself would then be marked as "read" - but we wouldn't want to remove the context about which comment was read last (since that's useful to you, the person who is trying to catch up on a thread).

If you want to mark the chat as unread again from a certain point in time / comment then we can move the indicator to that position. If you navigate away from the chat and navigate back to it then we'd mark the report as "read" again, but not necessarily remove the indicator since you still might want to know where you last left off with the chat.

So, if you think about the problem this way "read" or "unread" becomes more of a boolean concept and the position of the indicator is more of a bookmark that tells you what you read last.

Gonals · 2021-03-23T08:54:19Z

So, if you think about the problem this way "read" or "unread" becomes more of a boolean concept and the position of the indicator is more of a bookmark that tells you what you read last.

😆 I am somewhere in between. I am for having the new marker be persistent as long as the chat is open. If you leave the chat and then come back to no new messages, no marker should be shown (I believe this is what Slack does right now), but your option would simplify this a lot. I'd just have to store a sequence number in Report.NewMarkerPosition and we'd be good to go 😆

tgolen · 2021-03-24T21:14:11Z

I'm going OOO and need to bow-out of this review. Sorry!

Gonals · 2021-03-25T09:32:31Z

I'm going OOO and need to bow-out of this review. Sorry!

No problem. Thanks, @tgolen!

Gonals · 2021-03-26T08:31:15Z

@roryabraham, @marcaaron bump!

roryabraham

The NEW is appearing as cut-off on iOS... maybe it's just not properly centered vertically?

marcaaron

Left some questions and suggestions

marcaaron · 2021-03-26T18:43:24Z

src/pages/home/report/ReportActionsView.js

-        // received while the report is still active
-        this.shouldShowUnreadActionIndicator = true;
+        this.unreadTimer = null;
+        this.newMessageMarkerPosition = -1;


Why are we setting this to -1?

Just initializing it to a non-valid position so we know is has yet to be set. I could initialize it to null, probably, if you like that better.

Hmm... I'm not entirely sold we need this variable yet. But if we are making -1 have a special meaning then maybe we should use a constant like NON_VALID_POSITION.

I'm ok with that. If we keep the variable, we can do that :)

src/pages/home/report/ReportActionsView.js

marcaaron · 2021-03-26T19:12:43Z

src/pages/home/report/ReportActionsView.js


-        this.setUpUnreadActionIndicator();
+        if (this.newMessageMarkerPosition < 0) {
+            this.newMessageMarkerPosition = this.props.report.unreadActionCount === 0


I wonder if we really need this variable, but correct me if this sounds wrong...

Since the newMessageMarkerPosition can always be inferred by looking at props we can instead make a method that calculates the marker position + tells us if a current sequenceNumber matches? I think it would be cleaner to call a method in renderItem that does this so that the logic isn't all over the place.

I think that will eliminate the need to "reset" this variable back to -1, but haven't tested this out.

newMessageMarkerPosition can't really be always inferred by looking at props, right? Once we update the unreadActionCount (which gets updated when we update the last action), we'll lose that info.
I've been trying to get rid of this variable, but nothing I could think of really worked (for a "permanent" marker), so I'm down for any suggestions.

It seems there are really only two things we need to know, yet we are expressing it in a single variable (and also updating it in a render method). This makes everything a little harder to understand IMO. Hopefully this helps think about the problem in a different way.

We need to know...

Whether we should show a new message marker at all. something like shouldShowNewMessageMarker stored in state could tell us this. When would we need to hide and show this marker?

If we should show the new message marker what sequenceNumber should we show it at? The easier question, as this can always be known by looking at the props like we are already doing here.

Yep, we can totally have that piece of data in a different variable or in the state if we want.

Nope. That's the issue I've been fighting with. this.props.report.unreadActionCount gets updated to zero after the 3 seconds timer ends. At that point, if we haven't saved the sequence in a variable, we lose it. When a new message comes in and the component gets rerendered, we'll lose the marker.

Sorry for all the back and forth here. I'm having a hard time understanding things, but I think turning a corner on a real suggestion. 😅

we can totally have that piece of data in a different variable or in the state if we want

Ok I think actually such a variable is not necessary, but it's an interesting exercise to think about when it might be true.

I think it'd be true when our unreadActionCount > 0 (you've got that part), but we need to use the unread count when the report view mounts (set in the constructor) or we switch to a new report.

This is where things get unclear for me...

this.props.report.unreadActionCount gets updated to zero after the 3 seconds timer ends. At that point, if we haven't saved the sequence in a variable, we lose it. When a new message comes in and the component gets rerendered, we'll lose the marker.

Right now we are setting this to -1 when the reportID changes and then updating it in the render() method when it's less than 0. This seems like kind of an anti-pattern because we're using -1 as a "signal" to the render() method that it needs to save this reference. That's not really the job of the render method and we should be able to save this reference when the component mounts or when the reportID changes.

So we should be able to end up with something like:

Set this.initialUnreadActionCount in componentDidMount and when we switch to a new report (reset())

Do the same checks we already have in renderItem()

I think that would clean things up a bit. Does that sound like it might work?

Haha, no worries! I also thought this would be simpler than it ended up being. This didn't work for a couple of reasons:

I can't get rid of the -1 check (unless we use a different variable):
When we swap reports, componentDidUpdate is actually called twice. Once with prevProps.reportID !== newProps.reportID, which is when we call reset, and once more afterwards to actually update the report contents.
On the first call, we still don't have the correct unreadActionCount, so I can't set the variable at that point. We need to wait till the second round, so I still need to check that "set this only once" flag (the -1, in this case).

I still tried moving the logic to componentDidUpdate so it wouldn't be in render, but since this change does't trigger a rerender, even though the variable gets set correctly, the marker doesn't show up until something else happens. Maybe I could force a redraw, but we probably want to avoid that. 😢

Ok, thanks for explaining. It seems clear what is happening here is that there is one update for the reportID then another update when withOnyx re-fetches anything with keys depending on props (so reportActions and report in this case).

Seems like we have two options...

Implement this workaround for now (maybe try to add a lot of comments to explain why we need to do what we're doing)

Fix Onyx so that we don't run into this situation again in the future

What would you prefer to do?

My expectation is that if a key depends on a prop and the prop it depends on changes we should check to see if we can also batch additional updates together instead of causing an extra re-render. I'm going to start looking into a solution on the Onyx side regardless and if we want to hold off for that, cool.

Otherwise, we can go ahead with the workaround, but here's what I'd suggest to make everything a little clearer...

In reset() set a flag on an instance variable called this.didReportIDChange to true

In componentDidUpdate() check to see if this is equal to true

Create a method called this.getInitialUnreadCount() and use is to initialize a variable in state this.state.initialUnreadActionCount in the constructor

In componentDidUpdate() check for this.didReportIDChange and use getInitialUnreadCount() to update the value of this.state.initialUnreadActionCount and set this.didReportIDChange back to false.

This will "force" an update. But I can't say whether that is a problem or not. I'd think maybe not.

Gonals · 2021-03-29T11:10:35Z

The NEW is appearing as cut-off on iOS... maybe it's just not properly centered vertically?

I'm fighting with the simulator today. Hopefully I'll get it to work and I'll fix this 😆

Gonals · 2021-03-30T11:16:54Z

The NEW is appearing as cut-off on iOS... maybe it's just not properly centered vertically?

I'm fighting with the simulator today. Hopefully I'll get it to work and I'll fix this 😆

Fixed!

Gonals · 2021-03-30T11:17:15Z

All comments addressed!

roryabraham

New messages received are not appearing as bolded in LHN on iOS. They are bold for like 1 render cycle, then go back to the read state:

marcaaron · 2021-03-31T00:49:42Z

src/pages/home/report/ReportActionsView.js

-        // received while the report is still active
-        this.shouldShowUnreadActionIndicator = true;
+        this.unreadTimer = null;
+        this.newMessageMarkerPosition = -1;


Hmm... I'm not entirely sold we need this variable yet. But if we are making -1 have a special meaning then maybe we should use a constant like NON_VALID_POSITION.

src/pages/home/report/ReportActionsView.js

marcaaron · 2021-03-31T01:48:20Z

src/pages/home/report/ReportActionsView.js


-        this.setUpUnreadActionIndicator();
+        if (this.newMessageMarkerPosition < 0) {
+            this.newMessageMarkerPosition = this.props.report.unreadActionCount === 0


It seems there are really only two things we need to know, yet we are expressing it in a single variable (and also updating it in a render method). This makes everything a little harder to understand IMO. Hopefully this helps think about the problem in a different way.

We need to know...

Whether we should show a new message marker at all. something like shouldShowNewMessageMarker stored in state could tell us this. When would we need to hide and show this marker?

If we should show the new message marker what sequenceNumber should we show it at? The easier question, as this can always be known by looking at the props like we are already doing here.

ubuntu-xenial-16.04-cloudimg-console.log

Gonals · 2021-03-31T08:55:40Z

Comments addressed!

marcaaron · 2021-04-01T03:43:34Z

src/pages/home/report/ReportActionsView.js

-            if (Visibility.isVisible()) {
-                this.timers.push(setTimeout(this.recordMaxAction, 3000));
+            // If we are adding a new action while the report is open, record the max action immediately
+            if (Visibility.isVisible() && prevProps.reportID === this.props.reportID && !this.unreadTimer) {


It doesn't seem like it's possible to have anything else besides

prevProps.reportID === this.props.reportID

since we return early above when these are not equal.

Why only when !this.unreadTimer? Does it have some special significance?

What if we record the last action anyway. Would something bad happen?

!this.unreadTimer: If we have a non-selected chat with new messages and move to it, the timer triggers (3s). This prevents the last action from being recorded before that timer ends (so always wait those three seconds, even if we get a new message). Otherwise, since the last sequence is different when we swap, we'd set the lastAction immediately, without waiting for the timer.

Not sure what you mean

This feels like a code smell to me. Probably if all the props updated at once we would not have this problem.

src/pages/home/report/ReportActionsView.js

marcaaron · 2021-04-01T04:10:46Z

src/pages/home/report/ReportActionsView.js

-                }).start();
-            }, 3000));
+    scheduledRecordMaxAction() {
+        // Always cancel the existing timer


What happens if we do not cancel this timer ?

Related to the answer above: I get rid of it so I could check for existence accurately (There is no "is there a timer running?" function)

marcaaron · 2021-04-01T04:44:27Z

src/pages/home/report/ReportActionsView.js


-        this.setUpUnreadActionIndicator();
+        if (this.newMessageMarkerPosition < 0) {
+            this.newMessageMarkerPosition = this.props.report.unreadActionCount === 0


Sorry for all the back and forth here. I'm having a hard time understanding things, but I think turning a corner on a real suggestion. 😅

we can totally have that piece of data in a different variable or in the state if we want

Ok I think actually such a variable is not necessary, but it's an interesting exercise to think about when it might be true.

I think it'd be true when our unreadActionCount > 0 (you've got that part), but we need to use the unread count when the report view mounts (set in the constructor) or we switch to a new report.

This is where things get unclear for me...

this.props.report.unreadActionCount gets updated to zero after the 3 seconds timer ends. At that point, if we haven't saved the sequence in a variable, we lose it. When a new message comes in and the component gets rerendered, we'll lose the marker.

Right now we are setting this to -1 when the reportID changes and then updating it in the render() method when it's less than 0. This seems like kind of an anti-pattern because we're using -1 as a "signal" to the render() method that it needs to save this reference. That's not really the job of the render method and we should be able to save this reference when the component mounts or when the reportID changes.

So we should be able to end up with something like:

Set this.initialUnreadActionCount in componentDidMount and when we switch to a new report (reset())

Do the same checks we already have in renderItem()

I think that would clean things up a bit. Does that sound like it might work?

Gonals · 2021-04-01T11:18:18Z

Comments answered!

Gonals · 2021-04-01T11:22:49Z

Heads up! I'll be OOO next week, so if we can't get this done today/tomorrow, it'll have to wait for a bit

Gonals · 2021-04-02T10:00:11Z

Hey @marcaaron, @roryabraham, I'll be OOO next week on vacation. If either of you wants to give this the final push, feel free to do so! Otherwise, I'll jump back into it when I'm back!

marcaaron · 2021-04-05T19:54:14Z

Sorry @Gonals we stalled a bit on this one. There's an open PR that will make it a lot easier to tell when a report/chat has mounted (and therefore when we should record an initial unreadActionCount. I think it will simplify this logic a lot so maybe we should just hold. cc @roryabraham for his thoughts.

#2221

marcaaron · 2021-04-10T01:03:40Z

Hey @Gonals I'm gonna close this in favor of #2338 so we can get the "New" indicator working correctly again.

Gonals added 4 commits March 19, 2021 11:49

fix and revamp new marker

d6d7efa

keep the new marker visible

74d7b5c

correctly place marker when new messages come in

7700a2a

merge master

79d18ca

Gonals requested a review from a team as a code owner March 22, 2021 16:08

Gonals self-assigned this Mar 22, 2021

botify requested review from NikkiWines and removed request for a team March 22, 2021 16:08

fix merge master error

575ebe1

Gonals removed the request for review from NikkiWines March 22, 2021 16:51

Gonals mentioned this pull request Mar 22, 2021

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

Closed

5 tasks

Gonals requested review from marcaaron, roryabraham and tgolen March 22, 2021 16:53

roryabraham reviewed Mar 22, 2021

View reviewed changes

src/components/UnreadActionIndicator.js Show resolved Hide resolved

store the sequence instead of the actioncount

6394e39

tgolen removed their request for review March 24, 2021 21:13

roryabraham self-requested a review March 26, 2021 18:10

roryabraham requested changes Mar 26, 2021

View reviewed changes

marcaaron reviewed Mar 26, 2021

View reviewed changes

Gonals added 2 commits March 29, 2021 11:37

Merge branch 'master' into alberto-fixNewMarker

2536b82

refactor timer setting

4ef6b21

Gonals requested a review from a team March 30, 2021 09:32

MelvinBot requested review from Luke9389 and removed request for a team March 30, 2021 09:32

Gonals removed the request for review from Luke9389 March 30, 2021 09:33

Fix marker on Mobile

f4b5d79

roryabraham requested changes Mar 30, 2021

View reviewed changes

marcaaron requested changes Mar 31, 2021

View reviewed changes

remove log file

5d1cc73

add conditions

928a2b8

marcaaron requested changes Apr 1, 2021

View reviewed changes

Move some code around

0188cb9

style

4e6a172

merge master

fde2830

marcaaron mentioned this pull request Apr 5, 2021

Added transition handling between the side drawer and chats #2221

Merged

5 tasks

marcaaron mentioned this pull request Apr 10, 2021

Make New indicator permanent until navigating away from chat #2338

Merged

5 tasks

marcaaron closed this Apr 10, 2021

marcaaron deleted the alberto-fixNewMarker branch April 10, 2021 01:03

Fix "New" marker for the chat #1977

Fix "New" marker for the chat #1977

Uh oh!

Conversation

Gonals commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

Gonals commented Mar 22, 2021

Uh oh!

Uh oh!

roryabraham commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcaaron commented Mar 22, 2021

Uh oh!

Gonals commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgolen commented Mar 24, 2021

Uh oh!

Gonals commented Mar 25, 2021

Uh oh!

Gonals commented Mar 26, 2021

Uh oh!

roryabraham left a comment

Choose a reason for hiding this comment

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gonals Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gonals commented Mar 29, 2021

Uh oh!

Gonals commented Mar 30, 2021

Uh oh!

Gonals commented Mar 30, 2021

Uh oh!

roryabraham left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gonals commented Mar 22, 2021 •

edited

Loading

roryabraham commented Mar 22, 2021 •

edited

Loading

Gonals commented Mar 23, 2021 •

edited

Loading

Gonals Mar 29, 2021 •

edited

Loading

marcaaron Mar 31, 2021 •

edited

Loading

marcaaron Mar 31, 2021 •

edited

Loading

Gonals Apr 1, 2021 •

edited

Loading

Gonals Apr 1, 2021 •

edited

Loading