close the new page/window opened by file download action

[wintonzheng]

Important

Adds logic in handle_click_action() to close extra pages opened during file downloads and refactors set_download_file_listener() for clarity.

• Behavior:
  • In handle_click_action() in handler.py, added logic to close any extra page opened during a file download action.
  • Logs page count before and after download to identify if an extra page was opened.
• Misc:
  • Minor refactoring in set_download_file_listener() in browser_factory.py to improve code clarity.

^{This description was created by for 03b18fe. It will automatically update as commits are pushed.}

[ellipsis-dev]

👍 Looks good to me! Incremental review on 48f1d62 in 44 seconds

More details

• Looked at 89 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. skyvern/webeye/actions/handler.py:381

• Draft comment:
  Add a check for browser_state.browser_context before accessing pages to avoid potential AttributeError.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. skyvern/webeye/browser_factory.py:68

• Draft comment:
  The outer try-except block around async with asyncio.timeout is redundant. Consider removing it to simplify the code.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The set_download_file_listener function in browser_factory.py has a redundant try-except block around the async with asyncio.timeout context manager. The exception handling for asyncio.TimeoutError is already covered, so the outer try-except block is unnecessary.

3. skyvern/webeye/scraper/domUtils.js:1136

• Draft comment:
  The check for elementTagNameLower === "input" || elementTagNameLower === "textarea" is redundant here. Consider removing it to simplify the code.
• Reason this comment was not posted:
  Confidence changes required: 50%
  In domUtils.js, the buildElementObject function has a redundant check for elementTagNameLower === "input" || elementTagNameLower === "textarea" before setting attrs["value"]. This check is already handled earlier in the function.

Workflow ID: wflow_VaRlum2TcS8gPadc

---

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

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to 48f1d62 in 44 seconds

More details

• Looked at 89 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. skyvern/webeye/actions/handler.py:401

• Draft comment:
  Add a check to ensure browser_state.browser_context.pages is not empty before accessing the last page to close it. This prevents potential index errors.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. skyvern/webeye/browser_factory.py:66

• Draft comment:
  Retrieve workflow_run_id and task_id once at the beginning of listen_to_download to avoid redundant calls to kwargs.get().
• Reason this comment was not posted:
  Confidence changes required: 50%
  In set_download_file_listener in browser_factory.py, the workflow_run_id and task_id are retrieved multiple times. This can be optimized by retrieving them once and using the variables.

3. skyvern/webeye/scraper/domUtils.js:1136

• Draft comment:
  The logic for masking password input values has been removed. Ensure this change aligns with security requirements for handling sensitive data.
• Reason this comment was not posted:
  Comment did not seem useful.

Workflow ID: wflow_MeuVKlI3BCUUGXo8

---

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

[LawyZheng]

can you revert this change? it looks like something weired happened in syng PR 😂

[ellipsis-dev]

👍 Looks good to me! Incremental review on 9b9edbb in 33 seconds

More details

• Looked at 17 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. skyvern/webeye/scraper/domUtils.js:1136

• Draft comment:
  The refactoring for password input handling was not applied here. Consider updating this section to match the refactored logic.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

Workflow ID: wflow_Iu1UhuQmhDFuwJIE

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on 03b18fe in 31 seconds

More details

• Looked at 18 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. skyvern/webeye/actions/handler.py:400

• Draft comment:
  The check if page == browser_state.browser_context.pages[-1]: is redundant since the page is closed regardless. Consider removing this check and the associated log warning to simplify the code.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The check is not redundant because it serves a purpose beyond just closing the page; it provides a specific log message that could be useful for understanding the state of the application. Removing the check would remove this logging, which might be valuable.
  I might be overestimating the importance of the log message. If the log message is not critical, the check could indeed be considered redundant.
  The log message provides specific information that could be useful for debugging, so the check is not redundant. The comment overlooks the purpose of the log message.
  The comment should be deleted because the check is not redundant; it serves the purpose of providing a specific log message.

Workflow ID: wflow_41NSwyfVXdQFo3rD

---

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