bito-code-review[bot] commented on code in PR #40905:
URL: https://github.com/apache/superset/pull/40905#discussion_r3382270606
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Missing error handling for API call</b></div>
<div id="fix">
Add a `.catch()` handler to the `cachedSupersetGet` promise in
`DatasetColumnSelect` to handle API errors. Inside the catch, call
`addDangerToast` to notify users of failures and reset the fetched columns,
e.g.: `.catch(error => { setFetchState({ loadedForId: datasetId,
fetchedColumns: [] }); addDangerToast({ message: t('Failed to load columns'),
level: 'error' }); });`
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
The provided patch adds a `.catch()` handler but does not implement the
user notification (`addDangerToast`) described in the comment.
```
</div>
</details>
</div>
<small><i>Code Review Run #d2347d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx:
##########
@@ -217,33 +304,50 @@ export default function getControlItemsMap({
defaultDataMask: null,
});
}
- formChanged();
- forceUpdate();
+ notifyChange();
}}
>
- <>
- {typeof controlItem.config.label === 'function'
- ? (controlItem.config.label as Function)()
- : controlItem.config.label}
-
- {controlItem.config.description && (
- <InfoTooltip
- placement="top"
- tooltip={
- typeof controlItem.config.description === 'function'
- ? (controlItem.config.description as Function)()
- : (controlItem.config.description as React.ReactNode)
- }
- />
- )}
- </>
+ <ControlLabel
+ label={controlItem.config.label}
+ description={controlItem.config.description}
+ />
</Checkbox>
</StyledRowFormItem>
</Tooltip>
</>
);
mapControlItems[controlItem.name] = { element, checked: initialValue };
});
+
+ // Render plugin-declared column-picker controls using config hooks
+ controlItems
+ .filter((item: CustomControlItem) => item?.config?.isColumnSelect === true)
+ .forEach(controlItem => {
+ const initialValue = resolveInitialValue(
+ controlItem,
+ filterToEdit,
+ customizationToEdit,
+ );
+ const element = (
+ <StyledFormItem
+ expanded={expanded}
+ name={['filters', filterId, 'controlValues', controlItem.name]}
+ initialValue={initialValue}
+ label={
+ <ControlLabel
+ label={controlItem.config?.label}
+ description={controlItem.config?.description}
+ />
+ }
+ >
+ <DatasetColumnSelect datasetId={datasetId} onChange={notifyChange} />
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing value prop for saved data restoration</b></div>
<div id="fix">
The `DatasetColumnSelect` component is rendered at line 343 without the
`initialValue` prop, but the parent computes `initialValue` at line 326-330
using `resolveInitialValue()`. When editing an existing filter configuration,
the previously saved column value will not be restored, causing a confusing UX
where the saved value appears lost.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
---
a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
+++
b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
@@ -340,7 +340,10 @@ export default function getControlItemsMap({
/>
}
>
- <DatasetColumnSelect datasetId={datasetId}
onChange={notifyChange} />
+ <DatasetColumnSelect
+ datasetId={datasetId}
+ value={initialValue}
+ onChange={notifyChange}
+ />
```
</div>
</details>
</div>
<small><i>Code Review Run #d2347d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Divider edits silently dropped on save</b></div>
<div id="fix">
Reverting the divider-change-detection logic reintroduces a regression where
editing an existing divider's title or description marks the divider as
modified, enables the Save button, and includes the divider in the save
payload. Without this, `canSave` stays false and divider edits are silently
lost. Additionally, restore the `isDivider` import from `./utils`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
---
a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
+++
b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
@@ -57,6 +57,7 @@ import {
isFilterId,
isChartCustomizationId,
transformDividerId,
+ isDivider,
} from './utils';
import { ConfigModalContent } from './ConfigModalContent';
import ConfigModalSidebar from './ConfigModalSidebar';
@@ -470,9 +471,17 @@ function FiltersConfigModal({
[modalSaveLogic, setSaveAlertVisible],
);
- const handleValuesChange = useMemo(
- () => debouncedHandleErroredItems,
- [debouncedHandleErroredItems],
+ 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],
);
```
</div>
</details>
</div>
<small><i>Code Review Run #d2347d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
