[Hold][$2000] Unread message shows as bold in the LHN but green line does not appear

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

Two cases:

First Case

1. Send a message when receiver is not logged in
2. log into the receiver account
3. Check the message

Second Case

1. Open two sessions one as User A and one as User B
2. Make sure User B is navigate to the chat with User A and save the URL
3. Close User B tab (or kill app)
4. Create message as User A on chat with User B and wait 10 seconds
5. Open tab (or app) as User B using the URL saved from the second step (on native use a deep link)
6. See that new marker is set correctly
7. Verify that the chat in the LHN is marked as read

Expected Result:

Both in the LHN and in the chat shows New message indicator

Actual Result:

No new message indicator inside the chat, only in the LHN it shows as bold as new message indicator

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• MacOS / Chrome / Safari

Version Number: 1.2.68-0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Recording.1488.mp4

Screen.Recording.2023-02-09.at.4.48.49.PM.mov

Expensify/Expensify Issue URL:
Issue reported by: @gadhiyamanan
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1675942126884169

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01649bcaf76128383a
• Upwork Job ID: 1623884459709763584
• Last Price Increase: 2023-02-17

[MelvinBot]

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

[MelvinBot]

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) - previously reported in Bug: [New Expensify] Unread message shows as bold in the LHN but green line does not appear reported by @gadhiyamanan #11726
  • 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

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~01649bcaf76128383a

[MelvinBot]

Current assignee @bfitzexpensify is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia (External)

[MelvinBot]

Triggered auto assignment to @bondydaa (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[bfitzexpensify]

Just adding here, this is the green line in question that's missing:

[TMisiukiewicz]

I want to investigate that one, Callstack

[abdulrahuman5196]

Proposal

Please re-state the problem that we are trying to solve in this issue.

User logs in to his account and already has unread messages. If user goes to the unread chat 'new' message indicator doesn't show up.

What is the root cause of that problem?

Background 1: When the user logs in, we fetch the list of all reports(this doesn't include report actions). Now when user goes to any chat we call openReport API to fetch all the details of the chat including report actions. This openReport API also updates the lastReadTime to current time in the chat.
Background 2: The new indicator is shown based on internal state maintained in the ReportActionsView.js, it is initially set when the component is created and it gets updated based on user actions like message deletes, manual unreads. This is done to keep an internal state to show the new message indicator even if the last read time changes after chat is shown.
Code pointer on initial set - https://github.com/Expensify/App/blob/main/src/pages/home/report/ReportActionsView.js#L71

Root cause:
Step 1: User login and open an unread chat(message was sent during logout state as mentioned in issue)
Step 2: We don't show the ReportActionView in ReportScreen.js and show the skeleton view until we have the report actions data -> code - https://github.com/Expensify/App/blob/main/src/pages/home/ReportScreen.js#L269
Step 3: Now we initiate OpenReport API in parallel to fetch the report data when the user opens unread chat. This will also reset the lastReadTime to latest time as per background1.
Step 4: OpenReport API is successful.
Step 5: Now we have all the report data and report.lastReadTime is having the latest time value
Step 6: ReportScreen.js removes the skeletonView and starts to show ReportActionsView component.
Step 7: But when ReportActionsView tries to find which message to show as the latest message, we won't get any message because the lastReadTime of the report is latest and ReportActionsView ends up not showing any new message indicator - https://github.com/Expensify/App/blob/main/src/pages/home/report/ReportActionsView.js#L71
Logic to find the new message based on report.lastReadTime https://github.com/Expensify/App/blob/main/src/libs/ReportUtils.js#L1410

What changes do you think we should make in order to solve the problem?

I think there is a core problem in having newMarkerReportActionID as state to show new indicator which represents the message id to show as new. But we end up doing lot of checks and complex logic to make the newMarkerReportActionID a proper value when the message gets deleted or new message is marked as unread. It seems like we are re-doing the old sequence number unread logic.

I propose instead of having newMarkerReportActionID as a state to show new indicator, have the 'lastReadTime' of a report as a state to show the new line indicator and new message button indicator as well.
To flow will be like this.

1. When user opens the chat from LHN, we store the 'lastReadTime' in a state, so that we would show the next message after the time with a 'new' message indicator and appropriate 'new message' button as per requirement. For example store the 'lastReadTime' value in ReportScreen.js as local state value and pass it as props to ReportActionsView.js.
2. We update this 'lastReadTime' only on user interactions like 'unread message', 'scrollToBottomAndMarkReportAsRead' and any other user action if required.
3. While displaying we will check which message is be shown the 'new' message indicator.

This way only the 'lastReadTime' state is the primary source of truth to display the 'new' message UI in the ReportActionsView.js

Advantages:

1. This way we don't have to worry if a message is deleted/available or not. The new message indicator will automatically shift to the next message when a new line indicator message is deleted. And it will fix this current issue of messages not being available at first place as well.
2. We can remove multiple checks and complex logics in keeping the newMarkerReportActionID up-to-date here - https://github.com/Expensify/App/blob/main/src/pages/home/report/ReportActionsView.js#L179
3. This will be in-sync with the new sequence number migration which is completed and we can start to rely on lastReadTime instead of hardcoded messageIds(seems to be like old sequence number implementation)
4. This will not impact any current behaviours.

This could solve these 4 bugs in one shot
#13740
#14649
#15019
#12598

Only one disadvantage i see is that we would compute which message to show up as a new message while displaying, but it shouldn't be that complex computation as well as we only compare lastReadTime and action created time.

As this is not going to be an one line change there are multiple code-paths we can achieve the replacement of newMarkerReportActionID with lastReadTime state. One of best path at current codebase would be to maintain the state at reportScreen and propogate as we don't mount ReportActionsView(data won't be propagate to it when unmounted state). We can also do the same at other places but the core proposal is to remove the state of maintaining message ids and move to maintaining state of lastReadTime.

What alternative solutions did you explore? (Optional)

Alternative 1:
We can add checks in the ReportActionsView.js to update the newMarkerReportActionID after the report is fully loaded, but as mentioned above we would end up adding more checks and complex logics similar to this in future to make newMarkerReportActionID up-to-date as per requirements. So the alternative seems to be a short term solution.

Alternative 2:
In ReportScreen.js we can store the state of report.lastReadTime when user opens the report screen. Then pass it as props to ReportActionsView.js, so when we try to find the new message in ReportActionsView constructor we can use the lastReadTime sent from ReportScreen.js instead of using report.lastReadTime here - https://github.com/Expensify/App/blob/main/src/pages/home/report/ReportActionsView.js#L71

Alternative 3(Best IMO):
Instead of maintaining lastReadTime in states as per proposal, we can maintain a local onyx value similar to draftComment value. We will use this onyx lastReadTime value to display the 'new' line marker. We will update this value only on certain conditions for example,

1. When user opens ReportScreen we set the onyx local lastReadTime to report.lastReadTime
2. When user marks unread. set onyx local lastReadTime to unread message time
3. other applicable cases to not break current experience.

[s77rt]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Opening an unread report for the first time is missing the new marker indicator

What is the root cause of that problem?

Opening any report will not only return the report actions but also set the last read time of that report to the current time. This is not necessary a bug. I'm considering this as a design decision and working on top of that.

To help understand the problem let's consider two unread reports:

• Report 1: Unread report actions are loaded into onyx (got them via pusher as we were already signed in)
• Report 2: Unread report actions are not loaded into onyx

And let's consider the working flow after you click a report as follow:

1. Render report (and set the new marker indicator if eligible)
2. Make OpenReport API request (and update the last read time)

Rendering the first report will have the new marker indicator set since we have unread report actions in onyx. However that is not the case with the second report, in fact the second report will be rendered without the new messages (despite being shown on LHN) up until getting a response from OpenReport but at this point the last read time is also updated.

What changes do you think we should make in order to solve the problem?

Fetch unread report actions before rendering the report. This can be achieved by having the server prefetch report actions for prefetched reports (or at least for prefetched unread reports). The report actions should be returned as a part of the OpenApp response (this needs to be done internally). I have created this slack thread to get more feedback on that approach.

What alternative solutions did you explore? (Optional)

None

[thesahindia]

I propose instead of having newMarkerReportActionID as a state to show new indicator

I believe this is something that needs to be discussed on slack for more visibility. We need more opinions on this. Can you please start a discussion @abdulrahuman5196 ?

[abdulrahuman5196]

@thesahindia Thank you. I have raised slack conversation here - https://expensify.slack.com/archives/C01GTK53T8Q/p1676267128690339

[abdulrahuman5196]

@thesahindia I have updated the proposal with clear information as per slack request - #15019 (comment)
I have also added a alternative 2 proposal which should be simpler and can solve this particular issue alone if required.

[MelvinBot]

@bondydaa, @bfitzexpensify, @thesahindia Whoops! This issue is 2 days overdue. Let's get this updated quick!

[bondydaa]

we're discussing in the slack thread still https://expensify.slack.com/archives/C01GTK53T8Q/p1676267128690339

[JmillsExpensify]

Coming from this issue, please make sure that the case from #12598 is also addressed. I've also added the reproduction steps from that issue at the top of this one. Thanks!

[quinthar]

This is a regression right?

[quinthar]

Why is this on hold, this used to work

[bondydaa]

@quinthar this is being handled on this tracking issue #15212

[bfitzexpensify]

Tracking issue continues to progress

[Victor-Nyagudi]

This issue also occurs after flagging a message as spam, inconsiderate, etc.

expensify-flag-message-bug.mov

I thought I'd create a new bug, but it looked to similar to the current issue.

There's another recently opened issue where marking Concierge's message as unread doesn't work when it's the last message that could be worthy of being added to the tracking issue.

Thoughts, @bondydaa?

[bondydaa]

web deploy and deploy blockers have taken all my time this week

[bondydaa]

looks like design doc was reviewed and implementation has started per #15212 (comment)

[bondydaa]

looks like the tracking issue is getting close to being done 🎉

I'm going to retest these steps now.

[bondydaa]

Hmm I'm not sure if these are the intended behaviors or not anymore but this is what I'm seeing:

Logging in, no new message indicator: https://github.com/Expensify/App/assets/4073354/0c0f764f-82d6-4291-9fc9-d17b6d2a2b1d

Even when if I leave the tab open, it looks like a new message shows up then gets marked as read quickly after: https://github.com/Expensify/App/assets/4073354/9d89865d-dce1-456a-a022-57b388f00b89

cc @MonilBhavsar

[bondydaa]

oh sorry looks like the 2nd thing is being tracked here #23171

[bondydaa]

looks like #23171 is still open

[bfitzexpensify]

Still on hold

[MonilBhavsar]

This should be fixed! Can we please retest and close this issue

[bfitzexpensify]

Yes, I'm no longer able to reproduce. Closing this out.