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

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to [staging.new.expensify.com].
2. Open any chat
3. Send a random text message
4. Right-click on the message and select "Reply in Thread."
5. Send a random text in the thread
6. Reply in the thread to the text message sent in step 5. (The second thread which is in the middle)
7. Send another random text message in the thread
8. Hover over the text message sent in step 5
9. Right-click on the message and select "Mark as Unread."
10. Head over to the Left Hand Navigation (LHN)
11. Right-click on the bold message in the LHN. (The thread where you clicked as Unread on step 9)
12. 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
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6551569_1721830578914.1.mp4

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]

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]

@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]

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

[FitseTLT]

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

App/src/pages/home/report/ReportActionItemParentAction.tsx

Lines 75 to 79 in 3722b22

	onyxSubscribe({
	key: `${ONYXKEYS.COLLECTION.REPORT}${ancestorReportID}`,
	callback: (val) => {
	ancestorReports.current[ancestorReportID] = val;
	setAllAncestors(ReportUtils.getAllAncestorReportActions(report));

however at that moment with the way we grab the parent report from onyx here

App/src/libs/ReportUtils.ts

Lines 6793 to 6794 in 3722b22

	const parentReport = getReportOrDraftReport(parentReportID);
	const parentReportAction = ReportActionsUtils.getReportAction(parentReportID, parentReportActionID ?? '-1');

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

App/src/libs/ReportUtils.ts

Lines 6793 to 6794 in 3722b22

	const parentReport = getReportOrDraftReport(parentReportID);
	const parentReportAction = ReportActionsUtils.getReportAction(parentReportID, parentReportActionID ?? '-1');

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

What alternative solutions did you explore? (Optional)

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[eVoloshchak]

@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]

@eVoloshchak We are calling ReportUtils.getAllAncestorReportActions on the callback of onyx subscription on report collection here

App/src/pages/home/report/ReportActionItemParentAction.tsx

Lines 77 to 81 in 0e46257

	onyxSubscribe({
	key: `${ONYXKEYS.COLLECTION.REPORT}${ancestorReportID}`,
	callback: (val) => {
	ancestorReports.current[ancestorReportID] = val;
	setAllAncestors(ReportUtils.getAllAncestorReportActions(report));

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:

1. 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
2. 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.
   

   App/src/pages/home/report/ReportActionItemParentAction.tsx

   Lines 81 to 82 in 0e46257

   	setAllAncestors(ReportUtils.getAllAncestorReportActions(report));
   	},

                        setTimeout(() => setAllAncestors(ReportUtils.getAllAncestorReportActions(report)));

Both solutions work well.

[melvin-bot]

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

[eVoloshchak]

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]

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.

Can we change the order so allReports are updated before the ancestorReport?

As far as I know, No.

[melvin-bot]

@eVoloshchak @anmurali this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot]

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

[melvin-bot]

@eVoloshchak, @anmurali Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@eVoloshchak, @anmurali 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[melvin-bot]

This issue has not been updated in over 15 days. @eVoloshchak, @anmurali, @grgia eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[mvtglobally]

Issue not reproducible during KI retests. (Second week)

[mvtglobally]

Issue not reproducible during KI retests. (Third week)

[eVoloshchak]

• The PR that introduced the bug has been identified. Link to the PR: there isn't a PR that has caused this, this is a limitation of the initial implementation
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: Not needed, this is a rare edge case
• Determine if we should create a regression test for this bug
  Is it easy to test for this bug? Yes
  Is the bug related to an important user flow? Yes
  Is it an impactful bug? Yes
  It's easy to test for and is related to core functionality, so we might want to add a regression test

[eVoloshchak]

Regression Test Proposal

1. Go to [staging.new.expensify.com].
2. Open any chat
3. Send a random text message
4. Right-click on the message and select "Reply in Thread."
5. Send a random text in the thread
6. Reply in the thread to the text message sent in step 5. (The second thread which is in the middle)
7. Send another random text message in the thread
8. Hover over the text message sent in step 5
9. Right-click on the message and select "Mark as Unread."
10. Verify that the green unread marker is shown above the message.

Do we agree 👍 or 👎

[eVoloshchak]

@anmurali, the automation has failed, but this has been deployed to production for quite a while and is ready for payment

[FitseTLT]

@anmurali FYI You had already paid me accidentally, even before I raised the PR.

[mvtglobally]

Issue not reproducible during KI retests. (Fourth week)

[mvtglobally]

Issue not reproducible during KI retests. (Fifth week) Can we close it?

[eVoloshchak]

@anmurali, could you handle the payment on this one please? Thanks!

[anmurali]

@eVoloshchak - you have declined the offer
Are you able to still reverse that and accept it? Or do we need to send you a new offer?

[melvin-bot]

Triggered auto assignment to @OfstadC (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.

[anmurali]

Rotating the label so we can assign another BZ member while I am OOO. @OfstadC - you need to add regression test and handle @eVoloshchak payment.

[eVoloshchak]

Are you able to still reverse that and accept it? Or do we need to send you a new offer?

I'll request the payment through NewDot, all I need is a payment summary. Thank you!

[OfstadC]

Regression test

Sounds like @FitseTLT was already paid. Please correct me if i'm wrong!

Payment Summary

• @eVoloshchak due $250 via manual New Dot request