[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 [email protected] 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
• 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

Bug6225888_1696511150527.Screen_Recording_2023-10-05_at_1.45.06_PM.mp4

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

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• 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
• 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.
• 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.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01a81bbd7ec35b9394

[melvin-bot]

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

[AmjedNazzal]

Is this only on staging with beta? I can't repo on dev with beta on

[studentofcoding]

Proposal

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

On Desktop - FAB - Modal for 'Send Message' freezes when opened

What is the root cause of that problem?

The root cause of the problem is due to heavy rendering on src/pages/NewChatSelectorPage.js when the modal is opened that causing animation delay in the rendering process.

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

We should optimize the rendering of the components in the NewChatSelectorPage modal. via lazy loading NewChatPage to load the component only when needed.

Here below is the example code, with the result

import React, {lazy} from 'react';
//.. Other codes

// Lazy load NewChatPage, so the animation is smoother and load the data after Modal is opened
const NewChatPage = lazy(() => import('./NewChatPage'));

//.. Other codes

function NewChatSelectorPage(props) {
    return (
        <ScreenWrapper
            shouldEnableKeyboardAvoidingView={false}
            includeSafeAreaPaddingBottom={false}
            shouldEnableMaxHeight
            testID={NewChatSelectorPage.displayName}
        >
            <HeaderWithBackButton title={props.translate('sidebarScreen.fabNewChat')} />
            {Permissions.canUsePolicyRooms(props.betas) ? (
                <OnyxTabNavigator
                    id={CONST.TAB.NEW_CHAT_TAB_ID}
                    tabBar={({state, navigation, position}) => (
                        <TabSelector
                            state={state}
                            navigation={navigation}
                            position={position}
                        />
                    )}
                >
                    <TopTab.Screen
                        name={CONST.TAB.NEW_CHAT}
                        component={() => (
                            //.. Use Suspense to make sure that the component is loaded
                            //.. we can use another component as the fallback as well
                            <Suspense fallback={null}>
                                <NewChatPage />
                            </Suspense>
                        )}
                    />
                    <TopTab.Screen
                        name={CONST.TAB.NEW_ROOM}
                        component={WorkspaceNewRoomPage}
                    />
                </OnyxTabNavigator>
            ) : (
                //.. Use Suspense to make sure that the component is loaded
                //.. we can use another component as the fallback as well
                <Suspense fallback={null}>
                    <NewChatPage />
                </Suspense>
            )}
        </ScreenWrapper>
    );
}

NewChatSelectorPage.propTypes = propTypes;
NewChatSelectorPage.defaultProps = defaultProps;
NewChatSelectorPage.displayName = 'NewChatPage';

export default compose(
    withLocalize,
    withWindowDimensions,
    withOnyx({
        betas: {key: ONYXKEYS.BETAS},
    }),
)(NewChatSelectorPage);

Result

lag_fixed.mp4

What alternative solutions did you explore? (Optional)

NA

@eVoloshchak @lschurr

[melvin-bot]

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

[eVoloshchak]

Is this only on staging with beta? I can't repo on dev with beta on

@AmjedNazzal, it is reproducible on dev on my end

Screen.Recording.2023-10-09.at.15.10.36.mov

[eVoloshchak]

The root cause of the problem is due to heavy rendering on src/pages/NewChatSelectorPage.js

@studentofcoding, lazy loading the component would fix the symptom of the problem, but not the root cause.
What is particularly heavy about this page? What's causing those heavy renders?

[studentofcoding]

The root cause of the problem is due to heavy rendering on src/pages/NewChatSelectorPage.js

@studentofcoding, lazy loading the component would fix the symptom of the problem, but not the root cause.

What is particularly heavy about this page? What's causing those heavy renders?

It's on Search List that get fetch & rendered everytime we open the modal @eVoloshchak

[eVoloshchak]

But we don't have the same problem when opening the search page (CMD+K). Is it due to skeleton UI?

[studentofcoding]

But we don't have the same problem when opening the search page (CMD+K). Is it due to skeleton UI?

Yes it's @eVoloshchak

[lschurr]

Any update here @eVoloshchak?

[melvin-bot]

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

[melvin-bot]

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

[eVoloshchak]

Not overdue, this is awaiting more proposals
I think we need to dig deeper to figure out and fix the actual root cause for this

[melvin-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]

⚠️ 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 #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 #34474, and restoring to using state directly causing some performance lag.

If you agree with me, @hurali97 can you profile #34480 to make sure there is no performance degradation?

[melvin-bot]

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

[melvin-bot]

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

• perf: optimise transition on Desktop when click on start chat #33055

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

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]

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 #28916 (comment) here #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]

Payment Summary

Upwork Job

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

BugZero Checklist (@lschurr)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1709988778512769024/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• 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.

• [$500] [HOLD for payment 2024-01-24] [HOLD for payment 2024-01-22] Web - Request money - The 'Enter' button doesn't work for the confirmation money page #34367
• [HOLD for payment 2024-01-24] [HOLD for payment 2024-01-22] FAB - Skeleton appears first on Start chat page #34371
• [$500] Error shows up whenever tried to go back using keyboard #34474

[eVoloshchak]

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

[eVoloshchak]

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

[JmillsExpensify]

$500 approved for @eVoloshchak