gabotorresruiz commented on code in PR #45069:
URL: https://github.com/apache/superset/pull/45069#discussion_r4276322537
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -386,12 +392,30 @@ const FiltersConfigForm = (
[loadedDatasets],
);
+ // The datasource chosen in the form. Its type comes from the selected
+ // value's own kind, so id and type are always read from the same value; the
+ // hidden type field is the fallback for values that carry no kind.
+ const formSelection: DatasourceBinding | undefined = formFilter?.dataset
+ ?.value
+ ? {
+ id: formFilter.dataset.value,
+ type:
+ formFilter.dataset.kind === 'semantic_view'
+ ? DatasourceType.SemanticView
+ : formFilter.datasourceType || DatasourceType.Table,
+ }
+ : undefined;
+
const showTimeRangePicker = useMemo(() => {
- const currentDataset = Object.values(loadedDatasets).find(
- dataset => dataset.id === formFilter?.dataset?.value,
- );
+ // Dashboard datasources are keyed by uid (`<id>__<type>`); an id-only
+ // lookup could find a same-numbered datasource of the other type.
+ const currentDataset = formSelection
+ ? loadedDatasets[
+ displayControlBindingKey(formSelection.id, formSelection.type)
Review Comment:
Just a small NIT: this file already imports `getDatasourceKey` from
`./ColumnSelect` (line 116) and it builds the same `<id>__<type>` string, so we
now have two helpers for one key. Could we use that one here and drop the
second import, or do you want the display control helper to be the canonical
one?
```suggestion
getDatasourceKey(formSelection.id, formSelection.type)
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -409,39 +433,47 @@ const FiltersConfigForm = (
isChartCustomization &&
itemTypeField === ChartCustomizationPlugins.DynamicGroupBy;
- const getDatasetId = () => {
- if (isChartCustomization) {
- if (formFilter?.dataset?.value) {
- return formFilter.dataset.value;
- }
- if (customizationToEdit?.targets?.[0]?.datasetId) {
- return customizationToEdit.targets[0].datasetId;
- }
- return mostUsedDataset(loadedDatasets, charts);
+ // Set when the bound datasource fails to load: the form stays unbound, with
+ // an empty, usable datasource select, until the editor chooses a datasource.
+ const [loadFailed, setLoadFailed] = useState(false);
+ // The datasource select is memoized with its first-render props and ignores
+ // later value changes; remounting it after a failure shows the cleared
value.
+ const [datasetSelectKey, setDatasetSelectKey] = useState(0);
Review Comment:
Not a blocker, more of a question: both this remount key and `bindingRef`
below exist to work around `DatasetSelect`'s `useMemo(() => <DatasetSelect
{...props} />, [])`, which has sat there without an explanation since the
component was added in #15480. Did you consider dropping that memo instead, so
`value` and `onChange` just flow through and neither workaround is needed? I
can see why you would not want to touch it inside a bug fix, given how much
this form re-renders, so a follow-up is fine too.
--
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]
