Use aspect ratio when making thumbnails#857
Conversation
|
Warning Rate limit exceeded@tankerkiller125 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
WalkthroughThe thumbnail generation logic was refactored to preserve the original image's aspect ratio by calculating scaled dimensions that fit within configured maximum thumbnail sizes. A new helper method centralizes image processing steps including orientation correction, resizing, encoding to WebP, and storage upload. Test calls were updated to include group ID parameters. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AttachmentRepo
participant ImageDecoder
participant Storage
User->>AttachmentRepo: CreateThumbnail(image)
AttachmentRepo->>ImageDecoder: Decode image metadata and orientation
ImageDecoder-->>AttachmentRepo: Return image and orientation
AttachmentRepo->>AttachmentRepo: ApplyOrientation(image, orientation)
AttachmentRepo->>AttachmentRepo: calculateThumbnailDimensions(origWidth, origHeight, maxWidth, maxHeight)
AttachmentRepo->>AttachmentRepo: Resize image to calculated dimensions
AttachmentRepo->>AttachmentRepo: Encode image as WebP
AttachmentRepo->>Storage: Upload thumbnail
Storage-->>AttachmentRepo: Return thumbnail path
AttachmentRepo->>User: Return thumbnail path
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Poem
Security Recommendation: ✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/internal/data/repo/repo_item_attachments.go (1)
847-874: Add input validation for security hardening.The aspect ratio calculation logic is mathematically sound and handles edge cases well. However, consider adding input validation to prevent potential misuse.
func calculateThumbnailDimensions(origWidth, origHeight, maxWidth, maxHeight int) (int, int) { + // Validate input parameters + if origWidth <= 0 || origHeight <= 0 || maxWidth <= 0 || maxHeight <= 0 { + return 1, 1 + } + if origWidth <= maxWidth && origHeight <= maxHeight { return origWidth, origHeight }Security recommendation: While the current implementation is secure, adding explicit input validation provides defense-in-depth against potential integer overflow or division-by-zero scenarios, even though the current callers pass valid values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/internal/data/repo/repo_item_attachments.go(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
backend/internal/data/repo/repo_item_attachments.go (1)
Learnt from: tankerkiller125
PR: sysadminsmedia/homebox#777
File: backend/internal/data/repo/repo_item_attachments.go:187-196
Timestamp: 2025-06-08T00:32:27.457Z
Learning: In the Homebox application, file upload size limits are handled at the admin configuration level elsewhere in the application, not at the individual file processing method level in backend/internal/data/repo/repo_item_attachments.go.
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 4/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 3/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 2/4
- GitHub Check: Frontend Tests / Integration Tests PGSQL 17
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 1/4
- GitHub Check: Backend Server Tests / Go
- GitHub Check: Frontend Tests / Integration Tests PGSQL 16
- GitHub Check: Frontend Tests / Integration Tests PGSQL 15
- GitHub Check: Frontend Tests / Integration Tests
- GitHub Check: Frontend Tests / Lint
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (actions)
🔇 Additional comments (2)
backend/internal/data/repo/repo_item_attachments.go (2)
525-527: LGTM! Consistent implementation across all image formats.The aspect ratio preservation logic is correctly and consistently applied across all supported image formats (JPEG/PNG/GIF, WebP, AVIF, HEIC/HEIF, JPEG XL). The pattern of getting bounds, calculating dimensions, and creating appropriately sized destination images is well-implemented.
Also applies to: 565-567, 605-607, 645-647, 685-687
486-488: Security recommendation: File size limit provides good DoS protection.The 100MB file size limit effectively prevents resource exhaustion attacks through oversized image uploads. This is a solid security practice for thumbnail generation.
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
backend/go.mod(4 hunks)backend/internal/data/repo/repo_item_attachments.go(8 hunks)backend/pkgs/utils/image.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/internal/data/repo/repo_item_attachments.go
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Backend Server Tests / Go
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 2/4
- GitHub Check: Frontend Tests / Lint
- GitHub Check: Frontend Tests / Integration Tests
- GitHub Check: Frontend Tests / Integration Tests PGSQL 17
- GitHub Check: Frontend Tests / Integration Tests PGSQL 15
- GitHub Check: Frontend Tests / Integration Tests PGSQL 16
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (actions)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/api/main.go(1 hunks)backend/internal/data/repo/repo_item_attachments.go(8 hunks)backend/pkgs/utils/image.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/internal/data/repo/repo_item_attachments.go
- backend/pkgs/utils/image.go
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Frontend Tests / Integration Tests PGSQL 15
- GitHub Check: Frontend Tests / Integration Tests PGSQL 17
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 4/4
- GitHub Check: Frontend Tests / Integration Tests PGSQL 16
- GitHub Check: Frontend Tests / Lint
- GitHub Check: Backend Server Tests / Go
- GitHub Check: Frontend Tests / Integration Tests
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm64)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
I believe this PR introduced bug where images are squared all the time. |
|
I tested this PR on all the supported formats, and all of them where coming out in the correct aspect ratio, which version @StanislavJochman are you running? This PR has not made it into a tagged release yet. |
I am running Version: v0.20.2 Build: |
|
@StanislavJochman The aspect ratio being broken is a known issue in v0.20.2, this PR will fix it once a tagged release is made. |
|
@tankerkiller125 is this still an issue? Image: Version: v0.21.0 Build: 27e9eb2 ~ API |
|
@samuelkdavis see #715 |
4 participants
What type of PR is this?
What this PR does / why we need it:
Resolves issue where thumbnails were not taking aspect ratios into account. This PR takes the aspect ratio into account and still keeps the max size correct.
Which issue(s) this PR fixes:
Fixes: #848
Fixes: #844
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes