EnxDev commented on PR #37978:
URL: https://github.com/apache/superset/pull/37978#issuecomment-4809677822
## EnxDev's Review Agent — apache/superset#37978 · HEAD 3d52828
**comment** — Fix mechanism is reasonable, but the new tests don't actually
guard the regression. Builds on my earlier manual review: the `open()`
`preventDefault()` revert ✓ and Menu-nesting in the test ✓ are addressed; test
validity is still open.
### 🟡 Should-fix
- **`ModalTrigger.test.tsx:94-141`** — This test dispatches a **native**
`KeyboardEvent` (`input.dispatchEvent`) and asserts native `defaultPrevented
=== false`, but the fix only calls `e.stopPropagation()` on a **React
synthetic** `onKeyDown`. The two event systems don't intersect here, so the
assertion is decoupled from the code under test — it almost certainly stays
green with the `<div onKeyDown>` wrapper reverted, i.e. it's not
red-before-green. Could you confirm it fails with the wrapper removed (revert
`index.tsx`, run the test → must be red)? If it can't be made red, drive the
key through `fireEvent.keyDown`/`userEvent` on the rendered tree and assert the
user-visible outcome (dropdown stays open, cursor/selection reaches the input)
instead of native `defaultPrevented`.
- **`ModalTrigger.test.tsx:79-92`** — "trigger click should preventDefault"
pins an implementation detail (`preventDefault` was called) rather than the
behavior it protects (dropdown closes on select). It guards the revert
mechanically but wouldn't catch a real regression in close behavior.
### 🔵 Nits
- `index.tsx:136` — `React.KeyboardEvent<HTMLDivElement>` annotation is
redundant (the `onKeyDown` prop already infers the param type) and inconsistent
with this file's named-import style (`MouseEvent` is imported from `react`).
Drop the annotation.
- `ModalTrigger.test.tsx:20` — `import { Menu } from 'antd'` violates the
no-direct-antd-import rule; pull `Menu` from `@superset-ui/core/components`.
(Already flagged inline by the codeant-ai bot.)
- `index.tsx:135-142` — wrapping every `modalBody` in an unstyled `<div>`
applies to all `ModalTrigger` consumers, not just the dashboard dropdown case.
Low risk, but worth a quick check that no modal relies on its body's first
child being a direct flex/height child.
### 🙌 Praise
- `index.tsx` — clear root-cause writeup, and reverting the `open()`
`preventDefault()` change keeps dropdown-close behavior intact for all other
callers.
<!-- enxdev-review-agent:3d52828 -->
_Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 3d52828._
--
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]
