codeant-ai-for-open-source[bot] commented on code in PR #41100:
URL: https://github.com/apache/superset/pull/41100#discussion_r3418391007
##########
superset-frontend/src/explore/components/DataTableControl/FilterInput.test.tsx:
##########
@@ -34,3 +34,25 @@ test('Render a FilterInput', async () => {
expect(onChangeHandler).toHaveBeenCalledTimes(4);
});
+
+test('FilterInput does not steal focus when another input already has focus',
() => {
+ const onChangeHandler = jest.fn();
+
+ // Create a plain DOM input, focus it before mounting FilterInput
+ const otherInput = document.createElement('input');
+ document.body.appendChild(otherInput);
+ otherInput.focus();
+ expect(document.activeElement).toBe(otherInput);
+
+ // Mount FilterInput with shouldFocus=true while the other input is focused
+ const { container } = render(
+ <FilterInput onChangeHandler={onChangeHandler} shouldFocus />,
+ );
+ const filterInput = container.querySelector('input') as HTMLInputElement;
+
+ // FilterInput should not have stolen focus from the already-focused input
Review Comment:
**Suggestion:** This test can pass even when the filter textbox is missing
because `querySelector('input')` may return `null`, and
`expect(document.activeElement).not.toBe(filterInput)` will still pass in that
case. Add an explicit existence assertion (or query by role with a throwing
query) before the focus assertions so the test actually verifies the rendered
filter input. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Explore results filter regressions may bypass this focus test.
- ⚠️ Focus-stealing behavior changes might go undetected for
shouldFocus=true.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/src/explore/components/DataTableControl/FilterInput.test.tsx`
and locate the second test `FilterInput does not steal focus when another
input already
has focus` around lines 38–58 (confirmed via Grep output).
2. Observe that this test obtains the element with `const filterInput =
container.querySelector('input') as HTMLInputElement;` at line 51 but does
not assert that
`filterInput` is non-null before using it.
3. Note from the DOM API that `querySelector('input')` returns `null` if no
`<input>`
exists; in that case `filterInput` will be `null` at runtime, and
`expect(document.activeElement).not.toBe(filterInput);` at line 54 will
still pass because
`document.activeElement` (the other input created at lines 41–45) is not
`null`.
4. To concretely reproduce, temporarily modify `FilterInput` in
`superset-frontend/src/explore/components/DataTableControl/index.tsx` (lines
12–55 from
BulkRead) so that when `shouldFocus` is `true` it returns `null` instead of
the `<Input>`;
then run `npx jest
superset-frontend/src/explore/components/DataTableControl/FilterInput.test.tsx`
and
observe that the `FilterInput does not steal focus` test still passes even
though no
filter textbox was rendered, demonstrating the false-positive gap caused by
the missing
existence assertion.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f3aa223629644f689740e631b65c304a&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=f3aa223629644f689740e631b65c304a&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/src/explore/components/DataTableControl/FilterInput.test.tsx
**Line:** 51:53
**Comment:**
*Incomplete Implementation: This test can pass even when the filter
textbox is missing because `querySelector('input')` may return `null`, and
`expect(document.activeElement).not.toBe(filterInput)` will still pass in that
case. Add an explicit existence assertion (or query by role with a throwing
query) before the focus assertions so the test actually verifies the rendered
filter input.
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%2F41100&comment_hash=d2f4d93860f77be09680f333bf909c019efc58a4d2ecec97b148f48592c526d4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41100&comment_hash=d2f4d93860f77be09680f333bf909c019efc58a4d2ecec97b148f48592c526d4&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/DataTableControl/index.tsx:
##########
@@ -108,9 +108,16 @@ export const FilterInput = ({
const inputRef: RefObject<any> = useRef(null);
useEffect(() => {
- // Focus the input element when the component mounts
if (inputRef.current && shouldFocus) {
- inputRef.current.focus();
+ const activeEl = document.activeElement;
+ const isInputFocused =
+ activeEl &&
+ (activeEl.tagName === 'INPUT' ||
+ activeEl.tagName === 'TEXTAREA' ||
+ (activeEl as HTMLElement).isContentEditable);
+ if (!isInputFocused) {
+ inputRef.current.focus();
+ }
Review Comment:
**Suggestion:** The focus guard is too narrow: it only treats `INPUT`,
`TEXTAREA`, and contenteditable elements as "actively editing." If any other
focusable control is currently focused (for example a button-based control,
slider handle, or custom widget), this code will still force focus into the
filter input and can reproduce the same focus-stealing behavior. Guard against
stealing focus whenever `document.activeElement` is a meaningful focused
element (for example anything other than `body`/null), not only text-entry
tags. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Explore results controls can lose focus during data reloads.
- ⚠️ Keyboard users may experience unexpected focus jumps to search.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect `FilterInput` in
`superset-frontend/src/explore/components/DataTableControl/index.tsx`
(BulkRead lines
12–55): its `useEffect` at lines 21–33 focuses the underlying `<Input>`
(`inputRef.current.focus();`) whenever `shouldFocus` is `true` and
`isInputFocused` is
`false`.
2. Observe that `isInputFocused` is defined at lines 24–28 as `activeEl &&
(activeEl.tagName === 'INPUT' || activeEl.tagName === 'TEXTAREA' ||
(activeEl as
HTMLElement).isContentEditable);`, meaning any currently focused `<button>`,
`<span
role="button">`, `<select>`, or other custom control is *not* treated as a
protected focus
target.
3. Follow the call chain: `FilterInput` is used by `TableControls` in
`superset-frontend/src/explore/components/DataTablesPane/components/DataTableControls.tsx`
at line 17, which is in turn rendered by `SingleQueryResultPane`
(`superset-frontend/src/explore/components/DataTablesPane/components/SingleQueryResultPane.tsx`
lines 52–69) and wired up via `useResultsPane`
(`superset-frontend/src/explore/components/DataTablesPane/components/useResultsPane.tsx`
lines 29–60), so it appears in the Explore Results pane.
4. In a running Explore session using this code, focus the row-limit
`<Select>` or the
reload icon button rendered by `TableControls` (lines 25–33 and 57–65 in
`DataTableControls.tsx`); when `useResultsPane` triggers a re-fetch that
unmounts/remounts
`SingleQueryResultPane`, `FilterInput` mounts, `document.activeElement`
still points to
that non-text control, `isInputFocused` evaluates to `false`, and
`inputRef.current.focus();` at line 30 executes, stealing focus into the
search
input—demonstrating that the current guard still allows focus theft from
non-text but
meaningful focused controls.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d9a4b5f488934a24b91ed26e1498d5a3&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=d9a4b5f488934a24b91ed26e1498d5a3&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/src/explore/components/DataTableControl/index.tsx
**Line:** 113:120
**Comment:**
*Incorrect Condition Logic: The focus guard is too narrow: it only
treats `INPUT`, `TEXTAREA`, and contenteditable elements as "actively editing."
If any other focusable control is currently focused (for example a button-based
control, slider handle, or custom widget), this code will still force focus
into the filter input and can reproduce the same focus-stealing behavior. Guard
against stealing focus whenever `document.activeElement` is a meaningful
focused element (for example anything other than `body`/null), not only
text-entry tags.
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%2F41100&comment_hash=414343f68d1e2da88471929ff3134df8998ad0fd249f6a0a9179985a4d0a4592&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41100&comment_hash=414343f68d1e2da88471929ff3134df8998ad0fd249f6a0a9179985a4d0a4592&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]
