More image type support for thumbnails#814
Conversation
WalkthroughSupport for HEIC/HEIF and JPEG XL image formats was added across backend logic, including thumbnail creation and image type detection. The frontend was updated to utilize responsive image loading via Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant Storage
User->>Frontend: View item with attachments
Frontend->>Backend: Request item attachments
Backend->>Storage: Retrieve attachments (including .heic, .jxl)
Backend->>Backend: Detect MIME type, create thumbnails if needed
Backend->>Storage: Upload thumbnails
Backend->>Frontend: Return attachment info with thumbnails and MIME types
Frontend->>User: Render images using <picture> with original and thumbnail sources
Suggested labels
Suggested reviewers
Poem
Security Recommendations
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (18)
✨ 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: 1
🔭 Outside diff range comments (1)
backend/app/api/main.go (1)
348-368: Critical: Error handling changes mask security issues.The modified error handling is concerning from a security perspective. Previously, errors would terminate processing, but now they're only logged while processing continues. This could mask:
- Malformed UUID attacks - Invalid group_id/attachment_id values are logged but processing continues
- Thumbnail creation failures - Failed image processing (potentially from malicious images) is logged but the message is still acknowledged
- Resource exhaustion - Processing continues even when thumbnail creation fails due to resource issues
This creates a security vulnerability where malicious actors could submit crafted requests that cause errors but still get processed.
Consider implementing proper error categorization:
msg, err := subscription.Receive(ctx) if err != nil { log.Err(err).Msg("failed to receive message from pubsub topic") + continue // Skip this iteration on receive errors } groupId, err := uuid.Parse(msg.Metadata["group_id"]) if err != nil { log.Error().Err(err).Str("group_id", msg.Metadata["group_id"]).Msg("failed to parse group ID") + msg.Nack() // Reject malformed messages + continue } attachmentId, err := uuid.Parse(msg.Metadata["attachment_id"]) if err != nil { log.Error().Err(err).Str("attachment_id", msg.Metadata["attachment_id"]).Msg("failed to parse attachment ID") + msg.Nack() // Reject malformed messages + continue } err = app.repos.Attachments.CreateThumbnail(ctx, groupId, attachmentId, msg.Metadata["title"], msg.Metadata["path"]) if err != nil { log.Err(err).Msg("failed to create thumbnail") + msg.Nack() // Reject failed processing + continue } msg.Ack()
📜 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 (5)
backend/app/api/handlers/v1/v1_ctrl_items_attachments.go(1 hunks)backend/app/api/main.go(1 hunks)backend/go.mod(1 hunks)backend/internal/data/repo/repo_item_attachments.go(4 hunks)frontend/pages/item/[id]/index.vue(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/riscv64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/arm64)
- GitHub Check: Frontend Tests / Integration Tests PGSQL 15
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 4/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 1/4
- GitHub Check: Frontend Tests / Integration Tests PGSQL 16
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 3/4
- GitHub Check: Frontend Tests / Integration Tests PGSQL 17
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 2/4
- GitHub Check: Backend Server Tests / Go
- GitHub Check: Frontend Tests / Integration Tests
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/riscv64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
backend/go.mod (1)
12-13: Let’s fetch each repo’s popularity, last update, and license to ensure they’re well-maintained and properly licensed:#!/bin/bash echo "Gathering metadata for gen2brain/heic..." gh repo view gen2brain/heic --json stargazerCount,pushedAt,licenseInfo echo "Gathering metadata for gen2brain/jpegxl..." gh repo view gen2brain/jpegxl --json stargazerCount,pushedAt,licenseInfobackend/app/api/handlers/v1/v1_ctrl_items_attachments.go (1)
85-85: LGTM - File extension detection extended for new formats.The addition of
.heicand.jxlextensions follows the existing pattern for photo type detection. Note that this relies on file extensions rather than content-based validation, but the actual image processing and validation occurs downstream in the thumbnail creation pipeline.frontend/pages/item/[id]/index.vue (3)
103-103: LGTM - TypeScript type extension for responsive images.The optional
srcsetproperty correctly extends the Photo type to support responsive image loading.
110-122: LGTM - Proper responsive image implementation.The logic correctly implements responsive image loading by using thumbnails as the primary
srcand original images assrcsetwhen thumbnails are available. This provides good performance optimization for image display.
692-692: LGTM - Standard srcset implementation.The template correctly binds both
srcandsrcsetattributes for responsive image loading.backend/app/api/main.go (1)
312-312: Asynchronous execution is appropriate for background processing.Running the thumbnail subscription asynchronously is correct for background task processing.
backend/internal/data/repo/repo_item_attachments.go (3)
15-16: LGTM - Required imports for new image format support.The imports for HEIC and JPEG XL decoding libraries are necessary for the new functionality.
422-428: LGTM - Content type detection enhancement.The fallback content type detection for HEIC/HEIF and AVIF files is necessary since
http.DetectContentTypemay not recognize these newer formats. This follows a secure approach by checking file extensions as a fallback.
92-92: Verify performance impact of thumbnail query.The added thumbnail query in
ToItemAttachmentcould impact performance if called frequently, as it adds an additional database query for each attachment.#!/bin/bash # Check how frequently ToItemAttachment is called rg -A 5 -B 5 "ToItemAttachment" --type go
|
@tonyaellie I'm not 100% sure on the Vue updates here, it seems to work sometimes, but not always for HEIC/JPEG XL files. I'm really not sure if src and srcset is the correct way to do this. I should note that it works just fine in the item grid, it's only on the actual individual item page that I'm having trouble with. |
|
The best approach would probably be to generate a thumbnail and a high res version, then use the thumbnail for the like small picture and the high res version for the dialog, and then only use the original when they click download. I dont think srcset is really needed. |
|
Reading ur message on discord if u just want to first try the normal image and then fallback to the thumbnail you will want something like |
…if browser doesn't support the image type
Deploying homebox-docs with
|
| Latest commit: |
7b755ac
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://da287401.homebox-docs.pages.dev |
| Branch Preview URL: | https://mk-more-thumbnails.homebox-docs.pages.dev |
coderabbitai
bot
added
go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/internal/data/repo/repo_item_attachments.go (1)
558-633: Security and maintainability issues in thumbnail creation.The HEIC and JPEG XL thumbnail creation code has several critical issues that need to be addressed:
- Security vulnerability: No input validation for image dimensions after decoding, which could lead to memory exhaustion attacks
- Error message inconsistency: Both HEIC and JPEG XL error handlers incorrectly log "failed to decode avif image"
- Code duplication: Significant duplication across all image format handlers
Security fixes needed:
case contentType == "image/heic" || contentType == "image/heif": log.Debug().Msg("creating thumbnail for heic file") img, err := heic.Decode(bytes.NewReader(contentBytes)) if err != nil { - log.Err(err).Msg("failed to decode avif image") + log.Err(err).Msg("failed to decode heic image") // ... error handling } + + // Validate decoded image dimensions + bounds := img.Bounds() + if bounds.Dx() > 20000 || bounds.Dy() > 20000 { + return fmt.Errorf("image dimensions too large: %dx%d", bounds.Dx(), bounds.Dy()) + } case contentType == "image/jxl": log.Debug().Msg("creating thumbnail for jpegxl file") img, err := jpegxl.Decode(bytes.NewReader(contentBytes)) if err != nil { - log.Err(err).Msg("failed to decode avif image") + log.Err(err).Msg("failed to decode jpeg xl image") // ... error handling } + + // Validate decoded image dimensions + bounds := img.Bounds() + if bounds.Dx() > 20000 || bounds.Dy() > 20000 { + return fmt.Errorf("image dimensions too large: %dx%d", bounds.Dx(), bounds.Dy()) + }Refactor suggestion: Extract the common thumbnail creation logic into a shared function to eliminate code duplication and ensure consistent security measures across all image formats.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (15)
backend/app/api/static/docs/docs.gois excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/swagger.jsonis excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/swagger.yamlis excluded by!backend/app/api/static/docs/**backend/go.sumis excluded by!**/*.sumbackend/internal/data/ent/attachment.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment/attachment.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment/where.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/migrate/schema.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/mutation.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/runtime.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/attachment.gois excluded by!backend/internal/data/ent/**docs/en/api/openapi-2.0.jsonis excluded by!docs/en/api/**docs/en/api/openapi-2.0.yamlis excluded by!docs/en/api/**
📒 Files selected for processing (5)
backend/internal/data/migrations/postgres/20250625120010_add_mime_type.sql(1 hunks)backend/internal/data/migrations/sqlite3/20250625120000_add_mime_type.sql(1 hunks)backend/internal/data/repo/repo_item_attachments.go(6 hunks)frontend/lib/api/types/data-contracts.ts(2 hunks)frontend/pages/item/[id]/index.vue(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- backend/internal/data/migrations/sqlite3/20250625120000_add_mime_type.sql
- frontend/lib/api/types/data-contracts.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/pages/item/[id]/index.vue
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 4/4
- GitHub Check: Frontend Tests / Integration Tests PGSQL 16
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 2/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 3/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 1/4
- GitHub Check: Backend Server Tests / Go
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/riscv64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/riscv64)
- GitHub Check: build (linux/arm64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
backend/internal/data/migrations/postgres/20250625120010_add_mime_type.sql (1)
1-4: Migration looks good!The migration correctly adds the
mime_typecolumn with an appropriate default value. The syntax is correct and the change is non-breaking.backend/internal/data/repo/repo_item_attachments.go (5)
15-16: New image format dependencies added correctly.The imports for HEIC and JPEG XL libraries are properly added to support the new image formats.
67-67: MimeType field addition looks good.The
MimeTypefield is properly added to theItemAttachmentstruct with appropriate JSON tag.
434-440: Content type fallback logic is well implemented.The fallback logic for detecting HEIC/HEIF and AVIF formats when
http.DetectContentTypereturnsapplication/octet-streamis a good solution for handling formats that may not be properly detected by the standard library.
638-638: Good practice: Explicit MIME type setting for thumbnails.Setting the MIME type to
"image/webp"explicitly for all thumbnails ensures consistency and proper handling downstream.
93-95: The previous search didn’t surface any references—let’s locate all uses ofToItemAttachmentandQueryThumbnailto understand how they’re invoked in bulk:#!/bin/bash # Find every occurrence of ToItemAttachment rg --color=never -n 'ToItemAttachment' # Find every occurrence of QueryThumbnail and show nearby lines for context rg --color=never -n 'QueryThumbnail' -A3 -B3Security recommendation: Always validate and sanitize thumbnail data returned from the database to guard against potential injection or data-leak vulnerabilities.
2 participants
What type of PR is this?
What this PR does / why we need it:
Adds HEIC and JPEG XL support for thumbnailing. In theory we should be able to support all the image types with this for displaying in the grids and what not.
Summary by CodeRabbit
New Features
Bug Fixes
Chores