patchright #106

skull8888888 · 2025-05-07T16:11:23Z

Important

Add patchright instrumentor and enhance focus tracking in pw_utils.py and rrweb script.

Tracing:
- Add init_patchright_instrumentor() in tracing.py to support patchright instrumentation.
- Similar structure to existing instrumentors like playwright.
Utilities:
- Update pw_utils.py to track page focus state using window.addEventListener for blur and focus.
- Modify heartbeat events to check window.lmnrPageIsFocused instead of document.visibilityState.
Scripts:
- Add rrweb.umd.min.cjs file, a bundled script for rrweb.
- Modify rrweb script to handle focus events and improve event batching.

^{This description was created by}^{for 74b2d7e. You can customize this summary. It will automatically update as commits are pushed.}

ellipsis-dev

Caution

Changes requested ❌

Reviewed everything up to 05f1e61 in 2 minutes and 48 seconds. Click for details.

Reviewed 380 lines of code in 5 files
Skipped 0 files when reviewing.
Skipped posting 4 draft 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/tracing.py:456

Draft comment:
New PATCHRIGHT branch: Ensure returning True even when the package isn’t installed is intended (consistent with other instrumentors).
Reason this comment was not posted:
Comment looked like it was already resolved.

2. src/lmnr/sdk/browser/pw_utils.py:27

Draft comment:
The rrweb file name change to 'rrweb.umd.min.cjs' looks fine; also, ensure that commenting out the wait_for_function in async injection is intentional.
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% The comment violates the rule about not asking authors to confirm their intention. The comment is phrased as "Ensure that..." which is explicitly called out as likely not useful. Additionally, the code has error handling and retry mechanisms in place, so a failure would be caught and handled appropriately. Maybe removing the wait_for_function could cause race conditions or timing issues that would be hard to debug. The sync version still has this check. The code has comprehensive error handling and retry logic. If rrweb fails to load, it will be detected through the error handlers and retried. Delete the comment as it violates the rule about asking authors to confirm their intentions, and the code has sufficient error handling in place.

3. src/lmnr/sdk/browser/pw_utils.py:24

Draft comment:
The file name has been changed from 'rrweb.min.js' to 'rrweb.umd.min.cjs'. Please ensure that the new UMD build is fully compatible with the rest of the code (e.g. it exposes the expected functions) and that its interface remains stable. It might be useful to extract the file path into a constant to ease future maintenance.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 50% <= threshold 50% The comment is asking the PR author to ensure compatibility and stability, which violates the rules. However, it also suggests extracting the file path into a constant, which is a valid code suggestion.

4. src/lmnr/sdk/browser/rrweb/rrweb.umd.min.cjs:1

Draft comment:
A new minified UMD module has been added. Verify that the UMD wrapper correctly detects CommonJS, AMD, and global environments. In particular, note the deprecation warning related to importing the mirror directly; ensure that developers are informed via documentation about using replayer.getMirror() or record.mirror instead. Also, confirm that the sourceMappingURL comment at the end points to the correct map file and that all licensing/copyright notices are preserved.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to verify multiple aspects of the new UMD module, which violates the rule against asking the author to confirm or ensure things. It also includes a request to ensure developers are informed via documentation, which is not specific enough to be actionable. The comment does not provide specific code suggestions or ask for specific tests to be written.

Workflow ID: wflow_3IgoDcXOaNc1q6x5

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

ellipsis-dev · 2025-05-07T16:14:15Z

src/lmnr/sdk/browser/patchright_otel.py

+        "wrapper": _wrap_new_context_async,
+    },
+    {
+        "package": "patchright.sync_api",


In WRAPPED_METHODS_ASYNC, the last entry uses the sync API (package patchright.sync_api)—verify if this is intended for async instrumentation.

ellipsis-dev · 2025-05-07T16:14:15Z

src/lmnr/opentelemetry_lib/tracing/tracing.py

@@ -360,6 +360,7 @@ def init_instrumentations(
    # Remove any instruments that were explicitly blocked
    instruments = instruments - block_instruments

+    module_logger.error("instruments", instruments)


The log statement module_logger.error("instruments", instruments) may not work as intended because it doesn’t include a placeholder for the instruments variable. Consider using a formatted string (e.g., module_logger.error(f"Instruments: {instruments}")) or a placeholder (e.g., module_logger.error("Instruments: %s", instruments)) so that the list of instruments is clearly logged.

Suggested change

module_logger.error("instruments", instruments)

module_logger.error(f"Instruments: {instruments}")

ellipsis-dev

Important

Looks good to me! 👍

Reviewed 50034af in 1 minute and 35 seconds. Click for details.

Reviewed 46 lines of code in 3 files
Skipped 0 files when reviewing.
Skipped posting 7 draft 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/tracing.py:360

Draft comment:
Removed debug logging. Ensure removal of 'module_logger.error("instruments", instruments)' was intentional.
Reason this comment was not posted:
Comment looked like it was already resolved.

2. src/lmnr/sdk/browser/patchright_otel.py:22

Draft comment:
Changed package from 'patchright.sync_api' to 'patchright.async_api' and switched wrapper from sync to async. Confirm that the instrumentation should target the async API only.
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.

3. src/lmnr/sdk/browser/playwright_otel.py:242

Draft comment:
Changed package from 'playwright.sync_api' to 'playwright.async_api' and switched wrapper from sync to async for 'launch_persistent_context'. Verify this is the intended change for async usage.
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.

4. src/lmnr/opentelemetry_lib/tracing/tracing.py:360

Draft comment:
Removed error log for 'instruments'. Ensure that this debug log removal doesn’t affect error monitoring if needed.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50% None

5. src/lmnr/sdk/browser/patchright_otel.py:24

Draft comment:
Changed package from 'patchright.sync_api' to 'patchright.async_api' and wrapper to _wrap_new_context_async. Confirm this aligns with the intended async API usage.
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.

6. src/lmnr/sdk/browser/playwright_otel.py:320

Draft comment:
Switched package from 'playwright.sync_api' to 'playwright.async_api' and updated wrapper to _wrap_new_context_async for launch_persistent_context. Ensure consistency with the async implementation.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 40% <= threshold 50% The comment is informative and suggests ensuring consistency with the async implementation. It doesn't ask for confirmation or testing, but it does imply a need for consistency, which is a valid concern. However, it doesn't provide a specific suggestion or point out a specific issue.

7. src/lmnr/opentelemetry_lib/tracing/tracing.py:640

Draft comment:
Typographical error: In the init_google_generativeai_instrumentor function, the error message currently says "Error initializing Gemini instrumentor:" when it should refer to the Google GenerativeAI instrumentor. Please update this error message to properly reflect the correct instrumentor name.
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_n6ihLoq6uOPvt1k4

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

ellipsis-dev

Important

Looks good to me! 👍

Reviewed 3d0fd22 in 1 minute and 17 seconds. Click for details.

Reviewed 28 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 4 draft 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/pw_utils.py:157

Draft comment:
Removing the assignment to 'out' is cleaner if the result is unused. Consider using '_' if the return value is intentionally discarded.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50% None

2. src/lmnr/sdk/browser/pw_utils.py:196

Draft comment:
Uncommenting the wait_for_function ensures rrweb is fully loaded before proceeding, which is good to maintain proper initialization order.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50% None

3. src/lmnr/sdk/browser/pw_utils.py:157

Draft comment:
Removed assignment of the evaluation result since it's unused. Directly calling page.evaluate(RRWEB_CONTENT) is cleaner.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50% None

4. src/lmnr/sdk/browser/pw_utils.py:198

Draft comment:
Uncommenting and activating wait_for_function in async load_rrweb ensures rrweb loads before proceeding. Confirm the 5000ms timeout is suitable for all environments.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_O5BtV9cpYitdcXxF

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

ellipsis-dev

Important

Looks good to me! 👍

Reviewed c785693 in 3 minutes and 0 seconds. Click for details.

Reviewed 70 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 12 draft 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/pw_utils.py:105

Draft comment:
Changed logger.warn to logger.debug. Verify that downgrading the log level won't hide important errors.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 30% <= threshold 50% The comment is asking the PR author to verify the change, which violates the rule against asking for confirmation or verification. However, it does point out a potential issue with changing the log level, which could be useful if rephrased as a suggestion or observation.

2. src/lmnr/sdk/browser/pw_utils.py:134

Draft comment:
Logger level changed to debug in the sync version; ensure this matches the intended error reporting policy.
Reason this comment was not posted:
Comment looked like it was already resolved.

3. src/lmnr/sdk/browser/pw_utils.py:155

Draft comment:
Removed the wait_for_function call after loading rrweb. Confirm that this doesn't lead to race conditions if rrweb isn't fully loaded.
Reason this comment was not posted:
Comment looked like it was already resolved.

4. src/lmnr/sdk/browser/pw_utils.py:196

Draft comment:
Removed wait_for_function in the async rrweb loader. Ensure this change doesn't introduce timing issues.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the author to ensure that the change doesn't introduce timing issues, which is a form of asking the author to double-check their work. This violates the rule against asking the PR author to ensure the behavior is intended or to double-check things.

5. src/lmnr/sdk/browser/pw_utils.py:231

Draft comment:
Wrapping takeFullSnapshot in a try-catch is a good defensive pattern. Confirm that any errors logged via console.error meet logging conventions.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50% None

6. src/lmnr/sdk/browser/pw_utils.py:305

Draft comment:
Asynchronously wrapping takeFullSnapshot with try-catch is a sensible addition. Ensure consistency with sync counterpart.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50% None

7. src/lmnr/sdk/browser/pw_utils.py:108

Draft comment:
Changed logger.warn to logger.debug in send_events_async; confirm if this lower log level is intentional.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

8. src/lmnr/sdk/browser/pw_utils.py:137

Draft comment:
Changed logger.warn to logger.debug in send_events_sync; verify if lowering log level is desired.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

9. src/lmnr/sdk/browser/pw_utils.py:157

Draft comment:
Removed wait_for_function in load_rrweb_sync; ensure this change does not introduce race conditions if rrweb isn't fully initialized.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 40% <= threshold 50% The comment is asking the author to ensure that the change does not introduce race conditions, which violates the rule against asking the author to ensure behavior is intended or tested. However, it does point out a potential issue with the removal of wait_for_function, which could be useful for the author to consider. The comment could be rephrased to suggest checking for race conditions without directly asking the author to ensure it.

10. src/lmnr/sdk/browser/pw_utils.py:197

Draft comment:
Removed await page.wait_for_function in load_rrweb_async; confirm that rrweb initialization timing is handled properly.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 30% <= threshold 50% The comment is asking the PR author to confirm that the rrweb initialization timing is handled properly after removing an await statement. This falls under asking the author to ensure behavior is intended, which is against the rules. However, it does point out a specific change that could have implications, which might be useful for the author to consider. Still, it doesn't provide a specific suggestion or ask for a test, so it seems to violate the rules.

11. src/lmnr/sdk/browser/pw_utils.py:230

Draft comment:
Added try/catch around takeFullSnapshot in bring_to_front (sync); good defensive practice.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50% None

12. src/lmnr/sdk/browser/pw_utils.py:306

Draft comment:
Added try/catch around takeFullSnapshot in bring_to_front (async); ensures graceful error handling.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_leBmMSjBzJZr2SpW

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

dinmukhamedm

Left a small comment. Also, should we remove the old rrweb min.js file?

dinmukhamedm · 2025-05-07T19:32:50Z

src/lmnr/sdk/browser/pw_utils.py

-                    page.wait_for_function(
-                        """(() => typeof window.lmnrRrweb !== 'undefined')""",
-                        timeout=5000,
-                    )


why are we sure we can remove this?

for some reason it kept breaking patchright. I think outer try is enough, if page.evaluate on rrweb script doesn't fail, then we're good, otherwise it will trigger try

dinmukhamedm · 2025-05-07T19:33:02Z

src/lmnr/sdk/browser/pw_utils.py

-                    await page.wait_for_function(
-                        """(() => typeof window.lmnrRrweb !== 'undefined')""",
-                        timeout=5000,
-                    )


ellipsis-dev

Important

Looks good to me! 👍

Reviewed 74b2d7e in 2 minutes and 8 seconds. Click for details.

Reviewed 104 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 10 draft 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/pw_utils.py:36

Draft comment:
Avoid using console.log for debug messages in production; consider replacing with a proper logging mechanism if needed.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

2. src/lmnr/sdk/browser/pw_utils.py:44

Draft comment:
Consider using consistent logging for focus events. Using console.log in both focus and blur events might expose internal state in production logs.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

3. src/lmnr/sdk/browser/pw_utils.py:67

Draft comment:
Ensure that replacing document visibility checks with lmnrPageIsFocused matches the desired behavior during background activity.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

4. src/lmnr/sdk/browser/pw_utils.py:81

Draft comment:
Verify that ignoring events when the page isn’t focused (using lmnrPageIsFocused) is acceptable for all use cases; this replaces the previous document visibility check.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

5. src/lmnr/sdk/browser/pw_utils.py:104

Draft comment:
Check that combining focus state with function existence (lmnrGetAndClearEvents) in evaluation logic is the intended behavior across both async and sync event sending functions.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

6. src/lmnr/sdk/browser/pw_utils.py:37

Draft comment:
Consider using document.hasFocus() instead of a custom global flag to track page focus. This leverages a native API and simplifies the code.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

7. src/lmnr/sdk/browser/pw_utils.py:41

Draft comment:
Avoid using console.log for logging focus events in production; consider using a proper logging mechanism.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

8. src/lmnr/sdk/browser/pw_utils.py:105

Draft comment:
Consolidating the focus check with the function existence check inside page.evaluate is clear and concise.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50% None

9. src/lmnr/sdk/browser/pw_utils.py:132

Draft comment:
Consistent use of the custom focus check in send_events_sync is good. No issues here.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50% None

10. src/lmnr/sdk/browser/pw_utils.py:311

Draft comment:
An extra newline was added in the bring_to_front function; consider removing it for consistent formatting.
Reason this comment was not posted:
Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_dHmOcs5PC1XJxxic

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

ellipsis-dev

Skipped PR review on a03db9d because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.

ellipsis-dev · 2025-05-08T09:13:57Z

⚠️ This PR is too big for Ellipsis, but support for larger PRs is coming soon. If you want us to prioritize this feature, let us know at help@ellipsis.dev

Generated with ❤️ by ellipsis.dev

ellipsis-dev · 2025-05-08T09:18:45Z

⚠️ This PR is too big for Ellipsis, but support for larger PRs is coming soon. If you want us to prioritize this feature, let us know at help@ellipsis.dev

Generated with ❤️ by ellipsis.dev

ellipsis-dev · 2025-05-08T09:19:35Z

⚠️ This PR is too big for Ellipsis, but support for larger PRs is coming soon. If you want us to prioritize this feature, let us know at help@ellipsis.dev

Generated with ❤️ by ellipsis.dev

patchright

05f1e61