codeant-ai-for-open-source[bot] commented on code in PR #40905:
URL: https://github.com/apache/superset/pull/40905#discussion_r3501411729
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx:
##########
@@ -64,6 +70,118 @@ 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) return;
+ let cancelled = false;
+ cachedSupersetGet({
+ endpoint: `/api/v1/dataset/${datasetId}?q=${rison.encode({
+ columns: ['columns.column_name'],
+ })}`,
+ })
+ .then(({ json: { result } }) => {
+ if (!cancelled) {
+ setFetchState({
+ loadedForId: datasetId,
+ fetchedColumns: result.columns
+ .map((col: { column_name: string }) => col.column_name)
+ .filter(Boolean),
Review Comment:
**Suggestion:** The dataset-column picker fetches new column options but
never validates or clears the currently selected value when the dataset
changes, so a stale column from the previous dataset can remain in form state
and be saved as an invalid control value. After loading columns (or when
dataset is cleared), explicitly reset the field via `onChange(null)` when the
current value is not present in the fetched column list. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Plugin column-picker saves columns no longer in dataset.
- ⚠️ Native filter configuration may reference invalid dataset columns.
- ⚠️ Filter behavior undefined when plugin reads stale column.
- ⚠️ Users changing datasets keep invisible stale column selection.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `FiltersConfigForm.tsx:146-163`, note that `datasetId` is derived from
the current
filter's dataset selection (`getDatasetId()`), and in
`FiltersConfigForm.tsx:180-195` this
`datasetId` is passed into `getControlItemsMap({ datasetId, ... })`, so any
user changing
the dataset in the native filter configuration will cause `datasetId` to
change for all
controls, including plugin-declared column pickers.
2. In `getControlItemsMap.tsx:361-399`, when a control item has
`config.isColumnSelect ===
true`, the code renders a `StyledFormItem` bound to `['filters', filterId,
'controlValues', controlItem.name]` and a `DatasetColumnSelect` child with
`datasetId={datasetId}` but no explicit `value` prop; AntD-style `FormItem`
injects the
current form field value and `onChange` into `DatasetColumnSelect`, so the
selected column
name is stored in form state under
`filters[filterId].controlValues[controlItem.name]`.
3. The `DatasetColumnSelect` implementation in
`getControlItemsMap.tsx:126-183` contains a
`useEffect` (lines 143-169) that, on `datasetId` changes, calls
`cachedSupersetGet` to
fetch columns for `/api/v1/dataset/${datasetId}?q=...`, then updates local
state via
`setFetchState({ loadedForId: datasetId, fetchedColumns: ... })`; this
effect never reads
the current `value` prop nor calls the injected `onChange`, so it does not
validate or
clear a previously selected column when the dataset changes.
4. Contrast this with the core `ColumnSelect` in `ColumnSelect.tsx:89-115`,
where
`useChangeEffect(datasetId, ...)` explicitly calls `resetColumnField()`
(which uses
`form.setFields` to null out the column field) and also checks `valueExists`
against
`result.columns`, clearing the field if the selected column no longer
exists; no analogous
reset exists for `DatasetColumnSelect`, so a concrete real-world
sequence—selecting a
column in a plugin `isColumnSelect` control, then changing the filter's
dataset (updating
`datasetId`), and saving—will leave
`filters[filterId].controlValues[controlItem.name]`
set to a column name that is not present in the new dataset's
`fetchedColumns`, resulting
in a stale, invalid control value being persisted.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c0602957684a4aacbc856c3c6ed48dc6&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=c0602957684a4aacbc856c3c6ed48dc6&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:** 144:157
**Comment:**
*Logic Error: The dataset-column picker fetches new column options but
never validates or clears the currently selected value when the dataset
changes, so a stale column from the previous dataset can remain in form state
and be saved as an invalid control value. After loading columns (or when
dataset is cleared), explicitly reset the field via `onChange(null)` when the
current value is not present in the fetched column list.
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=fdedaaa94a9ef640f998a46935f61fb288a51dc31a51482d157aa86c338d8f73&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40905&comment_hash=fdedaaa94a9ef640f998a46935f61fb288a51dc31a51482d157aa86c338d8f73&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]
