rusackas commented on code in PR #44880:
URL: https://github.com/apache/superset/pull/44880#discussion_r4162927243
##########
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:
Good catch. useDeferredValue only lowers render priority, so it was
dispatching on every keystroke. I put a SLOW_DEBOUNCE quiet period back in
front of the search dispatch (the deferred value still drives the options
rendering), and clear-all cancels it. Pushed ce7f263.
##########
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:
Same fix here. The LIKE input now goes through a SLOW_DEBOUNCE again before
it hits updateDataMask, and it is canceled on clear-all, operator switch, and
unmount. Pushed ce7f263. I could not run the jest suite locally though, so
leaning on CI to confirm.
##########
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, and that is what I did in ce7f263: the search dispatch and the LIKE
update each have their own SLOW_DEBOUNCE quiet period now, independent of the
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]
