codeant-ai-for-open-source[bot] commented on code in PR #37669:
URL: https://github.com/apache/superset/pull/37669#discussion_r2769070836
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.ts:
##########
@@ -82,26 +83,32 @@ export default function buildQuery(formData:
DeckScatterFormData) {
// Only add metric if point_radius_fixed is a metric type
const isMetric = isMetricValue(point_radius_fixed);
- const metricValue = isMetric
- ? extractMetricKey(point_radius_fixed?.value)
- : null;
+ // When type is 'metric', value is QueryFormMetric (string or adhoc
object)
+ const metricValue: QueryFormMetric | null =
+ isMetric && point_radius_fixed?.value !== undefined
+ ? (point_radius_fixed.value as QueryFormMetric)
+ : null;
Review Comment:
**Suggestion:** The new logic only reads `point_radius_fixed.value` when it
is an object, so if `point_radius_fixed` is provided in the legacy string form
(which `isMetricValue` and `getMetricLabelFromFormData` still support
elsewhere), `isMetric` becomes true but `metricValue` stays null and the radius
metric is never added to `metrics` or `orderby`. This breaks existing charts
saved with a string radius metric: the backend never computes the radius metric
column while `transformProps` still expects it, resulting in missing radius
values and inconsistent behavior. Update `metricValue` derivation to also
handle the case where `point_radius_fixed` itself is a string, and to avoid
treating numeric fixed values as metrics. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Scatter point radius metric missing in saved legacy charts.
- ⚠️ Visualization shows missing or default point sizes.
- ⚠️ Backend doesn't compute required radius column.
- ⚠️ Affects charts saved before metric-object migration.
- ⚠️ Suggestion fixes real compatibility regression.
```
</details>
```suggestion
let metricValue: QueryFormMetric | null = null;
if (isMetric) {
if (typeof point_radius_fixed === 'string') {
// Legacy string format: treat the string itself as the metric
metricValue = point_radius_fixed;
} else if (
point_radius_fixed?.value !== undefined &&
typeof point_radius_fixed.value !== 'number'
) {
// Metric object format: use the metric value (string or adhoc
metric object)
metricValue = point_radius_fixed.value as QueryFormMetric;
}
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a saved Scatter chart that was created before the new PR fix and
that uses the
legacy string form for the radius metric (point_radius_fixed stored as a
plain string).
The UI stores this value in the chart form data used by buildQuery (file:
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.ts).
2. The chart rendering path calls buildQuery(formData) (function defined at
top of the
same file). At runtime buildQuery reaches the snippet beginning at line 84:
it calls
isMetricValue(point_radius_fixed) (line 84) which returns true for legacy
string metrics.
3. Immediately after, the current code computes metricValue using the
conditional
expression at lines 86-90. Because the legacy value is a string and not an
object with a
.value property, point_radius_fixed?.value is undefined, so metricValue
becomes null
(lines 86-90).
4. Later in the same function (lines 92-101 in the final file), metricValue
being null
causes the radius metric not to be appended to the metrics array and not to
be added to
orderby (metrics stays as existingMetrics, and orderby falls back to
baseQueryObject.orderby). Therefore the query sent to the backend does not
request the
radius metric column.
5. The backend therefore does not compute the metric column for point
radius. The
front-end code that expects the radius column (the deck.gl layer
transform/transformProps
logic described in the PR summary) receives missing radius values, resulting
in
empty/missing point sizes or errors in the visualization.
6. Expected correct behavior per PR description: legacy string metrics
should be treated
as valid metrics and included in metrics/orderby. The improved_code change
sets
metricValue when point_radius_fixed is a string, restoring the missing
metric column in
the query and preventing missing radius values.
7. Note: this reproduction is based on the actual lines in buildQuery.ts
(lines 84-101)
where isMetric is computed and metricValue is derived, and on the later use
of
metrics/orderby in the same function (lines 92-112) which control query
composition.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.ts
**Line:** 87:90
**Comment:**
*Logic Error: The new logic only reads `point_radius_fixed.value` when
it is an object, so if `point_radius_fixed` is provided in the legacy string
form (which `isMetricValue` and `getMetricLabelFromFormData` still support
elsewhere), `isMetric` becomes true but `metricValue` stays null and the radius
metric is never added to `metrics` or `orderby`. This breaks existing charts
saved with a string radius metric: the backend never computes the radius metric
column while `transformProps` still expects it, resulting in missing radius
values and inconsistent behavior. Update `metricValue` derivation to also
handle the case where `point_radius_fixed` itself is a string, and to avoid
treating numeric fixed values as metrics.
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]
