FEATURE: automatically downsize large images #1
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements automatic image downsizing functionality that allows users to upload large images which are then automatically reduced to fit within configured size limits. The implementation spans three key areas:
Backend Processing (uploads_controller.rb): The core feature adds automatic downsizing logic that iteratively reduces large images by 80% using OptimizedImage.downsize() until they meet size requirements. The API download limit is increased from the site setting to a fixed 10MB to accommodate larger initial uploads.
Image Processing (optimized_image.rb): The model is refactored to simplify parameter passing by formatting dimension strings at the method entry points rather than using a separate dimensions helper. The resize and downsize methods now format dimensions as strings (e.g., "800x600") and pass them directly to the optimize method.
Frontend Validation (utilities.js): File size validation is updated to use hardcoded 10MB limits instead of dynamic site settings, both in the file validation function and HTTP 413 error handling.
This change integrates with Discourse's existing image processing pipeline that handles thumbnail generation and optimization, leveraging ImageMagick's convert command through the OptimizedImage class. The feature aims to improve user experience by preventing upload failures and eliminating the need for manual image resizing.
Confidence score: 1/5
• This PR contains critical bugs that will break existing functionality and should not be merged
• The OptimizedImage model has duplicate method definitions that will cause runtime failures
• Multiple files contain potential infinite loops, missing error handling, and security vulnerabilities that need immediate attention
3 files reviewed, 4 comments
| def self.downsize(from, to, dimensions, opts={}) | ||
| optimize("downsize", from, to, dimensions, opts) | ||
| end |
There was a problem hiding this comment.
logic: Duplicate method definition - this downsize method will override the previous one on lines 145-147, breaking the API that expects separate max_width and max_height parameters
| def self.downsize(from, to, dimensions, opts={}) | |
| optimize("downsize", from, to, dimensions, opts) | |
| end | |
| def self.downsize_with_dimensions(from, to, dimensions, opts={}) | |
| optimize("downsize", from, to, dimensions, opts) | |
| end |
| // 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.
logic: Hardcoding file size limits bypasses site settings and breaks configurability. Consider using a larger site setting or removing frontend validation for images since backend now handles downsizing.
| 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 |
There was a problem hiding this comment.
logic: Potential infinite loop if OptimizedImage.downsize fails to reduce file size. The loop only decrements attempt but doesn't verify size reduction occurred.
| 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.
logic: Using potentially stale tempfile.size after downsizing operations. The size should be refreshed to reflect actual file size changes.
| upload = Upload.create_for(current_user.id, tempfile, filename, tempfile.size, content_type: content_type, image_type: type) | |
| # Refresh tempfile size after potential downsizing | |
| tempfile.rewind if tempfile.respond_to?(:rewind) | |
| actual_size = File.size(tempfile.path) | |
| upload = Upload.create_for(current_user.id, tempfile, filename, actual_size, content_type: content_type, image_type: type) |
2 participants
Test 1