-
Notifications
You must be signed in to change notification settings - Fork 347
Release branch for v1.0.6 #148
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.
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
Conversation
- Updated Playwright and related packages to version 1.53.1. - Added new tool `playwright_upload_file` for file uploads. - Enhanced content extraction tools and improved interactions. - Added support for specifying the Chrome executable path. - Updated documentation and added tests for new features.
…nd limit output length - By default, all `<script>` tags are now removed from the HTML output unless `removeScripts: false` is specified. - Added `maxLength` parameter to limit the output to a safe length (default 20,000 characters). - Updated documentation to reflect these changes.
- Modified the test case to specify the `removeScripts` argument as `false` to align with recent changes in the tool's functionality.
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
@@ -155,7 +155,7 @@ async function registerConsoleMessage(page) { | |||
/** | |||
* Ensures a browser is launched and returns the page | |||
*/ | |||
async function ensureBrowser(browserSettings?: BrowserSettings) { | |||
export async function ensureBrowser(browserSettings?: BrowserSettings) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description: The function is long and handles multiple responsibilities, which could make it harder to maintain and understand. Consider breaking down the function into smaller, more focused functions for each main task (e.g., browser cleanup, browser launch, page creation).
Severity: Medium
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix addresses the comment by breaking down the ensureBrowser
function into smaller, more focused functions. The main ensureBrowser
function now calls three separate functions: cleanupDisconnectedBrowser
, launchNewBrowserIfNeeded
, and createNewPageIfNeeded
(which needs to be implemented). This improves readability and maintainability by separating concerns and reducing the complexity of the main function.
export async function ensureBrowser(browserSettings?: BrowserSettings) { | |
* Ensures a browser is launched and returns the page | |
*/ | |
export async function ensureBrowser(browserSettings?: BrowserSettings) { | |
await cleanupDisconnectedBrowser(); | |
await launchNewBrowserIfNeeded(browserSettings); | |
return await createNewPageIfNeeded(); | |
} | |
async function cleanupDisconnectedBrowser() { | |
if (browser && !browser.isConnected()) { | |
console.error("Browser exists but is disconnected. Cleaning up..."); | |
try { | |
await browser.close().catch(err => console.error("Error closing disconnected browser:", err)); | |
} catch (e) { | |
// Ignore errors when closing disconnected browser | |
} | |
resetBrowserState(); | |
} | |
} | |
async function launchNewBrowserIfNeeded(browserSettings?: BrowserSettings) { | |
if (!browser) { | |
const { viewport, userAgent, headless = false, browserType = 'chromium' } = browserSettings ?? {}; | |
// If browser type is changing, force a new browser instance | |
if (browser && currentBrowserType !== browserType) { | |
try { |
browser = await browserInstance.launch({ headless, executablePath }); | ||
const executablePath = process.env.CHROME_EXECUTABLE_PATH; | ||
|
||
browser = await browserInstance.launch({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description: The browser launch configuration doesn't include any performance-related options, which could impact the tool's efficiency. Consider adding performance-enhancing options to the browser launch configuration, such as args: ['--no-sandbox', '--disable-setuid-sandbox']
for Chromium-based browsers.
Severity: Medium
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix addresses the performance issue by adding performance-enhancing options to the browser launch configuration. Specifically, it adds the arguments '--no-sandbox' and '--disable-setuid-sandbox' to the launch options for Chromium-based browsers. These options can improve the browser's startup time and overall performance, especially in containerized environments.
browser = await browserInstance.launch({ | |
browser = await browserInstance.launch({ | |
headless, | |
executablePath: executablePath, | |
args: ['--no-sandbox', '--disable-setuid-sandbox'] | |
}); | |
currentBrowserType = browserType; |
✅ I finished the code review, and left comments with the issues I found. I will now generate code fix suggestions. |
Release branch for v1.0.6