Skip to content

feat: add check for selector visibility, revert domcontentloaded to networkidle state #257

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Dec 14, 2024
Merged

feat: add check for selector visibility, revert domcontentloaded to networkidle state #257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
18b517c
chore: remove note
amhsirak Dec 12, 2024
ac13cd8
chore: remove clone instruction
amhsirak Dec 12, 2024
4469325
chore: sync compose master <-> develop
amhsirak Dec 14, 2024
7f48464
feat: add page navigation timeout
RohitR311 Dec 14, 2024
bdf908e
feat: add domcontentloaded wait load state
RohitR311 Dec 14, 2024
f38230d
feat: revert to networkidle for wait load state
RohitR311 Dec 14, 2024
7ce7a15
feat: check for selector visibility in getState
RohitR311 Dec 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,9 @@ Maxun lets you train a robot in 2 minutes and scrape the web on auto-pilot. Web

<img src="https://static.scarf.sh/a.png?x-pxid=c12a77cc-855e-4602-8a0f-614b2d0da56a" />

> Note: Maxun is in its early stages of development and currently does not support self-hosting. However, you can run Maxun locally. Self-hosting capabilities are planned for a future release and will be available soon.

# Local Installation
# Installation
### Docker Compose
```
git clone https://github.com/getmaxun/maxun
docker-compose up -d
```
You can access the frontend at http://localhost:5173/ and backend at http://localhost:8080/
Expand Down
7 changes: 1 addition & 6 deletions docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,6 @@ services:
- redis
- minio
volumes:
- ./server:/app/server # Mount server source code for hot reloading
- ./maxun-core:/app/maxun-core # Mount maxun-core for any shared code updates
- /var/run/dbus:/var/run/dbus

frontend:
Expand All @@ -79,13 +77,10 @@ services:
environment:
PUBLIC_URL: ${PUBLIC_URL}
BACKEND_URL: ${BACKEND_URL}
volumes:
- ./:/app # Mount entire frontend app directory for hot reloading
- /app/node_modules # Anonymous volume to prevent overwriting node_modules
depends_on:
- backend

volumes:
postgres_data:
minio_data:
redis_data:
redis_data:
19 changes: 16 additions & 3 deletions maxun-core/src/interpret.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,8 +192,8 @@ export default class Interpreter extends EventEmitter {
// const actionable = async (selector: string): Promise<boolean> => {
// try {
// const proms = [
// page.isEnabled(selector, { timeout: 5000 }),
// page.isVisible(selector, { timeout: 5000 }),
// page.isEnabled(selector, { timeout: 10000 }),
// page.isVisible(selector, { timeout: 10000 }),
// ];

// return await Promise.all(proms).then((bools) => bools.every((x) => x));
Expand All @@ -214,6 +214,17 @@ export default class Interpreter extends EventEmitter {
// return [];
// }),
// ).then((x) => x.flat());

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());
Comment on lines +217 to +227
Copy link

@coderabbitai coderabbitai bot Dec 14, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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());


const action = workflowCopy[workflowCopy.length - 1];

Expand All @@ -233,7 +244,7 @@ export default class Interpreter extends EventEmitter {
...p,
[cookie.name]: cookie.value,
}), {}),
selectors,
selectors: presentSelectors,
};
}

Expand Down Expand Up @@ -767,6 +778,8 @@ export default class Interpreter extends EventEmitter {
public async run(page: Page, params?: ParamType): Promise<void> {
this.log('Starting the workflow.', Level.LOG);
const context = page.context();

page.setDefaultNavigationTimeout(100000);

// Check proxy settings from context options
const contextOptions = (context as any)._options;
Expand Down