codeant-ai-for-open-source[bot] commented on code in PR #35777:
URL: https://github.com/apache/superset/pull/35777#discussion_r2594427181
##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -89,14 +89,37 @@ class ChartRenderer extends Component {
const suppressContextMenu = getChartMetadataRegistry().get(
props.formData.viz_type ?? props.vizType,
)?.suppressContextMenu;
+
+ // Load legend state from sessionStorage (per-chart)
+ const legendStateKey = `chart_legend_state_${props.chartId}`;
+ const legendIndexKey = `chart_legend_index_${props.chartId}`;
+ let savedLegendState = null;
+ let savedLegendIndex = 0;
+ try {
+ const storedState = sessionStorage.getItem(legendStateKey);
+ if (storedState !== null) {
+ savedLegendState = JSON.parse(storedState);
+ }
+ const storedIndex = sessionStorage.getItem(legendIndexKey);
+ if (storedIndex !== null) {
+ console.log(storedIndex);
+ savedLegendIndex = JSON.parse(storedIndex);
+ }
+ } catch (e) {
+ logging.warn(
+ '[ChartRenderer] Failed to load legend state from sessionStorage:',
+ e,
+ );
Review Comment:
**Suggestion:** Accessing browser-only APIs like `sessionStorage` during
construction can throw in non-browser environments (SSR) or if storage is
disabled; guard access with a `typeof window !== 'undefined'` check and remove
the stray `console.log` left in the PR to avoid noisy/leaking debug output.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
if (typeof window !== 'undefined' && window.sessionStorage) {
try {
const storedState = sessionStorage.getItem(legendStateKey);
if (storedState !== null) {
savedLegendState = JSON.parse(storedState);
}
const storedIndex = sessionStorage.getItem(legendIndexKey);
if (storedIndex !== null) {
savedLegendIndex = JSON.parse(storedIndex);
}
} catch (e) {
logging.warn(
'[ChartRenderer] Failed to load legend state from sessionStorage:',
e,
);
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The suggestion is valid: referencing sessionStorage/session in SSR or locked
environments can throw. Although the current try/catch will catch runtime
errors, adding a typeof window check avoids triggering ReferenceError in the
first place and is defensive for SSR. Removing the stray console.log is
definitely desirable to avoid noisy debug output.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Chart/ChartRenderer.jsx
**Line:** 98:112
**Comment:**
*Possible Bug: Accessing browser-only APIs like `sessionStorage` during
construction can throw in non-browser environments (SSR) or if storage is
disabled; guard access with a `typeof window !== 'undefined'` check and remove
the stray `console.log` left in the PR to avoid noisy/leaking debug output.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -261,6 +284,17 @@ class ChartRenderer extends Component {
handleLegendStateChanged(legendState) {
this.setState({ legendState });
+ try {
+ sessionStorage.setItem(
+ `chart_legend_state_${this.props.chartId}`,
+ JSON.stringify(legendState),
+ );
+ } catch (e) {
+ logging.warn(
+ '[ChartRenderer] Failed to save legend state to sessionStorage:',
+ e,
+ );
Review Comment:
**Suggestion:** Calling `sessionStorage.setItem` without confirming
`window`/`sessionStorage` exists can throw in SSR or restricted environments;
guard the write with a check for `typeof window !== 'undefined' &&
window.sessionStorage` before attempting to set the item. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
if (typeof window !== 'undefined' && window.sessionStorage) {
try {
sessionStorage.setItem(
`chart_legend_state_${this.props.chartId}`,
JSON.stringify(legendState),
);
} catch (e) {
logging.warn(
'[ChartRenderer] Failed to save legend state to sessionStorage:',
e,
);
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Guarding writes to sessionStorage with a window/sessionStorage existence
check is a safe, low-risk improvement. The current try/catch will handle
failures, but the guard avoids referencing a missing global in SSR and makes
intent explicit.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Chart/ChartRenderer.jsx
**Line:** 287:296
**Comment:**
*Possible Bug: Calling `sessionStorage.setItem` without confirming
`window`/`sessionStorage` exists can throw in SSR or restricted environments;
guard the write with a check for `typeof window !== 'undefined' &&
window.sessionStorage` before attempting to set the item.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/components/Chart/ChartRenderer.jsx:
##########
@@ -274,6 +308,17 @@ class ChartRenderer extends Component {
handleLegendScroll(legendIndex) {
this.setState({ legendIndex });
+ try {
+ sessionStorage.setItem(
+ `chart_legend_index_${this.props.chartId}`,
+ JSON.stringify(legendIndex),
+ );
+ } catch (e) {
+ logging.warn(
+ '[ChartRenderer] Failed to save legend index to sessionStorage:',
+ e,
+ );
Review Comment:
**Suggestion:** Like the legend state write, `sessionStorage.setItem` in
`handleLegendScroll` should be guarded by a check for `window`/`sessionStorage`
to avoid runtime errors in non-browser or restricted contexts. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
if (typeof window !== 'undefined' && window.sessionStorage) {
try {
sessionStorage.setItem(
`chart_legend_index_${this.props.chartId}`,
JSON.stringify(legendIndex),
);
} catch (e) {
logging.warn(
'[ChartRenderer] Failed to save legend index to sessionStorage:',
e,
);
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Same rationale as the previous suggestion — adding a typeof
window/sessionStorage guard before calling setItem is a prudent defensive
change for SSR and restricted environments. It isn't strictly required because
of the try/catch, but it clarifies intent and prevents unnecessary exceptions.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Chart/ChartRenderer.jsx
**Line:** 311:320
**Comment:**
*Possible Bug: Like the legend state write, `sessionStorage.setItem` in
`handleLegendScroll` should be guarded by a check for `window`/`sessionStorage`
to avoid runtime errors in non-browser or restricted contexts.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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]
