Add on_cancel callback for operator prompt #5348
Conversation
WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (4)app/packages/core/src/plugins/SchemaIO/components/TooltipProvider.tsx (1)Pattern app/packages/operators/src/state.ts (1)Pattern app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx (1)Pattern app/packages/operators/src/components/OperatorExecutionButton/index.tsx (1)Pattern ⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (10)
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
🧹 Nitpick comments (2)
app/packages/operators/src/state.ts (1)
582-586: Consider adding TypeScript types for better maintainability.The implementation is correct, but adding TypeScript types would improve code maintainability and prevent potential errors:
- const onCancel = promptingOperator.options?.onCancel; + const onCancel: (() => void) | undefined = promptingOperator.options?.onCancel; - const cancel = () => { + const cancel = (): void => { if (onCancel) onCancel(); close(); };app/packages/operators/src/built-in-operators.ts (1)
1049-1050: Consider adding error handling for invalid operator_uri.The destructuring is correct, but consider adding validation for the
operator_uriwhen handling cancellation:- const { params, operator_uri, on_success, on_error, on_cancel } = - ctx.params; + const { params, operator_uri, on_success, on_error, on_cancel } = ctx.params; + if (on_cancel && !operator_uri) { + console.error("Cannot handle cancellation: operator_uri is required"); + return; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/packages/operators/src/built-in-operators.ts(3 hunks)app/packages/operators/src/state.ts(2 hunks)app/packages/operators/src/usePanelEvent.ts(3 hunks)fiftyone/operators/executor.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/operators/src/usePanelEvent.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/built-in-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.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e / test-e2e
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: build
🔇 Additional comments (6)
fiftyone/operators/executor.py (1)
Line range hint
747-774: LGTM! Clean implementation of the cancel callback.The addition of the
on_cancelparameter to thepromptmethod is well-implemented and properly documented. The parameter follows the same pattern as the existingon_successandon_errorcallbacks.app/packages/operators/src/state.ts (1)
664-664: LGTM! Proper integration of the cancel function.The
cancelfunction is correctly exposed in the hook's return value, maintaining consistency with the existing pattern.app/packages/operators/src/usePanelEvent.ts (2)
14-14: LGTM! Well-typed addition to HandlerOptions.The
onCancelcallback is properly typed as an optional function in theHandlerOptionsinterface.
24-24: LGTM! Clean destructuring of the new option.The
onCanceloption is correctly destructured along with other options, maintaining consistency.app/packages/operators/src/built-in-operators.ts (2)
1040-1040: LGTM! Clean addition to input resolution.The
on_cancelparameter is properly added to the input resolution schema.
1059-1066: LGTM! Clean implementation of cancel handling.The cancel handling logic is well-implemented, properly triggering the cancel operation with the original operator URI.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx (2)
43-43: Document theexecutionOptionsprop
The newexecutionOptionsprop isn’t described in the component’s JSDoc, making it difficult for future maintainers to understand its role. Consider adding a brief explanation.
54-55: Include new props in JSDoc
onCancelandpromptare newly introduced but missing from the doc comment. Add them to maintain complete documentation.app/packages/operators/src/components/OperatorExecutionButton/index.tsx (4)
25-25: Update docstring to include new parameters
The component supportsprompt,executorOptions, andonCancel, but these fields are not mentioned in the doc comment. Keeping the docs aligned ensures clarity for future maintainers.
45-51: Ensure consistency with other operator-related props
onCancel,prompt, andexecutorOptionsmight need to be documented in the same style asonSuccessandonErrorfor consistency.
61-85: Conditional operator prompt vs. direct execution
This approach cleanly accommodates both scenarios. However, consider adding error/catch handling foroperator.execute(...)if rejections need to be processed.
92-98: Prompt-based execution flow
The conditional block forpromptclearly handles user prompts. Confirm that the returned promise fromonExecuteis used or handled upstream if needed.app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.tsx (2)
81-87: Consider including context parameters for consistencyWhile the cancel handler implementation is correct, consider passing context parameters to maintain consistency with other handlers (
handleOnSuccessandhandleOnError). This would provide more context to the cancel operation.- const handleOnCancel: ExecutionCancelCallback = () => { + const handleOnCancel: ExecutionCancelCallback = (_, { ctx }) => { if (on_cancel) { triggerEvent(panelId, { operator: on_cancel, + params: { + original_params: ctx.params + } }); } };
99-104: Improve clarity of the prompt conditionWhile the icon props refactoring is good, the condition's purpose could be more explicit. Consider using a more descriptive condition name or adding a comment explaining why icons are hidden when there's a prompt.
- const iconProps = prompt + // Hide icons when showing a prompt dialog + const iconProps = Boolean(prompt) ? {} : { startIcon: icon_position === "left" ? Icon : undefined, endIcon: icon_position === "right" ? Icon : undefined, };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.tsx(5 hunks)app/packages/operators/src/components/OperatorExecutionButton/index.tsx(2 hunks)app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx(4 hunks)app/packages/operators/src/types-internal.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
app/packages/operators/src/types-internal.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/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/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.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (12)
app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx (2)
2-2: Adoption of MUI Box and Button looks good
These additions should simplify styling and layout for the trigger component.
58-58: Check for naming consistency
executionOptionsis referenced both as a prop (line 43) and type-hinted here (line 58). Ensure consistent naming in external references for clarity.app/packages/operators/src/types-internal.ts (1)
12-12: NewExecutionCancelCallbacktype provides clarity
Defining a dedicated type for the cancellation callback is a good practice for readability and reusability.app/packages/operators/src/components/OperatorExecutionButton/index.tsx (6)
3-14: Imports are consistent with the new feature
AddinguseCallback,useMemo, and the operator-related imports aligns with the new prompt and cancellation mechanics. No issues noted.
32-38: Validate destructured props
Lines 32, 35, and 38 introduceonCancel,prompt, andexecutorOptions. Confirm that all references in the component are typed correctly and that default values or checks are not needed.
53-60: Memoizing handler callbacks
StoringonSuccessandonErrorinoperatorHandlershelps avoid unnecessary re-renders and ensures consistent references. Looks good.
87-91: Dynamic retrieval ofexecutionOptions
RetrievingexecutionOptionsfromuseOperatorExecutionOptionsis aligned with the new approach. Ensure it reflects any changes triggered bypromptor cancellation.
99-99: Condition with no prompt
Falling back to direct execution is a logical alternative to the prompt-based flow. No immediate issues found.
106-107: Pass-through ofonCancelandprompt
Ensures theOperatorExecutionTriggercan leverage the new callbacks. Looks consistent with the PR’s objectives.app/packages/core/src/plugins/SchemaIO/components/OperatorExecutionButtonView.tsx (3)
14-14: LGTM! Type import follows best practicesThe
ExecutionCancelCallbacktype import is properly grouped with related execution callback types.
34-35: LGTM! Props follow existing conventionsThe new
on_cancelandpromptprops are properly integrated and follow the established naming pattern.
114-115: LGTM! Button updates are well-structuredThe new props are properly integrated, and the iconProps spreading is correctly implemented.
Also applies to: 119-119
app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx (2)
Line range hint
11-36: Update JSDoc comments to reflect current props.The component's documentation is outdated. It references removed props (
operatorUri,onSuccess,onError,executionParams,executorOptions) and doesn't document new ones (onCancel,prompt,executionOptions).Update the JSDoc to include:
@param executionOptionsArray of execution options@param onCancelCallback for when the prompt is canceled@param promptFlag to enable prompting behavior
Line range hint
69-85: Consider adding type safety for spread props.While the implementation is correct, consider adding a type constraint for the spread operator
{...props}to prevent passing unintended props to the Box component.- ...props + ...({} as Omit<React.ComponentProps<typeof Box>, keyof { + ref: unknown; + onClick: unknown; + children: unknown; + }>)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: lint / eslint
- GitHub Check: build
🔇 Additional comments (1)
app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx (1)
Line range hint
37-56: Implementation follows React best practices.The component:
- Correctly uses React hooks
- Has clean separation of concerns
- Implements proper event handling
Also applies to: 57-67
app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx
Outdated
Show resolved
Hide resolved
What changes are proposed in this pull request?
Adds a new
on_cancelcallback when prompting for an operator. This will be called if a user cancels the operator prompt. NOTE: this behavior is not intended or available for immediately executing operators, such as whenskip_prompt=Trueor the operator does not have input.The example below is available as a panel example here.
How is this patch tested? If it is not, please explain why.
(Details)
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
Adds a new
on_cancelevent forctx.prompt().What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Summary by CodeRabbit
New Features
on_cancelparameter to prompt operations across multiple components.onCancelandpromptto relevant components for improved interaction.Improvements