Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to use observer in the prompt section #1406

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Dec 17, 2024

Important

Add observer functionality in PromptBox with version selection and mutation handling, controlled by VITE_OBSERVER_ENABLED.

  • Behavior:
    • Add observer functionality in PromptBox.tsx with version selection (v1, v2).
    • v2 triggers startObserverCruiseMutation for observer cruise.
    • Success and error toasts for observer cruise.
  • Types:
    • Add ObserverCruise type in types.ts.
  • UI Components:
    • Add CustomSelectItem and SelectItemText in select.tsx for custom select UI.
  • Environment:
    • Add observerFeatureEnabled in env.ts to control observer feature based on VITE_OBSERVER_ENABLED.

This description was created by Ellipsis for 1543e72. It will automatically update as commits are pushed.

…src/'

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Add observer functionality in `PromptBox` with version selection and mutation handling, controlled by `VITE_OBSERVER_ENABLED`.
>
>   - **Behavior**:
>     - Add observer functionality in `PromptBox.tsx` with version selection (`v1`, `v2`).
>     - `v2` triggers `startObserverCruiseMutation` for observer cruise.
>     - Success and error toasts for observer cruise.
>   - **Types**:
>     - Add `ObserverCruise` type in `types.ts`.
>   - **UI Components**:
>     - Add `CustomSelectItem` and `SelectItemText` in `select.tsx` for custom select UI.
>   - **Environment**:
>     - Add `observerFeatureEnabled` in `env.ts` to control observer feature based on `VITE_OBSERVER_ENABLED`.
>
> <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=Skyvern-AI%2Fskyvern-cloud&utm_source=github&utm_medium=referral)<sup> for 393a2763ea5281d209b15dceeb2aca9bd14debff. It will automatically update as commits are pushed.</sup>

<!-- ELLIPSIS_HIDDEN -->
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 1543e72 in 40 seconds

More details
  • Looked at 233 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. skyvern-frontend/src/util/env.ts:22
  • Draft comment:
    Consider using a boolean environment variable for VITE_OBSERVER_ENABLED instead of casting it to a string and comparing it to "true".
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The observerEnabled variable is cast to a string, but it should be a boolean since it's being compared to the string "true". This can be simplified by directly using a boolean environment variable.
2. skyvern-frontend/src/components/ui/select.tsx:117
  • Draft comment:
    CustomSelectItem and SelectItem have similar implementations. Consider refactoring to avoid code duplication.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The CustomSelectItem and SelectItem components have similar implementations. Consider refactoring to avoid code duplication.
3. skyvern-frontend/src/routes/tasks/create/PromptBox.tsx:164
  • Draft comment:
    Ensure error messages are user-friendly and provide actionable feedback.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The startObserverCruiseMutation and getTaskFromPromptMutation both handle errors by displaying a toast with the error message. This is a good practice for user feedback.

Workflow ID: wflow_OJQ0jIPcV2c0vyR0


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 1543e72 in 1 minute and 33 seconds

More details
  • Looked at 233 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. skyvern-frontend/src/components/ui/select.tsx:117
  • Draft comment:
    CustomSelectItem and SelectItem have similar implementations. Consider reusing one component or extracting common logic to avoid redundancy.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The CustomSelectItem and SelectItem components have almost identical implementations. This redundancy can be reduced by reusing one component or extracting common logic.
2. skyvern-frontend/src/routes/tasks/create/PromptBox.tsx:132
  • Draft comment:
    getClient(credentialGetter) is used in multiple mutation functions. Consider extracting this to a common function to avoid redundancy.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The startObserverCruiseMutation and getTaskFromPromptMutation functions both use getClient(credentialGetter) to get a client. This can be extracted to a common function to avoid redundancy.

Workflow ID: wflow_P44Z0BR1a02YVqxX


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@@ -19,4 +19,13 @@ if (!artifactApiBaseUrl) {
console.warn("artifactApiBaseUrl environment variable was not set");
}

export { apiBaseUrl, environment, envCredential, artifactApiBaseUrl };
const observerEnabled = import.meta.env.VITE_OBSERVER_ENABLED as string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider casting observerEnabled to a boolean directly when importing it, instead of comparing it to the string "true". This will ensure that observerFeatureEnabled is a boolean value.

Suggested change
const observerEnabled = import.meta.env.VITE_OBSERVER_ENABLED as string;
const observerFeatureEnabled = import.meta.env.VITE_OBSERVER_ENABLED === "true";

@@ -114,6 +114,30 @@ const SelectLabel = React.forwardRef<
));
SelectLabel.displayName = SelectPrimitive.Label.displayName;

const CustomSelectItem = React.forwardRef<
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be a duplicate of SelectItem. Consider using the existing component instead.

@msalihaltun msalihaltun merged commit b705593 into main Dec 17, 2024
2 checks passed
@msalihaltun msalihaltun deleted the salih/observer-version-zero branch December 17, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants