[$250] Thread - New line marker appeared incorrectly when mark as read is clicked

lanitochka17 commented 3 months 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!

Version Number: 9.0.13-3 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): biruknew45+3456@gmail.com Issue reported by: Applause - Internal Team

Action Performed:

Go to [staging.new.expensify.com].
Open any chat
Send a random text message
Right-click on the message and select "Reply in Thread."
Send a random text in the thread
Reply in the thread to the text message sent in step 5. (The second thread which is in the middle)
Send another random text message in the thread
Hover over the text message sent in step 5
Right-click on the message and select "Mark as Unread."
Head over to the Left Hand Navigation (LHN)
Right-click on the bold message in the LHN. (The thread where you clicked as Unread on step 9)
Click "Mark as Read."

Expected Result:

When a message is marked as unread in step 9, the new line marker should appear immediately. The marker should not appear upon marking the message as read in step 12

Actual Result:

In step 9, after marking the message as unread, the new line marker does not appear as expected. In step 12, when the message is marked as read, the new line marker incorrectly appears in the chat

Workaround:

Unknown

Platforms:

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

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/bcbb41b2-89b4-45e1-aef2-e9be89494e09

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~016aa3122eebc32cef
Upwork Job ID: 1819054764777494946
Last Price Increase: 2024-08-29

Issue Owner

Current Issue Owner: @eVoloshchak

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @anmurali (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

lanitochka17 commented 3 months ago

@anmurali FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

lanitochka17 commented 3 months ago

We think that this bug might be related to #vip-vsp

FitseTLT commented 3 months ago

Proposal

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

Thread - New line marker appeared incorrectly when mark as read is clicked

What is the root cause of that problem?

When marked as unread the first time the report.lastReadTime of the parent report is changed and getAllAncestorReportActions will be called to recalculate ancestor report actions with the new value of the parent report here https://github.com/Expensify/App/blob/3722b22302e5d89f2916d5e8958b2cf17ecb6285/src/pages/home/report/ReportActionItemParentAction.tsx#L75-L79 however at that moment with the way we grab the parent report from onyx here https://github.com/Expensify/App/blob/3722b22302e5d89f2916d5e8958b2cf17ecb6285/src/libs/ReportUtils.ts#L6793-L6794 it doesn't get the up-to-date value of the parent report (it gets the previous version of the parent report) and hence the marker not shown and second time we mark as read it will get the parent report status it had when we marked as unread the first time so this time the maker will appear.

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

In ReportActionItemParentAction we need to pass the up-to-date ancestorReports list to getAllAncestorReportActions as a param and use it instead of value found from getReportOrDraftReport here https://github.com/Expensify/App/blob/3722b22302e5d89f2916d5e8958b2cf17ecb6285/src/libs/ReportUtils.ts#L6793-L6794

        const parentReport = ancestorReports[parentReportID] ?? getReportOrDraftReport(parentReportID);

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 3 months ago

Job added to Upwork: https://www.upwork.com/jobs/~018307aa2c28cda505

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

@eVoloshchak, @anmurali Eep! 4 days overdue now. Issues have feelings too...

eVoloshchak commented 3 months ago

@FitseTLT, thank you for the thorough explanation!

however at that moment with the way we grab the parent report from onyx, it doesn't get the up-to-date value of the parent report (it gets the previous version of the parent report)

What's causing this? getReportOrDraftReport uses allReports, is it not up to date? If so, how do we make it up to date? That would ensure we fix this (and similar bugs that might be caused by this) throughout the app

FitseTLT commented 3 months ago

@eVoloshchak We are calling ReportUtils.getAllAncestorReportActions on the callback of onyx subscription on report collection here https://github.com/Expensify/App/blob/0e46257f3295243e5ef306b723288cb1673ed6f1/src/pages/home/report/ReportActionItemParentAction.tsx#L77-L81

So the moment you mark unread, the change in the report (lastReadTime) will trigger this onyxSubscribe callback and inside it we are calling ReportUtils.getAllAncestorReportActions which depends on allReports whose value is updated inside an onyx connect callback (with waitForCollectionCallback) but its value is updated in another onyxSubscribe callback after the execution of our onyx callback here so in our case we are accessing the old outdated version of the report. The problem lies in calling getAllAncestorReportActions in onyx subscribe callback which depends on allReports which only gets updated later on the same execution cycle. We have two options to tackle this:

as in my proposal, pass getAllAncestorReportActions the updated version of the report so that it will use the updated/changed version of the report that is ready for our onyxSubscribe in here
We can also delay the execution of getAllAncestorReportActions to be executed on another execution cycle on which case now allReports will have been updated at the end of the current execution cycle and the up-to-date value will be ready then. https://github.com/Expensify/App/blob/0e46257f3295243e5ef306b723288cb1673ed6f1/src/pages/home/report/ReportActionItemParentAction.tsx#L81-L82
```
                    setTimeout(() => setAllAncestors(ReportUtils.getAllAncestorReportActions(report)));
```


Both solutions work well.

melvin-bot[bot] commented 3 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

eVoloshchak commented 2 months ago

We can also delay the execution of getAllAncestorReportActions to be executed on another execution cycle on which case now allReports will have been updated at the end of the current execution cycle and the up-to-date value will be ready then.

@FitseTLT, let's avoid using setTimeout unless there is no alternative

but its value is updated in another onyxSubscribe callback after the execution of our onyx callback here

Looks like a typo, both links are identical, but it's still clear what's happening, thank you for the explanation

The problem lies in calling getAllAncestorReportActions in onyx subscribe callback which depends on allReports which only gets updated later on the same execution cycle

What determines the order in which these entries are updated? Can we change the order so allReports are updated before the ancestorReport?

FitseTLT commented 2 months ago

What determines the order in which these entries are updated?

It's because allReports is a collection subscription (and also with waitForCollectionCallback) but ancestorReport is a single report subscription.