codeant-ai-for-open-source[bot] commented on code in PR #37358:
URL: https://github.com/apache/superset/pull/37358#discussion_r2767597069
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx:
##########
@@ -459,6 +469,11 @@ function FiltersConfigModal({
[modalSaveLogic],
);
+ const handleValuesChange = useCallback(() => {
+ setFormValuesVersion(prev => prev + 1);
+ debouncedErrorHandling();
+ }, [debouncedErrorHandling]);
+
Review Comment:
**Suggestion:** The debounced error handler is recreated on every render
because it is defined with useMemo that depends on the entire modalSaveLogic
object, so every value change schedules a separate debounced function instance
and defeats debouncing, potentially triggering many handleErroredItems calls
instead of one and causing unnecessary work. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Filters edit modal: excessive validation calls on user typing.
- ⚠️ Modal performance degradation under rapid typing.
- ⚠️ Increased CPU work during filter configuration edits.
- ⚠️ Potential UI jank while editing filter names.
```
</details>
```suggestion
const debouncedErrorHandlingRef = useRef<ReturnType<typeof debounce>>();
useEffect(() => {
debouncedErrorHandlingRef.current = debounce(() => {
setSaveAlertVisible(false);
modalSaveLogic.handleErroredItems();
}, Constants.SLOW_DEBOUNCE);
return () => {
debouncedErrorHandlingRef.current?.cancel?.();
};
}, [modalSaveLogic.handleErroredItems]);
const handleValuesChange = useCallback(() => {
setFormValuesVersion(prev => prev + 1);
debouncedErrorHandlingRef.current?.();
}, []);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the dashboard and launch the Filters configuration modal which
renders
FiltersConfigModal component (file:
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx).
The BaseForm component passes onValuesChange to handleValuesChange (see the
BaseForm usage
that sets onValuesChange={handleValuesChange}).
2. Type into any input inside the modal. Each keystroke triggers BaseForm's
onValuesChange
which calls handleValuesChange (handleValuesChange definition at
FiltersConfigModal.tsx:472 in the PR hunk where debouncedErrorHandling is
defined).
3. On each render, debouncedErrorHandling is re-created because it's
memoized with a
dependency on the entire modalSaveLogic object (creation at
FiltersConfigModal.tsx:463).
If modalSaveLogic identity changes across renders (it is returned by
useModalSaveLogic
earlier in the same file at the call site: const modalSaveLogic =
useModalSaveLogic(...)
around FiltersConfigModal.tsx:232), the useMemo will produce a new debounce
wrapper each
render.
4. As a result, repeated typing causes many distinct debounced timers to be
scheduled
instead of reusing a single debounced function. Each timer eventually calls
modalSaveLogic.handleErroredItems(), causing multiple redundant
error-handling invocations
(observed behavior: multiple handleErroredItems calls logged or excessive
validation
work). This defeats the intended debounce and produces unnecessary work.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
**Line:** 463:476
**Comment:**
*Logic Error: The debounced error handler is recreated on every render
because it is defined with useMemo that depends on the entire modalSaveLogic
object, so every value change schedules a separate debounced function instance
and defeats debouncing, potentially triggering many handleErroredItems calls
instead of one and causing unnecessary work.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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]
