codeant-ai-for-open-source[bot] commented on code in PR #42530: URL: https://github.com/apache/superset/pull/42530#discussion_r3664020989
########## superset-frontend/packages/superset-ui-chart-controls/src/utils/buildSortMetricOrderby.ts: ########## @@ -0,0 +1,87 @@ +/** + * 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 { ensureIsArray, getMetricLabel } from '@superset-ui/core'; +import type { QueryFormMetric, QueryFormOrderBy } from '@superset-ui/core'; + +export interface BuildSortMetricOrderbyConfig { + /** The query's already-resolved metrics list. */ + metrics: QueryFormMetric[]; + /** The raw `timeseries_limit_metric` form-data value (single or multi). */ + timeseriesLimitMetric?: QueryFormMetric | QueryFormMetric[] | null; + order_desc?: boolean; + /** + * Falls back to the first selected metric when no sort metric is set. + * Charts ported from a legacy viz whose query_obj always had a sort + * metric (defaulting to the first one) should set this; charts whose + * legacy query_obj left ordering absent without one should not. + */ + fallbackToFirstMetric?: boolean; + /** + * When true, only order when `order_desc` is set (matching legacy vizzes + * whose query_obj left the result unordered unless the operator asked + * for descending). When false, always order (ascending unless + * order_desc), matching legacy vizzes that ordered unconditionally. + */ + orderOnlyWhenDesc?: boolean; +} + +export interface SortMetricOrderby { + /** `metrics`, with the sort metric appended if it wasn't already selected. */ + metrics: QueryFormMetric[]; + orderby: QueryFormOrderBy[]; +} + +/** + * Resolves a chart's sort metric and builds the corresponding query_obj + * `orderby`, appending the sort metric to `metrics` if it isn't already + * selected (so its value is present in the result to sort by). Several + * charts ported from the legacy chart-data pipeline share this exact + * shape with only the fallback/gating policy differing per their own + * legacy `query_obj` behavior -- see `fallbackToFirstMetric` and + * `orderOnlyWhenDesc`. + */ +export function buildSortMetricOrderby({ + metrics, + timeseriesLimitMetric, + order_desc: orderDesc, + fallbackToFirstMetric = false, + orderOnlyWhenDesc = false, +}: BuildSortMetricOrderbyConfig): SortMetricOrderby { + const sortByMetric = + ensureIsArray(timeseriesLimitMetric)[0] ?? + (fallbackToFirstMetric ? metrics[0] : undefined); + + if (!sortByMetric) { + return { metrics, orderby: [] }; + } + + const sortByLabel = getMetricLabel(sortByMetric); + const nextMetrics = metrics.some( + metric => getMetricLabel(metric) === sortByLabel, + ) + ? metrics Review Comment: **Suggestion:** Using `getMetricLabel` as the identity check can treat distinct adhoc metric definitions as the same metric when they share a label. In that case the selected sort metric is not appended to `metrics`, even though `orderby` references it, so the query can attempt to order by a metric absent from the selected result. Compare the metric definitions using their actual metric identity or canonical serialized definition rather than display labels. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Sorted queries can reference an unselected adhoc metric. - ⚠️ Partition ordering can be incorrect or fail. - ⚠️ Paired and parallel chart result ordering is affected. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure a Partition, Paired T-Test, or Parallel Coordinates chart, whose buildQuery functions call `buildSortMetricOrderby()` at `superset-frontend/plugins/plugin-chart-partition/src/buildQuery.ts:18`, `superset-frontend/plugins/plugin-chart-paired-t-test/src/buildQuery.ts:16`, and `superset-frontend/plugins/plugin-chart-parallel-coordinates/src/buildQuery.ts:18`. 2. Supply two distinct adhoc metric definitions with the same explicit `label`, but different aggregate, column, or SQL expression; `getMetricLabel()` at `superset-frontend/packages/superset-ui-core/src/query/getMetricLabel.ts:26-34` intentionally returns the label before examining the definition. 3. Keep one definition in `baseQueryObject.metrics` and use the other as `timeseries_limit_metric`; the code at `buildSortMetricOrderby.ts:74-78` considers the metrics identical because their labels match and therefore does not append the sort definition. 4. The returned object at `buildSortMetricOrderby.ts:83-85` still places the unselected sort-definition object in `orderby`, while the query's `metrics` list contains only the other definition, so the backend receives an order metric that was not selected. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=388af4b1937c413bb84aea9a3f7c7b4d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=388af4b1937c413bb84aea9a3f7c7b4d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/packages/superset-ui-chart-controls/src/utils/buildSortMetricOrderby.ts **Line:** 74:78 **Comment:** *Api Mismatch: Using `getMetricLabel` as the identity check can treat distinct adhoc metric definitions as the same metric when they share a label. In that case the selected sort metric is not appended to `metrics`, even though `orderby` references it, so the query can attempt to order by a metric absent from the selected result. Compare the metric definitions using their actual metric identity or canonical serialized definition rather than display labels. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42530&comment_hash=fecfe6741c65bcf706f923a843648584e4bf891d1c1184f3a44f35f730a48ea2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42530&comment_hash=fecfe6741c65bcf706f923a843648584e4bf891d1c1184f3a44f35f730a48ea2&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts: ########## @@ -185,7 +178,7 @@ export default function transformProps( const gridHeight = Math.max(height - gridTop - theme.sizeUnit * 6, 40); const rowHeight = gridHeight / categories.length; const markerOffsetPx = Math.round( - (MEASURE_BAR_FRACTION / 2) * rowHeight + 12, + (MEASURE_BAR_FRACTION / 2) * rowHeight + MARKER_GAP_BELOW_BAR_PX, ); Review Comment: **Suggestion:** Grouped empty results leave `categories` empty, so `categories.length` is zero and `rowHeight` becomes `Infinity`. This produces an infinite marker `symbolOffset` in the ECharts options whenever markers are configured, causing invalid geometry for an otherwise valid empty chart. Use a nonzero row count for the offset calculation or skip the row-based offset when there are no categories. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Empty grouped Bullet charts emit invalid marker geometry. - ⚠️ Marker-series rendering can produce ECharts warnings. - ⚠️ Empty-result chart rendering becomes less reliable. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open a Bullet chart with a non-empty `groupby` and a query returning no records; `transformProps()` constructs `categories` from `rows` at `superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts:65-68`, leaving it empty for grouped results. 2. Configure at least one marker through the `markers` control, which is defined at `superset-frontend/plugins/plugin-chart-echarts/src/Bullet/types.ts:37-40` and parsed at `transformProps.ts:76`. 3. The calculation at `transformProps.ts:178-180` divides the finite `gridHeight` by `categories.length` equal to zero, producing `rowHeight === Infinity`. 4. `Math.round()` preserves the invalid value, and each marker series receives `symbolOffset: [0, markerOffsetPx]` at `transformProps.ts:270-284`, causing the otherwise valid empty chart options to contain an infinite geometry offset. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=01272326cbaf4f14a95bbef52dc6573b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=01272326cbaf4f14a95bbef52dc6573b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <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/Bullet/transformProps.ts **Line:** 178:182 **Comment:** *Possible Bug: Grouped empty results leave `categories` empty, so `categories.length` is zero and `rowHeight` becomes `Infinity`. This produces an infinite marker `symbolOffset` in the ECharts options whenever markers are configured, causing invalid geometry for an otherwise valid empty chart. Use a nonzero row count for the offset calculation or skip the row-based offset when there are no categories. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42530&comment_hash=bf31c34556265487c9121cfdd1cf0d17fdc1a5a673e74f97f44a17eb698df09e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42530&comment_hash=bf31c34556265487c9121cfdd1cf0d17fdc1a5a673e74f97f44a17eb698df09e&reaction=dislike'>👎</a> ########## superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.tsx: ########## @@ -551,15 +561,12 @@ const DeckMulti = (props: DeckMultiProps) => { ); if (deckSlicesChanged || visibilityFilterChanged) { - // legacy explore_json payloads already carried the subslice metadata - const legacySlices = payload?.data?.slices; - if (legacySlices) { - loadLayers(formData, legacySlices, visibleDeckLayersFromRedux); - } else { - fetchSubslices(ensureIsArray(formData.deck_slices) as number[]).then( - slices => loadLayers(formData, slices, visibleDeckLayersFromRedux), - ); - } + // deck_multi issues no query of its own (see buildQuery.ts), so each + // sub-slice's saved form_data is always fetched client-side here -- + // there is no pre-merged payload to read subslice metadata from. + fetchSubslices(ensureIsArray(formData.deck_slices) as number[]).then( + slices => loadLayers(formData, slices, visibleDeckLayersFromRedux), + ); Review Comment: **Suggestion:** The generation guard starts only when `loadLayers` runs, after `fetchSubslices` completes. If the deck slices or visibility filter changes while an earlier metadata request is pending, the newer request can load first and receive a lower generation, then the older request can resolve later and call `loadLayers` with a newer generation, resetting state and launching stale layer requests. Track or invalidate the metadata fetch itself when the effect starts, so only the latest `fetchSubslices` result can call `loadLayers`. [race condition] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Deck Multi can display layers from stale configuration. - ⚠️ Current layer visibility can be reset by an older request. - ⚠️ Stale child queries and autozoom can replace newer state. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Render `DeckMulti`, whose effect at `superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.tsx:554-569` fetches each configured child through `fetchSubslices()`, which issues `GET /api/v1/chart/{sliceId}` at lines 512-530. 2. Change `formData.deck_slices` or the Redux layer-visibility filter twice before the first metadata request completes, causing the effect to start metadata requests for two configurations. 3. Allow the newer metadata request to resolve first. Its callback at `Multi.tsx:567-569` calls `loadLayers()`, which creates the first generation only at `Multi.tsx:455-467`. 4. Allow the older metadata request to resolve afterward. It also calls `loadLayers()`, increments the generation, clears current layers, and launches stale child requests; the generation guard at `Multi.tsx:383-387` cannot reject those requests because the stale metadata callback has just declared itself the newest generation. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1331c9055176428380eadf4115081fb9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1331c9055176428380eadf4115081fb9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/plugins/preset-chart-deckgl/src/Multi/Multi.tsx **Line:** 567:569 **Comment:** *Race Condition: The generation guard starts only when `loadLayers` runs, after `fetchSubslices` completes. If the deck slices or visibility filter changes while an earlier metadata request is pending, the newer request can load first and receive a lower generation, then the older request can resolve later and call `loadLayers` with a newer generation, resetting state and launching stale layer requests. Track or invalidate the metadata fetch itself when the effect starts, so only the latest `fetchSubslices` result can call `loadLayers`. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42530&comment_hash=5ea58c87cda76fa487a8c2bfad8dd6e18594b5814317e02125f16fb33025be56&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42530&comment_hash=5ea58c87cda76fa487a8c2bfad8dd6e18594b5814317e02125f16fb33025be56&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]
