alex241728 commented on PR #35777:
URL: https://github.com/apache/superset/pull/35777#issuecomment-3565393727
> 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
@rusackas Our team has used session storage instead of local storage. With
session storage, our team thinks we can git rid of all the functions that
manually check the clean up, so our team deletes all these functions. Now the
pull request is ready for check again.
--
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]
