codeant-ai-for-open-source[bot] commented on code in PR #39068:
URL: https://github.com/apache/superset/pull/39068#discussion_r3531270147
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -268,6 +278,8 @@ const AsyncSelect = forwardRef(
}
return previousState;
});
+ setInputValue('');
Review Comment:
**Suggestion:** This unconditionally clears the search input in multi-select
mode, which bypasses the `autoClearSearchValue` prop contract and changes
behavior for callers that intentionally disable auto-clearing. Only clear the
input when `autoClearSearchValue` is true. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Multi-select AsyncSelect ignores autoClearSearchValue=false configuration.
⚠️ Search text cannot be preserved across selections when desired.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Review the multi-select selection handler in
`superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:137-173`,
where the `else` branch of `handleOnSelect` (triggered when `mode !==
'single'`) appends
the new selection to `selectValue`.
2. In that multi-select branch, immediately after updating `selectValue`,
the code calls
`setInputValue('');` at `AsyncSelect.tsx:281` and then
`setSelectOptions(prev =>
prev.sort(sortComparatorForNoSearch));`, clearing the controlled
`inputValue` state
regardless of the `autoClearSearchValue` prop.
3. Beneath this block, a separate `if (autoClearSearchValue) {
setInputValue(''); ... }`
conditional at `AsyncSelect.tsx:166-171` is meant to honor the caller’s
`autoClearSearchValue` configuration (for example,
`SelectFilterPlugin.tsx:610-36` passes
`autoClearSearchValue` when using the related Select wrapper), but in
AsyncSelect’s
multi-select path the earlier unconditional `setInputValue('');` has already
cleared the
search text even when `autoClearSearchValue` is `false`.
4. If a caller configures AsyncSelect in multi-select mode with
`autoClearSearchValue={false}` (e.g. by extending the existing multi-select
usages in
`AccessSection.tsx:11-25` to keep search text after selection), they will
still see the
search input cleared on every selection because of the unconditional call at
line 281,
violating the expected API contract that `autoClearSearchValue` controls
whether the input
is cleared.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fb7e08aac4d749c2a340e37958fd5a44&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=fb7e08aac4d749c2a340e37958fd5a44&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx
**Line:** 281:281
**Comment:**
*Api Mismatch: This unconditionally clears the search input in
multi-select mode, which bypasses the `autoClearSearchValue` prop contract and
changes behavior for callers that intentionally disable auto-clearing. Only
clear the input when `autoClearSearchValue` is true.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39068&comment_hash=a7cd86e308c0a063de1b0dc5100bdd8bd8ceb72ac73022318a584c9eea80b974&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39068&comment_hash=a7cd86e308c0a063de1b0dc5100bdd8bd8ceb72ac73022318a584c9eea80b974&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -268,6 +278,8 @@ const AsyncSelect = forwardRef(
}
return previousState;
});
+ setInputValue('');
+ setSelectOptions(prev => prev.sort(sortComparatorForNoSearch));
Review Comment:
**Suggestion:** The state update sorts `prev` in place and returns the same
array reference, which mutates React state directly and can prevent a re-render
because the reference does not change. Sort a copied array instead so state
remains immutable and updates are reliably rendered. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Multi-select option order updates may not re-render.
⚠️ Direct state mutation risks subtle React rendering bugs.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the dashboard properties modal and navigate to the access section,
which renders
the multi-select AsyncSelect for owners at
`src/dashboard/components/PropertiesModal/sections/AccessSection.tsx:11-25`
with
`mode="multiple"` and `showSearch`.
2. In the owners AsyncSelect, select several owner values so the control is
populated;
this triggers `handleOnSelect` in `AsyncSelect.tsx:137-173`, which in
multi-select mode
appends the selected item to `selectValue` and then executes
`setSelectOptions(prev =>
prev.sort(sortComparatorForNoSearch));` at `AsyncSelect.tsx:150-164`.
3. Click again on an already-selected owner value so that
`setSelectValue(previousState =>
{ ... })` returns `previousState` unchanged (duplicate selection branch at
`AsyncSelect.tsx:150-160`), meaning React will see no change in
`selectValue`; the
subsequent `setSelectOptions(prev => prev.sort(...))` mutates the `prev`
array in place
and returns the same reference.
4. Because React compares the previous and next state by reference,
`setSelectOptions`
receives `prev` and returns the identical array reference (only mutated in
place), so it
can bail out of re-rendering; option ordering changes from the in-place
`.sort` may not be
reflected until some unrelated state change re-renders the component,
demonstrating direct
state mutation and potentially missed UI updates.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1ac4742983634829922420656683e25e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1ac4742983634829922420656683e25e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx
**Line:** 282:282
**Comment:**
*Logic Error: The state update sorts `prev` in place and returns the
same array reference, which mutates React state directly and can prevent a
re-render because the reference does not change. Sort a copied array instead so
state remains immutable and updates are reliably rendered.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39068&comment_hash=bde29478b5cd484b08643d02529941b0513efe192d4b4adbbdaaa66464f66bc4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39068&comment_hash=bde29478b5cd484b08643d02529941b0513efe192d4b4adbbdaaa66464f66bc4&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -693,15 +760,8 @@ const AsyncSelect = forwardRef(
setSelectValue(value);
}
} else {
- const token = tokenSeparators.find(token =>
pastedText.includes(token));
- const array = token
- ? uniq(
- pastedText
- .split(token)
- .map(s => s.trim())
- .filter(Boolean),
- )
- : [pastedText.trim()].filter(Boolean);
+ e.preventDefault();
+ const array = uniq(splitWithQuoteEscaping(pastedText,
tokenSeparators));
const values = (
Review Comment:
**Suggestion:** `splitWithQuoteEscaping` only applies one separator, but
this call passes the full `tokenSeparators` list; pasted text containing mixed
delimiters (for example comma plus newline/tab) will be split incorrectly into
merged tokens. Tokenize across all configured separators with quote awareness
instead of using a single-separator split. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Multi-value paste in AsyncSelect misparses CSV-style text.
⚠️ Newline-separated quoted values merge into single incorrect token.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the dashboard access modal and use the owners AsyncSelect at
`src/dashboard/components/PropertiesModal/sections/AccessSection.tsx:11-25`,
which is
configured with `mode="multiple"` and `showSearch`, and uses the shared
AsyncSelect
implementation from `AsyncSelect.tsx`.
2. In that multi-select AsyncSelect, paste CSV-like text where each value
may contain
commas and rows are separated by newlines, e.g. `"Australia,
Austria"\n"Canada, US"`, into
the input; this invokes the `onPaste` handler defined at
`AsyncSelect.tsx:118-135`, where
the multi-select branch (`!isSingleMode`) prevents default paste and
computes `const array
= uniq(splitWithQuoteEscaping(pastedText, tokenSeparators));`.
3. Inspect `splitWithQuoteEscaping` in
`superset-frontend/packages/superset-ui-core/src/components/Select/utils.tsx:67-99`,
which
takes a `separators: string[]` but chooses only the first separator present
via `const
separator = separators.find(sep => text.includes(sep));` and then splits
exclusively on
that one token; the shared `TOKEN_SEPARATORS` are `[' ,', '\r\n', '\n',
'\t', ';']` as
defined in `constants.ts:26`, so any pasted text containing both commas and
newlines will
always select `','` as the separator and ignore `\n` and other delimiters.
4. As a result, the owners AsyncSelect will split the sample text by commas
only
(preserving quoted commas but ignoring newlines), producing merged tokens
like
`"Australia, Austria"` and `"Canada\nUS"` instead of separate
newline-delimited values;
the filter sees fewer tokens than expected and cannot generate one chip per
row, breaking
the documented behavior for pasting multi-value lists with quotes and mixed
delimiters.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a3683b935ceb4561ab68a897e1988e8d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a3683b935ceb4561ab68a897e1988e8d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx
**Line:** 765:765
**Comment:**
*Logic Error: `splitWithQuoteEscaping` only applies one separator, but
this call passes the full `tokenSeparators` list; pasted text containing mixed
delimiters (for example comma plus newline/tab) will be split incorrectly into
merged tokens. Tokenize across all configured separators with quote awareness
instead of using a single-separator split.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39068&comment_hash=04ed3d859c03aad99f48dd76d0ce411cb2f6699b7249e7fa488b2ff6ffcb74cc&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39068&comment_hash=04ed3d859c03aad99f48dd76d0ce411cb2f6699b7249e7fa488b2ff6ffcb74cc&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -767,7 +827,7 @@ const AsyncSelect = forwardRef(
// surface, but the underlying input accepts it and we rely on that.
onPaste={onPaste}
onPopupScroll={handlePagination}
- onSearch={showSearch ? handleOnSearch : undefined}
+ onSearch={showSearch ? onSearchChange : undefined}
Review Comment:
**Suggestion:** The search handler is disabled when `showSearch` is false,
but search is still force-enabled when new options are allowed; with controlled
`searchValue`, typing will not update input state in that configuration. Use
the same condition for `onSearch` as for `showSearch` so searchable mode always
has an input-change handler. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Future free-form AsyncSelect configs may ignore keystrokes.
⚠️ Search input can appear but never update internal state.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect `AsyncSelect` props and wiring in
`superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:77-115`,
where `allowNewOptions`, `showSearch` and `onSearch` are accepted, and the
internal
`inputValue` state is bound to the select via `searchValue={inputValue}` at
`AsyncSelect.tsx:204-205`.
2. Observe that the underlying antd Select is configured with
`showSearch={allowNewOptions
? true : showSearch}` at `AsyncSelect.tsx:204`, forcing the search input
visible whenever
`allowNewOptions` is true, even if the caller passes `showSearch={false}`.
3. At `AsyncSelect.tsx:193`, the search handler is wired as
`onSearch={showSearch ?
onSearchChange : undefined}`, using the raw `showSearch` prop rather than
the effective
`showSearch` value passed to the Select; therefore, in any component that
sets
`allowNewOptions={true}` and `showSearch={false}` (for example, by updating
the existing
usage in `src/explore/components/SaveModal.tsx:14-29` that currently passes
`allowNewOptions` without `showSearch`), the input will be visible but
`onSearch` will be
`undefined`.
4. In that configuration, typing into the visible search box will not update
`inputValue`
or trigger `runSearchLogic` (which depends on `onSearchChange` at
`AsyncSelect.tsx:153-172`), so free-form entry and search callbacks break
even though the
input is shown, demonstrating a real API mismatch between the `showSearch`
and `onSearch`
wiring.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9084d0ff7ce94111a22868b9479279d6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9084d0ff7ce94111a22868b9479279d6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx
**Line:** 830:830
**Comment:**
*Api Mismatch: The search handler is disabled when `showSearch` is
false, but search is still force-enabled when new options are allowed; with
controlled `searchValue`, typing will not update input state in that
configuration. Use the same condition for `onSearch` as for `showSearch` so
searchable mode always has an input-change handler.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39068&comment_hash=df91934eedf2793ef1f915b3868857e77a8c5273b4489d358df99f71186b1c0c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39068&comment_hash=df91934eedf2793ef1f915b3868857e77a8c5273b4489d358df99f71186b1c0c&reaction=dislike'>👎</a>
--
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]
