bito-code-review[bot] commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4162928015
##########
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:
<!-- Bito Reply -->
The implementation of a quiet-period debounce for the search dispatch is
correct. By using a `useMemo` to create a debounced function that updates the
state, you successfully coalesce the `ownState` dispatches while allowing
`useDeferredValue` to maintain responsive UI rendering. This approach
effectively prevents the issue of excessive server requests during rapid typing.
**superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx**
```
const debouncedSearch = useMemo(
() => debounce((value: string) => {
hasSearchedRef.current = true;
setSearchInput(value);
}, Constants.SLOW_DEBOUNCE),
[]
);
const handleSearch = useCallback((value: string) => {
debouncedSearch(value);
}, [debouncedSearch]);
```
##########
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:
<!-- Bito Reply -->
The implementation appears to correctly address the reviewer's concern by
re-introducing a debounce mechanism for the LIKE operator input. By using
`SLOW_DEBOUNCE` and ensuring the debounced function is properly canceled on
clear-all, operator switches, and component unmounts, the fix effectively
prevents excessive state updates on every keystroke.
--
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]
