-
Notifications
You must be signed in to change notification settings - Fork 0
FEATURE: automatically downsize large images #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: image-processing-optimization
Are you sure you want to change the base?
FEATURE: automatically downsize large images #10
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Greptile OverviewConfidence Score: 0/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant Client as utilities.js
participant Controller as uploads_controller.rb
participant FileHelper
participant OptimizedImage
participant ImageMagick
User->>Client: Upload large image (e.g. 15MB)
Client->>Client: Check file.size > 10MB
alt File > 10MB (hardcoded)
Client->>User: Show error "file_too_large"
else File <= 10MB
Client->>Controller: POST upload
Controller->>FileHelper: download(url, 10.megabytes)
FileHelper-->>Controller: tempfile
Controller->>Controller: Check if image & size > SiteSetting.max_image_size_kb
alt Needs downsizing
loop attempt 1-5
Controller->>OptimizedImage: downsize(path, path, "80%", opts)
OptimizedImage->>ImageMagick: convert with resize "80%"
ImageMagick-->>OptimizedImage: Process image
OptimizedImage-->>Controller: Return
Controller->>Controller: Check tempfile.size
end
end
Controller->>Controller: Upload.create_for(tempfile)
Controller-->>User: Upload successful
end
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (1)
-
app/assets/javascripts/discourse/lib/utilities.js, line 180-186 (link)logic: hardcoding 10MB contradicts backend auto-downsize feature - users uploading 15MB images will see client-side error despite backend now handling it
also removes dynamic
SiteSettingconfiguration flexibility
3 files reviewed, 4 comments
| def self.downsize(from, to, max_width, max_height, opts={}) | ||
| optimize("downsize", from, to, max_width, max_height, opts) | ||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: duplicate method definition - both downsize methods will cause the second to override the first, breaking calls that pass 4 arguments (like in resize_emoji.rb:14)
| def self.downsize(from, to, max_width, max_height, opts={}) | |
| optimize("downsize", from, to, max_width, max_height, opts) | |
| 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 | |
| def self.downsize(from, to, dimensions_or_width, height_or_opts=nil, opts={}) | |
| if height_or_opts.is_a?(Hash) | |
| # Called with dimensions string: downsize(from, to, "80%", opts) | |
| optimize("downsize", from, to, dimensions_or_width, height_or_opts) | |
| else | |
| # Called with width/height: downsize(from, to, 100, 100, opts) | |
| optimize("downsize", from, to, "#{dimensions_or_width}x#{height_or_opts}", opts) | |
| end | |
| end |
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/models/optimized_image.rb
Line: 145:151
Comment:
**logic:** duplicate method definition - both `downsize` methods will cause the second to override the first, breaking calls that pass 4 arguments (like in `resize_emoji.rb:14`)
```suggestion
def self.downsize(from, to, dimensions_or_width, height_or_opts=nil, opts={})
if height_or_opts.is_a?(Hash)
# Called with dimensions string: downsize(from, to, "80%", opts)
optimize("downsize", from, to, dimensions_or_width, height_or_opts)
else
# Called with width/height: downsize(from, to, 100, 100, opts)
optimize("downsize", from, to, "#{dimensions_or_width}x#{height_or_opts}", opts)
end
end
```
How can I resolve this? If you propose a fix, please make it concise.| # 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: loop doesn't check if downsizing actually reduced file size - if OptimizedImage.downsize fails or file is already compressed, this creates infinite loop until attempts exhausted
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/controllers/uploads_controller.rb
Line: 63:70
Comment:
**logic:** loop doesn't check if downsizing actually reduced file size - if `OptimizedImage.downsize` fails or file is already compressed, this creates infinite loop until attempts exhausted
How can I resolve this? If you propose a fix, please make it concise.| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: passing "80%" as dimensions but downsize method expects either "WIDTHxHEIGHT" format or separate width/height integers - this will likely not work as intended with ImageMagick
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/controllers/uploads_controller.rb
Line: 67:67
Comment:
**logic:** passing "80%" as dimensions but `downsize` method expects either "WIDTHxHEIGHT" format or separate width/height integers - this will likely not work as intended with ImageMagick
How can I resolve this? If you propose a fix, please make it concise.
Test 1
Replicated from ai-code-review-evaluation/discourse-greptile#1