bito-code-review[bot] commented on code in PR #36708:
URL: https://github.com/apache/superset/pull/36708#discussion_r4100798453
##########
superset-frontend/src/explore/types.ts:
##########
@@ -59,6 +59,7 @@ export interface ChartState {
chartUpdateStartTime: number;
lastRendered: number;
latestQueryFormData: LatestQueryFormData;
+ form_data?: LatestQueryFormData;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate form state fields</b></div>
<div id="fix">
`form_data` and `latestQueryFormData` are both `LatestQueryFormData` and
hold the same data. `chartReducer` must manually keep them in sync
(`CHART_UPDATE_SUCCEEDED` sets `form_data: state.latestQueryFormData`;
`UPDATE_CHART_FORM_DATA` sets both). Any future update to one without the other
silently diverges. Consider documenting the intended distinction
(server-refreshed definition vs query-time) or consolidating to a single source
of truth.
</div>
</div>
<small><i>Code Review Run #48795a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/components/Chart/chartAction.ts:
##########
@@ -1031,38 +1046,71 @@ export function redirectSQLLab(
};
}
+// Re-fetches a chart's stored `params` from the server and returns it parsed
+// as form data, falling back to `fallback` on any network/parsing failure so a
+// refresh never hard-fails just because the freshness check did.
+async function fetchLatestChartFormData(
+ chartId: string | number,
+ fallback: QueryFormData | LatestQueryFormData,
+): Promise<QueryFormData | LatestQueryFormData> {
+ try {
+ const { json } = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${chartId}`,
+ });
+ const { params } = json?.result ?? {};
+ if (typeof params !== 'string') {
+ return fallback;
+ }
+ return { ...fallback, ...JSON.parse(params) };
+ } catch {
+ return fallback;
+ }
+}
+
export function refreshChart(
chartKey: string | number,
force: boolean,
dashboardId?: number,
+ // Re-fetches the chart's own definition before running the query, so a
+ // save made from Explore in another tab is reflected here instead of the
+ // stale `latestQueryFormData` this dashboard was hydrated with. Only the
+ // single-chart "Force Refresh" action opts into this: `force` alone is
+ // also used for whole-dashboard and periodic refreshes, where re-fetching
+ // every chart's definition on every tick would be wasteful.
+ refreshFormData = false,
): ChartThunkAction<Promise<void>> {
- return (
+ return async (
dispatch: ChartThunkDispatch,
getState: () => RootState,
): Promise<void> => {
const chart = (getState().charts || {})[chartKey];
if (!chart) {
- return Promise.resolve();
+ return;
}
const timeout =
getState().dashboardInfo.common.conf.SUPERSET_WEBSERVER_TIMEOUT;
- if (
- !chart.latestQueryFormData ||
- Object.keys(chart.latestQueryFormData).length === 0
- ) {
- return Promise.resolve();
+ let formData = chart.latestQueryFormData;
+ if (refreshFormData && chart.id) {
+ formData = await fetchLatestChartFormData(chart.id, formData);
+ if (formData !== chart.latestQueryFormData) {
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Redundant dispatch guard</b></div>
<div id="fix">
On the success path `fetchLatestChartFormData` always returns a fresh object
via `{ ...fallback, ...JSON.parse(params) }` (line 1064), so `formData !==
chart.latestQueryFormData` is always true and `updateChartFormData` is
dispatched on every Force Refresh even when nothing changed, causing a
redundant store update and re-render. Compare serialized content (or have the
helper return `fallback` when params is empty) so the guard actually skips
no-op dispatches.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
if (
JSON.stringify(formData) !==
JSON.stringify(chart.latestQueryFormData)
) {
````
</div>
</details>
</div>
<small><i>Code Review Run #48795a</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
