FEATURE: automatically downsize large images#10
Conversation
|
cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| dim = dimensions(width, height) | ||
| def self.downsize(from, to, dimensions, opts={}) | ||
| optimize("downsize", from, to, dimensions, opts) | ||
| end |
There was a problem hiding this comment.
Bug: Method Overload Conflict Breaks Downsize
Ruby doesn't support method overloading, so defining downsize twice causes the second definition to completely replace the first. The 4-parameter version downsize(from, to, max_width, max_height, opts={}) is lost, breaking existing callers like resize_emoji.rb that pass separate width and height values. The methods need different names or a single method that handles both parameter patterns.
| // 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.
Bug: Client validation ignores site configuration.
Hardcoding maxSizeKB to 10 * 1024 ignores the site's configured max_image_size_kb or max_attachment_size_kb settings. Since the feature downsizes large images server-side, the client validation should allow larger uploads or match the server's actual limit, not enforce an arbitrary 10MB cap that may conflict with site configuration.
| # API can provide a URL | ||
| if file.nil? && url.present? && is_api? | ||
| tempfile = FileHelper.download(url, SiteSetting.max_image_size_kb.kilobytes, "discourse-upload-#{type}") rescue nil | ||
| tempfile = FileHelper.download(url, 10.megabytes, "discourse-upload-#{type}") rescue nil |
There was a problem hiding this comment.
Bug: Hardcoded Limits Bypass Admin Control
Hardcoding the download limit to 10.megabytes ignores SiteSetting.max_image_size_kb for API-provided URLs. This creates inconsistent behavior where the configured site limit is bypassed, and may allow downloads that exceed or unnecessarily restrict what the site administrator intended.
| OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails) | ||
| attempt -= 1 | ||
| end | ||
| end |
There was a problem hiding this comment.
Bug: Infinite Downsizing Loop for Oversized Images
The downsizing loop always runs exactly 5 times for oversized images because tempfile.size reflects the in-memory object's size attribute, not the actual file size on disk after OptimizedImage.downsize modifies it. The loop condition tempfile.size > SiteSetting.max_image_size_kb.kilobytes never becomes false, causing unnecessary repeated downsizing to approximately 33% of original size (0.8^5) regardless of when the target size is reached.
2 participants
Test 1
Replicated from ai-code-review-evaluation/discourse-cursor#1
Note
Automatically downsizes oversized images during upload and standardizes a 10MB size limit in client validation and API downloads, with imaging utilities updated to accept dimension strings/percentages.
app/controllers/uploads_controller.rb: For API URL uploads, cap download size at10 MB; auto-downsize oversized images iteratively (80% per pass, up to 5 times) to meetSiteSetting.max_image_size_kb, honoringallow_animated_thumbnails.app/models/optimized_image.rb: Refactorresize/downsizeto pass dimension strings (e.g.,"#{w}x#{h}", percentages) and updateoptimizesignature; remove width/height helper.app/assets/javascripts/discourse/lib/utilities.js: Enforce a fixed10 MBmax in upload validation and 413 error display instead of site settings.Written by Cursor Bugbot for commit ffbaf8c. Configure here.