[HOLD for payment 2024-01-22] [$1000] Desktop - FAB - Modal for 'Send Message' freezes when opened

[lanitochka17]

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:

1. Open the testing desktop app
2. Click on the FAB button
3. Click on Send message
4. Repeat 2-3 times

Expected Result:

Modal opens without any lags or freezes

Actual Result:

Modal freezes when it is opened / sometimes the sliding motion is neglected and it appears immediately without any sliding effect

Workaround:

Unknown

Platforms:

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

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

Version Number: 1.3.78.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

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/78819774/39206801-5320-47fb-9d9c-2347b8476204

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a81bbd7ec35b9394
• Upwork Job ID: 1709988778512769024
• Last Price Increase: 2023-11-06

[muttmuure]

Sorry I missed this last week - I will call it out asking for a volunteer when I do the weekly update this week

[muttmuure]

Is this to do with FlatList specifically?

@TMisiukiewicz and @adhorodyski might have some insights into what is going wrong here

[muttmuure]

Going to try to get the performance update out tonight

[muttmuure]

performance update going out today

[muttmuure]

Update went out - @hurali97 and @kacper-mikolajczak are going to take a look

[hurali97]

Hey 👋

We have some findings from our investigation and we want to share those with you.

• The main outlier is the execution of the useEffect inside NewChatPage even before the animation finishes but since the component is mounted, the effect is called. The ideal way to handle that is to wait for the animation to complete and then execute the function. We have a prop called onEntryTransitionEnd on ScreenWrapper we can use that and move our useEffect body to a new method and pass it to the onEntryTransitionEnd. Then we can also leverage another prop didScreenTransitionEnd from ScreenWrapper and use that to decide to show Skeleton Loader by using isOptionsDataReady && didScreenTransitionEnd. Alternatively, we can solve this by using InteractionManager.runAfterInteractions to wait until the animations are completed.

The above fixes the issue but during the investigation we also found that there are many react commits when we close the Modal and they amount to more than 130+ in numbers, to fix those we suggest following options:

• The Button component re-renders a lot because it's used in OptionRow for the Add To Group UI element. Inside of Button component we have useIsFocused and useActiveElement. Now, since it's a part of the renderItem each time any of those changes, part of our renderItem will render, which is Button and it's children in this case.

  • First improvement is to move this code which is required for keyboard shortcut to outside of Button component and call it something like KeyboardShortcutComponent and in there use useIsFocused and useActiveElement and keyboard shortcut code. This way, we will only re-render KeyboardShortcutComponent and not the whole Button component.

  • Second, we can remove useIsFocused in favour of useFocusEffect and store that boolean value in a ref. The reason here is we are only using isFocused in the keyboardShortcutCallback which will take updated value each time the function is called, so we can safely remove the state dependency on useIsFocused hook.

  • Third, we can refactor the code for useActiveElement to useActiveElementRole because we are only using the role property from it, so there seems no point in storing the whole HTML there. On top of that, since the value from this hook is used to evaluate config for useKeyboardShortcut hook, so we can refactor the state implementation in useActiveElement to ref based.

Lastly, we will also benefit from the improvements in this PR, since it targets the transition improvements for SearchPage on slower devices. For example, BaseOptionsSelector also underperforms in our case, which is optimised in the linked PR. Once that PR is merged, we can simply sync main in our new PR that we will create.

The results below are from the main branch and from all of the improvements mentioned above ( including some parts of the linked PR )

Result from main:

https://github.com/Expensify/App/assets/47336142/b7e2d0d5-3199-45ca-bbff-b44a062dd48a

Result from Improvements:

https://github.com/Expensify/App/assets/47336142/05c5aae9-9918-4546-8f49-9cc7ebe76c89

[lschurr]

What's next on this one @eVoloshchak @muttmuure @Julesssss?

[Julesssss]

@lschurr the PR was just assigned to the C+ for review, so nothing from us yet

[Julesssss]

Seems like we'll need to fix this. Did you see the comment, @hurali97?

[hurali97]

@Julesssss yeah, I will try to fix that by Monday, since we are off tomorrow 👍

[melvin-bot[bot]]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot[bot]]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[shubham1206agra]

Hello @muttmuure @hurali97 @Julesssss @lschurr

I have tried to further optimize @hurali97's proposal here https://github.com/Expensify/App/pull/34480.

I tried to do 2 main things

1. Instead of creating 2 new listener for focusin, and focusout for every use of useActiveElementRole, we are going to create context of activeRole, and will listen only once for events inside the Provider, which will be supplied to App root component directly. The aim was to remove the redundancy of listeners, as we know every button will have these listeners, and there are almost no places on App where we don't have a button.
2. Instead of using the above context for every button rendered, we have used pressOnEnter prop to conditionally render KeyboardShortcutComponent, which will help in less re-render on changing of activeRole.

Now we use state for storing activeRole instead of ref cause this caused a regression https://github.com/Expensify/App/issues/34474, and restoring to using state directly causing some performance lag.

If you agree with me, @hurali97 can you profile https://github.com/Expensify/App/pull/34480 to make sure there is no performance degradation?

[melvin-bot[bot]]

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

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.24-8 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/33055

If no regressions arise, payment will be issued on 2024-01-22. :confetti_ball:

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

• @eVoloshchak requires payment through NewDot Manual Requests
• @hurali97 does not require payment (Contractor)

[melvin-bot[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:

• [ ] [@eVoloshchak] The PR that introduced the bug has been identified. Link to the PR:
• [ ] [@eVoloshchak] 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:
• [ ] [@eVoloshchak] 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:
• [ ] [@eVoloshchak] Determine if we should create a regression test for this bug.
• [ ] [@eVoloshchak] 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.
• [ ] [@lschurr / @muttmuure] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[Julesssss]

oh I missed this notification

[Julesssss]

I have tried to further optimize @hurali97's https://github.com/Expensify/App/issues/28916#issuecomment-1853920181 here https://github.com/Expensify/App/pull/34480.

We're going to go ahead with this change on this linked PR

[muttmuure]

Not overdue

[muttmuure]

Is the problem resolved? Can we go ahead and pay?

[melvin-bot[bot]]

Payment Summary

Upwork Job

• Reviewer: @eVoloshchak owed $500 via NewDot
• Contributor: @hurali97 is from an agency-contributor and not due payment

BugZero Checklist (@lschurr)

• [x] I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• [x] I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1709988778512769024/hired)
• [x] I have paid out the Upwork contracts or cancelled the ones that are incorrect
• [x] I have verified the payment summary above is correct

[lschurr]

This looks correct. @eVoloshchak - you can request payment via newdot.

Closing.

[situchan]

There were several regressions. One is not resolved yet.

• https://github.com/Expensify/App/issues/34367
• https://github.com/Expensify/App/issues/34371
• https://github.com/Expensify/App/issues/34474

[eVoloshchak]

@hurali97, could you take a look at the comment above?

[eVoloshchak]

@hurali97, bump on the above

[eVoloshchak]

@hurali97, bump on the above ignore this, I can see all of the regressions were solved