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 proxy location to workflow run screen #1158

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Nov 7, 2024

Important

Add proxy location selection to RunWorkflowForm with new ProxyLocation type and update related API types.

  • Types:
    • Add ProxyLocation type in types.ts with options for various residential proxies and None.
    • Update proxy_location fields in CreateTaskRequest, WorkflowApiResponse, WorkflowRunApiResponse, and WorkflowRunStatusApiResponse to use ProxyLocation type.
  • Form:
    • Add proxyLocation field to RunWorkflowForm in RunWorkflowForm.tsx with a select input for choosing proxy locations.
    • Default proxyLocation to Residential in form initial values.
    • Update getRunWorkflowRequestBody to include proxyLocation in the request body.
  • Misc:
    • Refactor onSubmit and cURL generation to use getRunWorkflowRequestBody.

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

…src/'

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Add proxy location selection to `RunWorkflowForm` and update related types to use new `ProxyLocation` type.
>
>   - **Types**:
>     - Add `ProxyLocation` type in `types.ts` with options for various residential proxies and `None`.
>     - Update `proxy_location` fields in `CreateTaskRequest`, `WorkflowApiResponse`, `WorkflowRunApiResponse`, and `WorkflowRunStatusApiResponse` to use `ProxyLocation` type.
>   - **Form**:
>     - Add `proxyLocation` field to `RunWorkflowForm` in `RunWorkflowForm.tsx` with a select input for choosing proxy locations.
>     - Default `proxyLocation` to `Residential` in form initial values.
>     - Update `getRunWorkflowRequestBody` to include `proxyLocation` in the request body.
>   - **Misc**:
>     - Refactor `onSubmit` and cURL generation to use `getRunWorkflowRequestBody`.
>
> <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 b4b2ecd73e1e187f7495522b4df5f7b7fe5ce572. 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 9980bc5 in 17 seconds

More details
  • Looked at 296 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern-frontend/src/routes/workflows/RunWorkflowForm.tsx:113
  • Draft comment:
    Ensure that setting proxyLocation to ProxyLocation.Residential as the default value is the intended behavior.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The proxy_location field is being set to ProxyLocation.Residential by default in the form's initial values. This is a good practice to ensure a default value is selected, but it should be confirmed that this is the intended default behavior.

Workflow ID: wflow_qechdVmCBZq5OBKT


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.

👍 Looks good to me! Reviewed everything up to 9980bc5 in 45 seconds

More details
  • Looked at 296 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. skyvern-frontend/src/routes/workflows/RunWorkflowForm.tsx:113
  • Draft comment:
    Ensure that setting proxyLocation to ProxyLocation.Residential as the default value is the intended behavior.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The proxy_location field is being set to ProxyLocation.Residential by default in the form's initial values. This is a good practice to ensure a default value is selected, but it should be confirmed that this is the intended default behavior.
2. skyvern-frontend/src/routes/workflows/RunWorkflowForm.tsx:157
  • Draft comment:
    Good use of getRunWorkflowRequestBody to ensure consistent request body preparation.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The onSubmit function in RunWorkflowForm is correctly using the getRunWorkflowRequestBody function to prepare the request body, ensuring consistency across different parts of the code.
3. skyvern-frontend/src/routes/workflows/RunWorkflowForm.tsx:93
  • Draft comment:
    Consider explicitly checking for non-null values for webhookCallbackUrl to avoid issues with falsy values like an empty string.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The getRunWorkflowRequestBody function is well-structured, but the webhookCallbackUrl is being checked for truthiness before being added to the body. This could be improved by explicitly checking for non-null values to avoid potential issues with falsy values like an empty string.

Workflow ID: wflow_1AnktOU23AKObrhx


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

@msalihaltun msalihaltun merged commit a6d6965 into main Nov 7, 2024
2 checks passed
@msalihaltun msalihaltun deleted the salih/add-proxy-location-when-running-workflow branch November 7, 2024 17:44
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