feat: added lynx browser support #214
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 02db7d9 in 45 seconds
More details
- Looked at
295lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. gptme/tools/_browser_lynx.py:14
- Draft comment:
Ensure that the query parameter is properly sanitized to prevent command injection vulnerabilities. - Reason this comment was not posted:
Comment did not seem useful.
2. gptme/tools/browser.py:27
- Draft comment:
Consider renaming the lambda functions to follow the standard naming conventions, such as has_playwright and has_lynx, to improve code readability. - Reason this comment was not posted:
Confidence changes required:50%
The lambda functions for checking the availability of playwright and lynx are not following the standard naming conventions for functions.
3. gptme/tools/browser.py:99
- Draft comment:
Add a check to handle the case where the browser is None to prevent runtime errors. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_EcrO1voqVPoNpNvj
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.
|
|
||
|
|
||
| def read_url(url): | ||
| return subprocess.run( |
There was a problem hiding this comment.
Consider adding error handling for subprocess.run to catch exceptions if the lynx command fails. This will help in debugging and provide meaningful error messages.
| raise ValueError(f"Unknown search engine: {engine}") | ||
|
|
||
|
|
||
| def test_read_url(): |
There was a problem hiding this comment.
Consider using a testing framework like unittest or pytest for the test functions instead of print statements. This will make the tests more robust and easier to manage.
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #214 +/- ##
==========================================
- Coverage 75.31% 74.84% -0.47%
==========================================
Files 57 58 +1
Lines 3609 3650 +41
==========================================
+ Hits 2718 2732 +14
- Misses 891 918 +27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 62fb25d in 22 seconds
More details
- Looked at
34lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. tests/test_browser.py:3
- Draft comment:
Consider adding tests for the Lynx backend to ensure the new functionality is working as expected. This is important for verifying the integration of Lynx alongside Playwright. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is redundant because the TODO comment already addresses the need for testing the Lynx backend. The automated comment does not provide any new information or actionable steps beyond what is already noted in the TODO.
I might be overlooking the possibility that the automated comment provides additional context or urgency that the TODO does not. However, the TODO is a clear indicator of the need for future work.
The TODO comment is sufficient to indicate the need for testing the Lynx backend, and the automated comment does not add any new value.
Delete the comment as it is redundant and does not provide additional value beyond the existing TODO comment.
Workflow ID: wflow_pnyflKAX8RFcrh3U
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
ErikBjare
force-pushed
the
dev/browser-lynx
branch
from
62fb25d to
b3d378d
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on b3d378d in 14 seconds
More details
- Looked at
34lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. tests/test_browser.py:3
- Draft comment:
Consider adding tests for the Lynx backend to ensure its functionality is covered. - Reason this comment was not posted:
Confidence changes required:50%
The comment on line 3 suggests testing the Lynx backend, but no tests are implemented for it. This is a missing feature in the test suite.
Workflow ID: wflow_TT02Z9AyVXcFapVr
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on f9268db in 14 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_1Vejxi4hrPCIP5fT
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
ErikBjare
changed the title
Fixes #205
Important
Adds Lynx browser support and refactors browser functions to handle both Playwright and Lynx backends.
browser.py, selecting between Playwright and Lynx based on availability.read_url,search, andscreenshot_urlinbrowser.pyto support both Playwright and Lynx.screenshot_urlraises an error if Lynx is used, as Lynx does not support screenshots._browser_lynx.pywithread_urlandsearchfunctions for Lynx.read_urlandsearchlogic to_browser_playwright.pyand_browser_lynx.py.search_playwrightinbrowser.pyfor Playwright-specific search logic.test_browser.pyto reflect changes, with a note to test Lynx backend.This description was created by
for f9268db. It will automatically update as commits are pushed.