fix: handle context destroyed and frame navigation #243
Conversation
WalkthroughThe pull request introduces several enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 2
🧹 Outside diff range and nitpick comments (3)
maxun-core/src/interpret.ts (2)
499-512: Improve Error Logging in Action ExecutionIn the error handling for
waitForLoadStateandclickmethods, the caught errors are not logged. This can make debugging difficult when an action fails.Consider logging the errors to provide better visibility into any issues during execution.
Apply this diff to log errors:
if (methodName === 'waitForLoadState') { try { await executeAction(invokee, methodName, step.args); } catch (error) { + this.log(`Error in waitForLoadState with args ${JSON.stringify(step.args)}: ${error.message}`, Level.ERROR); await executeAction(invokee, methodName, 'domcontentloaded'); } } else if (methodName === 'click') { try { await executeAction(invokee, methodName, step.args); } catch (error) { + this.log(`Error in click with args ${JSON.stringify(step.args)}: ${error.message}`, Level.ERROR); await executeAction(invokee, methodName, [step.args[0], { force: true }]); } } else { await executeAction(invokee, methodName, step.args); }
594-594: Use Playwright'sclick()Method Instead ofdispatchEvent('click')In the pagination handling,
dispatchEvent('click')is used to simulate a click on the "Load More" button. This approach may bypass Playwright's built-in event handling, potentially leading to flaky tests or missed actions.Consider using Playwright's
click()method, which ensures proper event dispatching and waits for the element to be actionable.Apply this diff:
- await loadMoreButton.dispatchEvent('click'); + await loadMoreButton.click();server/src/browser-management/classes/RemoteBrowser.ts (1)
133-148: Enhance Retry Mechanism for Script InjectionIn the
loadevent listener, if the script injection fails, theinjectScriptfunction only attempts once. Network delays or slow page loads might require additional retry attempts.Consider implementing a retry mechanism with multiple attempts and delays between retries to improve reliability.
Example modification:
const injectScript = async (): Promise<boolean> => { + const maxAttempts = 3; + for (let attempt = 1; attempt <= maxAttempts; attempt++) { try { await page.waitForLoadState('networkidle', { timeout: 5000 }); await page.evaluate(getInjectableScript()); return true; } catch (error: any) { logger.log('warn', `Script injection attempt ${attempt} failed: ${error.message}`); + if (attempt === maxAttempts) { + return false; + } + await page.waitForTimeout(1000 * attempt); // Exponential backoff } + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
maxun-core/src/interpret.ts(3 hunks)server/src/browser-management/classes/RemoteBrowser.ts(5 hunks)
🔇 Additional comments (2)
server/src/browser-management/classes/RemoteBrowser.ts (2)
93-108: Ensure Comprehensive URL Normalization
The normalizeUrl method may not account for all URL variations, such as default ports, query parameters, or hash fragments, which could affect URL comparison.
Please verify whether additional normalization is required for accurate URL comparisons, especially if URLs differ only by trailing slashes, ports, or case sensitivity.
✅ Verification successful
Let me check the usage context of normalizeUrl to better understand if additional normalization is needed.
Current URL normalization is sufficient for its use case
The normalizeUrl method is only used within the shouldEmitUrlChange method to compare URLs and prevent duplicate navigation events. The current implementation adequately handles the essential aspects:
- Normalizes protocol case
- Handles trailing slashes consistently
- Provides fallback for invalid URLs
- Maintains URL object's built-in normalization of hostname case and default ports
Since the method is private and only used for equality comparison in navigation change detection, the current level of normalization is appropriate for its specific purpose.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usages of `normalizeUrl` and validate URL comparison logic.
# Search for all calls to `normalizeUrl`
rg 'normalizeUrl'
# Review comparisons and ensure they handle default ports and case sensitivity
Length of output: 358
Script:
#!/bin/bash
# Get the context around normalizeUrl usage
rg -B 5 -A 5 'normalizeUrl\(' server/src/browser-management/classes/RemoteBrowser.ts
Length of output: 927
69-70: Initialize lastEmittedUrl Consistently
The lastEmittedUrl property is initialized as null, but later in the code, normalizeUrl may return undefined if it cannot parse the URL.
Ensure that normalizeUrl always returns a string to prevent unexpected undefined values, which could lead to errors during URL comparisons.
| 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); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ 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.
| 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); | |
| } | |
| }; |
| 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); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ 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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation