codeant-ai-for-open-source[bot] commented on code in PR #36708:
URL: https://github.com/apache/superset/pull/36708#discussion_r2715117268
##########
superset-frontend/src/components/Chart/chartReducer.ts:
##########
@@ -64,6 +64,7 @@ export default function chartReducer(
chartAlert: null,
queriesResponse: action.queriesResponse,
chartUpdateEndTime: now(),
+ form_data: state.latestQueryFormData,
Review Comment:
**Suggestion:** In the CHART_UPDATE_SUCCEEDED handler a snake_case property
`form_data` is being set from `state.latestQueryFormData`. The rest of this
reducer and the typed `ChartState` use `latestQueryFormData` (camelCase).
Introducing `form_data` creates an inconsistent state shape and consumers that
read `latestQueryFormData` will still see stale data; replace the snake_case
property with `latestQueryFormData` so the reducer consistently updates the
canonical field. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dashboard force-refresh shows stale BigNumber metric.
- ⚠️ Chart update reducer state shape inconsistent.
- ⚠️ CHART_UPDATE_SUCCEEDED flow uses wrong field.
```
</details>
```suggestion
latestQueryFormData: state.latestQueryFormData,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In Tab A, edit and save a chart. The save flow dispatches the
UPDATE_CHART_FORM_DATA
action handled by chartReducer at
superset-frontend/src/components/Chart/chartReducer.ts
lines 196-205 (the top-level if (action.type ===
actions.UPDATE_CHART_FORM_DATA) block
added in this PR). That block updates both charts[chartId].form_data and
charts[chartId].latestQueryFormData (see chartReducer.ts:196-205).
2. In a different browser Tab B showing the dashboard, click "Force Refresh"
which runs
the chart update lifecycle and dispatches CHART_UPDATE_STARTED then
CHART_UPDATE_SUCCEEDED. The reducer's inlined action handler for
CHART_UPDATE_SUCCEEDED is
defined in chartReducer.ts lines 64-67 (the actionHandlers mapping); inspect
chartReducer.ts:64-67 where the handler sets the single property "form_data:
state.latestQueryFormData,".
3. The actionHandlers are applied to the per-chart state entry by the
reducer code path
that uses actionHandlers (the reducer later applies actionHandlers to
charts[action.key]).
Concretely, CHART_UPDATE_SUCCEEDED's handler at chartReducer.ts:64-67 writes
only
form_data on the chart state but does not update the canonical
latestQueryFormData field
used elsewhere. Because Tab B's charts[chartId].latestQueryFormData was not
updated from
Tab A (the cross-tab update flow did not occur), setting only form_data here
leaves
latestQueryFormData stale.
4. Observe the user-visible result: after Force Refresh in Tab B the chart
rendering logic
that reads latestQueryFormData continues to use the stale metric/config and
displays stale
BigNumber data. This reproduces the bug reported in the PR description.
```
</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:** 67:67
**Comment:**
*Logic Error: In the CHART_UPDATE_SUCCEEDED handler a snake_case
property `form_data` is being set from `state.latestQueryFormData`. The rest of
this reducer and the typed `ChartState` use `latestQueryFormData` (camelCase).
Introducing `form_data` creates an inconsistent state shape and consumers that
read `latestQueryFormData` will still see stale data; replace the snake_case
property with `latestQueryFormData` so the reducer consistently updates the
canonical field.
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/actions/saveModalActions.ts:
##########
@@ -64,6 +64,39 @@ export function saveSliceSuccess(data:
Partial<QueryFormData>) {
return { type: SAVE_SLICE_SUCCESS, data };
}
+// Broadcast chart updates to other tabs
+let chartUpdateChannel: BroadcastChannel | null = null;
+
+function getChartUpdateChannel(): BroadcastChannel | null {
+ if (typeof BroadcastChannel === 'undefined') {
+ return null;
+ }
+
+ if (!chartUpdateChannel) {
+ chartUpdateChannel = new BroadcastChannel('superset_chart_updates');
Review Comment:
**Suggestion:** Creating a BroadcastChannel can throw (for example in some
browsers' private modes or restricted environments). The current code
constructs a new BroadcastChannel without guarding against exceptions, which
can bubble up and crash save flows; wrap the constructor in a try/catch and
gracefully disable broadcasting on failure. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Save/update chart flow can fail in some browsers.
- ⚠️ Explore save UI reports failure incorrectly.
- ⚠️ Inter-tab update notifications disabled silently.
```
</details>
```suggestion
try {
chartUpdateChannel = new BroadcastChannel('superset_chart_updates');
} catch (err) {
// Some browsers (e.g., Safari private mode or restricted
environments) throw when constructing BroadcastChannel.
// Disable broadcasting instead of letting the exception propagate and
break save operations.
// eslint-disable-next-line no-console
console.warn('BroadcastChannel unavailable, disabling chart update
broadcasts:', err);
chartUpdateChannel = null;
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the browser, open the Explore UI and edit a chart. Trigger save which
invokes
updateSlice in superset-frontend/src/explore/actions/saveModalActions.ts
(function export
const updateSlice) — flow starts at the try block after SupersetClient.put()
and reaches
the broadcast call at lines 290-293.
2. updateSlice calls broadcastChartUpdate(sliceId, formData) which calls
getChartUpdateChannel() defined in
superset-frontend/src/explore/actions/saveModalActions.ts at lines 70-80.
3. In restrictive environments (e.g., Safari private mode), the
BroadcastChannel
constructor at line 76 (chartUpdateChannel = new
BroadcastChannel('superset_chart_updates')) can throw a DOMException.
4. Because getChartUpdateChannel() does not catch that exception, the thrown
error bubbles
back into updateSlice's try block, is caught by its catch, dispatches
SAVE_SLICE_FAILED,
and rethrows — causing the UI to report save failure even though the server
request
already succeeded. Wrapping the constructor in try/catch prevents the
exception from
aborting the save flow.
```
</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/actions/saveModalActions.ts
**Line:** 76:76
**Comment:**
*Possible Bug: Creating a BroadcastChannel can throw (for example in
some browsers' private modes or restricted environments). The current code
constructs a new BroadcastChannel without guarding against exceptions, which
can bubble up and crash save flows; wrap the constructor in a try/catch and
gracefully disable broadcasting on failure.
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/chartReducers.test.js:
##########
@@ -19,45 +19,136 @@
import chartReducer, { chart } from 'src/components/Chart/chartReducer';
import * as actions from 'src/components/Chart/chartAction';
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
-describe('chart reducers', () => {
- const chartKey = 1;
- let testChart;
- let charts;
- beforeEach(() => {
- testChart = {
- ...chart,
- id: chartKey,
- };
- charts = { [chartKey]: testChart };
+const CHART_KEY = 1;
+
+// Helper function to create initial chart state
+const createChartState = (overrides = {}) => ({
+ [CHART_KEY]: {
+ ...chart,
+ id: CHART_KEY,
+ ...overrides,
+ },
+});
+
+test('chartReducer updates endtime and status when chart update is stopped',
() => {
+ const charts = createChartState();
+
+ const newState = chartReducer(charts, actions.chartUpdateStopped(CHART_KEY));
+
+ expect(newState[CHART_KEY].chartUpdateEndTime).toBeGreaterThan(0);
+ expect(newState[CHART_KEY].chartStatus).toEqual('stopped');
+});
+
+test('chartReducer updates endtime and status when chart update fails with
timeout', () => {
+ const charts = createChartState();
+
+ const newState = chartReducer(
+ charts,
+ actions.chartUpdateFailed(
+ {
+ statusText: 'timeout',
+ error: 'Request timed out',
+ errors: [
+ {
+ error_type: 'FRONTEND_TIMEOUT_ERROR',
+ extra: { timeout: 1 },
+ level: 'error',
+ message: 'Request timed out',
+ },
+ ],
+ },
+ CHART_KEY,
+ ),
+ );
+
+ expect(newState[CHART_KEY].chartUpdateEndTime).toBeGreaterThan(0);
+ expect(newState[CHART_KEY].chartStatus).toEqual('failed');
+});
+
+test('chartReducer updates form_data from latestQueryFormData on chart update
success', () => {
+ const latestQueryFormData = {
+ datasource: '1__table',
+ viz_type: 'big_number_total',
+ slice_id: CHART_KEY,
+ metric: { aggregate: 'COUNT_DISTINCT' },
+ };
+
+ const queriesResponse = [
+ {
+ data: [{ 'COUNT_DISTINCT(column)': 42 }],
+ colnames: ['COUNT_DISTINCT(column)'],
+ coltypes: [0],
+ },
+ ];
+
+ const charts = createChartState({
+ latestQueryFormData,
+ form_data: { datasource: '1__table', viz_type: 'big_number_total' },
});
- test('should update endtime on fail', () => {
- const newState = chartReducer(charts,
actions.chartUpdateStopped(chartKey));
- expect(newState[chartKey].chartUpdateEndTime).toBeGreaterThan(0);
- expect(newState[chartKey].chartStatus).toEqual('stopped');
+ const newState = chartReducer(
+ charts,
+ actions.chartUpdateSucceeded(queriesResponse, CHART_KEY),
+ );
+
+ expect(newState[CHART_KEY].chartStatus).toEqual('success');
+ expect(newState[CHART_KEY].queriesResponse).toEqual(queriesResponse);
+ expect(newState[CHART_KEY].form_data).toEqual(latestQueryFormData);
+ expect(newState[CHART_KEY].chartUpdateEndTime).toBeGreaterThan(0);
+});
+
+test('chartReducer syncs form_data with latestQueryFormData after refresh', ()
=> {
+ const oldFormData = {
+ datasource: '1__table',
+ viz_type: 'big_number_total',
+ metric: { aggregate: 'COUNT' },
+ };
+
+ const newFormData = {
+ datasource: '1__table',
+ viz_type: 'big_number_total',
+ metric: { aggregate: 'MAX' },
+ };
+
+ const charts = createChartState({
+ latestQueryFormData: newFormData,
+ form_data: oldFormData,
});
- test('should update endtime on timeout', () => {
- const newState = chartReducer(
- charts,
- actions.chartUpdateFailed(
- {
- statusText: 'timeout',
- error: 'Request timed out',
- errors: [
- {
- error_type: 'FRONTEND_TIMEOUT_ERROR',
- extra: { timeout: 1 },
- level: 'error',
- message: 'Request timed out',
- },
- ],
- },
- chartKey,
- ),
- );
- expect(newState[chartKey].chartUpdateEndTime).toBeGreaterThan(0);
- expect(newState[chartKey].chartStatus).toEqual('failed');
+ const newState = chartReducer(
+ charts,
+ actions.chartUpdateSucceeded([], CHART_KEY),
+ );
+
+ expect(newState[CHART_KEY].form_data).toEqual(newFormData);
+ expect(newState[CHART_KEY].form_data.metric.aggregate).toBe('MAX');
+});
+
+test('UPDATE_CHART_FORM_DATA updates both form_data and latestQueryFormData
for cross-tab sync', () => {
+ const oldFormData = {
+ datasource: '1__table',
+ viz_type: 'big_number_total',
+ metric: { aggregate: 'MIN' },
+ };
+
+ const updatedFormData = {
+ datasource: '1__table',
+ viz_type: 'big_number_total',
+ metric: { aggregate: 'MAX' },
+ };
+
+ const charts = createChartState({
+ form_data: oldFormData,
+ latestQueryFormData: oldFormData,
});
+
+ const newState = chartReducer(charts, {
+ type: actions.UPDATE_CHART_FORM_DATA,
+ chartId: CHART_KEY,
+ formData: updatedFormData,
Review Comment:
**Suggestion:** The dispatched action for updating chart form data uses
`chartId` and `formData` property names, but the reducer/action creators in
this filebase use `key` and `form_data` conventions; using the wrong property
names will cause the reducer to ignore the payload and the test to fail.
Dispatch the action with the expected property names (`key` and `form_data`).
[logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Unit test fails in chartReducers.test.js.
- ❌ CI job can fail blocking merges.
- ⚠️ Runtime UI unaffected if action creators used.
```
</details>
```suggestion
key: CHART_KEY,
form_data: updatedFormData,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the unit tests for charts: execute Jest for file
`superset-frontend/src/components/Chart/chartReducers.test.js`. The failing
test block is
defined starting at line 127 and the specific action dispatch is at lines
145-149 (`const
newState = chartReducer(...`).
2. The test constructs initial state via `createChartState()` at
`superset-frontend/src/components/Chart/chartReducers.test.js:25-31`,
producing an object
keyed by `CHART_KEY`.
3. The test dispatches a raw action object (not using an action creator) at
`...chartReducers.test.js:145-149` with properties `chartId` and `formData`.
Because the
codebase consistently passes `key`/`form_data` in actions (see action
creators that
include `key` at
`superset-frontend/src/components/Chart/chartAction.js:58-70`), the
reducer will not find the expected `action.key`/`action.form_data` fields.
4. The reducer returns an unchanged chart entry, so the assertions
immediately after
(expectations at `...chartReducers.test.js:151-153`) observe the old
`form_data` and the
test fails. This reproduces deterministically by running the test file as-is.
```
</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/chartReducers.test.js
**Line:** 147:148
**Comment:**
*Logic Error: The dispatched action for updating chart form data uses
`chartId` and `formData` property names, but the reducer/action creators in
this filebase use `key` and `form_data` conventions; using the wrong property
names will cause the reducer to ignore the payload and the test to fail.
Dispatch the action with the expected property names (`key` and `form_data`).
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/actions/saveModalActions.test.ts:
##########
@@ -727,3 +727,143 @@ describe('getSlicePayload', () => {
});
});
});
+
+// Helper function to set up broadcast channel listener
+const setupBroadcastListener = () => {
+ const receivedMessages: any[] = [];
+ const MockBroadcastChannel = global.BroadcastChannel as any;
+ const listener = new MockBroadcastChannel('superset_chart_updates');
+ listener.onmessage = (event: { data: any }) => {
+ receivedMessages.push(event.data);
+ };
+ return { receivedMessages, listener };
+};
Review Comment:
**Suggestion:** Duplicate helper declaration: `setupBroadcastListener` is
defined twice in this file (the PR added a second declaration). Redeclaring the
same `const` in the same scope will cause a runtime/parse error when the test
file is loaded; remove the duplicated definition (keep only one helper).
[possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Test file fails to load blocking Jest runs.
- ❌ CI job fails on this test file.
- ⚠️ Developers cannot run these local tests.
- ⚠️ Duplicate is accidental, not intentional design.
```
</details>
```suggestion
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the test file at
superset-frontend/src/explore/actions/saveModalActions.test.ts
and observe the helper defined at lines 731-740 (the snippet above).
2. Inspect the same file and find the earlier helper declaration with the
same identifier
(setupBroadcastListener) that already exists elsewhere in the same file (the
file contains
a second identical declaration later in the file). The duplicate definitions
are in the
same top-level scope of this test file.
3. Run the tests (e.g., npm test --
superset-frontend/src/explore/actions/saveModalActions.test.ts or the
repository's Jest
runner). When Node/TypeScript compiles/loads the test file, the
parser/runner will raise a
runtime/syntax error about the identifier already being declared (duplicate
const) because
setupBroadcastListener is declared twice in the same scope.
4. Result: the test file fails to load and the test run aborts with a
SyntaxError/ReferenceError. Fix: remove one of the duplicate declarations so
the helper is
declared exactly once.
```
</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/actions/saveModalActions.test.ts
**Line:** 731:740
**Comment:**
*Possible Bug: Duplicate helper declaration: `setupBroadcastListener`
is defined twice in this file (the PR added a second declaration). Redeclaring
the same `const` in the same scope will cause a runtime/parse error when the
test file is loaded; remove the duplicated definition (keep only one helper).
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]
