FEATURE: automatically downsize large images#1
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements automatic downsizing of large uploaded images to meet size constraints. Instead of rejecting oversized images, the system now attempts to reduce their file size through iterative compression.
Key Changes:
- Refactored image optimization API to accept dimension strings directly instead of separate width/height parameters
- Added iterative image downsizing logic that attempts up to 5 compression passes at 80% quality
- Increased client-side upload size limit from configurable to fixed 10MB to accommodate large images before server-side processing
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| app/models/optimized_image.rb | Simplified dimension handling by accepting pre-formatted dimension strings and adding overloaded downsize method |
| app/controllers/uploads_controller.rb | Added automatic image downsizing loop with configurable attempts and increased initial download limit |
| app/assets/javascripts/discourse/lib/utilities.js | Replaced dynamic size limit with hardcoded 10MB threshold for client-side validation |
| def self.downsize(from, to, dimensions, opts={}) | ||
| optimize("downsize", from, to, dimensions, opts) | ||
| end |
There was a problem hiding this comment.
Method name collision: there are now two downsize methods with different arities (one with 4 parameters at line 145, one with 3 parameters at line 149). Ruby will only keep the last definition, making the 4-parameter version unreachable. Consider renaming one method or combining them with optional parameters.
| 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) |
There was a problem hiding this comment.
The allow_animation parameter is passed as a keyword argument, but the method signature shows opts={} as the fourth positional parameter. This should be {allow_animation: SiteSetting.allow_animated_thumbnails} wrapped in curly braces, or the method definition needs to accept keyword arguments.
| OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails) | |
| OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", { allow_animation: SiteSetting.allow_animated_thumbnails }) |
| 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 |
There was a problem hiding this comment.
The loop doesn't refresh tempfile.size after downsizing. The file size needs to be re-read from disk after each optimization attempt, otherwise the loop will always run exactly 5 times regardless of whether the target size is reached.
| 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 | |
| current_size = File.size(tempfile.path) | |
| while attempt > 0 && current_size > SiteSetting.max_image_size_kb.kilobytes | |
| OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails) | |
| attempt -= 1 | |
| current_size = File.size(tempfile.path) |
| // 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 duplicates the magic number from uploads_controller.rb (line 55). Consider making this a shared constant or configuration value to ensure consistency across frontend and backend.
| var maxSizeKB = 10 * 1024; // 10MB | |
| var maxSizeKB = Discourse.SiteSettings.max_upload_size_kb; |
| // entity too large, usually returned from the web server | ||
| case 413: | ||
| var maxSizeKB = Discourse.SiteSettings.max_image_size_kb; | ||
| var maxSizeKB = 10 * 1024; // 10 MB |
There was a problem hiding this comment.
This is a duplicate of the same hardcoded limit on line 182. The magic number appears three times across two files (including uploads_controller.rb). Consolidate into a single source of truth.
| var maxSizeKB = 10 * 1024; // 10 MB | |
| var maxSizeKB = Discourse.SiteSettings.max_upload_size_kb; // from site setting |
Test 1
Replicated from ai-code-review-evaluation/discourse-copilot#1