-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[CP Staging] Revert "fix: Reports - More menu on expense RHP blocks primary and secondary button." #72070
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
Conversation
mountiny
left a comment
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.
Straight revert
Flaky test failure - straight revert |
| export default PopoverMenu; | ||
| export default React.memo( | ||
| PopoverMenu, | ||
| (prevProps, nextProps) => |
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.
❌ PERF-5 (docs)
Using deepEqual for React.memo comparisons creates performance overhead that often exceeds the re-render cost they aim to prevent. Deep equality checks recursively compare all nested properties.
Consider using shallow comparisons of specific relevant properties instead:
export default React.memo(
PopoverMenu,
(prevProps, nextProps) =>
prevProps.menuItems === nextProps.menuItems &&
prevProps.isVisible === nextProps.isVisible &&
prevProps.anchorPosition === nextProps.anchorPosition &&
// ... other primitive comparisons
);|
@mountiny looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
| export default React.memo( | ||
| PopoverMenu, | ||
| (prevProps, nextProps) => | ||
| deepEqual(prevProps.menuItems, nextProps.menuItems) && |
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.
❌ PERF-5 (docs)
Using deepEqual for comparing menuItems creates unnecessary performance overhead. Since menuItems is likely to be a new array reference on each render, this deep comparison will run on every update.
Consider using a reference equality check or memoizing the menuItems in the parent component:
prevProps.menuItems === nextProps.menuItems…ue/70364 [CP Staging] Revert "fix: Reports - More menu on expense RHP blocks primary and secondary button." (cherry picked from commit 9037835) (cherry-picked to staging by lakchote)
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Cherry-picked to staging by https://github.com/lakchote in version: 9.2.27-2 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.2.27-6 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/lakchote in version: 9.2.28-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.2.28-5 🚀
|
Reverts #71162
Fixes
#72058
#72064