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


##########
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx:
##########
@@ -411,16 +413,22 @@ const AdhocFilterEditPopoverSimpleTabContent: FC<Props> = 
props => {
           });
       }
     };
+
     if (!datePicker) {
       refreshComparatorSuggestions();
     }
-  }, [props.adhocFilter.subject]);
+  }, [
+    props.adhocFilter.subject,
+    props.adhocFilter.clause,
+    props.datasource,
+    datePicker,
+  ]);
 
   useEffect(() => {
     if (isFeatureEnabled(FeatureFlag.EnableAdvancedDataTypes)) {
       fetchSubjectAdvancedDataType(props);
     }
-  }, [props.adhocFilter.subject]);
+  }, [props.adhocFilter.subject, fetchSubjectAdvancedDataType, props]);

Review Comment:
   ### Overly broad useEffect dependencies causing unnecessary re-renders 
<sub>![category 
Performance](https://img.shields.io/badge/Performance-4f46e5)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The useEffect dependency array includes the entire `props` object, which 
will cause the effect to run on every render when any prop changes, not just 
when the relevant properties change.
   
   
   ###### Why this matters
   This creates unnecessary re-renders and API calls to 
fetchSubjectAdvancedDataType, potentially causing performance issues and race 
conditions. The effect should only run when specific properties that affect the 
advanced data type fetching actually change.
   
   ###### Suggested change ∙ *Feature Preview*
   Replace the broad `props` dependency with specific properties that actually 
affect the advanced data type fetching:
   
   ```typescript
   useEffect(() => {
     if (isFeatureEnabled(FeatureFlag.EnableAdvancedDataTypes)) {
       fetchSubjectAdvancedDataType(props);
     }
   }, [props.adhocFilter.subject, props.datasource, 
fetchSubjectAdvancedDataType]);
   ```
   
   Or if fetchSubjectAdvancedDataType is stable and doesn't need to be in 
dependencies:
   
   ```typescript
   useEffect(() => {
     if (isFeatureEnabled(FeatureFlag.EnableAdvancedDataTypes)) {
       fetchSubjectAdvancedDataType(props);
     }
   }, [props.adhocFilter.subject, props.datasource]);
   ```
   
   
   ###### 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/3b976ac7-69da-4f11-9c88-e31f0a6343d2/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3b976ac7-69da-4f11-9c88-e31f0a6343d2?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/3b976ac7-69da-4f11-9c88-e31f0a6343d2?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/3b976ac7-69da-4f11-9c88-e31f0a6343d2?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3b976ac7-69da-4f11-9c88-e31f0a6343d2)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:46f5364b-b059-45af-aca0-4ab953a12c13 -->
   
   
   [](46f5364b-b059-45af-aca0-4ab953a12c13)



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