Fix issue with get_cookies from nodriver in OpenaiChat #2495
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review for Pull Request: Fix issue with get_cookies from nodriver in OpenaiChat
Summary
Thank you, H Lohaus, for your contribution to the project! This pull request addresses an issue with the get_cookies function in the OpenaiChat provider, enhancing the handling of cookies when using the nodriver option. The changes made are well-structured and improve the overall functionality.
Changes
- Added checks for
has_nodriverandhas_curl_cffito ensure proper handling of arguments and cookies. - Improved the
get_modelsandcreate_async_generatormethods to handle cases wherenodriveris not available. - Updated the
loginmethod to yield a login URL if required, enhancing user experience. - Refactored the
get_cookiesfunction to streamline cookie retrieval.
Code Quality
- The code is clean and follows the project's coding standards.
- The use of conditional checks for
has_nodriverandhas_curl_cffiis a good practice to ensure compatibility. - The addition of comments would further enhance readability, especially in complex sections.
Suggestions
- Consider adding unit tests to cover the new functionality introduced in this pull request. This will help ensure that future changes do not break the intended behavior.
- A brief description in the pull request description would be helpful for reviewers to understand the context of the changes.
Conclusion
Overall, this is a solid contribution that improves the functionality of the OpenaiChat provider. Thank you again for your hard work and dedication to enhancing this project!
Looking forward to your future contributions!
| get_running_loop(check_nested=True) | ||
| args = get_args_from_nodriver(cls.url) | ||
| cls._args = asyncio.run(args) | ||
| elif not has_curl_cffi: |
There was a problem hiding this comment.
The condition elif not has_curl_cffi: should be reconsidered. If has_nodriver is false, it may be more appropriate to handle the case where neither has_nodriver nor has_curl_cffi is available.
| try: | ||
| raise_for_status(response) | ||
| except ResponseStatusError: | ||
| cls._args = None | ||
| raise | ||
| return cls.models |
There was a problem hiding this comment.
Returning cls.models in the exception handling block may lead to unexpected behavior. Consider whether this is the intended outcome.
| @@ -62,10 +66,10 @@ | |||
| **kwargs | |||
| ) -> AsyncResult: | |||
| if cls._args is None: | |||
| try: | |||
| if has_nodriver: | |||
| cls._args = await get_args_from_nodriver(cls.url, proxy, timeout, cookies) | |||
There was a problem hiding this comment.
The try-except block should be structured to handle the case where has_nodriver is false. This could lead to unhandled exceptions if get_args_from_nodriver fails.
| @@ -76,10 +77,13 @@ def create_completion( | |||
| cls._access_token, cls._cookies = readHAR(cls.url) | |||
| except NoValidHarFileError as h: | |||
| debug.log(f"Copilot: {h}") | |||
| try: | |||
| if has_nodriver: | |||
| login_url = os.environ.get("G4F_LOGIN_URL") | |||
There was a problem hiding this comment.
Consider providing a default URL or handling the case where G4F_LOGIN_URL is not set to avoid a potential None or empty string causing issues in the login process.
| if has_nodriver: | ||
| login_url = os.environ.get("G4F_LOGIN_URL") | ||
| if login_url: | ||
| yield f"[Login to {cls.label}]({login_url})\n\n" |
There was a problem hiding this comment.
Ensure that the generated markdown link from login_url is safe and cannot be exploited by inserting unintended markdown syntax or URLs (e.g., checking that the URL is well-formed and valid).
| @@ -116,6 +117,8 @@ | |||
| font-weight: 500; | |||
| background-color: rgba(0, 0, 0, 0.5); | |||
| color: var(--colour-3); | |||
| border: var(--colour-1) 1px solid; | |||
There was a problem hiding this comment.
The addition of border: var(--colour-1) 1px solid; could potentially lead to visual inconsistencies if --colour-1 is not defined properly. Verify that this variable is set and used consistently throughout the stylesheet.
| @@ -116,6 +117,8 @@ | |||
| font-weight: 500; | |||
| background-color: rgba(0, 0, 0, 0.5); | |||
| color: var(--colour-3); | |||
| border: var(--colour-1) 1px solid; | |||
| border-radius: var(--border-radius-1); | |||
There was a problem hiding this comment.
The addition of border-radius: var(--border-radius-1); should be checked to ensure that --border-radius-1 is defined. If not, it may cause unexpected rendering issues.
| @@ -826,6 +825,9 @@ | |||
| .count_total { | |||
| padding-left: 98px; | |||
| } | |||
| body:not(.white) .gradient{ | |||
There was a problem hiding this comment.
The new rule body:not(.white) .gradient { display: block; } should be reviewed to confirm that it does not conflict with other styles. Ensure that the intended behavior is clear and does not introduce layout issues.
| @@ -601,6 +601,7 @@ const ask_gpt = async (message_id, message_index = -1, regenerate = false, provi | |||
| api_key: api_key, | |||
| ignored: ignored, | |||
| }, files, message_id); | |||
| if (content_map.inner.dataset.timeout) clearTimeout(content_map.inner.dataset.timeout); | |||
There was a problem hiding this comment.
Consider checking if content_map.inner is defined before accessing dataset.timeout to avoid potential runtime errors.
| } | ||
| let cursorDiv = message_el.querySelector(".cursor"); |
There was a problem hiding this comment.
The variable cursorDiv is declared twice in this scope. Remove the first declaration to avoid redundancy.
No description provided.