-
Notifications
You must be signed in to change notification settings - Fork 16.1k
fix(dashboard): ensure charts re-render when visibility state changes #36011
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
base: master
Are you sure you want to change the base?
fix(dashboard): ensure charts re-render when visibility state changes #36011
Conversation
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.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
| Memo logic causes re-renders for invisible components ▹ view | ||
| Inefficient function reference comparison in memo ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| prevProps.updateSliceName === nextProps.updateSliceName && | ||
| prevProps.width === nextProps.width && | ||
| prevProps.height === nextProps.height) | ||
| prevProps.isComponentVisible === nextProps.isComponentVisible && |
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.
Memo logic causes re-renders for invisible components 
Tell me more
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:
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
💬 Looking for more details? Reply to this comment to chat with Korbit.
| 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 |
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.
Inefficient function reference comparison in memo 
Tell me more
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:
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
💬 Looking for more details? Reply to this comment to chat with Korbit.
SUMMARY
Fixes an intermittent bug where dashboard charts get stuck in loading state with async queries enabled in Superset 6.x. The issue was introduced in PR #31241 where a performance optimization in the Chart component's
React.memocomparison incorrectly prevented re-renders when theisComponentVisibleprop changed.Root Cause:
The memo comparison function had a flawed escape hatch (
!nextProps.isComponentVisible) that would skip ALL re-renders for invisible charts, regardless of whether other props changed. Additionally,isComponentVisibleitself was not included in the prop comparison list, so visibility changes (false→true or true→false) were never detected.The Fix:
Changed the memo comparison to explicitly check
isComponentVisiblelike any other prop:!nextProps.isComponentVisible || (/* other props */)prevProps.isComponentVisible === nextProps.isComponentVisible && (/* other props */)This ensures charts re-render when: