fix(share): Share square image #4122
Conversation
Signed-off-by: Deluan <deluan@navidrome.org>
There was a problem hiding this comment.
Hello @kgarner7, 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!
Summary of Changes
Hello team, gemini-code-assist here to provide a summary of this pull request. This PR addresses an issue where non-square images used for sharing were causing an offset or display problem. The core change is to introduce a mechanism to request and potentially generate square versions of artwork images specifically for the share feature, ensuring a consistent and correct display.
Highlights
- Image Handling Enhancement: The backend image handling endpoint (
/img/:id) has been updated to accept an optionalsquarequery parameter. This allows clients to request a square version of the image. - Frontend Integration for Sharing: The frontend code responsible for generating the cover image URL for the share player now explicitly requests a square image by passing
trueto the updated URL helper function.
Changelog
- server/public/handle_images.go
- Added a new
squareboolean parameter, read from the request query parameters usingp.BoolOr("square", false)(line 40). - Passed the new
squareparameter to thepub.artwork.Getfunction call (line 42).
- Added a new
- ui/src/share/SharePlayer.jsx
- Modified the call to
shareCoverUrlto passtrueas the second argument, explicitly requesting a square image for the share cover (line 32).
- Modified the call to
- ui/src/utils/urls.js
- Updated the
shareCoverUrlfunction signature to accept a second parameter,square(line 36). - Appended
&square=trueto the generated image URL query string if thesquareparameter is truthy (lines 41-42).
- Updated the
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
Images shared online,
Sometimes look quite out of line.
Add a square flag,
No more image lag,
Now the cover looks just fine.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a square parameter to image handling, allowing the frontend to request square cover images for the share player. This is intended to fix an issue where non-square images were causing layout offsets. The changes in the Go backend and JavaScript frontend seem to correctly plumb this new flag through the system.
The code is clear and directly addresses the described problem. I have one suggestion regarding testing the new functionality in the backend.
Summary of Findings
- Testing for new
squareparameter: A review comment was added suggesting to enhance test coverage incore/artwork/artwork_test.gofor theartwork.Getmethod to specifically include cases where the newsquareparameter istrue. This is to ensure the image processing logic (resizing, padding, etc.) behaves as expected with this flag. - Hardcoded image size in
shareCoverUrl: Inui/src/utils/urls.js, theshareCoverUrlfunction uses a hardcodedsize=300. While this might be the current requirement, if there's a future need for different sizes for shared covers, this might need to be parameterized. This was deemed low severity and not commented on directly due to review settings. - URL string construction in
shareCoverUrl: Inui/src/utils/urls.js, the URL path and query string are built using string concatenation. For example:config.publicBaseUrl + '/img/' + id + '?size=300' + (square ? '&square=true' : ''). While functional for the current case, using template literals (e.g.,`${config.publicBaseUrl}/img/${id}?size=300${squareParam}`) could offer slightly improved readability. This is a minor stylistic point, considered low severity, and not commented on directly due to review settings.
Merge Readiness
The pull request effectively implements the necessary changes to allow requesting square share images. The code is clear and well-targeted.
I've raised one medium-severity suggestion regarding enhancing test coverage for the backend artwork processing with the new square flag. Addressing this would further improve the robustness of the change.
As a reviewer, I am not authorized to approve pull requests. Please ensure further review and approval from authorized maintainers before merging, especially after considering the feedback on testing.
|
|
||
| imgReader, lastUpdate, err := pub.artwork.Get(ctx, artId, size, false) | ||
| imgReader, lastUpdate, err := pub.artwork.Get(ctx, artId, size, square) |
There was a problem hiding this comment.
This change correctly passes the new square parameter to pub.artwork.Get. This is key for the new functionality.
To ensure the underlying image processing behaves as expected when square is true, have we considered adding or updating unit tests for the artwork.Get method, likely in core/artwork/artwork_test.go? Specifically, test cases where square is true would be valuable to verify that images are indeed processed to be square (e.g., resized, padded, or cropped as intended by the resizedFromOriginal logic).
This would increase confidence that the core artwork processing correctly handles this new flag under various conditions.
Make non-square images for share square to make the image not offset