UrlBlock - a block to navigate to a url #1461
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to cb50d97 in 1 minute and 59 seconds
More details
- Looked at
137lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_jVv8fXYlRQqHsmqE
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.
| block_type: Literal[BlockType.GOTO_URL] = BlockType.GOTO_URL | ||
| url: str | ||
|
|
||
| async def execute( |
There was a problem hiding this comment.
Consider using prepend_scheme_and_validate_url to validate the URL before navigation to ensure it is well-formed.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on a891ee3 in 35 seconds
More details
- Looked at
89lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. skyvern/forge/sdk/workflow/models/block.py:1809
- Draft comment:
TheUrlBlockclass is missing theexecutemethod, which is necessary for its functionality. The PR description mentions implementing theexecute()method, but it is not present in the code. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is incorrect. The UrlBlock is inheriting from BaseTaskBlock which already has an execute() implementation. This is actually a refactoring to remove duplicate code by reusing the BaseTaskBlock's execute() method. The PR is making the code better by reducing duplication.
Could there be a reason why UrlBlock needed its own custom execute() implementation that's being lost in this change?
Looking at the old execute() implementation, it was doing the same basic task handling and browser management that BaseTaskBlock does. There's no special functionality being lost - this is a clean refactoring.
The comment is incorrect and should be deleted. The execute() method isn't missing - it's intentionally inheriting the implementation from BaseTaskBlock as part of a good refactoring.
Workflow ID: wflow_PWcIcdcziyFUIBmo
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
wintonzheng
force-pushed
the
shu/add_go_to_url_block
branch
from
a891ee3 to
ca18f3e
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on ca18f3e in 37 seconds
More details
- Looked at
114lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. skyvern/forge/sdk/workflow/models/block.py:1805
- Draft comment:
TheUrlBlockclass is missing anexecutemethod. This method is essential for the block to perform its intended function of navigating to a URL. Please reintroduce theexecutemethod with the necessary logic. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests adding back the execute method, but this would be incorrect. The change to inherit from BaseTaskBlock is intentional and good - it reuses the common task execution logic. The old execute method was duplicating functionality that already exists in BaseTaskBlock. The comment shows a misunderstanding of the codebase architecture.
Could there be some special URL handling logic in the old execute method that was important to preserve? Could this change break existing functionality?
Looking at the old execute method, it was just doing basic URL navigation which BaseTaskBlock already handles. There's no special logic being lost. The change actually reduces code duplication and improves maintainability.
The comment should be deleted. The change to inherit from BaseTaskBlock is correct and the execute method is not missing - it's properly inherited from BaseTaskBlock.
Workflow ID: wflow_11DOrfYfdBAFjs9s
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Add
UrlBlockandUrlBlockYAMLfor URL navigation in workflows with YAML configuration support.UrlBlockclass inblock.pyfor URL navigation in workflows.execute()method inUrlBlockfor URL navigation and error handling.UrlBlockYAMLclass inyaml.pyfor YAML configuration ofUrlBlock.BLOCK_YAML_SUBCLASSESandBLOCK_YAML_TYPESto includeUrlBlockYAML.This description was created by
for ca18f3e. It will automatically update as commits are pushed.