-
Notifications
You must be signed in to change notification settings - Fork 3
refactor(browser): update browser creation params #63
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
Conversation
Mesa DescriptionThis PR refactors the browser creation process by updating the parameters used during instantiation. The changes likely involve simplifying the configuration of new browser instances, potentially by removing or modifying parameters related to persistence, as suggested by the branch name Description generated by Mesa. Update settings |
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.
Performed full review of 8d6b8ce...382e043
Analysis
-
Breaking API Change: Removal of the
persistenceparameter without visible deprecation warnings or migration guide will break existing code using this feature. -
Inconsistent Type Definitions: TypeScript makes
browser_live_view_urloptional while Python keeps it required, creating an inconsistency between language implementations. -
Potential Functional Regression: The shift from persistence IDs to timeout-based management eliminates the ability to reuse browser state across invocations, which may impact use cases like captcha-solving that rely on persistent state.
-
Documentation Alignment: While documentation URLs are updated, there appears to be insufficient guidance for users on how to adapt to the new browser lifecycle management approach.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
4 files reviewed | 0 comments | Edit Agent Settings • Read Docs
dprevoznik
left a comment
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.
Requested two quick changes (sorta unrelated, but noticed them). Otherwise looks good
| const title = await page.title(); | ||
| return { title }; | ||
| } finally { | ||
| await browser.close(); |
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.
Can we replace this line with:
await kernel.browsers.deleteByID(kernelBrowser.session_id);
templates/python/sample-app/main.py
Outdated
|
|
||
| return {"title": title} | ||
| finally: | ||
| await browser.close() |
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.
Can we replace this line with:
kernel.browsers.delete_by_id(kernel_browser.session_id)
| return {"title": title} | ||
| finally: | ||
| await browser.close() | ||
| client.browsers.delete_by_id(kernel_browser.session_id) |
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.
Bug: Missing await on async browser deletion call
The client.browsers.delete_by_id() call is missing await in this async function. The TypeScript equivalent properly uses await kernel.browsers.deleteByID(...), indicating this is an async operation. Without await, the browser deletion may not complete before the function returns, errors will be silently lost, and browser sessions may not be properly cleaned up.
dprevoznik
left a comment
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.
LGTM
Note
Removes browser persistence, switches cleanup to delete-by-id, and introduces a long-running test browser action with timeout in both Python and TypeScript samples.
persistencefromclient.browsers.create(...)intemplates/python/advanced-sample/main.py.await browser.close()toclient.browsers.delete_by_id(kernel_browser.session_id)inget-page-title.create-browser-for-testing(rename types and action), usestealth=Trueandtimeout_seconds=3600(no persistence), update docs link.persistencefromkernel.browsers.create(...)intemplates/typescript/advanced-sample/index.ts.await browser.close()toawait kernel.browsers.deleteByID(kernelBrowser.session_id)inget-page-title.create-browser-for-testing(rename interface and action), addstealth: trueandtimeout_seconds: 3600(no persistence), update docs link.Written by Cursor Bugbot for commit b70c415. This will update automatically on new commits. Configure here.