Copilot commented on code in PR #35777:
URL: https://github.com/apache/superset/pull/35777#discussion_r2519204754
##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -274,6 +388,20 @@ class ChartRenderer extends Component {
handleLegendScroll(legendIndex) {
this.setState({ legendIndex });
+ try {
+ setItem(
+ `chart_legend_index_${this.props.chartId}`,
+ JSON.stringify(legendIndex),
+ );
Review Comment:
The `setItem` function from localStorageHelpers is typed to work with the
`LocalStorageKeys` enum and expects a properly typed value. Using it with a
dynamic string key like `chart_legend_index_${this.props.chartId}` bypasses
type safety. Consider using `dangerouslySetItemDoNotUse` directly or adding
chart legend keys to the LocalStorageKeys enum, or use `localStorage.setItem`
directly for these dynamic keys.
##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -261,6 +367,14 @@ class ChartRenderer extends Component {
handleLegendStateChanged(legendState) {
this.setState({ legendState });
+ try {
+ setItem(
+ `chart_legend_state_${this.props.chartId}`,
+ JSON.stringify(legendState),
+ );
Review Comment:
The `setItem` function from localStorageHelpers is typed to work with the
`LocalStorageKeys` enum and expects a properly typed value. Using it with a
dynamic string key like `chart_legend_state_${this.props.chartId}` bypasses
type safety. Consider using `dangerouslySetItemDoNotUse` directly or adding
chart legend keys to the LocalStorageKeys enum, or use `localStorage.setItem`
directly for these dynamic keys.
##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -89,16 +153,39 @@ class ChartRenderer extends Component {
const suppressContextMenu = getChartMetadataRegistry().get(
props.formData.viz_type ?? props.vizType,
)?.suppressContextMenu;
+
+ // Load legend state from localStorage (per-chart)
+ const legendStateKey = `chart_legend_state_${props.chartId}`;
+ const legendIndexKey = `chart_legend_index_${props.chartId}`;
+ let savedLegendState;
+ let savedLegendIndex = 0;
+ try {
+ const savedState = getItem(legendStateKey);
+ if (savedState) savedLegendState = JSON.parse(savedState);
+ const savedIndex = getItem(legendIndexKey);
Review Comment:
The `getItem` function from localStorageHelpers is typed to work with the
`LocalStorageKeys` enum. Using it with dynamic string keys like
`chart_legend_state_${props.chartId}` bypasses type safety. Consider using
`dangerouslyGetItemDoNotUse` or `localStorage.getItem` directly for these
dynamic keys.
```suggestion
const savedState = localStorage.getItem(legendStateKey);
if (savedState) savedLegendState = JSON.parse(savedState);
const savedIndex = localStorage.getItem(legendIndexKey);
```
##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -136,6 +223,25 @@ class ChartRenderer extends Component {
this.mutableQueriesResponse = cloneDeep(this.props.queriesResponse);
}
+ componentDidMount() {
+ // Only set up global listeners once (shared across all ChartRenderer
instances)
+ // Cleans up ALL chart legend states even charts out of view and unmounted
+ if (!window.__chartLegendCleanupListenersSetup) {
+ // Listen for browser navigation (close tab, navigate away)
+ window.addEventListener('beforeunload', globalBeforeUnloadHandler);
+ // Listen for browser back/forward navigation
+ window.addEventListener('popstate', globalRouteChangeHandler);
+ // Listen for hash changes (manual URL changes)
+ window.addEventListener('hashchange', globalRouteChangeHandler);
+ // Check periodically to catch route changes (including SPA navigation
via tabs/links)
+ window.__chartLegendCleanupInterval = setInterval(
+ globalRouteChangeHandler,
+ 100,
+ );
+ window.__chartLegendCleanupListenersSetup = true;
+ }
Review Comment:
The event listeners are never removed, which creates a memory leak. Even
though they're set up only once globally, they will persist for the entire
browser session even after all ChartRenderer components are unmounted. Consider
adding cleanup logic, perhaps in a separate cleanup function or by tracking
when the last ChartRenderer unmounts.
##########
superset-frontend/src/components/Chart/ChartRenderer.test.jsx:
##########
@@ -286,3 +295,276 @@ test('should detect nested matrixify property changes',
() => {
JSON.stringify(updatedProps.formData),
);
});
+
+// localStorage legend state tests
+let beforeUnloadHandlers = [];
+let popstateHandlers = [];
+let hashchangeHandlers = [];
+let originalLocation;
+let originalAddEventListener;
+let originalRemoveEventListener;
+
+beforeEach(() => {
+ localStorage.clear();
+ capturedSuperChartProps = null;
+ resetGlobalCleanupPathname();
+
+ window.__chartLegendCleanupListenersSetup = false;
+ if (window.__chartLegendCleanupInterval) {
+ clearInterval(window.__chartLegendCleanupInterval);
+ delete window.__chartLegendCleanupInterval;
+ }
+
+ // Reset handlers
+ beforeUnloadHandlers = [];
+ popstateHandlers = [];
+ hashchangeHandlers = [];
+
+ // Save original location
+ originalLocation = window.location;
+
+ // Mock window.location.pathname
+ delete window.location;
+ window.location = {
+ pathname: '/dashboard/1',
+ href: 'http://localhost/dashboard/1',
+ };
+
+ // Mock addEventListener to capture handlers
+ const originalAddEventListenerImpl = window.addEventListener;
+ originalAddEventListener = jest.spyOn(window, 'addEventListener');
+ originalAddEventListener.mockImplementation((event, handler) => {
+ if (event === 'beforeunload') {
+ beforeUnloadHandlers.push(handler);
+ } else if (event === 'popstate') {
+ popstateHandlers.push(handler);
+ } else if (event === 'hashchange') {
+ hashchangeHandlers.push(handler);
+ }
+ originalAddEventListenerImpl.call(window, event, handler);
+ });
+
+ // Spy on removeEventListener to preserve original functionality
+ originalRemoveEventListener = jest.spyOn(window, 'removeEventListener');
+
+ // Use fake timers for interval tests
+ jest.useFakeTimers();
+});
+
+afterEach(() => {
+ // Restore
+ window.location = originalLocation;
+ if (
+ originalAddEventListener &&
+ typeof originalAddEventListener.mockRestore === 'function'
+ ) {
+ originalAddEventListener.mockRestore();
+ }
+ if (
+ originalRemoveEventListener &&
+ typeof originalRemoveEventListener.mockRestore === 'function'
+ ) {
+ originalRemoveEventListener.mockRestore();
+ }
+
+ // Clear intervals and storage
+ if (window.__chartLegendCleanupInterval) {
+ clearInterval(window.__chartLegendCleanupInterval);
+ delete window.__chartLegendCleanupInterval;
+ }
+ localStorage.clear();
+
+ window.__chartLegendCleanupListenersSetup = false;
+
+ // Restore timers
+ jest.useRealTimers();
+});
+
+test('should save legend state to localStorage when handleLegendStateChanged
is called', () => {
+ const mockLegendState = { series1: true, series2: false };
+ const legendStateKey = `chart_legend_state_${requiredProps.chartId}`;
+ expect(localStorage.getItem(legendStateKey)).toBeNull();
+
+ render(<ChartRenderer {...requiredProps} />);
+
+ // Verify SuperChart received the hooks prop with the handler
+ expect(capturedSuperChartProps).not.toBeNull();
+ expect(capturedSuperChartProps.hooks).toBeDefined();
+ expect(capturedSuperChartProps.hooks.onLegendStateChanged).toBeDefined();
+
+ capturedSuperChartProps.hooks.onLegendStateChanged(mockLegendState);
+
+ // Verify it was saved to localStorage
+ const saved = JSON.parse(JSON.parse(localStorage.getItem(legendStateKey)));
Review Comment:
Double `JSON.parse` will fail because `localStorage.getItem` returns a
string. Since the test is calling the handler with `mockLegendState` which is
already an object, and the handler calls `JSON.stringify(legendState)` once,
only one `JSON.parse` is needed here.
##########
superset-frontend/src/components/Chart/ChartRenderer.test.jsx:
##########
@@ -286,3 +295,276 @@ test('should detect nested matrixify property changes',
() => {
JSON.stringify(updatedProps.formData),
);
});
+
+// localStorage legend state tests
+let beforeUnloadHandlers = [];
+let popstateHandlers = [];
+let hashchangeHandlers = [];
+let originalLocation;
+let originalAddEventListener;
+let originalRemoveEventListener;
+
+beforeEach(() => {
+ localStorage.clear();
+ capturedSuperChartProps = null;
+ resetGlobalCleanupPathname();
+
+ window.__chartLegendCleanupListenersSetup = false;
+ if (window.__chartLegendCleanupInterval) {
+ clearInterval(window.__chartLegendCleanupInterval);
+ delete window.__chartLegendCleanupInterval;
+ }
+
+ // Reset handlers
+ beforeUnloadHandlers = [];
+ popstateHandlers = [];
+ hashchangeHandlers = [];
+
+ // Save original location
+ originalLocation = window.location;
+
+ // Mock window.location.pathname
+ delete window.location;
+ window.location = {
+ pathname: '/dashboard/1',
+ href: 'http://localhost/dashboard/1',
+ };
+
+ // Mock addEventListener to capture handlers
+ const originalAddEventListenerImpl = window.addEventListener;
+ originalAddEventListener = jest.spyOn(window, 'addEventListener');
+ originalAddEventListener.mockImplementation((event, handler) => {
+ if (event === 'beforeunload') {
+ beforeUnloadHandlers.push(handler);
+ } else if (event === 'popstate') {
+ popstateHandlers.push(handler);
+ } else if (event === 'hashchange') {
+ hashchangeHandlers.push(handler);
+ }
+ originalAddEventListenerImpl.call(window, event, handler);
+ });
+
+ // Spy on removeEventListener to preserve original functionality
+ originalRemoveEventListener = jest.spyOn(window, 'removeEventListener');
+
+ // Use fake timers for interval tests
+ jest.useFakeTimers();
+});
+
+afterEach(() => {
+ // Restore
+ window.location = originalLocation;
+ if (
+ originalAddEventListener &&
+ typeof originalAddEventListener.mockRestore === 'function'
+ ) {
+ originalAddEventListener.mockRestore();
+ }
+ if (
+ originalRemoveEventListener &&
+ typeof originalRemoveEventListener.mockRestore === 'function'
+ ) {
+ originalRemoveEventListener.mockRestore();
+ }
+
+ // Clear intervals and storage
+ if (window.__chartLegendCleanupInterval) {
+ clearInterval(window.__chartLegendCleanupInterval);
+ delete window.__chartLegendCleanupInterval;
+ }
+ localStorage.clear();
+
+ window.__chartLegendCleanupListenersSetup = false;
+
+ // Restore timers
+ jest.useRealTimers();
+});
+
+test('should save legend state to localStorage when handleLegendStateChanged
is called', () => {
+ const mockLegendState = { series1: true, series2: false };
+ const legendStateKey = `chart_legend_state_${requiredProps.chartId}`;
+ expect(localStorage.getItem(legendStateKey)).toBeNull();
+
+ render(<ChartRenderer {...requiredProps} />);
+
+ // Verify SuperChart received the hooks prop with the handler
+ expect(capturedSuperChartProps).not.toBeNull();
+ expect(capturedSuperChartProps.hooks).toBeDefined();
+ expect(capturedSuperChartProps.hooks.onLegendStateChanged).toBeDefined();
+
+ capturedSuperChartProps.hooks.onLegendStateChanged(mockLegendState);
+
+ // Verify it was saved to localStorage
+ const saved = JSON.parse(JSON.parse(localStorage.getItem(legendStateKey)));
+ expect(saved).toEqual(mockLegendState);
+});
+
+test('should save legend index to localStorage when handleLegendScroll is
called', () => {
+ const mockLegendIndex = 5;
+ const legendIndexKey = `chart_legend_index_${requiredProps.chartId}`;
+
+ expect(localStorage.getItem(legendIndexKey)).toBeNull();
+
+ render(<ChartRenderer {...requiredProps} />);
+
+ // Verify SuperChart received the hooks prop with the handler
+ expect(capturedSuperChartProps).not.toBeNull();
+ expect(capturedSuperChartProps.hooks).toBeDefined();
+ expect(capturedSuperChartProps.hooks.onLegendScroll).toBeDefined();
+
+ capturedSuperChartProps.hooks.onLegendScroll(mockLegendIndex);
+
+ // Verify it was saved to localStorage
+ const saved = JSON.parse(JSON.parse(localStorage.getItem(legendIndexKey)));
Review Comment:
Double `JSON.parse` will fail because `localStorage.getItem` returns a
string. Since the handler calls `JSON.stringify(legendIndex)` once, only one
`JSON.parse` is needed here.
```suggestion
const saved = JSON.parse(localStorage.getItem(legendIndexKey));
```
##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -32,8 +32,72 @@ import {
import { Logger, LOG_ACTIONS_RENDER_CHART } from 'src/logger/LogUtils';
import { EmptyState } from '@superset-ui/core/components';
import { ChartSource } from 'src/types/ChartSource';
+import { setItem, getItem } from 'src/utils/localStorageHelpers';
import ChartContextMenu from './ChartContextMenu/ChartContextMenu';
+// Global pathname tracker to detect page navigation
+let globalCleanupPathname = '';
+
+/**
+ * Clean up ALL chart legend states from localStorage (including unmounted
charts)
+ */
+export function cleanupAllChartLegendStates() {
+ try {
+ // Get all localStorage keys
+ const keysToRemove = [];
+ for (let i = 0; i < localStorage.length; i += 1) {
+ const key = localStorage.key(i);
+ if (
+ key &&
+ (key.startsWith('chart_legend_state_') ||
+ key.startsWith('chart_legend_index_'))
+ ) {
+ keysToRemove.push(key);
+ }
+ }
+ // Remove all chart legend state keys
+ keysToRemove.forEach(key => {
+ localStorage.removeItem(key);
+ });
+ } catch (e) {
+ console.warn(
+ 'Failed to clean up all chart legend states from localStorage:',
Review Comment:
[nitpick] The error handling uses `console.warn` here but other error
handlers in the same file use `console.warn` with slightly different message
formats. For consistency and better debugging, consider using a structured
logging approach or at least maintaining consistent message formatting across
all error handlers.
```suggestion
logging.warn(
'[ChartRenderer] Failed to clean up all chart legend states from
localStorage:',
```
##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -136,6 +223,25 @@ class ChartRenderer extends Component {
this.mutableQueriesResponse = cloneDeep(this.props.queriesResponse);
}
+ componentDidMount() {
+ // Only set up global listeners once (shared across all ChartRenderer
instances)
+ // Cleans up ALL chart legend states even charts out of view and unmounted
+ if (!window.__chartLegendCleanupListenersSetup) {
+ // Listen for browser navigation (close tab, navigate away)
+ window.addEventListener('beforeunload', globalBeforeUnloadHandler);
+ // Listen for browser back/forward navigation
+ window.addEventListener('popstate', globalRouteChangeHandler);
+ // Listen for hash changes (manual URL changes)
+ window.addEventListener('hashchange', globalRouteChangeHandler);
+ // Check periodically to catch route changes (including SPA navigation
via tabs/links)
+ window.__chartLegendCleanupInterval = setInterval(
+ globalRouteChangeHandler,
+ 100,
Review Comment:
Running `setInterval` at 100ms (10 times per second) is very aggressive and
could impact browser performance, especially with multiple chart instances.
Consider increasing the interval to at least 500-1000ms, or use a more
efficient approach like listening to router change events if using a SPA router.
```suggestion
1000, // Increased from 100ms to 1000ms to reduce performance impact
```
--
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]
