-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(server): add wait and custom code tools #984
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
base: dev
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -657,13 +657,20 @@ export class Browser { | |
|
|
||
| async waitFor( | ||
| page: number, | ||
| opts: { text?: string; selector?: string; timeout: number }, | ||
| opts: { | ||
| text?: string | ||
| textGone?: string | ||
| selector?: string | ||
| selectorGone?: string | ||
| timeout: number | ||
| }, | ||
| ): Promise<boolean> { | ||
| const session = await this.resolveSession(page) | ||
| const deadline = Date.now() + opts.timeout | ||
| const interval = 500 | ||
| let selectorGoneWasPresent = false | ||
|
|
||
| while (Date.now() < deadline) { | ||
| while (Date.now() <= deadline) { | ||
| if (opts.text) { | ||
| const result = await session.Runtime.evaluate({ | ||
| expression: `document.body?.innerText?.includes(${JSON.stringify(opts.text)}) ?? false`, | ||
|
|
@@ -672,6 +679,14 @@ export class Browser { | |
| if (result.result?.value === true) return true | ||
| } | ||
|
|
||
| if (opts.textGone) { | ||
| const result = await session.Runtime.evaluate({ | ||
| expression: `!(document.body?.innerText?.includes(${JSON.stringify(opts.textGone)}) ?? false)`, | ||
| returnByValue: true, | ||
| }) | ||
| if (result.result?.value === true) return true | ||
| } | ||
|
Comment on lines
+683
to
+693
Contributor
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.
Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/browseros-agent/apps/server/src/browser/browser.ts
Line: 682-688
Comment:
**`textGone` lacks the "was-present" guard that `selectorGone` has**
`selectorGone` only reports success after the element was observed at least once (via `selectorGoneWasPresent`), preventing a false positive when the selector simply never existed. `textGone` has no equivalent guard and returns `true` on the very first poll if the text is already absent. A caller doing `waitFor({ textGone: 'Loading…' })` before the page has rendered any text would receive `found: true` immediately, even though the text never appeared.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| if (opts.selector) { | ||
| const result = await session.Runtime.evaluate({ | ||
| expression: `!!document.querySelector(${JSON.stringify(opts.selector)})`, | ||
|
|
@@ -680,7 +695,21 @@ export class Browser { | |
| if (result.result?.value === true) return true | ||
| } | ||
|
|
||
| await new Promise((r) => setTimeout(r, interval)) | ||
| if (opts.selectorGone) { | ||
| const result = await session.Runtime.evaluate({ | ||
| expression: `!!document.querySelector(${JSON.stringify(opts.selectorGone)})`, | ||
| returnByValue: true, | ||
| }) | ||
| if (result.result?.value === true) { | ||
| selectorGoneWasPresent = true | ||
| } else if (selectorGoneWasPresent) { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| const remaining = deadline - Date.now() | ||
| if (remaining <= 0) break | ||
| await new Promise((r) => setTimeout(r, Math.min(interval, remaining))) | ||
| } | ||
|
|
||
| return false | ||
|
|
@@ -941,6 +970,42 @@ export class Browser { | |
| } | ||
| } | ||
|
|
||
| async runCode( | ||
| page: number, | ||
| code: string, | ||
| args?: Record<string, unknown>, | ||
| ): Promise<{ | ||
| value?: unknown | ||
| error?: string | ||
| description?: string | ||
| }> { | ||
| const session = await this.resolveSession(page) | ||
| const expression = `( | ||
| async (args) => { | ||
| ${code} | ||
| } | ||
| )(${JSON.stringify(args ?? {})})` | ||
|
|
||
| const result = await session.Runtime.evaluate({ | ||
| expression, | ||
| returnByValue: true, | ||
| awaitPromise: true, | ||
| }) | ||
|
|
||
| if (result.exceptionDetails) { | ||
| return { | ||
| error: | ||
| result.exceptionDetails.exception?.description ?? | ||
| result.exceptionDetails.text, | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| value: result.result?.value, | ||
| description: result.result?.description, | ||
| } | ||
| } | ||
|
|
||
| async getDom(page: number, opts?: { selector?: string }): Promise<string> { | ||
| const session = await this.resolveSession(page) | ||
| const doc = await session.DOM.getDocument({ depth: 0 }) | ||
|
|
||
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.
textGonereturns true prematurely on an unloaded pageThe expression
!(document.body?.innerText?.includes(...) ?? false)evaluates totruewhendocument.bodyisnullorundefined(page still loading). The optional chain short-circuits toundefined,undefined ?? falsebecomesfalse, and!falseistrue, so the condition reports the text as "gone" even before any content has rendered. Contrast with thetextcheck which uses the same pattern affirmatively — it correctly returnsfalse(not found) when the body is absent.Prompt To Fix With AI