add exec button to py panels #5119
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
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
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.tsx (3)
59-60: Fix typo in getVariant function parameter nameThere's a typo in the
getVariantfunction parameter name (prosinstead ofprops). While it works, it affects code readability.Apply this fix:
-function getVariant(pros: ViewPropsType) { - const variant = pros.schema.view.variant; +function getVariant(props: ViewPropsType) { + const variant = props.schema.view.variant;
87-90: Consider improvements to variant and color handling
- The variant condition could be simplified
- Consider using theme type safety for better maintainability
Here's a suggested improvement:
- if (isNullish(variant) || variant === "contained") { + if (!variant || variant === "contained") { baseProps.variant = "contained"; baseProps.color = "primary"; - baseProps.sx.color = (theme) => theme.palette.text.primary; + baseProps.sx.color = (theme: Theme) => theme.palette.text.primary; }Don't forget to import the Theme type:
import { Theme } from '@mui/material/styles';
Line range hint
13-24: Consider adding error boundary for operator executionWhile the component is well-structured, it might benefit from proper error handling for operator execution failures. This is particularly important as it's being used in py panels where Python execution could fail.
Consider wrapping the component with an error boundary to gracefully handle and display operator execution errors:
import { ErrorBoundary } from 'react-error-boundary'; function ErrorFallback({error}: {error: Error}) { return ( <Typography color="error"> Error executing operator: {error.message} </Typography> ); } // Usage <ErrorBoundary FallbackComponent={ErrorFallback}> <OperatorExecutionButtonView {...props} /> </ErrorBoundary>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.tsx(3 hunks)fiftyone/operators/types.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.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 (2)
app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.tsx (1)
13-13: LGTM: Props destructuring is clean and follows best practices
The removal of the onClick prop in favor of OperatorExecutionButton's built-in execution handling improves component encapsulation.
fiftyone/operators/types.py (1)
1373-1400: OperatorExecutionButtonView class is properly implemented
The OperatorExecutionButtonView class correctly extends the Button class and includes necessary validation for the operator parameter. The examples provided are clear and helpful.
| icon (str): an icon for the button. Defaults to "expand_more" if not provided. | ||
| label (str): a label for the button. | ||
| description (str): a description for the button. | ||
| title (str): a tooltip title for the button. | ||
| operator (str): the name of the operator to execute when the button is clicked. | ||
| params (dict): the parameters to pass to the operator. | ||
| prompt (str): a prompt for the operation. | ||
| disabled (bool): whether the button is disabled. |
There was a problem hiding this comment.
Correct parameter types and defaults in the docstring
There are discrepancies between the docstring and the method implementation:
- The
promptparameter is documented as(str)but should be(bool). - Default values mentioned in the docstring (e.g.,
icondefaults to"expand_more") are not reflected in the code.
Suggested fix:
Update the docstring to accurately reflect parameter types and default values.
Args:
icon (str, optional): an icon for the button. Defaults to "expand_more".
label (str, optional): a label for the button.
description (str, optional): a description for the button.
title (str, optional): a tooltip title for the button.
operator (str): the name of the operator to execute when the button is clicked.
params (dict, optional): the parameters to pass to the operator.
- prompt (str): a prompt for the operation.
+ prompt (bool, optional): whether to prompt the user before executing the operator.
disabled (bool, optional): whether the button is disabled.Additionally, align the default values in the code with those specified in the docstring.
Committable suggestion skipped: line range outside the PR's diff.
| def __init__(self, **kwargs): | ||
| if "operator" not in kwargs or not isinstance(kwargs["operator"], str): | ||
| raise ValueError( | ||
| "The 'operator' parameter of type str is required." | ||
| ) | ||
| super().__init__(**kwargs) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define explicit __init__ parameters for clarity and type safety
Explicitly defining parameters in the __init__ method enhances readability and allows for better type checking. It makes the expected inputs clear to users and improves maintainability. Consider replacing **kwargs with explicit parameters.
Proposed change:
-def __init__(self, **kwargs):
+def __init__(
+ self,
+ operator: str,
+ icon: str = "expand_more",
+ label: Optional[str] = None,
+ description: Optional[str] = None,
+ title: Optional[str] = None,
+ params: Optional[dict] = None,
+ prompt: bool = False,
+ disabled: bool = False,
+ **kwargs,
+):
if not isinstance(operator, str):
raise ValueError("The 'operator' parameter of type str is required.")
- super().__init__(**kwargs)
+ super().__init__(
+ operator=operator,
+ icon=icon,
+ label=label,
+ description=description,
+ title=title,
+ params=params,
+ prompt=prompt,
+ disabled=disabled,
+ **kwargs,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __init__(self, **kwargs): | |
| if "operator" not in kwargs or not isinstance(kwargs["operator"], str): | |
| raise ValueError( | |
| "The 'operator' parameter of type str is required." | |
| ) | |
| super().__init__(**kwargs) | |
| def __init__( | |
| self, | |
| operator: str, | |
| icon: str = "expand_more", | |
| label: Optional[str] = None, | |
| description: Optional[str] = None, | |
| title: Optional[str] = None, | |
| params: Optional[dict] = None, | |
| prompt: bool = False, | |
| disabled: bool = False, | |
| **kwargs, | |
| ): | |
| if not isinstance(operator, str): | |
| raise ValueError("The 'operator' parameter of type str is required.") | |
| super().__init__( | |
| operator=operator, | |
| icon=icon, | |
| label=label, | |
| description=description, | |
| title=title, | |
| params=params, | |
| prompt=prompt, | |
| disabled=disabled, | |
| **kwargs, | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app/packages/operators/src/types.ts (1)
1244-1246: Enhance JSDoc documentation for better clarity.The current documentation is minimal. Consider adding more details about the purpose, usage, and parameters of this view.
/** - * Operator class for rendering a execution button. + * Operator class for rendering an execution button in the UI. + * This view can be used to create interactive buttons that trigger operator execution. + * + * @example + * ```typescript + * const view = new OperatorExecutionButtonView({ + * label: "Execute Operation", + * description: "Click to run the operation" + * }); + * ``` */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/packages/operators/src/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/packages/operators/src/types.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
| class OperatorExecutionButtonView extends View { | ||
| constructor(options: ViewProps) { | ||
| super(options); | ||
| this.name = "OperatorExecutionButtonView"; | ||
| } | ||
| static fromJSON(json) { | ||
| return new OperatorExecutionButtonView(json); | ||
| } | ||
| } |
There was a problem hiding this comment.
Export the OperatorExecutionButtonView class.
The class should be exported to make it available for use in other modules, following the pattern of other view classes in this file.
-class OperatorExecutionButtonView extends View {
+export class OperatorExecutionButtonView extends View {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class OperatorExecutionButtonView extends View { | |
| constructor(options: ViewProps) { | |
| super(options); | |
| this.name = "OperatorExecutionButtonView"; | |
| } | |
| static fromJSON(json) { | |
| return new OperatorExecutionButtonView(json); | |
| } | |
| } | |
| export class OperatorExecutionButtonView extends View { | |
| constructor(options: ViewProps) { | |
| super(options); | |
| this.name = "OperatorExecutionButtonView"; | |
| } | |
| static fromJSON(json) { | |
| return new OperatorExecutionButtonView(json); | |
| } | |
| } |
| return new OperatorExecutionButtonView(json); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Add OperatorExecutionButtonView to VIEWS constant.
The new view class needs to be added to the VIEWS constant to support proper JSON deserialization through the viewFromJSON function.
const VIEWS = {
View,
InferredView,
Form,
ReadOnlyView,
// ... other views
PillBadgeView,
ModalView,
ToastView,
+ OperatorExecutionButtonView,
};Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
OneOfView,ListView,TupleView,TreeSelectionView,FrameLoaderView, andTimelineView.