HIGH: [Reliability] [$1000] Chat - Chat does not scroll down when sending messages

[kbecciv]

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: 1.4.64-0 Reproducible in staging?: y Reproducible in production?: y Issue reported by: Applause - Internal Team

Action Performed:

1. Launch New Expensify app.
2. Go to empty chat.
3. Send a text.

Expected Result:

The chat view will scroll down to show the latest message.

Actual Result:

The chat view does not scroll down to show the latest message.

Workaround:

n/a

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [x] Android: Native
• [ ] Android: mWeb Chrome
• [x] iOS: Native
• [ ] iOS: mWeb Safari
• [ ] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/93399543/0006cc82-9880-42b6-8543-6c48e605ea0f

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016dcc2d8605cc9198
• Upwork Job ID: 1782503029072748544
• Last Price Increase: 2024-05-16
• Automatic offers:
• DylanDylann | Reviewer | 0
• ikevin127 | Contributor | 0

[melvin-bot[bot]]

Triggered auto assignment to @luacmartins (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[kbecciv]

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

[github-actions[bot]]

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[mountiny]

Looking for proposals

[luacmartins]

Gonna look into this one

[mountiny]

it might be related to https://github.com/Expensify/App/pull/40547

[climax0916]

I found the issue, and looking for possible solutions.

[luacmartins]

Cool, let us know what you find @climax0916

[climax0916]

When adding comments, shouldEnableAutoScroll turns to False so ReportActionsList doesn't scroll to the latest message.

https://github.com/Expensify/App/blob/a43595b99ac4d8451b6cc147ae007315dce481da/src/pages/home/report/ReportActionsView.tsx#L488-L512

[climax0916]

@luacmartins Am I right?

[luacmartins]

Why does it become false?

[climax0916]

When adding new transactions, this API.write() function is called: https://github.com/Expensify/App/blob/a43595b99ac4d8451b6cc147ae007315dce481da/src/libs/actions/Report.ts#L604-L608

Then, Onyx.update(optimisticData) is called for update Onyx storage: https://github.com/Expensify/App/blob/a43595b99ac4d8451b6cc147ae007315dce481da/src/libs/API/index.ts#L56-L63

After that, render again because of Onyx storage changed. But, here hasNewestReportAction will be False: https://github.com/Expensify/App/blob/a43595b99ac4d8451b6cc147ae007315dce481da/src/pages/home/report/ReportActionsView.tsx#L235

Because of this, the scroll doesn't down to the latest message.

[DylanDylann]

I think this issue have the same RCA with https://github.com/Expensify/App/issues/40664

[ikevin127]

Proposal

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

Chat does not scroll down when sending messages.

What is the root cause of that problem?

Two things which both cause our issue:

1.

Whenever we start a new chat there's this new action which I assume was introduced by BE recently: an additional new ADDCOMMENT action comes in right after the "CREATED" action, which is an automated message (one in the middle):

This addition causes hasNewestReportAction to return false since sortedReportActions?.[0] is now the automated ADDCOMMENT action (first in sortedReportActions) instead of the user posted message like it used to be -> meaning that the the automated message's created date is not equal to the report's lastVisibleActionCreated date because the lastVisibleActionCreated date equals to the sortedReportActions?.[1] at index 1 (user's comment) hence the condition returns false.

This makes it such that the scrollToBottomForCurrentUserAction returns early because of the !hasNewestReportActionRef.current check being true.

2.

Due to recent new architecture being enabled, it looks like the native specific scrollToBottom function does not trigger the scrolling anymore even though it's called and this happens because there's a few things going on when we post a comment from JS's point of view and because scrollToBottom gets called while the stack is ongoing (not cleared yet), the function does not perform it's logic as expected.

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

We have a two part solution to address both parts of the RCA:

1.

The first part of the RCA was already handled by PR https://github.com/Expensify/App/pull/40286, specifically this line where we make sure we don't early return when the user posts their first comment when the first action is a whisper (automated comment).

2. In order to have the scrollToBottom function work as expected, similar to what I proposed in issue https://github.com/Expensify/App/issues/38855 and was selected / approved -> use deferred execution via requestAnimationFrame to wrap the contents of the function like so:

    const scrollToBottom = useCallback(() => {
        requestAnimationFrame(() => {
            if (!flatListRef?.current) {
                return;
            }

            setScrollPosition({offset: 0});

            flatListRef.current?.scrollToOffset({animated: false, offset: 0});
        });
    }, [flatListRef, setScrollPosition]);

This ensures that the code within scrollToBottom will be executed on the next tick, essentially telling the JavaScript runtime to execute this code after the current call stack has cleared.

Here's a test branch of the solution: test/40724 and it's diff.

Videos (before / after)

iOS: Native

| Before | After | | ------------- | ------------- | |

[mountiny]

Based on the previous comments this seems like it might not be a deploy blocker as if its related to the new architecture, that has already been deployed to production

[mountiny]

It seems like from the analysis that its a mix of new architecture and backend changes which causes this bug.

I am not sure how we decided to handle the whisper messages but it seems like they both have the same previous report action ID

and that might be causing the comment linking code to not be happy

[melvin-bot[bot]]

Current assignee @luacmartins is eligible for the DeployBlockerCash assigner, not assigning anyone new.

[github-actions[bot]]

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[mountiny]

making a build on the linked PR

@ikevin127 I can reproduce this in old chats too

[DylanDylann]

This addition causes hasNewestReportAction to return false since sortedReportActions?.[0] is now the automated ADDCOMMENT action (first in sortedReportActions) instead of the user posted message like it used to be

@ikevin127 As I understand sortedReportActions?.[0] will be the latest comment then it should be user's comment (based on created field). Could you help to explain your point sortedReportActions?.[0] is now the automated ADDCOMMENT action

[DylanDylann]

You mean that when scrollToBottomForCurrentUserAction function is triggered, the new comment has not been added yet to sortedReportActions

[luacmartins]

It seems like the changes to scrollToBottom are enough to fix this issue. I'm not sure that I understand why the whisper message would be contributing to it.

[luacmartins]

As @mountiny pointed out, it seems like we have two actions pointing to the same previousReportActionID which might mess up the logic for comment linking, but I don't think contributes to the scrolling issue

[luacmartins]

@ikevin127 are you available to work on a fix with the solution for 2?

[melvin-bot[bot]]

📣 @DylanDylann 🎉 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[bot]]

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

[mountiny]

This seems to be not be fully fixed for me. It was on the adhoc build, but now in the 1.4.65-0 staging I can see a similar behaviour @ikevin127 can you please follow up with a fix as it was not completely fixed

https://github.com/Expensify/App/assets/36083550/1e3054c1-3e22-445c-909d-2aab435990fb

[parasharrajat]

We didn't test it against latest main so there might be some issue.

[ikevin127]

@ikevin127 As I understand sortedReportActions?.[0] will be the latest comment then it should be user's comment (based on created field). Could you help to explain your point sortedReportActions?.[0] is now the automated ADDCOMMENT action

@DylanDylann When opening a new chat with somebody, sortedReportActions contains report actions of actionName types CREATED and ADDCOMMENT. Since this new type: 'automated' action was added, now there are 2 ADDCOMMENT of which the first one (sortedReportActions?.[0]) is now the automated ADDCOMMENT action instead of the comment the user just posted.

Edit: Actually this specific line from PR https://github.com/Expensify/App/pull/40286 handles this, as instead of sorting out all whisper (automated) messages, it would just check that the first one is not an automated one and won't return early anymore, preventing the scroll but instead would scroll even if the first message is a whisper (automated).

@ikevin127 are you available to work on a fix with the solution for 2?

@luacmartins Yes! I have to mention that PR https://github.com/Expensify/App/pull/40286 which was merged in hopes to fix this issue, was expected that it would not fix this issue on native platforms because the requestAnimationFrame is only applied in the index.ts variant of the scrollToBottom, not on the index.native.ts one.

I noticed that you edited my PR's description and mentioned that it also fixes this issue, I edited it back and removed that since the PR code does not touch on the index.native.ts part of this issue, hence it was not meant to fix this issue fully.

This seems to be not be fully fixed for me. It was on the adhoc build, but now in the 1.4.65-0 staging I can see a similar behaviour @ikevin127 can you please follow up with a fix as it was not completely fixed

@mountiny As explained above, PR https://github.com/Expensify/App/pull/40286 was not meant to fix this issue on native platforms. I think there was a misunderstanding here where because the issues are similar in terms of the scrolling-related bug, the root cause is slightly different - the main thing that was omitted when the PR was merged is the index.native.ts part of the requestAnimationFrame fix of the scrollToBottom function.

We shouldn't consider this a regression PR https://github.com/Expensify/App/pull/40286 since at the time of the PR and before the new arch was merged, the fix worked as expected on all platforms. The reason why this issue wasn't fixed by PR https://github.com/Expensify/App/pull/40286, is because we omitted adding the requestAnimationFrame fix within the index.native.ts to the scrollToBottom function.

We didn't test it against latest main so there might be some issue.

@parasharrajat Makes sense here as this issue (#40724) will still be reproduced regardless of PR https://github.com/Expensify/App/pull/40286 being merged since the PR was only targeting the index.ts when the requestAnimationFrame fix was added as at the time (before new arch was merged) there were no deferred execution issues on native.

Edit: additional (in depth) discussion regarding recent events can be found at https://github.com/Expensify/App/pull/40286#issuecomment-2075718632.

[parasharrajat]

Ok, Thanks for the explanation. In any case, we should create a new PR to fix this issue.

[ikevin127]

I found the discrepancy between PR https://github.com/Expensify/App/pull/40286 and this issue's fix, there are two parts to it:

1. This specific line of PR https://github.com/Expensify/App/pull/40286, which is currently on main covers for the RCA problem number (1.) of proposal by making sure that the scrolling happens even if the first message is a whisper (automated) when the user posts their first comment.

✅ So this part of the PR is correct and handles proposal's solution part (1.) more elegantly, instead of filtering out all whisper actions which would be bad as the whisper messages wouldn't show up anymore in reports.

2. Part two of the proposal's solution (2.) was not applied yesterday before the PR was merged, specifically for index.native.ts (native only).

⚠️ This is why this issue is not fixed currently, even though the first part of the proposal's solution (1.) was handled by PR https://github.com/Expensify/App/pull/40286, the second part was not.

Therefore the only thing left to do here in order to have this issue fixed and closed is to make sure we wrap the index.native.ts's scrollToBottom function with requestAnimationFrame as well like we did in the index.ts file.

Updated proposal to reflect the latest developments detailed above.

I'll open a PR for this issue where I will only implement the part (2.) of the proposal's solution.

cc @mountiny @parasharrajat @luacmartins

[mountiny]

Thanks Kevin!

[ikevin127]

[!important] I applied requestAnimationFrame on native side and while it worked on iOS: Native, I noticed that it did not work on Android: Native on the first message (this issue) and also it did not work 100% reliably on subsequential messages (after the first one).

Not sure where to go from here regarding the behaviour on Android: Native, I have the sense that it might be new arch related but no proof so far regarding a root cause.

I even tried setTimeout with 300ms delay and it still does not work meaning it's not related to JS stack execution, this is why I'm assuming something's up natively on Android since new arch was enabled, regarding the scrollTo function.

I could split the index.native.ts file in index.android.ts and index.ios.ts and add the fix for iOS only - which would still leave us with the bug on Android.

cc @mountiny

[ikevin127]

Looks like the requestAnimationFrame solution created a regression here:

• https://github.com/Expensify/App/issues/40911

Therefore we should find alternative solutions / proposals for fixing this issue as well since it was suggested that we revert PR https://github.com/Expensify/App/pull/40286, here https://github.com/Expensify/App/issues/38855#issuecomment-2078080833.

[melvin-bot[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[bot]]

@luacmartins, @parasharrajat, @ikevin127, @DylanDylann Whoops! This issue is 2 days overdue. Let's get this updated quick!

[luacmartins]

@ikevin127 any luck finding a solution for this? Should we revert the original PR?

[ikevin127]

@luacmartins I don't have a solution for this issue, I tried applying defer execution to the scrollToBottom and this only solves the problem on iOS: Native, the issue persists on Android: Native.

Regarding PR https://github.com/Expensify/App/pull/40286, the only solution that works and would keep the original PR's fix and solve issue https://github.com/Expensify/App/issues/40911 would be to replace original PR's requestAnimationFrame with a 0 delay setTimeout. This is because we need to defer execution and requestAnimationFrame is problematic in the case of pressing the New message button.

[luacmartins]

It seems like we reverted the original PR. Let's keep exploring solutions then.

[melvin-bot[bot]]

@luacmartins, @parasharrajat, @ikevin127, @DylanDylann Whoops! This issue is 2 days overdue. Let's get this updated quick!

[parasharrajat]

We need a new proposal for this issue. No contributor is assigned. Also, looks like two C+ is assigned here.

[melvin-bot[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.

[anmurali]

Should we bump up the price to solve this problem?

[melvin-bot[bot]]

Upwork job price has been updated to $500

[luacmartins]

Gonna bump the price on this again.

[melvin-bot[bot]]

Upwork job price has been updated to $1000

[mountiny]

@ikevin127 any more ideas here from your side?

[ikevin127]

I don't have a solution for this issue, I tried applying defer execution to the scrollToBottom and this only solves the problem on iOS: Native, the issue persists on Android: Native.

This is because we need to defer execution and requestAnimationFrame is problematic in the case of pressing the New message button.

@mountiny So far, I stand by what I said in an earlier comment. I was planning to look deeper into this one but didn't get to it yet.

So far I went as deep as looking into the native flatlist scrolling function on Android: Native side, following it down from our scrollToBottom, but everything looks fine there.

Therefore, in no particular order I'd say, if we were to go with the 0 delay setTimeout for deferred execution, my take as to why Android: Native still doesn't work could be:

• some conflicting scrolling logic in our report actions list component which cancels out the scrolling
• new architecture related (found some changes to the native scrolling functions introduced with new arch, like if new_arch then return scrolling function which looks a bit different than the old way)

I'll keep digging and let you know in case I find anything.