FEATURE: automatically downsize large images#10
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements automatic image downsizing functionality to handle images that exceed upload size limits. Instead of rejecting large images outright, the system now attempts to reduce their size automatically before upload.
Changes:
- Modified image optimization methods to accept dimension strings directly (e.g., "80%") instead of separate width/height parameters
- Added automatic image downsizing loop in the upload controller that iteratively reduces oversized images
- Increased client-side and server-side file size limits from configured values to 10MB to allow large images to be uploaded for automatic downsizing
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| app/models/optimized_image.rb | Refactored optimization methods to accept dimension strings and removed the dimensions helper method |
| app/controllers/uploads_controller.rb | Added automatic downsizing loop for oversized images and increased download limit to 10MB |
| app/assets/javascripts/discourse/lib/utilities.js | Increased client-side file size validation limit from configured setting to hardcoded 10MB |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def self.downsize(from, to, dimensions, opts={}) | ||
| optimize("downsize", from, to, dimensions, opts) | ||
| end |
There was a problem hiding this comment.
Method overloading is not supported in Ruby. Having two downsize methods with different arities will cause the second definition (lines 149-151) to completely replace the first one (lines 145-147). Callers using the 4-parameter signature will encounter ArgumentError. Consider renaming one method or using optional parameters with different default values.
| if tempfile && tempfile.size > 0 && SiteSetting.max_image_size_kb > 0 && FileHelper.is_image?(filename) | ||
| attempt = 5 | ||
| while attempt > 0 && tempfile.size > SiteSetting.max_image_size_kb.kilobytes | ||
| OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails) | ||
| attempt -= 1 | ||
| end | ||
| end | ||
|
|
||
| upload = Upload.create_for(current_user.id, tempfile, filename, tempfile.size, content_type: content_type, image_type: type) |
There was a problem hiding this comment.
The downsizing loop does not refresh tempfile.size after each iteration. Since the method writes to the same file path, tempfile.size will remain unchanged, creating an infinite loop until attempt reaches 0. Consider using File.size(tempfile.path) to get the updated file size after each downsizing operation.
| if tempfile && tempfile.size > 0 && SiteSetting.max_image_size_kb > 0 && FileHelper.is_image?(filename) | |
| attempt = 5 | |
| while attempt > 0 && tempfile.size > SiteSetting.max_image_size_kb.kilobytes | |
| OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails) | |
| attempt -= 1 | |
| end | |
| end | |
| upload = Upload.create_for(current_user.id, tempfile, filename, tempfile.size, content_type: content_type, image_type: type) | |
| current_size = tempfile.size if tempfile | |
| if tempfile && current_size.to_i > 0 && SiteSetting.max_image_size_kb > 0 && FileHelper.is_image?(filename) | |
| attempt = 5 | |
| while attempt > 0 && current_size > SiteSetting.max_image_size_kb.kilobytes | |
| OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails) | |
| current_size = File.size(tempfile.path) | |
| attempt -= 1 | |
| end | |
| end | |
| upload = Upload.create_for(current_user.id, tempfile, filename, (current_size || tempfile.size), content_type: content_type, image_type: type) |
| // check file size | ||
| var fileSizeKB = file.size / 1024; | ||
| var maxSizeKB = Discourse.SiteSettings['max_' + type + '_size_kb']; | ||
| var maxSizeKB = 10 * 1024; // 10MB |
There was a problem hiding this comment.
Hardcoding the 10MB limit in multiple places (lines 182 and 246) creates a magic number that's duplicated across the codebase. Consider defining this as a constant at the module level to ensure consistency and ease maintenance.
|
|
||
| # allow users to upload large images that will be automatically reduced to allowed size | ||
| if tempfile && tempfile.size > 0 && SiteSetting.max_image_size_kb > 0 && FileHelper.is_image?(filename) | ||
| attempt = 5 |
There was a problem hiding this comment.
The magic number 5 for retry attempts lacks explanation. Consider extracting this to a named constant (e.g., MAX_DOWNSIZE_ATTEMPTS = 5) to improve code clarity and maintainability.
Test 1
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#1