Merge release/v1.1.0 to develop
#5117
Conversation
WalkthroughThe changes in this pull request primarily focus on the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (2)fiftyone/operators/builtin.py (2)
The changes correctly implement the target parameter handling, allowing the clear operation to be performed on different views (dataset, current view, or selected samples) while maintaining consistency with other operations.
The changes mirror the implementation in 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
|
Feat/operator exec ctx menu
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (25)
app/packages/core/src/plugins/SchemaIO/components/StatusButtonView.tsx (2)
7-10: Add props validation and safe defaultsWhile the current implementation works, it could be more robust with proper props validation and safe defaults.
Consider this improvement:
export default function StatusButtonView(props: ViewPropsType) { const { schema } = props; - const { view = {} } = schema; + const { view = {} } = schema ?? {}; const { label, on_click, params = {}, severity, disabled, title } = view;Also consider adding runtime validation for required props:
if (!label || !on_click) { console.warn('StatusButtonView: label and on_click are required props'); return null; }
20-28: Enhance accessibility and loading state handlingThe button rendering could be improved with proper accessibility attributes and loading state indication.
Consider these improvements:
<StatusButton label={label} onClick={handleClick} severity={severity} - disabled={disabled} + disabled={disabled || isLoading} + loading={isLoading} title={title} + aria-label={title || label} />app/packages/operators/src/components/OperatorExecutionMenu/index.tsx (3)
5-14: Update JSDoc to match component propsThe documentation has a few inconsistencies:
- Missing documentation for the
onOptionClickprop- The
onClickparameter description should be renamed toonOptionClick/** * Component which provides a context menu for executing an operator using a * specified execution target. * * @param anchor Element to use as context menu anchor * @param open If true, context menu will be visible * @param onClose Callback for context menu close events * @param executionOptions List of operator execution options - * @param onClick Callback for an option being clicked + * @param onOptionClick Callback for when an execution option is selected */
15-27: Consider extracting props interfaceFor better reusability and maintainability, consider extracting the props interface into a separate type definition.
+interface OperatorExecutionMenuProps { + anchor?: Element | null; + open: boolean; + onClose: () => void; + executionOptions: OperatorExecutionOption[]; + onOptionClick?: (option: OperatorExecutionOption) => void; +} -export const OperatorExecutionMenu = ({ - anchor, - open, - onClose, - executionOptions, - onOptionClick, -}: { - anchor?: Element | null; - open: boolean; - onClose: () => void; - executionOptions: OperatorExecutionOption[]; - onOptionClick?: (option: OperatorExecutionOption) => void; -}) => { +export const OperatorExecutionMenu = ({ + anchor, + open, + onClose, + executionOptions, + onOptionClick, +}: OperatorExecutionMenuProps) => {
50-51: Consider removing default exportHaving both named and default exports can lead to inconsistent import patterns across the codebase. Consider sticking with named exports only, as they are more explicit and better for tree-shaking.
-export default OperatorExecutionMenu;app/packages/operators/src/components/OperatorExecutionButton/index.tsx (4)
1-8: Consider reordering imports following React/TypeScript conventionsFor better maintainability, consider reordering imports following common conventions:
-import { Button } from "@mui/material"; -import { OperatorExecutionTrigger } from "../OperatorExecutionTrigger"; -import React from "react"; -import { - ExecutionCallback, - ExecutionErrorCallback, -} from "../../types-internal"; -import { OperatorExecutionOption } from "../../state"; +import React from "react"; +import { Button } from "@mui/material"; + +import { OperatorExecutionTrigger } from "../OperatorExecutionTrigger"; +import { OperatorExecutionOption } from "../../state"; +import { + ExecutionCallback, + ExecutionErrorCallback, +} from "../../types-internal";
10-19: Enhance JSDoc documentation with @returns and @exampleThe documentation is good but could be more comprehensive.
Consider adding:
/** * Button which acts as a trigger for opening an `OperatorExecutionMenu`. * * @param operatorUri Operator URI * @param onSuccess Callback for successful operator execution * @param onError Callback for operator execution error * @param executionParams Parameters to provide to the operator's execute call * @param onOptionSelected Callback for execution option selection * @param disabled If true, disables the button and context menu * @returns A button component that triggers the execution menu * @example * <OperatorExecutionButton * operatorUri="my-operator" * onSuccess={() => console.log("Success")} * disabled={false} * > * Execute * </OperatorExecutionButton> */
20-37: Extract props interface and add runtime validationConsider improving type safety and maintainability.
+import PropTypes from 'prop-types'; + +interface OperatorExecutionButtonProps extends React.ButtonHTMLAttributes<HTMLButtonElement> { + operatorUri: string; + onSuccess?: ExecutionCallback; + onError?: ExecutionErrorCallback; + executionParams?: object; + onOptionSelected?: (option: OperatorExecutionOption) => void; + disabled?: boolean; + children: React.ReactNode; +} -export const OperatorExecutionButton = ({ +export const OperatorExecutionButton: React.FC<OperatorExecutionButtonProps> = ({ // Add at the bottom of the file: +OperatorExecutionButton.propTypes = { + operatorUri: PropTypes.string.isRequired, + onSuccess: PropTypes.func, + onError: PropTypes.func, + executionParams: PropTypes.object, + onOptionSelected: PropTypes.func, + disabled: PropTypes.bool, + children: PropTypes.node.isRequired, +};
54-54: Consider using only named exportsHaving both named and default exports can lead to inconsistent import patterns across the codebase.
-export default OperatorExecutionButton;Use named exports consistently throughout the application:
import { OperatorExecutionButton } from './OperatorExecutionButton';app/packages/components/src/components/StatusButton/StatusButton.tsx (5)
1-14: Consider optimizing imports for better maintainability and bundle size.A few suggestions to improve the imports:
- The relative import
../foruseThemeis ambiguous. Consider using an absolute path.- Lodash import could be more specific:
import get from 'lodash/get'
15-21: Consider removing the !important flag from borderColor.The use of
!importantinborderColormight indicate a specificity issue. Consider restructuring the styles to avoid this by:
- Increasing selector specificity
- Adjusting the Material-UI theme configuration
34-40: Add JSDoc documentation and consider error handling.The
StatusButtonPropsinterface would benefit from:
- JSDoc documentation explaining the purpose of each prop
- Error handling for the onClick callback
Example documentation:
/** * Props for the StatusButton component * @property {string} [label="Disabled"] - The text to display in the button * @property {boolean} [disabled=false] - Whether the button is disabled * @property {() => void} onClick - Callback function when the settings icon is clicked * @property {Severity} severity - The severity level affecting the button's color * @property {string} [title] - Tooltip text to display on hover */
53-55: Remove unnecessary handleClick wrapper function.The
handleClickfunction can be removed andonClickcan be used directly in the Button props since it doesn't add any additional functionality.- const handleClick = () => { - onClick(); - };And update the Button props:
- onClick={handleClick} + onClick={onClick}
87-97: Consider using theme variables for consistent styling.The button styles contain hardcoded values that could be derived from the theme for better consistency:
- minWidth: 32
- opacity: 0.5
Consider updating to:
sx={{ - minWidth: 32, - opacity: disabled ? 0.5 : 1, + minWidth: theme.spacing(4), + opacity: disabled ? theme.palette.action.disabledOpacity : 1, outline: "none", border: "none !important", "&:hover": { border: "none", outline: "none", background: theme.action.hover, }, }}app/packages/embeddings/src/Embeddings.tsx (4)
Line range hint
29-29: Add TypeScript type definitions for component propsThe
Embeddingscomponent propscontainerHeightanddimensionslack TypeScript type definitions, which reduces type safety and IDE support.-export default function Embeddings({ containerHeight, dimensions }) { +interface EmbeddingsDimensions { + bounds: { + width: number; + height: number; + }; +} + +interface EmbeddingsProps { + containerHeight: number; + dimensions: EmbeddingsDimensions; +} + +export default function Embeddings({ containerHeight, dimensions }: EmbeddingsProps) {
Line range hint
30-52: Consider decomposing complex state management logicThe component contains multiple state management hooks and complex logic. Consider extracting the state management into a custom hook for better maintainability and reusability.
+// Create a new file: useEmbeddingsState.ts +export function useEmbeddingsState() { + const el = useRef(); + const theme = useTheme(); + const resetZoom = useResetPlotZoom(); + const brainResultSelector = useBrainResultsSelector(); + const labelSelector = useLabelSelector(); + const plotSelection = usePlotSelection(); + const [dragMode, setDragMode] = usePanelStatePartial("dragMode", "lasso", true); + + useEffect(() => { + setPanelCloseEffect(() => { + plotSelection.clearSelection(); + }); + }, [setPanelCloseEffect, plotSelection]); + + return { + el, + theme, + resetZoom, + brainResultSelector, + labelSelector, + plotSelection, + dragMode, + setDragMode, + }; +}
Line range hint
89-146: Enhance accessibility for interactive elementsWhile tooltips are provided, the interactive elements could benefit from additional accessibility improvements:
- Add ARIA labels for icons
- Ensure proper keyboard navigation
- Add role attributes where appropriate
<PlotOption cy="embeddings-plot-option-lasso" style={{ opacity: dragMode !== "lasso" ? 0.5 : 1 }} to={() => setDragMode("lasso")} title={"Select (s)"} + role="button" + aria-label="Enable lasso selection mode" > <HighlightAlt /> </PlotOption>
Line range hint
65-88: Consider performance optimizationsThe component handles complex rendering logic and multiple state updates. Consider these optimizations:
- Memoize callback functions with useCallback
- Memoize complex computations with useMemo
- Use React.memo for child components
+const MemoizedSelector = React.memo(Selector); + {brainResultSelector.hasSelection && !labelSelector.isLoading && ( - <Selector + <MemoizedSelector cy="embeddings-colorby" {...labelSelector.handlers} placeholder={"Color by"}app/packages/components/src/components/ThemeProvider/index.tsx (1)
Line range hint
1-299: Consider adding type definitions for custom theme propertiesThe theme configuration could benefit from TypeScript type definitions for custom properties. This would provide better type safety and autocompletion support when using custom colors like
primaryMedium.Example implementation:
declare module '@mui/material/styles' { interface CustomPalette { shadow: string; shadowDark: string; lightning: string; toastBackgroundColor: string; primarySoft: string; primaryMedium: string; } interface Palette { custom: CustomPalette; } }app/packages/operators/src/types.ts (1)
1071-1082: Documentation could be enhanced with more details.While the class implementation follows the established pattern, the documentation could be more descriptive by including:
- The purpose and use cases of the status button
- Available options/props and their effects
- Example usage
Consider expanding the documentation like this:
/** * Operator class for rendering a status button. + * + * This view renders a button that can display different states through its appearance. + * Useful for showing operation status, health indicators, or action states. + * + * @example + * ```typescript + * new StatusButtonView({ + * label: "Service Status", + * severity: "warning", + * onClick: () => {}, + * }) + * ``` */fiftyone/operators/types.py (1)
2203-2213: Add usage examples to the docstringConsider adding usage examples to demonstrate how to create and configure a StatusButtonView, similar to other view classes in this file. This would help developers understand how to properly use this component.
Example to add:
# Example usage status_btn = types.StatusButtonView( severity="success", title="Ready", on_click=self.handle_click, params={"action": "start"}, disabled=False ) panel.view("status", status_btn)app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx (1)
15-39: Document thechildrenprop in the component's JSDoc comment.The
childrenprop is required but is not documented in the JSDoc. Adding it will improve clarity for other developers using this component.Apply this diff to add the documentation:
/** * Component which acts as a trigger for opening an `OperatorExecutionMenu`. * * This component is meant to act as a wrapper around the interactable * component. For example, if you wanted to add operator execution to a button, * * ```tsx * <OperatorExecutionTrigger {...props}> * <Button>Execute operator</Button> * </OperatorExecutionTrigger> * ``` * * * This component registers a click handler which will manage the * `OperatorExecutionMenu` lifecycle. * * @param operatorUri Operator URI * @param onClick Callback for click events * @param onSuccess Callback for successful operator execution * @param onError Callback for operator execution error * @param executionParams Parameters to provide to the operator's execute call * @param executorOptions Operator executor options * @param onOptionSelected Callback for execution option selection * @param disabled If true, context menu will never open + * @param children The interactable component that triggers the execution menu */app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.tsx (2)
61-110: Refactor repetitive style assignments ingetButtonPropsThe
getButtonPropsfunction contains multipleifstatements that check thevariantvalue to assign styles. Consider refactoring this logic using aswitchstatement or a mapping object to improve readability and maintainability.
116-136: Optimize handling of disabled styles ingetCommonPropsThe styles for the disabled state are conditionally applied within the
sxprop. Since MUI components handle the disabled state internally, you might simplify the style overrides by leveraging MUI's built-in disabled styling. This can reduce complexity and potential styling inconsistencies.app/packages/operators/src/state.ts (1)
Line range hint
247-279: Remove hardcoded 'FOR TESTING' tag in execution optionsThe
tagproperty is set to"FOR TESTING", which appears to be a placeholder or debugging value. Consider removing it or replacing it with an appropriate value for production.Apply this diff to fix the issue:
"Execute", id: "execute", - tag: "FOR TESTING", default: defaultToExecute, description: "Run this operation synchronously. Only suitable for small datasets",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (17)
app/packages/components/src/components/StatusButton/StatusButton.tsx(1 hunks)app/packages/components/src/components/StatusButton/index.ts(1 hunks)app/packages/components/src/components/ThemeProvider/index.tsx(1 hunks)app/packages/components/src/components/index.ts(1 hunks)app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/StatusButtonView.tsx(1 hunks)app/packages/core/src/plugins/SchemaIO/components/index.ts(1 hunks)app/packages/core/src/plugins/SchemaIO/utils/index.ts(1 hunks)app/packages/embeddings/src/Embeddings.tsx(1 hunks)app/packages/operators/src/components/OperatorExecutionButton/index.tsx(1 hunks)app/packages/operators/src/components/OperatorExecutionMenu/index.tsx(1 hunks)app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx(1 hunks)app/packages/operators/src/index.ts(1 hunks)app/packages/operators/src/operators.ts(4 hunks)app/packages/operators/src/state.ts(8 hunks)app/packages/operators/src/types.ts(2 hunks)fiftyone/operators/types.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/packages/components/src/components/StatusButton/index.ts
🧰 Additional context used
📓 Path-based instructions (15)
app/packages/components/src/components/StatusButton/StatusButton.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.
app/packages/components/src/components/ThemeProvider/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.
app/packages/components/src/components/index.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.
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.
app/packages/core/src/plugins/SchemaIO/components/StatusButtonView.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.
app/packages/core/src/plugins/SchemaIO/components/index.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.
app/packages/core/src/plugins/SchemaIO/utils/index.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.
app/packages/embeddings/src/Embeddings.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.
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.
app/packages/operators/src/components/OperatorExecutionMenu/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.
app/packages/operators/src/components/OperatorExecutionTrigger/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.
app/packages/operators/src/index.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.
app/packages/operators/src/operators.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.
app/packages/operators/src/state.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.
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.
🔇 Additional comments (21)
app/packages/core/src/plugins/SchemaIO/components/StatusButtonView.tsx (1)
1-5: LGTM! Imports are well-organized and necessary.
The imports follow a logical order and all imported modules are utilized in the component.
app/packages/operators/src/index.ts (1)
7-7: LGTM! Export follows consistent patterns
The new export for OperatorExecutionButton is well-placed, maintains alphabetical ordering, and follows the established export patterns in the file.
app/packages/operators/src/components/OperatorExecutionMenu/index.tsx (2)
1-4: LGTM: Clean and specific imports
The imports are well-organized, importing only the necessary components and types.
28-48: LGTM: Well-structured menu rendering logic
The menu rendering logic is well-implemented with proper event handling and UI structure. However, be aware that executing multiple callbacks in the onClick handler could potentially lead to race conditions if any of these operations are asynchronous.
app/packages/components/src/components/index.ts (1)
35-35: LGTM! Export follows established patterns.
The new StatusButton export follows the consistent pattern used throughout the file and is properly alphabetically ordered.
Let's verify the StatusButton component implementation:
✅ Verification successful
Verified: StatusButton component exists and follows React patterns
The component is properly implemented in app/packages/components/src/components/StatusButton/StatusButton.tsx with:
- Correct TypeScript/React function component structure
- Proper props typing with
StatusButtonPropsinterface - Standard React hooks usage (
useTheme) - Consistent Material-UI component composition
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify StatusButton component exists and follows React/TypeScript patterns
# Check if StatusButton component and its types exist
fd -e tsx -e ts "StatusButton" app/packages/components/src/components/StatusButton
# Verify the component implementation follows React patterns
ast-grep --pattern 'export default function StatusButton($_) {
$$$
}' app/packages/components/src/components/StatusButton
Length of output: 6437
app/packages/core/src/plugins/SchemaIO/utils/index.ts (2)
115-115: LGTM! Addition of StatusButtonView to non-editable views is appropriate.
The StatusButtonView fits well with other non-editable view types like ButtonView and AlertView, maintaining consistency in how status-related UI components are handled.
115-115: Verify StatusButtonView component implementation.
Let's ensure the StatusButtonView component is properly defined and used consistently.
✅ Verification successful
Let me gather more information about the StatusButtonView implementation and its usage.
StatusButtonView component is properly implemented and integrated
The component is well-defined with:
- Proper implementation in
StatusButtonView.tsxusing React and required dependencies - Correct export in
components/index.ts - Appropriate classification as a non-editable view in
utils/index.ts
The implementation includes proper handling of:
- Event triggering through
usePanelEvent - Panel identification via
usePanelId - Required props and view configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify StatusButtonView component existence and usage
# Check if StatusButtonView component exists
echo "Checking for StatusButtonView component definition..."
ast-grep --pattern 'export class StatusButtonView' || ast-grep --pattern 'class StatusButtonView'
# Check for StatusButtonView imports and usage
echo "Checking for StatusButtonView usage..."
rg -l "StatusButtonView"
# Check for view component registration
echo "Checking if StatusButtonView is properly registered..."
ast-grep --pattern 'import { StatusButtonView }'
Length of output: 699
Script:
#!/bin/bash
# Check the actual implementation of StatusButtonView
echo "Checking StatusButtonView implementation..."
cat app/packages/core/src/plugins/SchemaIO/components/StatusButtonView.tsx
# Check how it's exported and used in components/index.ts
echo -e "\nChecking component exports..."
cat app/packages/core/src/plugins/SchemaIO/components/index.ts
# Check the usage context in utils/index.ts
echo -e "\nChecking usage in utils/index.ts..."
cat app/packages/core/src/plugins/SchemaIO/utils/index.ts
Length of output: 7612
app/packages/core/src/plugins/SchemaIO/components/index.ts (1)
41-41: LGTM! New exports follow established patterns
The new component exports maintain consistency with the codebase by:
- Following the established
*Viewnaming convention - Preserving alphabetical ordering
- Using correct TypeScript export syntax
Also applies to: 49-49
app/packages/embeddings/src/Embeddings.tsx (1)
3-3: Verify loading state handling after Loading component removal
The removal of the Loading import suggests a change in loading state management, but labelSelector.isLoading is still being used. Ensure that loading states are properly handled throughout the component.
app/packages/components/src/components/ThemeProvider/index.tsx (1)
Line range hint 284-299: Implementation follows React best practices
The ThemeProvider component and associated hooks are well-implemented with:
- Proper TypeScript types
- Clean separation of concerns
- Efficient state management using Recoil
- Proper error handling for theme state
app/packages/operators/src/types.ts (1)
1308-1308: LGTM!
The StatusButtonView is correctly added to the VIEWS constant, maintaining consistency with other view types.
fiftyone/operators/types.py (1)
2214-2218: LGTM!
The implementation follows the established patterns for view classes in this module and properly inherits from the base View class.
app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.tsx (1)
12-59: Component implementation follows best practices
The OperatorExecutionButtonView component is well-structured and correctly utilizes React and Material-UI components. Props are properly destructured, and default values are appropriately assigned. The use of utility functions enhances readability.
app/packages/operators/src/state.ts (5)
228-243: Type definition for OperatorExecutionOption is correctly defined
321-321: Option for scheduling on specific orchestrators is correctly implemented
333-341: Default selection logic in execution options is implemented correctly
349-355: Selected option handling is correctly implemented
385-410: Hook useOperatorExecutionOptions is well-defined
app/packages/operators/src/operators.ts (3)
911-911: Confirm query_performance handling in execution options resolution
The inclusion of query_performance: currentContext.queryPerformance in the /operators/resolve-execution-options request enhances performance tracking during execution options resolution. Please verify that the server-side endpoint accommodates this parameter correctly.
Execute the following script to validate the server implementation:
✅ Verification successful
Based on the search results, I can now provide a definitive response about the handling of query_performance in the execution options resolution.
The query_performance parameter is properly handled in the execution context
The codebase shows that query_performance is:
- Correctly passed from the client in
app/packages/operators/src/operators.tsas part of the execution options request - Properly handled in the server-side through the
ExecutionContextclass infiftyone/operators/executor.pywhich exposes it via thequery_performanceproperty - Accessible to operators through the context parameter in
resolve_execution_options()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Confirm that 'resolve_execution_options' function handles 'query_performance' parameter.
# Search for 'resolve_execution_options' function definitions that include 'query_performance' in their parameters.
ast-grep --pattern 'def resolve_execution_options($_, query_performance, $$):'
Length of output: 218
Script:
#!/bin/bash
# Let's search for resolve-execution-options endpoint handling in server-side code
rg -i "resolve.?execution.?options" -A 5
# Also search for query_performance parameter handling
rg -i "query.?performance" -A 3
Length of output: 87122
562-562: Verify server handling of the new query_performance parameter
The addition of query_performance: currentContext.queryPerformance ensures that performance tracking is included in the request body sent to /operators/execute/generator. Please verify that the server-side endpoint correctly accepts and processes the query_performance parameter.
Run the following script to confirm the server implementation:
✅ Verification successful
Server properly handles the query_performance parameter
Based on the codebase analysis, the server-side implementation correctly handles the query_performance parameter:
- The parameter is properly accessed through
request_paramsin the operator executor:fiftyone/operators/executor.pyshowsquery_performanceis retrieved from request parameters - The execution flow through
ExecuteOperatorAsGeneratorendpoint properly passes all request parameters - The parameter is integrated into the core configuration system (
fiftyone/core/config.py) with bothenable_query_performanceanddefault_query_performancesettings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the server-side 'execute_generator' function handles 'query_performance' parameter.
# Search for 'execute_generator' function definitions that include 'query_performance' in their parameters.
ast-grep --pattern 'def execute_generator($_, query_performance, $$):'
Length of output: 72
Script:
#!/bin/bash
# Let's try different approaches to find the server-side implementation
# Search for Python files containing 'execute_generator'
echo "=== Python files with execute_generator ==="
rg -l "execute_generator" --type py
# Search for the actual implementation with context
echo -e "\n=== Implementation of execute_generator ==="
rg "def execute_generator" -A 10 --type py
# Search for query_performance usage in Python files
echo -e "\n=== Usage of query_performance ==="
rg "query_performance" --type py
Length of output: 1335
Script:
#!/bin/bash
# Let's search for the operator execution endpoint and its handling
# Search for operator execution routes/endpoints
echo "=== Operator execution routes ==="
rg -B 5 -A 10 "/operators/execute" --type py
# Search for the Generator class or related implementation
echo -e "\n=== Generator class implementation ==="
rg "class.*Generator" -A 10 --type py
# Look for operator execution in executor.py with more context
echo -e "\n=== Operator execution implementation ==="
rg -B 5 -A 15 "execute.*operator" fiftyone/operators/executor.py
Length of output: 11547
945-945: Verify inclusion of query_performance in remote placements fetching
By adding query_performance: currentContext.queryPerformance, performance tracking is now part of the /operators/resolve-placements request. Ensure that the server correctly processes this parameter when fetching remote placements.
Run the following script to check the server-side handling:
| const panelId = usePanelId(); | ||
| const triggerEvent = usePanelEvent(); | ||
| const handleClick = () => { | ||
| triggerEvent(panelId, { | ||
| operator: on_click, | ||
| params, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and loading state management
The event handling could be more robust with proper error handling and loading state management.
Consider these improvements:
export default function StatusButtonView(props: ViewPropsType) {
const { schema } = props;
const { view = {} } = schema ?? {};
const { label, on_click, params = {}, severity, disabled, title } = view;
const panelId = usePanelId();
const triggerEvent = usePanelEvent();
+ const [isLoading, setIsLoading] = React.useState(false);
+
const handleClick = () => {
+ if (!panelId) {
+ console.error('StatusButtonView: No panel ID available');
+ return;
+ }
+ setIsLoading(true);
triggerEvent(panelId, {
operator: on_click,
params,
- });
+ }).catch(error => {
+ console.error('Error triggering event:', error);
+ }).finally(() => {
+ setIsLoading(false);
+ });
};Committable suggestion skipped: line range outside the PR's diff.
| return ( | ||
| <OperatorExecutionTrigger | ||
| operatorUri={operatorUri} | ||
| onSuccess={onSuccess} | ||
| onError={onError} | ||
| executionParams={executionParams} | ||
| onOptionSelected={onOptionSelected} | ||
| disabled={disabled} | ||
| > | ||
| <Button disabled={disabled} {...props}> | ||
| {children} | ||
| </Button> | ||
| </OperatorExecutionTrigger> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance component with memoization and accessibility
Consider the following improvements for better performance and accessibility:
-export const OperatorExecutionButton = ({
+export const OperatorExecutionButton = React.memo(({
// ... props
}) => {
return (
<OperatorExecutionTrigger
operatorUri={operatorUri}
onSuccess={onSuccess}
onError={onError}
executionParams={executionParams}
onOptionSelected={onOptionSelected}
disabled={disabled}
>
- <Button disabled={disabled} {...props}>
+ <Button
+ disabled={disabled}
+ aria-label={`Execute operator ${operatorUri}`}
+ {...props}
+ >
{children}
</Button>
</OperatorExecutionTrigger>
);
-};
+});
+OperatorExecutionButton.displayName = 'OperatorExecutionButton';📝 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.
| return ( | |
| <OperatorExecutionTrigger | |
| operatorUri={operatorUri} | |
| onSuccess={onSuccess} | |
| onError={onError} | |
| executionParams={executionParams} | |
| onOptionSelected={onOptionSelected} | |
| disabled={disabled} | |
| > | |
| <Button disabled={disabled} {...props}> | |
| {children} | |
| </Button> | |
| </OperatorExecutionTrigger> | |
| ); | |
| export const OperatorExecutionButton = React.memo(({ | |
| operatorUri, | |
| onSuccess, | |
| onError, | |
| executionParams, | |
| onOptionSelected, | |
| disabled, | |
| children, | |
| ...props | |
| }) => { | |
| return ( | |
| <OperatorExecutionTrigger | |
| operatorUri={operatorUri} | |
| onSuccess={onSuccess} | |
| onError={onError} | |
| executionParams={executionParams} | |
| onOptionSelected={onOptionSelected} | |
| disabled={disabled} | |
| > | |
| <Button | |
| disabled={disabled} | |
| aria-label={`Execute operator ${operatorUri}`} | |
| {...props} | |
| > | |
| {children} | |
| </Button> | |
| </OperatorExecutionTrigger> | |
| ); | |
| }); | |
| OperatorExecutionButton.displayName = 'OperatorExecutionButton'; |
| justifyContent="center" | ||
| padding="0 8px 0 16px" | ||
| sx={{ | ||
| borderRight: `1px solid ${theme.divider}`, |
There was a problem hiding this comment.
Fix incorrect theme property access.
The theme.divider access appears incorrect. It should likely be theme.palette.divider.
- borderRight: `1px solid ${theme.divider}`,
+ borderRight: `1px solid ${theme.palette.divider}`,📝 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.
| borderRight: `1px solid ${theme.divider}`, | |
| borderRight: `1px solid ${theme.palette.divider}`, |
| return ( | ||
| <Tooltip title={title}> | ||
| <ButtonGroup variant="outlined" sx={{ ...ButtonStylesOverrides }}> | ||
| <Box | ||
| display="flex" | ||
| alignItems="center" | ||
| justifyContent="center" | ||
| padding="0 8px 0 16px" | ||
| sx={{ | ||
| borderRight: `1px solid ${theme.divider}`, | ||
| }} | ||
| > | ||
| <CircleIcon | ||
| sx={{ width: "12px", height: "12px", color: severityColor, mr: 1 }} | ||
| /> | ||
| <Typography | ||
| variant="body1" | ||
| sx={{ | ||
| color: severityColor, | ||
| textTransform: "none", | ||
| mr: "8px", | ||
| }} | ||
| > | ||
| {label} | ||
| </Typography> | ||
| </Box> | ||
| <Button | ||
| disableRipple | ||
| disabled={disabled} | ||
| onClick={handleClick} | ||
| sx={{ | ||
| minWidth: 32, | ||
| opacity: disabled ? 0.5 : 1, | ||
| outline: "none", | ||
| border: "none !important", // overrides disabled border | ||
| "&:hover": { | ||
| border: "none", | ||
| outline: "none", | ||
| background: theme.action.hover, | ||
| }, | ||
| }} | ||
| > | ||
| <SettingsIcon sx={{ width: "20px", color: theme.secondary.main }} /> | ||
| </Button> | ||
| </ButtonGroup> | ||
| </Tooltip> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility support.
Consider adding the following accessibility improvements:
- Add aria-label to the settings button
- Include role="status" for the status indicator
- Ensure keyboard navigation works correctly with the button group
<Button
disableRipple
disabled={disabled}
onClick={onClick}
+ aria-label={`Configure ${label}`}
sx={{...}}
>Committable suggestion skipped: line range outside the PR's diff.
| @@ -180,6 +180,7 @@ let theme = extendMuiTheme({ | |||
| lightning: "#f5b700", | |||
| toastBackgroundColor: "#333", | |||
| primarySoft: "hsl(25, 100%, 80%)", | |||
| primaryMedium: "hsl(25, 100%, 71%)", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add primaryMedium to light theme for consistency
The new primaryMedium color is only defined in the dark theme. To maintain theme consistency and prevent potential styling issues when switching themes, consider adding a corresponding color definition to the light theme's custom palette.
| </Box> | ||
|
|
||
| <OperatorExecutionMenu | ||
| anchor={containerRef.current} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential null value of containerRef.current.
containerRef.current could be null on initial render, which may cause issues when passed as the anchor prop to OperatorExecutionMenu. Ensure that the component gracefully handles a null anchor.
Apply this diff to provide a fallback:
<OperatorExecutionMenu
- anchor={containerRef.current}
+ anchor={containerRef.current || undefined}
open={isMenuOpen && !disabled}
onClose={() => setIsMenuOpen(false)}
onOptionClick={onOptionSelected}
executionOptions={executionOptions}
/>Alternatively, modify OperatorExecutionMenu to handle a null or undefined anchor appropriately.
📝 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.
| anchor={containerRef.current} | |
| anchor={containerRef.current || undefined} |
|
|
||
| const defaultVariant = ["contained", "outlined"]; | ||
|
|
||
| function getVariant(pros: ViewPropsType) { |
There was a problem hiding this comment.
Fix typo in function parameter name 'pros'
The parameter name pros in the getVariant function is likely a typo. It should be props for consistency and to prevent any potential runtime errors.
Apply this diff to correct the typo:
-function getVariant(pros: ViewPropsType) {
+function getVariant(props: ViewPropsType) {📝 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.
| function getVariant(pros: ViewPropsType) { | |
| function getVariant(props: ViewPropsType) { |
| callback?.(new OperatorResult(operator, null, ctx.executor, e, false), { | ||
| ctx, | ||
| }); |
There was a problem hiding this comment.
Ensure ctx.executor is defined before usage
In the error handling callback, ctx.executor may be undefined if not properly initialized. Verify that ctx.executor is assigned before being used in OperatorResult.
| @@ -485,6 +542,7 @@ | |||
| return; | |||
| } | |||
| executor.execute(promptingOperator.params, { | |||
| ...options, | |||
There was a problem hiding this comment.
Ordering of spread operators may override intended options
In the call to executor.execute, the options passed may unintentionally override each other due to the order of the spread operators. If options should take precedence over promptingOperator.options, consider reversing the order.
Apply this diff to fix the issue:
executor.execute(promptingOperator.params, {
- ...options,
- ...promptingOperator.options,
+ ...promptingOperator.options,
+ ...options,
});Committable suggestion skipped: line range outside the PR's diff.
| @@ -830,6 +831,7 @@ export async function resolveRemoteType( | |||
| view: currentContext.view, | |||
| view_name: currentContext.viewName, | |||
| group_slice: currentContext.groupSlice, | |||
| query_performance: currentContext.queryPerformance, | |||
There was a problem hiding this comment.
💡 Codebase verification
Backend does not currently handle query_performance parameter in type resolution
Based on the examination of the server-side implementation in fiftyone/operators/server.py, the ResolveType endpoint handler does not currently process the query_performance parameter. While the parameter is being sent from the frontend, the server implementation needs to be updated to handle this new parameter for performance tracking.
- The
ResolveType.post()method infiftyone/operators/server.pyonly processesdataset_nameandoperator_urifrom the request data - While
query_performanceconfiguration exists in the codebase (as seen infiftyone/core/config.py), it's not integrated into the type resolution endpoint
🔗 Analysis chain
Ensure backend support for query_performance in type resolution
By adding query_performance: currentContext.queryPerformance, the request to /operators/resolve-type now includes performance tracking. Confirm that the server-side implementation of type resolution handles this new parameter appropriately.
Use the following script to check the server's capability:
Let me analyze the server-side implementation to verify the handling of query_performance parameter. The previous ast-grep command had syntax issues, so I'm using ripgrep to locate relevant files and examine the implementation.
I found the endpoint definition but need to examine the handler implementation to verify if it properly handles the query_performance parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'resolve_type' function in the server handles 'query_performance' parameter.
# Search for 'resolve_type' function definitions that include 'query_performance' in their parameters.
ast-grep --pattern 'def resolve_type($_, query_performance, $$):'
Length of output: 218
Script:
#!/bin/bash
# Let's search for resolve-type related endpoints and handlers in Python files
rg -l "resolve.?type" --type py
# Then let's look for query_performance usage in Python files
rg "query_performance" --type py
# Also search for the route definition
rg "/operators/resolve-type" -A 5
Length of output: 1962
Script:
#!/bin/bash
# Let's examine the ResolveType handler implementation
rg -A 10 "class ResolveType" fiftyone/operators/server.py
# Also check the request parameters handling in the handler
rg -B 5 -A 10 "def resolve" fiftyone/operators/server.py
Length of output: 585
add exec button to py panels
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
fiftyone/operators/types.py (1)
1373-1408: LGTM with suggestions for additional validation.The class implementation is solid with good documentation and basic validation. Consider adding:
- Type validation for the
paramsdictionary- Validation of icon names against a predefined set of valid Material Icons
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." ) + if "params" in kwargs and not isinstance(kwargs["params"], dict): + raise ValueError("The 'params' parameter must be a dictionary") + if "icon" in kwargs and kwargs["icon"] not in VALID_MATERIAL_ICONS: + raise ValueError(f"Invalid icon name: {kwargs['icon']}") super().__init__(**kwargs)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.tsx(1 hunks)app/packages/operators/src/types.ts(3 hunks)fiftyone/operators/types.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.tsx
- app/packages/operators/src/types.ts
| class StatusButtonView(View): | ||
| """Renders a status button. | ||
|
|
||
| Args: | ||
| severity (None): the severity of the alert displayed, one of | ||
| ``("info", "success", "warning", "error", "enabled", "disabled")`` | ||
| on_click (None): an operator to execute when the button is clicked | ||
| params (None): the parameters to pass to the operator | ||
| disabled: whether the button is disabled | ||
| title: tooltip title for the button | ||
| """ | ||
|
|
||
| def __init__(self, **kwargs): | ||
| super().__init__(**kwargs) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for severity values.
While the class is well-documented, it should validate the severity parameter against the allowed values to prevent runtime errors.
def __init__(self, **kwargs):
+ valid_severities = {"info", "success", "warning", "error", "enabled", "disabled"}
+ severity = kwargs.get("severity")
+ if severity is not None and severity not in valid_severities:
+ raise ValueError(
+ f"Invalid severity value: {severity}. Must be one of: {', '.join(valid_severities)}"
+ )
super().__init__(**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.
| class StatusButtonView(View): | |
| """Renders a status button. | |
| Args: | |
| severity (None): the severity of the alert displayed, one of | |
| ``("info", "success", "warning", "error", "enabled", "disabled")`` | |
| on_click (None): an operator to execute when the button is clicked | |
| params (None): the parameters to pass to the operator | |
| disabled: whether the button is disabled | |
| title: tooltip title for the button | |
| """ | |
| def __init__(self, **kwargs): | |
| super().__init__(**kwargs) | |
| class StatusButtonView(View): | |
| """Renders a status button. | |
| Args: | |
| severity (None): the severity of the alert displayed, one of | |
| ``("info", "success", "warning", "error", "enabled", "disabled")`` | |
| on_click (None): an operator to execute when the button is clicked | |
| params (None): the parameters to pass to the operator | |
| disabled: whether the button is disabled | |
| title: tooltip title for the button | |
| """ | |
| def __init__(self, **kwargs): | |
| valid_severities = {"info", "success", "warning", "error", "enabled", "disabled"} | |
| severity = kwargs.get("severity") | |
| if severity is not None and severity not in valid_severities: | |
| raise ValueError( | |
| f"Invalid severity value: {severity}. Must be one of: {', '.join(valid_severities)}" | |
| ) | |
| super().__init__(**kwargs) |
fix model evaluation panel name
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
fiftyone/operators/builtins/panels/model_evaluation/__init__.py (1)
Line range hint
279-283: Consider enhancing error handling and caching strategyWhile the error handling for store operations is good, consider these improvements:
- Use proper logging instead of just
traceback.print_exc()- Consider notifying the user when caching fails
- Consider implementing a TTL for cached evaluation data to prevent stale results
Here's a suggested improvement:
try: store.set(computed_eval_id, evaluation_data) except Exception: - traceback.print_exc() + logger.exception("Failed to cache evaluation data") + ctx.ops.notify( + "Warning: Failed to cache evaluation data. Performance may be affected.", + variant="warning" + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
fiftyone/operators/builtins/panels/model_evaluation/__init__.py(1 hunks)
🔇 Additional comments (1)
fiftyone/operators/builtins/panels/model_evaluation/__init__.py (1)
33-34: LGTM: Improved panel configuration naming
The changes to use the STORE_NAME constant and the more descriptive label "Model Evaluation" improve code maintainability and user experience.
Fix target view for clear sample/frame field operators
Merge
release/v1.1.0todevelopSummary by CodeRabbit
Release Notes
New Features
StatusButtonandOperatorExecutionButtoncomponents for enhanced user interactions.OperatorExecutionMenufor executing operators with options.StatusButtonViewandOperatorExecutionButtonViewfor improved UI representation.Improvements
primaryMediumcolor.Bug Fixes
Chores