-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Fix #21022] Improve performance by reducing re-renders SidebarLinks #21406
Merged
mountiny
merged 62 commits into
Expensify:main
from
margelo:perf/21022-sidebarlinks-performance
Jul 19, 2023
Merged
Changes from all commits
Commits
Show all changes
62 commits
Select commit
Hold shift + click to select a range
82974c6
Connect OptionRowLHN manually
hannojg eada423
don't rerender SidebarLinks just because modal opens
hannojg c31f7de
Remove currentReportId from shouldReportBeInOptionList
hannojg 32c2b70
add active report functionality back
hannojg 6a38ee8
optimize optionRowLHN to not depend on current report id
hannojg a651118
fix
hannojg c8ea565
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg 99c8a1d
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg 27f8eb0
fix issues after merge
hannojg f3a39b6
fix import issues
hannojg 99d2d40
change back name
hannojg 4bb3a19
remove currentReportId from params
hannojg 8bfad23
wip: data wrapper component
hannojg 0bdd608
logging
hannojg 4d9bb25
wip: fixing tests
hannojg 640753c
add currentReportID back
hannojg a84e26f
wip(test): simpler context usage
hannojg b7cb24d
wip: tests
hannojg cdb46d6
wip: tests
hannojg 69e46ae
include lastReadTime in wrapper component as its needed to compute GS…
hannojg 4128c82
fiz style in focus mode
hannojg 8a1a5c2
fix SidebarFilterTests
hannojg f0d54a7
wip: fixed all tests temporarily
hannojg 7c5521a
removed focused index as its obsolete
hannojg 8e4f2c2
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg 657fad8
removed logging statements
hannojg bd086ab
split up components + use deepEqual in react memo
hannojg 6c296ad
fix prop types
hannojg 8625724
cleaned up tests
hannojg db85064
fix prop types
hannojg 0afc57e
fixed last onyx data connections
hannojg 1a88a51
clean
hannojg b9c6c0f
wip: moving comment logic over to data component
hannojg f6d7bd0
fix option row not upating when personal details changing
hannojg 3b5bd35
Update src/libs/SidebarUtils.js
hannojg 4a8f494
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg c53cf6d
fix issues after merge
hannojg 00a4b5c
dont use this
hannojg 26e661a
removed unused imports
hannojg 448878d
fix comment
hannojg 7bc90eb
commented tests
hannojg 336d5fd
inline skeleton
hannojg 8180e87
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg 5c35629
destructure props
hannojg a73fa9c
fix highlight active chat
hannojg c4fed2b
add displayName and iouReportAmount for sorting
hannojg 2a73540
include total, remove displayName
hannojg de7209f
added other properties considered for sorting
hannojg de5fe1d
use prop spreaing
hannojg ecf4c0a
move navigation focus to sidebarlinksdata
hannojg b76a569
removed debug comment
hannojg c0fba28
remove withNavigation usage
hannojg 31a1167
add other missing fields
hannojg f2f3c9d
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg 0bb1e62
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg 36c8913
Merge branch 'main' of github.com:margelo/expensify-app-fork into per…
hannojg 478288d
removed unused onLayout prop
hannojg 3d60095
Memoize OptionRowLHNData as well
hannojg 9f88371
fix lint
hannojg e25fd20
Update src/components/LHNOptionsList/OptionRowLHNData.js
hannojg 17ed2a1
Update src/libs/ReportActionsUtils.js
hannojg 9ba0811
change comment format
hannojg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,142 @@ | ||
import {withOnyx} from 'react-native-onyx'; | ||
import lodashGet from 'lodash/get'; | ||
import _ from 'underscore'; | ||
import PropTypes from 'prop-types'; | ||
import React, {useEffect, useRef, useMemo} from 'react'; | ||
import {deepEqual} from 'fast-equals'; | ||
import {withReportCommentDrafts} from '../OnyxProvider'; | ||
import SidebarUtils from '../../libs/SidebarUtils'; | ||
import compose from '../../libs/compose'; | ||
import ONYXKEYS from '../../ONYXKEYS'; | ||
import withCurrentReportID, {withCurrentReportIDPropTypes, withCurrentReportIDDefaultProps} from '../withCurrentReportID'; | ||
import OptionRowLHN, {propTypes as basePropTypes, defaultProps as baseDefaultProps} from './OptionRowLHN'; | ||
import * as Report from '../../libs/actions/Report'; | ||
import * as UserUtils from '../../libs/UserUtils'; | ||
import participantPropTypes from '../participantPropTypes'; | ||
import CONST from '../../CONST'; | ||
|
||
const propTypes = { | ||
/** If true will disable ever setting the OptionRowLHN to focused */ | ||
shouldDisableFocusOptions: PropTypes.bool, | ||
|
||
/** List of users' personal details */ | ||
personalDetails: PropTypes.objectOf(participantPropTypes), | ||
|
||
/** The preferred language for the app */ | ||
preferredLocale: PropTypes.string, | ||
|
||
/** The full data of the report */ | ||
// eslint-disable-next-line react/forbid-prop-types | ||
fullReport: PropTypes.object, | ||
|
||
...withCurrentReportIDPropTypes, | ||
...basePropTypes, | ||
}; | ||
|
||
const defaultProps = { | ||
shouldDisableFocusOptions: false, | ||
personalDetails: {}, | ||
fullReport: {}, | ||
preferredLocale: CONST.LOCALES.DEFAULT, | ||
...withCurrentReportIDDefaultProps, | ||
...baseDefaultProps, | ||
}; | ||
|
||
/* | ||
* This component gets the data from onyx for the actual | ||
* OptionRowLHN component. | ||
* The OptionRowLHN component is memoized, so it will only | ||
* re-render if the data really changed. | ||
*/ | ||
function OptionRowLHNData({shouldDisableFocusOptions, currentReportID, fullReport, personalDetails, preferredLocale, comment, ...propsToForward}) { | ||
const reportID = propsToForward.reportID; | ||
// We only want to pass a boolean to the memoized component, | ||
// instead of a changing number (so we prevent unnecessary re-renders). | ||
const isFocused = !shouldDisableFocusOptions && currentReportID === reportID; | ||
|
||
const optionItemRef = useRef(); | ||
const optionItem = useMemo(() => { | ||
// Note: ideally we'd have this as a dependent selector in onyx! | ||
const item = SidebarUtils.getOptionData(fullReport, personalDetails, preferredLocale); | ||
if (deepEqual(item, optionItemRef.current)) { | ||
return optionItemRef.current; | ||
} | ||
optionItemRef.current = item; | ||
return item; | ||
}, [fullReport, preferredLocale, personalDetails]); | ||
|
||
useEffect(() => { | ||
if (!optionItem || optionItem.hasDraftComment || !comment || comment.length <= 0 || isFocused) { | ||
return; | ||
} | ||
Report.setReportWithDraft(reportID, true); | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, []); | ||
|
||
return ( | ||
<OptionRowLHN | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
{...propsToForward} | ||
isFocused={isFocused} | ||
optionItem={optionItem} | ||
/> | ||
); | ||
} | ||
|
||
OptionRowLHNData.propTypes = propTypes; | ||
OptionRowLHNData.defaultProps = defaultProps; | ||
OptionRowLHNData.displayName = 'OptionRowLHNData'; | ||
|
||
/** | ||
* @param {Object} [personalDetails] | ||
* @returns {Object|undefined} | ||
*/ | ||
const personalDetailsSelector = (personalDetails) => | ||
_.reduce( | ||
personalDetails, | ||
(finalPersonalDetails, personalData, accountID) => { | ||
// It's OK to do param-reassignment in _.reduce() because we absolutely know the starting state of finalPersonalDetails | ||
// eslint-disable-next-line no-param-reassign | ||
finalPersonalDetails[accountID] = { | ||
accountID: Number(accountID), | ||
login: personalData.login, | ||
displayName: personalData.displayName, | ||
firstName: personalData.firstName, | ||
avatar: UserUtils.getAvatar(personalData.avatar, personalData.accountID), | ||
}; | ||
return finalPersonalDetails; | ||
}, | ||
{}, | ||
); | ||
|
||
/** | ||
* This component is rendered in a list. | ||
* On scroll we want to avoid that a item re-renders | ||
* just because the list has to re-render when adding more items. | ||
* Thats also why the React.memo is used on the outer component here, as we just | ||
* use it to prevent re-renders from parent re-renders. | ||
*/ | ||
export default React.memo( | ||
compose( | ||
withCurrentReportID, | ||
withReportCommentDrafts({ | ||
propName: 'comment', | ||
transformValue: (drafts, props) => { | ||
const draftKey = `${ONYXKEYS.COLLECTION.REPORT_DRAFT_COMMENT}${props.reportID}`; | ||
return lodashGet(drafts, draftKey, ''); | ||
}, | ||
}), | ||
withOnyx({ | ||
fullReport: { | ||
key: (props) => ONYXKEYS.COLLECTION.REPORT + props.reportID, | ||
}, | ||
personalDetails: { | ||
key: ONYXKEYS.PERSONAL_DETAILS_LIST, | ||
selector: personalDetailsSelector, | ||
}, | ||
preferredLocale: { | ||
key: ONYXKEYS.NVP_PREFERRED_LOCALE, | ||
}, | ||
}), | ||
)(OptionRowLHNData), | ||
); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This default is causing the deploy blocker here: #23202
If
allReportActionsParam
is not passed, it has value{}
which is truthy, so we never useallReportActions
. TheallReportActionsParam || allReportActions
doesn't work as intended.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks you for quickly fixing this!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix