background compute card for OperatorExecutionTrigger#7266
background compute card for OperatorExecutionTrigger#7266
Conversation
|
Thanks for opening this pull request, @imanjra! 🎉 A maintainer will review your changes soon. In the meantime, please ensure:
Helpful resources: |
WalkthroughThis change introduces orchestrator setup requirement detection across the operator execution UI. A new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
imanjra
force-pushed
the
feat/inline-bc-msg
branch
from
5a03882 to
8dfbb6d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx (1)
118-139: 🧹 Nitpick | 🔵 TrivialConsider conditionally rendering
OperatorExecutionMenuwhen not in setup mode.When
requiresOrchestratorSetupis true, theBox(anchor element) is not rendered, socontainerRef.currentwill benull. While the menu won't open (sinceclickHandlercan't fire), renderingOperatorExecutionMenuwith a null anchor is unnecessary.♻️ Proposed refactor
{!requiresOrchestratorSetup && ( <Box ref={containerRef} onClick={clickHandler} {...props}> {children} </Box> )} - <OperatorExecutionMenu - anchor={containerRef.current} - open={isMenuOpen && !disabled} - onClose={() => setIsMenuOpen(false)} - onOptionClick={onOptionSelected} - executionOptions={executionOptions} - insideModal={insideModal} - anchorOrigin={menuAnchorOrigin} - transformOrigin={menuTransformOrigin} - /> + {!requiresOrchestratorSetup && ( + <OperatorExecutionMenu + anchor={containerRef.current} + open={isMenuOpen && !disabled} + onClose={() => setIsMenuOpen(false)} + onOptionClick={onOptionSelected} + executionOptions={executionOptions} + insideModal={insideModal} + anchorOrigin={menuAnchorOrigin} + transformOrigin={menuTransformOrigin} + /> + )} </>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx` around lines 118 - 139, The OperatorExecutionMenu is rendered even when requiresOrchestratorSetup is true which means the anchor Box (containerRef.current) doesn't exist; change rendering so OperatorExecutionMenu is only mounted when not in setup mode (i.e., wrap or gate the OperatorExecutionMenu with !requiresOrchestratorSetup) so it receives a valid anchor and is not unnecessarily rendered when requiresOrchestratorSetup is true; keep existing props (anchor, open, onClose, onOptionClick, executionOptions, insideModal, anchorOrigin, transformOrigin) and the open logic (isMenuOpen && !disabled).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/packages/operators/src/components/RequiresOrchestrator.tsx`:
- Around line 12-17: The external Link component rendering the "Set up now"
Button uses target="_blank" but is missing rel="noopener noreferrer"; update the
Link element (the Link wrapping the Button in RequiresOrchestrator.tsx) to
include rel="noopener noreferrer" so new windows cannot access window.opener and
to follow best practices for external links.
In `@app/packages/operators/src/state.ts`:
- Around line 437-446: The return object explicitly maps
requiresOrchestratorSetup property but can use ES object property shorthand;
update the return in the function that calls useOperatorPromptSubmitOptions
(where options and requiresOrchestratorSetup are destructured) to return {
executionOptions: options, requiresOrchestratorSetup } instead of repeating
requiresOrchestratorSetup: requiresOrchestratorSetup.
---
Outside diff comments:
In `@app/packages/operators/src/components/OperatorExecutionTrigger/index.tsx`:
- Around line 118-139: The OperatorExecutionMenu is rendered even when
requiresOrchestratorSetup is true which means the anchor Box
(containerRef.current) doesn't exist; change rendering so OperatorExecutionMenu
is only mounted when not in setup mode (i.e., wrap or gate the
OperatorExecutionMenu with !requiresOrchestratorSetup) so it receives a valid
anchor and is not unnecessarily rendered when requiresOrchestratorSetup is true;
keep existing props (anchor, open, onClose, onOptionClick, executionOptions,
insideModal, anchorOrigin, transformOrigin) and the open logic (isMenuOpen &&
!disabled).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a616a336-3a85-47a2-b439-21894dc961a5
⛔ Files ignored due to path filters (2)
app/packages/core/package.jsonis excluded by!**/*.jsonapp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (4)
app/packages/operators/src/components/OperatorExecutionTrigger/index.tsxapp/packages/operators/src/components/OperatorPromptFooter.tsxapp/packages/operators/src/components/RequiresOrchestrator.tsxapp/packages/operators/src/state.ts
| <Link | ||
| href="https://docs.voxel51.com/plugins/using_plugins.html#delegated-operations" | ||
| target="_blank" | ||
| > | ||
| <Button variant={Variant.Secondary}>Set up now</Button> | ||
| </Link> |
There was a problem hiding this comment.
Add rel="noopener noreferrer" to external link.
When using target="_blank", always include rel="noopener noreferrer" to prevent the new page from accessing window.opener (potential security risk) and to avoid performance issues.
🔒 Proposed fix
<Link
href="https://docs.voxel51.com/plugins/using_plugins.html#delegated-operations"
target="_blank"
+ rel="noopener noreferrer"
>
<Button variant={Variant.Secondary}>Set up now</Button>
</Link>📝 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.
| <Link | |
| href="https://docs.voxel51.com/plugins/using_plugins.html#delegated-operations" | |
| target="_blank" | |
| > | |
| <Button variant={Variant.Secondary}>Set up now</Button> | |
| </Link> | |
| <Link | |
| href="https://docs.voxel51.com/plugins/using_plugins.html#delegated-operations" | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| > | |
| <Button variant={Variant.Secondary}>Set up now</Button> | |
| </Link> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/packages/operators/src/components/RequiresOrchestrator.tsx` around lines
12 - 17, The external Link component rendering the "Set up now" Button uses
target="_blank" but is missing rel="noopener noreferrer"; update the Link
element (the Link wrapping the Button in RequiresOrchestrator.tsx) to include
rel="noopener noreferrer" so new windows cannot access window.opener and to
follow best practices for external links.
| const { options, requiresOrchestratorSetup } = useOperatorPromptSubmitOptions( | ||
| operatorUri, | ||
| execDetails, | ||
| onExecute | ||
| ); | ||
|
|
||
| return { | ||
| executionOptions: submitOptions.options, | ||
| executionOptions: options, | ||
| requiresOrchestratorSetup: requiresOrchestratorSetup, | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM - logic is correct.
The destructuring and return of requiresOrchestratorSetup correctly exposes the orchestrator setup state to consumers. Minor nit: line 445 can use object property shorthand.
✨ Optional shorthand refactor
return {
executionOptions: options,
- requiresOrchestratorSetup: requiresOrchestratorSetup,
+ requiresOrchestratorSetup,
};📝 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.
| const { options, requiresOrchestratorSetup } = useOperatorPromptSubmitOptions( | |
| operatorUri, | |
| execDetails, | |
| onExecute | |
| ); | |
| return { | |
| executionOptions: submitOptions.options, | |
| executionOptions: options, | |
| requiresOrchestratorSetup: requiresOrchestratorSetup, | |
| }; | |
| const { options, requiresOrchestratorSetup } = useOperatorPromptSubmitOptions( | |
| operatorUri, | |
| execDetails, | |
| onExecute | |
| ); | |
| return { | |
| executionOptions: options, | |
| requiresOrchestratorSetup, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/packages/operators/src/state.ts` around lines 437 - 446, The return
object explicitly maps requiresOrchestratorSetup property but can use ES object
property shorthand; update the return in the function that calls
useOperatorPromptSubmitOptions (where options and requiresOrchestratorSetup are
destructured) to return { executionOptions: options, requiresOrchestratorSetup }
instead of repeating requiresOrchestratorSetup: requiresOrchestratorSetup.
| </Link> | ||
| } | ||
| /> | ||
| <RequiresOrchestrator /> |
There was a problem hiding this comment.
I'm having a hard time reconciling the changes in this file with the changes in your other PR. It seems that both of these PRs touch a lot of the same files, and I'm not sure which set of changes is meant to supersede the other set.
Is there a way to simplify this? Could this PR use your other PR (#2469) as its target instead of the develop branch? Also, it may be possible that the other PR needs to be rebased onto the develop branch because there are some items in its diff that are already on develop.
There was a problem hiding this comment.
This PR has a subset of changes of the other PR. The changes have been pulled into other PR for testabilty
3 participants
🔗 Related Issues
📋 What changes are proposed in this pull request?
🧪 How is this patch tested? If it is not, please explain why.
Using an operator that cannot be executed immediately
📝 Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
Release Notes
New Features
Refactor