Skip if selected #604
Skip if selected #604
Conversation
![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 ecd7cef in 48 seconds
More details
- Looked at
94lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_NpYpT8zh3f1Z7pU1
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.
| @@ -828,9 +820,16 @@ async def normal_select( | |||
| action: actions.SelectOptionAction, | |||
| skyvern_element: SkyvernElement, | |||
| ) -> List[ActionResult]: | |||
| try: | |||
| current_text = await skyvern_element.get_attr("selected") | |||
There was a problem hiding this comment.
The change from using locator.input_value() to skyvern_element.get_attr("selected") might not always reflect the current state of the UI, especially in dynamic web applications. Consider verifying this approach's reliability in handling dynamically updated select elements.
There was a problem hiding this comment.
A good point here. Such as if the previous action caused the select value reset, the logic might be wrong. But maybe we can re-do the select in the next step. So it's not a blocker here.
| action_result: List[ActionResult] = [] | ||
| is_success = False | ||
| locator = skyvern_element.locator | ||
| locator = skyvern_element.get_locator() |
There was a problem hiding this comment.
whoa seems like a big bug
There was a problem hiding this comment.
? What do you mean? I don't follow
There was a problem hiding this comment.
Sorry for the confusion! Did the old.locator implementation work? If not that's a huge bug! Nice find.
There was a problem hiding this comment.
Oh both of them could work. I just refactor it into the same format.
| @@ -828,9 +820,16 @@ async def normal_select( | |||
| action: actions.SelectOptionAction, | |||
| skyvern_element: SkyvernElement, | |||
| ) -> List[ActionResult]: | |||
| try: | |||
There was a problem hiding this comment.
I don't think this is a good change. How can the LLM "unselect" an option? Here doing select option on an already selected option would unselect it before, but now that can never be the case right?
There was a problem hiding this comment.
What do you mean unselect before select? I don't understand.
There was a problem hiding this comment.
If llm want to unselect sth. He can return the value represented the "unselect" right? What the problem here?
| return [selectOptions, ""]; | ||
| } | ||
|
|
||
| return [selectOptions, removeMultipleSpaces(selectedOption.textContent)]; |
There was a problem hiding this comment.
is the selected value always textContent? could it be the "value" attribute of the element?
There was a problem hiding this comment.
because when we parse the option. We only use the textContext to represent the value. I think we should be consistent with the logic we do in the option parsing.
| if current_text == action.option.label or current_text == action.option.value: | ||
| return [ActionSuccess()] | ||
| except Exception: | ||
| LOG.info("failed to confirm if the select option has been done, force to take the action again.") |
There was a problem hiding this comment.
why do we say "force to take the action again"? we're not retrying here, are we?
There was a problem hiding this comment.
em. if the skip logic failed. we try to select again!
Summary:
Refactored
handle_select_option_actionto check for already selected options innormal_selectand updatedgetSelectOptionsto return the selected option's text.Key points:
handle_select_option_actioninskyvern/webeye/actions/handler.pyto remove the check for already selected options.normal_selectinskyvern/webeye/actions/handler.py.getSelectOptionsinskyvern/webeye/scraper/domUtils.jsto return the selected option's text along with the options list.selectedattribute inbuildTreeFromBodyinskyvern/webeye/scraper/domUtils.js.Generated with ❤️ by ellipsis.dev