[POC] OptionListContextProvider optimization#74071
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Code review completed. Found 3 performance-related violations (PERF-6: Hook dependencies). |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
src/pages/NewChatPage.tsx
Outdated
| setSelectedOptions(newSelectedOptions); | ||
| }, [newGroupDraft?.participants, listOptions.personalDetails, personalData.accountID]); | ||
| // We intentionally use listOptions?.personalDetails instead of listOptions to avoid unnecessary re-executions when other properties change | ||
| // eslint-disable-next-line react-compiler/react-compiler |
There was a problem hiding this comment.
We shouldn't integrate any code that breaks it for the compiler. Can you think of another way we can implement it?
There was a problem hiding this comment.
The code has been refactored — the logic is now in a useMemo hook with proper dependencies, and the useEffect includes a comparison to prevent unnecessary updates. The eslint-disable directives have been removed, and the code now follows React best practices.
|
same as above, unrelated |
|
✋ 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/JS00001 in version: 9.2.79-0 🚀
|
|
🚀 Deployed to staging by https://github.com/JS00001 in version: 9.2.81-0 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.2.81-5 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.2.81-5 🚀
|
| if (reportUtilsIsPolicyExpenseChat(report) && !isEmptyObject(iouReport)) { | ||
| // eslint-disable-next-line @typescript-eslint/no-deprecated | ||
| lastMessageTextFromReport = formatReportLastMessageText(getReportName(iouReport)); | ||
| const reportName = computeReportName(iouReport); |
There was a problem hiding this comment.
@martasudol @szymonzalarski98 Can you please use derived value instead of using computeReportName to compute report name from scratch?
There was a problem hiding this comment.
Sure, thanks for the comment
There was a problem hiding this comment.
Today, I'm going to create a proposal for this and create a draft PR
There was a problem hiding this comment.
Please check last comment here, there is my proposal and draft PR #75731 (comment)
| }); | ||
|
|
||
| const [nvpDismissedProductTraining] = useOnyx(ONYXKEYS.NVP_DISMISSED_PRODUCT_TRAINING, {canBeMissing: true}); | ||
| const defaultOptions = useMemo(() => { |
There was a problem hiding this comment.
Coming from this issue #78478 , removing the useMemo for defaultOptions caused the problem. We’ve fixed it in this PR #79283. More context here on proposal: #78478 (comment)
9 participants
Explanation of Change
This PR is a POC for getting rid of OptionListContextProvider usage in the NewChatPage component to reduce lags caused by background re-calculations on every personal details or reports changes.
Fixed Issues
$ #75731
Tests
Proceed with the whole new chat creation flow. There should be no lags.
Offline tests
Same as Tests.
QA Steps
Same as Tests.
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))npm run compress-svg)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_native.mov
Android: mWeb Chrome
android_web.mov
iOS: Native
ios_native.mov
iOS: mWeb Safari
ios_web.mov
MacOS: Chrome / Safari
chrome_web.mov
MacOS: Desktop
desk.mov