-
-
Notifications
You must be signed in to change notification settings - Fork 326
Walkthrough support #2892
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
base: master
Are you sure you want to change the base?
Walkthrough support #2892
Conversation
Summary of ChangesHello @SaraVieira, 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 pull request delivers a significant new feature by integrating comprehensive walkthrough management directly into the application. It enables users to enrich their game library with guides from various sources, enhancing the overall user experience by centralizing game-related information and tracking their engagement with these guides. 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 significant new feature for managing walkthroughs for ROMs, including fetching from GameFAQs and uploading local files (PDF, HTML, TXT). The backend implementation is comprehensive, with new API endpoints, database models, and handlers. The frontend is also updated with new components to display and manage these walkthroughs. The code is generally well-structured, but I've identified a few areas for improvement regarding code duplication, database efficiency, and type safety in the frontend. I've also found a potential bug related to a missing enum value in the auto-generated frontend types.
|
I'm on all of these reviews |
|
/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 comprehensive feature for managing walkthroughs for ROMs. The changes span across the backend and frontend, including new database tables, API endpoints, file handling, and UI components.
The backend implementation is solid, with good error handling and security considerations like HTML sanitization. The frontend work is also well-structured, with a dedicated composable for the walkthrough viewer logic, which includes nice features like progress tracking and virtual scrolling.
I've identified a few areas for improvement:
- A potential data inconsistency issue in the PDF upload logic due to multiple database transactions.
- Some minor code cleanup opportunities, such as removing an unused function parameter and an unused import.
- A suggestion to improve method naming for better clarity.
Overall, this is a great addition to the application. Please see my detailed comments for specific suggestions.
frontend/src/components/common/Game/Dialog/EditRom/WalkthroughPanel.vue
Outdated
Show resolved
Hide resolved
|
/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 comprehensive walkthrough feature, including backend support for fetching from GameFAQs, file uploads (PDF, HTML, TXT), and database storage, along with a well-designed frontend interface for managing and viewing walkthroughs. The implementation is solid, with good error handling and security considerations like HTML sanitization. I've identified a few areas for improvement, including a data handling bug in the frontend, a minor inconsistency in a database migration file, and an opportunity to make exception handling more specific in one of the endpoints. Overall, this is a great addition to the application.
frontend/src/components/common/Game/Dialog/EditRom/WalkthroughPanel.vue
Outdated
Show resolved
Hide resolved
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/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 comprehensive feature for managing and viewing walkthroughs for ROMs. The changes are well-structured, spanning the database schema, backend APIs for fetching and managing walkthroughs from GameFAQs and file uploads, and a polished frontend interface for display. The implementation is robust, featuring solid error handling, security measures like HTML sanitization, and a thoughtful user experience for reading long guides. I have one minor suggestion to enhance MIME type handling for Markdown file uploads.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
gantoine
left a comment
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.
first review coming your way (sorry for the wait), still need to look at walkthrough_handler but i'll do that in my editor another
| sa.Column("author", sa.String(length=250), nullable=True), | ||
| sa.Column( | ||
| "source", | ||
| sa.Enum("GAMEFAQS", "UPLOAD", name="walkthroughsource"), |
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.
would copying the contents as text be labeled as "UPLOAD", or do we need a "MANUAL" for it (like writing a note)?
| nullable=False, | ||
| ), | ||
| sa.Column( | ||
| "format", |
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.
i don't think we should save this since we can use the filename to get the format, and that offers us more flexibility in displaying content
| sa.Enum("html", "text", "pdf", name="walkthroughformat"), | ||
| nullable=False, | ||
| ), | ||
| sa.Column("file_path", sa.String(length=1000), nullable=True), |
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.
would like to see file_name and file_path stored separately like we do for roms
| log.error("Walkthrough fetch failed", exc_info=True) | ||
| raise HTTPException( | ||
| status_code=status.HTTP_502_BAD_GATEWAY, detail=str(exc) | ||
| ) from exc |
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.
👍🏼
| async def get_walkthrough( | ||
| request: Request, # noqa: ARG001 - required for authentication decorator | ||
| payload: WalkthroughRequest, | ||
| ) -> WalkthroughResponse: |
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.
why not use WalkthroughSchema here?
| if ext in {".html", ".htm"}: | ||
| return WalkthroughFormat.HTML | ||
| if ext in {".txt", ".text", ".md"}: | ||
| return WalkthroughFormat.TEXT |
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.
re my comment earlier, this can be a computed @Property on the model or in pydantic
| lazy="raise", back_populates="rom" | ||
| ) | ||
| rom_users: Mapped[list[RomUser]] = relationship(lazy="raise", back_populates="rom") | ||
| walkthroughs: Mapped[list["Walkthrough"]] = relationship( |
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.
strings not necessary if its only imported when type checking
| walkthroughs: Mapped[list["Walkthrough"]] = relationship( | |
| walkthroughs: Mapped[list[Walkthrough]] = relationship( |
| D:DB_HOST=127.0.0.1 | ||
| D:DB_NAME=romm_test | ||
| D:DB_USER=romm_test | ||
| D:DB_PASSWD=passwd |
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.
what does D: do?
Description
This PR adds a new tab in each rom for walkthroughs.
Missing
Checklist
Please check all that apply.
Screenshots (if applicable)