-
Notifications
You must be signed in to change notification settings - Fork 348
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
Changes from all commits
c6d73be
f60509d
7a577fc
e24dd54
8ea8b3f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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) { | ||||||||||||||||||
try { | ||||||||||||||||||
// Check if browser exists but is disconnected | ||||||||||||||||||
if (browser && !browser.isConnected()) { | ||||||||||||||||||
|
@@ -199,10 +199,14 @@ async function ensureBrowser(browserSettings?: BrowserSettings) { | |||||||||||||||||
browserInstance = chromium; | ||||||||||||||||||
break; | ||||||||||||||||||
} | ||||||||||||||||||
// Read the Chrome executable path from the environment variable | ||||||||||||||||||
const executablePath = process.env.PLAYWRIGHT_CHROME_EXECUTABLE_PATH || undefined; // Fallback to default if not set | ||||||||||||||||||
|
||||||||||||||||||
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 commentThe 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 Severity: Medium There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Suggested change
|
||||||||||||||||||
headless, | ||||||||||||||||||
executablePath: executablePath | ||||||||||||||||||
}); | ||||||||||||||||||
|
||||||||||||||||||
currentBrowserType = browserType; | ||||||||||||||||||
|
||||||||||||||||||
// Add cleanup logic when browser is disconnected | ||||||||||||||||||
|
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 mainensureBrowser
function now calls three separate functions:cleanupDisconnectedBrowser
,launchNewBrowserIfNeeded
, andcreateNewPageIfNeeded
(which needs to be implemented). This improves readability and maintainability by separating concerns and reducing the complexity of the main function.