codeant-ai-for-open-source[bot] commented on code in PR #40220:
URL: https://github.com/apache/superset/pull/40220#discussion_r3275994712
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -1418,7 +1418,7 @@ const FiltersConfigForm = (
}}
/>
</StyledRowFormItem>
- {hasMetrics && (
+ {hasMetrics && !isChartCustomization && (
Review Comment:
**Suggestion:** Hiding the Sort Metric field for chart customizations
without clearing any existing `controlValues.sortMetric` leaves legacy
metric-based sorting active in saved configurations. Because downstream
query-building still applies `sortMetric` to `order_by_cols`, users can no
longer see or remove that setting from the UI, causing persistent hidden
behavior. Clear `sortMetric` when `isChartCustomization` is true (for example
during sort toggle updates or before save) so hidden state is not retained.
[incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dynamic Group By charts keep invisible metric-based sorting.
- ⚠️ Users cannot clear legacy sortMetric via configuration.
- ⚠️ Embedded exports include outdated order_by_cols from customizations.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Migrate or load a dashboard containing a Dynamic Group By chart
customization that
previously used a sort metric, so
`ChartCustomization.controlValues.sortMetric` is
populated via `migrateChartCustomization()` in
`superset-frontend/src/dashboard/util/migrateChartCustomization.ts` lines
20–23
(`sortMetric: customization.sortMetric`).
2. Open the Filters/Customizations configuration UI for that item, which
renders
`FiltersConfigForm`
(`superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx`);
for chart customizations `isChartCustomization` is set at line 309 (`const
isChartCustomization = itemType === 'chartCustomization';`).
3. In the "Sort display control values" section, toggle sorting on/off or
change
ascending/descending; `onSortChanged` at lines 628–635 only updates
`controlValues.sortAscending` via `setNativeFilterFieldValues` and never
touches
`controlValues.sortMetric`, while the Sort Metric `<Select>` is now hidden
for chart
customizations because it is guarded by `{hasMetrics &&
!isChartCustomization && (` at
line 1421, so the existing `sortMetric` value remains stored but is no
longer visible or
editable in the UI.
4. Trigger a chart query that uses chart customizations (e.g., through
`getChartDataPayloads` in `superset-frontend/src/embedded/utils.ts` lines
77–131 or via
the tested helper in
`superset-frontend/src/dashboard/util/getFormDataWithExtraFilters.test.ts`),
which calls
`getFormDataWithExtraFilters`
(`superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts`).
Inside
`processGroupByCustomizations` at lines 187–191, the code reads `const
sortMetric =
item.controlValues?.sortMetric;` and, if set, applies `order_by_cols =
[JSON.stringify([sortMetric, !sortAscending])]`, so the host chart's query
is still sorted
by the legacy `sortMetric` even though the user can no longer see or clear
that value from
the configuration UI.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0b291cf26d8944599ca78a87734df552&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0b291cf26d8944599ca78a87734df552&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/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
**Line:** 1421:1421
**Comment:**
*Incomplete Implementation: Hiding the Sort Metric field for chart
customizations without clearing any existing `controlValues.sortMetric` leaves
legacy metric-based sorting active in saved configurations. Because downstream
query-building still applies `sortMetric` to `order_by_cols`, users can no
longer see or remove that setting from the UI, causing persistent hidden
behavior. Clear `sortMetric` when `isChartCustomization` is true (for example
during sort toggle updates or before save) so hidden state is not retained.
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%2F40220&comment_hash=2d8b8fa61c4f89de920c96ea790491a99050538741c3cef69ef7a367f7460f3a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40220&comment_hash=2d8b8fa61c4f89de920c96ea790491a99050538741c3cef69ef7a367f7460f3a&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]
