-
-
Notifications
You must be signed in to change notification settings - Fork 320
[HOTFIX] Fix importing media from gamelist.xml #2636
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
Conversation
Summary of ChangesHello @gantoine, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This hotfix addresses issues related to importing media from Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a new copy_file method to centralize file copying logic, which is a good refactoring. The main issue is that the new method, like other existing methods in base_handler.py, uses blocking I/O calls within an async function. This can severely impact the application's performance and responsiveness. I've added a high-severity comment with a suggested fix for the new copy_file method. I recommend applying a similar pattern to other blocking calls throughout the file (e.g., in move_file_or_folder, remove_file, make_directory, etc.) to make the filesystem handler truly asynchronous.
The changes in gamelist_handler.py are a good bugfix, correctly handling file:// URLs from gamelist.xml. The frontend change is a minor but useful UI improvement.
Overall, this is a valuable hotfix, but addressing the blocking I/O is critical for maintaining a healthy async application.
Test Results616 tests 615 ✅ 1m 13s ⏱️ Results for commit d4e0f67. ♻️ This comment has been updated with latest results. |
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
|
/gemini review |
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.
Code Review
This pull request introduces a hotfix for importing media from gamelist.xml files. The changes refactor file system operations by adding a new copy_file method in base_handler.py and using it across resources_handler.py. This correctly handles local file:// URLs from gamelists. The PR also simplifies several file system checks by removing redundant .exists() calls. Overall, the changes are a good improvement and fix the intended bug. However, I've identified a significant performance issue in the new copy_file method where blocking I/O operations are used within an async function, which could harm application responsiveness. My review includes a suggestion to address this.
| dest_full_path.parent.mkdir(parents=True, exist_ok=True) | ||
| shutil.copy2(str(source_full_path), str(dest_full_path)) |
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.
The copy_file method is async, but it uses dest_full_path.parent.mkdir() and shutil.copy2(), which are synchronous, blocking I/O operations. This will block the asyncio event loop, potentially for a long time if the file being copied is large, which can degrade the performance and responsiveness of the application. Blocking calls in async functions should be run in a separate thread to avoid this.
You can use anyio.to_thread.run_sync to execute these blocking calls without blocking the event loop. You will also need to add to_thread to your anyio import (e.g., from anyio import open_file, to_thread).
| dest_full_path.parent.mkdir(parents=True, exist_ok=True) | |
| shutil.copy2(str(source_full_path), str(dest_full_path)) | |
| await to_thread.run_sync(dest_full_path.parent.mkdir, parents=True, exist_ok=True) | |
| await to_thread.run_sync(shutil.copy2, str(source_full_path), str(dest_full_path)) |
Description
Explain the changes or enhancements you are proposing with this pull request.
Checklist
Please check all that apply.