Skip to content
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

browser automation support #668

Merged
merged 8 commits into from
Aug 28, 2024
Merged

browser automation support #668

merged 8 commits into from
Aug 28, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Aug 28, 2024

  • The package playwright has been added to the package.json dependencies, indicating the introduction of browser automation functionalities.🌐
  • A new browse method has been added to the RuntimeHost interface, which allows browsing a URL with specific options. This is reflected in the implementation changes in Nodehost. 🛠️
  • A new class BrowserManager has been introduced in packages/cli/src/playwright.ts to handle browsing activities with methods like init, launchBrowser, stopAndRemove, and browse. This class manages and controls browser instances and tasks. 🕹️
  • In packages/cli/src/nodehost.ts, the DockerManager has been renamed to containers, and a new instance of BrowserManager named browsers has been added. Moreover, the function removeContainers now includes the function removeBrowsers. 🔄
  • In packages/core/src/host.ts, packages/core/src/promptcontext.ts, and packages/core/src/testhost.ts, relevant changes are made to incorporate the new browser functionality. 📝
  • User-facing changes can be found in packages/core/src/types/prompt_template.d.ts where new interfaces BrowseSessionOptions, BrowserLocator, BrowseResponse, and BrowserPage are introduced to provide better structure for browser interactions. 🖥️
  • A new script has been created in packages/sample/genaisrc/browse.genai.mts demonstrating an example usage of browsing a webpage and fetching content using the new browse ability. 👩‍💻

generated by pr-describe

options?: BrowseSessionOptions & TraceOptions
): Promise<BrowserPage> {
return this.browsers.browse(url, options)
}

Choose a reason for hiding this comment

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

There is no error handling for the async function browse. Consider adding a try-catch block to handle potential exceptions.

generated by pr-review-commit missing_error_handling

private async launchBrowser(options?: {}) {
await this.init()
const browser = await this._playwright.chromium.launch()
return browser

Choose a reason for hiding this comment

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

The function launchBrowser does not handle potential errors during the async operations. Consider adding error handling to improve robustness.

generated by pr-review-commit missing_error_handling

} catch (e) {
logError(e)
}
}

Choose a reason for hiding this comment

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

The function stopAndRemove does not handle potential errors during the async operations. Consider adding error handling to improve robustness.

generated by pr-review-commit missing_error_handling

Copy link

The changes in this pull request mostly revolve around the introduction of a new functionality: browsing the web with Playwright.

A new playwright.ts file has been added, where the BrowserManager class is defined. It allows launching a browser, browsing a URL, and stopping and removing the browser.

Other notable changes include:

  • Modifications in nodehost.ts, the docker property has been renamed to containers, and a new browsers property has been added to hold an instance of the new BrowserManager class. Also, new methods browse, removeBrowsers have been added.
  • In host.ts, the RuntimeHost interface has been expanded to include a browse method.
  • In promptcontext.ts, a new browse method has been added to the context object.
  • New BrowseSessionOptions, BrowserLocator, BrowseResponse, and BrowserPage interfaces have been introduced in prompt_template.d.ts.

The changes look good, the new functionality has been well integrated with the existing codebase, and it seems to follow the project's coding standards. However, the playwright dependency is not checked and handled in case it's not available or fails to import. While TypeScript compiler will not find this issue, it's a functional issue that could lead to runtime failures. Consider adding a try-catch block around the import statement and handle the error appropriately.

Otherwise, LGTM 🚀.

generated by pr-review

}
})()
)
)

Choose a reason for hiding this comment

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

There is no error handling for the defImages function when handling Buffer files. Consider adding a try-catch block to handle potential errors.

generated by pr-review-commit missing_error_handling

const res = new Turndown().turndown(html)
return res
} catch (e) {
logError(e)
trace?.error("HTML conversion failed", e)
return undefined
}

Choose a reason for hiding this comment

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

The turndown package is used but it's not imported. Please import it at the top of the file.

generated by pr-review-commit missing_dependency

}
})()
)
)

Choose a reason for hiding this comment

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

The function passed to createImageNode is async but it's not awaited or returned. This could lead to unhandled promise rejections.

generated by pr-review-commit missing_async

if (this._playwright) return
const p = await import("playwright")
if (!p) throw new Error("playwright installation not completed")
this._playwright = p

Choose a reason for hiding this comment

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

There is no error handling for the init function. Consider adding a try-catch block to handle potential exceptions.

generated by pr-review-commit missing_error_handling

trace?.error("HTML conversion failed", e)
return undefined
}
}

Choose a reason for hiding this comment

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

There is no error handling for the HTMLToMarkdown function. Consider adding a try-catch block to handle potential exceptions.

generated by pr-review-commit missing_error_handling

@@ -103,7 +104,8 @@ export class NodeHost implements RuntimeHost {
readonly path = createNodePath()
readonly server = new NodeServerManager()
readonly workspace = createFileSystem()
readonly docker = new DockerManager()
readonly containers = new DockerManager()

Choose a reason for hiding this comment

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

The DockerManager class is not imported but it is used in this file.

generated by pr-review-commit missing_import

const { trace } = options || {}

try {
const res = new Turndown().turndown(html)

Choose a reason for hiding this comment

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

The Turndown class is not imported but it is used in this file.

generated by pr-review-commit missing_import

const { files } = await workspace.grep(
/[a-z][a-z0-9]+/,
"**/*.md")
const { files } = await workspace.grep(/[a-z][a-z0-9]+/, "**/*.md")

Choose a reason for hiding this comment

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

There seems to be a typo in the word "weahter". It should be "weather".

generated by pr-docs-review-commit typo

defTool("weather", "live weahter",
{ city: "Paris" }, // schema
async ({ city }) => // callback
defTool("weather", "live weahter",

Choose a reason for hiding this comment

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

There seems to be a typo in the word "weahter". It should be "weather".

generated by pr-docs-review-commit typo

import { Page } from "playwright"

const page = await host.browse(url) as Page
```

Choose a reason for hiding this comment

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

A new file "browse.md" has been added in the "scripts" reference section. This file provides documentation for interacting with a headless browser using Playwright.

generated by pr-docs-review-commit new_file


## Installation

You will need to install Playright locally before using the `browse` function.

Choose a reason for hiding this comment

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

There seems to be a typo here. The correct spelling should be "Playwright" not "Playright".

generated by pr-docs-review-commit typo


```js
import { Page } from "playwright"

Choose a reason for hiding this comment

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

There seems to be a typo here. The correct word should be "cast" not "case".

generated by pr-docs-review-commit typo

}
})()
)
)

Choose a reason for hiding this comment

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

The type check for files instanceof Buffer in the defImages function is incorrect. The files parameter can be a string or a WorkspaceFile object, but it cannot be a Buffer object. Please revise the type check.

generated by pr-review-commit incorrect_type_check

* @param url
* @param options
*/
browse(url: string, options?: BrowseSessionOptions): Promise<BrowserPage>

Choose a reason for hiding this comment

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

The browse function in the ShellHost interface is missing a description. Please add a description to explain what this function does.

generated by pr-review-commit missing_function_description

@pelikhan pelikhan merged commit 6d16b0b into main Aug 28, 2024
10 checks passed
@pelikhan pelikhan deleted the playwright branch August 28, 2024 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant