Fix tapping on new messages after marking a message unread leads to a blank page #32630

wlegolas · 2023-12-07T04:35:21Z

Details

This PR will fix the issue related to the displayed blank page when the user clicks on the "New messages" button before the scroll action ends.

Fixed Issues

$ #31591
PROPOSAL: #31591 (comment)

Tests

Navigate to https://dev.new.expensify.com:8082
Log in
Open a group conversation with a lot of messages
Swipe to the middle of the page and mark any message as unread
Swipe to the top of the page
Tap on the "New messages" button
The user is redirected to the unread message.

Verify that no errors appear in the JS console

Offline tests

Navigate to https://dev.new.expensify.com:8082
Log in
Open a group conversation with a lot of messages
Turn off your network connection
Swipe to the middle of the page and mark any message as unread
Swipe to the top of the page
Tap on the "New messages" button
The user is redirected to the unread message.

Verify that no errors appear in the JS console

QA Steps

Navigate to https://staging.new.expensify.com/
Log in
Open a group conversation with a lot of messages
Swipe to the middle of the page and mark any message as unread
Swipe to the top of the page
Tap on the "New messages" button
The user is redirected to the unread message.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

evidence-Android-Native.mp4

Android: mWeb Chrome

evidence-Android-mWeb-Chrome.mp4

iOS: Native

evidence-iOS-Native.mp4

iOS: mWeb Safari

evidence-iOS-mWeb-Safari.mp4

MacOS: Chrome / Safari

Chrome

evidence-MacOS-Chrome.mp4

Safari

evidence-MacOS-Safari.mp4

MacOS: Desktop

evidence-MacOS-Desktop.mp4

melvin-bot · 2023-12-07T04:35:26Z

@parasharrajat Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

wlegolas · 2023-12-07T04:40:11Z

src/pages/home/report/ReportActionsList.js

        onScroll(event);
+
+        if (isNative) {


We only call the handleUnreadFloatingButton for Native devices because the native InvertedFlatList doesn't have the implementation to handle the scroll events to call the onScrollEnd handler.

Looks like the issue we faced is present on mWeb then this check will prevent that fix. Right? Also, the scrollend implementation is done on web file(index.js) not on native (index.native.js) of InvertedFlatList.

This check is only used to have the same behavior (shows the "New message" button on scroll) for Native devices because there is no scroll implementation for the index.native.js file for InvetedFlatList.

I'll improve the index.native.js to use the scrolls mechanism instead of this isNative check.

parasharrajat · 2023-12-07T21:12:05Z

src/pages/home/report/ReportActionsList.js

    };

-    const scrollToBottomAndMarkReportAsRead = () => {
-        reportScrollManager.scrollToBottom();
+    const scrollToUnreadMessageAndMarkReportAsRead = () => {


Let's make sure that the unread message is fully visible on the window.

Using the méthod reportScrollManager.scrollToIndex the behavior is to put on the bottom the unread message and the user can see all the message content.

I don't know if your suggestion is to scroll and show this unread message in a particular place on the screen, for example in the middle of the screen. If so, we need to improve the method reportScrollManager.scrollToIndex to receive the viewPosition (scrolltoindex) with a number that indicates the place we want to show this message.

I don't know if your suggestion is to scroll and show this unread message in a particular place on the screen, for example in the middle of the screen. If so, we need to improve the method reportScrollManager.scrollToIndex to receive the viewPosition (scrolltoindex) with a number that indicates the place we want to show this message.

This is a good suggestion when there are multiple unread messages. so I would say let's leave that part for now. No added complexity.

parasharrajat · 2023-12-07T21:19:01Z

src/pages/home/report/ReportActionsList.js

+const platform = getPlatform();
+const isNative = platform === CONST.PLATFORM.IOS || platform === CONST.PLATFORM.ANDROID;


We don't allow direct use of Platform API. Instead, use platform files (index.js or index.native.js etc).

I understood, I saw another component using this approach and I thought I could use the same approach.

I'll change the behavior for native using the index.native.js file.

parasharrajat · 2023-12-22T16:54:13Z

Testing...

parasharrajat · 2023-12-22T16:56:53Z

src/components/InvertedFlatList/index.native.js

+    useEffect(
+        () => () => {
+            if (!scrollEndTimeout.current) {
+                return;
+            }
+
+            clearTimeout(scrollEndTimeout.current);
+        },
+        [props.innerRef],
+    );
+
+    /**
+     * Emits when the scrolling is in progress. Also,
+     * invokes the onScroll callback function from props.
+     *
+     * @param {Event} event - The onScroll event from the FlatList
+     */
+    const onScroll = (event) => {
+        props.onScroll(event);
+    };
+
+    /**
+     * Emits when the scrolling has ended. Also,
+     * invokes the onScrollEnd callback function from props.
+     */
+    const onScrollEnd = () => {
+        props.onScrollEnd();
+    };
+
+    /**
+     * Decides whether the scrolling has ended or not. If it has ended,
+     * then it calls the onScrollEnd function. Otherwise, it calls the
+     * onScroll function and pass the event to it.
+     *
+     * @param {Event} event - The onScroll event from the FlatList
+     */
+    const handleScroll = (event) => {
+        onScroll(event);
+
+        const timestamp = Date.now();
+
+        // console.log('=> handleScroll', {timestamp, 'lastScrollEvent.current': lastScrollEvent.current})
+
+        if (scrollEndTimeout.current) {
+            clearTimeout(scrollEndTimeout.current);
+        }
+
+        scrollEndTimeout.current = setTimeout(() => {
+            if (lastScrollEvent.current !== timestamp) {
+                return;
+            }
+            // Scroll has ended
+            lastScrollEvent.current = null;
+            onScrollEnd();
+        }, 250);
+
+        lastScrollEvent.current = timestamp;
+    };


Most of this logic same on both native and web file so we should move this to BaseInvertedFlatList

Could you please review this commit 590666a please?

Hi @parasharrajat if you have any questions or suggestions, please let me know.

parasharrajat · 2023-12-22T16:57:59Z

src/pages/home/report/ReportActionsList.js

+    const scrollToUnreadMessageAndMarkReportAsRead = () => {
+        const scrollIndex = getScrollIndex();
+
+        isUnreadMessageFocused.current = true;


can you explain the purpose of this flag?

This flag is used to avoid showing the "New Message" button when the scroll ends after the user clicks the "New Message" button.

When the user clicks on the "New Message" button the scroll occurs and we don't need to show again the "New Message" button because this current scroll was triggered by the "New Message" button action.

Ok. I think you tried you mimic Slack behaviour but it is still different from slack. On Slack, when an unread message is visible on the screen marker remains hidden.

parasharrajat · 2023-12-27T14:13:06Z

@wlegolas Can you please merge main?

…inishes

…age button

… in Native devices

…atList component for Native devices

…behavior after user clicks on the New message button

…mentation to the BaseInvertedFlatList component

wlegolas · 2023-12-27T23:58:01Z

@wlegolas Can you please merge main?

Sure, I updated my branch with the main branch. I tested again and It seems like everything is working as expected.

parasharrajat · 2023-12-28T11:16:33Z

Please use merge commit instead of force push. For now, we are fine on this PR.

wlegolas · 2024-01-02T02:53:32Z

Hi @parasharrajat I resolved a conflict in the BaseInvertedFlatList because this component was migrated to TypeScript from this Pull Request #33288

If you have any questions, please let me know.

wlegolas · 2024-01-02T03:02:45Z

I saw the Unit Test step is failing, but this problem is occurring on the main branch, if I see a commit with the fix, I'll do another merge to check this step.

parasharrajat · 2024-01-02T18:25:23Z

src/components/InvertedFlatList/BaseInvertedFlatList.tsx


 const AUTOSCROLL_TO_TOP_THRESHOLD = 128;
 const WINDOW_SIZE = 15;

-function BaseInvertedFlatList<T>(props: FlatListProps<T>, ref: ForwardedRef<FlatList>) {
+function BaseInvertedFlatList<T>({onScroll: onScrollProp = () => {}, onScrollEnd: onScrollEndProp = () => {}, ...props}: InvertedFlatListProps<T>, ref: ForwardedRef<FlatList>) {


No need to give inline type when have already defined that.

Sorry to ask @parasharrajat I didn't understand. Do you mean to not give inline type onScrollProp?

parasharrajat · 2024-01-02T18:28:28Z

src/components/InvertedFlatList/BaseInvertedFlatList.tsx

+     * onScroll function and pass the event to it.
+     *
+     * This is a temporary work around, since react-native-web doesn't
+     * support onScrollBeginDrag and onScrollEndDrag props for FlatList.


I think native has this method so we should use those instead for the native part.

parasharrajat · 2024-01-02T18:29:25Z

src/components/InvertedFlatList/index.tsx

+function InvertedFlatList<T>(
+    {onScroll: onScrollProp = () => {}, onScrollEnd: onScrollEndProp = () => {}, contentContainerStyle, ...props}: InvertedFlatListProps<T>,
+    ref: ForwardedRef<FlatList>,
+) {


Same as the previous comment.

parasharrajat · 2024-01-02T19:18:06Z

src/pages/home/report/ReportActionsList.js

+    const scrollToUnreadMessageAndMarkReportAsRead = () => {
+        const scrollIndex = getScrollIndex();
+
+        isUnreadMessageFocused.current = true;


Ok. I think you tried you mimic Slack behaviour but it is still different from slack. On Slack, when an unread message is visible on the screen marker remains hidden.

parasharrajat · 2024-01-02T19:20:17Z

I am not sure if we want to mimic Slack behavior. Also, this is a big decision to make. If we find this idle then we will have to propose this on Slack first.

I tested your branch and there are a few issues. I will post them here after I am done.

wlegolas · 2024-01-02T21:55:41Z

I am not sure if we want to mimic Slack behavior. Also, this is a big decision to make. If we find this idle then we will have to propose this on Slack first.

I tested your branch and there are a few issues. I will post them here after I am done.

I'm not trying to mimic Slack behavior, just trying to bring the behavior to the "New message" button closes that the behavior we know from Slack, but you're right, we need to propose to the Expensify team.

In my opinion, for this issue, we only need to know which is the expected result when there is a new message, I just putting this behavior because for me makes sense to show the "New message" button when there is a new message and the scroll ends.

But I'm totally open to hearing your suggestion

parasharrajat · 2024-01-17T18:58:46Z

Reviewing...

wlegolas · 2024-01-31T23:32:00Z

Hi @parasharrajat do you have updates about your review process?

parasharrajat · 2024-02-06T05:16:32Z

@wlegolas Please merge the main when you get time. Thanks for waiting.

wlegolas · 2024-02-06T23:32:18Z

@wlegolas Please merge the main when you get time. Thanks for waiting.

Hi @parasharrajat I merged the main into my branch, I did some tests and the implementation is working as expected.

If you need anything else, please let me know.

wlegolas · 2024-02-16T11:49:24Z

Hi @parasharrajat did you have time to review my PR?

parasharrajat · 2024-02-16T14:23:10Z

I have been OOO since 10 days. I was about to grt back but something came up. I will try to get this tomorrow. I started the analysis before going OOO.

wlegolas requested a review from a team as a code owner December 7, 2023 04:35

melvin-bot bot requested review from parasharrajat and removed request for a team December 7, 2023 04:35

wlegolas commented Dec 7, 2023

View reviewed changes

wlegolas mentioned this pull request Dec 7, 2023

[$500] mWeb - Chat - Tapping on new messages after marking a message unread leads to a blank page #31591

Closed

6 tasks

wlegolas force-pushed the bugfix/issue-31591 branch from c17eb26 to ab90916 Compare December 7, 2023 20:18

parasharrajat reviewed Dec 7, 2023

View reviewed changes

wlegolas force-pushed the bugfix/issue-31591 branch from 5cf0e42 to e74926d Compare December 9, 2023 00:33

wlegolas requested a review from parasharrajat December 10, 2023 02:03

wlegolas force-pushed the bugfix/issue-31591 branch from e74926d to 3a7aafd Compare December 19, 2023 21:52

parasharrajat suggested changes Dec 22, 2023

View reviewed changes

wlegolas requested a review from parasharrajat December 27, 2023 12:20

wlegolas added 10 commits December 27, 2023 20:51

Create new property onScrollEnd to be called when the scroll action f…

5aae219

…inishes

Show or hide the New Message button after user clicks on the New Mess…

4afbe65

…age button

Handle the visibility for the floating message counter when scrolling…

0322399

… in Native devices

Implement the functionality to call the onScrollEnd in the InvertedFl…

b273fd1

…atList component for Native devices

Change method to hide/show the new floation message to deal with the …

2a2579c

…behavior after user clicks on the New message button

Change the import order

0a6538f

Fix import order

587f3e1

Change component to use the new hook to get the Theme styles

c44c6c2

Refactor native InvertedFlatlit component to extract the common imple…

d2fbd67

…mentation to the BaseInvertedFlatList component

Fix Prettier issues

5ddfe37

wlegolas force-pushed the bugfix/issue-31591 branch from a095842 to 5ddfe37 Compare December 27, 2023 23:52

Merge branch 'main' into bugfix/issue-31591

bcb7654

wlegolas added 2 commits January 1, 2024 23:46

Resolve conflicts in the new BaseInvertedFlatList files

8d61c48

Fix Prettier issues

c281003

parasharrajat suggested changes Jan 2, 2024

View reviewed changes

wlegolas added 2 commits January 4, 2024 20:53

Merge branch 'main' into bugfix/issue-31591

2278efe

Merge branch 'main' into bugfix/issue-31591

f80434c

Merge branch 'main' into bugfix/issue-31591

83081d1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tapping on new messages after marking a message unread leads to a blank page #32630

Fix tapping on new messages after marking a message unread leads to a blank page #32630

wlegolas commented Dec 7, 2023 •

edited

Loading

melvin-bot bot commented Dec 7, 2023

wlegolas Dec 7, 2023 •

edited

Loading

parasharrajat Dec 7, 2023

wlegolas Dec 7, 2023

parasharrajat Dec 7, 2023

wlegolas Dec 7, 2023

parasharrajat Dec 8, 2023

parasharrajat Dec 7, 2023

wlegolas Dec 7, 2023

parasharrajat commented Dec 22, 2023

parasharrajat Dec 22, 2023

wlegolas Dec 26, 2023 •

edited

Loading

wlegolas Dec 26, 2023

parasharrajat Dec 22, 2023

wlegolas Dec 26, 2023

parasharrajat Jan 2, 2024

parasharrajat commented Dec 27, 2023

wlegolas commented Dec 27, 2023 •

edited

Loading

parasharrajat commented Dec 28, 2023 •

edited

Loading

wlegolas commented Jan 2, 2024 •

edited

Loading

wlegolas commented Jan 2, 2024

parasharrajat Jan 2, 2024

wlegolas Jan 2, 2024

parasharrajat Jan 2, 2024

parasharrajat Jan 2, 2024

parasharrajat Jan 2, 2024

parasharrajat commented Jan 2, 2024

wlegolas commented Jan 2, 2024

parasharrajat commented Jan 17, 2024 •

edited

Loading

wlegolas commented Jan 31, 2024

parasharrajat commented Feb 6, 2024

wlegolas commented Feb 6, 2024

wlegolas commented Feb 16, 2024

parasharrajat commented Feb 16, 2024

		const platform = getPlatform();
		const isNative = platform === CONST.PLATFORM.IOS \|\| platform === CONST.PLATFORM.ANDROID;

Fix tapping on new messages after marking a message unread leads to a blank page #32630

Are you sure you want to change the base?

Fix tapping on new messages after marking a message unread leads to a blank page #32630

Conversation

wlegolas commented Dec 7, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Dec 7, 2023

wlegolas Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat commented Dec 22, 2023

Choose a reason for hiding this comment

wlegolas Dec 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat commented Dec 27, 2023

wlegolas commented Dec 27, 2023 • edited Loading

parasharrajat commented Dec 28, 2023 • edited Loading

wlegolas commented Jan 2, 2024 • edited Loading

wlegolas commented Jan 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat commented Jan 2, 2024

wlegolas commented Jan 2, 2024

parasharrajat commented Jan 17, 2024 • edited Loading

wlegolas commented Jan 31, 2024

parasharrajat commented Feb 6, 2024

wlegolas commented Feb 6, 2024

wlegolas commented Feb 16, 2024

parasharrajat commented Feb 16, 2024

wlegolas commented Dec 7, 2023 •

edited

Loading

wlegolas Dec 7, 2023 •

edited

Loading

wlegolas Dec 26, 2023 •

edited

Loading

wlegolas commented Dec 27, 2023 •

edited

Loading

parasharrajat commented Dec 28, 2023 •

edited

Loading

wlegolas commented Jan 2, 2024 •

edited

Loading

parasharrajat commented Jan 17, 2024 •

edited

Loading