Bug: [New Expensify] Unread message shows as bold in the LHN but green line does not appear reported by @gadhiyamanan

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

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

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

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.2.12-3
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
https://user-images.githubusercontent.com/43996225/195195728-9396a493-a85d-48b3-90ba-b8dc7d876f2b.mp4
https://user-images.githubusercontent.com/43996225/195195752-26184b12-3057-40dc-bac5-e3e67d76a1ff.mov
Expensify/Expensify Issue URL:
Issue reported by: @gadhiyamanan
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1665479496661839

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @slafortune (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[slafortune]

Reproduced on slafortune@expenisfy.com
New Expensify
Version 1.2.13-3 (1.2.13-3)
Desktop App

While the message is bolded indicating a new message was sent - the unread message does not show the “NEW” green line above the unread message.

I was not logged into New Expensify when the message was sent -
Notice the name is bolded and has no green line and New -

I was logged into New Expensify but not my active window when the message was sent -
Notice the Green line and New and the Name not bolded - I was not logged out of New Expensify

[slafortune]

Also - similar issue is reported here -#10736

[melvin-bot]

Triggered auto assignment to @marcaaron (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[marcaaron]

This is a pretty frustrating one. Gonna take it internal so we can design and fix it properly, but here's why it happens:

• We are using componentDidUpdate() to detect that a "new action has arrived" by looking at the last action and comparing it to the latest here:

App/src/pages/home/report/ReportActionsView.js

Lines 179 to 180 in d0f0dd5

	const didNewReportActionAppear = previousLastSequenceNumber !== currentLastSequenceNumber;
	if (didNewReportActionAppear) {

• We then update the newMarkerSequenceNumber to 0 because we are scrolled to the bottom of the chat

That is the correct behavior when you are already looking at the chat. But doesn't really work as expected when you:

• visit a chat
• fetch more recent actions
• component updates and processes a "new action" as if it has just arrived in realtime

Said another way, we are making a really bad assumption that a "new action appeared" when we only look at the componentDidUpdate() data changing. A general solution here would be to only allow the componentDidUpdate() logic to run when a "new" report action actually appears and not just some are loaded from the server when the component mounts. But I think in that case we're better off just moving it all out of the component entirely to avoid any confusion.

[melvin-bot]

A Contributor Manager will be assigned to issue payment via Upwork if we deploy an associated Pull Request to production. Per Contributing.md.

[melvin-bot]

Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

Triggered auto assignment to @isabelastisser (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[isabelastisser]

hey @marcaaron is there anything for me to do here? I reviewed the issue and your comments, and it seems like we're taking this internally, and the payment in Upwork will only need to be made if we deploy an associated Pull Request to production.

Unassigning myself.

[JmillsExpensify]

I'll assign myself and try to find someone to take this one.

[marcaaron]

I think it would be fine to add a test cases for the steps in the PR. There are four test cases and I'm not sure if any of them are covered by the existing test case.

[JmillsExpensify]

Ok cool, I'll take this part then. Anything else if the checklist you think we should fill out from a CME perspective? I think the testing scripts was all that applied?

[JmillsExpensify]

Still haven't gotten to this one, though on my list.

[JmillsExpensify]

Circling back and closing the loop on this today.

[marcaaron]

I think this one will need to stay open based on the conversation here. Unfortunately, some changes in my PR to fix this issue will need to be reverted.

After that, I'm honestly not too sure what possible solution to apply. There is a general conflict between: showing cached content as fast as possible and showing an accurate placement for the new line indicator. If we want it fast then it will be inaccurate (at least for some amount of time). If we want an accurate placement for the new line then we have to wait for the server to tell us where it should go. By that time, it's not clear whether the behavior would be to show the "new line" or mark the report as "read".

I thought about it for a bit and couldn't come up with a clear solution. Anyways, I've worked on this code a lot so I will let some others step up and take a stab at this problem once the revert PR is up.

[JmillsExpensify]

Thanks for jumping in, as I was wavering on the best next steps. That said, what do you think about minimally ensuring that TestRail has coverage for the QA steps in #12722?

[marcaaron]

Yes, that is the first suggestion in the RCA (of several)

[JmillsExpensify]

Ah sorry for crossing wires. Did you ship the RCA yet? I'll make sure to give it a thorough look.

[marcaaron]

Working on it.

[JmillsExpensify]

Still need to circle back on TestRail. I'll make time for that this week.

[JmillsExpensify]

Sorry ya'll, I keep struggling to come back to this one. Crossing this off the list before Thanksgiving kicks in.

[JmillsExpensify]

Alright, apologies for the delay here. I think we are good on this regression test, which is covered by this script in TestRail.

1. On an HT account that has a conversation with a very long history (>100 messages)
2. Navigate to the conversation that has >100 messages and Verify the loading animation is briefly displayed before the messages are loaded.
3. Verify the chat history is displayed after the loading animation
4. Verify it happens quickly and that there is no excessive delay
5. Navigate to another conversation and back to the one with >100 messages
6. Verify the loading animation is not displayed after visiting the chat that has been previously opened
7. Close the app and reopen - In Web, close and reopen the tab
8. Navigate to a conversation that has ~25 (smaller conversation) Verify the loading animation is briefly displayed before the messages are loaded.
9. Navigate to another conversation and back to the convo with ~25 messages
10. Verify the loading animation is not displayed after visiting the chat that has been previously opened

Closing this issue. Please re-open if you think I missed something.

[gadhiyamanan]

@JmillsExpensify I think the issue is not resolved yet

[JmillsExpensify]

Are you sure about that? No one else is assigned to this issue. cc @marcaaron not sure how that happened, but can you clarify?

[gadhiyamanan]

Are you sure about that?

still able to reproduce, I think PR reverted due to regression

cc: @JmillsExpensify @marcaaron

[marcaaron]

Yes, @gadhiyamanan is correct. See my comment here.

[JmillsExpensify]

Ah thanks, I forgot about that. So what's our next step in that case? We have a daily on this issue, though we're all not treating it that way, so minimally we should agree on it's priority and update the labels/titles accordingly.

[marcaaron]

Tried reproducing the original test steps again and could not. I think we can close for now and create a new issue if it gets reported again.

2022-12-08_09-39-42.mp4

[gadhiyamanan]

issue is reproducible on the latest version
cc: @JmillsExpensify @marcaaron

Screen.Recording.2023-02-03.at.11.49.24.AM.mov

[JmillsExpensify]

@marcaaron I'm not sure what's going on here, and I know we still have some deprecate sequence numbers polish in play. That said, I was able to reproduce the issue all the same.

[JmillsExpensify]

Actually, given that we're working on it in the linked issue from Ben and the reporter is the same, let's keep this one closed.