codeant-ai-for-open-source[bot] commented on code in PR #42009:
URL: https://github.com/apache/superset/pull/42009#discussion_r3578596947
##########
superset-frontend/packages/superset-ui-core/src/components/ModalTrigger/index.tsx:
##########
@@ -126,7 +127,13 @@ export const ModalTrigger = forwardRef(
</Button>
)}
{!isButton && (
- <div data-test="span-modal-trigger" onClick={open} role="button">
+ <div
+ data-test="span-modal-trigger"
+ onClick={open}
+ onKeyDown={handleKeyboardActivation(open)}
+ role="button"
+ tabIndex={0}
+ >
Review Comment:
**Suggestion:** The `data-test` identifier says `span-modal-trigger` even
though the element is now a `<div>`, which creates a misleading test contract
and makes selector intent inconsistent with the rendered DOM. Rename the test
id to match the actual element naming convention. [inconsistent naming]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Suggestion addresses non-issue; behavior and tests unchanged.
- โ ๏ธ Existing tests intentionally rely on span-modal-trigger id.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. In
`superset-frontend/packages/superset-ui-core/src/components/ModalTrigger/index.tsx`
lines 129-137, when `isButton` is false the default trigger renders as a div
element with
`data-test="span-modal-trigger"`, so the attribute name still references the
legacy span
despite the DOM node now being a div.
2. Tests in
`superset-frontend/packages/superset-ui-core/src/components/ModalTrigger/ModalTrigger.test.tsx`
(e.g. line 39 from Grep output) use
`screen.getByTestId('span-modal-trigger')` to assert
the trigger exists, but they do not assert the underlying tag name, so all
tests continue
to pass even though the element is now a div.
3. Additional callers such as
`superset-frontend/src/SqlLab/components/ScheduleQueryButton/ScheduleQueryButton.test.tsx`
(lines 87 and 116 from Grep output) also query
`screen.getByTestId('span-modal-trigger')`
and then click it; these tests rely only on the test id string, not the
element type, so
runtime behavior and tests remain correct.
4. Because the `data-test` attribute is an opaque identifier used purely for
test
selection and not for semantic checks, the mismatch between the id name
("span") and the
actual element type (div) is cosmetic and aligns with existing tests that
deliberately
continue to reference `span-modal-trigger`, making this suggestion a naming
cleanup rather
than a functional bug.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a24a266959504e37a6d4911fae4da02e&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=a24a266959504e37a6d4911fae4da02e&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/ModalTrigger/index.tsx
**Line:** 130:136
**Comment:**
*Inconsistent Naming: The `data-test` identifier says
`span-modal-trigger` even though the element is now a `<div>`, which creates a
misleading test contract and makes selector intent inconsistent with the
rendered DOM. Rename the test id to match the actual element naming convention.
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%2F42009&comment_hash=a6abeda10a97848de24cabab0c1147dce9ee158207722b31d65a5e11a3393fc4&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42009&comment_hash=a6abeda10a97848de24cabab0c1147dce9ee158207722b31d65a5e11a3393fc4&reaction=dislike'>๐</a>
##########
superset-frontend/packages/superset-ui-core/src/utils/handleKeyboardActivation.ts:
##########
@@ -0,0 +1,43 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { KeyboardEvent } from 'react';
+
+/**
+ * Builds an `onKeyDown` handler that invokes `callback` when the user presses
+ * Enter or Space, mirroring a click for keyboard users. Pair it with an
+ * element's `onClick` so `role="button"` (or similar) controls are operable
+ * from the keyboard, satisfying `jsx-a11y/click-events-have-key-events`.
+ *
+ * <div role="button" onClick={handleClick}
+ * onKeyDown={handleKeyboardActivation(handleClick)} />
+ */
+export function handleKeyboardActivation(
+ callback: (event: KeyboardEvent) => void,
+) {
+ return (event: KeyboardEvent) => {
+ if (event.key === 'Enter' || event.key === ' ') {
+ // Prevent the page from scrolling on Space and stop any duplicate
+ // default activation on Enter.
+ event.preventDefault();
+ callback(event);
Review Comment:
**Suggestion:** The keyboard handler fires on every repeated `keydown` event
while Enter/Space is held, so a single long press can trigger the callback
multiple times (for example opening/submitting repeatedly). Guard against
auto-repeat (for example by ignoring events where the repeat flag is set) so
keyboard activation behaves like a single intentional action. [logic error]
<details>
<summary><b>Severity Level:</b> Major โ ๏ธ</summary>
```mdx
- โ Explore sidebar toggle misbehaves with held Space key.
- โ ๏ธ Dataset creation link may open modal multiple times.
- โ ๏ธ Keyboard activation inconsistent with click for many controls.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. In
`superset-frontend/packages/superset-ui-core/src/utils/handleKeyboardActivation.ts`
lines 30-38, `handleKeyboardActivation` returns an `onKeyDown` handler that
checks `if
(event.key === 'Enter' || event.key === ' ')` and always calls
`callback(event)` without
checking `event.repeat`, so every auto-repeated keydown will invoke the
callback.
2. In
`superset-frontend/src/explore/components/ExploreViewContainer/index.tsx` lines
545-547, `toggleCollapse()` toggles the sidebar state by calling
`setIsCollapsed(!isCollapsed)`, and further down at lines 996-1034, a span
with role
button wires `onKeyDown={handleKeyboardActivation(toggleCollapse)}` for both
the "Chart
Source" action button and the collapsed sidebar opener.
3. Run the frontend and navigate to an Explore view that renders
`ExploreViewContainer`;
using the keyboard, focus the "Open Datasource tab" sidebar control
(rendered from the
span at `ExploreViewContainer/index.tsx:30-38` in the snippet shown) by
tabbing until the
control has focus.
4. Press and hold the Space key: the browser emits repeated keydown events
while the key
is held, `handleKeyboardActivation` calls `toggleCollapse()` on every
repeat, and because
`toggleCollapse` flips `isCollapsed` on each call, the sidebar repeatedly
opens and
closes, leaving it in an effectively random final state depending on key
repeat timing,
demonstrating the multi-activation bug caused by not ignoring repeated
keydown events.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=17df90a2e5c346c59e62db0f722a48e5&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=17df90a2e5c346c59e62db0f722a48e5&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/utils/handleKeyboardActivation.ts
**Line:** 34:38
**Comment:**
*Logic Error: The keyboard handler fires on every repeated `keydown`
event while Enter/Space is held, so a single long press can trigger the
callback multiple times (for example opening/submitting repeatedly). Guard
against auto-repeat (for example by ignoring events where the repeat flag is
set) so keyboard activation behaves like a single intentional action.
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%2F42009&comment_hash=3175badd7df562973cf61b1571af44e7759b137cb99f4a6c2ac4e41177b1a2af&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42009&comment_hash=3175badd7df562973cf61b1571af44e7759b137cb99f4a6c2ac4e41177b1a2af&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]
