[$250][HOLD #49825][HOLD #49824] Chat - Chat is not autoscrolled to the bottom when receiving whisper after a mention

[lanitochka17]

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.39.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: N/A
Issue reported by: Applause - Internal Team

Action Performed:

1. Open the staging.new.expensify.com website
2. Open any room chat in inbox
3. Mention another user that is not a member of that room
4. Verify that a whisper is received afrer sending the mention, and the chat is autoscrolled to the bottom

Expected Result:

After mentioning another user that is not part of the chat, a whisper with options should be received, and the chat should be autoscrolled to the bottom

Actual Result:

When the whisper is received, after mentioning an user that is not part of the room, the chat is not autoscrolled to the bottom

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

Bug6612450_1727095488272.Whisper_Scroll.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021838383119719075523
• Upwork Job ID: 1838383119719075523
• Last Price Increase: 2024-09-24
• Automatic offers:
• paultsimura | Reviewer | 104109892
• klajdipaja | Contributor | 104109896

Issue Owner

Current Issue Owner: @dangrous

[melvin-bot]

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

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

[RachCHopkins]

Fine on web/desktop.

Same on iOS native app. Not just android/web, my guess is that it's anything with a mobile aspect ratio.

[melvin-bot]

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

[melvin-bot]

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

[klajdipaja]

Edited by proposal-police: This proposal was edited at 2024-09-27 08:30:08 UTC.

Proposal

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

When a whisper is received in the chat for the current user the screen is not scrolled down and the whisper is not visible until the user decides to scroll down.

What is the root cause of that problem?

The cause of the issue seems to be an intented behaviour in the logic of the scrolling for new actions. The current logic scrolls the user to the bottom if the action is from the current user, all other cases no scrolling is done. This logic is in ReportActionsList.tsx in this code block:

    const scrollToBottomForCurrentUserAction = useCallback(
        (isFromCurrentUser: boolean) => {
            // If a new comment is added and it's from the current user scroll to the bottom otherwise leave the user positioned where
            // they are now in the list.
            if (!isFromCurrentUser) {
                return;
            }
            if (!hasNewestReportActionRef.current) {
                Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.reportID));
                return;
            }
            InteractionManager.runAfterInteractions(() => reportScrollManager.scrollToBottom());
        },
        [reportScrollManager, report.reportID],
    );

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

We have a few options depending on how the scrolling is intented to behave;

1. Always scroll down for all new actions not just for current user messages. This is a very easy fix, we just remove the !isFromCurrentUser check.
2. Scroll down only for messages from the current user and whispers of the current user. First part is the existing logic, we need to add another code block that would check if the action is a whisper for the current user.
   After the isFromCurrentUser check would be replaced by the following block:

 const newestAction = sortedVisibleReportActions[0];
            const whisperedTo = ReportActionsUtils.getWhisperedTo(newestAction);

            const transactionsWithReceipts = ReportUtils.getTransactionsWithReceipts(report.reportID);
            const isWhisper = whisperedTo.length > 0 && transactionsWithReceipts.length === 0;
            const isWhisperOnlyVisibleByUser = isWhisper && ReportUtils.isCurrentUserTheOnlyParticipant(whisperedTo);
          
             const shouldScrollDown = isFromCurrentUser || isWhisperOnlyVisibleByUser;
            if (!shouldScrollDown) {
                return;
            }

           

3. Option 2 does not work when the device or internet is slow because it triggers an auto scroll when the action is created, i.e when the user sends the message, but the whisper itself comes as another action a later point from the BE.

We need to change the approach to check the latest action in the chat and check if that is a whisper for the current user or not by changing this existing useEffect to this:

    useEffect(() => {
        const isWhisperForUser = isWhisperVisibleOnlyToUser(sortedVisibleReportActions[0])
        const shouldScrollForWhisper = isWhisperForUser && hasNewestReportAction;

        if (
            (scrollingVerticalOffset.current < AUTOSCROLL_TO_TOP_THRESHOLD &&
            previousLastIndex.current !== lastActionIndex &&
            reportActionSize.current > sortedVisibleReportActions.length &&
            hasNewestReportAction) || shouldScrollForWhisper
        ) {
            reportScrollManager.scrollToBottom();
        }
        previousLastIndex.current = lastActionIndex;
        reportActionSize.current = sortedVisibleReportActions.length;
    }, [lastActionIndex, sortedVisibleReportActions, reportScrollManager, hasNewestReportAction, linkedReportActionID]);

In addition we need to fix how the hasNewestReportAction is calculated by replacing hasNewestReportAction = sortedVisibleReportActions[0]?.created === report.lastVisibleActionCreated with:
hasNewestReportAction = sortedVisibleReportActions[0]?.created >= report.lastVisibleActionCreated. Up to this point we had assumed that the sortedVisibleReportActions[0]?.created is always equal to report.lastVisibleActionCreated but for some reason the optimistic data are a bit behind the actual BE.

What alternative solutions did you explore? (Optional)

[paultsimura]

@klajdipaja is correct – this looks intentional at the moment. However, it seems reasonable to also enable auto-scroll on the whispers as they are system messages addressed directly to the user and often require some action/attention.

🎀👀🎀 C+ reviewed to summon an Engineer for decision.

[melvin-bot]

Triggered auto assignment to @blimpich, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot]

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

[melvin-bot]

📣 @klajdipaja 🎉 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 📖

[blimpich]

Agreed, lets got with solution 2 outlined in the proposal.

If possible, can we reach out the engineers who implemented this code originally to make sure they're aware of the changes we're making?

[paultsimura]

@marcaaron @Julesssss I see you were involved in #536, where this comment appeared for the first time:

If a new comment is added and it's from the current user scroll to the bottom

Just a heads-up: we plan to add auto-scroll to the bottom when the user receives a whisper message.
The reasoning: these are system messages addressed directly to the user and often require some action/attention. It will help draw the user's attention because currently you barely see that the new actionable message was received:

2024-09-25.-.12.37.-.Screen.Recording.2024-09-25.at.12.36.44.mp4

[klajdipaja]

@paultsimura While testing my changes in android emulator I found out that there's an issue with my proposal. When the internet or device is really slow I saw that the scrolling does not happen.
The root cause of that is that the scrollToBottomForCurrentUserAction is called as a callback after the API.write for the action that the user has executed but if the internet is slow the whisper message has not arrived yet.
We need to change the approach to check the latest action in the chat and check if that is a whisper for the current user or not by changing this existing useEffect to this:

    useEffect(() => {
        const isWhisperForUser = isWhisperVisibleOnlyToUser(sortedVisibleReportActions[0])
        const shouldScrollForWhisper = isWhisperForUser && hasNewestReportAction;

        if (
            (scrollingVerticalOffset.current < AUTOSCROLL_TO_TOP_THRESHOLD &&
            previousLastIndex.current !== lastActionIndex &&
            reportActionSize.current > sortedVisibleReportActions.length &&
            hasNewestReportAction) || shouldScrollForWhisper
        ) {
            reportScrollManager.scrollToBottom();
        }
        previousLastIndex.current = lastActionIndex;
        reportActionSize.current = sortedVisibleReportActions.length;
    }, [lastActionIndex, sortedVisibleReportActions, reportScrollManager, hasNewestReportAction, linkedReportActionID]);

This is testing really well, you can check this out in this branch and let me know what you think

[paultsimura]

@klajdipaja please make a draft PR and tag me there. It's easier to work with

[paultsimura]

I've played with these ACTIONABLE_WHISPER messages a little, and they look quite buggy. I will report separate bugs, and we may want to hold this one for them.

Duplicating the bugs here for clarity:

Bug 1:

1. Create a room;
2. Mention a user that is not a member of this room using the @{user} syntax;
3. Notice the actionable whisper "Heads up, {user} isn't a member of this room" message appears;
4. Verify the "New messages" floating button appears;
5. Scroll up a little;
6. Click the "New messages" floating button;

Expected: The report is scrolled to the bottom so the whisper action becomes visible.
Actual: The report is not scrolled.

Bug 2:

1. Create a room;
2. Mention a user that is not a member of this room using the @{user} syntax;
3. Notice the actionable whisper "Heads up, {user} isn't a member of this room" message appears;
4. Click "Do nothing"
5. Verify the report's last message in LHN changes to the last visible message – the @{user} mention

Expected: The "New messages" floating button should not appear as there is no new message
Actual: The "New messages" floating button appears

6. Refresh the page
7. Check the report's last message in LHN

Expected: The report's last message in LHN should remain @{user}
Actual: The report's last message in LHN changes to Expensify: Heads up...

2024-09-25.-.19.11.-.Screen.Recording.2024-09-25.at.19.10.18.mp4

[klajdipaja]

@paultsimura did you check those with the draft PR #49699 I created? I have a feeling it will solve both issues but haven't had the time to double check

[paultsimura]

@klajdipaja no it doesn't fix those. From what I see, is doesn't fix the current one either.

[dangrous]

Will do! Still confirming expected behavior - whispers are weird, haha

[dangrous]

I have a potential fix up, will fully test tomorrow and then hopefully have it up for review! It'll actually be a separate fix from that other issue, but that's okay.

[RachCHopkins]

How this going @dangrous?

[dangrous]

Backend is in review! Should hopefully be merged soon and then we can move forward here

[dangrous]

This should hopefully be fixed on the backend. Can we retest and figure out if anything else is needed?

[paultsimura]

@dangrous something's different now (some cases were fixed I believe), but the issue is still reproducible (recording from staging):

2024-11-14.-.22.42.-.Screen.Recording.2024-11-14.at.22.42.07.mp4

[paultsimura]

IMO, we should remove [HOLD #49825], keep [HOLD #49824] and re-visit once that one is done as well.

[paultsimura]

Also @dangrous could you please check this? Looks like another BE issue:

1. In a room, send a mention of a user that is not a member of the room
2. Verify the actionable whisper was added
3. Repeat this multiple times
4. Verify only the last mention has an actionable whisper
5. Refresh the page
6. See now each mention has its own actionable whisper.

I'm unsure what is expected here: to have a whisper for each message or the last one only. But this is inconsistent.

2024-11-14.-.22.56.-.Screen.Recording.2024-11-14.at.22.56.19.mp4

[dangrous]

so for the first one I think that might be expected? What's the behavior if someone sends a regular message when you're scrolled up in the chat, like you are? I don't think you should be rescrolled to the bottom, right?

For the second one - is that new behavior, or was it like that before too? It definitely seems weird to have them all pop back up like that - but I'm not sure why they disappeared in the first place. This would probably be an edge case since you'd be unlikely to keep inviting the same person over and over again like that, but still worth looking into. Is it the front end that's removing the previous whispers (maybe optimistically?) or the backend?

[paultsimura]

so for #49599 (comment) I think that might be expected? What's the behavior if someone sends a regular message when you're scrolled up in the chat, like you are? I don't think you should be rescrolled to the bottom, right?

You're right 👍 This GH seems to be resolved now.

How do we act if the Contributor has already been hired here but the PR has not been opened because we had to hold? Do we just close the issue?

[paultsimura]

For the second one

Apparently, we had a GH for it that got closed due to being a non-priority. You could take a look there, it was diagnosed as a BE issue: #47255

[dangrous]

Okay great, then I think we're good to close this! Since no PR raised, I don't think we need payment here but if anyone disagrees let me know. Thanks!

[klajdipaja]

@dangrous @paultsimura what should be done with the Upwork contract in this case?

[paultsimura]

@klajdipaja we can just close it by ourselves. Specify "the work is no longer needed" as the reason.

[dangrous]

actually, I've been informed that we pay 50% in this case (contributor hired but no PR needed)! @RachCHopkins can you help us out?

[RachCHopkins]

Payment Summary:

• Contributor: @klajdipaja to be paid $125 via Upwork
• Contributor+: @paultsimura to paid $125 via Upwork

Upwork job here

[paultsimura]

Thanks.
@RachCHopkins what is the payment date supposed to be here?

[dangrous]

I think we can pay whenever, since it was only backend we're not waiting on a regression period cc @RachCHopkins

[RachCHopkins]

@paultsimura it's ASAP! I'm just getting my ducks in a row! 🦆 🦆🦆

[RachCHopkins]

Contributors have been paid, the contracts have been completed, and the Upwork post has been closed.