Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$500] Chat - Inconsistency in displaying unread green line when navigating from thread #31026

Closed
6 tasks done
lanitochka17 opened this issue Nov 7, 2023 · 60 comments
Closed
6 tasks done
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Nov 7, 2023

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.3.96-5
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: Applause - Internal Team
Slack conversation:

Action Performed:

  1. Navigate to staging.new.expensify.com
  2. Go to chat with long history
  3. Mark the latest message as unread
  4. Scroll up and enter a thread
  5. Tap on the subtitle in thread to return to main chat
  6. Scroll down and note that green line is still there
  7. Tap on the back button
  8. Tap on the subtitle again
  9. Scroll down and note that the green line is gone
  10. Tap on the back button twice

Expected Result:

The green line for unread message will not show up since the unread message is visited and the green line is gone in Step 9

Actual Result:

The green line for unread message reappears

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

Bug6267891_1699386933157.Screen_Recording_20231107_222134_New_Expensify.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01e2d8d823ccdf91aa
  • Upwork Job ID: 1721983249610829824
  • Last Price Increase: 2024-04-11
  • Automatic offers:
    • Ollyws | Reviewer | 0
    • FitseTLT | Contributor | 0
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 7, 2023
@melvin-bot melvin-bot bot changed the title Chat - Inconsistency in displaying unread green line when navigating from thread [$500] Chat - Inconsistency in displaying unread green line when navigating from thread Nov 7, 2023
Copy link

melvin-bot bot commented Nov 7, 2023

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

Copy link

melvin-bot bot commented Nov 7, 2023

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 7, 2023
Copy link

melvin-bot bot commented Nov 7, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

Copy link

melvin-bot bot commented Nov 7, 2023

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

@s-alves10
Copy link
Contributor

s-alves10 commented Nov 8, 2023

This issue will be fixed by #30696 (comment)

@Ollyws

Will you take a look?

@s-alves10
Copy link
Contributor

Proposal

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

There are some inconsistencies in displaying unread green line marker when navigating from and to thread

What is the root cause of that problem?

The main issue is in weird logic of the following code

if (!shouldDisplayNewMarker(reportAction, index)) {
return;
}
markerFound = true;
if (!currentUnreadMarker && currentUnreadMarker !== reportAction.reportActionID) {
setCurrentUnreadMarker(reportAction.reportActionID);
}

  • Why can't we set currentUnreadMarker if that is not a null. I think this is wrong. currentUnreadMarker should be able to change when it's not a null
  • During the iteration, currentUnreadMarker can be changed and the iteration would be started again because the dependency changes. This forces re-render
  • We have redundant dependencies below. All other dependencies except shouldDisplayNewMarker are already shouldDisplayNewMarker's dependencies. No needs to add here
    }, [sortedReportActions, report.lastReadTime, messageManuallyMarkedUnread, shouldDisplayNewMarker, currentUnreadMarker]);

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

  1. Update
    const shouldDisplayNewMarker = useCallback(
    (reportAction, index) => {
    let shouldDisplay = false;
    if (!currentUnreadMarker) {
    const nextMessage = sortedReportActions[index + 1];
    const isCurrentMessageUnread = isMessageUnread(reportAction, report.lastReadTime);
    shouldDisplay = isCurrentMessageUnread && (!nextMessage || !isMessageUnread(nextMessage, report.lastReadTime));
    if (!messageManuallyMarkedUnread) {
    shouldDisplay = shouldDisplay && reportAction.actorAccountID !== Report.getCurrentUserAccountID();
    }
    } else {
    shouldDisplay = reportAction.reportActionID === currentUnreadMarker;
    }
    return shouldDisplay;
    },
    [currentUnreadMarker, sortedReportActions, report.lastReadTime, messageManuallyMarkedUnread],
    );
    useEffect(() => {
    // Iterate through the report actions and set appropriate unread marker.
    // This is to avoid a warning of:
    // Cannot update a component (ReportActionsList) while rendering a different component (CellRenderer).
    let markerFound = false;
    _.each(sortedReportActions, (reportAction, index) => {
    if (!shouldDisplayNewMarker(reportAction, index)) {
    return;
    }
    markerFound = true;
    if (!currentUnreadMarker && currentUnreadMarker !== reportAction.reportActionID) {
    setCurrentUnreadMarker(reportAction.reportActionID);
    }
    });
    if (!markerFound) {
    setCurrentUnreadMarker(null);
    }
    }, [sortedReportActions, report.lastReadTime, messageManuallyMarkedUnread, shouldDisplayNewMarker, currentUnreadMarker]);

    to
    const shouldDisplayNewMarker = useCallback(
        (unreadMarker, reportAction, index) => {
            let shouldDisplay = false;
            if (!currentUnreadMarker) {
                const nextMessage = sortedReportActions[index + 1];
                const isCurrentMessageUnread = isMessageUnread(reportAction, report.lastReadTime);
                shouldDisplay = isCurrentMessageUnread && (!nextMessage || !isMessageUnread(nextMessage, report.lastReadTime));
            } else {
                shouldDisplay = reportAction.reportActionID === unreadMarker;
            }
            return shouldDisplay;
        },
        [report.lastReadTime, sortedReportActions],
    );

    useEffect(() => {
        // Iterate through the report actions and set appropriate unread marker.
        // This is to avoid a warning of:
        // Cannot update a component (ReportActionsList) while rendering a different component (CellRenderer).
        let unreadMarker = null;
        _.each(sortedReportActions, (reportAction, index) => {
            if (!shouldDisplayNewMarker(unreadMarker, reportAction, index)) {
                return;
            }
            unreadMarker = reportAction.reportActionID;
        });
        setCurrentUnreadMarker(unreadMarker);
    }, [shouldDisplayNewMarker]);
  1. Remove the below code segments

    const [messageManuallyMarkedUnread, setMessageManuallyMarkedUnread] = useState(0);

    useEffect(() => {
    const didManuallyMarkReportAsUnread = report.lastReadTime < DateUtils.getDBTime() && ReportUtils.isUnread(report);
    if (didManuallyMarkReportAsUnread) {
    // Clearing the current unread marker so that it can be recalculated
    setCurrentUnreadMarker(null);
    setMessageManuallyMarkedUnread(new Date().getTime());
    return;
    }
    setMessageManuallyMarkedUnread(0);
    // We only care when a new lastReadTime is set in the report
    // eslint-disable-next-line react-hooks/exhaustive-deps
    }, [report.lastReadTime]);

  2. Update

    shouldDisplayNewMarker={shouldDisplayNewMarker(reportAction, index)}

    to

        shouldDisplayNewMarker={shouldDisplayNewMarker(currentUnreadMarker, reportAction, index)}

This works as expected

What alternative solutions did you explore? (Optional)

@FitseTLT
Copy link
Contributor

FitseTLT commented Nov 8, 2023

Proposal

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

Inconsistency in displaying unread green line when navigating from thread

What is the root cause of that problem?

When a user reply in thread and navigate to the same report via the subtitle link (repeatedly) report screens with the same report will exist in the navigation stack so when the user navigates back they will navigate back to the report screen with the same report in it's previous state as we are not reseting currentUnreadMarker whenever the user opens a new report.

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

In RepotActionList We need to have a useEffect that runs when the screen loses focus and the current report is not top most report we should reset currentUnreadMarker so we can do that by

    useEffect(() => {
        if (prevReportID === report.reportID) {
            return;
        }
        cacheUnreadMarkers.delete(report.reportID);
        setCurrentUnreadMarker(null);
    }, [isFocused, report.reportID]);

We can also use Navigation.getTopmostReportId to get the topmost report but the above suffices as we use a global variable prevReportID to track the top most reportID


And additionally if we want we can call calculateUnreadMaker whenever the screen gets focus back and currentUnreadMarker is null so that it will recalculate unread marker if there is a new message received before navigating back 👍 and also call readNewestAction to mark it as unread same as we do if the user opened the report from LHN

@FitseTLT
Copy link
Contributor

FitseTLT commented Nov 8, 2023

This issue will be fixed by #30696 (comment)

@Ollyws

Will you take a look?

@s-alves10 to proof that your solution doesn't work just on your suggested changes applied, mark as unread a report action and refresh the page now the marker is expected to be displayed as the logic of mark as unread is to have that marker when next time a user opens the report. But in your case the marker will display for a sec and it disappears when lastReadTime is changed. For your information messageManuallyMarkedUnread is used to decide not to display the marker for the user's own message or report action unless it is manually marked unread.

shouldDisplay = isCurrentMessageUnread && (!nextMessage || !isMessageUnread(nextMessage, report.lastReadTime));
if (!messageManuallyMarkedUnread) {
shouldDisplay = shouldDisplay && reportAction.actorAccountID !== Report.getCurrentUserAccountID();
}

In my opinion, all other logic is working fine (expect the unnecessary dependencies you mentioned which we can remove) so I suggest we only fix this specific issue.

@Tony-MK
Copy link
Contributor

Tony-MK commented Nov 9, 2023

Proposal

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

Inconsistency in displaying unread green line when navigating from thread

What is the root cause of that problem?

The shouldDisplayNewMarker() function in ReportActionsList.js determines if the unread line should be displayed or not.

const shouldDisplayNewMarker = useCallback(
(reportAction, index) => {
let shouldDisplay = false;
if (!currentUnreadMarker) {
const nextMessage = sortedReportActions[index + 1];
const isCurrentMessageUnread = isMessageUnread(reportAction, report.lastReadTime);
shouldDisplay = isCurrentMessageUnread && (!nextMessage || !isMessageUnread(nextMessage, report.lastReadTime));
if (!messageManuallyMarkedUnread) {
shouldDisplay = shouldDisplay && reportAction.actorAccountID !== Report.getCurrentUserAccountID();
}
} else {
shouldDisplay = reportAction.reportActionID === currentUnreadMarker;
}
return shouldDisplay;

The ReportActionsList.js component does not re-render when navigating back from other reports, thus currentUnreadMarker will not be initialized to null but remain equal to reportAction.reportActionID. Therefore, the root cause of the problem is that the logic in the shouldDisplayNewMarker() function fails to consider if the message has been viewed before when currentUnreadMarker is not null.

if (!markerFound) {
setCurrentUnreadMarker(null);
}

Hence, when the currentUnreadMarker has a value that corresponds to the reportAction.reportActionID, the shouldDisplayNewMarker() function returns true and markerFound will become true. Thus, the setCurrentUnreadMarker(null) line is not called.

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

We should fix the logic in the shouldDisplayNewMarker() function to return false in this situation. In addition to checking if the currentUnreadMarker is reportAction.reportActionID, considering if the message has been read will ensure markerFound remains false. Consequentially, the currentUnreadMarker will be set to null.

shouldDisplay = reportAction.reportActionID === currentUnreadMarker;

Therefore, slightly editing the code above to something similar to this should prohibit the unread line from appearing when navigating back to the report.

shouldDisplay = reportAction.reportActionID === currentUnreadMarker && isMessageUnread(reportAction, report.lastReadTime);

@melvin-bot melvin-bot bot added the Overdue label Nov 9, 2023
@Ollyws
Copy link
Contributor

Ollyws commented Nov 13, 2023

Will have a look at this one today.

@melvin-bot melvin-bot bot removed the Overdue label Nov 13, 2023
@Ollyws
Copy link
Contributor

Ollyws commented Nov 14, 2023

@FitseTLT

But in your case the marker will display for a sec and it disappears when lastReadTime is changed.

This only seems to apply if we add currentUnreadMarker as a dependency of shouldDisplayNewMarker(), which isn't what @s-alves10 is suggesting as far as I can see. (Although the linter does suggest we add that or disable the exhaustive-deps rule)

Copy link

melvin-bot bot commented Nov 14, 2023

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

@FitseTLT
Copy link
Contributor

This only seems to apply if we add currentUnreadMarker as a dependency of shouldDisplayNewMarker()

No @Ollyws it applies without adding currentUnreadMarker as a dependency. The reason the marker disappears after a second is : it appears first when the report action list is first rendered in which case the message will be regarded as new but immediately the lastReadTime is updated and the useEffect causes the re-execution of shouldDisplayNewMarker but now it is suppose to be determined by currentUnreadMarker but because his proposal removes the most important line of code

shouldDisplay = reportAction.reportActionID === currentUnreadMarker;

it will result in the removal of the new marker.
Generally, the illusion this proposal is making to seem like it is solving the issue is b/c it is breaking an already working logic of the component to sustain the marker as long as the user is on that screen which depends on the logic in shouldDisplayNewMarker that sets shouldDisplay true for the report action which is the currentUnreadMarker (whenever currentUnreadMarker is not null).

@MonilBhavsar MonilBhavsar self-assigned this Nov 16, 2023
@MonilBhavsar
Copy link
Contributor

Self assigning this issue

@MonilBhavsar
Copy link
Contributor

Putting this on HOLD for #27456

@MonilBhavsar MonilBhavsar changed the title [$500] Chat - Inconsistency in displaying unread green line when navigating from thread [HOLD 27456][$500] Chat - Inconsistency in displaying unread green line when navigating from thread Nov 16, 2023
@melvin-bot melvin-bot bot added the Overdue label Nov 20, 2023
Copy link

melvin-bot bot commented Nov 20, 2023

@JmillsExpensify, @Ollyws, @MonilBhavsar Whoops! This issue is 2 days overdue. Let's get this updated quick!

@MonilBhavsar
Copy link
Contributor

Still on hold, bumping to weekly

@melvin-bot melvin-bot bot removed the Overdue label Nov 21, 2023
@mvtglobally
Copy link

Issue not reproducible during KI retests. (Second week)

@melvin-bot melvin-bot bot added the Overdue label Apr 4, 2024
Copy link

melvin-bot bot commented Apr 4, 2024

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

@FitseTLT
Copy link
Contributor

FitseTLT commented Apr 4, 2024

It is still reproducible @mvtglobally Please follow the steps and video in OP. You have to reply in thread -> click on subtitle twice

2024-04-04.20-52-44.mp4

@MonilBhavsar MonilBhavsar added Needs Reproduction Reproducible steps needed retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause labels Apr 5, 2024
@MelvinBot
Copy link

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

@MonilBhavsar
Copy link
Contributor

Needs to be reproduced and confirmed by QA team.

@melvin-bot melvin-bot bot removed the Overdue label Apr 8, 2024
Copy link

melvin-bot bot commented Apr 11, 2024

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

@mvtglobally
Copy link

Issue is reproducible during KI retests.

1712759104485.0-02-01-ec6d3a4ef5057233e15b03f52bf53a1cc48c2cb51ea54f62db2b79d6cdc16638_7a4fa2c97dc8e2ed.mp4

@Ollyws
Copy link
Contributor

Ollyws commented Apr 12, 2024

Still reproducible.
@FitseTLT's proposal LGTM.
🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Apr 12, 2024

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

@MonilBhavsar
Copy link
Contributor

👍 Let's test it extensively to ensure we don't introduce any regression since it refactors core logic.
Here are the test cases we can test https://github.com/Expensify/Expensify/issues/378297#issue-2183153558

@MonilBhavsar MonilBhavsar removed Needs Reproduction Reproducible steps needed retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause labels Apr 12, 2024
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 12, 2024
Copy link

melvin-bot bot commented Apr 12, 2024

📣 @Ollyws 🎉 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

Copy link

melvin-bot bot commented Apr 12, 2024

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

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@FitseTLT
Copy link
Contributor

@MonilBhavsar We don't have access to it 😢

@MonilBhavsar
Copy link
Contributor

MonilBhavsar commented Apr 12, 2024

Oops sorry, Pasting it here

Unread tests
  1. Different chat open:
  • Given a chat report between user A and user B, and user A and user C, and user A has chat view with user B open
  • When user C sends message to user A, and user A switches to chat view with user C
  • Then user A should see the new marker line above the new messages, and the new marker line should persist until userA switches to another chat view.
  1. Same chat open(no unread message):
  • Given a chat report between user A and user B, and user A has chat view with user B open and no unread message and no unread marker
  • When user B sends message to user A
  • Then user A should not see any new marker line
  1. Same chat open(with unread message):
  • Given a chat report between user A and user B, and user A has chat view with user B open and user A has unread messages and new marker line
  • When user B sends message to user A
  • Then the new marker line should persist until userA switches to another chat view.
  1. Same chat open(with user scrolled up):
  • Given a chat report between user A and user B, and user A has chat view with user B open and has no unread messages and no new marker line
  • When user A scrolls up to the old chats and user B sends message to user A
  • Then user A should see the green badge with “New Messages” and the new marker line above the new messages, and the new marker line should persist until userA switches to another chat view.
  1. Redirecting to chat from email notification:
  • Given a chat report between user A and user B, and user A is not active on chat or offline
  • When user B sends message to user A and user A receives email notification and clicks on the link in the message
  • Then user A should be redirected to the chat view with user B and see the new marker line above the new messages, and the new marker line should persist until userA switches to another chat view.
  1. Marking message as unread:
  • Given a chat report between user A and user B, and user A is active on a chat view with user B
  • When user A marks any report as unread i.e. click on “Mark as unread”
  • Then user A should see the new marker line above that message, and it won’t be removed until the user revisits the chat and switches to another chat then.
  1. Deleting a message(other than unread message):
  • Given a chat report between user A and user B, and user A is active on a chat view with user B, and there is an unread message and unread marker
  • When user B sends a new message and deletes that message (other then the message above which there is an unread marker)
  • Then there should be no effect on the new marker line and user A should see the new marker line, and it won’t be removed until the user switches to another chat or the sender deletes all new messages.
  1. Deleting an unread message:
  • Given a chat report between user A and user B, and user A is active on a chat view with user B, and there is an unread message and unread marker
  • When user B deletes a message above which there is an unread marker
  • Then user A should should not see the unread marker and the new message that was deleted
  1. Marking as unread and deleting a message
  • Given a chat report between user A and user B
  • User A sends message to user B and marks the last sent message as unread
  • User A deletes the last sent message
  • Then the new marker line above last message should be removed
  • Now user A, sends three messages to user B - m1, m2, and m3 in order
  • User A marks m2 as unread
  • Then New marker line should be above m2
  • Now user A deletes m2
  • Then new marker line should be above next message i.e m3 in this case

@FitseTLT
Copy link
Contributor

Thx

@melvin-bot melvin-bot bot added the Overdue label Apr 22, 2024
@MonilBhavsar
Copy link
Contributor

@FitseTLT what's the ETA for the PR?

@melvin-bot melvin-bot bot removed the Overdue label Apr 23, 2024
@FitseTLT
Copy link
Contributor

@MonilBhavsar After #40315 now the subtitle link will goBack instead of navigate So this bug is not reproducible anymore as we can't have two report screen with the same report id simultaneously by using subtitle link.

@MonilBhavsar
Copy link
Contributor

Okay, thanks for clarifying. Let's reopen if reproducible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
No open projects
Development

No branches or pull requests

10 participants