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

optimize multiple select #1702

Closed
wants to merge 1 commit into from
Closed

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Feb 3, 2025

Important

Optimizes multi-selection handling by refining dropdown logic and context management, and updates an exception message.

  • Behavior:
    • Refines multi-selection logic in confirm-multi-selection-finish.j2 to handle mini goals and multi-level dropdowns.
    • Updates handle_select_option_action() and sequentially_select_from_dropdown() in handler.py to improve dropdown handling.
  • Context Handling:
    • Adds intention field to InputOrSelectContext in actions.py and updates related logic in handler.py.
  • Exceptions:
    • Updates message in NoIncrementalElementFoundForCustomSelection in exceptions.py to suggest retrying later or with another element.

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

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Optimizes multi-level dropdown selection handling by adding intention attributes and refining completion logic, with updated prompts and exception messages.
>
>   - **Behavior**:
>     - Adds `intention` attribute to `InputOrSelectContext` and `SelectOptionAction` in `actions.py` and `handler.py` to better handle multi-level dropdown selections.
>     - Updates logic in `sequentially_select_from_dropdown()` in `handler.py` to determine when a mini goal is achieved in dropdown selections.
>   - **Prompts**:
>     - Updates `confirm-multi-selection-finish.j2` to include `is_mini_goal_finished` and `is_multiple_selection` keys for better JSON response handling.
>   - **Exceptions**:
>     - Modifies message in `NoIncrementalElementFoundForCustomSelection` in `exceptions.py` for clarity.
>
> <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 f7b2a1d0cf514e036c40ecaa2ad52db5b7e18cc8. 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.

❌ Changes requested. Incremental review on 5dac5aa in 1 minute and 8 seconds

More details
  • Looked at 209 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 12 drafted comments based on config settings.
1. skyvern/exceptions.py:453
  • Draft comment:
    Consider whether the new simplified error message provides enough guidance compared to the previous one.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    None
2. skyvern/forge/prompts/skyvern/confirm-multi-selection-finish.j2:4
  • Draft comment:
    Typo: 'acheived' should be 'achieved'.
  • Reason this comment was not posted:
    Marked as duplicate.
3. skyvern/forge/prompts/skyvern/confirm-multi-selection-finish.j2:5
  • Draft comment:
    Typo: 'Somtimes' should be 'Sometimes'.
  • Reason this comment was not posted:
    Marked as duplicate.
4. skyvern/webeye/actions/actions.py:85
  • Draft comment:
    Added 'intention' to InputOrSelectContext repr. Ensure documentation reflects this additional field.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    None
5. skyvern/webeye/actions/handler.py:2054
  • Draft comment:
    The pattern for deriving field_information using 'context.field if not context.intention else context.intention' is repeated multiple times. Consider extracting it into a helper function to avoid duplication.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    None
6. skyvern/webeye/actions/handler.py:2067
  • Draft comment:
    Ensure that switching from 'is_finished' to 'is_mini_goal_finished' is consistently handled throughout the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 30%
    None
7. skyvern/webeye/actions/handler.py:765
  • Draft comment:
    Propagating the 'intention' field in the JSON response improves context. Confirm that all downstream consumers of InputOrSelectContext can handle the additional 'intention' field.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    None
8. skyvern/exceptions.py:453
  • Draft comment:
    Updated exception message in NoIncrementalElementFoundForCustomSelection. Consider whether retaining some diagnostic hints might aid debugging.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment suggests keeping diagnostic hints, but the new message is actually clearer and simpler. The old message made assumptions about what actions might work that could be misleading. The new message gives clearer, more general guidance. Comments about error message wording are generally not critical unless the new message is clearly wrong or missing key information.
    The comment could be right that more specific diagnostic hints would be helpful for debugging. Maybe there are common cases where specific guidance would help users fix the issue faster.
    While specific hints can be helpful, they can also be misleading if not always accurate. The new message is clearer and avoids potentially incorrect assumptions while still providing actionable guidance.
    The comment should be deleted. The error message change appears to be an intentional simplification that maintains clarity while avoiding potentially misleading specific suggestions.
9. skyvern/webeye/actions/actions.py:77
  • Draft comment:
    The addition of the 'intention' field to InputOrSelectContext and its inclusion in the repr is clear. When later determining 'field_information', consider using a more concise expression (e.g. 'context.intention or context.field').
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The first part of the comment is just stating what's obvious from the diff - the addition of the intention field. The second part makes a suggestion about code we can't see ('field_information' determination). Without seeing this code, we can't verify if the suggestion is valid or useful. The comment is partially descriptive and partially speculative about code not shown.
    I might be too harsh on the descriptive part of the comment - sometimes stating the obvious can help document changes. Also, the suggestion about code patterns could be valuable even without seeing the target code.
    The rules explicitly state not to make purely informative comments or speculative suggestions. We need to see strong evidence that a comment is correct and useful to keep it.
    The comment should be deleted as it combines obvious observations with suggestions about code we can't verify.
10. skyvern/webeye/actions/handler.py:2054
  • Draft comment:
    Consider simplifying the conditional used to compute 'mini_goal'. Using 'input_or_select_context.intention or input_or_select_context.field' would be more idiomatic than the current conditional.
  • Reason this comment was not posted:
    Marked as duplicate.
11. skyvern/webeye/actions/handler.py:762
  • Draft comment:
    Propagating the 'intention' field into the LLM API requests (e.g., setting json_response['intention'] = action.intention) is a good improvement. Ensure that all downstream consumers handle this field consistently.
  • Reason this comment was not posted:
    Comment did not seem useful.
12. skyvern/webeye/actions/handler.py:2510
  • Draft comment:
    There are several fixed sleep durations in the async flows (e.g., 5s, 10s delays). Consider parameterizing these values to ease tuning and improve performance under variable conditions.
  • 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_hQrPWyVLeEj24MUj


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.

Confirm if the user has finished the mini goal in the current opened dropdown selection based on the screenshot, user details, the HTML elements and select history provided in the list.

NOTE:
- Only consider the mini goal is acheived when there is(are) one(several) valid options selected in the dropdown.
Copy link
Contributor

Choose a reason for hiding this comment

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

There are typos in the prompt text: 'acheived' should be 'achieved' and 'Somtimes' should be 'Sometimes'.

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 5dac5aa in 1 minute and 14 seconds

More details
  • Looked at 209 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 9 drafted comments based on config settings.
1. skyvern/forge/prompts/skyvern/confirm-multi-selection-finish.j2:4
  • Draft comment:
    Typo: Change 'acheived' to 'achieved'.
  • Reason this comment was not posted:
    Marked as duplicate.
2. skyvern/forge/prompts/skyvern/confirm-multi-selection-finish.j2:5
  • Draft comment:
    Typo: Change 'Somtimes' to 'Sometimes'.
  • Reason this comment was not posted:
    Marked as duplicate.
3. skyvern/webeye/actions/actions.py:85
  • Draft comment:
    Good enhancement: Including 'intention' in the repr improves debugging.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
4. skyvern/webeye/actions/handler.py:763
  • Draft comment:
    Consistently forward the 'intention' field in json_response. Consider refactoring the 'field_information' assignment logic (repeated ternary operator) into a helper to reduce duplication.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
5. skyvern/exceptions.py:451
  • Draft comment:
    Updated error message in NoIncrementalElementFoundForCustomSelection is simpler. Ensure it still provides enough debugging context.
  • Reason this comment was not posted:
    Comment did not seem useful.
6. skyvern/webeye/actions/actions.py:85
  • Draft comment:
    Including the 'intention' field in the repr method improves debug output. Verify that 'intention' is populated correctly in all contexts.
  • Reason this comment was not posted:
    Comment did not seem useful.
7. skyvern/webeye/actions/handler.py:582
  • Draft comment:
    Passing 'intention=action.intention' when creating SelectOptionAction ensures the new context field is propagated. Confirm this integration across all action handlers.
  • Reason this comment was not posted:
    Marked as duplicate.
8. skyvern/webeye/actions/handler.py:2618
  • Draft comment:
    Consider refactoring the repetitive assignment of field_information to 'context.intention or context.field' for conciseness.
  • Reason this comment was not posted:
    Marked as duplicate.
9. skyvern/webeye/actions/handler.py:1953
  • Draft comment:
    Setting 'json_response["intention"] = action.intention' before validation propagates the new field. Verify that this does not override crucial data when 'intention' is None.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_EZaUMlE5az9nisSk


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.

Confirm if the user has finished the mini goal in the current opened dropdown selection based on the screenshot, user details, the HTML elements and select history provided in the list.

NOTE:
- Only consider the mini goal is acheived when there is(are) one(several) valid options selected in the dropdown.
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix typos in the note: change 'acheived' to 'achieved' and 'Somtimes' to 'Sometimes'.

@LawyZheng LawyZheng closed this Feb 3, 2025
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