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
Bug 1795707: improve notification drawer performance #4192
Bug 1795707: improve notification drawer performance #4192
Conversation
@spadgett: This pull request references Bugzilla bug 1795707, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/hold cancel |
3ad4ad5
to
59f458a
Compare
}; | ||
}; | ||
|
||
const notificationStateToProps = ({ UI }: RootState): WithNotificationsProps => ({ | ||
isDrawerExpanded: !!UI.getIn(['notifications', 'isExpanded']), | ||
notificationsRead: !!UI.getIn(['notifications', 'isRead']), | ||
alerts: UI.getIn(['monitoring', 'notificationAlerts']) || {}, |
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.
Returning a different empty object each time caused unnecessary renders.
} | ||
}, [criticalAlertList, isAlertExpanded, isDrawerExpanded, prevDrawerToggleState]); |
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.
criticalAlertList
was different every render, so this always fired
@@ -14,11 +14,12 @@ if [ -d "$ARTIFACT_DIR" ]; then | |||
cp public/dist/report.html "${ARTIFACT_DIR}" | |||
fi | |||
|
|||
MAX_BYTES=2621440 # ~2.5 MiB |
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.
You can't do floating point calculations in bash (2.5 * 1024 * 1024), so I just hard-coded the value
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.
@spadgett i ran into this too...was wondering how you wanted this to be modified...
59f458a
to
7d04c61
Compare
* Don't lazy-load component since it will always load * Fix problem where `useEffect` hook constantly fires * Don't return a new object each call to `notificationStateToProps` * Fix incorrect FirehoseResult type
7d04c61
to
e7694b5
Compare
Rebased, @jcaianirh @benjaminapetersen another look? |
/retest |
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.
lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcaianirh, spadgett The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
6 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
useEffect
hook constantly firesnotificationStateToProps
when alerts are empty/assign @jcaianirh @benjaminapetersen