codeant-ai-for-open-source[bot] commented on code in PR #37459:
URL: https://github.com/apache/superset/pull/37459#discussion_r2843448596
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -548,6 +558,7 @@ class ChartRenderer extends Component<ChartRendererProps,
ChartRendererState> {
legendState={this.state.legendState}
enableNoResults={bypassNoResult}
legendIndex={this.state.legendIndex}
+ isRefreshing={this.props.suppressLoadingSpinner}
{...drillToDetailProps}
Review Comment:
**Suggestion:** The `isRefreshing` prop passed to `SuperChart` is currently
tied only to `suppressLoadingSpinner`, so the chart is marked as refreshing on
every render when spinner suppression is enabled, even when no refresh is in
progress, which can cause incorrect behavior in plugins relying on this flag;
it should instead depend on the chart actually being in a loading state. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ SuperChart children see refreshing state after load completes.
- ⚠️ Auto-refresh dashboards may show stale refreshing indicators.
- ⚠️ Flicker-prevention logic can stay active unnecessarily.
```
</details>
```suggestion
isRefreshing={this.props.suppressLoadingSpinner && chartStatus
=== 'loading'}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render `ChartRenderer` from
`superset-frontend/src/components/Chart/ChartRenderer.tsx`
with props `chartStatus="loading"`, `suppressLoadingSpinner={true}`, and a
non-empty
`queriesResponse` without errors (see `render()` guard at lines 415–427).
2. After data loads, update props so `chartStatus` becomes `"rendered"` or
`"success"`
while keeping `suppressLoadingSpinner={true}` and the same `queriesResponse`
(normal
post-refresh state for auto-refresh dashboards).
3. On this update, `shouldComponentUpdate` (lines 86–137) returns `true`
because
`resultsReady` is satisfied and props have changed, so `render()` runs with
`chartStatus!=="loading"`.
4. In `render()` at lines 559–563, `SuperChart` is rendered with
`isRefreshing={this.props.suppressLoadingSpinner}` while `chartStatus` is no
longer
`"loading"`, so `isRefreshing` stays `true` even though no refresh is in
progress, causing
any SuperChart consumer of `isRefreshing` to see an incorrect persistent
"refreshing"
state.
```
</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.tsx
**Line:** 562:562
**Comment:**
*Logic Error: The `isRefreshing` prop passed to `SuperChart` is
currently tied only to `suppressLoadingSpinner`, so the chart is marked as
refreshing on every render when spinner suppression is enabled, even when no
refresh is in progress, which can cause incorrect behavior in plugins relying
on this flag; it should instead depend on the chart actually being in a loading
state.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37459&comment_hash=3ecb740b9eda4ede62666f60d21816275ed501f6ae2807b000dbaea6cf694bc3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37459&comment_hash=3ecb740b9eda4ede62666f60d21816275ed501f6ae2807b000dbaea6cf694bc3&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/orderby.ts:
##########
@@ -0,0 +1,47 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { QueryFormColumn, QueryFormOrderBy } from '@superset-ui/core';
+
+/**
+ * Builds orderby clauses from a list of columns, filtering out any non-string
+ * or nullish values. This ensures deterministic row ordering so that chart
+ * elements maintain stable positions across auto-refreshes.
+ */
+export function buildColumnsOrderBy(
+ columns: (QueryFormColumn | string | undefined | null)[],
+ ascending: boolean = true,
+): QueryFormOrderBy[] {
+ return columns
+ .filter((col): col is string => typeof col === 'string' && col !== '')
+ .map(col => [col, ascending]);
+}
+
+/**
+ * Conditionally applies orderby to a query object spread. Returns the
+ * orderby field only when row_limit is set (non-zero, non-null) and
+ * there are orderby entries to apply.
+ */
+export function applyOrderBy(
+ orderby: QueryFormOrderBy[],
+ rowLimit: string | number | undefined | null,
+): { orderby: QueryFormOrderBy[] } | Record<string, never> {
+ const shouldApply =
+ rowLimit !== undefined && rowLimit !== null && rowLimit !== 0;
Review Comment:
**Suggestion:** The `applyOrderBy` helper treats a string row limit of `'0'`
as a non-zero value, so when `rowLimit` is the string `"0"` it will still apply
`orderby`, contradicting the function's intent and potentially changing query
semantics compared with the numeric `0` case. To fix this, normalize string row
limits to numbers before checking for zero so that both `0` and `"0"` are
handled consistently as "no limit". [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ ECharts query utils mishandle string "0" row limits.
- ⚠️ Inconsistent behavior between numeric and string row_limit values.
- ⚠️ Potentially unexpected ORDER BY on charts with disabled limit.
```
</details>
```suggestion
const numericRowLimit =
typeof rowLimit === 'string' ? Number(rowLimit) : rowLimit;
const shouldApply =
numericRowLimit !== undefined &&
numericRowLimit !== null &&
numericRowLimit !== 0;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/plugins/plugin-chart-echarts/src/utils/orderby.ts` and locate
`applyOrderBy` at lines 40–47.
2. In any TypeScript test or caller, import this helper: `import {
applyOrderBy } from
'superset-frontend/plugins/plugin-chart-echarts/src/utils/orderby';`.
3. Invoke it with a string row limit of `"0"` and a non-empty orderby:
`applyOrderBy([['my_col', true]], '0');`.
4. Observe that inside `applyOrderBy` the `shouldApply` expression is `true`
(because `'0'
!== 0`), so the function returns `{ orderby: [['my_col', true]] }`, whereas
calling
`applyOrderBy([['my_col', true]], 0)` returns `{}`. This inconsistent
treatment of `"0"`
vs `0` contradicts the documented intent ("only when row_limit is set
(non-zero,
non-null)") for the same effective row limit.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/utils/orderby.ts
**Line:** 44:45
**Comment:**
*Logic Error: The `applyOrderBy` helper treats a string row limit of
`'0'` as a non-zero value, so when `rowLimit` is the string `"0"` it will still
apply `orderby`, contradicting the function's intent and potentially changing
query semantics compared with the numeric `0` case. To fix this, normalize
string row limits to numbers before checking for zero so that both `0` and
`"0"` are handled consistently as "no limit".
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37459&comment_hash=778128e1b1f59dd71bbea64d190a8bf6d7a6b1d2fc84966b5c460a835e402096&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37459&comment_hash=778128e1b1f59dd71bbea64d190a8bf6d7a6b1d2fc84966b5c460a835e402096&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/actions/dashboardState.ts:
##########
@@ -770,12 +771,20 @@ export function fetchCharts(
staggerRefresh && chartList.length > 1
? refreshTime / (chartList.length - 1)
: 0;
- chartList.forEach((chartKey: number, i: number) => {
- setTimeout(
- () => dispatch(refreshChart(chartKey, force, dashboardId)),
- delay * i,
- );
- });
+ return Promise.all(
+ chartList.map(
+ (chartKey: number, i: number) =>
+ new Promise<void>(resolve => {
+ setTimeout(() => {
+ Promise.resolve(
+ dispatch(refreshChart(chartKey, force, dashboardId)),
+ )
+ .then(() => resolve())
+ .catch(() => resolve());
Review Comment:
**Suggestion:** In the staggered refresh path, errors from individual chart
refreshes are caught and ignored, so the returned Promise always resolves and
callers like the dashboard refresh handler will dispatch a "success" action
even when some chart refreshes failed; instead, errors should propagate so that
overall refresh can be marked as failed consistently with the non-staggered
path. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dashboard-level refresh reports success despite chart failures.
- ⚠️ Auto-refresh status indicator may show green incorrectly.
- ⚠️ Filter refresh actions run after partially failed refresh.
```
</details>
```suggestion
new Promise<void>((resolve, reject) => {
setTimeout(() => {
Promise.resolve(
dispatch(refreshChart(chartKey, force, dashboardId)),
)
.then(() => resolve())
.catch(reject);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open any dashboard with multiple charts and auto-refresh enabled so that
`interval > 0`
and `chartList.length > 1` when `onRefresh` is dispatched (see `onRefresh` in
`src/dashboard/actions/dashboardState.ts`, around lines 832–860, which calls
`refreshCharts`, which calls `fetchCharts`).
2. Ensure dashboard metadata has `stagger_refresh` enabled or undefined
(defaulted to
`true` in `fetchCharts` at `dashboardState.ts`, lines 753–768) so the
staggered branch
with computed `delay` is taken.
3. Trigger a dashboard-level refresh (manual refresh button or timer-driven
auto-refresh)
so that `onRefresh(...)` dispatches `refreshCharts(chartList, force,
interval,
dashboardId, dispatch)`, which in turn calls `fetchCharts` and executes the
staggered
block shown at `dashboardState.ts`, lines 770–787.
4. Cause one of the per-chart refresh thunks `refreshChart(chartKey, force,
dashboardId)`
(imported from `src/components/Chart/chartAction`) to reject (for example,
by simulating a
failing network request in that thunk); observe that the `.catch(() =>
resolve())` in
`fetchCharts` swallows the rejection, `Promise.all` still resolves, and
`onRefresh(...).then(...)` proceeds to dispatch `onRefreshSuccess()` and
`onFiltersRefresh()` even though at least one chart refresh failed, whereas
in the
non-staggered path (no interval) such a rejection would cause the overall
promise to
reject and skip the "success" actions.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/actions/dashboardState.ts
**Line:** 777:783
**Comment:**
*Logic Error: In the staggered refresh path, errors from individual
chart refreshes are caught and ignored, so the returned Promise always resolves
and callers like the dashboard refresh handler will dispatch a "success" action
even when some chart refreshes failed; instead, errors should propagate so that
overall refresh can be marked as failed consistently with the non-staggered
path.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37459&comment_hash=dc6e601c9a8e46750f06a07ca20b994b2cb94b9cbf42c70612f3f9dd8027d706&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37459&comment_hash=dc6e601c9a8e46750f06a07ca20b994b2cb94b9cbf42c70612f3f9dd8027d706&reaction=dislike'>👎</a>
--
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]
