Re: [PR] fix: Exclude Filter Values [superset]

Tue, 29 Apr 2025 03:37:38 -0700 


korbit-ai[bot] commented on code in PR #33271:
URL: https://github.com/apache/superset/pull/33271#discussion_r2066026583


##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -158,6 +158,8 @@
       : filterState?.excludeFilterValues,
   );
 
+  const prevExcludeFilterValues = useRef(excludeFilterValues);

Review Comment:
   ### Missing ref purpose documentation <sub>![category 
Documentation](https://img.shields.io/badge/Documentation-7c3aed)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The useRef hook's purpose for tracking the previous exclude filter value is 
not documented.
   
   ###### Why this matters
   Without understanding the purpose of this ref, future developers may 
inadvertently modify the comparison logic in the useEffect dependency.
   
   ###### Suggested change ∙ *Feature Preview*
   // Track previous excludeFilterValues to prevent unnecessary dataMask updates
   const prevExcludeFilterValues = useRef(excludeFilterValues);
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8f61e123-b084-466c-9636-fe527b2b0ea9/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8f61e123-b084-466c-9636-fe527b2b0ea9?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8f61e123-b084-466c-9636-fe527b2b0ea9?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8f61e123-b084-466c-9636-fe527b2b0ea9?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8f61e123-b084-466c-9636-fe527b2b0ea9)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:40ea27a4-48d9-4394-9ee3-52563ffe545f -->
   
   
   [](40ea27a4-48d9-4394-9ee3-52563ffe545f)



##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx:
##########
@@ -323,23 +325,26 @@ export default function PluginFilterSelect(props: 
PluginFilterSelectProps) {
   }, [JSON.stringify(dataMask)]);
 
   useEffect(() => {
-    dispatchDataMask({
-      type: 'filterState',
-      extraFormData: getSelectExtraFormData(
-        col,
-        filterState.value,
-        !filterState.value?.length,
-        excludeFilterValues && inverseSelection,
-      ),
-      filterState: {
-        ...(filterState as {
-          value: SelectValue;
-          label?: string;
-          excludeFilterValues?: boolean;
-        }),
-        excludeFilterValues,
-      },
-    });
+    if (prevExcludeFilterValues.current !== excludeFilterValues) {
+      dispatchDataMask({

Review Comment:
   ### Incomplete useEffect Dependencies <sub>![category 
Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The dependencies array in the useEffect hook is incomplete. It's missing 
dependencies that are used within the effect: col, filterState, 
inverseSelection, and dispatchDataMask.
   
   ###### Why this matters
   If any of the missing dependencies change, the effect won't re-run, 
potentially leading to stale data being used in the data mask dispatch and 
incorrect filter behavior.
   
   ###### Suggested change ∙ *Feature Preview*
   Add all required dependencies to the useEffect dependency array:
   
   ```typescript
   useEffect(() => {
       if (prevExcludeFilterValues.current !== excludeFilterValues) {
         dispatchDataMask({
           type: 'filterState',
           extraFormData: getSelectExtraFormData(
             col,
             filterState.value,
             !filterState.value?.length,
             excludeFilterValues && inverseSelection,
           ),
           filterState: {
             ...(filterState as {
               value: SelectValue;
               label?: string;
               excludeFilterValues?: boolean;
             }),
             excludeFilterValues,
           },
         });
         prevExcludeFilterValues.current = excludeFilterValues;
       }
   }, [excludeFilterValues, col, filterState, inverseSelection, 
dispatchDataMask]);
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd2e6dd0-0fce-48ce-a27a-1c6ae7fd0208/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd2e6dd0-0fce-48ce-a27a-1c6ae7fd0208?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd2e6dd0-0fce-48ce-a27a-1c6ae7fd0208?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd2e6dd0-0fce-48ce-a27a-1c6ae7fd0208?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dd2e6dd0-0fce-48ce-a27a-1c6ae7fd0208)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:acf37b65-23c8-473e-a3e3-2255d29d21f2 -->
   
   
   [](acf37b65-23c8-473e-a3e3-2255d29d21f2)



-- 
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]

Reply via email to