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

Sending a message scrolls down to previous message #54071

49 changes: 37 additions & 12 deletions src/pages/home/report/ReportActionsList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ function ReportActionsList({
const lastMessageTime = useRef<string | null>(null);
const [isVisible, setIsVisible] = useState(Visibility.isVisible);
const isFocused = useIsFocused();
const [pendingBottomScroll, setPendingBottomScroll] = useState(false);

const [reportNameValuePairs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${report?.reportID}`);
const [accountID] = useOnyx(ONYXKEYS.SESSION, {selector: (session) => session?.accountID});
Expand Down Expand Up @@ -443,23 +444,47 @@ function ReportActionsList({
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, []);

const isNewMessageDisplayed = useMemo(() => {
const prevActions = Object.values(prevSortedVisibleReportActionsObjects);
const lastPrevVisibleAction = prevActions.at(0);
return lastAction?.reportActionID !== lastPrevVisibleAction?.reportActionID;
}, [prevSortedVisibleReportActionsObjects, lastAction]);

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 || scrollingVerticalOffset.current === 0 || !isReportScreenTopmostCentralPane()) {
return;
}
if (!hasNewestReportActionRef.current) {
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.reportID));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question, do we have any case that needs to run this navigation runAfterInteractions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is to make sure that the scroll occurs after any high-priority interaction in the RN event loop, most likely animation, without it it can lead to some jerky animations or incomplete rendering.

It was already added in the code before that PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right. I mean previously, this navigation is put inside runAfterInteractions, but with our change, it's moved outside of runAfterInteractions. I'm checking if it's safe to move outside.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it by hardcoding reportID to navigate to and I didn't see any issues. Here is an example video:

navigation.mp4

return;
}
if (!isNewMessageDisplayed) {
setPendingBottomScroll(true);
} else {
InteractionManager.runAfterInteractions(() => {
reportScrollManager.scrollToBottom();
});
}
},
[reportScrollManager, report.reportID, isNewMessageDisplayed],
);

useEffect(() => {
if (!pendingBottomScroll || scrollingVerticalOffset.current === 0) {
return;
}

if (isNewMessageDisplayed) {
InteractionManager.runAfterInteractions(() => {
// 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 || !isReportScreenTopmostCentralPane()) {
return;
}
if (!hasNewestReportActionRef.current) {
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.reportID));
return;
}
reportScrollManager.scrollToBottom();
setPendingBottomScroll(false);
});
},
[reportScrollManager, report.reportID],
);
}
}, [pendingBottomScroll, reportScrollManager, isNewMessageDisplayed]);

useEffect(() => {
// Why are we doing this, when in the cleanup of the useEffect we are already calling the unsubscribe function?
// Answer: On web, when navigating to another report screen, the previous report screen doesn't get unmounted,
Expand Down
Loading