[PAY 23/02] [Performance] Make the Search Sidebar transition relatively smooth on slower devices

[mountiny]

### Problem

coming from here

When user navigates back from Search Sidebar there’s a noticeable delay and lag in closing the Search Sidebar. It mainly arises from withNavigationFocus HOC as it re-renders the BaseOptionSelector tree including OptionsList . Adding a memo to OptionsList doesn’t help as it’s parent gets re-rendered due to the withNavigationFocus HOC.

Also, we notice that due to isFocused state value in ActionItemView the ReportActionsList re-renders which also adds the overhead to the rendering cycle as it recreates the instances of recordTimeToMeasureItemLayout and loadOlderChats which are passed as props to ReportActionsList .

Solution

We can remove the withNavigationFocus HOC and add navigation listeners(focus, blur) in BaseOptionSelector and memoize OptionsListto not re-render when state is set in it’s parent. We can avoid recreating instances of recordTimeToMeasureItemLayout and loadOlderChats by wrapping them in useCallbacks and memoizing ReportActionsList .

[melvin-bot]

Triggered auto assignment to @bfitzexpensify (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

[mountiny]

@hurali97 Could you comment on this one?

[mountiny]

Pr is close to be merged

[melvin-bot]

@mountiny, @bfitzexpensify, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

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

[hurali97]

Hey @mountiny 👋

[hurali97]

@hurali97 Could you comment on this one?

Is there something else should I comment ? or just the above is fine :D

[mountiny]

Haha all good i just cannot assign external people who did not comment on the issue ;)

[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.

[mountiny]

PR was reverted because there was couple of regressions with lists, we need to address them in a new PR

[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.

[situchan]

This is not ready for payment yet. 2nd PR is in review

[adelekennedy]

huh - updating the title

[situchan]

Please remove HOLD for payment from issue title

[mountiny]

Not overdue, pr is at works

[mountiny]

PR is at works

[melvin-bot]

@mountiny, @bfitzexpensify, @hurali97, @adelekennedy, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

[situchan]

PR is in review

[melvin-bot]

@mountiny, @bfitzexpensify, @hurali97, @adelekennedy, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

[situchan]

still in review

[bfitzexpensify]

Looks like the PR has hit prod - #34422.

I'm not sure the automation will work here, so updating the title manually.

As for payment - given the original PR was reverted due to regressions, it seems like the payout here would be $250.

Agree with that @mountiny?

[mountiny]

I would say that this was a complex change to test and the regressions were very hard to catch, I would still payout $500 to @situchan as this was challenging task taking lots of time. @hurali97 is from agency.

[bfitzexpensify]

Cool, sounds good to me. Offer sent @situchan

[bfitzexpensify]

Payment due Friday

[bfitzexpensify]

Payment complete - @situchan can you please complete the BZ checklist? Will copy it into a new comment

[bfitzexpensify]

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:

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

[situchan]

This is performance improvement, not bug. BZ Checklist doesn't apply here.

[muttmuure]

I think this can be closed

[melvin-bot]

@mountiny, @bfitzexpensify, @hurali97, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!