[$250] Self DM - Chat is not scrolled to the bottom when clicking 'New message'

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

Action Performed:

1. Navigate to a self DM report
2. Send some messages
3. Scroll to bottom
4. Mark unread the last message
5. Scroll up
6. Click on 'New message' button

Expected Result:

Chat should be scrolled to the latest message

Actual Result:

Chat is not scrolled to the latest message

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

Bug6460218_1713968475100.Screen_Recording_2024-04-24_at_5.20.32_in_the_afternoon.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0199f7298343d8bc01
• Upwork Job ID: 1783328974086828032
• Last Price Increase: 2024-04-25

[melvin-bot]

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

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

[amyevans]

Heads up I don't think this bug is exclusive to the Self-DM.

[melvin-bot]

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

[melvin-bot]

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

[abzokhattab]

Proposal

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

Chat is not scrolled to the bottom when clicking 'New message'

What is the root cause of that problem?

the following PR has introduced the current issue, where when the user clicks on new message it doesn't scroll to the bottom

• when the user clicks on the new message button, the scrollToBottomAndMarkReportAsRead is called and the reportScrollManager.scrollToBottom then executes
• the requestAnimationFrame inside the scrollToBottom is the cause of the issue

  App/src/hooks/useReportScrollManager/index.ts

  Lines 23 to 32 in ece28d0

  	const scrollToBottom = useCallback(() => {
  	// We're deferring execution here because on iOS: mWeb (WebKit based browsers)
  	// scrollToOffset method doesn't work unless called on the next tick
  	requestAnimationFrame(() => {
  	if (!flatListRef?.current) {
  	return;
  	}
  	flatListRef.current.scrollToOffset({animated: false, offset: 0});
  	});

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

• we can either revert the requestAnimationFrame change introduced in the previous PR
• Or we can create another function that doesn't use the requestAnimationFrame and return it here and then use this new function inside the scrollToBottomAndMarkReportAsRead instead of the scrollToBottom:

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

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

    const scrollToBottom = useCallback(() => {
        requestAnimationFrame(() => {
            scrollToBottomWithoutDelay();
        });
    }, [scrollToBottomWithoutDelay]);

[tienifr]

Proposal

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

Chat is not scrolled to the latest message

What is the root cause of that problem?

We're executing scrollToOffset inside requestAnimationFrame.

From the documentation of requestAnimationFrame

The window.requestAnimationFrame() method tells the browser you wish to perform an animation.

That means an animation is executed along with scrollToOffset that cause the scrollToOffset stop

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

To make sure scrollToOffset is called after that animation we can use setTimeout (The callback function of setTimeout will be triggered later)

            setTimeout(()=>{
                flatListRef.current.scrollToOffset({animated: false, offset: 0});
            },0)

What alternative solutions did you explore? (Optional)

We can use InteractionManager.runAfterInteractions to wait for animations end

[kaushiktd]

Proposal

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

Self DM - Chat is not scrolled to the bottom when clicking 'New message'

What is the root cause of that problem?

requestAnimationFrame inside scrollToBottom callback cause the main issue when user tap on new message button here.
If there are other operations or animations like requestAnimationFrame happening simultaneously, they might interfere with the scrolling operation.

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

You need to introduce delays between animations to prevent them from overlapping. You can use setTimeout to introduce delays in the animation sequence here.

Update the scrollToBottom function as mention below:

const scrollToBottom = useCallback(() => {
    // We're deferring execution here because on iOS: mWeb (WebKit based browsers)
    // scrollToOffset method doesn't work unless called on the next tick
    requestAnimationFrame(() => {
        setTimeout(() => {
            if (!flatListRef?.current) {
                return;
            }
            flatListRef.current.scrollToOffset({ animated: false, offset: 0 });
        }, 0);
    });
}, [flatListRef]);

In the specific context of scrollToBottom function, combining requestAnimationFrame with setTimeout(() => { ... }, 0) ensures that the scrolling operation occurs after the current stack has cleared, which can help prevent potential issues related to timing and synchronization with the rendering pipeline.

Video:

https://app.screencastify.com/v3/watch/g1QVXQfNszwp7Yp8kXEK

[dukenv0307]

Will review proposals in a hour

[dukenv0307]

Thanks for all your proposals

@abzokhattab requestAnimationFrame is added on purpose so I don't think we should remove it
@tienifr your main solution works well but setTimeout is not really a good idea. We try to avoid this method, so I'm thinking about your alternative solution, but it's quite slow

Your RCAs don't convince me enough. requestAnimationFrame can cause the animation but we already set animated: false in scrollToOffset -> scrollToOffset should be run without any animations

-> Why is scrollToOffset interrupted?

@ikevin127 Can you help identify the correct RCA since you're the author of that change? Thanks

[ikevin127]

@dukenv0307 I confirm that this issue is a regression caused by PR #40286 from issue #38855.

This happened because here we chose to go with requestAnimationFrame instead of setTimeout with 0 delay in order to defer execution since setTimeout is not preferred, and requestAnimationFrame apparently affected the "New message" scroll functionality.

In order to fix this we could either use setTimeout with 0 delay as I suggested initially in the other issue OR maintain the current function but only defer execution if argument shouldDeferExecution is passed as true to the scrollToBottom function.

Things to consider moving forward with a fix for this issue: #40724 (comment).

cc @robertjchen @mountiny

[parasharrajat]

Aha. I think we can revert the PR and think of a better solution. I don't think setTimeout and requestanimationframe are better solutions.

[robertjchen]

Got it, can this issue be combined with #40724 ?

[MitchExpensify]

@parasharrajat What do you think about the suggestion to consolidate with #40724?

[parasharrajat]

We should revert the PR #40286 that caused this issue and close it.

[robertjchen]

Got it, created a revert PR for review: #41302 and we'll close this out after, resuming the discussion in #40724