Skip to content

feat: increase browser timeouts #2

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: increase browser timeouts #2

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
039ad23
feat: increase browser timeouts for better reliability
daniel-lxs Oct 1, 2025
0e0d30e
feat: increase HTML stability wait timeout
daniel-lxs Oct 2, 2025
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
6 changes: 3 additions & 3 deletions src/services/browser/BrowserSession.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { BrowserActionResult } from "../../shared/ExtensionMessage"
import { discoverChromeHostUrl, tryChromeHostUrl } from "./browserDiscovery"

// Timeout constants
const BROWSER_NAVIGATION_TIMEOUT = 15_000 // 15 seconds
const BROWSER_NAVIGATION_TIMEOUT = 20_000 // 20 seconds
Copy link

@roo-code-dan roo-code-dan bot Oct 1, 2025

Choose a reason for hiding this comment

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

P2 - Missing justification for timeout values

The timeout was increased from 15s to 20s, but there's no explanation for why 20s was chosen. Consider:

  • What specific scenarios were timing out at 15s?
  • Is 20s based on empirical data or testing?
  • Could this mask underlying performance issues?

Adding a comment explaining the rationale would help future maintainers understand this choice.


interface PCRStats {
puppeteer: { launch: typeof launch }
Expand Down Expand Up @@ -256,7 +256,7 @@ export class BrowserSession {

// Wait for console inactivity, with a timeout
await pWaitFor(() => Date.now() - lastLogTs >= 500, {
timeout: 3_000,
timeout: 5_000,
Comment on lines 258 to +259
Copy link

@roo-code-dan roo-code-dan bot Oct 1, 2025

Choose a reason for hiding this comment

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

P2 - Potential timeout collision

This console inactivity timeout is now 5s, which matches the default timeout in waitTillHTMLStable() (line 425). If both are waiting simultaneously, they could expire at the same time, making it unclear which timeout caused a failure.

Consider either:

  • Using different timeout values (e.g., 4s here, 5s in waitTillHTMLStable)
  • Adding distinct error messages to differentiate timeout sources
  • Documenting the relationship between these timeouts

interval: 100,
}).catch(() => {})

Expand Down Expand Up @@ -422,7 +422,7 @@ export class BrowserSession {

// page.goto { waitUntil: "networkidle0" } may not ever resolve, and not waiting could return page content too early before js has loaded
// https://stackoverflow.com/questions/52497252/puppeteer-wait-until-page-is-completely-loaded/61304202#61304202
private async waitTillHTMLStable(page: Page, timeout = 5_000) {
private async waitTillHTMLStable(page: Page, timeout = 8_000) {
Copy link

@roo-code-dan roo-code-dan bot Oct 2, 2025

Choose a reason for hiding this comment

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

P2 - Missing justification for timeout value

The timeout was increased from 5s to 8s, but there's no explanation for why 8s was chosen. This has the same issue as the BROWSER_NAVIGATION_TIMEOUT change. Consider:

  • What specific scenarios were timing out at 5s?
  • Is 8s based on empirical data or testing?
  • Could this mask underlying performance issues?

Adding a comment explaining the rationale would help future maintainers understand this choice.

const checkDurationMsecs = 500 // 1000
const maxChecks = timeout / checkDurationMsecs
let lastHTMLSize = 0
Expand Down