codeant-ai-for-open-source[bot] commented on code in PR #40905:
URL: https://github.com/apache/superset/pull/40905#discussion_r3573253329
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -454,7 +448,7 @@ const FiltersConfigForm = (
formFilter?.filterType,
);
- const canDependOnOtherFilters = TYPES_SUPPORT_DEPENDENCIES.includes(
+ const canDependOnOtherFilters = filterSupportsDependencies(
formFilter?.filterType,
);
Review Comment:
**Suggestion:** The same `filterSupportsDependencies` predicate is now used
to decide whether to show the dependency picker for the current filter, but
that predicate is also used for “can be a dependency parent” checks and is
driven by `supportsCascadeDependencies`. This will incorrectly hide dependency
configuration for filters that are only marked as unsafe as parents (for
example time grain/time column), so they can no longer be configured as
dependent children. Split parent-eligibility and child-eligibility checks (or
use a child-specific check here) so `supportsCascadeDependencies` only controls
parent selection. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Time grain filters cannot be configured as dependency children.
- ⚠️ Time column filters lose cascading dependency configuration
capabilities.
- ⚠️ Native filter dependency UX inconsistent with ChartMetadata parent
semantics.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Register and use the core Time Grain or Time Column native filter plugins
on a
dashboard (plugins defined at
`superset-frontend/src/filters/components/TimeGrain/index.ts:7-14` and
`superset-frontend/src/filters/components/TimeColumn/index.ts:7-14`, both
with `behaviors:
[Behavior.InteractiveChart, Behavior.NativeFilter]` and
`supportsCascadeDependencies:
false`).
2. Open the “Manage native filters” modal for that dashboard and select a
Time Grain or
Time Column filter to edit; this instantiates `FiltersConfigForm` (component
at
`superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:281-299`)
for the current `filterId`, with `formFilter.filterType` set to
`filter_timegrain` or
`filter_timecolumn` (supported types declared in `constants.ts:27-38`).
3. Inside `FiltersConfigForm`, observe that `canDependOnOtherFilters` is
computed at
`FiltersConfigForm.tsx:451-453` as `const canDependOnOtherFilters =
filterSupportsDependencies(formFilter?.filterType);`, where
`filterSupportsDependencies()`
is implemented in `useFilterOperations.ts:33-41` to return
`metadata.supportsCascadeDependencies` when defined, otherwise fall back to
`metadata.behaviors?.includes(Behavior.NativeFilter)`. For
`filter_timegrain` and
`filter_timecolumn`, metadata has `supportsCascadeDependencies: false`, so
`filterSupportsDependencies()` returns `false` even though these plugins have
`Behavior.NativeFilter`.
4. Because `canDependOnOtherFilters` is false, the dependency UI is never
rendered: the
`<StyledRowFormItem>` containing `<DependencyList>` is only included when
`canDependOnOtherFilters && (hasAvailableFilters || dependencies.length >
0)` at
`FiltersConfigForm.tsx:1116-1152`. As a result, users cannot configure
“Values are
dependent on other filters” for Time Grain/Time Column filters, even though
`supportsCascadeDependencies` is documented in `ChartMetadata.ts:8-15` as
controlling
whether a native filter “can be selected as a dependency (cascade) parent”
(not child),
and `useFilterOperations.canBeUsedAsDependency()` at
`useFilterOperations.ts:152-163`
already correctly uses this flag for parent eligibility. This confirms the
bug: the
parent-only flag is reused to gate child dependency UI, hiding valid
dependent-child
configuration paths.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=26af989ef0fa429fb47f37389da6bc87&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=26af989ef0fa429fb47f37389da6bc87&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:** 451:453
**Comment:**
*Logic Error: The same `filterSupportsDependencies` predicate is now
used to decide whether to show the dependency picker for the current filter,
but that predicate is also used for “can be a dependency parent” checks and is
driven by `supportsCascadeDependencies`. This will incorrectly hide dependency
configuration for filters that are only marked as unsafe as parents (for
example time grain/time column), so they can no longer be configured as
dependent children. Split parent-eligibility and child-eligibility checks (or
use a child-specific check here) so `supportsCascadeDependencies` only controls
parent selection.
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=50dacd87f092ad116e93af9cc3f6166e88d32426df698ca674a98142844207a5&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40905&comment_hash=50dacd87f092ad116e93af9cc3f6166e88d32426df698ca674a98142844207a5&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx:
##########
@@ -64,6 +70,126 @@ 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;
Review Comment:
**Suggestion:** When a control `label` is a function that expects arguments,
`ControlLabel` resolves it to `undefined`, and several call sites do not
provide a fallback label. This renders unlabeled controls in the modal, which
is a functional UI regression; provide a deterministic fallback (for example
control name/default text) when non-zero-arity label functions cannot be
executed. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Native filter controls may render without any label text.
- ⚠️ Checkbox options become ambiguous, hurting usability and accessibility.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the unit test `getControlItemsMap.test.tsx` at
`superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.test.tsx:14-25`,
`getControlItems` is mocked to return a control item with
`config.renderTrigger: true` and
`config.label` / `config.description` defined as multi-argument functions
`(state:
unknown) => \`label:${state}\`` and `(state: unknown) =>
\`description:${state}\``.
2. `getControlItemsMap(props)` is invoked in that test
(`getControlItemsMap.test.tsx:26-27`). In production code, this hits the
render-trigger
branch of `getControlItemsMap` at `getControlItemsMap.tsx:295-367`, which
wraps the
control inside `StyledRowFormItem` and `Checkbox`, and passes
`controlItem.config.label`
and `controlItem.config.description` into `ControlLabel` without a
`fallbackLabel`
(`getControlItemsMap.tsx:356-359`).
3. Inside `ControlLabel` (`getControlItemsMap.tsx:88-124`), the
`resolvedLabel` is
computed at lines 101-106: because `label` is a function and `label.length`
is non-zero
(the function expects arguments), the ternary resolves to `undefined`
instead of invoking
the function, and there is no `fallbackLabel` supplied in these call sites.
Similarly,
`resolvedDescription` is computed at lines 107-112 and also becomes
`undefined` for
multi-argument functions.
4. As a result, when the Manage native filters modal renders such a control
through
`FiltersConfigForm` and `FilterContentRenderer`
(`FilterContentRenderer.tsx:12-29`), the
`Checkbox` at `getControlItemsMap.tsx:337-360` contains a `StyledLabel` with
no text or
tooltip, producing an unlabeled control option and a functional UI
regression whenever
plugins or configurations use function-shaped labels that require arguments.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2aa190e69c9143818e2350f833e5d950&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=2aa190e69c9143818e2350f833e5d950&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:** 101:106
**Comment:**
*Incomplete Implementation: When a control `label` is a function that
expects arguments, `ControlLabel` resolves it to `undefined`, and several call
sites do not provide a fallback label. This renders unlabeled controls in the
modal, which is a functional UI regression; provide a deterministic fallback
(for example control name/default text) when non-zero-arity label functions
cannot be executed.
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=c46480e7f94946434aa73f7e2744a46ccfe7c1a1c5d73ca1ccda83e83524bbb9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40905&comment_hash=c46480e7f94946434aa73f7e2744a46ccfe7c1a1c5d73ca1ccda83e83524bbb9&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx:
##########
@@ -64,6 +70,126 @@ 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;
+ // keep the saved value on a transient fetch failure; only clear
+ // on a confirmed miss from a successful response
+ setFetchState({ loadedForId: datasetId, fetchedColumns: [] });
+ });
+ return () => {
+ cancelled = true;
+ };
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [datasetId]);
Review Comment:
**Suggestion:** The async dataset fetch effect captures a stale `value`
because it only depends on `datasetId`. If the field value changes while the
request is in flight, the stale closure can incorrectly clear a newly selected
column (or fail to clear an outdated one). Split validation into a separate
effect keyed by `value`/fetched columns, or include the latest value via
ref/dependencies so the response handler validates against current state. [race
condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Plugin column picker clears or keeps wrong dataset column.
- ⚠️ Native filter configuration feels inconsistent and confusing users.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the Manage native filters modal in a dashboard, which renders
`FiltersConfigModal`
at
`superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx:9-22`
and, via `FilterContentRenderer`
(`ConfigModalContent/FilterContentRenderer.tsx:12-29`)
and `CustomizationContentRenderer`
(`ConfigModalContent/CustomizationContentRenderer.tsx:9-30`), mounts
`FiltersConfigForm`
(`FiltersConfigForm.tsx:20-35`).
2. In `FiltersConfigForm` (`FiltersConfigForm.tsx:20-35`),
`getControlItemsMap` is called
with the current `datasetId` from `getDatasetId()`
(`FiltersConfigForm.tsx:3-7`). For any
plugin control whose chart-control config has `config.isColumnSelect ===
true`,
`getControlItemsMap` builds a `StyledFormItem` and renders
`DatasetColumnSelect` as its
child (`getControlItemsMap.tsx:369-47`).
3. `DatasetColumnSelect` (defined in `getControlItemsMap.tsx:126-191`) uses
`useEffect`
with dependency array `[datasetId]` (`line 177`) to fetch column metadata via
`cachedSupersetGet` from `/api/v1/dataset/${datasetId}?q=${rison.encode({
columns:
['columns.column_name'] })}` (`lines 152-155`) and, on success, calls
`setFetchState` and
then validates the current `value` against the fetched `columnNames`,
clearing it via
`onChange(null)` when it is not present (`lines 162-165`).
4. If the user changes the column selection while the dataset fetch is
in-flight (the
`value` prop from the AntD `FormItem` changes, but `datasetId` remains the
same), the
effect does not re-run because its dependency list is `[datasetId]`, and the
`.then`
handler executed at `getControlItemsMap.tsx:157-165` still closes over the
stale `value`
from the render when the request was started. This can incorrectly clear a
previously
selected column or fail to clear a now-invalid selection, producing
inconsistent behavior
for plugin column-picker controls.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=061b1c6775b145efaab237bc0844a6d9&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=061b1c6775b145efaab237bc0844a6d9&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:** 163:177
**Comment:**
*Race Condition: The async dataset fetch effect captures a stale
`value` because it only depends on `datasetId`. If the field value changes
while the request is in flight, the stale closure can incorrectly clear a newly
selected column (or fail to clear an outdated one). Split validation into a
separate effect keyed by `value`/fetched columns, or include the latest value
via ref/dependencies so the response handler validates against current state.
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=179197853c1dd1f5b77ada49ea82d5de99aecc4f4f4365515412350103b9caf7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40905&comment_hash=179197853c1dd1f5b77ada49ea82d5de99aecc4f4f4365515412350103b9caf7&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]
