sadpandajoe commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4162824938
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -564,32 +608,27 @@ export default function PluginFilterSelect(props:
PluginFilterSelectProps) {
updateDataMaskRef.current = updateDataMask;
}, [updateDataMask]);
- const debouncedLikeChange = useMemo(
- () =>
- debounce((text: string) => {
- if (text) {
- updateDataMaskRef.current([text]);
- } else {
- updateDataMaskRef.current(null);
- }
- }, Constants.SLOW_DEBOUNCE),
- [],
- );
-
useEffect(() => {
- if (!isLikeOperator || clearAllTrigger) {
- debouncedLikeChange.cancel();
+ if (
+ deferredPendingLikeValue === null ||
Review Comment:
After typing a Contains value and clicking Clear All, onClearAllComplete
removes the trigger while deferredPendingLikeValue can still hold the old text;
this effect can then restore the cleared predicate, and the later null deferred
value does not undo it. Could the reset invalidate the pending edit
synchronously, with a regression that types first and releases the Clear All
trigger?
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -564,32 +608,27 @@ export default function PluginFilterSelect(props:
PluginFilterSelectProps) {
updateDataMaskRef.current = updateDataMask;
}, [updateDataMask]);
- const debouncedLikeChange = useMemo(
- () =>
- debounce((text: string) => {
- if (text) {
- updateDataMaskRef.current([text]);
- } else {
- updateDataMaskRef.current(null);
- }
- }, Constants.SLOW_DEBOUNCE),
- [],
- );
-
useEffect(() => {
- if (!isLikeOperator || clearAllTrigger) {
- debouncedLikeChange.cancel();
+ if (
+ deferredPendingLikeValue === null ||
+ !isLikeOperator ||
+ clearAllTrigger
+ ) {
+ return;
}
- }, [clearAllTrigger, debouncedLikeChange, isLikeOperator]);
-
- useEffect(() => () => debouncedLikeChange.cancel(), [debouncedLikeChange]);
+ if (deferredPendingLikeValue) {
+ updateDataMaskRef.current([deferredPendingLikeValue]);
+ } else {
+ updateDataMaskRef.current(null);
+ }
+ }, [deferredPendingLikeValue, isLikeOperator, clearAllTrigger]);
Review Comment:
Agreed—changing Contains to Exact clears the configured default, but
returning to Contains reruns this effect with the retained pendingLikeValue and
restores the old default without another edit. Could leaving LIKE mode discard
that pending edit rather than only skip dispatch?
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -280,32 +296,58 @@ export default function PluginFilterSelect(props:
PluginFilterSelectProps) {
const isDisabled =
appSection === AppSection.FilterConfigModal && defaultToFirstItem;
- const onSearch = useMemo(
- () =>
- debounce((search: string) => {
- setSearch(search);
- if (searchAllOptions) {
- dispatchDataMask({
- type: 'ownState',
- ownState: {
- coltypeMap: initialColtypeMap,
- // The dropdown offers `stripSurroundingQuotes(search)` as the
- // creatable option, so the server has to be asked for the same
- // string or the two disagree about what was searched for.
- search: stripSurroundingQuotes(search).trim(),
- },
- });
- }
- }, Constants.SLOW_DEBOUNCE),
- [dispatchDataMask, initialColtypeMap, searchAllOptions],
- );
+ const handleSearch = useCallback((value: string) => {
+ hasSearchedRef.current = true;
+ setSearchInput(value);
+ }, []);
+
+ // Read through a ref so a parent re-render that hands down new identities
+ // for these doesn't, by itself, re-fire the dispatch below -- only an
+ // actual search settling (a change to deferredSearchInput) should.
+ const searchDispatchRef = useRef({
+ searchAllOptions,
+ dispatchDataMask,
+ initialColtypeMap,
+ });
+ useEffect(() => {
+ searchDispatchRef.current = {
+ searchAllOptions,
+ dispatchDataMask,
+ initialColtypeMap,
+ };
+ });
+
+ useEffect(() => {
+ if (!hasSearchedRef.current) {
+ return;
+ }
+ const {
+ searchAllOptions: currentSearchAllOptions,
+ dispatchDataMask: currentDispatchDataMask,
+ initialColtypeMap: currentInitialColtypeMap,
+ } = searchDispatchRef.current;
+ if (!currentSearchAllOptions) {
+ return;
+ }
+ currentDispatchDataMask({
+ type: 'ownState',
+ ownState: {
+ coltypeMap: currentInitialColtypeMap,
+ // The dropdown offers `stripSurroundingQuotes(search)` as the
+ // creatable option, so the server has to be asked for the same
+ // string or the two disagree about what was searched for.
+ search: stripSurroundingQuotes(deferredSearchInput).trim(),
+ },
+ });
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [deferredSearchInput]);
Review Comment:
Agreed—the remaining Select debounce is 250 ms, so typing 300 ms apart now
starts partial server searches that the removed 500 ms gate coalesced; the
final term then waits behind an obsolete in-flight request. Could
query-producing search and LIKE updates retain a quiet-period debounce
independently of deferred rendering?
--
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]
