rusackas commented on PR #35777:
URL: https://github.com/apache/superset/pull/35777#issuecomment-3543374655
OK, here's the big diff it provided. It claims this will do what I was
mentioning, and
• Keep chart localStorage persistence per chart
• Clean up all chart legend state keys on SPA route navigation and browser
close, in a single place
• Reduce code complexity and improve testability
```
*** Update File: superset-frontend/src/components/Chart/ChartRenderer.jsx
@@
-import ChartContextMenu from './ChartContextMenu/ChartContextMenu';
+import ChartContextMenu from './ChartContextMenu/ChartContextMenu';
+import { getItem, setItem } from 'src/utils/localStorageHelpers';
@@
- this.state = {
- showContextMenu:
- props.source === ChartSource.Dashboard &&
- !suppressContextMenu &&
- isFeatureEnabled(FeatureFlag.DrillToDetail),
- inContextMenu: false,
- legendState: undefined,
- legendIndex: 0,
- };
+ // Load per-chart legend persistence from localStorage (if present)
+ const legendStateKey = `chart_legend_state_${props.chartId}`;
+ const legendIndexKey = `chart_legend_index_${props.chartId}`;
+ const savedLegendState = getItem(legendStateKey, undefined);
+ const savedLegendIndex = getItem(legendIndexKey, 0);
+
+ this.state = {
+ showContextMenu:
+ props.source === ChartSource.Dashboard &&
+ !suppressContextMenu &&
+ isFeatureEnabled(FeatureFlag.DrillToDetail),
+ inContextMenu: false,
+ legendState: savedLegendState,
+ legendIndex: savedLegendIndex,
+ };
@@
handleLegendStateChanged(legendState) {
- this.setState({ legendState });
+ this.setState({ legendState });
+ try {
+ setItem(`chart_legend_state_${this.props.chartId}`, legendState);
+ } catch (e) {
+ // swallow errors to avoid breaking charts if localStorage is
unavailable
+ }
}
@@
handleLegendScroll(legendIndex) {
- this.setState({ legendIndex });
+ this.setState({ legendIndex });
+ try {
+ setItem(`chart_legend_index_${this.props.chartId}`, legendIndex);
+ } catch (e) {
+ // swallow errors to avoid breaking charts if localStorage is
unavailable
+ }
}
*** End Patch
*** Add File: superset-frontend/src/components/Chart/legendStateUtils.ts
+export function cleanupAllChartLegendStates() {
+ try {
+ 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);
+ }
+ }
+ keysToRemove.forEach(k => localStorage.removeItem(k));
+ } catch (e) {
+ // intentionally no-op; callers shouldn't fail app when cleanup can't
run
+ }
+}
*** End Patch
*** Add File: superset-frontend/src/components/Chart/RouteLegendCleanup.tsx
+import React, { useEffect, useRef } from 'react';
+import { useLocation } from 'react-router-dom';
+import { cleanupAllChartLegendStates } from './legendStateUtils';
+
+export default function RouteLegendCleanup() {
+ const location = useLocation();
+ const prevPath = useRef(location.pathname);
+
+ useEffect(() => {
+ if (prevPath.current !== location.pathname) {
+ cleanupAllChartLegendStates();
+ prevPath.current = location.pathname;
+ }
+ }, [location.pathname]);
+
+ useEffect(() => {
+ const onUnload = () => cleanupAllChartLegendStates();
+ window.addEventListener('beforeunload', onUnload);
+ return () => window.removeEventListener('beforeunload', onUnload);
+ }, []);
+
+ return null;
+}
*** End Patch
*** Update File: superset-frontend/src/views/App.tsx
@@
-import { ScrollToTop } from './ScrollToTop';
+import { ScrollToTop } from './ScrollToTop';
+import RouteLegendCleanup from 'src/components/Chart/RouteLegendCleanup';
@@
<ScrollToTop />
<LocationPathnameLogger />
+ <RouteLegendCleanup />
*** End Patch
```
Summary
This refactor centralizes all chart legend localStorage cleanup on SPA
navigation and browser exit, removes per-component global listeners/polling,
and keeps ChartRenderer focused on per-chart persistence.
It adds a new router-level cleanup component, making legend state cleanup
more maintainable, efficient, and easier to test.
What's changed:
ChartRenderer.jsx now only loads/saves legend state/index for each chart to
localStorage
Removes all global listeners, polling, and window globals from ChartRenderer
Adds RouteLegendCleanup component (using react-router v5's useLocation) that
triggers cleanup when route changes and on browser unload
Cleanup helper (legendStateUtils.ts) is now used by both the router watcher
and tests
Mounts RouteLegendCleanup once inside Router in App.tsx (after
LocationPathnameLogger)
See patch for all unified changes
Why this is better:
No polling, no multiple global listeners
Cleanup runs only once on navigation and browser unload
Per-chart persistence is simple (no more component tracking/counting)
Easier to test (single cleanup, focused responsibilities, less time-based
logic)
Testing:
ChartRenderer persists/loads legend state/index to localStorage as before
RouteLegendCleanup runs cleanup only when navigating between pages
(pathnames) and once on browser unload
You can unit test RouteLegendCleanup with a MemoryRouter and assert cleanup
calls
--
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]
