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

Persistent browser sessions #1391

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

satansdeer
Copy link
Contributor

@satansdeer satansdeer commented Dec 16, 2024

  • Set close_browser_on_completion False
  • Add persistent sessions manager + test
  • Define route to get persistent session; Add route test
  • Test sessions list endpoint
  • Add session creation endpoint
  • Define test commands
  • Make it possible to set and use dev variables
  • Add browser session id to workflow runs
  • Add browser_session_id to context
  • Use persistent browser session if present
  • Pass browser_session_id through async executor

Important

Introduce persistent browser sessions with management, API endpoints, and workflow integration, including frontend and backend support.

  • Persistent Sessions:
    • Add PersistentSessionsManager to manage browser sessions in persistent_sessions_manager.py.
    • Update BrowserManager in browser_manager.py to use persistent sessions if available.
    • Add browser_session_id to SkyvernContext in skyvern_context.py.
  • API Endpoints:
    • Add endpoints in agent_protocol.py to create, list, and retrieve browser sessions.
    • Add tests for session endpoints in test_browser_sessions_routes.py.
  • Workflow Execution:
    • Update execute_workflow in service.py to use browser_session_id.
    • Modify RunWorkflowForm.tsx to include browser_session_id in requests.
  • Dev Commands:
    • Add initDevCommands.ts to create and list browser sessions from the frontend.
    • Initialize dev commands in main.tsx.
  • Testing:
    • Add test_persistent_sessions_manager.py to test session management.
    • Add run_tests.sh to facilitate running tests.

This description was created by Ellipsis for 6b2077e. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 6b2077e in 1 minute and 12 seconds

More details
  • Looked at 979 lines of code in 18 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 drafted comments based on config settings.
1. run_tests.sh:13
  • Draft comment:
    Add a newline at the end of the file to follow best practices.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The run_tests.sh script is missing a newline at the end of the file. This is a minor issue but it's a good practice to end files with a newline to avoid potential issues with some tools and editors.
2. skyvern-frontend/src/initDevCommands.ts:18
  • Draft comment:
    The non-null assertion operator ! is unnecessary here since envCredential is already checked for nullability. Consider removing it for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    In initDevCommands.ts, the envCredential! is used with a non-null assertion operator. This is unnecessary because the code already checks if envCredential is not set and returns early. The non-null assertion operator can be removed to improve code clarity.
3. skyvern-frontend/src/initDevCommands.ts:45
  • Draft comment:
    The non-null assertion operator ! is unnecessary here since envCredential is already checked for nullability. Consider removing it for clarity. This applies to the listBrowserSessions function as well.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    In initDevCommands.ts, the envCredential! is used with a non-null assertion operator. This is unnecessary because the code already checks if envCredential is not set and returns early. The non-null assertion operator can be removed to improve code clarity. This issue is also present in the listBrowserSessions function.
4. skyvern-frontend/src/routes/workflows/RunWorkflowForm.tsx:92
  • Draft comment:
    The double casting to unknown and then to string is unnecessary. Simplify by directly casting to string.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    In RunWorkflowForm.tsx, the browserSessionId is retrieved from window.devCommands and cast to unknown and then to string. This double casting is unnecessary and can be simplified by directly casting to string.
5. skyvern/forge/sdk/routes/test_browser_sessions_routes.py:27
  • Draft comment:
    Consider using datetime instead of time.time() for setting the JWT expiration to handle timezones and time precision more effectively.
  • Reason this comment was not posted:
    Confidence changes required: 30%
    In test_browser_sessions_routes.py, the setup_auth_mocks function uses time.time() to set the expiration time for the JWT. This can lead to issues with time synchronization across systems. It's better to use datetime for more precise control over time and timezone handling.
6. skyvern/forge/sdk/routes/test_browser_sessions_routes.py:29
  • Draft comment:
    Ensure settings.SECRET_KEY is securely managed and not hardcoded or exposed in the codebase.
  • Reason this comment was not posted:
    Confidence changes required: 30%
    In test_browser_sessions_routes.py, the x_api_key is generated using jwt.encode with settings.SECRET_KEY. Ensure that settings.SECRET_KEY is securely managed and not hardcoded or exposed in the codebase.
7. skyvern/forge/sdk/workflow/models/block.py:439
  • Draft comment:
    Ensure that setting close_browser_on_completion=False is the desired behavior for all cases where execute_step is called, as this change supports persistent browser sessions.
  • Reason this comment was not posted:
    Confidence changes required: 30%
    In block.py, the close_browser_on_completion parameter is set to False in the execute_step method. This change is consistent with the PR's intent to support persistent browser sessions, but it should be verified that this behavior is desired in all cases where execute_step is called.
8. skyvern/forge/sdk/workflow/service.py:125
  • Draft comment:
    Consider implementing a timeout reset for reusable browser sessions if a timeout mechanism exists, to keep the session alive during the workflow run.
  • Reason this comment was not posted:
    Confidence changes required: 30%
    In service.py, the setup_workflow_run method checks for a reusable browser session but does not reset its timeout. If sessions have a timeout mechanism, consider implementing a reset to keep the session alive during the workflow run.

Workflow ID: wflow_ZaNBZnAESCKepMob


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@satansdeer satansdeer force-pushed the maksimi/persistent-browser-session branch from 164bb41 to 33c15ac Compare December 17, 2024 17:23
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