[HOLD on #16660][$1000] Web – Assign Task – Whole page jitter on check and uncheck of checkbox.

kbecciv commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

Open any " Assign Task " chat.
Check and uncheck checkbox under title multiple times.
Notice after few times of check and uncheck whole page starts to jitter.

Expected Result:

Page should not start to jitter upon check and uncheck of checkbox.

Actual Result:

Page starts to jitter upon selection check and uncheck of checkbox.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[ ] Android / native
[ ] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[x] MacOS / Chrome / Safari
[ ] MacOS / Desktop

Version Number: 1.3.39-5 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 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/8401f72f-a157-44fc-84f3-75f2a2c30500

https://github.com/Expensify/App/assets/93399543/f4d8680e-7b8e-487a-9d96-2af02d4a2a36

Expensify/Expensify Issue URL: Issue reported by: @usmantariq96 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689069224351539

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0156490fa397b075d8
Upwork Job ID: 1681396985675022336
Automatic offers:

| | 0
| | 0

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @michaelhaxhiu (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[x] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[x] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[x] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot[bot] commented 1 year ago

@michaelhaxhiu Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 year ago

@michaelhaxhiu 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~0156490fa397b075d8

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

michaelhaxhiu commented 1 year ago

This was tough to reproduce, but I got it eventually. Make sure to check/uncheck the task until there is a scroll bar in the thead, then scroll up so that you can't see new updates at the footer if you continue to check/uncheck

robertKozik commented 1 year ago

Indeed very tough to repro, I'll keep it under on radar and wait for any proposals 👌🏼

akamefi202 commented 1 year ago

Proposal

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

In task page, the page jitters when the user clicks(check & uncheck) the checkbox continuously. (Approximately, more than 10~14 times)

What is the root cause of that problem?

https://github.com/Expensify/App/blob/8dcad168d11a71424b6ffd7fa30fd1770c0165d5/src/components/InvertedFlatList/BaseInvertedFlatList.js#L136-L138 I think it's because of getItemLayout in FlatList. Currently, we save size & offset of each item using measureItemLayout function while rendering and we input them using getItemLayout function in next rendering. We use sizeMap array to save size & offset of items.

But all these code was assumed and written as if new item will be added to the last element of the item array in FlatList when the user adds a comment. Actually, it will be added to first element of the item array.

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

We need to update getItemLayout and measureItemLayout functions based on correct mechanism. const sizeMapIndex = this.props.data.length - index - 1; We need to get index again in reverse order when we read & write size information of items with sizeMap array. And also offset of newly added item is 0.

I updated the code and I wasn't able to see jitter issue. Screencast from 2023年07月21日 16时46分54秒.webm

What alternative solutions did you explore? (Optional)

We may not use getItemLayout in FlatList. But it could affect performance for list of many items.

robertKozik commented 1 year ago

Hi @akamefi202, thanks for your proposal. First of all, I've tried to test your approach on my end, but it causes an error for me. I've attached a diff which I used.

Later, I'm not sure about the root cause you have written. As, most likely, you pinpointed the faulty code correctly, but you didn't provide an answer as to why it only occurs after spamming check/uncheck. If we were wrongly saving measurements, shouldn't it cause problems way earlier, not only after clicking multiple times?

diff --git a/src/components/InvertedFlatList/BaseInvertedFlatList.js b/src/components/InvertedFlatList/BaseInvertedFlatList.js
index d3fcda0ea5..e941933cec 100644
--- a/src/components/InvertedFlatList/BaseInvertedFlatList.js
+++ b/src/components/InvertedFlatList/BaseInvertedFlatList.js
@@ -53,13 +53,14 @@ class BaseInvertedFlatList extends Component {
      * @return {Object}
      */
     getItemLayout(data, index) {
-        const size = this.sizeMap[index];
+        const sizeMapIndex = this.props.data.length - index - 1;
+        const size = this.sizeMap[sizeMapIndex];

         if (size) {
             return {
                 length: size.length,
                 offset: size.offset,
-                index,
+                index: sizeMapIndex,
             };
         }

@@ -76,7 +77,7 @@ class BaseInvertedFlatList extends Component {
             // initialRowHeight since we can only assume that all previous items
             // have not yet been measured
             offset: _.isUndefined(lastMeasuredItem) ? this.props.initialRowHeight * index : lastMeasuredItem.offset + this.props.initialRowHeight,
-            index,
+            index: sizeMapIndex,
         };
     }

@@ -88,20 +89,22 @@ class BaseInvertedFlatList extends Component {
      */
     measureItemLayout(nativeEvent, index) {
         const computedHeight = nativeEvent.layout.height;
+        const sizeMapIndex = this.props.data.length - index - 1;

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

-        const previousItem = this.sizeMap[index - 1] || {};
+        const previousItem = this.sizeMap[sizeMapIndex - 1] || {};

         // If there is no previousItem this can mean we haven't yet measured
         // the previous item or that we are at index 0 and there is no previousItem
         const previousLength = previousItem.length || 0;
         const previousOffset = previousItem.offset || 0;
-        this.sizeMap[index] = {
+        this.sizeMap[sizeMapIndex] = {
             length: computedHeight,
             offset: previousLength + previousOffset,
         };

akamefi202 commented 1 year ago

@robertKozik About the code changes, we should use original index when return size data. We should use sizeMapIndex only for reading & writing sizeMap array.

if (size) {
     return {
         length: size.length,
         offset: size.offset,
         index,
     };
}

And offset should be 0, when we return size data of newly added item.

About root cause, getItemLayout is an optional optimization to improve performance of FlatList when it contains so many items. When FlatList has only few items, it is enough to compute size of items correctly. After many items are added to the list, the component start to refer the size data(which is not accurate now) to reduce time for the measurement of dynamic content. So we see this issue after many items are added to the list.

One more thing is we're adding new items to invisible viewport when we check & uncheck the checkbox. I think this is the reason why we see this issue only in task page. The component gives priority to compute size of visible items and we're adding new items to invisible viewport. So it might use wrong size data from sizeMap array instead of computing size of new items.

This is just my analysis and other engineer might find more accurate reason. But it's definitely clear that this issue is occurred by getItemLayout and we can fix by updating getItemLayout and measureItemLayout functions.

melvin-bot[bot] commented 1 year ago

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

kosmydel commented 1 year ago

Proposal

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

After checking/unchecking the task the page jitter.

What is the root cause of that problem?

The heights/offsets of the elements in the inverted FlatList are not calculated correctly. The full explanation for that is here. Previously, we addressed it by creating a BaseInvertedFlatList component, but it doesn't work fully as expected.

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

Previously, we used the BaseInvertedFlatList component for inverted lists, to make it work on the web. However, the react-native-web library has addressed this issue related to the inverted FlatLists in this PR.

Solution:

We can update the @expensify/react-native-web with the latest changes from the necolas/react-native-web repository.
Remove the BaseInvertedFlatList component.
Replace <BaseInvertedFlatList /> usages with:
```
<FlatList
inverted
{...otherProps}
/>
```

However, there are some dependency-related issues here.

What alternative solutions did you explore? (Optional)

We can add to the @expensify/react-native-web fork changes only from this PR and replace BaseInvertedFlatList with FlatList with inverted option.

I created a short PoC video showing the results.

PoC video

https://github.com/Expensify/App/assets/104823336/ec4b725e-6d07-440f-8a66-94008572c8e6

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

robertKozik commented 1 year ago

@akamefi202 Your proposal has some strange behaviour when the message list is very long:

https://github.com/Expensify/App/assets/61146594/a32dabd7-e281-4e91-a9f8-c72bd4b278d2

@kosmydel I think we have bumping the react-native-web version in plans. If thats truly the case your proposal is better suited for this issue

melvin-bot[bot] commented 1 year ago

@michaelhaxhiu @robertKozik this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

robertKozik commented 1 year ago

Working on proposals 👀

shubham1206agra commented 1 year ago

@robertKozik Can we try to move to flashlist / recyclerlistview instead of using flatlist?

melvin-bot[bot] commented 1 year ago

📣 @allroundexperts Please request via NewDot manual requests for the Contributor role ($1000)

melvin-bot[bot] commented 1 year ago

📣 @usmantariq96 We're missing your Upwork ID to automatically send you an offer for the Reporter role. Once you apply to the Upwork job, your Upwork ID will be stored and you will be automatically hired for future jobs!

allroundexperts commented 1 year ago

Thanks for your proposal @akamefi202. I'm not entirely convinced by the solution that you're proposing. I think if the upstream has a fix, we should go with that instead of updating our implementations.

I think @kosmydel's proposal has a better solution to fix the problem. Let's go with them.

🎀 👀 🎀 C+ reviewed

@kosmydel Before proceeding with updating react-native-web, I think it would be better to start a discussion in the open-source channel to see if we want to do a full blown upgrade (I fear that this will cause a lot of regressions) or just copying the changes from the PR you've mentioned.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

@michaelhaxhiu @allroundexperts @stitesExpensify this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

allroundexperts commented 1 year ago

Discussion ongoing on Slack.

stitesExpensify commented 1 year ago

I think that @kosmydel has the best proposal here, and we should wait for the react-native-web update before proceeding. That is in this issue which is also blocked, but we are very close to it being unblocked, so I think it's worth the wait.

michaelhaxhiu commented 1 year ago

Since this is on Hold, I'm gonna apply Weekly label.

Note: I'm going to be OOO the next ~2 weeks and given this is on Hold, I'm not going to assign a BZ teammate because I feel like this won't be complete before I return. If I'm mistaken and this sees progress before I return, can you please remove my assignment & re-apply the Bug label to assign a BZ that's online? Thanks!

stitesExpensify commented 1 year ago

Still on hold

stitesExpensify commented 1 year ago

hold

melvin-bot[bot] commented 11 months ago

Triggered auto assignment to @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

michaelhaxhiu commented 11 months ago

I'm re-assigning this to another BZ as part of my preparation for Sabbatical (starting Friday).

Next steps:

This is on hold for #16660
I'm labeling this Monthly as it's blocked, so please check to ensure we unblock when it's time.

adelekennedy commented 10 months ago

linked issue is still in progress

kosmydel commented 10 months ago

Hey, I've investigated this issue and it looks like it is fixed now. This PR introduced changes that I suggested in my proposal. I've retested this issue and for me, it doesn't occur anymore. I think we can close this one out.

Video

https://github.com/Expensify/App/assets/104823336/99bf071e-6282-4e0f-a794-22bb83ed5edf

cc @allroundexperts, @stitesExpensify, @adelekennedy

adelekennedy commented 9 months ago

I can't reproduce so closing makes sense to me, gut check with you @allroundexperts

kosmydel commented 9 months ago

So in that case, can we close this issue? cc @adelekennedy, @allroundexperts, @stitesExpensify

Expensify / App