fix operator prompt hidden for executing, output, and error #4445
Conversation
WalkthroughThis update enhances the operator prompt interface by adding new data attributes for testing, introducing new control variables in utility functions, and updating end-to-end test scripts. It also includes new classes and configurations for E2E tests, enabling more robust and detailed testing of prompt dialogs and drawers in the operators' browser interface. Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant OperatorsPromptPom
participant OperatorsBrowserPom
participant E2EProgress
User->>OperatorsBrowserPom: Initiate Prompt
OperatorsBrowserPom->>OperatorsPromptPom: Open Modal/Drawer
OperatorsPromptPom->>E2EProgress: Execute Action
E2EProgress-->>OperatorsPromptPom: Return Progress
OperatorsPromptPom-->>OperatorsBrowserPom: Display Progress
User-->>OperatorsPromptPom: Interact with Prompt
OperatorsPromptPom-->>User: Provide Feedback
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
imanjra
force-pushed
the
bugfix/missing-operator-prompt
branch
from
d0d0c56 to
3fd57c3
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: 5
Outside diff range and nitpick comments (2)
app/packages/operators/src/OperatorPrompt/OperatorModalPrompt.tsx (1)
Line range hint
26-27: Consider using template literals for better readability and maintainability.- "Cannot execute operator with validation errors\n\n" + validationErrorsStr + `Cannot execute operator with validation errors\n\n${validationErrorsStr}`app/packages/operators/src/OperatorPrompt/index.tsx (1)
Line range hint
18-20: Remove the unnecessary else clause to simplify the control flow, as the if condition already returns early.- } else { - return null; - } + return null;
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (9)
- app/packages/operators/src/OperatorPrompt/OperatorDrawerPrompt.tsx (2 hunks)
- app/packages/operators/src/OperatorPrompt/OperatorModalPrompt.tsx (1 hunks)
- app/packages/operators/src/OperatorPrompt/OperatorViewModal.tsx (1 hunks)
- app/packages/operators/src/OperatorPrompt/index.tsx (1 hunks)
- e2e-pw/src/oss/poms/operators/operators-prompt.ts (1 hunks)
- e2e-pw/src/oss/specs/operators/built-in-operators.spec.ts (1 hunks)
- e2e-pw/src/oss/specs/operators/prompt.spec.ts (1 hunks)
- e2e-pw/src/shared/assets/plugins/e2e/init.py (2 hunks)
- e2e-pw/src/shared/assets/plugins/e2e/fiftyone.yml (1 hunks)
Files skipped from review due to trivial changes (2)
- app/packages/operators/src/OperatorPrompt/OperatorDrawerPrompt.tsx
- app/packages/operators/src/OperatorPrompt/OperatorViewModal.tsx
Additional context used
Path-based instructions (5)
app/packages/operators/src/OperatorPrompt/OperatorModalPrompt.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.e2e-pw/src/oss/specs/operators/built-in-operators.spec.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.e2e-pw/src/oss/poms/operators/operators-prompt.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/OperatorPrompt/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.e2e-pw/src/oss/specs/operators/prompt.spec.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.
Biome
app/packages/operators/src/OperatorPrompt/OperatorModalPrompt.tsx
[error] 26-27: Template literals are preferred over string concatenation.
e2e-pw/src/oss/specs/operators/built-in-operators.spec.ts
[error] 6-6: Do not use template literals if interpolation and special-character handling are not needed.
e2e-pw/src/oss/poms/operators/operators-prompt.ts
[error] 13-13: Template literals are preferred over string concatenation.
app/packages/operators/src/OperatorPrompt/index.tsx
[error] 18-20: This else clause can be omitted because previous branches break early.
e2e-pw/src/oss/specs/operators/prompt.spec.ts
[error] 6-6: Do not use template literals if interpolation and special-character handling are not needed.
Additional comments not posted (6)
e2e-pw/src/shared/assets/plugins/e2e/fiftyone.yml (1)
8-10: The addition of new E2E operators is consistent with the PR's objectives to enhance testing capabilities.app/packages/operators/src/OperatorPrompt/OperatorModalPrompt.tsx (1)
31-31: AddingdialogPropswithdata-cyattribute enhances testability by providing a way to select this component in E2E tests.e2e-pw/src/shared/assets/plugins/e2e/__init__.py (1)
26-101: The introduction of new E2E operator classes (E2ESayHelloInModal,E2ESayHelloInDrawer,E2EProgress) is consistent with the PR's objectives to enhance testing capabilities.e2e-pw/src/oss/specs/operators/prompt.spec.ts (3)
57-69: The test case for canceling the modal prompt is well-structured and covers the necessary interactions.
90-101: The test case for canceling the drawer prompt is well-structured and covers the necessary interactions.
122-140: The test case for checking progress updates in a prompt is well-structured and covers the necessary interactions.
| @@ -21,5 +23,79 @@ def execute(self, ctx): | |||
| return {} | |||
|
|
|||
|
|
|||
| class E2ESayHelloInModal(foo.Operator): | |||
imanjra
force-pushed
the
bugfix/missing-operator-prompt
branch
from
750be0a to
fc56d4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/packages/operators/src/OperatorPrompt/OperatorModalPrompt.tsx (1)
Line range hint
26-27: Consider using template literals for constructing dynamic strings to enhance readability.- "Cannot execute operator with validation errors\n\n" + - validationErrorsStr + `Cannot execute operator with validation errors\n\n${validationErrorsStr}`
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (8)
- app/packages/operators/src/OperatorPrompt/OperatorDrawerPrompt.tsx (2 hunks)
- app/packages/operators/src/OperatorPrompt/OperatorModalPrompt.tsx (1 hunks)
- app/packages/operators/src/OperatorPrompt/OperatorViewModal.tsx (1 hunks)
- e2e-pw/src/oss/poms/operators/operators-prompt.ts (1 hunks)
- e2e-pw/src/oss/specs/operators/built-in-operators.spec.ts (1 hunks)
- e2e-pw/src/oss/specs/operators/prompt.spec.ts (1 hunks)
- e2e-pw/src/shared/assets/plugins/e2e/init.py (2 hunks)
- e2e-pw/src/shared/assets/plugins/e2e/fiftyone.yml (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- app/packages/operators/src/OperatorPrompt/OperatorViewModal.tsx
- e2e-pw/src/oss/specs/operators/built-in-operators.spec.ts
- e2e-pw/src/oss/specs/operators/prompt.spec.ts
- e2e-pw/src/shared/assets/plugins/e2e/init.py
- e2e-pw/src/shared/assets/plugins/e2e/fiftyone.yml
Additional context used
Path-based instructions (3)
app/packages/operators/src/OperatorPrompt/OperatorModalPrompt.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/OperatorPrompt/OperatorDrawerPrompt.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.e2e-pw/src/oss/poms/operators/operators-prompt.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.
Biome
app/packages/operators/src/OperatorPrompt/OperatorModalPrompt.tsx
[error] 26-27: Template literals are preferred over string concatenation.
[error] 3-4: All these imports are only used as types.
app/packages/operators/src/OperatorPrompt/OperatorDrawerPrompt.tsx
[error] 7-8: All these imports are only used as types.
e2e-pw/src/oss/poms/operators/operators-prompt.ts
[error] 1-1: Some named imports are only used as types.
Additional comments not posted (5)
app/packages/operators/src/OperatorPrompt/OperatorModalPrompt.tsx (1)
31-31: Ensure thedata-cyattribute is consistently used across all test environments.app/packages/operators/src/OperatorPrompt/OperatorDrawerPrompt.tsx (2)
35-35: Ensure thedata-cyattribute is consistently used across all test environments.
46-48: Adding adata-cyattribute to theBoxcomponent enhances the testability of the operator prompt drawer content.e2e-pw/src/oss/poms/operators/operators-prompt.ts (2)
13-13: Using template literals for constructing dynamic strings enhances readability and maintainability.
1-1: Ensure that named imports are used correctly and consistently across the project.Verification successful
The verification script successfully identified multiple instances of named imports from
"src/oss/fixtures". The imports are consistently using named imports, including the specific case inoperators-prompt.ts.
e2e-pw/src/oss/poms/operators/operators-prompt.ts:import { Locator, Page, expect } from "src/oss/fixtures";This confirms that the named imports are used correctly and consistently across the project.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that named imports are used correctly in the project. # Test: Search for incorrect usage of named imports. Expect: No occurrences of incorrect usage. rg --type typescript $'import {.*} from "src/oss/fixtures";'Length of output: 188
Script:
#!/bin/bash # Description: Verify that named imports are used correctly in the project. # Test: Search for incorrect usage of named imports. Expect: No occurrences of incorrect usage. rg --type typescript 'import \{.*\} from "src/oss/fixtures";'Length of output: 98
Script:
#!/bin/bash # Description: Verify that named imports are used correctly in the project. # Test: Search for incorrect usage of named imports. Expect: No occurrences of incorrect usage. rg --type ts 'import \{.*\} from "src/oss/fixtures";'Length of output: 6019
Tools
Biome
[error] 1-1: Some named imports are only used as types.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- app/packages/operators/src/OperatorPrompt/OperatorDrawerPrompt.tsx (3 hunks)
- app/packages/operators/src/OperatorPrompt/OperatorModalPrompt.tsx (1 hunks)
- app/packages/operators/src/utils.ts (2 hunks)
- e2e-pw/src/oss/poms/operators/operators-prompt.ts (1 hunks)
- e2e-pw/src/oss/specs/operators/prompt.spec.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- e2e-pw/src/oss/specs/operators/prompt.spec.ts
Additional context used
Path-based instructions (4)
app/packages/operators/src/OperatorPrompt/OperatorModalPrompt.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/OperatorPrompt/OperatorDrawerPrompt.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.e2e-pw/src/oss/poms/operators/operators-prompt.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/utils.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.
Biome
app/packages/operators/src/OperatorPrompt/OperatorModalPrompt.tsx
[error] 3-4: All these imports are only used as types.
app/packages/operators/src/OperatorPrompt/OperatorDrawerPrompt.tsx
[error] 7-8: All these imports are only used as types.
e2e-pw/src/oss/poms/operators/operators-prompt.ts
[error] 1-1: Some named imports are only used as types.
app/packages/operators/src/utils.ts
[error] 57-57: Declare variables separately
[error] 57-57: This variable implicitly has the any type.
[error] 59-59: Use === instead of ==.
== is only allowed when comparing againstnull
[error] 83-83: Template literals are preferred over string concatenation.
[error] 1-2: All these imports are only used as types.
[error] 2-3: Some named imports are only used as types.
Additional comments not posted (5)
app/packages/operators/src/OperatorPrompt/OperatorModalPrompt.tsx (1)
14-14: AddeddialogPropsto enhance testing capabilities. Ensure it integrates well with existing props.app/packages/operators/src/OperatorPrompt/OperatorDrawerPrompt.tsx (2)
35-35: Addeddata-cyattribute to enhance testing capabilities. Ensure it integrates well with existing props.
46-60: Ensure that the newdata-cyattributes on theBoxandStackcomponents are correctly utilized in tests.Verification successful
The
data-cyattributesoperators-prompt-drawer-contentandoperators-prompt-drawer-footerare correctly utilized in the test filee2e-pw/src/oss/poms/operators/operators-prompt.ts.
e2e-pw/src/oss/poms/operators/operators-prompt.ts:
return this.locator.getByTestId("operators-prompt-drawer-content");return this.locator.getByTestId("operators-prompt-drawer-footer");Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that new `data-cy` attributes are utilized in tests. # Test: Search for the usage of these attributes in test files. Expect: Proper usage in test assertions. rg --type typescript $'operators-prompt-drawer-content|operators-prompt-drawer-footer'Length of output: 122
Script:
#!/bin/bash # Description: Verify that new `data-cy` attributes are utilized in tests. # Test: Search for the usage of these attributes in test files. Expect: Proper usage in test assertions. rg --type ts 'operators-prompt-drawer-content|operators-prompt-drawer-footer' rg --type tsx 'operators-prompt-drawer-content|operators-prompt-drawer-footer'Length of output: 668
e2e-pw/src/oss/poms/operators/operators-prompt.ts (1)
13-13: Good use of template literals for constructing dynamic strings, enhancing readability and maintainability.app/packages/operators/src/utils.ts (1)
Line range hint
80-111: RefactoredgetOperatorPromptConfigsto handle new conditions and states. Ensure all new variables are properly integrated and tested.Verification successful
The new variables
loading,disableSubmit,disabledReason, andonCloseare properly integrated and used across multiple files in the codebase. This confirms that the refactoredgetOperatorPromptConfigsfunction handles these new conditions and states correctly.
loadingis used in various contexts, including state management and UI components.disableSubmitanddisabledReasonare used in components related to form submission and validation.onCloseis used in components handling modal or dialog closures.These usages indicate that the new variables are well-integrated and their handling is consistent with the application's logic.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all new variables in `getOperatorPromptConfigs` are properly integrated. # Test: Search for the usage of new variables in the application. Expect: Proper integration and handling. rg --type typescript $'loading|disableSubmit|disabledReason|onClose'Length of output: 104
Script:
#!/bin/bash # Description: Verify that all new variables in `getOperatorPromptConfigs` are properly integrated. # Test: Search for the usage of new variables in the application. Expect: Proper integration and handling. rg --type ts 'loading|disableSubmit|disabledReason|onClose'Length of output: 23842
Tools
Biome
[error] 83-83: Template literals are preferred over string concatenation.
| validationErrorsStr, | ||
| ...otherConfigs | ||
| } = getOperatorPromptConfigs(prompt); | ||
| const promptConfig = getOperatorPromptConfigs(prompt); |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure getOperatorPromptConfigs handles the new dialogProps correctly.
The getOperatorPromptConfigs function does not currently handle dialogProps. The function definition and its usage in the codebase do not show any reference to dialogProps. Additionally, the OperatorPromptType does not include dialogProps, and the useOperatorPrompt hook, which defines OperatorPromptType, does not handle dialogProps.
app/packages/operators/src/utils.ts: Definition ofgetOperatorPromptConfigs.app/packages/operators/src/types.ts: Definition ofOperatorPromptType.app/packages/operators/src/state.ts: Definition ofuseOperatorPrompt.
Please update the getOperatorPromptConfigs function and related types to handle dialogProps.
Analysis chain
Ensure getOperatorPromptConfigs handles the new dialogProps correctly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `getOperatorPromptConfigs` is updated to handle `dialogProps`.
# Test: Search for the function definition and usage. Expect: Proper handling of `dialogProps`.
rg --type typescript $'getOperatorPromptConfigs'
Length of output: 82
Script:
#!/bin/bash
# Description: Verify that `getOperatorPromptConfigs` is updated to handle `dialogProps`.
# Test: Search for the function definition and usage. Expect: Proper handling of `dialogProps`.
rg --type ts,tsx 'getOperatorPromptConfigs'
Length of output: 74
Script:
#!/bin/bash
# Description: Verify that `getOperatorPromptConfigs` is updated to handle `dialogProps`.
# Test: Search for the function definition and usage. Expect: Proper handling of `dialogProps`.
rg --type ts 'getOperatorPromptConfigs'
rg --type tsx 'getOperatorPromptConfigs'
Length of output: 946
Script:
#!/bin/bash
# Description: Extract the definition of `getOperatorPromptConfigs` to verify handling of `dialogProps`.
# Extract the function definition from `utils.ts`.
rg --type ts --context 10 'export function getOperatorPromptConfigs' app/packages/operators/src/utils.ts
Length of output: 849
Script:
#!/bin/bash
# Description: Verify if `dialogProps` is part of `OperatorPromptType`.
# Search for the definition of `OperatorPromptType`.
rg --type ts 'type OperatorPromptType' --context 10
Length of output: 1992
Script:
#!/bin/bash
# Description: Verify if `dialogProps` is included in the return type of `useOperatorPrompt`.
# Search for the implementation of `useOperatorPrompt`.
rg --type ts,tsx 'function useOperatorPrompt' --context 20
Length of output: 91
Script:
#!/bin/bash
# Description: Verify if `dialogProps` is included in the return type of `useOperatorPrompt`.
# Search for the implementation of `useOperatorPrompt`.
rg --type ts 'function useOperatorPrompt' --context 20
rg --type tsx 'function useOperatorPrompt' --context 20
Length of output: 142
Script:
#!/bin/bash
# Description: Locate custom hooks in the codebase to find the `useOperatorPrompt` function.
# Search for custom hooks in the codebase.
rg --type ts,tsx 'function use' --context 20
Length of output: 77
Script:
#!/bin/bash
# Description: Locate custom hooks in the codebase to find the `useOperatorPrompt` function.
# Search for custom hooks in `.ts` files.
rg --type ts 'function use' --context 20
# Search for custom hooks in `.tsx` files.
rg --type tsx 'function use' --context 20
Length of output: 164647
Script:
#!/bin/bash
# Description: Locate the `useOperatorPrompt` function in `.ts` files to verify handling of `dialogProps`.
# Search for the `useOperatorPrompt` function in `.ts` files.
rg --type ts 'useOperatorPrompt' --context 20
Length of output: 19100
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- e2e-pw/src/oss/poms/operators/operators-prompt.ts (1 hunks)
- e2e-pw/src/oss/specs/operators/prompt.spec.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- e2e-pw/src/oss/poms/operators/operators-prompt.ts
- e2e-pw/src/oss/specs/operators/prompt.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- e2e-pw/src/oss/poms/operators/operators-prompt.ts (1 hunks)
- e2e-pw/src/oss/specs/operators/prompt.spec.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- e2e-pw/src/oss/poms/operators/operators-prompt.ts
- e2e-pw/src/oss/specs/operators/prompt.spec.ts
There was a problem hiding this comment.
LGTM! 🚀 Specs and POMs look awesome! I moved validation to the asserter based on best practices from @sashankaryal. But somewhat subjective, minor detail
| const loading = resolving || pendingResolve; | ||
| const disableSubmit = hasValidationErrors || resolving || pendingResolve; | ||
| const disabledReason = hasValidationErrors | ||
| ? "Cannot execute operator with validation errors\n\n" + validationErrorsStr |
There was a problem hiding this comment.
Might be helpful to put these messages as const variables somewhere. Not important for this PR
What changes are proposed in this pull request?
fix operator prompt hidden for executing, output, and error stages of operator execution
How is this patch tested? If it is not, please explain why.
Manually in the app and E2E
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
Fixed issue where operator prompt is now shown for executing, output, and error stages of operator execution
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
e2e_say_hello_in_modal,e2e_say_hello_in_drawer, ande2e_progress.Improvements
Bug Fixes