Conversation
Summary of ChangesHello @tavdog, 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 introduces a new user preference that allows individuals to customize the default placement of newly added applications within their device's app list. This enhancement provides users with greater control over their app organization, enabling them to choose whether new apps appear at the top or bottom of their list. The change includes updates to the user data model, backend API for preference management, and the front-end user interface for configuration. 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.
Code Review
This pull request introduces a user preference to add new apps to the top of the list instead of the bottom. The changes are well-implemented across the data model, UI template, and server-side handlers. However, I've identified a critical issue in internal/server/handlers_app.go where the database operations for reordering and adding an app are not atomic, which could lead to data inconsistency. I've provided a detailed comment with a code suggestion to resolve this.
internal/server/handlers_app.go
Outdated
| newApp.Order = 1 | ||
| } else { | ||
| var maxOrder int64 | ||
| if err := tx.Model(&data.App{}).Where("device_id = ?", device.ID).Select("COALESCE(MAX(`order`), -1)").Scan(&maxOrder).Error; err != nil { |
There was a problem hiding this comment.
This should use getMaxAppOrder().
This query does not work across database dialects because order is a SQL keyword which must be escaped specially.
Same for the code above.
|
/gemini codereview |
|
It looks like you tried to invoke the |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a user preference to add new applications to the top of the list. The implementation involves changes to the data model, the user settings page, and the application creation logic. My review found a high-severity issue in the app ordering logic that could lead to data inconsistency, and a medium-severity issue regarding the use of inline styles in the HTML template which affects maintainability. These comments align with the provided rules or are not covered by them, and no modifications or drops were necessary.
| <div style="margin-bottom: 15px;"> | ||
| <div style="display: flex; align-items: center; gap: 10px;"> | ||
| <input type="checkbox" | ||
| name="add_apps_to_top" | ||
| id="add_apps_to_top" | ||
| value="1" | ||
| {{ if .User.AddAppsToTop }} | ||
| checked | ||
| {{ end }} | ||
| style="width: 18px; | ||
| height: 18px"> | ||
| <label for="add_apps_to_top" style="margin: 0;">{{ t .Localizer "Add new apps to top of list" }}</label> | ||
| </div> | ||
| <p style="color: #888; | ||
| font-size: 0.8em; | ||
| margin-top: 5px; | ||
| margin-bottom: 0"> | ||
| {{ t .Localizer "When enabled, newly added apps will appear at position #1 instead of at the end." }} | ||
| </p> | ||
| </div> |
There was a problem hiding this comment.
This new section adds a number of inline styles. While this works, it's generally better for maintainability to use CSS classes and define the styles in a separate stylesheet. This keeps the HTML cleaner and makes it easier to apply consistent styling across the application. Consider creating a few utility classes for this component and adding them to your site's stylesheet.
internal/server/handlers_app.go
Outdated
| if user.AddAppsToTop { | ||
| if _, err := gorm.G[data.App](tx). | ||
| Where("device_id = ?", device.ID). | ||
| Update(r.Context(), "order", gorm.Expr("`order` + 1")); err != nil { |
There was a problem hiding this comment.
`order` + 1 has the same escaping issue. GORM can only correctly escape it (SQL dialect dependent) if it's specified as a column name alone, like in clause.Column{Name: "order"}
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
No description provided.