perf: move screen focus to LHNOptionsList to avoid re-renders#62968
Conversation
There was a problem hiding this comment.
Fantastic, LGTM!! Thank you for working on this - the perf results look promising❤️
One idea that popped in my head while looking at the changes, we might as an alternative use context instead of props drilling. It would simplify the types (no need to distinguish between isOptionFocused and isScreenFocused because it stays the same).
When looking at it further, it'd be nice for ScreenWrapper to expose that functionality, but that's story for different ticket :) Thanks once again!
src/hooks/useIsScreenFocus.ts
Outdated
| import {useFocusEffect} from '@react-navigation/native'; | ||
| import {useCallback, useState} from 'react'; | ||
|
|
||
| export default function useIsScreenFocusedRef() { |
There was a problem hiding this comment.
Tiny typo useIsScreenFocusedRef -> useIsScreenFocused
There was a problem hiding this comment.
Would it be fine for our use-case (as we are using just state) to go with useIsFocus instead and avoid custom hook altogether?
There was a problem hiding this comment.
Good point. Replaced custom hook with useIsFocused as it has the same behavior but simplifies the code
@kacper-mikolajczak Thank you for the feedback ❤️ . Agree — using context could help avoid the prop drilling and clean up the types a bit. The main reason I went with props here was to keep things a bit more explicit and avoid introducing extra renders that might come from context updates (especially in big lists like LHN). |
|
@rushatgabhane Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
🚧 @mountiny has triggered a test app build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppWhatsApp.Video.2025-05-29.at.21.48.15.mp4Android: mWeb ChromeiOS: HybridAppScreen.Recording.2025-05-30.at.13.00.31.moviOS: mWeb SafariScreen.Recording.2025-05-30.at.13.06.15.movMacOS: Chrome / SafariScreen.Recording.2025-05-30.at.05.10.30.movMacOS: Desktop |
mountiny
left a comment
There was a problem hiding this comment.
Really nice improvement, great job!
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.55-0 🚀
|
|
@OlimpiaZurek Do we have QA steps for this? |
|
In this PR, we moved the focus logic to the parent to avoid unnecessary re-renders, so these QA steps are just to make sure nothing broke with focus-related behavior after the performance tweak: Tooltip Behavior:
Context menu:
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.56-2 🚀
|
|
@OlimpiaZurek Get.mp4 |
|
Based on the code, it shows one of 3 different tooltips depending on specific conditions, so it’s hard to say if the behavior was recently changed or reverted. In this PR, I didn’t touch the tooltip logic—so the only thing that needs to be checked is whether the tooltip still shows up correctly, just to make sure nothing broke as a side effect of the refactor. |
Which tooltip do you mean? |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.58-0 🚀
|
|
confirmed that this caused a regression, so going to revert |
|
Maybe when we redo it we can try to add an automated UI test that covers the regression? Not sure whether gestures like long-press are testable, but if they are it seems like it would be a good test |
|
Thanks for the revert |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.1.58-4 🚀
|
Explanation of Change
Problem
OptionRowLHNuse theuseFocusEffecthook locally inside each row component. This means that every time the screen focus changed (e.g., navigating between Inbox → Reports), all instances of OptionRowLHN triggered re-renders — even if nothing else changed.In large lists (e.g., LHN with hundreds/thousands of rows), this caused significant performance hits and longer render durations.
Solution
Move the screen focus logic to the parent component (
LHNOptionsList) and passedisScreenFocusedvalue as a prop to eachOptionRowLHNinstead of usinguseFocusEffectwithin the row. Replace the custom focus hook with React Navigation’s built-inuseIsFocused()in the parent for better clarity and simplicity. This avoided each row setting up its own focus listener and prevented cascading re-renders on screen transitions. This change preserves the required focus-based behavior while minimizing redundant work across many list items.Performance gain
8941msdown to6328ms-> Total Duration Difference:2613.470 ms)3297msdown to2820ms-> Total Duration Difference:-477.069 ms)Before:
After:
Fixed Issues
$ #60007
PROPOSAL:
Tests
This change is performance-related only — no functionality is expected to change. The goal is to avoid unnecessary re-renders in long LHN lists by lifting useFocusEffect `out of each row and managing focus state at the parent level.
Please check that there are no regressions in the behavior tied to screen focus
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop