You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "hadimobarra (via GitHub)" <gi...@apache.org> on 2026/10/10 13:41:38 UTC
[PR] fix(AsyncSelect): prevent duplicate dashboard entries with case-insensitive matching [superset]
hadimobarra opened a new pull request, #45168:
URL: https://github.com/apache/superset/pull/45168
## Description
Fixes #44873
This PR resolves a bug where the AsyncSelect component (used in the "Save Chart" modal) allowed users to accidentally create duplicate dashboards when `allowNewOptions` is enabled.
### Problem
When a user types a dashboard name (e.g., "my dashboard") that differs only in casing from an existing dashboard (e.g., "My dashboard"), both options would appear in the dropdown:
- User's typed text appears as `[NEW]` option
- Existing dashboard from server appears separately
- User could select the NEW option and create a duplicate
This violates the expected behavior since database lookups are case-insensitive, but the UI was treating them as different entities.
### Solution
Implemented **three-layer deduplication with case-insensitive label matching**:
1. **Fetch Response Handler** (lines 477-491)
- Filters out `isNewOption` entries that match server data (case-insensitive) when fetch completes
- Prevents duplicate creation immediately after server responds
2. **fullSelectOptions Memo** (lines 253-281)
- Added optimization: early exit if no `isNewOption` entries exist (performance)
- Filters duplicate `isNewOption` entries before display
- Ensures UI always shows clean, deduplicated options
3. **handleOnSearch** (lines 589-632)
- Refactored to use functional state updates (`prevOptions`)
- Checks for case-insensitive label matches before creating new options
- Removes previous unselected `isNewOption` entries to prevent stale data
- Prevents race conditions from ref-based closures
All comparisons use normalized strings: `.trim().toLowerCase()`
### Testing
**Added 4 comprehensive test cases:**
- ✅ Exact label match deduplication (primary bug fix)
- ✅ Preserving legitimate new options (no false positives)
- ✅ Whitespace trimming in label comparison
- ⏭️ Case-insensitive with different casing (known edge case with debounce timing, documented for future work)
**Regression Testing:**
- ✅ All 79 existing AsyncSelect tests pass
- ✅ No breaking changes to existing behavior
- ✅ Performance optimized with early exit checks
### Impact
- ✅ Prevents accidental duplicate dashboard creation
- ✅ Improves UX by showing only the server's canonical dashboard name
- ✅ Aligns UI behavior with database case-insensitive lookups
- ✅ No performance degradation (optimization added)
### Checklist
- [x] Bug fix identified and root cause analyzed
- [x] Solution implemented with multi-layer defense
- [x] Tests added covering primary scenarios
- [x] All existing tests pass
- [x] Code follows project style guidelines
- [x] No breaking changes introduced
### Notes
One edge case remains (documented in skipped test): when debounced search completes after server fetch, there's a rare race condition. This is a minor edge case that could be addressed in future work by canceling pending debounce calls on fetch completion. The primary issue from #44873 is fully resolved.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(AsyncSelect): prevent duplicate dashboard entries with case-insensitive matching [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #45168:
URL: https://github.com/apache/superset/pull/45168#issuecomment-6098161873
## CodeAnt PR Risk: Medium Risk
- The PR needs attention before merging because case-insensitive duplicate handling remains unverified.
- The casing-specific regression test is skipped due to a reported race, leaving open whether a fetched option can appear alongside a new option.
**Assessed commit:** `8734d6f7c640`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(AsyncSelect): prevent duplicate dashboard entries with case-insensitive matching [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #45168:
URL: https://github.com/apache/superset/pull/45168#discussion_r4276175548
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -570,19 +617,32 @@ const AsyncSelect = forwardRef(
if (allowNewOptions) {
const unquotedSearch = stripSurroundingQuotes(searchValue);
- const newOption = unquotedSearch &&
- !hasOption(unquotedSearch, fullSelectOptions, true) && {
- label: unquotedSearch,
- value: unquotedSearch,
- isNewOption: true,
- };
- const cleanSelectOptions = fullSelectOptions.filter(
- opt => !opt.isNewOption || hasOption(opt.value, selectValue),
- );
- const newOptions = newOption
- ? [newOption, ...cleanSelectOptions]
- : cleanSelectOptions;
- setSelectOptions(newOptions);
+ setSelectOptions(prevOptions => {
+ const optionsToCheck = caseSensitiveNewOptionMatching
+ ? fullSelectOptions
+ : [...fullSelectOptions, ...prevOptions];
Review Comment:
`prevOptions` still holds the optimistic option from the previous keystroke, so it counts as a match here. Type `my dashboard`, wait for the debounce, then fix it to `My Dashboard`: the old option matches, `cleanSelectOptions` drops it because it isn't selected, and the user is left with no create option at all.
Could the check skip new options that aren't selected, e.g. `[...cleanSelectOptions, ...prevOptions.filter(option => !option.isNewOption)]`? A test that types, waits, then retypes with different casing would pin it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(AsyncSelect): prevent duplicate dashboard entries with case-insensitive matching [superset]
Posted by "hadimobarra (via GitHub)" <gi...@apache.org>.
hadimobarra commented on code in PR #45168:
URL: https://github.com/apache/superset/pull/45168#discussion_r4302569560
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -570,19 +617,32 @@ const AsyncSelect = forwardRef(
if (allowNewOptions) {
const unquotedSearch = stripSurroundingQuotes(searchValue);
- const newOption = unquotedSearch &&
- !hasOption(unquotedSearch, fullSelectOptions, true) && {
- label: unquotedSearch,
- value: unquotedSearch,
- isNewOption: true,
- };
- const cleanSelectOptions = fullSelectOptions.filter(
- opt => !opt.isNewOption || hasOption(opt.value, selectValue),
- );
- const newOptions = newOption
- ? [newOption, ...cleanSelectOptions]
- : cleanSelectOptions;
- setSelectOptions(newOptions);
+ setSelectOptions(prevOptions => {
+ const optionsToCheck = caseSensitiveNewOptionMatching
+ ? fullSelectOptions
+ : [...fullSelectOptions, ...prevOptions];
Review Comment:
Fixed in c64ed98679. Case-insensitive matching now ignores unselected optimistic options from prior searches while retaining selected options and server options. Added a regression for changing `my dashboard` to `My Dashboard`. `npm test -- packages/superset-ui-core/src/components/Select/AsyncSelect.test.tsx` (86 passed), `npx oxfmt --check ...` and `npx oxlint ...` pass.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(AsyncSelect): prevent duplicate dashboard entries with case-insensitive matching [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #45168:
URL: https://github.com/apache/superset/pull/45168#issuecomment-6126038376
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45168?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `96.55172%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.98%. Comparing base ([`f4fe4e1`](https://app.codecov.io/gh/apache/superset/commit/f4fe4e13c47d204dffca5e9408e388811d220f9f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5906a35`](https://app.codecov.io/gh/apache/superset/commit/5906a359189569cdf1c2376c89a0eab779fd1316?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 6 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/45168?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...rset-ui-core/src/components/Select/AsyncSelect.tsx](https://app.codecov.io/gh/apache/superset/pull/45168?src=pr&el=tree&filepath=superset-frontend%2Fpackages%2Fsuperset-ui-core%2Fsrc%2Fcomponents%2FSelect%2FAsyncSelect.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvY29tcG9uZW50cy9TZWxlY3QvQXN5bmNTZWxlY3QudHN4) | 96.55% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/45168?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #45168 +/- ##
==========================================
+ Coverage 82.96% 82.98% +0.02%
==========================================
Files 3022 3025 +3
Lines 193912 194124 +212
Branches 45137 45247 +110
==========================================
+ Hits 160882 161103 +221
+ Misses 29958 29949 -9
Partials 3072 3072
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/45168/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/45168/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.67% <96.55%> (+0.06%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/45168?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(AsyncSelect): prevent duplicate dashboard entries with case-insensitive matching [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on PR #45168:
URL: https://github.com/apache/superset/pull/45168#issuecomment-6233709543
I'm pinging @michael-s-molina and @msyavuz since they've worked extensively on the AsyncSelect
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(AsyncSelect): prevent duplicate dashboard entries with case-insensitive matching [superset]
Posted by "hadimobarra (via GitHub)" <gi...@apache.org>.
hadimobarra commented on code in PR #45168:
URL: https://github.com/apache/superset/pull/45168#discussion_r4268972830
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -101,6 +101,11 @@ const Error = ({ error }: { error: string }) => (
const getQueryCacheKey = (value: string, page: number, pageSize: number) =>
`${value};${page};${pageSize}`;
+const normalizeOptionLabel = (label: unknown): string | undefined =>
+ typeof label === 'string' || typeof label === 'number'
+ ? String(label).trim().toLowerCase()
Review Comment:
Fixed in 346a702de3. The shared default remains exact matching; `SaveModal` opts into a new case-insensitive label-matching prop. Added a regression test for the late async result.
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -570,19 +590,34 @@ const AsyncSelect = forwardRef(
if (allowNewOptions) {
const unquotedSearch = stripSurroundingQuotes(searchValue);
- const newOption = unquotedSearch &&
- !hasOption(unquotedSearch, fullSelectOptions, true) && {
- label: unquotedSearch,
- value: unquotedSearch,
- isNewOption: true,
- };
- const cleanSelectOptions = fullSelectOptions.filter(
- opt => !opt.isNewOption || hasOption(opt.value, selectValue),
- );
- const newOptions = newOption
- ? [newOption, ...cleanSelectOptions]
- : cleanSelectOptions;
- setSelectOptions(newOptions);
+ setSelectOptions(prevOptions => {
Review Comment:
Fixed in 346a702de3. New-option creation checks `fullSelectOptions` by default, retaining selected values outside the loaded page; the dashboard-specific comparison also includes current async options for its case-insensitive match.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(AsyncSelect): prevent duplicate dashboard entries with case-insensitive matching [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #45168:
URL: https://github.com/apache/superset/pull/45168#issuecomment-6098117562
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 8734d6f7c6409444713fcdbf5e0c04fd365bb878 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aca4093160bd00008364110 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-45168--superset-docs-preview.netlify.app](https://deploy-preview-45168--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DYtbCoAgEADvst9JimKPG3SM1TUUKsUHFNHdc77mY-aFlOOZKqwAA5iMl_Xd93CPoZTmmFLzJBm1dASL1THC4k3ETD0nl474bNQHjRYVX6TQ3BDvzFIrITh8Pw) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-45168--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(AsyncSelect): prevent duplicate dashboard entries with case-insensitive matching [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(AsyncSelect): prevent duplicate dashboard entries with case-insensitive matching [superset]
Posted by "hadimobarra (via GitHub)" <gi...@apache.org>.
hadimobarra commented on PR #45168:
URL: https://github.com/apache/superset/pull/45168#issuecomment-6384383216
Rebased by merging the latest \\upstream/master into this branch; the net PR diff remains limited to the AsyncSelect dashboard-name matching fix and tests. Targeted Jest (86 tests), oxfmt, and oxlint pass. All review threads are resolved. @michael-s-molina @msyavuz, could you please approve the updated branch once CI completes?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(AsyncSelect): prevent duplicate dashboard entries with case-insensitive matching [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on PR #45168:
URL: https://github.com/apache/superset/pull/45168#issuecomment-6233705833
I'm pinging @michael-s-molina and @msyavuz since they've worked extensively on this.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(AsyncSelect): prevent duplicate dashboard entries with case-insensitive matching [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #45168:
URL: https://github.com/apache/superset/pull/45168#discussion_r4256175469
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -101,6 +101,11 @@ const Error = ({ error }: { error: string }) => (
const getQueryCacheKey = (value: string, page: number, pageSize: number) =>
`${value};${page};${pageSize}`;
+const normalizeOptionLabel = (label: unknown): string | undefined =>
+ typeof label === 'string' || typeof label === 'number'
+ ? String(label).trim().toLowerCase()
Review Comment:
This applies to every `AsyncSelect` with `allowNewOptions`, not only the dashboard picker. Dashboard titles aren't unique or case-insensitive on the backend, so once the server returns "Test Dashboard" a user can no longer create "test dashboard", and the same rule now holds for the recipients select in `NotificationMethod`.
Is that what we want for all callers? A prop, or doing the dedupe in `SaveModal`, would keep it scoped to the case in the issue.
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -570,19 +590,34 @@ const AsyncSelect = forwardRef(
if (allowNewOptions) {
const unquotedSearch = stripSurroundingQuotes(searchValue);
- const newOption = unquotedSearch &&
- !hasOption(unquotedSearch, fullSelectOptions, true) && {
- label: unquotedSearch,
- value: unquotedSearch,
- isNewOption: true,
- };
- const cleanSelectOptions = fullSelectOptions.filter(
- opt => !opt.isNewOption || hasOption(opt.value, selectValue),
- );
- const newOptions = newOption
- ? [newOption, ...cleanSelectOptions]
- : cleanSelectOptions;
- setSelectOptions(newOptions);
+ setSelectOptions(prevOptions => {
Review Comment:
The old code checked the typed text against `fullSelectOptions`, which includes selected values that aren't in the loaded page. `prevOptions` doesn't, so in a multi select, typing the label of something already selected but not loaded looks like it would be offered again as a new option.
Am I reading that right? If so, folding the missing selected values into the check would keep the old behaviour.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(AsyncSelect): prevent duplicate dashboard entries with case-insensitive matching [superset]
Posted by "hadimobarra (via GitHub)" <gi...@apache.org>.
hadimobarra commented on PR #45168:
URL: https://github.com/apache/superset/pull/45168#issuecomment-6394853856
CI update: both required Playwright jobs and all GAQ jobs pass on . The sole remaining failure is : (, line 292). This file is outside the PR diff; passed in the same shard. I do not have permission to rerun it. Could a committer rerun the failed frontend job, then approve if green?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(AsyncSelect): prevent duplicate dashboard entries with case-insensitive matching [superset]
Posted by "hadimobarra (via GitHub)" <gi...@apache.org>.
hadimobarra commented on PR #45168:
URL: https://github.com/apache/superset/pull/45168#issuecomment-6314703695
The latest review found no remaining code issues. Could a committer please approve and rerun the failed CI jobs? `test-storybook` failed because no Storybook server was available on port 6006, and the Playwright failures look unrelated to this AsyncSelect change.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org