codeant-ai-for-open-source[bot] commented on code in PR #36890:
URL: https://github.com/apache/superset/pull/36890#discussion_r2657700449
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/Scatter.tsx:
##########
@@ -44,9 +44,20 @@ function setTooltipContent(
verboseMap?: Record<string, string>,
) {
const defaultTooltipGenerator = (o: JsonObject) => {
- const label =
- verboseMap?.[formData.point_radius_fixed.value] ||
- getMetricLabel(formData.point_radius_fixed?.value);
+ // Check if point_radius_fixed is an object with type/value or just a value
+ let metricKey = null;
+ if (formData.point_radius_fixed) {
+ if (typeof formData.point_radius_fixed === 'object' &&
formData.point_radius_fixed.type === 'metric') {
+ metricKey = formData.point_radius_fixed.value;
+ } else if (typeof formData.point_radius_fixed === 'string') {
+ metricKey = formData.point_radius_fixed;
Review Comment:
**Suggestion:** The tooltip label resolution only treats
`point_radius_fixed` as a metric when it is a string or an object with `type
=== 'metric'`, ignoring legacy configurations where `point_radius_fixed` is an
object without a `type` field but still represents a metric; for such saved
charts, `o.object.metric` will be populated by `transformProps` but `label`
stays null, so the metric row is never rendered, breaking tooltips for those
legacy Scatter charts. The fix is to treat object configs without a `type` as
metric configs when their `value` is non-numeric (matching how legacy metric
structures are handled elsewhere), while still excluding fixed-size configs.
[logic error]
**Severity Level:** Minor ⚠️
```suggestion
const config = formData.point_radius_fixed as any;
if (typeof config === 'object') {
const { type, value } = config;
// Treat legacy metric configs (no type) as metrics, but keep
fixed-size configs excluded
if ((type === 'metric' || (type === undefined && typeof value !==
'number')) && value != null) {
metricKey = value;
}
} else if (typeof config === 'string') {
metricKey = config;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current implementation only treats point_radius_fixed as a metric when
it's a string or an object explicitly marked with type==='metric'. That will
miss legacy saved-chart payloads where point_radius_fixed is an object with a
value pointing to a metric but lacks the type field. In those cases
transformProps may populate o.object.metric but label stays null so the metric
row is never rendered in the tooltip. The proposed change sensibly treats
object configs without a type as metrics when their value isn't numeric (which
matches how legacy metric descriptors behave elsewhere) while still avoiding
treating explicit numeric fixed-size configs as metrics. This fixes a real UX
regression for legacy charts rather than being merely stylistic.
</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/Scatter.tsx
**Line:** 50:53
**Comment:**
*Logic Error: The tooltip label resolution only treats
`point_radius_fixed` as a metric when it is a string or an object with `type
=== 'metric'`, ignoring legacy configurations where `point_radius_fixed` is an
object without a `type` field but still represents a metric; for such saved
charts, `o.object.metric` will be populated by `transformProps` but `label`
stays null, so the metric row is never rendered, breaking tooltips for those
legacy Scatter charts. The fix is to treat object configs without a `type` as
metric configs when their `value` is non-numeric (matching how legacy metric
structures are handled elsewhere), while still excluding fixed-size configs.
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/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.ts:
##########
@@ -38,7 +38,8 @@ import {
export interface DeckScatterFormData
extends Omit<SpatialFormData, 'color_picker'>, SqlaFormData {
point_radius_fixed?: {
Review Comment:
**Suggestion:** The current query-building logic only treats
`point_radius_fixed` as a metric when it is an object with `type: 'metric'`,
but elsewhere in the codebase (`Scatter.tsx`) the same field is also supported
as a plain string metric name; when `point_radius_fixed` is a string,
`buildQuery` will skip adding any metric or `orderby`, so the backend query
will not include the radius metric that `transformProps` expects, resulting in
missing radius values and incorrect rendering for charts saved with the legacy
string form. [logic error]
**Severity Level:** Minor ⚠️
--
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]
