FEATURE: automatically downsize large images#10
Conversation
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
app/controllers/uploads_controller.rb (1)
55-55: Optional: Consider extracting rescue to improve readability.The inline
rescuemodifier works but is flagged by RuboCop. Consider extracting to a separate line for clarity.Apply this diff if desired:
- tempfile = FileHelper.download(url, 10.megabytes, "discourse-upload-#{type}") rescue nil + tempfile = begin + FileHelper.download(url, 10.megabytes, "discourse-upload-#{type}") + rescue + nil + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/assets/javascripts/discourse/lib/utilities.js(2 hunks)app/controllers/uploads_controller.rb(1 hunks)app/models/optimized_image.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/uploads_controller.rb (1)
app/models/optimized_image.rb (2)
downsize(145-147)downsize(149-151)
🪛 Biome (2.1.2)
app/assets/javascripts/discourse/lib/utilities.js
[error] 246-246: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🪛 RuboCop (1.81.7)
app/controllers/uploads_controller.rb
[convention] 55-55: Avoid using rescue in its modifier form.
(Style/RescueModifier)
app/models/optimized_image.rb
[warning] 149-149: Method OptimizedImage.downsize is defined at both app/models/optimized_image.rb:145 and app/models/optimized_image.rb:149.
(Lint/DuplicateMethods)
🔇 Additional comments (2)
app/models/optimized_image.rb (2)
141-143: LGTM: resize correctly formats dimensions string.The
resizemethod properly constructs the dimensions string from width and height parameters and passes it tooptimize. This change aligns with the new API design.
153-158: LGTM: optimize method consistently uses dimensions string.The updated
optimizemethod signature correctly accepts and propagates thedimensionsparameter through the instruction generation flow. The changes are internally consistent.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request introduces a fixed 10 MB upload size limit across client and server validation, implements automatic image downsampling for oversized uploads with retry logic, and refactors OptimizedImage to pass dimensions as a single string parameter instead of separate width/height arguments. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Controller as Upload Controller
participant Image as OptimizedImage
participant File as Tempfile
Client->>Controller: Upload Image
Controller->>File: Validate tempfile exists
alt Image oversized and imageable
loop Retry (up to 5 attempts)
Controller->>Image: downsize(from, to, "WxH")
Image->>File: Reduce with 80% scale
File-->>Image: Downsampled file
Controller->>File: Check size vs max_image_size_kb
alt Size acceptable
break Within limit
end
end
end
end
Controller->>Controller: Create Upload record
Controller-->>Client: Success/Error response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/controllers/uploads_controller.rb (1)
55-55: Consider using block form for rescue.The rescue modifier can make errors harder to debug.
Apply this diff:
- tempfile = FileHelper.download(url, 10.megabytes, "discourse-upload-#{type}") rescue nil + begin + tempfile = FileHelper.download(url, 10.megabytes, "discourse-upload-#{type}") + rescue + tempfile = nil + endapp/assets/javascripts/discourse/lib/utilities.js (1)
182-182: Extract the 10 MB limit to a constant to reduce duplication.The 10 MB limit is hardcoded in two places (lines 182 and 246), making it harder to maintain if this value needs to change.
Apply this diff:
Discourse.Utilities = { + MAX_UPLOAD_SIZE_KB: 10 * 1024, // 10MB translateSize: function(size) {Then update both usages:
// check file size var fileSizeKB = file.size / 1024; - var maxSizeKB = 10 * 1024; // 10MB + var maxSizeKB = Discourse.Utilities.MAX_UPLOAD_SIZE_KB; if (fileSizeKB > maxSizeKB) {// entity too large, usually returned from the web server case 413: - var maxSizeKB = 10 * 1024; // 10 MB + var maxSizeKB = Discourse.Utilities.MAX_UPLOAD_SIZE_KB; bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB }));Also applies to: 246-246
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/assets/javascripts/discourse/lib/utilities.js(2 hunks)app/controllers/uploads_controller.rb(1 hunks)app/models/optimized_image.rb(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
app/assets/javascripts/discourse/lib/utilities.js
[error] 246-246: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🪛 RuboCop (1.81.7)
app/controllers/uploads_controller.rb
[convention] 55-55: Avoid using rescue in its modifier form.
(Style/RescueModifier)
app/models/optimized_image.rb
[warning] 149-149: Method OptimizedImage.downsize is defined at both app/models/optimized_image.rb:145 and app/models/optimized_image.rb:149.
(Lint/DuplicateMethods)
🔇 Additional comments (2)
app/models/optimized_image.rb (2)
141-143: LGTM: Dimension string refactor is consistent.The change to use a dimensions string (
"#{width}x#{height}") instead of separate parameters simplifies the API and is consistent with the new design.
153-158: All callers ofoptimizehave been correctly updated.The signature change from
optimize(operation, from, to, width, height, opts={})tooptimize(operation, from, to, dimensions, opts={})is verified complete:
resize(line 141): constructs dimensions as"#{width}x#{height}"downsize(line 145): constructs dimensions as"#{max_width}x#{max_height}"downsize(line 149): passes dimensions directlyNo external callers exist outside
optimized_image.rb.
| case 413: | ||
| var maxSizeKB = Discourse.SiteSettings.max_image_size_kb; | ||
| var maxSizeKB = 10 * 1024; // 10 MB | ||
| bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB })); | ||
| return; |
There was a problem hiding this comment.
Wrap the variable declaration in a block to prevent access from other switch clauses.
JavaScript case clauses don't create block scope, so the maxSizeKB variable could be accessed by other cases, potentially causing issues.
Based on static analysis.
Apply this diff:
// entity too large, usually returned from the web server
- case 413:
+ case 413: {
var maxSizeKB = 10 * 1024; // 10 MB
bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB }));
return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case 413: | |
| var maxSizeKB = Discourse.SiteSettings.max_image_size_kb; | |
| var maxSizeKB = 10 * 1024; // 10 MB | |
| bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB })); | |
| return; | |
| case 413: { | |
| var maxSizeKB = 10 * 1024; // 10 MB | |
| bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB })); | |
| return; | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 246-246: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In app/assets/javascripts/discourse/lib/utilities.js around lines 245 to 248,
the case 413 clause declares maxSizeKB in function/global scope because case
clauses don’t create block scope; wrap the case body in a block ({ ... }) so
maxSizeKB is scoped to that case only (e.g. replace the raw statements with a
braced block containing the declaration, alert call, and return).
| # 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.
Critical: File size not refreshed after downsampling.
The tempfile.size attribute is cached and won't reflect the new file size after OptimizedImage.downsize modifies the file on disk. This means the while condition will always evaluate based on the original file size, causing the loop to run exactly 5 times regardless of whether downsampling succeeds or reduces the file size.
Additionally, there's no check for whether downsize succeeds (it returns a boolean).
Apply this diff to fix:
# 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)
+ success = OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
+ break unless success
+ # Reload file size from disk after modification
+ tempfile = File.open(tempfile.path)
attempt -= 1
end
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # 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 | |
| # 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 | |
| success = OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails) | |
| break unless success | |
| # Reload file size from disk after modification | |
| tempfile = File.open(tempfile.path) | |
| attempt -= 1 | |
| end | |
| end |
| 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.
🧩 Analysis chain
Critical: Method overloading creates a breaking change.
Ruby does not support method overloading. The second downsize definition at line 149 completely replaces the first one at line 145, meaning the 4-parameter signature downsize(from, to, max_width, max_height, opts={}) no longer exists.
Any existing code calling downsize with 4 positional parameters (width and height) will fail with an ArgumentError.
Based on static analysis.
Run the following script to find all callers of the downsize method and verify they use the correct signature:
Consider either:
- Renaming one of the methods (e.g.,
downsize_by_dimensionsfor the string version) - Using Ruby's flexible argument handling to support both signatures:
def self.downsize(from, to, width_or_dimensions, height_or_opts={}, opts={})
if width_or_dimensions.is_a?(String)
# New API: dimensions string
dimensions = width_or_dimensions
opts = height_or_opts
else
# Old API: separate width/height
dimensions = "#{width_or_dimensions}x#{height_or_opts}"
end
optimize("downsize", from, to, dimensions, opts)
end🏁 Script executed:
#!/bin/bash
# Description: Find all calls to OptimizedImage.downsize to verify compatibility
# Search for downsize method calls
rg -n -C3 -P '\.downsize\s*\(' --type=rbLength of output: 26
Let me retry with a different approach to search for downsize method calls:
🏁 Script executed:
#!/bin/bash
# Find all .rb files and search for downsize calls
fd '\.rb$' -exec rg -l 'downsize' {} \;Length of output: 241
🏁 Script executed:
#!/bin/bash
# Search for downsize method calls with context (no file type filter)
rg -n -C3 '\.downsize\s*\(' --include='*.rb'Length of output: 441
🏁 Script executed:
#!/bin/bash
# Examine the actual optimized_image.rb file to confirm the duplicate definitions
cat app/models/optimized_image.rb | sed -n '140,155p'Length of output: 559
🏁 Script executed:
#!/bin/bash
# Simple search for downsize calls - try basic pattern
rg 'downsize' -nLength of output: 839
🏁 Script executed:
#!/bin/bash
# Search specifically in Ruby files for downsize calls with simpler pattern
find . -name "*.rb" -type f | head -20Length of output: 971
Critical: Duplicate method definition breaks existing caller in resize_emoji.rb.
The second downsize definition (line 149) completely replaces the first (line 145) in Ruby. However, app/jobs/regular/resize_emoji.rb:14 calls OptimizedImage.downsize(path, path, 100, 100, opts) using the 4-parameter signature that no longer exists.
This call will fail with ArgumentError because the remaining definition expects (from, to, dimensions, opts), not (from, to, max_width, max_height, opts).
Fix: Either:
- Rename one method (e.g.,
downsize_by_dimensionsfor the string version) - Use a unified signature that accepts both formats:
def self.downsize(from, to, width_or_dimensions, height_or_opts={}, opts={})
if width_or_dimensions.is_a?(String)
dimensions = width_or_dimensions
opts = height_or_opts
else
dimensions = "#{width_or_dimensions}x#{height_or_opts}"
end
optimize("downsize", from, to, dimensions, opts)
endUpdate both call sites: app/controllers/uploads_controller.rb:67 and app/jobs/regular/resize_emoji.rb:14.
🧰 Tools
🪛 RuboCop (1.81.7)
[warning] 149-149: Method OptimizedImage.downsize is defined at both app/models/optimized_image.rb:145 and app/models/optimized_image.rb:149.
(Lint/DuplicateMethods)
🤖 Prompt for AI Agents
In app/models/optimized_image.rb around lines 145 to 151, there are two
conflicting downsize method definitions causing the 4-argument call in
app/jobs/regular/resize_emoji.rb to raise ArgumentError; replace them with a
single unified downsize that accepts either (from, to, "WxH"/dimensions_string,
opts) or (from, to, width, height, opts) by detecting if the third param is a
String and normalizing to a dimensions string before calling optimize, and then
update the two call sites (app/jobs/regular/resize_emoji.rb:14 and
app/controllers/uploads_controller.rb:67) to use the unified signature (no
behavior change expected) so both string and numeric width/height invocations
work.
2 participants
Test 1
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#1