feat(select): keep options order when in single mode (#19085)

superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx

@@ -106,8 +106,6 @@ const groupByControl: SharedControlConfig<'SelectControl', ColumnMeta> = {
     'One or many columns to group by. High cardinality groupings should include a sort by metric ' +
       'and series limit to limit the number of fetched and rendered series.',
   ),
-  sortComparator: (a: { label: string }, b: { label: string }) =>
-    a.label.localeCompare(b.label),
   optionRenderer: c => <ColumnOption showType column={c} />,
   valueRenderer: c => <ColumnOption column={c} />,
   valueKey: 'column_name',

superset-frontend/packages/superset-ui-core/src/utils/index.ts

@@ -21,6 +21,7 @@ export { default as ensureIsArray } from './ensureIsArray';
 export { default as ensureIsInt } from './ensureIsInt';
 export { default as isDefined } from './isDefined';
 export { default as isRequired } from './isRequired';
+export { default as isEqualArray } from './isEqualArray';
 export { default as makeSingleton } from './makeSingleton';
 export { default as promiseTimeout } from './promiseTimeout';
 export { default as logging } from './logging';

superset-frontend/packages/superset-ui-core/src/utils/isEqualArray.test.ts

@@ -0,0 +1,31 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import isEqualArray from './isEqualArray';
+
+test('isEqualArray', () => {
+  expect(isEqualArray([], [])).toBe(true);
+  expect(isEqualArray([1, 2], [1, 2])).toBe(true);
+  const item1 = { a: 1 };
+  expect(isEqualArray([item1], [item1])).toBe(true);
+  expect(isEqualArray(null, undefined)).toBe(true);
+  // compare is shallow
+  expect(isEqualArray([{ a: 1 }], [{ a: 1 }])).toBe(false);
+  expect(isEqualArray(null, [])).toBe(false);
+  expect(isEqualArray([1, 2], [])).toBe(false);
+});

superset-frontend/plugins/plugin-chart-table/src/utils/isEqualArray.ts → superset-frontend/packages/superset-ui-core/src/utils/isEqualArray.ts

@@ -23,9 +23,11 @@ export default function isEqualArray<T extends unknown[] | undefined | null>(
   return (
     arrA === arrB ||
     (!arrA && !arrB) ||
-    (arrA &&
+    !!(
+      arrA &&
       arrB &&
       arrA.length === arrB.length &&
-      arrA.every((x, i) => x === arrB[i]))
+      arrA.every((x, i) => x === arrB[i])
+    )
   );
 }

superset-frontend/packages/superset-ui-core/test/tsconfig.json

@@ -6,11 +6,7 @@
     "rootDir": "."
   },
   "extends": "../../../tsconfig.json",
-  "include": [
-    "**/*",
-    "../types/**/*",
-    "../../../types/**/*"
-  ],
+  "include": ["**/*", "../types/**/*", "../../../types/**/*"],
   "references": [
     {
       "path": ".."

superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx

@@ -116,8 +116,6 @@ const all_columns: typeof sharedControls.groupby = {
         ? [t('must have a value')]
         : [],
   }),
-  sortComparator: (a: { label: string }, b: { label: string }) =>
-    a.label.localeCompare(b.label),
   visibility: isRawMode,
 };
 
@@ -279,8 +277,6 @@ const config: ControlPanelConfig = {
                 choices: datasource?.order_by_choices || [],
               }),
               visibility: isRawMode,
-              sortComparator: (a: { label: string }, b: { label: string }) =>
-                a.label.localeCompare(b.label),
             },
           },
         ],

superset-frontend/plugins/plugin-chart-table/src/utils/isEqualColumns.ts

@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import isEqualArray from './isEqualArray';
+import { isEqualArray } from '@superset-ui/core';
 import { TableChartProps } from '../types';
 
 export default function isEqualColumns(

superset-frontend/plugins/plugin-chart-table/tsconfig.json

@@ -4,16 +4,9 @@
     "outDir": "lib",
     "rootDir": "src"
   },
-  "exclude": [
-    "lib",
-    "test"
-  ],
+  "exclude": ["lib", "test"],
   "extends": "../../tsconfig.json",
-  "include": [
-    "src/**/*",
-    "types/**/*",
-    "../../types/**/*",
-  ],
+  "include": ["src/**/*", "types/**/*", "../../types/**/*"],
   "references": [
     {
       "path": "../../packages/superset-ui-chart-controls"

superset-frontend/src/components/Select/Select.stories.tsx

@@ -187,10 +187,10 @@ export const InteractiveSelect = ({
 );
 
 InteractiveSelect.args = {
-  autoFocus: false,
+  autoFocus: true,
   allowNewOptions: false,
   allowClear: false,
-  showSearch: false,
+  showSearch: true,
   disabled: false,
   invertSelection: false,
   placeholder: 'Select ...',

superset-frontend/src/components/Select/Select.test.tsx

@@ -99,6 +99,18 @@ const findAllSelectValues = () =>
 
 const clearAll = () => userEvent.click(screen.getByLabelText('close-circle'));
 
+const matchOrder = async (expectedLabels: string[]) => {
+  const actualLabels: string[] = [];
+  (await findAllSelectOptions()).forEach(option => {
+    actualLabels.push(option.textContent || '');
+  });
+  // menu is a virtual list, which means it may not render all options
+  expect(actualLabels.slice(0, expectedLabels.length)).toEqual(
+    expectedLabels.slice(0, actualLabels.length),
+  );
+  return true;
+};
+
 const type = (text: string) => {
   const select = getSelect();
   userEvent.clear(select);
@@ -169,34 +181,64 @@ test('sort the options using a custom sort comparator', async () => {
   });
 });
 
-test('displays the selected values first', async () => {
-  render(<Select {...defaultProps} mode="multiple" />);
-  const option3 = OPTIONS[2].label;
-  const option8 = OPTIONS[7].label;
+test('should sort selected to top when in single mode', async () => {
+  render(<Select {...defaultProps} mode="single" />);
+  const originalLabels = OPTIONS.map(option => option.label);
+  await open();
+  userEvent.click(await findSelectOption(originalLabels[1]));
+  // after selection, keep the original order
+  expect(await matchOrder(originalLabels)).toBe(true);
+
+  // order selected to top when reopen
+  await type('{esc}');
+  await open();
+  let labels = originalLabels.slice();
+  labels = labels.splice(1, 1).concat(labels);
+  expect(await matchOrder(labels)).toBe(true);
+
+  // keep clicking other items, the updated order should still based on
+  // original order
+  userEvent.click(await findSelectOption(originalLabels[5]));
+  await matchOrder(labels);
+  await type('{esc}');
   await open();
-  userEvent.click(await findSelectOption(option3));
-  userEvent.click(await findSelectOption(option8));
+  labels = originalLabels.slice();
+  labels = labels.splice(5, 1).concat(labels);
+  expect(await matchOrder(labels)).toBe(true);
+
+  // should revert to original order
+  clearAll();
   await type('{esc}');
   await open();
-  const sortedOptions = await findAllSelectOptions();
-  expect(sortedOptions[0]).toHaveTextContent(option3);
-  expect(sortedOptions[1]).toHaveTextContent(option8);
+  expect(await matchOrder(originalLabels)).toBe(true);
 });
 
-test('displays the original order when unselecting', async () => {
+test('should sort selected to the top when in multi mode', async () => {
   render(<Select {...defaultProps} mode="multiple" />);
-  const option3 = OPTIONS[2].label;
-  const option8 = OPTIONS[7].label;
+  const originalLabels = OPTIONS.map(option => option.label);
+  let labels = originalLabels.slice();
+
   await open();
-  userEvent.click(await findSelectOption(option3));
-  userEvent.click(await findSelectOption(option8));
+  userEvent.click(await findSelectOption(labels[1]));
+  expect(await matchOrder(labels)).toBe(true);
+
   await type('{esc}');
+  await open();
+  labels = labels.splice(1, 1).concat(labels);
+  expect(await matchOrder(labels)).toBe(true);
+
+  await open();
+  userEvent.click(await findSelectOption(labels[5]));
+  await type('{esc}');
+  await open();
+  labels = [labels.splice(0, 1)[0], labels.splice(4, 1)[0]].concat(labels);
+  expect(await matchOrder(labels)).toBe(true);
+
+  // should revert to original order
   clearAll();
+  await type('{esc}');
   await open();
-  const options = await findAllSelectOptions();
-  options.forEach((option, key) =>
-    expect(option).toHaveTextContent(OPTIONS[key].label),
-  );
+  expect(await matchOrder(originalLabels)).toBe(true);
 });
 
 test('searches for label or value', async () => {
@@ -540,17 +582,35 @@ test('async - changes the selected item in single mode', async () => {
 test('async - deselects an item in multiple mode', async () => {
   render(<Select {...defaultProps} options={loadOptions} mode="multiple" />);
   await open();
-  const [firstOption, secondOption] = OPTIONS;
-  userEvent.click(await findSelectOption(firstOption.label));
-  userEvent.click(await findSelectOption(secondOption.label));
+  const option3 = OPTIONS[2];
+  const option8 = OPTIONS[7];
+  userEvent.click(await findSelectOption(option8.label));
+  userEvent.click(await findSelectOption(option3.label));
+
+  let options = await findAllSelectOptions();
+  expect(options).toHaveLength(Math.min(defaultProps.pageSize, OPTIONS.length));
+  expect(options[0]).toHaveTextContent(OPTIONS[0].label);
+  expect(options[1]).toHaveTextContent(OPTIONS[1].label);
+
+  await type('{esc}');
+  await open();
+
+  // should rank selected options to the top after menu closes
+  options = await findAllSelectOptions();
+  expect(options).toHaveLength(Math.min(defaultProps.pageSize, OPTIONS.length));
+  expect(options[0]).toHaveTextContent(option3.label);
+  expect(options[1]).toHaveTextContent(option8.label);
+
   let values = await findAllSelectValues();
-  expect(values.length).toBe(2);
-  expect(values[0]).toHaveTextContent(firstOption.label);
-  expect(values[1]).toHaveTextContent(secondOption.label);
-  userEvent.click(await findSelectOption(firstOption.label));
+  expect(values).toHaveLength(2);
+  // should keep the order by which the options were selected
+  expect(values[0]).toHaveTextContent(option8.label);
+  expect(values[1]).toHaveTextContent(option3.label);
+
+  userEvent.click(await findSelectOption(option3.label));
   values = await findAllSelectValues();
   expect(values.length).toBe(1);
-  expect(values[0]).toHaveTextContent(secondOption.label);
+  expect(values[0]).toHaveTextContent(option8.label);
 });
 
 test('async - adds a new option if none is available and allowNewOptions is true', async () => {

superset-frontend/src/components/Select/Select.tsx

@@ -156,6 +156,10 @@ export interface SelectProps extends PickedSelectProps {
    * Works in async mode only (See the options property).
    */
   onError?: (error: string) => void;
+  /**
+   * Customize how filtered options are sorted while users search.
+   * Will not apply to predefined `options` array when users are not searching.
+   */
   sortComparator?: typeof DEFAULT_SORT_COMPARATOR;
 }
 
@@ -314,8 +318,6 @@ const Select = (
   const isAsync = typeof options === 'function';
   const isSingleMode = mode === 'single';
   const shouldShowSearch = isAsync || allowNewOptions ? true : showSearch;
-  const initialOptions =
-    options && Array.isArray(options) ? options.slice() : EMPTY_OPTIONS;
   const [selectValue, setSelectValue] = useState(value);
   const [inputValue, setInputValue] = useState('');
   const [isLoading, setIsLoading] = useState(loading);
@@ -346,13 +348,27 @@ const Select = (
       sortSelectedFirst(a, b) || sortComparator(a, b, inputValue),
     [inputValue, sortComparator, sortSelectedFirst],
   );
-  const sortComparatorWithoutSearch = useCallback(
+  const sortComparatorForNoSearch = useCallback(
     (a: AntdLabeledValue, b: AntdLabeledValue) =>
-      sortSelectedFirst(a, b) || sortComparator(a, b, ''),
-    [sortComparator, sortSelectedFirst],
+      sortSelectedFirst(a, b) ||
+      // Only apply the custom sorter in async mode because we should
+      // preserve the options order as much as possible.
+      (isAsync ? sortComparator(a, b, '') : 0),
+    [isAsync, sortComparator, sortSelectedFirst],
+  );
+
+  const initialOptions = useMemo(
+    () => (options && Array.isArray(options) ? options.slice() : EMPTY_OPTIONS),
+    [options],
   );
+  const initialOptionsSorted = useMemo(
+    () => initialOptions.slice().sort(sortComparatorForNoSearch),
+    [initialOptions, sortComparatorForNoSearch],
+  );
+
   const [selectOptions, setSelectOptions] =
-    useState<OptionsType>(initialOptions);
+    useState<OptionsType>(initialOptionsSorted);
+
   // add selected values to options list if they are not in it
   const fullSelectOptions = useMemo(() => {
     const missingValues: OptionsType = ensureIsArray(selectValue)
@@ -433,13 +449,13 @@ const Select = (
           mergedData = prevOptions
             .filter(previousOption => !dataValues.has(previousOption.value))
             .concat(data)
-            .sort(sortComparatorWithoutSearch);
+            .sort(sortComparatorForNoSearch);
           return mergedData;
         });
       }
       return mergedData;
     },
-    [sortComparatorWithoutSearch],
+    [sortComparatorForNoSearch],
   );
 
   const fetchPage = useMemo(
@@ -575,11 +591,13 @@ const Select = (
     }
     // if no search input value, force sort options because it won't be sorted by
     // `filterSort`.
-    if (isDropdownVisible && !inputValue && fullSelectOptions.length > 0) {
-      const sortedOptions = [...fullSelectOptions].sort(
-        sortComparatorWithSearch,
-      );
-      if (!isEqual(sortedOptions, fullSelectOptions)) {
+    if (isDropdownVisible && !inputValue && selectOptions.length > 1) {
+      const sortedOptions = isAsync
+        ? selectOptions.slice().sort(sortComparatorForNoSearch)
+        : // if not in async mode, revert to the original select options
+          // (with selected options still sorted to the top)
+          initialOptionsSorted;
+      if (!isEqual(sortedOptions, selectOptions)) {
         setSelectOptions(sortedOptions);
       }
     }
@@ -624,10 +642,8 @@ const Select = (
     // when `options` list is updated from component prop, reset states
     fetchedQueries.current.clear();
     setAllValuesLoaded(false);
-    setSelectOptions(
-      options && Array.isArray(options) ? options : EMPTY_OPTIONS,
-    );
-  }, [options]);
+    setSelectOptions(initialOptions);
+  }, [initialOptions]);
 
   useEffect(() => {
     setSelectValue(value);

superset-frontend/src/components/TimezoneSelector/index.tsx

@@ -99,6 +99,7 @@ const TIMEZONE_OPTIONS_SORT_COMPARATOR = (
   moment.tz(currentDate, a.timezoneName).utcOffset() -
   moment.tz(currentDate, b.timezoneName).utcOffset();
 
+// pre-sort timezone options by time offset
 TIMEZONE_OPTIONS.sort(TIMEZONE_OPTIONS_SORT_COMPARATOR);
 
 const matchTimezoneToOptions = (timezone: string) =>

superset-frontend/src/dashboard/util/filterboxMigrationHelper.ts

@@ -510,7 +510,8 @@ export default function getNativeFilterConfig(
               childComponent.filterType as FILTER_COMPONENT_FILTER_TYPES,
             )
           ) {
-            childComponent.cascadeParentIds ||= [];
+            childComponent.cascadeParentIds =
+              childComponent.cascadeParentIds || [];
             childComponent.cascadeParentIds.push(parentComponentId);
           }
         });