Copilot commented on code in PR #40905:
URL: https://github.com/apache/superset/pull/40905#discussion_r3382620142
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx:
##########
@@ -49,20 +49,19 @@ import {
useFilterOperations,
useCustomizationOperations,
useModalSaveLogic,
- ALLOW_DEPENDENCIES,
+ filterSupportsDependencies,
} from './hooks';
import {
getFilterIds,
getChartCustomizationIds,
isFilterId,
isChartCustomizationId,
transformDividerId,
- isDivider,
} from './utils';
Review Comment:
The suggested `handleValuesChange` fix relies on `isDivider`, but it is no
longer imported from `./utils`. Without re-adding it, divider changes still
won't be tracked.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx:
##########
@@ -471,20 +470,9 @@ function FiltersConfigModal({
[modalSaveLogic, setSaveAlertVisible],
);
- const handleValuesChange = useCallback(
- (changedValues: Partial<NativeFiltersForm>) => {
- debouncedHandleErroredItems();
- // DividerConfigForm doesn't call handleModifyItem on change the way
- // FiltersConfigForm does, so detect divider field changes here and mark
- // the divider as modified so canSave becomes true and the save payload
- // includes the updated divider values.
- Object.keys(changedValues?.filters ?? {}).forEach(id => {
- if (isDivider(id)) {
- handleModifyItem(id);
- }
- });
- },
- [debouncedHandleErroredItems, handleModifyItem],
+ const handleValuesChange = useMemo(
+ () => debouncedHandleErroredItems,
+ [debouncedHandleErroredItems],
);
Review Comment:
`onValuesChange` no longer marks divider items as modified. Since `canSave`
is derived from `filterState/customizationState.changes.modified`, edits in
`DividerConfigForm` won't enable the Save button or be included in the save
payload.
##########
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>
+ );
+}
Review Comment:
`ControlLabel` can receive `label`/`description` values that are functions
(many control panels define `label: () => ...` / `description: () => ...`).
Rendering the function directly triggers React warnings, and
`String(description)` can display "[object Object]" instead of the intended
tooltip content.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -93,7 +89,7 @@ import {
} from 'src/dashboard/components/nativeFilters/utils';
import { DatasetSelectLabel } from 'src/features/datasets/DatasetSelectLabel';
import {
- ALLOW_DEPENDENCIES as TYPES_SUPPORT_DEPENDENCIES,
+ filterSupportsDependencies,
getFiltersConfigModalTestId,
} from '../FiltersConfigModal';
import { FilterRemoval, NativeFiltersForm } from '../types';
Review Comment:
The semantic-layers feature flag intentionally switches terminology between
"Dataset" and "Datasource" (see `src/features/semanticLayers/label.ts`). This
file removed `datasetLabel()` usage and hardcodes `t('Dataset')`, which makes
the config modal inconsistent when `SEMANTIC_LAYERS` is enabled.
##########
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),
+ });
+ });
+ }, [datasetId]);
+
+ return (
+ <Select
+ ariaLabel={t('Column')}
+ loading={loading}
+ value={value ?? undefined}
+ onChange={val => onChange?.(typeof val === 'string' ? val : null)}
+ options={options.map(col => ({ label: col, value: col }))}
+ placeholder={t('None — show filter values as labels')}
+ showSearch
+ />
+ );
Review Comment:
`DatasetColumnSelect` doesn't handle request failures/cancellation. If the
dataset changes quickly or the component unmounts, the in-flight promise can
still call `setFetchState`, and if the request errors the control will stay in
a perpetual loading state (`loadedForId` never updates). Also, without
`allowClear`, users can't return to the "None" state despite the placeholder
suggesting it.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx:
##########
@@ -171,10 +257,11 @@ export default function getControlItemsMap({
controlItem.name !== 'operatorType',
)
Review Comment:
`operatorType` is explicitly excluded from `controlItems` rendering, but the
dedicated UI for configuring Select filter match type was also removed from
`FiltersConfigForm`. As a result, Select filters lose the ability to configure
`operatorType` (Exact/ILIKE variants) via the config modal.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -1080,10 +992,7 @@ const FiltersConfigForm = (
rules={[
{
required: !isRemoved,
- message:
- datasetLabel() === t('Datasource')
- ? t('Datasource is required')
- : t('Dataset is required'),
+ message: t('Dataset is required'),
},
]}
Review Comment:
To keep terminology consistent with the semantic-layers feature flag, the
dataset form items should use `datasetLabel()` (and derive the validation
message from it) rather than hardcoding `t('Dataset')`.
##########
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:
With `allowClear`, AntD Select calls `onChange(undefined)` when cleared. The
current handler ignores `undefined`, which makes it impossible to clear an
already-selected `sortMetric` value from the form.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -1109,7 +1018,7 @@ const FiltersConfigForm = (
) : (
<StyledFormItem
expanded={expanded}
- label={<StyledLabel>{datasetLabel()}</StyledLabel>}
+ label={<StyledLabel>{t('Dataset')}</StyledLabel>}
>
<Loading position="inline-centered" />
</StyledFormItem>
Review Comment:
The loading state label should also use `datasetLabel()` so the config modal
stays consistent under the `SEMANTIC_LAYERS` feature flag.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -1661,67 +1501,6 @@ const FiltersConfigForm = (
hidden
initialValue={null}
/>
- {!isChartCustomization &&
- itemTypeField === 'filter_select' && (
- <StyledRowFormItem
- expanded={expanded}
- name={[
- 'filters',
- filterId,
- 'controlValues',
- 'operatorType',
- ]}
- initialValue={currentOperatorType}
- label={
- <>
- <StyledLabel>{t('Match
type')}</StyledLabel>
-
- <InfoTooltip
- placement="top"
- tooltip={t(
- 'Warning: ILIKE queries may be slow on
large datasets as they cannot use indexes effectively.',
- )}
- />
- </>
- }
- >
- <Select
- ariaLabel={t('Match type')}
- options={[
- {
- value: SelectFilterOperatorType.Exact,
- label: t('Exact match (IN)'),
- },
- ...(selectedColumnIsString
- ? [
- {
- value:
-
SelectFilterOperatorType.Contains,
- label: t(
- 'Contains text (ILIKE %x%)',
- ),
- },
- {
- value:
-
SelectFilterOperatorType.StartsWith,
- label: t('Starts with (ILIKE x%)'),
- },
- {
- value:
-
SelectFilterOperatorType.EndsWith,
- label: t('Ends with (ILIKE %x)'),
- },
- ]
- : []),
- ]}
- onChange={value => {
- onOperatorTypeChanged(
- value as SelectFilterOperatorType,
- );
- }}
- />
- </StyledRowFormItem>
- )}
<FormItem
name={['filters', filterId, 'defaultValue']}
Review Comment:
The Select filter "Match type" (`operatorType`) configuration UI was removed
from `FiltersConfigForm`, and `getControlItemsMap` still excludes
`operatorType` from its generic rendering. This makes it impossible to
configure `operatorType` in the native filter config modal, even though the
Select filter control panel defines it and the runtime plugin supports it.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -1297,78 +1206,6 @@ const FiltersConfigForm = (
</CollapsibleControl>
</FormItem>
)}
- {itemTypeField === 'filter_timegrain' &&
- hasDataset &&
- datasetDetails?.time_grain_sqla &&
- datasetDetails.time_grain_sqla.length > 0 &&
(
- <FormItem
- name={[
- 'filters',
- filterId,
- 'preFilterTimegrain',
- ]}
- >
- <CollapsibleControl
- initialValue={hasTimeGrainPreFilter}
- title={t('Pre-filter available
values')}
- tooltip={t(
- 'Select which time grains are
available in the filter control. This is a UI allow list only and does not add
extra conditions to the underlying queries.',
- )}
- onChange={checked => {
- if (!checked) {
- setNativeFilterFieldValues(
- form,
- filterId,
- { time_grains: undefined },
- );
- forceUpdate();
- }
- formChanged();
- }}
- >
- <FormItem
- name={[
- 'filters',
- filterId,
- 'time_grains',
- ]}
- initialValue={
-
filterToEditWithTimeGrains?.time_grains
- }
- {...getFiltersConfigModalTestId(
- 'time-grain-allowlist',
- )}
- >
- <Select
- mode="multiple"
- ariaLabel={t('Time grain options')}
- options={getTimeGrainOptions(
- datasetDetails.time_grain_sqla,
- )}
- sortComparator={() => 0}
- onChange={(values: string[]) => {
- setNativeFilterFieldValues(
- form,
- filterId,
- {
- time_grains:
- values.length > 0 &&
- values.length <
- datasetDetails
- .time_grain_sqla.length
- ? values
- : undefined,
- },
- );
- forceUpdate();
- formChanged();
- }}
- css={{ width: INPUT_WIDTH }}
- />
- </FormItem>
- </CollapsibleControl>
- </FormItem>
- )}
{itemTypeField !== 'filter_range' ? (
<FormItem
Review Comment:
The time grain filter pre-filter allowlist UI (`time_grains`) appears to
have been removed from the config modal. This prevents dashboard owners from
configuring the `time_grains` allowlist that is still supported at runtime by
`PluginFilterTimegrain` (and exercised by
`TimeGrainPreFilter.integration.test.tsx`).
--
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]
