m-natarajan commented 1 month 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.46-2 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @paultsimura Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1728390912425749

Action Performed:

Open the chat with long history.
Scroll up until last messages are not visible.
Send a message.
Immediately scroll up.

The "Unread" green line marker appears above the new message;
Expected Result:

Since the user is the author of the message:
The "New messages" pill shouldn't appear;
The "Unread" green line marker shouldn't appear above the new message;
Actual Result:

The "New messages" pill appears;

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

https://github.com/user-attachments/assets/2b7ab579-9309-4ff1-adca-f8a60e3edf04

https://github.com/user-attachments/assets/ae7a35e5-fe3e-48c9-8959-d9382135a1dc

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021844272500597248585
Upwork Job ID: 1844272500597248585
Last Price Increase: 2024-10-10
Automatic offers:

ikevin127 | Reviewer | 104460359

Issue Owner

Current Issue Owner: @lschurr

melvin-bot[bot] commented 1 month ago

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

MonilBhavsar commented 1 month ago

I would like to curate this so assigning myself

lschurr commented 1 month ago

Do you need BugZero on this @MonilBhavsar or will it be all internal?

MonilBhavsar commented 1 month ago

We need BugZero and C+ on this. Going to export it

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

daledah commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-10-10 08:56:49 UTC.

Proposal

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

The "New messages" pill appears;

What is the root cause of that problem?

unreadMarkerReportActionID returns the new action when users add new comment, the reason is because we don't have the logic to exclude the message that comes from current user

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

In

https://github.com/Expensify/App/blob/175af25b5ac2645a7d1f7edf87fd35e4529cd9be/src/pages/home/report/ReportActionsList.tsx#L483-L488

when checking hasUnreadReportAction we exclude the current user's message so we should do the same in

https://github.com/Expensify/App/blob/175af25b5ac2645a7d1f7edf87fd35e4529cd9be/src/pages/home/report/ReportActionsList.tsx#L224

            const shouldDisplay = isCurrentMessageUnread && isNextMessageRead && !ReportActionsUtils.shouldHideNewMarker(reportAction) && (
                (ReportActionsUtils.isReportPreviewAction(reportAction) ? reportAction.childLastActorAccountID : reportAction.actorAccountID) !== Report.getCurrentUserAccountID()
            );

What alternative solutions did you explore? (Optional)

We can consider to create the util function to avoid duplicate code
We can update isMessageUnread function to exclude the current user's message

bernhardoj commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-10-15 14:10:25 UTC.

Proposal

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

An unread marker shows every time we add a message.

What is the root cause of that problem?

This happens after https://github.com/Expensify/App/pull/49367. Previously, we ignore any message that the current user sends, unless the user mark it as unread manually.

The unread marker shows because the scroll offset is out of the threshold (250) and isMessageUnread for the new message is true. https://github.com/Expensify/App/blob/285bde8073f84dad0ca6d63ef6ec251b10e52882/src/pages/home/report/ReportActionsList.tsx#L224-L226

isMessageUnread will be true if the report action created value is bigger than the unreadMarkerTime. https://github.com/Expensify/App/blob/285bde8073f84dad0ca6d63ef6ec251b10e52882/src/pages/home/report/ReportActionsList.tsx#L125-L131

The unreadMarkerTime is updated in 3 cases as written on the comment below. https://github.com/Expensify/App/blob/285bde8073f84dad0ca6d63ef6ec251b10e52882/src/pages/home/report/ReportActionsList.tsx#L202-L214

For the 3rd case, we have this useEffect that updates the unreadMarkerTime, but only if unreadMarkerReportActionID doesn't exist, which exist in our case because we are out of the scroll threshold.

https://github.com/Expensify/App/blob/285bde8073f84dad0ca6d63ef6ec251b10e52882/src/pages/home/report/ReportActionsList.tsx#L265-L278

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

We will have a new useEffect that is similar to the 3rd case, but in this case, we will only update the unreadMarkerTime if there is a new last action created value from the current user and there is previously no unread marker.

When user adds a new message and there is currently an unread marker, the prevUnreadMarkerReportActionID will always have a value.

For example, the current user mark A as unread: prevUnreadMarkerReportActionID: undefined unreadMarkerReportActionID: A

then, the current user sends a message. prevUnreadMarkerReportActionID will now contain A.

const prevUnreadMarkerReportActionID = usePrevious(unreadMarkerReportActionID);
const lastAction = sortedVisibleReportActions.at(0);
useEffect(() => {
    if (prevUnreadMarkerReportActionID || !lastAction) {
        return;
    }

    const isFromCurrentUser = accountID === (ReportActionsUtils.isReportPreviewAction(lastAction) ? !lastAction.childLastActorAccountID : lastAction.actorAccountID);

    if (!isFromCurrentUser || lastAction.created <= unreadMarkerTime) {
        return;
    }

    setUnreadMarkerTime(lastAction.created);
}, [prevUnreadMarkerReportActionID, lastAction?.created]);

What alternative solutions did you explore? (Optional)

We need to add a new case for updating unreadMarkerTime if a new message is added by the current user, even when the user scroll offset is out of threshold.

Luckily, we already have an observer to detect a new message from the current user. https://github.com/Expensify/App/blob/285bde8073f84dad0ca6d63ef6ec251b10e52882/src/pages/home/report/ReportActionsList.tsx#L371 https://github.com/Expensify/App/blob/285bde8073f84dad0ca6d63ef6ec251b10e52882/src/pages/home/report/ReportActionsList.tsx#L341-L353

We can update the unreadMarkerTime inside the callback, scrollToBottomForCurrentUserAction, but that only covers for optimistic case. After the AddComment API success, the BE will send us a new action created which is bigger from the optimistic created, so we still need to update the unreadMarkerTime when receiving the API response.

To do that, we can have a new ref called lastActionAddedByCurrentUser. We set the ref every time we send a message. reportActionID is from the 2nd parameter of the callback (scrollToBottomForCurrentUserAction).

if (!unreadMarkerReportActionID) {
    lastActionAddedByCurrentUser.current = reportActionID;
}

It's important to check for existing unread marker to not remove it when adding a new message, which is the current behavior. If we want to remove the marker when adding a new message, then we can remove the if. Make sure to add unreadMarkerReportActionID to the useCallback deps and add scrollToBottomForCurrentUserAction to this useEffect where it's being registered. https://github.com/Expensify/App/blob/285bde8073f84dad0ca6d63ef6ec251b10e52882/src/pages/home/report/ReportActionsList.tsx#L371-L385

Next, adds a useEffect that updates unreadMarkerTime whenever the last action is updated. If the last action from props is the same as the last action saved to the ref, then we update the unreadMarkerTime to that last action created, so unreadMarkerTime will equal to the last action created.

const lastAction = sortedVisibleReportActions.at(0);
useEffect(() => {
    if (!lastActionAddedByCurrentUser.current || lastAction?.reportActionID !== lastActionAddedByCurrentUser.current) {
        return;
    }
    setUnreadMarkerTime(lastAction.created);

    if (lastAction.isOptimisticAction) {
        return;
    }

    lastActionAddedByCurrentUser.current = undefined;
}, [lastAction]);

lastActionAddedByCurrentUser should be cleared after being used, but we only want to clear that if the last action is not an optimistic action anymore because we still want to update the unreadMarkerTime when receiving the new action from the API response.

Next, return false here if the report action is the same as the last action added by user. https://github.com/Expensify/App/blob/285bde8073f84dad0ca6d63ef6ec251b10e52882/src/pages/home/report/ReportActionsList.tsx#L220-L227

return shouldDisplay && isWithinVisibleThreshold && reportAction.reportActionID !== lastActionAddedByCurrentUser.current;

Last, we need to clear lastActionAddedByCurrentUser when mark as unread/read manually. https://github.com/Expensify/App/blob/285bde8073f84dad0ca6d63ef6ec251b10e52882/src/pages/home/report/ReportActionsList.tsx#L245-L251

ikevin127 commented 1 month ago

♻️ Reviewing proposals...

ikevin127 commented 1 month ago

From my experience with the ReportActionsList.tsx unread marker functionality I know that there's always more to consider and whenever a straight-forward fix was suggested in the past, we always had regressions because there's a lot of functionality which can be disrupted even with small changes.

Given this I'm inclined to go with bernhardoj's proposal since it mentioned the offending PR which caused the issue and also both RCA and solution proposed are more detailed and consider multiple aspects of the current functionality - hence it shows better understanding of the issue at hand.

However, before moving on with assignment here I want test the proposed solution thoroughly and because I don't want to miss any part of the solution, @bernhardoj can you please share a test branch with the complete solution so I can test it ?

♻️ Will get back on this once I get the chance to test the proposed solution.

bernhardoj commented 1 month ago

@ikevin127 here is the test branch

ikevin127 commented 1 month ago

@bernhardoj Thanks for putting together the test branch, I got to test the fix implementation and while it does seem to work compared to staging:

Compare Videos

| Staging | Test Branch | | ------------- | ------------- | |

⚠️ However I found an edge case in which the fix doen't work, allow me to explain the steps and show you a video demonstrating the issue:

[!tip]

Steps to reproduce

Login into the same account on 2 separate browser windows, one on local dev (with testing branch) and one on current staging (https://staging.new.expensify.com).

On local dev (with testing branch), verify that you can NOT reproduce the issue by following the steps in issue's description.

On local dev (with testing branch), make sure there is no new message marker or pill showing and scroll up such that you cannot see the last message.

On the same account / report on Staging, verify that you CAN reproduce the issue by following the steps in issue's description.

Now returning to the local dev test branc, observe that the new message marker and pill are showing even though you posted them with the same user account from Staging.

On local dev (with testing branch), mark as read to ensure there is no new message marker or pill showing then repeat step (2.) and notice that the fix does not work anymore after doing the Staging / Dev back n' forth.

[!note] The same thing happens if the steps are followed on 2 different sessions of the local dev (with testing branch) for example if logging in with the same account and following the above steps on 2 different browsers.

Here's a video for better visualisation:

Video

https://github.com/user-attachments/assets/5b34bd19-112f-4c45-9789-5a14594b2f86

Given this, from my side I see two issues here:

Regarding step (5.) and according to the issue's expected result, we should not see the new message marker and pill showing-up because the message was posted by the same user, even though it was posted from a different (Staging) session.
Regarding step (6.) which seems to somehow be caused and linked to step (5.), it looks like when the edge case steps are performed, something happens which invalidates the solution implementation and causes it to not work anymore.

bernhardoj commented 1 month ago

I updated my proposal with a different solution to cover that (and moved the previous main solution as the alternative).

ikevin127 commented 1 month ago

Thanks for the update, I'll review again today.

ikevin127 commented 1 month ago

I reviewed @bernhardoj's updated proposal's main solution again and this time it passed the tests. Given this I think we're good to go with their proposal as they show a deep understanding of the issue / functionality, the RCA is valid and the updated solution fixes the issue as per the expected result, without altering current working functionality.

For reference, here's how I implemented the solution locally:

Solution Code

```tsx const [session] = useOnyx(ONYXKEYS.SESSION); const prevUnreadMarkerReportActionID = usePrevious(unreadMarkerReportActionID); const currentUserAccountID = Number(session?.accountID); const lastAction = sortedVisibleReportActions.at(0); useEffect(() => { if (prevUnreadMarkerReportActionID || !lastAction) { return; } const isFromCurrentUser = currentUserAccountID === (ReportActionsUtils.isReportPreviewAction(lastAction) ? !lastAction.childLastActorAccountID : lastAction.actorAccountID); if (!isFromCurrentUser || lastAction.created <= unreadMarkerTime) { return; } setUnreadMarkerTime(lastAction.created); }, [prevUnreadMarkerReportActionID, lastAction?.created]); ```

[!tip]

Notes for PR

Make sure to comment the new useEffect in detail for others to understand why we need it and what it does.

Ensure thorough testing on narrow layout devices (native / mweb) since on these devices the unread marker / pill can work slightly different because of the DeviceEventEmitter listeners and LHN / Central Pane not being mounted both at the same time.

🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 1 month ago

Current assignee @MonilBhavsar is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

MonilBhavsar commented 1 month ago

Thanks for the detailed proposal! Looks good 👍 Let's also make sure to test other unread marker cases to ensure we don't introduce regression

melvin-bot[bot] commented 1 month ago

📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

bernhardoj commented 1 month ago

I'm still struggling to make the solution fully works. If you see the video below, while doing it offline (easier to repro), the unread marker will still show briefly.

https://github.com/user-attachments/assets/3b388d52-d426-46f0-bc65-0003b6074511

With the current solution, we can't really avoid that because the useMemo is always calculated first before we update the unreadMarkerTime.

bernhardoj commented 1 month ago

I'm rethinking to reuse the previous solution. The advantage of the previous solution is that we can tell that there is a new message added by the current user before unreadMarkerReportActionID is recalculated. And to overcome this issue where the unread marker still shows when adding message from another client, we can trigger the observer too when receiving the new message from Pusher.

If we read the comment here, the observer/subscriber is supposed to receive the event either locally or from Pusher. https://github.com/Expensify/App/blob/40e22304feb89061bf790231fdaba2917754d726/src/pages/home/report/ReportActionsList.tsx#L376-L378

However, currently the event is only triggered inside the notification callback. So, it's only triggered if the user will receive a notification which being guarded by some conditions. https://github.com/Expensify/App/blob/40e22304feb89061bf790231fdaba2917754d726/src/libs/actions/Report.ts#L2498 https://github.com/Expensify/App/blob/40e22304feb89061bf790231fdaba2917754d726/src/libs/actions/Report.ts#L2467-L2470 https://github.com/Expensify/App/blob/40e22304feb89061bf790231fdaba2917754d726/src/libs/actions/User.ts#L746-L750

The solution is to move the notifyNewAction calls outside the notification callback.

pushJSON.forEach((update) => {
    if (!update.key.startsWith(ONYXKEYS.COLLECTION.REPORT_ACTIONS)) {
        return;
    }
    const reportID = update.key.replace(ONYXKEYS.COLLECTION.REPORT_ACTIONS, '');
    const reportActions = Object.values((update.value as OnyxCollection<ReportAction>) ?? {});
    reportActions.forEach((action) => action && Report.notifyNewAction(reportID, action.actorAccountID, action.reportActionID));
});
const onyxUpdatePromise = Onyx.update(pushJSON).then(() => {
    triggerNotifications(pushJSON);
});

With the previous + new solutions above, the unread marker won't show if the current user adds a message, either from the current client or a different client.

@ikevin127 what do you think?

NOTE: sending message from the 2nd client will scroll to bottom the first client too. This is the same as WhatsApp.

ikevin127 commented 1 month ago

@bernhardoj Thanks for catching the offline behaviour issue, didn't think to test that scenario with the previous solution.

https://github.com/Expensify/App/issues/50469#issuecomment-2422671734 sounds good to me, I'd say you can move forward and start working on the PR with the (1st) previous solution + the new solution mentioned in above comment.

Unless @MonilBhavsar has any objections to https://github.com/Expensify/App/issues/50469#issuecomment-2422671734, I'd say you're good to go.

[!note] We just have to make sure we test properly and also make sure to add code comments for the new code additions in order for other contributors to know why and what the code does and why the notifyNewAction call has to be moved outside the notification callback.

bernhardoj commented 1 month ago

PR is ready

cc: @ikevin127

ikevin127 commented 1 month ago

♻️ Status update: We're working on reviewing and fixing edge cases in the PR, hopefully will be merged in 1-2 days.

MonilBhavsar commented 1 month ago

Thanks for the update! The new solution looks right 👍

melvin-bot[bot] commented 1 month ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 1 month ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.55-10 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

https://github.com/Expensify/App/pull/51113

If no regressions arise, payment will be issued on 2024-11-07. :confetti_ball:

For reference, here are some details about the assignees on this issue:

@bernhardoj requires payment through NewDot Manual Requests
@ikevin127 requires payment automatic offer (Reviewer)
@paultsimura requires payment (Needs manual offer from BZ)

melvin-bot[bot] commented 1 month ago

@ikevin127 @lschurr The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

ikevin127 commented 1 month ago

BugZero Checklist:

[x] [Contributor] Classify the bug:

Bug classification

Source of bug: - [ ] 1a. Result of the original design (eg. a case wasn't considered) - [x] 1b. Mistake during implementation - [ ] 1c. Backend bug - [ ] 1z. Other: Where bug was reported: - [ ] 2a. Reported on production - [x] 2b. Reported on staging (deploy blocker) - [ ] 2c. Reported on a PR - [ ] 2z. Other: Who reported the bug: - [ ] 3a. Expensify user - [ ] 3b. Expensify employee - [x] 3c. Contributor - [ ] 3d. QA - [ ] 3z. Other:

[x] [Contributor] 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: https://github.com/Expensify/App/pull/49367#issuecomment-2455934386.
[x] [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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: No need as the regression wasn't critical / interrupting a core flow.
[x] [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

[x] [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

Link to issue: https://github.com/Expensify/Expensify/issues/442577

Regression Test Proposal

Test:

Open any chat that is scrollable (has longer conversation history.)
Scroll to the top of the chat.
Send a message and immediately scroll to the top as the UX will try to scroll to bottom once the message is sent.
Now scroll up/down slightly and verify that there's no unread marker above the newly sent message and no New messages pill showing up at the top of the chat report when the newly sent message is not visible (out of scroll view).

[!note] An easier way to test this is to go offline / back online after posting a message, then there's no need to immediately scroll to the top, especially on small screen where there's no draggable scrollbar.

Please refer to the issue's description for see of how the issue was reproduced to better understand how to verify that the issue is not reproducible anymore.

Do we agree 👍 or 👎.

lschurr commented 3 weeks ago

Payment summary:

Contributor: @bernhardoj $250 (please request via Newdot)
Reviewer: @ikevin127 $250 (paid in Upwork)

bernhardoj commented 3 weeks ago

Requested in ND.

JmillsExpensify commented 3 weeks ago

$250 approved for @bernhardoj

Expensify / App

[HOLD for payment 2024-11-07] [$250] Chat: "Unread" marker appears when the user sends a new message #50469

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Steps to reproduce

Notes for PR

BugZero Checklist:

Regression Test Proposal

Test: