Add External browser (Tetra) examples#130
Conversation
|
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment
|
paveldudka
requested review from
KateZhang98,
Zechereh,
colriot,
desi003,
frankfeng98,
hongjingzhou,
jayfish0,
lozzle,
taha-tf,
thakkerurvish and
wjwjtf
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Ambiguous console output ▹ view | |
 |
Missing API Key Validation ▹ view | |
 |
Unstructured connection logging ▹ view | |
|
Unclear GraphQL query intention ▹ view | |
|
Missing Search Box Element Validation ▹ view | |
|
Missing session creation error context ▹ view | |
|
Insufficient Error Handling and Cleanup ▹ view | |
 |
Incomplete Function Context in Docstring ▹ view | |
|
Unhandled HTTP error ▹ view | |
 |
Inefficient HTTP Client Creation ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| examples/js/interact-with-external-browser/main.js | ✅ |
| examples/python/interact_with_external_browser/main.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| const SEARCH_QUERY = ` | ||
| { | ||
| search_products_box | ||
| } | ||
| `; |
There was a problem hiding this comment.
Unclear GraphQL query intention 
Tell me more
What is the issue?
The GraphQL query structure is not immediately clear without context about what schema is being used.
Why this matters
Developers unfamiliar with the specific AgentQL schema will need to search documentation or make assumptions about what search_products_box represents.
Suggested change ∙ Feature Preview
Add a comment explaining what the query returns or use a more descriptive constant name:
// Query to get the search input element from the page
const GET_SEARCH_BOX_QUERY = `
{
search_products_box
}
`;Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
There was a problem hiding this comment.
Its not GraphQL. Its AgentQL
There was a problem hiding this comment.
You're right - this is AgentQL specific syntax for browser automation, not GraphQL. The query naming and structure is appropriate for AgentQL's domain-specific language. I retract my previous suggestion.
| console.log(stockResponse); | ||
| await browser.close(); |
There was a problem hiding this comment.
Ambiguous console output
Tell me more
What is the issue?
The final console.log doesn't indicate what information is being logged, making it unclear what output to expect.
Why this matters
Developers running this example won't know what data structure or values to expect in the output.
Suggested change ∙ Feature Preview
Add context to the log statement:
console.log('Product stock information:', stockResponse);
await browser.close();Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| const searchResponse = await wrappedPage.queryElements(SEARCH_QUERY); | ||
|
|
||
| // Use Playwright's API to fill the search box and press Enter | ||
| await searchResponse.search_products_box.type('Charmander'); |
There was a problem hiding this comment.
Missing Search Box Element Validation 
Tell me more
What is the issue?
No null check or error handling for searchResponse.search_products_box before attempting to type into it. The search box might not be found on the page.
Why this matters
If the search box is not found on the page, this will throw an uncaught error and crash the program, as you cannot call .type() on undefined.
Suggested change ∙ Feature Preview
Add validation before attempting to type in the search box:
if (!searchResponse.search_products_box) {
throw new Error('Search box not found on page');
}
await searchResponse.search_products_box.type('Charmander');Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| console.log(`Remote browser URL: ${cdpUrl}`); | ||
| console.log(`Page streaming URL: ${session.getPageStreamingUrl(0)}`); |
There was a problem hiding this comment.
Unstructured connection logging 
Tell me more
What is the issue?
Connection details are logged using console.log() without proper log levels or structured format.
Why this matters
Connection details are important for debugging and should be properly structured and leveled for easier log analysis and filtering.
Suggested change ∙ Feature Preview
logger.debug('Browser session created', {
cdpUrl,
streamingUrl: session.getPageStreamingUrl(0)
});Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
|
|
||
| async function getRemoteBrowserUrl() { | ||
| // This function creates a new browser session with Windows user agent preset | ||
| const session = await createBrowserSession(UserAgentPreset.WINDOWS); |
There was a problem hiding this comment.
Missing session creation error context
Tell me more
What is the issue?
No error handling for the browser session creation which could fail due to external dependencies.
Why this matters
If the browser session creation fails, the error will propagate up without any specific context about where it occurred.
Suggested change ∙ Feature Preview
Wrap the session creation in a try-catch block with context:
async function getRemoteBrowserUrl() {
try {
const session = await createBrowserSession(UserAgentPreset.WINDOWS);
const cdpUrl = session.cdpUrl;
console.log(`Remote browser URL: ${cdpUrl}`);
console.log(`Page streaming URL: ${session.getPageStreamingUrl(0)}`);
return cdpUrl;
} catch (error) {
throw new Error(`Failed to create browser session: ${error.message}`);
}
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| await browser.close(); | ||
| } | ||
|
|
||
| runInExternalBrowser().catch(console.error); |
There was a problem hiding this comment.
Insufficient Error Handling and Cleanup
Tell me more
What is the issue?
The error handling is minimal, only logging the error to console without proper cleanup or resource management.
Why this matters
If an error occurs, the browser connection might remain open, leading to resource leaks.
Suggested change ∙ Feature Preview
Implement proper error handling with cleanup:
async function main() {
let browser;
try {
browser = await runInExternalBrowser();
} catch (error) {
console.error('Failed to run browser automation:', error);
} finally {
if (browser) {
await browser.close().catch(console.error);
}
}
}
main();Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
|
|
||
|
|
||
| def get_remote_browser_url(): | ||
| """This function allocates a new browser instance.""" |
There was a problem hiding this comment.
Incomplete Function Context in Docstring 
Tell me more
What is the issue?
The docstring for get_remote_browser_url() only states what the function does, not why it's needed or any important assumptions.
Why this matters
Missing context about API authentication requirements and browser session management could lead to integration issues.
Suggested change ∙ Feature Preview
"""Allocates a new remote browser instance by creating a session via AgentQL's API.
Requires AGENTQL_API_KEY environment variable to be set.
Returns the Chrome DevTools Protocol URL for browser connection."""
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| response = client.post( | ||
| "https://api.agentql.com/tetra/sessions", | ||
| headers={"X-API-Key": AGENTQL_API_KEY}, | ||
| ) | ||
| response.raise_for_status() |
There was a problem hiding this comment.
Unhandled HTTP error
Tell me more
What is the issue?
The raise_for_status() call lacks a try-except block to handle potential HTTP errors
Why this matters
If the API request fails, the program will crash without any contextual information about what went wrong with the request
Suggested change ∙ Feature Preview
try:
response = client.post(
"https://api.agentql.com/tetra/sessions",
headers={"X-API-Key": AGENTQL_API_KEY},
)
response.raise_for_status()
except httpx.HTTPStatusError as e:
raise RuntimeError(f"Failed to allocate browser: {e.response.status_code} - {e.response.text}") from eProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| with httpx.Client() as client: | ||
| response = client.post( | ||
| "https://api.agentql.com/tetra/sessions", | ||
| headers={"X-API-Key": AGENTQL_API_KEY}, | ||
| ) |
There was a problem hiding this comment.
Inefficient HTTP Client Creation 
Tell me more
What is the issue?
A new HTTP client is created for each browser session without any connection pooling or reuse strategy.
Why this matters
Creating a new HTTP client for each request prevents connection reuse and adds overhead from TCP handshakes and TLS negotiations.
Suggested change ∙ Feature Preview
Create a single, reusable HTTP client at module level for connection pooling:
_http_client = httpx.Client()
def get_remote_browser_url():
response = _http_client.post(...)Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| } | ||
| """ | ||
|
|
||
| AGENTQL_API_KEY = os.getenv("AGENTQL_API_KEY") |
There was a problem hiding this comment.
Missing API Key Validation
Tell me more
What is the issue?
No error handling for missing API key which is critical for the functionality.
Why this matters
The application will fail with unclear errors if the environment variable is not set, preventing users from understanding the root cause.
Suggested change ∙ Feature Preview
Add validation for the API key existence:
AGENTQL_API_KEY = os.getenv("AGENTQL_API_KEY")
if not AGENTQL_API_KEY:
raise ValueError("AGENTQL_API_KEY environment variable is not set")Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
colriot
left a comment
colriot
left a comment
There was a problem hiding this comment.
needs a few thing to be addressed
|
|
||
| def get_remote_browser_url(): | ||
| """This function allocates a new browser instance.""" | ||
| with httpx.Client() as client: |
There was a problem hiding this comment.
i think there should be a method in sdk, like in js sdk
|
/resolve-all |
3 participants
No description provided.