feat: add navigation timeout & timeout browser options #437
Conversation
| }); | ||
| page.setDefaultNavigationTimeout(60000); | ||
| page.setDefaultTimeout(5000); | ||
| page.setDefaultNavigationTimeout(this.context.config.browser.navigationTimeout || 60000); |
There was a problem hiding this comment.
these should be called on the browser context in _setupBrowserContext
There was a problem hiding this comment.
@pavelfeldman just to confirm, by this you mean updating the _setupBrowserContent method to something like this, while also reverting the modifs from src/tab.ts?
private async _setupBrowserContext(): Promise<BrowserContextAndBrowser> {
const { browser, browserContext } = await this._createBrowserContext();
await this._setupRequestInterception(browserContext);
for (const page of browserContext.pages()) {
this._onPageCreated(page);
// New Code Starts Here
if (this.config.browser.navigationTimeout)
page.setDefaultNavigationTimeout(this.config.browser.navigationTimeout);
if (this.config.browser.timeout)
page.setDefaultTimeout(this.config.browser.timeout);
// New Code Ends Here
}
browserContext.on('page', page => this._onPageCreated(page));
if (this.config.saveTrace) {
await browserContext.tracing.start({
name: 'trace',
screenshots: false,
snapshots: true,
sources: false,
});
}
return { browser, browserContext };
}There was a problem hiding this comment.
No, I meant to set those on context.
There was a problem hiding this comment.
Could you clarify which part of the code you want me to change? I’m not really sure what you mean.
Thanks!
There was a problem hiding this comment.
#437 (comment) - this should be enough of the clue. We expect familiarity with the Playwright API from the contributors.
|
Looks good overall, minor comment and it is good to go. |
README.md
Outdated
| example "1280, 720" | ||
| --vision Run server that uses screenshots (Aria snapshots | ||
| are used by default) | ||
| --navigation-timeout <ms> Maximum time to wait for navigation/load events, |
There was a problem hiding this comment.
these are sorted alphabetically
src/program.ts
Outdated
| .option('--user-data-dir <path>', 'path to the user data directory. If not specified, a temporary directory will be created.') | ||
| .option('--viewport-size <size>', 'specify browser viewport size in pixels, for example "1280, 720"') | ||
| .option('--vision', 'Run server that uses screenshots (Aria snapshots are used by default)') | ||
| .option('--navigation-timeout <ms>', 'Maximum time to wait for navigation/load events, in milliseconds (default: 60000)') |
…eview suggestions
|
Closing as non-converging. |
|
Hi hi @pavelfeldman sorry what did you mean, I think allowing to set a custom timeout is a very valid case. I tried using playwright-mcp and it works for popular and fast pages but always prematurely disconnects for internal pages that are JS-heavy and slow.... orz |
|
@pavelfeldman Hey Pavel! I would like to reopen this, I see it's pretty much done, I can fix the last issues, in hope to merge it as it's a must for our usage of Claude Code |
4 participants
Fixes #224