sadpandajoe opened a new issue, #44916:
URL: https://github.com/apache/superset/issues/44916
### Bug description
`ControlPopover` accepts an `open` prop (read as `visibleProp`) but copies
it into local `visible` state. Trigger clicks go through
`handleOnVisibleChange`, which calls `setVisible(!!visible)` unconditionally
and only then notifies `onOpenChange`. A parent that passes `open={false}` and
does not flip it (e.g. to veto opening, or because its own state has not
changed) still gets an open popover. The prop is only re-synced when its value
changes, so a parent that sets the same value again cannot close it either.
Steps to reproduce: render `<ControlPopover open={false} trigger="click"
content="x">trigger</ControlPopover>` and click the trigger. The popover opens.
Expected: with `open` provided the component is fully controlled and only
`onOpenChange` is emitted.
### Where
`superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx`
- L86, L93-L95 `open: visibleProp` copied into `useState`
- L146-L155 `handleOnVisibleChange`: `setVisible(!!visible);
props.onOpenChange?.(!!visible);`
- L175-L179 effect syncs only when `visibleProp` changes
- Same pattern in the Escape handler L157-L165.
### Suggested fix
When `visibleProp !== undefined`, derive `visible` from the prop and skip
`setVisible` in the handlers; keep local state only for the uncontrolled case.
### Found by
Found while adding React Testing Library coverage for Explore controls (all
line numbers are against `master` at `bc3698b5d6`).
--
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]
