feat: add check for selector visibility, revert domcontentloaded to networkidle state
#257
Conversation
WalkthroughThe pull request introduces enhancements to the Changes
Sequence DiagramsequenceDiagram
participant Interpreter
participant Page
participant Workflow
Interpreter->>Page: setDefaultNavigationTimeout(100000)
Interpreter->>Workflow: getSelectors()
Workflow-->>Interpreter: Unique Selectors
Interpreter->>Page: waitForSelector(selector, {state: 'attached', timeout: 10000})
Page-->>Interpreter: Selector State
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
maxun-core/src/interpret.ts (3)
195-196: Remove commented-out code to improve readabilityLines 195-196 contain commented-out code. If this code is no longer needed, consider removing it to keep the codebase clean and improve readability.
Line range hint
172-179: Review the early return ingetSelectors; it might not collect all selectorsIn the
getSelectorsmethod, the loop iterates from the end of the workflow to the beginning. Upon finding the first non-emptyselectors, it adds them toselectorsSetand immediately returns. This means that only selectors from the last workflow step with selectors are collected, and earlier selectors are not considered. If the intention is to collect all unique selectors from the entire workflow, you might want to remove thereturnstatement inside the loop so that the loop continues through all steps.Apply this diff to collect all selectors:
for (let index = workflow.length - 1; index >= 0; index--) { const currentSelectors = workflow[index]?.where?.selectors; if (currentSelectors && currentSelectors.length > 0) { currentSelectors.forEach((selector) => selectorsSet.add(selector)); - return Array.from(selectorsSet); } } return Array.from(selectorsSet);
781-782: Consider making the navigation timeout configurableSetting the default navigation timeout to 100,000 milliseconds (100 seconds) in the
runmethod may not be appropriate in all cases. Long timeouts can mask issues with slow-loading pages and may lead to inefficient resource usage. Consider making this timeout a configurable parameter or handling timeouts on a per-case basis to provide flexibility.
|
|
||
| const presentSelectors: SelectorArray = await Promise.all( | ||
| selectors.map(async (selector) => { | ||
| try { | ||
| await page.waitForSelector(selector, { state: 'attached' }); | ||
| return [selector]; | ||
| } catch (e) { | ||
| return []; | ||
| } | ||
| }), | ||
| ).then((x) => x.flat()); |
There was a problem hiding this comment.
Potential issue: waitForSelector with state: 'attached' doesn't ensure visibility or interactability
The updated code uses waitForSelector with state: 'attached', which only ensures that the elements are present in the DOM, not that they are visible or interactable. If the goal is to interact with or scrape these elements, you should use state: 'visible' to ensure the elements are visible. Alternatively, consider checking for both visibility and enabled state to ensure the elements are actionable.
Apply this diff to ensure selectors are visible:
const presentSelectors: SelectorArray = await Promise.all(
selectors.map(async (selector) => {
try {
- await page.waitForSelector(selector, { state: 'attached' });
+ await page.waitForSelector(selector, { state: 'visible' });
return [selector];
} catch (e) {
return [];
}
}),
).then((x) => x.flat());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const presentSelectors: SelectorArray = await Promise.all( | |
| selectors.map(async (selector) => { | |
| try { | |
| await page.waitForSelector(selector, { state: 'attached' }); | |
| return [selector]; | |
| } catch (e) { | |
| return []; | |
| } | |
| }), | |
| ).then((x) => x.flat()); | |
| const presentSelectors: SelectorArray = await Promise.all( | |
| selectors.map(async (selector) => { | |
| try { | |
| await page.waitForSelector(selector, { state: 'visible' }); | |
| return [selector]; | |
| } catch (e) { | |
| return []; | |
| } | |
| }), | |
| ).then((x) => x.flat()); |
amhsirak
changed the title
domcontentloaded to networkidle state
Summary by CodeRabbit
New Features
Bug Fixes
Chores