[HOLD for Payment 2024-08-22][$250] Chat - Message marker disappears when the 2nd user deletes the first message

[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: 1.4.72-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: https://expensify.testrail.io/index.php?/tests/view/4550091
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

Precondition: User A in main testing device, user B in secondary device

1. Open the testing app as first user
2. Open any chat as user A (not the chat with user B)
3. As a use B send multiple messages (e..g, 1,2,3,4)
4. As user A open the chat with user B and observe the new message marker
5. As user B delete the first message sent (under the new message marker)

Expected Result:

New message marker to change place to the 2nd new message sent

Actual Result:

New message marker disappears

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

Bug6476074_1715268284452.Screen_Recording_2024-05-09_at_4.56.40_PM.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01074a670eb87f437f
• Upwork Job ID: 1790122767878377472
• Last Price Increase: 2024-05-20

Issue Owner

Current Issue Owner: @arosiclair

[melvin-bot]

Triggered auto assignment to @adelekennedy (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]

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

[adelekennedy]

still can't reproduce but I think this is due to site slowness

[adelekennedy]

aha - finally able to reproduce on chrome

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01074a670eb87f437f

[melvin-bot]

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

[dominictb]

Proposal

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

Chat - Message marker disappears when the 2nd user deletes the first message

What is the root cause of that problem?

When a message is deleted, the calculateUnreadMarker here will be called again.

Since the user is inside the report, at this point the lastReadTimeRef.current is already set to latest report lastReadTime (it will be set as such after the initial unread marker is calculated), so no report action is considered "unread" now, so the New marker disappears.

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

Thanks to the suggestion here, I've investigated and it seems we can fix this by storing the lastReadTime at the point of setting the marker

• In here, define a markerLastReadTimeRef to store the lastReadTime used to calculate the marker. We'll keep using it to calculate the next marker in case a report action was deleted

const markerLastReadTimeRef = useRef();

• When the currentUnreadMarker becomes null, makes sure to set markerLastReadTimeRef.current back to undefined, as markerLastReadTimeRef should only be available if we have a currentUnreadMarker calculated

useEffect(() => {
    if (currentUnreadMarker) {
        return;
    }

    markerLastReadTimeRef.current = undefined;
}, [currentUnreadMarker]);

• When we set the initial value of currentUnreadMarker here, also set the markerLastReadTimeRef to store its value

markerLastReadTimeRef.current = lastReadTimeRef.current;

• When calculating the marker to display here and here, pass in the lastReadTimeRef.current as 3rd param
• In shouldDisplayNewMarker here, define the lastReadTime as 3rd param, if it exists, use it as the basis for calculating the marker, instead of lastReadTimeRef.current
• [Optional] Update this so && becomes ||. The current condition doesn't make much sense because if the 1st condition is true, the second will always be true. We should update to || so that the currentUnreadMarker will also be updated if the marker changes from one report action to another. Marking this as optional because I don't see this causing any issues yet.

I've tested and it's working well.

What alternative solutions did you explore? (Optional)

A thing to ponder: we could explore making lastReadTimeRef and markerLastReadTimeRef 1 ref rather than two, but I think they serve different purposes so it likely will cause regressions. It'd be best to keep them separate. 1 ref is very light weight so there shouldn't be any problem.

[melvin-bot]

@allroundexperts, @adelekennedy Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[adelekennedy]

@allroundexperts one proposal to review above

[allroundexperts]

On it!

[tsa321]

Proposal

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

Unread message marker disappears when the other user delete first marked unread message of the current user.

What is the root cause of that problem?

When other user delete a message,calculateUnreadMarker will be called in here. This function mainly calling shouldDisplayNewMarker in here to determine whether marker should be displayed. In shouldDisplayNewMarker if currentUnreadMarker exist the flow will go in here:

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

Line 460 in a5bd1f2

shouldDisplay = reportAction.reportActionID === currentUnreadMarker;

But because of the deleted report action the report action will not be include in sortedVisibleReportActions, the marker would not be found:

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

Lines 492 to 494 in a5bd1f2

	if (!shouldDisplayNewMarker(reportAction, index)) {
	return;
	}

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

We could add useRef to store next report action created time of current marked report action, for example
const currentUnreadMarkerCreatedTime = useRef(null)

Then below this line:

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

Line 461 in a5bd1f2

}

We set the ref:

if (shouldDisplay) {
    currentUnreadMarkerCreatedTime.current =  index > 0 ? sortedVisibleReportActions[index - 1].created : null;
}

Then above this line , we check whether the currentUnreadMarker exist in sortedVisibleReportActions and change the currentUnreadMarker if it is not exist. The code could be:

if (currentUnreadMarker) {
    const isCurrentMarkerReportActionExist = sortedVisibleReportActions?.findIndex((reportAction) => {
        return reportAction.reportActionID === currentUnreadMarker;
    });
    if (isCurrentMarkerReportActionExist < 0) {
        const markeredAction = sortedVisibleReportActions.findLast((action) => new Date(action.created) >= new Date(currentUnreadMarkerCreatedTime.current));
        currentUnreadMarkerCreatedTime.current = markeredAction?.created;
        setCurrentUnreadMarker(markeredAction.reportActionID);
        return;
    }
} else {
    currentUnreadMarkerCreatedTime.current = null;
}

---

If user can delete multiple message at once we could store the nextMessagesIds list instead of exactly one nextMessageId. Then search inside this list which message isn't deleted and setCurrentMarker to the message.

The code could be:

Set the ref:

if (shouldDisplay) {
    reportActionIdsNextToCurrentMarker.current =
        sortedVisibleReportActions.slice(0, index).map((item) => {
            return item.reportActionID
        });
}

and set marker:

if (currentUnreadMarker) {
    const isReportActionIdExist = (reportActionID) => {
        return sortedVisibleReportActions?.find((item) => {
            return reportActionID === item.reportActionID;
        });
    }
    if (!isReportActionIdExist(currentUnreadMarker)) {
        const oldestUnreadId = reportActionIdsNextToCurrentMarker.current?.findLast((id) => {
            return isReportActionIdExist(id);
        })

        setCurrentUnreadMarker(oldestUnreadId ?? null);
        return;
    }
} else {
    reportActionIdsNextToCurrentMarker.current = null;
}

Result:

macos-web-d.mp4

[dominictb]

@tsa321 FYI your proposal won't fix this because the user could delete multiple report actions at once (ie. the action with the marker and the next one). In this case your reportActionIdNextToCurrentMarker also won't exist in the list so the marker will still disappear.

That's why we need to find "the first action after the marker that's still available" as suggested in my proposal

cc @allroundexperts

[tsa321]

@dominictb How to delete multiple message at once? I can only delete one message at a time?
If user can delete multiple message at once we could store the nextMessagesIds list instead of exactly one nextMessageId. Then search inside this list which message isn't deleted and setCurrentMarker to the message.

I have updated my proposal to address the multiple deleted message at once.

[adelekennedy]

@allroundexperts will review the proposals above (chill out Melvin)

[melvin-bot]

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

[dominictb]

If user can delete multiple message at once we could store the nextMessagesIds list instead of exactly one nextMessageId. Then search inside this list which message isn't deleted and setCurrentMarker to the message.

@tsa321 Then this is same idea as my proposal, except that I don't define a new list but use the existing sortedVisibleReportActions list 😃

Let's wait for @allroundexperts to chime in.

[dominictb]

@arosiclair following up on your comment here:#44856 (comment)

Shall we proceed the payment here (half the original bounty due to regression)? Thank you!

[arosiclair]

Per my last comment on the PR, I'm going to take this internal. I'll try taking the approach I outlined on this other issue.

[arosiclair]

@arosiclair following up on your comment here:#44856 (comment)

Shall we proceed the payment here (half the original bounty due to regression)? Thank you!

Unfortunately there will be no payment since your original PR had a regression and was reverted and we did not accept your follow up PR (so ultimately there was nothing delivered).

[dominictb]

@arosiclair apology to ask. I see there's some inconsistency here. I have another issue with the same problem (original PR is reverted, following PR is declined), but based on this comment #41197 (comment) there should be payment for both C and C+?

[arosiclair]

I believe they just made an exception in that case it's not a rule. I'll ask internally if we should do that again.

[mallenexpensify]

@dominictb this issue is different than #41197 (comment) because we gave you another chance to submit an acceptable PR. Since the follow up PR was overly complex and deviated from their proposal, @arosiclair chose to take this internal.

If the PR that ends up fixing the bug contains code from your followup PR, then you might be due compensation. We can address that once the PR hits production. Thx

[arosiclair]

Draft is done I just need to complete the checklist and post the PR

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[arosiclair]

^ this regression got fixed in #47393, but the "fix" is really just a partial revert. So I still have to go back and clean it up.

[ikevin127]

⚠️ Production deploy automation failed here -> this should be on [HOLD for Payment 2024-08-22] according to yesterday’s production deploy confirmed in #45797 (comment) and deploy checklist.

I reviewed the PR here as C+.

cc @adelekennedy

[ikevin127]

cc @adelekennedy

[adelekennedy]

payment due today!

I think the only payment due here is for @ikevin127 for reviewing the PR, I've asked internally about any other additional payments.

Payments Due

• Contributor+: $250 @ikevin127

Upwork offer is here.

[ikevin127]

@adelekennedy Offer accepted, thank you!