[$250] Clicking on "New Messages" jumps to newest messages, skipping over any messages marked unread report by @chauchausoup

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Go to any chat
2. Click at Mark as Unread
3. Click on New Messages button

Expected Result:

Clicking "new messages" should jump directly to the message marked as Unread

Actual Result:

Clicking "new messages" jumps to the bottom of the message thread, skipping over the message marked as unread

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web

Version Number: 1.2.11-2 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: https://user-images.githubusercontent.com/43996225/193866710-b8402785-759a-4f1f-aa86-80b0878cb3c9.mov https://user-images.githubusercontent.com/43996225/193866742-10e455e5-acd7-4d2d-862a-b61a85c3e9c5.mp4 Expensify/Expensify Issue URL: Issue reported by: @chauchausoup Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1664725500869019 View all open jobs on GitHub

[melvin-bot[bot]]

Triggered auto assignment to @greg-schroeder (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[greg-schroeder]

I reproduced this on the desktop app:

https://user-images.githubusercontent.com/29386373/193938022-1bc5a558-2b98-4196-84f0-9276480058bc.mp4

Adjusted some of the OP to make it clearer

[melvin-bot[bot]]

Triggered auto assignment to @srikarparsi (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[melvin-bot[bot]]

Triggered auto assignment to @conorpendergrast (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Current assignee @srikarparsi is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

@conorpendergrast, @eVoloshchak, @srikarparsi Whoops! This issue is 2 days overdue. Let's get this updated quick!

[conorpendergrast]

Oops, I'll get to this today, sorry!

[tienifr]

Proposal

Problem

• Currently, the logic scroll to unread message is using scrollToBottom function https://github.com/Expensify/App/blob/7b21103bb56eb0cf3fc1561715613d2a16c3255f/src/pages/home/report/ReportActionsView.js#L285

• function ReportScrollManager.scrollToIndex isn't work correctly in web because it has different behavior scroll from native

Solution

In https://github.com/Expensify/App/blob/7b21103bb56eb0cf3fc1561715613d2a16c3255f/src/pages/home/report/ReportActionsView.js#L286 we'll change the function name scrollToBottomAndMarkReportAsRead to scrollToUnreadMsgAndMarkReportAsRead we'll also change to use scrollToIndex. In theory, we just need to pass this.props.report.lastReadSequenceNumber as the index but we're using inverted flatList so we have to pass this.props.report.maxSequenceNumber - this.state.newMarkerSequenceNumber

-        this.scrollToBottomAndMarkReportAsRead = this.scrollToBottomAndMarkReportAsRead.bind(this);
+       this.scrollToUnreadMsgAndMarkReportAsRead = this.scrollToUnreadMsgAndMarkReportAsRead.bind(this);

-    scrollToBottomAndMarkReportAsRead() {
-        ReportScrollManager.scrollToBottom();
+    scrollToUnreadMsgAndMarkReportAsRead() {
+        ReportScrollManager.scrollToIndex({animated: true, index: this.props.report.maxSequenceNumber - this.state.newMarkerSequenceNumber}, false);
        Report.readNewestAction(this.props.report.reportID);
    }

-                            onClick={this.scrollToBottomAndMarkReportAsRead}
+                           onClick={this.scrollToUnreadMsgAndMarkReportAsRead}

In https://github.com/Expensify/App/blob/main/src/components/InvertedFlatList/BaseInvertedFlatList.js we need to measure again because the image can be loaded later. And we also pass sizeMap to FlatList to calculate the offset value

    measureItemLayout(nativeEvent, index) {
        const computedHeight = nativeEvent.layout.height;

 -       // We've already measured this item so we don't need to
 -       // measure it again.
 -       if (this.sizeMap[index]) {
 -           return;
 -       }

      <FlatList
                // eslint-disable-next-line react/jsx-props-no-spreading
                {...this.props}
                ref={this.props.innerRef}
                inverted
                renderItem={this.renderItem}
+              sizeMap={this.sizeMap}

In https://github.com/Expensify/App/blob/7b21103bb56eb0cf3fc1561715613d2a16c3255f/src/libs/ReportScrollManager/index.js#L14 we have to re-implement the logic scrollToIndex in non-native platform because we're using inverted flatList so we have to calculate the total offset value from top to index, then + paddingTop, + viewOffset, - clientHeight * index.viewPosition if need. Finally pass to offset value in scrollToOffset function

-    flatListRef.current.scrollToIndex(index);
+    const sizeMap = flatListRef.current.props.sizeMap;
+    const clientHeight = flatListRef.current.getScrollableNode().clientHeight;
+    const paddingTop = parseFloat(flatListRef.current.getScrollableNode().childNodes[0].style.paddingTop || '0px');

+   const sizeArray = _.map(_.values(sizeMap), s => s.length);
+    const sumNum = _.reduce(sizeArray.slice(0, (index.index)), (acc, val) => acc + val);
+   flatListRef.current.scrollToOffset({
+       animated: index.animated,
+       offset: (sumNum + paddingTop + (index.viewOffset || 0)) - (clientHeight * (index.viewPosition || 0)),
+    });
}

https://user-images.githubusercontent.com/113963320/195074818-5736b458-7c56-4b14-824a-ed1a6a7b533a.mp4

https://user-images.githubusercontent.com/113963320/195074906-b0cd9614-8351-45fd-a4ea-808f74ec983b.mp4

[conorpendergrast]

Job posting: https://www.upwork.com/ab/applicants/1579975844227059712/job-details

[conorpendergrast]

@eVoloshchak Hey! Ready for your review on the proposal ^^^

[eVoloshchak]

Need to test this one thoroughly, expect a review in 24 hours

[melvin-bot[bot]]

@conorpendergrast, @eVoloshchak, @srikarparsi Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[eVoloshchak]

@tienifr proposal looks good, confirmed it working on all of the platforms A couple of small things:

• Is there a way to obtain clientHeight and paddingTop differently? Maybe there are constants defined in styles.js? If not it's ok
• Could you please overexplain what is the sizeArray and sumNum variables and what we need them for?

[tienifr]

@eVoloshchak Thanks for your review

1. The scrollToIndex is used in a lot of places, so I think it is better way to access clientHeight and paddingTop
2. Because we're using sizeArray to store the dimension of the item and we want to access to them to calculate the total height from index to length - 1 (sumNum). In theory, we need to scroll from 0 to index, but we're using invertedWheelEvent, so I just want to calculate the total height from index to length - 1

[eVoloshchak]

1. The scrollToIndex is used in a lot of places, so I think it is better way to access clientHeight and paddingTop
2. Because we're using sizeArray to store the dimension of the item and we want to access to them to calculate the total height from index to length - 1 (sumNum). In theory, we need to scroll from 0 to index, but we're using invertedWheelEvent, so I just want to calculate the total height from index to length - 1

I see, thank you

I agree with the first part of your proposal, about using ReportScrollManager.scrollToIndex instead of ReportScrollManager. scrollToBottom

However, the updated logic for scrollToBottom function looks like it belongs in the package with FlatList itself. Can we fix this for all of the inverted lists inside of the package itself? I imagine it's the react-native-web

[tienifr]

@eVoloshchak sorry, I don't catch on your mean

1. I'm updating the logic for scrollToIndex not scrollToBottom
2. We're implementing invertedWheelEvent in our project and FlatList in react-native-web works well, so I don't think we should change the inverted lists inside of the package itself

[eVoloshchak]

I'm updating the logic for scrollToIndex not scrollToBottom

I was talking about this function, but yes, I can see that your implementation calls scrollToBottom inside of it

We're implementing invertedWheelEvent in our project

Oh cool, didn't notice it. In that case your fix seems like the best solution. We essentially need to correctly calculate the offset to the first unread message and scroll to it, which it does.

I assume you have already seen this but if not, please take a look at the STYLE.md contributing guide before submitting the PR.

cc: @conorpendergrast, @tienifr's proposal looks good to me 🎀👀🎀 C+ reviewed!

[conorpendergrast]

Great! @srikarparsi Are you happy here too? If so, I'll hire via Upwork (this process)

[srikarparsi]

yup this looks good to me as well, assigning @tienifr to the issue

[melvin-bot[bot]]

📣 @tienifr You have been assigned to this job by @srikarparsi! Please apply to this job in Upwork 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 📖

[tienifr]

Thanks @srikarparsi , I applied to the job and the PR will be ready by October 21

[conorpendergrast]

Via Upwork, hired @tienifr to fix the issue, @chauchausoup for reporting the bug, @eVoloshchak as contributor+

[melvin-bot[bot]]

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

• [ ] A regression test has been added or updated so that the same bug will not reach production again. Link to the updated test here:
• [ ] The PR that introduced the bug has been identified. Link to the PR:
• [ ] 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:
• [ ] A discussion in #contributor-plus 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:
• [ ] Payment has been made to the issue reporter (if applicable)
• [ ] Payment has been made to the contributor that fixed the issue (if applicable)
• [ ] Payment has been made to the contributor+ that helped on the issue (if applicable)

[melvin-bot[bot]]

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

• [ ] A regression test has been added or updated so that the same bug will not reach production again. Link to the updated test here:
• [ ] The PR that introduced the bug has been identified. Link to the PR:
• [ ] 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:
• [ ] A discussion in #contributor-plus 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:
• [ ] Payment has been made to the issue reporter (if applicable)
• [ ] Payment has been made to the contributor that fixed the issue (if applicable)
• [ ] Payment has been made to the contributor+ that helped on the issue (if applicable)

[melvin-bot[bot]]

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

• [ ] A regression test has been added or updated so that the same bug will not reach production again. Link to the updated test here: Proposed in internal channel
• [x] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/4603
• [x] 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: https://github.com/Expensify/App/pull/4603#issuecomment-1310192920
• [x] A discussion in #contributor-plus 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: https://expensify.slack.com/archives/C02NK2DQWUX/p1668155104493619
• [x] Payment has been made to the issue reporter (if applicable)
• [x] Payment has been made to the contributor that fixed the issue (if applicable)
• [x] Payment has been made to the contributor+ that helped on the issue (if applicable)

Proposed testing steps:

1. Set up a DM or group chat that has messages from at lease one other user, and that has more than two screens of messages
2. Open that DM or group chat
3. On computer hover over a message from another user toward the top of your screen, and click on Mark as Unread. On phone, long press the message and click on Mark as Unread
4. Scroll further up, and click/ tap on New Messages button
5. Confirm you are scrolled to the message you marked as unread

[conorpendergrast]

@srikarparsi @eVoloshchak @tienifr Hey! I've added some proposed testing steps. Who can help with the second, third and fourth points (finding the PR that introduced the bug, the comments on that PR, and starting a discussion in #contributor-plus)? Then, I will get started on the last 3 steps!

[melvin-bot[bot]]

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

• https://github.com/Expensify/App/pull/12124

If no regressions arise, payment will be issued on 2022-11-07. :confetti_ball:

[conorpendergrast]

Looks like we're all set to pay this. I'll review the process and will do that tomorrow!

[conorpendergrast]

And, also, this checklist above!