codeant-ai-for-open-source[bot] commented on code in PR #36708:
URL: https://github.com/apache/superset/pull/36708#discussion_r2666801865
##########
superset-frontend/src/components/Chart/chartAction.js:
##########
@@ -579,7 +580,7 @@ export function redirectSQLLab(formData, history) {
}
export function refreshChart(chartKey, force, dashboardId) {
- return (dispatch, getState) => {
+ return async (dispatch, getState) => {
const chart = (getState().charts || {})[chartKey];
const timeout =
Review Comment:
**Suggestion:** `chart` may be undefined when reading
`chart.latestQueryFormData`, causing a runtime TypeError; add an existence
check for `chart` before accessing its properties. [null pointer]
**Severity Level:** Minor ⚠️
```suggestion
if (!chart) {
return;
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code can throw when `chart` is undefined because it dereferences
`chart.latestQueryFormData`. Adding an existence check (`if (!chart)
return;`)
prevents a runtime TypeError. The proposed change is small, correct, and
low-risk.
</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/chartAction.js
**Line:** 585:585
**Comment:**
*Null Pointer: `chart` may be undefined when reading
`chart.latestQueryFormData`, causing a runtime TypeError; add an existence
check for `chart` before accessing its properties.
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/chartAction.js:
##########
@@ -590,9 +591,30 @@ export function refreshChart(chartKey, force, dashboardId)
{
) {
return;
}
- dispatch(
+
+ // Fetch the latest chart definition from the database to ensure we use
+ // the most recent configuration (e.g., if the chart was modified in
another tab)
+ let formDataToUse = chart.latestQueryFormData;
+ try {
+ const queryParams = rison.encode({
+ columns: ['params'],
+ });
+ const response = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${chart.id}?q=${queryParams}`,
+ });
+ formDataToUse = JSON.parse(response.json.result.params);
Review Comment:
**Suggestion:** The SupersetClient.get response is accessed as
`response.json.result.params`, but elsewhere the client returns a `{ json }`
object and `json.result` can be either an array or object; this may cause
undefined access or wrong parsing — destructure `{ json }` and handle both
array/object `result` shapes before JSON.parse. [type error]
**Severity Level:** Minor ⚠️
```suggestion
const { json } = await SupersetClient.get({
endpoint: `/api/v1/chart/${chart.id}?q=${queryParams}`,
});
const chartResult = Array.isArray(json.result) ? json.result[0] :
json.result;
if (chartResult && chartResult.params) {
formDataToUse = JSON.parse(chartResult.params);
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The SupersetClient response shape can vary; destructuring `{ json }` and
handling array vs object `result` before parsing `params` is safer and
prevents undefined access or parse errors. The improved code is a correct,
defensive fix.
</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/chartAction.js
**Line:** 602:605
**Comment:**
*Type Error: The SupersetClient.get response is accessed as
`response.json.result.params`, but elsewhere the client returns a `{ json }`
object and `json.result` can be either an array or object; this may cause
undefined access or wrong parsing — destructure `{ json }` and handle both
array/object `result` shapes before JSON.parse.
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/explore/exploreUtils/index.js:
##########
@@ -20,6 +20,7 @@
import { useCallback, useEffect } from 'react';
/* eslint camelcase: 0 */
import URI from 'urijs';
+import rison from 'rison';
Review Comment:
**Suggestion:** Unused import: `rison` is imported but not used anywhere in
this file, which introduces dead code, may fail linting (unused-vars) and
increases bundle size; remove the import if it's not needed. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The final file state shows `rison` is imported but never referenced
anywhere. Removing the import fixes an actual problem (unused import -> linter
warning, tiny unnecessary bundle cost) and is directly applicable to the diff.
This is a simple, correct cleanup.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/exploreUtils/index.js
**Line:** 23:23
**Comment:**
*Possible Bug: Unused import: `rison` is imported but not used anywhere
in this file, which introduces dead code, may fail linting (unused-vars) and
increases bundle size; remove the import if it's not needed.
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/chartActions.test.js:
##########
@@ -502,4 +502,143 @@ describe('chart actions timeout', () => {
expect(postSpy).toHaveBeenCalledWith(expectedPayload);
});
+
+ // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
+ describe('refreshChart', () => {
+ const chartKey = 1;
+ const chartId = 123;
+ const dashboardId = 456;
+ const mockLatestQueryFormData = {
+ datasource: '1__table',
+ viz_type: 'big_number_total',
+ slice_id: chartId,
+ };
+
+ const mockFreshFormData = {
+ ...mockLatestQueryFormData,
+ metric: { aggregate: 'COUNT_DISTINCT' },
+ };
+
+ const mockStateWithChart = {
+ charts: {
+ [chartKey]: {
+ id: chartId,
+ latestQueryFormData: mockLatestQueryFormData,
+ },
+ },
+ dashboardInfo: {
+ common: {
+ conf: {
+ SUPERSET_WEBSERVER_TIMEOUT: 60,
+ },
+ },
+ },
+ dataMask: {
+ [chartId]: {
+ ownState: {},
+ },
+ },
+ };
+
+ const chartApiEndpoint = `glob:*/api/v1/chart/${chartId}*`;
+
+ beforeEach(() => {
+ fetchMock.reset();
+ fetchMock.get(
+ chartApiEndpoint,
+ {
+ result: {
+ params: JSON.stringify(mockFreshFormData),
+ },
+ },
+ { overwriteRoutes: true },
+ );
+ });
+
+ afterEach(() => {
+ fetchMock.reset();
Review Comment:
**Suggestion:** Similarly, `fetchMock.reset()` in the `afterEach` for this
describe block removes configured routes (not just call history), which can
cause subsequent tests to run without expected mocks and fail intermittently;
replace it with `fetchMock.resetHistory()` to avoid wiping route definitions.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
fetchMock.resetHistory();
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Same reasoning as above: wiping route definitions in afterEach can remove
globally-needed stubs and introduce flakiness. Using resetHistory() preserves
route configuration while clearing call logs.
</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/chartActions.test.js
**Line:** 559:559
**Comment:**
*Possible Bug: Similarly, `fetchMock.reset()` in the `afterEach` for
this describe block removes configured routes (not just call history), which
can cause subsequent tests to run without expected mocks and fail
intermittently; replace it with `fetchMock.resetHistory()` to avoid wiping
route definitions.
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/chartReducer.ts:
##########
@@ -64,6 +64,9 @@ export default function chartReducer(
chartAlert: null,
queriesResponse: action.queriesResponse,
chartUpdateEndTime: now(),
+ // Update form_data to match latestQueryFormData so dashboard charts
+ // render with the correct configuration after refresh
+ form_data: state.latestQueryFormData,
Review Comment:
**Suggestion:** Null/undefined risk: `state.latestQueryFormData` may be
undefined for some charts; assigning it directly to `form_data` can produce an
undefined value where code expects an object—use a nullish/default fallback so
`form_data` is always an object (or explicit null). [null pointer]
**Severity Level:** Minor ⚠️
```suggestion
form_data: state.latestQueryFormData ?? {},
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a reasonable defensive change — state.latestQueryFormData could be
undefined in some edge cases, and coalescing to an empty object avoids passing
undefined into consumers that expect an object. The fallback is low-risk and
fixes a plausible runtime issue. Note the repo's default chart sets
latestQueryFormData to {} so this may be redundant in many cases, but the
change is still sensible defensive coding.
</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/chartReducer.ts
**Line:** 69:69
**Comment:**
*Null Pointer: Null/undefined risk: `state.latestQueryFormData` may be
undefined for some charts; assigning it directly to `form_data` can produce an
undefined value where code expects an object—use a nullish/default fallback so
`form_data` is always an object (or explicit null).
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/chartActions.test.js:
##########
@@ -502,4 +502,143 @@ describe('chart actions timeout', () => {
expect(postSpy).toHaveBeenCalledWith(expectedPayload);
});
+
+ // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
+ describe('refreshChart', () => {
+ const chartKey = 1;
+ const chartId = 123;
+ const dashboardId = 456;
+ const mockLatestQueryFormData = {
+ datasource: '1__table',
+ viz_type: 'big_number_total',
+ slice_id: chartId,
+ };
+
+ const mockFreshFormData = {
+ ...mockLatestQueryFormData,
+ metric: { aggregate: 'COUNT_DISTINCT' },
+ };
+
+ const mockStateWithChart = {
+ charts: {
+ [chartKey]: {
+ id: chartId,
+ latestQueryFormData: mockLatestQueryFormData,
+ },
+ },
+ dashboardInfo: {
+ common: {
+ conf: {
+ SUPERSET_WEBSERVER_TIMEOUT: 60,
+ },
+ },
+ },
+ dataMask: {
+ [chartId]: {
+ ownState: {},
+ },
+ },
+ };
+
+ const chartApiEndpoint = `glob:*/api/v1/chart/${chartId}*`;
+
+ beforeEach(() => {
+ fetchMock.reset();
Review Comment:
**Suggestion:** The test uses `fetchMock.reset()` in `beforeEach`, which
clears route definitions as well as call history; that can remove globally
registered mocks created in higher-level setup and make this test brittle. Use
`fetchMock.resetHistory()` to clear only call history while preserving route
stubs created in shared setup. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
fetchMock.resetHistory();
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
reset() clears both routes and history; in this test suite global route
stubs are established elsewhere and should be preserved. resetHistory() is the
safer operation here to avoid accidentally removing globally registered mocks
and causing flaky tests.
</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/chartActions.test.js
**Line:** 546:546
**Comment:**
*Possible Bug: The test uses `fetchMock.reset()` in `beforeEach`, which
clears route definitions as well as call history; that can remove globally
registered mocks created in higher-level setup and make this test brittle. Use
`fetchMock.resetHistory()` to clear only call history while preserving route
stubs created in shared setup.
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]
