korbit-ai[bot] commented on code in PR #36011:
URL: https://github.com/apache/superset/pull/36011#discussion_r2496528637
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx:
##########
@@ -612,18 +612,18 @@ export default memo(Chart, (prevProps, nextProps) => {
return false;
}
return (
- !nextProps.isComponentVisible ||
- (prevProps.isInView === nextProps.isInView &&
- prevProps.componentId === nextProps.componentId &&
- prevProps.id === nextProps.id &&
- prevProps.dashboardId === nextProps.dashboardId &&
- prevProps.extraControls === nextProps.extraControls &&
- prevProps.handleToggleFullSize === nextProps.handleToggleFullSize &&
- prevProps.isFullSize === nextProps.isFullSize &&
- prevProps.setControlValue === nextProps.setControlValue &&
- prevProps.sliceName === nextProps.sliceName &&
- prevProps.updateSliceName === nextProps.updateSliceName &&
- prevProps.width === nextProps.width &&
- prevProps.height === nextProps.height)
+ prevProps.isComponentVisible === nextProps.isComponentVisible &&
+ prevProps.isInView === nextProps.isInView &&
+ prevProps.componentId === nextProps.componentId &&
+ prevProps.id === nextProps.id &&
+ prevProps.dashboardId === nextProps.dashboardId &&
+ prevProps.extraControls === nextProps.extraControls &&
+ prevProps.handleToggleFullSize === nextProps.handleToggleFullSize &&
+ prevProps.isFullSize === nextProps.isFullSize &&
+ prevProps.setControlValue === nextProps.setControlValue &&
+ prevProps.sliceName === nextProps.sliceName &&
+ prevProps.updateSliceName === nextProps.updateSliceName &&
+ prevProps.width === nextProps.width &&
+ prevProps.height === nextProps.height
Review Comment:
### Inefficient function reference comparison in memo <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The memo comparison function performs shallow equality checks on function
references (handleToggleFullSize, setControlValue, updateSliceName) which will
cause unnecessary re-renders when parent components recreate these functions.
###### Why this matters
Function references change on every parent render unless wrapped in
useCallback, causing the memo to return false and trigger re-renders even when
the actual functionality hasn't changed, negating the performance benefits of
memoization.
###### Suggested change ∙ *Feature Preview*
Remove function reference comparisons from the memo comparison or ensure
parent components wrap these functions in useCallback with stable dependencies:
```jsx
return (
prevProps.isComponentVisible === nextProps.isComponentVisible &&
prevProps.isInView === nextProps.isInView &&
prevProps.componentId === nextProps.componentId &&
prevProps.id === nextProps.id &&
prevProps.dashboardId === nextProps.dashboardId &&
prevProps.extraControls === nextProps.extraControls &&
prevProps.isFullSize === nextProps.isFullSize &&
prevProps.sliceName === nextProps.sliceName &&
prevProps.width === nextProps.width &&
prevProps.height === nextProps.height
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b2448a1e-a5af-45a6-b7fd-26604149bc65/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b2448a1e-a5af-45a6-b7fd-26604149bc65?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b2448a1e-a5af-45a6-b7fd-26604149bc65?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b2448a1e-a5af-45a6-b7fd-26604149bc65?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b2448a1e-a5af-45a6-b7fd-26604149bc65)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d94af292-ca27-4705-be70-f888de13c943 -->
[](d94af292-ca27-4705-be70-f888de13c943)
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx:
##########
@@ -612,18 +612,18 @@ export default memo(Chart, (prevProps, nextProps) => {
return false;
}
return (
- !nextProps.isComponentVisible ||
- (prevProps.isInView === nextProps.isInView &&
- prevProps.componentId === nextProps.componentId &&
- prevProps.id === nextProps.id &&
- prevProps.dashboardId === nextProps.dashboardId &&
- prevProps.extraControls === nextProps.extraControls &&
- prevProps.handleToggleFullSize === nextProps.handleToggleFullSize &&
- prevProps.isFullSize === nextProps.isFullSize &&
- prevProps.setControlValue === nextProps.setControlValue &&
- prevProps.sliceName === nextProps.sliceName &&
- prevProps.updateSliceName === nextProps.updateSliceName &&
- prevProps.width === nextProps.width &&
- prevProps.height === nextProps.height)
+ prevProps.isComponentVisible === nextProps.isComponentVisible &&
Review Comment:
### Memo logic causes re-renders for invisible components <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The memoization logic now includes isComponentVisible in the equality check,
but when a component becomes invisible (isComponentVisible changes from true to
false), the memo function will return false, causing a re-render. However, the
component should skip rendering when it's not visible to optimize performance.
###### Why this matters
This change defeats the performance optimization purpose of the memo. When
charts become invisible, they will still re-render instead of being skipped,
potentially causing unnecessary computation and performance degradation in
dashboards with many charts.
###### Suggested change ∙ *Feature Preview*
Restore the original logic that returns true (skip re-render) when the
component is not visible:
```jsx
export default memo(Chart, (prevProps, nextProps) => {
if (prevProps.cacheBusterProp !== nextProps.cacheBusterProp) {
return false;
}
return (
!nextProps.isComponentVisible ||
(prevProps.isInView === nextProps.isInView &&
prevProps.componentId === nextProps.componentId &&
prevProps.id === nextProps.id &&
prevProps.dashboardId === nextProps.dashboardId &&
prevProps.extraControls === nextProps.extraControls &&
prevProps.handleToggleFullSize === nextProps.handleToggleFullSize &&
prevProps.isFullSize === nextProps.isFullSize &&
prevProps.setControlValue === nextProps.setControlValue &&
prevProps.sliceName === nextProps.sliceName &&
prevProps.updateSliceName === nextProps.updateSliceName &&
prevProps.width === nextProps.width &&
prevProps.height === nextProps.height)
);
});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/04f2d84f-2c7f-45a7-8f56-3f522c6ecbed/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/04f2d84f-2c7f-45a7-8f56-3f522c6ecbed?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/04f2d84f-2c7f-45a7-8f56-3f522c6ecbed?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/04f2d84f-2c7f-45a7-8f56-3f522c6ecbed?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/04f2d84f-2c7f-45a7-8f56-3f522c6ecbed)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:c3ca235c-ed6a-4ecb-a4ce-3dbc1fb62657 -->
[](c3ca235c-ed6a-4ecb-a4ce-3dbc1fb62657)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
