Skip to content
New issue

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

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

Already on GitHub? # to your account

Support ISP Proxies in Skyvern #1823

Merged
merged 2 commits into from
Feb 24, 2025
Merged

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Feb 24, 2025

Important

Add support for RESIDENTIAL_ISP proxy location in Skyvern, including UI and timezone handling.

  • Behavior:
    • Adds RESIDENTIAL_ISP to ProxyLocation in types.ts and tasks.py.
    • Updates get_tzinfo_from_proxy() in tasks.py to return ZoneInfo("America/New_York") for RESIDENTIAL_ISP.
  • UI:
    • Adds Residential ISP (US) option to ProxySelector in ProxySelector.tsx.

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

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Add support for ISP proxies in Skyvern by updating proxy configurations and UI components to include `RESIDENTIAL_ISP`.
>
>   - **Backend**:
>     - Add `RESIDENTIAL_ISP` to `ProxyLocation` in `tasks.py`.
>     - Update `build_proxy_config_smartproxy_isp()` in `proxy.py` to handle `RESIDENTIAL_ISP`.
>     - Modify `build_proxy_config()` in `proxy.py` to include `RESIDENTIAL_ISP` logic.
>   - **Frontend**:
>     - Add `ResidentialISP` to `ProxyLocation` in `types.ts`.
>     - Update `ProxySelector.tsx` to include "Residential ISP (US)" option.
>   - **Misc**:
>     - Update `get_tzinfo_from_proxy()` in `tasks.py` to return `ZoneInfo("America/New_York")` for `RESIDENTIAL_ISP`.
>
> <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 af0efa4dfa277dffbcfdfc995b3578be6c258cd4. It will automatically update as commits are pushed.</sup>

<!-- ELLIPSIS_HIDDEN -->
…src/'

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Add support for ISP proxies in Skyvern by updating proxy configurations and UI components to include `RESIDENTIAL_ISP`.
>
>   - **Backend**:
>     - Add `RESIDENTIAL_ISP` to `ProxyLocation` in `tasks.py`.
>     - Update `build_proxy_config_smartproxy_isp()` in `proxy.py` to handle `RESIDENTIAL_ISP`.
>     - Modify `build_proxy_config()` in `proxy.py` to include `RESIDENTIAL_ISP` logic.
>   - **Frontend**:
>     - Add `ResidentialISP` to `ProxyLocation` in `types.ts`.
>     - Update `ProxySelector.tsx` to include "Residential ISP (US)" option.
>   - **Misc**:
>     - Update `get_tzinfo_from_proxy()` in `tasks.py` to return `ZoneInfo("America/New_York")` for `RESIDENTIAL_ISP`.
>
> <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 af0efa4dfa277dffbcfdfc995b3578be6c258cd4. 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 4de9fe1 in 35 seconds

More details
  • Looked at 26 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. skyvern-frontend/src/api/types.ts:41
  • Draft comment:
    Add a comment to explain the purpose of ResidentialISP if needed.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
2. skyvern-frontend/src/components/ProxySelector.tsx:24
  • Draft comment:
    Double-check the label 'Residential ISP (US)' for consistency with its value.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
3. skyvern-frontend/src/api/types.ts:41
  • Draft comment:
    Consider adding a brief comment clarifying that 'ResidentialISP' represents a US-based ISP proxy and check if its ordering aligns with the UI design.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
4. skyvern-frontend/src/components/ProxySelector.tsx:24
  • Draft comment:
    Verify that the label 'Residential ISP (US)' and its position in the dropdown meet the intended design and are consistent with the backend ordering.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None

Workflow ID: wflow_TIoLpAXGTT4bZxVz


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 4de9fe1 in 2 minutes and 25 seconds

More details
  • Looked at 48 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 drafted comments based on config settings.
1. skyvern-frontend/src/api/types.ts:41
  • Draft comment:
    ResidentialISP added to ProxyLocation; ensure consistency with backend enum.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    This comment violates several rules. It's asking for verification ("ensure consistency") rather than pointing out a specific issue. It's speculative - it assumes there might be an inconsistency without evidence. If there was an actual inconsistency with the backend, it would likely cause build or runtime errors that would be caught automatically.
    Maybe there's a real risk of enum mismatches that aren't caught by the build system? Maybe this is a critical part of the system where double-checking is warranted?
    Even if enum consistency is important, this kind of verification comment isn't actionable. If there was a mismatch, it should be pointed out directly with evidence, not as a suggestion to check.
    The comment should be deleted as it's asking for verification without providing evidence of an actual issue, and any real enum mismatches would likely be caught by the build system.
2. skyvern-frontend/src/components/ProxySelector.tsx:24
  • Draft comment:
    Adds 'Residential ISP (US)' option; ordering and label are clear.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states that the addition is good and clear, which violates the rule against making purely informative comments.
3. skyvern/forge/sdk/schemas/tasks.py:29
  • Draft comment:
    New RESIDENTIAL_ISP enum member and tzinfo mapping; confirm timezone (America/New_York) is accurate.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to confirm the accuracy of a timezone mapping, which violates the rule against asking for confirmation of intention or accuracy. It does not provide a specific suggestion or point out a clear issue with the code.
4. skyvern-frontend/src/api/types.ts:40
  • Draft comment:
    Ensure the new 'ResidentialISP' key follows consistent naming and documentation conventions.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
5. skyvern-frontend/src/components/ProxySelector.tsx:24
  • Draft comment:
    Confirm that labeling 'Residential ISP (US)' and placement in the select list align with backend logic.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
6. skyvern/forge/sdk/schemas/tasks.py:29
  • Draft comment:
    Verify that mapping 'RESIDENTIAL_ISP' to ZoneInfo('America/New_York') is intentional and aligns with business rules.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
7. skyvern/forge/sdk/schemas/tasks.py:293
  • Draft comment:
    Typo detected: The f-string in the ValueError at line 293 is missing a closing parenthesis. It should be something like:

raise ValueError(f"status_requires_failure_reason({status},{self.task_id})")

Please add the missing parenthesis.

  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. skyvern/forge/sdk/schemas/tasks.py:296
  • Draft comment:
    Typo detected: The f-string in the ValueError at line 296 is missing a closing parenthesis. It should be:

raise ValueError(f"status_requires_extracted_information({status},{self.task_id})")

Please fix this typo.

  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_f8CT9lpb7ZdQko8d


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

@suchintan suchintan merged commit 23744b3 into main Feb 24, 2025
7 checks passed
@suchintan suchintan deleted the suchintan.support-isp-proxies branch February 24, 2025 14:29
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants