EnxDev commented on code in PR #45168:
URL: https://github.com/apache/superset/pull/45168#discussion_r4270496632
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -475,8 +505,17 @@ const AsyncSelect = forwardRef(
// typed when the server returns no match.
setSelectOptions(prevOptions => {
const dataValues = new Set(data.map(opt => opt.value));
+ const dataLabels = caseSensitiveNewOptionMatching
+ ? undefined
+ : new Set(
+ data.map(option => normalizeOptionLabel(option.label)),
+ );
const preservedNew = prevOptions.filter(
- opt => opt.isNewOption && !dataValues.has(opt.value),
+ opt =>
+ opt.isNewOption &&
+ (hasOption(opt.value, selectValueRef.current) ||
Review Comment:
None of the new tests reach this `hasOption(opt.value,
selectValueRef.current)` branch, and it's the case the dashboard picker cares
about most: the user picks the typed name before the lookup comes back.
Worth a test that selects the new option, then resolves a matching search
response and checks it's still there.
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.test.tsx:
##########
@@ -1721,6 +1748,130 @@ test('cancels pending debounce on unmount', async () =>
{
expect(mockOnSearch).not.toHaveBeenCalled();
});
+test.each([
+ ['My dashboard', 'My dashboard', 123],
+ ['test dashboard', 'Test Dashboard', 456],
+ [' Spaced Name ', 'Spaced Name', 999],
+])(
+ 'uses the server option when its label matches a new option (%s)',
+ async (search, label, value) => {
+ const loadOptions = jest.fn(async (query: string) =>
+ query === search
+ ? { data: [{ label, value }], totalCount: 1 }
+ : { data: [], totalCount: 100 },
+ );
+ const onChange = jest.fn();
+
+ render(
+ <AsyncSelect
+ {...defaultProps}
+ allowNewOptions
+ caseSensitiveNewOptionMatching={false}
+ filterOption={false}
+ onChange={onChange}
+ options={loadOptions}
+ />,
+ );
+
+ await open();
+ await type(search);
+ await waitFor(() =>
+ expect(loadOptions).toHaveBeenCalledWith(
+ search,
+ 0,
+ defaultProps.pageSize,
+ ),
+ );
+ const options = await findAllSelectOptions();
+ expect(options).toHaveLength(1);
+ expect(options[0]).toHaveTextContent(label);
+
+ await userEvent.click(await findSelectOption(label));
+ await waitFor(() =>
+ expect(onChange).toHaveBeenCalledWith(
+ expect.objectContaining({ label, value }),
+ expect.objectContaining({ label, value }),
+ ),
+ );
+ },
+);
+
+test('preserves a new option when the server response has no matching label',
async () => {
+ const loadOptions = jest.fn(async (search: string) =>
+ search === 'New Dashboard'
+ ? { data: [{ label: 'Other Dashboard', value: 789 }], totalCount: 1 }
+ : { data: [], totalCount: 100 },
+ );
+
+ render(
+ <AsyncSelect
+ {...defaultProps}
+ allowNewOptions
+ filterOption={false}
+ options={loadOptions}
+ />,
+ );
+
+ await open();
+ await type('New Dashboard');
+ await waitFor(() =>
+ expect(loadOptions).toHaveBeenCalledWith(
+ 'New Dashboard',
+ 0,
+ defaultProps.pageSize,
+ ),
+ );
+ const options = await findAllSelectOptions();
+ expect(options).toHaveLength(2);
+ expect(options[0]).toHaveTextContent('New Dashboard');
+ expect(options[1]).toHaveTextContent('Other Dashboard');
+});
+
+test('removes a new option when a pending base fetch returns a matching
label', async () => {
+ let resolveBase: (value: {
+ data: { label: string; value: number }[];
+ totalCount: number;
+ }) => void = () => {};
+ const loadOptions = jest.fn((search: string) => {
+ if (!search) {
+ return new Promise<{
+ data: { label: string; value: number }[];
+ totalCount: number;
+ }>(resolve => {
+ resolveBase = resolve;
+ });
+ }
+ return Promise.resolve({ data: [], totalCount: 100 });
+ });
+
+ render(
+ <AsyncSelect
+ {...defaultProps}
+ allowNewOptions
Review Comment:
This one renders without `caseSensitiveNewOptionMatching={false}`, so it
only passes because of the ungated filter in `mergeData`.
Mind adding the prop here, plus a default-mode twin where the base page
returns `my dashboard` and the typed `My dashboard` survives?
##########
superset-frontend/packages/superset-ui-core/src/components/Select/types.ts:
##########
@@ -104,6 +104,12 @@ export interface BaseSelectProps extends AntdExposedProps {
* False by default.
* */
allowNewOptions?: boolean;
+ /**
+ * Whether labels are matched exactly before creating a new option.
+ * Set to false when the backing resource treats labels case-insensitively.
+ * True by default.
+ */
+ caseSensitiveNewOptionMatching?: boolean;
Review Comment:
Since this lives on `BaseSelectProps`, `<Select
caseSensitiveNewOptionMatching={false}>` type-checks but does nothing. The sync
`Select` never reads it and just spreads it through `...props` to antd.
Moving it to `AsyncSelectProps` would keep it from looking supported there.
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -376,10 +395,21 @@ const AsyncSelect = forwardRef(
if (data && Array.isArray(data) && data.length) {
// unique option values should always be case sensitive so don't
lowercase
const dataValues = new Set(data.map(opt => opt.value));
+ const dataLabels = new Set(
Review Comment:
`dataLabels` here isn't gated on `caseSensitiveNewOptionMatching`.
`mergeData` handles a base list that finishes loading mid-search and page 2+ of
a search, so default-mode callers like the recipient selects in
`NotificationMethod` still lose a typed option when a trimmed, case-insensitive
match lands.
Could this get the same `caseSensitiveNewOptionMatching ? undefined : new
Set(...)` gate as line 508, with `dataLabels?.has` below and the prop added to
the deps at line 427?
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -257,10 +263,23 @@ const AsyncSelect = forwardRef(
? { value: opt.value, label: opt.label }
: { value: opt, label: String(opt) },
);
- return missingValues.length > 0
+ const combined = missingValues.length > 0
? missingValues.concat(selectOptions)
: selectOptions;
- }, [selectOptions, selectValue]);
+
+ if (caseSensitiveNewOptionMatching) {
+ return combined;
+ }
+
+ return combined.filter(opt => {
+ if (!opt.isNewOption) return true;
+ const optLabel = String(opt.label).trim().toLowerCase();
+ return !combined.some(
+ other => !other.isNewOption &&
+ String(other.label).trim().toLowerCase() === optLabel
+ );
+ });
Review Comment:
pre-commit is failing because oxfmt rewrites this block. While it's open:
this is the only copy of the rule that drops a new option the user already
selected (lines 408 and 516 keep it), and it re-implements
`normalizeOptionLabel` inline.
Something like this keeps the three paths consistent, then a quick oxfmt
pass:
```suggestion
const combined =
missingValues.length > 0
? missingValues.concat(selectOptions)
: selectOptions;
if (caseSensitiveNewOptionMatching) {
return combined;
}
const serverLabels = new Set(
combined
.filter(option => !option.isNewOption)
.map(option => normalizeOptionLabel(option.label)),
);
return combined.filter(
option =>
!option.isNewOption ||
hasOption(option.value, selectValue) ||
!serverLabels.has(normalizeOptionLabel(option.label)),
);
```
--
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]
