Repository navigation
fix(select): comprehensive hardening for option deduplication and new option handling - #44894
Conversation
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if ( | ||
| previousOption.isNewOption && | ||
| (hasOption(previousOption.label as V, data, true) || | ||
| hasOption(previousOption.value as V, data, true)) | ||
| ) { |
There was a problem hiding this comment.
Suggestion: hasOption checks only top-level entries, so matching children inside grouped options are missed and the optimistic entry remains beside the fetched option.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Logic error
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx
**Line:** 388:392
**Comment:**
*Logic Error: `hasOption` checks only top-level entries, so matching children inside grouped `options` are missed and the optimistic entry remains beside the fetched option.
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 fixThere was a problem hiding this comment.
Resolved in commit e7c9b0e0de: updated getOption in Select/utils.tsx to recursively inspect child options when encountering option groups (isObject(x) && Array.isArray(x.options)), ensuring that matching child options within grouped structures are identified and optimistic duplicates are dropped properly.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of af604c5.
isRedundantNewOption now checks each group's options children for a matching label before deciding the optimistic option is not redundant.
If that's not right, unresolve this thread and CodeAnt will leave it open.
|
The flagged issue is correct. The current Would you like me to check the rest of the comments on this PR and implement fixes for them as well? superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #cfc715
Actionable Suggestions - 1
-
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx - 1
- duplicated new-option filter · Line 388-394
Additional Suggestions - 1
-
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx - 1
-
loose-equality label match · Line 390-391`hasOption(label, data, true)` matches both value and label using loose equality (`isEqual` returns `actualA == actualB`). A new option typed as a numeric string (e.g. '42') is dropped when the server returns an option with value `42` but a different label, even though no label match exists — contradicting the 'match for the label' intent.
-
Review Details
-
Files reviewed - 2 · Commit Range:
20be903..20be903- superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.test.tsx
- superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| if ( | ||
| previousOption.isNewOption && | ||
| (hasOption(previousOption.label as V, data, true) || | ||
| hasOption(previousOption.value as V, data, true)) | ||
| ) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The new-option-dropping predicate is duplicated: mergeData (lines 388-394) and the page === 0 preservedNew filter (lines 493-497) both implement 'drop isNewOption when hasOption(label/value, data, true)'. Extract one helper used by both paths to avoid divergence.
Code Review Run #cfc715
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Resolved in commit 7019744718: extracted the shared predicate isRedundantNewOption in AsyncSelect.tsx and unified the duplicate check across mergeData and the initial page search response filter.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44894 +/- ##
=======================================
Coverage 84.70% 84.70%
=======================================
Files 3069 3069
Lines 205597 205657 +60
Branches 48327 48366 +39
=======================================
+ Hits 174148 174204 +56
- Misses 28157 28161 +4
Partials 3292 3292
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
20be903 to
16d248c
Compare
FrancescoCastaldi
changed the title
|
Hi team, Here is a summary of the hardening and fixes included in this PR for #44873:
All quality checks ( |
Code Review Agent Run #32516Actionable Suggestions - 0Additional Suggestions - 3
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
github-actions
Bot
added
the
requires:rebase
|
github-actions
Bot
removed
the
requires:rebase
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Pushed commits
All TypeScript checks and unit tests pass cleanly. Whenever you have a moment, please take a look. @coderabbitai review |
…h in AsyncSelect test (apache#44894)
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
CI: the 2 red playwright-tests-gaq lanes aren't yours - they need superset_test_config_gaq.py, added to master 2026-10-06 after this branch's base, and the job checks out your head. Merge master to clear; they're also continue-on-error. Real blocker is lint-frontend, which fails at tsc, not lint.
apache#44894) 1. Harmonize SelectValue union with LabeledValue in types.ts to preserve consumer contracts and resolve TS2322. 2. Type story args with Omit<AsyncSelectProps, 'ref'> in AsyncSelect.stories.tsx to prevent ref spread mismatch. 3. Refine isEqual equality invariant in utils.tsx to distinguish empty string and whitespace while preserving whitespace trimming and numeric zero equality. 4. Add regression unit tests in utils.test.ts for whitespace boundary conditions and numeric zero.
|
Hi @aminghadersohi, I have pushed commit 844d017 addressing all three review comments on this PR:
Validation and test evidence:
The branch fix/asyncselect-drop-duplicate-new-option is updated and ready for your re-review. Best regards, |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Verified here: npm run type RC=0 (0 errors), lint-frontend green, Select suites 293/293. Only reds are the 2 playwright-tests-gaq lanes - still the missing superset_test_config_gaq.py from the stale base, not this code, not required. Trimmed-label equality (' Sales '=='Sales') stays open by design.
|
@aminghadersohi Thank you for the detailed verification! Glad that Everything is in place and verified green on frontend lint, types, and Jest test suites. Ready for final review and merge when you have a moment, thank you! |
…h in AsyncSelect test (apache#44894)
apache#44894) 1. Harmonize SelectValue union with LabeledValue in types.ts to preserve consumer contracts and resolve TS2322. 2. Type story args with Omit<AsyncSelectProps, 'ref'> in AsyncSelect.stories.tsx to prevent ref spread mismatch. 3. Refine isEqual equality invariant in utils.tsx to distinguish empty string and whitespace while preserving whitespace trimming and numeric zero equality. 4. Add regression unit tests in utils.test.ts for whitespace boundary conditions and numeric zero.
844d017 to
4ff49e2
Compare
|
Hi @aminghadersohi, The branch has been rebased on the latest master to pick up superset_test_config_gaq.py for the Playwright GAQ test suite. All merge conflicts in utils.tsx, AsyncSelect.tsx, and test suites have been resolved, and the PR is cleanly mergeable. |
…nents (apache#44894) - Export isNewOption type guard helper in utils.tsx - Import isNewOption in Select.tsx, AsyncSelect.tsx, and utils.test.ts - Resolves ReferenceError in SelectFilterPlugin and unit tests Signed-off-by: Francesco Castaldi <info@francescocastaldi.it>
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Two missing StyledNewOptionBadge imports crash any dropdown that shows a new option, and the narrowed isNewOption guard breaks type-checking; fixes inline.
github-actions
Bot
added
the
requires:rebase
…te, and search clearing
…a and search responses
…h in AsyncSelect test (apache#44894)
apache#44894) 1. Harmonize SelectValue union with LabeledValue in types.ts to preserve consumer contracts and resolve TS2322. 2. Type story args with Omit<AsyncSelectProps, 'ref'> in AsyncSelect.stories.tsx to prevent ref spread mismatch. 3. Refine isEqual equality invariant in utils.tsx to distinguish empty string and whitespace while preserving whitespace trimming and numeric zero equality. 4. Add regression unit tests in utils.test.ts for whitespace boundary conditions and numeric zero.
…nents (apache#44894) - Export isNewOption type guard helper in utils.tsx - Import isNewOption in Select.tsx, AsyncSelect.tsx, and utils.test.ts - Resolves ReferenceError in SelectFilterPlugin and unit tests Signed-off-by: Francesco Castaldi <info@francescocastaldi.it>
…pache#44894) Signed-off-by: Francesco Castaldi <info@francescocastaldi.it>
d98f30c to
5f33ef3
Compare
Summary of Changes addressing Review FeedbackI have rebased onto current
cc @aminghadersohi for re-review. |
github-actions
Bot
removed
the
requires:rebase
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
The new label check calls lodash isEqual, so it fails type-checking and never drops the duplicate new option; fixes inline.
…p preservation, and trim key (apache#44894) Signed-off-by: Francesco Castaldi <info@francescocastaldi.it>
Review Follow-Up & CI RemediationAll review comments have been addressed in commit
@aminghadersohi, the PR has been updated and is ready for your re-review whenever you have a moment. Thank you! |
SUMMARY
Comprehensive hardening suite for
SelectandAsyncSelectoption handling, custom option deduplication, and visual indicators (resolving #44873).In
AsyncSelectwithallowNewOptions={true}(such as the dashboard picker in Explore's "Save chart" modal),handleOnSearchintroduces an optimisticisNewOptionentry ({ label: typed, value: typed, isNewOption: true }) whenever the typed title is not yet present in currently loaded options (e.g., while waiting for the asynchronous search request to resolve or when existing dashboards span subsequent pages).Previously, both
mergeDataand thepage === 0search merge infetchPagededuplicated incoming server options strictly byvalue(!dataValues.has(opt.value)). Because the optimistic entry'svalueis the typed label string whereas backend options have numeric IDs, the optimistic entry was preserved alongside the server option even when titles matched. As filed in #44873 (and encountered while fixing Playwright tests in #44856), this displayed duplicate rows with identical labels; selecting the optimistic row silently created an empty dashboard with a duplicate title instead of associating the chart with the existing dashboard.Key Changes:
AsyncSelect): InAsyncSelect, filter outisNewOptionentries in bothmergeDataandfetchPage(page 0) whenever the server response contains an option with a matching label or value viahasOption.utils.tsx): HardenedisEqual,getOption, andhasOptionfor bidirectional string/number and trimmed comparisons; addedisNewOptionhelper and unit tests.Select: Aligned synchronousSelect.tsxto check against both loaded and initial options before generating temporary custom entries.StyledNewOptionBadgerendering a subtle[new]tag inoptionRenderfor custom options in both Select and AsyncSelect.clear, and match both label and value during removal ondeselect.CustomLabeledValuewith explicitisNewOption?: booleantyping.AsyncSelect.test.tsxfor Save chart modal can create a duplicate dashboard when the typed title matches an existing one #44873, edge-case tests for paste deduplication and clear cleanup, and interactive Storybook stories (WithAllowNewOptions).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
[new]tag in the dropdown.TESTING INSTRUCTIONS
npm test superset-frontend/packages/superset-ui-core/src/components/Select/ADDITIONAL INFORMATION