codeant-ai-for-open-source[bot] commented on code in PR #40905:
URL: https://github.com/apache/superset/pull/40905#discussion_r3529990242
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx:
##########
@@ -64,6 +70,127 @@ 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,
+ fallbackLabel,
+}: {
+ label?: BaseControlConfig['label'];
+ description?: BaseControlConfig['description'];
+ fallbackLabel?: ReactNode;
+}) {
+ // Only zero-argument label/description functions are safe to invoke here:
+ // (state, controlState, chartState) are supplied by the Explore control
+ // panel renderer (ControlPanelsContainer), which this filter-config-modal
+ // control list does not have access to.
+ const resolvedLabel =
+ (typeof label === 'function'
+ ? label.length === 0
+ ? (label as () => ReactNode)()
+ : undefined
+ : label) ?? fallbackLabel;
+ const resolvedDescription =
+ typeof description === 'function'
+ ? description.length === 0
+ ? (description as () => ReactNode)()
+ : undefined
+ : description;
+ return (
+ <StyledLabel>
+ {resolvedLabel}
+ {resolvedDescription != null && (
+ <>
+
+ <InfoTooltip placement="top" tooltip={resolvedDescription} />
+ </>
+ )}
+ </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) {
+ // dataset cleared — drop any stale selection immediately
+ if (value) {
+ onChange?.(null);
+ }
+ return undefined;
+ }
+ let cancelled = false;
+ cachedSupersetGet({
+ endpoint: `/api/v1/dataset/${datasetId}?q=${rison.encode({
+ columns: ['columns.column_name'],
+ })}`,
+ })
+ .then(({ json: { result } }) => {
+ if (cancelled) return;
+ const columnNames: string[] = result.columns
+ .map((col: { column_name: string }) => col.column_name)
+ .filter(Boolean);
+ setFetchState({ loadedForId: datasetId, fetchedColumns: columnNames });
+ if (value && !columnNames.includes(value)) {
+ onChange?.(null);
+ }
+ })
+ .catch(() => {
+ if (cancelled) return;
+ setFetchState({ loadedForId: datasetId, fetchedColumns: [] });
+ if (value) {
+ onChange?.(null);
+ }
+ });
Review Comment:
**Suggestion:** The error path clears the current column value whenever the
dataset-column fetch fails, so a transient API/network failure will silently
wipe an already-saved plugin column selection and trigger downstream
default-mask reset logic even though the user did not change anything. Keep the
existing value on fetch failure and only clear when you positively know the
selected column is invalid from a successful response. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Native filter plugin column-picker loses selection on fetch failure.
⚠️ Default data mask resets unexpectedly on transient dataset API errors.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a dashboard and launch the Manage native filters modal, which renders
`FiltersConfigForm` at
`/workspace/superset/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:410-449`;
this calls `getControlItemsMap` with `datasetId` and `filterToEdit` (line
10-24 of that
snippet).
2. Register or use a filter plugin whose control panel item has
`config.isColumnSelect ===
true` so that `getControlItemsMap` renders the plugin column picker block at
`/workspace/superset/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx:111-154`,
wrapping `DatasetColumnSelect` in a `StyledFormItem` with `name={['filters',
filterId,
'controlValues', controlItem.name]}` and `initialValue` bound to the
previously saved
column (lines 115-125).
3. Ensure the filter already has a saved column selection in
`controlValues[controlItem.name]` so that AntD Form injects this as the
`value` prop into
`DatasetColumnSelect` when the modal opens; the component’s effect (lines
143-178) calls
`cachedSupersetGet` to fetch dataset columns from
`/api/v1/dataset/${datasetId}` (lines
152-156).
4. Trigger a transient failure for that dataset API call (e.g., by forcing
the endpoint to
return 500 or simulating a network error) so `cachedSupersetGet` rejects;
the `.catch`
block at lines 167-173 executes, and because `value` still contains the
previously saved
column, `if (value) { onChange?.(null); }` clears the form field, wiping the
user’s column
selection and (via the Form-controlled `onChange` plus the plugin’s
`onChange` callback at
lines 138-146) also resets `defaultDataMask` even though the user did not
change the
column.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6614622892f54508a99f745fcebed1d0&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=6614622892f54508a99f745fcebed1d0&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:** 167:173
**Comment:**
*Logic Error: The error path clears the current column value whenever
the dataset-column fetch fails, so a transient API/network failure will
silently wipe an already-saved plugin column selection and trigger downstream
default-mask reset logic even though the user did not change anything. Keep the
existing value on fetch failure and only clear when you positively know the
selected column is invalid from a successful response.
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=cbcef8e85fde4888ad918fb7a2f5487b88c5761d20087c3ac3769a510dd6b806&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40905&comment_hash=cbcef8e85fde4888ad918fb7a2f5487b88c5761d20087c3ac3769a510dd6b806&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]
