-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Update Sidebar to only listen to specific properties on Onyx connections to speed up performance #12083
Merged
Merged
Update Sidebar to only listen to specific properties on Onyx connections to speed up performance #12083
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
cc01592
Record all the properties being accessed
tgolen 15e57cd
Merge branch 'main' into tgolen-onyx-specific-properties
tgolen d9da250
Utilize property selectors for sidebar links
tgolen f895c38
Merge branch 'main' into tgolen-onyx-specific-properties
tgolen b5734f3
Fix lint
tgolen 96868db
Update onyx version
tgolen 7af6253
Remove comments about which properties are used
tgolen 8ff25f2
Remove empty line
tgolen a1e720f
Add some properties to the report to listen to
tgolen a6dce11
Update Onyx to the latest version to get tests passing
tgolen d5fc838
Merge branch 'main' into tgolen-onyx-specific-properties
tgolen 3feacfb
Merge branch 'main' into tgolen-onyx-specific-properties
tgolen 7c90bd9
Move selector functions above the withOnyx definition
tgolen fe739ad
Merge branch 'main' into tgolen-onyx-specific-properties
tgolen 0abba4a
Update lock file after fixing conflict
tgolen ee2eddb
Merge branch 'main' into tgolen-onyx-specific-properties
tgolen 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -164,6 +164,65 @@ class SidebarLinks extends React.Component { | |
SidebarLinks.propTypes = propTypes; | ||
SidebarLinks.defaultProps = defaultProps; | ||
|
||
/** | ||
* This function (and the few below it), narrow down the data from Onyx to just the properties that we want to trigger a re-render of the component. This helps minimize re-rendering | ||
* and makes the entire component more performant because it's not re-rendering when a bunch of properties change which aren't ever used in the UI. | ||
* @param {Object} [report] | ||
* @returns {Object|undefined} | ||
*/ | ||
const reportSelector = report => report && ({ | ||
reportID: report.reportID, | ||
participants: report.participants, | ||
hasDraft: report.hasDraft, | ||
isPinned: report.isPinned, | ||
errorFields: { | ||
addWorkspaceRoom: report.errorFields && report.errorFields.addWorkspaceRoom, | ||
}, | ||
maxSequenceNumber: report.maxSequenceNumber, | ||
lastReadSequenceNumber: report.lastReadSequenceNumber, | ||
lastMessageText: report.lastMessageText, | ||
lastMessageTimestamp: report.lastMessageTimestamp, | ||
iouReportID: report.iouReportID, | ||
hasOutstandingIOU: report.hasOutstandingIOU, | ||
statusNum: report.statusNum, | ||
stateNum: report.stateNum, | ||
chatType: report.chatType, | ||
policyID: report.policyID, | ||
}); | ||
|
||
/** | ||
* @param {Object} [personalDetails] | ||
* @returns {Object|undefined} | ||
*/ | ||
const personalDetailsSelector = personalDetails => _.reduce(personalDetails, (finalPersonalDetails, personalData, login) => { | ||
// 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[login] = { | ||
login: personalData.login, | ||
displayName: personalData.displayName, | ||
firstName: personalData.firstName, | ||
avatar: personalData.avatar, | ||
}; | ||
return finalPersonalDetails; | ||
}, {}); | ||
|
||
/** | ||
* @param {Object} [reportActions] | ||
* @returns {Object|undefined} | ||
*/ | ||
const reportActionsSelector = reportActions => reportActions && _.map(reportActions, reportAction => ({ | ||
errors: reportAction.errors, | ||
})); | ||
|
||
/** | ||
* @param {Object} [policy] | ||
* @returns {Object|undefined} | ||
*/ | ||
const policySelector = policy => policy && ({ | ||
type: policy.type, | ||
name: policy.name, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We missed listening for avatar property on policies which caused #18226 |
||
}); | ||
|
||
export default compose( | ||
withLocalize, | ||
withCurrentUserPersonalDetails, | ||
|
@@ -176,9 +235,11 @@ export default compose( | |
// with 10,000 withOnyx() connections, it would have unknown performance implications. | ||
reports: { | ||
key: ONYXKEYS.COLLECTION.REPORT, | ||
selector: reportSelector, | ||
}, | ||
personalDetails: { | ||
key: ONYXKEYS.PERSONAL_DETAILS, | ||
selector: personalDetailsSelector, | ||
}, | ||
priorityMode: { | ||
key: ONYXKEYS.NVP_PRIORITY_MODE, | ||
|
@@ -188,9 +249,11 @@ export default compose( | |
}, | ||
reportActions: { | ||
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS, | ||
selector: reportActionsSelector, | ||
}, | ||
policies: { | ||
key: ONYXKEYS.COLLECTION.POLICY, | ||
selector: policySelector, | ||
}, | ||
preferredLocale: { | ||
key: ONYXKEYS.NVP_PREFERRED_LOCALE, | ||
|
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.
Hey 👋, this PR caused this regression where room names are not updated in the sidebar when renamed while offline. The problem is that
reportName
was not included in the reportSelector.