[HOLD for payment 2024-07-10] [$250] Chat does not scroll to the last message when a new message received

[m-natarajan]

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.4.82-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:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @youssef-lr
Slack conversation:

Action Performed:

1. Go to a report with lots of messages
2. Send a message to user B

Expected Result:

For user B the chat scrolls to the last message received

Actual Result:

Does not scroll

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

Recording.205.mp4

Screen.Recording.2024-06-11.at.21.37.40.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0178751bd086a2062a
• Upwork Job ID: 1802672136974977044
• Last Price Increase: 2024-06-17
• Automatic offers:
• s77rt | Reviewer | 102816257
• tsa321 | Contributor | 102816260

Issue Owner

Current Issue Owner: @bfitzexpensify

[melvin-bot]

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

[tsa321]

Proposal

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

Chat does not scroll to the last message when a new message received

What is the root cause of that problem?

When user receive new message, the pushJSON data contains onyx update for report and the new reportAction.
The report onyx data will update lastVisibleAcitonCreated and reportAction will add new latest reportAction.
In here:

App/src/pages/home/report/ReportActionsView.js

Line 215 in a5d0c4d

const hasNewestReportAction = lodashGet(reportActions[0], 'created') === props.report.lastVisibleActionCreated;

when onyx update on report data the hasNewestReportAction will be false because the reportActions isn't updated yet, then the onyx will update reportActions which will make hasNewestReportAction to be true. So there is toggling false then true after the complete update of data from push event.

The hasNewestReportAction will affect enableAutoscrollToTopThreshold value, which is the scrolling logic depends on.

In here:

App/src/components/FlatList/MVCPFlatList.js

Lines 101 to 102 in a5d0c4d

	if (mvcpAutoscrollToTopThreshold != null && scrollOffset <= mvcpAutoscrollToTopThreshold) {
	scrollToOffset(0, true);

when the pushData update completed, the mvcpAutoscrollToTopThreshold value used in here isn't the latest mvcpAutoscrollToTopThreshold. the value is still undefined because of the update from report onyx data and hasn't been updated after reportAction onyx merged. So when the delta value is higher 0.5 / new message inserted in list, the value of mvcpAutoscrollToTopThreshold hasn't been updated.

Since the `adjustForMaintainVisibleContentPosition` is used in subscriber of `MutationObserver`, we need the updated value right at when message is inserted into the list. So we need the latest value and using ref here can solve the issue. If we are using useCallback that depend on the `mvcpAutoscrollToTopThreshold` the new subscriber will be created and will disturb detection in `mutationObserver` when observing the list content difference. Sometimes I noticed, when new subscriber based on new callback produced by useCallback, the list is already changes and delta difference is still 0. So we must remove the dependency of `mvcpAutoscrollToTopThreshold` in `adjustForMaintainVisibleContentPosition` and use ref of mvcpAutoscrollToTopThreshold.

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

We must remove the dependency of mvcpAutoscrollToTopThreshold in adjustForMaintainVisibleContentPosition and we could use ref of mvcpAutoscrollToTopThreshold at the top of the function component and immediately update it when new mvcpAutoscrollToTopThreshold available, then use it inside adjustForMaintainVisibleContentPosition

The code could be:

const mvcpAutoscrollToTopThresholdRef = useRef(mvcpAutoscrollToTopThreshold);
mvcpAutoscrollToTopThresholdRef.current = mvcpAutoscrollToTopThreshold;
....
// then in adjustForMaintainVisibleContentPosition:
if (mvcpAutoscrollToTopThresholdRef.current != null && scrollOffset <= mvcpAutoscrollToTopThresholdRef.current) {
     scrollToOffset(0, true);
}

and we must remove mvcpAutoscrollToTopThreshold in :

App/src/components/FlatList/index.tsx

Line 112 in f838795

}, [getScrollOffset, scrollToOffset, mvcpMinIndexForVisible, mvcpAutoscrollToTopThreshold, horizontal]);

Other alternative is (workaround):

We could use useMemo to avoid unnecessary value changes on hasNewestReportAction when the onyx pushJSON data isn't fully merged yet.

In here, the code could be:

const hasNewestReportAction = useMemo(() => lodashGet(reportActions[0], 'created') === props.report.lastVisibleActionCreated, [reportActions.length]);

So when new report action is updated, the shouldEnableAutoScroll that depend on hasNewestReportAction will keep on true and the List will scroll.

[melvin-bot]

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

[melvin-bot]

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

[s77rt]

I'm not able to reproduce the bug. @tsa321 Is this still reproducible for you?

Screen.Recording.2024-06-17.at.11.50.57.PM.mov

[tsa321]

@s77rt yes, I can still reprdouce it. You must scroll up to 1 or 2 messages above before receiving new message.

macos-web-d.mp4

[s77rt]

@tsa321 I'm able to reproduce now. Regarding your RCA:

the mvcpAutoscrollToTopThreshold value used in here isn't the latest mvcpAutoscrollToTopThreshold. the value is still undefined because of the update from report onyx data and hasn't updated after reportAction onyx merged. So it won't scroll

After testing the latest rendered value is 250 but this is still not scrolling. Can you please investigate this more?

Screen.Recording.2024-06-19.at.1.08.33.AM.mov

[tsa321]

@s77rt you are logging above this line right? :

App/src/components/FlatList/index.tsx

Line 104 in f838795

if (Math.abs(delta) > 0.5) {

Please add the delta value, console.log({delta, mvcpAutoscrollToTopThreshold}) and notice when the delta value is above 0.5, the mvcpAutoscrollToTopThreshold is undefined:

macos-web--d.mp4

The delta here is important to determine whether there is a change in list content (in this issue: new message from other user). The mvcpAutoscrollToTopThreshold must be latest value when the delta is above 0.5.

I have updated my proposal on last paragraph of RCA .

[s77rt]

@tsa321 hasNewestReportAction and mvcpAutoscrollToTopThreshold are being updated at the same time. The bug is that the value that we end up using in adjustForMaintainVisibleContentPosition is outdated even though mvcpAutoscrollToTopThreshold is added as a dependency.

Screen.Recording.2024-06-19.at.9.25.55.PM.mov

[s77rt]

Adding to ^ I'm seeing that setupMutationObserver is getting called unexpectedly (or too early)

Screen.Recording.2024-06-19.at.9.41.40.PM.mov

[tsa321]

@s77rt in here:

App/src/components/FlatList/index.tsx

Lines 148 to 156 in a4a1b20

	useEffect(() => {
	if (!isListRenderedRef.current) {
	return;
	}
	requestAnimationFrame(() => {
	prepareForMaintainVisibleContentPosition();
	setupMutationObserver();
	});
	}, [prepareForMaintainVisibleContentPosition, setupMutationObserver]);

When mvcpAutoscrollToTopThreshold changes, the setupMutationObserver will changes and will be executed by this useEffect.
This is because: setupMutationObserver depend on adjustForMaintainVisibleContentPosition which is depend on mvcpAutoscrollToTopThreshold. (you can confirm this by removing mvcpAutoscrollToTopThreshold dependency in adjustForMaintainVisibleContentPosition, the setupMutationObserver won't be triggered when new message coming/mvcpAutoscrollToTopThreshold changes).

So what you say about setupMutationObserver on:

I'm seeing that setupMutationObserver is getting called unexpectedly (or too early)

It is not too early, it is because of first value change of mvcpAutoscrollToTopThreshold from 250 to undefined, setupMutationObserver will be triggered and will create new MutationObserver.

When new reportAction merged by onyx, (mvcpAutoscrollToTopThreshold changes from undefined to 250) the setupMutationObserver will be executed again because value mvcpAutoscrollToTopThreshold changes from undefined to 250. The creation and disconnection of MutationObserver because of mvcpAutoscrollToTopThreshold value changes will disturb the detection of content change in the list.

So the simplest fix in this issue is to prevent creating new subscriber when mvcpAutoscrollToTopThreshold value changes.
In here:

App/src/components/FlatList/index.tsx

Line 112 in a4a1b20

}, [getScrollOffset, scrollToOffset, mvcpMinIndexForVisible, mvcpAutoscrollToTopThreshold, horizontal]);

We remove adjustForMaintainVisibleContentPosition dependency on mvcpAutoscrollToTopThreshold. Then use ref of the mvcpAutoscrollToTopThreshold inside adjustForMaintainVisibleContentPosition. The code could be

// at the top 
const mvcpAutoscrollToTopThresholdRef = useRef(mvcpAutoscrollToTopThreshold);
mvcpAutoscrollToTopThresholdRef.current = mvcpAutoscrollToTopThreshold;
....
....
// then inside adjustForMaintainVisibleContentPosition:
if (mvcpAutoscrollToTopThresholdRef.current != null && scrollOffset <= mvcpAutoscrollToTopThresholdRef.current) {
     scrollToOffset(0, true);
}

[s77rt]

@tsa321 Makes sense! The solution looks good to me

🎀 👀 🎀 C+ reviewed
Link to proposal

[melvin-bot]

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

[melvin-bot]

📣 @s77rt 🎉 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]

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

[tsa321]

@s77rt PR is ready...

[mountiny]

Can we retest again in a it to verify this is fixed everywhere based on #44132 (comment) thanks!

[s77rt]

@tsa321 This actually seems broken on main. Can you check from your side?

[tsa321]

@s77rt you are right. it is broken in main. I am investigating it

[tsa321]

@s77rt Early investigation: after setupMutationObserver is called (creating the mutationObserver), the mutationObserver eventually gets disconnected here:

App/src/components/FlatList/index.tsx

Lines 176 to 181 in 27a8d0a

	useEffect(() => {
	const mutationObserver = mutationObserverRef.current;
	return () => {
	mutationObserver?.disconnect();
	};
	}, []);

I will further investigate why it is currently behaving that way.

Update: When opening a report, the ReportScreen gets mounted, unmounted, mounted, unmounted, and then mounted again, resulting in it being remounted 2-3 times.
As you can see in the developer tools's network tab, the OpenReport request is being called twice, and it appears that comment linking is broken.

[s77rt]

@tsa321 Any idea on which PR broke this? It's seems to be a deploy blocker as this works on staging

[tsa321]

@s77rt Currently, I am not with my computer, but I am guessing that the PR causing the issue is #42592
For the comment linking should also check on #44412

[s77rt]

@tsa321 I see, this is related to React.StrictMode which makes the bug dev only. This is a regression from that PR then.

[melvin-bot]

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

[melvin-bot]

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

• Fix report doesn't scroll down to bottom when receiving new message #44132

If no regressions arise, payment will be issued on 2024-07-10. 🎊

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

• @s77rt requires payment automatic offer (Reviewer)
• @tsa321 requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
• [@s77rt] 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:
• [@s77rt] A discussion in #expensify-bugs 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:
• [@s77rt] Determine if we should create a regression test for this bug.
• [@s77rt] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@bfitzexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[s77rt]

• The PR that introduced the bug has been identified: HIGH: (Comment linking: step 2) [23220] WEB maintain visible content position #32098
• The offending PR has been commented on: HIGH: (Comment linking: step 2) [23220] WEB maintain visible content position #32098 (comment)
• A discussion in #expensify-bugs has been started: Not needed. Unique bug
• Determine if we should create a regression test for this bug: I don't think this requires a test, it could cause false positives as the auto scroll is supposed to happen only if the scroll position is below a certain threshold

[bfitzexpensify]

Payments complete. Closing this one out.