aminghadersohi commented on code in PR #44894:
URL: https://github.com/apache/superset/pull/44894#discussion_r4404426057
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -903,7 +927,18 @@ const AsyncSelect = forwardRef(
}
onClear={handleClear}
options={fullSelectOptions}
- optionRender={option => option.label || option.value}
+ optionRender={option =>
+ isNewOption(option.data) || isNewOption(option) ? (
+ <>
+ {option.label || option.value}
+ <StyledNewOptionBadge data-test="new-option-badge">
Review Comment:
`StyledNewOptionBadge` is not in this file's `./styles` import (line 84,
outside the diff), so typing a new option, e.g. a new dashboard name in Save
chart, throws `ReferenceError`. Add it to that import.
##########
superset-frontend/packages/superset-ui-core/src/components/Select/utils.test.ts:
##########
@@ -22,6 +22,7 @@ import {
stripSurroundingQuotes,
makeQuoteAwareTokenizer,
propertyComparator,
+ isNewOption,
} from './utils';
Review Comment:
`isEqual`, `getOption` and `hasOption` are called from line 190 on but never
imported, so this suite throws `ReferenceError`.
```suggestion
isNewOption,
isEqual,
getOption,
hasOption,
} from './utils';
```
##########
superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx:
##########
@@ -1054,7 +1058,18 @@ const Select = forwardRef(
)
}
options={visibleOptions}
- optionRender={option => option.label || option.value}
+ optionRender={option =>
+ isNewOption(option.data) || isNewOption(option) ? (
+ <>
+ {option.label || option.value}
+ <StyledNewOptionBadge data-test="new-option-badge">
Review Comment:
`StyledNewOptionBadge` is not in this file's `./styles` import (line 69,
outside the diff), so rendering any new option throws `ReferenceError` and the
dropdown crashes. Add it to that import.
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -370,6 +373,16 @@ const AsyncSelect = forwardRef(
fullSelectOptionsRef.current = fullSelectOptions;
+ const isRedundantNewOption = (
+ opt: DefaultOptionType | AntdLabeledValue,
+ incomingData: SelectOptionsType,
+ ) =>
+ isNewOption(opt) &&
+ ((typeof opt.label === 'string' || typeof opt.label === 'number'
+ ? hasOption(opt.label, incomingData, true)
+ : false) ||
+ hasOption(opt.value as RawValue | undefined, incomingData, true));
Review Comment:
`hasOption(..., true)` matches `value` before `label`, so typing `12` drops
the new entry whenever a result has id 12, even labelled `Report 12`; base
keeps it. Compare against labels only.
##########
superset-frontend/packages/superset-ui-core/src/components/Select/utils.test.ts:
##########
@@ -184,3 +185,70 @@ test('propertyComparator sorts mixed bigint and number
properties numerically',
bigHundred,
]);
});
+
+test('isEqual handles matching numbers and strings', () => {
+ expect(isEqual(1, 1, 'value')).toBe(true);
+ expect(isEqual(1, '1', 'value')).toBe(true);
+ expect(isEqual({ value: 1 }, 1, 'value')).toBe(true);
+ expect(isEqual({ value: 1 }, '1', 'value')).toBe(true);
+ expect(isEqual(' test ', 'test', 'value')).toBe(true);
+ expect(isEqual({ value: 'abc' }, { value: 'abc' }, 'value')).toBe(true);
+ expect(isEqual({ value: 'abc' }, { value: 'def' }, 'value')).toBe(false);
+});
+
+test('isEqual distinguishes empty strings and whitespace while preserving
deduplication', () => {
+ expect(isEqual('', ' ', 'value')).toBe(false);
+ expect(isEqual(' ', '', 'value')).toBe(false);
+ expect(isEqual('', ' ', 'value')).toBe(false);
+ expect(isEqual(' ', ' ', 'value')).toBe(false);
+ expect(isEqual('', '', 'value')).toBe(true);
+ expect(isEqual(' ', ' ', 'value')).toBe(true);
+ expect(isEqual(' test ', 'test', 'value')).toBe(true);
+ expect(isEqual(' Sales ', 'Sales', 'value')).toBe(true);
+ expect(isEqual('Sales', ' Sales ', 'value')).toBe(true);
+ expect(isEqual('Sales', 'sales', 'value')).toBe(false);
+ expect(isEqual(0, '', 'value')).toBe(false);
+ expect(isEqual(0, ' ', 'value')).toBe(false);
+ expect(isEqual(0, '0', 'value')).toBe(true);
+ expect(isEqual(1, '1', 'value')).toBe(true);
+ expect(isEqual(null, undefined, 'value')).toBe(true);
+ expect(isEqual(null, '', 'value')).toBe(false);
+ expect(isEqual({ value: 0 }, '', 'value')).toBe(false);
+ expect(isEqual({ value: 0 }, '0', 'value')).toBe(true);
+ expect(isEqual({ label: ' Sales ' }, 'Sales', 'label')).toBe(true);
+});
+
+test('getOption and hasOption locate options by value or label', () => {
+ const options = [
+ { label: 'Option 1', value: 1 },
+ { label: 'Option 2', value: 2 },
+ { label: 'New Option', value: 'New Option', isNewOption: true },
+ ];
+ expect(hasOption(1, options)).toBe(true);
+ expect(hasOption(99, options)).toBe(false);
+ expect(hasOption('Option 2', options, true)).toBe(true);
+ expect(hasOption('Option 2', options, false)).toBe(false);
+ expect(getOption(2, options)).toEqual({ label: 'Option 2', value: 2 });
+ expect(getOption('Option 1', options, true)).toEqual({
+ label: 'Option 1',
+ value: 1,
+ });
+ expect(getOption({ value: 2 }, options)).toEqual({
Review Comment:
TS2345 here and at line 240: `getOption`/`hasOption` declare `value: V`
(utils.tsx:87 and 117, outside the diff), so object arguments fail `npm run
type`. Widen both to `value: V | CustomLabeledValue`.
##########
superset-frontend/packages/superset-ui-core/src/components/Select/utils.tsx:
##########
@@ -36,6 +36,12 @@ export function isLabeledValue(value: unknown): value is
AntdLabeledValue {
return isObject(value) && 'value' in value && 'label' in value;
}
+export function isNewOption(
+ option: unknown,
+): option is { isNewOption: boolean } {
+ return isObject(option) && Boolean((option as { isNewOption?: boolean
}).isNewOption);
+}
Review Comment:
This predicate narrows options to `{ isNewOption: boolean }`, so
`opt.label`/`opt.value` in AsyncSelect.tsx (382-385, 503) fail tsc with TS2339,
and line 42 fails oxfmt. A `boolean` return type compiles.
```suggestion
export function isNewOption(option: unknown): boolean {
return isObject(option) && Boolean(option.isNewOption);
}
```
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.test.tsx:
##########
@@ -1195,6 +1232,66 @@ test('preserves new option entry across search fetch
when allowNewOptions is on'
expect(screen.queryByText('Option 0')).not.toBeInTheDocument();
});
+test('drops new option entry when fetched search options contain a matching
label (regression for #44873)', async () => {
+ const page0Data = Array.from({ length: 10 }, (_, i) => ({
+ label: `Option ${i}`,
+ value: i,
+ }));
+ const searchData = [{ label: 'Matching Option', value: 42 }];
+ const loadOptions = jest.fn(async (search: string) => {
+ if (search === '') {
+ return { data: page0Data, totalCount: 100 };
+ }
+ return { data: searchData, totalCount: 1 };
+ });
+
+ render(
+ <AsyncSelect {...defaultProps} options={loadOptions} allowNewOptions />,
+ );
+ await open();
+ await waitFor(() => expect(loadOptions).toHaveBeenCalledTimes(1));
+
+ await type('Matching Option');
+ await waitFor(() => expect(loadOptions).toHaveBeenCalledTimes(2));
+
+ const options = await findAllSelectOptions();
+ // Drops optimistic new option entry when server returns a match for the
label
+ expect(options).toHaveLength(1);
+ expect(options[0]).toHaveTextContent('Matching Option');
+ expect(screen.queryByText('Option 0')).not.toBeInTheDocument();
+});
+
+test('drops optimistic new option when paginated mergeData contains matching
label', async () => {
+ const initialData = [{ label: 'Existing 1', value: 1 }];
+ const nextPageData = [{ label: 'Created Later', value: 99 }];
+ const loadOptions = jest.fn(async (_search: string, page: number) => {
+ if (page === 0) {
+ return { data: initialData, totalCount: 2 };
+ }
+ return { data: nextPageData, totalCount: 2 };
+ });
+
+ const { rerender } = render(
+ <AsyncSelect {...defaultProps} options={loadOptions} allowNewOptions />,
+ );
+ await open();
+ await waitFor(() => expect(loadOptions).toHaveBeenCalledTimes(1));
+
+ // Type a term that will match next page's label
+ await type('Created Later');
+ await waitFor(() => expect(loadOptions).toHaveBeenCalledTimes(2));
+ const options = await findAllSelectOptions();
+ expect(
+ Array.from(options).some(opt => opt.textContent?.includes('Created
Later')),
+ ).toBe(true);
+
+ // When next page loads and mergeData fires, duplicate optimistic option is
dropped
+ rerender(
+ <AsyncSelect {...defaultProps} options={loadOptions} allowNewOptions />,
+ );
+ expect(loadOptions).toBeDefined();
Review Comment:
This test never loads a second page and ends on
`expect(loadOptions).toBeDefined()`, so it cannot fail: removing the
`isRedundantNewOption` check from `mergeData` leaves the whole file passing.
--
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]
