codeant-ai-for-open-source[bot] commented on code in PR #35218:
URL: https://github.com/apache/superset/pull/35218#discussion_r3253509336
##########
superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx:
##########
@@ -438,7 +438,11 @@ const Select = forwardRef(
onSearch?.(searchValue);
}, Constants.FAST_DEBOUNCE);
- useEffect(() => () => handleOnSearch.cancel(), [handleOnSearch]);
+ useEffect(() => {
+ if (handleOnSearch !== undefined) {
+ handleOnSearch.cancel();
+ }
+ }, [handleOnSearch]);
Review Comment:
**Suggestion:** This effect calls `cancel()` during normal effect execution
instead of in an effect cleanup, so it does not run on unmount. Any pending
debounced search callback can still fire after the component is destroyed,
causing stale `onSearch` calls and state updates from an unmounted instance.
Move cancellation into the returned cleanup function. [missing cleanup]
<details>
<summary><b>Severity Level:</b> Critical ๐จ</summary>
```mdx
- โ Native select filters can dispatch search after unmount.
- โ ๏ธ State updates scheduled on unmounted Select instances.
- โ ๏ธ Potential React warnings and confusing dashboard filter behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. In
`superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx`,
find
`handleOnSearch` defined as a debounced function at lines 387โ460 (from
Grep): it trims
the search string, updates local state via `setIsSearching`,
`setVisibleOptions`,
`setInputValue`, and calls the external `onSearch?.(searchValue)` callback.
2. In the same file, note that this `handleOnSearch` is passed to the
underlying Ant
Design `<Select>` as its `onSearch` prop (line ~775 from Grep:
`onSearch={shouldShowSearch
? handleOnSearch : undefined}`), so any user input in searchable selects
will eventually
call `handleOnSearch` after the debounce delay.
3. Immediately after the debounced function, observe the added effect at
lines 441โ445:
`useEffect(() => { if (handleOnSearch !== undefined) {
handleOnSearch.cancel(); } },
[handleOnSearch]);`. This effect calls `cancel()` when the effect body runs
(after mount
and whenever `handleOnSearch` identity changes) but **does not return a
cleanup
function**, so React does not call `handleOnSearch.cancel()` on unmount or
when replacing
a previous debounced instance.
4. In real usage, `Select` is used by the dashboard native filter plugin:
`src/filters/components/Select/SelectFilterPlugin.tsx` defines a debounced
`onSearch`
(lines 265โ279) that dispatches data mask updates and passes it to `<Select
...
onSearch={onSearch} ...>` (lines 603โ609). If a user types a new value
(triggering
`handleOnSearch`) and then quickly removes/closes the filter so that the
`<Select>`
unmounts within the debounce window, the pending `handleOnSearch` callback
still fires
after unmount (because no cleanup calls `cancel()`), calling
`setIsSearching`,
`setVisibleOptions`, and the plugin's `onSearch`. This causes state updates
and data-mask
dispatches originating from an unmounted filter instanceโthe exact behavior
the PR seeks
to avoidโand will only be prevented if `handleOnSearch.cancel()` is moved
into an effect
cleanup (`useEffect(() => () => handleOnSearch?.cancel(),
[handleOnSearch]);`).
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fpackages%2Fsuperset-ui-core%2Fsrc%2Fcomponents%2FSelect%2FSelect.tsx%0A%2A%2ALine%3A%2A%2A%20441%3A445%0A%2A%2AComment%3A%2A%2A%0A%09%2AMissing%20Cleanup%3A%20This%20effect%20calls%20%60cancel%28%29%60%20during%20normal%20effect%20execution%20instead%20of%20in%20an%20effect%20cleanup%2C%20so%20it%20does%20not%20run%20on%20unmount.%20Any%20pending%20debounced%20search%20callback%20can%20still%20fire%20after%20the%20component%20is%20destroyed%2C%20causing%20stale%20%60onSearch%60%20calls%20and%20state%20updates%20from%20an%20unmounted%20instance.%20Move%20cancellation%20into%20the%20returned%20cleanup%20function.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20c
oncise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fpackages%2Fsuperset-ui-core%2Fsrc%2Fcomponents%2FSelect%2FSelect.tsx%0A%2A%2ALine%3A%2A%2A%20441%3A445%0A%2A%2AComment%3A%2A%2A%0A%09%2AMissing%20Cleanup%3A%20This%20effect%20calls%20%60cancel%28%29%60%20during%20normal%20effect%20execution%20instead%20of%20in%20an%20effect%20cleanup%2C%20so%20it%20does%20not%20run%20on%20unmount.%20Any%20pending%20debounced%20search%20callback%20can%20still%20fire%20after%20the%20component%20is%20destroyed%2C%20causing%20stale
%20%60onSearch%60%20calls%20and%20state%20updates%20from%20an%20unmounted%20instance.%20Move%20cancellation%20into%20the%20returned%20cleanup%20function.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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:** 441:445
**Comment:**
*Missing Cleanup: This effect calls `cancel()` during normal effect
execution instead of in an effect cleanup, so it does not run on unmount. Any
pending debounced search callback can still fire after the component is
destroyed, causing stale `onSearch` calls and state updates from an unmounted
instance. Move cancellation into the returned cleanup function.
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=431367d04a8fe6b771b26c7e85a0ab59938689905df97e91ed977617d0ba71a0&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35218&comment_hash=431367d04a8fe6b771b26c7e85a0ab59938689905df97e91ed977617d0ba71a0&reaction=dislike'>๐</a>
##########
superset-frontend/packages/superset-ui-core/src/components/Select/Select.test.tsx:
##########
@@ -1294,6 +1294,33 @@ describe('grouped options search', () => {
});
});
+test('should not call search after unmount', async () => {
+ const mockOnSearch = jest.fn();
+ const { unmount, rerender } = render(
+ <Select
+ {...defaultProps}
+ allowNewOptions
+ mode="multiple"
+ onSearch={mockOnSearch}
+ />,
+ );
+ await type('12', 0);
+ unmount();
+ await new Promise(resolve => setTimeout(resolve, 300));
+ expect(mockOnSearch).toHaveBeenCalledWith('12');
+ rerender(
+ <Select
+ {...defaultProps}
+ allowNewOptions
+ mode="multiple"
+ onSearch={mockOnSearch}
+ />,
+ );
+ await type('1234', 0);
+ unmount();
+ await new Promise(resolve => setTimeout(resolve, 300));
+ expect(mockOnSearch).toHaveBeenCalledWith('1234');
Review Comment:
**Suggestion:** The test title says search should not run after unmount, but
the assertions require it to have been called after waiting for the debounce
window. This inverts the intended behavior and will pass even when the unmount
cleanup is broken; assert that the callback is not called after unmount
instead. [logic error]
<details>
<summary><b>Severity Level:</b> Major โ ๏ธ</summary>
```mdx
- โ Unit test encodes opposite of desired behavior.
- โ ๏ธ Broken unmount cleanup will not be caught by tests.
- โ ๏ธ Future fixes to cancel search on unmount will fail tests.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Open
`superset-frontend/packages/superset-ui-core/src/components/Select/Select.test.tsx`
and
locate the test `test('should not call search after unmount', async () => {
... })` around
lines 1297โ1323, which is intended to verify that search is not invoked
after the
component is unmounted.
2. Inside this test, note the first scenario: `render(<Select
{...defaultProps}
allowNewOptions mode="multiple" onSearch={mockOnSearch} />); await
type('12', 0);
unmount(); await new Promise(resolve => setTimeout(resolve, 300));
expect(mockOnSearch).toHaveBeenCalledWith('12');` (expect call at line
1310). The
expectation explicitly requires `mockOnSearch` to be called after unmounting
and waiting
for the debounce window.
3. Observe the second scenario in the same test: after `rerender(...)`, it
again types
`'1234'`, calls `unmount()`, waits 300 ms, and asserts
`expect(mockOnSearch).toHaveBeenCalledWith('1234');` at line 1322. This
again asserts that
`onSearch` is invoked after unmount.
4. Compare this with the intended runtime behavior: `Select.tsx` defines a
debounced
`handleOnSearch` (lines 387โ460 in
`superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx`)
and a
`useEffect` (lines 441โ445) intended to cancel it, and the PR description
says it should
prevent cleanup/search from running after unmount. Because this test
currently asserts
that `mockOnSearch` *does* fire after unmount, it will pass even when
unmount cleanup is
broken (current state) and would fail if `handleOnSearch` were correctly
canceled on
unmount, meaning the test enforces the opposite of the desired behavior.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fpackages%2Fsuperset-ui-core%2Fsrc%2Fcomponents%2FSelect%2FSelect.test.tsx%0A%2A%2ALine%3A%2A%2A%201310%3A1322%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20The%20test%20title%20says%20search%20should%20not%20run%20after%20unmount%2C%20but%20the%20assertions%20require%20it%20to%20have%20been%20called%20after%20waiting%20for%20the%20debounce%20window.%20This%20inverts%20the%20intended%20behavior%20and%20will%20pass%20even%20when%20the%20unmount%20cleanup%20is%20broken%3B%20assert%20that%20the%20callback%20is%20not%20called%20after%20unmount%20instead.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check
%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fpackages%2Fsuperset-ui-core%2Fsrc%2Fcomponents%2FSelect%2FSelect.test.tsx%0A%2A%2ALine%3A%2A%2A%201310%3A1322%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20The%20test%20title%20says%20search%20should%20not%20run%20after%20unmount%2C%20but%20the%20assertions%20require%20it%20to%20have%20been%20called%20after%20waiting%20for%20the%20debounce%20window.%20This%20inverts%20the%20intended%20behavior%20and%20will%20pass%20even%20when%20the%20unmount%20cleanup%20is%20broken%3B%20assert%20that%20the%20callback%20is%20no
t%20called%20after%20unmount%20instead.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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.test.tsx
**Line:** 1310:1322
**Comment:**
*Logic Error: The test title says search should not run after unmount,
but the assertions require it to have been called after waiting for the
debounce window. This inverts the intended behavior and will pass even when the
unmount cleanup is broken; assert that the callback is not called after unmount
instead.
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=4f40a0a7f32e2d4b14aa17a9c2adf91290a8a66bb3f67a0b2844071a5962e681&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35218&comment_hash=4f40a0a7f32e2d4b14aa17a9c2adf91290a8a66bb3f67a0b2844071a5962e681&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]
