-
Notifications
You must be signed in to change notification settings - Fork 9
fix browser-use-cdp #202
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
fix browser-use-cdp #202
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.
Caution
Changes requested ❌
Reviewed everything up to 3c16026 in 1 minute and 42 seconds. Click for details.
- Reviewed
572lines of code in4files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/lmnr/sdk/browser/background_send_events.py:157
- Draft comment:
Typographical error: "Finsihing" should be "Finishing". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While this is technically correct, it's a very minor typo in a log message that doesn't affect functionality. Log message typos are generally not important enough to warrant a PR comment. The comment doesn't suggest any meaningful code improvements or point out actual bugs. The typo could potentially confuse users who see this log message, and fixing it would improve the polish of the codebase. It's a valid catch. However, this is an extremely minor issue in a log message. We should focus PR comments on more substantial issues that affect code quality or functionality. Delete this comment as it points out an extremely minor issue that doesn't meaningfully impact the code quality or functionality.
Workflow ID: wflow_OduM5dG4AO2RlORD
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
a1e5b02 to
f5408b9
Compare
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.
Caution
Changes requested ❌
Reviewed f5408b9 in 1 minute and 56 seconds. Click for details.
- Reviewed
525lines of code in6files - Skipped
0files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/lmnr/opentelemetry_lib/tracing/_instrument_initializers.py:80
- Draft comment:
Removal of the 'client' parameter: all instrumentors now accept only async_client. Ensure documentation and tests are updated accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that documentation and tests are updated, which violates the rule against asking the author to ensure things are tested or documented. It does not provide a specific suggestion or ask for a specific test to be written.
2. src/lmnr/opentelemetry_lib/tracing/instruments.py:127
- Draft comment:
Initializer calls now pass only async_client. Confirm this meets all instrumentation use cases. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm that the change meets all use cases, which is against the rules. It does not provide a specific suggestion or point out a specific issue with the code.
3. src/lmnr/sdk/browser/background_send_events.py:48
- Draft comment:
Removal of the synchronous executor (submit_sync_task) unifies sending via the async background loop. Verify that no legacy sync submissions rely on the removed function. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify that no legacy sync submissions rely on the removed function. This falls under the rule of not asking the author to confirm or verify intentions or dependencies. Therefore, this comment should be removed.
4. src/lmnr/sdk/browser/patchright_otel.py:96
- Draft comment:
PatchrightInstrumentor now only uses async_client. Confirm that downstream usage of client._browser_events.send remains compatible. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm compatibility with downstream usage, which violates the rule against asking for confirmation or ensuring behavior is intended. It does not provide a specific code suggestion or ask for a test to be written.
5. src/lmnr/sdk/browser/playwright_otel.py:268
- Draft comment:
PlaywrightInstrumentor now relies exclusively on async_client for both sync and async wrappers. Verify that this change does not impact any browser interactions. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify that a change does not impact browser interactions. This falls under the rule of not asking the author to ensure behavior is intended or to double-check things. Therefore, this comment should be removed.
6. src/lmnr/sdk/browser/pw_utils.py:760
- Draft comment:
In start_recording_events_sync, the expose_function 'lmnrSendEvents' is set using run_coroutine_threadsafe. Confirm robust error handling in cases where the background loop might not be available. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. src/lmnr/sdk/browser/pw_utils.py:787
- Draft comment:
In start_recording_events_async, exposing the async handler via page.expose_function is done directly. Ensure that any exceptions in send_events_from_browser are properly handled. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_8Ibab2irAVQD1KTu
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Note
Adds a background asyncio loop to send browser events without blocking and refactors Playwright/Patchright/browser-use session instrumentation to use AsyncLaminarClient only.
lmnr/sdk/browser/background_send_events.pywith a dedicated background asyncio loop, future tracking, and graceful atexit shutdown.asyncio.run_coroutine_threadsafe(...)andtrack_async_send(...)to avoid blocking.cdp_utils.pyandpw_utils.py: wire background loop; reassemble/chunk events and submit sends in the background; convert exposed bindings to async-safe handlers; add create_task usage in CDP path.page.is_closed()), and switch toget_default_loggerwith quieter debug logs.PlaywrightInstrumentorandPatchrightInstrumentornow accept onlyAsyncLaminarClient; wrappers for sync/async paths use the async client._instrument_initializers.pyandinstruments.py: initializers updated to requireasync_client; init calls pass onlyasync_client.window.lmnrStartedRecordingEvents.utils.retry_sync: final-attempt log lowered todebug.Written by Cursor Bugbot for commit f5408b9. This will update automatically on new commits. Configure here.
Important
Introduce non-blocking background event sending for browser events, integrating it into Playwright and CDP utilities with improved logging and error handling.
background_send_events.pyfor non-blocking background sending of browser events using a dedicated event loop.submit_sync_task(),get_background_loop(), andtrack_async_send()for managing async sends.cdp_utils.py): Use background loop for event uploads withasyncio.run_coroutine_threadsafe(...)andtrack_async_send.pw_utils.py):lmnrSendEventswrapper to push work tosubmit_sync_task(); add closed-page guards.run_coroutine_threadsafe+track_async_send.maskInputOptions; guard to avoid double-init; adjust large-event handling.retry_sync()logs final failure withlogger.debuginstead oflogger.error.This description was created by
for f5408b9. You can customize this summary. It will automatically update as commits are pushed.