codeant-ai-for-open-source[bot] commented on code in PR #40905:
URL: https://github.com/apache/superset/pull/40905#discussion_r3382175221
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -1462,21 +1299,24 @@ const FiltersConfigForm = (
}),
)}
onChange={value => {
- const previous =
-
form.getFieldValue('filters')?.[
- filterId
- ].controlValues || {};
- setNativeFilterFieldValues(
- form,
- filterId,
- {
- controlValues: {
- ...previous,
- sortMetric: value,
+ if (value !== undefined) {
+ const previous =
+ form.getFieldValue(
+ 'filters',
+ )?.[filterId].controlValues
||
+ {};
+ setNativeFilterFieldValues(
+ form,
+ filterId,
+ {
+ controlValues: {
+ ...previous,
+ sortMetric: value,
+ },
},
- },
- );
- forceUpdate();
+ );
+ forceUpdate();
+ }
formChanged();
}}
Review Comment:
**Suggestion:** Clearing the "Sort Metric" select no longer persists because
updates are skipped when the new value is `undefined`; with `allowClear`, this
leaves the old metric in form state and silently ignores the user's clear
action. Always write the field back (including cleared/empty values) so cleared
metrics are actually removed. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Native filter sort metric cannot be reliably cleared.
- ⚠️ Dashboard filters may keep unintended metric-based ordering.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the Manage native filters modal, which renders `FiltersConfigForm`
from
`superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:275-745`
for a filter whose dataset metrics are loaded via `cachedSupersetGet` at
lines 678-715.
2. In the "Sort filter values" section (same file, lines 1210-1257), enable
sorting and
choose a metric in the "Sort Metric" `<Select>` defined at lines 1259-1323,
which updates
`filters[filterId].controlValues.sortMetric` through the `onChange={value =>
{ ... }}`
handler.
3. Click the clear ("x") icon on the Sort Metric control; the `Select`
implementation in
`packages/superset-ui-core/src/components/Select/Select.tsx` (lines 339-361
and 619-628)
calls the external `onChange` with `values` set to `undefined` when a
single-value select
is cleared.
4. The handler at `FiltersConfigForm.tsx:1301-1321` only calls
`setNativeFilterFieldValues()` when `value !== undefined`, so clearing leaves
`filters[filterId].controlValues.sortMetric` unchanged; saving the modal
keeps the
previous metric-based sort even though the UI field appears empty.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7823b6b2a2e64fe19f616da3b6d70d13&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=7823b6b2a2e64fe19f616da3b6d70d13&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:** 1302:1321
**Comment:**
*Incorrect Condition Logic: Clearing the "Sort Metric" select no longer
persists because updates are skipped when the new value is `undefined`; with
`allowClear`, this leaves the old metric in form state and silently ignores the
user's clear action. Always write the field back (including cleared/empty
values) so cleared metrics are actually removed.
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%2F40905&comment_hash=25bcf2586d8bf390a12e1435acc19b0c2deb304968071cd45a42c9de41ef5836&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40905&comment_hash=25bcf2586d8bf390a12e1435acc19b0c2deb304968071cd45a42c9de41ef5836&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx:
##########
@@ -64,6 +67,87 @@ const CleanFormItem = styled(FormItem)`
margin-bottom: 0;
`;
+/** Resolves the saved or default initial value for a control. */
+function resolveInitialValue(
+ controlItem: CustomControlItem,
+ filterToEdit?: ControlItemsProps['filterToEdit'],
+ customizationToEdit?: ControlItemsProps['customizationToEdit'],
+) {
+ return (
+ filterToEdit?.controlValues?.[controlItem.name] ??
+ customizationToEdit?.controlValues?.[controlItem.name] ??
+ controlItem?.config?.default ??
+ null
+ );
+}
+
+/** Renders a StyledLabel with an optional description tooltip. */
+function ControlLabel({
+ label,
+ description,
+}: {
+ label?: ReactNode;
+ description?: ReactNode;
+}) {
+ return (
+ <StyledLabel>
+ {label}
+ {description && (
+ <>
+
+ <InfoTooltip placement="top" tooltip={String(description)} />
Review Comment:
**Suggestion:** Converting tooltip descriptions with `String(...)` breaks
non-string ReactNode descriptions by rendering `[object Object]` instead of the
intended content. Pass the description through as a React node so
plugin-provided rich descriptions display correctly. [logic error]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Rich tooltip descriptions render as "[object Object]" text.
- ⚠️ Plugin tooltip content loses formatting and explanatory detail.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Define a filter control in a chart or filter plugin whose control config
uses a rich
ReactNode description, e.g. `description: (<span><strong>Advanced</strong>
option</span>)`, which is explicitly allowed by
`BaseControlConfig['description']` being
typed as `ReactNode | ((...) => ReactNode)` in
`packages/superset-ui-chart-controls/src/types.ts:246-253`.
2. Use this control as part of a native filter plugin
(Behavior.NativeFilter) so that
`getControlItemsMap` renders it; for render-trigger controls, `ControlLabel`
is used
inside `getControlItemsMap` at
`superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx:85-103`
with `description={controlItem.config.description}` (lines 310-313).
3. At runtime, `ControlLabel` renders `<InfoTooltip placement="top"
tooltip={String(description)} />` at line 98, coercing the ReactNode
description into a
string.
4. The `InfoTooltip` component in
`packages/superset-ui-core/src/components/InfoTooltip/index.tsx:34-43,119-122`
passes
`tooltip` as the AntD Tooltip `title`, which expects a ReactNode; because it
receives a
stringified React element (e.g. `[object Object]`), the tooltip displays
`[object Object]`
instead of the rich description content.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1efad51af0c3423797619a63dec339ca&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=1efad51af0c3423797619a63dec339ca&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/getControlItemsMap.tsx
**Line:** 98:98
**Comment:**
*Logic Error: Converting tooltip descriptions with `String(...)` breaks
non-string ReactNode descriptions by rendering `[object Object]` instead of the
intended content. Pass the description through as a React node so
plugin-provided rich descriptions display correctly.
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%2F40905&comment_hash=9a96b8eb945bd65f84b39ae0bb8bd4ca99abf25153086244f2d78b0c877ba9d0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40905&comment_hash=9a96b8eb945bd65f84b39ae0bb8bd4ca99abf25153086244f2d78b0c877ba9d0&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx:
##########
@@ -64,6 +67,87 @@ const CleanFormItem = styled(FormItem)`
margin-bottom: 0;
`;
+/** Resolves the saved or default initial value for a control. */
+function resolveInitialValue(
+ controlItem: CustomControlItem,
+ filterToEdit?: ControlItemsProps['filterToEdit'],
+ customizationToEdit?: ControlItemsProps['customizationToEdit'],
+) {
+ return (
+ filterToEdit?.controlValues?.[controlItem.name] ??
+ customizationToEdit?.controlValues?.[controlItem.name] ??
+ controlItem?.config?.default ??
+ null
+ );
+}
+
+/** Renders a StyledLabel with an optional description tooltip. */
+function ControlLabel({
+ label,
+ description,
+}: {
+ label?: ReactNode;
+ description?: ReactNode;
+}) {
+ return (
+ <StyledLabel>
+ {label}
+ {description && (
+ <>
+
+ <InfoTooltip placement="top" tooltip={String(description)} />
+ </>
+ )}
+ </StyledLabel>
+ );
+}
+
+function DatasetColumnSelect({
+ datasetId,
+ value,
+ onChange,
+}: {
+ datasetId?: number;
+ value?: string | null;
+ onChange?: (value: string | null) => void;
+}) {
+ const [{ loadedForId, fetchedColumns }, setFetchState] = useState<{
+ loadedForId?: number;
+ fetchedColumns: string[];
+ }>({ fetchedColumns: [] });
+
+ const loading = !!(datasetId && loadedForId !== datasetId);
+ const options = loadedForId === datasetId ? fetchedColumns : [];
+
+ useEffect(() => {
+ if (!datasetId) return;
+ cachedSupersetGet({
+ endpoint: `/api/v1/dataset/${datasetId}?q=${rison.encode({
+ columns: ['columns.column_name'],
+ })}`,
+ }).then(({ json: { result } }) => {
+ setFetchState({
+ loadedForId: datasetId,
+ fetchedColumns: result.columns
+ .map((col: { column_name: string }) => col.column_name)
+ .filter(Boolean),
+ });
+ });
Review Comment:
**Suggestion:** The async dataset-column fetch has a stale-response race:
when `datasetId` changes quickly, an older request can resolve after a newer
one and overwrite state for the wrong dataset, leaving the current dataset with
empty/incorrect column options. Guard state updates with a request token/abort
flag and ignore responses that are no longer for the latest `datasetId`. [race
condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Plugin column picker shows no options after dataset change.
- ⚠️ Native filter plugin configuration breaks for dynamic datasets.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Register a native filter plugin whose control panel declares a custom
control with
`config.isColumnSelect === true` (supported by `CustomControlItem.config` in
`packages/superset-ui-chart-controls/src/types.ts:375-379`), so
`getControlItemsMap` in
`superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx:151-355`
renders a `DatasetColumnSelect`.
2. Open the Manage native filters modal so `FiltersConfigForm`
(`FiltersConfigForm.tsx:407-421`) calls `getControlItemsMap` with a
`datasetId` derived
from the currently selected dataset, and `DatasetColumnSelect` (lines
105-149) starts
loading columns via its `useEffect` that calls `cachedSupersetGet` at lines
122-135.
3. Quickly change the filter's dataset in the UI from dataset A to dataset
B; this
triggers `DatasetColumnSelect`'s `useEffect` twice (for A and then B),
issuing two
`/api/v1/dataset/{datasetId}` requests while sharing the same
`setFetchState` updater at
lines 124-135.
4. If the slower request (for dataset A) resolves after the faster request
(for dataset
B), the late response sets `loadedForId` back to A while `datasetId` prop is
B; with
`loading = !!(datasetId && loadedForId !== datasetId)` and `options =
loadedForId ===
datasetId ? fetchedColumns : []` (lines 119-121), the column picker for the
current
dataset B remains in a perpetual loading state with empty options,
preventing column
selection.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d5966501f6084592a3a2d8ede2269870&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=d5966501f6084592a3a2d8ede2269870&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/getControlItemsMap.tsx
**Line:** 124:135
**Comment:**
*Race Condition: The async dataset-column fetch has a stale-response
race: when `datasetId` changes quickly, an older request can resolve after a
newer one and overwrite state for the wrong dataset, leaving the current
dataset with empty/incorrect column options. Guard state updates with a request
token/abort flag and ignore responses that are no longer for the latest
`datasetId`.
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%2F40905&comment_hash=93f1825b80a62404fbd9b3c4aebc9d3a2cbef840670d6140f2fcd27c735f7be8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40905&comment_hash=93f1825b80a62404fbd9b3c4aebc9d3a2cbef840670d6140f2fcd27c735f7be8&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]
