Skip to content

fix: handle context destroyed and frame navigation #243

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 2 commits into from
Dec 10, 2024
Merged

fix: handle context destroyed and frame navigation #243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
142c90a
fix: handle context destroyed, frame navigation URL
RohitR311 Dec 10, 2024
c7af54e
feat: add fallback mechanism for click and waitForLoadState action
RohitR311 Dec 10, 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
28 changes: 24 additions & 4 deletions maxun-core/src/interpret.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,16 @@ export default class Interpreter extends EventEmitter {
}),
};

const executeAction = async (invokee: any, methodName: string, args: any) => {
console.log("Executing action:", methodName, args);
if (!args || Array.isArray(args)) {
await (<any>invokee[methodName])(...(args ?? []));
} else {
await (<any>invokee[methodName])(args);
}
};
Comment on lines +472 to +479
Copy link

@coderabbitai coderabbitai bot Dec 10, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance Type Safety in executeAction Function

The executeAction function uses any types for invokee, methodName, and args, which can lead to runtime errors due to lack of type checking. Casting with <any> may hide potential issues if the method does not exist or is incorrectly invoked.

Consider specifying more precise types or using generics to improve type safety.

Apply this diff to enhance type safety:

- const executeAction = async (invokee: any, methodName: string, args: any) => {
+ const executeAction = async (invokee: Record<string, Function>, methodName: string, args: any[] | any) => {
    console.log("Executing action:", methodName, args);
    if (!args || Array.isArray(args)) {
      await invokee[methodName](...(args ?? []));
    } else {
      await invokee[methodName](args);
    }
  };
📝 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 executeAction = async (invokee: any, methodName: string, args: any) => {
console.log("Executing action:", methodName, args);
if (!args || Array.isArray(args)) {
await (<any>invokee[methodName])(...(args ?? []));
} else {
await (<any>invokee[methodName])(args);
}
};
const executeAction = async (invokee: Record<string, Function>, methodName: string, args: any[] | any) => {
console.log("Executing action:", methodName, args);
if (!args || Array.isArray(args)) {
await invokee[methodName](...(args ?? []));
} else {
await invokee[methodName](args);
}
};



for (const step of steps) {
this.log(`Launching ${String(step.action)}`, Level.LOG);

Expand All @@ -486,10 +496,20 @@ export default class Interpreter extends EventEmitter {
invokee = invokee[level];
}

if (!step.args || Array.isArray(step.args)) {
await (<any>invokee[methodName])(...(step.args ?? []));
if (methodName === 'waitForLoadState') {
try {
await executeAction(invokee, methodName, step.args);
} catch (error) {
await executeAction(invokee, methodName, 'domcontentloaded');
}
} else if (methodName === 'click') {
try {
await executeAction(invokee, methodName, step.args);
} catch (error) {
await executeAction(invokee, methodName, [step.args[0], { force: true }]);
}
} else {
await (<any>invokee[methodName])(step.args);
await executeAction(invokee, methodName, step.args);
}
}

Expand Down Expand Up @@ -571,7 +591,7 @@ export default class Interpreter extends EventEmitter {
return allResults;
}
// Click the 'Load More' button to load additional items
await loadMoreButton.click();
await loadMoreButton.dispatchEvent('click');
await page.waitForTimeout(2000); // Wait for new items to load
// After clicking 'Load More', scroll down to load more items
await page.evaluate(() => window.scrollTo(0, document.body.scrollHeight));
Expand Down
94 changes: 64 additions & 30 deletions server/src/browser-management/classes/RemoteBrowser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ export class RemoteBrowser {
maxRepeats: 1,
};

private lastEmittedUrl: string | null = null;

/**
* {@link WorkflowGenerator} instance specific to the remote browser.
*/
Expand All @@ -88,6 +90,64 @@ export class RemoteBrowser {
this.generator = new WorkflowGenerator(socket);
}

/**
* Normalizes URLs to prevent navigation loops while maintaining consistent format
*/
private normalizeUrl(url: string): string {
try {
const parsedUrl = new URL(url);
// Remove trailing slashes except for root path
parsedUrl.pathname = parsedUrl.pathname.replace(/\/+$/, '') || '/';
// Ensure consistent protocol handling
parsedUrl.protocol = parsedUrl.protocol.toLowerCase();
return parsedUrl.toString();
} catch {
return url;
}
}

/**
* Determines if a URL change is significant enough to emit
*/
private shouldEmitUrlChange(newUrl: string): boolean {
if (!this.lastEmittedUrl) {
return true;
}
const normalizedNew = this.normalizeUrl(newUrl);
const normalizedLast = this.normalizeUrl(this.lastEmittedUrl);
return normalizedNew !== normalizedLast;
}

private async setupPageEventListeners(page: Page) {
page.on('framenavigated', async (frame) => {
if (frame === page.mainFrame()) {
const currentUrl = page.url();
if (this.shouldEmitUrlChange(currentUrl)) {
this.lastEmittedUrl = currentUrl;
this.socket.emit('urlChanged', currentUrl);
}
}
});

// Handle page load events with retry mechanism
page.on('load', async () => {
const injectScript = async (): Promise<boolean> => {
try {
await page.waitForLoadState('networkidle', { timeout: 5000 });

await page.evaluate(getInjectableScript());
return true;
} catch (error: any) {
logger.log('warn', `Script injection attempt failed: ${error.message}`);
return false;
}
};

const success = await injectScript();
console.log("Script injection result:", success);
});
}

Comment on lines +121 to +150
Copy link

@coderabbitai coderabbitai bot Dec 10, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Prevent Duplicate Event Listeners on Pages

The setupPageEventListeners method adds event listeners to the page. If this method is called multiple times on the same page instance, it could result in multiple listeners being registered, causing duplicate events.

Consider removing existing event listeners before adding new ones or checking if listeners already exist.

Apply this diff to prevent duplicate listeners:

+ // Remove existing listeners to prevent duplication
+ page.removeListener('framenavigated', this.framenavigatedHandler);
+ page.removeListener('load', this.loadHandler);

+ // Define handlers as class properties to maintain references
+ private framenavigatedHandler = async (frame: Frame) => { /* existing code */ };
+ private loadHandler = async () => { /* existing code */ };

page.on('framenavigated', this.framenavigatedHandler);
page.on('load', this.loadHandler);

Alternatively, ensure setupPageEventListeners is called only once per page instance.

Committable suggestion skipped: line range outside the PR's diff.

/**
* An asynchronous constructor for asynchronously initialized properties.
* Must be called right after creating an instance of RemoteBrowser class.
Expand Down Expand Up @@ -167,15 +227,7 @@ export class RemoteBrowser {
this.context = await this.browser.newContext(contextOptions);
this.currentPage = await this.context.newPage();

this.currentPage.on('framenavigated', (frame) => {
if (frame === this.currentPage?.mainFrame()) {
this.socket.emit('urlChanged', this.currentPage.url());
}
});

this.currentPage.on('load', (page) => {
page.evaluate(getInjectableScript())
})
await this.setupPageEventListeners(this.currentPage);

// await this.currentPage.setExtraHTTPHeaders({
// 'User-Agent': 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.3'
Expand Down Expand Up @@ -375,15 +427,7 @@ export class RemoteBrowser {
await this.stopScreencast();
this.currentPage = page;

this.currentPage.on('framenavigated', (frame) => {
if (frame === this.currentPage?.mainFrame()) {
this.socket.emit('urlChanged', this.currentPage.url());
}
});

this.currentPage.on('load', (page) => {
page.evaluate(getInjectableScript())
})
await this.setupPageEventListeners(this.currentPage);

//await this.currentPage.setViewportSize({ height: 400, width: 900 })
this.client = await this.currentPage.context().newCDPSession(this.currentPage);
Expand Down Expand Up @@ -411,18 +455,8 @@ export class RemoteBrowser {
await this.currentPage?.close();
this.currentPage = newPage;
if (this.currentPage) {
this.currentPage.on('framenavigated', (frame) => {
if (frame === this.currentPage?.mainFrame()) {
this.socket.emit('urlChanged', this.currentPage.url());
}
});

this.currentPage.on('load', (page) => {
page.evaluate(getInjectableScript())
})
// this.currentPage.on('load', (page) => {
// this.socket.emit('urlChanged', page.url());
// })
await this.setupPageEventListeners(this.currentPage);

this.client = await this.currentPage.context().newCDPSession(this.currentPage);
await this.subscribeToScreencast();
} else {
Expand Down