-
Notifications
You must be signed in to change notification settings - Fork 794
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
Define browser manager API #1497
base: main
Are you sure you want to change the base?
Define browser manager API #1497
Conversation
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.
❌ Changes requested. Reviewed everything up to f19e60e in 1 minute and 45 seconds
More details
- Looked at
658
lines of code in9
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. skyvern/forge/sdk/db/client.py:2293
- Draft comment:
Remove the print statement used for debugging purposes. Consider using proper logging instead. - Reason this comment was not posted:
Confidence changes required:50%
Thecreate_persistent_browser_session
method inAgentDB
has a print statement that seems to be used for debugging purposes. This should be removed or replaced with proper logging to maintain clean and professional code.
2. skyvern/webeye/persistent_sessions_manager.py:62
- Draft comment:
Remove the print statement used for debugging purposes. Consider using proper logging instead. - Reason this comment was not posted:
Confidence changes required:50%
Thecreate_session
method inPersistentSessionsManager
has a print statement that seems to be used for debugging purposes. This should be removed or replaced with proper logging to maintain clean and professional code.
3. scripts/test_persistent_browsers.py:8
- Draft comment:
Consider adding a check to ensureAPI_KEY
is notNone
after retrieval from the environment. This will help avoid issues if the environment variable is not set. - Reason this comment was not posted:
Confidence changes required:50%
Intest_persistent_browsers.py
, theAPI_KEY
is retrieved from the environment but not checked forNone
. This could lead to issues if the environment variable is not set. It's a good practice to handle such cases.
Workflow ID: wflow_O6uQXZpwqX26pgM4
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
1679678
to
9c2e551
Compare
from skyvern.forge.sdk.schemas.persistent_browser_sessions import PersistentBrowserSession | ||
|
||
|
||
class BrowserSessionResponse(BaseModel): |
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.
[not a block] nit picky:
we can reuse the PersistentBrowserSession
pydantic model. https://chatgpt.com/share/677e032d-f414-800b-8914-3785008cba3b
@@ -161,20 +161,26 @@ def update_chromium_browser_preferences(user_data_dir: str, download_dir: str) - | |||
f.write(preference_file_content) | |||
|
|||
@staticmethod | |||
def build_browser_args(proxy_location: ProxyLocation | None = None) -> dict[str, Any]: | |||
def build_browser_args(proxy_location: ProxyLocation | None = None, **kwargs: dict) -> dict[str, Any]: |
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.
@LawyZheng pls also take a look at this part
Co-authored-by: Shuchang Zheng <[email protected]>
Depends on: #1495 (db migration)
Testing
The testing helper provides HTTP API and direct method call commands: