EnxDev commented on PR #39068:
URL: https://github.com/apache/superset/pull/39068#issuecomment-4901167669
## EnxDev's Review Agent — apache/superset#39068 · HEAD 86106e4
**comment** — Approach is sound and already approved as a temporary
workaround (pending the antd upstream `tokenEscapeChar`). A narrow paste
regression, a state-mutation bug, and untested tokenization paths remain.
Context: fixes comma-in-value filtering by removing `,` from antd's
`tokenSeparators` and reimplementing quote-aware comma tokenization manually in
`Select`/`AsyncSelect`. Already `APPROVED`; @rusackas is rebasing to clear CI.
Findings below are re-verified against HEAD 86106e4.
### 🔴 Functional
- **`Select.tsx:787` / `AsyncSelect.tsx:763`** · _Medium_ — Multi-select
paste now calls `e.preventDefault()` unconditionally, then discards the paste
if it maps to zero selectable values (`allowNewOptions=false` + no matching
option — e.g. the dashboard Owners `AsyncSelect`). Old code didn't
`preventDefault`, so unmatched text landed in the search box and the user could
search it; now the paste is silently dropped. Fix: only `preventDefault()` when
≥1 token is consumed, otherwise fall through / push the text into `inputValue`.
**regression test:** paste an unmatched string into a multi-select with
`allowNewOptions=false` → assert the search input keeps it (or a chip appears),
not silently dropped.
- **`AsyncSelect.tsx:520`** · _Low_ — `selectTokenizedValues` always passes
`handleOnSelect` a `{label, value}` object, ignoring `labelInValue`, whereas
the sync `Select` (`Select.tsx:502`) passes the raw value when `!labelInValue`.
For a non-`labelInValue` AsyncSelect, typed comma tokens enter `selectValue` as
objects, so `onChange` emits a mixed array. Needs runtime confirmation. Fix:
mirror `Select.tsx`'s `labelInValue` branch. **regression test:** tokenize a
comma value in a non-`labelInValue` multi `AsyncSelect` → assert `onChange`
receives raw values, not `{label,value}` objects.
### 🟡 Should-fix
- **`AsyncSelect.tsx:282`** — `setSelectOptions(prev =>
prev.sort(sortComparatorForNoSearch))` sorts in place and returns the same
reference, so React's `Object.is` bail-out skips this state update; the re-sort
only renders because the sibling `setInputValue('')` triggers one — and it
won't when `inputValue` is already empty. Use `[...prev].sort(...)`. (Raised by
@jesperct; still open at HEAD.)
- **Test coverage** — the core tokenization paths (`onSearchChange`,
`selectTokenizedValues`) have no component tests; only the pure helpers in
`utils.test.ts` are covered. Codecov: patch 60.74%, 53 lines uncovered
(`Select.tsx` 42%, `AsyncSelect.tsx` 51%). The feature's actual behavior is
unguarded. Add RTL tests: typing `"A, B",` → one chip `A, B`; pasting `"A,
B",C` → two chips `A, B` and `C`.
### 🔵 Nits
- **`utils.tsx` `splitWithQuoteEscaping`** — once any separator is present,
every `"` is stripped, so values that legitimately contain a double-quote
character can't be entered (CSV-style doubled `""` unsupported). Acceptable for
the stated scope; worth a comment noting the limitation so it isn't mistaken
for a bug later.
### 🙌 Praise
- **`utils.test.ts`** — thorough edge-case table for the three helpers
(mismatched/unclosed quotes, adjacent/edge separators, empty input). Locks in
the intended semantics.
<!-- enxdev-review-agent:86106e4 -->
_Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 86106e4._
--
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]
