codeant-ai-for-open-source[bot] commented on code in PR #39068:
URL: https://github.com/apache/superset/pull/39068#discussion_r3488701973
##########
superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx:
##########
@@ -332,6 +342,9 @@ const Select = forwardRef(
}
return previousState;
});
+ setInputValue('');
+ setIsSearching(false);
+ setVisibleOptions(fullSelectOptions);
Review Comment:
**Suggestion:** The new unconditional search reset in multi-select clears
the typed query even when `autoClearSearchValue` is disabled, which breaks the
component contract and makes incremental selection workflows lose the user’s
search text. Keep this reset inside the `autoClearSearchValue` branch (or
otherwise preserve input when that prop is false). [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Multi-select Select cannot honor autoClearSearchValue=false setting.
- ⚠️ Incremental search selections lose query text unexpectedly.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The Select wrapper defines an `autoClearSearchValue` prop with default
`true` in
`Select.tsx:90-41`, and exposes multi-select mode via `mode='multiple'` (see
`Select.tsx`
hunk with `autoClearSearchValue = true` at line 100 and `mode = 'single'` at
line 19 in
the diff).
2. Any consumer can use multi-select with `autoClearSearchValue={false}` by
passing these
props to the exported `Select` component (for example, the native filter
control at
`superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:603-615`
already
uses `mode={multiSelect ? 'multiple' : 'single'}` and would only need to set
`autoClearSearchValue={false}` to exercise this path).
3. When a user types into the multi-select search box, the text is stored in
`inputValue`
and drives filtering via `onSearchChange` and `runSearchLogic` in
`Select.tsx:492-512`,
with `inputValue` bound to the Antd Select `searchValue` prop (see
`Select.tsx` bottom
hunk where `searchValue={inputValue}` and `onSearch={onSearchChange}` are
passed).
4. After the user selects an option in multi-select mode, `handleOnSelect` in
`Select.tsx:166-207` executes the multi-select branch (lines 179-195) and
unconditionally
calls `setInputValue(''); setIsSearching(false);
setVisibleOptions(fullSelectOptions);` at
diff lines 345-347, clearing the search state before reaching the `if
(autoClearSearchValue)` guard at lines 350-353, so the search input is
cleared even when
`autoClearSearchValue` was explicitly set to `false`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9e4b73d46c89475c87e0ddf9ee760040&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=9e4b73d46c89475c87e0ddf9ee760040&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/Select.tsx
**Line:** 345:347
**Comment:**
*Api Mismatch: The new unconditional search reset in multi-select
clears the typed query even when `autoClearSearchValue` is disabled, which
breaks the component contract and makes incremental selection workflows lose
the user’s search text. Keep this reset inside the `autoClearSearchValue`
branch (or otherwise preserve input when that prop is false).
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=9da7192e7c997cb34be6fee07ef64486a4b755888acbcf6a72ad522b7a6a067d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39068&comment_hash=9da7192e7c997cb34be6fee07ef64486a4b755888acbcf6a72ad522b7a6a067d&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-ui-core/src/components/Select/utils.tsx:
##########
@@ -250,3 +260,57 @@ export const mapOptions = (values: SelectOptionsType):
Record<string, any>[] =>
key: opt.value,
...opt,
}));
+
+// Splits text by separators, preserving commas inside double quotes.
+// TODO(antd): drop once https://github.com/ant-design/ant-design/issues/57820
ships.
+export function splitWithQuoteEscaping(
+ text: string,
+ separators: string[],
+): string[] {
+ const separator = separators.find(sep => text.includes(sep));
+ if (!separator) {
+ return [text];
+ }
+
+ const results: string[] = [];
+ let current = '';
+ let inQuotes = false;
+
+ for (let i = 0; i < text.length; i += 1) {
+ const char = text[i];
+
+ if (char === '"') {
+ inQuotes = !inQuotes;
+ } else if (!inQuotes && text.startsWith(separator, i)) {
+ results.push(current.trim());
+ current = '';
+ i += separator.length - 1;
+ } else {
+ current += char;
+ }
+ }
+
Review Comment:
**Suggestion:** The tokenizer only picks a single separator from the list
and then ignores all others, so mixed-separator input (for example commas plus
semicolons/newlines) is parsed incorrectly and no longer matches previous
tokenization behavior. Split logic should honor every configured separator
(while still respecting quoted regions), not just the first matching one.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Native filter multi-select paste mis-parses mixed separators.
- ⚠️ Some pasted values become merged, breaking expected filter values.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The default token separators are defined as `[',', '\r\n', '\n', '\t',
';']` in
`superset-frontend/packages/superset-ui-core/src/components/Select/constants.ts:26`
and
used as the default `tokenSeparators` prop for the Select wrapper
(`Select.tsx`
destructuring at line 123 in the diff).
2. The native filter Select control
(`superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:603-615`)
uses
the wrapped `Select` in multi-select mode (`mode={multiSelect ? 'multiple' :
'single'}`)
and enables paste tokenization via `allowNewOptionsOnPaste={multiSelect &&
searchAllOptions}`, which drives the `onPaste` handler in
`Select.tsx:728-79`.
3. In multi-select paste, `onPaste` in `Select.tsx:28-79` calls `const array
=
uniq(splitWithQuoteEscaping(pastedText, tokenSeparators));` (line 37) so the
tokenizer in
`utils.tsx` receives the full separators array `[',', '\r\n', '\n', '\t',
';']`.
4. When a user pastes a string containing multiple separator types, e.g.
`"A,B\nC"`,
`splitWithQuoteEscaping` in
`superset-frontend/packages/superset-ui-core/src/components/Select/utils.tsx:266-292`
executes `const separator = separators.find(sep => text.includes(sep));` at
line 270,
picks only the first matching separator (`,`), and then, inside the loop at
lines 279-292,
splits only on that separator while treating `\n` and `;` as regular
characters, so the
result becomes `['A', 'B\nC']` instead of `['A', 'B', 'C']`, causing
mixed-separator input
to be mis-tokenized compared to the previous behavior where all configured
separators were
honored.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cc110b181bb44a5bac1e0145933b3c8d&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=cc110b181bb44a5bac1e0145933b3c8d&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/utils.tsx
**Line:** 270:292
**Comment:**
*Logic Error: The tokenizer only picks a single separator from the list
and then ignores all others, so mixed-separator input (for example commas plus
semicolons/newlines) is parsed incorrectly and no longer matches previous
tokenization behavior. Split logic should honor every configured separator
(while still respecting quoted regions), not just the first matching one.
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=d2b336cd557f290591cb11c5e09962dea38fdce62bee54a19458b973f0925afe&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39068&comment_hash=d2b336cd557f290591cb11c5e09962dea38fdce62bee54a19458b973f0925afe&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -721,14 +780,15 @@ const AsyncSelect = forwardRef(
// @ts-expect-error
onPaste={onPaste}
onPopupScroll={handlePagination}
- onSearch={showSearch ? handleOnSearch : undefined}
+ onSearch={showSearch ? onSearchChange : undefined}
onSelect={handleOnSelect}
onClear={handleClear}
options={fullSelectOptions}
optionRender={option => <Space>{option.label ||
option.value}</Space>}
placeholder={placeholder}
showSearch={allowNewOptions ? true : showSearch}
Review Comment:
**Suggestion:** The search handler is gated by `showSearch`, but the
component can still force search UI on via `allowNewOptions`; in that case the
input is rendered as searchable but `onSearch` is undefined, so typing won’t
update `searchValue` or trigger tokenization/new-option logic. Use the same
effective search flag for both props so they stay consistent. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Creatable AsyncSelect can render search UX with dead input.
- ⚠️ Users unable to type custom dashboard or filter values.
- ⚠️ Confusing behavior if showSearch disabled while creatable mode enabled.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. AsyncSelect props are defined so callers can pass both `allowNewOptions`
(creatable
mode) and `showSearch` (search UI flag), as seen in `BaseSelectProps` at
`superset-frontend/packages/superset-ui-core/src/components/Select/types.ts:20-31,70-80`.
2. In the AsyncSelect render, the Select’s search UI is controlled with
`showSearch={allowNewOptions ? true : showSearch}` at `AsyncSelect.tsx:789`,
meaning any
caller that sets `allowNewOptions={true}` and `showSearch={false}` will
still render a
search input because the effective flag becomes `true`.
3. However, the `onSearch` handler is wired as `onSearch={showSearch ?
onSearchChange :
undefined}` at `AsyncSelect.tsx:783`, using the raw `showSearch` prop
instead of the
effective flag; under the configuration above, `onSearch` will be
`undefined` while the
Select is treated as controlled via `searchValue={inputValue}` at
`AsyncSelect.tsx:790-791`.
4. To reproduce with existing code, update the creatable AsyncSelect in
`superset-frontend/src/explore/components/SaveModal.tsx:793-799` to pass
`showSearch={false}` alongside `allowNewOptions`. Reload the “Save chart”
modal, focus the
“Add to dashboard” AsyncSelect, and type into the search box: the text input
will not
update and no tokenization/new-option logic in `onSearchChange`
(`AsyncSelect.tsx:115-134`) will run, leaving a visible search box that does
not process
user input.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b339d1e2675642a588cd3cdf69222e72&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=b339d1e2675642a588cd3cdf69222e72&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:** 783:789
**Comment:**
*Api Mismatch: The search handler is gated by `showSearch`, but the
component can still force search UI on via `allowNewOptions`; in that case the
input is rendered as searchable but `onSearch` is undefined, so typing won’t
update `searchValue` or trigger tokenization/new-option logic. Use the same
effective search flag for both props so they stay consistent.
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=ac11d9154d6ac139dd5fe678560bcbc41a79d6aea883529849c3a923f387f283&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39068&comment_hash=ac11d9154d6ac139dd5fe678560bcbc41a79d6aea883529849c3a923f387f283&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]
