do not use operator executor for disabled buttons #5236
Conversation
WalkthroughThe Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/packages/operators/src/components/OperatorExecutionButton/index.tsx (2)
40-46: Consider prop consistency between render pathsThe early return pattern is a good optimization, but there's a subtle difference in how the
disabledprop is handled between the two render paths. The main path passesdisabledto bothOperatorExecutionTriggerandButton, while the early return only passes it toButton.Consider this more explicit implementation:
if (disabled) { return ( - <Button disabled {...props}> + <Button disabled={disabled} {...props}> {children} </Button> ); }
Line range hint
18-39: Consider extracting and extending props interfaceThe component's props are well-typed, but we could improve type reusability and maintainability.
Consider this improvement:
import { ButtonProps } from '@mui/material'; interface OperatorExecutionButtonProps extends ButtonProps { operatorUri: string; onSuccess?: ExecutionCallback; onError?: ExecutionErrorCallback; executionParams?: object; onOptionSelected?: (option: OperatorExecutionOption) => void; } export const OperatorExecutionButton: React.FC<OperatorExecutionButtonProps> = ({ operatorUri, onSuccess, onError, onClick, executionParams, onOptionSelected, disabled, children, ...props }) => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/packages/operators/src/components/OperatorExecutionButton/index.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/operators/src/components/OperatorExecutionButton/index.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (1)
app/packages/operators/src/components/OperatorExecutionButton/index.tsx (1)
40-46: Verify operator execution behavior with disabled state
The optimization to prevent mounting OperatorExecutionTrigger when disabled is a good architectural improvement. However, let's verify that no operator-related side effects depend on the trigger component being mounted.
✅ Verification successful
Early return for disabled state is safe to implement
The code review shows that the OperatorExecutionTrigger component already handles the disabled state appropriately:
- It prevents menu opening when disabled (
clickHandlersetsisMenuOpento false) - The menu's
openprop is controlled byisMenuOpen && !disabled - No side effects or state updates occur when the component is disabled
- All operator execution logic is tied to menu interactions, which are already prevented in disabled state
The early return optimization in OperatorExecutionButton is safe as it maintains the same behavior while improving performance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any dependencies on OperatorExecutionTrigger mounting
rg -A 5 "OperatorExecutionTrigger" --type ts --type tsx
# Search for any effects or state updates in the trigger component
ast-grep --pattern 'useEffect($_) { $$$ }' src/components/OperatorExecutionTrigger
Length of output: 255
Script:
#!/bin/bash
# Search for OperatorExecutionTrigger in the codebase
fd -e ts -e tsx -e js -e jsx | xargs rg -l "OperatorExecutionTrigger"
# Once we find the file, let's check its content and any effects
fd -e ts -e tsx -e js -e jsx | xargs rg "OperatorExecutionTrigger" -A 10
# Look for any useEffect hooks in files containing OperatorExecutionTrigger
fd -e ts -e tsx -e js -e jsx | xargs rg "OperatorExecutionTrigger" -l | xargs rg "useEffect"
Length of output: 5539
Script:
#!/bin/bash
# Let's check the content of OperatorExecutionTrigger component to understand its behavior
cat app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx
# Look for any state management or menu-related code in the trigger component
rg -A 10 "const \[.*\].*useState" app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx
# Check for any menu-related imports or components
rg "OperatorExecutionMenu" app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx -A 5
Length of output: 5044
| @@ -37,6 +37,14 @@ export const OperatorExecutionButton = ({ | |||
| disabled?: boolean; | |||
| children: React.ReactNode; | |||
| }) => { | |||
| if (disabled) { | |||
| return ( | |||
| <Button disabled {...props}> | |||
There was a problem hiding this comment.
would this change show the tooltip when disabled as well?
could cause an issue in DQ panel where we use disabled and title to show permission messaging.
operator_button = types.OperatorExecutionButtonView(
icon="expand_more",
operator=operator_mapping[issue_type],
on_success=self.computation_handler,
on_error=self.cancel_compute,
params={
"issue_type": issue_type,
},
on_option_selected=self._on_compute_option_selected,
label=button_string,
disabled=no_access or is_computing,
title=(NOT_PERMITTED_TEXT if no_access else ""),
componentsProps={
"container": {
"sx": {
"display": "flex",
"justifyContent": "center",
}
}
},
)
card_content.obj(
"execution_button",
view=operator_button,
)
we can wrap this Button with TitleProvider and pass the props to keep it backward compatible
There was a problem hiding this comment.
It does - note that this change is already in teams.
Summary by CodeRabbit
New Features
OperatorExecutionButtonto directly render a disabled state when the button is inactive, improving user experience by simplifying the button's behavior.Bug Fixes