sadpandajoe commented on code in PR #40039:
URL: https://github.com/apache/superset/pull/40039#discussion_r3277326846
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -335,13 +337,22 @@ const AsyncSelect = forwardRef(
const fetchOptions = options as SelectOptionsPagePromise;
fetchOptions(search, page, pageSize)
.then(({ data, totalCount }: SelectOptionsTypePage) => {
- const mergedData = mergeData(data);
+ let resultData: SelectOptionsType;
+ if (search && page === 0) {
+ resultData = data.slice().sort(sortComparatorForNoSearch);
+ setSelectOptions(resultData);
+ } else {
+ resultData = mergeData(data);
+ if (!search) {
+ initialOptionsRef.current = resultData;
+ }
Review Comment:
Skipping — the multi-page accumulator is by design. Restoring on clear
should reflect everything the user already scrolled through, not a stale
snapshot of page 0 alone. The PR summary calls out replace-on-search /
restore-on-clear, but the accumulator that backs restore is intentionally
cumulative across all no-search pages (see the inline comment at
AsyncSelect.tsx#L362-385). This thread is also marked outdated against the
current diff.
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -518,12 +530,26 @@ const AsyncSelect = forwardRef(
if (loadingEnabled && allowFetch) {
// trigger fetch every time inputValue changes
if (inputValue) {
+ wasSearchingRef.current = true;
debouncedFetchPage(inputValue, 0);
} else {
+ if (wasSearchingRef.current && initialOptionsRef.current.length > 0)
{
+ setSelectOptions(
+ [...initialOptionsRef.current].sort(sortComparatorForNoSearch),
+ );
+ }
Review Comment:
Skipping — already mitigated by two layers in the current diff. (1) The
clear branch of the inputValue effect calls debouncedFetchPage.cancel() before
restoring/refetching (AsyncSelect.tsx#L592). (2) Even if a debounced search has
already fired, the response is dropped when inputValueRef.current !== search
(AsyncSelect.tsx#L358-361). The 're-shows search results when the same search
term is repeated after a clear' and 'replaces results when switching between
two searches' tests cover the resulting behavior.
##########
superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx:
##########
@@ -160,6 +160,13 @@ const AsyncSelect = forwardRef(
const [allValuesLoaded, setAllValuesLoaded] = useState(false);
const selectValueRef = useRef(selectValue);
const fetchedQueries = useRef(new Map<string, number>());
+ const initialOptionsRef = useRef<SelectOptionsType>(EMPTY_OPTIONS);
+ const inputValueRef = useRef('');
+ // Counts fetches whose `.finally` has not yet run. Loading is cleared only
+ // when this drops to 0, so a stale response (which returns early without
+ // updating selectOptions) cannot flip the spinner off while a newer
+ // request is still pending.
+ const inFlightFetchesRef = useRef(0);
Review Comment:
Good catch — applied in bd11723432. clearCache() now also resets
initialOptionsRef.current and allValuesLoaded so the imperative API truly
clears all cached state.
Note: while writing a test for this I hit a separate pre-existing issue —
the useImperativeHandle / <StyledSelect ref={ref}> pattern in this component
doesn't reliably expose clearCache via the forwarded ref (calling
selectRef.current?.clearCache() under @testing-library returns undefined).
That's orthogonal to this finding and predates this PR; worth a follow-up to
refactor onto an internal ref + useImperativeHandle pattern.
--
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]
