korbit-ai[bot] commented on code in PR #31792:
URL: https://github.com/apache/superset/pull/31792#discussion_r1910638350
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopoverTrigger.tsx:
##########
@@ -72,26 +72,33 @@ const ColumnSelectPopoverTrigger = ({
setPopoverLabel(initialPopoverLabel);
}, [initialPopoverLabel, popoverVisible]);
- const togglePopover = useCallback((visible: boolean) => {
- setPopoverVisible(visible);
- }, []);
+ const handleTogglePopover = useCallback(
+ (visible: boolean) => {
+ if (isControlledComponent) {
+ props.togglePopover!(visible);
+ } else {
+ setPopoverVisible(visible);
+ }
- const closePopover = useCallback(() => {
- setPopoverVisible(false);
- }, []);
+ if (!visible) {
+ setPopoverLabel(initialPopoverLabel);
+ setHasCustomLabel(false);
+ }
+ },
+ [initialPopoverLabel, isControlledComponent, props.togglePopover],
+ );
+
+ const handleClosePopover = useCallback(() => {
+ if (isControlledComponent) {
+ props.closePopover!();
+ } else {
+ setPopoverVisible(false);
+ }
+ setPopoverLabel(initialPopoverLabel);
+ setHasCustomLabel(false);
+ }, [initialPopoverLabel, isControlledComponent, props.closePopover]);
- const { visible, handleTogglePopover, handleClosePopover } =
- isControlledComponent
- ? {
- visible: props.visible,
- handleTogglePopover: props.togglePopover!,
- handleClosePopover: props.closePopover!,
- }
- : {
- visible: popoverVisible,
- handleTogglePopover: togglePopover,
- handleClosePopover: closePopover,
- };
+ const visible = isControlledComponent ? props.visible! : popoverVisible;
Review Comment:
### Unsafe visibility state access <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Non-null assertion is used on props.visible without ensuring it exists for
controlled components.
###### Why this matters
This could lead to runtime errors if the visible prop is not provided when
the component is used in controlled mode.
###### Suggested change ∙ *Feature Preview*
Add a validation check or provide a default value:
```typescript
const visible = isControlledComponent ? props.visible ?? false :
popoverVisible;
```
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:ffc64cdf-788f-49b5-92bd-59887d1fe76b -->
--
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]
