codeant-ai-for-open-source[bot] commented on code in PR #35218:
URL: https://github.com/apache/superset/pull/35218#discussion_r3609098339
##########
superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx:
##########
@@ -468,7 +468,15 @@ const Select = forwardRef(
onSearch?.(searchValue);
}, Constants.FAST_DEBOUNCE);
- useEffect(() => () => handleOnSearch.cancel(), [handleOnSearch]);
+ const memoizedHandleOnSearch = useMemo(
+ () => handleOnSearch,
+ [handleOnSearch],
+ );
Review Comment:
**Suggestion:** This memoization does not stabilize the debounced callback
because `handleOnSearch` is recreated every render, so the memoized value also
changes every render. That defeats the intended lifecycle control and causes
the related effect to rerun continuously; memoize the debounce creation itself
(with proper dependencies) so the handler identity is actually stable. [code
quality]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Debounced search handler identity changes on every render.
- ⚠️ Cancellation effect reruns continuously without stable target.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In
`superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx`
at
line 417 (Grep output), `handleOnSearch` is declared as `const
handleOnSearch =
debounce((search: string) => { ... }, Constants.FAST_DEBOUNCE);` directly
inside the
component body, meaning a new debounced function instance is created on
every render.
2. Immediately after this, the new code at lines 471–474 in the diff
(confirmed by Grep at
472–473) defines `const memoizedHandleOnSearch = useMemo(() =>
handleOnSearch,
[handleOnSearch]);`. Because `handleOnSearch` itself is recreated on each
render, the
dependency array `[handleOnSearch]` also changes on every render, forcing
`useMemo` to
recompute and return the latest function instance each time.
3. The subsequent `useEffect` at lines 475–479 depends on
`[memoizedHandleOnSearch]`, so
it re-runs after every render as well. There is no stable handler identity
across renders
for lifecycle management; the debounced callback instance used for
`onSearch` is
continuously recreated rather than being anchored for the component lifetime.
4. In practice, any user interaction that causes the Select to
re-render—such as typing
into the search box (wired via `onSearch={shouldShowSearch ? handleOnSearch
: undefined}`
at lines 37–38 of the JSX) or selecting options (`onSelect`/`onDeselect`
handlers
nearby)—will recreate `handleOnSearch`, recompute `memoizedHandleOnSearch`,
and rerun the
effect. This defeats the attempted memoization and means the effect cannot
reliably track
or cancel a single debounced handler instance, leaving the debounce
lifecycle control
fragile and making it hard to reason about when `.cancel()` actually affects
pending
search work.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=54a5f97bbfae4655917a143e0a860818&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=54a5f97bbfae4655917a143e0a860818&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx
**Line:** 471:474
**Comment:**
*Code Quality: This memoization does not stabilize the debounced
callback because `handleOnSearch` is recreated every render, so the memoized
value also changes every render. That defeats the intended lifecycle control
and causes the related effect to rerun continuously; memoize the debounce
creation itself (with proper dependencies) so the handler identity is actually
stable.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35218&comment_hash=f8aa72e8a331c0dd81f2a489b013209b66de5068f3cfe70bc442ed5fb6360a5a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35218&comment_hash=f8aa72e8a331c0dd81f2a489b013209b66de5068f3cfe70bc442ed5fb6360a5a&reaction=dislike'>👎</a>
##########
superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx:
##########
@@ -468,7 +468,15 @@ const Select = forwardRef(
onSearch?.(searchValue);
}, Constants.FAST_DEBOUNCE);
- useEffect(() => () => handleOnSearch.cancel(), [handleOnSearch]);
+ const memoizedHandleOnSearch = useMemo(
+ () => handleOnSearch,
+ [handleOnSearch],
+ );
+ useEffect(() => {
+ if (memoizedHandleOnSearch?.cancel) {
+ memoizedHandleOnSearch.cancel();
+ }
+ }, [memoizedHandleOnSearch]);
Review Comment:
**Suggestion:** The debounce cancellation is executed inside the effect body
instead of in a cleanup function, so it runs immediately after render and on
every dependency change rather than at unmount. This can cancel in-flight
searches during normal rerenders and drop user-typed search updates; move
cancellation to the effect cleanup so it only cancels the previous handler
lifecycle. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Pending debounced searches not cancelled when Select unmounts.
- ⚠️ React warns about updating unmounted Select component.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In
`superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx`
at
line 417 (per Grep output), `handleOnSearch` is defined as `const
handleOnSearch =
debounce((search: string) => { ... }, Constants.FAST_DEBOUNCE);` and inside
the debounced
body it calls `setIsSearching`, `setSelectOptions`, `setVisibleOptions`,
`setInputValue`,
and `onSearch?.(searchValue)` to drive the Select's search behaviour.
2. At the bottom of the same file, around lines 37–43 of the AntD Select JSX
(Read output
lines 23–43), the component wires `onSearch={shouldShowSearch ?
handleOnSearch :
undefined}`, so every time a user types into the Select's search box, the
debounced
`handleOnSearch` is invoked and may schedule state updates after a delay.
3. The new hook block introduced in this PR (lines 471–479 in the diff,
confirmed by Grep
at 472–477) creates `const memoizedHandleOnSearch = useMemo(() =>
handleOnSearch,
[handleOnSearch]);` and then runs `useEffect(() => { if
(memoizedHandleOnSearch?.cancel) {
memoizedHandleOnSearch.cancel(); } }, [memoizedHandleOnSearch]);`. This
effect body
executes immediately after each render, and there is no cleanup function
returned from the
effect.
4. Because React only runs effect cleanup on unmount, and this effect does
all
cancellation in the body instead of the cleanup, unmounting the Select while
a debounced
search is pending (for example when a dashboard filter using this Select is
removed or the
user navigates away mid-typing) will not call `cancel()`. The pending
debounced callback
can therefore still fire after unmount and attempt to call the state setters
defined in
this component, leading to React "state update on unmounted component"
warnings and stale
search work instead of the intended cleanup-on-unmount behaviour.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bce6fb04b34944aabb1161172bdb9445&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=bce6fb04b34944aabb1161172bdb9445&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx
**Line:** 475:479
**Comment:**
*Logic Error: The debounce cancellation is executed inside the effect
body instead of in a cleanup function, so it runs immediately after render and
on every dependency change rather than at unmount. This can cancel in-flight
searches during normal rerenders and drop user-typed search updates; move
cancellation to the effect cleanup so it only cancels the previous handler
lifecycle.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35218&comment_hash=e1c09f4e04e7b87f49a8a75a23253125dc4033a7b3706cb89ad359f0a7819c0a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35218&comment_hash=e1c09f4e04e7b87f49a8a75a23253125dc4033a7b3706cb89ad359f0a7819c0a&reaction=dislike'>👎</a>
--
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]
