fix(select): replace cached options with search results in AsyncSelect#40039
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40039 +/- ##
==========================================
+ Coverage 64.14% 64.15% +0.01%
==========================================
Files 2592 2592
Lines 138841 138875 +34
Branches 32201 32207 +6
==========================================
+ Hits 89064 89101 +37
+ Misses 48245 48242 -3
Partials 1532 1532
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
sadpandajoe
force-pushed
the
fix-asyncselect-search-option-merging
branch
from
0b935a4 to
76cc1d9
Compare
There was a problem hiding this comment.
Pull request overview
Fixes AsyncSelect behavior when server-side searching is enabled and option sets are large, ensuring search results are displayed correctly and filterOption={false} works as intended.
Changes:
- Fix
handleFilterOptionHelperto treatfilterOption === falseas “show all provided options”. - Update
AsyncSelectfetching logic to replace cached options with search results (page 0), restore base options on search clear, and drop stale search responses; add an in-flight counter to keep loading state consistent across races. - Add Jest tests covering search replacement,
filterOption={false}, restore-on-clear, and several race/pagination regressions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| superset-frontend/packages/superset-ui-core/src/components/Select/utils.tsx | Fixes filterOption={false} so options aren’t incorrectly hidden. |
| superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx | Reworks async option handling to replace options on search, restore base options on clear, and guard against stale responses/loading races. |
| superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.test.tsx | Adds regression tests for search replace/restore, filterOption={false}, pagination during search, and race conditions. |
Code Review Agent Run #4b7030Actionable Suggestions - 0Additional Suggestions - 1
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 |
When searching with >100 cached records, server search results were merged with page-0 options instead of replacing them, burying matches at the end. Also fixes filterOption=false falling through to return false (hiding all options) instead of returning true (show all, server-side filtering). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ons-prop change Preserve optimistic isNewOption entries inserted by handleOnSearch when the search fetch resolves, so allowNewOptions users can still pick the value they typed when the server returns no match (regression seen via SaveModal "Add to dashboard"). Also reset initialOptionsRef and wasSearchingRef when the options loader changes, so loader swaps don't briefly restore stale options. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ests Replace wasSearchingRef with usePrevious(inputValue), restructure the fetchPage().then() branching so allValuesLoaded lives in the non-search branch (removes the dead resultData variable), and harden the new tests with disjoint datasets and negative assertions so they would fail against the original merge-on-search bug. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Search responses no longer write to fetchedQueries — caching only the totalCount made re-typing a previously-resolved search term short- circuit the fetch and leave selectOptions stale (e.g. after restore-on- clear had reset to the base list). Search refetches are cheap; only the base accumulator benefits from the totalCount cache. Late responses whose search arg no longer matches inputValueRef are dropped, so a slow base or search fetch cannot pollute the dropdown after the user has moved on. Base pages accumulate in initialOptionsRef independently of selectOptions so restore-on-clear has a complete snapshot even when base pages land during an active search. Also cancel any pending debounced search fetch on clear so it cannot fire after the base list has been restored. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The stale-response early-return at line 354 drops the data but the shared `.finally` block previously still cleared `isLoading` to false. When a user typed 'alpha' (in-flight) then 'beta' (also in-flight), alpha's dropped response flipped the spinner off while beta was still pending. The undebounced `handlePagination` scroll handler reads `!isLoading` and could then fire `fetchPage` against stale `totalCount`. Gate `setIsLoading(false)` on an in-flight fetch counter so the spinner only clears when every dispatched request has settled. Add regression test covering the held-alpha + in-flight-beta race, plus coverage for page>1 results during an active search. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
clearCache() previously only cleared fetchedQueries, leaving initialOptionsRef populated and allValuesLoaded sticky. After calling clearCache(), a subsequent search + clear would restore stale options from initialOptionsRef and the next no-search fetch could short-circuit on allValuesLoaded. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sadpandajoe
force-pushed
the
fix-asyncselect-search-option-merging
branch
from
bd11723 to
fe12683
Compare
|
Concern: sortComparatorForNoSearch in the accumulation. The accumulated base options are sorted on every page arrival. For large datasets this is O(n log n) on each page, where n grows. This was not in the original code and could become noticeably slow for very large cached option sets. Consider accumulating unsorted and sorting only once when restoring. Concern: sort(sortComparatorForNoSearch) on search results. The server already returned results ranked by relevance. Sorting client-side with sortComparatorForNoSearch could reorder them and push the most relevant match down. Is this intentional? The existing mergeData also sorts, but for search results, server ranking is typically preferred. This deserves a comment or consideration. Concern: page > 0 during search (the else branch, line 401+). The "append normally" path for search pagination pages isn't explicitly shown in the diff but falls through to the original mergeData path. Verify that paginating through search results (scrolling the dropdown) still works correctly. |
The initialOptionsRef accumulator was sorted on every base-page fetch, even though it's only consumed at restore-on-clear, which sorts a copy at consumption time. Combined with the mergeData sort on the live selectOptions path, this doubled per-page sort work and grew as more pages were cached. Dedupe-and-append only; defer the sort to consumption. Also adds an explicit return type to the isSpinnerVisible test helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
LGTM, but I agree with rusackas concern related to the client side sorting over the server ranked results. I'd suggest adding a comment if intended.
Both of these have the same current behavior as master. Unsure if we want in-line comments, but the functionality hasn't changed. |
|
Bito Automatic Review Skipped – PR Already Merged |
#40039) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
SUMMARY
Fixes two intertwined bugs in
AsyncSelectthat cause server search results to be buried under cached page-0 options when there are >100 total records.Bug 1 —
handleFilterOptionHelperbreaksfilterOption={false}:When callers pass
filterOption={false}(AntD's documented convention for "server handles filtering, show all provided options"), the helper falls through toreturn false, hiding every option and collapsing the dropdown to "No data".Bug 2 —
mergeDatamerges search results with cached page-0 options:When a search fetch resolves, results are merged with all previously-cached options instead of replacing them. With >100 records, the actual match is appended at position 100+, buried under unrelated cached entries.
BEFORE/AFTER SCREENSHOTS OR COVERAGE
Before: Typing a search term that matches 1 record shows 101 options (100 cached page-0 + 1 match at the bottom).
After: Typing a search term shows only the server's response. Clearing the search restores the original page-0 options.
TESTING INSTRUCTIONS
AsyncSelectconsumer (e.g., users, datasets, dashboards).AsyncSelect.ADDITIONAL INFORMATION
Changes:
utils.tsx: Added explicitfilterOption === falsecheck that returnstrue(show all options) instead of falling through toreturn false.AsyncSelect.tsx:initialOptionsRefto cache base (no-search) options andwasSearchingRefto track search state.fetchPageto replaceselectOptionson search page-0 instead of merging.inputValueeffect to restore base options when search clears, withwasSearchingRefguard to avoid interfering with normal dropdown open behavior.AsyncSelect.test.tsx: Added 3 tests covering search-replace,filterOption={false}, and search-clear restoration.Has associated issue
Changes UI
Includes DB Migration
Changes existing API(s) or adds new API(s)