search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

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

Closed kavimuru closed 1 year ago

kavimuru commented 2 years ago

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?

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[bot] commented 2 years ago

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

slafortune commented 2 years ago

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 - image

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 image

slafortune commented 2 years ago

Also - similar issue is reported here -https://github.com/Expensify/App/issues/10736

melvin-bot[bot] commented 2 years ago

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

marcaaron commented 2 years ago

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

https://github.com/Expensify/App/blob/d0f0dd5ee7b88bf31d544f758ab7cfba88c6f0c0/src/pages/home/report/ReportActionsView.js#L179-L180

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

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[bot] commented 2 years ago

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[bot] commented 2 years ago

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

melvin-bot[bot] commented 2 years ago

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

isabelastisser commented 2 years ago

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 commented 2 years ago

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

melvin-bot[bot] commented 2 years ago

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

JmillsExpensify commented 2 years ago

Still need to circle back and find an internal volunteer. I have OOO this week, so I need to circle back on this one when I return.

marcaaron commented 2 years ago

I'll grab it @JmillsExpensify

marcaaron commented 2 years ago

Alright so kept looking into this and came up with a solution which is basically to extract the logic out of ReportActionsView here (since it's entirely unreliable to detect "new actions" this way and we've been doing it since the app's inception) and instead emit an event from Report.js that detects whether a new action has arrived via Pusher.

This will bring the unread indicator behavior in line with the browser notification behavior and fixes this issue (kinda).

However, there's currently a weird side effect where this only works if we wait 10 seconds between leaving a message and checking it as the recipient (because of this issue). So the new test steps would be something like:

  1. Leave message as User A and wait 10 seconds
  2. Open new tab to different chat as User B
  3. Navigate to chat with User A as User B
  4. See that new marker has been set correctly

I also noticed a different issue when working through this one.

The new marker line never shows if you visit the report that is unread in a new tab.

Repro steps for this new issue are:

  1. Leave message as User A
  2. Open new session to the chat with User A as User B
  3. See that new marker is not set and the chat gets marked as read in the LHN

It's correct that the report is marked as read in the LHN, but the new marker line should also be present. Not sure yet why this happens, but my working theory is that we mark the report as read when you open it, but there isn't enough time to calculate the new marker position since we can only reliably do that if the OpenApp command has finished.

marcaaron commented 2 years ago

Ok so for this other issue it does look like we are initializing the report with incorrect data. If we wait until the "report data" is done loading this is still not enough. The order of operations in the SaveResponseInOnyx middleware and our use of mergeCollection() to update the report data means that data from the API is only available some time after we set the isLoadingReportData flag to false - so it's not really dependable.

Left a comment about it here.

I'm not sure how to fix that without making either a change to the middleware, react-native-onyx or both.

marcaaron commented 2 years ago

Had to make a change in react-native-onyx to get around the second issue I came across. Will add focus on testing and writing up QA for the WIP PR today.

JmillsExpensify commented 2 years ago

Awesome, thanks a bunch @marcaaron for keeping this moving while I was out. The main point of clarity for me is whether the linked Draft PR solves both issues you identified here?

marcaaron commented 2 years ago

Yes it will! Writing up test steps now and will have an update soon.

JmillsExpensify commented 2 years ago

Ok awesome, thanks!

marcaaron commented 2 years ago

In review now.

JmillsExpensify commented 2 years ago

Linked PR waiting on confirmed tests to merge.

melvin-bot[bot] commented 2 years ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist will need to be completed before the issue can be closed:

JmillsExpensify commented 2 years ago

I'll have to get to this one tomorrow. Slipped my todos today.

JmillsExpensify commented 2 years ago

@marcaaron I'm curious for your take. Looking at the issue, your comment above, and the related PR, I'm not seeing any obvious updates that we should make to the unread test case in TestRail. Do you think I'm missing anything?

marcaaron commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

JmillsExpensify commented 2 years ago

Circling back and closing the loop on this today.

marcaaron commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

JmillsExpensify commented 2 years ago

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

marcaaron commented 2 years ago

Working on it.

JmillsExpensify commented 2 years ago

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

JmillsExpensify commented 1 year ago

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

JmillsExpensify commented 1 year ago

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 commented 1 year ago

@JmillsExpensify I think the issue is not resolved yet

JmillsExpensify commented 1 year ago

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 commented 1 year ago

Are you sure about that?

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

cc: @JmillsExpensify @marcaaron

marcaaron commented 1 year ago

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

JmillsExpensify commented 1 year ago

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 commented 1 year ago

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.

https://user-images.githubusercontent.com/32969087/206552452-d5c5d660-8d31-47ad-874f-d0904c352e6f.mp4

gadhiyamanan commented 1 year ago

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

https://user-images.githubusercontent.com/54790231/216527107-f94c50dd-716e-49a0-bd1a-2d47815250da.mov

JmillsExpensify commented 1 year ago

@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 commented 1 year ago

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.