FEATURE: automatically downsize large images #11
Conversation
Code Review by Qodo (Alpha)Analysis CompletedPublished 5 inline comment(s) |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| optimize("downsize", from, to, "#{max_width}x#{max_height}", opts) | ||
| end | ||
|
|
||
| def self.optimize(operation, from, to, width, height, opts={}) | ||
| 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.
[action_required] [Ruby method overloading breaks emoji resize]
• Ruby does not support method overloading - defining two methods with the same name causes the second to completely replace the first
• The PR defines downsize(from, to, max_width, max_height, opts={}) then downsize(from, to, dimensions, opts={}) - only the latter survives
• The ResizeEmoji job calls OptimizedImage.downsize(path, path, 100, 100, opts) with 5 arguments, which will now raise ArgumentError: wrong number of arguments (given 5, expected 3..4)
Agent Prompt
Copy this prompt and use it to remediate the issue with your preferred AI generation tools
## Issue description
Ruby doesn't support method overloading. The PR defines two `downsize` methods with different signatures, but the second definition completely replaces the first. This breaks the `ResizeEmoji` job which still uses the old 5-argument signature.
## Issue Context
- The first `downsize(from, to, max_width, max_height, opts={})` method is shadowed
- The second `downsize(from, to, dimensions, opts={})` method is the only one that exists
- `ResizeEmoji` job calls with 5 arguments: `OptimizedImage.downsize(path, path, 100, 100, opts)`
## Fix Focus Areas
- app/models/optimized_image.rb[145-151]
| // 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.
[action_required] [Hardcoded values replace configurable settings]
• The JavaScript code now uses hardcoded 10 * 1024 (10MB) instead of reading from Discourse.SiteSettings['max_' + type + '_size_kb']
• This removes administrator control over upload file size limits configured in site settings
• Error messages will always display '10MB' even if the actual server-side limit differs, causing user confusion
Agent Prompt
Copy this prompt and use it to remediate the issue with your preferred AI generation tools
## Issue description
Hardcoded 10MB values replace configurable site settings, removing admin control over upload size limits and causing inconsistent error messages.
## Issue Context
- Original code used `Discourse.SiteSettings['max_' + type + '_size_kb']` and `Discourse.SiteSettings.max_image_size_kb`
- Server-side still uses `SiteSetting.max_image_size_kb` for validation
- Error messages will show wrong values to users
## Fix Focus Areas
- app/assets/javascripts/discourse/lib/utilities.js[180-186]
- app/assets/javascripts/discourse/lib/utilities.js[243-248]
| tempfile = FileHelper.download(url, 10.megabytes, "discourse-upload-#{type}") rescue nil | ||
| filename = File.basename(URI.parse(url).path) | ||
| else | ||
| tempfile = file.tempfile | ||
| filename = file.original_filename | ||
| content_type = file.content_type | ||
| end | ||
|
|
||
| # 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 | ||
| 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.
[remediation_recommended] [Inconsistent download and validation limits]
• The download limit is hardcoded to 10.megabytes but validation uses SiteSetting.max_image_size_kb
• If an admin sets max_image_size_kb to a value larger than 10MB, API downloads will fail before the file can be processed
• Creates unpredictable behavior depending on configuration
Agent Prompt
Copy this prompt and use it to remediate the issue with your preferred AI generation tools
## Issue description
Download limit is hardcoded to 10MB but validation uses site settings, creating inconsistency.
## Issue Context
- Original code: `FileHelper.download(url, SiteSetting.max_image_size_kb.kilobytes, ...)`
- New code: `FileHelper.download(url, 10.megabytes, ...)`
- Validation still uses: `SiteSetting.max_image_size_kb.kilobytes`
## Fix Focus Areas
- app/controllers/uploads_controller.rb[54-56]
| # 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 | ||
| 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 | ||
|
|
There was a problem hiding this comment.
[remediation_recommended] [No error handling for failed downsizing]
• The downsizing loop runs up to 5 times reducing image by 80% each iteration
• If the image is still too large after 5 attempts, the code silently proceeds to upload
• No logging or error returned when downsizing fails to achieve the target size, making debugging difficult
Agent Prompt
Copy this prompt and use it to remediate the issue with your preferred AI generation tools
## Issue description
No validation or logging when image downsizing fails to achieve target size after 5 attempts.
## Issue Context
- Loop reduces image by 80% up to 5 times
- No check after loop if image is now within limits
- Proceeds to Upload.create_for which will likely fail validation
## Fix Focus Areas
- app/controllers/uploads_controller.rb[63-72]
| } | ||
| } | ||
|
|
||
| // check file size |
There was a problem hiding this comment.
[action_required] [Hardcoded values replace configurable settings]
• The JavaScript code now uses hardcoded 10 * 1024 (10MB) instead of reading from Discourse.SiteSettings['max_' + type + '_size_kb']
• This removes administrator control over upload file size limits configured in site settings
• Error messages will always display '10MB' even if the actual server-side limit differs, causing user confusion
Agent Prompt
Copy this prompt and use it to remediate the issue with your preferred AI generation tools
## Issue description
Hardcoded 10MB values replace configurable site settings, removing admin control over upload size limits and causing inconsistent error messages.
## Issue Context
- Original code used `Discourse.SiteSettings['max_' + type + '_size_kb']` and `Discourse.SiteSettings.max_image_size_kb`
- Server-side still uses `SiteSetting.max_image_size_kb` for validation
- Error messages will show wrong values to users
## Fix Focus Areas
- app/assets/javascripts/discourse/lib/utilities.js[180-186]
- app/assets/javascripts/discourse/lib/utilities.js[243-248]
Benchmark PR from qodo-benchmark/discourse-qodo#1