kgabryje commented on PR #40984:
URL: https://github.com/apache/superset/pull/40984#issuecomment-4767014758
Thanks for the fix — the three-part approach (trim pasted tokens, surface
typed values as creatable options for multi-select, and retain created values
as chips) is solid, and reusing the existing
`allowNewOptions={!searchAllOptions && creatable !== false}` gating keeps the
new logic consistent. One thing I'd tighten before merge:
### Type coercion in the created-value dedup
In the new block in the `uniqueOptions` memo
(`superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx`):
```js
if (creatable !== false && filterState.value) {
const existing = new Set(allOptions);
ensureIsArray(filterState.value)
.filter(v => v !== null && v !== undefined && !existing.has(v))
.forEach(v => {
baseOptions.push({ label: String(v), value: String(v), isNewOption:
true });
existing.add(v);
});
}
```
The dedup uses a strict `Set.has(v)` against the raw DB values, but then
appends the value as `String(v)`. For numeric columns that's inconsistent with
the `Select`'s own loose matching (`hasOption`) and can bite in two ways:
1. **Duplicate option** — if the dataset has `10107` (number) but
`filterState.value` holds `"10107"` (string), `existing.has("10107")` is
`false`, so the same logical value gets re-added as a separate option.
2. **Type flip** — a value that should resolve to the numeric option `10107`
instead becomes an option with `value: "10107"`, so the emitted filter (and
select‑all) can end up carrying both the string and numeric variants.
Suggestion: dedup with the same loose helper this file already uses for the
typed-search case, and preserve the original value type:
```js
ensureIsArray(filterState.value)
.filter(v => v != null && !hasOption(v, baseOptions, true))
.forEach(v => {
baseOptions.push({ label: String(v), value: v, isNewOption: true });
});
```
### Minor / non-blocking
It'd be good to add plugin-level tests for the new behaviors — multi-select
typed creatable option display, `creatable: false`, `searchAllOptions: true`,
created values from `filterState.value` rendering as chips, and the numeric
string-vs-number case — plus an `AsyncSelect` trimming test to mirror the new
`Select.test.tsx` one.
Otherwise looks good 👍
--
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]
