fix select dropdown issue #1718
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_HIDDEN --> > [!IMPORTANT] > Add error handling to `addIncrementalNodeToMap` in `domUtils.js` and remove metadata files from `cloud/extensions/autoplaystopper-1.9.8.1/_metadata/`. > > - **Error Handling**: > - Add `try-catch` block in `addIncrementalNodeToMap` in `domUtils.js` to catch and log errors during incremental node building. > - **File Deletions**: > - Remove `computed_hashes.json` and `verified_contents.json` from `cloud/extensions/autoplaystopper-1.9.8.1/_metadata/`. > > <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 cc82d178f1e80f20c1d7728632dde1958f83b997. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN -->
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 3e4ac92 in 37 seconds
More details
- Looked at
24lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. skyvern/webeye/scraper/domUtils.js:2087
- Draft comment:
Wrapping the loop in a try/catch block is useful to prevent a single error from stopping incremental node processing. However, ensure that errors from one child don't mask issues with others. Consider logging additional context or handling errors per iteration if needed. - Reason this comment was not posted:
Comment did not seem useful.
2. skyvern/webeye/scraper/domUtils.js:1115
- Draft comment:
Consider handling multi-select elements. Using querySelector('option:checked') returns only one option, which may be incorrect for multiple selections. 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. 3. skyvern/webeye/scraper/domUtils.js:1109 Draft comment: Consider trimming the option text after replacing multiple spaces to remove any unintended leading or trailing whitespace. 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. 4. skyvern/webeye/scraper/domUtils.js:1240 Draft comment: Storing the bare selected option text on the select's attributes may be atypical. Consider including additional metadata (e.g., option value or index) for clarity. 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_9OgK8bg4U2HQcOGp You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 3e4ac92 in 1 minute and 17 seconds
More details
- Looked at
24lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. skyvern/webeye/scraper/domUtils.js:2088
- Draft comment:
Consider wrapping the call to buildElementTree(child, '', true) inside its own try/catch. This prevents a failure in processing one child from skipping the rest. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_ROHuwT9DRZgbaRWW
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.
| const [_, newNodeTree] = buildElementTree(child, "", true); | ||
| if (newNodeTree.length > 0) { | ||
| newNodesTreeList.push(...newNodeTree); | ||
| try { |
There was a problem hiding this comment.
The try-catch around the entire loop may cause one faulty child to skip processing all subsequent children. Consider wrapping each buildElementTree call in its own try-catch so that one error won't prevent processing of all children.
| } | ||
| } catch (error) { | ||
| console.error("Error building incremental element node:", error); |
There was a problem hiding this comment.
Consider including additional context (e.g. a child identifier or depth info) in the error log to ease debugging.
LawyZheng
approved these changes
Feb 4, 2025
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 error handling in
addIncrementalNodeToMapindomUtils.jsto catch and log errors during node tree building.try-catchblock inaddIncrementalNodeToMapindomUtils.jsto catch errors during node tree building and log them to the console.This description was created by
for 3e4ac92. It will automatically update as commits are pushed.