apache
fix(chart): Fix the problem that chart legend resets after user scrolls down
User description
SUMMARY
When there are changes in selection of categories or change of legend page, it will be stored locally with localStorageHelpers. When the charts are rendered again, it should first check if these states of the legend are already in local storage, otherwise it would initialize them as it was before.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
- Create a dashboard long enough that you have to scroll down about three or more viewport heights away
- Create a line chart with legends on
- Select any category, using inv or all or only one
- Scroll down until you can't see the chart anymore
- Go back to it, the state of the legend will remain the same
ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #31741
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in SIP-59)
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
CodeAnt-AI Description
Persist chart legend selection and page across scrolls using session storage
What Changed
- Legend selections and the legend page index are saved per chart to sessionStorage and restored when the chart is re-rendered or scrolled back into view
- Charts no longer lose the user's legend selection or current legend page after the chart is unmounted or scrolled out of view during the same browser session
- Unit tests added to verify saving and loading of legend state and legend index
Impact
✅ Legend selection preserved after scrolling
✅ Legend page preserved when charts re-render or return into view
✅ Restored legend state within the same browser session
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
@codeant-ai ask: Your question here
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
@codeant-ai ask: Can you suggest a safer alternative to storing this secret?
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
@codeant-ai: Your feedback here
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
@codeant-ai: Do not flag unused imports.
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
@codeant-ai: review
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Interaction Diagram by Bito
sequenceDiagram
participant USER as User
participant DASH as Dashboard
participant CHART as Chart Component
participant RENDERER as ChartRenderer<br/>🔄 Updated | ●●● High
participant STORAGE as LocalStorage Helpers<br/>🔄 Updated | ●●○ Medium
participant SUPER as SuperChart
participant BROWSER as Browser Storage
Note over RENDERER: Legend persistence added<br/>Saves state & scroll position
USER->>DASH: Interact with dashboard
DASH->>CHART: Render chart component
CHART->>RENDERER: Initialize with chartId
RENDERER->>STORAGE: getItem(legend keys)
STORAGE->>BROWSER: Retrieve saved legend state
BROWSER-->>STORAGE: Return legend data
STORAGE-->>RENDERER: Provide saved state
RENDERER->>SUPER: Render with legend state
SUPER-->>RENDERER: Legend state changed
RENDERER->>STORAGE: setItem(legend state)
STORAGE->>BROWSER: Persist legend changes
Critical path: Dashboard->Chart Component->ChartRenderer->LocalStorage Helpers->Browser Storage
Note: ChartRenderer now persists legend state and scroll position to localStorage per chart ID. This enables legend state to survive page refreshes and navigation, improving user experience by maintaining chart legend preferences across sessions.
![bito-code-review[bot] avatar](https://avatars.githubusercontent.com/in/1061978?v=4 "Published by a pub.dev verified publisher"){kind=small}
Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following:
pip3 install -r requirements/development.txt
pre-commit install
A series of checks will now run when you make a git commit.
Alternatively it is possible to run pre-commit by running pre-commit manually:
pre-commit run --all-files
Thank you @rusackas for letting us know about Git pre-commit!
All checks have passed for me. Please let me know if there's anything else on anyone else's end or would like us to change.
Hi, I have provided a solution of clearing the chart legend data in localStorage when the user leaves the dashboard. There is now use of console.warn() as I seen in other files for error handling. @sadpandajoe there are also tests now for the issue fix, and for clearing the data in localStorage as mentioned.
Please let us know if there is anything else!
Hi @rusackas,
I have as copilot suggested:
- switched to using localStorage.setItem() and localStorage.getItem()
- Increased Interval to 1000ms
- Created a way to delete event listeners when the last ChartRenderer unmounts
- Removed the double JSON.parse() (Tests still pass as expected)
- Converted the console.warn() to logging.warn()
Thanks for taking care of so many of the review comments.
Added a comment elsewhere, but in re-reviewing this, I'm noticing that a lot of JSON.parse is being used here for direct localstorage access. You can slim down the code (and prevent things found by the bot reviews) by using localStorageHelpers as mentioned in your PR description.
As you'll spot in other areas of the codebase, you can import the bits you need like
import {
LocalStorageKeys,
getItem,
setItem,
} from 'src/utils/localStorageHelpers';
and then use setItem and getItem to not worry about the JSON bits, and provide defaults as needed.
Hi @rusackas,
I have went ahead and removed the line mentioned before in the package-lock.json file since it is not needed.
I was also able to slim down on the code by using the localStorageHelpers. You're right and I do not have to worry about JSON bits!
Please let me know if theres anything else
Hey again,
I was looking at this a bit over the weekend, and wondering if this is the right approach for this, really. It seems that there are a few things going on which might be problematic:
- The polling, and using the window object, rather than using react router for these state changes
- Using localstorage rather than sessionstorage... it seems the latter might actually be more applicable here and provide less bloat.
- Just unsure of the event listeners in general and whether that's the right angle.
Had a discussion with Copilot today about this (not my preferred agent, but it's here in GitHub) and it suggested to avoid per-component global listeners and polling by handling cleanup at the router level using React Router's location change detection. That keeps ChartRenderer focused on per-chart state (save/load to localStorage or sessionStorage) and puts any cross-chart cleanup in one tiny place that only runs when the app actually navigates.
This would: • Keep ChartRenderer's per-chart save/load behavior (setItem/getItem) as-is. Remove the global counters, interval, and add/removeEventListener logic from ChartRenderer. • Add a single small component mounted once inside the Router that runs cleanup when the location pathname changes (useLocation / history.listen). No polling, no per-instance listeners, only one effect.
I'll paste it's suggestions in another comment for your consideration...
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
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 PatchSummary 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.
Heya! This is MUCH cleaner! Might you (and your team) have time to actually sync up on this? I spoke with some other reviewers on zoom, and the consenus thought is that this does fix the accute issue with minimal code/impact, but is not the right generalized approach for the same sort of issue with other charts.
For example, there are other chart interactions that can be done (scrolling in a table, searching/sorting or rearranging columns in a table), adjusting the zoom/scrubber on bar/line charts, zooming on maps, etc. I hope to find time to test this (you're welcome to, too!) but probably ALL of these "chart state interaction" details are lost when scrolling through a dashboard and charts get de-rendered and re-rendered.
What might be the right way to do this is to generalize the approach. I'm not sure where the right place is to grab "all chart state" - maybe it's to clone the props for a chart as an object, whatever they may be (legend, search, column order, whatever). Then that entire object could be saved as state when it's de-rendered by dashboard virtualization (de-rendering/re-rendering). It could be saved as Dashboard state, or in sessionStorage, so long as that part of sessionStorage is cleared when moving away from the dashboard, with a React Router hook.
Hope that all makes sense? Again, if not, happy to talk about this more realtime (zoom, a Slack channel for the project, etc).
![codeant-ai-for-open-source[bot] avatar](https://avatars.githubusercontent.com/u/151810221?v=4 "Published by a pub.dev verified publisher"){kind=small}