korbit-ai[bot] commented on code in PR #35777:
URL: https://github.com/apache/superset/pull/35777#discussion_r2450161487
##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -259,6 +278,13 @@ class ChartRenderer extends Component {
handleLegendStateChanged(legendState) {
this.setState({ legendState });
+ try {
+ setItem(
+ `chart_legend_state_${this.props.chartId}`,
+ JSON.stringify(legendState),
+ );
+ } catch (e) {
+ }
}
Review Comment:
### Mixed Storage and State Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Local storage persistence logic is mixed with component state management,
violating the Single Responsibility Principle.
###### Why this matters
Mixing storage concerns with component logic makes the code less
maintainable and harder to test. It also makes it difficult to change the
storage mechanism in the future.
###### Suggested change ∙ *Feature Preview*
Extract storage logic into a separate service:
```javascript
class ChartLegendStorage {
static saveLegendState(chartId, state) {
return setItem(CHART_LEGEND_KEYS.STATE(chartId), JSON.stringify(state));
}
// ... other methods
}
handleLegendStateChanged(legendState) {
this.setState({ legendState });
ChartLegendStorage.saveLegendState(this.props.chartId, legendState);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ddd0e45-1100-4b65-8707-7605866168b3/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ddd0e45-1100-4b65-8707-7605866168b3?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ddd0e45-1100-4b65-8707-7605866168b3?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ddd0e45-1100-4b65-8707-7605866168b3?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ddd0e45-1100-4b65-8707-7605866168b3)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5f764e68-0210-4c8f-8570-210aef750c6c -->
[](5f764e68-0210-4c8f-8570-210aef750c6c)
##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -88,14 +93,28 @@ 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);
+ if (savedIndex) savedLegendIndex = JSON.parse(savedIndex);
+ } catch (e) {
+ }
Review Comment:
### Blocking localStorage reads in constructor <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Synchronous localStorage operations in constructor block the main thread
during component initialization.
###### Why this matters
This causes UI freezing during chart rendering, especially when multiple
charts are loaded simultaneously or when localStorage contains large amounts of
data.
###### Suggested change ∙ *Feature Preview*
Move localStorage operations to `componentDidMount()` or use asynchronous
patterns with `setTimeout(() => {}, 0)` to defer execution until after initial
render. Consider implementing a cache layer or lazy loading for localStorage
data.
```jsx
componentDidMount() {
this.loadLegendStateAsync();
}
loadLegendStateAsync() {
setTimeout(() => {
try {
const savedState = getItem(legendStateKey);
if (savedState) {
this.setState({ legendState: JSON.parse(savedState) });
}
// Similar for legendIndex
} catch (e) {}
}, 0);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ebbd24bd-11e7-42ef-bdff-46cdc0a48bba/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ebbd24bd-11e7-42ef-bdff-46cdc0a48bba?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ebbd24bd-11e7-42ef-bdff-46cdc0a48bba?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ebbd24bd-11e7-42ef-bdff-46cdc0a48bba?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ebbd24bd-11e7-42ef-bdff-46cdc0a48bba)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:faf16603-10c6-4678-84f8-fe258caea84e -->
[](faf16603-10c6-4678-84f8-fe258caea84e)
--
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]
